feat: distinguish SD card error states in GetSdCardFilesAsync

[tylerkron]

Summary

Closes #181.

GetSdCardFilesAsync silently swallowed SCPI errors and returned an empty list, making real failures (no card, corrupt FS, busy SD subsystem) indistinguishable from "directory is empty." A corrupt SD card on firmware 3.4.4 sent multiple debug sessions chasing phantom firmware bugs before someone thought to swap the card.

This adds a typed exception hierarchy and classifies the response after the existing retry loop so callers can show actionable detail.

What changed

• New exception hierarchy in Daqifi.Core.Device.SdCard:
  • SdCardOperationException — base; carries RawDeviceResponse and LastScpiError
  • SdCardNotPresentException — "No SD Card Detected"
  • SdCardFilesystemException — "Failed to open directory" with the device's raw error text in DeviceMessage
  • SdCardBusyException — defined for completeness (firmware cannot distinguish busy from timeout on the wire, so this currently surfaces as the base type)
• DaqifiStreamingDevice.GetSdCardFilesAsync now calls a private ThrowIfSdCardListError classifier after the retry loop. Specific firmware markers take precedence over the generic content/error check; an empty directory still returns an empty list.
• ISdCardOperations xmldoc updated to document the new exceptions.

Behavior preserved

• Empty directory (no errors, no content lines) → returns empty list (unchanged).
• File lines present, even with stray interleaved **ERROR lines → returns parsed files (unchanged).
• LAN interface is still restored in the finally block on the throwing path.

Hardware verification

Tested on /dev/cu.usbmodem2101 via the example app:

• Card with files → returns files (no exception)
• No card → throws SdCardNotPresentException (previously silently returned 0 files)
• A "corrupt" card available for testing returned an empty SD:LISt? response at the firmware level (i.e. firmware mounted it and saw 0 files), so the SdCardFilesystemException and bare-**ERROR paths are exercised by unit tests against the firmware wire formats from the issue.

Test plan

• Unit tests pass on net9.0 + net10.0 (878 passing, 0 failing)
• New tests cover: not-present, filesystem-error (with and without SCPI co-error), persistent-SCPI-error, files-with-interleaved-error, empty-directory, LAN-restore-on-error
• Updated test that previously asserted empty list on persistent SCPI error now asserts SdCardOperationException
• Manual verification on physical device for happy path and not-present path

🤖 Generated with Claude Code

[qodo-code-review]

Review Summary by Qodo

Distinguish SD card error states with typed exceptions

✨ Enhancement 🧪 Tests

Walkthroughs

Description

• Add typed exception hierarchy for SD card operations
  - SdCardOperationException base class with RawDeviceResponse and LastScpiError
  - SdCardNotPresentException for missing SD card
  - SdCardFilesystemException for corrupt/unreadable filesystem
  - SdCardBusyException for busy device (defined for completeness)
• Implement error classification in GetSdCardFilesAsync after retry loop
  - Specific firmware markers take precedence over generic checks
  - Empty directories still return empty list (behavior preserved)
  - File lines present with interleaved errors still return files (behavior preserved)
• Update XML documentation for ISdCardOperations and DaqifiStreamingDevice
• Expand test coverage with 7 new test cases for error scenarios

Diagram

flowchart LR
  A["GetSdCardFilesAsync"] --> B["Retry loop"]
  B --> C["ThrowIfSdCardListError"]
  C --> D1["No SD Card Detected"]
  C --> D2["Failed to open directory"]
  C --> D3["Generic SCPI error"]
  C --> D4["Empty directory"]
  D1 --> E1["SdCardNotPresentException"]
  D2 --> E2["SdCardFilesystemException"]
  D3 --> E3["SdCardOperationException"]
  D4 --> E4["Return empty list"]

Loading

File Changes

1. src/Daqifi.Core/Device/SdCard/SdCardOperationException.cs ✨ Enhancement +42/-0

Base exception for SD card operations

• New base exception class for SD card operation failures
• Carries RawDeviceResponse and LastScpiError for diagnostics
• Provides structured access to device error details

src/Daqifi.Core/Device/SdCard/SdCardOperationException.cs

---

2. src/Daqifi.Core/Device/SdCard/SdCardNotPresentException.cs ✨ Enhancement +26/-0

Exception for missing SD card

• New exception for missing SD card condition
• Inherits from SdCardOperationException
• Detected from firmware "No SD Card Detected" response

src/Daqifi.Core/Device/SdCard/SdCardNotPresentException.cs

---

3. src/Daqifi.Core/Device/SdCard/SdCardFilesystemException.cs ✨ Enhancement +44/-0

Exception for filesystem errors

• New exception for SD card filesystem errors
• Includes DeviceMessage property with raw error text
• Detected from firmware "[Error:N]Failed to open directory" response

src/Daqifi.Core/Device/SdCard/SdCardFilesystemException.cs

---

View more (4)

4. src/Daqifi.Core/Device/SdCard/SdCardBusyException.cs ✨ Enhancement +26/-0

Exception for busy SD card

• New exception for busy SD card condition
• Defined for completeness (firmware cannot distinguish busy from timeout)
• Inherits from SdCardOperationException

src/Daqifi.Core/Device/SdCard/SdCardBusyException.cs

---

5. src/Daqifi.Core/Device/SdCard/ISdCardOperations.cs 📝 Documentation +3/-0

Document SD card exception types

• Updated XML documentation for GetSdCardFilesAsync
• Documents three new exception types that can be thrown
• Clarifies that empty directories return empty list rather than throwing

src/Daqifi.Core/Device/SdCard/ISdCardOperations.cs

---

6. src/Daqifi.Core/Device/DaqifiStreamingDevice.cs ✨ Enhancement +60/-0

Implement SD card error classification logic

• Added ThrowIfSdCardListError private method to classify SD card errors
• Checks for specific firmware markers (no card, filesystem error) first
• Falls back to generic SCPI error handling
• Preserves behavior for empty directories and files with interleaved errors
• Updated XML documentation for GetSdCardFilesAsync with new exceptions
• Added IsScpiErrorLine helper method for error line detection

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs

---

7. src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs 🧪 Tests +135/-4

Expand SD card error handling test coverage

• Changed existing test to assert SdCardOperationException instead of empty list
• Added 6 new test cases covering error scenarios
• Tests include: no card detected, filesystem error (with/without SCPI), files with interleaved
 errors, empty directory, LAN restore on error
• Validates exception properties (LastScpiError, RawDeviceResponse, DeviceMessage)

src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

1. Duplicate error-line predicate ☑ 🐞 Bug ⚙ Maintainability

Description

ContainsScpiError and IsScpiErrorLine implement the same predicate logic separately, increasing
the chance retry behavior and final classification diverge if one is changed without the other.

Code

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[R740-748]

            });
        }

+        private static bool IsScpiErrorLine(string line)
+        {
+            var trimmed = line.TrimStart();
+            return trimmed.StartsWith("**ERROR", StringComparison.OrdinalIgnoreCase)
+                || trimmed.StartsWith("ERROR", StringComparison.OrdinalIgnoreCase);
+        }

Evidence

Both methods currently check the same TrimStart().StartsWith("**ERROR") || StartsWith("ERROR")
condition; the retry loop calls ContainsScpiError while classification calls IsScpiErrorLine, so
duplication risks inconsistent future edits.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[726-748]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`ContainsScpiError` duplicates the same logic as `IsScpiErrorLine`. This is a maintenance trap because retry behavior (uses `ContainsScpiError`) and classification behavior (uses `IsScpiErrorLine`) can drift.

### Issue Context
This code path is central to SD card list retries and the new typed-exception classification.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[726-748]

### Suggested change
- Implement the predicate once (prefer `IsScpiErrorLine`) and have `ContainsScpiError` delegate:
 - `return lines.Any(IsScpiErrorLine);`
- If you adopt the stricter SCPI-only detection from the other finding, apply it in one place so retry + classification stay consistent.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. LastScpiError misclassified ☑ 🐞 Bug ≡ Correctness

Description

ThrowIfSdCardListError derives lastScpiError using IsScpiErrorLine, but that predicate matches
any line starting with "ERROR" (not just SCPI **ERROR:/ERROR: lines), so firmware text like
"Error !! ..." can end up in SdCardOperationException.LastScpiError and in the generic exception
message when the response doesn't match the two specific markers.

Code

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[R743-792]

