Fix extraction of Motorola (big endian) signals - #27
Open
RyotaroIsoyama wants to merge 1 commit into
Open
Conversation
DbcParser converts the DBC start bit of a big endian signal into an
MSB-first bit index counted from the first transmitted bit:
normalized = (byte_index * 8) + (7 - bit_in_byte)
extractRawSignal() then used that value as a little endian (LSB0) shift
amount, which mirrors the bit positions inside the byte: bit 7-n is read
where bit n was meant. The `length > 8` byte swap happens to repair the
result for signals that start at the MSB of a byte and are a whole
number of bytes long, so byte aligned signals decode correctly and the
problem only shows up on sub-byte fields.
Concretely, for `SG_ AC_Request : 7|1@0+` with byte0 = 0xC0 the LSB of
byte0 was returned instead of the MSB, and for
`SG_ Reserved_1 : 5|6@0+` the reserved field returned 48 (= 0xC0 >> 2)
instead of 0. Status bits and flags therefore read as if they were
mirrored, while speeds, voltages and currents looked fine.
Extract big endian signals directly from the payload read as one big
endian value instead, which needs no byte swap and no special case for
the length. Little endian extraction is unchanged.
Also restore the bounds check that was commented out, which stops
signals that lie outside the DLC from returning bits of adjacent or
stale bytes.
Verified against two recorded logs from a vehicle HVAC bus (11 big
endian signals plus little endian regression cases); see PR description
for the value table.
Fixes normaldotcom#2
Fixes normaldotcom#3
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Fixes #2 (single-bit fields only work when set to Intel) and #3 (Motorola fields broken for messages shorter than 8 bytes).
The problem
DbcParser::parseSectionBoSg()converts the DBC start bit of a big endian signal into an MSB-first bit index counted from the first transmitted bit:That conversion is correct. But
CanMessage::extractRawSignal()then uses the result as a little endian (LSB0) shift amount:The two are different coordinate systems, so the bit positions get mirrored inside the byte — bit
7-nis read where bitnwas meant.The
length > 8byte swap accidentally repairs this for signals that start at the MSB of a byte and are a whole number of bytes long (then the normalized start bit is a multiple of 8 and the shift lands on the right bytes). 8-bit signals need no swap and are also fine. So byte aligned signals decode correctly and only sub-byte fields are wrong, which makes this easy to miss: physical values look right while status bits and flags are silently mirrored.Example
From a vehicle HVAC bus,
AC_530hwith payloadC0 00 00 0B B8 1E 00 00:AC_Request7|1@0+RearDefrost6|1@0+Reserved_15|6@0+0xC0 >> 2)AC_speed_of_compressor31|16@0+A reserved field returning 48 while both request flags read 0, on a bus where the compressor is visibly running, is the clearest symptom.
The change
Extract big endian signals directly from the payload read as one big endian value. No byte swap, no special case on the length:
Little endian extraction is unchanged apart from being folded into a single return.
The commented-out bounds check is restored (and clamped to the 8 bytes this function can actually reach), so signals outside the DLC return 0 instead of bits of adjacent or stale bytes — that is #3.
Note this keeps the normalization in
DbcParser. It is also whatCanDbSignal::isPresentInMessage()already assumes, so that function stays correct as-is.Verification
Both implementations plus the proposed one were run against two logs recorded from a vehicle:
Expected values were cross-checked against
cantools, and independently against the physical behaviour of the bus (theAC_Requestbit pattern matches when the AC was switched on and off, and the compressor status follows it 110-460 ms later).DLC < 8 was checked separately: a 16-bit big endian signal in bytes 0-1 of a 4-byte frame decodes correctly, and a signal pointing at byte 5 returns 0.
Relation to #11
PR #11 targets the same bug with a different approach (removing the normalization from the parser and walking bytes in
extractRawSignal). It fixes the sub-byte cases but breaks 16-bit signals, because of an off-by-one whenlength - (start_bit % 8 + 1)is an exact multiple of 8:For the very common
7|16@0+this yields664879instead of2597. Worth knowing for anyone applying that patch, since the sub-byte signals do start working and it looks fixed.