diff --git a/Packages/WhoopProtocol/Sources/WhoopProtocol/DataRange.swift b/Packages/WhoopProtocol/Sources/WhoopProtocol/DataRange.swift index 78c1ec358e..ca2ee57e94 100644 --- a/Packages/WhoopProtocol/Sources/WhoopProtocol/DataRange.swift +++ b/Packages/WhoopProtocol/Sources/WhoopProtocol/DataRange.swift @@ -87,4 +87,19 @@ public enum DataRange { guard behind >= 0, behind <= t else { return nil } return behind } + + /// True when a GET_DATA_RANGE COMMAND_RESPONSE is the `PENDING(2)` acknowledgement rather than the + /// answer. The strap replies twice: a short PENDING ack, then the payload with `SUCCESS(1)`. Framing's + /// own result-code table already states it — "2=PENDING precedes SUCCESS on GET_DATA_RANGE + /// (hardware-confirmed, #78 fork)" — but `pagesBehind` had no way to tell the two apart, so the ack + /// (which carries no ring pointers by construction) decoded to nil and logged as a decode FAILURE once + /// per sync. + /// + /// The result byte sits at `cmdOff + 2`, after the echoed opcode and the origin sequence. A frame too + /// short to hold one is not a PENDING ack, so it stays false and the caller keeps its existing + /// too-short handling. + public static func isPendingResponse(_ frame: [UInt8], cmdOff: Int) -> Bool { + guard cmdOff >= 0, cmdOff + 2 < frame.count else { return false } + return frame[cmdOff + 2] == 2 + } } diff --git a/Packages/WhoopProtocol/Tests/WhoopProtocolTests/DataRangeTests.swift b/Packages/WhoopProtocol/Tests/WhoopProtocolTests/DataRangeTests.swift index 18a7058c38..c70ce4d53f 100644 --- a/Packages/WhoopProtocol/Tests/WhoopProtocolTests/DataRangeTests.swift +++ b/Packages/WhoopProtocol/Tests/WhoopProtocolTests/DataRangeTests.swift @@ -126,4 +126,39 @@ final class DataRangeTests: XCTestCase { "pagesBehind for \(h) at cmdOff \(cmdOff) should be \(expected)") } } + + // MARK: - #689 PENDING ack. Byte-parity twin of the Kotlin DataRangeScanTest cases. + // + // GET_DATA_RANGE answers twice and only the second carries the ring pointers. Both frames below are + // real, captured on a WHOOP MG (WS50_r00, fw 50.39.1.0) on 2026-08-28, and they are one request's + // two replies. + + /// The PENDING(2) ack: 20 bytes, result byte at cmdOff+2 is 0x02, no ring pointers in it at all. + func testIsPendingResponse_trueForPendingAck() { + XCTAssertTrue(DataRange.isPendingResponse(hex("aa010c000100271124e8220402000000c391bc3d"), cmdOff: 10)) + } + + /// The SUCCESS(1) answer to that same request. Must NOT be skipped. + func testIsPendingResponse_falseForSuccessAnswer() { + let h = "aa014c00010032d124e92204010100bf0000b4be0000c1be0000b4be00000c000000000002008f00" + XCTAssertFalse(DataRange.isPendingResponse(hex(h), cmdOff: 10)) + } + + /// The reason this pair matters: the ack decodes to nil, so before the guard it logged as a decode + /// failure once per sync while the real answer decoded fine two lines later. + func testPendingAckIsExactlyTheFramePagesBehindCannotDecode() { + let pending = hex("aa010c000100271124e8220402000000c391bc3d") + XCTAssertNil(DataRange.pagesBehind(from: pending, cmdOff: 10)) + XCTAssertTrue(DataRange.isPendingResponse(pending, cmdOff: 10)) + } + + /// A frame too short to hold a result byte is not a PENDING ack; the caller keeps its own handling. + func testIsPendingResponse_falseWhenTooShortForResultByte() { + XCTAssertFalse(DataRange.isPendingResponse([UInt8](repeating: 0, count: 11), cmdOff: 10)) + } + + /// A negative cmdOff is rejected rather than indexing backwards. + func testIsPendingResponse_falseForNegativeCmdOff() { + XCTAssertFalse(DataRange.isPendingResponse([UInt8](repeating: 0, count: 40), cmdOff: -1)) + } } diff --git a/Strand/BLE/BLEManager.swift b/Strand/BLE/BLEManager.swift index e40ff37521..65926cfa80 100644 --- a/Strand/BLE/BLEManager.swift +++ b/Strand/BLE/BLEManager.swift @@ -6161,11 +6161,19 @@ extension BLEManager: @preconcurrency CBPeripheralDelegate { // log was indistinguishable from one where the strap never answered, which is why a broken decode // survived unnoticed. The raw-frame dump above is unconditional for the same reason. If a firmware // revision ever shifts these fields again, the rejection must be visible rather than silent. - if let pages = DataRange.pagesBehind(from: frame, cmdOff: cmdOff) { - log("Strap backlog pages behind: \(pages) (#689 — GET_DATA_RANGE ring backlog, diagnostic only)") - } else { - log("Strap backlog pages behind: not decodable from this frame (#689 — offsets may have moved; " - + "the raw frame above is the input). Diagnostic only, sync is unaffected.") + // ...but NOT on the PENDING(2) ack. GET_DATA_RANGE answers twice — a short ack, then the payload — + // which Framing's result-code table already records ("2=PENDING precedes SUCCESS on + // GET_DATA_RANGE"). The ack carries no ring pointers at all, so decoding it failed every time and + // reported a decode problem that did not exist: one "offsets may have moved" per sync, on healthy + // hardware, pointing the reader at an alignment bug rather than at the ack. Skip it here; the + // SUCCESS frame still logs its failure loudly, which is the case the paragraph above protects. + if !DataRange.isPendingResponse(frame, cmdOff: cmdOff) { + if let pages = DataRange.pagesBehind(from: frame, cmdOff: cmdOff) { + log("Strap backlog pages behind: \(pages) (#689 — GET_DATA_RANGE ring backlog, diagnostic only)") + } else { + log("Strap backlog pages behind: not decodable from this frame (#689 — offsets may have moved; " + + "the raw frame above is the input). Diagnostic only, sync is unaffected.") + } } if let newest = BLEManager.dataRangeNewestUnix(from: frame) { // #695: the sync-affecting side-effects (strapNewestTs, backfill window, LiveState range) only diff --git a/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt b/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt index 689b522c01..d641a35121 100644 --- a/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt +++ b/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt @@ -6640,14 +6640,23 @@ class WhoopBleClient( // logged NOTHING — a strap log was indistinguishable from one where the strap never // answered, which is why a broken decode survived unnoticed. The raw-frame dump above // is unconditional for the same reason. Twin of the Swift branch. - val pagesBehind = com.noop.protocol.DataRange.pagesBehind(frame, cmdOff) - if (pagesBehind != null) { - log("Strap backlog pages behind: $pagesBehind (#689 — GET_DATA_RANGE ring backlog, diagnostic only)") - } else { - log( - "Strap backlog pages behind: not decodable from this frame (#689 — offsets may " + - "have moved; the raw frame above is the input). Diagnostic only, sync is unaffected.", - ) + // ...but NOT on the PENDING(2) ack. GET_DATA_RANGE answers twice — a short ack, then + // the payload — which Framing's result-code table already records ("2=PENDING + // precedes SUCCESS on GET_DATA_RANGE"). The ack carries no ring pointers at all, so + // decoding it failed every time and reported a decode problem that did not exist: + // one "offsets may have moved" per sync on healthy hardware, pointing the reader at + // an alignment bug rather than at the ack. Skip it here; a SUCCESS frame that will + // not decode still logs loudly, which is the case the paragraph above protects. + if (!com.noop.protocol.DataRange.isPendingResponse(frame, cmdOff)) { + val pagesBehind = com.noop.protocol.DataRange.pagesBehind(frame, cmdOff) + if (pagesBehind != null) { + log("Strap backlog pages behind: $pagesBehind (#689 — GET_DATA_RANGE ring backlog, diagnostic only)") + } else { + log( + "Strap backlog pages behind: not decodable from this frame (#689 — offsets may " + + "have moved; the raw frame above is the input). Diagnostic only, sync is unaffected.", + ) + } } dataRangeNewestUnix(frame)?.let { strapNewestTs = it diff --git a/android/app/src/main/java/com/noop/protocol/DataRange.kt b/android/app/src/main/java/com/noop/protocol/DataRange.kt index cefd9ccf3e..a322926531 100644 --- a/android/app/src/main/java/com/noop/protocol/DataRange.kt +++ b/android/app/src/main/java/com/noop/protocol/DataRange.kt @@ -97,4 +97,21 @@ object DataRange { if (behind < 0L || behind > t) return null return behind } + + /** + * True when a GET_DATA_RANGE COMMAND_RESPONSE is the `PENDING(2)` acknowledgement rather than the + * answer. The strap replies twice: a short PENDING ack, then the payload with `SUCCESS(1)`. Framing's + * own result-code table already states it — "2=PENDING precedes SUCCESS on GET_DATA_RANGE + * (hardware-confirmed, #78 fork)" — but [pagesBehind] had no way to tell the two apart, so the ack + * (which carries no ring pointers by construction) decoded to null and logged as a decode FAILURE once + * per sync. + * + * The result byte sits at `cmdOff + 2`, after the echoed opcode and the origin sequence. A frame too + * short to hold one is not a PENDING ack, so it stays false and the caller keeps its existing + * too-short handling. Twin of the Swift `DataRange.isPendingResponse`. + */ + fun isPendingResponse(frame: ByteArray, cmdOff: Int): Boolean { + if (cmdOff < 0 || cmdOff + 2 >= frame.size) return false + return (frame[cmdOff + 2].toInt() and 0xFF) == 2 + } } diff --git a/android/app/src/test/java/com/noop/ble/DataRangeScanTest.kt b/android/app/src/test/java/com/noop/ble/DataRangeScanTest.kt index 674ac299ec..3f26a4224e 100644 --- a/android/app/src/test/java/com/noop/ble/DataRangeScanTest.kt +++ b/android/app/src/test/java/com/noop/ble/DataRangeScanTest.kt @@ -2,7 +2,9 @@ package com.noop.ble import com.noop.protocol.DataRange import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue import org.junit.Test /** @@ -156,4 +158,42 @@ class DataRangeScanTest { ) } } + + // #689 PENDING ack: GET_DATA_RANGE answers twice and only the second carries the ring pointers. + // Byte-parity twin of the Swift DataRangeTests cases. Both frames are real, captured on a WHOOP MG + // (WS50_r00, fw 50.39.1.0) on 2026-08-28; the pair below is one request's two replies. + + /** The PENDING(2) ack: 20 bytes, result byte at cmdOff+2 is 0x02, no ring pointers in it at all. */ + @Test fun `isPendingResponse true for the PENDING ack`() = + assertTrue(DataRange.isPendingResponse(hex("aa010c000100271124e8220402000000c391bc3d"), 10)) + + /** The SUCCESS(1) answer to that same request. Must NOT be skipped. */ + @Test fun `isPendingResponse false for the SUCCESS answer`() = + assertFalse( + DataRange.isPendingResponse( + hex( + "aa014c00010032d124e92204010100bf0000b4be0000c1be0000b4be00000c00000000" + + "0002008f00", + ), + 10, + ), + ) + + /** + * The reason this pair matters: the ack decodes to null, so before the guard it logged as a decode + * failure once per sync while the real answer decoded fine two lines later. + */ + @Test fun `the PENDING ack is exactly the frame pagesBehind cannot decode`() { + val pending = hex("aa010c000100271124e8220402000000c391bc3d") + assertNull(DataRange.pagesBehind(pending, 10)) + assertTrue(DataRange.isPendingResponse(pending, 10)) + } + + /** A frame too short to hold a result byte is not a PENDING ack; the caller keeps its own handling. */ + @Test fun `isPendingResponse false when too short for a result byte`() = + assertFalse(DataRange.isPendingResponse(ByteArray(11), 10)) + + /** A negative cmdOff is rejected rather than indexing backwards. */ + @Test fun `isPendingResponse false for a negative cmdOff`() = + assertFalse(DataRange.isPendingResponse(ByteArray(40), -1)) }