From ce2329bdb1de20416b845afe91e0b2dc85fef2a6 Mon Sep 17 00:00:00 2001 From: Chris Lange Date: Mon, 11 May 2026 22:31:02 -0600 Subject: [PATCH 1/7] feat(firmware): split WiFi update planning from execution (closes #143) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a public CheckWifiFirmwareStatusAsync planning method and a skipVersionCheck parameter on UpdateWifiModuleAsync so callers (typically desktop UI) can make the version decision once and run the flash without triggering Core's hidden second probe. The previous flow conflated two responsibilities in UpdateWifiModuleAsync: deciding whether the device was already current, then executing the flash. Desktop's workaround was to query the version separately, then route the flash through an adapter that intentionally hid ILanChipInfoProvider so Core would skip its internal probe — an API-shape workaround the issue explicitly called out. API additions: - WifiFirmwareStatus record + WifiFirmwareStatusReason enum exposing the chip info, latest release, and categorical decision so callers can distinguish "definitely up to date" from "couldn't check". - IFirmwareUpdateService.CheckWifiFirmwareStatusAsync — pure planning method that does NOT mutate service state (no Complete transition, no progress events). Callers own their own logging / UI flow. - IFirmwareUpdateService.UpdateWifiModuleAsync gains skipVersionCheck: false default. Existing call sites unchanged; callers that just made the decision pass true to bypass. Implementation pulls the data-gathering out of IsWifiFirmwareUpToDateAsync into CheckWifiFirmwareStatusCoreAsync; the legacy method now delegates to it and only adds the side effects (Complete state + progress) needed by the all-in-one caller path. Test plan: 4 new tests cover the planning method's three primary outcomes (UpToDate / UpdateAvailable / DeviceDoesNotSupportLanQuery) and the skipVersionCheck pathway. 894/896 (+4 net new); 2 skips are real-hardware. All 47 WiFi-related tests pass. --- .../Firmware/FirmwareUpdateServiceTests.cs | 160 ++++++++++++++++++ .../Firmware/FirmwareUpdateService.cs | 120 ++++++++++--- .../Firmware/IFirmwareUpdateService.cs | 21 +++ .../Firmware/WifiFirmwareStatus.cs | 67 ++++++++ 4 files changed, 347 insertions(+), 21 deletions(-) create mode 100644 src/Daqifi.Core/Firmware/WifiFirmwareStatus.cs diff --git a/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs b/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs index d86346b..e24ba11 100644 --- a/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs +++ b/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs @@ -491,6 +491,166 @@ public async Task UpdateWifiModuleAsync_WhenDeviceDoesNotSupportLanQuery_Proceed Assert.Contains("SYSTem:COMMUnicate:LAN:FWUpdate", device.SentCommands); } + [Fact] + public async Task CheckWifiFirmwareStatusAsync_WhenVersionMatches_ReturnsUpToDate() + { + // Closes #143: callers can now make the version decision themselves + // without triggering UpdateWifiModuleAsync's hidden internal probe. + // This test asserts the new public planning method returns the + // expected status object and DOES NOT mutate service state. + var wifiRelease = new FirmwareReleaseInfo + { + Version = new FirmwareVersion(19, 5, 4, null, 0), + TagName = "19.5.4", + IsPreRelease = false + }; + var device = new FakeLanChipInfoStreamingDevice("COM9", chipInfo: new LanChipInfo + { + ChipId = 1234, + FwVersion = "19.5.4", + BuildDate = "Jan 8 2019" + }); + + var service = new FirmwareUpdateService( + new FakeHidTransport(), + new FakeFirmwareDownloadService { LatestWifiRelease = wifiRelease }, + new FakeExternalProcessRunner(), + NullLogger.Instance, + new FakeBootloaderProtocol([[0x10]]), + new FakeHidDeviceEnumerator([]), + CreateFastOptions()); + + var status = await service.CheckWifiFirmwareStatusAsync(device); + + Assert.True(status.IsUpToDate); + Assert.Equal(WifiFirmwareStatusReason.UpToDate, status.Reason); + Assert.NotNull(status.CurrentChipInfo); + Assert.Equal("19.5.4", status.CurrentChipInfo!.FwVersion); + Assert.NotNull(status.LatestRelease); + Assert.Equal("19.5.4", status.LatestRelease!.TagName); + + // The planning method must NOT mutate service state (the internal + // IsWifiFirmwareUpToDateAsync transitions to Complete on its + // hit-path; CheckWifiFirmwareStatusAsync must not, so callers can + // call it freely without locking out a subsequent flash. + Assert.Equal(FirmwareUpdateState.Idle, service.CurrentState); + } + + [Fact] + public async Task CheckWifiFirmwareStatusAsync_WhenVersionOlder_ReturnsUpdateAvailable() + { + var wifiRelease = new FirmwareReleaseInfo + { + Version = new FirmwareVersion(19, 6, 1, null, 0), + TagName = "19.6.1", + IsPreRelease = false + }; + var device = new FakeLanChipInfoStreamingDevice("COM10", chipInfo: new LanChipInfo + { + ChipId = 1234, + FwVersion = "19.5.4", + BuildDate = "Jan 8 2019" + }); + + var service = new FirmwareUpdateService( + new FakeHidTransport(), + new FakeFirmwareDownloadService { LatestWifiRelease = wifiRelease }, + new FakeExternalProcessRunner(), + NullLogger.Instance, + new FakeBootloaderProtocol([[0x10]]), + new FakeHidDeviceEnumerator([]), + CreateFastOptions()); + + var status = await service.CheckWifiFirmwareStatusAsync(device); + + Assert.False(status.IsUpToDate); + Assert.Equal(WifiFirmwareStatusReason.UpdateAvailable, status.Reason); + Assert.Equal(FirmwareUpdateState.Idle, service.CurrentState); + } + + [Fact] + public async Task CheckWifiFirmwareStatusAsync_WhenDeviceDoesNotSupportLanQuery_ReturnsDeviceDoesNotSupportLanQuery() + { + var device = new FakeStreamingDevice("COM11"); + + var service = new FirmwareUpdateService( + new FakeHidTransport(), + new FakeFirmwareDownloadService(), + new FakeExternalProcessRunner(), + NullLogger.Instance, + new FakeBootloaderProtocol([[0x10]]), + new FakeHidDeviceEnumerator([]), + CreateFastOptions()); + + var status = await service.CheckWifiFirmwareStatusAsync(device); + + Assert.False(status.IsUpToDate); + Assert.Equal(WifiFirmwareStatusReason.DeviceDoesNotSupportLanQuery, status.Reason); + Assert.Null(status.CurrentChipInfo); + Assert.Null(status.LatestRelease); + } + + [Fact] + public async Task UpdateWifiModuleAsync_WithSkipVersionCheck_BypassesProbeAndAlwaysFlashes() + { + // The motivating case for #143: caller already made the version + // decision (via CheckWifiFirmwareStatusAsync). Pass skipVersionCheck:true + // so Core does NOT re-probe the device — even when the device's + // current version equals the latest, the flash flow runs. + var wifiRelease = new FirmwareReleaseInfo + { + Version = new FirmwareVersion(19, 5, 4, null, 0), + TagName = "19.5.4", + IsPreRelease = false + }; + var device = new FakeLanChipInfoStreamingDevice("COM12", chipInfo: new LanChipInfo + { + ChipId = 1234, + FwVersion = "19.5.4", // Matches latest — would normally short-circuit + BuildDate = "Jan 8 2019" + }); + + var externalProcessRunner = new FakeExternalProcessRunner + { + NextResult = new ExternalProcessResult(0, false, TimeSpan.FromMilliseconds(10), [], []) + }; + + var options = CreateFastOptions(); + options.PostLanFirmwareModeDelay = TimeSpan.FromMilliseconds(5); + options.PostWifiReconnectDelay = TimeSpan.FromMilliseconds(5); + + var service = new FirmwareUpdateService( + new FakeHidTransport(), + new FakeFirmwareDownloadService { LatestWifiRelease = wifiRelease }, + externalProcessRunner, + NullLogger.Instance, + new FakeBootloaderProtocol([[0x10]]), + new FakeHidDeviceEnumerator([]), + options); + + var firmwareDir = CreateTempDirectory(); + File.WriteAllText(Path.Combine(firmwareDir, "winc_flash_tool.cmd"), "@echo off"); + + try + { + await service.UpdateWifiModuleAsync( + device, + firmwareDir, + progress: null, + cancellationToken: CancellationToken.None, + skipVersionCheck: true); + } + finally + { + Directory.Delete(firmwareDir, recursive: true); + } + + // Even though device version matched latest, flash ran because + // skipVersionCheck bypassed the probe. + Assert.Equal(FirmwareUpdateState.Complete, service.CurrentState); + Assert.Contains("SYSTem:COMMUnicate:LAN:FWUpdate", device.SentCommands); + } + private static FirmwareUpdateServiceOptions CreateFastOptions() { return new FirmwareUpdateServiceOptions diff --git a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs index 7118aa5..4c8e512 100644 --- a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs +++ b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs @@ -158,7 +158,8 @@ public async Task UpdateWifiModuleAsync( IStreamingDevice device, string firmwarePath, IProgress? progress = null, - CancellationToken cancellationToken = default) + CancellationToken cancellationToken = default, + bool skipVersionCheck = false) { ArgumentNullException.ThrowIfNull(device); @@ -174,10 +175,21 @@ public async Task UpdateWifiModuleAsync( } await RunExclusiveAsync( - ct => RunWifiUpdateAsync(device, firmwarePath, progress, ct), + ct => RunWifiUpdateAsync(device, firmwarePath, progress, skipVersionCheck, ct), cancellationToken).ConfigureAwait(false); } + /// + public async Task CheckWifiFirmwareStatusAsync( + IStreamingDevice device, + CancellationToken cancellationToken = default) + { + ArgumentNullException.ThrowIfNull(device); + ThrowIfDisposed(); + + return await CheckWifiFirmwareStatusCoreAsync(device, cancellationToken).ConfigureAwait(false); + } + private async Task RunExclusiveAsync( Func operation, CancellationToken cancellationToken) @@ -330,13 +342,15 @@ private async Task RunWifiUpdateAsync( IStreamingDevice device, string firmwarePath, IProgress? progress, + bool skipVersionCheck, CancellationToken cancellationToken) { const long totalBytes = 100; try { - if (await IsWifiFirmwareUpToDateAsync(device, progress, cancellationToken).ConfigureAwait(false)) + if (!skipVersionCheck + && await IsWifiFirmwareUpToDateAsync(device, progress, cancellationToken).ConfigureAwait(false)) { return; } @@ -432,10 +446,50 @@ private async Task IsWifiFirmwareUpToDateAsync( IStreamingDevice device, IProgress? progress, CancellationToken cancellationToken) + { + // Internal callsite: in addition to deciding the boolean, we must + // transition to Complete + report 100% progress so the caller's + // single UpdateWifiModuleAsync(...) call observes the same end-state + // as a successful flash. CheckWifiFirmwareStatusAsync (the public + // planning API) does not have that side effect — its callers own + // their own logging / UI transitions. + var status = await CheckWifiFirmwareStatusCoreAsync(device, cancellationToken).ConfigureAwait(false); + + switch (status.Reason) + { + case WifiFirmwareStatusReason.UpdateAvailable: + _logger.LogInformation( + "WiFi firmware update available: device has {DeviceVersion}, latest is {LatestVersion}.", + status.CurrentChipInfo!.FwVersion, + status.LatestRelease!.TagName); + return false; + + case WifiFirmwareStatusReason.UpToDate: + var message = $"WiFi firmware is already up to date (device: {status.CurrentChipInfo!.FwVersion}, latest: {status.LatestRelease!.TagName})."; + _logger.LogInformation(message); + TransitionToState(FirmwareUpdateState.Complete, message); + ReportProgress(progress, FirmwareUpdateState.Complete, 100, message, 100, 100); + return true; + + default: + // DeviceDoesNotSupportLanQuery, ChipInfoUnavailable, + // LatestReleaseUnavailable, VersionUnparseable — proceed + // with the flash conservatively. + return false; + } + } + + private async Task CheckWifiFirmwareStatusCoreAsync( + IStreamingDevice device, + CancellationToken cancellationToken) { if (device is not ILanChipInfoProvider lanChipInfoProvider) { - return false; + return new WifiFirmwareStatus + { + IsUpToDate = false, + Reason = WifiFirmwareStatusReason.DeviceDoesNotSupportLanQuery, + }; } LanChipInfo? chipInfo; @@ -445,13 +499,21 @@ private async Task IsWifiFirmwareUpToDateAsync( } catch (Exception ex) when (ex is not OperationCanceledException) { - _logger.LogDebug(ex, "Failed to query LAN chip info; proceeding with WiFi firmware update."); - return false; + _logger.LogDebug(ex, "Failed to query LAN chip info; reporting status as ChipInfoUnavailable."); + return new WifiFirmwareStatus + { + IsUpToDate = false, + Reason = WifiFirmwareStatusReason.ChipInfoUnavailable, + }; } if (chipInfo == null) { - return false; + return new WifiFirmwareStatus + { + IsUpToDate = false, + Reason = WifiFirmwareStatusReason.ChipInfoUnavailable, + }; } FirmwareReleaseInfo? latestWifi; @@ -463,29 +525,45 @@ private async Task IsWifiFirmwareUpToDateAsync( } catch (Exception ex) when (ex is not OperationCanceledException) { - _logger.LogDebug(ex, "Failed to query latest WiFi firmware release; proceeding with update."); - return false; + _logger.LogDebug(ex, "Failed to query latest WiFi firmware release; reporting status as LatestReleaseUnavailable."); + return new WifiFirmwareStatus + { + CurrentChipInfo = chipInfo, + IsUpToDate = false, + Reason = WifiFirmwareStatusReason.LatestReleaseUnavailable, + }; } if (latestWifi == null) { - return false; + return new WifiFirmwareStatus + { + CurrentChipInfo = chipInfo, + IsUpToDate = false, + Reason = WifiFirmwareStatusReason.LatestReleaseUnavailable, + }; } - if (!IsWifiVersionCurrent(chipInfo.FwVersion, latestWifi.TagName)) + if (!FirmwareVersion.TryParse(chipInfo.FwVersion, out _) || + !FirmwareVersion.TryParse(latestWifi.TagName, out _)) { - _logger.LogInformation( - "WiFi firmware update available: device has {DeviceVersion}, latest is {LatestVersion}.", - chipInfo.FwVersion, - latestWifi.TagName); - return false; + return new WifiFirmwareStatus + { + CurrentChipInfo = chipInfo, + LatestRelease = latestWifi, + IsUpToDate = false, + Reason = WifiFirmwareStatusReason.VersionUnparseable, + }; } - var message = $"WiFi firmware is already up to date (device: {chipInfo.FwVersion}, latest: {latestWifi.TagName})."; - _logger.LogInformation(message); - TransitionToState(FirmwareUpdateState.Complete, message); - ReportProgress(progress, FirmwareUpdateState.Complete, 100, message, 100, 100); - return true; + var isCurrent = IsWifiVersionCurrent(chipInfo.FwVersion, latestWifi.TagName); + return new WifiFirmwareStatus + { + CurrentChipInfo = chipInfo, + LatestRelease = latestWifi, + IsUpToDate = isCurrent, + Reason = isCurrent ? WifiFirmwareStatusReason.UpToDate : WifiFirmwareStatusReason.UpdateAvailable, + }; } private static bool IsWifiVersionCurrent(string deviceVersion, string latestTagName) diff --git a/src/Daqifi.Core/Firmware/IFirmwareUpdateService.cs b/src/Daqifi.Core/Firmware/IFirmwareUpdateService.cs index 46df9df..4bd3a9c 100644 --- a/src/Daqifi.Core/Firmware/IFirmwareUpdateService.cs +++ b/src/Daqifi.Core/Firmware/IFirmwareUpdateService.cs @@ -38,9 +38,30 @@ Task UpdateFirmwareAsync( /// Path to a WiFi tool executable/script or directory containing it. /// Optional progress reporter. /// Cancellation token. + /// + /// When true, bypass the internal version probe and always run the flash + /// flow. Use after a separate + /// call so the device isn't queried twice — see issue #143 for the + /// motivating callsite. + /// Task UpdateWifiModuleAsync( IStreamingDevice device, string firmwarePath, IProgress? progress = null, + CancellationToken cancellationToken = default, + bool skipVersionCheck = false); + + /// + /// Probes the device for its current WiFi chip info and looks up the + /// latest WiFi firmware release, returning the comparison result without + /// mutating service state. Lets callers (typically GUI/desktop integrations) + /// surface their own logging, retry policy, or UI before deciding whether + /// to call . Pass + /// skipVersionCheck: true to that call to avoid a second probe. + /// + /// The connected streaming device to inspect. + /// Cancellation token. + Task CheckWifiFirmwareStatusAsync( + IStreamingDevice device, CancellationToken cancellationToken = default); } diff --git a/src/Daqifi.Core/Firmware/WifiFirmwareStatus.cs b/src/Daqifi.Core/Firmware/WifiFirmwareStatus.cs new file mode 100644 index 0000000..7a5cb83 --- /dev/null +++ b/src/Daqifi.Core/Firmware/WifiFirmwareStatus.cs @@ -0,0 +1,67 @@ +namespace Daqifi.Core.Firmware; + +/// +/// Result of : +/// the inputs Core uses to decide whether a WiFi update is needed plus the +/// boolean conclusion. Returned without mutating service state, so callers can +/// inspect, log, retry, or surface UI before deciding to call +/// . +/// +/// +/// When is +/// or , both +/// and are non-null. +/// Other reasons leave one or both null and conservatively report +/// = false so callers default to running the update. +/// +public sealed record WifiFirmwareStatus +{ + /// + /// The current WiFi chip info read from the device, or null if the device + /// did not expose or the query failed. + /// + public LanChipInfo? CurrentChipInfo { get; init; } + + /// + /// The latest WiFi firmware release on GitHub, or null if the lookup + /// failed (e.g. offline, rate-limited). + /// + public FirmwareReleaseInfo? LatestRelease { get; init; } + + /// + /// True only when both versions are available AND the device version is + /// at least the latest release. Any unknown is reported as false so the + /// caller defaults to "needs update". + /// + public required bool IsUpToDate { get; init; } + + /// + /// Why has its current value — lets callers + /// distinguish "definitively up to date" from "couldn't check, assuming not". + /// + public required WifiFirmwareStatusReason Reason { get; init; } +} + +/// +/// Categorical outcome for . +/// +public enum WifiFirmwareStatusReason +{ + /// Device version >= latest release version. + UpToDate, + + /// Device version < latest release version. + UpdateAvailable, + + /// The device does not implement . + DeviceDoesNotSupportLanQuery, + + /// Querying the device for chip info failed. + ChipInfoUnavailable, + + /// Looking up the latest release on GitHub failed. + LatestReleaseUnavailable, + + /// Either version string failed to parse. + VersionUnparseable, +} From f3a37ec2bde6d826505b7288771ef469d3cc903e Mon Sep 17 00:00:00 2001 From: Chris Lange Date: Mon, 11 May 2026 22:32:38 -0600 Subject: [PATCH 2/7] Code review: clean up before PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move skipVersionCheck before cancellationToken in UpdateWifiModuleAsync — .NET CA1068 says CancellationToken should be the last parameter. Update the one test using positional CancellationToken.None to match the new signature (named arg works either way). --- src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs | 1 - src/Daqifi.Core/Firmware/FirmwareUpdateService.cs | 4 ++-- src/Daqifi.Core/Firmware/IFirmwareUpdateService.cs | 4 ++-- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs b/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs index e24ba11..e5c8a4c 100644 --- a/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs +++ b/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs @@ -637,7 +637,6 @@ await service.UpdateWifiModuleAsync( device, firmwareDir, progress: null, - cancellationToken: CancellationToken.None, skipVersionCheck: true); } finally diff --git a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs index 4c8e512..f15f10a 100644 --- a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs +++ b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs @@ -158,8 +158,8 @@ public async Task UpdateWifiModuleAsync( IStreamingDevice device, string firmwarePath, IProgress? progress = null, - CancellationToken cancellationToken = default, - bool skipVersionCheck = false) + bool skipVersionCheck = false, + CancellationToken cancellationToken = default) { ArgumentNullException.ThrowIfNull(device); diff --git a/src/Daqifi.Core/Firmware/IFirmwareUpdateService.cs b/src/Daqifi.Core/Firmware/IFirmwareUpdateService.cs index 4bd3a9c..ccb37f6 100644 --- a/src/Daqifi.Core/Firmware/IFirmwareUpdateService.cs +++ b/src/Daqifi.Core/Firmware/IFirmwareUpdateService.cs @@ -48,8 +48,8 @@ Task UpdateWifiModuleAsync( IStreamingDevice device, string firmwarePath, IProgress? progress = null, - CancellationToken cancellationToken = default, - bool skipVersionCheck = false); + bool skipVersionCheck = false, + CancellationToken cancellationToken = default); /// /// Probes the device for its current WiFi chip info and looks up the From 1ccb31b92db258e7c001a3f605348dcc6dafe314 Mon Sep 17 00:00:00 2001 From: Chris Lange Date: Mon, 11 May 2026 22:37:21 -0600 Subject: [PATCH 3/7] fix: Apply Qodo /improve pass 1 on PR #198: serialize status probe + use parsed Version Two findings, both importance 8: 1. Race condition: CheckWifiFirmwareStatusAsync and UpdateWifiModuleAsync both run SCPI text exchanges on the same transport. Without serialization, two concurrent callers would interleave on the wire and either corrupt the consumer swap or get partial replies. Now acquires _operationLock directly (without going through RunExclusiveAsync's Idle-state check, since a status probe is read-only and should still be available to UI mid-update). 2. Redundant version parsing: latestWifi.Version is already a typed FirmwareVersion populated by FirmwareDownloadService, but the code re-parsed latestWifi.TagName via IsWifiVersionCurrent. Use the typed value directly to avoid the extra parse and any divergence from canonical (e.g., differing tag-prefix conventions). The now- unused IsWifiVersionCurrent helper is dead-coded out. --- .../Firmware/FirmwareUpdateService.cs | 37 +++++++++++-------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs index f15f10a..ee798db 100644 --- a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs +++ b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs @@ -187,7 +187,22 @@ public async Task CheckWifiFirmwareStatusAsync( ArgumentNullException.ThrowIfNull(device); ThrowIfDisposed(); - return await CheckWifiFirmwareStatusCoreAsync(device, cancellationToken).ConfigureAwait(false); + // Serialize device I/O with UpdateFirmwareAsync / UpdateWifiModuleAsync. + // GetLanChipInfoAsync runs a SCPI text exchange on the same transport + // those updates use; a concurrent caller would interleave on the wire + // and either corrupt the consumer swap or get partial replies. + // Acquired without the RunExclusiveAsync state check — a status probe + // is read-only and should be available to UI even when an update is + // in flight (it just waits for the in-flight I/O to release the lock). + await _operationLock.WaitAsync(cancellationToken).ConfigureAwait(false); + try + { + return await CheckWifiFirmwareStatusCoreAsync(device, cancellationToken).ConfigureAwait(false); + } + finally + { + _operationLock.Release(); + } } private async Task RunExclusiveAsync( @@ -544,8 +559,11 @@ private async Task CheckWifiFirmwareStatusCoreAsync( }; } - if (!FirmwareVersion.TryParse(chipInfo.FwVersion, out _) || - !FirmwareVersion.TryParse(latestWifi.TagName, out _)) + // Only the device-reported version needs parsing; latestWifi.Version + // is already a strongly-typed FirmwareVersion from FirmwareDownloadService. + // Re-parsing TagName would risk divergence from the canonical Version + // (different tag prefix conventions, etc.) and cost an extra parse. + if (!FirmwareVersion.TryParse(chipInfo.FwVersion, out var deviceVersion)) { return new WifiFirmwareStatus { @@ -556,7 +574,7 @@ private async Task CheckWifiFirmwareStatusCoreAsync( }; } - var isCurrent = IsWifiVersionCurrent(chipInfo.FwVersion, latestWifi.TagName); + var isCurrent = deviceVersion >= latestWifi.Version; return new WifiFirmwareStatus { CurrentChipInfo = chipInfo, @@ -566,17 +584,6 @@ private async Task CheckWifiFirmwareStatusCoreAsync( }; } - private static bool IsWifiVersionCurrent(string deviceVersion, string latestTagName) - { - if (!FirmwareVersion.TryParse(deviceVersion, out var device) || - !FirmwareVersion.TryParse(latestTagName, out var latest)) - { - return false; - } - - return device >= latest; - } - private ExternalProcessRequest BuildWifiProcessRequest( IStreamingDevice device, string firmwarePath, From bba154a9f16b886a1ea7f16579a067dcd9ccd2df Mon Sep 17 00:00:00 2001 From: Chris Lange Date: Mon, 11 May 2026 22:40:05 -0600 Subject: [PATCH 4/7] fix: Apply Qodo /agentic_review pass 2 on PR #198: keep CancellationToken at 4th position MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Earlier code-review pass moved skipVersionCheck before cancellationToken to satisfy CA1068 ("CancellationToken should be last"). Qodo correctly flagged this as source-breaking — any existing positional caller passing CancellationToken as the 4th argument would no longer compile. Reverted: cancellationToken stays at 4th position; skipVersionCheck goes after it as a 5th optional. Suppressed CA1068 with a comment explaining the additivity-over-style trade-off. --- src/Daqifi.Core/Firmware/FirmwareUpdateService.cs | 6 ++++-- src/Daqifi.Core/Firmware/IFirmwareUpdateService.cs | 10 ++++++++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs index ee798db..73fef46 100644 --- a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs +++ b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs @@ -154,12 +154,14 @@ await RunExclusiveAsync( } /// +#pragma warning disable CA1068 public async Task UpdateWifiModuleAsync( IStreamingDevice device, string firmwarePath, IProgress? progress = null, - bool skipVersionCheck = false, - CancellationToken cancellationToken = default) + CancellationToken cancellationToken = default, + bool skipVersionCheck = false) +#pragma warning restore CA1068 { ArgumentNullException.ThrowIfNull(device); diff --git a/src/Daqifi.Core/Firmware/IFirmwareUpdateService.cs b/src/Daqifi.Core/Firmware/IFirmwareUpdateService.cs index ccb37f6..de57b85 100644 --- a/src/Daqifi.Core/Firmware/IFirmwareUpdateService.cs +++ b/src/Daqifi.Core/Firmware/IFirmwareUpdateService.cs @@ -44,12 +44,18 @@ Task UpdateFirmwareAsync( /// call so the device isn't queried twice — see issue #143 for the /// motivating callsite. /// + // skipVersionCheck added AFTER cancellationToken (technically violates + // CA1068 "CancellationToken should be last") to avoid breaking source + // compat for any existing positional caller passing CancellationToken + // as the 4th argument. Additivity wins over strict style here. +#pragma warning disable CA1068 Task UpdateWifiModuleAsync( IStreamingDevice device, string firmwarePath, IProgress? progress = null, - bool skipVersionCheck = false, - CancellationToken cancellationToken = default); + CancellationToken cancellationToken = default, + bool skipVersionCheck = false); +#pragma warning restore CA1068 /// /// Probes the device for its current WiFi chip info and looks up the From b076e5f2d419a55f70e388a093918b3e699024d9 Mon Sep 17 00:00:00 2001 From: Chris Lange Date: Mon, 11 May 2026 22:44:27 -0600 Subject: [PATCH 5/7] fix: Apply Qodo /agentic_review pass 3 on PR #198: AsyncLocal reentrancy guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Qodo flagged a real deadlock: UpdateWifiModuleAsync holds _operationLock while synchronously firing progress.Report. If a handler calls CheckWifiFirmwareStatusAsync from inside that callback, WaitAsync would block waiting for the lock its own caller's flow already holds — SemaphoreSlim is not re-entrant. Added AsyncLocal _isInsideOperation flag, set true by both lock acquisition sites (CheckWifiFirmwareStatusAsync and RunExclusiveAsync). CheckWifiFirmwareStatusAsync now skips the lock acquisition when the flag is already true on the current async flow — safe because the outer flow already provides exclusive device-I/O access. AsyncLocal flows through await resumptions on different threads, so this catches re-entrancy even when the inner call resumes on a different thread than the outer (mirrors the AsyncLocal pattern from PR #196's ExecuteTextCommandAsync hardening). Regression test added: invokes CheckWifiFirmwareStatusAsync from inside a progress.Report callback during UpdateWifiModuleAsync. Without the guard the test hangs on the deadlocked semaphore; with the guard the inner call returns immediately and the update completes normally. --- .../Firmware/FirmwareUpdateServiceTests.cs | 87 +++++++++++++++++++ .../Firmware/FirmwareUpdateService.cs | 27 ++++++ 2 files changed, 114 insertions(+) diff --git a/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs b/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs index e5c8a4c..549a52e 100644 --- a/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs +++ b/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs @@ -650,6 +650,93 @@ await service.UpdateWifiModuleAsync( Assert.Contains("SYSTem:COMMUnicate:LAN:FWUpdate", device.SentCommands); } + [Fact] + public async Task CheckWifiFirmwareStatusAsync_ReentrantCallFromUpdateProgressCallback_DoesNotDeadlock() + { + // Closes a Qodo finding on PR #198: UpdateWifiModuleAsync holds + // _operationLock while synchronously firing progress.Report(). + // A handler that calls back into CheckWifiFirmwareStatusAsync + // would deadlock waiting for the same lock the update flow owns + // (SemaphoreSlim is not re-entrant). The AsyncLocal _isInsideOperation + // flag detects this case and skips the second acquisition. + // + // The test invokes CheckWifiFirmwareStatusAsync from inside a + // progress callback fired by UpdateWifiModuleAsync. Without the + // re-entrancy guard, this hangs until xunit's per-test budget + // kills it. With the guard, the inner call returns quickly and + // the update completes normally. + var wifiRelease = new FirmwareReleaseInfo + { + Version = new FirmwareVersion(19, 6, 1, null, 0), + TagName = "19.6.1", + IsPreRelease = false + }; + var device = new FakeLanChipInfoStreamingDevice("COM13", chipInfo: new LanChipInfo + { + ChipId = 1234, + FwVersion = "19.5.4", + BuildDate = "Jan 8 2019" + }); + + var externalProcessRunner = new FakeExternalProcessRunner + { + NextResult = new ExternalProcessResult(0, false, TimeSpan.FromMilliseconds(10), [], []) + }; + + var options = CreateFastOptions(); + options.PostLanFirmwareModeDelay = TimeSpan.FromMilliseconds(5); + options.PostWifiReconnectDelay = TimeSpan.FromMilliseconds(5); + + var service = new FirmwareUpdateService( + new FakeHidTransport(), + new FakeFirmwareDownloadService { LatestWifiRelease = wifiRelease }, + externalProcessRunner, + NullLogger.Instance, + new FakeBootloaderProtocol([[0x10]]), + new FakeHidDeviceEnumerator([]), + options); + + var firmwareDir = CreateTempDirectory(); + File.WriteAllText(Path.Combine(firmwareDir, "winc_flash_tool.cmd"), "@echo off"); + + WifiFirmwareStatus? reentrantStatus = null; + var reentryAttempted = false; + + // Progress handler that re-enters CheckWifiFirmwareStatusAsync + // exactly once (on the first event) — mirrors a UI consumer + // that might want to refresh status display when an update + // transitions state. + var progress = new SyncProgress(_ => + { + if (reentryAttempted) + { + return; + } + reentryAttempted = true; + reentrantStatus = service.CheckWifiFirmwareStatusAsync(device).GetAwaiter().GetResult(); + }); + + try + { + await service.UpdateWifiModuleAsync(device, firmwareDir, progress); + } + finally + { + Directory.Delete(firmwareDir, recursive: true); + } + + Assert.True(reentryAttempted, "Progress callback never fired — test setup wrong."); + Assert.NotNull(reentrantStatus); + Assert.Equal(FirmwareUpdateState.Complete, service.CurrentState); + } + + private sealed class SyncProgress : IProgress + { + private readonly Action _handler; + public SyncProgress(Action handler) => _handler = handler; + public void Report(T value) => _handler(value); + } + private static FirmwareUpdateServiceOptions CreateFastOptions() { return new FirmwareUpdateServiceOptions diff --git a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs index 73fef46..a248d36 100644 --- a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs +++ b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs @@ -75,6 +75,16 @@ private static readonly IReadOnlyDictionary _isInsideOperation = new(); private readonly IHidTransport _hidTransport; private readonly IExternalProcessRunner _externalProcessRunner; private readonly ILogger _logger; @@ -196,13 +206,28 @@ public async Task CheckWifiFirmwareStatusAsync( // Acquired without the RunExclusiveAsync state check — a status probe // is read-only and should be available to UI even when an update is // in flight (it just waits for the in-flight I/O to release the lock). + // + // Reentrancy guard: an update fires progress / state-change callbacks + // synchronously while it holds _operationLock. If a callback handler + // calls back into CheckWifiFirmwareStatusAsync, WaitAsync would + // deadlock waiting for the same lock the caller's flow already owns + // (SemaphoreSlim is not re-entrant). The AsyncLocal flag detects + // this case and skips the second acquisition; we're already in a + // serialized device-I/O context. + if (_isInsideOperation.Value) + { + return await CheckWifiFirmwareStatusCoreAsync(device, cancellationToken).ConfigureAwait(false); + } + await _operationLock.WaitAsync(cancellationToken).ConfigureAwait(false); + _isInsideOperation.Value = true; try { return await CheckWifiFirmwareStatusCoreAsync(device, cancellationToken).ConfigureAwait(false); } finally { + _isInsideOperation.Value = false; _operationLock.Release(); } } @@ -214,6 +239,7 @@ private async Task RunExclusiveAsync( ThrowIfDisposed(); await _operationLock.WaitAsync(cancellationToken).ConfigureAwait(false); + _isInsideOperation.Value = true; try { ResetIfTerminalState(); @@ -231,6 +257,7 @@ private async Task RunExclusiveAsync( } finally { + _isInsideOperation.Value = false; _operationLock.Release(); } } From df91d5cfddaf5ddbb28376339fb4e81b7ba3f638 Mon Sep 17 00:00:00 2001 From: Christopher Lange Date: Mon, 11 May 2026 23:05:02 -0600 Subject: [PATCH 6/7] feat(firmware): retry LAN chip-info probe during startup transients (closes #144) (#199) --- .../Firmware/FirmwareUpdateServiceTests.cs | 207 +++++++++++++++++- .../Firmware/FirmwareUpdateService.cs | 120 ++++++++-- .../Firmware/FirmwareUpdateServiceOptions.cs | 48 ++++ 3 files changed, 359 insertions(+), 16 deletions(-) diff --git a/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs b/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs index 549a52e..3eb0948 100644 --- a/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs +++ b/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs @@ -730,6 +730,197 @@ public async Task CheckWifiFirmwareStatusAsync_ReentrantCallFromUpdateProgressCa Assert.Equal(FirmwareUpdateState.Complete, service.CurrentState); } + [Fact] + public async Task CheckWifiFirmwareStatusAsync_WhenLanChipInfoFailsTransiently_RetriesUntilSuccess() + { + // Closes #144: post-PIC32-reboot the WiFi subsystem can lag the + // application by a few seconds, so the first chip-info query + // transiently fails. Without retry, the WiFi version decision + // would short-circuit and trigger an unnecessary multi-minute + // reflash. The retry budget covers the startup window. + var wifiRelease = new FirmwareReleaseInfo + { + Version = new FirmwareVersion(19, 5, 4, null, 0), + TagName = "19.5.4", + IsPreRelease = false + }; + var device = new FakeLanChipInfoStreamingDevice( + "COM14", + chipInfo: new LanChipInfo + { + ChipId = 1234, + FwVersion = "19.5.4", + BuildDate = "Jan 8 2019" + }, + transientFailuresBeforeSuccess: 2); + + var options = CreateFastOptions(); + options.LanChipInfoMaxAttempts = 3; + options.LanChipInfoRetryDelay = TimeSpan.FromMilliseconds(5); // Keep test fast + + var service = new FirmwareUpdateService( + new FakeHidTransport(), + new FakeFirmwareDownloadService { LatestWifiRelease = wifiRelease }, + new FakeExternalProcessRunner(), + NullLogger.Instance, + new FakeBootloaderProtocol([[0x10]]), + new FakeHidDeviceEnumerator([]), + options); + + var status = await service.CheckWifiFirmwareStatusAsync(device); + + Assert.Equal(3, device.GetLanChipInfoCallCount); + Assert.True(status.IsUpToDate); + Assert.Equal(WifiFirmwareStatusReason.UpToDate, status.Reason); + } + + [Fact] + public async Task CheckWifiFirmwareStatusAsync_WhenLanChipInfoFailsAllAttempts_ReturnsChipInfoUnavailable() + { + // After exhausting LanChipInfoMaxAttempts the planning method must + // fall through to ChipInfoUnavailable, NOT hang or surface the + // raw exception. This preserves the "couldn't check, default to + // running update" semantics for genuinely-broken devices. + var device = new FakeLanChipInfoStreamingDevice( + "COM15", + chipInfo: new LanChipInfo + { + ChipId = 1234, + FwVersion = "19.5.4", + BuildDate = "Jan 8 2019" + }, + transientFailuresBeforeSuccess: 99); + + var options = CreateFastOptions(); + options.LanChipInfoMaxAttempts = 3; + options.LanChipInfoRetryDelay = TimeSpan.FromMilliseconds(5); + + var service = new FirmwareUpdateService( + new FakeHidTransport(), + new FakeFirmwareDownloadService(), + new FakeExternalProcessRunner(), + NullLogger.Instance, + new FakeBootloaderProtocol([[0x10]]), + new FakeHidDeviceEnumerator([]), + options); + + var status = await service.CheckWifiFirmwareStatusAsync(device); + + Assert.Equal(3, device.GetLanChipInfoCallCount); + Assert.False(status.IsUpToDate); + Assert.Equal(WifiFirmwareStatusReason.ChipInfoUnavailable, status.Reason); + } + + [Fact] + public async Task CheckWifiFirmwareStatusAsync_TotalTimeoutHit_ShortCircuitsToChipInfoUnavailable() + { + // Closes a Qodo follow-up on PR #199: per-attempt query timeouts + // compound with retry delays, so a high MaxAttempts × non-trivial + // per-attempt latency could block far beyond the configured retry + // budget while holding _operationLock. The total-timeout cap caps + // wall-clock time independent of attempt counts. + // + // The fake's per-attempt latency is the Task.Delay below; with + // 200ms latency × 10 max attempts × 100ms retry delay, a naive + // implementation would block ~2.9s. The 100ms total timeout + // forces an early ChipInfoUnavailable return after the first + // attempt's latency exceeds the budget. + var device = new SlowFakeLanChipInfoStreamingDevice( + "COM17", + attemptLatency: TimeSpan.FromMilliseconds(200)); + + var options = CreateFastOptions(); + options.LanChipInfoMaxAttempts = 10; + options.LanChipInfoRetryDelay = TimeSpan.FromMilliseconds(100); + options.LanChipInfoTotalTimeout = TimeSpan.FromMilliseconds(100); + + var service = new FirmwareUpdateService( + new FakeHidTransport(), + new FakeFirmwareDownloadService(), + new FakeExternalProcessRunner(), + NullLogger.Instance, + new FakeBootloaderProtocol([[0x10]]), + new FakeHidDeviceEnumerator([]), + options); + + var stopwatch = System.Diagnostics.Stopwatch.StartNew(); + var status = await service.CheckWifiFirmwareStatusAsync(device); + stopwatch.Stop(); + + Assert.False(status.IsUpToDate); + Assert.Equal(WifiFirmwareStatusReason.ChipInfoUnavailable, status.Reason); + // Should bail well before attempting all 10 × (200ms + 100ms) = 3s. + // Allowing 1500ms for CI variance / xunit overhead. + Assert.True(stopwatch.Elapsed < TimeSpan.FromMilliseconds(1500), + $"Probe took {stopwatch.ElapsedMilliseconds}ms, expected <1500ms with TotalTimeout=100ms."); + } + + private sealed class SlowFakeLanChipInfoStreamingDevice : IStreamingDevice, ILanChipInfoProvider + { + private readonly TimeSpan _attemptLatency; + public SlowFakeLanChipInfoStreamingDevice(string name, TimeSpan attemptLatency) + { + Name = name; + _attemptLatency = attemptLatency; + IsConnected = true; + } + public string Name { get; } + public IPAddress? IpAddress => null; + public bool IsConnected { get; private set; } + public ConnectionStatus Status => ConnectionStatus.Connected; + public int StreamingFrequency { get; set; } + public bool IsStreaming { get; private set; } + public event EventHandler? StatusChanged { add { } remove { } } + public event EventHandler? MessageReceived { add { } remove { } } + public void Connect() => IsConnected = true; + public void Disconnect() => IsConnected = false; + public void Send(IOutboundMessage message) { } + public void StartStreaming() => IsStreaming = true; + public void StopStreaming() => IsStreaming = false; + public async Task GetLanChipInfoAsync(CancellationToken cancellationToken = default) + { + await Task.Delay(_attemptLatency, cancellationToken).ConfigureAwait(false); + return null; + } + } + + [Fact] + public async Task CheckWifiFirmwareStatusAsync_FirstAttemptSucceeds_DoesNotRetry() + { + // Steady-state path: no retry overhead when the first call works. + var wifiRelease = new FirmwareReleaseInfo + { + Version = new FirmwareVersion(19, 5, 4, null, 0), + TagName = "19.5.4", + IsPreRelease = false + }; + var device = new FakeLanChipInfoStreamingDevice( + "COM16", + chipInfo: new LanChipInfo + { + ChipId = 1234, + FwVersion = "19.5.4", + BuildDate = "Jan 8 2019" + }); + + var options = CreateFastOptions(); + options.LanChipInfoMaxAttempts = 3; + options.LanChipInfoRetryDelay = TimeSpan.FromMilliseconds(5); + + var service = new FirmwareUpdateService( + new FakeHidTransport(), + new FakeFirmwareDownloadService { LatestWifiRelease = wifiRelease }, + new FakeExternalProcessRunner(), + NullLogger.Instance, + new FakeBootloaderProtocol([[0x10]]), + new FakeHidDeviceEnumerator([]), + options); + + await service.CheckWifiFirmwareStatusAsync(device); + + Assert.Equal(1, device.GetLanChipInfoCallCount); + } + private sealed class SyncProgress : IProgress { private readonly Action _handler; @@ -848,14 +1039,18 @@ private sealed class FakeLanChipInfoStreamingDevice : IStreamingDevice, ILanChip { private readonly LanChipInfo? _chipInfo; private ConnectionStatus _status = ConnectionStatus.Connected; + private int _remainingTransientFailures; - public FakeLanChipInfoStreamingDevice(string name, LanChipInfo? chipInfo) + public FakeLanChipInfoStreamingDevice(string name, LanChipInfo? chipInfo, int transientFailuresBeforeSuccess = 0) { Name = name; _chipInfo = chipInfo; + _remainingTransientFailures = transientFailuresBeforeSuccess; IsConnected = true; } + public int GetLanChipInfoCallCount { get; private set; } + public string Name { get; } public IPAddress? IpAddress => null; public bool IsConnected { get; private set; } @@ -899,6 +1094,16 @@ public void Send(IOutboundMessage message) public Task GetLanChipInfoAsync(CancellationToken cancellationToken = default) { + GetLanChipInfoCallCount++; + if (_remainingTransientFailures > 0) + { + _remainingTransientFailures--; + // Faulted task (not sync throw) more accurately simulates how + // a real async method surfaces failure — caller's await sees a + // genuinely-async exception path. + return Task.FromException( + new InvalidOperationException("Simulated transient post-reboot failure.")); + } return Task.FromResult(_chipInfo); } } diff --git a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs index a248d36..41c2ff3 100644 --- a/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs +++ b/src/Daqifi.Core/Firmware/FirmwareUpdateService.cs @@ -536,21 +536,15 @@ private async Task CheckWifiFirmwareStatusCoreAsync( }; } - LanChipInfo? chipInfo; - try - { - chipInfo = await lanChipInfoProvider.GetLanChipInfoAsync(cancellationToken).ConfigureAwait(false); - } - catch (Exception ex) when (ex is not OperationCanceledException) - { - _logger.LogDebug(ex, "Failed to query LAN chip info; reporting status as ChipInfoUnavailable."); - return new WifiFirmwareStatus - { - IsUpToDate = false, - Reason = WifiFirmwareStatusReason.ChipInfoUnavailable, - }; - } - + // Bounded retry for the LAN chip-info probe (closes #144). Right + // after a PIC32 firmware update the application is up while WiFi + // is still finishing startup, so the first chip-info query can + // transiently fail; without retry, the WiFi version decision + // would short-circuit to ChipInfoUnavailable and flow on to a + // multi-minute reflash of already-current WiFi firmware. The + // retry budget is bounded (LanChipInfoMaxAttempts × RetryDelay) + // and observes cancellation between attempts. + var chipInfo = await TryGetLanChipInfoWithRetryAsync(lanChipInfoProvider, cancellationToken).ConfigureAwait(false); if (chipInfo == null) { return new WifiFirmwareStatus @@ -613,6 +607,102 @@ private async Task CheckWifiFirmwareStatusCoreAsync( }; } + private async Task TryGetLanChipInfoWithRetryAsync( + ILanChipInfoProvider lanChipInfoProvider, + CancellationToken cancellationToken) + { + var maxAttempts = Math.Max(1, _options.LanChipInfoMaxAttempts); + var retryDelay = _options.LanChipInfoRetryDelay; + var totalTimeout = _options.LanChipInfoTotalTimeout; + + // Wall-clock budget guards against the pathological case where + // attempt-count × per-attempt-timeout + retry-delay sum vastly + // exceeds the configured retry budget (e.g., 3 × 2s device timeout + // + 2 × 2s delay = ~10s while _operationLock is held). Linking + // the caller's CT preserves cancellation semantics; the timeout + // CTS just adds a deadline. + using var timeoutCts = new CancellationTokenSource(totalTimeout); + using var linkedCts = CancellationTokenSource.CreateLinkedTokenSource( + cancellationToken, timeoutCts.Token); + var linkedToken = linkedCts.Token; + + for (var attempt = 1; attempt <= maxAttempts; attempt++) + { + try + { + linkedToken.ThrowIfCancellationRequested(); + } + catch (OperationCanceledException) when (timeoutCts.IsCancellationRequested && !cancellationToken.IsCancellationRequested) + { + _logger.LogDebug( + "LAN chip-info probe hit total timeout ({Timeout}) before attempt {Attempt}/{Max}.", + totalTimeout, + attempt, + maxAttempts); + return null; + } + + try + { + var chipInfo = await lanChipInfoProvider.GetLanChipInfoAsync(linkedToken).ConfigureAwait(false); + if (chipInfo != null) + { + if (attempt > 1) + { + _logger.LogDebug( + "LAN chip-info query succeeded on attempt {Attempt}/{Max}.", + attempt, + maxAttempts); + } + return chipInfo; + } + _logger.LogDebug( + "LAN chip-info query returned null on attempt {Attempt}/{Max}.", + attempt, + maxAttempts); + } + catch (OperationCanceledException) when (timeoutCts.IsCancellationRequested && !cancellationToken.IsCancellationRequested) + { + _logger.LogDebug( + "LAN chip-info probe hit total timeout ({Timeout}) during attempt {Attempt}/{Max}.", + totalTimeout, + attempt, + maxAttempts); + return null; + } + catch (Exception ex) when (ex is not OperationCanceledException) + { + _logger.LogDebug( + ex, + "LAN chip-info query failed on attempt {Attempt}/{Max}.", + attempt, + maxAttempts); + } + + if (attempt < maxAttempts) + { + try + { + await Task.Delay(retryDelay, linkedToken).ConfigureAwait(false); + } + catch (OperationCanceledException) when (timeoutCts.IsCancellationRequested && !cancellationToken.IsCancellationRequested) + { + _logger.LogDebug( + "LAN chip-info probe hit total timeout ({Timeout}) during retry delay after attempt {Attempt}/{Max}.", + totalTimeout, + attempt, + maxAttempts); + return null; + } + } + } + + _logger.LogDebug( + "LAN chip-info query exhausted {Max} attempts; reporting status as ChipInfoUnavailable.", + maxAttempts); + return null; + } + private ExternalProcessRequest BuildWifiProcessRequest( IStreamingDevice device, string firmwarePath, diff --git a/src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.cs b/src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.cs index 4b75984..8d920b4 100644 --- a/src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.cs +++ b/src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.cs @@ -119,6 +119,43 @@ public sealed class FirmwareUpdateServiceOptions /// public string? WifiPortOverride { get; set; } + /// + /// Total attempts (initial + retries) for LAN chip-info queries before + /// the WiFi version decision falls through to "couldn't check, proceed + /// with flash". Right after a PIC32 firmware update the application is + /// typically up while the WiFi subsystem is still finishing startup, so + /// the first chip-info query can transiently fail; bounded retry covers + /// that window so callers don't unnecessarily reflash up-to-date WiFi + /// firmware (closes #144). + /// + /// + /// Each attempt also incurs the per-attempt timeout from the device + /// implementation (e.g., DaqifiStreamingDevice.GetLanChipInfoAsync + /// uses 2s). Total wall-clock budget is therefore + /// sum(attempt durations) + (MaxAttempts-1) * RetryDelay; with the + /// 2s device default, 3 attempts and 2s delay sum to ~10s in the worst + /// case. The retry loop holds _operationLock, so use + /// to cap the actual wall-clock + /// time independent of attempt counts. + /// + public int LanChipInfoMaxAttempts { get; set; } = 3; + + /// + /// Delay between LAN chip-info retry attempts (cancellation-aware). + /// + public TimeSpan LanChipInfoRetryDelay { get; set; } = TimeSpan.FromSeconds(2); + + /// + /// Hard upper bound on wall-clock time spent in the LAN chip-info + /// probe (including per-attempt query timeouts and retry delays). + /// When exceeded, the loop short-circuits to ChipInfoUnavailable + /// regardless of remaining attempts. Prevents pathological multi- + /// attempt cases (e.g., 3 attempts × 2s device timeout + 2 × 2s delays + /// = ~10s) from stalling firmware flows or UI status probes that hold + /// the operation lock. + /// + public TimeSpan LanChipInfoTotalTimeout { get; set; } = TimeSpan.FromSeconds(8); + /// /// Gets the configured timeout for a given firmware update state. /// @@ -176,6 +213,17 @@ public void Validate() "Flash write retry count must be at least 1."); } + if (LanChipInfoMaxAttempts < 1) + { + throw new ArgumentOutOfRangeException( + nameof(LanChipInfoMaxAttempts), + LanChipInfoMaxAttempts, + "LAN chip-info max attempts must be at least 1."); + } + + ValidatePositive(LanChipInfoRetryDelay, nameof(LanChipInfoRetryDelay)); + ValidatePositive(LanChipInfoTotalTimeout, nameof(LanChipInfoTotalTimeout)); + if (BootloaderVendorId < 0 || BootloaderVendorId > 0xFFFF) { throw new ArgumentOutOfRangeException( From 2496323a230795eeb5aaf8270d9cf96dd04a0956 Mon Sep 17 00:00:00 2001 From: Chris Lange Date: Tue, 12 May 2026 13:16:54 -0600 Subject: [PATCH 7/7] Apply Qodo /agentic_review pass 4: hard timeout on deadlock test Wrap the reentrant-callback regression test in WaitAsync(30s) so a regression of the AsyncLocal re-entrancy guard fails fast with a clear "deadlocked" message instead of stalling the xunit run waiting for the global per-test budget. --- .../Firmware/FirmwareUpdateServiceTests.cs | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs b/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs index 3eb0948..be848e5 100644 --- a/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs +++ b/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs @@ -716,9 +716,28 @@ public async Task CheckWifiFirmwareStatusAsync_ReentrantCallFromUpdateProgressCa reentrantStatus = service.CheckWifiFirmwareStatusAsync(device).GetAwaiter().GetResult(); }); + // Hard timeout via WaitAsync so a regression of the deadlock fails + // fast instead of stalling the whole xunit run waiting for the + // global per-test budget. The fast-options + ms-scale delays mean + // the happy path completes in well under a second; 30s is plenty + // of margin under heavy CI load while still being a clear "stuck". try { - await service.UpdateWifiModuleAsync(device, firmwareDir, progress); + using var timeoutCts = new CancellationTokenSource(TimeSpan.FromSeconds(30)); + await service.UpdateWifiModuleAsync(device, firmwareDir, progress) + .WaitAsync(timeoutCts.Token); + } + catch (OperationCanceledException) when (!reentryAttempted) + { + throw new Xunit.Sdk.XunitException( + "Test setup failed before reentrancy was attempted."); + } + catch (OperationCanceledException) + { + throw new Xunit.Sdk.XunitException( + "UpdateWifiModuleAsync did not complete within 30s — the " + + "AsyncLocal re-entrancy guard likely regressed and the " + + "inner CheckWifiFirmwareStatusAsync call is deadlocked."); } finally {