feat(sdcard): warn on low SD free space before logging (closes #230)

[tylerkron]

Summary

Implements #230 — a client-side pre-flight that warns (never blocks) the user before starting an SD-output capture when the card is nearly full or the planned capture won't fit. Per the firmware/client split recorded in ADR 0001: the firmware owns the safe SD:MINFree gate, the client owns the UX warning.

The SD space query (GetSdCardStorageAsync → SdCardStorageInfo) already landed in #214; this PR builds the warning layer on top.

What's added

• SdCardCaptureEstimate — a pure, best-effort size estimator (rate × channels × bytes/sample × duration) with BytesPerSecond / EstimatedBytes (overflow-clamped). Defaults BytesPerSamplePerChannel to the raw 16-bit ADC width, documented as a floor (encoded logs are larger).
• SdCardSpaceCheck + SdCardSpaceCheckResult — pure evaluator applying the two rules from the issue: free < estimate → "won't fit" (+ truncation ETA), and free < 100 MB (configurable) → "nearly full". Produces a human-readable message.
• LowSdSpaceWarningEventArgs + LowSdSpaceWarning event on ISdCardOperations / DaqifiStreamingDevice.
• CheckSdCardSpaceAsync(plannedCapture?, minimumFreeBytes?, ct) — queries SD space, evaluates, raises the event when warranted, returns the structured result. Advisory only; the caller decides whether to proceed before StartSdCardLoggingAsync.
• SetSdCardMinimumFreeSpace(bytes) + SCPI SetSdMinFreeSpace — optional hand-off to the firmware SD:MINFree gate (v3.5.0+, at/below the supported floor, so safe to send unconditionally).

Design note

The pre-flight is a separate method, not auto-run inside StartSdCardLoggingAsync. The issue requires "let the user proceed if they want" — the user must see the warning and decide before start commits, which is only possible as a distinct step (an internal check that raised mid-start couldn't let them abort, since the warning must not block). StartSdCardLoggingAsync's docs cross-reference CheckSdCardSpaceAsync for discoverability.

Testing

• Unit: estimator, evaluator (all rule combinations, custom floor, null/negative guards), the device method (event raised/not raised, disconnected, command emitted), and the SCPI producer. Full suite: 1164 passed, 0 failed on both net9.0 and net10.0.
• Real hardware (USB): built the example CLI against this worktree's core and ran a streaming smoke test, plus an isolated harness exercising the new API directly:
  • 7.8 GB free, no estimate → no warning ✓
  • ~12.87 GB estimate > 7.27 GB free → "won't fit" warning + event, truncation ETA ✓
  • floor raised to 8 GB → "nearly full" warning + event ✓
  • SetSdCardMinimumFreeSpace(52428800) accepted by firmware, no error (reset to 0 afterward) ✓

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Warn on low SD free space before starting SD logging
✨ Enhancement 🧪 Tests 🕐 40+ Minutes

Description

• Add advisory SD space pre-flight API and LowSdSpaceWarning event before SD logging.
• Introduce capture-size estimator and evaluator with truncation ETA and human-readable warnings.
• Add SCPI command to set firmware SD:MINFree floor, with unit test coverage.

Diagram

graph TD
  caller["Client app"] --> preflight["CheckSdCardSpaceAsync"] --> scpiGet["SCPI GetSdSpace"] --> fw[/"Device firmware"/]
  preflight --> eval["SdCardSpaceCheck"] --> warn{"ShouldWarn?"} --> evt["LowSdSpaceWarning event"]
  est["SdCardCaptureEstimate"] --> eval
  setFloor["SetSdCardMinimumFreeSpace"] --> scpiSet["SCPI SetSdMinFreeSpace"] --> fw
  subgraph Legend
    direction LR
    _proc["API/Component"] ~~~ _dec{"Decision"} ~~~ _fw[/"Firmware"/]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Auto-run check inside StartSdCardLoggingAsync

• ➕ Callers get warnings without learning a new API surface
• ➕ Centralizes SD safety behavior in one method
• ➖ Harder for UX to let the user decide before start commits
• ➖ Still can’t block without changing contract; warning timing becomes ambiguous

2. Rely solely on firmware SD:MINFree gate

• ➕ Single source of truth for preventing truncation
• ➕ No client-side estimation needed
• ➖ Does not satisfy UX requirement to warn (and allow override)
• ➖ Gate blocks starts rather than presenting a confirmable warning

3. Return-only result (no event)

• ➕ Simpler API; avoids event subscription lifecycle concerns
• ➕ Callers can render their own UX from result
• ➖ Harder to integrate into existing event-driven clients
• ➖ Easy for callers to ignore unless explicitly checked everywhere

Recommendation: Current approach is the best fit for the stated UX requirement: an explicit preflight method that returns a structured result and optionally raises an advisory event. It cleanly separates (1) firmware safety enforcement (SD:MINFree) from (2) client UX warnings, and avoids changing StartSdCardLoggingAsync semantics while still enabling discoverable, testable behavior.

Files changed (11) +760 / -0

Enhancement (7) +429 / -0

ScpiMessageProducer.csAdd SCPI producer for firmware SD:MINFree gate +24/-0

Add SCPI producer for firmware SD:MINFree gate

• Adds SetSdMinFreeSpace(bytes) with input validation and documentation describing firmware behavior and version availability. Produces the SYSTem:STORage:SD:MINFree command string.

src/Daqifi.Core/Communication/Producers/ScpiMessageProducer.cs

DaqifiStreamingDevice.csExpose SD space preflight API, warning event, and MINFree setter +51/-0

Expose SD space preflight API, warning event, and MINFree setter

• Adds LowSdSpaceWarning event and a CheckSdCardSpaceAsync method that queries SD storage, evaluates warning rules, and raises the event when warranted (advisory only). Adds SetSdCardMinimumFreeSpace to send the firmware gate command with consistent validation/connection checks.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs

ISdCardOperations.csExtend SD card operations interface with preflight + warning event +49/-0

Extend SD card operations interface with preflight + warning event

• Adds a LowSdSpaceWarning event and CheckSdCardSpaceAsync preflight method to query/evaluate SD space before logging. Adds SetSdCardMinimumFreeSpace API and documents that StartSdCardLoggingAsync does not auto-run the preflight.

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

LowSdSpaceWarningEventArgs.csAdd event args type for low SD space warning +26/-0

Add event args type for low SD space warning

• Introduces LowSdSpaceWarningEventArgs carrying the evaluated SdCardSpaceCheckResult, with null-guarded construction.

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

SdCardCaptureEstimate.csAdd best-effort capture size estimator +96/-0

Add best-effort capture size estimator

• Implements SdCardCaptureEstimate for planned capture inputs (frequency, channels, duration, bytes/sample). Computes BytesPerSecond and an overflow-clamped EstimatedBytes with documented defaults and argument validation.

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

SdCardSpaceCheck.csAdd pure evaluator for SD free-space warning conditions +142/-0

Add pure evaluator for SD free-space warning conditions

• Implements SdCardSpaceCheck.Evaluate to apply nearly-full and won’t-fit rules and to compute an estimated time-until-full. Builds a human-readable message with byte and duration formatting helpers and validates inputs.

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

SdCardSpaceCheckResult.csAdd structured result type for SD space checks +41/-0

Add structured result type for SD space checks

• Adds SdCardSpaceCheckResult record carrying storage, estimate, thresholds, flags, time-until-full, and an optional message. Provides a convenience ShouldWarn property.

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

Tests (4) +331 / -0

ScpiMessageProducerTests.csAdd unit tests for SetSdMinFreeSpace SCPI command +23/-0

Add unit tests for SetSdMinFreeSpace SCPI command

• Adds coverage ensuring the produced SCPI command matches expected formatting. Verifies zero disables the gate and negative inputs throw.

src/Daqifi.Core.Tests/Communication/Producers/ScpiMessageProducerTests.cs

SdCardCaptureEstimateTests.csAdd tests for capture size/rate estimator +61/-0

Add tests for capture size/rate estimator

• Introduces unit tests validating bytes-per-second and total estimate calculations, default bytes-per-sample behavior, argument validation, and overflow clamping.

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

SdCardOperationsTests.csAdd device-level tests for SD space preflight and MINFree setter +115/-0

Add device-level tests for SD space preflight and MINFree setter

• Adds tests for CheckSdCardSpaceAsync warning behavior (nearly-full, won’t-fit, and no-warning cases), event raising, connection requirements, and that the SD space query command is issued. Adds tests for SetSdCardMinimumFreeSpace command emission and argument/state validation.

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

SdCardSpaceCheckTests.csAdd tests for SD space evaluation rules and messaging +132/-0

Add tests for SD space evaluation rules and messaging

• Validates default threshold, nearly-full rule, won’t-fit rule with truncation ETA, combined conditions, custom thresholds, and defensive argument checks.

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

[qodo-code-review]

Code Review by Qodo

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

Context used

✅ Tickets: 🎫 Warn user of low SD free space before SYST:STR:START

1. Estimator overflow wraps long ✓ Resolved 🐞 Bug ≡ Correctness

Description

SdCardCaptureEstimate computes BytesPerSecond via unchecked integer multiplication, so large (but
still valid) inputs can overflow and wrap to a negative/incorrect long. That can cause
SdCardSpaceCheck.Evaluate to miscompute IsInsufficientForCapture and/or skip EstimatedTimeUntilFull
(it only computes ETA when BytesPerSecond > 0), suppressing a warning that should have been raised.

Code

src/Daqifi.Core/Device/SdCard/SdCardCaptureEstimate.cs[R82-94]

+    public long BytesPerSecond => (long)FrequencyHz * ChannelCount * BytesPerSamplePerChannel;
+
+    /// <summary>
+    /// Gets the estimated total bytes the capture will write, clamped to <see cref="long.MaxValue"/> if it
+    /// would otherwise overflow.
+    /// </summary>
+    public long EstimatedBytes
+    {
+        get
+        {
+            var bytes = BytesPerSecond * Duration.TotalSeconds;
+            return bytes >= long.MaxValue ? long.MaxValue : (long)bytes;
+        }

Relevance

⭐⭐ Medium

No prior reviews on overflow-safe rate multiplication; team often accepts defensive
validation/clamping elsewhere (e.g., PR #214).

PR-#214

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The estimator uses unchecked multiplication for BytesPerSecond and then uses that value to compute
EstimatedBytes; SdCardSpaceCheck.Evaluate directly consumes EstimatedBytes for the “won’t fit” rule
and gates the truncation ETA on BytesPerSecond being positive. If BytesPerSecond overflows to a
negative/incorrect value, both the warning decision and ETA can be wrong.

src/Daqifi.Core/Device/SdCard/SdCardCaptureEstimate.cs[34-64]
src/Daqifi.Core/Device/SdCard/SdCardCaptureEstimate.cs[78-95]
src/Daqifi.Core/Device/SdCard/SdCardSpaceCheck.cs[50-62]

Agent prompt

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

### Issue description
`SdCardCaptureEstimate.BytesPerSecond` multiplies `(long)FrequencyHz * ChannelCount * BytesPerSamplePerChannel` without overflow checking. In C#, this overflows silently in the default unchecked context, potentially producing negative/incorrect rates. `EstimatedBytes` then derives from that value, and the warning evaluator (`SdCardSpaceCheck.Evaluate`) relies on `EstimatedBytes` and `BytesPerSecond > 0` to decide whether to warn and whether to compute a truncation ETA.

### Issue Context
This estimator is public API and currently only validates inputs are positive, so extreme values are permitted. When overflow happens, the warning layer may incorrectly report that a capture fits or omit the truncation ETA.

### Fix Focus Areas
- src/Daqifi.Core/Device/SdCard/SdCardCaptureEstimate.cs[34-64]
- src/Daqifi.Core/Device/SdCard/SdCardCaptureEstimate.cs[78-95]

### Suggested fix approach
- Compute `BytesPerSecond` using `checked` arithmetic and clamp to `long.MaxValue` on overflow (or validate upper bounds and throw).
 - Example: `long bps; try { bps = checked((long)frequencyHz * channelCount * bytesPerSamplePerChannel); } catch (OverflowException) { bps = long.MaxValue; }`
- Compute `EstimatedBytes` using an overflow-safe path as well:
 - If `BytesPerSecond == long.MaxValue`, return `long.MaxValue`.
 - Otherwise multiply using a safe numeric type (e.g., `decimal` using `duration.Ticks / TimeSpan.TicksPerSecond`) and clamp to `long.MaxValue` before casting.
- Add a unit test that demonstrates `BytesPerSecond` clamping/behavior under overflow inputs (today tests only cover `EstimatedBytes` clamping).

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

---

2. StartSdCardLoggingAsync skips SD query 📎 Requirement gap ≡ Correctness

Description

StartSdCardLoggingAsync starts SD logging without first querying SYST:STOR:SD:SPACe?, so callers
can start an SD session without having free/total bytes available in the start decision path. This
violates the requirement to perform a pre-flight SD-space query before issuing the SD streaming
start command.

Code

src/Daqifi.Core/Device/SdCard/ISdCardOperations.cs[R94-98]

+        /// <remarks>
+        /// This method does not perform a free-space pre-flight. To warn the user about a near-full card
+        /// before committing to a capture, call <see cref="CheckSdCardSpaceAsync"/> first and let the user
+        /// decide whether to proceed.
+        /// </remarks>

Relevance

⭐ Low

Repo intentionally keeps SD-space preflight separate (CheckSdCardSpaceAsync);
StartSdCardLoggingAsync remains start-only (PR #153/#214).

PR-#153
PR-#214

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1 requires a pre-flight SYST:STOR:SD:SPACe? query prior to starting SD streaming.
The updated interface remark explicitly states StartSdCardLoggingAsync does not perform the
pre-flight, and the start implementation shows it proceeds to StartStreaming(...) without querying
SD space first.

Pre-flight SD free space query before starting SD streaming
src/Daqifi.Core/Device/SdCard/ISdCardOperations.cs[94-98]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[845-905]

Agent prompt

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

## Issue description
`StartSdCardLoggingAsync` does not query SD free/total space before starting SD-output streaming, which prevents a reliable pre-flight assessment in the start path.

## Issue Context
The compliance requirement expects `SYST:STOR:SD:SPACe?` to be sent prior to `SYST:STR:START` for SD-output sessions, with parsed `FreeBytes`/`TotalBytes` available to the start/UX decision path.

## Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[845-905]
- src/Daqifi.Core/Device/SdCard/ISdCardOperations.cs[94-98]

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

---

[tylerkron]

Response to Qodo review

1. 🐞 Estimator overflow wraps long — ✅ Fixed (899adaf)

Agreed. SdCardCaptureEstimate.BytesPerSecond multiplied three ints in the default unchecked context, so extreme-but-valid inputs could overflow and wrap negative — which would make a capture wrongly appear to fit and suppress the truncation ETA (gated on BytesPerSecond > 0). Now computed with checked arithmetic and clamped to long.MaxValue on overflow. EstimatedBytes already saturates (it multiplies in double, which goes to +∞ rather than wrapping) and now consumes the clamped rate. Added BytesPerSecond_WhenProductOverflowsLong_ClampsToMaxValue to cover the rate path (previously only EstimatedBytes clamping was tested).

2. 📎 StartSdCardLoggingAsync skips the SD-space query — declining (by design)

I'm leaving this as-is. The pre-flight is intentionally a separate method (CheckSdCardSpaceAsync) rather than wired into the start path, for the reason Qodo's own High-Level Assessment reaches: it's "the best fit for the stated UX requirement," and the Relevance note acknowledges "Repo intentionally keeps SD-space preflight separate."

The issue's requirement is "Don't block the start. Surface the warning … and let the user proceed if they want." That only works if the space check and the user's decision happen before StartSdCardLoggingAsync is called. If the query were internal to the start path, the start would already be committed by the time the warning fired, and — since we must not block — the user could not actually decide to abort. So the start-path query would query but couldn't gate, which doesn't meet the requirement any better.

The intended flow (documented in the StartSdCardLoggingAsync remarks):

device.LowSdSpaceWarning += (s, e) => /* show confirmable dialog */;
var check = await device.CheckSdCardSpaceAsync(estimate);   // queries SYST:STOR:SD:SPACe?, raises the event
if (check.ShouldWarn && !userConfirms) return;              // user decides
await device.StartSdCardLoggingAsync(...);                  // start only after the decision

The compliance-mapper read the requirement as "query inside the start path"; the issue's intent (query + warn + let the user decide before start) is satisfied by the separate pre-flight. Qodo flagged this Optional/Low, which matches.