fix(sd): use the device's own timestamp clock when parsing SD logs (closes #426)

[tylerkron]

Why

Every timestamp reconstructed from an SD card log was about 19% fast. SdCardDeviceConfiguration.FromDevice was handed a live, initialized device and threw away the one thing it was uniquely able to supply — the device's real timestamp clock — so parsing fell back to 50 MHz while the hardware counts at 42 MHz. Nothing warned and the exit code was 0: a recording that really spanned 6.2 seconds simply reported 5.2, and --sd-export-csv baked the error into the Time column for anything downstream.

What

Parse an SD log with the device connected and the timestamps now come out at the rate the data was actually logged at — 50.0 ms apart for a 20 Hz log instead of 42.0 ms. When neither the log file nor a connected device can supply a frequency, the parser still falls back to a guess, but the session now reports which frequency it used and where that number came from, so a wrong answer can no longer be mistaken for a right one.

How

FromDevice now passes device.TimestampFrequency instead of a hardcoded 0, keeping 0 only when the device genuinely reported none. A log that states its own frequency still wins, so this is a backstop rather than an override of better data. SdCardLogSession gained two additive properties — TimestampFrequency and TimestampFrequencySource — that name the figure used and whether it came from the file, the device, or a fallback guess, mirroring the existing ITimestampProcessor.HasTimestampFrequency idea.

Second bug fixed here, worth calling out on its own: the CSV and JSON parsers seeded the fallback frequency before merging the device override, so a connected device's real clock could never win — it was always beaten by the 50 MHz guess. That made fix #1 inert for those two formats. All three parsers now share one precedence: file, then device, then fallback. One existing JSON test asserted the old behaviour explicitly ("Inferred frequency takes precedence"); that assertion is deliberately inverted in this PR, since it codified the bug.

Bench test

Bench Nq1, FW 3.7.2, parsing log_20260802_084946.bin — the exact file from the issue, 125 samples logged at 20 Hz (50.0 ms expected).

USB / serial, before (Core main):

Timestamp freq:   0 Hz
first: [18:47:07.727]   last: [18:47:12.935]   125 samples
=> 5.208 s over 124 intervals = 42.00 ms

USB / serial, after (this branch):

Timestamp freq:   42000000 Hz
first: [18:47:15.470]   last: [18:47:21.670]   125 samples
=> 6.200 s over 124 intervals = 50.00 ms

