From fc9bd89dcae43a935a1807c84da91b5695c4af65 Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Fri, 28 Aug 2026 19:29:35 +0200 Subject: [PATCH] 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")