From 7b11dfa5119149a512f4cd43c6525a6e6ac997e1 Mon Sep 17 00:00:00 2001 From: Joseph <162703152+josephnef@users.noreply.github.com> Date: Mon, 3 Aug 2026 12:40:36 +0300 Subject: [PATCH 1/2] Kestrel per-antenna stats: fold CRC-clean frames only + pure physts output param MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two correctness follow-ups from the #355 review. The Kestrel RX loop fed _rxq/_rxpaths for every WIFI frame; the Jaguar generations fold window aggregates only for CRC-clean frames, and a garbled frame's cached physts biases the means and the active-chain classification — gate both on !crc_err. And parse_physts_8852 now clears its output struct on entry, so an is_valid=0 stub or an absent IE reads 0 instead of a reused struct's previous blob (selftested). On-air re-validated (tests/rxpath_perantenna_onair.sh, 8852C RX, ch36, ~4.2k frames): PASS with the gate in place. Co-Authored-By: Claude Opus 4.8 --- src/kestrel/FrameParserKestrel.h | 4 ++++ src/kestrel/RtlKestrelDevice.cpp | 25 +++++++++++++++---------- tests/kestrel_rxparse_selftest.cpp | 9 +++++++-- 3 files changed, 26 insertions(+), 12 deletions(-) diff --git a/src/kestrel/FrameParserKestrel.h b/src/kestrel/FrameParserKestrel.h index cb89964..38f1745 100644 --- a/src/kestrel/FrameParserKestrel.h +++ b/src/kestrel/FrameParserKestrel.h @@ -148,6 +148,10 @@ inline bool parse_physts_8852(const uint8_t *p, size_t len, bool is_8852c, KestrelPhySts &out) { if (p == nullptr || len < 8) return false; + /* Pure output param: the result depends only on this buffer, never on what + * a reused struct held before (an is_valid=0 stub or an absent IE must + * read 0, not a previous blob's value). */ + out = KestrelPhySts{}; out.rssi_avg = static_cast(p[3] >> 1); for (int i = 0; i < 4; i++) out.rssi[i] = static_cast(p[4 + i] >> 1); diff --git a/src/kestrel/RtlKestrelDevice.cpp b/src/kestrel/RtlKestrelDevice.cpp index 64edbd1..36bd7cd 100644 --- a/src/kestrel/RtlKestrelDevice.cpp +++ b/src/kestrel/RtlKestrelDevice.cpp @@ -338,16 +338,21 @@ void RtlKestrelDevice::StartRxLoop(Action_ParsedRadioPacket packetProcessor) { p.RxAtrib.snr[i] = _last_physts.snr[i]; p.RxAtrib.evm[i] = _last_physts.evm[i]; } - /* Feed the windowed RX-quality aggregate (passive rssi-snr floor + - * LinkHealth) with path-A RSSI + the IE01 average SNR (the - * all-paths quantity the passive floor was validated against; - * per-path SNR goes to _rxpaths instead). A frame with no IE_01 - * SNR (snr_avg=0) still counts toward RSSI. */ - if (_last_physts.rssi[0] > 0) - _rxq.add(_last_physts.rssi[0], _last_physts.snr_avg, 0); - /* Per-antenna window means (GetActiveRxPaths). Both dies are 2 RX - * chains; C/D read 0 and are excluded by n_chains. */ - _rxpaths.add(p.RxAtrib.rssi, p.RxAtrib.snr, p.RxAtrib.evm, 2); + /* Window aggregates fold CRC-clean frames only (the Jaguar + * convention): a garbled frame's cached physts would bias the + * means and the active-chain classification. + * + * _rxq: passive rssi-snr floor + LinkHealth, fed path-A RSSI + + * the IE01 average SNR (the all-paths quantity the passive floor + * was validated against; per-path SNR goes to _rxpaths instead). + * A frame with no IE_01 SNR (snr_avg=0) still counts toward RSSI. + * _rxpaths: per-antenna window means (GetActiveRxPaths). Both + * dies are 2 RX chains; C/D read 0 and are excluded by n_chains. */ + if (!f.crc_err) { + if (_last_physts.rssi[0] > 0) + _rxq.add(_last_physts.rssi[0], _last_physts.snr_avg, 0); + _rxpaths.add(p.RxAtrib.rssi, p.RxAtrib.snr, p.RxAtrib.evm, 2); + } p.Data = std::span(const_cast(f.payload), f.payload_len); packetProcessor(p); diff --git a/tests/kestrel_rxparse_selftest.cpp b/tests/kestrel_rxparse_selftest.cpp index 2e1a588..7982d34 100644 --- a/tests/kestrel_rxparse_selftest.cpp +++ b/tests/kestrel_rxparse_selftest.cpp @@ -216,7 +216,8 @@ int main() { CHECK(pb.evm[0] == -50 && pb.evm[1] == -30); /* EVM page read on both */ } - /* --- physts: is_valid=0 keeps header RSSI but skips the IE walk --- */ + /* --- physts: is_valid=0 keeps header RSSI but skips the IE walk; a reused + * output struct is cleared (result depends only on this buffer) --- */ { std::vector b(8 + 32, 0); b[0] = 0x01; /* no is_valid bit */ @@ -225,8 +226,12 @@ int main() { b[8] = 1; b[16] = 30; KestrelPhySts ps; + ps.snr_avg = 44; /* stale garbage from a "previous parse" */ + ps.snr[0] = 33; + ps.evm[1] = -40; CHECK(parse_physts_8852(b.data(), b.size(), true, ps)); - CHECK(ps.rssi[0] == 65 && ps.snr_avg == 0 && ps.snr[0] == 0); + CHECK(ps.rssi[0] == 65 && ps.snr_avg == 0 && ps.snr[0] == 0 && + ps.evm[1] == 0); } /* --- physts: truncated / lying total-length is bounded by the buffer --- */ From 6f9c879827597e4717ec70898cd15d34eb851c91 Mon Sep 17 00:00:00 2001 From: Joseph <162703152+josephnef@users.noreply.github.com> Date: Mon, 3 Aug 2026 14:24:16 +0300 Subject: [PATCH 2/2] parse_physts_8852: clear the output struct on the failure path too The pure-output contract must hold on every return: the clear now precedes the null/length validation, so a too-short buffer can no longer leave a reused struct carrying a previous blob's values (Qodo re-review finding). Co-Authored-By: Claude Opus 4.8 --- src/kestrel/FrameParserKestrel.h | 9 +++++---- tests/kestrel_rxparse_selftest.cpp | 5 +++++ 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/kestrel/FrameParserKestrel.h b/src/kestrel/FrameParserKestrel.h index 38f1745..b4b5c82 100644 --- a/src/kestrel/FrameParserKestrel.h +++ b/src/kestrel/FrameParserKestrel.h @@ -146,12 +146,13 @@ struct KestrelPhySts { inline bool parse_physts_8852(const uint8_t *p, size_t len, bool is_8852c, KestrelPhySts &out) { + /* Pure output param: the result depends only on this call's buffer, never + * on what a reused struct held before — including the failure return (an + * is_valid=0 stub, an absent IE, or a too-short buffer must all leave 0, + * not a previous blob's values), so the clear precedes the validation. */ + out = KestrelPhySts{}; if (p == nullptr || len < 8) return false; - /* Pure output param: the result depends only on this buffer, never on what - * a reused struct held before (an is_valid=0 stub or an absent IE must - * read 0, not a previous blob's value). */ - out = KestrelPhySts{}; out.rssi_avg = static_cast(p[3] >> 1); for (int i = 0; i < 4; i++) out.rssi[i] = static_cast(p[4 + i] >> 1); diff --git a/tests/kestrel_rxparse_selftest.cpp b/tests/kestrel_rxparse_selftest.cpp index 7982d34..e9f19de 100644 --- a/tests/kestrel_rxparse_selftest.cpp +++ b/tests/kestrel_rxparse_selftest.cpp @@ -243,8 +243,13 @@ int main() { KestrelPhySts ps; CHECK(parse_physts_8852(b.data(), b.size(), true, ps)); CHECK(ps.snr_avg == 0); /* torn IE not parsed */ + /* The failure return also clears a reused struct — the pure-output + * contract holds on every path, not just successful parses. */ KestrelPhySts p2; + p2.rssi[0] = 65; + p2.snr_avg = 44; CHECK(!parse_physts_8852(b.data(), 4, true, p2)); /* below header size */ + CHECK(p2.rssi[0] == 0 && p2.snr_avg == 0); } if (failures == 0)