fix: add LAN disable and SD stream interface setup to StartSdCardLoggingAsync - #153
Merged
Merged
Conversation
…ingAsync SD card and LAN share the SPI bus on the hardware. StartSdCardLoggingAsync was missing the DisableNetworkLan and SetStreamInterface(SdCard) commands that the desktop app was sending via its PrepareSdInterface() workaround. This caused SD card logging to silently produce no files when called from Core alone (e.g. via the example CLI app). StopSdCardLoggingAsync now also restores the stream interface to USB for USB connections so subsequent non-SD operations work correctly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: a0fb8798807b
Review Summary by QodoFix SD card logging by disabling LAN and configuring stream interface
WalkthroughsDescription• Disable LAN and set stream interface to SD card before logging • Restore stream interface to USB after SD card logging stops • Update tests to verify new 6-command sequence instead of 4 Diagramflowchart LR
A["StartSdCardLoggingAsync"] --> B["DisableNetworkLan"]
B --> C["EnableStorageSd"]
C --> D["SetStreamInterface SdCard"]
D --> E["SetSdLoggingFileName"]
E --> F["SetStreamFormat"]
F --> G["StartStreamData"]
H["StopSdCardLoggingAsync"] --> I["StopStreaming"]
I --> J["DisableStorageSd"]
J --> K["SetStreamInterface USB"]
File Changes1. src/Daqifi.Core/Device/DaqifiStreamingDevice.cs
|
Code Review by Qodo
1.
|
4 tasks
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
448
to
+456
| StopStreaming(); | ||
| Send(ScpiMessageProducer.DisableStorageSd); | ||
|
|
||
| // Restore stream interface to USB so subsequent non-SD operations work. | ||
| if (IsUsbConnection) | ||
| { | ||
| Send(ScpiMessageProducer.SetStreamInterface(StreamInterface.Usb)); | ||
| } | ||
|
|
There was a problem hiding this comment.
2. Lan not restored on stop 🐞 Bug ⛯ Reliability
StopSdCardLoggingAsync disables SD storage but never re-enables LAN after StartSdCardLoggingAsync disabled it, leaving the device in a LAN-disabled state after logging. This can break later network operations unless callers know to manually call PrepareLanInterface.
Agent Prompt
### Issue description
`StopSdCardLoggingAsync` does not restore the LAN interface after `StartSdCardLoggingAsync` disables it, leaving the device in a LAN-disabled state.
### Issue Context
The class already defines `PrepareLanInterface()` as the standard way to restore LAN (it disables SD storage and enables LAN). Other SD operations restore LAN in `finally` blocks.
### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[439-460]
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[272-285]
### Suggested fix
- In `StopSdCardLoggingAsync`, after `Send(DisableStorageSd)`, restore LAN using one of:
- Call `PrepareLanInterface()` (may resend DisableStorageSd but is idempotent), or
- Send `ScpiMessageProducer.EnableNetworkLan` explicitly.
- Consider adding a short settle delay (reuse `SD_INTERFACE_SETTLE_DELAY_MS`) if subsequent commands are likely to follow immediately.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
SD card logging requires USB because SD and LAN share the SPI bus. Add an early guard that throws InvalidOperationException for non-USB connections, matching the existing pattern in DownloadSdCardFileAsync. Also fixes StopSdCardLoggingAsync test to expect the new SetStreamInterface(Usb) restore command, and updates the download non-USB test to use a dedicated TestableNonUsbStreamingDevice. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: a0fb8798807b
cptkoolbeenz
added a commit
that referenced
this pull request
Jul 18, 2026
Two real defects the pre-merge review caught, both blocking the feature: - Qodo "WiFi SD ops disable LAN" (Correctness, high): PrepareSdInterface() unconditionally sent DisableNetworkLan to free the shared SPI bus for the SD card — correct for USB, but over WiFi it tears down the very TCP channel that carries the SD reply (codebase precedent: PRs #153/#159). Firmware #598/#599 disable the enable-level SPI mutex because the Harmony SPI driver arbitrates SD/WiFi transactions on the shared bus (app_freertos.c: "transaction-level arbitration", 7+ hours stable concurrent operation), and the reply routes back over the requesting interface. So PrepareSdInterface/PrepareLanInterface are now transport-aware: over WiFi the LAN is left enabled (only the SD subsystem is toggled); over USB the LAN disable/restore is unchanged. - Audit "overflowing firmware strings crash the gate" (crash, low): the regex matches an unbounded digit run for the major component but FirmwareVersion. TryParse used int.Parse (throws) for major vs int.TryParse (safe) for minor/patch — a TryParse that violates its no-throw contract. A corrupted/ spoofed DeviceFwRev like "999999999999999999.0.0" (untrusted wire string on the non-USB path the gate hardens) threw OverflowException instead of failing closed to FeatureNotSupportedException. Fixed at the root: major now uses int.TryParse and an out-of-range version is reported unparseable, symmetric with minor/patch. Tests: WiFi PrepareSdInterface/PrepareLanInterface leave-LAN-alone coverage + USB toggle regression; SD-over-WiFi LIST asserts LAN untouched; TryParse overflow cases; NetworkConfigurableTests device marked USB (its interface-prep assertions encode the USB sequence). Full suite 1404 passed / 0 failed / 2 skipped. Claude-Session: https://claude.ai/code/session_01PK5697Y1eDH3DB75Tii5Nh
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
StartSdCardLoggingAsyncnow sendsDisableNetworkLanandSetStreamInterface(SdCard)before enabling SD storage and starting streaming. SD card and LAN share the SPI bus on the hardware — without disabling LAN first, SD card logging silently produces no files.StopSdCardLoggingAsyncnow restores the stream interface to USB (for USB connections) so subsequent non-SD operations work correctly.Context
The desktop app had been working around this via its
PrepareSdInterface()method, which sent these commands before calling Core. When using Core directly (e.g. via the CLI example app), SD card logging would appear to start/stop successfully but no files were written to the card.Test plan
--sd-log-start --sd-log-format csvvia example CLI creates a file on the SD card--sd-log-start --sd-log-format protobufvia example CLI creates a file on the SD card--sd-listshows the new files after logging🤖 Generated with Claude Code