Skip to content

rtl8733b: runtime TX-power offset on the TSSI target - #399

Merged
josephnef merged 8 commits into
OpenIPC:masterfrom
snokvist:feat/8733b-txpower-offset
Aug 16, 2026
Merged

rtl8733b: runtime TX-power offset on the TSSI target#399
josephnef merged 8 commits into
OpenIPC:masterfrom
snokvist:feat/8733b-txpower-offset

Conversation

@snokvist

@snokvist snokvist commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Closes the runtime TX-power gap on the RTL8733B. Follow-up to the FastRetune
port (#398); the issue is snokvist#1, which deferred these knobs out
of #388's scope deliberately.

Why now

Rtl8733bDevice overrode none of the IRtlDevice power family, so every
SetTxPowerOffsetQdb landed on IRtlDevice.h:134 (void)qdb; return 0;.
Measured on a CV610 bench 2026-08-14: 18 dB of commanded offset moved the air
by nothing
, EVM bit-identical across every rung, while a matched 8822EU
control on the same receiver moved 5.6 dB for a 6 dB command and then railed.

That the knob was unported is documented. The defect is that return 0 is
indistinguishable from a successful zero-offset apply — the consumer's state
read {"applied_qdb":0,"saturated_low":false}, which is exactly what a healthy
actuator with travel remaining looks like. Reproduced at register level here:
master writes the same all-zero TSSI target dwords whether asked for 0 or −48.

What it does

On a TSSI-offset PG unit the closed loop is the TX-power control, so the
actuator is its per-rate target table: the five packed dwords at
0x3a00..0x3a10, rewritten in place with tracking left enabled. That is the
shape #389 validated and fast_retune already uses for its per-channel rewrite
— not the ~165 ms disable/re-enable pair. Everything that can refuse is computed
before the first chip write; a failed readback rolls back to the offsets the
chip was carrying.

The offset caps first and shifts second, so the calibrated per-rate spread
survives the move; a lowered ceiling would move only the rates above it. It is
sticky by construction — configure_tx_power re-folds it on every channel set
and FastRetune passes it into the hop rewrite, without which a hop would
recompute from the bare ceiling and silently walk the operator's backoff back
up.

Caps report the dBm-target model Kestrel already uses (index_max = 0, one qdB
per step). Only the relative knob is ported: SetTxPowerIndexOverride is
overridden solely to log a refusal (the base returns void, so silence
would otherwise be the caller's only answer), and SetTxPowerRateDiffs /
ReApplyTxPower keep their false defaults. kSafeTxAgcIndex8733b was
witnessed unable to carry HT at all — MCS7, 300/300 submitted, 0 captured,
twice.

Range: the int8 delta field at both ends, [-128, +127]

The 0x3a00 bytes are a signed int8 offset from the 64 qdBm anchor, so the
field spans targets from -16 dBm to +47.75 dBm. Neither end is re-clamped at
something softer, the same clamped-only-at-the-hardware-rail answer Jaguar1
(±126) and Jaguar3 (±127) give. src/TxPower.h is deliberate that headroom
above the generated table belongs to the operator, and a per-unit EFUSE trimmed
too cold is precisely what a bench calibration exists to correct.

Both knees are measured and documented rather than enforced, because on this
part both sit well inside the field and at points only a bench can find:

  • ~-96 qdB — where the loop stops reducing power. Below it three successive
    rungs read 52.96 / 52.97 / 52.99.
  • ~+32 qdB — where the PA compresses: 8.7 dB louder with EVM collapsed to
    -18. See the table below.

An earlier cut of this PR clamped the floor at -64 (a 0 dBm target) on the
assumption that a negative absolute target was meaningless. That was an
assumption rather than a measurement, in a change whose subject is knobs that
assume instead of checking, and it cost ~9 dB of working backoff — power
keeps falling 7.3 dB past the 0 dBm target with no sign wrap. Corrected in
03daab5.

Measured on air

RTL8731BU DUT → RTL8812AU witness (chip-RSSI ground station — the SDR saturates
at this range, per tests/txpwr_offset_onair.sh). Four independent 6-point
passes, one of them on a cold re-enumeration:

pass slope (dB/qdB) span rms
warm 1 0.222 14.2 dB 1.02
warm 2 0.231 14.8 dB 0.64
warm 3 (harness) 0.227 14.5 dB 1.02
cold replug 0.253 16.2 dB 0.49

Do-nothing control, same session and geometry: the pre-change binary
measured flat — 0.3 dB across the same 64 qdB — because it had no actuator.
Without that control the 14 dB would not be readable.

step_measured stays false, now for a measured reason rather than an
unexamined one: the step is not constant. The bottom 12 qdB delivered 0.125 and
0.126 dB/qdB — the one structure that reproduced exactly — while everything
above −52 ran 0.233..0.242. Same call the 8822E gets, for the same reason.

Overdrive buys ~3 dB and then the PA compresses — EVM is the only tell

Sweeping UP from the clip (MCS0, ch36, witness reporting EVM beside RSSI):

offset RSSI EVM SNR
0 75.4 −62.0 62.0
+16 (top of the PG table) 78.2 −50.2 63.7
+32 84.1 −18.0 57.9
+48 83.7 −18.0 58.1
+64 83.8 −18.0 58.2

+16 is real gain that already cost 12 dB of EVM. +32 is not gain: 8.7 dB
more energy with the constellation collapsed, and +48/+64 move nothing at all.
SNR held 58..64 throughout and never saw it — the failure
docs/bench-testing-near-field.md exists to warn about. So the vendor's PG
table lands about where this part stops being linear: +16 qdB is the edge of
usable overdrive even though the API allows +127. Below the clip EVM is flat
at −58..−61 across all 16 dB of backoff.

Verification

  • ctest 54/54. Per-chip configs: 8733B-only 50/50, 8733B-off
    45/45 (the new tests correctly absent). ASan+UBSan selftest passes.
    Clean full build adds zero warnings — the 8 present are pre-existing in
    vendored headers, chanmig, hopset and Radiotap.c.
  • Cross-builds for armv7 musl (the CV610 craft that runs this adapter).
  • tests/rtl8733b_txpwr_regcheck.sh7/7 on the device, warm and cold:
    caps, offset-0 parity vs a master build, the null control (master ignores a
    −48 request), the −24 shift, both rails, stickiness across
    SetMonitorChannel + FastRetune, and hw_readback after the hop.
  • tests/rtl8733b_txpwr_selftest.cpp in ctest — offset math, shape
    preservation, rails, and kMaxPgTargetQdbm8733b pinned against the generated
    table. Both mutations (drop the shift; implement it as a lowered ceiling)
    fail it.
  • CI note: GitHub Actions is billing-blocked account-wide on the fork, so the
    build matrix above was run locally instead. A red X there is not a content
    failure.

Notes for review

  • GetTxPowerCaps() is static and EFUSE-free even though the actuator
    needs TSSI-offset PG mode. IRtlDevice.h:194 contracts GetAdapterCaps as
    resolved at construction and callable before Init, EFUSE is not read until
    bring-up, and consumers cache it — so mutating caps mid-session would be
    worse. The refusal surfaces on the setter (returns 0, logs) and in
    GetTxPowerState instead. An offset latched before bring-up on such a unit
    is dropped loudly and zeroed by configure_tx_power, so reported state never
    claims an offset no register carries. That path has no hardware coverage —
    every unit seen is TSSI-offset PG — but it writes no registers.
  • A positive session offset lifts bring-up targets above the safe clip by
    design. That is a deliberate widening of what bring-up may program, not a
    hole in enable_tssi_tracking's max_target_qdbm > 64 guard, which still
    refuses a caller that tries to raise the ceiling argument itself.
  • tests/txpwr_offset_onair.sh gained a knob column. Every existing row
    passes DEVOURER_TX_PWR, exactly what the loop hardcoded before, so the
    Jaguar/Kestrel rows are unchanged. The 8733B row sweeps
    DEVOURER_TX_PWR_OFFSET_QDB because DEVOURER_TX_PWR drives the unported
    flat-index knob — verified a no-op on the device (0x10 vs 0x3f produce
    identical registers), so sweeping it would draw a flat line inside the test
    meant to catch flat lines. Its cells stop at 0: the overdrive half is not
    monotone in received power and cannot be scored by an RSSI slope.
  • A separate rtl8733b_txpwr_regcheck.sh rather than a row in
    txpwr_offset_regcheck.sh, because every cell there reads TXAGC indices this
    dBm-model family reports as −1. Kestrel is absent from that script for the
    same reason and has its own.
  • One physical unit, one witness, near-field geometry, integer-quantised RSSI,
    no SDR — as with every other on-air claim in this backend.

snokvist and others added 5 commits August 16, 2026 07:16
Rtl8733bDevice overrode none of the IRtlDevice runtime-power family, so
every SetTxPowerOffsetQdb landed on the base class's `(void)qdb; return 0`.
Measured on a CV610 bench 2026-08-14: 18 dB of commanded offset moved the
air by nothing, EVM bit-identical across every rung, while a matched 8822EU
control on the same receiver moved 5.6 dB for a 6 dB command and railed.
That the knob was unported is documented; the defect is that `return 0` is
indistinguishable from a successful zero-offset apply, and the consumer's
state came back {"applied_qdb":0,"saturated_low":false} — a healthy actuator
with travel remaining.

On a TSSI-offset PG unit the closed loop IS the TX-power control, so the
actuator is its per-rate target table: the five packed dwords at
0x3a00..0x3a10, rewritten IN PLACE with tracking left enabled. That is the
shape OpenIPC#389 validated and fast_retune already uses for its per-channel
rewrite, not the ~165 ms disable/re-enable dance.

The offset caps first and shifts second — clamp(min(target, ceiling) + qdb).
A lowered ceiling would move only the rates sitting above it and silently
flatten the calibrated spread that src/TxPower.h promises to preserve; the
selftest's shape cell fails on exactly that mistake.

Capabilities are the dBm-target model Kestrel already reports (index_max 0,
one qdB per step), range [-64, 0]: offset 0 is kSafeTssiTargetQdbm8733b =
16 dBm, the highest level this backend has characterised, so the knob can
only back off and no un-measured power increase is reachable through the
API. step_measured stays false — the quarter-dB step is what the hardware
target table is denominated in, not a slope anyone has measured on air for
this part. Caps stay static and EFUSE-free per the GetAdapterCaps contract;
a flat-PG unit's lack of an actuator surfaces as a loud refusal from
SetTxPowerOffsetQdb and in GetTxPowerState, not by mutating capabilities.

The offset is sticky by construction: configure_tx_power folds it back in on
every channel set, and FastRetune passes it to the in-place hop rewrite —
without that a hop would recompute targets from the bare ceiling and walk
the caller's backoff back up. GetTxPowerState reports hw_readback from an
actual 0x3a00 read, so the offset is confirmed against the chip rather than
echoed from a shadow that always agrees with itself.

Still not ported, deliberately: SetTxPowerIndexOverride, SetTxPowerRateDiffs
and ReApplyTxPower. kSafeTxAgcIndex8733b was witnessed unable to carry HT at
all (MCS7, 300/300 submitted, 0 captured, twice) and no dB-per-step slope
has been measured for the flat index.

ctest 54/54. On-air slope measurement pending — step_measured flips on that
evidence, not before.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up to the same issue, from two review questions.

1. The range was one-sided, [-64, 0]. That made this the only backend where
   headroom above the generated table is unreachable, against src/TxPower.h's
   explicit statement that such headroom is deliberate and compliance is the
   caller's, and against Jaguar1 (+126) / Jaguar3 (+127) / Kestrel (+12).
   It also blocked the actual consumer: a per-unit EFUSE trimmed too cold is
   exactly what a bench calibration exists to correct, and clamping at the PG
   table would leave a measured operating point uncommandable. The range is now
   [-64, +127], the int8 per-rate delta field, and the shift is symmetric.

2. "Operator owns compliance" is only a fair answer if the operator is told
   what they are choosing between, so the overdrive half was swept with the
   witness reporting EVM beside RSSI (MCS0, ch36):

     offset   RSSI    EVM     SNR
       0      75.4   -62.0   62.0
     +16      78.2   -50.2   63.7    <- top of the PG table
     +32      84.1   -18.0   57.9
     +48      83.7   -18.0   58.1
     +64      83.8   -18.0   58.2

   +16 is real gain (+2.8 dB) that already cost 12 dB of EVM. +32 is not gain
   at all: 8.7 dB more energy with the constellation collapsed, and +48/+64
   move nothing — RSSI and EVM both pinned. The PA is in hard compression, and
   SNR held 58..64 throughout and never saw it, which is the exact failure
   docs/bench-testing-near-field.md warns about. So the vendor's PG table lands
   about where this part stops being linear: +16 qdB is the edge of USABLE
   overdrive even though the field allows +127, and a caller sweeping for its
   own operating point must watch EVM, not RSSI. The backoff half stays clean
   by comparison — EVM flat at -58..-61 across all 16 dB.

kMaxPgTargetQdbm8733b records where the vendor's calibration ends (80 qdBm =
20 dBm at 2.4 GHz), pinned against the generated table by the selftest so the
figure the docs quote cannot drift off the data.

The on-air harness row stays at the backoff half: the overdrive half is not
monotone in received power and cannot be scored by an RSSI slope. It is
characterised with the EVM column in docs/rtl8733b.md instead.

Also adds tests/rtl8733b_txpwr_regcheck.sh — the register-level cells this
family needs, separate from txpwr_offset_regcheck.sh because every cell there
reads TXAGC indices this dBm-model backend reports as -1 (Kestrel is absent
from that script for the same reason). 7/7 on the device: caps, offset-0
parity vs master, the null control (master ignores a -48 request), the -24
shift, both rails, stickiness across SetMonitorChannel + FastRetune, and
hw_readback after the hop. ctest 54/54.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Self-review before upstreaming. Four findings, one of which reintroduced the
very defect this work exists to remove.

1. A latched offset died silently on a flat-PG unit. SetTxPowerOffsetQdb
   accepts an offset before InitWrite and returns it as applied — the demos'
   ordering, and the documented family contract. configure_tx_power's non-TSSI
   branch returned right after set_flat_tx_power and never read _tx_offset_qdb,
   so on a unit with no TSSI calibration the caller got a non-zero "applied"
   for a value no register would ever carry. That is the same
   indistinguishable-from-success shape the issue is about, on the one path
   that has no actuator. The branch now drops the latch loudly and zeroes it,
   so the reported state agrees with the chip. Not fatal — an unported optional
   knob is not a hardware-safety event. No hardware coverage: every unit seen
   is TSSI-offset PG, and the path writes no registers.

2. SetTxPowerIndexOverride was silently ignored while the docs claimed the flat
   index was "refused". IRtlDevice's default returns void and drops the value,
   so on this backend — where the index genuinely is unported — the caller's
   only answer was silence, which is exactly what this header's own rule
   forbids ("unsupported optional controls refuse loudly rather than silently
   no-opping"). Overridden now to log the refusal and name the knob that does
   work. Verified on the device: --flat 32 logs the refusal.
   SetTxPowerRateDiffs keeps the default; its `false` return already says it.

3. hw_readback compared one shadow against the chip. tssi_offsets_confirmed()
   checked the PHY's own _fr_tssi_offsets bookkeeping, which cannot catch a
   disagreement between the device's session state and the PHY's. It now
   recomputes the expected plan from (channel, ceiling, offset) — the offset
   the CALLER believes is applied — so the check is chip-versus-claim rather
   than shadow-versus-itself.

4. GetTxPowerState warned on every call. It is a getter a control loop polls
   (waybeam-link serves GET /api/v1/tx/power from it), so an unconfirmed chip
   would emit one warning per poll forever. Latched to once; the state field is
   the machine-readable signal.

Also documents, at the configure_tx_power call site, that a positive session
offset deliberately lifts bring-up targets above the safe clip — it is a
widening of what bring-up may program, not a hole in enable_tssi_tracking's
`max_target_qdbm > 64` guard, which still refuses a raised ceiling argument.
And drops an unused `steps` out-param.

Verified: ctest 54/54, tests/rtl8733b_txpwr_regcheck.sh 7/7 on the device with
hw_readback still confirming after a hop under the stricter check.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tssi_rate_offsets wrote into the caller's TssiOffsetSat8733b as it walked the
rate table, so a table that failed partway (a missing non-CCK target) returned
nullopt with rail flags already set from the rates computed before it. No
caller reads sat on the nullopt path today, so nothing was wrong on the wire —
but the contract read badly and the fix is a local accumulator published at the
end. ctest 54/54.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The docs claimed -64 qdB was where "the int8 delta field bottoms out". It is
not. The 0x3a00 bytes hold a signed int8 offset from the 64 qdBm anchor, so the
field spans [-128, +127] — targets from -16 dBm to +47.75 dBm. Only the +127
end is a hardware limit.

-64 is a judgement: it is where the ABSOLUTE target reaches 0 dBm, and stopping
there is backed by the sweep rather than by the register width. The bottom
12 qdB of travel already delivers only 0.125 dB/qdB against 0.25 nominal —
the one structure that reproduced exactly across both passes — so the loop is
visibly running out of authority as its target approaches zero, and nothing
below 0 dBm has been characterised. Extending into that region would hand a
controller commands the loop cannot act on, which is the defect this lever
exists to remove; saturated_low marks the boundary instead.

Corrected at all four sites that had it wrong: the GetTxPowerCaps comment,
src/rtl8733b/CLAUDE.md, docs/rtl8733b.md, and the regcheck script header, plus
the TssiOffsetSat8733b declaration (its `low` flag is the absolute floor, not
the field's -128). Comment and documentation only — no behaviour change, and
the code always computed it this way (tssi_rate_offsets floors `shifted` at 0
and notes the delta therefore cannot go below -64).

ctest 54/54.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

RTL8733B: implement runtime TX-power offset via live TSSI target rewrite

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

Grey Divider

AI Description

• Port SetTxPowerOffsetQdb for RTL8733B by rewriting live TSSI per-rate targets.
• Persist offset across SetMonitorChannel and FastRetune; add chip readback confirmation.
• Add selftests, on-device regcheck, and docs clarifying range, rails, and limitations.
Diagram

graph TD
  A["Caller / control loop"] --> B["Rtl8733bDevice::SetTxPowerOffsetQdb"] --> C{"TSSI tracking live?"}
  C -->|"No"| D["Refuse or latch pre-init"]
  C -->|"Yes"| E["Phy8733b::set_tssi_offset"] --> F["Compute plan: tssi_bb_plan / tssi_rate_offsets"] --> G["Write BB regs 0x3a00..0x3a10"] --> H{"Readback matches?"}
  H -->|"No"| I["Rollback to previous targets"]
  H -->|"Yes"| J["Update shadow + saturation flags"]
  K["configure_tx_power / FastRetune"] --> L["Pass offset into enable_tssi_tracking / fast_retune"]
  M["GetTxPowerState"] --> N["Confirm via tssi_offsets_confirmed"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Disable/re-enable TSSI tracking to apply new targets
  • ➕ Simpler correctness story (single enable path writes the full plan).
  • ➕ Avoids partial-write risk without custom rollback logic.
  • ➖ Adds large latency (~165 ms) per adjustment; unsuitable for runtime control loops.
  • ➖ More disruptive (tracking restarts) and duplicates FastRetune’s established in-place approach.
2. Implement TXAGC index-based runtime power for RTL8733B
  • ➕ Aligns with older Jaguar families’ power model (index steps).
  • ➕ Could allow a more uniform dB/step model if characterized.
  • ➖ Measured inability to carry HT at the safe index; no reliable actuator proven.
  • ➖ Requires substantial characterization and likely different safety semantics.
3. Clamp positive offsets at the factory PG target table
  • ➕ Reduces risk of overdrive/PA compression by keeping within calibrated table.
  • ➕ More conservative for regulatory/compliance-sensitive deployments.
  • ➖ Breaks the existing TxPower contract that headroom is operator-controlled.
  • ➖ Prevents correcting under-trimmed EFUSE units via bench calibration.

Recommendation: Keep the PR’s in-place rewrite of 0x3a00..0x3a10 with readback+rollback. It matches the proven FastRetune mechanism, avoids the 165 ms tracking restart, preserves per-rate shape by capping then shifting, and fixes the critical ‘silent no-op’ failure mode by refusing loudly on non-actuated (non-TSSI-calibrated) units while still providing chip-truth confirmation via GetTxPowerState.

Files changed (11) +1031 / -51

Enhancement (4) +449 / -23
Phy8733b.cppAdd offset-aware TSSI target planning and in-place target rewrite actuator +133/-15

Add offset-aware TSSI target planning and in-place target rewrite actuator

• Extends tssi_rate_offsets/tssi_bb_plan/enable_tssi_tracking to incorporate a runtime offset with cap-then-shift semantics and explicit saturation reporting. Adds set_tssi_offset to rewrite 0x3a00..0x3a10 with tracking enabled, verify via readback, and roll back on mismatch; threads the offset through fast_retune to preserve stickiness across hops.

src/rtl8733b/Phy8733b.cpp

Phy8733b.hExpose TSSI offset saturation struct and new PHY APIs for runtime offset +64/-4

Expose TSSI offset saturation struct and new PHY APIs for runtime offset

• Defines TssiOffsetSat8733b and kMaxPgTargetQdbm8733b, updates planning helpers to accept an offset, and adds set_tssi_offset plus tssi_offsets_confirmed for chip-truth verification. Extends fast_retune and enable_tssi_tracking signatures to carry the active offset.

src/rtl8733b/Phy8733b.h

Rtl8733bDevice.cppImplement TX-power caps/state/offset setter for RTL8733B and persist across hops +229/-4

Implement TX-power caps/state/offset setter for RTL8733B and persist across hops

• Implements GetTxPowerCaps (dBm-target model with offset range [-64, +127]), SetTxPowerOffsetQdb with quantization, refusal on non-actuated units, and pre-init latching. Adds GetTxPowerState with chip readback confirmation, threads offset through configure_tx_power/enable_tssi_tracking and FastRetune, and logs refusal for SetTxPowerIndexOverride.

src/rtl8733b/Rtl8733bDevice.cpp

Rtl8733bDevice.hDeclare RTL8733B runtime TX-power APIs and session offset state +23/-0

Declare RTL8733B runtime TX-power APIs and session offset state

• Adds overrides for GetTxPowerCaps/SetTxPowerOffsetQdb/GetTxPowerState and a refusal-only override of SetTxPowerIndexOverride. Introduces session state fields for the applied offset, saturation rails, and one-time readback warning latching.

src/rtl8733b/Rtl8733bDevice.h

Tests (4) +402 / -19
CMakeLists.txtAdd RTL8733B TX-power selftest binary to ctest +6/-0

Add RTL8733B TX-power selftest binary to ctest

• Introduces a new test executable (Rtl8733bTxPwrSelftest) and registers it as a ctest entry when DEVOURER_8733B is enabled.

CMakeLists.txt

rtl8733b_txpwr_regcheck.shAdd on-device register-level validation for RTL8733B TX-power offset +159/-0

Add on-device register-level validation for RTL8733B TX-power offset

• Adds a dedicated script validating caps, parity vs master at offset=0, pre-change null actuator, deterministic target shifts, rail behavior, stickiness across SetMonitorChannel and FastRetune, and GetTxPowerState hw_readback confirmation.

tests/rtl8733b_txpwr_regcheck.sh

rtl8733b_txpwr_selftest.cppAdd headless unit tests for RTL8733B offset math and invariants +197/-0

Add headless unit tests for RTL8733B offset math and invariants

• Introduces a ctest validating cap-then-shift semantics, shape preservation, rail signaling, ability to command above factory targets, and invariants pinned to the generated PG table (including kMaxPgTargetQdbm8733b). Ensures offset=0 remains byte-identical to the pre-knob behavior.

tests/rtl8733b_txpwr_selftest.cpp

txpwr_offset_onair.shGeneralize on-air power sweep to support RTL8733B offset knob +40/-19

Generalize on-air power sweep to support RTL8733B offset knob

• Adds a per-DUT knob column so most devices sweep DEVOURER_TX_PWR while RTL8733B sweeps DEVOURER_TX_PWR_OFFSET_QDB. Updates logging/units in the fit output and extends table comments to document why positive offsets are excluded from RSSI-based monotonicity scoring.

tests/txpwr_offset_onair.sh

Documentation (3) +180 / -9
CLAUDE.mdUpdate top-level docs to reflect RTL8733B runtime TX-power support +9/-4

Update top-level docs to reflect RTL8733B runtime TX-power support

• Adjusts documentation to clarify RTL8733B now ports the relative TX-power offset knob while the flat-index and per-rate-diff knobs still fall back to IRtlDevice defaults. Updates the runtime TX power section to describe the dBm-target model and the effective offset range semantics.

CLAUDE.md

rtl8733b.mdDocument RTL8733B runtime TX-power offset mechanism and measured behavior +99/-1

Document RTL8733B runtime TX-power offset mechanism and measured behavior

• Adds a detailed section describing SetTxPowerOffsetQdb as a TSSI target-table rewrite, its asymmetric range rationale, stickiness across channel/hop, and why step_measured remains false. Extends limitations and measurement notes, including overdrive/EVM behavior.

docs/rtl8733b.md

CLAUDE.mdUpdate RTL8733B backend notes for new runtime TX-power offset +72/-4

Update RTL8733B backend notes for new runtime TX-power offset

• Reframes TSSI closed-loop power as a safe ceiling plus a runtime offset and documents rails, stickiness, and refusal behavior for unported knobs. Adds measured on-air slope/variance and overdrive compression guidance.

src/rtl8733b/CLAUDE.md

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Aug 16, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Offset docs contradict range ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
Rtl8733bDevice now supports positive TX-power offsets up to +127 qdB, but Rtl8733bDevice.h still
documents the session offset as "<= 0" and root CLAUDE.md describes a one-sided [-64, 0] range.
This contradiction can lead to incorrect validation/clamping in future code or confuse operators
about supported behavior.
Code

src/rtl8733b/Rtl8733bDevice.h[R94-97]

+  /* Session TX-power offset (qdB, <= 0) and the rails the last apply hit.
+   * Sticky by construction: configure_tx_power folds it back in on every
+   * channel set, and FastRetune passes it to the in-place hop rewrite. */
+  int16_t _tx_offset_qdb = 0;
Evidence
The header and root documentation claim the offset is non-positive/one-sided, but the implementation
and RTL8733B docs advertise and implement offset_max_qdb = 127 and explicitly describe the range
as [-64, +127].

src/rtl8733b/Rtl8733bDevice.h[94-100]
CLAUDE.md[343-351]
src/rtl8733b/Rtl8733bDevice.cpp[774-786]
docs/rtl8733b.md[140-153]
src/rtl8733b/CLAUDE.md[56-66]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
RTL8733B TX-power offset support is now implemented with a range of `[-64, +127] qdB` (per `GetTxPowerCaps()`), but some documentation/comments still claim the offset is non-positive / one-sided. This creates a misleading contract and can cause future code to clamp incorrectly or users to avoid supported positive offsets.
## Issue Context
The code and RTL8733B-specific docs clearly document `[-64, +127]`, while:
- `src/rtl8733b/Rtl8733bDevice.h` comments `_tx_offset_qdb` as `<= 0`.
- Root `CLAUDE.md` states a one-sided `[-64, 0] qdB` range.
## Fix Focus Areas
- src/rtl8733b/Rtl8733bDevice.h[94-100]
- CLAUDE.md[343-351]
### Suggested changes
- Update the `_tx_offset_qdb` member comment to reflect the actual supported range (e.g., `[-64, +127] qdB`) or remove the `<= 0` claim.
- Update root `CLAUDE.md` to match the implemented caps/range (or explicitly explain “characterized backoff is [-64,0] but API supports +127” if that distinction is intended).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Tip of the day
💡 Did you know, you can add REVIEW.md to your repo root and Qodo follows it on every PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread src/rtl8733b/Rtl8733bDevice.h Outdated
Review question: if "the field is the limit and the operator judges" holds at
the top of the offset range, why does the bottom stop at -64? It did not have a
good answer. -64 was a 0 dBm absolute target, clamped there on the assumption
that a negative target was meaningless — an assumption, not a measurement, in
a change whose entire subject is knobs that assume instead of checking.

Measured it instead (MCS0, ch36, witness RSSI + EVM):

    offset  target      RSSI     EVM
     -32     8.0 dBm    69.40   -60.8
     -64     0.0 dBm    62.07   -59.3   <- the old clamp
     -80    -4.0 dBm    54.80   -54.0
     -96    -8.0 dBm    52.96   -53.1
    -112   -12.0 dBm    52.97   -52.8
    -128   -16.0 dBm    52.99   -52.8

Power keeps falling straight past the 0 dBm target — 7.3 dB more between it and
-4 dBm — with no sign wrap, and pins from about -96 qdB where three successive
rungs agree within 0.03 dB. EVM softens from -59 to -53 across the extension and
the link stays decodable. The old clamp was throwing away ~9 dB of working
backoff: real range for near-field bench work, where an over-strong witness
inverts evidence, and for a link that wants to sit quiet.

So the floor becomes the int8 delta field (-128, a -16 dBm target), symmetric
with the +127 ceiling, and ~-96 qdB is documented as the measured end of usable
travel rather than enforced — exactly how the +32 qdB PA-compression knee is
handled at the other end.

saturated_low now means the field rail rather than the 0 dBm target, and the
selftest says so: -64 rails nothing and the ladder keeps its calibrated spread
through it, -100 still preserves that spread, -128 rails the colder rate while
the hot one sits exactly on the field floor.

Verified on the device: regcheck 7/7 (rails cell now -200 -> -128), on-air
0.223 dB/qdB over the shipped -64..0 range, ctest 54/54.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@snokvist

Copy link
Copy Markdown
Contributor Author

Pushed 03daab5: the offset floor moves from −64 to −128 qdB, and the PR description's range statement is now out of date in one direction — the measurement below supersedes it.

Review question that prompted it: if the field is the limit and the operator judges holds at +127, why did the bottom stop at −64? It didn't have a good answer. −64 was a 0 dBm absolute target, clamped there on the assumption that a negative target is meaningless — an assumption, not a measurement, in a change whose whole subject is knobs that assume instead of checking. So I swept past it (MCS0, ch36, witness RSSI + EVM):

offset target RSSI EVM
−32 8.0 dBm 69.40 −60.8
−64 0.0 dBm 62.07 −59.3
−80 −4.0 dBm 54.80 −54.0
−96 −8.0 dBm 52.96 −53.1
−112 −12.0 dBm 52.97 −52.8
−128 −16.0 dBm 52.99 −52.8

Power keeps falling straight past the 0 dBm target — another 7.3 dB, no sign wrap — and pins from about −96 qdB, where three successive rungs agree within 0.03 dB. EVM softens −59 → −53 across the extension and the link stays decodable throughout.

The old clamp was discarding ~9 dB of working backoff. Usable travel is ~23 dB below the 16 dBm clip, not ~16 dB.

So both ends of the range are now the int8 delta field, and both knees are documented rather than enforced: ~−96 qdB where the loop stops reducing, ~+32 qdB where the PA compresses (EVM −18). saturated_low now means the field rail; the selftest pins that −64 rails nothing and the ladder keeps its calibrated spread through it, −100 still preserves that spread, and −128 rails the colder rate while the hot one sits exactly on the floor.

Re-verified on the device: regcheck 7/7, on-air 0.223 dB/qdB over the shipped −64..0 range, ctest 54/54.

Qodo's only finding on OpenIPC#399, and it was right. The member comment on
_tx_offset_qdb still said "qdB, <= 0" and root CLAUDE.md still described a
one-sided [-64, 0] range — both written when the range genuinely was one-sided,
and neither updated when the positive half landed, let alone when the floor
moved to the field limit. A contract that contradicts the code is how the next
person clamps wrongly.

Both now state [-128, +127] with the measured knees (~-96 qdB down, ~+32 up)
noted as documented rather than enforced. Swept the rest of the tree for the
same claim; nothing else carried it.

ctest 54/54.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
josephnef
josephnef previously approved these changes Aug 16, 2026

@josephnef josephnef left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve, with the two stale-doc fixes below requested before merge. Both are leftovers from the pre-03daab5 cut (the −64 floor that cost ~9 dB) that survived the 2956621 sweep of stale one-sided range claims.

What I verified against the head, by hand:

  • Cap-then-shift math: traced every selftest cell against the implementation — -64 passes through the 0 dBm target unrailed (−64/−68 on the synthetic ladder), -128 puts the anchor exactly on the field floor while the colder rate rails with sat.low. The shifted < -64 clamp = int8 delta −128 = −16 dBm target, all arithmetic in int, no wrap.
  • Rails accumulate locally, publish only on success — a nullopt caller can't see partial flags.
  • Refuse-before-first-write and rollback-on-failed-readback in set_tssi_offset are both real, and _fr_tssi_offsets is correctly left describing the state being restored.
  • Locking: SetTxPowerOffsetQdb / GetTxPowerState / FastRetune / SetMonitorChannel all take _reg_mu, including the FastRetune → full-path fallback.
  • Stickiness plumbing: configure_tx_power re-folds on every channel set — and the post-enable set_tssi_offset call is a nice no-write way to derive chip-truth saturation flags — while FastRetune carries the offset into the hop rewrite.
  • tssi_offsets_confirmed recomputes from (channel, ceiling, offset) rather than checking the shadow against itself — the right design in a PR that exists to abolish self-agreeing shadows, and it makes hw_readback genuinely chip-derived on a write-mostly family.

The validation record is unusually complete for a one-unit backend: master-build null control (what makes the 14 dB readable), four on-air passes including a cold replug, both-rail sweeps, a mutation-killing selftest, and kMaxPgTargetQdbm8733b pinned against the generated table. Every favourable number carries its adversarial counterpart in the same breath, per the house rule.

One non-blocking observation, no change requested: on the flat-PG path GetTxPowerState reports flat_index = agc.ofdm_ref_a. Per src/TxPower.h, flat_index >= 0 conventionally reads as "a flat override is active", which no caller can have set here (the override setter refuses). It is chip truth — the chip really does run a flat index — but a one-line comment at the site acknowledging the reading would spare a generic consumer trying SetTxPowerIndexOverride(-1) to clear it.

Comment thread src/rtl8733b/Phy8733b.h Outdated
Comment thread docs/rtl8733b.md Outdated
Comment thread tests/rtl8733b_txpwr_selftest.cpp
Review follow-ups from OpenIPC#399. All three are leftovers from
the pre-03daab5 cut, where the floor was -64 qdB (a 0 qdBm target) before
the bench sweep showed power keeps falling ~7 dB past it.

- TssiOffsetSat8733b documented the inverse of the shipped clamp: it said
  `low` fires at the 0 qdBm floor "not the int8 field's -128", when the
  code clamps at `shifted < -64` — which IS the field's -128, a -16 dBm
  target — and deliberately does not stop at 0 qdBm. This is the struct a
  closed-loop controller reads to know the knob is out of travel, so it
  is the worst one to have backwards.
- docs/rtl8733b.md still said `[-64, +127]` three paragraphs before the
  same section derives both ends from the int8 field. GetTxPowerCaps
  reports -128.
- tests/txpwr_offset_onair.sh called 0 qdBm "its floor". The compression
  measured there is real, but it is not the floor; same stale claim, in a
  file the review did not cite.

Also: the `+16 qdB rails nothing` cell was missing `up16 &&`. Rails
publish on success only, so a nullopt would leave sat_up default-false
and the cell would pass vacuously. The adjacent cell would still catch
it, but the guard is free.

And a note at the flat-PG GetTxPowerState site: flat_index >= 0 reads as
"a flat override is active" per src/TxPower.h, which no caller can have
set here. It is chip truth — the unit really does run a flat index — so
the comment records the reading rather than changing the value.

ctest 54/54, build clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@snokvist

Copy link
Copy Markdown
Contributor Author

Thanks for the review — all three requested fixes are in 2b5d3a8, threads replied to and resolved. ctest 54/54, build clean.

Summary of the three, since they share one root cause: each was a leftover from the pre-03daab5 cut, where the floor was −64 qdB (a 0 dBm target). The 2956621 sweep caught the one-sided range claims but not the floor location claims, which are a different sentence shape. The TssiOffsetSat8733b one was the worst of them — a controller reading low per that comment stops backing off about 9 dB early, which is exactly the travel the sweep recovered. Sweeping again for the floor phrasing turned up a fourth site you didn't cite, tests/txpwr_offset_onair.sh:65, fixed in the same commit.

On the non-blocking flat_index observation — I took it, as a comment rather than a value change. You've read it exactly as intended: it is chip truth, the unit really does run a flat index, and there is no override to clear. But src/TxPower.h's convention does say flat_index >= 0 means an override is active, so a generic consumer can reasonably try SetTxPowerIndexOverride(-1) to release something nothing set. That call now meets a logged refusal, which is the honest answer, and the site says so:

flat_index >= 0 reads as "a flat override is active" per src/TxPower.h, which no caller can have set here — SetTxPowerIndexOverride refuses, and there is nothing to clear. The index is simply what this unit runs at, because a no-TSSI-calibration EFUSE leaves bring-up's flat index as the level.

I'd rather leave the value as chip truth than report −1 to satisfy the convention, since −1 would claim "the EFUSE per-rate baseline is active" on a unit where it isn't. Happy to reverse that if you'd rather the convention win over the reading.

One scope note for the record: no hardware coverage exists for that flat-PG branch — every unit seen so far is TSSI-offset PG, so the path is reasoned and reviewed but never executed on silicon. It writes no registers, only reads. Called out here rather than buried because it's the one part of this PR the bench record doesn't cover.

The device suites were not re-run for this commit: the diff is four comments, one doc range, and one test guard — no register sequence, no arithmetic, no plan change. Last full device pass on 2956621 stands (regcheck 7/7, on-air 0.223 dB/qdB, cold-replug included). Say the word if you'd like a re-run on the head anyway.

@josephnef josephnef left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving on 2b5d3a8 — verified all three fixes on the head: the TssiOffsetSat8733b comment now states the shipped clamp correctly (int8 field's −128, deliberately not 0 qdBm), docs/rtl8733b.md reads [-128, +127] consistently, and the up16 && guard is in. Credit for catching the fourth instance of the same stale claim in txpwr_offset_onair.sh that my pass missed, and the flat_index convention note at the flat-PG site is exactly what I had in mind. LGTM.

@josephnef
josephnef merged commit 5bf059a into OpenIPC:master Aug 16, 2026
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants