fix(network): reject WiFi reconfiguration over a WiFi/TCP transport (closes #352)

[tylerkron]

Summary

UpdateNetworkConfigurationAsync applies LAN settings with SYSTem:COMMunicate:LAN:APPLY (and later re-enables the interface), which restarts the WiFi module. Over a WiFi/TCP control connection that restart tears down the very channel carrying the command stream before the trailing SYSTem:COMMunicate:LAN:SAVE is delivered — leaving the device with the new config applied-but-not-persisted (lost on the next power cycle), or the tail of the sequence undelivered. (Follow-up from #347, Qodo thread at DaqifiStreamingDevice.cs.)

This was latent: UpdateNetworkConfigurationAsync is a public INetworkConfigurable method with no production callers today.

Approach — Option B from #352 (gate to USB)

Rather than the persist-first reorder (Option A), which depends on unverified firmware semantics for persisting applied-but-not-yet-enabled settings and still leaves the first APPLY restart racing the SAVE over the same link, this rejects reconfiguration over WiFi/TCP up front:

• New typed NetworkReconfigurationRequiresUsbException (derives from InvalidOperationException, so existing broad handlers keep working while callers can special-case the "reconnect over USB" guidance).
• The guard fires before any command is dispatched, so nothing is ever half-applied over WiFi.
• Over USB — where a WiFi-module restart cannot drop the control connection — the full sequence runs unchanged.

This inverts the deliberate "reconfig re-enables LAN even over WiFi" decision from #347; the obsolete _OverWifi_StillReEnablesLan regression test is replaced with tests for the new reject path and the unchanged USB path.

Tests

• UpdateNetworkConfigurationAsync_OverWifi_ThrowsRequiresUsbAndSendsNothing — WiFi transport throws and emits zero commands.
• UpdateNetworkConfigurationAsync_OverUsb_DoesNotThrowRequiresUsb — USB transport still sends APPLY/SAVE.
• Full suite green on net9.0 + net10.0 (1762 passed, 2 skipped) + MCP tests (23 passed). Release build 0-warning on both TFMs.

Bench

Not bench-validated: the method rewrites the device's WiFi SSID/password and restarts the module (disruptive), and the guard only affects the WiFi-transport path — fully covered by unit tests over a USB bench.

Not merging — opened for your review.

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Fix network reconfiguration by requiring USB transport (avoid WiFi/TCP drop)

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 20-40 Minutes

AI Description

• Reject UpdateNetworkConfigurationAsync over WiFi/TCP to prevent apply-without-save outcomes.
• Introduce a typed exception guiding callers to reconnect over USB.
• Replace obsolete WiFi regression test with USB/WiFi transport-guard coverage.

Diagram

graph TD
  A["Caller"] --> B["DaqifiStreamingDevice\nUpdateNetworkConfigurationAsync"] --> C{"Is USB\nconnection?"}
  C -->|"No (WiFi/TCP)"| D["Throw NetworkReconfiguration\nRequiresUsbException"]
  C -->|"Yes (USB)"| E["Send SCPI: APPLY → delay → LAN enable → SAVE"] --> F["Device WiFi module\nrestarts safely"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Reorder to SAVE-before-APPLY (persist-first)

• ➕ Could allow reconfiguration over WiFi/TCP without forcing USB access
• ➕ Avoids introducing a new exception path for non-USB callers
• ➖ Relies on firmware semantics for persisting settings that are not yet applied/enabled
• ➖ Still risks transport drop if any subsequent step restarts WiFi before the stream is fully delivered
• ➖ Harder to reason about correctness without bench/firmware verification

2. Firmware-side atomic command (APPLY+SAVE) or transactional reconfig primitive

• ➕ Eliminates host-side race conditions entirely
• ➕ Best UX: safe over any transport
• ➖ Requires firmware change and coordinated rollout/version gating
• ➖ Longer lead time than a host-side guard

3. Two-phase API: stage config then explicit reboot/apply (with reconnect handshake)

• ➕ Makes the disruptive restart explicit to callers
• ➕ Could support WiFi/TCP by expecting reconnect after apply
• ➖ More complex public API and state management
• ➖ Still susceptible to partial progress if callers do not complete the second phase

Recommendation: Current approach (fail-fast to USB) is the safest host-side mitigation: it prevents unrecoverable half-applied configurations by ensuring no commands are sent over a transport that will be torn down mid-sequence. Consider SAVE-before-APPLY only if firmware behavior is verified; the long-term robust solution is a firmware-side atomic/transactional reconfiguration primitive.

Files changed (4) +129 / -26

Enhancement (1) +57 / -0

NetworkReconfigurationRequiresUsbException.csIntroduce typed exception for WiFi/TCP reconfiguration attempts +57/-0

Introduce typed exception for WiFi/TCP reconfiguration attempts

• Adds 'NetworkReconfigurationRequiresUsbException' (sealed, derives from 'InvalidOperationException') with a default explanatory message and standard overloads, enabling callers to special-case the “reconnect over USB” guidance while preserving broad exception handling.

src/Daqifi.Core/Device/Network/NetworkReconfigurationRequiresUsbException.cs

Bug fix (1) +23 / -5

DaqifiStreamingDevice.csGuard network reconfiguration to USB and document WiFi/TCP failure mode +23/-5

Guard network reconfiguration to USB and document WiFi/TCP failure mode

• Adds an early 'IsUsbConnection' check in 'UpdateNetworkConfigurationAsync' that throws 'NetworkReconfigurationRequiresUsbException' before dispatching any SCPI messages. Expands XML documentation to explain why WiFi/TCP is unsafe and clarifies the unconditional LAN re-enable now runs only under the USB-only guard.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs

Tests (1) +42 / -21

NetworkConfigurableTests.csAdd transport-guard tests and remove obsolete WiFi re-enable regression +42/-21

Add transport-guard tests and remove obsolete WiFi re-enable regression

• Adds a WiFi/TCP-path test asserting 'UpdateNetworkConfigurationAsync' throws and sends zero commands, and a USB-path test asserting APPLY/SAVE commands still emit. Removes the previous regression test that asserted reconfiguration proceeds over non-USB transports.

src/Daqifi.Core.Tests/Device/Network/NetworkConfigurableTests.cs

Documentation (1) +7 / -0

INetworkConfigurable.csDocument USB-only requirement for network reconfiguration +7/-0

Document USB-only requirement for network reconfiguration

• Updates interface remarks and exception documentation to specify that network reconfiguration requires USB, since APPLY/enable restart WiFi and can drop WiFi/TCP control before SAVE persists changes.

src/Daqifi.Core/Device/Network/INetworkConfigurable.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. Docs imply streaming stops ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

The INetworkConfigurable.UpdateNetworkConfigurationAsync docs list “Stops any active streaming” as
step 1, but the new USB-only guard throws before reaching the StopStreaming() branch. If a caller
invokes this method over WiFi/TCP while streaming, streaming will continue even though the docs read
like the stop is part of the method’s behavior.

Code

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[R1248-1261]

+            // Reject reconfiguration over WiFi/TCP up front. ApplyNetworkLan (and the later
+            // EnableNetworkLan) restart the WiFi module, tearing down a WiFi/TCP control connection
+            // before the trailing SaveNetworkLan is delivered — leaving the new config
+            // applied-but-not-persisted (lost on power cycle) or the tail undelivered. Fail before
+            // sending any command so nothing is half-applied; the caller must reconfigure over USB
+            // (#352).
+            if (!IsUsbConnection)
+            {
+                throw new NetworkReconfigurationRequiresUsbException();
+            }
+
            // Stop streaming if active
            if (IsStreaming)
            {

Relevance

⭐⭐⭐ High

Team frequently fixes XML doc/behavior mismatches (accepted doc-drift fixes in PRs #160, #321,
#357).

PR-#160
PR-#321
PR-#357

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The interface documentation describes a step-by-step sequence starting with stopping streaming, but
the implementation now performs an early USB-only check and throws before the
IsStreaming/StopStreaming() block is reached, so the documented step order doesn’t apply to the
non-USB reject path.

src/Daqifi.Core/Device/Network/INetworkConfigurable.cs[16-50]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1234-1263]

Agent prompt

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

### Issue description
`UpdateNetworkConfigurationAsync` now throws `NetworkReconfigurationRequiresUsbException` before it evaluates `IsStreaming`/calls `StopStreaming()`. The interface docs still present a step list starting with “Stops any active streaming”, which can be read as unconditional behavior.

### Issue Context
This is intentionally a “send nothing / do nothing” reject path over WiFi/TCP, so it’s reasonable that streaming is not stopped. The docs should explicitly scope the step list to the USB path and/or note that the non-USB reject path throws before stopping streaming.

### Fix Focus Areas
- src/Daqifi.Core/Device/Network/INetworkConfigurable.cs[16-50]
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1234-1263]

### Suggested change
Update the XML docs (likely in `INetworkConfigurable.UpdateNetworkConfigurationAsync` remarks) to say that the numbered sequence applies only when the USB precondition is met, and that over non-USB transports the method throws before stopping streaming and before dispatching any commands.

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

---

Previous review results

Review updated until commit 13edece ⚖️ Balanced

Results up to commit b677b2f ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Docs imply streaming stops ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

The INetworkConfigurable.UpdateNetworkConfigurationAsync docs list “Stops any active streaming” as
step 1, but the new USB-only guard throws before reaching the StopStreaming() branch. If a caller
invokes this method over WiFi/TCP while streaming, streaming will continue even though the docs read
like the stop is part of the method’s behavior.

Code

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[R1248-1261]

+            // Reject reconfiguration over WiFi/TCP up front. ApplyNetworkLan (and the later
+            // EnableNetworkLan) restart the WiFi module, tearing down a WiFi/TCP control connection
+            // before the trailing SaveNetworkLan is delivered — leaving the new config
+            // applied-but-not-persisted (lost on power cycle) or the tail undelivered. Fail before
+            // sending any command so nothing is half-applied; the caller must reconfigure over USB
+            // (#352).
+            if (!IsUsbConnection)
+            {
+                throw new NetworkReconfigurationRequiresUsbException();
+            }
+
            // Stop streaming if active
            if (IsStreaming)
            {

Relevance

⭐⭐⭐ High

Team frequently fixes XML doc/behavior mismatches (accepted doc-drift fixes in PRs #160, #321,
#357).

PR-#160
PR-#321
PR-#357

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The interface documentation describes a step-by-step sequence starting with stopping streaming, but
the implementation now performs an early USB-only check and throws before the
IsStreaming/StopStreaming() block is reached, so the documented step order doesn’t apply to the
non-USB reject path.

src/Daqifi.Core/Device/Network/INetworkConfigurable.cs[16-50]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1234-1263]

Agent prompt

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

### Issue description
`UpdateNetworkConfigurationAsync` now throws `NetworkReconfigurationRequiresUsbException` before it evaluates `IsStreaming`/calls `StopStreaming()`. The interface docs still present a step list starting with “Stops any active streaming”, which can be read as unconditional behavior.

### Issue Context
This is intentionally a “send nothing / do nothing” reject path over WiFi/TCP, so it’s reasonable that streaming is not stopped. The docs should explicitly scope the step list to the USB path and/or note that the non-USB reject path throws before stopping streaming.

### Fix Focus Areas
- src/Daqifi.Core/Device/Network/INetworkConfigurable.cs[16-50]
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1234-1263]

### Suggested change
Update the XML docs (likely in `INetworkConfigurable.UpdateNetworkConfigurationAsync` remarks) to say that the numbered sequence applies only when the USB precondition is met, and that over non-USB transports the method throws before stopping streaming and before dispatching any commands.

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

---

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 13edece

[tylerkron]

Closing as superseded by #391, which fixes #352 by reordering two commands instead of gating reconfiguration to USB.

The problem this PR identified is real — a save that can be lost to the WiFi module restart leaves the device applied-but-not-persisted. What's changed is that the firmware semantic this PR treated as unverified has now been checked, and it doesn't support the USB-only conclusion.

The premise. This PR treats the dropped control connection as the defect to prevent. But moving a device onto a different network necessarily kills the connection carrying the command stream — that's the operation succeeding, not a race. Gating to USB doesn't avoid the drop; it just makes the workflow (switching hotspots in the field, no cable to hand) unreachable. The narrower defect is that Core saved last, so the save was what got lost.

Firmware. SCPI_LANSettingsSave memcpy's BOARDRUNTIME_WIFI_SETTINGS — the staged struct — straight to NVM, with no dependency on APPLY having run. LAN:ENAbled writes isEnabled into that same struct and restarts nothing. Bare LAN:APPLY never writes NVM, so applying after saving can't clobber the save. Persist-first is therefore valid, which is the assumption this PR's "Approach" section set aside as unverified.

Bench, on an Nq1 (WINC1500 fw 19.7.7):

Test	Result
Stage SSID + SAVE, no APPLY at all, cold boot	Persisted
Full persist-first sequence with a successful APPLY (every command 0,"No error"), reboot	Persisted through a real module REINIT + power cycle

So the reorder is:

before: SET… -> APPLY -> delay -> SD off -> LAN on -> SAVE   # SAVE stranded behind the restart
after:  SET… -> SD off -> LAN on -> SAVE -> APPLY -> delay   # durable before anything restarts

Worth noting #391 keeps _OverWifi_StillReEnablesLan (the #347 regression test this PR had to delete) and adds three ordering tests that fail against the old sequence.

One incidental finding for anyone testing APPLY on the bench: it returns -200 when the device is in standby (SYSTem:POWer:STATe? -> 0) because the radio isn't initialized — SYSTem:POWer:STATe 1 first. Unrelated to either PR, but it does reinforce the ordering: APPLY is the step that can fail, and under the old sequence a rejected apply still had a save queued behind it.