From 0826138657fb3153844d28e3ef2b8aa80556869c Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 15 Jun 2026 17:34:18 +0000 Subject: [PATCH] Harden cook stability: graceful shutdown, preheat race, PID guards Address the highest-value stability/safety findings from the project review: - Graceful shutdown (C1): Smoker is now IDisposable and the host's ApplicationStopping hook tears it down, so a clean stop/SIGTERM drives the auger and igniter to a safe off-state and releases GPIO/SPI/I2C instead of leaving relays in their last commanded state. All background loops (mode, fire minder, display, ADC read, preheat) now stop on a lifetime token. - Preheat sampling (C2/N1): moved PreheatMonitor.Update out of the Status property getter into a dedicated 1 Hz loop, and guarded the monitor's non-concurrent queue with a lock. Fixes a data race (display loop + every /status caller) and keeps the 60-sample window a true ~60s regardless of how many clients poll status. - CancellationTokenSource lifecycle (N3): the per-mode token is now created and disposed by ModeLoop under a lock that SetMode also takes to cancel, eliminating the dispose-vs-cancel race that could throw ObjectDisposedException and drop a mode change. - PID guards (N4): seed _lastUpdate/_lastTemp, return proportional-only on the first (or post-dropout) reading, skip integral/derivative when dt<=0, and re-seed after a NaN so a sensor dropout can't spike the derivative. - SmokerProxy (N8): add a 10s HTTP timeout so a hung API can't stall callers (e.g. the MQTT bridge), and parse p-value defensively. Adds regression tests for the PID same-instant divide and concurrent PreheatMonitor access. Full review notes captured separately. --- Inferno.Api/Devices/RtdArray.cs | 15 ++- Inferno.Api/Pid/SmokerPid.cs | 50 ++++++-- Inferno.Api/Program.cs | 10 ++ Inferno.Api/Services/DisplayUpdater.cs | 17 ++- Inferno.Api/Services/FireMinder.cs | 17 ++- Inferno.Api/Services/PreheatMonitor.cs | 57 +++++---- Inferno.Api/Services/Smoker.cs | 155 +++++++++++++++++++------ Inferno.Common/Services/SmokerProxy.cs | 7 +- Inferno.Tests/PreheatMonitorTests.cs | 26 +++++ Inferno.Tests/SmokerPidTests.cs | 15 +++ 10 files changed, 291 insertions(+), 78 deletions(-) diff --git a/Inferno.Api/Devices/RtdArray.cs b/Inferno.Api/Devices/RtdArray.cs index 0154132..485c10a 100644 --- a/Inferno.Api/Devices/RtdArray.cs +++ b/Inferno.Api/Devices/RtdArray.cs @@ -30,6 +30,7 @@ public class RtdArray : IRtdArray, IDisposable int _probeInvalidCount; Task _adcReadTask; + readonly CancellationTokenSource _stopCts = new(); public RtdArray(SpiDevice spi) { @@ -52,7 +53,7 @@ private static double GetTemp(ConcurrentQueue resistances) private async Task ReadAdc() { - while (true) + while (!_stopCts.IsCancellationRequested) { int grillValue; int probeValue; @@ -62,17 +63,23 @@ private async Task ReadAdc() grillValue = _adc.Read(0); probeValue = _adc.Read(1); } + catch (OperationCanceledException) + { + break; + } catch (Exception ex) { Console.WriteLine($"{DateTime.Now} {ex.Message} {ex.StackTrace}"); - await Task.Delay(TimeSpan.FromMilliseconds(10)); + try { await Task.Delay(TimeSpan.FromMilliseconds(10), _stopCts.Token); } + catch (OperationCanceledException) { break; } continue; } EnqueueIfValid(_grillResistances, grillValue, "Grill", ref _grillInvalidCount); EnqueueIfValid(_probeResistances, probeValue, "Probe", ref _probeInvalidCount); - await Task.Delay(TimeSpan.FromMilliseconds(10)); + try { await Task.Delay(TimeSpan.FromMilliseconds(10), _stopCts.Token); } + catch (OperationCanceledException) { break; } } } @@ -129,6 +136,8 @@ protected virtual void Dispose(bool disposing) { if (disposing) { + _stopCts.Cancel(); + _stopCts.Dispose(); _adc.Dispose(); } disposedValue = true; diff --git a/Inferno.Api/Pid/SmokerPid.cs b/Inferno.Api/Pid/SmokerPid.cs index f6a0d9e..100a578 100644 --- a/Inferno.Api/Pid/SmokerPid.cs +++ b/Inferno.Api/Pid/SmokerPid.cs @@ -22,33 +22,65 @@ public SmokerPid(double PB, double Ti, double Td) _PB = PB; _Ti = Ti; _Td = Td; + _lastUpdate = DateTime.Now; + // NaN means "no valid previous sample yet" — the next reading seeds state + // instead of computing a derivative/integral across an unknown gap. + _lastTemp = double.NaN; } public double GetControlVariable(double currentTemp) { + DateTime now = DateTime.Now; + if (double.IsNaN(currentTemp)) { - _lastUpdate = DateTime.Now; + // Sensor dropout: hold the integral, advance the clock, and force the + // next valid reading to re-seed so we don't compute a bogus derivative + // across the gap. + _lastUpdate = now; + _lastTemp = double.NaN; return 0; } double error = currentTemp - SetPoint; - double P = GainP() * error; - TimeSpan dT = DateTime.Now - _lastUpdate; - _integral += error * dT.TotalSeconds; - _integral = _integral.Clamp(-IntegralMax(), IntegralMax()); - double I = GainI() * _integral; + if (double.IsNaN(_lastTemp)) + { + // First reading (or first after a dropout): seed state and return + // proportional-only. A stale/huge dt here would otherwise spike the + // integral and derivative. + _lastTemp = currentTemp; + _lastUpdate = now; + Debug.WriteLine($"u={P} (seed)"); + return P; + } + + double dtSeconds = (now - _lastUpdate).TotalSeconds; - double derivative = (currentTemp - _lastTemp) / dT.TotalSeconds; - double D = GainD() * derivative; + double I; + double D = 0; + if (dtSeconds > 0) + { + _integral += error * dtSeconds; + _integral = _integral.Clamp(-IntegralMax(), IntegralMax()); + I = GainI() * _integral; + + double derivative = (currentTemp - _lastTemp) / dtSeconds; + D = GainD() * derivative; + } + else + { + // Two calls in the same instant: no time elapsed, so don't accumulate + // the integral or divide by zero for the derivative. + I = GainI() * _integral; + } double u = P + I + D; Debug.WriteLine($"u={u} ({P}+{I}+{D})"); _lastTemp = currentTemp; - _lastUpdate = DateTime.Now; + _lastUpdate = now; return u; } diff --git a/Inferno.Api/Program.cs b/Inferno.Api/Program.cs index 9ab4df7..7c1619c 100644 --- a/Inferno.Api/Program.cs +++ b/Inferno.Api/Program.cs @@ -30,4 +30,14 @@ app.MapControllers(); +// On a clean stop (systemctl stop / SIGTERM / Ctrl-C), tear the smoker down so the +// auger and igniter relays are de-energized instead of being left in their last +// commanded state. Smoker.Dispose() drives a hard safe-off and releases the devices; +// the shared GPIO controller is disposed afterward. +app.Lifetime.ApplicationStopping.Register(() => +{ + (app.Services.GetService() as IDisposable)?.Dispose(); + _gpio.Dispose(); +}); + app.Run(); diff --git a/Inferno.Api/Services/DisplayUpdater.cs b/Inferno.Api/Services/DisplayUpdater.cs index 08ba684..e1fa9bf 100644 --- a/Inferno.Api/Services/DisplayUpdater.cs +++ b/Inferno.Api/Services/DisplayUpdater.cs @@ -5,7 +5,7 @@ namespace Inferno.Api.Services { - public class DisplayUpdater + public class DisplayUpdater : IDisposable { ISmoker _smoker; IDisplay _display; @@ -13,6 +13,7 @@ public class DisplayUpdater bool _heartbeatFlag; Task _updateDisplayLoop; + readonly CancellationTokenSource _stopCts = new(); public DisplayUpdater(ISmoker smoker, IDisplay display) { @@ -25,7 +26,7 @@ public DisplayUpdater(ISmoker smoker, IDisplay display) private async Task UpdateDisplayLoop() { Debug.WriteLine("Starting display thread."); - while (true) + while (!_stopCts.IsCancellationRequested) { try { @@ -61,7 +62,11 @@ private async Task UpdateDisplayLoop() } _heartbeatFlag = !_heartbeatFlag; - await Task.Delay(TimeSpan.FromSeconds(1)); + await Task.Delay(TimeSpan.FromSeconds(1), _stopCts.Token); + } + catch (OperationCanceledException) + { + break; } catch (Exception ex) { @@ -71,6 +76,12 @@ private async Task UpdateDisplayLoop() } } + public void Dispose() + { + _stopCts.Cancel(); + _stopCts.Dispose(); + } + private string HardwareStatus() { var status = _smoker.Status; diff --git a/Inferno.Api/Services/FireMinder.cs b/Inferno.Api/Services/FireMinder.cs index f08ce26..cf07ab8 100644 --- a/Inferno.Api/Services/FireMinder.cs +++ b/Inferno.Api/Services/FireMinder.cs @@ -6,13 +6,14 @@ namespace Inferno.Api.Services { - public class FireMinder + public class FireMinder : IDisposable { ISmoker _smoker; IRelayDevice _igniter; Func _now; LidMonitor _lidMonitor; Task _fireMinderLoop; + readonly CancellationTokenSource _stopCts = new(); TimeSpan _igniterTimeout = TimeSpan.FromMinutes(10); TimeSpan _fireTimeout = TimeSpan.FromMinutes(10); /// @@ -84,12 +85,16 @@ private async Task FireMinderLoop() { Debug.WriteLine("Starting Fire Minder thread."); ResetFireStatus(); - while (true) + while (!_stopCts.IsCancellationRequested) { try { Tick(); - await Task.Delay(TimeSpan.FromSeconds(1)); + await Task.Delay(TimeSpan.FromSeconds(1), _stopCts.Token); + } + catch (OperationCanceledException) + { + break; } catch (Exception ex) { @@ -100,6 +105,12 @@ private async Task FireMinderLoop() } } + public void Dispose() + { + _stopCts.Cancel(); + _stopCts.Dispose(); + } + /// /// One iteration of the fire-health state machine. Extracted from the loop so /// it can be driven deterministically in tests with an injected clock. diff --git a/Inferno.Api/Services/PreheatMonitor.cs b/Inferno.Api/Services/PreheatMonitor.cs index 3b7ddb2..dad51e2 100644 --- a/Inferno.Api/Services/PreheatMonitor.cs +++ b/Inferno.Api/Services/PreheatMonitor.cs @@ -7,46 +7,55 @@ public class PreheatMonitor public const double ProximityPct = 0.10; private readonly Queue _tempHistory = new(); + // Update() runs on a fixed-cadence loop while Reset() is driven from API + // threads (mode changes); guard the (non-concurrent) queue against both. + private readonly object _lock = new(); public bool IsPreheated { get; private set; } public void Update(double grillTemp, int setPoint, bool isCookingMode, bool isFireHealthy) { - if (IsPreheated) return; - - if (!isCookingMode || !isFireHealthy) + lock (_lock) { - _tempHistory.Clear(); - return; - } + if (IsPreheated) return; + + if (!isCookingMode || !isFireHealthy) + { + _tempHistory.Clear(); + return; + } - if (Double.IsNaN(grillTemp) || grillTemp < 0) - return; + if (Double.IsNaN(grillTemp) || grillTemp < 0) + return; - _tempHistory.Enqueue(grillTemp); - while (_tempHistory.Count > WindowSize) - _tempHistory.Dequeue(); + _tempHistory.Enqueue(grillTemp); + while (_tempHistory.Count > WindowSize) + _tempHistory.Dequeue(); - if (_tempHistory.Count < WindowSize) - return; + if (_tempHistory.Count < WindowSize) + return; - double min = _tempHistory.Min(); - double max = _tempHistory.Max(); - if (max - min >= MaxRange) - return; + double min = _tempHistory.Min(); + double max = _tempHistory.Max(); + if (max - min >= MaxRange) + return; - double avg = _tempHistory.Average(); - double threshold = setPoint * (1.0 - ProximityPct); - if (avg < threshold) - return; + double avg = _tempHistory.Average(); + double threshold = setPoint * (1.0 - ProximityPct); + if (avg < threshold) + return; - IsPreheated = true; + IsPreheated = true; + } } public void Reset() { - IsPreheated = false; - _tempHistory.Clear(); + lock (_lock) + { + IsPreheated = false; + _tempHistory.Clear(); + } } } } diff --git a/Inferno.Api/Services/Smoker.cs b/Inferno.Api/Services/Smoker.cs index 74cb965..657d8c5 100644 --- a/Inferno.Api/Services/Smoker.cs +++ b/Inferno.Api/Services/Smoker.cs @@ -7,7 +7,7 @@ namespace Inferno.Api.Services { - public class Smoker : ISmoker + public class Smoker : ISmoker, IDisposable { SmokerMode _mode; IRelayDevice _auger; @@ -39,7 +39,12 @@ public class Smoker : ISmoker /// TimeSpan _holdCycle = TimeSpan.FromSeconds(10); + // Per-mode token: cancelled by SetMode to interrupt the running mode's delay. + // Guarded by _ctsLock so SetMode's Cancel() can never race ModeLoop's Dispose(). CancellationTokenSource _cts = null!; + readonly object _ctsLock = new(); + // Cancelled once, on Dispose, to stop every background loop for a clean shutdown. + readonly CancellationTokenSource _lifetimeCts = new(); SmokerPid _pid; DateTime _lastModeChange; @@ -74,6 +79,7 @@ public class Smoker : ISmoker TimeSpan _recoveryFeedWaitTime = TimeSpan.FromSeconds(5); Task _modeLoopTask; + Task _preheatLoopTask; DisplayUpdater _displayUpdater; FireMinder _fireMinder; PreheatMonitor _preheatMonitor; @@ -95,14 +101,13 @@ public Smoker(IRelayDevice auger, _lastModeChange = DateTime.Now; PValue = 2; - _cts = new CancellationTokenSource(); - _pid = new SmokerPid(60.0, 180.0, 45.0); _displayUpdater = new DisplayUpdater(this, _display); _fireMinder = new FireMinder(this, _igniter); _preheatMonitor = new PreheatMonitor(); _modeLoopTask = ModeLoop(); + _preheatLoopTask = PreheatLoop(); } public SmokerMode Mode => _mode; @@ -136,9 +141,9 @@ public SmokerStatus Status { get { - _preheatMonitor.Update( - _rtdArray.GrillTemp, _setPoint, - _mode.IsCookingMode(), _fireMinder.IsFireHealthy); + // Preheat sampling runs on its own fixed-cadence loop (PreheatLoop); + // reading status no longer mutates it. This keeps the 60-sample window + // a true ~60s and avoids a data race on the monitor's queue. return new SmokerStatus() { AugerOn = _auger.IsOn, @@ -207,9 +212,14 @@ public bool SetMode(SmokerMode newMode) _mode = newMode; _lastModeChange = DateTime.Now; - if (_cts != null && !_cts.IsCancellationRequested) + // Interrupt the running mode's in-flight delay. ModeLoop owns disposal of + // the token (under the same lock), so cancelling here is always safe. + lock (_ctsLock) { - _cts.Cancel(); + if (_cts != null && !_cts.IsCancellationRequested) + { + _cts.Cancel(); + } } return true; } @@ -220,35 +230,42 @@ public bool SetMode(SmokerMode newMode) private async Task ModeLoop() { Debug.WriteLine("Starting mode thread."); - while (true) + while (!_lifetimeCts.IsCancellationRequested) { + // Fresh per-mode token, linked to the lifetime token so Dispose() also + // unblocks the running mode. The previous iteration's token is disposed + // here (its awaits have all completed) under the lock SetMode uses to + // cancel, so Cancel() and Dispose() can never race. + lock (_ctsLock) + { + _cts?.Dispose(); + _cts = CancellationTokenSource.CreateLinkedTokenSource(_lifetimeCts.Token); + } + try { - using (_cts = new CancellationTokenSource()) + switch (_mode) { - switch (_mode) - { - case SmokerMode.Error: - case SmokerMode.Shutdown: - await Shutdown(); - break; - - case SmokerMode.Hold: - await Hold(); - break; - - case SmokerMode.Sear: - await Sear(); - break; - - case SmokerMode.Smoke: - await Smoke(); - break; - - case SmokerMode.Ready: - await Ready(); - break; - } + case SmokerMode.Error: + case SmokerMode.Shutdown: + await Shutdown(); + break; + + case SmokerMode.Hold: + await Hold(); + break; + + case SmokerMode.Sear: + await Sear(); + break; + + case SmokerMode.Smoke: + await Smoke(); + break; + + case SmokerMode.Ready: + await Ready(); + break; } } catch (Exception ex) @@ -261,6 +278,33 @@ private async Task ModeLoop() } } + /// + /// Samples the preheat detector on a fixed 1 Hz cadence. Kept off the Status + /// getter so the rolling window stays a true ~60s regardless of how many + /// clients poll status, and so the (non-concurrent) window isn't raced. + /// + private async Task PreheatLoop() + { + while (!_lifetimeCts.IsCancellationRequested) + { + try + { + _preheatMonitor.Update( + _rtdArray.GrillTemp, _setPoint, + _mode.IsCookingMode(), _fireMinder.IsFireHealthy); + await Task.Delay(TimeSpan.FromSeconds(1), _lifetimeCts.Token); + } + catch (OperationCanceledException) + { + break; + } + catch (Exception ex) + { + Debug.WriteLine($"{DateTime.Now} Preheat loop exception! {ex.Message}"); + } + } + } + /// /// Releases pellets at a pre-determined rate for /// low-temperature cooking with lots of smoke. @@ -498,7 +542,50 @@ private async Task Ready() _blower.Off(); _igniter.Off(); - await Task.Delay(TimeSpan.FromSeconds(1)); + try + { + await Task.Delay(TimeSpan.FromSeconds(1), _cts.Token); + } + catch (TaskCanceledException) + { + } + } + + private bool _disposed; + + public void Dispose() + { + if (_disposed) return; + _disposed = true; + + // Stop every background loop first so nothing re-energizes a relay while + // we're tearing down. + _lifetimeCts.Cancel(); + _fireMinder?.Dispose(); + _displayUpdater?.Dispose(); + + // Drive the fire to a safe terminal state: cut fuel and ignition. Process + // shutdown can't block for the timed Shutdown-mode cooldown (systemd would + // SIGKILL us), so this is a hard safe-off — residual heat dissipates on its + // own. The blower is released below. + try { _auger.Off(); } catch { } + try { _igniter.Off(); } catch { } + + // Release hardware. RelayDevice.Dispose drives the pin off and closes it; + // RtdArray stops its read loop and frees the ADC/SPI; Display frees the LCD. + (_auger as IDisposable)?.Dispose(); + (_blower as IDisposable)?.Dispose(); + (_igniter as IDisposable)?.Dispose(); + (_rtdArray as IDisposable)?.Dispose(); + (_display as IDisposable)?.Dispose(); + + lock (_ctsLock) + { + _cts?.Dispose(); + } + // Deliberately not disposing _lifetimeCts: ModeLoop may still read its Token + // as it winds down, and disposing it would throw. The process is exiting, so + // the single lingering CancellationTokenSource is reclaimed anyway. } } } \ No newline at end of file diff --git a/Inferno.Common/Services/SmokerProxy.cs b/Inferno.Common/Services/SmokerProxy.cs index 3b2ba79..2bc2c90 100644 --- a/Inferno.Common/Services/SmokerProxy.cs +++ b/Inferno.Common/Services/SmokerProxy.cs @@ -15,7 +15,9 @@ public class SmokerProxy : IDisposable public SmokerProxy() { - _client = new HttpClient(); + // Bound every call so a hung API can't stall a caller indefinitely + // (e.g. the MQTT bridge's state loop). + _client = new HttpClient { Timeout = TimeSpan.FromSeconds(10) }; } private bool disposedValue; @@ -40,7 +42,8 @@ public async Task SetPValueAsync(int pValue) public async Task GetPValueAsync() { HttpResponseMessage result = await InfernoApiRequestAsync(SmokerEndpoint.pvalue); - return int.Parse(await result.Content.ReadAsStringAsync()); + string body = await result.Content.ReadAsStringAsync(); + return int.TryParse(body, out int pValue) ? pValue : 0; } public async Task SetModeAsync(SmokerMode smokerMode) diff --git a/Inferno.Tests/PreheatMonitorTests.cs b/Inferno.Tests/PreheatMonitorTests.cs index 3f60285..f4e5ddf 100644 --- a/Inferno.Tests/PreheatMonitorTests.cs +++ b/Inferno.Tests/PreheatMonitorTests.cs @@ -4,6 +4,32 @@ namespace Inferno.Tests; public class PreheatMonitorTests { + [Fact] + public void Update_And_Reset_Concurrently_DoNotThrow() + { + // Update() runs on the preheat loop while Reset() fires from API threads on a + // mode change. The internal queue isn't concurrent, so hammer both from many + // threads and assert the lock keeps it from throwing/corrupting. + var monitor = new PreheatMonitor(); + var stop = DateTime.UtcNow + TimeSpan.FromMilliseconds(500); + + var updaters = Enumerable.Range(0, 4).Select(_ => Task.Run(() => + { + var rng = new Random(); + while (DateTime.UtcNow < stop) + monitor.Update(rng.Next(150, 230), 225, isCookingMode: true, isFireHealthy: true); + })); + + var resetters = Enumerable.Range(0, 2).Select(_ => Task.Run(() => + { + while (DateTime.UtcNow < stop) + monitor.Reset(); + })); + + // Should complete without InvalidOperationException ("collection modified"). + Task.WaitAll(updaters.Concat(resetters).ToArray()); + } + private static void FeedStableTemps(PreheatMonitor monitor, double temp, int setPoint, int count) { for (int i = 0; i < count; i++) diff --git a/Inferno.Tests/SmokerPidTests.cs b/Inferno.Tests/SmokerPidTests.cs index 1949987..8d65b68 100644 --- a/Inferno.Tests/SmokerPidTests.cs +++ b/Inferno.Tests/SmokerPidTests.cs @@ -68,6 +68,21 @@ public void GetControlVariable_NaN_DoesNotCorruptState() Assert.False(double.IsInfinity(u), "Control variable should not be Infinity after NaN recovery"); } + [Fact] + public void GetControlVariable_TwoCallsSameInstant_ReturnsFinite() + { + var pid = new SmokerPid(60.0, 180.0, 45.0); + pid.SetPoint = 225; + + // Back-to-back calls with effectively zero elapsed time must not divide by + // zero in the derivative term and produce NaN/Infinity. + pid.GetControlVariable(200); // seed + double u = pid.GetControlVariable(200); + + Assert.False(double.IsNaN(u), $"Control variable should not be NaN, got {u}"); + Assert.False(double.IsInfinity(u), $"Control variable should not be Infinity, got {u}"); + } + [Fact] public void SetPoint_CanBeUpdated() {