WiFi / TCP 192.168.1.30:9760, after (one consolidated run; SD-over-TCP works since #327):

Timestamp freq:   42000000 Hz
first: [18:48:02.786]   last: [18:48:08.936]
=> 6.150 s over 123 intervals = 50.00 ms

Supporting measurements from the same unit: the live status message reports TimestampFreq = 42,000,000 Hz, and raw stream tick deltas were 4,200,000 @ 10 Hz, 2,100,000 @ 20 Hz and 840,000 @ 50 Hz — each one exactly 42 MHz divided by the requested rate.

Full Core suite green: 2483 passed, 0 failed, on both net9.0 and net10.0, with 14 new tests.

Not covered by this bench run

The fresh --sd-log-start → --sd-list → --sd-download repro cycle did not complete. Once SD logging stopped, every subsequent serial connect failed with did not report its channel configuration within 8s — twice, including after a 25 second settle — on a freshly power-cycled unit, while TCP to the same device kept working normally throughout. This looks like a previously unrecorded device behaviour (writing an SD log leaves the serial interface unable to complete init for a while) and is distinct from the known low-heap empty-transfer state, which presents as an empty marker-only download rather than a failed init. It is not caused by this change: the same binary downloaded and parsed successfully seconds earlier over the same port. The before/after evidence above does not depend on that cycle, since it uses the exact log file from the issue.

Two things the reviewer should decide on

1. The issue's claim that this is unrelated to firmware #716 is wrong, and this PR does not fix the residual error. Timing the live stream against the host clock shows the counter advances at ~33.34 MHz in real time, not 42 MHz — and 33.34/42.0 = 0.794, exactly firmware #716's "streams at 79.4% of the requested rate". The tick deltas were perfectly uniform with no dropped samples or buffered backlog, at rates far below any bandwidth limit, so this is not a transport artifact: the device's declared timebase and its slow sample cadence are one and the same firmware defect. Practically, after this PR an SD timestamp matches the device's declared timebase and the nominal logged rate, but is still about 26% short of true wall-clock elapsed time until firmware corrects its clock constant. I deliberately did not bake 33.34 MHz into Core — trusting the device's declared frequency is the right contract, and a compensation constant would silently become wrong the moment firmware is fixed.

2. TimestampProcessor.DefaultTimestampFrequency is left at 50 MHz. Changing it to 42 MHz would retime data for every live-streaming consumer, not just the SD path, and it is only ever reached when a device reports no frequency at all. I corrected its documentation to say plainly that it matches no shipped board rather than swapping the value, and made the SD fallback observable instead. Happy to change the constant if you'd rather take that behaviour change.

Closes #426

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Fix SD log timestamp conversion by using device-reported clock frequency

🐞 Bug fix 🧪 Tests ✨ Enhancement 🕐 40+ Minutes

AI Description

• Use connected device’s timestamp frequency when SD logs omit frequency metadata.
• Unify precedence for timestamp frequency: file, then device override, then fallback guess.
• Surface chosen frequency and its source on the parsed session to avoid silent mis-scaling.

Diagram

graph TD
  A["Caller / CLI"] --> B["SD log parser"] --> C[["Timestamp freq resolver"]] --> D["SdCardLogSession"]
  E["SD log file"] --> B --> C
  F(("Connected device")) --> G["Config override"] --> C
  H(("Fallback option")) --> C
  subgraph Legend
    direction LR
    _proc["Process"] ~~~ _sub[["Resolver"]] ~~~ _io["SD log file"] ~~~ _opt(("Option / device"))
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Change fallback default to 42 MHz

• ➕ Would reduce error for offline parsing when no device is available
• ➕ Simpler than adding source reporting
• ➖ Bakes in a hardware-specific assumption that may be wrong for other devices/firmware
• ➖ Still silent when wrong; doesn’t address correctness/traceability

2. Fail/return error when frequency is unknown (no fallback)

• ➕ Prevents silently producing incorrect timestamps
• ➕ Forces callers to supply device override or explicit frequency
• ➖ Breaking behavior for existing offline workflows that relied on best-effort timestamps
• ➖ Requires downstream handling changes (CLI exit codes, UX decisions)

3. Infer frequency heuristically from sample timing patterns

• ➕ Could improve offline accuracy without a connected device
• ➕ May work for fixed-rate logs with clear cadence
• ➖ Unreliable across variable-rate logs, dropouts, or mixed message types
• ➖ Adds complexity and risk of confidently-wrong inference

Recommendation: Current approach is the best tradeoff: keep backward-compatible best-effort parsing (fallback), but fix precedence so device-reported clocks actually apply, and make the chosen frequency + source explicit on SdCardLogSession so downstream tooling can warn or reject fallback-derived timestamps. Consider (separately) whether the default fallback should change or be disabled in CLI workflows, but that’s a product/UX decision beyond this fix.

Files changed (11) +497 / -31

Enhancement (2) +73 / -0

SdCardLogSession.csExpose resolved timestamp frequency and its source on SdCardLogSession +21/-0

Expose resolved timestamp frequency and its source on SdCardLogSession

• Adds TimestampFrequency and TimestampFrequencySource init-only properties with documentation warning about constant-factor timestamp scaling when frequency is wrong. Enables callers to detect when timestamps were derived from a fallback guess or not converted at all.

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

SdCardTimestampSource.csAdd enum describing where timestamp frequency came from +52/-0

Add enum describing where timestamp frequency came from

• Adds SdCardTimestampSource with values None, LogFile, Device, and Fallback, with detailed remarks about the constant-factor nature of timestamp errors. Used by SdCardLogSession to make fallback-derived timestamps detectable.

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

Bug fix (4) +66 / -17

SdCardCsvFileParser.csMake CSV parsing resolve timestamp frequency after merges and report its source +22/-3

Make CSV parsing resolve timestamp frequency after merges and report its source

• Stops seeding TimestampFrequency from fallback during header parsing; keeps header frequency file-only. Resolves the final frequency using the shared resolver (file, device override, fallback) and sets both TimestampFrequency and TimestampFrequencySource on the returned SdCardLogSession.

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

SdCardDeviceConfiguration.csPropagate device TimestampFrequency in FromDevice snapshot +11/-2

Propagate device TimestampFrequency in FromDevice snapshot

• Updates documentation to clarify why device timestamp frequency is needed for older firmware logs. Changes FromDevice to populate TimestampFrequency from device.TimestampFrequency instead of hardcoding 0, preserving 0 only when the device reported none.

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

SdCardFileParser.csUnify BIN parser timestamp frequency precedence and session reporting +15/-8

Unify BIN parser timestamp frequency precedence and session reporting

• Captures file-embedded frequency before merging overrides so file/device sources remain distinguishable. Replaces ad-hoc fallback logic with SdCardTimestampFrequencyResolver and returns SdCardLogSession annotated with TimestampFrequency and TimestampFrequencySource.

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

SdCardJsonFileParser.csResolve JSON timestamp frequency via shared resolver and report provenance +18/-4

Resolve JSON timestamp frequency via shared resolver and report provenance

• Removes early fallback seeding during inferred configuration. Resolves the effective frequency using device override and fallback (JSON contributes none) and annotates both the config and SdCardLogSession with the chosen frequency and source.

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

Refactor (1) +49 / -0

SdCardTimestampFrequencyResolver.csAdd shared resolver for frequency selection and provenance tracking +49/-0

Add shared resolver for frequency selection and provenance tracking

• Introduces a single internal resolver that chooses the effective timestamp frequency using consistent precedence across formats. Returns both the frequency and an SdCardTimestampSource to record provenance, including a None case when fallback is disabled.

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

Tests (2) +283 / -4

SdCardJsonFileParserTests.csUpdate JSON parser test to prefer device frequency over fallback +8/-4

Update JSON parser test to prefer device frequency over fallback

• Adjusts the configuration override test to assert that a connected device’s TimestampFrequency beats the fallback guess for JSON logs. Adds assertions for the new session-level TimestampFrequency and TimestampFrequencySource reporting.

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

SdCardTimestampFrequencyTests.csAdd end-to-end tests for timestamp frequency resolution and scaling regression +275/-0

Add end-to-end tests for timestamp frequency resolution and scaling regression

• Introduces a dedicated test suite covering SdCardDeviceConfiguration.FromDevice propagation, resolver precedence (file → device → fallback → none), and parser reporting of frequency/source. Includes regression tests demonstrating the 20 Hz spacing fix with a 42 MHz device clock and the residual offline limitation when only fallback is available.

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

Documentation (2) +26 / -10

SdCardParseOptions.csDocument timestamp frequency precedence and risks of fallback default +14/-6

Document timestamp frequency precedence and risks of fallback default

• Clarifies precedence order (file → device override → fallback) and explicitly documents the historical nature of the 50 MHz default vs device-reported 42 MHz clocks. Points callers to ConfigurationOverride and session source reporting to avoid silently trusting guessed timestamps.

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

TimestampProcessor.csClarify TimestampProcessor 50 MHz default as historical and potentially wrong +12/-4

Clarify TimestampProcessor 50 MHz default as historical and potentially wrong

• Expands documentation on DefaultTickPeriod/DefaultTimestampFrequency to discourage relying on 50 MHz defaults when device-reported frequencies are available. Reinforces using SetTimestampFrequency and checking HasTimestampFrequency to avoid systematic timestamp scaling errors.

src/Daqifi.Core/Device/TimestampProcessor.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

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