Implement the remaining canopen datatypes

[sveinse]

• Add datatype defs for the canopen standard types
• Rename datatypes_24bit.py to datatypes_struct.py
• Replace Unsigned24 and Interger24 by generic UnsignedN and IntegerN respectively
• Add EDS-file containing all datatypes
• Added tests for encoding and decoding all datatypes
• Added tests for SDO uploads of all datatypes

Related to #436 and #439. This issue will close #437. Closes #320

[sveinse]

There are some notable comments for this PR.

1. The PR currently fails because of the issue in Inconsistent data truncation of unsupported data types in SDO upload #436. It fails because this PR contains a test that is affected by it. So I need to find a resolution to it before this PR can be finished.

2. I've taken the liberty to remove Unsigned24 and Integer24 and replace them by new generic classes UnsignedN and IntegerN. I grepped for it and it doesn't seem to be used anywhere else.

3. The file datatypes_24bit.py to datatypes_struct.py since the name is more generic.

[sveinse]

BTW: The following code asks if 'utf-16-le' is correct for UNICODE_STRING. The canopen standard doesn't define any encoding variant to use, except that it should be 16-bit glyphs. I think its a good assumption and choice to use utf-16-le here.

canopen/canopen/objectdictionary/__init__.py

Lines 388 to 390 in 24df6e8

	elif self.data_type == UNICODE_STRING:
	# Is this correct?
	return data.rstrip(b"\x00").decode("utf_16_le", errors="ignore")

[acolomb]

Looks good overall, with some outstanding issues noted below.

I haven't checked the tests thoroughly, but trusting in your diligence that they're correct.

[sveinse]

Comments updated

[sveinse]

Updates pushed. There are currently two open issues

• How to deal with Integer24 and Unsigned24 compatibility
• The outcome of the data truncation issue as described in Inconsistent data truncation of unsupported data types in SDO upload #436

[acolomb]

Thanks for considering the previous feedback. Some styling nit-picks remain, and an actual fix of wrong numeric data type constants.

Regarding the 24-bit compatibility classes, I'm okay with dropping them after @raffi-g's feedback.

I've commented on #436 with my current understanding of the problem, and a possibly easy fix. Should be able to get that squared away soon.

[sveinse]

Should we disable the two failing tests and make a new PR from the result of the discussions in #436? This way we can complete this PR and focus on solving the other.

FAILED test/test_sdo.py::TestSDOClientDatatypes::test_unknown_datatype112 - AssertionError: b'\xb2' != b'\xb2\x01 \x02\x91\x12\x03\x19!p\xfe\xfd\xfc\xfb'
FAILED test/test_sdo.py::TestSDOClientDatatypes::test_unknown_datatype32 - AssertionError: b'\xfe' != b'\xfe\xfd\xfc\xfb'

[sveinse]

Please disregard the "Proposed fix for..." reference above. It was a hasty mistake.

[acolomb]

Should we disable the two failing tests and make a new PR [...]?

Testing the solution to #436 requires the same tests. Ideally we'd have one commit where they fail, then the fix. And finally squash both when merging. I guess disabling with a FIXME is appropriate here, since work on the other issue is ongoing and foreseeable to fix them soon.

[acolomb]

I just noticed that using these classes (just as the previous 24-bit only variants) would break with PDO that are not aligned to full bytes. That's because the code there actually examines the .format attribute, which is only available on the real Struct class:

canopen/canopen/pdo/base.py

Line 567 in 94f337d

if od_struct.format.islower() and (1 << (self.length - 1)) < data:

Let's add a read-only str property to forward .format to the self.struct.format instance variable. Or should we instead just derive these classes from struct.Struct?

[sveinse]

What exactly is this test doing? It tests that the lower-case of the struct format string is not empty? So is it then a method to determine all types that are struct native, i.e. not 24, 48, 56 and so on? This code is very strange to me. The comment sais "Check if the variable is signed and if the data is negative prepend signedness". I think I need some enlightenment here.

[acolomb]

