Big Endian Decoding Errors about cantools HOT 9 CLOSED

Hi. I have not used KCD but the last time that I had to cope with byte order and bit numbering with the DBC format, I found this writeup particulary useful:

• Signal Byte Order

They also sum up which format takes which approach. Maybe that helps.

from cantools.

You can also have a look here: http://cantools.readthedocs.io/en/latest/#cantools.db.Signal

Signal placement in your test_big_endian_signal_two_byte_decode() test is as below.

The big endian signal placement is very counter intuitive, but I have no plans on changing it. It is based on DBC placement, if I remeber correctly. To place a big endian signal at the end, the start bit should be 55, not 48.

Start bit 48:

Byte:       0        1        2        3        4        5        6        7
       +--------+--------+--------+--------+--------+--------+--------+--------+
       |        |        |        |        |        |        |       x|xxxxxxxx|xxxxxxx
       +--------+--------+--------+--------+--------+--------+--------+--------+
Bit:    7      0 15     8 23    16 31    24 39    32 47    40 55    48 63    56

Start bit 55:

Byte:       0        1        2        3        4        5        6        7
       +--------+--------+--------+--------+--------+--------+--------+--------+
       |        |        |        |        |        |        |xxxxxxxx|xxxxxxxx|
       +--------+--------+--------+--------+--------+--------+--------+--------+
Bit:    7      0 15     8 23    16 31    24 39    32 47    40 55    48 63    56 

I have not used big endian KCD in practice myself, and if it turns out they are encoded erroneously, we should do something about it.

from cantools.

I implemented a fix where the KCD big endian signal offset is converted the the cantools start bit definition. That is, KCD offset 0 becomes start bit 7. The Kayak project, that originally defined the KCD format, implements signal unpacking as: https://github.com/dschanoeh/Kayak/blob/5c45f9249d0c4cb5ee04f28a3ff14a1400858213/Kayak-core/src/main/java/com/github/kayak/core/description/SignalDescription.java#L279-L301

I think the new cantools implementation does the same thing. Feel free to try it out and report any issues.

See c30a631 for the fix.

from cantools.

Thanks a lot for sharing links to help me understand the correct decoding behavior. KCD unlike DBC, isn't as established so it's hard to determine what the correct behavior should be.

The following info from Canmatrix Signal ByteOrder was the key revelation for me:

With "consistent" format/numbering, bit numbering follows the byte format. MSB0 for big endian, LSB0 for little endian.

KCD, SYM (consistent bit numbering)
little,LSB0
big,MSB0

DBC, ARXML (OSEK bit numbering)
little,LSB0
big,LSB0 (inconsistent)

Although I think their use of MSB0 and LSB0 is confusing, as I think they are mixing up bit numbering with byte order, and could rewrite to be less ambiguous.

I like the explaination here, where I assume KCD uses "Normal / Consistent Bit Number", and DBC uses "Reverse Bit Numbering".
http://vi-firmware.openxcplatform.com/en/master/config/bit-numbering.html

Unfortunately, the KCD XML schema seems to make no mention of bit order consistency.
https://github.com/dschanoeh/Kayak/blob/master/Kayak-kcd/src/main/resources/com/github/kayak/canio/kcd/loader/Definition.xsd

It seems to me the offset section of the KCD schema should mention regardless of endianess, the bit numbering is the same.

<xs:attribute name="offset" use="required">
      <xs:annotation>
        <xs:documentation>Least significant bit offset of the signal relative to the least significant bit of the messages data payload.</xs:documentation>
</xs:annotation>

Given the lack of clarity in the schema, it seems citing the source code is the only way to go, or cite other implementations such as canmatrix.

@eerimoq If it is infact the case that KCD has consistent bit numbering, the commit you made makes sense / works for me.

from cantools.

@maxchill Please open an issue in the KCD project ask for clarification of the signal endianess and placement. Then we can implement this correctly.

from cantools.

@maxchill Otherwise, please close the issue and we'll deal with this in the future.

from cantools.

The fix implemented as part of this issue is part of release 22.0.0 on PyPI.

from cantools.

I will follow through / get to the bottom of this later this week. Thanks.

from cantools.

I will close this if there is no feedback soon.

from cantools.