diff --git a/goodwe_controller/CHANGELOG.md b/goodwe_controller/CHANGELOG.md index a9dbfd8..f4b8415 100644 --- a/goodwe_controller/CHANGELOG.md +++ b/goodwe_controller/CHANGELOG.md @@ -2,6 +2,83 @@ ## Unreleased +**Maintenance option defaults lived in three places that had already disagreed, +and MAINT-04's tripwire was holding them apart rather than closing them +(CFG-03).** `config.yaml` shipped `maintenance_charge_w: 2000`, `main.py` carried +two `opts.get("maintenance_charge_w", 2500)` fallbacks, and `MaintConfig`'s +dataclass carried a third `2500.0`. An options dict with the key **absent** was +therefore accepted by `validate.py`'s rule 6 — which read 2000 — and then built +by `main.py` at 2500, against a 2000 W ceiling. Demonstrated through the shipped +code: + + options dict with maintenance_charge_w ABSENT, max_w=2000 + validate_startup_config -> [] (ACCEPTED, no problems) + MaintConfig().charge_w -> 2500.0 + arbiter.resolve() -> 0.0 W, contradiction=True + +That is the bug rule 6 exists to refuse, **passing rule 6**. Unreachable through +the Supervisor, which always supplies every key from `config.yaml`; fully +reachable from `sim/inject.py` and from the test suite, which is where anyone +debugging maintenance is. + +**`maintenance_discharge_w` had the identical divergence** (`config.yaml` 2000, +`MaintConfig` 2500) and the card named only the charge side. Closing one key and +leaving its twin would have reproduced the defect one line down, so both are +closed. + +**The single source of truth is `config.yaml`'s `options:` block** — the file the +Supervisor reads and the operator edits. There is now exactly **one** in-code +copy, `maintenance.MAINT_OPTION_DEFAULTS`, which is TARIFF-02's +`PRICE_OPTION_DEFAULTS` and SAFETY-24's `PEAK_OPTION_DEFAULTS` pattern applied to +the family that named the card. A mirror rather than a runtime read because the +`Dockerfile` copies `app/` and `run.sh` into the image and **not** `config.yaml`: +the running add-on cannot open the file it is configured by. `test_maintenance.py` +pins every entry by **reading** the shipped file, so a drift is a CI failure +rather than a comment. Every other declaration site now derives from it or has +ceased to exist: + +- `MaintConfig`'s eight dataclass defaults are indexed out of the mirror — no + numeric literal survives in the class that held the third opinion; +- `MaintConfig.from_options()` is the **only** place in the add-on that reads a + `maintenance_*` option. `main.py`'s eight inline `opts.get(key, )` + calls are gone; +- `main.py`'s second fallback site (the cheap-window charge floor) reads + `self.maint.cfg.charge_w` instead of re-opening the options dict, so the rate + the shaping asks for and the rate the state machine drains at cannot be two + numbers from one install; +- `validate.py`'s rule 6 and its `maintenance_discharge_w` warning take their + fallbacks from the mirror. + +**Where the RANGE lives is now stated and checked, so the next person does not +add a fifth site.** `maintenance_charge_w: int(500,5000)` in `config.yaml`'s +`schema:` block is a fourth declaration of a different kind — a bound, not a +default. Bounds live in the schema block **only**: the Supervisor enforces them +and reads only that file. `test_maintenance.py` asserts that every mirrored key +has a schema entry (or the Supervisor silently drops the option and the add-on +runs on the mirror instead of the operator's value) and that every shipped +default lies **inside** its own range. What the schema grammar cannot express is +a relation *between* keys — `maintenance_charge_w` against `max_w` — which is +why rule 6 is written in Python and does not belong in the mirror either. + +**MAINT-04's tripwire in `test_validate.py` is DELETED, not left passing**, as it +and `validate.py`'s ⚠️ block both instructed each other to do. Its passing +condition *was* the bug. In its place, the same four consumers exercised the same +way and now agreeing: the validator accepts a dict with every `maintenance_*` key +missing, a **real `Controller`** built from that dict holds 2000 W, `MaintConfig()` +answers 2000 W with no dict at all, and the real arbiter emits `-2000 W, limited +by supervised(max_w)` instead of `0 W, contradiction=True`. + +**Checked by breaking it, five ways, each observed red before being reverted:** +move `config.yaml` alone → the mirror pin fails; move `config.yaml` **and** the +mirror to 1500 → every consumer follows together (`MaintConfig()` 1500 W, the +`Controller` 1500 W, the arbiter `-1500 W, limited by maintenance(cheap-window +charge)`) and both suites go green again; retype `charge_w: float = 2500.0` into +the dataclass → red; restore `opts.get("maintenance_charge_w", 2500)` in +`main.py` → red; delete a key from `config.yaml`'s options block → red. + +Stale prose that asserted the closed divergence as current is corrected in +`app/watchdog.py`, `test_watchdog.py` and `test_peakguard.py`. + **`peak_soc_floor` reads like a pack-wide reserve and is not one, and on a sustained-load evening the grid-following loop discharges straight past it to the BMS cut-out (SAFETY-25).** The gap has been written down four times without diff --git a/goodwe_controller/app/main.py b/goodwe_controller/app/main.py index f36f476..28775cc 100644 --- a/goodwe_controller/app/main.py +++ b/goodwe_controller/app/main.py @@ -131,19 +131,16 @@ class Controller: integrator_max_w=(float(opts["integrator_max_w"]) if opts.get("integrator_max_w") else None), ) - self.maint = Maintenance( - MaintConfig( - enabled=bool(opts.get("maintenance_enabled", False)), - interval_days=int(opts.get("maintenance_interval_days", 28)), - start_hour=int(opts.get("maintenance_start_hour", 10)), - discharge_w=float(opts.get("maintenance_discharge_w", 2500)), - charge_w=float(opts.get("maintenance_charge_w", 2500)), - soc_floor=float(opts.get("maintenance_soc_floor", 11)), - soc_target=float(opts.get("maintenance_soc_target", 99)), - hold_min=int(opts.get("maintenance_hold_min", 120)), - ), - store, - ) + # ⚠️ CFG-03. EIGHT `opts.get(key, )` CALLS USED TO LIVE HERE, + # and two of those literals disagreed with config.yaml - 2500 against a + # shipped 2000, which is a config validate.py refuses. The read now + # belongs to `MaintConfig.from_options()`, next to the one in-code + # mirror of config.yaml it falls back to, so this file states no opinion + # at all about what a missing maintenance key means. Read + # maintenance.MAINT_OPTION_DEFAULTS before adding a ninth key: it goes + # in config.yaml's `options:` and `schema:` blocks and in that dict, and + # nowhere else. + self.maint = Maintenance(MaintConfig.from_options(opts), store) # SAFETY-02. One instance, one actuator, one cadence - see watchdog.py. # @@ -240,10 +237,14 @@ class Controller: # knows, and refusing one set below the pack's own cut-out means knowing # that cut-out too. So this card adds exactly two keys and reads their # defaults from `peakguard.PEAK_OPTION_DEFAULTS` - TARIFF-02's mirror - # pattern - rather than typing literals here. THAT is what keeps CFG-03 - # at three places: the divergence validate.py's rule 6 documents - # (`maintenance_charge_w` 2000 there, 2500 here) is exactly what a - # literal in this file would repeat. + # pattern - rather than typing literals here. THAT is what kept this + # card from widening: the divergence validate.py's rule 6 used to + # document (`maintenance_charge_w` 2000 in config.yaml, 2500 in this + # file) is exactly what a literal here would have repeated. CFG-03 has + # since closed that one too - `maintenance.MAINT_OPTION_DEFAULTS`, + # read through `MaintConfig.from_options()` - so the rule now holds for + # every option family this file builds: no numeric fallback in main.py + # that config.yaml has not shipped. self.peak_guard = None self._peak_engaged = False self._peak_saturated = False @@ -692,8 +693,16 @@ class Controller: hi = 0.0 reason = "charge-only" if self.cheap: + # ⚠️ CFG-03. `self.maint.cfg.charge_w`, NOT A SECOND READ + # OF `self.o`. This line used to re-open the options dict + # with its own literal fallback, so the rate the shaping + # asked for and the rate the state machine drained at could + # be different numbers from the same install - and, with the + # key absent, they were. The Controller already parsed this + # once in __init__; asking the object that owns it cannot + # drift from it. floor = maintenance_charge_floor( - float(self.o.get("maintenance_charge_w", 2500)), + self.maint.cfg.charge_w, self.peak_fc, float(self.o.get("peak_cap_w", 3500))) if floor > 0: hi, reason = -floor, "cheap-window charge" diff --git a/goodwe_controller/app/maintenance.py b/goodwe_controller/app/maintenance.py index 5119b0e..6656b8c 100644 --- a/goodwe_controller/app/maintenance.py +++ b/goodwe_controller/app/maintenance.py @@ -38,17 +38,112 @@ CHARGE_TIMEOUT_MIN = 2160 # After an abort, do not immediately re-trigger in the same start hour. RESTART_COOLDOWN_MIN = 90 +# ⚠️ CFG-03. THE ONE IN-CODE MIRROR OF config.yaml FOR EVERY `maintenance_*` +# OPTION, and the only place in the add-on where one of these numbers is typed. +# +# WHAT WENT WRONG WITHOUT IT. Until this card each default was written down +# three times - config.yaml's `options:` block, an `opts.get(key, )` +# fallback in main.py, and this dataclass - and MAINT-04 proved they had already +# drifted: config.yaml said 2000, the other two said 2500. An options dict with +# `maintenance_charge_w` ABSENT was therefore ACCEPTED by validate.py's rule 6 +# (which read 2000) and then BUILT by main.py at 2500, against a 2000 W ceiling +# - the exact `lo > hi` contradiction rule 6 exists to refuse, resolving to 0 W +# with `contradiction=True`. Unreachable through the Supervisor, which always +# supplies every key; fully reachable from sim/inject.py and from the test +# suite, which is where anyone debugging maintenance actually is. +# +# WHERE THE SINGLE SOURCE ACTUALLY IS: config.yaml's `options:` block. It is +# what the Supervisor reads and what the operator edits, so it wins by +# definition. This dict is a MIRROR of it and not a second opinion: +# test_maintenance.py pins every entry by READING the shipped file, so a drift +# goes red in CI instead of shipping. That is TARIFF-02's `PRICE_OPTION_DEFAULTS` +# and SAFETY-24's `PEAK_OPTION_DEFAULTS` pattern, now applied to the family that +# named the card. A mirror rather than a runtime read because the Dockerfile +# copies `app/` and `run.sh` into the image and NOT config.yaml - the add-on +# cannot open the file it is configured by, so there is nothing to read at 03:00 +# on a client's Pi, and a parser that could fail at import is a new way for the +# add-on not to start. +# +# ⚠️ DEFAULTS ONLY. NO BOUNDS LIVE HERE, and that is a decision, not an +# omission. The RANGE of each key (`maintenance_charge_w: int(500,5000)`) lives +# in config.yaml's `schema:` block and nowhere else, because the Supervisor is +# what enforces it and the Supervisor reads only that file. What this module +# would gain by mirroring the bounds too is a fifth place to disagree with. +# validate.py's rule 6 is NOT a range and does not belong here either: it is a +# relation BETWEEN TWO KEYS (`maintenance_charge_w` against `max_w`) that the +# schema grammar cannot express at all, which is the whole reason that rule is +# written in Python. +# +# ⚠️ KEYED BY THE OPTION NAME, not by the MaintConfig field name. The dict is +# what `from_options()` indexes while reading the operator's dict, so the two +# strings have to be the same string; a `charge_w` key here would reintroduce a +# transcription step between config.yaml and the reader. +MAINT_OPTION_DEFAULTS = { + "maintenance_enabled": False, + "maintenance_interval_days": 28, + "maintenance_start_hour": 10, + # ⚠️ 2000, NOT THE 2500 THIS SHIPPED AS, and config.yaml carries the + # argument: both rates must stay at or below `max_w` (2000) or the + # cheap-window charge shaping asks the arbiter for `hi = -charge_w` against + # a supervised `lo = -max_w` and contradicts itself into 0 W. Raising max_w + # is what unlocks a faster cycle - raise it FIRST, then these. + "maintenance_discharge_w": 2000, + "maintenance_charge_w": 2000, + "maintenance_soc_floor": 11, + "maintenance_soc_target": 99, + "maintenance_hold_min": 120, +} + @dataclass(frozen=True) class MaintConfig: - enabled: bool = False - interval_days: int = 28 - start_hour: int = 10 - discharge_w: float = 2500.0 - charge_w: float = 2500.0 - soc_floor: float = 11.0 - soc_target: float = 99.0 - hold_min: int = 120 + """The maintenance cycle's settings, already parsed. + + ⚠️ EVERY DEFAULT IS READ OUT OF `MAINT_OPTION_DEFAULTS`, never typed. A + literal in this class is what made `MaintConfig()` a THIRD opinion about + what a missing key means - see the block above - so `MaintConfig()` and + `MaintConfig.from_options({})` and validate.py's fallback are now + arithmetically the same number by construction rather than by vigilance. + """ + + enabled: bool = bool(MAINT_OPTION_DEFAULTS["maintenance_enabled"]) + interval_days: int = int(MAINT_OPTION_DEFAULTS["maintenance_interval_days"]) + start_hour: int = int(MAINT_OPTION_DEFAULTS["maintenance_start_hour"]) + discharge_w: float = float(MAINT_OPTION_DEFAULTS["maintenance_discharge_w"]) + charge_w: float = float(MAINT_OPTION_DEFAULTS["maintenance_charge_w"]) + soc_floor: float = float(MAINT_OPTION_DEFAULTS["maintenance_soc_floor"]) + soc_target: float = float(MAINT_OPTION_DEFAULTS["maintenance_soc_target"]) + hold_min: int = int(MAINT_OPTION_DEFAULTS["maintenance_hold_min"]) + + @classmethod + def from_options(cls, opts: dict) -> "MaintConfig": + """Build from an add-on options dict. + + ⚠️ THE ONLY PLACE IN THE ADD-ON THAT READS A `maintenance_*` OPTION. + main.py used to do this inline as eight `opts.get(key, )` + calls, which is eight opportunities to type a number that config.yaml + does not ship - and one of them had already been taken. Moving the read + next to the mirror means the fallback cannot be anything but the mirror, + because there is no dict literal in sight of it to disagree with. + + ⚠️ `key in opts`, not `opts.get(key) or ...`. A shipped `false` and a + shipped `0` are values an operator can legitimately set, and the truthy + form would quietly replace both with the default - which is this card's + own bug wearing a different hat. + """ + def _opt(key): + return opts[key] if key in opts else MAINT_OPTION_DEFAULTS[key] + + return cls( + enabled=bool(_opt("maintenance_enabled")), + interval_days=int(_opt("maintenance_interval_days")), + start_hour=int(_opt("maintenance_start_hour")), + discharge_w=float(_opt("maintenance_discharge_w")), + charge_w=float(_opt("maintenance_charge_w")), + soc_floor=float(_opt("maintenance_soc_floor")), + soc_target=float(_opt("maintenance_soc_target")), + hold_min=int(_opt("maintenance_hold_min")), + ) @dataclass diff --git a/goodwe_controller/app/validate.py b/goodwe_controller/app/validate.py index 14343dd..e911ce8 100644 --- a/goodwe_controller/app/validate.py +++ b/goodwe_controller/app/validate.py @@ -202,6 +202,7 @@ WHAT IS DELIBERATELY *NOT* REFUSED, because it is untidy rather than unsafe: import logging from .arbitrage import enabled as arbitrage_on +from .maintenance import MAINT_OPTION_DEFAULTS from .p1 import is_enabled, validate_meter_options from .peakguard import PEAK_OPTION_DEFAULTS from .price import PRICE_OPTION_DEFAULTS, PROVIDERS @@ -379,26 +380,23 @@ def validate_startup_config(opts: dict, dev_min=None, dev_max=None) -> list[str] # leave the trap armed for the one operator action - ticking # maintenance_enabled - that cannot possibly be expected to change what a # charge rate means. - # ⚠️ THE FALLBACK IS config.yaml's SHIPPED VALUE, and it has to be kept - # equal to it - that is the whole point of AC3. It is reachable only when a - # caller hands over a dict with the key missing (sim/inject.py, a test); the - # Supervisor always supplies every key from config.yaml, so on a real - # install the argument is always present. - # - # ⚠️ KNOWN MISMATCH, LEFT DELIBERATELY AND FLAGGED RATHER THAN FIXED HERE: - # `main.py` still falls back to 2500 in two places - # (`opts.get("maintenance_charge_w", 2500)`), as does maintenance.py's - # MaintConfig dataclass. So a dict with the key ABSENT is accepted by this - # 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 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) + # ⚠️ THE FALLBACK IS config.yaml's SHIPPED VALUE, READ RATHER THAN TYPED + # (CFG-03). It is reachable only when a caller hands over a dict with the + # key missing (sim/inject.py, a test); the Supervisor always supplies every + # key from config.yaml, so on a real install the argument is always present + # and this default is dead code. That is precisely why it went stale + # unnoticed for a whole card: MAINT-04 moved config.yaml to 2000 while + # main.py and MaintConfig still said 2500, so a dict with the key ABSENT was + # accepted here at 2000 and then built by main.py at 2500 - the + # contradiction this rule exists to catch, in the one case it could not see. + # The KNOWN MISMATCH block that used to stand here, and the tripwire in + # test_validate.py that pinned it, are both DELETED: the divergence is + # closed rather than recorded. `maintenance.MAINT_OPTION_DEFAULTS` is now + # the one in-code mirror of config.yaml for this family - the same move + # rule 8 below already makes with `PEAK_OPTION_DEFAULTS` - and + # test_maintenance.py pins it by reading the shipped file. + charge_w = _number(opts, "maintenance_charge_w", + float(MAINT_OPTION_DEFAULTS["maintenance_charge_w"])) if charge_w is None: problems.append( f"maintenance_charge_w {opts.get('maintenance_charge_w')!r} is not a number") @@ -420,10 +418,12 @@ def validate_startup_config(opts: dict, dev_min=None, dev_max=None) -> list[str] # --- 8. an EMS energy floor the BMS does not already enforce ------------- # ⚠️ THE FALLBACKS ARE peakguard.PEAK_OPTION_DEFAULTS, NOT LITERALS, and # that is CFG-03 read as a rule rather than as a complaint. Rule 6's own - # comment documents a LIVE divergence in this repo caused by typing 2000 - # here and 2500 in main.py; this card refuses to add a second instance of - # it. The dict is the single in-code mirror, pinned to config.yaml by - # test_peakguard.py, and main.py reads the same one. + # comment used to document a LIVE divergence in this repo caused by typing + # 2000 here and 2500 in main.py; SAFETY-24 refused to add a second instance + # of it, and CFG-03 has since closed the first. The dict is the single + # in-code mirror, pinned to config.yaml by test_peakguard.py, and main.py + # reads the same one - exactly as maintenance.MAINT_OPTION_DEFAULTS now is + # for the `maintenance_*` family. soc_floor = _number(opts, "peak_soc_floor", PEAK_OPTION_DEFAULTS["peak_soc_floor"]) bms_floor = _number(opts, "bms_soc_floor", @@ -465,7 +465,13 @@ def validate_startup_config(opts: dict, dev_min=None, dev_max=None) -> list[str] # words on the status page every cycle, and the cycle still completes - # slower, and eating into maintenance.DRAIN_TIMEOUT_MIN. Loud and it moves # energy, so it is neither (a) nor (b): warned, not refused. - disch_w = _number(opts, "maintenance_discharge_w", 2000.0) + # ⚠️ AND THE FALLBACK COMES FROM THE SAME MIRROR AS RULE 6's. CFG-03's PM + # proofread caught this one: `maintenance_discharge_w` had the IDENTICAL + # three-way divergence (config.yaml 2000, MaintConfig 2500) and the card + # named only the charge side. Closing one key and leaving its twin is how a + # single-source fix gets reproduced as a single-source bug one line down. + disch_w = _number(opts, "maintenance_discharge_w", + float(MAINT_OPTION_DEFAULTS["maintenance_discharge_w"])) if disch_w is not None and max_w is not None and max_w > 0 and disch_w > max_w: _LOG.warning( "maintenance_discharge_w %.0f W is above max_w %.0f W. Not refused - " diff --git a/goodwe_controller/app/watchdog.py b/goodwe_controller/app/watchdog.py index afaf2a4..d2ff923 100644 --- a/goodwe_controller/app/watchdog.py +++ b/goodwe_controller/app/watchdog.py @@ -97,15 +97,17 @@ Four gates, and each one exists to exclude a specific thing that is NOT a fault: caller says whether the grid-following loop actually won the arbitration this cycle, and the timer only accumulates when it did. - ⚠️ 1500 W, NOT THE DEFAULT 2500 W, AND THE REASON IS NOT COSMETIC. At the - shipped defaults `maintenance_charge_w` is 2500 and `max_w` is 2000, so the - cheap-window claim is `hi = -2500` against a supervised `lo = -2000`: lo > hi, - which arbiter.py resolves as a CONTRADICTION and 0 W, not as a -2500 W charge. - The example above is written at 1500 W because that is a combination the - schema and the arbiter both actually produce. (The contradiction at the - defaults is a real pre-existing bug - a default maintenance charge silently - never happens in a cheap window - but it is maintenance's, not this module's, - and it is filed separately. Do not "fix" it here.) + ⚠️ 1500 W, NOT 2500 W, AND THE REASON IS NOT COSMETIC. The example has to be + a charge the arbiter actually produces. When this was written the shipped + `maintenance_charge_w` was 2500 against a `max_w` of 2000, so the cheap-window + claim was `hi = -2500` against a supervised `lo = -2000`: lo > hi, which + arbiter.py resolves as a CONTRADICTION and 0 W, not as a -2500 W charge - a + default maintenance charge that silently never happened. MAINT-04 refused that + pair in validate.py and moved the shipped rate to 2000; CFG-03 gave the + default one source of truth so main.py and MaintConfig could not go on + answering 2500 to a dict that had never said so. The number here stays 1500 + because the point of the example is a charge STRICTLY INSIDE the ceiling, not + one that happens to sit exactly on it. 2. The strict `< 0` product. "Opposed" and "neither side is zero" in one expression. diff --git a/goodwe_controller/config.yaml b/goodwe_controller/config.yaml index daf5fe2..fa47061 100644 --- a/goodwe_controller/config.yaml +++ b/goodwe_controller/config.yaml @@ -135,6 +135,21 @@ options: meter_freeze_s: 300 # --- maintenance ----------------------------------------------------------- + # ⚠️ CFG-03. THESE EIGHT LINES ARE THE SOURCE OF TRUTH FOR THE MAINTENANCE + # DEFAULTS. The Supervisor reads this file and the operator edits this file, + # so nothing else in the add-on gets an opinion about what a missing + # `maintenance_*` key means. There is exactly ONE in-code copy - + # `app/maintenance.py`'s `MAINT_OPTION_DEFAULTS` - because the Dockerfile does + # not put this file in the image and the running add-on therefore cannot read + # it; test_maintenance.py pins that copy by READING these lines, so a drift + # goes red in CI rather than shipping. main.py and validate.py both read the + # mirror and type no number of their own. + # + # Before this card the same defaults were written down three times and had + # already disagreed: this block said 2000 while main.py and MaintConfig said + # 2500, so an options dict with the key ABSENT passed validation at 2000 and + # then ran at 2500 against a 2000 W ceiling - a silent 0 W arbiter + # contradiction. Changing one of these numbers now moves every consumer. maintenance_enabled: false maintenance_interval_days: 28 maintenance_start_hour: 10 @@ -383,6 +398,16 @@ schema: # app/freeze.py. meter_freeze_s: int(60,3600) + # ⚠️ CFG-03 - THIS IS WHERE A RANGE LIVES, AND THE ONLY PLACE IT LIVES. The + # `options:` block above holds the DEFAULT; this block holds the BOUNDS, and + # `MAINT_OPTION_DEFAULTS` deliberately mirrors the former and not the latter - + # the Supervisor is what enforces a range and the Supervisor reads only this + # file, so a second copy of `int(500,5000)` in Python would be a place to + # disagree and nothing else. What is NOT expressible here is a relation + # BETWEEN keys: `maintenance_charge_w` must also stay at or below `max_w`, the + # schema grammar has no way to say so, and that is why validate.py's rule 6 is + # written in code instead. Add a ninth maintenance option in three places - + # options:, schema:, MAINT_OPTION_DEFAULTS - and in no fourth. maintenance_enabled: bool maintenance_interval_days: int(1,90) maintenance_start_hour: int(0,23) diff --git a/goodwe_controller/test_maintenance.py b/goodwe_controller/test_maintenance.py index b582f0d..3df01c3 100644 --- a/goodwe_controller/test_maintenance.py +++ b/goodwe_controller/test_maintenance.py @@ -12,10 +12,15 @@ is TEST-15, and it has already turned one live mutation into a false survivor. Read MUTATION.md before you report a survivor. """ +import dataclasses +import re import sys from datetime import datetime, timedelta, timezone +from pathlib import Path -from app.maintenance import CHARGE, DRAIN, HOLD, IDLE, MaintConfig, Maintenance +from app.maintenance import ( + CHARGE, DRAIN, HOLD, IDLE, MAINT_OPTION_DEFAULTS, MaintConfig, Maintenance, +) fails = [] @@ -111,6 +116,114 @@ print("disabled schedule") m = Maintenance(MaintConfig(enabled=False), FakeStore()) check("disabled never starts", m.tick(t0, soc=80).phase == IDLE) +print("CFG-03: config.yaml is the source, this module is the only mirror") +# ⚠️ WHY THIS IS PINNED BY READING THE FILE. Before CFG-03 each maintenance +# default was written down three times - config.yaml's `options:` block, +# `opts.get(key, )` in main.py, and MaintConfig's dataclass defaults - +# and MAINT-04 proved they had already drifted (2000 shipped, 2500 in code). The +# fix is one source and one mirror; the mirror is only worth anything if drift +# from it is a CI failure rather than a comment, and a transcription of +# config.yaml into this file would just be a fourth place to disagree. So the +# shipped values are READ out of the file the Supervisor reads. +# +# Regex rather than PyYAML on purpose, for test_validate.py's reason: the add-on +# image installs py3-aiohttp and py3-paho-mqtt only, and a check that skips on +# ImportError is not a check. +HERE = Path(__file__).resolve().parent +_CFG_TEXT = (HERE / "config.yaml").read_text(encoding="utf-8") +_SCHEMA_AT = _CFG_TEXT.index("\nschema:") + + +def shipped_default(key): + """The value under `options:` - the FIRST match, before `schema:`.""" + m = re.search(rf"^ {key}: *(.+?) *$", _CFG_TEXT[:_SCHEMA_AT], re.M) + return None if m is None else m.group(1) + + +def schema_range(key): + """`int(lo,hi)` under `schema:` -> (lo, hi); None if the key is absent.""" + m = re.search(rf"^ {key}: *\w+\((-?[\d.]+),(-?[\d.]+)\) *$", + _CFG_TEXT[_SCHEMA_AT:], re.M) + return None if m is None else (float(m.group(1)), float(m.group(2))) + + +def as_number(text): + if text in ("true", "false"): + return 1.0 if text == "true" else 0.0 + return float(text) + + +_ship = {k: shipped_default(k) for k in MAINT_OPTION_DEFAULTS} +print(f" config.yaml ships {_ship}") +check("every mirrored key is actually shipped in config.yaml's options: block", + all(v is not None for v in _ship.values())) +check("...and MAINT_OPTION_DEFAULTS reproduces each one exactly, so the mirror " + "is a copy of the source and not an opinion about it", + all(str(MAINT_OPTION_DEFAULTS[k]).lower() == _ship[k] + for k in MAINT_OPTION_DEFAULTS)) +# ⚠️ THE OTHER DIRECTION MATTERS TOO. Equality alone would still pass if the +# mirror quietly stopped covering a key someone added to config.yaml, and the +# key nobody mirrors is exactly the key that gets a literal typed for it. +_shipped_keys = set(re.findall(r"^ (maintenance_[a-z_]+):", _CFG_TEXT[:_SCHEMA_AT], + re.M)) +check(f"...and it covers EVERY maintenance_* option the file ships, so a ninth " + f"key cannot arrive unmirrored {sorted(_shipped_keys)}", + _shipped_keys == set(MAINT_OPTION_DEFAULTS)) + +# ⚠️ WHERE THE RANGE LIVES, ASSERTED RATHER THAN COMMENTED. The PM proofread on +# this card named a FOURTH declaration site of a different kind: +# `maintenance_charge_w: int(500,5000)` in the schema block. It is a BOUND, not +# a default, and the decision this card records is that bounds live in the +# schema block ONLY - the Supervisor enforces them and reads only that file, so +# a Python copy would be a place to disagree and nothing else. What that +# decision needs from a check is (a) every mirrored key really does have a +# schema entry, or the Supervisor drops the option and the add-on silently runs +# on the mirror instead of the operator's value, and (b) the shipped default +# lies INSIDE its own range, because a default the schema rejects is a config +# that cannot be re-entered once it is edited. +_ranged = {k: schema_range(k) for k in MAINT_OPTION_DEFAULTS} +check("every mirrored key is in the schema: block too - `bool` has no range, so " + "it is the one exception and it is named", + all(v is not None for k, v in _ranged.items() + if k != "maintenance_enabled") + and _ranged["maintenance_enabled"] is None) +_outside = {k: (as_number(_ship[k]), rng) for k, rng in _ranged.items() + if rng is not None and not rng[0] <= as_number(_ship[k]) <= rng[1]} +check(f"...and every shipped default sits inside its own schema range, so the " + f"Supervisor accepts the file it ships with {_outside or 'all inside'}", + not _outside) + +# ⚠️ AND THE CONSUMERS MOVE WITH IT - checked as a RELATION, not against +# numbers. AC4 asks for the replacement to be broken and every consumer observed +# moving together. A number typed here would pass whether or not the derivation +# survived; `dataclasses.fields()` reads what the class actually defaulted to, +# so this can only pass while the class is still indexing the mirror. +_fields = {f.name: f.default for f in dataclasses.fields(MaintConfig)} +check(f"MaintConfig's field names are the option names minus the prefix, so a " + f"new field cannot miss the mirror by being spelt differently {sorted(_fields)}", + {f"maintenance_{n}" for n in _fields} == set(MAINT_OPTION_DEFAULTS)) +check("...and every dataclass default IS the mirror's value - no literal " + "survives in the class that used to hold the third opinion", + all(float(d) == float(MAINT_OPTION_DEFAULTS[f"maintenance_{n}"]) + for n, d in _fields.items())) +# from_options() is the only reader, so it is the only thing that can put a +# different number in front of main.py. +_from_empty = MaintConfig.from_options({}) +check("...and from_options({}) - the sim/inject.py and test-suite path, the one " + "the Supervisor never takes and the only one the divergence was reachable " + "from - equals MaintConfig() exactly", + _from_empty == MaintConfig()) +check("...while a supplied value still wins, or the mirror would be a ceiling " + "rather than a default", + MaintConfig.from_options({"maintenance_charge_w": 1234}).charge_w == 1234.0) +# ⚠️ `key in opts`, not truthiness: `false` and `0` are values an operator can +# set, and a truthy read would silently replace both with the default - this +# card's own bug in a different costume. +check("...and an explicit falsey value is honoured rather than defaulted away", + MaintConfig.from_options({"maintenance_start_hour": 0}).start_hour == 0 + and MaintConfig.from_options({"maintenance_enabled": False}).enabled is False) + +print() print() if fails: print(f"{len(fails)} FAILED: {', '.join(fails)}") diff --git a/goodwe_controller/test_peakguard.py b/goodwe_controller/test_peakguard.py index b522cfa..0b5f512 100644 --- a/goodwe_controller/test_peakguard.py +++ b/goodwe_controller/test_peakguard.py @@ -1838,9 +1838,9 @@ check("...and both are in the schema: block too, or the Supervisor drops the " "operator's value", sorted(_schema24) == sorted(PEAK_OPTION_DEFAULTS)) _val_src = (HERE / "app" / "validate.py").read_text(encoding="utf-8") check("...and validate.py takes its fallbacks from that same mirror rather than " - "typing literals - which is the divergence its own rule 6 documents " - "(maintenance_charge_w 2000 there, 2500 in main.py), refused a second " - "outing here", + "typing literals - which is the divergence its own rule 6 used to " + "document (maintenance_charge_w 2000 in config.yaml, 2500 in main.py), " + "refused a second outing here and since closed by CFG-03", "PEAK_OPTION_DEFAULTS" in _val_src and not re.search(r'"peak_soc_floor"\s*,\s*[0-9]', _val_src) and not re.search(r'"peak_soc_floor"\s*,\s*[0-9]', _mainsrc)) diff --git a/goodwe_controller/test_validate.py b/goodwe_controller/test_validate.py index eb08fae..f354704 100644 --- a/goodwe_controller/test_validate.py +++ b/goodwe_controller/test_validate.py @@ -17,6 +17,7 @@ Read MUTATION.md before you report a survivor. import asyncio import sys +from app.maintenance import MAINT_OPTION_DEFAULTS, MaintConfig from app.validate import ( ACTUATOR_WRITE_KEYS, FIRMWARE_INGEST_VETO_S, ConfigRefused, refuse, validate_startup_config, @@ -34,10 +35,16 @@ def check(name, cond): def base(**over): """A configuration that must start. Everything else is this, spoilt.""" # ⚠️ The maintenance rates are named EXPLICITLY, at the values config.yaml - # ships (MAINT-04). They are not decoration: with the key absent, rule 6 - # falls back to main.py's 2500 W, which against this 2000 W ceiling is a - # genuine refusal - so a base() without them would be a config that does - # not start, and every check below would be measuring the wrong thing. + # ships (MAINT-04). Under MAINT-04 that was load-bearing: with the key + # absent, rule 6 fell back to a 2500 W literal that against this 2000 W + # ceiling is a genuine refusal, so a base() without them was a config that + # does not start and every check below would have been measuring the wrong + # thing. CFG-03 closed that - the fallback is now config.yaml's own 2000 - + # so they are here to say out loud what this fixture asks for rather than to + # rescue it. The CFG-03 section at the foot of this file is what asserts + # that absent and present are now THE SAME NUMBER; do not delete these two + # keys, because a base() that agrees with the default by accident is a + # fixture that cannot tell you when the default moves. o = {"setpoint_entity": "number.goodwe_setpoint", "max_w": 2000, "soc_entity": "sensor.soc", "batt_entity": "sensor.batt", "meter_source": "off", @@ -379,74 +386,6 @@ check("...and a ceiling already past the billing quarter, so no value the " "schema still accepts can alarm inside the quarter that bills it", _hi is not None and _hi > _QUARTER_S) -# --------------------------------------------------------------------------- # -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 @@ -607,6 +546,94 @@ try: except TypeError: check("a Controller with no stop Event is rejected at construction", True) +# --------------------------------------------------------------------------- # +print("CFG-03: the key ABSENT is the SAME NUMBER everywhere, through real code") +# ⚠️ WHAT THIS REPLACES, AND WHY IT IS THE OPPOSITE ASSERTION. MAINT-04 left a +# TRIPWIRE here whose PASSING CONDITION WAS THE BUG: it asserted that +# config.yaml said 2000 while main.py's two `opts.get(..., 2500)` fallbacks and +# MaintConfig's dataclass default said 2500, and it went red if EITHER side +# moved - including when somebody closed the divergence. That check has been +# DELETED, as it and validate.py's ⚠️ block both instructed, and this is what +# stands in its place: the same four consumers, exercised the same way, now +# agreeing. +# +# The four are the four the defect ran through. The validator ACCEPTS a dict +# with the key missing; main.py BUILDS a MaintConfig from that same dict; the +# dataclass answers on its own with no dict at all; and the arbiter is what +# turns their disagreement into a number an operator can see. Any three of them +# agreeing was never the property that mattered. +_absent = {k: v for k, v in base().items() + if not k.startswith("maintenance_")} +_ship_charge = _shipped["maintenance_charge_w"] +_ship_drain = _shipped["maintenance_discharge_w"] + +check("the validator still ACCEPTS a dict with every maintenance key missing - " + "the fixture is the reachable one, not a refusal in disguise", + validate_startup_config(_absent, -5000, 5000) == []) + +# 1. the mirror against the shipped file. Read, not transcribed: a transcription +# is the thing that drifted. +check(f"the ONE in-code mirror equals what config.yaml ships " + f"(charge {MAINT_OPTION_DEFAULTS['maintenance_charge_w']}, " + f"shipped {_ship_charge:.0f})", + float(MAINT_OPTION_DEFAULTS["maintenance_charge_w"]) == _ship_charge + and float(MAINT_OPTION_DEFAULTS["maintenance_discharge_w"]) == _ship_drain) + +# 2. MaintConfig on its own - the one that bites when main.py is bypassed +# entirely, which is sim/inject.py and most of this suite. +check(f"MaintConfig() answers the shipped number, not a third one " + f"({MaintConfig().charge_w:.0f} W)", + MaintConfig().charge_w == _ship_charge + and MaintConfig().discharge_w == _ship_drain) + +# 3. main.py's own path, through a real Controller rather than through a regex +# over its source. A source scan can only say the literal is gone; this says +# the object an operator's install actually runs on holds the right number. +_ctl_absent = Controller(_absent, None, _Store2(), _Pub2(), stop=asyncio.Event()) +print(f" a Controller built from a dict with NO maintenance key: " + f"charge_w={_ctl_absent.maint.cfg.charge_w:.0f} W, " + f"discharge_w={_ctl_absent.maint.cfg.discharge_w:.0f} W") +check("main.py builds the shipped number from a dict that does not contain it", + _ctl_absent.maint.cfg.charge_w == _ship_charge + and _ctl_absent.maint.cfg.discharge_w == _ship_drain) +# ...and it is the SAME number when the operator supplies it explicitly, which +# is what "the default is not a special case" means. +_ctl_named = Controller(base(), None, _Store2(), _Pub2(), stop=asyncio.Event()) +check("...and an explicit key builds the identical config, so absent and " + "present are not two code paths", + _ctl_named.maint.cfg == _ctl_absent.maint.cfg) + +# 4. the arbiter - the consequence itself. Under MAINT-04 this pair was +# (2000, 2500) and resolved to 0 W with contradiction=True; that was the +# ACCEPTED-then-contradicted hole. The rates come out of the built config, +# not out of a literal, so if the single source moves this moves with it. +_r_ok = _charge(float(_absent["max_w"]), _ctl_absent.maint.cfg.charge_w) +print(f" arbiter with that config: {_r_ok.explain()}") +check("the arbiter CHARGES instead of contradicting itself - the hole MAINT-04 " + "pinned open is closed at the only place it was ever reachable", + _r_ok.target_w == -_ship_charge and not _r_ok.contradiction) +_r_drain = _drain(float(_absent["max_w"]), _ctl_absent.maint.cfg.discharge_w) +check("...and the drain SET is not clamped either, so the twin key the card " + "did not name is closed too", + _r_drain.target_w == _ship_drain and not _r_drain.contradiction) + +# 5. nothing typed a number behind the mirror's back. Rule 6's fallback and +# main.py's are the two sites the divergence lived at, so they are the two +# sites a scan is worth running over. +_srcs = {name: open(os.path.join(os.path.dirname(os.path.abspath(__file__)), + "app", name), encoding="utf-8").read() + for name in ("main.py", "validate.py", "maintenance.py")} +_literals = {name: re.findall( + r'"(maintenance_[a-z_]+)"\s*,\s*(-?\d+(?:\.\d+)?)\s*[,)]', src) + for name, src in _srcs.items()} +check(f"no numeric fallback for any maintenance_* key survives in main.py, " + f"validate.py or maintenance.py {_literals}", + not any(_literals.values())) +check("...and MaintConfig's dataclass defaults carry no numeric literal either " + "- they are indexed out of the mirror", + not re.search(r"^\s*(charge_w|discharge_w):\s*float\s*=\s*-?\d", + _srcs["maintenance.py"], re.M)) + # --------------------------------------------------------------------------- # print() if fails: diff --git a/goodwe_controller/test_watchdog.py b/goodwe_controller/test_watchdog.py index af422e9..e4044b7 100644 --- a/goodwe_controller/test_watchdog.py +++ b/goodwe_controller/test_watchdog.py @@ -211,13 +211,15 @@ TABLE = [ Claim.limit("maintenance", P_MAINT_SHAPE, hi=0.0, reason="charge-only"), MAXW], 900, False), # ⚠️ THE CASE `winner == \"loop\"` ALONE WOULD HAVE GOT WRONG. 1500 W, not - # the default 2500 W: at max_w 2000 a -2500 hi against a -2000 lo is an - # arbiter contradiction, not a charge - see main.py. + # 2500 W: at max_w 2000 a -2500 hi against a -2000 lo is an arbiter + # contradiction, not a charge. That pair was the SHIPPED default until + # MAINT-04 refused it and CFG-03 gave the default one source of truth; it is + # kept below as a case rather than as a configuration. ("cheap-window charge REDIRECTS the loop while it still 'wins'", [Claim.set("loop", P_LOOP, 500), Claim.limit("maintenance", P_MAINT_SHAPE, hi=-1500, reason="cheap-window"), MAXW], 500, False), - ("...and the contradiction at the shipped defaults is not following either", + ("...and the contradiction when charge_w exceeds max_w is not following either", [Claim.set("loop", P_LOOP, 500), Claim.limit("maintenance", P_MAINT_SHAPE, hi=-2500, reason="cheap-window"), MAXW], 500, False),