hil: flood the bench with unique BLE addresses, and watch heap - #9
Merged
Conversation
The HIL bench cannot currently reproduce ESPresense#2309, and could not detect
it if it did.
Load: ble_flood.py advertises from a fresh static random address ~40x/sec off a
USB Bluetooth adapter on the bench host. ESPresense fingerprints those by MAC
(ID_TYPE_RAND_STATIC_MAC), so every rotation costs a node a slot against a pool
of 100-200 — the churn of a busy room, compressed. Driven raw over
HCI_CHANNEL_USER so BlueZ is neither required nor able to fight for the adapter.
Detection: hil_monitor.py gains two checks it was missing.
- Restart mid-run (exit 7). Nothing noticed a node going down and coming back,
because every other signal here is satisfied by the fresh boot. An S3
power-cycling every ~6h under the low-heap watchdog passes the current 8h
window clean, which is exactly what happened on #2309.
- Free heap decline (exit 8). /json already reports freeHeap/maxHeap/
fingerprints, so sampling it needs no firmware change. Every sample is
logged, so the run itself answers leak vs fragmentation vs churn — the
question #2309 took months to settle by hand.
Gating: the flood only runs while a HIL step asks for it, via a request file in
the already-mounted /var/lock/woodpecker. A directory rather than one flag
because the four device steps finish at different times, and stale requests
expire so a killed container cannot leave the bench spraying junk MACs at every
ESPresense node in the house.
Checks: test_heap_trend.py runs the real #2309 shapes through the verdict (S3
slide fails, C3 hold passes, transient dip passes); ble_flood.py --selftest
covers HCI framing, static-random address rules, and both directions of the gate
without hardware.
Not yet run against a real adapter — needs the USB dongle passed through to the
bench VM first.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughChangesThe PR adds a raw HCI BLE flood utility with systemd host integration. It extends HIL monitoring with restart detection, heap sampling, trend evaluation, and regression tests. BLE flood utility
HIL monitoring checks
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/hil_monitor.py`:
- Around line 58-63: Update the HEAP_TREND_EDGE comment to state that the
configured number of samples is taken at each end and summarized using the
median, matching the statistics.median behavior in edges().
- Around line 228-236: Update the summary construction in the monitor analysis
function to calculate the sampled span from the timestamps in `samples`, using
the first and last sample times, and pass that span to `format_duration` instead
of the full `duration` argument. Keep the existing heap and fingerprint summary
fields unchanged.
- Around line 187-213: Update heap_sampler’s JSON handling to require
isinstance(doc, dict) before accessing freeHeap, maxHeap, or fingerprints,
matching the guard used by json_endpoint_check. Skip non-dict bodies such as
null and continue sampling without allowing an AttributeError to terminate the
sampler thread.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0fd7d21d-6f31-4826-9d85-c782ab568c76
📒 Files selected for processing (5)
bench/README.mdbench/ble-flood.servicescripts/ble_flood.pyscripts/hil_monitor.pyscripts/test_heap_trend.py
👮 Files not reviewed due to content moderation or server errors (3)
- scripts/ble_flood.py
- bench/ble-flood.service
- bench/README.md
Two corrections found by running this against real hardware. Unique identities: the flood advertised a constant name, and a live node reported two different MACs both as id "name:hil-flood" — ESPresense ranks a name (ID_TYPE_NAME, 35) above a static random address (5), so every advert collapsed onto one logical id. The slot pool still churned, but the id space this exists to exercise did not. The name now carries the full MAC; a short suffix would collide tens of thousands of times over an 8h soak and quietly merge ids again. Sampling: /json only ever carried room/ver/firm, so the heap check would have collected nothing and skipped silently forever. It now reads /json/tele (ESPresense#2430), which is allocation-free and answers under low heap — unlike /json, which refuses with 429 when it cannot afford a 12KB document and would therefore hide the numbers exactly as a node degraded. A missing /json/tele now fails the run rather than skipping. A heap check that samples nothing and still reports PASS is how #2309 survived an 8h soak. Unreachable nodes stay a skip: that is not a firmware fault. Also: send() now waits for each command's completion event and checks status, so a wedged adapter can no longer report thousands of addresses while radiating nothing.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/test_heap_trend.py (1)
61-67: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover propagation to exit code 8.
Lines 61-67 call
heap_verdictdirectly. They verify the returned failure string, but they do not verify that the monitor converts a long-run missing/json/teleresult into exit code 8. Add a lifecycle assertion, or confirm equivalent coverage elsewhere inscripts/hil_monitor.py.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/test_heap_trend.py` around lines 61 - 67, Extend the tests around heap_verdict and the monitor lifecycle to verify that a long-run missing /json/tele result propagates through scripts/hil_monitor.py to exit code 8, not just a failure string. Add a lifecycle assertion or reuse equivalent existing coverage, while preserving the current skip behavior for unreachable nodes and short runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@scripts/test_heap_trend.py`:
- Around line 61-67: Extend the tests around heap_verdict and the monitor
lifecycle to verify that a long-run missing /json/tele result propagates through
scripts/hil_monitor.py to exit code 8, not just a failure string. Add a
lifecycle assertion or reuse equivalent existing coverage, while preserving the
current skip behavior for unreachable nodes and short runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 64566c9b-632d-42bd-a2c4-f3c2828c6cee
📒 Files selected for processing (3)
scripts/ble_flood.pyscripts/hil_monitor.pyscripts/test_heap_trend.py
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/hil_monitor.py
- scripts/ble_flood.py
DTTerastar
added a commit
to ESPresense/ESPresense
that referenced
this pull request
Aug 1, 2026
* ci(hil): request the bench BLE flood during device tests Each step drops a request file in the already-mounted /var/lock/woodpecker and removes it on exit, so ble-flood.service on the bench host advertises unique addresses for the duration of the test and stops afterwards. A directory of per-step requests rather than one on/off flag: the four device steps finish at different times, and the first one out must not cut the flood out from under the other three. Needs ESPresense/firmware-tester#9. * feat(json): add /json/tele reporting freeHeap, maxHeap and fingerprints These three numbers already go out over MQTT telemetry, but HTTP only carried room/ver/firm — so diagnosing a heap complaint meant broker access and correlating graphs by hand, which is how #2309 stayed unexplained for months. With them on an endpoint the three failure modes are distinguishable with curl: freeHeap falling while fingerprints holds steady is a leak, maxHeap falling while freeHeap holds is fragmentation, both moving with the device count is churn. Its own endpoint rather than serializeInfo, for two reasons. Availability: serveJson refuses with 429 when it cannot afford a 12KB document, so heap numbers hung off /json would vanish exactly when the node is in the trouble they describe. serveTele writes a fixed stack buffer and answers at any heap level. Cost: /json is polled by the UI, and serializeInfo runs on every /json and /json/devices request. Putting these there would add a fingerprintMutex acquisition (contending the BLE scan task) and a second walk of the free-block list to every poll. Registered before "/json" because that handler also matches path prefixes — which is how /json/devices reaches serveJson — and the first match wins. Size(false) so a GET reports what is there rather than expiring fingerprints as a side effect of being observed. Costs 170 bytes of flash on esp32c3; RAM unchanged. * Apply suggestion from @DTTerastar * Apply suggestion from @DTTerastar * fix(json): escape room in /json/tele and refuse rather than truncate serveTele hand-formatted its body with snprintf, which was wrong twice over. room is user-set free text (main.cpp: HeadlessWiFiSettings.string("room", ...)), so a quote, backslash or control character in it produced a malformed body — and the body is parsed by machines: Home Assistant, the companion, the HIL monitor. Worse, snprintf silently truncates. An over-long room yielded half a JSON object served as 200, which is precisely the "success that lies" failure this endpoint exists to help people diagnose. It now refuses with 500 instead. Escaping is ArduinoJson's job, not ours — the builder moves to TeleJson.h and uses a StaticJsonDocument, keeping the endpoint off the heap so it still answers when /json cannot. The length check measures the *encoded* size, since escaping can double it. Costs 172 bytes of flash on esp32c3 over the snprintf version; RAM unchanged. Covered by test/test_native_tele: quotes, backslashes, control characters, a key-injection attempt, oversize, escaping growth, the exact-fit boundary and degenerate buffers. Needed an [env:native]; it is not in default_envs, so firmware builds are unaffected (pio run still builds esp32 alone).
CodeRabbit on #9: - heap_sampler: guard that the /json/tele body is a dict before doc.get(). A null/non-object body (the low-heap path) made None.get() raise AttributeError, which isn't in the caught tuple — so the sampler thread died silently exactly when heap was declining, the one thing this check exists to catch. - heap_verdict: report the actual sampled span (from sample timestamps) rather than the full monitor duration, which overstates the window near the 30-min floor. - Fix the HEAP_TREND_EDGE comment: median at each end, not average.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bench cannot currently reproduce ESPresense#2309, and could not detect it if it did. Two halves: generate the load, then actually watch for the failure.
Load —
scripts/ble_flood.pyAdvertises from a fresh static random address ~40x/sec off a USB Bluetooth adapter on the bench host, driven raw over
HCI_CHANNEL_USER(so BlueZ is not required and should not be installed — the kernel hands over the controller exclusively andbluetoothdwould only fight for it).Each advert carries a name containing its own MAC, so every rotation is a distinct identity to ESPresense. That detail came from hardware, not theory — see below.
Detection —
scripts/hil_monitor.py/json/tele(ESPresense#2430) once a minute. Every sample is logged, so a run answers leak vs fragmentation vs churn on its own. Judged only on runs ≥30 min./json/telefails the run. A heap check that silently samples nothing and still reports PASS is how #2309 survived an 8h soak. An unreachable node stays a skip — that is not a firmware fault.What the hardware corrected
Three bugs in the first version, all found by running it on the bench rather than reasoning about it:
name:hil-flood— ESPresense ranksID_TYPE_NAME(35) aboveID_TYPE_RAND_STATIC_MAC(5). The slot pool still churned, but the id space this exists to exercise did not. The name now carries the full MAC; a 3-byte suffix would collide tens of thousands of times over an 8h soak and quietly merge ids again./jsononly ever carriedroom/ver/firm— the heap numbers are in the MQTT telemetry path. It would have collected nothing and skipped silently forever. Hence/json/tele.send()ignored command status. A wedged or unplugged adapter would swallow every HCI command while the flood cheerfully reported thousands of addresses. It now waits for each completion event and checks status.Gating
The flood runs only while a HIL step asks for it, via a request file in the already-mounted
/var/lock/woodpecker. A directory rather than a single flag because the four device steps finish at different times and the first one out must not cut the flood out from under the rest. Stale requests expire (9h) so a hard-killed container cannot leave the bench spraying junk MACs at every ESPresense node in the house.Verified on the bench
USB adapter (Intel 8087:0a2b) passed through to
tsi-ha:A production node in range (
esp32s3-cdc, Bathroom) picked the adverts up and fingerprinted them, which is what caught bug #1 above.The leak case is Garkus98's actual S3 curve from #2309; the healthy case is the C3 from the same report.
--selftestcovers HCI framing, completion-status parsing, address rules and both directions of the gate without hardware. Existingtest_json_check.pystill passes.Still to validate
The monitor changes have not run against a live node end-to-end — that needs this image plus ESPresense#2430 firmware on the bench. Note merging here does not disturb the running HIL:
hil.ymlpinsfirmware-tester:1, which only moves on av1.*release tag, not on a merge tomain.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests