commit stale changes

components/pid/TODO.md

@@ -32,17 +32,19 @@ git history for `temp_controller.py`/`temp_controller_smith.py`).
   hold_scale)` — the overshoot-compensation scheduling logic now lives
   entirely in the temp controller, not the generic `Pid` block.
 
-- [ ] **No automated tests for the heat-rate filtering or Smith
-  correction specifically.** An earlier `tests/components/pid/` suite
-  (stdlib `unittest`, no pytest) covering FSM transitions, anti-windup
-  clamping, and Kalman convergence was removed before the Kalman-filter
-  removal/Smith rewrite. A new `tests/components/pid/` suite was added
-  alongside the HOLD-windup fix below (`test_pid.py`,
-  `test_temp_controller_closed_loop.py`) covering the `yi_max` clamp and
-  closed-loop disturbance/ramp/transition behavior, but
-  `_compute_heatrate()`'s backward-difference + low-pass filtering and
-  the two-model Smith correction itself still have no dedicated
-  coverage. This drives a physical heater — worth extending.
+- [x] **No automated tests for the heat-rate filtering or Smith
+  correction specifically.** Fixed: `tests/components/pid/test_heatrate_filter.py`
+  covers `_compute_heatrate()` - a first-tick-reads-zero regression test
+  (verified it would catch the old hardcoded `last_theta_ist = 20` bug),
+  convergence to the true rate on a steady ramp (heating and cooling),
+  and the `beta` pre-filter measurably suppressing noise amplification vs.
+  raw differentiation. `tests/components/pid/test_temp_controller_smith_correction.py`
+  covers the two-model correction in `temp_controller_smith.py`: with a
+  matched internal model, `theta_ist` stays close to the fast (zero-delay)
+  model's own prediction (contrasted against a deliberately mismatched
+  model, which diverges 10x+ further); and the corrected estimate visibly
+  rises before the real transport delay (`Td`) has elapsed, while the raw
+  plant reading is still flat.
 
 - [x] **`set_model_power` isn't defined on every controller, but
   `brewpi.py` wires it unconditionally.** Fixed: `TempControllerBase`

tests/components/pid/test_heatrate_filter.py

@@ -0,0 +1,80 @@
+import unittest
+
+from components.pid.temp_controller_base import TempControllerBase
+
+
+def _make(beta=0.05, dt=1.0):
+    ctrl = TempControllerBase(dt=dt)
+    ctrl.beta = beta
+    return ctrl
+
+
+class TestComputeHeatrateFirstTick(unittest.TestCase):
+    """Regression test for the historical bug (see README's "Forecast vs.
+    actual duration" section): both TempController variants used to
+    hardcode last_theta_ist = 20 at construction, producing a bogus
+    heatrate_ist spike on the very first tick whenever the real starting
+    temperature wasn't actually 20 - most visible in SudForecastEstimator,
+    which builds a fresh controller on every call. last_theta_ist now
+    starts None and the first tick's rate reads as 0 regardless of the
+    absolute starting temperature."""
+
+    def test_first_tick_reads_zero_rate_regardless_of_starting_temperature(self):
+        for starting_temp in (0.0, 20.0, 87.3, -5.0):
+            with self.subTest(starting_temp=starting_temp):
+                ctrl = _make()
+                ctrl._compute_heatrate(starting_temp)
+                self.assertEqual(ctrl.heatrate_ist, 0.0)
+
+
+class TestComputeHeatrateConvergence(unittest.TestCase):
+    """For a perfectly linear ramp, both low-pass stages (the beta
+    pre-filter on theta_ist, then the alpha filter on the differentiated
+    rate) settle to the true underlying rate once their transient decays -
+    a linear signal's slope survives exponential smoothing unchanged."""
+
+    def _run_ramp(self, rate_per_min, ticks=400, dt=1.0, start=30.0):
+        ctrl = _make(dt=dt)
+        theta = start
+        for _ in range(ticks):
+            theta += rate_per_min / 60.0 * dt
+            ctrl._compute_heatrate(theta)
+        return ctrl.heatrate_ist
+
+    def test_converges_to_true_heating_rate(self):
+        self.assertAlmostEqual(self._run_ramp(1.2), 1.2, delta=0.01)
+
+    def test_converges_to_true_cooling_rate(self):
+        self.assertAlmostEqual(self._run_ramp(-0.8), -0.8, delta=0.01)
+
+
+class TestComputeHeatratePrefilterSuppressesNoise(unittest.TestCase):
+    """Filtering theta_ist before differentiating (see _compute_heatrate()'s
+    own docstring) keeps stirrer-induced temperature wobble from being
+    amplified into heatrate_ist noise of the same order as a typical
+    rate_soll. beta=1.0 disables the pre-filter entirely (theta_ist_filtered
+    reduces to the raw reading each tick), giving a same-signal comparison
+    against the real default beta."""
+
+    def _peak_abs_heatrate(self, beta, amplitude=0.05, period=4, ticks=300):
+        ctrl = _make(beta=beta)
+        peak = 0.0
+        for i in range(ticks):
+            wobble = amplitude if (i // (period // 2)) % 2 == 0 else -amplitude
+            ctrl._compute_heatrate(30.0 + wobble)
+            if i > 20:  # past the initial filter transient
+                peak = max(peak, abs(ctrl.heatrate_ist))
+        return peak
+
+    def test_prefilter_reduces_noise_amplification(self):
+        filtered_peak = self._peak_abs_heatrate(beta=0.05)
+        raw_peak = self._peak_abs_heatrate(beta=1.0)
+
+        # The unfiltered case must actually show strong noise amplification
+        # for this comparison to mean anything.
+        self.assertGreater(raw_peak, 0.25)
+        self.assertLess(filtered_peak, raw_peak / 2)
+
+
+if __name__ == '__main__':
+    unittest.main()

tests/components/pid/test_temp_controller_smith_correction.py

@@ -0,0 +1,116 @@
+import unittest
+
+from components.pid.temp_controller_smith import TempController
+from components.plant.pot import Pot
+
+DT = 1.0
+AMBIENT = 20.0
+PLANT_PARAMS = {"M": 27.96, "C": 3403.43, "L": 0.2, "Td": 17}
+GAINS = {"kp": 0.08, "ki": 0.02, "kd": 0.0, "kt": 1.5}
+
+
+def make_controller(model_params):
+    ctrl = TempController(DT)
+    ctrl.set_params({
+        "beta": 0.9,
+        "Outer": dict(GAINS),
+        "Inner": {"Heat": dict(GAINS), "Hold": dict(GAINS), "Cool": dict(GAINS)},
+    })
+    ctrl.set_model_plant_params(model_params)
+    ctrl.set_ambient_temperature(AMBIENT)
+    ctrl.set_enabled(True)
+    return ctrl
+
+
+def make_plant(initial_temp):
+    plant = Pot(DT)
+    plant.set_plant_params(PLANT_PARAMS)
+    plant.set_ambient_temperature(AMBIENT)
+    plant.initial(initial_temp)
+    return plant
+
+
+def step(ctrl, plant, power):
+    """Open-loop: power is an externally chosen step function, not fed back
+    from ctrl.get_power() - isolates the Smith correction's own math from
+    PID tuning. Ordering (plant.process() -> read theta_ist -> ctrl.process()
+    -> only then set next tick's power on both plant and model) mirrors
+    tasks/sud.py's real wiring and test_temp_controller_closed_loop.py's own
+    tick() helper, keeping plant and internal model symmetrically one tick
+    behind whatever power was last commanded."""
+    plant.process()
+    ctrl.set_theta_ist(plant.get_temperature())
+    ctrl.set_theta_soll(100.0)
+    ctrl.set_heatrate_soll(1.0)
+    ctrl.process()
+    plant.set_power(power)
+    ctrl.set_model_power(power)
+
+
+def _run_power_step(ctrl, plant, ticks=200, power=2000.0):
+    max_gap = 0.0
+    for i in range(ticks):
+        step(ctrl, plant, power if i > 0 else 0.0)
+        max_gap = max(max_gap, abs(ctrl.theta_ist - ctrl.theta_ist_model))
+    return max_gap
+
+
+class TestSmithCorrectionTracksFastModelWhenMatched(unittest.TestCase):
+    """theta_ist = theta_ist_model + (theta_ist_plant - theta_ist_model_delay)
+    (temp_controller_smith.py's process()): when the internal model's plant
+    params exactly match the real plant's, model_delay tracks the real
+    plant so closely that the correction term stays near zero and theta_ist
+    reduces to the fast (zero transport-delay) model's own prediction -
+    the core Smith-predictor identity. A small residual persists even here:
+    theta_ist_model_delay is one tick behind theta_ist_plant by construction
+    (post_pid() advances the models after this tick's comparison, so a real
+    plant.process() call always has a one-tick head start - see step()'s
+    own comment), so this checks a small bound, not exact equality."""
+
+    def test_matched_model_stays_close_to_fast_model(self):
+        ctrl = make_controller(PLANT_PARAMS)
+        plant = make_plant(AMBIENT)
+        self.assertLess(_run_power_step(ctrl, plant), 0.1)
+
+    def test_mismatched_model_diverges_much_further(self):
+        # 4x the real mass - the internal model predicts a much slower,
+        # smaller temperature response than the real plant actually has.
+        bad_params = {**PLANT_PARAMS, "M": PLANT_PARAMS["M"] * 4}
+        ctrl = make_controller(bad_params)
+        plant = make_plant(AMBIENT)
+        mismatched_gap = _run_power_step(ctrl, plant)
+
+        matched_gap = _run_power_step(make_controller(PLANT_PARAMS), make_plant(AMBIENT))
+
+        # Confirms the tight bound above is actually meaningful (the
+        # correction mechanism responds to a wrong model), not just always
+        # true regardless of whether the model matches anything.
+        self.assertGreater(mismatched_gap, matched_gap * 10)
+
+
+class TestSmithCorrectionSeesThroughDeadTime(unittest.TestCase):
+    """The whole point of the two-model correction: theta_ist must start
+    responding to a commanded power step well before Td ticks have elapsed,
+    even though the real (delayed) plant's own raw temperature - Pot.process()'s
+    transport-delay line - hasn't moved yet. Without this, the PID would only
+    ever see the effect of a power change Td seconds after commanding it."""
+
+    def test_corrected_estimate_rises_before_raw_plant_reading_does(self):
+        ctrl = make_controller(PLANT_PARAMS)
+        plant = make_plant(AMBIENT)
+
+        ticks_well_inside_dead_time = 5
+        self.assertLess(ticks_well_inside_dead_time, PLANT_PARAMS["Td"])
+
+        for i in range(ticks_well_inside_dead_time):
+            step(ctrl, plant, 2000.0 if i > 0 else 0.0)
+
+        # Raw plant reading: still at the transport delay's input queue,
+        # not showing any temperature rise yet.
+        self.assertEqual(ctrl.theta_ist_plant, AMBIENT)
+        # Corrected estimate: already rising, fed by the zero-delay model.
+        self.assertGreater(ctrl.theta_ist, AMBIENT + 0.01)
+
+
+if __name__ == '__main__':
+    unittest.main()