GENIVI / CANdevStudio

Development tool for CAN bus simulation
Mozilla Public License 2.0
939 stars 253 forks source link

signal{encoder/decoder}: fix byte order & size calculation #206

Closed mguentner closed 3 years ago

mguentner commented 4 years ago

This PR might fix all possible encoding issues, these seem related:

https://github.com/GENIVI/CANdevStudio/issues/202 https://github.com/GENIVI/CANdevStudio/issues/200

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

This I tested this PR together with https://github.com/GENIVI/CANdevStudio/pull/205 To test simply cherry-pick dfe3e9062ce9584a6ee79c9fdaa25da47b4f6b01

mguentner commented 4 years ago

The mixup of big-endian and little-endian is also present in the tests, including the dbc files:

mrclgl commented 3 years ago

Why is this not merged? I'm confused... I'm using dbcppp in my application and the byte-order wasn't matching, this fixes my issue. Or is dbcppp wrong and CANdevStudio's current byte-order is correct?

rkollataj commented 3 years ago

The error is in CANdevStudio. I still not merged it as it is not passing CI build (fails on tests). Before fixing the test I need to update CANdb which requires currently C++17. I am working on moving CI to GitHub actions. During this switch I will drop support for old compilers and will be able to update CANdb. Right after this will be done I will fix tests and merge this PR. Pretty long explanation, but that's the full picture :-)

rkollataj commented 3 years ago

I've completed rework for Big Endian handling in signal decoder. The code required few more changes and alignment of unit tests. I've brefiely tested decoding with https://github.com/eerimoq/cantools/ and it seems to work fine. Let me know if you have any comments.

codecov[bot] commented 3 years ago

Codecov Report

Merging #206 (5155647) into master (c984022) will increase coverage by 0.03%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #206      +/-   ##
==========================================
+ Coverage   98.83%   98.87%   +0.03%     
==========================================
  Files          96       96              
  Lines        3002     3018      +16     
==========================================
+ Hits         2967     2984      +17     
+ Misses         35       34       -1     
Impacted Files Coverage Δ
...components/cansignaldecoder/cansignaldecoder_p.cpp 97.29% <100.00%> (+1.77%) :arrow_up:
...components/cansignalencoder/cansignalencoder_p.cpp 100.00% <100.00%> (ø)
src/common/pluginloader.h 97.95% <0.00%> (+0.04%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c984022...5155647. Read the comment docs.