fix: release-review findings for 4.1.0 — BLE settings list, LED rail, strict setting parse - #151
Merged
Merged
Conversation
…lease notes FIRMWARE_VERSION 4.0.0 -> 4.1.0 (matches the v4.1.0 tag to come). MINOR is right: new settings and device behaviour, nothing removed and no format break — track files, the DOVEX layout, log filenames and the BLE command protocol are byte-for-byte what 4.0.0 wrote. CHANGELOG: [Unreleased] had accreted the same shape as last cut — two separate "### Changed" headings and a trailing "### Added" after "### Fixed" from successive merges. Consolidated into one Added/Changed/Fixed set under [4.1.0] - 2026-08-22. All 20 entries and every body line moved VERBATIM (verified by diffing the sorted line multiset before and after); only the five entries below gained a channel marker. The marker is the substantive change here. Most of this release's headline work — the whole NeoPixel subsystem — is behind BIRDSEYE_ENABLE_NEOPIXEL, which is off in the published images, so a reader of these notes would otherwise go looking for LEDs that a release build never lights. Each affected entry is now marked *(beta channel)*, and the section intro says plainly which half of the release is live for every user. Two plan-0007 behaviours are NOT gated and were being described as if they were: the tach page's corrected *OVER REV* header (display_pages.ino reads settingOverrevLimit outside any #if) and the pace page's STOPPED state (raceEngineStopped(), same) both ship active — those entries now say *(every build)* on that half and *(beta channel)* on the LED half. Link refs: [Unreleased] was still comparing from v3.1.0, stale since the 4.0.0 cut, and [4.0.0] never got a ref at all. Both fixed, [4.1.0] added. DovesLapTimer pins are deliberately untouched. compile-sketch.yml warns that promoting the channel means bumping them, but the library's BETA branch is source-identical to the v4.3.0 tag master already pins (the only difference between the two refs is five CI workflow files), so there is nothing to bump. Verified: 507 host test cases / 325,581 assertions pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U6kd8xwnUbXyd2Ce6m8ydX
Coverage — host-testable units📂 Overall coverage
📄 File coverage
|
…EL defaults to 1 The LED strip stops being a beta-only build and becomes a core feature in 4.1.0: a stock logger drives a strip the moment someone wires one, with no special firmware. The flag's #ifndef default in project.h goes 0 -> 1, which is the whole mechanism — every workflow already installs the Adafruit NeoPixel library, release.yml passes no feature flags at all, and compile-sketch.yml's non-BETA arm passes an empty FEATURE_FLAGS, so all three channels pick the new default up on their own. beta.yml's explicit -DBIRDSEYE_ENABLE_NEOPIXEL=1 is now redundant but harmless, and left alone. WHAT THIS COSTS, stated plainly because no later firmware can undo it: the subsystem's first boot programs UICR->NFCPINS to convert the two NFC pads to GPIO and self-resets once so the latch takes effect. That is a ONE-WAY change — reversing it needs a full chip erase and a bootloader reflash over USB — and with the flag on by default it is charged to EVERY device that installs 4.1.0, wired for LEDs or not, since the firmware cannot tell the difference. The trade was made deliberately: this hardware never uses NFC, the pads are otherwise idle, and gating a headline feature behind a separate build had kept it out of everyone's hands. Users who want the pads for something else must not install 4.1.0, so the CHANGELOG leads its 4.1.0 section with an upgrade note saying exactly that, including the extra reboot to expect. No other translation unit changes: grep confirms `#if BIRDSEYE_ENABLE_NEOPIXEL` appears nowhere outside neopixel.ino, so nothing else compiles differently and the simulator (which excludes that file) is untouched — 6/6 sim ctest still green with the flag flipped, goldens and both lap oracles included. Docs follow the decision rather than trailing it: the CHANGELOG's channel note is replaced by the upgrade warning and the per-entry "(beta channel)" markers come off the LED entries (they are not beta any more); CLAUDE.md subsystem 16, its flags list, constants table, File Map intro and workflows row; the ARCHITECTURE LED bullet; CONTRIBUTING's flag table (now noting this is the one flag defaulting to 1); and compile-sketch.yml's channel comment, where the difference between the two arms is now just SensorEgg and the library ref. Two in-code comments still calling the strip beta-only are corrected too. Verified: 507 host test cases / 325,581 assertions and 6/6 sim ctest. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U6kd8xwnUbXyd2Ce6m8ydX
… strict setting parse Six defects found reviewing the BETA -> master promotion diff. Each restores behaviour the code already claimed; none adds a feature. BLE `SLIST` was broken on every 4.1.0 device (release build, not flag-gated). /SETTINGS.json is parsed in TWO places, and only one was raised by plan 0010: settings.ino went 512 -> 1024 because the nine new keys take the default file from 329 to 538 bytes, while bluetooth.ino's SLIST handler kept a 512-byte buffer and a <512> document. It read 511 bytes of 538, deserializeJson() returned IncompleteInput, and the handler answered SERR:PARSE without emitting a single SVAL: line — the companion app's settings screen came up empty. SGET/SSET were unaffected (they route through settings.ino). Both parsers are now sized by SETTINGS_JSON_CAPACITY in settings.h, so they cannot drift again; the SLIST pair is static rather than stack, matching sd_functions.ino's "keeps JSON_BUFFER_SIZE off the stack" idiom now that it is 2 KB. A beta-converted board updating to a release build never powered its LED rail down. The flag-off stubs did nothing at all, by design — correct for virgin hardware, wrong for a board carrying the one-way UICR NFC->GPIO conversion an earlier beta build performed. There P0.09 (boost EN) was left in reset state (input, disconnected) for the whole session and through System OFF, where the driven level is the only thing holding the rail down (same retention as the "blue conn LED stays on after sleep" report). A floating EN reads as enabled, so "off" kept the 5 V rail and 11 idle WS2812s alive on a device with no power switch. NEOPIXEL_SETUP/NEOPIXEL_SLEEP now drive EN low, but ONLY when UICR->NFCPINS shows the pads are already GPIO — an unconverted board is still never touched, so the flag's promise holds exactly. Numeric settings were parsed with atoi(), which answers 0 for "" and for "garbage". For rev_limit and temp1_alert_c that is harmless (0 is below their floors), but for every key plans 0006/0010 added, 0 is IN range — and for led_brightness it means "LEDs off, never raise the 5 V rail". So a blank or corrupt value silently killed the strip instead of "clamping back to the compiled-in default per the house idiom" as its own comment claimed, and looked exactly like dead hardware. New host-tested setting_parse unit rejects anything that is not a complete integer, so the existing range check keeps the default; a deliberate 0 still works. evalStatus() strobed when handed clearBelow above threshold. rev_limit and overrev_limit clamp independently, so overrev_limit <= rev_limit * 0.97 put the overrev action's release point above its own trip point: the latch set on one frame and cleared on the next, flickering all 11 pixels at the 30 Hz frame rate instead of flashing at 100 ms. Fixed in the pure unit rather than at the call site so it covers the settings-driven assignability planned for phase 2, with a regression test. Pace page "STOPPED" was clipped: 8 chars at text size 3 is a 144 px advance on a 128 px panel. Seven chars at x=1 fits in 126 and centres. local_time.cpp was the only new pure unit missing from clang-tidy.yml, and it links into the release image (BirdsEye.ino calls isValidOffsetMinutes() unconditionally). Added, along with setting_parse.cpp. CLAUDE.md corrections found in the same pass: the NeoPixel library is not "linked in but inert" on a flag-off build, it is not compiled at all; the LED settings keys are written AND parsed on every channel, only their use is compiled out; the BLE manual-exit reboot was dated to a 4.0.1 release that does not exist; and subsystem 10's teardown order predated NEOPIXEL_SLEEP/WAKE. Subsystem 8 now documents that two parsers of the settings file exist. Verified: 515 host test cases / 325,646 assertions (up from 507/325,581 — 7 new setting_parse cases, 1 new led_modes case), and 6/6 sim ctest including the golden pixel fixtures and both lap oracles. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U6kd8xwnUbXyd2Ce6m8ydX
TheAngryRaven
force-pushed
the
claude/release-4.1.0-review-fixes
branch
from
August 22, 2026 21:59
92febd3 to
9ff7c17
Compare
This was referenced Aug 22, 2026
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
2 participants
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.
Summary
Six defects found reviewing the
BETA -> masterpromotion diff for 4.1.0 (#150). Each one restores behaviour the code already claimed; none adds a feature.Stacked on #149 — this branch is based on the release cut, so the CHANGELOG entries land in the
[4.1.0]section where they belong. Merge #149 first and this PR's diff reduces to just the fixes below.1. BLE
SLISTwas broken on every 4.1.0 device — release build, not flag-gated/SETTINGS.jsonis parsed in two places and plan 0010 only raised one.settings.inowent 512 → 1024 because the nine new keys take the default file from 329 to 538 bytes (measured, not estimated).bluetooth.ino'sSLISThandler kept a 512-byte buffer and aStaticJsonDocument<512>, so it read 511 bytes of 538,deserializeJson()returnedIncompleteInput, and the handler answeredSERR:PARSEwithout emitting a singleSVAL:line — the companion app's settings screen came up empty on every device, on a path that worked in 4.0.0.SGET:/SSET:were unaffected (they route throughsettings.ino), which is why it wasn't obvious.Both parsers are now sized by
SETTINGS_JSON_CAPACITYinsettings.h, so they cannot drift again. TheSLISTpair isstaticrather than stack, matchingsd_functions.ino's "keepsJSON_BUFFER_SIZEoff the stack" idiom now that it's 2 KB in one frame.2. A beta-converted board updating to a release build never powered its LED rail down
The flag-off stubs did nothing at all — correct for virgin hardware, wrong for a board carrying the one-way UICR NFC→GPIO conversion an earlier beta build performed. On that board P0.09 (boost EN) was left in reset state (input, disconnected) for the whole session and through System OFF, where the driven level is the only thing holding the rail down — the same retention behind the "blue conn LED stays on after sleep" report in subsystem 10. A floating EN reads as enabled on the Adafruit boost module, so "off" kept the 5 V rail and 11 idle WS2812s alive on a device with no power switch. Flat pack in a day or two, for exactly the people most likely to install the release.
NEOPIXEL_SETUP()/NEOPIXEL_SLEEP()now drive EN low, but only whenUICR->NFCPINSshows the pads are already GPIO. An unconverted board is still never touched, so the flag's promise holds exactly asproject.hstates it.3. A blank
led_brightnesssilently killed the stripNumeric settings were read with
atoi(), which answers 0 for""and for"garbage". Harmless forrev_limitandtemp1_alert_c(0 is below their floors) — but for every key plans 0006/0010 added, 0 is in range, and forled_brightnessit means LEDs off, and never raise the 5 V rail. So a blank or corrupt value switched the strip off instead of "clamping back to the compiled-in default per the house idiom" as its own comment claimed, and presented as dead hardware.New host-tested
setting_parseunit rejects anything that isn't a complete integer, so the existing range check keeps the default. A deliberate0still works.4. The overrev alert strobed when the two RPM limits sit close together
rev_limitandoverrev_limitvalidate independently, so nothing stoppedoverrev_limit <= rev_limit * 0.97, which puts the overrev action's release point above its own trip point. The latch then set on one frame and cleared on the next — all 11 pixels flickering at the 30 Hz frame rate instead of flashing at 100 ms. Fixed inled_modes::evalStatus()rather than at the call site, so it also covers the settings-driven action assignability planned for phase 2. Regression test included.5. Pace page
STOPPEDwas clipped8 characters at text size 3 is a 144 px advance on a 128 px panel, so the final "D" was cut off on every render. 7 chars at x=1 fits in 126 and centres.
6.
local_time.cppwas the only new pure unit missing from clang-tidy91 lines of new epoch/leap arithmetic that links into the release image (
BirdsEye.inocallsisValidOffsetMinutes()unconditionally). Added, along withsetting_parse.cpp.Docs corrected in the same pass
Found while checking prose against code, and it matters because
CLAUDE.mdloads into every future session:#if, so it isn't compiled at all.BIRDSEYE_ENABLE_NEOPIXELbuilds".NEOPIXEL_SLEEP()/NEOPIXEL_WAKE().Type of change
How it was verified
setting_parsecases, 1 newled_modescase)sim_lap_oracle,sim_lap_oracle_hardware,sim_two_session_carryover), built against DovesLapTimerBETAclang-tidyclean — CISLISTwants a companion-app round-trip. Both are reasoned from the registers/measured file size rather than measured on a device.The
SLISTsize claim is arithmetic, not estimation — the 22-key default file serializes to 538 bytes compact, against a 511-byte read cap.Checklist
CHANGELOG.mdupdated — five entries under[4.1.0]'s Fixed sectionARCHITECTURE.md/CLAUDE.mdupdated — new unit in the File Map, subsystem 8 (two parsers), subsystem 10 (teardown order), plus the four corrections abovetests/—setting_parsewith 7 cases; theevalStatusclamp has a regression testRelated issues
Findings from the review of #150. Does not touch the settings-downgrade question (a rollback to ≤4.0.0 still resets the file, since the keys are written on every channel) — see the PR discussion; that one interacts with the build-flag decision.
Generated by Claude Code