Testing the windup-fix rework live against sude/sud_0030.json surfaced a
distinct bug: a HOLD overshoot from grain-fill-in cooling never decayed -
process_pid()'s pid_outer_y floor clamped to exactly 0.0, so pid_inner
fought the pot's own ambient loss to hold the overshot temperature flat
instead of declining back to setpoint (see docs/overshoot2.png).
Replace the hardcoded 0.0 floor with a configurable Outer.y_hold_min
(default 0.0, backward compatible), set to -0.1 in config.json, both
.tpl templates, and the demo scripts - small enough to avoid
reintroducing the bb5af3c limit cycle while letting HOLD request a
gentle decline matching passive ambient cooling.
Adds TestHoldOvershootRecoversToSetpoint and documents the finding in
docs/overshoot_hold_windup.md's Follow-up section and
components/pid/TODO.md. Confirmed against a live sud_0030 re-run, not
just the unit test.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M2ierBoxW3v7nUbDw3M2pE
Renames pid_hold/pid_heat/pid_cool to pid_outer/pid_inner/pid_inner_cool
to match what actually runs when, and splits inner-loop config into
Inner.Heat/Inner.Hold/Inner.Cool so the same PID instance gets a tight
yi_max ceiling only while HOLD drives it, without capping legitimate
1.5 K/min ramps. Fixes the overshoot from docs/overshoot_hold_windup.md
where a cold-water disturbance during HOLD wound up pid_heat's integral
term with no anti-windup engagement, taking ~35s+ to unwind naturally.
Breaking config change: Hold/Heat/Cool -> Outer/Inner.{Heat,Hold,Cool}
in config.json, both .tpl templates, the pid/sud demo scripts, and
replay_sim.py's CLI flags. Adds tests/components/pid/ (stdlib unittest)
covering the Pid clamp/recovery behavior and closed-loop disturbance,
ramp, and HOLD<->HEAT transition cases.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DGQhVQ2Y3yXAQTXhrxVd5u
During hold, a small temperature overshoot causes pid_hold.y to go
slightly negative, inverting heatrate_soll and driving pid_heat's
power to 0. With no power the pot coasts back down, pid_hold goes
positive again, power builds back up, overshoot repeats — a ~110s
limit cycle visible as pumping in the power trace.
Clamping pid_hold.y to [0, 1] in HOLD state lets the outer loop
reduce the inner rate setpoint to zero (stop adding heat) but not
invert it (actively demand cooling), breaking the limit cycle while
preserving normal HEAT/COOL cascade behaviour.
Note: Hold.kt must remain 0 when Hold.ki=0 — a non-zero kt drains
pid_hold.yi during ramp saturation, suppressing heatrate_soll at the
start of each hold phase and leaking into the next ramp via the
bumpless HOLD→HEAT transfer.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Two root causes for the visible power dip at every ramp-to-ramp
step boundary:
1. One-tick cascade delay: heatrate_soll was computed from
pid_hold.get_y() before pid_hold.process() ran in that tick,
so the first tick of a new ramp used the stale hold-phase
output (≈0) as the rate setpoint, driving pid_heat to near
zero. Fix: move heatrate_soll computation inside process_pid(),
after pid_hold.process().
2. Cold-start reset: pid_heat.reset() at HOLD→HEAT discarded
the hold-phase integral, so pid_heat had to ramp up from
zero on every new step. Fix: remove the reset (bumpless
transfer) — the hold-phase integral is a warm starting point
that carries the baseline hold power into the new ramp.
Together these eliminate the ~1s dip to near-zero and replace
the slow 20–30s integral ramp-up with an immediate start at
roughly hold-level power, climbing monotonically to the target.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds a configurable IIR pre-filter (beta, default 0.05, τ≈19 s at dt=1 s)
applied to theta_ist before the derivative, so stirrer-induced temperature
noise is attenuated before being amplified by differentiation rather than
after. The shared filter/derivative/post-filter logic is factored into
TempControllerBase._compute_heatrate(); both Normal and Smith subclasses
call it, replacing their duplicated inline rate blocks.
beta is read from TempCtrl.beta in the config (default 0.05 if absent) and
added to config-sim.json.tpl and config-real.json.tpl.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
With TempControllerBase(APid, TempControllerFsm), APid.is_holding()
(abstract stub returning None) shadowed TempControllerFsm.is_holding()
in the MRO, so tc.is_holding() always returned None. The forecast
simulation never called sud.temp_reached() and always hit MAX_TICKS.
Swapping to TempControllerBase(TempControllerFsm, APid) puts the
concrete FSM methods before APid's abstract stubs in the MRO.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Tqxrk8uj4M3w3d3eXm3xK8
States, DEFAULT_THRESHOLDS, the three PIDs, FSM state vars, process_fsm(),
on_state_entered(), set_enabled(), and is_holding() move to a new
TempControllerFsm class in temp_controller_fsm.py. TempControllerBase
inherits from both APid and TempControllerFsm; States is re-exported so
existing imports in temp_controller_smith.py are unaffected.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Tqxrk8uj4M3w3d3eXm3xK8
Same pattern as set_model_power: these methods only existed on the Smith
controller, forcing five hasattr guards at call sites. No-op stubs on the
base class give all controllers a stable interface; Smith overrides them.
Remove the now-redundant hasattr guards.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Tqxrk8uj4M3w3d3eXm3xK8
The Normal controller had no set_model_power(), causing callers to
guard with hasattr() instead of relying on a stable interface. Adding a
no-op stub to the base class means all controllers have the method;
TempControllerSmith overrides it to actually update its internal model.
Remove the now-redundant hasattr guards at both call sites.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Tqxrk8uj4M3w3d3eXm3xK8
Re-enabling the temp controller landed unconditionally in HOLD and
relied on the next tick to correct it, but SudTask.on_process() reads
is_holding() synchronously in the same call chain that pushes a fresh
step's setpoint - so a restart could finish a hold-less step instantly
instead of actually ramping. Resolve the FSM against the real gap right
away instead. Also fix the status bar showing one step number below
what the Progress tab's plates show for the same step.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0189FkmyJkY77HqsNomr1DwV
send_forecast() was defaulting to a cold start at ambient (SudForecastEstimator.
estimate()'s default when start_theta isn't passed), so the forecast's t=0
never matched whatever temperature the pot actually was at - understating
how long the first ramp would really take whenever it wasn't already cold.
Anchoring at tc.get_theta_ist() seemed like the obvious fix but isn't
reliable right after Load: that's only ever updated inside process(), which
a model-based controller (Smith) doesn't run until its plant params are
configured - which now only happens once a Sud is loaded (see the
"inert until loaded" change) - so theta_ist can still be sitting at its
never-updated __init__ default of 0 the instant send_forecast() reads it.
Added TempControllerBase.get_theta_ist_set() (mirroring the existing
get_theta_soll_set()) - the raw sensor reading, updated the instant a
reading comes in regardless of whether process() has ever run - and used
that instead.
That still leaves a gap between Load and Start: the real temperature can
drift (time passing, manual heating via the Manual tab) between loading a
schedule and actually starting it, leaving the forecast anchored to a
now-stale temperature. recv()'s 'Start' handler now re-sends the forecast,
freshly anchored to the real temperature at that moment, on every fresh
start (not a Pause->resume, which keeps the already-established forecast
rather than discarding it mid-run).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DkkuG48uHFCGKe6dPSERFk
server/brewpi.py no longer pre-configures the real Pot's or Smith's model's
plant params at startup - there's no longer any generic baseline at all,
since the only source of truth is now a loaded Sud's own doc (applied via
tasks/sud.py's SudTask.apply_plant_params(), on Load and on every step).
Ambient temperature and PID gains stay configured at startup, since
they're independent of any Sud (global setting / hardware tuning, not
doc-derived).
Add Pot.is_configured() (already existed)/TempControllerBase.is_configured()/
TempController(Smith).is_configured(), and have tasks/pot.py's PotTask and
tasks/tempctrl.py's TcTask skip process() while not yet configured instead
of letting it raise - so plant and temp control are genuinely inert (not
crashing) until a Sud is loaded. "Normal" has no plant-model dependency,
so it stays active regardless.
Also refreshes README.md: removes stale tracer.py/.mat references (already
removed in an earlier commit), documents SudLogTask's JSON logs, the doc's
L/Td fields, and this inert-until-loaded behavior.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DkkuG48uHFCGKe6dPSERFk
TempController(Smith).__init__ no longer takes model_params/theta_amb -
set_model_plant_params() and the existing set_ambient_temperature() must be
called instead (mirrors components/plant/pot.py's own Pot constructor
refactor). temp_controller.py's now-pointless model_params/theta_amb
constructor placeholders (kept only for "compatibility" with Smith) are
dropped too.
Both TempController variants now raise a clear RuntimeError from process()
itself if set_params() wasn't called, instead of letting it surface deep
inside Pid.process() as an opaque "'NoneType' object is not subscriptable".
Smith additionally checks its internal models via Pot.is_configured() (new)
and raises if set_model_plant_params()/set_ambient_temperature() weren't
called either.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DkkuG48uHFCGKe6dPSERFk
Mirrors components/pid/pid.py's own Pid.set_params() pattern: params/
thresholds start out None, and set_params() configures pid_hold/pid_heat/
pid_cool from them. process()/process_fsm() fail naturally (TypeError on
None) if called before set_params() - same as Pid.process() already does -
rather than a new bespoke check. Updates all callers (server/brewpi.py,
SudForecastEstimator, and the pid/sud demo scripts) to call set_params()
right after construction.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DkkuG48uHFCGKe6dPSERFk
Previously, whether a schedule step ramped at all was decided purely by
the presence of a 'ramp' key (components/sud.py's _advance()) - a
hold-only step jumped straight into its hold countdown, assuming the
plant was already at temperature. Now every step ramps toward
'temperature' first, for as long as the controller's own gap-tracking
FSM (TempControllerBase, already distinguishing HEAT/COOL/HOLD) says
the gap actually warrants it - via the new is_holding() (on APid and
TempControllerBase), which replaces a separate, redundant
TEMP_REACHED_TOLERANCE constant duplicated across tasks/sud.py,
components/sud_forecast.py, and scripts/demos/sud/demo_sud.py.
set_theta_soll() now recomputes the FSM eagerly so is_holding() can't
read stale HOLD for a tick after a much-further-away target is pushed.
components/sud.py's _build_step() always synthesizes a 'ramp' block
from default.step.ramp (so 'rate' is always available), but leaves
'temperature' undefaulted - every real sude/*.json's
default.step.temperature is inert template filler, and defaulting it
would send hold-only steps chasing 0 degrees. SudTask/
SudForecastEstimator/the demo only push a new theta_soll/heatrate_soll
when a step actually specifies its own temperature; otherwise the
controller keeps whatever the previous step left running.
Also fixes a related crash this surfaced: SudTask.remaining_schedule()'s
synthetic mid-hold step dropped 'ramp' to signal "don't re-ramp" - now
that every step always carries a 'ramp' dict, dropping it left 'rate'
missing the moment the synthetic step was re-resolved by
SudForecastEstimator. Drops 'temperature' instead, which is what
actually signals "no new target" under the new model.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LhiQe64F74uHV8jzuoSa5K
HoldCool=0.1 was inherited from the pre-COOL-state era, where it meant
"eagerly give up active holding and coast" - harmless, since coasting
had no side effects. COOL is a real, actively-controlled state now
(its own pid_cool, reset on every entry), so a threshold this tight
relative to a real heater's discrete power steps (or even just sensor
noise) caused the system to flip in and out of COOL nearly every tick
once resting near a setpoint - resetting both pid_hold's and
pid_cool's integrators each time and never letting either actually
converge.
Confirmed via a full multi-task simulation (real Pot/Sensor/Heater/
TempCtrl tasks, not a simplified loop) of sude/sud_0010.json: 44+
TempCtrl state changes and the run not settling within hours at
HoldCool=0.1, dropping to 8 state changes and a clean 186.5 min finish
at HoldCool=1.0 (matching the other thresholds). This was the dominant
cause of real brews taking far longer than their nominal schedule
duration - not just a forecasting inaccuracy, an actual control-
quality bug.
process_pid() was advancing pid_rate and pid_cool unconditionally on
every tick regardless of which one actually drove y. While COOL had
pid_cool driving, pid_rate kept silently integrating against the same
(very negative) heatrate_err with no way to act on it, building a
large stale windup. The moment the FSM returned to HOLD (which only
reset pid_hold), y read straight from that saturated pid_rate instead
of a fresh value, so the heater stayed off well past the target
before the windup finally unwound - an undershoot of about 1 degree
in scripts/demos/pid/demo_temp_controller.py's last (cooling) step.
Now only the PID currently driving y is process()'d each tick, and
pid_rate is also reset on IDLE->HOLD and COOL->HOLD (it was already
reset on the transitions into HEAT/COOL). Undershoot drops to ~0.06
degrees, in line with normal PID settling.
TempControllerBase.cooling/set_cooling() forced y=0 in advance of a
Sud ramp-down step; redundant now that the FSM's own COOL state
detects and handles a negative diff itself. Removed from the
controller, SudTask's anticipatory detection (and the Step message's
now-unused "Cooling" field), and demo_sud.py's mirror of it.
gui: the "Cool down" status-bar indicator now reflects the
controller's actual State ("States.COOL") via the TempCtrl channel,
rather than Sud's removed anticipatory flag - renamed
sud_cooling/set_sud_cooling to tc_cooling/set_tc_cooling to match.
IDLE conflated two unrelated things: "controller disabled" and "needs
to cool, which this heat-only actuator fakes via zero output." Split
them apart:
- IDLE now means disabled only - the FSM forces it whenever
enabled=False, regardless of the temperature gap, and process_pid()
zeroes y for it same as before.
- New COOL state (mirroring HEAT) takes over the negative-diff case,
with its own pid_cool (separate "Cool" gains, alongside Hold/Heat)
instead of reusing pid_rate. Its output can legitimately be negative
- it's the actuator (tasks/heater.py's actor(), already max(0, ...))
that clamps it to 0 because *this* plant (Pot) can only heat. A
plant with real cooling capability could one day honor it directly.
- Thresholds renamed accordingly (HoldIdle->HoldCool, HeatIdle->
HeatCool, new CoolHold/CoolHeat); config.json/.sim/.templ and the
hand-rolled ctrl_params in scripts/demos/pid/ and demo_sud.py
updated with a "Cool" params section (mirrors "Heat" for now, since
there's no real cooling actuator to tune against yet).
Also fixes a regression in demo_sud.py/the other PID demos: none of
them ever called set_enabled(True), so since enabled defaults to
False they never drove the heater at all - only caught because this
change's demo_sud.py re-run got stuck in RAMPING forever.
New TempControllerBase.enabled (default False) forces y=0 in
process_pid() when off, same mechanism as the existing cooling
override - the controller tries not to drive the heater at all.
tasks/tempctrl.py: new 'Enable' recv command and 'Enabled' broadcast
on the TempCtrl channel. tasks/sud.py: SudTask now enables the
controller on any non-IDLE/DONE Sud state and disables it (forcing
power back to 0) on IDLE/DONE, so automatic mode owns it for the
duration of a run and hands back manual control once it stops/finishes.
gui: new "Enabled" checkbox on the Manual tab's Controller group,
synced from the server; the temp/heatrate setpoint controls are
disabled whenever the controller itself is disabled.
A ramp step whose target is below the current actual temperature can
only be reached by ambient cooling - the heater can't actively cool.
SudTask now detects this once per ramp phase and calls the controller's
new set_cooling() override (forces y=0, bypassing the FSM) instead of
waiting for its hysteresis-based IDLE transition. The GUI shows a
blinking blue "Cool down" label in the status bar while this is active.
Progression already only advances once theta_ist matches theta_soll
regardless of direction, so no change was needed there.
Pid.scale(k) set self.k as persistent state, mutated from outside the
class and never reset in reset(), so a stale scale factor could survive
a state-transition reset. Replace it with a plain scale=1.0 argument on
Pid.process(), and have temp_controller.py/temp_controller_smith.py
compute the heat-rate-overshoot compensation factor themselves and pass
it through process_pid() each tick instead of mutating pid_hold's state.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
tc_constants.py only held States and DEFAULT_THRESHOLDS, both used
exclusively by temp_controller_base.py and its subclasses; fold them
into temp_controller_base.py directly and drop the now-empty module.
Also drop heat_diffusion.py, unused since pot.py switched to the
delay-line model, and the matching dead import in pot.py.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
THRESH_HOLD_IDLE/HOLD_HEAT/IDLE_HEAT/IDLE_HOLD/HEAT_HOLD/HEAT_IDLE in
tc_constants.py were hardcoded module-level constants shared by every
controller instance, so tuning the IDLE/HOLD/HEAT hysteresis required
a code change.
Replace them with DEFAULT_THRESHOLDS plus an optional
TempCtrl.Thresholds config section, merged at construction time in
TempControllerBase.__init__ so configs that omit the section keep
behaving exactly as before. Documented the new section in
config.json.templ; left config.json.sim without it to exercise the
default-fallback path.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
TempControllerBase no longer threads model_params through (only the
Smith subclass needs it, for its own Pot model and Kalman filters).
Enable the Smith predictor's actual delay-compensated error term
(theta_err now uses theta_ist_plant - theta_ist_model_delay +
theta_ist_model instead of the plain plant reading), which is the
correction this controller is named for.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Both controllers had nearly identical process_fsm(), process_pid(),
and all getters/setters; only Kalman/model setup and process()
genuinely differed, and the duplication had already drifted (the
Smith variant resets model state on entering HEAT, the plain one
didn't).
Add TempControllerBase with the shared logic and three small hooks
(init_kalman, on_state_entered, post_pid) subclasses use to plug in
their own Kalman/model behavior. Each subclass now contains only what
makes it different.
Also fixes a latent crash: TempController's constructor only accepted
(dt, params), but PidFactory/brewpi.py always call it with a third
model_params arg, so pid_type "Normal" would have raised TypeError.
The shared base's model_params=None default fixes this. Also
initializes the Smith controller's trace attributes in __init__
instead of leaving them undefined until the first process() call.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jensandClaude Sonnet 5