Repository navigation
fix(streaming): scale reconstructed timestamps by the device's own clock frequency (closes #36) - #37
Conversation
…ock frequency Ports the applicable half of upstream desktop #770. ProcessStreamMessage called _timestampProcessor.ProcessTimestamp but never SetTimestampFrequency, so Core's processor reconstructed the timeline against its 20 ns fallback tick (50 MHz) while current firmware runs the streaming timestamp timer at 42 MHz -- a ~16% compression that drifts without bound and lands in the DB (TimestampTicks). Apply the device's real TimestampFrequency to the processor once per session, guarded by a _timestampFrequencyApplied flag reset alongside both ResetAll() sites (InitializeStreaming + StopStreaming), which per Core's contract also drop per-device frequencies. SetTimestampFrequency has existed since Core 1.0.0, so this is independent of the Core 1.3.0 bump. #770's other half (coreSample.Timestamp -> _currentFrameTimestamp) does not apply: the port self-decodes frames and already stamps every channel sample with the single per-frame messageTimestamp (it never adopted Core's per-channel OnCoreChannelSampleReceived pipeline), so there is no divergence to fix. Desktop head builds 0 errors. Closes #36. Claude-Session: https://claude.ai/code/session_019fhAAGrHshwojdLJ99DUhc
PR Summary by QodoFix streaming timestamps by applying device clock frequency to TimestampProcessor
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo
1. Unsynced session frequency state
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| // Apply the device's real streaming-clock frequency to Core's processor once per session. | ||
| // Without it the processor reconstructs against its 20 ns fallback tick (50 MHz) while current | ||
| // firmware runs the timer at 42 MHz, compressing the reconstructed timeline ~16% and drifting | ||
| // without bound (each timestamp is the previous plus elapsed ticks). Applied here, where the | ||
| // device-id key is known to match the one ProcessTimestamp uses, and re-applied per session | ||
| // because InitializeStreaming's ResetAll() drops per-device frequencies with the baselines. | ||
| if (!_timestampFrequencyApplied && TimestampFrequency != 0) | ||
| { | ||
| _timestampProcessor.SetTimestampFrequency(deviceId, TimestampFrequency); | ||
| _timestampFrequencyApplied = true; | ||
| } |
There was a problem hiding this comment.
1. Unsynced session frequency state 🐞 Bug ☼ Reliability
_timestampFrequencyApplied and _timestampProcessor are accessed from the fire-and-forget inbound message pipeline while InitializeStreaming/StopStreaming reset them, with no shared lock/session token to establish ordering. This can race such that a new session observes a stale "applied" state (or sees ResetAll after applying), leaving the processor on fallback frequency for that session and reintroducing incorrect timestamp reconstruction.
Agent Prompt
### Issue description
`ProcessStreamMessage()` may run concurrently with `InitializeStreaming()` / `StopStreaming()` because inbound messages are routed via a fire-and-forget async handler. The new `_timestampFrequencyApplied` gate and `_timestampProcessor.ResetAll()` are not synchronized, so the frequency application can be skipped or mis-ordered relative to resets.
### Issue Context
- `OnInboundMessageReceived` calls `_protocolHandler.HandleAsync(...)` without awaiting.
- `ProcessStreamMessage` reads/writes `_timestampFrequencyApplied` and calls `_timestampProcessor.SetTimestampFrequency`/`ProcessTimestamp`.
- `InitializeStreaming` and `StopStreaming` call `_timestampProcessor.ResetAll()` and set `_timestampFrequencyApplied = false`.
### Fix Focus Areas
- Daqifi.Avalonia/Daqifi.Desktop/Device/AbstractStreamingDevice.cs[96-120]
- Daqifi.Avalonia/Daqifi.Desktop/Device/AbstractStreamingDevice.cs[528-543]
- Daqifi.Avalonia/Daqifi.Desktop/Device/AbstractStreamingDevice.cs[587-616]
- Daqifi.Avalonia/Daqifi.Desktop/Device/AbstractStreamingDevice.cs[1079-1158]
### Suggested fix
1. Introduce a private lock object (e.g., `_timestampSync = new object();`).
2. Wrap **all** uses/mutations of `_timestampProcessor` and `_timestampFrequencyApplied` under this lock:
- In `ProcessStreamMessage`: lock around the `if (!_timestampFrequencyApplied ...) { SetTimestampFrequency... }` block **and** the subsequent `ProcessTimestamp` call.
- In `InitializeStreaming` and `StopStreaming`: lock around `ResetAll()` and `_timestampFrequencyApplied = false`.
3. (Optional) If locking `ProcessTimestamp` is too contentious, use a session-generation counter instead (increment on ResetAll; store the generation used to apply frequency), but keep the ordering explicit and thread-safe.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
The flag is written by ProcessStreamMessage (transport thread) and reset by InitializeStreaming/StopStreaming (UI thread); volatile matches the class's other cross-thread streaming-state flags (_checkForLeftoverFrames, _pendingFirstFrameValidation) so the transport thread promptly observes the reset. Claude-Session: https://claude.ai/code/session_019fhAAGrHshwojdLJ99DUhc
|
Re: "Unsynced session frequency state" — addressed in
|
|
/agentic_review |
|
/improve |
PR Code Suggestions ✨Warning
|
|||||||||
|
Code review by qodo was updated up to the latest commit 82caeda |
What
Ports the applicable half of upstream desktop #770 (
b469ca1) — the streaming timeline was reconstructed against the wrong clock.Closes #36.
Bug
AbstractStreamingDevice.ProcessStreamMessagecalls_timestampProcessor.ProcessTimestamp(deviceId, ...)but never callsSetTimestampFrequency, so Core's processor reconstructs against its 20 ns fallback tick (50 MHz, the legacy clock) while current firmware runs the streaming timestamp timer at 42 MHz. The result is a ~16% timeline compression that drifts without bound (each timestamp is the previous plus elapsed ticks) and is persisted to the DB (TimestampTicks).Fix
Apply the device's real
TimestampFrequencyto the processor once per session, guarded by a new_timestampFrequencyAppliedflag reset alongside both_timestampProcessor.ResetAll()sites (InitializeStreaming + StopStreaming — Core drops per-device frequencies on reset). Applied inProcessStreamMessagewhere the device-id key is known to match the oneProcessTimestampuses.SetTimestampFrequency(string, uint)has existed in Core since 1.0.0 (#236), so this is independent of the Core 1.3.0 bump. The port's own uses ofTimestampFrequency(leftover-frame detection atIsLeftoverFrameFromPreviousSession/ValidateFirstFramesWithoutReference) are untouched — they already used the real frequency and operate on a separate path.Not ported (N/A)
#770's other half (
coreSample.Timestamp→_currentFrameTimestamp) does not apply: the port self-decodes frames inProcessStreamMessageand already stamps every channel sample with the single per-framemessageTimestamp(it never adopted Core's per-channelOnCoreChannelSampleReceivedpipeline), so there is nocoreSample.Timestampdivergence.Validation
Desktop head builds 0 errors. The streaming pipeline (discover → connect → stream 16ch@100Hz → plot) was validated live on-device as part of the Core 1.3.0 work; this change adds one guarded
SetTimestampFrequencycall on that same path. Part of the core/desktop → Avalonia sync sweep.