feat(binary_sensor): battery out-of-spec alert with hybrid debounce (#78)#116
Conversation
) Adds a single plant-level binary_sensor (device_class: problem) that trips when any cell voltage (LFP 3.0-3.5 V band) or cell-group temperature (0-50 C) across any pack has been out of spec for both >= 5 minutes wall-clock AND >= 3 distinct polls. The dual debounce comfortably exceeds the ~2-minute dongle bad-read persistence (modbus#78), so transient garbage can't trip it, while surviving both fast and slow pollers. Unpopulated cell slots (~0 V) and dropped reads are excluded. Attributes enumerate the current offenders for adaptive automations. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 32 minutes and 10 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new binary sensor platform to monitor and alert on out-of-spec battery cell voltages and temperatures, utilizing a hybrid debounce mechanism to filter out transient bad reads. The review feedback identifies a critical issue where transient dropped reads (which return None) mistakenly reset the debounce state machine because they are skipped during evaluation. To address this, the reviewer suggests only clearing offenders when they are explicitly read and confirmed to be back in spec, and provides a corresponding test case to verify this behavior.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| current: dict[str, _Reading] = { | ||
| f"{r.battery}:{r.metric}": r | ||
| for r in _iter_readings(self.coordinator.data.batteries) | ||
| if _out_of_spec(r) | ||
| } | ||
| # Drop anything that has returned to spec. | ||
| for key in list(self._offenders): | ||
| if key not in current: | ||
| del self._offenders[key] | ||
| # Record/advance current offenders. | ||
| for key, reading in current.items(): | ||
| existing = self._offenders.get(key) | ||
| if existing is None: | ||
| self._offenders[key] = _Offender( | ||
| battery=reading.battery, | ||
| metric=reading.metric, | ||
| value=reading.value, | ||
| first_seen=refresh, | ||
| ) | ||
| else: | ||
| existing.value = reading.value | ||
| existing.poll_count += 1 |
There was a problem hiding this comment.
Issue: Transient dropped reads (None) reset the debounce state machine
In the current implementation, any dropped read (None) or unpopulated slot (which are skipped in _iter_readings) will not be present in current. As a result, key not in current evaluates to True, and the offender is immediately deleted from self._offenders.
This means a single transient dropped read (which is common with flaky Modbus dongles) will completely reset the debounce counter for an active excursion, potentially preventing the alert from ever tripping or causing it to flap.
Solution
Instead of clearing offenders that are missing from current, we should only clear them if they are successfully read and confirmed to be back in spec. If a reading is missing (e.g., due to a dropped read), we should preserve the offender's state without incrementing its poll count.
readings = {
f"{r.battery}:{r.metric}": r
for r in _iter_readings(self.coordinator.data.batteries)
}
# Drop anything that has returned to spec.
for key in list(self._offenders):
if key in readings and not _out_of_spec(readings[key]):
del self._offenders[key]
# Record/advance current offenders.
for key, reading in readings.items():
if _out_of_spec(reading):
existing = self._offenders.get(key)
if existing is None:
self._offenders[key] = _Offender(
battery=reading.battery,
metric=reading.metric,
value=reading.value,
first_seen=refresh,
)
else:
existing.value = reading.value
existing.poll_count += 1| entity, coordinator = _entity([bat]) | ||
| for n in range(5): | ||
| _poll(entity, coordinator, BASE + timedelta(seconds=n * DEBOUNCE_SECONDS)) | ||
| assert entity.is_on is False |
There was a problem hiding this comment.
Improvement: Add test for transient dropped reads
To ensure that transient dropped reads (None) do not reset the debounce state machine, we should add a test case verifying that the offender state and poll count are preserved when a None reading is encountered during an active excursion.
| assert entity.is_on is False | |
| assert entity.is_on is False | |
| def test_transient_none_reading_does_not_reset_debounce(): | |
| bad = [_battery(cells=[2.5] + [3.30] * 15)] | |
| dropped = [_battery(cells=[None] + [3.30] * 15)] | |
| entity, coordinator = _entity(bad) | |
| _poll(entity, coordinator, BASE) | |
| assert entity._offenders["BT1:cell_01_voltage"].poll_count == 1 | |
| # A dropped read should preserve the offender and its poll count | |
| _poll(entity, coordinator, BASE + timedelta(seconds=150), batteries=dropped) | |
| assert "BT1:cell_01_voltage" in entity._offenders | |
| assert entity._offenders["BT1:cell_01_voltage"].poll_count == 1 | |
| # The next out-of-spec read resumes the counter | |
| _poll(entity, coordinator, BASE + timedelta(seconds=300), batteries=bad) | |
| assert entity._offenders["BT1:cell_01_voltage"].poll_count == 2 |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Python |  |
Jun 3, 2026 11:37p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
1 participant
Summary
binary_sensor(device_class: problem) —battery_out_of_spec— rather than per-cell/per-metric sensors, so a consumer reads one entity and drills into its attributes.None) reads are excluded from the checks. Attributes enumerate the current offenders (battery, metric, value, polls, seconds out of spec).Notes
Test plan
Nonereads ignored; sustained over-temperature trips; offender attributes correctofffor an in-spec plant; absent on a battery-less (PV-only) plant🤖 Generated with Claude Code