Bug 1085

Summary:	Incorrect stuff bit handling
Product:	libsigrokdecode	Reporter:	Soeren Apel <soeren>
Component:	PD: can	Assignee:	Nobody <nobody>
Status:	RESOLVED FIXED
Severity:	normal	CC:	spamMayGoHere, uwe
Priority:	Normal
Version:	unreleased development snapshot
Target Milestone:	---
Hardware:	All
OS:	All
Attachments:	Failure case 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)).) A stuff bit is also inserted after 00000 at the end of the CRC segment

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