From 547d796c5dee72f3cf7a125f93c58aee4c2ab136 Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Fri, 28 Aug 2026 20:05:10 +0200 Subject: [PATCH] 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 Claude-Session: https://claude.ai/code/session_01Du77usMj8XNKNFZGmUiWDa --- goodwe_controller/CHANGELOG.md | 29 +++++- goodwe_controller/app/main.py | 20 +++- goodwe_controller/app/validate.py | 114 ++++++++++++++++++++-- goodwe_controller/test_p1.py | 21 +++- goodwe_controller/test_validate.py | 150 ++++++++++++++++++++++++++++- 5 files changed, 310 insertions(+), 24 deletions(-) diff --git a/goodwe_controller/CHANGELOG.md b/goodwe_controller/CHANGELOG.md index e704b63..8f996c3 100644 --- a/goodwe_controller/CHANGELOG.md +++ b/goodwe_controller/CHANGELOG.md @@ -7,8 +7,14 @@ instead of logging a line and carrying on. `validate_startup_config()` runs in `amain()` before the broker, the web server or any task exists, so a refusal cannot leave half a controller running behind a UI. -It refuses exactly three things, all of which share one property - continuing -makes a safety mechanism inoperative or makes a stated limit untrue: +It refuses only where continuing produces one of two outcomes: **(a)** a stated +limit is untrue - the operator configured a bound and a lower layer silently +enforces a different one - or **(b)** the add-on is inert and indistinguishable +from healthy. (b) is not a special case of (a): with no `setpoint_entity`, layer +1 pinning the inverter at 0 W is the failsafe *working*. The system does not +become unsafe, it becomes a convincing impression of a working controller. + +Four things qualify: - **`max_w` above (or below) the setpoint device's own rating.** This is the one that changed behaviour. It used to warn `the device wins` and run anyway, with @@ -25,8 +31,25 @@ makes a safety mechanism inoperative or makes a stated limit untrue: ingestion*, which leaves the grid reading None forever: permanent "inputs missing", 0 W, and a status page indistinguishable from a healthy meter nobody has sent anything to yet. +- **`meter_max_age_s` above the firmware's ingest veto.** The schema accepts + `int(5,300)`; the ESP32's threshold is a compile-time `max_age_s: "30"` and it + trips layer 1 at `p1_age_s >= 30`. Set 120 and the add-on treats a 119 s + reading as fresh and keeps commanding, while the firmware has already vetoed + and is holding the inverter at 0 W - healthy meter, running controller, + nothing moving, and no line anywhere saying the number you set was overruled. + Structurally the same fault as the `max_w` case. Only *above* the veto is + refused: below it the add-on gives up first, which is fail-safe and visible on + its own status page. Mirrored as `validate.FIRMWARE_INGEST_VETO_S` - it cannot + be imported from a compile-time YAML substitution, so it carries a note about + the one direction in which it can go stale. -The predicates for the last group still live in `p1.validate_meter_options()`, +`max_w` at or below zero is refused too. Schema-blocked on a real install, but +`validate_startup_config()` is now also the simulation suite's entry point, and +a non-positive `max_w` inverts the arbiter's `supervised` bounds into a +contradiction that resolves to 0 W - i.e. it would read as a safety trip rather +than as the typo it is. + +The predicates for the meter group still live in `p1.validate_meter_options()`, which `build_source()` also calls - one definition, two callers, not two competing validators. Two transports gained a check they never had: `ha_dsmr` with only one of its two entities, and `mqtt_p1` with a blank topic, both of diff --git a/goodwe_controller/app/main.py b/goodwe_controller/app/main.py index 89af3a5..f4a20cd 100644 --- a/goodwe_controller/app/main.py +++ b/goodwe_controller/app/main.py @@ -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() diff --git a/goodwe_controller/app/validate.py b/goodwe_controller/app/validate.py index c6fd338..a1e8a22 100644 --- a/goodwe_controller/app/validate.py +++ b/goodwe_controller/app/validate.py @@ -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) diff --git a/goodwe_controller/test_p1.py b/goodwe_controller/test_p1.py index fdd24d2..9d6e5b8 100644 --- a/goodwe_controller/test_p1.py +++ b/goodwe_controller/test_p1.py @@ -1346,6 +1346,17 @@ check("with P1 on the age sensor IS announced", from app.main import Controller # noqa: E402 +def _never(): + """A stop Event these publish-only tests never set. + + `stop` is a REQUIRED keyword argument (CFG-01): a Controller that cannot + stop itself used to be the default one, and on the late-refusal path that + meant a live process with no control loop and no exit. These cases only call + publish(), so any Event will do - but they may not skip it. + """ + return asyncio.Event() + + class _Store: data = {} @@ -1368,14 +1379,14 @@ class _Pub: pub_off = _Pub() -Controller({"meter_source": "off"}, None, _Store(), pub_off).publish() +Controller({"meter_source": "off"}, None, _Store(), pub_off, stop=_never()).publish() check("with P1 off, p1_age is absent from the published payload", "p1_age" not in pub_off.last) check("...while the normal status keys are still published", "setpoint" in pub_off.last and "grid" in pub_off.last) pub_on = _Pub() -Controller({"meter_source": SOURCE_HA}, None, _Store(), pub_on).publish() +Controller({"meter_source": SOURCE_HA}, None, _Store(), pub_on, stop=_never()).publish() check("with P1 on, p1_age is published", "p1_age" in pub_on.last) check("...as a number, so has_state() becomes true only once we feed it", isinstance(pub_on.last["p1_age"], float)) @@ -1384,14 +1395,14 @@ check("...as a number, so has_state() becomes true only once we feed it", # age sensor is a hard prerequisite for the FW-01 flash, and it has to appear on # the transport that can actually read the meter in this house. pub_sig = _Pub() -Controller({"meter_source": SOURCE_HA_SIGNED}, None, _Store(), pub_sig).publish() +Controller({"meter_source": SOURCE_HA_SIGNED}, None, _Store(), pub_sig, stop=_never()).publish() check("with ha_signed selected, p1_age is published too", "p1_age" in pub_sig.last and isinstance(pub_sig.last["p1_age"], float)) # ⚠️ And on homewizard_local, where it is the one age FW-01 may actually # threshold - every other transport's age measures when the VALUE changed. pub_hw = _Pub() -Controller({"meter_source": SOURCE_HOMEWIZARD}, None, _Store(), pub_hw).publish() +Controller({"meter_source": SOURCE_HOMEWIZARD}, None, _Store(), pub_hw, stop=_never()).publish() check("with homewizard_local selected, p1_age is published", "p1_age" in pub_hw.last and isinstance(pub_hw.last["p1_age"], float)) @@ -1421,7 +1432,7 @@ def p1_rows(ctl): def _fed(source): - ctl = Controller({"meter_source": source}, None, _Store(), _Pub()) + ctl = Controller({"meter_source": source}, None, _Store(), _Pub(), stop=_never()) ctl.p1.submit(make_sample(source, 350.0, 0.0, phases=1, ingest_mono=time.monotonic())) return ctl diff --git a/goodwe_controller/test_validate.py b/goodwe_controller/test_validate.py index 247f1fe..2c1848d 100644 --- a/goodwe_controller/test_validate.py +++ b/goodwe_controller/test_validate.py @@ -7,10 +7,12 @@ a validator dies is not by missing a fault, it is by refusing something harmless until somebody deletes the call. """ +import asyncio import sys from app.validate import ( - ACTUATOR_WRITE_KEYS, ConfigRefused, refuse, validate_startup_config, + ACTUATOR_WRITE_KEYS, FIRMWARE_INGEST_VETO_S, ConfigRefused, refuse, + validate_startup_config, ) fails = [] @@ -120,6 +122,47 @@ check("a misspelt meter_source is refused rather than silently ignored", why(validate_startup_config(base(meter_source="ha_signd"), -5000, 5000), "is not one of")) +# --------------------------------------------------------------------------- # +print("4. meter_max_age_s against the firmware's compile-time veto") +# ⚠️ The schema allows int(5,300); the firmware's threshold is baked in at 30 s +# and wins silently. Same shape as the max_w case: a configured bound overridden +# by a lower layer, with nothing saying so. +hw = dict(meter_source="homewizard_local", p1_host="m") +p = validate_startup_config(base(**hw, meter_max_age_s=120), -5000, 5000) +check("a meter_max_age_s above the firmware veto is refused", len(p) == 1) +check("...naming the configured value", why(p, "120")) +check("...naming the firmware's value", why(p, "30")) +check("...and saying which one wins", why(p, "THE FIRMWARE WINS")) +check("the tested pair 30/30 is accepted", + validate_startup_config(base(**hw, meter_max_age_s=30), -5000, 5000) == []) +check("one second above it is refused", + validate_startup_config(base(**hw, meter_max_age_s=31), -5000, 5000) != []) +# Below the veto the add-on gives up FIRST, which is fail-safe and visible on +# its own status page. Only above is the silent case. +check("below the veto is fine - the add-on being stricter is not a conflict", + validate_startup_config(base(**hw, meter_max_age_s=10, + meter_poll_s=4), -5000, 5000) == []) +# ⚠️ Scope: meter_max_age_s is read by nothing but P1Ingest, and the veto keys +# off a sensor only published when ingestion runs. With the meter off it governs +# nothing, so refusing would be pure trivia. +check("with meter_source off the number governs nothing and is not refused", + validate_startup_config(base(meter_source="off", meter_max_age_s=300), + -5000, 5000) == []) +check("the constant is the firmware's, not a rounded guess", + FIRMWARE_INGEST_VETO_S == 30.0) + +# --------------------------------------------------------------------------- # +print("max_w must be a positive magnitude") +# Schema-blocked on a real install, but validate_startup_config() is also +# sim/inject.py's entry point and can be handed any dict at all. +for bad in (0, -1, -2000): + p = validate_startup_config(base(max_w=bad), -5000, 5000) + check(f"max_w {bad} is refused", len(p) == 1 and why(p, "must be positive")) +check("...and the message explains it would read as a trip, not a mistake", + why(validate_startup_config(base(max_w=0), -5000, 5000), "contradiction")) +check("the smallest sane max_w still starts", + validate_startup_config(base(max_w=1), -5000, 5000) == []) + # --------------------------------------------------------------------------- # print("what must NOT be refused - a validator that fires on trivia gets deleted") # DOCS.md: the arbiter's `supervised` limit still binds the wire to +-max_w, so @@ -153,6 +196,111 @@ err = ConfigRefused(p) check("ConfigRefused carries every problem", err.problems == p) check("...and str() names one of them", str(err) == p[0]) +# --------------------------------------------------------------------------- # +print("the LATE refusal, when the device only shows up after the gate has run") +# ⚠️ This path had no test at all and no live demonstration - the review's +# catch. ESPHome is routinely not up when the add-on boots, so hass.limits() +# returns (None, None), the range checks are skipped, and run_control() is the +# first place the device's real rating is ever known. Everything below is about +# what happens when it turns out to conflict. +from app.main import Controller # noqa: E402 + + +class _LateHass: + """Reports the range only run_control() gets to see.""" + + def __init__(self, lo, hi): + self.lo, self.hi = lo, hi + self.writes = [] + + async def limits(self, entity): + return (self.lo, self.hi, 1.0) + + async def number(self, *a, **k): + return 0.0 + + async def set_number(self, entity, value): + self.writes.append(value) + return True + + +class _Store2: + data = {} + + def set(self, *a): + pass + + +class _Pub2: + enabled = False + + def publish(self, *a, **k): + pass + + def close(self): + pass + + +def late(opts_over, lo, hi): + """Drive run_control() against a device that only reports (lo, hi) now. + + ⚠️ Returns whether run_control() EXITED ON ITS OWN. That is the assertion + that matters: on a refusal it must return without entering the while loop, + and on a clean config it must not return at all. So the clean case is stopped + from outside after it has had time to write, rather than waited on - a + wait_for() here would just time out, which is the correct behaviour dressed + up as a failure. + """ + async def go(): + stop = asyncio.Event() + hass = _LateHass(lo, hi) + # A device narrower than the shipped 5000 W schema ceiling is exactly + # the case that cannot be reached through the Supervisor on the live + # install, so it is reached here instead. + c = Controller(base(**opts_over), hass, _Store2(), _Pub2(), stop=stop) + c.auto = True + task = asyncio.create_task(c.run_control()) + for _ in range(30): # up to ~3 s + if task.done(): + break + await asyncio.sleep(0.1) + exited = task.done() + if not exited: + c.stopping = True + await asyncio.wait_for(task, timeout=5) + return c, stop, hass, exited + return asyncio.run(go()) + +c, stop, hass, exited = late({"max_w": 5000}, -3600.0, 3600.0) +check("a device narrower than max_w, discovered late, is refused", + c.refused is not None and len(c.refused) == 1) +check("...naming both values", why(c.refused, "5000") and why(c.refused, "3600")) +check("...signalling amain to wind down", stop.is_set()) +check("...and returning instead of looping forever", exited) +# The whole point: run_control() must not fall through into the while loop. A +# single write here would mean it commanded the inverter under a config it had +# just declared unusable. +check("...having never entered the control loop, so it wrote nothing", + hass.writes == []) +check("...and it is recorded in the event log an operator reads", + any("REFUSED" in e for e in c.events)) + +# The same path when there is no conflict: it must NOT refuse, and it must +# actually regulate - asserted by a write reaching the device, not by absence. +c2, stop2, hass2, exited2 = late({"max_w": 2000}, -5000.0, 5000.0) +check("a late device that agrees does not trip the re-check", c2.refused is None) +check("...and the control loop runs, writing to the device", hass2.writes != []) +check("...and does not signal a shutdown", not stop2.is_set() and not exited2) + +# ⚠️ THE FOOTGUN THE REVIEW FOUND. `stop` used to default to None, so this +# construction was legal and the refusal path then returned into a live process +# with no control loop, no exit and no further output. +try: + Controller(base(), _LateHass(-5000.0, 5000.0), _Store2(), _Pub2()) + check("a Controller with no stop Event is rejected at construction", False) +except TypeError: + check("a Controller with no stop Event is rejected at construction", True) + # --------------------------------------------------------------------------- # print() if fails: