rtl8733b: port USB TX aggregation (send_packets) by snokvist · Pull Request #400 · OpenIPC/devourer

snokvist · 2026-08-17T17:07:09Z

The RTL8733B was the last USB backend without send_packets batching, so every frame cost its own bulk-OUT URB. That is cheap on a desktop and expensive on the SoCs this part actually ships on — which is the whole reason to bother.

The measurement came first

I measured the cost before writing any code, because the answer could well have been "not worth it". Method: flood txdemo, let bring-up finish, then sample process CPU against frames submitted in steady state (startup excluded by construction, not subtracted), sweeping payload to separate per-call from per-byte.

	x86	CV610 craft (armv7)
per submission	~22 µs	~248 µs
per-call term	19.1 µs	~248 µs (payload term below noise)
per-byte term	0.002 µs/B	not resolvable

perf attributes ~87% to the kernel USB submit/completion path, ~5% to the descriptor build, ~4% libc. That split is what makes packing worth it: a 3:1 URB removes two submissions in three, while the per-frame descriptor build stays.

An independent in-situ cross-check on the real consumer agreed: the craft's hub sits at 5.9% of a core with no video TX and 27.1% at its operating point — ~21 points for ~1100 pps ≈ 193 µs/frame, same order as txdemo's 248 µs (txdemo also logs and polls thermal per frame, so it should read higher).

What changed

send_packets over the shared devourer::plan_tx_agg planner (desc_size 40, no first-block reserve, max 3 blocks), plus a DMA_TXAGG_NUM field on the first descriptor.

Two things differ from the 88xx siblings:

MAC init already programmed BLK_DESC_NUM = 3 into DWBCN0_CTRL[7:4] (0x0208) — the same field and value Jaguar3 writes at REG_AUTO_LLT_V1, paired with the same TXDMA_OFFSET_CHK+1 |= BIT(1). Bring-up needed no change at all.
The count must be written before the checksum, not patched on after the way the 8822C does it. This family folds its checksum inside fill_tx_desc_8733b, and the fold covers 32 bytes skipping only 0x1c-0x1d — so byte 0x1f is inside the checksummed span. A selftest cell pins the ordering; moving the write below the checksum fails exactly that cell (verified by mutation).

Result

Same craft, ~1750 fps, frame rate unchanged (1733 → 1800):

	µs/frame	CPU
single	248.08	43.0% of one core
batch 3:1	148.15	26.7% of one core

At the craft's ~1100 pps operating point that is about ten points of a core. On x86 the same A/B reads 21.5 → 10.6 µs. My pre-implementation prediction was 12–18%; the measured 40% per-frame saving (not the modelled 58%) brings it to ~10, so the model was slightly optimistic and the measurement corrects it.

The failure mode this was checked against

DMA_TXAGG_NUM's placement at dword7[31:24] was inferred from 8822C parity, so it was confirmed on silicon rather than assumed. The 8822BU precedent is that wrong packing makes the TXDMA re-air block 1 agg_num times — and frame counts cannot see that: rx_hits came back identical (23900) in both modes, exactly as it would if the chip were re-airing.

So every frame was stamped (DEVOURER_TX_QOS_DATA) and the stamps counted distinct at an RTL8812AU witness (DEVOURER_RX_PCTR):

rx.seq=31721  DISTINCT pctr=31721  dup-ratio=1.00   aggURBs=10940 (all frames=3)

1.00 where re-airing would read 3.00. tests/txagg_bench.sh now documents that its own rx_hits cell cannot make this distinction, and how to run the stamped check.

No-change control

Knob off is the default and is byte-identical: agg_num 0 leaves dword7[31:24] clear and the single-frame path is untouched — pinned by a selftest cell, and by the existing golden-byte descriptor tests still passing.

One accounting note for reviewers: GetTxStats().submitted counts bulk-OUT transfers, so an aggregated session reports roughly frames/3. That is the same accounting Jaguar1/2/3 have and not a throughput drop — the per-URB tx.agg event carries the true frame count. It briefly looked like a 3× regression until I checked it.

Verification

ctest 54/54, including the new agg-num cells; mutation check confirms the ordering cell is load-bearing.
Device, this host: 8733BU DUT → 8812AU witness, stamped distinctness 1.00, all URBs frames=3.
Device, CV610 craft: the CPU A/B above, cross-built with the existing armv7 toolchain.

Blast radius

Confined to src/rtl8733b/. No other backend, no shared planner change (TxAggPlan.h is used as-is), no MAC-init change. Knob defaults to off, so an unmodified caller gets today's behaviour byte for byte.

Follow-on, deliberately not here: consumers must call send_packets to see any of this. waybeam-link currently calls send_packet singly at all three TX sites, and batching there is a latency question rather than a CPU one — separate change, separate evidence.

