CAN ISO-TP updates to allow padding and fixed addressing

[martinjaeger]

This PR fixes some small issues and adds two new features to the ISO-TP implementation:

Allow Padding in SF, FF and last CF

Some protocols including OBD require that CAN messages always have a length of 8 bytes, which requires padding in the ISO-TP implementation.

Similar to AUTOSAR requirements, I added two build-time configuration flags that allow to activate padding in received and sent frames. If padding is activated for RX, a frame with length < 8 leads to an error, as specified in AUTOSAR. Otherwise, both types of frames are now accepted (default setting).

Padding is mandatory for CAN-FD, as the frame length cannot be adjusted to perfectly match the actual data. However, proper CAN-FD support will require some further modifications which are not part of this PR.

Add support for fixed addressing

ISO 15765-2 specifies how to use ISO-TP for CAN ID layouts according to SAE J1939 and NMEA-2000, where a source (N_SA) and target address (N_TA) is encoded inside the ID (called "fixed addressing"):

 Format of 29-bit CAN identifier:
 ------------------------------------------------------
 | 28 .. 26 | 25  | 24 | 23 .. 16 | 15 .. 8  | 7 .. 0 |
 ------------------------------------------------------
 | Priority | EDP | DP | N_TAtype |   N_TA   |  N_SA  |
 ------------------------------------------------------

In order to support this message type, a CAN filter mask that ignores the source address and the priority during reception was added. In addition to that, FC frames are sent back to the correct source address with the same priority, which requires an automatic modification of the TX CAN ID.

[alexanderwachter]

Thanks for the fix and extension!

[alexanderwachter]

Can you add a test for the fixed addressing feature?

[martinjaeger]

Can you add a test for the fixed addressing feature?

Yep, can do that.

Just identified a length calculation error in the padding implementation in process_ff_sf while doing further tests with Linux can-utils, so I will also provide a small update for that.

[alexanderwachter]

Would it make sense to add a fag to the address struct for padding instead of compile time setting?

[martinjaeger]

Would it make sense to add a fag to the address struct for padding instead of compile time setting?

AUTOSAR spec chapter 7.3.8 suggests to select it at compile-time. Also this library which I use in an ESP32 has a compile-time flag (with padding enabled by default, though). I think it's fine to select it at compile-time depending on the higher-level protocol in use and save some memory if we don't need the checks and can accept padded and non-padded frames.

[martinjaeger]

Added new unit-tests for the fixed addressing feature and updated existing tests to work with and without padding enabled.

tests/subsys/canbus/isotp/conformance passes on my STM32G431-based custom board (with the new CAN-FD driver PR already applied). However, tests/subsys/canbus/isotp/implementation fails even without the commits added by this PR. @alexanderwachter can you please double check in your test setup to see if it is related to my hardware?

[alexanderwachter]

Beside my comment, LGTM.

[martinjaeger]

Beside my comment, LGTM.

Cool. Which comment do you mean? I think I have addressed both of your comments. The tests have been added and I stated why I suggest to keep the padding switch as a compile-time option.

The unit test which fails on some platforms is not related to this patch and seems to be a more generic issue, see also #31149 and #31150.

[alexanderwachter]

Why do you remove this check? According to the standard, the length in the frame must match if there is no padding, IIRC

[alexanderwachter]

@martinjaeger any thoughts here?

[martinjaeger]

I can't see any statement that it must match. Some quotes from ISO 15765-2:2004:

6.5.2.2 (SF_DL error handling)

If the network layer receives an SF with an SF_DL greater than 7 when using normal addressing, or greater than 6 for extended or mixed addressing, then the network layer shall ignore the received SF N_PDU.

7.4.3 CAN frame data optimization

[...] The DLC parameter of the CAN frame is set by the sender and read by the receiver to determine the number of data bytes per CAN frame to be processed by the network layer. The DLC parameter cannot be used to determine the message length; this information shall be extracted from the N_PCI information in the beginning of a message.

7.4.4 Data Length Code error handling

[...] The reception of a CAN frame with a DLC value smaller than expected [...] shall be ignored by the network layer without any further action.

So in my understanding we should ignore any bytes beyond ISOTP_PCI_SF_DL, i.e. allow padded and non-padded messages. It is not clearly stated what to do with a message that is padded to less than 8 bytes.

However, we might still want to check if the DLC is too small to fit the message with the length calculated based on ISOTP_PCI_SF_DL. I can add that check. What do you think?

[alexanderwachter]

That makes sense!

[martinjaeger]

Done. And rebased.

[alexanderwachter]

Sry, forgot to submit them (it was in pending sine two weeks) 🤦‍♂️

[martinjaeger]

@carlescufi and @galak: Unfortunately we don't find additional reviewers for this PR as it covers a very specific topic. We've asked for additional reviews on slack multiple times already.

@legoabram has worked with ISO-TP and is already working on a PR to port it to CAN-FD. Can we accept his review as a second approval even though he is not (yet) an official collaborator?