Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement the remaining canopen datatypes #440

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 18 additions & 7 deletions canopen/objectdictionary/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
import logging

from canopen.objectdictionary.datatypes import *
from canopen.objectdictionary.datatypes_24bit import Integer24, Unsigned24
sveinse marked this conversation as resolved.
Show resolved Hide resolved

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -281,16 +280,24 @@ class ODVariable:
"""Simple variable."""

STRUCT_TYPES = {
# Use struct module to pack/unpack data where possible and use the
# custom IntegerN and UnsignedN classes for the special data types.
BOOLEAN: struct.Struct("?"),
INTEGER8: struct.Struct("b"),
INTEGER16: struct.Struct("<h"),
INTEGER24: Integer24(),
INTEGER24: IntegerN(24),
INTEGER32: struct.Struct("<l"),
INTEGER40: IntegerN(40),
INTEGER48: IntegerN(48),
INTEGER56: IntegerN(56),
INTEGER64: struct.Struct("<q"),
UNSIGNED8: struct.Struct("B"),
UNSIGNED16: struct.Struct("<H"),
UNSIGNED24: Unsigned24(),
UNSIGNED24: UnsignedN(24),
UNSIGNED32: struct.Struct("<L"),
UNSIGNED40: UnsignedN(40),
UNSIGNED48: UnsignedN(48),
UNSIGNED56: UnsignedN(56),
UNSIGNED64: struct.Struct("<Q"),
REAL32: struct.Struct("<f"),
REAL64: struct.Struct("<d")
Expand Down Expand Up @@ -384,10 +391,13 @@ def add_bit_definition(self, name: str, bits: List[int]) -> None:

def decode_raw(self, data: bytes) -> Union[int, float, str, bytes, bytearray]:
if self.data_type == VISIBLE_STRING:
return data.rstrip(b"\x00").decode("ascii", errors="ignore")
# Strip any trailing NUL characters from C-based systems
return data.decode("ascii", errors="ignore").rstrip("\x00")
elif self.data_type == UNICODE_STRING:
# Is this correct?
return data.rstrip(b"\x00").decode("utf_16_le", errors="ignore")
# The canopen standard does not specify the encoding. This
# library assumes utf-16-le, being the most common encoding format.
sveinse marked this conversation as resolved.
Show resolved Hide resolved
# Strip any trailing NUL characters from C-based systems
return data.decode("utf_16_le", errors="ignore").rstrip("\x00")
elif self.data_type in self.STRUCT_TYPES:
try:
value, = self.STRUCT_TYPES[self.data_type].unpack(data)
Expand All @@ -405,8 +415,9 @@ def encode_raw(self, value: Union[int, float, str, bytes, bytearray]) -> bytes:
elif self.data_type == VISIBLE_STRING:
return value.encode("ascii")
elif self.data_type == UNICODE_STRING:
# Is this correct?
return value.encode("utf_16_le")
elif self.data_type in (DOMAIN, OCTET_STRING):
return bytes(value)
elif self.data_type in self.STRUCT_TYPES:
if self.data_type in INTEGER_TYPES:
value = int(value)
Expand Down
78 changes: 76 additions & 2 deletions canopen/objectdictionary/datatypes.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import struct

BOOLEAN = 0x1
INTEGER8 = 0x2
Expand All @@ -10,16 +11,89 @@
VISIBLE_STRING = 0x9
OCTET_STRING = 0xA
UNICODE_STRING = 0xB
TIME_OF_DAY = 0xC
TIME_DIFFERENCE = 0xD
DOMAIN = 0xF
INTEGER24 = 0x10
REAL64 = 0x11
INTEGER40 = 0x12
INTEGER48 = 0x13
INTEGER56 = 0x14
INTEGER64 = 0x15
UNSIGNED24 = 0x16
UNSIGNED40 = 0x18
UNSIGNED48 = 0x19
UNSIGNED56 = 0x1A
UNSIGNED64 = 0x1B
PDO_COMMUNICATION_PARAMETER = 0x1C
PDO_MAPPING = 0x1D
SDO_PARAMETER = 0x1E
sveinse marked this conversation as resolved.
Show resolved Hide resolved

SIGNED_TYPES = (INTEGER8, INTEGER16, INTEGER24, INTEGER32, INTEGER64)
UNSIGNED_TYPES = (UNSIGNED8, UNSIGNED16, UNSIGNED24, UNSIGNED32, UNSIGNED64)
SIGNED_TYPES = (INTEGER8, INTEGER16, INTEGER24, INTEGER32, INTEGER40, INTEGER48, INTEGER56, INTEGER64)
UNSIGNED_TYPES = (UNSIGNED8, UNSIGNED16, UNSIGNED24, UNSIGNED32, UNSIGNED40, UNSIGNED48, UNSIGNED56, UNSIGNED64)
INTEGER_TYPES = SIGNED_TYPES + UNSIGNED_TYPES
FLOAT_TYPES = (REAL32, REAL64)
NUMBER_TYPES = INTEGER_TYPES + FLOAT_TYPES
DATA_TYPES = (VISIBLE_STRING, OCTET_STRING, UNICODE_STRING, DOMAIN)


class UnsignedN:
Copy link
Collaborator

@acolomb acolomb Jun 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@acolomb acolomb Jun 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@erlend-aasland erlend-aasland Jun 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
>>> 

""" struct-like class for packing and unpacking unsigned integers of arbitrary width.
The width must be a multiple of 8 and must be between 8 and 64.
"""
sveinse marked this conversation as resolved.
Show resolved Hide resolved
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:
self.struct = struct.Struct("B")
elif width <= 16:
self.struct = struct.Struct("<H")
elif width <= 32:
self.struct = struct.Struct("<L")
else:
self.struct = struct.Struct("<Q")

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

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

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


class IntegerN:
""" struct-like class for packing and unpacking integers of arbitrary width.
The width must be a multiple of 8 and must be between 8 and 64.
"""
sveinse marked this conversation as resolved.
Show resolved Hide resolved
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 IntegerN")
elif width <= 8:
self.struct = struct.Struct("b")
elif width <= 16:
self.struct = struct.Struct("<h")
elif width <= 32:
self.struct = struct.Struct("<l")
else:
self.struct = struct.Struct("<q")

def unpack(self, buffer):
mask = 0x80
neg = (buffer[self.size - 1] & mask) > 0
return self.struct.unpack(buffer + (b'\xff' if neg else b'\x00') * (self.struct.size - self.size))

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

@property
def size(self):
return self.width // 8
33 changes: 0 additions & 33 deletions canopen/objectdictionary/datatypes_24bit.py

This file was deleted.

Loading
Loading