The 8733B was the last USB backend without send_packets batching, so every frame cost its own bulk-OUT URB. That is cheap on a desktop and expensive on the SoCs this part actually ships on. Measured before writing any code, on the CV610 craft and on x86, by sweeping payload and reading process CPU against frames submitted in steady state: per submission x86 ~22 us CV610 ~248 us (11x) split per-call 19.1 us + 0.002 us/byte on x86; on ARM the payload term is below noise -- it is essentially ALL per-call attribution ~87% kernel USB submit/completion, ~5% descriptor build, ~4% libc Per-call dominance is what makes packing worth it, because a 3:1 URB removes two submissions in three while the descriptor build stays. Ported: send_packets over the shared devourer::plan_tx_agg planner (desc_size 40, no first-block reserve, max 3 blocks), and a DMA_TXAGG_NUM field on the first descriptor. Two things differ from the 88xx siblings. MAC init ALREADY programmed BLK_DESC_NUM = 3 into DWBCN0_CTRL[7:4], the same field and value, so bring-up needed no change. And the count must be written BEFORE the checksum rather than patched on after: the fold covers 32 bytes skipping only 0x1c-0x1d, so byte 0x1f is inside it. A selftest cell pins that ordering and fails if the write moves below the checksum (verified by mutation). Result, same craft, ~1750 fps, frame rate unchanged (1733 -> 1800): us/frame 248.08 -> 148.15 CPU 43.0% -> 26.7% of one core At the craft's ~1100 pps operating point that is about ten points of a core. On x86 the same A/B reads 21.5 -> 10.6 us. DMA_TXAGG_NUM's placement was inferred from 8822C parity, so it was confirmed on silicon rather than assumed. The 8822BU precedent is that wrong packing makes the TXDMA re-air block 1 agg_num times, which frame counts cannot see -- rx_hits is identical either way. Every frame was therefore stamped (DEVOURER_TX_QOS_DATA) and the stamps counted distinct at an 8812AU witness (DEVOURER_RX_PCTR): 31721 receptions, 31721 distinct counters, ratio 1.00 where re-airing would read 3.00. All 10940 aggregated URBs carried frames=3. Knob off (the default) is byte-identical: agg_num 0 leaves dword7[31:24] clear and the single-frame path is untouched, which a selftest cell pins. Note GetTxStats().submitted counts URBs rather than frames, so an aggregated session reports about a third -- the same accounting the other families have, not a throughput drop; tx.agg carries the true count. ctest 54/54. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

qodo-free-for-open-source-projects · 2026-08-17T17:08:32Z

PR Summary by Qodo

rtl8733b: add USB TX aggregation via send_packets batching

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Add RTL8733B USB send_packets batching to pack up to 3 frames per URB.
• Encode DMA_TXAGG_NUM in first TX descriptor before checksum to satisfy silicon.
• Add selftests and bench guidance to validate distinct delivery and accounting.

Diagram

graph TD
  caller(["TX caller"]) --> dev["Rtl8733bDevice\nsend_packets"] --> plan["TxAggPlan\nplan_tx_agg"] --> build["build_tx_block"] --> desc["fill_tx_desc_8733b\n(agg_num+checksum)"] --> usb[("USB bulk-OUT URB")]
  cfg{{"cfg.tx.usb_agg_max\nDEVOURER_TX_USB_AGG"}} --> dev
  
  subgraph Legend
    direction LR
    _actor(["Caller"]) ~~~ _mod["Module/Function"] ~~~ _cfg{{"Config knob"}} ~~~ _io[("I/O")]
  end

High-Level Assessment

The following are alternative approaches to this PR:

1. Move aggregation into a shared USB transport layer

➕ Avoids per-backend send_packets implementations and keeps batching logic centralized
➕ Could standardize stats/events semantics (URBs vs frames) across devices
➖ Harder to support per-family descriptor quirks (e.g., agg_num must precede checksum fold here)
➖ Would require new interfaces for “patch before checksum” vs “patch after + recompute” behaviors
➖ Larger blast radius across all USB backends

2. Patch agg_num after build and expose a descriptor re-checksum API

➕ Keeps send_packets assembly simple (build blocks, then patch first descriptor)
➕ Could align more closely with some sibling implementations
➖ RTL8733B checksum is folded inside fill_tx_desc_8733b; refactor needed to checksum after patching
➖ Risky: requires changing tested descriptor construction flow and invariants

3. Use async/libusb URB pipelining instead of batching

➕ Can improve throughput/latency overlap beyond simple batching
➕ Potentially reduces CPU stalls on synchronous submissions
➖ More complex lifecycle/buffering and error handling
➖ Does not reduce per-URB kernel submit/completion cost as directly as batching
➖ Significantly higher implementation and testing cost

