chore: delete the stream-start leftover-frame guard now that Core owns it - #810
Open
tylerkron wants to merge 4 commits into
Open
chore: delete the stream-start leftover-frame guard now that Core owns it#810tylerkron wants to merge 4 commits into
tylerkron wants to merge 4 commits into
Conversation
Contributor
PR Summary by QodoRemove desktop leftover-frame guard; rely on Core StreamFrameGate diagnostics
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Contributor
Code Review by Qodo
Context used✅ Compliance rules (platform):
49 rules 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
tylerkron
force-pushed
the
chore/delete-leftover-frame-guard
branch
from
August 3, 2026 18:11
d148279 to
fb5b4aa
Compare
…owns them daqifi-core#398 shipped in Core v1.4.0, so the four desktop workarounds that stood in for it (#776, #779, #780, #782) can go. 1. Timestamp reset (gap 3). Core's ResetAll() now clears session baselines only and keeps per-device frequencies, so the desktop's _timestampFrequencyApplied gate and the _timestampProcessorSync lock that kept the gate and the frequency changing together are both gone. The apply is now once per device, keyed on Core's own HasTimestampFrequency. Bonus from the same Core release: TimestampResult.UsedFallbackTickPeriod makes the previously silent 50 MHz fallback observable, so a device that reports no clock now logs one Warning per device instead of nothing at all. 2. Firmware classifier (gap 4). Core added FirmwareUpdateState.ReconnectingAfterFlash, emitted only by WifiModuleUpdater after the WINC success marker. Verifying is now unambiguously the PIC32 flash CRC check, so the FirmwareFlashPhase enum and the phase parameter threaded through the coordinator and DaqifiViewModel are deleted and the classifier keys on FailedState alone. Note the issue text is wrong that this "reduces to keying on ReconnectingAfterFlash": PIC32's benign post-flash state is still JumpingToApp, which Core did not change. Both arms remain; what the split bought is that they are disjoint across the two flows, which is what makes the phase parameter unnecessary. 3. SD transfer stalls (gap 1). SdCardDownloadStalledException is deleted and the classifier keys on Core's SdCardTransferStalledException.Reason: TransferTimeout and TransportClosed stop a batch, NoDataReceived does not (the old IsProlongedFailure binary, with strictly more information). Core still throws a bare TimeoutException from its own hard download deadline, so the scoped normalization at the download call site stays - it now produces Core's type instead of ours. Deleting it outright would have put a wedged card back on the Sentry arm, which is the whole point of #779. 4. Empty transfers (gap 2). Core discriminates a wedged SD subsystem from a legitimately 0-byte file using the directory listing's reported size, so the importer's own "0 bytes on disk" throw is deleted - a listed 0-byte file now imports as an empty session rather than failing and burning a slot in an Import All batch (#780). The separate null-FilePath guard is kept but reclassified as a contract violation (Error/Sentry), because Core's temp-file overload always sets FilePath. Tests: TimestampProcessorSerializationTests is deleted along with the lock it tested, and replaced by StreamTimestampReconstructionTests, which asserts the user-visible outcomes that survive (a stop/start leaves the session on the device clock; a device with no reported clock warns once and never at Error). The SD and firmware classifier suites keep their log-level assertions. Closes #803 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
WarnOnceAboutFallbackTickPeriod takes a deviceId but suppressed on a wrapper-wide bool, so the signature promised per-device and the implementation delivered per-wrapper. That gap is reachable: discovery reuses the SerialStreamingDevice already registered for a COM port rather than constructing a new one, so swapping units on that port leaves one instance decoding a different device's frames. The second unit would lose the diagnostic entirely. Key on the frame's own serial via Interlocked.Exchange, which makes the claim and the already-warned test a single atomic step so racing frames emit once. Also separates Act from Assert in the firmware classifier tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tylerkron
force-pushed
the
chore/retire-core-398-bandaids
branch
from
August 3, 2026 21:02
60d65af to
95bdafc
Compare
…s it Core v1.4.0 took over the host-side guard the desktop had been carrying since issue #573 (daqifi-core#425/#428, internal StreamFrameGate), so the desktop's copy is now dead weight sitting on the hot streaming path. Deleted from AbstractStreamingDevice: - constants STALE_FRAME_WINDOW_SECONDS / MAX_DISCARDED_LEFTOVER_FRAMES - fields _lastSeenDeviceTimestamp, _hasSeenDeviceTimestamp, _checkForLeftoverFrames, _discardedLeftoverFrameCount, _pendingFirstFrameValidation, _heldFirstFrame - methods TrackLastSeenDeviceTimestamp, IsLeftoverFrameFromPreviousSession, ValidateFirstFramesWithoutReference - the arming block in InitializeStreaming and both guard branches in OnStreamMessageReceived - StreamStartLeftoverFrameTests Core's version is strictly better: its detection window is 2.5 sample periods computed per session from the streaming frequency, where ours was a fixed 2.5 seconds - 100x wider than needed at 100 Hz, which is exactly the margin in which a genuine early frame could be misclassified as a leftover. Core also screens the raw-frame path, so a withheld frame no longer reaches StreamMessageReceived at all. In its place the desktop subscribes to Core's StreamFrameDiscarded and records each discard as a debug Sentry breadcrumb - the diagnostic signal _discardedLeftoverFrameCount used to provide. Deliberately not Error: discards are EXPECTED on affected firmware (one PartialAnalogFrame per stream start up to 3.7.2, one StaleLeftoverFrame per restart), and Error is the only path that captures to Sentry, so it would file an issue per streaming session for a device-side condition no app change can fix - the flood this repo has already hit three times (#775, #779, #801). The handler also closes _acceptChannelSamples. A withheld frame raises no StreamMessageReceived, so OnStreamMessageReceived cannot close the gate for it, yet Core still decodes a PartialAnalogFrame for its digital payload - which would otherwise reach desktop channels stamped with the previously accepted frame's timestamp. Covered by a test that fails when the line is removed. InitializeStreaming still assigns Core's StreamingFrequency before StartStreaming, which is now load-bearing beyond commanding the device's rate: StartStreaming is what arms the gate, and the window is sized from that value. Closes #679 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… tests The two SetActiveSample calls used DateTime.Now for a value the assertions never inspect - they check whether a sample reaches the wrapper at all, not when. A fixed stamp keeps the tests deterministic across environments. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tylerkron
force-pushed
the
chore/delete-leftover-frame-guard
branch
from
August 3, 2026 21:07
fb5b4aa to
2b84029
Compare
📊 Code Coverage ReportSummarySummary
CoverageDAQiFi - 57.5%
Daqifi.Desktop.Common - 46.9%
Coverage report generated by ReportGenerator • View full report in build artifacts |
Contributor
Author
Rebased onto main (post-#812), and a bench-status noteRebased after #812 merged, with the unit gate re-run after the rebase rather than before. All three PRs were already reported textually mergeable, but #808 and #810 modify the same file #812 touched, so "no conflict" was not sufficient reason to skip re-testing. Hardware gate: partially blocked, and NOT on anything in these PRsDevice-gated FlaUI run on the chain-1 tip: 19/23. Four failures, none attributable to this work:
The last two are device state, not code. Established by control, not assumption:
Since the identical code passed earlier and fails now, the variable is the bench, not the branch. A Core-level probe on the same port confirms the device is still alive at the transport layer — connects, initializes, firmware 3.7.2, reports This needs a physical power-cycle of the bench device, which I can't perform. I deliberately did not force re-enumeration via Once the device is power-cycled the device-gated suite should be re-run against these branches to close out the gate. Everything else — build, unit gate, Qodo — is green. |
Contributor
Author
Hardware gate resultRun with parallelism disabled (
The extra failure on #809 is not #809
Identical code passing earlier and failing now, plus Status
Everything else on all three: build clean, unit gates green, Qodo clean. |
Contributor
Author
Hardware gate: clearedRe-run on a freshly power-cycled device with The one outstanding question was
All three pass. The full-suite failure is a harness defect, not a branch one: a preceding test leaves its app running ( I was wrong twice on the way to this, and both are worth recording so the next person does not repeat them: I first attributed these failures to an SD-induced device wedge (excluding the SD tests made it worse), and then to device degradation across the session (a fresh device reproduced it identically). Neither held up. Final state, all three PRs
Every remaining device-gated failure is accounted for: the two SD imports are the known firmware SD-heap defect (firmware#703) on files dated June/July, and #809's third is the leaked-app harness issue above, disproved by isolated re-run. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Deletes the desktop's stream-start leftover-frame guard (issue #573) now that Core v1.4.0 owns it, and replaces it with a subscription to Core's
StreamFrameDiscardedfor diagnostics.Closes #679
Why this is no longer blocked on a product decision
The issue body says this is blocked on setting a supported firmware floor at >= 3.6.0, because the guard still protects users on older firmware, and records that Core would not take a host-side guard (daqifi-core#246, closed won't-implement). Core reversed that. daqifi-core#425/#428 shipped in v1.4.0 as the internal
StreamFrameGate, a faithful port of ours:StreamFrameGateSTALE_FRAME_WINDOW_SECONDS = 2.5LeftoverWindowSamplePeriods = 2.5MAX_DISCARDED_LEFTOVER_FRAMES = 5MaxDiscardedFrames = 5_lastSeenDeviceTimestamp/_hasSeenDeviceTimestamp_lastSeenDeviceTimestamp/_hasDeviceTimestampReferenceIsLeftoverFrameFromPreviousSessionIsLeftoverFromPreviousSessionInitializeStreamingBeginSession(timestampFrequency, streamingFrequencyHz)So users on firmware <= 3.5.0 keep the protection — with a tighter window — and no floor is needed.
Core's is the better implementation. Ours hardcoded a 2.5 second window. Core's is 2.5 sample periods, computed per session from the actual streaming frequency (
2.5 * ticksPerSecond / rate). At 100 Hz ours was 100x wider than it needed to be — exactly the margin in which a genuine early frame could be misclassified as a leftover. Core also screens the raw-frame path, so a withheld frame never reachesStreamMessageReceivedat all.What changed
Deleted (
AbstractStreamingDevice.cs, −164 lines against +86):STALE_FRAME_WINDOW_SECONDS,MAX_DISCARDED_LEFTOVER_FRAMES_lastSeenDeviceTimestamp,_hasSeenDeviceTimestamp,_checkForLeftoverFrames,_discardedLeftoverFrameCount,_pendingFirstFrameValidation,_heldFirstFrameTrackLastSeenDeviceTimestamp,IsLeftoverFrameFromPreviousSession,ValidateFirstFramesWithoutReferenceInitializeStreaming, both guard branches inOnStreamMessageReceivedDaqifi.Desktop.Test/Device/StreamStartLeftoverFrameTests.cs(373 lines)Added:
OnCoreStreamFrameDiscarded, wired into the existingSubscribeCoreDeviceEvents/UnsubscribeCoreDeviceEventspair, plusStreamFrameDiscardDiagnosticsTests.cs.Bench evidence
Not re-benched here (per the hardware coordination policy, and the issue's suggestion to verify against firmware <= 3.5.0 is explicitly declined — this bench hardware has known brick-recovery problems, firmware#568). It does not need to be: Core 1.4.0's gate already fired unprompted on this exact hardware (Nyquist on COM3) during PR #806's bench pass:
The second is precisely the #573 scenario this guard was written for, confirmed on real hardware. Core's own unit coverage plus the end-to-end tests below cover the rest.
Three judgment calls
1. Log level: debug breadcrumb, never Error. Discards are expected on affected firmware — one
PartialAnalogFrameper stream start up to 3.7.2, oneStaleLeftoverFrameper restart.AppLogger.Erroris the only path that captures to Sentry, so routing discards there would file an issue per streaming session for a device-side condition no app change can fix. This repo has hit that exact flood three separate times (#775, #779, #801). A breadcrumb keeps every discard on the timeline of any error that does get captured, which is where it is actually useful, andDiscardedFrame_IsRecordedAsBreadcrumb_AndNeverAsErroris the tripwire.2. Nothing outside the guard read the deleted state. Grepped every reader of all six fields before deleting.
_hasSeenDeviceTimestamp(which gated the arming decision) and_lastSeenDeviceTimestampwere read only byIsLeftoverFrameFromPreviousSession,ValidateFirstFramesWithoutReference, and theInitializeStreamingarming block — all deleted.TrackLastSeenDeviceTimestamp's call from the!IsStreamingearly-return had a real purpose (the device can emit a final frame after the stop lands, and the next session's latched frame follows that one); Core covers it —StreamFrameGate.TrackFrameruns for frames outside a session too.3. Core arms
BeginSessionitself; the desktop just must not reorder.DaqifiStreamingDevice.StartStreaming→BeginStreamingSession→_frameGate.BeginSession(TimestampFrequency, StreamingFrequency). The desktop needs no new call — butInitializeStreaming's existingcoreStreamingDevice.StreamingFrequency = StreamingFrequencyassignment beforeStartStreamingis now load-bearing beyond commanding the rate, since the window is sized from that value. Commented in place so it does not get moved.One thing the deletion also fixes
OnCoreStreamFrameDiscardedcloses_acceptChannelSamples. A frame Core withholds raises noStreamMessageReceived, soOnStreamMessageReceivednever runs for it and cannot close the gate — yet Core still decodes aPartialAnalogFramefor its digital payload. Without closing it, those samples reach desktop channels stamped with_currentFrameTimestampfrom whichever frame was accepted before them, possibly from the previous session, andDataSampleis what gets persisted.DiscardedFrame_ClosesTheChannelSampleGatewas mutation-checked: removing the single line makes it fail.Behavior difference worth naming
Core deliberately does not port
ValidateFirstFramesWithoutReference— the held-first-frame validation for the very first session after connect, where there is no counter reference. Core documents this as a known limitation and rejects the hold on the grounds that it delays every consumer's first sample to defend against a frame never observed on current firmware. Every session from the second onward — the stop/start case #573 actually describes — is covered. Accepted; the issue's plan calls for deleting that method.Testing
dotnet build -tl:on— clean, zero new warnings (the 6 remaining >120-char lines in the file are all pre-existing; the deletions removed 3 of 9).dotnet test --filter "TestCategory!=Ui&FullyQualifiedName!~WindowsFirewallWrapperTests" -tl:on— 864 passing, 0 failing. Baseline on the parent branch is 868; 868 − 10 deleted + 6 new = 864.The 6 new tests drive Core's real
StreamFrameGateend to end through the desktop wrapper rather than mocking the event, because the deletion is only safe if the protection genuinely survives it:UnsubscribeCoreDeviceEventsdetaches the discard handler with the rest (asserted against Core's ownDiscardedStreamFrameCount, so it cannot pass by simply not discarding)Docs touched:
docs/architecture.md's "one frame, one timestamp" note, and the stale comments inFrameTimestampSourceTests/StreamRestartTeststhat described the desktop-side hold.Stacked PR
Stacked on #808. #806 already merged; this branch is rebased onto it.
🤖 Generated with Claude Code