ARQ end-to-end delivery bench: three-ledger harness; count spsc-fat pool-exhaustion drops - #359
Conversation
…ool-exhaustion drops Field failure shape (PixelPilot, 8812AU drone -> 8812EU ground): hardware ARQ logs a packet delivered-after-retry while the RX app never receives it. The existing ACK harnesses (ack_responder_check.sh, ack_txreport_matrix.sh) judge the loop from the TX side's CCX reports alone, so this class was structurally invisible to them. tests/arq_e2e_delivery.sh + arq_e2e_analyze.py close the loop with three per-frame ledgers on one channel: DUT = 8812EU examples/duplex (the one-handle RX+TX topology, ACK responder armed, host ledger via rx.seq), drone-sim = 8812CU txdemo (retrying unicast QoS-Data, per-frame tx.report — the halmac SW_DEFINE tag echoes the payload counter mod 256, so reports join to payloads frame-exactly), witness = 8814AU rxdemo (independent on-air ledger). The ledger identity (ok - acked_undelivered - tail_suspect + dropped_but_delivered = dut_pctrs) held delta-0 on every joinable run, 129k-410k frames each. Measured, both directions: - The hardware ACK is admission-gated on the 8812EU. 150 ms consumer freezes against a ~2.3k fps flood dropped 14,214 frames inside the receiver, in 143 contiguous stall-window gaps - and every one of them was reported ok=0 and retried by the peer. Zero ACKed-but-undelivered in ~460k ok'd frames across all default-ring arms: backpressure that reaches the chip keeps ARQ truthful. - The same stalls under RxMode::SpscFat reproduce the field failure: at pool exhaustion the pump preserves its never-block invariant by re-arming the URB with the received buffer and dropping the payload - after the chip admitted and ACKed the frame. 5,667 ACKed-but-undelivered (~99% witness-confirmed on air), replicated within 2%. The TX peer logs them delivered and never retries; with FEC off nothing else can repair them. So the drop is now counted (AsyncRxShared::pool_dropped, emitted in rx.ring) instead of silent, the ARQ caveat is documented at the mode comment and in CLAUDE.md, and examples/duplex grows the rx.seq/pctr ledger plus the consumer-cost knobs (DEVOURER_RX_SINK_SPIN_US / _STALL_MS / _STALL_EVERY, mirroring rxdemo) so the duplex ground-station topology stays benchable. Two traps the analyzer encodes: a killed event stream truncates its stdout tail and masquerades as end-of-run loss (TAIL_GUARD=512 excludes it; the harness runs per-line flush), and rx.ring telemetry is structurally blind at the loss instant because the frozen pump is what would have sampled it - the per-frame ledgers, not the ring counters, carry the verdict. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
PR Summary by QodoAdd ARQ end-to-end delivery bench and count spsc-fat pool-exhaustion drops
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
1. arq_e2e_analyze.py mixes stdout
|
… rx counter, teardown-gated pool_dropped, /run blacklist Qodo review round on #359, all five findings addressed and re-validated on air (third spsc-fat replication: acked_undelivered 5,468 vs 5,667/5,570 on the prior runs, pool_dropped 3,477 — the counter still tracks exhaustion drops with the teardown gate in place): - arqe2e.verdict now emits compact separators — machine events must stay in the grep-able {"ev":"name",...} form (docs/logging.md). - DEVOURER_RX_SINK_STALL_EVERY=0/garbage no longer divides by zero: parse clamps to the 100 default, in duplex and in rxdemo's identical knob. - duplex g_rx_seen is atomic (the RX callback can run on the TX thread's event pump too) and the stall decision uses the increment's return value. - pool_dropped counts only resubmit-eligible drops: a teardown-window frame is intentional loss, not an overload signal. - The temp module blacklist moved to /run/modprobe.d (read like /etc/modprobe.d, tmpfs) so it self-cleans on reboot even after SIGKILL — it still must exist during the run because modprobe -r does not survive the re-enumeration claim_interface_then_reset triggers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
/review |
|
Code review by qodo was updated up to the latest commit 111a067 |
… fallback, checkout-scoped cleanup - A run spanning fewer than 2x TAIL_GUARD indices now refuses to conclude (INCONCLUSIVE-SHORT-RUN, exit 3, verdict field in the machine event) instead of letting the tail window silently swallow every missing frame into tail_suspect and print a vacuous NOT-REPRODUCED. - The exhaustive base-scan fallback scores candidates on a 2k-report sample (a wrong base matches ~1/256 at random, so the sample separates decisively) and re-scores only the winner in full — non-joining logs now fail fast instead of costing O(m_hi * n_ok). - The cleanup backstop pkills full paths under this checkout's build dir rather than bare comm names, so a root-run bench can no longer kill unrelated duplex/txdemo/rxdemo instances elsewhere on the host. Validated: full-run parity on the round-1 logs (same au, ~5 s), a 341-index truncation triggers the INCONCLUSIVE path, and a fourth on-air spsc-fat replication (acked_undelivered 5,739; series 5,667/5,570/5,468/5,739) exits with zero leftover processes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
/review |
|
Code review by qodo was updated up to the latest commit 8aa6031 |
The one JSONL machine event the analyzer emits now flushes explicitly, so it lands atomically even under a live consumer. The human report stays on stdout by design: this is an offline report generator the harness tees into report.txt — the same contract as rxq_analyze.py / pp109_starve_analyze.py / ack_txreport_analyze.py — not a demo's runtime event plane, where the two-plane stdout/stderr split applies. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
/review |
|
Code review by qodo was updated up to the latest commit 95ab617 |
…q emitter gains duplex rx.ring documents the reorder-pool/spsc-fat mode values, qdepth, and the new pool_dropped counter — including what a pool_dropped frame means under hardware ARQ (chip-ACKed, so the TX peer counts it delivered) and the counters' structural blind spot while the pump is frozen, which the per-frame rx.seq ledger covers. rx.seq is emitted by duplex too, where the SA gate is required. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
/review |
|
Code review by qodo was updated up to the latest commit 2559ec7 |
… analyzer verdict tests/pp109_uplink_feeder.py was still untracked from the pp109 bench set, so the harness could not run from a clean checkout — it is the PixelPilot-shaped burst generator (SET_RATE phase markers, per-phase burst counts) both benches share. And the analyzer's exit code (3 = INCONCLUSIVE-SHORT-RUN) now survives the tee: the harness exits with the verdict. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
/review |
|
Code review by qodo was updated up to the latest commit 7f26e99 |
…cket, feeder BrokenPipe exit - SPAN and the preflight sleep compute in float (awk) — fractional WARMUP_S/PHASE_S no longer truncate the drone's timeout short of the feeder's real runtime. - An undelivered frame whose nearest delivered neighbours straddle a phase edge lands in its own "boundary" row instead of silently biasing the earlier phase's count. Total acked_undelivered is invariant (re-checked on the round-1 logs: 5,739). - The feeder exits quietly when its pipe reader goes away (bench teardown or duplex crash) instead of tracebacking; os._exit skips the interpreter's flush-at-exit re-raise. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
/review |
|
Code review by qodo was updated up to the latest commit 37b67ea |
…ly; rx.ring sync-mode caveat pool_dropped deliberately counts every received frame discarded on the spsc-fat drop path — pool exhaustion and failed re-arm alike, both post-ACK host drops; the re-arm failure stays separable via resubmit_fail. The comment and docs/logging.md now say exactly that, and the rx.ring row notes sync mode emits a reduced line without the pool fields. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
/review |
|
Code review by qodo was updated up to the latest commit e489327 |
….ring field list find_base tests membership against the two ledgers' key views instead of materializing a ~1M-int union per analysis, and the rx.ring row now lists the sync-mode reduced line exactly as emitted (pool_free pinned at -1; no qdepth/pool_dropped/completions/empties) — verified against the emit site. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
/review |
|
Code review by qodo was updated up to the latest commit 8e15c97 |
The drone timeout is an upper bound on the feeder's runtime, so fractional WARMUP_S/PHASE_S round up rather than down — truncation leaned on the slack constant to stay safe. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
/review |
|
Code review by qodo was updated up to the latest commit ec8d765 |
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
/review |
|
Code review by qodo was updated up to the latest commit 24ae535 |
$BUILD is caller-controlled and pkill -f matches an ERE — a path like ".../build+asan" would broaden the match past this checkout's binaries. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
## Problem The head-pinned gate rule makes every review-response push invalidate the review evidence: push a fix → summon `/review` → the bot re-scans the whole diff → it opens a fresh batch of findings → fix → push → repeat. On #359 this ran **eleven rounds**, converging on one-line style nits (float truncation vs ceil, `max` over key views) long after the substantive findings were exhausted. The re-review passes also re-anchor stale comment bodies and open re-litigations of already-dismissed findings, so the loop does not terminate on its own. ## Change The gate now requires: 1. **At least one Qodo review on the PR** — the first, whole-diff pass, which is where the review value is concentrated (on #359: a SIGFPE, a data race, a missing file dependency, a host-wide pkill — all from the first two passes). 2. **All Qodo-authored threads resolved** — unchanged. Every finding still must be addressed or explicitly dismissed with rationale in its thread before resolution; the accountability mechanism stays. Dropped: the head-oid match and the update-in-place marker-comment fallback (and the `issues: read` permission only that fallback needed). Follow-up commits after the review are maintainer judgment — the same contract as a human reviewer who does not re-review every fixup. Thread pagination, any-comment bot attribution, and the `skip-qodo-gate` escape hatch are unchanged. ## Validation The workflow parses (same trigger set as the current file, which is the parser-accepted form from #357), and this PR itself runs under the new logic via the merge ref: its gate must see Qodo's first review of this PR plus resolved threads to go green — a live demonstration of exactly the intended flow. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…keep the hardware-ARQ contract under consumer stalls (#366) Closes #361. ## Problem The spsc-fat ring keeps the USB ring armed by discarding the payload at pool exhaustion — after the chip already admitted **and ACKed** the frame. Under a hardware-ARQ peer that converts receiver congestion into silent, unrecoverable loss the TX side logs as *delivered*: the exact field failure from the PixelPilot ACK/retry deployment, reproduced on the bench in #359 (5,468–5,739 ACKed-but-undelivered per escalation run). ## Change `PoolExhaust` policy (`DeviceConfig::Rx::pool_exhaust`, `DEVOURER_RX_POOL_EXHAUST=backpressure|drop`, **default backpressure**): - **backpressure**: at exhaustion the completed URB **parks bufferless** — its payload still reaches the consumer queue (nothing is lost host-side), the ring shrinks, the chip FIFO fills and the chip **declines further ACKs**, so overload loss happens chip-side where the ARQ loop sees and retries it. The consumer's buffer-return path re-arms parked URBs; the submit is held under `pool_mu` so teardown's `parking_closed` flip strictly excludes a late launch of an uncancellable infinite-timeout URB, and teardown retires the parked set before its cancel pass (parked URBs are not in flight — the active-drain would otherwise hang). Parked URBs stay counted in `active`: the pump loop must keep running while the whole ring is parked. - **drop**: previous behaviour, kept for latency-over-integrity consumers, still counted (`pool_dropped`). Telemetry: `rx.ring` gains `pool_stalls` (cumulative park events). Docs: `docs/logging.md` schema row + the CLAUDE.md ACK-horizon paragraph. Harness: `DUT_POOL_EXHAUST` passthrough in `tests/arq_e2e_delivery.sh`. ## Validation (on-air, 8812EU DUT / 8812CU drone-sim / 8814AU witness, per-seq three-ledger accounting) Escalation conditions throughout: 40 µs/frame consumer cost + 150 ms consumer freeze every 3000 frames vs a ~2.3 k fps retrying unicast flood — the conditions that reliably produce ~5.5 k contract violations on the drop policy. | arm | acked_undelivered | pool_stalls | pool_dropped | note | |---|---|---|---|---| | backpressure, escalation ×3 | **0 / 0 / 0** | 1,428 / 1,433 / 1,454 | 0 | contract holds; min_armed→0 now *visible* in telemetry (the pump isn't the stalled thread) | | drop control (same conditions) | 5,465 | 0 | 3,484 | knob genuinely selects; matches the pre-change 5,468–5,739 series | | backpressure, no-stall parity | 0 | 0 | 0 | parked path dormant; ok 99.68% ≈ baseline | Bonus measured property: burst-phase ok% under backpressure is ~98.6 vs ~93 for plain async under identical stalls — the pool still absorbs most of each stall and only the tail converts to chip backpressure, so the mode keeps spsc-fat's smoothness while restoring async's honesty. Visible (retryable) drops per burst phase: ~150 vs async's ~700–900. All five runs exited cleanly through the parked-retire teardown path; `ctest` 48/48. Residual noted for the record: an ASan-instrumented run of the parked-URB lifecycle (per #361's teardown checklist) has not been done — the path was exercised by five hardware teardowns, four of them with parked URBs plausible at kill time. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
) Steps 1 and 4 of #362 (issue stays open for the rate-fallback and deadline research, steps 2–3). ## What's added - **`tests/arq_retry_sweep.sh`** — runs the #359 arq_e2e bench once per `DEVOURER_TX_RETRY_LIMIT`, tabulates delivered% / drops / mean retries (the airtime proxy), and hands each run to the residual analyzer. - **`tests/arq_fec_dimension.py`** — post-ARQ residual gap-length distribution per run: the quantity a wfb-style (K,N) block FEC must cover. Header states the caveats: single-gap-per-window mapping, near-field scope, and unreported-frames-count-as-lost (conservative — the safe direction for dimensioning). - **`docs/scheduled-mac.md`** — the measured curve + recommendation next to the existing retry-knob paragraph. ## Measured (8812CU retrying TX → 8812EU duplex ground airing PixelPilot-shaped feedback bursts, ~1 k fps collision regime, near-field) | limit | delivered% | drops | mean retries | residual | gap P99.9 | FEC to cover | |---|---|---|---|---|---|---| | 3 | 99.72 | 352 | 0.035 | 0.26% | 62¹ | — | | 8 | 99.97 | 34 | 0.039 | 0.03% | 3 | **K=8 → N=11** | | 16 | **100.00** | 0 | 0.054 | 0.00% | — | residual gap-free | | 32 | 100.00 | 0 | 0.063 | — | — | +17% retries over 16 for nothing | ¹ single 62-frame gap in one run — one occurrence, P99 is still 3; not treated as a distribution feature. Queue-time p99 is flat (~10–11 fw units) across all limits; only the rare worst case doubles (18 → 35) and then stops growing — deep retries are rare enough that latency does not enter the trade. **Recommendation carried in the docs:** `DEVOURER_TX_RETRY_LIMIT=16` on an ARQ link (zero bench loss at +5.4% retry airtime), or `8` plus a light FEC floor (K8/N11-class) where airtime is precious. Retries are backoff-spaced, so the field default of 3 can burn entirely inside one 2–3 ms ground-station feedback burst — that is where its 0.26% residual comes from. Also re-derived from the recorded #359/#366 ledgers (posted to #362): the stall-regime contrast — plain-async stalls produce ~110-frame contiguous gaps no sane FEC rate covers (rate 0.07), while the #366 backpressure policy bounds the same stalls to ≤13-frame gaps. FEC should be sized to the collision residual; stall regimes are a ring-policy problem. ## Validation Four full on-air bench runs (one per limit, ~129 k frames each, frame-exact three-ledger accounting from #359); analyzer parity-checked against the exploratory version on the recorded baseline. `ctest` 48/48 (no library changes in this PR). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…s, dead MISSED_RPT_NUM (#363 step 1) (#368) Step 1 of #363 (the issue stays open for SPE_RPT sampling and windowed RX receipts, steps 2–3). ## What the recorded #359/#366/#367 ledgers + a 3-point envelope sweep establish **The 53% coverage collapse is a fw-side CCX emission ceiling, and the fw does not account its own drops.** | demanded fps | coverage | achieved reports/s | |---|---|---| | 1.04 k | 100.0% | 1.04 k (unsaturated) | | 1.26 k | 99.4% | 1.25 k — the knee | | 1.82 k | 77.2% | 1.40 k | | 2.39 k | 53.4–55.6% | 1.27–1.33 k | `coverage ≈ min(1, ceiling/fps)` with ceiling ≈ **1.3–1.4 k reports/s** fits every measured point (8812CU TX). **Attribution to emission-time, not transport:** 99.3% of unreported frames sit in SW_DEFINE tag gaps ≤ 2 (95 k singles + 47 k doubles at 2.39 k fps, max gap 28) — interleaved per-report loss, the signature of a rate limiter at report generation. Transport-batch loss (a dropped C2H aggregate) would lose contiguous dozens per event; the data shows the opposite. The tag-unwrap ground truth is trusted because the same runs' ledger joins matched 203,799/203,799 ok-frames in #359. **MISSED_RPT_NUM is dead on this fw:** constant 4 on every report across all runs — including 100%-coverage runs where nothing was dropped. Parse verified bit-exact against the 8822B/8822C/8822E vendor headers (`CCX_RPT_GET_MISSED_RPT_NUM`: dword +0x4, bits [15:13] — identical in all three trees), so this is fw behaviour, not a devourer bug. Tag gaps are the only drop signal available. ## What's in the PR - `tests/txrpt_coverage_attrib.py` — coverage, gap-shape histogram, missed-field distribution per recorded run (refuses tag-less J1 ledgers loudly). - `src/TxReport.h` — the `missed_rpt` field documents its measured deadness. - `docs/scheduled-mac.md` — the envelope + the consequence for accounting layers: keep the demanded report rate under ~1.3 k/s (SPE_RPT 1-in-N — step 2) or treat report-less frames as "unknown". ## Validation Three fresh on-air envelope runs (1.5 k/2 k/3 k fps demanded) + re-analysis of four recorded runs; tool parity-checked against the exploratory pass; `ctest` 48/48 (comment-only library change). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Motivation
Field report from the PixelPilot ACK/retry experiments (8812AU drone → 8812EU ground,
DEVOURER_TX_RETRY_LIMIT=3, responder armed, WFB FEC K=N=1): the drone'stx.reportsays a packet was delivered after a retry, yet the ground app never receives it. Our existing ACK harnesses (ack_responder_check.sh,ack_txreport_matrix.sh) judge the loop from the TX side's CCX reports alone — the responder's own host delivery was never counted, and the responder was always a quiet monitor. This failure class was structurally invisible to them.What this adds
tests/arq_e2e_delivery.sh+tests/arq_e2e_analyze.py— three per-frame ledgers on one channel: DUT = 8812EUduplex(the PixelPilot one-handle RX+TX topology: RX loop + feedback bursts, ACK responder armed), drone-sim = 8812CUtxdemo(retrying unicast QoS-Data, per-frametx.report), witness = 8814AUrxdemo. The halmac report tag echoes the payload counter (mod 256), so reports join to payloads frame-exactly; the ledger identity (ok − acked_undelivered − tail_suspect + dropped_but_delivered = dut_pctrs) held delta-0 on every joinable run (129k–410k frames each).examples/duplex— grows therx.seq/pctr host-delivery ledger (mirrors rxdemo,DEVOURER_RX_PCTR+DEVOURER_RX_AGG_SA) and the consumer-cost knobs (DEVOURER_RX_SINK_SPIN_US/_STALL_MS/_STALL_EVERY).src/UsbTransport.cpp— the spsc-fat pool-exhaustion drop is now counted (pool_droppedinrx.ring; it was silent) and the ARQ caveat is documented at the mode comment +CLAUDE.md.Measured (ch36, MCS3/512 B, near-field −10 dB, retry_limit 3)
pool_droppedcounter = 3,535 aggregates ≈ matchesBoth directions of the contract, bench-proven: the hardware ACK's horizon is chip-FIFO admission. On the 8812EU, congestion upstream of admission declines the ACK — the ARQ loop sees it and recovers it (zero ACKed-but-undelivered in ~460k ok'd frames on the default ring). Any host stage that drops on a full queue while keeping URBs armed (spsc-fat at pool exhaustion) converts the same loss into ACKed-but-undelivered that the TX peer logs as delivered and never retries — the field failure, reproduced and localized. Under ARQ or delivery accounting, prefer backpressure to the chip over drop-on-full host buffering, and watch
pool_dropped.Traps the analyzer encodes
tail_suspect, reported separately). Measured: 19 consecutive false "losses" at the exact end of a run.rx.ringtelemetry is structurally blind at the loss instant: the frozen pump is what would have sampled it (armed=7/empties=0 throughout a run that dropped 14k frames). The per-frame ledgers, not the ring counters, carry the verdict.Validation
Five on-air runs on the bench (8812EU DUT / 8812CU drone-sim / 8814AU witness), ~1.3M frames total;
ctest48/48 on the branch. The spsc-fat replication run exercises the new counter live.🤖 Generated with Claude Code