admin/goodwe-addon
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Merge release/1.0 into SAFETY-30
checks / unit (pull_request) Successful in 4m39s
Details

Browse Source 
CFG-04 (#45) merged first and moved the base. Two conflicts, both resolved
as the union of the two sides:

- app/main.py: an import-block collision. CFG-04 added validate_unit_ranges
  to the .validate import; SAFETY-30 added AGAINST_METER, AGAINST_OWN_POWER,
  SIGN_OPTION_DEFAULTS and SignWatchdog to the .watchdog import. Neither
  side dropped anything, so the resolution keeps both lists whole.
- CHANGELOG.md: two adjacent new entries under ## Unreleased. Both kept.

Both sides also independently rewrote large parts of test_peakguard.py
(60 and 47 insertions against the merge base) and git merged them
textually, so the full suite was run on the MERGE RESULT rather than
inferred from the two parents: 19 passed, 0 failed, twice.

One caveat recorded rather than hidden: test_peakguard.py failed once
during the first sequential full-suite run and then passed 10/10 in
isolation and in two further full runs on this same tree. It passes on
both parents too, so this is the pre-existing flakiness CFG-04 already
disclosed, not a merge defect - it is timing- or load-sensitive. It sits
on the required CI context and can therefore block an unrelated merge at
random, which wants a card of its own.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BwRLw7eKYj2byVdwB6kbjU
This commit is contained in:
glenn schrooyen
2026-09-04 12:23:50 +02:00
co-authored by Claude Opus 5
parent d5c1577212 18f052a071
commit ae21b71824
13 changed files with 2090 additions and 120 deletions
Download Patch File Download Diff File Expand all files Collapse all files
goodwe_controller/app/validate.py
+323 -17
View File
@@ -151,6 +151,32 @@ NOT READY banner already names the problem, (b) does not apply.
permits equality because equality there leaves one reachable point that
works; here it leaves none.
9. An `inverters:` roster that cannot mean what it says. [(a) and (b)]
CFG-04's five cases are enumerated on `_roster_problems()`. Three of them
are (b) - an entry with no `setpoint_entity`, or no rating, is a unit that
takes a share of the site target every cycle and delivers none of it, on a
status page that reports a site running at full strength. Two are (a): two
entries sharing a name share one Store namespace, so one unit's SAFETY-02
latch IS the other's and an operator clearing one clears both; and a flat
`setpoint_entity` left set beside a declared roster is an actuator the
configuration names and nothing writes.
⚠️ RULES 5 AND 6 CHANGED SHAPE FOR THIS, and that is the part worth
reading. Both compared a threshold against the flat `max_w`, which was
right while there was exactly one inverter and is a PERMISSIVE READER the
moment there are two: `max_w: 5000` left over from a single-inverter
install, beside a roster of two 2000 W units, passed rule 5 at
`sign_disagree_w: 3000` while the watchdog could fire on NEITHER unit -
rule 5's own failure mode, reached through rule 5. They now read
`unit_caps()`, which is the roster's caps and, at one inverter, is
`[max_w]` exactly.
What rule 9 deliberately does NOT do is grade a rating against a device.
`dev_min`/`dev_max` here are one device's range and there are now N of
them, none of which has answered at startup. `validate_unit_ranges()` is
the per-unit form and main.py runs it from `run_control()`, once every
unit's own ESPHome has replied.
WHAT IS DELIBERATELY *NOT* REFUSED, because it is untidy rather than unsafe:
- `integrator_max_w` above `max_w`. DOCS.md is right that the only thing the
@@ -203,6 +229,7 @@ WHAT IS DELIBERATELY *NOT* REFUSED, because it is untidy rather than unsafe:
import logging
from .arbitrage import enabled as arbitrage_on
from .inverter import roster_entries
from .maintenance import MAINT_OPTION_DEFAULTS
from .p1 import is_enabled, validate_meter_options
from .peakguard import PEAK_OPTION_DEFAULTS
@@ -309,6 +336,229 @@ def actuator_writers(opts: dict) -> dict:
return census
def unit_caps(opts: dict) -> list[float]:
"""Each configured inverter's supervised cap, in roster order.
⚠️ WHY THIS EXISTS AT ALL, AND IT IS THE PERMISSIVE-READER PROBLEM IN THIS
FILE. Rules 5 and 6 both compare a threshold against "max_w", and both of
them are really comparing it against WHAT ONE UNIT MAY BE COMMANDED - the
arbiter files `supervised` per inverter, from `inv.max_w`, and there is no
such thing as a site rating on the wire. At one inverter those are the same
number and reading the flat option was right. At two they are not: a config
with `max_w: 5000` left over from a single-inverter install and a roster of
two 2000 W units would pass rule 5 at `sign_disagree_w: 3000` while the
watchdog could not fire on EITHER unit - the disarmed-guard-reporting-armed
state those rules exist to refuse, reached through the rule itself.
So the caps come from the roster the add-on will actually build
(`inverter.roster_entries()`, not a second reading of the option), and fall
back to the flat `max_w` only where that roster is the flat one too.
"""
return [cap for _, cap in unit_caps_indexed(opts)]
def unit_caps_indexed(opts: dict) -> list[tuple[int, float]]:
"""`unit_caps()`, but each cap carries the ROSTER INDEX it came from.
⚠️ WHY THE INDEX HAS TO TRAVEL WITH THE NUMBER. This function SKIPS entries
with no usable `max_w`, so position i in the caps list is not position i in
the roster the moment one entry is malformed. Rules 5 and 6 used to feed a
caps-list index straight into `_unit_label()`, which indexes `entries` - so
on a roster whose first entry has a junk `max_w`, the refusal named the
WRONG INVERTER. An operator told to lower a number on `inverter-a` when the
number is on `inverter-b` is being sent to the wrong device by the very
message that exists to stop that. Rule 9(d) already refuses such a config so
the two lists cannot desynchronise in shipped use - but "unreachable" is a
claim about today's rule ordering, not a property of this function, and
`validate_startup_config()` is also `sim/inject.py`'s entry point.
"""
entries = roster_entries(opts)
if not entries:
cap = _number(opts, "max_w", 2000.0)
return [] if cap is None else [(0, cap)]
caps: list[tuple[int, float]] = []
for index, entry in enumerate(entries):
if not isinstance(entry, dict):
continue
cap = _number(entry, "max_w", None)
if cap is not None:
caps.append((index, cap))
return caps
def _unit_label(opts: dict, index: int) -> str:
""""" at a single-inverter config, " (inverter-b)" at a roster.
The messages below have to read identically on the install that upgrades
into this version - it is the same configuration, refused for the same
reason - and have to NAME THE UNIT the moment there is more than one, or an
operator with three inverters is told a number is wrong and not which one.
"""
entries = roster_entries(opts)
if len(entries) <= 1:
return ""
entry = entries[index] if index < len(entries) else {}
name = ""
if isinstance(entry, dict):
name = str(entry.get("name") or "").strip()
return f" ({name or f'inverters[{index}]'})"
def _roster_problems(opts: dict) -> list[str]:
"""Rule 9 (CFG-04): the `inverters:` roster, refused where it cannot mean
what it says.
⚠️ THE SUPERVISOR ALREADY ENFORCES MOST OF THIS AND THAT IS NOT A REASON TO
DROP IT. `config.yaml`'s schema makes `name`, `setpoint_entity` and `max_w`
required per entry, and Supervisor 2026.08.0 refuses a blob that omits one
by name - measured, not assumed. But `validate_startup_config()` is also
`sim/inject.py`'s entry point and every check's, and those reach it with a
dict the Supervisor never saw; the same argument `price_provider` makes two
rules up. A rule the Supervisor duplicates costs nothing and covers the
callers it does not.
Five things are refused, and each is a config that would otherwise RUN:
a. `inverters` is not a list. `roster_entries()` wraps a scalar so this
can name it; without the rule the roster would be one inverter built
out of a string's `.get`, i.e. an immediate AttributeError.
b. An entry with no usable `name`. Store keys are namespaced BY NAME
(`store.unit_key`), so two nameless entries would share one namespace
and one unit's SAFETY-02 latch would silently be the other's.
c. Two entries with the SAME name - (b) reached the long way round.
d. An entry with no `setpoint_entity`, or no usable `max_w`. The first is
a unit this controller cannot command; the second is a unit whose
supervised claim would be `+-0 W`, which is a unit pinned at zero by a
missing key rather than by a decision.
e. A flat `setpoint_entity` alongside a declared roster. The flat keys are
the SINGLE-inverter shape; leaving one set while a roster is declared
means one of two things and neither is safe. If it names an entity an
entry also names, SAFETY-28's census below already refuses it as two
writers on one actuator. If it names a DIFFERENT one, nothing writes
it, the site runs quietly smaller than the configuration reads, and the
one place it would show up is a status page nobody compares against the
config. Refused rather than ignored, for §1.5's reason and for this
card's: a config that silently becomes a different config is worse than
one that refuses to load.
"""
problems: list[str] = []
raw = opts.get("inverters")
if raw is None or raw == "" or raw == []:
return problems
if not isinstance(raw, (list, tuple)):
return [f"inverters is {type(raw).__name__} {raw!r}, and it must be a "
"LIST of inverter objects - one per inverter, each naming its "
"own name, setpoint_entity and max_w. A single inverter needs "
"no list at all: leave `inverters` empty and the flat "
"setpoint_entity/soc_entity/batt_entity/max_w keys describe it."]
entries = roster_entries(opts)
seen: dict = {}
for i, entry in enumerate(entries):
where = f"inverters[{i}]"
if not isinstance(entry, dict):
problems.append(
f"{where} is {type(entry).__name__} {entry!r}, and every entry "
"must be an object with at least name, setpoint_entity and "
"max_w.")
continue
name = str(entry.get("name") or "").strip()
if not name:
problems.append(
f"{where} has no name. Every inverter needs one: the Store "
"namespaces this unit's SAFETY-02 latch, its heartbeat and its "
"write bookkeeping under it, so two unnamed units would share "
"one namespace and a trip caused by one would latch the other. "
"Any short identifier works - inverter-a, inverter-b, garage.")
elif name in seen:
problems.append(
f"{where} and {seen[name]} are both called {name!r}. The Store "
"keys every per-unit latch under the unit's name, so two units "
"with one name share one SAFETY-02 latch and one heartbeat "
"deadline - a trip caused by either would stop both, and an "
"operator clearing it would clear both. Give them different "
"names.")
else:
seen[name] = where
who = f"{where} ({name})" if name else where
if not str(entry.get("setpoint_entity") or "").strip():
problems.append(
f"{who} has no setpoint_entity, so this add-on cannot command "
"it. It would sit in the roster, take a share of the site "
"target every cycle and deliver none of it, while the status "
"page showed a site running at full strength. Give it the "
"ESPHome number entity that carries ITS setpoint, or remove "
"the entry.")
if _number(entry, "max_w", None) is None or (_number(entry, "max_w", None) or 0) <= 0:
problems.append(
f"{who} has no usable max_w (got {entry.get('max_w')!r}). It is "
"the supervised cap on what may be written to THIS inverter, it "
"is not shared with any other unit and it is deliberately not "
"inherited from the flat max_w - a rating borrowed from a "
"sibling is exactly what DEV-02 removed. State this unit's own "
"rating in watts.")
flat_sp = str(opts.get("setpoint_entity") or "").strip()
if entries and flat_sp:
problems.append(
f"setpoint_entity is set to {flat_sp} AND an inverters list of "
f"{len(entries)} is declared. The flat setpoint_entity/soc_entity/"
"batt_entity/max_w keys are the SINGLE-inverter shape and are not "
"read at all once a roster exists, so this config declares an "
"actuator that nothing will ever write - a site running quietly "
"smaller than its own configuration. Move that inverter into the "
"list as its own entry and clear setpoint_entity.")
return problems
def validate_unit_ranges(units, *, skip_entity: str = "") -> list[str]:
"""Rule 2, per inverter, against each unit's OWN reported device range.
⚠️ THE READER THIS REPLACES FAILED PERMISSIVE, WHICH IS THE DIRECTION THAT
HIDES A DEFECT. `run_control()` re-ran rule 2 behind `if
self.o.get("setpoint_entity")` - the FLAT key - because the flat options
were the only per-device options there were. Declare a roster and that key
is blank by rule 9(e), so the whole re-check was skipped and NO inverter's
`max_w` was ever graded against the device it is written to. That is rule
2's own failure mode restored by a config shape: the operator's stated
ceiling is not the ceiling and nothing says which won.
`units` is the roster (anything with `.name`, `.setpoint_entity`, `.max_w`,
`.dev_min`, `.dev_max`). A unit whose device did not answer is SKIPPED, not
guessed at, for the reason in this module's docstring: ESPHome is often not
up when the add-on boots and (None, None) means "not read", not "zero".
`skip_entity` is the flat `setpoint_entity` when the caller has already
graded it through `validate_startup_config(opts, dev_min, dev_max)`. At a
single-inverter config that is the one unit, and grading it twice would
report the same conflict twice under two wordings.
"""
problems: list[str] = []
skip = str(skip_entity or "").strip().lower()
for unit in units:
entity = str(getattr(unit, "setpoint_entity", "") or "").strip()
if not entity or entity.lower() == skip:
continue
cap = float(getattr(unit, "max_w", 0.0) or 0.0)
dev_min = getattr(unit, "dev_min", None)
dev_max = getattr(unit, "dev_max", None)
name = getattr(unit, "name", entity)
if dev_max is not None and cap > float(dev_max):
problems.append(
f"{name}: max_w is {cap:.0f} W but {entity} accepts at most "
f"{float(dev_max):.0f} W - THE DEVICE WINS, so the {cap:.0f} W "
f"configured for this inverter would silently never happen. "
f"Lower this entry's max_w to {float(dev_max):.0f} W or less.")
elif dev_min is not None and -cap < float(dev_min):
problems.append(
f"{name}: max_w is {cap:.0f} W, so this controller may command "
f"{-cap:.0f} W, but {entity} accepts no less than "
f"{float(dev_min):.0f} W - THE DEVICE WINS on the charge side. "
f"Lower this entry's max_w to {abs(float(dev_min)):.0f} W or "
"less.")
return problems
class ConfigRefused(Exception):
"""The configuration is wrong. Not "something broke" - see main.py's handler.
@@ -367,7 +617,21 @@ def validate_startup_config(opts: dict, dev_min=None, dev_max=None) -> list[str]
"each actuator exactly one writer, or point the second one at "
"its own entity.")
# --- 9. CFG-04's roster, graded BEFORE anything reads a per-unit number --
# ⚠️ FIRST, THOUGH IT IS THE NEWEST RULE. Rules 5 and 6 below now ask the
# roster what one unit may be commanded, so a roster that is nonsense has to
# have been named as nonsense before they read it - otherwise an entry with
# no `max_w` silently contributes no cap, `min()` sees a shorter list, and
# the operator is told about a sign watchdog when the real problem is a
# missing rating one line up.
problems.extend(_roster_problems(opts))
# --- 2. the configured ceiling against the device's own rating -----------
# ⚠️ THE FLAT `max_w`, AND ONLY THE FLAT ONE. `dev_min`/`dev_max` are ONE
# device's range - the flat `setpoint_entity`'s - so this rule can only ever
# speak for the single-inverter shape. The per-unit form of it is
# `validate_unit_ranges()`, which main.py runs once every unit's own device
# has answered; it is not called from here because at startup no device has.
max_w = _number(opts, "max_w", 2000.0)
if max_w is None:
problems.append(f"max_w {opts.get('max_w')!r} is not a number")
@@ -446,18 +710,37 @@ def validate_startup_config(opts: dict, dev_min=None, dev_max=None) -> list[str]
# below max_w, so threshold == max_w is already unsatisfiable. Anything
# BELOW max_w is reachable and therefore fine - this refuses the disarmed
# case only, not "close to the rail".
#
# ⚠️ AND IT IS GRADED AGAINST THE SMALLEST UNIT'S CAP, NOT AGAINST THE FLAT
# `max_w`. There is one SignWatchdog PER INVERTER (DEV-02) and each grades
# its own unit's standing command, which the arbiter bounds by THAT UNIT's
# `supervised` claim - so the watchdog that can never fire is the one on the
# smallest inverter, and a threshold under the site's largest cap says
# nothing about it. At one inverter `unit_caps()` is `[max_w]` and this is
# the identical comparison it always was.
sign_w = _number(opts, "sign_disagree_w", 300.0)
# ⚠️ (index, cap) PAIRS, NOT BARE CAPS. `unit_caps()` skips entries with no
# usable `max_w`, so `caps.index(weakest)` was an index into the CAPS list
# being handed to `_unit_label()`, which indexes the ROSTER - and the message
# named the wrong inverter as soon as the two lengths differed. The index
# travels with the number now, so the name is the name of the unit the
# number came off.
caps_ix = unit_caps_indexed(opts)
caps = [cap for _, cap in caps_ix]
weakest = min(caps) if caps else None
if sign_w is None:
problems.append(f"sign_disagree_w {opts.get('sign_disagree_w')!r} is not a number")
elif max_w is not None and max_w > 0 and sign_w >= max_w:
elif weakest is not None and weakest > 0 and sign_w >= weakest:
where = _unit_label(opts, min(caps_ix, key=lambda p: p[1])[0])
problems.append(
f"sign_disagree_w is {sign_w:.0f} W but max_w is {max_w:.0f} W, so "
f"sign_disagree_w is {sign_w:.0f} W but max_w{where} is "
f"{weakest:.0f} W, so "
"the SAFETY-02 sign-disagreement watchdog can never fire: the "
"arbiter never lets a command exceed max_w, and the watchdog needs "
f"a command over {sign_w:.0f} W. It would sit there reporting "
"'armed' on the status page while being incapable of tripping - the "
f"one runaway failure mode this system has actually observed. Lower "
f"sign_disagree_w below {max_w:.0f} W (300 W is the measured "
f"sign_disagree_w below {weakest:.0f} W (300 W is the measured "
"default), or raise max_w.")
# ⚠️ AND THE SAME RULE FOR SAFETY-30's PER-UNIT THRESHOLD, WHICH IS A
@@ -518,22 +801,40 @@ def validate_startup_config(opts: dict, dev_min=None, dev_max=None) -> list[str]
# the one in-code mirror of config.yaml for this family - the same move
# rule 8 below already makes with `PEAK_OPTION_DEFAULTS` - and
# test_maintenance.py pins it by reading the shipped file.
#
# ⚠️ AND IT IS GRADED AGAINST THE FIRST UNIT'S CAP, NOT THE SMALLEST AND NOT
# THE FLAT ONE. Rule 5 takes the weakest unit because there is a watchdog on
# every one of them; there is exactly ONE `Maintenance` in this add-on and
# `Controller.maint_unit` names the pack it recalibrates - the first in the
# roster, because the cycle's history lives under un-namespaced Store keys.
# The shaping claim is filed on THAT actuator and on no other, so the cap
# that can contradict it is THAT unit's. Taking `min()` here would refuse a
# perfectly workable site - a 5000 W pack being recalibrated beside a 2000 W
# sibling the cycle never touches - which is the over-refusal this file's
# docstring says gets a validator switched off. When `maint_unit` stops
# being `inverters[0]`, this line moves with it.
charge_w = _number(opts, "maintenance_charge_w",
float(MAINT_OPTION_DEFAULTS["maintenance_charge_w"]))
# ⚠️ THE SAME PAIRING AS RULE 5, FOR THE SAME REASON. `caps[0]` is the first
# unit WITH A USABLE CAP, which is `inverters[0]` only while every earlier
# entry has one; `_unit_label(opts, 0)` was hard-wired to `inverters[0]`
# regardless. Both come off the one pair now.
maint_index, maint_cap = caps_ix[0] if caps_ix else (0, None)
maint_where = _unit_label(opts, maint_index)
if charge_w is None:
problems.append(
f"maintenance_charge_w {opts.get('maintenance_charge_w')!r} is not a number")
elif max_w is not None and max_w > 0 and charge_w > max_w:
elif maint_cap is not None and maint_cap > 0 and charge_w > maint_cap:
problems.append(
f"maintenance_charge_w is {charge_w:.0f} W but max_w is "
f"{max_w:.0f} W, so a maintenance charge in a cheap window can "
f"maintenance_charge_w is {charge_w:.0f} W but max_w{maint_where} is "
f"{maint_cap:.0f} W, so a maintenance charge in a cheap window can "
"never happen: the shaping asks the arbiter for at most "
f"{-charge_w:.0f} W while the supervised limit allows no less than "
f"{-max_w:.0f} W, which is a contradiction it resolves to 0 W - "
f"{-maint_cap:.0f} W, which is a contradiction it resolves to 0 W - "
"THE CEILING WINS, and what you would see is an ARBITER "
"CONTRADICTION line that reads like a bug rather than like these "
f"two numbers. Lower maintenance_charge_w to {max_w:.0f} W or less, "
f"or raise max_w to at least {charge_w:.0f} W.")
f"two numbers. Lower maintenance_charge_w to {maint_cap:.0f} W or "
f"less, or raise max_w to at least {charge_w:.0f} W.")
# --- 7. two price sources on one site ------------------------------------
problems.extend(_price_problems(opts))
@@ -593,18 +894,23 @@ def validate_startup_config(opts: dict, dev_min=None, dev_max=None) -> list[str]
# three-way divergence (config.yaml 2000, MaintConfig 2500) and the card
# named only the charge side. Closing one key and leaving its twin is how a
# single-source fix gets reproduced as a single-source bug one line down.
# ⚠️ AND AGAINST THE MAINTENANCE UNIT'S CAP, for rule 6's reason: it is the
# same one actuator being clamped, so grading the warning against the flat
# `max_w` would warn about a site number while the clamp is a unit number.
disch_w = _number(opts, "maintenance_discharge_w",
float(MAINT_OPTION_DEFAULTS["maintenance_discharge_w"]))
if disch_w is not None and max_w is not None and max_w > 0 and disch_w > max_w:
if (disch_w is not None and maint_cap is not None and maint_cap > 0
and disch_w > maint_cap):
_LOG.warning(
"maintenance_discharge_w %.0f W is above max_w %.0f W. Not refused - "
"the drain phase SETS the setpoint and the arbiter clamps it to "
"%.0f W and names the clamp ('limited by supervised(max_w)'), so "
"nothing is silent and the cycle still completes. But it drains at "
"%.0f W, not the %.0f W you configured, and the drain phase has a "
"720 min timeout. Lower it to %.0f W or raise max_w to remove the "
"maintenance_discharge_w %.0f W is above max_w%s %.0f W. Not "
"refused - the drain phase SETS the setpoint and the arbiter clamps "
"it to %.0f W and names the clamp ('limited by supervised(max_w)'), "
"so nothing is silent and the cycle still completes. But it drains "
"at %.0f W, not the %.0f W you configured, and the drain phase has "
"a 720 min timeout. Lower it to %.0f W or raise max_w to remove the "
"surprise.",
disch_w, max_w, max_w, max_w, disch_w, max_w)
disch_w, maint_where, maint_cap, maint_cap, maint_cap, disch_w,
maint_cap)
return problems