kestrel: ACK responder on its own macid / ADDR_CAM entry

[agustinglez]

Follow-up to #487, review point 5 ("Arming changes injected TX context").

The problem

The 8852B ACK responder registered its station at macid 0 / ADDR_CAM entry 0, which is the context InitWrite programs for injection and that every injected frame's WD points at. Arming rewrote that context and clearing re-ran the bring-up registration over it.

In practice this had no measurable cost: a witness counted the same share of injected frames with the responder armed and unarmed. So this PR is about keeping the injection context intact, not about fixing delivery.

The change

• The responder's station now lives at macid 127 / ADDR_CAM entry 127 (BSSID CAM 1), the last entry of the 0x7f ADDR_CAM search range. That keeps it apart from the injection context at macid 0 / entry 0 and from the low slots peer registrations take by convention (tests/ul_trigger_ap.cpp and kestrelprobe trig register their peer at macid 1 / entry 1). RegisterPeerSta and RegisterBeamformee refuse slot 127 (logged), so a peer and the responder cannot replace each other, and clearing the responder cannot invalidate a peer's entry.
• Clearing changed with it. The 8852B firmware leaves a disconnected station's entry VALID: join_info(disconnect) plus a NO_LINK addr_cam, a role REMOVE, and role CREATE plus NO_LINK all left the entry valid with the responder's address. The old layout never showed this, because the bring-up re-registration rewrote entry 0 wholesale. Clear now disconnects the station in the firmware and clears VALID from the host, the way the vendor's no-firmware mac_upd_addr_cam path writes the CAM. It repeats the clear until the entry has read back empty for 25 ms, and only that counts as disarmed: on a timeout the clear is reported as not taken and the last address the entry held stays recorded for a retry.
• HalKestrel: register_infra_sta takes the macid and CAM indices; new unregister_infra_sta, write_mac_mem32 and clear_addr_cam_entry.
• The header doc-comment, docs/kestrel-ack-responder.md and the AdapterCaps.h evidence are updated.

The 8852C path is unchanged: its firmware still does not commit the entry, and the arm still refuses.

Measured

TX20U Nano (RTL8852BU) as responder, TX50UH (RTL8832CU) injecting and counting the ACKs it receives, ch161, MCS1, one attempt per frame, 10 s per cell, 3 runs per row, on the final version (slot 127):

row	ACKs	per run
armed at Init (rxdemo)	14414/14422	100.0 / 99.9 / 99.9
armed at InitWrite (txdemo)	14376/14417	99.7 / 99.7 / 99.7
retargeted (non-EFUSE MAC)	14419/14432	100.0 / 99.8 / 99.9
old address after the retarget	0/6597	0 / 0 / 0
never armed	0/6575	0 / 0 / 0
after a live disarm	0/6561	0 / 0 / 0
responder transmitting (txdemo armed, carrier sense off)	14308/14437	99.0 / 99.0 / 99.3

Arm A, retarget to B, clear, re-arm A in one session, 3 runs: A 99.9-100% armed, then A 0% and B 99.8-99.9% after the retarget, both 0% after the clear, and A 99.9-100% (B 0%) after the re-arm.

Injection delivery, TX20U txdemo injecting 3000 frames to a TX50UH witness, armed and unarmed: at least 2900/3000 in every cell on this branch and on master.

Full MSVC build and ctest pass (78 tests, 4 skipped as on master).

What this does not show:

• One armed cell in about 55 across development (station at entry 1) read the entry back committed and answered no ACKs. It has not reproduced since; master's layout had 0 such cells in about 40. I don't have an explanation for it yet.
• One unarmed delivery cell heard none of the 3179 frames txdemo sent at the witness. No arm runs in that cell, so this change's code is not on its path; three re-runs read at least 2900/3000.
• In the earlier run of the matrix at entry 1, the responder-transmitting row read 80.6% once, with the injector's own USB sends stalling (up to 1 s per send, 7 failures).
• One bench and one pair of dongles, near field.

[openipc-ai]

/review

[qodo-free-for-open-source-projects]

Code Review by Qodo

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

1. Arming can overwrite the peer station ✓ Resolved

Description

SetAckResponder() registers the responder at macid 1 and address-CAM entry 1, which
RegisterPeerSta() also uses for the associated peer. When both features run on one device,
whichever registration runs last replaces the other's firmware and CAM context; clearing the
responder can then invalidate the peer's entry.

