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 calculation of ISO-TP SF size field length #56

Merged
merged 1 commit into from
Jan 7, 2022
Merged

Fix calculation of ISO-TP SF size field length #56

merged 1 commit into from
Jan 7, 2022

Conversation

lumagi
Copy link

@lumagi lumagi commented Jan 7, 2022

Given the following setup:

  • CAN-FD bus
  • tx_data_length = 64
  • extended addressing

When sending a 7 byte payload, the full ISO-TP frame with size and address fields would have a minimum size of 7 (data) + 1 (addr) + 1 (header + size field) = 9. According to the standard, when receiving a CAN frame with a data length of >8 byte, the size field must have a length of 12 bit and the first four bits must be equal to 0. However, the current implementation does not correctly calculate the length of the size field and uses only four bits.

In the patch, I updated the size field length calculation to what I would assume to be correct. I also added a unit test to verify the correct behavior.

Given the following setup:
* CAN-FD bus
* tx_data_length = 64
* extended addressing

When sending a 7 byte payload, the full ISO-TP frame with size and address fields
would have a minimum size of 7 + 1 (addr) + 1 (size) = 9. According to the standard,
such a payload must use a two-byte size field. However, the current implementation
does not correctly calculate the length of the size field.

This commit fixes the calculation and introduces a unit test to verify the correct
behavior.
@pylessard
Copy link
Owner

Thank you very much for this. That's a good catch.

high quality pull request. Merging right away.

@pylessard pylessard merged commit 8acdaeb into pylessard:master Jan 7, 2022
@lumagi lumagi deleted the bugfix/ext_addr_frame_size branch January 9, 2022 20:08
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.

2 participants