From bd55a5d94df9626a085c64c2eb81e7e7528c44de Mon Sep 17 00:00:00 2001 From: snokvist Date: Sun, 30 Aug 2026 20:08:30 +0200 Subject: [PATCH 1/3] =?UTF-8?q?rtl8733b:=20port=20the=20hardware-ARQ=20sur?= =?UTF-8?q?face=20=E2=80=94=20ACK=20responder,=20retry=20limit,=20honoured?= =?UTF-8?q?=20ACK=20window?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the four items in snokvist/devourer#2, filed by a consumer scoping a ground->craft return path on this die. Three are now measured working; the fourth is root-caused and refused out loud instead of being faked. - **`tx.ack_timeout_us` was silently ignored.** `init_wmac()` hardcoded REG_ACKTO to 0x21 (33 us) and the config field was never read anywhere in `src/rtl8733b/` — so the one library default every generation is supposed to program identically was, on this die alone, pinned to the bottom of the 33..128 us spread that default exists to abolish. It is now applied at bring-up with the same register and 1..255 clamp the Jaguars use, and read back before it is claimed: `/128/33/200 -> 128/128/33/200`. The vendor write stays in `init_wmac()` because that MAC plane is shared verbatim with `rtl8733bprobe`, which wants the vendor recipe and carries no DeviceConfig. - **`SetAckResponder` is ported and measured** — `ack_responder_ok = true`. The `src/AckResponder.h` recipe applies unchanged, which is not an assumption: the vendor tree's own port-0 descriptor names these three registers (net_type `REG_CR_8733B + 2` = 0x0102, MACID 0x0610, BSSID 0x0618). MAC bring-up leaves net_type at No Link because `init_mac` writes only REG_CR's low half, which is exactly why a monitor radio here never ACKed. Measured against an RTL8812AU soliciting TX: armed 1736/1736 frames ACKed at retries_mean 0.00; re-armed on a different MAC, 1736/1736 again; disarmed, 0.00 with retries pinned at 12. The arm refuses a group MAC and is read back before being reported, and teardown disarms it — `_mac.stop()` clears only REG_CR's low half, so a session ending with `teardown_power_down` off would otherwise leave an unowned radio auto-ACKing (verified: after such a session the peer reads ack_rate 0.00). - **`tx.retry_limit` drives real autonomous retransmission** — `tx_retry_limit_ok = true`. It could not be measured the way the Jaguars were: that A/B reads the TX side's own CCX reports and this die has none. New `tests/rtl8733b_retry_limit_onair.sh` judges from the air instead — unicast to an unowned RA so no ACK ever returns, a passive monitor counting airings per submitted frame — and takes a dose-response rather than an on/off pair, because one pair could be ambient and a straight line through three levels cannot: 0/3/12 -> 0.93/3.93/12.27 airings per frame against an expected 1 + N, repeatable over a 0/3/12/0/12 ladder. - **CCX / `tx.report` is NOT ported, and the cause is the firmware.** Everything under this backend's control was verified correct on air — descriptor SPE_RPT and SW_DEFINE at the vendor's own bit positions, C2H decoded at the vendor's dword2[28], bulk-IN as the vendor's delivery path, and the fw-offload C2H format this firmware speaks per its own dispatch — and with an RX loop live and 3160 frames received the chip returned zero C2H packets in any format, with and without net_type armed and with the peer both ACKing and silent. The plumbing is therefore NOT shipped: descriptors stay byte-identical and the knob warns at bring-up, because a knob that looks granted while producing nothing is the exact failure this issue was filed about. The outstanding lead (halmac H2C queue + MEDIA_STATUS_RPT to register the descriptor MACID) is recorded so the next attempt starts where this one stopped. Counterparts, stated because the numbers above are uniformly favourable: one physical unit and one peer generation, no second responder die, no vendor-driver A/B. The retry figure is witnessed by a monitor that can only lose airings, never invent them, so it is a floor; its ~0.07/frame shortfall is the sampling quantization the harness documents. Verification: builds clean on default / 8733B-only / 8733B-off / ASan+UBSan with ctest green in each; lifecycle soak 6/6 HEALTHY; monitor RX unaffected by arming the responder; ASan teardown clean on hardware, and the 8733B added to `tests/teardown_gen_sanity.sh`, whose cell list predated the chip. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VjcZRPjNtsy2mxfDkAWNpd --- CLAUDE.md | 5 +- docs/rtl8733b.md | 48 ++++++++- docs/scheduled-mac.md | 15 ++- src/AdapterCaps.h | 9 +- src/rtl8733b/CLAUDE.md | 87 +++++++++++++++- src/rtl8733b/Halmac8733bMac.cpp | 15 +++ src/rtl8733b/Halmac8733bMac.h | 5 + src/rtl8733b/Rtl8733bDevice.cpp | 155 ++++++++++++++++++++++++++++ src/rtl8733b/Rtl8733bDevice.h | 9 ++ tests/rtl8733b_retry_limit_onair.sh | 123 ++++++++++++++++++++++ tests/teardown_gen_sanity.sh | 2 + 11 files changed, 463 insertions(+), 10 deletions(-) create mode 100755 tests/rtl8733b_retry_limit_onair.sh diff --git a/CLAUDE.md b/CLAUDE.md index ad0b2c95..f594843e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -59,9 +59,10 @@ construction from the `SYS_CFG2` chip-id (Kestrel: PID-first): only what an independent witness decoded — legacy OFDM + HT MCS0-7, BCC, 20/40 MHz on 2.4/5 GHz, plus long-preamble CCK on 2.4 GHz at 20 MHz, plus 10 MHz narrowband (5 MHz refused — `src/rtl8733b/CLAUDE.md`). - Everything the backend has not ported (TSF/beacons, hardware ACK, A-MPDU, + Everything the backend has not ported (TSF/beacons, A-MPDU, CCX/`tx.report`, the flat-index and per-rate TX-power knobs) falls through to `IRtlDevice`'s - not-ported defaults rather than being faked, so read the base class before + not-ported defaults rather than being faked (hardware ACK/BlockAck IS ported + and measured, as is `tx.retry_limit` — `src/rtl8733b/CLAUDE.md`), so read the base class before assuming a cross-generation feature below applies here. `FastRetune` IS ported (intra-band, TSSI kept live — `src/rtl8733b/CLAUDE.md`). SGI, LDPC, STBC, VHT and HE are refused outright. Scope, one-unit validation record and the diff --git a/docs/rtl8733b.md b/docs/rtl8733b.md index 21b266b6..719d34e4 100644 --- a/docs/rtl8733b.md +++ b/docs/rtl8733b.md @@ -370,6 +370,33 @@ silently or failing to initialise. Silence is the only outcome that would let a refused knob look like a granted one; a hard failure would be a harsh answer to a request a caller may be making only through an inherited environment. +## Hardware ARQ + +Opened by issue #2; measured against an RTL8812AU peer on the validation unit. +Both directions of a reliable-unicast link now work on this die. + +| knob | state | evidence | +| --- | --- | --- | +| `tx.ack_timeout_us` | honoured (was silently pinned at 33 us) | REG_ACKTO read back at bring-up: `/128/33/200 -> 128/128/33/200` | +| `SetAckResponder` | ported, `ack_responder_ok = true` | 8733B as responder: armed 1736/1736 ACKed, retries_mean 0.00; re-armed on a different MAC 1736/1736; disarmed 0.00 with retries pinned at 12 | +| `tx.retry_limit` | live, `tx_retry_limit_ok = true` | airtime dose-response 0/3/12 -> 0.93/3.93/12.27 airings per frame (expected 1 + N), repeatable over a 0/3/12/0/12 ladder | +| `tx.report` (CCX) | **not** ported — firmware emits nothing | see Known gaps below | + +Implementation detail, the register recipe and the full CCX bench narrative: +`src/rtl8733b/CLAUDE.md` "Hardware ARQ". The cross-generation ARQ matrix this +die now joins is `docs/scheduled-mac.md`. + +The retry measurement could not use the 12/0/12 CCX A/B the Jaguars are judged +by, because this die has no `tx.report` to judge itself with; it counts airings +at a passive monitor instead (`tests/rtl8733b_retry_limit_onair.sh`), and takes +three dose levels rather than an on/off pair so that a straight line — not a +single delta that ambient conditions could fake — carries the claim. Its +counterparts: one physical unit and one peer generation, like every other +on-air claim in this section — note the document as a whole rests on two units, +but these ARQ cells ran on the `f72b` sample only; the monitor can only lose +airings, never invent them, so the measured ratio is a floor; and the ~0.07/frame shortfall is the `rx.txhit` +sampling quantization the harness documents rather than loss. + ## Known gaps and deferred validation These results have **not** been claimed: @@ -403,9 +430,24 @@ These results have **not** been claimed: the normal NIC image. - SGI remains disabled: a descriptor with its short-GI bit set submitted successfully, but an independent RTL8812AU decoded both broadcast and - unicast probes as long GI. STBC and LDPC remain rejected. ACK/BlockAck - response, A-MPDU and throughput were not independently - validated and remain unadvertised or unsupported by this backend. + unicast probes as long GI. STBC and LDPC remain rejected. A-MPDU and + throughput were not independently validated and remain unadvertised or + unsupported by this backend. (ACK/BlockAck response is no longer in this + list — it is ported and measured, see Hardware ARQ below.) +- No CCX path, so no `tx.report` events — and unlike every other gap here the + cause is the firmware, not a missing port. Everything under the backend's + control was verified correct on air (descriptor SPE_RPT/SW_DEFINE set at the + vendor's own bit positions, C2H decoded at the vendor's dword2[28], bulk-IN + delivery as the vendor uses, and the fw-offload C2H format this firmware + speaks per its own dispatch), yet with an RX loop live and 3160 frames + received the firmware returned **zero** C2H packets in any format — with and + without net_type armed, peer ACKing and silent. The outstanding lead is the + halmac H2C queue + a MEDIA_STATUS_RPT registering the descriptor MACID; this + backend has no H2C transport at all. The knob warns at bring-up instead of + stamping descriptors that buy nothing. Consequence: an 8733BU can be either + end of a hardware-ARQ link but cannot see per-frame delivery, so detecting a + departed peer needs an application-level timeout + (`src/rtl8733b/CLAUDE.md` "Hardware ARQ"). - Fast retune is now ported and independently witnessed (intra-band, same-width; `src/rtl8733b/CLAUDE.md` has the measured contract and its counterparts): channel-state readback parity 7/7 hops, a 299/300 post-hop diff --git a/docs/scheduled-mac.md b/docs/scheduled-mac.md index d3ec34a4..286894f2 100644 --- a/docs/scheduled-mac.md +++ b/docs/scheduled-mac.md @@ -173,6 +173,13 @@ scheduled MAC runs TX+RX anyway, so this is the relevant session shape. | Jaguar2 8812BU | 0.91 / 2.1 (run-to-run 0.12–0.91) | 0.64 / 5.3 | yes (12) | 0.86 | 0 | | Jaguar3 8822CU | 1.00 / 0.24 | 1.00 / 0.13 | yes (12) | 0.96 | 0 | +The RTL8733B is deliberately absent from this table: it is the one die that +runs closed-loop hardware ARQ with **no CCX report at all**, so none of these +columns can be filled for it as the soliciting TX. Its retry knob is measured +from the air instead (airings per submitted frame, 0/3/12 -> 0.93/3.93/12.27) +and its responder side is measured with this same harness pointed the other +way — `docs/rtl8733b.md` "Hardware ARQ". + The OFF-phase pin is set by `DEVOURER_TX_RETRY_LIMIT` (the matrix runs 12, the value the descriptors used to hardcode) — the knob, not a descriptor constant, is now the single source of truth for the retry limit on @@ -251,6 +258,7 @@ nonzero limit for absolute numbers; 8821AU row re-measured ch6): | 8821AU | 62% | 0% | works (94% closed-loop at retry 8) | | 8812EU | 98% | 0% | works | | 8812CU | 69% | 0% | works | +| 8733B | 100% (retry 12) | 0% | works (1736/1736, retarget- and disarm-proof) | | 8852CU (Kestrel) | 0% | 0% | not implemented (SetAckResponder is J1/2/3-only) | Unmeasured for lack of plugged hardware: 8821CU / PCIe 8821CE (recipe-shared @@ -300,8 +308,11 @@ carry this table per die. 2,048-bit window leaked 2,846 delivered frames out of coverage when a stalled spsc-fat pool drained ~3 k frames in one receipt interval; the 8192 default clears that bench worst case ~2.7×). -2. **Closed-loop hardware ACK + autonomous retry is GO on Jaguar1 and - Jaguar3** (100% delivery, retries ≈ 0.2–0.3) including retargeting an +2. **Closed-loop hardware ACK + autonomous retry is GO on Jaguar1, Jaguar3 and + the RTL8733B** (100% delivery, retries ≈ 0.2–0.3; the 8733B closes the loop + as responder at 1736/1736 but has no CCX report of its own, so its TX-side + retry evidence is airtime rather than `tx.report` — `docs/rtl8733b.md`) + including retargeting an arbitrary UE MAC mid-session (re-arm `SetAckResponder`, change the descriptor RA — both fully dynamic). Requires a nonzero `DEVOURER_TX_RETRY_LIMIT` — the hardware ARQ loop retransmits until ACK diff --git a/src/AdapterCaps.h b/src/AdapterCaps.h index 4137094d..20e2a35f 100644 --- a/src/AdapterCaps.h +++ b/src/AdapterCaps.h @@ -175,12 +175,17 @@ struct AdapterCaps { * an earlier "broken" verdict was a harness artifact: the responder's arm * was never verified, so a silently dead responder read as on=0/off=0), * 8822B, 8812C/8822C, 8812E/8822E (the 8811A rides the 8812 die path and - * inherits its row). False-as-unmeasured (the + * inherits its row), 8733B (1736/1736 frames ACKed at retries_mean 0.00, + * retarget-proof and disarm-proof — tests/ack_txreport_matrix.sh run with + * the 8733B as the responder). False-as-unmeasured (the * vht_2g4_ok reading: unmeasured, not incapable): the 8821C — it shares * the recipe but no 8821CU/CE cell has run. FALSE on Kestrel: * SetAckResponder is not implemented on the AX generation. * tx_retry_limit_ok: DEVOURER_TX_RETRY_LIMIT drives hardware autonomous - * retransmission (measured 12/0/12 A/B: 8821AU, 8812BU, 8822CU; Kestrel + * retransmission (measured 12/0/12 A/B: 8821AU, 8812BU, 8822CU; the 8733B + * by airtime dose-response instead, 0/3/12 -> 0.93/3.93/12.27 airings per + * frame, because that die has no CCX path to judge its own frames + * (tests/rtl8733b_retry_limit_onair.sh); Kestrel * 8832CU witness-measured — the AX WD DATA_TXCNT_LMT field counts * ATTEMPTS, folded +1 to the N-retries contract, limits {0,2,8} -> modal * on-air copies {1,3,8-9}). FALSE on the 8814A die (the vendor diff --git a/src/rtl8733b/CLAUDE.md b/src/rtl8733b/CLAUDE.md index cf590eba..ccd0c5e3 100644 --- a/src/rtl8733b/CLAUDE.md +++ b/src/rtl8733b/CLAUDE.md @@ -240,7 +240,7 @@ untouched) and fall back to the full path. ## Not ported -`ReadTsf`/beacons, hardware ACK/BlockAck, A-MPDU, +`ReadTsf`/beacons, A-MPDU, CCX / `tx.report` per-frame TX outcomes, `FastSetBandwidth`, the flat-index / per-rate-diff TX-power knobs (`SetTxPowerIndexOverride`, `SetTxPowerRateDiffs`, `ReApplyTxPower` — only the relative `SetTxPowerOffsetQdb` is ported), `rx.path` per-chain telemetry, @@ -276,6 +276,91 @@ request the setter refuses loudly instead vanishes without a word. The warning sits in `bring_up_to_phy`, not `InitWrite`, so an RX-only session that set the knob is told too, and so it fires exactly once per bring-up. +## Hardware ARQ + +All three knobs of the hardware-ARQ surface were opened by issue #2. Two are +now measured true on this die; the third is refused loudly for a reason that +took a bench to find. + +The counterparts for everything measured below, stated once here because the +numbers that follow are uniformly favourable: **one physical unit and one peer +generation** (the `f72b` RTL8731BU against an RTL8812AU), the same caveat every +on-air claim in this subtree carries. Neither direction has been cross-checked +against a second responder die, the second `b733` sample has not run these +cells, and no vendor-driver A/B exists to compare against. The retry number in +particular is witnessed by a monitor that can only ever LOSE airings, never +invent them, so the measured ratio is a floor rather than a point estimate. + +`DeviceConfig::tx::ack_timeout_us` **is** honoured, and was not until that +issue: `bring_up_to_phy` overwrites REG_ACKTO (0x0640) from the config with the +same 1..255 clamp the Jaguars use, right after MAC bring-up wrote the vendor +0x21 — 33 us, the bottom of the 33..128 per-chip spread that field's one +default exists to abolish. The write is read back and logged, because a value +reported without a readback is the same shape of claim the bug was; measured +`/128/33/200 -> 128/128/33/200`. The CCK companion 0x0639 keeps its +vendor value, as on every other generation. + +**`SetAckResponder` is ported and measured.** The `src/AckResponder.h` recipe +applies unchanged — not an assumption, the vendor's own port-0 descriptor names +these three registers (`hal/rtl8733b/rtl8733b_ops.c` `port_cfg[0]`: +net_type `REG_CR_8733B + 2` = 0x0102 shift 0, macaddr 0x0610, bssid 0x0618). +MAC bring-up leaves net_type at No Link because `init_mac` writes only REG_CR's +low half, which is exactly why a monitor radio here never ACKed. Teardown +disarms it explicitly, because `_mac.stop()` clears only that same low half and +net_type at 0x0102 would survive it: a session ending with +`teardown_power_down` off would otherwise leave the chip auto-ACKing with no +session owning it. Verified on air — after an armed session ends with +power-down disabled, the peer's reports read ack_rate 0.00 with retries pinned +at 12. The disarm is guarded like the TSSI rollback (register reads throw on a +disconnected device, and losing the rest of teardown is worse than losing the +disarm) and is skipped entirely when nothing was armed. The arm is +read back before it is claimed, and refuses a group MAC outright (a station +cannot ACK-target one — the I/G footgun, again). Measured against an RTL8812AU +soliciting TX (`tests/ack_txreport_matrix.sh`, 8733B as RESPONDER): armed +1736/1736 frames ACKed at retries_mean 0.00; re-armed on a **different** MAC, +1736/1736 again (the address is arbitrary, not baked in); disarmed, 0.00 with +retries pinned at the descriptor limit of 12. So this die can be the receiving +end of a reliable-unicast link. + +**`tx.retry_limit` drives real autonomous retransmission** — `tx_retry_limit_ok` +is now true. It could not be measured the way the Jaguars were: that A/B reads +the TX side's own CCX reports, and this die has none. `tests/rtl8733b_retry_limit_onair.sh` +judges from the air instead — unicast to an unowned RA so no ACK ever returns, +a passive monitor counting airings per submitted frame — and takes a +dose-response rather than an on/off pair, because one pair could be ambient and +a straight line through three levels cannot. Measured 0 -> 0.93, 3 -> 3.93, +12 -> 12.27 airings/frame against an expected 1 + N, repeatable across a +0/3/12/0/12 ladder. The ~0.07 shortfall is the `rx.txhit` sampling +quantization the harness documents (the event fires on the first 10 hits then +every 100th, so its `hits` field understates by up to 99), not loss. + +**CCX / `tx.report` is NOT ported, and the reason is the firmware.** This is +the one entry on the Not-ported list whose cause is known but not fixable from +the descriptor side, so it is recorded in full to save the next person the +bench time. Everything under this backend's control was verified correct on +air: SPE_RPT is dword2[19] and SW_DEFINE dword6[11:0] via the generic halmac +NIC macros the 8733B maps straight onto (`hal/halmac/halmac_tx_desc_chip.h` +maps `SET_TX_DESC_{SPE_RPT,SW_DEFINE}_8733B` onto the non-V2 pair, not the +0x20/0x24 V2 placement), and a probe build confirmed the bit set in a live +descriptor; the RX side already decodes C2H at the vendor's own dword2[28] +(`GET_RX_DESC_C2H_8733B`); the delivery path is bulk-IN, which is what the +vendor uses (its USB interrupt handler is an empty stub behind an undefined +config); and the vendor's C2H dispatch confirms this firmware speaks the +fw-offload format `parse_ccx_halmac` already decodes (`C2H_EXTEND` 0xFF + +sub_cmd 0x0F, `hal/rtl8733b/rtl8733b_cmd.c`). With an RX loop live and 3160 +frames received, the firmware returned **zero** C2H packets in any format — +with and without a net_type armed, and with the peer both ACKing and silent. +The outstanding lead is the halmac H2C queue plus a MEDIA_STATUS_RPT +registering the descriptor MACID with the firmware; this backend has no H2C +transport at all, which is the gap to close first. Until then the knob warns +at bring-up rather than stamping descriptors that buy nothing — `tx.report` +requested on this die is refused out loud, not silently dropped. + +Consequence a consumer should plan around: an 8733BU can be **either end** of a +hardware-ARQ link, but it cannot see per-frame delivery. `TxReport.state == 1` +— the retry write-off that says a peer stopped ACKing — is unavailable here, so +detecting a departed peer needs an application-level timeout. + ## Validation status On-air claims rest on **two** physical units: the original bare unbranded 1T1R diff --git a/src/rtl8733b/Halmac8733bMac.cpp b/src/rtl8733b/Halmac8733bMac.cpp index 59fe4bd3..f4705479 100644 --- a/src/rtl8733b/Halmac8733bMac.cpp +++ b/src/rtl8733b/Halmac8733bMac.cpp @@ -636,6 +636,13 @@ void Halmac8733bMac::init_wmac() { _device.rtw_write32(kRegMar, 0xffffffff); _device.rtw_write32(kRegMar + 4, 0xffffffff); _device.rtw_write8(kRegBbpsfCtrl + 2, 0x84); + /* Vendor HALMAC values. 0x21 (33 us) is the bottom of the 33..128 us + * per-chip spread DeviceConfig::tx::ack_timeout_us exists to abolish, so + * Rtl8733bDevice overwrites REG_ACKTO from the config right after + * bring-up (set_ack_timeout_us) — the same shape Jaguar3 uses against + * halmac's per-bandwidth defaults. Left as-written here because this MAC + * plane is shared verbatim with rtl8733bprobe, which wants the vendor + * recipe and carries no DeviceConfig. */ _device.rtw_write8(kRegAckTimeout, 0x21); _device.rtw_write8(kRegAckTimeoutCck, 0x6a); _device.rtw_write16(kRegEifs, 0x0040); @@ -673,6 +680,14 @@ void Halmac8733bMac::init_wmac() { _device.rtw_write8(kRegWmacOption1, 0x18); // 0x98 with early-drop disabled } +uint8_t Halmac8733bMac::set_ack_timeout_us(uint8_t microseconds) { + _device.rtw_write8(kRegAckTimeout, microseconds); + /* Read back and return what the register carries. The caller logs it: this + * knob was silently ignored here once (issue #2), and a value reported + * without a readback is the same shape of claim that bug was. */ + return _device.rtw_read8(kRegAckTimeout); +} + void Halmac8733bMac::init_usb() { uint8_t mode = 0x0e; // DMA mode + burst count 3 if (_device.rtw_read8(0x00ff) != 0x20) { diff --git a/src/rtl8733b/Halmac8733bMac.h b/src/rtl8733b/Halmac8733bMac.h index 7e77a6ee..0b82b1c0 100644 --- a/src/rtl8733b/Halmac8733bMac.h +++ b/src/rtl8733b/Halmac8733bMac.h @@ -112,6 +112,11 @@ class Halmac8733bMac { bool read_efuse(EfuseInfo &out); bool initialize(const EfuseInfo &efuse); bool configure_monitor_rx(bool keep_corrupted); + /* REG_ACKTO (0x0640) only, in microseconds. The caller owns the + * 1..255 clamp DeviceConfig::tx::ack_timeout_us documents; this is the + * register plane, and the CCK companion 0x0639 is deliberately left at + * its vendor value, matching the register the Jaguar backends touch. */ + uint8_t set_ack_timeout_us(uint8_t microseconds); void stop(); MacState read_mac_state(); diff --git a/src/rtl8733b/Rtl8733bDevice.cpp b/src/rtl8733b/Rtl8733bDevice.cpp index c62c13c0..131f34aa 100644 --- a/src/rtl8733b/Rtl8733bDevice.cpp +++ b/src/rtl8733b/Rtl8733bDevice.cpp @@ -7,6 +7,7 @@ #include #include +#include "AckResponder.h" /* hardware ACK/BlockAck responder recipe */ #include "RateDefinitions.h" #include "RadiotapPeek.h" /* send_packets batch pre-parse */ #include "RadiotapTxFlags.h" @@ -76,6 +77,40 @@ void Rtl8733bDevice::bring_up_to_phy() { _phy_ready = true; _logger->info("RTL8733B factory path reached PHY-ready (cut {})", _chip.cut); + + /* ACK window (DEVOURER_ACK_TIMEOUT_US): the one library default every + * generation programs identically, replacing the vendor value init_wmac just + * wrote (0x21 = 33 us — the bottom of the 33..128 per-chip spread the single + * default exists to abolish). Same clamp and same register as jaguar1/2/3; + * see the DeviceConfig field doc for the range budget it buys. + * + * This was silently ignored until issue #2, which is the failure this + * backend refuses everywhere else (disable_cca, SetTxPowerIndexOverride): a + * config value that reads as applied while the radio runs something else. + * The window is load-bearing here now — both ends of a hardware-ARQ link are + * measured working on this die (SetAckResponder, tx.retry_limit). + * + * Sited in bring_up_to_phy for the same reason the disable_cca warning below + * is: it is where the MAC bring-up that wrote the vendor value has just run, + * once per bring-up, on both the RX and TX session shapes. */ + const uint8_t ackto_want = static_cast( + _cfg.tx.ack_timeout_us > 255 ? 255 + : _cfg.tx.ack_timeout_us < 1 ? 1 + : _cfg.tx.ack_timeout_us); + const uint8_t ackto_got = _mac.set_ack_timeout_us(ackto_want); + if (ackto_got != ackto_want) + _logger->warn("RTL8733B: ACK window did not latch — REG_ACKTO reads {} us, " + "wanted {} us", + ackto_got, ackto_want); + else + _logger->info("RTL8733B: ACK window {} us (REG_ACKTO 0x640, verified)", + ackto_got); + /* DEVOURER_ACK_RESPONDER — opt-in only, never a default: it turns a passive + * monitor into an active SIFS-timed transmitter. Sited here with the other + * bring-up knobs so an RX-only session (Init) arms it too, which is the + * session shape a pure responder actually runs. */ + if (_cfg.rx.ack_responder) + SetAckResponder(*_cfg.rx.ack_responder); /* Every other generation applies tuning.disable_cca during bring-up * (jaguar1/2/3, kestrel all call SetCcaMode there), so a caller setting * DEVOURER_DIS_CCA=1 reasonably expects it to take effect. This backend has @@ -96,6 +131,22 @@ void Rtl8733bDevice::bring_up_to_phy() { _logger->warn( "RTL8733B: DEVOURER_DIS_CCA / tuning.disable_cca is not implemented by " "this backend — carrier-sense stays ENABLED for this session"); + /* DEVOURER_TX_REPORT: the CCX per-frame TX-status path is not ported, and + * unlike the knobs above the reason is the firmware rather than a missing + * register — everything under this backend's control was verified correct on + * air and the chip still returned no C2H at all. The full bench narrative and + * the outstanding lead live in src/rtl8733b/CLAUDE.md "Hardware ARQ"; it is + * not repeated here. + * + * Warn rather than drop it silently: a consumer that sets this knob is + * asking for its per-frame delivery sensor, and would otherwise read the + * absence of tx.report events as a quiet link rather than an absent + * feature. */ + if (_cfg.tx.report) + _logger->warn( + "RTL8733B: DEVOURER_TX_REPORT / tx.report is not implemented by this " + "backend — the firmware emits no CCX reports, so no tx.report events " + "will arrive (see src/rtl8733b/CLAUDE.md)"); } void Rtl8733bDevice::Init(Action_ParsedRadioPacket packetProcessor, @@ -755,6 +806,81 @@ size_t Rtl8733bDevice::build_tx_block(const uint8_t *packet, size_t length, return frame_offset + frame_len; } +bool Rtl8733bDevice::SetAckResponder(const devourer::MacAddr &mac) { + std::lock_guard lock(_reg_mu); + if (!_mac_ready) { + _logger->error("RTL8733B: ACK responder requires MAC bring-up first"); + return false; + } + /* The AckResponder.h recipe is map-identical on this die, which is not an + * assumption: the vendor 8733BU tree's port-0 descriptor names exactly these + * three registers — net_type = REG_CR_8733B + 2 (0x0100 + 2 = 0x0102, shift + * 0), macaddr = REG_MACID_8733B (0x0610), bssid = REG_BSSID_8733B (0x0618) + * — the same addresses and the same MSR field the Jaguar backends use + * (hal/rtl8733b/rtl8733b_ops.c port_cfg[0], hal/halmac/halmac_reg_8733b.h). + * + * MAC bring-up leaves net_type at 0 (No Link) because init_mac writes only + * REG_CR's low half (0x0100-0x0101) — which is exactly why a monitor radio + * on this die never ACKs. Flipping the field is the whole gate. */ + if ((mac.bytes[0] & 0x01u) != 0) { + /* A station cannot ACK-target a group address. Refuse rather than arm a + * responder that can never fire — the same footgun AckResponder.h records + * from the AP-mode work. */ + _logger->error("RTL8733B: ACK responder needs a UNICAST MAC (I/G set in " + "{:02x}) — not armed", + mac.bytes[0]); + return false; + } + devourer::ack::enable(_device, mac.data()); + /* Latch BEFORE the readback, not after it. The writes have already been + * issued at this point, so the hardware may be armed whatever the readback + * says; if the flag tracked the readback verdict instead, a mismatch would + * leave Stop() believing there was nothing to disarm while the chip went on + * auto-ACKing — the precise failure this flag exists to prevent. A redundant + * disarm costs one USB round-trip at teardown; a missed one leaves an + * unowned transmitter on the air. */ + _ack_armed = true; + /* Read back before claiming it. This is the first port of the responder onto + * HALMAC 87xx, and this backend does not report a write it cannot verify + * (the same standard SetCcaMode and SetTxPowerOffsetQdb are held to). */ + const uint8_t net_type = static_cast(_device.rtw_read8(0x0102) & 0x03u); + const uint32_t id_lo = _device.rtw_read(0x0610); + const uint16_t id_hi = _device.rtw_read16(0x0614); + const uint32_t want_lo = static_cast(mac.bytes[0]) | + (static_cast(mac.bytes[1]) << 8) | + (static_cast(mac.bytes[2]) << 16) | + (static_cast(mac.bytes[3]) << 24); + const uint16_t want_hi = + static_cast(mac.bytes[4] | (mac.bytes[5] << 8)); + if (net_type != 0x03 || id_lo != want_lo || id_hi != want_hi) { + _logger->error("RTL8733B: ACK responder did not latch — net_type={} " + "macid=0x{:04x}{:08x} (wanted 3 / 0x{:04x}{:08x})", + net_type, id_hi, id_lo, want_hi, want_lo); + /* Best-effort undo, then leave _ack_armed set regardless: if this disable + * also failed we must not tell teardown the radio is quiet. */ + try { + devourer::ack::disable(_device); + } catch (const std::exception &e) { + _logger->warn("RTL8733B: ACK responder rollback failed: {}", e.what()); + } + return false; + } + _logger->info("RTL8733B: hardware ACK responder armed for " + "{:02x}:{:02x}:{:02x}:{:02x}:{:02x}:{:02x} (net_type=AP)", + mac.bytes[0], mac.bytes[1], mac.bytes[2], mac.bytes[3], + mac.bytes[4], mac.bytes[5]); + return true; +} + +void Rtl8733bDevice::ClearAckResponder() { + std::lock_guard lock(_reg_mu); + if (!_mac_ready) + return; + devourer::ack::disable(_device); + _ack_armed = false; + _logger->info("RTL8733B: hardware ACK responder disarmed (net_type=NoLink)"); +} + void Rtl8733bDevice::SetCcaMode(bool disabled) { if (!disabled) { /* `false` is the universal default — carrier-sense + EDCCA enabled — and @@ -798,6 +924,27 @@ void Rtl8733bDevice::Stop() { } _phy_ready = false; if (_mac_ready) { + /* Disarm the ACK responder explicitly. _mac.stop() clears REG_CR's low + * half (0x0100-0x0101) but net_type lives at 0x0102 and would survive it, + * so a session that ends with teardown_power_down off would leave the chip + * auto-ACKing — an active SIFS-timed transmitter with no session owning + * it. The knob is opt-in precisely because it makes the radio transmit; + * it must not outlive the session that asked for it. + * + * Guarded like the TSSI rollback above and for the same reason: this is a + * read-modify-write, register reads throw on a disconnected device + * (UsbTransport rtw_read), and letting that escape here would abandon the + * REST of teardown — _mac.stop() and the card-disable sequence — which is + * worse than losing the disarm on a device that has already gone away. */ + if (_ack_armed) { + try { + devourer::ack::disable(_device); + } catch (const std::exception &e) { + _logger->warn("RTL8733B: ACK responder disarm failed during shutdown: " + "{}", e.what()); + } + _ack_armed = false; + } _mac.stop(); _mac_ready = false; } @@ -851,6 +998,14 @@ devourer::AdapterCaps Rtl8733bDevice::GetAdapterCaps() { * 10 MHz is SDR- and cross-decode-qualified on both bands * (docs/rtl8733b.md "Narrowband status"). */ caps.narrowband_ok = true; + /* Hardware ARQ (truth table at the AdapterCaps declarations): both knobs + * measured true on this die. The responder had to be proven before the TX + * side could be, because this die has no CCX path to judge its own frames — + * so the retry knob is witnessed from the air instead of from tx.report + * (tests/rtl8733b_retry_limit_onair.sh; bench narrative in + * src/rtl8733b/CLAUDE.md "Hardware ARQ"). */ + caps.ack_responder_ok = true; + caps.tx_retry_limit_ok = true; caps.txpwr = GetTxPowerCaps(); return caps; } diff --git a/src/rtl8733b/Rtl8733bDevice.h b/src/rtl8733b/Rtl8733bDevice.h index aae5a843..f8fa676a 100644 --- a/src/rtl8733b/Rtl8733bDevice.h +++ b/src/rtl8733b/Rtl8733bDevice.h @@ -56,6 +56,8 @@ class Rtl8733bDevice : public IRtlDevice { void SetTxMode(const devourer::TxMode &mode) override; void ClearTxMode() override; SelectedChannel GetSelectedChannel() override; + bool SetAckResponder(const devourer::MacAddr &mac) override; + void ClearAckResponder() override; void SetCcaMode(bool disabled) override; void Stop() override; @@ -122,6 +124,13 @@ class Rtl8733bDevice : public IRtlDevice { std::atomic _rx_active{false}; std::atomic _rx_configured_bw{0}; std::atomic _tx_submits{0}; + /* Whether the hardware ACK responder is currently armed. Teardown has + * to disarm it (net_type at 0x0102 survives _mac.stop(), which clears + * only REG_CR's low half), but only when it was actually armed: the + * disarm is a read-modify-write, and spending a USB round-trip on every + * teardown to clear bits that are already zero is a cost the common + * path should not pay. Guarded by _reg_mu like the other state. */ + bool _ack_armed = false; mutable std::recursive_mutex _reg_mu; std::optional _tx_mode_default; }; diff --git a/tests/rtl8733b_retry_limit_onair.sh b/tests/rtl8733b_retry_limit_onair.sh new file mode 100755 index 00000000..ef3a308d --- /dev/null +++ b/tests/rtl8733b_retry_limit_onair.sh @@ -0,0 +1,123 @@ +#!/usr/bin/env bash +# rtl8733b_retry_limit_onair.sh — is DeviceConfig::tx::retry_limit a LIVE +# actuator on the RTL8733B, or just an encoded descriptor field? (issue #2) +# +# The 8733B has no CCX / tx.report path (the firmware emits no C2H reports — +# src/rtl8733b/CLAUDE.md), so the TX side cannot be its own witness the way +# tests/ack_txreport_matrix.sh judges the Jaguars. This bench judges from the +# AIR instead: the DUT sends unicast QoS-Data to a MAC that nobody owns, so no +# ACK ever comes back and the MAC must exhaust its descriptor retry limit on +# every frame. A passive monitor counts how many times each submitted frame +# actually aired. +# +# retry_limit = N -> airings/frame ~= 1 + N (1 original + N retries) +# +# That is a DOSE-RESPONSE, not an A/B: a single on/off pair could be explained +# by ambient conditions, three or more levels on a straight line cannot. +# +# COUNTING NOTE: rxdemo emits rx.txhit sampled (first 10, then every 100th) — +# counting EVENTS undercounts by 100x. The event's own `hits` field is the +# cumulative truth, so we read the LAST hits value. That quantizes the total to +# the largest multiple of 100 <= H, i.e. H is understated by up to 99 airings +# (worst case ~6% on the retry=0 arm). Each arm therefore gets a FRESH witness +# so `hits` starts at zero — the counter is static for the process lifetime and +# does not reset between arms. +# +# sudo bash tests/rtl8733b_retry_limit_onair.sh +# ARMS="0 3 12" FRAMES=3000 CH=36 sudo bash tests/rtl8733b_retry_limit_onair.sh +set -u +ROOT="$(cd "$(dirname "$0")/.." && pwd)" +BUILD=${BUILD:-$ROOT/build} + +DUT_VID=${DUT_VID:-0x0bda}; DUT_PID=${DUT_PID:-0xf72b} # RTL8731BU/8733BU +WIT_VID=${WIT_VID:-0x0bda}; WIT_PID=${WIT_PID:-0x8812} # RTL8812AU witness +CH=${CH:-6} +ARMS=${ARMS:-"0 3 12 0 12"} +FRAMES=${FRAMES:-1500} +RATE=${RATE:-MCS0} +GAP_US=${GAP_US:-3000} +PWR_QDB=${PWR_QDB:-12} +# RA must be UNICAST (so an ACK is expected) and unowned (so none ever comes). +RA=${RA:-02:de:ad:be:ef:01} +OUT=${OUT:-/tmp/rtl8733b_retry} + +KILL(){ sudo pkill -9 -x rxdemo 2>/dev/null; sudo pkill -9 -x txdemo 2>/dev/null; return 0; } +trap KILL EXIT +mkdir -p "$OUT"; RESULTS="$OUT/results.jsonl"; : >"$RESULTS" + +idx=0 +for arm in $ARMS; do + idx=$((idx+1)) + # Per-ARM-INDEX filenames, not per-retry-value: a ladder repeats values + # (0/3/12/0/12) and reusing the value as the name lets a slow-dying witness + # from the earlier arm append into the next one's log. + tag="$(printf '%02d_r%s' "$idx" "$arm")" + KILL; sleep 3 # USB release after a -9 is not instantaneous + sudo env DEVOURER_VID=$WIT_VID DEVOURER_PID=$WIT_PID DEVOURER_CHANNEL=$CH \ + DEVOURER_LOG_LEVEL=info \ + "$BUILD/rxdemo" >"$OUT/wit_$tag.jsonl" 2>"$OUT/wit_$tag.err" & + waited=0 + until grep -qE "async ring of .* URBs submitted|Listening air" "$OUT/wit_$tag.err"; do + sleep 1; waited=$((waited+1)) + if [ "$waited" -ge 25 ]; then + echo "ABORT: witness never reached RX for arm=$arm (#$idx)" >&2 + tail -5 "$OUT/wit_$tag.err" >&2; exit 1 + fi + done + sleep 2 + sudo env DEVOURER_VID=$DUT_VID DEVOURER_PID=$DUT_PID DEVOURER_CHANNEL=$CH \ + DEVOURER_TX_QOS_DATA=1 DEVOURER_TX_RA=$RA \ + DEVOURER_TX_RATE=$RATE DEVOURER_TX_PAYLOAD_BYTES=200 \ + DEVOURER_TX_GAP_US=$GAP_US DEVOURER_TX_FRAMES=$FRAMES \ + DEVOURER_TX_RETRY_LIMIT=$arm DEVOURER_TX_PWR_OFFSET_QDB=$PWR_QDB \ + DEVOURER_LOG_LEVEL=warn \ + timeout -s INT 90 "$BUILD/txdemo" >"$OUT/tx_$tag.jsonl" 2>"$OUT/tx_$tag.err" || true + sleep 3 + sent=$(grep '"ev":"tx.stats"' "$OUT/tx_$tag.jsonl" | tail -1 | + sed -n 's/.*"submitted":\([0-9]*\).*/\1/p'); sent=${sent:-0} + hits=$(grep -o '"ev":"rx.txhit","hits":[0-9]*' "$OUT/wit_$tag.jsonl" | tail -1 | + sed -n 's/.*"hits":\([0-9]*\).*/\1/p'); hits=${hits:-0} + KILL + # A cell that did not run is NOT a measurement of zero. An arm whose DUT + # never opened (sent=0) or whose witness heard nothing at all (hits=0) is a + # harness failure, and reporting it as 0.0 airings/frame would read exactly + # like a dead retry engine — the same false-verdict shape that made an + # 8821AU look broken in tests/ack_txreport_matrix.sh. Abort instead. + if [ "$sent" -eq 0 ] || [ "$hits" -eq 0 ]; then + echo "ABORT: arm=$arm (#$idx) did not run — submitted=$sent airings=$hits" >&2 + echo " (DUT or witness failed to open; this is not a zero result)" >&2 + tail -5 "$OUT/tx_$tag.err" >&2 + exit 1 + fi + if [ "$sent" -ne "$FRAMES" ]; then + echo "WARN: arm=$arm (#$idx) submitted $sent of $FRAMES requested" >&2 + fi + python3 - "$arm" "$sent" "$hits" >>"$RESULTS" <<'PY' +import json, sys +arm, sent, hits = int(sys.argv[1]), int(sys.argv[2]), int(sys.argv[3]) +per = round(hits / sent, 2) if sent else 0.0 +print(json.dumps({"ev": "retry.arm", "retry_limit": arm, "submitted": sent, + "airings": hits, "airings_per_frame": per, + "expected": 1 + arm})) +PY + tail -1 "$RESULTS" +done + +echo "==== VERDICT ====" +python3 - "$RESULTS" <<'PY' +import json, sys +rows = [json.loads(l) for l in open(sys.argv[1])] +ok = True +for r in rows: + exp, got = r["expected"], r["airings_per_frame"] + # Generous band: airings can only be LOST (a monitor misses frames), never + # invented, so the floor is what matters. 0.6*expected still separates + # every adjacent level in a 0/3/12 ladder. + good = 0.6 * exp <= got <= 1.15 * exp + ok &= good + print(f" retry={r['retry_limit']:>2} expected~{exp:>2} measured={got:>5}" + f" {'OK' if good else 'FAIL'}") +print(json.dumps({"ev": "retry.verdict", "tx_retry_limit_ok": bool(ok), + "arms": len(rows)})) +sys.exit(0 if ok else 1) +PY diff --git a/tests/teardown_gen_sanity.sh b/tests/teardown_gen_sanity.sh index 72fef71a..054af842 100755 --- a/tests/teardown_gen_sanity.sh +++ b/tests/teardown_gen_sanity.sh @@ -24,6 +24,8 @@ DUTS="${DUTS:-J1-8821AU:0x2357:0x0120,\ J2-8822BU:0x2357:0x012d,\ J3-8812CU:0x0bda:0xc812,\ J3-8812EU:0x0bda:0xa81a,\ +RTL8733B:0x0bda:0xf72b,\ +RTL8733B-combo:0x0bda:0xb733,\ KESTREL:0x35bc:0x0101}" mkdir -p "$OUT"; rm -f "$OUT"/*.log "$OUT"/asan.* 2>/dev/null || true From 2f90e9ee7166bfd2520a48fea51ec8e5d2e9ca74 Mon Sep 17 00:00:00 2001 From: snokvist Date: Sun, 30 Aug 2026 20:35:09 +0200 Subject: [PATCH 2/3] =?UTF-8?q?rtl8733b:=20address=20Qodo=20review=20?= =?UTF-8?q?=E2=80=94=20arm-latch=20ordering,=20doc=20scope,=20header=20dup?= =?UTF-8?q?lication?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - **Partial arm could escape teardown** (Qodo #4, real bug). `_ack_armed` was set after `ack::enable()` returned. That call is five register writes ending with the net_type field that actually arms the engine, and any of them can throw on a stalling device — including after the arming write reached hardware. The exception unwinds into Init()/InitWrite()'s catch, which calls Stop(); with the flag still false that teardown skipped the disarm, leaving an unowned radio auto-ACKing when teardown_power_down is off. Latch before the first write instead: a redundant disarm costs one USB round-trip, a missed one costs an unowned transmitter. - **Root CLAUDE.md carried subtree facts** (Qodo #1, #3). The root file is cross-cutting only, and the added parenthetical also stated a favourable measured result with its counterpart a link away rather than in the same breath. Dropped — the not-ported list is updated, and the ARQ narrative stays in src/rtl8733b/CLAUDE.md where its counterparts already sit. - **ARQ prose restated the ack_timeout_us contract** (Qodo #2). Range, clamp, default and register are doc-commented at the DeviceConfig declaration; the subtree file now points there and keeps only what is specific to this backend (the vendor 0x21 that init_wmac still writes and why, the readback, the measured values). Re-verified on hardware after the change: responder 1349/1349 ACKed at retries_mean 0.00, retarget 1348/1349, disarmed control 0.00 with retries pinned at 12; teardown disarm still holds with power-down disabled. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VjcZRPjNtsy2mxfDkAWNpd --- CLAUDE.md | 3 +-- src/rtl8733b/CLAUDE.md | 13 ++++++++----- src/rtl8733b/Rtl8733bDevice.cpp | 19 +++++++++++-------- 3 files changed, 20 insertions(+), 15 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index f594843e..ca6a63c1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -61,8 +61,7 @@ construction from the `SYS_CFG2` chip-id (Kestrel: PID-first): 10 MHz narrowband (5 MHz refused — `src/rtl8733b/CLAUDE.md`). Everything the backend has not ported (TSF/beacons, A-MPDU, CCX/`tx.report`, the flat-index and per-rate TX-power knobs) falls through to `IRtlDevice`'s - not-ported defaults rather than being faked (hardware ACK/BlockAck IS ported - and measured, as is `tx.retry_limit` — `src/rtl8733b/CLAUDE.md`), so read the base class before + not-ported defaults rather than being faked, so read the base class before assuming a cross-generation feature below applies here. `FastRetune` IS ported (intra-band, TSSI kept live — `src/rtl8733b/CLAUDE.md`). SGI, LDPC, STBC, VHT and HE are refused outright. Scope, one-unit validation record and the diff --git a/src/rtl8733b/CLAUDE.md b/src/rtl8733b/CLAUDE.md index ccd0c5e3..f87fdf9f 100644 --- a/src/rtl8733b/CLAUDE.md +++ b/src/rtl8733b/CLAUDE.md @@ -292,11 +292,14 @@ particular is witnessed by a monitor that can only ever LOSE airings, never invent them, so the measured ratio is a floor rather than a point estimate. `DeviceConfig::tx::ack_timeout_us` **is** honoured, and was not until that -issue: `bring_up_to_phy` overwrites REG_ACKTO (0x0640) from the config with the -same 1..255 clamp the Jaguars use, right after MAC bring-up wrote the vendor -0x21 — 33 us, the bottom of the 33..128 per-chip spread that field's one -default exists to abolish. The write is read back and logged, because a value -reported without a readback is the same shape of claim the bug was; measured +issue. The field's contract — range, clamp, default, register and the range +budget it buys — is doc-commented at its declaration in `src/DeviceConfig.h` +and is not restated here. What is specific to this backend: `init_wmac()` still +writes the vendor `0x21`, so `bring_up_to_phy` overwrites it from the config +afterwards (the vendor write stays because that MAC plane is shared verbatim +with `rtl8733bprobe`, which carries no `DeviceConfig`), and the write is read +back and logged rather than assumed — a value reported without a readback is +the same shape of claim the original bug was. Measured `/128/33/200 -> 128/128/33/200`. The CCK companion 0x0639 keeps its vendor value, as on every other generation. diff --git a/src/rtl8733b/Rtl8733bDevice.cpp b/src/rtl8733b/Rtl8733bDevice.cpp index 131f34aa..1b270d43 100644 --- a/src/rtl8733b/Rtl8733bDevice.cpp +++ b/src/rtl8733b/Rtl8733bDevice.cpp @@ -831,15 +831,18 @@ bool Rtl8733bDevice::SetAckResponder(const devourer::MacAddr &mac) { mac.bytes[0]); return false; } - devourer::ack::enable(_device, mac.data()); - /* Latch BEFORE the readback, not after it. The writes have already been - * issued at this point, so the hardware may be armed whatever the readback - * says; if the flag tracked the readback verdict instead, a mismatch would - * leave Stop() believing there was nothing to disarm while the chip went on - * auto-ACKing — the precise failure this flag exists to prevent. A redundant - * disarm costs one USB round-trip at teardown; a missed one leaves an - * unowned transmitter on the air. */ + /* Latch BEFORE the first write, not after the call and not after the + * readback. ack::enable() is five register writes ending with the net_type + * field that actually arms the engine, and any of them can throw + * (UsbTransport rtw_read/rtw_write on a stalling device) — including after + * the arming write has already reached hardware. An exception here unwinds + * through bring_up_to_phy into Init()/InitWrite()'s catch, which calls + * Stop(); if the flag were set only on the success path, that teardown would + * believe there was nothing to disarm while the chip went on auto-ACKing. + * Same reasoning for a failed readback below. A redundant disarm costs one + * USB round-trip; a missed one leaves an unowned transmitter on the air. */ _ack_armed = true; + devourer::ack::enable(_device, mac.data()); /* Read back before claiming it. This is the first port of the responder onto * HALMAC 87xx, and this backend does not report a write it cannot verify * (the same standard SetCcaMode and SetTxPowerOffsetQdb are held to). */ From 466bf5e111609c430cfcc590307864a1b3f0e2dd Mon Sep 17 00:00:00 2001 From: snokvist Date: Mon, 31 Aug 2026 11:20:13 +0200 Subject: [PATCH 3/3] =?UTF-8?q?rtl8733b:=20address=20review=20=E2=80=94=20?= =?UTF-8?q?fail=20a=20refused=20arm,=20share=20the=20ACK=20recipe,=20scope?= =?UTF-8?q?=20the=20claims?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Runtime - A config-requested arm no longer discards its result. `DEVOURER_ACK_RESPONDER` with a group MAC (easy to reach: the canonical TX SA 57:42:.. has the I/G bit set) previously gave a green init and a session running as a plain monitor, so the operator debugged the RF link instead of the config. It now fails the bring-up. - The disarm moves into `Halmac8733bMac::stop()`, unconditional. `stop()` clears only REG_CR's low half, so net_type at 0x0102 survives it; siting the clear there means no path can reach `stop()` and leave an unowned SIFS-timed transmitter airing. This deletes the `_ack_armed` flag, its latch-ordering subtlety and the flag-guarded special case at the device layer. - `ack::is_unicast()` and `ack::verify()` join `enable()`/`disable()`, so the register map lives in one file instead of being re-hardcoded by a backend that can drift from it. Jaguar1/2/3 adopt the unicast guard — they returned `true` for a group MAC, the "silently dead responder" shape AdapterCaps.h records from the 8821AU episode. They do NOT adopt verify(): a family whose 0x0102 does not read back would start refusing healthy arms, and that wants a bench cell per die (J1 and J3 verified here, no J2 on this bench). Harness - `FRAMES` gets a floor. The rx.txhit readout quantizes to <=99 airings whatever the frame count, so `FRAMES=60` scored a healthy retry=0 arm at 0.17 against a 0.6 band — a false FAIL, the same shape the script already refuses for an arm that did not run. Docs - The ACK claim scopes to what ran: normal-ACK response to unicast singles. BlockAck response was never measured; `tests/ampdu_ba_check.sh` against this die came back indeterminate (armed and disarmed both 0% delivered at retries 0 — the control did not separate), which is the documented consequence of CCX accounting not surviving AGG_EN, not a verdict on the chip. - The quantization attribution was arithmetically wrong for the retry=12 arm: the <=99-airing bound is <=0.066/frame and covers the 0 and 3 arms, but 12.27 against 13 is 0.73/frame (~1095 airings), an order of magnitude past it. That residue is monitor loss — which is why the ratio is a floor. - `docs/scheduled-mac.md`: the 8733B's single-shot cell is marked unmeasured with the closed-loop figure parenthetical, since that column's neighbours are single-shot rates and 100%-at-retry-12 is not comparable to them; and the "J1/2/3-only" line one row below is corrected. - `src/DeviceConfig.h`, the authoritative per-knob reference, now names the 8733B for both `retry_limit` and `ack_timeout_us`/REG_ACKTO. - Changelog framing removed from the new comments and docs; the rationale halves stand on their own. Verified: builds + ctest green on default / 8733B-only / 8733B-off / ASan+UBSan. On hardware, the new Jaguar guard refuses a group MAC and still arms a unicast one on both 8812AU and 8812CU, and the pre-existing responder path is unregressed (8812AU responder, 8812CU soliciting: on 1.00, retarget 1.00, off 0.00 with retries pinned at 12). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VjcZRPjNtsy2mxfDkAWNpd --- docs/rtl8733b.md | 20 ++++-- docs/scheduled-mac.md | 4 +- src/AckResponder.h | 23 +++++++ src/DeviceConfig.h | 8 ++- src/jaguar1/RtlJaguarDevice.cpp | 12 ++++ src/jaguar2/RtlJaguar2Device.cpp | 12 ++++ src/jaguar3/RtlJaguar3Device.cpp | 12 ++++ src/rtl8733b/CLAUDE.md | 68 +++++++++++-------- src/rtl8733b/Halmac8733bMac.cpp | 28 +++++--- src/rtl8733b/Rtl8733bDevice.cpp | 100 +++++++++------------------- src/rtl8733b/Rtl8733bDevice.h | 7 -- tests/rtl8733b_retry_limit_onair.sh | 20 +++++- 12 files changed, 187 insertions(+), 127 deletions(-) diff --git a/docs/rtl8733b.md b/docs/rtl8733b.md index 719d34e4..4ad30a35 100644 --- a/docs/rtl8733b.md +++ b/docs/rtl8733b.md @@ -372,13 +372,13 @@ a request a caller may be making only through an inherited environment. ## Hardware ARQ -Opened by issue #2; measured against an RTL8812AU peer on the validation unit. +Measured against an RTL8812AU peer on the validation unit. Both directions of a reliable-unicast link now work on this die. | knob | state | evidence | | --- | --- | --- | -| `tx.ack_timeout_us` | honoured (was silently pinned at 33 us) | REG_ACKTO read back at bring-up: `/128/33/200 -> 128/128/33/200` | -| `SetAckResponder` | ported, `ack_responder_ok = true` | 8733B as responder: armed 1736/1736 ACKed, retries_mean 0.00; re-armed on a different MAC 1736/1736; disarmed 0.00 with retries pinned at 12 | +| `tx.ack_timeout_us` | honoured | REG_ACKTO read back at bring-up: `/128/33/200 -> 128/128/33/200` | +| `SetAckResponder` | ported, `ack_responder_ok = true` (normal-ACK singles; BlockAck untested) | 8733B as responder: armed 1736/1736 ACKed, retries_mean 0.00; re-armed on a different MAC 1736/1736; disarmed 0.00 with retries pinned at 12 | | `tx.retry_limit` | live, `tx_retry_limit_ok = true` | airtime dose-response 0/3/12 -> 0.93/3.93/12.27 airings per frame (expected 1 + N), repeatable over a 0/3/12/0/12 ladder | | `tx.report` (CCX) | **not** ported — firmware emits nothing | see Known gaps below | @@ -394,8 +394,13 @@ single delta that ambient conditions could fake — carries the claim. Its counterparts: one physical unit and one peer generation, like every other on-air claim in this section — note the document as a whole rests on two units, but these ARQ cells ran on the `f72b` sample only; the monitor can only lose -airings, never invent them, so the measured ratio is a floor; and the ~0.07/frame shortfall is the `rx.txhit` -sampling quantization the harness documents rather than loss. +airings, never invent them, so the measured ratio is a floor. Two different things account for the shortfalls, and only the smaller one is an +artefact: the `rx.txhit` readout quantizes to at most 99 airings (the event +fires on the first 10 hits then every 100th), which is <=0.066/frame at 1500 +frames and covers the 0 and 3 arms entirely. It does NOT cover the 12 arm — +12.27 against 13 is 0.73/frame, about 1095 airings, an order of magnitude past +that bound. That residue is monitor loss (or genuinely fewer airings), which is +why the ratio is reported as a floor and not a point estimate. ## Known gaps and deferred validation @@ -432,8 +437,9 @@ These results have **not** been claimed: successfully, but an independent RTL8812AU decoded both broadcast and unicast probes as long GI. STBC and LDPC remain rejected. A-MPDU and throughput were not independently validated and remain unadvertised or - unsupported by this backend. (ACK/BlockAck response is no longer in this - list — it is ported and measured, see Hardware ARQ below.) + unsupported by this backend. Normal-ACK response IS ported and measured + (Hardware ARQ above); BlockAck response to an aggregate remains untested, as + does A-MPDU itself. - No CCX path, so no `tx.report` events — and unlike every other gap here the cause is the firmware, not a missing port. Everything under the backend's control was verified correct on air (descriptor SPE_RPT/SW_DEFINE set at the diff --git a/docs/scheduled-mac.md b/docs/scheduled-mac.md index 286894f2..2dcf6030 100644 --- a/docs/scheduled-mac.md +++ b/docs/scheduled-mac.md @@ -258,8 +258,8 @@ nonzero limit for absolute numbers; 8821AU row re-measured ch6): | 8821AU | 62% | 0% | works (94% closed-loop at retry 8) | | 8812EU | 98% | 0% | works | | 8812CU | 69% | 0% | works | -| 8733B | 100% (retry 12) | 0% | works (1736/1736, retarget- and disarm-proof) | -| 8852CU (Kestrel) | 0% | 0% | not implemented (SetAckResponder is J1/2/3-only) | +| 8733B | unmeasured | 0% | works (closed-loop 1736/1736 at retry 12; single-shot cell never run) | +| 8852CU (Kestrel) | 0% | 0% | not implemented on the AX generation | Unmeasured for lack of plugged hardware: 8821CU / PCIe 8821CE (recipe-shared with the 8822B; their `AdapterCaps.ack_responder_ok` stays false-as-unmeasured diff --git a/src/AckResponder.h b/src/AckResponder.h index d8941200..8d2c35ba 100644 --- a/src/AckResponder.h +++ b/src/AckResponder.h @@ -59,5 +59,28 @@ inline void disable(RtlAdapter &dev) { dev.rtw_write8(0x0102, static_cast(nt & ~0x03u)); } +/* The MAC must be UNICAST: a station cannot ACK-target a group address, so an + * arm on one can never fire. Lives here rather than in each backend because + * the precondition is a property of the recipe, not of any one die. */ +inline bool is_unicast(const uint8_t mac[6]) { return (mac[0] & 0x01u) == 0; } + +/* Did the arm actually land? Reads back the gate (net_type) and the RA the ACK + * engine matches (MACID), composed exactly as enable() writes them — keeping + * the register map in ONE file, so a change to enable() cannot silently + * diverge from a copy of it somewhere else. + * + * NB the BSSID companion at 0x0618 is written but not checked: the ACK engine + * matches on MACID, and 0x0618 is programmed only because the proven AP recipe + * programs both. Verifying the two fields that gate the behaviour keeps this + * honest without asserting on one that does not. */ +inline bool verify(RtlAdapter &dev, const uint8_t mac[6]) { + const uint32_t want_lo = (uint32_t)mac[0] | ((uint32_t)mac[1] << 8) | + ((uint32_t)mac[2] << 16) | ((uint32_t)mac[3] << 24); + const uint16_t want_hi = (uint16_t)(mac[4] | (mac[5] << 8)); + return (dev.rtw_read8(0x0102) & 0x03u) == 0x03u && + dev.rtw_read(0x0610) == want_lo && + dev.rtw_read16(0x0614) == want_hi; +} + } /* namespace ack */ } /* namespace devourer */ diff --git a/src/DeviceConfig.h b/src/DeviceConfig.h index 8e04f793..e517d7be 100644 --- a/src/DeviceConfig.h +++ b/src/DeviceConfig.h @@ -216,7 +216,8 @@ struct DeviceConfig { /* env: DEVOURER_TX_RETRY_LIMIT — per-frame hardware retry limit (0..63; * Kestrel ceiling 62 — its attempts-counting WD field folds +1). Maps to * the TX descriptor DATA_RETRY_LIMIT / RTS_DATA_RTY_LMT field on the - * 11ac generations and wd_info DATA_TXCNT_LMT on Kestrel. 0 = no retries + * 11ac generations and the RTL8733B, and wd_info DATA_TXCNT_LMT on + * Kestrel. 0 = no retries * (WFB default: FEC provides reliability, not MAC retries). On a busy * half-duplex link retries flood the air and blind the receiver. * Hardware-ARQ (SetAckResponder + unicast TA, docs/scheduled-mac.md) @@ -234,8 +235,9 @@ struct DeviceConfig { * per-bandwidth vendor value is 117 µs), so it also replaces the * per-chip / per-bandwidth vendor defaults (which ranged 33..128 µs * and made hardware-ARQ range silently die-dependent). The register: - * REG_ACKTO 0x640 on the 11ac generations, R_AX_RSP_CHK_SIG 0xCC00 - * byte0 on Kestrel; the CTS window (REG_CTS2TO 0x641) is separate and + * REG_ACKTO 0x640 on the 11ac generations and the RTL8733B (which + * overwrites its HALMAC vendor default at bring-up), R_AX_RSP_CHK_SIG + * 0xCC00 byte0 on Kestrel; the CTS window (REG_CTS2TO 0x641) is separate and * untouched. Sizing: ~6.7 µs x round-trip km + ~50 µs ACK flight and * detection margin; a longer window is NOT free — every retry of a * LOST frame waits the full window, measured (dead RA, retry 8, max diff --git a/src/jaguar1/RtlJaguarDevice.cpp b/src/jaguar1/RtlJaguarDevice.cpp index 7494b3ec..dc69850e 100644 --- a/src/jaguar1/RtlJaguarDevice.cpp +++ b/src/jaguar1/RtlJaguarDevice.cpp @@ -754,6 +754,18 @@ bool RtlJaguarDevice::send_packet(const uint8_t *packet, size_t length) { } bool RtlJaguarDevice::SetAckResponder(const devourer::MacAddr &mac) { + if (!devourer::ack::is_unicast(mac.data())) { + /* A station cannot ACK-target a group address, so this arm could never + * fire. Refusing beats returning true for a responder that will read as + * silently dead — the shape AdapterCaps.h records from the 8821AU + * episode. Only the precondition is enforced here: adopting the shared + * readback verify() too wants a bench cell per die, since a family whose + * 0x0102 does not read back would start refusing healthy arms. */ + _logger->error("{}: ACK responder needs a UNICAST MAC (I/G set in " + "{:02x}) — not armed", + "Jaguar1", mac.bytes[0]); + return false; + } /* Hardware ACK responder (src/AckResponder.h) — same register recipe as * the HalMAC generations (0x610/0x618/0x102 are map-identical here). */ devourer::ack::enable(_device, mac.data()); diff --git a/src/jaguar2/RtlJaguar2Device.cpp b/src/jaguar2/RtlJaguar2Device.cpp index b7251aec..3922bcf3 100644 --- a/src/jaguar2/RtlJaguar2Device.cpp +++ b/src/jaguar2/RtlJaguar2Device.cpp @@ -365,6 +365,18 @@ void RtlJaguar2Device::apply_replay_wseq() { } bool RtlJaguar2Device::SetAckResponder(const devourer::MacAddr &mac) { + if (!devourer::ack::is_unicast(mac.data())) { + /* A station cannot ACK-target a group address, so this arm could never + * fire. Refusing beats returning true for a responder that will read as + * silently dead — the shape AdapterCaps.h records from the 8821AU + * episode. Only the precondition is enforced here: adopting the shared + * readback verify() too wants a bench cell per die, since a family whose + * 0x0102 does not read back would start refusing healthy arms. */ + _logger->error("{}: ACK responder needs a UNICAST MAC (I/G set in " + "{:02x}) — not armed", + "Jaguar2", mac.bytes[0]); + return false; + } /* Hardware ACK responder (src/AckResponder.h): port identity + net_type so * the MAC auto-ACKs unicast frames to `mac`. Same registers the proven * StartBeacon/AP path programs, minus the beacon machinery. */ diff --git a/src/jaguar3/RtlJaguar3Device.cpp b/src/jaguar3/RtlJaguar3Device.cpp index e9c92eb4..93073ad6 100644 --- a/src/jaguar3/RtlJaguar3Device.cpp +++ b/src/jaguar3/RtlJaguar3Device.cpp @@ -2150,6 +2150,18 @@ void RtlJaguar3Device::WriteTsf(uint64_t tsf) { } bool RtlJaguar3Device::SetAckResponder(const devourer::MacAddr &mac) { + if (!devourer::ack::is_unicast(mac.data())) { + /* A station cannot ACK-target a group address, so this arm could never + * fire. Refusing beats returning true for a responder that will read as + * silently dead — the shape AdapterCaps.h records from the 8821AU + * episode. Only the precondition is enforced here: adopting the shared + * readback verify() too wants a bench cell per die, since a family whose + * 0x0102 does not read back would start refusing healthy arms. */ + _logger->error("{}: ACK responder needs a UNICAST MAC (I/G set in " + "{:02x}) — not armed", + "Jaguar3", mac.bytes[0]); + return false; + } /* Hardware ACK responder (src/AckResponder.h): port identity + net_type so * the MAC auto-ACKs unicast frames to `mac`. Same registers the proven * StartBeacon/AP path programs, minus the beacon machinery. Serialized on diff --git a/src/rtl8733b/CLAUDE.md b/src/rtl8733b/CLAUDE.md index f87fdf9f..4dd5f0e0 100644 --- a/src/rtl8733b/CLAUDE.md +++ b/src/rtl8733b/CLAUDE.md @@ -278,9 +278,8 @@ knob is told too, and so it fires exactly once per bring-up. ## Hardware ARQ -All three knobs of the hardware-ARQ surface were opened by issue #2. Two are -now measured true on this die; the third is refused loudly for a reason that -took a bench to find. +Two of the three hardware-ARQ knobs are measured true on this die; the third is +refused loudly for a reason that took a bench to find. The counterparts for everything measured below, stated once here because the numbers that follow are uniformly favourable: **one physical unit and one peer @@ -291,15 +290,14 @@ cells, and no vendor-driver A/B exists to compare against. The retry number in particular is witnessed by a monitor that can only ever LOSE airings, never invent them, so the measured ratio is a floor rather than a point estimate. -`DeviceConfig::tx::ack_timeout_us` **is** honoured, and was not until that -issue. The field's contract — range, clamp, default, register and the range +`DeviceConfig::tx::ack_timeout_us` is honoured. The field's contract — range, clamp, default, register and the range budget it buys — is doc-commented at its declaration in `src/DeviceConfig.h` and is not restated here. What is specific to this backend: `init_wmac()` still writes the vendor `0x21`, so `bring_up_to_phy` overwrites it from the config afterwards (the vendor write stays because that MAC plane is shared verbatim with `rtl8733bprobe`, which carries no `DeviceConfig`), and the write is read -back and logged rather than assumed — a value reported without a readback is -the same shape of claim the original bug was. Measured +back and logged rather than assumed: a knob reported without a readback cannot +be told apart from one the radio never received. Measured `/128/33/200 -> 128/128/33/200`. The CCK companion 0x0639 keeps its vendor value, as on every other generation. @@ -308,22 +306,36 @@ applies unchanged — not an assumption, the vendor's own port-0 descriptor name these three registers (`hal/rtl8733b/rtl8733b_ops.c` `port_cfg[0]`: net_type `REG_CR_8733B + 2` = 0x0102 shift 0, macaddr 0x0610, bssid 0x0618). MAC bring-up leaves net_type at No Link because `init_mac` writes only REG_CR's -low half, which is exactly why a monitor radio here never ACKed. Teardown -disarms it explicitly, because `_mac.stop()` clears only that same low half and -net_type at 0x0102 would survive it: a session ending with -`teardown_power_down` off would otherwise leave the chip auto-ACKing with no -session owning it. Verified on air — after an armed session ends with -power-down disabled, the peer's reports read ack_rate 0.00 with retries pinned -at 12. The disarm is guarded like the TSSI rollback (register reads throw on a -disconnected device, and losing the rest of teardown is worse than losing the -disarm) and is skipped entirely when nothing was armed. The arm is -read back before it is claimed, and refuses a group MAC outright (a station -cannot ACK-target one — the I/G footgun, again). Measured against an RTL8812AU -soliciting TX (`tests/ack_txreport_matrix.sh`, 8733B as RESPONDER): armed -1736/1736 frames ACKed at retries_mean 0.00; re-armed on a **different** MAC, -1736/1736 again (the address is arbitrary, not baked in); disarmed, 0.00 with -retries pinned at the descriptor limit of 12. So this die can be the receiving -end of a reliable-unicast link. +low half, which is why a monitor radio here does not ACK. + +Three properties of the port worth knowing, none of them local inventions: the +arm refuses a group MAC and is read back before it is reported, both through +the shared `ack::is_unicast` / `ack::verify` beside `enable()` — the register +map lives in one file, so no backend carries a copy that can drift from it. A +config-driven arm that fails **fails the bring-up** rather than handing back a +session that quietly answers nothing. And the disarm is unconditional inside +`Halmac8733bMac::stop()`, not a flag-guarded special case at the device layer: +`stop()` clears only REG_CR's low half, so net_type at 0x0102 survives it, and +siting the clear there means no future path can reach `stop()` and leave an +unowned SIFS-timed transmitter on the air. Verified on air — after an armed +session ends with `teardown_power_down` off, the peer reads ack_rate 0.00 with +retries pinned at 12. + +Measured against an RTL8812AU soliciting TX (`tests/ack_txreport_matrix.sh`, +8733B as RESPONDER): armed 1736/1736 frames ACKed at retries_mean 0.00; +re-armed on a **different** MAC, 1736/1736 again (the address is arbitrary, not +baked in); disarmed, 0.00 with retries pinned at the descriptor limit of 12. + +**What ran is normal-ACK response to unicast singles, and the claim scopes to +that.** `AckResponder.h` notes the same gate is also a hardware *BlockAck* +responder on the generations where that was proven; it is NOT proven here. +`tests/ampdu_ba_check.sh` was pointed at this die (8812CU aggregating TX, 8733B +responder) and came back indeterminate — armed and disarmed both read 0% +delivered at retries 0, so the control arm did not separate — which is the +documented consequence of per-frame CCX accounting not surviving AGG_EN +(`docs/aggregation.md`), not a verdict on this chip. A-MPDU is unported here +anyway. So: normal-ACK response measured; BlockAck response untested, pending +an A-MPDU-capable instrument that does not judge by `tx.report`. **`tx.retry_limit` drives real autonomous retransmission** — `tx_retry_limit_ok` is now true. It could not be measured the way the Jaguars were: that A/B reads @@ -333,9 +345,13 @@ a passive monitor counting airings per submitted frame — and takes a dose-response rather than an on/off pair, because one pair could be ambient and a straight line through three levels cannot. Measured 0 -> 0.93, 3 -> 3.93, 12 -> 12.27 airings/frame against an expected 1 + N, repeatable across a -0/3/12/0/12 ladder. The ~0.07 shortfall is the `rx.txhit` sampling -quantization the harness documents (the event fires on the first 10 hits then -every 100th, so its `hits` field understates by up to 99), not loss. +0/3/12/0/12 ladder. Two different things account for the shortfalls, and only the smaller one is an +artefact: the `rx.txhit` readout quantizes to at most 99 airings (the event +fires on the first 10 hits then every 100th), which is <=0.066/frame at 1500 +frames and covers the 0 and 3 arms entirely. It does NOT cover the 12 arm — +12.27 against 13 is 0.73/frame, about 1095 airings, an order of magnitude past +that bound. That residue is monitor loss (or genuinely fewer airings), which is +why the ratio is reported as a floor and not a point estimate. **CCX / `tx.report` is NOT ported, and the reason is the firmware.** This is the one entry on the Not-ported list whose cause is known but not fixable from diff --git a/src/rtl8733b/Halmac8733bMac.cpp b/src/rtl8733b/Halmac8733bMac.cpp index f4705479..53659a5c 100644 --- a/src/rtl8733b/Halmac8733bMac.cpp +++ b/src/rtl8733b/Halmac8733bMac.cpp @@ -1,5 +1,7 @@ #include "Halmac8733bMac.h" +#include "AckResponder.h" /* shared ACK-responder register recipe */ + #include #include #include @@ -636,13 +638,12 @@ void Halmac8733bMac::init_wmac() { _device.rtw_write32(kRegMar, 0xffffffff); _device.rtw_write32(kRegMar + 4, 0xffffffff); _device.rtw_write8(kRegBbpsfCtrl + 2, 0x84); - /* Vendor HALMAC values. 0x21 (33 us) is the bottom of the 33..128 us - * per-chip spread DeviceConfig::tx::ack_timeout_us exists to abolish, so - * Rtl8733bDevice overwrites REG_ACKTO from the config right after - * bring-up (set_ack_timeout_us) — the same shape Jaguar3 uses against - * halmac's per-bandwidth defaults. Left as-written here because this MAC - * plane is shared verbatim with rtl8733bprobe, which wants the vendor - * recipe and carries no DeviceConfig. */ + /* Vendor HALMAC values. Rtl8733bDevice overwrites REG_ACKTO from + * DeviceConfig::tx::ack_timeout_us right after bring-up + * (set_ack_timeout_us) — the same shape Jaguar3 uses against halmac's + * per-bandwidth defaults. The vendor write stays here because this MAC plane + * is shared verbatim with rtl8733bprobe, which wants the vendor recipe and + * carries no DeviceConfig. */ _device.rtw_write8(kRegAckTimeout, 0x21); _device.rtw_write8(kRegAckTimeoutCck, 0x6a); _device.rtw_write16(kRegEifs, 0x0040); @@ -682,9 +683,9 @@ void Halmac8733bMac::init_wmac() { uint8_t Halmac8733bMac::set_ack_timeout_us(uint8_t microseconds) { _device.rtw_write8(kRegAckTimeout, microseconds); - /* Read back and return what the register carries. The caller logs it: this - * knob was silently ignored here once (issue #2), and a value reported - * without a readback is the same shape of claim that bug was. */ + /* Read back and return what the register carries, so the caller can report + * the window the hardware actually holds: a value reported without a + * readback cannot be told apart from one the radio never received. */ return _device.rtw_read8(kRegAckTimeout); } @@ -806,6 +807,13 @@ bool Halmac8733bMac::configure_monitor_rx(bool keep_corrupted) { } void Halmac8733bMac::stop() { + /* Clear net_type first. The CR write below covers 0x0100-0x0101 only, so the + * port-0 net_type field at 0x0102 survives it — and a nonzero net_type is the + * whole gate of the hardware ACK engine. Left set, the chip keeps answering + * unicast frames with SIFS-timed ACKs after the session that armed it is + * gone. Unconditional and sited here so no caller can reach _mac.stop() + * without it; a no-op on a session (or on rtl8733bprobe) that never armed. */ + devourer::ack::disable(_device); _device.rtw_write32(kRegRcr, 0); _device.rtw_write8( kRegTxdmaPqMap, diff --git a/src/rtl8733b/Rtl8733bDevice.cpp b/src/rtl8733b/Rtl8733bDevice.cpp index 1b270d43..53408842 100644 --- a/src/rtl8733b/Rtl8733bDevice.cpp +++ b/src/rtl8733b/Rtl8733bDevice.cpp @@ -84,11 +84,12 @@ void Rtl8733bDevice::bring_up_to_phy() { * default exists to abolish). Same clamp and same register as jaguar1/2/3; * see the DeviceConfig field doc for the range budget it buys. * - * This was silently ignored until issue #2, which is the failure this - * backend refuses everywhere else (disable_cca, SetTxPowerIndexOverride): a - * config value that reads as applied while the radio runs something else. - * The window is load-bearing here now — both ends of a hardware-ARQ link are - * measured working on this die (SetAckResponder, tx.retry_limit). + * Applied rather than left at the vendor value for the reason this backend + * refuses knobs elsewhere (disable_cca, SetTxPowerIndexOverride): a config + * value that reads as applied while the radio runs something else is the one + * failure worse than an unported knob. The window is load-bearing here — + * both ends of a hardware-ARQ link work on this die (SetAckResponder, + * tx.retry_limit). * * Sited in bring_up_to_phy for the same reason the disable_cca warning below * is: it is where the MAC bring-up that wrote the vendor value has just run, @@ -109,8 +110,17 @@ void Rtl8733bDevice::bring_up_to_phy() { * monitor into an active SIFS-timed transmitter. Sited here with the other * bring-up knobs so an RX-only session (Init) arms it too, which is the * session shape a pure responder actually runs. */ - if (_cfg.rx.ack_responder) - SetAckResponder(*_cfg.rx.ack_responder); + if (_cfg.rx.ack_responder && + !SetAckResponder(*_cfg.rx.ack_responder)) { + /* The caller asked for a responder, not a monitor. Swallowing the refusal + * here would hand back a green init and a session that silently answers + * nothing — the operator then debugs the RF link instead of the config + * (a group MAC in DEVOURER_ACK_RESPONDER is the easy way in: the canonical + * TX SA 57:42:.. has the I/G bit set). Fail the bring-up instead; the + * setter has already logged which of the two reasons applied. */ + throw std::runtime_error( + "RTL8733B: configured ACK responder could not be armed"); + } /* Every other generation applies tuning.disable_cca during bring-up * (jaguar1/2/3, kestrel all call SetCcaMode there), so a caller setting * DEVOURER_DIS_CCA=1 reasonably expects it to take effect. This backend has @@ -816,56 +826,28 @@ bool Rtl8733bDevice::SetAckResponder(const devourer::MacAddr &mac) { * assumption: the vendor 8733BU tree's port-0 descriptor names exactly these * three registers — net_type = REG_CR_8733B + 2 (0x0100 + 2 = 0x0102, shift * 0), macaddr = REG_MACID_8733B (0x0610), bssid = REG_BSSID_8733B (0x0618) - * — the same addresses and the same MSR field the Jaguar backends use * (hal/rtl8733b/rtl8733b_ops.c port_cfg[0], hal/halmac/halmac_reg_8733b.h). * * MAC bring-up leaves net_type at 0 (No Link) because init_mac writes only - * REG_CR's low half (0x0100-0x0101) — which is exactly why a monitor radio - * on this die never ACKs. Flipping the field is the whole gate. */ - if ((mac.bytes[0] & 0x01u) != 0) { - /* A station cannot ACK-target a group address. Refuse rather than arm a - * responder that can never fire — the same footgun AckResponder.h records - * from the AP-mode work. */ + * REG_CR's low half (0x0100-0x0101) — which is why a monitor radio on this + * die does not ACK. Flipping the field is the whole gate. */ + if (!devourer::ack::is_unicast(mac.data())) { _logger->error("RTL8733B: ACK responder needs a UNICAST MAC (I/G set in " "{:02x}) — not armed", mac.bytes[0]); return false; } - /* Latch BEFORE the first write, not after the call and not after the - * readback. ack::enable() is five register writes ending with the net_type - * field that actually arms the engine, and any of them can throw - * (UsbTransport rtw_read/rtw_write on a stalling device) — including after - * the arming write has already reached hardware. An exception here unwinds - * through bring_up_to_phy into Init()/InitWrite()'s catch, which calls - * Stop(); if the flag were set only on the success path, that teardown would - * believe there was nothing to disarm while the chip went on auto-ACKing. - * Same reasoning for a failed readback below. A redundant disarm costs one - * USB round-trip; a missed one leaves an unowned transmitter on the air. */ - _ack_armed = true; devourer::ack::enable(_device, mac.data()); - /* Read back before claiming it. This is the first port of the responder onto - * HALMAC 87xx, and this backend does not report a write it cannot verify - * (the same standard SetCcaMode and SetTxPowerOffsetQdb are held to). */ - const uint8_t net_type = static_cast(_device.rtw_read8(0x0102) & 0x03u); - const uint32_t id_lo = _device.rtw_read(0x0610); - const uint16_t id_hi = _device.rtw_read16(0x0614); - const uint32_t want_lo = static_cast(mac.bytes[0]) | - (static_cast(mac.bytes[1]) << 8) | - (static_cast(mac.bytes[2]) << 16) | - (static_cast(mac.bytes[3]) << 24); - const uint16_t want_hi = - static_cast(mac.bytes[4] | (mac.bytes[5] << 8)); - if (net_type != 0x03 || id_lo != want_lo || id_hi != want_hi) { - _logger->error("RTL8733B: ACK responder did not latch — net_type={} " - "macid=0x{:04x}{:08x} (wanted 3 / 0x{:04x}{:08x})", - net_type, id_hi, id_lo, want_hi, want_lo); - /* Best-effort undo, then leave _ack_armed set regardless: if this disable - * also failed we must not tell teardown the radio is quiet. */ - try { - devourer::ack::disable(_device); - } catch (const std::exception &e) { - _logger->warn("RTL8733B: ACK responder rollback failed: {}", e.what()); - } + /* Verify with the shared readback rather than a local copy of the map: this + * backend does not report a write it cannot confirm (the standard SetCcaMode + * and SetTxPowerOffsetQdb are held to). Teardown disarms unconditionally in + * Halmac8733bMac::stop(), so a half-landed arm cannot outlive the session + * whatever this returns. */ + if (!devourer::ack::verify(_device, mac.data())) { + _logger->error("RTL8733B: ACK responder did not latch for " + "{:02x}:{:02x}:{:02x}:{:02x}:{:02x}:{:02x}", + mac.bytes[0], mac.bytes[1], mac.bytes[2], mac.bytes[3], + mac.bytes[4], mac.bytes[5]); return false; } _logger->info("RTL8733B: hardware ACK responder armed for " @@ -880,7 +862,6 @@ void Rtl8733bDevice::ClearAckResponder() { if (!_mac_ready) return; devourer::ack::disable(_device); - _ack_armed = false; _logger->info("RTL8733B: hardware ACK responder disarmed (net_type=NoLink)"); } @@ -927,27 +908,6 @@ void Rtl8733bDevice::Stop() { } _phy_ready = false; if (_mac_ready) { - /* Disarm the ACK responder explicitly. _mac.stop() clears REG_CR's low - * half (0x0100-0x0101) but net_type lives at 0x0102 and would survive it, - * so a session that ends with teardown_power_down off would leave the chip - * auto-ACKing — an active SIFS-timed transmitter with no session owning - * it. The knob is opt-in precisely because it makes the radio transmit; - * it must not outlive the session that asked for it. - * - * Guarded like the TSSI rollback above and for the same reason: this is a - * read-modify-write, register reads throw on a disconnected device - * (UsbTransport rtw_read), and letting that escape here would abandon the - * REST of teardown — _mac.stop() and the card-disable sequence — which is - * worse than losing the disarm on a device that has already gone away. */ - if (_ack_armed) { - try { - devourer::ack::disable(_device); - } catch (const std::exception &e) { - _logger->warn("RTL8733B: ACK responder disarm failed during shutdown: " - "{}", e.what()); - } - _ack_armed = false; - } _mac.stop(); _mac_ready = false; } diff --git a/src/rtl8733b/Rtl8733bDevice.h b/src/rtl8733b/Rtl8733bDevice.h index f8fa676a..50528ae6 100644 --- a/src/rtl8733b/Rtl8733bDevice.h +++ b/src/rtl8733b/Rtl8733bDevice.h @@ -124,13 +124,6 @@ class Rtl8733bDevice : public IRtlDevice { std::atomic _rx_active{false}; std::atomic _rx_configured_bw{0}; std::atomic _tx_submits{0}; - /* Whether the hardware ACK responder is currently armed. Teardown has - * to disarm it (net_type at 0x0102 survives _mac.stop(), which clears - * only REG_CR's low half), but only when it was actually armed: the - * disarm is a read-modify-write, and spending a USB round-trip on every - * teardown to clear bits that are already zero is a cost the common - * path should not pay. Guarded by _reg_mu like the other state. */ - bool _ack_armed = false; mutable std::recursive_mutex _reg_mu; std::optional _tx_mode_default; }; diff --git a/tests/rtl8733b_retry_limit_onair.sh b/tests/rtl8733b_retry_limit_onair.sh index ef3a308d..f4e259b1 100755 --- a/tests/rtl8733b_retry_limit_onair.sh +++ b/tests/rtl8733b_retry_limit_onair.sh @@ -1,6 +1,6 @@ #!/usr/bin/env bash # rtl8733b_retry_limit_onair.sh — is DeviceConfig::tx::retry_limit a LIVE -# actuator on the RTL8733B, or just an encoded descriptor field? (issue #2) +# actuator on the RTL8733B, or just an encoded descriptor field? # # The 8733B has no CCX / tx.report path (the firmware emits no C2H reports — # src/rtl8733b/CLAUDE.md), so the TX side cannot be its own witness the way @@ -19,7 +19,8 @@ # counting EVENTS undercounts by 100x. The event's own `hits` field is the # cumulative truth, so we read the LAST hits value. That quantizes the total to # the largest multiple of 100 <= H, i.e. H is understated by up to 99 airings -# (worst case ~6% on the retry=0 arm). Each arm therefore gets a FRESH witness +# (at the FRAMES_MIN floor below, <=0.099 airings/frame). Each arm gets a FRESH +# witness # so `hits` starts at zero — the counter is static for the process lifetime and # does not reset between arms. # @@ -41,6 +42,21 @@ PWR_QDB=${PWR_QDB:-12} RA=${RA:-02:de:ad:be:ef:01} OUT=${OUT:-/tmp/rtl8733b_retry} +# The readout is quantized to the last rx.txhit checkpoint (see COUNTING NOTE), +# so the error is a fixed <=99 airings whatever FRAMES is — which only stays +# negligible while FRAMES is large. At FRAMES=60 a healthy retry=0 arm airs ~55 +# times, the last checkpoint reads hits=10, and the arm scores 0.17 against a +# 0.6 floor: a FAIL on working hardware. Refuse below a floor rather than +# report that, for the same reason an arm that did not run is refused instead +# of reported as zero. +FRAMES_MIN=${FRAMES_MIN:-1000} +if [ "$FRAMES" -lt "$FRAMES_MIN" ]; then + echo "ABORT: FRAMES=$FRAMES is below FRAMES_MIN=$FRAMES_MIN — the rx.txhit" >&2 + echo " readout quantizes to <=99 airings, which at this size is a" >&2 + echo " false FAIL on healthy hardware, not a measurement." >&2 + exit 2 +fi + KILL(){ sudo pkill -9 -x rxdemo 2>/dev/null; sudo pkill -9 -x txdemo 2>/dev/null; return 0; } trap KILL EXIT mkdir -p "$OUT"; RESULTS="$OUT/results.jsonl"; : >"$RESULTS"