Fix decoding of multi-element register payloads (#44)

decode now sizes the payload from payload_class rather than from the
register length, which only an array register declares, so a payload
spanning several elements is no longer rejected. That covers every
struct, string and mask register, DeviceName included. An array register
declaring length in its class body now sizes its payload too, where
before only the call form did and it read the first element only.

A declared length of 1 is distinct from an absent one, so it emits a
one-element array rather than a scalar, and length: 0 is rejected for a
register and a payload member. length is no longer a member of every
register, moving to the array register metaclass, and
ConverterContext.length is never zero, reporting the effective element
count.

Closes #36

Author: glopesdev

src/packages/harp-device/src/harp/device/schema/_emit.py

@@ -104,14 +104,14 @@ class ConverterContext:
     name: str  # yml field key ("__value__" for a whole-register value)
     interface_type: Optional[str]  # the DSL interfaceType (None = raw/native)
     mask: Optional[int]  # bit mask, when the value is bit-packed
-    length: int  # element count this value spans (0 = unset -> scalar)
+    length: int  # register elements this value spans, at least one
     element: np.dtype  # base element dtype of the register, from PayloadType
     element_size: int  # base element byte size of the register
 
     @property
     def span(self) -> int:
-        """Byte span of the value (element count * element size)."""
-        return max(1, self.length) * self.element_size
+        """Byte span of the value, its element count times the element size."""
+        return self.length * self.element_size
 
     @property
     def member_dtype(self) -> np.dtype:
@@ -343,7 +343,7 @@ def _extension(self, symbol: str, ctx: ConverterContext) -> Converter[Any]:
     def _default(self, member: PayloadMember, type_name: str, ctx: ConverterContext) -> Any:
         """The typed default value of the field, or ``_NO_DEFAULT`` when it has none."""
         _default_value = member.defaultValue if member.defaultValue is not None else member.minValue
-        if _default_value is None or (member.length or 0) > 1:
+        if _default_value is None or ctx.length > 1:
             return _NO_DEFAULT
         value = float(_default_value.root)
         group_mask = self._find_mask(type_name)
@@ -367,17 +367,17 @@ def _build_field(self, key: str, member: PayloadMember, reg: Register) -> Any:
         # ``key`` stays the verbatim yml name: it feeds ``ConverterContext.name``, and
         # a custom converter symbol is derived from the pre-rename key ("Data" ->
         # "DataConverter"). The renamed attribute name is applied by the caller.
-        elem_np = _ELEMENT[reg.type]
-        elem_size = np.dtype(elem_np).itemsize
+        elem = np.dtype(_ELEMENT[reg.type])
+        elem_size = elem.itemsize
         offset = member.offset or 0
         it = member.interfaceType.root if member.interfaceType else None
         type_name = it or (member.maskType.root if member.maskType else "")
         ctx = ConverterContext(
             name=key,
             interface_type=it,
             mask=member.mask,
-            length=member.length or 0,
-            element=np.dtype(elem_np),
+            length=member.length or 1,
+            element=elem,
             element_size=elem_size,
         )
         default = self._default(member, type_name, ctx)
@@ -416,16 +416,16 @@ def _build_payload(self, name: str, reg: Register) -> type:
 
     def _new_payload(self, class_name: str, owner: str, reg: Register) -> type:
         elem_np = _ELEMENT[reg.type]
-        elem_size = np.dtype(elem_np).itemsize
-        length = reg.length or 1
+        elem = np.dtype(elem_np)
+        elem_size = elem.itemsize
 
         if reg.payloadSpec is not None:
             renamed = self._rename("field", owner, reg.payloadSpec, field_name, reserved=True)
             namespace = {
                 renamed[key]: self._build_field(key, member, reg)
                 for key, member in reg.payloadSpec.items()
             }
-            kwds = {"length": length} if length > 1 else {}
+            kwds = {"length": reg.length}
             return _new_class(class_name, (StructPayload[elem_np],), namespace, kwds)
 
         # anonymous single-value payload
@@ -451,8 +451,8 @@ def _new_payload(self, class_name: str, owner: str, reg: Register) -> type:
                 name="__value__",
                 interface_type=it,
                 mask=None,
-                length=length,
-                element=np.dtype(elem_np),
+                length=reg.length or 1,
+                element=elem,
                 element_size=elem_size,
             )
             descriptor = Field(self._resolve_converter(ctx))
