[tylerkron]

Summary

Fixes silently-broken SD card logging against current firmware.

Firmware v3.5.0 hard-renamed the SD logging filename command from SYSTem:STORage:SD:LOGging "file" to SYSTem:STORage:SD:FILE "file" with no backward-compatible alias (daqifi-nyquist-firmware#323). daqifi-core's SetSdLoggingFileName has been emitting the old name ever since, so on any v3.5.0+ device the firmware rejects it with -113 "Undefined header".

Because StartSdCardLoggingAsync issues the command fire-and-forget (no response check), the failure is silent — logging appears to start but the device records nothing.

This PR switches the single producer string to SYSTem:STORage:SD:FILE. No .NET API changes.

Note: the three other renames in the abandoned #168 (StartStreamData→STReam:START, StopStreamData→STReam:STOP, USB:SetTransparentMode→USB:TRANSparent:MODE) are not included — firmware kept those old names as aliases, so they work on all firmware and need no change. This PR is only the one command that has no alias.

Verified on hardware

Probed the connected device's SCPI error queue under identical conditions:

Command	Firmware response	Error queue
SYSTem:STORage:SD:LOGging "f" (old)	**ERROR: -113, "Undefined header"	-113,"Undefined header"
SYSTem:STORage:SD:FILE "f" (new)	(none)	0,"No error"

Testing

• dotnet build -c Release: 0 warnings, 0 errors (TreatWarningsAsErrors).
• Full suite (net9.0): 1053 passed, 0 failed, 2 skipped.

⚠️ Breaking change / firmware floor

SD card logging now requires device firmware v3.5.0 or newer. There is no single command name that works on both sides of the v3.5.0 cutover (old firmware only accepts SD:LOGging, new firmware only accepts SD:FILE), so this intentionally drops < v3.5.0 support for SD logging rather than version-gating.

Per discussion, universal firmware-version capability gating (which would let this select the command per device version instead of requiring a floor) is tracked in #251. The producer XML doc references it inline.

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Fix SD logging SCPI filename command for firmware v3.5.0+
🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 10-20 Minutes

Description

• Switch SD logging filename SCPI command to the firmware v3.5.0+ name.
• Document the firmware floor / rename rationale on the producer API.
• Update unit tests to assert the new SCPI command in SD logging sequences.

Diagram

graph TD
  A["Client code"] --> B["DaqifiStreamingDevice"] --> C["ScpiMessageProducer"] --> D["SCPI transport"] --> E["Device firmware v3.5+"] --> F[("SD card")]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Version-gate by firmware capabilities

• ➕ Maintains SD logging support for pre-v3.5.0 devices by emitting SD:LOGging there
• ➕ Keeps behavior correct across mixed fleets without requiring a firmware floor
• ➖ Requires reliable firmware version/capability discovery and plumbing it into the logging path
• ➖ Adds branching and test matrix complexity (and is tracked separately as investigate: firmware-version-aware feature gating (when did each SCPI feature land?) #251)

2. Optimistic dual-send with error handling

• ➕ Could support both firmware eras without explicit version detection (try SD:FILE, fallback to SD:LOGging)
• ➕ Minimal additional API surface
• ➖ Requires converting the current fire-and-forget step into a checked command (read response and/or drain error queue), which changes timing and failure modes
• ➖ Still needs careful handling to avoid polluting the stream or missing errors

3. Expose an override/config knob for SCPI command variant

• ➕ Allows advanced users to force old/new command without capability detection
• ➕ Keeps implementation simple internally
• ➖ Pushes protocol complexity to consumers and increases support burden
• ➖ Harder to ensure correct defaults across releases

Recommendation: Given the stated decision to require firmware v3.5.0+ for SD logging (and the absence of a backward-compatible alias in firmware), the PR’s approach is the simplest and most reliable immediate fix. If backward compatibility becomes necessary, prefer a capability/version-gated approach (tracked in #251) over a user-facing override; it keeps the API clean and avoids silent misconfiguration.

Files changed (3) +14 / -12

Bug fix (1) +4 / -2

ScpiMessageProducer.csRename SD logging filename SCPI command to SD:FILE and document firmware floor +4/-2

Rename SD logging filename SCPI command to SD:FILE and document firmware floor

• Updates SetSdLoggingFileName to emit 'SYSTem:STORage:SD:FILE "..."' instead of the deprecated 'SD:LOGging' header. Expands XML remarks to call out the v3.5.0+ requirement and the rename rationale/reference.

src/Daqifi.Core/Communication/Producers/ScpiMessageProducer.cs

Tests (2) +10 / -10

ScpiMessageProducerTests.csUpdate SCPI producer test for SD filename command rename +1/-1

Update SCPI producer test for SD filename command rename

• Adjusts the SetSdLoggingFileName unit test to assert the new 'SYSTem:STORage:SD:FILE' command string.

src/Daqifi.Core.Tests/Communication/Producers/ScpiMessageProducerTests.cs

SdCardOperationsTests.csUpdate SD logging operation tests to expect SD:FILE command +9/-9

Update SD logging operation tests to expect SD:FILE command

• Updates multiple SD logging operation tests to assert that StartSdCardLoggingAsync sends the renamed 'SYSTem:STORage:SD:FILE' command, including timestamped filename cases and format-specific sequences.

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

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

Context used

✅ Tickets: 🎫 investigate: firmware-version-aware feature gating (when did each SCPI feature land?)

1. Misleading filename quoting docs ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

The XML doc for SetSdLoggingFileName says the filename "must be enclosed in quotes", but the
producer itself adds quotes around fileName and device-side validation rejects quote characters.
This documentation mismatch can cause callers to supply incorrectly-quoted values and get invalid
SCPI strings or validation errors.

Code

src/Daqifi.Core/Communication/Producers/ScpiMessageProducer.cs[R144-145]

    /// <param name="fileName">The name of the file to create or append to. Must be enclosed in quotes.</param>
    /// <remarks>

Relevance

⭐⭐⭐ High

Team historically accepts fixing misleading XML docs vs behavior (e.g., baud-rate docs PR#98; SCPI
docs PR#160).

PR-#98
PR-#160

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The XML docs explicitly claim the filename must be quoted, while the implementation always adds
quotes and the device-level validator forbids quote characters.

src/Daqifi.Core/Communication/Producers/ScpiMessageProducer.cs[141-155]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[958-970]

Agent prompt

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

## Issue description
`SetSdLoggingFileName` XML docs currently instruct callers that `fileName` must be enclosed in quotes, but the method already wraps the value in quotes.

## Issue Context
At the device layer, `ValidateSdCardFileName` rejects `\"` characters, so callers following the current doc guidance can be misled into passing an invalid value.

## Fix Focus Areas
- src/Daqifi.Core/Communication/Producers/ScpiMessageProducer.cs[141-155]
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[958-970]

## Suggested fix
Update the `<param name="fileName">` text to state the filename should be provided *without* surrounding quotes (they are added by the producer). Optionally apply the same correction to other SD filename-producing methods for consistency.

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

---

2. Silent SD logging failure 🐞 Bug ☼ Reliability

Description

SetSdLoggingFileName now emits SYSTem:STORage:SD:FILE (documented as firmware v3.5.0+), but
StartSdCardLoggingAsync does not gate on firmware version or check for SCPI errors before setting
_isLoggingToSdCard/IsStreaming true. On older firmware this command will be rejected and logging
will not start, yet the library will report it as started and may leave subsequent SD operations in
an inconsistent state.

Code

src/Daqifi.Core/Communication/Producers/ScpiMessageProducer.cs[R147-155]

+    /// Command: SYSTem:STORage:SD:FILE "filename.bin"
+    /// Requires firmware v3.5.0 or newer; the command was renamed from
+    /// <c>SYSTem:STORage:SD:LOGging</c> and older firmware does not accept it (see daqifi-core#251).
    /// Example: messageProducer.Send(ScpiMessageProducer.SetSdLoggingFileName("data.bin"));
    /// </remarks>
    public static IOutboundMessage<string> SetSdLoggingFileName(string fileName)
    {
-        return new ScpiMessage($"SYSTem:STORage:SD:LOGging \"{fileName}\"");
+        return new ScpiMessage($"SYSTem:STORage:SD:FILE \"{fileName}\"");
    }

Relevance

⭐ Low

Similar “don’t set state until confirmed response” feedback was rejected (PR#60) and ack/error-check
refactors rejected (PR#130).

PR-#60
PR-#130

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The producer explicitly documents that the new SD:FILE command requires firmware v3.5.0+, but the
SD logging start path does not perform any firmware gating or SCPI error validation and still marks
logging as active unconditionally.

src/Daqifi.Core/Communication/Producers/ScpiMessageProducer.cs[141-155]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[515-575]
src/Daqifi.Core/Device/DeviceMetadata.cs[20-25]
src/Daqifi.Core/Device/DeviceMetadata.cs[76-97]
src/Daqifi.Core/Device/DaqifiDevice.cs[992-995]
src/Daqifi.Core/Firmware/FirmwareVersion.cs[46-88]

Agent prompt

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

## Issue description
`StartSdCardLoggingAsync` can now fail silently on devices with firmware < v3.5.0 because `SetSdLoggingFileName` sends `SYSTem:STORage:SD:FILE` (renamed command) and the start sequence does not validate firmware compatibility or check the SCPI error queue.

## Issue Context
- The producer docs state `SYSTem:STORage:SD:FILE` requires firmware v3.5.0+.
- The start sequence uses `Send(...)` and delays, then unconditionally sets `_isLoggingToSdCard = true` and `IsStreaming = true`.
- Firmware version is already tracked in `device.Metadata.FirmwareVersion` and can be compared via `FirmwareVersion.TryParse`.

## Fix Focus Areas
- src/Daqifi.Core/Communication/Producers/ScpiMessageProducer.cs[141-155]
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[515-575]
- src/Daqifi.Core/Device/DeviceMetadata.cs[20-25]
- src/Daqifi.Core/Firmware/FirmwareVersion.cs[46-88]

## Suggested fix
1. In `StartSdCardLoggingAsync`, parse `Metadata.FirmwareVersion` and throw a clear exception when it is < 3.5.0 (or when unparseable, consider a conservative probe/check).
2. Only set `_isLoggingToSdCard`/`IsStreaming` after the command sequence is known to be accepted (at minimum via the firmware guard; optionally by reading the SCPI error queue at the end of the sequence).

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

---

[tylerkron]

Response to Qodo review

1. Misleading filename quoting docs — ✅ fixed (9667bbd)

Good catch, and confirmed: SetSdLoggingFileName wraps the value in quotes itself, and ValidateSdCardFileName (DaqifiStreamingDevice.cs:1242) throws ArgumentException on filenames containing ". So a caller following the old "Must be enclosed in quotes" doc would actually get an exception. Reworded the <param> text to say the bare name should be passed without surrounding quotes. Applied the same correction to GetSdFile, which had the identical wrong text (DeleteSdFile was already correct).

2. Silent SD logging failure — ⏭️ intentionally deferred, not actioned here

This is a real latent issue, but out of scope for this PR by design:

• Pre-existing, not introduced here. StartSdCardLoggingAsync has always been fire-and-forget (Send(...) with no SCPI-error check), setting _isLoggingToSdCard/IsStreaming unconditionally. This PR only changes one command string; it doesn't add the silent-failure behavior.
• Firmware-version gating is explicitly deferred. The decision on this PR was to require firmware v3.5.0+ for SD logging rather than version-gate now. The general "select command / guard by firmware version" mechanism — including this exact start-path guard — is tracked in investigate: firmware-version-aware feature gating (when did each SCPI feature land?) #251 (which Qodo correctly picked up as ticket context). Adding an ad-hoc FirmwareVersion check here would pre-empt that holistic design.
• Qodo rates this Low and notes similar "don't set state until confirmed" / ack-check refactors were rejected before (PR#60, fix: add delays and channelMask to StartSdCardLoggingAsync, preserve download extension #130).

Folding it into #251 instead of bolting a one-off guard onto this fix.