From ffb8ec6373fae742e937d3764de48e01311d43fa Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Sun, 30 Aug 2026 02:51:40 +0200 Subject: [PATCH] MAINT-04 review: pin the default divergence with a check, not a comment validate.py's comment said "if config.yaml's value moves, move this and main.py's together" and nothing enforced it - a comment where a check belonged, in a PR that elsewhere proves it knows the difference. test_validate.py now reads main.py and maintenance.py directly and asserts the divergence EXACTLY AS IT STANDS: config.yaml/validate.py at 2000, main.py's two fallbacks and MaintConfig's dataclass default at 2500. It records the mismatch rather than demanding it be closed, so it passes today and goes red if either side moves - including when somebody closes it in main.py, at which point the check and validate.py's block are deleted together. Nothing in a file another ticket owns is touched. The consequence is asserted through the real code as well as the literals: with the key absent the validator accepts, MaintConfig builds 2500 W against a 2000 W ceiling, and the arbiter resolves it to 0 W as a contradiction - the bug rule 6 exists to catch, passing rule 6. Production-unreachable (the Supervisor always supplies every key) but fully reachable from sim/inject.py and the tests, which is where anyone debugging maintenance works. Also: a CHANGELOG section for the upgrade path, covering both the case where the Supervisor re-derives the default and the case where it persisted the old one, since which applies was not verifiable without probing the live install. And maintenance.py's timeout comment said 13.5 kWh where everything else says 13.8 (300 Ah x 51.2 V, DoD 90); SAFETY-18 does not touch that file. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015VXjA3LWccN1rJsk3okyPC --- goodwe_controller/CHANGELOG.md | 31 +++++++++++++ goodwe_controller/app/maintenance.py | 7 +-- goodwe_controller/app/validate.py | 8 +++- goodwe_controller/test_validate.py | 68 ++++++++++++++++++++++++++++ 4 files changed, 109 insertions(+), 5 deletions(-) diff --git a/goodwe_controller/CHANGELOG.md b/goodwe_controller/CHANGELOG.md index 96a7815..d312405 100644 --- a/goodwe_controller/CHANGELOG.md +++ b/goodwe_controller/CHANGELOG.md @@ -43,6 +43,37 @@ faster maintenance cycle — raise it first, then these. `test_validate.py` now reads `config.yaml` itself and asserts the shipped numbers pass the rule, so a default that the validator refuses cannot ship again. +### ⚠️ Upgrading an existing default install: the add-on may refuse to start + +An install still carrying `maintenance_charge_w: 2500` against `max_w: 2000` +now meets rule 6 and **exits 78 with the refusal banner** instead of starting. +That is the intended behaviour — the configuration was already broken, it was +just silent about it — but an add-on that will not come back after an update +needs the fix stated here rather than only in the log. + +**The fix, either way: set `maintenance_charge_w` to `2000` or lower, OR raise +`max_w` to `2500` or higher** (Settings → Add-ons → GoodWe RS485 Controller → +Configuration), then start it. The banner in the log says the same thing and +names both numbers. + +Whether you hit this at all depends on whether the Supervisor kept the old +value, and **both cases are covered above because we did not verify which one +applies here.** The Supervisor merges an add-on's *stored* options over the +defaults in `config.yaml`, so: + +- **A key the operator never edited** re-derives from the new `config.yaml` and + becomes 2000 on upgrade. Non-event, nothing to do. +- **A key the operator did set** persists at 2500 and the add-on refuses to + start until it is changed. Note this includes anyone who used the + Configuration page's YAML editor, which writes the whole options block back — + so "I never touched that field" is not a guarantee the old value is gone. + +⚠️ **This is reasoned from the Supervisor's documented merge behaviour, not +measured on the live install.** The reference install's stored options could not +be read (`/api/hassio/` refuses a long-lived token) and probing it was out of +scope — it regulates a real battery. It runs `max_w: 5000` and is unaffected +either way. + ⚠️ **Known, deliberately left, and worth a follow-up:** `main.py` and `maintenance.py` still fall back to **2500** when the key is absent from the options dict, while `config.yaml` and `validate.py` now say 2000. Unreachable on diff --git a/goodwe_controller/app/maintenance.py b/goodwe_controller/app/maintenance.py index b12669d..5119b0e 100644 --- a/goodwe_controller/app/maintenance.py +++ b/goodwe_controller/app/maintenance.py @@ -28,9 +28,10 @@ _LOG = logging.getLogger("goodwe.maint") IDLE, DRAIN, CHARGE, HOLD = "idle", "drain", "charge", "hold" -# A drain of a 13.5 kWh usable pack at 2500 W is ~5 h even with no house load -# helping. 12 h means the battery is not following commands - a fault, not a -# slow day. The charge phase deliberately waits for sun and for cheap hours, so +# A drain of a 13.8 kWh usable pack (300 Ah x 51.2 V, DoD 90) at the shipped +# 2000 W is ~6.9 h even with no house load helping - and less in practice, since +# house load drains alongside the export and soc_floor stops short of empty. +# 12 h means the battery is not following commands - a fault, not a slow day. The charge phase deliberately waits for sun and for cheap hours, so # it is expected to span a night; 36 h still bounds it. DRAIN_TIMEOUT_MIN = 720 CHARGE_TIMEOUT_MIN = 2160 diff --git a/goodwe_controller/app/validate.py b/goodwe_controller/app/validate.py index d4f285d..b17b449 100644 --- a/goodwe_controller/app/validate.py +++ b/goodwe_controller/app/validate.py @@ -327,8 +327,12 @@ def validate_startup_config(opts: dict, dev_min=None, dev_max=None) -> list[str] # rule at 2000 and then built by main.py at 2500 - the contradiction this # rule exists to catch, in the one case it cannot see. It is unreachable on # a Supervisor install and those files are another ticket's right now. - # THE REAL FIX is one source of truth for option defaults; until then, if - # config.yaml's value moves, move this and main.py's together. + # THE REAL FIX is one source of truth for option defaults; until then the + # divergence is PINNED BY A CHECK rather than by this comment - see + # test_validate.py, "the KNOWN cross-file default divergence". It asserts + # the mismatch as it stands and goes red if EITHER side moves, including + # when somebody closes it in main.py, at which point this block and that + # check both get deleted. charge_w = _number(opts, "maintenance_charge_w", 2000.0) if charge_w is None: problems.append( diff --git a/goodwe_controller/test_validate.py b/goodwe_controller/test_validate.py index d314d7c..23a029e 100644 --- a/goodwe_controller/test_validate.py +++ b/goodwe_controller/test_validate.py @@ -253,6 +253,74 @@ check("...and the shipped charge rate is at or under the shipped ceiling", check("...and the shipped discharge rate too, so it warns about nothing", _shipped["maintenance_discharge_w"] <= _shipped["max_w"]) +# --------------------------------------------------------------------------- # +print("the KNOWN cross-file default divergence, pinned rather than commented") +# ⚠️ WHAT THIS GUARDS, AND WHY IT ASSERTS THE BUG RATHER THAN THE FIX. +# config.yaml and validate.py now default maintenance_charge_w to 2000, while +# main.py's two `opts.get(..., 2500)` fallbacks and MaintConfig's dataclass +# default still say 2500. So an options dict with the key ABSENT is ACCEPTED by +# rule 6 at 2000 and then built by main.py at 2500 - which is a contradiction: +# +# validate_startup_config -> [] (accepted) +# MaintConfig().charge_w -> 2500.0 +# arbiter -> 0.0 W, contradiction=True +# +# That is the bug rule 6 exists to catch, passing rule 6. It is unreachable on a +# Supervisor install (every key is always supplied from config.yaml) but fully +# reachable from sim/inject.py and from tests - which is exactly where anyone +# debugging maintenance works. +# +# The fix is ONE SOURCE OF TRUTH for option defaults. That is a bigger change +# than this ticket and it lands in main.py, which another ticket is live in - so +# the divergence is RECORDED here instead of closed. The check therefore passes +# on the mismatch and FAILS when either side moves, in EITHER direction: +# +# - config.yaml moves -> FAIL: re-decide what the fallback should be +# - main.py is fixed to match -> FAIL: the divergence is gone, DELETE this +# check and the ⚠️ block in validate.py +# +# A comment cannot do that, and "a comment where a check belonged" is this +# repo's most repeated defect. +_APP = os.path.join(os.path.dirname(os.path.abspath(__file__)), "app") + +def _read(*parts): + with open(os.path.join(*parts), encoding="utf-8") as fh: + return fh.read() + +# Every literal fallback for this key in main.py, whatever the call shape. +_main_falls = [float(m) for m in re.findall( + r'\.get\(\s*"maintenance_charge_w"\s*,\s*(-?\d+(?:\.\d+)?)\s*\)', + _read(_APP, "main.py"))] +# ...and MaintConfig's own dataclass default, which is the one that bites when +# main.py is bypassed entirely (a Maintenance built straight from defaults). +_dc_falls = [float(m) for m in re.findall( + r"^\s*charge_w:\s*float\s*=\s*(-?\d+(?:\.\d+)?)\s*$", + _read(_APP, "maintenance.py"), re.M)] + +check(f"main.py's fallbacks are still found and still 2500 {_main_falls}", + len(_main_falls) == 2 and set(_main_falls) == {2500.0}) +check(f"MaintConfig's default is still found and still 2500 {_dc_falls}", + len(_dc_falls) == 1 and set(_dc_falls) == {2500.0}) +check(f"...and config.yaml still ships 2000, so the divergence is UNCHANGED " + f"(shipped {_shipped['maintenance_charge_w']:.0f}, code 2500)", + _shipped["maintenance_charge_w"] == 2000.0) + +# ⚠️ And the consequence itself, through the REAL code rather than through the +# numbers above - so that if someone "fixes" this by changing a literal without +# understanding it, the thing that goes red is the behaviour, not the transcript. +from app.maintenance import MaintConfig # noqa: E402 + +_absent = {k: v for k, v in base().items() if k != "maintenance_charge_w"} +_built = MaintConfig().charge_w +check("with the key ABSENT the validator accepts...", + validate_startup_config(_absent, -5000, 5000) == []) +check(f"...while the code would build {_built:.0f} W against a " + f"{_absent['max_w']:.0f} W ceiling...", _built > _absent["max_w"]) +_r = _charge(_absent["max_w"], _built) +check("...which the arbiter resolves to 0 W as a contradiction - the hole, " + "asserted so it cannot widen unnoticed", + _r.target_w == 0.0 and _r.contradiction) + # --------------------------------------------------------------------------- # print("max_w must be a positive magnitude") # Schema-blocked on a real install, but validate_startup_config() is also