Skip to content

fix: config serialization correctness — padding, mfr metadata, sizes, tx_power (C7/C8/M5/M6)#100

Merged
g4bri3lDev merged 1 commit into
OpenDisplay:mainfrom
balloob:fix/config-serialization-correctness
Jul 5, 2026
Merged

fix: config serialization correctness — padding, mfr metadata, sizes, tx_power (C7/C8/M5/M6)#100
g4bri3lDev merged 1 commit into
OpenDisplay:mainfrom
balloob:fix/config-serialization-correctness

Conversation

@balloob

@balloob balloob commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Four related config wire-format correctness fixes.

C8 — pad fixed-size buffers on serialize (🔴)

Every serializer except display did reserved[:N] with no padding. The TLV format has no per-packet length — the firmware memcpys a fixed sizeof(struct) — so one short packet shifts and misparses everything after it on the device. All fixed-size reserved buffers are now ljust-padded to their exact width.

C7 — ManufacturerData round-trip corrupted bytes 4–21 (🔴)

_parse_manufacturer_data stored data[4:22] into reserved and left the simple-config fields at 0, but the serializer writes simple_config_driver/display/power_index + configured_at at offsets 4–15. A read-modify-write therefore zeroed the toolbox "simple config" metadata. Now delegates to ManufacturerData.from_bytes, which parses the layout correctly.

M5 — public from_bytes SIZE constants disagreed with firmware (🟠)

DisplayConfig.SIZE 66→46, PowerOption.SIZE 32→30, DataBus.SIZE 28→30. The main parse path used the correct private sizes, but the public APIs rejected/misparsed real packets, and DataBus.from_bytes produced a 12-byte reserved that fed the C8 short-packet bug. Sizes and reserved slices aligned to the firmware structs.

M6 — tx_power sign confusion (🟠)

Parsed signed in the BLE path, unsigned in from_bytes, packed signed on serialize (firmware field is uint8). 0xF4 became -12 one way and serializing 244 raised struct.error. Now unsigned end-to-end.

Test plan

  • uv run pytest -q → 445 passed (6 new tests: mfr round-trip, public sizes, DisplayConfig/DataBus from_bytes, short-reserved padding, tx_power unsigned round-trip)
  • ruff, mypy clean

🤖 Generated with Claude Code

… tx_power

C8: every serializer except display truncated fixed-size reserved buffers with
reserved[:N] and no padding. The TLV format has no per-packet length, so one
short packet shifts and misparses everything after it on the device. Pad all
fixed-size buffers on serialize (ljust to the exact width).

C7: _parse_manufacturer_data stored data[4:22] into reserved and left the
simple-config fields at 0; a read-modify-write then zeroed the toolbox
simple-config metadata at offsets 4-15. Delegate to ManufacturerData.from_bytes,
which parses the layout correctly.

M5: public from_bytes SIZE constants disagreed with the firmware structs
(DisplayConfig 66->46, PowerOption 32->30, DataBus 28->30), so the public APIs
misparsed real packets and DataBus.from_bytes produced a 12-byte reserved
(feeding the C8 short-packet bug). Align the sizes and reserved slices.

M6: tx_power was parsed signed in the BLE path, unsigned in from_bytes, and
packed signed on serialize (firmware field is uint8). 0xF4 became -12 one way
and serializing 244 raised struct.error. Treat as unsigned end-to-end.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JRrm95f1qNZzDM9r2SB6KW
@balloob balloob requested a review from g4bri3lDev as a code owner July 4, 2026 06:55
@codecov

codecov Bot commented Jul 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.88889% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/opendisplay/protocol/config_serializer.py 85.71% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

@g4bri3lDev g4bri3lDev merged commit 8bd3504 into OpenDisplay:main Jul 5, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants