feat(channel): say what a reading means, not just what it measures

[tylerkron]

What was wrong

If you wire a 0-100 PSI transducer to an analog terminal, the library gave you back volts and left the conversion to you. It even knew better: the device's capability document states each channel's unit, Core parsed it, and then dropped it on the floor — no consumer ever read it. So anyone who wanted "12.4 PSI" instead of "2.48 V" wrote their own scaling, and the desktop app duly did, mutating each sample in place as it went.

How it was fixed

An analog channel now takes a ChannelScaling — a gain, an offset, and a unit label:

((IScaledChannel)ai0).Scaling = new ChannelScaling(gain: 20.0, offset: 0.0, unit: "PSI");
// sample.Value      -> 2.48   (the volts the device reported, unchanged)
// sample.ScaledValue -> 49.6
// sample.Unit        -> "PSI"

Every sample decoded from then on carries it. Two decisions a reviewer might want to push on:

• The scaling travels on the sample, not looked up from the channel. That is what stops a reconfiguration from retroactively reinterpreting readings that were already taken, and it means nothing is ever mutated in place. It costs one reference per sample — the scaling object is shared, so there is no per-sample allocation.
• Value keeps its old meaning. The converted number lives on the new ScaledValue, which equals Value when no scaling is set, so no existing consumer's numbers move.

The whole change is additive: IScaledChannel is a new capability interface (if (channel is IScaledChannel scaled)) rather than new members on IChannel/IAnalogChannel, and IDataSample's three new members are defaulted — an implementation written before this existed still compiles and reports "no scaling", which is what it has. There is a test that would stop compiling if that ever stopped being true.

Connecting now also copies the device's own unit onto each analog channel as an identity scaling — a label, no arithmetic — and never overwrites a scaling you configured, which matters because the MCP layer re-reads the capability document after every channel-configuration call.

This is Tier 1 of the issue (linear scaling, no new dependency). Tier 2 — expression-based scaling — is deliberately left as the separate decision the issue frames it as.

Verified

• +54 tests. Full suite green on net9.0 + net10.0: 3189 passed / 2 skipped each, plus 86 in Daqifi.Mcp.Tests, 0 warnings.
• Bench, real Nyquist (fw 3.7.2, USB, non-destructive — streaming and SCPI only). All 16 analog channels picked up Unit="V" from the device's own capability document on connect, as an identity scaling, with the digital channels correctly left bare. A live channel was then given a 20x + 1 PSI conversion and streamed at 200 Hz: 1,095 samples, ScaledValue == Value * gain + offset to 0.00E+00 max deviation on real firmware data (0.0085-0.0110 V → 1.1709-1.2197 PSI), with the volts still readable alongside. Reconfiguring the gain mid-stream took effect on the next frame without any channel-set change, the 778 samples already taken kept their original scaling object, the neighbouring channel stayed on plain volts, and a capability re-read did not clobber the configured conversion.

closes #501

Not merging — opened for review.

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Add engineering-unit scaling for analog channels and stamp it onto samples

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Add optional per-channel ChannelScaling (gain/offset/unit) for analog inputs via
 IScaledChannel.
• Stamp scaling onto each decoded sample to preserve historical meaning across reconfiguration.
• Read per-channel units from the capability document as identity scaling without overwriting user
 scaling.

Diagram

graph TD
  A["Caller app"] --> B["AnalogChannel (IScaledChannel)"] --> C["DataSample (Value/ScaledValue/Unit)"] --> I["MCP ChannelInfo.Unit"]
  B --> D["ChannelScaling (gain/offset/unit)"]
  E["DaqifiDevice.ReadCapabilityDocumentAsync"] --> F[("Capability document")] --> G["CapabilityChannelUnits"] --> B
  B --> H["StreamFrameDecoder"] --> C

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Look up scaling from channel at read-time only

• ➕ Avoids carrying a Scaling reference per sample
• ➕ Simplifies sample object model (no Scaling field)
• ➖ Reconfiguration can retroactively reinterpret historical readings unless additional snapshotting is introduced
• ➖ Encourages in-place mutation patterns or implicit reinterpretation that this PR explicitly avoids

2. Add scaling members directly to IChannel/IAnalogChannel

