Jaguar3 8822E: caller-supplied per-rate TXAGC diffs (SetTxPowerRateDiffs) by gilankpam · Pull Request #323 · OpenIPC/devourer

gilankpam · 2026-07-20T16:36:18Z

Adds a third Runtime-TX-power knob on the 8822E: SetTxPowerRateDiffs, a
caller-supplied per-rate TXAGC diff table that replaces the chip's default
phy_reg_pg by-rate walk. Motivating consumer is a wall-equalized rate
ladder — each rate parked a uniform margin below its measured PA-compression
wall.

API (`src/IRtlDevice.h`)

virtual bool SetTxPowerRateDiffs(
    const std::optional<devourer::TxRateDiffsQdb>& diffs);

TxRateDiffsQdb = signed qdB diffs for cck, legacy, mcs0..7
(src/TxPower.h). On the 8822E one qdB == one TXAGC index step.
std::nullopt restores the default by-rate walk.
Follows the Runtime-TX-power family contract: applies live, sticks across
SetMonitorChannel / FastRetune and an override set/clear round-trip,
folded on top of whichever reference (efuse table or flat override) is
active.
Default base returns false — 8822E-only in v1; every other generation
is unaffected.
GetTxPowerState now reports post-diff per-rate indices and a
rate_diffs_custom flag so a caller can read back chip truth.

Reference consumer & validation

examples/txpower/: --rate-diffs cck,legacy,m0..m7|clear and a
--flat-pulse N helper (override + dump + clear in one process).
tests/rate_diffs_selftest.cpp (ctest rate_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 --build clean; ctest 37/37 pass (incl. new rate_diffs).
Rebased cleanly onto current master (only doc/TxPower.h context
overlaps with the recently-merged per-packet-TXpower work, resolved to
keep both features).

…iffs)

…configured != live)

…process

josephnef

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.sh on 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=1 still flagged), spread restored post-clear.
Regression guard tests/txpwr_offset_regcheck.sh over 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-only config 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 N help 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.

josephnef · 2026-07-21T08:20:11Z

+ * IRtlDevice::SetTxPowerRateDiffs; 8822E-only in v1. On the 8822E one qdB
+ * equals one TXAGC index step (step_qdb = 1), so values are used verbatim
+ * as index diffs. */
+struct TxRateDiffsQdb {


The hardware field is 7-bit two's complement, so the usable range is [−64, 63] — but these are int8_t (±127) and pack_rate_diff_word masks with & 0x7f, which silently aliases the sign: a caller asking for −100 gets +28. For the adaptive-controller callers this knob targets, that's a power boost where a big cut was requested.

The codebase already guards the analogous ref wrap ("an unclamped over-range ref would wrap to near-zero TX silently", RadioManagementJaguar3.h). Same treatment here please: clamp to [−64, 63] in SetTxPowerRateDiffs (or reject), and document the range on these fields.

Clamped to [-64, 63] in SetTxPowerRateDiffs before the table is stored, and documented the range on the TxRateDiffsQdb fields + struct comment. With in-range input the & 0x7f mask now encodes 7-bit two's-complement without aliasing the sign. 3d1687b

josephnef · 2026-07-21T08:20:11Z

+    return false;
+  }
+  std::lock_guard<std::mutex> lk(_reg_mu);
+  _rate_diffs = diffs;


Companion to the TxPower.h range comment — this is the natural place for the clamp (or validation-reject), before the table is stored and walked. Everything downstream (apply_rate_diffs_8822e, the GetTxPowerState ref+diff estimate) then only ever sees in-range values.

Done — the clamp lands here in SetTxPowerRateDiffs, before _rate_diffs is stored and walked, so apply_rate_diffs_8822e and the GetTxPowerState ref+diff estimate only ever see in-range values. 3d1687b

josephnef · 2026-07-21T08:20:11Z

+      s.mcs7_index = static_cast<int16_t>(ofdm);
+      s.cck_index = static_cast<int16_t>(cck);
+    }
+    s.rate_diffs_custom = _rate_diffs.has_value();


This sits inside if (_brought_up && !_cw_active), but the flag means "a table is configured" — knowable regardless of bring-up state. As written, a snapshot taken pre-bring-up or during a CW tone reads rate_diffs_custom=false with a table set. Move it outside the readback block, next to the other cached (non-readback) fields.

