Skip to content

refactor(scpi)!: update producers for firmware SCPI breaking renames#168

Open
cptkoolbeenz wants to merge 1 commit into
mainfrom
refactor/scpi-breaking-renames
Open

refactor(scpi)!: update producers for firmware SCPI breaking renames#168
cptkoolbeenz wants to merge 1 commit into
mainfrom
refactor/scpi-breaking-renames

Conversation

@cptkoolbeenz
Copy link
Copy Markdown
Member

Coordinates with daqifi-nyquist-firmware #311. Groups the 4 SCPI renames that affect .NET consumers into one PR for clean version compatibility. Merge after firmware #324 lands.

Coordinates with daqifi-nyquist-firmware #311 audit. Renames that
affect this .NET library (and thus daqifi-desktop via dependency):

## Command renames

| Producer method | Old command | New command | Firmware PR |
|---|---|---|---|
| SetSdLoggingFileName | SYSTem:STORage:SD:LOGging | SYSTem:STORage:SD:FILE | #323 (merged) |
| StartStreaming | SYSTem:StartStreamData | SYSTem:STReam:START | #324 |
| StopStreaming | SYSTem:StopStreamData | SYSTem:STReam:STOP | #324 |
| SetUsbTransparencyMode | SYSTem:USB:SetTransparentMode | SYSTem:USB:TRANSparent:MODE | #324 |

## Deleted: GetSdLoggingState

The `SYSTem:STORage:SD:LOGging?` query form referenced by this producer
method never existed in the firmware SCPI table — it would have failed
at runtime with -113 "Undefined header" against any device. Removing
the stale producer + its test.

## Why grouped

All four SCPI renames that affect .NET consumers landed in a single PR
so the version-compatibility story is simple: "daqifi-core N+1 pairs
with firmware that has merged #323 + #324". Renames that don't affect
.NET (Stats/ClearStats, TESTpattern, FAULTCLear, DIOProbe:ASSign) are
not touched here.

## Test updates

- ScpiMessageProducerTests.cs: 3 updated, 1 deleted (GetSdLoggingState)
- SdCardOperationsTests.cs: 20 string constants updated
- DaqifiDeviceInitializeTests.cs: 1 updated
- EndToEndTests.cs: 2 updated

No back-compat aliases. Breaking change by design — matches the
clean-break precedent established across the firmware rename PRs.
@cptkoolbeenz cptkoolbeenz requested a review from a team as a code owner April 19, 2026 19:04
@qodo-code-review
Copy link
Copy Markdown

Review Summary by Qodo

Update SCPI commands for firmware breaking renames

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Update SCPI command strings to match firmware breaking renames
  - SetSdLoggingFileName: SYSTem:STORage:SD:LOGging → SYSTem:STORage:SD:FILE
  - StartStreaming: SYSTem:StartStreamData → SYSTem:STReam:START
  - StopStreaming: SYSTem:StopStreamData → SYSTem:STReam:STOP
  - SetUsbTransparencyMode: SYSTem:USB:SetTransparentMode → SYSTem:USB:TRANSparent:MODE
• Remove stale GetSdLoggingState producer method and test
  - Query command never existed in firmware SCPI table
• Update all test assertions across four test files
  - 20+ test string constants updated to reflect new commands
Diagram
flowchart LR
  A["Old SCPI Commands"] -->|"SetSdLoggingFileName"| B["SYSTem:STORage:SD:FILE"]
  A -->|"StartStreaming"| C["SYSTem:STReam:START"]
  A -->|"StopStreaming"| D["SYSTem:STReam:STOP"]
  A -->|"SetUsbTransparencyMode"| E["SYSTem:USB:TRANSparent:MODE"]
  F["GetSdLoggingState"] -->|"Removed"| G["Stale Query"]
  B --> H["Updated Tests"]
  C --> H
  D --> H
  E --> H
Loading

Grey Divider

File Changes

1. src/Daqifi.Core/Communication/Producers/ScpiMessageProducer.cs ✨ Enhancement +8/-17

Update SCPI command strings for firmware renames

• Removed GetSdLoggingState property (stale query command)
• Updated SetSdLoggingFileName to use SYSTem:STORage:SD:FILE instead of SYSTem:STORage:SD:LOGging
• Updated StartStreaming to use SYSTem:STReam:START instead of SYSTem:StartStreamData
• Updated StopStreaming to use SYSTem:STReam:STOP instead of SYSTem:StopStreamData
• Updated SetUsbTransparencyMode to use SYSTem:USB:TRANSparent:MODE instead of
 SYSTem:USB:SetTransparentMode

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


