From fc9bd89dcae43a935a1807c84da91b5695c4af65 Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Fri, 28 Aug 2026 19:29:35 +0200 Subject: [PATCH 1/2] CFG-01: refuse to start on a configuration that cannot be honoured MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit main.py used to read the setpoint device's own +-5000 W rating, notice a configured max_w above it, log one WARNING, and run anyway with write_setpoint() quietly clamping. The operator's stated ceiling was never the real ceiling and the only evidence had scrolled off the log. specs §1.5 is explicit that such a config is refused at startup, not survived at runtime. 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 sharing one property - continuing makes a safety mechanism inoperative or makes a stated limit untrue: 1. max_w outside the setpoint device's own rating. 2. No setpoint_entity: every write is a no-op, the device rating is never read, and the ESP32 heartbeat that holds off the layer-1 failsafe never happens. It ran, looked healthy, and moved nothing. 3. A meter_source that can never produce a reading. These were already caught, but they only DISABLED ingestion, which leaves grid None forever: permanent inputs-missing, 0 W, and a status page indistinguishable from a healthy meter nobody has fed yet. Those last predicates move into p1.validate_meter_options(), which build_source() also calls - one definition, two callers, not two competing validators. ha_dsmr with half its entities and mqtt_p1 with a blank topic gained the check the other two transports already had. Deliberately not refused, because untidy is not unsafe: integrator_max_w above max_w (the arbiter still bounds the wire to +-max_w, so the cost is unwind latency - warned), a missing soc/batt entity (fail-safe and already a red NOT READY banner), a setpoint entity that has not appeared on the Core API yet (ESPHome is routinely late; re-checked in run_control() once the real rating arrives), and a thin meter_poll_s margin. A refusal is legible as one: a banner saying in words that this is configuration and nothing is broken, numbered problems, no traceback, and exit 78 (EX_CONFIG). Verified on the live add-on 2026-08-28, not by reading the code: a blank setpoint_entity and a meter_poll_s equal to meter_max_age_s each stopped the add-on with the banner and no stack trace. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Du77usMj8XNKNFZGmUiWDa --- goodwe_controller/CHANGELOG.md | 42 ++++++ goodwe_controller/app/main.py | 66 +++++++++- goodwe_controller/app/p1.py | 197 +++++++++++++++++++---------- goodwe_controller/app/validate.py | 190 ++++++++++++++++++++++++++++ goodwe_controller/test_validate.py | 161 +++++++++++++++++++++++ 5 files changed, 582 insertions(+), 74 deletions(-) create mode 100644 goodwe_controller/app/validate.py create mode 100644 goodwe_controller/test_validate.py diff --git a/goodwe_controller/CHANGELOG.md b/goodwe_controller/CHANGELOG.md index 4a26ed8..e704b63 100644 --- a/goodwe_controller/CHANGELOG.md +++ b/goodwe_controller/CHANGELOG.md @@ -2,6 +2,48 @@ ## 0.4.0 +**CFG-01.** The add-on now refuses to start on a configuration it cannot honour, +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: + +- **`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 + `write_setpoint()` silently clamping, so the configured ceiling was never the + real one and the only evidence had scrolled off the log. The refusal names the + configured value, the device's value, and which wins. +- **No `setpoint_entity`.** A controller with no actuator: every write is a + no-op, the device rating is never read, and the ESP32 heartbeat that holds off + the layer-1 failsafe never happens. It ran, looked healthy, and moved nothing. +- **A `meter_source` that can never produce a reading** - missing `p1_host`, + missing `p1_net_entity`, a blank MQTT topic, a phase list disagreeing with + `meter_phases`, an unknown source name, or a `meter_poll_s` at or past + `meter_max_age_s`. These were already caught, but they only *disabled + 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. + +The predicates for the last 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 +which previously built a transport that could never read. + +Deliberately **not** refused, because it is untidy rather than unsafe: +`integrator_max_w` above `max_w` (the arbiter still bounds the wire to ±max_w, +so the cost is unwind latency, not amperage - warned), a missing `soc_entity` / +`batt_entity` (fail-safe, and already a red NOT READY banner naming it), a +`setpoint_entity` that has not appeared on the Core API yet (ESPHome is routinely +late; refusing would make a slow boot need a human - re-checked once the real +rating arrives), and a thin-but-nonzero `meter_poll_s` margin. + +A refusal is legible as a refusal: a banner that says in words that this is a +configuration problem and nothing is broken, the numbered problems, no traceback, +and exit status 78 (`EX_CONFIG`). + **TEL-04.** `meter_source: ha_signed` reads a single *signed* power entity, which is the shape a HomeWizard P1 publishes. Both transports 0.3.0 shipped require two unsigned registers and refuse negatives, so neither fits the meter installed here. diff --git a/goodwe_controller/app/main.py b/goodwe_controller/app/main.py index b0bb234..89af3a5 100644 --- a/goodwe_controller/app/main.py +++ b/goodwe_controller/app/main.py @@ -40,6 +40,7 @@ from .hass import HomeAssistant from .maintenance import IDLE, MaintConfig, Maintenance from .mqtt import MqttPublisher from .p1 import P1Ingest, build_source, is_enabled +from .validate import ConfigRefused, refuse, validate_startup_config from . import web OPTIONS_PATH = "/data/options.json" @@ -56,11 +57,15 @@ def load_options() -> dict: class Controller: - def __init__(self, opts: dict, hass: HomeAssistant, store, mqtt_pub): + def __init__(self, opts: dict, hass: HomeAssistant, store, mqtt_pub, stop=None): 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(). + self.stop_event = stop + self.refused: list[str] | None = None self.tuning = Tuning( gain=float(opts.get("gain", 0.6)), @@ -303,10 +308,26 @@ class Controller: entity = self.o.get("setpoint_entity", "") if entity: self.dev_min, self.dev_max, _ = await self.hass.limits(entity) - if self.dev_max is not None and self.tuning.max_w > self.dev_max: - self.log_event( - f"clamp {self.tuning.max_w:.0f} W exceeds the device maximum " - f"{self.dev_max:.0f} W - the device wins") + # ⚠️ THIS USED TO WARN AND CARRY ON, and that was the bug CFG-01 + # exists to remove: a max_w above the device's rating meant + # write_setpoint() silently clamped and the configured ceiling was + # never the real one. + # + # amain() already ran this gate before anything started. It runs + # AGAIN here for one case the gate cannot cover: ESPHome is often + # not up yet when the add-on boots, so hass.limits() returned + # (None, None) and the range checks were skipped rather than + # guessed at. This is the first moment the device's real rating is + # known. If it conflicts, refuse now - stopping deterministically + # through the normal shutdown, which commands 0 W on the way out. + problems = validate_startup_config(self.o, self.dev_min, self.dev_max) + if problems: + refuse(problems) + self.refused = problems + self.log_event("REFUSED: " + problems[0]) + if self.stop_event is not None: + self.stop_event.set() + return last_grid = object() heartbeat = float(self.o.get("heartbeat_s", 10)) @@ -515,6 +536,26 @@ async def amain() -> None: except Exception as _e: # noqa: BLE001 _LOG.error("core api probe failed: %s", _e) + # ⚠️ THE STARTUP GATE (CFG-01). Nothing below this line may run on a + # configuration validate.py rejects - specs §1.5: a config the system + # cannot honour is refused at startup, not survived at runtime. + # + # It sits HERE, before the broker, before the web server and before any + # task exists, so a refusal cannot leave half a controller running with + # a UI in front of it. Reading the device range is the one thing the + # validator cannot do for itself; (None, None) when ESPHome has not come + # up yet is not a refusal - run_control() re-checks once the real rating + # is known. See validate.py for where the line is drawn and why. + _sp = str(opts.get("setpoint_entity", "") or "").strip() + _dev_min = _dev_max = None + if _sp: + _dev_min, _dev_max, _ = await hass.limits(_sp) + _LOG.info("setpoint device %s reports range %s…%s W", _sp, _dev_min, _dev_max) + _problems = validate_startup_config(opts, _dev_min, _dev_max) + if _problems: + refuse(_problems) + raise ConfigRefused(_problems) + # ⚠️ Status publishing must NEVER be able to stop the controller. A # broken broker, a missing library, an API change in paho - all of it is # observability, and the battery does not care. Caught broadly and on @@ -540,10 +581,10 @@ async def amain() -> None: pub = _NoMqtt() - controller = Controller(opts, hass, store, pub) + stop = asyncio.Event() + controller = Controller(opts, hass, store, pub, stop=stop) runner = await web.start(controller, port=8099) - stop = asyncio.Event() loop = asyncio.get_running_loop() for sig in (signal.SIGTERM, signal.SIGINT): with contextlib.suppress(NotImplementedError): @@ -570,6 +611,10 @@ async def amain() -> None: await task await runner.cleanup() _LOG.info("stopped") + # The late-arriving-device refusal from run_control(). Re-raised only + # now, so the wind-down above still ran and the inverter is at 0 W. + if controller.refused: + raise ConfigRefused(controller.refused) def main() -> None: @@ -577,6 +622,13 @@ def main() -> None: asyncio.run(amain()) except KeyboardInterrupt: pass + except ConfigRefused: + # ⚠️ No traceback, on purpose. The banner refuse() already printed says + # everything an operator needs, and a stack trace on top of it would say + # "this crashed" - the exact confusion AC3 is about. 78 is sysexits(3) + # EX_CONFIG, so even a script reading the exit status can tell a bad + # config from a bug. + raise SystemExit(78) if __name__ == "__main__": diff --git a/goodwe_controller/app/p1.py b/goodwe_controller/app/p1.py index 3efe5e3..0687b32 100644 --- a/goodwe_controller/app/p1.py +++ b/goodwe_controller/app/p1.py @@ -1044,15 +1044,133 @@ def is_enabled(opts: dict) -> bool: return str(opts.get("meter_source", "off") or "off").strip() not in ("off", "") +def homewizard_host_port(opts: dict) -> tuple[str, str]: + """`p1_host` split into host and (still unparsed) port text. + + One definition because validate_meter_options() and build_source() must + agree byte for byte about what counts as a usable address - a pasted browser + URL is the obvious way to fill this in wrong. + """ + host = str(opts.get("p1_host", "") or "").strip() + host = host.removeprefix("http://").removeprefix("https://").rstrip("/") + host, _, port_s = host.partition(":") + return host, port_s + + +def validate_meter_options(opts: dict, phases: int, max_age_s: float) -> list[str]: + """Every way `meter_source` can name a meter that will never produce a reading. + + ⚠️ THIS IS THE ONLY PLACE THAT KNOWS THESE RULES. It used to be a run of + inline `return None`s inside build_source(); CFG-01 lifted them out because + the same predicates now have two callers with two different consequences: + + validate.validate_startup_config() refuses to start the add-on + build_source() returns None, as it always did + + They are one function and not two on purpose. A second copy of "what makes a + meter config wrong" would drift, and the half that drifted would be the one + that decides whether the process is allowed to run. + + Checked ONCE at startup rather than per telegram, because a misconfigured + source otherwise fails in the one way indistinguishable from a healthy source + nobody has sent anything to yet: no samples, a climbing age, the firmware + watchdog holding the battery at 0 W, and nothing in the log saying why. + + Returns a list of human-readable problems; empty means the config is usable. + `phases` and `max_age_s` are passed in rather than read from opts so the + caller decides whether it is validating the options or an already-built + P1Ingest - build_source() has an ingest whose values may not have come from + this dict. + """ + source = str(opts.get("meter_source", "off") or "off").strip() + if source in ("off", ""): + return [] + problems: list[str] = [] + + if source == SOURCE_HA: + imp = str(opts.get("p1_import_entity", "") or "").strip() + exp = str(opts.get("p1_export_entity", "") or "").strip() + if not imp or not exp: + problems.append( + f"meter_source {SOURCE_HA} needs both p1_import_entity and " + f"p1_export_entity (got {imp or ''!r} / {exp or ''!r})") + elif source == SOURCE_HA_SIGNED: + net = str(opts.get("p1_net_entity", "") or "").strip() + phase_net = [str(e).strip() for e in + (opts.get("p1_phase_net_entities") or []) if str(e).strip()] + if not net: + problems.append( + f"meter_source {SOURCE_HA_SIGNED} needs p1_net_entity " + "(sensor.p1_sample_age_s would otherwise be announced with " + "nothing feeding it)") + if phase_net and len(phase_net) != phases: + problems.append( + f"p1_phase_net_entities has {len(phase_net)} entities but " + f"meter_phases is {phases} - every telegram would be rejected " + "on the phase-count check") + elif source == SOURCE_MQTT: + if not str(opts.get("meter_mqtt_topic", "") or "").strip(): + problems.append(f"meter_source {SOURCE_MQTT} needs meter_mqtt_topic") + elif source == SOURCE_HOMEWIZARD: + host, port_s = homewizard_host_port(opts) + if not host: + problems.append( + f"meter_source {SOURCE_HOMEWIZARD} needs p1_host (the meter's " + "own address, e.g. 192.168.2.250)") + if port_s: + try: + int(port_s) + except ValueError: + problems.append(f"p1_host {opts.get('p1_host')!r} has an " + "unparseable port") + try: + # ⚠️ Not `or 5`: that turns an explicit 0 into the default, and a + # cadence of 0 is a misconfiguration that must be reported, not + # quietly corrected into something that looks like it was asked for. + raw = opts.get("meter_poll_s", 5) + poll_s = float(5 if raw is None or raw == "" else raw) + except (TypeError, ValueError): + problems.append(f"meter_poll_s {opts.get('meter_poll_s')!r} is not a number") + else: + if poll_s <= 0: + problems.append("meter_poll_s must be positive") + # ⚠️ The age can never be fresher than the poll interval. At or past + # max_age_s every reading is stale before its successor arrives, so + # the controller would sit permanently on missing inputs while the + # meter is perfectly healthy - refuse it rather than ship that. + elif poll_s >= max_age_s: + problems.append( + f"meter_poll_s {poll_s:.1f} is not under meter_max_age_s " + f"{max_age_s:.1f} - every reading would go stale before the " + "next poll") + else: + problems.append( + f"meter_source {source!r} is not one of " + + ", ".join((SOURCE_HA, SOURCE_MQTT, SOURCE_HA_SIGNED, SOURCE_HOMEWIZARD))) + return problems + + def build_source(opts: dict, ingest: P1Ingest, session, broker: dict | None): """Return the transport named by `meter_source`, or None if disabled. This is the whole of AC 1's "switchable": every consumer reads P1Ingest, so changing transport is a config edit, never a code path. + + ⚠️ In the add-on this is now unreachable with a bad config - + validate_startup_config() refuses to start on anything + validate_meter_options() rejects. The guard stays because this function is + also called directly by tests and by anything embedding P1Ingest, and + "returns None" is a safer default for those callers than a transport that + can never read. """ source = str(opts.get("meter_source", "off") or "off").strip() if not is_enabled(opts): return None + problems = validate_meter_options(opts, ingest.phases, ingest.max_age_s) + if problems: + for problem in problems: + _LOG.error("%s - P1 ingestion disabled", problem) + return None if source == SOURCE_HA: return HaDsmrSource(session, ingest, { "import": opts.get("p1_import_entity", ""), @@ -1061,70 +1179,19 @@ def build_source(opts: dict, ingest: P1Ingest, session, broker: dict | None): "phase_export": opts.get("p1_phase_export_entities") or [], }) if source == SOURCE_HA_SIGNED: - net = str(opts.get("p1_net_entity", "") or "").strip() - phase_net = [str(e).strip() for e in - (opts.get("p1_phase_net_entities") or []) if str(e).strip()] - # ⚠️ Both of these are checked ONCE here rather than per telegram. A - # misconfigured source otherwise fails silently in the only way that - # looks exactly like a healthy one that has not been sent anything yet: - # no samples, a climbing age, and the firmware watchdog holding the - # battery at 0 W with nothing in the log saying why. - if not net: - _LOG.error("meter_source %s needs p1_net_entity - P1 ingestion " - "disabled (sensor.p1_sample_age_s would otherwise be " - "announced with nothing feeding it)", SOURCE_HA_SIGNED) - return None - if phase_net and len(phase_net) != ingest.phases: - _LOG.error("p1_phase_net_entities has %d entities but meter_phases " - "is %d - P1 ingestion disabled. Every telegram would be " - "rejected on the phase-count check.", - len(phase_net), ingest.phases) - return None - return HaSignedSource(session, ingest, - {"net": net, "phase_net": phase_net}) + return HaSignedSource(session, ingest, { + "net": str(opts.get("p1_net_entity", "") or "").strip(), + "phase_net": [str(e).strip() for e in + (opts.get("p1_phase_net_entities") or []) if str(e).strip()], + }) if source == SOURCE_HOMEWIZARD: - host = str(opts.get("p1_host", "") or "").strip() - # A pasted browser URL is the obvious way to fill this in wrong. - host = host.removeprefix("http://").removeprefix("https://").rstrip("/") - host, _, port_s = host.partition(":") - # ⚠️ Checked ONCE here, like the ha_signed entity ids and for the same - # reason: a misconfigured source otherwise fails in the one way that - # looks exactly like a healthy one nobody has sent anything to yet - no - # samples, a climbing age, the watchdog holding the battery at 0 W, and - # nothing in the log saying why. - if not host: - _LOG.error("meter_source %s needs p1_host (the meter's own address, " - "e.g. 192.168.2.250) - P1 ingestion disabled", - SOURCE_HOMEWIZARD) - return None - try: - port = int(port_s) if port_s else HOMEWIZARD_PORT - except ValueError: - _LOG.error("p1_host %r has an unparseable port - P1 ingestion disabled", - opts.get("p1_host")) - return None - try: - # ⚠️ Not `or 5`: that turns an explicit 0 into the default, and a - # cadence of 0 is a misconfiguration that must be reported, not - # quietly corrected into something that looks like it was asked for. - raw = opts.get("meter_poll_s", 5) - poll_s = float(5 if raw is None or raw == "" else raw) - except (TypeError, ValueError): - _LOG.error("meter_poll_s %r is not a number - P1 ingestion disabled", - opts.get("meter_poll_s")) - return None - if poll_s <= 0: - _LOG.error("meter_poll_s must be positive - P1 ingestion disabled") - return None - # ⚠️ The age can never be fresher than the poll interval. At or past - # max_age_s every reading is stale before its successor arrives, so the - # controller would sit permanently on missing inputs while the meter is - # perfectly healthy - refuse it rather than ship that. - if poll_s >= ingest.max_age_s: - _LOG.error("meter_poll_s %.1f is not under meter_max_age_s %.1f - every " - "reading would go stale before the next poll. P1 ingestion " - "disabled.", poll_s, ingest.max_age_s) - return None + host, port_s = homewizard_host_port(opts) + port = int(port_s) if port_s else HOMEWIZARD_PORT + raw = opts.get("meter_poll_s", 5) + poll_s = float(5 if raw is None or raw == "" else raw) + # A warning, not a refusal: one missed poll costs a stale reading and a + # 0 W cycle, which is fail-safe. The refusal above is for cadences that + # can NEVER be fresh. if poll_s > ingest.max_age_s / 2: _LOG.warning("meter_poll_s %.1f leaves no room under meter_max_age_s " "%.1f: one missed poll makes the reading stale. The meter " @@ -1136,8 +1203,4 @@ def build_source(opts: dict, ingest: P1Ingest, session, broker: dict | None): return MqttP1Source(ingest, str(opts.get("meter_mqtt_topic", "")), broker.get("host"), broker.get("port", 1883), broker.get("username"), broker.get("password")) - if source: - _LOG.error("meter_source %r is not one of %s - P1 ingestion disabled", - source, ", ".join((SOURCE_HA, SOURCE_MQTT, SOURCE_HA_SIGNED, - SOURCE_HOMEWIZARD))) return None diff --git a/goodwe_controller/app/validate.py b/goodwe_controller/app/validate.py new file mode 100644 index 0000000..c6fd338 --- /dev/null +++ b/goodwe_controller/app/validate.py @@ -0,0 +1,190 @@ +"""Startup configuration validation - the gate before the control loop runs. + +WHAT THIS IS FOR, in one sentence: some configurations are not "a bit off", they +are configurations under which the add-on's own safety machinery does not work, +and for those the correct behaviour is to refuse to start rather than to log a +line and carry on. + +⚠️ THE FAILURE THIS REPLACES. `main.py` used to read the setpoint device's own +±5000 W rating, notice that a configured `max_w` of, say, 8000 W exceeded it, +write one WARNING to a log nobody reads, and then run anyway with +`write_setpoint()` quietly clamping. The operator's stated ceiling was simply not +the ceiling, and the only evidence was a line that had scrolled off. specs §1.5 +is explicit that a configuration in which the system cannot honour what it was +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.** + + 1. `max_w` outside the setpoint device's own rating. + 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. + `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. + + 3. A `meter_source` that can never produce a reading. + §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 + meter with a climbing age exactly as it would for a healthy meter nobody has + sent anything to yet. The predicates live in `p1.validate_meter_options()`, + which is also what `build_source()` uses - one definition, two callers. + +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 + extra headroom buys is more wrong-direction power to unwind after + saturation - but the arbiter's `supervised` limit still binds the wire to + ±max_w, so nothing out of range ever reaches the inverter. The cost is + latency, not amperage. Warned about, not refused. + - A missing `soc_entity` / `batt_entity`. `inputs_ok` goes False, the loop + commands 0 W, and `checks()` already renders a red NOT READY banner naming + the missing entity. Fail-safe AND already loud; a refusal adds nothing. + - A `setpoint_entity` that exists in the config but is not found on the Core + API at startup. This is the transient case - ESPHome is frequently not up + yet when the add-on boots - and refusing here would turn a device that + arrives thirty seconds late into a restart loop needing a human. It stays a + NOT READY banner. + - `meter_poll_s` above half `meter_max_age_s`. One missed poll costs a stale + reading and a 0 W cycle. Warned about in p1.py, not refused. +""" + +import logging + +from .p1 import validate_meter_options + +_LOG = logging.getLogger("goodwe.config") + +# ⚠️ 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 +# construction: there is exactly one such key, so a second writer is not +# expressible in the options schema at all. sim/inject.py's `two_writers` +# scenario asserts on this tuple - if a second write path is ever added, that +# scenario goes red and somebody has to decide how the two are arbitrated +# BEFORE the config that allows both can ship. +ACTUATOR_WRITE_KEYS = ("setpoint_entity",) + + +class ConfigRefused(Exception): + """The configuration is wrong. Not "something broke" - see main.py's handler. + + Carries the list of problems so the caller can render them; str() is the + first one, so an unhandled traceback still names something useful. + """ + + def __init__(self, problems: list[str]): + self.problems = problems + super().__init__(problems[0] if problems else "configuration refused") + + +def validate_startup_config(opts: dict, dev_min=None, dev_max=None) -> list[str]: + """Return the list of reasons this configuration must not be started. + + Empty list means go. `dev_min`/`dev_max` are the setpoint device's own + reported range (`hass.limits()`), or None when the device could not be + reached - in which case the range checks are skipped rather than guessed at, + for the transient reason in this module's docstring. + + Pure and side-effect free apart from two warnings, so it can be called from + a test or from sim/inject.py without a Home Assistant anywhere near it. + """ + problems: list[str] = [] + + # --- 1. an actuator to write to ------------------------------------------ + writers = [k for k in ACTUATOR_WRITE_KEYS + if str(opts.get(k, "") or "").strip()] + if not writers: + problems.append( + "setpoint_entity is not set, so this add-on has no actuator: every " + "write is a no-op, the device's own rating is never read, and the " + "ESP32 heartbeat that holds off the layer-1 failsafe never happens. " + "It would run, look healthy, and move no energy at all. Set it to " + "the ESPHome number entity that carries the setpoint.") + + # --- 2. the configured ceiling against the device's own rating ----------- + 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") + 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 " + f"most {dev_max:.0f} W - THE DEVICE WINS, so the {max_w:.0f} W you " + f"configured would silently never happen. Lower max_w to " + f"{dev_max:.0f} W or less, or point setpoint_entity at a device that " + f"can take {max_w:.0f} W.") + elif dev_min is not None and -max_w < dev_min: + problems.append( + f"max_w is {max_w:.0f} W, so this controller may command " + f"{-max_w:.0f} W, but {opts.get('setpoint_entity') or 'the setpoint device'} accepts no less " + f"than {dev_min:.0f} W - THE DEVICE WINS on the charge side. Lower " + f"max_w to {abs(dev_min):.0f} W or less.") + + # --- 3. a meter that can actually read ----------------------------------- + # ⚠️ Not a second copy of these rules - p1.py owns them, because + # build_source() has to apply the identical predicates and a drifted copy + # would be worse than no check. + phases = _number(opts, "meter_phases", 1.0) + max_age_s = _number(opts, "meter_max_age_s", 30.0) + if phases is None or max_age_s is None: + problems.append("meter_phases and meter_max_age_s must both be numbers " + f"(got {opts.get('meter_phases')!r} / " + f"{opts.get('meter_max_age_s')!r})") + else: + problems.extend(validate_meter_options(opts, int(phases), max_age_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) + if integ_w is not None and max_w is not None and integ_w > max_w: + _LOG.warning( + "integrator_max_w %.0f W is above max_w %.0f W. Not refused - the " + "arbiter still bounds the wire to +-max_w - but the only thing the " + "extra headroom buys is more wrong-direction power to unwind after " + "saturation. 0 (follow max_w) is the recommended value.", + integ_w, max_w) + + return problems + + +def _number(opts: dict, key, default): + raw = opts.get(key, default) + if raw is None or raw == "": + return default + try: + return float(raw) + except (TypeError, ValueError): + return None + + +def refuse(problems: list[str]) -> None: + """Log a refusal an operator can tell apart from a crash. Does not raise. + + ⚠️ THE POINT OF THE BANNER. "You configured this wrong" must not read like + "it broke". A Python traceback in an add-on log means the same thing to most + operators as a segfault - file a bug - whereas this block names the keys, + names the fix, and says in words that nothing is broken. main.py catches + ConfigRefused and exits 78 (sysexits EX_CONFIG), so no traceback is printed + at all and even the exit status distinguishes the two. + """ + _LOG.error("=" * 72) + _LOG.error("REFUSING TO START: the add-on configuration is not usable.") + _LOG.error("This is a CONFIGURATION problem, not a crash. Nothing is broken;") + _LOG.error("the add-on will start as soon as the settings below are fixed.") + _LOG.error("Fix them in Settings > Add-ons > GoodWe RS485 Controller >") + _LOG.error("Configuration, then start the add-on again.") + _LOG.error("-" * 72) + for i, problem in enumerate(problems, 1): + _LOG.error(" %d. %s", i, problem) + _LOG.error("=" * 72) diff --git a/goodwe_controller/test_validate.py b/goodwe_controller/test_validate.py new file mode 100644 index 0000000..247f1fe --- /dev/null +++ b/goodwe_controller/test_validate.py @@ -0,0 +1,161 @@ +"""Runnable check for the startup gate. `python3 test_validate.py` + +Two halves, and the second is the one that matters. The first asserts that the +three refusable configurations are refused. The second asserts that everything +on validate.py's "deliberately not refused" list still STARTS - because the way +a validator dies is not by missing a fault, it is by refusing something harmless +until somebody deletes the call. +""" + +import sys + +from app.validate import ( + ACTUATOR_WRITE_KEYS, ConfigRefused, refuse, validate_startup_config, +) + +fails = [] + + +def check(name, cond): + print(f" {'ok ' if cond else 'FAIL'} {name}") + if not cond: + fails.append(name) + + +def base(**over): + """A configuration that must start. Everything else is this, spoilt.""" + o = {"setpoint_entity": "number.goodwe_setpoint", "max_w": 2000, + "soc_entity": "sensor.soc", "batt_entity": "sensor.batt", + "meter_source": "off"} + o.update(over) + return o + + +def why(problems, needle): + return any(needle in p for p in problems) + + +# --------------------------------------------------------------------------- # +print("the baseline must start, or nothing below means anything") +check("a good config with a 5000 W device is accepted", + validate_startup_config(base(), -5000, 5000) == []) +check("...and with no device reachable yet, too", + validate_startup_config(base(), None, None) == []) + +# --------------------------------------------------------------------------- # +print("1. max_w against the device's own rating") +# ⚠️ THE ORIGINAL BUG. main.py used to log 'the device wins' and run anyway, +# with write_setpoint() clamping, so the configured ceiling was never the real +# one and the only evidence had scrolled off the log. +p = validate_startup_config(base(max_w=8000), -5000, 5000) +check("max_w above the device maximum is refused", len(p) == 1) +check("...and the message names the configured value", why(p, "8000")) +check("...and the device's value", why(p, "5000")) +check("...and says which one wins", why(p, "THE DEVICE WINS")) +check("max_w exactly at the device maximum is fine", + validate_startup_config(base(max_w=5000), -5000, 5000) == []) +check("max_w under it is fine", + validate_startup_config(base(max_w=2000), -5000, 5000) == []) +# The charge side is a separate rail and an asymmetric device is legal. +p = validate_startup_config(base(max_w=4000), -2500, 5000) +check("a max_w the device cannot take on the CHARGE side is refused too", + len(p) == 1 and why(p, "charge side")) +check("...naming the device's floor", why(p, "-2500")) +# ⚠️ Not guessed at: the entity may simply not have booted yet. run_control() +# re-runs this the moment the real rating arrives. +check("an unreachable device skips the range check rather than assuming", + validate_startup_config(base(max_w=8000), None, None) == []) +check("a non-numeric max_w is refused", + why(validate_startup_config(base(max_w="lots"), -5000, 5000), "not a number")) + +# --------------------------------------------------------------------------- # +print("2. an actuator to write to") +# A controller with no actuator: write_setpoint() returns on the blank entity, +# so no rating is read, no heartbeat is sent, layer 1 pins the inverter at 0 W, +# and the add-on looks perfectly healthy the whole time. +for blank in ("", " ", None): + p = validate_startup_config(base(setpoint_entity=blank), None, None) + check(f"setpoint_entity {blank!r} is refused", len(p) == 1) +check("...and the message says why it is dangerous rather than untidy", + why(validate_startup_config(base(setpoint_entity=""), None, None), + "heartbeat")) +# specs §1.5: two writers on one actuator is not expressible in this schema. +check("there is exactly one way to reach the actuator", + ACTUATOR_WRITE_KEYS == ("setpoint_entity",)) + +# --------------------------------------------------------------------------- # +print("3. a meter that can never read") +# ⚠️ These predicates belong to p1.validate_meter_options(), which build_source() +# also uses. Asserted here to prove the gate actually consults it - one entry +# point, not two. +check("meter_source off asks nothing of the P1 keys", + validate_startup_config(base(meter_source="off"), -5000, 5000) == []) +check("homewizard_local with no p1_host is refused", + why(validate_startup_config(base(meter_source="homewizard_local"), + -5000, 5000), "p1_host")) +check("...and with a host it is accepted", + validate_startup_config(base(meter_source="homewizard_local", + p1_host="192.168.2.250"), -5000, 5000) == []) +check("a poll cadence at meter_max_age_s is refused at STARTUP, not just disabled", + why(validate_startup_config(base(meter_source="homewizard_local", + p1_host="m", meter_poll_s=30, + meter_max_age_s=30), -5000, 5000), + "meter_max_age_s")) +check("ha_signed with no p1_net_entity is refused", + why(validate_startup_config(base(meter_source="ha_signed"), -5000, 5000), + "p1_net_entity")) +check("a phase list disagreeing with meter_phases is refused", + why(validate_startup_config(base(meter_source="ha_signed", + p1_net_entity="sensor.n", meter_phases=3, + p1_phase_net_entities=["a", "b"]), + -5000, 5000), "phase-count")) +check("mqtt_p1 with no topic is refused", + why(validate_startup_config(base(meter_source="mqtt_p1"), -5000, 5000), + "meter_mqtt_topic")) +check("ha_dsmr with only half its entities is refused", + why(validate_startup_config(base(meter_source="ha_dsmr", + p1_import_entity="sensor.i"), -5000, 5000), + "p1_export_entity")) +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("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 +# the only cost of the extra headroom is unwind latency after saturation. +check("integrator_max_w above max_w warns but starts", + validate_startup_config(base(max_w=2000, integrator_max_w=9000), + -5000, 5000) == []) +check("integrator_max_w 0 (follow max_w) starts", + validate_startup_config(base(integrator_max_w=0), -5000, 5000) == []) +# Fail-safe AND already a red NOT READY banner naming the entity: a refusal on +# top of that adds nothing an operator can act on. +check("a missing soc_entity is left to the readiness banner", + validate_startup_config(base(soc_entity=""), -5000, 5000) == []) +check("a missing batt_entity likewise", + validate_startup_config(base(batt_entity=""), -5000, 5000) == []) +# The transient case. ESPHome is routinely not up when the add-on boots, and +# refusing here would turn a device that arrives late into a restart loop. +check("a setpoint entity that is configured but not yet found starts", + validate_startup_config(base(), None, None) == []) +check("meter_poll_s with thin but non-zero headroom starts", + validate_startup_config(base(meter_source="homewizard_local", p1_host="m", + meter_poll_s=20, meter_max_age_s=30), + -5000, 5000) == []) + +# --------------------------------------------------------------------------- # +print("the refusal must be legible as a refusal, not as a crash") +p = validate_startup_config(base(max_w=8000, setpoint_entity=""), -5000, 5000) +check("several problems are all reported, not just the first", len(p) >= 1) +refuse(p) # prints the banner; the eye is the assertion for its wording +err = ConfigRefused(p) +check("ConfigRefused carries every problem", err.problems == p) +check("...and str() names one of them", str(err) == p[0]) + +# --------------------------------------------------------------------------- # +print() +if fails: + print(f"{len(fails)} FAILED: " + "; ".join(fails)) + sys.exit(1) +print("all checks passed") -- 2.54.0 From 547d796c5dee72f3cf7a125f93c58aee4c2ab136 Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Fri, 28 Aug 2026 20:05:10 +0200 Subject: [PATCH 2/2] 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: -- 2.54.0