refactor(sdcard): extract shared ScaleRawAnalogValues helper

[tylerkron]

Summary

• Extracts the byte-identical ScaleRawAnalogValues bodies (and their copy-pasted <remarks> doc block) out of SdCardFileParser, SdCardCsvFileParser, and SdCardJsonFileParser into a single internal SdCardAnalogScaling.ScaleRawAnalogValues helper.
• All three parsers now delegate to the shared implementation, which itself delegates to AnalogScaling.Scale (unchanged).
• SdCardFileParser's protobuf path converts its RepeatedField<int> to double[] before calling the shared helper, matching the CSV/JSON parsers' IReadOnlyList<double> signature.

Pure refactor — no behavior change.

Closes #462

Test plan

• dotnet build — 0 warnings, 0 errors
• dotnet test (SD-card filter) — 405/405 passed
• dotnet test (full suite) — 2826/2826 passed, 2 skipped (pre-existing)

🤖 Generated with Claude Code

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Refactor SD-card parsers to share ScaleRawAnalogValues helper

✨ Enhancement 🕐 20-40 Minutes

AI Description

• Extract shared SD-card analog scaling into internal SdCardAnalogScaling helper.
• Update protobuf/CSV/JSON parsers to delegate to the shared scaling implementation.
• Convert protobuf raw ints to double[] before scaling for signature consistency.

Diagram

graph TD
  Csv["SdCardCsvFileParser"] --> Shared["SdCardAnalogScaling"]
  Json["SdCardJsonFileParser"] --> Shared
  Proto["SdCardFileParser"] --> Conv["int→double[]"] --> Shared --> Scale["AnalogScaling.Scale"]
  Cfg["SdCardDeviceConfiguration"] --> Shared

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Move helper into AnalogScaling as an overload taking SdCardDeviceConfiguration

• ➕ Single canonical place for scaling logic across the codebase
• ➕ Could reduce SD-card-specific surface area
• ➖ Tangles generic channel scaling with SD-card configuration semantics
• ➖ May broaden API surface for a narrowly-scoped refactor

2. Introduce a shared SD-card parser base class with protected scaling method

• ➕ Keeps helper close to parsing workflow
• ➕ Avoids static helper if inheritance already exists
• ➖ Adds inheritance/structure purely to share one method
• ➖ Likely more churn than a small internal helper

Recommendation: Current approach (internal SdCardAnalogScaling helper) is the best trade-off: it guarantees identical scaling across protobuf/CSV/JSON without expanding public APIs or restructuring parser types, and keeps the logic scoped to the SD-card domain.

Files changed (4) +58 / -109

Refactor (4) +58 / -109

SdCardAnalogScaling.csAdd shared SD-card analog scaling helper +53/-0

Add shared SD-card analog scaling helper

• Introduces internal SdCardAnalogScaling.ScaleRawAnalogValues to centralize per-channel calibration/range/internal-scale handling and delegate the final computation to AnalogScaling.Scale. This removes the risk of the three SD-card parsing paths drifting in behavior.

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

SdCardCsvFileParser.csDelegate CSV analog scaling to shared helper +1/-36

Delegate CSV analog scaling to shared helper

• Replaces the local ScaleRawAnalogValues implementation with a call to SdCardAnalogScaling.ScaleRawAnalogValues. Deletes the now-duplicated private method and its duplicated XML remarks.

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

SdCardFileParser.csDelegate protobuf analog scaling and normalize raw ints +3/-37

Delegate protobuf analog scaling and normalize raw ints

• Replaces the protobuf-specific ScaleRawAnalogValues method by converting RepeatedField<int> to double[] and calling SdCardAnalogScaling.ScaleRawAnalogValues. Adjusts local variable type to IReadOnlyList<double> and removes the duplicated scaling method.

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

SdCardJsonFileParser.csDelegate JSON analog scaling to shared helper +1/-36

Delegate JSON analog scaling to shared helper

• Replaces the local ScaleRawAnalogValues implementation with a call to SdCardAnalogScaling.ScaleRawAnalogValues. Deletes the duplicated method and associated XML remarks.

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

[qodo-code-review]

Code Review by Qodo

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

1. Extra scaling array allocation ✓ Resolved 🐞 Bug ➹ Performance

Description

In SdCardFileParser’s protobuf raw-int path, the code now materializes an intermediate double[] via
Select(...).ToArray() and then SdCardAnalogScaling allocates another double[] for the scaled result,
adding per-record GC pressure for large/high-rate binary logs.

Code

src/Daqifi.Core/Device/SdCard/SdCardFileParser.cs[R314-316]

+                analogValues = SdCardAnalogScaling.ScaleRawAnalogValues(
+                    msg.AnalogInData.Select(v => (double)v).ToArray(), config);
            }

Relevance

●●● Strong

Team has accepted removing ToArray/extra allocations in hot parsing paths; this is a similar
low-risk perf win.

PR-#169

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The protobuf raw-int path now allocates an intermediate double[] before calling the shared scaler,
and the shared scaler always allocates a new double[] for results when scaling is
enabled—resulting in two arrays per parsed record (configured case) instead of one.

src/Daqifi.Core/Device/SdCard/SdCardFileParser.cs[306-316]
src/Daqifi.Core/Device/SdCard/SdCardAnalogScaling.cs[29-49]

Agent prompt

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

### Issue description
`SdCardFileParser` converts `msg.AnalogInData` (a `RepeatedField<int>`) to a `double[]` before scaling. The shared scaler then allocates a second `double[]` for the result when `config != null && config.Resolution != 0`. This introduces an extra per-record allocation versus the previous protobuf-specific implementation.

### Issue Context
The CSV/JSON paths already operate on `IReadOnlyList<double>`, but the protobuf raw-int path does not need to allocate an intermediate `double[]` just to feed the shared helper.

### Fix Focus Areas
- src/Daqifi.Core/Device/SdCard/SdCardFileParser.cs[306-316]
- src/Daqifi.Core/Device/SdCard/SdCardAnalogScaling.cs[25-52]

### Suggested fix
- Add an overload in `SdCardAnalogScaling`, e.g.:
 - `internal static double[] ScaleRawAnalogValues(IReadOnlyList<int> rawValues, SdCardDeviceConfiguration? config)` (or a `RepeatedField<int>` overload)
 - Implement scaling by reading `rawValues[ch]` and writing directly into a single `double[] result`.
- Update `SdCardFileParser` to call the new overload, removing the `Select(v => (double)v).ToArray()` intermediate allocation.

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

---

2. Misleading no-config comment ✓ Resolved 🐞 Bug ⚙ Maintainability ⭐ New

Description

In SdCardAnalogScaling.ScaleRawAnalogValues(IReadOnlyList<int>), the no-config branch comment says
it returns raw values “as-is”, but the method necessarily allocates and returns a new double[] with
copied/converted values. This can mislead future maintainers about aliasing/allocation behavior (the
XML doc is accurate; the inline comment is not).

Code

src/Daqifi.Core/Device/SdCard/SdCardAnalogScaling.cs[R59-65]

+        if (config == null || config.Resolution == 0)
+        {
+            // No config or resolution available — return raw values as-is
+            for (var ch = 0; ch < rawValues.Count; ch++)
+            {
+                result[ch] = rawValues[ch];
+            }

Relevance

●●● Strong

Team often accepts fixes clarifying misleading comments/docs to match actual behavior/semantics.

PR-#357
PR-#321
PR-#391

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The method allocates result before the null/zero-resolution check, then in the no-config branch it
fills result[ch] = rawValues[ch]; (int-to-double) and returns result, so it does not return the
original rawValues collection “as-is”.

src/Daqifi.Core/Device/SdCard/SdCardAnalogScaling.cs[57-67]

Agent prompt

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

### Issue description
The integer overload’s no-config branch comment claims it returns raw values “as-is”, but it actually copies/converts into a newly allocated `double[]` (which is correct behavior given the return type). The comment should describe the actual behavior to avoid misleading future refactors/optimizations.

### Issue Context
This is in the new shared helper `SdCardAnalogScaling.ScaleRawAnalogValues(IReadOnlyList<int>, SdCardDeviceConfiguration?)`.

### Fix Focus Areas
- src/Daqifi.Core/Device/SdCard/SdCardAnalogScaling.cs[59-65]

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

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

Previous review results

Review updated until commit 0011c70

Results up to commit c16cdce ⚖️ Balanced

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

1. Extra scaling array allocation ✓ Resolved 🐞 Bug ➹ Performance

Description

In SdCardFileParser’s protobuf raw-int path, the code now materializes an intermediate double[] via
Select(...).ToArray() and then SdCardAnalogScaling allocates another double[] for the scaled result,
adding per-record GC pressure for large/high-rate binary logs.

Code

src/Daqifi.Core/Device/SdCard/SdCardFileParser.cs[R314-316]

+                analogValues = SdCardAnalogScaling.ScaleRawAnalogValues(
+                    msg.AnalogInData.Select(v => (double)v).ToArray(), config);
            }

Relevance

●●● Strong

Team has accepted removing ToArray/extra allocations in hot parsing paths; this is a similar
low-risk perf win.

PR-#169

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The protobuf raw-int path now allocates an intermediate double[] before calling the shared scaler,
and the shared scaler always allocates a new double[] for results when scaling is
enabled—resulting in two arrays per parsed record (configured case) instead of one.

src/Daqifi.Core/Device/SdCard/SdCardFileParser.cs[306-316]
src/Daqifi.Core/Device/SdCard/SdCardAnalogScaling.cs[29-49]

Agent prompt

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

### Issue description
`SdCardFileParser` converts `msg.AnalogInData` (a `RepeatedField<int>`) to a `double[]` before scaling. The shared scaler then allocates a second `double[]` for the result when `config != null && config.Resolution != 0`. This introduces an extra per-record allocation versus the previous protobuf-specific implementation.

### Issue Context
The CSV/JSON paths already operate on `IReadOnlyList<double>`, but the protobuf raw-int path does not need to allocate an intermediate `double[]` just to feed the shared helper.

### Fix Focus Areas
- src/Daqifi.Core/Device/SdCard/SdCardFileParser.cs[306-316]
- src/Daqifi.Core/Device/SdCard/SdCardAnalogScaling.cs[25-52]

### Suggested fix
- Add an overload in `SdCardAnalogScaling`, e.g.:
 - `internal static double[] ScaleRawAnalogValues(IReadOnlyList<int> rawValues, SdCardDeviceConfiguration? config)` (or a `RepeatedField<int>` overload)
 - Implement scaling by reading `rawValues[ch]` and writing directly into a single `double[] result`.
- Update `SdCardFileParser` to call the new overload, removing the `Select(v => (double)v).ToArray()` intermediate allocation.

ⓘ 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 8ffda79

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 0011c70