Moved rate_diffs_custom = _rate_diffs.has_value() out of the readback block — it's reported regardless of bring-up / CW state now. Hoisted _reg_mu over both the flag read and the readback so the non-atomic _rate_diffs read stays race-free (removed the now-redundant inner lock). 3d1687b

josephnef · 2026-07-21T08:20:11Z

+        p = end + 1;
+      }
+      if (n != 10 || *end != '\0') {
+        std::fprintf(stderr, "devourer [E] --rate-diffs wants 10 comma ints "


This validation runs in main after the device is opened and InitWrite has completed — a typo in the diff string costs a full chip bring-up before exiting 2. Every other flag validates in parse_args; parse the 10 ints there too and fail before touching the adapter.

(Also: the strtol → int8_t cast is a second silent truncation layer — 200 → −56. Moot once the library clamps, but a range check here gives the CLI user an actual error message.)

Parsing + range-check [-64, 63] moved into parse_args; a malformed or out-of-range --rate-diffs now fails with an explicit error and exit 2 before the adapter is opened. The range check also replaces the silent strtol->int8_t truncation with a real CLI error. 3d1687b

josephnef · 2026-07-21T08:20:12Z

 }

+/* Caller-supplied per-rate TXAGC diffs (signed qdB vs the reference anchor).
+ * v1 consumer: mabur's wall-equalized ladder — each rate parked a uniform


The why here is genuinely useful (each rate parked a uniform margin below its measured PA-compression wall — keep that). The person's name is provenance clutter in a library header; the PR/git history carries attribution.

Dropped the name; kept the wall-equalized-ladder / PA-compression-margin rationale. 3d1687b

josephnef · 2026-07-21T08:20:12Z

+    else
+        pd_want_mcs7=$((pd_ofdm - 16))
+        pc_want_mcs7=$((pc_ofdm - 16))
+        ok=1


ok is assigned (ok=1, then ok=0 in the failure branches) but never read — dead code, drop it.

Removed. 3d1687b

josephnef · 2026-07-21T08:20:12Z

+#     mcs7-ofdm=0 (mcs7 == ofdm == the flat index, 40) — the honest flat
+#     truth: apply_tx_power_current's flat branch zeroed the chip's per-rate
+#     diffs, and GetTxPowerState must report that chip truth rather than
+#     ref+diff during the override (this is the Issue 1 fix under test: a


"Issue 1" is review-thread context that won't mean anything in-tree. The surrounding sentence already states the invariant (GetTxPowerState must report chip truth, not ref+diff, during an override) — just end it there.

Trimmed to the invariant sentence; dropped the 'Issue 1' thread reference. 3d1687b

…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>

gilankpam · 2026-07-21T22:30:34Z

Pushed 3d1687b addressing the review. The two nits from the summary (no inline thread):

"in v1" phrasing — removed across the feature: IRtlDevice.h, TxPower.h, the warn string, the regcheck header, and the two RadioManagementJaguar3 comments. grep -rn 'in v1' over src/tests is now empty.
--flat-pulse help space — removed the stray space; the description column realigns with the rest of the block.

Local ctest 37/37; the register-level regcheck assertions are unchanged (the clamp only affects out-of-range input, which the bench cells don't exercise).

gilankpam added 7 commits July 20, 2026 23:31

jaguar3(8822e): TxRateDiffsQdb type + diff-word packing (groundwork)

d841d27

jaguar3(8822e): caller-supplied per-rate TXAGC diffs (SetTxPowerRateD…

62ab2ae

…iffs)

jaguar3(8822e): GetTxPowerState reports post-diff per-rate indices

5436d7c

jaguar3(8822e): txpower --rate-diffs reference consumer

a75addb

jaguar3(8822e): rate-diffs regcheck + Runtime TX power docs

eb7429a

jaguar3(8822e): state reports chip truth during flat override (diffs …

f96258d

…configured != live)

jaguar3(8822e): txpower --flat-pulse; regcheck override-clear in one …

a94d43d

…process

josephnef requested changes Jul 21, 2026

View reviewed changes

Uh oh!

Uh oh!

Conversation

gilankpam commented Jul 20, 2026

API (src/IRtlDevice.h)

Reference consumer & validation

Scope

Testing

Uh oh!

josephnef left a comment

Choose a reason for hiding this comment

Review: SetTxPowerRateDiffs (8822E per-rate TXAGC diffs)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gilankpam commented Jul 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

API (`src/IRtlDevice.h`)

Review: `SetTxPowerRateDiffs` (8822E per-rate TXAGC diffs)