Refresh README and design-backlog docs against current code
Both TODO.md backlogs and the README's architecture section still described the Kalman-filter/heat-diffusion design that's since been replaced by the delay-line Pot model and Kalman-free Smith predictor. Check off the items that rewrite already fixed (heat-loss units, transport delay vs. single-pole lag, three-Kalman-tuning), note how the kn/sensor-noise item was resolved differently than proposed, and add newly-spotted issues: brewpi.py wiring set_model_power unconditionally (breaks pid_type "Normal"), kalman.py being dead code in production, stale Kalman/kn keys in the config templates, and debug print()s left in Pot.process(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -6,8 +6,8 @@ rate) according to a configurable mash schedule, drives a heater and stirrer,
|
|||||||
and exposes live control/telemetry over a WebSocket so a desktop GUI (or any
|
and exposes live control/telemetry over a WebSocket so a desktop GUI (or any
|
||||||
other client) can monitor and steer the brew.
|
other client) can monitor and steer the brew.
|
||||||
|
|
||||||
The project began as a simulation/control-theory playground (Kalman filter +
|
The project began as a simulation/control-theory playground (Smith-predictor
|
||||||
Smith-predictor temperature control, pot heat-diffusion model, see
|
temperature control, pot transport-delay model, see
|
||||||
[`docs/NonLinMPC.pdf`](docs/NonLinMPC.pdf)) and has grown real-hardware
|
[`docs/NonLinMPC.pdf`](docs/NonLinMPC.pdf)) and has grown real-hardware
|
||||||
backends for an induction hob and an RTD temperature probe.
|
backends for an induction hob and an RTD temperature probe.
|
||||||
|
|
||||||
@@ -26,10 +26,13 @@ client/brewpi_gui.py PyQt5 desktop client (brewpi.ui) that connects
|
|||||||
and switch the heater/stirrer on or off.
|
and switch the heater/stirrer on or off.
|
||||||
|
|
||||||
components/ Pluggable building blocks behind factories:
|
components/ Pluggable building blocks behind factories:
|
||||||
pid/ temperature controllers (PID, Smith
|
pid/ temperature controllers: plain PID
|
||||||
predictor + Kalman filter) and the math
|
("Normal") or PID + Smith predictor
|
||||||
model used for prediction
|
("Smith", runs two internal pot models —
|
||||||
plant/ pot heat-diffusion model used in simulation
|
one with the plant's transport delay, one
|
||||||
|
without — to compensate for the dead time)
|
||||||
|
plant/ pot thermal model (transport-delay line)
|
||||||
|
used in simulation
|
||||||
sensor/ temperature sensors: simulated, or a real
|
sensor/ temperature sensors: simulated, or a real
|
||||||
MAX31865 RTD amplifier over SPI
|
MAX31865 RTD amplifier over SPI
|
||||||
actor/ heater and stirrer drivers: simulated, a
|
actor/ heater and stirrer drivers: simulated, a
|
||||||
|
|||||||
+50
-28
@@ -1,15 +1,26 @@
|
|||||||
# components/pid design backlog
|
# components/pid design backlog
|
||||||
|
|
||||||
Findings from a design review, not yet acted on. See git history on
|
Findings from a design review, refreshed against the current state of
|
||||||
`claude_refactor` for items already fixed (Smith predictor's delay
|
`master` after the Kalman-filter removal and Smith-predictor rewrite (see
|
||||||
correction enabled, the two TempController classes deduplicated into
|
git history for `temp_controller.py`/`temp_controller_smith.py`).
|
||||||
`temp_controller_base.py`).
|
|
||||||
|
|
||||||
- [x] **FSM thresholds aren't configurable.** Moved into an optional
|
- [x] **FSM thresholds aren't configurable.** Moved into an optional
|
||||||
`TempCtrl.Thresholds` config section (`HoldIdle`, `HoldHeat`,
|
`TempCtrl.Thresholds` config section (`HoldIdle`, `HoldHeat`,
|
||||||
`IdleHeat`, `IdleHold`, `HeatHold`, `HeatIdle`), merged over
|
`IdleHeat`, `IdleHold`, `HeatHold`, `HeatIdle`), merged over
|
||||||
`DEFAULT_THRESHOLDS` in `tc_constants.py` so existing configs without
|
`DEFAULT_THRESHOLDS` (now in `temp_controller_base.py`, formerly
|
||||||
the section keep working unchanged.
|
`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.
|
||||||
|
|
||||||
- [ ] **`Pid.scale()` is a gain-scheduling hack, not anti-windup.**
|
- [ ] **`Pid.scale()` is a gain-scheduling hack, not anti-windup.**
|
||||||
`pid.py`'s `scale(k)` multiplies every gain by `1/heatrate_soll_set`
|
`pid.py`'s `scale(k)` multiplies every gain by `1/heatrate_soll_set`
|
||||||
@@ -18,30 +29,41 @@ correction enabled, the two TempController classes deduplicated into
|
|||||||
a few lines below. `self.k` also never resets in `reset()`, so a
|
a few lines below. `self.k` also never resets in `reset()`, so a
|
||||||
stale scale factor can survive a state-transition reset. Consider
|
stale scale factor can survive a state-transition reset. Consider
|
||||||
moving this scheduling logic out of the generic `Pid` class and into
|
moving this scheduling logic out of the generic `Pid` class and into
|
||||||
the temp controller, and resetting `k` in `reset()`.
|
the temp controller, and resetting `k` in `reset()`. Still present.
|
||||||
|
|
||||||
- [ ] **Three Kalman filters share one tuning.** In
|
- [ ] **No automated tests.** `tests/components/pid/` was added once
|
||||||
`temp_controller_smith.py`, `kalman_model`, `kalman_model_delay`,
|
(stdlib `unittest`, no pytest) covering FSM transitions, anti-windup
|
||||||
and `kalman_plant` are all constructed with the same
|
clamping, and Kalman convergence, but was removed again before the
|
||||||
`params['Kalman']` despite tracking signals with different noise
|
Kalman-filter removal/Smith rewrite, so none of the current
|
||||||
characteristics. Allow independent `var_P`/`var_Q`/`var_R` per
|
`temp_controller*.py` behavior (backward-difference + low-pass
|
||||||
filter in config.
|
filtered heat rate, the two-model Smith correction) has test
|
||||||
|
coverage. This drives a physical heater — worth re-adding.
|
||||||
|
|
||||||
- [ ] **matplotlib imported at module level in production code.**
|
- [ ] **`set_model_power` isn't defined on every controller, but
|
||||||
`temp_controller.py`, `temp_controller_smith.py`, and `kalman.py`
|
`brewpi.py` wires it unconditionally.** `brewpi.py` always does
|
||||||
all `from matplotlib.pyplot import ...` just to support their
|
`heater.set_on_changed("power_set", tc.set_model_power)` regardless
|
||||||
`__main__` self-test plots, which couples the live server's import
|
of `Controller.pid_type`. Only `temp_controller_smith.py` (the Smith
|
||||||
graph (and its dependency list) to a GUI plotting library it never
|
predictor) defines `set_model_power`; `temp_controller.py` (`"Normal"`)
|
||||||
uses at runtime. Move the plotting demos into separate scripts.
|
does not, so starting the server with `"pid_type": "Normal"` crashes
|
||||||
|
at wiring time with `AttributeError: 'TempController' object has no
|
||||||
- [ ] **No automated tests.** The `__main__` blocks (and
|
attribute 'set_model_power'`. Either add a no-op `set_model_power` to
|
||||||
`kalman_eval.py`) are manual, eyeballed-plot harnesses. Add
|
`TempControllerBase`, or only wire it when the configured controller
|
||||||
regression tests for FSM transitions, anti-windup clamping, and
|
actually exposes a model to feed.
|
||||||
Kalman convergence — this code drives a physical heater.
|
|
||||||
|
|
||||||
- [ ] **Config access is unchecked `dict[key]` everywhere.**
|
- [ ] **Config access is unchecked `dict[key]` everywhere.**
|
||||||
`params['Kalman']`, `model_params['gain']`, etc., throughout, with
|
`params['Hold']`, `model_params['gain']`, etc., throughout, with no
|
||||||
no schema validation at load time. We already hit this bug class
|
schema validation at load time. We already hit this bug class once
|
||||||
once (`config.json.sim`'s `gain`/`Model` nesting mismatch). A small
|
(`config.json.sim`'s `gain`/`Model` nesting mismatch). A small
|
||||||
schema/dataclass validation layer at config-load would surface
|
schema/dataclass validation layer at config-load would surface
|
||||||
errors immediately instead of mid-`__init__`.
|
errors immediately instead of mid-`__init__` — and would have caught
|
||||||
|
the dead `TempCtrl.Kalman` / `Model.kn` / `Plant.kn` keys still
|
||||||
|
sitting in `config.json.templ` and `config.json.sim` (harmless today
|
||||||
|
since extra keys are just ignored, but nothing reads them anymore
|
||||||
|
after the Kalman/`kn` removal — see `components/plant/TODO.md`).
|
||||||
|
|
||||||
|
- [ ] **`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.
|
||||||
|
|||||||
+40
-42
@@ -1,51 +1,49 @@
|
|||||||
# components/plant design backlog
|
# components/plant design backlog
|
||||||
|
|
||||||
Findings from reviewing `pot.py`'s water-bath model against real-plant
|
Findings from reviewing `pot.py`'s water-bath model against real-plant
|
||||||
behavior (sim/real control mismatch reported by user). Not yet acted on.
|
behavior (sim/real control mismatch reported by user), refreshed against
|
||||||
|
the current `delay`-line rewrite of `Pot`.
|
||||||
|
|
||||||
- [ ] **Heat-loss term has wrong units.** `pot.py:37`:
|
- [x] **Heat-loss term has wrong units.** Fixed by the `Pot` rewrite:
|
||||||
`leak = 1/self.M * self.L * (self.theta_amb - self.temp)/self.theta_amb`.
|
`process()` now computes `p_loss = self.L * self.M * (self.temp -
|
||||||
The power term right above it is `power/(self.M * self.C)` (W /
|
self.theta_amb)` in Watts and applies `self.temp += (self.delay.get()
|
||||||
(J/K) -> K/s). `leak` should follow the same pattern —
|
- p_loss) / (self.M * self.C) * self.dt` — the same `/(M*C)`
|
||||||
`self.L * (self.theta_amb - self.temp) / (self.M * self.C)` — but
|
normalization for both the gain and loss terms, instead of the old
|
||||||
instead it's missing `/C` and divides by `theta_amb` (~20) instead,
|
mismatched `leak = 1/self.M * self.L * (...)/ self.theta_amb`.
|
||||||
which isn't a physically meaningful normalization. With
|
|
||||||
`C=4190` this suppresses ambient heat loss by ~3-4 orders of
|
|
||||||
magnitude versus what the power term implies, so the simulated pot
|
|
||||||
barely cools at all. The Smith predictor's internal model is also a
|
|
||||||
`Pot` instance with the identical bug, so this never showed up in
|
|
||||||
sim-vs-sim testing — only against the real plant. Likely the
|
|
||||||
primary cause of the sim/real control mismatch.
|
|
||||||
|
|
||||||
- [ ] **`kn` is loaded but never applied.** `pot.py:18` reads
|
- [x] **Thermal lag modeled as single-pole lag, not transport delay.**
|
||||||
`params['kn']` but nothing in `process()`/`get_temperature()` uses
|
Fixed: `Pot` no longer uses `HeatDiffusion`'s `alpha=dt/Td`
|
||||||
it. Git history shows it used to add Gaussian sensor noise
|
exponential smoothing (that module was deleted). It now delays the
|
||||||
(`self.kn * np.random.normal(...)`) to the output, since commented
|
input power through `plant/delay.py`'s `Delay` (true dead time)
|
||||||
out and then dropped during the heat-diffusion refactor. Sim is
|
before integrating it, matching what this item recommended.
|
||||||
currently fully deterministic/noise-free, making the Kalman filter
|
|
||||||
and PID derivative term look better-behaved in sim than they will
|
|
||||||
against a real noisy sensor. Re-enable as measurement noise (and
|
|
||||||
decide if process noise is also wanted).
|
|
||||||
|
|
||||||
- [ ] **`theta_amb` is hardcoded, not configured.** `brewpi.py:40`
|
- [x] **`kn` is loaded but never applied.** Resolved differently than
|
||||||
passes a literal `20` instead of reading it from config or a live
|
originally proposed: `kn` was removed from `Pot` entirely rather than
|
||||||
ambient sensor. Real ambient temperature drifts and is never
|
wired up as plant-side measurement noise. Sensor noise is now
|
||||||
modeled as a disturbance, so the controller's robustness to it is
|
modeled one layer up, in `TempSensorSim` (`temp_offset`/`variance`,
|
||||||
untested.
|
wired from `brewpi.py`'s `TempSensorFactory.create(..., temp_offset=
|
||||||
|
-0.15, variance=0.01)`), not in the plant model. Note: `config.json.templ`
|
||||||
|
and `config.json.sim` still carry stale `Model.kn`/`Plant.kn` keys
|
||||||
|
that nothing reads anymore — harmless but worth deleting (see
|
||||||
|
`components/pid/TODO.md`'s config-validation item).
|
||||||
|
|
||||||
- [ ] **Thermal lag modeled as single-pole lag, not transport delay.**
|
- [ ] **`theta_amb` is hardcoded, not configured.** Still true —
|
||||||
`HeatDiffusion` (`heat_diffusion.py`) implements `alpha=dt/Td`
|
`brewpi.py`: `pot = Pot(DT, pot_params, 20)` passes a literal `20`
|
||||||
exponential smoothing, not true dead time. There's already a
|
instead of reading it from config or a live ambient sensor. Real
|
||||||
`Delay` class (`plant/delay.py`) for pure transport delay, dropped
|
ambient temperature drifts and is never modeled as a disturbance, so
|
||||||
in favor of `HeatDiffusion` in commit `16cb3d3`. If the real pot's
|
the controller's robustness to it is untested.
|
||||||
heater-to-sensor coupling behaves more like dead time (heat must
|
|
||||||
propagate through the water before reaching the sensor) than a
|
|
||||||
single lag, `Td` won't represent the same dynamic in sim vs. reality,
|
|
||||||
and the Smith predictor's delay compensation (which assumes `Td`
|
|
||||||
matches the real plant) will be off.
|
|
||||||
|
|
||||||
- [ ] **No actuator/process realism.** No heater power saturation
|
- [ ] **No actuator/process realism.** Partially improved: `process()`
|
||||||
enforced inside `Pot` itself (handled ad hoc in test harnesses), no
|
now clamps `self.temp` to `min(100, ...)` (won't simulate past
|
||||||
evaporation/lid heat loss, no varying thermal mass `M` as volume
|
boiling), but there's still no heater power saturation enforced
|
||||||
|
inside `Pot` itself (handled ad hoc in demo harnesses), no
|
||||||
|
evaporation/lid heat loss, and no varying thermal mass `M` as volume
|
||||||
changes (e.g. boil-off, adding water/grain) — `M` is a fixed
|
changes (e.g. boil-off, adding water/grain) — `M` is a fixed
|
||||||
constant for the whole simulation.
|
constant for the whole simulation.
|
||||||
|
|
||||||
|
- [ ] **Debug `print()`s left in `Pot.process()`.** Every call prints
|
||||||
|
`p_loss`, `theta_amb`, and `temp` unconditionally — this runs once
|
||||||
|
per simulated/real tick (e.g. every `Controller.dt`), spamming stdout
|
||||||
|
in both the server and every demo script. Looks like leftover
|
||||||
|
debugging from the heat-loss-units fix; remove or gate behind a
|
||||||
|
debug flag.
|
||||||
|
|||||||
Reference in New Issue
Block a user