fix: batch of small bug fixes and test coverage (#204, #311, #282, #264)

[tylerkron]

Summary

Four small, independent fixes/test additions batched into one PR:

• closes Add parser test using real captured device payload #204 — Ports the real-captured-payload DaqifiOutMessage parser test from daqifi-desktop (which is removing its direct Google.Protobuf reference now that Core owns the protocol layer). Guards against protobuf schema drift silently breaking real-device discovery responses.
• closes Silent 65535 resolution fallback turns a device's missing analog_in_res into 4x-wrong samples #311 — PopulateAnalogChannels silently substituted 65535 for a missing analog_in_res with no warning anywhere, silently scaling every sample wrong on devices (e.g. NQ3) that never report resolution. Now logs a Trace warning naming the device, and IAnalogChannel/AnalogChannel expose ResolutionIsAssumed so consumers can distinguish a device-reported resolution from a guessed one. Also documents the Resolution convention (max raw count, i.e. 2^bits - 1) and adds coverage for 12/18/24-bit resolutions.
• closes DaqifiDevice.Send<T> non-string fallback throws NotImplementedException; producer-less constructor connects but can never send #282 — Send<T> only ever worked for string payloads; any other IOutboundMessage<T> threw NotImplementedException with a stale "later steps" comment even though the transport abstraction it was waiting on already landed. Non-string payloads now write directly to the underlying stream via IOutboundMessage<T>.GetBytes(). The producer-less (name, ipAddress) constructor still reports Connected (kept for compatibility with existing lifecycle tests), but Send now throws a clear InvalidOperationException naming the missing transport instead of the misleading NotImplementedException. Also scrubs a stray // test change comment on IDevice.cs.
• closes DownloadSdCardFileAsync silently returns a 0-byte success on an empty (marker-only) transfer #264 — SdCardFileReceiver.ReceiveAsync returned a successful 0-byte result when the device served an empty, marker-only transfer, which DownloadSdCardFileAsync reported as a clean FileSize=0 "success" — making a wedged/not-ready SD subsystem look like a data or import bug downstream. Now throws SdCardEmptyTransferException on a marker-only transfer, and DownloadSdCardFileAsync retries the GET once, mirroring GetSdCardFilesAsync's existing LIST retry for the same class of transient hiccup.

Test plan

• dotnet build — clean, 0 warnings/errors
• dotnet test src/Daqifi.Core.Tests — 1513 passed, 2 skipped (pre-existing), 0 failed, both net9.0/net10.0
• Bench-verified DownloadSdCardFileAsync silently returns a 0-byte success on an empty (marker-only) transfer #264 on a real Nyquist 1 device (USB/serial): --sd-list and --sd-download of an existing 20,118-byte log file both complete normally with no retry triggered and no SdCardEmptyTransferException — confirms the healthy download path is unaffected

[qodo-code-review]

PR Summary by Qodo

Fix device send/SD download edge cases; surface assumed ADC resolution; add protocol tests

🐞 Bug fix 🧪 Tests ✨ Enhancement 🕐 40+ Minutes

AI Description

• Add regression tests for real protobuf discovery payload and ADC scaling behavior.
• Surface assumed ADC resolution and trace-log when devices omit analog_in_res.
• Send now writes non-string payload bytes; SD downloads retry and fail on empty transfers.

Diagram

graph TD
  T["Core tests"] --> P["Protocol parser"] --> M["DaqifiOutMessage"]
  T --> D["DaqifiDevice"] --> A["AnalogChannel"]
  D --> S[("Transport stream")]
  SD["DaqifiStreamingDevice"] --> S --> R["SdCardFileReceiver"] --> E["Empty transfer exception"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Split into four focused PRs

• ➕ Easier review/rollback and targeted release notes per issue (Add parser test using real captured device payload #204/Silent 65535 resolution fallback turns a device's missing analog_in_res into 4x-wrong samples #311/DaqifiDevice.Send<T> non-string fallback throws NotImplementedException; producer-less constructor connects but can never send #282/DownloadSdCardFileAsync silently returns a 0-byte success on an empty (marker-only) transfer #264)
• ➕ Reduces risk of unrelated conflicts and simplifies cherry-picking
• ➖ More overhead (CI runs, approvals) and less convenient batching for small changes

2. Use ILogger abstraction instead of Trace.WriteLine

• ➕ Consistent logging routing/levels across apps and easier test verification
• ➕ Avoids reliance on global Trace listeners
• ➖ Requires plumbing an ILogger (or logging facade) into device/channel creation paths

3. Represent missing ADC resolution as nullable/optional instead of a boolean flag

• ➕ Stronger modeling: callers can’t ignore the “unknown resolution” case
• ➕ Avoids “guess + flag” potentially being treated as authoritative
• ➖ More pervasive API changes (call sites, scaling code, serialization) and potentially breaking for consumers

Recommendation: The PR’s approach is reasonable for a compatibility-preserving fix: keep a concrete Resolution for scaling, but expose ResolutionIsAssumed so consumers can warn/adjust. The SdCardEmptyTransferException + bounded retry makes the failure mode explicit while matching existing LIST retry behavior. If reviewer bandwidth allows, consider (in a follow-up) migrating Trace.WriteLine to a logging abstraction and splitting future batches to keep changes independently releasable.

Files changed (13) +595 / -27

Enhancement (2) +30 / -4

AnalogChannel.csAdd ResolutionIsAssumed and document resolution convention +20/-3

Add ResolutionIsAssumed and document resolution convention

• Extends AnalogChannel to track whether its resolution was device-reported vs assumed, and clarifies that Resolution is max raw count (2^bits - 1). Updates UpdateScalingFromStatus to atomically update the new flag.

src/Daqifi.Core/Channel/AnalogChannel.cs

IAnalogChannel.csExpose ResolutionIsAssumed on analog channel interface +10/-1

Expose ResolutionIsAssumed on analog channel interface

• Adds a ResolutionIsAssumed property to IAnalogChannel and documents that assumed values can lead to systematically incorrect scaled samples.

src/Daqifi.Core/Channel/IAnalogChannel.cs

Bug fix (4) +108 / -15

DaqifiDevice.csImplement Send<T> direct-stream fallback and surface assumed ADC resolution +31/-8

Implement Send<T> direct-stream fallback and surface assumed ADC resolution

• Send<T> now queues string messages through the producer when available, otherwise writes message bytes to the transport stream (or direct stream from stream-based constructor) and throws a clear InvalidOperationException if neither exists. PopulateAnalogChannels now logs when AnalogInRes is missing, propagates a resolutionIsAssumed flag to AnalogChannel creation/updates, and preserves channel instances across re-population.

src/Daqifi.Core/Device/DaqifiDevice.cs

DaqifiStreamingDevice.csRetry SD GET once and document empty-transfer failure mode +30/-7

Retry SD GET once and document empty-transfer failure mode

• DownloadSdCardFileAsync now treats marker-only transfers as a transient SD-not-ready condition, retrying the GET with a bounded delay, and documents that SdCardEmptyTransferException is thrown if all attempts are empty.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs

SdCardEmptyTransferException.csIntroduce exception type for marker-only SD transfers +35/-0

Introduce exception type for marker-only SD transfers

• Adds SdCardEmptyTransferException (derived from SdCardOperationException) to explicitly signal 0-byte, marker-only downloads and include the file name in the error context.

src/Daqifi.Core/Device/SdCard/SdCardEmptyTransferException.cs

SdCardFileReceiver.csThrow on marker-only transfers in SD file receiver +12/-0

Throw on marker-only transfers in SD file receiver

• Updates SdCardFileReceiver.ReceiveAsync to throw SdCardEmptyTransferException when the EOF marker is received with zero preceding file bytes, preventing silent 0-byte 'success' downloads.

src/Daqifi.Core/Device/SdCard/SdCardFileReceiver.cs

Refactor (1) +1 / -1

IDevice.csRemove stray trailing comment +1/-1

Remove stray trailing comment

• Removes a leftover '// test change' comment from the IDevice interface file footer.

src/Daqifi.Core/Device/IDevice.cs

Tests (6) +456 / -7

AnalogChannelTests.csAdd tests for resolution-assumption flag and multi-bit-depth scaling +55/-0

Add tests for resolution-assumption flag and multi-bit-depth scaling

• Adds coverage that AnalogChannel defaults ResolutionIsAssumed to false, honors a true value, and correctly stores/uses 12/18/24-bit max-count resolutions when scaling to PortRange.

src/Daqifi.Core.Tests/Channel/AnalogChannelTests.cs

DaqifiOutMessageTests.csAdd real-captured protobuf payload parsing regression test +99/-0

Add real-captured protobuf payload parsing regression test

• Introduces a test using a Wireshark-captured Nyquist device SYSInfoPB response to validate DaqifiOutMessage ParseDelimitedFrom and key parsed fields (SSID, hostname, IP, firmware).

src/Daqifi.Core.Tests/Communication/Messages/DaqifiOutMessageTests.cs

ChannelPopulationTests.csVerify PopulateChannelsFromStatus sets/clears assumed ADC resolution +66/-0

Verify PopulateChannelsFromStatus sets/clears assumed ADC resolution

• Adds tests ensuring AnalogInRes==0 marks channels as assumed, reported resolutions do not, and a later reported resolution clears the assumed flag without replacing the channel instance.

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

DaqifiDeviceWithMessageProducerTests.csTest non-string Send<T> direct-stream writes and producer-less error message +41/-0

Test non-string Send<T> direct-stream writes and producer-less error message

• Adds a byte[] outbound message test confirming non-string payloads are written directly to the stream, and verifies producer-less constructor throws InvalidOperationException with a clear message.

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

SdCardFileReceiverTests.csTreat marker-only SD transfers as errors in receiver tests +22/-7

Treat marker-only SD transfers as errors in receiver tests

• Updates the prior 'empty file' test to expect SdCardEmptyTransferException on marker-only transfers and adds a variant where the marker is split across read chunks.

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

SdCardOperationsTests.csAdd SD GET retry tests for marker-only transfer and eventual success +173/-0

Add SD GET retry tests for marker-only transfer and eventual success

• Adds tests verifying DownloadSdCardFileAsync retries once on marker-only transfers and either throws after all attempts or succeeds on the retry. Introduces helper stream/device types to serve per-attempt canned responses.

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

[tylerkron]

Thanks for the review, @qodo-code-review[bot]. Addressing the High-Level Assessment points:

1. Split into four focused PRs — Declined for this batch. Batching these four small, independent fixes into one PR was an explicit, deliberate choice for this round (each is a self-contained bug fix/test addition touching disjoint files, so the review/rollback cost of one PR is low). Future batches can revisit per-issue PRs if reviewer bandwidth is a concern.

2. Use ILogger abstraction instead of Trace.WriteLine — Agreed as a good idea, declined for this PR. Trace.WriteLine is DaqifiDevice.cs's existing, established diagnostic-logging convention (used throughout the file, e.g. ExecuteTextCommandAsync, DrainErrorQueueAsync, subscriber-exception logging) — the new warning in PopulateAnalogChannels follows that precedent rather than introducing a second logging mechanism inconsistent with the rest of the class. Migrating the whole file to ILogger is a legitimate but separate, larger change (needs plumbing through device/channel construction paths); flagging as a follow-up rather than mixing it into this batch.

3. Represent missing ADC resolution as nullable/optional instead of a boolean flag — Declined. The ResolutionIsAssumed flag was the explicitly preferred option in the originating issue (Silent 65535 resolution fallback turns a device's missing analog_in_res into 4x-wrong samples #311, "Option 1: Surface it"), specifically because it keeps Resolution a concrete, non-null uint — AnalogChannel.GetScaledValue divides by it directly, so a nullable resolution would either need a fallback value anyway (reintroducing the original bug) or push null-handling into every scaling call site. It also avoids a breaking API change to IAnalogChannel.Resolution, consistent with the 1.x-compatibility constraint called out in the related DaqifiDevice.Send<T> non-string fallback throws NotImplementedException; producer-less constructor connects but can never send #282.

No inline code-level findings were posted on this PR (0 review comments, 0 formal reviews) — only the summary/assessment above, which is what this reply addresses. Build is green.

[qodo-code-review]

Thanks for the review, @qodo-code-review[bot]. Addressing the High-Level Assessment points:

1. Split into four focused PRs — Declined for this batch. Batching these four small, independent fixes into one PR was an explicit, deliberate choice for this round (each is a self-contained bug fix/test addition touching disjoint files, so the review/rollback cost of one PR is low). Future batches can revisit per-issue PRs if reviewer bandwidth is a concern.

2. Use ILogger abstraction instead of Trace.WriteLine — Agreed as a good idea, declined for this PR. Trace.WriteLine is DaqifiDevice.cs's existing, established diagnostic-logging convention (used throughout the file, e.g. ExecuteTextCommandAsync, DrainErrorQueueAsync, subscriber-exception logging) — the new warning in PopulateAnalogChannels follows that precedent rather than introducing a second logging mechanism inconsistent with the rest of the class. Migrating the whole file to ILogger is a legitimate but separate, larger change (needs plumbing through device/channel construction paths); flagging as a follow-up rather than mixing it into this batch.

3. Represent missing ADC resolution as nullable/optional instead of a boolean flag — Declined. The ResolutionIsAssumed flag was the explicitly preferred option in the originating issue (Silent 65535 resolution fallback turns a device's missing analog_in_res into 4x-wrong samples #311, "Option 1: Surface it"), specifically because it keeps Resolution a concrete, non-null uint — AnalogChannel.GetScaledValue divides by it directly, so a nullable resolution would either need a fallback value anyway (reintroducing the original bug) or push null-handling into every scaling call site. It also avoids a breaking API change to IAnalogChannel.Resolution, consistent with the 1.x-compatibility constraint called out in the related DaqifiDevice.Send<T> non-string fallback throws NotImplementedException; producer-less constructor connects but can never send #282.

No inline code-level findings were posted on this PR (0 review comments, 0 formal reviews) — only the summary/assessment above, which is what this reply addresses. Build is green.

Thanks for the detailed rationale. The decisions are clear and consistent with the current codebase and compatibility goals—especially retaining Trace.WriteLine for this focused change and using ResolutionIsAssumed without altering the existing Resolution contract. With no inline findings and a green build, I have no further concerns for this batch.

[qodo-code-review]

Code Review by Qodo

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

Context used

✅ Tickets: 🎫 Add parser test using real captured device payload 🎫 DownloadSdCardFileAsync silently returns a 0-byte success on an empty (marker-only) transfer

1. Concurrent stream writes 🐞 Bug ☼ Reliability

Description

DaqifiDevice.Send<T> can now write directly to the underlying stream for non-string payloads while
MessageProducer<string> may simultaneously write queued SCPI messages to the same stream on a
background thread, allowing byte interleaving and corrupted frames. This path also skips the
producer’s send discipline (including its per-write flush policy), making behavior inconsistent
across message types.

Code

src/Daqifi.Core/Device/DaqifiDevice.cs[R420-440]

+            // Use the queued message producer when available and the message is string-based;
+            // this is the common path (SCPI text commands).
            if (_messageProducer != null && message is IOutboundMessage<string> stringMessage)
            {
                _messageProducer.Send(stringMessage);
+                return;
            }
-            else
+
+            // Non-string payloads (or a string payload with no producer) bypass the queue and
+            // write directly to the underlying stream, since IOutboundMessage<T> already knows
+            // how to serialize itself regardless of T.
+            var stream = _transport?.Stream ?? _directStream;
+            if (stream == null)
            {
-                // Fallback for backward compatibility - no implementation yet
-                // This will be enhanced in later steps when we add transport abstraction
-                throw new NotImplementedException("Direct message sending without message producer is not yet implemented. Use constructor with Stream parameter.");
+                throw new InvalidOperationException(
+                    "This device has no transport or stream to send on. Use a constructor that accepts a Stream or IStreamTransport.");
            }
+
+            var bytes = message.GetBytes();
+            stream.Write(bytes, 0, bytes.Length);
        }

Relevance

⭐ Low

Similar concurrency/locking suggestions around device/producers were repeatedly rejected (e.g.,
thread-safety/serialization in PRs #185, #99).

PR-#185
PR-#99
PR-#33

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new direct-write path can target the same stream that the MessageProducer writes to
asynchronously; without a shared lock, concurrent writes can interleave and corrupt the protocol on
the wire.

src/Daqifi.Core/Device/DaqifiDevice.cs[403-440]
src/Daqifi.Core/Communication/Producers/MessageProducer.cs[146-171]
src/Daqifi.Core/Communication/Producers/MessageProducer.cs[213-221]

Agent prompt

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

### Issue description
`DaqifiDevice.Send<T>` now has two different write paths to the same underlying stream:
- queued/background writes via `MessageProducer<string>`
- synchronous direct writes for non-string payloads

There is no shared synchronization, so concurrent sends can interleave bytes on the stream.

### Issue Context
`MessageProducer<T>` writes on a background thread. The direct-write path can execute on caller threads at any time when `_transport?.Stream` or `_directStream` is available.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiDevice.cs[413-440]
- src/Daqifi.Core/Communication/Producers/MessageProducer.cs[213-221]

### Implementation notes
- Introduce a shared write lock used by *both* the producer and the direct-write path (e.g., inject a lock into `MessageProducer`, or wrap the stream in a synchronized adapter used by both).
- Alternatively, route *all* writes through a single producer (e.g., add a byte-oriented producer or a unified producer abstraction) so there is only one writer.
- Ensure `Send<T>` validates `message` is non-null (consistent with `MessageProducer.Send`).
- If you keep direct writes, make the direct path follow the same write policy as the producer (e.g., flush if that’s required for your device/transport expectations).

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

---

2. Empty SD file blocked 🐞 Bug ≡ Correctness

Description

SdCardFileReceiver.ReceiveAsync now throws SdCardEmptyTransferException on any marker-only transfer
(0 bytes before EOF), which makes legitimately empty (0-byte) files impossible to download. The core
code path has no expected-size context because file sizes are not retained from LIST responses.

Code

src/Daqifi.Core/Device/SdCard/SdCardFileReceiver.cs[R153-160]

+                    if (totalBytesReceived == 0)
+                    {
+                        // An immediate EOF marker with no preceding file bytes means the device
+                        // opened the file but never sent any content — a transient/wedged SD
+                        // subsystem, not a legitimate empty download. See #264.
+                        throw new SdCardEmptyTransferException(fileName);
+                    }
+

Relevance

⭐ Low

SD download code historically treats marker-only transfer as an error, not a valid empty file (SD
receiver introduced in PR #115).

PR-#115

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The receiver throws solely based on totalBytesReceived == 0, but the SD file list model/parser
does not preserve file sizes, so there is no way to prove the file should have been non-empty.

src/Daqifi.Core/Device/SdCard/SdCardFileReceiver.cs[110-163]
src/Daqifi.Core/Device/SdCard/SdCardFileInfo.cs[10-32]
src/Daqifi.Core/Device/SdCard/SdCardFileListParser.cs[52-57]

Agent prompt

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

### Issue description
`SdCardFileReceiver.ReceiveAsync` treats any marker-only transfer as an error, but the library does not reliably know whether the requested file is expected to be non-empty. This creates a deterministic regression for legitimate 0-byte files.

### Issue Context
- `SdCardFileReceiver` only has `(destinationStream, fileName, progress, timeout, cancellationToken)`; it cannot know expected file size.
- `SdCardFileInfo` does not store size, and the LIST parser discards size tokens even if the device returns them.

### Fix Focus Areas
- src/Daqifi.Core/Device/SdCard/SdCardFileReceiver.cs[114-163]
- src/Daqifi.Core/Device/SdCard/SdCardFileInfo.cs[10-32]
- src/Daqifi.Core/Device/SdCard/SdCardFileListParser.cs[52-57]

### Implementation notes
Pick one approach:
1) **Preserve file sizes from LIST**: extend `SdCardFileInfo` with optional `FileSizeBytes`, parse it in `SdCardFileListParser`, and only throw `SdCardEmptyTransferException` when the expected size is known to be > 0.
2) **Pass expected-size into ReceiveAsync**: add an optional `expectedSizeBytes` (or `allowEmpty`) parameter so callers can decide whether marker-only is invalid.

Keep the retry behavior in `DownloadSdCardFileAsync`, but ensure legitimate empty files remain downloadable when the expected size is 0/unknown.

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

---

3. Trace warning can spam 🐞 Bug ◔ Observability

Description

PopulateAnalogChannels emits a Trace.WriteLine every time a status message reports AnalogInRes==0,
which can repeatedly spam diagnostics for devices that never report resolution. This can drown out
other Trace output and create unnecessary overhead during frequent status refreshes.

Code

src/Daqifi.Core/Device/DaqifiDevice.cs[R1331-1337]

+            var resolutionIsAssumed = analogInResolution == 0;
            var resolution = analogInResolution > 0 ? analogInResolution : 65535;

+            if (resolutionIsAssumed && count > 0)
+            {
+                Trace.WriteLine($"[PopulateAnalogChannels] Device '{Name}' reported no ADC resolution (analog_in_res=0) for {count} analog channel(s); assuming {resolution}. Scaled samples on this device may be systematically wrong.");
+            }

Relevance

⭐ Low

Repo has rejected Trace-related refinement suggestions before; team recently added Trace diagnostics
and kept them (PR #126 rejected).

PR-#126
PR-#323

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The log statement is unconditional for every qualifying status refresh; there is no state/transition
check to prevent repeated emission.

src/Daqifi.Core/Device/DaqifiDevice.cs[1322-1338]

Agent prompt

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

### Issue description
`PopulateAnalogChannels` writes a Trace line on every call when `analog_in_res == 0` and there are analog channels.

### Issue Context
Devices that never report ADC resolution will trigger this message repeatedly across status refreshes.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiDevice.cs[1330-1337]

### Implementation notes
- Log only once per device instance (or once per connect session), or only when the assumed/reported state transitions.
- Consider moving to the project’s structured logging approach if one exists, instead of raw `Trace.WriteLine`.

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

---

[tylerkron]

Thanks for the detailed findings, @qodo-code-review[bot]. Triage:

1. Concurrent stream writes (Send) — Valid theoretical race: the new direct-write path for non-string payloads and MessageProducer<string>'s background write thread aren't synchronized, so mixing string and non-string sends on a device with an active producer could interleave bytes. Declining to fix in this PR: a correct fix needs a synchronization primitive plumbed into MessageProducer<T> itself (a lock around the enqueue call alone wouldn't help, since the actual write happens later on the producer's background thread) — that's a real change to shared infrastructure, not a batch-fix, and matches the team's prior stance in #185 and #99 on similar producer-locking proposals. Filed as a follow-up to investigate whether those precedents still apply here given this is a genuinely new dual-write-path scenario.

2. Empty SD file blocked — This is the most substantive one and you're right that it's architecturally real: SdCardFileListParser already discards a file-size token the firmware sends (// If a file size is present after the path, keep only the first token), so SdCardFileInfo has no size to distinguish "legitimately empty file" from "wedged device" — both produce byte-identical marker-only transfers, which is inherent to the download protocol, not just this PR's fix. Declining to implement in this PR since it's a real design decision (your own writeup offers two valid approaches) that widens scope beyond a batch of small fixes, and needs to establish first whether DAQiFi log files can ever legitimately be 0 bytes in practice. Filed as a follow-up to investigate and implement.

3. Trace warning can spam — Agreed, fixed in bce6cce: the warning now logs once per device instance and re-arms only if the device later reports a real resolution and then regresses back to omitting it. Added test coverage for both the throttling and the re-arm behavior.

All existing tests + new coverage pass (1515 tests, 0 failures).

[qodo-code-review]

Thanks for the detailed findings, @qodo-code-review[bot]. Triage:

1. Concurrent stream writes (Send) — Valid theoretical race: the new direct-write path for non-string payloads and MessageProducer<string>'s background write thread aren't synchronized, so mixing string and non-string sends on a device with an active producer could interleave bytes. Declining to fix in this PR: a correct fix needs a synchronization primitive plumbed into MessageProducer<T> itself (a lock around the enqueue call alone wouldn't help, since the actual write happens later on the producer's background thread) — that's a real change to shared infrastructure, not a batch-fix, and matches the team's prior stance in #185 and #99 on similar producer-locking proposals. Filed as a follow-up to investigate whether those precedents still apply here given this is a genuinely new dual-write-path scenario.

2. Empty SD file blocked — This is the most substantive one and you're right that it's architecturally real: SdCardFileListParser already discards a file-size token the firmware sends (// If a file size is present after the path, keep only the first token), so SdCardFileInfo has no size to distinguish "legitimately empty file" from "wedged device" — both produce byte-identical marker-only transfers, which is inherent to the download protocol, not just this PR's fix. Declining to implement in this PR since it's a real design decision (your own writeup offers two valid approaches) that widens scope beyond a batch of small fixes, and needs to establish first whether DAQiFi log files can ever legitimately be 0 bytes in practice. Filed as a follow-up to investigate and implement.

3. Trace warning can spam — Agreed, fixed in bce6cce: the warning now logs once per device instance and re-arms only if the device later reports a real resolution and then regresses back to omitting it. Added test coverage for both the throttling and the re-arm behavior.

All existing tests + new coverage pass (1515 tests, 0 failures).

Thanks for the thorough triage. The dispositions are clear:

• Finding 2 — Concurrent stream writes: declined for this PR and tracked as a follow-up, pending a broader producer-synchronization decision.
• Finding 1 — Empty SD file blocked: declined for this PR and tracked as a follow-up to resolve the protocol-level ambiguity around legitimate zero-byte files.
• Finding 3 — Trace warning can spam: fixed in bce6cce, with coverage for throttling and re-arming.

The reported test result—1,515 tests passing with zero failures—provides good coverage for the merged changes.