Fix config parser dropping packets after unknown TLV types#1
Open
balloob wants to merge 1 commit into
Open
Conversation
The TLV config parser aborted on the first packet ID it did not recognize by jumping straight to the CRC (offset = configLen - 2), silently dropping every packet that followed. nRF had no case for packet IDs 0x28 (touch), 0x29 (buzzer), 0x2A (nfc), 0x2B (flash) or 0x2C (data_extended), so any config containing one of these (e.g. touch emitted before other packets) truncated parsing. Add cases that skip these unused packet types by their correct on-wire size and continue, so subsequent packets are still parsed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012g2e8mr132vcizx92WsgiR
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
parseConfigBytes()inconfig_parser.cwalks the TLV config as aswitchonpacketId, and itsdefaultcase doesoffset = configLen - 2(jump to the CRC) and stops. So on the first packet ID it doesn't recognize, it silently drops every packet that follows.nRF has no case for these packet IDs:
The client emits touch (0x28) before other packets, so a config with touch present makes nRF abort at 0x28 and lose the display/LED/sensor/etc. packets after it — the device parses an incomplete config.
Fix
Add cases for these five IDs that skip past the packet by its correct on-wire size and continue the loop, instead of aborting. nRF does not functionally use these features, so skip-by-size is sufficient to stop the truncation.
defaultremains as the last-resort abort.Sizes (all packed structs, cross-referenced)
struct TouchController(structs.h); clientserialize_touch_controllerstruct PassiveBuzzerConfig; clientserialize_passive_buzzerstruct NfcConfig(opendisplay_structs.h); clientserialize_nfc_configstruct FlashConfig; clientserialize_flash_configstruct DataExtended(9 × 32); clientserialize_data_extendedTesting
The nRF ARM toolchain (
arm-none-eabi-gcc) is not available in my environment, so the full firmware image was not built. I verified the parse-loop logic instead with a standalone host-gcc simulation reproducing the loop: a config containing touch (0x28) + flash (0x2B) + data_extended (0x2C) followed by a display packet now parses all packets and reaches the trailing display packet, where previously it aborted at 0x28. Types, struct sizes, and the newCONFIG_PKT_*constants were checked manually against the existing code.Related
The Silabs firmware has the analogous gap (no case for 0x28/0x29/0x2C); a companion PR addresses that side.
🤖 Generated with Claude Code
https://claude.ai/code/session_012g2e8mr132vcizx92WsgiR
Companion PR: OpenDisplay/Firmware_Silabs#6 (same MJ-8 fix, Silabs side — 0x28/0x29/0x2C). Sizes cross-validated: both derived 0x28=32, 0x29=32, 0x2C=288 independently.