• ➕ More discoverable API surface (no capability interface cast)
• ➕ Fewer interface types
• ➖ Breaking change for external implementations of the interfaces
• ➖ Forces digital channels/other implementations to adopt irrelevant members

3. Represent scaling as an expression/function rather than linear gain/offset

• ➕ Supports non-linear transducers and richer conversions
• ➕ May reduce future churn for Tier 2
• ➖ Higher complexity and potential dependency/security concerns
• ➖ Harder to guarantee decode-thread safety (no throws, no NaN/Inf) and performance predictability

Recommendation: The PR’s approach (linear ChannelScaling + IScaledChannel capability + stamping scaling onto each sample) is the best fit for backwards compatibility and correctness over time. It preserves existing Value semantics, prevents retroactive reinterpretation after reconfiguration, avoids per-sample allocations by sharing an immutable scaling instance, and keeps decode-thread behavior safe (no exceptions / no non-finite propagation). Tier 2 expression-based scaling can be layered later without undermining this model.

Files changed (16) +1077 / -5

Enhancement (9) +353 / -5

AnalogChannel.csMake AnalogChannel implement IScaledChannel with lock-free scaling reads +25/-2

Make AnalogChannel implement IScaledChannel with lock-free scaling reads

• Adds an immutable ChannelScaling field with Volatile read/write to avoid decode-path locking. Ensures SetActiveSample(value) stamps the current Scaling onto the created DataSample while the SetActiveSample(IDataSample) overload remains non-overwriting.

src/Daqifi.Core/Channel/AnalogChannel.cs

ChannelScaling.csAdd ChannelScaling immutable linear transform + unit label +118/-0

Add ChannelScaling immutable linear transform + unit label

• Introduces ChannelScaling as a sealed record with Gain/Offset/Unit, Identity instance, Apply() that never returns non-finite (falls back to raw), and unit normalization/WithUnit optimization for repeated capability refreshes.

src/Daqifi.Core/Channel/ChannelScaling.cs

DataSample.csAttach scaling metadata to DataSample and expose ScaledValue/Unit +17/-0

Attach scaling metadata to DataSample and expose ScaledValue/Unit

• Adds init-only Scaling property and computed ScaledValue/Unit accessors. Keeps Value meaning unchanged (raw volts) while enabling additive engineering-unit support on the same sample object.

src/Daqifi.Core/Channel/DataSample.cs

IDataSample.csAdd defaulted scaling members to IDataSample +29/-0

Add defaulted scaling members to IDataSample

• Adds default interface members for Scaling, ScaledValue, and Unit to keep the change additive for external implementations. Documents why scaling is carried on the sample (non-retroactive semantics).

src/Daqifi.Core/Channel/IDataSample.cs

IScaledChannel.csIntroduce IScaledChannel capability interface for engineering units +46/-0

Introduce IScaledChannel capability interface for engineering units

• Adds a new capability interface exposing ChannelScaling and a Unit shorthand, explicitly designed to avoid breaking IChannel/IAnalogChannel external implementations. Documents that digital channels intentionally do not implement it.

src/Daqifi.Core/Channel/IScaledChannel.cs

CapabilityChannelUnits.csApply capability-document units to analog channels as identity scaling +90/-0

Apply capability-document units to analog channels as identity scaling

• Adds a capability consumer that matches analog inputs by kind+id, converts the unit field into an identity ChannelScaling, and never overwrites a caller-configured scaling. Returns a count for logging.

src/Daqifi.Core/Device/Capabilities/CapabilityChannelUnits.cs

DaqifiDevice.csSeed channel units during capability document read +13/-0

Seed channel units during capability document read

• Hooks CapabilityChannelUnits into ReadCapabilityDocumentAsync so analog channels gain their device-reported unit on connect/refresh. Adds debug logging of how many channels were updated.

src/Daqifi.Core/Device/DaqifiDevice.cs

StreamFrameDecoder.csStamp per-sample scaling during analog decode +9/-1

Stamp per-sample scaling during analog decode

• Updates analog decode to read IScaledChannel.Scaling per sample (to handle mid-stream changes without cache invalidation) and stamp it onto the constructed DataSample. Preserves raw Value and avoids in-place conversion/mutation.

src/Daqifi.Core/Device/Internal/StreamFrameDecoder.cs

Dtos.csExpose channel unit in MCP ChannelInfo DTO +6/-2

