diff --git a/goodwe_controller/app/main.py b/goodwe_controller/app/main.py index 006e651..bdabc32 100644 --- a/goodwe_controller/app/main.py +++ b/goodwe_controller/app/main.py @@ -515,11 +515,17 @@ class Controller: # default install, so until now the suspension could not fire on the # site that needed it. `self.peak_guard` is measured from the meter # and is always there. - suspend = peak_at_risk(self.peak_fc, - float(self.o.get("peak_cap_w", 3500))) - if self.peak_guard is not None and self.peak_guard.engaged: - suspend = True - if suspend: + # + # ⚠️ THE CONJUNCTION LIVES IN peakguard.py, NOT HERE, and that is not + # tidiness. It was inline, and a reviewer mutated it back to its + # historical broken form - re-gated on `peak_forecast_entity` alone - + # and every suite in this repository stayed green while a default + # install parked at 0 W in the quarter that sets the month's bill. + # Read shaping_suspended() before touching this: both terms are + # there, and why neither is sufficient alone. + if peakguard.shaping_suspended( + self.peak_guard, self.peak_fc, + float(self.o.get("peak_cap_w", 3500))): # Edge-logged: level-logging here would write one line per cycle # for as long as the event lasts and flush the 40-line action log # of the very context that explains it. diff --git a/goodwe_controller/app/peakguard.py b/goodwe_controller/app/peakguard.py index 2a3c454..c8373f3 100644 --- a/goodwe_controller/app/peakguard.py +++ b/goodwe_controller/app/peakguard.py @@ -245,6 +245,21 @@ validate.py are untouched by this ticket, exactly as TARIFF-01 left them, and 14's `capacity_peak_limit_kw` stays declined - a kW key beside a W key is the mismatch trap 14's own warning box describes. +------------------------------------------------------------------------------ +⚠️ WHAT THIS GUARD DOES NOT HAVE: AN ENERGY BACKSTOP. SEE SAFETY-24. + +The graded branch can FORCE DISCHARGE, and nothing in this file or in the loop +below it consults the state of charge - `soc_floor` belongs to the maintenance +cycle, and freeze.py already records that for the grid-following loop "the BMS +is the sole energy backstop". On the 2026-08-20 replay the guard asks for 878 Wh +in a day. Against a pack sitting near 9 % that is not nothing, and the guard will +keep asking as long as the meter says the quarter is over the ceiling. + +That is deliberate for THIS card and it is filed as SAFETY-24. It is also the +reason this must not go to a client install before SAFETY-24 lands: on the +reference site the BMS is a known quantity and an operator is present, and on a +client site neither is true. + ------------------------------------------------------------------------------ AND IT READS NO PER-PHASE QUANTITY, ON PURPOSE @@ -258,6 +273,7 @@ not need re-testing. Nothing here indexes a phase. from dataclasses import dataclass from .arbiter import P_PEAK, Claim +from .control import peak_at_risk from .tariff import QUARTER_S # Why the projection may not be graded. Reasons, not booleans: the status page @@ -454,6 +470,40 @@ def decide(projection, *, batt_w, cap_w, reading, achievable_w, f"only give {ceil_top:.0f} W" if saturated else ""))) +def shaping_suspended(decision, peak_forecast_w, cap_w) -> bool: + """Must the maintenance charge-only shaping stand down this cycle? + + THE ONE PLACE THIS CONJUNCTION IS WRITTEN, and it is public for the reason + `watchdog.loop_is_following()` and `freeze.looks_frozen()` are: the check + that grades it has to assert on THE SHIPPED EXPRESSION. A safety predicate + transcribed into its own test is one copy plus a future disagreement about + what the check means, and the copy that drifts is always the grading one. + It was inline in `main.py` and nothing could reach it; a reviewer mutated it + back to its historical broken form and all twelve suites stayed green. + + ⚠️ WHY SUPPRESSION AND NOT PRIORITY. The shaping is a LIMIT (`hi <= 0`) and + the peak guard's claim is a LIMIT (`lo >= 0`). arbiter.py clause 2 applies + EVERY limit at or above the winner's priority and takes the MOST RESTRICTIVE, + with no notion of one limit outranking another - which is precisely what + makes the safety bounds unconditional and must not be changed. Two crossing + limits are therefore a CONTRADICTION, which resolves to 0 W: the battery + PARKED, in the one quarter of the month that sets the bill. P_PEAK 50 > + P_MAINT_SHAPE 40 does not save it and never could. + + ⚠️ AND THE FIRST TERM IS NOT ENOUGH ON ITS OWN. `peak_forecast_entity` is + OPTIONAL and is empty on a default install, so `peak_forecast_w` is None + there and `peak_at_risk()` answers False forever. Before this card the + suspension therefore could not fire on the very site that needed it. The + second term is the MEASURED guard, which is always present wherever P1 is. + Deleting either term - or re-gating the whole thing on the forecast - parks + a default install at 0 W, silently, in the billed quarter. test_peakguard.py + part 7 drives the real cycle with each of those mutations in place. + """ + if peak_at_risk(peak_forecast_w, float(cap_w)): + return True + return decision is not None and decision.engaged + + def claim(decision: PeakDecision) -> Claim | None: """The decision as one arbiter claim, or None when the guard is idle. diff --git a/goodwe_controller/test_peakguard.py b/goodwe_controller/test_peakguard.py index 95636ab..793e2ba 100644 --- a/goodwe_controller/test_peakguard.py +++ b/goodwe_controller/test_peakguard.py @@ -80,6 +80,7 @@ from app import peakguard from app.arbiter import ( P_LOOP, P_MAINT_SHAPE, P_MAINTENANCE, P_PEAK, P_SAFETY, Claim, resolve, ) +from app.control import peak_at_risk from app.freeze import FreezeWatchdogConfig, looks_frozen, rail_w from app.main import Controller from app.p1 import ( @@ -763,14 +764,21 @@ print("7. the wiring: the real Controller, the real cycle, the real status page" class _Store: - data = {} + # ⚠️ PER INSTANCE. `data` was a CLASS attribute, which is fine while + # nothing writes it and a trap the moment part 7 forces a maintenance phase + # on one controller: every controller built afterwards would inherit it. + def __init__(self, data=None): + self.data = dict(data or {}) - def set(self, *a): - pass + def set(self, key, value): + self.data[key] = value def get_time(self, *a): return None + def set_time(self, *a): + pass + class _Pub: enabled = False @@ -802,7 +810,7 @@ class _Hass: return True -def built(db_path, opts=None): +def built(db_path, opts=None, store=None): hass = _Hass() c = Controller(dict( {"setpoint_entity": "number.sp", "meter_source": "ha_signed", @@ -810,7 +818,8 @@ def built(db_path, opts=None): "max_w": LIVE_MAX_W, "gain": 0.6, "slew_w": 1000, "deadband_w": 15, "target_grid_w": 0, "step_w": 10, "heartbeat_s": 10, "stale_input_s": 15}, **(opts or {})), - hass, _Store(), _Pub(), stop=asyncio.Event(), peak_db_path=str(db_path)) + hass, store or _Store(), _Pub(), stop=asyncio.Event(), + peak_db_path=str(db_path)) c.dev_min, c.dev_max = -LIVE_MAX_W, LIVE_MAX_W return c, hass @@ -862,7 +871,8 @@ check("...and the status page says NOT COVERED rather than 'idle' or a green " # block with no seconds remaining - so the whole setup is retried on a fresh # Controller rather than left to flake. It is bounded at three attempts: a fourth # would mean something is wrong that is not a boundary. -def wired_over_limit(db, want_floor_w, opts=None): +def wired_over_limit(db, want_floor_w, opts=None, store=None, batt_w=0.0, + standing_w=None): """A real Controller driven one real cycle over a real over-limit quarter. ⚠️ THE LOAD IS DERIVED FROM WHERE IN THE QUARTER THE WALL CLOCK LANDS, and @@ -874,9 +884,14 @@ def wired_over_limit(db, want_floor_w, opts=None): computed backwards from `remaining` at the moment of the run. """ for attempt in range(4): - ctl, hs = built(db if attempt == 0 else Path(f"{db}.{attempt}"), opts) - ctl.auto, ctl.soc, ctl.batt = True, 55.0, 0.0 + ctl, hs = built(db if attempt == 0 else Path(f"{db}.{attempt}"), opts, + store=_Store(store) if store is not None else None) + ctl.auto, ctl.soc, ctl.batt = True, 55.0, batt_w ctl.dev_min, ctl.dev_max = -LIVE_MAX_W, LIVE_MAX_W + if standing_w is not None: + # The command STANDING ON THE WIRE, which is what B3 below needs to + # differ from the measured battery power. + ctl.target = ctl.last_write = standing_w now = datetime.now(timezone.utc) blk = datetime.fromtimestamp( int(now.timestamp() // QUARTER_S) * QUARTER_S, timezone.utc) @@ -951,6 +966,184 @@ check("...and the command is still bounded by the rating, so a saturated guard " "never asks the hardware for more than it has", abs(hass2.writes[-1]) <= LIVE_MAX_W + 1e-6) +# ============================================================================== +# THE WIRING'S OWN MUTATIONS. Everything above this line grades `peakguard.py`, +# and a reviewer showed that is not the same thing as grading the guard: THREE +# one-line changes to main.py's CALL SITE left all twelve suites green while a +# default install parked at 0 W in the quarter that sets the month's bill. +# +# So these three are executed, not described. Each mutation is put in place on +# the SHIPPED module and the REAL `Controller.cycle()` is driven with it - which +# also proves main.py routes through the thing being mutated rather than +# inlining it, because a monkeypatch of an inlined expression would do nothing +# and these checks would pass vacuously. +print() +print(" the wiring's own mutations - a default install, maintenance charging") + +# A DEFAULT INSTALL: `peak_forecast_entity` is absent, so `peak_fc` is None and +# `peak_at_risk()` answers False forever. That is the whole point - it is the +# configuration on which the old suspension could not fire. +MAINT_CHARGING = {"phase": "charge"} +_mainsrc = (HERE / "app" / "main.py").read_text(encoding="utf-8") + + +def maint_cycle(tag, want_floor_w=LIVE_MAX_W * 0.3, **kw): + """One real cycle with maintenance in CHARGE and no forecast entity.""" + return wired_over_limit(TMP / tag, want_floor_w, store=MAINT_CHARGING, **kw) + + +base, base_hass, BASE_NET, BASE_REM = maint_cycle("mut-base.db") +res0 = base.maint.tick(datetime.now(timezone.utc), base.soc) +print(f" maintenance phase {res0.phase!r}: owns_setpoint={res0.owns_setpoint}, " + f"charge_only={res0.charge_only}; peak_forecast_entity absent so " + f"peak_fc={base.peak_fc}") +check("the fixture really is the dangerous one: maintenance in its CHARGE phase " + "(a P_MAINT_SHAPE `hi <= 0` limit) on an install with no " + "peak_forecast_entity, and the guard genuinely engaged on the measurement", + res0.charge_only and not res0.owns_setpoint and base.peak_fc is None + and base.peak_guard is not None and base.peak_guard.engaged + and base.peak_guard.blind is None) +print(f" SHIPPED: suspended=" + f"{peakguard.shaping_suspended(base.peak_guard, base.peak_fc, CAP_W)}, " + f"{base.reason}, target {base.target:+.1f} W") +check("SHIPPED: the shaping stands down, the arbiter does not contradict, and " + "the battery discharges into the peak", + peakguard.shaping_suspended(base.peak_guard, base.peak_fc, CAP_W) + and "CONTRADICTION" not in base.reason + and near(base.target, base.peak_guard.floor_w, 1.5) + and base.target > 0.0) + +# ---- B1: the suppression removed altogether -------------------------------- +_shipped_pred = peakguard.shaping_suspended +try: + peakguard.shaping_suspended = lambda d, fc, cap: False + b1, b1_hass, _, _ = maint_cycle("mut-b1.db") +finally: + peakguard.shaping_suspended = _shipped_pred +print(f" B1 (suppression removed): {b1.reason}, target {b1.target:+.1f} W") +check("B1: with the suppression gone the two LIMITS cross - `lo >= 0` from the " + "peak guard against `hi <= 0` from the shaping - and the arbiter answers " + "CONTRADICTION and 0 W. The battery is PARKED in the billed quarter, and " + "P_PEAK 50 > P_MAINT_SHAPE 40 does not save it", + "CONTRADICTION" in b1.reason and near(b1.target, 0.0) + and b1.peak_guard.engaged) + +# ---- B2: re-gated on peak_forecast_entity, i.e. the historical bug ---------- +# WARNING - THIS IS THE ONE THAT MUST NOT SHIP UNDEFENDED. It is verbatim the +# defect this card's own comment describes, and until this check existed the +# comment explained a fix that nothing could regress. That is the same shape as +# a corrected comment nobody tests, which this project has already been caught +# by once. +try: + peakguard.shaping_suspended = lambda d, fc, cap: peak_at_risk(fc, float(cap)) + b2, b2_hass, _, _ = maint_cycle("mut-b2.db") +finally: + peakguard.shaping_suspended = _shipped_pred +print(f" B2 (re-gated on peak_forecast_entity): " + f"peak_at_risk({b2.peak_fc}, {CAP_W:.0f}) = " + f"{peak_at_risk(b2.peak_fc, CAP_W)}, {b2.reason}, " + f"target {b2.target:+.1f} W") +check("B2: gating the suspension on peak_forecast_entity ALONE parks a default " + "install at 0 W. That entity is optional and empty there, so peak_at_risk " + "is False forever and the shaping is filed against the guard's floor - " + "silently, in the quarter that sets the month's bill", + not peak_at_risk(b2.peak_fc, CAP_W) + and "CONTRADICTION" in b2.reason and near(b2.target, 0.0)) +check("...and the shipped predicate differs from that mutation on exactly this " + "input, which is what makes the measured term load-bearing rather than " + "decorative", + _shipped_pred(base.peak_guard, None, CAP_W) is True + and peak_at_risk(None, CAP_W) is False) +check("...while the forecast term is not redundant either: on a site that DOES " + "set peak_forecast_entity, a forecast over the cap suspends the shaping " + "even before the measured guard engages", + _shipped_pred(None, CAP_W + 1000.0, CAP_W) is True + and _shipped_pred(None, None, CAP_W) is False) + +# ---- B3: the floor sized from intent rather than from delivered power ------- +# `decide()` is graded above with a hand-passed `batt_w`; nothing graded WHICH +# value main.py hands it. Recorded off the real call, with the measured battery +# and the standing command deliberately far apart. +STANDING = 3000.0 +_seen = {} +_shipped_decide = peakguard.decide + + +def _recording_decide(projection, **kw): + _seen.update(kw) + return _shipped_decide(projection, **kw) + + +try: + peakguard.decide = _recording_decide + b3, b3_hass, _, _ = maint_cycle("mut-b3.db", batt_w=0.0, standing_w=STANDING) +finally: + peakguard.decide = _shipped_decide +print(f" B3: measured battery {b3.batt:+.0f} W, command standing on the wire " + f"{STANDING:+.0f} W; main.py handed decide() batt_w={_seen.get('batt_w')}") +check("B3: main.py sizes the floor from the MEASURED battery power, not from " + "the command. The utility bills what FLOWED, and `grid = load - batt` is " + "true of delivered power and false of intent", + near(_seen.get("batt_w"), b3.batt) + and not near(_seen.get("batt_w"), STANDING)) +# THE CONSEQUENCE, ITERATED. One cycle of the intent form is merely inflated by +# the standing command; the failure is what happens on the NEXT cycle, when its +# own unmet ask has become the standing command. So both forms are run forward +# against the same frozen projection - the inverter never follows, so the meter +# never moves - and the two are allowed to separate over time rather than being +# judged at a single point where they need not differ. +_proj3 = PeakTracker.project(b3.p1.averager, datetime.now(timezone.utc)) +_kw = dict(cap_w=CAP_W, reading=b3.peak.peak(), achievable_w=LIVE_MAX_W, + max_hold_s=b3.p1.averager.max_hold_s) +_intent = _shipped_decide(_proj3, batt_w=STANDING, **_kw) +check("one cycle of the intent form is exactly the delivered form plus the " + "command standing on the wire - which is the whole defect in one line: " + "watts that were asked for and never arrived, counted as if they had", + near(_intent.floor_w - b3.peak_guard.floor_w, STANDING, 1.5)) + +_i_cmd, _d_batt = STANDING, 0.0 +_i_track, _d_track, _i_sat_at = [], [], None +for _k in range(1, 9): + _di = _shipped_decide(_proj3, batt_w=_i_cmd, **_kw) + _dd = _shipped_decide(_proj3, batt_w=_d_batt, **_kw) + _i_track.append(_di.floor_w) + _d_track.append(_dd.floor_w) + if _di.saturated and _i_sat_at is None: + _i_sat_at = _k + _i_cmd = _di.floor_w # the intent form's ask becomes what stands + # ...while the inverter delivers nothing, so the delivered form's input + # does not move at all. +print(f" ...iterated against a frozen meter and an inverter that never " + f"follows:") +print(f" intent-sized floor {[f'{v:.0f}' for v in _i_track]}" + f" -> SATURATED at cycle {_i_sat_at}") +print(f" delivered-power floor {[f'{v:.0f}' for v in _d_track]} -> stationary") +check("...and the consequence is the wind-up part 1 demonstrates, now on the " + "REAL controller's own projection: the intent-sized floor re-adds its own " + "unmet ask every cycle and reaches the rail SATURATED, while the " + "delivered-power floor does not move at all", + _i_sat_at is not None and _i_track[-1] > _i_track[0] + and len({f"{v:.3f}" for v in _d_track}) == 1 + and not b3.peak_guard.saturated) +# Graded on the parsed call syntax too, so a rename cannot quietly reintroduce +# it: the keyword main.py passes must be the measured attribute. +_call = next(n for n in ast.walk(ast.parse(_mainsrc)) + if isinstance(n, ast.Call) and isinstance(n.func, ast.Attribute) + and n.func.attr == "decide") +_batt_src = ast.unparse(next(k.value for k in _call.keywords if k.arg == "batt_w")) +print(f" main.py's decide(batt_w=...) is `{_batt_src}`") +check("...and the call site says so in the source: the argument is the measured " + "battery attribute, not the standing command and not the resolved target", + _batt_src == "self.batt") + +# SAFETY-24 must be pointed at from the module that needs it, not only from a +# review thread - a ticket nobody can find from the code is a ticket nobody +# reads before shipping this to a client install. +check("peakguard.py names SAFETY-24, the missing SoC backstop on the forced " + "discharge this guard can command", "SAFETY-24" in src) + + +# ============================================================================== # CFG-03: no fourth defaults site. cfg = (HERE / "config.yaml").read_text(encoding="utf-8") val = (HERE / "app" / "validate.py").read_text(encoding="utf-8") @@ -977,7 +1170,6 @@ check("peakguard.py reads no option of its own: it makes no `.get()` call at " # test_validate.py's CFG-03 block spells out at length: the two diverging is a # live defect in this repo for `maintenance_charge_w`, and it is not to be # repeated on the key this guard's ceiling comes from. -_mainsrc = (HERE / "app" / "main.py").read_text(encoding="utf-8") _fallbacks = {float(m) for m in re.findall(r'peak_cap_w"\s*,\s*([0-9]+(?:\.[0-9]+)?)', _mainsrc)} print(f" peak_cap_w: config.yaml ships {CAP_W:.0f}, main.py falls back to "