Inconsistent data truncation of unsupported data types in SDO upload

@ljungholm

I'm having problems reading SDO objects that are using UNSIGNED48. It used to work before PR #395 by @ljungholm - but it turns out to be coincidental.

https://github.com/christiansandberg/canopen/blob/24df6e846a4e71508fd82c378d1385d65f000c94/canopen/sdo/client.py#L123-L133

I believe the root cause is that canopen doesn't support the UNSIGNED48 type (which I'll contribute in another PR). The unfortunate behavior is that due to the unknown type, the data gets truncated to 1 byte. It didn't do that before #395 .

>> upload((8192, 2), {})
<-- 602     :  40 00 20 02 00 00 00 00
--> 582     :  41 00 20 02 08 00 00 00
  << read_response() = b'A\x00 \x02\x08\x00\x00\x00'
<-- 602     :  60 00 00 00 00 00 00 00
--> 582     :  00 b2 01 20 02 91 12 00
  << read_response() = b'\x00\xb2\x01 \x02\x91\x12\x00'
<-- 602     :  70 00 00 00 00 00 00 00
--> 582     :  1d 00 00 00 00 00 00 00
  << read_response() = b'\x1d\x00\x00\x00\x00\x00\x00\x00'
     data=b'\xb2\x01 \x02\x91\x12\x00\x00'
     response_size = 8
     var.datatype = 25   # UNSIGNED48 (unsupported)
     var_size = 1   # Because len(var) on unknown datatypes is 8
     data = b'\xb2'   # Truncated due to ^
<< upload() = b'\xb2'

Observations:

Reading SDO objects without OD record will preserve data as-is
Reading SDO objects with OD, but of unsupported type will return data of size 1
ODVariable.__len__() return 8 on unknown types, while it reports number of bits in the other types. This is why it gets truncated to one byte.

Would you agree that upload() should return the the full data for both of the first use cases? Is the fix that ODVariable.__len__() on unknown datatypes should return 64?

Would you agree that upload() should return the the full data for both of the first use cases? Is the fix that ODVariable.__len__() on unknown datatypes should return 64?

PR #440 implements four test cases (test_unknown_*()) that address this issue. The test fails in the PR because the contains longer data and it gets truncated. IMHO canopen shouldn't truncate any messages it encounters for unknown datatypes. Especially when it is passed transparently if there is no OD entry for it.

Why do we have the logic of if var.data_type not in objectdictionary.DATA_TYPES: test and then proceed with truncating the message?

Maybe use 64 bits instead of 8 is the easiest solution. Hopefully it shouldn't happen too often after 48 bits support is added.