Recommendation: The PR’s approach is the best fit: it reuses the existing shared TxAggPlan for layout, keeps the change localized to RTL8733B, and correctly handles the family-specific constraint that DMA_TXAGG_NUM must be set before the descriptor checksum fold. A transport-layer refactor or async pipelining would increase scope and risk without being necessary to realize the measured CPU savings.

Files changed (8) +274 / -6

Enhancement (3) +151 / -5

Rtl8733bDevice.cppImplement RTL8733B send_packets USB batching using TxAggPlan +119/-3

Implement RTL8733B send_packets USB batching using TxAggPlan

• Adds a send_packets override that groups up to 3 compatible frames per URB, using TxAggPlan for offsets/alignment and optional shim. Extends build_tx_block to accept an agg_num for the first descriptor, emits per-URB tx.agg events, and preserves the byte-identical single-frame path when disabled or not applicable.

src/rtl8733b/Rtl8733bDevice.cpp

Rtl8733bDevice.hExpose send_packets override and agg_num-aware block builder +18/-1

Expose send_packets override and agg_num-aware block builder

• Declares the send_packets override and documents knob behavior and stats semantics. Updates build_tx_block signature to include an optional agg_num parameter and documents the “first descriptor only” rule for aggregation metadata.

src/rtl8733b/Rtl8733bDevice.h

TxDescriptor8733b.hAdd DMA_TXAGG_NUM field and enforce checksum ordering +14/-1

Add DMA_TXAGG_NUM field and enforce checksum ordering

• Extends TxDescConfig with agg_num, validates it (<=3), and writes DMA_TXAGG_NUM into dword7[31:24] before computing the folded checksum. Adds comments explaining why writing after checksum is invalid on this family.

src/rtl8733b/TxDescriptor8733b.h

Tests (2) +61 / -0

rtl8733b_tx_desc_selftest.cppAdd selftests for RTL8733B agg_num encoding and checksum coverage +45/-0

Add selftests for RTL8733B agg_num encoding and checksum coverage

• Adds selftest cases verifying agg_num defaults to 0 (no-change control), agg_num=3 lands at byte 0x1f, the checksum remains valid (ordering cell), and agg_num>3 is rejected by validation. Ensures the checksum-order constraint is pinned by a load-bearing test.

tests/rtl8733b_tx_desc_selftest.cpp

txagg_bench.shClarify bench usage and document distinctness verification for re-air failure mode +16/-0

Clarify bench usage and document distinctness verification for re-air failure mode

• Adds guidance on preserving env vars through sudo, shows RTL8733B-specific invocation with batch capped at 3, and documents why rx_hits cannot detect the “re-air block 1” failure mode. Provides a stamped distinctness check procedure using DEVOURER_TX_QOS_DATA and DEVOURER_RX_PCTR.

tests/txagg_bench.sh

Documentation (3) +62 / -1

aggregation.mdDocument RTL8733B USB TX aggregation rules and measured CPU savings +16/-1

Document RTL8733B USB TX aggregation rules and measured CPU savings

• Adds an RTL8733B-specific section describing 3-descriptor batching, descriptor size, and why agg_num must be written before checksum. Records measured per-frame CPU improvements on embedded ARM vs x86 and clarifies why batching is most valuable on small hosts.

docs/aggregation.md

rtl8733b.mdAdd RTL8733B USB TX aggregation overview and verification notes +24/-0

Add RTL8733B USB TX aggregation overview and verification notes

• Documents the new send_packets batching behavior, the BLK_DESC_NUM=3 constraint, and the DMA_TXAGG_NUM placement/checksum ordering requirement. Adds performance results and explains the stamped distinctness check vs misleading rx_hits parity, plus URB-vs-frame stats accounting.

docs/rtl8733b.md

CLAUDE.mdUpdate RTL8733B engineering notes for USB TX aggregation behavior +22/-0

Update RTL8733B engineering notes for USB TX aggregation behavior

• Records the motivation, the invariants (BLK_DESC_NUM already set, agg_num-before-checksum), and the silicon verification strategy using per-frame stamps. Notes stats accounting differences (URBs vs frames) and measured CPU improvements.

src/rtl8733b/CLAUDE.md

qodo-free-for-open-source-projects · 2026-08-17T17:11:25Z

Code Review by Qodo

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

1. ~~Channel mismatch infinite loop~~ ✓ Resolved 🐞 Bug ☼ Reliability

Description

Rtl8733bDevice::send_packets breaks on a radiotap CHANNEL mismatch without advancing done, and
then immediately continues when lens is empty. If the next unprocessed packet requests a
different channel (including after earlier aggregated sends), this spins forever and hangs TX.

Code

src/rtl8733b/Rtl8733bDevice.cpp[R534-537]