The test hinges on the implementation detail that the struct mini-language uses uppercase codes (B, H, L, Q) for unsigned and lowercase (b, h, l, q) for signed types. It then goes on to check whether the value is in the upper half of the representable number space (with the specified number of bits), which indicates that it was a negative number being truncated. In that case, padding with 1-bits is added to the left to make it a truly negative integer (in two's complement) after converting back to the full-width integer type.

This is needed for partially mapped signed integer variables. Judging from the name, the STRUCT_TYPES listing was meant to indicate we do have a struct-based en- / decoder available, thus it can be assumed that the uppercase / lowercase distinction works.

I think the easiest way to fix this is to actually inherit from struct.Struct in your generic integer classes, so that we do have access to a real .format attribute again. Inheritance instead of composition, as an OOP specialist would put it.

[sveinse]

Ahhh. It sais islower() not lower()... Sorry, I misread it.

I agree, let's implement some upper case letter for unsigned and lower case for signed. I think simply proxying format to self.struct.format in the UnsignedN and SignedN object will suffice.

[acolomb]

I really prefer the inheritance approach, as it allows us to also type-annotate the STRUCT_TYPES mapping as dict[int, struct.Struct] and we don't need to list the IntegerN and UnsignedN classes anywhere. Should also simplify these classes a bit.

Should I take a stab at converting them? For example:

class UnsignedN(struct.Struct):
    """Packing and unpacking unsigned integers of arbitrary width, like struct.Struct.

    The width must be a multiple of 8 and must be between 8 and 64.
    """
    def __init__(self, width: int):
        self.width = width
        if width % 8 != 0:
            raise ValueError("Width must be a multiple of 8")
        if width <= 0 or width > 64:
            raise ValueError("Invalid width for UnsignedN")
        elif width <= 8:
            fmt = "B"
        elif width <= 16:
            fmt = "<H"
        elif width <= 32:
            fmt = "<L"
        else:
            fmt = "<Q"
        super().__init__(fmt)

    def unpack(self, buffer):
        return super().unpack(buffer + b'\x00' * (super().size - self.size))

    def pack(self, *v):
        return super().pack(*v)[:self.size]

    @property
    def size(self) -> int:
        return self.width // 8

[erlend-aasland]

struct.Struct is a base type; you can safely inherit from it. Builtin types and stdlib types that are not safe to inherit from are not base types (or should not be base types). See also:

• https://docs.python.org/3.14/c-api/typeobj.html#c.Py_TPFLAGS_BASETYPE

I see now that that page needs some care (yes, the triple question marks...)

[erlend-aasland]

FTR, the struct.Struct type spec can be found here:

https://github.com/python/cpython/blob/4055577221f5f52af329e87f31d81bb8fb02c504/Modules/_struct.c#L2239-L2246

[acolomb]

Wow, it never crossed my mind that a Python class in the stdlib could NOT be subclassed. If such a thing exists, I hope there is a big fat runtime or compiler warning, besides being clearly stated in the documentation.

[erlend-aasland]

You'll get a TypeError:

Python 3.13.0b1 (v3.13.0b1:2268289a47, May  8 2024, 06:41:53) [Clang 13.0.0 (clang-1300.0.29.30)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import datetime
>>> class C(datetime.timezone): ...
... 
Traceback (most recent call last):
  File "<python-input-1>", line 1, in <module>
    class C(datetime.timezone): ...
TypeError: type 'datetime.timezone' is not an acceptable base type
>>> 

For most user-visible types, there is a notice of some kind in the docs:

• https://docs.python.org/3/library/datetime.html#datetime.tzinfo

Wrong link, and more importantly, the docs do not mention it in this case.

[erlend-aasland]

Anyway (continuing the digression): most non-base types are not user visible. For example, the _struct.unpack_iter type:

Python 3.13.0b1 (v3.13.0b1:2268289a47, May  8 2024, 06:41:53) [Clang 13.0.0 (clang-1300.0.29.30)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import struct
>>> tp = type(struct.iter_unpack("b", b'x'))
>>> tp.__name__
'unpack_iterator'
>>> class C(tp): ...
... 
Traceback (most recent call last):
  File "<python-input-3>", line 1, in <module>
    class C(tp): ...
TypeError: type '_struct.unpack_iterator' is not an acceptable base type
>>> 

[acolomb]

@sveinse I feel pretty confident this is going in the right direction and will work well except for the failure #436, which we should postpone until after a short-term release. But if conflicts are resolved and the failing tests masked out (temporarily), IMHO we could target this for the upcoming release as well. The errors only do manifest under certain conditions, right? So we could at least deliver support for the additional types, even if still buggy in some edge-cases?

WDYT @christiansandberg?

[sveinse]

@acolomb It would be awesome to have this included in the release.

The failing tests only fail because of the lack of support for custom datatypes as per #436. It would be fine to disable these two tests for now and just state that canopen doesn't support custom types (as it never has).

I'm a bit afk this week, so if you'd like to add the struct proposal, please do so. I believe the PR is opened for maintainer push. Thanks!