Skip to content
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

Add Support for CAN FD #21

Closed
JWhitleyWork opened this issue Oct 5, 2022 · 6 comments · Fixed by #28
Closed

Add Support for CAN FD #21

JWhitleyWork opened this issue Oct 5, 2022 · 6 comments · Fixed by #28

Comments

@JWhitleyWork
Copy link
Collaborator

CAN FD is becoming common-place in new vehicles and my company also needs a driver which supports it. The Linux kernel/socketcan has supported it for some time. However, the can_msgs/msg/Frame message does not support DLCs above 8 or payloads above 8 bytes and, unfortunately, the maintainers of ros_canopen do not seem interested in supporting CAN FD (see ros-industrial/ros_canopen#375) or ROS2 (see ros-industrial/ros_canopen#322). This necessitates that a new message be created for this purpose. There are basically two options:

  1. Create a new message which supports both traditional CAN and CAN FD.
  2. Use can_msgs/msg/Frame for traditional CAN and a new message for CAN FD.

Option 1 introduces an API break for all users of this repository if the node is switched to only using this new message. As such, it is not advisable to back-port this change to older ROS versions like Foxy. Option 2 splinters support and makes it so that CAN and CAN FD messages could not be published on the same topic - even though they can exist on the same bus, per the CAN FD spec. This is also not ideal.

As an alternative, I offer a "re-thinking" of option 1: Adding a new message which supports both traditional CAN and CAN FD but making the use of this message optional in the node with a parameter - something like support_fd. This flag would then enable publishing/subscribing using the new message while the default behavior would be to publish/subscribe using the old message.

I would like to hear feedback from @mitsudome-r, @wep21, or others active in the Autoware and ROS2 communities regarding the above options. Thanks!

@JWhitleyWork
Copy link
Collaborator Author

@kenji-miyake / @wep21 / @xmfcx Please review #28 if you can. I have not tested this on a real CAN FD device yet but I am working on it which is why it's still in Draft. It deviates slightly from my description above because it implements the FD frames as new topics in addition to the standard CAN messages. I need to see if SocketCAN requires that only FD read calls be made to FD sockets and standard calls to standard sockets.

@kenji-miyake
Copy link
Contributor

@JWhitleyWork Thank you for working on this cool feature!

I'm sorry I'm not familiar with CAN FD, so it's difficult to review it in detail. 😢
But I've asked my colleagues to review it if they know CAN FD well.

Regarding merge, if you are done with your work, and if it doesn't break existing behaviors, I think it's okay to merge it. Before that, I think we can help testing with real vehicles.

@RFRIEDM-Trimble
Copy link

Hey there,

How would this work with ISO11783 transport layer with array length up to 1785 bytes? Or, even larger, such as for extended transport protocol?

Before this is merged and there is an established ABI in usage, seems like a good time to double check the potential uses. Since SocketCan supports J1939, seems logical to ask here.

I would be floored if I could use this library eventually to do things like firmware updates for my J1939 devices, and want to propose some additional thought on the max payload length allowed in consideration for other standard J1939 uses.

@JWhitleyWork
Copy link
Collaborator Author

@RFRIEDM-Trimble This repository currently only implements raw CAN transmission/receiption. While you are correct that J1939 is supported in SocketCAN, it would not be related to this PR as CAN FD is a different protocol that is implemented on a CAN_RAW socket. To enable J1939 features, the socket must be configured with CAN_J1939. We would be happy to review a PR which adds the ability to create/configure a SocketCAN socket with J1939 support but I don't personally have the hardware to test/debug this feature.

@JWhitleyWork
Copy link
Collaborator Author

@kenji-miyake Thanks for the support! Based on some help given by @FranzAlbers, I have had to make some changes to my PR which include a subtle API break. See my comment here for details and let me know what you think.

@kenji-miyake
Copy link
Contributor

@JWhitleyWork Thank you for letting me know. For code-level API breaks, we don't care so much. If launch-level API breaks, we might need to discuss. (But it seems to be okay for now.)
Regarding the detailed design, unfortunately I don't have enough knowledge to add an appropriate comment. I'm sorry for that...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants