fix: don't modify msg.data when padding frame for GsUsbFrame - #2085
Open
bjarki-andreasen wants to merge 1 commit into
Open
fix: don't modify msg.data when padding frame for GsUsbFrame#2085bjarki-andreasen wants to merge 1 commit into
bjarki-andreasen wants to merge 1 commit into
Conversation
The current implementation of GsUsbBus send() modifies the msg.data bytearray in place, when padding it up to CAN_MAX_DLC for a GsUsbFrame. This is an issue as the can.Message equals method compares the data fields of messages, so if GsUsbBus send() modifies the data of a can.Message with dlc == 0, the padded data does not match the original message: The passed can.Message should not be modified, only read. This commit creates a new padded bytearray from the msg.data bytearray, leaving the passed can.Message intact. Signed-off-by: Bjarki Arge Andreasen <bjarki.andreasen@nordicsemi.no>
Contributor
|
Tick the box to add this pull request to the merge queue (same as
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The current implementation of GsUsbBus send() modifies the msg.data bytearray in place, when padding it up to CAN_MAX_DLC for a GsUsbFrame. This is an issue as the can.Message equals method compares the data fields of messages, so if GsUsbBus send() modifies the data of a can.Message with dlc == 0, the padded data does not match the original message:
The passed can.Message should not be modified, only read. This commit creates a new padded bytearray from the msg.data bytearray, leaving the passed can.Message intact.
Summary of Changes
Related Issues / Pull Requests
Type of Change
Checklist
tox).Note that tox run showed 47 failed tests when run locally, unrelated to changes in this PR (mostly interfaces like pcan and kvaser). The added test passed.
Additional Notes
Issue was discovered while running this zephyr-rtos test suite https://github.com/zephyrproject-rtos/zephyr/tree/main/tests/drivers/can/host using a cannectivity based probe, which implements the gs_usb interface. The following error was observed:
which was quite puzling given it "looks" identical because the data field is not printed if DLC == 0. debugging showed the mismatching data members, which failed the can.Message equals check, as this one checks data == data even when DLC == 0
There is no explicit documentation stating that the can.Message should not be modified, it can only be inferred from what I can see, maybe we should document this if it is indeed expected to not mutate the can.Message objects post creation? A small step in that direction could be to make the data a bytes object rather than bytearray, then gs_usb.py could not have called .extend() in the first place.