CAN: Fix CRC field decoding for CAN-FD#131
Open
stephan-thiele wants to merge 7 commits intosigrokproject:masterfrom
Open
CAN: Fix CRC field decoding for CAN-FD#131stephan-thiele wants to merge 7 commits intosigrokproject:masterfrom
stephan-thiele wants to merge 7 commits intosigrokproject:masterfrom
Conversation
0ba3408 to
96c4e38
Compare
Contributor
Author
|
Removed a redundant variable assignment via rebase and force-pushed to avoid a fixup commit, as the review had not started yet. |
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
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.
Currently in CAN decoder, CRC field of CAN-FD is decoded in a wrong way. On classic CAN, CRC-15 field occurs directly after data field. On CAN-FD however, after data field, an SBC field (stuff bit count, modulo 8, gray coded with parity bit) and a CRC-17 or CRC-21 (depending on data length) occur. Additionally, CRC sequence on CAN-FD does not use NRZ bit stuffing but a fixed bit stuffing instead. Currently on CAN-FD, the SBC field and the stuff bits are part of the CRC value which is incorrect.
To fix this, in this PR the following steps are done:
Behaviour of the decoder changes from:
To:
Dump used shown in the images is: https://github.com/sigrokproject/sigrok-dumps/blob/master/can/can_fd/arbitrary_traffic/can_fd_std_brs_8.sr
I have made a regression test with sigrok-test by running
pdtest -r -v can. Tests of classic CAN dumps pass, except nmea2000_fuel_flow_gps_snippet. To me it looks like the sample rate of the recording is too low which leads to invalid values:There are only very few samples per bit and the decoder also annotates warnings on some frames. I'd suggest to remove it entirely as, in my opinion, it is not very useful for regression tests in this state.
CAN-FD tests fail as expected in CRC field area.
I have validated some decoded CRC values for CRC-17 and CRC-21 by using PEAK CAN FD Frame Analyzer tool: https://www.peak-system.com/products/software/tools/can-fd-frame-analyzer/
I will do another PR in sigrok-test repo with updated test vectors and link it here once it is created.Edit: PR with the updated test vectors is now created: sigrokproject/sigrok-test#29