chore: retire the four daqifi-core#398 bandaids now that Core v1.4.0 owns them - #808
Conversation
PR Summary by QodoRetire Core#398 desktop workarounds now covered by Core v1.4.0
AI Description
Diagram
High-Level Assessment
Files changed (13)
|
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}
b0b442c to
1fe0e7b
Compare
Rebased onto main — #806 has merged#806 squash-merged as The note above is now stale and can be ignored — it has been executed. Effect on this PR's diff: it no longer re-displays the already-merged bump. Reviewers see only this change. Unit gate re-run after the rebase, not just before it. |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 60d65af |
Status: ready for human review
On Qodo's "fallback warning not per-device" findingThe conclusion was right and the fix shipped, but the mechanism in the suggestion was wrong, and the difference mattered. Recording it because the wrong version is superficially convincing. The suggestion said discovery "overwrites My first regression test modelled the suggested mechanism literally, mutating The real vector is the same instance reuse one step later: Outstanding: needs a human at the benchNot covered by the unit gate:
Note when testing SD: firmware#703 (SD read buffer allocation failing under low heap — LIST works, GET returns marker-only 0 bytes) is a firmware-side failure that presents as a desktop bug. Rule it out before attributing a failure here. |
…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>
60d65af to
95bdafc
Compare
📊 Code Coverage ReportSummarySummary
CoverageDAQiFi - 57.6%
Daqifi.Desktop.Common - 46%
Coverage report generated by ReportGenerator • View full report in build artifacts |
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. |
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. |
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. |
2 participants
Closes #803
Stacked on #806 (
chore/core-1.4.0-bump). Base is that branch, notmain.daqifi-core#398 shipped in Core v1.4.0, so the four desktop workarounds that stood in for it (#776, #779, #780, #782) can be deleted. Every claim below was verified by reading the Core source at
v1.4.0, not the XML docs or the ticket text — two of the ticket's claims turned out to be wrong (see "Where the issue was wrong").1. Timestamp reset — daqifi-core#398 gap 3
Deleted:
_timestampProcessorSync,_timestampFrequencyApplied, the ~30 lines of doc explaining why the lock was load-bearing, andTimestampProcessorSerializationTestsin full.Why it's safe.
src/Daqifi.Core/Device/TimestampProcessor.csatv1.4.0:_deviceTickPeriodsis untouched. The desktop's two-part invariant — "the gate and the frequency it stands for have to change together" — no longer exists, because there is no desktop-side gate:ITimestampProcessor.HasTimestampFrequency(deviceId)(new in v1.4.0, confirmedpublicon the interface) is now the record of whether the apply happened, and it lives inside the same concurrent dictionary the apply writes.TimestampProcessorwas already thread-safe on its own.Lock audit (judgment call 1).
_timestampProcessorSynchad exactly three uses, all of them the apply/gate/reset interleaving:ProcessStreamMessageProcessTimestampInitializeStreamingResetAll()+ gate clearStopStreamingResetAll()+ gate clearNothing else had crept under it, so it is removed entirely rather than narrowed.
Bonus taken in the same PR.
TimestampResult.UsedFallbackTickPeriodis new in v1.4.0 and is set from the same dictionary read that chooses the tick period, so it cannot disagree with what was actually used. The failure mode behind #782 — a 42 MHz device silently reconstructed against the 50 MHz default, scaling every timestamp by ~1.19 with no error — is now logged. Warning, not Error, and once per device: it is recoverable device configuration, Error is the Sentry path, and a per-frame log at streaming rate would bury everything else.2. Firmware classifier — daqifi-core#398 gap 4
Deleted: the
FirmwareFlashPhaseenum, the phase parameter on both classifier methods, and the phase variable threaded throughFirmwareUpdateCoordinatorandDaqifiViewModel.Why it's safe.
FirmwareUpdateState.ReconnectingAfterFlash = 12is new in v1.4.0.git grep ReconnectingAfterFlash v1.4.0 -- src/Daqifi.Core/returns exactly one emitting site —WifiModuleUpdater.cs:133, immediately after the WINC success-marker check and wrappingWaitForSerialReconnectAsync+ the LAN restore.Pic32FirmwareUpdater.csstill transitions toJumpingToAppafter the CRC pass and never entersReconnectingAfterFlash. The two states are therefore disjoint across the two flows, which is what makes the caller-supplied phase unnecessary.Core also fixed its own
BuildRecoveryGuidancein the process:ReconnectingAfterFlashgets "the firmware was flashed and verified successfully…" instead of the PIC32 CRC text a successful WiFi flash used to be handed.Every
switchoverFirmwareUpdateState(judgment call 4). There were none in the desktop other than the classifier's ownswitchoverFirmwareFlashPhase(now deleted). Rather than rely on that staying true, the classifier tests now carry an exhaustiveness guard that comparesEnum.GetValues<FirmwareUpdateState>()against the union of the downgraded and not-downgraded lists — a future Core state fails the suite instead of being silently defaulted.3. SD transfer stalls — daqifi-core#398 gap 1
Deleted:
Daqifi.Desktop/Loggers/SdCardDownloadStalledException.csin full.Why it's safe.
SdCardTransferStalledException(v1.4.0) carriesReason,BytesReceived,FileNameandTimeout— strictly more than the binaryIsProlongedFailureit replaces. The classifier now keys onReason, and the mapping preserves the old behaviour exactly:TransferTimeoutIsProlongedFailure == trueTransportClosedNoDataReceivedIsProlongedFailure == falseOver USB serial — the only transport SD import supports — a wedged device produces
NoDataReceivedin about half a second, which stays per-file. That is the #779 case, and it stays off the Error/Sentry arm.The Sentry arm (judgment call 2). Deleting the type without care would have reopened #779 in two different ways, and both are guarded:
SdCardTransferStalledExceptionderives fromSdCardOperationException, so the new arm is placed before the genericSdCardOperationExceptionarm. Landing on that arm would have silently turned a device-wide stall into a per-file one with "the card may be corrupt" advice. There is a dedicated regression test for the arm order.TimeoutExceptionfromSdCardOperations.RunWithHardDeadlineAsyncwhen it abandons a parked worker. Unnormalized that reaches the classifier's default arm — a Sentry issue plus "check the device connection", exactly what bug: SD-download stall watchdog is dead code over USB serial — Core's plain TimeoutException hits the classifier's default (Sentry) arm #779 removed. The scopedcatch (TimeoutException)at the download call site therefore stays; it now rethrows as Core'sSdCardTransferStalledException(TransferTimeout)instead of ours. A bareTimeoutExceptionreaching the classifier from anywhere else still keeps the Error path, and that test is unchanged.Assertions are on log level, not "did not throw":
ImportFile_TransportDetectedStall_LogsWarningNotErrorand the newImportFile_TransportClosedStall_LogsWarningNotErrorboth verifyWarningonce andErrornever.Watchdog kept, deliberately. Core does bound the download now (daqifi-core#399/#401), but
DaqifiStreamingDevice.SdCardDownloadTimeoutisinternal virtualat 30 minutes and not settable from here. Shrinking the desktop's 90-second bound to 30 minutes would regress the #754 busy-overlay symptom, so the watchdog stays; only the exception type it raises changed. Worth a follow-up once Core exposes the knob.4. Empty transfers — daqifi-core#398 gap 2
Deleted: the importer's
fileInfo.Length == 0throw.Why it's safe.
SdCardFileReceiver.ReceiveAsyncnow takeslistedFileSizeBytesand only raisesSdCardEmptyTransferExceptionwhentotalBytesReceived == 0 && listedFileSizeBytes != 0— so a file the listing reports as 0 bytes returns a legitimate empty download.SdCardOperations.TryGetListedFileSizesources that from the lastGetSdCardFilesAsynclisting, and the desktop'sRefreshSdCardFilesgoes through exactly that Core call, so the discrimination is genuinely wired up here rather than just theoretically available. A 0-byte log now imports as an empty session and logs a Warning naming the file.Batch continuation (judgment call 3). #780 is preserved and slightly improved: the
SdCardEmptyTransferExceptionarm is stillIsCardUnavailable: false(Core's unknown-listed-size case keeps its conservative throw, so it can still be one file), and the common empty-log case no longer fails at all, so it does not even consume a "skipped" slot.ImportAllFiles_WhenTheFirstFileIsEmpty_StillImportsEveryHealthyFileAfterItandImportAllFiles_WhenTheTransportTimesOut_KeepsGoingAndStaysOffTheErrorPathare unchanged and green.The separate
string.IsNullOrEmpty(FilePath)guard is kept but reclassified: Core's temp-file overload always returnsresult with { FilePath = tempPath }, so an empty path can only be a brokenIStreamingDevice. It now throwsInvalidOperationExceptionand keeps the Error/Sentry path, where a contract violation belongs — it is no longer reported to the user as a wedged SD subsystem.Where the issue was wrong
Both found by reading Core at the tag:
"the classifier reduces to keying on
FailedState == ReconnectingAfterFlash". No — PIC32's benign post-flash state is stillJumpingToApp, which Core did not touch. Both arms remain. What the split actually bought is that the two states are disjoint across the two flows, which is what removes the phase parameter, not one of the arms. Dropping theJumpingToApparm would have regressed Daqifi.Core.Firmware.FirmwareUpdateException: Firmware update failed in state 'JumpingToApp' while Jumping to application firmware.. #738."the download is bounded now, so a wedged card can no longer hang the call" — true, but the bound is enforced by throwing a bare
TimeoutExceptionfromRunWithHardDeadlineAsync. Thecatch (TimeoutException)normalization the issue lists for deletion is still load-bearing; deleting it puts a wedged card back on the Sentry arm.Verification
dotnet build -tl:on— clean, zero warnings.dotnet test --filter "TestCategory!=Ui&FullyQualifiedName!~WindowsFirewallWrapperTests" -tl:on— 868 passed, 0 failed (parent branch baseline: 871).TimestampProcessorSerializationTests(tested the deleted lock), +4StreamTimestampReconstructionTests(assert the user-visible outcomes that survive).FirmwareFailureClassifierTestsphase-crossproduct rows, replaced by one exhaustiveness guard over the whole enum.SilentFor/IsProlongedFailureon the deleted type; +1TransportClosedcase, +1 arm-order regression guard, +1 listed-0-byte import test.Rebase note
When #806 squash-merges, rebase this branch with:
rather than merging
main— the squash creates a duplicate of the pre-squash commit, and merging resolves it into a conflicted mess by hand.🤖 Generated with Claude Code