Skip to content

rtl8733b: add RTL8731BU/RTL8733BU USB RX and TX - #388

Merged
josephnef merged 15 commits into
OpenIPC:masterfrom
snokvist:rtl8733bu-support
Aug 13, 2026
Merged

rtl8733b: add RTL8731BU/RTL8733BU USB RX and TX#388
josephnef merged 15 commits into
OpenIPC:masterfrom
snokvist:rtl8733bu-support

Conversation

@snokvist

@snokvist snokvist commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds a dedicated userspace HALMAC 87xx backend for the RTL8731BU/RTL8733BU Wi-Fi function.

  • Recognizes vendor USB IDs 0bda:f72b and 0bda:b733, while requiring live RTL8733B chip ID 0x16.
  • Implements card power sequencing, cut-selected firmware boot, HALMAC/MAC and EFUSE setup, generated BB/RF/RFK tables, dual-band channel setup, monitor RX, raw TX, thermal telemetry and safe teardown.
  • Advertises the hardware-tested surface only: 1x1 legacy OFDM + HT MCS0-7, BCC, 20/40 MHz, 2.4/5 GHz; long-preamble CCK is accepted only on 2.4 GHz at 20 MHz.
  • Adds reproducible, hash-pinned firmware/table extraction, parser/descriptor/EFUSE/table self-tests, a bounded lifecycle harness, rtl8733bprobe, adapter-doctor integration and capability documentation.
  • Integrates RTL8733B with the cross-generation rx.parse_abort telemetry added in rx.parse_abort: count the abandoned-aggregate RX loss on every generation #387.

This progresses the USB-first portion of #233. It does not auto-close that issue because narrowband RF validation and combo-module/coexistence work remain outside this PR.

Why a separate backend

RTL8733B is HALMAC 87xx, not a Jaguar2/3 variant. Its power, firmware, MAC, RX/TX descriptor and PHY paths therefore remain isolated under src/rtl8733b/.

Both reviewed vendor trees describe this device as 1SS 802.11b/g/n with a 20/40 MHz capability mask. The 2024 tree also contains a later Fix VHT flags commit that removes the accidentally enabled VHT flag. This PR consequently does not adopt #233's provisional VHT/80 MHz/2T2R assumptions.

Firmware and tables are pinned to libc0607/rtl8733bu-20230626 commit 9e5f6845b4e3393207720fc1886668bc80b6af1c. The implementation was compared with libc0607/rtl8733bu-20240806 commit 2ec19e154cffbc2abd98d43d59278dffa6e50d49, including its active-path TSSI, SRRC band-edge, GPIO/coexistence, power-saving and C2H/TX-tasklet changes. The already hardware-validated artifacts were not silently replaced.

Hardware tested

Test device: one 0bda:f72b, RTL8733B cut D, USB high speed. Independent witness: RTL8812AU in monitor mode.

Verified through the normal WiFiDriver / IRtlDevice path:

  • USB discovery/endpoints, repeated power/firmware/MAC/BB/RF bring-up, stable EFUSE reads and permanent MAC extraction.
  • Ambient monitor RX on channels 6 and 36, descriptor aggregation, PHY status, CRC reporting and clean stop.
  • A 60-second high-traffic RX run exposed a 20 KiB device-aggregate versus 16 KiB host-URB split. Capping RX aggregation at 12 KiB fixed it; five repeated high-traffic channel-36 runs then had zero malformed descriptors, aggregate mismatches or parser aborts.
  • Raw TX independently captured on both bands: legacy OFDM and HT MCS0-7 at 20 MHz, plus HT 40 MHz. BCC was forced throughout.
  • Long-preamble CCK independently captured at 1, 2, 5.5 and 11 Mbps on channel 6. A mixed CCK/OFDM run submitted 650/650 frames; 636 were captured with valid FCS across all requested rates. A 1 Mbps request on channel 36 was rejected before USB TX.
  • A rapid transition run submitted 1,000/1,000 frames through 118 CCK/OFDM TSSI table changes with zero USB failures. The witness captured 660 canonical frames across both rates, all with valid FCS; the thermal code stayed at 32-33 against EFUSE baseline 32.
  • Ten bounded warm lifecycle cycles rotating channels 6/36 passed EFUSE, firmware, initialization, RX parsing, thermal, rollback, card-disable and USB-persistence gates.
  • Concurrent RX/TX, bounded sustained TX, malformed-input rejection, teardown during active work, SIGINT during initialization and explicit failure cleanup passed.
  • Correctly targeted post-stress adapter doctor: HEALTHY; 4/4 identical EFUSE reads, firmware ready, 91 clean + 34 corrupt ambient frames, 125 frames total with zero malformed/aggregate errors.
  • Final explicit card-disable readback: RF_CTRL=0x00, CR=0xea.

Checks

  • Full default build: passed.
  • Full CTest matrix: 53/53 passed.
  • RTL8733B-only build (all other chip options disabled): passed.
  • RTL8733B-only CTest matrix: 49/49 passed.
  • RTL8733B-only ASan/UBSan build and CTest matrix: 49/49 passed.
  • bash -n, ShellCheck and failure-cleanup exercise for tests/rtl8733b_lifecycle_soak.sh: passed.
  • Firmware extractor --check: reproduced both checked-in NIC images byte-for-byte.
  • Table extractor --check: reproduced all checked-in MAC/BB/AGC/RF/RFK arrays byte-for-byte.
  • Based on current OpenIPC/devourer:master (da06ccc) for the final local builds and hardware checks.

Explicitly not tested or claimed

  • No SDR equipment was available. Occupied bandwidth, spectral mask, EVM, absolute output power and SDR duty measurements were not performed.
  • Experimental 5/10 MHz has register-readback and normal-path RX evidence only. Narrowband TX and independent narrowband-peer decode are deferred; narrowband_ok remains false.
  • No physical 0bda:b733 combo module was available. Its ID is from the vendor table; Bluetooth/coexistence is not implemented or claimed.
  • SGI remains disabled: descriptors with the short-GI bit set submitted, but the RTL8812AU witness decoded both broadcast and unicast probes as long GI.
  • STBC and LDPC are rejected. The vendor reports TX-LDPC but not RX-LDPC; this raw-injection path has only been validated with global BCC forced, and the known injection defect makes enabling LDPC here unjustified.
  • CCK short preamble, CCK outside 2.4 GHz/20 MHz, VHT, 80 MHz, ACK/BlockAck, A-MPDU, fast retune and throughput are not claimed.
  • The hub cannot switch VBUS, so automated repeated true-cold boots were not possible; physical replug and warm reinitialization were tested.
  • A vendor-kernel-driver cross-test was attempted with the 2024 tree on Linux 6.14. Its build first omitted required include paths, then failed on incompatible MODULE_IMPORT_NS syntax and missing platform_ops.h. No vendor module was loaded, so no vendor-driver parity result is claimed.
  • An RTL8731BU-labelled f72b passed early identity/EFUSE/firmware checks, but later stopped enumerating and overheated while attached to a custom power supply. It is excluded from all radio-validation claims.

The full tested/deferred matrix and provenance are in docs/rtl8733b.md.

@snokvist

Copy link
Copy Markdown
Contributor Author

/review

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

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

Copy link
Copy Markdown

Code Review by Qodo

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

Grey Divider


Action required

1. CR upper byte not cleared 🐞 Bug ≡ Correctness
Description
Halmac8733bMac::configure_monitor_rx enables the MAC using a 16-bit CR write (0x06ff), but
Halmac8733bMac::stop() clears only the low byte via rtw_write8(kRegCr, 0). This leaves CR[15:8] at
0x06 (not symmetric with enable), so stop() does not fully undo the MAC enable it applied,
undermining safe teardown/re-init sequencing.
Code

src/rtl8733b/Halmac8733bMac.cpp[R787-789]

+  _device.rtw_write16(kRegTxPause, 0xffff);
+  _device.rtw_write8(kRegCr, 0);
+}
Evidence
The RTL8733B monitor RX path programs CR via a 16-bit write to 0x06ff (commented as “full
DMA/protocol/MAC TX+RX enable”) and validates it with a 16-bit readback, meaning bits outside the
low byte are intentionally set. The stop() implementation only clears the low byte, so the high byte
(0x06) is not cleared. Other chip teardown code (Jaguar3) clears CR with a 16-bit write to 0x0000
before power-off, reinforcing that CR should be cleared at the same width it was enabled with.

src/rtl8733b/Halmac8733bMac.cpp[757-789]
src/jaguar3/HalJaguar3.cpp[453-461]

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

