feat(nrf): hardware watchdog with reset-reason and phase breadcrumbs - #10
Closed
davelee98 wants to merge 5 commits into
Closed
feat(nrf): hardware watchdog with reset-reason and phase breadcrumbs#10davelee98 wants to merge 5 commits into
davelee98 wants to merge 5 commits into
Conversation
SPIM3 is the only nRF52840 SPIM instance carrying anomalies 195 and 198, and
both bite this firmware:
- 195: the instance keeps drawing current after disable. The nrfx workaround
pokes 0x4002F004 on every uninit and never undoes it on re-init.
- 198: transmit data may be corrupted when another bus master touches the TX
buffer's RAM block mid-transfer. The Arduino per-byte path makes that buffer
a one-byte STACK local, contended by SoftDevice interrupts on every byte --
and 198's workaround is compiled out here entirely, because
NRFX_SPIM3_NRF52840_ANOMALY_198_WORKAROUND_ENABLED is never defined.
Both nrfx workarounds are gated on `p_spim == NRF_SPIM3`, so moving to SPIM2
makes the anomalies inapplicable rather than worked around.
The Adafruit core binds the global `SPI` object to an instance chosen by
SPI_32MHZ_INTERFACE (framework SPI.cpp): 0 -- its default -- gives SPIM3, 1
gives SPIM2. Only `SPI` matters: bb_epaper drives the panel through that global
and nothing here ever calls SPI1.begin(), so the instance handed to SPI1 is
left uninitialised.
Free at our clock. SPIM2 tops out at 8 MHz and bbepInitIO already asks for
exactly 8000000, which setClockDivider maps to NRF_SPIM_FREQ_8M on either
instance. It forfeits the 16/32 MHz only SPIM3 offers -- deliberately, since
raising the panel clock increases the DMA bus pressure this change exists to
avoid. No pin change; PIN_SPI_* are untouched. No I2C conflict: Wire is TWIM0
and Wire1 is TWIM1, while SPIM2's block is shared only with SPIS2/SPI2.
The macro is consumed by the framework's SPI.cpp, not by our sources, so it
only works while -D reaches the framework build -- verified for this commit:
pio run -e nrf52840custom -v | grep 'SPI_32MHZ_INTERFACE=1.*SPI\.cpp'
Not yet exercised on hardware.
Merged
4 tasks
…-abuse disconnect) (OpenDisplay#135) * docs: freeze-hardening plan and CONNECTION_POLICY Squashes 16 successive revisions of the freeze-hardening planning docs into their settled state. They were written and re-cut against the tree as the phases were designed, so the intermediate versions record drafting churn -- plan restructurings, threshold decisions later moved to their point of use, and two rounds of external-review discharge -- rather than anything a reader or a bisect would want to land on. CONNECTION_POLICY.md is the normative ruleset the Phase 2-4 commits implement; the PLAN_* documents are the working notes behind it. * feat(ble): Phase 2 BLE-HAL foundation — owner token, instance table, frame identity Implements Phase 2 of docs/PLAN_FREEZE_HARDENING_2026-07-31.md: the transport/HAL mechanisms the later phases stand on. No idle timeout and no reclaim of a held slot — those stay Phase 3. Mechanisms: - src/link_owner.{h,cpp}: the owner token as ONE 32-bit atomic word (transport:2|handle:14|epoch:16), CAS-claimed at the earliest transport hook so stack callbacks can read it; atomic epoch allocator that never yields the reserved 0; linkMarkTerminal() returning the displaced identity; the R4 activity clock, whose baseline carries an owner tag so a newly admitted client can never inherit a prior session's silence. - Per-handle instance table in both transports. Liveness IS the packed identity word, so identity and liveness are read in one atomic load. - Callback-side filtering: per-link write filter, per-link subscribe state, and handle-targeted notify — the last closes a LIVE LEAK where every response, authentication traffic included, went to all subscribed clients. - Frame identity: CommandQueueItem carries its writer's identity word, stamped before the release-store; serviceBleRx drops frames whose tag is no longer the owner. This retires the RX-boundary mechanism (bleRxQueueDiscardTo and the capture that fed it), which could lose its boundary to handle reuse. - session_guard: abortToKnownState(reason, dropLink, ownerId) with the 11 ordered steps, transport-dispatched drop, release strictly last; bleDropAndWait() polling per-handle liveness (not the aggregate count) on a plain delay tick. - endRefresh() closes the refresh bracket on both paths and re-stamps the clock. - Deep sleep gates admission with linkMarkTerminal() BEFORE the abort, then does its own sleep quiescing (panel force-off incl. WARM, buzzer/LED silence). The abort itself never silences effects and never kills a WARM panel. - The 15-minute transfer watchdog routes through the abort. SCOPE ADJUSTMENT — contender refusal is pulled forward from Phase 3, because Phase 2 is not safely shippable without it. Admission is decided once per instance and never revisited, so a client reconnecting into a still-held slot (the ordinary case when loop() was blocked in a refresh) becomes a permanent contender — and on nRF it occupies the only peripheral link, leaving the device unreachable until it happens to leave. Releasing the token in the disconnect callback instead was tried and reverted: it admits a new owner while the departed session's transfer, crypto and TX ring are still live. LAN accept likewise refuses rather than evicts, which also removes a path that stranded the token until reboot and closes an unauthenticated eviction (LAN-TLS bypasses app auth). Loop order is now R7d-normative: disconnect cleanup and refusal run BEFORE the RX drain, so a departed session's state is gone before any frame dispatches. Verification: all 11 PlatformIO envs build; tools/test_link_owner.cpp (70,170 checks) passes under ASan+UBSan and ThreadSanitizer, and kills five mutants (handle-only identity, epoch-0 allocation, unconditional release, missing admission baseline, releasable terminal gate). Not verified: any hardware. Known gaps, stated rather than assumed covered: the notify handle race is narrowed but not closed (NimBLE can reassign a numeric handle at any time and notify() takes no epoch); the window between linkClaim's CAS and its baseline store is not deterministically testable without a firmware-side test hook, so it rests on publication order plus TSan. * fix(ble): close the Phase 2 review findings — LAN teardown ordering, refusal races Four review rounds against the Phase 2 implementation (dbec776). Each fix below replaced an earlier one that a later round showed to be wrong; the reasoning is recorded at each site so the rejected shapes are not re-tried. - LAN released its token before the deferred abort, on every ordinary exit path (TLS failure, peer close, read error, idle timeout, config restart). A BLE connect could claim the freed slot first, at which point the cleanup saw a live BLE owner and skipped the LAN abort entirely -- leaving the new owner's frames running against the departed LAN session's transfer state. disconnectWiFiServer no longer releases; the abort's final step does, exactly as on BLE. - Contender refusal could disconnect the winner. The connect callback publishes its table entry BEFORE its claim CAS, so an entry seen against a stale owner snapshot may belong to the connection taking the slot. Skipping the scan while unowned (the first attempt) then left a decided loser attached forever -- the original nRF unavailability, reintroduced. Fixed by publishing the claim DISPOSITION: each entry carries the identity word its claim was decided for, so the scan can tell in-flight from decided-and-lost. The identity binding (rather than a bool) is what excludes pairing one entry's identity with another's disposition after a slot is reused. - The scan's three loads are now ordered so a stale read is safe: entry word, then its decided-for word (must match, proving the claim resolved for THIS instance), then the owner word read fresh and last. - disconnect() takes (handle, epoch) and re-validates against the table before asking the stack, so a caller acting on a slightly stale scan cannot drop whoever inherited the numeric handle. - Refusal no longer raises the shared cleanup flag indirectly: serviceBleEvents schedules teardown only when the token's BLE owner has no live table entry (state-based, so it survives coalesced events). - The deep-sleep wake prologue tested aggregate ble.isConnected(), so a contender triggered fullSetupAfterConnection() and closed the wake window. It now tests for a live BLE owner, and reaps contenders in that branch. Residuals, stated rather than implied: notify() and disconnect() are both handle-addressed, so a host-task handle reassignment between validation and the stack call remains possible -- narrowed to a few instructions, and the exposure is a spuriously dropped client that reconnects, never stranded ownership. 11 envs build; host test 70,170 checks passes under ASan+UBSan and TSan. * ci: gate the link-owner host test; record that refusal moved to Phase 2 The Phase 2 work added tools/test_link_owner.cpp but never wired it into CI, so 70k checks covering the owner token, epoch discrimination, the terminal gate and the activity clock were not a repo gate. Run it in the existing host-tests job, twice: ASan+UBSan for the single-threaded semantics and TSan for the concurrent claim/epoch paths, since the two sanitizers cannot be combined and the cross-task claim is exactly what needs the second one. Also reconcile the plan with what shipped: contender refusal is in Phase 2, not Phase 3, because Phase 2 is not safely shippable without it (a client that reconnects into a held slot becomes a permanent contender, and on nRF occupies the only link). The phase table said otherwise, which would have misled the next reader about where that mechanism lives. * feat(ble): Phase 3 — idle drop, and one activity clock for both transports Implements Phase 3 of docs/PLAN_FREEZE_HARDENING_2026-07-31.md. Contender refusal already landed in Phase 2 (it was not safely separable), so what remains here is the reclaim path and the LAN half of R4's clock semantics. - serviceIdleTimeout() reclaims a slot whose owner has gone silent (7c). Called LAST in the pass (7d step 4), after BLE RX and handleWiFiServer, so traffic parsed this pass counts. Excludes refresh (7c row 3) and an unowned or terminal slot (7c row 4). Since admission never evicts, this is the ONLY way a held slot is released short of the client leaving. - NO transferActive() gate, per R4. A client that goes silent mid-upload is exactly the case that wedges the device, and a transfer gate would exempt it. The partial transfer is discarded by the abort. - OD_BLE_IDLE_TIMEOUT_MS = 120 s, #ifndef-guarded beside the code that services it, with the client behaviour it assumes recorded on the define. Deliberately generous: R4 inverted the direction of the error, since erring short now costs a legitimate upload rather than a stale session. - LAN's separate activity clock is RETIRED. lastLanActivityMs and its four stamp sites are gone, as is the inline 30 s check inside handleWiFiServer. Both transports now use the shared clock, differing only in their constant (BLE's local 120 s vs LAN's OD_LAN_READ_TIMEOUT_S, which is a client-visible wire-header contract). Two clocks for one rule is how they drift. The TLS handshake completion site stamps the clock, so the idle baseline starts there rather than at TCP accept -- which is why no separate handshake deadline is needed. - Accept/refuse is extracted into admitOrRefuseLanClient() so handleWiFiServer CONTINUES servicing the incumbent after a refusal. Returning early there let a contender starve the incumbent: its handshake never advanced and its frames were never dispatched, so the clock stopped being stamped and the new end-of-pass idle drop killed it with valid commands still unread -- an R3 violation (refusal must be inert) and an R7d one (step 3 before step 4). Found in review. 11 envs build; host tests pass under ASan+UBSan. No hardware: landed, not closed. * fix(ble): close the full-audit findings — activity requires an accepted command An independent audit of the whole Phase 2+3 stack (fresh review context, not the incremental ones) found eight issues. Fixed here, except two dropped as hostile-DoS only; one of those came BACK in scope on its merits and is the headline fix. - ACTIVITY NOW REQUIRES AN ACCEPTED COMMAND, not merely a recognised one. The old rule let an UNAUTHENTICATED session hold the exclusive slot forever, and not only under abuse: CMD_FIRMWARE_VERSION dispatches before the auth gate, so it never draws RESP_AUTH_REQUIRED and would never have incremented Phase 4's auth-abuse counter either -- the two mechanisms were documented as exhaustive and were not. The benign triggers are a monitoring integration polling firmware version and a client retrying auth with stale credentials. A handshake still cannot race the clock: the window runs from admission, so a client gets the whole 120 s to authenticate; it just may not extend it by retrying. - The deferred cleanup flag carried no identity, so a stale one could run a destructive teardown against whoever held the slot later -- resetting a freshly admitted client's crypto and rings. It now records the owner it was raised for and acts only if the slot still holds exactly that. A cleanup raised while UNOWNED is skipped outright: zero is not a session identity and must not authorise destruction. - A peer-closed LAN socket is now reaped early in the pass (wifiLanReapClosedSession) rather than inside handleWiFiServer. Reaping there was too late: it only raises the deferred cleanup, so the accept a few lines on still tested the corpse's token and refused an ordinary reconnect -- 7d wants release before admission. - BLE event publication is atomic on both targets: payload stored before the flag, flag RELEASE-stored, consumers using __atomic_exchange_n(ACQUIRE). The ESP32 connect site had been missed by the first attempt at this and was still a plain store paired against an atomic consumer. - Comment corrections where the text had become false: the transports' "callbacks only copy and flag" contract (they claim ownership now), command_queue.h's "nRF carries these unused", link_owner.h's "loop-task-only" activity clock, and the LAN baseline description. CONNECTION_POLICY now describes the provisional accept-time window restarted at handshake completion, which is what the code does, and no longer states the supervision timeout as categorically 4-6 s (the central chooses it; the spec permits up to 32 s). - CI/docs said eleven environments. There are twelve: esp32-wrover-e-N4R8 ships via firmware-targets.json but is absent from default_envs, so a bare `pio run` skips it -- and it is a no-WiFi target, the one most likely to catch a broken #ifndef OPENDISPLAY_HAS_WIFI path. Recorded in CLAUDE.md and built explicitly. Out of scope by direction (hostile DoS): a peer that ignores link termination holding controller capacity, which the refusal scan retries every pass anyway. All 12 envs build; host tests pass under ASan+UBSan and TSan. # Conflicts: # .github/workflows/main.yaml * fix(ble): the activity rule must not depend on authentication being configured The previous fix required an ACCEPTED command, which on a device with encryption enabled correctly stops an unauthenticated session pinning the exclusive slot. But encryption_enabled defaults to 0 -- documented in opendisplay_structs.h as "all commands work unauthenticated" -- and with no gate to pass, "accepted" degrades to "recognised" and the rule becomes a no-op. The hole was therefore still open on the default configuration, which is likely the common one in the field. Split the rule so it behaves the same either way: - CMD_AUTHENTICATE and CMD_FIRMWARE_VERSION never count as activity in ANY configuration. Both dispatch ahead of the auth gate and neither is work the device does for a client, so a session sending only those is idle by any useful definition. This is the half that carries the auth-off case. - Where a gate does exist, the command must also be past it, so an unauthenticated peer's refused commands cannot outlive a working client. Behaviour is unchanged for real clients in both configurations: they send real commands, which stamp. A handshake still cannot race the clock, because the window runs from admission -- 120 s against a one-exchange handshake. 12 envs build. * docs: correct an overclaiming memory-ordering comment; Phase 4 is now optional Two loose ends found while auditing what remains. - takeConnectedEvent/takeDisconnectedEvent claimed the acquire exchange guarantees the payload belongs to the flag just consumed. It does not: release/acquire orders writes PRECEDING the release and nothing freezes the payload afterwards, so a second event landing between the exchange and the load hands us its word. Tolerable only because nothing decides on it -- teardown and connect-side work both derive from the owner token and instance table -- so the comment now says the payload is diagnostic and warns against building a decision on it. - Phase 4's stated premise is stale. It claimed Phase 3 depended on the auth-abuse counter, because the clock stamped any recognised command before the auth gate, so a never-authenticating peer kept it fresh forever. Phase 3's narrower activity rule removed that dependency: handshake/discovery opcodes never stamp, so such a peer now ages normally and the idle timeout drops it. Phase 4 is demoted from correctness requirement to optimisation -- it shortens a 120 s reclaim to about one exchange and gives the client an explicit reason instead of a silent timeout. It should be scheduled on that value, not on the old argument. * fix(config): make resetChunkedWriteState the single primitive it was specified as The freeze-hardening plan's Phase 2 step 6 called for "a real primitive replacing the open-coded inline clears in communication.cpp; call it here AND from those sites". Only half landed: the primitive was defined inside session_guard.cpp with no declaration in any header, and the three inline clears stayed. That left the exact drift the step existed to remove. The three sites each cleared a different subset -- one set only `active`, one also the counters, none the totals -- so a config upload aborted down one path kept a stale totalSize and expectedChunks, and the abort's own reset was the only complete one. Move the definition beside the state it owns (config_parser.cpp, declared in config_parser.h) and convert all three sites. abortToKnownState keeps calling it. Also record why the abort's step 2 (client NACK) has no code: every caller either drops the link or runs because the link is already gone, so none can deliver one. Written down so it reads as a decision rather than an omission. 12 envs build. * feat(ble): Phase 4 — auth-abuse disconnect Drops a BLE link after OD_AUTH_ABUSE_THRESHOLD (10) consecutive commands answered RESP_AUTH_REQUIRED, so a session that cannot authenticate stops holding the globally-exclusive slot while it retries. An OPTIMISATION, not a hole-closer, and built to that standard: Phase 3's narrower activity rule already means such a peer ages normally and the idle timeout reclaims at 120 s. This adds speed and a reason -- about one exchange instead of two minutes, and an explicit final RESP_AUTH_REQUIRED before a deliberate drop rather than a silent timeout. It may fail safe without reopening anything. - Counter is BLE-only (the same auth gate is reachable from plaintext LAN, and counting those would let LAN traffic drop a BLE client), identifies the offender from the frame's own instance tag rather than "whichever peer the stack lists first", and is cleared at every session end via the abort and on a successful handshake. - Threshold 10 chosen deliberately BELOW py-opendisplay's 16-frame pipe window: when a session dies mid-upload every in-flight frame is doomed, so dropping at 10 beats waiting out a full window of pointless round trips. An earlier prototype inherited this number by accident; it is now a decision. - Best-effort delivery of the final FE: drain TX, then dwell one negotiated connection interval, both inside a 500 ms hard bound, then abort with dropLink=true. An empty ring proves stack acceptance of an unacknowledged notification, not receipt, so a deadline-truncated attempt forfeits it by design. THE ACTIVITY DECISION MOVED TWICE UNDER REVIEW, and both earlier positions were wrong the same way -- anything that predicts acceptance is wrong at whatever layer rejects next: - gated on isAuthenticated() at the top: an authenticated client sending a too-short plaintext frame stamped, then got RESP_AUTH_REQUIRED from the length check below it -- defeating the idle timeout AND pinning the rejection run at one, so neither mechanism could ever fire. - just before the dispatch switch: TLS-LAN frames bypass the CCM gate but the config-write handlers apply their own app-layer auth, so a TLS client repeating CMD_CONFIG_WRITE stamped on every rejected attempt. It now sits AFTER the switch and reads an outcome flag set by every rejection site on every transport. That position has nothing below it. The servicer also defers one pass while RX is pending, so an authentication frame that arrived after this pass's drain is dispatched and can cancel the drop. 12 envs build; host tests pass. No hardware: landed, not closed.
…acket loss (OpenDisplay#136) * fix(nonce): replace 512 B replay value ring with a 32 B sliding bitmap Step 1 of docs/PLAN_PHASE1_NONCE_REPLAY_2026-07-26.md. New dependency-free src/nonce_window.h holds the whole window state machine as static inline functions over plain values: no Arduino, no mbedtls, no millis(), no logging, no session. That is what tools/test_nonce_window.cpp targets (Decision D), and it keeps nonceCheck/nonceCommit file-static in encryption.cpp (Decision C) at no cost to testability. Representation is IPsec/DTLS shifting style (RFC 4303 / RFC 6347), not the circular RFC 6479 / WireGuard form (Decision B): bit i == "counter (last_seen - i) consumed", bit 0 == last_seen. Eviction and falling out of window become the same event, so the hand-maintained D >= 2W coupling between constants in two files ceases to exist. That representation deletes two defects outright rather than patching them: D3 - no reserved sentinel, so "not seen" is a clear bit and the counter_diff != 0 exemption that made the highest-seen frame replayable is gone. [H3]: that exemption also flushed the ring, unlocking the last 32 genuine counters, not just the last one. D4 - a bitmap has no insertion point, so the function-static replay_window_index cannot recur. OD_NONCE_BACKWARD_BITS 256 (uint64_t[4], 32 B) is kept strictly greater than OD_NONCE_FORWARD_CAP 128 so a legal forward slide can never exceed the bitmap width, keeping the wholesale-clear branch off the normal path. Cap is 128, not 64 [C1]: the old derivation (PIPE_MAX_W + MAX_PTO = 35, asserted as a hard bound) missed the client's selective-repair transmit site, which spends no window credit; the reachable gap is ~96 at blocks_per_ack = 1. Under a bitmap the cap is a comparison, not storage, so the width is free. encryptionSession shrinks by exactly 480 B (verified: sizeof 0x118 on esp32-N4). * fix(nonce): split check from commit; stop counting packet loss as tampering Steps 2-4 of docs/PLAN_PHASE1_NONCE_REPLAY_2026-07-26.md. verifyNonceReplay() is deleted outright (Decision C - no compatibility wrapper), along with its declarations in encryption.h and the duplicate in main.h. D1 - a nonce rejection no longer touches integrity_failures. The rule in one line: only a CCM tag failure is evidence of tampering; a nonce failure is evidence of a lossy link. Previously a lost window put the next frame out of range and each such frame counted toward session destruction, after which the device answered RESP_AUTH_REQUIRED to everything until reconnect. D2 - nonceCheck() is pure. It writes nothing to encryptionSession on any path, so replay state is advanced only by nonceCommit(), which runs as the FIRST statement of decryptCommand's success arm - after aes_ccm_decrypt. Placement is load-bearing [L2]: there is an early return in that arm for a decrypted-but-malformed payload_length, and that frame is authentic (it passed the tag). Today's unconditional commit does record it; committing after the early return would silently leave an authentic frame replayable. M1 - unsigned wrapping deltas only, no signed arithmetic. The 8 counter bytes are parsed off the wire before the tag is verified, so an unauthenticated attacker controls both operands: (int64_t)counter for >= 2^63 is implementation-defined pre-C++20, the subtraction can overflow, and negating INT64_MIN is UB. Unsigned overflow is defined as modular arithmetic, making the expression total over all 2^64 inputs. The four tests in od_nonce_check are ordered and the order is load-bearing - fwd and back are complements mod 2^64 and cannot both be small. M3 - resetNonceState() names all four shared fields explicitly. The two callers share only these four and are opposite on everything else, so a helper described loosely as "the bitmap and last_seen_counter" would invite dropping nonce_counter = 0 - which would carry the device's outbound counter across a re-auth while the client restarts at 0, walking into the [H2] keystream reuse against itself. L7 - both nonce-rejection logs demoted from ERROR to WARN and rate-limited to one per 5 s (one shared budget, so alternating between them cannot bypass it). The out-of-window log now fires routinely on a lossy link, and with counting removed nothing else throttles a peer driving the session-id line, which also no longer dumps two full session IDs. Routing NONCE_BAD_SESSION to "integrity_failures untouched" is a deliberate policy change, not a consequence of D1: a mismatched session id is usually a stale client talking to a device that re-authenticated. decryptCommand gains a NonceResult* reason out-param (internal signature only, nothing on the wire) so its single caller can distinguish nonce rejection from tag failure. Step 4b uses it. * fix(pipe): do not answer a nonce-dropped 0x0081 frame with a fatal NACK Step 4b / [H1] of docs/PLAN_PHASE1_NONCE_REPLAY_2026-07-26.md. Without this, Phase 1 saves the device but still loses the transfer. Every decryptCommand failure - nonce and tag alike - produced the same unencrypted 3-byte RESP_NACK, and the client turns that shape into IntegrityCheckError before it ever reaches pipe-frame classification (device.py:833-838). The pipe send loop's only except is BLETimeoutError, so one out-of-window frame aborts the whole upload - and the frame the client would have repaired is the one whose NACK killed the transfer. No forward cap is wide enough to fix that. Now: NONCE_OUT_OF_WINDOW / NONCE_REPLAY on CMD_PIPE_WRITE_DATA sends NOTHING. Silence is already a first-class signal on the pipe path - it means "lost", the seq is absent from the next SACK mask, the client retransmits, and the transfer continues. This is a conformance fix, not a protocol change. pipe-write-protocol.md 5.2 already reserves NACKs for unrecoverable conditions, "not ordinary packet loss", and 5.1 makes an 0x81 NACK unconditionally fatal - so today's firmware violates the pipe spec as written. No opcode, response code, or envelope changes; no canonical-header edit. Deliberately narrow: tag failures keep the NACK (tamper evidence, not loss), and 0x0071 legacy DIRECT_WRITE_DATA is left alone - different ACK discipline, not analysed here, and the field failure lives on the pipe path. Step 6 - the stale comment at the 0x0081 case rewritten. Per [C1] it now states the MECHANISM rather than a number: the forward gap is bounded by the client's retransmit budget max_retx = max(3*W, n/2) and by blocks_per_ack, which live in another repo and one of which is a user-facing Home Assistant option, so OD_NONCE_FORWARD_CAP is a heuristic with headroom, not an invariant firmware can prove. A number written here would be falsified silently by a client-side config change. * ci: separate host-tests job for the nonce-window state machine [L5] of docs/PLAN_PHASE1_NONCE_REPLAY_2026-07-26.md. A top-level job, NOT a step inside the existing `build` job - that is an 11-entry matrix and would run the host test eleven times. Needs no toolchain beyond the runner's stock g++. -fsanitize=undefined,address is the point of the gate, not decoration: it is what catches the `x << 64` UB in the bitmap shift automatically rather than relying on the test author to predict it. tools/ is invisible to every firmware build (build_src_filter is relative to src_dir and no env adds tools/), so the test file cannot perturb the matrix. * test: host test for the nonce sliding-window state machine Step 5 / Decision D of docs/PLAN_PHASE1_NONCE_REPLAY_2026-07-26.md. Standalone, no framework, no PlatformIO env and no test/ directory - so the 11-env matrix and a bare `pio run` are untouched. Includes ONLY src/nonce_window.h. g++ -std=c++17 -Wall -Wextra -Werror -O1 -fsanitize=undefined,address \ tools/test_nonce_window.cpp -o /tmp/test_nonce_window && /tmp/test_nonce_window -> PASSED 38199 checks Coverage: - Purity of od_nonce_check (D2) - the single most valuable assertion here. Snapshot the state, call every result class, memcmp byte-for-byte after. "The tag is the only thing that may advance replay state" rests on it. - D3 at fwd == 0: re-presenting a committed counter is REPLAY, including the case today's firmware exempts. - Fresh session: counter 0 accepted exactly once from a virgin state, with no has_seen_counter sentinel anywhere. - Shift edges: fwd = 0, 1, 63, 64, 65, 127, 128 through od_nonce_check (the reachable range), plus 129, 191, 192, 255, 256, 257 driven directly against od_nonce_commit for the word boundaries and the wholesale-clear guard. - Wholesale slide asserts the EXACT enum: previously-seen counters come back OUT_OF_WINDOW, not REPLAY. Both reject, so conflating them would be invisible in behaviour and would hide a genuine slide bug. - Bit-index invariant stated directly: the bit denoting (L - i) must sit at i + d under L' = L + d. - [M1] counter arithmetic at counter = 2^63 / last_seen = 1 and around UINT64_MAX including the wrap - the inputs that make the signed form UB. UBSan makes this self-checking. - Differential test against a std::set oracle, fixed mt19937_64 seed, with a full backward-window sweep at the end of each sequence. The oracle prunes counters that fall out of the window so it agrees on REPLAY vs OUT_OF_WINDOW, not merely on accept vs reject. __ubsan_on_report is overridden so a UBSan report exits nonzero: UBSan defaults to print-and-continue, which would let a reintroduced signed-arithmetic regression pass CI with a runtime-error line nobody reads. Mutation-checked: breaking the backward bit test, the cap comparison, the fwd == 0 bit test, or the cross-word shift carry all fail the test. * fix(nonce): review follow-ups - split log budgets, readable replay log, honest comments Findings from an independent adversarial review of the Phase 1 diff against docs/PLAN_PHASE1_NONCE_REPLAY_2026-07-26.md. No behaviour change to the window state machine; all 38199 host-test checks and all 11 firmware envs still pass. 1. One rate-limit budget PER LOG SITE, not one shared. A stale client spamming session-id mismatches could otherwise silence the out-of-window line for 5 s at a time - and out-of-window is precisely the condition Step 5's hardware tests 0-2 exist to observe. The shared budget would have masked the measurement the plan depends on. 2. The rejection log now prints `back` for a replay and `fwd` for out-of-window. A replayed counter is normally BEHIND last_seen, where the (correctly) wrapping fwd delta printed as a 20-digit number - unreadable in the case the line fires in most. 3. Decision C's "linkage enforces that only decryptCommand may commit state" is softened to what is actually true. nonceCheck/nonceCommit are file-static, so it holds for them - but encryption_state.h must include nonce_window.h for OD_NONCE_BITMAP_WORDS and main.h includes encryption_state.h, so the raw od_nonce_commit() primitive is visible in every TU next to the extern encryptionSession. Unavoidable while the struct needs the width macro; recorded rather than left as a claim the code does not support. 4. od_nonce_commit's totality comment now states its sharp edge instead of implying it away: a counter more than OD_NONCE_BACKWARD_BITS *behind* last_seen also lands in the forward branch (fwd and back are complements), so it clears the bitmap and REWINDS last_seen, un-seeing everything. Unreachable through od_nonce_check - which returns OUT_OF_WINDOW so it is never committed - but it is the edge to watch if a caller ever commits without checking. 5. nonce_window.h includes <stdbool.h>, so its "zero dependencies" claim also holds for a C translation unit. * docs(pipe): a nonce gap beyond the forward cap is not recoverable Two comments claimed the silent drop was repaired by the normal SACK path. That holds for a rejection inside the window; it is false past OD_NONCE_FORWARD_CAP. Once a frame is rejected for fwd > cap, nothing commits, so last_seen never advances. Every retransmission re-encrypts with a fresh, higher counter (py-opendisplay _write_pipe_frame never resends the original ciphertext), so each one is rejected at a greater distance than the last. The session cannot recover without re-authenticating, and the transfer stalls until the stuck-transfer timeout releases the panel. The cap's job is to put that state out of reach, not to make it recoverable. Dropping silently still beats a 0x81 NACK, which is unconditionally fatal and kills the upload immediately -- but it does not rescue the transfer, and the comments should not have implied it did. Comments only; no behaviour change. nrf52840custom builds. * fix(nonce): remove the forward cap; order counters numerically OD_NONCE_FORWARD_CAP turned a transient link fault into a permanent session fault. A counter more than 128 ahead was rejected, and because nothing commits until the CCM tag verifies, last_seen never advanced. The client re-encrypts every retransmission with a fresh, higher counter and never resends the original ciphertext, so each subsequent frame was rejected at a greater distance than the last. The session could not recover without re-authenticating; the transfer stalled until the 15-minute watchdog released the panel. It is reachable with supported settings. With W=32 and blocks_per_ack=1, 16 queued gap-ACKs at PIPE_RETX_ACK_SPACING=2 burn 128 counters on repairs alone, and the gap accumulates across aborted attempts because the client deliberately does not re-authenticate mid-transfer. max_retx = max(3*W, n/2) is order thousands for a full-panel upload, so the budget is nowhere near spent when the cap is crossed. The cap bought nothing. All 8 counter bytes sit inside the CCM nonce, so a tampered counter fails the tag; commit runs only on the success arm; and passing the check mutates nothing. An attacker who cannot forge a tag could not advance last_seen at any distance, with or without a cap -- and the session id is cleartext in every frame, so anyone who could flood CCM with a capped window could flood it without one. So: no forward bound. A counter ahead of last_seen is accepted at any distance and gated by the tag, exactly as RFC 4303 Appendix A2 does. Comparison also moves from modular to numeric. Modular arithmetic made a counter far behind indistinguishable from one far ahead, which is what let an ancient counter present as an enormous forward jump -- the "sharp edge" the header admitted, where committing one would rewind last_seen and clear the bitmap. That is now impossible by construction rather than by the caller's contract. For the same reason, do not reintroduce a bound as cap=UINT64_MAX or as "not-backward implies forward": either restores the overlap. Consequences: - Counters no longer wrap. Reaching UINT64_MAX requires re-authentication, per RFC 4303 3.3.3; wrapping would reuse a (key, nonce) pair, which is the exact failure this file prevents. 2^64 counters in one session is unreachable. - NONCE_OUT_OF_WINDOW now means only "too far behind", so the rejection log computes direction from the counters instead of inferring it from the reason, which would have printed an underflowed 20-digit distance. - OD_NONCE_BACKWARD_BITS keeps its value but not its old justification, which was stated in terms of the cap. It is now purely out-of-order tolerance, and its exact value is not load-bearing: a backward rejection is self-healing, because the retransmit carries a higher counter that is accepted unconditionally. Tests: the oracle no longer prunes its seen set -- that pruning encoded the implementation's forgetting, so it could only ever agree with it. It now keeps every counter ever committed and states the property directly: a consumed counter is never returned NONCE_OK, at any distance, by any route. Added the cliff as a sequence (not a point, which cap=UINT64_MAX would also pass), the escalating-retransmit pattern, and far-behind-is-never-forward, which pins both forbidden shortcuts. Shift edges now run through od_nonce_check, so the wholesale-clear path is exercised the way production reaches it. Verified: 47445 checks pass under -Werror with ASan+UBSan; the same suite fails 1635 checks against the pre-change implementation. nrf52840custom and esp32-c3-N16 build. No wire change: nonce format, response codes and framing are untouched, and the accept set only grows, so no peer needs updating in lockstep. * fix(pipe): log the fatal NACK that ends a transfer Every 0x81 NACK is terminal: it sets pipeState.error, releases the panel, and makes the client raise. None of that left a trace in the device log, so a failed upload could not be told apart from a stall or a link drop without a sniffer or the client's traceback. Logged before the send, at ERROR, with the state a diagnosis needs: the error code, expected_seq, highest_seen, reorder-queue occupancy and the negotiated window. It cannot flood -- pipeState.error is set immediately after and makes every later 0x0081 frame discard, so at most one fires per session. The comment singles out err 0x04. A conforming client cannot produce it: it only transmits seq within W of its own window_base (all four transmit sites in py-opendisplay's _pipe_stream are bounded by that rule), and the device's expected_seq is provably within W of that same base, so fwd < W or back <= W always holds and the "out of window on both sides" arm is unreachable. Client and device agree on W because the device advertises PIPE_MAX_W in the START ACK and both apply the same min-rule -- including on the PIPE_SMALL_DRAM_WINDOW envs where PIPE_MAX_W is 16 rather than 32. That proof rests on a window rule enforced in another repo and on nothing in this firmware, which is exactly why the line is worth having: if the assumption ever drifts, this is the only thing that would say so. nrf52840custom, esp32-c3-N16 and esp32-N4 build. * docs: pull in the freeze-fix Phase 1-3 planning docs Findings, plans, and the IT8951 integration/timer-watchdog inventory docs from debug/freeze-fix-phase2, including PLAN_PHASE1_NONCE_REPLAY, directly relevant to this branch's nonce work. PLAN_PHASE2_BOUND_WAITS carries an OBSOLETE banner, superseded by PLAN_PHASE2_REFRESH_BOUNDS; kept as that branch retained it. * docs(nonce): record the Decision A reversal; restore the surviving citations d65696f brought the Phase 1 planning doc onto this branch. It had been absent from every branch, so aef3a6b stripped the citations pointing at it as dangling. Now that it exists they belong back -- but not all of them, because aef3a6b reversed one of the decisions it records. Doc: Decision A ("forward cap 128") is struck through and marked REVERSED, with a pointer to a new section at the end that carries the argument -- what the cap did (nothing commits, so last_seen never advances, and each retransmission's fresh higher counter is rejected further out than the last), why no number could have been safe (the ceiling is the client's retransmit budget scaled by a user-facing HA option, and it accumulates across aborted attempts), why removing it costs nothing (the counter is inside the CCM nonce; commit is post-tag; the session id is cleartext so the DoS argument never held), and what replaced it. A table records the effect on every other item: B, C, D and E stand; Step 6's "write the mechanism, not a number" is now literal; Step 5's hardware tests 1/2/2c are obsolete as written, since they existed to validate the 128. Worth noting the doc had already flagged OD_NONCE_FORWARD_CAP as "unvalidated against a real link" in its own unverified-on-hardware list. The flaw was found by analysis rather than by the bench it was waiting on, so that list is unchanged by this reversal. Code: restored the citations that still hold -- Decision D at nonce_window.h and the CI job and the host test, Decision B for the shifting-bitmap representation (unchanged; only the arithmetic over it moved), Step 1 at encryption.cpp and encryption_state.h. The two sites that used to cite Decision A now cite the reversal instead, so a reader lands on the argument that supersedes it rather than on the superseded one. Comments only; no behaviour change. 47445 host checks pass; nrf52840custom builds.
nRF had no watchdog, so any unbounded wait was permanent: nrfx_spim.c's
`while (!nrf_spim_event_check(END)){}` and the six Wire_nRF52 TWIM spins
have no timeout and no yield, and checkTransferTimeouts() cannot help
because it runs FROM loop(), which is what is stuck. This makes that
class recoverable -- the residual PLAN_PHASE2_BOUND_WAITS D-L accepted
and D-K assumed unrecoverable.
Portable module, not an nRF-only one: every feed site and breadcrumb
stamp lives in code that compiles for both targets, so an nRF-only API
would mean #ifdef TARGET_NRF around ~20 call sites in shared files.
Follows the ble_transport pattern -- one header with no vendor includes,
two whole-file-gated implementations, ESP32 stubbed. Its reset-reason
decode moves out of main.cpp, a net #ifdef reduction there.
Timeout is 300 s, and the number alone is not what makes it safe. The
longest span the firmware cannot instrument is a REFRESH_FULL on a
7-colour split-buffer panel: 4 BUSY_WAIT entries x 30 s, sent to BOTH
controllers, ~240 s inside one bbepRefresh(). What keeps that from
resetting a healthy device is the feed immediately before all 15
bb_epaper entry points, so the dog faces one call rather than that call
plus everything preceding it. Margin is ~1.25x -- re-check it when
adding a panel.
Three details that are easy to get wrong:
- RESETREAS must come from readResetReason(). The core reads AND
clears the register in init() before setup(), so reading the
peripheral (or sd_power_reset_reason_get) returns zero forever and
reports every watchdog reset as a power-on.
- GPREGRET2 needs two access paths. sd_power_gpregret_* are numbered
from SOC_SVC_BASE_NOT_AVAILABLE and cannot be used before
ble.begin(); direct register access is correct while the SoftDevice
is disabled, SVCs once it is enabled.
- A running WDT cannot be stopped or reconfigured, and which resets
clear it is NOT established by anything in-tree. So inherit-detection
via RUNSTATUS runs on every build INCLUDING the disabled one, and
feeds every enabled RREN channel of whatever it finds. A disabled
build that inherited a live dog it never fed would be a brick.
Boot-loop containment: armed before the boot panel path so boot wedges
are covered, with a 3-strike counter in GPREGRET2 bits 5:4 entering a
safe mode that refuses panel work at both epdSessionAcquire and pwrmgm.
Strikes clear after 10 min of uptime rather than on a successful
refresh -- refresh-based clearing would never accumulate (every boot
refreshes) and would make safe mode permanent (safe mode never
refreshes).
Not verified on hardware. The 240 s figure is read from bb_ep.inl, not
measured; T2 in the plan is the gate before this reaches devices.
Safe mode rejects transfers late and generically -- a clean NACK needs a
"device in safe mode" code, which must originate in opendisplay-protocol.
Plan, decisions and four rounds of review findings:
docs/PLAN_NRF_HARDWARE_WATCHDOG_2026-08-01.md
Timeout dropped from 300s to 120s, which is now below the ~240s worst-case REFRESH_FULL span on a 7-colour split-buffer panel -- a healthy refresh on that panel class will trip the watchdog mid-refresh. Known gap, documented in platformio.ini; re-check before shipping to such a panel. Adds two new breadcrumb phases (IDLE_OFF/IDLE_WARM replacing the shared IDLE) so a freeze while the panel session is idle can be told apart from one during keep-alive, plus four more (PWRMGM_AXP2101/RAIL/PINS/WIRE) instrumenting pwrmgm()'s previously-blind power-up path, added after a watchdog reset landed there with no breadcrumb to explain why. A phaseName() lookup makes the retained phase human-readable in the boot log instead of a bare integer. WDT-DEBUG-tagged od_log_debug lines pair with each EPD-session and pwrmgm() breadcrumb (stamped first, since the log call itself can hang on the same USB CDC mutex delay() depends on) -- grep "WDT-DEBUG" to remove the whole set later. Also adds a bounded (2s cap), debug-build-only wait for the USB CDC host to reconnect before the first log line: without it, the reset-reason and retained-breadcrumb lines -- the whole point of this feature -- reliably lose the race against USB re-enumeration after a reset and are silently discarded by od_log's dark-port check, which doesn't count them as drops.
davelee98
force-pushed
the
feat/nrf-hardware-watchdog
branch
from
August 3, 2026 19:15
780a41e to
4b54a88
Compare
Owner
Author
|
Superseded by OpenDisplay#139, which is now clean directly against main now that OpenDisplay#135 and OpenDisplay#136 have merged. |
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.
Summary
watchdog.hAPI, ESP32 stubbed) to recover from unbounded waits belowloop()that no software timeout can catch (nrfx SPIM busy-spin, Wire_nRF52 TWIM spins) — seedocs/PLAN_NRF_HARDWARE_WATCHDOG_2026-08-01.md.OPENDISPLAY_NRF_WDT_S), below this plan's documented 240 s worst-caseREFRESH_FULLspan on a 7-colour split-buffer panel — a known, deliberate gap, called out in bothplatformio.iniand a status-update section at the top of the plan doc. Do not ship to that panel class without re-deriving the timeout.pwrmgm()(previously blind to the watchdog) now has 4 breadcrumb phases covering its power-up sub-steps, added after a watchdog reset landed there with no breadcrumb to explain why.WDT-DEBUG-tagged debug logging at every EPD-session andpwrmgm()stage transition (grep "WDT-DEBUG"to strip later).Test plan
pio run -e nrf52840customand-e nrf52840custom-debugbuild cleanpio run(all 11 default envs) builds clean