admin/goodwe-addon
1
0
Fork 0
Code Issues Pull Requests 4 Actions Packages Projects Releases Wiki Activity

SAFETY-04 review fixes: the freeze deadlocked, the bound was too loose

Browse Source 
S-1. The frozen branch admitted a correction only if it shrank |i_w|. That is
unsatisfiable for BOTH signs of error whenever |correction| > 2*|i_w|, i.e.
whenever the integrator is near zero, so the loop stopped moving and the freeze
could never clear - it clears when the inverter tracks, and not tracking is
what saturation means. Measured: 0 W held into a 2 kW import indefinitely,
where release/1.0 recovers on the next cycle. Re-encoded as the same asymmetric
rule the output freeze has always used: may not wind further in the direction
it is already pushing, may fall, cross zero or reverse. Same interpretation,
an encoding that cannot deadlock.

S-2. integrator_max_w defaulted to 1.5x max_w, which ADDED windup: in
release/1.0 the accumulator was the post-clamp command and could never pass the
rail. Default is now "follow max_w" (config 0 = unset). Measured on the 4000 W
load-drop sim, first cycle after the drop: 1000 W at the new default, 1800 W at
3000. DOCS row inverted - the useful direction is below max_w, and the 14 768 W
anecdote is a vendor controller, not evidence about this code.

S-3. The claim that i_w=None preserved release/1.0 exactly was false, because
the S-1 gate ran regardless of seeding. It is true again, and now asserted
rather than asserted-about: 3024-case exhaustive comparison against a
transcription of the old law, over both freeze states, both signs and either
side of the deadband. Added the carried-i_w convergence/overshoot sim that the
shipped configuration was missing.

S-4. Cycles are distinct meter values, not seconds: cycle() runs only when the
meter reading changes, so the window has no wall-clock bound. Comment and DOCS
corrected; the stall is detection latency, not a windup hazard, because the
same condition stalls the whole loop.