Code

src/kestrel/RtlKestrelDevice.cpp[R543-545]

+constexpr uint8_t kAckMacid = 1;
+constexpr uint8_t kAckAddrCamIdx = 1;
+constexpr uint8_t kAckBssidCamIdx = 1;

Evidence

The new constants and registration target slot 1. The peer path accepts that same slot and writes
its role and address-CAM entry, while an existing caller explicitly registers its associated peer
there.

src/kestrel/RtlKestrelDevice.cpp[539-545]
src/kestrel/RtlKestrelDevice.cpp[579-580]
src/kestrel/HalKestrel.h[355-365]
tests/ul_trigger_ap.cpp[65-66]
tests/ul_trigger_ap.cpp[406-410]
src/kestrel/RtlKestrelDevice.cpp[1074-1081]

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 ACK responder and existing peer registration both use macid 1 and address-CAM entry 1, so either operation can replace the other's context.
## Fix Focus Areas
- src/kestrel/RtlKestrelDevice.cpp[539-545]
- src/kestrel/RtlKestrelDevice.cpp[1074-1081]
- tests/ul_trigger_ap.cpp[65-66]
## Recommended Fix
Allocate nonoverlapping slots for the responder and peer, or enforce slot ownership so registration and clearing cannot overwrite an active peer.

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

---

2. Disarm can forget an active responder ✓ Resolved

Description

disarm_ack_locked() breaks at its 150 ms deadline even when the entry has not stayed empty for the
required 25 ms, then treats a null left as success. If firmware rewrites the entry near that
deadline, the next host clear can produce one empty read before the timeout; the function drops the
recorded responder without completing its stability check.

Code

src/kestrel/RtlKestrelDevice.cpp[R664-667]

+    if (!left && clock::now() - clean_since >= std::chrono::milliseconds(25))
+      break;
+    if (clock::now() - t0 >= std::chrono::milliseconds(150))
+      break;

Evidence

A detected held entry resets clean_since when the loop clears it; the deadline can then break the
loop while the next read is empty but before 25 ms elapses. Only left, not completion of the clean
interval, controls the success return and whether ClearAckResponder retains its address.

src/kestrel/RtlKestrelDevice.cpp[652-680]
src/kestrel/RtlKestrelDevice.cpp[621-630]
src/kestrel/HalKestrel.h[293-309]

Agent prompt

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

## Issue description
A timeout can be reported as a successful disarm after only one empty CAM read, before the required 25 ms observation completes.
## Fix Focus Areas
- src/kestrel/RtlKestrelDevice.cpp[652-675]
- src/kestrel/RtlKestrelDevice.cpp[621-630]
## Recommended Fix
Track whether the full empty-entry interval completed. On timeout, report failure and retain the responder address for a later clear attempt, even if the last read was empty.

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

---

Tip of the day

💡 Did you know, you can switch off images and animations for a plain-text comment

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[josephnef]

Reviewed at 60d79c7. Approving: the slot move and the host-side clear are what the measurements say they are, and the 8852C path is unchanged on the bench.

Bench (this host, no 8852B plugged — two TX50UH / 8832CU and an 8812CU witness only):

• Refusal path on the 8852C (bus 9, ch6), PR build with DEVOURER_ACK_RESPONDER: join_info + upd_addr_cam_sta go out at macid 127 / a_idx 127 / b_idx 1, the read-back refuses ("accepted but did not commit"), the rollback sends join_info(disconnect) + NO_LINK addr_cam at the same slot and reports disarmed (ADDR_CAM entry 127 no longer holds ...), then the configured-arm throw as on master.
• Plain RX A/B, master 036a860 vs PR, 2 × 12 s each on the 8852C: 300/200 vs 300/200 rx.pkt, the same two coex: LTE interface not ready lines on both.
• Injection A/B, 8852C txdemo 3000 frames → 8812CU witness, 2 runs each: master 2907 / 2947, PR 2965 / 2945 of 3000. Inside probe noise, as expected for a path the diff does not touch on the C die.

Nothing here exercises the committed-entry path or the host clear against a firmware that rewrites the entry; those are the author's 8852B rows.

Two low notes on the clear loop, neither blocking:

1. disarm_ack_locked declares success after the entry has read empty for 25 ms since the last clear. The arm's own commit latency was measured 2–20 ms, so the margin over the observed tail is ~5 ms. If a late NO_LINK rewrite lands after the window closes, the clear is reported taken, _ack_responder goes nullopt, and nothing retries — and whether a VALID NO_LINK entry at 127 still elicits ACKs is not measured. A wider window (50 ms costs nothing on a 150 ms bound) would put the margin past the tail rather than on it.
2. The 150 ms bound is checked after the stability check, so an entry that first reads empty after ~125 ms is reported "did not stay clear of" even though it is clean. The failure direction is the safe one (it stays on record and retries), so this is only a spurious warning on a slow host.

Doc nit: "it and RegisterBeamformee now refuse 127" in docs/kestrel-ack-responder.md reads as changelog; the docs here are current-state only, so just "refuse 127".

[agustinglez]

Reviewed at 60d79c7. Approving: the slot move and the host-side clear are what the measurements say they are, and the 8852C path is unchanged on the bench.

Bench (this host, no 8852B plugged — two TX50UH / 8832CU and an 8812CU witness only):

• Refusal path on the 8852C (bus 9, ch6), PR build with DEVOURER_ACK_RESPONDER: join_info + upd_addr_cam_sta go out at macid 127 / a_idx 127 / b_idx 1, the read-back refuses ("accepted but did not commit"), the rollback sends join_info(disconnect) + NO_LINK addr_cam at the same slot and reports disarmed (ADDR_CAM entry 127 no longer holds ...), then the configured-arm throw as on master.
• Plain RX A/B, master 036a860 vs PR, 2 × 12 s each on the 8852C: 300/200 vs 300/200 rx.pkt, the same two coex: LTE interface not ready lines on both.
• Injection A/B, 8852C txdemo 3000 frames → 8812CU witness, 2 runs each: master 2907 / 2947, PR 2965 / 2945 of 3000. Inside probe noise, as expected for a path the diff does not touch on the C die.

Nothing here exercises the committed-entry path or the host clear against a firmware that rewrites the entry; those are the author's 8852B rows.

Two low notes on the clear loop, neither blocking:

1. disarm_ack_locked declares success after the entry has read empty for 25 ms since the last clear. The arm's own commit latency was measured 2–20 ms, so the margin over the observed tail is ~5 ms. If a late NO_LINK rewrite lands after the window closes, the clear is reported taken, _ack_responder goes nullopt, and nothing retries — and whether a VALID NO_LINK entry at 127 still elicits ACKs is not measured. A wider window (50 ms costs nothing on a 150 ms bound) would put the margin past the tail rather than on it.
2. The 150 ms bound is checked after the stability check, so an entry that first reads empty after ~125 ms is reported "did not stay clear of" even though it is clean. The failure direction is the safe one (it stays on record and retries), so this is only a spurious warning on a slow host.

Doc nit: "it and RegisterBeamformee now refuse 127" in docs/kestrel-ack-responder.md reads as changelog; the docs here are current-state only, so just "refuse 127".

Thanks for the review and for running the 8852C side. All three notes are addressed in 9d9d9b8:

•	The clear now waits for 50 ms of empty read-backs instead of 25 ms, so the margin sits past the 2-20 ms commit tail rather than on it.
•	The 150 ms bound only ends the wait while the entry is still held. An entry that reads empty always gets its full window (worst case about 200 ms), so a timeout now always means the entry still holds the address, and the "did not stay clear" warning is gone.
•	docs: "refuse 127".

Re-checked on the 8852B: 3 live disarms at 0 ACKs each (0/2215, 0/2217, 0/2223) with the entry-127 read-back, 2 arm -> retarget -> clear -> re-arm cycles with every phase as expected, armed 4809/4811. Full MSVC build and ctest pass.

You're right that whether a VALID NO_LINK entry at 127 would still elicit ACKs isn't measured; the wider window makes that case much less likely to be reached, but it doesn't measure it.

[josephnef]

Re-approving at 9d9d9b8: the 50 ms stable window, the deadline applying only while the entry is still held, and the doc wording cover the three notes. Re-ran the 8852C refusal cell on this head: arm at macid 127 / entry 127 / BSSID CAM 1 refused on the read-back, rollback reported entry 127 clean, same as 60d79c7.