usb: pipelined register writes for the bring-up; Jaguar3 stage timing - #417
usb: pipelined register writes for the bring-up; Jaguar3 stage timing#417gilankpam wants to merge 1 commit into
Conversation
InitWrite on the RTL8812EU is ~14k synchronous EP0 round trips and nothing else. InitTimer now brackets every Jaguar3 rtw_hal_init / InitWrite stage and reports the control-transfer count per stage (UsbXferCount.h), which is the unit the bring-up is actually paid in. init.timing gains an `xfers` field; docs/logging.md carries the schema. A transfer costs 76-80 us synchronous on an embedded host (ssc338q) and ~27 us pipelined 8-deep. EP0 completes URBs in submission order, so IRtlTransport::write_batch_begin/end lets UsbTransport queue writes as async URBs and wait only on reads (submitted behind the queue), bulk transfers and flush_writes. Jaguar3 InitWrite runs its whole bring-up in one batch (RAII scope, closed before the coex thread starts); the ms-scale settle delays flush first. The three methods default to no-ops, so PCIe is unaffected. Batches are single-threaded by contract: open one only while no other thread touches the transport, and close it before any worker starts. A drain that times out cancels what is still submitted and keeps pumping for the cancellations rather than declaring the queue empty: an in-flight count zeroed by hand goes negative on the late callback, which silently disables every later drain, returns a slot to the free list twice, and leaves the destructor freeing a transfer libusb still owns. Slots that genuinely cannot be reaped (dead event loop, yanked device) are retired for the session and leaked at teardown instead. Separately, the RF radio-table load is write-only: bits [31:20] of the direct window read back 0 for all 1540 entries, cold and warm, so the vendor's MASK20BITS read-modify-write preserved nothing while paying a synchronous read per entry -- about half the RF-table stage. Measured on one drone-side unit: warm InitWrite 1.30 -> 0.65 s, cold 2.04 -> ~0.7 s. Init (RX-only) opens no batch yet and is unmeasured on a ground-station card, so the RX bring-up path is unchanged by this commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JiAS32956Z3cmTbnmcXYkT
PR Summary by QodoPipeline Jaguar3 USB bring-up writes and add transfer timing
AI Description
Diagram
High-Level Assessment
Files changed (13)
|
…e timing Jaguar3 InitWrite is ~14k synchronous EP0 round trips and nothing else. InitTimer now reports the control-transfer count per stage, and IRtlTransport::write_batch_begin/end pipelines the bring-up's register writes behind EP0's in-order completion: warm InitWrite 1.30 -> 0.65 s, cold 2.04 -> ~0.7 s on one drone-side unit. The RX-only Init path opens no batch yet and is unchanged. Submitted upstream as OpenIPC#417. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JiAS32956Z3cmTbnmcXYkT
Code Review by Qodo
1. Transport guidance can drift
|
| in submission order, so `UsbTransport` queues writes asynchronously inside | ||
| a `write_batch_begin/end` scope and only reads (submitted behind the queue, | ||
| waited on their own completion), bulk transfers and `flush_writes` wait. |
There was a problem hiding this comment.
1. Transport guidance can drift 📘 Rule violation ⚙ Maintainability
src/jaguar3/CLAUDE.md restates the batching API's ordering, wait, threading, and delay-flush contract instead of referring to IRtlTransport and its implementation documentation. A later transport-contract change can update the authoritative headers without updating this backend guide, leaving Jaguar3 maintainers with conflicting instructions.
Agent Prompt
## Issue description
The Jaguar3 guidance duplicates the pipelined-write API contract already documented in the transport headers.
## Issue Context
Keep only Jaguar3-specific bring-up facts here and refer readers to the authoritative transport interface for batching semantics, ordering, waits, and threading requirements.
## Fix Focus Areas
- src/jaguar3/CLAUDE.md[39-55]
- src/RtlTransport.h[71-83]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| /* The event loop itself is gone (a yanked device reports the error | ||
| * immediately, so the turns above cost nothing). Leave the slots | ||
| * submitted and off the free list; the destructor leaks them. */ | ||
| _aw_errors += _aw_inflight; | ||
| _aw_abandoned = true; | ||
| _logger->error("USB: {} pipelined transfer(s) could not be reaped; " | ||
| "their slots are retired for this session", | ||
| _aw_inflight); | ||
| } |
There was a problem hiding this comment.
2. Usb teardown can access freed memory 🐞 Bug ☼ Reliability
flush_writes() can mark submitted control transfers abandoned and return, while each unreaped AsyncWrite retains a raw UsbTransport *self that async_write_cb() uses to update transport counters and its free-list vector. If cancellation callbacks cannot be pumped before destruction but a shared live libusb context later dispatches the transfer, the leaked slot survives while its transport does not, and the callback dereferences freed transport state.
Agent Prompt
## Issue description
Abandoned asynchronous control transfers retain a callback pointer to `UsbTransport`, but the destructor may complete while libusb still owns those transfers. A later callback can then access destroyed transport counters and free-list state.
## Issue Context
Leaking an unreaped transfer slot preserves its transfer and buffer memory, but it does not preserve the `UsbTransport` referenced by `AsyncWrite::self`. Make callback-owned bookkeeping independent of the transport lifetime, or otherwise guarantee that every submitted transfer is reaped and its callback cannot execute after transport destruction, including when another adapter sharing the process continues pumping the libusb context.
## Fix Focus Areas
- src/UsbTransport.cpp[338-355]
- src/UsbTransport.cpp[404-420]
- src/UsbTransport.cpp[486-515]
- src/UsbTransport.h[107-140]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if (_aw_all.empty()) { | ||
| for (int i = 0; i < kAsyncWriteDepth; ++i) { | ||
| auto *w = new AsyncWrite{}; | ||
| w->t = libusb_alloc_transfer(0); | ||
| w->self = this; | ||
| _aw_all.push_back(w); | ||
| _aw_free.push_back(w); | ||
| } |
There was a problem hiding this comment.
3. Low-memory bring-up can crash usb setup 🐞 Bug ☼ Reliability
write_batch_begin() inserts each AsyncWrite into both pool vectors without checking whether libusb_alloc_transfer(0) returned a null t pointer. Under allocation pressure during Jaguar3 TX initialization, the next batched read or write passes that null transfer to the fill and submit path instead of falling back to synchronous bring-up or returning a controlled error.
Agent Prompt
## Issue description
The asynchronous slot pool can accept an `AsyncWrite` whose libusb transfer allocation failed, leaving a null transfer for later fill and submission. Do not enter batch mode with an unusable slot.
## Issue Context
Build the pool transactionally and validate every `libusb_alloc_transfer(0)` result before inserting or exposing a slot. If any allocation fails, release all partially allocated batch slots and leave batching disabled so register access falls back to the synchronous path, or return a controlled error. Other transfer-allocation paths in this transport already check for null before filling or submitting a transfer.
## Fix Focus Areas
- src/UsbTransport.cpp[376-394]
- src/UsbTransport.cpp[439-469]
- src/UsbTransport.cpp[518-539]
- src/UsbTransport.cpp[608-627]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| static uint64_t xfers() { | ||
| return devourer::usb_ctrl_xfers().load(std::memory_order_relaxed); |
There was a problem hiding this comment.
4. Timing records mix adapters' usb work 🐞 Bug ◔ Observability
InitTimer computes each stage's xfers delta from a process-wide usb_ctrl_xfers atomic instead of a counter associated with the timer's transport or adapter. When adapters initialize concurrently or any USB worker performs control transfers during the checkpoint interval, those transfers are attributed to unrelated stages and can also reach PCIe timer metrics that are documented to remain zero.
Agent Prompt
## Issue description
`init.timing.xfers` is intended to describe the control-transfer work performed by a single initialization stage, but `InitTimer` derives it from a process-wide USB counter. Concurrent adapters or workers therefore contaminate one another's stage measurements, and PCIe timers can report USB activity despite being documented to report zero.
## Issue Context
Associate transfer accounting with each `UsbTransport` or adapter, or inject the relevant transport-specific counter into `InitTimer`, so each timer snapshots only its own instance's activity. Preserve a zero-valued source for PCIe timers and count transfers consistently with the documented metric semantics.
## Fix Focus Areas
- src/InitTimer.h[25-45]
- src/UsbXferCount.h[1-14]
- src/UsbTransport.h[55-73]
- src/UsbTransport.h[212-237]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
josephnef
left a comment
There was a problem hiding this comment.
The mechanism is sound and the reasoning behind it is the strongest part of the PR — never zeroing _aw_inflight by hand, retiring unreapable slots rather than handing libusb a dangling transfer, and WriteBatchScope's destructor closing the batch on a throw are all correct, and those comments earn their length. EP0 submission ordering does make the read-behind-writes trick safe, and the tx_async/tx_sync/write_bytes flushes cover the bulk paths.
I'd still hold it. Two of the four substantive issues are cross-variant validation gaps rather than logic errors, which on a bring-up-critical path is the expensive kind: every number in this PR is one 8812EU, and two of the changes ship to 8822C/8812CU as well.
1. The 8822C settle delays don't flush (inline on Halrf8822e.cpp). Halrf8822c::delay_ms is still static and untouched, while the batch scope covers dac_calibrate() and run_iqk() on both variants.
2. The write-only RF-table load is variant-agnostic (inline on HalJaguar3.cpp). The argument is airtight given readback 0 — an RMW that reads 0 writes exactly data, so the two are bit-identical — so the entire risk is whether 8822C's direct window behaves the same, and that's unmeasured.
3. _aw_abandoned is set but never acted on mid-batch (inline on UsbTransport.cpp).
4. libusb_alloc_transfer(0) isn't null-checked (inline).
The rebase is not mechanical. The PR is CONFLICTING. Post-#415 there is no IRtlTransport — RtlAdapter forwards to devourer::ITransport (src/Transport.h), so the three virtuals have to land there, and the IRtlTransport::write_batch_begin doc references in RtlAdapter.h, HalJaguar3.cpp and src/jaguar3/CLAUDE.md need retargeting.
Validation, by the repo's own bar. Neither the pipelining nor the RF-table rewrite has an on-air or regress.py run, and #417 changes how the RF radio tables are programmed on both dies. I'd want a 2x2 on an 8822EU and an 8822CU, plus tests/tx_teardown_asan.sh over the new drain/cancel paths, before this merges — the description already flags that those haven't been run, which is the right call, but it's also the reason to hold.
Everything else is inline and minor.
| std::this_thread::sleep_for(std::chrono::microseconds(us)); | ||
| } | ||
| void Halrf8822e::delay_ms(uint32_t ms) { | ||
| _device.flush_writes(); /* the settle time must follow the writes */ |
There was a problem hiding this comment.
This flush landed on the 8822E calibration only. Halrf8822c::delay_ms (Halrf8822c.h:114) is still static and so structurally cannot flush — and WriteBatchScope in InitWrite covers _cal->dac_calibrate() and _hal.run_iqk() on both variants. Halrf8822c.cpp is full of the same bb_write(...); delay_ms(1..10) settle pairs (188, 957, 1110-1138, 1180-1206, ...).
If the flush is load-bearing here it is load-bearing there; if it isn't, this one is noise. Either way the 8822C DACK/IQK path now runs pipelined with no measurement behind it — every number in the PR is 8812EU. Make it non-static and flush, or say explicitly why 8822C is exempt.
| * no-op that cost a synchronous read per entry -- half the RF table | ||
| * stage, ~80 ms on the ssc338q. Write-only also pipelines | ||
| * (IRtlTransport::write_batch_begin). */ | ||
| _device.rtw_write32(static_cast<uint16_t>(base + ((addr & 0xff) << 2)), |
There was a problem hiding this comment.
rf_writer is variant-agnostic — the same lambda walks radioA/radioB for 8822C and 8822E alike (:1000-1001), so this drops the MASK20BITS RMW on 8812CU/8822CU too.
The argument itself is airtight given readback 0: an RMW whose read returns 0 writes exactly data, so the old and new writes are bit-identical. The whole risk is therefore the premise, and the histogram is one 8812EU. Cheap to close either way — rerun the same 1540-entry histogram on an 8822CU, or gate the plain write on _variant == C8822E until someone does and leave 8822C on phy_set_bb_reg.
| * immediately, so the turns above cost nothing). Leave the slots | ||
| * submitted and off the free list; the destructor leaks them. */ | ||
| _aw_errors += _aw_inflight; | ||
| _aw_abandoned = true; |
There was a problem hiding this comment.
_aw_abandoned is only ever read in write_batch_begin, so setting it here doesn't affect the batch that is currently open. _batch stays true, and the retired slots keep inflight == true, so _aw_inflight > 0 forever.
Every subsequent register access then walks async_take_slot -> async_wait_progress (8 x 250 ms) -> flush_writes (another ~2 s plus the cancel turns) -> nullptr. That's roughly 4 s per register for the remainder of a bring-up that issues thousands of them.
The trigger is narrow — USB_TIMEOUT is 500 ms so libusb normally completes stuck transfers itself, and a genuinely dead loop fails async_wait_progress immediately — but the fix is one line: set _batch = false here so the batch degrades to synchronous for the rest of the session, which is what the comment above already describes as the intent.
| while (_aw_inflight > 0) { | ||
| if (async_wait_progress()) | ||
| continue; | ||
| /* Stuck queue (~2 s without a completion): cancel what is still submitted |
There was a problem hiding this comment.
Minor: with USB_TIMEOUT at 500 ms (UsbTransport.h:28), libusb times a stuck transfer out long before 2 s and completes it through the callback. This path is really only reachable when the event loop isn't being pumped at all — which is what the code below handles correctly, but the "~2 s without a completion" framing points at the wrong cause.
| if (_aw_all.empty()) { | ||
| for (int i = 0; i < kAsyncWriteDepth; ++i) { | ||
| auto *w = new AsyncWrite{}; | ||
| w->t = libusb_alloc_transfer(0); |
There was a problem hiding this comment.
libusb_alloc_transfer can return NULL; w->t then flows straight into libusb_fill_control_transfer in async_write/async_read. OOM-only, but a one-line guard here (bail out of the batch, leave _batch false) costs nothing.
| if (async_read(static_cast<uint16_t>(addr & 0xFFFF), | ||
| static_cast<uint16_t>(addr >> 16), &data, sizeof(data))) | ||
| return data; | ||
| return 0xFFFFFFFFu; |
There was a problem hiding this comment.
The batch path returns the 0xFFFFFFFF sentinel silently while the batch path in ctrl_read below logs and throws. That asymmetry is pre-existing between read32_wide and ctrl_read, but a failed pipelined read is a new and more interesting failure than a failed synchronous one — worth an _logger->error here so it isn't indistinguishable from a register that genuinely reads all-ones.
| * Single-threaded by contract: open a batch only while no other thread | ||
| * touches the transport (the Jaguar3 InitWrite/Init bring-up), and close | ||
| * it before any worker thread starts. Defaults are no-ops (PCIe). */ | ||
| virtual void write_batch_begin() {} |
There was a problem hiding this comment.
Rebase note: after #415 (90e1cad) there is no IRtlTransport — RtlAdapter forwards to devourer::ITransport in src/Transport.h, so these three virtuals need to land there instead, and the IRtlTransport::write_batch_begin references in RtlAdapter.h, HalJaguar3.cpp and src/jaguar3/CLAUDE.md need retargeting. Flagging because it's a real conflict resolution, not a textual one.
| #include <cstdint> | ||
|
|
||
| namespace devourer { | ||
| inline std::atomic<uint64_t> &usb_ctrl_xfers() { |
There was a problem hiding this comment.
Worth stating here that the counter is process-wide while InitTimer is per-device: two adapters brought up in one process cross-attribute their xfers. The demos are one-device-per-process so it doesn't bite today, but the field is going into docs/logging.md as "the transfers the stage spent", which is stronger than what this can promise.
| @@ -1,3 +1,4 @@ | |||
| #include "InitTimer.h" | |||
There was a problem hiding this comment.
Nit: this puts the module's own header second. Convention everywhere else in the tree is HalJaguar3.h first (it's what proves the header is self-contained), then the rest. Same in RtlJaguar3Device.cpp:1.
Jaguar3
InitWriteon an RTL8812EU is ~14k synchronous EP0 round trips andessentially nothing else — no waiting on hardware, just USB control-transfer
latency. This measures that, then removes most of it.
Measuring it
InitTimeralready bracketed init stages with a duration. It now also reportsthe number of USB vendor control transfers each stage spent
(
src/UsbXferCount.h, a process-wide counter bumped byUsbTransport), whichis the unit the bring-up is actually paid in.
init.timinggains anxfersfield;
docs/logging.mdcarries the schema.Every Jaguar3
rtw_hal_init/InitWritestage is bracketed.Removing it
A control transfer costs 76–80 µs synchronous on an embedded host (ssc338q)
and ~27 µs pipelined 8-deep. EP0 completes URBs in submission order, so a
queue of pending writes followed by a read behaves exactly like the
synchronous sequence — the host just doesn't sit through a round trip per
write.
IRtlTransportgainswrite_batch_begin/write_batch_end/flush_writes. Inside a batch,UsbTransportsubmits register writes asasync URBs; a read (submitted behind the queue and waited on its own
completion), a bulk transfer, or an explicit flush is what waits. Jaguar3
InitWriteruns its whole bring-up in one RAII scope, ended before the coexthread starts. The ms-scale settle delays flush first.
All three methods default to no-ops, so
PcieTransportand everygeneration other than Jaguar3 are untouched.
Contract
Batches are single-threaded: open one only while no other thread touches the
transport, and close it before any worker starts. This is enforced by
convention, not by an assert — worth a reviewer's attention.
Failure paths
A drain that times out cancels what is still submitted and keeps pumping for
the cancellations rather than declaring the queue empty. Zeroing the in-flight
count by hand would let a late callback drive it negative (silently disabling
every later drain), return a slot to the free list twice, and leave the
destructor calling
libusb_free_transferon a transfer libusb still owns.Slots that genuinely cannot be reaped — dead event loop, yanked device — are
retired for the session and leaked at teardown instead, which is the lesser
evil against handing libusb a dangling transfer.
Separately: the RF radio-table load is write-only
Bits [31:20] of the direct window read back 0 for all 1540 entries, cold and
warm (histogrammed on one 8812EU unit), so the vendor's
MASK20BITSread-modify-write preserved nothing while paying a synchronous read per
entry — about half the RF-table stage.
Results and limits
Measured on one drone-side unit: warm
InitWrite1.30 → 0.65 s, cold2.04 → ~0.7 s. One unit, one host — the transfer-count reduction is
deterministic, the wall-clock saving is not replicated across parts.
Init(RX-only) opens no batch yet and is unmeasured on a ground-stationcard, so the RX bring-up path is unchanged by this PR and gets none of the
speedup.
cmake --build+ctestgreen (55/55,la_csi_mathskipped — it needspython numpy/scipy). No headless coverage of the pipelining itself: it needs a
real device, and the drain/teardown paths above need a wedged or unplugged
one, which is
tests/tx_teardown_asan.shterritory rather thanctest. Thosehave not been run against this branch.
🤖 Generated with Claude Code
https://claude.ai/code/session_01JiAS32956Z3cmTbnmcXYkT