From d8132b198ae2e7e7ffcb465e23d267dc4de243a8 Mon Sep 17 00:00:00 2001 From: Fanboynz Date: Fri, 9 Oct 2026 20:28:15 +1300 Subject: [PATCH 1/3] fix(ecg): let a probe run reach the frame that carries the result 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 --- .../WhoopProtocol/Whoop5EcgProbe.swift | 62 +++++++++++ .../Whoop5EcgProbeWindowTests.swift | 57 ++++++++++ Strand/BLE/BLEManager.swift | 65 +++++++++--- Tools/parity_twin_map.json | 12 +-- .../main/java/com/noop/ble/WhoopBleClient.kt | 100 +++++++++++++++--- .../java/com/noop/protocol/Whoop5EcgProbe.kt | 67 ++++++++++++ .../noop/protocol/Whoop5EcgProbeWindowTest.kt | 64 +++++++++++ 7 files changed, 394 insertions(+), 33 deletions(-) create mode 100644 Packages/WhoopProtocol/Tests/WhoopProtocolTests/Whoop5EcgProbeWindowTests.swift create mode 100644 android/app/src/test/java/com/noop/protocol/Whoop5EcgProbeWindowTest.kt diff --git a/Packages/WhoopProtocol/Sources/WhoopProtocol/Whoop5EcgProbe.swift b/Packages/WhoopProtocol/Sources/WhoopProtocol/Whoop5EcgProbe.swift index 3f1528c034..0522a04161 100644 --- a/Packages/WhoopProtocol/Sources/WhoopProtocol/Whoop5EcgProbe.swift +++ b/Packages/WhoopProtocol/Sources/WhoopProtocol/Whoop5EcgProbe.swift @@ -201,6 +201,68 @@ public enum Whoop5EcgProbe { } } + /// How long one run listens, in seconds, chosen by what started it. + /// + /// `capture` is 60 rather than 30 because of @meta1971's field timing in #891: across five MG + /// sessions the strap's terminal frame (classifier state 2, progress 100) landed 38 to 39 s after the + /// first live frame, so a 30 s window could not reach a completed reading on ANY strap or firmware. + /// The verdict sheet landing at 30 s made that 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. + /// + /// `selectOrStop` stays at 30. Those runs wait for a COMMAND_RESPONSE rather than for a reading, so + /// the longer window would be dead time in front of an answer that already arrived. + /// + /// `terminalGrace` is how long a run keeps listening AFTER the terminal frame instead of ending on + /// it. @meta1971 measured the variability field sitting at `0xffff` for 2 to 9 s past that frame + /// before it settled, so a probe that stopped on the terminal frame would pin the unset value as the + /// reading's own. It also means a run that ends early reports early: a terminal frame at 12 s renders + /// the verdict at 22 s rather than holding the sheet for the rest of the window. + public enum Window { + public static let selectOrStop = 30 + public static let capture = 60 + public static let terminalGrace = 10 + } + + /// Cap on the per-packet candidate lines one run records. Each line is rendered into both the report + /// and the strap log, so a chatty stream must not grow either without bound. + public static let maxCandidateLines = 12 + + /// Slots a run keeps in reserve for TERMINAL frames once `maxCandidateLines` is spent. + /// + /// R17 arrives at about one frame a second, so a flat cap keeps seconds 1 to 12 and discards + /// everything after. Per #891 the result, average HR and reason mask populate only on the terminal + /// frame, which means the flat cap threw away the single frame carrying the answer and kept twelve + /// that could not. A reserve rather than a bigger cap: the point is not more evidence, it is the RIGHT + /// evidence, and the ceiling stays bounded at `maxCandidateLines + maxTerminalCandidateLines`. + public static let maxTerminalCandidateLines = 4 + + /// Whether a decoded frame's line is kept, given what the run has already recorded. + /// + /// A terminal frame inside the first `maxCandidateLines` is kept by the ordinary rule and still spends + /// one reserve slot, so a run can never record more than the ceiling either way. + /// + /// Kotlin twin: `Whoop5EcgProbe.retainsCandidate`. + public static func retainsCandidate(linesKept: Int, + terminalLinesKept: Int, + isTerminal: Bool) -> Bool { + if linesKept < maxCandidateLines { return true } + return isTerminal && terminalLinesKept < maxTerminalCandidateLines + } + + /// Seconds to wait before rendering the verdict once a terminal frame lands, or nil to leave the + /// run's existing deadline alone. + /// + /// ONCE per run (`alreadyExtended`), because the strap sends more than one terminal frame: extending + /// on each would let a terminal stream push the verdict out indefinitely and the report would never + /// render. + /// + /// Kotlin twin: `Whoop5EcgProbe.terminalGraceSeconds`. + public static func terminalGraceSeconds(isTerminal: Bool, alreadyExtended: Bool) -> Int? { + guard isTerminal, !alreadyExtended else { return nil } + return Window.terminalGrace + } + /// Classify the run. /// /// Order matters, and it puts the ATTESTED signals first: the COMMAND_RESPONSE result codes are diff --git a/Packages/WhoopProtocol/Tests/WhoopProtocolTests/Whoop5EcgProbeWindowTests.swift b/Packages/WhoopProtocol/Tests/WhoopProtocolTests/Whoop5EcgProbeWindowTests.swift new file mode 100644 index 0000000000..718b79e0c8 --- /dev/null +++ b/Packages/WhoopProtocol/Tests/WhoopProtocolTests/Whoop5EcgProbeWindowTests.swift @@ -0,0 +1,57 @@ +import XCTest +@testable import WhoopProtocol + +/// #891 — the two rules that decide whether a run can OBSERVE a completed reading at all. +/// +/// Both defects they pin were invisible to every existing test, because the probe's own report looked +/// correct: the packet count was honest, the verdict wording was honest, and the run simply never +/// reached the frame that carries the answer. @meta1971's field timing (terminal frame at 38 to 39 s +/// across five MG sessions, variability unset for 2 to 9 s after it) is the evidence behind the numbers. +/// +/// The Kotlin twin pins the same cases in `Whoop5EcgProbeWindowTest`. +final class Whoop5EcgProbeWindowTests: XCTestCase { + + func testCaptureWindowOutlastsTheMeasuredTerminalFrame() { + // The whole defect in one assertion: 30 s cannot reach a frame that lands at 38 to 39 s. + XCTAssertGreaterThan(Whoop5EcgProbe.Window.capture, 39) + XCTAssertEqual(Whoop5EcgProbe.Window.capture, 60) + // A run waiting on a COMMAND_RESPONSE is not waiting on a reading, so it keeps the short window. + XCTAssertEqual(Whoop5EcgProbe.Window.selectOrStop, 30) + // Long enough to outlast the measured 2-to-9 s unset variability window after the terminal frame. + XCTAssertGreaterThan(Whoop5EcgProbe.Window.terminalGrace, 9) + XCTAssertEqual(Whoop5EcgProbe.Window.terminalGrace, 10) + } + + func testOrdinaryFramesFillTheCapAndThenStop() { + XCTAssertTrue(Whoop5EcgProbe.retainsCandidate(linesKept: 0, terminalLinesKept: 0, + isTerminal: false)) + XCTAssertTrue(Whoop5EcgProbe.retainsCandidate(linesKept: 11, terminalLinesKept: 0, + isTerminal: false)) + XCTAssertFalse(Whoop5EcgProbe.retainsCandidate(linesKept: 12, terminalLinesKept: 0, + isTerminal: false)) + XCTAssertFalse(Whoop5EcgProbe.retainsCandidate(linesKept: 99, terminalLinesKept: 0, + isTerminal: false)) + } + + func testTerminalFramesAreKeptPastTheCapButStayBounded() { + // THE #891 CASE. R17 arrives about once a second, so at the terminal frame the ordinary cap has + // long since filled with seconds 1 to 12 — the twelve frames that carry no result. + XCTAssertTrue(Whoop5EcgProbe.retainsCandidate(linesKept: 12, terminalLinesKept: 0, + isTerminal: true)) + XCTAssertTrue(Whoop5EcgProbe.retainsCandidate(linesKept: 40, terminalLinesKept: 3, + isTerminal: true)) + // Bounded: the reserve is four slots, not an exemption. + XCTAssertFalse(Whoop5EcgProbe.retainsCandidate(linesKept: 40, terminalLinesKept: 4, + isTerminal: true)) + XCTAssertEqual(Whoop5EcgProbe.maxCandidateLines + Whoop5EcgProbe.maxTerminalCandidateLines, 16) + } + + func testTheGraceExtensionHappensOncePerRun() { + XCTAssertEqual(Whoop5EcgProbe.terminalGraceSeconds(isTerminal: true, alreadyExtended: false), 10) + // A terminal STREAM must not be able to postpone the verdict indefinitely. + XCTAssertNil(Whoop5EcgProbe.terminalGraceSeconds(isTerminal: true, alreadyExtended: true)) + // An ordinary frame leaves the run's deadline alone. + XCTAssertNil(Whoop5EcgProbe.terminalGraceSeconds(isTerminal: false, alreadyExtended: false)) + XCTAssertNil(Whoop5EcgProbe.terminalGraceSeconds(isTerminal: false, alreadyExtended: true)) + } +} diff --git a/Strand/BLE/BLEManager.swift b/Strand/BLE/BLEManager.swift index 0feffde48b..25005f7910 100644 --- a/Strand/BLE/BLEManager.swift +++ b/Strand/BLE/BLEManager.swift @@ -1097,6 +1097,15 @@ public final class BLEManager: NSObject, ObservableObject { private var ecgProbeDeadline: Date? /// Supersedes a previous run's pending verdict timer when the user taps again. private var ecgProbeRunToken = 0 + /// When the run in flight began, so the report states the time actually listened rather than the + /// window asked for. A terminal frame ends a run early (`Whoop5EcgProbe.terminalGraceSeconds`), so + /// those two stopped being the same number. + private var ecgProbeStartedAt: Date? + /// Whether this run already pushed its deadline out on a terminal frame. Once only, or a terminal + /// stream would postpone the verdict forever. + private var ecgProbeTerminalExtended = false + /// How many TERMINAL candidate lines this run has kept, against the reserve the cap holds back. + private var ecgProbeTerminalLinesKept = 0 private var clockRequested = false /// #700: retry count for GET_CLOCK when no correlation establishes before backfill. Capped at 3. private var clockRetries = 0 @@ -4299,10 +4308,12 @@ public final class BLEManager: NSObject, ObservableObject { /// Sentinel shown while a probe run is in flight (twin of the #592/#690 constants). public static let ecgProbeWaiting = "__waiting__" - /// How long the probe listens for ECG-shaped packets before rendering its verdict. - private static let ecgProbeWindow: TimeInterval = 30 - /// Cap on recorded candidate-frame lines, so a chatty stream can't grow the report without bound. - private static let ecgProbeMaxCandidates = 12 + /// How long a run that is waiting on a COMMAND_RESPONSE listens before rendering its verdict. + /// Derived from `Whoop5EcgProbe.Window` so the two platforms cannot hold different numbers: that + /// they both held 30 independently is how the window outlived the evidence against it (#891). + private static let ecgProbeWindow = TimeInterval(Whoop5EcgProbe.Window.selectOrStop) + /// The CAPTURE run's window. Longer, because a reading's terminal frame lands at 38 to 39 s. + private static let ecgProbeCaptureWindow = TimeInterval(Whoop5EcgProbe.Window.capture) /// Cap on recorded steps. A run sends at most three, so the headroom is for UNSOLICITED replies — /// which are inbound-driven and therefore need the same bound as the candidate list. private static let ecgProbeMaxSteps = 12 @@ -4446,13 +4457,13 @@ public final class BLEManager: NSObject, ObservableObject { public func ecgStartCapture() { guard ecgGatesAllow() else { return } ecgMayBeRunning = true // latched BEFORE the sends, so a mid-sequence drop still leaves Stop offered - beginEcgProbeRun(clearingSteps: true) + beginEcgProbeRun(clearingSteps: true, window: BLEManager.ecgProbeCaptureWindow) log("ECG probe: starting the ECG turn-on sequence on an MG (experimental, unvalidated instrumentation)") sendEcgCommand(.toggleLabradorFiltered, arg: 1) sendEcgCommand(.toggleLabradorRawSave, arg: 1) ecgSendAbortHistorical() sendEcgCommand(.toggleLabradorDataGeneration, arg: Whoop5Ecg.ControlSignal.start.rawValue) - scheduleEcgProbeVerdict() + scheduleEcgProbeVerdict(after: BLEManager.ecgProbeCaptureWindow) } /// The START list's first member: ABORT_HISTORICAL_TRANSMITS (20), immediately ahead of `124`. @@ -4523,28 +4534,36 @@ public final class BLEManager: NSObject, ObservableObject { /// Clear the probe result (dialog dismissed). public func clearEcgProbe() { state.ecgProbe = nil } - private func beginEcgProbeRun(clearingSteps: Bool) { + private func beginEcgProbeRun(clearingSteps: Bool, + window: TimeInterval = BLEManager.ecgProbeWindow) { if clearingSteps { ecgProbeSteps = [] ecgProbeCandidates = [] ecgProbePacketsSeen = 0 } ecgProbeRunToken &+= 1 - ecgProbeDeadline = Date().addingTimeInterval(BLEManager.ecgProbeWindow) + ecgProbeStartedAt = Date() + ecgProbeTerminalExtended = false + ecgProbeTerminalLinesKept = 0 + ecgProbeDeadline = Date().addingTimeInterval(window) state.ecgProbe = BLEManager.ecgProbeWaiting } /// Render the verdict once the listen window closes. BLE callbacks and this timer both run on the /// main queue, so the token check is race-free without a lock. - private func scheduleEcgProbeVerdict() { + private func scheduleEcgProbeVerdict(after window: TimeInterval = BLEManager.ecgProbeWindow) { let token = ecgProbeRunToken - DispatchQueue.main.asyncAfter(deadline: .now() + BLEManager.ecgProbeWindow) { [weak self] in + DispatchQueue.main.asyncAfter(deadline: .now() + window) { [weak self] in guard let self, self.ecgProbeRunToken == token else { return } self.ecgProbeDeadline = nil + // The time LISTENED, not the window asked for. A terminal frame cuts a run short, and the + // verdict text quotes this number ("no ECG packet arrived in Ns"), so quoting the request + // would have the report claim a window it did not run. + let listened = Int((Date().timeIntervalSince(self.ecgProbeStartedAt ?? Date())).rounded()) let text = Whoop5EcgProbe.report(steps: self.ecgProbeSteps, ecgPacketsSeen: self.ecgProbePacketsSeen, candidateFrames: self.ecgProbeCandidates, - windowSeconds: Int(BLEManager.ecgProbeWindow)) + windowSeconds: listened) self.log("ECG probe:\n\(text)") self.state.ecgProbe = text } @@ -4617,7 +4636,26 @@ public final class BLEManager: NSObject, ObservableObject { private func noteEcgProbeCandidate(_ frame: [UInt8]) { guard frame.count >= 12, let packet = Whoop5Ecg.r17FromFrame(frame) else { return } ecgProbePacketsSeen += 1 - guard ecgProbeCandidates.count < BLEManager.ecgProbeMaxCandidates else { return } + // A TERMINAL frame is kept even past the cap, and ends the run on a grace period rather than on + // the frame itself. Both halves come from #891: the result, average HR and reason mask populate + // only on that frame, so the old flat cap kept seconds 1 to 12 and discarded the one frame that + // carried the answer; and the variability field reads 0xffff for several seconds after it, so + // stopping on the frame would pin the unset value as the reading's own. + if let grace = Whoop5EcgProbe.terminalGraceSeconds(isTerminal: packet.isTerminal, + alreadyExtended: ecgProbeTerminalExtended) { + ecgProbeTerminalExtended = true + // Re-arm rather than cancel: bumping the token strands the pending verdict timer, which is + // the same mechanism a second user tap already uses. + ecgProbeRunToken &+= 1 + ecgProbeDeadline = Date().addingTimeInterval(TimeInterval(grace)) + log("ECG probe: terminal frame (state=\(packet.classifierState) progress=" + + "\(packet.progress.raw)) — listening \(grace)s more before the verdict") + scheduleEcgProbeVerdict(after: TimeInterval(grace)) + } + guard Whoop5EcgProbe.retainsCandidate(linesKept: ecgProbeCandidates.count, + terminalLinesKept: ecgProbeTerminalLinesKept, + isTerminal: packet.isTerminal) else { return } + if packet.isTerminal { ecgProbeTerminalLinesKept += 1 } // The classifier byte is logged as a NUMBER, never as its token name: a strap log is a shareable // artefact, and no line in it should read like a clinical finding. The decoder maps the value // offline; the raw byte is lossless. @@ -6192,6 +6230,9 @@ extension BLEManager: @preconcurrency CBCentralManagerDelegate { ecgProbeSteps = [] ecgProbeCandidates = [] ecgProbePacketsSeen = 0 + ecgProbeStartedAt = nil + ecgProbeTerminalExtended = false + ecgProbeTerminalLinesKept = 0 // The DIS strings belong to the link that just dropped; a stale variant must not keep an MG-only // capability unlocked for whatever connects next. state.whoop5Variant = nil diff --git a/Tools/parity_twin_map.json b/Tools/parity_twin_map.json index f462057855..6d97b4e14d 100644 --- a/Tools/parity_twin_map.json +++ b/Tools/parity_twin_map.json @@ -19,14 +19,14 @@ }, "authority": { "files": {"count": 510, "sha256": "b1c73338e79009d984d1313d8b3166e52672712c68074f5fee190c503e299b18"}, - "functions": {"count": 4534, "sha256": "91a1670dfe86761a907dc0d1fa73b219e9ca8b5b6d90c36310db21c1657dc884"}, + "functions": {"count": 4538, "sha256": "f62a1ca0dfb2122fdca87a2f455ec0ca4435903c24069ae13bc4e75c0c3f32c0"}, "properties": {"count": 488, "sha256": "77d60fe0f6218252233b9e60da8bf5135968e49be3c6d2027d0973ea20ddf792"}, - "constants": {"count": 1991, "sha256": "ab5fad0319f0ee260993445c0388fbcd89801491c316977095e01014888f9228"}, - "file_pairs": {"count": 80, "sha256": "eeeb9e0f4adabed6e6c3c5886e980ea3a3656b65b5bd2a9b3b9e43b5ea3b051c"}, - "function_pairs": {"count": 224, "sha256": "af7d03c31dc065970bbefee6d86f549137dd3acbcb698017df57f63f436098b6"}, + "constants": {"count": 2001, "sha256": "a9c1f36d2acc614dd747fd6cc0b3ea48079b81517afda5591d219f9591e353c3"}, + "file_pairs": {"count": 81, "sha256": "3c94824ccff843da495a212a973d89ce61e536b6795dbee2d0e496a7e7a498f0"}, + "function_pairs": {"count": 226, "sha256": "7c7f2298ad7921101241f39d86ecab774d21eadeb89d52d4d5db0397878905f2"}, "property_pairs": {"count": 163, "sha256": "b7d4a29086beec661993370657c0bcdae86f79b221fdc0043206161df5e1117f"}, - "constant_pairs": {"count": 697, "sha256": "1f02bdcb642db9d6b24a4cdcbe1351fefbf5ec269830900eba0df49104745b5a"}, - "unpaired_files": {"count": 372, "sha256": "37aa705af1be255d3d18adfe912b93bcfeaa3760e1513412aad81388a4c1410b"}, + "constant_pairs": {"count": 702, "sha256": "e3de4c1b84b8500c5700292648bf23dc680e5e14cde055c36c5d1ef4ecc39dc2"}, + "unpaired_files": {"count": 370, "sha256": "441451c8f68b68e1dc3d3556c28952b4bdac243de9d6f909d3c18d6ac9732a1e"}, "unpaired_functions": {"count": 4099, "sha256": "3ee3e74d901e17225e294c309c1b00a7a5b178bb3164bff43eaaa5309d574169"}, "unpaired_properties": {"count": 162, "sha256": "dabff97ba9bce0aebdc8ef84b9c4f107ce574020e8b0455d8f9214082948246b"}, "unpaired_constants": {"count": 597, "sha256": "578c607cc05b1cd1f867c206c9fab70237ac40707cd49d4e0c5cc66b400184c7"} diff --git a/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt b/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt index b7a7a7e7e7..0505d44c43 100644 --- a/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt +++ b/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt @@ -5146,12 +5146,13 @@ class WhoopBleClient( */ // ---- WHOOP MG ECG ("Labrador") turn-on probe — twin of macOS BLEManager.ecg* ---------------- - /** Listen window after the turn-on burst, matching macOS `ecgProbeWindow`. */ - private val ECG_PROBE_WINDOW_SECONDS = 30 + /** Listen window for a run waiting on a COMMAND_RESPONSE, matching macOS `ecgProbeWindow`. + * Derived from [Whoop5EcgProbe.Window] so the two platforms cannot hold different numbers: that + * they both held 30 independently is how the window outlived the evidence against it (#891). */ + private val ECG_PROBE_WINDOW_SECONDS = Whoop5EcgProbe.Window.SELECT_OR_STOP - /** Detailed candidate lines are capped; the PACKET COUNT is not, so the verdict stays complete - * while a chatty stream cannot grow the strap log without bound. Matches macOS. */ - private val ECG_PROBE_MAX_CANDIDATES = 12 + /** The CAPTURE run's window. Longer, because a reading's terminal frame lands at 38 to 39 s. */ + private val ECG_PROBE_CAPTURE_WINDOW_SECONDS = Whoop5EcgProbe.Window.CAPTURE /** * Guards every probe accumulator below. [noteEcgProbeCandidate] runs on the BINDER thread (the @@ -5170,6 +5171,21 @@ class WhoopBleClient( private var ecgProbePacketsSeen = 0 + /** Supersedes a previous run's pending verdict post when a run is re-armed. Guarded by + * [ecgProbeLock], because a terminal frame re-arms from the binder thread. */ + private var ecgProbeRunToken = 0 + + /** When the run in flight began, so the report states the time actually listened rather than the + * window asked for: a terminal frame ends a run early, so those stopped being the same number. */ + private var ecgProbeStartedAtMs = 0L + + /** Whether this run already pushed its deadline out on a terminal frame. Once only, or a terminal + * stream would postpone the verdict forever. */ + private var ecgProbeTerminalExtended = false + + /** How many TERMINAL candidate lines this run has kept, against the reserve the cap holds back. */ + private var ecgProbeTerminalLinesKept = 0 + /** * True only while a probe run is sending. The send() allowlist reads this, so the three toggles are * unformable on a default install even with the opt-in on: permission and in-flight are separate @@ -5247,19 +5263,48 @@ class WhoopBleClient( ecgProbeSteps.clear() ecgProbeCandidates.clear() ecgProbePacketsSeen = 0 + ecgProbeRunToken += 1 + ecgProbeStartedAtMs = System.currentTimeMillis() + ecgProbeTerminalExtended = false + ecgProbeTerminalLinesKept = 0 } ecgProbeListening = true } - private fun scheduleEcgProbeVerdict() { + private fun scheduleEcgProbeVerdict(windowSeconds: Int = ECG_PROBE_WINDOW_SECONDS) { + val token = synchronized(ecgProbeLock) { ecgProbeRunToken } handler.postDelayed({ - ecgProbeListening = false - val (steps, packets, candidates) = synchronized(ecgProbeLock) { - Triple(ecgProbeSteps.toList(), ecgProbePacketsSeen, ecgProbeCandidates.toList()) + // Token check, not removeCallbacks: a terminal frame re-arms the run from the binder thread + // and a second user tap re-arms from main, so a stale post must simply do nothing. Twin of + // the macOS run-token guard. + val (stale, steps, packets, candidates, listened) = synchronized(ecgProbeLock) { + EcgVerdictSnapshot( + ecgProbeRunToken != token, + ecgProbeSteps.toList(), + ecgProbePacketsSeen, + ecgProbeCandidates.toList(), + // Integer rounding rather than a Double hop: this is a whole number of seconds in + // a report line, and it matches the Swift side's `.rounded()` for a positive span. + ((System.currentTimeMillis() - ecgProbeStartedAtMs + 500L) / 1000L).toInt(), + ) } - log(Whoop5EcgProbe.report(steps, packets, candidates, ECG_PROBE_WINDOW_SECONDS)) - }, ECG_PROBE_WINDOW_SECONDS * 1000L) - } + if (stale) return@postDelayed + ecgProbeListening = false + // The time LISTENED, not the window asked for. The verdict text quotes this number ("no ECG + // packet arrived in Ns"), so quoting the request would have the report claim a window it + // did not run. + log(Whoop5EcgProbe.report(steps, packets, candidates, listened)) + }, windowSeconds * 1000L) + } + + /** The verdict's inputs, read under [ecgProbeLock] in one go so the report cannot mix two runs. */ + private data class EcgVerdictSnapshot( + val stale: Boolean, + val steps: List, + val packets: Int, + val candidates: List, + val listenedSeconds: Int, + ) /** * The START list's first member: ABORT_HISTORICAL_TRANSMITS (20), immediately ahead of `124`. @@ -5331,7 +5376,7 @@ class WhoopBleClient( // Scheduled in the finally, because it is what CLOSES the listen window. If a send threw, // an early return would leave `ecgProbeListening` true forever and the triage running on // every frame for the life of the process. - scheduleEcgProbeVerdict() + scheduleEcgProbeVerdict(ECG_PROBE_CAPTURE_WINDOW_SECONDS) } } @@ -5379,10 +5424,35 @@ class WhoopBleClient( private fun noteEcgProbeCandidate(frame: ByteArray) { if (frame.size < 12) return val packet = Whoop5Ecg.r17FromFrame(frame) ?: return - synchronized(ecgProbeLock) { + // A TERMINAL frame is kept even past the cap, and ends the run on a grace period rather than on + // the frame itself. Both halves come from #891: the result, average HR and reason mask populate + // only on that frame, so the old flat cap kept seconds 1 to 12 and discarded the one frame that + // carried the answer; and the variability field reads 0xffff for several seconds after it, so + // stopping on the frame would pin the unset value as the reading's own. + var grace: Int? = null + val keep = synchronized(ecgProbeLock) { ecgProbePacketsSeen += 1 - if (ecgProbeCandidates.size >= ECG_PROBE_MAX_CANDIDATES) return + grace = Whoop5EcgProbe.terminalGraceSeconds(packet.isTerminal, ecgProbeTerminalExtended) + if (grace != null) ecgProbeTerminalExtended = true + val retain = Whoop5EcgProbe.retainsCandidate( + linesKept = ecgProbeCandidates.size, + terminalLinesKept = ecgProbeTerminalLinesKept, + isTerminal = packet.isTerminal, + ) + if (retain && packet.isTerminal) ecgProbeTerminalLinesKept += 1 + retain + } + grace?.let { seconds -> + log( + "ECG probe: terminal frame (state=${packet.classifierState} " + + "progress=${packet.progress.raw}) — listening ${seconds}s more before the verdict", + ) + // Re-arms the run: beginEcgProbeRun bumped the token at the start, and scheduling against a + // fresh token strands the pending verdict post. Same mechanism a second user tap uses. + synchronized(ecgProbeLock) { ecgProbeRunToken += 1 } + scheduleEcgProbeVerdict(seconds) } + if (!keep) return // The classifier byte is logged as a NUMBER, never as its token name: a strap log is a shareable // artefact, and no line in it should read like a clinical finding. // diff --git a/android/app/src/main/java/com/noop/protocol/Whoop5EcgProbe.kt b/android/app/src/main/java/com/noop/protocol/Whoop5EcgProbe.kt index 9bbd52afe1..73bca48483 100644 --- a/android/app/src/main/java/com/noop/protocol/Whoop5EcgProbe.kt +++ b/android/app/src/main/java/com/noop/protocol/Whoop5EcgProbe.kt @@ -206,6 +206,73 @@ object Whoop5EcgProbe { } } + /** + * How long one run listens, in seconds, chosen by what started it. + * + * [CAPTURE] is 60 rather than 30 because of @meta1971's field timing in #891: across five MG sessions + * the strap's terminal frame (classifier state 2, progress 100) landed 38 to 39 s after the first live + * frame, so a 30 s window could not reach a completed reading on ANY strap or firmware. The verdict + * sheet landing at 30 s made that 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. + * + * [SELECT_OR_STOP] stays at 30. Those runs wait for a COMMAND_RESPONSE rather than for a reading, so + * the longer window would be dead time in front of an answer that already arrived. + * + * [TERMINAL_GRACE] is how long a run keeps listening AFTER the terminal frame instead of ending on it. + * @meta1971 measured the variability field sitting at `0xffff` for 2 to 9 s past that frame before it + * settled, so a probe that stopped on the terminal frame would pin the unset value as the reading's + * own. It also means a run that ends early reports early: a terminal frame at 12 s renders the verdict + * at 22 s rather than holding the sheet for the rest of the window. + */ + object Window { + const val SELECT_OR_STOP = 30 + const val CAPTURE = 60 + const val TERMINAL_GRACE = 10 + } + + /** Cap on the per-packet candidate lines one run records. Each line is rendered into both the report + * and the strap log, so a chatty stream must not grow either without bound. */ + const val MAX_CANDIDATE_LINES = 12 + + /** + * Slots a run keeps in reserve for TERMINAL frames once [MAX_CANDIDATE_LINES] is spent. + * + * R17 arrives at about one frame a second, so a flat cap keeps seconds 1 to 12 and discards everything + * after. Per #891 the result, average HR and reason mask populate only on the terminal frame, which + * means the flat cap threw away the single frame carrying the answer and kept twelve that could not. A + * reserve rather than a bigger cap: the point is not more evidence, it is the RIGHT evidence, and the + * ceiling stays bounded at [MAX_CANDIDATE_LINES] + [MAX_TERMINAL_CANDIDATE_LINES]. + */ + const val MAX_TERMINAL_CANDIDATE_LINES = 4 + + /** + * Whether a decoded frame's line is kept, given what the run has already recorded. + * + * A terminal frame inside the first [MAX_CANDIDATE_LINES] is kept by the ordinary rule and still spends + * one reserve slot, so a run can never record more than the ceiling either way. + * + * Swift twin: `Whoop5EcgProbe.retainsCandidate`. + */ + fun retainsCandidate(linesKept: Int, terminalLinesKept: Int, isTerminal: Boolean): Boolean { + if (linesKept < MAX_CANDIDATE_LINES) return true + return isTerminal && terminalLinesKept < MAX_TERMINAL_CANDIDATE_LINES + } + + /** + * Seconds to wait before rendering the verdict once a terminal frame lands, or null to leave the run's + * existing deadline alone. + * + * ONCE per run ([alreadyExtended]), because the strap sends more than one terminal frame: extending on + * each would let a terminal stream push the verdict out indefinitely and the report would never render. + * + * Swift twin: `Whoop5EcgProbe.terminalGraceSeconds`. + */ + fun terminalGraceSeconds(isTerminal: Boolean, alreadyExtended: Boolean): Int? { + if (!isTerminal || alreadyExtended) return null + return Window.TERMINAL_GRACE + } + /** * Classify the run. * diff --git a/android/app/src/test/java/com/noop/protocol/Whoop5EcgProbeWindowTest.kt b/android/app/src/test/java/com/noop/protocol/Whoop5EcgProbeWindowTest.kt new file mode 100644 index 0000000000..bf0cea7a8c --- /dev/null +++ b/android/app/src/test/java/com/noop/protocol/Whoop5EcgProbeWindowTest.kt @@ -0,0 +1,64 @@ +package com.noop.protocol + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * #891 — the two rules that decide whether a run can OBSERVE a completed reading at all. + * + * Both defects they pin were invisible to every existing test, because the probe's own report looked + * correct: the packet count was honest, the verdict wording was honest, and the run simply never reached + * the frame that carries the answer. @meta1971's field timing (terminal frame at 38 to 39 s across five + * MG sessions, variability unset for 2 to 9 s after it) is the evidence behind the numbers. + * + * Twin of Swift `Whoop5EcgProbeWindowTests`, pinning the same cases. + */ +class Whoop5EcgProbeWindowTest { + + @Test + fun `capture window outlasts the measured terminal frame`() { + // The whole defect in one assertion: 30 s cannot reach a frame that lands at 38 to 39 s. + assertTrue(Whoop5EcgProbe.Window.CAPTURE > 39) + assertEquals(60, Whoop5EcgProbe.Window.CAPTURE) + // A run waiting on a COMMAND_RESPONSE is not waiting on a reading, so it keeps the short window. + assertEquals(30, Whoop5EcgProbe.Window.SELECT_OR_STOP) + // Long enough to outlast the measured 2-to-9 s unset variability window after the terminal frame. + assertTrue(Whoop5EcgProbe.Window.TERMINAL_GRACE > 9) + assertEquals(10, Whoop5EcgProbe.Window.TERMINAL_GRACE) + } + + @Test + fun `ordinary frames fill the cap and then stop`() { + assertTrue(Whoop5EcgProbe.retainsCandidate(0, 0, isTerminal = false)) + assertTrue(Whoop5EcgProbe.retainsCandidate(11, 0, isTerminal = false)) + assertFalse(Whoop5EcgProbe.retainsCandidate(12, 0, isTerminal = false)) + assertFalse(Whoop5EcgProbe.retainsCandidate(99, 0, isTerminal = false)) + } + + @Test + fun `terminal frames are kept past the cap but stay bounded`() { + // THE #891 CASE. R17 arrives about once a second, so at the terminal frame the ordinary cap has + // long since filled with seconds 1 to 12 — the twelve frames that carry no result. + assertTrue(Whoop5EcgProbe.retainsCandidate(12, 0, isTerminal = true)) + assertTrue(Whoop5EcgProbe.retainsCandidate(40, 3, isTerminal = true)) + // Bounded: the reserve is four slots, not an exemption. + assertFalse(Whoop5EcgProbe.retainsCandidate(40, 4, isTerminal = true)) + assertEquals( + 16, + Whoop5EcgProbe.MAX_CANDIDATE_LINES + Whoop5EcgProbe.MAX_TERMINAL_CANDIDATE_LINES, + ) + } + + @Test + fun `the grace extension happens once per run`() { + assertEquals(10, Whoop5EcgProbe.terminalGraceSeconds(isTerminal = true, alreadyExtended = false)) + // A terminal STREAM must not be able to postpone the verdict indefinitely. + assertNull(Whoop5EcgProbe.terminalGraceSeconds(isTerminal = true, alreadyExtended = true)) + // An ordinary frame leaves the run's deadline alone. + assertNull(Whoop5EcgProbe.terminalGraceSeconds(isTerminal = false, alreadyExtended = false)) + assertNull(Whoop5EcgProbe.terminalGraceSeconds(isTerminal = false, alreadyExtended = true)) + } +} From 24ce11a3d5d30ceacf449620b1ad9ddc2e8ee260 Mon Sep 17 00:00:00 2001 From: Fanboynz Date: Fri, 9 Oct 2026 20:33:20 +1300 Subject: [PATCH 2/3] fix(ecg): keep a capture's frames when the user stops it 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 --- Strand/BLE/BLEManager.swift | 30 ++++++++++++++--- .../main/java/com/noop/ble/WhoopBleClient.kt | 33 ++++++++++++++++--- 2 files changed, 54 insertions(+), 9 deletions(-) diff --git a/Strand/BLE/BLEManager.swift b/Strand/BLE/BLEManager.swift index 25005f7910..06f370fdce 100644 --- a/Strand/BLE/BLEManager.swift +++ b/Strand/BLE/BLEManager.swift @@ -4522,7 +4522,15 @@ public final class BLEManager: NSObject, ObservableObject { } ecgStopOverride = true defer { ecgStopOverride = false } - if reportsResult { beginEcgProbeRun(clearingSteps: true) } else { ecgProbeSteps = [] } + // Read BEFORE the run is re-armed: an open window means the capture's verdict has not rendered + // yet, so its evidence has never been reported and must survive into this run. Once the verdict + // has rendered the lines are already out, and a later stop is a genuinely separate run. + let captureStillListening = ecgProbeArmed + if reportsResult { + beginEcgProbeRun(clearingSteps: true, keepingEvidence: captureStillListening) + } else { + ecgProbeSteps = [] + } log("ECG probe: stopping ECG data generation and both streams") sendEcgCommand(.toggleLabradorDataGeneration, arg: Whoop5Ecg.ControlSignal.stop.rawValue) sendEcgCommand(.toggleLabradorRawSave, arg: 0) @@ -4534,15 +4542,29 @@ public final class BLEManager: NSObject, ObservableObject { /// Clear the probe result (dialog dismissed). public func clearEcgProbe() { state.ecgProbe = nil } + /// `keepingEvidence` carries a capture's collected R17 lines, packet count and start time into the + /// STOP run that follows it, instead of clearing them with the steps. + /// + /// Without it the stop discards exactly what this probe exists to retain. The reading completes at + /// 38 to 39 s (#891) while the sheet still reads "waiting", so the natural next action is to stop a + /// session the user can tell has finished — and that wiped every candidate line, the terminal frame + /// among them. The 30 s window hid this by rendering the verdict before most people got there. + /// + /// The START TIME travels with the packets on purpose. The report states "packets seen in Ns", so + /// keeping 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. private func beginEcgProbeRun(clearingSteps: Bool, + keepingEvidence: Bool = false, window: TimeInterval = BLEManager.ecgProbeWindow) { if clearingSteps { ecgProbeSteps = [] - ecgProbeCandidates = [] - ecgProbePacketsSeen = 0 + if !keepingEvidence { + ecgProbeCandidates = [] + ecgProbePacketsSeen = 0 + } } ecgProbeRunToken &+= 1 - ecgProbeStartedAt = Date() + if !keepingEvidence { ecgProbeStartedAt = Date() } ecgProbeTerminalExtended = false ecgProbeTerminalLinesKept = 0 ecgProbeDeadline = Date().addingTimeInterval(window) diff --git a/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt b/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt index 0505d44c43..8fd7e63730 100644 --- a/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt +++ b/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt @@ -5258,13 +5258,28 @@ class WhoopBleClient( send(cmd, Whoop5Ecg.commandPayload(arg).map { it.toByte() }.toByteArray()) } - private fun beginEcgProbeRun() { + /** + * [keepingEvidence] carries a capture's collected R17 lines, packet count and start time into the + * STOP run that follows it, instead of clearing them with the steps. + * + * Without it the stop discards exactly what this probe exists to retain. The reading completes at 38 + * to 39 s (#891) while the sheet still reads "waiting", so the natural next action is to stop a + * session the user can tell has finished, and that wiped every candidate line, the terminal frame + * among them. The 30 s window hid this by rendering the verdict before most people got there. + * + * The START TIME travels with the packets on purpose. The report states "packets seen in Ns", so + * keeping 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. + */ + private fun beginEcgProbeRun(keepingEvidence: Boolean = false) { synchronized(ecgProbeLock) { ecgProbeSteps.clear() - ecgProbeCandidates.clear() - ecgProbePacketsSeen = 0 + if (!keepingEvidence) { + ecgProbeCandidates.clear() + ecgProbePacketsSeen = 0 + ecgProbeStartedAtMs = System.currentTimeMillis() + } ecgProbeRunToken += 1 - ecgProbeStartedAtMs = System.currentTimeMillis() ecgProbeTerminalExtended = false ecgProbeTerminalLinesKept = 0 } @@ -5395,7 +5410,15 @@ class WhoopBleClient( } return } - if (reportsResult) beginEcgProbeRun() else synchronized(ecgProbeLock) { ecgProbeSteps.clear() } + // Read BEFORE the run is re-armed: an open window means the capture's verdict has not rendered + // yet, so its evidence has never been reported and must survive into this run. Once the verdict + // has rendered the lines are already out, and a later stop is a genuinely separate run. + val captureStillListening = ecgProbeListening + if (reportsResult) { + beginEcgProbeRun(keepingEvidence = captureStillListening) + } else { + synchronized(ecgProbeLock) { ecgProbeSteps.clear() } + } log("ECG probe: stopping ECG data generation and both streams") ecgProbeArmed = true try { From 7dc8f1fd6fa79e3b79bdbbd97327da23c9b8c5b6 Mon Sep 17 00:00:00 2001 From: Fanboynz Date: Fri, 9 Oct 2026 20:36:51 +1300 Subject: [PATCH 3/3] refactor(ecg): keep the listen window in one place 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 --- Strand/BLE/BLEManager.swift | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/Strand/BLE/BLEManager.swift b/Strand/BLE/BLEManager.swift index 06f370fdce..053813a014 100644 --- a/Strand/BLE/BLEManager.swift +++ b/Strand/BLE/BLEManager.swift @@ -1093,7 +1093,9 @@ public final class BLEManager: NSObject, ObservableObject { /// Structural-triage hits: the empirical search for the packet TYPE these records arrive under. private var ecgProbeCandidates: [String] = [] private var ecgProbePacketsSeen = 0 - /// Non-nil while the listen window is open; drives `ecgProbeArmed`. + /// Non-nil while the listen window is open; drives `ecgProbeArmed`. A MARKER, not a deadline: + /// nothing compares the stamp to the clock, and the window's length lives only in the verdict + /// timer, so do not start reading this as the moment a run ends. private var ecgProbeDeadline: Date? /// Supersedes a previous run's pending verdict timer when the user taps again. private var ecgProbeRunToken = 0 @@ -4457,7 +4459,7 @@ public final class BLEManager: NSObject, ObservableObject { public func ecgStartCapture() { guard ecgGatesAllow() else { return } ecgMayBeRunning = true // latched BEFORE the sends, so a mid-sequence drop still leaves Stop offered - beginEcgProbeRun(clearingSteps: true, window: BLEManager.ecgProbeCaptureWindow) + beginEcgProbeRun(clearingSteps: true) log("ECG probe: starting the ECG turn-on sequence on an MG (experimental, unvalidated instrumentation)") sendEcgCommand(.toggleLabradorFiltered, arg: 1) sendEcgCommand(.toggleLabradorRawSave, arg: 1) @@ -4553,9 +4555,7 @@ public final class BLEManager: NSObject, ObservableObject { /// The START TIME travels with the packets on purpose. The report states "packets seen in Ns", so /// keeping 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. - private func beginEcgProbeRun(clearingSteps: Bool, - keepingEvidence: Bool = false, - window: TimeInterval = BLEManager.ecgProbeWindow) { + private func beginEcgProbeRun(clearingSteps: Bool, keepingEvidence: Bool = false) { if clearingSteps { ecgProbeSteps = [] if !keepingEvidence { @@ -4567,13 +4567,20 @@ public final class BLEManager: NSObject, ObservableObject { if !keepingEvidence { ecgProbeStartedAt = Date() } ecgProbeTerminalExtended = false ecgProbeTerminalLinesKept = 0 - ecgProbeDeadline = Date().addingTimeInterval(window) + // Armed from HERE rather than from `scheduleEcgProbeVerdict`, because the sends in between can + // draw a COMMAND_RESPONSE and the triage is gated on this. The VALUE is a marker only: nothing + // compares it to the clock, and the length of the window lives in the verdict timer alone, so + // there is one number to get right instead of two that can disagree. + ecgProbeDeadline = Date() state.ecgProbe = BLEManager.ecgProbeWaiting } /// Render the verdict once the listen window closes. BLE callbacks and this timer both run on the /// main queue, so the token check is race-free without a lock. private func scheduleEcgProbeVerdict(after window: TimeInterval = BLEManager.ecgProbeWindow) { + // Renews the armed marker as well as the timer: a terminal frame re-arms through here, and the + // two must not be able to drift apart. + ecgProbeDeadline = Date() let token = ecgProbeRunToken DispatchQueue.main.asyncAfter(deadline: .now() + window) { [weak self] in guard let self, self.ecgProbeRunToken == token else { return } @@ -4669,7 +4676,6 @@ public final class BLEManager: NSObject, ObservableObject { // Re-arm rather than cancel: bumping the token strands the pending verdict timer, which is // the same mechanism a second user tap already uses. ecgProbeRunToken &+= 1 - ecgProbeDeadline = Date().addingTimeInterval(TimeInterval(grace)) log("ECG probe: terminal frame (state=\(packet.classifierState) progress=" + "\(packet.progress.raw)) — listening \(grace)s more before the verdict") scheduleEcgProbeVerdict(after: TimeInterval(grace))