Repository navigation
fix(streaming): report acquisition statistics in the channel's own units - #554
Conversation
Configure a channel with engineering units -- a pressure transducer reading 12.4 PSI rather than 2.48 V -- and AcquisitionStatistics reported its min, max and mean in VOLTS, with nothing in the snapshot saying which of the two you had got. Both entry points read sample.Value instead of sample.ScaledValue: Record(IChannel, IDataSample) and the attached SampleReceived handler. It looks like an ordering accident rather than a decision -- AcquisitionStatistics landed in #518 four hours before ChannelScaling existed (#523, the same day), so it could not have considered engineering units, and AcquisitionStatisticsTests had zero references to Scaling or ScaledValue, which is why nothing caught it. WHY RECORD THE SCALED VALUE RATHER THAN DOCUMENT THE VOLTS. The issue offered both. The deciding evidence is that a negative gain TRANSPOSES the extremes, so recording volts is not merely mislabelled, it cannot be corrected afterwards: with gain -3, the smallest voltage is the LARGEST engineering reading, so a consumer re-applying the scaling by hand gets a MinValue that is really the maximum. ChannelScaling permits a negative gain -- only finiteness is validated -- so that is a supported configuration, not an abuse. It is also the only place in Core where the loss is unrecoverable. Every other path hands the consumer the sample, so they can read ScaledValue and Unit themselves; this type consumes the samples on their behalf and returns just the derived numbers. ChannelAcquisitionStatistics now carries Unit, which the issue asked for either way -- every analog channel already has one, so the snapshot was dropping a label it was already being handed. The unit follows the same last-seen-wins rule as Name, because a channel's scaling can be reassigned mid-session and freezing the first unit would label later samples with a unit they were not measured in. BEHAVIOUR CHANGE. A consumer with scaling configured sees different numbers -- that is the fix. A consumer without scaling sees exactly what it saw before, since ScaledValue returns Value when there is no scaling. Three tests, each checked for whether it can actually fail: ReportsTheEngineeringValueAndItsUnit fails on the pre-change recorder DoesNotTransposeTheExtremes fails on the pre-change recorder WithNoScaling_IsUnchangedAndReportsNoUnit passes both, so it is a control rather than a proof -- but a real one: mutating the code to default an unstated unit to "V" makes it fail. Baseline-checked: main fails 4 tests on this station before any change (Device.Discovery.LinuxUsbPortDescriptorProviderTests, a WSL/Windows runtime artefact). After: the same 4, passing 3532 -> 3535. Fixes #534
|
/improve |
|
/agentic_review |
PR Code Suggestions ✨Warning
|
|||||||||
PR Summary by QodoReport acquisition statistics in channel engineering units
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
1. Positional parameters break compatibility
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| RecordCore(channel.Type, channel.ChannelNumber, channel.Name, sample.Timestamp, | ||
| sample.ScaledValue, sample.Unit); |
There was a problem hiding this comment.
2. Overflow fallback mislabels volts 🐞 Bug ≡ Correctness
The new sample.ScaledValue and sample.Unit pairing records the original unscaled value but labels it with the engineering unit whenever ChannelScaling.Apply overflows. Such samples are consequently reported as PSI or another configured unit even though the accumulated number remains in volts, and may be mixed with successfully converted readings.
Agent Prompt
## Issue description
`ChannelScaling.Apply` deliberately returns the original value when conversion arithmetic overflows, while `IDataSample.Unit` continues returning the configured engineering unit. Recording that pair makes acquisition statistics label unscaled volts as engineering-unit values.
## Issue Context
Overflow fallback is a supported and tested behavior, not an exception path. Add a way to distinguish successful engineering conversion from fallback, then define statistics behavior for failed conversions without folding unscaled values into an engineering-unit accumulator.
## Fix Focus Areas
- src/Daqifi.Core/Device/AcquisitionStatistics.cs[273-282]
- src/Daqifi.Core/Device/AcquisitionStatistics.cs[533-539]
- src/Daqifi.Core/Channel/ChannelScaling.cs[91-105]
- src/Daqifi.Core/Channel/IDataSample.cs[48-62]
- src/Daqifi.Core.Tests/Device/AcquisitionStatisticsTests.cs[614-691]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
…mid-window Recording the engineering value (the #534 fix) introduced a failure mode that recording raw volts did not have: a window can now hold samples measured in two DIFFERENT units. IScaledChannel documents mid-session reassignment as supported -- "set it and every sample decoded from then on carries it" -- and samples keep the scaling in force when they were decoded. So a channel rescaled from PSI to kPa mid-window produced one aggregate spanning both, labelled with only the newest unit. A minimum in PSI and a maximum in kPa are not the extremes of anything, and the label asserted they were kPa. Before this PR the window was always volts and therefore at least unit-homogeneous, so this was a new way to be wrong, introduced by the fix. The VALUE accumulators now restart when the scaling changes, so Min, Max and Mean always describe exactly one scaling. Timing, ordering and sample counts are unit-independent and continue across the change untouched. Compared on the whole ChannelScaling rather than just its unit label: recalibrating a transducer leaves the unit reading "PSI" while making the earlier numbers incomparable with the later ones. Aggregating those is the same error wearing a matching label. ChannelScaling is a record, so this is structural equality and a fresh-but-equal instance per sample does not look like a reassignment. ChannelAcquisitionStatistics gained ValueSampleCount so the restart is visible rather than an unexplained gap: without it, MeanValue would silently describe fewer samples than SampleCount reports. Three tests, all of which fail without this change: a unit change restarts the figures and reports the new unit; a gain-only change restarts them too; and a stable scaling supplied as a new-but-equal instance each time does NOT restart, which is the control that stops the rule collapsing every window to its last sample. ALSO FIXES THE #531 FLAKE, which this change would otherwise have made deterministic. Record_AfterTheFirstSamplePerChannel_AllocatesNothing warmed up over 100 iterations and measured 10,000. Tiered compilation promotes a hot method partway through a long loop and that rejit allocates; with a larger RecordCore the promotion moved inside the measured window, and the test reported 7,128 bytes across 10,000 records. That is 0.71 bytes per call -- not a per-call allocation at all, since a real one is at least 24 bytes an object and so at least 240 KB here -- but enough to fail an == 0 assertion. So #531's "fails on a full run, passes in isolation" is not run-order dependent, it is warmup-dependent: any change that grows RecordCore pushes the promotion later. Warming over the same count as the measurement puts it firmly before the measurement starts. The assertion keeps its teeth -- a genuine per-call allocation is three orders of magnitude above the noise this removes. Verified 3/3 runs green where it previously failed 3/3. Baseline unchanged: the same 4 pre-existing LinuxUsbPortDescriptorProvider failures, passing 3535 -> 3538. Refs #531
Audit round 1 and Qodo both landed on the same gap. Fixed — it was a way my own fix made things worse.Recording the engineering value introduced a failure mode that recording raw volts did not have: a window can now hold samples measured in two different units.
Before this PR the window was always volts and therefore at least unit-homogeneous. So this was a new way to be wrong that my fix created — not a pre-existing gap it inherited. That is the reason it had to be fixed here rather than filed. The audit recommended resetting the accumulator on a scaling change; that is what landed. What restarts, and what deliberately does notThe value accumulators restart — Compared on the whole
Tests — all three fail without the change
Also in this commit: the #531 flake, which my change would have made deterministic
The failure said 7,128 bytes across 10,000 records. That is 0.71 bytes per call — not a per-call allocation at all, since a real one is ≥24 bytes an object and so ≥240 KB here. It is a one-off inside the measured window: tiered compilation promoting a now-larger So #531's "fails on a full net10 run, passes in isolation" is not run-order dependent — it is warmup-dependent. Any change that grows Baseline unchanged: same 4 pre-existing failures, passing 3532 → 3538. Round 2 of the audit is running. |
|
/agentic_review |
Qodo pass 2: mixed-unit bug ✓ Resolved. One new finding — real mechanism, wrong place to fix it.New: "Overflow fallback mislabels volts"The mechanism is real, and I verified it in source rather than taking it on faith. var scaled = value * Gain + Offset;
return double.IsFinite(scaled) ? scaled : value; // <- the original, in the original unitSo on overflow But this PR cannot fix it, and did not cause it. Given a bare double, Reachability, measured rather than asserted. Overflow needs
Not a transducer configuration anyone types — an adversarial input. And the non-finite-input case is unaffected: One thing worth saying plainly rather than burying: before this PR, no production code in Core read The remaining open suggestion is the same item
State
|
| string? Unit = null, | ||
| long ValueSampleCount = 0) |
There was a problem hiding this comment.
1. Positional parameters break compatibility 🐞 Bug ≡ Correctness
Adding Unit and ValueSampleCount to the public record's primary constructor removes the previous constructor and 14-output Deconstruct signatures; optional defaults only preserve recompiled constructor calls. Existing compiled consumers can fail with MissingMethodException, while source consumers using positional deconstruction no longer compile.
Agent Prompt
## Issue description
Adding positional record parameters changes the generated constructor and `Deconstruct` signatures, breaking existing binary and deconstruction consumers.
## Issue Context
`Daqifi.Core` is a public package, and `SdCardLogEntry` already preserves compatibility by adding new metadata as `init` properties rather than primary-constructor parameters. Preserve the original primary constructor and expose `Unit` and `ValueSampleCount` as properties, initializing `ValueSampleCount` from `SampleCount` for legacy construction; update snapshot creation with an object initializer. Alternatively, provide exact legacy constructor and `Deconstruct` overloads.
## Fix Focus Areas
- src/Daqifi.Core/Device/ChannelAcquisitionStatistics.cs[105-121]
- src/Daqifi.Core/Device/AcquisitionStatistics.cs[187-206]
ⓘ 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 2629b0f |
Audit round 2:
|
| check | result |
|---|---|
| adversarial audit round 2 | ready_to_merge, 1 finding deferred → #557 |
| Qodo bugs | 1 resolved, 1 declined → #555 |
| tests | 4 pre-existing failures (unchanged), passing 3532 → 3538 |
| tests that earn their place | 5 fail on pre-change code; 2 are mutation-proved controls |
| bonus | #531 root-caused as warmup-dependent, not run-order dependent, and fixed |
Merging.
Fixes #534.
The defect
Configure a channel with engineering units — a pressure transducer reading
12.4 PSIrather than2.48 V— andAcquisitionStatisticsreported its min, max and mean in volts, with nothing in the snapshot saying which of the two you had got.Both entry points read
sample.Valueinstead ofsample.ScaledValue:Record(IChannel, IDataSample)and the attachedSampleReceivedhandler.It looks like an ordering accident rather than a decision —
AcquisitionStatisticslanded in #518 four hours beforeChannelScalingexisted (#523, the same day), so it could not have considered engineering units.AcquisitionStatisticsTestshad zero references toScalingorScaledValue, which is why nothing caught it.Why record the scaled value rather than document the volts
The issue laid out both options fairly and called it a judgement call. The deciding evidence is that a negative gain transposes the extremes, so recording volts is not merely mislabelled — it cannot be corrected afterwards:
ChannelScalingpermits a negative gain — only finiteness is validated — so that is a supported configuration, not an abuse. Option 2 (keep volts, document it) leaves that consumer with no safe path at all.It is also the only place in Core where the loss is unrecoverable: every other path hands the consumer the sample so they can read
ScaledValue/Unitthemselves. This type consumes the samples on their behalf and returns only the derived numbers.Uniton the snapshotChannelAcquisitionStatisticsnow carriesUnit— which the issue asked for either way, since every analog channel already has one (CapabilityChannelUnitsgives them an identity scaling labelled"V"at connect), so the snapshot was dropping a label it was already being handed.It follows the same last-seen-wins rule as
Name: a channel's scaling can be reassigned mid-session, and freezing the first unit would label later samples with a unit they were not measured in.Behaviour change — worth a release note
A consumer with scaling configured sees different numbers. That is the fix. A consumer without scaling sees exactly what it saw before, because
ScaledValuereturnsValuewhen there is no scaling.Unitis appended with a default, so existing positional construction still compiles.Test plan — each test checked for whether it can actually fail
ReportsTheEngineeringValueAndItsUnitDoesNotTransposeTheExtremesWithNoScaling_IsUnchangedAndReportsNoUnitThe third passes before and after, so it does not prove anything on its own — I say so rather than implying all three discriminate. It is still worth keeping: mutating the code to default an unstated unit to
"V"makes it fail, which is the plausible way this path would regress.Baseline-checked, not assumed.
mainfails 4 tests on this station before any change — allDevice.Discovery.LinuxUsbPortDescriptorProviderTests, a WSL/Windows runtime artefact unrelated to this work. After this change: the same 4, with passing 3532 → 3535.No hardware needed; the bench reproduction in the issue is already recorded there.
🤖 Generated with Claude Code