Test and fix non-byte-aligned PDO mappings #454
Conversation
Use some more exotic variations of mappings which occupy bytes only partially, even spanning several PDO bytes with unaligned start and end bits. Introduce test objects for UNSIGNED16 and UNSIGNED32 to use as additional dummies in the test. Half of the tested objects are signed to see any handling problems there. The test verifies the PDO byte representation after setting dummy values and the round-trip from decoding these bytes again.
This was already respected for the non-byte-aligned mapping conditional.
For an unaligned variable mapping, both the first and the last containing byte can be only partially occupied by the variable data. Thus unpacking these bytes using the variable's original Struct format can result in wrong values. And the current code would even disregard the partial bits in the final byte. Instead, calculate the last occupied byte position by rounding up the last bit index divided by 8. Then use a generic int variable for the bit shifts and masking operations. The OD variable Struct description is only needed for checking the signedness anymore.
For an unaligned variable mapping, both the first and the last containing byte can be only partially occupied by the variable data. Thus unpacking these bytes using the variable's original Struct format can result in wrong values. And the current code would even disregard the partial bits in the final byte, or simply fail on the wrong data size. Instead, calculate the last occupied byte position by rounding up the last bit index divided by 8. Then use generic int variables for the bit shifts and masking operations on both the surrounding, unaffected data bits, and the new variable data. The OD variable Struct description is not needed at all anymore. Also use the calculated final affected byte position in the byte-aligned case, instead truncating the passed in data if too long for the mapping. This would previously overwrite more bytes than actually mapped, when given a too long data buffer.
|
Notice the failing test on 6a33115 without any other code changes. |
|
@sveinse, @christiansandberg, @af-silva this hasn't received much real-world testing yet on my end. Mainly for lack of a current project where PDOs are used extensively. So I'd be happy to get some feedback from testing in other applications. Should we aim to get this into the upcoming release though, or better let it mature and hope there are enough beta testers? |
|
@acolomb I don't have a current (HW) use case for unaligned PDOs, so I am not able to test. |
|
@sveinse Well my main concern is not breaking the currently supported use-cases. I.e. if you can verify that all your (aligned) PDO mappings are working fine, that would be enough to consider it for merging (after v2.3.0). The current code for unaligned mappings is broken beyond repair AFAICT, it had actual logic errors besides not supporting all possible cases. So I suspect nobody really used it, that's why the errors went unnoticed. |
|
I think it is important to note that unless there exists an amendment from CiA about non-aligned PDOs, this is officially not supported by canopen I think. There are canopen implementation might have a defacto practice, but I have no idea if we all do it the same. Have we found any references to any documentation that describes this anywhere? Unfortunately, we only use the async port of canopen at work, so I'd need to port this over to canopen-asyncio to be able to set this into a suitable use case for testing. TBH, I don't know if I'll get the time this week. |
|
@acolomb On my end is the same, right now I don't have access to HW for testing this, and I don't have any use case for non-aligned PDO's. On my main previous project I had 8 motors being controlled simultaneously using this API but my use case in particular the "regular" PDO's suffice. My main motivation for contributing to this project was that at the time I ended up making changes to the original DS 402 profile, as a way to understand how thinks worked but also to implement my requirements and try to simplify its usage. |
Section 7.4.8.2 in the CiA 301 spec lays out the "PDO mapping parameter record" datatype (0x21). It contains up to 64 (0x40) "objects to be mapped". That only makes sense if the extreme case is filling the PDO with 64 single bits (BOOLEAN values). So much for the minimum length an object mapping can occupy within the PDO. Sections 7.5.2.36 and 7.5.2.38 specify "The length contains the length of the application object in bit. This may be used to verify the mapping." Which indicates that the length parameter within the mapping should not differ from the length of the object's data type. I.e. "partially" mapped objects (such as only the low word of a 32-bit integer, if 16 bits is enough in practice) are not explicitly allowed. On the other hand, it is not forbidden either, it just says the length may be used for verification, not that it must match. The fact that the length is given in bits here, not bytes, hints toward arbitrary bit lengths being supported. Same goes for the example in section 7.1.5, where a STRUCT OF (INTEGER10, UNSIGNED5) is defined. And section 7.1.6.5 where TIME_OF_DAY has an UNSIGNED28 member followed by VOID4. Having such non-aligned lengths automatically leads to non-aligned mapping boundaries. I.e. if the first mapped object is a single BOOLEAN, all following whole-byte mapped objects will start and end on an odd bit index, therefore spanning partial PDO bytes. To summarize:
|
|
I concur with the assessment here. It makes sense. |
The PDO mapping code supports variables with bit offsets and lengths not a multiple of 8 bits, in theory. However the code is buggy and doesn't actually work correctly on many cases.
Add a test case for some more exotic variations of mappings which occupy bytes only partially, even spanning several PDO bytes with unaligned start and end bits. Introduce test objects for
UNSIGNED16andUNSIGNED32to use as additional dummies in the test. Half of the tested objects are signed to see any handling problems there. The test verifies the PDO byte representation after setting dummy values and the round-trip from decoding these bytes again.To fix the new test cases, rework
get_data()andset_data()in classPdoVariablecompletely. More details in the commit messages.