CFG-01 review: widen the rule, refuse the firmware-veto case, fix the stop footgun
Review findings, all confirmed in code before changing anything. THE RULE DID NOT AUTHORISE ITS OWN BEST REFUSAL. The stated test was "makes a safety mechanism inoperative, or makes a stated limit untrue", and a blank setpoint_entity does neither - layer 1 pinning the inverter at 0 W is the failsafe WORKING, as validate.py's own text admitted. The real justification is a second, separate category: the add-on is inert and indistinguishable from healthy. Stated as (a) and (b), with each refusal tagged, and with both halves explicitly bounded - (a) is not "any two numbers that could disagree", (b) is not "anything degraded". Left implicit, the gap is where trivia-creep comes back in. meter_max_age_s ABOVE THE FIRMWARE'S VETO IS NOW REFUSED. The schema allows int(5,300); the firmware's threshold is a compile-time `max_age_s: "30"` and it trips layer 1 at `p1_age_s >= 30`. At 120 the add-on believes a 119 s reading is fresh and keeps commanding while the board has already vetoed and is holding at 0 W - healthy meter, running controller, nothing moving. Structurally identical to the max_w case, and unlike max_w it is settable on this install, so the headline behaviour finally has a live demonstration. That firmware file's own comment says a lower bound on this "belongs in CFG-01, not here". Only ABOVE is refused: below the veto the add-on gives up first, which is fail-safe and visible on its own status page. Scoped to P1 being enabled, because meter_max_age_s governs nothing otherwise. FIRMWARE_INGEST_VETO_S cannot be imported from a compile-time YAML substitution, so it carries a note on the single direction in which it can go stale - and a warning that TEL-06 is editing `ingest_stale`, so a matching integer may stop meaning a matching rule. stop= WAS A LIVE FOOTGUN. It defaulted to None, which made a Controller that cannot stop itself the DEFAULT one. On the late-refusal path that meant: log the banner, return, and leave the process alive forever with no control loop and no exit - the exact "inert but looks healthy" failure this module exists to refuse. Now required and keyword-only, so the broken shape does not construct; test_p1.py's five call sites pass an Event. That whole path had no test and no live demo. It has one now: it drives run_control() against a device that only reports a narrower range once the loop starts, and asserts it refuses, signals, returns, and writes NOTHING - plus the converse, that an agreeing device leads to real writes. This also covers the max_w composition that cannot be reached through the Supervisor on the live install. max_w <= 0 is refused. Schema-blocked in production, but validate_startup_config() is the simulation suite's entry point too, and a non-positive max_w inverts the arbiter's supervised bounds into a contradiction that reads as a safety trip rather than a typo. Verified live 2026-08-28 (backup goodwe_controller-cfg01r2-1787940076): meter_max_age_s 120 refused with a message naming 120, 30, and THE FIRMWARE WINS; no traceback; add-on restored byte-exact and running. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Du77usMj8XNKNFZGmUiWDa
This commit is contained in:
glenn schrooyen
co-authored by
Claude Opus 5
Claude Opus 5
parent
fc9bd89dca
commit
547d796c5d
@@ -57,13 +57,20 @@ def load_options() -> dict:
|
||||
|
||||
|
||||
class Controller:
|
||||
def __init__(self, opts: dict, hass: HomeAssistant, store, mqtt_pub, stop=None):
|
||||
def __init__(self, opts: dict, hass: HomeAssistant, store, mqtt_pub, *, stop):
|
||||
self.o = opts
|
||||
self.hass = hass
|
||||
self.store = store
|
||||
self.mqtt = mqtt_pub
|
||||
# Set by run_control() if the startup gate could not see a conflict
|
||||
# because the setpoint device had not appeared yet. See run_control().
|
||||
# ⚠️ REQUIRED, AND KEYWORD-ONLY, AFTER A REVIEW CATCH. This started life
|
||||
# as `stop=None`, which made a Controller that CANNOT STOP ITSELF the
|
||||
# default one. On the late-refusal path in run_control() that default
|
||||
# meant: log the banner, return, and leave the process alive forever
|
||||
# with no control loop, no exit and no further output - the exact
|
||||
# "inert but looks healthy" failure this whole module exists to refuse.
|
||||
# amain() always passed a real Event, so it was latent, but test_p1.py
|
||||
# was already constructing Controllers without one. No default now, so
|
||||
# the broken shape does not typecheck.
|
||||
self.stop_event = stop
|
||||
self.refused: list[str] | None = None
|
||||
|
||||
@@ -325,8 +332,11 @@ class Controller:
|
||||
refuse(problems)
|
||||
self.refused = problems
|
||||
self.log_event("REFUSED: " + problems[0])
|
||||
if self.stop_event is not None:
|
||||
self.stop_event.set()
|
||||
# Signal, then return WITHOUT entering the loop. amain() wakes
|
||||
# on this, runs the normal wind-down (which commands 0 W) and
|
||||
# re-raises ConfigRefused afterwards, so the exit code is 78 and
|
||||
# the inverter is left at zero rather than latched.
|
||||
self.stop_event.set()
|
||||
return
|
||||
|
||||
last_grid = object()
|
||||
|
||||
@@ -15,25 +15,45 @@ told is refused at startup, not survived at runtime.
|
||||
|
||||
WHERE THE LINE IS DRAWN, and why it is not further out. A validator that refuses
|
||||
on trivia gets switched off by the first person it inconveniences, so this one
|
||||
refuses on exactly three things, all of which share one property: **continuing
|
||||
makes a safety mechanism inoperative or makes a stated limit untrue.**
|
||||
refuses only where **continuing produces one of two outcomes:**
|
||||
|
||||
1. `max_w` outside the setpoint device's own rating.
|
||||
(a) **a stated limit is untrue** - the operator configured a bound, a lower
|
||||
layer silently enforces a different one, and nothing says which won; or
|
||||
(b) **the add-on is inert and indistinguishable from healthy** - it starts,
|
||||
serves its UI, reports green, and moves no energy, with no symptom that
|
||||
separates it from a correctly-idle system.
|
||||
|
||||
⚠️ (b) IS NOT A SPECIAL CASE OF (a), and an earlier draft of this file wrongly
|
||||
implied it was by claiming every refusal here "makes a safety mechanism
|
||||
inoperative". It does not: when `setpoint_entity` is blank, layer 1 pinning the
|
||||
inverter at 0 W is the failsafe WORKING. The problem is not that the system
|
||||
becomes unsafe, it is that it becomes a very convincing impression of a working
|
||||
controller - and an operator has no way to tell. That is its own reason to
|
||||
refuse, and stating it as one is what stops the rule being stretched later to
|
||||
cover mere untidiness.
|
||||
|
||||
Both halves have to stay narrow. (a) is not "any two numbers that could
|
||||
disagree"; it is a bound the operator SET being overridden by a layer they
|
||||
cannot see. (b) is not "anything degraded"; it is inert AND silent - if a red
|
||||
NOT READY banner already names the problem, (b) does not apply.
|
||||
|
||||
1. `max_w` outside the setpoint device's own rating. [(a)]
|
||||
The stated limit is untrue. arbiter.py's `supervised` SAFETY claim says the
|
||||
output is bounded by ±max_w; it is not, it is bounded by the device, and the
|
||||
two numbers disagree. Which one wins is a coin toss to anyone reading the
|
||||
config.
|
||||
|
||||
2. No `setpoint_entity`.
|
||||
This is a controller with no actuator, and nothing about it looks broken.
|
||||
2. No `setpoint_entity`. [(b)]
|
||||
A controller with no actuator, and nothing about it looks broken.
|
||||
`write_setpoint()` returns immediately on a blank entity, so: no device
|
||||
rating is ever read (the `device` SAFETY claim is never made at all), and
|
||||
the ESP32 heartbeat - the thing that feeds LAYER 1 - never happens. The
|
||||
add-on serves a UI, publishes MQTT, resolves claims, and moves nothing. It
|
||||
is fail-safe by accident, not by design, and it is indistinguishable from
|
||||
working.
|
||||
add-on serves a UI, publishes MQTT, resolves claims, and moves nothing.
|
||||
Layer 1 then holds the inverter at 0 W, which is the failsafe working
|
||||
exactly as designed - this is (b), not (a). Nothing is unsafe; it is simply
|
||||
impossible to tell from the outside that the thing is doing nothing.
|
||||
|
||||
3. A `meter_source` that can never produce a reading.
|
||||
3. A `meter_source` that can never produce a reading. [(b)]
|
||||
§5.1 makes P1 the only authoritative measurement and deliberately provides
|
||||
no fallback, so a transport that cannot be built means `grid` is None
|
||||
forever: permanent "inputs missing", 0 W, and a status page that reports a
|
||||
@@ -41,6 +61,19 @@ makes a safety mechanism inoperative or makes a stated limit untrue.**
|
||||
sent anything to yet. The predicates live in `p1.validate_meter_options()`,
|
||||
which is also what `build_source()` uses - one definition, two callers.
|
||||
|
||||
4. `meter_max_age_s` above the firmware's own ingest veto. [(a)]
|
||||
The purest example of (a) in the system, and structurally the same fault as
|
||||
1. The add-on's schema accepts `int(5,300)`. The firmware's threshold is a
|
||||
COMPILE-TIME SUBSTITUTION - `max_age_s: "30"` in
|
||||
goodwe-master-stage4-failsafe.yaml - used by both its monotonic receipt
|
||||
clock and its `ingest_stale` veto, which trips layer 1 on
|
||||
`p1_age_s >= 30`. Set 120 and the add-on believes a 119 s reading is fresh
|
||||
and keeps commanding into it, while the firmware vetoed at 30 s and is
|
||||
holding the inverter at 0 W. Status page: healthy meter, running
|
||||
controller, nothing moving, and no line anywhere saying the number you set
|
||||
was overruled. That firmware file's own comment says a lower bound on this
|
||||
"belongs in CFG-01, not here" - so here it is.
|
||||
|
||||
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
|
||||
@@ -62,10 +95,29 @@ WHAT IS DELIBERATELY *NOT* REFUSED, because it is untidy rather than unsafe:
|
||||
|
||||
import logging
|
||||
|
||||
from .p1 import validate_meter_options
|
||||
from .p1 import is_enabled, validate_meter_options
|
||||
|
||||
_LOG = logging.getLogger("goodwe.config")
|
||||
|
||||
# ⚠️ A MIRROR OF A FIRMWARE CONSTANT, AND IT CANNOT BE IMPORTED. The firmware
|
||||
# threshold is a compile-time YAML substitution baked into the ESP32 image
|
||||
# (`max_age_s: "30"`, master/goodwe-master-stage4-failsafe.yaml), so nothing at
|
||||
# runtime can ask the board what value it was built with. This number is a copy,
|
||||
# and a copy of a constant in another repo is a thing that goes stale.
|
||||
#
|
||||
# What makes that acceptable here: the copy can only ever be WRONG IN THE SAFE
|
||||
# DIRECTION for the check below. If the firmware's veto is raised above 30, this
|
||||
# refuses some configs that would in fact have worked - annoying, visible
|
||||
# immediately, and fixed by editing one line. It cannot start accepting a config
|
||||
# the firmware vetoes unless the firmware's number goes DOWN, which would be a
|
||||
# tightening nobody does silently.
|
||||
#
|
||||
# ⚠️ TEL-06 IS EDITING `ingest_stale` DIRECTLY. If that ticket changes what the
|
||||
# veto MEANS - e.g. thresholding arrival rather than the sensor's value - this
|
||||
# constant's justification has to be re-read, not just its number. Do not assume
|
||||
# a matching integer means a matching rule.
|
||||
FIRMWARE_INGEST_VETO_S = 30.0
|
||||
|
||||
# ⚠️ The complete set of option keys through which this add-on can reach an
|
||||
# actuator. specs §1.5 makes "any configuration in which two writers can reach
|
||||
# one actuator" a non-goal, and the way that non-goal is ENFORCED here is by
|
||||
@@ -117,6 +169,21 @@ def validate_startup_config(opts: dict, dev_min=None, dev_max=None) -> list[str]
|
||||
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")
|
||||
# ⚠️ config.yaml's int(100,5000) already blocks this on a real install, so
|
||||
# the check is not for the Supervisor path - it is because
|
||||
# validate_startup_config() is now also sim/inject.py's entry point and can
|
||||
# be handed any dict at all. A max_w of 0 or below inverts the arbiter's
|
||||
# `supervised` bounds (lo=-max_w becomes the HIGHER number), and a limit
|
||||
# claim whose lo exceeds its hi is exactly the contradiction the arbiter
|
||||
# resolves to 0 W - i.e. it would look like a safety trip rather than the
|
||||
# nonsense config it is.
|
||||
elif max_w <= 0:
|
||||
problems.append(
|
||||
f"max_w is {max_w:.0f} W and must be positive - it is a magnitude, "
|
||||
"not a signed setpoint. At or below zero the arbiter's supervised "
|
||||
f"limit becomes lo={-max_w:.0f} / hi={max_w:.0f}, which is a "
|
||||
"contradiction it resolves to 0 W, so the controller would look "
|
||||
"like it was tripping rather than misconfigured.")
|
||||
elif dev_max is not None and max_w > dev_max:
|
||||
problems.append(
|
||||
f"max_w is {max_w:.0f} W but {opts.get('setpoint_entity') or 'the setpoint device'} accepts at "
|
||||
@@ -144,6 +211,33 @@ def validate_startup_config(opts: dict, dev_min=None, dev_max=None) -> list[str]
|
||||
else:
|
||||
problems.extend(validate_meter_options(opts, int(phases), max_age_s))
|
||||
|
||||
# --- 4. the staleness window the FIRMWARE will actually enforce -----------
|
||||
# ⚠️ Only when P1 ingestion is on, and that is a real scope limit rather than
|
||||
# caution: meter_max_age_s is read by nothing but P1Ingest, and the veto it
|
||||
# collides with keys off `p1_age_s.has_state()`, which is only ever published
|
||||
# when ingestion is running (see Controller.publish()). With meter_source off
|
||||
# there is no sensor, the veto is inert, and the number governs nothing.
|
||||
#
|
||||
# Strictly greater, not >=: the firmware trips at `>= 30` and P1Ingest goes
|
||||
# stale at `> max_age_s`, so 30 against 30 has the firmware fractionally
|
||||
# stricter by one sample. That is the shipped default and the tested pair.
|
||||
# BELOW 30 is fine and deliberately not refused - the add-on then gives up
|
||||
# first, which is fail-safe and visible on its own status page. Only ABOVE is
|
||||
# the silent case, where the add-on keeps commanding into a reading the board
|
||||
# has already rejected.
|
||||
if (is_enabled(opts) and max_age_s is not None
|
||||
and max_age_s > FIRMWARE_INGEST_VETO_S):
|
||||
problems.append(
|
||||
f"meter_max_age_s is {max_age_s:.0f} s but the ESP32 firmware vetoes "
|
||||
f"any P1 sample at {FIRMWARE_INGEST_VETO_S:.0f} s and forces the "
|
||||
"layer-1 failsafe - THE FIRMWARE WINS, and it wins silently. This "
|
||||
f"add-on would treat a {max_age_s - 1:.0f} s reading as fresh and "
|
||||
"keep commanding, while the inverter sat at 0 W with a healthy-"
|
||||
"looking meter on the status page. Lower meter_max_age_s to "
|
||||
f"{FIRMWARE_INGEST_VETO_S:.0f} s or less (30 s is the tested value), "
|
||||
"or rebuild the firmware with a matching max_age_s and update "
|
||||
"validate.FIRMWARE_INGEST_VETO_S.")
|
||||
|
||||
# --- warnings: real, but not worth refusing over -------------------------
|
||||
integ = opts.get("integrator_max_w")
|
||||
integ_w = None if not integ else _number(opts, "integrator_max_w", None)
|
||||
|
||||
Reference in New Issue
Block a user