Skip to content

fix(solis): record a fatal status when no inverter has a battery - #4718

Draft
mgazza wants to merge 2 commits into
mainfrom
fix/solis-no-battery-fatal-status
Draft

fix(solis): record a fatal status when no inverter has a battery#4718
mgazza wants to merge 2 commits into
mainfrom
fix/solis-no-battery-fatal-status

Conversation

@mgazza

@mgazza mgazza commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

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:

Error: No battery found on any inverter - PredBat requires a battery to optimise

Recorded with had_errors=True, which also makes it terminal for the cycle: record_status()
sets self.had_errors as its last statement, and record_final_run_status()'s if self.had_errors:
branch logs without calling record_status, so the run-completion path does not overwrite it.

Re-asserted from run() rather than automatic_config(), because automatic_config() runs only
when first is true while had_errors resets every plan cycle — a status written once at startup
would be silently overwritten. The current_status check keeps that to one write per overwrite
rather than one per tick.

The predicate is deliberately narrow

_no_battery_fatal is computed from _reports_no_battery() alone, not from
num_inverters == 0. Those differ: an inverter only reaches num_inverters if it also has a
parseable batteryHealthSoh, so keying off that count would make a single-inverter account with a
real, 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 live inverterDetail fixtures 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. MockBase gains record_status/current_status mirroring
output.py, and MockSolisAPI gains the _no_battery_fatal default that initialize() sets.

Full suite --quick: no new failures (data_age_metrics fails identically on unmodified main).
black --check clean.

mgazza and others added 2 commits August 24, 2026 19:58
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.
@springfall2008

Copy link
Copy Markdown
Owner

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 error

This 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 is_battery_inverter() from #4643: "It is still polled and published, so its PV sensors keep working - only the writes are withheld."

For that user every Solis inverter reports no battery, so _no_battery_fatal is set and predbat.status gets pinned to the error even though their battery is fine and Predbat is optimising it correctly.

The gate on automatic config does exist implicitly (_no_battery_fatal is only ever assigned inside automatic_config(), which needs solis_automatic: true), but it isn't enough on its own: templates/solis_cloud.yaml ships solis_automatic: true and docs/apps-yaml.md calls it recommended, so that user is quite likely to have it on — they'd be using the component for the PV/load sensors and configuring the battery inverter separately.

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 anywhere

Running the scenario the PR targets — Solis account, solis_automatic: true, every inverter PV-only — automatic_config() bails at num_inverters == 0 and configures nothing. num_inverters then defaults to 1, inverter_type to GE, charge_start_time isn't in args and there's no givtcp_rest or ge_cloud_direct, so Inverter.update_status() hits the permanent-setup-gap branch (inverter.py ~L1516-1523):

Error: Inverter 0 unable to read charge window time - no source is configured
(set givtcp_rest, ge_cloud_direct, or charge_start_time/charge_start_hour in apps.yaml)

recorded with had_errors=True, then raise ValueError. There's a second unconditional one for the export window at ~L1630-1635. The ValueError escapes fetch_inverter_data() (the try there only wraps Inverter(self, id), not the update_status() call), so it also lands in the handler around update_pred() and records Error: Exception raised ..., again with had_errors=True.

So the run already dies with a fatal, terminal status — and a more actionable one than NO_BATTERY_STATUS, because it names exactly what's missing. That makes the PR's premise ("nothing downstream can tell 'no battery anywhere' apart from 'running fine'") not quite right, and leaves the change with no upside on its target case while regressing the one above.

My preference is to drop the fatal status and keep the Warn: log #4643 already emits. If the instance-health sweep needs to spot an account that can't be optimised, it should match the existing statuses (unable to read charge window time, Failed to fetch inverter data) — those are vendor-agnostic and cover AlphaESS, DEYE, GECloud and manual apps.yaml configs too, rather than Solis alone. It also avoids a cross-repo dependency on a magic substring in a vendor module.

The gap that does survive

Narrower 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 battery_size_tracking() (inverter.py ~L655) papers over the missing capacity with only a Warn:. If we want to close that, it belongs at that fallback in the core, where it can see every inverter — flag when a size couldn't be determined, and raise the status only if every inverter has been in that state for several consecutive cycles (the momentary-sensor-unavailable case needs to not trip it). Not in a vendor component.

Smaller points, if any version of this lands

  • The status doesn't stay terminal, it flaps. update_pred() resets had_errors = False at the top of every cycle. On the following cycle current_status is still NO_BATTERY_STATUS, so the assert no-ops, had_errors stays False, and record_final_run_status() writes the normal status again — then run() re-asserts within 60s. predbat.status ends up alternating every 5-minute cycle rather than settling.
  • That inflates error_count indefinitelyrecord_status() reads the attribute off the entity and increments it on every had_errors=True write, so ~288/day. Worth following the pattern in gecloud.py (~L1568), which reports its equivalent condition once per episode on transition, with a comment saying explicitly that it's to avoid inflating error_count.
  • The flag is latched until restartautomatic_config() only runs while first is True, so fitting a battery leaves the status fatal until Predbat is restarted.
  • Test fidelityMockBase.record_status mirrors current_status but not self.had_errors = True, which the real one sets. The correctness argument here rests on that interaction, and with it modelled the flapping above would show up in a test.

@springfall2008
springfall2008 marked this pull request as draft August 25, 2026 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants