SensorEgg: GATT link and pairing review fixes (plans 0017/0018) - #167
Open
TheAngryRaven wants to merge 8 commits into
Open
TheAngryRaven wants to merge 8 commits into
TheAngryRaven wants to merge 8 commits into
Conversation
The central disconnect callback runs in a higher-priority task than the main loop. If the egg dropped right after enableNotify(), the callback could set BACKOFF and invalidate the handle before the loop consumed eggBringupReady — and the loop then overwrote BACKOFF with STREAMING. STREAMING only acts when the handle is valid and counts as "engaged", so the scanner stayed stopped and EGT read NaN until shutdown or a transfer. The commit is now conditional (state still BRINGUP and a handle still held) and is checked and applied inside one critical section; a BRINGUP/STREAMING state with no handle is reconciled to BACKOFF every loop as a backstop. Both rules live in sensoregg_gatt (LinkState + linkMayCommitStreaming / linkReconcileOrphan) with host tests. The connect callback and SENSOREGG_SLEEP() clear stale ready/failed flags so a bring-up staged before a sleep can never be consumed against a later link's buffers. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SdhRyfZ4eYuooXi9uy28aw
SENSOREGG_SLEEP() cancels a pending connect and disconnects a known handle, but there is a window where the SoftDevice has already raised CONNECTED while the deferred central connect callback has not run: the handle is still INVALID and connect_cancel() is a no-op. The callback then ran the full bring-up after sleep, and in BLE transfer mode or the charging park nothing ever reconciled it — the egg link held the radio for the whole session. The connect callback now keeps a connection only when it is the one we asked for (state CONNECTING), the sleep gate is down and the link is still wanted (linkAcceptCentralConnect, host-tested); otherwise it disconnects that handle without adopting it (a CONNECTING state is retired to BACKOFF instead of waiting out the timeout). That also covers a connect landing after the 10 s connect timeout already moved to BACKOFF. The disconnect callback now ignores any handle that is not eggConnHandle, so a refused connection can't drive the state machine. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SdhRyfZ4eYuooXi9uy28aw
The GATT stream writes into the same Reading the beacon does, but only into the roles the descriptor maps, while every frame refreshes the shared eggRxMs. So tcFault, the aux temperature and the battery kept whatever the last beacon said for as long as frames kept coming: a stuck *TC FAULT*, and a pod with no IAT channel logging a flat line into Temp2 — exactly what the never-hold rule forbids. Zombie detection only watches the fastest channel, so an EGT channel that stopped while CJ continued also held EGT. On STREAMING commit the surface is now reset (readingResetForStream: temps NaN, battery 0xFF, flags/status/tcFault/pairing false, seq 0; protoVersion kept) and the sequence monitor restarts. Each mapped role gets its own receive stamp (RoleFreshness) and every value accessor also requires its role to be fresh while the stream was the last writer; a beacon parse hands the gate back to the plain 1 s rule. The per-role window is the channel's own frame cadence plus one frame of slack (2 x n x interval), floored at kStalenessMs and capped at 60 s: a flat 1 s rule would flap the EGT pod's 1 s IAT channel and blank its 30 s battery forever, while EGT/CJ keep exactly the 1 s rule. All of it lives in sensoregg_gatt with host tests. eggRoleLive() takes a uint8_t because the auto-prototype lands above the sensoregg_gatt include. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SdhRyfZ4eYuooXi9uy28aw
The scan callback ignored Bluefruit.Central.connect()'s return. A refused request (radio busy, bad params) never produces a connect callback, so the link sat in CONNECTING for the full 10 s timeout, then 5 s of backoff — with the scanner still paused on the report that triggered the connect, so the beacon path could not fill in. That is 15 s of NaN EGT per refusal. On false the state now goes straight to BACKOFF (linkAfterConnectRequest, host-tested), the backoff clock is stamped, and the scanner is resumed from the callback when the scan is wanted — the same callback-safe resume the beacon path already does. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SdhRyfZ4eYuooXi9uy28aw
The notify ring had 32-byte slots, enough for the current egg's <=26-byte frames but not for the spec's 117-sample maximum: any frame over 11 samples was counted in eggFrameDrops and discarded while the link stayed STREAMING and looked healthy, and nothing ever read the counter. Slots are now kMaxFrameLen (244 = ATT_MTU 247 - 3, the largest notify the link can carry at the MTU cap we configure), still 8 deep: ~1.9 KB instead of 256 B, justified in the comment — the depth rides out a main-loop stall at the stream's frame rate and the extra RAM is small next to the track-JSON buffers. The drop counter is exposed (sensoreggFrameDrops(), no-op twins in the flag-off build and the sim) and rendered on EGG TEST as D<n>. A new host-tested DropMonitor drops the link after drops in 3 consecutive 1 s windows so the beacon path takes over; one SD-stall burst is a single window and never trips it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SdhRyfZ4eYuooXi9uy28aw
The battery role routed the raw int16 straight into the percent, assuming scale 1.0 / offset 0 — true of the EGT pod, but the whole point of the self-describing descriptor is that a future pod need not match. A pod declaring deci-percent would have read 870 -> "unknown". The frame value now goes through sampleToReal() like every other role, then the new host-tested batteryPercent(): sentinel/NaN -> 0xFF, otherwise clamped to 0-100 and rounded. NaN is detected with isNanF() because the device build is -Ofast, where isnan() folds to false and NaN comparisons are undefined. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SdhRyfZ4eYuooXi9uy28aw
Capture is persist-first: a failed setSetting() leaves the pairing window open so a later frame retries. But "a later frame" was every matching beacon, ~10 Hz — a full SD read-modify-write of SETTINGS.json per frame, for up to the 2-minute window, against a card that had just failed a write. That is the worst possible load for a struggling card and stalls the main loop on every beacon. Attempts now go through sensoregg_protocol::pairPersistDue(): the window's first attempt is immediate, later ones at least kPairPersistRetryMs (1 s) apart, wrap-safe and host-tested. Requesting a new window re-arms the immediate first attempt. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SdhRyfZ4eYuooXi9uy28aw
Review flagged clockFitPodToLogger() and ClockFit::halfRttMs as having no firmware consumer. That is true and intended — the surface is latest-value-only and DOVEX rows are stamped by the GPS clock — but nothing said so, so the API read as dead code. The tested API stays (it is the groundwork for per-sample row timestamping); sensoregg_gatt.h, sensoregg.h, the sketch's eggClockFit and plan 0018 now say plainly that only the boot_id epoch check is consumed today and the mapping is reserved for the resampling follow-up. No sketch state was dead: the anchored fit carries the epoch the reboot check uses. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SdhRyfZ4eYuooXi9uy28aw
Coverage — host-testable units📂 Overall coverage
📄 File coverage
|
This branch has not been deployed
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.
Requested by Dove · project thread
Summary
Before: the 4.2.0 release review found eight problems in the beta-only SensorEgg GATT link and pairing capture:
connect()hung until the 10 s timeout.tcFaultwere held under fresh stream stamps.After: each of those is fixed in its own commit, so any single fix can be reverted or cherry-picked.
be6329e(E1)3da5e88(E2)5372b3f(E3)a5d0f0a(E4)6c10853(E5)D<n>, and drops in three consecutive 1 s windows drop the link so the beacon takes over.4cb8d0c(E6)75f8e4b(E7)513af17(E8)The link states and their race-sensitive transition rules moved into the host-tested
sensoregg_gattunit. The sketch keeps itsEGG_LINK_*names as aliases. Everything stays behindBIRDSEYE_ENABLE_SENSOREGG, and there are flag-off and sim stubs for the newsensoreggFrameDrops().Judgement calls:
protoVersionkeeps its last beacon value.Type of change
How it was verified
clang-tidyclean (CI)sensoregg.inowas only syntax-checked against a hand-written Bluefruit/FreeRTOS mock, with the flag on and off.The native sim passes 6/6 with golden hashes unchanged. The sim builds with the flag off, so an unchanged hash also confirms no gating leak.
Checklist
CHANGELOG.mdupdated under[Unreleased]CLAUDE.mdsubsystem 14 and the plan 0017/0018 records updated (also corrects the claim thattcFaultreads false while streaming)tests/Related issues
Follow-up to the 4.2.0 release review on #160.
🤖 Generated with Claude Code
https://claude.ai/code/session_01SdhRyfZ4eYuooXi9uy28aw
Generated by Claude Code