diff --git a/src/opendisplay/models/config.py b/src/opendisplay/models/config.py index 1cf49f5..34e04a2 100644 --- a/src/opendisplay/models/config.py +++ b/src/opendisplay/models/config.py @@ -190,7 +190,7 @@ def from_bytes(cls, data: bytes) -> ManufacturerData: class PowerOption: """Power configuration (TLV packet type 0x04). - Size: 32 bytes (packed struct from firmware) + Size: 30 bytes (packed struct from firmware) """ power_mode: int # uint8 @@ -233,7 +233,7 @@ def has_battery_sense(self) -> bool: """Return True if device has a battery sense circuit.""" return self.battery_sense_pin != 0xFF - SIZE: ClassVar[int] = 32 + SIZE: ClassVar[int] = 30 @classmethod def from_bytes(cls, data: bytes) -> PowerOption: @@ -254,7 +254,7 @@ def from_bytes(cls, data: bytes) -> PowerOption: voltage_scaling_factor=int.from_bytes(data[12:14], "little"), deep_sleep_current_ua=int.from_bytes(data[14:18], "little"), deep_sleep_time_seconds=int.from_bytes(data[18:20], "little"), - reserved=data[20:32], + reserved=data[20:30], ) @@ -350,7 +350,7 @@ def rotation_enum(self) -> Rotation | int: except ValueError: return _INDEX_TO_ROTATION.get(self.rotation, self.rotation) - SIZE: ClassVar[int] = 66 + SIZE: ClassVar[int] = 46 @classmethod def from_bytes(cls, data: bytes) -> DisplayConfig: @@ -379,7 +379,7 @@ def from_bytes(cls, data: bytes) -> DisplayConfig: clk_pin=data[23], reserved_pins=data[24:31], # pins 2-8 full_update_mC=int.from_bytes(data[31:33], "little"), - reserved=data[33:66], + reserved=data[33:46], ) @@ -471,7 +471,7 @@ def from_bytes(cls, data: bytes) -> SensorData: class DataBus: """Data bus configuration (TLV packet type 0x24, repeatable max 4). - Size: 28 bytes (packed struct from firmware) + Size: 30 bytes (packed struct from firmware) """ instance_number: int # uint8 @@ -489,7 +489,7 @@ class DataBus: pulldowns: int # uint8 bitfield reserved: bytes # 14 bytes - SIZE: ClassVar[int] = 28 + SIZE: ClassVar[int] = 30 @classmethod def from_bytes(cls, data: bytes) -> DataBus: @@ -511,7 +511,7 @@ def from_bytes(cls, data: bytes) -> DataBus: bus_flags=data[13], pullups=data[14], pulldowns=data[15], - reserved=data[16:28], + reserved=data[16:30], ) @property diff --git a/src/opendisplay/protocol/config_parser.py b/src/opendisplay/protocol/config_parser.py index aa7df16..ed7c4f2 100644 --- a/src/opendisplay/protocol/config_parser.py +++ b/src/opendisplay/protocol/config_parser.py @@ -299,15 +299,11 @@ def _parse_manufacturer_data(data: bytes) -> ManufacturerData: if len(data) < 22: raise ConfigParseError(f"ManufacturerData too short: {len(data)} bytes (need 22)") - mfg_id, board_type, board_rev = struct.unpack_from(" PowerOption: @@ -331,7 +327,7 @@ def _parse_power_option(data: bytes) -> PowerOption: voltage_scaling_factor, deep_sleep_current_ua, deep_sleep_time_seconds, - ) = struct.unpack_from(" bytes: ) reserved = config.reserved if config.reserved else b"\x00" * 15 - data += reserved[:15] + data += reserved[:15].ljust(15, b"\x00") data += bytes([config.pwr_pin_2 & 0xFF, config.pwr_pin_3 & 0xFF]) return data @@ -138,7 +138,7 @@ def serialize_manufacturer_data(config: ManufacturerData) -> bytes: # Pad with reserved bytes to 22 total reserved = config.reserved if config.reserved else b"\x00" * 6 - return data + reserved[:6] + return data + reserved[:6].ljust(6, b"\x00") def serialize_power_option(config: PowerOption) -> bytes: @@ -148,7 +148,7 @@ def serialize_power_option(config: PowerOption) -> bytes: - power_mode: uint8 - battery_capacity_mah: 3 bytes (24-bit LE) - sleep_timeout_ms: uint16 - - tx_power: int8 (signed) + - tx_power: uint8 - sleep_flags: uint8 - battery_sense_pin: uint8 - battery_sense_enable_pin: uint8 @@ -176,9 +176,9 @@ def serialize_power_option(config: PowerOption) -> bytes: capacity_bytes = config.battery_capacity_mah.to_bytes(3, byteorder="little") data += capacity_bytes - # Pack remaining fields + # Pack remaining fields (tx_power is uint8, not int8) data += struct.pack( - " bytes: # Pad with reserved bytes to 30 total reserved = config.reserved if config.reserved else b"\x00" * 10 - return data + reserved[:10] + return data + reserved[:10].ljust(10, b"\x00") def serialize_display_config(config: DisplayConfig) -> bytes: @@ -253,7 +253,7 @@ def serialize_display_config(config: DisplayConfig) -> bytes: # Add reserved pins (7 bytes) reserved_pins = config.reserved_pins if config.reserved_pins else b"\xff" * 7 - data += reserved_pins[:7] + data += reserved_pins[:7].ljust(7, b"\xff") # full_update_mC (uint16 LE) sits between reserved_pins and reserved in the # firmware struct; omitting it truncates the packet and the device drops the display. @@ -297,7 +297,7 @@ def serialize_led_config(config: LedConfig) -> bytes: # Pad with reserved bytes to 22 total reserved = config.reserved if config.reserved else b"\x00" * 15 - return data + reserved[:15] + return data + reserved[:15].ljust(15, b"\x00") def serialize_sensor_data(config: SensorData) -> bytes: @@ -327,7 +327,7 @@ def serialize_sensor_data(config: SensorData) -> bytes: data += bytes([config.i2c_addr_7bit & 0xFF, config.msd_data_start_byte & 0xFF]) reserved = config.reserved if config.reserved else b"\x00" * 24 - return data + reserved[:24] + return data + reserved[:24].ljust(24, b"\x00") def serialize_data_bus(config: DataBus) -> bytes: @@ -369,7 +369,7 @@ def serialize_data_bus(config: DataBus) -> bytes: # Pad with reserved bytes to 30 total reserved = config.reserved if config.reserved else b"\x00" * 14 - return data + reserved[:14] + return data + reserved[:14].ljust(14, b"\x00") def serialize_binary_inputs(config: BinaryInputs) -> bytes: @@ -403,7 +403,7 @@ def serialize_binary_inputs(config: BinaryInputs) -> bytes: # Add reserved pins (8 bytes) reserved_pins = config.reserved_pins if config.reserved_pins else b"\x00" * 8 - data += reserved_pins[:8] + data += reserved_pins[:8].ljust(8, b"\x00") # Add flags data += struct.pack( @@ -419,7 +419,7 @@ def serialize_binary_inputs(config: BinaryInputs) -> bytes: # Pad with reserved bytes to 30 total reserved = config.reserved if config.reserved else b"\x00" * 14 - return data + reserved[:14] + return data + reserved[:14].ljust(14, b"\x00") def serialize_security_config(config: SecurityConfig) -> bytes: @@ -429,7 +429,7 @@ def serialize_security_config(config: SecurityConfig) -> bytes: data += config.session_timeout_seconds.to_bytes(2, "little") data += bytes([config.flags & 0xFF, config.reset_pin & 0xFF]) reserved = config.reserved if config.reserved else b"\x00" * 43 - return data + reserved[:43] + return data + reserved[:43].ljust(43, b"\x00") def serialize_touch_controller(config: TouchController) -> bytes: @@ -448,7 +448,7 @@ def serialize_touch_controller(config: TouchController) -> bytes: ) data += bytes([config.touch_data_start_byte & 0xFF]) reserved = config.reserved if config.reserved else b"\x00" * 21 - return data + reserved[:21] + return data + reserved[:21].ljust(21, b"\x00") def serialize_passive_buzzer(config: PassiveBuzzer) -> bytes: @@ -462,7 +462,7 @@ def serialize_passive_buzzer(config: PassiveBuzzer) -> bytes: config.duty_percent, ) reserved = config.reserved if config.reserved else b"\x00" * 27 - return data + reserved[:27] + return data + reserved[:27].ljust(27, b"\x00") def serialize_nfc_config(config: NfcConfig) -> bytes: @@ -487,7 +487,7 @@ def serialize_nfc_config(config: NfcConfig) -> bytes: config.reserved_pin_2, ) reserved = config.reserved if config.reserved else b"\x00" * 16 - return data + reserved[:16] + return data + reserved[:16].ljust(16, b"\x00") def serialize_flash_config(config: FlashConfig) -> bytes: @@ -508,7 +508,7 @@ def serialize_flash_config(config: FlashConfig) -> bytes: config.mode, ) reserved = config.reserved if config.reserved else b"\x00" * 20 - return data + reserved[:20] + return data + reserved[:20].ljust(20, b"\x00") def serialize_wifi_config(config: WifiConfig) -> bytes: diff --git a/tests/unit/test_config_serialization_fixes.py b/tests/unit/test_config_serialization_fixes.py new file mode 100644 index 0000000..3f64a3a --- /dev/null +++ b/tests/unit/test_config_serialization_fixes.py @@ -0,0 +1,80 @@ +"""Regression tests for config serialization correctness (C7, C8, M5, M6).""" + +from __future__ import annotations + +import dataclasses + +from opendisplay.models.config import DataBus, DisplayConfig, ManufacturerData, PowerOption +from opendisplay.protocol.config_parser import ( + _parse_manufacturer_data, + _parse_power_option, +) +from opendisplay.protocol.config_serializer import ( + serialize_data_bus, + serialize_manufacturer_data, + serialize_power_option, +) + + +# ── C7: ManufacturerData round-trip preserves simple-config metadata ────────── +def test_manufacturer_data_roundtrip_preserves_simple_config() -> None: + m = ManufacturerData( + manufacturer_id=0x2446, + board_type=1, + board_revision=2, + reserved=b"\xaa" * 6, + simple_config_driver_index=5, + simple_config_display_index=6, + simple_config_power_index=7, + simple_config_configured_at=123456, + ) + parsed = _parse_manufacturer_data(serialize_manufacturer_data(m)) + assert parsed.simple_config_driver_index == 5 + assert parsed.simple_config_display_index == 6 + assert parsed.simple_config_power_index == 7 + assert parsed.simple_config_configured_at == 123456 + assert parsed.reserved == b"\xaa" * 6 + + +# ── M5: public SIZE constants match the firmware structs ────────────────────── +def test_public_sizes_match_firmware() -> None: + assert PowerOption.SIZE == 30 + assert DisplayConfig.SIZE == 46 + assert DataBus.SIZE == 30 + + +def test_display_config_from_bytes_parses_46_byte_packet() -> None: + data = bytes(range(46)) + cfg = DisplayConfig.from_bytes(data) + assert cfg.reserved == data[33:46] + assert len(cfg.reserved) == 13 + + +def test_data_bus_from_bytes_yields_full_14_byte_reserved() -> None: + data = bytes(range(30)) + bus = DataBus.from_bytes(data) + assert len(bus.reserved) == 14 + assert bus.reserved == data[16:30] + + +# ── C8: serializers pad short reserved buffers to the fixed size ────────────── +def test_data_bus_serialize_pads_short_reserved_to_full_length() -> None: + bus = DataBus.from_bytes(bytes(30)) + short = dataclasses.replace(bus, reserved=b"\x01\x02") # only 2 bytes + out = serialize_data_bus(short) + assert len(out) == 30 # would be 18 without padding, desyncing the stream + + +# ── M6: tx_power is treated as unsigned end-to-end ──────────────────────────── +def test_tx_power_unsigned_roundtrip() -> None: + raw = bytearray(30) + raw[6] = 0xF4 # 244 unsigned; would be -12 if parsed signed + po = PowerOption.from_bytes(bytes(raw)) + assert po.tx_power == 244 + + parsed = _parse_power_option(bytes(raw)) + assert parsed.tx_power == 244 + + out = serialize_power_option(po) # must not raise struct.error + assert out[6] == 0xF4 + assert len(out) == 30