From 2fc27acadfee1a4e76717cf537aac5828db2bd13 Mon Sep 17 00:00:00 2001 From: Chris Lange Date: Mon, 11 May 2026 23:11:01 -0600 Subject: [PATCH 01/16] feat(firmware): wait for application readiness after PIC32 reconnect (closes #145) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds an opt-in readiness probe to FirmwareUpdateService so a PIC32 firmware update doesn't transition to Complete until the device is actually ready to answer normal application commands. The serial transport re-enumerates well before the application firmware is up; without this wait, downstream flows (LAN chip-info queries, WiFi prep) hit a half-started device and either fail or have to reimplement their own retry loop in the calling app — exactly the pattern desktop had to work around. API additions on FirmwareUpdateServiceOptions: - PostReconnectReadinessProbe — Func>?. Returns true when the application is responsive. Null disables the wait (legacy behavior). - PostReconnectReadinessTimeout (default 30s) — wall-clock budget - PostReconnectReadinessRetryDelay (default 500ms) — between probe attempts When the timeout elapses without the probe returning true, the update transitions to Failed with a clear TimeoutException wrapped in FirmwareUpdateException — NOT a silent Complete on a half-ready device. Test plan: 3 new tests cover (a) probe succeeds on attempt N >1 holding back Complete until ready, (b) timeout raises a properly wrapped FirmwareUpdateException with FailedState=JumpingToApp and the readiness keyword in the inner message, (c) null probe preserves legacy fast-complete path. Full suite 893/895 (2 skipped require live hardware). --- .../Firmware/FirmwareUpdateServiceTests.cs | 157 ++++++++++++++++++ .../Firmware/FirmwareUpdateService.cs | 80 +++++++++ .../Firmware/FirmwareUpdateServiceOptions.cs | 32 ++++ 3 files changed, 269 insertions(+) diff --git a/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs b/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs index d86346b4..75d87968 100644 --- a/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs +++ b/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs @@ -491,6 +491,163 @@ public async Task UpdateWifiModuleAsync_WhenDeviceDoesNotSupportLanQuery_Proceed Assert.Contains("SYSTem:COMMUnicate:LAN:FWUpdate", device.SentCommands); } + [Fact] + public async Task UpdateFirmwareAsync_PostReconnectReadinessProbe_AwaitedBeforeComplete() + { + // Closes #145: serial reconnect succeeds before the application + // firmware is ready to answer protobuf status queries. The + // optional readiness probe gives Core a way to wait for true + // application readiness — when set, no caller needs to + // reimplement that retry loop. + // + // The probe here returns false on the first 2 attempts and true + // on the 3rd, simulating a slow PIC32 application boot. Without + // the wait, the update would Complete immediately after serial + // reopens; with the wait, Complete is held back until the probe + // succeeds. + var device = new FakeStreamingDevice("COM3"); + var hidTransport = new FakeHidTransport(); + hidTransport.EnqueueRead([0x01, 0x10]); + hidTransport.EnqueueRead([0x01, 0x02]); + hidTransport.EnqueueRead([0x01, 0x03]); + hidTransport.EnqueueRead([0x01, 0x03]); + hidTransport.EnqueueRead([0x01, 0x10]); + + var enumerator = new FakeHidDeviceEnumerator([ + Array.Empty(), + [new HidDeviceInfo(0x04D8, 0x003C, "path-1", "SN-1", "DAQiFi Bootloader")] + ]); + + var probeCallCount = 0; + var options = CreateFastOptions(); + options.PostReconnectReadinessProbe = (_, _) => + { + probeCallCount++; + return Task.FromResult(probeCallCount >= 3); + }; + options.PostReconnectReadinessTimeout = TimeSpan.FromSeconds(2); + options.PostReconnectReadinessRetryDelay = TimeSpan.FromMilliseconds(10); + + var service = new FirmwareUpdateService( + hidTransport, + new FakeFirmwareDownloadService(), + new FakeExternalProcessRunner(), + NullLogger.Instance, + new FakeBootloaderProtocol([[0xA1, 0x01], [0xA1, 0x02]]), + enumerator, + options); + + var hexPath = CreateTempFile(); + try + { + await service.UpdateFirmwareAsync(device, hexPath); + } + finally + { + File.Delete(hexPath); + } + + Assert.Equal(FirmwareUpdateState.Complete, service.CurrentState); + Assert.Equal(3, probeCallCount); + } + + [Fact] + public async Task UpdateFirmwareAsync_PostReconnectReadinessProbe_TimeoutFailsTheUpdate() + { + // When the application never becomes ready within the configured + // budget, the update must transition to Failed with a clear + // timeout message — NOT silently complete and hand back a + // half-ready device. That's the entire reason for #145. + var device = new FakeStreamingDevice("COM3"); + var hidTransport = new FakeHidTransport(); + hidTransport.EnqueueRead([0x01, 0x10]); + hidTransport.EnqueueRead([0x01, 0x02]); + hidTransport.EnqueueRead([0x01, 0x03]); + hidTransport.EnqueueRead([0x01, 0x03]); + hidTransport.EnqueueRead([0x01, 0x10]); + + var enumerator = new FakeHidDeviceEnumerator([ + Array.Empty(), + [new HidDeviceInfo(0x04D8, 0x003C, "path-1", "SN-1", "DAQiFi Bootloader")] + ]); + + var options = CreateFastOptions(); + // Probe always returns false → never ready → must time out + options.PostReconnectReadinessProbe = (_, _) => Task.FromResult(false); + options.PostReconnectReadinessTimeout = TimeSpan.FromMilliseconds(150); + options.PostReconnectReadinessRetryDelay = TimeSpan.FromMilliseconds(10); + + var service = new FirmwareUpdateService( + hidTransport, + new FakeFirmwareDownloadService(), + new FakeExternalProcessRunner(), + NullLogger.Instance, + new FakeBootloaderProtocol([[0xA1, 0x01], [0xA1, 0x02]]), + enumerator, + options); + + var hexPath = CreateTempFile(); + FirmwareUpdateException ex; + try + { + ex = await Assert.ThrowsAsync( + () => service.UpdateFirmwareAsync(device, hexPath)); + } + finally + { + File.Delete(hexPath); + } + + Assert.Equal(FirmwareUpdateState.Failed, service.CurrentState); + Assert.Equal(FirmwareUpdateState.JumpingToApp, ex.FailedState); + var inner = Assert.IsType(ex.InnerException); + Assert.Contains("application-ready", inner.Message); + } + + [Fact] + public async Task UpdateFirmwareAsync_NoPostReconnectProbe_PreservesLegacyBehavior() + { + // No probe configured == legacy behavior: Complete fires as + // soon as serial reopens. Belt-and-suspenders test that the + // new code path is fully opt-in. + var device = new FakeStreamingDevice("COM3"); + var hidTransport = new FakeHidTransport(); + hidTransport.EnqueueRead([0x01, 0x10]); + hidTransport.EnqueueRead([0x01, 0x02]); + hidTransport.EnqueueRead([0x01, 0x03]); + hidTransport.EnqueueRead([0x01, 0x03]); + hidTransport.EnqueueRead([0x01, 0x10]); + + var enumerator = new FakeHidDeviceEnumerator([ + Array.Empty(), + [new HidDeviceInfo(0x04D8, 0x003C, "path-1", "SN-1", "DAQiFi Bootloader")] + ]); + + var options = CreateFastOptions(); + // No PostReconnectReadinessProbe set - legacy path + + var service = new FirmwareUpdateService( + hidTransport, + new FakeFirmwareDownloadService(), + new FakeExternalProcessRunner(), + NullLogger.Instance, + new FakeBootloaderProtocol([[0xA1, 0x01], [0xA1, 0x02]]), + enumerator, + options); + + var hexPath = CreateTempFile(); + try + { + await service.UpdateFirmwareAsync(device, hexPath); + } + finally + { + File.Delete(hexPath); + } + + Assert.Equal(FirmwareUpdateState.Complete, service.CurrentState); + } + private static FirmwareUpdateServiceOptions CreateFastOptions() { return new FirmwareUpdateServiceOptions diff --git a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs index 7118aa52..77f1d9ca 100644 --- a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs +++ b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs @@ -806,6 +806,86 @@ await _hidTransport await SafeDisconnectHidAsync().ConfigureAwait(false); await WaitForSerialReconnectAsync(device, cancellationToken).ConfigureAwait(false); + + // Application-readiness probe (closes #145). Serial transport + // re-enumeration succeeds well before the PIC32 application + // firmware is actually ready to answer protobuf status queries; + // if a downstream flow (LAN chip info, WiFi prep) starts before + // the app is up, those queries fail and callers reimplement + // their own retry. The probe is opt-in via options — when null, + // the legacy "serial reopened == done" semantics apply. + if (_options.PostReconnectReadinessProbe is { } probe) + { + await WaitForApplicationReadyAsync(device, probe, cancellationToken).ConfigureAwait(false); + } + } + + private async Task WaitForApplicationReadyAsync( + IStreamingDevice device, + Func> probe, + CancellationToken cancellationToken) + { + var totalTimeout = _options.PostReconnectReadinessTimeout; + var retryDelay = _options.PostReconnectReadinessRetryDelay; + + using var timeoutCts = new CancellationTokenSource(totalTimeout); + using var linkedCts = CancellationTokenSource.CreateLinkedTokenSource( + cancellationToken, timeoutCts.Token); + var linkedToken = linkedCts.Token; + + var attempt = 0; + while (true) + { + attempt++; + try + { + linkedToken.ThrowIfCancellationRequested(); + } + catch (OperationCanceledException) when (timeoutCts.IsCancellationRequested && !cancellationToken.IsCancellationRequested) + { + throw new TimeoutException( + $"Device did not become application-ready within {totalTimeout.TotalSeconds:F0}s after PIC32 reconnect (attempt {attempt}). " + + "The serial transport reopened but the readiness probe never returned true; the device may still be initializing or the firmware may have failed to start."); + } + + try + { + if (await probe(device, linkedToken).ConfigureAwait(false)) + { + if (attempt > 1) + { + _logger.LogDebug( + "Device became application-ready on probe attempt {Attempt}.", + attempt); + } + return; + } + _logger.LogDebug("Application-ready probe returned false on attempt {Attempt}; will retry.", attempt); + } + catch (OperationCanceledException) when (timeoutCts.IsCancellationRequested && !cancellationToken.IsCancellationRequested) + { + throw new TimeoutException( + $"Device did not become application-ready within {totalTimeout.TotalSeconds:F0}s after PIC32 reconnect (attempt {attempt}). " + + "The readiness probe was canceled by the timeout while running."); + } + catch (Exception ex) when (ex is not OperationCanceledException) + { + _logger.LogDebug( + ex, + "Application-ready probe threw on attempt {Attempt}; treating as not-ready and retrying.", + attempt); + } + + try + { + await Task.Delay(retryDelay, linkedToken).ConfigureAwait(false); + } + catch (OperationCanceledException) when (timeoutCts.IsCancellationRequested && !cancellationToken.IsCancellationRequested) + { + throw new TimeoutException( + $"Device did not become application-ready within {totalTimeout.TotalSeconds:F0}s after PIC32 reconnect (attempt {attempt})."); + } + } } private async Task WaitForSerialReconnectAsync( diff --git a/src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.cs b/src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.cs index 4b759847..7a03fdbf 100644 --- a/src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.cs +++ b/src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.cs @@ -1,3 +1,5 @@ +using Daqifi.Core.Device; + namespace Daqifi.Core.Firmware; /// @@ -119,6 +121,34 @@ public sealed class FirmwareUpdateServiceOptions /// public string? WifiPortOverride { get; set; } + /// + /// Optional callback that returns true once the device is ready to + /// answer normal application commands after PIC32 firmware update + + /// reconnect (closes #145). The serial transport can re-enumerate + /// well before the application firmware is actually ready to respond + /// to protobuf status queries — without this probe, the next steps + /// in a downstream flow (LAN chip-info, WiFi prep) hit a half-started + /// device and have to retry. When set, the firmware service polls + /// the probe with bounded retry after each PIC32 reconnect; if it + /// never returns true within , + /// the update transitions to Failed with a clear timeout exception + /// rather than silently handing back a half-ready device. When null, + /// reconnect succeeds as soon as the serial port reopens (legacy + /// behavior). + /// + public Func>? PostReconnectReadinessProbe { get; set; } + + /// + /// Wall-clock budget for the post-reconnect readiness probe. Default + /// 30s covers a slow PIC32 boot and downstream-firmware initialization. + /// + public TimeSpan PostReconnectReadinessTimeout { get; set; } = TimeSpan.FromSeconds(30); + + /// + /// Delay between readiness-probe attempts (cancellation-aware). + /// + public TimeSpan PostReconnectReadinessRetryDelay { get; set; } = TimeSpan.FromMilliseconds(500); + /// /// Gets the configured timeout for a given firmware update state. /// @@ -159,6 +189,8 @@ public void Validate() ValidatePositive(HidConnectRetryDelay, nameof(HidConnectRetryDelay)); ValidatePositive(FlashWriteRetryDelay, nameof(FlashWriteRetryDelay)); ValidatePositive(WifiProcessTimeout, nameof(WifiProcessTimeout)); + ValidatePositive(PostReconnectReadinessTimeout, nameof(PostReconnectReadinessTimeout)); + ValidatePositive(PostReconnectReadinessRetryDelay, nameof(PostReconnectReadinessRetryDelay)); if (HidConnectRetryCount < 1) { From 7dc8bdf1e62b95d227196a7b4908e7d98e37d044 Mon Sep 17 00:00:00 2001 From: Chris Lange Date: Mon, 11 May 2026 23:14:47 -0600 Subject: [PATCH 02/16] fix: Apply Qodo /improve pass 1 on PR #200: enforce timeout against ill-behaved probes Defensive cancellation re-check after the await: a probe that ignores its CancellationToken could otherwise return true after the timeout elapsed and slip past the budget. The post-await check forces the TimeoutException path even for that case. Caller-cancellation semantics preserved: the inner try/catch only reinterprets the timeout-CT case; OperationCanceledException from the caller-CT propagates out unchanged via the outer catch. --- .../Firmware/FirmwareUpdateService.cs | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs index 77f1d9ca..0732bb94 100644 --- a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs +++ b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs @@ -850,7 +850,26 @@ private async Task WaitForApplicationReadyAsync( try { - if (await probe(device, linkedToken).ConfigureAwait(false)) + var isReady = await probe(device, linkedToken).ConfigureAwait(false); + + // Re-check the budget after the await: a probe that ignores + // its CancellationToken could otherwise return true after + // the timeout has elapsed and bypass the budget. Caller's + // CT propagates as OperationCanceledException out of this + // method (caught at the same handler below); only the + // timeout-CT case is reinterpreted as TimeoutException. + try + { + linkedToken.ThrowIfCancellationRequested(); + } + catch (OperationCanceledException) when (timeoutCts.IsCancellationRequested && !cancellationToken.IsCancellationRequested) + { + throw new TimeoutException( + $"Device did not become application-ready within {totalTimeout.TotalSeconds:F0}s after PIC32 reconnect (attempt {attempt}). " + + "The readiness probe ignored cancellation and returned after the deadline."); + } + + if (isReady) { if (attempt > 1) { From cff6e8f8825997ba1183b87e11f23a53b43fa340 Mon Sep 17 00:00:00 2001 From: Chris Lange Date: Mon, 11 May 2026 23:17:16 -0600 Subject: [PATCH 03/16] fix: Apply Qodo /improve pass 2 on PR #200: preserve sub-second precision in timeout messages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaced {totalTimeout.TotalSeconds:F0}s with {totalTimeout} across all 4 readiness-probe TimeoutException messages. The :F0 formatter rounded sub-second timeouts to "0s" — including the test value of 150ms — which was unhelpful for diagnostics. {TimeSpan} formats as hh:mm:ss.fffffff and preserves precision. --- src/Daqifi.Core/Firmware/FirmwareUpdateService.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs index 0732bb94..08808d24 100644 --- a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs +++ b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs @@ -844,7 +844,7 @@ private async Task WaitForApplicationReadyAsync( catch (OperationCanceledException) when (timeoutCts.IsCancellationRequested && !cancellationToken.IsCancellationRequested) { throw new TimeoutException( - $"Device did not become application-ready within {totalTimeout.TotalSeconds:F0}s after PIC32 reconnect (attempt {attempt}). " + + $"Device did not become application-ready within {totalTimeout} after PIC32 reconnect (attempt {attempt}). " + "The serial transport reopened but the readiness probe never returned true; the device may still be initializing or the firmware may have failed to start."); } @@ -865,7 +865,7 @@ private async Task WaitForApplicationReadyAsync( catch (OperationCanceledException) when (timeoutCts.IsCancellationRequested && !cancellationToken.IsCancellationRequested) { throw new TimeoutException( - $"Device did not become application-ready within {totalTimeout.TotalSeconds:F0}s after PIC32 reconnect (attempt {attempt}). " + + $"Device did not become application-ready within {totalTimeout} after PIC32 reconnect (attempt {attempt}). " + "The readiness probe ignored cancellation and returned after the deadline."); } @@ -884,7 +884,7 @@ private async Task WaitForApplicationReadyAsync( catch (OperationCanceledException) when (timeoutCts.IsCancellationRequested && !cancellationToken.IsCancellationRequested) { throw new TimeoutException( - $"Device did not become application-ready within {totalTimeout.TotalSeconds:F0}s after PIC32 reconnect (attempt {attempt}). " + + $"Device did not become application-ready within {totalTimeout} after PIC32 reconnect (attempt {attempt}). " + "The readiness probe was canceled by the timeout while running."); } catch (Exception ex) when (ex is not OperationCanceledException) @@ -902,7 +902,7 @@ private async Task WaitForApplicationReadyAsync( catch (OperationCanceledException) when (timeoutCts.IsCancellationRequested && !cancellationToken.IsCancellationRequested) { throw new TimeoutException( - $"Device did not become application-ready within {totalTimeout.TotalSeconds:F0}s after PIC32 reconnect (attempt {attempt})."); + $"Device did not become application-ready within {totalTimeout} after PIC32 reconnect (attempt {attempt})."); } } } From eb28b8dce36f5e10bd6d2e21f15f834d3ad06d64 Mon Sep 17 00:00:00 2001 From: Chris Lange Date: Mon, 11 May 2026 23:20:05 -0600 Subject: [PATCH 04/16] fix: Apply Qodo /improve pass 3 on PR #200: WaitAsync for hard timeout enforcement Replaced the post-await cancellation re-check from pass 2 with Task.WaitAsync(linkedToken) on the probe call itself. WaitAsync is the stronger primitive: when the timeout fires, it throws OperationCanceledException immediately instead of waiting for the rogue probe to return. The post-await re-check was defensive but still required the probe to complete; WaitAsync short-circuits. Skipped a redundant inline option-validation suggestion (already covered by Validate() in FirmwareUpdateServiceOptions). --- .../Firmware/FirmwareUpdateService.cs | 28 +++++++------------ 1 file changed, 10 insertions(+), 18 deletions(-) diff --git a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs index 08808d24..f9af3d0a 100644 --- a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs +++ b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs @@ -850,24 +850,16 @@ private async Task WaitForApplicationReadyAsync( try { - var isReady = await probe(device, linkedToken).ConfigureAwait(false); - - // Re-check the budget after the await: a probe that ignores - // its CancellationToken could otherwise return true after - // the timeout has elapsed and bypass the budget. Caller's - // CT propagates as OperationCanceledException out of this - // method (caught at the same handler below); only the - // timeout-CT case is reinterpreted as TimeoutException. - try - { - linkedToken.ThrowIfCancellationRequested(); - } - catch (OperationCanceledException) when (timeoutCts.IsCancellationRequested && !cancellationToken.IsCancellationRequested) - { - throw new TimeoutException( - $"Device did not become application-ready within {totalTimeout} after PIC32 reconnect (attempt {attempt}). " + - "The readiness probe ignored cancellation and returned after the deadline."); - } + // WaitAsync(linkedToken) enforces the timeout deadline even + // when the probe ignores its own CancellationToken and would + // otherwise hang or return after the budget elapses. When + // the deadline fires, WaitAsync throws OperationCanceledException + // immediately — we don't keep waiting for the rogue probe. + // (Stronger than a post-await re-check: that would still + // wait for the probe to return; this short-circuits.) + var isReady = await probe(device, linkedToken) + .WaitAsync(linkedToken) + .ConfigureAwait(false); if (isReady) { From 32e6f9148fc514fa235b57fd107e8cd91ce0d415 Mon Sep 17 00:00:00 2001 From: Chris Lange Date: Mon, 11 May 2026 23:26:28 -0600 Subject: [PATCH 05/16] fix: Apply Qodo /agentic_review pass 4 on PR #200: preserve probe root cause in TimeoutException MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Qodo flagged that deterministic probe failures lose their root cause: the catch swallows the probe exception, retries, and eventually throws TimeoutException without the underlying error attached. With Debug logs off the real cause is invisible. Now captures the most-recent probe exception and attaches it as InnerException to all 3 timeout-path TimeoutExceptions. Steady-state behavior unchanged; observability improves for the failure case. Skipped 3 other findings: - "Readiness probe is optional" — defensible design per the issue's explicit "callers can provide a readiness probe" wording; making it always-on requires Core to know what "ready" means, which it doesn't. - "Conflicting timeout budgets" — readiness timeout (30s default) fits inside JumpingToApplicationTimeout (45s default) by design; documented in the existing options docstrings. - "Readiness wait not surfaced" — observability nice-to-have; out of scope for this fix. --- .../Firmware/FirmwareUpdateService.cs | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs index f9af3d0a..9213c213 100644 --- a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs +++ b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs @@ -833,6 +833,14 @@ private async Task WaitForApplicationReadyAsync( cancellationToken, timeoutCts.Token); var linkedToken = linkedCts.Token; + // Capture the most recent probe-thrown exception so a TimeoutException + // can carry the underlying cause as InnerException. Without this, + // deterministic probe failures (e.g. transport says it's open but + // the device never responds to status queries) report only as + // "timed out" — losing the actual error context unless Debug logs + // are on. + Exception? lastProbeException = null; + var attempt = 0; while (true) { @@ -845,7 +853,8 @@ private async Task WaitForApplicationReadyAsync( { throw new TimeoutException( $"Device did not become application-ready within {totalTimeout} after PIC32 reconnect (attempt {attempt}). " + - "The serial transport reopened but the readiness probe never returned true; the device may still be initializing or the firmware may have failed to start."); + "The serial transport reopened but the readiness probe never returned true; the device may still be initializing or the firmware may have failed to start.", + lastProbeException); } try @@ -855,8 +864,6 @@ private async Task WaitForApplicationReadyAsync( // otherwise hang or return after the budget elapses. When // the deadline fires, WaitAsync throws OperationCanceledException // immediately — we don't keep waiting for the rogue probe. - // (Stronger than a post-await re-check: that would still - // wait for the probe to return; this short-circuits.) var isReady = await probe(device, linkedToken) .WaitAsync(linkedToken) .ConfigureAwait(false); @@ -877,10 +884,12 @@ private async Task WaitForApplicationReadyAsync( { throw new TimeoutException( $"Device did not become application-ready within {totalTimeout} after PIC32 reconnect (attempt {attempt}). " + - "The readiness probe was canceled by the timeout while running."); + "The readiness probe was canceled by the timeout while running.", + lastProbeException); } catch (Exception ex) when (ex is not OperationCanceledException) { + lastProbeException = ex; _logger.LogDebug( ex, "Application-ready probe threw on attempt {Attempt}; treating as not-ready and retrying.", @@ -894,7 +903,8 @@ private async Task WaitForApplicationReadyAsync( catch (OperationCanceledException) when (timeoutCts.IsCancellationRequested && !cancellationToken.IsCancellationRequested) { throw new TimeoutException( - $"Device did not become application-ready within {totalTimeout} after PIC32 reconnect (attempt {attempt})."); + $"Device did not become application-ready within {totalTimeout} after PIC32 reconnect (attempt {attempt}).", + lastProbeException); } } } From 89657bababb04568606a72f5bd090f4dab43537d Mon Sep 17 00:00:00 2001 From: Chris Lange Date: Mon, 11 May 2026 23:28:58 -0600 Subject: [PATCH 06/16] docs(firmware): clarify PostReconnectReadinessTimeout vs JumpingToApplicationTimeout interaction (PR #200) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address Qodo finding 'Conflicting timeout budgets': the readiness probe's budget runs inside the JumpingToApp state, so users who raise the readiness timeout near/above JumpingToApplicationTimeout will see the outer state-timeout fire first. Default values (45s state, 30s readiness) leave headroom for reconnect; documented the interaction explicitly so configuration mistakes are visible at the call site. Skipping the remaining 2 findings as design choices: - 'Readiness probe is optional' — the issue's own wording specifies caller-provided probe; making it always-on would require Core to know what 'ready' means for an arbitrary device. - 'Readiness wait not surfaced' — observability nice-to-have; out of scope for this fix. --- src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.cs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.cs b/src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.cs index 7a03fdbf..b9ab3326 100644 --- a/src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.cs +++ b/src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.cs @@ -142,6 +142,15 @@ public sealed class FirmwareUpdateServiceOptions /// Wall-clock budget for the post-reconnect readiness probe. Default /// 30s covers a slow PIC32 boot and downstream-firmware initialization. /// + /// + /// This budget runs INSIDE the JumpingToApp state, so the effective + /// upper bound is JumpingToApplicationTimeout - (serial reconnect + /// elapsed). With the defaults (45s state timeout, ~1-5s typical + /// reconnect, 30s readiness budget) the readiness probe gets its full + /// budget; if you raise this near or above the state timeout, the + /// outer state-timeout will fire first and callers will see a generic + /// JumpingToApp timeout instead of the readiness-specific message. + /// public TimeSpan PostReconnectReadinessTimeout { get; set; } = TimeSpan.FromSeconds(30); /// From d34ad9f063ef04339cbbe144ff0c6df43e8ab560 Mon Sep 17 00:00:00 2001 From: Chris Lange Date: Tue, 12 May 2026 00:12:53 -0600 Subject: [PATCH 07/16] Apply Qodo /agentic_review pass 5 on PR #200 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings addressed in this pass: 1. Misleading probe-timeout message: WaitAsync(linkedToken) cancels the *await*, not necessarily the probe (the surrounding comment notes the probe may ignore cancellation and keep running). Reword the TimeoutException message so it doesn't claim the probe itself was canceled. 2. Conflicting timeout budgets: the JumpToApp step is bounded by JumpingToApplicationTimeout via ExecuteWithStateTimeoutAsync, so a probe budget that meets or exceeds the state timeout would always be cut off by the outer timeout — surfacing a generic JumpingToApp error instead of the readiness-specific message. Validate() now rejects the misconfiguration up front when a probe is set. 3. Transport-specific readiness timeout text: the messages hardcoded "after PIC32 reconnect" / "the serial transport reopened", undermining the transport-agnostic readiness design. Reworded to neutral phrasing ("transport reconnected", drop PIC32 prefix). Updates the AwaitedBeforeComplete test to use a 1s readiness budget (under the 2s JumpingToApp timeout from CreateFastOptions). Adds a focused test that locks in the new validation contract. --- .../Firmware/FirmwareUpdateServiceTests.cs | 29 ++++++++++++++++++- .../Firmware/FirmwareUpdateService.cs | 10 +++---- .../Firmware/FirmwareUpdateServiceOptions.cs | 15 ++++++++++ 3 files changed, 48 insertions(+), 6 deletions(-) diff --git a/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs b/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs index 75d87968..deaef36b 100644 --- a/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs +++ b/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs @@ -525,7 +525,9 @@ [new HidDeviceInfo(0x04D8, 0x003C, "path-1", "SN-1", "DAQiFi Bootloader")] probeCallCount++; return Task.FromResult(probeCallCount >= 3); }; - options.PostReconnectReadinessTimeout = TimeSpan.FromSeconds(2); + // Readiness budget must be strictly less than JumpingToApplicationTimeout + // (CreateFastOptions uses 2s) — the probe succeeds within 50ms anyway. + options.PostReconnectReadinessTimeout = TimeSpan.FromSeconds(1); options.PostReconnectReadinessRetryDelay = TimeSpan.FromMilliseconds(10); var service = new FirmwareUpdateService( @@ -648,6 +650,31 @@ [new HidDeviceInfo(0x04D8, 0x003C, "path-1", "SN-1", "DAQiFi Bootloader")] Assert.Equal(FirmwareUpdateState.Complete, service.CurrentState); } + [Fact] + public void Constructor_ProbeWithReadinessTimeoutGteJumpToApp_Throws() + { + // The whole JumpToApp step is bounded by JumpingToApplicationTimeout + // via the outer state-timeout. If the readiness budget meets or + // exceeds it, the outer timeout fires first and surfaces a generic + // JumpingToApp error instead of the readiness-specific one. Validate() + // must reject the misconfiguration up front. + var options = CreateFastOptions(); + options.PostReconnectReadinessProbe = (_, _) => Task.FromResult(true); + options.JumpingToApplicationTimeout = TimeSpan.FromSeconds(2); + options.PostReconnectReadinessTimeout = TimeSpan.FromSeconds(2); + + var ex = Assert.Throws(() => new FirmwareUpdateService( + new FakeHidTransport(), + new FakeFirmwareDownloadService(), + new FakeExternalProcessRunner(), + NullLogger.Instance, + new FakeBootloaderProtocol([[0xA1, 0x01]]), + new FakeHidDeviceEnumerator([Array.Empty()]), + options)); + + Assert.Equal(nameof(FirmwareUpdateServiceOptions.PostReconnectReadinessTimeout), ex.ParamName); + } + private static FirmwareUpdateServiceOptions CreateFastOptions() { return new FirmwareUpdateServiceOptions diff --git a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs index 9213c213..10959cac 100644 --- a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs +++ b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs @@ -852,8 +852,8 @@ private async Task WaitForApplicationReadyAsync( catch (OperationCanceledException) when (timeoutCts.IsCancellationRequested && !cancellationToken.IsCancellationRequested) { throw new TimeoutException( - $"Device did not become application-ready within {totalTimeout} after PIC32 reconnect (attempt {attempt}). " + - "The serial transport reopened but the readiness probe never returned true; the device may still be initializing or the firmware may have failed to start.", + $"Device did not become application-ready within {totalTimeout} (attempt {attempt}). " + + "The transport reconnected but the readiness probe never returned true; the device may still be initializing or the firmware may have failed to start.", lastProbeException); } @@ -883,8 +883,8 @@ private async Task WaitForApplicationReadyAsync( catch (OperationCanceledException) when (timeoutCts.IsCancellationRequested && !cancellationToken.IsCancellationRequested) { throw new TimeoutException( - $"Device did not become application-ready within {totalTimeout} after PIC32 reconnect (attempt {attempt}). " + - "The readiness probe was canceled by the timeout while running.", + $"Device did not become application-ready within {totalTimeout} (attempt {attempt}). " + + "The wait for the readiness probe was canceled by the timeout — note the probe may ignore cancellation and continue running in the background.", lastProbeException); } catch (Exception ex) when (ex is not OperationCanceledException) @@ -903,7 +903,7 @@ private async Task WaitForApplicationReadyAsync( catch (OperationCanceledException) when (timeoutCts.IsCancellationRequested && !cancellationToken.IsCancellationRequested) { throw new TimeoutException( - $"Device did not become application-ready within {totalTimeout} after PIC32 reconnect (attempt {attempt}).", + $"Device did not become application-ready within {totalTimeout} (attempt {attempt}).", lastProbeException); } } diff --git a/src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.cs b/src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.cs index b9ab3326..afe310ef 100644 --- a/src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.cs +++ b/src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.cs @@ -201,6 +201,21 @@ public void Validate() ValidatePositive(PostReconnectReadinessTimeout, nameof(PostReconnectReadinessTimeout)); ValidatePositive(PostReconnectReadinessRetryDelay, nameof(PostReconnectReadinessRetryDelay)); + // The whole JumpToApp step is wrapped by JumpingToApplicationTimeout, + // so a probe budget that meets or exceeds it would always be cut off + // by the outer state-timeout — surfacing a generic JumpingToApp error + // instead of the readiness-specific message. Reject the configuration + // up front so misconfigurations fail fast. + if (PostReconnectReadinessProbe is not null && + PostReconnectReadinessTimeout >= JumpingToApplicationTimeout) + { + throw new ArgumentOutOfRangeException( + nameof(PostReconnectReadinessTimeout), + PostReconnectReadinessTimeout, + $"{nameof(PostReconnectReadinessTimeout)} must be strictly less than {nameof(JumpingToApplicationTimeout)} when {nameof(PostReconnectReadinessProbe)} is set, " + + "otherwise the outer state-timeout fires first and masks the readiness-specific timeout."); + } + if (HidConnectRetryCount < 1) { throw new ArgumentOutOfRangeException( From 881f65a1a49ac11ae3f937cebc1e424a74672f1c Mon Sep 17 00:00:00 2001 From: Chris Lange Date: Tue, 12 May 2026 00:15:18 -0600 Subject: [PATCH 08/16] Apply Qodo /improve pass 6 on PR #200: unify timeout messages The third TimeoutException branch (Task.Delay catch during retry sleep) had a stripped-down message compared to the other two. Extend it to match: same "transport reconnected but probe never returned true" suffix so all three timeout exits read the same way. --- src/Daqifi.Core/Firmware/FirmwareUpdateService.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs index 10959cac..22850fb6 100644 --- a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs +++ b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs @@ -903,7 +903,8 @@ private async Task WaitForApplicationReadyAsync( catch (OperationCanceledException) when (timeoutCts.IsCancellationRequested && !cancellationToken.IsCancellationRequested) { throw new TimeoutException( - $"Device did not become application-ready within {totalTimeout} (attempt {attempt}).", + $"Device did not become application-ready within {totalTimeout} (attempt {attempt}). " + + "The transport reconnected but the readiness probe never returned true; the device may still be initializing or the firmware may have failed to start.", lastProbeException); } } From 782a4f8ab810ab201c5e5c815770b15a70b95b50 Mon Sep 17 00:00:00 2001 From: Chris Lange Date: Tue, 12 May 2026 00:22:07 -0600 Subject: [PATCH 09/16] Apply Qodo /improve pass 7 on PR #200: handle probe-thrown OCE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The OperationCanceledException catch only fired when our own timeoutCts canceled — a probe that raised OCE for an unrelated reason (its own internal CTS, transport-level cancellation, etc.) would fall past every catch and crash the update loop. Widen the catch to all non-caller cancellations and dispatch on timeoutCts.IsCancellationRequested: deadline → TimeoutException as before; otherwise treat as a probe failure, capture as lastProbeException, log Debug, and retry just like any other thrown exception. Adds a focused test that locks in the new behavior. --- .../Firmware/FirmwareUpdateServiceTests.cs | 57 +++++++++++++++++++ .../Firmware/FirmwareUpdateService.cs | 26 +++++++-- 2 files changed, 78 insertions(+), 5 deletions(-) diff --git a/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs b/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs index deaef36b..c2f26ac9 100644 --- a/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs +++ b/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs @@ -606,6 +606,63 @@ [new HidDeviceInfo(0x04D8, 0x003C, "path-1", "SN-1", "DAQiFi Bootloader")] Assert.Contains("application-ready", inner.Message); } + [Fact] + public async Task UpdateFirmwareAsync_PostReconnectProbeThrowsOwnOCE_RetriesAndCompletes() + { + // The probe may legitimately throw OperationCanceledException on its + // own (e.g. its internal CTS expired) without our timeoutCts firing. + // That must NOT crash the update loop — it should be treated as a + // probe failure and retried, just like any other thrown exception. + var device = new FakeStreamingDevice("COM3"); + var hidTransport = new FakeHidTransport(); + hidTransport.EnqueueRead([0x01, 0x10]); + hidTransport.EnqueueRead([0x01, 0x02]); + hidTransport.EnqueueRead([0x01, 0x03]); + hidTransport.EnqueueRead([0x01, 0x03]); + hidTransport.EnqueueRead([0x01, 0x10]); + + var enumerator = new FakeHidDeviceEnumerator([ + Array.Empty(), + [new HidDeviceInfo(0x04D8, 0x003C, "path-1", "SN-1", "DAQiFi Bootloader")] + ]); + + var probeCallCount = 0; + var options = CreateFastOptions(); + options.PostReconnectReadinessProbe = (_, _) => + { + probeCallCount++; + // First two attempts: probe throws its own OCE (e.g. internal CTS). + // Third attempt: probe returns ready. + return probeCallCount < 3 + ? Task.FromException(new OperationCanceledException("probe-internal-cancel")) + : Task.FromResult(true); + }; + options.PostReconnectReadinessTimeout = TimeSpan.FromSeconds(1); + options.PostReconnectReadinessRetryDelay = TimeSpan.FromMilliseconds(10); + + var service = new FirmwareUpdateService( + hidTransport, + new FakeFirmwareDownloadService(), + new FakeExternalProcessRunner(), + NullLogger.Instance, + new FakeBootloaderProtocol([[0xA1, 0x01], [0xA1, 0x02]]), + enumerator, + options); + + var hexPath = CreateTempFile(); + try + { + await service.UpdateFirmwareAsync(device, hexPath); + } + finally + { + File.Delete(hexPath); + } + + Assert.Equal(FirmwareUpdateState.Complete, service.CurrentState); + Assert.Equal(3, probeCallCount); + } + [Fact] public async Task UpdateFirmwareAsync_NoPostReconnectProbe_PreservesLegacyBehavior() { diff --git a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs index 22850fb6..b4fcebfd 100644 --- a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs +++ b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs @@ -880,12 +880,28 @@ private async Task WaitForApplicationReadyAsync( } _logger.LogDebug("Application-ready probe returned false on attempt {Attempt}; will retry.", attempt); } - catch (OperationCanceledException) when (timeoutCts.IsCancellationRequested && !cancellationToken.IsCancellationRequested) + catch (OperationCanceledException ex) when (!cancellationToken.IsCancellationRequested) { - throw new TimeoutException( - $"Device did not become application-ready within {totalTimeout} (attempt {attempt}). " + - "The wait for the readiness probe was canceled by the timeout — note the probe may ignore cancellation and continue running in the background.", - lastProbeException); + // Two cases reach here: + // 1. The wait deadline fired (timeoutCts canceled) — surface + // as TimeoutException so callers see the readiness budget. + // 2. The probe itself threw OperationCanceledException for + // some unrelated reason (its own internal CTS, etc). That + // must NOT crash the update loop — treat it as a probe + // failure and retry, same as any other thrown exception. + if (timeoutCts.IsCancellationRequested) + { + throw new TimeoutException( + $"Device did not become application-ready within {totalTimeout} (attempt {attempt}). " + + "The wait for the readiness probe was canceled by the timeout — note the probe may ignore cancellation and continue running in the background.", + lastProbeException ?? ex); + } + + lastProbeException = ex; + _logger.LogDebug( + ex, + "Application-ready probe was canceled on attempt {Attempt}; treating as not-ready and retrying.", + attempt); } catch (Exception ex) when (ex is not OperationCanceledException) { From 5ac58c00acf9f7a9e24a4f85a992102687f9b6ae Mon Sep 17 00:00:00 2001 From: Chris Lange Date: Tue, 12 May 2026 00:28:12 -0600 Subject: [PATCH 10/16] Apply Qodo /agentic_review pass 8: probes-executed counter, not loop counter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The TimeoutException messages reported "(attempt N)" using the loop iteration counter, which was incremented before the probe ran. A timeout firing during the initial cancellation check could claim "attempt 1" when zero probes had executed — misleading for telemetry. Track probesExecuted separately and increment it inside the probe try block so the messages always reflect the actual count of probe invocations. Also covers the case where the timeout fires before any probe runs (probes executed: 0). --- .../Firmware/FirmwareUpdateService.cs | 23 +++++++++++-------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs index b4fcebfd..bc482639 100644 --- a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs +++ b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs @@ -841,10 +841,12 @@ private async Task WaitForApplicationReadyAsync( // are on. Exception? lastProbeException = null; - var attempt = 0; + // Tracks how many probe invocations have actually run. Distinct from + // the loop iteration counter so the timeout messages don't claim + // "attempt N" when the timeout fired before a probe ever executed. + var probesExecuted = 0; while (true) { - attempt++; try { linkedToken.ThrowIfCancellationRequested(); @@ -852,13 +854,14 @@ private async Task WaitForApplicationReadyAsync( catch (OperationCanceledException) when (timeoutCts.IsCancellationRequested && !cancellationToken.IsCancellationRequested) { throw new TimeoutException( - $"Device did not become application-ready within {totalTimeout} (attempt {attempt}). " + + $"Device did not become application-ready within {totalTimeout} (probes executed: {probesExecuted}). " + "The transport reconnected but the readiness probe never returned true; the device may still be initializing or the firmware may have failed to start.", lastProbeException); } try { + probesExecuted++; // WaitAsync(linkedToken) enforces the timeout deadline even // when the probe ignores its own CancellationToken and would // otherwise hang or return after the budget elapses. When @@ -870,15 +873,15 @@ private async Task WaitForApplicationReadyAsync( if (isReady) { - if (attempt > 1) + if (probesExecuted > 1) { _logger.LogDebug( "Device became application-ready on probe attempt {Attempt}.", - attempt); + probesExecuted); } return; } - _logger.LogDebug("Application-ready probe returned false on attempt {Attempt}; will retry.", attempt); + _logger.LogDebug("Application-ready probe returned false on attempt {Attempt}; will retry.", probesExecuted); } catch (OperationCanceledException ex) when (!cancellationToken.IsCancellationRequested) { @@ -892,7 +895,7 @@ private async Task WaitForApplicationReadyAsync( if (timeoutCts.IsCancellationRequested) { throw new TimeoutException( - $"Device did not become application-ready within {totalTimeout} (attempt {attempt}). " + + $"Device did not become application-ready within {totalTimeout} (probes executed: {probesExecuted}). " + "The wait for the readiness probe was canceled by the timeout — note the probe may ignore cancellation and continue running in the background.", lastProbeException ?? ex); } @@ -901,7 +904,7 @@ private async Task WaitForApplicationReadyAsync( _logger.LogDebug( ex, "Application-ready probe was canceled on attempt {Attempt}; treating as not-ready and retrying.", - attempt); + probesExecuted); } catch (Exception ex) when (ex is not OperationCanceledException) { @@ -909,7 +912,7 @@ private async Task WaitForApplicationReadyAsync( _logger.LogDebug( ex, "Application-ready probe threw on attempt {Attempt}; treating as not-ready and retrying.", - attempt); + probesExecuted); } try @@ -919,7 +922,7 @@ private async Task WaitForApplicationReadyAsync( catch (OperationCanceledException) when (timeoutCts.IsCancellationRequested && !cancellationToken.IsCancellationRequested) { throw new TimeoutException( - $"Device did not become application-ready within {totalTimeout} (attempt {attempt}). " + + $"Device did not become application-ready within {totalTimeout} (probes executed: {probesExecuted}). " + "The transport reconnected but the readiness probe never returned true; the device may still be initializing or the firmware may have failed to start.", lastProbeException); } From ea82fd1fba3731082f1840c40370cd9657ed3964 Mon Sep 17 00:00:00 2001 From: Chris Lange Date: Tue, 12 May 2026 00:38:00 -0600 Subject: [PATCH 11/16] Apply Qodo /improve pass 10: gate readiness validation on probe presence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wrap PostReconnectReadinessTimeout / RetryDelay positive checks AND the cross-property constraint (Timeout < JumpingToApplicationTimeout) in the 'probe is configured' guard. Callers that don't supply a probe now never have to think about these timeouts — symmetric with the no-op runtime behavior in WaitForApplicationReadyAsync. --- .../Firmware/FirmwareUpdateServiceOptions.cs | 37 +++++++++++-------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.cs b/src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.cs index afe310ef..fd79e49e 100644 --- a/src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.cs +++ b/src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.cs @@ -198,22 +198,29 @@ public void Validate() ValidatePositive(HidConnectRetryDelay, nameof(HidConnectRetryDelay)); ValidatePositive(FlashWriteRetryDelay, nameof(FlashWriteRetryDelay)); ValidatePositive(WifiProcessTimeout, nameof(WifiProcessTimeout)); - ValidatePositive(PostReconnectReadinessTimeout, nameof(PostReconnectReadinessTimeout)); - ValidatePositive(PostReconnectReadinessRetryDelay, nameof(PostReconnectReadinessRetryDelay)); - - // The whole JumpToApp step is wrapped by JumpingToApplicationTimeout, - // so a probe budget that meets or exceeds it would always be cut off - // by the outer state-timeout — surfacing a generic JumpingToApp error - // instead of the readiness-specific message. Reject the configuration - // up front so misconfigurations fail fast. - if (PostReconnectReadinessProbe is not null && - PostReconnectReadinessTimeout >= JumpingToApplicationTimeout) + + // The readiness options only matter when a probe is configured — + // gate every readiness validation behind that, both the positive + // checks and the cross-property constraint, so callers that don't + // use the probe never have to think about these timeouts. + if (PostReconnectReadinessProbe is not null) { - throw new ArgumentOutOfRangeException( - nameof(PostReconnectReadinessTimeout), - PostReconnectReadinessTimeout, - $"{nameof(PostReconnectReadinessTimeout)} must be strictly less than {nameof(JumpingToApplicationTimeout)} when {nameof(PostReconnectReadinessProbe)} is set, " + - "otherwise the outer state-timeout fires first and masks the readiness-specific timeout."); + ValidatePositive(PostReconnectReadinessTimeout, nameof(PostReconnectReadinessTimeout)); + ValidatePositive(PostReconnectReadinessRetryDelay, nameof(PostReconnectReadinessRetryDelay)); + + // The whole JumpToApp step is wrapped by JumpingToApplicationTimeout, + // so a probe budget that meets or exceeds it would always be cut off + // by the outer state-timeout — surfacing a generic JumpingToApp error + // instead of the readiness-specific message. Reject the configuration + // up front so misconfigurations fail fast. + if (PostReconnectReadinessTimeout >= JumpingToApplicationTimeout) + { + throw new ArgumentOutOfRangeException( + nameof(PostReconnectReadinessTimeout), + PostReconnectReadinessTimeout, + $"{nameof(PostReconnectReadinessTimeout)} must be strictly less than {nameof(JumpingToApplicationTimeout)} when {nameof(PostReconnectReadinessProbe)} is set, " + + "otherwise the outer state-timeout fires first and masks the readiness-specific timeout."); + } } if (HidConnectRetryCount < 1) From b089763cc5729d22916fe2ee41534f8f9803cf20 Mon Sep 17 00:00:00 2001 From: Chris Lange Date: Tue, 12 May 2026 00:42:31 -0600 Subject: [PATCH 12/16] Apply Qodo /agentic_review pass 11: clear stale probe exception MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the probe throws on attempt N then later attempts return false until the readiness budget expires, the TimeoutException carried the stale exception from attempt N as InnerException — misleading debugging and any handler that inspects InnerException. Clear lastProbeException whenever a probe invocation completes normally (true OR false), so a subsequent timeout only attaches a probe-thrown exception when it's actually the most recent outcome. Locked in with a focused test. --- .../Firmware/FirmwareUpdateServiceTests.cs | 61 +++++++++++++++++++ .../Firmware/FirmwareUpdateService.cs | 6 ++ 2 files changed, 67 insertions(+) diff --git a/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs b/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs index c2f26ac9..1e1953a1 100644 --- a/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs +++ b/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs @@ -663,6 +663,67 @@ [new HidDeviceInfo(0x04D8, 0x003C, "path-1", "SN-1", "DAQiFi Bootloader")] Assert.Equal(3, probeCallCount); } + [Fact] + public async Task UpdateFirmwareAsync_PostReconnectProbeThrowsThenFalseThenTimeout_DoesNotAttachStaleInner() + { + // The probe throws once, then returns false until the readiness budget + // expires. The TimeoutException must NOT carry the stale exception + // from attempt 1 as InnerException — a successful probe call (true OR + // false) should clear the captured exception. + var device = new FakeStreamingDevice("COM3"); + var hidTransport = new FakeHidTransport(); + hidTransport.EnqueueRead([0x01, 0x10]); + hidTransport.EnqueueRead([0x01, 0x02]); + hidTransport.EnqueueRead([0x01, 0x03]); + hidTransport.EnqueueRead([0x01, 0x03]); + hidTransport.EnqueueRead([0x01, 0x10]); + + var enumerator = new FakeHidDeviceEnumerator([ + Array.Empty(), + [new HidDeviceInfo(0x04D8, 0x003C, "path-1", "SN-1", "DAQiFi Bootloader")] + ]); + + var probeCallCount = 0; + var options = CreateFastOptions(); + options.PostReconnectReadinessProbe = (_, _) => + { + probeCallCount++; + // Attempt 1: throw. Subsequent attempts: return false until timeout. + return probeCallCount == 1 + ? Task.FromException(new InvalidOperationException("transient-probe-error")) + : Task.FromResult(false); + }; + options.PostReconnectReadinessTimeout = TimeSpan.FromMilliseconds(150); + options.PostReconnectReadinessRetryDelay = TimeSpan.FromMilliseconds(10); + + var service = new FirmwareUpdateService( + hidTransport, + new FakeFirmwareDownloadService(), + new FakeExternalProcessRunner(), + NullLogger.Instance, + new FakeBootloaderProtocol([[0xA1, 0x01], [0xA1, 0x02]]), + enumerator, + options); + + var hexPath = CreateTempFile(); + FirmwareUpdateException ex; + try + { + ex = await Assert.ThrowsAsync( + () => service.UpdateFirmwareAsync(device, hexPath)); + } + finally + { + File.Delete(hexPath); + } + + var inner = Assert.IsType(ex.InnerException); + // Critical: the InvalidOperationException from attempt 1 must NOT + // be attached — the successful (false) probe in attempt 2+ cleared + // lastProbeException. InnerException.InnerException should be null. + Assert.Null(inner.InnerException); + } + [Fact] public async Task UpdateFirmwareAsync_NoPostReconnectProbe_PreservesLegacyBehavior() { diff --git a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs index bc482639..af204e5b 100644 --- a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs +++ b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs @@ -871,6 +871,12 @@ private async Task WaitForApplicationReadyAsync( .WaitAsync(linkedToken) .ConfigureAwait(false); + // Successful probe invocation (true OR false) means the most + // recent attempt completed normally. Clear lastProbeException + // so a later timeout doesn't carry forward a stale exception + // from an earlier failed attempt as its InnerException. + lastProbeException = null; + if (isReady) { if (probesExecuted > 1) From 26b19fe48e3191ccacdc90540f492043c0df6a4e Mon Sep 17 00:00:00 2001 From: Chris Lange Date: Tue, 12 May 2026 13:29:29 -0600 Subject: [PATCH 13/16] Apply Qodo /agentic_review pass 12: surface readiness wait at Info Add LogInformation at WaitForApplicationReadyAsync entry and on success (with elapsed time + attempt count) so observers tailing the log can distinguish a deliberate readiness poll from a stuck flow. Wait can take up to PostReconnectReadinessTimeout (default 30s); previously only Debug-level breadcrumbs existed during that window. --- .../Firmware/FirmwareUpdateService.cs | 21 +++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs index af204e5b..0787ebdf 100644 --- a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs +++ b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs @@ -828,6 +828,16 @@ private async Task WaitForApplicationReadyAsync( var totalTimeout = _options.PostReconnectReadinessTimeout; var retryDelay = _options.PostReconnectReadinessRetryDelay; + // Surface the wait at Information level so observers tailing the + // log can distinguish "stuck" from "deliberately polling". The + // wait can take up to PostReconnectReadinessTimeout (default 30s); + // without this, the JumpingToApp state appears hung beyond the + // initial transport reopen. + _logger.LogInformation( + "Waiting up to {Timeout} for device to become application-ready (post-reconnect readiness probe).", + totalTimeout); + var waitStart = DateTime.UtcNow; + using var timeoutCts = new CancellationTokenSource(totalTimeout); using var linkedCts = CancellationTokenSource.CreateLinkedTokenSource( cancellationToken, timeoutCts.Token); @@ -879,12 +889,11 @@ private async Task WaitForApplicationReadyAsync( if (isReady) { - if (probesExecuted > 1) - { - _logger.LogDebug( - "Device became application-ready on probe attempt {Attempt}.", - probesExecuted); - } + var elapsed = DateTime.UtcNow - waitStart; + _logger.LogInformation( + "Device became application-ready after {Elapsed} on probe attempt {Attempt}.", + elapsed, + probesExecuted); return; } _logger.LogDebug("Application-ready probe returned false on attempt {Attempt}; will retry.", probesExecuted); From e59410e836d347d510214bf0ae1aac7c85f2d810 Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Thu, 14 May 2026 21:44:38 -0600 Subject: [PATCH 14/16] fix(device): rebuild MessageProducer/Consumer after Disconnect/Connect (PR #200 follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SerialStreamTransport.Stream returns _serialPort.BaseStream, which is a fresh Stream instance after Disconnect() reopens the port. Previously DaqifiDevice.Disconnect kept _messageProducer / _messageConsumer alive with references to the old BaseStream, and Connect's "if (... == null)" guard skipped recreation — so any Send() after a transport reconnect wrote to the disposed stream and silently no-op'd, while the text consumer on the new stream saw zero bytes. Surfaced by PR #200's post-reconnect readiness probe: GetLanChipInfoAsync returned null on every attempt because the probe's Send went to the dead stream. Null the producer/consumer in Disconnect so Connect's existing guard triggers fresh construction against the current Stream. Regression test fails without this change (Send post-reconnect lands on the previous, disposed stream rather than the rotated current one). Note: this does not fully resolve the post-PIC32-reset readiness probe on macOS hardware — a separate USB CDC re-enumeration race appears to leave the new SerialPort handle unable to receive bytes despite IsOpen=true. That's tracked separately. Co-Authored-By: Claude Sonnet 4.6 --- .../Device/DaqifiDeviceWithTransportTests.cs | 100 +++++++++++++++++- src/Daqifi.Core/Device/DaqifiDevice.cs | 12 +++ 2 files changed, 108 insertions(+), 4 deletions(-) diff --git a/src/Daqifi.Core.Tests/Device/DaqifiDeviceWithTransportTests.cs b/src/Daqifi.Core.Tests/Device/DaqifiDeviceWithTransportTests.cs index fc2c499a..83af4988 100644 --- a/src/Daqifi.Core.Tests/Device/DaqifiDeviceWithTransportTests.cs +++ b/src/Daqifi.Core.Tests/Device/DaqifiDeviceWithTransportTests.cs @@ -217,14 +217,106 @@ public void DaqifiDevice_TransportConnectionLost_ShouldUpdateStatus() // Arrange using var transport = new MockStreamTransport(); using var device = new DaqifiDevice("Mock Device", transport); - + device.Connect(); Assert.Equal(ConnectionStatus.Connected, device.Status); - - // Act - Simulate transport connection loss + + // Act - Simulate transport connection loss transport.SimulateConnectionLoss(); - + // Assert Assert.Equal(ConnectionStatus.Lost, device.Status); } + + [Fact] + public void DaqifiDevice_DisconnectThenConnect_SendsToCurrentTransportStream() + { + // Regression for the latent stale-stream bug surfaced by PR #200's + // post-reconnect readiness probe: SerialStreamTransport.Stream returns + // _serialPort.BaseStream, which is a fresh instance after a transport + // Disconnect → Connect. Pre-fix, DaqifiDevice.Disconnect left + // _messageProducer / _messageConsumer alive with references to the + // PREVIOUS BaseStream, and Connect's "if (_messageProducer == null)" + // guard skipped recreation — so any subsequent Send() wrote to the + // disposed stream and silently no-op'd, leaving the text consumer + // with zero bytes on the new stream. Fix nulls them in Disconnect so + // Connect rebuilds them against the transport's current Stream. + using var transport = new SwappingMockStreamTransport(); + using var device = new DaqifiDevice("Mock Device", transport); + + device.Connect(); + Thread.Sleep(50); // MessageProducer background thread spin-up + device.Send(ScpiMessageProducer.GetDeviceInfo); + Thread.Sleep(200); // Allow the producer to flush + var firstStreamBytes = transport.CurrentStreamSnapshot(); + Assert.NotEmpty(firstStreamBytes); + + device.Disconnect(); + transport.RotateStream(); + device.Connect(); + Thread.Sleep(50); + device.Send(ScpiMessageProducer.GetDeviceInfo); + Thread.Sleep(200); + + // The send AFTER reconnect must land on the new stream — not on the + // first (now-disposed) stream we captured above. + var secondStreamBytes = transport.CurrentStreamSnapshot(); + Assert.NotEmpty(secondStreamBytes); + Assert.NotSame(firstStreamBytes, secondStreamBytes); + } + + // Mock transport that swaps to a fresh MemoryStream on RotateStream(), + // mirroring the real SerialStreamTransport whose Stream property returns + // _serialPort.BaseStream — a new instance after each Disconnect → Connect. + private class SwappingMockStreamTransport : IStreamTransport + { + private MemoryStream _stream = new(); + private bool _isConnected; + private bool _disposed; + + public Stream Stream => _disposed ? throw new ObjectDisposedException(nameof(SwappingMockStreamTransport)) : _stream; + public bool IsConnected => _isConnected && !_disposed; + public string ConnectionInfo => _disposed ? "Disposed" : (_isConnected ? "Swap: Connected" : "Swap: Disconnected"); + + public event EventHandler? StatusChanged; + + public void RotateStream() + { + var old = _stream; + _stream = new MemoryStream(); + old.Dispose(); + } + + public byte[] CurrentStreamSnapshot() => _stream.ToArray(); + + public Task ConnectAsync() => ConnectAsync(null); + + public Task ConnectAsync(ConnectionRetryOptions? retryOptions) + { + if (_disposed) throw new ObjectDisposedException(nameof(SwappingMockStreamTransport)); + _isConnected = true; + StatusChanged?.Invoke(this, new TransportStatusEventArgs(true, ConnectionInfo)); + return Task.CompletedTask; + } + + public Task DisconnectAsync() + { + _isConnected = false; + StatusChanged?.Invoke(this, new TransportStatusEventArgs(false, ConnectionInfo)); + return Task.CompletedTask; + } + + public void Connect() => ConnectAsync().Wait(); + public void Disconnect() => DisconnectAsync().Wait(); + + public void Dispose() + { + if (!_disposed) + { + _isConnected = false; + _stream.Dispose(); + _disposed = true; + } + } + } } \ No newline at end of file diff --git a/src/Daqifi.Core/Device/DaqifiDevice.cs b/src/Daqifi.Core/Device/DaqifiDevice.cs index 53ac3329..6a2d3c5e 100644 --- a/src/Daqifi.Core/Device/DaqifiDevice.cs +++ b/src/Daqifi.Core/Device/DaqifiDevice.cs @@ -266,6 +266,18 @@ public void Disconnect() _messageConsumer?.StopSafely(); _messageProducer?.StopSafely(); + // Null the producer/consumer so a subsequent Connect() + // rebuilds them against the transport's current Stream. + // SerialStreamTransport.Stream returns _serialPort.BaseStream, + // which is a new instance after Disconnect() → Connect() + // reopens the port; reusing the old producer/consumer would + // leave them bound to the previous (disposed) BaseStream + // and any Send() would silently no-op. Surfaced by PR #200's + // post-reconnect readiness probe (LAN chip-info returning + // null on every attempt because Send went to a dead stream). + _messageConsumer = null; + _messageProducer = null; + // Disconnect transport if available _transport?.Disconnect(); } From a5ce969cec8d2f4915a8f4ad34214502a6bad706 Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Thu, 14 May 2026 22:06:29 -0600 Subject: [PATCH 15/16] fix(firmware): discard race-winning serial handle after PIC32 reset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After PIC32 firmware update jumps to application, the device's USB CDC interface re-enumerates. On macOS, the first SerialPort.Open() to succeed inside the re-enum window is a "shadow" handle — IsOpen==true but writes silently drop and reads see zero bytes. Hardware-validated on a Nyquist1 (/dev/cu.usbmodem101): the first reconnect succeeded at ~2s with UnauthorizedAccessException rejected for every preceding attempt. Once that race-winning handle was held, subsequent SCPI exchanges returned 0 bytes for tens of seconds; a fresh process opening the same path got a clean handle immediately. Add PostReconnectStaleHandleDelay option (default 2s) and a dance in JumpToApplicationAndReconnectAsync that, after WaitForSerialReconnectAsync succeeds, closes the port, sleeps the delay, then reconnects to obtain a clean kernel binding. Validated end-to-end with PR #200's readiness probe: with the dance + a probe that runs InitializeAsync to wake the post-reset dormant device, the LAN chip-info query returns valid data and the firmware update transitions to Complete. Opt out by setting the option to TimeSpan.Zero (Windows, where the first open is already clean). Tests: - UpdateFirmwareAsync_PostReconnectStaleHandleDelay_TriggersExtraDisconnectReconnect asserts the dance fires (2 disconnects + 2 reconnects vs baseline 1 each). - UpdateFirmwareAsync_PostReconnectStaleHandleDelayZero_SkipsExtraDisconnectReconnect asserts the opt-out path with TimeSpan.Zero keeps the baseline counts. - CreateFastOptions sets the delay to Zero so the unit-test suite (which doesn't exercise USB CDC) doesn't pay the per-test 2s settling cost. Co-Authored-By: Claude Sonnet 4.6 --- .../Firmware/FirmwareUpdateServiceTests.cs | 108 +++++++++++++++++- .../Firmware/FirmwareUpdateService.cs | 20 ++++ .../Firmware/FirmwareUpdateServiceOptions.cs | 14 +++ 3 files changed, 141 insertions(+), 1 deletion(-) diff --git a/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs b/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs index 1c16140c..c1e40e61 100644 --- a/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs +++ b/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs @@ -768,6 +768,108 @@ [new HidDeviceInfo(0x04D8, 0x003C, "path-1", "SN-1", "DAQiFi Bootloader")] Assert.Equal(FirmwareUpdateState.Complete, service.CurrentState); } + [Fact] + public async Task UpdateFirmwareAsync_PostReconnectStaleHandleDelay_TriggersExtraDisconnectReconnect() + { + // After PIC32 reset on macOS, the first SerialPort.Open() to succeed + // inside the USB CDC re-enum window is a "shadow" handle — open but + // bytes don't flow. The fix: close + brief settling delay + reopen + // to obtain a clean kernel binding. Validate the dance runs: + // expect TWO disconnect calls and TWO reconnect cycles inside + // JumpToApplicationAndReconnectAsync (the original after FORCEBOOT, + // plus the extra one introduced by the stale-handle workaround). + var device = new FakeStreamingDevice("COM3"); + var hidTransport = new FakeHidTransport(); + hidTransport.EnqueueRead([0x01, 0x10]); + hidTransport.EnqueueRead([0x01, 0x02]); + hidTransport.EnqueueRead([0x01, 0x03]); + hidTransport.EnqueueRead([0x01, 0x03]); + hidTransport.EnqueueRead([0x01, 0x10]); + + var enumerator = new FakeHidDeviceEnumerator([ + Array.Empty(), + [new HidDeviceInfo(0x04D8, 0x003C, "path-1", "SN-1", "DAQiFi Bootloader")] + ]); + + var options = CreateFastOptions(); + options.PostReconnectStaleHandleDelay = TimeSpan.FromMilliseconds(20); + + var disconnectsBeforeUpdate = device.DisconnectCalls; + var connectAttemptsBeforeUpdate = device.ConnectAttempts; + + var service = new FirmwareUpdateService( + hidTransport, + new FakeFirmwareDownloadService(), + new FakeExternalProcessRunner(), + NullLogger.Instance, + new FakeBootloaderProtocol([[0xA1, 0x01], [0xA1, 0x02]]), + enumerator, + options); + + var hexPath = CreateTempFile(); + try + { + await service.UpdateFirmwareAsync(device, hexPath); + } + finally + { + File.Delete(hexPath); + } + + // Without the dance: 1 disconnect (FORCEBOOT) + 1 connect (reconnect after JumpToApp). + // With the dance: 2 disconnects (FORCEBOOT + stale-handle discard) + // + 2 connects (initial reconnect + clean re-open). + Assert.Equal(FirmwareUpdateState.Complete, service.CurrentState); + Assert.Equal(disconnectsBeforeUpdate + 2, device.DisconnectCalls); + Assert.Equal(connectAttemptsBeforeUpdate + 2, device.ConnectAttempts); + } + + [Fact] + public async Task UpdateFirmwareAsync_PostReconnectStaleHandleDelayZero_SkipsExtraDisconnectReconnect() + { + // Opt-out path: setting PostReconnectStaleHandleDelay to Zero + // (e.g. on Windows where the first open is already clean) must + // skip the extra disconnect/reconnect cycle entirely. + var device = new FakeStreamingDevice("COM3"); + var hidTransport = new FakeHidTransport(); + hidTransport.EnqueueRead([0x01, 0x10]); + hidTransport.EnqueueRead([0x01, 0x02]); + hidTransport.EnqueueRead([0x01, 0x03]); + hidTransport.EnqueueRead([0x01, 0x03]); + hidTransport.EnqueueRead([0x01, 0x10]); + + var enumerator = new FakeHidDeviceEnumerator([ + Array.Empty(), + [new HidDeviceInfo(0x04D8, 0x003C, "path-1", "SN-1", "DAQiFi Bootloader")] + ]); + + var options = CreateFastOptions(); // CreateFastOptions sets this to Zero + + var service = new FirmwareUpdateService( + hidTransport, + new FakeFirmwareDownloadService(), + new FakeExternalProcessRunner(), + NullLogger.Instance, + new FakeBootloaderProtocol([[0xA1, 0x01], [0xA1, 0x02]]), + enumerator, + options); + + var hexPath = CreateTempFile(); + try + { + await service.UpdateFirmwareAsync(device, hexPath); + } + finally + { + File.Delete(hexPath); + } + + Assert.Equal(FirmwareUpdateState.Complete, service.CurrentState); + // 1 FORCEBOOT disconnect + 1 reconnect; no stale-handle dance. + Assert.Equal(1, device.DisconnectCalls); + Assert.Equal(1, device.ConnectAttempts); + } + [Fact] public void Constructor_ProbeWithReadinessTimeoutGteJumpToApp_Throws() { @@ -1267,7 +1369,11 @@ private static FirmwareUpdateServiceOptions CreateFastOptions() PostWifiReconnectDelay = TimeSpan.FromMilliseconds(5), HidConnectRetryDelay = TimeSpan.FromMilliseconds(5), FlashWriteRetryDelay = TimeSpan.FromMilliseconds(5), - WifiProcessTimeout = TimeSpan.FromSeconds(2) + WifiProcessTimeout = TimeSpan.FromSeconds(2), + // Disable the macOS USB CDC stale-handle settling delay for unit + // tests — FakeStreamingDevice doesn't reproduce the re-enum race, + // so the dance is pure latency that would blow the fast budgets. + PostReconnectStaleHandleDelay = TimeSpan.Zero }; } diff --git a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs index fc9fa533..080bc7c4 100644 --- a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs +++ b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs @@ -1011,6 +1011,26 @@ await _hidTransport await SafeDisconnectHidAsync().ConfigureAwait(false); await WaitForSerialReconnectAsync(device, cancellationToken).ConfigureAwait(false); + // Discard the race-winning serial handle from the USB CDC re-enumeration + // window. On macOS the first SerialPort.Open() that succeeds after a PIC32 + // reset is typically a "shadow" handle: IsOpen==true, but the kernel + // device-node isn't fully wired yet — writes silently drop and reads see + // zero bytes. A fresh open after a brief settling delay yields a clean + // binding. Symptom without this step: SCPI Sends after reconnect appear + // to succeed but the device never responds (LEDs stay off, readiness + // probe returns null indefinitely until budget expires). + // Opt out by setting PostReconnectStaleHandleDelay = TimeSpan.Zero + // (callers on platforms where the first open is already clean). + if (_options.PostReconnectStaleHandleDelay > TimeSpan.Zero) + { + _logger.LogDebug( + "Discarding race-winning serial handle; closing and re-opening after {Delay} to obtain a clean USB CDC binding.", + _options.PostReconnectStaleHandleDelay); + device.Disconnect(); + await Task.Delay(_options.PostReconnectStaleHandleDelay, cancellationToken).ConfigureAwait(false); + await WaitForSerialReconnectAsync(device, cancellationToken).ConfigureAwait(false); + } + // Application-readiness probe (closes #145). Serial transport // re-enumeration succeeds well before the PIC32 application // firmware is actually ready to answer protobuf status queries; diff --git a/src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.cs b/src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.cs index 4eecfcc4..efaec8a8 100644 --- a/src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.cs +++ b/src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.cs @@ -158,6 +158,20 @@ public sealed class FirmwareUpdateServiceOptions /// public TimeSpan PostReconnectReadinessRetryDelay { get; set; } = TimeSpan.FromMilliseconds(500); + /// + /// Settling delay between discarding the race-winning serial handle + /// and re-opening the port after a PIC32 reset (closes the macOS-USB-CDC + /// shadow-handle problem). After PIC32 firmware update completes the + /// device's USB CDC interface re-enumerates; on macOS the first + /// SerialPort.Open() that succeeds during that window is typically a + /// "shadow" handle — IsOpen==true but writes silently drop and reads + /// see zero bytes. Closing and re-opening after a brief delay yields + /// a clean kernel binding. Default 2s covers macOS-observed timing; + /// set to TimeSpan.Zero on platforms (notably Windows) where the + /// first open is already clean and the extra cycle is pure latency. + /// + public TimeSpan PostReconnectStaleHandleDelay { get; set; } = TimeSpan.FromSeconds(2); + /// /// Total attempts (initial + retries) for LAN chip-info queries before /// the WiFi version decision falls through to "couldn't check, proceed From 3e4fd88dc665508315f5a71ad1503cb289ab445a Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Thu, 14 May 2026 22:26:45 -0600 Subject: [PATCH 16/16] feat(firmware): auto-wake dormant device + raise stale-handle log level MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After PR #200 hardware validation revealed two ergonomic issues with the readiness probe contract: 1. PIC32 application firmware boots dormant after the bootloader jumps to application (LEDs off, WiFi subsystem unpowered, won't answer LAN queries). Callers writing a "natural" probe like GetLanChipInfoAsync != null would silently fail for tens of seconds because the device needs SYSTem:POWer:STATe 1 (sent by InitializeAsync) before it'll respond. That dormant-state knowledge shouldn't be required of every probe author. 2. The PostReconnectStaleHandleDelay dance was logged at Debug level while the comparable readiness-probe wait is at Information. The dance is a significant operation (2+ seconds inside JumpingToApp); observers tailing logs should see it. Changes in JumpToApplicationAndReconnectAsync: - After the stale-handle dance reopens the port with a clean kernel binding, call InitializeAsync on the device if it's a DaqifiDevice (skipped for test fakes that don't extend it). Wrapped in try/catch with LogWarning so an init failure doesn't mask the eventual probe outcome — the probe stays the source of truth for "ready". - Stale-handle dance logged at Information (was Debug). Hardware-validated end-to-end: with this change, the example app probe simplifies to just `GetLanChipInfoAsync != null` (no wake-up code in the caller). Total post-reconnect time ~9s on Nyquist1 (2.3s race window + 2s stale-handle settle + ~4s init + ~1s probe). Co-Authored-By: Claude Sonnet 4.6 --- .../Firmware/FirmwareUpdateService.cs | 28 ++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs index 080bc7c4..46a5b1ea 100644 --- a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs +++ b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs @@ -1023,7 +1023,7 @@ await _hidTransport // (callers on platforms where the first open is already clean). if (_options.PostReconnectStaleHandleDelay > TimeSpan.Zero) { - _logger.LogDebug( + _logger.LogInformation( "Discarding race-winning serial handle; closing and re-opening after {Delay} to obtain a clean USB CDC binding.", _options.PostReconnectStaleHandleDelay); device.Disconnect(); @@ -1031,6 +1031,32 @@ await _hidTransport await WaitForSerialReconnectAsync(device, cancellationToken).ConfigureAwait(false); } + // Wake the post-reset device. PIC32 application firmware boots + // dormant (LEDs off, WiFi subsystem unpowered, won't answer LAN + // queries) until SYSTem:POWer:STATe 1 is sent. InitializeAsync + // handles that plus the rest of the standard init sequence + // (echo off, stream format, etc.). Without this, callers writing + // a "natural" probe like GetLanChipInfoAsync would silently fail + // for tens of seconds because the device is still dormant. + // Skipped for non-DaqifiDevice transports (e.g. test fakes); they + // are responsible for their own readiness if needed. + if (device is DaqifiDevice initializableDevice) + { + _logger.LogInformation("Waking post-reset device via InitializeAsync."); + try + { + await initializableDevice.InitializeAsync().ConfigureAwait(false); + } + catch (Exception ex) + { + // Don't fail the firmware update outright — the readiness + // probe (if configured) is the source of truth for "ready". + // Surface the init failure as a warning so a probe timeout + // later isn't mysterious. + _logger.LogWarning(ex, "InitializeAsync after reconnect threw; continuing to readiness probe."); + } + } + // Application-readiness probe (closes #145). Serial transport // re-enumeration succeeds well before the PIC32 application // firmware is actually ready to answer protobuf status queries;