diff --git a/README.md b/README.md index e49ccd4..9b4bed3 100644 --- a/README.md +++ b/README.md @@ -113,6 +113,45 @@ Each component runs inside its own `ATask` at a configurable interval keyed WebSocket channel that any connected client can subscribe to and send commands back on (e.g. `{"TempCtrl": {"Soll": {"Temp": 65}}}`). +### State replay on connect + +A fundamental assumption baked into this protocol: a client never knows what +it's connecting to. The GUI might be the first thing to ever talk to a +freshly started server, or it might connect to one that's been running +unattended for hours, mid-brew, with a schedule already loaded and paused +partway through step 4. A client can also disconnect (network hiccup, the +user closing and reopening the GUI) and reconnect later, at any server state, +with no special "resume" handshake. In every one of these cases, the client +must end up with the *same* full picture of current state as one that's been +connected the whole time - not just whatever changes happen after it joins. + +This is what `WsServerMultiUser.global_state` (`ws/server/ws_server_multi_user.py`) +exists for: every message ever broadcast over the dispatcher is merged into +it (`ws/user.py`'s `update()`), keyed exactly like the live channels are. +Subscribing to a channel (`{"+": "Sud"}`, sent automatically for every +channel on connect - see `MessageDispatcherSync(auto_subscribe=True)`) +doesn't just start a future stream of changes; the server immediately +replays the *entire* currently-known state for that channel in one message, +via `User.send()`'s `dpath` search over `global_state`. A reconnecting +client gets exactly the same replay a brand-new one would, because the +server doesn't distinguish between the two cases at all - there's no +"first connect" special-casing anywhere in this path. + +The corollary, easy to miss: this mechanism has no concept of "transient" - +anything sent through it is implicitly durable state that will be replayed +to whoever connects next, even long after the fact. A one-shot *event* +(something that happened, rather than something that's currently true) has +to be modeled as a value that gets explicitly cleared back to neutral right +after sending, or it will linger in `global_state` and get handed to some +unrelated future client as if it just happened to them. `tasks/sud.py`'s +`SudTask.recv()` hit this directly: a `Load` rejected because a run is in +progress sends `{"Sud": {"Error": "..."}}` to explain why, immediately +followed by `{"Sud": {"Error": None}}` to clear it - without that second +send, every client connecting afterwards, no matter how much later or how +unrelated to the rejected `Load`, would immediately see that same stale +error on connect. The client mirrors this by treating a falsy `Error` as a +no-op rather than an empty dialog. + ## Requirements - Python 3.8+ @@ -217,12 +256,17 @@ controller's `theta_soll`/`heatrate_soll` toward each step's target tolerance), counts down hold steps' `duration`, and applies each step's `stirrer` block (`interval_time`/ `on_ratio` map directly onto the stirrer's cycle time/duty cycle). It -exposes progress (current step, remaining hold time, state, any -`user_message`) on the `"Sud"` WebSocket channel and accepts +exposes progress (current step, remaining hold time, elapsed run time - +tick-counted, not derived from wall-clock time times the warp factor, since +the latter is only nominal and drifts under real scheduling load - state, +any `user_message`) on the `"Sud"` WebSocket channel and accepts `{"Sud": {"Start": true}}` to begin the schedule (also restarts one that's already finished), `{"Sud": {"Pause": true}}` to freeze progress without -losing it, and `{"Sud": {"Confirm": true}}` to acknowledge a pause and move -to the next step. +losing it, `{"Sud": {"Confirm": true}}` to acknowledge a pause and move to +the next step, and `{"Sud": {"Load": }}` to replace the +running schedule - refused (with an explicit, one-shot `Error` reply - see +"State replay on connect" above) while a run is already in progress, rather +than the client trying to guess that from its own view of the state. The temperature controller has a master enabled switch (off by default - see `components/pid/temp_controller_base.py`): `SudTask` enables it for as @@ -249,8 +293,10 @@ The GUI's Automatic tab shows up to three time estimates for a schedule: second since it's pure CPU-bound iteration, no real time/IO involved. This replaces the naive preview a moment after a schedule is loaded. - Once running, a *dynamic* one: the already-elapsed part is the actual - measured trace, and only the remaining steps are re-projected from the - live temperature/step/`hold_remaining` each tick. + measured trace, plotted against `Sud.elapsed` (see "State replay on + connect" above) rather than wall-clock time, and only the remaining steps + are re-projected from the live temperature/step/`hold_remaining` each + tick. The naive estimate is structurally optimistic - it assumes the plant instantly tracks the declared rate and that "reached" is instant once the @@ -265,7 +311,14 @@ finishes - if the two diverge by far more than that, suspect a real control issue rather than a forecasting one (see the `HoldCool` threshold note in `components/pid/temp_controller_base.py` - too tight a threshold there once caused exactly this kind of large, otherwise-unexplained gap, by making the -controller chatter in and out of `COOL` and never actually converge). +controller chatter in and out of `COOL` and never actually converge). Rule +out a *display* artifact first, though: the dynamic trace used to position +itself using wall-clock time times the configured `sim_warp_factor`, but +that factor is only nominal - real asyncio/Python scheduling overhead means +the actually-achieved speedup runs measurably below it (confirmed: ~80x +instead of a configured 100x), which alone produced a divergence large +enough to look like a control problem. `Sud.elapsed` (tick-counted, immune +to wall-clock pacing) replaced that reconstruction for exactly this reason. ## Logging & analysis diff --git a/client/brewpi_gui.py b/client/brewpi_gui.py index 7d8e349..52ea29a 100755 --- a/client/brewpi_gui.py +++ b/client/brewpi_gui.py @@ -880,7 +880,13 @@ class Window(QtWidgets.QMainWindow, Ui_MainWindow): elif "Forecast" in key: self.on_sud_forecast_received(msg['Forecast']) elif "Error" in key: - QtWidgets.QMessageBox.warning(self, "Sud", msg['Error']) + # The server clears this back to None right after sending + # it (see tasks/sud.py's recv()) so it doesn't linger in + # global_state and get replayed as stale to a client + # connecting later - the clearing message itself must be + # a no-op here, not an empty dialog. + if msg['Error']: + QtWidgets.QMessageBox.warning(self, "Sud", msg['Error']) self.update_sud_actions() def on_sud_forecast_received(self, forecast): diff --git a/tasks/sud.py b/tasks/sud.py index d9a3a43..db733e4 100644 --- a/tasks/sud.py +++ b/tasks/sud.py @@ -202,7 +202,17 @@ class SudTask(ATask): # silently dropping the request. The client has no # business pre-emptively guessing this itself from its # own (replicated, laggy) view of the state. + # + # Immediately cleared back to None - unlike every other + # field here, this is a one-shot event, not state. The + # dispatcher has no concept of "don't persist this into + # global_state" (see ws/user.py's update()), so without + # clearing it, any client connecting later - even one + # that never touched Load - would get this stale error + # replayed on connect, with nothing it just did to + # explain why. await self.send({'Error': 'Cannot load a new schedule while a run is in progress - stop it first.'}) + await self.send({'Error': None}) async def send(self, data): await self.msg_handler.send(data)