ARQ × A-MPDU measured: contract holds, CCX telemetry and goodput do not

[josephnef]

Executes the #364 measurement plan end-to-end on the bench rig (8812CU drone → 8812EU DUT duplex, 8814AU witness; MCS3/512 B unicast, hardware ACK, retry 8, batch-8 feed via send_packets).

Findings

1. The delivery contract survives aggregation. acked_undelivered = 0 in every BA arm (A-MPDU 0/31) exactly as in every per-frame-ACK arm — the ACK-horizon law is unchanged: admission-gated, declines under congestion, no phantom ACKs introduced by the BA session.

2. Per-frame CCX accounting does NOT survive AGG_EN (the new negative result, now in docs/aggregation.md):

metric	control (no agg)	BA arm
SPE_RPT reports (N=2 sampling, same span)	49,636	19,516 (−61%)
tag-delta distribution	uniform ×2	patterned 2×4537 / 4×6022 / 6×3912 / 8×5044
reports with retries>0	healthy tail {1:633, 2:239, …}	zero
frames with multiple reports	0	0

Reports are suppressed in whole-aggregate bunches, retransmission cycles are invisible (BA retries happen below CCX), and the multi-report-per-MPDU model never materializes. The SW_DEFINE tag-gap drop signal is therefore accounting-grade only for un-aggregated frames; under A-MPDU the windowed RX receipts (#363) are the only delivery truth.

3. No goodput win at the ARQ shape — a measured loss. At 4 k fps saturation demand, BA delivered −8% vs the identical un-aggregated feed (208,792 vs 226,691 frames). The docs' +30% is a high-MCS broadcast near-PHY-ceiling number; at MCS3 the preamble amortization is small and the BA machinery costs more than it saves. Both numbers now sit side by side in docs/aggregation.md + the CLAUDE.md pointer.

Recommendation shipped in docs: do not enable A-MPDU on the ARQ/FPV link as measured; if revisited (high-MCS, receipts-only accounting), the analyzer is ready — see below.

Code

• tests/arq_e2e_analyze.py: per-tag frame reconciliation (last-state-wins, max-retries, multi-report counter) so a multi-report BA future stays measurable; reduces to the previous identity at one report per frame — parity-verified byte-identical on recorded per-frame-ACK runs (clean baseline delta-0/au=0 and spsc au=5739 both reproduce).
• tests/arq_e2e_delivery.sh: DRONE_AMPDU / DRONE_BATCH knobs (aggregation only forms with a back-to-back queue feed).
• docs/aggregation.md, CLAUDE.md: the two adversarial pairings above.

Closes #364

🤖 Generated with Claude Code

[qodo-free-for-open-source-projects]

PR Summary by Qodo

ARQ × A‑MPDU bench results: update docs plus ARQ E2E harness/analyzer

🧪 Tests 📝 Documentation 🕐 20-40 Minutes

AI Description

• Document A‑MPDU bench results: contract holds; CCX telemetry and goodput regress.
• Extend ARQ E2E analyzer to reconcile multi-report frames under BlockAck.
• Add DRONE_AMPDU/DRONE_BATCH knobs to the delivery harness for aggregation runs.

Diagram

graph TD
  A["tests/arq_e2e_delivery.sh"] --> B["txdemo (DRONE)"] --> C["Over-the-air link"] --> D["duplex (DUT + ACK/BA)"] --> E["run logs (jsonl)"] --> F["tests/arq_e2e_analyze.py"] --> G["docs/aggregation.md"]
  A --> H["rxdemo (WITNESS)"] --> E

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Sniff BlockAck/MPDU retries via monitor-mode capture

• ➕ Can observe retransmission behavior even when CCX tx.report suppresses or flattens retries under A‑MPDU
• ➕ Independent of vendor C2H reporting quirks
• ➖ Higher setup complexity (capture hardware/filters), more post-processing, and still may miss some chipset-internal details

2. Instrument TX path to emit per-attempt counters (firmware/driver-side)

• ➕ Direct ground truth for per-attempt retries regardless of aggregation/BlockAck behavior
• ➕ Avoids inference from missing/flattened CCX telemetry
• ➖ Requires invasive changes and per-chip maintenance; risks perturbing timing/throughput

3. Treat receipts-only as the primary delivery truth under A‑MPDU

• ➕ Matches the measured finding that receiver-side ledgers remain stable under TX aggregation
• ➕ Keeps methodology consistent across chips and avoids relying on degraded CCX accounting
• ➖ Loses TX-side retry distribution as a link-quality sensor under A‑MPDU without additional instrumentation

Recommendation: Keep the PR’s approach: document that CCX per-frame accounting is unreliable under AGG_EN and prefer receiver-side receipts for delivery truth, while updating the analyzer to remain correct if multi-report-per-tag ever appears. The alternatives (sniffing or instrumentation) are useful follow-ups if retry visibility under A‑MPDU becomes a requirement, but they add significant complexity compared to the current receipts-first methodology.

Files changed (4) +55 / -5

Tests (2) +34 / -4

arq_e2e_analyze.pyReconcile multiple tx.report entries per frame tag under BlockAck +28/-4

Reconcile multiple tx.report entries per frame tag under BlockAck

• Introduces per-frame reconciliation keyed by tag/index to handle a possible future where one MPDU emits multiple reports under BlockAck. Uses last-state-wins for ok status and max-retries across reports, and updates the ledger identity and emitted verdict JSON to use reconciled frame-ok counts instead of raw report-ok counts.

tests/arq_e2e_analyze.py

arq_e2e_delivery.shAdd A‑MPDU and batching knobs for aggregation-capable bench runs +6/-0

Add A‑MPDU and batching knobs for aggregation-capable bench runs

• Adds DRONE_AMPDU and DRONE_BATCH environment variables and wires them into the txdemo invocation. This enables controlled formation of aggregates (requires back-to-back queued frames) during the ARQ end-to-end delivery bench.

tests/arq_e2e_delivery.sh

Documentation (2) +21 / -1

CLAUDE.mdClarify A‑MPDU tradeoffs for unicast ARQ and CCX accounting limits +4/-1

Clarify A‑MPDU tradeoffs for unicast ARQ and CCX accounting limits

• Updates the Aggregation/ACK/TX reports section to note that the previously cited +30% goodput is a high-MCS broadcast result. Adds the measured unicast-ARQ outcome (−8% delivered at MCS3) and documents that per-frame CCX accounting degrades under AGG_EN, recommending receipts as the delivery truth for A‑MPDU.

CLAUDE.md

aggregation.mdAdd measured negative results for A‑MPDU on unicast ARQ shape +17/-0

Add measured negative results for A‑MPDU on unicast ARQ shape

• Adds two new findings: (1) aggregation can reduce delivered frames (−8%) for the FPV unicast ARQ configuration at MCS3, and (2) CCX SPE_RPT/tx.report behavior is structurally unreliable under AGG_EN (suppressed reports, retries pinned to 0, no multi-report frames observed). Recommends using windowed RX receipts for delivery truth when aggregated TX is enabled.

docs/aggregation.md

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Env quotes break knobs 🐞 Bug ≡ Correctness

Description

tests/arq_e2e_delivery.sh injects DEVOURER_TX_AMPDU_MODE and DEVOURER_TX_BATCH as literals like
DEVOURER_TX_BATCH="8", causing the quotes to become part of the environment variable value and
making parsers reject/misparse them. As a result, enabling DRONE_AMPDU/DRONE_BATCH will silently not
apply the intended A-MPDU/batching settings, potentially invalidating bench runs.

Code

tests/arq_e2e_delivery.sh[R176-177]

+    ${DRONE_AMPDU:+DEVOURER_TX_AMPDU_MODE="$DRONE_AMPDU"} \
+    ${DRONE_BATCH:+DEVOURER_TX_BATCH="$DRONE_BATCH"} \

Evidence

The bench script passes AMPDU/BATCH via ${DRONE_AMPDU:+...} and ${DRONE_BATCH:+...} with
embedded quotes. DEVOURER_TX_AMPDU_MODE is parsed from the environment as a raw string spec
(quotes will break parse), and DEVOURER_TX_BATCH is parsed with strtol (a leading " will parse
as 0, falling back to 1).

tests/arq_e2e_delivery.sh[170-178]
examples/common/env_config.cpp[109-115]
examples/tx/main.cpp[1315-1327]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`tests/arq_e2e_delivery.sh` uses `${VAR:+NAME="$VALUE"}` for optional env assignments. The quotes inside the expansion are *literal characters* (not shell quoting), so the target program reads values starting with `"` and fails to parse them.
## Issue Context
This PR adds the same pattern for `DEVOURER_TX_AMPDU_MODE` and `DEVOURER_TX_BATCH`, which are parsed by C/C++ code expecting unquoted strings/numbers.
## Fix Focus Areas
- tests/arq_e2e_delivery.sh[170-178]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Docs omit au/dup rates ✓ Resolved 📎 Requirement gap ◔ Observability

Description

The new A-MPDU+BlockAck bench writeup documents goodput and CCX-report behavior but does not publish
the required acked_undelivered (au)=0 confirmation and duplicate/write-off comparisons vs
per-frame ACK. This leaves the BA-vs-per-frame comparison incomplete against the stated bench
compliance criteria.

Code

docs/aggregation.md[R142-146]

+- **The gain does not generalize to the unicast-ARQ shape.** The same
+  aggregation at MCS3/512 B unicast with hardware ACK + retry 8 (the FPV
+  ARQ configuration, `tests/arq_e2e_delivery.sh`, batch-8 feed at 4 k fps
+  demand) delivered **−8%** vs the identical un-aggregated feed (208.8 k vs
+  226.7 k frames over the same span) — at low MCS the preamble amortization

Evidence

PR Compliance ID 2 requires publishing BA-arm correctness and comparison metrics (au=0,
duplicate/write-off rates, goodput delta) vs per-frame ACK. The updated docs/aggregation.md
section adds goodput delta and CCX-report limitations, but does not include the au=0 confirmation or
dup/write-off comparisons.

ARQ e2e bench under A-MPDU + BlockAck: au=0 and asymmetry/goodput quantified vs per-frame ACK
docs/aggregation.md[142-158]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The BA/A-MPDU bench documentation currently reports goodput and CCX-report collapse, but it does not publish the required apples-to-apples ARQ correctness/comparison metrics: `acked_undelivered (au)=0` for BA arms, plus duplicate delivery rate and ok=0-but-delivered (write-off) rate compared vs per-frame ACK.
## Issue Context
PR Compliance ID 2 requires these BA-arm metrics be measured and published for the same phases/stalls/RF setup as the per-frame-ACK arms.
## Fix Focus Areas
- docs/aggregation.md[142-158]
- CLAUDE.md[420-437]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Verdict counters unit mismatch ✓ Resolved 🐞 Bug ◔ Observability

Description

tests/arq_e2e_analyze.py now emits ok as a reconciled per-frame count (ok_frames) while leaving
reports as the raw tx.report count, changing semantics and making ok/reports meaningless under
multi-report-per-frame behavior. This is a backward-incompatible machine-event schema change that
can mislead any consumer expecting the previous ok==#ok reports meaning.

Code

tests/arq_e2e_analyze.py[R355-358]

  print(json.dumps({"ev": "arqe2e.verdict", "verdict": verdict,
                    "acked_undelivered": total_au,
                    "tail_suspect": tot_tail,
-                      "reports": len(reports), "ok": n_ok,
+                      "reports": len(reports), "ok": ok_frames,

Evidence

ok_frames is computed from the new frames reconciliation map (per-frame), but the emitted JSON
still reports reports: len(reports) (per-report). This introduces a mixed-denominator event
payload unless additional fields/renames are added.

tests/arq_e2e_analyze.py[244-259]
tests/arq_e2e_analyze.py[330-333]
tests/arq_e2e_analyze.py[355-360]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The `arqe2e.verdict` machine-event JSON mixes units: `reports` remains `len(reports)` (raw report events) while `ok` becomes `ok_frames` (reconciled frames). This changes the meaning of existing fields and makes derived rates ambiguous.
## Issue Context
The reconciliation is intentional and useful, but the event schema should remain self-describing and ideally backward compatible.
## Fix Focus Areas
- tests/arq_e2e_analyze.py[244-259]
- tests/arq_e2e_analyze.py[330-360]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}