Files
brewpi/components/pid/temp_controller_base.py
jensandClaude Sonnet 5 2d032bf9b2 fix: allow gentle negative HOLD floor to fix steady-state overshoot
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
2026-07-06 08:19:10 +02:00

182 lines
7.8 KiB
Python

from components import APid
from components.pid.temp_controller_fsm import TempControllerFsm, States, DEFAULT_THRESHOLDS
class TempControllerBase(TempControllerFsm, APid):
def __init__(self, dt):
APid.__init__(self)
TempControllerFsm.__init__(self, dt)
self.dt = dt
self.theta_ist_set = 0
self.theta_soll_set = 0
self.heatrate_ist_set = 0
self.heatrate_soll_set = 1.0
self.heatrate_soll = 1.0
self.theta_ist = 0
self.heatrate_ist = 0
# None until set_params() is called - mirrors Pid's own
# params/set_params() (components/pid/pid.py), whose process()
# already relies on the same "None means not configured yet".
self.params = None
self._inner_heat_params = None
self._inner_hold_params = None
self._outer_hold_y_min = 0.0
self.y = -1
# Heat-rate pre-filter state (option A) - None until first process() tick
self.last_theta_ist = None
self.theta_ist_filtered = None
self.beta = 0.05
def set_params(self, params):
self.params = params
self.thresholds = {**DEFAULT_THRESHOLDS, **params.get('Thresholds', {})}
self.beta = params.get('beta', 0.05)
self.pid_outer.set_params(params['Outer'])
# How far into "actively cool" pid_outer is allowed to reach during
# HOLD - see the floor in process_pid() below. Defaults to 0.0
# (old flat-clamp behaviour) so configs that don't set it are
# unaffected.
self._outer_hold_y_min = params['Outer'].get('y_hold_min', 0.0)
self._inner_heat_params = params['Inner']['Heat']
self._inner_hold_params = params['Inner']['Hold']
self.pid_inner_cool.set_params(params['Inner']['Cool'])
def _compute_heatrate(self, theta_ist):
"""Pre-filter theta_ist, then differentiate and low-pass to get heatrate_ist.
Filtering before differentiating (option A) keeps noise that comes in on
theta_ist from being amplified by the derivative — stirrer-induced
temperature fluctuations would otherwise produce K/min rate noise of the
same order as a typical rate_soll, directly corrupting the heat PID error."""
if self.theta_ist_filtered is None:
self.theta_ist_filtered = theta_ist
else:
self.theta_ist_filtered = (1 - self.beta) * self.theta_ist_filtered + self.beta * theta_ist
heatrate = 0.0 if self.last_theta_ist is None else \
(self.theta_ist_filtered - self.last_theta_ist) / self.dt * 60
alpha = 0.1
self.heatrate_ist = (1 - alpha) * self.heatrate_ist + alpha * heatrate
self.last_theta_ist = self.theta_ist_filtered
def _require_params(self):
"""Called by each subclass's process() before doing anything else -
without it, a missing set_params() call would otherwise only
surface as an unhelpful "'NoneType' object is not subscriptable"
buried inside Pid.process() (components/pid/pid.py), once
process_pid() below gets to it."""
if self.params is None:
raise RuntimeError(
"{}.process(): PID gains not set - call set_params() first".format(type(self).__name__))
def is_configured(self):
"""Whether set_params() has been called - lets a caller (e.g.
TcTask's own process loop) check upfront and skip processing
gracefully while not yet configured, rather than relying on
process() raising. Overridden by TempController(Smith) to also
require its internal model's plant params/ambient."""
return self.params is not None
def post_pid(self):
pass
def set_ambient_temperature(self, theta_amb):
pass
def set_model_plant_params(self, model_params):
pass
def set_model_power(self, power):
pass
def set_theta_ist(self, value):
self.theta_ist_set = value
if self.is_startup:
self.is_startup = False
def get_theta_ist(self):
return self.theta_ist
def get_theta_ist_set(self):
"""The raw sensor reading, as last pushed via set_theta_ist() -
unlike get_theta_ist() (self.theta_ist), this is updated the
instant a reading comes in, regardless of whether process() has
ever actually run (e.g. before set_params()/a model-based
controller's plant params have been configured - see SudTask.
send_forecast(), which needs a real "current temperature" at the
moment a Sud is loaded, before this controller may have ticked
even once)."""
return self.theta_ist_set
def set_heatrate_ist(self, value):
self.heatrate_ist_set = value
def get_heatrate_ist(self):
return self.heatrate_ist
def set_theta_soll(self, value):
self.theta_soll_set = value
# Recompute the FSM right away against the new target, rather than
# waiting for the next process() tick - otherwise self.state can
# still read HOLD from the previous target for up to one tick
# after a much-further-away one is pushed, which would make
# is_holding() report "reached" instantly instead of once the gap
# has actually closed.
self.process_fsm(self.theta_soll_set - self.theta_ist)
def get_theta_soll(self):
return self.theta_soll
def get_theta_soll_set(self):
return self.theta_soll_set
def set_heatrate_soll(self, value):
self.heatrate_soll_set = value
def get_heatrate_soll(self):
return self.heatrate_soll
def get_heatrate_soll_set(self):
return self.heatrate_soll_set
def process_pid(self, theta_err, hold_scale=1.0):
self.pid_outer.process(theta_err, -self.theta_ist, hold_scale)
# In HOLD state, floor pid_outer's output at Outer.y_hold_min (<= 0)
# rather than letting it swing all the way to -1: a large negative
# heatrate_soll asks for a decline far steeper than the pot's own
# ambient heat loss can deliver, so pid_inner pins power at 0 for an
# extended stretch, undershoots, and swings back hard - a ~110s
# limit cycle (see git history for this clamp). A small negative
# floor instead lets HOLD ask for a gentle decline that roughly
# matches passive cooling, so a small overshoot coasts back down to
# setpoint instead of sitting in a permanent flat-clamp dead zone
# (the outer loop otherwise commands "stay flat" forever, and
# pid_inner actively fights the pot's own ambient loss to hold that
# flat line - see docs/overshoot_hold_windup.md).
pid_outer_y = self.pid_outer.get_y()
if self.state == States.HOLD:
pid_outer_y = max(self._outer_hold_y_min, pid_outer_y)
self.heatrate_soll = self.heatrate_soll_set * pid_outer_y
heatrate_err = self.heatrate_soll - self.heatrate_ist
# Only the PID actually driving y is advanced - otherwise the
# inactive one (e.g. pid_inner while COOL has pid_inner_cool driving)
# would keep silently integrating against a heatrate_err that
# isn't actually under its control, building a stale windup that
# causes a discontinuity in y the moment it takes back over.
if self.state == States.IDLE:
self.y = 0
elif self.state == States.COOL:
self.pid_inner_cool.process(heatrate_err, -self.heatrate_ist)
self.y = self.pid_inner_cool.get_y()
else:
inner_params = self._inner_heat_params if self.state == States.HEAT else self._inner_hold_params
self.pid_inner.set_params(inner_params)
self.pid_inner.process(heatrate_err, -self.heatrate_ist)
self.y = self.pid_inner.get_y()
self.post_pid()
def get_power(self):
return self.y