fix(solis): record a fatal status when no inverter has a battery - #4718
fix(solis): record a fatal status when no inverter has a battery#4718mgazza wants to merge 2 commits into
Conversation
PredBat optimises battery charging. An account where no inverter has a battery cannot be optimised at all, but until now that was only a log line - the SaaS health sweep classifies on predbat.status and saw nothing wrong. Record it as an Error status with had_errors=True, which also makes it terminal for the cycle: predbat.py's completion path logs without calling record_status when had_errors is set. Re-asserted from run() rather than automatic_config(), because automatic_config runs only on first while had_errors resets every plan cycle. The current_status check keeps that to one write per overwrite. A mixed account (some inverters with batteries) is deliberately NOT fatal - the customer has a working battery and must never be paused for it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
_no_battery_fatal was derived from num_inverters == 0, but an inverter only counts toward num_inverters if it is both not-"No Battery" AND has a parseable batteryHealthSoh. A single-inverter account with a real, connected battery whose firmware omits or non-numerically reports batteryHealthSoh therefore hit num_inverters == 0, got the fatal "No battery found on any inverter" status, and would be auto-paused on day 7 despite having a working battery. Compute _no_battery_fatal directly from _reports_no_battery() over every configured inverter, before the SoH-based enrolment gate runs. This matches NO_BATTERY_STATUS's own wording and the SaaS onboarding block's predicate (reportsNoBattery has no SoH clause), and keeps num_inverters == 0's existing log-only behaviour for the SoH-missing case. Adds a regression test proving a real battery with missing/non-numeric batteryHealthSoh does not trigger the fatal status. Found in final whole-branch review of the no-battery-detection feature.
|
Thanks for digging into this, but I don't think we should take it in this form — I think it regresses a supported setup, and the gap it's aiming at is mostly already covered. 1. A PV-only Solis alongside a different battery system becomes a permanent errorThis is my main concern. Someone with a Solis inverter for solar only, plus a battery from another manufacturer, is a configuration we explicitly support — see the docstring on For that user every Solis inverter reports no battery, so The gate on automatic config does exist implicitly ( The underlying issue is that the component can only answer "does Solis have a battery", not "does this account have a battery". Those aren't the same question, and the fatal status is asserted on the wrong one. 2. Predbat already errors when there is genuinely no battery anywhereRunning the scenario the PR targets — Solis account, recorded with So the run already dies with a fatal, terminal status — and a more actionable one than My preference is to drop the fatal status and keep the The gap that does surviveNarrower than the PR frames it: an account where the control entities are configured but the battery has gone — a stale apps.yaml, or a component that auto-configured while a battery was present and later lost it. There the charge/export windows resolve fine, and the 8 kWh fallback in Smaller points, if any version of this lands
|
Problem
PredBat optimises battery charging. An account where no inverter has a battery cannot be
optimised at all — but #4643 only skips those inverters and logs it, so nothing downstream can
tell "no battery anywhere" apart from "running fine".
Fix
Record a fatal status when every discovered inverter explicitly declares no battery:
Recorded with
had_errors=True, which also makes it terminal for the cycle:record_status()sets
self.had_errorsas its last statement, andrecord_final_run_status()'sif self.had_errors:branch logs without calling
record_status, so the run-completion path does not overwrite it.Re-asserted from
run()rather thanautomatic_config(), becauseautomatic_config()runs onlywhen
firstis true whilehad_errorsresets every plan cycle — a status written once at startupwould be silently overwritten. The
current_statuscheck keeps that to one write per overwriterather than one per tick.
The predicate is deliberately narrow
_no_battery_fatalis computed from_reports_no_battery()alone, not fromnum_inverters == 0. Those differ: an inverter only reachesnum_invertersif it also has aparseable
batteryHealthSoh, so keying off that count would make a single-inverter account with areal, connected battery whose firmware omits SoH report "no battery found" — and, on the SaaS side,
eventually auto-pause a customer whose battery is fine. That is exactly the inference
_reports_no_battery's own comments forbid, and it is covered by a regression test here.A mixed account (some inverters with batteries, some without) is deliberately not fatal — the
customer has a working battery and nothing is broken for them.
Testing
Three new tests in
test_solis.py, reusing the liveinverterDetailfixtures from #4643:fatal status for a wholly battery-less account (recorded once, idempotent, re-asserted after an
overwrite); no status for a mixed account; and no status for a real battery with a
missing/non-numeric
batteryHealthSoh.MockBasegainsrecord_status/current_statusmirroringoutput.py, andMockSolisAPIgains the_no_battery_fataldefault thatinitialize()sets.Full suite
--quick: no new failures (data_age_metricsfails identically on unmodifiedmain).black --checkclean.