feat(channel): validate AnalogChannel bounds + cover bipolar scaling (closes #300, #297)

[tylerkron]

Summary

Hardens AnalogChannel against physically-nonsensical scaling inputs (#300) and locks in signed/bipolar scaling behavior with explicit test coverage plus a range-polarity accessor for consuming UIs (#297).

Closes #300. Closes #297.

#300 — bounds validation

• Constructor and the PortRange / CalibrationM / CalibrationB / InternalScaleM / MinValue / MaxValue setters now reject invalid values with informative ArgumentOutOfRangeExceptions, consistent with the existing constructor style:
  • Resolution: must be a plausible ADC max-count in [255, 16_777_216] (8–24 bit).
  • PortRange: finite, 0 < x <= 50 V.
  • CalibrationM / InternalScaleM: finite, non-zero, within ±1e6. Negative allowed (signal inversion); zero rejected (discards the measurement).
  • CalibrationB: finite, within ±1e6. Zero allowed.
  • Min/MaxValue: reject NaN/Infinity.
  • Bounds exposed as public consts.
• Device population is kept robust: DaqifiDevice.PopulateAnalogChannels sanitizes device-reported coefficients (NaN/Infinity/out-of-range/zero-scale) to safe defaults with a Trace log before they reach the validating setters — mirroring the existing analog_in_res=0 fallback. A corrupted status frame can neither crash channel population mid-stream nor silently propagate garbage into scaled samples.

#297 — bipolar / signed scaling

• Added explicit GetScaledValue coverage for negative/signed raw counts across representative bipolar range + calibration combinations: full-/half-scale sign, zero-point, symmetry about zero, offset-after-signed-gain, and inverting slope.
• Added IAnalogChannel.IsBipolar (derived from the configured MinValue) so range-selection UIs can branch on polarity without hardcoding per-device assumptions. Firmware confirmation that differential inputs emit signed two's-complement counts remains tracked separately (out of scope here).

Tests

• AnalogChannelTests: validation (valid/invalid/boundary) + signed/bipolar scaling + IsBipolar.
• ChannelPopulationTests: regression test that a status frame with corrupt scaling values yields defaulted channels without throwing.
• Full suite green (1556 passing).

Bench test

Built the example CLI against this worktree's Daqifi.Core and streamed a real Nyquist over USB (/dev/cu.usbmodem1101, 10 Hz, ch0+1, 3 s): channels populated, samples flowed with validation active, exit code 0 — normal device data passes the new validation unaffected.

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Validate AnalogChannel scaling bounds and add bipolar/signed scaling coverage

🐞 Bug fix ✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Enforce physically plausible bounds for AnalogChannel resolution, ranges, and calibration
 coefficients.
• Sanitize corrupt device-reported scaling values during channel population to avoid
 crashes/garbage.
• Add IsBipolar accessor and expand tests for signed/bipolar scaling behavior.

Diagram

graph TD
  Status["Status frame"] --> Device["DaqifiDevice"] --> Sanitize{"Sanitize values?"} --> Channel["AnalogChannel"] --> Scale["GetScaledValue"] --> Consumer["UI/Client"]
  Channel --> Consumer

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Clamp inside AnalogChannel setters instead of throwing

• ➕ Avoids exceptions for all callers (not just device population)
• ➕ Keeps channel instances always in a usable state even with bad inputs
• ➖ Silently hides programming/configuration errors
• ➖ Harder to detect invalid configurations early; may mask bugs in callers

2. Introduce a validated ScalingConfig object

• ➕ Single place to validate/sanitize; can be constructed via factory/TryCreate
• ➕ Clear separation between raw device inputs and runtime channel state
• ➖ Larger API change; more types and migration work
• ➖ Potentially overkill if only a few fields need guarding

3. TryUpdateScalingFromStatus(...) returning success/error

• ➕ Avoids throwing while still surfacing failure to caller
• ➕ Allows caller to decide on fallback vs. abort population
• ➖ More branching/handling required at each call site
• ➖ Existing setter-based API still needs guidance/consistency

Recommendation: Current approach (strict validation in AnalogChannel + sanitization at the device/status boundary) is the best tradeoff: it prevents nonsensical scaling from entering the system while keeping device population robust against corrupted frames. The main alternatives either risk silently hiding invalid inputs (clamping everywhere) or require a larger API redesign (config object / TryUpdate) for limited incremental benefit in this context.

Files changed (5) +432 / -8

Enhancement (1) +7 / -0

IAnalogChannel.csExpose IsBipolar on the analog channel interface +7/-0

Expose IsBipolar on the analog channel interface

• Extends IAnalogChannel with a read-only IsBipolar property so consumers can detect bipolar vs. unipolar configured ranges.

src/Daqifi.Core/Channel/IAnalogChannel.cs

Bug fix (2) +194 / -8

AnalogChannel.csAdd scaling bounds validation and IsBipolar accessor +148/-8

Add scaling bounds validation and IsBipolar accessor

• Introduces public constants for resolution/range/calibration bounds and enforces them in constructor and property setters via shared validation helpers. Adds IsBipolar derived from MinValue to support polarity-aware consumers without hardcoded assumptions.

src/Daqifi.Core/Channel/AnalogChannel.cs

DaqifiDevice.csSanitize device scaling coefficients during analog channel population +46/-0

Sanitize device scaling coefficients during analog channel population

• Adds sanitization helpers in PopulateAnalogChannels to replace NaN/Infinity/out-of-range/zero scale factors and invalid port ranges with safe defaults and Trace logging. Prevents validating setters from throwing and avoids propagating corrupt coefficients into scaled samples.

src/Daqifi.Core/Device/DaqifiDevice.cs

Tests (2) +231 / -0

AnalogChannelTests.csAdd validation and bipolar/signed scaling test coverage +190/-0

Add validation and bipolar/signed scaling test coverage

• Adds constructor/setter boundary and invalid-input tests for resolution, port range, calibration coefficients, and min/max finiteness. Adds explicit signed/bipolar GetScaledValue scenarios (negative raw counts, symmetry, offset ordering, inverting slope) plus IsBipolar behavior.

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

ChannelPopulationTests.csRegression test for corrupt device scaling value sanitization +41/-0

Regression test for corrupt device scaling value sanitization

• Adds a status-frame population test ensuring NaN/Infinity/zero-scale/negative-range inputs are substituted with defaults without throwing, while valid channel values remain intact.

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

[qodo-code-review]

Code Review by Qodo

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

Context used

✅ Tickets: 🎫 Add resolution/range/calibration bounds validation to AnalogChannel 🎫 Verify and cover bipolar/negative voltage range scaling for differential inputs

1. Invalid resolution aborts population ✓ Resolved 🐞 Bug ☼ Reliability

Description

AnalogChannel now throws if Resolution is outside [255..16,777,216], but PopulateAnalogChannels only
substitutes a fallback when AnalogInRes==0. A corrupted status frame with a non-zero out-of-range
AnalogInRes will throw ArgumentOutOfRangeException during channel creation and abort channel
population.

Code

src/Daqifi.Core/Channel/AnalogChannel.cs[R235-241]

        if (channelNumber < 0)
            throw new ArgumentOutOfRangeException(nameof(channelNumber), "Channel number must be non-negative.");

-        if (resolution == 0)
-            throw new ArgumentOutOfRangeException(nameof(resolution), "Resolution must be greater than zero.");
+        ValidateResolution(resolution, nameof(resolution));

        ChannelNumber = channelNumber;
        _resolution = resolution;

Relevance

⭐⭐⭐ High

Team prioritizes robust status/channel population; similar hardening and “don’t crash on corrupt
frames” fixes accepted.

PR-#314
PR-#325

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

AnalogChannel’s constructor now calls ValidateResolution, which throws for out-of-range values.
PopulateAnalogChannels only falls back when AnalogInRes==0 and otherwise passes the device value
directly into the AnalogChannel constructor, so a non-zero corrupt resolution now becomes a hard
failure during status population.

src/Daqifi.Core/Channel/AnalogChannel.cs[233-242]
src/Daqifi.Core/Channel/AnalogChannel.cs[328-341]
src/Daqifi.Core/Device/DaqifiDevice.cs[1328-1337]
src/Daqifi.Core/Device/DaqifiDevice.cs[1363-1374]

Agent prompt

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

### Issue description
`AnalogChannel` now validates `resolution` via `ValidateResolution(...)`. However, `DaqifiDevice.PopulateAnalogChannels` only treats `AnalogInRes==0` as invalid. Any other corrupted-but-nonzero value (e.g. `1`, `uint.MaxValue`) will be passed into `new AnalogChannel(i, resolution, ...)` and throw, aborting status-based channel population.

### Issue Context
This PR added strict bounds validation for resolution in `AnalogChannel`. The status ingestion path already sanitizes floating-point scaling fields (calibration/portRange) but does not sanitize `AnalogInRes` unless it is exactly 0.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiDevice.cs[1328-1337]
- src/Daqifi.Core/Device/DaqifiDevice.cs[1363-1374]
- src/Daqifi.Core/Channel/AnalogChannel.cs[328-341]

### Suggested fix
1. In `PopulateAnalogChannels`, validate/sanitize `analogInResolution` similarly to the coefficient sanitizers:
  - If `analogInResolution` is outside `AnalogChannel.MinResolution..AnalogChannel.MaxResolution`, log via `Trace.WriteLine(...)`, set `resolutionIsAssumed = true`, and substitute a safe fallback (e.g. 65535).
2. Use the sanitized `resolution` for both new channel construction and `UpdateScalingFromStatus` updates.

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

---

2. Scaling update skips validation ✓ Resolved 🐞 Bug ≡ Correctness

Description

PopulateAnalogChannels updates reused channels via UpdateScalingFromStatus, which writes backing
fields directly and does not invoke any validation helpers. This allows an invalid non-zero
AnalogInRes (and any future unsanitized scaling inputs) to be installed into an existing
AnalogChannel, silently corrupting GetScaledValue results.

Code

src/Daqifi.Core/Device/DaqifiDevice.cs[R1356-1358]

                if (existingByKey.TryGetValue((ChannelType.Analog, i), out var existing) && existing is AnalogChannel existingAnalog)
                {
                    existingAnalog.UpdateScalingFromStatus(resolution, calibrationB, calibrationM, internalScaleM, portRange, resolutionIsAssumed);

Relevance

⭐⭐⭐ High

They accepted UpdateScalingFromStatus refactor; likely to also enforce validation/sanitization on
reuse path.

PR-#314
PR-#325

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PopulateAnalogChannels calls UpdateScalingFromStatus for existing channels, and
UpdateScalingFromStatus directly assigns backing fields without calling
ValidateResolution/ValidateScaleFactor/ValidatePortRange. Since PopulateAnalogChannels currently
only special-cases AnalogInRes==0, a non-zero out-of-range resolution can bypass validation entirely
on the reuse path and corrupt scaling.

src/Daqifi.Core/Device/DaqifiDevice.cs[1328-1333]
src/Daqifi.Core/Device/DaqifiDevice.cs[1356-1361]
src/Daqifi.Core/Channel/AnalogChannel.cs[254-274]

Agent prompt

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

### Issue description
`AnalogChannel.UpdateScalingFromStatus(...)` assigns `_resolution`, `_calibration*`, `_internalScaleM`, and `_portRange` directly, bypassing the validating setters and helpers introduced/used elsewhere in `AnalogChannel`. With current code, `PopulateAnalogChannels` sanitizes coefficient fields but does not sanitize resolution; therefore an invalid non-zero `AnalogInRes` can be installed into *reused* channels without throwing, yielding silently incorrect scaling.

### Issue Context
- New channel creation uses `AnalogChannel` constructor which validates resolution.
- Reused channels are updated via `UpdateScalingFromStatus`, which currently does **no** validation.
This creates inconsistent enforcement: new channels may throw, reused channels may accept invalid state.

### Fix Focus Areas
- src/Daqifi.Core/Channel/AnalogChannel.cs[254-274]
- src/Daqifi.Core/Device/DaqifiDevice.cs[1356-1361]
- src/Daqifi.Core/Device/DaqifiDevice.cs[1328-1337]

### Suggested fix
1. Add validation inside `UpdateScalingFromStatus`:
  - Call `ValidateResolution(resolution, nameof(resolution))`.
  - Call `ValidateOffset/ValidateScaleFactor/ValidatePortRange` for the other parameters.
2. Ensure `PopulateAnalogChannels` sanitizes `resolution` before calling `UpdateScalingFromStatus` (and before constructing new channels). If you prefer not to throw from `UpdateScalingFromStatus` in production paths, consider:
  - Adding a `TryUpdateScalingFromStatus(...)` that returns `bool`, and
  - Having `PopulateAnalogChannels` fall back + log when validation fails.

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

---

3. CalibrationM allows negative values 📎 Requirement gap ≡ Correctness

Description

AnalogChannel.CalibrationM validation allows negative scale factors, but the compliance checklist
requires rejecting negative CalibrationM values as physically nonsensical. This may permit
inverted calibration behavior contrary to the specified bounds policy.

Code

src/Daqifi.Core/Channel/AnalogChannel.cs[R359-372]

+    /// Validates a multiplicative scale factor (<see cref="CalibrationM"/>/<see cref="InternalScaleM"/>):
+    /// finite, non-zero, and within ±<see cref="MaxCalibrationMagnitude"/>. Negative factors are allowed
+    /// (they invert the signal); zero is not (it discards the measurement entirely).
+    /// </summary>
+    /// <exception cref="ArgumentOutOfRangeException">Thrown when <paramref name="value"/> is not a valid scale factor.</exception>
+    internal static void ValidateScaleFactor(double value, string paramName)
+    {
+        if (!double.IsFinite(value) || value == 0.0 || Math.Abs(value) > MaxCalibrationMagnitude)
+        {
+            throw new ArgumentOutOfRangeException(
+                paramName, value,
+                $"Scale factor must be a finite, non-zero value within ±{MaxCalibrationMagnitude}.");
+        }
+    }

Relevance

⭐ Low

Repo codifies negative CalibrationM as valid (signal inversion) with explicit unit tests; unlikely
to reverse.

PR-#279
PR-#325

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 3 requires CalibrationM bounds checking that rejects negative scale factors. The
updated ValidateScaleFactor() explicitly states negative factors are allowed and does not check
value < 0, and the new unit test CalibrationM_WithNegativeValue_IsAccepted codifies this
behavior.

Add bounds checking for CalibrationM and CalibrationB (finite and reasonable)
src/Daqifi.Core/Channel/AnalogChannel.cs[358-372]
src/Daqifi.Core.Tests/Channel/AnalogChannelTests.cs[323-329]

Agent prompt

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

## Issue description
Compliance requires `CalibrationM` to reject negative values, but current validation explicitly permits negative scale factors.

## Issue Context
`ValidateScaleFactor()` is used for both `CalibrationM` and `InternalScaleM` and currently only rejects non-finite, zero, or over-magnitude values; tests also assert negative `CalibrationM` is accepted.

## Fix Focus Areas
- src/Daqifi.Core/Channel/AnalogChannel.cs[171-178]
- src/Daqifi.Core/Channel/AnalogChannel.cs[358-372]
- src/Daqifi.Core.Tests/Channel/AnalogChannelTests.cs[323-329]

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

---

[tylerkron]

Re Qodo's optional finding "CalibrationM allows negative values": keeping this as designed. Ticket #300 scopes "reject zero/negative" to PortRange; for CalibrationM it asks for "reasonable scale-factor bounds," which ValidateScaleFactor enforces (finite, non-zero, bounded magnitude). A negative slope is a legitimate calibration for an inverted/differential channel — directly relevant to the bipolar work in #297 — so rejecting it would break valid configurations. Qodo itself rates this ⭐ Low / "unlikely to reverse," consistent with existing repo precedent. Keeping CalibrationM_WithNegativeValue_IsAccepted.