diff --git a/src/packages/harp-device/src/harp/device/schema/_emit.py b/src/packages/harp-device/src/harp/device/schema/_emit.py index 4923e5c..9411de9 100644 --- a/src/packages/harp-device/src/harp/device/schema/_emit.py +++ b/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,8 +367,8 @@ 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 "") @@ -376,8 +376,8 @@ def _build_field(self, key: str, member: PayloadMember, reg: Register) -> Any: 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,8 +416,8 @@ 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) @@ -425,7 +425,7 @@ def _new_payload(self, class_name: str, owner: str, reg: Register) -> type: 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}) diff --git a/src/packages/harp-device/src/harp/device/schema/_model.py b/src/packages/harp-device/src/harp/device/schema/_model.py index 5e6ac67..03a7542 100644 --- a/src/packages/harp-device/src/harp/device/schema/_model.py +++ b/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." ) diff --git a/src/packages/harp-protocol/src/harp/protocol/_message.py b/src/packages/harp-protocol/src/harp/protocol/_message.py index 020a517..7ede863 100644 --- a/src/packages/harp-protocol/src/harp/protocol/_message.py +++ b/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( diff --git a/src/packages/harp-protocol/src/harp/protocol/_register.py b/src/packages/harp-protocol/src/harp/protocol/_register.py index 31109e1..35af47a 100644 --- a/src/packages/harp-protocol/src/harp/protocol/_register.py +++ b/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}), ) diff --git a/tests/device/test_device.py b/tests/device/test_device.py index e359175..9b46c97 100644 --- a/tests/device/test_device.py +++ b/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" diff --git a/tests/device/test_emit.py b/tests/device/test_emit.py index f51c4b4..d8ddd16 100644 --- a/tests/device/test_emit.py +++ b/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 # --------------------------------------------------------------------------- diff --git a/tests/device/test_schema.py b/tests/device/test_schema.py index bfda793..f2d77ad 100644 --- a/tests/device/test_schema.py +++ b/tests/device/test_schema.py @@ -1,8 +1,9 @@ import pytest from pydantic import ValidationError +from pydantic_yaml import to_yaml_str from harp.device.schema import parse_device_schema -from harp.device.schema._model import DeviceModel, PayloadType +from harp.device.schema._model import Access, DeviceModel, PayloadType, Register def test_parse_full_device(device_yml): @@ -92,3 +93,79 @@ def test_access_list_and_scalar(core_yml): c = parse_device_schema(core_yml) # TimestampSeconds has a list access [Read, Write, Event]; WhoAmI a scalar. assert isinstance(c.registers["TimestampSeconds"].access, list) + + +def test_absent_register_length_stays_absent(): + # An absent length means a single value. A declared 1 means an array of one element. The + # model has to keep them apart. + registers = parse_device_schema( + "registers:\n" + " Absent: {address: 32, type: U16, access: Read}\n" + " One: {address: 33, type: U16, length: 1, access: Read}\n" + ).registers + assert registers["Absent"].length is None + assert registers["One"].length == 1 + + +def test_absent_member_length_stays_absent(): + members = ( + parse_device_schema( + "registers:\n" + " R:\n" + " address: 32\n" + " type: U8\n" + " length: 4\n" + " access: Read\n" + " payloadSpec:\n" + " Absent: {offset: 0}\n" + " One: {offset: 1, length: 1}\n" + ) + .registers["R"] + .payloadSpec + ) + assert members is not None + assert members["Absent"].length is None + assert members["One"].length == 1 + + +@pytest.mark.parametrize( + "schema", + [ + "registers:\n R: {address: 32, type: U16, length: 0, access: Read}\n", + "registers:\n" + " R:\n" + " address: 32\n" + " type: U8\n" + " access: Read\n" + " payloadSpec:\n" + " Zero: {offset: 0, length: 0}\n", + ], + ids=["register", "member"], +) +def test_zero_length_is_rejected(schema): + # registers.json sets a minimum of 1 at both levels, so 0 is never declared. + with pytest.raises(ValidationError): + parse_device_schema(schema) + + +def test_serialized_schema_round_trips(device_yml): + # An absent length is None, so exclude_none writes a valid device.yml. + model = parse_device_schema(device_yml) + written = to_yaml_str(model, exclude_none=True) + assert "length:" in written # the declared ones survive + for name, register in model.registers.items(): + if register.length is not None: + assert f"length: {register.length}" in written, name + assert parse_device_schema(written) == model + + +def test_modified_schema_round_trips(device_yml): + # Read, change, write back. A register built in code has no length until one is set. + model = parse_device_schema(device_yml) + model.registers["DigitalInputs"].length = 4 + model.registers["Added"] = Register(address=60, type=PayloadType.U16, access=Access.Read) + written = to_yaml_str(model, exclude_none=True) + reparsed = parse_device_schema(written) + assert reparsed.registers["DigitalInputs"].length == 4 + assert reparsed.registers["Added"].length is None + assert reparsed == model diff --git a/tests/protocol/test_message.py b/tests/protocol/test_message.py index c27f20b..2ef9cf6 100644 --- a/tests/protocol/test_message.py +++ b/tests/protocol/test_message.py @@ -1,9 +1,12 @@ import struct +from typing import ClassVar import numpy as np import pytest from harp.protocol._message import HarpMessage, HarpParseError -from harp.protocol._register import RegisterU8, RegisterU16 +from harp.protocol._payload import Field, StructPayload +from harp.protocol._payload_converters import IdentityConverter +from harp.protocol._register import RegisterBase, RegisterU8, RegisterU16 from harp.protocol._message_type import MessageType from harp.protocol._payload_type import PayloadType @@ -136,3 +139,31 @@ def test_decode_accepts_register_at_another_address(): # The address says which register the device meant, so an identical layout decodes # either way and a frame can be read through more than one register. assert _u8_frame().decode(RegisterU8(0x2A)).payload == 5 + + +class _SettingsPayload(StructPayload[np.uint16], length=3): + gain: np.uint16 = Field(IdentityConverter(np.uint16)) + offset: np.uint16 = Field(IdentityConverter(np.uint16), offset=1) + threshold: np.uint16 = Field(IdentityConverter(np.uint16), offset=2) + + +class _Settings(RegisterBase[_SettingsPayload]): + address: ClassVar[int] = 32 + payload_type: ClassVar[PayloadType] = PayloadType.U16 + payload_class: ClassVar = _SettingsPayload + + +def test_decode_accepts_payload_spanning_several_elements(): + # decode sizes the payload from the payload class, as parse does, so several elements + # are not read as one. + payload = _SettingsPayload(gain=np.uint16(7), offset=np.uint16(8), threshold=np.uint16(9)) + typed = HarpMessage.parse(_Settings.format(payload)).decode(_Settings) + assert (int(typed.payload.gain), int(typed.payload.threshold)) == (7, 9) + + +def test_decode_rejects_payload_of_wrong_byte_count(): + # A read request carries no payload, so the count check stops a register reading + # past the frame. + request = HarpMessage.parse(_Settings.format(message_type=MessageType.Read)) + with pytest.raises(HarpParseError, match="reads 6 payload bytes but this message carries 0"): + request.decode(_Settings) diff --git a/tests/protocol/test_register.py b/tests/protocol/test_register.py index ccd4a88..ea115d8 100644 --- a/tests/protocol/test_register.py +++ b/tests/protocol/test_register.py @@ -293,6 +293,31 @@ def test_array_register_format_write(): assert msg.payload_bytes == values.tobytes() +def test_declared_length_sizes_array_payload(): + # The generator declares an array register by subclassing, with length in the class body. + # Inheriting the one-element base payload reads the first element and discards the rest. + class Attenuation(RegisterU16Array): + address: ClassVar[int] = 0x22 + length: ClassVar[int] = 3 + + called = RegisterU16Array(0x22, length=3) + assert Attenuation.payload_class.payload_dtype == called.payload_class.payload_dtype + values = np.array([10, 20, 30], dtype=np.uint16) + assert list(Attenuation.parse(_parse_frame(called.format(values)))) == [10, 20, 30] + + +def test_array_subclass_inherits_sized_payload(): + reg = RegisterU16Array(0x22, length=3) + assert type("Aux", (reg,), {}).payload_class is reg.payload_class + + +def test_redeclared_length_raises_type_error(): + # Sizing an already-sized payload would nest the sub-arrays. + reg = RegisterU16Array(0x22, length=3) + with pytest.raises(TypeError, match="redeclares a length"): + type("Wider", (reg,), {"length": 4}) + + def test_array_register_parse_roundtrip(): reg = RegisterU32Array(0x08, length=3) values = np.array([10, 20, 30], dtype=np.dtype("