Skip to content

fix: re-enable LAN after stopping SD card logging#159

Merged
tylerkron merged 3 commits intomainfrom
fix/sd-card-lan-restoration
Apr 4, 2026
Merged

fix: re-enable LAN after stopping SD card logging#159
tylerkron merged 3 commits intomainfrom
fix/sd-card-lan-restoration

Conversation

@tylerkron
Copy link
Copy Markdown
Contributor

Summary

  • StopSdCardLoggingAsync() never re-enabled the LAN interface after stopping SD card logging. StartSdCardLoggingAsync() disables LAN because the SD card and WiFi share the SPI bus, but the stop method only disabled SD storage and restored the stream interface to USB — it never sent SYSTem:COMMunicate:LAN:ENAbled 1.
  • This left WiFi/LAN silently disabled after every SD logging session. Every other SD operation (GetSdCardFilesAsync, DeleteSdCardFileAsync, DownloadSdCardFileAsync) correctly restored LAN via PrepareLanInterface() in their finally blocks.
  • The existing test StopSdCardLoggingAsync_SendsCorrectCommands asserted only 3 commands (matching the buggy behavior). Updated to assert 4 commands including the LAN re-enable.

Test plan

  • All 195 SD card tests pass on both net8.0 and net9.0
  • Manual test: connect over serial, start SD logging, stop SD logging, verify device is reachable over WiFi

🤖 Generated with Claude Code

StartSdCardLoggingAsync disables LAN (SD card and WiFi share the SPI
bus), but StopSdCardLoggingAsync never re-enabled it. This left WiFi/LAN
silently disabled after SD logging was stopped. Every other SD operation
(GetSdCardFilesAsync, DeleteSdCardFileAsync, DownloadSdCardFileAsync)
correctly restored LAN in their finally blocks via PrepareLanInterface().

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Entire-Checkpoint: a0fb8798807b
@tylerkron tylerkron requested a review from a team as a code owner March 29, 2026 15:43
@qodo-code-review
Copy link
Copy Markdown

ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan

Review Summary by Qodo

Re-enable LAN after stopping SD card logging

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Re-enable LAN interface after stopping SD card logging
• StartSdCardLoggingAsync disables LAN (shared SPI bus), but StopSdCardLoggingAsync never restored
  it
• Left WiFi/LAN silently disabled after SD logging sessions
• Updated test to verify LAN re-enable command is sent
Diagram
flowchart LR
  A["StartSdCardLoggingAsync<br/>disables LAN"] --> B["SD card logging active"]
  B --> C["StopSdCardLoggingAsync"]
  C --> D["Stop stream data"]
  D --> E["Disable SD storage"]
  E --> F["Restore USB interface"]
  F --> G["Re-enable LAN interface<br/>NEW"]
  G --> H["LAN restored"]
Loading

Grey Divider

File Changes

1. src/Daqifi.Core/Device/DaqifiStreamingDevice.cs 🐞 Bug fix +4/-0

Add LAN re-enable to stop SD logging

• Added LAN re-enable command in StopSdCardLoggingAsync() method
• Sends SYSTem:COMMunicate:LAN:ENAbled 1 command after restoring USB interface
• Added explanatory comment about shared SPI bus between SD card and WiFi/LAN

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs


2. src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs 🧪 Tests +2/-1

Update test to verify LAN re-enable

• Updated StopSdCardLoggingAsync_SendsCorrectCommands test assertion from 3 to 4 commands
• Added assertion to verify LAN re-enable command is sent as fourth command
• Test now validates complete SD logging stop sequence including LAN restoration

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


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review bot commented Mar 29, 2026

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX Issues (0)

Grey Divider


Remediation recommended

1. Stop command can be skipped🐞 Bug ≡ Correctness
Description
StopSdCardLoggingAsync calls StopStreaming(), which is a no-op when IsStreaming is false; the
codebase already documents that IsStreaming can be stale, so this method can skip sending the
stop-stream SCPI command and still proceed to restore interfaces (including re-enabling LAN). This
can leave the device still streaming (or in an unexpected stream state) even though SD logging was
“stopped” locally.
Code

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[R464-467]

+            // Re-enable LAN interface. StartSdCardLoggingAsync disables LAN because
+            // the SD card and WiFi/LAN share the SPI bus on the hardware.
+            Send(ScpiMessageProducer.EnableNetworkLan);
+
Evidence
StopSdCardLoggingAsync invokes StopStreaming() and then re-enables LAN. StopStreaming() returns
early when IsStreaming is false, meaning the stop-stream SCPI command may not be sent. Elsewhere in
the same class, SD operations explicitly send the stop command regardless of IsStreaming because the
flag can be stale (issue #118), indicating this is a known failure mode and StopSdCardLoggingAsync
is inconsistent with that defensive pattern.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[439-471]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[161-176]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[295-306]

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

## Issue description
`StopSdCardLoggingAsync()` relies on `StopStreaming()`, which skips sending the SCPI stop-stream command when `IsStreaming` is false. The codebase already indicates `IsStreaming` can be stale, so this method should defensively send the stop command even if the flag is false—especially now that it re-enables LAN.
### Issue Context
Other SD operations (e.g., `GetSdCardFilesAsync`) unconditionally send `ScpiMessageProducer.StopStreaming` and then set `IsStreaming = false` due to known stale-state behavior.
### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[446-470]
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[165-176]
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[303-306]
### Suggested change
In `StopSdCardLoggingAsync()`, replace/augment `StopStreaming()` with the same defensive pattern used elsewhere:
- `Send(ScpiMessageProducer.StopStreaming);`
- `IsStreaming = false;`
Optionally keep `StopStreaming()` for symmetry but ensure the SCPI stop command is sent regardless of `IsStreaming`.

ⓘ 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

tylerkron and others added 2 commits April 3, 2026 22:01
StopStreaming() is a no-op when IsStreaming is false, but IsStreaming can
be stale (issue #118). Other SD operations already use the defensive
pattern of unconditionally sending the stop command. Apply the same
pattern here and add a test verifying the command is sent even when
IsStreaming is false.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@tylerkron tylerkron merged commit b9c6589 into main Apr 4, 2026
1 check passed
@tylerkron tylerkron deleted the fix/sd-card-lan-restoration branch April 4, 2026 04:07
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