From 1e89a583703532854e564174ae729c6833b5fe88 Mon Sep 17 00:00:00 2001 From: Jens Ahrensfeld Date: Fri, 19 Jun 2026 17:37:55 +0200 Subject: [PATCH] Replace Pid.scale()'s mutable gain factor with a stateless process() arg 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 --- components/pid/TODO.md | 17 +++++++++-------- components/pid/pid.py | 15 +++++---------- components/pid/temp_controller.py | 5 ++--- components/pid/temp_controller_base.py | 4 ++-- components/pid/temp_controller_smith.py | 5 ++--- 5 files changed, 20 insertions(+), 26 deletions(-) diff --git a/components/pid/TODO.md b/components/pid/TODO.md index b9f15b9..5a820f4 100644 --- a/components/pid/TODO.md +++ b/components/pid/TODO.md @@ -22,14 +22,15 @@ git history for `temp_controller.py`/`temp_controller_smith.py`). `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.py`'s `scale(k)` multiplies every gain by `1/heatrate_soll_set` - (called from `temp_controller*.py`'s `process()`), but it's framed - as overshoot compensation, conflated with the real anti-windup logic - a few lines below. `self.k` also never resets in `reset()`, so a - stale scale factor can survive a state-transition reset. Consider - moving this scheduling logic out of the generic `Pid` class and into - the temp controller, and resetting `k` in `reset()`. Still present. +- [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 diff --git a/components/pid/pid.py b/components/pid/pid.py index 8cd7b68..b82dfdd 100644 --- a/components/pid/pid.py +++ b/components/pid/pid.py @@ -5,8 +5,6 @@ class Pid: self.dt = dt self.params = None - self.k = 1.0 - # Integrator self.yi = 0 @@ -21,9 +19,6 @@ class Pid: # Output self.y = 0 - def scale(self, k): - self.k = k - def set_params(self, params): self.params = params @@ -32,12 +27,12 @@ class Pid: self.d = 0 self.awu = 0 - def process(self, err, d): + def process(self, err, d, scale=1.0): dt = self.dt - kp = self.params['kp'] * self.k - ki = self.params['ki'] * self.k - kd = self.params['kd'] * self.k - kt = self.params['kt'] * self.k + kp = self.params['kp'] * scale + ki = self.params['ki'] * scale + kd = self.params['kd'] * scale + kt = self.params['kt'] * scale self.yi = self.yi + ki*dt * err + kt*dt * self.awu yd = kd/dt*(d - self.d) diff --git a/components/pid/temp_controller.py b/components/pid/temp_controller.py index 3eda9c5..e0b9a16 100644 --- a/components/pid/temp_controller.py +++ b/components/pid/temp_controller.py @@ -18,8 +18,7 @@ class TempController(TempControllerBase): self.last_theta_ist = self.theta_ist # Compensate for max heat rate to reduce overshoot - if self.heatrate_soll_set > 0: - self.pid_hold.scale(1.0/self.heatrate_soll_set) + hold_scale = 1.0/self.heatrate_soll_set if self.heatrate_soll_set > 0 else 1.0 self.heatrate_soll = self.heatrate_soll_set * self.pid_hold.get_y() theta_err = self.theta_soll_set - self.theta_ist @@ -27,4 +26,4 @@ class TempController(TempControllerBase): diff = self.theta_soll_set - self.theta_ist self.process_fsm(diff) - self.process_pid(theta_err, heatrate_err) + self.process_pid(theta_err, heatrate_err, hold_scale) diff --git a/components/pid/temp_controller_base.py b/components/pid/temp_controller_base.py index ec09664..610707b 100644 --- a/components/pid/temp_controller_base.py +++ b/components/pid/temp_controller_base.py @@ -106,8 +106,8 @@ class TempControllerBase(APid): print("New state = {}".format(state_next)) self.on_state_entered(state_next) - def process_pid(self, theta_err, heatrate_err): - self.pid_hold.process(theta_err, -self.theta_ist) + def process_pid(self, theta_err, heatrate_err, hold_scale=1.0): + self.pid_hold.process(theta_err, -self.theta_ist, hold_scale) self.pid_rate.process(heatrate_err, -self.heatrate_ist) if self.state == States.IDLE: diff --git a/components/pid/temp_controller_smith.py b/components/pid/temp_controller_smith.py index ee336fb..f4b7cc9 100755 --- a/components/pid/temp_controller_smith.py +++ b/components/pid/temp_controller_smith.py @@ -50,8 +50,7 @@ class TempController(TempControllerBase): self.last_theta_ist = self.theta_ist # Compensate for max heat rate to reduce overshoot - if self.heatrate_soll_set > 0: - self.pid_hold.scale(1.0/self.heatrate_soll_set) + hold_scale = 1.0/self.heatrate_soll_set if self.heatrate_soll_set > 0 else 1.0 self.heatrate_soll = self.heatrate_soll_set * self.pid_hold.get_y() theta_err = self.theta_soll_set - self.theta_ist @@ -59,4 +58,4 @@ class TempController(TempControllerBase): diff = self.theta_soll_set - self.theta_ist self.process_fsm(diff) - self.process_pid(theta_err, heatrate_err) + self.process_pid(theta_err, heatrate_err, hold_scale)