feat(sdcard): add SdCardLogEntry.HasDeviceTimestamp

[tylerkron]

Summary

• Add SdCardLogEntry.HasDeviceTimestamp (default true), reporting per-entry whether Timestamp was reconstructed from a real device tick or substituted with the session base time
• Wire it through all three parsers:
  • SdCardFileParser (protobuf): false when MsgTimeStamp == 0 or the tick rate is unknown
  • SdCardJsonFileParser: false when the tick rate is unknown
  • SdCardCsvFileParser: false when the tick rate is unknown

Why

Core already knows, exactly and per-entry, whether a timestamp was real or substituted, but previously threw that away — only the resulting (possibly-substituted) tick was exposed. daqifi-desktop needs this distinction to warn users about a flat time axis on import, so it reverse-engineers it statistically in ImportTimestampQuality.cs by counting how many ticks equal the first tick past a 20% threshold. That heuristic misclassifies any legitimately high-rate file whose first ≥20% of samples happen to share a tick. Exposing the real per-entry flag lets desktop drop the heuristic (and the ~60 lines of Observe/CollapsedFraction/HasFlatTimeAxis state it requires) in favor of an exact answer.

Closes #303

Test plan

• New unit tests in SdCardFileParserTests, SdCardJsonFileParserTests, SdCardCsvFileParserTests covering HasDeviceTimestamp true/false cases (zero message timestamp, unknown tick rate) for all three parsers
• Full dotnet test suite: 1475/1475 passing (net9.0 and net10.0)
• Verified on physical hardware: built the daqifi-core-example-app CLI against this branch (DaqifiCoreProjectPath), downloaded a real SD card log from a Nyquist device on COM3, and confirmed parsed entries report HasDeviceTimestamp=true with correctly reconstructed, monotonically increasing timestamps

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Add per-entry HasDeviceTimestamp flag to SD card parsers

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Add SdCardLogEntry.HasDeviceTimestamp to indicate real vs substituted timestamps.
• Propagate the flag through protobuf, JSON, and CSV SD card parsers.
• Add unit tests covering tick-rate/zero-timestamp scenarios across all parsers.

Diagram

graph TD
  F1{{"Protobuf log (.bin)"}} --> P1["SdCardFileParser"] --> E[("SdCardLogEntry")] --> C["Consumer"]
  F2{{"JSON log (.json)"}} --> P2["SdCardJsonFileParser"] --> E
  F3{{"CSV log (.csv)"}} --> P3["SdCardCsvFileParser"] --> E
  O["SdCardParseOptions"] --> P1
  O --> P2
  O --> P3
  subgraph Legend
    direction LR
    _ext{{"Input file"}} ~~~ _proc["Parser"] ~~~ _data[("Entry model")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Expose a TimestampSource/TimestampQuality enum

• ➕ Extensible beyond a boolean (e.g., real tick vs inferred vs missing)
• ➕ More self-documenting for future cases like partial reconstruction
• ➖ More API surface and downstream branching for a currently binary need
• ➖ May be over-design until additional states are confirmed

2. Keep core unchanged; infer in consumers (heuristics)

• ➕ No core API change and no need to thread a new field through parsers
• ➕ Consumer can tailor logic per product/UI requirements
• ➖ Inherently inaccurate and brittle (already misclassifies valid high-rate logs)
• ➖ Duplicates logic across consumers and is hard to test exhaustively

Recommendation: The PR’s approach (a per-entry boolean emitted by the parsers) is the best fit for the stated requirement: it is precise, low-overhead, and testable at the source of truth. If additional timestamp states emerge later, consider evolving the bool into an enum while keeping HasDeviceTimestamp as a convenience wrapper for backward compatibility.

Files changed (7) +185 / -7

Enhancement (4) +17 / -7

SdCardLogEntry.csAdd HasDeviceTimestamp to SdCardLogEntry record +8/-1

Add HasDeviceTimestamp to SdCardLogEntry record

• Extends SdCardLogEntry with a new HasDeviceTimestamp boolean (default true) and documents when timestamps are reconstructed vs substituted with session base time.

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

SdCardFileParser.csEmit HasDeviceTimestamp from protobuf parser +3/-2

Emit HasDeviceTimestamp from protobuf parser

• Computes per-entry hasDeviceTimestamp based on MsgTimeStamp != 0 and known tick period, and passes it into SdCardLogEntry creation.

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

SdCardJsonFileParser.csEmit HasDeviceTimestamp from JSON parser +3/-2

Emit HasDeviceTimestamp from JSON parser

• Marks HasDeviceTimestamp true only when tickPeriod is available (i.e., timestamp frequency known) and threads the flag into SdCardLogEntry.

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

SdCardCsvFileParser.csEmit HasDeviceTimestamp from CSV parser +3/-2

Emit HasDeviceTimestamp from CSV parser

• Derives hasDeviceTimestamp from tickPeriod > 0 and includes it when yielding SdCardLogEntry values, preserving base-time substitution behavior when tick rate is unknown.

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

Tests (3) +168 / -0

SdCardFileParserTests.csAdd HasDeviceTimestamp unit coverage for protobuf parsing +75/-0

Add HasDeviceTimestamp unit coverage for protobuf parsing

• Adds tests verifying HasDeviceTimestamp is true with real timestamps and false when MsgTimeStamp is zero or timestamp frequency is missing (and no fallback is provided).

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

SdCardJsonFileParserTests.csAdd HasDeviceTimestamp unit coverage for JSON parsing +43/-0

Add HasDeviceTimestamp unit coverage for JSON parsing

• Adds tests asserting HasDeviceTimestamp is true when a timestamp frequency is provided and false when frequency is unavailable (fallback disabled).

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

SdCardCsvFileParserTests.csAdd HasDeviceTimestamp unit coverage for CSV parsing +50/-0

Add HasDeviceTimestamp unit coverage for CSV parsing

• Adds tests asserting HasDeviceTimestamp is true when the CSV provides a timestamp tick rate and false when no tick rate is present and fallback frequency is disabled.

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

[qodo-code-review]

Code Review by Qodo

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

Context used

✅ Tickets: 🎫 feat: report whether an SD log entry had a real device timestamp

1. Binary-breaking record change ✓ Resolved 🐞 Bug ☼ Reliability

Description

Adding HasDeviceTimestamp as a new primary-constructor parameter on the public positional record
SdCardLogEntry changes the generated constructor and Deconstruct signatures, which can cause
runtime MissingMethodException for already-compiled downstream consumers and breaks existing
compiled deconstruction calls.

Code

src/Daqifi.Core/Device/SdCard/SdCardLogEntry.cs[R19-24]

public sealed record SdCardLogEntry(
    DateTime Timestamp,
    IReadOnlyList<double> AnalogValues,
    uint DigitalData,
-    IReadOnlyList<uint>? AnalogTimestamps);
+    IReadOnlyList<uint>? AnalogTimestamps,
+    bool HasDeviceTimestamp = true);