2. src/Daqifi.Core.Tests/Communication/Producers/ScpiMessageProducerTests.cs 🧪 Tests +4/-12

Update producer tests for SCPI command renames

• Deleted GetSdLoggingState test method (stale query)
• Updated SetSdLoggingFileName test assertion to expect new command
• Updated StartStreaming test assertion to expect new command
• Updated StopStreaming test assertion to expect new command
• Updated SetUsbTransparencyMode test assertion to expect new command

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


3. src/Daqifi.Core.Tests/Device/DaqifiDeviceInitializeTests.cs 🧪 Tests +1/-1

Update device initialization test assertion

• Updated InitializeAsync test to expect SYSTem:STReam:STOP instead of SYSTem:StopStreamData

src/Daqifi.Core.Tests/Device/DaqifiDeviceInitializeTests.cs


View more (2)
4. src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs 🧪 Tests +20/-20

Update SD card operations test assertions

• Updated 14 test assertions across multiple test methods
• Changed SYSTem:STORage:SD:LOGging to SYSTem:STORage:SD:FILE in 8 assertions
• Changed SYSTem:StartStreamData to SYSTem:STReam:START in 4 assertions
• Changed SYSTem:StopStreamData to SYSTem:STReam:STOP in 2 assertions

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


5. src/Daqifi.Core.Tests/Integration/EndToEndTests.cs 🧪 Tests +2/-2

Update end-to-end test assertions

• Updated EndToEnd_MockTransport test to expect new streaming command strings
• Changed SYSTem:StartStreamData to SYSTem:STReam:START
• Changed SYSTem:StopStreamData to SYSTem:STReam:STOP

src/Daqifi.Core.Tests/Integration/EndToEndTests.cs


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review Bot commented Apr 19, 2026

Code Review by Qodo

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

Grey Divider


Action required

1. SD logging state query removed 🐞 Bug ≡ Correctness
Description
ScpiMessageProducer.GetSdLoggingState was removed (not renamed), breaking any consumers that
compile against it and eliminating a SD logging-state query from the public producer surface. This
is a functional/API removal beyond the four SCPI renames described in the PR.
Code

src/Daqifi.Core/Communication/Producers/ScpiMessageProducer.cs[L98-106]

-    /// <summary>
-    /// Creates a query message to get the current SD card logging state.
-    /// </summary>
-    /// <remarks>
-    /// Command: SYSTem:STORage:SD:LOGging?
-    /// Example: messageProducer.Send(ScpiMessageProducer.GetSdLoggingState);
-    /// </remarks>
-    public static IOutboundMessage<string> GetSdLoggingState => new ScpiMessage("SYSTem:STORage:SD:LOGging?");
-
Evidence
In the SD-card section of ScpiMessageProducer, there is no longer any API to query SD logging
state; the SD-related commands now jump directly from enable/disable to file listing. This confirms
the query API is gone in the current PR state.

src/Daqifi.Core/Communication/Producers/ScpiMessageProducer.cs[79-106]

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

### Issue description
The public producer member for querying SD logging state was removed. This creates a breaking API change and removes functionality (ability to query SD logging state) rather than performing a rename.

### Issue Context
PR intent is described as "4 SCPI renames"; removal is a larger change and should either be replaced with the renamed query or explicitly preserved as an alias for compatibility.

### Fix Focus Areas
- src/Daqifi.Core/Communication/Producers/ScpiMessageProducer.cs[79-106]
- src/Daqifi.Core.Tests/Communication/Producers/ScpiMessageProducerTests.cs[61-93]

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



Remediation recommended

2. Simulator docs use old SCPI 🐞 Bug ⚙ Maintainability
Description
The producer now sends SYSTem:STReam:START/STOP, but simulator docs still specify
SYSTem:StartStreamData/StopStreamData, which will lead to incompatible simulator
implementations/tests. This creates a repo-internal source of truth conflict for the streaming
command names.
Code

src/Daqifi.Core/Communication/Producers/ScpiMessageProducer.cs[R252-265]

    public static IOutboundMessage<string> StartStreaming(int frequency)
    {
-        return new ScpiMessage($"SYSTem:StartStreamData {frequency}");
+        return new ScpiMessage($"SYSTem:STReam:START {frequency}");
    }

    /// <summary>
    /// Creates a command message to stop data streaming.
    /// </summary>
    /// <remarks>
