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

fix(socket_can_receiver): support variable-length CAN FD frames #50

Merged
merged 5 commits into from
Sep 25, 2024

Conversation

xmfcx
Copy link
Contributor

@xmfcx xmfcx commented Jul 18, 2024

From @knzo25 's branch in https://github.com/knzo25/ros2_socketcan/commits/feat/continental_fd/

Related PR:

Summary

This PR fixes an issue in the SocketCanReceiver class where it incorrectly assumed that all CAN FD frames would be the maximum size (64 bytes). This assumption caused errors when handling shorter frames, as observed with SRR520 frames.

The fix ensures that the receive_fd method can properly handle variable-length CAN FD frames by dynamically calculating the expected frame size based on the actual data length (frame.len). It also introduces additional safety checks to prevent corrupted or incomplete frames from being processed.

Changes

  • Adjusted the logic in receive_fd to handle CAN FD frames shorter than 64 bytes.
  • Added a calculation for expected_length based on the fixed header size and the variable data length.
  • Improved error handling to ensure that corrupted or incomplete frames are rejected.

Additional Improvements

  • Added a check to ensure that the frame length (frame.len) does not exceed the CAN FD maximum allowed data length (64 bytes).
  • Refactored the error handling to provide more detailed messages in cases of failure.

Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>
Copy link
Contributor

@mojomex mojomex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the above suggestion

@knzo25
Copy link
Collaborator

knzo25 commented Sep 19, 2024

@mojomex
Sorry but can you check it this commit addresses your concern?

Copy link
Contributor

@mojomex mojomex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@mojomex mojomex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests are failing because CPPlint enforces <= 100 character lines with at least 2 spaces between code and comments.

ros2_socketcan/src/socket_can_receiver.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@mojomex mojomex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@mojomex
Copy link
Contributor

mojomex commented Sep 19, 2024

@xmfcx it seems like I don't have write access in this repo, so I don't count towards the required reviews. Could you review/merge it?

@xmfcx
Copy link
Contributor Author

xmfcx commented Sep 19, 2024

This PR makes significant changes in how the can frames are parsed.

Was this tested on real hardware?

@knzo25
Copy link
Collaborator

knzo25 commented Sep 20, 2024

@xmfcx
Previously to this I had zero experience with can fd so that is why I never sent a PR to the upstream.
This code has been tested using Continental's SRR520, but we do not have any other devices to test.

As explained in the code, the problem is that the original implementation seemed to only accept fd frames of the max size (64 bytes).
SRR520 has some fd frames that are quite shorter, making the main branch complain.

@xmfcx xmfcx changed the title feat: changes for continental srr520 fix(socket_can_receiver): support variable-length CAN FD frames Sep 23, 2024
@xmfcx xmfcx merged commit 85da8c3 into main Sep 25, 2024
6 checks passed
@xmfcx xmfcx deleted the feat/continental_fd branch September 25, 2024 11:24
@mitsudome-r mitsudome-r restored the feat/continental_fd branch October 17, 2024 06:21
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 this pull request may close these issues.

3 participants