diff --git a/src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs b/src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs new file mode 100644 index 00000000..895fccc4 --- /dev/null +++ b/src/Daqifi.Core.Tests/Device/DaqifiDeviceStaleTextLineTests.cs @@ -0,0 +1,203 @@ +using Daqifi.Core.Communication.Transport; +using Daqifi.Core.Device; +using System.Text; + +namespace Daqifi.Core.Tests.Device; + +/// +/// Coverage for the stale-line boundary in the text exchange (raised while fixing #396). +/// +/// +/// A late reply to an EARLIER command can still be in flight when the next text exchange opens +/// its consumer, and would otherwise be returned as part of the new exchange's response. That is +/// wrong for every caller, but it is actively dangerous for one that infers device liveness from +/// response content: the SD listing accepts a SYSTem:ERRor? reply as proof that the device +/// answered and that the listing before it is complete. A stale line satisfying that check would +/// let a silent device pass as a healthy empty SD card — the exact bug #396 is about. +/// +public class DaqifiDeviceStaleTextLineTests +{ + [Fact] + public async Task ExecuteTextCommand_DropsLinesThatArrivedBeforeTheExchangeSentAnything() + { + // The stale line is released into the stream at the moment the exchange binds its text + // consumer — after the protobuf consumer has been stopped, and before the setup action + // has sent anything. That is exactly the window a late reply to an earlier command can + // land in. The device then stays silent, as one that has stopped answering would. + using var transport = new ReleaseOnStreamAccessMockTransport("0,\"No error\"\r\n"); + using var device = new StaleLineTestableDevice("Preloaded Device", transport); + + device.Connect(); + transport.ReleaseOnStreamAccess(2); // 2nd access inside the exchange = text-consumer bind + + var lines = await device.CallExecuteTextCommandAsync(() => { }); + + // The exchange sent nothing, so nothing in it can legitimately have been answered. + Assert.Empty(lines); + + device.Disconnect(); + } + + [Fact] + public async Task ExecuteTextCommand_KeepsLinesThatArriveAfterTheExchangeSentSomething() + { + // The complement, so the fix cannot be "drop everything": a reply that arrives once the + // setup action has sent its command must still be returned. + using var transport = new ReleaseOnStreamAccessMockTransport("0,\"No error\"\r\n"); + using var device = new StaleLineTestableDevice("Answering Device", transport); + + device.Connect(); + + var lines = await device.CallExecuteTextCommandAsync(() => transport.Release()); + + Assert.Contains(lines, l => l.Contains("No error")); + + device.Disconnect(); + } + + /// Exposes the protected text-exchange entry point. + private class StaleLineTestableDevice : DaqifiDevice + { + public StaleLineTestableDevice(string name, IStreamTransport transport) + : base(name, transport) + { + } + + public Task> CallExecuteTextCommandAsync(Action setupAction) + { + return ExecuteTextCommandAsync(setupAction, responseTimeoutMs: 500, completionTimeoutMs: 150); + } + } + + /// + /// Transport whose stream withholds one canned line until released, and which can arm that + /// release on the Nth access of its property. + /// + /// + /// Keying off the property access — rather than a delay — makes the timing deterministic: + /// the text exchange reads Stream once up front and again when it binds the temporary + /// text consumer, and that second access happens after the protobuf consumer has been stopped + /// (so it cannot swallow the line first) and before the setup action runs. + /// + private sealed class ReleaseOnStreamAccessMockTransport : IStreamTransport + { + private readonly WithheldLineStream _stream; + private int _streamAccessCount; + private int _releaseOnAccess = -1; + private bool _isConnected; + private bool _disposed; + + public ReleaseOnStreamAccessMockTransport(string line) + { + _stream = new WithheldLineStream(line); + } + + public Stream Stream + { + get + { + if (_disposed) throw new ObjectDisposedException(nameof(ReleaseOnStreamAccessMockTransport)); + + var access = Interlocked.Increment(ref _streamAccessCount); + if (_releaseOnAccess > 0 && access == _releaseOnAccess) + { + _stream.Release(); + } + + return _stream; + } + } + + public bool IsConnected => _isConnected && !_disposed; + + public string ConnectionInfo => _isConnected ? "Withheld: Connected" : "Withheld: Disconnected"; + + public event EventHandler? StatusChanged; + + /// Arms the release for the Nth subsequent access of . + public void ReleaseOnStreamAccess(int accessNumber) + { + Interlocked.Exchange(ref _streamAccessCount, 0); + _releaseOnAccess = accessNumber; + } + + /// Releases the withheld line immediately. + public void Release() => _stream.Release(); + + public Task ConnectAsync() => ConnectAsync(null); + + public Task ConnectAsync(ConnectionRetryOptions? retryOptions) + { + if (_disposed) throw new ObjectDisposedException(nameof(ReleaseOnStreamAccessMockTransport)); + _isConnected = true; + StatusChanged?.Invoke(this, new TransportStatusEventArgs(true, ConnectionInfo)); + return Task.CompletedTask; + } + + public Task DisconnectAsync() + { + _isConnected = false; + StatusChanged?.Invoke(this, new TransportStatusEventArgs(false, ConnectionInfo)); + return Task.CompletedTask; + } + + public void Connect() => ConnectAsync().Wait(); + + public void Disconnect() => DisconnectAsync().Wait(); + + public void Dispose() + { + if (_disposed) return; + _isConnected = false; + _disposed = true; + } + + private sealed class WithheldLineStream : Stream + { + private readonly byte[] _line; + private readonly object _gate = new(); + private bool _released; + private int _position; + + public WithheldLineStream(string line) => _line = Encoding.ASCII.GetBytes(line); + + public void Release() + { + lock (_gate) + { + _released = true; + } + } + + public override bool CanRead => true; + public override bool CanSeek => false; + public override bool CanWrite => true; + public override long Length => throw new NotSupportedException(); + public override long Position { get => throw new NotSupportedException(); set => throw new NotSupportedException(); } + + public override void Flush() { } + + public override int Read(byte[] buffer, int offset, int count) + { + lock (_gate) + { + if (_released && _position < _line.Length) + { + var toCopy = Math.Min(count, _line.Length - _position); + Array.Copy(_line, _position, buffer, offset, toCopy); + _position += toCopy; + return toCopy; + } + } + + // Idle link: nothing to hand over, and no busy-spin in the reader thread. + Thread.Sleep(10); + return 0; + } + + public override long Seek(long offset, SeekOrigin origin) => throw new NotSupportedException(); + public override void SetLength(long value) => throw new NotSupportedException(); + public override void Write(byte[] buffer, int offset, int count) { } + } + } +} diff --git a/src/Daqifi.Core.Tests/Device/ScpiResponseClassifierTests.cs b/src/Daqifi.Core.Tests/Device/ScpiResponseClassifierTests.cs index 8558c4a2..a15c1191 100644 --- a/src/Daqifi.Core.Tests/Device/ScpiResponseClassifierTests.cs +++ b/src/Daqifi.Core.Tests/Device/ScpiResponseClassifierTests.cs @@ -55,5 +55,34 @@ public void TryExtractErrorCode_ReturnsFalseAndZero_ForNonNumericOrNonError(stri Assert.False(ScpiResponseClassifier.TryExtractErrorCode(line, out var code)); Assert.Equal(0, code); } + + [Theory] + [InlineData("0,\"No error\"")] // clean queue — the common case + [InlineData("-200,\"Execution error\"")] + [InlineData("+0,\"No error\"")] // explicit positive sign + [InlineData("-420, \"Query UNTERMINATED\"")] // space after the comma + [InlineData(" 0,\"No error\" \r\n")] // leading/trailing whitespace + CRLF + [InlineData("0,\"\"")] // empty message + public void IsSystemErrorReplyLine_MatchesErrorQueueReplies(string line) + { + Assert.True(ScpiResponseClassifier.IsSystemErrorReplyLine(line)); + } + + [Theory] + [InlineData("Daqifi/log_20240115_103000.bin 1024")] // SD listing entry + [InlineData("Daqifi/log_20240115_103000.bin")] // listing entry with no size + [InlineData("0,\"No error\" 1024")] // reply shape but with a trailing size + [InlineData("**ERROR: -200, \"Execution error\"")] // ERROR-prefixed, not a query reply + [InlineData("Error !! No SD Card Detected")] + [InlineData("0 1024")] // no comma + [InlineData("0,No error")] // unquoted message + [InlineData("0,\"")] // unterminated quote + [InlineData("-,\"No error\"")] // sign with no digits + [InlineData("[Error:3]Failed to open directory")] + [InlineData("")] + public void IsSystemErrorReplyLine_DoesNotMatchListingOrOtherText(string line) + { + Assert.False(ScpiResponseClassifier.IsSystemErrorReplyLine(line)); + } } } diff --git a/src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs b/src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs index 41d5a951..17fab924 100644 --- a/src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs +++ b/src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs @@ -871,8 +871,9 @@ public async Task GetSdCardFilesAsync_WithFilesAndInterleavedError_ReturnsFiles( [Fact] public async Task GetSdCardFilesAsync_WithEmptyDirectory_ReturnsEmptyList() { - // Arrange - device returns no lines (empty directory, no errors). This is - // the legitimate "0 files" case and must keep its existing behavior. + // Arrange - device returns no listing lines (empty directory, no errors) but does + // answer the terminator query. This is the legitimate "0 files" case and must keep + // its existing behavior: an empty list, no exception (#396). var device = new RetryableSdCardStreamingDevice("TestDevice"); device.ResponseSequence.Enqueue(new List()); device.Connect(); @@ -885,6 +886,150 @@ public async Task GetSdCardFilesAsync_WithEmptyDirectory_ReturnsEmptyList() Assert.Equal(1, device.ExecuteTextCommandCallCount); } + #region Timed-out vs. empty listing (issue #396) + + [Fact] + public async Task GetSdCardFilesAsync_SendsTerminatorQueryAfterListCommand() + { + // The terminator only proves the listing is complete if it is requested AFTER the + // listing itself, in the same exchange — the transport is ordered, so its reply + // cannot overtake listing lines the device had already written. + var device = new TestableSdCardStreamingDevice("TestDevice"); + device.CannedTextResponse = new List { "Daqifi/log_20240115_103000.bin" }; + device.Connect(); + + await device.GetSdCardFilesAsync(); + + var sentCommands = device.SentMessages.Select(m => m.Data).ToList(); + var listIndex = sentCommands.IndexOf("SYSTem:STORage:SD:LIST?"); + var terminatorIndex = sentCommands.IndexOf("SYSTem:ERRor?"); + Assert.True(listIndex >= 0, "The file-list query was not sent."); + Assert.True(terminatorIndex > listIndex, "The terminator query must be sent after the file-list query."); + } + + [Fact] + public async Task GetSdCardFilesAsync_TerminatorReplyIsNotParsedAsAFile() + { + // The terminator is a protocol artifact, not directory content: it must never reach + // the file parser and show up as a phantom SD card file. + var device = new TestableSdCardStreamingDevice("TestDevice"); + device.CannedTextResponse = new List { "Daqifi/log_20240115_103000.bin" }; + device.Connect(); + + var files = await device.GetSdCardFilesAsync(); + + Assert.Single(files); + Assert.Equal("log_20240115_103000.bin", files[0].FileName); + } + + [Fact] + public async Task GetSdCardFilesAsync_WhenDeviceNeverAnswers_ThrowsInsteadOfReturningEmptyList() + { + // The bug in #396: a device that never answered produced the exact same empty list as + // a healthy empty card, so downstream rendered "SD card OK - 0 files" for an + // unreachable device holding data. Both attempts go unanswered here. + var device = new TestableSdCardStreamingDevice("TestDevice"); + device.CannedTextResponse = new List { "Daqifi/log_20240115_103000.bin" }; + device.Connect(); + + // A first, healthy listing populates the cache... + Assert.Single(await device.GetSdCardFilesAsync()); + + // ...then the device goes silent. + device.CannedTextResponse = new List(); + device.UnterminatedAttempts = int.MaxValue; + + await Assert.ThrowsAsync( + () => device.GetSdCardFilesAsync()); + + // The cache must not be overwritten with a listing we never actually received. + Assert.Single(device.SdCardFiles); + Assert.Equal("log_20240115_103000.bin", device.SdCardFiles[0].FileName); + } + + [Fact] + public async Task GetSdCardFilesAsync_WhenListingIsTruncated_ThrowsRatherThanReturningAShortList() + { + // The case no downstream mitigation can reach: the device answered, but stopped + // part-way through the listing. Corroborating with a later query (as the Avalonia port + // does) cannot detect this — only the missing terminator can. + var device = new TestableSdCardStreamingDevice("TestDevice"); + device.CannedTextResponse = new List + { + "Daqifi/log_20240115_103000.bin", + "Daqifi/log_20240115_1030", + }; + device.UnterminatedAttempts = int.MaxValue; + device.Connect(); + + var ex = await Assert.ThrowsAsync( + () => device.GetSdCardFilesAsync()); + + // The partial response is preserved for diagnostics rather than silently returned. + Assert.Equal(2, ex.RawDeviceResponse.Count); + } + + [Fact] + public async Task GetSdCardFilesAsync_WithStaleTerminatorAheadOfTheListing_StillReturnsTheFiles() + { + // A terminator reply from a previous, timed-out exchange can still be in the transport + // buffer and lead this response. Splitting at the FIRST match would discard the real + // listing behind it and report an empty card — the very failure #396 is about. + var device = new TestableSdCardStreamingDevice("TestDevice"); + device.CannedTextResponse = new List + { + "-200,\"Execution error\"", // stale reply left over from an earlier exchange + "Daqifi/log_20240115_103000.bin", + "Daqifi/data.bin", + }; + device.Connect(); + + var files = await device.GetSdCardFilesAsync(); + + // Both files survive, and the stale reply is not parsed as a phantom file. + Assert.Equal(2, files.Count); + Assert.Equal("log_20240115_103000.bin", files[0].FileName); + Assert.Equal("data.bin", files[1].FileName); + } + + [Fact] + public async Task GetSdCardFilesAsync_WhenTerminatorMissingOnFirstAttemptOnly_RetriesAndSucceeds() + { + // A one-off stall is retried on the same terms as a transient SCPI error, so a single + // dropped reply does not become a user-visible failure. + var device = new RetryableSdCardStreamingDevice("TestDevice"); + device.ResponseSequence.Enqueue(new List()); + device.ResponseSequence.Enqueue(new List { "Daqifi/log_20240115_103000.bin" }); + device.UnterminatedAttempts = 1; + device.Connect(); + + var files = await device.GetSdCardFilesAsync(); + + Assert.Single(files); + Assert.Equal("log_20240115_103000.bin", files[0].FileName); + Assert.Equal(2, device.ExecuteTextCommandCallCount); + } + + [Fact] + public async Task GetSdCardFilesAsync_WhenDeviceNeverAnswers_RestoresLanInterface() + { + // The throw must not skip the interface restore — the SD subsystem would be left + // enabled and the LAN disabled for every later command. + var device = new TestableSdCardStreamingDevice("TestDevice"); + device.CannedTextResponse = new List(); + device.UnterminatedAttempts = int.MaxValue; + device.Connect(); + + await Assert.ThrowsAsync( + () => device.GetSdCardFilesAsync()); + + var sentCommands = device.SentMessages.Select(m => m.Data).ToList(); + Assert.Contains("SYSTem:STORage:SD:ENAble 0", sentCommands); // DisableStorageSd + Assert.Contains("SYSTem:COMMunicate:LAN:ENAbled 1", sentCommands); // EnableNetworkLan + } + + #endregion + [Fact] public async Task GetSdCardFilesAsync_LastScpiError_ContainsOnlyScpiFormattedLine() { @@ -1799,6 +1944,9 @@ private class RetryableSdCardStreamingDevice : DaqifiStreamingDevice public Queue> ResponseSequence { get; } = new(); public int ExecuteTextCommandCallCount { get; private set; } + /// + public int UnterminatedAttempts { get; set; } + public RetryableSdCardStreamingDevice(string name, IPAddress? ipAddress = null) : base(name, ipAddress) { @@ -1824,12 +1972,14 @@ protected override Task> ExecuteTextCommandAsync( int completionTimeoutMs = 250, CancellationToken cancellationToken = default) { + var sentBefore = SentMessages.Count; setupAction(); ExecuteTextCommandCallCount++; var response = ResponseSequence.Count > 0 ? ResponseSequence.Dequeue() : new List(); - return Task.FromResult>(response); + return Task.FromResult(SdCardTestResponses.AnswerErrorQuery( + response, SentMessages, sentBefore, ExecuteTextCommandCallCount, UnterminatedAttempts)); } protected override async Task> ExecuteTextCommandAsync( @@ -1838,12 +1988,63 @@ protected override async Task> ExecuteTextCommandAsync( int completionTimeoutMs = 250, CancellationToken cancellationToken = default) { + var sentBefore = SentMessages.Count; await setupActionAsync(cancellationToken).ConfigureAwait(false); ExecuteTextCommandCallCount++; var response = ResponseSequence.Count > 0 ? ResponseSequence.Dequeue() : new List(); - return response; + return SdCardTestResponses.AnswerErrorQuery( + response, SentMessages, sentBefore, ExecuteTextCommandCallCount, UnterminatedAttempts); + } + } + + /// + /// Shared device-behavior helper for the SD card fakes: models the way a live device + /// answers the SYSTem:ERRor? query that GetSdCardFilesAsync appends to the + /// listing exchange as an end-of-listing terminator (#396). + /// + private static class SdCardTestResponses + { + /// The reply a healthy device gives to SYSTem:ERRor? with a clean queue. + public const string NoErrorReply = "0,\"No error\""; + + /// + /// Appends the SYSTem:ERRor? reply to when the + /// exchange actually asked for it and this attempt is not one of the + /// leading attempts being simulated as + /// unanswered. + /// + public static IReadOnlyList AnswerErrorQuery( + IReadOnlyList response, + IReadOnlyList> sentMessages, + int sentBefore, + int attemptNumber, + int unterminatedAttempts) + { + if (attemptNumber <= unterminatedAttempts) + { + return response; + } + + var errorQuery = ScpiMessageProducer.GetSystemError.Data; + var askedForError = false; + for (var i = sentBefore; i < sentMessages.Count; i++) + { + if (sentMessages[i].Data == errorQuery) + { + askedForError = true; + break; + } + } + + if (!askedForError) + { + return response; + } + + var withTerminator = new List(response) { NoErrorReply }; + return withTerminator; } } @@ -1853,9 +2054,20 @@ protected override async Task> ExecuteTextCommandAsync( /// private class TestableSdCardStreamingDevice : DaqifiStreamingDevice { + private int _executeTextCommandCallCount; + public List> SentMessages { get; } = new(); public List CannedTextResponse { get; set; } = new(); + /// + /// Number of leading text exchanges to answer WITHOUT the SYSTem:ERRor? reply + /// that GetSdCardFilesAsync uses as its end-of-listing terminator (#396) — i.e. + /// how many attempts simulate a device that never answered, or stopped answering + /// part-way through the listing. Defaults to 0, so the fake behaves like a healthy + /// device and always terminates its listing. + /// + public int UnterminatedAttempts { get; set; } + /// /// Simulates a USB connection so SD card operations are allowed. /// @@ -1880,9 +2092,13 @@ protected override Task> ExecuteTextCommandAsync( int completionTimeoutMs = 250, CancellationToken cancellationToken = default) { + var sentBefore = SentMessages.Count; + // Execute the setup action so we can capture the SCPI commands setupAction(); - return Task.FromResult>(CannedTextResponse); + _executeTextCommandCallCount++; + return Task.FromResult(SdCardTestResponses.AnswerErrorQuery( + CannedTextResponse, SentMessages, sentBefore, _executeTextCommandCallCount, UnterminatedAttempts)); } protected override async Task> ExecuteTextCommandAsync( @@ -1891,8 +2107,11 @@ protected override async Task> ExecuteTextCommandAsync( int completionTimeoutMs = 250, CancellationToken cancellationToken = default) { + var sentBefore = SentMessages.Count; await setupActionAsync(cancellationToken).ConfigureAwait(false); - return CannedTextResponse; + _executeTextCommandCallCount++; + return SdCardTestResponses.AnswerErrorQuery( + CannedTextResponse, SentMessages, sentBefore, _executeTextCommandCallCount, UnterminatedAttempts); } } @@ -2143,8 +2362,10 @@ protected override Task> ExecuteTextCommandAsync( int completionTimeoutMs = 250, CancellationToken cancellationToken = default) { + var sentBefore = SentMessages.Count; setupAction(); - return Task.FromResult>(CannedTextResponse); + return Task.FromResult(SdCardTestResponses.AnswerErrorQuery( + CannedTextResponse, SentMessages, sentBefore, attemptNumber: 1, unterminatedAttempts: 0)); } protected override async Task> ExecuteTextCommandAsync( @@ -2153,8 +2374,10 @@ protected override async Task> ExecuteTextCommandAsync( int completionTimeoutMs = 250, CancellationToken cancellationToken = default) { + var sentBefore = SentMessages.Count; await setupActionAsync(cancellationToken).ConfigureAwait(false); - return CannedTextResponse; + return SdCardTestResponses.AnswerErrorQuery( + CannedTextResponse, SentMessages, sentBefore, attemptNumber: 1, unterminatedAttempts: 0); } protected override async Task ExecuteRawCaptureAsync( diff --git a/src/Daqifi.Core/Device/DaqifiDevice.cs b/src/Daqifi.Core/Device/DaqifiDevice.cs index 29a3bf30..13be9768 100644 --- a/src/Daqifi.Core/Device/DaqifiDevice.cs +++ b/src/Daqifi.Core/Device/DaqifiDevice.cs @@ -738,7 +738,11 @@ 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. - /// A list of text lines received from the device. + /// + /// 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 + /// had begun sending is returned. + /// /// Thrown when the device is not connected or has no transport. /// Thrown when the operation is canceled. protected virtual Task> ExecuteTextCommandAsync( @@ -863,6 +867,10 @@ private async Task> ExecuteTextCommandCoreAsync( var stream = _transport.Stream; int? originalReadTimeout = null; + // Number of lines that were already in flight when this exchange opened — see the + // note at the point it is captured, below. + var staleLineCount = 0; + try { if (stream.CanTimeout) @@ -911,6 +919,23 @@ private async Task> ExecuteTextCommandCoreAsync( SafeLog(() => _logger.LogDebug("[ExecuteTextCommandAsync] Text consumer started at {ElapsedMs}ms", sw.ElapsedMilliseconds)); + // Mark the boundary between "already in flight" and "answers to this exchange". + // Anything captured before the setup action has sent anything is a late reply to + // an EARLIER command, or line noise — never a response to a command this exchange + // has yet to send. Those lines are dropped from the result below. + // + // Position matters as much as content: a caller that keys off response content — + // e.g. the SD listing's end-of-listing terminator (#396) — would otherwise accept + // a stale line as proof that the device answered a query it never even received, + // and report a complete listing for a device that has gone silent. + staleLineCount = collectedLines.Count; + if (staleLineCount > 0) + { + SafeLog(() => _logger.LogDebug( + "[ExecuteTextCommandAsync] Discarding {StaleLineCount} line(s) received before this exchange sent anything", + staleLineCount)); + } + // Execute the setup action (sends SCPI commands). ConfigureAwait(false) // matches the surrounding lock-protected awaits. await setupActionAsync(cancellationToken).ConfigureAwait(false); @@ -984,7 +1009,11 @@ private async Task> ExecuteTextCommandCoreAsync( SafeLog(() => _logger.LogDebug("[ExecuteTextCommandAsync] Total elapsed: {ElapsedMs}ms", sw.ElapsedMilliseconds)); } - return collectedLines; + // The text consumer is stopped by this point, so the list is no longer being + // appended to concurrently and can be re-projected safely. + return staleLineCount > 0 + ? collectedLines.Skip(staleLineCount).ToList() + : collectedLines; } finally { diff --git a/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs b/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs index ebcd6dcf..625386fa 100644 --- a/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs +++ b/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs @@ -48,6 +48,19 @@ public class DaqifiStreamingDevice : DaqifiDevice, IStreamingDevice, INetworkCon /// private const int SD_LIST_MAX_RETRIES = 1; + /// + /// Inactivity window that ends the SD listing text exchange, in milliseconds. + /// + /// + /// Deliberately longer than the 250ms default. The listing is only accepted once its + /// end-of-listing terminator has been seen (see ), and the + /// terminator can trail the last listing line by more than the default window — the firmware + /// walks the directory tree between chunks, and a congested WiFi link adds its own gaps. With + /// the default, a merely-slow terminator would read as a missing one and fail a listing that + /// was about to complete. + /// + private const int SD_LIST_COMPLETION_TIMEOUT_MS = 1000; + /// /// Maximum number of retry attempts for the USB stream-interface command sent during /// when the device returns a transient SCPI error @@ -1583,6 +1596,39 @@ private void EnsureSdFileTransferSupportedOnTransport() /// Thrown when no SD card is installed in the device. /// Thrown when the SD card filesystem cannot satisfy the request (corrupt card, unreadable directory). /// Thrown when the device returned an SCPI error that did not match a more specific condition. Empty directories return an empty list rather than throwing. + /// + /// Thrown when the listing did not arrive in full — the device never answered, or stopped + /// answering part-way through. Distinguishing this from a genuinely empty card is the whole + /// point of the terminator probe described in the remarks (closes #396). + /// + /// + /// + /// The firmware emits no end-of-listing marker, and for an empty directory it writes nothing + /// at all, so a lost or truncated reply is byte-for-byte indistinguishable from a healthy + /// empty card. Core closes that gap by appending a SYSTem:ERRor? query to the same + /// text exchange: the transport delivers in order and the firmware does not process the + /// next command until the listing has been handed to the output, so receiving the reply + /// proves both that the device is answering and that the listing ahead of it is complete. + /// Its absence means the response is incomplete, and the caller gets an exception instead of + /// a plausible-looking empty list. + /// + /// + /// The terminator is only meaningful if it cannot be confused with a late reply to an + /// earlier command, so two things guard that boundary: the text exchange discards whatever + /// was already in flight when it opened, and this method does its SPI-bus switch and settle + /// delay before the exchange rather than inside it, leaving the exchange with no internal + /// gap for a stale reply to slip into. + /// + /// + /// The terminator's error code is used only as a liveness marker, never for classification: + /// the queue it pops can hold entries left by earlier commands, so attributing the code to + /// this listing would misreport stale failures. SD errors continue to be classified from the + /// listing lines themselves. Note the side effect this implies — each listing consumes one + /// entry from the device's SCPI error queue, so a + /// run afterwards will not see the entry + /// this listing generated. + /// + /// public async Task> GetSdCardFilesAsync(CancellationToken cancellationToken = default) { if (!IsConnected) @@ -1598,42 +1644,54 @@ public async Task> GetSdCardFilesAsync(Cancellatio Send(ScpiMessageProducer.StopStreaming); IsStreaming = false; - IReadOnlyList lines; + IReadOnlyList lines = Array.Empty(); + IReadOnlyList listing = Array.Empty(); + var isComplete = false; try { - lines = await ExecuteTextCommandAsync(async ct => - { - PrepareSdInterface(); - - // Allow the device firmware to complete the SPI bus switch - // before querying the SD card. Without this delay, the device - // can return SCPI error -200 (Execution error). - await Task.Delay(SD_INTERFACE_SETTLE_DELAY_MS, ct).ConfigureAwait(false); - - Send(ScpiMessageProducer.GetSdFileList); - }, responseTimeoutMs: 3000, cancellationToken: cancellationToken); - - // If the response contains a SCPI error (transient timing issue), - // retry once after an additional settle delay. - if (ContainsScpiError(lines)) + // Attempt 0 plus SD_LIST_MAX_RETRIES retries. A SCPI error here is often a transient + // timing issue, and an unterminated response can be a one-off stall, so both are + // retried once after an additional settle delay before being surfaced. + for (var attempt = 0; attempt <= SD_LIST_MAX_RETRIES; attempt++) { - for (var retry = 0; retry < SD_LIST_MAX_RETRIES; retry++) + if (attempt > 0) { cancellationToken.ThrowIfCancellationRequested(); - await Task.Delay(SD_INTERFACE_SETTLE_DELAY_MS, cancellationToken); + 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(async ct => + lines = await ExecuteTextCommandAsync( + () => { - PrepareSdInterface(); - await Task.Delay(SD_INTERFACE_SETTLE_DELAY_MS, ct).ConfigureAwait(false); Send(ScpiMessageProducer.GetSdFileList); - }, responseTimeoutMs: 3000, cancellationToken: cancellationToken); - if (!ContainsScpiError(lines)) - { - break; - } + // End-of-listing terminator — see this method's remarks. Sent inside + // the same text exchange so the ordering guarantee holds. + Send(ScpiMessageProducer.GetSystemError); + }, + responseTimeoutMs: 3000, + completionTimeoutMs: SD_LIST_COMPLETION_TIMEOUT_MS, + cancellationToken: cancellationToken); + + isComplete = TrySplitAtSdListTerminator(lines, out listing); + + if (isComplete && !ContainsScpiError(listing)) + { + break; } } } @@ -1646,13 +1704,73 @@ public async Task> GetSdCardFilesAsync(Cancellatio } } - ThrowIfSdCardListError(lines); + if (!isComplete) + { + throw new SdCardListIncompleteException(lines); + } - var files = SdCardFileListParser.ParseFileList(lines); + ThrowIfSdCardListError(listing); + + var files = SdCardFileListParser.ParseFileList(listing); _sdCardFiles = files; return files; } + /// + /// Splits a raw SD listing response at the SYSTem:ERRor? terminator reply that + /// appends to the exchange. + /// + /// The raw response lines captured from the device. + /// + /// The lines that precede the terminator — the directory listing proper — when the method + /// returns true; otherwise the unmodified input. + /// + /// + /// true when the terminator was present, meaning the response is complete; + /// false when it never arrived, meaning the response is missing or truncated. + /// + private static bool TrySplitAtSdListTerminator( + IReadOnlyList lines, + out IReadOnlyList listingLines) + { + // Scan from the end. A terminator reply from a PREVIOUS, timed-out exchange can still + // be sitting in the transport buffer and lead this response; splitting at the first + // match would then discard the listing that follows it and report an empty card — + // exactly the failure this terminator exists to prevent. + var terminatorIndex = -1; + for (var i = lines.Count - 1; i >= 0; i--) + { + if (ScpiResponseClassifier.IsSystemErrorReplyLine(lines[i])) + { + terminatorIndex = i; + break; + } + } + + if (terminatorIndex < 0) + { + listingLines = lines; + return false; + } + + var listing = new List(terminatorIndex); + for (var j = 0; j < terminatorIndex; j++) + { + // Any other terminator-shaped line is a stale reply of the same kind, not + // directory content — no firmware listing entry can match that shape, since + // entries are always " ". + if (ScpiResponseClassifier.IsSystemErrorReplyLine(lines[j])) + { + continue; + } + + listing.Add(lines[j]); + } + + listingLines = listing; + return true; + } + /// /// Retrieves the free and total byte counts of the device's SD card. /// @@ -2004,13 +2122,23 @@ public async Task DeleteSdCardFileAsync(string fileName, CancellationToken cance IReadOnlyList lines; try { - lines = await ExecuteTextCommandAsync(async ct => - { - PrepareSdInterface(); - await Task.Delay(SD_INTERFACE_SETTLE_DELAY_MS, ct).ConfigureAwait(false); - Send(ScpiMessageProducer.DeleteSdFile(fileName)); - Send(ScpiMessageProducer.GetSdFileList); - }, responseTimeoutMs: 3000, cancellationToken: cancellationToken); + // 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( + () => + { + Send(ScpiMessageProducer.DeleteSdFile(fileName)); + Send(ScpiMessageProducer.GetSdFileList); + }, + responseTimeoutMs: 3000, + cancellationToken: cancellationToken); if (ContainsScpiError(lines)) { @@ -2018,15 +2146,19 @@ public async Task DeleteSdCardFileAsync(string fileName, CancellationToken cance { cancellationToken.ThrowIfCancellationRequested(); - await Task.Delay(SD_INTERFACE_SETTLE_DELAY_MS, cancellationToken); + await Task.Delay(SD_INTERFACE_SETTLE_DELAY_MS, cancellationToken).ConfigureAwait(false); - lines = await ExecuteTextCommandAsync(async ct => - { - PrepareSdInterface(); - await Task.Delay(SD_INTERFACE_SETTLE_DELAY_MS, ct).ConfigureAwait(false); - Send(ScpiMessageProducer.DeleteSdFile(fileName)); - Send(ScpiMessageProducer.GetSdFileList); - }, responseTimeoutMs: 3000, cancellationToken: cancellationToken); + PrepareSdInterface(); + await Task.Delay(SD_INTERFACE_SETTLE_DELAY_MS, cancellationToken).ConfigureAwait(false); + + lines = await ExecuteTextCommandAsync( + () => + { + Send(ScpiMessageProducer.DeleteSdFile(fileName)); + Send(ScpiMessageProducer.GetSdFileList); + }, + responseTimeoutMs: 3000, + cancellationToken: cancellationToken); if (!ContainsScpiError(lines)) { @@ -2331,6 +2463,8 @@ private static void ThrowIfSdCardListError(IReadOnlyList lines) } // No error lines and no content lines — empty directory. Caller continues. + // Safe to treat as empty rather than as a lost reply: GetSdCardFilesAsync only reaches + // this point once the device has answered the end-of-listing terminator (#396). } /// diff --git a/src/Daqifi.Core/Device/ScpiResponseClassifier.cs b/src/Daqifi.Core/Device/ScpiResponseClassifier.cs index 1edfcd3b..edb503c6 100644 --- a/src/Daqifi.Core/Device/ScpiResponseClassifier.cs +++ b/src/Daqifi.Core/Device/ScpiResponseClassifier.cs @@ -51,6 +51,67 @@ internal static bool IsScpiErrorLine(string line) || MatchesStrictScpiErrorPrefix(trimmed, "ERROR"); } + /// + /// Returns true if the line is the reply to a SYSTem:ERRor? query — the IEEE 488.2 + /// error-queue format <code>,"<message>", e.g. 0,"No error" or + /// -200,"Execution error". Deliberately narrow: the code must be the first thing on + /// the line and the quoted message must run to the end of it, so a device-emitted SD listing + /// entry (always <path> <size>, space-separated, per the firmware's + /// "%s %u\r\n" format) can never match. Unlike this does + /// not require an ERROR token — the query reply carries the bare code. + /// + /// + /// Used as the end-of-response marker for the SD card directory listing (closes #396): the + /// firmware emits no terminator of its own and writes nothing at all for an empty directory, + /// so Core appends a SYSTem:ERRor? query to the same text exchange and treats its + /// reply as proof that everything the device had to say about the listing has arrived. + /// + internal static bool IsSystemErrorReplyLine(string line) + { + var trimmed = line.Trim(); + + var index = 0; + if (index < trimmed.Length && (trimmed[index] == '+' || trimmed[index] == '-')) + { + index++; + } + + var digitStart = index; + while (index < trimmed.Length && trimmed[index] >= '0' && trimmed[index] <= '9') + { + index++; + } + + if (index == digitStart) + { + return false; + } + + index = SkipSpaces(trimmed, index); + + if (index >= trimmed.Length || trimmed[index] != ',') + { + return false; + } + + index = SkipSpaces(trimmed, index + 1); + + // Require an opening quote and a distinct closing quote at end-of-line. + return index < trimmed.Length - 1 + && trimmed[index] == '"' + && trimmed[trimmed.Length - 1] == '"'; + } + + private static int SkipSpaces(string value, int index) + { + while (index < value.Length && (value[index] == ' ' || value[index] == '\t')) + { + index++; + } + + return index; + } + /// /// Extracts the numeric error code from a SCPI error line — e.g. -200 from /// **ERROR: -200,"Execution error", ERROR -113,"Undefined header", or diff --git a/src/Daqifi.Core/Device/SdCard/ISdCardOperations.cs b/src/Daqifi.Core/Device/SdCard/ISdCardOperations.cs index 55f97822..884a04a0 100644 --- a/src/Daqifi.Core/Device/SdCard/ISdCardOperations.cs +++ b/src/Daqifi.Core/Device/SdCard/ISdCardOperations.cs @@ -40,6 +40,11 @@ public interface ISdCardOperations /// Thrown when no SD card is installed in the device. /// Thrown when the SD card filesystem cannot satisfy the request (e.g. corrupt card, unreadable directory). /// Thrown when the device returned an SCPI error that did not match a more specific condition. An empty directory returns an empty list rather than throwing. + /// + /// Thrown when the device did not answer the listing query, or stopped answering part-way + /// through it. An empty result therefore always means an empty card — never an unreachable + /// device — so callers can render it as such (closes #396). + /// Task> GetSdCardFilesAsync(CancellationToken cancellationToken = default); /// diff --git a/src/Daqifi.Core/Device/SdCard/SdCardListIncompleteException.cs b/src/Daqifi.Core/Device/SdCard/SdCardListIncompleteException.cs new file mode 100644 index 00000000..4b22020e --- /dev/null +++ b/src/Daqifi.Core/Device/SdCard/SdCardListIncompleteException.cs @@ -0,0 +1,43 @@ +using System.Collections.Generic; + +#nullable enable + +namespace Daqifi.Core.Device.SdCard +{ + /// + /// Thrown when an SD card directory listing did not arrive in full: the device either never + /// answered the SYSTem:STORage:SD:LISt? query, or stopped answering part-way through it. + /// + /// + /// + /// The firmware emits no end-of-listing marker and writes nothing at all for an empty directory, + /// so "no bytes received" is byte-for-byte identical to a healthy empty card on the wire. Core + /// therefore appends a SYSTem:ERRor? query to the same text exchange and uses its reply as + /// a terminator: the transport is ordered, so a terminator reply proves both that the device is + /// answering and that everything it had to say about the listing arrived first. This exception + /// is raised when that terminator never came back — which previously surfaced as a healthy-looking + /// "empty SD card" (closes #396). + /// + /// + /// A caller seeing this should treat the listing as unknown, not empty. Typical causes are a + /// silently dropped link, a device that is wedged or powered down, or congestion severe enough + /// to push the reply past the response window. Retrying once the link is known good is + /// reasonable; rendering "0 files" is not. + /// + /// + public class SdCardListIncompleteException : SdCardOperationException + { + /// + /// Initializes a new instance of the class. + /// + /// The raw response lines captured from the device, if any. + public SdCardListIncompleteException(IReadOnlyList rawDeviceResponse) + : base( + "The SD card directory listing did not complete: the device did not finish " + + "answering the file-list query, so the listing may be missing entries or be " + + "absent entirely. This is not an empty SD card — check the connection and retry.", + rawDeviceResponse) + { + } + } +}