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..053813a014 100644 --- a/Strand/BLE/BLEManager.swift +++ b/Strand/BLE/BLEManager.swift @@ -1093,10 +1093,21 @@ 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 + /// 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 +4310,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 @@ -4452,7 +4465,7 @@ public final class BLEManager: NSObject, ObservableObject { 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`. @@ -4511,7 +4524,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) @@ -4523,28 +4544,55 @@ public final class BLEManager: NSObject, ObservableObject { /// Clear the probe result (dialog dismissed). public func clearEcgProbe() { state.ecgProbe = nil } - private func beginEcgProbeRun(clearingSteps: Bool) { + /// `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) { if clearingSteps { ecgProbeSteps = [] - ecgProbeCandidates = [] - ecgProbePacketsSeen = 0 + if !keepingEvidence { + ecgProbeCandidates = [] + ecgProbePacketsSeen = 0 + } } ecgProbeRunToken &+= 1 - ecgProbeDeadline = Date().addingTimeInterval(BLEManager.ecgProbeWindow) + if !keepingEvidence { ecgProbeStartedAt = Date() } + ecgProbeTerminalExtended = false + ecgProbeTerminalLinesKept = 0 + // 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() { + 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() + 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 +4665,25 @@ 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 + 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 +6258,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..8fd7e63730 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 @@ -5242,24 +5258,68 @@ 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 + 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 +5391,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) } } @@ -5350,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 { @@ -5379,10 +5447,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)) + } +}