Skip to content

Commit e9a850b

Browse files
mguentnerrkollataj
mguentner
and
rkollataj
authored
signal{encoder/decoder}: fix byte order & size calculation (#206)
* signal{encoder/decoder}: fix byte order & size calculation The PDF that supposedly describes the DBC format got the byte-order apparently wrong :) The correct values are: 0 => big-endian, motorola 1 => little-endian, intel Both encoder/decoder do bound checks before inserting the bits into the raw signal. The bound checks need to take the endianess into account because: if the signal is little_endian / intel, start_bit describes the LSB, so end_bit = start_bit + signal_size if the signal is big_endian / motorola, start_bit describes the MSB, so end_bit = start_bit - signal_size + 1 * Fix Big Endian handling for Signal Decoder Signed-off-by: Remigiusz Kołłątaj <remigiusz.kollataj@gmail.com> * Fix missing argument in a log line Signed-off-by: Remigiusz Kołłątaj <remigiusz.kollataj@gmail.com> * Typos fixed. Python script for generation of transition table simplified Signed-off-by: Remigiusz Kołłątaj <remigiusz.kollataj@gmail.com> * Fix CanSignalEncoder logic and tests Signed-off-by: Remigiusz Kołłątaj <remigiusz.kollataj@gmail.com> Co-authored-by: Remigiusz Kołłątaj <remigiusz.kollataj@gmail.com>
1 parent 52d4515 commit e9a850b

File tree

6 files changed

+255
-146
lines changed

6 files changed

+255
-146
lines changed

‎src/components/cansignaldecoder/cansignaldecoder_p.cpp‎

Lines changed: 87 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,33 @@
11
#include "cansignaldecoder_p.h"
22
#include <QCanBusFrame>
3+
#include <array>
34
#include <log.h>
45

