brewpi/components/pid/TODO.md

components/pid design backlog

Findings from a design review, refreshed against the current state of master after the Kalman-filter removal and Smith-predictor rewrite (see git history for temp_controller.py/temp_controller_smith.py).

• FSM thresholds aren't configurable. Moved into an optional TempCtrl.Thresholds config section (HoldIdle, HoldHeat, IdleHeat, IdleHold, HeatHold, HeatIdle), merged over DEFAULT_THRESHOLDS (now in temp_controller_base.py, formerly tc_constants.py) so existing configs without the section keep working unchanged.

• matplotlib imported at module level in production code. temp_controller.py, temp_controller_smith.py, and kalman.py used to from matplotlib.pyplot import ... just to support their __main__ self-test plots. The plotting demos (and kalman_eval.py) moved to scripts/demos/pid/demo_*.py; production modules no longer import matplotlib.

• Three Kalman filters share one tuning. No longer applicable: temp_controller_smith.py was rewritten to drop Kalman filtering entirely (see below), so there's no shared tuning to split anymore.

• Pid.scale() is a gain-scheduling hack, not anti-windup. Fixed: removed Pid.scale()/self.k entirely. Pid.process(err, d, scale=1.0) now takes the gain multiplier as a plain per-call argument instead of mutable state, so there's nothing to go stale across a reset(). temp_controller*.py compute hold_scale = 1.0/heatrate_soll_set if heatrate_soll_set > 0 else 1.0 themselves and pass it through process_pid(theta_err, heatrate_err, hold_scale) — the overshoot-compensation scheduling logic now lives entirely in the temp controller, not the generic Pid block.

• No automated tests for the heat-rate filtering or Smith correction specifically. An earlier tests/components/pid/ suite (stdlib unittest, no pytest) covering FSM transitions, anti-windup clamping, and Kalman convergence was removed before the Kalman-filter removal/Smith rewrite. A new tests/components/pid/ suite was added alongside the HOLD-windup fix below (test_pid.py, test_temp_controller_closed_loop.py) covering the yi_max clamp and closed-loop disturbance/ramp/transition behavior, but _compute_heatrate()'s backward-difference + low-pass filtering and the two-model Smith correction itself still have no dedicated coverage. This drives a physical heater — worth extending.

• set_model_power isn't defined on every controller, but brewpi.py wires it unconditionally. brewpi.py always does heater.set_on_changed("power_set", tc.set_model_power) regardless of Controller.pid_type. Only temp_controller_smith.py (the Smith predictor) defines set_model_power; temp_controller.py ("Normal") does not, so starting the server with "pid_type": "Normal" crashes at wiring time with AttributeError: 'TempController' object has no attribute 'set_model_power'. Either add a no-op set_model_power to TempControllerBase, or only wire it when the configured controller actually exposes a model to feed.

• Config access is unchecked dict[key] everywhere. params['Hold'], params['Td'], etc., throughout, with no schema validation at load time. We already hit this bug class once (config.json.sim's gain/Model nesting mismatch). A small schema/dataclass validation layer at config-load would surface errors immediately instead of mid-__init__ — and would have caught the dead TempCtrl.Kalman / Model.kn / Plant.kn keys that used to sit unused in config-sim.json.tpl/.sim (since deleted), and the Model.gain/Plant.gain keys that did the same in config.json.sim before that file was removed entirely — Pot dropped gain entirely, see components/plant/TODO.md.

• pid_heat can wind up during a HOLD-state disturbance with no anti-windup engagement. A cold-water disturbance while holding drove pid_heat's integral term up without ever saturating y (peaked at y≈0.74 of the 1.0 ceiling), so the existing back-calculation anti-windup (Pid.process(), pid.py:45-48) never triggered — the FSM never even left HOLD (diff stayed under HoldHeat=1.0). The resulting overshoot took ~35s+ to unwind naturally. A flat yi_max 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. An FSM-gating alternative (freeze the loop's output in HOLD unless engaged) was also superseded. Fixed: renamed 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 docs/fsm_states.png for a static screenshot of the FSM-states panel of the cascade architecture diagram (states/thresholds, and the HOLD→HEAT no-reset note). The full interactive diagram (signal-flow cascade + FSM inset + config-mapping table) is a Claude Artifact, not a repo file: https://claude.ai/code/artifact/a32e4752-b4a6-4153-b344-eb2423eb6512 — only reachable by whoever has access to the Claude account/session that created it, not a durable link for the team; docs/fsm_states.png is the durable copy. This was also a breaking config change (Hold/Heat/Cool → Outer/Inner.*) — config.json, the templates, and the demo scripts were all migrated. Test coverage (closed-loop disturbance/ramp/transition cases) was added under tests/components/pid/ — see the "No automated tests" item above.

• 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.

• Architecture isn't ready for a real active cooler yet, only a heat-only actuator. Asked (not yet built): would the cascade/FSM design still hold up if an active cooler (compressor/chiller, some finite cooling power) replaced "heater off + passive ambient loss"? Mostly yes at the PID/plant-model level: Pid.y_min/y_max are already symmetric -1.0/1.0 (pid.py:12-13), Pot.set_power()/ process() are sign-agnostic so the simulator already models negative power correctly, and the FSM already has a dedicated COOL state with its own PID instance (pid_inner_cool, temp_controller_fsm.py:35-39) whose comment explicitly anticipates this ("the actuator ... is responsible for clamping the resulting negative power to whatever it's actually capable of"). The gap is at the actuator boundary: today y only ever reaches one consumer, tasks/heater.py:59's self.power_actor = max(0, ... * y), which silently discards every negative command — that's what's made tuning mismatches free so far. A real cooler needs (a) a second actuator/device class consuming the negative half instead of that max(0, ...) clamp — there's no Cooler/chiller abstraction anywhere yet (grep -rniE "cooler|chiller|compressor" across all .py is empty, fully greenfield), and heater_hendi.py:80's set_power only clamps to a max, not a min, so feeding it a negative value today would call setPowerWatts(negative) on real hardware with undefined result; and (b) its own power ceiling, since one normalized y scaled by one get_power_max() breaks once heater and cooler have different max power. Biggest risk once that clamp is removed: two separate paths start issuing real negative-power commands that were previously harmless - HOLD's Outer.y_hold_min floor (via Inner.Hold, see the steady-state overshoot item above) and the pre-existing pid_inner_cool (via Inner.Cool) - and neither has been tuned against actual active-cooling dynamics (compressor lag, min-on-time). Inner.Cool in the demo configs is literally a copy of Inner.Heat's gains, never validated independently. So adding a real cooler is a wiring task (new actuator class, per-actuator power scaling) plus a re-tuning task (Inner.Cool, and re-checking the HoldCool/CoolHold thresholds in temp_controller_fsm.py:14-17), not just wiring.

• 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 scripts/demos/pid/demo_kalman.py and demo_kalman_eval.py. Decide whether to keep it as a documented standalone filtering example or delete it along with the demos.