Repository navigation
feat(sd): enable SD-card offload over WiFi/TCP (closes #1) - #2
Conversation
PR Summary by QodoEnable SD-card file offload over WiFi/TCP
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
/improve |
|
/agentic_review |
PR Code Suggestions ✨Latest suggestions up to 5b540c3 Warning
Previous suggestionsSuggestions up to commit 77bed16
✅ Suggestions up to commit dec05fc
✅ Suggestions up to commit 18f7971
Suggestions up to commit 74bc67e
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| SelectedDevice is { IsConnected: true } && | ||
| SelectedDevice.ConnectionType is ConnectionType.Usb or ConnectionType.Wifi; |
There was a problem hiding this comment.
2. Selecteddevice double-read race 🐞 Bug ≡ Correctness
CanAccessSdCard reads SelectedDevice twice, so if SelectedDevice becomes null between the two evaluations (e.g., selection/disconnect reentrancy), the second access can throw. Capturing SelectedDevice in a local before checking avoids the race and keeps the result consistent.
Agent Prompt
### Issue description
`CanAccessSdCard` evaluates `SelectedDevice` twice; if `SelectedDevice` changes to `null` between evaluations, `SelectedDevice.ConnectionType` can throw. Even without null, the result can be inconsistent across two different instances.
### Issue Context
This is defensive hardening: it narrows a small but real race window and makes the property evaluation atomic.
### Fix Focus Areas
- Daqifi.Avalonia/Daqifi.Desktop/ViewModels/DeviceLogsViewModel.cs[46-57]
- Daqifi.Avalonia/Daqifi.Desktop/ViewModels/DeviceLogsViewModel.cs[106-108]
### Suggested change
Rewrite as:
```csharp
public bool CanAccessSdCard
{
get
{
var device = SelectedDevice;
return device is { IsConnected: true } &&
device.ConnectionType is ConnectionType.Usb or ConnectionType.Wifi;
}
}
```
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 74bc67e |
|
Pass-2 dispositions (commit 18f7971):
|
|
/improve |
|
/agentic_review |
|
Persistent suggestions updated to latest commit 18f7971 |
|
Code review by qodo was updated up to the latest commit 18f7971 |
|
/improve |
|
/agentic_review |
|
Persistent suggestions updated to latest commit dec05fc |
|
Code review by qodo was updated up to the latest commit dec05fc |
|
Batch disposition of the open
The blocking-tier bug Refresh overwrites disconnect reset is fixed in b249fff (post-await connectivity re-check in |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit b249fff |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 48b1b97 |
|
|
Upstream enabler opened: daqifi-core PR #347 — allow SD file transfer over WiFi on firmware >= v3.7.0. It replaces Core's blanket USB-only SD-download guard with a firmware-capability gate (WiFi SD requires fw >= v3.7.0, per firmware #598/#599). Once that merges + releases, this PR bumps the Core package and comes out of draft. Per user direction (full path, firmware-version-gated). |
|
Code review by qodo was updated up to the latest commit 6677367 |
…ading by device state The previous commit's fix over-corrected. Downgrading every failure to Warning whenever the device was no longer current also silenced genuine app defects that merely coincided with a disconnect or a device switch, dropping their Error/Sentry report — the inverse of the false-positive it set out to fix. Move the decision to where it belongs: the classifier, keyed on exception TYPE. A link that dies under an in-flight SD operation throws TransportNotConnectedException (the typed error AbstractStreamingDevice.IsTransportDisconnectedError already keys on) or ObjectDisposedException from a torn-down stream; both are now expected device conditions — Warning, no Sentry, "reconnect and try again" — and card-unavailable, so a batch import stops rather than re-failing every remaining file against a dead link. RefreshFilesAsync goes back to logging unconditionally through LogFailure. Whether an exception is a disconnect artifact is a property of the exception, not of the ambient device state at the moment it was caught, so severity is now decided in exactly one place. Claude-Session: https://claude.ai/code/session_019fhAAGrHshwojdLJ99DUhc
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 95c096a |
… exception only ObjectDisposedException was too broad. An SD import also disposes a DbContext and file streams, so a disposal defect in parsing or the database write would have been classified as transport loss — downgraded to Warning, told the user to reconnect, and marked the card unavailable, hiding a real bug behind a wrong diagnosis. Keep only TransportNotConnectedException, which is unambiguous. A suppressed Error costs more than a noisy one, so anything less specific stays on the Error path. This is where the disconnect-logging cascade converges: state-based downgrade (too broad, hid coincidental defects) -> type-based (right idea) -> typed transport exception only (precise). Claude-Session: https://claude.ai/code/session_019fhAAGrHshwojdLJ99DUhc
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 928e68e |
…untyped disconnect guard
1. HIGH — an ordinary disconnect still reached the Error/Sentry path. Core does NOT use its
typed transport exception for its own connectivity guards: every SD entry point opens
with a plain InvalidOperationException("Device is not connected."), and the text path
adds "...disposing or disconnecting" variants. Worse, Disconnect() sets _isDisconnecting
and updates Status BEFORE the typed check deeper in ExecuteTextCommandAsync is reached,
so in a real race the untyped guard always wins. Narrowing to the typed exception last
pass therefore left the original false positive intact — pressing Disconnect mid-refresh
would file a Sentry issue.
Match those guards explicitly. Message matching is fragile and is used only because Core
exposes no type here; filed upstream as daqifi-core#395 to give the guards a typed
exception, and this arm retires when that lands. Our own SD_UNAVAILABLE_MESSAGE is
deliberately NOT matched — "no SD support" is a different condition, and reporting it as
a lost connection would send the user to reconnect a healthy device.
2. MEDIUM — an unreachable device was rendered as "SD card OK · 0 files". Core's text
exchange returns the lines collected so far when its response window elapses rather than
throwing, and its list parser reads "no error lines, no content lines" as an empty
directory — so a device that never answered is indistinguishable from an empty card. Over
USB this barely mattered (an unplug faults the serial transport and Core throws), but
issue #1 puts SD on WiFi, where a silently dead or merely congested link produces exactly
this shape. The user would be told the card is fine and empty while their files sit on an
unreachable device.
Corroborate an empty list with the storage query, whose parser cannot read a missing reply
as success and throws when nothing comes back. The exception propagates into the existing
catch, so SdCardFailureClassifier reports it correctly with no view-model change. One
extra round-trip, only on the empty-list path.
Claude-Session: https://claude.ai/code/session_019fhAAGrHshwojdLJ99DUhc
|
/agentic_review |
| if (files.Count == 0) | ||
| { | ||
| Task.Run(() => coreDevice.GetSdCardStorageAsync()).GetAwaiter().GetResult(); |
There was a problem hiding this comment.
1. Storage check cannot validate list 🐞 Bug ≡ Correctness
A successful storage query only proves the device answered a later request; it cannot distinguish an actually empty directory from a selectively lost, truncated, or timed-out LIST response. In that case, RefreshSdCardFiles publishes the ambiguous empty result and the UI reports the card as healthy with no files.
Agent Prompt
## Issue description
The independent storage query does not validate that the preceding empty LIST response was complete. A LIST-specific timeout or response loss followed by a successful storage request is still presented as a healthy empty card.
## Issue Context
The storage request is useful for detecting a completely unresponsive device, but it cannot prove directory emptiness. Require an explicitly complete LIST response, retry LIST, or preserve an unknown/error state when LIST completion cannot be established.
## Fix Focus Areas
- Daqifi.Avalonia/Daqifi.Desktop/Device/AbstractStreamingDevice.cs[1159-1177]
- Daqifi.Avalonia/Daqifi.Desktop/ViewModels/DeviceLogsViewModel.cs[428-446]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit e89ea7f |
…am root cause Qodo is right that a successful storage query proves the device answered a LATER request, not that the listing was complete — a LIST lost or truncated while the link then recovered still reads as an empty card. The corroboration narrows the ambiguity rather than removing it, and the comment now says so instead of implying a guarantee (the fourth comment on this PR corrected for over-claiming). The residual is not fixable at this layer: only Core knows a reply never arrived. Filed as daqifi-core#396, with three suggested fixes — the useful one also covers truncation, which no downstream mitigation can reach. Accepted here as the strict improvement over reporting every unreachable device as a healthy empty card. Claude-Session: https://claude.ai/code/session_019fhAAGrHshwojdLJ99DUhc
Converged — dispositioning the last finding and mergingAdversarial audit: PASS / clean on the final state (2/2 legs, 636 KB of transcript, engagement verified — the auditors traced Core's guards and the storage-query parser directly). Disposition: "Storage check cannot validate LIST" — ACCEPTED AS A KNOWN RESIDUALQodo is correct, and the point is now documented in the code and filed upstream. A successful storage query proves the device answered a later request; it cannot prove the listing itself was complete. A LIST that was lost or truncated while the link then recovered still reads as an empty card. Why this is dispositioned rather than fixed:
The cascade, and where it settledFive audit rounds on this PR, and the disconnect-classification question alone took four iterations, each correcting the previous over-correction:
Severity across rounds: high → clean → high/medium → clean → low-residual. That flattening is the convergence signal. Upstream filed from this PR
|
Summary
SD-card file LIST / download / delete now works over WiFi/TCP, not just USB. The USB-only restriction was a stale client-side policy that predated the firmware's TCP SD-reply routing (#598/#599). This is a big mobile win — WiFi SD offload with no OTG cable.
Why this is safe (transport-agnostic all the way down)
daqifi-nyquist-firmware#598/#599):sd_reply_write_tcp()writes SD READ/LIST replies to the TCP server;sd_card_manager_DataReadyCBpicks the target fromreplyTarget == SD_CARD_REPLY_WIFI_TCPand tracks the originating connection generation (mid-transfer client change aborts cleanly — no cross-client leak).Daqifi.Core1.0.0):GetSdCardFilesAsync/DownloadSdCardFileAsyncdrive a protobuf-consumer pause/swap againstIStreamTransport.Stream— identical for TCP and serial. They defensivelyStopStreamingfirst, toggle the SD interface, and restore the LAN interface in afinally(correct for a WiFi link). No transport-type gate anywhere in the Core SD path orSdCardFileReceiver.ConnectionType == Usbchecks.Changes (SD file access path only)
WiFiDevice/DaqifiStreamingDevice.csCoreDeviceForSd => CoreDevice(was null → SD unavailable by construction), mirroring the serial/USB devicesAbstractStreamingDevice.RefreshSdCardFilesConnectionType != Usbthrow; availability gated byCoreDeviceForSd(null →SD_UNAVAILABLE).EnsureSdOperationsQuiescedstill blocks an active stream/log ("Stop streaming first") — covers the issue's streaming-interaction caveat, transport-agnostically. Download/Delete already relied onCoreDeviceForSd.DeviceLogsViewModelCanAccessSdCard+RefreshFilesAsyncguard acceptUsborWifi;ConnectionTypeMessageWiFi branch → "SD Card Access Available";Has*doc comments de-USB-ifiedDeviceLogsView.axaml/DeviceLogsMobileView.axaml!CanAccessSdCardcallout is now a generic "connect a device (USB or Wi-Fi)" empty state, not "USB required"Scope boundary
SD logging (
StartSdCardLogging/StopSdCardLogging— streaming to the SD card) stays USB-gated. That's a separate feature outside this issue.Divergence note
Touches
@port-marked generated files. This is a downstream enhancement beyond upstream WPF (daqifi/daqifi-desktopcarries the sameCanAccessSdCard => Usbgate) — to be reconciled/pushed upstream on next sync.Verification
Daqifi.Avalonialibrary build 0 errors (mobile XAML compiles in the shared library).SD:LIST?+SD:GETvalidation (large-file download +__END_OF_FILE__framing) per the issue's must-verify caveat. Code-complete, hardware-unverified.Closes #1.
🤖 Generated with Claude Code