Relevance

⭐⭐⭐ High

Team has accepted avoiding breaking public API signatures/ABI (PR #198, #275), so record positional
change likely revised.

PR-#198
PR-#275

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The PR modifies the public record’s primary constructor signature by adding a new positional
parameter, which changes the generated constructor and deconstruction methods that compiled
consumers bind to.

src/Daqifi.Core/Device/SdCard/SdCardLogEntry.cs[19-24]

Agent prompt

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

## Issue description
`SdCardLogEntry` is a public positional record. Adding a new primary-constructor parameter changes the emitted `.ctor(...)` and `Deconstruct(...)` method signatures, breaking binary compatibility for consumers compiled against the previous 4-parameter shape.

## Issue Context
This is especially risky for apps/plugins that update the library without recompiling.

## Fix Focus Areas
- src/Daqifi.Core/Device/SdCard/SdCardLogEntry.cs[19-24]

### Implementation options (pick one)
1) **Preferred (non-breaking):** Keep the original primary constructor shape and add `HasDeviceTimestamp` as an `init` property with default `true`. Update parser call sites to set it via object initializer.

2) **ABI-compat shim:** Keep the new primary constructor, but add back:
  - an explicit 4-parameter constructor matching the old signature that chains to the 5-parameter one
  - an explicit `Deconstruct(out DateTime, out IReadOnlyList<double>, out uint, out IReadOnlyList<uint>?)` overload

Either approach should restore binary compatibility for existing compiled consumers.

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

---

2. CSV missing-timestamp doc mismatch ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

The HasDeviceTimestamp XML docs cite “missing CSV timestamp column” as a case where an entry is
emitted with a substituted base time, but the CSV parser currently rejects rows that don’t match the
required interleaved ts,val pair format (returning null and skipping the line), so such entries
are not emitted to be flagged.

Code

src/Daqifi.Core/Device/SdCard/SdCardLogEntry.cs[R13-17]

+/// <param name="HasDeviceTimestamp">
+/// <c>true</c> when <see cref="Timestamp"/> was reconstructed from a real device tick for this
+/// entry; <c>false</c> when no usable device timestamp was available (e.g. a zero message
+/// timestamp, a missing CSV timestamp column, or an unknown tick rate) and the session's base
+/// time was substituted instead.

Relevance

⭐⭐⭐ High

Team often fixes doc/behavior drift; similar doc mismatch corrections accepted (PR #160, #288).

PR-#160
PR-#288

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The docs explicitly mention missing CSV timestamp columns as a HasDeviceTimestamp=false scenario,
but the CSV parser skips malformed rows when parsing fails and requires successful uint parsing
for the timestamp field in each analog pair.

src/Daqifi.Core/Device/SdCard/SdCardLogEntry.cs[13-17]
src/Daqifi.Core/Device/SdCard/SdCardCsvFileParser.cs[279-284]
src/Daqifi.Core/Device/SdCard/SdCardCsvFileParser.cs[334-363]

Agent prompt

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

## Issue description
`SdCardLogEntry.HasDeviceTimestamp` documentation mentions “missing CSV timestamp column” as a scenario that yields `HasDeviceTimestamp=false` entries. In the current CSV parser, rows without the expected `ts,val` pairs (or with unparseable `ts`) are treated as malformed and skipped, so the documented scenario does not produce an entry.

## Issue Context
This is primarily a documentation/semantics mismatch that could mislead consumers about when they will see `HasDeviceTimestamp=false`.

## Fix Focus Areas
- src/Daqifi.Core/Device/SdCard/SdCardLogEntry.cs[13-17]
- src/Daqifi.Core/Device/SdCard/SdCardCsvFileParser.cs[279-284]
- src/Daqifi.Core/Device/SdCard/SdCardCsvFileParser.cs[334-363]

## Suggested fix
Either:
- Update the XML docs to remove or rephrase the “missing CSV timestamp column” example to match actual parser behavior (e.g., note that malformed rows are skipped),

or (if desired behavior is to keep samples even without timestamps):
- Extend the CSV parser to accept value-only rows, emit entries using `baseTime`, and set `HasDeviceTimestamp=false` for those emitted entries.

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

---