diff --git a/components/pid/TODO.md b/components/pid/TODO.md index a4db2cf..a3e43e8 100644 --- a/components/pid/TODO.md +++ b/components/pid/TODO.md @@ -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` diff --git a/docs/overshoot_hold_windup.md b/docs/overshoot_hold_windup.md index 4c8d257..ed04bbb 100644 --- a/docs/overshoot_hold_windup.md +++ b/docs/overshoot_hold_windup.md @@ -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