From 0171cb20c7fcbf0d7df256c53b71d478362c635b Mon Sep 17 00:00:00 2001 From: Jens Ahrensfeld Date: Sat, 11 Jul 2026 12:33:57 +0200 Subject: [PATCH] config: fail fast on missing TempCtrl PID gains Config access was unchecked dict[key] everywhere, so a missing/ misnamed gain (we've hit this once already: config.json.sim's gain/ Model nesting mismatch) surfaced as a bare KeyError from deep inside some component's __init__ or first process() tick, with no indication which config key was wrong. utils/config_validate.py adds a small stdlib-only dotted-path checker (no schema library - matches this project's stdlib-first leaning) and validate_temp_ctrl(), scoped to the section that actually caused an incident: TempCtrl.pid_type plus every kp/ki/kd/kt gain under TempCtrl.Outer and TempCtrl.Inner.{Heat,Hold,Cool}. Called once in server/brewpi.py right after json.load(), before any factory runs. Optional-with-default keys (Outer.y_hold_min, Inner.Hold.yi_max, Thresholds) stay optional here too. Heater/Stirrer/TempSensor/Pot sections and unknown-key warnings are deliberately left for later - see components/pid/TODO.md. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01F4oEE99rkKVU8L8tkzXWdG --- components/pid/TODO.md | 31 ++++++++----- server/brewpi.py | 2 + tests/utils/__init__.py | 0 tests/utils/test_config_validate.py | 72 +++++++++++++++++++++++++++++ utils/config_validate.py | 35 ++++++++++++++ 5 files changed, 129 insertions(+), 11 deletions(-) create mode 100644 tests/utils/__init__.py create mode 100644 tests/utils/test_config_validate.py create mode 100644 utils/config_validate.py diff --git a/components/pid/TODO.md b/components/pid/TODO.md index cb1e0d0..0e0be55 100644 --- a/components/pid/TODO.md +++ b/components/pid/TODO.md @@ -55,17 +55,26 @@ git history for `temp_controller.py`/`temp_controller_smith.py`). at both call sites (`brewpi.py`, `components/sud_forecast.py`) were removed along with it. -- [ ] **Config access is unchecked `dict[key]` everywhere.** - `params['Hold']`, `params['Td']`, etc., throughout, with no schema - validation at load time. We already hit this bug class once - (`config.json.sim`'s `gain`/`Model` nesting mismatch). A small - schema/dataclass validation layer at config-load would surface - errors immediately instead of mid-`__init__` — and would have caught - the dead `TempCtrl.Kalman` / `Model.kn` / `Plant.kn` keys that used - to sit unused in `config-sim.json.tpl`/`.sim` (since deleted), and the - `Model.gain`/`Plant.gain` keys that did the same in `config.json.sim` - before that file was removed entirely — `Pot` dropped `gain` - entirely, see `components/plant/TODO.md`. +- [x] **Config access is unchecked `dict[key]` everywhere.** Partially + fixed, scoped to the section that actually caused an incident + (`config.json.sim`'s `gain`/`Model` nesting mismatch): `utils/ + config_validate.py`'s `require()` is a small stdlib-only dotted-path + checker (no schema library added - matches this project's no-pytest/ + stdlib-first preference), and `validate_temp_ctrl()` requires + `TempCtrl.pid_type` plus every `kp`/`ki`/`kd`/`kt` gain under + `TempCtrl.Outer` and `TempCtrl.Inner.{Heat,Hold,Cool}` - the ones every + `pid_type` reads unconditionally. Called once in `server/brewpi.py` + right after `json.load()`, before any factory runs, so a missing/ + misnamed gain now fails fast with `ConfigError: missing required + config key: TempCtrl.Inner.Hold.kp` instead of a bare `KeyError` from + inside some component's `__init__` or first `process()` tick. Covered + by `tests/utils/test_config_validate.py`. + Deliberately left for later, since neither has actually crashed + anything yet: `Heater`/`Stirrer`/`TempSensor`/`Pot` config sections + (all already default gracefully via `.get(..., default)` in + `PlantFactory` etc.), and erroring (vs. just `log.warning()`) on + unknown/stale keys like the old `TempCtrl.Kalman`/`Model.kn`/ + `Plant.kn`/`Model.gain` leftovers - see `components/plant/TODO.md`. - [x] **`pid_heat` can wind up during a `HOLD`-state disturbance with no anti-windup engagement.** A cold-water disturbance while holding drove diff --git a/server/brewpi.py b/server/brewpi.py index e7d1066..ce122db 100755 --- a/server/brewpi.py +++ b/server/brewpi.py @@ -10,6 +10,7 @@ import threading import time from http.server import SimpleHTTPRequestHandler, ThreadingHTTPServer from utils import ChangedFloat +from utils.config_validate import validate_temp_ctrl from ws.message import MessageDispatcher from ws.server.ws_server_multi_user import WsServerMultiUser from components.pid import PidFactory @@ -103,6 +104,7 @@ if __name__ == '__main__': log.info("Logging to {}".format(log_path)) config = json.load(open(args.config)) + validate_temp_ctrl(config) dispatcher = MessageDispatcher() server = WsServerMultiUser(listener=dispatcher) taskmgr = TaskManager() diff --git a/tests/utils/__init__.py b/tests/utils/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/utils/test_config_validate.py b/tests/utils/test_config_validate.py new file mode 100644 index 0000000..0be9b04 --- /dev/null +++ b/tests/utils/test_config_validate.py @@ -0,0 +1,72 @@ +import unittest + +from utils.config_validate import ConfigError, require, validate_temp_ctrl + + +def _valid_temp_ctrl_config(): + gains = {"kp": 0.08, "ki": 0.02, "kd": 0.0, "kt": 1.5} + return { + "TempCtrl": { + "pid_type": "Smith", + "Outer": dict(gains), + "Inner": { + "Heat": dict(gains), + "Hold": dict(gains), + "Cool": dict(gains), + }, + } + } + + +class TestRequire(unittest.TestCase): + def test_returns_value_when_present(self): + self.assertEqual(require({"a": {"b": 1}}, "a.b"), 1) + + def test_raises_on_missing_leaf(self): + with self.assertRaises(ConfigError): + require({"a": {}}, "a.b") + + def test_raises_on_missing_intermediate(self): + with self.assertRaises(ConfigError): + require({}, "a.b") + + def test_error_names_full_path_up_to_missing_key(self): + with self.assertRaisesRegex(ConfigError, r"a\.b"): + require({"a": {}}, "a.b.c") + + def test_raises_on_wrong_type(self): + with self.assertRaises(ConfigError): + require({"a": "not a number"}, "a", type=(int, float)) + + def test_accepts_matching_type(self): + self.assertEqual(require({"a": 1.5}, "a", type=(int, float)), 1.5) + + +class TestValidateTempCtrl(unittest.TestCase): + def test_valid_config_passes(self): + validate_temp_ctrl(_valid_temp_ctrl_config()) + + def test_missing_gain_raises(self): + config = _valid_temp_ctrl_config() + del config["TempCtrl"]["Inner"]["Hold"]["kp"] + with self.assertRaisesRegex(ConfigError, r"TempCtrl\.Inner\.Hold\.kp"): + validate_temp_ctrl(config) + + def test_missing_pid_type_raises(self): + config = _valid_temp_ctrl_config() + del config["TempCtrl"]["pid_type"] + with self.assertRaises(ConfigError): + validate_temp_ctrl(config) + + def test_optional_keys_not_required(self): + config = _valid_temp_ctrl_config() + # Outer.y_hold_min, Inner.Hold.yi_max, and the whole Thresholds + # section all have their own defaults elsewhere and must stay + # optional here too. + validate_temp_ctrl(config) + self.assertNotIn("y_hold_min", config["TempCtrl"]["Outer"]) + self.assertNotIn("Thresholds", config["TempCtrl"]) + + +if __name__ == '__main__': + unittest.main() diff --git a/utils/config_validate.py b/utils/config_validate.py new file mode 100644 index 0000000..c2603b8 --- /dev/null +++ b/utils/config_validate.py @@ -0,0 +1,35 @@ +class ConfigError(Exception): + """Raised for a missing or wrong-typed config key, named by its full + dotted path (e.g. "TempCtrl.Inner.Hold.kp") so the error points straight + at the offending line in config.json instead of surfacing as a bare + KeyError/TypeError from wherever the value first gets used.""" + + +def require(config, path, type=None): + node = config + parts = path.split('.') + for i, key in enumerate(parts): + if not isinstance(node, dict) or key not in node: + raise ConfigError("missing required config key: {}".format('.'.join(parts[:i + 1]))) + node = node[key] + if type is not None and not isinstance(node, type): + type_name = ' or '.join(t.__name__ for t in type) if isinstance(type, tuple) else type.__name__ + raise ConfigError("config key {} must be {}, got {!r}".format(path, type_name, node)) + return node + + +def validate_temp_ctrl(config): + """Checked eagerly at startup so a missing/misnamed PID gain fails with + a clear message here instead of a bare KeyError mid-__init__ or on the + first process() tick (this bit us once already: config.json.sim's gain/ + Model nesting mismatch). Only the gains every pid_type actually reads + unconditionally are required; anything with its own default elsewhere + (Outer.y_hold_min, Inner.Hold.yi_max, the whole Thresholds section - + see temp_controller_base.py/temp_controller_fsm.py's DEFAULT_THRESHOLDS) + stays optional here too, on purpose.""" + require(config, 'TempCtrl', dict) + require(config, 'TempCtrl.pid_type', str) + for gain in ('kp', 'ki', 'kd', 'kt'): + require(config, 'TempCtrl.Outer.{}'.format(gain), (int, float)) + for branch in ('Heat', 'Hold', 'Cool'): + require(config, 'TempCtrl.Inner.{}.{}'.format(branch, gain), (int, float))