+      const int want =
+          devourer::radiotap_peek_channel(pkts[i].data, pkts[i].len);
+      if (want > 0 && want != _channel.Channel)
+        break;

Evidence

The new batching loop breaks on CHANNEL mismatch but only advances done for malformed/null leading
frames; a leading CHANNEL mismatch leaves done unchanged and lens empty, causing an immediate
continue and a tight infinite loop. build_tx_block() shows such frames are expected to be
refused on this backend, so the loop must still advance past them.

src/rtl8733b/Rtl8733bDevice.cpp[513-542]
src/rtl8733b/Rtl8733bDevice.cpp[676-679]

Agent prompt

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

## Issue description
`Rtl8733bDevice::send_packets()` can enter an infinite loop when the next packet has a radiotap CHANNEL that differs from the session channel: the inner loop `break`s, `lens` stays empty, `done` is not incremented, and the outer loop `continue`s.
## Issue Context
This backend’s `build_tx_block()` explicitly refuses a mismatched radiotap CHANNEL (no per-submission retune), so these frames must be rejected per-frame without stalling the entire batch.
## Fix Focus Areas
- src/rtl8733b/Rtl8733bDevice.cpp[519-542]
## Suggested change
When the CHANNEL mismatch is detected and it would leave `lens` empty (i.e., mismatch on the leading frame), ensure forward progress by advancing `done` (and optionally running the single-frame path to preserve the per-frame rejection behavior), e.g.:
- if `lens.empty()` on mismatch: call `send_packet(pkts[i].data, pkts[i].len)` (will be rejected) and `++done`, then `break`.
- keep the existing `break` behavior for non-leading mismatches (flush current run).

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

2. ~~Short writes counted success~~ ✓ Resolved 🐞 Bug ≡ Correctness

Description

The aggregated bulk-OUT path treats any rc >= 0 as success and increments ok by all frames, but
the USB transport returns the actual bytes transferred. If a short write occurs (`0 <= rc <
urb.size()`), the batch is treated as fully submitted even though the device received only a prefix.

Code

src/rtl8733b/Rtl8733bDevice.cpp[R582-585]

