From c87c40047cfea228465006644e87977cc5ed7c97 Mon Sep 17 00:00:00 2001 From: Ryotaro Isoyama Date: Tue, 4 Aug 2026 18:42:01 +0900 Subject: [PATCH] Fix extraction of Motorola (big endian) signals 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 #2 Fixes #3 Co-Authored-By: Claude Opus 5 (1M context) --- src/core/CanMessage.cpp | 42 ++++++++++++++++++++--------------------- 1 file changed, 20 insertions(+), 22 deletions(-) diff --git a/src/core/CanMessage.cpp b/src/core/CanMessage.cpp index 386cfbfc..948f1647 100644 --- a/src/core/CanMessage.cpp +++ b/src/core/CanMessage.cpp @@ -184,33 +184,31 @@ void CanMessage::setByte(const uint8_t index, const uint8_t value) { uint64_t CanMessage::extractRawSignal(uint8_t start_bit, const uint8_t length, const bool isBigEndian) const { -// if ((start_bit+length) > (getLength()*8)) { -// return 0; -// } + if ((length == 0) || (length > 64)) { + return 0; + } // FIXME: This only gives access to data bytes 0-8. Need to rework for CANFD. - uint64_t data = le64toh(_u64[0]); - - data >>= start_bit; - - uint64_t mask = 0xFFFFFFFFFFFFFFFF; - mask <<= length; - mask = ~mask; - - data &= mask; - - // If the length is greater than 8, we need to byteswap to preserve endianness - if (isBigEndian && (length > 8)) - { - - // Swap bytes - data = __builtin_bswap64(data); + const uint8_t avail_bits = 8 * ((getLength() > 8) ? 8 : getLength()); + if (start_bit + length > avail_bits) { + return 0; + } - // Shift out unused bits - data >>= 64 - length; + const uint64_t mask = (length == 64) ? ~(uint64_t)0 : (((uint64_t)1 << length) - 1); + + if (isBigEndian) { + // DbcParser has already normalized start_bit into an MSB-first bit index + // counted from the first transmitted bit of the payload. The signal + // therefore occupies bits [start_bit, start_bit+length) of the payload + // read as one big-endian 64 bit value. + uint64_t be = 0; + for (int i = 0; i < 8; i++) { + be = (be << 8) | _u8[i]; + } + return (be >> (64 - start_bit - length)) & mask; } - return data; + return (le64toh(_u64[0]) >> start_bit) & mask; } void CanMessage::setDataAt(uint8_t position, uint8_t data)