rtl8733b: harden ARQ and qualify BlockAck response - #407
Conversation
PR Summary by QodoHarden RTL8733B ARQ and qualify BlockAck responses
AI Description
Diagram
High-Level Assessment
Files changed (18)
|
Code Review by Qodo
1. cleanup skips responder teardown
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| cleanup() { | ||
| sudo pkill -9 -f "^$ESC_BUILD/rxdemo" 2>/dev/null | ||
| sudo pkill -9 -f "^$ESC_BUILD/txdemo" 2>/dev/null |
There was a problem hiding this comment.
1. cleanup skips responder teardown 📘 Rule violation ☼ Reliability
The soliciting-TX ARQ harness runs the responder-ON phase first and ends it with SIGKILL, preventing Halmac8733bMac::stop() from explicitly disarming the persistent hardware ACK gate before the OFF phase. The supposedly responder-off measurement can therefore still receive ACKs from an armed radio, collapsing retries, violating orderly device teardown, and invalidating the A/B result.
Agent Prompt
## Issue description
The ARQ harness kills the ON-phase `rxdemo` process with `SIGKILL`, bypassing the RTL8733B stop path that clears the persistent hardware ACK-responder gate. The subsequent OFF measurement may therefore still receive ACKs from the previously armed adapter.
## Issue Context
The harness runs the armed responder phase before the OFF control, and its common cleanup directly kills the responder without resetting or disarming the adapter. Use graceful receiver shutdown and verify responder disarm before accepting the OFF control, following the established BlockAck harness pattern; reserve `SIGKILL` for fallback cleanup after the measurement has already been rejected. The BlockAck harness documents that killing the process cannot guarantee passive state because `SIGKILL` cannot execute device stop logic, and Jaguar1 performs the relevant hardware deinitialization from `Stop()`.
## Fix Focus Areas
- tests/rtl8733b_arq_tx_onair.sh[34-39]
- tests/rtl8733b_arq_tx_onair.sh[55-95]
- tests/rtl8733b_arq_tx_onair.sh[130-131]
- tests/rtl8733b_blockack_onair.sh[91-114]
- src/jaguar1/RtlJaguarDevice.cpp[1998-2009]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| /* A failed status does not prove the gate write had no side effect. Make a | ||
| * best-effort close before reporting failure; RTL8733B additionally reads | ||
| * this back in its caller. */ | ||
| (void)dev.rtw_write8(0x0102, static_cast<uint8_t>(nt & ~0x03u)); |
There was a problem hiding this comment.
2. Failed arm may stay active 🐞 Bug ☼ Reliability
ack::enable() returns failure after only a best-effort, unverified gate clear, while Jaguar1/2/3 callers trust that result and perform no verified rollback. If the failed arm write landed and the cleanup write did not, the API or constructor reports refusal while the radio remains an active ACK/BlockAck responder.
Agent Prompt
## Issue description
The shared ACK-responder arm helper can return `false` without proving that the net-type gate is closed. Jaguar callers do not add the verified rollback used by RTL8733B, so a failed arm can leave the radio responding after the caller was told it is passive.
## Issue Context
The transport write result is not sufficient to establish register state, as the new code itself notes. This applies both when the initial gate-close write fails and when the final arm write reports failure after potentially taking effect.
## Fix Focus Areas
- src/AckResponder.h[43-68]
- src/jaguar1/RtlJaguarDevice.cpp[773-775]
- src/jaguar2/RtlJaguar2Device.cpp[383-385]
- src/jaguar3/RtlJaguar3Device.cpp[2174-2176]
- src/rtl8733b/Rtl8733bDevice.cpp[844-874]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if ! stop_receivers; then | ||
| echo "ABORT: phase=$phase receivers did not stop gracefully" >&2 | ||
| cleanup | ||
| exit 1 | ||
| fi | ||
| cleanup |
There was a problem hiding this comment.
3. Final disarm goes unchecked 🐞 Bug ☼ Reliability
After each BlockAck phase, the harness only checks that receiver processes exit; it never checks the responder's stop-time disarm warning as it does during the preparatory passive reset. Since Halmac8733bMac::stop() catches a failed verified clear and continues shutdown, the test can pass while leaving the responder gate active and while its claimed shutdown verification actually failed.
Agent Prompt
## Issue description
The BlockAck harness validates the preparatory passive reset but does not inspect stop-time disarm results after the OFF and ON phases. A process can exit gracefully even when the verified responder clear failed, allowing a passing test to leave active radio state behind.
## Issue Context
`Halmac8733bMac::stop()` intentionally logs and suppresses disarm failures so the rest of teardown continues. Process exit therefore cannot serve as proof that the gate was cleared.
## Fix Focus Areas
- tests/rtl8733b_blockack_onair.sh[79-114]
- tests/rtl8733b_blockack_onair.sh[179-195]
- src/rtl8733b/Halmac8733bMac.cpp[811-826]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| nonzero_bitmaps = sum(ba.get("bitmap", "0" * 16) != "0" * 16 | ||
| for ba in blockacks) | ||
| compressed = sum(bool(int(ba.get("ctrl", 0)) & 0x4) for ba in blockacks) |
There was a problem hiding this comment.
4. Blockack properties are disconnected 🐞 Bug ≡ Correctness
The harness counts compressed BlockAcks and nonzero bitmaps independently, then requires only that each total exceed 100. A mix of compressed frames with zero bitmaps and other addressed frames with nonzero bitmap bytes can therefore pass without proving that any compressed BlockAck carries a nonzero bitmap.
Agent Prompt
## Issue description
The verdict combines separate compressed-frame and nonzero-bitmap totals, so the two required properties may come from different BlockAck events. This weakens the claimed proof that addressed compressed BlockAcks contain meaningful bitmaps.
## Issue Context
Each decoded event already contains both `ctrl` and `bitmap`, allowing the witness analysis to count their conjunction directly and optionally reject unexpected BlockAck variants.
## Fix Focus Areas
- tests/rtl8733b_blockack_onair.sh[215-224]
- tests/rtl8733b_blockack_onair.sh[251-263]
- tests/rtl8733b_blockack_onair.sh[286-302]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
josephnef
left a comment
josephnef
left a comment
There was a problem hiding this comment.
Reviewed the ARQ hardening + BlockAck qualification. The goal is right — no silently dead responder, a verified ACK window — but the hardening contradicts its own stated contracts in three places (throw-on-write-status vs readback-is-authority, fail-passive with a stale-responder hole, a bool error contract the read path bypasses), and the three on-air harnesses have already drifted apart from each other inside this one PR. Details inline; one finding is outside the diff:
examples/chanmig/main.cpp:576 — the chanmig drone discards SetAckResponder's bool. With this PR's new deliberate fail-passive paths (enable() now closes the gate and returns false on a failed write), a single transient control-transfer failure at drone bring-up leaves the radio passive by design while chanmig proceeds anyway. The ground side's hardware-ARQ frames all retry to the limit against a silent responder and the operator debugs the RF link — the exact silent-dead-responder scenario the 8733B comments say this hardening exists to prevent, unhandled at the one in-tree runtime call site.
| } | ||
| _logger->info( | ||
| "RTL8733B: ACK window {} us (REG_ACKTO 0x640 + CCK 0x639, verified)", | ||
| ackto_want); |
There was a problem hiding this comment.
bring_up_to_phy throws on !ackto.writes_ok even when both ACK-window readbacks equal the wanted value — contradicting this PR's own readback-is-the-authority rule (AckResponder.h's disable_verified comment calls a failed-status-but-landed write realistic). A transient control-transfer status error becomes a hard bring-up abort: set_ack_timeout_us returns writes_ok=false with non_cck==cck==wanted, and a session with verified-correct register state refuses to start.
The same warn→throw escalation also applies to pure monitor-RX bring-up, where the ACK window doesn't matter — and the claim that REG_ACKTO_CCK 0x639 always latches rests on one f72b unit. Pre-PR behavior was warn-and-continue. Suggest: throw only when the readback disagrees; treat write status as diagnostic.
|
|
||
| void RtlJaguarDevice::ClearAckResponder() { | ||
| devourer::ack::disable(_device); | ||
| if (!devourer::ack::disable(_device)) { |
There was a problem hiding this comment.
ClearAckResponder now throws std::runtime_error from a void, teardown-shaped API on a raw write status — on all four backends (also RtlJaguar2Device.cpp:394, RtlJaguar3Device.cpp:2185, Rtl8733bDevice.cpp:912) — while this same PR treats the identical failure as swallow-and-warn in Halmac8733bMac::stop() because "teardown is best-effort after disconnect".
A consumer disarming the responder during shutdown after a USB error/unplug (the common reason the 0x0102 write fails) now gets a throw; in a destructor or unwind path that's std::terminate. And per AckResponder.h's own comment, a failed write status doesn't prove the write missed — so the throw can fire on a successfully closed gate.
| /* Close the gate before changing identity. Besides avoiding a transient | ||
| * responder for a half-written MAC during retargeting, this makes every | ||
| * failed identity write leave the radio passive. */ | ||
| if (!dev.rtw_write8(0x0102, static_cast<uint8_t>(nt & ~0x03u))) |
There was a problem hiding this comment.
Fail-passive has a hole on re-arm: if enable()'s first write (the gate close, rtw_write8(0x0102, nt & ~0x03)) fails during a retarget, it returns false having changed nothing — gate still open, MACID still the old MAC. The radio keeps SIFS-ACKing MAC A while SetAckResponder(B) reports failure, and the comment claims every failure leaves the radio passive. On Jaguar2 (no teardown power-down) the stale responder answers until re-enumeration.
Only Rtl8733bDevice attempts disable_verified rollback on enable()==false; the three Jaguar callers just log "not armed". Either roll back in enable() itself or in all four callers.
| ((uint32_t)mac[3] << 24)); | ||
| dev.rtw_write16(0x061c, (uint16_t)(mac[4] | (mac[5] << 8))); | ||
| inline bool enable(RtlAdapter &dev, const uint8_t mac[6]) { | ||
| const uint8_t nt = dev.rtw_read8(0x0102); |
There was a problem hiding this comment.
The bool error contract only covers writes. enable()/disable()/is_disabled() all begin with rtw_read8(0x0102), which throws std::ios_base::failure on a failed transfer (UsbTransport.h:167) — read failures bypass the return-false/rollback/log paths entirely. A timed-out 0x0102 read inside ack::enable propagates uncaught out of SetAckResponder (and out of Init/InitWrite as a different exception type than the intended runtime_error).
Likewise (void)disable(dev) inside disable_verified is not actually best-effort — its internal read can throw before is_disabled runs — and is_disabled's "a control transfer may report failure" comment is unreachable for reads: they never report, they throw.
| on, off = rows["on"], rows["off"] | ||
| on_c = on["copies_per_observed_frame"] | ||
| off_c = off["copies_per_observed_frame"] | ||
| ok = (on["coverage"] >= 0.60 and off["coverage"] >= 0.60 and |
There was a problem hiding this comment.
The verdict band is arithmetically unsatisfiable at the guard's own floor: the guard admits RETRY_LIMIT 3..63, but the verdict requires both off_c <= 1.15*(limit+1) and off_c >= 5*on_c. At RETRY_LIMIT=3, perfect hardware gives on_c >= 1.0 (copies per observed frame is ≥1 by construction), so the verdict demands off_c >= 5.0 while the band caps it at 4.6 — contradiction, exit 1 on every healthy run (and it's near-unsatisfiable at 4). The sibling rtl8733b_blockack_onair.sh floors RETRY_LIMIT at 5 for exactly this reason — floor this guard the same way (or derive the floor from the band arithmetic).
| tail -1 "$RESULTS" | ||
| } | ||
|
|
||
| run_phase on 1 |
There was a problem hiding this comment.
The responder-ON phase runs before the OFF control arm and teardown is pkill -9, which skips Stop() — Jaguar1's responder disarm lives only in Stop()/the destructor, and its own comment says the chip "stays in ACT with its RF front end live" after the owner dies. So the 8812AU's hardware responder (net_type=AP + MACID in silicon) can stay armed into the OFF arm: if the orphaned MAC still ACKs RESP_MAC, off_c collapses toward 1 and the run FAILs on healthy hardware. The sibling blockack script added force_responder_passive() and runs off-before-on for exactly this SIGKILL hazard; this script needs both.
| sudo env DEVOURER_VID="$DUT_VID" DEVOURER_PID="$DUT_PID" \ | ||
| DEVOURER_CHANNEL="$CH" DEVOURER_TX_QOS_DATA=1 \ | ||
| DEVOURER_TX_RA="$RA" DEVOURER_TX_SA="$TX_SA" \ | ||
| DEVOURER_TX_RATE="$RATE" DEVOURER_TX_PAYLOAD_BYTES=200 \ |
There was a problem hiding this comment.
Airings are counted with grep -c on rx.seq with no CRC filter, while both sibling harnesses in this PR discard crc-flagged copies — and the documented witness for these cells (RTL8812CU, Jaguar3) keeps FCS-error frames unconditionally (ACRC32|AICV set in monitor bring-up). A marginal-RF arm delivering crc-error duplicates inflates airings_per_frame past the 1.15*expected ceiling and FAILs the dose-response on healthy hardware (or masks a real shortfall at the floor). The three harnesses silently measure different quantities while src/rtl8733b/CLAUDE.md describes them as one method — filter crc here like the siblings do.
| exit 2 | ||
| fi | ||
|
|
||
| ESC_BUILD=$(printf '%s' "$BUILD" | sed 's/[][\.*^$/]/\\&/g') |
There was a problem hiding this comment.
ESC_BUILD's sed class [][\.*^$/] escapes sed/BRE metachars but not the ERE metachars + ? ( ) { } | that pkill -f matches with. A BUILD path containing any of them (e.g. /home/user/devourer-g++/build: the + survives unescaped, anchored pattern never matches — verified with grep -E) makes cleanup()/stop_receivers() silently kill nothing, stderr discarded — the previous arm's receiver keeps the adapter claimed into the next arm, which is the cross-arm contamination this harness rewrite exists to prevent. Same construct in tests/rtl8733b_arq_tx_onair.sh:33 and tests/rtl8733b_retry_limit_onair.sh; either escape the ERE set too or match on pkill -f -x-safe fixed strings.
| re-armed on a **different** MAC, 1728/1728 again (the address is arbitrary, not | ||
| baked in); disarmed, 0/1723 successful reports with retries pinned at 12. | ||
|
|
||
| The other direction is now measured independently rather than inferred. |
There was a problem hiding this comment.
Historical/changelog framing in current-state docs: "is now measured independently rather than inferred" (here), "BlockAck response is now separately measured" / "The replacement tests/rtl8733b_blockack_onair.sh" (:337, :341), "The corrected harness uses..." (:359), and docs/rtl8733b.md:399 "avoiding the old sampled/quantized rx.txhit readout" — each references a deleted past state a future reader must reconstruct from git. Repo convention is current-state only (git is the changelog): "is measured independently by tests/..." carries the same facts without the narration.
2 participants
Follow-up to #406 after its merge.
Summary
tx.ack_timeout_usto both RTL8733B response-window registers (REG_ACKTOandREG_ACKTO_CCK) and verify both readbacks.0x94compressed BlockAck frames, including the bitmap.Hardware evidence
Current rig:
0bda:b812, RTL8822B/88x2BU, Jaguar20bda:f72b0bda:8812, RTL8812AU/Jaguar1BlockAck, ch36/MCS3/retry 12 (
tests/rtl8733b_blockack_onair.sh):paggr=0.665, max burst 9, 0 matching BlockAcks;paggr=1.0, max burst 9, 14,402 addressed compressed BlockAcks; all 14,402 carried nonzero bitmaps;Other reruns:
0/3/12/0/12: 1.00/4.00/12.34/1.00/12.34 copies per frame; verdict true.Checks
ctest --test-dir build --output-on-failure: 54/54 pass.bash -nandshellcheckpass for all three RTL8733B on-air ARQ harnesses.git diff --checkpasses.Scope
This qualifies RTL8733B as a BlockAck responder only on the measured Jaguar2-to-RTL8733B combination. RTL8733B A-MPDU TX remains unported and unmeasured. CCX/
tx.reportremains unimplemented pending a separately tested H2C queue plusMEDIA_STATUS_RPTregistration path.