fix(firmware): stop truncating HE20 CSI to half its subcarriers on C6/C5 - #1792
Conversation
EDGE_MAX_SUBCARRIERS is a flat 128, which silently truncates CSI on any part that carries HE20. An ESP32-C6 or C5 reports 256 subcarriers per frame; every buffer sized by this constant holds half of them, and edge_process_frame() returns early on n_subcarriers > EDGE_MAX_SUBCARRIERS, so on those parts the edge pipeline stops entirely -- no log past init and no vitals packet ever reaching the sink. Gated on CONFIG_SOC_WIFI_HE_SUPPORT, the same switch csi_collector.c already uses to pick its rx_ctrl layout, so pre-HE parts keep 128 and do not pay ~3.5 KB of .bss they can never use. Co-Authored-By: claude-flow <ruv@ruv.net>
|
Confirmed real: `EDGE_MAX_SUBCARRIERS` (128) silently truncates HE20 frames from C6/C5, which produce 256 subcarriers — and `process_frame()` just discards everything past 128 with no warning or counter. The fix's mechanism (conditional buffer sizing via `CONFIG_SOC_WIFI_HE_SUPPORT`) is consistent with how `csi_collector.c` already handles the same HE-capable/non-HE split for its own struct layout, so this isn't a new pattern — good. Correctly scoped to HE-capable chips only, so no wasted RAM on S3. One gap: `fuzz_edge_enqueue.c` still hardcodes its own local `EDGE_MAX_SUBCARRIERS 128` and won't exercise the new 256-subcarrier path, so there's no regression test proving a 256-subcarrier frame is actually now accepted end-to-end. Worth adding before merge, or at minimum updating the fuzz harness's local constant so it doesn't silently drift from the real one. |
|
Hardware evidence, for the record now that this has merged. Captured from a real ESP32-C6 ( The premise holds: a C6 does deliver 256-bin HE20 frames.
And with the 256 grid in place, nothing is rejected. The node reports its edge-pipeline counters over the wire:
One correction to the commit message, from the measurement. It says "on C6 every frame was rejected and the whole edge pipeline ... silently did nothing". The C6 actually delivers a mix, and the pooled distribution over 44 sampled callbacks is: So under the old 128 grid roughly 84% of frames were rejected, not 100% — the 64-bin frames always fit and were processed normally. The pipeline was running at about a sixth of its input rather than being dead. That is still a serious bug, and it arguably explains why it went unnoticed for so long: vitals and presence kept producing something, just from a small and biased subset of frames, so nothing downstream looked obviously broken. Two follow-ups from this:
|
…itches on Follow-up to the review on ruvnet#1792. The requested regression test turned up two defects in this PR that the test now guards. **edge_processing.h never included sdkconfig.h.** The header switches EDGE_MAX_SUBCARRIERS on CONFIG_SOC_WIFI_HE_SUPPORT but did not include the file that defines it. Any translation unit that reaches this header without sdkconfig.h already in scope evaluates an undefined identifier in #if as 0, silently selects the 128-bin pre-HE grid on a 256-bin part, and reproduces the exact bug this PR exists to fix -- no error, no warning, a clean build and a dead edge pipeline. It happens to work today only because the .c files that include it pull sdkconfig.h in first. Also repairs mojibake in that comment: two em-dashes had been round-tripped through cp1252 and read "—". **The fuzz target carried a dead copy of the constant.** As flagged, fuzz_edge_enqueue.c defined its own EDGE_MAX_SUBCARRIERS 128 -- but it is referenced nowhere in that file. It exercises the SPSC ring, bounded by EDGE_MAX_IQ_BYTES, and never consults the subcarrier grid. So updating it would not have produced the requested end-to-end coverage; the define was simply dead and free to drift. Removed, with a comment saying where the grid is pinned instead. The real coverage is test_edge_subcarrier_grid.c, built twice against the REAL header because the constant is target-conditional and one compilation can only prove one branch. It asserts the grid matches the target, that a 256-bin HE20 frame clears the guard, that a full-width frame still fits one ring slot (so truncation cannot simply relocate into ring_push's memcpy clamp), and that the pre-HE size is unchanged so fixing C6 costs S3 no .bss. Two deliberate choices worth stating. The test never re-implements the guard predicate -- restating it would recreate precisely the drift hazard that let this bug through. And the HE macro is supplied by a stub sdkconfig.h (test/stubs_he/), never by -D on the command line, because -D would compile the right branch regardless and mask the missing include entirely. The expectation is driven by a test-only EXPECT_HE marker rather than by CONFIG_SOC_WIFI_HE_SUPPORT itself. That is not incidental: the first version of this test used the real macro to choose its own expectations, so when the header failed to see it the test quietly asserted the pre-HE case and passed against the broken header. Running it as a negative control caught that. Verified in espressif/idf:v5.4 -- against the unfixed header the HE build fails three checks naming the missing include; with the fix both builds pass; and the C6 firmware still builds clean. Co-Authored-By: claude-flow <ruv@ruv.net>
…itches on Follow-up to the review on ruvnet#1792. The requested regression test turned up two defects in this PR that the test now guards. **edge_processing.h never included sdkconfig.h.** The header switches EDGE_MAX_SUBCARRIERS on CONFIG_SOC_WIFI_HE_SUPPORT but did not include the file that defines it. Any translation unit that reaches this header without sdkconfig.h already in scope evaluates an undefined identifier in #if as 0, silently selects the 128-bin pre-HE grid on a 256-bin part, and reproduces the exact bug this PR exists to fix -- no error, no warning, a clean build and a dead edge pipeline. It happens to work today only because the .c files that include it pull sdkconfig.h in first. Also repairs mojibake in that comment: two em-dashes had been round-tripped through cp1252 and read "—". **The fuzz target carried a dead copy of the constant.** As flagged, fuzz_edge_enqueue.c defined its own EDGE_MAX_SUBCARRIERS 128 -- but it is referenced nowhere in that file. It exercises the SPSC ring, bounded by EDGE_MAX_IQ_BYTES, and never consults the subcarrier grid. So updating it would not have produced the requested end-to-end coverage; the define was simply dead and free to drift. Removed, with a comment saying where the grid is pinned instead. The real coverage is test_edge_subcarrier_grid.c, built twice against the REAL header because the constant is target-conditional and one compilation can only prove one branch. It asserts the grid matches the target, that a 256-bin HE20 frame clears the guard, that a full-width frame still fits one ring slot (so truncation cannot simply relocate into ring_push's memcpy clamp), and that the pre-HE size is unchanged so fixing C6 costs S3 no .bss. Two deliberate choices worth stating. The test never re-implements the guard predicate -- restating it would recreate precisely the drift hazard that let this bug through. And the HE macro is supplied by a stub sdkconfig.h (test/stubs_he/), never by -D on the command line, because -D would compile the right branch regardless and mask the missing include entirely. The expectation is driven by a test-only EXPECT_HE marker rather than by CONFIG_SOC_WIFI_HE_SUPPORT itself. That is not incidental: the first version of this test used the real macro to choose its own expectations, so when the header failed to see it the test quietly asserted the pre-HE case and passed against the broken header. Running it as a negative control caught that. Verified in espressif/idf:v5.4 -- against the unfixed header the HE build fails three checks naming the missing include; with the fix both builds pass; and the C6 firmware still builds clean. Rebased onto 33a9e90. The one conflict was in test/Makefile against the test_serial_onboarding targets ruvnet#1902 added; the resolution is additive, both targets are kept in .PHONY, all and host_tests. The clean: rule now also removes test_edge_grid_he and test_edge_grid_pre_he, which it never did. Co-Authored-By: claude-flow <ruv@ruv.net>
What is wrong
EDGE_MAX_SUBCARRIERSis 128 (edge_processing.h:36). An ESP32-C6 or C5capturing HE20 produces 256 subcarriers. Everything past 128 is discarded.
The loss is silent. No warning, no error, no counter — the edge pipeline simply
operates on half the channel and reports normally. On an S3 (64-bin HT20) the
cap is never reached, so this only appears on newer silicon, where it looks like
poor sensitivity rather than a truncation.
The fix
Raise the cap to 256 so an HE20 frame survives intact.
Cost
Buffers sized by this constant double. That is the honest trade and worth
stating plainly rather than burying: the alternative is that half of every HE20
capture is thrown away on the hardware most likely to be used for new builds.
Testing
Built for esp32c6 with ESP-IDF v5.4 in the
espressif/idf:v5.4container.