It depends on not getting a future datatype which is larger than 64 bits. If we chose to cap it at 64, I would suggest that we warn about it to the logger that the data has been capped with an unknown datatype. (And the test in PR #440 must be altered to make it pass.)

I'm not really fond of replacing the fall-back 8 bits value by 64 bits.

It makes sense for any type to have at least 8 bits, since one octet is the smallest unit supported by SDO (disregarding PDO mapping with finer granularity here). Using any other fall-back value is arbitrary and will break unexpectedly when it does.

We should rather fix the min(OD, SDO) length assumption to only get applied when the OD size is actually meaningful and not itself a fall-back value.

We should rather fix the min(OD, SDO) length assumption to only get applied when the OD size is actually meaningful and not itself a fall-back value.

I think this makes sense. Since the data type is of unknown type, no assumptions about the data contents can be made. So it should be passed transparently without truncation. -- Especially since it would pass transparently if the object have no OD entry.

I agree, better to check if size is known or not.

I attempted

var = self.od.get_variable(index, subindex)
if var is not None:
    if var.data_type not in objectdictionary.DATA_TYPES:
        # Use max to determine enough space for the unknown data type
       var_size = max(len(var) // 8, response_size)
        if response_size is None or var_size < response_size:
            # This wil never happen...
            data = data[0:var_size]
return data

But it turned out a bit non-sensical. So I'm kind of back at: Why do we even truncate? Why do we want to reduce the data for something we don't know what is?

Just a quick note, the truncating was introduced to avoid an exception from the struct methods. They are quite strict about the buffer length, but could be tricked otherwise by slicing.

Will need to take another good look at the alternatives and code flow...

I would assume that known data types can or should be truncated in case of excessive data. Here we're talking about unknown data types with no reference to validate its proper size.

I'm really still uncertain what the actual problem here is. SdoClient.upload() is actually independent from the ODVariable. It used to (before #395) only consult the OD when the uploaded data was of unspecified size. #395 changed that to apply even when an upload size was indicated, which I think is not always correct. For broken devices that send e.g. an UNSIGNED8 as UNSIGNED32 with four data bytes, it does make sense to truncate the data instead of failing during unpack(). But for any type not based on struct.Struct, truncating is usually harmful.

The real culprit here is IMHO the check var.data_type not in objectdictionary.DATA_TYPES. That applies for any unknown OD entry type as well, but should actually be limited to types where the length is known in advance. So better to check for var.data_type in var.STRUCT_TYPES here, assuming that all known-length data types will be unpacked using structs. Even better to encapsulate this into a new property ODVariable.length_known.

And maybe this whole check should be moved to SdoVariable.get_data(), where we are certain that an associated ODVariable is actually available? For the "OD-independent" (as stated by the docstring) .upload() method, consulting the OD is a bit inconsistent.

I think we have two independent factors or conditions for any object ready by SDO upload:

Is the var using a known data type
Is there a OD entry for the object

(The PR #440 contains unit tests for both of these cases)

Known Data type	Have OD entry	Result data
Yes	Yes	As-is
Yes	No	As-is
No	No	As-is
No	Yes	Data truncated

So I think the summary of the problem statement is that the current implementation truncates the data in the latter case.

I think the problem is to more clearly define what is a "known" data type. The check against objectdictionary.DATA_TYPES actually just matches (VISIBLE_STRING, OCTET_STRING, UNICODE_STRING, DOMAIN). Those are the defined ones which are not parsed by a struct, and the check also matches undefined data type entries. We should invert that and apply the truncation only for known data types where a struct gets used for parsing, I think.

Also, your second case is impossible, as the data type is defined in the OD entry, thus without one we cannot know.

I think the problem is to more clearly define what is a "known" data type. The check against objectdictionary.DATA_TYPES actually just matches (VISIBLE_STRING, OCTET_STRING, UNICODE_STRING, DOMAIN). Those are the defined ones which are not parsed by a struct, and the check also matches undefined data type entries. We should invert that and apply the truncation only for known data types where a struct gets used for parsing, I think.

Yes, I think that is a good idea. We must except array-types from this truncation, being (VISIBLE_STRING, OCTET_STRING, UNICODE_STRING, DOMAIN).

Also, your second case is impossible, as the data type is defined in the OD entry, thus without one we cannot know.

Yes, you're right. Without OD one cannot know the data type. I missed the fact. It is possible thou to read an object with SDO that is missing from the OD and thus its data type is missing. Its data should definitely not be truncated.

Right, so to summarize: We only truncate if the OD contains an entry AND its data type is contained in STRUCT_TYPES. That automatically leaves array types (DOMAIN etc.) untouched, and will not explode for yet undefined type constants.

On to the other questions:

Should the truncation happen always in SdoClient.upload(), or do we move it to SdoVariable.get_data(), which will only ever be called with a valid ODVariable associated? IMHO if upload() states that it works "without an Object Dictionary", then looking up anything from the OD in that function will be a surprise.
I suggested to encapsulate the check into some ODVariable.length_known property for better readability and separation of concerns.

It must look up the OD to get the data type, so in order for SdoClient.upload() to do any length validation, it must use the OD.

IMHO SdoClient.upload() is just a raw producer of bytes using an SDO upload request. The deserialization step into native python objects happens later. So I think I lean towards not doing any validation in SdoClient.upload().

Today the deserialization happens in ODVariable.decode_raw() with its call from the Variable.raw property. This is where the stream from the SDO upload should be validated, I think.

EDIT I vote for removing any validation from SdoClient.upload() and add it to ODVariable.decode_raw(). The validation should only be made on known data types (that is types this library can decode) with the exception of the known array formats.

PPS! Should we validate, e.g. by warning or error, or should we truncate (and warn)?

Disable failing tests waiting on a fix for canopen-python#436.

@acolomb

@acolomb Yes, let's not delay the release because of this. Current behavior is what it is and we're not introducing any regressions by not fixing this issue.

Implement the remaining canopen datatypes (#440)

@friederschueler

@friederschueler not sure I'd label this one as an enhancement. Its describing unexpected behavior, so it's a bug.

I think we should pick up this one soon. It was a great effort earlier to get all the missing datatypes implemented.

I'm having a system with manufacturer specific datatypes. Due to how the current logic is implemented it gets truncated. A working example against HW:

        od = ObjectDictionary()
        record = ODRecord("Record", 0x2020)
        var = ODVariable("Variable", 0x2020, 1)
        var.data_type = 0x40  # Custom manufacturer data-type. Returns 12 bytes
        record.add_member(var)  # <-- If this is commented out, the code returns right value
        od.add_object(record)
        node = await network.aadd_node(2, od)
        data = await node.sdo.aaupload(0x2020, 1)
        # data == b'2' (one byte), which is due to the truncation into 8 bits

Trying to summarize it into a table again (may need to scroll horizontally):

What's missing is handling of use case (9) - (12), so I'll focus on them.

(9) Truncates to 1 byte
(10) Truncates to 1 byte (the above example is in this class)
(11) Works fine, returns 12 bytes
(12) Works fine, returns 12 bytes

It is fully valid for users to define custom data types and we should not interfere with the data in those instances.

I created #447 some time ago and I have updated it with the latest branch. I think this is a fix to this issue. Please take a look at it.

What you're fixing in #447 touches several of these cases. The changed test is actually about (6), totally unrelated to the case of a missing OD entry or unknown data type.

So I think we should in fact concentrate on the wrongfully truncating cases, (9) and (10). But without changing behavior of other cases needlessly. If a test needs to be adjusted to accomodate a change in the code, it always smells like API break, which might have unexpected effects on peoples' application code. So why not go back to the previous suggestion and only make sure that truncation affects only existing OD entries with known types:

The real culprit here is IMHO the check var.data_type not in objectdictionary.DATA_TYPES. That applies for any unknown OD entry type as well, but should actually be limited to types where the length is known in advance. So better to check for var.data_type in var.STRUCT_TYPES here, assuming that all known-length data types will be unpacked using structs. Even better to encapsulate this into a new property ODVariable.length_known.

Regarding my other previous suggestion of having .upload() disregard the OD entry altogether, that's the part which constitutes and API break IMHO. Your PR #447 removes the checks in that function altogether, which I'd rather postpone until we have collected other breaking changes -- not before the next release.

At that point, we need to decide whether any truncation should be done in SdoVariable.get_data() or ODVariable.decode_raw(). I haven't yet considered all other cases where the latter is involved.

What you're fixing in #447 touches several of these cases. The changed test is actually about (6), totally unrelated to the case of a missing OD entry or unknown data type.

So I think we should in fact concentrate on the wrongfully truncating cases, (9) and (10).

The changed test test_size_not_specified where to make the test case pass, as #447 moves the expected data truncation to ODVariable.decode_raw(). However, tests test_unknown_datatype32 and test_unknown_datatype112 were reenabled, which tests use case (10) and (12). It should be failry easy to add for (9) and (11).

...not before the next release.

Well... We said the same thing a release ago. 😜

Well... We said the same thing a release ago. 😜

Right, but this time we already have a plan to branch off with other possibly API-incompatible changes. That's why I see merit in getting a first improvement out the door without breakage. Proposed code changes in #591.

To be clear, I do agree we should remove OD access from .upload() completely, at which point the change on the test case will be correct. Proposed code changes in #592.

Avoid truncating unknown types in SDO upload (fixes canopen-python#436)…

Navigation Menu

Repository navigation

Inconsistent data truncation of unsupported data types in SDO upload #436

3 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Inconsistent data truncation of unsupported data types in SDO upload #436

Description

Activity

sveinse commented on May 20, 2024

christiansandberg commented on May 20, 2024

sveinse commented on May 20, 2024

acolomb commented on May 23, 2024

sveinse commented on May 23, 2024

christiansandberg commented on May 23, 2024

sveinse commented on May 23, 2024

acolomb commented on May 23, 2024

sveinse commented on May 23, 2024

acolomb commented on May 27, 2024

sveinse commented on May 27, 2024

acolomb commented on May 27, 2024

sveinse commented on May 27, 2024

acolomb commented on May 27, 2024

sveinse commented on May 27, 2024

3 remaining items

sveinse commented on Jun 11, 2024

sveinse commented on Jun 16, 2024

sveinse commented on Jun 14, 2025

sveinse commented on Jun 15, 2025

acolomb commented on Jun 15, 2025

sveinse commented on Jun 15, 2025

acolomb commented on Jun 15, 2025

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions