rtl8733b: keep the send path free of register I/O, scope the TSSI rate-table knob - #396
Conversation
Follow-up to OpenIPC#388/OpenIPC#395, addressing @josephnef's post-merge review. **The TSSI thermal table is chosen once per channel set, not per frame.** 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 and 136 USB register round trips per crossing (OpenIPC#389). The vendor never does that — `_halrf_tssi_set_tmeter_tbl_8733b` is reachable only from full TSSI setup, keyed on `phydm_get_tx_rate` at that instant, and is never re-selected at runtime. `configure_tx_power` now picks the curve from the configured TX mode and leaves it. Measured on the DUT: an alternating CCK/OFDM stream drops from 19.69 s to 2.82 s per 200 frames, 200 switches to 0, TSSI still enabled. The closed loop itself stays on wherever the EFUSE is in TSSI-offset PG mode, because there it *is* the TX-power control. An earlier 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. Per-frame switching survives as `DeviceConfig::tuning::tssi_rate_table` / `DEVOURER_TSSI_RATE_TABLE=1`, and its declaration says what it is rather than selling it as a performance toggle: the default is validated at room temperature (1 Mbps CCK, 300/300 on the OFDM table vs 299/300 on the CCK table), the elevated-thermal-delta regime is unmeasured on this part, and if the hot case turns out not to diverge the honest outcome is to delete the knob. Raised in review — a knob shipped as neutral reads as though both settings had been characterized. **`tuning.disable_cca`.** OpenIPC#395 landed the same fix concurrently; per review, this keeps the wording that names both spellings (`DEVOURER_DIS_CCA / tuning.disable_cca`), since the env var is what an operator sets and the struct field is what a library caller sets, and a grep for either should find it. Sited 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. **Docs.** `src/rtl8733b/CLAUDE.md` described the 84 ms as an unconditional per-frame cost and stated the table cannot be changed while tracking is enabled; both are now wrong, and the paragraph is rewritten. `docs/rtl8733b.md` gains the witness qualification review asked for: sweeping the RTL8812AU across the HT ladder gives 100% at MCS1/3/5 and 98.7% at MCS6, so the witness is nowhere near its cliff and the MCS0/MCS7 dips are per-run link variation, not modulation — MCS0 being the most robust rate and also the lowest reading is what rules a cliff out. 53/53, RTL8733B-only 49/49, RTL8733B-only ASan+UBSan 49/49. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PR Summary by QodoRTL8733B: pick TSSI thermal table at TX-power setup; gate per-frame switching
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Two review findings on the opt-in TSSI rate-table path. `src/rtl8733b/CLAUDE.md` restated the characterization that belongs to the `tssi_rate_table` declaration in src/DeviceConfig.h — the room-temperature validation, the unmeasured hot regime, the not-a-neutral-toggle framing. The root CLAUDE.md's own standing rule is to point at the header rather than copy it, so the subtree file now keeps only the subtree fact (the table is chosen once, the vendor does the same, what the transition's failure mode is) and points at the declaration for when the knob is worth paying for. Enabling the knob re-introduces register I/O on the send path. That is its documented purpose, so the invariant it appears to break — nothing reads a register per frame — is a statement about the default, not about every reachable configuration. What was genuinely wrong is that it was silent: a caller who inherited DEVOURER_TSSI_RATE_TABLE from an environment would meet the ~84 ms per rate crossing as an unexplained frame-rate ceiling. InitWrite now warns once when the knob is active and TSSI is running, naming the cost and the issue. The same reasoning as tuning.disable_cca one function above: a knob whose effect the operator cannot see is the one that gets misdiagnosed as a hardware fault. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both bot findings addressed in 1. CLAUDE.md duplicating the knob docs — valid, fixed. The root CLAUDE.md rule is explicit ("never duplicate what a header already doc-comments") and I had restated the room-temperature validation, the unmeasured hot regime and the not-a-neutral-toggle framing in the subtree file. The subtree file now keeps only what is genuinely subtree knowledge — the table is chosen once, the vendor does the same, and what the transition failure mode is and is not — and points at 2. What the finding did surface is a real defect I had missed: the knob was silent. Someone who inherits @josephnef the standing offer from the PR description is unchanged and this does not pre-empt it — if you would rather not carry the knob at all, say so and I will drop it, and the capability should then come back (if #390 shows the hot case diverges) implemented as the validated in-place swap at 13.8 ms rather than the 84 ms teardown/rebuild it uses today. Deleting is a one-commit change; I have kept it only because #390 is the experiment that would justify or bury it. 53/53, RTL8733B-only 49/49, ASan+UBSan 49/49 on both commits. |
The heat soak settles it. The knob is gone, and so is everything that existed only to serve it: `select_tssi_rate_table`, the `DeviceConfig` field, the env translation, and `_tx_fatal` (whose sole producer was that function's failure path). `send_packet` now has no conditional register-I/O path at all. Why it can never matter on this silicon, rather than merely not mattering at room temperature: The CCK and OFDM/HT thermal-compensation curves are **bit-identical for thermal deltas 0..+17** and first differ at **+18** — the vendor swing ramp starts there and the words either side of the baseline are zero in both. A five-minute max-duty MCS7 soak on the validation unit climbed from +1 to +8, plateaued after two minutes and 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. Room temperature agreed independently: 300 frames of 1 Mbps CCK delivered 300/300 with the OFDM table loaded versus 299/300 with the CCK table, zero FCS errors either way. Re-verified after the deletion: MCS7 279/300, 1 Mbps CCK 300/300, unchanged. The table is still chosen from the rate class at TSSI setup, because that costs nothing and is what the vendor does — but the code and docs now say plainly that the choice is not expected to be measurable, so nobody re-derives the switch from first principles. If a future board does reach +18 — a sealed module at high ambient might — OpenIPC#389 holds the validated in-place implementation (13.8 ms, tracking left enabled) to revisit, not the teardown/rebuild this deletes. `_tx_fatal` goes with it rather than being kept as scaffolding: it distinguished "the session was deliberately stopped" from "InitWrite has not run", and with the TSSI transition gone nothing can stop the card from inside `send_packet`, so it would have been a permanently-null member guarding a dead branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ran the heat soak. The knob is deleted — it could never have done anything.
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, 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 ( It stopped climbing after about two minutes and held +8 for the remaining three — asymptotic, not still-rising-when-I-stopped. That is less than half the delta needed for a single table entry to differ. So a runtime switch would spend 84 ms and 136 USB register round trips, inside Re-verified on air after the deletion: MCS7 279/300, 1 Mbps CCK 300/300 — unchanged from 277/300 and 300/300 before it. What I deliberately did not claim+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 now state the reasoning rather than just 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. I have added the same to #390 so the heat-soak work there has a specific number to test against. 53/53, RTL8733B-only 49/49, ASan+UBSan 49/49. The build warnings in the log are pre-existing ( |
josephnef
left a comment
josephnef
left a comment
There was a problem hiding this comment.
Reviewed at ba15a5d. The code change is good — deleting the knob on the table-identity evidence is the right call, and I verified the +18 claim against Phy8733b::tssi_thermal_plan (CCK swing 0 for 0..17, then 3,3,5…; OFDM plan all-zero — both sides of the baseline). Built clean, ctest 53/53 here too. Findings are all doc/consistency, two of them inline below, three here:
-
The PR description is stale — it argues for keeping the knob the head commit deletes. The body still says "I kept it rather than deleting it… say so and I will drop it in this PR", but ba15a5d already dropped it. Please update the description before merge so the merge record matches the change — and the bit-identical-through-+17 evidence deserves to be in the body, it's the strongest part of the PR.
-
Two CLAUDE.md sites still say the
disable_ccawarning fires "atInitWrite". The PR moves it tobring_up_to_phy, but rootCLAUDE.md("setting the config knob warns atInitWrite", ~line 337) andsrc/rtl8733b/CLAUDE.md("InitWritewarns rather than dropping it", ~line 141) weren't updated. Given this PR exists partly to fix #395's doc drift, it shouldn't introduce the same shape of drift. -
docs/rtl8733b.mdstill narrates measurements of the removed mechanism as current behaviour: "submitted 250 frames through a CCK/OFDM TSSI table switch on every frame" (~line 95) and "1,000/1,000 frames through 118 CCK/OFDM TSSI table changes" (~line 125) describe a code path this PR deletes — those runs cannot be reproduced on this tree. The delivery/thermal evidence is worth keeping; the table-switch framing should go (docs are current-state only in this repo).
Nothing here touches the hardware-validated behaviour. Happy to approve once the doc drift and the PR body are fixed.
Review follow-ups on OpenIPC#396, all doc/consistency — no behavioural change. - `_tssi_cck` went write-only when `select_tssi_rate_table` was deleted: set in `configure_tx_power`, cleared in `Stop()`, read nowhere. Deleted rather than kept for a reader that does not exist. - The table-selection comment claimed vendor-equivalent keying without saying the canonical flow reaches it with no mode configured — txdemo calls `InitWrite` before `SetTxMode`, so `_tx_mode_default` is usually unset and a `DEVOURER_TX_RATE=1M` session loads the OFDM/HT table. Left as is (the two tables are the same bits below +18 and the part plateaus at +8), but the comment now says so instead of implying parity. - Root `CLAUDE.md` and `src/rtl8733b/CLAUDE.md` still placed the `disable_cca` warning at `InitWrite` after this PR moved it to `bring_up_to_phy`. - `docs/rtl8733b.md` narrated two runs in terms of per-frame TSSI table switching, a path this PR deletes and which cannot be reproduced on this tree. The rate-coverage and thermal evidence stays; the mechanism framing goes, with the tables' identity at those deltas stated instead. 53/53, RTL8733B-only ASan+UBSan 49/49. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
All five findings addressed in 1. PR description. Rewritten. It no longer argues for keeping the knob it deletes, and the two pieces of evidence you called the strongest are now in the body rather than buried in a thread: the curves are bit-identical for deltas 0..+17 and first differ at +18 (CCK swing 0 through 17, then 3,3,5…; OFDM plan all-zero), and the soak trajectory that plateaus at +8. The "+8 is this unit, +18 is the silicon" split is stated there too. 2. 3. 4 and 5 answered inline: 53/53, RTL8733B-only ASan+UBSan 49/49. |
josephnef
left a comment
josephnef
left a comment
There was a problem hiding this comment.
All five findings addressed in 70e95fc — verified against the diff, not just the replies. The comment at the table-selection site now states the InitWrite-before-SetTxMode ordering and why the gap is deliberately not chased; both CLAUDE.md sites track the warning's real location; docs/rtl8733b.md re-narrates the two runs without the deleted mechanism and pairs the evidence with its scope; _tssi_cck is gone. The commit is no-behavioural-change as claimed (only the two dead-member writes leave the .cpp), and the witness-ladder qualification closes the last open question about the delivery numbers. Approving.
## The friction this removes Merging #396 surfaced a gap in the gate's unblock flow: after resolving the last Qodo thread, the reply-triggered green run lands **beside** the stale red runs rather than superseding them — each trigger event type creates its own workflow run, and the branch-protection rollup counts every run of a required check on the head commit. The PR stayed `BLOCKED` with a green latest run until all four red `pull_request`-event runs were re-run by hand from the Checks tab. ## The fix A passing run now sweeps: it lists this workflow's `failure`-conclusion runs on the same head SHA and `gh run rerun --failed`s each (new `actions: write` permission). Properties: - **Convergent**: only a PASS triggers sweeps, and a re-run that passes finds nothing red left to sweep. - **Still a gate**: if threads are genuinely unresolved, the swept runs go red again and the check keeps blocking — the sweep can only align the rollup with the live resolution state, never bypass it. - **Best-effort**: the sweep step tolerates individual `rerun` failures; it cannot turn a PASS into a FAIL. The failure message now points at the one remaining manual step (reply in a thread to retrigger) instead of also asking for Checks-tab re-runs. ## The alternative, re-tested and still dead Before adding machinery I re-verified the header's claim that GitHub rejects the documented `pull_request_review_thread` trigger — if that parsed, plain thread resolution would retrigger the gate and most of this would be unnecessary. It still doesn't: a scratch workflow carrying `pull_request_review_thread: {types: [resolved, unresolved]}` produced a zero-job "workflow file issue" run on push (run 31778980528). The header comment now records that this was re-verified empirically, not just inherited. ## What this does not change The gate's semantics are untouched: not pinned to head (the anti-treadmill rationale stands), `skip-qodo-gate` label escape hatch, pagination, thread-attribution logic — all as before. One net-new permission (`actions: write`), scoped to the workflow's own runs by construction (`--workflow qodo-gate --commit <head>`). Validation: YAML parses; the sweep's list/filter pipeline is the same query used to unblock #396 by hand. The self-referential part (a green qodo-gate re-running red qodo-gates) can only be observed on a PR that first goes red and then resolves — this PR itself will exercise the gate normally. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
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_powernow picks the curve once per channel set and leaves it, which is what the vendor does:_halrf_tssi_set_tmeter_tbl_8733bis reachable only from full TSSI setup, keyed onphydm_get_tx_rateat 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_packetnow 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_TABLEas 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: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, theDeviceConfigfield, 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_ccaTook the reviewer's call — kept my wording naming both spellings, dropped theirs. One placement change on top: it sits in
bring_up_to_phyrather thanInitWrite, 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.mdfrom #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_ccawarning atInitWrite, anddocs/rtl8733b.mdstill 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:
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_ccawarning, and the ladder above.Still no SDR on this part, so nothing here asserts an RF-domain magnitude.