+        private static bool IsScpiErrorLine(string line)
+        {
+            var trimmed = line.TrimStart();
+            return trimmed.StartsWith("**ERROR", StringComparison.OrdinalIgnoreCase)
+                || trimmed.StartsWith("ERROR", StringComparison.OrdinalIgnoreCase);
+        }
+
+        /// <summary>
+        /// Inspects the final response from a <c>SYSTem:STORage:SD:LISt?</c> exchange
+        /// and throws a typed <see cref="SdCardOperationException"/> when the device
+        /// reported a real failure (no SD card, filesystem error, generic SCPI error).
+        /// If any non-error/non-empty line is present, callers proceed to parse — even
+        /// if SCPI error lines are interleaved — so a successful directory listing is
+        /// never masked by stray transient errors.
+        /// </summary>
+        private static void ThrowIfSdCardListError(IReadOnlyList<string> lines)
+        {
+            var lastScpiError = lines.LastOrDefault(IsScpiErrorLine)?.Trim();
+
+            // Specific firmware-emitted error markers take precedence over generic
+            // content/error checks. They're plain text (not SCPI-shaped), so a
+            // simple "is there any content line?" check would otherwise miss them
+            // and pass garbage to the parser.
+            if (lines.Any(l => l.IndexOf("No SD Card Detected", StringComparison.OrdinalIgnoreCase) >= 0))
+            {
+                throw new SdCardNotPresentException(lines, lastScpiError);
+            }
+
+            var filesystemErrorLine = lines.FirstOrDefault(l =>
+                l.IndexOf("Failed to open directory", StringComparison.OrdinalIgnoreCase) >= 0);
+            if (filesystemErrorLine != null)
+            {
+                throw new SdCardFilesystemException(lines, lastScpiError, filesystemErrorLine.Trim());
+            }
+
+            // If any line looks like a real result (non-empty, non-SCPI-error),
+            // hand off to the parser even if SCPI error lines are interleaved.
+            var hasContentLine = lines.Any(line =>
+                !string.IsNullOrWhiteSpace(line) && !IsScpiErrorLine(line));
+            if (hasContentLine)
+            {
+                return;
+            }
+
+            if (lastScpiError != null)
+            {
+                throw new SdCardOperationException(
+                    "The SD card list operation failed: " + lastScpiError,
+                    lines,
+                    lastScpiError);

Evidence

IsScpiErrorLine treats any leading "ERROR" as a SCPI error line, and ThrowIfSdCardListError uses
the last such line as lastScpiError. However, SdCardOperationException.LastScpiError is
explicitly documented as “the last SCPI error line” (example **ERROR: -200, "Execution error"),
while tests show firmware marker lines like "Error !! No SD Card Detected" that also match the
broad predicate.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[726-796]
src/Daqifi.Core/Device/SdCard/SdCardOperationException.cs[22-40]
src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs[574-594]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`ThrowIfSdCardListError` currently uses a broad “starts with ERROR” check to populate `LastScpiError`, but `LastScpiError` is documented as a SCPI-formatted error line. Firmware marker text like `Error !! ...` can match the broad predicate and be surfaced as `LastScpiError`, which is misleading for callers.

### Issue Context
- Known marker strings like `No SD Card Detected` are handled before the generic path, but any future firmware error text that begins with `Error` and does *not* match the two explicit markers can flow into the generic `SdCardOperationException` path with a non-SCPI `LastScpiError`.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[726-796]
- src/Daqifi.Core/Device/SdCard/SdCardOperationException.cs[22-40]

### Suggested change
- Split the concepts:
 - `IsScpiErrorLine`: only match actual SCPI error formats (e.g., starts with `"**ERROR"` or `"ERROR:"`), and use *that* to compute `lastScpiError`.
 - If you still want to treat `Error !! ...` lines as “non-file/non-result” lines, handle them separately (e.g., `IsNonResultLine`) without feeding them into `LastScpiError`.
- Keep raw firmware marker lines in `RawDeviceResponse` for diagnostics.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[tylerkron]

Thanks @qodo-code-review — both findings addressed in d0dda48.

Bug #1 — LastScpiError misclassified: Agreed. Tightened IsScpiErrorLine to require **ERROR or ERROR: (with the colon), so firmware status text like Error !! No SD Card Detected no longer ends up in LastScpiError. The non-SCPI text is still preserved in RawDeviceResponse for diagnostics. Added a separate IsNonResultLine (permissive) for the hasContentLine check, so firmware text is still recognized as a non-result line — just not as a SCPI error.

Also added a defensive fallback in ThrowIfSdCardListError: if a response contains only firmware status text with no SCPI error and no recognized marker, we throw SdCardOperationException (with LastScpiError = null) rather than silently returning an empty list. Shouldn't happen for known firmware paths, but it's a meaningful safety net for future firmware revisions.

Bug #2 — Duplicate predicate: Agreed. ContainsScpiError now delegates to IsScpiErrorLine so retry triggering and classification share a single source of truth.

New tests cover both behaviors:

• GetSdCardFilesAsync_LastScpiError_ContainsOnlyScpiFormattedLine — verifies LastScpiError is the SCPI line, not the firmware text, even when both are present.
• GetSdCardFilesAsync_WithFirmwareTextOnly_ThrowsWithNullLastScpiError — verifies the defensive fallback path.

Full suite still green: 880 passing on net9.0 + net10.0.