From 264abe1da9c00a239e47e4e28767a81ef8ec9fc9 Mon Sep 17 00:00:00 2001 From: Artur Iskakov Date: Sat, 26 Sep 2026 12:22:37 +0200 Subject: [PATCH 01/38] fix(ble): decode the 5/MG GET_CLOCK reply MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit W06-001 — the strap's GET_CLOCK reply was only hex-logged, and the log line said the result offset was unestablished for the clock. - Severity: S4 · Category: truth · Upstream-able: yes - Location: `Packages/WhoopProtocol/.../StrapClock.swift` `decodeReply`; `Strand/BLE/FrameRouter.swift` clock line - V0: 20 SET/GET pairs in 13 strap logs from firmware 50.39.1.0 share one layout: origin sequence at byte 11, result at 12 (SUCCESS in all 20), the strap's unix seconds as a u32 LE at 13, then seven zero bytes. This is the format-1 response layout in docs/PROTOCOL_TRANSPORT.md. - Fix: `StrapClock.decodeReply` reads the origin sequence, result and seconds, bounded by the declared length. The FrameRouter line prints the result and the strap's seconds beside the full frame. - V1: `StrapClockTests` decode three real replies, and refuse a SET_CLOCK reply, a request frame, and seconds past the declared length. --- .../Sources/WhoopProtocol/Interpreter.swift | 5 +- .../Sources/WhoopProtocol/StrapClock.swift | 37 ++++++++++++ .../WhoopProtocolTests/StrapClockTests.swift | 59 +++++++++++++++++++ Strand/BLE/FrameRouter.swift | 22 +++---- docs/BLE_REVERSE_ENGINEERING.md | 6 +- docs/review/workstreams/W06-ble-collect.md | 4 +- 6 files changed, 112 insertions(+), 21 deletions(-) create mode 100644 Packages/WhoopProtocol/Sources/WhoopProtocol/StrapClock.swift create mode 100644 Packages/WhoopProtocol/Tests/WhoopProtocolTests/StrapClockTests.swift diff --git a/Packages/WhoopProtocol/Sources/WhoopProtocol/Interpreter.swift b/Packages/WhoopProtocol/Sources/WhoopProtocol/Interpreter.swift index 187a3696bc..1be11bd001 100644 --- a/Packages/WhoopProtocol/Sources/WhoopProtocol/Interpreter.swift +++ b/Packages/WhoopProtocol/Sources/WhoopProtocol/Interpreter.swift @@ -933,8 +933,9 @@ public func whoop5HistoricalAckFrame(endData: [UInt8], seq: UInt8) -> [UInt8] { /// The response command is at frame[10] (the 4.0 frame[6] + 4) and its payload at frame[11]. WHOOP 5 /// reuses the 4.0 command NUMBERS, but the response PAYLOADS differ from 4.0 — so each field below is /// mapped from a real WHOOP 5 capture (firmware 50.38.1.0), not ported on faith. Commands that return -/// a short stub on this firmware (REPORT_VERSION_INFO / GET_EXTENDED_BATTERY_INFO) or aren't served -/// (GET_CLOCK — unneeded, since realtime + historical carry real unix) are intentionally left undecoded. +/// a short stub on this firmware (REPORT_VERSION_INFO / GET_EXTENDED_BATTERY_INFO) are intentionally left +/// undecoded. GET_CLOCK, not served on 50.38.1.0, is served on 50.39.1.0 and decoded by +/// `StrapClock.decodeReply` rather than here. private func decodeWhoop5CommandResponse(_ frame: [UInt8], fb: FieldBuilder, schema: Schema, payloadEnd: Int?, limit: Int) { guard let payloadEnd = payloadEnd, 11 < payloadEnd, payloadEnd <= frame.count else { return } diff --git a/Packages/WhoopProtocol/Sources/WhoopProtocol/StrapClock.swift b/Packages/WhoopProtocol/Sources/WhoopProtocol/StrapClock.swift new file mode 100644 index 0000000000..1fbe8c3ef0 --- /dev/null +++ b/Packages/WhoopProtocol/Sources/WhoopProtocol/StrapClock.swift @@ -0,0 +1,37 @@ +import Foundation + +/// The WHOOP 5/MG strap clock as GET_CLOCK (11) reports it (W06-001). +public enum StrapClock { + + /// One GET_CLOCK reply. + public struct Reading: Equatable, Sendable { + /// The sequence of the request this answers (format-1 byte 11), which ties the reply to one read. + public let originSequence: UInt8 + /// The COMMAND_RESPONSE result (byte 12): 0 failure, 1 success, 2 pending, 3 unsupported. + public let result: UInt8 + /// Whole Unix seconds on the strap clock, or nil when the reply body stops before them. + public let seconds: UInt32? + + public init(originSequence: UInt8, result: UInt8, seconds: UInt32?) { + self.originSequence = originSequence; self.result = result; self.seconds = seconds + } + } + + /// Decode a format-1 COMMAND_RESPONSE to GET_CLOCK: type 36 at byte 8, command 11 at byte 10, the + /// request's sequence at 11, the result at 12 and the strap's Unix seconds as a u32 LE at 13. Twenty + /// replies from firmware 50.39.1.0 have this layout, each followed by seven zero bytes that are not read. + /// + /// Structure only: the caller passes a frame that passed `verifyFrame`. The body ends where the declared + /// length (bytes 2–3) puts the CRC32 trailer. Returns nil for any other frame or a body that stops before + /// the result. + public static func decodeReply(_ frame: [UInt8]) -> Reading? { + guard frame.count >= 4 else { return nil } + let bodyEnd = 4 + (Int(frame[2]) | Int(frame[3]) << 8) + guard bodyEnd <= frame.count, bodyEnd > 12, frame[8] == 36, frame[10] == 11 else { return nil } + var seconds: UInt32? + if bodyEnd >= 17 { + seconds = UInt32(frame[13]) | UInt32(frame[14]) << 8 | UInt32(frame[15]) << 16 | UInt32(frame[16]) << 24 + } + return Reading(originSequence: frame[11], result: frame[12], seconds: seconds) + } +} diff --git a/Packages/WhoopProtocol/Tests/WhoopProtocolTests/StrapClockTests.swift b/Packages/WhoopProtocol/Tests/WhoopProtocolTests/StrapClockTests.swift new file mode 100644 index 0000000000..b330114e71 --- /dev/null +++ b/Packages/WhoopProtocol/Tests/WhoopProtocolTests/StrapClockTests.swift @@ -0,0 +1,59 @@ +import XCTest +@testable import WhoopProtocol + +/// W06-001: the GET_CLOCK reply decode. The three GET_CLOCK frames and the SET_CLOCK frame are real replies +/// from one WHOOP MG on firmware 50.39.1.0; the failure reply is built with `puffinCommandFrame`, since no +/// strap log holds one. +final class StrapClockTests: XCTestCase { + + private func bytes(_ s: String) -> [UInt8] { + stride(from: 0, to: s.count, by: 2).map { i in + let a = s.index(s.startIndex, offsetBy: i) + return UInt8(s[a...s.index(a, offsetBy: 1)], radix: 16)! + } + } + + /// A format-1 COMMAND_RESPONSE with the envelope header bytes the strap sends (01 00). + private func reply(cmd: UInt8, origin: UInt8, result: UInt8, body: [UInt8] = []) -> [UInt8] { + puffinCommandFrame(cmd: cmd, seq: 0x40, payload: [origin, result] + body, type: 36, header: [0x01, 0x00]) + } + + private func u32(_ v: UInt32) -> [UInt8] { (0..<4).map { UInt8((v >> (8 * $0)) & 0xFF) } } + + // MARK: decodeReply + + func testRealGetClockRepliesDecodeTheirSecondsAndOrigin() { + let cases: [(String, UInt8, UInt32)] = [ + ("aa011400010021b124230be80146aaaf6a0000000000000014cbbb6c", 0xe8, 1_789_897_286), + ("aa011400010021b124700b020194b5b66a0000000000000005a36ce6", 0x02, 1_790_358_932), + ("aa011400010021b124800b0201bab6b66a00000000000000c0966a91", 0x02, 1_790_359_226), + ] + for (hex, origin, seconds) in cases { + let frame = bytes(hex) + XCTAssertTrue(verifyFrame(frame, family: .whoop5).ok, hex) + XCTAssertEqual(StrapClock.decodeReply(frame), + StrapClock.Reading(originSequence: origin, result: 1, seconds: seconds), hex) + } + } + + func testOtherRepliesAreNotAClockReading() { + let setClockReply = bytes("aa010c000100271124220ae701000000bcc4efc3") + XCTAssertTrue(verifyFrame(setClockReply, family: .whoop5).ok) + XCTAssertNil(StrapClock.decodeReply(setClockReply)) + // Command 11 in a COMMAND frame (type 35) is our own request, not a reply. + XCTAssertNil(StrapClock.decodeReply(puffinCommandFrame(cmd: 11, seq: 3, payload: []))) + XCTAssertNil(StrapClock.decodeReply([0xAA, 0x01])) + } + + func testAFailureReplyWithNoBodyDecodesWithoutSeconds() { + let frame = reply(cmd: 11, origin: 7, result: 3) + XCTAssertTrue(verifyFrame(frame, family: .whoop5).ok) + XCTAssertEqual(StrapClock.decodeReply(frame), StrapClock.Reading(originSequence: 7, result: 3, seconds: nil)) + } + + func testTheBodyEndsAtTheDeclaredLengthNotTheBufferEnd() { + // Extra bytes after the frame (a reassembly overrun) must not be read as seconds. + let frame = reply(cmd: 11, origin: 7, result: 1) + u32(1_790_000_000) + XCTAssertEqual(StrapClock.decodeReply(frame)?.seconds, nil) + } +} diff --git a/Strand/BLE/FrameRouter.swift b/Strand/BLE/FrameRouter.swift index 2f3a39c459..e777f75ef3 100644 --- a/Strand/BLE/FrameRouter.swift +++ b/Strand/BLE/FrameRouter.swift @@ -178,21 +178,13 @@ public final class FrameRouter { // 1970/71 — two contradictory lines with nothing to separate them. Same accept/reject shape // REBOOT_STRAP already uses. LOG-ONLY; it never gates behaviour. if let cmd = parsed.cmdName, cmd.hasPrefix("SET_CLOCK") || cmd.hasPrefix("GET_CLOCK") { - // NO accept/reject verdict here, and that is deliberate: the result byte may not exist at - // all for this command. The captured-frame fixture builds a puffin COMMAND_RESPONSE as - // [36, seq, cmd] + payload at offset 8, so @11 is already PAYLOAD and the @12 that - // `commandResultByte` reads is a payload byte, not a result code. REBOOT_STRAP's use of it - // was validated against reboot's own frames; nothing establishes it for the clock. - // - // Inventing a verdict from that is precisely the fault this line was added to fix - the - // old "clock synced" log asserted an outcome nobody had checked. So quote the evidence - // and let a maintainer decode it: the byte at the result offset, and the WHOLE - // frame (#900's format), uncapped. A truncated clock frame answers nothing, and the full - // frame is what makes a wrong offset assumption visible instead of silently misleading. - let r = Self.commandResultByte(in: frame, family: family) - let rhex = r.map { String(format: "0x%02x", UInt8(truncatingIfNeeded: $0)) } ?? "none" - state.append(log: "clock: \(cmd) reply byte@resultOffset=\(rhex) " - + "frame=\(Self.fullFrameHex(frame))", + // W06-001: both replies carry the format-1 result at byte 12, and GET_CLOCK's carries the + // strap's seconds at 13 (`StrapClock.decodeReply`; 20 replies from 50.39.1.0 agree). This + // line quotes the reply; BLEManager's "WHOOP 5/MG clock:" line judges it. The WHOLE frame + // (#900's format) stays, uncapped, so a moved offset on another firmware stays visible. + let result = parsed.parsed["result"]?.stringValue ?? "none" + let reading = StrapClock.decodeReply(frame)?.seconds.map { " strap=\($0)" } ?? "" + state.append(log: "clock: \(cmd) result=\(result)\(reading) frame=\(Self.fullFrameHex(frame))", domain: .connection) } // #1303: the hunt for a stable per-strap id, which is what multi-strap identity waits on — diff --git a/docs/BLE_REVERSE_ENGINEERING.md b/docs/BLE_REVERSE_ENGINEERING.md index 94adcb3a3a..ed3e015fac 100644 --- a/docs/BLE_REVERSE_ENGINEERING.md +++ b/docs/BLE_REVERSE_ENGINEERING.md @@ -477,8 +477,10 @@ real capture (firmware **50.38.1.0**), never ported on faith (`decodeWhoop5Comma What does **not** transfer: `REPORT_VERSION_INFO` (7) and `GET_EXTENDED_BATTERY_INFO` (98) return short stub payloads on this firmware (so the firmware version lives in the `GET_HELLO` block instead), and -`GET_CLOCK` (11) isn't served at all — WHOOP 5 doesn't need it, since realtime (type-40) and historical -(type-47) both carry real unix rather than a device epoch. +`GET_CLOCK` (11) isn't served at all. Record dating does not need it, since realtime (type-40) and +historical (type-47) both carry real unix rather than a device epoch. Firmware 50.39.1.0 does serve it: +twenty replies carry the format-1 result at `frame[12]` and the strap's unix seconds as a u32 LE at +`frame[13]`, then seven zero bytes (`StrapClock.decodeReply`). > **Privacy.** The `GET_HELLO` response also contains a **session token**, which the decoder never > reads or exposes — only the device name and firmware version are surfaced. The `device_name`/ diff --git a/docs/review/workstreams/W06-ble-collect.md b/docs/review/workstreams/W06-ble-collect.md index 379621849f..b2fcd13f13 100644 --- a/docs/review/workstreams/W06-ble-collect.md +++ b/docs/review/workstreams/W06-ble-collect.md @@ -75,7 +75,7 @@ exactly what was tested on hardware. | ID | Sev | Status | Finding | Location | Evidence | PR | |---|---|---|---|---|---|---| -| W06-001 | S4 | Reported | GET_CLOCK reply dropped for 5/MG | `BLEManager.swift` 5/MG handshake; `FrameRouter.swift` `COMMAND_RESPONSE` | State confirmed: sent on every connect, the reply is only hex-logged (10 replies in owner logs), no decode sets a clock, `clockRef` stays nil. No effect on history dating (records carry unix seconds) or safe-trim, so lowered from S3; what remains is readout truth (W06-019), Phase 3 | | +| W06-001 | S4 | Fixed | GET_CLOCK reply dropped for 5/MG | `BLEManager.swift` 5/MG handshake; `FrameRouter.swift` `COMMAND_RESPONSE` | State confirmed: sent on every connect, the reply is only hex-logged (10 replies in owner logs), no decode sets a clock, `clockRef` stays nil. No effect on history dating (records carry unix seconds) or safe-trim, so lowered from S3; what remains is readout truth (W06-019), Phase 3. V0 (2026-09-26): the reply layout is confirmed on 20 replies from 50.39.1.0: origin sequence at byte 11, result at 12 (SUCCESS in all 20), u32 LE seconds at 13, then seven zero bytes, as `docs/PROTOCOL_TRANSPORT.md` §Responses gives for format 1. Needed by W06-050's read-first policy, so fixed with it. Fixed: `StrapClock.decodeReply` (WhoopProtocol) reads the origin sequence, the result and the seconds, bounded by the declared length, and the FrameRouter clock line now prints the result and the strap's seconds instead of saying the result offset is unestablished. V1: `StrapClockTests` decode three real replies, and refuse a SET_CLOCK reply, our own request frame, and seconds read past the declared length | | | W06-002 | S2 | Fixed | A Raw Data Collector session on a 5/MG never records an IMU sample: `ImuSessionFileStore.append` is reachable only through `Collector.ingest(frame:parsed:)`, which nothing calls | `Collector.swift` `ingest`, `recordGroundTruthImu`; `BLEManager.swift` 5/MG frame loop | P5 confirmed (found by W1 and W6). #1709 moved IMU storage behind an ingest seam 5/MG never enters; upstream has the same gap. V0: 0 production callers of `Collector.ingest(frame:parsed:)`, the only path to `ImuSessionFileStore.append`. Fix: the 5/MG frame loop calls `Collector.bankImuForSessions` for every frame, live and offload, gated to intact 1244-byte buffers. V1: `CollectorImuBankingTests` (intact banked, CRC-flipped refused); Strand and NOOPiOS (no watch) build. V2 (independent reviewer): holds, with gaps recorded as W06-026 … W06-032 and W06-035: the gate checks no packet type (W06-029), a relaunch makes duplicates rescan the segment (W06-026), and no test pinned the call site (W06-027), so V1 was not complete until W06-027 pinned the call; no double bank and no cross-device bank (oracle, relaunch included). V3 (11.9.8 on the iPhone): a 152 s session banked 148 consecutive one-second buffers, all intact, none missing between the first and the last; the 4 missing are the session's last seconds (W06-025). V3 of the offload half, the 11.9.9 strap run (2026-09-25; a session of about three minutes, one marker, an app relaunch mid-session): after the relaunch, the history sync banked the 22 seconds the live stream had missed, the first hardware proof that history reaches a session | | | W06-003 | S3 | Reported | Mapped v20 (optical) and v21 (IMU) have no storage lane: every record goes to the 5 MB rolling reject archive, which one sync saturates, so only the newest ~630–720 of each survive after the strap has trimmed them | `Interpreter.swift` `mappedWhoop5HistoricalVersions`; `HistoricalStreams.swift` `rejectedHistoricalRecords`; `RawHistoryArchive.swift` | P5 confirmed with caveat: nothing consumes v20/v21, so no shown metric is wrong; a retention decision is missing. Every line in the 9 archives in the repo root is type 47 v16, v20 or v21. Root cause of W06-004 and W06-005. AD-6 | | | W06-004 | S3 | Reported | Every over-cap reject-archive rewrite runs on the main actor (about 290 ms for a 5 MB archive on an M-series Mac) and, with v20/v21 holding the archive at cap, recurs every 7–8 chunks through an offload | `RawHistoryArchive.swift` `archive`, `evictLines`; `Backfiller.swift` `rejectedSink` | Found by pass 5 (missed by pass 3). AD-3 evidence; measure on the iPhone in Phase 3 | | @@ -124,7 +124,7 @@ exactly what was tested on hardware. | W06-047 | S4 | Fixed | Every banked IMU buffer builds and configures a `DateFormatter` on the main actor to name its segment file | `ImuSessionFileStore.swift` `segmentFile`, `utcName` | Hot-path cost of the same order as the decode W06-031 removed. V0 (oracle, -O on an M-series Mac): 45.5 µs a call against 1.2 µs with one formatter, same output; so worse than the 8 µs decode W06-031 removed. Fix: one formatter, built once. Refactor check: `ImuSessionFileStoreTests.testSegmentsAreNamedForTheirUTCHalfHour` pins the name on disk and in the export, and passes before and after | | | W06-048 | S4 | Reported | The per-segment `seen` sets kept since W06-026 are never evicted for closed windows; only `remove(id:)` prunes them | `ImuSessionFileStore.swift` `seen` | A week-long historical window re-delivered after a relaunch keeps 336 segment sets | | | W06-049 | S4 | Fixed | `segmentScans` is state the production store carries only so one test can read it | `ImuSessionFileStore.swift` | Cleanup. Fix: the store takes its file reader as a parameter; W06-026's test counts reads through it and now states the property itself, 49 more duplicates reading no file again, and still fails with W06-026's fix reverted | | -| W06-050 | S3 | Reported | A SET_CLOCK during a live Raw Data Collector session steps the strap clock, and the IMU timeline with it. A forward step skips labels, so the session reads incomplete although no motion is missing; a backward step would give new buffers seconds already banked, and `append` drops them as duplicates | `BLEManager.swift` 5/MG handshake (SET_CLOCK on every connect); `ImuSessionFileStore.swift` `append` | Found in the 11.9.9 strap run. The relaunch's SET_CLOCK (value the second after the gap) skipped exactly the three labels before it; the delay from buffer second to arrival fell from 4.7 s to 1.86 s at that moment, and the strap's console counter ran about 3.1 s behind the phone between that set and the one 179 s earlier (its unit read as milliseconds, unconfirmed). The strap's own history has no buffer for those labels either. Caveat: one observation, and the delay held constant through the session, so steady drift does not explain a 3 s lag; the backward case is from the code. Needs an owner decision: set the clock only when GET_CLOCK shows it off (needs W06-001's decode), or record the step in the session and count coverage in strap time. Seen again on 11.9.10 (2026-09-25): the relaunch after W06-053's crash sent SET_CLOCK mid-session; the delay from buffer second to arrival fell from 5.8 s to 1.9 s across it, the four labels before it are missing from both the live stream and the strap's history, and the session read 113 of 117 s. The marker-free session with no relaunch read complete. Two observations now, both a forward step of about four seconds at a mid-session SET_CLOCK. Owner decision 2026-09-25: read the clock, then set it. Decode the GET_CLOCK reply (W06-001), send GET_CLOCK first, and send SET_CLOCK only when the strap clock is invalid or off by more than about two seconds. Its own PR with W06-001, after the reply layout is confirmed on the strap | | +| W06-050 | S3 | Reproduced | A SET_CLOCK during a live Raw Data Collector session steps the strap clock, and the IMU timeline with it. A forward step skips labels, so the session reads incomplete although no motion is missing; a backward step would give new buffers seconds already banked, and `append` drops them as duplicates | `BLEManager.swift` 5/MG handshake (SET_CLOCK on every connect); `ImuSessionFileStore.swift` `append` | Found in the 11.9.9 strap run. The relaunch's SET_CLOCK (value the second after the gap) skipped exactly the three labels before it; the delay from buffer second to arrival fell from 4.7 s to 1.86 s at that moment, and the strap's console counter ran about 3.1 s behind the phone between that set and the one 179 s earlier (its unit read as milliseconds, unconfirmed). The strap's own history has no buffer for those labels either. Caveat: one observation, and the delay held constant through the session, so steady drift does not explain a 3 s lag; the backward case is from the code. Needs an owner decision: set the clock only when GET_CLOCK shows it off (needs W06-001's decode), or record the step in the session and count coverage in strap time. Seen again on 11.9.10 (2026-09-25): the relaunch after W06-053's crash sent SET_CLOCK mid-session; the delay from buffer second to arrival fell from 5.8 s to 1.9 s across it, the four labels before it are missing from both the live stream and the strap's history, and the session read 113 of 117 s. The marker-free session with no relaunch read complete. Two observations now, both a forward step of about four seconds at a mid-session SET_CLOCK. Owner decision 2026-09-25: read the clock, then set it. Decode the GET_CLOCK reply (W06-001), send GET_CLOCK first, and send SET_CLOCK only when the strap clock is invalid or off by more than about two seconds. Its own PR with W06-001, after the reply layout is confirmed on the strap. V0 (2026-09-26, 13 owner strap logs, 20 SET/GET pairs on 50.39.1.0): the GET_CLOCK readback equals the SET value exactly in all 20, including the four whose replies came about four seconds after the send, so the strap applies a value stamped seconds earlier. Both observed forward steps follow such a late set: the set before the 11.9.9 session was answered about 4 s late and the relaunch set 179 s later was prompt; on 11.9.10 the first relaunch's set was answered 4 s late and the next relaunch's set, prompt, stepped the clock by the 3.9 s the IMU delay fell. So the step is the correction of an earlier stale set, and reading first removes both: a strap within 2 s is not set at all. Confirmed; reclassified from one observation to a mechanism | | | W06-051 | S2 | Reproduced | The app process ended three times in about four and a half minutes (before the session, mid-session, and just after Stop), each time relaunched by CoreBluetooth state restoration; the log shows nothing before any of them | iOS app | Found in the 11.9.9 strap run: three "previous app session" generations, each ending without an error line and followed by "Restored CONNECTED peripheral". State restoration relaunches only an app the system ended, so this was not a force-quit. Cause unknown until the iPhone's crash or jetsam reports for those minutes are read; a crash on real data would be S1. V0 (2026-09-25, the 11.9.10 strap run): the mid-session death on 11.9.9 came 3.5 s after a marker was added, and the 11.9.10 death that the install does not explain came under 2 s after one; that crash is W06-053. A marker-free session of about two minutes on 11.9.10 had no death. The other two 11.9.9 deaths are not attributed: the log does not record the marker sheet, and a Cancel crashes the same way as a Save. V3 with W06-053: a strap run with markers and no death closes this row with it; a death without a marker needs the iPhone's crash reports | | | W06-052 | S4 | Fixed | After a relaunch during a session, the Raw IMU fail-safe takes the session's own stream for a stray producer and asks for a stop: `rawCaptureInFlight` belongs to the process, while the session is persisted and only the collector view re-arms it | `BLEManager.swift` `stopUnexpectedRealtimeImu`; `RawDataCollectorView.swift` re-arm | Found in the 11.9.9 strap run: twice, "Raw IMU fail-safe: unexpected realtime packet type 43 while capture was off; stop requested", each dropped only because the command characteristic was not yet discovered; W06-018's allowlist would drop it afterwards. The line claims a stray producer the app itself started. Seen again on 11.9.10 after W06-053's relaunch mid-session: the same line, dropped the same way. Fixed with W06-018 on `review/w06-fixes-5`: a session still open on disk for the strap owns the stream (`ImuSessionFileStore.hasOpenWindow`). V0: `testASessionStillOpenOnDiskOwnsTheStream` on the old code logged the fail-safe line and formed both stop commands; it passes after. That test's manager already has its collector, so it models the frames after the store is built, not the relaunch's first frames, which arrive before it: W06-054 completes the fix. V3 due: a relaunch mid-session logs no Raw IMU line | | | W06-053 | S1 | Fixed | Closing the Raw Data Collector's marker sheet ends the app, by Save or by Cancel: the sheet edits `markerDraft` through `Binding($markerDraft)`, which force-unwraps on every read, and SwiftUI reads it again while the sheet closes, after the close has set the draft to nil | `RawDataCollectorView.swift` marker sheet | Found through W06-051 and the 11.9.10 strap run (2026-09-25): both marker saves on record, one on 11.9.9 and one on 11.9.10, were followed by an app death 2 to 4 s later, and a marker-free session on 11.9.10 had none. V0: a simulator app running the sheet's code verbatim, opening it as Add marker does and closing it as Save or Cancel does, crashed by the second close in 3 of 3 runs (two with Save, one with Cancel), in `BindingOperations.ForceUnwrapping.get`, the unwrap that `Binding($markerDraft)` installs. The 11.9.10 death lost the three seconds of IMU still held in memory; they came back only because a history sync ran after Stop and the strap's v21 history still held them. Fix: the sheet is its own view and edits its own `@State` copy of the draft; Save, Cancel and Delete hand the draft back through closures, so nothing unwraps `markerDraft` once it is nil. V1: the same harness running the fixed sheet, copied from the file, survives 39 closes (12 saves, 12 cancels, and 15 mixed with deletes) with the right marker count; `OptionalBindingUnwrapTests` finds no `Binding($` in the app sources and fails at the old line with the fix reverted; `StrandTests` (1,992) pass and Strand and NOOPiOS (no watch) build. V3 pending: markers added, edited, cancelled and deleted during a strap session, with no app death | | From 9e219b6b29bb3b8693c1912c4b94d86cdf3d906d Mon Sep 17 00:00:00 2001 From: Artur Iskakov Date: Sat, 26 Sep 2026 12:23:03 +0200 Subject: [PATCH 02/38] fix(ble): read the 5/MG strap clock before setting it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit W06-050 — every connect sent SET_CLOCK. The strap applies the value when it processes the command, which on a busy relaunch came about 4 s after the phone stamped it, so a correct clock was left seconds behind. The next prompt set stepped it forward, and during a Raw Data Collector session that step skipped IMU labels, so the session read incomplete with no motion missing. - Severity: S3 · Category: bug · Upstream-able: yes - Location: `Strand/BLE/BLEManager.swift` 5/MG handshake; `Packages/WhoopProtocol/.../StrapClock.swift` `judge`, `Check` - V0: in all 20 SET/GET pairs in the owner's strap logs, the GET_CLOCK readback equals the SET value exactly, including the four answered about four seconds late. Both observed forward steps (11.9.9 and 11.9.10) follow such a late set; the prompt set that corrected it moved the IMU arrival delay by the same 3 to 4 s. - Fix (the owner's 2026-09-25 decision): the handshake sends GET_CLOCK only. `StrapClock.judge` asks for a SET_CLOCK only when the reply is refused, reads at or below the validity floor, or puts the clock more than 2 s off across the whole request round trip; a long round trip never sets by itself. With no reply in 10 s the clock is set without a reading, as before. The handshake counts as done (`connectHandshakeDone`, `historyReady`, `connectSettled`) when the check settles, so a set still precedes the first offload and the alarm re-arm. The stale-record copy now says NOOP checks the clock every connect and sets a wrong one. - W06-022: the dead `ClockPolicy` is deleted; `StrapClock` is the live policy. - V1: 19 `StrapClockTests`. Five mutations (always set, which is the old behaviour; ignore the request sequence; expire after settling; ignore the round trip; floor at zero) each fail the suite. `StrandTests` (2,013) and both app builds pass (NOOPiOS without the watch app). - V3: needs a strap run. --- .../Sources/WhoopProtocol/StrapClock.swift | 160 +++++++++++++++++- .../WhoopProtocolTests/StrapClockTests.swift | 156 ++++++++++++++++- Strand/BLE/BLEManager.swift | 143 ++++++++++++---- Strand/Collect/Backfiller.swift | 15 +- Strand/Collect/ClockPolicy.swift | 12 -- StrandTests/BackfillerSessionTallyTests.swift | 5 +- Tools/linux-capture/README.md | 2 +- Tools/linux-capture/whoop_frame.py | 4 +- Tools/linux-capture/whoop_setclock.py | 4 +- Tools/linux-capture/whoop_sync.py | 2 +- docs/ARCHITECTURE.md | 1 - docs/BLE_REVERSE_ENGINEERING.md | 3 +- docs/CONTRIBUTING.md | 5 +- docs/PROTOCOL_IMPLEMENTATION.md | 17 +- docs/review/workstreams/W06-ble-collect.md | 6 +- 15 files changed, 460 insertions(+), 75 deletions(-) delete mode 100644 Strand/Collect/ClockPolicy.swift diff --git a/Packages/WhoopProtocol/Sources/WhoopProtocol/StrapClock.swift b/Packages/WhoopProtocol/Sources/WhoopProtocol/StrapClock.swift index 1fbe8c3ef0..3074a98f53 100644 --- a/Packages/WhoopProtocol/Sources/WhoopProtocol/StrapClock.swift +++ b/Packages/WhoopProtocol/Sources/WhoopProtocol/StrapClock.swift @@ -1,8 +1,32 @@ import Foundation -/// The WHOOP 5/MG strap clock as GET_CLOCK (11) reports it (W06-001). +/// The WHOOP 5/MG strap clock as GET_CLOCK (11) reports it, and whether a connect sets it (W06-050). +/// +/// Every connect used to send SET_CLOCK and then GET_CLOCK. The strap applies a SET_CLOCK value when it +/// processes the command, and on a busy link (a state-restoration relaunch) that came about four seconds +/// after the phone stamped the value, so the strap then ran that far behind. The next prompt set stepped it +/// forward again, and when that set came from a relaunch during a Raw Data Collector session the IMU +/// timeline skipped the seconds of the step. Reading first and setting only a clock that is invalid or +/// provably more than `driftThresholdSeconds` off removes both steps for a strap whose clock is fine. public enum StrapClock { + /// Seconds at or below this are not a wall clock. The strap applies a stored time only above it + /// (`docs/PROTOCOL_TRANSPORT.md` §Clock and identity contracts), and a failed read can report zero with + /// a success result, so a reading here needs a set whatever the result byte says. + public static let validityFloor: UInt32 = 1_293_840_001 + + /// How far the strap clock may sit from the phone's before a connect sets it. + public static let driftThresholdSeconds: Double = 2 + + /// How long a connect waits for the GET_CLOCK reply before setting the clock without one. Replies on a + /// busy relaunch came about four seconds after the request; a firmware that does not serve GET_CLOCK + /// never answers, and its strap must still be clocked, because an un-clocked WHOOP 5 banks no sensor data. + public static let replyTimeoutSeconds: Double = 10 + + /// The COMMAND_RESPONSE result that carries a reading. Pending (2) promises a later answer. + public static let resultSuccess: UInt8 = 1 + public static let resultPending: UInt8 = 2 + /// One GET_CLOCK reply. public struct Reading: Equatable, Sendable { /// The sequence of the request this answers (format-1 byte 11), which ties the reply to one read. @@ -34,4 +58,138 @@ public enum StrapClock { } return Reading(originSequence: frame[11], result: frame[12], seconds: seconds) } + + /// What one reading says about the strap clock, as the offset strap − phone in seconds. + public enum Verdict: Equatable, Sendable { + /// No reading: the result was not success, or the reply carried no seconds. + case unread(result: UInt8) + /// A reading at or below `validityFloor`. + case invalid(seconds: UInt32) + /// The offset lies within the threshold either way. + case inSync(seconds: UInt32, low: Double, high: Double) + /// The offset may pass the threshold, but the round trip was too long to show that it does. + case unresolved(seconds: UInt32, low: Double, high: Double) + /// The offset lies wholly beyond the threshold. + case off(seconds: UInt32, low: Double, high: Double) + + /// Whether a connect should set the clock on this verdict. + public var needsSet: Bool { + switch self { + case .unread, .invalid, .off: return true + case .inSync, .unresolved: return false + } + } + } + + /// Judge a reading from a GET_CLOCK sent at `sentAt` and answered at `receivedAt`, both phone Unix + /// seconds. The strap read its clock at some moment in that window, and a whole-second reading `s` means + /// its clock stood in [s, s + 1) then. So the offset strap − phone lies in (s − receivedAt, s + 1 − sentAt), + /// and the clock is judged off only when that whole range lies past the threshold. A long round trip + /// widens the range and so never produces a set by itself. + public static func judge(_ reading: Reading, sentAt: Double, receivedAt: Double, + threshold: Double = driftThresholdSeconds) -> Verdict { + guard reading.result == resultSuccess, let s = reading.seconds else { return .unread(result: reading.result) } + guard s > validityFloor else { return .invalid(seconds: s) } + let low = Double(s) - max(receivedAt, sentAt) + let high = Double(s) + 1 - sentAt + if high < -threshold || low > threshold { return .off(seconds: s, low: low, high: high) } + if low >= -threshold && high <= threshold { return .inSync(seconds: s, low: low, high: high) } + return .unresolved(seconds: s, low: low, high: high) + } + + /// One connection's clock check: one read, then a set only when its verdict needs one. + public struct Check: Equatable, Sendable { + + /// A GET_CLOCK in flight: its request sequence and when it was sent. + public struct Request: Equatable, Sendable { + public let sequence: UInt8 + public let sentAt: Double + } + + /// The read that decides this connection; nil once it has been answered or has expired. + public private(set) var read: Request? + /// The GET_CLOCK sent after a set, so its reply is recognised and reported rather than judged. + public private(set) var readback: Request? + /// True once the check has decided, by a reply or by the timeout. + public private(set) var settled = false + + public init() {} + + /// What a reply means for this connection. + public enum Step: Equatable, Sendable { + /// Answers no request in flight: a late or foreign reply, reported and otherwise ignored. + case notOurs + /// The strap acknowledged the read and will answer later. + case pending + /// The read is answered and the clock is left alone. + case settle(Verdict, roundTrip: Double) + /// The read is answered and the clock needs a set. + case set(Verdict, roundTrip: Double) + /// The reply to the readback after a set; `roundTrip` counts from the set. + case readback(Reading, roundTrip: Double) + } + + /// A read went out with request sequence `sequence` at `at`. + public mutating func beginRead(sequence: UInt8, at: Double) { + guard !settled else { return } + read = Request(sequence: sequence, sentAt: at) + } + + /// A readback went out after a set. + public mutating func beginReadback(sequence: UInt8, at: Double) { + readback = Request(sequence: sequence, sentAt: at) + } + + /// Fold in one decoded reply received at `at`. + public mutating func receive(_ reading: Reading, at: Double) -> Step { + if let r = read, !settled, reading.originSequence == r.sequence { + if reading.result == resultPending { return .pending } + read = nil + settled = true + let verdict = judge(reading, sentAt: r.sentAt, receivedAt: at) + let roundTrip = at - r.sentAt + return verdict.needsSet ? .set(verdict, roundTrip: roundTrip) : .settle(verdict, roundTrip: roundTrip) + } + if let r = readback, reading.originSequence == r.sequence { + if reading.result == resultPending { return .pending } + readback = nil + return .readback(reading, roundTrip: at - r.sentAt) + } + return .notOurs + } + + /// The read went unanswered for `replyTimeoutSeconds`. True when this settles the check, and the + /// caller then sets the clock without a reading; false when a reply already settled it. + public mutating func expire() -> Bool { + guard !settled else { return false } + settled = true + read = nil + return true + } + } + + /// The log line for a verdict, stating the reading and the range it allows rather than a single offset + /// the reply cannot establish. + public static func describe(_ verdict: Verdict, roundTrip: Double) -> String { + let rt = String(format: "%.1f", max(0, roundTrip)) + switch verdict { + case .unread(let result): + return "GET_CLOCK answered result \(result) with no reading — setting the clock" + case .invalid(let s): + return "strap clock reads \(s), not a wall clock (at or below \(validityFloor)) — setting it" + case let .inSync(s, low, high): + return "strap clock reads \(s), \(range(low, high)) from the phone over a \(rt) s round trip — " + + "within \(Int(driftThresholdSeconds)) s, not set" + case let .unresolved(s, low, high): + return "strap clock reads \(s), \(range(low, high)) from the phone over a \(rt) s round trip — " + + "that does not show it more than \(Int(driftThresholdSeconds)) s off, not set" + case let .off(s, low, high): + return "strap clock reads \(s), \(range(low, high)) from the phone over a \(rt) s round trip — " + + "more than \(Int(driftThresholdSeconds)) s off, setting it" + } + } + + private static func range(_ low: Double, _ high: Double) -> String { + String(format: "%+.1f…%+.1f s", low, high) + } } diff --git a/Packages/WhoopProtocol/Tests/WhoopProtocolTests/StrapClockTests.swift b/Packages/WhoopProtocol/Tests/WhoopProtocolTests/StrapClockTests.swift index b330114e71..9b7cc8f6f7 100644 --- a/Packages/WhoopProtocol/Tests/WhoopProtocolTests/StrapClockTests.swift +++ b/Packages/WhoopProtocol/Tests/WhoopProtocolTests/StrapClockTests.swift @@ -1,9 +1,9 @@ import XCTest @testable import WhoopProtocol -/// W06-001: the GET_CLOCK reply decode. The three GET_CLOCK frames and the SET_CLOCK frame are real replies -/// from one WHOOP MG on firmware 50.39.1.0; the failure reply is built with `puffinCommandFrame`, since no -/// strap log holds one. +/// W06-050 and W06-001: the GET_CLOCK reply decode and the read-first clock policy. The three GET_CLOCK +/// frames and the SET_CLOCK frame are real replies from one WHOOP MG on firmware 50.39.1.0; the failure and +/// pending replies are built with `puffinCommandFrame`, since no strap log holds one. final class StrapClockTests: XCTestCase { private func bytes(_ s: String) -> [UInt8] { @@ -56,4 +56,154 @@ final class StrapClockTests: XCTestCase { let frame = reply(cmd: 11, origin: 7, result: 1) + u32(1_790_000_000) XCTAssertEqual(StrapClock.decodeReply(frame)?.seconds, nil) } + + // MARK: judge + + private let t0 = 1_790_000_000.25 // phone time the read was sent + + func testAClockWithinTheThresholdIsNotSet() { + let r = StrapClock.Reading(originSequence: 1, result: 1, seconds: 1_790_000_000) + let v = StrapClock.judge(r, sentAt: t0, receivedAt: t0 + 0.5) + XCTAssertEqual(v, .inSync(seconds: 1_790_000_000, low: -0.75, high: 0.75)) + XCTAssertFalse(v.needsSet) + } + + func testAClockProvablyBehindIsSet() { + // Four seconds behind, answered quickly: the whole range lies past −2 s. + let r = StrapClock.Reading(originSequence: 1, result: 1, seconds: 1_789_999_996) + let v = StrapClock.judge(r, sentAt: t0, receivedAt: t0 + 0.5) + XCTAssertEqual(v, .off(seconds: 1_789_999_996, low: -4.75, high: -3.25)) + XCTAssertTrue(v.needsSet) + } + + func testAClockProvablyAheadIsSet() { + let r = StrapClock.Reading(originSequence: 1, result: 1, seconds: 1_790_000_005) + XCTAssertEqual(StrapClock.judge(r, sentAt: t0, receivedAt: t0 + 1), + .off(seconds: 1_790_000_005, low: 3.75, high: 5.75)) + } + + func testALongRoundTripNeverSetsByItself() { + // The relaunch shape: the reply came four seconds after the request and reads the second it was + // sent. The strap may have read its clock at once (in sync) or four seconds later (four behind). + let r = StrapClock.Reading(originSequence: 1, result: 1, seconds: 1_790_000_000) + let v = StrapClock.judge(r, sentAt: t0, receivedAt: t0 + 4) + XCTAssertEqual(v, .unresolved(seconds: 1_790_000_000, low: -4.25, high: 0.75)) + XCTAssertFalse(v.needsSet) + } + + func testAnExactlyTwoSecondRangeEdgeIsNotOff() { + // high == −2 is not past the threshold. + let r = StrapClock.Reading(originSequence: 1, result: 1, seconds: 1_789_999_997) + XCTAssertFalse(StrapClock.judge(r, sentAt: 1_790_000_000, receivedAt: 1_790_000_000.5).needsSet) + let r2 = StrapClock.Reading(originSequence: 1, result: 1, seconds: 1_789_999_996) + XCTAssertTrue(StrapClock.judge(r2, sentAt: 1_790_000_000, receivedAt: 1_790_000_000.5).needsSet) + } + + func testNoWallClockIsSetWhateverTheResult() { + for s: UInt32 in [0, 86_400, StrapClock.validityFloor] { + let v = StrapClock.judge(.init(originSequence: 1, result: 1, seconds: s), sentAt: t0, receivedAt: t0) + XCTAssertEqual(v, .invalid(seconds: s)) + XCTAssertTrue(v.needsSet) + } + // One second past the floor is a wall clock, and this one is decades off. + let past = StrapClock.judge(.init(originSequence: 1, result: 1, seconds: StrapClock.validityFloor + 1), + sentAt: t0, receivedAt: t0) + guard case .off = past else { return XCTFail("\(past)") } + } + + func testAReplyWithoutAReadingIsSet() { + let refused = StrapClock.judge(.init(originSequence: 1, result: 3, seconds: 1_790_000_000), + sentAt: t0, receivedAt: t0) + XCTAssertEqual(refused, .unread(result: 3)) + XCTAssertEqual(StrapClock.judge(.init(originSequence: 1, result: 1, seconds: nil), sentAt: t0, receivedAt: t0), + .unread(result: 1)) + XCTAssertTrue(refused.needsSet) + } + + func testAPhoneClockThatStepsBackMidReadDoesNotNarrowTheRange() { + let r = StrapClock.Reading(originSequence: 1, result: 1, seconds: 1_790_000_000) + XCTAssertEqual(StrapClock.judge(r, sentAt: t0, receivedAt: t0 - 3), + .inSync(seconds: 1_790_000_000, low: -0.25, high: 0.75)) + } + + // MARK: Check + + private let now = 1_790_000_000.0 + private func reading(_ origin: UInt8, result: UInt8 = 1, seconds: UInt32? = 1_790_000_000) -> StrapClock.Reading { + .init(originSequence: origin, result: result, seconds: seconds) + } + + func testAnInSyncReadSettlesWithoutASet() { + var check = StrapClock.Check() + check.beginRead(sequence: 9, at: now) + guard case let .settle(v, rt) = check.receive(reading(9), at: now + 0.5) else { return XCTFail() } + XCTAssertFalse(v.needsSet) + XCTAssertEqual(rt, 0.5) + XCTAssertTrue(check.settled) + XCTAssertFalse(check.expire(), "a settled check must not expire into a blind set") + } + + func testAnOffReadAsksForASetAndItsReadbackIsReportedNotJudged() { + var check = StrapClock.Check() + check.beginRead(sequence: 9, at: now) + guard case let .set(v, _) = check.receive(reading(9, seconds: 1_789_999_990), at: now + 0.2) else { + return XCTFail() + } + XCTAssertTrue(v.needsSet) + check.beginReadback(sequence: 11, at: now + 0.3) + XCTAssertEqual(check.receive(reading(11, seconds: 1_790_000_000), at: now + 1.3), + .readback(reading(11, seconds: 1_790_000_000), roundTrip: 1.0)) + // A second copy of the readback answers nothing in flight. + XCTAssertEqual(check.receive(reading(11), at: now + 2), .notOurs) + } + + func testPendingWaitsForTheAnswer() { + var check = StrapClock.Check() + check.beginRead(sequence: 9, at: now) + XCTAssertEqual(check.receive(reading(9, result: 2, seconds: nil), at: now + 0.1), .pending) + XCTAssertFalse(check.settled) + guard case .settle = check.receive(reading(9), at: now + 0.6) else { return XCTFail() } + check.beginReadback(sequence: 10, at: now + 1) + XCTAssertEqual(check.receive(reading(10, result: 2, seconds: nil), at: now + 1.1), .pending) + } + + func testAReplyToAnotherRequestIsNotOurs() { + var check = StrapClock.Check() + check.beginRead(sequence: 9, at: now) + XCTAssertEqual(check.receive(reading(8), at: now + 0.2), .notOurs) + XCTAssertFalse(check.settled) + } + + func testAnUnansweredReadExpiresOnceAndALateReplyChangesNothing() { + var check = StrapClock.Check() + check.beginRead(sequence: 9, at: now) + XCTAssertTrue(check.expire()) + XCTAssertFalse(check.expire()) + XCTAssertEqual(check.receive(reading(9, seconds: 1_789_000_000), at: now + 12), .notOurs) + check.beginRead(sequence: 12, at: now + 13) + XCTAssertNil(check.read, "a settled check starts no second read") + } + + func testAReadThatNeverWentOutStillExpires() { + var check = StrapClock.Check() + XCTAssertTrue(check.expire()) + } + + // MARK: describe + + func testLogLinesStateTheReadingAndTheRange() { + XCTAssertEqual(StrapClock.describe(.inSync(seconds: 1_790_000_000, low: -0.65, high: 0.75), roundTrip: 0.4), + "strap clock reads 1790000000, -0.7…+0.8 s from the phone over a 0.4 s round trip — " + + "within 2 s, not set") + XCTAssertEqual(StrapClock.describe(.unresolved(seconds: 1_790_000_000, low: -4.25, high: 0.75), roundTrip: 4), + "strap clock reads 1790000000, -4.2…+0.8 s from the phone over a 4.0 s round trip — " + + "that does not show it more than 2 s off, not set") + XCTAssertEqual(StrapClock.describe(.off(seconds: 1_789_999_996, low: -4.75, high: -3.25), roundTrip: 0.5), + "strap clock reads 1789999996, -4.8…-3.2 s from the phone over a 0.5 s round trip — " + + "more than 2 s off, setting it") + XCTAssertEqual(StrapClock.describe(.invalid(seconds: 0), roundTrip: 0.1), + "strap clock reads 0, not a wall clock (at or below 1293840001) — setting it") + XCTAssertEqual(StrapClock.describe(.unread(result: 3), roundTrip: 0.1), + "GET_CLOCK answered result 3 with no reading — setting the clock") + } } diff --git a/Strand/BLE/BLEManager.swift b/Strand/BLE/BLEManager.swift index 2c91c62c0a..ce898f5295 100644 --- a/Strand/BLE/BLEManager.swift +++ b/Strand/BLE/BLEManager.swift @@ -915,8 +915,8 @@ public final class BLEManager: NSObject, ObservableObject { /// deliver, and the subscribe loops skip anything already `isNotifying` — so nothing re-arms and no live /// HR/R-R arrive, while `didUpdateNotificationStateFor` never fires (→ `cmdNotifyConfirmedActive` → /// `connectSettled` never latch). While this is true, `requestNotify` forces one real off→on - /// re-subscribe so delivery — and the settle/alarm-re-arm chain — is re-established. Cleared the moment - /// `connectSettled` bumps, and on disconnect. + /// re-subscribe so delivery — and the settle/alarm-re-arm chain — is re-established. Cleared once that + /// forced pass has run (the 5/MG handshake, or `connectSettled` bumping on WHOOP 4), and on disconnect. private var restoreNeedsResubscribe = false /// Re-entrancy guard for captureRawAccel: true while a bounded on-demand window is running. /// A second tap is a no-op until the active capture's asyncAfter block fires and clears this. @@ -1111,6 +1111,12 @@ public final class BLEManager: NSObject, ObservableObject { private var clockRequested = false /// #700: retry count for GET_CLOCK when no correlation establishes before backfill. Capped at 3. private var clockRetries = 0 + /// W06-050: this link's read-first strap clock check (5/MG). Reset on disconnect. + private var strapClockCheck = StrapClock.Check() + /// Fences a previous link's clock-check timeout, so it can never settle the next link's handshake. + private var strapClockCheckToken = 0 + /// When this link's 5/MG handshake ran, so the first offload keeps its ~1.5 s settle delay after it. + private var whoop5HandshakeAt: Date? private var intentionalDisconnect = false /// Consecutive `didFailToConnect` count, for the auto-reconnect backoff (#414). Reset to 0 on a /// successful connect; grows the reschedule delay so a strap that's genuinely out of range doesn't @@ -6083,6 +6089,9 @@ extension BLEManager: @preconcurrency CBCentralManagerDelegate { whoop5SessionStarted = false clockRequested = false clockRetries = 0 + strapClockCheck = StrapClock.Check() + strapClockCheckToken &+= 1 + whoop5HandshakeAt = nil connectHandshakeDone = false // Mirrors the flag above: a new link has not done the handshake, so it cannot hand over history // until it does. Cleared HERE and nowhere else, so the two can never disagree. @@ -6679,41 +6688,19 @@ extension BLEManager: @preconcurrency CBPeripheralDelegate { // only this block is gated. `whoop5SessionStarted` resets on disconnect. if !whoop5SessionStarted { whoop5SessionStarted = true - connectHandshakeDone = true // unblocks beginBackfill()'s guard - state.historyReady = true // and the sync controls, which must not offer what it declines - log("WHOOP 5/MG: connect handshake done — backfill unblocked") + whoop5HandshakeAt = Date() noteRebootReconnectIfNeeded() // Re-apply the Broadcast-HR device-config flag if the user opted in (#181). if PuffinExperiment.broadcastHrEnabled { setBroadcastHr(true) } // Clock the strap BEFORE history: an un-clocked WHOOP 5 discards sensor data ("RTC // timestamp … is invalid; not saving data to flash") and history offloads "succeed" - // with metadata only. Same 8-byte payload as the WHOOP4 handshake, puffin-framed; - // GET_CLOCK's reply rides the puffin notify chars and never touches the WHOOP4 - // clockRef correlation path. The 1.5s deferral below keeps clock-before-history. - // Hardware-validated ordering (#78 fork). - send(.setClock, payload: BLEManager.setClockPayload()) - send(.getClock, payload: []) - // #1823: say what was SENT, not what resulted. This previously read "clock synced — - // strap can persist history now", logged before any reply had arrived, so a log could - // assert the clock was set while the Devices readout said 1970/71. The strap's actual - // answer now arrives as the "clock: …" ack line from FrameRouter. - log("WHOOP 5/MG: SET_CLOCK + GET_CLOCK sent — awaiting the strap's answer") - log("WHOOP 5/MG: scheduling first historical offload (connect)") - // Deferred ~1.5s so the puffin notify subscriptions settle before SEND_HISTORICAL_DATA, - // mirroring the WHOOP4 kick. requestSync → beginBackfill is itself gated on - // connectHandshakeDone, so a racing foreground/restore trigger can't fire it early. - DispatchQueue.main.asyncAfter(deadline: .now() + 1.5) { [weak self] in self?.requestConnectSync() } + // with metadata only (#78 fork). W06-050: read the clock first and set it only when the + // reading says it is wrong. `connectHandshakeDone`, which the first offload, the sync + // controls and the alarm re-arm all wait on, is set when this check settles + // (`strapClockSettled`), so a SET_CLOCK it sends still precedes all three. + beginStrapClockCheck() startBackfillTimer() // re-offload the type-47 store every backfillIntervalSeconds - // #34: signal settled directly (skip the cmd-notify gate `maybeSignalConnectSettled()` - // uses) — armStrapAlarm's 5/MG branch never sends GET_ALARM_TIME (log-only readback is - // WHOOP4-only, and the 5/MG alarm itself stays behind the Experimental toggle), so there's - // no reply-channel race to wait out here; this just keeps the re-arm-on-bond signal firing - // for 5/MG the way it did before `connectSettled` replaced raw `bonded`. - if !connectSettledSignaled { - connectSettledSignaled = true - state.connectSettled &+= 1 - restoreNeedsResubscribe = false // #613: forced re-subscribe pass is done (5/MG path) - } + restoreNeedsResubscribe = false // #613: forced re-subscribe pass is done (5/MG path) } return } @@ -6755,7 +6742,7 @@ extension BLEManager: @preconcurrency CBPeripheralDelegate { // ignores the empty form (#120). Send both — the strap answers whichever its firmware // accepts, and the `clockRef == nil` correlation guard makes a second reply a no-op. // Without the [0x00] form, correlation never establishes on 41.17.x, so a lost RTC (the - // 1971 clock behind #120) stays invisible and ClockPolicy can never re-fix it. Both + // 1971 clock behind #120) stays invisible and no drift check can re-fix it. Both // GET_CLOCKs ride behind both SET_CLOCKs above, so the reply reflects the corrected clock. // (Offload doesn't depend on this — Backfiller falls back to an identity clockRef — but a // real correlation drives realtime decode and the drift re-set.) @@ -6839,6 +6826,94 @@ extension BLEManager: @preconcurrency CBPeripheralDelegate { send(.setClock, payload: BLEManager.setClockPayload(now: now)) } + /// `send`, returning the request sequence the frame carried, or nil when `send` wrote nothing. A reply + /// names its request by that sequence (format-1 byte 11). + private func sendReturningSequence(_ command: WhoopCommand, payload: [UInt8]) -> UInt8? { + let before = seq + send(command, payload: payload) + return seq == before ? nil : seq + } + + /// W06-050: read the 5/MG strap clock. Its reply (`handleStrapClockReply`) or the timeout settles the + /// check, and a SET_CLOCK follows only when the reading is invalid or provably more than + /// `StrapClock.driftThresholdSeconds` off. A strap that never answers is set without a reading after + /// `StrapClock.replyTimeoutSeconds`, as every connect did before, since an un-clocked strap banks nothing. + private func beginStrapClockCheck() { + strapClockCheck = StrapClock.Check() + strapClockCheckToken &+= 1 + let token = strapClockCheckToken + if let sequence = sendReturningSequence(.getClock, payload: []) { + strapClockCheck.beginRead(sequence: sequence, at: Date().timeIntervalSince1970) + } + // #1823: say what was SENT, not what resulted; the verdict line follows the strap's answer. + log("WHOOP 5/MG: GET_CLOCK sent — SET_CLOCK follows only if the strap clock is invalid or more than " + + "\(Int(StrapClock.driftThresholdSeconds)) s off") + DispatchQueue.main.asyncAfter(deadline: .now() + StrapClock.replyTimeoutSeconds) { [weak self] in + guard let self, token == self.strapClockCheckToken, self.strapClockCheck.expire() else { return } + self.log("WHOOP 5/MG clock: no GET_CLOCK reply within \(Int(StrapClock.replyTimeoutSeconds)) s — " + + "setting the clock without a reading") + self.setStrapClock() + self.strapClockSettled() + } + } + + /// W06-050: fold one GET_CLOCK reply into this link's clock check and act on its verdict. Verified first, + /// since the verdict can send a SET_CLOCK. + private func handleStrapClockReply(_ frame: [UInt8]) { + guard verifyFrame(frame, family: .whoop5).ok, let reading = StrapClock.decodeReply(frame) else { return } + switch strapClockCheck.receive(reading, at: Date().timeIntervalSince1970) { + case .notOurs: + log("WHOOP 5/MG clock: a GET_CLOCK reply to request \(reading.originSequence) answers no read in " + + "flight — ignored") + case .pending: + log("WHOOP 5/MG clock: GET_CLOCK answered PENDING — waiting for the reading") + case let .settle(verdict, roundTrip): + log("WHOOP 5/MG clock: " + StrapClock.describe(verdict, roundTrip: roundTrip)) + strapClockSettled() + case let .set(verdict, roundTrip): + log("WHOOP 5/MG clock: " + StrapClock.describe(verdict, roundTrip: roundTrip)) + setStrapClock() + strapClockSettled() + case let .readback(r, roundTrip): + let reads = r.seconds.map { "reads \($0)" } ?? "gave no reading" + log("WHOOP 5/MG clock: after the set the strap \(reads) (result \(r.result)), answered " + + String(format: "%.1f", roundTrip) + " s after the set was sent") + } + } + + /// Set the 5/MG strap clock to the phone's and read it back, so the log shows what latched. + private func setStrapClock() { + send(.setClock, payload: BLEManager.setClockPayload()) + if let sequence = sendReturningSequence(.getClock, payload: []) { + strapClockCheck.beginReadback(sequence: sequence, at: Date().timeIntervalSince1970) + } + } + + /// The 5/MG clock check has decided, so the connect handshake is done. The first offload, the sync + /// controls and the alarm re-arm unblock here, after any SET_CLOCK the check sent. + private func strapClockSettled() { + connectHandshakeDone = true // unblocks beginBackfill()'s guard + state.historyReady = true // and the sync controls, which must not offer what it declines + log("WHOOP 5/MG: connect handshake done — backfill unblocked") + log("WHOOP 5/MG: scheduling first historical offload (connect)") + // ~1.5 s after the handshake, so the puffin notify subscriptions settle before SEND_HISTORICAL_DATA, + // mirroring the WHOOP4 kick. requestSync → beginBackfill is itself gated on connectHandshakeDone, so a + // racing foreground/restore trigger can't fire it early. + let elapsed = whoop5HandshakeAt.map { Date().timeIntervalSince($0) } ?? 0 + DispatchQueue.main.asyncAfter(deadline: .now() + max(0, 1.5 - elapsed)) { [weak self] in + self?.requestConnectSync() + } + // #34: signal settled directly (skip the cmd-notify gate `maybeSignalConnectSettled()` uses) — + // armStrapAlarm's 5/MG branch never sends GET_ALARM_TIME (log-only readback is WHOOP4-only, and the + // 5/MG alarm itself stays behind the Experimental toggle), so there's no reply-channel race to wait + // out here; this just keeps the re-arm-on-bond signal firing for 5/MG the way it did before + // `connectSettled` replaced raw `bonded`. + if !connectSettledSignaled { + connectSettledSignaled = true + state.connectSettled &+= 1 + } + } + /// Newest plausible-unix marker in a GET_DATA_RANGE COMMAND_RESPONSE = the strap's newest stored /// record. Mirrors re/diagnose_biometrics.py: scan u32 LE words in the response body (data starts at /// frame[7], after [type,seq,cmd]), keep those in the unix range, return the max. nil if none. @@ -7190,6 +7265,10 @@ extension BLEManager: @preconcurrency CBPeripheralDelegate { handleBroadcastHrGateReadBack(frame, isWhoop5: true) } } + // W06-050: a GET_CLOCK reply decides whether this link sets the strap clock. Verified inside. + if frame.count > 10, frame[8] == 0x24, frame[10] == WhoopCommand.getClock.rawValue { + handleStrapClockReply(frame) + } // #695: a 5/MG GET_DATA_RANGE COMMAND_RESPONSE (puffin envelope: type @8, cmd @10). Feeds // the SAME newest/oldest window + backfill gate + diagnostics as the 4.0 path above — this // reply was previously ignored on 5/MG (the 4.0 handler keyed on frame[6]). cmdOff = 10. diff --git a/Strand/Collect/Backfiller.swift b/Strand/Collect/Backfiller.swift index 2fb937ffc0..792a5d51f1 100644 --- a/Strand/Collect/Backfiller.swift +++ b/Strand/Collect/Backfiller.swift @@ -529,13 +529,14 @@ final class Backfiller { /// number is unremarkable. Asserting a corrupt RTC here would claim more than the data supports, which /// is how this area has misled people before. /// - /// It also says the part the existing advice omits: NOOP re-sends SET_CLOCK on every connect, so - /// "charge it" alone has already been retried every session. + /// It also says the part the existing advice omits: NOOP reads the strap clock on every connect and + /// sets it when it is invalid or off (W06-050), so "charge it" alone has already been retried every + /// session. /// /// Byte-identical to the Android twin. No em-dash (project rule). nonisolated static func staleRecordLine(newestUnix: Int, wallNowUnix: Int) -> String { let ageDays = max(0, wallNowUnix - newestUnix) / 86_400 - return "Backfill: this sync banked nothing and the strap's newest stored record is about \(ageDays) day(s) old. If you have worn it since then, it has stopped saving history to its flash. NOOP already re-sends the clock on every connect, so charging alone may not be enough: charge to 100% and reconnect, then use Restart strap in Devices, and if that does not help forget and re-pair. If the official WHOOP app is also missing these days, the strap is the cause and not NOOP." + return "Backfill: this sync banked nothing and the strap's newest stored record is about \(ageDays) day(s) old. If you have worn it since then, it has stopped saving history to its flash. NOOP already checks the strap clock on every connect and sets it when it is wrong, so charging alone may not be enough: charge to 100% and reconnect, then use Restart strap in Devices, and if that does not help forget and re-pair. If the official WHOOP app is also missing these days, the strap is the cause and not NOOP." } @@ -544,15 +545,15 @@ final class Backfiller { /// The standing banner says "fully charge it to 100%, then reconnect, and it should start banking /// again". It omits the one fact that makes the situation legible - how long the strap has been /// silent - and it PROMISES a recovery that has already failed every session for weeks, because NOOP - /// re-sends SET_CLOCK on every connect and the charge advice has therefore been retried all along. A - /// banner that keeps promising something that keeps not happening teaches people to distrust the app - /// rather than their strap. + /// checks the clock on every connect and sets a wrong one, so the charge advice has been retried all + /// along. A banner that keeps promising something that keeps not happening teaches people to distrust + /// the app rather than their strap. /// /// Byte-identical to the Android twin. Not localized, matching the sibling `lastSyncError` copy on /// both platforms; localizing that surface is its own change. No em-dash (project rule). nonisolated static func staleRecordBanner(newestUnix: Int, wallNowUnix: Int) -> String { let ageDays = max(0, wallNowUnix - newestUnix) / 86_400 - return "Synced, but your strap handed over no stored history, and its newest saved record is about \(ageDays) day(s) old. If you have been wearing it since then, it has stopped saving to flash. Charge it to 100% and reconnect; NOOP already re-sets its clock every connect, so if that does not help, try Restart strap in Devices, then forget and re-pair. If the official WHOOP app is missing these days too, the strap is the cause and not NOOP." + return "Synced, but your strap handed over no stored history, and its newest saved record is about \(ageDays) day(s) old. If you have been wearing it since then, it has stopped saving to flash. Charge it to 100% and reconnect; NOOP already checks its clock every connect and sets it when it is wrong, so if that does not help, try Restart strap in Devices, then forget and re-pair. If the official WHOOP app is missing these days too, the strap is the cause and not NOOP." } /// #1754: the banner for an empty offload whose flash cursor is VALID and ADVANCING — the strap is diff --git a/Strand/Collect/ClockPolicy.swift b/Strand/Collect/ClockPolicy.swift deleted file mode 100644 index e500afe75b..0000000000 --- a/Strand/Collect/ClockPolicy.swift +++ /dev/null @@ -1,12 +0,0 @@ -import Foundation - -/// Pure decision: should we SET_CLOCK on this connect? Mirrors WHOOP — set only when the strap's RTC -/// has drifted beyond a small threshold (or is frozen/way off), not blindly every connect. Avoids -/// gratuitous clock resets that could introduce discontinuities in derived historical timestamps. -enum ClockPolicy { - /// `deviceClock` = the strap's current RTC reading (unix seconds, from GET_CLOCK). `wallNow` = - /// phone wall time (unix seconds). Returns true if |drift| >= threshold. - static func shouldSetClock(deviceClock: Int, wallNow: Int, driftThreshold: Int = 2) -> Bool { - abs(wallNow - deviceClock) >= driftThreshold - } -} diff --git a/StrandTests/BackfillerSessionTallyTests.swift b/StrandTests/BackfillerSessionTallyTests.swift index 13f00e6596..19e59f76b0 100644 --- a/StrandTests/BackfillerSessionTallyTests.swift +++ b/StrandTests/BackfillerSessionTallyTests.swift @@ -279,8 +279,9 @@ final class BackfillerSessionTallyTests: XCTestCase { let line = Backfiller.staleRecordLine(newestUnix: 1_785_692_420, wallNowUnix: 1_787_820_941) XCTAssertTrue(line.contains("about 24 day(s) old"), line) XCTAssertTrue(line.contains("stopped saving history"), line) - // The part the old advice omitted: charging alone has already been retried every connect. - XCTAssertTrue(line.contains("re-sends the clock on every connect"), line) + // The part the old advice omitted: charging alone has already been retried every connect. Since + // W06-050 a connect sets the clock only when its reading is wrong, and the copy says so. + XCTAssertTrue(line.contains("checks the strap clock on every connect and sets it when it is wrong"), line) // The test that tells the user whether NOOP is even involved. XCTAssertTrue(line.contains("official WHOOP app"), line) XCTAssertFalse(line.contains("\u{2014}")) diff --git a/Tools/linux-capture/README.md b/Tools/linux-capture/README.md index 6d74346b27..df5d9d66a9 100644 --- a/Tools/linux-capture/README.md +++ b/Tools/linux-capture/README.md @@ -197,7 +197,7 @@ Exit codes: `0` accepted, `2` strap not found/reachable, `3` connected but no bu A strap left offline (no app) for a long time loses its clock — its RTC drifts or resets, so realtime and newly-recorded historical frames get **bogus timestamps** (e.g. dated 1971). The phone app fixes this with `SET_CLOCK` on every connect; this does the same from Linux. It **reads the clock first and -only writes if it has drifted** past `--if-drift` seconds (mirrors the app's `ClockPolicy` — no +only writes if it has drifted** past `--if-drift` seconds (mirrors the app's `StrapClock` — no gratuitous resets), then re-reads to verify the new time latched. ```bash diff --git a/Tools/linux-capture/whoop_frame.py b/Tools/linux-capture/whoop_frame.py index bae2bf78b5..fa9f3e714b 100644 --- a/Tools/linux-capture/whoop_frame.py +++ b/Tools/linux-capture/whoop_frame.py @@ -178,8 +178,8 @@ def buzz_frame(family: str, seq: int = 0) -> bytes: # --- Clock ---------------------------------------------------------------------------------------- -# SET_CLOCK (command 10) sets the strap RTC. The phone app sends this on every connect when the strap's -# clock has drifted (ClockPolicy). Needed here because a strap left offline for months loses its clock. +# SET_CLOCK (command 10) sets the strap RTC. The phone app sends it on connect only when GET_CLOCK shows +# the strap's clock invalid or drifted (StrapClock in WhoopProtocol). Needed here because a strap left offline for months loses its clock. CMD_SET_CLOCK = 10 diff --git a/Tools/linux-capture/whoop_setclock.py b/Tools/linux-capture/whoop_setclock.py index d52eb659d5..da147e6ef6 100644 --- a/Tools/linux-capture/whoop_setclock.py +++ b/Tools/linux-capture/whoop_setclock.py @@ -12,7 +12,7 @@ Phone Bluetooth must be OFF and the strap bonded + advertising. It reads the strap clock first and only writes when it has drifted past `--if-drift` seconds (mirrors -the app's ClockPolicy — avoid gratuitous resets), then re-reads to verify the new clock latched. +the app's StrapClock — avoid gratuitous resets), then re-reads to verify the new clock latched. Usage: python3 whoop_setclock.py --model whoop4 --address AA:BB:CC:DD:EE:FF # set to now @@ -111,7 +111,7 @@ async def run(args) -> int: async with BleakClient(dev) as client: print(f"connected: {client.is_connected}") - # 1. READ the strap clock first (so we only write when needed — mirrors the app's ClockPolicy). + # 1. READ the strap clock first (so we only write when needed — mirrors the app's StrapClock). before = await read_strap_clock(client, cfg, args.model) wall = int(time.time()) if before is None: diff --git a/Tools/linux-capture/whoop_sync.py b/Tools/linux-capture/whoop_sync.py index eb33b25314..b63ae51f0b 100644 --- a/Tools/linux-capture/whoop_sync.py +++ b/Tools/linux-capture/whoop_sync.py @@ -407,7 +407,7 @@ async def _acquire(address, tries=6): async def preflight_clock(fam, args): """Before syncing, read the strap RTC and set it to now if it has drifted past - `args.clock_threshold` seconds — mirrors the app's ClockPolicy (only write on real drift, so we + `args.clock_threshold` seconds — mirrors the app's StrapClock (only write on real drift, so we don't gratuitously reset). A strap left offline for months loses its clock, which would otherwise mis-date every captured/recorded frame. diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 138e21e2fd..911f51e8bf 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -79,7 +79,6 @@ Strand/ macOS SwiftUI app target (the reference implemen │ ├── Collector.swift buffers live frames → decoded-first persistence │ ├── Backfiller.swift historical-offload state machine (safe-trim) │ ├── ClockCorrelation.swift device-epoch ↔ wall-clock correlation (pure) -│ ├── ClockPolicy.swift when to (re)issue SET_CLOCK │ ├── StorePaths.swift on-disk SQLite location (App Support/OpenWhoop) │ ├── PrunePolicy.swift raw-outbox retention (24h / 50MB) │ └── RawCaptureWindow.swift bounded on-demand raw-capture window diff --git a/docs/BLE_REVERSE_ENGINEERING.md b/docs/BLE_REVERSE_ENGINEERING.md index ed3e015fac..dc6fc837c1 100644 --- a/docs/BLE_REVERSE_ENGINEERING.md +++ b/docs/BLE_REVERSE_ENGINEERING.md @@ -480,7 +480,8 @@ stub payloads on this firmware (so the firmware version lives in the `GET_HELLO` `GET_CLOCK` (11) isn't served at all. Record dating does not need it, since realtime (type-40) and historical (type-47) both carry real unix rather than a device epoch. Firmware 50.39.1.0 does serve it: twenty replies carry the format-1 result at `frame[12]` and the strap's unix seconds as a u32 LE at -`frame[13]`, then seven zero bytes (`StrapClock.decodeReply`). +`frame[13]`, then seven zero bytes. NOOP reads it on connect and sets the clock only when the reading is +invalid or more than 2 s off (`StrapClock`). > **Privacy.** The `GET_HELLO` response also contains a **session token**, which the decoder never > reads or exposes — only the device name and firmware version are surfaced. The `device_name`/ diff --git a/docs/CONTRIBUTING.md b/docs/CONTRIBUTING.md index 834326484c..e22ba88042 100644 --- a/docs/CONTRIBUTING.md +++ b/docs/CONTRIBUTING.md @@ -444,8 +444,9 @@ load-bearing and was hardened against real failure modes that are documented in realtime-raw flood that dominated flash). Treat it as stable infrastructure: - **Don't reorder the connect handshake.** Offload is deliberately gated on - `connectHandshakeDone`; `SET_CLOCK` (cmd 10) must precede arming the firmware alarm so the strap - RTC is UTC-correct. + `connectHandshakeDone`; a `SET_CLOCK` (cmd 10) must precede arming the firmware alarm so the strap + RTC is UTC-correct. On WHOOP 5/MG the handshake reads the clock first and sets it only when the + reading is invalid or more than 2 s off; `connectHandshakeDone` waits for that check. - **Don't `ENTER` high-frequency sync.** The app no longer enters it and sends `exitHighFreqSync` defensively on connect to release straps parked there by older builds. - **Prefer `.withoutResponse` writes** (the `send(_:payload:writeType:)` default); use diff --git a/docs/PROTOCOL_IMPLEMENTATION.md b/docs/PROTOCOL_IMPLEMENTATION.md index 355e1b0d66..ef140ee2f4 100644 --- a/docs/PROTOCOL_IMPLEMENTATION.md +++ b/docs/PROTOCOL_IMPLEMENTATION.md @@ -47,7 +47,8 @@ supported connection. |---|---|---| | [Curated sender enum](PROTOCOL_COMMANDS.md#canonical-command-matrix) | [WhoopCommand](../Strand/BLE/Commands.swift) | Sender surface is intentionally smaller than the decode catalogue | | [WHOOP 5 command builder](PROTOCOL_TRANSPORT.md#format-1-framing) | [puffinCommandFrame(cmd:seq:payload:type:header:)](../Packages/WhoopProtocol/Sources/WhoopProtocol/Framing.swift) | Pads the inner record before checksums | -| [Clock 8-byte form](PROTOCOL_COMMANDS.md#whoop-5mg) | [setClockPayload(now:)](../Strand/BLE/BLEManager.swift) | WHOOP 5/MG sends the eight-byte form once per connection and reads the clock back | +| [Clock 8-byte form](PROTOCOL_COMMANDS.md#whoop-5mg) | [setClockPayload(now:)](../Strand/BLE/BLEManager.swift) | WHOOP 5/MG sends the eight-byte form only when the connect's clock reading calls for it, then reads the clock back | +| [Clock reading](PROTOCOL_TRANSPORT.md#responses-and-correlation) | [StrapClock.decodeReply(_:)](../Packages/WhoopProtocol/Sources/WhoopProtocol/StrapClock.swift), [StrapClock.judge(_:sentAt:receivedAt:threshold:)](../Packages/WhoopProtocol/Sources/WhoopProtocol/StrapClock.swift) | GET_CLOCK's u32 seconds follow the format-1 result; a set follows only an invalid reading or one more than 2 s off across the whole round trip | | [Alarm 9-byte body](PROTOCOL_ALARMS.md#whoop-5mg) | [WhoopCommand.setAlarmPayload(epochSec:)](../Strand/BLE/Commands.swift) | Two trailing bytes remain explicit | | [Haptic preset](PROTOCOL_ALARMS.md#whoop-5mg) | [MaverickHaptics.notificationBuzz(loops:)](../Packages/WhoopProtocol/Sources/WhoopProtocol/HapticPayloads.swift) | The common request is remapped to the 5/MG family body | | [Wrist and ECG controls](PROTOCOL_ECG.md#commands-and-independent-output-gates) | [Whoop5Ecg.selectWristPayload(_:)](../Packages/WhoopProtocol/Sources/WhoopProtocol/Whoop5Ecg.swift), [Whoop5Ecg.togglePayload(on:)](../Packages/WhoopProtocol/Sources/WhoopProtocol/Whoop5Ecg.swift) | MG capability gate is separate from framing | @@ -158,9 +159,14 @@ not protocol requirements. every later confirmed write, and re-blasting the handshake mid-offload was the historical root cause of the strap refusing to stream type-47. - The handshake re-arms the puffin notify subscriptions, arms realtime HR only if - a screen asked for it, sends `SET_CLOCK` in the eight-byte form followed by - `GET_CLOCK` for readback, and schedules the first historical offload about 1.5 s - later so the link settles first. + a screen asked for it, and reads the strap clock with `GET_CLOCK`. It sends + `SET_CLOCK` in the eight-byte form, followed by a `GET_CLOCK` readback, only when + the reading is invalid, refused, or more than 2 s from the phone's clock over the + whole request round trip, or when no reply comes within 10 s. The strap applies a + set value when it processes the command, which on a busy link can be seconds after + the phone stamped it, so an unconditional set left a correct clock seconds behind. + The handshake counts as done once that check settles: the first historical offload + (about 1.5 s after the handshake at the earliest) and the alarm re-arm wait for it. - A 15-minute backfill timer (`backfillIntervalSeconds`, matching WHOOP) and a 30-second keep-alive timer (`keepAliveIntervalSeconds`: re-arm realtime, poll battery, watchdog the link) are then started. With Low refresh enabled the @@ -406,7 +412,8 @@ the two-byte selector `[0x01, 0x01]`. Stop uses `STOP_RAW_DATA` (82) `[0x01]`, t bytes are not evaluated. A seven-byte request was acknowledged in one run but did not produce the scheduled vibration; the subsecond field, not body length, is semantic. - `BLEManager.setClockPayload(now:)` → `[secs u32 LE][0,0,0,0]` (8 bytes). WHOOP 5/MG - sends this form once per connection and follows it with `GET_CLOCK` for readback. + sends this form only when the connect's `GET_CLOCK` reading calls for a set, and + follows it with `GET_CLOCK` for readback. - `BLEManager.setClockPayloadLegacy(now:)` → `[secs u32 LE][0,0,0,0,0]` (9 bytes) is the legacy WHOOP 4.0 firmware form; it remains in code for reference and is not part of the 5/MG send sequence. diff --git a/docs/review/workstreams/W06-ble-collect.md b/docs/review/workstreams/W06-ble-collect.md index b2fcd13f13..830fc0c277 100644 --- a/docs/review/workstreams/W06-ble-collect.md +++ b/docs/review/workstreams/W06-ble-collect.md @@ -27,7 +27,7 @@ The only CoreBluetooth surface, plus live collection and historical offload. `St | `Strand/Collect/Backfiller.swift` | `finishChunk`, `setCursor("strap_trim", …)` | Historical chunk assembly and the durable trim cursor | | `Strand/Collect/Collector.swift` | `flush` | Live batch ingest | | `Strand/Collect/RawHistoryArchive.swift` | archive | Unmapped layouts archived for later decode | -| `Strand/Collect/PrunePolicy.swift`, `ClockPolicy.swift`, `StorePaths.swift` | policies | Prune window, clock handling, file locations | +| `Strand/Collect/PrunePolicy.swift`, `StorePaths.swift`; `Packages/WhoopProtocol/…/StrapClock.swift` | policies | Prune window, file locations; the 5/MG read-first clock check (W06-050) | Tests: `StrandTests` (23 files match BLE, bond, hello, backfill, offload, trim, collector or raw history); app tests only run under `xcodebuild … test`. @@ -96,7 +96,7 @@ exactly what was tested on hardware. | W06-019 | S4 | Reported | The 5/MG "Clock latched" readout says "no (records dated 1970/71)" after any decoded offload: a sentinel `StrapRange(newestUnix: 0)` is read as a date, while `rtcWarning` on the same card ignores 0 | `LiveState.swift` `setStrapFirmwareLayout`; `ConnectionReadout.swift` `clockLatchedLabel` | P5 confirmed. Two readouts of one fact disagree on the Devices card | | | W06-020 | S4 | Reported | `ARCHITECTURE.md` §4–5, AD-5 and in-code comments describe a live/historical split the 5/MG code does not have; the Collector's puffin ingest, flush and clock path have no caller | `docs/ARCHITECTURE.md`; `BLEManager.swift`; `Collector.swift` | P5 confirmed. Feeds the AD-5 amendment and W12. W06-002's V2 extends the list: `captureRawAccel` has no caller, so `beginRawCapture` and `endRawCapture` are unreachable and the live half of `enableRawCapture` never persists on 5/MG; `bufferedCount` has no reader | | | W06-021 | S4 | Reported | `StuckStrapDetector` is inert on 5/MG: `strapNewestTs` is never set, `strapNeedsReboot` has no reader, and its recovery sends a command the 5/MG allowlist drops | `StuckStrapDetector.swift`; `BLEManager.swift` `checkStrapLiveness` | P5 confirmed. Dead code; fork-only | | -| W06-022 | S4 | Reported | `ClockPolicy.shouldSetClock` has had no caller since a5ad278c, and its doc contradicts the 5/MG path, which sets the clock on every connect | `ClockPolicy.swift` | P5 confirmed. Dead code; fork-only | | +| W06-022 | S4 | Fixed | `ClockPolicy.shouldSetClock` has had no caller since a5ad278c, and its doc contradicts the 5/MG path, which sets the clock on every connect | `ClockPolicy.swift` | P5 confirmed. Dead code; fork-only. Fixed with W06-050: the file is deleted, and `StrapClock` in WhoopProtocol is the policy the handshake calls | | | W06-023 | S4 | Reported | AD-5 Amend: the architecture doc and AD-5 describe the split the code has; `isOffloadFrame` is pinned by a test; EVENT side effects gate on event age; the drain carries a generation token | `BLEManager.swift` frame loop; `docs/ARCHITECTURE.md` §4–5 | Design row for the AD-5 verdict ([DECISIONS.md](../DECISIONS.md#ad-5)); the fixes are W06-008, W06-009, W06-013, W06-020 | | | W06-024 | S3 | Reported | AD-6 Amend: restate safe-trim as "every record the strap will free is in a durable lane or the archive before the ack"; no layout is mapped without a lane; the archive never evicts records NOOP understands | `Backfiller.swift` `finishChunk`; `HistoricalStreams.swift` `rejectedHistoricalRecords`; `RawHistoryArchive.swift` | Design row for the AD-6 verdict ([DECISIONS.md](../DECISIONS.md#ad-6)); covers W01-003, W01-004, W01-006, W06-003, W06-010, W06-011, W06-016. Hold-ack tests (W06-007) first | | | W06-025 | S3 | Fixed | A live Raw Data Collector session loses its last seconds, and every session reads "incomplete": Stop sends STOP_RAW_DATA at once, and the strap discards the buffers it has not produced yet | `RawDataCollectorView.swift` `stop`; `BLEManager.swift` `stopGroundTruthRawCapture` | Found in W06-002's V3. V0, 11.9.8 session (counts only): 148 of 152 requested seconds banked; every buffer arrived 5.67–5.73 s after its own timestamp, the last one 0.14 s before Stop; the strap read its clock back as set 23 s before the session; its own v21 history of the session ends at the same second as the live stream, so no sync can return the tail. The stop log line also says "+ flushed" before any flush. Fix: the session end is stamped when Stop is pressed, and the stream stops once the last full second is banked, after at most 10 s, or when the strap disconnects (`RawSessionTail.wait`); the log names which ended the wait and no longer claims a flush; the button reads Stopping… meanwhile. V1: with the wait forced to give up at its first check, 3 of the 6 `RawSessionTailTests` fail (the waits for the last second, for a stall and for a disconnect); the other three, an immediate delivery, a nothing-banked outcome and the log text, hold for such a wait too (rechecked under W06-046). V3 pending: a session on the iPhone must read ready. V3, the 11.9.9 strap run (2026-09-25; a session of about three minutes, one marker, an app relaunch mid-session): the last full second was banked and the stop line read "stopped after the last full second was banked"; the session still read incomplete, 175 of 178 s, because of a strap clock step (W06-050), not the tail. The 11.9.10 strap run (2026-09-25): a marker-free session of about two minutes, with no relaunch, read ready, 122 of 122 s, and its stop line read "stopped after the last full second was banked". V3 passes | | @@ -124,7 +124,7 @@ exactly what was tested on hardware. | W06-047 | S4 | Fixed | Every banked IMU buffer builds and configures a `DateFormatter` on the main actor to name its segment file | `ImuSessionFileStore.swift` `segmentFile`, `utcName` | Hot-path cost of the same order as the decode W06-031 removed. V0 (oracle, -O on an M-series Mac): 45.5 µs a call against 1.2 µs with one formatter, same output; so worse than the 8 µs decode W06-031 removed. Fix: one formatter, built once. Refactor check: `ImuSessionFileStoreTests.testSegmentsAreNamedForTheirUTCHalfHour` pins the name on disk and in the export, and passes before and after | | | W06-048 | S4 | Reported | The per-segment `seen` sets kept since W06-026 are never evicted for closed windows; only `remove(id:)` prunes them | `ImuSessionFileStore.swift` `seen` | A week-long historical window re-delivered after a relaunch keeps 336 segment sets | | | W06-049 | S4 | Fixed | `segmentScans` is state the production store carries only so one test can read it | `ImuSessionFileStore.swift` | Cleanup. Fix: the store takes its file reader as a parameter; W06-026's test counts reads through it and now states the property itself, 49 more duplicates reading no file again, and still fails with W06-026's fix reverted | | -| W06-050 | S3 | Reproduced | A SET_CLOCK during a live Raw Data Collector session steps the strap clock, and the IMU timeline with it. A forward step skips labels, so the session reads incomplete although no motion is missing; a backward step would give new buffers seconds already banked, and `append` drops them as duplicates | `BLEManager.swift` 5/MG handshake (SET_CLOCK on every connect); `ImuSessionFileStore.swift` `append` | Found in the 11.9.9 strap run. The relaunch's SET_CLOCK (value the second after the gap) skipped exactly the three labels before it; the delay from buffer second to arrival fell from 4.7 s to 1.86 s at that moment, and the strap's console counter ran about 3.1 s behind the phone between that set and the one 179 s earlier (its unit read as milliseconds, unconfirmed). The strap's own history has no buffer for those labels either. Caveat: one observation, and the delay held constant through the session, so steady drift does not explain a 3 s lag; the backward case is from the code. Needs an owner decision: set the clock only when GET_CLOCK shows it off (needs W06-001's decode), or record the step in the session and count coverage in strap time. Seen again on 11.9.10 (2026-09-25): the relaunch after W06-053's crash sent SET_CLOCK mid-session; the delay from buffer second to arrival fell from 5.8 s to 1.9 s across it, the four labels before it are missing from both the live stream and the strap's history, and the session read 113 of 117 s. The marker-free session with no relaunch read complete. Two observations now, both a forward step of about four seconds at a mid-session SET_CLOCK. Owner decision 2026-09-25: read the clock, then set it. Decode the GET_CLOCK reply (W06-001), send GET_CLOCK first, and send SET_CLOCK only when the strap clock is invalid or off by more than about two seconds. Its own PR with W06-001, after the reply layout is confirmed on the strap. V0 (2026-09-26, 13 owner strap logs, 20 SET/GET pairs on 50.39.1.0): the GET_CLOCK readback equals the SET value exactly in all 20, including the four whose replies came about four seconds after the send, so the strap applies a value stamped seconds earlier. Both observed forward steps follow such a late set: the set before the 11.9.9 session was answered about 4 s late and the relaunch set 179 s later was prompt; on 11.9.10 the first relaunch's set was answered 4 s late and the next relaunch's set, prompt, stepped the clock by the 3.9 s the IMU delay fell. So the step is the correction of an earlier stale set, and reading first removes both: a strap within 2 s is not set at all. Confirmed; reclassified from one observation to a mechanism | | +| W06-050 | S3 | Fixed | A SET_CLOCK during a live Raw Data Collector session steps the strap clock, and the IMU timeline with it. A forward step skips labels, so the session reads incomplete although no motion is missing; a backward step would give new buffers seconds already banked, and `append` drops them as duplicates | `BLEManager.swift` 5/MG handshake (SET_CLOCK on every connect); `ImuSessionFileStore.swift` `append` | Found in the 11.9.9 strap run. The relaunch's SET_CLOCK (value the second after the gap) skipped exactly the three labels before it; the delay from buffer second to arrival fell from 4.7 s to 1.86 s at that moment, and the strap's console counter ran about 3.1 s behind the phone between that set and the one 179 s earlier (its unit read as milliseconds, unconfirmed). The strap's own history has no buffer for those labels either. Caveat: one observation, and the delay held constant through the session, so steady drift does not explain a 3 s lag; the backward case is from the code. Needs an owner decision: set the clock only when GET_CLOCK shows it off (needs W06-001's decode), or record the step in the session and count coverage in strap time. Seen again on 11.9.10 (2026-09-25): the relaunch after W06-053's crash sent SET_CLOCK mid-session; the delay from buffer second to arrival fell from 5.8 s to 1.9 s across it, the four labels before it are missing from both the live stream and the strap's history, and the session read 113 of 117 s. The marker-free session with no relaunch read complete. Two observations now, both a forward step of about four seconds at a mid-session SET_CLOCK. Owner decision 2026-09-25: read the clock, then set it. Decode the GET_CLOCK reply (W06-001), send GET_CLOCK first, and send SET_CLOCK only when the strap clock is invalid or off by more than about two seconds. Its own PR with W06-001, after the reply layout is confirmed on the strap. V0 (2026-09-26, 13 owner strap logs, 20 SET/GET pairs on 50.39.1.0): the GET_CLOCK readback equals the SET value exactly in all 20, including the four whose replies came about four seconds after the send, so the strap applies a value stamped seconds earlier. Both observed forward steps follow such a late set: the set before the 11.9.9 session was answered about 4 s late and the relaunch set 179 s later was prompt; on 11.9.10 the first relaunch's set was answered 4 s late and the next relaunch's set, prompt, stepped the clock by the 3.9 s the IMU delay fell. So the step is the correction of an earlier stale set, and reading first removes both: a strap within 2 s is not set at all. Confirmed; reclassified from one observation to a mechanism. Fixed: the 5/MG handshake sends GET_CLOCK only, and `StrapClock.judge` asks for a SET_CLOCK only when the reading is refused, at or below the validity floor, or more than 2 s off across the whole request round trip (a long round trip never sets by itself); with no reply in 10 s it sets the clock without a reading, as before. `connectHandshakeDone`, which the first offload, the sync controls and the alarm re-arm wait on, is set when the check settles, so a set still precedes history and the alarm. The stale-record copy said NOOP re-sets the clock every connect; it now says NOOP checks it and sets a wrong one. V1: 19 `StrapClockTests`; five mutations (always set, which is the old behaviour; ignore the request sequence; expire after settling; ignore the round trip; floor at zero) each fail the suite. Both app builds pass. V3 needs a strap run: a connect logs a "within 2 s, not set" line and no SET_CLOCK, and a relaunch during a Raw Data Collector session skips no IMU labels | | | W06-051 | S2 | Reproduced | The app process ended three times in about four and a half minutes (before the session, mid-session, and just after Stop), each time relaunched by CoreBluetooth state restoration; the log shows nothing before any of them | iOS app | Found in the 11.9.9 strap run: three "previous app session" generations, each ending without an error line and followed by "Restored CONNECTED peripheral". State restoration relaunches only an app the system ended, so this was not a force-quit. Cause unknown until the iPhone's crash or jetsam reports for those minutes are read; a crash on real data would be S1. V0 (2026-09-25, the 11.9.10 strap run): the mid-session death on 11.9.9 came 3.5 s after a marker was added, and the 11.9.10 death that the install does not explain came under 2 s after one; that crash is W06-053. A marker-free session of about two minutes on 11.9.10 had no death. The other two 11.9.9 deaths are not attributed: the log does not record the marker sheet, and a Cancel crashes the same way as a Save. V3 with W06-053: a strap run with markers and no death closes this row with it; a death without a marker needs the iPhone's crash reports | | | W06-052 | S4 | Fixed | After a relaunch during a session, the Raw IMU fail-safe takes the session's own stream for a stray producer and asks for a stop: `rawCaptureInFlight` belongs to the process, while the session is persisted and only the collector view re-arms it | `BLEManager.swift` `stopUnexpectedRealtimeImu`; `RawDataCollectorView.swift` re-arm | Found in the 11.9.9 strap run: twice, "Raw IMU fail-safe: unexpected realtime packet type 43 while capture was off; stop requested", each dropped only because the command characteristic was not yet discovered; W06-018's allowlist would drop it afterwards. The line claims a stray producer the app itself started. Seen again on 11.9.10 after W06-053's relaunch mid-session: the same line, dropped the same way. Fixed with W06-018 on `review/w06-fixes-5`: a session still open on disk for the strap owns the stream (`ImuSessionFileStore.hasOpenWindow`). V0: `testASessionStillOpenOnDiskOwnsTheStream` on the old code logged the fail-safe line and formed both stop commands; it passes after. That test's manager already has its collector, so it models the frames after the store is built, not the relaunch's first frames, which arrive before it: W06-054 completes the fix. V3 due: a relaunch mid-session logs no Raw IMU line | | | W06-053 | S1 | Fixed | Closing the Raw Data Collector's marker sheet ends the app, by Save or by Cancel: the sheet edits `markerDraft` through `Binding($markerDraft)`, which force-unwraps on every read, and SwiftUI reads it again while the sheet closes, after the close has set the draft to nil | `RawDataCollectorView.swift` marker sheet | Found through W06-051 and the 11.9.10 strap run (2026-09-25): both marker saves on record, one on 11.9.9 and one on 11.9.10, were followed by an app death 2 to 4 s later, and a marker-free session on 11.9.10 had none. V0: a simulator app running the sheet's code verbatim, opening it as Add marker does and closing it as Save or Cancel does, crashed by the second close in 3 of 3 runs (two with Save, one with Cancel), in `BindingOperations.ForceUnwrapping.get`, the unwrap that `Binding($markerDraft)` installs. The 11.9.10 death lost the three seconds of IMU still held in memory; they came back only because a history sync ran after Stop and the strap's v21 history still held them. Fix: the sheet is its own view and edits its own `@State` copy of the draft; Save, Cancel and Delete hand the draft back through closures, so nothing unwraps `markerDraft` once it is nil. V1: the same harness running the fixed sheet, copied from the file, survives 39 closes (12 saves, 12 cancels, and 15 mixed with deletes) with the right marker count; `OptionalBindingUnwrapTests` finds no `Binding($` in the app sources and fails at the old line with the fix reverted; `StrandTests` (1,992) pass and Strand and NOOPiOS (no watch) build. V3 pending: markers added, edited, cancelled and deleted during a strap session, with no app death | | From 5c842b9fa0d0d5c20807fcbcc7c59f62834613d0 Mon Sep 17 00:00:00 2001 From: Artur Iskakov Date: Sat, 26 Sep 2026 12:23:17 +0200 Subject: [PATCH 03/38] fix(ble): read a command response's payload up to its declared length MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit W06-065 — `FrameRouter.commandResponsePayload` took the declared length from bytes 1–2 instead of 2–3. On every real 5/MG reply the computed end lay past the frame, so it returned nil, and the Test Centre GET_HELLO identity probe (#1303), its only caller, never printed. - Severity: S4 · Category: truth · Upstream-able: yes - Location: `Strand/BLE/FrameRouter.swift` `commandResponsePayload` - V0: the helper, run verbatim on a real GET_CLOCK reply (28 bytes, declared length 20), reads length 5121 and returns nil; on a SET_CLOCK reply, 3073 and nil. None of the 13 owner strap logs has a #1303 line, though they carry Test Centre connection lines and decoded hellos. No other site reads a 5/MG length there. - Fix: the payload ends at the CRC32 trailer, 4 bytes past the declared length. - V1: `FrameRouterCommandPayloadTests` (two real replies, two short frames) pass; with the old offset all three fail. - V3: a connect with Test Centre → Connection on logs the #1303 line. --- Strand/BLE/FrameRouter.swift | 15 ++++---- .../FrameRouterCommandPayloadTests.swift | 36 +++++++++++++++++++ docs/review/README.md | 4 +++ docs/review/workstreams/W06-ble-collect.md | 8 +++++ 4 files changed, 56 insertions(+), 7 deletions(-) create mode 100644 StrandTests/FrameRouterCommandPayloadTests.swift diff --git a/Strand/BLE/FrameRouter.swift b/Strand/BLE/FrameRouter.swift index e777f75ef3..23a46c1c66 100644 --- a/Strand/BLE/FrameRouter.swift +++ b/Strand/BLE/FrameRouter.swift @@ -380,16 +380,17 @@ public final class FrameRouter { } /// The payload of a COMMAND_RESPONSE: the bytes after [type,seq,cmd,origin_seq,result] (payload - /// starts at inner+5) up to the crc32 trailer at `length`. nil when the frame is too short to carry - /// any payload. + /// starts at inner+5) up to the crc32 trailer, zero padding included. The trailer starts 4 bytes past + /// the declared length, the u16 LE at bytes 2–3 (W06-065: reading bytes 1–2 put it past every real + /// frame). nil when the frame is too short to carry any payload. nonisolated static func commandResponsePayload(in frame: [UInt8], family: DeviceFamily = .whoop5) -> [UInt8]? { _ = family - guard frame.count > 2 else { return nil } - let length = Int(frame[1]) | (Int(frame[2]) << 8) // crc32 starts here - let start = innerOffset + 5 // skip type,seq,cmd,origin_seq,result - guard length <= frame.count, start < length else { return nil } - return Array(frame[start..= 4 else { return nil } + let bodyEnd = 4 + (Int(frame[2]) | (Int(frame[3]) << 8)) // crc32 starts here + let start = innerOffset + 5 // skip type,seq,cmd,origin_seq,result + guard bodyEnd <= frame.count, start < bodyEnd else { return nil } + return Array(frame[start.. [UInt8] { + stride(from: 0, to: s.count, by: 2).map { i in + let a = s.index(s.startIndex, offsetBy: i) + return UInt8(s[a...s.index(a, offsetBy: 1)], radix: 16)! + } + } + + func testARealGetClockReplyYieldsItsBodyUpToTheTrailer() { + let frame = bytes("aa011400010021b124230be80146aaaf6a0000000000000014cbbb6c") + XCTAssertEqual(FrameRouter.commandResponsePayload(in: frame), + [0x46, 0xaa, 0xaf, 0x6a, 0, 0, 0, 0, 0, 0, 0]) + XCTAssertEqual(FrameRouter.commandResponsePayloadHex(in: frame), "46 aa af 6a 00 00 00 00 00 00 00") + } + + func testARealSetClockReplyYieldsItsPadding() { + let frame = bytes("aa010c000100271124220ae701000000bcc4efc3") + XCTAssertEqual(FrameRouter.commandResponsePayload(in: frame), [0, 0, 0]) + } + + func testAFrameShorterThanItsDeclaredLengthHasNoPayload() { + let frame = bytes("aa011400010021b124230be80146aaaf6a0000000000000014cbbb6c") + XCTAssertNil(FrameRouter.commandResponsePayload(in: Array(frame.dropLast(5)))) + XCTAssertNil(FrameRouter.commandResponsePayload(in: [0xAA, 0x01, 0x14])) + } +} diff --git a/docs/review/README.md b/docs/review/README.md index 53a4dac3f7..4bfc9d4bc8 100644 --- a/docs/review/README.md +++ b/docs/review/README.md @@ -82,6 +82,10 @@ item as it lands. missed the relaunch's first frames) and the rest are fixed on the same branch, except W06-057 (not a bug). Owner decisions 2026-09-25: W06-050 reads the clock before setting it, in its own PR with W06-001; W06-052 keeps an honest line rather than none. Then a strap run: a relaunch mid-session logs no Raw IMU line, and an MG ECG session that brings a type-43 stream logs one line naming it. +- [ ] W06-050 with W06-001, `review/w06-clock-read-first`: the 5/MG handshake reads the strap clock and sets it + only when the reading is refused, invalid or more than 2 s off (the owner's 2026-09-25 decision); W06-022 and + W06-065 ride along. Then a strap run: a plain connect sends no SET_CLOCK, a relaunch during a Raw Data + Collector session skips no IMU label, and a Test Centre connect logs the #1303 line. - [ ] Batch check (METHOD step 6) for the first three batches: suites and both builds pass on 11.9.8; the day and night on the strap with 11.9.8 is due before their findings move to `Verified`. - [x] Owner decisions, 2026-09-25: W02-004 deferred to Phase 5, W02-007 to Phase 4, W02-009 to Phase 3; diff --git a/docs/review/workstreams/W06-ble-collect.md b/docs/review/workstreams/W06-ble-collect.md index 830fc0c277..57e9cd4b53 100644 --- a/docs/review/workstreams/W06-ble-collect.md +++ b/docs/review/workstreams/W06-ble-collect.md @@ -139,6 +139,7 @@ exactly what was tested on hardware. | W06-062 | S4 | Fixed | The note's tests read the host app's `enableRawCapture`, so they fail where the research toggle is on, and cover none of the guard's branches | `StrandTests/CollectorImuBankingTests.swift` | Found by the batch's code review. Fix: `BLEManager.continuousRawCapture` is the manager's one read of the toggle, and the tests set it. `RealtimeRawOwnerTests` pins every input combination and the per-stream rules from a standalone run; `RealtimeRawStreamNoteTests` drives the relaunch, an open session, continuous capture, and an armed capture with its tail through the frame loop. Mutation run: making the store check `?? false`, dropping the armed flag, and letting an unknown owner end a stream each fail a test | | | W06-063 | S4 | Fixed | Every hold-ack test builds the Backfiller with raw capture on and follows each chunk with an empty END, so the default ordering (raw capture off) and a chunk with records after a stall are not pinned | `StrandTests/BackfillerHoldAckTests.swift` | Found by the batch's code review. V0, two mutations of `finishChunk`, each passing all seven hold-ack tests: no cursor or ack unless raw capture is on, and a stall that holds only empty ENDs. Fixed (tests only): `testWithRawCaptureOffAChunkIsAckedAfterItsRowsArchiveAndCursor` pins the default order, and `testAChunkStoredDuringAStallIsNotAcked` (raw capture off and on; the spy can now fail a step once) pins that a chunk whose rows land after a failed one is still not acked. Each mutation fails one of them | | | W06-064 | S4 | Fixed | Five rows ran old and new text together without a sentence break, and W06-052's V0 said a manager with its collector modelled the relaunch | `docs/review/workstreams/W06-ble-collect.md` | Found by the batch's code review. Fixed in this file: the breaks are in (W06-007, W06-018, W06-041, W06-050, W06-052), and W06-052's row says what its test models | | +| W06-065 | S4 | Fixed | `FrameRouter.commandResponsePayload` reads the declared length from bytes 1–2 instead of 2–3, so on every real 5/MG reply the end it computes is past the frame and it returns nil; the Test Centre GET_HELLO identity probe (#1303), its only caller, has never printed | `FrameRouter.swift` `commandResponsePayload` | Found while decoding W06-001. V0: the helper, run verbatim on a real GET_CLOCK reply (28 bytes, declared length 20) reads length 5121 and returns nil; on a SET_CLOCK reply, 3073 and nil. The 13 owner strap logs carry Test Centre connection lines and decoded hellos, and none has a #1303 line. Same pattern swept: no other site reads a 5/MG length there. Fixed: the helper reads the declared length at bytes 2–3 and ends the payload at the CRC32 trailer. V1: `FrameRouterCommandPayloadTests` (two real replies, two short frames) pass, and with the old offset all three fail. V3: a connect with Test Centre → Connection on logs the #1303 line | | ## Log @@ -198,3 +199,10 @@ exactly what was tested on hardware. mutation that passed the old suite). `StrandTests` (2,010) and both app builds pass; one run hit the timing-dependent control recorded as W11-002. Every finding from the batch's code review is now Fixed or Not a bug. Next: push and a PR on the owner's word, then the batch's strap run. +- 2026-09-26 — W06-050 with W06-001 on `review/w06-clock-read-first` (off main), with W06-022 and W06-065. V0 on the + 13 owner strap logs: every GET_CLOCK readback equals the SET value, even when the reply came about four seconds + later, so both forward steps were corrections of an earlier stale set. The handshake now reads the clock and sets + it only when the reading is refused, invalid, or more than 2 s off across the round trip; the handshake counts as + done when that check settles. W06-065 (found while decoding) revives the #1303 hello probe. `WhoopProtocol` tests, + both app builds and `StrandTests` pass. Next: V2 by an independent reviewer, then a strap run: a plain connect, a + relaunch during a Raw Data Collector session, and a connect with Test Centre → Connection on. From 4a77cbbe530973f5c4bb590683c8ed0c1e852fdd Mon Sep 17 00:00:00 2001 From: Artur Iskakov Date: Sat, 26 Sep 2026 12:53:07 +0200 Subject: [PATCH 04/38] fix(ble): run the hello probe on the answer, at the decoder's name offset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit W06-066 — W06-065 revived the Test Centre GET_HELLO probe (#1303) with two faults. It fired on the PENDING acknowledgement, whose all-zero block reports "none", which the probe reads as "no ASCII serial in this block". And it labelled the device name at offset 16, the decoder's coordinate (payload from frame byte 11), while this payload starts at byte 13, so the name sits at 14. - Severity: S4 · Category: truth · Upstream-able: yes - Location: `Strand/BLE/FrameRouter.swift` GET_HELLO probe - V0: found by the batch's V2 review with an oracle over the old and new helpers; the owner's logs hold 15 PENDING hello replies. - Fix: the probe runs only on a SUCCESS reply and passes `knownNameOffset: 14`. - V1: two `FrameRouterCommandPayloadTests` on synthetic replies pass, and both fail with the gate and the offset reverted. W06-065's row now says that two of its three tests, not all three, fail with the old offset. --- Strand/BLE/FrameRouter.swift | 9 ++++- .../FrameRouterCommandPayloadTests.swift | 39 +++++++++++++++++++ docs/review/workstreams/W06-ble-collect.md | 3 +- 3 files changed, 49 insertions(+), 2 deletions(-) diff --git a/Strand/BLE/FrameRouter.swift b/Strand/BLE/FrameRouter.swift index 23a46c1c66..21281d0f0d 100644 --- a/Strand/BLE/FrameRouter.swift +++ b/Strand/BLE/FrameRouter.swift @@ -198,11 +198,18 @@ public final class FrameRouter { // deliberately never reads, so `HelloIdentityProbe` prints only serial-shaped runs and // withholds the rest. Test Centre → Connection gated on top of that, so nothing here reaches a // default (shareable) strap log. Log-only; decodes and persists nothing. + // + // W06-066: only a SUCCESS reply carries the block; the PENDING acknowledgement that precedes it + // is all zeros and would report "none", which the probe reads as "no ASCII serial here". And this + // payload starts after the result byte (frame byte 13), two bytes later than the decoder's (frame + // byte 11), so the name the decoder reads at its offset 16 sits at 14 here. if let cmd = parsed.cmdName, cmd.hasPrefix("GET_HELLO("), // Schema appends the raw value, e.g. "(145)" + parsed.parsed["result"]?.stringValue?.hasPrefix("SUCCESS") == true, TestCentre.active(.connection), let pay = Self.commandResponsePayload(in: frame, family: family) { - state.append(log: HelloIdentityProbe.report(payload: pay) + " — locate the strap serial (#1303)") + state.append(log: HelloIdentityProbe.report(payload: pay, knownNameOffset: 14) + + " — locate the strap serial (#1303)") } // The 5/MG battery pack (cmd 151). `BatteryPackInfo` has decoded this reply since its offsets // were captured, and until now nothing sent the command — so the decoder had no caller and the diff --git a/StrandTests/FrameRouterCommandPayloadTests.swift b/StrandTests/FrameRouterCommandPayloadTests.swift index 6cce82126c..737440d71b 100644 --- a/StrandTests/FrameRouterCommandPayloadTests.swift +++ b/StrandTests/FrameRouterCommandPayloadTests.swift @@ -28,6 +28,45 @@ final class FrameRouterCommandPayloadTests: XCTestCase { XCTAssertEqual(FrameRouter.commandResponsePayload(in: frame), [0, 0, 0]) } + // MARK: - W06-066: the #1303 hello probe W06-065 revived + + /// A GET_HELLO reply shaped like the real 5/MG block: origin sequence and result, then a body in which + /// the decoder's offset 16 (frame byte 27) holds the device name and a serial-shaped run sits later. + /// Synthetic, so no real name, serial or session token enters the repository. + private func helloReply(result: UInt8) -> [UInt8] { + var body = [UInt8](repeating: 0, count: 110) + if result == 1 { + for (i, c) in "WHOOP-FAKE01".utf8.enumerated() { body[14 + i] = c } + for (i, c) in "3A1B2405003655".utf8.enumerated() { body[40 + i] = c } + } + return puffinCommandFrame(cmd: 145, seq: 0x6d, payload: [0x01, result] + body, type: 36, + header: [0x01, 0x00]) + } + + private func probeLines(after frame: [UInt8]) -> [String] { + TestCentre.activate(.connection) + defer { TestCentre.deactivate(.connection) } + let live = LiveState() + FrameRouter(state: live).handle(frame: frame) + return live.log.filter { $0.contains("#1303") } + } + + func testTheHelloProbeLabelsTheNameWhereTheDecoderReadsIt() { + let frame = helloReply(result: 1) + XCTAssertEqual(parseFrame(frame, family: .whoop5).parsed["device_name"]?.stringValue, "WHOOP-FAKE01", + "the fixture puts the name where the decoder reads it") + let lines = probeLines(after: frame) + XCTAssertEqual(lines.count, 1) + XCTAssertTrue(lines.first?.contains("off=14 len=12 mixed (device name, already decoded)") == true, + lines.first ?? "nil") + XCTAssertTrue(lines.first?.contains(#""3A1B2405003655""#) == true, lines.first ?? "nil") + } + + func testTheHelloProbeIgnoresThePendingAcknowledgement() { + XCTAssertEqual(probeLines(after: helloReply(result: 2)), [], + "a PENDING reply carries no block, so it must not report one without a serial") + } + func testAFrameShorterThanItsDeclaredLengthHasNoPayload() { let frame = bytes("aa011400010021b124230be80146aaaf6a0000000000000014cbbb6c") XCTAssertNil(FrameRouter.commandResponsePayload(in: Array(frame.dropLast(5)))) diff --git a/docs/review/workstreams/W06-ble-collect.md b/docs/review/workstreams/W06-ble-collect.md index 57e9cd4b53..fed32a222b 100644 --- a/docs/review/workstreams/W06-ble-collect.md +++ b/docs/review/workstreams/W06-ble-collect.md @@ -139,7 +139,8 @@ exactly what was tested on hardware. | W06-062 | S4 | Fixed | The note's tests read the host app's `enableRawCapture`, so they fail where the research toggle is on, and cover none of the guard's branches | `StrandTests/CollectorImuBankingTests.swift` | Found by the batch's code review. Fix: `BLEManager.continuousRawCapture` is the manager's one read of the toggle, and the tests set it. `RealtimeRawOwnerTests` pins every input combination and the per-stream rules from a standalone run; `RealtimeRawStreamNoteTests` drives the relaunch, an open session, continuous capture, and an armed capture with its tail through the frame loop. Mutation run: making the store check `?? false`, dropping the armed flag, and letting an unknown owner end a stream each fail a test | | | W06-063 | S4 | Fixed | Every hold-ack test builds the Backfiller with raw capture on and follows each chunk with an empty END, so the default ordering (raw capture off) and a chunk with records after a stall are not pinned | `StrandTests/BackfillerHoldAckTests.swift` | Found by the batch's code review. V0, two mutations of `finishChunk`, each passing all seven hold-ack tests: no cursor or ack unless raw capture is on, and a stall that holds only empty ENDs. Fixed (tests only): `testWithRawCaptureOffAChunkIsAckedAfterItsRowsArchiveAndCursor` pins the default order, and `testAChunkStoredDuringAStallIsNotAcked` (raw capture off and on; the spy can now fail a step once) pins that a chunk whose rows land after a failed one is still not acked. Each mutation fails one of them | | | W06-064 | S4 | Fixed | Five rows ran old and new text together without a sentence break, and W06-052's V0 said a manager with its collector modelled the relaunch | `docs/review/workstreams/W06-ble-collect.md` | Found by the batch's code review. Fixed in this file: the breaks are in (W06-007, W06-018, W06-041, W06-050, W06-052), and W06-052's row says what its test models | | -| W06-065 | S4 | Fixed | `FrameRouter.commandResponsePayload` reads the declared length from bytes 1–2 instead of 2–3, so on every real 5/MG reply the end it computes is past the frame and it returns nil; the Test Centre GET_HELLO identity probe (#1303), its only caller, has never printed | `FrameRouter.swift` `commandResponsePayload` | Found while decoding W06-001. V0: the helper, run verbatim on a real GET_CLOCK reply (28 bytes, declared length 20) reads length 5121 and returns nil; on a SET_CLOCK reply, 3073 and nil. The 13 owner strap logs carry Test Centre connection lines and decoded hellos, and none has a #1303 line. Same pattern swept: no other site reads a 5/MG length there. Fixed: the helper reads the declared length at bytes 2–3 and ends the payload at the CRC32 trailer. V1: `FrameRouterCommandPayloadTests` (two real replies, two short frames) pass, and with the old offset all three fail. V3: a connect with Test Centre → Connection on logs the #1303 line | | +| W06-065 | S4 | Fixed | `FrameRouter.commandResponsePayload` reads the declared length from bytes 1–2 instead of 2–3, so on every real 5/MG reply the end it computes is past the frame and it returns nil; the Test Centre GET_HELLO identity probe (#1303), its only caller, has never printed | `FrameRouter.swift` `commandResponsePayload` | Found while decoding W06-001. V0: the helper, run verbatim on a real GET_CLOCK reply (28 bytes, declared length 20) reads length 5121 and returns nil; on a SET_CLOCK reply, 3073 and nil. The 13 owner strap logs carry Test Centre connection lines and decoded hellos, and none has a #1303 line. Same pattern swept: no other site reads a 5/MG length there. Fixed: the helper reads the declared length at bytes 2–3 and ends the payload at the CRC32 trailer. V1: `FrameRouterCommandPayloadTests` (two real replies, two short frames) pass; with the old offset the two real-reply tests fail and the short-frame test passes either way (V2 corrected an earlier "all three"). V3: a connect with Test Centre → Connection on logs the #1303 line | | +| W06-066 | S4 | Fixed | W06-065 revived the Test Centre GET_HELLO probe (#1303) with two faults: it fires on the PENDING acknowledgement, whose all-zero block reports "none", which the probe reads as "no ASCII serial here"; and it labels the device name at offset 16, the decoder's coordinate (payload from frame byte 11), while this payload starts at byte 13, so the name sits at 14 | `FrameRouter.swift` GET_HELLO probe | Found by the V2 review of the clock batch, with an oracle over the old and new helpers; the owner logs hold 15 PENDING hello replies. Privacy unchanged: the probe withholds non-serial runs either way. Fixed: the probe runs only on a SUCCESS reply and passes `knownNameOffset: 14`. V1: `testTheHelloProbeLabelsTheNameWhereTheDecoderReadsIt` and `testTheHelloProbeIgnoresThePendingAcknowledgement` (synthetic replies, the name placed where the decoder reads it) pass, and both fail with the gate and the offset reverted | | ## Log From c899af45c82b9bbcb96c31e3ac4382cf34fc73a8 Mon Sep 17 00:00:00 2001 From: Artur Iskakov Date: Sat, 26 Sep 2026 12:54:11 +0200 Subject: [PATCH 05/38] fix(ble): say when the clock check sent nothing or found the link down MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit W06-067 — two lines of the new clock check could claim what did not happen. "GET_CLOCK sent" printed even when `send` wrote nothing. After a Bluetooth power-off inside the 10 s window, which never reaches `didDisconnectPeripheral` and so leaves the check's token alive, the timeout logged "setting the clock without a reading" on a dead link. - Severity: S4 · Category: truth · Upstream-able: yes - Location: `Strand/BLE/BLEManager.swift` `beginStrapClockCheck` - V0: found by the batch's V2 review; by construction, since no test seam reaches the private handshake. The flags already matched the old handshake. - Fix: the sent line prints only when a sequence came back. The timeout sets the clock only on a connected link, otherwise says it is neither read nor set, and then settles the flags as before. - V1: both app builds (NOOPiOS without the watch app). --- Strand/BLE/BLEManager.swift | 24 ++++++++++++++++------ docs/review/workstreams/W06-ble-collect.md | 1 + 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/Strand/BLE/BLEManager.swift b/Strand/BLE/BLEManager.swift index ce898f5295..c3822cb9d1 100644 --- a/Strand/BLE/BLEManager.swift +++ b/Strand/BLE/BLEManager.swift @@ -6842,17 +6842,29 @@ extension BLEManager: @preconcurrency CBPeripheralDelegate { strapClockCheck = StrapClock.Check() strapClockCheckToken &+= 1 let token = strapClockCheckToken + // #1823: say what was SENT, not what resulted; the verdict line follows the strap's answer. W06-067: + // and only when it was sent. if let sequence = sendReturningSequence(.getClock, payload: []) { strapClockCheck.beginRead(sequence: sequence, at: Date().timeIntervalSince1970) + log("WHOOP 5/MG: GET_CLOCK sent — SET_CLOCK follows only if the strap clock is invalid or provably " + + "more than \(Int(StrapClock.driftThresholdSeconds)) s off") + } else { + log("WHOOP 5/MG: GET_CLOCK was not sent, so the clock check waits out its " + + "\(Int(StrapClock.replyTimeoutSeconds)) s timeout") } - // #1823: say what was SENT, not what resulted; the verdict line follows the strap's answer. - log("WHOOP 5/MG: GET_CLOCK sent — SET_CLOCK follows only if the strap clock is invalid or more than " - + "\(Int(StrapClock.driftThresholdSeconds)) s off") DispatchQueue.main.asyncAfter(deadline: .now() + StrapClock.replyTimeoutSeconds) { [weak self] in guard let self, token == self.strapClockCheckToken, self.strapClockCheck.expire() else { return } - self.log("WHOOP 5/MG clock: no GET_CLOCK reply within \(Int(StrapClock.replyTimeoutSeconds)) s — " - + "setting the clock without a reading") - self.setStrapClock() + // W06-067: a Bluetooth power-off does not end the link through `didDisconnectPeripheral`, so the + // token can outlive it. Say so rather than claim a set nothing can send; the handshake flags still + // settle, as they did when the handshake set them directly. + if self.state.connected { + self.log("WHOOP 5/MG clock: no GET_CLOCK reply within \(Int(StrapClock.replyTimeoutSeconds)) s — " + + "setting the clock without a reading") + self.setStrapClock() + } else { + self.log("WHOOP 5/MG clock: no GET_CLOCK reply within \(Int(StrapClock.replyTimeoutSeconds)) s, " + + "and the link is down — the clock is neither read nor set") + } self.strapClockSettled() } } diff --git a/docs/review/workstreams/W06-ble-collect.md b/docs/review/workstreams/W06-ble-collect.md index fed32a222b..82c6550ee6 100644 --- a/docs/review/workstreams/W06-ble-collect.md +++ b/docs/review/workstreams/W06-ble-collect.md @@ -141,6 +141,7 @@ exactly what was tested on hardware. | W06-064 | S4 | Fixed | Five rows ran old and new text together without a sentence break, and W06-052's V0 said a manager with its collector modelled the relaunch | `docs/review/workstreams/W06-ble-collect.md` | Found by the batch's code review. Fixed in this file: the breaks are in (W06-007, W06-018, W06-041, W06-050, W06-052), and W06-052's row says what its test models | | | W06-065 | S4 | Fixed | `FrameRouter.commandResponsePayload` reads the declared length from bytes 1–2 instead of 2–3, so on every real 5/MG reply the end it computes is past the frame and it returns nil; the Test Centre GET_HELLO identity probe (#1303), its only caller, has never printed | `FrameRouter.swift` `commandResponsePayload` | Found while decoding W06-001. V0: the helper, run verbatim on a real GET_CLOCK reply (28 bytes, declared length 20) reads length 5121 and returns nil; on a SET_CLOCK reply, 3073 and nil. The 13 owner strap logs carry Test Centre connection lines and decoded hellos, and none has a #1303 line. Same pattern swept: no other site reads a 5/MG length there. Fixed: the helper reads the declared length at bytes 2–3 and ends the payload at the CRC32 trailer. V1: `FrameRouterCommandPayloadTests` (two real replies, two short frames) pass; with the old offset the two real-reply tests fail and the short-frame test passes either way (V2 corrected an earlier "all three"). V3: a connect with Test Centre → Connection on logs the #1303 line | | | W06-066 | S4 | Fixed | W06-065 revived the Test Centre GET_HELLO probe (#1303) with two faults: it fires on the PENDING acknowledgement, whose all-zero block reports "none", which the probe reads as "no ASCII serial here"; and it labels the device name at offset 16, the decoder's coordinate (payload from frame byte 11), while this payload starts at byte 13, so the name sits at 14 | `FrameRouter.swift` GET_HELLO probe | Found by the V2 review of the clock batch, with an oracle over the old and new helpers; the owner logs hold 15 PENDING hello replies. Privacy unchanged: the probe withholds non-serial runs either way. Fixed: the probe runs only on a SUCCESS reply and passes `knownNameOffset: 14`. V1: `testTheHelloProbeLabelsTheNameWhereTheDecoderReadsIt` and `testTheHelloProbeIgnoresThePendingAcknowledgement` (synthetic replies, the name placed where the decoder reads it) pass, and both fail with the gate and the offset reverted | | +| W06-067 | S4 | Fixed | Two lines of the new clock check can claim what did not happen: "GET_CLOCK sent" printed even when `send` wrote nothing, and after a Bluetooth power-off inside the 10 s window, which never reaches `didDisconnectPeripheral` and so leaves the check's token alive, the timeout logged "setting the clock without a reading" on a dead link | `BLEManager.swift` `beginStrapClockCheck` | Found by the V2 review of the clock batch. V0 by construction: the first line followed the send unconditionally, and the power-off path resets no per-link state (only `didDisconnectPeripheral` does); no test seam reaches the private handshake. The handshake flags reach the same state the old code set at the handshake, so only the log claims were new. Fixed: the sent line prints only when a sequence came back, with a line saying the check waits out its timeout otherwise; the timeout sets the clock only on a connected link and otherwise says it is neither read nor set, then settles the flags as before. V1: both app builds | | ## Log From 29763a35969d083781d80253a94262628d7c456d Mon Sep 17 00:00:00 2001 From: Artur Iskakov Date: Sat, 26 Sep 2026 12:56:17 +0200 Subject: [PATCH 06/38] docs(ble): correct the clock batch's counts and stale text MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit W06-068 — the clock batch's own text overstated or went stale: - "20 replies" where the logs hold 12 distinct ones. - "Seven zero bytes" stated as layout, when each GET ran in the same millisecond as its SET, so they may be sub-second ticks. - The linux-capture tools claimed to mirror `StrapClock`, but they read stream timestamps, and `whoop_setclock` writes by default. - A test comment, two restore-flag comments and CONTRIBUTING still described the old set-every-connect order. - The Backfiller copy still claimed a byte-identical Android twin. - The `validityFloor` doc cited the 146/147 contract as if it covered command 11. - Severity: S4 · Category: truth · Upstream-able: yes - V0: found by the batch's V2 review. Each item was checked before the edit: 12 distinct GET frames, and tool defaults of 0 and 30 s. - V1: `doc_comment_lint`, `check_source_references` and the linux-capture tests pass. The W06-001 and W06-050 rows now carry the 12-reply count and the console evidence that the late sets were processed 2.3–3.9 s after the phone stamped them. W06-050 also records V2's two questions for the strap run: a lost read reply, and drift. --- .../Sources/WhoopProtocol/StrapClock.swift | 14 +++++++++----- Strand/BLE/BLEManager.swift | 5 +++-- Strand/BLE/FrameRouter.swift | 2 +- Strand/Collect/Backfiller.swift | 6 +++--- StrandTests/BackfillerSessionTallyTests.swift | 2 +- Tools/linux-capture/README.md | 7 ++++--- Tools/linux-capture/whoop_setclock.py | 11 ++++++----- Tools/linux-capture/whoop_sync.py | 4 ++-- docs/BLE_REVERSE_ENGINEERING.md | 7 ++++--- docs/CONTRIBUTING.md | 7 ++++--- docs/review/workstreams/W06-ble-collect.md | 11 +++++++++-- 11 files changed, 46 insertions(+), 30 deletions(-) diff --git a/Packages/WhoopProtocol/Sources/WhoopProtocol/StrapClock.swift b/Packages/WhoopProtocol/Sources/WhoopProtocol/StrapClock.swift index 3074a98f53..239232b4c9 100644 --- a/Packages/WhoopProtocol/Sources/WhoopProtocol/StrapClock.swift +++ b/Packages/WhoopProtocol/Sources/WhoopProtocol/StrapClock.swift @@ -10,9 +10,11 @@ import Foundation /// provably more than `driftThresholdSeconds` off removes both steps for a strap whose clock is fine. public enum StrapClock { - /// Seconds at or below this are not a wall clock. The strap applies a stored time only above it - /// (`docs/PROTOCOL_TRANSPORT.md` §Clock and identity contracts), and a failed read can report zero with - /// a success result, so a reading here needs a set whatever the result byte says. + /// Seconds at or below this are not a wall clock. The 146/147 clock contract applies a stored time only + /// above it (`docs/PROTOCOL_TRANSPORT.md` §Clock and identity contracts) and warns that a failed read can + /// return zero with a success result. That contract is for the other command pair; here the floor is + /// only a validity bound for the command-11 reply, so a reading at or below it needs a set whatever the + /// result byte says. public static let validityFloor: UInt32 = 1_293_840_001 /// How far the strap clock may sit from the phone's before a connect sets it. @@ -42,8 +44,10 @@ public enum StrapClock { } /// Decode a format-1 COMMAND_RESPONSE to GET_CLOCK: type 36 at byte 8, command 11 at byte 10, the - /// request's sequence at 11, the result at 12 and the strap's Unix seconds as a u32 LE at 13. Twenty - /// replies from firmware 50.39.1.0 have this layout, each followed by seven zero bytes that are not read. + /// request's sequence at 11, the result at 12 and the strap's Unix seconds as a u32 LE at 13. Twelve + /// distinct replies from firmware 50.39.1.0 have this layout. Seven more bytes follow, zero in every one + /// of them; each of those GETs ran in the same millisecond as the SET before it, so the bytes may be + /// sub-second ticks that read zero there. They are not read. /// /// Structure only: the caller passes a frame that passed `verifyFrame`. The body ends where the declared /// length (bytes 2–3) puts the CRC32 trailer. Returns nil for any other frame or a body that stops before diff --git a/Strand/BLE/BLEManager.swift b/Strand/BLE/BLEManager.swift index c3822cb9d1..7c35cc5d51 100644 --- a/Strand/BLE/BLEManager.swift +++ b/Strand/BLE/BLEManager.swift @@ -5198,7 +5198,8 @@ public final class BLEManager: NSObject, ObservableObject { // no longer delivers, and `setNotifyValue(true)` on an already-notifying char yields NO callback // (no state change). Force one real off→on cycle so delivery is re-established AND // `didUpdateNotificationStateFor` fires — the only path that latches `cmdNotifyConfirmedActive` - // → `connectSettled` → the alarm re-arm. One-shot: `restoreNeedsResubscribe` clears at settle. + // → `connectSettled` → the alarm re-arm. One-shot: `restoreNeedsResubscribe` clears once the + // forced pass has run (the 5/MG handshake, or the settle on WHOOP 4). if restoreNeedsResubscribe { log("Notify re-arming after restore \(c.uuid) (\(reason))") p.setNotifyValue(false, for: c) @@ -6312,7 +6313,7 @@ extension BLEManager: @preconcurrency CBCentralManagerDelegate { // #613: the inherited notify subscriptions come back reported-active but dead. Force one real // off→on re-subscribe this session (see `requestNotify`) so live HR/R-R resume AND // `didUpdateNotificationStateFor` fires → `cmdNotifyConfirmedActive` → `connectSettled` → the - // alarm re-arm. Cleared when `connectSettled` bumps. + // alarm re-arm. Cleared once the forced pass has run (the 5/MG handshake, or the settle on WHOOP 4). restoreNeedsResubscribe = true log("Restored CONNECTED peripheral \(p.identifier) — re-discovering services") discoverPrimaryServices(on: p) diff --git a/Strand/BLE/FrameRouter.swift b/Strand/BLE/FrameRouter.swift index 21281d0f0d..d2813d5d3f 100644 --- a/Strand/BLE/FrameRouter.swift +++ b/Strand/BLE/FrameRouter.swift @@ -179,7 +179,7 @@ public final class FrameRouter { // REBOOT_STRAP already uses. LOG-ONLY; it never gates behaviour. if let cmd = parsed.cmdName, cmd.hasPrefix("SET_CLOCK") || cmd.hasPrefix("GET_CLOCK") { // W06-001: both replies carry the format-1 result at byte 12, and GET_CLOCK's carries the - // strap's seconds at 13 (`StrapClock.decodeReply`; 20 replies from 50.39.1.0 agree). This + // strap's seconds at 13 (`StrapClock.decodeReply`; 12 distinct replies from 50.39.1.0 agree). This // line quotes the reply; BLEManager's "WHOOP 5/MG clock:" line judges it. The WHOLE frame // (#900's format) stays, uncapped, so a moved offset on another firmware stays visible. let result = parsed.parsed["result"]?.stringValue ?? "none" diff --git a/Strand/Collect/Backfiller.swift b/Strand/Collect/Backfiller.swift index 792a5d51f1..c93f06cae2 100644 --- a/Strand/Collect/Backfiller.swift +++ b/Strand/Collect/Backfiller.swift @@ -533,7 +533,7 @@ final class Backfiller { /// sets it when it is invalid or off (W06-050), so "charge it" alone has already been retried every /// session. /// - /// Byte-identical to the Android twin. No em-dash (project rule). + /// No em-dash (project rule). The upstream Android twin still says NOOP re-sends the clock every connect. nonisolated static func staleRecordLine(newestUnix: Int, wallNowUnix: Int) -> String { let ageDays = max(0, wallNowUnix - newestUnix) / 86_400 return "Backfill: this sync banked nothing and the strap's newest stored record is about \(ageDays) day(s) old. If you have worn it since then, it has stopped saving history to its flash. NOOP already checks the strap clock on every connect and sets it when it is wrong, so charging alone may not be enough: charge to 100% and reconnect, then use Restart strap in Devices, and if that does not help forget and re-pair. If the official WHOOP app is also missing these days, the strap is the cause and not NOOP." @@ -549,8 +549,8 @@ final class Backfiller { /// along. A banner that keeps promising something that keeps not happening teaches people to distrust /// the app rather than their strap. /// - /// Byte-identical to the Android twin. Not localized, matching the sibling `lastSyncError` copy on - /// both platforms; localizing that surface is its own change. No em-dash (project rule). + /// Not localized, matching the sibling `lastSyncError` copy; localizing that surface is its own change. + /// No em-dash (project rule). The upstream Android twin still says NOOP re-sets the clock every connect. nonisolated static func staleRecordBanner(newestUnix: Int, wallNowUnix: Int) -> String { let ageDays = max(0, wallNowUnix - newestUnix) / 86_400 return "Synced, but your strap handed over no stored history, and its newest saved record is about \(ageDays) day(s) old. If you have been wearing it since then, it has stopped saving to flash. Charge it to 100% and reconnect; NOOP already checks its clock every connect and sets it when it is wrong, so if that does not help, try Restart strap in Devices, then forget and re-pair. If the official WHOOP app is missing these days too, the strap is the cause and not NOOP." diff --git a/StrandTests/BackfillerSessionTallyTests.swift b/StrandTests/BackfillerSessionTallyTests.swift index 19e59f76b0..67ccb266ab 100644 --- a/StrandTests/BackfillerSessionTallyTests.swift +++ b/StrandTests/BackfillerSessionTallyTests.swift @@ -297,7 +297,7 @@ final class BackfillerSessionTallyTests: XCTestCase { /// The banner is what the user READS; the log line needs a capture export. The standing banner /// omitted the age entirely and PROMISED that charging "should" work - advice NOOP has effectively - /// retried on every connect for weeks, since it re-sends SET_CLOCK each time. + /// retried on every connect for weeks, since every connect checks the clock and sets a wrong one. func testStaleRecordBannerDatesTheSilenceAndPromisesNothing() { let line = Backfiller.staleRecordBanner(newestUnix: 1_785_692_420, wallNowUnix: 1_787_820_941) XCTAssertTrue(line.contains("about 24 day(s) old"), line) diff --git a/Tools/linux-capture/README.md b/Tools/linux-capture/README.md index df5d9d66a9..78d3d48f48 100644 --- a/Tools/linux-capture/README.md +++ b/Tools/linux-capture/README.md @@ -196,9 +196,10 @@ Exit codes: `0` accepted, `2` strap not found/reachable, `3` connected but no bu A strap left offline (no app) for a long time loses its clock — its RTC drifts or resets, so realtime and newly-recorded historical frames get **bogus timestamps** (e.g. dated 1971). The phone app fixes -this with `SET_CLOCK` on every connect; this does the same from Linux. It **reads the clock first and -only writes if it has drifted** past `--if-drift` seconds (mirrors the app's `StrapClock` — no -gratuitous resets), then re-reads to verify the new time latched. +this by reading the clock on every connect and sending `SET_CLOCK` when it is off; this sets it from +Linux. It **reads the clock first**, from stream-frame timestamps, and **only writes if it has drifted** +past `--if-drift` seconds (default 0, so it always writes unless you pass one), then re-reads to verify +the new time latched. ```bash # read-only: report the strap's current clock and how far it has drifted diff --git a/Tools/linux-capture/whoop_setclock.py b/Tools/linux-capture/whoop_setclock.py index da147e6ef6..7e3817e444 100644 --- a/Tools/linux-capture/whoop_setclock.py +++ b/Tools/linux-capture/whoop_setclock.py @@ -2,8 +2,8 @@ """whoop_setclock.py — set a WHOOP strap's RTC to the current wall-clock time, and verify it latched. A strap left offline (no app) for a long time loses its clock — realtime/historical frames then carry -bogus timestamps. The phone app fixes this with SET_CLOCK on every connect; this does the same from -Linux. WRITE operation, but safe and reversible (the phone re-sets the clock on its next sync). +bogus timestamps. The phone app reads the clock on every connect and sets it when it is off; this sets it +from Linux. WRITE operation, but safe and reversible (the phone checks the clock again on its next connect). Verification: after writing SET_CLOCK, the tool briefly subscribes and scans incoming frames (events carry the strap RTC in a u32 timestamp) for a value within a minute of wall time. If found, the clock @@ -11,8 +11,9 @@ Phone Bluetooth must be OFF and the strap bonded + advertising. -It reads the strap clock first and only writes when it has drifted past `--if-drift` seconds (mirrors -the app's StrapClock — avoid gratuitous resets), then re-reads to verify the new clock latched. +It reads the strap clock first (from the timestamps of stream frames, not GET_CLOCK) and only writes when +it has drifted past `--if-drift` seconds, default 0, so by default it always writes; then it re-reads to +verify the new clock latched. The app's own rule is `StrapClock` in WhoopProtocol. Usage: python3 whoop_setclock.py --model whoop4 --address AA:BB:CC:DD:EE:FF # set to now @@ -111,7 +112,7 @@ async def run(args) -> int: async with BleakClient(dev) as client: print(f"connected: {client.is_connected}") - # 1. READ the strap clock first (so we only write when needed — mirrors the app's StrapClock). + # 1. READ the strap clock first, so we only write when `--if-drift` says it is off. before = await read_strap_clock(client, cfg, args.model) wall = int(time.time()) if before is None: diff --git a/Tools/linux-capture/whoop_sync.py b/Tools/linux-capture/whoop_sync.py index b63ae51f0b..580cc88fb3 100644 --- a/Tools/linux-capture/whoop_sync.py +++ b/Tools/linux-capture/whoop_sync.py @@ -407,8 +407,8 @@ async def _acquire(address, tries=6): async def preflight_clock(fam, args): """Before syncing, read the strap RTC and set it to now if it has drifted past - `args.clock_threshold` seconds — mirrors the app's StrapClock (only write on real drift, so we - don't gratuitously reset). A strap left offline for months loses its clock, which would otherwise + `args.clock_threshold` seconds (default 30), read from stream-frame timestamps. Like the app, it + writes only on real drift, so it does not gratuitously reset; the app's own rule is `StrapClock`. A strap left offline for months loses its clock, which would otherwise mis-date every captured/recorded frame. Self-contained: its own short connection that reads the clock and (if needed) sets it, so it never diff --git a/docs/BLE_REVERSE_ENGINEERING.md b/docs/BLE_REVERSE_ENGINEERING.md index dc6fc837c1..58b7ea1a93 100644 --- a/docs/BLE_REVERSE_ENGINEERING.md +++ b/docs/BLE_REVERSE_ENGINEERING.md @@ -479,9 +479,10 @@ What does **not** transfer: `REPORT_VERSION_INFO` (7) and `GET_EXTENDED_BATTERY_ stub payloads on this firmware (so the firmware version lives in the `GET_HELLO` block instead), and `GET_CLOCK` (11) isn't served at all. Record dating does not need it, since realtime (type-40) and historical (type-47) both carry real unix rather than a device epoch. Firmware 50.39.1.0 does serve it: -twenty replies carry the format-1 result at `frame[12]` and the strap's unix seconds as a u32 LE at -`frame[13]`, then seven zero bytes. NOOP reads it on connect and sets the clock only when the reading is -invalid or more than 2 s off (`StrapClock`). +twelve distinct replies carry the format-1 result at `frame[12]` and the strap's unix seconds as a u32 LE +at `frame[13]`. Seven more bytes follow, zero in all twelve; each of those reads ran in the same millisecond +as a SET, so they may be sub-second ticks. NOOP reads the clock on connect and sets it only when the reading +is invalid, or provably more than 2 s off once the request's round trip is allowed for (`StrapClock`). > **Privacy.** The `GET_HELLO` response also contains a **session token**, which the decoder never > reads or exposes — only the device name and firmware version are surfaced. The `device_name`/ diff --git a/docs/CONTRIBUTING.md b/docs/CONTRIBUTING.md index e22ba88042..4dbd3b5bb0 100644 --- a/docs/CONTRIBUTING.md +++ b/docs/CONTRIBUTING.md @@ -444,9 +444,10 @@ load-bearing and was hardened against real failure modes that are documented in realtime-raw flood that dominated flash). Treat it as stable infrastructure: - **Don't reorder the connect handshake.** Offload is deliberately gated on - `connectHandshakeDone`; a `SET_CLOCK` (cmd 10) must precede arming the firmware alarm so the strap - RTC is UTC-correct. On WHOOP 5/MG the handshake reads the clock first and sets it only when the - reading is invalid or more than 2 s off; `connectHandshakeDone` waits for that check. + `connectHandshakeDone`; the strap RTC must be UTC-correct before the firmware alarm is armed. On + WHOOP 5/MG the handshake reads the clock and sends `SET_CLOCK` (cmd 10) only when the reading is + invalid or provably more than 2 s off, and `connectHandshakeDone`, which the alarm re-arm waits on, + is set only after that check, so any set still comes first. - **Don't `ENTER` high-frequency sync.** The app no longer enters it and sends `exitHighFreqSync` defensively on connect to release straps parked there by older builds. - **Prefer `.withoutResponse` writes** (the `send(_:payload:writeType:)` default); use diff --git a/docs/review/workstreams/W06-ble-collect.md b/docs/review/workstreams/W06-ble-collect.md index 82c6550ee6..64f70555c5 100644 --- a/docs/review/workstreams/W06-ble-collect.md +++ b/docs/review/workstreams/W06-ble-collect.md @@ -75,7 +75,7 @@ exactly what was tested on hardware. | ID | Sev | Status | Finding | Location | Evidence | PR | |---|---|---|---|---|---|---| -| W06-001 | S4 | Fixed | GET_CLOCK reply dropped for 5/MG | `BLEManager.swift` 5/MG handshake; `FrameRouter.swift` `COMMAND_RESPONSE` | State confirmed: sent on every connect, the reply is only hex-logged (10 replies in owner logs), no decode sets a clock, `clockRef` stays nil. No effect on history dating (records carry unix seconds) or safe-trim, so lowered from S3; what remains is readout truth (W06-019), Phase 3. V0 (2026-09-26): the reply layout is confirmed on 20 replies from 50.39.1.0: origin sequence at byte 11, result at 12 (SUCCESS in all 20), u32 LE seconds at 13, then seven zero bytes, as `docs/PROTOCOL_TRANSPORT.md` §Responses gives for format 1. Needed by W06-050's read-first policy, so fixed with it. Fixed: `StrapClock.decodeReply` (WhoopProtocol) reads the origin sequence, the result and the seconds, bounded by the declared length, and the FrameRouter clock line now prints the result and the strap's seconds instead of saying the result offset is unestablished. V1: `StrapClockTests` decode three real replies, and refuse a SET_CLOCK reply, our own request frame, and seconds read past the declared length | | +| W06-001 | S4 | Fixed | GET_CLOCK reply dropped for 5/MG | `BLEManager.swift` 5/MG handshake; `FrameRouter.swift` `COMMAND_RESPONSE` | State confirmed: sent on every connect, the reply is only hex-logged (10 replies in owner logs), no decode sets a clock, `clockRef` stays nil. No effect on history dating (records carry unix seconds) or safe-trim, so lowered from S3; what remains is readout truth (W06-019), Phase 3. V0 (2026-09-26): the reply layout is confirmed on 12 distinct replies from 50.39.1.0 (the logs repeat some; an earlier count said 20): origin sequence at byte 11, result at 12 (SUCCESS in all 12), u32 LE seconds at 13, then seven bytes that were zero in all 12 and may be sub-second ticks, as `docs/PROTOCOL_TRANSPORT.md` §Responses gives for format 1. Needed by W06-050's read-first policy, so fixed with it. Fixed: `StrapClock.decodeReply` (WhoopProtocol) reads the origin sequence, the result and the seconds, bounded by the declared length, and the FrameRouter clock line now prints the result and the strap's seconds instead of saying the result offset is unestablished. V1: `StrapClockTests` decode three real replies, and refuse a SET_CLOCK reply, our own request frame, and seconds read past the declared length | | | W06-002 | S2 | Fixed | A Raw Data Collector session on a 5/MG never records an IMU sample: `ImuSessionFileStore.append` is reachable only through `Collector.ingest(frame:parsed:)`, which nothing calls | `Collector.swift` `ingest`, `recordGroundTruthImu`; `BLEManager.swift` 5/MG frame loop | P5 confirmed (found by W1 and W6). #1709 moved IMU storage behind an ingest seam 5/MG never enters; upstream has the same gap. V0: 0 production callers of `Collector.ingest(frame:parsed:)`, the only path to `ImuSessionFileStore.append`. Fix: the 5/MG frame loop calls `Collector.bankImuForSessions` for every frame, live and offload, gated to intact 1244-byte buffers. V1: `CollectorImuBankingTests` (intact banked, CRC-flipped refused); Strand and NOOPiOS (no watch) build. V2 (independent reviewer): holds, with gaps recorded as W06-026 … W06-032 and W06-035: the gate checks no packet type (W06-029), a relaunch makes duplicates rescan the segment (W06-026), and no test pinned the call site (W06-027), so V1 was not complete until W06-027 pinned the call; no double bank and no cross-device bank (oracle, relaunch included). V3 (11.9.8 on the iPhone): a 152 s session banked 148 consecutive one-second buffers, all intact, none missing between the first and the last; the 4 missing are the session's last seconds (W06-025). V3 of the offload half, the 11.9.9 strap run (2026-09-25; a session of about three minutes, one marker, an app relaunch mid-session): after the relaunch, the history sync banked the 22 seconds the live stream had missed, the first hardware proof that history reaches a session | | | W06-003 | S3 | Reported | Mapped v20 (optical) and v21 (IMU) have no storage lane: every record goes to the 5 MB rolling reject archive, which one sync saturates, so only the newest ~630–720 of each survive after the strap has trimmed them | `Interpreter.swift` `mappedWhoop5HistoricalVersions`; `HistoricalStreams.swift` `rejectedHistoricalRecords`; `RawHistoryArchive.swift` | P5 confirmed with caveat: nothing consumes v20/v21, so no shown metric is wrong; a retention decision is missing. Every line in the 9 archives in the repo root is type 47 v16, v20 or v21. Root cause of W06-004 and W06-005. AD-6 | | | W06-004 | S3 | Reported | Every over-cap reject-archive rewrite runs on the main actor (about 290 ms for a 5 MB archive on an M-series Mac) and, with v20/v21 holding the archive at cap, recurs every 7–8 chunks through an offload | `RawHistoryArchive.swift` `archive`, `evictLines`; `Backfiller.swift` `rejectedSink` | Found by pass 5 (missed by pass 3). AD-3 evidence; measure on the iPhone in Phase 3 | | @@ -124,7 +124,7 @@ exactly what was tested on hardware. | W06-047 | S4 | Fixed | Every banked IMU buffer builds and configures a `DateFormatter` on the main actor to name its segment file | `ImuSessionFileStore.swift` `segmentFile`, `utcName` | Hot-path cost of the same order as the decode W06-031 removed. V0 (oracle, -O on an M-series Mac): 45.5 µs a call against 1.2 µs with one formatter, same output; so worse than the 8 µs decode W06-031 removed. Fix: one formatter, built once. Refactor check: `ImuSessionFileStoreTests.testSegmentsAreNamedForTheirUTCHalfHour` pins the name on disk and in the export, and passes before and after | | | W06-048 | S4 | Reported | The per-segment `seen` sets kept since W06-026 are never evicted for closed windows; only `remove(id:)` prunes them | `ImuSessionFileStore.swift` `seen` | A week-long historical window re-delivered after a relaunch keeps 336 segment sets | | | W06-049 | S4 | Fixed | `segmentScans` is state the production store carries only so one test can read it | `ImuSessionFileStore.swift` | Cleanup. Fix: the store takes its file reader as a parameter; W06-026's test counts reads through it and now states the property itself, 49 more duplicates reading no file again, and still fails with W06-026's fix reverted | | -| W06-050 | S3 | Fixed | A SET_CLOCK during a live Raw Data Collector session steps the strap clock, and the IMU timeline with it. A forward step skips labels, so the session reads incomplete although no motion is missing; a backward step would give new buffers seconds already banked, and `append` drops them as duplicates | `BLEManager.swift` 5/MG handshake (SET_CLOCK on every connect); `ImuSessionFileStore.swift` `append` | Found in the 11.9.9 strap run. The relaunch's SET_CLOCK (value the second after the gap) skipped exactly the three labels before it; the delay from buffer second to arrival fell from 4.7 s to 1.86 s at that moment, and the strap's console counter ran about 3.1 s behind the phone between that set and the one 179 s earlier (its unit read as milliseconds, unconfirmed). The strap's own history has no buffer for those labels either. Caveat: one observation, and the delay held constant through the session, so steady drift does not explain a 3 s lag; the backward case is from the code. Needs an owner decision: set the clock only when GET_CLOCK shows it off (needs W06-001's decode), or record the step in the session and count coverage in strap time. Seen again on 11.9.10 (2026-09-25): the relaunch after W06-053's crash sent SET_CLOCK mid-session; the delay from buffer second to arrival fell from 5.8 s to 1.9 s across it, the four labels before it are missing from both the live stream and the strap's history, and the session read 113 of 117 s. The marker-free session with no relaunch read complete. Two observations now, both a forward step of about four seconds at a mid-session SET_CLOCK. Owner decision 2026-09-25: read the clock, then set it. Decode the GET_CLOCK reply (W06-001), send GET_CLOCK first, and send SET_CLOCK only when the strap clock is invalid or off by more than about two seconds. Its own PR with W06-001, after the reply layout is confirmed on the strap. V0 (2026-09-26, 13 owner strap logs, 20 SET/GET pairs on 50.39.1.0): the GET_CLOCK readback equals the SET value exactly in all 20, including the four whose replies came about four seconds after the send, so the strap applies a value stamped seconds earlier. Both observed forward steps follow such a late set: the set before the 11.9.9 session was answered about 4 s late and the relaunch set 179 s later was prompt; on 11.9.10 the first relaunch's set was answered 4 s late and the next relaunch's set, prompt, stepped the clock by the 3.9 s the IMU delay fell. So the step is the correction of an earlier stale set, and reading first removes both: a strap within 2 s is not set at all. Confirmed; reclassified from one observation to a mechanism. Fixed: the 5/MG handshake sends GET_CLOCK only, and `StrapClock.judge` asks for a SET_CLOCK only when the reading is refused, at or below the validity floor, or more than 2 s off across the whole request round trip (a long round trip never sets by itself); with no reply in 10 s it sets the clock without a reading, as before. `connectHandshakeDone`, which the first offload, the sync controls and the alarm re-arm wait on, is set when the check settles, so a set still precedes history and the alarm. The stale-record copy said NOOP re-sets the clock every connect; it now says NOOP checks it and sets a wrong one. V1: 19 `StrapClockTests`; five mutations (always set, which is the old behaviour; ignore the request sequence; expire after settling; ignore the round trip; floor at zero) each fail the suite. Both app builds pass. V3 needs a strap run: a connect logs a "within 2 s, not set" line and no SET_CLOCK, and a relaunch during a Raw Data Collector session skips no IMU labels | | +| W06-050 | S3 | Fixed | A SET_CLOCK during a live Raw Data Collector session steps the strap clock, and the IMU timeline with it. A forward step skips labels, so the session reads incomplete although no motion is missing; a backward step would give new buffers seconds already banked, and `append` drops them as duplicates | `BLEManager.swift` 5/MG handshake (SET_CLOCK on every connect); `ImuSessionFileStore.swift` `append` | Found in the 11.9.9 strap run. The relaunch's SET_CLOCK (value the second after the gap) skipped exactly the three labels before it; the delay from buffer second to arrival fell from 4.7 s to 1.86 s at that moment, and the strap's console counter ran about 3.1 s behind the phone between that set and the one 179 s earlier (its unit read as milliseconds, unconfirmed). The strap's own history has no buffer for those labels either. Caveat: one observation, and the delay held constant through the session, so steady drift does not explain a 3 s lag; the backward case is from the code. Needs an owner decision: set the clock only when GET_CLOCK shows it off (needs W06-001's decode), or record the step in the session and count coverage in strap time. Seen again on 11.9.10 (2026-09-25): the relaunch after W06-053's crash sent SET_CLOCK mid-session; the delay from buffer second to arrival fell from 5.8 s to 1.9 s across it, the four labels before it are missing from both the live stream and the strap's history, and the session read 113 of 117 s. The marker-free session with no relaunch read complete. Two observations now, both a forward step of about four seconds at a mid-session SET_CLOCK. Owner decision 2026-09-25: read the clock, then set it. Decode the GET_CLOCK reply (W06-001), send GET_CLOCK first, and send SET_CLOCK only when the strap clock is invalid or off by more than about two seconds. Its own PR with W06-001, after the reply layout is confirmed on the strap. V0 (2026-09-26, 13 owner strap logs, which overlap to 12 distinct SET/GET pairs on 50.39.1.0): the GET_CLOCK readback equals the SET value in all 12, including those answered about four seconds after the send. The readback alone proves little, since the strap handled each GET in the same millisecond as its SET; V2 settled it with the strap's own console uptime (a millisecond counter, now confirmed): against the prompt sets, five sets were processed 2.3–3.9 s after the phone stamped them, so the delay comes before the strap processes the command, not in reply transit. Both observed forward steps follow such a late set (about 3.1 s and 3.7 s by the console): the set before the 11.9.9 session was answered about 4 s late and the relaunch set 179 s later was prompt; on 11.9.10 the first relaunch's set was answered 4 s late and the next relaunch's set, prompt, stepped the clock by the 3.9 s the IMU delay fell. So the step is the correction of an earlier stale set, and reading first removes both: a strap within 2 s is not set at all. Confirmed; reclassified from one observation to a mechanism. Fixed: the 5/MG handshake sends GET_CLOCK only, and `StrapClock.judge` asks for a SET_CLOCK only when the reading is refused, at or below the validity floor, or more than 2 s off across the whole request round trip (a long round trip never sets by itself); with no reply in 10 s it sets the clock without a reading, as before. `connectHandshakeDone`, which the first offload, the sync controls and the alarm re-arm wait on, is set when the check settles, so a set still precedes history and the alarm. The stale-record copy said NOOP re-sets the clock every connect; it now says NOOP checks it and sets a wrong one. V1: 19 `StrapClockTests`; five mutations (always set, which is the old behaviour; ignore the request sequence; expire after settling; ignore the round trip; floor at zero) each fail the suite. Both app builds pass. V2 (independent reviewer, 2026-09-26): holds; an oracle over 2 M random cases judged no in-sync strap off and no strap more than 2 s off in sync, and a strap is always set once it is about 3 s plus the round trip off. Its truth findings are W06-066 … W06-068. V3 needs a strap run: a connect logs a "within 2 s, not set" line and no SET_CLOCK, and a relaunch during a Raw Data Collector session skips no IMU labels. Two things the run can show: a lost read reply falls through to the blind set after 10 s and would bring the step back (12 of 12 replies arrived so far, 7 on restored links; retrying the read once is the cheap remedy if one is lost), and the strap's counter ran about 15 ppm slow, so an occasional "setting it" after about two days of unbroken connection is expected, not a fault | | | W06-051 | S2 | Reproduced | The app process ended three times in about four and a half minutes (before the session, mid-session, and just after Stop), each time relaunched by CoreBluetooth state restoration; the log shows nothing before any of them | iOS app | Found in the 11.9.9 strap run: three "previous app session" generations, each ending without an error line and followed by "Restored CONNECTED peripheral". State restoration relaunches only an app the system ended, so this was not a force-quit. Cause unknown until the iPhone's crash or jetsam reports for those minutes are read; a crash on real data would be S1. V0 (2026-09-25, the 11.9.10 strap run): the mid-session death on 11.9.9 came 3.5 s after a marker was added, and the 11.9.10 death that the install does not explain came under 2 s after one; that crash is W06-053. A marker-free session of about two minutes on 11.9.10 had no death. The other two 11.9.9 deaths are not attributed: the log does not record the marker sheet, and a Cancel crashes the same way as a Save. V3 with W06-053: a strap run with markers and no death closes this row with it; a death without a marker needs the iPhone's crash reports | | | W06-052 | S4 | Fixed | After a relaunch during a session, the Raw IMU fail-safe takes the session's own stream for a stray producer and asks for a stop: `rawCaptureInFlight` belongs to the process, while the session is persisted and only the collector view re-arms it | `BLEManager.swift` `stopUnexpectedRealtimeImu`; `RawDataCollectorView.swift` re-arm | Found in the 11.9.9 strap run: twice, "Raw IMU fail-safe: unexpected realtime packet type 43 while capture was off; stop requested", each dropped only because the command characteristic was not yet discovered; W06-018's allowlist would drop it afterwards. The line claims a stray producer the app itself started. Seen again on 11.9.10 after W06-053's relaunch mid-session: the same line, dropped the same way. Fixed with W06-018 on `review/w06-fixes-5`: a session still open on disk for the strap owns the stream (`ImuSessionFileStore.hasOpenWindow`). V0: `testASessionStillOpenOnDiskOwnsTheStream` on the old code logged the fail-safe line and formed both stop commands; it passes after. That test's manager already has its collector, so it models the frames after the store is built, not the relaunch's first frames, which arrive before it: W06-054 completes the fix. V3 due: a relaunch mid-session logs no Raw IMU line | | | W06-053 | S1 | Fixed | Closing the Raw Data Collector's marker sheet ends the app, by Save or by Cancel: the sheet edits `markerDraft` through `Binding($markerDraft)`, which force-unwraps on every read, and SwiftUI reads it again while the sheet closes, after the close has set the draft to nil | `RawDataCollectorView.swift` marker sheet | Found through W06-051 and the 11.9.10 strap run (2026-09-25): both marker saves on record, one on 11.9.9 and one on 11.9.10, were followed by an app death 2 to 4 s later, and a marker-free session on 11.9.10 had none. V0: a simulator app running the sheet's code verbatim, opening it as Add marker does and closing it as Save or Cancel does, crashed by the second close in 3 of 3 runs (two with Save, one with Cancel), in `BindingOperations.ForceUnwrapping.get`, the unwrap that `Binding($markerDraft)` installs. The 11.9.10 death lost the three seconds of IMU still held in memory; they came back only because a history sync ran after Stop and the strap's v21 history still held them. Fix: the sheet is its own view and edits its own `@State` copy of the draft; Save, Cancel and Delete hand the draft back through closures, so nothing unwraps `markerDraft` once it is nil. V1: the same harness running the fixed sheet, copied from the file, survives 39 closes (12 saves, 12 cancels, and 15 mixed with deletes) with the right marker count; `OptionalBindingUnwrapTests` finds no `Binding($` in the app sources and fails at the old line with the fix reverted; `StrandTests` (1,992) pass and Strand and NOOPiOS (no watch) build. V3 pending: markers added, edited, cancelled and deleted during a strap session, with no app death | | @@ -142,6 +142,7 @@ exactly what was tested on hardware. | W06-065 | S4 | Fixed | `FrameRouter.commandResponsePayload` reads the declared length from bytes 1–2 instead of 2–3, so on every real 5/MG reply the end it computes is past the frame and it returns nil; the Test Centre GET_HELLO identity probe (#1303), its only caller, has never printed | `FrameRouter.swift` `commandResponsePayload` | Found while decoding W06-001. V0: the helper, run verbatim on a real GET_CLOCK reply (28 bytes, declared length 20) reads length 5121 and returns nil; on a SET_CLOCK reply, 3073 and nil. The 13 owner strap logs carry Test Centre connection lines and decoded hellos, and none has a #1303 line. Same pattern swept: no other site reads a 5/MG length there. Fixed: the helper reads the declared length at bytes 2–3 and ends the payload at the CRC32 trailer. V1: `FrameRouterCommandPayloadTests` (two real replies, two short frames) pass; with the old offset the two real-reply tests fail and the short-frame test passes either way (V2 corrected an earlier "all three"). V3: a connect with Test Centre → Connection on logs the #1303 line | | | W06-066 | S4 | Fixed | W06-065 revived the Test Centre GET_HELLO probe (#1303) with two faults: it fires on the PENDING acknowledgement, whose all-zero block reports "none", which the probe reads as "no ASCII serial here"; and it labels the device name at offset 16, the decoder's coordinate (payload from frame byte 11), while this payload starts at byte 13, so the name sits at 14 | `FrameRouter.swift` GET_HELLO probe | Found by the V2 review of the clock batch, with an oracle over the old and new helpers; the owner logs hold 15 PENDING hello replies. Privacy unchanged: the probe withholds non-serial runs either way. Fixed: the probe runs only on a SUCCESS reply and passes `knownNameOffset: 14`. V1: `testTheHelloProbeLabelsTheNameWhereTheDecoderReadsIt` and `testTheHelloProbeIgnoresThePendingAcknowledgement` (synthetic replies, the name placed where the decoder reads it) pass, and both fail with the gate and the offset reverted | | | W06-067 | S4 | Fixed | Two lines of the new clock check can claim what did not happen: "GET_CLOCK sent" printed even when `send` wrote nothing, and after a Bluetooth power-off inside the 10 s window, which never reaches `didDisconnectPeripheral` and so leaves the check's token alive, the timeout logged "setting the clock without a reading" on a dead link | `BLEManager.swift` `beginStrapClockCheck` | Found by the V2 review of the clock batch. V0 by construction: the first line followed the send unconditionally, and the power-off path resets no per-link state (only `didDisconnectPeripheral` does); no test seam reaches the private handshake. The handshake flags reach the same state the old code set at the handshake, so only the log claims were new. Fixed: the sent line prints only when a sequence came back, with a line saying the check waits out its timeout otherwise; the timeout sets the clock only on a connected link and otherwise says it is neither read nor set, then settles the flags as before. V1: both app builds | | +| W06-068 | S4 | Fixed | The clock batch's own text overstated or went stale: "20 replies" where the logs hold 12 distinct ones; "seven zero bytes" stated as the layout when the GETs ran in the same millisecond as their SETs; the linux-capture tools said they mirror the app's `StrapClock` (they read stream timestamps, and `whoop_setclock` writes by default); a test comment, two restore-flag comments and CONTRIBUTING still described the old set-every-connect order; the Backfiller copy still claimed a byte-identical Android twin; and the `validityFloor` doc cited the 146/147 contract as if it covered command 11 | `StrapClock.swift`, `FrameRouter.swift`, `BLEManager.swift`, `Backfiller.swift`, `BackfillerSessionTallyTests.swift`, `Tools/linux-capture/`, `docs/BLE_REVERSE_ENGINEERING.md`, `docs/CONTRIBUTING.md` | Found by the V2 review of the clock batch; each item checked against the logs or the code before the edit (12 distinct GET frames; the tools' defaults are 0 and 30 s). Fixed: each text now says what was observed. V1: `doc_comment_lint`, `check_source_references` and the linux-capture tests pass | | ## Log @@ -208,3 +209,9 @@ exactly what was tested on hardware. done when that check settles. W06-065 (found while decoding) revives the #1303 hello probe. `WhoopProtocol` tests, both app builds and `StrandTests` pass. Next: V2 by an independent reviewer, then a strap run: a plain connect, a relaunch during a Raw Data Collector session, and a connect with Test Centre → Connection on. +- 2026-09-26 — V2 of the clock batch by an independent reviewer: the fix holds (oracle over 2 M cases; state machine, + ordering and sibling sweep all pass) and the strap's console confirms the late-set mechanism. Its S4 findings are + W06-066 (the revived hello probe), W06-067 (two clock log claims) and W06-068 (counts and stale text), all fixed on + the branch; W06-065's V1 was corrected. Two open questions go to the V3 strap run: a lost read reply, and drift. + Its note that the Health pill reads "Pairing…" during the check is left as is: the pill shows only before a strap's + first sync, the window was already there before the handshake, and the handshake is genuinely not done. From e2d2845f95257ac7f41dfd76c58a290353f924e4 Mon Sep 17 00:00:00 2001 From: Artur Iskakov Date: Sat, 26 Sep 2026 13:34:50 +0200 Subject: [PATCH 07/38] =?UTF-8?q?docs(review):=20record=20the=20clock=20ba?= =?UTF-8?q?tch's=20code=20review=20as=20W06-069=20=E2=80=A6=20W06-083?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fourteen findings from a /code-review of review/w06-clock-read-first, each with its V0 result, and W06-083, found while checking W06-069: a Bluetooth power-off runs none of the per-link teardown. --- docs/review/workstreams/W06-ble-collect.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/docs/review/workstreams/W06-ble-collect.md b/docs/review/workstreams/W06-ble-collect.md index 64f70555c5..c71bd208ce 100644 --- a/docs/review/workstreams/W06-ble-collect.md +++ b/docs/review/workstreams/W06-ble-collect.md @@ -143,6 +143,21 @@ exactly what was tested on hardware. | W06-066 | S4 | Fixed | W06-065 revived the Test Centre GET_HELLO probe (#1303) with two faults: it fires on the PENDING acknowledgement, whose all-zero block reports "none", which the probe reads as "no ASCII serial here"; and it labels the device name at offset 16, the decoder's coordinate (payload from frame byte 11), while this payload starts at byte 13, so the name sits at 14 | `FrameRouter.swift` GET_HELLO probe | Found by the V2 review of the clock batch, with an oracle over the old and new helpers; the owner logs hold 15 PENDING hello replies. Privacy unchanged: the probe withholds non-serial runs either way. Fixed: the probe runs only on a SUCCESS reply and passes `knownNameOffset: 14`. V1: `testTheHelloProbeLabelsTheNameWhereTheDecoderReadsIt` and `testTheHelloProbeIgnoresThePendingAcknowledgement` (synthetic replies, the name placed where the decoder reads it) pass, and both fail with the gate and the offset reverted | | | W06-067 | S4 | Fixed | Two lines of the new clock check can claim what did not happen: "GET_CLOCK sent" printed even when `send` wrote nothing, and after a Bluetooth power-off inside the 10 s window, which never reaches `didDisconnectPeripheral` and so leaves the check's token alive, the timeout logged "setting the clock without a reading" on a dead link | `BLEManager.swift` `beginStrapClockCheck` | Found by the V2 review of the clock batch. V0 by construction: the first line followed the send unconditionally, and the power-off path resets no per-link state (only `didDisconnectPeripheral` does); no test seam reaches the private handshake. The handshake flags reach the same state the old code set at the handshake, so only the log claims were new. Fixed: the sent line prints only when a sequence came back, with a line saying the check waits out its timeout otherwise; the timeout sets the clock only on a connected link and otherwise says it is neither read nor set, then settles the flags as before. V1: both app builds | | | W06-068 | S4 | Fixed | The clock batch's own text overstated or went stale: "20 replies" where the logs hold 12 distinct ones; "seven zero bytes" stated as the layout when the GETs ran in the same millisecond as their SETs; the linux-capture tools said they mirror the app's `StrapClock` (they read stream timestamps, and `whoop_setclock` writes by default); a test comment, two restore-flag comments and CONTRIBUTING still described the old set-every-connect order; the Backfiller copy still claimed a byte-identical Android twin; and the `validityFloor` doc cited the 146/147 contract as if it covered command 11 | `StrapClock.swift`, `FrameRouter.swift`, `BLEManager.swift`, `Backfiller.swift`, `BackfillerSessionTallyTests.swift`, `Tools/linux-capture/`, `docs/BLE_REVERSE_ENGINEERING.md`, `docs/CONTRIBUTING.md` | Found by the V2 review of the clock batch; each item checked against the logs or the code before the edit (12 distinct GET frames; the tools' defaults are 0 and 30 s). Fixed: each text now says what was observed. V1: `doc_comment_lint`, `check_source_references` and the linux-capture tests pass | | +| W06-069 | S3 | Reproduced | W06-067's power-off guard reads `state.connected`, which no power-off path clears, so the timeout still logs "setting the clock without a reading" and calls `setStrapClock()` on a dead link, its "link is down" branch never runs, and a link that comes back inside the 10 s window gets a set without a reading from the old link's timer | `BLEManager.swift` `beginStrapClockCheck` timeout | Found by the clock batch's code review. V0: the only `state.connected = false` writes are `didDisconnectPeripheral`, `release`, `prepareForModelSwitch` and restore, and `centralManagerDidUpdateState(.poweredOff)` only sets `lastSyncError`; CoreBluetooth on iOS does not call `didDisconnectPeripheral` for a power-off (Apple Developer Forums thread 30613, bluejay issue 211), and a real disconnect bumps the token first, so the else branch cannot run on either path. The root cause is W06-083 | | +| W06-070 | S4 | Not a bug | On a strap that never answers GET_CLOCK, every connect waits the full 10 s before the handshake settles, and the Sync Strap shortcut (8 s wait) then reports "will sync when connected" | `BLEManager.swift` `beginStrapClockCheck`; `AppModel+iOS.swift` `startStrapSyncFromShortcut` | Found by the clock batch's code review, on the premise that 50.38.1.0 is documented as not serving command 11. V0: that premise is this batch's own misreading. The upstream decoder comment said GET_CLOCK was left undecoded because it was unneeded, not that the firmware ignores it, and no log or doc shows a 5/MG strap that stays silent to command 11. The wait is the designed cost for such a strap, and the shortcut still runs the sync through `armPendingManualSync`. The misread comment is corrected under W06-080 | | +| W06-071 | S3 | Reproduced | The 1.5 s settle before SEND_HISTORICAL_DATA is counted from the handshake, not from the SET_CLOCK, so a set sent 1.5 s or more after the handshake (always on the timeout, and after a slow read) is followed by the offload request in the same main-queue turn, against the hardware-validated set → 1.5 s → offload order | `BLEManager.swift` `strapClockSettled` | Found by the clock batch's code review. V0: the delay is `max(0, 1.5 − elapsed since the handshake)`; on the timeout, elapsed ≥ 10 s gives 0, and a `.set` verdict after a 4 s relaunch round trip gives 0. The deleted comment said "The 1.5s deferral below keeps clock-before-history" | | +| W06-072 | S4 | Not a bug | The timeout calls `strapClockSettled()` even in the branch that has just said the link is down, which sets `historyReady` and bumps `connectSettled` for a gone link | `BLEManager.swift` `beginStrapClockCheck` timeout | Found by the clock batch's code review. V0: that branch runs only when the link went away without `didDisconnectPeripheral`, which is a power-off (W06-083). The reconnect after one inherits `whoop5SessionStarted` and skips the 5/MG handshake, so if the old check never settled, the new link would never set `connectHandshakeDone` and its backfill would stay blocked for the whole link. Settling is the safe choice until W06-083 gives a power-off the per-link teardown; the sends it unblocks on a dead link are dropped by `send`'s guard, as they were when the handshake set these flags directly | | +| W06-073 | S3 | Reproduced | No test covers the app-layer half of W06-050: that a `.settle` verdict sends no SET_CLOCK, that the handshake waits for the check, and that the timeout sets | `BLEManager.swift` `handleStrapClockReply`, `beginStrapClockCheck` | Found by the clock batch's code review. V0: the three mutations it names (`setStrapClock()` in the `.settle` case, `strapClockSettled()` called straight from `beginStrapClockCheck`, `strapClockSettled()` dropped from the `.set` case) touch only private app code that no test reaches; the five recorded mutations were all inside `StrapClock` | | +| W06-074 | S4 | Reproduced | `judge` takes the round trip from two wall-clock samples, so a phone clock stepped back during the read collapses the range to `sentAt` and a correct strap is judged off and set, the spurious step W06-050 exists to prevent; a test pins the narrowing | `StrapClock.swift` `judge`, `Check` | Found by the clock batch's code review. V0, oracle (`swiftc -O StrapClock.swift main.swift`): phone 4 s fast, stepped back mid-read, strap correct, `judge(sentAt: T+4, receivedAt: T+0.2)` → `off(low: -4.0, high: -3.0)` | | +| W06-075 | S4 | Reproduced | The 5/MG set stamps whole seconds with zero subseconds, so each set leaves the strap up to 1 s behind before any processing delay; with a set the strap applies late, the next connect is then judged off and set again | `BLEManager.swift` `setStrapClock` | Found by the clock batch's code review. V0, oracle over 1,000 stamp phases × 10 read phases, set applied 2.3 s late, next read with a 0.1 s round trip: the floor stamp is judged off on the next connect in 6,792 of 10,000 cases, a stamp rounded to the nearest second in 2,443. A set applied promptly is never judged off on the next connect either way; the residual comes from the late apply itself | | +| W06-076 | S4 | Reproduced | `knownNameOffset: 14` hard-codes a protocol offset in app code: the decoder's name offset 16 minus the two-byte shift between two payload coordinates for one reply | `FrameRouter.swift` GET_HELLO probe | Found by the clock batch's code review. V0: the decoder reads the name at `pay[16]` (payload from frame byte 11) in `decodeWhoop5CommandResponse`, and FrameRouter passes a literal 14; nothing ties the two, so a moved name offset would leave the probe labelling the wrong run. Against the AGENTS.md rule that protocol facts live in the decoders | | +| W06-077 | S4 | Reported | The declared-length body end is computed in three places (`parseFrame`, `FrameRouter.commandResponsePayload`, `StrapClock.decodeReply`), and one GET_CLOCK reply goes through two decoders | `Interpreter.swift`, `FrameRouter.swift`, `StrapClock.swift` | Found by the clock batch's code review. Design, no wrong output today: all three read bytes 2–3 and agree. A dedupe is a refactor, which never shares a fix PR; proposed for Phase 5 | | +| W06-078 | S4 | Reported | `sendReturningSequence` infers whether a write happened from a side effect on `seq`, not from anything `send` reports | `BLEManager.swift` `sendReturningSequence` | Found by the clock batch's code review. Design, no wrong output today: every `send` path that writes bumps `seq` once after its guards. Having `send` return the framed sequence changes the central send function; proposed for Phase 5 | | +| W06-079 | S4 | Reproduced | The per-connect "GET_CLOCK sent" line and the in-sync verdict go through ungated `log()` on every connect, against the AGENTS.md rule to gate per-connect readouts behind Test Centre and keep only rare events always-on | `BLEManager.swift` `beginStrapClockCheck`, `handleStrapClockReply` | Found by the clock batch's code review. V0: both are plain `log()` calls with no `TestCentre.active` check; the in-sync verdict is the expected outcome of every connect | | +| W06-080 | S4 | Reproduced | Comments still describe the old set-every-connect order or claim what the code does not do: the `armStrapAlarm` doc (the RTC is "set during the connect handshake"), the `send()` allowlist ("SET_CLOCK/GET_CLOCK are MANDATORY before history"), the `send()` guard comment (a power-off "publishes state.connected = false", W06-083), and the decoder comment saying GET_CLOCK is "not served on 50.38.1.0" (W06-070) | `BLEManager.swift`, `Interpreter.swift` | Found by the clock batch's code review (the first two) and while checking W06-069 and W06-070 (the last two). V0: each checked against the code; `centralManagerDidUpdateState` writes no `state.connected` | | +| W06-081 | S4 | Reproduced | W06-068's text for `whoop_setclock.py` ("default 0, so by default it always writes", also in the README and the `--if-drift` help) is false: the check is `abs(drift) <= args.if_drift`, so with the default the tool skips the write whenever the reading equals the phone's whole second | `Tools/linux-capture/whoop_setclock.py`, `Tools/linux-capture/README.md` | Found by the clock batch's code review. V0: `abs(0) <= 0` is true, so a drift of 0 prints "within ±0s threshold" and writes nothing | | +| W06-082 | S4 | Reproduced | `judge` accepts a `threshold` that `Verdict` does not carry, while `describe` always prints `driftThresholdSeconds`, so a non-default threshold would log a threshold the verdict was not judged against | `StrapClock.swift` `judge`, `describe` | Found by the clock batch's code review. V0: `judge(r, …, threshold: 5)` on a strap 7 s off returns `.off`, which `describe` prints as "more than 2 s off". No caller passes a threshold | | +| W06-083 | S3 | Reported | A Bluetooth power-off runs none of the per-link teardown: `didDisconnectPeripheral` does not fire for it on iOS, and `centralManagerDidUpdateState(.poweredOff)` only sets the banner, so `state.connected`, `didBond`, `whoop5SessionStarted`, `connectHandshakeDone` and the rest outlive the link, and the reconnect skips the 5/MG handshake | `BLEManager.swift` `centralManagerDidUpdateState` | Found while checking W06-069. The `send()` guard comment claims a power-off publishes `state.connected = false`; no code does. Connection path, so Phase 3; the strap run can include Bluetooth off and on | | ## Log From 959329346d62287b89f3c731b7ff9cf1ee5743dc Mon Sep 17 00:00:00 2001 From: Artur Iskakov Date: Sat, 26 Sep 2026 13:39:04 +0200 Subject: [PATCH 08/38] test(ble): pin what the app does with each clock-check outcome MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### W06-073 — no test covered the app-layer half of W06-050 - Severity: S3 · Category: test · Upstream-able: yes - Location: `Strand/BLE/BLEManager.swift` `handleStrapClockReply`, `beginStrapClockCheck` - Reproduction: a `.settle` verdict that also sends SET_CLOCK, a handshake that settles at `beginStrapClockCheck`, or a `.set` that never settles, each passed every suite. - Fix: the clock-check state, `beginStrapClockCheck`, `handleStrapClockReply` and the timeout (now `strapClockCheckTimedOut(token:)`) are internal, and `StrapClockCheckTests` drives them on a manager with no strap, where `send` logs "ignored" for each command it is asked to write. - V1: 8 tests pass; each of the three mutations fails at least one. --- Strand/BLE/BLEManager.swift | 47 ++++++----- StrandTests/StrapClockCheckTests.swift | 91 ++++++++++++++++++++++ docs/review/workstreams/W06-ble-collect.md | 2 +- 3 files changed, 121 insertions(+), 19 deletions(-) create mode 100644 StrandTests/StrapClockCheckTests.swift diff --git a/Strand/BLE/BLEManager.swift b/Strand/BLE/BLEManager.swift index 7c35cc5d51..eb02217e2a 100644 --- a/Strand/BLE/BLEManager.swift +++ b/Strand/BLE/BLEManager.swift @@ -1111,8 +1111,9 @@ public final class BLEManager: NSObject, ObservableObject { private var clockRequested = false /// #700: retry count for GET_CLOCK when no correlation establishes before backfill. Capped at 3. private var clockRetries = 0 - /// W06-050: this link's read-first strap clock check (5/MG). Reset on disconnect. - private var strapClockCheck = StrapClock.Check() + /// W06-050: this link's read-first strap clock check (5/MG). Reset on disconnect. Internal so + /// `StrapClockCheckTests` can put a read in flight without a strap. + var strapClockCheck = StrapClock.Check() /// Fences a previous link's clock-check timeout, so it can never settle the next link's handshake. private var strapClockCheckToken = 0 /// When this link's 5/MG handshake ran, so the first offload keeps its ~1.5 s settle delay after it. @@ -6839,7 +6840,10 @@ extension BLEManager: @preconcurrency CBPeripheralDelegate { /// check, and a SET_CLOCK follows only when the reading is invalid or provably more than /// `StrapClock.driftThresholdSeconds` off. A strap that never answers is set without a reading after /// `StrapClock.replyTimeoutSeconds`, as every connect did before, since an un-clocked strap banks nothing. - private func beginStrapClockCheck() { + /// + /// Returns the check's token, which its timeout carries. Internal for `StrapClockCheckTests`. + @discardableResult + func beginStrapClockCheck() -> Int { strapClockCheck = StrapClock.Check() strapClockCheckToken &+= 1 let token = strapClockCheckToken @@ -6854,25 +6858,32 @@ extension BLEManager: @preconcurrency CBPeripheralDelegate { + "\(Int(StrapClock.replyTimeoutSeconds)) s timeout") } DispatchQueue.main.asyncAfter(deadline: .now() + StrapClock.replyTimeoutSeconds) { [weak self] in - guard let self, token == self.strapClockCheckToken, self.strapClockCheck.expire() else { return } - // W06-067: a Bluetooth power-off does not end the link through `didDisconnectPeripheral`, so the - // token can outlive it. Say so rather than claim a set nothing can send; the handshake flags still - // settle, as they did when the handshake set them directly. - if self.state.connected { - self.log("WHOOP 5/MG clock: no GET_CLOCK reply within \(Int(StrapClock.replyTimeoutSeconds)) s — " - + "setting the clock without a reading") - self.setStrapClock() - } else { - self.log("WHOOP 5/MG clock: no GET_CLOCK reply within \(Int(StrapClock.replyTimeoutSeconds)) s, " - + "and the link is down — the clock is neither read nor set") - } - self.strapClockSettled() + self?.strapClockCheckTimedOut(token: token) + } + return token + } + + /// The clock check begun with `token` got no reply in `StrapClock.replyTimeoutSeconds`. A no-op when a + /// reply settled it or a later link replaced it. Internal for `StrapClockCheckTests`. + func strapClockCheckTimedOut(token: Int) { + guard token == strapClockCheckToken, strapClockCheck.expire() else { return } + // W06-067: a Bluetooth power-off does not end the link through `didDisconnectPeripheral`, so the + // token can outlive it. Say so rather than claim a set nothing can send; the handshake flags still + // settle, as they did when the handshake set them directly. + if state.connected { + log("WHOOP 5/MG clock: no GET_CLOCK reply within \(Int(StrapClock.replyTimeoutSeconds)) s — " + + "setting the clock without a reading") + setStrapClock() + } else { + log("WHOOP 5/MG clock: no GET_CLOCK reply within \(Int(StrapClock.replyTimeoutSeconds)) s, " + + "and the link is down — the clock is neither read nor set") } + strapClockSettled() } /// W06-050: fold one GET_CLOCK reply into this link's clock check and act on its verdict. Verified first, - /// since the verdict can send a SET_CLOCK. - private func handleStrapClockReply(_ frame: [UInt8]) { + /// since the verdict can send a SET_CLOCK. Internal for `StrapClockCheckTests`. + func handleStrapClockReply(_ frame: [UInt8]) { guard verifyFrame(frame, family: .whoop5).ok, let reading = StrapClock.decodeReply(frame) else { return } switch strapClockCheck.receive(reading, at: Date().timeIntervalSince1970) { case .notOurs: diff --git a/StrandTests/StrapClockCheckTests.swift b/StrandTests/StrapClockCheckTests.swift new file mode 100644 index 0000000000..30cf07a487 --- /dev/null +++ b/StrandTests/StrapClockCheckTests.swift @@ -0,0 +1,91 @@ +import XCTest +import WhoopProtocol +@testable import Strand + +/// W06-050's app-layer half (W06-073): what `BLEManager` does with each clock-check outcome. `StrapClockTests` +/// pins the verdicts; these pin that a verdict needing no set sends none, that the connect handshake waits for +/// the check, and that a set and the timeout both settle it. The manager has no strap, so `send` writes +/// nothing and logs "send(