@@ -464,7 +464,6 @@ def _class_name(self, name: str, reg: Register) -> str:
         return f"_{name}" if reg.visibility is Visibility.private else name
 
     def _build_register(self, name: str, class_name: str, reg: Register) -> type[RegisterBase[Any]]:
-        length = reg.length or 1
         it = reg.interfaceType.root if reg.interfaceType else None
 
         # A plain scalar/array register needs no payload wrapper: its whole value is a
@@ -475,8 +474,8 @@ def _build_register(self, name: str, class_name: str, reg: Register) -> type[Reg
             and reg.converter is None
             and _is_native(it)
         ):
-            if length > 1:  # plain array register
-                cls = _ARRAY_REGISTER[reg.type](reg.address, length=length)
+            if reg.length is not None:  # plain array register
+                cls = _ARRAY_REGISTER[reg.type](reg.address, length=reg.length)
                 cls.__name__ = cls.__qualname__ = class_name
                 return cls
             return _new_class(class_name, (_SCALAR_REGISTER[reg.type],), {"address": reg.address})

src/packages/harp-device/src/harp/device/schema/_model.py

@@ -126,9 +126,10 @@ class PayloadMember(BaseModel):
         None,
         description="The zero-based index at which encoding of this payload member starts.",
     )
-    length: Optional[int] = Field(
-        None, description="The number of elements used to encode this payload member."
-    )
+    length: Annotated[
+        Optional[int],
+        Field(ge=1, description="The number of elements used to encode this payload member."),
+    ] = None
     description: Optional[str] = Field(
         None, description="A summary description of this payload member."
     )
@@ -164,8 +165,8 @@ class Register(BaseModel):
     address: Annotated[int, Field(le=255, description="The unique 8-bit address of the register.")]
     type: PayloadType = Field(..., description="The type of the register payload.")
     length: Annotated[
-        Optional[int], Field(ge=1, default=1, description="The length of the register payload.")
-    ]
+        Optional[int], Field(ge=1, description="The length of the register payload.")
+    ] = None
     access: Union[Access, List[Access]] = Field(
         ..., description="The expected use of the register."
     )

src/packages/harp-protocol/src/harp/protocol/_message.py

@@ -3,7 +3,6 @@
 import struct
 from typing import Any, ClassVar, Generic, Protocol, TypeVar, cast
 
-import numpy as np
 from typing_extensions import Sentinel
 
 from ._builder import build_message_frame
@@ -18,6 +17,7 @@
     _TIMESTAMPED_PAYLOAD_OFFSET,
 )
 from ._message_type import MessageType, _message_type_from_byte_safe
+from ._payload import PayloadBase
 from ._payload_type import PayloadType, decode_payload_type
 
 P = TypeVar("P")
@@ -38,14 +38,14 @@ class PayloadDecoder(Protocol[_P_co]):
     """Reads a payload of type ``_P_co`` out of a message.
 
     Structural rather than nominal, so a message never has to know about registers, and
-    anything declaring a payload type, a length and a ``parse`` satisfies it. Every
-    ``RegisterBase`` does. ``length`` is the element count, or ``None`` for a single
-    value, and together with ``payload_type`` it fixes how many payload bytes the
-    decoder consumes.
+    anything declaring a payload type, a payload class and a ``parse`` satisfies it. Every
+    ``RegisterBase`` does. ``payload_class.payload_dtype`` is what fixes how many payload
+    bytes the decoder consumes, and it is the same quantity ``parse`` reads the frame
+    with, so the two cannot disagree about the extent of a payload.
     """
 
     payload_type: ClassVar["PayloadType"]
-    length: ClassVar[int | None]
+    payload_class: ClassVar[type[PayloadBase[Any]]]
 
     @classmethod
     def parse(cls, value: Any) -> _P_co: ...