test_control.py: 33 -> 41 checks, all passing.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Du77usMj8XNKNFZGmUiWDa
This commit is contained in:
glenn schrooyen
2026-08-24 22:13:55 +02:00
co-authored by Claude Opus 5
parent 37bac79ad8
commit e46175559b
5 changed files with 176 additions and 58 deletions
Download Patch File Download Diff File Expand all files Collapse all files
goodwe_controller/DOCS.md
+1 -1
View File
@@ -60,7 +60,7 @@ phase having charged nothing.
| `step_w` | 10 | Quantisation |
| `saturation_w` | 500 | Divergence that counts as "the inverter is at a limit" |
| `saturation_cycles` | 3 | How many consecutive cycles before freezing. **Do not set to 1** |
| `integrator_max_w` | 3000 | Bound on the loop's accumulator, separate from `max_w`. Caps how much stale error can be waiting to unwind when the sign flips. **Keep it above `max_w`, and do not set it equal to `max_w`** |
| `integrator_max_w` | 0 | Bound on the loop's accumulator, and 0 means "same as `max_w`". Caps how much stale error can be waiting to unwind when the sign flips. **Do not raise it above `max_w`** - the output clamp already bounds what is commanded, so the only thing extra headroom buys is more cycles of wrong-direction power after every saturation event. Lowering it below `max_w` is the useful direction |
| `heartbeat_s` | 10 | Refresh interval; must stay well under the firmware watchdog |
| `stale_input_s` | 15 | How long inputs may be missing before commanding 0 W |
| `auto_start` | false | Start controlling on boot (only after commissioning) |
goodwe_controller/app/control.py
+67 -40
View File
@@ -28,16 +28,22 @@ class Tuning:
step_w: int = 10
saturation_w: float = 500.0
saturation_cycles: int = 3
# ⚠️ The integrator's OWN bound, and deliberately not max_w. A commercial
# controller on this same site clamped only its output and still reported
# 14 768 W: with the inverter switched off its integrator climbed ~130 W
# every 4 s past 10 kW while the output sat on the 5 kW rail, so the moment
# the error flipped there were minutes of accumulated wind to burn off
# before the command moved at all. Bounding the accumulator is what makes
# recovery time finite; bounding the output only hides it.
# Headroom above max_w is wanted (a legitimate large error must not be
# truncated at the rail), headroom without limit is the bug.
integrator_max_w: float = 3000.0
# The integrator's own bound. None means "follow max_w", which is the
# default and the recommended setting.
#
# ⚠️ DO NOT RAISE THIS ABOVE max_w without a measurement to justify it.
# Every watt of integrator above the rail is a watt of wind that has to be
# burned off before the command can start moving the other way, i.e. extra
# cycles of discharge into an already-exporting meter after every
# saturation event. Measured on the closed-loop sim, 4000 W load dropped to
# 0: at integrator_max_w == max_w the command is 1000 W two cycles later; at
# 1.5x max_w it is 1800 W. The output clamp already bounds what reaches the
# wire, so headroom here buys nothing but unwind latency.
#
# It is a separate key because it has to be able to be SMALLER than max_w,
# which is the only direction that buys anything: it caps unwind latency
# below what the rail implies. Merging it into max_w would take that away.
integrator_max_w: float | None = None
# What the meter should rest at, in W. Negative = a slight export.
# ⚠️ The deadband is a one-way ratchet: any resting point inside it holds
# forever, and the meter's IMPORT register counts every positive one with
@@ -54,9 +60,9 @@ class Decision:
sat_count: int
frozen: bool
reason: str
# The integrator AFTER this cycle, pre-clamp-to-max_w. Carry it back in as
# `i_w` next cycle; that is what keeps it a separate quantity from the
# command, which is the whole point of the bound above.
# The integrator AFTER this cycle, before the output clamp, the slew limit
# and quantisation. Carry it back in as `i_w` next cycle; that is what keeps
# it a separate quantity from the command.
i_w: float = 0.0
@@ -89,14 +95,19 @@ def compute(
# lets slew be larger than saturation_w.
#
# The spec states this window twice and differently: "> 10 s" (§11.2) and
# "3 samples" (§10.3). Cycles are authoritative here because this function
# has no clock - it is driven one cycle per meter update by run_control(),
# which only calls cycle() when the meter value changes. At the ~5 s
# HomeWizard P1 cadence the default 3 cycles is ~15 s, i.e. the stricter
# reading of the two. On a faster meter it is not, so saturation_cycles is
# configurable and must be raised to keep the window over 10 s.
# ponytail: a seconds-based window would mean plumbing wall-clock or dt
# into a pure function whose whole value is that it has neither.
# "3 samples" (§10.3). This counts CYCLES, and a cycle is not a unit of
# time: run_control() calls cycle() only when the meter value CHANGES
# (`if self.grid != last_grid`), so three cycles is three distinct meter
# readings and nothing more. At the reference P1's ~5 s update rate that is
# usually ~15 s, but there is no upper bound on it - a meter that repeats a
# value stalls the counter.
#
# That is a detection-latency limit, not a windup hazard: the same
# condition that stalls the counter stalls the whole loop, so nothing
# accumulates in the meantime either. If a wall-clock window is ever
# required, it belongs in Controller (which has a clock) and not here.
# ponytail: this function is worth keeping clockless; the ceiling is that
# saturation_cycles cannot express a guaranteed number of seconds.
saturated_now = abs(prev_w - actual_w) > tuning.saturation_w
sat_count = min(sat_count + 1, 10) if saturated_now else 0
frozen = sat_count >= tuning.saturation_cycles
@@ -112,32 +123,48 @@ def compute(
# --- the integrator ----------------------------------------------------
# This loop is in velocity form: the accumulator IS the commanded power, so
# for years "the integrator" and "the output" were one variable and could
# not be bounded apart. `i_w` is that accumulator made explicit. A caller
# that passes nothing gets the old behaviour exactly - seeded from the last
# command every cycle - and main.py carries it instead, which is what turns
# the two clamps below into two independent limits.
# "the integrator" and "the output" were one variable and could not be
# bounded apart. `i_w` is that accumulator made explicit; main.py carries it
# between cycles, which is what turns the two clamps into two limits.
#
# Passing i_w=None re-seeds it from the last command every cycle. With
# integrator_max_w following max_w that reduces this function to the exact
# velocity form it replaced, frozen branch included - asserted by an
# exhaustive comparison against a transcription of the old law in
# test_control.py, not by inspection. Break either the gate or the bound
# below and that test is what tells you the equivalence went with it.
if i_w is None:
i_w = float(prev_w)
limit = tuning.max_w if tuning.integrator_max_w is None else tuning.integrator_max_w
if abs(error) < tuning.deadband_w:
reason = "deadband"
else:
step_i = tuning.gain * error
# ⚠️ Freeze means "may not wind FURTHER", not "may not move". A strict
# freeze would strand the command at whatever it had reached until the
# inverter started tracking again - and the inverter is not tracking,
# that is what saturation means, so nothing would ever release it. The
# unwind direction is the escape route and stays open; the same rule is
# applied again to the output below.
if not frozen or abs(i_w + step_i) < abs(i_w):
i_w = i_w + step_i
moved = i_w + tuning.gain * error
# ⚠️ Freeze means "may not wind FURTHER in the direction it is already
# pushing". It may fall, cross zero, or reverse outright.
#
# It must NOT be encoded as "only corrections that shrink |i_w|": that
# is unsatisfiable for BOTH signs of error whenever the correction is
# larger than twice the integrator, i.e. every time the integrator is
# near zero. The loop then sits at its last value forever, because what
# clears the freeze is the inverter tracking again and not-tracking is
# the definition of saturation. Measured on that encoding: 0 W held
# indefinitely into a 2 kW import, where this form recovers next cycle.
#
# This is the same asymmetric rule the output freeze uses below, which
# has been in service on real hardware. It is applied here as well
# because the requirement is that the INTEGRATOR stop accumulating, not
# only the command.
if not frozen:
i_w = moved
else:
i_w = min(moved, i_w) if i_w > 0 else max(moved, i_w)
# ⚠️ Applied EVERY cycle, frozen or not, and before the output clamp: the
# freeze is conditional, this bound is not. Order matters only in that the
# command below is derived from the already-bounded integrator, so no
# accumulated value can reach the wire even once.
i_w = max(-tuning.integrator_max_w, min(tuning.integrator_max_w, i_w))
# ⚠️ 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))
want = i_w
# ⚠️ Maintenance shaping (charge-only, cheap-window floor) used to live
goodwe_controller/app/main.py
+5 -1
View File
@@ -70,7 +70,11 @@ class Controller:
step_w=int(opts.get("step_w", 10)),
saturation_w=float(opts.get("saturation_w", 500)),
saturation_cycles=int(opts.get("saturation_cycles", 3)),
integrator_max_w=float(opts.get("integrator_max_w", 3000)),
# 0 / unset means "follow max_w", which is the recommended
# value. Read the note in control.py before raising it above
# max_w: every watt above the rail is unwind latency.
integrator_max_w=(float(opts["integrator_max_w"])
if opts.get("integrator_max_w") else None),
)
self.maint = Maintenance(
MaintConfig(
goodwe_controller/config.yaml
+2 -2
View File
@@ -48,7 +48,7 @@ options:
step_w: 10
saturation_w: 500
saturation_cycles: 3
integrator_max_w: 3000
integrator_max_w: 0
heartbeat_s: 10
stale_input_s: 15
auto_start: false
@@ -89,7 +89,7 @@ schema:
step_w: int(1,100)
saturation_w: int(100,2000)
saturation_cycles: int(1,10)
integrator_max_w: int(100,15000)
integrator_max_w: int(0,15000)
heartbeat_s: int(2,25)
stale_input_s: int(5,120)
auto_start: bool
goodwe_controller/test_control.py
+101 -14
View File
@@ -78,7 +78,7 @@ print("SAFETY-04: the integrator is bounded apart from the output")
# The historical runaway, with its real numbers. A commercial controller on
# this site, with the inverter switched OFF, wound ~130 W every 4 s past 10 kW
# and reported 14 768 W while its output clamp sat at 5 kW. At gain 0.6 that
# rate is a standing error of 130/0.6 ≈ 217 W that never resolves, because the
# rate is a standing error of 130/0.6 = 217 W that never resolves, because the
# inverter is not there to resolve it. 150 cycles is past the ~113 it took to
# reach 14 768 W at that rate.
RUNAWAY_ERROR = 130.0 / 0.6
@@ -99,34 +99,53 @@ def runaway(tuning):
return worst_i, worst_cmd
TR = Tuning(max_w=2000, integrator_max_w=3000)
TR = Tuning(max_w=2000) # integrator_max_w unset => follows max_w
wi, wc = runaway(TR)
check(f"runaway: integrator plateaus at {wi:.0f} W (<= 3000)", wi <= TR.integrator_max_w)
check(f"runaway: integrator plateaus at {wi:.0f} W (<= 2000)", wi <= TR.max_w)
check(f"runaway: emitted command peaks at {wc:.0f} W (<= 2000)", wc <= TR.max_w)
check("runaway: nowhere near the historical 14 768 W", wc < HISTORICAL_W / 4)
# ...and with the saturation detector deliberately defeated, so that only the
# clamp is holding. This is the AC that says the two mechanisms are
# independent: kill one, the other still bounds it.
TD = Tuning(max_w=2000, integrator_max_w=3000, saturation_w=1e9)
# clamp is holding. Kill one mechanism, the other still bounds it.
TD = Tuning(max_w=2000, saturation_w=1e9)
wi, wc = runaway(TD)
check(f"runaway with the detector defeated: integrator still <= 3000 ({wi:.0f} W)",
wi <= TD.integrator_max_w)
check(f"runaway with the detector defeated: integrator still bounded ({wi:.0f} W)",
wi <= TD.max_w)
check("runaway with the detector defeated: command still <= max_w", wc <= TD.max_w)
# The bound is not max_w. If someone "simplifies" them into one key this fails.
# The bound is a separate quantity, and the useful direction is BELOW max_w:
# there it binds first and caps unwind latency tighter than the rail does.
d = compute(prev_w=0, grid_w=6000, actual_w=0,
tuning=Tuning(max_w=2000, integrator_max_w=3000, slew_w=5000))
check("integrator bound is separate from the output clamp",
d.i_w == 3000 and d.target_w == 2000)
tuning=Tuning(max_w=2000, integrator_max_w=1000, slew_w=5000))
check("integrator bound binds independently of the output clamp",
d.i_w == 1000 and d.target_w == 1000)
# Freeze = does not accumulate. Same input twice; the integrator must not move.
# Freeze = may not wind further in the direction it is already pushing.
TF = Tuning(saturation_w=500, saturation_cycles=3)
f1 = compute(prev_w=2000, grid_w=800, actual_w=0, tuning=TF, sat_count=3, i_w=2000.0)
check("frozen: integration does not accumulate", f1.i_w == 2000.0 and f1.frozen)
check("frozen: integration does not wind further", f1.i_w == 2000.0 and f1.frozen)
f2 = compute(prev_w=2000, grid_w=-800, actual_w=0, tuning=TF, sat_count=3, i_w=2000.0)
check("frozen: unwinding is still allowed", f2.i_w < 2000.0)
# ⚠️ REGRESSION, and the reason the first cut of SAFETY-04 was rejected. A
# freeze encoded as "only corrections that shrink |i_w|" is unsatisfiable for
# BOTH signs of error whenever |correction| > 2*|i_w|, so near zero the loop
# stops moving forever - the freeze cannot clear, because clearing it needs the
# inverter to track and not-tracking is what saturation means. Measured on that
# encoding: 0 W held into a 2 kW import for as long as the sim ran.
z = compute(prev_w=0, grid_w=2000, actual_w=600, tuning=T, sat_count=3, i_w=0.0)
check("frozen at i_w=0: a 2 kW import still moves the command",
z.frozen and z.target_w == 1000)
# ...and the next cycle the inverter is inside saturation_w of the command, so
# the freeze clears on its own. Deadlock would show up here as frozen=True.
z2 = compute(prev_w=1000, grid_w=1000, actual_w=600, tuning=T,
sat_count=z.sat_count, i_w=z.i_w)
check("frozen at i_w=0: the freeze then clears", not z2.frozen)
# Same stranding on the other side: a small positive integrator against export.
z3 = compute(prev_w=100, grid_w=-1000, actual_w=800, tuning=T, sat_count=3, i_w=100.0)
check("frozen at i_w=+100: a 1 kW export still moves the command",
z3.frozen and z3.target_w < 0)
# False-positive guard: a normal 2 kW load step must not trip the detector,
# because the plant needs several cycles to catch up on every one of them.
prev, actual, sat, i_w, froze = 0.0, 0.0, 0, 0.0, False
@@ -137,6 +156,74 @@ for _ in range(12):
froze = froze or d.frozen
check("a normal 2 kW load step does not trip the saturation freeze", not froze)
# The convergence sim below runs WITHOUT a carried integrator. This is the same
# 2 kW step in the configuration that actually ships, where main.py carries it.
prev, actual, sat, i_w = 0.0, 0.0, 0, 0.0
carried = 0
for _ in range(12):
d = compute(prev, 2000.0 - actual, actual, T, sat, i_w)
prev, sat, i_w = d.target_w, d.sat_count, d.i_w
actual = actual + 0.94 * (prev - actual)
carried += 1
if abs(2000.0 - actual) < T.deadband_w:
break
check(f"carried integrator converges in {carried} cycles (<=6)", carried <= 6)
check("carried integrator does not overshoot the load", actual <= 2000.0 + T.deadband_w)
# ⚠️ REGRESSION: an integrator allowed to wind past the rail buys nothing (the
# output clamp already bounds the wire) and costs extra cycles of
# wrong-direction power after every saturation event. 4000 W load held to
# saturation, then dropped to 0; the figure is the command on the first cycle
# after the drop. This is what makes the DOCS advice checkable.
def unwind(t):
prev, actual, sat, i_w, load = 0.0, 0.0, 0, 0.0, 4000.0
for c in range(16):
if c == 15:
load = 0.0
d = compute(prev, load - actual, actual, t, sat, i_w)
prev, sat, i_w = d.target_w, d.sat_count, d.i_w
actual = actual + 0.94 * (prev - actual)
return prev
tight, loose = unwind(Tuning(max_w=2000)), unwind(Tuning(max_w=2000, integrator_max_w=3000))
check(f"after saturation ends the command is {tight:.0f} W (<= 1000)", tight <= 1000)
check(f"headroom above max_w makes that worse ({loose:.0f} W) - hence the default",
loose > tight)
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."""
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
target = max(-t.max_w, min(t.max_w, want))
target = max(prev - t.slew_w, min(prev + t.slew_w, target))
if frozen:
target = min(target, prev) if prev > 0 else max(target, prev)
step = max(1, int(t.step_w))
return float(round(target / step) * step), sc
# 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.
diffs = []
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"
+ (f" (first diff {diffs[0]})" if diffs else ""), not diffs)
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)