diff --git a/.gitignore b/.gitignore index 8eab5b1..c7ea016 100644 --- a/.gitignore +++ b/.gitignore @@ -1,7 +1,7 @@ ### C++ template # Prerequisites *.d -./*.uf2 +/*.uf2 # Compiled Object files *.slo *.lo diff --git a/docs/AUDIT_FIRMWARE_2026-07-13.md b/docs/AUDIT_FIRMWARE_2026-07-13.md index a2069c9..3f761fc 100644 --- a/docs/AUDIT_FIRMWARE_2026-07-13.md +++ b/docs/AUDIT_FIRMWARE_2026-07-13.md @@ -34,6 +34,16 @@ Counts: CRITICAL 0, HIGH 0, MEDIUM 2, LOW 5. ## M1 — 4 KB stack buffer in the BLE write-callback context (nRF) +- **STATUS: RESOLVED**, on both of its independent counts. + 1. The buffer itself is gone: `handleReadConfig` now takes its scratch from + `getConfigScratch()` rather than declaring `uint8_t configData[4096]` on the + stack. This landed separately from the BLE work. + 2. The context is gone: Phase 3 of + `docs/PLAN_BLE_TRANSPORT_ABSTRACTION_2026-07-27.md` moved nRF command + dispatch off the Bluefruit callback task onto `loop()`, so no command + handler runs on the small callback stack any more. This class of finding — + "handler X is unsafe because of the stack it runs on" — is now structurally + impossible on both targets. - **Severity:** MEDIUM (latent stack overflow / crash) - **Location:** `src/communication.cpp:352` (`handleReadConfig`), reached from `imageDataWritten` case `0x0040` (`communication.cpp:584-589`). @@ -209,6 +219,21 @@ before `bbepWriteData`, and fail if the stream still has more to emit. ## L4 — Buzzer playback busy-waits up to ~5 s, starving the main loop +- **STATUS: RESOLVED**, independently of the BLE transport work and before it. + The software square-wave busy-loop is gone: `buzzer_drive_tone_sw` no longer + exists. Tone generation is now hardware PWM (`buzzer_hw_tone_start` in + `src/buzzer_hw.cpp` — nRF PWM peripheral, ESP32 LEDC), and playback is a + millis()-poll state machine (`buzzer_run` / `buzzerService` over `s_buzzer`, + `buzzer_control.cpp`) driven from `loop()` and `idleDelay()`. Nothing blocks: + `buzzerService()` returns immediately while a step is still timing out. The + 5 s cap survives as `kBuzzerMaxTotalMs`, now enforced across polls rather than + inside a spin. + + Note for anyone tracing this finding's history: a Phase 5 note briefly claimed + L4 was still open and made worse by moving nRF dispatch to `loop()`. That was + wrong — it reasoned from this document's original text without checking the + code, which had already been rewritten. The reasoning would have been correct + had the busy-wait still existed. - **Severity:** LOW (bounded; degrades BLE latency / keep-alive during a tone) - **Location:** `src/buzzer_control.cpp:71-78` (`buzzer_drive_tone_sw`), driven by `handleBuzzerActivate` (buzzer_control.cpp:148-175). diff --git a/docs/DESIGN_COOPERATIVE_REFRESH_WAIT_2026-07-27.md b/docs/DESIGN_COOPERATIVE_REFRESH_WAIT_2026-07-27.md new file mode 100644 index 0000000..65cdaad --- /dev/null +++ b/docs/DESIGN_COOPERATIVE_REFRESH_WAIT_2026-07-27.md @@ -0,0 +1,180 @@ +# 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/FIRMWARE_NIMBLE_PORT_CODE_REVIEW_2026-07-17.md b/docs/FIRMWARE_NIMBLE_PORT_CODE_REVIEW_2026-07-17.md index c4b27f4..7aba237 100644 --- a/docs/FIRMWARE_NIMBLE_PORT_CODE_REVIEW_2026-07-17.md +++ b/docs/FIRMWARE_NIMBLE_PORT_CODE_REVIEW_2026-07-17.md @@ -244,6 +244,13 @@ A second, independent cluster of findings is in the **encryption/session layer** - **Confidence:** High that the length byte is unused; Med on real-world triggerability (*verify against the wire format the toolbox emits*). ### #20 — Config reload mutates `globalConfig` in the BLE callback task while `loop()` reads it (nRF only) +- **STATUS: RESOLVED 2026-07-27** by Phase 3 of + `docs/PLAN_BLE_TRANSPORT_ABSTRACTION_2026-07-27.md`, which applied this + finding's own first proposed solution: nRF now marshals command processing to + the main loop as ESP32 does. The nRF write callback pushes onto the shared RX + ring and `serviceBleRx()` dispatches from `loop()`, so `loadGlobalConfig()` and + every `globalConfig` reader run on one task. No critical section or + double-buffer swap was needed. `src/ble_init.cpp` no longer exists. - **Location:** `src/config_parser.cpp:263-264` (`memset`/repopulate in `loadGlobalConfig`), reached via `handleWriteConfig`/`reloadConfigAfterSave`; nRF dispatch at `src/ble_init.cpp:160` - **Category:** race - **Provenance:** Pre-existing (Bluefruit path), **not** the NimBLE port — but in-scope for the callback-vs-loop concern. diff --git a/docs/PLAN_BLE_TRANSPORT_ABSTRACTION_2026-07-27.md b/docs/PLAN_BLE_TRANSPORT_ABSTRACTION_2026-07-27.md new file mode 100644 index 0000000..18ff196 --- /dev/null +++ b/docs/PLAN_BLE_TRANSPORT_ABSTRACTION_2026-07-27.md @@ -0,0 +1,340 @@ +# 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_UNIFY_NRF_ESP32_LOOP_BLE_2026-07-27.md b/docs/PLAN_UNIFY_NRF_ESP32_LOOP_BLE_2026-07-27.md new file mode 100644 index 0000000..8d353e0 --- /dev/null +++ b/docs/PLAN_UNIFY_NRF_ESP32_LOOP_BLE_2026-07-27.md @@ -0,0 +1,252 @@ +# 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/TEST_PLAN_UNIFY_NRF_ESP32_2026-07-27.md b/docs/TEST_PLAN_UNIFY_NRF_ESP32_2026-07-27.md new file mode 100644 index 0000000..ba35bbc --- /dev/null +++ b/docs/TEST_PLAN_UNIFY_NRF_ESP32_2026-07-27.md @@ -0,0 +1,197 @@ +# 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. diff --git a/docs/pipe-write-protocol.md b/docs/pipe-write-protocol.md index 527ed10..a67857e 100644 --- a/docs/pipe-write-protocol.md +++ b/docs/pipe-write-protocol.md @@ -282,11 +282,27 @@ and `back > W`) is a protocol violation → NACK `0x04`. ### 3.4 ESP32 ingest ring (transport, not protocol) -On ESP32 the BLE callback copies each command into a lock-free SPSC ring -(`COMMAND_QUEUE_SIZE = 33`, `MAX_COMMAND_SIZE = 256`) with acquire/release -atomics; the main loop drains up to 33 per pass and flushes responses **between** -commands so small ACK cadences can't overflow the response ring. Sized to hold a -full W=32 window plus END across an SPI stall. +On **both** targets (nRF since the Phase 3 transport work, ESP32 since the NimBLE +port) the BLE callback copies each command into a lock-free SPSC ring with +acquire/release atomics; the main loop drains up to `COMMAND_QUEUE_SIZE` per pass +and flushes responses **between** commands so small ACK cadences can't overflow +the response ring. + +Depth is **derived**, not hardcoded: `COMMAND_QUEUE_SIZE = PIPE_MAX_W + 2` +(`src/command_queue.h`, with a `static_assert` enforcing it). The ring reserves +one slot to distinguish full from empty, so usable capacity is `SLOTS - 1`, and +the worst case it must absorb across an SPI stall is a full `PIPE_MAX_W` window +of DATA **plus END** — END carries no `seq`, sits outside the window's sequence +space, and is therefore not bounded by the sender's window rule. That gives 34 +slots at `PIPE_MAX_W = 32`, and 18 on `env:esp32-N4` +(`PIPE_SMALL_DRAM_WINDOW`, `PIPE_MAX_W = 16`). + +> This constant read a hardcoded `33` until 2026-07-27, which yielded 32 usable +> slots — one short of the "full window plus END" case it was documented to +> cover, so the END was rejected at exactly the stall it was sized for. Note that +> `PIPE_REORDER_SLOTS = W + 1` is *correct* for its own purpose (collision-free +> `seq % SLOTS` indexing); the two constants have different rationales and must +> not be kept equal by habit. --- diff --git a/platformio.ini b/platformio.ini index 5a6451f..1f5dae2 100644 --- a/platformio.ini +++ b/platformio.ini @@ -1,6 +1,29 @@ [platformio] extra_configs = platformio.local.ini +; The 11 shipping environments, and the only ones a bare `pio run` builds. This +; list must match the matrix in .github/workflows/main.yaml and the per-env +; builds in .github/workflows/release.yml -- both of those name every env +; explicitly, so the diagnostic envs below can never reach CI or a release +; artifact. default_envs closes the remaining hole: a local or scripted +; `pio run` with no -e would otherwise build all of them. +; +; DIAGNOSTIC ENVS ARE DELIBERATELY ABSENT: nrf52840custom-debug, +; nrf52840-bootdiag, nrf52840-reformat, esp32-s3-N16R8-extuart-debug. +; Build them with an explicit `pio run -e `. See each env's own header for +; why it must never ship. +default_envs = + nrf52840custom + esp32-s3-N16R8 + esp32-s3-N8R8 + esp32-s3-N32R8 + esp32-s3-N32R8-extuart + esp32-s3-N16R8-extuart + esp32-s3-E1004 + esp32-c3-N4 + esp32-c3-N16 + esp32-c6-N4 + esp32-N4 [env] lib_deps = @@ -8,6 +31,9 @@ lib_deps = h2zero/NimBLE-Arduino@^2.5.0 extra_scripts = pre:scripts/factory_config_gen.py +build_src_filter = + +<*> + - ; Factory provisioning (scripts/factory_config_gen.py): ; OPENDISPLAY_FACTORY_CONFIG_HEX="BD 00 01 ..." pio run -e @@ -31,10 +57,61 @@ board_build.variants_dir = variants board_build.variant = nrf52840custom board = xiaoblesense_adafruit monitor_speed = 115200 -build_src_filter = +<*> - +build_src_filter = +<*> - - ; NimBLE-Arduino is ESP32-only here; the nRF target uses Adafruit Bluefruit. lib_ignore = NimBLE-Arduino +; nrf52840custom with DEBUG-level logging compiled in -- the nRF counterpart of +; esp32-s3-N16R8-extuart-debug. OD_LOG_LEVEL defaults to INFO, so every +; od_log_debug() is dead-code-eliminated and the config parse dump, the BLE RX/TX +; hex lines with their [BLE][Q:n] queue depth, and the DIRECT_WRITE progress / +; throughput lines ("DW complete: ... KB/s") are ABSENT from a normal build. This +; env turns them on. Debug build, not a shipping one: the strings and calls survive, +; and the serial time they cost is real. +; +; Output goes to the nRF52840 NATIVE USB CDC (Serial) at 115200. There is no +; OPENDISPLAY_LOG_UART path on this target -- main.cpp gates that on TARGET_ESP32 -- +; so unlike the S3 extuart pair there is no separate wire to watch: a battery build +; with no USB host attached emits nothing. +; +; Headroom is not a concern: the INFO build sits at 254 KB of 811 KB flash. +; +; Not in .github/workflows/main.yaml, matching esp32-s3-N16R8-extuart-debug -- CI +; builds the 11 shipping envs only. Low bit-rot risk by construction: od_log_debug() +; is a runtime `if` on a compile-time constant, not an #if, so every debug format +; string is still type-checked by the 11 CI builds. +[env:nrf52840custom-debug] +extends = env:nrf52840custom +build_flags = + ${env:nrf52840custom.build_flags} + -DOD_LOG_LEVEL=OD_LOG_DEBUG + +; NEVER SHIP. Destructive recovery image: erases the internal LittleFS region +; (0xED000-0xF4000) on every boot and skips loading configuration. Boot it once, +; then flash normal firmware. Links none of the OpenDisplay sources -- it is a +; standalone utility that happens to share this project's toolchain. +; Excluded from default_envs, CI and release; build with -e explicitly. +[env:nrf52840-reformat] +extends = env:nrf52840custom +lib_deps = + Adafruit TinyUSB Library +build_src_filter = + -<*> + + + +; NEVER SHIP. Full normal firmware with an earliest-setup USB gate and staged +; boot logs, to distinguish pre-setup faults from config, I/O and SoftDevice +; faults. OPENDISPLAY_BOOT_DIAG makes setup() BLOCK on `while (!Serial)` with no +; timeout: with no USB host attached the device never advertises, never drives +; the panel, and never reaches loop(). That is the point on a bench and fatal on +; a battery. Excluded from default_envs, CI and release; build with -e explicitly. +[env:nrf52840-bootdiag] +extends = env:nrf52840custom +build_flags = + ${env:nrf52840custom.build_flags} + -DOD_LOG_LEVEL=OD_LOG_DEBUG + -DOPENDISPLAY_BOOT_DIAG=1 + [env:esp32-s3-N16R8] platform = https://github.com/pioarduino/platform-espressif32/releases/download/55.03.39/platform-espressif32.zip diff --git a/src/ble_init.cpp b/src/ble_init.cpp deleted file mode 100644 index 53dcf86..0000000 --- a/src/ble_init.cpp +++ /dev/null @@ -1,318 +0,0 @@ -#include "ble_init.h" -#include "structs.h" -#include "encryption.h" -#include "display_service.h" -#include "od_log.h" - -#ifdef TARGET_NRF -#include -extern "C" { -#include "nrf_soc.h" -} -extern BLEDfu bledfu; -extern BLEService imageService; -extern BLECharacteristic imageCharacteristic; -extern struct GlobalConfig globalConfig; -void connect_callback(uint16_t conn_handle); -void disconnect_callback(uint16_t conn_handle, uint8_t reason); -void imageDataWritten(uint16_t conn_hdl, BLECharacteristic* chr, uint8_t* data, uint16_t len); -String getChipIdHex(); -#endif - -#ifdef TARGET_ESP32 -// NimBLE-Arduino + BLE* aliases arrive via ble_init.h (included above). -String getChipIdHex(); -#include "esp32_ble_callbacks.h" - -extern struct GlobalConfig globalConfig; -extern BLEServer* pServer; -extern BLEService* pService; -extern BLECharacteristic* pTxCharacteristic; -extern BLECharacteristic* pRxCharacteristic; -extern BLEAdvertisementData* advertisementData; -extern MyBLEServerCallbacks staticServerCallbacks; -extern MyBLECharacteristicCallbacks staticCharCallbacks; -#endif - -#ifdef TARGET_NRF -static uint32_t s_nrf_adv_boost_until = 0; - -static constexpr uint16_t NRF_ADV_INTERVAL_MIN = 256; // 160 ms -static constexpr uint16_t NRF_ADV_INTERVAL_MAX = 1600; // 1000 ms -static constexpr uint16_t NRF_ADV_BOOST_MIN = 32; // 20 ms -static constexpr uint16_t NRF_ADV_BOOST_MAX = 48; // 30 ms -static constexpr uint32_t NRF_ADV_BOOST_MS = 3000; - -void ble_nrf_boost_advertising(void) { - s_nrf_adv_boost_until = millis() + NRF_ADV_BOOST_MS; -} - -void ble_nrf_apply_adv_interval(void) { - if (s_nrf_adv_boost_until != 0 && millis() < s_nrf_adv_boost_until) { - Bluefruit.Advertising.setInterval(NRF_ADV_BOOST_MIN, NRF_ADV_BOOST_MAX); - } else { - s_nrf_adv_boost_until = 0; - Bluefruit.Advertising.setInterval(NRF_ADV_INTERVAL_MIN, NRF_ADV_INTERVAL_MAX); - } -} - -void ble_nrf_advertising_tick(void) { - static bool was_boosted = false; - const bool boosting = (s_nrf_adv_boost_until != 0 && millis() < s_nrf_adv_boost_until); - if (boosting) { - was_boosted = true; - return; - } - if (!was_boosted || !Bluefruit.Advertising.isRunning()) { - was_boosted = false; - s_nrf_adv_boost_until = 0; - return; - } - was_boosted = false; - s_nrf_adv_boost_until = 0; - Bluefruit.Advertising.setInterval(NRF_ADV_INTERVAL_MIN, NRF_ADV_INTERVAL_MAX); - Bluefruit.Advertising.stop(); - Bluefruit.Advertising.start(0); -} - -// --- Link-layer diagnostics ------------------------------------------------- -// DLE (Data Length Extension) sets the max Link-Layer PDU payload: 27 octets by -// default, up to 251 once negotiated. The nRF peripheral only auto-accepts the -// central's request, which arrives AFTER connect_callback, so we log twice: once -// at connect (baseline) and once ~2.5 s later (negotiated). -void ble_nrf_log_link_params(uint16_t conn_handle, const char* phase) { - BLEConnection* conn = Bluefruit.Connection(conn_handle); - if (conn == nullptr) { - od_log_debug("[LINK %s] no connection (handle %u)", phase, conn_handle); - return; - } - uint8_t phy = conn->getPHY(); - uint16_t mtu = conn->getMtu(); // ATT MTU (23 default; 247 cap here) - uint16_t dle = conn->getDataLength(); // LL PDU payload octets (27 default; 251 max) - uint16_t ci = conn->getConnectionInterval(); // units of 1.25 ms - const char* phyStr = (phy == BLE_GAP_PHY_2MBPS) ? "2M" : - (phy == BLE_GAP_PHY_1MBPS) ? "1M" : "?"; - od_log_debug("[LINK %s] PHY=%s ATT_MTU=%u DLE=%u octets connInterval=%.2f ms", - phase, phyStr, mtu, dle, ci * 1.25f); -} - -// One-shot timer (armed only in connect_callback — no per-loop polling). Fires -// once on the FreeRTOS timer task after the central finishes negotiation. -static SoftwareTimer s_link_diag_timer; -static uint16_t s_link_diag_conn = BLE_CONN_HANDLE_INVALID; - -static void ble_nrf_link_diag_cb(TimerHandle_t /*xTimer*/) { - if (Bluefruit.connected()) { - ble_nrf_log_link_params(s_link_diag_conn, "negotiated"); - } -} - -// Proactively upgrade the link for throughput: the nRF peripheral only auto-accepts -// the central's PHY/DLE requests, so if the phone never asks we stay at 1M / 27 octets. -// Requesting here (both are no-ops if the peer already negotiated the same or better). -void ble_nrf_request_fast_link(uint16_t conn_handle) { - BLEConnection* conn = Bluefruit.Connection(conn_handle); - if (conn == nullptr) return; - - // 2 Mbps PHY (tx + rx). Peer may decline and stay at 1M. - conn->requestPHY(BLE_GAP_PHY_2MBPS); - - // 251-octet Link-Layer PDUs (max DLE). AUTO time lets the controller derive the - // PHY-appropriate on-air duration. - ble_gap_data_length_params_t dl; - dl.max_tx_octets = 251; - dl.max_rx_octets = 251; - dl.max_tx_time_us = BLE_GAP_DATA_LENGTH_AUTO; - dl.max_rx_time_us = BLE_GAP_DATA_LENGTH_AUTO; - ble_gap_data_length_limitation_t limit = { 0, 0, 0 }; - if (!conn->requestDataLengthUpdate(&dl, &limit)) { - od_log_warn("DLE 251 request rejected (tx_lim=%u rx_lim=%u time_lim_us=%u)", - limit.tx_payload_limited_octets, limit.rx_payload_limited_octets, - limit.tx_rx_time_limited_us); - } - od_log_debug("Requested fast link: 2M PHY + 251-octet DLE"); -} - -void ble_nrf_arm_link_diag(uint16_t conn_handle) { - s_link_diag_conn = conn_handle; - static bool created = false; - if (!created) { - // Create the one-shot (repeating=false) on the first connection only. - s_link_diag_timer.begin(500, ble_nrf_link_diag_cb, NULL, false); - created = true; - } - s_link_diag_timer.reset(); // start/restart the one-shot from now; fires ~2.5 s later -} - -void ble_nrf_stack_init() { - Bluefruit.configCentralBandwidth(BANDWIDTH_MAX); - Bluefruit.configPrphBandwidth(BANDWIDTH_MAX); - Bluefruit.autoConnLed(false); - Bluefruit.setTxPower(globalConfig.power_option.tx_power); - Bluefruit.begin(1, 0); - od_log_info("BLE initialized successfully"); - od_log_info("Setting up BLE service 0x2446..."); - imageService.begin(); - od_log_info("BLE service started"); - imageCharacteristic.setWriteCallback(imageDataWritten); - od_log_info("BLE write callback set"); - imageCharacteristic.begin(); - od_log_info("BLE characteristic started"); - // Register the DFU service LAST so its presence/absence (it is only added when - // encryption is disabled) never shifts the handles of imageCharacteristic and its - // CCCD. GATT handles are assigned in begin() order; keeping the app characteristic - // ahead of the conditional DFU service keeps its handles stable across encryption - // on/off, so a client's cached CCCD handle stays valid and notify setup won't fail - // with ATT "Invalid handle". Must stay after Bluefruit.begin() (SoftDevice up first). - if (!isEncryptionEnabled()) { - bledfu.begin(); - od_log_info("BLE DFU initialized successfully (encryption disabled)"); - } else { - od_log_info("BLE DFU service NOT initialized (encryption enabled - use CMD_ENTER_DFU)"); - } - Bluefruit.Periph.setConnectCallback(connect_callback); - Bluefruit.Periph.setDisconnectCallback(disconnect_callback); - od_log_info("BLE callbacks registered"); - String deviceName = "OD" + getChipIdHex(); - Bluefruit.setName(deviceName.c_str()); - od_log_info("Device name set to: %s", deviceName.c_str()); - od_log_info("Configuring power management..."); - sd_power_mode_set(NRF_POWER_MODE_LOWPWR); - sd_power_dcdc_mode_set(NRF_POWER_DCDC_ENABLE); - od_log_info("Power management configured"); -} - -void ble_nrf_advertising_start() { - od_log_info("Configuring BLE advertising..."); - Bluefruit.Advertising.clearData(); - Bluefruit.Advertising.addFlags(BLE_GAP_ADV_FLAGS_LE_ONLY_GENERAL_DISC_MODE); - Bluefruit.Advertising.addName(); - updatemsdata(); - Bluefruit.Advertising.restartOnDisconnect(true); - ble_nrf_apply_adv_interval(); - Bluefruit.Advertising.setFastTimeout(10); - od_log_info("Starting BLE advertising..."); - Bluefruit.Advertising.start(0); -} -#endif - -#ifdef TARGET_ESP32 -void ble_init() { - ble_init_esp32(true); -} -#endif - -#ifdef TARGET_ESP32 -volatile bool bleRestartAdvertisingPending = false; -volatile bool esp32BleNotifySubscribed = false; -volatile bool bleDisconnectCleanupPending = false; -volatile bool msdUpdatePending = false; - -void esp32_ble_clear_handles(void) { - pServer = nullptr; - pService = nullptr; - pTxCharacteristic = nullptr; - pRxCharacteristic = nullptr; - esp32BleNotifySubscribed = false; -} - -bool esp32_ble_notify_enabled(void) { - if (pTxCharacteristic == nullptr || pServer == nullptr || pServer->getConnectedCount() == 0) { - return false; - } - // NimBLE auto-creates the 0x2902 CCCD; onSubscribe tracks the client's toggle. - return esp32BleNotifySubscribed; -} - -void esp32_restart_ble_advertising(void) { - if (pServer == nullptr) { - bleRestartAdvertisingPending = true; - return; - } - if (pServer->getConnectedCount() > 0) { - bleRestartAdvertisingPending = false; - return; - } - if (epdRefreshInProgress) { - bleRestartAdvertisingPending = true; - return; - } - bleRestartAdvertisingPending = false; - delay(100); - BLEDevice::startAdvertising(); - updatemsdata(); - od_log_info("BLE advertising restarted"); -} - -void ble_init_esp32(bool update_manufacturer_data) { - esp32_ble_clear_handles(); - od_log_info("=== Initializing ESP32 BLE ==="); - String deviceName = "OD" + getChipIdHex(); - od_log_info("Device name will be: %s", deviceName.c_str()); - BLEDevice::init(deviceName.c_str()); - // Preferred only: the central drives the exchange and may settle lower. - od_log_info("Setting preferred BLE ATT MTU to %u...", (unsigned)OD_BLE_PREFERRED_ATT_MTU); - BLEDevice::setMTU(OD_BLE_PREFERRED_ATT_MTU); - pServer = BLEDevice::createServer(); - if (pServer == nullptr) { - od_log_error("ERROR: Failed to create BLE server"); - return; - } - // deleteCallbacks=false: staticServerCallbacks is a static object; NimBLE must - // not delete it on deinit()/replacement. - pServer->setCallbacks(&staticServerCallbacks, false); - od_log_info("Server callbacks configured"); - BLEUUID serviceUUID("00002446-0000-1000-8000-00805F9B34FB"); - pService = pServer->createService(serviceUUID); - if (pService == nullptr) { - od_log_error("ERROR: Failed to create BLE service"); - return; - } - od_log_info("BLE service 0x2446 created successfully"); - BLEUUID charUUID("00002446-0000-1000-8000-00805F9B34FB"); - // Declaring max_len (rather than inheriting NimBLE's 512 default) makes the GATT - // layer reject an oversize write with ATT 0x0D instead of letting it reach onWrite() - // and be dropped silently by the MAX_COMMAND_SIZE check. - pTxCharacteristic = pService->createCharacteristic( - charUUID, - NIMBLE_PROPERTY::READ | - NIMBLE_PROPERTY::NOTIFY | - NIMBLE_PROPERTY::WRITE | - NIMBLE_PROPERTY::WRITE_NR, - OD_BLE_MAX_FRAME - ); - if (pTxCharacteristic == nullptr) { - od_log_error("ERROR: Failed to create BLE characteristic"); - return; - } - od_log_info("Characteristic created with properties: READ, NOTIFY, WRITE, WRITE_NR"); - // NimBLE auto-adds the 0x2902 CCCD for NOTIFY characteristics; no BLE2902 needed. - pTxCharacteristic->setCallbacks(&staticCharCallbacks); - pRxCharacteristic = pTxCharacteristic; - // NimBLE starts services automatically when the server starts advertising; no - // explicit pService->start() (deprecated no-op in NimBLE 2.x). - BLEAdvertising* pAdvertising = BLEDevice::getAdvertising(); - if (pAdvertising == nullptr) { - od_log_error("ERROR: Failed to get advertising object"); - return; - } - pAdvertising->addServiceUUID(serviceUUID); - od_log_info("Service UUID added to advertising"); - advertisementData->setName(deviceName.c_str()); - advertisementData->setFlags(0x06); - od_log_info("Device name added to advertising"); - if (update_manufacturer_data) { - updatemsdata(); - } - // setAdvertisementData() must be the LAST advertising-data call before start(): - // NimBLE's enableScanResponse()/setPreferredParams()/etc. reset the internal - // "custom data set" flag, which would make start() discard this payload and - // re-advertise the piecemeal builder (no manufacturer data / no name). Scan - // response is off by default in NimBLE 2.x, so no enableScanResponse() needed. - pAdvertising->setAdvertisementData(*advertisementData); - pServer->getAdvertising()->start(); - od_log_info("=== BLE advertising started successfully ==="); - od_log_info("Device ready: %s", deviceName.c_str()); - od_log_info("Waiting for BLE connections..."); -} -#endif diff --git a/src/ble_init.h b/src/ble_init.h deleted file mode 100644 index fc85e75..0000000 --- a/src/ble_init.h +++ /dev/null @@ -1,45 +0,0 @@ -#ifndef BLE_INIT_H -#define BLE_INIT_H - -#include - -#ifdef TARGET_NRF -void ble_nrf_stack_init(); -void ble_nrf_advertising_start(); -void ble_nrf_boost_advertising(void); -void ble_nrf_apply_adv_interval(void); -void ble_nrf_advertising_tick(void); -// Link diagnostics: log negotiated PHY / ATT MTU / DLE (LL PDU octets) / conn interval. -void ble_nrf_log_link_params(uint16_t conn_handle, const char* phase); -void ble_nrf_arm_link_diag(uint16_t conn_handle); // one-shot: re-log ~2.5 s after connect -void ble_nrf_request_fast_link(uint16_t conn_handle); // request 2M PHY + 251-octet DLE (max throughput) -#endif -#ifdef TARGET_ESP32 -// ESP32 BLE is backed by NimBLE-Arduino (h2zero). The aliases below let the rest -// of the firmware keep the historical BLE* spellings so only the genuinely -// changed NimBLE 2.x call sites need edits. -#include -using BLEDevice = NimBLEDevice; -using BLEServer = NimBLEServer; -using BLEService = NimBLEService; -using BLECharacteristic = NimBLECharacteristic; -using BLEAdvertising = NimBLEAdvertising; -using BLEAdvertisementData = NimBLEAdvertisementData; -using BLEServerCallbacks = NimBLEServerCallbacks; -using BLECharacteristicCallbacks = NimBLECharacteristicCallbacks; -using BLEUUID = NimBLEUUID; - -void ble_init(); -void ble_init_esp32(bool update_manufacturer_data = true); -void esp32_restart_ble_advertising(void); -void esp32_ble_clear_handles(void); -bool esp32_ble_notify_enabled(void); -extern volatile bool bleRestartAdvertisingPending; -extern volatile bool esp32BleNotifySubscribed; -// Set flag-only by MyBLEServerCallbacks (NimBLE host task); serviced from loop() -// so the heavyweight teardown / I2C+advertisement work never races loop(). -extern volatile bool bleDisconnectCleanupPending; -extern volatile bool msdUpdatePending; -#endif - -#endif diff --git a/src/ble_transport.h b/src/ble_transport.h new file mode 100644 index 0000000..e324269 --- /dev/null +++ b/src/ble_transport.h @@ -0,0 +1,84 @@ +#ifndef BLE_TRANSPORT_H +#define BLE_TRANSPORT_H + +#include + +// Portable BLE link abstraction. Deliberately includes NO stack headers, so any +// translation unit can use it without dragging in Bluefruit or NimBLE types. +// +// Exactly one implementation is linked per build (ble_transport_nrf.cpp or +// ble_transport_esp32.cpp), so this is a plain class rather than an abstract +// base: virtual dispatch would cost a vtable and indirect calls for zero +// benefit, and application code would still only ever see one type. +// +// Threading, as of Phase 1: the callback contract is NOT yet symmetric. ESP32 +// stack callbacks are flag-only (they copy into the RX ring and set a flag); +// nRF still dispatches commands inline on the SoftDevice callback task and runs +// the app connect/disconnect hooks there. Phase 3 makes nRF match ESP32. Until +// then the asymmetry lives entirely inside the two implementation files -- see +// docs/PLAN_BLE_TRANSPORT_ABSTRACTION_2026-07-27.md. +class BleTransport { +public: + // --- lifecycle --- + // begin()/startAdvertising() are separate calls so each target keeps its own + // init ordering: nRF must bring the SoftDevice up BEFORE display/SPI and only + // advertise after the boot screen, whereas ESP32 inits BLE after the display. + bool begin(const char* deviceName); + void startAdvertising(); + // Unconditional: brings advertising back up now. Deferral policy (mid-EPD + // refresh, still connected, stack not up) belongs to the caller, not here. + void restartAdvertising(); + void stopAdvertising(); + void end(); // full teardown; no-op on nRF + + // --- state --- + bool isReady() const; // stack initialised and usable + uint8_t connectedCount() const; + bool isConnected() const { return connectedCount() > 0; } + bool notifyReady() const; // connected AND the client has subscribed (CCCD) + + // --- data out --- + // false means backpressure ("retry next pass"), not a hard failure: the + // caller must leave the entry queued and not advance its tail. + bool notify(const uint8_t* data, uint16_t len); + + // --- advertising payload --- + // Pushes a new manufacturer-specific-data payload into the advertisement. + // Each implementation keeps its own restart semantics (see the .cpp). + void setManufacturerData(const uint8_t* msd, uint8_t len); + + // True where the stack re-arms advertising by itself after a disconnect + // (nRF: Bluefruit.Advertising.restartOnDisconnect(true)). Where it is false + // the application must schedule the restart itself. A genuine capability + // difference, stated as a query so callers need no target #ifdef. + bool restartsAdvertisingOnDisconnect() const; + + // --- link policy (no-op where the stack does not support it) --- + void requestFastLink(); // nRF: 2M PHY + 251-octet DLE on the live link + void boostAdvertising(); // nRF: temporary fast advertising interval + void tick(); // periodic housekeeping (advertising interval restore) + + // --- events: consume-once, polled from loop(). No app-facing callbacks. --- + bool takeConnectedEvent(); + // Optionally reports the stack's disconnect reason code, which is otherwise + // lost now that the callback no longer runs application code inline. + // rxBoundary, when requested, is the RX ring head at the instant the link went + // down -- the dividing line between the departed client's queued frames and any + // pushed by whoever connected afterwards. Pass it to bleRxQueueDiscardTo(); a + // flush without it drops the next client's frames whenever loop() was blocked + // long enough for a reconnect to land before this event was serviced. + bool takeDisconnectedEvent(uint8_t* reason = nullptr, uint8_t* rxBoundary = nullptr); + + // --- identity --- + const char* addressString(); // advertised BLE address, lowercase "aa:bb:.." +}; + +extern BleTransport ble; + +// The loop()-serviced deferred-work flags that used to be declared here have +// moved: they encode application policy, not link state, so exporting them from +// the transport seam was backwards. The two that other translation units need to +// raise are now requestTransferSessionCleanup() / requestAdvertisingRestart() in +// communication.h; the flags themselves are private to main.cpp. + +#endif // BLE_TRANSPORT_H diff --git a/src/ble_transport_esp32.cpp b/src/ble_transport_esp32.cpp new file mode 100644 index 0000000..808260e --- /dev/null +++ b/src/ble_transport_esp32.cpp @@ -0,0 +1,368 @@ +// ESP32 (NimBLE-Arduino) implementation of BleTransport. +// +// The whole file is gated on TARGET_ESP32, so an nRF build compiles it to an +// empty translation unit -- no build_src_filter changes needed across the CI +// environments. Every NimBLE object lives here as file-static state; nothing +// outside this file names a NimBLE type. +// +// Threading contract (already holds here, and is what Phase 3 brings to nRF): +// NimBLE host-task callbacks do exactly two things -- copy bytes into the RX +// ring, and set a flag. Everything else runs on the loop() task. +#ifdef TARGET_ESP32 + +#include +#include + +#include "ble_transport.h" +#include "ble_transport_esp32.h" +#include "command_queue.h" +#include "structs.h" +#include "od_log.h" + +String getChipIdHex(); + +BleTransport ble; + +// --- stack objects (were globals in main.h) --------------------------------- +static BLEServer* s_server = nullptr; +static BLEService* s_service = nullptr; +static BLECharacteristic* s_txCharacteristic = nullptr; +static BLEAdvertisementData s_advertisementData; + +static volatile bool s_notifySubscribed = false; +static volatile bool s_connectedEvent = false; +static volatile bool s_disconnectedEvent = false; +static volatile uint8_t s_disconnectReason = 0; +// RX ring head at the instant the link dropped: the boundary between the departed +// client's queued frames and anything the next client pushes. Captured in the +// disconnect callback because that is the only moment it is knowable -- by the time +// loop() services the event, a reconnect may already have queued frames of its own. +static volatile uint8_t s_rxBoundaryAtDisconnect = 0; +static volatile uint16_t s_connHandle = BLE_HS_CONN_HANDLE_NONE; + +static void clearHandles() { + s_server = nullptr; + s_service = nullptr; + s_txCharacteristic = nullptr; + s_notifySubscribed = false; +} + +// --- link diagnostics (implementation-private) ------------------------------ +static const char* phyName(uint8_t phy) { + switch (phy) { + case BLE_GAP_LE_PHY_1M: return "1M"; + case BLE_GAP_LE_PHY_2M: return "2M"; + case BLE_GAP_LE_PHY_CODED: return "Coded"; + default: return "?"; + } +} + +// Report the whole negotiated picture on every event rather than one field per +// callback, so a single log line is enough to judge the link. Logged at INFO: +// this is the answer to "did the 2M PHY / big MTU actually get granted", which +// is worth having in a default-level capture from the bench. +// +// DLE (max LL PDU octets) is absent: NimBLEConnInfo exposes MTU and connection +// interval but not the negotiated data length, unlike Bluefruit's +// BLEConnection::getDataLength(). Not an oversight -- there is no accessor. +static void logNegotiatedLink(NimBLEConnInfo& info, const char* trigger) { + uint8_t txPhy = 0; + uint8_t rxPhy = 0; + if (s_server != nullptr) { + s_server->getPhy(info.getConnHandle(), &txPhy, &rxPhy); + } + od_log_info("[LINK negotiated after %s] PHY tx=%s rx=%s ATT_MTU=%u connInterval=%.2f ms", + trigger, phyName(txPhy), phyName(rxPhy), + (unsigned)info.getMTU(), info.getConnInterval() * 1.25f); +} + +// --- stack callbacks (NimBLE host task -- flag-only) ------------------------ +class OdServerCallbacks : public BLEServerCallbacks { + void onConnect(NimBLEServer* pServer, NimBLEConnInfo& connInfo) override { + (void)pServer; + od_log_info("=== BLE CLIENT CONNECTED (ESP32) ==="); + s_notifySubscribed = false; + // Captured here because it is the only place NimBLE hands it to us; the + // link tuning that consumes it runs later, on the loop task. + s_connHandle = connInfo.getConnHandle(); + // Flag-only. The app work this implies (rebootFlag reset, updatemsdata() + // -- which polls I2C and mutates the shared advertisement vector that + // loop() also drives on a 60 s cadence) would corrupt the heap if run + // here on the NimBLE host task. loop() consumes the event instead. + s_connectedEvent = true; + } + void onDisconnect(NimBLEServer* pServer, NimBLEConnInfo& connInfo, int reason) override { + (void)pServer; + (void)connInfo; + od_log_info("=== BLE CLIENT DISCONNECTED (ESP32) ==="); + s_notifySubscribed = false; + s_disconnectReason = (uint8_t)reason; + s_connHandle = BLE_HS_CONN_HANDLE_NONE; + // Producer-task read of the producer-owned head: no synchronisation needed, + // and within the copy-and-flag contract. Nothing this client can still send + // exists past this point -- the link is gone -- so this is exactly the last + // frame of the departed session. + s_rxBoundaryAtDisconnect = bleRxQueueHead(); + // Flag-only. The session teardown this implies (EPD force-off with + // SPI.end()/rail cut, partial + pipe cleanup) is heavyweight, + // state-mutating work that races loop()'s SPI streaming and pipe-frame + // processing. loop() consumes the event and applies its own deferral + // policy (see serviceBleDisconnectCleanup in main.cpp). + s_disconnectedEvent = true; + } + // Negotiation completes asynchronously, after requestFastLink() returns, so + // these are the only points where the granted values are knowable. Both are + // log-only -- no state is touched, so the callback contract holds. + void onPhyUpdate(NimBLEConnInfo& connInfo, uint8_t txPhy, uint8_t rxPhy) override { + (void)txPhy; // logNegotiatedLink re-reads both via getPhy() for one + (void)rxPhy; // consistent source, and reports MTU/interval alongside + logNegotiatedLink(connInfo, "PHY update"); + } + void onMTUChange(uint16_t MTU, NimBLEConnInfo& connInfo) override { + (void)MTU; // read back through connInfo.getMTU() with the rest + logNegotiatedLink(connInfo, "MTU exchange"); + } +}; + +class OdCharacteristicCallbacks : public BLECharacteristicCallbacks { +public: + void onSubscribe(NimBLECharacteristic* pCharacteristic, NimBLEConnInfo& connInfo, uint16_t subValue) override { + (void)pCharacteristic; + (void)connInfo; + s_notifySubscribed = (subValue & 0x0001) != 0; + od_log_info("BLE notify subscription: %s", s_notifySubscribed ? "enabled" : "disabled"); + } + void onWrite(NimBLECharacteristic* pCharacteristic, NimBLEConnInfo& connInfo) override { + (void)connInfo; + // Keep the raw NimBLEAttValue: converting to Arduino String uses the + // C-string (strlen) constructor, which truncates at the first 0x00 byte. + // Pipe-write frames start with 0x00 (00 70 / 00 71 / 00 81), so String() + // would report length 0. .length()/.c_str() on NimBLEAttValue preserve + // the binary payload. + NimBLEAttValue value = pCharacteristic->getValue(); + // Copy-and-enqueue is all this callback may do; loop() dispatches. + // bleRxQueuePush() owns the arrival log and every drop reason (empty, too + // large, ring full) so this callback and nRF's onWriteCb() cannot report the + // same frame differently. Add no logging here. + (void)bleRxQueuePush((const uint8_t*)value.c_str(), value.length()); + } +}; + +// Static callback objects (no dynamic allocation). +static OdServerCallbacks s_serverCallbacks; +static OdCharacteristicCallbacks s_charCallbacks; + +// --- BleTransport ------------------------------------------------------------ +bool BleTransport::begin(const char* deviceName) { + clearHandles(); + od_log_info("=== Initializing ESP32 BLE ==="); + od_log_info("Device name will be: %s", deviceName); + BLEDevice::init(deviceName); + // Preferred only: the central drives the exchange and may settle lower. + od_log_info("Setting preferred BLE ATT MTU to %u...", (unsigned)OD_BLE_PREFERRED_ATT_MTU); + BLEDevice::setMTU(OD_BLE_PREFERRED_ATT_MTU); + s_server = BLEDevice::createServer(); + if (s_server == nullptr) { + od_log_error("ERROR: Failed to create BLE server"); + return false; + } + // deleteCallbacks=false: s_serverCallbacks is a static object; NimBLE must + // not delete it on deinit()/replacement. + s_server->setCallbacks(&s_serverCallbacks, false); + od_log_info("Server callbacks configured"); + BLEUUID serviceUUID("00002446-0000-1000-8000-00805F9B34FB"); + s_service = s_server->createService(serviceUUID); + if (s_service == nullptr) { + od_log_error("ERROR: Failed to create BLE service"); + return false; + } + od_log_info("BLE service 0x2446 created successfully"); + BLEUUID charUUID("00002446-0000-1000-8000-00805F9B34FB"); + // Declaring max_len (rather than inheriting NimBLE's 512 default) makes the + // GATT layer reject an oversize write with ATT 0x0D instead of letting it + // reach onWrite() and be dropped silently by the MAX_COMMAND_SIZE check. + s_txCharacteristic = s_service->createCharacteristic( + charUUID, + NIMBLE_PROPERTY::READ | + NIMBLE_PROPERTY::NOTIFY | + NIMBLE_PROPERTY::WRITE | + NIMBLE_PROPERTY::WRITE_NR, + OD_BLE_MAX_FRAME + ); + if (s_txCharacteristic == nullptr) { + od_log_error("ERROR: Failed to create BLE characteristic"); + return false; + } + od_log_info("Characteristic created with properties: READ, NOTIFY, WRITE, WRITE_NR"); + // NimBLE auto-adds the 0x2902 CCCD for NOTIFY characteristics; no BLE2902 needed. + s_txCharacteristic->setCallbacks(&s_charCallbacks); + // NimBLE starts services automatically when the server starts advertising; no + // explicit start() (deprecated no-op in NimBLE 2.x). + BLEAdvertising* pAdvertising = BLEDevice::getAdvertising(); + if (pAdvertising == nullptr) { + od_log_error("ERROR: Failed to get advertising object"); + return false; + } + pAdvertising->addServiceUUID(serviceUUID); + od_log_info("Service UUID added to advertising"); + s_advertisementData.setName(deviceName); + s_advertisementData.setFlags(0x06); + od_log_info("Device name added to advertising"); + return true; +} + +void BleTransport::startAdvertising() { + BLEAdvertising* pAdvertising = BLEDevice::getAdvertising(); + if (pAdvertising == nullptr || s_server == nullptr) { + return; + } + // setAdvertisementData() must be the LAST advertising-data call before + // start(): NimBLE's enableScanResponse()/setPreferredParams()/etc. reset the + // internal "custom data set" flag, which would make start() discard this + // payload and re-advertise the piecemeal builder (no manufacturer data / no + // name). Scan response is off by default in NimBLE 2.x, so no + // enableScanResponse() needed. + pAdvertising->setAdvertisementData(s_advertisementData); + s_server->getAdvertising()->start(); + od_log_info("=== BLE advertising started successfully ==="); +} + +void BleTransport::restartAdvertising() { + // Unconditional by contract: the caller owns the "still connected / mid-EPD + // refresh / stack not up" deferral policy (see serviceBleAdvertisingRestart + // in main.cpp). The delay mirrors the historical sequence. + delay(100); + BLEDevice::startAdvertising(); + od_log_info("BLE advertising restarted"); +} + +void BleTransport::stopAdvertising() { + if (s_server == nullptr) return; + BLEAdvertising* pAdvertising = s_server->getAdvertising(); + if (pAdvertising != nullptr) { + pAdvertising->stop(); + od_log_info("BLE advertising stopped"); + } +} + +void BleTransport::end() { + BLEDevice::deinit(true); // clearAll: disables + releases the BT controller + clearHandles(); +} + +bool BleTransport::isReady() const { + return s_server != nullptr; +} + +uint8_t BleTransport::connectedCount() const { + return (s_server != nullptr) ? (uint8_t)s_server->getConnectedCount() : 0; +} + +bool BleTransport::notifyReady() const { + if (s_txCharacteristic == nullptr || s_server == nullptr || s_server->getConnectedCount() == 0) { + return false; + } + // NimBLE auto-creates the 0x2902 CCCD; onSubscribe tracks the client's toggle. + return s_notifySubscribed; +} + +bool BleTransport::notify(const uint8_t* data, uint16_t len) { + if (s_txCharacteristic == nullptr) return false; + // notify(data,len) copies the payload into an mbuf immediately, so a + // concurrent client WRITE_NR on this shared RX/TX characteristic cannot + // corrupt the outgoing frame (as setValue()+notify() could, since the no-arg + // notify sends whatever value is currently stored). On mbuf exhaustion this + // returns false -- backpressure, not failure. + return s_txCharacteristic->notify(data, len); +} + +void BleTransport::setManufacturerData(const uint8_t* msd, uint8_t len) { + s_advertisementData.setManufacturerData(msd, len); + BLEAdvertising* pAdvertising = (s_server != nullptr) ? s_server->getAdvertising() + : BLEDevice::getAdvertising(); + if (pAdvertising == nullptr || connectedCount() > 0) { + // Only rebuild+restart advertising while disconnected. The former + // connected branch rebuilt the advertisement data but never pushed it via + // setAdvertisementData(), so it was dead work -- dropped. + return; + } + pAdvertising->stop(); + BLEAdvertisementData fresh; + static String savedDeviceName = ""; + if (savedDeviceName.length() == 0) savedDeviceName = "OD" + getChipIdHex(); + fresh.setName(savedDeviceName.c_str()); + fresh.setFlags(0x06); + fresh.setManufacturerData(msd, len); + s_advertisementData = fresh; + // setAdvertisementData() must be the last data call before start(): + // enableScanResponse()/setPreferredParams() reset NimBLE's custom-data flag + // and would make start() drop this manufacturer-data payload. + pAdvertising->setAdvertisementData(fresh); + delay(50); + pAdvertising->start(); +} + +// Match nRF's link tuning: 2M PHY + 251-octet DLE. Like nRF, the peripheral only +// auto-accepts what the central asks for, so without this the link stays at +// 1M / 27 octets whenever the phone does not request better. +// +// Called from loop() when the connect event is consumed, not from the connect +// callback -- these are host-stack calls, which the callback contract excludes. +void BleTransport::requestFastLink() { + if (s_server == nullptr || s_connHandle == BLE_HS_CONN_HANDLE_NONE) { + return; + } + // 2 Mbps both directions. phyOptions applies only to the CODED PHY, so 0. + // The peer may decline and stay at 1M -- not an error. + if (!s_server->updatePhy(s_connHandle, BLE_GAP_LE_PHY_2M_MASK, BLE_GAP_LE_PHY_2M_MASK, 0)) { + od_log_warn("2M PHY request rejected (staying at 1M)"); + } + // 251-octet Link-Layer PDUs (max DLE); NimBLE derives the PHY-appropriate + // on-air duration itself, so there is no time parameter to pass. + s_server->setDataLen(s_connHandle, 251); + od_log_debug("Requested fast link: 2M PHY + 251-octet DLE"); + // No negotiated-parameter logging here yet, unlike nRF: that would need an + // equivalent of nRF's delayed one-shot, since negotiation completes after + // this returns. NimBLEServer::getPhy() is the hook if it is wanted. +} + +void BleTransport::boostAdvertising() { + // No-op: the temporary fast-advertising interval is nRF-only today. +} + +void BleTransport::tick() { + // No-op: nothing periodic to restore, since boostAdvertising() is a no-op. +} + +bool BleTransport::takeConnectedEvent() { + if (!s_connectedEvent) return false; + s_connectedEvent = false; + return true; +} + +bool BleTransport::takeDisconnectedEvent(uint8_t* reason, uint8_t* rxBoundary) { + if (!s_disconnectedEvent) return false; + s_disconnectedEvent = false; + if (reason != nullptr) *reason = s_disconnectReason; + if (rxBoundary != nullptr) *rxBoundary = s_rxBoundaryAtDisconnect; + return true; +} + +bool BleTransport::restartsAdvertisingOnDisconnect() const { + // NimBLE does not re-advertise by itself here; the application schedules it + // via requestAdvertisingRestart() so the restart can be held off while an + // EPD refresh is mid-flight. + return false; +} + +const char* BleTransport::addressString() { + // The advertised BLE address, lowercase colon-separated (SECTION 9 rule 6, + // key `mac`). HARDWARE VALIDATION REQUIRED: confirm NimBLEDevice::getAddress() + // == the advertised AdvA HA sees (public vs static-random) on BOTH S3 and C6. + static String cached; + cached = String(NimBLEDevice::getAddress().toString().c_str()); + cached.toLowerCase(); + return cached.c_str(); +} + +#endif // TARGET_ESP32 diff --git a/src/ble_transport_esp32.h b/src/ble_transport_esp32.h new file mode 100644 index 0000000..d22a6a4 --- /dev/null +++ b/src/ble_transport_esp32.h @@ -0,0 +1,27 @@ +#ifndef BLE_TRANSPORT_ESP32_H +#define BLE_TRANSPORT_ESP32_H + +// NimBLE-facing declarations for the ESP32 BleTransport implementation. +// Included ONLY from ble_transport_esp32.cpp, inside that file's TARGET_ESP32 +// gate. This is what stops NimBLE types leaking: the BLE* aliases below used to +// live in ble_init.h, which six translation units included (main.h, +// communication.cpp, display_service.cpp, device_control.cpp, wifi_service.cpp, +// esp32_ble_callbacks.h). Application code now includes ble_transport.h only. +#ifdef TARGET_ESP32 + +#include + +// ESP32 BLE is backed by NimBLE-Arduino (h2zero). The aliases keep the +// historical BLE* spellings inside this implementation. +using BLEDevice = NimBLEDevice; +using BLEServer = NimBLEServer; +using BLEService = NimBLEService; +using BLECharacteristic = NimBLECharacteristic; +using BLEAdvertising = NimBLEAdvertising; +using BLEAdvertisementData = NimBLEAdvertisementData; +using BLEServerCallbacks = NimBLEServerCallbacks; +using BLECharacteristicCallbacks = NimBLECharacteristicCallbacks; +using BLEUUID = NimBLEUUID; + +#endif // TARGET_ESP32 +#endif // BLE_TRANSPORT_ESP32_H diff --git a/src/ble_transport_nrf.cpp b/src/ble_transport_nrf.cpp new file mode 100644 index 0000000..50ff656 --- /dev/null +++ b/src/ble_transport_nrf.cpp @@ -0,0 +1,356 @@ +// nRF52840 (Adafruit Bluefruit / SoftDevice S140) implementation of BleTransport. +// +// The whole file is gated on TARGET_NRF, so an ESP32 build compiles it to an +// empty translation unit -- no build_src_filter changes needed across the CI +// environments. Every Bluefruit object lives here as file-static state; nothing +// outside this file names a Bluefruit type. +#ifdef TARGET_NRF + +#include "ble_transport.h" +#include "ble_transport_nrf.h" +#include "command_queue.h" +#include "structs.h" +#include "encryption.h" +#include "od_log.h" + +extern "C" { +#include "nrf_soc.h" +} + +extern struct GlobalConfig globalConfig; +String getChipIdHex(); + +BleTransport ble; + +// --- stack objects (were globals in main.h) --------------------------------- +static BLEDfu s_dfu; +static BLEService s_imageService("2446"); +// max_len is a GATT-declared attribute length the SoftDevice reserves for real +// (vloc = BLE_GATTS_VLOC_STACK), so 512 cost 512 B of attribute table while the +// link caps at ATT MTU 247 (payload 244) -- half of it was unreachable. +static BLECharacteristic s_imageCharacteristic( + "2446", BLEWrite | BLEWriteWithoutResponse | BLENotify, OD_BLE_MAX_FRAME); + +static bool s_begun = false; +static uint16_t s_connHandle = BLE_CONN_HANDLE_INVALID; +static volatile bool s_connectedEvent = false; +static volatile bool s_disconnectedEvent = false; +static volatile uint8_t s_disconnectReason = 0; +// RX ring head at the instant the link dropped: the boundary between the departed +// client's queued frames and anything the next client pushes. Captured in the +// disconnect callback because that is the only moment it is knowable -- by the time +// loop() services the event, a reconnect may already have queued frames of its own. +static volatile uint8_t s_rxBoundaryAtDisconnect = 0; + +// --- advertising interval policy -------------------------------------------- +static uint32_t s_advBoostUntil = 0; + +static constexpr uint16_t NRF_ADV_INTERVAL_MIN = 256; // 160 ms +static constexpr uint16_t NRF_ADV_INTERVAL_MAX = 1600; // 1000 ms +static constexpr uint16_t NRF_ADV_BOOST_MIN = 32; // 20 ms +static constexpr uint16_t NRF_ADV_BOOST_MAX = 48; // 30 ms +static constexpr uint32_t NRF_ADV_BOOST_MS = 3000; + +static void applyAdvInterval() { + if (s_advBoostUntil != 0 && millis() < s_advBoostUntil) { + Bluefruit.Advertising.setInterval(NRF_ADV_BOOST_MIN, NRF_ADV_BOOST_MAX); + } else { + s_advBoostUntil = 0; + Bluefruit.Advertising.setInterval(NRF_ADV_INTERVAL_MIN, NRF_ADV_INTERVAL_MAX); + } +} + +// --- link-layer diagnostics (implementation-private) ------------------------ +// DLE (Data Length Extension) sets the max Link-Layer PDU payload: 27 octets by +// default, up to 251 once negotiated. The nRF peripheral only auto-accepts the +// central's request, which arrives AFTER the connect callback, so we log twice: +// once at connect (baseline) and once ~2.5 s later (negotiated). +// `atInfo` separates the two callers: the pre-negotiation baseline is diagnostic +// noise and stays at DEBUG, while the negotiated result -- the answer to "did the +// 2M PHY and 251-octet DLE actually get granted" -- logs at INFO so it survives a +// default-level bench capture. Mirrors logNegotiatedLink() on ESP32. +static void logLinkParams(uint16_t conn_handle, const char* phase, bool atInfo) { + BLEConnection* conn = Bluefruit.Connection(conn_handle); + if (conn == nullptr) { + od_log_debug("[LINK %s] no connection (handle %u)", phase, conn_handle); + return; + } + uint8_t phy = conn->getPHY(); + uint16_t mtu = conn->getMtu(); // ATT MTU (23 default; 247 cap here) + uint16_t dle = conn->getDataLength(); // LL PDU payload octets (27 default; 251 max) + uint16_t ci = conn->getConnectionInterval(); // units of 1.25 ms + const char* phyStr = (phy == BLE_GAP_PHY_2MBPS) ? "2M" : + (phy == BLE_GAP_PHY_1MBPS) ? "1M" : "?"; + char line[128]; + snprintf(line, sizeof(line), + "[LINK %s] PHY=%s ATT_MTU=%u DLE=%u octets connInterval=%.2f ms", + phase, phyStr, mtu, dle, ci * 1.25f); + if (atInfo) { + od_log_info("%s", line); + } else { + od_log_debug("%s", line); + } +} + +// One-shot timer (armed only on connect -- no per-loop polling). Fires once on +// the FreeRTOS timer task after the central finishes negotiation. This is a +// third execution context; it only logs, which is why it is safe here. +static SoftwareTimer s_linkDiagTimer; +static uint16_t s_linkDiagConn = BLE_CONN_HANDLE_INVALID; + +static void linkDiagCallback(TimerHandle_t /*xTimer*/) { + if (Bluefruit.connected()) { + logLinkParams(s_linkDiagConn, "negotiated", true); // INFO: the granted result + } +} + +static void armLinkDiag(uint16_t conn_handle) { + s_linkDiagConn = conn_handle; + static bool created = false; + if (!created) { + // Create the one-shot (repeating=false) on the first connection only. + s_linkDiagTimer.begin(500, linkDiagCallback, NULL, false); + created = true; + } + s_linkDiagTimer.reset(); // start/restart the one-shot; fires ~2.5 s later +} + +// --- stack callbacks (SoftDevice callback task -- flag-only) ----------------- +// The threading contract, as of Phase 3 and matching ESP32: a stack callback may +// do exactly two things, copy bytes into the RX ring and set a flag. Everything +// else -- command dispatch, zlib inflate, EPD SPI streaming, notify(), the +// connect/disconnect application work, even the PHY/DLE request -- runs on the +// loop() task. Anything added below that is not a push or a flag store +// reintroduces the cross-task races this phase exists to remove. +static void onConnectCb(uint16_t conn_handle) { + od_log_info("=== BLE CLIENT CONNECTED (nRF) ==="); + s_connHandle = conn_handle; + s_connectedEvent = true; +} + +static void onDisconnectCb(uint16_t conn_handle, uint8_t reason) { + (void)conn_handle; + od_log_info("=== BLE CLIENT DISCONNECTED (nRF) ==="); + s_connHandle = BLE_CONN_HANDLE_INVALID; + s_disconnectReason = reason; + // Producer-task read of the producer-owned head: no synchronisation needed, and + // within the copy-and-flag contract. Nothing this client can still send exists + // past this point -- the link is gone -- so this is exactly the last frame of + // the departed session. + s_rxBoundaryAtDisconnect = bleRxQueueHead(); + s_disconnectedEvent = true; +} + +// Adapter: Bluefruit's write_callback_t is BLECharacteristic*-shaped, whereas the +// shared dispatcher takes an opaque pointer (it ignores both leading arguments on +// every target). Adapting here is what keeps Bluefruit types out of +// communication.cpp. +static void onWriteCb(uint16_t conn_hdl, BLECharacteristic* chr, uint8_t* data, uint16_t len) { + (void)conn_hdl; + (void)chr; + // bleRxQueuePush() owns the arrival log and every drop reason (empty, too large, + // ring full) so this callback and ESP32's onWrite() cannot report the same frame + // differently. This site used to print "queue full" for all three, sending you + // after ring depth when the real cause was a malformed frame. Add no logging here. + (void)bleRxQueuePush(data, len); +} + +// --- BleTransport ------------------------------------------------------------ +bool BleTransport::begin(const char* deviceName) { + Bluefruit.configCentralBandwidth(BANDWIDTH_MAX); + Bluefruit.configPrphBandwidth(BANDWIDTH_MAX); + Bluefruit.autoConnLed(false); + Bluefruit.setTxPower(globalConfig.power_option.tx_power); + Bluefruit.begin(1, 0); + od_log_info("BLE initialized successfully"); + od_log_info("Setting up BLE service 0x2446..."); + s_imageService.begin(); + od_log_info("BLE service started"); + s_imageCharacteristic.setWriteCallback(onWriteCb); + od_log_info("BLE write callback set"); + s_imageCharacteristic.begin(); + od_log_info("BLE characteristic started"); + // Register the DFU service LAST so its presence/absence (it is only added when + // encryption is disabled) never shifts the handles of s_imageCharacteristic and + // its CCCD. GATT handles are assigned in begin() order; keeping the app + // characteristic ahead of the conditional DFU service keeps its handles stable + // across encryption on/off, so a client's cached CCCD handle stays valid and + // notify setup won't fail with ATT "Invalid handle". Must stay after + // Bluefruit.begin() (SoftDevice up first). + if (!isEncryptionEnabled()) { + s_dfu.begin(); + od_log_info("BLE DFU initialized successfully (encryption disabled)"); + } else { + od_log_info("BLE DFU service NOT initialized (encryption enabled - use CMD_ENTER_DFU)"); + } + Bluefruit.Periph.setConnectCallback(onConnectCb); + Bluefruit.Periph.setDisconnectCallback(onDisconnectCb); + od_log_info("BLE callbacks registered"); + Bluefruit.setName(deviceName); + od_log_info("Device name set to: %s", deviceName); + od_log_info("Configuring power management..."); + sd_power_mode_set(NRF_POWER_MODE_LOWPWR); + sd_power_dcdc_mode_set(NRF_POWER_DCDC_ENABLE); + od_log_info("Power management configured"); + s_begun = true; + return true; +} + +void BleTransport::startAdvertising() { + od_log_info("Configuring BLE advertising..."); + Bluefruit.Advertising.clearData(); + Bluefruit.Advertising.addFlags(BLE_GAP_ADV_FLAGS_LE_ONLY_GENERAL_DISC_MODE); + Bluefruit.Advertising.addName(); + // Deliberately kept inside this sequence rather than hoisted to the caller: + // updatemsdata() lands in setManufacturerData() below, which itself sets + // setFastTimeout(1). Calling it after the setFastTimeout(10) line would leave + // the fast-advertising window at 1 s instead of 10 s -- a real behaviour + // change. Phase 1 keeps the historical order byte-for-byte. + updatemsdata(); + Bluefruit.Advertising.restartOnDisconnect(true); + applyAdvInterval(); + Bluefruit.Advertising.setFastTimeout(10); + od_log_info("Starting BLE advertising..."); + Bluefruit.Advertising.start(0); +} + +void BleTransport::restartAdvertising() { + Bluefruit.Advertising.stop(); + Bluefruit.Advertising.start(0); +} + +void BleTransport::stopAdvertising() { + Bluefruit.Advertising.stop(); +} + +void BleTransport::end() { + // No-op: the SoftDevice stays up for the life of the nRF firmware. Only the + // ESP32 tears the controller down (deep sleep / pre-restart). +} + +bool BleTransport::isReady() const { + return s_begun; +} + +uint8_t BleTransport::connectedCount() const { + // Bluefruit.connected() returns the number of active connections; the + // peripheral is configured for a single link (Bluefruit.begin(1, 0)). + return (uint8_t)Bluefruit.connected(); +} + +bool BleTransport::notifyReady() const { + return Bluefruit.connected() && s_imageCharacteristic.notifyEnabled(); +} + +bool BleTransport::notify(const uint8_t* data, uint16_t len) { + return s_imageCharacteristic.notify(data, len); +} + +void BleTransport::setManufacturerData(const uint8_t* msd, uint8_t len) { + Bluefruit.Advertising.clearData(); + Bluefruit.Advertising.addFlags(BLE_GAP_ADV_FLAGS_LE_ONLY_GENERAL_DISC_MODE); + Bluefruit.Advertising.addName(); + Bluefruit.Advertising.addData(BLE_GAP_AD_TYPE_MANUFACTURER_SPECIFIC_DATA, msd, len); + applyAdvInterval(); + Bluefruit.Advertising.setFastTimeout(1); + Bluefruit.Advertising.stop(); + Bluefruit.Advertising.start(0); +} + +// Proactively upgrade the link for throughput: the nRF peripheral only +// auto-accepts the central's PHY/DLE requests, so if the phone never asks we +// stay at 1M / 27 octets. Both requests are no-ops if the peer already +// negotiated the same or better. +// +// Called from loop() when the connect event is consumed, NOT from the connect +// callback: these are SoftDevice calls, which the Phase 3 callback contract +// ("copy bytes, set a flag") excludes. The few milliseconds of delay cost +// nothing -- the central's own request arrives later than this either way, which +// is why the diagnostics below log twice. +void BleTransport::requestFastLink() { + BLEConnection* conn = Bluefruit.Connection(s_connHandle); + if (conn == nullptr) return; + + logLinkParams(s_connHandle, "at connect", false); // DEBUG: baseline, pre-negotiation + + // 2 Mbps PHY (tx + rx). Peer may decline and stay at 1M. + conn->requestPHY(BLE_GAP_PHY_2MBPS); + + // 251-octet Link-Layer PDUs (max DLE). AUTO time lets the controller derive + // the PHY-appropriate on-air duration. + ble_gap_data_length_params_t dl; + dl.max_tx_octets = 251; + dl.max_rx_octets = 251; + dl.max_tx_time_us = BLE_GAP_DATA_LENGTH_AUTO; + dl.max_rx_time_us = BLE_GAP_DATA_LENGTH_AUTO; + ble_gap_data_length_limitation_t limit = { 0, 0, 0 }; + if (!conn->requestDataLengthUpdate(&dl, &limit)) { + od_log_warn("DLE 251 request rejected (tx_lim=%u rx_lim=%u time_lim_us=%u)", + limit.tx_payload_limited_octets, limit.rx_payload_limited_octets, + limit.tx_rx_time_limited_us); + } + od_log_debug("Requested fast link: 2M PHY + 251-octet DLE"); + armLinkDiag(s_connHandle); // re-log once negotiation settles +} + +void BleTransport::boostAdvertising() { + s_advBoostUntil = millis() + NRF_ADV_BOOST_MS; +} + +void BleTransport::tick() { + static bool was_boosted = false; + const bool boosting = (s_advBoostUntil != 0 && millis() < s_advBoostUntil); + if (boosting) { + was_boosted = true; + return; + } + if (!was_boosted || !Bluefruit.Advertising.isRunning()) { + was_boosted = false; + s_advBoostUntil = 0; + return; + } + was_boosted = false; + s_advBoostUntil = 0; + Bluefruit.Advertising.setInterval(NRF_ADV_INTERVAL_MIN, NRF_ADV_INTERVAL_MAX); + Bluefruit.Advertising.stop(); + Bluefruit.Advertising.start(0); +} + +bool BleTransport::takeConnectedEvent() { + if (!s_connectedEvent) return false; + s_connectedEvent = false; + return true; +} + +bool BleTransport::takeDisconnectedEvent(uint8_t* reason, uint8_t* rxBoundary) { + if (!s_disconnectedEvent) return false; + s_disconnectedEvent = false; + if (reason != nullptr) *reason = s_disconnectReason; + if (rxBoundary != nullptr) *rxBoundary = s_rxBoundaryAtDisconnect; + return true; +} + +bool BleTransport::restartsAdvertisingOnDisconnect() const { + // Bluefruit.Advertising.restartOnDisconnect(true) in startAdvertising(): the + // SoftDevice re-arms the radio itself, so the application must not also + // schedule a restart or the two fight over the advertising state. + return true; +} + +const char* BleTransport::addressString() { + // Lowercase colon-separated, matching the ESP32 implementation's contract. + // + // Byte order matters: Bluefruit::getAddr(mac) memcpy's ble_gap_addr_t.addr + // straight out of sd_ble_gap_addr_get(), and the SoftDevice stores that + // LSB-first (bluefruit.cpp:508-515). The advertised AdvA and every + // human-readable form are MSB-first, so emit it reversed -- otherwise this + // returns a byte-swapped address that looks plausible and matches nothing. + static char s_addr[18]; + uint8_t mac[6] = {0}; + (void)Bluefruit.getAddr(mac); // return value is the address TYPE, not a status + snprintf(s_addr, sizeof(s_addr), "%02x:%02x:%02x:%02x:%02x:%02x", + mac[5], mac[4], mac[3], mac[2], mac[1], mac[0]); + return s_addr; +} + +#endif // TARGET_NRF diff --git a/src/ble_transport_nrf.h b/src/ble_transport_nrf.h new file mode 100644 index 0000000..e571cf3 --- /dev/null +++ b/src/ble_transport_nrf.h @@ -0,0 +1,17 @@ +#ifndef BLE_TRANSPORT_NRF_H +#define BLE_TRANSPORT_NRF_H + +// Bluefruit-facing declarations for the nRF BleTransport implementation. +// Included ONLY from ble_transport_nrf.cpp, inside that file's TARGET_NRF gate -- +// application code includes ble_transport.h and nothing else, so Bluefruit types +// never leak past this implementation. +#ifdef TARGET_NRF + +#include + +// Defined in display_service.cpp. Called from startAdvertising() to keep the +// historical advertising bring-up sequence byte-for-byte (see the .cpp). +void updatemsdata(); + +#endif // TARGET_NRF +#endif // BLE_TRANSPORT_NRF_H diff --git a/src/command_queue.cpp b/src/command_queue.cpp new file mode 100644 index 0000000..0772eb1 --- /dev/null +++ b/src/command_queue.cpp @@ -0,0 +1,211 @@ +// Storage and access for the BLE command rings. Previously defined in main.h, a +// single-inclusion globals header included only by main.cpp -- the rings are +// shared between main.cpp, communication.cpp and the transport implementations, +// so they belong in a translation unit of their own. +// +// Phase 2 removed the TARGET_ESP32 guard: both targets now compile the rings. +// nRF carries them unused until Phase 3 moves its dispatch to loop(). + +#include +#include + +#include "command_queue.h" +#include "ble_transport.h" +#include "encryption.h" // isEncryptionEnabled(), for the ERX/URX token +#include "od_log.h" + +// Defined in display_service.cpp. True for a mid-stream image-write data frame +// (0x0071 / 0x0081) whose per-frame RX logging should be suppressed. +bool imageWriteLogQuietFrame(const uint8_t* data, uint16_t len); + +// --- RX ---------------------------------------------------------------------- +static CommandQueueItem s_rx[COMMAND_QUEUE_SIZE]; +static volatile uint8_t s_rxHead = 0; +static volatile uint8_t s_rxTail = 0; + +// The single RX log line, and the single place the three push failures are named. +// +// It lives here, not in either transport, because this is the one function both +// stack callbacks call -- ESP32's onWrite() and nRF's onWriteCb(). A copy in each +// callback is how the two targets drifted in the first place: ESP32 had a hex line +// and separate "too large" / "empty" warnings, nRF had neither and reported all +// three failures as "queue full", pointing at ring depth for what was actually a +// malformed frame. +// +// Logged at ARRIVAL, on the stack callback task, so the timestamp is when the radio +// delivered the frame rather than when loop() got to it -- that ordering is the +// point of the line, and it cannot be had from the consumer side. The cost is real +// and deliberate: od_log ends in a blocking serial write (~9 ms for a full hex line +// at 115200), so this delays the NimBLE host / SoftDevice callback task. It is +// compiled out entirely at the default INFO level; only the -debug envs pay it, and +// only for frames the quiet predicate does not suppress. +// +// Depth is the PRE-push count, matching logTxFrame()'s pre-enqueue depth, so a +// healthy path reads [BLE][Q:0] and a rising Q means arrivals are outrunning +// loop()'s drain. RX is BLE-only by construction -- LAN frames reach the dispatcher +// without touching this ring -- so the tag is a literal, not originTag(). +bool bleRxQueuePush(const uint8_t* data, uint16_t len) { + if (len == 0) { + od_log_warn("WARNING: Empty BLE frame received, dropping"); + return false; + } + if (len > MAX_COMMAND_SIZE) { + od_log_warn("WARNING: Command too large for queue (%u > %u), dropping", + (unsigned)len, (unsigned)MAX_COMMAND_SIZE); + return false; + } + // Publish head with RELEASE after the payload is fully written so the + // consumer never observes a slot before its bytes land. + uint8_t head = __atomic_load_n(&s_rxHead, __ATOMIC_RELAXED); + uint8_t tail = __atomic_load_n(&s_rxTail, __ATOMIC_ACQUIRE); + uint8_t nextHead = (head + 1) % COMMAND_QUEUE_SIZE; + if (nextHead == tail) { + od_log_error("ERROR: Command queue full, dropping command (%u slots)", + (unsigned)COMMAND_QUEUE_SIZE); + return false; + } + if (!imageWriteLogQuietFrame(data, len)) { + const uint16_t cmd = (len >= 2) ? (uint16_t)((data[0] << 8) | data[1]) : data[0]; + const uint8_t depth = (uint8_t)((head - tail + COMMAND_QUEUE_SIZE) % COMMAND_QUEUE_SIZE); + // ERX / URX: does this frame carry the app-layer CCM envelope? Mirrors the + // gate in imageDataWritten() -- the two handshake opcodes are dispatched + // before it, and a frame too short to hold nonce+tag cannot be wrapped. The + // ORIGIN_LAN_TLS term of that gate is omitted deliberately: this ring is BLE + // only, LAN frames never reach it. Anything URX while encryption is on is + // rejected by the dispatcher, so the token is the frame's form, not intent. + const bool encrypted = isEncryptionEnabled() && + cmd != CMD_AUTHENTICATE && cmd != CMD_FIRMWARE_VERSION && + len >= BLE_CMD_HEADER_SIZE + ENCRYPTION_NONCE_SIZE + ENCRYPTION_TAG_SIZE; + char label[48]; + snprintf(label, sizeof(label), "[BLE][Q:%u] %s 0x%04X (%u B): ", + (unsigned)depth, encrypted ? "ERX" : "URX", cmd, (unsigned)len); + char line[192]; + od_log_hex_line(line, sizeof(line), label, data, len); + od_log_debug("%s", line); + } + memcpy(s_rx[head].data, data, len); + s_rx[head].len = len; + s_rx[head].pending = true; + __atomic_store_n(&s_rxHead, nextHead, __ATOMIC_RELEASE); + return true; +} + +CommandQueueItem* bleRxQueuePeek(void) { + // ACQUIRE the head so the payload the producer wrote before its RELEASE + // store is visible to us. + uint8_t tail = __atomic_load_n(&s_rxTail, __ATOMIC_RELAXED); + uint8_t head = __atomic_load_n(&s_rxHead, __ATOMIC_ACQUIRE); + if (tail == head) return nullptr; + return &s_rx[tail]; +} + +void bleRxQueueConsume(void) { + uint8_t tail = __atomic_load_n(&s_rxTail, __ATOMIC_RELAXED); + s_rx[tail].pending = false; + __atomic_store_n(&s_rxTail, (uint8_t)((tail + 1) % COMMAND_QUEUE_SIZE), __ATOMIC_RELEASE); +} + +uint8_t bleRxQueueDiscardTo(uint8_t boundary) { + // Discard up to `boundary` -- a head value captured when the departed client's + // link went down -- NOT up to the current head. Discarding to the current head + // is what broke: loop() can be blocked for tens of seconds inside an EPD refresh, + // during which the old client's disconnect, the next client's connect, and that + // client's first command all land. Servicing the stale disconnect then threw away + // a frame that had never belonged to the departed session. + // + // ACQUIRE the head for the same reason peek does. Safe against a concurrent + // producer: it only ever advances the head, so frames pushed after this load + // survive to the next pass rather than being lost or double-counted. + uint8_t tail = __atomic_load_n(&s_rxTail, __ATOMIC_RELAXED); + uint8_t head = __atomic_load_n(&s_rxHead, __ATOMIC_ACQUIRE); + if (tail == head) return 0; + const uint8_t occupied = (uint8_t)((head - tail + COMMAND_QUEUE_SIZE) % COMMAND_QUEUE_SIZE); + const uint8_t wanted = (uint8_t)((boundary - tail + COMMAND_QUEUE_SIZE) % COMMAND_QUEUE_SIZE); + // The consumer already drained past the boundary: nothing of the old session is + // left. Without this test the modular subtraction above would read as a nearly + // full ring and discard the live client's frames -- the very bug being fixed. + if (wanted > occupied) return 0; + for (uint8_t i = tail; i != boundary; i = (uint8_t)((i + 1) % COMMAND_QUEUE_SIZE)) { + s_rx[i].pending = false; + } + __atomic_store_n(&s_rxTail, boundary, __ATOMIC_RELEASE); + return wanted; +} + +uint8_t bleRxQueueHead(void) { + return __atomic_load_n(&s_rxHead, __ATOMIC_RELAXED); +} + +uint8_t bleRxQueueDepth(void) { + // RELAXED both: a snapshot for logging, not a synchronisation point. The + // producer may push concurrently, in which case this simply reads one frame + // stale -- which is the correct answer for "how deep was it a moment ago". + const uint8_t head = __atomic_load_n(&s_rxHead, __ATOMIC_RELAXED); + const uint8_t tail = __atomic_load_n(&s_rxTail, __ATOMIC_RELAXED); + return (uint8_t)((head - tail + COMMAND_QUEUE_SIZE) % COMMAND_QUEUE_SIZE); +} + +bool bleRxQueuePending(void) { + return __atomic_load_n(&s_rxTail, __ATOMIC_RELAXED) != + __atomic_load_n(&s_rxHead, __ATOMIC_RELAXED); +} + +// --- TX ---------------------------------------------------------------------- +static ResponseQueueItem s_tx[RESPONSE_QUEUE_SIZE]; +static uint8_t s_txHead = 0; +static uint8_t s_txTail = 0; + +bool bleTxQueuePush(const uint8_t* data, uint16_t len) { + if (len > MAX_RESPONSE_SIZE) { + od_log_error("ERROR: Response too large for queue (%u > %u)", len, MAX_RESPONSE_SIZE); + return false; + } + uint8_t nextHead = (s_txHead + 1) % RESPONSE_QUEUE_SIZE; + if (nextHead == s_txTail) { + od_log_error("ERROR: Response queue full, dropping response"); + return false; + } + memcpy(s_tx[s_txHead].data, data, len); + s_tx[s_txHead].len = len; + s_tx[s_txHead].pending = true; + s_txHead = nextHead; + return true; +} + +uint8_t bleTxQueueDepth(void) { + return (uint8_t)((s_txHead - s_txTail + RESPONSE_QUEUE_SIZE) % RESPONSE_QUEUE_SIZE); +} + +uint8_t bleTxQueueHead(void) { + return s_txHead; +} + +bool bleTxQueuePending(void) { + return s_txTail != s_txHead; +} + +void serviceBleTx(void) { + if (s_txTail == s_txHead) return; + if (ble.notifyReady()) { + uint8_t drained = 0; + while (s_txTail != s_txHead && drained < 16) { + // false means backpressure (NimBLE mbuf exhaustion): stop draining and + // leave the entry queued to retry next pass rather than advancing past + // a dropped ACK, which would stall the pipe window. See + // BleTransport::notify(). + if (!ble.notify(s_tx[s_txTail].data, s_tx[s_txTail].len)) { + break; + } + s_tx[s_txTail].pending = false; + s_txTail = (s_txTail + 1) % RESPONSE_QUEUE_SIZE; + drained++; + } + } else if (ble.isConnected()) { + // Connected but CCCD not enabled yet -- keep responses queued + } else { + while (s_txTail != s_txHead) { + s_tx[s_txTail].pending = false; + s_txTail = (s_txTail + 1) % RESPONSE_QUEUE_SIZE; + } + } +} diff --git a/src/command_queue.h b/src/command_queue.h new file mode 100644 index 0000000..4e54a5a --- /dev/null +++ b/src/command_queue.h @@ -0,0 +1,144 @@ +#ifndef COMMAND_QUEUE_H +#define COMMAND_QUEUE_H + +#include +#include "opendisplay_protocol.h" // OD_BLE_MAX_FRAME +#include "structs.h" // PIPE_MAX_W (shrunk by PIPE_SMALL_DRAM_WINDOW) + +// The two BLE command rings, declared together because they are two halves of +// one thing: frames in from the stack callback, responses out to it. +// +// Portable by construction -- no Bluefruit, no NimBLE, no Arduino -- and +// compiled on every target as of Phase 2. nRF carries the storage but does not +// yet use it: its write callback still dispatches inline on the SoftDevice +// callback task, and its responses still go straight out through +// BleTransport::notify(). Phase 3 makes nRF a producer on the RX ring and moves +// its dispatch to loop(), at which point serviceBleTx() drives both targets. +// See docs/PLAN_BLE_TRANSPORT_ABSTRACTION_2026-07-27.md. +// +// Deliberately not in structs.h: that header is the config-packet / wire-protocol +// hub pulled into every translation unit, and these rings are neither. +// Deliberately not on the BleTransport class either: this is buffering, not link +// state. + +// --- RX: BLE stack callback (producer) -> loop() (consumer) ------------------ +// DEPTH IS DERIVED FROM THE PIPE WINDOW, not hardcoded. The worst case this ring +// must absorb is a 60 s Spectra SPI stall (loop blocked inside bbepWriteData) +// while a client streams a PIPE_WRITE transfer: +// +// * up to PIPE_MAX_W unacknowledged DATA (0x0081) frames -- the sender's +// window rule bounds these, and +// * the END (0x0082) that closes the transfer. END carries no seq, so it sits +// OUTSIDE the sliding window's sequence space and the window rule does not +// bound it: a sender that pipelines its tail can have END in flight on top +// of a full window. +// +// => frames in flight <= PIPE_MAX_W + 1 +// +// The ring reserves one slot to tell full from empty (see bleRxQueuePush), so +// usable capacity is SLOTS - 1. Hence SLOTS = PIPE_MAX_W + 2. +// +// This previously read 33, which gave 32 usable and was one short of its own +// stated design point ("a full W=32 window plus END") -- the END would be +// rejected at exactly the stall the number was chosen for. 33 is correct for +// PIPE_REORDER_SLOTS, where W + 1 makes seq % SLOTS collision-free; that is +// modular-indexing arithmetic, not queue capacity, and the value looks to have +// been carried across. +// +// Deriving it also right-sizes env:esp32-N4, which sets PIPE_SMALL_DRAM_WINDOW +// (PIPE_MAX_W 16) yet carried a ring sized for a 32-frame window it can never +// receive. +// +// OD_BLE_MAX_FRAME (256) covers pipe <=244, legacy <=232, HA <=244; the GATT +// layer rejects anything larger with ATT 0x0D rather than the app dropping it +// silently. +// +// BLE_RX_QUEUE_SLOTS remains overridable as an escape hatch, not an expected +// fallback: nRF RAM headroom was confirmed sufficient (2026-07-27) and no env +// overrides it. Overriding it BELOW the derived value caps the effective +// PIPE_WRITE window and costs throughput -- a deliberate trade, never a +// link-time discovery. The assert below makes that trade explicit rather than +// silent. +#ifndef BLE_RX_QUEUE_SLOTS +#define BLE_RX_QUEUE_SLOTS (PIPE_MAX_W + 2) +#endif +#define COMMAND_QUEUE_SIZE BLE_RX_QUEUE_SLOTS +#define MAX_COMMAND_SIZE OD_BLE_MAX_FRAME + +static_assert(COMMAND_QUEUE_SIZE - 1 >= PIPE_MAX_W + 1, + "RX ring too small for a full PIPE_WRITE window plus END " + "(usable capacity is COMMAND_QUEUE_SIZE - 1)"); + +struct CommandQueueItem { + uint8_t data[MAX_COMMAND_SIZE]; + uint16_t len; + bool pending; +}; + +// SPSC. Push runs on the stack callback task; peek/consume run on loop(). The +// producer publishes head with RELEASE after the payload lands, so the consumer +// never observes a slot before its bytes are visible. +// +// peek/consume rather than the copying pop the plan sketched: the consumer is +// the only party that may touch a slot until it advances the tail, so handing +// out a pointer is safe and avoids a 256-byte stack buffer plus one memcpy per +// frame -- up to 33 of them per loop pass during a pipe transfer. The peeked +// slot is deliberately mutable: the dispatcher decrypts in place, exactly as it +// did when it was handed the raw ring slot. +// bleRxQueuePush also OWNS the RX logging, for both targets: the arrival hex line +// and the three distinct failure reasons (empty / too large / ring full). Callers +// are stack callbacks and must add no logging of their own -- a copy in each +// transport is what let nRF report a malformed frame as "queue full". It logs at +// arrival, on the callback task, so the timestamp is delivery time rather than +// dispatch time; see the note on the definition for what that costs. +bool bleRxQueuePush(const uint8_t* data, uint16_t len); // false = dropped (logged) +CommandQueueItem* bleRxQueuePeek(void); // nullptr = empty +void bleRxQueueConsume(void); // advance past the peeked slot +uint8_t bleRxQueueHead(void); // producer-side, for pollActivity() +uint8_t bleRxQueueDepth(void); // unconsumed frame count +bool bleRxQueuePending(void); // unconsumed frames waiting + +// Discard unconsumed frames up to `boundary`, a head value captured at the instant +// the departed client's link went down (BleTransport::takeDisconnectedEvent hands it +// out). Consumer-side: call only from the loop task. Returns how many were dropped. +// +// The boundary is what makes this safe, and it is not optional. loop() can be blocked +// for tens of seconds inside an EPD refresh -- long enough for the old client's +// disconnect, the next client's connect, and that client's first command to all land +// before the disconnect is serviced. A "discard everything present now" flush then +// eats the NEW client's frames; observed on nRF as a dropped 0x0080 immediately after +// a reconnect. Frames pushed after the boundary belong to whoever connected next and +// must survive. +uint8_t bleRxQueueDiscardTo(uint8_t boundary); + +// --- TX: command handlers (producer) -> loop() flush (consumer) -------------- +// One definition of the struct, in one place: communication.cpp used to carry +// its own copy plus a MAX_RESPONSE_SIZE_LOCAL constant, so the bound checked +// before the memcpy in the enqueue path lived in a different file from the slot +// it guarded -- two definitions of one type (an ODR violation) that had to be +// edited in lockstep or the guard would admit a response larger than the slot. +#define RESPONSE_QUEUE_SIZE 10 +#define MAX_RESPONSE_SIZE OD_BLE_MAX_FRAME + +struct ResponseQueueItem { + uint8_t data[MAX_RESPONSE_SIZE]; + uint16_t len; + bool pending; +}; + +// Both ends run on loop() today, so no atomics here. +bool bleTxQueuePush(const uint8_t* data, uint16_t len); // false = too large or full +uint8_t bleTxQueueDepth(void); +uint8_t bleTxQueueHead(void); // producer-side, for pollActivity() +bool bleTxQueuePending(void); + +// Drain queued responses to BLE notifications. Called once per loop() pass and -- +// critically -- between commands inside the bounded command drain: at small +// negotiated ack_every (N_eff 1-2) a 33-command drain can emit up to ~32 pipe +// ACKs, which would overflow the 10-slot response ring (dropping the NEWEST +// entry) and leave only stale ACKs, lagging window refunds and collapsing +// throughput. handleReadConfig() calls it between config chunks for the same +// reason. +void serviceBleTx(void); + +#endif // COMMAND_QUEUE_H diff --git a/src/communication.cpp b/src/communication.cpp index eb39673..cc37d9a 100644 --- a/src/communication.cpp +++ b/src/communication.cpp @@ -10,15 +10,14 @@ #include #include +#include "ble_transport.h" +#include "command_queue.h" + #ifdef TARGET_ESP32 -#include "ble_init.h" // NimBLE-Arduino + BLE* aliases -#include +// wifi_service.h only, deliberately: this file talks to the LAN transport +// through opendisplay_lan_send_frame() and needs no WiFi stack types. +// was here solely for two extern declarations that nothing used. #include "wifi_service.h" -extern BLEServer* pServer; -#endif - -#ifdef TARGET_NRF -#include #endif bool isAuthenticated(); @@ -86,50 +85,19 @@ extern uint8_t msd_payload[16]; String getChipIdHex(); float readBatteryVoltage(); -#ifdef TARGET_ESP32 -// ResponseQueueItem / RESPONSE_QUEUE_SIZE / MAX_RESPONSE_SIZE come from structs.h. -// This file previously redeclared the struct with a hardcoded 512 and kept private -// *_LOCAL copies of both sizes, so the guard in esp32_queue_ble_notify_copy() and the -// slot it protects were sized in two different files and had to be edited in lockstep. -extern WiFiClient wifiClient; -extern bool wifiServerConnected; -extern ResponseQueueItem responseQueue[RESPONSE_QUEUE_SIZE]; -extern uint8_t responseQueueHead; -extern uint8_t responseQueueTail; -// Drains the response ring to BLE (defined in main.cpp). handleReadConfig() calls -// this between chunks so a multi-chunk config read never overflows the ring. -extern void flushResponseQueueToBle(); - /** Mirror responses to BLE only when a central is connected; LAN responses go via opendisplay_lan_send_frame. */ -static void esp32_queue_ble_notify_copy(const uint8_t* response, uint16_t len, bool quiet = false) { - if (len > MAX_RESPONSE_SIZE) { - od_log_error("ERROR: Response too large for queue (%u > %u)", len, MAX_RESPONSE_SIZE); +static void queueBleNotifyCopy(const uint8_t* response, uint16_t len) { + // Nothing to queue against with no central attached; serviceBleTx() would + // discard it on the next pass anyway. + if (!ble.isConnected()) { return; } - if (pServer == nullptr || pServer->getConnectedCount() == 0) { - return; - } - uint8_t nextHead = (responseQueueHead + 1) % RESPONSE_QUEUE_SIZE; - if (nextHead == responseQueueTail) { - od_log_error("ERROR: Response queue full, dropping response"); - return; - } - memcpy(responseQueue[responseQueueHead].data, response, len); - responseQueue[responseQueueHead].len = len; - responseQueue[responseQueueHead].pending = true; - responseQueueHead = nextHead; - // The "[BLE][Q:n] TX ..." line above already reports every response and its - // queue depth, so the nominal enqueue (depth 1, drained next loop pass) is - // pure noise. Log only when a backlog is forming and the 10-slot ring is at - // risk of dropping responses. - const uint8_t depth = (responseQueueHead - responseQueueTail + RESPONSE_QUEUE_SIZE) % RESPONSE_QUEUE_SIZE; - if (!quiet && depth >= 2) od_log_debug("BLE: response queue backlog (queue size: %u)", depth); + // No log here. logTxFrame() reports every response with its PRE-enqueue depth, + // so the backlog this used to announce at "depth >= 2 after push" is the same + // event as "[Q:1] or higher" on the line immediately above -- it only ever + // restated the number. bleTxQueuePush logs the failures (oversize / ring full). + (void)bleTxQueuePush(response, len); } -#endif - -#ifdef TARGET_NRF -extern BLECharacteristic imageCharacteristic; -#endif #ifndef BUILD_VERSION #define BUILD_VERSION "1.0.0" @@ -186,56 +154,58 @@ static uint8_t parseFirmwareVersionComponent(unsigned index) { return (uint8_t)n; } -// Builds "