fix: allow gentle negative HOLD floor to fix steady-state overshoot
Testing the windup-fix rework live against sude/sud_0030.json surfaced a
distinct bug: a HOLD overshoot from grain-fill-in cooling never decayed -
process_pid()'s pid_outer_y floor clamped to exactly 0.0, so pid_inner
fought the pot's own ambient loss to hold the overshot temperature flat
instead of declining back to setpoint (see docs/overshoot2.png).
Replace the hardcoded 0.0 floor with a configurable Outer.y_hold_min
(default 0.0, backward compatible), set to -0.1 in config.json, both
.tpl templates, and the demo scripts - small enough to avoid
reintroducing the bb5af3c limit cycle while letting HOLD request a
gentle decline matching passive ambient cooling.
Adds TestHoldOvershootRecoversToSetpoint and documents the finding in
docs/overshoot_hold_windup.md's Follow-up section and
components/pid/TODO.md. Confirmed against a live sud_0030 re-run, not
just the unit test.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M2ierBoxW3v7nUbDw3M2pE
This commit is contained in:
Claude Sonnet 5
@@ -99,6 +99,25 @@ git history for `temp_controller.py`/`temp_controller_smith.py`).
|
||||
cases) was added under `tests/components/pid/` — see the "No automated
|
||||
tests" item above.
|
||||
|
||||
- [x] **HOLD overshoot was a permanent steady-state offset, not just a
|
||||
transient windup.** Found testing the rework against a real Sud run
|
||||
(`sude/sud_0030.json`): a grain-fill-in disturbance during a `HOLD`
|
||||
overshot by ~0.4°C (under the `HoldCool` threshold, so the FSM stayed in
|
||||
`HOLD`) and then never converged back — `temp_ist` sat in a 55.3-55.4°C
|
||||
band for the rest of the 20-minute hold instead of returning to 55.0.
|
||||
Cause: `process_pid()`'s HOLD-state floor on `pid_outer_y` clamped to
|
||||
exactly `0.0`, so once overshot, `heatrate_soll` = 0 ("hold flat") and
|
||||
`pid_inner` actively fought the pot's own ambient loss to keep the
|
||||
overshot temperature flat instead of declining back to setpoint. Fixed:
|
||||
the floor is now a configurable `Outer.y_hold_min` (default `0.0`,
|
||||
backward compatible), set to `-0.1` in `config.json`/both `.tpl`
|
||||
templates, letting HOLD request a gentle decline that roughly matches
|
||||
passive ambient cooling without reopening the `bb5af3c` limit cycle
|
||||
(fully unclamped negative). See the "Follow-up" section in
|
||||
`docs/overshoot_hold_windup.md` and
|
||||
`TestHoldOvershootRecoversToSetpoint` in
|
||||
`tests/components/pid/test_temp_controller_closed_loop.py`.
|
||||
|
||||
- [ ] **`kalman.py` is now dead code in production.** Neither
|
||||
`temp_controller.py` nor `temp_controller_smith.py` uses `Kalman`
|
||||
anymore; the only remaining references are
|
||||
|
||||
@@ -21,6 +21,7 @@ class TempControllerBase(TempControllerFsm, APid):
|
||||
self.params = None
|
||||
self._inner_heat_params = None
|
||||
self._inner_hold_params = None
|
||||
self._outer_hold_y_min = 0.0
|
||||
self.y = -1
|
||||
# Heat-rate pre-filter state (option A) - None until first process() tick
|
||||
self.last_theta_ist = None
|
||||
@@ -32,6 +33,11 @@ class TempControllerBase(TempControllerFsm, APid):
|
||||
self.thresholds = {**DEFAULT_THRESHOLDS, **params.get('Thresholds', {})}
|
||||
self.beta = params.get('beta', 0.05)
|
||||
self.pid_outer.set_params(params['Outer'])
|
||||
# How far into "actively cool" pid_outer is allowed to reach during
|
||||
# HOLD - see the floor in process_pid() below. Defaults to 0.0
|
||||
# (old flat-clamp behaviour) so configs that don't set it are
|
||||
# unaffected.
|
||||
self._outer_hold_y_min = params['Outer'].get('y_hold_min', 0.0)
|
||||
self._inner_heat_params = params['Inner']['Heat']
|
||||
self._inner_hold_params = params['Inner']['Hold']
|
||||
self.pid_inner_cool.set_params(params['Inner']['Cool'])
|
||||
@@ -135,14 +141,21 @@ class TempControllerBase(TempControllerFsm, APid):
|
||||
|
||||
def process_pid(self, theta_err, hold_scale=1.0):
|
||||
self.pid_outer.process(theta_err, -self.theta_ist, hold_scale)
|
||||
# In HOLD state, clamp pid_outer's output to [0, 1]: a small temperature
|
||||
# overshoot makes pid_outer.y go negative, which would invert heatrate_soll
|
||||
# and drive pid_inner's power to 0, causing a limit cycle (power on →
|
||||
# overshoot → power off → coast down → repeat). Clamping to 0 lets the
|
||||
# outer loop reduce the inner setpoint to zero but no further.
|
||||
# In HOLD state, floor pid_outer's output at Outer.y_hold_min (<= 0)
|
||||
# rather than letting it swing all the way to -1: a large negative
|
||||
# heatrate_soll asks for a decline far steeper than the pot's own
|
||||
# ambient heat loss can deliver, so pid_inner pins power at 0 for an
|
||||
# extended stretch, undershoots, and swings back hard - a ~110s
|
||||
# limit cycle (see git history for this clamp). A small negative
|
||||
# floor instead lets HOLD ask for a gentle decline that roughly
|
||||
# matches passive cooling, so a small overshoot coasts back down to
|
||||
# setpoint instead of sitting in a permanent flat-clamp dead zone
|
||||
# (the outer loop otherwise commands "stay flat" forever, and
|
||||
# pid_inner actively fights the pot's own ambient loss to hold that
|
||||
# flat line - see docs/overshoot_hold_windup.md).
|
||||
pid_outer_y = self.pid_outer.get_y()
|
||||
if self.state == States.HOLD:
|
||||
pid_outer_y = max(0.0, pid_outer_y)
|
||||
pid_outer_y = max(self._outer_hold_y_min, pid_outer_y)
|
||||
self.heatrate_soll = self.heatrate_soll_set * pid_outer_y
|
||||
heatrate_err = self.heatrate_soll - self.heatrate_ist
|
||||
|
||||
|
||||
@@ -8,7 +8,8 @@
|
||||
"kp": 0.4,
|
||||
"ki": 0.0,
|
||||
"kd": 0.0,
|
||||
"kt": 0.0
|
||||
"kt": 0.0,
|
||||
"y_hold_min": -0.1
|
||||
},
|
||||
"Inner": {
|
||||
"Heat": {
|
||||
|
||||
+2
-1
@@ -8,7 +8,8 @@
|
||||
"kp": 0.4,
|
||||
"ki": 0.0,
|
||||
"kd": 0.0,
|
||||
"kt": 0.0
|
||||
"kt": 0.0,
|
||||
"y_hold_min": -0.1
|
||||
},
|
||||
"Inner": {
|
||||
"Heat": {
|
||||
|
||||
Binary file not shown.
|
After Width: | Height: | Size: 48 KiB |
@@ -247,6 +247,60 @@ small (~0.07 in testing, well below the pre-fix disturbance's ~0.74 peak)
|
||||
step in `y` rather than the fully bumpless transfer described above. Not
|
||||
addressed here — flagged for awareness, not a blocker.
|
||||
|
||||
## Follow-up: steady-state overshoot in HOLD (`Outer.y_hold_min`)
|
||||
|
||||
Found while testing the rework against a real Sud run (`sude/sud_0030.json`,
|
||||
`logs/log_20260706T074658_Sud-0030.json`). Visible in `docs/overshoot2.png`
|
||||
(`theta_ist` vs `theta_soll` across the run): at both the 55°C and 63°C
|
||||
rests, `theta_ist` overshoots the step and then plateaus above `theta_soll`
|
||||
for the rest of the hold instead of converging back down — most clearly at
|
||||
the first rest, where it settles at ~55.5-55.6°C against a 55.0°C target.
|
||||
|
||||
A grain-fill-in disturbance during "1. Rast" (mash-in rest, `HOLD` at 55°C)
|
||||
pushed `temp_ist` to a ~0.4°C overshoot — well under `HoldCool`'s 1.0
|
||||
threshold, so the FSM stayed in `HOLD` throughout, same as the transient
|
||||
windup case above. But this overshoot never decayed: `temp_ist` sat in a
|
||||
55.30-55.44 band for the rest of the 20-minute hold instead of converging
|
||||
back to 55.0. Distinct failure mode from the transient windup fixed above
|
||||
(that one unwound over ~35s; this one was flat/permanent for as long as the
|
||||
hold lasted).
|
||||
|
||||
Root cause: `process_pid()`'s HOLD-state floor on `pid_outer_y` (added in
|
||||
`bb5af3c` to break a limit cycle - see the History section) clamped to
|
||||
exactly `0.0`. Once `temp_ist > temp_soll`, that floor forces
|
||||
`heatrate_soll = 0`, i.e. "hold flat" - and `pid_inner` then actively fights
|
||||
the pot's own ambient heat loss to keep the *overshot* temperature flat,
|
||||
rather than being allowed to request a genuine decline back toward
|
||||
setpoint. With `Outer.ki = 0`, there's no integral action to null the
|
||||
resulting steady-state error any other way, so the offset persists for the
|
||||
rest of the hold.
|
||||
|
||||
Fix: replaced the hardcoded `0.0` floor with a configurable
|
||||
`Outer.y_hold_min` (default `0.0`, so configs that don't set it keep the old
|
||||
behavior), set to `-0.1` in `config.json`, both `.tpl` templates, and the
|
||||
three demo scripts. A small negative floor lets HOLD ask for a gentle
|
||||
decline that roughly matches
|
||||
passive ambient cooling, rather than the fully unclamped `[-1, 1]` range
|
||||
that caused the original limit cycle (a large negative `heatrate_soll` asks
|
||||
for a decline steeper than passive loss can deliver, pinning power at 0 for
|
||||
an extended stretch and producing a hard undershoot/rebound). Test coverage
|
||||
added: `TestHoldOvershootRecoversToSetpoint` in
|
||||
`tests/components/pid/test_temp_controller_closed_loop.py` reproduces the
|
||||
sud_0030 disturbance, asserts the old flat-clamp behavior still fails to
|
||||
recover (regression guard) and the new floor converges close to setpoint,
|
||||
plus a guard that the recovery doesn't undershoot by more than the injected
|
||||
disturbance itself (i.e. doesn't reintroduce the `bb5af3c` limit cycle).
|
||||
|
||||
`-0.1` was chosen from the real Sud's plant params (`Pot.mass=5.96` +
|
||||
`water_mass=22` ≈ the test harness's `M=27.96`, `L=0.2`): passive ambient
|
||||
loss at a ~35°C delta works out to roughly 0.1-0.15 K/min, so
|
||||
`heatrate_soll_set * -0.1` lands in that same ballpark for a typical
|
||||
`heatrate_soll_set` of ~1.0-1.5 K/min. Not derived from first-principles
|
||||
tuning - may need adjustment per installation, same as the other PID gains.
|
||||
|
||||
Confirmed against a live re-run of `sud_0030` with the fix applied, not just
|
||||
the unit test above.
|
||||
|
||||
## Architecture diagram
|
||||
|
||||
A full signal-flow diagram of the cascade (`Outer`/`Inner.*` PIDs, FSM
|
||||
|
||||
@@ -10,7 +10,8 @@ if __name__ == '__main__':
|
||||
"kp": 0.4,
|
||||
"ki": 0.0,
|
||||
"kd": 0.0,
|
||||
"kt": 0.0
|
||||
"kt": 0.0,
|
||||
"y_hold_min": -0.1
|
||||
},
|
||||
"Inner": {
|
||||
"Heat": {
|
||||
|
||||
@@ -10,7 +10,8 @@ if __name__ == '__main__':
|
||||
"kp": 0.4,
|
||||
"ki": 0.0,
|
||||
"kd": 0.0,
|
||||
"kt": 0.0
|
||||
"kt": 0.0,
|
||||
"y_hold_min": -0.1
|
||||
},
|
||||
"Inner": {
|
||||
"Heat": {
|
||||
|
||||
@@ -21,7 +21,8 @@ if __name__ == '__main__':
|
||||
"kp": 0.4,
|
||||
"ki": 0.0,
|
||||
"kd": 0.0,
|
||||
"kt": 0.0
|
||||
"kt": 0.0,
|
||||
"y_hold_min": -0.1
|
||||
},
|
||||
"Inner": {
|
||||
"Heat": {
|
||||
|
||||
@@ -0,0 +1,119 @@
|
||||
{
|
||||
"Name": "Sud-0030",
|
||||
"Description": "Münchner Hell",
|
||||
"pot": {
|
||||
"grain_mass": 0,
|
||||
"water_mass": 22,
|
||||
"volumen": 30
|
||||
},
|
||||
"default": {
|
||||
"step": {
|
||||
"descr": "Put description here",
|
||||
"user_message": "Put user message here",
|
||||
"user_wait_for_continue": false,
|
||||
"pot": {
|
||||
"grain_mass": 5.21,
|
||||
"water_mass": 22
|
||||
},
|
||||
"temperature": 0,
|
||||
"ramp": {
|
||||
"rate": 1.0,
|
||||
"stirrer": {
|
||||
"speed": 50,
|
||||
"interval_time": 0,
|
||||
"on_ratio": 1.0
|
||||
}
|
||||
},
|
||||
"hold": {
|
||||
"duration": 0,
|
||||
"stirrer": {
|
||||
"speed": 30,
|
||||
"interval_time": 60,
|
||||
"on_ratio": 0.5
|
||||
}
|
||||
}
|
||||
}
|
||||
},
|
||||
"steps": [
|
||||
{
|
||||
"pot": {
|
||||
"grain_mass": 0
|
||||
},
|
||||
"descr": "Aufheizen",
|
||||
"temperature": 57,
|
||||
"ramp": {
|
||||
"rate": 1.5,
|
||||
"stirrer": {
|
||||
"speed": 75
|
||||
}
|
||||
}
|
||||
},
|
||||
{
|
||||
"pot": {
|
||||
"grain_mass": 0
|
||||
},
|
||||
"descr": "Einmaischen",
|
||||
"user_message": "Bitte Malz einfüllen und bestätigen",
|
||||
"user_wait_for_continue": true,
|
||||
"hold": {
|
||||
"stirrer": {
|
||||
"speed": 0
|
||||
}
|
||||
}
|
||||
},
|
||||
{
|
||||
"descr": "1. Rast",
|
||||
"temperature": 55,
|
||||
"hold": {
|
||||
"duration": 20
|
||||
}
|
||||
},
|
||||
{
|
||||
"descr": "Glucose-Rast",
|
||||
"temperature": 63,
|
||||
"ramp": {
|
||||
"rate": 1.0
|
||||
},
|
||||
"hold": {
|
||||
"duration": 40,
|
||||
"stirrer": {
|
||||
"speed": 20,
|
||||
"interval_time": 90,
|
||||
"on_ratio": 0.8
|
||||
}
|
||||
}
|
||||
},
|
||||
{
|
||||
"descr": "Verzuckerungs-Rast",
|
||||
"temperature": 72,
|
||||
"ramp": {
|
||||
"rate": 1.0
|
||||
},
|
||||
"hold": {
|
||||
"duration": 30,
|
||||
"stirrer": {
|
||||
"speed": 35,
|
||||
"interval_time": 120
|
||||
}
|
||||
}
|
||||
},
|
||||
{
|
||||
"pot": {
|
||||
"water_mass": 20
|
||||
},
|
||||
"descr": "Abmaischen",
|
||||
"user_message": "Quittieren zum Abmaischen",
|
||||
"user_wait_for_continue": true,
|
||||
"temperature": 76,
|
||||
"ramp": {
|
||||
"rate": 1.0
|
||||
},
|
||||
"hold": {
|
||||
"duration": 30,
|
||||
"stirrer": {
|
||||
"speed": 50
|
||||
}
|
||||
}
|
||||
}
|
||||
]
|
||||
}
|
||||
@@ -10,16 +10,17 @@ AMBIENT = 20.0
|
||||
PLANT_PARAMS = {"M": 27.96, "C": 3403.43, "L": 0.2, "Td": 17}
|
||||
|
||||
OUTER_PARAMS = {"kp": 0.6, "ki": 0.0, "kd": 0.0, "kt": 0.0}
|
||||
OUTER_PARAMS_HOLD_COOL = {"kp": 0.6, "ki": 0.0, "kd": 0.0, "kt": 0.0, "y_hold_min": -0.1}
|
||||
INNER_HEAT_PARAMS = {"kp": 0.08, "ki": 0.02, "kd": 0.0, "kt": 1.5}
|
||||
INNER_HOLD_CLAMPED = {"kp": 0.08, "ki": 0.02, "kd": 0.0, "kt": 1.5, "yi_max": 0.3}
|
||||
INNER_HOLD_UNCLAMPED = dict(INNER_HEAT_PARAMS)
|
||||
|
||||
|
||||
def make_controller(inner_hold_params):
|
||||
def make_controller(inner_hold_params, outer_params=None):
|
||||
ctrl = TempController(DT)
|
||||
ctrl.set_params({
|
||||
"beta": 0.9,
|
||||
"Outer": dict(OUTER_PARAMS),
|
||||
"Outer": dict(outer_params if outer_params is not None else OUTER_PARAMS),
|
||||
"Inner": {
|
||||
"Heat": dict(INNER_HEAT_PARAMS),
|
||||
"Hold": dict(inner_hold_params),
|
||||
@@ -88,6 +89,56 @@ class TestHoldDisturbanceOvershoot(unittest.TestCase):
|
||||
self.assertLess(overshoot_clamped, overshoot_unclamped)
|
||||
|
||||
|
||||
class TestHoldOvershootRecoversToSetpoint(unittest.TestCase):
|
||||
"""Reproduces the sud_0030 grain-fill-in incident: a HOLD overshoot
|
||||
(post-recovery temp_ist above temp_soll, well under HoldCool's
|
||||
threshold so the FSM never leaves HOLD) must coast back down to
|
||||
setpoint rather than sitting in a permanent flat-clamp dead zone -
|
||||
see docs/overshoot_hold_windup.md and the Outer.y_hold_min floor in
|
||||
temp_controller_base.py's process_pid()."""
|
||||
|
||||
def _run_overshoot(self, outer_params):
|
||||
ctrl = make_controller(INNER_HOLD_CLAMPED, outer_params)
|
||||
plant = make_plant(30.0)
|
||||
|
||||
for _ in range(60):
|
||||
tick(ctrl, plant, 30.0, 1.0)
|
||||
self.assertEqual(ctrl.state, States.HOLD)
|
||||
|
||||
# Overshoot above setpoint, small enough to stay inside HOLD
|
||||
# (matches the ~0.3-0.4 degC band observed in the real trace).
|
||||
plant.temp += 0.4
|
||||
|
||||
min_temp = plant.get_temperature()
|
||||
for _ in range(900):
|
||||
tick(ctrl, plant, 30.0, 1.0)
|
||||
self.assertEqual(ctrl.state, States.HOLD)
|
||||
min_temp = min(min_temp, plant.get_temperature())
|
||||
|
||||
return plant.get_temperature(), min_temp
|
||||
|
||||
def test_negative_floor_recovers_flat_clamp_does_not(self):
|
||||
final_flat, _ = self._run_overshoot(OUTER_PARAMS)
|
||||
final_floored, min_floored = self._run_overshoot(OUTER_PARAMS_HOLD_COOL)
|
||||
|
||||
# Old behaviour (y clamped to exactly 0.0): pid_inner fights the
|
||||
# pot's own ambient loss to hold the overshot temperature flat -
|
||||
# it stays measurably above setpoint within this window.
|
||||
self.assertGreater(final_flat - 30.0, 0.05)
|
||||
|
||||
# New behaviour (small negative floor): the outer loop can ask
|
||||
# for a gentle decline, so the overshoot recovers much closer to
|
||||
# setpoint than the flat clamp manages in the same window.
|
||||
self.assertLess(abs(final_floored - 30.0), 0.15)
|
||||
|
||||
# Guard against reintroducing the violent limit cycle the original
|
||||
# [0, 1] clamp was added to break (bb5af3c): recovery should coast
|
||||
# down smoothly, undershooting by no more than the injected
|
||||
# disturbance itself (0.4 degC) - not the ~1 degC+ swings of a
|
||||
# runaway power on/off cycle.
|
||||
self.assertGreater(min_floored, 30.0 - 0.4)
|
||||
|
||||
|
||||
class TestRealRampStillReachesTarget(unittest.TestCase):
|
||||
"""Guards against reintroducing the rejected flat-clamp regression:
|
||||
Inner.Heat has no yi_max, so a genuine ramp must still be able to
|
||||
|
||||
Reference in New Issue
Block a user