+        .f("shim", plan.shim)
+        .f("ok", rc >= 0);
+    if (rc >= 0) {
+      ok += plan.frames();

Evidence

UsbTransport::tx_sync returns actual bytes from libusb_bulk_transfer, so rc is a byte count, not
just a success/failure flag. The existing RTL8733B single-frame path treats short transfers as
failure, but the new aggregated path does not, so a short write would be miscounted as fully
successful.

src/UsbTransport.cpp[1002-1023]
src/rtl8733b/Rtl8733bDevice.cpp[452-458]
src/rtl8733b/Rtl8733bDevice.cpp[576-586]

Agent prompt

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

## Issue description
`send_packets()` currently treats `bulk_send_sync_ep()` as successful when `rc >= 0`. On USB, `tx_sync()` returns the number of bytes actually transferred, so a short write can occur with a non-negative return value; counting it as success can misreport submissions and potentially feed truncated aggregates to the device.
## Issue Context
The RTL8733B single-frame `send_packet()` path already enforces `sent == usb_frame.size()` and logs on short sends, but the new aggregated path does not.
## Fix Focus Areas
- src/rtl8733b/Rtl8733bDevice.cpp[576-597]
## Suggested change
- Treat success only when `rc == static_cast<int>(urb.size())`.
- If `rc` is non-negative but short, log an error similar to the single-frame path (include EP, rc, expected size) and do **not** increment `ok`.
- Ensure the emitted `tx.agg` event’s `ok` field reflects full completion (e.g., `ok = (rc == (int)urb.size())`).

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

3. ~~CLAUDE.md duplicates TX agg docs~~ ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

src/rtl8733b/CLAUDE.md re-documents the USB TX aggregation API/descriptor details that are already
documented in header doc-comments, creating a double source of truth. This increases maintenance
risk when the headers evolve but CLAUDE.md is not updated in lockstep.

Code

src/rtl8733b/CLAUDE.md[R111-114]

+- **USB TX aggregation is ported, and this is the family it matters most on.**
+  `send_packets` packs up to 3 `[txdesc][frame]` blocks into one bulk-OUT URB
+  (`cfg.tx.usb_agg_max` / `DEVOURER_TX_USB_AGG`; 0 = off = byte-identical).
+  Two facts made it cheap to port and one made it worth porting. MAC init

Evidence
PR Compliance ID 1 requires CLAUDE.md to reference authoritative header doc-comments rather than
re-stating them. The added CLAUDE.md section re-documents the same send_packets aggregation
behavior and DMA_TXAGG_NUM/checksum-ordering details that are already described in
Rtl8733bDevice.h and TxDescriptor8733b.h.
CLAUDE.md: Do Not Duplicate Header Doc-Comments in CLAUDE.md: CLAUDE.md: Do Not Duplicate Header Doc-Comments in CLAUDE.md: CLAUDE.md: Do Not Duplicate Header Doc-Comments in CLAUDE.md: CLAUDE.md: Do Not Duplicate Header Doc-Comments in CLAUDE.md: CLAUDE.md: Do Not Duplicate Header Doc-Comments in CLAUDE.md: CLAUDE.md: Do Not Duplicate Header Doc-Comments in CLAUDE.md: CLAUDE.md: Do Not Duplicate Header Doc-Comments in CLAUDE.md: CLAUDE.md: Do Not Duplicate Header Doc-Comments in CLAUDE.md
src/rtl8733b/CLAUDE.md[111-132]
src/rtl8733b/Rtl8733bDevice.h[42-54]
src/rtl8733b/TxDescriptor8733b.h[117-124]

Agent prompt

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

## Issue description
`src/rtl8733b/CLAUDE.md` duplicates authoritative documentation that already exists as doc-comments in headers (USB TX aggregation behavior and `DMA_TXAGG_NUM`/checksum ordering). This violates the repo guidance to keep API/structure docs single-sourced in headers and have CLAUDE.md point to them.
## Issue Context
This PR adds/updates header doc-comments describing:
- `Rtl8733bDevice::send_packets` USB TX aggregation behavior and related stats semantics.
- `TxDescConfig::agg_num` (`DMA_TXAGG_NUM`) placement and the requirement to write it before the checksum.
CLAUDE.md currently repeats these details (including measurements), making it easy for docs to drift.
## Fix Focus Areas
- src/rtl8733b/CLAUDE.md[111-132]
- src/rtl8733b/Rtl8733bDevice.h[42-54]
- src/rtl8733b/TxDescriptor8733b.h[117-124]

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

Tip of the day

💡 Did you know, you can keep summaries lean with Finding overflow, which tucks the rest behind 'View more'

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Two findings from an adversarial pass over the previous commit. 1. An off-channel LEADING frame hung the TX thread. The run collector ended the run on a radiotap CHANNEL mismatch, so when the mismatch was on the first frame the run came back empty, `done` never advanced, and the while loop spun forever -- holding _reg_mu, which every other path on this device takes, so it deadlocks the adapter rather than just stalling TX. Reachable from any caller that does radiotap-driven per-packet hopping with the knob on. A leading off-channel frame now enters the run alone, so the single-frame path refuses it (build_tx_block already does) and `done` moves past it. The empty-run branch also advances unconditionally now, so termination is structural rather than a property of the current run rules -- the previous cut had the correct rule for null/malformed frames documented one branch above the case that got it wrong. 2. The boundary shim was never exercised. All prior verification used a fixed payload, so the packed total never landed on a bulk-MPS multiple and the 8-byte first-block PKT_OFFSET reserve never appeared: 0 shim=true across ~22k URBs. That is the exact path that made the 8822BU re-air block 1, so leaving it unmeasured was the weakest point in the evidence. For three blocks at USB HS the reserve engages when the MPDU length is 472 mod 512 -- confirmed exactly: urb_bytes 1544 / 3080 / 4616 at payloads 472 / 984 / 1496, each n*512 + 8. Stamped distinctness with the shim engaged: 15027 shim URBs, 44032 receptions, 44032 distinct, ratio 1.00, against 1.00 for the no-shim control. This part's block walker does account the reserve. Also checked and NOT a defect: this backend ignores radiotap DBM_TX_POWER, so Jaguar3's rule about breaking a run when the power bank changes has nothing to guard here. ctest 54/54. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

snokvist · 2026-08-17T17:13:19Z

Adversarial pass over my own diff turned up two things, both fixed in a8c1cbc.

1. An off-channel leading frame hung the TX thread. The run collector ended a run on a radiotap CHANNEL mismatch — but when the mismatch was on the first frame the run came back empty, done never advanced, and the while spun forever. It holds _reg_mu while doing so, and every other path on this device takes that mutex, so it deadlocks the adapter rather than merely stalling TX. Reachable from any caller doing radiotap-driven per-packet hopping with the knob on.

Galling detail: I had the correct rule written out one branch above, in the comment explaining why a null or malformed leading frame must advance done — and then broke it in the next branch down. A leading off-channel frame now enters the run alone so the single-frame path refuses it (build_tx_block already does), and the empty-run branch advances unconditionally, so termination is structural rather than a property of whatever the run rules happen to be.

2. The boundary shim was never exercised — 0 shim=true across ~22k URBs. Every run I did used a fixed payload, so the packed total never landed on an exact bulk-MPS multiple and the 8-byte first-block PKT_OFFSET reserve never appeared. That is precisely the path that made the 8822BU re-air block 1, so it was the weakest point in the evidence and I had reported the port verified without it.

For three blocks at USB HS the reserve engages when the MPDU length is ≡ 472 mod 512. Confirmed exactly — urb_bytes 1544 / 3080 / 4616 at payloads 472 / 984 / 1496, each n × 512 + 8:

shim ON  (payload=472, 15027 shim URBs): rx.seq=44032  DISTINCT=44032  ratio=1.00
shim OFF (payload=1000,     0 shim URBs): rx.seq=32087  DISTINCT=32087  ratio=1.00

So this part's block walker does account the reserve, unlike the 8822BU's.

Checked and not a defect: this backend ignores radiotap DBM_TX_POWER, so Jaguar3's rule about ending a run when the power bank changes has nothing to guard here. The simplification is deliberate, not an omission.

ctest 54/54.

Qodo review on 400, plus a self-review pass for the same classes. 1. A short write counted as a full send. bulk_send_sync_ep returns BYTES SUBMITTED, so `rc >= 0` also matches a truncated transfer: the chip got a prefix, some trailing block is partial or absent, and there is no way to say which frames aired -- but all of them were reported submitted. send_packet has always refused a short write, so the aggregated path was the looser of the two in the same backend. It now requires rc == urb.size(), logs the short write, and reports none of the batch. tx.agg gains `sent` and `ok` now means a FULL write. Note the same pattern is live in jaguar2 and jaguar3 send_packets (both `rc >= 0` -> `ok += plan.frames()`). Not touched here -- that is two other backends and belongs in its own change -- but it is real, and jaguar1 is exempt only because its async path returns bool. 2. src/rtl8733b/CLAUDE.md restated the descriptor placement, the checksum-ordering rule and the URB accounting that the headers already doc-comment, against this repo's standing "never duplicate what a header already doc-comments". Trimmed to what only it can carry -- the bring-up archaeology, the measured cost, and the verification method -- pointing at the headers for the mechanism. The ordering rule now lives on TxDescConfig::agg_num where a reader looks for it, instead of only in the fill body. Self-review found three more instances of the class the previous round was about: a claim outliving what it described. - The header quoted ~283 us for a craft submission. That was the interpolated figure from the rate sweep; the direct A/B measured 248 and every other site says 248. - docs/logging.md still listed tx.agg as "frames, bytes, shim, ok" after this commit added `sent` and changed what `ok` means. - The run-collector comment still described the pre-a8c1cbc design, where an off-channel leading frame led the NEXT URB rather than entering the current run alone. The channel-mismatch infinite loop Qodo also reports was already fixed in a8c1cbc; their review targets 720a4dd. ctest 54/54. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

snokvist · 2026-08-17T17:43:18Z

All three review findings addressed in 9581f6a; replies are in each thread.

Short writes counted as success — real, mine, fixed. bulk_send_sync_ep returns bytes submitted, so rc >= 0 matched a truncated transfer. The sharper form of the bug: send_packet has always refused a short write, so the aggregated path was the looser of the two paths in the same backend.
Channel-mismatch infinite loop — real; already fixed in a8c1cbc (the review targets 720a4dd). Worth adding that it holds _reg_mu throughout, so it deadlocks the whole adapter rather than only stalling TX.
CLAUDE.md duplicating header docs — accepted; the repo states the rule at CLAUDE.md:11 and I had the checksum-ordering rule in three places.

Self-review pass on top

I ran a second pass looking for the same classes rather than the same instances. The class from the last round — a claim outliving what it described — turned up three more times, all mine:

The send_packets header comment quoted ~283 µs for a craft submission. That was the interpolated figure from the rate sweep; the direct A/B measured 248, and every other site already said 248. Fixed, and all four sites now agree on 248 → 148.
docs/logging.md still described tx.agg as "frames, bytes, shim, ok" after this very commit added sent and changed what ok means.
The run-collector comment still described the pre-a8c1cbc design, in which an off-channel leading frame led the next URB rather than entering the current run alone.

The loop-progress class I closed structurally rather than case-by-case: the empty-run branch now advances done unconditionally, so a future run rule cannot reintroduce a non-advancing path even if it gets the case analysis wrong — which is precisely how the first one happened.

The return-value class I swept across the tree, which is what surfaced the jaguar2 / jaguar3 instances noted in the short-write thread. Those are pre-existing and left untouched deliberately; say the word and they are a follow-up PR.

Unrelated but verified while I had the bench open: the aggregation path is now confirmed on a Jaguar3 8812EU (0bda:a81a, chip-id 0x17, variant C8822E) — single / batched / batched-with-shim, all stamped ratio 1.00, all URBs frames=3, and 27.8 → 13.6 µs per frame on x86. Worth mentioning because docs/aggregation.md's only on-air bench is 8822BU, a Jaguar2 — so as far as I can tell that was the first on-air aggregation verification of any Jaguar3 part, on code that was already shipping. No change needed; I can send the doc note separately if you want it recorded.

ctest 54/54.

Host CPU only — not A-MPDU, and measured not to change on-air spacing. One bulk-OUT submission costs ~248 us of CPU on the CV610 craft against ~22 us on x86, ~87% of it the kernel USB submit/completion path. Batching up to 3 frames per URB measured 8.6-10.5 points of one core on the craft at ~1100 pps with pps unchanged, and 60% fewer USB submissions for identical frames (usbmon: 10974 -> 4347 URBs, frames/URB 1.00 -> 2.52). Only frames the framer emitted back to back within one fan-out are batched, so nothing is deferred. Ordering is enforced structurally by node::StagedAir, whose send_now()/resend() flush by construction — the first cut used ten hand-placed flush calls and adversarial review deleted all ten with the suite staying green. Correctness verified end to end rather than by counts: 8733BU TX to an 8812AU running a real rx node, every payload byte checked after §6.3a reassembly, bad=0 in every run. Default 0 = off, so every deployment stays byte-identical; only the CV610 craft profile opts in. Spec: §15.2 + Pass 184. third_party/devourer is pinned to OpenIPC/devourer#400's PR HEAD, not a merge commit — accepted deliberately; re-pin tracked as #215.

josephnef

Approve. Correct, well-evidenced, blast radius as claimed.

Independently verified on this bench:

Full suite 54/54 on the PR head.
Mutation check reproduced: moving the agg_num write below the checksum line fails exactly the two ordering cells (agg-num is inside the checksum, and precedes it + agg-num actually changed the checksum) — the cell is genuinely load-bearing.
Packer loop termination: every path advances done, including the leading off-channel frame entering the run alone so the single-frame path refuses it and moves past — the a8c1cbc hang fix is right, and the comment pins the invariant against future run rules.
Parity vs RtlJaguar3Device::send_packets: same structure, two deliberate and defensible divergences (off-channel frames refused per this backend's single-frame contract rather than retuned; no power-grouping since per-packet power isn't ported here). The short-write handling is stricter than Jaguar3's rc >= 0 accounting — the right semantics.
The hardware evidence is the right kind: stamped distinctness (1.00 vs the 3.00 re-air signature frame counts can't see), the boundary shim exercised at a payload chosen to hit it (MPDU ≡ 472 mod 512), and txagg_bench.sh documenting its own rx_hits blind spot.

Findings, all minor — only #1 is worth fixing before merge:

Silent transport error on the aggregated path: the error log gates on rc >= 0 && !sent_all, so a hard libusb failure (rc < 0) produces only the JSONL tx.agg event and no stderr diagnostic. The single-frame path logs both cases (sent != size). Suggest logging on !sent_all unconditionally.
Jaguar3's looser ok counting is now the odd one out — not this PR's bug, but "must not be the looser of the two" cuts both ways; follow-up material to align J3 (and siblings) to full-write-only accounting.
The measurement lives in four places (Rtl8733bDevice.h, src/rtl8733b/CLAUDE.md, docs/rtl8733b.md, docs/aggregation.md all carry 248→148 µs / 43.0→26.7%). The subtree CLAUDE.md says "read it there" and then repeats the numbers. A remeasure now has four spots to update; consider one canonical home with pointers.
Nit: the explicit pkts[i].data == nullptr ternary in the collector is redundant — radiotap_hdr_len already null-checks (J3 relies on that). The comment does carry contract value.
Lock scope: _reg_mu is held across the entire batch (vs J3 per-operation). Worst case with a wedged device: count/3 URBs × 100 ms timeout each blocks FastRetune/GetThermalStatus for the duration. Fine at realistic batch sizes; a one-line comment saying it's deliberate would stop a future "why" hunt.

Docs discipline is good — every win paired with its counterpart in the same breath, and the URB-vs-frame submitted accounting trap called out before someone rediscovers it as a "3× regression".

…401) Follow-up from the #400 review: the RTL8733B port counted an aggregated frame as submitted only on a full bulk write, and that made its siblings the odd ones out — Jaguar2 and Jaguar3 counted `rc >= 0`, but `bulk_send_sync_ep` returns *bytes submitted*, so a short write (the chip got a prefix) was reported as delivered work. In an aggregated URB that is worse: trailing blocks partial or absent, no way to say which frames aired, all of them counted. ## What changed - **Jaguar2 + Jaguar3, single-frame and aggregated paths**: full-write-or-nothing accounting, matching the 8733B (and Kestrel, which was already strict). - **Jaguar2 + Jaguar3 aggregated paths** now emit the `tx.agg` `sent` byte count and log a genuine short write (`rc >= 0`, truncated) as an error. `rc < 0` stays quiet there deliberately — their single-frame paths' NAK-backoff contract means a failure log would flood exactly when the caller is already backing off. - **RTL8733B**: the aggregated error log no longer gates on `rc >= 0` — a hard transport failure reaches stderr like it does on that backend's single-frame path (the #400 review's finding 1). - **Jaguar1**: comment only. Its TX is async by design; `ok` is URB acceptance and bytes-on-wire resolve at completion reaping, so submit-time full-write accounting cannot apply there. - `docs/logging.md`: the `tx.agg` row now states the sync-generation `sent`/full-write semantics and the Jaguar1 async exception. ## Verification - `ctest` 54/54. - On-air on both changed generations (`tests/txagg_bench.sh`, BATCH=3, MCS7, 10 s floods): - 8812BU (Jaguar2) TX → 8812CU witness: **7459 agg URBs, every one `sent==bytes`, `ok=true`**, frames/URB 3.0, rx_hits 17100 vs 17200 single — delivery parity. - 8812CU (Jaguar3) TX → 8812BU witness: **10654 agg URBs, every one full-write**, frames/URB 3.0, rx_hits 26500 vs 26600 single. - Zero aggregated-TX error lines in either run — the new logging is silent when nothing is wrong. - The 8733B change is log-gate-only (its accounting was already strict and hardware-validated in #400); no 8733B unit was on the bench for this run. ## What this deliberately does not do No retry of a short-written aggregated URB: the chip holds a prefix, and resubmitting could re-air frames that did make it. Frames in a truncated URB are dropped and simply not counted — the caller's own accounting sees the loss. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>

Closes #215. OpenIPC/devourer#400 merged as 52c6549, retiring the PR-head pin this tree was carrying. The diff #215 insisted on was not a formality: upstream is TWO commits ahead. #401 is the maintainer's follow-up from #400's review — jaguar2/ jaguar3 counted a short write as delivered on both their single-frame and aggregated paths (bulk_send_sync_ep returns bytes submitted, so rc >= 0 matched one); both now require a full write. Reaching us, since the fleet default is 8812AU/CU/EU: on 8812CU/EU send_packet returns false where it returned true, which RadioAir::inject already treats as not-sent — better accounting, not a behaviour flip, and unreachable in practice anyway. The 8733B delta is log-only; the ok accounting flush_staged reads is untouched. Verified as vendored TX code rather than a docs bump: devourer ctest 54/54, gates.sh 26/0, and the byte-exact hardware check re-run on the re-vendored tree — 399 frames bad=0, batched and unbatched.

qodo-free-for-open-source-projects Bot reviewed Aug 17, 2026

View reviewed changes

Comment thread src/rtl8733b/CLAUDE.md Outdated

Comment thread src/rtl8733b/Rtl8733bDevice.cpp

Comment thread src/rtl8733b/Rtl8733bDevice.cpp

This was referenced Aug 18, 2026

Re-pin vendored devourer to OpenIPC/devourer#400's merge commit snokvist/waybeam-link#215

Closed

feat(tx): fold consecutive TX submissions into one bulk-OUT URB snokvist/waybeam-link#216

Merged

josephnef approved these changes Aug 18, 2026

View reviewed changes

josephnef merged commit 52c6549 into OpenIPC:master Aug 18, 2026
29 of 31 checks passed

josephnef mentioned this pull request Aug 18, 2026

tx: count only full bulk writes as submitted on the sync generations #401

Merged

snokvist mentioned this pull request Aug 18, 2026

chore(vendor): devourer 9581f6a -> d2120b4 — the merge, plus #401 snokvist/waybeam-link#217

Merged

Navigation Menu

Uh oh!

Uh oh!

rtl8733b: port USB TX aggregation (send_packets) - #400

rtl8733b: port USB TX aggregation (send_packets)#400
josephnef merged 3 commits into
OpenIPC:masterfrom
snokvist:feat/8733b-usb-tx-agg

snokvist commented Aug 17, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Aug 17, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Aug 17, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

snokvist commented Aug 17, 2026

Uh oh!

snokvist commented Aug 17, 2026

Uh oh!

josephnef left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

snokvist commented Aug 17, 2026

The measurement came first

What changed

Result

The failure mode this was checked against

No-change control

Verification

Blast radius

Uh oh!

qodo-free-for-open-source-projects Bot commented Aug 17, 2026

PR Summary by Qodo

Uh oh!

qodo-free-for-open-source-projects Bot commented Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

Uh oh!

Uh oh!

Uh oh!

snokvist commented Aug 17, 2026

Uh oh!

snokvist commented Aug 17, 2026

Self-review pass on top

Uh oh!

josephnef left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

qodo-free-for-open-source-projects Bot commented Aug 17, 2026 •

edited

Loading