Expose channel unit in MCP ChannelInfo DTO

• Extends ChannelInfo with an optional Unit property and populates it from IScaledChannel.Unit so downstream consumers can display engineering units.

src/Daqifi.Mcp/Dtos.cs

Tests (6) +691 / -0

AnalogChannelTests.csAdd AnalogChannel scaling behavior tests +97/-0

Add AnalogChannel scaling behavior tests

• Adds tests covering default null scaling, Unit shorthand, stamping scaling on samples created by SetActiveSample(value), preserving caller-supplied samples, non-retroactive semantics across reconfiguration, clearing scaling, and ordering relative to device calibration.

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

ChannelScalingTests.csIntroduce ChannelScaling unit test suite +164/-0

Introduce ChannelScaling unit test suite

• Adds thorough tests for constructor validation/normalization, Apply semantics (gain then offset), overflow/non-finite handling, Identity/IsIdentity behavior, WithUnit allocation avoidance, immutability, and value equality.

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

DataSampleTests.csTest sample scaled value/unit defaults and compatibility +79/-0

Test sample scaled value/unit defaults and compatibility

• Verifies ScaledValue falls back to Value when no scaling is present, that scaling converts without mutating Value, that ScaledValue reflects later Value writes, that overflow degrades safely, and that legacy IDataSample implementations still compile via default interface members.

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

CapabilityChannelUnitsTests.csTest capability unit application rules +129/-0

Test capability unit application rules

• Adds tests for applying per-channel unit as identity scaling, matching by kind+id, stable mapping across ordering, never overwriting configured scaling, and correctly handling blank/missing units and digital channels.

src/Daqifi.Core.Tests/Device/Capabilities/CapabilityChannelUnitsTests.cs

DaqifiDeviceCapabilityDocumentTests.csVerify capability document seeds analog units and preserves overrides +61/-0

Verify capability document seeds analog units and preserves overrides

• Extends device capability document tests to assert analog channels receive the document unit (as identity scaling), digital channels do not implement IScaledChannel, configured scaling is not overwritten on refresh, and untrusted documents do not partially apply units.

src/Daqifi.Core.Tests/Device/Capabilities/DaqifiDeviceCapabilityDocumentTests.cs

StreamFrameDecoderTests.csValidate scaling propagation through both USB and WiFi decode paths +161/-0

Validate scaling propagation through both USB and WiFi decode paths

• Adds tests ensuring pre-scaled float samples and raw-count samples carry channel scaling correctly, scaling is per-channel, scaling changes mid-stream take effect immediately without cache invalidation, overflow never faults decode, and digital samples carry no scaling.

src/Daqifi.Core.Tests/Device/Internal/StreamFrameDecoderTests.cs

Documentation (1) +33 / -0

DEVICE_INTERFACES.mdDocument engineering-unit scaling and sample semantics +33/-0

Document engineering-unit scaling and sample semantics

• Adds a new section describing ChannelScaling, how to configure it via IScaledChannel, and how samples expose Value vs ScaledValue and Unit. Clarifies identity unit seeding from the capability document and overflow behavior.

docs/DEVICE_INTERFACES.md

[qodo-code-review]

Code Review by Qodo

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

1. Misleading Apply finiteness docs ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

ChannelScaling's XML remarks claim Apply “never returns a non-finite value”, but Apply intentionally
returns non-finite inputs (NaN/Infinity) unchanged, as verified by tests. This misleading contract
can cause consumers/maintainers to incorrectly assume ScaledValue is always finite and omit needed
double.IsFinite handling.

Code

src/Daqifi.Core/Channel/ChannelScaling.cs[R23-27]

+/// <see cref="Gain"/> and <see cref="Offset"/> are validated when the instance is constructed —
+/// on the caller's thread, where an exception is a usable error message. <see cref="Apply"/> itself
+/// never throws and never returns a non-finite value: a configuration whose arithmetic overflows
+/// for a particular reading degrades to the unscaled value for that reading rather than poisoning
+/// the stream, because it runs on the decode thread where there is nobody to catch it.

Relevance

●●● Strong

Team often accepts fixes aligning XML docs/comments with actual behavior; misleading contracts are
routinely corrected.

PR-#321
PR-#357
PR-#466

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The ChannelScaling XML remarks assert a finiteness guarantee that is contradicted by the unit test
explicitly asserting NaN/Infinity are returned unchanged by Apply.

