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