Skip to content

Conversation

@henrikbrixandersen
Copy link
Member

@henrikbrixandersen henrikbrixandersen commented Oct 18, 2022

The can_frame and can_filter structs support a number of different flags (standard/extended CAN ID type, Remote Transmission Request, CAN-FD format, Bit Rate Switch, ...). Each of these flags is represented as a discrete bit in the given structure.

This design pattern requires every user of these structs to initialize all of these flags to either 0 or 1, which does not scale well for future flag additions.

Some of these flags have associated enumerations to be used for assignment, some do not. CAN drivers and protocols tend to rely on the logical value of the flag instead of using the enumeration, leading to a very fragile API. The enumerations are used inconsistently between the can_frame and can_filter structures, which further complicates the API.

The CAN_EXTENDED_IDENTIFIER and CAN_STANDARD_IDENTIFIER definitions from the CAN controller driver API are reused directly for ISO-TP structure members, which leads to a very fragile design of this API. The ISO-TP layer must be responsible for its own definitions where needed. This PR replaces the "id_type" ISO-TP struct member with a well-known abbreviated "ide" bit (Identifier Extension Bit) struct member.

The discrete flags are converted to bitfields with separate flag definitions for the can_frame and can_filter structures. This API allows for future extensions without having to revisit existing users of the two structures. Furthermore, this allows driver to easily check for unsupported flags in the respective API calls.

As this change leads to the "id_mask" field of the can_filter to be the only mask present in that structure, rename it to "mask" for simplicity.

With the above changes in place, we can add test case for verifying that CAN-FD format frames cannot be sent in non-FD mode, which fixes #50776.

Avoid reusing the CAN_EXTENDED_IDENTIFIER and CAN_STANDARD_IDENTIFIER
definitions from the CAN controller driver API for ISO-TP structure members
as this is a fragile design.

The ISO-TP layer must be responsible for its own definitions where
needed. Replace the "id_type" ISO-TP struct member with a well-known
abbreviated "ide" bit (Identifier Extension Bit) struct member.

Signed-off-by: Henrik Brix Andersen <[email protected]>
@henrikbrixandersen henrikbrixandersen force-pushed the can_frame_flags branch 6 times, most recently from d713447 to 67c9895 Compare October 18, 2022 10:41
@henrikbrixandersen henrikbrixandersen added the bug The issue is a bug, or the PR is fixing a bug label Oct 18, 2022
@henrikbrixandersen
Copy link
Member Author

henrikbrixandersen commented Oct 18, 2022

The main driver behind the change from discrete struct members to using a flag bitfields was the discoveries made by testing the proposed change in #50769. This revealed that older CAN code (written before the introduction of CAN-FD support in Zephyr) did not explicitly set the value of the fd and brs members in struct can_frame before calling can_send() (since these struct members where not present when the code was written). These fields were thus uninitialized and could take on either a value of 0 or 1, leading to failures once the CAN driver started to actually check those fields properly.

Moving to a single flags field will allow for future extensions without similar breakage.

@henrikbrixandersen henrikbrixandersen force-pushed the can_frame_flags branch 3 times, most recently from e9c3e5d to 4a0f783 Compare October 18, 2022 18:08
The can_frame and can_filter structs support a number of different flags
(standard/extended CAN ID type, Remote Transmission Request, CAN-FD format,
Bit Rate Switch, ...). Each of these flags is represented as a discrete bit
in the given structure.

This design pattern requires every user of these structs to initialize all
of these flags to either 0 or 1, which does not scale well for future flag
additions.

Some of these flags have associated enumerations to be used for assignment,
some do not. CAN drivers and protocols tend to rely on the logical value of
the flag instead of using the enumeration, leading to a very fragile
API. The enumerations are used inconsistently between the can_frame and
can_filter structures, which further complicates the API.

Instead, convert these flags to bitfields with separate flag definitions
for the can_frame and can_filter structures. This API allows for future
extensions without having to revisit existing users of the two
structures. Furthermore, this allows driver to easily check for unsupported
flags in the respective API calls.

As this change leads to the "id_mask" field of the can_filter to be the
only mask present in that structure, rename it to "mask" for simplicity.

Fixes: zephyrproject-rtos#50776

Signed-off-by: Henrik Brix Andersen <[email protected]>
Add test case for verifying that CAN-FD format frames cannot be sent in
non-FD mode.

Signed-off-by: Henrik Brix Andersen <[email protected]>
Set the CAN controller to CAN-FD mode before attempting to send FD format
frames.

Signed-off-by: Henrik Brix Andersen <[email protected]>
@carlescufi carlescufi merged commit 30c7d31 into zephyrproject-rtos:main Oct 25, 2022
@henrikbrixandersen henrikbrixandersen deleted the can_frame_flags branch October 26, 2022 09:10
martinjaeger added a commit to martinjaeger/zephyr that referenced this pull request Feb 15, 2023
The member variable was renamed from id_mask to mask in zephyrproject-rtos#51361, but
the docs were not adjusted accordingly.

Signed-off-by: Martin Jäger <[email protected]>
stephanosio pushed a commit that referenced this pull request Feb 16, 2023
The member variable was renamed from id_mask to mask in #51361, but
the docs were not adjusted accordingly.

Signed-off-by: Martin Jäger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: CAN area: native port Host native arch port (native_sim) area: Networking area: Sockets Networking sockets bug The issue is a bug, or the PR is fixing a bug manifest-canopennode platform: NXP NXP platform: Renesas R-Car Renesas R-Car platform: STM32 ST Micro STM32

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CAN Drivers allow sending FD frames without device being set to FD mode

4 participants