src/Daqifi.Core/Channel/ChannelScaling.cs[23-27]
src/Daqifi.Core.Tests/Channel/ChannelScalingTests.cs[100-107]

Agent prompt

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

## Issue description
`ChannelScaling`’s XML remarks promise that `Apply` “never returns a non-finite value”, but the implementation (and tests) preserve non-finite inputs by returning the original input when the computed result is non-finite.

## Issue Context
This is a public API documentation contract mismatch that can mislead downstream consumers into assuming `ScaledValue`/`Apply(...)` always produces finite outputs.

## Fix Focus Areas
- src/Daqifi.Core/Channel/ChannelScaling.cs[23-27]

## Suggested fix
Update the remarks to match reality, e.g.:
- “Apply never throws; for finite inputs it never produces a non-finite result (overflow falls back to the original finite input). Non-finite inputs are returned unchanged.”
Optionally align the `<returns>` text similarly if needed.

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

---

Tip of the day

💡 Did you know, you can type 'qodo, fix this' on a finding and the fix lands right on your PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous review results

Review updated until commit 034f095 ⚖️ Balanced

Results up to commit 00f5280 ⚖️ Balanced

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

1. Misleading Apply finiteness docs ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

ChannelScaling's XML remarks claim Apply “never returns a non-finite value”, but Apply intentionally
returns non-finite inputs (NaN/Infinity) unchanged, as verified by tests. This misleading contract
can cause consumers/maintainers to incorrectly assume ScaledValue is always finite and omit needed
double.IsFinite handling.

Code

src/Daqifi.Core/Channel/ChannelScaling.cs[R23-27]

+/// <see cref="Gain"/> and <see cref="Offset"/> are validated when the instance is constructed —
+/// on the caller's thread, where an exception is a usable error message. <see cref="Apply"/> itself
+/// never throws and never returns a non-finite value: a configuration whose arithmetic overflows
+/// for a particular reading degrades to the unscaled value for that reading rather than poisoning
+/// the stream, because it runs on the decode thread where there is nobody to catch it.

Relevance

●●● Strong

Team often accepts fixes aligning XML docs/comments with actual behavior; misleading contracts are
routinely corrected.

PR-#321
PR-#357
PR-#466

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The ChannelScaling XML remarks assert a finiteness guarantee that is contradicted by the unit test
explicitly asserting NaN/Infinity are returned unchanged by Apply.

src/Daqifi.Core/Channel/ChannelScaling.cs[23-27]
src/Daqifi.Core.Tests/Channel/ChannelScalingTests.cs[100-107]

Agent prompt

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

## Issue description
`ChannelScaling`’s XML remarks promise that `Apply` “never returns a non-finite value”, but the implementation (and tests) preserve non-finite inputs by returning the original input when the computed result is non-finite.

## Issue Context
This is a public API documentation contract mismatch that can mislead downstream consumers into assuming `ScaledValue`/`Apply(...)` always produces finite outputs.

## Fix Focus Areas
- src/Daqifi.Core/Channel/ChannelScaling.cs[23-27]

## Suggested fix
Update the remarks to match reality, e.g.:
- “Apply never throws; for finite inputs it never produces a non-finite result (overflow falls back to the original finite input). Non-finite inputs are returned unchanged.”
Optionally align the `<returns>` text similarly if needed.

ⓘ 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 034f095

[tylerkron]

Qodo-clean, CI green — ready for review.

2 rounds on head 034f095. Round 1 (on 00f5280) found one valid issue — the ChannelScaling remarks claimed Apply never returns a non-finite value, which the tests contradict; fixed and the thread resolved. Round 2 came back Bugs (0) / Rule violations (0) / Skill insights (0) with the earlier finding struck through, 0 unresolved review threads, and its SHA reference matching this head. Settle re-check at +4 min: review comment byte-identical, still 0 unresolved threads, build SUCCESS, MERGEABLE/CLEAN.

Full suite green on net9.0 + net10.0 (3189 passed / 2 skipped each, +86 Mcp, 0 warnings). Bench re-run on the real Nyquist against this head: all 13 checks pass, including ScaledValue == Value * gain + offset to 0.00E+00 max deviation over 1,097 samples of real firmware data.

Not merging — for review.