fix: validate config CRC with CRC-16/CCITT (advisory) to match toolbox/nRF/Silabs#83
Open
balloob wants to merge 1 commit into
Open
fix: validate config CRC with CRC-16/CCITT (advisory) to match toolbox/nRF/Silabs#83balloob wants to merge 1 commit into
balloob wants to merge 1 commit into
Conversation
…x/nRF/Silabs The on-wire toolbox-outer config container carries a trailing 2-byte CRC that was validated three different ways across the ecosystem. The website/toolbox (which configured most deployed displays), the nRF firmware and the Silabs firmware all use the canonical CRC-16/CCITT-FALSE over the container body with the first two (length) bytes forced to zero. This Arduino firmware was the outlier, validating with the low 16 bits of a CRC32. The team decided to standardize on CRC-16/CCITT. This moves the on-wire validation onto a new static config_toolbox_outer_crc16 helper that mirrors the nRF/Silabs implementation byte-for-byte (init 0xFFFF, poly 0x1021, MSB-first, no reflection, no final XOR, length bytes zeroed). The check stays advisory / warn-only exactly as before: a mismatch logs a WARNING and parsing continues; nothing is rejected. The unrelated internal flash-storage integrity CRC (calculateConfigCRC, CRC32) for the config_storage_t wrapper is deliberately left untouched -- it never leaves the device and its save/load call sites still use CRC32. 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
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
The on-wire toolbox-outer config container carries a trailing 2-byte CRC. Across the ecosystem this was validated three different ways. The website/toolbox (which configured most deployed displays), the nRF firmware, and the Silabs firmware all use the canonical CRC-16/CCITT-FALSE over the container body with the first two (length) bytes forced to zero. This Arduino firmware was the outlier: it validated with the low 16 bits of a CRC32.
Decision
Standardize the on-wire config-container CRC on CRC-16/CCITT, and keep it advisory (NOT enforced) — validation stays warn-only, exactly as it is today: it logs a warning on mismatch and continues parsing. CCITT was chosen because most deployed displays were configured by the website/toolbox, which writes CCITT, and the nRF/Silabs firmware and toolbox already use the canonical CCITT.
Fix
config_toolbox_outer_crc16(const uint8_t*, uint32_t)helper insrc/config_parser.cppthat mirrors the nRF/Silabs implementation byte-for-byte: init0xFFFF, poly0x1021, MSB-first, no reflection, no final XOR, over[byte0, byte1, version, ...packets]with the first two (length) bytes forced to zero, excluding the trailing 2 CRC bytes. Built on acrc16_ccitt_feedprimitive, matching the reference tree.calculateConfigCRC(...) & 0xFFFFcomputation at the on-wire validation site with a call to the new helper.WARNING: Config CRC mismatch (...)and parsing continues. Nothing is rejected.Scope note — the internal storage CRC32 is deliberately untouched
There are two distinct CRCs in this repo. Only the on-wire one changed. The internal flash-storage integrity CRC (
calculateConfigCRC, CRC32) used for theconfig_storage_twrapper on save/load never leaves the device and is unrelated to the on-wire config — it is left as CRC32, along with both of its call sites.Testing
00 00 01→0xDCBD2A 00 01 AA BB→0xC239[len_lo len_hi] 01 00 01+ fourteen00→0x0D7Ffor any length-byte values (proves zeroing).pio run -e nrf52840custom(SUCCESS) andpio run -e esp32-c3-N4(SUCCESS).Advisory, not enforced
This does not add any rejection path. A CRC mismatch remains a logged warning only; config parsing proceeds regardless.
Part of the config-CRC CCITT standardization (MJ-7); companion changes in OpenDisplay/py-opendisplay and OpenDisplay/opendisplay.org. Cross-links to be added.
🤖 Generated with Claude Code
https://claude.ai/code/session_012g2e8mr132vcizx92WsgiR
Related PRs — config-CRC CCITT standardization (finding MJ-7):
calculate_config_crc→ CCITTparseConfigBytesread-back (zero length bytes)Already canonical (no change): toolbox writer, nRF firmware, Silabs firmware.
opendisplay-js(TS) intentionally not updated (abandoned).