Bug 1085 - Incorrect stuff bit handling
Summary: Incorrect stuff bit handling
Status: RESOLVED FIXED
Alias: None
Product: libsigrokdecode
Classification: Unclassified
Component: PD: can (show other bugs)
Version: unreleased development snapshot
Hardware: All All
: Normal normal
Target Milestone: ---
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-12-18 19:28 CET by Soeren Apel
Modified: 2018-04-12 15:27 CEST (History)
2 users (show)



Attachments
Failure case (486 bytes, application/zip)
2017-12-18 19:28 CET, Soeren Apel
Details
Another sample data file where the CAN protocol decoder fails by assigning a dominant (0) stuff bit after 11111 in the CRC segment to the CRC delimiter (that is always recessive (1)).) (501 bytes, application/zip)
2017-12-19 17:16 CET, Peter Mortensen
Details
A stuff bit is also inserted after 00000 at the end of the CRC segment (518 bytes, application/zip)
2017-12-19 18:15 CET, Peter Mortensen
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Soeren Apel 2017-12-18 19:28:10 CET
Created attachment 359 [details]
Failure case

As reported by celeron55 on IRC:

> the decoder currently thinks a CRC cannot end with a stuffed bit
> but it actually can, and the crc delimiter then comes after the stuffed bit

Suggested change:
https://gist.github.com/celeron55/a951799517850dfec9a793b5247fa046
Comment 1 Peter Mortensen 2017-12-19 17:16:31 CET
Created attachment 360 [details]
Another sample data file where the CAN protocol decoder fails by assigning a dominant (0) stuff bit after 11111 in the CRC segment to the CRC delimiter (that is always recessive (1)).)

The bit rate is 250 kbit/s. It was recorded by an oscilloscope (analog data) at 5 MHz sample rate (nominally exactly 20 samples per bit time) and converted to logic format by a script for CSV import into PulveView and saved from PulveView. 

The (recorded) CAN message is complete, without error (though the bit timings are somewhat off in places - but presumed to be within the CAN specification).

The significant signal is "CAN RX" - the other two are for info only.
Comment 2 Peter Mortensen 2017-12-19 18:14:13 CET
Empirically, a stuff bit is also inserted after 00000 at the end of the 15 bit CRC segment, even though it is not necessary as the CRC delimiter bit is always 1.

See the third attachment.
Comment 3 Peter Mortensen 2017-12-19 18:15:29 CET
Created attachment 361 [details]
A stuff bit is also inserted after 00000 at the end of the CRC segment
Comment 4 Peter Mortensen 2017-12-19 18:17:15 CET
Comment on attachment 361 [details]
A stuff bit is also inserted after 00000 at the end of the CRC segment

The bit rate is 250 kbit/s.
Comment 5 Peter Mortensen 2018-04-11 22:12:19 CEST
In December 2017 I recorded 22483 oscilloscope traces on a real CAN bus system with about 20 CAN devices for the purpose of evaluating the fix - input data for sigrok (after processing of the analog data) is in http://pmortensen.eu/temp2/22483_CSVsForSigrok.7z.zip

The result of running the data through both the existing CAN decoder and the proposed new one is in: http://pmortensen.eu/temp2/Deep_compare_of_old_and_new_CAN_decoder.zi

The fix correctly decodes in 549 cases where the existing protocol decoder falsely reports "CRC delimiter must be a recessive bit" and/or "ACK delimiter must be a recessive bit". The two decoders produces identical output for the remaining 21934 traces.

There are no CAN error frames in the 22483 - thus there is high confidence in the integrity of the CAN messages (they all have the correct CRC value, don't have any bit errors or frame errors, etc.)
Comment 6 Uwe Hermann 2018-04-12 13:45:19 CEST
Merged in cffb6592f4cff804745b8456e2c9f8abc6571603, thanks for the patch and testing!
Comment 7 Peter Mortensen 2018-04-12 15:27:30 CEST
Correction: The second URL in the the comment 2 up is http://pmortensen.eu/temp2/Deep_compare_of_old_and_new_CAN_decoder.zip