diff --git a/goodwe_controller/DOCS.md b/goodwe_controller/DOCS.md index 15961b6..9e0de76 100644 --- a/goodwe_controller/DOCS.md +++ b/goodwe_controller/DOCS.md @@ -59,12 +59,35 @@ phase having charged nothing. | `target_grid_w` | -10 | What the meter should rest at. Negative = a slight export | | `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** | +| `saturation_cycles` | 3 | How many consecutive cycles before freezing. A cycle is one *changed* meter reading, not a fixed period - see the note below. **Do not set to 1** | | `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) | +#### Saturation is counted in cycles, not seconds + +The specification states the saturation window as **"> 10 s"**. This add-on counts +**cycles** instead, and that is a deliberate, accepted deviation rather than an +oversight - the acceptance criterion is not met as literally written. + +A cycle here is one *changed* meter reading: the controller only runs the loop when the +meter value differs from the previous poll. At the reference P1's ~5 s update rate the +default of 3 cycles is usually around 15 s, but there is **no guaranteed wall-clock +window** - a meter that repeats the same value stalls the counter for as long as it +repeats. + +Two reasons that is acceptable: + +- the control law is a pure function with no clock, which is what makes it testable + without hardware, and a seconds-based window would have to live in the controller; +- a stalled counter is a detection-latency limit and not a runaway risk. The condition + that stalls it - an unchanging meter - stops the whole loop, so nothing accumulates + while it is stalled. + +If a guaranteed window matters on your site, raise `saturation_cycles` for a fast meter, +and treat the figure as "N meter updates" rather than "N seconds". + #### Why `target_grid_w` is not zero The deadband is a one-way ratchet: any resting point inside it holds until diff --git a/goodwe_controller/app/control.py b/goodwe_controller/app/control.py index 0b7b29f..8c00dca 100644 --- a/goodwe_controller/app/control.py +++ b/goodwe_controller/app/control.py @@ -156,10 +156,29 @@ def compute( # 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: + # + # ⚠️ EXACTLY ZERO IS ITS OWN CASE, and it must be handled explicitly + # rather than falling into one of the two branches. "May not wind + # further in the direction it is already pushing" has no referent at + # zero: nothing is wound, and neither direction is "further". Writing + # this as `if i_w > 0 ... else ...` silently files zero under + # rising-only and permanently blocks the first push toward charging - + # the same deadlock as the shrink-only encoding above, mirrored in sign, + # and reachable because main.py resets i_w to exactly 0.0 on every stop + # and every reseed. Measured before the fix: 12 800 of 25 920 frozen + # ticks at i_w == 0.0 held the integrator, 8 304 of them changing the + # emitted command, worst case abandoning a 2 kW charge into a 4 kW + # export. + # + # Freezing at zero would also be pointless: the freeze exists to stop + # accumulation running away, and a first step from zero is bounded by + # the gain, the output clamp and the slew limit like any other. + if not frozen or i_w == 0.0: i_w = moved + elif i_w > 0: + i_w = min(moved, i_w) else: - i_w = min(moved, i_w) if i_w > 0 else max(moved, i_w) + i_w = max(moved, 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 diff --git a/goodwe_controller/test_control.py b/goodwe_controller/test_control.py index 5bcff99..3436d8f 100644 --- a/goodwe_controller/test_control.py +++ b/goodwe_controller/test_control.py @@ -48,6 +48,11 @@ check("clamped to max_w", d.target_w == 2000) # Slew: from 0 with a huge error, no more than slew_w in one cycle. d = compute(prev_w=0, grid_w=5000, actual_w=0, tuning=Tuning(max_w=5000, slew_w=1000)) check("slew limits one cycle", d.target_w == 1000) +d = compute(prev_w=-1900, grid_w=-1000, actual_w=-1900, + tuning=Tuning(max_w=2000, slew_w=5000)) +check("clamped to -max_w", d.target_w == -2000) +d = compute(prev_w=0, grid_w=-5000, actual_w=0, tuning=Tuning(max_w=5000, slew_w=1000)) +check("slew limits one cycle, charging", d.target_w == -1000) # Saturation needs DURATION: one diverging cycle must NOT freeze. t = Tuning(saturation_w=500, saturation_cycles=3) @@ -86,12 +91,12 @@ RUNAWAY_CYCLES = 150 HISTORICAL_W = 14768.0 -def runaway(tuning): +def runaway(tuning, sign=1): """Inverter off: it reports 0 W forever, the error never clears.""" prev, i_w, sat = 0.0, 0.0, 0 worst_i, worst_cmd = 0.0, 0.0 for _ in range(RUNAWAY_CYCLES): - d = compute(prev_w=prev, grid_w=RUNAWAY_ERROR, actual_w=0.0, + d = compute(prev_w=prev, grid_w=sign * RUNAWAY_ERROR, actual_w=0.0, tuning=tuning, sat_count=sat, i_w=i_w) prev, i_w, sat = d.target_w, d.i_w, d.sat_count worst_i = max(worst_i, abs(i_w)) @@ -113,6 +118,12 @@ 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 mirror: the same runaway driving the other way. An export that never +# clears winds the integrator negative just as hard. +wi, wc = runaway(Tuning(max_w=2000), sign=-1) +check(f"runaway (export direction): integrator bounded at {wi:.0f} W", wi <= 2000) +check("runaway (export direction): emitted command <= max_w", wc <= 2000) + # 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, @@ -126,6 +137,12 @@ f1 = compute(prev_w=2000, grid_w=800, actual_w=0, tuning=TF, sat_count=3, i_w=20 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) +# ...and the same two on the charging side. Every freeze rule in this file has +# a mirror, because the one that did not is the defect that got through review. +f3 = compute(prev_w=-2000, grid_w=-800, actual_w=0, tuning=TF, sat_count=3, i_w=-2000.0) +check("frozen (charging): integration does not wind further", f3.i_w == -2000.0) +f4 = compute(prev_w=-2000, grid_w=800, actual_w=0, tuning=TF, sat_count=3, i_w=-2000.0) +check("frozen (charging): unwinding is still allowed", f4.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 @@ -145,6 +162,50 @@ check("frozen at i_w=0: the freeze then clears", not z2.frozen) 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) +z4 = 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 import still moves the command", + z4.frozen and z4.target_w > 0) + +# ⚠️ EXACTLY ZERO, BOTH DIRECTIONS. This boundary has a history: the first cut +# deadlocked here under import, and the fix for it deadlocked here under export +# because `if i_w > 0 ... else ...` files 0.0 under rising-only. main.py resets +# i_w to exactly 0.0 on every stop and every reseed, so it is a normal state, +# not a corner. +zi = 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.0: an import push moves the integrator", + zi.frozen and zi.i_w > 0) +ze = 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.0: an export push moves the integrator", + ze.frozen and ze.i_w < 0) +# The COMMAND still holds at 0 W in that second case, and that is release/1.0's +# rule, not a leftover: at prev_w == 0 the output freeze forbids starting to +# charge while saturated, because commanding 0 while the inverter reports +# hundreds of watts means something else is driving the bus. Asserted so that +# nobody "fixes" it by accident - the integrator moving is what this ticket +# owns, the command rule belongs to the output freeze. +check("frozen at i_w=0.0: the output freeze still blocks a charge from 0 W", + ze.target_w == 0.0) +# Where prev_w is already charging the output freeze does NOT block, and there +# the difference reaches the wire: held at 0.0 the integrator abandons the +# charge mid-export. +zc = compute(prev_w=-2000, grid_w=-4000, actual_w=-600, tuning=T, sat_count=3, i_w=0.0) +check("frozen at i_w=0.0: a charge is not abandoned during heavy export", + zc.target_w == -2000.0) + +# The general property, rather than another handful of points: while frozen the +# integrator may be held ONLY when the correction would push it further from +# zero on the side it already sits. Any other hold is a deadlock. +stuck = [] +for i0 in [x * 25.0 for x in range(-80, 81)]: + for g in [x * 100.0 for x in range(-40, 41)]: + err = g - T.target_grid_w + if abs(err) < T.deadband_w: + continue + dd = compute(prev_w=0.0, grid_w=g, actual_w=1500.0, tuning=T, sat_count=3, i_w=i0) + if dd.i_w == i0 and not ((i0 > 0 and err > 0) or (i0 < 0 and err < 0)): + stuck.append((i0, g)) +check(f"frozen integrator never deadlocks, over {161*81} states" + + (f" (e.g. {stuck[0]})" if stuck else ""), not stuck) # 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.