Repository navigation
fix(ecg): let a probe run reach the frame that carries the result - #2746
Merged
Merged
Conversation
The MG ECG probe could not observe a completed reading on any strap or firmware, and its report looked correct while it happened. @meta1971's field timing on #891 is the evidence: across five MG sessions the terminal frame (classifier state 2, progress 100) landed 38 to 39 s after the first live frame, against a 30 s listen window held independently by both platforms. The verdict sheet appearing at 30 s made it worse than a missed deadline, since Stop is the next control a person reaches for. Second defect, same report: the candidate cap kept the FIRST twelve frames and R17 arrives about once a second, so the retained evidence was seconds 1 to 12 while result, average HR and the reason mask populate only on the terminal frame. The packet count stayed honest; the retained evidence did not. Whoop5EcgProbe now owns the window, the caps and the two decisions, with a declared twin on each side, so the numbers cannot drift apart again. A capture run listens 60 s, wrist-select and stop keep 30 s, a terminal frame is retained past the cap within a bounded reserve, and it re-arms the run for 10 s more rather than ending it: the variability field reads 0xffff for 2 to 9 s after that frame, so stopping on it would pin the unset value as the reading's own. A terminal frame also now ends a run EARLY, so the report quotes the time listened rather than the window asked for. The client wiring is BLE code and cannot be covered without a strap. The rules it calls are pure and pinned on both platforms, including the window against the measured 39 s, and a mutation back to the old behaviour kills the two cases that name the two defects on both sides. Tools/parity_twin_map.json carries the derived counts for the four new declarations (functions 4534 to 4538) so the authority keeps reproducing. Refs #891
Found re-reviewing the window change, which is what created it. A stop re-arms the probe run, and re-arming cleared the candidate lines and the packet count along with the steps. At a 30 s window the verdict sheet had almost always rendered first, so little was lost. At 60 s it has not: the reading completes at 38 to 39 s while the sheet still reads "waiting", so the obvious next action is to stop a session the user can see has finished, and that discarded every R17 line including the terminal frame this change exists to retain. So a stop issued while the capture's window is still open carries the lines, the count and the run's START TIME. The start time travels with them because the report states "packets seen in Ns", and a count from a 47 s capture under a 30 s stop window would attribute those packets to a span that did not collect them. Only while the window is open. Once the verdict has rendered the lines are already out, and a later stop is a separate run. A fresh capture still clears, or a previous reading's frames would surface as the new one's, and wrist select still clears because its report is about a different command. Refs #891
Second re-review pass, on my own first pass. `ecgProbeDeadline` is a MARKER: `ecgProbeArmed` tests it for nil and nothing in the file compares the stamp to the clock. So stamping it with `now + window` encoded the window a second time, where only the verdict timer's delay can act on it, and `ecgStartCapture` had to pass the same number twice for the code to read correctly. The window now lives in `scheduleEcgProbeVerdict` alone, which also renews the marker, and `beginEcgProbeRun` goes back to arming the run. Arming stays there rather than moving into the scheduler because the sends in between can draw a COMMAND_RESPONSE and the triage is gated on it. This is the shape Kotlin already had, where `ecgProbeListening` is a plain flag and the window exists only in the posted delay, so the two clients now make the same four window decisions in the same four places. Refs #891
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
1 participant
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.
The MG ECG probe could not observe a completed reading on any strap or any firmware, and its report
looked correct while it happened. Two defects, both on both platforms, both found by @meta1971's field
timing on #891.
The window ended before the strap answered
30 s on both platforms, held independently. Across five MG sessions the terminal frame (classifier
state 2, progress 100) landed 38 to 39 s after the first live frame. @DX23876's 12.0.0 run on 50.41.1.0
hit the same wall from the other side: 30 R17 packets decoded, progress reached 10, no terminal packet.
The verdict sheet appearing at 30 s made it worse than a missed deadline. Stop is the next control a
person reaches for, so the window trained people to end the session 8 to 9 s before the classifier
would have answered, and the probe then reported the silence it had caused.
The cap kept the wrong twelve frames
R17 arrives about once a second and the cap kept the FIRST twelve, so the retained evidence was seconds
1 to 12. Result, average HR and the reason mask populate only on the terminal frame, so the one frame
carrying the answer was discarded by construction.
ecgProbePacketsSeenincrements before the cap, sothe count stayed honest, which is why this was invisible in the report.
What changed
Whoop5EcgProbenow owns the window, the caps and the two decisions, with a declared twin on eachside. That they both held 30 independently is how the window outlived the evidence against it.
Window.capture = 60,Window.selectOrStop = 30,Window.terminalGrace = 10.COMMAND_RESPONSE rather than on a reading.
retainsCandidatekeeps a terminal frame past the cap, inside a bounded four-slot reserve, so theceiling is 16 lines rather than an exemption.
terminalGraceSecondsre-arms the run for 10 s on the first terminal frame instead of ending on it:the variability field reads
0xfffffor 2 to 9 s after that frame, so stopping there would pin theunset value as the reading's own. Once per run, or a terminal stream would postpone the verdict
forever. Implemented by bumping the run token, which strands the pending verdict timer, the same
mechanism a second user tap already used.
verdict text embeds that number ("no ECG packet arrived in Ns"), and quoting the request would have
the report claim a window it did not run.
Verification
Whoop5EcgProbeWindowTests4/4 (Swift, fresh.build),Whoop5EcgProbeWindowTest4/4 (Kotlin),alongside
Whoop5EcgProbeTest20/20 andWhoop5EcgTest45/45.cases that name the two defects, and leaves the other two green since neither depends on the mutated
rules.
parity_ledger.pyOK, twin references 1093 to 1097, all resolved.doc_comment_lint.pyOK, no new detached doc comments.tests.test_parity_governance_acceptance50/50 after refreshing the derived authority(
functions4534 to 4538, and the pair and unpaired sets that follow from it). Every delta in thatfile is this change: 4 functions, 10 constants, 2 function pairs, 5 constant pairs, and the two
Whoop5EcgProbefiles leaving the unpaired set.Not covered
The client wiring is BLE code, so neither suite reaches it: the rules are pure and pinned, the calls
into them are not. This needs a run on an MG to confirm the longer window and the grace extension
behave on a real strap, which I cannot do from here.