Jaguar3 8822E: caller-supplied per-rate TXAGC diffs (SetTxPowerRateDiffs)#323
Conversation
…configured != live)
josephnef
left a comment
There was a problem hiding this comment.
Review: SetTxPowerRateDiffs (8822E per-rate TXAGC diffs)
Well-built PR — this extends the Runtime-TX-power family the way it was designed to be extended: apply_rate_diffs_8822e mirrors the proven pg-walk byte-for-byte (same 0x1c90[15] gate pattern, ref clamps, MRateToHwRate → 0x3a00 + (hw & 0xfc) addressing, set_tx_power_ref's zero range), routing through apply_tx_power_current(full=true) reuses the existing _diffs_zeroed machinery instead of inventing new state, and the one-process flat-pulse regcheck is better test design than most contributions.
Hardware-validated on the bench (2026-07-21):
tests/txpwr_rate_diffs_regcheck.shon an 8822EU (0bda:a81a): 6/6 PASS — apply (mcs7−ofdm = −16 exact), sticky across ch36→ch149 re-fold, and the flat-pulse cell: mid-pulse honest flat truth (mcs7=ofdm=40,rate_diffs=1still flagged), spread restored post-clear.- Regression guard
tests/txpwr_offset_regcheck.shover 4 plugged DUTs (8812AU/J1, 8812BU/J2, 8812CU/J3, 8822EU/J3): 36/36 PASS, canary register dumps byte-identical to master on every DUT — with no diff table configured this PR changes nothing at register level. - CI 18/18 green incl.
8822e-only/8822c-onlyconfig legs.
Requesting changes for one real footgun (the ±64 diff aliasing, inline) plus two small correctness items; nits below.
Nits not worth inline threads:
- "8822E-only in v1" phrasing (
IRtlDevice.h,TxPower.h, the warn string, regcheck header) — this repo keeps docs/comments current-state only; plain "8822E-only" reads the same today and doesn't promise a future. Git is the changelog. --flat-pulse Nhelp line has a stray extra space breaking the column alignment.
Not covered by bench validation (and fine to leave open): whether the chip clamps or wraps when ref+diff exceeds 127 — needs an SDR power sweep. The inline clamp makes the question moot for sane callers either way.
…tion Code-review follow-ups on SetTxPowerRateDiffs: - Clamp caller diffs to the 7-bit two's-complement field range [-64, 63] in SetTxPowerRateDiffs before storing. pack_rate_diff_word masks with & 0x7f, which would alias the sign of an over-range int8_t (a -100 cut lands +28, a power boost). Range documented on the TxRateDiffsQdb fields. - GetTxPowerState: report rate_diffs_custom regardless of bring-up / CW state (it means "a table is configured", knowable without a readback). Hoisted _reg_mu so the non-atomic _rate_diffs read stays race-free. - txpower --rate-diffs: parse and range-check the 10 ints in parse_args so a typo fails before chip bring-up, with a real error instead of a silent strtol->int8_t truncation. - Drop provenance clutter from the TxPower.h header comment (keep the why); drop "in v1" future-promising phrasing across the feature; remove a dead shell var and a review-thread reference in the regcheck; fix a help-column space. ctest 37/37; register-level regcheck assertions unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Pushed 3d1687b addressing the review. The two nits from the summary (no inline thread):
Local |
josephnef
left a comment
There was a problem hiding this comment.
Follow-up commit 3d1687b addresses all seven review threads cleanly, and I re-validated the fixed head on hardware.
Resolutions
- [blocking] ±64 clamp —
SetTxPowerRateDiffsnow clamps to the 7-bit two's-complement field range[-64, 63]before storing, at the top of the setter, so bothapply_rate_diffs_8822eand theGetTxPowerStateref+diff estimate only ever see in-range values. The& 0x7fsign-aliasing footgun is closed. rate_diffs_customreported regardless of bring-up/CW state, under a hoisted_reg_mu(redundant inner lock removed).--rate-diffsparse +[-64, 63]range-check moved intoparse_args, failing before the adapter opens rather than silently truncating.- Dropped the provenance name, the dead
ok=var, the review-thread reference, and the "in v1" future-promising phrasing.
Validation on 3d1687b3
- Build clean; ctest 37/37 green.
- Hardware regcheck 6/6 PASS on the 8822EU (
0bda:a81a): default walk, applymcs7−ofdm=−16, sticky acrossch36→ch149, flat-pulse honest-truth + override-clear restore. - CLI range-check confirmed: out-of-range and wrong-count inputs are rejected before bring-up.
LGTM — thanks for the clean turnaround.
Adds a third Runtime-TX-power knob on the 8822E:
SetTxPowerRateDiffs, acaller-supplied per-rate TXAGC diff table that replaces the chip's default
phy_reg_pgby-rate walk. Motivating consumer is a wall-equalized rateladder — each rate parked a uniform margin below its measured PA-compression
wall.
API (
src/IRtlDevice.h)TxRateDiffsQdb= signed qdB diffs forcck,legacy,mcs0..7(
src/TxPower.h). On the 8822E one qdB == one TXAGC index step.std::nulloptrestores the default by-rate walk.SetMonitorChannel/FastRetuneand an override set/clear round-trip,folded on top of whichever reference (efuse table or flat override) is
active.
false— 8822E-only in v1; every other generationis unaffected.
GetTxPowerStatenow reports post-diff per-rate indices and arate_diffs_customflag so a caller can read back chip truth.Reference consumer & validation
examples/txpower/:--rate-diffs cck,legacy,m0..m7|clearand a--flat-pulse Nhelper (override + dump + clear in one process).tests/rate_diffs_selftest.cpp(ctestrate_diffs) — diff-word packing.tests/txpwr_rate_diffs_regcheck.sh— register-level 0x3a00 validation.Scope
Jaguar3 8822E only; no behavioral change to any other chip/generation
(the default virtual returns false). 12 files, +513/-16.
Testing
cmake --buildclean;ctest37/37 pass (incl. newrate_diffs).master(only doc/TxPower.hcontextoverlaps with the recently-merged per-packet-TXpower work, resolved to
keep both features).