add 0x19 ReadDTCInformation

[ParaZera]

The PR should add support for the 0x19 ReadDTCInformation function

• addded a handler for the 0x19 function
• added events and args
  • I opted to create a union struct for the subfunction specific arguments to simplify handling in the handler-function
• added tests for happy path using the examples from the standard

I created one commit for each subfunction with some refactoring commits in between to help with the review.

[ParaZera]

I just pushed and update of the iso14229.c/h files.

I generated them using a different bazel and gcc version. (See attached patch)

0001-change-bazel-and-gcc-version.patch

[codecov]

Codecov Report

❌ Patch coverage is 61.30952% with 65 lines in your changes missing coverage. Please review.
✅ Project coverage is 33.22%. Comparing base (1ed564a) to head (f75394c).

Files with missing lines	Patch %	Lines
iso14229.c	61.30%	30 Missing and 35 partials ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main      #86       +/-   ##
===========================================
- Coverage   57.29%   33.22%   -24.07%     
===========================================
  Files           6        2        -4     
  Lines        3004     2251      -753     
  Branches      609      470      -139     
===========================================
- Hits         1721      748      -973     
- Misses        903     1333      +430     
+ Partials      380      170      -210     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[driftregion]

Thanks for this impressive PR @ParaZera . Please allow me some time to familiarize myself with the DTC submodule.

[ParaZera]

Thanks for this impressive PR @ParaZera . Please allow me some time to familiarize myself with the DTC submodule.

Sure, I know it's a huge PR!

I took the liberty of adding myself to the AUTHORS.txt file, since you offered this to my colleague as well. I hope that’s okay?!

[ParaZera]

Just a heads up for your review:

In my code I opted to do a length check for the responses from the event callback to ensure compliance with the standard. But I am unsure on how to correctly propagate a length error to the client since no valid NRC's seem to fit this usecase.

Currently I respond wit the raw data from the callback (even if it is the wrong size), as it is also done in this way in the Handle_0x23_ReadMemoryByAddress function.

Other solutions I came up with are not responding at all and let the client run into a timeout or responding with NRC 0x10 (General Reject).

If you have a suggestion or an opinion on what would be the correct way to handle this, I'll update the code accordingly.

[driftregion]

Just a heads up for your review:

In my code I opted to do a length check for the responses from the event callback to ensure compliance with the standard. But I am unsure on how to correctly propagate a length error to the client since no valid NRC's seem to fit this usecase.

Currently I respond wit the raw data from the callback (even if it is the wrong size), as it is also done in this way in the Handle_0x23_ReadMemoryByAddress function.

I see. This needs updating. My intent here is return NegativeResponse(r, ...) -- not returning the raw data. A length mismatch indicates server programmer error, in which case it makes the most sense to return no data other than the NRC.

iso14229/src/server.c

Line 292 in 4b4100b

return UDS_NRC_GeneralProgrammingFailure;

Other solutions I came up with are not responding at all and let the client run into a timeout or responding with NRC 0x10 (General Reject).

If you have a suggestion or an opinion on what would be the correct way to handle this, I'll update the code accordingly.

I think sending a UDS_NRC_GeneralReject is a good approach for server user implementation issues. Some history: I misinterpreted the meaning of UDS_NRC_GeneralProgrammingFailure. Its use in the other services is definitely inconsistent with the standard and needs updating to UDS_NRC_GeneralReject.

In summary, for all server user implementation issues, my preferred approach is:

1. Complain loudly with UDS_LOGE -- this allows the programmer to detect issues during bringup.
2. Return UDS_NRC_GeneralReject -- this correctly reflects that there was a server error.

[driftregion]

sorry about that -- sonarqube is not yet configured properly.

[ParaZera]

In summary, for all server user implementation issues, my preferred approach is:

1. Complain loudly with UDS_LOGE -- this allows the programmer to detect issues during bringup.
2. Return UDS_NRC_GeneralReject -- this correctly reflects that there was a server error.

I updated the code and added tests for malformed responses and unknown subFunctions. I created one tests that iterates over all subfunctions and tests the correct NRC response. This has the advantage that there is less code duplication but also stops at the first error, so subsequent failures are not explicitly detected.

[ParaZera]

I'm currently a little bit at a loss regarding the windows CI.

I fixed the previous error by giving the union holding the subfunction arguments a name:

typedef struct {
    const uint8_t type; /*! invoked subfunction */
    uint8_t (*copy)(UDSServer_t *srv, const void *src,
                    uint16_t count); /*! function for copying data */

    union {
        struct {
            uint8_t mask; /*! DTC status mask */
        } numOfDTCByStatusMaskArgs, dtcStatusByMaskArgs;
       // ...
        struct {
            uint8_t functionalGroup; /*! Functional Group Identifier */
            uint8_t
                readinessGroup; /*! DTC Readiness Group Identifier (only used when type == 0x56) */
        } wwhobdDTCWithPermStatusArgs, dtcInfoByDTCReadinessGroupIdArgs;
    } subFuncArgs;
} UDSRDTCIArgs_t;

But now the job complains about:

test/test_server.c(3238): warning C4133: '=': incompatible types - from 'int (__cdecl *)(UDSServer_t *,UDSEvent_t,void *)' to 'UDSErr_t (__cdecl *)(UDSServer *,UDSEvent_t,void *)'
C:\users\runneradmin\_bazel_runneradmin\ppqlvi7y\execroot\_main\test\test_server.c(542) : fatal error C1001: Internal compiler error.
(compiler file 'D:\a\_work\1\s\src\vctools\Compiler\Utc\src\p2\main.cpp', line 258)
 To work around this problem, try simplifying or changing the program near the locations listed above.
If possible please provide a repro here: https://developercommunity.visualstudio.com/ 
Please choose the Technical Support command on the Visual C++ 
 Help menu, or open the Technical Support help file for more information
[52 / 53] 3 / 8 tests, 1 failed; checking cached actions
INFO: Elapsed time: 66.816s, Critical Path: 12.47s
INFO: 52 processes: 28 internal, 24 local.
ERROR: Build did NOT complete successfully

Where line 542 in the test_server.c file is:

void test_0x19_sub_0x03_no_snapshots(void **state) {

Sadly, I can't really solve this problem locally as I seldom work on windows and don't know about its quirks. Do you have an idea why the compiler complains here?

[driftregion]

I can confirm that the same compiler error occurs on my Windows machine. I can also see the same warning here: https://godbolt.org/z/79ds1dYWc

I can make that warning go away by writing

Test0x19FnData_t fn_data = {.data = NULL, .len = 0};

instead of

uint8_t ResponseData[] = {};
Test0x19FnData_t fn_data = {.data = ResponseData, .len = sizeof(ResponseData)};

[sonarqubecloud]

Quality Gate failed

Failed conditions
7 Security Hotspots
C Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

[driftregion]

Looks good. Nice work.