diff --git a/docs/DESIGN_COOPERATIVE_REFRESH_WAIT_2026-07-27.md b/docs/DESIGN_COOPERATIVE_REFRESH_WAIT_2026-07-27.md deleted file mode 100644 index 65cdaad..0000000 --- a/docs/DESIGN_COOPERATIVE_REFRESH_WAIT_2026-07-27.md +++ /dev/null @@ -1,180 +0,0 @@ -# Cooperative refresh wait - -**Status: PROPOSED — not implemented.** Written 2026-07-27 on `feat/unify-nrf-esp-phase3` -after a live nRF capture showed a 16 s refresh stalling BLE event servicing long enough -to corrupt reconnect handling. Nothing in this document has been coded; the two bugs it -describes as *observed* were fixed separately in `c4bd4bc`, which treated the symptom. - -## The problem - -`waitforrefresh()` ([`src/display_service.cpp:747`](../src/display_service.cpp)) polls the -panel BUSY line with a raw `delay(10)`: - -```c -for (size_t i = 0; i < (size_t)(timeout * 100); i++){ - delay(10); - if(i % 50 == 0) od_log_raw("."); - if(!bbepIsBusy(&bbep)){ ... return true; } -} -``` - -`delay()` is not `idleDelay()`. For the whole refresh — up to 60 s by the bound, ~16 s -measured on a Spectra 6-colour panel — the loop task does **nothing**: - -| Serviced by `idleDelay()` | Serviced during a refresh | -|---|---| -| `processButtonEvents()` / `processTouchInput()` | — | -| `processLedFlash()` | — | -| `buzzerService()` | — | -| `epdSessionTick()` | — | -| `serviceBleTx()` | — | -| (`loop()` only) `serviceBleEvents()` | — | - -The refresh is a dead zone, not merely a busy one. - -### Observed consequence - -nRF, `nrf52840custom-debug`, 2026-07-27: - -``` -[0194.860] I: === BLE CLIENT DISCONNECTED (nRF) === <- flag latched, callback task -[0199.713] I: Refresh took 15.98 seconds <- loop task unblocks -[0212.962] I: === BLE CLIENT CONNECTED (nRF) === <- next client -[0213.167] D: [BLE][Q:0] URX 0x0080 (12 B): 00 80 ... <- its first command, queued -[0213.241] I: Disconnect reason: 19 <- 18 s stale, serviced now -[0213.242] W: Dropped 1 queued command(s) ... <- ate the NEW client's frame -``` - -Two defects fell out of that single stall, both fixed in `c4bd4bc`: - -1. `bleRxQueueDiscardAll()` discarded the ring at *service* time rather than at - disconnect time, so a reconnect landing inside the stall lost its first command. - Fixed with a boundary captured in the disconnect callback. -2. `serviceBleDisconnectCleanup()`'s "owner still up" guard was inside - `#ifdef OPENDISPLAY_HAS_WIFI`, leaving nRF with no guard at all — - `resetPipeWriteState()` would have destroyed the new client's session. Fixed by - hoisting the `ble.isConnected()` half out of the `#ifdef`. - -Both fixes make deferred work *correct when serviced late*. Neither reduces the -lateness. Any future deferred-work path inherits the same 16 s exposure. - -## Blocking is not required by the hardware - -`bbepRefresh()` issues `SSD1608_MASTER_ACTIVATE` and returns. The panel drives the -waveform on its own and reports progress on a single GPIO. The MCU has no work to do -for the duration — `waitforrefresh` is polling a pin. There is no DMA to babysit, no -SPI transaction held open, no timing constraint tighter than the 10 ms poll. - -Two properties of the existing code confirm the design was already headed this way. - -**The protocol does not couple the ACK to the refresh.** `directWriteFinishAndRefresh` -sends the END ack and force-flushes it *before* starting the refresh -([`src/display_service.cpp:2369-2379`](../src/display_service.cpp)), with a comment -naming the reason — the loop task is the response ring's only drainer. The refresh -outcome is reported afterwards as an independent -`RESP_DIRECT_WRITE_REFRESH_SUCCESS` ([`:2418`](../src/display_service.cpp)). Clients -already accept the result as a later, separate frame. - -**`epdRefreshInProgress` is already the right shape.** It is raised around exactly this -window, and both `serviceBleDisconnectCleanup()` and `serviceBleAdvertisingRestart()` -already gate on it ([`src/main.cpp:317`, `:367`](../src/main.cpp)). But it can only ever -be observed as `true` from *inside* the blocking wait, so nothing can currently act on -it. The flag exists for a design that was never realized. - -## Option A — cooperative wait (recommended) - -Keep the call signature and the straight-line control flow. Replace the `delay(10)` -inside the poll with the servicing block `idleDelay()` already runs. - -Factor that block out of `idleDelay()` ([`src/main.cpp:619`](../src/main.cpp)) into a -shared `serviceLoopMaintenance()` and call it from both. Two copies of the list is how -the two directions drift — the same failure mode this branch removed from the RX/TX -logging. - -The refresh still owns the loop task; no new commands dispatch; RX stays queued. What -changes is that the 16 s stops being dead. - -**Hard invariant: `serviceLoopMaintenance()` must NOT call `serviceBleRx()`.** -`waitforrefresh` is reached from inside a command handler, dispatched by -`serviceBleRx()`. Dispatching from within would make every handler reentrant and -corrupt multi-frame transfer state mid-stream. `idleDelay()` already states this rule in -its comment; the shared helper must carry it. - -`idleDelay()`'s early return on `bleRxQueuePending()` is **not** part of the shared -block — it is `idleDelay`-specific (cap command latency) and wrong for the refresh poll, -which must keep waiting on BUSY regardless of queued RX. - -Calling `serviceBleEvents()` from the wait is safe and is the point of the change: -`requestFastLink()` touches only the BLE stack, and the two flag consumers already defer -on `epdRefreshInProgress`, so cleanup and advertising correctly stay deferred until the -refresh ends. - -**Effect on the observed bug:** the disconnect is consumed within ~10 ms instead of -18 s, so the reconnect race never opens. The `c4bd4bc` boundary remains correct and -still covers the residual window; it simply stops being load-bearing. - -**Cost:** BLE notifications and button/touch input now interleave with SPI-idle polling. -No SPI transaction is open during the wait, so there is no bus contention. Battery -builds do more work during a refresh than before — measurable, and worth capturing on -the bench alongside the two nRF baselines still outstanding. - -## Option B — true non-blocking state machine - -Poll BUSY from `loop()` and split all six `waitforrefresh` call sites into before/after -halves: - -| Site | Path | -|---|---| -| [`:543`](../src/display_service.cpp) | boot refresh, bb_epaper | -| [`:1592`](../src/display_service.cpp) | boot refresh, FastEPD | -| [`:2389`](../src/display_service.cpp) | transfer end, FastEPD | -| [`:2398`](../src/display_service.cpp) | transfer end, bb_epaper | -| [`:3210`](../src/display_service.cpp) | partial refresh, skip-reinit panels | -| [`:3213`](../src/display_service.cpp) | partial refresh, normal path | - -**The blocking is currently doing double duty as mutual exclusion.** Remove it and a -`DIRECT_WRITE_START` arriving mid-refresh reaches `epdSessionAcquire()` and stomps the -panel. Option B therefore also requires explicit gating for every display-touching -opcode — a new rejection or deferral policy, and a wire-visible one if it NACKs. - -What it buys over Option A is command dispatch *during* a refresh. On a single-link -peripheral already mid-transfer that is of limited value and arguably undesirable. - -**Not recommended** unless a concrete requirement appears for servicing commands while -the panel draws. - -## Sequencing - -This rewrites the body of `waitforrefresh()`, which `debug/freeze-fix-phase2` -(`5f3e74c`) already replaced with `waitForPanelIdle()` — a `millis()` deadline plus a -per-driver busy predicate. **Land that branch first, or fold both into one change.** -Doing them independently guarantees a conflict in the same loop body. - -Landing phase 2 first is preferable: `waitForPanelIdle()` is already the single wait all -drivers poll through, so Option A becomes a one-line substitution inside it rather than -a change repeated per driver. - -One interaction to note: on this branch `fastepd_wait_refresh()` -([`src/display_fastepd.cpp:228`](../src/display_fastepd.cpp)) is a stub that returns -immediately, so FastEPD panels have no wait to make cooperative. Phase 2 replaces it -with a real LUTAFSR poll. Until that lands, Option A affects bb_epaper panels only. - -## Acceptance criteria - -Bench-only; CI builds but executes nothing. - -1. During a full refresh on a Spectra 6-colour panel, a BLE disconnect is logged as - `Disconnect reason: N` within ~100 ms of the `CLIENT DISCONNECTED` banner — not after - the refresh completes. -2. Queued responses continue to drain mid-refresh: `[BLE][Q:n] ETX ...` lines appear - during the dot cadence, and `Q` does not climb monotonically across the refresh. -3. A button press mid-refresh registers within one poll interval. -4. Disconnect + reconnect + first command, entirely inside a refresh: the command - dispatches, and `Dropped N queued command(s)` reports only the departed client's - frames. -5. No command dispatches mid-refresh — RX depth may rise, and the drain happens after - `epdRefreshInProgress` clears. A dispatch banner between `EPD refresh:` and - `Refresh took` means the reentrancy invariant was broken. -6. Refresh wall-clock is unchanged within noise versus the blocking build. -7. Battery idle current and pipe-write throughput captured against the same reference - build as the other outstanding nRF baselines. diff --git a/docs/FINDINGS_C6_NIMBLE_IDF555_MEMPOOL_ABI_2026-07-25.md b/docs/FINDINGS_C6_NIMBLE_IDF555_MEMPOOL_ABI_2026-07-25.md deleted file mode 100644 index af2a99a..0000000 --- a/docs/FINDINGS_C6_NIMBLE_IDF555_MEMPOOL_ABI_2026-07-25.md +++ /dev/null @@ -1,110 +0,0 @@ -# Do not move past IDF 5.5.4 until NimBLE-Arduino catches up (ESP32-C6) - -**Date:** 2026-07-25 -**Applies to:** `esp32-c6-N4` (and any future C6/H2/C2 target) -**Short version:** pin pioarduino **55.03.39** (Arduino 3.3.9 / IDF 5.5.4) or older. -**55.03.311** (Arduino 3.3.11 / IDF 5.5.5) does not link on C6 with -NimBLE-Arduino 2.5.0. Do not bump the pin until NimBLE-Arduino ships a release -built against IDF 5.5.5. - -## The break - -IDF 5.5.5 renamed the NimBLE OS-porting mempool exports for the C6 BLE -controller. They **lost their `r_` prefix and moved archives**: - -| Symbol NimBLE-Arduino 2.5.0 calls | IDF 5.5.4 (`55.03.39`) | IDF 5.5.5 (`55.03.311`) | -|---|---|---| -| `r_os_mempool_init` | `libble_app.a(os_mempool.c.o)` | **not defined anywhere** | -| `r_os_memblock_get` | `libble_app.a(os_mempool.c.o)` | **not defined anywhere** | -| `r_os_memblock_put` | `libble_app.a(os_mempool.c.o)` | **not defined anywhere** | - -Under 5.5.5 the same functions exist as `os_mempool_init` / `os_memblock_get` / -`os_memblock_put` in `libbt.a`. Neighbouring families (`r_os_mbuf_*`, -`r_os_cputime_*`) kept their prefix, so this is a targeted rename, not a -blanket one. - -NimBLE-Arduino 2.5.0 (2026-04-02, the latest release as of this writing) still -emits calls to the `r_`-prefixed names, so the C6 link fails with a wall of: - -``` -ble_att_svr.c:3287: undefined reference to `r_os_mempool_init' -ble_gap.c:9045: undefined reference to `r_os_mempool_init' -ble_hs.c:784: undefined reference to `r_os_mempool_init' -... (ble_gatts, ble_gattc, ble_hs_conn, ble_l2cap, ble_l2cap_sig, ble_sm) -collect2: error: ld returned 1 exit status -``` - -There is no library-side fix available: 2.5.0 *is* current. This has to be -resolved upstream in NimBLE-Arduino. - -## Why only C6 - -C6 is the only target in this repo whose BLE controller ships as a -**precompiled blob** (`libble_app.a`) carrying its own copy of the NimBLE -OS-porting layer. Its sdkconfig therefore sets -`CONFIG_BT_LE_CONTROLLER_NPL_OS_PORTING_SUPPORT=y`, which tells the NimBLE host -*not* to compile mempool/mbuf/cputime itself and to call the blob's exports -instead. Measured across `framework-arduinoespressif32-libs`: - -| Chip | `..._NPL_OS_PORTING_SUPPORT` | `libble_app.a` | -|---|---|---| -| esp32 | 0 | absent | -| esp32s3 | 0 | absent | -| esp32c3 | 0 | absent | -| **esp32c6** | **1** | **present** | - -ESP32/S3/C3 compile the porting layer from source, reference plain -`os_mempool_init`, and are unaffected by the rename. All ten non-C6 -environments build clean on `55.03.311`. - -## What was verified (2026-07-25) - -All eleven environments, built locally: - -| Platform | C6 without the script | C6 with the script | Other 10 envs | -|---|---|---|---| -| 55.03.32 (Arduino 3.3.2 / IDF 5.5.1) | links | links (no-op, +28 B) | OK | -| **55.03.39 (3.3.9 / IDF 5.5.4)** — current pin | links | links (no-op, +148 B) | OK | -| 55.03.311 (3.3.11 / IDF 5.5.5) | **link fails** | **link fails** | OK | - -A linker-alias workaround does link: - -```ini --Wl,--defsym,r_os_mempool_init=os_mempool_init --Wl,--defsym,r_os_memblock_get=os_memblock_get --Wl,--defsym,r_os_memblock_put=os_memblock_put -``` - -It is **not applied**, and should not be without on-device validation: it -aliases BLE memory-pool allocation to symbols assumed ABI-identical from their -names alone. A signature mismatch corrupts the BLE heap at runtime rather than -failing at link time. - -## `scripts/esp32c6_nimble_mempool_link.py` - -The script extracts `os_mempool.c.o` from `libble_app.a` and links it as a -plain object, because CI's `--as-needed` plus single-pass archive scan was -reported to drop the member. On both `.32` and `.39` the ordinary link resolves -`r_os_mempool_init` unaided, so locally the script is belt-and-braces; it is -kept for CI, which is where the ordering problem was observed. - -Its failure paths were changed on 2026-07-25 from `sys.exit(1)` to -**warn-and-skip**. Under `.311` the hard exit aborted with -`no entry os_mempool.c.o in archive`, which hid the actual cause (the rename) -behind a missing-member error. Skipping lets the link proceed and report the -genuine `undefined reference to r_os_mempool_init`, which names the real -problem. - -## Before bumping the pin - -1. Confirm NimBLE-Arduino has a release built against IDF 5.5.5 — i.e. one - that calls the unprefixed `os_mempool_*` names on C6. -2. Build `esp32-c6-N4` both with and without the script; both should link. -3. Flash a real C6 and confirm BLE advertises, connects, and completes an - image transfer. A mempool ABI problem will not show up at build time. - -## Related - -- `platformio.ini` — the pin and a condensed version of this warning. -- Upstream commit `0d95b37` — introduced the version pin (at `55.03.32`) and - the current form of the C6 force-link script. diff --git a/docs/FINDINGS_EP75_PARTIAL_REFRESH_WHITE_BLANK_2026-07-17.md b/docs/FINDINGS_EP75_PARTIAL_REFRESH_WHITE_BLANK_2026-07-17.md deleted file mode 100644 index 86755a0..0000000 --- a/docs/FINDINGS_EP75_PARTIAL_REFRESH_WHITE_BLANK_2026-07-17.md +++ /dev/null @@ -1,136 +0,0 @@ -# Findings — EP75 (7.5" 800×480) partial refresh blanks the surround to white - -**Repo:** `Firmware` (branch `debug/v2.2-audit`) -**Date:** 2026-07-17 -**Panel:** EP75 7.5" 800×480 **mono B/W** — UC8179-class controller (`BBEP_CHIP_UC81xx`). Reproducing SKU is almost certainly `EP75_800x480_GEN2` (config id `0x003B`), **not** plain `EP75_800x480` (`0x0014`) — see [§6](#6-which-ep75-sku-are-you-on). -**Symptom (confirmed on hardware):** sending a PARTIAL RECTANGLE update repaints the entire display **outside** the rectangle to **white**. Only the rectangle should change. -**Status:** Root cause confirmed at register level. **No code changed** — this is an investigation/decision doc. Fix options in [§5](#5-fix-options). -**Method:** Read of `src/display_service.cpp` partial paths + the vendored `bb_epaper` library under `.pio/libdeps/nrf52840custom/bb_epaper/src/` (`bb_ep.inl`, `bb_epaper.h`). Every claim cites `file:line`. - ---- - -## 1. TL;DR - -On this panel, the firmware's "partial refresh" is not a differential, region-limited partial — it is a **whole-panel drive-to-target** refresh: - -- `bbepRefresh(&bbep, REFRESH_PARTIAL)` for the GEN2 EP75 either uses the OTP **full**-refresh LUT (when `pInitPart == NULL`) or a **non-hold** "partial" LUT (GEN2), both of which drive **every** pixel to the value in the NEW/DTM2 bank. -- The DRF (display refresh) is **never region-limited**: bb_epaper programs the UC8179 partial window (`PTL 0x90`) with "refresh whole screen" and issues `PTOU` (partial-out) *before* `DRF`, so the panel scans the whole gate/source range. -- The firmware white-fills **both** RAM banks outside the rectangle (`partial_prepare_panel_ram`), so the NEW/DTM2 bank outside the rect is **white**. - -Whole-panel drive-to-target × white NEW bank outside the rect ⇒ **the entire surround is actively driven white.** That is the symptom. - -The white-fill is only *exposed* here; it is harmless on a panel with a genuine differential-hold LUT (only `EP75_800x480` 0x0014 qualifies). The prior belief that "the generic `bbepRefresh` partial path is a true differential and is unaffected" is **false for this hardware**. - ---- - -## 2. Controller and plane wiring (this part is correct) - -**Controller family.** The EP75 panelDefs are `BBEP_CHIP_UC81xx` (UC8179-class) — `bb_ep.inl:3762-3766, 3780-3781, 3785, 3812`. UC8179 command set (`bb_epaper.h:413-447`): `PSR=0x00`, `PWR=0x01`, `PON=0x04`, **`DTM1=0x10` (old bank)**, `DRF=0x12`, **`DTM2=0x13` (new bank)**, LUT registers `VCOM=0x20 / WW=0x21 / BW=0x22 / WB=0x23 / BB=0x24 / VCOM2=0x25`, `CDI=0x50` (border), `TCON=0x60`, `TRES=0x61`, **`PTL=0x90` (partial window)**, **`PTIN=0x91` (partial-in)**, **`PTOU=0x92` (partial-out)**. - -**Plane→command mapping.** For 1bpp EP75, `bbepStartWrite` / `bbepFill` map `PLANE_1 → DTM1 (0x10, OLD)` and `PLANE_0 → DTM2 (0x13, NEW)` — `bb_ep.inl:4145-4156, 4333-4360`. The firmware streams `PLANE_1` then `PLANE_0` into the rect window (`src/display_service.cpp:2808-2829`). So the two-bank/old-new wiring is right; the defect is entirely in the refresh waveform + window handling. - ---- - -## 3. Root cause (register-level) - -### 3.1 The refresh drives the whole panel to the NEW bank (no differential hold) - -`bbepRefresh(&bbep, REFRESH_PARTIAL)` — `bb_ep.inl:4390-4416`: - -- **`pInitPart == NULL` variants silently degrade to full/fast.** For `EP75_800x480_4GRAY` (0x0015), `_4GRAY_V2` (0x0016), `_4GRAY_GEN2` (0x003C), `EP73_800x480` (0x0022), `EP73_SPECTRA` (0x0023), `EP75R_800x480` (0x0026), `EP75YR` (0x0040), the panelDef `pInitPart` field is `NULL` (`bb_ep.inl:3764-3766, 3780-3781, 3785, 3812`). `bb_ep.inl:4391-4396` then falls back to `pInitFast` / `pInitFull` — the **OTP full-refresh LUT**, which drives every pixel to its NEW (DTM2) value. -- **`EP75_800x480_GEN2` (0x003B) has a `pInitPart`, but it is a drive-to-target LUT, not a hold LUT.** `epd75_init_partial_gen2` — `bb_ep.inl:859-890` — has `WW=0x80`, `BW=0x80`, `WB=0x40`, `BB=0x40`; **every** source/target combination applies drive voltage. It also begins with `EPD_RESET` (`bb_ep.inl:838`) → `bbepWakeUp` (`bb_ep.inl:4216-4217`), a hardware controller reset. - -Contrast — a genuine differential-hold LUT (only plain `EP75_800x480` 0x0014): `epd75_init_sequence_partial` — `bb_ep.inl:702-762`, with `WW=0x00` and `BB=0x00` (unchanged pixels get **no** movement, `bb_ep.inl:720-751`) and `PSR=0x3f` (LUT-from-register). There, unchanged pixels physically hold and the white-fill is harmless. - -### 3.2 The DRF is never region-limited - -Even though UC8179 *can* limit the refresh to a window, bb_epaper defeats it: - -- `bbepSetAddrWindow` emits `PTIN (0x91)` + `PTL (0x90)` but sets `PTL`'s last parameter byte to `1` = **"refresh whole screen"** — `bb_ep.inl:4054`. -- `bbepRefresh` sends `PTOU (0x92, partial-out)` **immediately before** `DRF (0x12)` — `bb_ep.inl:4414-4415` — with the literal source comment "update the entire panel, not just the last memory window." - -So the gate/source scan covers the **whole panel** regardless of the RAM address window. (On SSD16xx this region-limiting is impossible; on UC8179 it *is* possible but is being switched off here.) - -### 3.3 The firmware supplies the white - -`partial_prepare_panel_ram` (`src/display_service.cpp:2848-2872`) white-fills both banks over the full panel at partial START (`bbepFill(&bbep, BBEP_WHITE, PLANE_1)` then `PLANE_0`, `:2866-2867`), skipped only for a full-frame rect. The subsequent stream overwrites only the rect window. So outside the rect, DTM2/NEW = white. - -### 3.4 Composition - -whole-panel scan (3.2) × drive-to-target LUT (3.1) × white NEW bank outside the rect (3.3) ⇒ **every pixel outside the rect is driven to white.** Inside the rect the client's real content lands correctly; outside, the panel is repainted white. Exactly the observed symptom. - ---- - -## 4. Why the two earlier conclusions were wrong - -1. **"The white-fill is correct because OLD==NEW==white ⇒ differential drives nothing."** This assumes a differential-hold waveform is active. On GEN2 EP75 it is not (3.1) — the LUT drives to target — so equal banks do **not** produce a hold; they produce a drive to the (white) target. -2. **"The generic `bbepRefresh(REFRESH_PARTIAL)` path is a true differential and is unaffected (only EP397/EP426 are broken)."** The generic UC81xx path is differential **only** when `pInitPart` is a hold-LUT, which across the entire EP75 family is true for **exactly one SKU** (`EP75_800x480` 0x0014). Every other EP75 variant is broken the way this doc describes. The EP397/EP426 blank documented in `FIRMWARE_NIMBLE_PORT_CODE_REVIEW`-adjacent notes has a *different* root cause (SSD16xx DISP_CTRL1 `0x21` left in OLD-plane-bypass); this EP75 case is a distinct bug in a distinct controller family. - -### Related: the partial gate is too permissive - -`handlePartialWriteStart` only rejects `getBitsPerPixel() != 1` (`src/display_service.cpp:1864-1870`); it does not check whether the panel actually has a differential-hold partial LUT. So GEN2 mono (1bpp, no hold LUT) slips through and reaches the broken refresh. - ---- - -## 5. Fix options - -UC8179 **can** do a real region-limited partial, so unlike the SSD16xx panels a proper fix exists — bb_epaper's refresh/window helpers just have to be bypassed. - -### Option A — Real region-scan partial (correct, higher effort, needs hardware validation) - -After streaming `DTM1 (old)` + `DTM2 (new)` for the rect, drive a **windowed** refresh with raw commands instead of `bbepRefresh`: - -``` -PTIN (0x91) // partial-in -PTL (0x90) x_start,x_end, y_start,y_end, 0x00 // last byte = 0x00 → scan WINDOW ONLY (not 0x01) -DRF (0x12) // display refresh; then wait BUSY -PTOU (0x92) // partial-out -``` - -Key deltas vs current behavior: -- Do **not** call `bbepRefresh` (it sends `PTOU` before `DRF`, `bb_ep.inl:4414-4415`). -- Do **not** use `bbepSetAddrWindow`'s `PTL` last-byte=1 (`bb_ep.inl:4054`); use `0x00` so only the window is scanned. -- With window-scan, pixels outside the rect receive **no voltage** and physically hold — so this works even with the OTP full LUT, and the `bbepFill` white-fill at `display_service.cpp:2866-2867` becomes unnecessary and should be **removed** (it is only needed to make the full-panel drive "safe," which we are eliminating). - -Caveats: requires per-SKU validation that the panel honors `PT_SCAN=0`; the in-window waveform may still flash (drive-to-target within the rect) unless a differential LUT is also loaded — acceptable, since the reported bug is the *surround*, not in-rect flashing. This touches flashable panel-drive code and must be eyeballed on hardware. - -### Option B — Reject partial for non-differential EP75 variants (safe, minimal) - -Mirror the existing Seeed / `getBitsPerPixel()!=1` guard in `handlePartialWriteStart`: reject partial (e.g. NACK `ERR_PARTIAL_UNSUPPORTED`) for `bbep.type` in `{EP75_800x480_GEN2, EP75_800x480_4GRAY, EP75_800x480_4GRAY_V2, EP75_800x480_4GRAY_GEN2, EP73_800x480, EP73_SPECTRA_800x480, EP75R_800x480, EP75YR_800x480}`, allowing partial only for panels with a differential-hold `pInitPart` (currently just `EP75_800x480` 0x0014). The client then falls back to a full-frame update. Low risk, no glitch, but loses the partial-update speed/flicker benefit. - -### Option C — Sequence: B now, A later - -Land the safe reject/fallback first so the display is correct immediately, then implement and validate the raw UC8179 region-scan (Option A) as a follow-up when hardware test time is available. - ---- - -## 6. Which EP75 SKU are you on? - -Plain `EP75_800x480` (`0x0014`) has a true differential-hold LUT (`bb_ep.inl:702-762`) and is **immune** to this bug. Because the symptom reproduces on your "mono B/W" panel, the configured `panel_ic_type` is almost certainly **`0x003B` (`EP75_800x480_GEN2`)**, whose GEN2 partial LUT drives-to-target (`bb_ep.inl:859-890`). Worth confirming the exact `globalConfig.displays[0].panel_ic_type` on the device (mapping table at `src/display_service.cpp:433-473`) before applying a SKU-scoped fix — if it *were* `0x0014` the bug would point somewhere else entirely. - ---- - -## 7. Does bb_epaper support partial on EP75 at all? - -Effectively **no**, except plain `EP75_800x480` (0x0014). Every other EP75/EP73 variant either has `pInitPart == NULL` (silently full/fast refresh) or a drive-to-target "partial" LUT (GEN2). So region/differential partial is unsupported across the family bar that one SKU; the firmware must either implement raw UC8179 region-scan (Option A) or reject partial for the rest (Option B). - ---- - -## 8. Key references - -Firmware — `src/display_service.cpp`: -- `partial_trigger_refresh` — `:2831-2846` (EP75 falls through to `bbepRefresh` at `:2844`) -- `partial_prepare_panel_ram` (white-fill) — `:2848-2872` (fills at `:2866-2867`) -- `partial_write_stream_bytes` (PLANE_1 then PLANE_0) — `:2808-2829` -- `panel_skips_reinit_on_partial_refresh` predicate (EP397/EP426 only) — `:2609-2626` -- `handlePartialWriteStart` bpp-only gate — `:1864-1870` -- panel id → type map — `:433-473` - -bb_epaper — `.pio/libdeps/nrf52840custom/bb_epaper/src/`: -- `bbepRefresh` (pInitPart fallback + PTOU-before-DRF) — `bb_ep.inl:4390-4416` (fallback `:4391-4396`, `PTOU`/`DRF` `:4414-4415`) -- `bbepSetAddrWindow` (PTL last byte = 1) — `bb_ep.inl:4054` -- `bbepStartWrite` / `bbepFill` plane→command mapping — `bb_ep.inl:4145-4156, 4333-4360` -- EP75 0x0014 differential-hold partial LUT — `bb_ep.inl:702-762` -- EP75 GEN2 (0x003B) drive-to-target partial LUT + `EPD_RESET` — `bb_ep.inl:838, 859-890` -- EP75 panelDefs (`pInitPart == NULL` variants) — `bb_ep.inl:3762-3766, 3780-3781, 3785, 3812` -- UC8179 command constants — `bb_epaper.h:413-447` diff --git a/docs/PLAN_BLE_TRANSPORT_ABSTRACTION_2026-07-27.md b/docs/PLAN_BLE_TRANSPORT_ABSTRACTION_2026-07-27.md deleted file mode 100644 index 18ff196..0000000 --- a/docs/PLAN_BLE_TRANSPORT_ABSTRACTION_2026-07-27.md +++ /dev/null @@ -1,340 +0,0 @@ -# Plan: BLE transport abstraction + nRF copy-and-enqueue callbacks - -**Date:** 2026-07-27 · **Scope:** `Firmware` repo only · **Status:** plan, not implemented - -> **Amendment 2026-07-27 — Phase 0 retired.** The owner confirms nRF has -> sufficient RAM headroom for the ≈11 KB of new `.bss`, so the measurement gate -> no longer blocks the start of work and `BLE_RX_QUEUE_SLOTS` keeps its full -> 33-slot depth. The other two Phase 0 numbers (battery idle current, -> pipe-write throughput) were never gates on Phases 1–2 — they are before/after -> baselines and are now required **before Phase 3 lands**, not before Phase 1 -> starts. See §6. - -Companion to `PLAN_UNIFY_NRF_ESP32_LOOP_BLE_2026-07-27.md`, which established -*why* this direction is the correct one. This document is the *how*. - -## Goal - -1. nRF BLE callbacks do **copy-and-enqueue only** — no command dispatch, no - crypto, no SPI/I2C, no panel work, no `notify()` on the callback task. -2. All application work runs on the main `loop()` task. -3. Stack-specific code lives in its own `.cpp` + `.h` per platform, gated by - platform macro; no `#ifdef TARGET_*` inside application files. -4. A **thin** `BleTransport` class abstracts the small feature set the - application actually uses. -5. Every BLE touch in application code routes through that class. - -### In scope - -`ble_init.{h,cpp}`, `esp32_ble_callbacks.h`, and the BLE call sites in -`main.{h,cpp}`, `communication.cpp`, `display_service.cpp`, `device_control.cpp`, -`wifi_service.cpp`. - -### Explicitly out of scope - -Deep sleep / wake policy (stays ESP32-only, behind the existing guards); -FastEPD; WiFi/LAN transport; filesystem and crypto backends; DFU entry -(`enterDFUMode()` stays in `device_control.cpp` — it is a bootloader handoff, -not a link concern); any protocol or wire change. - ---- - -## 1. The threading contract - -Stated once, enforced everywhere afterwards: - -> **BLE stack callbacks may do exactly two things: copy bytes into the RX ring, -> and set a flag. Everything else happens on the `loop()` task.** - -This already holds on ESP32. The work is making it hold on nRF, and encoding it -so it cannot silently regress. - -**Ordering hazard — read before sequencing the work.** Today nRF's -`connect_callback` / `disconnect_callback` do heavyweight work -(`updatemsdata()` → I2C + ADC + advertising rebuild; -`cleanupDirectWriteState(true)` → SPI + rail cut). That is currently *safe* -only because command dispatch is on the same task, so the two are serialized by -construction. **The moment dispatch moves to `loop()`, those callbacks become a -genuine cross-task race** — precisely findings #1/#2/#3 from -`FIRMWARE_NIMBLE_PORT_CODE_REVIEW_2026-07-17.md`, which is exactly how ESP32 -acquired them during the NimBLE migration. Converting the nRF callbacks to -flag-only is therefore **not a separate follow-up**; it must land in the same -commit as the dispatch move (Phase 3 below), or the refactor reintroduces a -known Critical bug on nRF. - ---- - -## 2. The transport interface - -`src/ble_transport.h` — portable, includes **no** stack headers, safe for any -translation unit: - -```cpp -#ifndef BLE_TRANSPORT_H -#define BLE_TRANSPORT_H -#include - -class BleTransport { -public: - // --- lifecycle (kept as separate calls so each target keeps its own - // init ordering: nRF must start the SoftDevice BEFORE display/SPI and - // advertise after the boot screen; ESP32 inits BLE after the display) --- - bool begin(const char* deviceName); - void startAdvertising(); - void restartAdvertising(); // idempotent; NEVER defers (see note) - void stopAdvertising(); - void end(); // full teardown; no-op on nRF - - // --- state --- - uint8_t connectedCount() const; - bool isConnected() const { return connectedCount() > 0; } - bool notifyReady() const; // connected AND CCCD subscribed - - // --- data out --- - // false = backpressure ("retry next pass"), not failure. Caller must leave - // the entry queued and not advance its tail. - bool notify(const uint8_t* data, uint16_t len); - - // --- advertising payload --- - void setManufacturerData(const uint8_t* msd, uint8_t len); - - // --- link policy (no-op where the stack does not support it) --- - void requestFastLink(); // nRF: 2M PHY + 251-octet DLE - void boostAdvertising(); // nRF: temporary fast adv interval - void tick(); // periodic housekeeping (adv interval restore) - - // --- events: consume-once, polled from loop(). No app callbacks. --- - bool takeConnectedEvent(); - bool takeDisconnectedEvent(); - - // --- identity --- - const char* addressString(); // wifi_service.cpp's advertised-MAC use -}; - -extern BleTransport ble; -#endif -``` - -Design notes that matter: - -- **`restartAdvertising()` never defers.** Today `esp32_restart_ble_advertising()` - re-pends itself when `epdRefreshInProgress` — an application concern living - inside link code. Under this design the *app* owns deferral policy and simply - doesn't call the method yet. Strictly simpler, and it removes - `bleRestartAdvertisingPending` from the transport's surface. -- **`notify()` gets one unified contract**: return false, leave queued, retry - next pass. nRF's current inline `delay(5)` × 4 retry loop is deleted — it - blocks, and the ESP32 policy is the proven one. -- **Events are polled, not dispatched.** No virtuals, no app-facing callbacks; - that is what keeps callback context from leaking back into application code. -- **No RX method.** RX is buffering, not link state — see §4. - -### Why one class, two `.cpp`s (not an abstract base) - -Exactly one implementation is live per build, so virtual dispatch would cost a -vtable and indirect calls for zero benefit. Same class name, same header, the -platform selects the translation unit. Zero-overhead, and application code sees -one type. - ---- - -## 3. File layout and platform gating - -| File | Contents | Gate | -|---|---|---| -| `src/ble_transport.h` | the class above; no stack headers | none — portable | -| `src/ble_transport_nrf.h` | Bluefruit objects, `connect_callback`/`disconnect_callback` decls | whole file in `#ifdef TARGET_NRF` | -| `src/ble_transport_nrf.cpp` | Bluefruit impl of every method | whole file in `#ifdef TARGET_NRF` | -| `src/ble_transport_esp32.h` | NimBLE aliases, `MyBLEServerCallbacks`, `MyBLECharacteristicCallbacks` | whole file in `#ifdef TARGET_ESP32` | -| `src/ble_transport_esp32.cpp` | NimBLE impl of every method | whole file in `#ifdef TARGET_ESP32` | -| `src/ble_rx_queue.{h,cpp}` | shared RX ring (§4) | none — portable | - -Each platform `.h` is included **only** from its own `.cpp`, inside that file's -gate. Application files include `ble_transport.h` and nothing else. - -**Gating mechanism:** wrap each platform file's entire body in its `#ifdef`, so -the wrong-target build produces an empty translation unit. This needs **no -`build_src_filter` changes across the 11 CI environments** and matches the -convention already used in `esp32_ble_callbacks.h`. If genuinely-not-compiled is -preferred later, `build_src_filter` is the stricter alternative — but that means -editing every ESP32 env (most currently set no filter), so it is deliberately -not the default here. - -**Deletions this enables:** `ble_init.h`'s `using BLEDevice = NimBLEDevice;` -alias block currently leaks NimBLE types into six translation units -(`main.h`, `communication.cpp`, `display_service.cpp`, `device_control.cpp`, -`wifi_service.cpp`, `esp32_ble_callbacks.h`). It moves into -`ble_transport_esp32.h` and stops leaking. `ble_init.{h,cpp}` and -`esp32_ble_callbacks.h` are removed once empty. - -Globals `pServer` / `pService` / `pTxCharacteristic` / `pRxCharacteristic` / -`advertisementData` / `imageService` / `imageCharacteristic` / `bledfu` become -**file-static** inside their platform `.cpp`, and leave `main.h` entirely. -(`main.h` is included only by `main.cpp`, so it is a single-inclusion globals -header — moving definitions out is safe and a strict improvement.) - ---- - -## 4. Shared RX and TX queues - -Both rings move out of ESP32-only guards into portable code. - -**RX** — `src/ble_rx_queue.{h,cpp}`, lifted from `esp32_ble_callbacks.h` / -`main.h`'s `#ifdef TARGET_ESP32` block, keeping the SPSC acquire/release -atomics unchanged: - -```cpp -bool bleRxQueuePush(const uint8_t* data, uint16_t len); // callback task -bool bleRxQueuePop(uint8_t* out, uint16_t* outLen); // loop task -uint8_t bleRxQueueDepth(); // pollActivity() -``` - -Sizing stays `COMMAND_QUEUE_SIZE 33` (`W=32` pipe window + END) × -`MAX_COMMAND_SIZE 256` (`OD_BLE_MAX_FRAME`) ≈ **8.4 KB**, now on both targets. -Add a per-env override knob mirroring the existing `PIPE_SMALL_DRAM_WINDOW` -precedent, in case nRF cannot afford the full depth: - -```c -#ifndef BLE_RX_QUEUE_SLOTS -#define BLE_RX_QUEUE_SLOTS 33 -#endif -``` - -Shrinking it below `PIPE_MAX_W + 1` caps the pipe window and costs throughput — -a deliberate trade, never a link-time discovery. - -**TX** — move `ResponseQueueItem` / `RESPONSE_QUEUE_SIZE` / `MAX_RESPONSE_SIZE` -out of `structs.h`'s `#ifdef TARGET_ESP32` (10 × 256 ≈ **2.6 KB**). -`flushResponseQueueToBle()` becomes portable `bleServiceTx()`. - -**New `.bss` on nRF: ≈11 KB**, atop the ≈8.3 KB pipe reorder queue it already -carries. This was the plan's primary risk; it is **retired** — nRF headroom is -confirmed sufficient, so the full 33-slot depth stands and the -`BLE_RX_QUEUE_SLOTS` knob is kept only as a future escape hatch, not as an -expected fallback. See §7. - ---- - -## 5. Call-site migration - -| Today | Becomes | -|---|---| -| `pServer->getConnectedCount()` (main.cpp ×4, communication.cpp) | `ble.connectedCount()` | -| `esp32_ble_notify_enabled()` | `ble.notifyReady()` | -| `pTxCharacteristic->notify(d,l)` | `ble.notify(d,l)` | -| `imageCharacteristic.notify()` + 4× retry loop | `ble.notify(d,l)`, retry next pass | -| `Bluefruit.connected() && imageCharacteristic.notifyEnabled()` | `ble.notifyReady()` | -| `ble_init()` / `ble_nrf_stack_init()` | `ble.begin(name)` | -| `ble_nrf_advertising_start()` | `ble.startAdvertising()` | -| `esp32_restart_ble_advertising()` | `ble.restartAdvertising()` (app gates on `epdRefreshInProgress`) | -| `BLEDevice::deinit(true)` + `esp32_ble_clear_handles()` | `ble.end()` | -| `updatemsdata()`'s two advertising blocks | `ble.setManufacturerData(msd_payload, 16)` | -| `ble_nrf_boost_advertising()` | `ble.boostAdvertising()` | -| `ble_nrf_advertising_tick()` | `ble.tick()` | -| `ble_nrf_request_fast_link()` / `_arm_link_diag()` / `_log_link_params()` | `ble.requestFastLink()` (diagnostics become impl-private) | -| `NimBLEDevice::getAddress().toString()` (wifi_service.cpp:396) | `ble.addressString()` | -| `bleRestartAdvertisingPending` | removed — app-side deferral | -| `msdUpdatePending` / `bleDisconnectCleanupPending` | `ble.takeConnectedEvent()` / `takeDisconnectedEvent()` | - -`updatemsdata()` splits cleanly: payload computation (I2C/ADC/pack — loop task -only) stays in `display_service.cpp`; the advertising push becomes one -transport call. - ---- - -## 6. Phasing - -Each phase must build all 11 CI environments green and be independently -revertable. - -- ~~**Phase 0 — measurement gate (no code).**~~ **Retired 2026-07-27** — nRF RAM - headroom confirmed sufficient by the owner. Work starts at Phase 1 with the - full 33-slot `BLE_RX_QUEUE_SLOTS`. -- **Phase 1 — introduce the abstraction, no behaviour change.** Create the six - files; move existing per-target code behind the class verbatim; migrate all - ~50 call sites. Threading untouched — nRF still dispatches in its callback. - Delete `ble_init.*` and `esp32_ble_callbacks.h`. *This phase alone delivers - requirements 3, 4 and 5.* -- **Phase 2 — portable queues.** Move RX/TX rings out of the ESP32 guards - (§4). ESP32 behaviour identical; nRF still bypasses them. -- **Phase 3 — nRF copy-and-enqueue (the core change).** *Prerequisite (was - Phase 0): capture the two nRF **baselines** first — battery idle current, and - a pipe-write throughput run per `docs/pipe-write-protocol.md`. Neither gates - the work; both are the "before" half of a before/after pair, and Phase 3 is - the commit that can move them.* nRF write callback → - `bleRxQueuePush()`. `loop()` drains and dispatches. `idleDelay()` gains a - queue-service call. **In the same commit:** nRF `connect_callback` / - `disconnect_callback` become flag-only (see §1 ordering hazard). - `handleReadConfig()`'s `#else delay(50)` becomes the shared TX flush. -- **Phase 4 — shared `loop()` skeleton.** Common body (drain → service events → - timeouts → input polling) with a `platformIdle(bool workInFlight)` hook: - deep-sleep policy on ESP32, `idleDelay` + `ble.tick()` on nRF. -- **Phase 5 — cleanup.** Update the `pwrmgmLock` comment (now uncontended, kept - as defence in depth); update `structs.h:64-66` MTU commentary; refresh - `AUDIT`/`CODE_REVIEW` docs — Phase 3 closes review finding **#20** - (`loadGlobalConfig()` rebuilding `globalConfig` on the callback task while - `loop()` reads it), which should be marked resolved. - -Phases 1–2 are safe to land without Phase 3. **Phase 3 must not be split.** - -### Implementation record (2026-07-27) - -All five phases are implemented on `feat/unify-nrf-esp-phase3`. Deviations from -the plan as written, each deliberate: - -| Phase | Deviation | -|---|---| -| 1 | The `sendResponse()` `#ifdef` tails were **not** fully removed here. The stack-API half went (both arms call `ble.notify()`), but the queue-vs-inline split is a *threading* difference and only dissolved in Phase 3. The plan overstated what Phase 1 could deliver. | -| 1 | `requestFastLink()` keeps no parameter, but the nRF impl latches the connection handle from its own connect callback rather than being handed one. | -| 2 | Accessors are `peek`/`consume`, not the sketched copying `bleRxQueuePop(out, outLen)`: the consumer owns the slot until it advances the tail, so a pointer is safe and avoids a 256-byte stack buffer plus a memcpy per frame. | -| 2 | Does **not** land the ~11 KB on nRF as implied — `--gc-sections` drops the rings while nothing references them. The `.bss` first appears in Phase 3 (measured: +11 184 B, against the predicted ≈11 KB). | -| 3 | `idleDelay()` drains TX but deliberately does **not** drain RX, contrary to §7's mitigation. Dispatching there would make command handlers reentrant the moment anything calls `idleDelay()` from a handler. It returns early on RX instead, which also caps command latency at one 100 ms check interval rather than the caller's full delay — a stronger mitigation than the one specified. | -| 3 | The app hooks were deleted rather than converted. Routing all teardown through `serviceBleDisconnectCleanup()` was necessary: keeping `bleAppOnDisconnect()` alongside the flag ran the teardown twice, the first time without the `epdRefreshInProgress` guard. That also closed a pre-existing nRF bug — its old disconnect callback ran the teardown with neither the mid-refresh nor the LAN-ownership guard. | -| 3 | `restartsAdvertisingOnDisconnect()` was added so the one genuine capability difference reads as a query instead of a target `#ifdef` at the call site. | -| 4 | nRF **gains** the two session watchdogs (15-minute direct-write timeout, `checkPartialWriteTimeout()`). Both are transport-agnostic and were ESP32-only only because they lived in the ESP32 loop arm. | -| 5 | Audit **L4** does not belong in the "callback-stack class" claim (it is a busy-wait), but it is moot: it was already fixed before this work. See the correction in `PLAN_UNIFY_NRF_ESP32_LOOP_BLE_2026-07-27.md` §5. | - -Still outstanding: the entire §7 bench matrix, and the two nRF baselines, which -were never captured — so the idle-current and throughput regressions Phase 3 -could cause remain unmeasured. The three §8 open questions are also unanswered: -`Bluefruit.connected()`'s exact return semantics, a NimBLE `requestFastLink()`, -and whether the `SoftwareTimer` link-diagnostic one-shot should fold into -`tick()`. - ---- - -## 7. Risks and gates - -| Risk | Severity | Mitigation | -|---|---|---| -| ~~nRF `.bss` +11 KB doesn't fit alongside SoftDevice S140 @ `BANDWIDTH_MAX`~~ | ~~High~~ **Retired** | Headroom confirmed sufficient (2026-07-27); `BLE_RX_QUEUE_SLOTS` knob kept as an escape hatch only | -| Command stalls up to 100 ms inside `idleDelay()` — nRF cannot stall today | **High** | `idleDelay()` must drain RX/TX every iteration, not just poll input | -| nRF idle current rises if `loop()` spins to stay responsive | Medium | Baseline before Phase 3; spin only while `workInFlight`, as ESP32 does | -| Pipe-write throughput regression on nRF (ACK now costs a loop pass) | Medium | Benchmark before/after per `docs/pipe-write-protocol.md` | -| Callback→flag conversion missed somewhere on nRF | **High** | Same-commit rule (§1); grep `ble_transport_nrf.cpp` for any call outside push/flag | -| Init-ordering regression (SoftDevice before SPI on nRF) | Medium | `begin()`/`startAdvertising()` deliberately kept separate; `setup()` ordering unchanged | - -Verification is **hardware bench only** — CI builds all 11 environments but -executes nothing. - -**Bench matrix, both targets:** pipe-write full image (throughput + retry -count); multi-chunk config read-back > 864 B; disconnect mid-transfer; -reconnect and re-subscribe; button + touch during an active transfer; buzzer -command during transfer; ESP32 deep-sleep/wake cycle; nRF battery idle current. - -**Rollback:** phases are independent commits; Phase 3 is the only one that -changes runtime behaviour on nRF and can be reverted alone, leaving the -abstraction (Phases 1–2) in place. - ---- - -## 8. Open questions - -1. `connectedCount()` on nRF — confirm whether `Bluefruit.connected()` returns a - connection count or a bool in the pinned core version, and adapt. -2. Should `requestFastLink()` gain a NimBLE implementation (2M PHY + DLE)? ESP32 - has no link tuning today; the abstraction makes adding it a one-file change. - Recommend yes, but as separate work after Phase 1 so it is measured on its own. -3. nRF's `SoftwareTimer` link-diagnostic one-shot fires on the FreeRTOS timer - task. It only logs, so it is safe, but it is a third context — either keep it - impl-private and documented, or fold it into `tick()`. diff --git a/docs/PLAN_EPD_KEEPALIVE_CONFIG_2026-07-13.md b/docs/PLAN_EPD_KEEPALIVE_CONFIG_2026-07-13.md deleted file mode 100644 index 0908347..0000000 --- a/docs/PLAN_EPD_KEEPALIVE_CONFIG_2026-07-13.md +++ /dev/null @@ -1,204 +0,0 @@ -# Plan: Configurable EPD Keep-Alive via PowerOption.screen_timeout_seconds - -> Plan of record, 2026-07-13, branch `feat/less-latency`. Companion implementation -> summary: `IMPLEMENTATION_EPD_KEEPALIVE_CONFIG_2026-07-13.md`. - -## Context - -The `feat/less-latency` branch introduced an EPD panel power session state machine -(`PWR_OFF` / `PWR_WARM` / `PWR_ACTIVE`) with a keep-alive window: after a successful -refresh the panel rail + controller stay powered for a hardcoded `EPD_KEEPALIVE_MS` -(30 s), so a follow-up push skips the ~900 ms cold bring-up. This feature is **new on -this branch — `main` does not have it**; on `main` every refresh powers the panel -straight off. - -Goal: replace the hardcoded 30 s with a per-device config value: - -- New field **`uint8_t screen_timeout_seconds`** in `PowerOption` (0x04 config packet) - — seconds the panel stays powered (PWR_WARM) after a refresh before shutdown. -- **Hard maximum 30 s**: effective window = `min(30, configured value)`. -- **0 → panel shuts down immediately** after refresh (this is also the default: old - persisted config blobs and factory defaults have zeros in the reserved bytes, so - existing devices match `main`'s shipped immediate-off behavior). -- This supersedes the earlier sleep_flags-bit-1 + sleep_timeout_ms idea: no flag bit is - needed — 0 vs non-zero encodes disable/enable in one field, and the window is tunable - independently of `sleep_timeout_ms` (which stays triple-duty: nRF advertising - timeout, nRF loop interval, ESP32 idle-hold). -- The AXP2101 "force window to 0" sensor-scan safety override is **kept**: on AXP2101 - PMIC boards (warm idle draw unmeasured) the window is forced to 0 regardless of the - configured value, and the override announces itself via `writeSerial` whenever it - actually suppresses a non-zero configured value. - -## Validation of existing keep-alive logic (pre-work, done) - -The nRF-path keep-alive logic is **sound**: -- Timer is a wrap-safe `millis()` deadline poll (`(int32_t)(millis() - deadline) >= 0`, - `display_service.cpp:299`), ticked from `loop()` top (`main.cpp:263`) and inside - `idleDelay()`'s wait loop (`main.cpp:433`) — so nRF, which idles in - `idleDelay(sleep_timeout_ms)`, expires on time. -- The nRF cross-task race (Bluefruit write-callback task runs Acquire/Release; loop - task runs the tick) is guarded by `pwrmgmLock`; the tick try-locks and skips if held, - and the take-spin yields via `delay(1)` (avoids the f8d683e priority-inversion - livelock). -- Config-read race during reload is safe-direction: a Release racing - `loadGlobalConfig()`'s memset reads `screen_timeout_seconds == 0` → immediate off. - Single-byte read; no tearing. Same pattern as existing `globalConfig` reads in - Acquire/Release. -- Minor pre-existing nit fixed by this change: `display_service.h:16` claims a - "hard cap ~60 s" that was never enforced — the min(30 s, value) clamp makes the cap - real (at 30 s). - -## Analysis: handle `screen_timeout_seconds == 0` explicitly or via loop()/timers? - -**Explicitly — and the explicit path already exists.** `epdSessionRelease()` -(`display_service.cpp:276`) already branches `if (window == 0 || !refreshSuccess) → -epdSessionForceOffLocked()`: synchronous power-down under `pwrmgmLock`, controller -properly slept (`bbepSleep` + 50 ms settle) before the rail cut. Value 0 needs **zero -new shutdown code** — `epdKeepAliveWindowMs()` returning 0 routes into it. - -The timer alternative (arm `pwrmgmOffDeadlineMs = millis() + 0`, let `epdSessionTick()` -collect it) is strictly worse: -- Power-off latency becomes tick-cadence-dependent. Usually one loop()/idleDelay pass - (ms), but unbounded in principle: on nRF the higher-priority Bluefruit callback task - can keep the loop task off-CPU through back-to-back commands, and the tick's - try-lock deliberately skips passes while a transfer holds the lock. -- The panel would transit PWR_WARM with a dead deadline — state churn, a misleading - "panel warm-idle, off in 0 ms" log, and `epdSessionIsWarm()` briefly true for no - benefit. -- The `!refreshSuccess` path already proves the synchronous branch is the intended - "no keep-alive" route. - -## Changes - -### 1. `src/structs.h` — carve the field from reserved (line 61) -```c - uint16_t min_wake_time_seconds; // Min awake window after first boot or button wake; 0 = default 120 s - uint8_t screen_timeout_seconds; // EPD keep-alive: seconds panel stays powered (WARM) after a - // refresh before shutdown. Clamped to 30 max; 0 = power off - // immediately after refresh (default; matches pre-session behavior) - uint8_t reserved[4]; -``` -Same total struct size (packed layout preserved) — the fixed-size 0x04 `memcpy` in -`config_parser.cpp:313-316` needs no change; old blobs yield 0. Same carve-out pattern -as `min_wake_time_seconds`. - -### 2. `src/display_service.h:16` — cap constant replaces the default -```c -#define EPD_KEEPALIVE_MAX_S 30 // hard cap on power_option.screen_timeout_seconds (clamped, not rejected) -``` -Remove `EPD_KEEPALIVE_MS` (no longer any default window — 0 means off). Update the -surrounding comment block. - -### 3. `src/display_service.cpp` — `epdKeepAliveWindowMs()` (~lines 182-189) -Keep the AXP2101 safety override (first, before the config lookup) and make it -announce itself when it actually overrides a configured value; then source the window -from config: -```c -// Keep-alive window from config: screen_timeout_seconds, clamped to EPD_KEEPALIVE_MAX_S; -// 0 (also the old-blob/factory default) -> Release powers the panel straight down. -// Forced to 0 on AXP2101 boards regardless of config (PMIC warm idle draw unmeasured) — -// announced on the log whenever the override suppresses a non-zero configured value. -static uint32_t epdKeepAliveWindowMs(void) { - uint8_t s = globalConfig.power_option.screen_timeout_seconds; - for (uint8_t i = 0; i < globalConfig.sensor_count; i++) { - if (globalConfig.sensors[i].sensor_type == SENSOR_TYPE_AXP2101) { - if (s != 0) { - writeSerial("[EPD session] AXP2101 present - keep-alive forced off (screen_timeout_seconds ignored)", true); - } - return 0; - } - } - if (s > EPD_KEEPALIVE_MAX_S) s = EPD_KEEPALIVE_MAX_S; - return (uint32_t)s * 1000; -} -``` -The `s != 0` guard keeps the log quiet in the common case (AXP2101 board with the -field left at its 0 default — no override is actually happening). With a non-zero -config it logs once per Release (i.e. once per image push), which is informative -without being spammy. (`structs.h` already visible in this TU; no include changes.) - -### 4. `src/communication.cpp` — `reloadConfigAfterSave()` (~line 26) hardening -After a successful reload, if keep-alive is now disabled and the panel is warm, power -it off so disabling takes effect immediately instead of after the stale deadline -(worst case 30 s): -```c -if (globalConfig.power_option.screen_timeout_seconds == 0 && epdSessionIsWarm()) { - epdSessionForceOff(); -} -``` -`display_service.h` is already included (line 7); `epdSessionForceOff()` / -`epdSessionIsWarm()` are public, idempotent, and safe on the nRF callback task -(disconnect cleanup already calls ForceOff there). Do NOT force off unconditionally — -that would kill the warm panel on every config save. (A *shortened* non-zero value -applies from the next Release; residual ≤ 30 s, not worth re-clamping live.) - -### 5. `src/config_parser.cpp` diagnostics (~lines 680-685) -Next to the existing "Sleep Flags" / "Button Wake" prints, add a decoded line, e.g. -`Screen Timeout: s (EPD keep-alive; 0 = off immediately after refresh)` — mirrors -the bit-0 precedent used for log-based verification. - -### 6. Stale "30 s" comments (code) -- `src/main.cpp:263` — "power the panel down 30 s after last release" → config-driven - (`screen_timeout_seconds`). -- `src/main.cpp:488-494` — `enterDeepSleep` comment block ("expires it at 30 s", - "effective keep-alive = min(30 s, idle-hold)") → min(window, idle-hold). -- `src/main.h:192` — pointer comment mentioning `EPD_KEEPALIVE_MS` → rename to - `EPD_KEEPALIVE_MAX_S` / reword. - -### 7. Docs -- **`docs/epd-panel-power-session.md`**: §4 keep-alive timer (lines ~226-260) — window - now sourced from `screen_timeout_seconds` (clamped to 30 s, 0 = off/default); - note that the AXP2101 override is retained and now logs when it suppresses a - configured value. Update the hardcoded "30 s" mentions at lines 52, 238-239, 287-288, 307, 335 - (e.g. "disconnect within the keep-alive window reconnects onto a warm panel"). -- **`docs/architecture-deep-sleep-power-buttons.md`**: add `screen_timeout_seconds` to - the PowerOption/timer documentation (timer table ~line 93): purpose, clamp, default, - and the ESP32-battery note that the effective warm time is min(window, idle-hold) - because `enterDeepSleep` force-offs the panel. -- **`docs/PLAN_EPD_KEEPALIVE_CONFIG_2026-07-13.md`** (this file): plan copy in docs. - -### Out of scope / follow-up -- **Toolbox**: the companion `opendisplay.org` toolbox `config.yaml` must expose the - new `screen_timeout_seconds` byte in the 0x04 power_option packet (same as was done - for `min_wake_time_seconds` / sleep_flags bit 0), or nobody can enable the feature — - since the default is 0/off, the branch's warm-reconnect latency win ships disabled - until the toolbox exposes it. Different repo; flagged for follow-up. - -## Semantics summary - -| screen_timeout_seconds | Effective keep-alive window | -|---|---| -| 0 (default; old blobs/factory) | none — panel powers off immediately after refresh (matches main) | -| 1–30 | value × 1000 ms | -| 31–255 | clamped to 30 000 ms | -| any, on an AXP2101 board | forced to 0 (safety override); logged when a non-zero value is suppressed | - -ESP32 battery note: `enterDeepSleep` always calls `epdSessionForceOff()`, so the -effective window remains min(window, idle-hold); ForceOff is idempotent, so either -timer firing first is safe. nRF has no deep-sleep path — the loop/idleDelay tick is -the sole expiry mechanism there, and it is sound (see validation above). - -## Verification - -1. **Build both targets**: `pio run` for the nRF and ESP32 envs in `platformio.ini` - (at minimum the default env and one `TARGET_ESP32` env) — must compile clean; - `sizeof(struct PowerOption)` unchanged (packed carve-out). -2. **Default / 0**: flash nRF board with existing config (field absent → 0), push an - image; serial log must show `[EPD session] release: keep-alive disabled, powering - off` immediately after refresh (no `panel warm-idle` line); second push shows - `acquire: COLD bring-up`. -3. **Enabled (e.g. 15)**: write config via 0x0041/0x0042 with screen_timeout_seconds = - 15; boot diagnostic prints `Screen Timeout: 15 s`; push an image → log shows - `release: panel warm-idle, off in 15000 ms`; second push within 15 s shows - `acquire: WARM re-acquire` (fast path); idle >15 s shows - `keep-alive expired — powering panel off`. -4. **Clamp**: set 120 → log shows `off in 30000 ms`. -5. **Live disable**: while panel is warm, write a config with the field = 0 → panel - forced off during `reloadConfigAfterSave` (log `[EPD session] force off`). -6. **ESP32 battery sanity**: with a non-zero value on a battery-mode ESP32, confirm - deep sleep still enters on idle-hold and the panel is off before - `esp_deep_sleep_start()`. -7. **AXP2101 override (if hardware available)**: on an AXP2101 board with a non-zero - `screen_timeout_seconds`, push an image → log shows `[EPD session] AXP2101 present - - keep-alive forced off (screen_timeout_seconds ignored)` followed by the immediate - power-off release line. diff --git a/docs/PLAN_NONBLOCKING_LOG_2026-07-29.md b/docs/PLAN_NONBLOCKING_LOG_2026-07-29.md deleted file mode 100644 index ec75784..0000000 --- a/docs/PLAN_NONBLOCKING_LOG_2026-07-29.md +++ /dev/null @@ -1,425 +0,0 @@ -# Plan — make `od_log` non-blocking on nRF via TinyUSB, short-circuited on ESP32 - -**Date:** 2026-07-29 -**Branch:** `fix/loop-hang-3` -**Supersedes:** the reverted `a84e512` / `1fc524b`, and the check-guarded draft of this plan -**Related:** [`FINDINGS_NRF_BLOCKING_CALLS_2026-07-29.md`](FINDINGS_NRF_BLOCKING_CALLS_2026-07-29.md) -§C (this path), §D (starvation), §E4 (stack pressure) - -**Architecture note.** An earlier version of this plan kept writing through Arduino -`Stream::write` and *prevented* the hang with a capacity check. That check was load-bearing for -safety, so it needed a mutex to be correct, static allocation for the mutex, a context-aware -budget, a level-aware budget, a drop backoff, tick deadlines, and short-write rules — 521 lines, -six review rounds, and three defects found in the scaffolding itself. - -This version writes through `tud_cdc_write()` instead. **Blocking becomes impossible by -construction rather than prevented by a correct check.** Everything that remains survives for -*output quality*, so a bug in it costs a mangled log line, not a bricked tag. That reordering of -what is load-bearing is the whole point of the rewrite. - ---- - -## The bug - -`Adafruit_USBD_CDC::write()` spins with no timeout and no iteration cap while DTR is asserted and -the host is not draining: - -```c -// Adafruit_TinyUSB_Arduino/src/arduino/Adafruit_USBD_CDC.cpp:218 -size_t Adafruit_USBD_CDC::write(const uint8_t *buffer, size_t size) { - size_t remain = size; - while (remain && tud_cdc_n_connected(_instance)) { - size_t wrcount = tud_cdc_n_write(_instance, buffer, remain); - remain -= wrcount; buffer += wrcount; - if (remain) { yield(); } - } -``` - -`od_log` writes through it ([`src/od_log.cpp:40`](../src/od_log.cpp)), so a terminal that stays -open but stops reading wedges the logging task. On `loop()` that is fatal: nRF has no watchdog -and every fault handler in the linked image is `b .`, so `epdSessionTick()` stops, the keep-alive -never expires, and the tag goes silent while the link stays up. - -**The wrapper is the only blocking layer.** Everything beneath it already reports backpressure -instead of waiting on it: - -```c -// class/cdc/cdc_device.c:168 -- writes what fits, returns the count. No loop. -uint32_t tud_cdc_n_write(uint8_t itf, void const* buffer, uint32_t bufsize) { - uint16_t ret = tu_fifo_write_n(&_cdcd_itf[itf].tx_ff, buffer, bufsize); - if (tu_fifo_count(&p_cdc->tx_ff) >= BULK_PACKET_SIZE) tud_cdc_n_write_flush(itf); - return ret; -} - -// :182 -- endpoint busy? return 0. Never waits. -uint32_t tud_cdc_n_write_flush(uint8_t itf) { - TU_VERIFY(tud_ready(), 0); - TU_VERIFY(usbd_edpt_claim(rhport, p_cdc->ep_in), 0); - ... -} -``` - -The Arduino wrapper takes a non-blocking primitive that already says "I only took 40 of your 210 -bytes" and retries it forever instead of reporting it. - -**Necessary conditions** — only one host state hangs: - -| Host state | `tud_cdc_n_connected()` | Hangs? | -|---|---|---| -| Unplugged / not enumerated | false (`tud_mounted()`) | No | -| Bus suspended | false (`tud_suspended()`) | No | -| Port closed, DTR low | false | No — and the FIFO is *overwritable*, so writes never fill | -| **Enumerated, not suspended, DTR high, app not reading** | true | **Yes** | - -DTR does two independent things: it keeps the loop's continue condition true, *and* `cdcd_init` -flips the TX FIFO from overwritable to strict on it (`cdc_device.c:394`, -`tu_fifo_set_overwritable(&tx_ff, !dtr)`). Both halves come from the same bit, which is why an -unattended tag never hits this. - -This is **not** the freeze presently under investigation — the audit ranks TWIM I²C spins (§A1) -and SoftDevice flash `portMAX_DELAY` (§A2) above it, with a watchdog as the remedy for both. It -removes one confirmed unbounded wait and makes log gaps self-describing. - ---- - -## Design - -### 1. One backend function — the only place the targets differ - -```c -// All-or-nothing. Returns false without writing anything if the port cannot take -// the whole record. NEVER blocks on nRF: tud_cdc_write() is a bare tu_fifo_write_n. -static bool od_port_write(const uint8_t *b, size_t n) { -#ifdef TARGET_ESP32 - return s_port->write(b, n) == n; // Stream, as today; see §6 -#else - return tud_cdc_write(b, n) == n; // caller has already reserved capacity -#endif -} -``` - -Contained in one function rather than spread through `od_log` as conditionals. nRF loses the -`Stream*` abstraction; that is the price of the guarantee, and it is paid in exactly one place. - -`#include ` already compiles in this project -([`src/utilities/nrf52840_reformat/main.cpp:22`](../src/utilities/nrf52840_reformat/main.cpp)), -and `tud_cdc_write` / `tud_cdc_write_available` / `tud_cdc_write_flush` are instance-0 inlines at -`class/cdc/cdc_device.h:219-236`. - -### 2. Capacity reservation — for line integrity, not for safety - -Check `tud_cdc_write_available() >= total` once, then issue the record's writes. Because the -reservation and the writes happen **under our mutex**, no producer of ours can consume the space -in between, so every write is guaranteed to take its bytes in full. Partial records are therefore -impossible on nRF without an assembly buffer. - -| Case | Writes | -|---|---| -| Untagged | `write(text, len)`, `write("\r\n", 2)` | -| Tagged | `write(text, tagAt)`, `write(tag, tagLen)`, `write(text + tagAt, len - tagAt)`, `write("\r\n", 2)` | - -Note what this check is **not** doing any more: it is not what keeps the firmware alive. If it -were wrong, the consequence is a truncated line, because `tud_cdc_write()` returns short rather -than spinning. - -Byte-identity is exact for any record ≤232 bytes. `len = min(strlen(text), 232)`; the longest -current caller is [`command_queue.cpp:82`](../src/command_queue.cpp) `char line[192]` plus a ≤20 -char header = 211. - -### 3. The mutex — line serialisation only - -TinyUSB's FIFO is already multi-writer safe: `CFG_TUSB_OS = OPT_OS_FREERTOS` -(`arduino/ports/nrf/tusb_config_nrf.h:43`) ⇒ `CFG_FIFO_MUTEX` (`common/tusb_fifo.h:48`), and -`cdcd_init` gives `tx_ff` a write mutex (`cdc_device.c:252`). So a **single** `tud_cdc_write()` -is atomic against other writers. - -Our mutex exists because the *reservation-plus-N-writes span* is not. Without it, a producer can -consume the reserved space between our check and our third write, truncating the record. It is a -`xSemaphoreCreateMutexStatic()` with a file-scope `StaticSemaphore_t` -(`configSUPPORT_STATIC_ALLOCATION`, `FreeRTOSConfig.h:72`) — static purely to avoid an -allocation-failure branch, not because failure is now dangerous. - -**On lock timeout: drop and count.** A dropped line reads better than a mangled one. This is -now a quality choice with no safety consequence, which is why it needs no fail-closed reasoning. - -### 4. The 20 ms bounded wait — delivery quality - -```c -const TickType_t start = xTaskGetTickCount(); // ONCE, before xSemaphoreTake -const TickType_t budget = pdMS_TO_TICKS(od_budget_ms()); -#define OD_EXPIRED() ((TickType_t)(xTaskGetTickCount() - start) >= budget) - -for (;;) { - if (tud_cdc_write_available() >= need) break; // ORDER MATTERS -- see below - if (OD_EXPIRED()) { drop(); return; } - vTaskDelay(1); -} -``` - -Without a wait, a single-shot check fails constantly on a *healthy* host: the FIFO is 256 bytes, -the longest line is ~210, and one image push emits ~300 lines back to back. The wait is what -keeps the frame dumps. - -`start` is stamped **once, before the mutex take**, and the take's timeout is the remaining -budget — not a fresh one — or worst case per line is 2× budget. - -**Ticks, not `millis()`.** `millis()` on nRF is `tick2ms(xTaskGetTickCount())` with -`tick2ms(t) = (uint64_t)t * 1000 / configTICK_RATE_HZ` (`cores/nRF5/rtos.h:65`) at 1024 Hz, so it -wraps at **4,194,303,999**, not `2³²`. The `(int32_t)(a - b)` idiom is unsound on it; the tick -counter does wrap modulo `2³²`, so unsigned tick subtraction is correct by construction. - -**The room check must precede the expiry check**, so a 0 ms budget degenerates to "try once, then -discard" rather than "discard without trying". Reversing them makes off-loop logging drop 100%. - -**`vTaskDelay(1)`, not `delay(1)`.** `delay()` returns *without* `vTaskDelay` when the CDC flush -spans a tick (`cores/nRF5/delay.c:33-48`: `if (flush_tick >= ticks) return;`, and `ms2tick(1)` is -1 tick). Under load that degrades into a busy-spin at priority 2 that never yields to `loop()`; -time slicing is disabled (`FreeRTOSConfig.h:68`). - -### 5. Budget: 0 off-loop, backoff on stall - -```c -static uint32_t od_budget_ms(void) { - // NULL-check first: without it an uncaptured handle makes the inequality true - // for every caller and silently puts everything in try-once-then-drop mode. - if (s_loopTask != NULL && xTaskGetCurrentTaskHandle() != s_loopTask) return 0; - if (s_loopConsecutiveDrops >= 3) return 0; - return 20; -} -``` - -**0 ms off-loop.** Waiting above `loop()` is priority inversion even though it is no longer -dangerous. See the inventory below for exactly which sites this covers. - -**Backoff.** Waiting is a bet that pays off on transient fullness (~1 ms drain) and loses on a -real stall. Without it a stalled port costs 20 ms on *every* line — ~300 lines per push ≈ **6 s of -added `loop()` latency**, enough to starve BLE ACK draining and time out a transfer. Three -consecutive drops presumes a stall and zeroes the budget; any successful write resets it. - -Fed by **loop-context drops only** — it only gates the loop budget, and a global counter is -poisoned across contexts on a healthy host: a `-debug` frame burst drops three off-loop hex lines -(budget 0, FIFO draining ~64 B/ms) and a loop-task ERROR 1 ms later then gets budget 0 when -20 ms would have saved it. Loop-only also makes it single-writer, so it needs no atomics. - -Two further rules: a **lock-timeout** drop does not feed the backoff (it says nothing about the -port), and the **ready-hook** early return neither counts nor resets, so a counter ≥3 can survive -a DTR-low period — self-healing on the first success. - -**Cut from the previous design: the level-aware budget** (DEBUG 5 ms vs 20 ms). It existed to -ration a safety-critical resource. With safety structural, and with 0-off-loop plus the backoff -covering the pathological cases, the extra state is not worth it. - -`s_loopTask` is captured via `od_log_set_loop_task(xTaskGetCurrentTaskHandle())` **immediately -after `od_log_init()`** in `setup()`. Both cores run `setup()` and `loop()` on the same task — -nRF's `loop_task` calls `setup()` then loops (`cores/nRF5/main.cpp:47-73`), and the ESP32 -`loopTask` does the same. - -### 6. ESP32 — unchanged, and it never drops - -`od_port_write()` uses `Stream::write` there; `od_budget_ms()` is irrelevant because -`od_port_wait_ready()` short-circuits to true. Neither ESP32 log port can block on host -backpressure: - -| Port | Envs | Bound | -|---|---|---| -| HWCDC | all except `-extuart` | 100 ms lock + 20 × 100 ms ≈ **2.1 s**, then short write | -| UART1 | the three `-extuart` envs | **baud-bounded**, ~18 ms for a 210 B line at 115200 | - -`HWCDC::write()` caps at `max_consec_timeouts` × `tx_timeout_ms` and separates real unplug from -backpressure. `uartBegin()` hardwires `flow_ctrl = UART_HW_FLOWCTRL_DISABLE` -(`esp32-hal-uart.c:1117`) and `begin()` never enables CTS. - -Two caveats: `UART_MUTEX_LOCK()` is `do {} while (xSemaphoreTake(uart->lock, portMAX_DELAY))` -(`esp32-hal-uart.c:108`) — bounded by the invariant that the logger is UART1's only user, not by -construction. And bounded is not fast; "always ready" means "cannot hang", not "cannot stall". - -**ESP32 never counts a drop**, so `[DROP: x]` can never appear there and byte-identity holds -unconditionally. Short writes are consequently *not* counted there — an unqualified "short writes -count" rule would set `s_dropped`, put the next line on a tagged path that is nRF-only, and break -that guarantee. ESP32 keeps today's behaviour exactly: return ignored, loss silent. - -> **Correction to an earlier claim in this investigation.** The bounded `HWCDC::write()` was -> reported as an untracked local patch CI would not have. Wrong — every file in `cores/esp32/` -> carries the package install mtime, so nothing was hand-edited; the cap ships in pioarduino -> `55.03.39`, which every build pins. - -### 7. Drop counter and `[DROP: x]` - -`__atomic_fetch_add` on every increment — producers increment outside the mutex. Wrap accepted, -not saturated (saturation needs a CAS loop; 2³² drops is 49 days of continuous dropping at -1000/s). `reported = load()` before the writes; `fetch_sub(reported)` after they complete. - -Placement is after the level, before the message, so the timestamp stays in column 1: - -``` -[0416.212|C0] I: === [BLE] PIPE WRITE END COMMAND (0x0082) === -[0416.213|C0] I: [DROP: 214] DW complete: 307 chunks, 96000/96000 bytes -[0416.214|C0] I: EPD refresh: FULL (mode=0, end payload 0x00) -``` - -No extra line; zero drops ⇒ no tag. `tagAt` is the `pos` returned by `_od_log`'s header -`snprintf` ([`od_log.cpp:27-30`](../src/od_log.cpp)); the worst-case header is ~29 chars so it -cannot truncate, and `pos < 0` is handled at `:31`. Max tag is 19 bytes -(`"[DROP: 4294967295] "`), so the worst wire line is 232 + 19 + 2 = **253**, inside the 256-byte -`CFG_TUD_CDC_TX_BUFSIZE`. - -`od_log_raw()` passes `tagAt = -1`. It emits partial lines — `waitforrefresh()` progress dots — -so it respects the wait and counts drops but is never spliced; the count surfaces on the next -`_od_log()` line. - -### 8. Dark-port guard — nRF only - -With DTR low the FIFO is overwritable (`cdc_device.c:394`), so `tud_cdc_write_available()` can -read 0 while a write would in fact succeed by discarding old bytes. All-or-nothing gating would -then drop every line on an unattended tag and hand the first attaching terminal -`[DROP: 4102931]` — a true number that says nothing. - -`od_log_set_ready_hook([]() -> bool { return (bool)Serial; })` returns early **without counting**. -`Adafruit_USBD_CDC::operator bool()` is `tud_cdc_n_connected()` — literally the old write loop's -continue condition. - -Not installed on ESP32: `HWCDC::isCDC_Connected()` documents that its SOF watchdog "is known to -flap even on a healthy link" (`HWCDC.cpp:268-275`), so a hook there would discard good output. - -### 9. `od_log_flush()` - -Routes to `tud_cdc_write_flush()` on nRF (non-blocking by construction) and `s_port->flush()` on -ESP32. Takes the mutex to avoid racing a mid-record emitter; the existing unconditional `delay(5)` -([`od_log.cpp:92`](../src/od_log.cpp)) stays **outside** the lock — 16 boot call sites × 5 ms of -hold is not something to add. - ---- - -## Off-loop logging inventory - -Bluefruit defers callbacks to a dedicated task — `xTaskCreate(adafruit_callback_task, "Callback", -..., TASK_PRIO_NORMAL, ...)` (`cores/nRF5/utility/AdaCallback.c:147`), `TASK_PRIO_NORMAL == 2` -(`cores/nRF5/rtos.h:59`). Every site below runs at priority 2, not on `loop()`: - -| Site | Level | Fires on | -|---|---|---| -| [`command_queue.cpp:49`](../src/command_queue.cpp) "Empty BLE frame received" | WARN | every zero-length write | -| [`command_queue.cpp:53`](../src/command_queue.cpp) "Command too large for queue" | WARN | every oversized write | -| [`command_queue.cpp:63`](../src/command_queue.cpp) "Command queue full" | ERROR | ring full | -| [`command_queue.cpp:84`](../src/command_queue.cpp) the `ERX`/`URX` hex line | DEBUG | every accepted frame not suppressed by `imageWriteLogQuietFrame` | -| [`ble_transport_nrf.cpp:126`](../src/ble_transport_nrf.cpp) "BLE CLIENT CONNECTED" | INFO | per connect | -| [`ble_transport_nrf.cpp:133`](../src/ble_transport_nrf.cpp) "BLE CLIENT DISCONNECTED" | INFO | per disconnect | - -The first four are inside `bleRxQueuePush()`, reached from `onWriteCb` -([`ble_transport_nrf.cpp:155`](../src/ble_transport_nrf.cpp)), which deliberately logs nothing -itself. - -Plus the **FreeRTOS timer task** (also priority 2): `linkDiagCallback` -([`ble_transport_nrf.cpp:101`](../src/ble_transport_nrf.cpp)) → `logLinkParams()` → the -`[LINK negotiated]` line at [`:89`](../src/ble_transport_nrf.cpp). `logLinkParams()` is also -called from `requestFastLink()` on `loop()` ([`main.cpp:449`](../src/main.cpp)), so the task check -must be a **runtime** test, not a compile-time split. - -**The write callback can escalate to priority 3.** `setWriteCallback(onWriteCb)` defaults to -`useAdaCallback = true`, but the dispatch has an inline fallback: - -```c -// Bluefruit52Lib/src/BLECharacteristic.cpp:538 -if ( !(_use_ada_cb.write && ada_callback(...)) ) { - _wr_cb(conn_hdl, this, request->data, request->len); // inline: BLE task, priority 3 -} -``` - -`ada_callback()` fails when its queue is full (`xQueueSend(..., CFG_CALLBACK_TIMEOUT)`, 100 ms, -`AdaCallback.h:42`). So under exactly the flood conditions that matter, those four log lines move -to priority **3**. The `xTaskGetCurrentTaskHandle() != s_loopTask` test catches it identically — -recorded because it makes the worst case "priority 3 above everything", which is the strongest -argument for a 0 ms off-loop budget rather than a small non-zero one. - ---- - -## The single-writer invariant — now integrity, not safety - -Under the previous design this proved the firmware could not hang. It no longer does: a foreign -writer consuming our reservation truncates a line, it cannot make `tud_cdc_write()` spin. Still -worth keeping as an integrity check. - -Verified for shipping envs: no nRF env sets `CFG_DEBUG` ([`platformio.ini:43`](../platformio.ini); -platform default 0 in `nordicnrf52/builder/frameworks/arduino/adafruit.py:250`), so Bluefruit -`LOG_LV*` compiles out; SEGGER RTT is a separate buffer; no `tud_cdc_tx_complete_cb` refill writer -is linked. **But the stdio retarget is linked and dormant, not absent** — `_write()` routes stdout -to `Serial.write()` (`cores/nRF5/main.cpp:121`). - ---- - -## Files - -| File | Change | -|---|---| -| [`src/od_log.cpp`](../src/od_log.cpp) | `od_port_write()` backend; `od_emit()`; capacity reservation; tick deadline; static mutex; budget + backoff; atomic drop counter; `od_log_flush()` routing | -| [`src/od_log.h`](../src/od_log.h) | `od_log_set_ready_hook()`, `od_log_set_loop_task()`, drop/tag contract comment | -| [`src/main.cpp`](../src/main.cpp) | nRF-only ready hook after `od_log_init`; `od_log_set_loop_task()` immediately after it | -| [`src/command_queue.cpp`](../src/command_queue.cpp) | correct the now-false "od_log ends in a blocking serial write (~9 ms …)" comment at `:36-41` — load-bearing rationale for logging on the callback task | -| `tests/serial_stall_test.py`, `tests/README.md` | restore from `13fb679`; match string `"[od_log] dropped"` → `"[DROP:"` (4 sites: `DROP_NOTICE`, docstring, `--expect-drop-notice` help, failure messages) | - -Estimated code change: **~100 lines** in `od_log.cpp`, against the previous design's surface. No -`platformio.ini` change. No `diagnostics.*`. No `display_service.cpp` change. - -**Out of scope:** the heap-stats and 5 s heartbeat from the reverted `a84e512`. The audit's -Stage 1 items 5–6 still want a heartbeat; separate commit. - ---- - -## Accepted limitations - -- **Starvation is reduced, not solved.** Off-loop logging becomes non-blocking, removing the - priority-inversion wait — but a flood still costs formatting time on those tasks. Audit §D stays - open; the watchdog is its remedy. -- **The debug build is not diagnostically equivalent to today under load.** The 256-byte FIFO - drains 64 bytes per completed bulk-IN transaction (`cdc_device.c:168-179`, `:465-467`), so a - sustained `-debug` rate above the IN completion rate drops even with a healthy host, and - off-loop lines drop immediately. `imageWriteLogQuietFrame` - ([`display_service.cpp:1910`](../src/display_service.cpp)) already suppresses the highest-rate - source. The trade is completeness for not wedging. -- **Evidence density collapses where the freeze hunt needs it.** The first lines to drop are the - `[hb]` heartbeat and the ERX arrival lines. "No ERX line" becomes ambiguous between *frame never - arrived* and *logger dropped it*; `[DROP: n]` gives the count, not the identity. Worth an - addendum to the §D bracketing methodology in the findings doc. -- **ESP32 loss is silent** — short writes uncounted there, as today. The drop counter is an - nRF-only instrument. -- **nRF loses the `Stream*` abstraction.** Contained to `od_port_write()`, but `od_log` is no - longer backend-agnostic on that target. -- **Two diagnostic envs still write `Serial` directly**: `OPENDISPLAY_BOOT_DIAG` (unbounded - `while (!Serial)` at [`src/main.cpp:70`](../src/main.cpp)) and - [`src/utilities/nrf52840_reformat/main.cpp`](../src/utilities/nrf52840_reformat/main.cpp) - (`:149`). Both excluded from `default_envs`. -- **"Just enlarge the FIFO" is unavailable.** `CFG_TUD_CDC_TX_BUFSIZE` is a bare `#define` with no - `#ifndef` guard (`arduino/ports/nrf/tusb_config_nrf.h:66`). - ---- - -## Verification - -- `pio run` — all 11 CI envs, plus `nrf52840custom-debug`, `nrf52840-reformat`, - `nrf52840-bootdiag`, `esp32-s3-N16R8-extuart-debug`. -- **Zero-drop byte-identity on a quiet link**, normalising the timestamp prefix (every line starts - `[%04lu.%03lu|C%lu]`, [`od_log.cpp:27`](../src/od_log.cpp), and millis differs every boot): - `sed -E 's/^\[[0-9]+\.[0-9]+\|C[0-9]+\] //' before.log > a; …; diff a b`. - Must hold unconditionally on ESP32. -- **Single-writer / integrity regression test**, anchored so it is usable: - ```bash - grep -rnE '\b(printf|puts)\s*\(|\bSerial\.(write|print|println|printf)\b' src/ \ - --exclude=od_log.cpp --exclude-dir=utilities - ``` - Expect **9 hits, all in `src/main.cpp`, all inside `#ifdef OPENDISPLAY_BOOT_DIAG`** (lines - 79-81, 137, 142, 156, 161, 171, 176 — verified 2026-07-29). The unanchored form gives 121 hits - on a clean tree because `printf` substring-matches `snprintf`. -- **Bench, nRF** (`nrf52840custom-debug`): `./tests/serial_stall_test.py --port /dev/ttyACM0 - --stall 45 --expect-drop-notice`, triggering an image push during the stall. Expect the transfer - and refresh to complete and the first complete line on resume to carry `[DROP: n]` after its - level. **Run it on the parent commit first** — a FAIL there is the single result that validates - the whole diagnosis. -- **Callback-task WARN flood**: drive [`command_queue.cpp:47,52,62`](../src/command_queue.cpp) - with malformed/oversized frames and confirm `loop()` keeps being scheduled. -- **Backoff engages and releases**: `loop()` latency must not grow with the number of attempted - lines during a stall (~3 × budget, not ~300 ×), and the first line after the host resumes must - restore full-budget behaviour rather than staying latched. -- **Stall during a refresh's progress dots**: drops counted but untagged; the count appears on the - next `_od_log()` line, not inside the dot run. -- Scope the timing claim correctly: the **waits** are budgeted. Formatting, preemption and the - writes are outside it. diff --git a/docs/PLAN_TINFL_INFLATE_SWAP_2026-07-23.md b/docs/PLAN_TINFL_INFLATE_SWAP_2026-07-23.md deleted file mode 100644 index 20e0624..0000000 --- a/docs/PLAN_TINFL_INFLATE_SWAP_2026-07-23.md +++ /dev/null @@ -1,153 +0,0 @@ -# Plan: Transparent swap to ROM `tinfl` for the ESP32-WiFi inflate path - -## Context - -On the WiFi/LAN transport, compressed image uploads are **slower** than uncompressed -despite a ~4:1 wire saving. Root cause (traced this session): the transfer is -**consumer-bound on software inflate**, not wire-bound. The active inflater is a -hand-rolled, fully bit-serial, resumable DEFLATE state machine -([lib/uzlib/src/od_zlib_stream.c](../lib/uzlib/src/od_zlib_stream.c), from PR #26): -per-bit reads, per-symbol Huffman walk, one-byte-at-a-time output, and a `% 65521` -Adler-32 reduction on *every* byte. It was designed BLE-first — where wire speed is far -below inflate speed, so decode was never the bottleneck. WiFi is ~10–100× faster, so -inflate becomes the limiter. Crossover: compression only wins when -`inflate_rate > ~1.33 × wire_rate`; the current engine fails that on LAN. - -The three WiFi chips (ESP32-S3 / C3 / C6) carry **miniz `tinfl_decompress` in mask ROM** -(fixed addresses in `.rom.ld`; `esp_rom/include/miniz.h` ships with the framework) -— a word-at-a-time, table-driven inflater. Using it costs **0 bytes of flash**. The S3 -framebuffer is in **PSRAM**, so the design keeps the 32 KB history/output ring in -**internal SRAM** (fast match reads) and flushes decoded bursts **sequentially** to the -PSRAM framebuffer — never decoding directly into PSRAM. - -Intended outcome: on ESP32-WiFi builds, compressed transfers decode several× faster, -flipping compression back to a net win on LAN — with **no protocol/wire change** and no -flash growth. nRF52840 and classic ESP32 (no WiFi) keep uzlib. - -## Hard constraint - -**`lib/uzlib/` must not be modified at all** — no edits to `od_zlib_stream.c`, `uzlib.h`, -or any file under it, and no new files added there. The swap therefore happens **one level -up**, in the firmware's own adapter layer in `src/`. uzlib stays compiled and byte-for-byte -intact; it is simply not *called* on WiFi builds (unused `od_zlib_stream_*` get dropped by -`--gc-sections`). All firmware callers of the inflater live in **one file**, -[src/display_service.cpp](../src/display_service.cpp) (`od_zlib_stream_reset` ×3, and -`push`/`poll`/`error` inside `zlib_stream_to_direct_write` / `zlib_stream_to_partial_write`) — -confirmed the only call sites in `src/`. - -## Approach: src-level tinfl implementation + compile-time remap in display_service.cpp - -Provide a tinfl-backed implementation of the same streaming contract under new names in -`src/`, then bind the existing call sites to it on WiFi builds via a small `#define` remap — -so the ~11 existing call sites in `display_service.cpp` are **not edited**. The swap is -invisible above the adapter layer; the wire protocol, `communication.cpp` dispatch, framing, -and py-opendisplay are untouched. Because the gate is per-build (chip family), on WiFi-capable -ESP32 builds *all* compressed transfers (BLE + LAN) use tinfl — a superset of uzlib's behavior. - -### The gate - -Reuse the exact condition that already defines `OPENDISPLAY_HAS_WIFI` -([wifi_service.h:13](../src/wifi_service.h#L13)): `TARGET_ESP32 && OPENDISPLAY_ENABLE_WIFI`. -Covers all S3/C6/C3 WiFi envs (incl. `esp32-s3-E1004`, which extends `…-N32R8-extuart`) and -excludes `nrf52840custom` and `esp32-N4`. No `platformio.ini` change — `src/` files compile -automatically and the flag already exists. - -## Files to change (2 new in `src/`, 1 edited; zero uzlib/platformio changes) - -1. **`src/od_inflate_tinfl.h`** (new) — declares the 5 functions - (`od_inflate_tinfl_reset/push/poll/error/output_count`) and the `OPENDISPLAY_USE_TINFL` - gate macro. Reuses `od_zlib_status_t` / `OD_ZLIB_STATUS_*` by `#include "uzlib.h"` (include - only — no modification), so the tinfl path returns the identical status type the callers - already switch on. - -2. **`src/od_inflate_tinfl.cpp`** (new) — body wrapped in `#if OPENDISPLAY_USE_TINFL`. - `#include "miniz.h"`. Implements the tinfl wrapper over ROM `tinfl_decompress`. - -3. **[src/display_service.cpp](../src/display_service.cpp)** — add, right after the includes - block (after line 18), ~8 lines: - ```c - #include "od_inflate_tinfl.h" - #if OPENDISPLAY_USE_TINFL - // Route the inflate adapter to the ROM-tinfl engine on ESP32-WiFi builds; uzlib - // (lib/uzlib) is left untouched and unused here. See od_inflate_tinfl.h. - #define od_zlib_stream_reset od_inflate_tinfl_reset - #define od_zlib_stream_push od_inflate_tinfl_push - #define od_zlib_stream_poll od_inflate_tinfl_poll - #define od_zlib_stream_error od_inflate_tinfl_error - #endif - ``` - The macro is placed *after* `#include "uzlib.h"` (line 15) so the existing - `od_zlib_stream_*` call sites (2076, 2245, 2778, 3120–3177) bind to the tinfl impl with **no - edits to those lines**. `od_zlib_status_t` and `OD_ZLIB_STATUS_*` stay as-is (shared type). - Gate off (nRF / classic ESP32): the macros vanish and everything calls uzlib exactly as today. - -### tinfl wrapper design (`od_inflate_tinfl.cpp`; static BSS — chosen) - -File-scope `static` state (plain arrays land in internal `.bss`/DRAM automatically, satisfying -"dict must be internal SRAM" with zero effort; ~43 KB permanent on gated builds): -- `tinfl_decompressor s_decomp;` (~11 KB) -- `uint8_t s_dict[TINFL_LZ_DICT_SIZE];` (32768 — history + output ring) -- ring/delivery cursors `s_dict_ofs`, `s_deliver_ofs`, `s_pending`; input staging - `s_in`, `s_in_remaining`, `s_more_input`; bookkeeping `s_expected`, `s_produced`, - `s_done`, `s_initialized`, `s_error`. - -- **`od_inflate_tinfl_reset(expected)`**: `tinfl_init(&s_decomp)`; zero cursors/counters; - store `s_expected`; clear error/done; `s_initialized=true`. -- **`od_inflate_tinfl_push(input,len,final)`**: mirror uzlib semantics (error if not - initialized / previous input unconsumed); stash input; `s_more_input = !final` → maps the - `final` flag to clearing `TINFL_FLAG_HAS_MORE_INPUT` so tinfl finalizes + verifies Adler-32 - on the last frame (the empty `push(NULL,0,true)` at - [display_service.cpp:2368](../src/display_service.cpp#L2368) resolves to DONE). -- **`od_inflate_tinfl_poll(output,capacity,produced)`** — rate-matching core: - - **Deliver first, decode only when drained.** While `s_pending>0`, `memcpy` a contiguous run - from `s_dict+s_deliver_ofs` into `output` (bounded by `capacity`); advance cursors; return - `OD_ZLIB_STATUS_OUTPUT_READY` when `output` fills. - - When `s_pending==0`, call `tinfl_decompress(&s_decomp, s_in, &in_bytes, s_dict, - s_dict+s_dict_ofs, &out_bytes, TINFL_FLAG_PARSE_ZLIB_HEADER | (s_more_input ? - TINFL_FLAG_HAS_MORE_INPUT : 0))` with `out_bytes` bounded to **contiguous room to ring end** - (`32768 - s_dict_ofs`). One contiguous burst per decode, fully delivered before the next - decode → tinfl never overwrites undelivered bytes and delivery stays a simple contiguous copy. - - Advance `s_in`, add to `s_produced`/`s_pending`, wrap `s_dict_ofs &= 32767`. - - Status map: `<0` → `OD_ZLIB_STATUS_ERROR` (set `s_error`); `TINFL_STATUS_DONE(0)` → mark done, - return DONE when drained; `NEEDS_MORE_INPUT(1)` drained → `OD_ZLIB_STATUS_NEEDS_INPUT`; - `HAS_MORE_OUTPUT(2)` → loop to deliver. -- **`od_inflate_tinfl_error()` / `_output_count()`**: return `s_error` / `s_produced`. - -tinfl API confirmed present in the ROM header: `tinfl_init` macro (miniz.h:587), status enum -`DONE=0 / NEEDS_MORE_INPUT=1 / HAS_MORE_OUTPUT=2 / negatives` (miniz.h:578–583), -`TINFL_LZ_DICT_SIZE 32768`, `TINFL_FLAG_*`, full `tinfl_decompressor` struct. - -### Intended, documented behavior differences (benign / desirable) -- **Window ceiling relaxed.** tinfl always supports up to a 32 KB window, so it accepts any - standard zlib stream regardless of `OPENDISPLAY_ZLIB_WINDOW_BITS`. Today's sender emits - `wbits=9`, which still decodes; this additionally *unlocks* a future `wbits=15` sender change - for a better ratio (py-opendisplay, out of scope). -- **Adler-32 handled by tinfl** via `TINFL_FLAG_PARSE_ZLIB_HEADER` (no per-byte modulo). - -## Out of scope -- No changes to `lib/uzlib/` (hard constraint), `communication.cpp`, the wire protocol, framing, - `platformio.ini`, or py-opendisplay. -- No BLE-only carve-out — the build gate naturally includes BLE on WiFi builds (superset; no regress). - -## Verification - -1. **Include-path smoke test (first).** Confirm `#include "miniz.h"` resolves for a `src/` - `.cpp` on the S3 env (ROM header at `.../esp32s3/include/esp_rom/include/miniz.h`; symbol - guaranteed by `.rom.ld`). If bare include fails, add the `esp_rom/include` dir to the - env `build_flags` `-I`, or include via its resolvable path. -2. **Builds (keep CI green).** - - `pio run -e esp32-s3-N16R8` and `pio run -e esp32-c3-N16` → compile with tinfl remap; - `.bss` grows ~43 KB, flash does **not** grow for inflate code. - - `pio run -e nrf52840custom` → gate off; compiles/links against uzlib exactly as today. -3. **uzlib untouched.** `git diff` shows zero changes under `lib/uzlib/`. Host unit test - `tools/test_zlib_stream.c` (uzlib impl, gate off on host) still builds/passes. -4. **Functional on hardware (S3, PSRAM framebuffer).** Flash `esp32-s3-N16R8`; send a compressed - LAN direct-write image via py-opendisplay. Confirm correct render (Adler-32 passes; - `directWriteBytesWritten == directWriteDecompressedTotal`) and that the - `DW complete … zlib B on wire (x)` log ([display_service.cpp:1884](../src/display_service.cpp#L1884)) - shows the expected ratio. Also exercise the **partial** (0x76) and **pipe** (0x80) compressed - paths, and a plain **BLE** compressed transfer (now also tinfl on this build). -5. **Perf confirmation (no new instrumentation).** Use the existing `DW complete … chunks … - KB/s` log ([display_service.cpp:1873](../src/display_service.cpp#L1873)): send the same - image on a tinfl build vs a uzlib build and confirm the compressed-LAN rate rises and now - beats the uncompressed-LAN rate (`inflate_rate > ~1.33 × wire_rate`). diff --git a/docs/PLAN_UNIFY_NRF_ESP32_LOOP_BLE_2026-07-27.md b/docs/PLAN_UNIFY_NRF_ESP32_LOOP_BLE_2026-07-27.md deleted file mode 100644 index 8d353e0..0000000 --- a/docs/PLAN_UNIFY_NRF_ESP32_LOOP_BLE_2026-07-27.md +++ /dev/null @@ -1,252 +0,0 @@ -# Investigation: unifying the nRF52840 and ESP32 loop / BLE architecture - -**Date:** 2026-07-27 · **Status:** investigation only — no code changed - -> **Amendment 2026-07-27 — the RAM gate is retired.** The owner confirms nRF has -> sufficient headroom for the shared rings, so the "≥15 KB or don't bother" -> condition in §5 no longer blocks Level 2, and the RX ring keeps its full depth -> (no narrowed PIPE window, no throughput trade). Idle current and pipe-write -> throughput remain worth capturing, but as before/after **baselines** taken -> ahead of the execution-model change — not as go/no-go gates. Sequencing and -> file-level detail live in `PLAN_BLE_TRANSPORT_ABSTRACTION_2026-07-27.md`. - -## Question - -What would it take to collapse the two per-target loop/BLE paths into one, so the -firmware stops carrying two ways of doing the same thing? - -## Answer in one paragraph - -The *protocol* layer is already unified and should be left alone. The divergence -that matters is a **threading model difference**, not a code-organisation one: on -ESP32 every command is dispatched from `loop()`; on nRF every command is -dispatched from the Bluefruit callback task and `loop()` is an idler. Everything -else people notice as "two paths" — two `#ifdef` tails in `sendResponse()`, two -advertising blocks in `updatemsdata()`, two halves of `ble_init.cpp` — is a -consequence or a cosmetic sibling of that. There are three separable levels of -work; **Level 1 is cheap and worth doing now, Level 2 is the real unification and -should not start until nRF RAM headroom and idle current are measured on -hardware.** - ---- - -## 1. What is already unified (do not touch) - -| Area | Where | Notes | -|---|---|---| -| Command dispatch | `communication.cpp:625` `imageDataWritten()` | One opcode switch serving nRF BLE, ESP32 BLE and ESP32 LAN. `BLEConnHandle`/`BLECharPtr` typedefs absorb the signature difference. | -| All command handlers | `display_service.cpp`, `communication.cpp`, `device_control.cpp` | Config read/write/chunk, direct write, partial write, PIPE_WRITE, LED, buzzer — no target branches in the logic. | -| Encryption envelope | `communication.cpp:663-711` | AES-CCM gate, replay window, origin-gated decrypt — shared. | -| PIPE_WRITE window/reorder | `display_service.cpp:2483-2900`, `structs.h:32-54` | Sequence, ACK cadence, reorder queue — fully shared. | -| Wire constants | `include/opendisplay_protocol.h` (vendored) | Single source of truth. | -| Transport-origin routing | `g_commandOrigin` / `originTag()` | Already models "N transports, one dispatcher". | - -This is the important part: **a third transport (LAN) was added without forking -the dispatcher.** The abstraction the codebase is missing is not on the command -path — it is on the *delivery* path and the *scheduling* path. - -## 2. What actually diverges - -### A. Execution model — the root cause - -**ESP32.** NimBLE host task → `MyBLECharacteristicCallbacks::onWrite()` -(`esp32_ble_callbacks.h:81`) copies the frame into a 33-slot SPSC ring → -`loop()` (`main.cpp:406-423`) drains it, dispatches, and flushes a 10-slot TX ring -back out via `notify()`. The host task touches nothing but the ring. Heavy or -state-mutating work is deferred to `loop()` behind flags: -`bleDisconnectCleanupPending`, `msdUpdatePending`, `bleRestartAdvertisingPending`. - -**nRF.** Bluefruit's SoftDevice callback task calls `imageDataWritten()` -**directly** (`ble_init.cpp:157` `setWriteCallback`). Dispatch, zlib inflate, EPD -SPI streaming and `notify()` all run on the BLE task. `loop()` -(`main.cpp:518-530`) is a housekeeping idler: `idleDelay(500)` (or -`sleep_timeout_ms`), advertising tick, buttons, touch, buzzer. - -Direct consequences, all visible in the tree today: - -- `pwrmgmLock` (`main.h:185`, `display_service.cpp:401-526`) exists **only** - because of this: it is a genuine cross-task try-lock on nRF (BLE task vs loop - task) and uncontended on ESP32. The audit already records it as deliberate - (`docs/AUDIT_FIRMWARE_2026-07-13.md:274`). -- The flag-and-defer callback pattern exists only on ESP32. -- `handleReadConfig()` needs an `#ifdef` in the middle of a loop - (`communication.cpp:468-476`): ESP32 flushes the TX ring between chunks, nRF - just `delay(50)`. -- Audit finding M1 (4 KB stack buffer in the BLE-callback context) is an - nRF-only class of bug that cannot exist under the ESP32 model. - -### B. Response path - -| | nRF | ESP32 | -|---|---|---| -| Call site | inline from BLE task | enqueue → `flushResponseQueueToBle()` in `loop()` | -| Backpressure | 4 retries × 5 ms on `notify()==false` (`communication.cpp:345-349`) | leave entry queued, retry next pass (`main.cpp:288`) | -| Overflow | none possible (synchronous) | 10-slot ring; mid-drain flushes added to stop pipe ACKs overflowing it | -| Latency | same radio event | ≥1 loop pass | - -### C. BLE stack API - -Bluefruit value-objects (`imageService`, `imageCharacteristic`, `bledfu`, -`Bluefruit.Advertising.*`) vs NimBLE-Arduino pointers, aliased back to the -historical `BLE*` spellings in `ble_init.h:21-30`. Behavioural gaps that are -*not* stack limitations, just work done on one target only: - -- **Link tuning**: nRF explicitly requests 2M PHY + 251-octet DLE and logs - negotiated params (`ble_init.cpp:83-145`). ESP32/NimBLE has no equivalent. -- **Advertising interval boost** on button press: nRF only - (`ble_init.cpp:46-76`, `device_control.cpp:616`). -- **MSD update**: nRF rebuilds and restarts advertising inside `updatemsdata()`; - ESP32 pushes `setAdvertisementData()` and restarts only while disconnected - (`display_service.cpp:1767-1800`). -- **MTU**: nRF fixed at 247 by `configPrphBandwidth(BANDWIDTH_MAX)`; ESP32 - requests `OD_BLE_PREFERRED_ATT_MTU` (256). Documented in `structs.h:64-66`. -- **DFU**: nRF registers `bledfu` when encryption is off; ESP32 has no analogue. - -### D. Lifecycle / power — genuine capability difference - -`main.cpp`'s largest `#ifdef TARGET_ESP32` regions are deep-sleep machinery: -wake-cause detection, `pollActivity()`, min-wake window, advertising-timeout -window, `fullSetupAfterConnection()`, `enterDeepSleep()` with stack teardown. -nRF has none (`getDeepSleepCount()` returns 0). **This is not accidental -divergence and should not be merged into shared code** — only hooked. - -### E. Adjacent, non-BLE platform splits (out of scope for this question) - -Filesystem (`InternalFS` vs `LittleFS`, ~21 branches in `config_parser.cpp`), -crypto backend (CC310 vs mbedTLS, ~16 in `encryption.cpp`), chip ID, chip -temperature, FastEPD, WiFi/LAN, ADC ladder, `IRAM_ATTR` ISRs. - ---- - -## 3. Three levels of work - -### Level 1 — symmetric BLE port layer (recommended now) - -Introduce `src/ble_port.h` with one interface and two implementations -(`ble_port_nrf.cpp`, `ble_port_esp32.cpp`), each compiled whole — no `#ifdef` -*inside* either file: - -```c -void od_ble_init(const char* name); -void od_ble_advertising_start(void); -void od_ble_advertising_restart(void); -void od_ble_set_manufacturer_data(const uint8_t* msd, uint8_t len); -uint8_t od_ble_connected_count(void); -bool od_ble_notify_ready(void); -bool od_ble_notify(const uint8_t* data, uint16_t len); -void od_ble_request_fast_link(void); // no-op where unsupported -void od_ble_deinit(void); // no-op on nRF -``` - -Removes, without touching threading: - -- both `#ifdef` tails in `sendResponse()` / `sendResponseUnencrypted()` - (`communication.cpp:216-238`, `324-355`) → one `od_ble_notify()` call; -- both advertising blocks in `updatemsdata()` → one - `od_ble_set_manufacturer_data()`; -- `pServer` / `advertisementData` externs leaking into `display_service.cpp`, - `communication.cpp`, `main.cpp`; -- the target split inside `ble_init.cpp` (becomes a file split). - -Also the natural place to close the C-gaps: implementing `od_ble_request_fast_link()` -for NimBLE (2M PHY + DLE) gets ESP32 the link tuning nRF already has. - -**Cost:** ~2–3 days including a bench pass on both targets. **Risk:** low — no -scheduling change, and the notify/advertising call sequences move verbatim. -**Note:** NimBLE's "`setAdvertisementData()` must be the last call before -`start()`" constraint (`ble_init.cpp:308-312`) has to survive the port; it is the -one non-obvious ordering rule in the ESP32 implementation. - -### Level 2 — unify the execution model (the actual fix) - -Make nRF adopt the ESP32 model: the Bluefruit write callback enqueues, `loop()` -dispatches. - -1. Move `CommandQueueItem` / `commandQueue` / heads out of - `esp32_ble_callbacks.h` and out of `main.h`'s `#ifdef TARGET_ESP32` block into - a shared `src/ble_rx_queue.{h,cpp}`. Move `ResponseQueueItem` out of - `structs.h:77-91`'s ESP32 guard likewise. -2. nRF write callback becomes a thin `od_ble_rx_push(data, len)` — the same SPSC - acquire/release ring, now shared. -3. `flushResponseQueueToBle()` becomes shared `bleServiceTx()`; nRF's retry - policy folds into the existing "stop on `notify()==false`, retry next pass" - rule (the ESP32 policy is strictly better — it never blocks). -4. Rewrite `loop()` as one shared skeleton — drain commands (bounded, flush TX - between) → service deferred flags → timeouts → input polling — plus a - `platform_idle(bool workInFlight)` hook that is the deep-sleep policy on - ESP32 and `idleDelay` + advertising tick on nRF. `pollActivity()` becomes - shared but only the ESP32 policy consumes it. -5. **`idleDelay()` must also drain both queues.** Today nRF cannot stall a - command inside `idleDelay` because dispatch is on the BLE task. After the - change it can, for up to 100 ms per chunk. This is the single largest - behavioural regression risk in the whole plan. -6. `handleReadConfig()`'s `#else delay(50)` disappears — nRF gains the - flush-between-chunks semantics. -7. `pwrmgmLock` becomes uncontended. Keep it (it is nearly free) rather than - remove it — touch/button paths still run from `loop()` and ISRs. - -**Measurements:** - -- ~~**RAM.**~~ **Resolved 2026-07-27 — headroom confirmed sufficient.** The - shared rings add ≈8.4 KB (`33 × 256`) + ≈2.6 KB (`10 × 256`) of `.bss` on nRF, - on top of the ≈8.3 KB PIPE reorder queue it already carries. That fits, so the - RX ring keeps its full depth. The concern was that shrinking it below `W+1` - would cap the PIPE_WRITE window and cost throughput — the `env:esp32-N4` - `PIPE_SMALL_DRAM_WINDOW` precedent (`structs.h:40-53`). That trade is no longer - on the table for nRF. -- **Idle current** *(baseline, not a gate)*. nRF currently spends idle time inside `delay()`, which yields - to the FreeRTOS idle task and `sd_app_evt_wait`. A loop that spins at - `delay(1)` while a link is up (as ESP32 does under `workInFlight`) changes the - battery profile. Measure before/after on a battery unit. -- **Throughput** *(baseline, not a gate)*. nRF ACKs a PIPE frame within the same radio event today; via - the queue it waits for a loop pass. Re-run the pipe-write benchmark in - `docs/pipe-write-protocol.md` on both targets and compare. - -**Cost:** ~1–2 weeks including bench validation. **Risk:** high — it touches the -two properties the firmware is most sensitive to (transfer throughput and battery -current) on the target with the least headroom, and there are no automated tests; -CI builds all 11 environments but verifies nothing at runtime. - -### Level 3 — full platform HAL (independent) - -`od_fs_*`, `od_crypto_*`, `od_chip_id()`, `od_chip_temperature()`. Mechanical, -orthogonal to this question, removes ~40 more `#ifdef`s. Can be done any time, -before or after Level 2. - ---- - -## 4. What should stay divergent - -- **Deep sleep / wake / power latch** — real capability difference. Hook it - (`platform_idle()`), do not merge it. -- **FastEPD, WiFi/LAN** — ESP32-only subsystems, already cleanly guarded. -- **The stack APIs themselves** — two implementation files with no internal - `#ifdef` is the goal, not one file that branches. - -## 5. Recommendation - -Do **Level 1** now: it removes the visible duplication, is independently -valuable, has low hardware risk, and creates the seam Level 2 needs anyway. - -~~Then take the two nRF measurements … **Level 2 is only worth it if nRF has -≥15 KB of headroom**~~ — **superseded 2026-07-27.** nRF headroom is confirmed -sufficient, so Level 2 is cleared to proceed with a full-depth RX ring. The -feared outcome (smaller ring → narrower PIPE window → throughput regression -traded for architectural tidiness) does not apply. Capture idle current and a -pipe-write run as "before" baselines ahead of the execution-model change, so a -regression is detectable rather than gate-keeping. - -The payoff for Level 2, stated plainly, is: one loop to reason about, one place -to fix a queue/backpressure bug, nRF gains the response-ring flow control that -ESP32 got from real overflow incidents, and a whole class of -"runs-on-the-BLE-callback-stack" bugs (audit M1) becomes structurally -impossible. - -> **Correction 2026-07-27.** This sentence originally cited "audit M1, L4". L4 -> does not belong in the claim: it is a busy-wait, not a callback-context bug, so -> moving dispatch to `loop()` neither fixes it nor is required to. It is moot in -> any case — L4 was already fixed before this work (hardware PWM plus a -> millis()-poll state machine, `src/buzzer_hw.cpp` and `buzzer_control.cpp`), and -> is marked RESOLVED in `docs/AUDIT_FIRMWARE_2026-07-13.md`. Only M1 belongs to -> the class this phase eliminates. That is a real payoff — it is just not free, and the cost lands -entirely on the most constrained target. diff --git a/docs/PLAN_WAKE_ON_BUTTON_2026-07-12.md b/docs/PLAN_WAKE_ON_BUTTON_2026-07-12.md deleted file mode 100644 index 0261e19..0000000 --- a/docs/PLAN_WAKE_ON_BUTTON_2026-07-12.md +++ /dev/null @@ -1,495 +0,0 @@ -# Wake-on-Button-Press from Deep Sleep — Implementation Plan - -## Context - -The ESP32 path of this firmware supports timer-based deep sleep for battery devices (`power_mode == 1`, `deep_sleep_time_seconds > 0`). Today the only wake source armed on the idle/timer sleep path is the RTC timer (`src/main.cpp:463`), with an explicit TODO at `src/main.cpp:464` to add button wake. The user wants: any configured button on a wake-capable pin should wake the display from deep sleep; on button wake, the device should stay awake for a minimum window (default 120 s) so the user can interact; this window should be unified with the existing first-boot 120 s holdoff (`FIRST_BOOT_DEEP_SLEEP_DELAY_MS`); it must be correct with battery latches (MOSFET and D-FF) and must not disturb the idle timer, post-wake advertising window, deep-sleep timer, power-off path, or nRF52840 builds. - -**Deliverable 0 (user request):** save the consolidated architecture report below to `docs/architecture-deep-sleep-power-buttons.md` as the first implementation step. - ---- - -## Step 0 — Write `docs/architecture-deep-sleep-power-buttons.md` - -Create the file with exactly the following content (update line anchors if the implementation lands after other changes): - -~~~markdown -# Architecture: Deep Sleep, Battery Latch, and Buttons (ESP32 path) - -Status: as of branch `feat/pipe-partial`, 2026-07-12. All behavior described here is -ESP32-only (`TARGET_ESP32`); the nRF52840 target compiles these subsystems as no-op -stubs and has no deep sleep. - -## 1. Deep sleep - -There are two distinct deep-sleep entry points. - -### 1.1 Timer deep sleep — `enterDeepSleep(bool force)` (`src/main.cpp:431-470`) - -The normal battery low-power path. Guards, in order: - -1. `globalConfig.power_option.power_mode != 1` (not battery) → return, no sleep. -2. `deep_sleep_time_seconds == 0` (disabled) → return. -3. A BLE client is connected and `!force` → return (live link aborts sleep). - -Then: sets RTC-persisted `woke_from_deep_sleep = true`, stops advertising, -`BLEDevice::deinit(true)` + handle clear, arms the timer wake -(`esp_sleep_enable_timer_wakeup(deep_sleep_time_seconds * 1e6)`, `main.cpp:463`), -flushes the log, calls `powerLatchHoldForSleep()` (`main.cpp:468`) so a latched -device keeps its power rail across sleep, and enters `esp_deep_sleep_start()`. - -Wake source on this path today: **timer only**. (`main.cpp:464` carries the TODO -for button wake.) - -Callers: -- `main.cpp:250` — post-wake advertising window timed out. -- `main.cpp:363` — main idle quiet-window elapsed. -- `device_control.cpp:711` — BLE command `0x0052` (`force = true`). - -### 1.2 Power-off deep sleep — `powerOff()` (`src/power_latch.cpp:83-106`) - -User-initiated shutdown on the MOSFET-latch path (3 s long-press on `pwr_pin_3`, or -a `binary_inputs` button flagged `power_off`). Waits for button release, drives the -latch pin LOW, `esp_sleep_config_gpio_isolate()`, `gpio_hold_en()`, then — under -`#if SOC_GPIO_SUPPORT_DEEPSLEEP_WAKEUP` — arms -`esp_deep_sleep_enable_gpio_wakeup(1ULL << buttonPin(), ESP_GPIO_WAKEUP_GPIO_LOW)` -on the shutdown button and calls `esp_deep_sleep_start()`. Wake source: **button -only** (or hardware re-latch if the rail actually drops). - -### 1.3 Wake-cause detection (`src/main.cpp:66-83`, in `setup()`) - -`esp_sleep_get_wakeup_cause()` is called once. Any cause other than -`ESP_SLEEP_WAKEUP_UNDEFINED` is treated identically as "woke from deep sleep": -sets `is_deep_sleep_wake` / `woke_from_deep_sleep`, increments RTC-persisted -`deep_sleep_count`. The specific cause (timer vs GPIO) is logged but not acted on. - -### 1.4 Boot sequence differences on wake - -`setup()` order: serial init → wake detect → `full_config_init()` (which calls -`powerLatchBegin()` at `config_parser.cpp:856`) → `initio()` → **if cold boot only:** -`initDisplay()` (EPD power + full refresh) → `ble_init()` → **if cold boot only:** -`initWiFi(false)` → `updatemsdata()`, `initButtons()`, `initTouchInput()` → -**if wake:** arm the post-wake advertising window (`advertising_timeout_active = true`, -`advertising_start_time = millis()`) → `lastActivityMs = millis()`. - -On a deep-sleep wake, display init, boot screen, and WiFi are skipped; the e-paper -image is retained. - -## 2. Sleep/wake timers - -All timing state lives in `src/main.h`; config fields come from the binary TLV -config blob (LittleFS-persisted), not NVS. - -| Timer / window | Definition | Default | Purpose | -|---|---|---|---| -| Deep sleep duration | `PowerOption.deep_sleep_time_seconds` (`structs.h:56`, uint16) | 0 = disabled | Timer wake interval | -| Idle quiet window ("stay awake") | `PowerOption.sleep_timeout_ms` (`structs.h:47`, uint16 ms) | 0 → `DEFAULT_IDLE_HOLD_MS` | Quiet time before sleeping; also the post-wake advertising window length | -| Idle hold fallback | `DEFAULT_IDLE_HOLD_MS` (`main.h:325`) | 10 000 ms | Fallback when `sleep_timeout_ms == 0` | -| First-boot holdoff | `FIRST_BOOT_DEEP_SLEEP_DELAY_MS` (`main.h:328`) | 120 000 ms | Cold-boot grace period before first sleep (gated by `deep_sleep_count == 0`) | -| Direct-write timeout | inline `main.cpp:293` | 900 000 ms | PIPE/direct-write session timeout | -| Power-off long-press | `POWER_OFF_HOLD_MS` (`power_latch.cpp:21`) | 3 000 ms | Latch shutdown hold | - -Runtime/RTC state: `RTC_DATA_ATTR bool woke_from_deep_sleep` (`main.h:312`), -`RTC_DATA_ATTR uint32_t deep_sleep_count` (`main.h:313`), -`advertising_timeout_active` / `advertising_start_time` (`main.h:316-317`), -`lastActivityMs` (`main.h:323`), first-boot-delay state (`main.h:329-331`). - -### 2.1 Loop decision flow (`src/main.cpp:219-389`) - -`pollActivity()` (`main.cpp:144-187`) stamps `lastActivityMs` whenever BLE queues, -connection state, touch, LAN state, or the button MSD payload (`memcmp` of -`dynamicreturndata`) change — so a button press resets the idle timer indirectly. - -- **Post-wake window branch** (`main.cpp:226-260`), active while - `woke_from_deep_sleep && advertising_timeout_active`: a BLE connect exits the - window into full setup; otherwise after `sleep_timeout_ms` (or 10 s) of quiet → - `enterDeepSleep()`. -- **Normal branch**: drains BLE work; computes `workInFlight`; when idle applies the - first-boot 120 s holdoff, then the idle quiet window → `enterDeepSleep()`. - -## 3. Battery latch - -Two mechanisms, both in `src/power_latch.cpp` (ESP32-only; no-op stubs otherwise), -selected by `SystemConfig.device_flags` bits and sharing pins `pwr_pin_2`/`pwr_pin_3` -(`structs.h:29-32`): - -| Flag | Bit | Mechanism | pwr_pin_2 | pwr_pin_3 | -|---|---|---|---|---| -| `DEVICE_FLAG_BATTERY_LATCH` | 1<<3 | Self-holding MOSFET/load-switch | latch enable | active-low shutdown button | -| `DEVICE_FLAG_PWR_LATCH_DFF` | 1<<4 | 74AHC1G79 D flip-flop | D (`PWR_HOLD`) | CP clock (`PWR_LOCK`) | - -Key behaviors: -- `powerLatchBegin()` (`power_latch.cpp:110-122`, called from `full_config_init`): - releases any RTC hold from a prior sleep (`gpio_hold_dis`), engages the D-FF - (drive D high, pulse CP), sets the MOSFET shutdown button to `INPUT_PULLUP`. -- `powerLatchHoldForSleep()` (`power_latch.cpp:148-167`): before timer deep sleep, - drives the hold pin HIGH and latches it with `gpio_hold_en()` + - `gpio_deep_sleep_hold_en()` (skipped on C6, which lacks that API) so the rail - stays up through deep sleep. **A latched battery device therefore does enter - timer deep sleep and self-wakes — it does not cut power on the idle path.** -- `powerOff()` (MOSFET) — see §1.2. `dffLatchRelease()` (D-FF) clocks Q low → hard - power cut, no deep sleep; re-power is via the hardware button re-latching the FF. -- BLE command `0x0052` (`device_control.cpp:700-714`): D-FF → hard off; otherwise - `enterDeepSleep(true)`. -- There is **no low-battery cutoff logic**; battery voltage (ADC or BQ27220) is - measured and reported only. - -## 4. Buttons - -Configured via the repeatable `binary_inputs` TLV packet (id `0x25`), struct -`BinaryInputs` (`structs.h:247-268`): up to 4 instances × 8 pins = 32 buttons -(`MAX_BUTTONS`, `structs.h:400`). Per-pin bitmasks: `input_flags` (active pins), -`invert` (active-low), `pullups`, `pulldowns`, `power_off_flags`; -`power_off_hold_sec`; `reserved[12]` spare bytes. Parsed by fixed-size `memcpy` -(`config_parser.cpp:381-391`) — struct size is ABI. - -Runtime: -- `initButtons()` (`device_control.cpp:551-658`): pinMode with per-pin pull config, - `attachInterruptArg(pin, buttonISR, idx, CHANGE)`, 50 ms boot settle that resets - `press_count` and discards spurious startup edges. Skips `0xFF` pins and the - GT911 touch INT pin. -- ISR (`device_control.cpp:512-527`, `IRAM_ATTR`): edge-triggered; updates - `current_state`, increments 4-bit `press_count` on press, flags - `buttonEventPending`. -- `processButtonEvents()` (`device_control.cpp:420-452`): packs - `(button_id | press_count<<3 | state<<7)` into `dynamicreturndata[byte_index]` - and re-publishes the BLE advertising MSD. Buttons report to the host; they do - not drive local page changes. -- Power-off: `pollConfiguredPowerOffButtons()` (`device_control.cpp:50-81`) for - flagged buttons, `powerButtonPoll()` (`power_latch.cpp:124-145`) for the - dedicated `pwr_pin_3` button. - -Gaps (pre-feature): no RTC-capability validation of button pins, no -ext0/ext1/`rtc_gpio_*` usage, and normal buttons never arm a deep-sleep wake — -only the dedicated latch shutdown button does, and only on the power-off path. - -## 5. Variant awareness - -A single `TARGET_ESP32` macro covers all ESP32 chips (envs: classic ESP32, S3, C3, -C6). The only per-chip guards are `#if !defined(CONFIG_IDF_TARGET_ESP32C6)` around -`gpio_deep_sleep_hold_en()` and `#if SOC_GPIO_SUPPORT_DEEPSLEEP_WAKEUP` around the -GPIO wake call in `powerOff()`. Wake-capable pins differ by chip: classic ESP32 -and S2/S3 wake via ext0/ext1 on RTC GPIOs; C3/C6 wake via -`esp_deep_sleep_enable_gpio_wakeup()` on their low-power GPIO range. -~~~ - ---- - -## Verified framework facts (arduino-esp32 core 3.3.9, checked in `~/.platformio/packages/framework-arduinoespressif32-libs//include/`) - -| Capability | esp32 classic | esp32s3 | esp32c3 | esp32c6 | -|---|---|---|---|---| -| ext0 (`SOC_PM_SUPPORT_EXT0_WAKEUP`) | yes | yes | — | — | -| ext1 (`SOC_PM_SUPPORT_EXT1_WAKEUP`) | yes (ALL_LOW / ANY_HIGH only) | yes (ANY_LOW / ANY_HIGH) | — | yes (+ per-pin mode) | -| `SOC_GPIO_SUPPORT_DEEPSLEEP_WAKEUP` | — | — | yes (GPIO0–5) | yes (GPIO0–7) | -| Wake-valid pins | 0,2,4,12–15,25–27,32–39 | 0–21 | 0–5 | 0–7 | - -- `esp_sleep_is_valid_wakeup_gpio(gpio_num_t)` exists unguarded on every chip (`esp_sleep.h:250`) — use it as the runtime capability check; **no hand-rolled pin tables**. -- `esp_sleep_get_ext1_wakeup_status()` (unguarded) and `esp_sleep_get_gpio_wakeup_status()` (guarded by `SOC_GPIO_SUPPORT_DEEPSLEEP_WAKEUP`) identify the waking pin(s). There is **no ext0 status API** — remember the armed ext0 pin in `RTC_DATA_ATTR`. -- `rtc_gpio_pullup_en` / `rtc_gpio_pulldown_en` exist in `driver/rtc_io.h`. -- `gpio_deep_sleep_hold_en()` only affects pads on which `gpio_hold_en()` was called (driver/gpio.h:433–450) — so `powerLatchHoldForSleep()` cannot freeze wake-button pads; **power_latch.cpp needs no changes**. -- `CONFIG_ESP_SLEEP_GPIO_ENABLE_INTERNAL_RESISTORS=y` in shipped sdkconfigs — for C3/C6 GPIO wake, `esp_deep_sleep_start()` auto-enables the pull opposite the wake level. -- ext1 note (esp_sleep.h:354–361): with RTC_PERIPH powered down, IDF maintains configured pulls via the HOLD feature automatically; `esp_sleep_pd_config(ESP_PD_DOMAIN_RTC_PERIPH, ON)` is a fallback only. -- `ESP_EXT1_WAKEUP_ANY_LOW` does **not exist** on classic ESP32. - -**Unverifiable locally (precompiled libs — confirm on hardware, see validation list):** -1. Two successive `esp_deep_sleep_enable_gpio_wakeup()` calls with different levels accumulate per-pin (true in IDF v5.3–5.5 source; check return code of the second call at runtime). -2. IDF forces RTC_PERIPH on when ext0 is armed on classic ESP32. -3. Sleep-current impact of held pulls — hardware measurement. - ---- - -## Step 1 — New module `src/wake_button.h` / `src/wake_button.cpp` - -Follow the `power_latch.cpp` pattern: entire implementation inside `#if defined(TARGET_ESP32)`, empty stubs in `#else` (nRF unaffected). Include the local `DEVICE_FLAG_*` fallback defines exactly as `power_latch.cpp:10-15` does. - -Public API: -```cpp -// Arm all eligible button-wake sources for the upcoming timer deep sleep. -// Never fails; logs each pin's disposition. No-op on nRF. -void armButtonWakeSources(); -// Classify wake cause and log the waking pin(s). Call once, early in setup(). -// Returns true for EXT0/EXT1/GPIO causes. Safe pre-config (reads sleep regs only). -bool detectButtonWake(esp_sleep_wakeup_cause_t cause); -``` - -Internal state: `RTC_DATA_ATTR static uint8_t s_ext0WakePin = 0xFF;` (survives sleep; only way to log the ext0 wake pin). Externs for `buttonStates[]`, `buttonStateCount`, `globalConfig` (same style as `device_control.cpp:42`). - -### Wake-mask construction (all variants) - -Candidates: -1. Every initialized `buttonStates[i]` pin (these already exclude `0xFF` and the GT911 touch INT pin). Wake level = pressed level = `inverted ? LOW : HIGH`. -2. `system_config.pwr_pin_3` as an **active-low** candidate **only when** `DEVICE_FLAG_BATTERY_LATCH` is set and the pin is valid — so the MOSFET-latch power button also wakes from timer sleep (mirrors `powerOff()` semantics). - -Exclusions, each logged: -- `power_option.sleep_flags` bit 0 set (`SLEEP_FLAG_BUTTON_WAKE_DISABLE`) → arm nothing (feature default-ON per the "any button press should wake" principle). -- Pin == `pwr_pin_2` always; pin == `pwr_pin_3` when `DEVICE_FLAG_PWR_LATCH_DFF` (**critical**: on D-FF boards pwr_pin_3 is the flip-flop CP clock — a wake-armed pull could clock the latch off). -- `!esp_sleep_is_valid_wakeup_gpio(pin)` → warn "pin N not wake-capable on this chip; timer-only for this button". -- Pin currently reading its pressed level at sleep entry → skip this cycle, log "held at sleep entry" (prevents instant-wake ping-pong; pin re-qualifies next sleep entry after release). - -Build 64-bit `lowMask` (wake on LOW) and `highMask` (wake on HIGH). - -### Per-variant arming - -```cpp -#if SOC_GPIO_SUPPORT_DEEPSLEEP_WAKEUP // C3 (GPIO0-5), C6 (GPIO0-7) - if (lowMask) esp_deep_sleep_enable_gpio_wakeup(lowMask, ESP_GPIO_WAKEUP_GPIO_LOW); - if (highMask) err = esp_deep_sleep_enable_gpio_wakeup(highMask, ESP_GPIO_WAKEUP_GPIO_HIGH); - // If the second call errors (mixed-polarity accumulation is the unverified item): - // log, keep the first group, never abort sleep. Pulls auto-configured by IDF. -#elif SOC_PM_SUPPORT_EXT1_WAKEUP // classic, S2, S3 - #if CONFIG_IDF_TARGET_ESP32 - // classic has no ANY_LOW: HIGH group -> ext1 ANY_HIGH; LOW group -> ext0 (ONE pin, - // lowest-numbered), s_ext0WakePin = pin; warn for each additional low pin not armed. - if (highMask) esp_sleep_enable_ext1_wakeup(highMask, ESP_EXT1_WAKEUP_ANY_HIGH); - if (lowMask) esp_sleep_enable_ext0_wakeup(firstLowPin, 0); - #else - // S2/S3: larger polarity group -> ext1 (ANY_HIGH or ANY_LOW); other group's first - // pin -> ext0; warn extras. (One ext1 call only — a second call replaces the first.) - #endif - // Pull retention: rtc_gpio_pullup_en()/rtc_gpio_pulldown_en() per the button's - // configured pullups/pulldowns bitmasks (and INPUT_PULLUP for the latch button). - // Pins with no internal pull and unknown external hardware: warn "floating wake - // pin may cause spurious wakes" but still arm. -#endif -``` - -Timer wake (`esp_sleep_enable_timer_wakeup`, main.cpp:463) is **always left armed** — ext0/ext1/gpio/timer coexist. Both masks empty → single log "no wake-capable buttons — timer-only deep sleep". - -### `detectButtonWake(cause)` - -Switch on cause: `EXT0` (log `s_ext0WakePin`), `EXT1` (log `esp_sleep_get_ext1_wakeup_status()` mask), `GPIO` under `#if SOC_GPIO_SUPPORT_DEEPSLEEP_WAKEUP` (log `esp_sleep_get_gpio_wakeup_status()` mask) → return true; `TIMER` → log "timer wake", return false; default → false. - -**Decision:** a button wake is logged and arms the stay-awake window but does **not** inject a synthetic press into the BLE MSD payload. The press happened while the ISR was dead; `initButtons()`'s settle pass resets `press_count`/state from live pin levels anyway, so a synthetic event would be erased or double-counted. If the button is still held when `initButtons()` runs, its state lands in MSD naturally. - ---- - -## Step 2 — Shared minimum-wake-time (the timer refactor) - -### Config (`src/structs.h`) - -`PowerOption` (structs.h:44-61): split `reserved[7]` (line 60) → -```cpp -uint16_t min_wake_time_seconds; // Min awake window after first boot or button wake; 0 = default 120 s -uint8_t reserved[5]; -``` -Struct size unchanged → the fixed-size `memcpy` parse and all existing config blobs stay valid; old blobs read 0 → default 120 s. (`sleep_timeout_ms` is uint16 **ms**, max 65.5 s — cannot hold 120 s, hence a new seconds field.) - -Add near structs.h:63: -```cpp -#define SLEEP_FLAG_BUTTON_WAKE_DISABLE (1u << 0) // power_option.sleep_flags bit 0 -``` -(`sleep_flags` bits are all reserved today — verified in firmware and the toolbox schema.) - -### One mechanism: a minimum-wake hold deadline (`src/main.h` + `src/main.cpp`) - -**Semantics: the hold is a floor layered under the existing quiet-window logic, not a replacement.** Sleep requires both the existing idle/advertising quiet condition AND the hold expired. `pollActivity()`/`lastActivityMs` untouched — interaction keeps extending the quiet window inside and beyond the floor. On timer wake the hold is never armed → behavior bit-identical to today. - -`src/main.h`: delete lines 328-331 (`FIRST_BOOT_DEEP_SLEEP_DELAY_MS`, `firstBootDelayInitialized/Elapsed/Start`); add next to the advertising globals (~line 316): -```cpp -static constexpr uint16_t DEFAULT_MIN_WAKE_TIME_SECONDS = 120; -bool minWakeWindowActive = false; // armed in setup() on first boot or button wake -uint32_t minWakeWindowStartMs = 0; -``` - -`src/main.cpp` helpers (near `pollActivity()`): -```cpp -static uint32_t minWakeTimeMs() { - uint16_t s = globalConfig.power_option.min_wake_time_seconds; - return (uint32_t)(s ? s : DEFAULT_MIN_WAKE_TIME_SECONDS) * 1000UL; -} -static bool minWakeHoldActive() { - if (!minWakeWindowActive) return false; - if (millis() - minWakeWindowStartMs >= minWakeTimeMs()) { - minWakeWindowActive = false; - writeSerial("Minimum wake window elapsed, deep sleep permitted"); - return false; - } - return true; -} -``` - -Documented behavior delta: the first-boot holdoff now counts from end of setup() rather than from the first quiet loop pass — makes "awake ≥ 120 s from power-on" a real guarantee. - ---- - -## Step 3 — `src/main.cpp` edits - -1. **setup() wake-cause block (main.cpp:66-83):** after line 68, add `bool woke_by_button = detectButtonWake(wakeup_reason);` — replaces the raw numeric log at line 74 with named cause + waking pin mask. -2. **Window arming (main.cpp:122-133):** keep the existing advertising-window arm; extend: -```cpp -if (is_deep_sleep_wake) { - advertising_timeout_active = true; - advertising_start_time = millis(); - if (woke_by_button) { - minWakeWindowActive = true; - minWakeWindowStartMs = millis(); - writeSerial("Button wake: holding awake >= " + String(minWakeTimeMs()) + " ms"); - } -} else if (deep_sleep_count == 0) { // first boot (RTC count survives soft resets) - minWakeWindowActive = true; - minWakeWindowStartMs = millis(); -} -``` -(Arm regardless of power_mode — every consuming path is already gated by `power_mode == 1` / `deep_sleep_time_seconds > 0`, so the flag is inert on wired devices.) -3. **Post-wake advertising branch (main.cpp:246):** `if (idle_duration >= advertising_timeout_ms)` → `if (idle_duration >= advertising_timeout_ms && !minWakeHoldActive())`. Timer wake: hold never armed → unchanged short window. Button wake: window ends at max(quiet window, min wake time); `idleDelay(50)` at line 258 keeps servicing buttons/touch throughout. -4. **Delete the first-boot block (main.cpp:336-352)** — superseded by the hold. -5. **Idle gate (main.cpp:359):** `if (idleMs < idleHoldMs)` → `if (idleMs < idleHoldMs || minWakeHoldActive())`. Also covers connect-then-drop during a button-wake window (`woke_from_deep_sleep` cleared at line 231 → normal idle logic still honors the floor). -6. **`enterDeepSleep()` (main.cpp:431-470):** - - After the connected-client guard (line 448), defense-in-depth: `if (!force && minWakeHoldActive()) return;` (`force` from BLE 0x0052 bypasses — command behavior unchanged). - - Replace the TODO at line 464 with `armButtonWakeSources();` — after `esp_sleep_enable_timer_wakeup()` (463), **before** `powerLatchHoldForSleep()` (468). Ordering: latch-hold manipulation then can't disturb freshly configured RTC pulls, and `gpio_hold_en` applies only to the latch pin. - -## Step 4 — Supporting edits - -- `src/config_parser.cpp` (~line 681, config dump): print `min_wake_time_seconds` and the `sleep_flags` button-wake bit (log-based verification). -- `src/power_latch.cpp`: **no changes** — `powerOff()`/`dffLatchRelease()` hard-off paths (with `esp_sleep_config_gpio_isolate`) keep their existing single-button wake arming; `enterDeepSleep()` never calls isolate, so new wake pins stay live through timer sleep. -- Companion (separate repo, non-blocking): `opendisplay.org/httpdocs/firmware/toolbox/config.yaml` power_option — name `sleep_flags` bit 0 `button_wake_disable`; carve `min_wake_time_seconds` (2 bytes) from reserved. - ---- - -## Step 5 — Protocol change: `0x0052` optional 2-byte sleep-duration payload - -**Requirement:** `0x0052` gains an optional 2-byte big-endian payload of seconds — e.g. `00 52 00 FF` commands "sleep for 255 seconds." The value overrides `deep_sleep_time_seconds` for **exactly one** deep-sleep cycle. - -### Backward compatibility — verified against the dispatcher - -`imageDataWritten()` (`communication.cpp:498-506`) only requires `len >= 2` and parses the big-endian command ID from bytes 0-1; `case 0x0052` (`communication.cpp:655`) currently calls `handleDeepSleepCommand()` with no payload pointer, ignoring any trailing bytes. Therefore: -- **New host → old firmware:** `00 52 00 FF` matches `0x0052`; the extra 2 bytes are ignored; device sleeps with the config duration. Graceful degradation, no error path. -- **Old host → new firmware:** bare `00 52` yields zero payload bytes → no override → config duration. Bit-identical to today. -- Big-endian seconds matches both the user-specified example and the command-ID framing convention; the `data + 2, len - 2` payload convention matches every other parameterized command (0x70, 0x73, 0x77, …). - -### Changes - -1. **`src/communication.cpp:655-657`:** `case 0x0052: handleDeepSleepCommand(data + 2, len - 2); break;` -2. **`src/device_control.h:16` / `src/device_control.cpp:700-715`:** `void handleDeepSleepCommand(const uint8_t* payload, uint16_t payloadLen)`. Parse: - - `payloadLen >= 2` → `overrideSeconds = ((uint16_t)payload[0] << 8) | payload[1]` (extra bytes beyond 2 ignored for forward compatibility). `0x0000` = explicit "no override" (sentinel, uses config). - - `payloadLen == 1` → malformed: log warning, treat as no payload. - - `payloadLen == 0` → no override (legacy behavior). - - **D-FF path:** payload is meaningless (hard power cut has no timer, no self-wake) — log "duration payload ignored (D-FF hard power off)" if `overrideSeconds != 0`, then proceed with the existing ACK + `powerLatchPowerOff()` unchanged. - - **Non-DFF path — eligibility pre-check with NACK (consistency decision, see below):** before calling `enterDeepSleep`, the handler checks the same two config guards `enterDeepSleep` enforces and reports rejection using the codebase's standard response convention (success `{0x00, cmd, 0x00, 0x00}` / error `{0xFF, cmd, code, 0x00}`, as in `handleLedActivate`, device_control.cpp:374-417): - - `power_mode != 1` → NACK `{0xFF, 0x52, 0x02, 0x00}`, return (no sleep — wired guard, unchanged eligibility). - - `deep_sleep_time_seconds == 0` → NACK `{0xFF, 0x52, 0x01, 0x00}`, return (**the payload does NOT enable sleep on a config-disabled device**). - - Otherwise: `enterDeepSleep(true, overrideSeconds);` - Backward compatible: today's non-DFF `0x0052` sends no response at all, so old hosts ignore the new unsolicited NACK; new hosts gain observable rejection. -3. **`enterDeepSleep()` signature (`src/main.cpp:431`, decl in main.h):** `void enterDeepSleep(bool force = false, uint16_t overrideSleepSeconds = 0)`. Inside: - - **All existing guards unchanged** (power_mode 432, `deep_sleep_time_seconds == 0` 437, BLE-connected 444). The override never changes *eligibility* — only the duration of an otherwise-permitted sleep. - - After the guards: `uint16_t sleepSeconds = overrideSleepSeconds ? overrideSleepSeconds : globalConfig.power_option.deep_sleep_time_seconds;` — timer arm (line 462-463) uses `sleepSeconds`; the entry log prints the effective duration and whether it came from the command override. - -**Consistency decision — why the override does NOT enable sleep when `deep_sleep_time_seconds == 0`:** the two config guards in `enterDeepSleep` are uniformly absolute today — both reject even the forced BLE command — and `deep_sleep_time_seconds == 0` is the firmware-wide deep-sleep disable switch (idle gate, first-boot holdoff, and `enterDeepSleep` all treat 0 as "disabled"; structs.h:56 "0 if not used"). Letting a payload bypass one guard but not the other would be asymmetric, and would give the same command different *eligibility* with vs without payload (bare `0x0052` already rejects on such devices). Rule adopted: **payload = duration only; eligibility is config's alone.** The legitimate future use case (host-driven duty cycling on a device with no autonomous timer) should be an explicit opt-in — e.g. a new `sleep_flags` bit — not a silent side effect of the duration payload; noted as out of scope. -4. **One-cycle semantics by construction:** the override is threaded as a **parameter**, never stored in a global and never `RTC_DATA_ATTR`. The idle-timer callers (`main.cpp:250`, `main.cpp:363`) use the default `0` → config duration. If the guarded entry aborts (wired device), the override is discarded with the call — it cannot leak into a later sleep. After the override sleep, wake → boot reinitializes everything → every subsequent cycle uses config. No clearing logic needed, no invalid state possible. -5. **Interaction with button wake:** none required — `armButtonWakeSources()` runs identically; a button can cut an override sleep short, and the subsequent wake windows behave per the boot-side table (timer wake → short window; button wake → 120 s floor). -6. **Docs:** note the payload in `docs/architecture-deep-sleep-power-buttons.md` §3.1 and in the companion host-side command reference (separate repo, non-blocking). - -### Logic table addendum (replaces/refines rows 9-10) - -| # | Trigger | Latch | Payload | Behavior | -|---|---|---|---|---| -| 9a | `0x0052`, D-FF | D-FF | none | ACK, hard power cut — unchanged | -| 9b | `0x0052`, D-FF | D-FF | N seconds | payload logged + ignored, ACK, hard power cut (no timer exists once power drops) | -| 10a | `0x0052`, non-DFF | any/none | none or `0x0000` | `enterDeepSleep(true, 0)` — config duration; buttons + timer armed | -| 10b | `0x0052`, non-DFF | any/none | N seconds | sleeps N seconds (config overridden this cycle only); buttons + timer armed; next idle sleep uses config | -| 10c | `0x0052`, non-DFF, config `deep_sleep_time_seconds == 0` | any/none | any (or none) | rejected: NACK `{0xFF, 0x52, 0x01, 0x00}`, no sleep — payload does not enable a config-disabled device | -| 10d | `0x0052`, non-DFF, `power_mode != 1` | any/none | any | rejected: NACK `{0xFF, 0x52, 0x02, 0x00}`, no sleep (wired guard unchanged) | -| 10e | `0x0052`, malformed 1-byte payload | any | 1 byte | warning logged, treated as 10a | - ---- - -## Logic / state table - -Sleep-entry rows (non-forced `enterDeepSleep()`; timer source armed in every "sleeps" row): - -| # | power_mode / deep_sleep_time | Latch | Buttons | Armed wake sources at sleep entry | Notes | -|---|---|---|---|---|---| -| 1 | wired (≠1) or time=0 | any | any | never sleeps (early return 432/437) | unchanged | -| 2 | battery, >0 | none | none | timer only; log "timer-only" | unchanged + 1 log | -| 3 | battery, >0 | none | all non-capable pins | timer only; per-pin warn | e.g. C3 button on GPIO9 | -| 4 | battery, >0 | none | active-low capable | classic: ext0 (first) + warn rest; S3: ext1 ANY_LOW; C3/C6: gpio LOW | | -| 5 | battery, >0 | none | active-high capable | classic/S3: ext1 ANY_HIGH; C3/C6: gpio HIGH | | -| 6 | battery, >0 | none | mixed polarity | classic: ext1 ANY_HIGH + ext0 one low; S3: ext1 larger group + ext0 first of other; C3/C6: two gpio calls (hw-verify) | warns list unarmed pins | -| 7 | battery, >0 | MOSFET | rows 2–6 | same + pwr_pin_3 joins low group if capable; pwr_pin_2 excluded, held HIGH via gpio_hold_en | power button wakes; hold unaffected | -| 8 | battery, >0 | D-FF | rows 2–6 | same, but pwr_pin_2 AND pwr_pin_3 force-excluded (CP clock) | latch cannot be clocked off | -| 9 | BLE 0x0052, D-FF | D-FF | any | hard power cut — no wake sources; duration payload ignored/logged | see Step 5 addendum | -| 10 | BLE 0x0052, non-DFF | any | any | timer (config or 2-byte payload override, one cycle) + buttons | see Step 5 addendum rows 10a-10e | -| 11 | MOSFET 3 s power-off | MOSFET | any | unchanged `powerOff()`: isolate + gpio wake on pwr_pin_3 only | untouched path | -| 12 | any sleeps-row, button held at entry | any | held pin | held pin skipped (logged); if only wake pin → timer-only this cycle | ping-pong mitigation | - -Boot-side rows: - -| Wake cause | woke_from_deep_sleep | Window(s) armed | -|---|---|---| -| UNDEFINED, deep_sleep_count==0 (true first boot OR hidden mid-cycle reset — see below) | false | min-wake hold (120 s default); full display init | -| UNDEFINED, deep_sleep_count>0 | false | none — defensive row only; unreachable in practice (see "Hidden mid-cycle resets") | -| TIMER | true | short advertising window only — unchanged | -| EXT0 / EXT1 / GPIO | true | advertising window + min-wake floor | -| GPIO after MOSFET `powerOff()` | per prior RTC flag | as row above when flag set; additionally gets the floor (improvement) | -| other (UART/ULP — never armed) | true | treated like timer wake (default case) — safe | - -### Hidden mid-cycle resets (wake cause UNDEFINED after a crash) — focused analysis - -**Question: when a hidden mid-cycle reset occurs (panic / WDT / brownout / `esp_restart()` between deep-sleep cycles), what is correct behavior — and should it do a full display init? Answer: yes, full display init is correct and required, and the plan's first-boot rule already delivers the right window behavior. Details:** - -**Empirical fact (changes the state model):** `docs/FINDINGS_DEEP_SLEEP_WAKE_BOOT_SCREEN_2026-07-07.md` captured this exact scenario on hardware (reTerminal E1001): a PANIC on the wake path → `RTC_SW_CPU_RST` → `=== NORMAL BOOT === Deep sleep count (RTC): 0` — the count was 2 immediately before the panic. `RTC_DATA_ATTR` variables do **not** survive non-deep-sleep resets on this platform: the second-stage bootloader reloads RTC memory segments from the app image on every reset except a deep-sleep wake. So after any panic/WDT/SW/brownout reset, the device boots with `deep_sleep_count = 0`, `woke_from_deep_sleep = false`, `rebootFlag = 1`, `displayed_etag = 0` — **indistinguishable from a true first boot.** The `UNDEFINED + count>0` table row is defensive only; the code comment at `main.cpp:79-81` claiming RTC survives soft resets is contradicted by the captured log and must be fixed during implementation. - -**Why full display init is correct here:** -1. **Panel controller state is unknown.** The crash may have hit mid-refresh; an EPD controller abandoned mid-waveform (rails up, charge on the panel) must be re-initialized. `initDisplay()` (rail power cycle + controller init + full refresh) is the only path that guarantees a known-good panel state. Skipping it to preserve the image would trade a cosmetic flash for an unverifiable panel state. -2. **Host re-sync is forced automatically.** `rebootFlag = 1` (reloaded initializer) is advertised in MSD, and `displayed_etag = 0` makes any partial/etag-gated write NACK into a full push. The crash recovery contract is self-healing — full init + boot screen is consistent with it. -3. **Diagnostic value.** The boot screen appearing on a battery device is precisely how the 2026-07-07 panic was discovered. Silent "seamless" recovery would hide crash loops on headless devices. - -**Window behavior on a hidden reset (what this plan produces):** since the reset presents as `count == 0`, the 120 s min-wake hold arms. This is desirable: after an abnormal reset the device stays connectable long enough for the host to re-push the image (and for a developer to capture logs). It is also **not a behavior change** — today's first-boot gate (`!woke_from_deep_sleep && deep_sleep_count == 0`) already fires after a crash for the same reason, so the crash reboot already waits 2 minutes before its first sleep. A crash *loop* therefore costs ~120 s awake per crash; that is accepted — a crash loop is a firmware bug to fix, not a state to optimize battery for. - -**If the defensive row ever fires** (`UNDEFINED + count>0`, e.g. a future IDF/bootloader that preserves RTC segments): no hold is armed, the device full-inits the display, and sleeps after the normal idle quiet window — safe, no invalid state. If distinguishing a true cold boot from a crash reset ever becomes necessary, the correct tool is `esp_reset_reason()` (already logged at `main.cpp:66-67`: `ESP_RST_POWERON` vs `PANIC`/`SW`/`WDT`/`BROWNOUT`), not RTC counters. - -### Advertising continuity during the awake windows (EXT0/EXT1/GPIO wake) — verified - -The button-wake windows do not introduce any state where the device is awake with advertising stopped: - -1. **Start:** on every deep-sleep wake, `ble_init_esp32()` starts advertising unconditionally in setup (`ble_init.cpp:331`) before the windows are armed, so the post-wake branch always begins with the radio advertising. -2. **Only two deliberate stop sites exist while disconnected, and neither strands the device:** - - `updatemsdata()` (`display_service.cpp:1376-1389`) stops advertising only to swap in a fresh MSD payload and restarts it ~50 ms later in the same call (pre-existing refresh blip, unchanged by this feature). - - `enterDeepSleep()` stops advertising (`main.cpp:450-456`) — but every abort guard precedes that point, and after the stop there is no return path: it always reaches `esp_deep_sleep_start()`. **Ordering requirement:** the new `if (!force && minWakeHoldActive()) return;` guard MUST be inserted after the connected-client guard (line 448) and before the advertising stop (line 450), so an aborted sleep can never leave advertising stopped. (Step 3 item 6 places it there.) -3. **Disconnect healing:** a connect-then-drop sets `bleRestartAdvertisingPending` (`esp32_ble_callbacks.h:69`), serviced in both loop branches (window branch `main.cpp:236-238`, normal branch `main.cpp:288-290`). The pending flag is also in `pollActivity()`'s watch list (`main.cpp:174`), so a pending restart stamps `lastActivityMs` — no window can expire while a re-advertise is owed. `esp32_restart_ble_advertising()` defers while `epdRefreshInProgress` (`ble_init.cpp:251-253`), but that flag also sets `workInFlight` and counts as activity, so the device stays awake and retries. -4. **The hold adds no stop point:** when `minWakeHoldActive()` keeps a branch alive past its quiet timeout, the branch only runs `idleDelay(50)`/`idleDelay(5)` (buttons/touch serviced); advertising is untouched. While a client is connected, advertising is off — standard single-connection BLE peripheral behavior, not a gap. - -Net guarantee: for the entire button-wake awake window (advertising window + 120 s min-wake floor), the device is either advertising, momentarily restarting advertising to refresh MSD, connected to a client, or deferring a restart behind an in-flight refresh that itself holds the device awake. - -**Implementation additions from this analysis:** -- Fix the incorrect comment at `main.cpp:79-81` (RTC does not survive soft resets; a non-zero count on NORMAL BOOT is not an expected signal). -- Validation task: force a mid-cycle crash (`abort()` or a test command) between sleep cycles → verify full display init, `count` reset to 0, 120 s hold armed, host re-push succeeds (etag 0 → full write). -- **Optional hardening, out of scope for this feature:** brownout-aware boot. The findings doc (source #4) documents the loop: brownout → boot → full-refresh current spike → brownout again. A follow-up could special-case `ESP_RST_BROWNOUT` (defer the full refresh, shorten the awake window, sleep quickly to let the battery recover). Not part of the wake-on-button change. - -**No invalid states:** `minWakeWindowActive` on a wired device is inert (all consumers power_mode-gated); the hold self-clears by time (single `millis()` compare, wraparound-safe subtraction); every failure mode degrades to "timer-only sleep" or "shorter window" — never to no-sleep or no-wake lockup. The only loop-like scenario (button held across sleep entry) is skipped at arming time and self-limiting regardless. No dynamic allocation, no exceptions, no unbounded loops in any new code. - -**No new latency:** `armButtonWakeSources()` runs only inside `enterDeepSleep()` (already a slow teardown with delays); `detectButtonWake()` once in setup(); loop() gains exactly two short-circuit `minWakeHoldActive()` calls in branches that only execute when idle. The hot BLE/WiFi path (main.cpp:264-334), `pollActivity()`, idle-timer math, and deep-sleep timer are untouched. - ---- - -## Validation tasks - -**Compile matrix** (all must build clean): `pio run` for every ESP32 env (esp32-s3-* variants, esp32-c3-N4/N16, esp32-c6-N4, esp32-N4) plus the nRF env (must compile against wake_button stubs with no behavior change). - -**Hardware, per variant (S3 + C3/C6 + classic where available):** -1. Timer wake regression: sleeps, wakes after `deep_sleep_time_seconds`, log shows "timer wake", short window, re-sleeps after quiet window. -2. Button wake: press during sleep → boots, log shows named cause + pin mask + "holding awake >= 120000 ms"; device connectable ≥ 120 s with no interaction, then sleeps. -3. `min_wake_time_seconds` override (e.g. 30) honored for both first boot and button wake. -4. First boot after flash/battery insert: ≥ 120 s before first sleep. -5. Button held at sleep entry: "held at sleep entry" logged, timer-only sleep, no wake ping-pong. -6. Non-capable pin (e.g. C3 button on GPIO > 5): warning logged, timer wake unaffected. -7. Mixed polarity on C3/C6: second `esp_deep_sleep_enable_gpio_wakeup` returns ESP_OK and both polarities wake (unverified item 1). -8. MOSFET latch board: timer sleep keeps rail up; pwr_pin_3 wakes from timer sleep; 3 s hold power-off + button-on still works; sleep current measured vs pre-change. -9. D-FF board: 0x0052 hard-off unchanged; config-button wake works; pwr_pin_2/3 never appear in the logged wake mask. -10. BLE 0x0052 on non-DFF: sleeps immediately even inside the 120 s window (force path); button press wakes it. -10a. `0x0052` payload matrix: bare `00 52` → config duration (regression); `00 52 00 1E` → wakes after 30 s, next idle sleep uses config again (one-cycle check); `00 52 00 00` → config duration; `00 52 00 FF` on a device with `deep_sleep_time_seconds = 0` → NACK `{0xFF, 0x52, 0x01, 0x00}`, stays awake; same on a wired device (`power_mode != 1`) → NACK `{0xFF, 0x52, 0x02, 0x00}`; 1-byte payload → warning + config duration; payload sent to a D-FF board → ignored-payload log + hard off unchanged. -10b. Cross-version compatibility: new host with payload against previous firmware release → device sleeps with config duration, no error (backward-compat check). -11. Idle-timer/advertising-window timings identical to current firmware logs on timer-wake cycles. -12. Log checks: config dump prints `min_wake_time_seconds`/`sleep_flags`; sleep entry prints per-pin arming dispositions; boot prints named wake cause + pin mask. - -## Critical files - -- `src/wake_button.h` / `src/wake_button.cpp` — new module -- `src/main.cpp` — setup() wake detect (66-83), window arming (122-133), advertising branch (246), first-boot block removal (336-352), idle gate (359), enterDeepSleep (448, 464) -- `src/main.h` — remove first-boot statics (328-331), add min-wake globals (~316) -- `src/structs.h` — `PowerOption.min_wake_time_seconds` from reserved (60), `SLEEP_FLAG_BUTTON_WAKE_DISABLE` (~63) -- `src/config_parser.cpp` — config dump line (~681) -- `src/communication.cpp` — 0x0052 dispatch passes payload (655-657) -- `src/device_control.cpp` / `.h` — `handleDeepSleepCommand(payload, len)` (700-715, decl device_control.h:16) -- `docs/architecture-deep-sleep-power-buttons.md` — new (Step 0 content above) -- `src/power_latch.cpp` — reference only, must remain unchanged diff --git a/docs/PLAN_WORK_GATE_TRANSFER_TERMS_2026-07-29.md b/docs/PLAN_WORK_GATE_TRANSFER_TERMS_2026-07-29.md deleted file mode 100644 index f3d1d50..0000000 --- a/docs/PLAN_WORK_GATE_TRANSFER_TERMS_2026-07-29.md +++ /dev/null @@ -1,235 +0,0 @@ -# Plan — orphaned transfer state: heal it, don't idle on it - -**Date:** 2026-07-29 -**Branch:** `fix/loop-hang-3` -**Follows:** `4d37d43` *fix(ble): let a transport event interrupt the cooperative idle wait* - -Part (b) of the three-part fix for the ~40 s post-disconnect park. Parts (a) and -(c) landed in `4d37d43`. - -### Revision history - -This plan has been rewritten twice under review, and both rewrites changed the -conclusion rather than the wording. Recorded because the reasoning matters more -than the diff. - -**Draft 1** — amend `transferActive()` to exclude `pipeState.error`, add it and -`ble.eventPending()` to `workInFlight`. *Refuted:* the 15-minute direct-write -watchdog orphans full-PIPE state in a form the exclusion does not catch, so the -predicate would still latch true forever. Split into two commits, the first a -standalone bug fix. - -**Draft 2** — Commit 1 (the watchdog fix) landed as `77c2226`; Commit 2 kept the -gate terms. *Refuted:* `transferActive()` in the gate is redundant in every state -where the transfer can still progress, and actively harmful in the states where -it cannot — it converts a low-power park into up to fifteen minutes of full-CPU -spinning. The invariant it was meant to enforce is better enforced by healing the -orphan than by refusing to sleep on it. - -**This draft** keeps `ble.eventPending()`, drops `transferActive()` from the gate, -and replaces it with a self-healing assertion in the watchdog. - ---- - -## Commit 1 — landed as `77c2226` - -`fix(pipe): terminate the pipe session when the transfer watchdog fires` - -The direct-write watchdog released the panel but left `pipeState.active` set with -`pipeState.error` false. Because the `0x0081` handler gates on `pipeState` alone, -a timed-out full PIPE kept accepting frames into a torn-down session — and since -the cleanup zeroes the byte counters, the uncompressed auto-complete test read -`0 >= 0` and drove `bbepRefresh()` + `waitforrefresh(60)` at an unpowered panel. - -Both watchdogs now live in `display_service.cpp` behind `checkTransferTimeouts()` -and share `TRANSFER_WATCHDOG_MS`. Full reasoning is in the commit message; it is -not repeated here. - ---- - -## Commit 2 (revised) — `fix(loop): heal orphaned transfer state; count pending events as work` - -### Why `transferActive()` does not belong in the gate - -Draft 2 argued the gate was "wrong about what constitutes work" because a -half-finished transfer with the panel powered was not counted. That framing does -not survive contact with the state space. **A transfer whose transport is gone -cannot progress** — frames arrive only over BLE or LAN — so enumerate every state -in which `transferActive()` would be true: - -| State | What the gate does today | What the term would add | -|---|---|---| -| Owner connected, transfer live | `ble.isConnected()` / `wifiLanSession` already true | Nothing — redundant | -| Owner disconnecting | `serviceBleDisconnectCleanup()` runs at `main.cpp:619`, **before** the gate at :665, and resets all three flags. Deferral needs `epdRefreshInProgress`, refusal needs `ownerStillUp` — both already gate terms | Nothing — redundant | -| Orphaned: state set, transport gone | Gate false → `platformIdle()` → park (nRF) or deep sleep (ESP32) | Holds the gate until the 15-minute watchdog | - -The third row is the only distinct behaviour, and it is a regression: - -- `workInFlight` true takes the `delay(1)` path. On nRF that is **one tick**, - below `configEXPECTED_IDLE_TIME_BEFORE_SLEEP` (2), and the core's - `vApplicationIdleHook` is an empty weak stub — so it does not sleep. The idle - task spins at 64 MHz and the loop body re-runs ~1000×/s. -- The watchdog measures from START, so the spin lasts *15 minutes minus however - long the transfer already ran*. -- Cost per event: **~0.9–1.5 mAh** on nRF52840 (≈1.5–2 days of CR2450 standby), - **~6–12 mAh** on an ESP32-S3 tag with WiFi+BLE up. -- Worse, it removes an existing recovery. On battery ESP32 an orphan is - *self-healing today*, because deep sleep is a reboot and all transfer state is - plain RAM. The term converts a self-healing state into a fifteen-minute awake - one. - -So the term buys invariant-hardening at the price of the only states it applies -to. The right response to "this state should not exist" is to remove the state, -not to refuse to sleep while it exists. - -### The change - -**1. `main.cpp` — one gate term, not two:** - -```diff - const bool workInFlight = bleRxQueuePending() || bleTxQueuePending() || - ble.isConnected() || -+ ble.eventPending() || - s_advertisingRestartPending || - epdRefreshInProgress || - wifiLanSession; -``` - -`eventPending()` closes the real gap: an event raised after `serviceBleEvents()` -ran in this pass is otherwise invisible until the next one, and the pass is about -to park. This defers idle by exactly one pass, deliberately. - -**2. `display_service.cpp` — heal the orphan in `checkTransferTimeouts()`:** - -```c -// Commit 77c2226 proved a live pipe session always has a hardware half, and -// closed the one path that broke it. This asserts it at runtime rather than -// resting on the proof: any future path that recreates the orphan gets one log -// line and a reset, instead of a session that silently accepts 0x0081 frames -// into torn-down state. Deliberately not a gate term -- a transfer whose -// transport is gone cannot progress, so refusing to sleep on it burns power for -// work that will never happen. -if (pipeState.active && !pipeState.error && !directWriteActive && !partialCtx.active) { - od_log_error("ERROR: orphaned pipe session (no hardware half) - resetting"); - resetPipeWriteState(); -} -``` - -**3. `display_service.cpp` — drop the `> 0` timestamp guards:** - -```c -if (directWriteActive && directWriteStartTime > 0) // -> drop "&& ... > 0" -if (partialCtx.active && partialCtx.start_time > 0 && ...) // -> drop "&& ... > 0" -``` - -The `active` flag is set in straight-line code eleven lines from the `millis()` -stamp with no return between, so it already implies a valid timestamp. The guard -is not merely redundant: a transfer starting in the ~1 ms window where `millis()` -wraps through zero has its watchdog disabled **permanently**. Odds are of order -1 in 10⁹ transfers — this is a free removal of a reasoning burden, not a live -risk, and it matters because item 2 makes the watchdog the backstop for the -orphan assertion. - -**4. Split the two questions `transferActive()` currently answers.** It has five -callers asking two different things: - -| Caller | Question | Wants | -|---|---|---| -| `workInFlight` (proposed) | is live work in flight? | — *not adding it, see above* | -| `touch_input.cpp:587` | may I poll GT911 over I2C? | live work only | -| `wifi_service.cpp:697` | may I run a full-channel scan? | live work only | -| `display_service.cpp:1921,1925` | would logging this frame spam? | **any** stream, including a dead one still receiving frames | - -Amend `transferActive()` to `directWriteActive || partialCtx.active || -(pipeState.active && !pipeState.error)` for the first three, and give the -log-quieting predicates their own broader test that keeps the errored case quiet. - -Without the split, a fatal NACK un-suppresses every remaining in-flight `0x0081` -frame — up to a full window from a compliant client, unbounded from one that -ignores the NACK until END — at ~90 frames/s, two log lines each -(`bleRxQueuePush()` arrival + dispatch banner), evicting the NACK itself from the -ring. That is the opposite of the diagnostic improvement Draft 2 claimed. - -Note also that `imageWriteLogQuietFrame()` is called from `bleRxQueuePush()`, -which runs on the **callback task** — so `transferActive()` is already read -cross-task. Keeping the logging predicate separate avoids widening that read to -`pipeState.error`. - -**5. Optional, same commit or its own:** make `takeConnectedEvent()` / -`takeDisconnectedEvent()` atomic read-and-clear (`__atomic_exchange_n`). The -current check-then-clear can lose a whole event, not just its payload — and a -lost disconnect means no `s_disconnectCleanupPending` *and* no -`s_advertisingRestartPending`, so the radio never re-arms. Instruction-scale -window, but a one-line fix. - -### Proof obligations - -| Term | Set by | Cleared by | Cannot latch because | -|---|---|---|---| -| `ble.eventPending()` | stack callbacks | `take*Event()` at the next loop top | The peek clears nothing; the take always runs. Continuous new events are ongoing work, not a latch | -| orphan assertion | n/a — it is the clear | itself | Runs every pass, unconditionally | - -No transfer flag enters the gate, so no transfer flag can veto sleep. That is the -point of this revision. - -### ESP32 deep sleep - -`ble.eventPending()` defers `platformIdle()` by one pass when an event lands -after `serviceBleEvents()`. That is the term's purpose and its entire effect. -Nothing else changes: no transfer state reaches the gate, `lastActivityMs` still -supplies the quiet window, and a deep-sleep wake is a full reboot, so no state -here survives it. - -### Test - -1. **`eventPending()`.** Raise a connect or disconnect after `serviceBleEvents()` - has run and before the gate is evaluated; assert the pass takes `delay(1)` and - the next pass consumes the event. On hardware, the observable is a - mid-transfer disconnect being serviced one pass later rather than after a - `CHECK_INTERVAL_MS`. -2. **Orphan assertion.** Fault-inject the orphan (clear `directWriteActive` - without resetting the pipe), then assert: one `ERROR:` line, pipe state - cleared, a subsequent `0x0081` frame rejected, and the device idling/sleeping - normally. Confirm it does *not* fire in ordinary operation — a full transfer, - a partial transfer, a fatal NACK, and a mid-transfer disconnect should each - complete with the assertion silent. -3. **Watchdog guards.** Regression only: both watchdogs still fire at 15 minutes. -4. **Log split.** Force a fatal NACK mid-stream with frames still in flight; - assert the NACK line survives in the ring and the per-frame lines stay - suppressed. -5. **nRF idle current**, disconnected, before and after. Expected unchanged; a - delta means something reaches the gate that should not. -6. **Build matrix**, all 11 environments. - ---- - -## Residuals — not in this commit - -1. **Duration, not idle.** Both watchdogs measure from START, so a genuinely slow - 15-minute push is aborted mid-flight. Converting to an idle timer (stamp on - each accepted frame) is a behaviour change deserving its own argument. -2. **A fatally-NACKed pipe session is remembered indefinitely.** `PipeWriteState` - has no timestamp, so nothing bounds it; harmless, since it is excluded from - every live-work predicate and bounded in practice by the next - START/END/disconnect. -3. **Cleanup is dropped, not deferred.** `serviceBleDisconnectCleanup()` clears - `s_disconnectCleanupPending` *before* the `ownerStillUp` test and returns, so - when the skip fires that disconnect's teardown never runs. Less severe now - that no transfer flag vetoes sleep, but still a silent drop. -4. **`sessionOrigin` is never cleared** — stamped at every START, so a refusal log - line can cite a transfer that ended long ago. -5. **nRF MSD cadence** is still coupled to the idle duration. -6. **A stale source comment** at `display_service.cpp:2771-2772` still says nRF - dispatches from its callback task; untrue since Phase 3. - -## Risk and rollback - -| Risk | Likelihood | Mitigation | -|---|---|---| -| Orphan assertion fires in normal operation | Low | Test 2's negative cases; it logs at ERROR, so a false positive is loud rather than silent | -| Dropping the `> 0` guards changes watchdog timing | None | `active` already implies a valid stamp | -| Log split leaves a case unsuppressed | Low | Test 4 | -| `eventPending()` defers sleep unexpectedly | By design, one pass | Test 5 measures the aggregate | - -Every item reverts independently. None depends on `77c2226` except the orphan -assertion, which asserts the invariant that commit established. diff --git a/docs/TEST_PLAN_UNIFY_NRF_ESP32_2026-07-27.md b/docs/TEST_PLAN_UNIFY_NRF_ESP32_2026-07-27.md deleted file mode 100644 index ba35bbc..0000000 --- a/docs/TEST_PLAN_UNIFY_NRF_ESP32_2026-07-27.md +++ /dev/null @@ -1,197 +0,0 @@ -# Bench test plan — `feat/unify-nrf-esp-phase3` - -**Status: NOT RUN.** Written 2026-07-27. Expands the eight-item bench matrix in -`PLAN_BLE_TRANSPORT_ABSTRACTION_2026-07-27.md` §7 into executable procedures and adds -coverage for the work that landed after that plan was written. - -Verification is **hardware only**. CI builds all 13 environments and executes nothing, -so a green CI says the code compiles and nothing else. Nothing on this branch has been -run on hardware. - -## 1. What is under test - -`main` (`772e9f8`) → `feat/unify-nrf-esp-phase3` (`6cc4e13`), 27 commits. - -The change with real behavioural risk is **Phase 3**: nRF command dispatch moved off -the SoftDevice callback task onto `loop()`. Everything else is either an abstraction -that preserved behaviour, logging, or a fix whose ESP32 blast radius is bounded by an -unchanged binary (§2). - -## 2. Scoping: what does NOT need retesting on ESP32 - -Three commits produced a **byte-identical** `esp32-N4` binary (998,507 B before and -after each): - -| Commit | Change | -|---|---| -| `b3b23dc` | power latch made portable | -| `7b94aa4` | ADC ladder + touch parity (B3, B4) | -| `6cc4e13` | `od_log_flush` delay on both targets | - -For those three the ESP32 regression surface is nil and testing effort belongs on nRF. -This does **not** extend to the logging commits (`1998691`, `f76693e`, `8f08ed5`, -`14b89b6`) or the race fix (`c4bd4bc`), all of which changed the ESP32 binary. - -## 3. Builds required - -```bash -pio run -e nrf52840custom-debug -t upload # nRF, DEBUG logging, USB CDC 115200 -pio run -e nrf52840custom -t upload # nRF, shipping log level -pio run -e esp32-s3-N16R8-extuart-debug -t upload # ESP32, DEBUG, CH343P UART -pio run -e esp32-N4 -t upload # ESP32, shipping, PIPE_SMALL_DRAM_WINDOW -``` - -Run functional tests on the `-debug` envs (the ERX/URX and ETX/UTX lines are -`od_log_debug` and are absent otherwise), then confirm the headline cases on the -shipping envs — the debug builds add ~13 KB and real serial time, so timing-sensitive -results must be confirmed without them. - -**Reference build for regressions: `feat/unify-nrf-esp` (`1050517`).** That is the last -commit where nRF still dispatched on the callback task, so it is the only build that -can supply "before" numbers for T7. Its nRF RAM figure is *not* comparable -(`--gc-sections` drops the unused rings there); only current and throughput are. - -## 4. Test groups - -Ordered by risk. T1 and T2 gate everything else. - -### T1 — Smoke, both targets - -| # | Step | Pass | -|---|---|---| -| T1.1 | Power on | Boot screen renders; `=== FIRMWARE INFO ===` and the git SHA log | -| T1.2 | Scan | Device advertises as `OD`, manufacturer id 9286 | -| T1.3 | Connect | `=== BLE CLIENT CONNECTED ===` then `[LINK negotiated] PHY=2M … DLE=251` at INFO | -| T1.4 | Authenticate | `Authentication successful, session established` | -| T1.5 | Full-frame push | Image renders; `RESP_DIRECT_WRITE_REFRESH_SUCCESS` reaches the client | - -T1.3 is worth its own line: `requestFastLink()` is new on ESP32 (`6891956`) and the -INFO-level link log is new on both. On ESP32 the negotiated **DLE is not reported** — -NimBLE exposes no accessor — so confirm PHY and MTU only there. - -### T2 — Core BLE, both targets (the §7 matrix) - -| # | Test | Pass | -|---|---|---| -| T2.1 | PIPE_WRITE full image | Completes; record throughput + retry count for T7 | -| T2.2 | Config read-back > 864 B | Multi-chunk read returns intact; no response-ring drops | -| T2.3 | Disconnect mid-transfer | Panel powers down, no zombie session; reconnect works | -| T2.4 | Reconnect + re-subscribe | CCCD re-enabled; notifications resume | -| T2.5 | Button + touch during transfer | See T5.2 — behaviour **changed on nRF** | -| T2.6 | Buzzer command during transfer | Plays; transfer completes | -| T2.7 | Partial write (0x0076) | Rect updates; ETAG committed | -| T2.8 | ESP32 deep-sleep / wake cycle | Wakes, reconnects, `Deep sleep count` increments | - -### T3 — Phase 3 execution model (nRF only, highest risk) - -The threading contract: stack callbacks may only copy bytes into the RX ring and set a -flag; everything else runs on `loop()`. - -| # | Test | Pass | -|---|---|---| -| T3.1 | Sustained PIPE_WRITE at full window | No `Command queue full` at any point | -| T3.2 | Command latency | Response follows command within ~100 ms (the `idleDelay` chunk); no multi-second stalls | -| T3.3 | Command during `idleDelay` | `idleDelay()` returns early on pending RX — no 500 ms floor | -| T3.4 | Ring depth under load | RX `[Q:n]` stays ≪ `PIPE_MAX_W + 2`; TX `[Q:n]` returns to 0 between commands | -| T3.5 | Rapid connect/disconnect ×20 | No hang, no leaked session, advertising always resumes | - -T3.4 is the direct readout of whether the derived ring depth is right. A TX `Q` that -climbs monotonically means the drain is behind the producer; an RX `Q` that climbs -means arrivals are outrunning `loop()`. - -### T4 — The reconnect race (`c4bd4bc`) — both targets - -This reproduces a defect actually observed on nRF on 2026-07-27, so it is a regression -test with a known-failing predecessor, not a hypothetical. - -**T4.1 — stale disconnect must not eat the next client's frames** -1. Start a full-frame push to a Spectra 6-colour panel (~16 s refresh). -2. While `Refresh took …` has not yet printed, **disconnect** client A. -3. Still inside the refresh, **connect** client B and send one command (e.g. `0x0080`). -4. Wait for the refresh to finish. - -**Pass:** B's command dispatches. `Dropped N queued command(s)` either does not appear -or reports only A's frames. **Fail (pre-fix behaviour):** `Disconnect reason: 19` and -`Dropped 1 queued command(s)` print *after* `=== BLE CLIENT CONNECTED ===`, and B's -command never dispatches. - -**T4.2 — cleanup must not tear down the new client's session.** Same setup, but B -starts a PIPE_WRITE before the refresh ends. **Pass:** `Disconnect cleanup skipped: -transfer still owned by a live BLE session`, and B's transfer completes. This path was -unguarded on nRF before `c4bd4bc` (the guard sat inside `#ifdef OPENDISPLAY_HAS_WIFI`). - -**T4.3 — the ordinary case still flushes.** Disconnect mid-transfer with no reconnect. -**Pass:** `Dropped N queued command(s)` reports the frames A actually left. - -### T5 — Parity fixes (nRF only; ESP32 binaries unchanged) - -**T5.1 — power latch (`b3b23dc`).** No nRF board has the hardware. Confirm only that a -config *without* `DEVICE_FLAG_BATTERY_LATCH` / `DEVICE_FLAG_PWR_LATCH_DFF` behaves -exactly as before: no spurious power-off, `0x0052` still NACKs. On ESP32 latch -hardware, re-run press-and-hold power-off and the D-FF rail cut — the refactor touched -those call sequences even though the binary did not change. - -**T5.2 — touch suspension during transfers (B4, `7b94aa4`).** *Behaviour change on -nRF.* During a transfer or refresh, touch must **stop** responding and resume after. -Compare pipe-write throughput with and without continuous touch input; it should no -longer degrade. Confirm touch is not left permanently disabled after a failed or -aborted transfer. - -**T5.3 — ADC ladder (B3, `7b94aa4`).** No nRF ladder hardware exists. Confirm the -negative case: a config declaring `BINARY_INPUT_TYPE_ADC_LADDER` must log -`ADC ladder: pin …` and must **not** attach a digital-button interrupt to that pin -(pre-fix, the `continue` was compiled out and it did). On ESP32, re-run ladder button -detection to confirm `adcLadderConfigurePin()` is equivalent to the old inline -attenuation call. - -### T6 — Logging correctness (`-debug` envs) - -| # | Test | Pass | -|---|---|---| -| T6.1 | Every command | Exactly one `ERX`/`URX` line, then the banner, then one `ETX`/`UTX` line | -| T6.2 | nRF parity | nRF emits RX lines at all (it emitted none before `8f08ed5`) and carries `[Q:n]` | -| T6.3 | Encryption token | Authenticated traffic reads `ERX`/`ETX`; handshake (0x0050/0x000A) reads `URX`/`UTX` | -| T6.4 | Mid-stream quiet | 0x0071/0x0081 frames log chunk 1 then go silent; no per-frame spam | -| T6.5 | Oversize frame | Logs `Command too large for queue`, **not** `queue full` (the nRF misreport fixed in `8f08ed5`) | -| T6.6 | Decrypt failure | `Decryption failed (0x…, N B payload, nonce …)` — nonce present on the failure path only | - -T6.5 is the specific pre-fix misdiagnosis: nRF reported all three push failures as -"queue full", pointing at ring depth for a malformed frame. - -### T7 — Power and throughput regressions (nRF) - -The two baselines still uncaptured. Measure on `feat/unify-nrf-esp` (`1050517`) first, -then on this branch, on the same hardware and battery. - -| # | Metric | Threshold | -|---|---|---| -| T7.1 | Battery idle current, advertising, no client | No material rise. `loop()` spins only while `workInFlight`; a persistent rise means a term is stuck true | -| T7.2 | PIPE_WRITE throughput | Within noise of the reference. Each ACK now costs a loop pass | -| T7.3 | Retry / NACK count | No increase | - -T7.1 is the one Phase 3 most plausibly regresses, and the one no amount of code reading -settles. - -## 5. Not covered, and why - -| Item | Reason | -|---|---| -| nRF power latch on real hardware | No such board exists | -| nRF ADC ladder classification | No such board exists; thresholds need per-board calibration and reference voltages differ from ESP32 | -| The 16 s refresh stall itself | Out of scope — see `DESIGN_COOPERATIVE_REFRESH_WAIT_2026-07-27.md`. T4 tests that deferred work is *correct when late*, not that it stops being late | -| Audit findings M2, L1, L2, L3, L5 | Unreviewed; `AUDIT_FIRMWARE_2026-07-13.md` is demonstrably stale (M1 and L4 were already fixed) | -| `esp32-s3-E1004` / FastEPD paths | Needs the reTerminal E1004 panel | - -## 6. Exit criteria - -Ship when: - -1. T1–T4 pass on **both** targets. -2. T5 passes on nRF, and T5.1/T5.3's ESP32 halves pass on latch/ladder hardware. -3. T6 passes on both `-debug` envs. -4. T7 shows no material regression against `feat/unify-nrf-esp`. -5. Anything that fails is either fixed or recorded here with a decision. - -**Rollback:** phases are independent commits. Phase 3 is the only one that changes nRF -runtime behaviour and can be reverted alone, leaving the abstraction (Phases 1–2) in -place. `c4bd4bc` depends on Phase 3 and must revert with it.