6+
// Python script used to generate below:
7+
//
8+
// for rows in range (1,9):
9+
// bit = rows*8 - 1
10+
// for cols in range(0,8):
11+
// val = int(bit - cols)
12+
// print('%3d,' % val, end='')
13+
// if val % 8 == 0:
14+
// print('')
15+
//
16+
// clang-format off
17+
namespace {
18+
const std::array beTransTable = {
19+
7, 6, 5, 4, 3, 2, 1, 0,
20+
15, 14, 13, 12, 11, 10, 9, 8,
21+
23, 22, 21, 20, 19, 18, 17, 16,
22+
31, 30, 29, 28, 27, 26, 25, 24,
23+
39, 38, 37, 36, 35, 34, 33, 32,
24+
47, 46, 45, 44, 43, 42, 41, 40,
25+
55, 54, 53, 52, 51, 50, 49, 48,
26+
63, 62, 61, 60, 59, 58, 57, 56
27+
};
28+
}
29+
// clang-format on
30+
531
CanSignalDecoderPrivate::CanSignalDecoderPrivate(CanSignalDecoder* q, CanSignalDecoderCtx&& ctx)
632
: _ctx(std::move(ctx))
733
, q_ptr(q)
@@ -61,37 +87,45 @@ void CanSignalDecoderPrivate::decodeFrame(const QCanBusFrame& frame, Direction c
6187

6288
if (msgDesc) {
6389
for (auto& sig : msgDesc->second) {
64-
if ((sig.startBit >= (frame.payload().size() * 8))
65-
|| ((sig.startBit + sig.signalSize - 1) >= (frame.payload().size() * 8))) {
66-
67-
cds_error("Invalid message size - startBit {}, sigSize {}, payload size {}", sig.startBit,
68-
sig.signalSize, frame.payload().size());
69-
70-
continue;
71-
}
7290

7391
switch (sig.byteOrder) {
7492
case 0:
75-
// Little endian
76-
littleEndian = true;
77-
break;
78-
79-
case 1:
8093
// Big endian
8194
littleEndian = false;
8295
break;
96+
case 1:
97+
// Little endian
98+
littleEndian = true;
99+
break;
83100

84101
default:
85102
cds_error("byte order {} not suppoerted", sig.byteOrder);
86103
continue;
87104
}
88105

106+
// Calculate how many bits are used already before this signal. Calculations are different for
107+
// little and big endian. Good overview on how big endian signals are aligned can be found
108+
// here: https://github.com/eerimoq/cantools#the-dump-subcommand
109+
uint8_t bitsBefore = 0;
110+
111+
if (littleEndian) {
112+
bitsBefore = sig.startBit;
113+
} else {
114+
bitsBefore = beTransTable[sig.startBit];
115+
}
116+
117+
if (bitsBefore + sig.signalSize > (frame.payload().size() * 8)) {
118+
cds_error(
119+
"Invalid message size - startBit {}, sigSize {}, bitsBefore {}, payload size {}, littleEndian: {}",
120+
sig.startBit, sig.signalSize, bitsBefore, frame.payload().size(), littleEndian);
121+
continue;
122+
}
123+
89124
int64_t value = rawToSignal((const uint8_t*)frame.payload().constData(), sig.startBit, sig.signalSize,
90125
littleEndian, sig.valueSigned);
91126

92127
QVariant sigVal;
93128

94-
95129
if ((std::fmod(sig.factor, 1) == 0.0) && (std::fmod(sig.offset, 1) == 0.0)) {
96130
// resulting number will be integer
97131
value = value * sig.factor + sig.offset;
@@ -123,17 +157,49 @@ int64_t CanSignalDecoderPrivate::rawToSignal(
123157
{
124158
int64_t result = 0;
125159

126-
int bit = startBit;
127-
for (int bitpos = 0; bitpos < sigSize; bitpos++) {
128-
if (data[bit / 8] & (1 << (bit % 8))) {
129-
if (littleEndian) {
160+
if (littleEndian) {
161+
// Little endian signal example with start bit 2 and length 9 (0=LSB, 8=MSB):
162+
// Byte: 0 1 2 3
163+
// +--------+--------+--------+--- - -
164+
// |543210| | |876| |
165+
// +--------+--------+--------+--- - -
166+
// Bit: 7 0 15 8 23 16 31
167+
//
168+
// Source: https://github.com/eerimoq/cantools/blob/master/cantools/database/can/signal.py
169+
170+
uint8_t bit = startBit;
171+
for (uint8_t bitpos = 0; bitpos < sigSize; bitpos++) {
172+
173+
if (data[bit / 8] & (1 << (bit % 8))) {
130174
result |= (1ULL << bitpos);
131-
} else {
132-
result |= (1ULL << (sigSize - bitpos - 1));
133175
}
176+
177+
++bit;
134178
}
179+
} else {
135180

136-
bit++;
181+
// Big endian signal example with start bit 2 and length 5 (0=LSB, 4=MSB):
182+
// Byte: 0 1 2 3
183+
// +--------+--------+--------+--- - -
184+
// | |432|10| | |
185+
// +--------+--------+--------+--- - -
186+
// Bit: 7 0 15 8 23 16 31
187+
//
188+
// Source: https://github.com/eerimoq/cantools/blob/master/cantools/database/can/signal.py
189+
190+
uint8_t bitpos = 0;
191+
for (int i = sigSize - 1; i >= 0; --i) {
192+
// First beTransTable returns number of bytes used before startBit
193+
// Then we are adding length of the signal and getting 'offset' where the LSB is
194+
// Second 'translation' of 'offset' with beTransTable gives us actual bit position
195+
int bit = beTransTable[beTransTable[startBit] + i];
196+
197+
if (data[bit / 8] & (1 << (bit % 8))) {
198+
result |= (1ULL << bitpos);
199+
}
200+
201+
++bitpos;
202+
}
137203
}
138204

139205
// if signal is signed and sign bit (MSB) is set make sure

‎src/components/cansignaldecoder/tests/cansignaldecoder_test.cpp‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -354,7 +354,7 @@ TEST_CASE("UnsignedSignals_BE", "[cansignaldecoder]")
354354
data.startSimulation();
355355
c.startSimulation();
356356

357-
QByteArray p = QByteArray::fromHex("2542100808104080");
357+
QByteArray p = QByteArray::fromHex("a442081010080201");
358358

359359
QCanBusFrame frame;
360360
frame.setFrameId(0x1002);
@@ -393,7 +393,7 @@ TEST_CASE("UnsignedSignals_BE", "[cansignaldecoder]")
393393
c.startSimulation();
394394
sigSndSpy.clear();
395395

396-
p = QByteArray::fromHex("4b84201010208000");
396+
p = QByteArray::fromHex("d221040808040100");
397397

398398
frame.setFrameId(0x1002);
399399
frame.setPayload(p);
@@ -444,7 +444,7 @@ TEST_CASE("SignedSignals_BE", "[cansignaldecoder]")
444444
data.startSimulation();
445445
c.startSimulation();
446446

447-
QByteArray p = QByteArray::fromHex("dabdeff7f7efbfff");
447+
QByteArray p = QByteArray::fromHex("5bbdf7efeff7fdff");
448448

449449
QCanBusFrame frame;
450450
frame.setFrameId(0x1003);
@@ -517,7 +517,7 @@ TEST_CASE("SignedSignals_BE", "[cansignaldecoder]")
517517
c.startSimulation();
518518
sigSndSpy.clear();
519519

520-
p = QByteArray::fromHex("3763180c0c186080");
520+
p = QByteArray::fromHex("ecc6183030180601");
521521

522522
frame.setFrameId(0x1003);
523523
frame.setPayload(p);
@@ -553,7 +553,7 @@ TEST_CASE("SignedSignals_BE", "[cansignaldecoder]")
553553
c.startSimulation();
554554
sigSndSpy.clear();
555555

556-
p = QByteArray::fromHex("2542100808104000");
556+
p = QByteArray::fromHex("a442081010080200");
557557

558558
frame.setFrameId(0x1003);
559559
frame.setPayload(p);

‎src/components/cansignaldecoder/tests/dbc/tesla_can.dbc‎

Lines changed: 30 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -61,31 +61,6 @@ VAL_TABLE_ DI_state 4 "DI_STATE_ENABLE" 3 "DI_STATE_FAULT" 2 "DI_STATE_CLEAR_FAU
6161
VAL_TABLE_ DI_velocityEstimatorState 4 "VE_STATE_BACKUP_MOTOR" 3 "VE_STATE_BACKUP_WHEELS_B" 2 "VE_STATE_BACKUP_WHEELS_A" 1 "VE_STATE_WHEELS_NORMAL" 0 "VE_STATE_NOT_INITIALIZED" ;
6262

6363
BO_ 4096 UnsignedSignals_LE: 8 TEST
64-
SG_ Test01 : 0|1@0+ (1,0) [0|0] "" TEST
65-
SG_ Test02 : 1|2@0+ (1,0) [0|0] "" TEST
66-
SG_ Test03 : 3|3@0+ (1,0) [0|0] "" TEST
67-
SG_ Test04 : 6|4@0+ (1,0) [0|0] "" TEST
68-
SG_ Test05 : 10|5@0+ (1,0) [0|0] "" TEST
69-
SG_ Test06 : 15|6@0+ (1,0) [0|0] "" TEST
70-
SG_ Test07 : 21|7@0+ (1,0) [0|0] "" TEST
71-
SG_ Test08 : 28|8@0+ (1,0) [0|0] "" TEST
72-
SG_ Test09 : 36|9@0+ (1,0) [0|0] "" TEST
73-
SG_ Test10 : 45|10@0+ (1,0) [0|0] "" TEST
74-
SG_ Test11 : 55|9@0+ (1,0) [0|0] "" TEST
75-
76-
BO_ 4097 SignedSignals_LE: 8 TEST
77-
SG_ Test02 : 0|2@0- (1,0) [0|0] "" TEST
78-
SG_ Test03 : 2|3@0- (1,0) [0|0] "" TEST
79-
SG_ Test04 : 5|4@0- (1,0) [0|0] "" TEST
80-
SG_ Test05 : 9|5@0- (1,0) [0|0] "" TEST
81-
SG_ Test06 : 14|6@0- (1,0) [0|0] "" TEST
82-
SG_ Test07 : 20|7@0- (1,0) [0|0] "" TEST
83-
SG_ Test08 : 27|8@0- (1,0) [0|0] "" TEST
84-
SG_ Test09 : 35|9@0- (1,0) [0|0] "" TEST
85-
SG_ Test10 : 44|10@0- (1,0) [0|0] "" TEST
86-
SG_ Test11 : 54|10@0- (1,0) [0|0] "" TEST
87-
88-
BO_ 4098 UnsignedSignals_BE: 8 TEST
8964
SG_ Test01 : 0|1@1+ (1,0) [0|0] "" TEST
9065
SG_ Test02 : 1|2@1+ (1,0) [0|0] "" TEST
9166
SG_ Test03 : 3|3@1+ (1,0) [0|0] "" TEST
@@ -98,7 +73,7 @@ BO_ 4098 UnsignedSignals_BE: 8 TEST
9873
SG_ Test10 : 45|10@1+ (1,0) [0|0] "" TEST
9974
SG_ Test11 : 55|9@1+ (1,0) [0|0] "" TEST
10075

101-
BO_ 4099 SignedSignals_BE: 8 TEST
76+
BO_ 4097 SignedSignals_LE: 8 TEST
10277
SG_ Test02 : 0|2@1- (1,0) [0|0] "" TEST
10378
SG_ Test03 : 2|3@1- (1,0) [0|0] "" TEST
10479
SG_ Test04 : 5|4@1- (1,0) [0|0] "" TEST
@@ -110,11 +85,36 @@ BO_ 4099 SignedSignals_BE: 8 TEST
11085
SG_ Test10 : 44|10@1- (1,0) [0|0] "" TEST
11186
SG_ Test11 : 54|10@1- (1,0) [0|0] "" TEST
11287

88+
BO_ 4098 UnsignedSignals_BE: 8 TEST
89+
SG_ Test01 : 7|1@0+ (1,0) [0|0] "" TEST
90+
SG_ Test02 : 6|2@0+ (1,0) [0|0] "" TEST
91+
SG_ Test03 : 4|3@0+ (1,0) [0|0] "" TEST
92+
SG_ Test04 : 1|4@0+ (1,0) [0|0] "" TEST
93+
SG_ Test05 : 13|5@0+ (1,0) [0|0] "" TEST
94+
SG_ Test06 : 8|6@0+ (1,0) [0|0] "" TEST
95+
SG_ Test07 : 18|7@0+ (1,0) [0|0] "" TEST
96+
SG_ Test08 : 27|8@0+ (1,0) [0|0] "" TEST
97+
SG_ Test09 : 35|9@0+ (1,0) [0|0] "" TEST
98+
SG_ Test10 : 42|10@0+ (1,0) [0|0] "" TEST
99+
SG_ Test11 : 48|9@0+ (1,0) [0|0] "" TEST
100+
101+
BO_ 4099 SignedSignals_BE: 8 TEST
102+
SG_ Test02 : 7|2@0- (1,0) [0|0] "" TEST
103+
SG_ Test03 : 5|3@0- (1,0) [0|0] "" TEST
104+
SG_ Test04 : 2|4@0- (1,0) [0|0] "" TEST
105+
SG_ Test05 : 14|5@0- (1,0) [0|0] "" TEST
106+
SG_ Test06 : 9|6@0- (1,0) [0|0] "" TEST
107+
SG_ Test07 : 19|7@0- (1,0) [0|0] "" TEST
108+
SG_ Test08 : 28|8@0- (1,0) [0|0] "" TEST
109+
SG_ Test09 : 36|9@0- (1,0) [0|0] "" TEST
110+
SG_ Test10 : 43|10@0- (1,0) [0|0] "" TEST
111+
SG_ Test11 : 49|10@0- (1,0) [0|0] "" TEST
112+
113113
BO_ 1160 DAS_steeringControl: 4 NEO
114-
SG_ DAS_steeringControlType : 23|80@0+ (0,0) [0|0] "" EPAS
115-
SG_ DAS_steeringControlChecksum : 31|80@0+ (1,0) [0|0] "" EPAS
116-
SG_ DAS_steeringControlCounter : 19|80@0+ (1,0) [0|0] "" EPAS
117-
SG_ DAS_steeringAngleRequest : 6|80@0+ (0.1,-1638.35) [-1638.35|1638.35] "deg" EPAS
114+
SG_ DAS_steeringControlType : 23|80@1+ (0,0) [0|0] "" EPAS
115+
SG_ DAS_steeringControlChecksum : 31|80@1+ (1,0) [0|0] "" EPAS
116+
SG_ DAS_steeringControlCounter : 19|80@1+ (1,0) [0|0] "" EPAS
117+
SG_ DAS_steeringAngleRequest : 6|80@1+ (0.1,-1638.35) [-1638.35|1638.35] "deg" EPAS
118118
SG_ DAS_steeringHapticRequest : 7|80@2+ (1,0) [0|0] "" EPAS
119119

120120
BO_ 257 GTW_epasControl: 3 NEO

0 commit comments

Comments
 (0)