From 7123aa00a496b9a7fcb524c308e40df0da60b1f4 Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Mon, 24 Aug 2026 22:29:10 +0200 Subject: [PATCH] SAFETY-04: revive the clamp reason, and compare reasons in the sweep `want = i_w` after the integrator bound, so at the default limit == max_w the output clamp can never fire and `reason == "clamped"` had become unreachable. Observability only today - nothing gates on the string - but SAFETY-03 exists to alarm on exactly that engagement, so its hook was dead before it was built. The integrator bound now reports "i-clamped", and that is the signal SAFETY-03 must watch: it is the one that fires on a default install. "clamped" stays reachable for a configuration that lets the integrator run above the rail, where both fire and the output clamp - which describes the value actually emitted - is the one reported. Two names because the two events want different alarms: the loop winding, versus a command that came out over the rating. The real fix is the second half. The equivalence sweep compared (target_w, sat_count), which is how a dead reason survived 3024 cases. It now compares (target_w, sat_count, frozen, reason) and it catches this defect: dropping the emit turns it red. Deliberate rename aliased explicitly, so any OTHER reason divergence still fails. Result of adding reason to the tuple: 105 of 3024 cases differ, and every one of them is the i-clamped/clamped rename. Zero value divergences, `frozen` included. Nothing else surfaced. test_control.py: 41 -> 43 checks, all passing. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Du77usMj8XNKNFZGmUiWDa --- goodwe_controller/app/control.py | 13 +++++++- goodwe_controller/test_control.py | 53 +++++++++++++++++++++++++------ 2 files changed, 55 insertions(+), 11 deletions(-) diff --git a/goodwe_controller/app/control.py b/goodwe_controller/app/control.py index 9dbcae6..0b7b29f 100644 --- a/goodwe_controller/app/control.py +++ b/goodwe_controller/app/control.py @@ -164,7 +164,18 @@ def compute( # ⚠️ Applied EVERY cycle, frozen or not: the freeze is conditional, this # bound is not. It is what makes the worst-case unwind time finite and # knowable instead of a function of how long the error happened to stand. - i_w = max(-limit, min(limit, i_w)) + bounded = max(-limit, min(limit, i_w)) + if bounded != i_w: + # ⚠️ SAFETY-03 (alarm whenever the loop winds into a rail) must watch + # for THIS, not for "clamped" below. At the default limit == max_w the + # integrator bound is reached first and the command derived from it can + # then never exceed max_w, so "clamped" is unreachable on a default + # install - it survives only for a configuration that deliberately lets + # the integrator run above the rail. Two reasons rather than one + # because the two events want different alarms: "i-clamped" is the loop + # winding, "clamped" is a command that came out over the rating anyway. + reason = "i-clamped" + i_w = bounded want = i_w # ⚠️ Maintenance shaping (charge-only, cheap-window floor) used to live diff --git a/goodwe_controller/test_control.py b/goodwe_controller/test_control.py index 1cd1e07..5bcff99 100644 --- a/goodwe_controller/test_control.py +++ b/goodwe_controller/test_control.py @@ -196,34 +196,67 @@ print("SAFETY-04: the i_w=None path is still release/1.0, exactly") def legacy(prev, grid, actual, t, sat_count): """release/1.0's control law, transcribed. Do not 'improve' this.""" + reason = "tracking" sc = min(sat_count + 1, 10) if abs(prev - actual) > t.saturation_w else 0 frozen = sc >= t.saturation_cycles error = grid - t.target_grid_w - want = prev if abs(error) < t.deadband_w else prev + t.gain * error + if abs(error) < t.deadband_w: + want, reason = prev, "deadband" + else: + want = prev + t.gain * error target = max(-t.max_w, min(t.max_w, want)) - target = max(prev - t.slew_w, min(prev + t.slew_w, target)) + if target != want: + reason = "clamped" + slewed = max(prev - t.slew_w, min(prev + t.slew_w, target)) + if slewed != target: + reason = "slew-limited" + target = slewed if frozen: target = min(target, prev) if prev > 0 else max(target, prev) + reason = "saturated-freeze" step = max(1, int(t.step_w)) - return float(round(target / step) * step), sc + return float(round(target / step) * step), sc, frozen, reason -# Exhaustive over the interesting corners, both freeze states, both signs, and -# either side of the deadband. This is what makes the claim in control.py's -# integrator comment a checked fact rather than an assertion. +# ⚠️ Compare EVERYTHING observable, not just the number. A previous version of +# this sweep compared (target_w, sat_count) only and passed 3024 cases while +# `reason` had silently lost a value - which is the kind of thing a sweep this +# broad exists to catch. `frozen` and `reason` are both in the tuple now. +# +# The one deliberate rename: what release/1.0 called "clamped" is now +# "i-clamped", because the truncation happens on the integrator before the +# command is derived from it. Aliased here rather than papered over - if any +# OTHER reason ever diverges, this check goes red. +ALIAS = {"i-clamped": "clamped"} diffs = [] +seen = set() for tune in (Tuning(), Tuning(target_grid_w=-10.0), Tuning(max_w=5000, slew_w=5000)): for prev in (-2000.0, -500.0, -100.0, 0.0, 100.0, 500.0, 2000.0): for grid in (-6000.0, -1000.0, -500.0, -14.0, 0.0, 14.0, 500.0, 1000.0, 6000.0): for actual in (-2000.0, 0.0, 600.0, 2000.0): for sc in (0, 2, 3, 9): d = compute(prev, grid, actual, tune, sc) # i_w defaults to None - lt, lsc = legacy(prev, grid, actual, tune, sc) - if (d.target_w, d.sat_count) != (lt, lsc): - diffs.append((prev, grid, actual, sc, d.target_w, lt)) -check(f"i_w=None reproduces release/1.0 over {3*7*9*4*4} cases" + seen.add(d.reason) + got = (d.target_w, d.sat_count, d.frozen, + ALIAS.get(d.reason, d.reason)) + if got != legacy(prev, grid, actual, tune, sc): + diffs.append((prev, grid, actual, sc, got, + legacy(prev, grid, actual, tune, sc))) +check(f"i_w=None reproduces release/1.0 over {3*7*9*4*4} cases, reason included" + (f" (first diff {diffs[0]})" if diffs else ""), not diffs) +# ...and the rename is not a quiet deletion: the signal SAFETY-03 alarms on has +# to actually occur in that sweep, or its hook is dead. +check("the integrator clamp reports itself as 'i-clamped'", "i-clamped" in seen) + +# "clamped" stays reachable, but only where the integrator is deliberately +# allowed above the rail - then BOTH fire and the output clamp, which describes +# the value actually emitted, is the one reported. +dc = compute(prev_w=0, grid_w=6000, actual_w=0, + tuning=Tuning(max_w=2000, integrator_max_w=3000, slew_w=5000)) +check("the output clamp still reports 'clamped' when it is the binding one", + dc.reason == "clamped" and dc.i_w == 3000 and dc.target_w == 2000) + print("capacity tariff") check("no forecast means no cap", maintenance_charge_floor(2500, None, 3500) == 2500) check("headroom caps the charge", maintenance_charge_floor(2500, 2000, 3500) == 1500)