feat(hil): --dma-bisect for #2309 internal-DMA leak - #11
Conversation
Splits a DIAG_DMA_LEAK soak into two arms on one boot: BLE-flood-only for the first half (arm A baseline), then a 4x /json hammer for the second (arm B, browser-like). Parses the dmaFree serial line the firmware already emits every 5s and reports the internal-DMA slope of each arm — a steeper arm B is the browser-accelerated leak reproduced. Samples tag themselves off the load flag so the A/B split is exact, no device-clock reconciliation. Diagnostic only: never fails the build, and the flag is opt-in so main's HIL runs are unaffected. test_dma_bisect.py covers the slope fit, degenerate inputs, and the arm split. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
WalkthroughChangesThe monitor now supports optional DMA leak diagnostics. It captures DMA samples across BLE-only and DMA bisect diagnostics
🚥 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: 2
🤖 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 291-301: Update dma_load_arm to accept an absolute midpoint
deadline instead of a full duration, and wait only until that deadline before
setting load_on and starting the json_load workers. In main, calculate and pass
the midpoint based on the monitor start time so boot confirmation does not
shorten arm B or prevent it from starting.
- Line 328: Update the `fmt` helper in `hil_monitor` to use a nested function
instead of the assigned lambda, replacing the `fmt = lambda s: ...` pattern with
a local `def fmt(...)` that preserves the same formatting behavior for `None`
and numeric values. Keep the existing call sites and output format unchanged
while satisfying Ruff E731.
🪄 Autofix
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: 02a8a966-7b4a-49b9-bd4f-a93d6ea9785e
📒 Files selected for processing (2)
scripts/hil_monitor.pyscripts/test_dma_bisect.py
| def dma_load_arm(ip, duration, load_on, stop): | ||
| """Idle for the first half of the run (arm A: BLE-flood-only baseline), then flip load_on | ||
| and spawn the /json hammer for the second half (arm B). Samples tag themselves with | ||
| load_on, so the A/B split is exact — no clock reconciliation with the device.""" | ||
| if stop.wait(duration / 2): | ||
| return # run ended before we reached arm B | ||
| load_on.set() | ||
| print(f"[hil] DMA bisect: arm B — {DMA_LOAD_WORKERS}x {DMA_LOAD_PATH} load at midpoint", flush=True) | ||
| for _ in range(DMA_LOAD_WORKERS): | ||
| threading.Thread(target=json_load, args=(ip, stop), daemon=True).start() | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Schedule arm B from the monitor midpoint.
main() starts the monitor duration before boot confirmation. dma_load_arm() waits for half of the full duration after confirmation. Arm B is therefore shorter by the boot time. If boot takes more than half the duration, arm B never starts.
Pass an absolute midpoint deadline to dma_load_arm().
Proposed fix
-def dma_load_arm(ip, duration, load_on, stop):
+def dma_load_arm(ip, arm_b_deadline, load_on, stop):
- if stop.wait(duration / 2):
+ if stop.wait(max(0, arm_b_deadline - time.monotonic())):
return # run ended before we reached arm B- args=(match.group(1), args.duration, dma_load_on, stop_sampling),
+ args=(match.group(1), start + args.duration / 2,
+ dma_load_on, stop_sampling),Also applies to: 471-476
🤖 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/hil_monitor.py` around lines 291 - 301, Update dma_load_arm to accept
an absolute midpoint deadline instead of a full duration, and wait only until
that deadline before setting load_on and starting the json_load workers. In
main, calculate and pass the midpoint based on the monitor start time so boot
confirmation does not shorten arm B or prevent it from starting.
| arm_b = [(t, v) for t, v, load in dma_samples if load] | ||
| sa = _ols_slope_per_hour(arm_a) | ||
| sb = _ols_slope_per_hour(arm_b) | ||
| fmt = lambda s: "n/a" if s is None else f"{s:+.0f} B/h" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Resolve Ruff E731.
The assigned lambda violates the configured Ruff rule. Use a nested function for fmt.
Proposed fix
- fmt = lambda s: "n/a" if s is None else f"{s:+.0f} B/h"
+ def fmt(s):
+ return "n/a" if s is None else f"{s:+.0f} B/h"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fmt = lambda s: "n/a" if s is None else f"{s:+.0f} B/h" | |
| def fmt(s): | |
| return "n/a" if s is None else f"{s:+.0f} B/h" |
🧰 Tools
🪛 Ruff (0.16.1)
[error] 328-328: Do not assign a lambda expression, use a def
Rewrite fmt as a def
(E731)
🤖 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/hil_monitor.py` at line 328, Update the `fmt` helper in `hil_monitor`
to use a nested function instead of the assigned lambda, replacing the `fmt =
lambda s: ...` pattern with a local `def fmt(...)` that preserves the same
formatting behavior for `None` and numeric values. Keep the existing call sites
and output format unchanged while satisfying Ruff E731.
Source: Linters/SAST tools
There was a problem hiding this comment.
Pull request overview
Adds an opt-in DMA leak “bisect” diagnostic mode to the HIL monitor so a single soak can compare internal-DMA decline under two conditions (BLE-only vs BLE + browser-like /json load), and includes a small Python test script to validate the slope fitting and A/B partitioning logic.
Changes:
- Add
--dma-bisectflag tohil_monitor.py, including serial parsing of[DMALEAK]lines, midpoint load generation, and OLS slope reporting for arms A/B. - Add
scripts/test_dma_bisect.pyto validate slope fitting, degenerate inputs, settle-window exclusion, and load-flag arm partitioning.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| scripts/hil_monitor.py | Adds DMA bisect mode: parses DMA serial samples, runs a midpoint /json hammer, and reports A/B slopes. |
| scripts/test_dma_bisect.py | Adds a standalone assertion-based test for slope fitting and correct A/B arm splitting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def json_load(ip, stop): | ||
| """Hammer the heavy /json endpoint to mimic a browser polling it — the 'browser | ||
| accelerates the leak' arm of the #2309 bisect. Errors are swallowed: the point is | ||
| TCP/heap pressure, not correctness (json_endpoint_check already owns that).""" | ||
| while not stop.is_set(): | ||
| try: | ||
| conn = http.client.HTTPConnection(ip, timeout=10) | ||
| conn.request("GET", DMA_LOAD_PATH) | ||
| conn.getresponse().read() | ||
| conn.close() | ||
| except (OSError, http.client.HTTPException): | ||
| pass | ||
| stop.wait(0.25) |
| if args.dma_bisect: | ||
| threading.Thread( | ||
| target=dma_load_arm, | ||
| args=(match.group(1), args.duration, dma_load_on, stop_sampling), | ||
| daemon=True, | ||
| ).start() |
|
@copilot Fix the code for all comments in this review thread. When a review comment includes a suggested change, apply the suggestion exactly. Do not make changes beyond what is described in the linked review thread. Why call it dma? |
…to dma_load_arm Co-authored-by: DTTerastar <1491145+DTTerastar@users.noreply.github.com>
Head branch was pushed to by a user without write access
Fixed in the latest commit:
"Why call it dma?" — the flag targets the |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 289-292: In the cleanup blocks surrounding conn.close(), replace
broad Exception swallowing with targeted OSError handling, and log the captured
close failure at debug level. Preserve swallowing of request-level network
errors, but do not suppress unexpected cleanup exceptions or trigger
BLE001/S110.
🪄 Autofix
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: 9f9283aa-26b6-4150-a3fe-461c97b39339
📒 Files selected for processing (1)
scripts/hil_monitor.py
| if conn is not None: | ||
| conn.close() | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repository files matching hil_monitor.py:\n'
fd -a 'hil_monitor\.py$' . || true
file="$(fd 'hil_monitor\.py$' . | head -n 1 || true)"
if [ -n "${file:-}" ]; then
printf '\nFile: %s\n' "$file"
wc -l "$file"
printf '\nAround lines 260-310:\n'
sed -n '260,310p' "$file" | nl -ba -v260
fi
printf '\nRuff config indicators:\n'
for f in pyproject.toml ruff.toml .ruff.toml setup.cfg; do
[ -e "$f" ] && { printf '\n--- %s ---\n'; sed -n '1,220p' "$f"; }
done
printf '\nRelevant exception context search:\n'
rg -n "conn\.close|close\(|BLE001|S110|RUF100|allow |raise" pyproject.toml ruff.toml .ruff.toml "$file" || trueRepository: ESPresense/firmware-tester
Length of output: 373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd 'hil_monitor\.py$' . 2>/dev/null | head -n 1 || true)"
if [ -n "${file:-}" ]; then
printf 'File: %s\n' "$file"
wc -l "$file"
printf '\nLines 260-310:\n'
sed -n '260,310p' "$file" | awk '{printf "%6d%c", NR+259, 10; print}'
fi
printf '\nRuff config files:\n'
for f in pyproject.toml ruff.toml .ruff.toml setup.cfg tox.ini Makefile; do
if [ -e "$f" ]; then
printf '\n--- %s ---\n' "$f"
sed -n '1,240p' "$f" | awk '{printf "%6d%c", NR, 10; print}'
fi
done
printf '\nTarget exception cleanup snippets:\n'
python3 - <<'PY'
from pathlib import Path
p=Path('scripts/hil_monitor.py')
for line in p.read_text().splitlines():
pass
lines=p.read_text().splitlines()
for start in [280, 295]:
print(f"\n--- {start}-{start+17} ---")
for i in range(start, min(start+18, len(lines))+1):
print(f"{i:5d}: {lines[i-1]}")
PY
printf '\nRelevant search:\n'
(grep -nE "conn\.close|close\(|BLE001|S110|RUF100|allow |\!|ignore|preview|lint|select|extend" pyproject.toml ruff.toml .ruff.toml setup.cfg scripts/hil_monitor.py 2>/dev/null || true)Repository: ESPresense/firmware-tester
Length of output: 5922
Narrow the cleanup exception handling.
Lines 289-292 and 295-299 catch Exception and discard it, which hides unexpected conn.close() failures and triggers Ruff BLE001 / S110. Capture the close cleanup error, such as an OSError, and log it as debug output while keeping request-level network errors swallowed.
🧰 Tools
🪛 Ruff (0.16.1)
[error] 291-292: try-except-pass detected, consider logging the exception
(S110)
[warning] 291-291: Do not catch blind exception: Exception
(BLE001)
🤖 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/hil_monitor.py` around lines 289 - 292, In the cleanup blocks
surrounding conn.close(), replace broad Exception swallowing with targeted
OSError handling, and log the captured close failure at debug level. Preserve
swallowing of request-level network errors, but do not suppress unexpected
cleanup exceptions or trigger BLE001/S110.
Source: Linters/SAST tools
Adds an opt-in
--dma-bisectmode tohil_monitor.pyfor ESPresense#2309.What it does — on a
DIAG_DMA_LEAKbuild, splits one soak into two arms on a single boot:/jsonhammer (browser-like) — does the browser load steepen the decline?It parses the
dmaFreeserial line the firmware already prints every 5s (serial, not/json/tele, so it keeps reporting even when the heap is too low to serve). Samples tag themselves off the load flag → exact A/B split. Reports the OLS slope (B/h) of each arm; a steeper arm B is the browser-accelerated leak reproduced.Safety — diagnostic only: never fails the build; flag is opt-in so main's HIL runs are unaffected.
test_dma_bisect.pycovers slope fit, degenerate inputs, and the arm split.Deploy note — the ESPresense S3 HIL step pulls
firmware-tester:1; that tag only rebuilds on av1.x.xrelease. This flag must ship in:1before the ESPresense diag PR's S3 step invokes it.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests