feat(util): SnapshotPublisher — lock-free single-writer seqcount snapshot

[sritchie]

SnapshotPublisher — foundation for the rate-decoupled filter stack

Header-only template class implementing the standard Linux-kernel-style seqcount snapshot pattern in C++20, plus a 10-test native suite that covers the race semantics. No callsite changes — this is pure infrastructure that future PRs build on.

Step 1 of the multi-commit plan in `local-plans/PLAN_RATE_DECOUPLED_FILTER.md` that decouples sample rate, AHRS update rate, and log capture rate. Each future migration commit introduces one specific publisher (`g_AhrsSnapshot`, `g_FlapSnapshot`, etc.) and migrates one or more consumers off `xAhrsMutex` onto this primitive.

What it does

```cpp
struct AhrsSnapshotPayload { /* trivially copyable */ };
SnapshotPublisher g_AhrsSnap;

// One writer task (e.g. ImuReadTask), lock-free:
g_AhrsSnap.publish(payload);

// Many readers (web handlers, display, log producer), lock-free + coherent:
const AhrsSnapshotPayload s = g_AhrsSnap.read();

// Or with explicit bailout for real-time-deadlined callers:
AhrsSnapshotPayload s;
if (g_AhrsSnap.tryRead(s)) { /* coherent / } else { / skip */ }
```

Why seqcount instead of a mutex

Today's pattern (`xAhrsMutex`, established by PR #634 closing #520) requires every web handler / display / log consumer to take the same mutex as `ImuReadTask`. Under web load this contention produced 18-20 ms IMU stalls every config save (PR #647 bench data) and 50 ms-class web-handler timeouts. The seqcount pattern eliminates the cross-core contention entirely:

• Writer never blocks on readers
• Readers never block writers or each other
• Coherent reads guaranteed (the version counter catches any tearing)
• Realistic ~150-200 ns per read on ESP32-S3 (one acquire-load + memcpy + one acquire-load, including barriers). Still 10-50× faster than the mutex it replaces (mutex is ~1-2 μs uncontended, ~10 μs contended).

Memory ordering

Initial implementation without explicit `atomic_thread_fence` calls between the memcpy and the seqcount loads produced torn reads in the race tests. The current implementation adds defensive release/acquire fences around the memcpy on both sides. These are STRICTLY REDUNDANT with the release-store / acquire-load semantics already on the version counter — kept as belt-and-suspenders against a future edit that downgrades the atomic ops to relaxed by accident. The header documents which sync is load-bearing vs defensive.

Tests

10 native tests, all passing:

```
test_default_read_returns_zero_payload
test_publish_then_read
test_most_recent_publish_wins
test_tryread_succeeds_when_idle
test_version_for_telemetry_advances
test_concurrent_seq_is_monotonic # writer+reader threads, 200000 reads, asserts ==0 torn
test_concurrent_two_pattern_integrity # alternating 0xAA/0x55, 100000 reads, asserts ==0 mixed
test_tryread_coherence_under_pressure # asserts every true return is coherent
test_pointer_bearing_payload_round_trip
test_version_wrap_arithmetic # parity + equality across uint32_t wrap
```

The strict-zero tolerance on the concurrent coherence tests is load-bearing: `read()` is documented as "NEVER returns torn data," and any nonzero count is a real race in the seqcount, not test flakiness.

Verification

```
pio run -e esp32s3-v4p # SUCCESS, header-only template (no firmware bloat)
pio test -e native # 1179 cases: 1178 succeeded, 1 skipped
```

Bulldog review feedback addressed

Two independent reviewers converged on the same fixes; a second commit on this branch (`d8280524`) addresses all of them:

• Test tolerances tightened from "<2" torn reads to "==0" (the original "<2" was masking, not defending against flake — the implementation is genuinely clean and the strict assertion passes)
• Fence comments rewritten to identify which is load-bearing vs defensive belt-and-suspenders
• read() docs now warn DEADLINED CALLERS MUST USE tryRead()
• Per-read latency claim corrected from optimistic "~6 ns" to realistic "~150-200 ns" (still 10-50× better than the mutex)
• New version-wrap test locks the property that the seqcount math is correct across uint32_t wrap (~59 days at 416 Hz)
• File-level comment fixed (previously promised a synthetic-bailout test that didn't exist)

Migration sequencing

Plan now calls for DataServer to migrate first (validates the lock-free win against PR #647's measured 18-20 ms IMU stalls under web load), then LogSensor::Write, then web handlers, then Housekeeping. Original plan had Housekeeping first; per bulldog feedback, DataServer is the right load-bearing first migration since it tests against the actual observed contention scenario.

Two-writer constraint

Single-writer is a hard invariant of this primitive. The original plan's "two writers publishing to one snapshot" option (open question 1 in the plan doc) is unsafe — two interleaved `version_.load(relaxed) → store(v+1) → store(v+2)` sequences can leave the counter persistently odd. Plan now mandates separate publishers per writer (e.g. `g_AhrsPredictSnap` and `g_AhrsCorrectSnap` in the future predict/correct split), composed at a view layer.

Related

• `local-plans/PLAN_RATE_DECOUPLED_FILTER.md` — composed sequence this commit is step 1 of (local-only per CLAUDE.md doc rules)
• Decouple log producer task from sample/AHRS tasks (configurable log rate) #645 — LogProducerTask architecture; this primitive is its foundation
• 416 Hz hardening: deferred follow-up work #644 — atomic AHRS snapshots called out as a deferred 416 Hz hardening item
• AHRS: drop dead constructor arg, require explicit Init() before use #649 — AHRS lifecycle cleanup; folds in once migrations begin
• PR Add 416 Hz Log Rate option (experimental) #647 (merged) — 416 Hz mode whose contention pattern this work addresses

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[github-actions]

Firmware Artifacts

Main firmware (Gen3 box)

Variant	Download
V4P (Phil's box)	onspeed-V4P-4.23.1-dev.11+f3a830f.zip
V4B (Bob's box)	onspeed-V4B-4.23.1-dev.11+f3a830f.zip

Each .zip contains three files: firmware.bin, bootloader.bin, and partitions.bin.
For an OTA update, you only need firmware.bin — upload it at http://onspeed.local/upgrade.
For a USB flash (initial setup or recovery), you need all three — see the flashing docs.

External display firmware

Board	Download
M5Stack Basic	m5-display-basic-4.23.1-dev.11+f3a830f.zip
M5Stack Core2	m5-display-core2-4.23.1-dev.11+f3a830f.zip
huVVer-AVI ⚠️ unverified — see #298	m5-display-huvver-avi-4.23.1-dev.11+f3a830f.zip

Each .zip contains firmware.bin, bootloader.bin, and partitions.bin. For an OTA update on M5Stack, hold Button B during boot to enter WiFi update mode and upload firmware.bin. For a USB flash, see the external display docs.

The huVVer-AVI binary is built but not yet validated on real hardware — see the bring-up checklist for what to verify on first flash.

X-Plane plugin

Platform	Download
macOS	AOA-Tone-FlyOnSpeed-mac_x64.xpl
Windows	AOA-Tone-FlyOnSpeed-win_x64.xpl
Linux	AOA-Tone-FlyOnSpeed-lin_x64.xpl

Drop the .xpl into X-Plane 12/Resources/plugins/AOA-Tone-FlyOnSpeed/<arch>/. Restart X-Plane to load. See the X-Plane plugin docs for install details and usage.

---

Built from feat/snapshot-publisher at d828052499697f5be76d20553421fa6c2c9dd665.

Downloading these artifacts requires a GitHub login. Artifacts expire after 30 days.

[sritchie]

Review against docs/ARCHITECTURE_DECOUPLING.md and the decoupling roadmap

Bottom line: this is exactly the primitive the roadmap calls for, the implementation is solid, and the scope of "infrastructure only, no callsites" is the right way to land it. Ship it. Details and a couple of non-blocking concerns below.

Where this fits in the bigger plan

The architecture doc has been arguing for a sink-tier composed snapshot (LiveDataFrame) for several reconciliations, and #645 sharpened the picture with the seqcount-versus-mutex motivation. The two designs combine cleanly: SnapshotPublisher<T> is the concurrency primitive; per-stream snapshots feed a LiveDataFrame composition helper that downstream sinks consume. The five-place-in-production pattern the doc has been tracking now has its sixth place — the formal primitive. Python LiveSnapshot (#380), JS M5State (#526), C++ four-stage AHRS (#590), independent UART tasks (#622), in-production snapshot pattern at LogSensor (#634), producer/consumer struct handoff (#625), and now this. The seam between #645's lock-free reads and the doc's composed-struct view becomes obvious once this lands.

This unblocks the right next things in the right order:

• DataServer can migrate off xAhrsMutex (validates the lock-free win against the measured 18-20 ms IMU stalls from PR Add 416 Hz Log Rate option (experimental) #647 bench data).
• LogProducerTask can be built (closes Decouple log producer task from sample/AHRS tasks (configurable log rate) #645 properly).
• The composed LiveDataFrame builder reads N lock-free per-stream snapshots and assembles, with no mutex.
• AoaEstimates (parallel estimators) gets a foundation: multiple publishers for different AOA estimates, view-layer fusion.

Implementation review

Read the header end-to-end and the tests through line 280. Notes:

Memory ordering is correct. The release/acquire pairing on version_.store(release) and version_.load(acquire) is sufficient for the seqcount invariant under the C++20 memory model. The release-store at v+2 synchronizes-with the reader's acquire-load that observes v+2 (or later), and that happens-before chain runs through both memcpys via program order. The explicit atomic_thread_fence calls are conservatively redundant — they don't hurt but they aren't strictly required given the release/acquire on the atomic ops themselves. If the empirical "without fences we got torn reads" observation came from an earlier draft that used relaxed somewhere, the fences are belt-and-suspenders insurance that's worth keeping for defensive style. If it came from something else, worth double-checking — but the code as written is sound either way.

Tests cover the right invariants. The two-pattern integrity test (alternating AAAA/5555) is the cleanest of the bunch — any torn read produces a non-uniform observed payload, which the test catches. The tryRead_coherence_under_pressure test correctly asserts the contract that matters (never claim coherence on a torn payload) without over-asserting on bailout rate. The tolerance of <2 torn reads in 200000 is generous to avoid CI flakiness; on the actual code path the count should be 0.

Trivially-copyable static_assert is right. The doc comment about pointer-bearing payloads being safe-as-values (because the seqcount catches torn reads) is correct and worth keeping. One minor: std::atomic<int> inside a payload is technically trivially-copyable in C++20 but memcpy'ing it has implementation-defined semantics. Unlikely to bite us — we won't put atomics in snapshot payloads — but the doc could note "POD-of-non-atomic-fields" as a tighter guideline.

Namespace placement (util/) is right. This is a domain-agnostic concurrency primitive, not a wire-format codec. proto/ would be wrong; util/ is correct alongside Crc.h and OnSpeedTypes.h.

Non-blocking concerns worth flagging

1. The single-writer constraint is real and worth defending. The header comment correctly says "Behavior is undefined if more than one task calls publish() on the same instance concurrently." But the local-plan commit 6 (move AHRS predict to ImuReadTask, correct to SensorReadTask, both publishing the same g_AhrsSnapshot) violates this. If two tasks call publish() concurrently, the version_.load(relaxed) → store(v+1) → store(v+2) sequence can be interleaved between the two writers and produce a corrupted version counter (v+1 from writer B landing between writer A's v+1 and v+2, leaving the counter persistently odd or out-of-sequence). The "two writers, same publisher, coordinated by rate" claim in the plan is not safe under this primitive's invariants.

The fix when commit 6 comes around is either (a) two separate publishers (g_AhrsPredictSnap, g_AhrsCorrectSnap) with readers picking by version, (b) a brief writer-side mutex around publish() (defeats some win), or (c) move both predict and correct publishes into the same task (which is what the post-#622 topology arguably already enables — SensorReadTask could host both). Worth resolving before commit 6 lands; not blocking this PR.

2. read() is unbounded; consider documenting it more loudly as "non-realtime only." The pathological case is a writer preempted between W1 and W4 (e.g., ImuReadTask hit by a 10 ms WiFi interrupt mid-publish). Every reader spins for 10 ms. The bounded tryRead() exists for realtime callers — the header documents this well. The risk is a future contributor reaching for read() from a deadlined consumer because it's the friendlier API. Suggest: add a one-liner to read()'s doc comment saying "DEADLINED CALLERS MUST USE tryRead() — read() may spin if the writer is preempted mid-publish."

3. No version-wrap test. At 416 Hz with version_ += 2 per publish, the counter wraps every ~59 days of continuous operation. Not realistically reachable in a single flight, and the seqcount math is wrap-correct (unsigned arithmetic, v1 == v2 comparison is correct under wrap, the parity check v1 & 1u is correct under wrap). But a one-line test asserting (0xFFFFFFFE → 0xFFFFFFFF → 0) doesn't spuriously match would lock the property. Not blocking; the math is right.

4. The "6 ns per read on ESP32-S3" estimate in the PR description is optimistic. A 100-byte memcpy on Xtensa is ~25-40 cycles (~170 ns at 240 MHz) plus two acquire-loads with memw barriers (~5 cycles each). Realistic per-read is closer to 150-200 ns. Doesn't change the design; the win over a mutex (which has ~1-2 µs overhead uncontended, ~10 µs contended) is still ~10×-50×. Just a precision note for any future PERF discussion.

Recommendations going forward

When the next migration PR comes — and I think DataServer should be first, not LogSensor — it should:

1. Bench the actual 18-20 ms IMU stall measurement from PR Add 416 Hz Log Rate option (experimental) #647 against the post-migration state. This is the load-bearing validation that the lock-free primitive does what it claims. If the IMU stall doesn't measurably shrink under the same web-load scenario, something else is going on and we need to know before fanning out.
2. Keep the differential-test discipline from PR Web UI: port /calwiz to Preact + /api/calwiz/save with differential test #373 (test_calwiz_save_diff) — assert byte-equality of the JSON output before and after the migration, against a recorded fixture. This is what makes the migration provably behavior-preserving.
3. Update docs/ARCHITECTURE_DECOUPLING.md step Add PlatformIO build + versioning #1 to reflect the merged design (lock-free seqcount under, composed LiveDataFrame view on top). I'll do that pass after this lands.

The architecture-decoupling doc will get an update reflecting this PR as the realization of step #1's concurrency primitive. The plan is converging.

[sritchie]

Addressed both bulldog reviews in commit d828052. Concrete changes:

• Test tolerances tightened from <2 to ==0 on the concurrent coherence tests. The original tolerance was masking — the implementation is genuinely clean, the strict assertion passes, and any future failure is signal to investigate, not flake to absorb.
• Fence comments rewritten to be honest about what they actually do. The standalone atomic_thread_fence calls are STRICTLY REDUNDANT with the release-store / acquire-load semantics that surround them under the C++20 model. Kept as belt-and-suspenders against a future edit that accidentally downgrades a version_.store/load to relaxed. The previous comments implied the fences were load-bearing, which would mislead the next reader.
• read() now warns DEADLINED CALLERS MUST USE tryRead() to prevent a future contributor reaching for the friendlier API from a real-time path.
• Per-read latency claim corrected from optimistic "~6 ns" to realistic "~150-200 ns" (still 10-50× better than the mutex; just not the optimistic number).
• New test_version_wrap_arithmetic locks the property that the seqcount math is correct across uint32_t wrap (~59 days at 416 Hz).
• File-level comment fixed (previously promised a synthetic-bailout test that didn't exist).

Plan doc updates that flow from the second review:

• Two-writer constraint resolved as definitive: commit 6 ships separate g_AhrsPredictSnap + g_AhrsCorrectSnap publishers, composed at the view layer (LiveDataFrame pattern). The "two writers on one snapshot, coordinated by rate" option was unsafe — the bulldog correctly identified that the version_ counter can be left persistently odd by an interleave between two writers' load(relaxed) → store(v+1) → store(v+2) sequences.
• Migration sequencing flipped: DataServer migrates first, not Housekeeping. DataServer is the load-bearing case (validates the lock-free win against PR Add 416 Hz Log Rate option (experimental) #647's measured 18-20 ms IMU stall under web load). Housekeeping was the smallest-blast-radius option, which is the wrong heuristic for a foundational primitive — we want the migration that LOAD-TESTS the new design, not the one that minimally exercises it.

Tests: 10 cases (was 9), all passing including the strict-zero tolerance. Native suite: 1178/1179 (1 skipped), no regressions. pio run -e esp32s3-v4p: SUCCESS.

The bulldog review at #653 (comment) is the one that surfaced the two-writer-on-one-publisher unsafety. That alone was worth getting before this lands.