Add design-flaw backlog for components/plant
Review of pot.py found a likely root cause for sim-vs-real control mismatches: the heat-loss term carries the wrong units, masking ambient cooling in simulation. Also notes the dropped sensor-noise term, hardcoded ambient temp, and lag-vs-dead-time modeling gap. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,51 @@
|
||||
# components/plant design backlog
|
||||
|
||||
Findings from reviewing `pot.py`'s water-bath model against real-plant
|
||||
behavior (sim/real control mismatch reported by user). Not yet acted on.
|
||||
|
||||
- [ ] **Heat-loss term has wrong units.** `pot.py:37`:
|
||||
`leak = 1/self.M * self.L * (self.theta_amb - self.temp)/self.theta_amb`.
|
||||
The power term right above it is `power/(self.M * self.C)` (W /
|
||||
(J/K) -> K/s). `leak` should follow the same pattern —
|
||||
`self.L * (self.theta_amb - self.temp) / (self.M * self.C)` — but
|
||||
instead it's missing `/C` and divides by `theta_amb` (~20) instead,
|
||||
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
|
||||
`params['kn']` but nothing in `process()`/`get_temperature()` uses
|
||||
it. Git history shows it used to add Gaussian sensor noise
|
||||
(`self.kn * np.random.normal(...)`) to the output, since commented
|
||||
out and then dropped during the heat-diffusion refactor. Sim is
|
||||
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`
|
||||
passes a literal `20` instead of reading it from config or a live
|
||||
ambient sensor. Real ambient temperature drifts and is never
|
||||
modeled as a disturbance, so the controller's robustness to it is
|
||||
untested.
|
||||
|
||||
- [ ] **Thermal lag modeled as single-pole lag, not transport delay.**
|
||||
`HeatDiffusion` (`heat_diffusion.py`) implements `alpha=dt/Td`
|
||||
exponential smoothing, not true dead time. There's already a
|
||||
`Delay` class (`plant/delay.py`) for pure transport delay, dropped
|
||||
in favor of `HeatDiffusion` in commit `16cb3d3`. If the real pot's
|
||||
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
|
||||
enforced inside `Pot` itself (handled ad hoc in test harnesses), no
|
||||
evaporation/lid heat loss, no varying thermal mass `M` as volume
|
||||
changes (e.g. boil-off, adding water/grain) — `M` is a fixed
|
||||
constant for the whole simulation.
|
||||
Reference in New Issue
Block a user