## Issue description
`Halmac8733bMac::configure_monitor_rx()` writes `kRegCr` as a 16-bit value (`0x06ff`) to fully enable DMA/protocol/MAC TX+RX, but `Halmac8733bMac::stop()` only clears the low byte with `rtw_write8(kRegCr, 0)`. This can leave the upper CR byte (`0x06`) set after stop.
## Issue Context
- The same file explicitly treats CR as a 16-bit value during monitor RX enable and validates it with `rtw_read16(kRegCr) == 0x06ff`.
- Other generations’ deinit paths clear CR with a 16-bit write before powering off.
## Fix Focus Areas
- src/rtl8733b/Halmac8733bMac.cpp[782-789]
### Suggested change
In `Halmac8733bMac::stop()`, replace the 8-bit CR clear with a 16-bit clear (and keep the existing RCR/TxPause shutdown):
- `rtw_write16(kRegCr, 0x0000)` instead of `rtw_write8(kRegCr, 0)`
If RTL8733B CR is wider than 16 bits on this MAC, consider also explicitly clearing any additional CR bytes that are set during init/monitor config, but at minimum stop() should clear the same width it uses to enable.

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



Remediation recommended

2. Rtl8733b omits 5/10 MHz 📎 Requirement gap ≡ Correctness
Description
RTL8733B capability reporting currently excludes 5/10 MHz narrowband (and does not advertise
FastRetune), contrary to the narrowband/FastRetune requirement. This prevents the expected
narrowband long-range mode from being enabled/verified through the normal capability surfaces.
Code

src/AdapterCaps.h[R83-86]

+  return g == ChipGeneration::Rtl8733b ? (kBw20 | kBw40)
+         : g == ChipGeneration::Jaguar1  ? ac
: g == ChipGeneration::Unknown ? 0
                               : (ac | kBw5 | kBw10);
Evidence
PR Compliance ID 7 expects RTL8733B narrowband 5/10 MHz support (and FastRetune). The PR sets
RTL8733B bandwidth mask to only 20/40 MHz, and GetTxCaps() explicitly documents that 5/10 MHz is
omitted; no RTL8733B caps path marks FastRetune as supported.

Implement narrowband 5/10 MHz support and FastRetune (M4 gate)
src/AdapterCaps.h[77-86]
src/rtl8733b/Rtl8733bDevice.cpp[529-536]
src/AdapterCaps.h[213-218]

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

## Issue description
PR Compliance ID 7 requires RTL8733B narrowband 5/10 MHz support plus FastRetune behavior. Current RTL8733B capability reporting excludes 5/10 MHz and does not surface FastRetune support, so narrowband cannot be enabled/validated via the standard capability path.
## Issue Context
- `bw_mask_for_generation(ChipGeneration::Rtl8733b)` currently returns only 20/40.
- `Rtl8733bDevice::GetTxCaps()` explicitly states 5/10 MHz is omitted.
- `AdapterCaps::fastretune_ok` remains at its default (`false`) for RTL8733B.
## Fix Focus Areas
- src/AdapterCaps.h[77-86]
- src/rtl8733b/Rtl8733bDevice.cpp[529-559]
- src/AdapterCaps.h[213-218]

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


3. GetAdapterCaps leaves narrowband_ok false 📎 Requirement gap ≡ Correctness
Description
RTL8733B GetAdapterCaps() does not set narrowband_ok (or fastretune_ok) and hard-codes chain
counts rather than runtime-detecting them, which violates the RTL8733B adapter capability reporting
requirement. This causes demos/telemetry to misrepresent RTL8733B feature availability.
Code

src/rtl8733b/Rtl8733bDevice.cpp[R548-551]

+  caps.tx_chains = 1;
+  caps.rx_chains = 1;
+  caps.tx = GetTxCaps();
+  caps.bw_mask = devourer::bw_mask_for_generation(caps.generation);
Evidence
PR Compliance ID 8 requires RTL8733B capability reporting (including narrowband capability and
correct feature flags). In the added RTL8733B backend, GetAdapterCaps() never assigns
narrowband_ok/fastretune_ok (so they remain false per the struct defaults) and sets chain
counts as fixed constants rather than runtime-derived.

Adapter capabilities reporting for RTL8733B (identity and feature flags)
src/rtl8733b/Rtl8733bDevice.cpp[539-559]
src/AdapterCaps.h[213-218]

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

## Issue description
PR Compliance ID 8 requires RTL8733B `AdapterCaps` to correctly surface identity and feature flags, including narrowband capability, and to avoid misreporting RF chain capabilities.
## Issue Context
- `AdapterCaps::narrowband_ok` and `AdapterCaps::fastretune_ok` default to `false`.
- `Rtl8733bDevice::GetAdapterCaps()` populates identity/bands/`bw_mask`, but does not set `narrowband_ok`/`fastretune_ok` and hard-codes `tx_chains`/`rx_chains`.
## Fix Focus Areas
- src/rtl8733b/Rtl8733bDevice.cpp[539-559]
- src/AdapterCaps.h[118-120]
- src/AdapterCaps.h[213-218]

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


4. UB shift in txdesc ✓ Resolved 🐞 Bug ≡ Correctness
Description
rtl8733b::txdesc_set_bits uses (1u << width) which is undefined behavior for width==32, so encoding
a 32-bit field would produce unpredictable masks and corrupt the TX descriptor. Because this helper
is header-only and reusable, a future call site can trigger this silently.
Code

src/rtl8733b/TxDescriptor8733b.h[R47-50]

