-
Notifications
You must be signed in to change notification settings - Fork 271
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Can FD support #375
base: melodic-devel
Are you sure you want to change the base?
Can FD support #375
Conversation
d555f76
to
0cbc512
Compare
First of all: Great work! I haven't used CAN FD yet. Would your code still work with a CAN FD dongle and classic CAN slave?
As an alternative, we could introduce |
A CAN FD dongle with a classic CAN slave works The difference from an electrical perspective is that the r0-bit in the classical CAN header is always 0, and for FD frames it is 1 which alters the rest of the header. From a socketcan perspective, a socket with the CAN_RAW_FD_FRAMES option set will cause reads from the socket to return 72 bytes of canfd_frame instead of 16 bytes can_frame. The canfd_frame is backwards compatible with can_frame so user code is compatible with older kernels that doesn't support CAN FD, read more at https://www.kernel.org/doc/Documentation/networking/can.txt and https://www.kvaser.com/wp-content/uploads/2016/10/comparing-can-fd-with-classical-can.pdf. I wanted to preserve this backwards compatibility, but I realize that it's not possible in this case. Introducing a new message might be a good idea, it will still break user code publishing/subscribing to sent_messages and received_messages though, if it is used by default by the bridge. We could add a conversion node that converts from one message type to the other as a remedy...? |
Breaks compat with can_msgs/Frame due to changed array type
0cbc512
to
ca99abe
Compare
If this is required, it should happen at some point. Maybe it would be good to propose this already for ROS2 rolling. This distribution allows breaking changes and gets a discussion going. It could be part of the next ROS2 Humble release |
It's not strictly necessary, there is a possibility to add a separate data type for FD frames. I would propose to not go that way though, since it will become a mess for the user always having to deal with two data types depending on what to send. This is something that should be hidden from the user at this application level imo. So, having this break between ROS and ROS2 seems like a perfect opportunity. |
So, this is a bold pull request, sine it breaks compatibility with can_msgs/Frame due to changed array type. Initially I thought of using the MSB of can_msgs/Frame.dlc to indicate a bit rate switch to avoid changing the message at all, but I soon realized that the 8 bytes was hard coded. It will break both compatibility with old bags and users will need to reserve memory themselves for their data. At the same time I don't want to add a new message for FD frames, which would be the only option to preserve compatibility, because this would remove much of the transparency between CAN and CAN FD.
What do you think of this?