Fix active control breaking after invalid grid readings (NaN/Inf)#550
Merged
Conversation
A single NaN/Inf grid reading fed into the active-control loop poisoned the adaptive grid-state predictor permanently: the NaN innovation never clears the trust gate, so no later meter sample could correct the estimate, and the ramp-pacing clamp turned the resulting NaN reading into a constant +pace_base_step discharge command for every battery until restart. On the ESPHome port this is trivially reachable — sensors publish NAN for "unavailable" and filter chains propagate it (the reporter's own config produces one on every boot via a circular template pair). The Python stack is exposed the same way by any source that yields a non-finite float. Both handlers now route a non-finite reading through the same zero-delta hold path as an unavailable meter (issue #403): the battery holds its output for that poll, the stateful controller never sees the sample, and control resumes with the next finite reading. The ESPHome sensor cache additionally drops non-finite publishes so the last good value bridges a transient gap (aging out into the meter-unavailable path on a persistent outage) and NaN never reaches the Marstek MQTT / cloud reporting readers. Regression coverage: a shared e2e scenario (both backends) asserting the zero hold and post-recovery steering, plus Python unit tests for the handler path and the finiteness helper. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018xWd3CJyRa95VpASh3Sn7S
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughCT002 now treats non-finite meter readings as unavailable, holds battery output for that poll, avoids caching invalid values, and resumes normal control after valid readings return. Python and ESPHome implementations include validation, regression tests, and changelog documentation. ChangesCT002 invalid meter recovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 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 |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018xWd3CJyRa95VpASh3Sn7S
Contributor
Steering evaluation (base vs head)Overall: 0 improved, 0 regressed, 15 unchanged across 15 metrics — mean 0% (unchanged). Priority: priority-weighted 0% (unchanged) — ✅ no do-no-harm guardrail regressions. Lower is better for every metric. See Metrics are the per-scenario mean of 5 seeds. Aggregate — mean across 32 scenarios
📊 Interactive grid-power charts (zoom / hover / toggle series) are in the self-contained What do these metrics mean?
Per-scenario tables (32 scenarios)full_battery_low_pace — settle 0.0→0.0s, overshoot 0.0→0.0W, RMS 29.1→29.1W
mixed_cadence/eff — settle 50.8→50.8s, overshoot 164.5→164.5W, RMS 21.9→21.9W
mixed_cadence/fair — settle 47.7→47.7s, overshoot 70.4→70.4W, RMS 12.9→12.9W
mixed_cadence_solar/eff — settle 52.5→52.5s, overshoot 384.7→384.7W, RMS 28.6→28.6W
mixed_cadence_solar/fair — settle 56.0→56.0s, overshoot 75.4→75.4W, RMS 23.7→23.7W
mixed_venus_b2500/eff — settle 81.2→81.2s, overshoot 221.9→221.9W, RMS 18.6→18.6W
mixed_venus_b2500/fair — settle 75.4→75.4s, overshoot 231.1→231.1W, RMS 22.3→22.3W
phase_imbalance — settle 53.4→53.4s, overshoot 145.2→145.2W, RMS 30.3→30.3W
single_venus_d_solar — settle 24.2→24.2s, overshoot 94.4→94.4W, RMS 15.9→15.9W
single_venus_d_steps — settle 26.3→26.3s, overshoot 90.3→90.3W, RMS 15.5→15.5W
single_venus_d_washer — settle 0.0→0.0s, overshoot 0.0→0.0W, RMS 61.0→61.0W
single_venus_drain — settle 0.0→0.0s, overshoot 0.0→0.0W, RMS 907.3→907.3W
single_venus_fill — settle 360.0→360.0s, overshoot 0.0→0.0W, RMS 953.6→953.6W
single_venus_noisy — settle 0.0→0.0s, overshoot 0.0→0.0W, RMS 94.3→94.3W
single_venus_pv — settle 0.0→0.0s, overshoot 0.0→0.0W, RMS 60.8→60.8W
single_venus_solar — settle 26.8→26.8s, overshoot 80.3→80.3W, RMS 17.8→17.8W
single_venus_solar_slow — settle 33.9→33.9s, overshoot 68.3→68.3W, RMS 22.8→22.8W
single_venus_steps — settle 26.0→26.0s, overshoot 88.0→88.0W, RMS 14.7→14.7W
single_venus_steps_slow — settle 40.5→40.5s, overshoot 98.5→98.5W, RMS 14.8→14.8W
single_venus_trace — settle 0.0→0.0s, overshoot 0.0→0.0W, RMS 278.9→278.9W
single_venus_washer — settle 0.0→0.0s, overshoot 0.0→0.0W, RMS 61.0→61.0W
two_venus/eff — settle 18.1→18.1s, overshoot 126.1→126.1W, RMS 14.0→14.0W
two_venus/fair — settle 18.4→18.4s, overshoot 116.7→116.7W, RMS 13.8→13.8W
two_venus_noisy/eff — settle 0.0→0.0s, overshoot 0.0→0.0W, RMS 94.3→94.3W
two_venus_noisy/fair — settle 0.0→0.0s, overshoot 0.0→0.0W, RMS 94.2→94.2W
two_venus_slow/fair — settle 41.8→41.8s, overshoot 174.5→174.5W, RMS 14.0→14.0W
two_venus_solar/eff — settle 26.0→26.0s, overshoot 396.6→396.6W, RMS 20.4→20.4W
two_venus_solar/fair — settle 25.9→25.9s, overshoot 151.4→151.4W, RMS 20.4→20.4W
two_venus_trace/eff — settle 0.0→0.0s, overshoot 0.0→0.0W, RMS 283.1→283.1W
two_venus_trace/fair — settle 0.0→0.0s, overshoot 0.0→0.0W, RMS 282.1→282.1W
venus_d_plus_c/eff — settle 20.1→20.1s, overshoot 128.9→128.9W, RMS 14.7→14.7W
venus_d_plus_c/fair — settle 21.6→21.6s, overshoot 121.0→121.0W, RMS 14.6→14.6W
📊 Open the interactive report — |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/astrameter/ct002/ct002.py`:
- Around line 84-95: Update _values_finite to also catch OverflowError alongside
TypeError and ValueError when coercing values with float(), ensuring oversized
numeric inputs return False and follow the hold path.
🪄 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
Run ID: b070c783-4b7d-435c-b843-14f31e8ab89c
📒 Files selected for processing (5)
CHANGELOG.mdesphome/components/ct002/ct002.cppsrc/astrameter/ct002/ct002.pysrc/astrameter/ct002/ct002_test.pytests/components/ct002/test_shared_e2e.py
float() of an oversized int (e.g. a JSON source delivering 10**400) raises OverflowError, which escaped the finiteness check and turned the poll into a logged handler error instead of the zero-delta hold. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018xWd3CJyRa95VpASh3Sn7S
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.
Summary
Fixed a critical bug where a single invalid grid reading (NaN or Inf value) would permanently break active control until restart. The controller would get stuck sending every battery a constant small discharge command regardless of actual grid state.
Root Cause
When a non-finite meter value (NaN/Inf) reached the stateful controller, it would:
pace_base_stepdischarge commandThis commonly occurred with ESPHome sensors that publish
NANfor unavailable readings or filter chains that propagate invalid values.Changes
Python (
ct002.py): Added_values_finite()helper to detect non-finite readings (NaN/Inf/unparseable values). When detected in_handle_request(), treat them like unavailable meters — setmeter_failed=Trueand use zero values instead of feeding them to the controller.C++ (
ct002.cpp): Mirrored the Python logic:cachelambda to skip non-finite values without updating the timestamp, allowing the last good value to bridge transient gapsmeter_okcheck to validate all three phase values for finiteness before deciding whether to use them or fall back to zeroTests:
test_values_finite_helper()covering edge cases (NaN, Inf, non-numeric strings, None)test_non_finite_reading_holds_and_control_recovers()verifying the hold-and-recover behaviortest_nan_meter_reading_holds_then_control_recovers()across all backendsChangelog: Documented the fix for users
Implementation Details
Invalid readings now follow the same zero-delta hold path as unavailable meters, leaving the stateful controller untouched so control resumes normally with the next finite reading. The timestamp is not updated on invalid reads, allowing the last good value to age out naturally into the meter-unavailable path if the source remains broken.
Maintains Python ↔ C++ parity as required by the project conventions.
https://claude.ai/code/session_018xWd3CJyRa95VpASh3Sn7S
Summary by CodeRabbit