From be6329e532097cb2491ca0b08b76e1d2eb1edb2f Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 04:44:28 +0000 Subject: [PATCH 1/8] plan 0018: never commit STREAMING onto a link that already dropped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01SdhRyfZ4eYuooXi9uy28aw --- BirdsEye/sensoregg.ino | 72 +++++++++++++++++++------- BirdsEye/sensoregg_gatt.cpp | 11 ++++ BirdsEye/sensoregg_gatt.h | 32 ++++++++++++ CHANGELOG.md | 10 ++++ docs/plans/0018-sensoregg-gatt-link.md | 18 +++++++ tests/sensoregg_gatt_test.cpp | 31 +++++++++++ 6 files changed, 156 insertions(+), 18 deletions(-) diff --git a/BirdsEye/sensoregg.ino b/BirdsEye/sensoregg.ino index 1d0550f..cb7fe17 100644 --- a/BirdsEye/sensoregg.ino +++ b/BirdsEye/sensoregg.ino @@ -132,17 +132,15 @@ static BLEClientCharacteristic eggChrDesc{BLEUuid(kPwDescUuid)}; static BLEClientCharacteristic eggChrSamp{BLEUuid(kPwSampUuid)}; static BLEClientCharacteristic eggChrClk{BLEUuid(kPwClkUuid)}; -// Ordered so "engaged" (a connection exists or is being made) is a -// single >= compare: BACKOFF sits between the idle states and -// CONNECTING on purpose. -enum EggLinkState : uint8_t { - EGG_LINK_IDLE = 0, // link not wanted (unpaired / gate closed) - EGG_LINK_WAIT_ADV, // wanted — the scan callback fires the connect - EGG_LINK_BACKOFF, // cooling off after a failure/disconnect - EGG_LINK_CONNECTING, // sd_ble_gap_connect in flight - EGG_LINK_BRINGUP, // discovery/reads running in the callback task - EGG_LINK_STREAMING, // notify subscription live, surface fed by GATT -}; +// The link states live in the host-tested sensoregg_gatt unit (with +// the transition rules that race the callback task); short local names +// keep the glue readable. Ordered so "engaged" is a single >= compare. +static constexpr uint8_t EGG_LINK_IDLE = sensoregg_gatt::LINK_IDLE; +static constexpr uint8_t EGG_LINK_WAIT_ADV = sensoregg_gatt::LINK_WAIT_ADV; +static constexpr uint8_t EGG_LINK_BACKOFF = sensoregg_gatt::LINK_BACKOFF; +static constexpr uint8_t EGG_LINK_CONNECTING = sensoregg_gatt::LINK_CONNECTING; +static constexpr uint8_t EGG_LINK_BRINGUP = sensoregg_gatt::LINK_BRINGUP; +static constexpr uint8_t EGG_LINK_STREAMING = sensoregg_gatt::LINK_STREAMING; static volatile uint8_t eggLinkState = EGG_LINK_IDLE; static volatile uint16_t eggConnHandle = BLE_CONN_HANDLE_INVALID; static volatile uint32_t eggLinkEventMs = 0; // stamp of the last transition @@ -323,6 +321,11 @@ static void sensoreggSampleNotifyCb(BLEClientCharacteristic* chr, // staging block's comment; plan 0018). Stages raw bytes; the main loop // parses and commits. No Serial here — the commit path narrates. static void sensoreggCentralConnectCb(uint16_t connHandle) { + // A ready/failed flag still up from an EARLIER bring-up (consumed by + // nobody — e.g. staged just before a sleep) must never pair with this + // link's staging buffers. Clear both before touching the buffers. + eggBringupReady = false; + eggBringupFailed = false; eggConnHandle = connHandle; eggLinkState = EGG_LINK_BRINGUP; eggLinkEventMs = millis(); @@ -465,6 +468,10 @@ void SENSOREGG_SLEEP() { Bluefruit.disconnect(eggConnHandle); } eggLinkState = EGG_LINK_IDLE; + // A bring-up staged before the sleep is moot — don't let it be + // consumed on the wake side. + eggBringupReady = false; + eggBringupFailed = false; } void SENSOREGG_WAKE() { @@ -573,13 +580,28 @@ void SENSOREGG_LOOP() { eggFastestIdx = sensoregg_gatt::fastestChannel(eggPod); sensoregg_gatt::clockFitAnchor(eggClockFit, clkBoot, clkPod, eggClkReqMs, eggClkRspMs); - eggLinkState = EGG_LINK_STREAMING; - debug(F("SensorEgg: GATT link up — fw ")); - debug(eggPod.fwMajor); - debug(F(".")); - debug(eggPod.fwMinor); - debug(F(", channels ")); - debugln(eggPod.channelCount); + // Commit only onto the bring-up we staged: the disconnect callback + // (higher-priority task) may already have written BACKOFF and + // dropped the handle. Check + set inside one critical section so + // it cannot land between them (BASEPRI-masked — the SoftDevice's + // radio interrupts are unaffected; sd_functions.ino precedent). + bool committed; + taskENTER_CRITICAL(); + committed = sensoregg_gatt::linkMayCommitStreaming( + (sensoregg_gatt::LinkState)eggLinkState, + eggConnHandle != BLE_CONN_HANDLE_INVALID); + if (committed) eggLinkState = EGG_LINK_STREAMING; + taskEXIT_CRITICAL(); + if (committed) { + debug(F("SensorEgg: GATT link up — fw ")); + debug(eggPod.fwMajor); + debug(F(".")); + debug(eggPod.fwMinor); + debug(F(", channels ")); + debugln(eggPod.channelCount); + } else { + debugln(F("SensorEgg: link gone before commit — not streaming")); + } } else { debugln(F("SensorEgg: descriptor/clock parse failed — dropping link")); if (eggConnHandle != BLE_CONN_HANDLE_INVALID) { @@ -657,6 +679,20 @@ void SENSOREGG_LOOP() { // connecting (it holds the fresh adv report). { const uint32_t now = millis(); + // Orphan guard: BRINGUP/STREAMING with no handle held means a + // disconnect was lost in a race — fall back to BACKOFF so the + // scanner and the retry path take over instead of wedging. + taskENTER_CRITICAL(); + { + const uint8_t fixed = sensoregg_gatt::linkReconcileOrphan( + (sensoregg_gatt::LinkState)eggLinkState, + eggConnHandle != BLE_CONN_HANDLE_INVALID); + if (fixed != eggLinkState) { + eggLinkState = fixed; + eggLinkEventMs = now; + } + } + taskEXIT_CRITICAL(); switch (eggLinkState) { case EGG_LINK_IDLE: if (eggLinkWanted()) { diff --git a/BirdsEye/sensoregg_gatt.cpp b/BirdsEye/sensoregg_gatt.cpp index bd80fb8..8c97874 100644 --- a/BirdsEye/sensoregg_gatt.cpp +++ b/BirdsEye/sensoregg_gatt.cpp @@ -140,4 +140,15 @@ uint32_t clockFitPodToLogger(const ClockFit& f, uint32_t podMs) { return f.loggerMs0 + (uint32_t)delta; } +bool linkMayCommitStreaming(LinkState state, bool handleValid) { + return state == LINK_BRINGUP && handleValid; +} + +LinkState linkReconcileOrphan(LinkState state, bool handleValid) { + if (!handleValid && (state == LINK_BRINGUP || state == LINK_STREAMING)) { + return LINK_BACKOFF; + } + return state; +} + } // namespace sensoregg_gatt diff --git a/BirdsEye/sensoregg_gatt.h b/BirdsEye/sensoregg_gatt.h index f6b13a1..de969f9 100644 --- a/BirdsEye/sensoregg_gatt.h +++ b/BirdsEye/sensoregg_gatt.h @@ -117,4 +117,36 @@ bool clockFitSameEpoch(const ClockFit& f, uint8_t frameBootId); // within +/- ~24.8 days of the anchor, i.e. always in practice. uint32_t clockFitPodToLogger(const ClockFit& f, uint32_t podMs); +// ---- Link state machine (review fixes 2026-09) --------------------------- +// The sketch's central-link states, owned here so the transition rules +// that race the Bluefruit callback task are host-tested. Ordered so +// "engaged" (a connection exists or is being made) is a single >= +// compare: BACKOFF sits between the idle states and CONNECTING on +// purpose. +enum LinkState : uint8_t { + LINK_IDLE = 0, // link not wanted (unpaired / gate closed) + LINK_WAIT_ADV, // wanted — the scan callback fires the connect + LINK_BACKOFF, // cooling off after a failure/disconnect + LINK_CONNECTING, // sd_ble_gap_connect in flight + LINK_BRINGUP, // discovery/reads running in the callback task + LINK_STREAMING, // notify subscription live, surface fed by GATT +}; + +// May the main loop promote a staged bring-up to STREAMING? Only while +// the bring-up it staged is still the live one: state still BRINGUP and +// a connection handle still held. The disconnect callback runs in a +// higher-priority task and can land between the bring-up's ready flag +// and the loop's commit — it has already written BACKOFF and dropped +// the handle, and an unconditional commit would overwrite that with +// STREAMING on a link that no longer exists (scanner held off, EGT NaN +// until shutdown). The caller evaluates this and writes the new state +// inside one critical section. +bool linkMayCommitStreaming(LinkState state, bool handleValid); + +// Reconcile guard: a state that claims a connection (BRINGUP or +// STREAMING) while no handle is held is an orphan left by a lost race — +// send it to BACKOFF so the normal retry path (and the scanner) takes +// over. Every other state is returned unchanged. +LinkState linkReconcileOrphan(LinkState state, bool handleValid); + } // namespace sensoregg_gatt diff --git a/CHANGELOG.md b/CHANGELOG.md index d8cd041..1501c91 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,16 @@ and this project aims to follow [Semantic Versioning](https://semver.org/spec/v2 (FAULT, no erase offer), and a volume that mounts on the retry boots normally. The recovered-on-probe path also now records the active SPI clock it had been leaving at 0. +- **SensorEgg GATT link can no longer wedge "streaming" with no link** + (plan 0018 review). If the egg dropped in the instant between the + bring-up finishing and the main loop committing it, the disconnect was + overwritten and the logger sat in STREAMING with no connection — the + scanner held off (a link counted as engaged) and EGT read `---` until + shutdown or a transfer. The commit now happens only while the staged + bring-up is still the live one (checked and set atomically), a + "connected" state with no connection handle is reconciled back to the + retry path, and a ready flag left over from an earlier bring-up can't + be consumed by a later one. ### Added - **SensorEgg GATT link** (plan 0018): a paired egg is no longer just diff --git a/docs/plans/0018-sensoregg-gatt-link.md b/docs/plans/0018-sensoregg-gatt-link.md index e8e32f5..123e85e 100644 --- a/docs/plans/0018-sensoregg-gatt-link.md +++ b/docs/plans/0018-sensoregg-gatt-link.md @@ -136,3 +136,21 @@ dropping the link for transfers and shutdown. (its anchor + epoch machinery ships here, unused by rows yet); LESC bonding when the egg's phase 4 lands; multi-pod (the state machine is single-link by design today). + +## Review fixes (2026-09) + +Findings from the post-merge review, one commit each. The link-state +rules that race the Bluefruit callback task moved into +`sensoregg_gatt` (`LinkState` + decision helpers) so they are +host-tested; the sketch keeps only the critical sections and radio calls. + +- **E1 — STREAMING with no link.** The disconnect callback (a + higher-priority task) could run between the bring-up's ready flag and + the main loop's commit; the unconditional `STREAMING` write then + resurrected a dead link and held the scanner off. Now: + `linkMayCommitStreaming()` (state still BRINGUP + handle held) is + evaluated and applied inside one `taskENTER_CRITICAL()` section; + `linkReconcileOrphan()` sends BRINGUP/STREAMING-without-a-handle to + BACKOFF every loop; the connect callback clears any stale + ready/failed flag before touching the staging buffers, and + `SENSOREGG_SLEEP()` clears them too. diff --git a/tests/sensoregg_gatt_test.cpp b/tests/sensoregg_gatt_test.cpp index 326cc1c..ee35e54 100644 --- a/tests/sensoregg_gatt_test.cpp +++ b/tests/sensoregg_gatt_test.cpp @@ -206,3 +206,34 @@ TEST_CASE("sensoregg_gatt - clock fit survives the u32 millis wrap") { // And a frame slightly BEFORE the anchor maps backward, not 4 Gms off. CHECK(clockFitPodToLogger(f, 0xFFFFFE00UL) == 500010UL - 256UL); } + +// --------------------------------------------------------------------------- +// Link state machine (review fixes 2026-09) +// --------------------------------------------------------------------------- + +TEST_CASE("sensoregg_gatt - STREAMING commits only onto the live bring-up") { + CHECK(linkMayCommitStreaming(LINK_BRINGUP, true)); + // E1: the disconnect callback beat the loop's commit — it already + // wrote BACKOFF and dropped the handle. The commit must not + // resurrect a dead link as STREAMING. + CHECK(!linkMayCommitStreaming(LINK_BACKOFF, false)); + CHECK(!linkMayCommitStreaming(LINK_BRINGUP, false)); + // A stale ready flag consumed after sleep (IDLE) never commits. + CHECK(!linkMayCommitStreaming(LINK_IDLE, false)); + CHECK(!linkMayCommitStreaming(LINK_IDLE, true)); + CHECK(!linkMayCommitStreaming(LINK_STREAMING, true)); + CHECK(!linkMayCommitStreaming(LINK_CONNECTING, true)); +} + +TEST_CASE("sensoregg_gatt - an orphaned connected state falls back to BACKOFF") { + CHECK(linkReconcileOrphan(LINK_STREAMING, false) == LINK_BACKOFF); + CHECK(linkReconcileOrphan(LINK_BRINGUP, false) == LINK_BACKOFF); + // Live links are untouched. + CHECK(linkReconcileOrphan(LINK_STREAMING, true) == LINK_STREAMING); + CHECK(linkReconcileOrphan(LINK_BRINGUP, true) == LINK_BRINGUP); + // States that legitimately hold no handle are untouched. + CHECK(linkReconcileOrphan(LINK_IDLE, false) == LINK_IDLE); + CHECK(linkReconcileOrphan(LINK_WAIT_ADV, false) == LINK_WAIT_ADV); + CHECK(linkReconcileOrphan(LINK_BACKOFF, false) == LINK_BACKOFF); + CHECK(linkReconcileOrphan(LINK_CONNECTING, false) == LINK_CONNECTING); +} From 3da5e88d2a3f295d326deb0d6036c280f34206ed Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 04:45:16 +0000 Subject: [PATCH 2/8] plan 0018: refuse a central connection that completes after sleep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01SdhRyfZ4eYuooXi9uy28aw --- BirdsEye/sensoregg.ino | 27 ++++++++++++++++++++++++-- BirdsEye/sensoregg_gatt.cpp | 4 ++++ BirdsEye/sensoregg_gatt.h | 12 ++++++++++++ CHANGELOG.md | 8 ++++++++ docs/plans/0018-sensoregg-gatt-link.md | 10 ++++++++++ tests/sensoregg_gatt_test.cpp | 13 +++++++++++++ 6 files changed, 72 insertions(+), 2 deletions(-) diff --git a/BirdsEye/sensoregg.ino b/BirdsEye/sensoregg.ino index cb7fe17..c10e852 100644 --- a/BirdsEye/sensoregg.ino +++ b/BirdsEye/sensoregg.ino @@ -321,6 +321,23 @@ static void sensoreggSampleNotifyCb(BLEClientCharacteristic* chr, // staging block's comment; plan 0018). Stages raw bytes; the main loop // parses and commits. No Serial here — the commit path narrates. static void sensoreggCentralConnectCb(uint16_t connHandle) { + // A connection we no longer want (SENSOREGG_SLEEP() ran while the + // SoftDevice was already establishing it, or the connect timeout gave + // up first) is dropped on arrival — otherwise it would come up in a + // transfer session / the charging park where nothing reconciles it. + // The handle is never adopted (the disconnect callback ignores a + // handle that is not eggConnHandle); a CONNECTING state whose gate + // dropped is retired to BACKOFF rather than left for the 10 s timeout. + if (!sensoregg_gatt::linkAcceptCentralConnect( + (sensoregg_gatt::LinkState)eggLinkState, eggSleeping, + eggLinkWanted())) { + Bluefruit.disconnect(connHandle); + if (eggLinkState == EGG_LINK_CONNECTING) { + eggLinkState = EGG_LINK_BACKOFF; + eggLinkEventMs = millis(); + } + return; + } // A ready/failed flag still up from an EARLIER bring-up (consumed by // nobody — e.g. staged just before a sleep) must never pair with this // link's staging buffers. Clear both before touching the buffers. @@ -365,8 +382,11 @@ static void sensoreggCentralConnectCb(uint16_t connHandle) { static void sensoreggCentralDisconnectCb(uint16_t connHandle, uint8_t reason) { - (void)connHandle; (void)reason; + // Only OUR link's drop moves the state machine. A connection the + // connect callback refused (above) was never adopted; its disconnect + // must not knock a sleeping/idle machine into BACKOFF. + if (connHandle != eggConnHandle) return; eggConnHandle = BLE_CONN_HANDLE_INVALID; eggLinkState = EGG_LINK_BACKOFF; eggLinkEventMs = millis(); @@ -460,7 +480,10 @@ void SENSOREGG_SLEEP() { // live link — this covers BLE transfer mode (BLE_SETUP calls this // first) and shutdown (bleShutdownQuiesce()'s bounded settle runs // after us and gives the disconnect airtime; its own disconnect only - // handles the peripheral handle). + // handles the peripheral handle). A link the SoftDevice already + // established but whose connect callback has not run yet is invisible + // here (no handle, connect_cancel() a no-op) — the callback itself + // refuses it on arrival (linkAcceptCentralConnect sees eggSleeping). if (eggLinkState == EGG_LINK_CONNECTING) { sd_ble_gap_connect_cancel(); } diff --git a/BirdsEye/sensoregg_gatt.cpp b/BirdsEye/sensoregg_gatt.cpp index 8c97874..10f501a 100644 --- a/BirdsEye/sensoregg_gatt.cpp +++ b/BirdsEye/sensoregg_gatt.cpp @@ -151,4 +151,8 @@ LinkState linkReconcileOrphan(LinkState state, bool handleValid) { return state; } +bool linkAcceptCentralConnect(LinkState state, bool sleeping, bool wanted) { + return state == LINK_CONNECTING && !sleeping && wanted; +} + } // namespace sensoregg_gatt diff --git a/BirdsEye/sensoregg_gatt.h b/BirdsEye/sensoregg_gatt.h index de969f9..1fa96f8 100644 --- a/BirdsEye/sensoregg_gatt.h +++ b/BirdsEye/sensoregg_gatt.h @@ -149,4 +149,16 @@ bool linkMayCommitStreaming(LinkState state, bool handleValid); // over. Every other state is returned unchanged. LinkState linkReconcileOrphan(LinkState state, bool handleValid); +// Should the central connect callback keep the connection it was just +// handed? Only when it is the connect we asked for (state CONNECTING) +// and the link is still wanted and the radio is not asleep. The +// SoftDevice can raise CONNECTED after SENSOREGG_SLEEP() already ran +// (its connect_cancel() is a no-op once the link exists, and the +// handle was not yet known to disconnect) or after the connect timeout +// gave up; accepting then would bring the link up during a transfer +// session / the charging park, where nothing reconciles it. A false +// answer means: disconnect that handle without adopting it (a +// CONNECTING state is retired to BACKOFF; IDLE/BACKOFF stay put). +bool linkAcceptCentralConnect(LinkState state, bool sleeping, bool wanted); + } // namespace sensoregg_gatt diff --git a/CHANGELOG.md b/CHANGELOG.md index 1501c91..2991420 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,6 +54,14 @@ and this project aims to follow [Semantic Versioning](https://semver.org/spec/v2 "connected" state with no connection handle is reconciled back to the retry path, and a ready flag left over from an earlier bring-up can't be consumed by a later one. +- **Shutdown and BLE transfer mode can no longer leave a SensorEgg link + up** (plan 0018 review). If the egg connection was completing inside + the radio at the moment the logger went to sleep or entered transfer + mode, the pending-connect cancel was a no-op and the link finished + coming up afterwards — occupying the radio for the whole transfer + session or charging park. The logger now drops such a late connection + the moment it arrives, and ignores the disconnect of a connection it + never adopted. ### Added - **SensorEgg GATT link** (plan 0018): a paired egg is no longer just diff --git a/docs/plans/0018-sensoregg-gatt-link.md b/docs/plans/0018-sensoregg-gatt-link.md index 123e85e..138bbb6 100644 --- a/docs/plans/0018-sensoregg-gatt-link.md +++ b/docs/plans/0018-sensoregg-gatt-link.md @@ -154,3 +154,13 @@ host-tested; the sketch keeps only the critical sections and radio calls. BACKOFF every loop; the connect callback clears any stale ready/failed flag before touching the staging buffers, and `SENSOREGG_SLEEP()` clears them too. +- **E2 — a link completing across `SENSOREGG_SLEEP()`.** With the + SoftDevice's CONNECTED raised but the deferred connect callback not + yet run, sleep saw no handle and `sd_ble_gap_connect_cancel()` was a + no-op; the callback then brought the link up inside a transfer + session or the charging park. The connect callback now asks + `linkAcceptCentralConnect()` (state CONNECTING, not sleeping, still + wanted) and disconnects the handle otherwise — which also covers a + connect that lands after the 10 s timeout gave up. The disconnect + callback ignores handles other than `eggConnHandle`, so refusing a + connection cannot knock the machine into BACKOFF. diff --git a/tests/sensoregg_gatt_test.cpp b/tests/sensoregg_gatt_test.cpp index ee35e54..e22e4a3 100644 --- a/tests/sensoregg_gatt_test.cpp +++ b/tests/sensoregg_gatt_test.cpp @@ -237,3 +237,16 @@ TEST_CASE("sensoregg_gatt - an orphaned connected state falls back to BACKOFF") CHECK(linkReconcileOrphan(LINK_BACKOFF, false) == LINK_BACKOFF); CHECK(linkReconcileOrphan(LINK_CONNECTING, false) == LINK_CONNECTING); } + +TEST_CASE("sensoregg_gatt - the connect callback keeps only a wanted connect") { + CHECK(linkAcceptCentralConnect(LINK_CONNECTING, false, true)); + // E2: SENSOREGG_SLEEP() ran while the SoftDevice was establishing + // the link (state already IDLE, sleep gate up). + CHECK(!linkAcceptCentralConnect(LINK_IDLE, true, false)); + CHECK(!linkAcceptCentralConnect(LINK_CONNECTING, true, true)); + // Gate dropped (race over / unpaired) while connecting. + CHECK(!linkAcceptCentralConnect(LINK_CONNECTING, false, false)); + // The connect timeout already gave up and moved to BACKOFF. + CHECK(!linkAcceptCentralConnect(LINK_BACKOFF, false, true)); + CHECK(!linkAcceptCentralConnect(LINK_WAIT_ADV, false, true)); +} From 5372b3f2b573280983380ca7c3ffb111bf0dcf25 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 04:47:35 +0000 Subject: [PATCH 3/8] plan 0018: clear beacon-only fields on stream commit, stamp each role MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01SdhRyfZ4eYuooXi9uy28aw --- BirdsEye/sensoregg.h | 24 ++++++--- BirdsEye/sensoregg.ino | 56 ++++++++++++++++++-- BirdsEye/sensoregg_gatt.cpp | 38 ++++++++++++++ BirdsEye/sensoregg_gatt.h | 46 ++++++++++++++++ CHANGELOG.md | 11 ++++ CLAUDE.md | 11 ++-- docs/plans/0018-sensoregg-gatt-link.md | 18 +++++++ tests/sensoregg_gatt_test.cpp | 72 ++++++++++++++++++++++++++ 8 files changed, 262 insertions(+), 14 deletions(-) diff --git a/BirdsEye/sensoregg.h b/BirdsEye/sensoregg.h index b396a42..2c36833 100644 --- a/BirdsEye/sensoregg.h +++ b/BirdsEye/sensoregg.h @@ -33,11 +33,18 @@ // the link for transfers and shutdown. While streaming, the scanner's // 44% duty is not paid at all. // -// GATT DEGRADATIONS (documented, deliberate): Sample frames carry no -// MCP STATUS, so sensoreggTcFault() reads false while streaming (an -// open probe still shows as sentinel -> NaN -> '---', the same visible -// outcome); sensoreggProtoVersion()/sensoreggPairingFlag() hold their -// last beacon values. +// WHAT THE STREAM REPORTS (plan 0018 review fixes): committing the +// stream CLEARS every beacon-only field rather than holding it — +// temperatures NaN and battery 0xFF until their own channel's first +// frame, tcFault and the pairing flag false (Sample frames carry no MCP +// STATUS and no pairing bit; an open probe still shows as sentinel -> +// NaN -> '---', the same visible outcome). Each value is then live only +// while ITS OWN channel keeps arriving (per-role stamps, staleness = +// the channel's frame cadence with one frame of slack, floored at the +// 1 s rule) — so an unmapped role (a pod with no IAT) stays NaN, and an +// EGT channel that stops while CJ continues goes NaN. Only +// sensoreggProtoVersion() keeps its last beacon value (it names the +// egg's beacon firmware). // // PAIRING (plan 0017): runtime MAC filter, persisted as the // "sensoregg_mac" setting ("AA:BB:CC:DD:EE:FF"; empty = unpaired = @@ -119,7 +126,8 @@ bool sensoreggLinkUp(); bool sensoreggAppHung(); // Latest EGT / cold junction in degC. NaN when the link is stale OR the -// egg reported the invalid sentinel (open probe, sensor fault). +// egg reported the invalid sentinel (open probe, sensor fault), or — +// on the GATT stream — when that value's own channel has gone stale. float sensoreggEgtC(); float sensoreggJunctionC(); @@ -133,7 +141,9 @@ float sensoreggAuxC(); uint8_t sensoreggBatteryPct(); // True while fresh AND the egg flags a thermocouple fault (open / -// out-of-range probe, MCP9600 STATUS input-range bit). +// out-of-range probe, MCP9600 STATUS input-range bit). Beacon-only: +// always false while the GATT stream is the source (frames carry no +// STATUS; the fault shows as a NaN EGT instead). bool sensoreggTcFault(); // Free-running egg sequence counter from the latest payload (debug). diff --git a/BirdsEye/sensoregg.ino b/BirdsEye/sensoregg.ino index c10e852..86dc866 100644 --- a/BirdsEye/sensoregg.ino +++ b/BirdsEye/sensoregg.ino @@ -177,6 +177,14 @@ static sensoregg_gatt::PodDescriptor eggPod; static int8_t eggRoleIdx[sensoregg_gatt::ROLE_COUNT] = {-1, -1, -1, -1}; static int8_t eggFastestIdx = 0; // whose frame seq feeds the zombie monitor static sensoregg_gatt::ClockFit eggClockFit; +// Stream surface bookkeeping (plan 0018 review, E3). eggReadingFromGatt +// is true while the stream was the last writer of eggReading — then +// every value accessor is additionally gated on ITS OWN role's receive +// stamp (eggRoleFresh), because the shared eggRxMs is kept fresh by any +// channel's frames. A beacon parse clears it (the beacon rewrites the +// whole Reading at once, so the plain 1 s rule is exact there). +static bool eggReadingFromGatt = false; +static sensoregg_gatt::RoleFreshness eggRoleFresh; // RACE-GATED SCANNER (plan 0012). The scanner's 40-of-every-90 ms radio // claim is only paid while a race session is running — the one time the EGT @@ -520,6 +528,7 @@ void SENSOREGG_LOOP() { sensoregg_protocol::Reading r; if (sensoregg_protocol::parsePayload(local, localLen, r)) { eggReading = r; + eggReadingFromGatt = false; eggRxMs = atMs; eggHaveReading = true; sensoregg_protocol::seqMonitorFeed(eggSeqMon, r.sequence, atMs); @@ -616,6 +625,13 @@ void SENSOREGG_LOOP() { if (committed) eggLinkState = EGG_LINK_STREAMING; taskEXIT_CRITICAL(); if (committed) { + // Start the stream from a clean surface: nothing a beacon left + // behind (tcFault, pairing flag, an aux/battery value the pod + // may never stream) may be held under a fresh frame stamp. + sensoregg_gatt::readingResetForStream(eggReading); + sensoregg_gatt::roleFreshnessReset(eggRoleFresh); + eggSeqMon = sensoregg_protocol::SeqMonitor(); + eggReadingFromGatt = true; debug(F("SensorEgg: GATT link up — fw ")); debug(eggPod.fwMajor); debug(F(".")); @@ -671,18 +687,28 @@ void SENSOREGG_LOOP() { const int16_t rawLast = f.raw[f.n - 1]; const float real = sensoregg_gatt::sampleToReal(rawLast, eggPod.ch[chIdx]); + int8_t role = -1; if (chIdx == eggRoleIdx[sensoregg_gatt::ROLE_EGT]) { eggReading.egtC = real; + role = sensoregg_gatt::ROLE_EGT; } else if (chIdx == eggRoleIdx[sensoregg_gatt::ROLE_CJ]) { eggReading.junctionC = real; + role = sensoregg_gatt::ROLE_CJ; } else if (chIdx == eggRoleIdx[sensoregg_gatt::ROLE_AUX]) { eggReading.auxC = real; + role = sensoregg_gatt::ROLE_AUX; } else if (chIdx == eggRoleIdx[sensoregg_gatt::ROLE_BATT]) { // Ratio channel is whole percent (scale 1.0) — route the raw, // sentinel/out-of-range -> the surface's 0xFF "unknown". eggReading.battery = (rawLast < 0 || rawLast > 100) ? (uint8_t)0xFF : (uint8_t)rawLast; + role = sensoregg_gatt::ROLE_BATT; + } + if (role >= 0) { + sensoregg_gatt::roleFreshnessStamp(eggRoleFresh, + (sensoregg_gatt::Role)role, atMs, + f.n, f.intervalMs); } eggReading.sequence = f.seq; // display: the stream's own counter // Zombie detection feeds on the FASTEST channel's frame seq only — @@ -805,6 +831,16 @@ void SENSOREGG_LOOP() { // DATA SURFACE /////////////////////////////////////////// +// Per-role gate on top of the link-level rule: while the stream was the +// last writer, a value is live only while its own channel is (E3). +// Takes a plain uint8_t: Arduino's auto-prototype generator places this +// signature above the includes, where sensoregg_gatt::Role is unknown. +static bool eggRoleLive(uint8_t role) { + return !eggReadingFromGatt || + sensoregg_gatt::roleFresh(eggRoleFresh, (sensoregg_gatt::Role)role, + millis()); +} + bool sensoreggLinkUp() { return eggHaveReading && sensoregg_protocol::isFresh(eggRxMs, millis()); } @@ -814,26 +850,38 @@ float sensoreggEgtC() { // link still yields NaN when the egg itself sent the invalid sentinel // OR when the egg's app has hung (radio beaconing a frozen payload — // a flat line must never masquerade as data). - if (!sensoreggLinkUp() || sensoreggAppHung()) return NAN; + if (!sensoreggLinkUp() || sensoreggAppHung() || + !eggRoleLive(sensoregg_gatt::ROLE_EGT)) { + return NAN; + } return eggReading.egtC; } float sensoreggJunctionC() { - if (!sensoreggLinkUp() || sensoreggAppHung()) return NAN; + if (!sensoreggLinkUp() || sensoreggAppHung() || + !eggRoleLive(sensoregg_gatt::ROLE_CJ)) { + return NAN; + } return eggReading.junctionC; } float sensoreggAuxC() { // Same gating as the EGT: stale link or hung app -> NaN. Also NaN when // the egg is v1 (no aux field) or its divider reported the sentinel. - if (!sensoreggLinkUp() || sensoreggAppHung()) return NAN; + if (!sensoreggLinkUp() || sensoreggAppHung() || + !eggRoleLive(sensoregg_gatt::ROLE_AUX)) { + return NAN; + } return eggReading.auxC; } uint8_t sensoreggBatteryPct() { // 0xFF = unknown: stale/hung link, v1 stub, or the egg's own // no-pack-fitted gate. Never report a stale percent as current. - if (!sensoreggLinkUp() || sensoreggAppHung()) return 0xFF; + if (!sensoreggLinkUp() || sensoreggAppHung() || + !eggRoleLive(sensoregg_gatt::ROLE_BATT)) { + return 0xFF; + } return eggReading.battery; } diff --git a/BirdsEye/sensoregg_gatt.cpp b/BirdsEye/sensoregg_gatt.cpp index 10f501a..6164c10 100644 --- a/BirdsEye/sensoregg_gatt.cpp +++ b/BirdsEye/sensoregg_gatt.cpp @@ -155,4 +155,42 @@ bool linkAcceptCentralConnect(LinkState state, bool sleeping, bool wanted) { return state == LINK_CONNECTING && !sleeping && wanted; } +void readingResetForStream(sensoregg_protocol::Reading& r) { + r.egtC = NAN; + r.junctionC = NAN; + r.auxC = NAN; + r.flags = 0; + r.pairingActive = false; + r.tcFault = false; + r.status = 0; + r.battery = 0xFF; + r.sequence = 0; + // protoVersion deliberately kept (see header). +} + +uint32_t roleStaleAfterMs(uint8_t n, uint16_t intervalMs) { + const uint32_t span = (uint32_t)n * (uint32_t)intervalMs; // <= 117*65535 + uint32_t stale = 2u * span; // no overflow + if (stale < sensoregg_protocol::kStalenessMs) { + stale = sensoregg_protocol::kStalenessMs; + } + if (stale > kRoleStaleMaxMs) stale = kRoleStaleMaxMs; + return stale; +} + +void roleFreshnessReset(RoleFreshness& f) { f = RoleFreshness(); } + +void roleFreshnessStamp(RoleFreshness& f, Role role, uint32_t atMs, + uint8_t n, uint16_t intervalMs) { + if (role >= ROLE_COUNT) return; + f.have[role] = true; + f.atMs[role] = atMs; + f.staleAfterMs[role] = roleStaleAfterMs(n, intervalMs); +} + +bool roleFresh(const RoleFreshness& f, Role role, uint32_t nowMs) { + if (role >= ROLE_COUNT || !f.have[role]) return false; + return (uint32_t)(nowMs - f.atMs[role]) < f.staleAfterMs[role]; +} + } // namespace sensoregg_gatt diff --git a/BirdsEye/sensoregg_gatt.h b/BirdsEye/sensoregg_gatt.h index 1fa96f8..2a07909 100644 --- a/BirdsEye/sensoregg_gatt.h +++ b/BirdsEye/sensoregg_gatt.h @@ -21,6 +21,8 @@ #include #include +#include "sensoregg_protocol.h" // Reading + kStalenessMs (the shared surface) + namespace sensoregg_gatt { constexpr uint8_t kMaxChannels = 21; // 8 + 21*24 = 512 = ATT max value @@ -161,4 +163,48 @@ LinkState linkReconcileOrphan(LinkState state, bool handleValid); // CONNECTING state is retired to BACKOFF; IDLE/BACKOFF stay put). bool linkAcceptCentralConnect(LinkState state, bool sleeping, bool wanted); +// ---- Stream surface rules (review fixes 2026-09) ------------------------- +// The GATT stream feeds the same sensoregg_protocol::Reading the beacon +// does, but it only carries the channels the descriptor maps — nothing +// else in that struct is refreshed by a frame, while every frame keeps +// the link's arrival stamp fresh. Two rules keep that from holding a +// value (a held value draws a flat line indistinguishable from data): + +// On committing STREAMING, clear everything the stream will (or can +// never) rewrite: temperatures NaN, battery 0xFF (unknown), flags / +// status / tcFault / pairingActive cleared, sequence 0. Sample frames +// carry no MCP STATUS and no pairing bit, so while streaming those +// read false (a fault still reaches the page as the sentinel -> NaN -> +// '---'). protoVersion is left as the last beacon's: it names the +// egg's beacon firmware, which the stream does not contradict. +void readingResetForStream(sensoregg_protocol::Reading& r); + +// Per-role receive stamps: each mapped role is fresh only while ITS OWN +// channel keeps arriving, so an EGT channel that stops while CJ carries +// on goes NaN instead of being held by CJ's traffic. A role that was +// never stamped (e.g. a pod with no IAT channel) is never fresh. +// +// Staleness per role = the frame's own cadence with one frame of slack, +// 2 x (n x interval_ms), floored at sensoregg_protocol::kStalenessMs +// (so a fast channel keeps exactly the 1 s rule) and capped at +// kRoleStaleMaxMs (a corrupt interval can't hold a value for minutes). +// The floor alone would be wrong for slow channels: the EGT pod's IAT +// runs at 1 s and its battery at 30 s, so a flat 1 s rule would flap +// IAT and blank the battery forever. +constexpr uint32_t kRoleStaleMaxMs = 60000; + +uint32_t roleStaleAfterMs(uint8_t n, uint16_t intervalMs); + +struct RoleFreshness { + bool have[ROLE_COUNT] = {false, false, false, false}; + uint32_t atMs[ROLE_COUNT] = {0, 0, 0, 0}; + uint32_t staleAfterMs[ROLE_COUNT] = {0, 0, 0, 0}; +}; + +void roleFreshnessReset(RoleFreshness& f); +void roleFreshnessStamp(RoleFreshness& f, Role role, uint32_t atMs, + uint8_t n, uint16_t intervalMs); +// Wrap-safe: fresh while (now - at) < staleAfter. +bool roleFresh(const RoleFreshness& f, Role role, uint32_t nowMs); + } // namespace sensoregg_gatt diff --git a/CHANGELOG.md b/CHANGELOG.md index 2991420..bf8e595 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -62,6 +62,17 @@ and this project aims to follow [Semantic Versioning](https://semver.org/spec/v2 session or charging park. The logger now drops such a late connection the moment it arrives, and ignores the disconnect of a connection it never adopted. +- **SensorEgg GATT stream no longer holds stale values** (plan 0018 + review). Fields only the beacon carries were kept from the last beacon + while the stream's frames kept the link "fresh": a thermocouple fault + seen once could read `*TC FAULT*` for the whole session, and a pod + without an intake-air channel logged a flat line into `Temp2`. And + since frames from any channel kept the link alive, an EGT channel that + stopped while cold-junction frames continued left EGT frozen. Starting + the stream now clears those fields, and every value goes `nan`/`---` + as soon as its own channel stops arriving (the 1 s rule for EGT/CJ; a + slow channel gets one frame of slack at its own rate, e.g. 2 s for a + 1 s intake-air channel). ### Added - **SensorEgg GATT link** (plan 0018): a paired egg is no longer just diff --git a/CLAUDE.md b/CLAUDE.md index 513d644..ba9ab86 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1371,9 +1371,14 @@ hardware needs no power switch. Wake = chip reset = fresh `setup()`. subsystem 1) and is not paid at all while streaming. GATT bring-up (blocking discovery/reads) runs in the Bluefruit callback task — the one documented deviation from "callbacks only copy"; the notify data - plane keeps the copy-only frame-ring discipline. Degradations while - streaming: `tcFault` reads false (frames carry no MCP STATUS), - `protoVersion`/`pairingFlag` hold their last beacon values. + plane keeps the copy-only frame-ring discipline. **Committing the + stream clears the beacon-only fields** (temps NaN, battery 0xFF, + `tcFault`/pairing flag false — frames carry no MCP STATUS or pairing + bit; only `protoVersion` keeps its beacon value), and each value is + live only while **its own channel** keeps arriving + (`sensoregg_gatt::RoleFreshness`: 2 × the frame's n × interval, + floored at the 1 s rule, capped at 60 s) — the shared arrival stamp + alone would let any channel's traffic hold another's value. - **Scanner robustness (bench-proven, do not remove)**: (1) `Scanner.filterMSD(0xFFFF)` rejects ambient packets INLINE — Bluefruit self-resumes filtered reports, while an accepted report pauses scanning diff --git a/docs/plans/0018-sensoregg-gatt-link.md b/docs/plans/0018-sensoregg-gatt-link.md index 138bbb6..63cc7c9 100644 --- a/docs/plans/0018-sensoregg-gatt-link.md +++ b/docs/plans/0018-sensoregg-gatt-link.md @@ -164,3 +164,21 @@ host-tested; the sketch keeps only the critical sections and radio calls. connect that lands after the 10 s timeout gave up. The disconnect callback ignores handles other than `eggConnHandle`, so refusing a connection cannot knock the machine into BACKOFF. +- **E3 — beacon-only fields held under GATT traffic.** `tcFault`, the + aux temperature and the battery kept their last beacon values while + every frame refreshed the shared `eggRxMs`: a stuck `*TC FAULT*`, and + an IAT-less pod logging a flat line into `Temp2` (the never-hold rule, + broken). And because zombie detection watches only the fastest + channel, an EGT channel that stopped while CJ carried on held EGT. + Now `readingResetForStream()` clears the surface on commit (temps NaN, + battery 0xFF, flags/status/tcFault/pairing false, seq 0; protoVersion + kept) and the zombie monitor restarts; `RoleFreshness` stamps each + mapped role on its own frames, and every value accessor also requires + its role fresh while the stream was the last writer. *Judgement + call:* the per-role window is the channel's own frame cadence with one + frame of slack (`2 × n × interval_ms`), **floored at the 1 s + `kStalenessMs` rule** and capped at 60 s — a flat 1 s rule would flap + the pod's 1 s IAT channel and permanently blank its 30 s battery, while + the EGT/CJ channels (250 ms) keep exactly the beacon's 1 s rule. + This supersedes the "graceful degradations" bullet above: nothing but + `protoVersion` holds a beacon value any more. diff --git a/tests/sensoregg_gatt_test.cpp b/tests/sensoregg_gatt_test.cpp index e22e4a3..325dee3 100644 --- a/tests/sensoregg_gatt_test.cpp +++ b/tests/sensoregg_gatt_test.cpp @@ -250,3 +250,75 @@ TEST_CASE("sensoregg_gatt - the connect callback keeps only a wanted connect") { CHECK(!linkAcceptCentralConnect(LINK_BACKOFF, false, true)); CHECK(!linkAcceptCentralConnect(LINK_WAIT_ADV, false, true)); } + +// --------------------------------------------------------------------------- +// Stream surface rules (review fixes 2026-09) +// --------------------------------------------------------------------------- + +TEST_CASE("sensoregg_gatt - committing a stream clears every beacon-only field") { + sensoregg_protocol::Reading r; + r.egtC = 650.0f; + r.junctionC = 30.0f; + r.auxC = 40.0f; // an IAT-less pod would otherwise hold this forever + r.flags = 0x03; + r.pairingActive = true; + r.tcFault = true; // would otherwise read *TC FAULT* all session + r.status = 0x10; + r.battery = 87; + r.sequence = 1234; + r.protoVersion = 2; + readingResetForStream(r); + CHECK(std::isnan(r.egtC)); + CHECK(std::isnan(r.junctionC)); + CHECK(std::isnan(r.auxC)); + CHECK(r.flags == 0); + CHECK(!r.pairingActive); + CHECK(!r.tcFault); + CHECK(r.status == 0); + CHECK(r.battery == 0xFF); + CHECK(r.sequence == 0); + CHECK(r.protoVersion == 2); // kept: names the beacon firmware +} + +TEST_CASE("sensoregg_gatt - role staleness follows the channel's own cadence") { + // Fast channel (EGT, one 250 ms sample per frame): exactly the + // beacon's 1 s rule. + CHECK(roleStaleAfterMs(1, 250) == sensoregg_protocol::kStalenessMs); + // Batched fast channel: 4 x 250 ms = 1 s per frame -> 2 s. + CHECK(roleStaleAfterMs(4, 250) == 2000UL); + // IAT at 1 s: 2 s, not a 1 s rule that would flap every frame. + CHECK(roleStaleAfterMs(1, 1000) == 2000UL); + // Battery at 30 s: 60 s (the cap). + CHECK(roleStaleAfterMs(1, 30000) == kRoleStaleMaxMs); + // Corrupt / extreme interval: capped, never minutes. + CHECK(roleStaleAfterMs(117, 65535) == kRoleStaleMaxMs); + // Aperiodic / zero: the floor. + CHECK(roleStaleAfterMs(1, 0) == sensoregg_protocol::kStalenessMs); +} + +TEST_CASE("sensoregg_gatt - each role goes stale on its own channel") { + RoleFreshness f; + // Never stamped (a pod without that channel) -> never fresh. + CHECK(!roleFresh(f, ROLE_AUX, 0)); + CHECK(!roleFresh(f, ROLE_EGT, 100)); + + roleFreshnessStamp(f, ROLE_EGT, 1000, 1, 250); + roleFreshnessStamp(f, ROLE_CJ, 1000, 1, 250); + CHECK(roleFresh(f, ROLE_EGT, 1999)); + // EGT's frames stop; CJ's keep coming. EGT must go NaN on its own. + roleFreshnessStamp(f, ROLE_CJ, 1900, 1, 250); + CHECK(!roleFresh(f, ROLE_EGT, 2000)); + CHECK(roleFresh(f, ROLE_CJ, 2000)); + CHECK(!roleFresh(f, ROLE_AUX, 2000)); + + // Wrap-safe across the u32 millis wrap. + roleFreshnessStamp(f, ROLE_BATT, 0xFFFFFF00UL, 1, 30000); + CHECK(roleFresh(f, ROLE_BATT, 0x00000100UL)); + + // Out-of-range role is ignored, never fresh. + roleFreshnessStamp(f, ROLE_COUNT, 0, 1, 250); + CHECK(!roleFresh(f, ROLE_COUNT, 0)); + + roleFreshnessReset(f); + CHECK(!roleFresh(f, ROLE_CJ, 2000)); +} From a5d0f0a9eb504cb30961be16b05432b88b811476 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 04:48:06 +0000 Subject: [PATCH 4/8] plan 0018: back off at once when the SoftDevice refuses a connect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01SdhRyfZ4eYuooXi9uy28aw --- BirdsEye/sensoregg.ino | 14 +++++++++++++- BirdsEye/sensoregg_gatt.cpp | 4 ++++ BirdsEye/sensoregg_gatt.h | 8 ++++++++ CHANGELOG.md | 6 ++++++ docs/plans/0018-sensoregg-gatt-link.md | 6 ++++++ tests/sensoregg_gatt_test.cpp | 7 +++++++ 6 files changed, 44 insertions(+), 1 deletion(-) diff --git a/BirdsEye/sensoregg.ino b/BirdsEye/sensoregg.ino index 86dc866..0808292 100644 --- a/BirdsEye/sensoregg.ino +++ b/BirdsEye/sensoregg.ino @@ -258,9 +258,21 @@ static void sensoreggScanCallback(ble_gap_evt_adv_report_t* report) { sensoregg_protocol::matchesMagic(buf, len) && sensoreggMacAccepted(report->peer_addr.addr) && sensoreggIsPaired()) { + // CONNECTING is posted BEFORE the request: the connect callback + // only adopts a link it finds in CONNECTING. eggLinkState = EGG_LINK_CONNECTING; eggLinkEventMs = millis(); - Bluefruit.Central.connect(report); + const bool accepted = Bluefruit.Central.connect(report); + if (!accepted) { + // Refused outright — no connect callback will ever come. Back off + // now and un-pause the scanner (it is still parked on this + // report) so the beacon keeps feeding the surface meanwhile. + eggLinkState = sensoregg_gatt::linkAfterConnectRequest(accepted); + eggLinkEventMs = millis(); + if (eggScanWanted()) { + Bluefruit.Scanner.resume(); + } + } return; } diff --git a/BirdsEye/sensoregg_gatt.cpp b/BirdsEye/sensoregg_gatt.cpp index 6164c10..cac97a2 100644 --- a/BirdsEye/sensoregg_gatt.cpp +++ b/BirdsEye/sensoregg_gatt.cpp @@ -155,6 +155,10 @@ bool linkAcceptCentralConnect(LinkState state, bool sleeping, bool wanted) { return state == LINK_CONNECTING && !sleeping && wanted; } +LinkState linkAfterConnectRequest(bool accepted) { + return accepted ? LINK_CONNECTING : LINK_BACKOFF; +} + void readingResetForStream(sensoregg_protocol::Reading& r) { r.egtC = NAN; r.junctionC = NAN; diff --git a/BirdsEye/sensoregg_gatt.h b/BirdsEye/sensoregg_gatt.h index 2a07909..07beb3d 100644 --- a/BirdsEye/sensoregg_gatt.h +++ b/BirdsEye/sensoregg_gatt.h @@ -163,6 +163,14 @@ LinkState linkReconcileOrphan(LinkState state, bool handleValid); // CONNECTING state is retired to BACKOFF; IDLE/BACKOFF stay put). bool linkAcceptCentralConnect(LinkState state, bool sleeping, bool wanted); +// State after the scan callback asked the SoftDevice to connect. A +// refused request (Central.connect() == false: radio busy, invalid +// params) never produces a connect callback, so staying CONNECTING +// would sit out the full 10 s connect timeout — plus the 5 s backoff — +// with the scanner paused on the report that triggered it. Refused -> +// BACKOFF immediately (and the caller resumes the scanner). +LinkState linkAfterConnectRequest(bool accepted); + // ---- Stream surface rules (review fixes 2026-09) ------------------------- // The GATT stream feeds the same sensoregg_protocol::Reading the beacon // does, but it only carries the channels the descriptor maps — nothing diff --git a/CHANGELOG.md b/CHANGELOG.md index bf8e595..c7562f0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -73,6 +73,12 @@ and this project aims to follow [Semantic Versioning](https://semver.org/spec/v2 as soon as its own channel stops arriving (the 1 s rule for EGT/CJ; a slow channel gets one frame of slack at its own rate, e.g. 2 s for a 1 s intake-air channel). +- **A refused SensorEgg connect no longer costs 15 s of silence** (plan + 0018 review). When the radio refused the connection request outright, + the logger still waited out the full 10 s connect timeout and 5 s + backoff with the scanner paused, so neither the GATT stream nor the + beacon fed the EGT readout. It now backs off immediately and resumes + the scanner. ### Added - **SensorEgg GATT link** (plan 0018): a paired egg is no longer just diff --git a/docs/plans/0018-sensoregg-gatt-link.md b/docs/plans/0018-sensoregg-gatt-link.md index 63cc7c9..8c2a680 100644 --- a/docs/plans/0018-sensoregg-gatt-link.md +++ b/docs/plans/0018-sensoregg-gatt-link.md @@ -182,3 +182,9 @@ host-tested; the sketch keeps only the critical sections and radio calls. the EGT/CJ channels (250 ms) keep exactly the beacon's 1 s rule. This supersedes the "graceful degradations" bullet above: nothing but `protoVersion` holds a beacon value any more. +- **E4 — `Central.connect()` result ignored.** A refused request never + yields a connect callback, so the machine sat in CONNECTING for the + 10 s timeout (+5 s backoff) with the scanner paused on the triggering + report. Now `linkAfterConnectRequest(false)` → BACKOFF, stamped, and + the callback resumes the scanner (when the scan is wanted) so the + beacon keeps feeding the surface. diff --git a/tests/sensoregg_gatt_test.cpp b/tests/sensoregg_gatt_test.cpp index 325dee3..dac73c2 100644 --- a/tests/sensoregg_gatt_test.cpp +++ b/tests/sensoregg_gatt_test.cpp @@ -322,3 +322,10 @@ TEST_CASE("sensoregg_gatt - each role goes stale on its own channel") { roleFreshnessReset(f); CHECK(!roleFresh(f, ROLE_CJ, 2000)); } + +TEST_CASE("sensoregg_gatt - a refused connect request backs off at once") { + CHECK(linkAfterConnectRequest(true) == LINK_CONNECTING); + // E4: no connect callback will follow a refusal — don't sit out the + // 10 s connect timeout with the scanner paused. + CHECK(linkAfterConnectRequest(false) == LINK_BACKOFF); +} From 6c10853679c7543fc1e9165625a2437175dc8a5c Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 04:49:24 +0000 Subject: [PATCH 5/8] plan 0018: size the frame ring for a full frame, act on sustained drops MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. 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) Claude-Session: https://claude.ai/code/session_01SdhRyfZ4eYuooXi9uy28aw --- BirdsEye/display_pages.ino | 9 +++++- BirdsEye/sensoregg.h | 5 +++ BirdsEye/sensoregg.ino | 39 ++++++++++++++++++++--- BirdsEye/sensoregg_gatt.cpp | 24 ++++++++++++++ BirdsEye/sensoregg_gatt.h | 32 +++++++++++++++++++ BirdsEye/sim/stubs/module_stubs.cpp | 1 + CHANGELOG.md | 8 +++++ docs/plans/0018-sensoregg-gatt-link.md | 9 ++++++ tests/sensoregg_gatt_test.cpp | 44 ++++++++++++++++++++++++++ 9 files changed, 165 insertions(+), 6 deletions(-) diff --git a/BirdsEye/display_pages.ino b/BirdsEye/display_pages.ino index 79e6e19..c9e5c73 100644 --- a/BirdsEye/display_pages.ino +++ b/BirdsEye/display_pages.ino @@ -784,11 +784,18 @@ void displayPage_egg_test() { } display.println(); - // Row 4: live flags from the latest frame. + // Row 4: live flags from the latest frame, plus the GATT notify + // frames the logger has had to drop since boot (D, only once + // nonzero; clamped so the worst case "FLG PAIR FAULT D9999" is 20). display.print(F("FLG")); if (sensoreggPairingFlag()) display.print(F(" PAIR")); if (sensoreggTcFault()) display.print(F(" FAULT")); if (!sensoreggPairingFlag() && !sensoreggTcFault()) display.print(F(" -")); + const uint32_t eggDrops = sensoreggFrameDrops(); + if (eggDrops != 0) { + display.print(F(" D")); + display.print(eggDrops > 9999UL ? 9999UL : eggDrops); + } display.println(); // Row 5: which egg the filter accepts. diff --git a/BirdsEye/sensoregg.h b/BirdsEye/sensoregg.h index 2c36833..6b07b6f 100644 --- a/BirdsEye/sensoregg.h +++ b/BirdsEye/sensoregg.h @@ -214,3 +214,8 @@ uint8_t sensoreggLinkMode(); // Live ATT MTU of the egg link (0 when not connected). 247 after a // successful exchange; frames are sized to it on the egg side. uint16_t sensoreggGattMtu(); + +// Cumulative notify frames the logger could not keep since boot (ring +// full, or oversize). Shown on EGG TEST as D; drops in 3 consecutive +// 1 s windows drop the link so the beacon path takes over. +uint32_t sensoreggFrameDrops(); diff --git a/BirdsEye/sensoregg.ino b/BirdsEye/sensoregg.ino index 0808292..80959d7 100644 --- a/BirdsEye/sensoregg.ino +++ b/BirdsEye/sensoregg.ino @@ -161,16 +161,27 @@ static volatile bool eggBringupReady = false; static volatile bool eggBringupFailed = false; // Notify frame ring (the data plane keeps the copy-only discipline): -// the notify callback memcpys, SENSOREGG_LOOP drains. 32-byte slots -// cover the egg's <=26-byte frames; a fatter future pod's frames are -// counted as drops rather than truncated. -static uint8_t eggFrameBuf[8][32]; +// the notify callback memcpys, SENSOREGG_LOOP drains. Slots are sized +// to kMaxFrameLen (244 = ATT_MTU 247 - 3, a full 117-sample frame), the +// largest notify the link can legally carry — the old 32-byte slots +// silently dropped any batch over 11 samples while the link stayed up +// looking healthy. 8 x 244 = ~1.9 KB (was 256 B): the depth is kept at +// 8 so a main-loop stall (SD GC, 100 ms-2 s) at the stream's ~7 Hz +// frame rate still loses at most a second of superseded values, and +// ~1.7 KB extra is noise next to the 8 KB track-JSON buffers on the +// 256 KB part. Drops (ring full, or an impossible oversize frame) are +// counted, shown on EGG TEST, and a sustained run of them drops the +// link (sensoregg_gatt::DropMonitor) so the beacon path takes over. +static uint8_t eggFrameBuf[8][sensoregg_gatt::kMaxFrameLen]; static volatile uint8_t eggFrameLen[8]; static volatile uint32_t eggFrameAtMs[8]; static volatile bool eggFrameReady[8] = {false}; static volatile uint8_t eggFrameW = 0; // notify callback writes static uint8_t eggFrameR = 0; // main loop reads -static uint32_t eggFrameDrops = 0; +static volatile uint32_t eggFrameDrops = 0; // callback writes, loop reads +static sensoregg_gatt::DropMonitor eggDropMon; // main loop only +static_assert(sensoregg_gatt::kMaxFrameLen <= 255, + "eggFrameLen is a uint8_t"); // Committed pod identity (main loop only, valid while STREAMING). static sensoregg_gatt::PodDescriptor eggPod; @@ -644,6 +655,8 @@ void SENSOREGG_LOOP() { sensoregg_gatt::roleFreshnessReset(eggRoleFresh); eggSeqMon = sensoregg_protocol::SeqMonitor(); eggReadingFromGatt = true; + sensoregg_gatt::dropMonitorReset(eggDropMon, eggFrameDrops, + millis()); debug(F("SensorEgg: GATT link up — fw ")); debug(eggPod.fwMajor); debug(F(".")); @@ -735,6 +748,17 @@ void SENSOREGG_LOOP() { eggRateCount++; // the test page's Hz meter counts GATT frames too } + // Sustained frame drops (E5): the stream isn't being consumed — drop + // the link so the beacon path takes over; the reconnect after the + // backoff starts a fresh evaluation. + if (eggLinkState == EGG_LINK_STREAMING && + eggConnHandle != BLE_CONN_HANDLE_INVALID && + sensoregg_gatt::dropMonitorUpdate(eggDropMon, eggFrameDrops, + millis())) { + debugln(F("SensorEgg: sustained frame drops — dropping link")); + Bluefruit.disconnect(eggConnHandle); + } + // Link reconcile: post/withdraw the wanted state, time out a stuck // connect, expire the backoff. The scan callback does the actual // connecting (it holds the fresh adv report). @@ -987,6 +1011,10 @@ uint8_t sensoreggLinkMode() { return sensoreggLinkUp() ? 1 : 0; } +uint32_t sensoreggFrameDrops() { + return eggFrameDrops; +} + uint16_t sensoreggGattMtu() { if (eggConnHandle == BLE_CONN_HANDLE_INVALID) return 0; BLEConnection* conn = Bluefruit.Connection(eggConnHandle); @@ -1041,5 +1069,6 @@ float sensoreggPacketHz() { return 0.0f; } void sensoreggGattClientInit() {} uint8_t sensoreggLinkMode() { return 0; } uint16_t sensoreggGattMtu() { return 0; } +uint32_t sensoreggFrameDrops() { return 0; } #endif // BIRDSEYE_ENABLE_SENSOREGG diff --git a/BirdsEye/sensoregg_gatt.cpp b/BirdsEye/sensoregg_gatt.cpp index cac97a2..6a4d5b4 100644 --- a/BirdsEye/sensoregg_gatt.cpp +++ b/BirdsEye/sensoregg_gatt.cpp @@ -159,6 +159,30 @@ LinkState linkAfterConnectRequest(bool accepted) { return accepted ? LINK_CONNECTING : LINK_BACKOFF; } +void dropMonitorReset(DropMonitor& m, uint32_t totalDrops, uint32_t nowMs) { + m.started = true; + m.windowStartMs = nowMs; + m.dropsAtWindowStart = totalDrops; + m.badWindows = 0; +} + +bool dropMonitorUpdate(DropMonitor& m, uint32_t totalDrops, uint32_t nowMs) { + if (!m.started) { + dropMonitorReset(m, totalDrops, nowMs); + return false; + } + if ((uint32_t)(nowMs - m.windowStartMs) < kDropWindowMs) return false; + const bool bad = totalDrops != m.dropsAtWindowStart; + m.badWindows = bad ? (uint8_t)(m.badWindows + 1) : (uint8_t)0; + m.windowStartMs = nowMs; + m.dropsAtWindowStart = totalDrops; + if (m.badWindows >= kDropWindowsToFail) { + m.badWindows = 0; + return true; + } + return false; +} + void readingResetForStream(sensoregg_protocol::Reading& r) { r.egtC = NAN; r.junctionC = NAN; diff --git a/BirdsEye/sensoregg_gatt.h b/BirdsEye/sensoregg_gatt.h index 07beb3d..d871737 100644 --- a/BirdsEye/sensoregg_gatt.h +++ b/BirdsEye/sensoregg_gatt.h @@ -32,6 +32,10 @@ constexpr uint8_t kMinRecordLen = 24; // schema v1; larger = newer schema, constexpr size_t kSampleHeaderLen = 10; constexpr size_t kClockLen = 6; constexpr uint8_t kMaxSamplesPerFrame = 117; // (247-3-10)/2, spec section 5 +// Largest Sample notify the link can carry: ATT_MTU 247 (the logger's +// configCentralConn cap) - 3 = 244 = a full 117-sample frame. Receive +// buffers sized to this take every legal frame at any negotiated MTU. +constexpr size_t kMaxFrameLen = 10 + 2 * (size_t)kMaxSamplesPerFrame; constexpr int16_t kInvalidSentinel = INT16_MIN; // 0x8000 = "no valid reading" // One parsed channel descriptor record (PW_CHANNEL_SCHEMA.md section 5). @@ -171,6 +175,34 @@ bool linkAcceptCentralConnect(LinkState state, bool sleeping, bool wanted); // BACKOFF immediately (and the caller resumes the scanner). LinkState linkAfterConnectRequest(bool accepted); +// Sustained-drop rule for the notify frame ring. A drop is a frame the +// logger could not keep (ring full — the main loop fell behind — or a +// frame over kMaxFrameLen). One bad window is normal: an SD garbage- +// collection stall parks the main loop for up to ~2 s and the ring +// overflows once, and latest-value semantics lose nothing but +// superseded samples. Drops in kDropWindowsToFail CONSECUTIVE +// kDropWindowMs windows mean the stream is not being consumed; the +// caller then drops the link so the beacon path (and the reconnect) +// takes over rather than streaming into the floor. Feed the cumulative +// drop counter every loop while streaming; a stall spanning several +// windows is judged as one window (windows tumble on loop time). +constexpr uint32_t kDropWindowMs = 1000; +constexpr uint8_t kDropWindowsToFail = 3; + +struct DropMonitor { + bool started = false; + uint32_t windowStartMs = 0; + uint32_t dropsAtWindowStart = 0; + uint8_t badWindows = 0; +}; + +// Start a fresh evaluation at the current cumulative count. +void dropMonitorReset(DropMonitor& m, uint32_t totalDrops, uint32_t nowMs); + +// Returns true once, when the kDropWindowsToFail-th consecutive bad +// window closes (and re-arms the count). +bool dropMonitorUpdate(DropMonitor& m, uint32_t totalDrops, uint32_t nowMs); + // ---- Stream surface rules (review fixes 2026-09) ------------------------- // The GATT stream feeds the same sensoregg_protocol::Reading the beacon // does, but it only carries the channels the descriptor maps — nothing diff --git a/BirdsEye/sim/stubs/module_stubs.cpp b/BirdsEye/sim/stubs/module_stubs.cpp index c375dc7..a61344c 100644 --- a/BirdsEye/sim/stubs/module_stubs.cpp +++ b/BirdsEye/sim/stubs/module_stubs.cpp @@ -147,6 +147,7 @@ float sensoreggPacketHz() { return 0.0f; } void sensoreggGattClientInit() {} uint8_t sensoreggLinkMode() { return 0; } uint16_t sensoreggGattMtu() { return 0; } +uint32_t sensoreggFrameDrops() { return 0; } // ---- neopixel.ino surface ---- diff --git a/CHANGELOG.md b/CHANGELOG.md index c7562f0..dd6d6ef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -79,6 +79,14 @@ and this project aims to follow [Semantic Versioning](https://semver.org/spec/v2 backoff with the scanner paused, so neither the GATT stream nor the beacon fed the EGT readout. It now backs off immediately and resumes the scanner. +- **SensorEgg GATT frames of any legal size are received, and lost frames + are visible** (plan 0018 review). The logger's receive slots held only + 32 bytes, so an egg batching more than 11 samples per frame (the spec + allows 117) had every such frame silently discarded while the link + looked healthy. Slots now take the largest frame the link can carry + (244 bytes), the running count of dropped frames shows on EGG TEST as + `D`, and drops in three consecutive seconds drop the link so the + beacon takes over instead of streaming into the floor. ### Added - **SensorEgg GATT link** (plan 0018): a paired egg is no longer just diff --git a/docs/plans/0018-sensoregg-gatt-link.md b/docs/plans/0018-sensoregg-gatt-link.md index 8c2a680..a63227b 100644 --- a/docs/plans/0018-sensoregg-gatt-link.md +++ b/docs/plans/0018-sensoregg-gatt-link.md @@ -188,3 +188,12 @@ host-tested; the sketch keeps only the critical sections and radio calls. report. Now `linkAfterConnectRequest(false)` → BACKOFF, stamped, and the callback resumes the scanner (when the scan is wanted) so the beacon keeps feeding the surface. +- **E5 — oversize frames dropped silently.** 32-byte ring slots took + only ≤11-sample frames; the spec allows 117 per frame. Slots are now + `kMaxFrameLen` = 244 (ATT_MTU 247 − 3: every legal frame at any MTU the + logger negotiates), 8 deep = ~1.9 KB (was 256 B; depth kept so a ~1 s + main-loop stall at the ~7 Hz frame rate loses only superseded values). + `eggFrameDrops` is surfaced (`sensoreggFrameDrops()`, EGG TEST row 4 + `D`), and `DropMonitor` drops the link after drops in 3 consecutive + 1 s windows — one SD-stall burst is judged as a single window and + never fires it. diff --git a/tests/sensoregg_gatt_test.cpp b/tests/sensoregg_gatt_test.cpp index dac73c2..5cf14d7 100644 --- a/tests/sensoregg_gatt_test.cpp +++ b/tests/sensoregg_gatt_test.cpp @@ -329,3 +329,47 @@ TEST_CASE("sensoregg_gatt - a refused connect request backs off at once") { // 10 s connect timeout with the scanner paused. CHECK(linkAfterConnectRequest(false) == LINK_BACKOFF); } + +// --------------------------------------------------------------------------- +// Frame ring sizing + sustained-drop rule (review fixes 2026-09) +// --------------------------------------------------------------------------- + +TEST_CASE("sensoregg_gatt - the largest legal frame fits the receive slot") { + CHECK(kMaxFrameLen == 244); // ATT_MTU 247 - 3 + CHECK(kMaxFrameLen == 247 - 3); + // And a full-size frame parses. + uint8_t buf[kMaxFrameLen] = {0}; + buf[9] = kMaxSamplesPerFrame; + SampleFrame f; + CHECK(parseSampleFrame(buf, sizeof(buf), f)); + CHECK(f.n == kMaxSamplesPerFrame); +} + +TEST_CASE("sensoregg_gatt - one stall's burst of drops never drops the link") { + DropMonitor m; + CHECK(!dropMonitorUpdate(m, 0, 0)); // first call arms + // A 2 s SD stall: the loop comes back once, 10 frames lost. + CHECK(!dropMonitorUpdate(m, 10, 2000)); + // Healthy afterwards: the bad streak resets. + CHECK(!dropMonitorUpdate(m, 10, 3000)); + CHECK(!dropMonitorUpdate(m, 12, 4000)); // one more bad window + CHECK(!dropMonitorUpdate(m, 12, 5000)); // clean again + CHECK(!dropMonitorUpdate(m, 12, 5999)); // mid-window: no verdict +} + +TEST_CASE("sensoregg_gatt - drops in consecutive windows drop the link once") { + DropMonitor m; + dropMonitorReset(m, 100, 0); + CHECK(!dropMonitorUpdate(m, 101, 1000)); // bad 1 + CHECK(!dropMonitorUpdate(m, 101, 1500)); // mid-window + CHECK(!dropMonitorUpdate(m, 105, 2000)); // bad 2 + CHECK(dropMonitorUpdate(m, 106, 3000)); // bad 3 -> fail + // Re-armed: it takes another full streak to fire again. + CHECK(!dropMonitorUpdate(m, 107, 4000)); + CHECK(!dropMonitorUpdate(m, 108, 5000)); + CHECK(dropMonitorUpdate(m, 109, 6000)); + // Wrap-safe window timing across the millis wrap. + dropMonitorReset(m, 0, 0xFFFFFF00UL); + CHECK(!dropMonitorUpdate(m, 1, 0x00000100UL)); // only 512 ms elapsed + CHECK(!dropMonitorUpdate(m, 1, 0x00000400UL)); // window closes, bad 1 +} From 4cb8d0c9bf1ae532b6e6e7b7d9ee5a4a298954c6 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 04:49:58 +0000 Subject: [PATCH 6/8] plan 0018: convert the battery channel through its descriptor scaling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01SdhRyfZ4eYuooXi9uy28aw --- BirdsEye/sensoregg.ino | 9 ++++----- BirdsEye/sensoregg_gatt.cpp | 9 +++++++++ BirdsEye/sensoregg_gatt.h | 8 ++++++++ CHANGELOG.md | 6 ++++++ docs/plans/0018-sensoregg-gatt-link.md | 4 ++++ tests/sensoregg_gatt_test.cpp | 20 ++++++++++++++++++++ 6 files changed, 51 insertions(+), 5 deletions(-) diff --git a/BirdsEye/sensoregg.ino b/BirdsEye/sensoregg.ino index 80959d7..4b24560 100644 --- a/BirdsEye/sensoregg.ino +++ b/BirdsEye/sensoregg.ino @@ -723,11 +723,10 @@ void SENSOREGG_LOOP() { eggReading.auxC = real; role = sensoregg_gatt::ROLE_AUX; } else if (chIdx == eggRoleIdx[sensoregg_gatt::ROLE_BATT]) { - // Ratio channel is whole percent (scale 1.0) — route the raw, - // sentinel/out-of-range -> the surface's 0xFF "unknown". - eggReading.battery = (rawLast < 0 || rawLast > 100) - ? (uint8_t)0xFF - : (uint8_t)rawLast; + // Through the descriptor's scale/offset like every other channel + // (the EGT pod happens to declare 1.0/0, a future pod need not); + // sentinel -> 0xFF "unknown", otherwise clamped 0-100. + eggReading.battery = sensoregg_gatt::batteryPercent(real); role = sensoregg_gatt::ROLE_BATT; } if (role >= 0) { diff --git a/BirdsEye/sensoregg_gatt.cpp b/BirdsEye/sensoregg_gatt.cpp index 6a4d5b4..1af4ff0 100644 --- a/BirdsEye/sensoregg_gatt.cpp +++ b/BirdsEye/sensoregg_gatt.cpp @@ -3,6 +3,8 @@ #include #include +#include "nan_bits.h" + namespace sensoregg_gatt { namespace { @@ -87,6 +89,13 @@ float sampleToReal(int16_t raw, const ChannelInfo& c) { return (float)raw * c.scale + c.offset; } +uint8_t batteryPercent(float real) { + if (isNanF(real)) return 0xFF; + if (real <= 0.0f) return 0; + if (real >= 100.0f) return 100; + return (uint8_t)(real + 0.5f); +} + void mapChannels(const PodDescriptor& pd, int8_t outIdx[ROLE_COUNT]) { for (uint8_t r = 0; r < ROLE_COUNT; r++) outIdx[r] = -1; for (uint8_t i = 0; i < pd.channelCount; i++) { diff --git a/BirdsEye/sensoregg_gatt.h b/BirdsEye/sensoregg_gatt.h index d871737..8a1a6d5 100644 --- a/BirdsEye/sensoregg_gatt.h +++ b/BirdsEye/sensoregg_gatt.h @@ -89,6 +89,14 @@ bool parseClock(const uint8_t* d, size_t len, uint8_t& bootId, // sentinel becomes NaN BEFORE the conversion (never scaled). float sampleToReal(int16_t raw, const ChannelInfo& c); +// The surface's battery percent from a battery-role channel's +// engineering value (i.e. AFTER sampleToReal — the descriptor's +// scale/offset apply to the ratio channel like any other): NaN (the +// sentinel) -> 0xFF "unknown"; otherwise clamped to 0-100 and rounded. +// NaN is detected by bit pattern (nan_bits.h) — the device builds with +// -Ofast, where isnan() folds to false and NaN comparisons are UB. +uint8_t batteryPercent(float real); + // Descriptor-driven role mapping — the consumer never hardcodes channel // ids. Temperatures route by the schema's normative names ("EGT", "CJ", // "IAT"); the battery routes by quantity 0x08 (ratio). outIdx[] holds diff --git a/CHANGELOG.md b/CHANGELOG.md index dd6d6ef..f46b080 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -87,6 +87,12 @@ and this project aims to follow [Semantic Versioning](https://semver.org/spec/v2 (244 bytes), the running count of dropped frames shows on EGG TEST as `D`, and drops in three consecutive seconds drop the link so the beacon takes over instead of streaming into the floor. +- **SensorEgg battery over GATT honours the pod's declared scaling** + (plan 0018 review). The battery channel's raw value was used as a + percent directly, ignoring the scale/offset the pod's channel table + declares, so a pod reporting in tenths of a percent would have read as + "unknown". It now converts like every other channel and clamps to + 0–100. ### Added - **SensorEgg GATT link** (plan 0018): a paired egg is no longer just diff --git a/docs/plans/0018-sensoregg-gatt-link.md b/docs/plans/0018-sensoregg-gatt-link.md index a63227b..9ee1bdd 100644 --- a/docs/plans/0018-sensoregg-gatt-link.md +++ b/docs/plans/0018-sensoregg-gatt-link.md @@ -197,3 +197,7 @@ host-tested; the sketch keeps only the critical sections and radio calls. `D`), and `DropMonitor` drops the link after drops in 3 consecutive 1 s windows — one SD-stall burst is judged as a single window and never fires it. +- **E6 — battery ignored scale/offset.** Now `sampleToReal()` then + `batteryPercent()` (NaN → 0xFF, clamp 0–100, rounded; NaN tested by + bit pattern for `-Ofast`). Out-of-range values now clamp instead of + reading "unknown". diff --git a/tests/sensoregg_gatt_test.cpp b/tests/sensoregg_gatt_test.cpp index 5cf14d7..799297e 100644 --- a/tests/sensoregg_gatt_test.cpp +++ b/tests/sensoregg_gatt_test.cpp @@ -373,3 +373,23 @@ TEST_CASE("sensoregg_gatt - drops in consecutive windows drop the link once") { CHECK(!dropMonitorUpdate(m, 1, 0x00000100UL)); // only 512 ms elapsed CHECK(!dropMonitorUpdate(m, 1, 0x00000400UL)); // window closes, bad 1 } + +TEST_CASE("sensoregg_gatt - battery percent honours the descriptor scaling") { + ChannelInfo c; + c.scale = 1.0f; + c.offset = 0.0f; + CHECK(batteryPercent(sampleToReal(87, c)) == 87); + CHECK(batteryPercent(sampleToReal(INT16_MIN, c)) == 0xFF); // sentinel + // E6: a pod declaring deci-percent must not read 870 -> "unknown". + c.scale = 0.1f; + CHECK(batteryPercent(sampleToReal(870, c)) == 87); + CHECK(batteryPercent(sampleToReal(875, c)) == 88); // rounds + // An offset applies too. + c.scale = 1.0f; + c.offset = -10.0f; + CHECK(batteryPercent(sampleToReal(60, c)) == 50); + // Out of range clamps rather than wrapping a uint8_t. + c.offset = 0.0f; + CHECK(batteryPercent(sampleToReal(250, c)) == 100); + CHECK(batteryPercent(sampleToReal(-5, c)) == 0); +} From 75f8e4b89bcfbdebdc9e8c6289e00f453dd2f3b2 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 04:50:36 +0000 Subject: [PATCH 7/8] plan 0017: throttle the pairing-capture persist retry to ~1 Hz MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01SdhRyfZ4eYuooXi9uy28aw --- BirdsEye/sensoregg.ino | 14 ++++++++++++-- BirdsEye/sensoregg_protocol.cpp | 5 +++++ BirdsEye/sensoregg_protocol.h | 13 +++++++++++++ CHANGELOG.md | 5 +++++ CLAUDE.md | 5 +++-- docs/plans/0017-sensoregg-pairing-menu.md | 11 +++++++++++ tests/sensoregg_protocol_test.cpp | 15 +++++++++++++++ 7 files changed, 64 insertions(+), 4 deletions(-) diff --git a/BirdsEye/sensoregg.ino b/BirdsEye/sensoregg.ino index 4b24560..9a4bd97 100644 --- a/BirdsEye/sensoregg.ino +++ b/BirdsEye/sensoregg.ino @@ -66,6 +66,10 @@ static uint8_t eggMacFilter[6] = SENSOREGG_MAC; static volatile bool eggPairingActive = false; // capture window open static volatile bool eggTestActive = false; // EGG TEST bench latch static uint32_t eggPairStartMs = 0; // main loop only +// Capture persist throttle (main loop only): a failing settings write +// is retried at ~1 Hz, not at the beacon rate (plan 0017 review). +static bool eggPairPersistTried = false; +static uint32_t eggPairPersistLastMs = 0; // Per-slot advertiser address (raw LSB-first), captured alongside the // payload so the main-loop drain can pair without the callback ever // learning any protocol. Plain RAM like eggBuf; the ready flags below @@ -560,11 +564,16 @@ void SENSOREGG_LOOP() { // Pairing capture (plan 0017): first frame whose egg advertises // its own pairing-window flag wins. PERSIST FIRST (camera // precedent, camera_ble.ino) — a failed SD write leaves the - // window open so the next frame retries. The RAM filter is + // window open and a later frame retries, throttled to ~1 Hz + // (pairPersistDue) so a failing card isn't hammered at 10 Hz. The RAM filter is // updated while eggPairingActive still bypasses it in the // callback, so a torn filter read can never reject the captured // egg; the flag clears last. - if (eggPairingActive && r.pairingActive) { + if (eggPairingActive && r.pairingActive && + sensoregg_protocol::pairPersistDue(eggPairPersistTried, + eggPairPersistLastMs, atMs)) { + eggPairPersistTried = true; + eggPairPersistLastMs = atMs; uint8_t human[6]; char macStr[sensoregg_protocol::kMacStrLen]; sensoregg_protocol::macReverse(localPeer, human); @@ -947,6 +956,7 @@ bool sensoreggIsPaired() { void sensoreggRequestPair() { // Idempotent; a re-request restarts the timeout clock. eggPairStartMs = millis(); + eggPairPersistTried = false; // a fresh window's first capture is immediate eggPairingActive = true; } diff --git a/BirdsEye/sensoregg_protocol.cpp b/BirdsEye/sensoregg_protocol.cpp index d8d11c4..5bdbd9f 100644 --- a/BirdsEye/sensoregg_protocol.cpp +++ b/BirdsEye/sensoregg_protocol.cpp @@ -148,4 +148,9 @@ bool macAccepts(const uint8_t filterHuman[6], const uint8_t peerLsbFirst[6]) { return true; } +bool pairPersistDue(bool attempted, uint32_t lastAttemptMs, uint32_t nowMs) { + if (!attempted) return true; + return (uint32_t)(nowMs - lastAttemptMs) >= kPairPersistRetryMs; +} + } // namespace sensoregg_protocol diff --git a/BirdsEye/sensoregg_protocol.h b/BirdsEye/sensoregg_protocol.h index 2a84527..f882974 100644 --- a/BirdsEye/sensoregg_protocol.h +++ b/BirdsEye/sensoregg_protocol.h @@ -169,6 +169,19 @@ bool seqMonitorLive(const SeqMonitor& m, uint32_t nowMs); // before giving up. Matches the camera's pairing timeout. constexpr uint32_t kPairingTimeoutMs = 120000; +// Capture persist retry throttle. A capture persists FIRST (an SD +// read-modify-write of /SETTINGS.json); when that write fails the +// window stays open so a later frame retries. Unthrottled, every +// matching beacon (~10 Hz) re-ran the whole SD read-modify-write for up +// to the 2-minute window against a card that is already failing. +// ~1 Hz is plenty for a human-scale pairing gesture. +constexpr uint32_t kPairPersistRetryMs = 1000; + +// May a capture persist be attempted now? Always for the window's first +// attempt (attempted == false); afterwards only once kPairPersistRetryMs +// has passed since the last attempt (wrap-safe). +bool pairPersistDue(bool attempted, uint32_t lastAttemptMs, uint32_t nowMs); + // "AA:BB:CC:DD:EE:FF" + NUL. constexpr size_t kMacStrLen = 18; diff --git a/CHANGELOG.md b/CHANGELOG.md index f46b080..a8e19cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -93,6 +93,11 @@ and this project aims to follow [Semantic Versioning](https://semver.org/spec/v2 declares, so a pod reporting in tenths of a percent would have read as "unknown". It now converts like every other channel and clamps to 0–100. +- **A failing SD card is no longer hammered during egg pairing** (plan + 0017 review). When saving a newly captured egg failed, the logger + retried the settings write on every matching beacon — about ten full + read-modify-writes of `SETTINGS.json` a second for up to two minutes. + Retries are now paced at about once a second. ### Added - **SensorEgg GATT link** (plan 0018): a paired egg is no longer just diff --git a/CLAUDE.md b/CLAUDE.md index ba9ab86..284e970 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1403,8 +1403,9 @@ hardware needs no power switch. Wake = chip reset = fresh `setup()`. filter (so a different egg can be captured while one is paired), and the first parsed frame advertising the egg's OWN pairing-window flag (egg-side long-press, flags bit0) wins — persist-first (a failed SD - write keeps the window open to retry), then the RAM filter, then the - window closes. Unpair = persist-first `""` → accept-any. Applied LIVE + write keeps the window open to retry, throttled to ~1 Hz by + `pairPersistDue` rather than the ~10 Hz beacon rate), then the RAM + filter, then the window closes. Unpair = persist-first `""` → accept-any. Applied LIVE on pair/unpair — no reboot, same exception as `camera_serial`. The scan callback filters length + magic + MAC, copies **up to the largest known layout** (`kPayloadLenMax` — the old fixed-14 copy diff --git a/docs/plans/0017-sensoregg-pairing-menu.md b/docs/plans/0017-sensoregg-pairing-menu.md index 25e88a8..9cd0aaa 100644 --- a/docs/plans/0017-sensoregg-pairing-menu.md +++ b/docs/plans/0017-sensoregg-pairing-menu.md @@ -115,3 +115,14 @@ read BLE task), same care as `eggSleeping`. - Status: shipped with this plan's PR into BETA. - Deferred: manual MAC entry page (B1 reserved); DOVEX egg-battery column; sim egg-frame injection + flag-1 golden variant. + +## Review fixes (2026-09) + +- **E7 — capture persist retried at the beacon rate.** Persist-first + keeps the window open when `setSetting()` fails, but the retry fired + on 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 is already failing. Retries now go through + `sensoregg_protocol::pairPersistDue()` (first attempt of a window + immediate, then ≥ `kPairPersistRetryMs` = 1 s apart; host-tested); + `sensoreggRequestPair()` re-arms the immediate first attempt. diff --git a/tests/sensoregg_protocol_test.cpp b/tests/sensoregg_protocol_test.cpp index fa4321c..835626d 100644 --- a/tests/sensoregg_protocol_test.cpp +++ b/tests/sensoregg_protocol_test.cpp @@ -397,3 +397,18 @@ TEST_CASE("sensoregg_protocol - macAccepts pins the LSB-first convention") { // exact bug class the reversal exists to prevent. CHECK(!macAccepts(filterHuman, filterHuman)); } + +TEST_CASE("sensoregg_protocol - pairing persist retries are throttled to ~1 Hz") { + using namespace sensoregg_protocol; + // The window's first capture attempt is immediate. + CHECK(pairPersistDue(false, 0, 0)); + CHECK(pairPersistDue(false, 5000, 5000)); + // After a failed write, beacons at ~10 Hz must not re-run the SD + // read-modify-write each time. + CHECK(!pairPersistDue(true, 5000, 5100)); + CHECK(!pairPersistDue(true, 5000, 5999)); + CHECK(pairPersistDue(true, 5000, 6000)); + // Wrap-safe. + CHECK(!pairPersistDue(true, 0xFFFFFF00UL, 0x00000100UL)); // 512 ms + CHECK(pairPersistDue(true, 0xFFFFFF00UL, 0x00000300UL)); // 1024 ms +} From 513af17a6a93cb107b65c9a16542a25ba610d2f1 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 04:51:05 +0000 Subject: [PATCH 8/8] plan 0018: document the clock fit as anchored but not yet consumed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01SdhRyfZ4eYuooXi9uy28aw --- BirdsEye/sensoregg.h | 5 ++++- BirdsEye/sensoregg.ino | 3 +++ BirdsEye/sensoregg_gatt.h | 9 +++++++++ docs/plans/0018-sensoregg-gatt-link.md | 11 ++++++++++- 4 files changed, 26 insertions(+), 2 deletions(-) diff --git a/BirdsEye/sensoregg.h b/BirdsEye/sensoregg.h index 6b07b6f..d02a451 100644 --- a/BirdsEye/sensoregg.h +++ b/BirdsEye/sensoregg.h @@ -21,7 +21,10 @@ // SoftDevice. A PAIRED egg in range, while the egg radio is wanted, // gets a GATT connection and streams the PerchWerks Sensor Service // (self-describing channel table, per-channel batch frames stamped at -// acquisition, a Clock/boot_id epoch — sensoregg_gatt.{h,cpp}). +// acquisition, a Clock/boot_id epoch — sensoregg_gatt.{h,cpp}). The +// clock fit is anchored on every bring-up but only its epoch (boot_id) +// check is consumed; mapping pod time onto logger time is reserved for +// a future per-sample resampling plan — values here are latest-sample. // Everything else — unpaired pods, the pre-connect window, backoff, // and the pairing capture itself — rides the passive PW-ADV observer // exactly as before (no SCAN_REQ; sensoregg_protocol.{h,cpp}). The two diff --git a/BirdsEye/sensoregg.ino b/BirdsEye/sensoregg.ino index 9a4bd97..20f808c 100644 --- a/BirdsEye/sensoregg.ino +++ b/BirdsEye/sensoregg.ino @@ -191,6 +191,9 @@ static_assert(sensoregg_gatt::kMaxFrameLen <= 255, static sensoregg_gatt::PodDescriptor eggPod; static int8_t eggRoleIdx[sensoregg_gatt::ROLE_COUNT] = {-1, -1, -1, -1}; static int8_t eggFastestIdx = 0; // whose frame seq feeds the zombie monitor +// Anchored on every bring-up; only its epoch (boot_id) is consumed today +// — pod->logger time mapping is reserved for per-sample resampling +// (plan 0018 follow-ups). static sensoregg_gatt::ClockFit eggClockFit; // Stream surface bookkeeping (plan 0018 review, E3). eggReadingFromGatt // is true while the stream was the last writer of eggReading — then diff --git a/BirdsEye/sensoregg_gatt.h b/BirdsEye/sensoregg_gatt.h index 8a1a6d5..31acb68 100644 --- a/BirdsEye/sensoregg_gatt.h +++ b/BirdsEye/sensoregg_gatt.h @@ -114,6 +114,15 @@ int8_t fastestChannel(const PodDescriptor& pd); // anchors on the connect-time Clock read: logger time = the // request/response midpoint (tightest pair available), pod time = the // value read. boot_id inequality = the pod's millis restarted (epoch). +// +// FIRMWARE CONSUMPTION TODAY: the sketch anchors a fit on every +// bring-up and uses ONLY clockFitSameEpoch() (the pod-reboot check). +// clockFitPodToLogger() and halfRttMs have no firmware consumer yet — +// the surface is latest-value-only and DOVEX rows are stamped by the +// logger's GPS clock. They are kept, tested, deliberately: reserved for +// the follow-up plan that timestamps per-sample rows from the fit (plan +// 0018 "Status / follow-ups"). Do not read an unused-API warning as +// dead code to delete. struct ClockFit { bool valid = false; uint8_t bootId = 0; diff --git a/docs/plans/0018-sensoregg-gatt-link.md b/docs/plans/0018-sensoregg-gatt-link.md index 9ee1bdd..2e1fbb9 100644 --- a/docs/plans/0018-sensoregg-gatt-link.md +++ b/docs/plans/0018-sensoregg-gatt-link.md @@ -133,7 +133,8 @@ dropping the link for transfers and shutdown. - Status: shipped with this plan's PR into BETA. - Follow-ups: per-sample DOVEX row timestamping using the clock fit - (its anchor + epoch machinery ships here, unused by rows yet); + (its anchor + epoch machinery ships here; only the epoch check is + consumed — `clockFitPodToLogger()` is reserved for this); LESC bonding when the egg's phase 4 lands; multi-pod (the state machine is single-link by design today). @@ -201,3 +202,11 @@ host-tested; the sketch keeps only the critical sections and radio calls. `batteryPercent()` (NaN → 0xFF, clamp 0–100, rounded; NaN tested by bit pattern for `-Ofast`). Out-of-range values now clamp instead of reading "unknown". +- **E8 — clock-fit API with no firmware consumer.** Stated plainly + rather than deleted: the sketch anchors `eggClockFit` on every + bring-up and consumes only `clockFitSameEpoch()` (pod-reboot + detection). `clockFitPodToLogger()` and `halfRttMs` are anchored but + **not yet consumed** — reserved, tested, for the per-sample row + timestamping follow-up below. Documented in `sensoregg_gatt.h`, + `sensoregg.h` and at the sketch's `eggClockFit`. No sketch state was + truly dead (the fit itself carries the epoch).