@@ -187,7 +187,7 @@ def decode(self, decoder: type[PayloadDecoder[_P]]) -> "HarpMessage[_P]":
                 f"{decoder.__name__} declares {decoder.payload_type!r} but this "
                 f"message declares {self.payload_type!r}."
             )
-        expected = (decoder.length or 1) * np.dtype(decoder.payload_type.value).itemsize
+        expected = decoder.payload_class.payload_dtype.itemsize
         actual = len(self.payload_bytes)
         if actual != expected:
             raise HarpParseError(

src/packages/harp-protocol/src/harp/protocol/_register.py

@@ -18,6 +18,7 @@
 from ._message import HarpMessage, HarpParseError
 from ._message_type import MessageType, message_type_to_byte
 from ._payload import (
+    AnonymousPayload,
     Batch,
     PayloadBase,
     PayloadFloat,
@@ -148,14 +149,13 @@ class RegisterBase(ABC, Generic[U], metaclass=_RegisterBaseMeta):
       or ``RegisterBase[ClockConfigurationFlags]`` for a whole-register
       ``BitMask`` or ``GroupMask``, even though each still has a ``payload_class``.
 
-    Subclasses must define ``address``, ``payload_type``, and
-    ``payload_class`` as ``ClassVar``s.
+    Subclasses must define ``address``, ``payload_type``, and ``payload_class`` as
+    ``ClassVar``s. The extent of a payload is always read from ``payload_class``.
     """
 
     address: ClassVar[int]
     payload_type: ClassVar[PayloadType]
     payload_class: ClassVar[type[PayloadBase[Any]]]
-    length: ClassVar[int | None] = None
 
     @classmethod
     def parse(cls, value: HarpMessage | bytes | bytearray | memoryview) -> U:
@@ -435,32 +435,40 @@ class RegisterFloat(RegisterBase[np.float32], metaclass=_ScalarRegisterMeta):
 
 
 class _ArrayRegisterMeta(_RegisterBaseMeta):
-    """A base metaclass for array registers. Calling with address and length creates a concrete subclass: ``RegisterU16Array(0x28, length=3)``."""
+    """A declared ``length`` sizes the payload, and calling a register base with an address
+    and a length creates a one-off subclass: ``RegisterU16Array(0x28, length=3)``.
 
-    def __call__(cls: "type[_AR]", address: int, *, length: int) -> "type[_AR]":  # type: ignore[override, misc]
-        _require_no_address(cls)
-        base_payload = cls.payload_class  # type: ignore[attr-defined]
-        # Anonymous payloads carry a plain (non-structured) dtype. The array
-        # variant uses a sub-dtype (inner_dtype, (length,)) so a single buffer
-        # element decodes directly to an ndarray of shape (length,).
-        inner = base_payload.payload_dtype
-        sub_dtype = np.dtype((inner, (length,)))
-        concrete_payload = type(
+    ``length`` is declared here rather than on ``RegisterBase``, so only an array register
+    carries one. It is the element count, and nothing reads it to size a payload.
+    """
+
+    length: int
+    payload_class: type[AnonymousPayload[Any]]
+
+    def __init__(
+        cls, name: str, bases: tuple[type, ...], namespace: dict[str, Any], **kwargs: Any
+    ) -> None:
+        super().__init__(name, bases, namespace, **kwargs)
+        # The namespace holds this class body only, not inherited values, so a plain
+        # subclass reads None and keeps the payload already sized by its base.
+        length = namespace.get("length")
+        if length is None:
+            return
+        base_payload = cls.payload_class
+        if base_payload.payload_dtype.subdtype is not None:
+            raise TypeError(f"{name} redeclares a length already applied by its base class.")
+        # A sub-array dtype, so reading one buffer element gives an ndarray of that shape.
+        cls.payload_class = type(
             f"{base_payload.__name__}_{length}",
             (base_payload,),
-            {"payload_dtype": sub_dtype},
+            {"payload_dtype": np.dtype((base_payload.payload_dtype, (length,)))},
         )
+
+    def __call__(cls: "type[_AR]", address: int, *, length: int) -> "type[_AR]":  # type: ignore[override, misc]
+        _require_no_address(cls)
         return cast(
             "type[_AR]",
-            type(
-                f"{cls.__name__}_{address:#04x}",
-                (cls,),
-                {
-                    "address": address,
-                    "length": length,
-                    "payload_class": concrete_payload,
-                },
-            ),
+            type(f"{cls.__name__}_{address:#04x}", (cls,), {"address": address, "length": length}),
         )
 
 

tests/device/test_device.py

@@ -176,3 +176,21 @@ def test_error_reply_returned_when_not_raising():
         reply = device.read(core.WhoAmI)
     assert reply.has_error
     assert int(reply.payload) == 7
+
+
+def test_read_multi_element_register_returns_payload():
+    # read decodes the reply through the register, so a payload of several elements has to
+    # survive that step.
+    transport = _ScriptedTransport()
+    transport.on_write = lambda _: (
+        core.DeviceName.format("Behavior", message_type=MessageType.Read),
+    )
+    with _ShortTimeoutDevice(transport) as device:
+        assert device.read(core.DeviceName).payload == "Behavior"
+
+
+def test_write_multi_element_register_returns_payload():
+    transport = _ScriptedTransport()
+    transport.on_write = lambda data: (data,)  # a device echoing the write
+    with _ShortTimeoutDevice(transport) as device:
+        assert device.write(core.DeviceName, "Behavior").payload == "Behavior"

tests/device/test_emit.py

@@ -136,6 +136,62 @@ def test_enum_names_match_generator_for_every_enum(device_registers):
             }
 
 
+# ---------------------------------------------------------------------------
+# The declared length decides whether a register is an array
+# ---------------------------------------------------------------------------
+
+_LENGTH_YML = """
+registers:
+  Absent: {address: 32, type: U16, access: Read}
+  One: {address: 33, type: U16, length: 1, access: Read}
+  Three: {address: 34, type: U16, length: 3, access: Read}
+"""
+
+
+def test_absent_length_emits_scalar_register():
+    # A register with no declared length holds a single value, and carries no length at all.
+    reg = create_registers(_LENGTH_YML)["Absent"]
+    assert not hasattr(reg, "length")
+    assert reg.payload_class.payload_dtype.shape == ()
+
+
+@pytest.mark.parametrize("name, count", [("One", 1), ("Three", 3)])
+def test_declared_length_emits_array_register(name, count):
+    # Any declared length means an array, 1 included. Reading a declared 1 as a single value
+    # gives the same type as declaring nothing, and the generator would then disagree.
+    reg = create_registers(_LENGTH_YML)[name]
+    assert reg.length == count
+    assert reg.payload_class.payload_dtype.shape == (count,)
+    values = np.arange(count, dtype=np.uint16)
+    np.testing.assert_array_equal(reg.parse(HarpMessage.parse(reg.format(values))), values)
+
+
+def test_multi_element_struct_register_decodes_through_message():
+    # A struct register declares no length, so decode has to size its payload from the
+    # payload class.
+    reg = create_registers(
+        "registers:\n"
+        "  Settings:\n"
+        "    address: 32\n"
+        "    type: U16\n"
+        "    length: 3\n"
+        "    access: Write\n"
+        "    payloadSpec:\n"
+        "      Gain: {offset: 0}\n"
+        "      Offset: {offset: 1}\n"
+        "      Threshold: {offset: 2}\n"
+    )["Settings"]
+    payload = reg.payload_class(gain=1, offset=2, threshold=3)
+    decoded = HarpMessage.parse(reg.format(payload)).decode(reg)
+    assert (int(decoded.payload.gain), int(decoded.payload.threshold)) == (1, 3)
+
+
+def test_core_string_register_decodes_through_message():
+    # DeviceName spans 25 U8 elements, so the same defect reached every Harp device.
+    frame = core.DeviceName.format("Behavior")
+    assert HarpMessage.parse(frame).decode(core.DeviceName).payload == "Behavior"
+
+
 # ---------------------------------------------------------------------------
 # Core registers: the emitter and the generated package, from the same core.yml
 # ---------------------------------------------------------------------------