docs: working context for the RTL8733B backend - #395
Merged
Conversation
The RTL8733B merge added a fifth hardware backend but left the auto-loaded guidance describing four, so the cross-cutting file now contradicts the tree it describes and several blanket "every generation" claims are false on the new part. Root CLAUDE.md: five backends with an RTL8733B entry, the subtree list, the DEVOURER_8733B option, the device-class list (RtlKestrelDevice was missing too), and the naming trap for the BS/BU split. Scope the claims the new backend breaks — FastRetune, TSF/beacons, the runtime TX-power knobs, rx.path and CCA disable are the four Jaguar/Kestrel generations only. Optional device methods are virtual with not-ported defaults rather than pure virtual, which is the general reason a backend can be missing a feature without saying so; state that once at the factory description instead of per feature. Two cross-cutting facts earned during the port: an aggregate that exceeds one URB gets split by xHCI with the descriptor tail and body in separate completions (the RTL8733B is the worked example, and its cap and URB floor are tied by a static_assert so neither can move alone), and nothing may read a register per frame on the send path — one thermal read measured 93% of the per-frame budget on USB high speed. src/rtl8733b/CLAUDE.md: the subtree file every other HAL has. Descriptor geometry and checksum polarity, the aggregate invariant, the TSSI table-switch cost and its settling behaviour, EFUSE exhaustion semantics, the power-sequence teardown ordering, and the admission contract — including which predicates the radiotap path does not share, so the next reader does not assume the legacy paths are kept in step by construction. The bench figures are labelled as single-unit with no in-repo oracle, because the whole backend rests on one module.
PR Summary by QodoDocument RTL8733B backend context and update root guidance
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
…e header Review follow-ups. InitWrite warns when tuning.disable_cca is set. SetCcaMode(true) already refuses loudly, so the config path was the one door where the same request vanished silently — an operator would believe carrier-sense was off and read the resulting deferral as a transmitter fault. Warn rather than throw: the knob is on by default for the streamtx downlink, and refusing to bring TX up over an unported optimisation is the worse trade. The subtree CLAUDE.md restated the admission contract that TxDescriptor8733b.h already doc-comments, against the standing rule about not duplicating headers. It now points at the header and keeps only what the header cannot say: that the two legacy paths are not held in step by a shared predicate, and that the CCK band gate closes end-to-end. Drop the ~5 dB TSSI overshoot figure. No SDR has been on this part, so an RF-domain magnitude is not ours to quote; the settling behaviour is stated qualitatively with the measurement deferred. The timing figures that remain are register-sequence and USB-transfer costs, and say so.
This was referenced Aug 13, 2026
josephnef
pushed a commit
that referenced
this pull request
Aug 14, 2026
…e-table knob (#396) The two post-merge findings, rebased onto master and answering the review points on both. Fixes #389. ## The send path no longer touches a register This backend was the only one doing register I/O inside `send_packet` — it re-selected the CCK/OFDM thermal-compensation curve on every rate-class crossing at 84 ms / 136 USB round trips (#389). `configure_tx_power` now picks the curve once per channel set and leaves it, which is what the vendor does: `_halrf_tssi_set_tmeter_tbl_8733b` is reachable only from full TSSI setup, keyed on `phydm_get_tx_rate` at that instant, never re-selected at runtime. Measured on the DUT — alternating CCK/OFDM, 200 frames: **19.69 s → 2.82 s, 200 table switches → 0**, TSSI still enabled. `send_packet` now has no conditional register-I/O path at all. The TSSI loop itself stays on wherever the EFUSE is TSSI-offset PG, because there it *is* the TX-power control. An early cut made TSSI opt-in with a flat-index fallback; witnessed, that could not carry HT at all — **MCS7, 300/300 submitted, 0 captured, twice**. ## No runtime table switch, and the measurement that closes it The first revisions carried `DEVOURER_TSSI_RATE_TABLE` as an opt-in for per-crossing switching, on the argument that the hot thermal-delta regime is where the two curves are *designed* to differ. Both reviewers landed on "probably delete it". A heat soak turned that from a judgement call into a measurement, so **the knob is deleted** — it could never have done anything on this part. **The two curves are bit-identical until thermal delta +18.** Reconstructing the plan against the vendor 64-entry layout: the swing ramp starts at index 18 (CCK swing 0 for 0..17, then 3,3,5…; the OFDM plan is all-zero), and every word either side of the baseline is zero in both tables. Below +18 the CCK table and the OFDM table are the same bits. **The part plateaus at +8.** Five-minute max-duty MCS7 soak (`DEVOURER_TX_GAP_US=0`, ~5000 fps submission), thermal polled every 2 s: ``` raw 33 -> 40 delta +1 -> +8 trajectory: 33/1, 37/5, 38/6, 39/7, 39/7, 39/7, 40/8, 40/8, 40/8, 40/8, 40/8 ``` It stopped climbing after about two minutes and held +8 for the remaining three — asymptotic, not still-rising-when-I-stopped. Less than half the delta needed for a single table entry to differ. A runtime switch would therefore spend 84 ms and 136 USB register round trips, inside `send_packet`, to install a table bit-identical to the one already loaded. Deleted with it: `select_tssi_rate_table`, the `DeviceConfig` field, the env translation, and `_tx_fatal` — whose only producer was that function's failure path, so keeping it would have left a permanently-null member guarding a dead branch. Re-verified on air after the deletion: MCS7 279/300, 1 Mbps CCK 300/300 — unchanged from 277/300 and 300/300 before it. **+8 is a property of this unit, not of the silicon.** Bare module, open air, room ambient, and submission-rate duty rather than measured radiated duty. A sealed BL-M8733BU2 at high ambient could plausibly reach +18 where this one cannot. The **+18 threshold** is the durable half — it comes from the vendor table contents, not the unit. So the code and docs state the reasoning rather than the conclusion, and both point at #389 for the validated in-place implementation (13.8 ms, tracking left enabled) as the thing to revisit if a board ever does get hot enough; #390's heat-soak work now has a specific number to test against. ## `disable_cca` Took the reviewer's call — kept my wording naming both spellings, dropped theirs. One placement change on top: it sits in `bring_up_to_phy` rather than `InitWrite`, so an RX-only session that set the knob is told too, and so it fires exactly once per bring-up rather than twice now that both existed. Verified on hardware: fires once, bring-up completes, TX proceeds. ## Docs `src/rtl8733b/CLAUDE.md` from #395 described the 84 ms as an unconditional per-frame cost and stated the table cannot be changed while tracking is enabled — both now wrong. Rewritten, pointing at #389. Follow-up review pass also removed the drift this PR would otherwise have introduced: both CLAUDE.md sites still placed the `disable_cca` warning at `InitWrite`, and `docs/rtl8733b.md` still narrated two runs in terms of per-frame TSSI table switching — a path this tree deletes, so those runs are not reproducible as described. The rate-coverage and thermal evidence stays; the mechanism framing is gone. ## The witness qualification you suggested Cheap and worth it. Sweeping the RTL8812AU across the HT ladder, 300 frames each: | rate | delivered | witness RSSI | |---|---|---| | MCS0 | 270 / 300 | 57.8 | | MCS1 | 300 / 300 | 65.0 | | MCS3 | 300 / 300 | 65.3 | | MCS5 | 300 / 300 | 65.2 | | MCS6 | 296 / 300 | 65.1 | | MCS7 | 277 / 300 | 60.3 | Flat at 100% through MCS5, so the witness is not measuring itself — a receiver at its cliff falls off progressively at the top rather than dipping at both ends. Both low readings also carried 5-7 units less RSSI than the flat middle, which places the variation in per-run link conditions. ## Checks 53/53, RTL8733B-only 49/49, RTL8733B-only ASan+UBSan 49/49. Hardware verified on the same one-unit DUT: send-path switch count, the heat soak, the single `disable_cca` warning, and the ladder above. Still no SDR on this part, so nothing here asserts an RF-domain magnitude. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The RTL8733B merge (#388) added a fifth hardware backend but left the auto-loaded guidance describing four. The result is a cross-cutting file that contradicts the tree it describes, and — more damaging for an agent reading it — several blanket "every generation" claims that are now false on the new part. This closes both, and adds the
src/rtl8733b/CLAUDE.mdthat every other HAL subtree has.Docs only; no code touched.
Root
CLAUDE.mdStructural, previously stale:
src/{...}/nested-file list, the per-backend HAL list, andDEVOURER_8733Bin the per-chip options.RtlKestrelDeviceas well asRtl8733bDevice.Claims the new backend makes false, now scoped to the four Jaguar/Kestrel generations:
FastRetune, TSF/beacons, the three runtime TX-power knobs,rx.pathper-chain telemetry, and CCA disable.Rather than repeat "except RTL8733B" five times, the underlying rule is stated once at the factory description: optional device methods are virtual with not-ported defaults, not pure virtual, so a backend that hasn't ported a feature inherits
false/0/a full-path fallback rather than a fake.SetCcaModeis called out as the deliberate exception — it is pure virtual precisely so no backend can silently no-op it.Two facts earned during the port that are genuinely cross-cutting, not 8733B trivia:
static_assertthat ties the device cap and the URB floor together so neither can be raised alone.New
src/rtl8733b/CLAUDE.mdFollows the existing subtree files: HAL layout, dispatch, chip facts, then what is not ported. The parts worth having written down are the ones that cost time to learn — the 40-byte descriptor and its inverted checksum polarity (the opposite of some sibling parts), the 12 KiB aggregate invariant, the TSSI table-switch cost and the settling behaviour that makes a fast rate-switching run misreport power, EFUSE address-space exhaustion being success rather than corruption, and why the power-sequence teardown flag is armed before the attempt.
On accuracy
I fact-checked every technical claim in both files against the source before committing, and it caught two of my own errors worth naming:
SetTxModeand radiotap paths "so the two cannot drift".legacy_request_supported_8733bis in fact only reachable viaSetTxMode; the radiotapRATEbranch is constrained downstream byvalid_tx_desc_config's range check instead. The file now documents the asymmetry rather than an invariant that does not hold — adequate today only because the radiotap RATE field cannot express SGI/LDPC/STBC.enable_tssi_trackingrestores its own snapshot, but the transition as a whole leaves tracking off and the caller tears the session down. That is a safe design and now described as what it is. The source comment atselect_tssi_rate_tablestill says "exact rollback" — worth a look, since it is the code's own description that is misleading.All four bench figures are marked as coming from the single validation unit with no in-repo oracle, per the standing rule about not quoting a measurement as though it generalizes.
Follow-up to #388. Deferred hardware work is tracked in #390-#394.