brewpi/docs/overshoot_hold_windup.md

# HOLD-state overshoot from a disturbance: root cause and fix plan

Investigation of the overshoot visible in `docs/overshoot.png`: a cold-water
disturbance during a steady HOLD at 30°C caused `theta_ist` to overshoot to
~30.5°C and stay there, rather than settling back at soll. This document
records the diagnosis and the (not yet implemented) fix plan. Numbers below
are pulled from `logs/log_latest.json`.

## Incident summary

- Steady HOLD at 30.0°C. Cold water added → `theta_ist` (Smith-corrected)
  dipped to ~29.49°C (`diff ≈ 0.51°C`).
- `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".
- 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 `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`).
- Plant dead time (`Td=17s`) and thermal mass (`M=27.96 kg`,
  `C≈3403 J/(kg·K)`, from `PlantParams` in the log) meant heat from that
  burst kept arriving after `theta_ist` had already crossed back over soll.
  By the time `rate_soll` had fallen back to exactly `0` (outer loop asking
  for zero heat rate), `power_set` was still **1442 W**, decaying only
  gradually over the next ~35s (1193 → 979 → 817 → 632 → 487 → 337 → 142 W)
  rather than snapping to 0.
- No active cooling actuator exists; the passive loss coefficient
  (`L=0.2 W/(kg·K)`) gives a time constant `C/L ≈ 17,000s (~4.7h)`, so the
  resulting ~0.4°C plateau above soll does not visibly decay within any
  reasonable observation window.

**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
`y` peaked at ~0.74, well under the `1.0` ceiling, `awu` was `0` the entire
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: a flat `yi_max` clamp on the inner loop

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 |
|---|---|---|
| 1.5 K/min ramp @ 30°C | 2435 W | 0.696 |
| 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, 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.**

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.

## Refined plan: split the clamp by *which state is driving the same loop*,
## not by freezing the loop

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.

### Rename for honesty

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.

### Config: `Hold`/`Heat`/`Cool` → `Outer` / `Inner.{Heat,Hold,Cool}`

```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 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 `Outer`/`Inner.*` gains. Run closed-loop (controller's own `y`
drives the plant, unlike `utils/replay_sim.py`'s open-loop observe-only
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

Implemented: `pid.py`'s `yi_max` clamp, the `pid_outer`/`pid_inner`/
`pid_inner_cool` rename, the `Outer`/`Inner.{Heat,Hold,Cool}` config
restructure (`config.json`, both `.tpl` templates, and the three demo
scripts), and `utils/replay_sim.py`'s matching CLI-flag/print-loop rename.
Tests added under `tests/components/pid/` (`test_pid.py` for the isolated
`Pid` clamp behavior, `test_temp_controller_closed_loop.py` for the
closed-loop disturbance/ramp/transition cases) — all passing.

One subtlety found while writing the closed-loop transition test that
this plan didn't anticipate: `Inner.Hold`'s `yi_max` clamp applies
retroactively. If a sustained `HEAT` ramp pushes `yi` above the `Hold`
ceiling before the `HeatHold` threshold fires, the very next tick after
the `HEAT→HOLD` transition clamps `yi` back down immediately, producing a
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.