docs: revise pid_heat windup plan to pid_outer/pid_inner rename + Inner.Hold split
Supersedes the earlier flat yi_max clamp / FSM-gating ideas: same inner PID instance switches its active param set (Inner.Heat vs Inner.Hold) by state instead of freezing, giving bumpless transfer for free and a tight yi_max that only applies while HOLD is driving it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KefnwbGDM8CGrhb4sFhVq9
This commit is contained in:
Claude Sonnet 5
+12
-5
@@ -73,11 +73,18 @@ git history for `temp_controller.py`/`temp_controller_smith.py`).
|
||||
clamp was considered and rejected: sustaining a genuine 1.5 K/min ramp
|
||||
needs `y≈0.7-0.75` from `yi` alone at steady state, the same range the
|
||||
disturbance itself peaked at, so no single clamp value can suppress the
|
||||
windup without also capping legitimate ramps. Plan is instead to gate
|
||||
`pid_heat`'s activity by FSM state (frozen/offline while `HOLD`, engaged
|
||||
by command or a new `HoldHeatEngage` threshold) — see
|
||||
`docs/overshoot_hold_windup.md` for the full writeup, open design
|
||||
question (threshold relationship with `HoldHeat`), and test plan.
|
||||
windup without also capping legitimate ramps. An FSM-gating alternative
|
||||
(freeze the loop's output in `HOLD` unless engaged) was also superseded.
|
||||
Current plan: rename `pid_hold`/`pid_heat`/`pid_cool` to `pid_outer`/
|
||||
`pid_inner`/`pid_inner_cool` (matching what actually runs when), and
|
||||
split the inner loop's config into `Inner.Heat`/`Inner.Hold`/
|
||||
`Inner.Cool` so the *same* PID instance gets a tight `yi_max` only while
|
||||
`HOLD` is driving it and stays unclamped for real `HEAT` ramps — no
|
||||
freeze/thaw, bumpless transfer preserved for free. See
|
||||
`docs/overshoot_hold_windup.md` for the full writeup and test plan. This
|
||||
is also a breaking config change (`Hold`/`Heat`/`Cool` → `Outer`/
|
||||
`Inner.*`) — every deployed `config.json` needs migrating, not just the
|
||||
repo templates.
|
||||
|
||||
- [ ] **`kalman.py` is now dead code in production.** Neither
|
||||
`temp_controller.py` nor `temp_controller_smith.py` uses `Kalman`
|
||||
|
||||
+148
-71
@@ -13,12 +13,12 @@ are pulled from `logs/log_latest.json`.
|
||||
- `diff` never reached `Thresholds.HoldHeat` (1.0°C, `temp_controller_fsm.py`),
|
||||
so **the FSM never left `HOLD`** — the entire episode happened inside what
|
||||
the system still considered "holding".
|
||||
- `pid_hold` (outer loop, `Hold: {kp:0.6, ki:0, kd:0, kt:0}` — pure
|
||||
- The outer loop (`pid_hold`, `Hold: {kp:0.6, ki:0, kd:0, kt:0}` — pure
|
||||
proportional) tracked `0.6 * max(0, diff)` exactly, tick for tick, peaking
|
||||
at `pid_hold_y ≈ 0.31` — nowhere near its `y_max=1.0` ceiling. Confirmed
|
||||
directly against logged samples: `rate_soll` == `0.6*diff` to the last
|
||||
digit throughout.
|
||||
- `pid_heat` (inner loop, `Heat: {kp:0.08, ki:0.02, kd:0, kt:1.5}`) then
|
||||
at `y ≈ 0.31` — nowhere near its `y_max=1.0` ceiling. Confirmed directly
|
||||
against logged samples: `rate_soll` == `0.6*diff` to the last digit
|
||||
throughout.
|
||||
- The inner loop (`pid_heat`, `Heat: {kp:0.08, ki:0.02, kd:0, kt:1.5}`) then
|
||||
chased that `heatrate_soll` target, driving commanded power from a
|
||||
baseline ~500W burst-cycle up to **~2600W** (`y ≈ 0.74` of the 3500W max,
|
||||
see `tasks/heater.py:58-59`: `power = get_power_max() * y`).
|
||||
@@ -34,8 +34,8 @@ are pulled from `logs/log_latest.json`.
|
||||
resulting ~0.4°C plateau above soll does not visibly decay within any
|
||||
reasonable observation window.
|
||||
|
||||
**Root cause:** `pid_heat`'s integral term (`yi`) accumulated during the
|
||||
~130s the outer loop demanded a real (if modest) heat rate, and nothing
|
||||
**Root cause:** the inner loop's integral term (`yi`) accumulated during
|
||||
the ~130s the outer loop demanded a real (if modest) heat rate, and nothing
|
||||
bounded that accumulation — the existing anti-windup in `Pid.process()`
|
||||
(`components/pid/pid.py:45-48`) is back-calculation that only corrects `yi`
|
||||
when the combined output `y` is actually clamped at `y_min`/`y_max`. Since
|
||||
@@ -44,12 +44,11 @@ time and the anti-windup mechanism never engaged. The integrator just had to
|
||||
unwind naturally against real (delayed) negative error, which took ~35+
|
||||
seconds after the outer loop had already zeroed its target.
|
||||
|
||||
## Rejected approach: flat `yi_max` clamp on `pid_heat`
|
||||
## Rejected approach: a flat `yi_max` clamp on the inner loop
|
||||
|
||||
The first fix considered was bounding `yi` directly
|
||||
(`self.yi = max(-yi_max, min(yi_max, self.yi))` in `Pid.process()`,
|
||||
independent of the existing `y`-clamp anti-windup). Numeric check against
|
||||
the real plant model kills this as a standalone fix:
|
||||
The first fix considered was bounding `yi` directly, with one constant for
|
||||
the inner loop regardless of state. Numeric check against the real plant
|
||||
model kills this as a standalone fix:
|
||||
|
||||
| scenario | P needed | y needed |
|
||||
|---|---|---|
|
||||
@@ -57,91 +56,169 @@ the real plant model kills this as a standalone fix:
|
||||
| 1.5 K/min ramp @ 50°C | 2547 W | 0.728 |
|
||||
| 1.5 K/min ramp @ 66°C | 2636 W | 0.753 |
|
||||
| the actual HOLD disturbance (peak) | 2600 W | 0.74 |
|
||||
| steady HOLD loss compensation @ 66°C | ~257 W | ~0.073 |
|
||||
|
||||
At steady state (`err=0`), `kp*err` contributes nothing — the entire
|
||||
`y≈0.7-0.75` needed to sustain a genuine 1.5 K/min ramp has to come from
|
||||
`yi` alone, for as long as the ramp lasts. That's the *same* range the
|
||||
disturbance transient itself peaked at. So:
|
||||
disturbance transient itself peaked at, but far above what real steady-HOLD
|
||||
loss compensation ever needs (~0.07-0.15 at realistic brew temperatures). A
|
||||
single constant can't serve both: tight enough to matter for the disturbance
|
||||
(well under ~0.74) permanently starves a real ramp of the ~0.7 it needs;
|
||||
loose enough not to interfere with ramps (~0.75+) never engages during the
|
||||
disturbance at all. **Rejected as a single global constant.**
|
||||
|
||||
- `yi_max` set low enough to meaningfully shorten the observed hangover
|
||||
(well under ~0.74, e.g. ~0.3) would permanently cap a real 1.5 K/min ramp
|
||||
far below its commanded rate — not a transient dip, a persistent,
|
||||
uncorrectable shortfall for the entire ramp.
|
||||
- `yi_max` set high enough not to interfere with legitimate ramps (~0.75+)
|
||||
sits at or above what the incident already peaked at, so it never
|
||||
engages and does nothing for the hangover.
|
||||
An FSM-gating alternative (freeze the inner loop's output to 0 while in
|
||||
`HOLD` unless explicitly engaged by command or a new threshold) was also
|
||||
sketched, but left an open question about how a new engage threshold should
|
||||
relate to the existing `HoldHeat` FSM threshold, and needed extra
|
||||
engage/disengage hysteresis to avoid chatter. **Superseded** by the plan
|
||||
below, which reaches the same effect with less new machinery.
|
||||
|
||||
A flat magnitude clamp cannot distinguish "leftover integral from an
|
||||
already-resolved disturbance" from "integral correctly holding up a real
|
||||
ongoing ramp" — in this plant they occupy the same output range. **Rejected.**
|
||||
## Refined plan: split the clamp by *which state is driving the same loop*,
|
||||
## not by freezing the loop
|
||||
|
||||
## Refined plan: gate `pid_heat` by FSM state instead
|
||||
The insight above — real HOLD-time demand (~0.07-0.15) and real ramp demand
|
||||
(~0.7-0.75) occupy clearly different ranges — means a **per-state parameter
|
||||
set** on the *same* PID instance solves this more cleanly than gating the
|
||||
loop on/off: same accumulated `yi`/`d` state carried across `HOLD↔HEAT`
|
||||
transitions (bumpless transfer for free, no explicit reset, no engage/
|
||||
disengage hysteresis to tune), just a different `yi_max` ceiling depending
|
||||
on which state is currently active.
|
||||
|
||||
Bound (or freeze) the *rate* at which `pid_heat` reacts based on FSM state,
|
||||
not a constant on `yi`, since real sustained-rate demand only ever happens
|
||||
in `HEAT`; in `HOLD` any large `yi` is by definition windup, since the
|
||||
target rate there is always ~0 outside of a real disturbance.
|
||||
### Rename for honesty
|
||||
|
||||
1. **New flag `heat_loop_active`**, scoped to `HOLD` only, in
|
||||
`TempControllerFsm`. `is_holding()` stays `state == HOLD` regardless —
|
||||
Sud step "have we reached target" logic is unaffected.
|
||||
2. **While `heat_loop_active` is False in `HOLD`:** force `y=0`, skip
|
||||
`pid_heat.process()` entirely (freeze it, don't reset — same
|
||||
bumpless-resume convention already used for `HOLD→HEAT`, see
|
||||
`temp_controller_fsm.py:99-101`). `pid_hold` keeps running every tick
|
||||
regardless (it's pure-P, no windup risk, and its output is needed to
|
||||
evaluate the engage condition below).
|
||||
3. **Engagement (`False→True`)**, checked each tick while in `HOLD`:
|
||||
- Explicit command — a new `engage_heat_loop()`/`force_heat()` call,
|
||||
wired to a manual UI action and/or auto-fired from `set_theta_soll()`
|
||||
when the new target is a genuine change (not the same value re-sent
|
||||
every tick).
|
||||
- `diff >= Thresholds.HoldHeatEngage` (new config key).
|
||||
4. **Disengagement:** once `diff` settles back inside the existing
|
||||
`HoldHeat`/`HoldCool` band, held for a short dwell time (a few seconds),
|
||||
to avoid chatter right at the boundary.
|
||||
5. **Config/back-compat:** new threshold merges into `DEFAULT_THRESHOLDS`
|
||||
the same way existing ones do (`temp_controller_base.py:30`).
|
||||
The current names don't match what actually runs when:
|
||||
- `pid_hold` already runs unconditionally *every* tick regardless of state
|
||||
(`process_pid()`'s first line, `temp_controller_base.py:134`) — it's the
|
||||
outer loop, not "the HOLD-state PID". Rename to **`pid_outer`**.
|
||||
- `pid_heat` already runs in *both* `HOLD` and `HEAT` today (only `IDLE`/
|
||||
`COOL` skip it, `temp_controller_base.py:151-158`) — it's the inner loop
|
||||
for the heating direction. Rename to **`pid_inner`**.
|
||||
- `pid_cool` only ever runs in `COOL` — no `HOLD`-time ambiguity, since a
|
||||
disturbance that pushes temp *above* soll during `HOLD` is still handled
|
||||
by `pid_inner` (the outer loop's `max(0.0, pid_hold_y)` floor sends
|
||||
`heatrate_soll` to 0, and `pid_inner` reacts to the resulting negative
|
||||
`heatrate_err` — the state only escalates to real `COOL` past
|
||||
`Thresholds.HoldCool`). Rename to **`pid_inner_cool`** for symmetry, kept
|
||||
as its own instance — today's explicit `reset()` calls when crossing
|
||||
between heat-direction and cool-direction states
|
||||
(`temp_controller_fsm.py:104,108,115,123`) stay exactly as they are; there
|
||||
is no reason to share integrator state across a heater/chiller boundary.
|
||||
|
||||
### Open design question (unresolved)
|
||||
### Config: `Hold`/`Heat`/`Cool` → `Outer` / `Inner.{Heat,Hold,Cool}`
|
||||
|
||||
`HoldHeatEngage` needs to sit **below** the existing `HoldHeat`/`HeatHold`
|
||||
FSM threshold (currently `1.0°C`) to ever fire from inside `HOLD` — if
|
||||
`HoldHeat` stays at `1.0`, the FSM fully escalates to `HEAT` (where
|
||||
`pid_heat` always runs anyway) before any larger engage threshold is ever
|
||||
reached. Two ways to resolve, not yet decided:
|
||||
- Raise `HoldHeat`/`HeatHold` too, widening the HOLD↔HEAT band so `HOLD`
|
||||
covers the full deadband and the new flag is the only gate within it.
|
||||
- Keep `HoldHeat` at `1.0`, set the engage threshold below it (e.g.
|
||||
~0.7-0.8°C) — loop only goes offline for small disturbances, engaging
|
||||
before the FSM would escalate to full `HEAT` anyway.
|
||||
```json
|
||||
"TempCtrl": {
|
||||
"pid_type": "Smith",
|
||||
"beta": 0.9,
|
||||
"Outer": { "kp": 0.6, "ki": 0.0, "kd": 0.0, "kt": 0.0 },
|
||||
"Inner": {
|
||||
"Heat": { "kp": 0.08, "ki": 0.02, "kd": 0.0, "kt": 1.5 },
|
||||
"Hold": { "kp": 0.08, "ki": 0.02, "kd": 0.0, "kt": 1.5, "yi_max": 0.3 },
|
||||
"Cool": { "kp": 0.08, "ki": 0.02, "kd": 0.0, "kt": 1.5 }
|
||||
},
|
||||
"Thresholds": { "...": "unchanged" }
|
||||
}
|
||||
```
|
||||
- `Inner.Heat` keeps today's `Heat` gains, no `yi_max` (or a very loose one)
|
||||
— a real ramp must be able to reach `y≈0.75`.
|
||||
- `Inner.Hold` starts as a copy of the same gains, with `yi_max≈0.2-0.3`
|
||||
added — comfortably above realistic steady-loss compensation (~0.07-0.15)
|
||||
but well below what turned a 0.5°C dip into a 2600W burst.
|
||||
- `Inner.Cool` is `Cool`'s existing gains, moved under `Inner` purely for
|
||||
structural consistency — introduced now, not because we've observed a
|
||||
cooling-side incident. `pid_inner_cool` never runs during `HOLD`, so it
|
||||
doesn't need its own `Hold` variant the way `Heat` does; one params block
|
||||
is enough.
|
||||
- **This is a breaking config change** — no backward-compat shim for the
|
||||
old flat `Hold`/`Heat`/`Cool` keys (per the "no compat hacks" convention).
|
||||
Every deployed `config.json` needs migrating, not just the repo's
|
||||
`config-real.json.tpl`/`config-sim.json.tpl` templates.
|
||||
|
||||
### Code changes (planned, not yet implemented)
|
||||
|
||||
1. **`components/pid/pid.py`** — add the symmetric `yi_max` clamp inside
|
||||
`process()` (already planned): `self.yi = max(-yi_max, min(yi_max,
|
||||
self.yi))` when `self.params.get('yi_max')` is set, applied right after
|
||||
accumulating `yi` and before it's summed into `y`.
|
||||
2. **`components/pid/temp_controller_fsm.py`** — rename `self.pid_hold` →
|
||||
`self.pid_outer`, `self.pid_heat` → `self.pid_inner`, `self.pid_cool` →
|
||||
`self.pid_inner_cool` (constructor at lines 33-34/40, all `reset()` call
|
||||
sites and comments at lines 11-12, 83, 87-89, 99, 104, 108, 111, 115,
|
||||
118-119, 123).
|
||||
3. **`components/pid/temp_controller_base.py`**:
|
||||
- `set_params()` (line 28-34): `self.pid_outer.set_params(params['Outer'])`;
|
||||
store `self._inner_heat_params = params['Inner']['Heat']` and
|
||||
`self._inner_hold_params = params['Inner']['Hold']` for the per-tick
|
||||
lookup below; `self.pid_inner_cool.set_params(params['Inner']['Cool'])`.
|
||||
- `process_pid()` (lines 133-160): rename `pid_hold_y` → `pid_outer_y`
|
||||
and the `self.pid_hold.process(...)` call. In the combined `HOLD`/`HEAT`
|
||||
branch (today's `else`, lines 156-158), select the active param set
|
||||
before processing:
|
||||
```python
|
||||
else:
|
||||
inner_params = self._inner_heat_params if self.state == States.HEAT else self._inner_hold_params
|
||||
self.pid_inner.set_params(inner_params)
|
||||
self.pid_inner.process(heatrate_err, -self.heatrate_ist)
|
||||
self.y = self.pid_inner.get_y()
|
||||
```
|
||||
`set_params()` is a cheap dict-reference assignment (`pid.py:22-23`),
|
||||
so calling it every tick has no meaningful cost. Because `kp`/`ki`/`kd`/
|
||||
`kt` are identical between `Inner.Heat` and `Inner.Hold` in the starting
|
||||
config, switching the active set at a `HOLD↔HEAT` transition changes
|
||||
no term of `y` at that instant — only the `yi_max` ceiling going
|
||||
forward. No bump at the transition, unlike the freeze/thaw approach.
|
||||
4. **Config files** — `config.json`, `config-real.json.tpl`,
|
||||
`config-sim.json.tpl`: restructure into `Outer`/`Inner.{Heat,Hold,Cool}`
|
||||
as above. Inline `"Cool": {...}` dicts in `scripts/demos/pid/
|
||||
demo_temp_controller_smith.py:21`, `demo_temp_controller.py:21`, and
|
||||
`scripts/demos/sud/demo_sud.py:32` need the same restructure.
|
||||
5. **`utils/replay_sim.py`** — `_apply_gain_overrides()` and the CLI flag
|
||||
loop (lines 50, 214-222) iterate `('Hold','hold'), ('Heat','heat'),
|
||||
('Cool','cool')`; becomes `('Outer','outer'), ('Inner.Heat','inner-heat'),
|
||||
('Inner.Hold','inner-hold'), ('Inner.Cool','inner-cool')` (nested dict
|
||||
access needed since these aren't flat top-level keys any more). The
|
||||
`for section in ('Hold','Heat','Cool')` print loop at line 284 and the
|
||||
`_infer_heatrate_soll_set()` docstring's "hold PID" reference (line 66)
|
||||
need the same rename.
|
||||
6. **`components/pid/TODO.md`** — update the cross-link entry added for the
|
||||
previous (superseded) plan to point at this rewritten section instead.
|
||||
|
||||
## Test plan (not yet implemented)
|
||||
|
||||
Stdlib `unittest`, no pytest — `tests/components/pid/test_pid.py`,
|
||||
discoverable via `python -m unittest discover -t . -s tests/components/pid`.
|
||||
Simpler than the FSM-gating plan's test plan: no engage/disengage hysteresis
|
||||
or threshold-relationship behavior to cover, since the loop is never turned
|
||||
off — only its `yi_max` ceiling changes with state.
|
||||
|
||||
**A. Unit-level, isolated `Pid`** — no plant involved. Feed a synthetic
|
||||
`err` sequence shaped like the incident (positive `heatrate_err ≈ 0.3` held
|
||||
for ~130 ticks, then decaying/negative tail, matching the real
|
||||
`rate_soll - rate_ist` pulled from the log) into `pid_heat` with the loop
|
||||
gated (frozen while "HOLD"/inactive) vs ungated (today's behavior). Assert:
|
||||
recovery time (ticks after error goes negative until `y` drops back under a
|
||||
small threshold) is measurably shorter when gated; `yi` never grows during
|
||||
the frozen window.
|
||||
`rate_soll - rate_ist` pulled from the log) into two `Pid` instances with
|
||||
identical gains, one with `yi_max` set (the `Inner.Hold` case) and one
|
||||
without (`Inner.Heat`). Assert: recovery time (ticks after error goes
|
||||
negative until `y` drops back under a small threshold) is measurably
|
||||
shorter when clamped; `yi` never exceeds the configured bound.
|
||||
|
||||
**B. Closed-loop, self-contained synthetic scenario** — no dependency on
|
||||
the multi-MB log file. Real numbers from `PlantParams`/`config.json`:
|
||||
`Pot(dt)` with `M=27.96, C=3403.43, L=0.2, Td=17`, ambient from
|
||||
`config.json`; `TempController` via `PidFactory.create('Smith', dt)` with
|
||||
the real `Hold`/`Heat`/`Cool` gains. Run closed-loop (controller's own `y`
|
||||
the real `Outer`/`Inner.*` gains. Run closed-loop (controller's own `y`
|
||||
drives the plant, unlike `utils/replay_sim.py`'s open-loop observe-only
|
||||
mode): hold at 30°C until settled, knock `plant.temp` down ~0.5°C to
|
||||
emulate the cold-water event, keep ticking for several minutes. Run twice —
|
||||
gating on vs off — and assert peak overshoot above 30.0°C is measurably
|
||||
smaller with gating, while a separate run driving a genuine 1.5 K/min ramp
|
||||
confirms gating does *not* reduce the sustained ramp rate (guards against
|
||||
reintroducing the flat-clamp regression above).
|
||||
mode):
|
||||
- **Disturbance case**: hold at 30°C until settled, knock `plant.temp` down
|
||||
~0.5°C to emulate the cold-water event, keep ticking for several minutes.
|
||||
Assert peak overshoot above 30.0°C is measurably smaller with `Inner.Hold`'s
|
||||
`yi_max` set than without.
|
||||
- **Ramp case**: command a genuine 1.5 K/min ramp (state reaches `HEAT`) and
|
||||
assert the sustained heat rate actually reaches ~1.5 K/min — guards against
|
||||
reintroducing the flat-clamp regression from the rejected approach above.
|
||||
- **Transition case**: drive a `HOLD→HEAT→HOLD` sequence and assert `y` has
|
||||
no discontinuity at either transition beyond what the changing `heatrate_err`
|
||||
itself would explain — confirms the per-state param swap is truly bumpless.
|
||||
|
||||
## Status
|
||||
|
||||
|
||||
Reference in New Issue
Block a user