-    /// Command: SYSTem:StopStreamData
+    /// Command: SYSTem:STReam:STOP
    /// Example: messageProducer.Send(ScpiMessageProducer.StopStreaming);
    /// </remarks>
-    public static IOutboundMessage<string> StopStreaming => new ScpiMessage("SYSTem:StopStreamData");
+    public static IOutboundMessage<string> StopStreaming => new ScpiMessage("SYSTem:STReam:STOP");
Evidence
The library’s SCPI producer has been updated to the new streaming commands, while the simulator
documentation continues to list the old command names as supported/required, creating inconsistent
guidance within the same repo.

src/Daqifi.Core/Communication/Producers/ScpiMessageProducer.cs[243-265]
docs/simulator/SIMULATOR_DESIGN.md[139-152]
docs/simulator/GITHUB_ISSUES.md[254-264]

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

### Issue description
Simulator documentation still references the legacy streaming commands (`SYSTem:StartStreamData` / `SYSTem:StopStreamData`) even though the .NET SCPI producer now emits `SYSTem:STReam:START` / `SYSTem:STReam:STOP`.

### Issue Context
These docs act as an implementation guide/contract for the simulator; leaving them stale makes simulator work incompatible with the updated client.

### Fix Focus Areas
- docs/simulator/SIMULATOR_DESIGN.md[139-152]
- docs/simulator/GITHUB_ISSUES.md[254-264]

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



Advisory comments

3. SD filename docs incorrect 🐞 Bug ⚙ Maintainability
Description
SetSdLoggingFileName XML docs say the filename must be quoted, but the implementation always adds
quotes, so callers following the docs can double-quote and emit an invalid SCPI command. The same
misleading statement exists for GetSdFile in the same section.
Code

src/Daqifi.Core/Communication/Producers/ScpiMessageProducer.cs[R123-132]

    /// <param name="fileName">The name of the file to create or append to. Must be enclosed in quotes.</param>
    /// <remarks>
    /// The specified file will be created if it doesn't exist, or appended to if it already exists.
-    /// Command: SYSTem:STORage:SD:LOGging "filename.bin"
+    /// Command: SYSTem:STORage:SD:FILE "filename.bin"
    /// 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}\"");
    }
Evidence
The XML <param> docs state the caller must include quotes, while the code interpolates the value
inside quotes itself ("{fileName}"), meaning the documented call contract does not match the
actual behavior.

src/Daqifi.Core/Communication/Producers/ScpiMessageProducer.cs[109-132]

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

### Issue description
The XML docs for SD filename parameters claim the caller must provide quotes, but the methods already add quotes in the SCPI message. This can cause callers to double-quote when following documentation.

### Issue Context
This is a docs/API contract mismatch (not a runtime bug by itself) but it leads to malformed SCPI when consumers follow the docs literally.

### Fix Focus Areas
- src/Daqifi.Core/Communication/Producers/ScpiMessageProducer.cs[109-132]

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


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

/// </remarks>
public static IOutboundMessage<string> DisableStorageSd => new ScpiMessage("SYSTem:STORage:SD:ENAble 0");

/// <summary>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Sd logging state query removed 🐞 Bug ≡ Correctness

ScpiMessageProducer.GetSdLoggingState was removed (not renamed), breaking any consumers that
compile against it and eliminating a SD logging-state query from the public producer surface. This
is a functional/API removal beyond the four SCPI renames described in the PR.
Agent Prompt
### Issue description
The public producer member for querying SD logging state was removed. This creates a breaking API change and removes functionality (ability to query SD logging state) rather than performing a rename.

### Issue Context
PR intent is described as "4 SCPI renames"; removal is a larger change and should either be replaced with the renamed query or explicitly preserved as an alias for compatibility.

### Fix Focus Areas
- src/Daqifi.Core/Communication/Producers/ScpiMessageProducer.cs[79-106]
- src/Daqifi.Core.Tests/Communication/Producers/ScpiMessageProducerTests.cs[61-93]

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

@cptkoolbeenz cptkoolbeenz changed the title refactor(scpi): update producers for firmware SCPI breaking renames refactor(scpi)!: update producers for firmware SCPI breaking renames Apr 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant