jensandClaude Sonnet 5
13bb011099
Records the root cause of the cold-water overshoot in docs/overshoot.png (integral windup in pid_heat with no anti-windup engagement, since y never saturated) and the refined fix plan: gate pid_heat by FSM state instead of a flat yi_max clamp, which was rejected after the numbers showed it would also cripple legitimate high-rate ramps. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KefnwbGDM8CGrhb4sFhVq9
88 lines
4.9 KiB
Markdown
88 lines
4.9 KiB
Markdown
# 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`).
|
|
|
|
- [x] **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.
|
|
|
|
- [x] **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.
|
|
|
|
- [x] **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.
|
|
|
|
- [x] **`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.** `tests/components/pid/` was added once
|
|
(stdlib `unittest`, no pytest) covering FSM transitions, anti-windup
|
|
clamping, and Kalman convergence, but was removed again before the
|
|
Kalman-filter removal/Smith rewrite, so none of the current
|
|
`temp_controller*.py` behavior (backward-difference + low-pass
|
|
filtered heat rate, the two-model Smith correction) has test
|
|
coverage. This drives a physical heater — worth re-adding.
|
|
|
|
- [ ] **`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. 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.
|
|
|
|
- [ ] **`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.
|