Skip to content

fix: write_config first chunk must carry 200 data bytes (C4)#99

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

fix: write_config first chunk must carry 200 data bytes (C4)#99
g4bri3lDev merged 1 commit into
OpenDisplay:mainfrom
balloob:fix/write-config-chunking

Conversation

@balloob

@balloob balloob commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Summary

The chunked write_config first packet (build_write_config_command) sent total_size(2) + 198 data = 200 payload bytes. But the firmware:

  • enters chunked mode only when the payload length exceeds 200, and
  • expects the first chunk to be exactly [total:2LE][200 data] = 202 payload bytes.

With a 200-byte payload, the firmware took the single-chunk path and saveConfig()'d the raw 200 bytes (the 2-byte size header plus a truncated config stored as the whole config), then NACKed every subsequent 0x42 chunk. The 198-byte first chunk also broke the firmware's expectedChunks = ceil(total / 200) accounting.

Since a realistic full config (system + manufacturer + power + display + …) exceeds 200 bytes, write_config was broken against this firmware for most real configs.

Fix

The first chunk now carries a full 200 data bytes (payload = size(2) + 200 = 202), and continuations remain 200 bytes each — so the firmware enters chunked mode and its ceil(total/200) accounting matches.

Test plan

  • uv run pytest -q → 442 passed (3 new tests: single-chunk boundary, first chunk carries 200 data bytes, chunk count == ceil(total/200))
  • ruff, mypy clean

🤖 Generated with Claude Code

The chunked write_config first packet sent total_size(2) + 198 data = 200
payload bytes. Firmware enters chunked mode only when the payload length
exceeds 200 and expects [total:2LE][200 data] = 202 payload bytes. With 200
bytes it took the single-chunk path and saveConfig()'d the 2-byte size header
plus a truncated config, then NACKed every following 0x42 chunk; the 198-byte
first chunk also broke its expectedChunks = ceil(total/200) accounting. Since
a realistic full config exceeds 200 bytes, write_config was broken for most
real configs. Send a full 200 data bytes in the first chunk.

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:50
@balloob

balloob commented Jul 4, 2026

Copy link
Copy Markdown
Contributor Author

Independently re-verified this finding by simulating the firmware's handleWriteConfig/handleWriteConfigChunk logic (communication.cpp:357-443) line-by-line against the library's actual output — worth writing up because "the firmware works today" is also true.

Why this never failed in practice: typical configs never reach the chunked path. Serialized with this library: system + manufacturer + power + display = 133 bytes, plus an LED = 157 bytes — both ≤ 200, so they take the single-chunk path, which was always correct. The chunked path only engages above 200 bytes (e.g. any config with a data_extended packet — 288 bytes on its own — or wifi at 160 bytes). The bug was real but latent.

The load-bearing firmware detail: the dispatcher calls handleWriteConfig(data + 2, len - 2) (communication.cpp:549), so len excludes the 2 command bytes. The old code's 202-byte ATT packet therefore arrives as exactly a 200-byte payload, and the chunked-mode gate is len > 200 (communication.cpp:368) — not met.

Broken case, demonstrated (old code, 423-byte config with data_extended): firmware takes the single-chunk path and saveConfig()s 200 bytes beginning a7 01 — the size prefix 423 LE stored as config data — then NACKs both following 0x42 chunks with {FF,42,00,00}.

This PR's fix, verified against the same simulation: round-trips byte-identically at 201, 202, 250, 400, 401, 600, 3800, and 4000 bytes.

One follow-up worth noting: the firmware rejects the 21st packet (receivedChunks >= 20, communication.cpp:431), so the maximum writable config over chunking is 4000 bytes, not 4096. A 4096-byte config (which the serializer allows) dies mid-stream with a generic NACK. A pre-check in write_config raising a clear error above 4000 bytes would round this out — fine as a separate PR.

@codecov

codecov Bot commented Jul 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@g4bri3lDev g4bri3lDev merged commit 388a372 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