+inline void txdesc_set_bits(uint8_t *p, unsigned bit, unsigned width,
+                            uint32_t value) {
+  const uint32_t field = ((1u << width) - 1u) << bit;
+  txdesc_write_le32(p, (txdesc_le32(p) & ~field) |
Evidence
The TX helper computes its mask via 1u << width without guarding width == 32, which is UB. The
nearby RTL8733B RX helper (rx_bits) explicitly special-cases width == 32, demonstrating the
expected safe handling pattern in this repo.

src/rtl8733b/TxDescriptor8733b.h[47-52]
src/rtl8733b/FrameParser8733b.h[21-25]

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

## Issue description
`txdesc_set_bits()` builds its bitmask with `1u << width`; when `width == 32` this is undefined behavior in C/C++ and can generate an incorrect mask/descriptor.
### Issue Context
The RTL8733B TX descriptor encoder is intended to be a generic bitfield writer; the RX-side helper already special-cases `width == 32`, suggesting 32-bit fields are a known boundary in this codebase.
### Fix Focus Areas
- src/rtl8733b/TxDescriptor8733b.h[47-52]
### Suggested fix
- Special-case `width == 32` (and validate `bit == 0`) or compute the mask using a 64-bit intermediate (`1ull << width`) and validate `bit + width <= 32` before shifting.
- Consider explicitly handling `width == 0` as a no-op to avoid accidental full-clear patterns.

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



Informational

5. RX malformed counter misleading ✓ Resolved 🐞 Bug ◔ Observability
Description
Rtl8733bDevice increments malformed on any RX parse failure even when the remainder is all-zero
padding (which emit_rx_parse_abort explicitly treats as benign). This inflates/poisons the RX
summary counters and makes troubleshooting RX parser regressions harder.
Code

src/rtl8733b/Rtl8733bDevice.cpp[R181-184]

+                                    frame)) {
+        ++malformed;
+        devourer::emit_rx_parse_abort(
+            _logger->events(), data + offset,
Evidence
The RX loop counts every parse failure as malformed. However, emit_rx_parse_abort explicitly
excludes all-zero remainder padding, and parse_rx_8733b returns false when frame_len==0—the
decode result you’d get from a zero-filled padded descriptor region—so benign padding can be counted
as malformed.

src/rtl8733b/Rtl8733bDevice.cpp[170-188]
src/RxParseAbort.h[4-10]
src/RxParseAbort.h[19-37]
src/rtl8733b/FrameParser8733b.h[96-103]

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

## Issue description
The RX loop increments the `malformed` counter before determining whether the parse failure is a real abort or just benign all-zero aggregate padding.
### Issue Context
`emit_rx_parse_abort()` is explicitly designed to suppress events for all-zero remainder padding. Separately, `parse_rx_8733b()` returns `false` when `frame_len == 0`, which is exactly what an all-zero padded “descriptor” would decode as.
### Fix Focus Areas
- src/rtl8733b/Rtl8733bDevice.cpp[170-189]
- src/rtl8733b/FrameParser8733b.h[96-103]
- src/RxParseAbort.h[19-37]
### Suggested fix
- Before incrementing `malformed`, detect the benign-padding case (all-zero remainder) and break without counting it as malformed.
- Alternatively, restructure as:
- detect `all_zero` remainder locally (even when event sink is disabled),
- only then increment `malformed` + emit `rx.parse_abort` for non-zero remainder.

ⓘ 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 type 'qodo, fix this' on a finding and the fix lands right on your PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread src/AdapterCaps.h
Comment thread src/rtl8733b/Rtl8733bDevice.cpp
Comment thread src/rtl8733b/TxDescriptor8733b.h
Comment thread src/rtl8733b/Rtl8733bDevice.cpp Outdated
@snokvist
snokvist marked this pull request as ready for review August 11, 2026 15:43
@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

rtl8733b: add USB monitor RX and raw TX support

✨ Enhancement 🧪 Tests 📝 Documentation ⚙️ Configuration changes 🕐 40+ Minutes

Grey Divider

AI Description

• Adds a dedicated HALMAC 87xx backend for RTL8731BU and RTL8733BU USB Wi-Fi.
• Enables validated monitor RX and bounded raw CCK, OFDM, and HT transmission.
• Pins vendor firmware and PHY tables with extraction tools, tests, diagnostics, and documentation.
Diagram

graph TD
  A["WiFiDriver"] --> B["Rtl8733bDevice"] --> C["Bringup and MAC"] --> D["PHY and RF"] --> E["Firmware and Tables"]
  B --> F["RX and TX"] --> G["IRtlDevice Clients"]
  H["Probe and Tests"] --> B
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Extend a Jaguar backend
  • ➕ Could reuse some existing device plumbing and reduce file count.
  • ➖ RTL8733B has incompatible HALMAC power, firmware, descriptor, and PHY paths; shared code risks unsupported capability leakage.
2. Accept PID-only dispatch
  • ➕ Could create the device when a live chip-ID read is transiently unavailable.
  • ➖ Would weaken identity validation and may misroute a device into an incompatible backend.

Recommendation: Keep the isolated HALMAC 87xx backend and require live chip ID 0x16. This matches the hardware architecture, preserves conservative capability gating, and explicitly prevents unsafe Jaguar fallback; common interfaces are reused only at the correct abstraction boundary.

Files changed (38) +38857 / -81

Enhancement (17) +5044 / -34
main.cppInclude RTL8733B USB IDs in adapter doctor +2/-1

Include RTL8733B USB IDs in adapter doctor

• Adds RTL8731BU/RTL8733BU product IDs to the doctor discovery PID set.

examples/doctor/main.cpp

main.cppAdd staged RTL8733B diagnostic probe +406/-0

Add staged RTL8733B diagnostic probe

• Introduces a command-line probe for identity, power, EFUSE, firmware, MAC, PHY, channel, and TSSI stages. Uses guarded teardown to leave diagnostic hardware safely powered down.

examples/rtl8733bprobe/main.cpp

main.cppGeneralize RX thermal telemetry and cleanup +80/-23

Generalize RX thermal telemetry and cleanup

• Moves thermal polling to the common IRtlDevice interface and includes CRC, ICV, and RSSI event fields. Ensures background emitters stop before USB teardown.

examples/rx/main.cpp

main.cppBound TX runs and use time-based thermal polling +26/-9

Bound TX runs and use time-based thermal polling

• Adds an optional finite TX frame limit for reproducible captures. Changes thermal polling to wall-clock scheduling so it remains accurate with custom frame pacing.

examples/tx/main.cpp

AdapterCaps.hAdd RTL8733B capability generation +5/-1

Add RTL8733B capability generation

• Adds the RTL8733B chip generation name and constrains its advertised bandwidth mask to 20/40 MHz.

src/AdapterCaps.h

WiFiDriver.cppDispatch validated RTL8733B devices +27/-0

Dispatch validated RTL8733B devices

• Creates Rtl8733bDevice for live chip ID 0x16 and known RTL8733B USB identities. Refuses the historical Jaguar fallback when a known RTL8733B PID reports an unexpected chip ID.

src/WiFiDriver.cpp

FrameParser8733b.hImplement RTL8733B RX descriptor parsing +161/-0

Implement RTL8733B RX descriptor parsing

• Adds bounds-checked parsing for HALMAC 87xx RX aggregates, C2H packets, PHY status, alignment, and signal metadata.

src/rtl8733b/FrameParser8733b.h

Halmac8733bMac.cppImplement RTL8733B EFUSE and MAC setup +791/-0

Implement RTL8733B EFUSE and MAC setup

• Implements physical EFUSE decoding, normal USB queue and WMAC setup, monitor RX configuration, and safe 12 KiB RX aggregation. Validates expected MAC state and disables unsafe LDPC configuration.

src/rtl8733b/Halmac8733bMac.cpp

Halmac8733bMac.hDefine RTL8733B MAC and EFUSE interfaces +141/-0

Define RTL8733B MAC and EFUSE interfaces

• Defines EFUSE, TX-power, MAC-state, and HALMAC initialization contracts used by the device backend.

src/rtl8733b/Halmac8733bMac.h

Phy8733b.cppImplement RTL8733B PHY, channel, and TSSI control +1606/-0

Implement RTL8733B PHY, channel, and TSSI control

• Loads conditional vendor tables, configures dual-band channels and widths, and applies safe flat or capped TSSI power control with rollback checks.

src/rtl8733b/Phy8733b.cpp

Phy8733b.hDefine RTL8733B PHY planning interfaces +255/-0

Define RTL8733B PHY planning interfaces

• Declares PHY initialization, channel planning, table selection, thermal telemetry, TX gain, and TSSI state validation APIs.

src/rtl8733b/Phy8733b.h

Rtl8733bBringup.cppImplement RTL8733B power and firmware boot +558/-0

Implement RTL8733B power and firmware boot

• Implements identity reads, card enable/disable sequencing, cut-selected firmware download, checksum verification, and ready handshakes.

src/rtl8733b/Rtl8733bBringup.cpp

Rtl8733bBringup.hDefine RTL8733B bring-up contract +83/-0

Define RTL8733B bring-up contract

• Declares chip snapshots and focused card power, firmware, and register-access operations.

src/rtl8733b/Rtl8733bBringup.h

Rtl8733bDevice.cppImplement RTL8733B monitor RX and raw TX +647/-0

Implement RTL8733B monitor RX and raw TX

• Adds production lifecycle orchestration, monitor RX parsing, constrained radiotap TX, thermal gating, TSSI transitions, capabilities, and safe teardown.

src/rtl8733b/Rtl8733bDevice.cpp

Rtl8733bDevice.hDefine dedicated RTL8733B IRtlDevice backend +81/-0

Define dedicated RTL8733B IRtlDevice backend

• Declares the isolated device implementation and its bring-up, MAC, PHY, RX/TX, telemetry, and teardown state.

src/rtl8733b/Rtl8733bDevice.h

Rtl8733bUsbIds.hDefine RTL8733B USB identity gates +34/-0

Define RTL8733B USB identity gates

• Adds vendor USB IDs and authoritative live chip-ID predicates for RTL8731BU/RTL8733BU dispatch.

src/rtl8733b/Rtl8733bUsbIds.h

TxDescriptor8733b.hImplement validated RTL8733B TX descriptors +141/-0

Implement validated RTL8733B TX descriptors

• Encodes 40-byte HALMAC TX descriptors with rate mapping, checksum generation, USB padding, and strict rejection of unsupported modes.

src/rtl8733b/TxDescriptor8733b.h

Bug fix (1) +14 / -11
RxParseAbort.hSeparate RX padding classification from event emission +14/-11

Separate RX padding classification from event emission

• Adds a reusable zero-padding classifier and prevents benign padding from being treated as parse aborts regardless of event-sink state.

src/RxParseAbort.h

Tests (7) +977 / -5
adapter_caps_selftest.cppTest RTL8733B capability and identity gates +14/-0

Test RTL8733B capability and identity gates

• Verifies RTL8733B generation naming, 20/40 MHz limits, chip ID, and supported USB IDs.

tests/adapter_caps_selftest.cpp

parse_abort_smoke.shExercise RTL8733B parse-abort telemetry +5/-5

Exercise RTL8733B parse-abort telemetry

• Adds the tested RTL8733B PID to the cross-generation RX parse-abort smoke suite.

tests/parse_abort_smoke.sh

rtl8733b_efuse_selftest.cppTest RTL8733B EFUSE and MAC invariants +172/-0

Test RTL8733B EFUSE and MAC invariants

• Covers EFUSE map decoding, direct and TSSI power data, fallback behavior, and bounded RX aggregation MAC-state validation.

tests/rtl8733b_efuse_selftest.cpp

rtl8733b_lifecycle_soak.shAdd bounded RTL8733B warm lifecycle soak +180/-0

Add bounded RTL8733B warm lifecycle soak

• Runs repeated doctor/probe cycles with USB persistence, firmware, EFUSE, RX parser, thermal, TSSI rollback, and card-disable gates.

tests/rtl8733b_lifecycle_soak.sh

rtl8733b_phy_table_selftest.cppTest RTL8733B tables, channels, and TSSI plans +295/-0

Test RTL8733B tables, channels, and TSSI plans

• Validates generated table sizes and selection, power targets, channel plans, narrowband readback predicates, and PHY/TSSI state checks.

tests/rtl8733b_phy_table_selftest.cpp

rtl8733b_rx_parse_selftest.cppTest RTL8733B RX descriptor parsing +168/-0

Test RTL8733B RX descriptor parsing

• Covers normal and C2H descriptors, aggregate walking, malformed inputs, padding handling, and CCK/OFDM/HT PHY status parsing.

tests/rtl8733b_rx_parse_selftest.cpp

rtl8733b_tx_desc_selftest.cppTest RTL8733B TX descriptor encoding +143/-0

Test RTL8733B TX descriptor encoding

• Verifies descriptor golden bytes, checksums, CCK/OFDM/HT rate mapping, and unsupported-mode rejection.

tests/rtl8733b_tx_desc_selftest.cpp

Documentation (5) +173 / -19
README.mdDocument RTL8733B support surface +23/-12

Document RTL8733B support surface

• Updates supported generations and hardware tables for RTL8731BU/RTL8733BU. Documents conservative 1x1 HT/20–40 MHz capability limits and the diagnostic probe.

README.md

narrowband.mdClarify experimental RTL8733B narrowband status +8/-3

Clarify experimental RTL8733B narrowband status

• Documents the 5/10 MHz register workaround and explicitly keeps it unadvertised pending RF validation.

docs/narrowband.md

rtl8733b.mdAdd RTL8733B validation and provenance record +136/-0

Add RTL8733B validation and provenance record

• Documents supported identities, validated RX/TX capabilities, safety limits, deferred work, hardware results, and pinned vendor-source provenance.

docs/rtl8733b.md

IRtlDevice.hRegister RTL8733B as an IRtlDevice implementation +3/-1

Register RTL8733B as an IRtlDevice implementation

• Documents the new dedicated HALMAC 87xx backend in the common device contract.

src/IRtlDevice.h

WiFiDriver.hUpdate factory backend documentation +3/-3

Update factory backend documentation

• Describes that the factory can return the dedicated RTL8733B backend through IRtlDevice.

src/WiFiDriver.h

Other (8) +32649 / -12
cmake-multi-platform.ymlAdd RTL8733B build-selection coverage +14/-11

Add RTL8733B build-selection coverage

• Adds an RTL8733B-only matrix job and explicitly disables RTL8733B in other chip-only jobs. Updates the no-chip configure failure gate.

.github/workflows/cmake-multi-platform.yml

CMakeLists.txtBuild and test the RTL8733B backend +55/-1

Build and test the RTL8733B backend

• Adds the DEVOURER_8733B option, backend sources, generated artifacts, probe executable, and RTL8733B self-tests. Allows RTL8733B as the sole enabled chip family.

CMakeLists.txt

hal8733b_fw.cVendor RTL8733B NIC firmware blobs +30566/-0

Vendor RTL8733B NIC firmware blobs

• Adds generated, hash-pinned normal and CCV NIC firmware images extracted from the selected vendor source.

hal/hal8733b_fw.c

hal8733b_fw.hExpose RTL8733B firmware blobs +15/-0

Expose RTL8733B firmware blobs

• Declares the generated normal and CCV firmware arrays and their lengths.

hal/hal8733b_fw.h

hal8733b_tables.cVendor RTL8733B PHY parameter tables +1642/-0

Vendor RTL8733B PHY parameter tables

• Adds generated, hash-pinned MAC, BB, AGC, RF, power-target, and RFK initialization tables.

hal/hal8733b_tables.c

hal8733b_tables.hExpose RTL8733B parameter tables +26/-0

Expose RTL8733B parameter tables

• Declares generated parameter arrays consumed by the RTL8733B PHY implementation.

hal/hal8733b_tables.h

extract_8733b_fw.pyAdd reproducible RTL8733B firmware extractor +144/-0

Add reproducible RTL8733B firmware extractor

• Extracts only required NIC firmware images while validating pinned source, blob sizes, hashes, and checked-in output reproducibility.

tools/extract_8733b_fw.py

extract_8733b_tables.pyAdd reproducible RTL8733B table extractor +187/-0

Add reproducible RTL8733B table extractor

• Extracts vendor MAC/BB/AGC/RF/RFK arrays with source and stream hash checks, then verifies or regenerates checked-in artifacts.

tools/extract_8733b_tables.py

Comment thread src/rtl8733b/Halmac8733bMac.cpp
@qodo-free-for-open-source-projects

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 0bfd8c3

@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.

Review

Overall: high-quality backend — the code reads like the existing HALs, the RX parse is genuinely bounds-safe, caps are honest, and the extractors are the best hash-pinning in tools/. A handful of items below are worth resolving before merge; three of them cluster around one design decision (the hardcoded thermal-policy layer), plus the destructive SetCcaMode refusal.

Verified during review (not just read)

  • Both ON and 8733B-only builds compiled here; ctest 53/53 and 49/49 pass, matching the PR claims.
  • git diff master...HEAD -- src/jaguar1 src/jaguar2 src/jaguar3 src/kestrel is empty — no existing HAL touched. The RxParseAbort.h refactor is behavior-preserving for the four existing generations (checked combination-by-combination).
  • Dispatch is conservative in exactly the right way: chip-id 0x16 required, a known 8733B PID with a failed/wrong chip-id read is refused rather than falling through to Jaguar1, and DEVOURER_8733B=OFF returns nullptr+log on both paths.
  • All 9 generated-array SHA-256 pins in the extractors were recomputed against the checked-in hal/hal8733b_*.c — all match, and the tools do no network access.
  • The 12 KiB RX-aggregate cap is real, enforced at init and readback-verified via MacState::matches_normal_usb3out().
  • EFUSE packed-map decoding (extended-header block math, active-low word-enable, bounds on truncated headers) was hand-checked against vendor semantics; the TX golden descriptors were hand-decoded field-by-field.
  • The examples/rx rework fixes a real pre-existing bug (the qd/thermal emitter threads were never stopped/joined — a std::terminate on early-return paths).

Findings not tied to a single diff line

  • 8733B PIDs missing from the demo discovery lists: examples/doctor gained 0xf72b/0xb733 in kRealtekPids, but neither examples/rx nor examples/tx kRealtekProductIds did — a plugged 8733B is invisible to rxdemo/txdemo without DEVOURER_PID=0xf72b. Add to both or note why not.
  • DEVOURER_TX_FRAMES (new txdemo demo-local var) is referenced nowhere — README/docs need a line.
  • The first-8 RX completion/frame breadcrumbs log at info; convention puts per-frame diagnostics at debug/trace (bounded to 8, so cosmetic).
  • rtl8733bprobe's add_executable sits mid-Kestrel section in CMakeLists.txt, far from the other example targets — works, but breaks the file's grouping.
  • Extractors use read_text()/write_text() without explicit encoding=/newline=: on Windows --check won't be byte-identical (CRLF). Pre-existing pattern in older extractors, but these otherwise raise the bar — worth the two keywords.
  • Headless test gaps: STBC/VHT radiotap rejection lives only in Rtl8733bDevice.cpp and has no selftest; the RX walk's tail case (last frame's 8-byte-rounded next_offset past buflen) is untested.

The hardware-validation narrative in the PR description and docs/rtl8733b.md is unusually honest (SGI decode failure, the dead second unit, the failed vendor-module cross-test all disclosed) — appreciated.

Comment thread examples/rx/main.cpp Outdated
Comment thread src/rtl8733b/Rtl8733bDevice.cpp Outdated
Comment thread src/rtl8733b/Rtl8733bDevice.cpp Outdated
Comment thread src/rtl8733b/Rtl8733bDevice.cpp Outdated
Comment thread tools/extract_8733b_tables.py
Comment thread src/rtl8733b/Rtl8733bBringup.cpp Outdated
Comment thread docs/rtl8733b.md Outdated
Comment thread README.md Outdated
Comment thread tests/rtl8733b_lifecycle_soak.sh Outdated
Comment thread tests/rtl8733b_tx_desc_selftest.cpp
snokvist and others added 5 commits August 12, 2026 22:36
Review follow-ups on the RTL8733B backend (OpenIPC#388).

Thermal readings become telemetry, matching every other generation:

- rxdemo's cross-generation poller no longer stops RX at a hardcoded
  delta >= 25. Generalizing it onto IRtlDevice::GetThermalStatus was the
  point; the kill-switch rode along and applied that threshold to Jaguar1/2/3
  and Kestrel too — sharpest on the 8822C, whose baseline is a first-read cold
  reference rather than an EFUSE constant. It emits and warns, nothing more.
- InitWrite no longer refuses TX on a warm chip. It keeps a one-shot bring-up
  snapshot, the same shape as the Kestrel InitWrite snapshot.
- send_packet reads no register at all. read_thermal() is 3 RF writes + a 15 us
  settle + an RF read, several USB control transfers per frame on a USB-HS part,
  and no other generation does register I/O on the send path.
- enable_tssi_tracking's verdict is register readback only. It runs on every
  CCK<->OFDM rate-table transition — i.e. inside send_packet — and the meter
  tracks PA bias, not whether those BB writes landed.
- rtl8733bprobe reports the "critical" bucket instead of aborting.

The meter is a PA-bias tracking index, not a calibrated junction temperature,
and docs/warm-tx-degradation.md shows it is not a validated degradation
predictor. The remaining bound, kTssiAuditThermalCeiling, is named and confined
to the audit_tssi_* probe stages.

Other device-side fixes:

- SetCcaMode(false) is the universal default and now succeeds as a no-op
  instead of card-disabling the chip and throwing. Only `true` refuses, and it
  no longer tears the session down; radio_operation_unavailable goes with it.
- The RX URB floor is the device-side aggregate ceiling (kRxAggregateBytes8733b,
  12 KiB) rather than 4096, so DEVOURER_RX_URB_BYTES cannot reintroduce the
  aggregate straddle from the host side. A static_assert keeps the constant and
  the programmed RXDMA_AGG page count in step.
- A fatal TSSI rate-table failure latches its reason in _tx_fatal, so later
  rejections say the session was stopped rather than "TX rejected before
  InitWrite". send_packet keeps its bool contract.
- parse_physical_efuse treats address-space exhaustion as
  success-with-whatever-parsed. A fully written map has no 0xff terminator left
  to find, and failing there bricked bring-up on a valid but heavily
  reprogrammed EFUSE.
- _power_ready is armed before power_on(), so a mid-sequence failure still
  rolls back through power_off() instead of leaving the card half-enabled.
- power_on() logs the computed verdict instead of asserting "card active" one
  line before the caller throws "card-enable sequence failed".
- The first-8 RX completion/frame breadcrumbs move to the debug plane.
- The modulation-admission contract moves into two pure predicates in
  TxDescriptor8733b.h, shared by the SetTxMode and radiotap branches, so the
  SGI/LDPC/STBC refusals are testable without hardware.
- rxdemo and txdemo discover 0bda:f72b / 0bda:b733, so a plugged RTL8733B is
  visible without DEVOURER_PID.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both extractors read reference/rtl8733bu-20230626, but nothing pinned it, so
`--check` was irreproducible from a fresh checkout and neither the generated
firmware/tables nor the hand-transcribed halmac_reg2.h and power-sequence
constants in src/rtl8733b/ could be re-verified by a reviewer.

Add libc0607/rtl8733bu-20230626 at 9e5f684 (branch v5.13.0.1, shallow, matching
the other reference pins), document it in reference/README.md, and make both
extractors' missing-input errors name the pinned commit and the exact
`git submodule update --init` line.

The 2024 tree (2ec19e1) stays out of the submodule set and is recorded as a
deviation: nothing in hal/ is generated from it, and the already
hardware-validated artifacts were not replaced with untested ones.

Verified against the pinned tree: both `--check` runs reproduce every
checked-in artifact byte-for-byte.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Soak script:
- set -euo pipefail (was set -u), with the doctor exit captured explicitly and
  the negated parse_abort match rewritten as `! grep || failed=1` — as written
  it exited non-zero on the expected no-match case. mkdir/sha256sum failures no
  longer fall through. shellcheck clean.
- Anchor the JSON value delimiter on the numeric gates: '"efuse_reads":4' also
  matched '"efuse_reads":40'.
- Canonicalise VID/PID to lowercase 0xNNNN and lowercase the sysfs read, so a
  VID=0x0BDA no longer fails verification of a present, correct device.

Extractors: explicit encoding="utf-8", newline="" on the generated-artifact
read/write, so `--check` compares bytes identically on Windows instead of
passing a CRLF round-trip while the checked-in file differs.

Selftests, closing the gaps the review named:
- The CCK-on-5GHz / CCK-at-40MHz chain end-to-end: tx_rate_id_8733b returns the
  0xff sentinel *and* fill_tx_desc_8733b refuses it, plus an out-of-range
  rate_id. The two halves were only tested in isolation.
- The modulation-admission contract (HT MCS0-7, 20/40 MHz, no SGI/LDPC/STBC;
  legacy 20 MHz only), which previously had no headless coverage at all — the
  descriptor encoder has no STBC field to reject on.
- The RX walk's tail case: a last frame whose 8-byte-rounded next_offset lands
  past buflen must still parse, and must terminate the walk.
- EFUSE address-space exhaustion is accepted with the full span reported.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- README: split RTL8731BU and RTL8733BU into one row per chip, matching the
  table's schema. The validated surface belongs to the tested unit — a bare
  unbranded 1T1R module, 0bda:f72b cut D, an RTL8731BU. The RTL8733BU row says
  what it is: recognized from the vendor ID table only, no physical unit, no
  Bluetooth/coexistence.
- docs/rtl8733b.md: state present-tense facts instead of PR-relative changelog
  ("is now capped", "the claims in this PR", the OpenIPC#233 framing). Name the single
  DUT the whole matrix rests on, and record that the unit that overheated and
  stopped enumerating was the same chip — so the RTL8731BU row is a one-unit
  result, not a population. Rewrite the thermal paragraph as telemetry, document
  the SetCcaMode contract, and point the extraction commands at the submodule.
- CLAUDE.md: document DEVOURER_TX_FRAMES, and correct the thermal knob's entry —
  it claimed a rising delta is "the early TX-degradation warning", which
  docs/warm-tx-degradation.md contradicts and which is what the removed policy
  layer was built on.
- CMakeLists: move rtl8733bprobe out of the middle of the Kestrel section to sit
  with pcieprobe and kestrelprobe, its siblings.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The review's last named test gap. tx_mode_supported_8733b folds the PPDU family
into the shared admission predicate, so build_tx_block's SetTxMode branch no
longer carries its own `else { return 0; }` and the VHT/HE refusal is asserted
in tests/rtl8733b_tx_desc_selftest.cpp alongside the STBC/SGI/LDPC and
second-stream cases.

The two radiotap VHT/HE case labels in build_tx_block still have no headless
coverage — they sit inside the iterator switch and testing them would need a
device — but they now enforce the same contract this predicate states.

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

Copy link
Copy Markdown
Contributor Author

Thanks — that was a genuinely useful review, and the thermal cluster was the right thing to lead with. Five commits pushed (c374834..03502d3), replies on each thread.

The thermal layer is gone, not made configurable

Your three thermal findings shared one root cause, so rather than gate each one behind a knob I checked what the other backends actually do: thermal is pure telemetry everywhere — GetThermalStatus() on IRtlDevice, Kestrel logs one snapshot at InitWrite, nothing gates TX. This backend had grown a policy layer no other generation has, and the honest fix was to match them:

  • rxdemo's poller emits and warns, never stops RX.
  • InitWrite keeps a one-shot bring-up snapshot and refuses nothing.
  • send_packet reads no register.
  • Phy8733b::enable_tssi_tracking folded a live read_thermal() into its readback verdict — and select_tssi_rate_table calls it on every CCK↔OFDM transition, i.e. inside send_packet. So a mixed-rate run was paying the RF-read cost there as well as in the two places you flagged. Its verdict is register readback only now.
  • rtl8733bprobe reports the "critical" bucket instead of aborting.

The only threshold left is kTssiAuditThermalCeiling, named and confined to the probe-only audit_tssi_* stages.

I also corrected CLAUDE.md, which described a rising delta as "the early TX-degradation warning". That claim is what the policy layer was built on, and docs/warm-tx-degradation.md contradicts it — delivery scattered 63–83% with no relation to the meter.

For the record on where the layer came from: an adapter overheated and died early in this bring-up (almost certainly a faulty bench supply, not the chip), and the guards were added to protect the hardware while new features were being written. They were development scaffolding that should not have shipped in the library.

Non-line findings

Finding Status
8733B PIDs missing from rxdemo/txdemo Added 0xf72b + 0xb733 to both kRealtekProductIds
DEVOURER_TX_FRAMES undocumented Documented in CLAUDE.md beside DEVOURER_TX_GAP_US
First-8 RX breadcrumbs at info Both the completion and frame breadcrumbs moved to debug
rtl8733bprobe mid-Kestrel in CMakeLists Moved to sit with pcieprobe/kestrelprobe
Extractors' read_text/write_text encoding="utf-8", newline="" on both, with the CRLF reasoning in a comment
Headless test gaps Closed — see the two selftest threads

Two places I did not do what you suggested

  • The fatal TSSI path kept its bool return rather than throwing: send_packet throws on no other generation and txdemo's loop does not catch. The reason is latched in _tx_fatal instead, so later sends say the session was stopped rather than "TX rejected before InitWrite" — which was the actual confusion. Say the word if you'd rather have the consistency with the other fatal paths.
  • SetCcaMode(true) still throws (non-destructively). Minor correction to the finding: both timesync call sites are SetCcaMode(true), not false, and both sit behind c.hwbeacon — so timesync still raises there. What's fixed is that it no longer powers the chip down first.

Verification

Headless only: full build 53/53, RTL8733B-only 49/49, RTL8733B-only ASan+UBSan 49/49. Both extractors' --check reproduce every checked-in artifact byte-for-byte from the newly pinned submodule, and their missing-input paths were exercised. bash -n + shellcheck clean on the soak script.

Nothing here has been re-tested on hardware. The changes with real on-air exposure are the removal of the enable_tssi_tracking thermal term (which had been part of the pass criteria for the CCK/OFDM transition runs quoted in the PR body) and the RX URB floor. The soak script's rewrite is lint-checked only — it has not been run against the DUT since. I'll re-run the transition-churn and lifecycle soak on the RTL8731BU unit and post the results before you merge, unless you'd rather take it as-is.

@snokvist

Copy link
Copy Markdown
Contributor Author

Hardware re-validation of 03502d3

Ran on the RTL8731BU DUT as promised. DUT: bare unbranded 1T1R module, 0bda:f72b, die 0x16 cut D, USB high speed, bus 1 port 1.2, no kernel driver bound (rtw88 does not claim f72b). Everything below is against 03502d3, with c374834 — the pre-review PR head — built into a throwaway worktree as the before-side.

Lifecycle soak — 10/10 PASS

REPS=10 CHANNELS="6 36". All HEALTHY, thermal 32–33 vs baseline 32, the same range as the original run. Per-rep gates (EFUSE stability, fw-ready, init, TSSI rollback, power-off ready=1 CR=0xea, RF_CTRL=0x00, malformed=0 agg_mismatch=0) all matched real output — re-checked independently against every rep log, and zero rx.parse_abort across all ten.

The rewritten script got a negative control as well, since set -euo pipefail could have turned a graded failure into a silent abort: forcing a doctor timeout produced FAIL rep=1 rc=124, exit 1, and a cleanup-trap card-disable that landed. Details on that thread.

Transition churn — 1000/1000, zero failures

Alternating 1M,6M,2M,MCS0,5.5M,MCS4,11M,MCS7 on ch6, so every frame forced a CCK↔OFDM table switch — 1000 transitions (500 each way) rather than the 118 in the original run, i.e. ~8.5× the churn. submitted=1000 failed=0, and zero of: TSSI rate-table transition failed, TSSI tracking: enable readback failed, unsafe enable precondition, TX failed/short, TX rejected.

That is the specific thing the change needed to prove: enable_tssi_tracking's readback verdict, with the thermal term removed, held across 1000 consecutive transitions.

What removing the reads actually cost — measured

Wall clock minus the measured txdemo.first_tx_submit mark (so bring-up is excluded; it was 2.22–2.26 s on both builds). Two runs each, interleaved.

Per-frame read — max duty, 2000 frames, MCS4, DEVOURER_TX_GAP_US=0:

build ms/frame submission rate
c374834 2.707 / 2.708 369 fps
03502d3 0.200 / 0.196 ~5060 fps

Per-transition read — 500 frames, every frame a table switch:

build ms/transition
c374834 91.96 / 92.19
03502d3 86.71 / 86.72

So the per-frame read was 2.51 ms of a 2.71 ms budget — @josephnef's "at injection rates this dominates the per-frame budget" was literally right, at 93% — and the per-transition read was a further 5.36 ms.

RX URB floor — clamps on hardware

DEVOURER_RX_URB_BYTES=4096, the exact window that previously let an aggregate straddle two URBs:

devourer [W] RTL8733B RX: raising urb_bytes 4096 -> 12288 (device RX aggregate cap)
devourer [I] RTL8733B RX: stopped completions=691 frames=1117 malformed=0 agg_mismatch=0

Post-stress health

Doctor after everything: HEALTHY, 4/4 identical EFUSE reads, 686 clean + 161 corrupt ambient frames on ch36, clean card-disable (CR=0xea, RF_CTRL=0x00).

Not claimed

  • No witness adapter was plugged, so nothing above is an on-air result. The TX numbers are submission rate — bulk-OUT accepted by the chip, failed=0 — not airtime and not delivery. The independently-witnessed CCK/OFDM captures in the PR description still stand on the earlier runs; I have not re-witnessed them since these commits.
  • No SDR, so the occupied-bandwidth / mask / EVM gaps are unchanged.
  • Still warm-only — the hub cannot switch VBUS.

One finding that is not about this PR

The residual ~84 ms per CCK↔OFDM table switch is the switch itself — disable tracking → rewrite the thermal table → re-enable, all USB control transfers — and it caps a mixed CCK/OFDM stream at ~11.5 fps on this backend. It predates this PR and I have not touched it here; happy to open a separate issue if that is worth pursuing.

Two diagnostic corrections found while profiling the CCK<->OFDM table switch
on hardware.

`prepare_tssi_thermal` logged `offsets[0]` and `offsets[15]` as "cold_edge" /
"hot_edge". Those two words are `00000000` in *both* the CCK and the OFDM/HT
plan — the tables differ only in words 4..11, the swing ramp either side of the
baseline — so the line could never show whether a rate-table swap took effect.
Every log from a churn run read `edge=00000000/00000000` regardless of rate.
Report `swing_lo`/`swing_hi` (words 4 and 8) plus `fold`, an XOR over all 16
words that moves if any word changes. On the DUT the two tables are now
distinguishable: `swing=00000000/00000000 fold=00000000` for OFDM/HT versus
`swing=fdfd0000/05050505 fold=0300fdfb` for CCK.

The `0x881a` entry added to the rx/tx demo discovery lists was commented as
RTL8812EU only. That PID also ships on the Jaguar1 RTL8812AU-VS — observed here
on hardware, reporting chip-id 0x04, 2T2R, `EFUSE_HIDDEN_8812AU_VS`. Harmless,
because the factory dispatches on the SYS_CFG2 chip-id rather than the PID, but
the comment said otherwise.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Re-validated on the DUT with an RTL8812AU (0bda:8812) witnessing in monitor
mode — the independent-capture gap the earlier post-review runs could not
close, because no second adapter was plugged at the time.

Per-rate delivery at 300 frames each: 1 Mbps CCK, 6 Mbps OFDM and HT MCS7 all
300/300 with zero FCS errors; 11 Mbps CCK >= 900/1000 over two repetitions.
The mixed run alternating all eight rates put a CCK/OFDM table switch on every
frame and the witness decoded all eight rate_hw values on air with valid FCS,
20 MHz, STBC and LDPC clear.

Also records two things the profiling turned up, both pre-existing and neither
introduced here: the 84 ms CCK<->OFDM table switch (OpenIPC#389), and the TSSI
settling behaviour behind it — alternating at ~9 ms/frame leaves CCK about 5 dB
above its settled level, while the same stream paced to ~86 ms/frame lands on
it. That is the closed loop converging, and it is a trap for anyone reading
power numbers off a fast rate-switching run.

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

Copy link
Copy Markdown
Contributor Author

Witnessed on-air results — closing the gap from my last comment

A working RTL8812AU turned up, so the "submission rate, not airtime" caveat on my previous comment no longer applies. Recorded in docs/rtl8733b.md (c516d5d).

Witness: RTL8812AU 0bda:8812, monitor mode, channel 6. DUT unchanged (bare RTL8731BU module, 0bda:f72b cut D). 300 frames submitted per rate. The witness's hit counter reports the first ten hits then every hundredth, so these are floors at 100-frame granularity:

requested captured FCS errors
1 Mbps CCK 300 / 300 0
6 Mbps OFDM 300 / 300 0
HT MCS7 300 / 300 0
11 Mbps CCK >= 900 / 1000 0

11 Mbps first landed exactly on a counter boundary, so I re-ran it at 1,000 frames; both repetitions landed in the same bucket. The small deficit relative to the other three is repeatable rather than noise, and expected — it is the least robust CCK rate.

Mixed run: 250 frames alternating all eight rates, i.e. a CCK/OFDM TSSI table switch on every frame. The witness decoded all eight rate_hw values on air — 0/1/2/3 (1/2/5.5/11 Mbps CCK), 4 (6 Mbps OFDM), 12/16/19 (MCS0/4/7) — every one with valid FCS, bw=0, stbc=0, ldpc=0, matching the forced-BCC contract this backend advertises.

Two more commits since the review, both from things the profiling turned up:

  • 2b88b1cprepare_tssi_thermal logged offsets[0]/offsets[15] as cold/hot edge. Those two words are 00000000 in both the CCK and OFDM plans (they differ only in words 4..11), so the line read edge=00000000/00000000 on every churn log regardless of rate and could never show whether a swap took effect. Now reports the words that differ plus a fold over all 16. Same commit corrects the 0x881a comment I added to the demo PID lists: that PID also ships on a Jaguar1 RTL8812AU-VS, observed here reporting chip-id 0x04. Dispatch was always right — it reads SYS_CFG2, not the PID — only the comment was wrong.
  • c516d5d — the results above.

Full matrix re-run on every commit: 53/53, RTL8733B-only 49/49, RTL8733B-only ASan+UBSan 49/49.

Still not claimed

No SDR, so occupied bandwidth, spectral mask, EVM and absolute power remain unmeasured — the witness gives delivery and relative RSSI, not calibrated power. Narrowband TX, the 0bda:b733 combo module and true VBUS-cold boots are unchanged from the PR description.

Two questions from my earlier replies still open for you

Whether the fatal TSSI path should throw rather than return bool (I kept bool for consistency with the other generations' send_packet, and latched the reason in _tx_fatal), and whether SetCcaMode(true) throwing non-destructively is acceptable. Happy to change either.

@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.

Follow-up review — all 13 comments addressed

Re-reviewed at c516d5d. Every item from the previous round is resolved, several beyond what I asked for. Verified independently rather than taken on faith:

  • Fresh worktree at PR head + git submodule update --init reference/rtl8733bu-20230626 → checked out exactly 9e5f6845b4e3393207720fc1886668bc80b6af1c, then both extractors' --check verified all 9 arrays byte-for-byte from that clone — which is precisely the property that was missing before. Full build clean (no new warnings), ctest 53/53, bash -n clean.
  • git grep -n 'delta >= 25' now returns only Phy8733b.cpp's kTssiAuditThermalCeiling, correctly scoped to the audit_tssi_* probe stages with the comment explaining why a probe verdict is not a runtime gate. The device send path, InitWrite, the rxdemo poller and rtl8733bprobe are all clear.

On the measurements you added

The per-frame thermal read at 2.51 ms of a 2.71 ms budget is a better result than I expected — and thank you for labelling it submission rate with no witness plugged rather than letting 13.7× read as an on-air number. That distinction is exactly the house style. Same for the negative control on the soak script (LISTEN_SECS=8 TIMEOUT_SECS=10 → rc=124 captured, gates ran, cleanup trap fired) — proving set -euo pipefail didn't convert a graded failure into a silent abort is the check that actually matters when adding -e to a script whose whole job is grading non-zero exits. And the note that the verdict line itself read HEALTHY there while the gate still failed on the exit code is the kind of detail that usually gets left out.

The [,}] anchoring turning out to be load-bearing ("init":1} at end-of-object) is a good catch on my behalf — I'd have suggested a comma and broken that gate.

Two things you volunteered that weren't asked for and are worth keeping: the TSSI settling observation (CCK airing ~5 dB above its settled level when alternating at ~9 ms/frame, converging at 86 ms/frame) paired with the favourable delivery table, and the 11 Mbps deficit reported as repeatable-across-two-runs rather than smoothed away. Both are the adversarial counterpart the docs convention asks for. Filing the 84 ms table-switch cost as #389 instead of burying it in this PR is the right call.

Fixes that went past the ask

  • kRxAggregateBytes8733b with a static_assert tying the URB floor to the programmed RXDMA_AGG page count — I asked for a clamp; pinning the two constants to each other so they can't drift is better, and the warn on raise means a user who set DEVOURER_RX_URB_BYTES=4096 learns why it moved.
  • _power_ready armed before power_on() with the reasoning spelled out (power_off() is the full card-disable flow and is safe against a partially-enabled card) — that's the correct inversion, not just a rollback bolt-on.
  • tx_mode_supported_8733b / ht_request_supported_8733b factored out so the SetTxMode and radiotap branches share one predicate. That converts "these two branches happen to agree today" into a structural property, and makes the STBC refusal testable on a descriptor encoder that has no STBC field to reject on.
  • _tx_fatal latching a static reason string so a retry loop can distinguish "InitWrite hasn't run" from "the session was deliberately stopped" — cleaner than the throw I floated.
  • The 881a PID collision comment (Jaguar3 8812EU vs Jaguar1 8812AU-VS, chip-id dispatch makes it harmless) is a genuinely useful drive-by that has nothing to do with this backend.

README

The split is exactly right, and separating RTL8731BU (1T1R, the validated row) from RTL8733BU (1T1R + BT, vendor-table-only, BT/coex not implemented) matches how RTL8812EU/RTL8822EU and RTL8852BU/RTL8832BU are handled. "bare unbranded 1T1R module" is an honest DUT description — it's the right answer when there's no brand to name. And docs/rtl8733b.md now stating that the second f72b unit would have covered the same README row, so the RTL8731BU row rests on one surviving sample — "a one-unit result, not a population" — is the sentence that makes the table readable a year from now.

One remaining nit (non-blocking, your call)

_cfg.tuning.disable_cca is silently dropped on this backend. Every other generation applies the knob during bring-up (if (_cfg.tuning.disable_cca) SetCcaMode(true); — jaguar1:82/1349, jaguar2:251, jaguar3:102/868, kestrel:257), so DEVOURER_DIS_CCA=1 is a no-op here while the direct setter throws. Given you deliberately made the setter refuse loudly rather than no-op, the config path is the one door left where the request vanishes without a word. Wiring it would surface as a hard throw at InitWrite — arguably correct but harsh for a knob CLAUDE.md describes as on-by-default for the streamtx FPV downlink; a one-line warn in bring-up when the knob is set would probably be the better trade. Either way it doesn't need to hold the PR.

LGTM. Nothing blocking from my side.

@josephnef

Copy link
Copy Markdown
Collaborator

Deferred work from this PR is now tracked, all of it gated on the incoming BL-M8733BU2 board so nothing here blocks the merge:

#233 stays open as the umbrella; its USB-first scope is delivered here and the remainder is the list above.

Merging. Thanks for the two review rounds — the follow-ups measured the things I could only assert, and the 93%-of-send-budget number turned a code-review opinion into a fact.

@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.

Approving. Round-two verification: fresh worktree at PR head, submodule init checked out exactly 9e5f684, both extractors' --check verified all 9 arrays byte-for-byte from that clone, full build clean, ctest 53/53. All 13 review comments resolved, several beyond the ask. Deferred RF work tracked in #390-#394.

@josephnef
josephnef merged commit cccb863 into OpenIPC:master Aug 13, 2026
23 checks passed
@snokvist

Copy link
Copy Markdown
Contributor Author

Thanks for the re-review, and for re-verifying the submodule property from a fresh clone rather than trusting my word for it — that was the one claim that couldn't be checked before.

disable_cca — wired, as a warning (412f149)

You were right that the config path was the one door where the request vanished without a word, and right about the trade. Bring-up now emits:

devourer [W] RTL8733B: DEVOURER_DIS_CCA / tuning.disable_cca is not implemented
             by this backend — carrier-sense stays ENABLED for this session

and carries on. Verified on the DUT: warning fires, InitWrite completes, TX proceeds. A hard throw would have been the harsh answer you described, for a knob that is on by default on the streamtx downlink and that a caller may be setting only through an inherited environment. The comment at the site says why it is a warning and not the throw the setter raises, so nobody "fixes" it into a throw later.

One more thing the same question turned up

Checking whether the TSSI path was opt-in — it wasn't — surfaced that this backend was the only one doing register I/O inside send_packet. All four existing HALs score zero on sed -n '/::send_packet(/,/^}/p' | grep -c 'rtw_write|rtw_read|set_bb|write_rf'; this one re-selected the CCK/OFDM thermal-compensation curve on every rate-class crossing, at the 84 ms from #389.

The vendor doesn't do that either: _halrf_tssi_set_tmeter_tbl_8733b is only ever called from full TSSI setup, keyed on phydm_get_tx_rate at that instant, and never re-selected at runtime. So the table is now chosen once per channel set and left alone, with per-frame switching behind DEVOURER_TSSI_RATE_TABLE=1. An alternating CCK/OFDM stream goes from 19.69 s to 2.82 s per 200 frames, 200 switches to 0.

The first cut of this was wrong and the witness caught it. I initially made TSSI itself opt-in, falling back to the flat kSafeTxAgcIndex8733b. On air that path cannot carry HT at all — MCS7, 300/300 submitted, 0 captured, twice. On a TSSI-offset PG unit the loop is the power control, so it stays on; only the table switching is opt-in. With the loop on and the table fixed, MCS7 delivers 277/300 and 1 Mbps CCK 300/300, zero FCS errors.

Fixing the table costs nothing measurable at room temperature — the two curves differ only in the swing ramp away from baseline, so near zero thermal delta they agree. 1 Mbps CCK: 300/300 on the OFDM table, 299/300 on the CCK table.

Refreshed delivery table

Now exact per-frame counts (DEVOURER_RX_DUMP_ALL filtered to the injected frames) rather than the throttled hit-counter floors I reported before, all under the shipped default:

requested delivered decoded as FCS errors
1 Mbps CCK 298 / 300 1 Mbps 0
2 Mbps CCK 300 / 300 2 Mbps 0
5.5 Mbps CCK 300 / 300 5.5 Mbps 0
11 Mbps CCK 294 / 300 11 Mbps 0
6 Mbps OFDM 300 / 300 6 Mbps 0
HT MCS0 270 / 300 MCS0 0
HT MCS7 277 / 300 MCS7 0

Single probes, so ~±3 points each. The HT column sitting under the legacy column is not a finding on this evidence — MCS0 reading below MCS7 is the giveaway, since MCS0 is the more robust rate. I've said so in the doc rather than presenting the ordering as a modulation effect.

Note this supersedes my earlier "11 Mbps deficit is repeatable" claim in the direction of less certainty: with exact counts, 11 Mbps at 294/300 sits inside the same band as 1 Mbps at 298/300, so I'd no longer call it a distinguishable deficit.

53/53, 49/49 RTL8733B-only, 49/49 ASan+UBSan on every commit.

Still not claimed

No SDR — occupied bandwidth, spectral mask, EVM and absolute power remain unmeasured; the witness gives delivery and relative RSSI only. Narrowband TX, the 0bda:b733 combo module and true VBUS-cold boots are unchanged.

@snokvist

Copy link
Copy Markdown
Contributor Author

Small precision note on the send-path claim in my last comment, since it is the kind of thing someone will re-check.

Running that same grep against the merged Rtl8733bDevice.cpp returns 4, not 0:

16:  /* No thermal read here. read_thermal() is 3 RF writes + a 15 us settle ...
75:  if (_tx_submits.fetch_add(1) == 0) {
82:        _device.rtw_read8(0x0207), _device.rtw_read16(0x041a),
83:        _device.rtw_read32(0x0210), _device.rtw_read8(0x0522),
84:        _device.rtw_read16(0x2de0), _device.rtw_read16(0x2de2));

Line 16 is a comment. Lines 82-84 are six reads guarded by _tx_submits.fetch_add(1) == 0 — a deliberate one-shot on the very first submission that dumps FIFO / TXDMA / BB-TX-enable state, which is what made the original TX bring-up debuggable.

So the accurate statement is: steady-state per-frame register I/O is zero, matching the other four generations, with six reads on the first frame of a session only. The grep counts text, not executions. I did not want the parity claim resting on a number that does not reproduce.

@josephnef

Copy link
Copy Markdown
Collaborator

Both of these landed after the merge, so replying here — and one of them collides with something I merged 20 minutes later.

Heads-up: disable_cca is already on master, and your wording is better

I hit the same gap from the docs side and shipped it in 64d77b5 (#395) before reading your comment. InitWrite there emits:

RTL8733B: CCA disable (DEVOURER_DIS_CCA) is not implemented by this backend;
transmitting with carrier-sense enabled

Your 412f149 is not pushed anywhere — master is cccb863 + 64d77b5 — so you will get a conflict in exactly that hunk on your next rebase. Keep yours and overwrite mine: naming both spellings (DEVOURER_DIS_CCA / tuning.disable_cca) is the more useful line, because the env var is what an operator sets and the struct field is what a library caller sets, and a grep for either should find the warning. Same reasoning at the site, same trade, so it is a one-line resolution.

Note #395 also added src/rtl8733b/CLAUDE.md — the subtree file the other HALs have. It currently documents the 84 ms table switch as a per-frame cost, which your change invalidates; that paragraph needs revising in the same PR.

The TSSI finding is the real result here

This is the better catch of the two comments, and the method is why: you checked what the vendor does at the call site rather than reasoning about what the registers permit. _halrf_tssi_set_tmeter_tbl_8733b being reachable only from full TSSI setup, keyed on phydm_get_tx_rate at that instant, is decisive in a way no amount of register inspection would have been — the table was never designed to be re-selected at runtime, so the 84 ms was paying for a mechanism that does not exist upstream. 19.69 s → 2.82 s per 200 frames, 200 switches to 0, effectively closes #389.

The wrong first cut is the part worth writing down. Making TSSI itself opt-in and falling back to the flat AGC index looked like the more conservative choice and was the more destructive one — MCS7 at 0/300, twice, on air. "On a TSSI-offset PG unit the loop is the power control" is a transferable fact about this PG mode, not an RTL8733B curiosity, and it is exactly the class of thing a register-readback test would have called green. The witness earned its keep.

One review question on the shape, not the result: what is DEVOURER_TSSI_RATE_TABLE=1 for now? Your own evidence says the two curves differ only in the swing ramp away from baseline, so at room temperature fixing the table costs nothing measurable (300/300 vs 299/300 is not a difference). If that is the whole story the knob is dead weight and the vendor's behaviour is simply correct. But if the curves genuinely diverge at elevated thermal delta — which is the regime nobody has measured on this part — then the knob is an opt-in for a case that might matter, and it should say so at its declaration in those terms: default is validated at room temperature; whether it is right hot is unmeasured. Either "delete it" or "scope it honestly" is fine; what I would avoid is shipping it as a neutral performance toggle, since that reads as though someone has characterized both settings.

The retraction is the right call

with exact counts, 11 Mbps at 294/300 sits inside the same band as 1 Mbps at 298/300, so I'd no longer call it a distinguishable deficit

Applying the ±3 rule against your own earlier claim, unprompted, is worth more than the original claim was. Same for refusing to read the HT-under-legacy ordering as a modulation effect — MCS0 at 270 sitting below MCS7 at 277 is indeed the giveaway, because it inverts the robustness ordering, which means the spread is measuring something other than the transmitter.

Worth naming what that something probably is: on this evidence the honest suspect is the receiver, not the DUT. tests/ground_station_qualify.sh sweeps the witness's own ladder and refuses the pairing when the test rate sits off its flat part — running it on that RTL8812AU would tell you whether the ~270-277 HT band is the transmitter or the witness's margin. Cheap, and it converts "not a finding" into "attributed".

Grep precision note: accepted, and the claim survives

Good instinct to pre-empt the re-check. For the record the merged CLAUDE.md wording is scoped the way your correction requires — "nothing reads a register per frame on the send path" — so the one-shot first-submission dump guarded by _tx_submits.fetch_add(1) == 0 is not a counterexample to it. Keep that dump; six reads once per session that made the original bring-up debuggable is a good trade, and it is the steady-state number that has to match the other four HALs.


None of the above is pushed yet, so it all wants a PR — happy to review it when it is up.

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