diff --git a/components/pid/TODO.md b/components/pid/TODO.md index 0e0be55..6b84fac 100644 --- a/components/pid/TODO.md +++ b/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` diff --git a/tests/components/pid/test_heatrate_filter.py b/tests/components/pid/test_heatrate_filter.py new file mode 100644 index 0000000..5321742 --- /dev/null +++ b/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() diff --git a/tests/components/pid/test_temp_controller_smith_correction.py b/tests/components/pid/test_temp_controller_smith_correction.py new file mode 100644 index 0000000..1f28fe6 --- /dev/null +++ b/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()