From a254d321aa6aafc6086ecc764ecdf187734fb07c Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Wed, 29 Jul 2026 10:41:17 -0600 Subject: [PATCH 1/2] fix(sdcard): run the SD bus switch as a prepare phase inside the exchange lock Hoisting PrepareSdInterface() above ExecuteTextCommandAsync in a82c3af closed a stale-terminator window but opened an interface-interleaving one: the SPI switch had been running inside _textExchangeLock, and outside it a competing text exchange can restore the LAN interface between the switch and the LIST, leaving the listing to run against the wrong interface. Both properties are wanted, so split the setup rather than choose. A new ExecuteTextCommandWithPrepareAsync seam runs a prepare phase inside the lock and before the consumer swap, so: - no other exchange can interleave between the switch and the sends, and - the settle wait sits ahead of the stale-line boundary, so the setup action is still gap-free. Both listing and delete call sites share PrepareSdInterfaceAndSettleAsync. Bench (Nyquist 1, fw 3.7.2, USB): 31 files on repeated listings, SD storage and a 10 Hz two-channel stream unaffected. Co-Authored-By: Claude Opus 5 --- .../Device/DaqifiDeviceStaleTextLineTests.cs | 52 ++++++++++++++- .../Device/SdCard/SdCardOperationsTests.cs | 42 ++++++++++++ src/Daqifi.Core/Device/DaqifiDevice.cs | 61 +++++++++++++++++ .../Device/DaqifiStreamingDevice.cs | 66 +++++++++++-------- 4 files changed, 192 insertions(+), 29 deletions(-) diff --git a/src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs b/src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs index 895fccc4..2f0202f1 100644 --- a/src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs +++ b/src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs @@ -55,7 +55,49 @@ public async Task ExecuteTextCommand_KeepsLinesThatArriveAfterTheExchangeSentSom device.Disconnect(); } - /// Exposes the protected text-exchange entry point. + [Fact] + public async Task ExecuteTextCommandWithPrepare_RunsPrepareBeforeTheSetupAction() + { + using var transport = new ReleaseOnStreamAccessMockTransport("0,\"No error\"\r\n"); + using var device = new StaleLineTestableDevice("Prepared Device", transport); + + device.Connect(); + + var order = new List(); + await device.CallWithPrepareAsync( + _ => { order.Add("prepare"); return Task.CompletedTask; }, + () => order.Add("setup")); + + Assert.Equal(new[] { "prepare", "setup" }, order); + + device.Disconnect(); + } + + [Fact] + public async Task ExecuteTextCommandWithPrepare_RunsPrepareInsideTheExchange() + { + // The property that matters for the SD card operations: the prepare phase holds the + // device-wide text-exchange lock, so no competing exchange can interleave between the SPI + // bus switch it performs and the commands that depend on it. Asserted through the + // exchange's own re-entrancy guard rather than by racing two threads — if prepare runs + // inside the critical section, a nested exchange must be refused, and if it had been + // hoisted back outside the lock this would silently succeed instead. + using var transport = new ReleaseOnStreamAccessMockTransport("0,\"No error\"\r\n"); + using var device = new StaleLineTestableDevice("Nested Device", transport); + + device.Connect(); + + var ex = await Assert.ThrowsAsync( + () => device.CallWithPrepareAsync( + async _ => await device.CallExecuteTextCommandAsync(() => { }), + () => { })); + + Assert.Contains("not re-entrant", ex.Message); + + device.Disconnect(); + } + + /// Exposes the protected text-exchange entry points. private class StaleLineTestableDevice : DaqifiDevice { public StaleLineTestableDevice(string name, IStreamTransport transport) @@ -67,6 +109,14 @@ public Task> CallExecuteTextCommandAsync(Action setupActio { return ExecuteTextCommandAsync(setupAction, responseTimeoutMs: 500, completionTimeoutMs: 150); } + + public Task> CallWithPrepareAsync( + Func prepareAsync, + Action setupAction) + { + return ExecuteTextCommandWithPrepareAsync( + prepareAsync, setupAction, responseTimeoutMs: 500, completionTimeoutMs: 150); + } } /// diff --git a/src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs b/src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs index 17fab924..35ade026 100644 --- a/src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs +++ b/src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs @@ -1982,6 +1982,22 @@ protected override Task> ExecuteTextCommandAsync( response, SentMessages, sentBefore, ExecuteTextCommandCallCount, UnterminatedAttempts)); } + protected override async Task> ExecuteTextCommandWithPrepareAsync( + Func prepareAsync, + Action setupAction, + int responseTimeoutMs = 1000, + int completionTimeoutMs = 250, + CancellationToken cancellationToken = default) + { + // The prepare phase precedes the exchange proper, so its commands are outside the + // window whose replies belong to this response — mirroring the real device, where + // it runs before the consumer swap. + await prepareAsync(cancellationToken).ConfigureAwait(false); + + return await ExecuteTextCommandAsync( + setupAction, responseTimeoutMs, completionTimeoutMs, cancellationToken).ConfigureAwait(false); + } + protected override async Task> ExecuteTextCommandAsync( Func setupActionAsync, int responseTimeoutMs = 1000, @@ -2101,6 +2117,19 @@ protected override Task> ExecuteTextCommandAsync( CannedTextResponse, SentMessages, sentBefore, _executeTextCommandCallCount, UnterminatedAttempts)); } + protected override async Task> ExecuteTextCommandWithPrepareAsync( + Func prepareAsync, + Action setupAction, + int responseTimeoutMs = 1000, + int completionTimeoutMs = 250, + CancellationToken cancellationToken = default) + { + await prepareAsync(cancellationToken).ConfigureAwait(false); + + return await ExecuteTextCommandAsync( + setupAction, responseTimeoutMs, completionTimeoutMs, cancellationToken).ConfigureAwait(false); + } + protected override async Task> ExecuteTextCommandAsync( Func setupActionAsync, int responseTimeoutMs = 1000, @@ -2368,6 +2397,19 @@ protected override Task> ExecuteTextCommandAsync( CannedTextResponse, SentMessages, sentBefore, attemptNumber: 1, unterminatedAttempts: 0)); } + protected override async Task> ExecuteTextCommandWithPrepareAsync( + Func prepareAsync, + Action setupAction, + int responseTimeoutMs = 1000, + int completionTimeoutMs = 250, + CancellationToken cancellationToken = default) + { + await prepareAsync(cancellationToken).ConfigureAwait(false); + + return await ExecuteTextCommandAsync( + setupAction, responseTimeoutMs, completionTimeoutMs, cancellationToken).ConfigureAwait(false); + } + protected override async Task> ExecuteTextCommandAsync( Func setupActionAsync, int responseTimeoutMs = 1000, diff --git a/src/Daqifi.Core/Device/DaqifiDevice.cs b/src/Daqifi.Core/Device/DaqifiDevice.cs index 13be9768..39e12f9e 100644 --- a/src/Daqifi.Core/Device/DaqifiDevice.cs +++ b/src/Daqifi.Core/Device/DaqifiDevice.cs @@ -752,6 +752,52 @@ protected virtual Task> ExecuteTextCommandAsync( CancellationToken cancellationToken = default) { return ExecuteTextCommandCoreAsync( + prepareAsync: null, + _ => { setupAction(); return Task.CompletedTask; }, + responseTimeoutMs, + completionTimeoutMs, + cancellationToken); + } + + /// + /// Variant of that + /// first runs a phase, for callers that must put the device + /// into a particular state before the commands they are about to send will work. + /// + /// + /// + /// The prepare phase runs inside the device-wide text-exchange lock, so no other text + /// exchange can interleave between it and . That is the whole + /// point of the seam: the SD card operations switch the shared SPI bus over to the card and + /// wait for the firmware to settle, and a competing exchange restoring the LAN interface in + /// that gap would leave their commands running against the wrong interface. + /// + /// + /// It also runs BEFORE the consumer swap and therefore before the stale-line boundary is + /// taken, which is the second reason the phases are split: the settle wait a prepare phase + /// needs would, if it sat inside , widen that boundary into a + /// window where a late reply to an earlier command could be captured as part of this + /// response (#396). Anything the device emits in reply to the prepare phase is consumed by + /// the protobuf consumer, exactly as it was before this exchange began. + /// + /// + /// An async function that puts the device into the required state. Receives the operation's cancellation token. + /// An action that sends SCPI commands to the device while the text consumer is active. + /// The time in milliseconds to wait for the first text response after sending commands. + /// The time in milliseconds of inactivity after the first response before considering the response complete. Defaults to 250ms. + /// A cancellation token to observe while waiting for the task to complete. + /// A list of text lines received from the device, excluding anything already in flight when the exchange opened. + /// Thrown when the device is not connected or has no transport. + /// Thrown when the operation is canceled. + protected virtual Task> ExecuteTextCommandWithPrepareAsync( + Func prepareAsync, + Action setupAction, + int responseTimeoutMs = 1000, + int completionTimeoutMs = 250, + CancellationToken cancellationToken = default) + { + return ExecuteTextCommandCoreAsync( + prepareAsync, _ => { setupAction(); return Task.CompletedTask; }, responseTimeoutMs, completionTimeoutMs, @@ -778,6 +824,7 @@ protected virtual Task> ExecuteTextCommandAsync( CancellationToken cancellationToken = default) { return ExecuteTextCommandCoreAsync( + prepareAsync: null, setupActionAsync, responseTimeoutMs, completionTimeoutMs, @@ -785,6 +832,7 @@ protected virtual Task> ExecuteTextCommandAsync( } private async Task> ExecuteTextCommandCoreAsync( + Func? prepareAsync, Func setupActionAsync, int responseTimeoutMs, int completionTimeoutMs, @@ -863,6 +911,19 @@ private async Task> ExecuteTextCommandCoreAsync( } var sw = Stopwatch.StartNew(); + + // Prepare phase, if any. Deliberately here: inside the lock, so no competing text + // exchange can interleave between it and the setup action below and undo the state + // it establishes; and before the consumer swap, so the wait it typically needs + // cannot widen the stale-line boundary taken further down. Any device output it + // provokes goes to the protobuf consumer, which is still running at this point. + if (prepareAsync != null) + { + await prepareAsync(cancellationToken).ConfigureAwait(false); + + SafeLog(() => _logger.LogDebug("[ExecuteTextCommandAsync] Prepare phase completed at {ElapsedMs}ms", sw.ElapsedMilliseconds)); + } + var collectedLines = new List(); var stream = _transport.Stream; int? originalReadTimeout = null; diff --git a/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs b/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs index 625386fa..741114cc 100644 --- a/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs +++ b/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs @@ -1661,20 +1661,15 @@ public async Task> GetSdCardFilesAsync(Cancellatio await Task.Delay(SD_INTERFACE_SETTLE_DELAY_MS, cancellationToken).ConfigureAwait(false); } - // Switch the shared SPI bus over to the SD card and let the firmware settle - // BEFORE opening the text exchange. Querying the card too soon after the switch - // makes the device answer -200 (Execution error), so the delay itself is not - // optional — but running it outside the exchange leaves the exchange with no - // internal gap at all, so its very first act is the LIST query. That matters for - // the terminator: the exchange discards anything received before its setup - // action sends, and a gap inside the action would widen that boundary into a - // window where a late reply to an earlier command could still be mistaken for - // this listing's terminator. The delay is unchanged from the device's point of - // view — if anything longer, since the consumer swap now follows it. - PrepareSdInterface(); - await Task.Delay(SD_INTERFACE_SETTLE_DELAY_MS, cancellationToken).ConfigureAwait(false); - - lines = await ExecuteTextCommandAsync( + // The SPI bus switch and its settle wait run as the exchange's prepare phase: + // inside the exchange lock, so a competing text exchange cannot restore the LAN + // interface between the switch and the LIST, and ahead of the stale-line + // boundary, so the settle wait does not become a window in which a late reply to + // an earlier command could pass for this listing's terminator. Querying the card + // too soon after the switch makes the device answer -200 (Execution error), so + // the wait itself is not optional. + lines = await ExecuteTextCommandWithPrepareAsync( + PrepareSdInterfaceAndSettleAsync, () => { Send(ScpiMessageProducer.GetSdFileList); @@ -1716,6 +1711,26 @@ public async Task> GetSdCardFilesAsync(Cancellatio return files; } + /// + /// Prepare phase shared by the SD card text exchanges: switches the shared SPI bus over to + /// the card and waits for the firmware to complete the switch. + /// + /// + /// Passed to rather than run + /// inline, so it executes inside the text-exchange lock — a competing exchange restoring the + /// LAN interface between the switch and the commands that depend on it would leave them + /// running against the wrong interface — and ahead of the exchange's stale-line boundary, so + /// the settle wait cannot be mistaken for a window in which the device was answering. + /// + private async Task PrepareSdInterfaceAndSettleAsync(CancellationToken cancellationToken) + { + PrepareSdInterface(); + + // Querying the card too soon after the switch makes the device answer -200 + // (Execution error), so this wait is not optional. + await Task.Delay(SD_INTERFACE_SETTLE_DELAY_MS, cancellationToken).ConfigureAwait(false); + } + /// /// Splits a raw SD listing response at the SYSTem:ERRor? terminator reply that /// appends to the exchange. @@ -2122,16 +2137,13 @@ public async Task DeleteSdCardFileAsync(string fileName, CancellationToken cance IReadOnlyList lines; try { - // Switch the shared SPI bus to the SD card and settle BEFORE opening the text - // exchange, for the same reason as GetSdCardFilesAsync: a gap inside the setup - // action is a window in which a late reply to an earlier command can be captured - // as part of this response. Here that would mean a stale error line triggering a - // pointless delete-and-relist retry rather than a bad listing, but it is the same - // defect, so it gets the same treatment. - PrepareSdInterface(); - await Task.Delay(SD_INTERFACE_SETTLE_DELAY_MS, cancellationToken).ConfigureAwait(false); - - lines = await ExecuteTextCommandAsync( + // Same prepare-phase treatment as GetSdCardFilesAsync, for the same two reasons — + // the SPI switch stays serialized against competing text exchanges, and its settle + // wait stays outside the stale-line boundary. The consequence of a stale line is + // milder here (delete keys off ContainsScpiError, so it would mean a pointless + // delete-and-relist retry rather than a bad listing) but it is the same defect. + lines = await ExecuteTextCommandWithPrepareAsync( + PrepareSdInterfaceAndSettleAsync, () => { Send(ScpiMessageProducer.DeleteSdFile(fileName)); @@ -2148,10 +2160,8 @@ public async Task DeleteSdCardFileAsync(string fileName, CancellationToken cance await Task.Delay(SD_INTERFACE_SETTLE_DELAY_MS, cancellationToken).ConfigureAwait(false); - PrepareSdInterface(); - await Task.Delay(SD_INTERFACE_SETTLE_DELAY_MS, cancellationToken).ConfigureAwait(false); - - lines = await ExecuteTextCommandAsync( + lines = await ExecuteTextCommandWithPrepareAsync( + PrepareSdInterfaceAndSettleAsync, () => { Send(ScpiMessageProducer.DeleteSdFile(fileName)); From a002460ba4f3cac64c8acc99cb327316f5ff356c Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Wed, 29 Jul 2026 11:03:29 -0600 Subject: [PATCH 2/2] fix(sdcard): collapse the prepare seam into the existing virtual The parallel ExecuteTextCommandWithPrepareAsync bypassed subclass overrides. It called the core directly, so a subclass overriding ExecuteTextCommandAsync silently stopped intercepting SD LIST and DELETE -- no compile error, no runtime signal, just an instrumented device or test double quietly missing SD traffic. The three new overrides the test fakes needed were the tell. Prepare is now an optional parameter on the existing virtual, so overrides catch every SD operation again. Placed after cancellationToken with CA1068 suppressed, matching IFirmwareUpdateService. Overriders must widen their signature -- a compile error, which is the point: loud beats silent, and it is the same defect class this series has been retiring. The lock ordering is unchanged: prepare still runs inside _textExchangeLock and ahead of the stale-line boundary, both of which live in the core. Adds a test that fails if the seam ever splits in two again. Co-Authored-By: Claude Opus 5 --- .../DaqifiDeviceDrainErrorQueueTests.cs | 14 ++- .../Device/DaqifiDeviceInitializeTests.cs | 37 ++++-- .../Device/DaqifiDeviceStaleTextLineTests.cs | 64 +++++++++- .../Diagnostics/DeviceDiagnosticsTests.cs | 14 ++- .../Device/GetLanChipInfoAsyncTests.cs | 14 ++- .../Device/SdCard/SdCardOperationsTests.cs | 118 +++++++++--------- src/Daqifi.Core/Device/DaqifiDevice.cs | 78 +++++------- .../Device/DaqifiStreamingDevice.cs | 22 ++-- 8 files changed, 219 insertions(+), 142 deletions(-) diff --git a/src/Daqifi.Core.Tests/Device/DaqifiDeviceDrainErrorQueueTests.cs b/src/Daqifi.Core.Tests/Device/DaqifiDeviceDrainErrorQueueTests.cs index fb3ab072..847fea4f 100644 --- a/src/Daqifi.Core.Tests/Device/DaqifiDeviceDrainErrorQueueTests.cs +++ b/src/Daqifi.Core.Tests/Device/DaqifiDeviceDrainErrorQueueTests.cs @@ -203,17 +203,25 @@ public override void Send(IOutboundMessage message) } } - protected override Task> ExecuteTextCommandAsync( + protected override async Task> ExecuteTextCommandAsync( Action setupAction, int responseTimeoutMs = 1000, int completionTimeoutMs = 250, - CancellationToken cancellationToken = default) + CancellationToken cancellationToken = default, + Func? prepareAsync = null) { + // Honor the exchange's prepare phase the way the real device does: it runs first, + // before anything this exchange sends (#396). + if (prepareAsync != null) + { + await prepareAsync(cancellationToken).ConfigureAwait(false); + } + cancellationToken.ThrowIfCancellationRequested(); setupAction(); ExecuteTextCommandCallCount++; var reply = Replies.Count > 0 ? Replies.Dequeue() : Array.Empty(); - return Task.FromResult(reply); + return reply; } } } diff --git a/src/Daqifi.Core.Tests/Device/DaqifiDeviceInitializeTests.cs b/src/Daqifi.Core.Tests/Device/DaqifiDeviceInitializeTests.cs index f56f319e..a29a0deb 100644 --- a/src/Daqifi.Core.Tests/Device/DaqifiDeviceInitializeTests.cs +++ b/src/Daqifi.Core.Tests/Device/DaqifiDeviceInitializeTests.cs @@ -428,23 +428,30 @@ public override void Send(IOutboundMessage message) } } - protected override Task> ExecuteTextCommandAsync( + protected override async Task> ExecuteTextCommandAsync( Action setupAction, int responseTimeoutMs = 1000, int completionTimeoutMs = 250, - CancellationToken cancellationToken = default) + CancellationToken cancellationToken = default, + Func? prepareAsync = null) { + // Honor the exchange's prepare phase the way the real device does: it runs first, + // before anything this exchange sends (#396). + if (prepareAsync != null) + { + await prepareAsync(cancellationToken).ConfigureAwait(false); + } + // Run the setup action so that Send() calls inside it are captured setupAction(); TextCommandAttemptCount++; if (_failFirstAttempt && TextCommandAttemptCount == 1) { - return Task.FromResult>( - new[] { "**ERROR: -200, \"Execution error\"\r\n" }); + return new[] { "**ERROR: -200, \"Execution error\"\r\n" }; } - return Task.FromResult(_textCommandResponse); + return _textCommandResponse; } } @@ -501,12 +508,20 @@ public override void Send(IOutboundMessage message) } } - protected override Task> ExecuteTextCommandAsync( + protected override async Task> ExecuteTextCommandAsync( Action setupAction, int responseTimeoutMs = 1000, int completionTimeoutMs = 250, - CancellationToken cancellationToken = default) + CancellationToken cancellationToken = default, + Func? prepareAsync = null) { + // Honor the exchange's prepare phase the way the real device does: it runs first, + // before anything this exchange sends (#396). + if (prepareAsync != null) + { + await prepareAsync(cancellationToken).ConfigureAwait(false); + } + var before = _sent.Count; setupAction(); var sentThisCall = _sent.Skip(before).ToList(); @@ -519,13 +534,11 @@ protected override Task> ExecuteTextCommandAsync( switch (_usbStepBehavior) { case UsbStepBehavior.ScpiError: - return Task.FromResult>( - new[] { "**ERROR: -200, \"Execution error\"\r\n" }); + return new[] { "**ERROR: -200, \"Execution error\"\r\n" }; case UsbStepBehavior.ScpiErrorThenSucceed: if (UsbStepAttemptCount == 1) { - return Task.FromResult>( - new[] { "**ERROR: -200, \"Execution error\"\r\n" }); + return new[] { "**ERROR: -200, \"Execution error\"\r\n" }; } break; case UsbStepBehavior.Cancel: @@ -533,7 +546,7 @@ protected override Task> ExecuteTextCommandAsync( } } - return Task.FromResult>(Array.Empty()); + return Array.Empty(); } } } diff --git a/src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs b/src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs index 2f0202f1..010a98db 100644 --- a/src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs +++ b/src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs @@ -97,6 +97,63 @@ public async Task ExecuteTextCommandWithPrepare_RunsPrepareInsideTheExchange() device.Disconnect(); } + [Fact] + public async Task ExecuteTextCommand_CarryingAPreparePhase_IsStillCaughtByASubclassOverride() + { + // The prepare phase is a parameter on the existing virtual rather than a second virtual + // method, so a subclass that overrides ExecuteTextCommandAsync keeps intercepting the SD + // operations that use it. A parallel seam would route past such an override with no compile + // error and no runtime signal — an instrumented device or test double would simply stop + // seeing SD traffic. If this ever regresses to a sibling method, this test fails. + using var transport = new ReleaseOnStreamAccessMockTransport("0,\"No error\"\r\n"); + using var device = new InterceptingTestableDevice("Intercepting Device", transport); + + device.Connect(); + + var prepared = false; + var lines = await device.CallWithPrepareAsync( + _ => { prepared = true; return Task.CompletedTask; }, + () => { }); + + Assert.True(device.Intercepted, "The subclass override did not see the call."); + Assert.True(prepared, "The override was handed the prepare phase and ran it."); + Assert.Equal(new[] { "from the override" }, lines); + + device.Disconnect(); + } + + /// + /// Stands in for a downstream subclass or test double that intercepts the text exchange — + /// the case the single-seam design protects. + /// + private sealed class InterceptingTestableDevice : StaleLineTestableDevice + { + public InterceptingTestableDevice(string name, IStreamTransport transport) + : base(name, transport) + { + } + + public bool Intercepted { get; private set; } + + protected override async Task> ExecuteTextCommandAsync( + Action setupAction, + int responseTimeoutMs = 1000, + int completionTimeoutMs = 250, + CancellationToken cancellationToken = default, + Func? prepareAsync = null) + { + Intercepted = true; + + if (prepareAsync != null) + { + await prepareAsync(cancellationToken).ConfigureAwait(false); + } + + setupAction(); + return new List { "from the override" }; + } + } + /// Exposes the protected text-exchange entry points. private class StaleLineTestableDevice : DaqifiDevice { @@ -114,8 +171,11 @@ public Task> CallWithPrepareAsync( Func prepareAsync, Action setupAction) { - return ExecuteTextCommandWithPrepareAsync( - prepareAsync, setupAction, responseTimeoutMs: 500, completionTimeoutMs: 150); + return ExecuteTextCommandAsync( + setupAction, + responseTimeoutMs: 500, + completionTimeoutMs: 150, + prepareAsync: prepareAsync); } } diff --git a/src/Daqifi.Core.Tests/Device/Diagnostics/DeviceDiagnosticsTests.cs b/src/Daqifi.Core.Tests/Device/Diagnostics/DeviceDiagnosticsTests.cs index 714a3322..83533900 100644 --- a/src/Daqifi.Core.Tests/Device/Diagnostics/DeviceDiagnosticsTests.cs +++ b/src/Daqifi.Core.Tests/Device/Diagnostics/DeviceDiagnosticsTests.cs @@ -302,15 +302,23 @@ public override void Send(IOutboundMessage message) } } - protected override Task> ExecuteTextCommandAsync( + protected override async Task> ExecuteTextCommandAsync( Action setupAction, int responseTimeoutMs = 1000, int completionTimeoutMs = 250, - CancellationToken cancellationToken = default) + CancellationToken cancellationToken = default, + Func? prepareAsync = null) { + // Honor the exchange's prepare phase the way the real device does: it runs first, + // before anything this exchange sends (#396). + if (prepareAsync != null) + { + await prepareAsync(cancellationToken).ConfigureAwait(false); + } + cancellationToken.ThrowIfCancellationRequested(); setupAction(); - return Task.FromResult>(CannedTextResponse.ToList()); + return CannedTextResponse.ToList(); } protected override async Task> ExecuteTextCommandAsync( diff --git a/src/Daqifi.Core.Tests/Device/GetLanChipInfoAsyncTests.cs b/src/Daqifi.Core.Tests/Device/GetLanChipInfoAsyncTests.cs index 6d449e09..7fc05269 100644 --- a/src/Daqifi.Core.Tests/Device/GetLanChipInfoAsyncTests.cs +++ b/src/Daqifi.Core.Tests/Device/GetLanChipInfoAsyncTests.cs @@ -90,15 +90,23 @@ public override void Send(IOutboundMessage message) } } - protected override Task> ExecuteTextCommandAsync( + protected override async Task> ExecuteTextCommandAsync( Action setupAction, int responseTimeoutMs = 1000, int completionTimeoutMs = 250, - CancellationToken cancellationToken = default) + CancellationToken cancellationToken = default, + Func? prepareAsync = null) { + // Honor the exchange's prepare phase the way the real device does: it runs first, + // before anything this exchange sends (#396). + if (prepareAsync != null) + { + await prepareAsync(cancellationToken).ConfigureAwait(false); + } + cancellationToken.ThrowIfCancellationRequested(); setupAction(); - return Task.FromResult>(CannedTextResponse.ToList()); + return CannedTextResponse.ToList(); } protected override async Task> ExecuteTextCommandAsync( diff --git a/src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs b/src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs index 35ade026..c307cef0 100644 --- a/src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs +++ b/src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs @@ -1966,36 +1966,28 @@ public override void Send(IOutboundMessage message) } } - protected override Task> ExecuteTextCommandAsync( + protected override async Task> ExecuteTextCommandAsync( Action setupAction, int responseTimeoutMs = 1000, int completionTimeoutMs = 250, - CancellationToken cancellationToken = default) + CancellationToken cancellationToken = default, + Func? prepareAsync = null) { + // Honor the exchange's prepare phase the way the real device does: it runs first, + // before anything this exchange sends (#396). + if (prepareAsync != null) + { + await prepareAsync(cancellationToken).ConfigureAwait(false); + } + var sentBefore = SentMessages.Count; setupAction(); ExecuteTextCommandCallCount++; var response = ResponseSequence.Count > 0 ? ResponseSequence.Dequeue() : new List(); - return Task.FromResult(SdCardTestResponses.AnswerErrorQuery( - response, SentMessages, sentBefore, ExecuteTextCommandCallCount, UnterminatedAttempts)); - } - - protected override async Task> ExecuteTextCommandWithPrepareAsync( - Func prepareAsync, - Action setupAction, - int responseTimeoutMs = 1000, - int completionTimeoutMs = 250, - CancellationToken cancellationToken = default) - { - // The prepare phase precedes the exchange proper, so its commands are outside the - // window whose replies belong to this response — mirroring the real device, where - // it runs before the consumer swap. - await prepareAsync(cancellationToken).ConfigureAwait(false); - - return await ExecuteTextCommandAsync( - setupAction, responseTimeoutMs, completionTimeoutMs, cancellationToken).ConfigureAwait(false); + return SdCardTestResponses.AnswerErrorQuery( + response, SentMessages, sentBefore, ExecuteTextCommandCallCount, UnterminatedAttempts); } protected override async Task> ExecuteTextCommandAsync( @@ -2102,32 +2094,27 @@ public override void Send(IOutboundMessage message) } } - protected override Task> ExecuteTextCommandAsync( + protected override async Task> ExecuteTextCommandAsync( Action setupAction, int responseTimeoutMs = 1000, int completionTimeoutMs = 250, - CancellationToken cancellationToken = default) + CancellationToken cancellationToken = default, + Func? prepareAsync = null) { + // Honor the exchange's prepare phase the way the real device does: it runs first, + // before anything this exchange sends (#396). + if (prepareAsync != null) + { + await prepareAsync(cancellationToken).ConfigureAwait(false); + } + var sentBefore = SentMessages.Count; // Execute the setup action so we can capture the SCPI commands setupAction(); _executeTextCommandCallCount++; - return Task.FromResult(SdCardTestResponses.AnswerErrorQuery( - CannedTextResponse, SentMessages, sentBefore, _executeTextCommandCallCount, UnterminatedAttempts)); - } - - protected override async Task> ExecuteTextCommandWithPrepareAsync( - Func prepareAsync, - Action setupAction, - int responseTimeoutMs = 1000, - int completionTimeoutMs = 250, - CancellationToken cancellationToken = default) - { - await prepareAsync(cancellationToken).ConfigureAwait(false); - - return await ExecuteTextCommandAsync( - setupAction, responseTimeoutMs, completionTimeoutMs, cancellationToken).ConfigureAwait(false); + return SdCardTestResponses.AnswerErrorQuery( + CannedTextResponse, SentMessages, sentBefore, _executeTextCommandCallCount, UnterminatedAttempts); } protected override async Task> ExecuteTextCommandAsync( @@ -2171,14 +2158,22 @@ public override void Send(IOutboundMessage message) } } - protected override Task> ExecuteTextCommandAsync( + protected override async Task> ExecuteTextCommandAsync( Action setupAction, int responseTimeoutMs = 1000, int completionTimeoutMs = 250, - CancellationToken cancellationToken = default) + CancellationToken cancellationToken = default, + Func? prepareAsync = null) { + // Honor the exchange's prepare phase the way the real device does: it runs first, + // before anything this exchange sends (#396). + if (prepareAsync != null) + { + await prepareAsync(cancellationToken).ConfigureAwait(false); + } + setupAction(); - return Task.FromResult>(CannedTextResponse); + return CannedTextResponse; } protected override async Task> ExecuteTextCommandAsync( @@ -2310,14 +2305,22 @@ public override void Send(IOutboundMessage message) } } - protected override Task> ExecuteTextCommandAsync( + protected override async Task> ExecuteTextCommandAsync( Action setupAction, int responseTimeoutMs = 1000, int completionTimeoutMs = 250, - CancellationToken cancellationToken = default) + CancellationToken cancellationToken = default, + Func? prepareAsync = null) { + // Honor the exchange's prepare phase the way the real device does: it runs first, + // before anything this exchange sends (#396). + if (prepareAsync != null) + { + await prepareAsync(cancellationToken).ConfigureAwait(false); + } + setupAction(); - return Task.FromResult>(new List()); + return new List(); } protected override async Task> ExecuteTextCommandAsync( @@ -2385,29 +2388,24 @@ public override void Send(IOutboundMessage message) } } - protected override Task> ExecuteTextCommandAsync( + protected override async Task> ExecuteTextCommandAsync( Action setupAction, int responseTimeoutMs = 1000, int completionTimeoutMs = 250, - CancellationToken cancellationToken = default) + CancellationToken cancellationToken = default, + Func? prepareAsync = null) { + // Honor the exchange's prepare phase the way the real device does: it runs first, + // before anything this exchange sends (#396). + if (prepareAsync != null) + { + await prepareAsync(cancellationToken).ConfigureAwait(false); + } + var sentBefore = SentMessages.Count; setupAction(); - return Task.FromResult(SdCardTestResponses.AnswerErrorQuery( - CannedTextResponse, SentMessages, sentBefore, attemptNumber: 1, unterminatedAttempts: 0)); - } - - protected override async Task> ExecuteTextCommandWithPrepareAsync( - Func prepareAsync, - Action setupAction, - int responseTimeoutMs = 1000, - int completionTimeoutMs = 250, - CancellationToken cancellationToken = default) - { - await prepareAsync(cancellationToken).ConfigureAwait(false); - - return await ExecuteTextCommandAsync( - setupAction, responseTimeoutMs, completionTimeoutMs, cancellationToken).ConfigureAwait(false); + return SdCardTestResponses.AnswerErrorQuery( + CannedTextResponse, SentMessages, sentBefore, attemptNumber: 1, unterminatedAttempts: 0); } protected override async Task> ExecuteTextCommandAsync( diff --git a/src/Daqifi.Core/Device/DaqifiDevice.cs b/src/Daqifi.Core/Device/DaqifiDevice.cs index 39e12f9e..164d2121 100644 --- a/src/Daqifi.Core/Device/DaqifiDevice.cs +++ b/src/Daqifi.Core/Device/DaqifiDevice.cs @@ -509,7 +509,7 @@ public void Connect() /// /// Waits up to 10 seconds to acquire _textExchangeLock before /// tearing down the consumer / producer / transport. This prevents - /// a race where an in-flight + /// a race where an in-flight /// is mid-swap (text consumer running on the stream, protobuf /// consumer not yet restarted) and Disconnect rips the transport /// out from under it. If the wait times out, Disconnect proceeds @@ -738,6 +738,20 @@ private void RestartMessageConsumerAfterSwap() /// The time in milliseconds to wait for the first text response after sending commands. /// The time in milliseconds of inactivity after the first response before considering the response complete. Defaults to 250ms. /// A cancellation token to observe while waiting for the task to complete. + /// + /// Optional phase that puts the device into the state the commands require, for callers that + /// need one — the SD card operations use it to switch the shared SPI bus over to the card and + /// wait for the firmware to settle. + /// + /// It runs inside the device-wide text-exchange lock, so no competing exchange can interleave + /// between it and and undo what it established. It also runs + /// before the consumer swap, and therefore before the stale-line boundary below: a settle + /// wait placed inside would widen that boundary into a window + /// where a late reply to an earlier command could be captured as part of this response + /// (#396). Anything the device emits in reply to it goes to the protobuf consumer, exactly as + /// it did before this exchange began. + /// + /// /// /// A list of text lines received from the device. Lines that were already in flight when the /// exchange opened — late replies to earlier commands — are excluded: only what arrived once @@ -745,56 +759,21 @@ private void RestartMessageConsumerAfterSwap() /// /// Thrown when the device is not connected or has no transport. /// Thrown when the operation is canceled. + // prepareAsync is added AFTER cancellationToken (technically violating CA1068 + // "CancellationToken should be last") to keep existing positional callers working, matching + // the convention established in IFirmwareUpdateService for the same reason. It is a + // parameter on this seam rather than a second virtual method deliberately: a parallel + // method would be bypassed silently by any subclass that overrides only this one, which for + // an instrumented device or a test double means the override quietly stops intercepting SD + // operations with nothing to indicate it. Overriders must widen their signature — a compile + // error, which is the point. +#pragma warning disable CA1068 protected virtual Task> ExecuteTextCommandAsync( Action setupAction, int responseTimeoutMs = 1000, int completionTimeoutMs = 250, - CancellationToken cancellationToken = default) - { - return ExecuteTextCommandCoreAsync( - prepareAsync: null, - _ => { setupAction(); return Task.CompletedTask; }, - responseTimeoutMs, - completionTimeoutMs, - cancellationToken); - } - - /// - /// Variant of that - /// first runs a phase, for callers that must put the device - /// into a particular state before the commands they are about to send will work. - /// - /// - /// - /// The prepare phase runs inside the device-wide text-exchange lock, so no other text - /// exchange can interleave between it and . That is the whole - /// point of the seam: the SD card operations switch the shared SPI bus over to the card and - /// wait for the firmware to settle, and a competing exchange restoring the LAN interface in - /// that gap would leave their commands running against the wrong interface. - /// - /// - /// It also runs BEFORE the consumer swap and therefore before the stale-line boundary is - /// taken, which is the second reason the phases are split: the settle wait a prepare phase - /// needs would, if it sat inside , widen that boundary into a - /// window where a late reply to an earlier command could be captured as part of this - /// response (#396). Anything the device emits in reply to the prepare phase is consumed by - /// the protobuf consumer, exactly as it was before this exchange began. - /// - /// - /// An async function that puts the device into the required state. Receives the operation's cancellation token. - /// An action that sends SCPI commands to the device while the text consumer is active. - /// The time in milliseconds to wait for the first text response after sending commands. - /// The time in milliseconds of inactivity after the first response before considering the response complete. Defaults to 250ms. - /// A cancellation token to observe while waiting for the task to complete. - /// A list of text lines received from the device, excluding anything already in flight when the exchange opened. - /// Thrown when the device is not connected or has no transport. - /// Thrown when the operation is canceled. - protected virtual Task> ExecuteTextCommandWithPrepareAsync( - Func prepareAsync, - Action setupAction, - int responseTimeoutMs = 1000, - int completionTimeoutMs = 250, - CancellationToken cancellationToken = default) + CancellationToken cancellationToken = default, + Func? prepareAsync = null) { return ExecuteTextCommandCoreAsync( prepareAsync, @@ -803,9 +782,10 @@ protected virtual Task> ExecuteTextCommandWithPrepareAsync completionTimeoutMs, cancellationToken); } +#pragma warning restore CA1068 /// - /// Async overload of + /// Async overload of /// that accepts an async setup action so callers can await cancellable operations /// (e.g. ) between SCPI commands without /// blocking the thread-pool thread. @@ -1114,7 +1094,7 @@ private async Task> ExecuteTextCommandCoreAsync( /// on hardware faults, or discard them if known-stale. /// /// - /// Each iteration uses , which + /// Each iteration uses , which /// pauses the protobuf consumer for the duration of the text exchange. /// Avoid calling this during active streaming or concurrently with /// other text commands. diff --git a/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs b/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs index 741114cc..b7312e2a 100644 --- a/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs +++ b/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs @@ -1668,8 +1668,7 @@ public async Task> GetSdCardFilesAsync(Cancellatio // an earlier command could pass for this listing's terminator. Querying the card // too soon after the switch makes the device answer -200 (Execution error), so // the wait itself is not optional. - lines = await ExecuteTextCommandWithPrepareAsync( - PrepareSdInterfaceAndSettleAsync, + lines = await ExecuteTextCommandAsync( () => { Send(ScpiMessageProducer.GetSdFileList); @@ -1680,7 +1679,8 @@ public async Task> GetSdCardFilesAsync(Cancellatio }, responseTimeoutMs: 3000, completionTimeoutMs: SD_LIST_COMPLETION_TIMEOUT_MS, - cancellationToken: cancellationToken); + cancellationToken: cancellationToken, + prepareAsync: PrepareSdInterfaceAndSettleAsync); isComplete = TrySplitAtSdListTerminator(lines, out listing); @@ -1716,7 +1716,9 @@ public async Task> GetSdCardFilesAsync(Cancellatio /// the card and waits for the firmware to complete the switch. /// /// - /// Passed to rather than run + /// Passed as the prepareAsync phase of + /// + /// rather than run /// inline, so it executes inside the text-exchange lock — a competing exchange restoring the /// LAN interface between the switch and the commands that depend on it would leave them /// running against the wrong interface — and ahead of the exchange's stale-line boundary, so @@ -2142,15 +2144,15 @@ public async Task DeleteSdCardFileAsync(string fileName, CancellationToken cance // wait stays outside the stale-line boundary. The consequence of a stale line is // milder here (delete keys off ContainsScpiError, so it would mean a pointless // delete-and-relist retry rather than a bad listing) but it is the same defect. - lines = await ExecuteTextCommandWithPrepareAsync( - PrepareSdInterfaceAndSettleAsync, + lines = await ExecuteTextCommandAsync( () => { Send(ScpiMessageProducer.DeleteSdFile(fileName)); Send(ScpiMessageProducer.GetSdFileList); }, responseTimeoutMs: 3000, - cancellationToken: cancellationToken); + cancellationToken: cancellationToken, + prepareAsync: PrepareSdInterfaceAndSettleAsync); if (ContainsScpiError(lines)) { @@ -2160,15 +2162,15 @@ public async Task DeleteSdCardFileAsync(string fileName, CancellationToken cance await Task.Delay(SD_INTERFACE_SETTLE_DELAY_MS, cancellationToken).ConfigureAwait(false); - lines = await ExecuteTextCommandWithPrepareAsync( - PrepareSdInterfaceAndSettleAsync, + lines = await ExecuteTextCommandAsync( () => { Send(ScpiMessageProducer.DeleteSdFile(fileName)); Send(ScpiMessageProducer.GetSdFileList); }, responseTimeoutMs: 3000, - cancellationToken: cancellationToken); + cancellationToken: cancellationToken, + prepareAsync: PrepareSdInterfaceAndSettleAsync); if (!ContainsScpiError(lines)) {