From 32bc007397fb0c002ca33831fab72e1004bac69b Mon Sep 17 00:00:00 2001 From: Fanboynz Date: Wed, 26 Aug 2026 13:24:06 +1200 Subject: [PATCH 1/4] devices: attribute firmware to the strap it came from, not the last to connect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A WHOOP 5/MG shows 41.17.6.0 in the Devices window — the 4.0's firmware. The persisted value lives under ONE global key, "noop.lastFirmware", written on any WHOOP connect and read back for whichever strap is active. On a single-strap install those are the same strap, which is why this went unnoticed; with two paired it reports the other one. The strap log had it worse: its header printed that global key with no live value and no device qualification at all, so a 5/MG log named a 4.0's firmware outright. Resolve per device instead, by one rule shared across both platforms: live handshake value, else THIS device's own persisted value, else the legacy global key ONLY when a single device is paired — where it cannot have come from anything else. Otherwise nothing, because "unknown" is a better answer than another strap's number. The single-device carry is what stops an existing install regressing to "unknown" on upgrade. Android is wired end to end: the write is keyed by the address THIS connection used (mirroring #1527's rule that a stamp belongs to the identity the strap adopted, not to whatever is active now), and the Devices card, the strap-log header and the #1633 inventory all read per device. The inventory line gains fw=, so a two-strap log states each strap's firmware. Apple is CORRECT but not yet complete, and the difference is structural: FrameRouter has no peripheral identity to key a write on, and LiveState carries none either, so giving it one is a BLE-path change wanting its own review and hardware verification. Until then Apple writes nothing per-device and the same rule simply refuses the global key whenever more than one device is paired — so it stops mis-attributing without yet being able to say more. The one place still reading the raw global is Swift's sync prefs-only strapStateLines header, which is marked in place and superseded by the Devices block in the same export. Both platforms, byte-identical across nine attribution cases and the inventory line, diffed rather than eyeballed. 7 attribution tests and 5 inventory tests per platform, including the reported bug as a test: two straps, no per-device value, global present -> null. Verified: compileFullDebugKotlin clean; 138 JVM tests green; doc_comment_lint and i18n_audit clean. Swift edits are app-target and need app-build. --- Strand/BLE/FirmwareAttribution.swift | 51 +++++++++++++++++ Strand/Screens/DevicesView.swift | 16 ++++-- Strand/System/DebugDataDiagnostics.swift | 20 ++++++- Strand/System/DeviceInventory.swift | 2 + StrandTests/DeviceInventoryTests.swift | 10 ++-- StrandTests/FirmwareAttributionTests.swift | 49 ++++++++++++++++ .../java/com/noop/ble/FirmwareAttribution.kt | 44 +++++++++++++++ .../main/java/com/noop/ble/WhoopBleClient.kt | 5 +- .../com/noop/testcentre/AndroidDiagnostics.kt | 23 +++++++- .../com/noop/testcentre/DeviceInventory.kt | 4 +- .../main/java/com/noop/ui/DevicesScreen.kt | 16 ++++-- .../src/main/java/com/noop/ui/MainActivity.kt | 14 +++++ .../com/noop/ble/FirmwareAttributionTest.kt | 56 +++++++++++++++++++ .../noop/testcentre/DeviceInventoryTest.kt | 11 ++-- 14 files changed, 298 insertions(+), 23 deletions(-) create mode 100644 Strand/BLE/FirmwareAttribution.swift create mode 100644 StrandTests/FirmwareAttributionTests.swift create mode 100644 android/app/src/main/java/com/noop/ble/FirmwareAttribution.kt create mode 100644 android/app/src/test/java/com/noop/ble/FirmwareAttributionTest.kt diff --git a/Strand/BLE/FirmwareAttribution.swift b/Strand/BLE/FirmwareAttribution.swift new file mode 100644 index 0000000000..2fe40bf353 --- /dev/null +++ b/Strand/BLE/FirmwareAttribution.swift @@ -0,0 +1,51 @@ +import Foundation + +/// Which firmware string belongs to a device, given what is known about it. +/// +/// The persisted firmware used to live under one global key, written on any WHOOP connect and read back +/// for whichever strap was active. On a single-strap install those are the same strap, which is why it +/// went unnoticed; with two straps paired it reports the OTHER strap's firmware — a WHOOP 5/MG showing a +/// 4.0's 41.17.6.0 because the 4.0 connected last. +/// +/// The rule, in order: +/// - `live` wins: it came from THIS connection's handshake. +/// - `perDevice` next: this device's own persisted value, from a previous connection. +/// - `legacyGlobal` ONLY when `pairedCount` is 1. The old global key cannot say which strap it belongs +/// to, so it is trustworthy exactly when there is only one strap it could have come from. This keeps a +/// single-strap install reading correctly across the upgrade instead of showing "unknown" until the +/// next connect; a multi-strap install refuses it, which is the bug. +/// - otherwise nil — "not known yet" is the honest answer, and better than another strap's number. +/// +/// Pure so the attribution is unit-tested without defaults, a strap, or a registry. Kotlin twin: +/// `com.noop.ble.resolveFirmware`. +enum FirmwareAttribution { + + static func resolve(live: String?, + perDevice: String?, + legacyGlobal: String?, + pairedCount: Int) -> String? { + if let live, !live.isBlank { return live } + if let perDevice, !perDevice.isBlank { return perDevice } + if let legacyGlobal, !legacyGlobal.isBlank, pairedCount == 1 { return legacyGlobal } + return nil + } + + /// The per-device defaults key for a persisted firmware string. + /// + /// Keyed on the BLE peripheral identifier rather than the registry id: the BLE layer knows the + /// peripheral it connected to, and resolving it to a registry row there would repeat the mis-mapping + /// #1527 fixed for `lastSeen` (stamping "the active row" records a sighting of a strap that was never + /// connected). The registry row carries the same identifier, so the read side resolves without + /// guessing. + /// + /// Returns nil for a blank identifier, so a caller with none writes nothing rather than writing to a + /// key that belongs to no device. + static func prefKey(peripheralId: String?) -> String? { + guard let p = peripheralId?.trimmingCharacters(in: .whitespaces), !p.isEmpty else { return nil } + return "noop.lastFirmware.\(p.lowercased())" + } +} + +private extension String { + var isBlank: Bool { trimmingCharacters(in: .whitespaces).isEmpty } +} diff --git a/Strand/Screens/DevicesView.swift b/Strand/Screens/DevicesView.swift index 15b7944314..d00f23aab2 100644 --- a/Strand/Screens/DevicesView.swift +++ b/Strand/Screens/DevicesView.swift @@ -193,10 +193,18 @@ private struct DevicesContent: View { // handshake, so a non-WHOOP active device (Oura) must NOT inherit it. Single last-connected- // strap key, so a not-yet-connected active strap can briefly show the other strap's build on // a multi-WHOOP install until it republishes. Twin of Android. - liveFirmware: device.status == .active - ? (live.strapFirmware - ?? (SourceCoordinator.isWhoop(device) ? UserDefaults.standard.string(forKey: "noop.lastFirmware") : nil)) - : nil, + // #1633 follow-up: resolve against THIS device, never the last strap to connect. The old + // fallback read one global key, so with two straps paired a 5/MG reported the 4.0's firmware. + // The legacy key is honoured only when a single device is paired, where it cannot belong to + // anything else. + liveFirmware: FirmwareAttribution.resolve( + live: device.status == .active ? live.strapFirmware : nil, + perDevice: SourceCoordinator.isWhoop(device) + ? FirmwareAttribution.prefKey(peripheralId: device.peripheralId) + .flatMap { UserDefaults.standard.string(forKey: $0) } : nil, + legacyGlobal: SourceCoordinator.isWhoop(device) + ? UserDefaults.standard.string(forKey: "noop.lastFirmware") : nil, + pairedCount: registry.devices.count), // Historical record layout (v24/v25 on WHOOP 4.0) observed from this connection's // backfill. Distinct from the strap firmware build shown as FW. liveHistoryLayout: (device.status == .active && live.connected) ? live.strapRange?.firmwareLayout : nil, diff --git a/Strand/System/DebugDataDiagnostics.swift b/Strand/System/DebugDataDiagnostics.swift index f0ab455ec0..ae92d97d16 100644 --- a/Strand/System/DebugDataDiagnostics.swift +++ b/Strand/System/DebugDataDiagnostics.swift @@ -37,6 +37,12 @@ enum DebugDataDiagnostics { let model = WhoopModel(rawValue: d.string(forKey: "selectedWhoopModel") ?? "")?.displayName ?? "unknown (never paired)" lines.append("Model: \(model)") + // NOTE: still the legacy GLOBAL key, and therefore the last strap to connect rather than this + // device's own firmware. strapStateLines is sync and prefs-only by contract (the scheduled export + // calls it with no store), and the per-device rule needs a registry read for pairedCount. The + // Devices block further down IS resolved per device, so a multi-strap export carries the correct + // per-device value there; this line is superseded by it and wants the same follow-up as the Apple + // write site, which has no peripheral identity to key on today. lines.append("Firmware: \(d.string(forKey: "noop.lastFirmware") ?? "unknown (connect to record)")") let syncSec = d.double(forKey: "lastSyncedAt") lines.append("Last sync: \(syncSec > 0 ? relTime(Date().timeIntervalSince1970 - syncSec) : "never")") @@ -109,8 +115,20 @@ enum DebugDataDiagnostics { if let invStore = await repo.storeHandle() { let invRegistry = DeviceRegistryStore(dbQueue: invStore.registryWriter) let invRows = ((try? invRegistry.all()) ?? []).map { + // Firmware resolved by the same rule as the Devices card: this device's own persisted + // value when there is one, and the LEGACY global key only when a single device is paired + // (it cannot have come from anything else). Apple does not yet write the per-device key — + // the write site has no peripheral identity to key on — so today this yields the global + // value for a single-strap install and "unknown" for a multi-strap one, which is honest + // rather than another strap's number. InventoryRow(id: $0.id, brand: $0.brand, model: $0.model, - status: $0.status.rawValue, lastSeenAt: $0.lastSeenAt) + status: $0.status.rawValue, lastSeenAt: $0.lastSeenAt, + firmware: FirmwareAttribution.resolve( + live: nil, + perDevice: FirmwareAttribution.prefKey(peripheralId: $0.peripheralId) + .flatMap { UserDefaults.standard.string(forKey: $0) }, + legacyGlobal: UserDefaults.standard.string(forKey: "noop.lastFirmware"), + pairedCount: invDevices.count)) } let invActive = (try? invRegistry.activeDeviceId()) ?? nil lines.append(contentsOf: deviceInventoryLines(rows: invRows, diff --git a/Strand/System/DeviceInventory.swift b/Strand/System/DeviceInventory.swift index 2ad669906e..6c72a662ad 100644 --- a/Strand/System/DeviceInventory.swift +++ b/Strand/System/DeviceInventory.swift @@ -7,6 +7,7 @@ struct InventoryRow { let model: String let status: String let lastSeenAt: Int + let firmware: String? } /// The strap log's paired-device inventory: every registry row, which one is ACTIVE, and when each was @@ -46,5 +47,6 @@ func deviceInventoryLines(rows: [InventoryRow], let marker = r.id == activeId ? "ACTIVE" : r.status let seen = r.lastSeenAt > 0 ? relTime(Double(nowSec - r.lastSeenAt)) : "never" return " device id=\(r.id) status=\(marker) brand=\(r.brand) model=\(r.model) lastSeen=\(seen)" + + " fw=\(r.firmware ?? "unknown")" } } diff --git a/StrandTests/DeviceInventoryTests.swift b/StrandTests/DeviceInventoryTests.swift index ddaaa106f6..a66e886d36 100644 --- a/StrandTests/DeviceInventoryTests.swift +++ b/StrandTests/DeviceInventoryTests.swift @@ -17,8 +17,8 @@ final class DeviceInventoryTests: XCTestCase { } private func row(_ id: String, _ status: String, _ seen: Int, - model: String = "WHOOP 4.0") -> InventoryRow { - InventoryRow(id: id, brand: "WHOOP", model: model, status: status, lastSeenAt: seen) + model: String = "WHOOP 4.0", fw: String? = nil) -> InventoryRow { + InventoryRow(id: id, brand: "WHOOP", model: model, status: status, lastSeenAt: seen, firmware: fw) } func testAnEmptyRegistrySaysSoRatherThanPrintingABareHeader() { @@ -36,8 +36,8 @@ final class DeviceInventoryTests: XCTestCase { activeId: "my-whoop", nowSec: now, relTime: rel) XCTAssertEqual(lines, [ "Devices: 2 registered (1 active, 1 paired, 0 archived)", - " device id=my-whoop status=ACTIVE brand=WHOOP model=WHOOP 5.0 / MG lastSeen=3h 10m ago", - " device id=whoop-B status=paired brand=WHOOP model=WHOOP 4.0 lastSeen=10m ago", + " device id=my-whoop status=ACTIVE brand=WHOOP model=WHOOP 5.0 / MG lastSeen=3h 10m ago fw=unknown", + " device id=whoop-B status=paired brand=WHOOP model=WHOOP 4.0 lastSeen=10m ago fw=unknown", ]) } @@ -45,7 +45,7 @@ final class DeviceInventoryTests: XCTestCase { let lines = deviceInventoryLines(rows: [row("whoop-C", "paired", 0)], activeId: nil, nowSec: 100_000, relTime: rel) XCTAssertEqual(lines[1], - " device id=whoop-C status=paired brand=WHOOP model=WHOOP 4.0 lastSeen=never") + " device id=whoop-C status=paired brand=WHOOP model=WHOOP 4.0 lastSeen=never fw=unknown") } func testCountsSplitActivePairedAndArchived() { diff --git a/StrandTests/FirmwareAttributionTests.swift b/StrandTests/FirmwareAttributionTests.swift new file mode 100644 index 0000000000..16e24353f6 --- /dev/null +++ b/StrandTests/FirmwareAttributionTests.swift @@ -0,0 +1,49 @@ +import XCTest +@testable import Strand + +/// Pins firmware attribution. Kotlin twin: `FirmwareAttributionTest`. +/// +/// The reported bug: a WHOOP 5/MG showed 41.17.6.0 — the 4.0's firmware — because the persisted value +/// lived under ONE global key, written on any WHOOP connect and read back for whichever strap was active. +final class FirmwareAttributionTests: XCTestCase { + + func testLiveWinsOverEverything() { + XCTAssertEqual(FirmwareAttribution.resolve(live: "50.1.0.0", perDevice: "49.0.0.0", + legacyGlobal: "41.17.6.0", pairedCount: 2), "50.1.0.0") + } + + func testThisDevicesOwnPersistedValueBeatsTheLegacyGlobal() { + XCTAssertEqual(FirmwareAttribution.resolve(live: nil, perDevice: "50.1.0.0", + legacyGlobal: "41.17.6.0", pairedCount: 2), "50.1.0.0") + } + + func testTheLegacyGlobalIsRefusedWhenMoreThanOneDeviceIsPaired() { + // The exact reported bug: two straps, no per-device value yet for the active one, and the global + // key holds the other strap's firmware. nil is correct; 41.17.6.0 is not. + XCTAssertNil(FirmwareAttribution.resolve(live: nil, perDevice: nil, + legacyGlobal: "41.17.6.0", pairedCount: 2)) + } + + func testTheLegacyGlobalIsHonouredForASingleDeviceInstall() { + XCTAssertEqual(FirmwareAttribution.resolve(live: nil, perDevice: nil, + legacyGlobal: "41.17.6.0", pairedCount: 1), "41.17.6.0") + } + + func testBlankValuesAreTreatedAsAbsentNotAsAnAnswer() { + XCTAssertEqual(FirmwareAttribution.resolve(live: "", perDevice: " ", + legacyGlobal: "41.17.6.0", pairedCount: 1), "41.17.6.0") + XCTAssertNil(FirmwareAttribution.resolve(live: "", perDevice: "", legacyGlobal: "", pairedCount: 1)) + } + + func testThePrefKeyIsPerDeviceAndCaseInsensitiveOnTheAddress() { + XCTAssertEqual(FirmwareAttribution.prefKey(peripheralId: "F1:D4:F7:24:53:DE"), + "noop.lastFirmware.f1:d4:f7:24:53:de") + XCTAssertEqual(FirmwareAttribution.prefKey(peripheralId: "f1:d4:f7:24:53:de"), + FirmwareAttribution.prefKey(peripheralId: "F1:D4:F7:24:53:DE")) + } + + func testNoAddressMeansNoKey() { + XCTAssertNil(FirmwareAttribution.prefKey(peripheralId: nil)) + XCTAssertNil(FirmwareAttribution.prefKey(peripheralId: " ")) + } +} diff --git a/android/app/src/main/java/com/noop/ble/FirmwareAttribution.kt b/android/app/src/main/java/com/noop/ble/FirmwareAttribution.kt new file mode 100644 index 0000000000..984b07fb90 --- /dev/null +++ b/android/app/src/main/java/com/noop/ble/FirmwareAttribution.kt @@ -0,0 +1,44 @@ +package com.noop.ble + +/** + * Which firmware string belongs to a device, given what is known about it. + * + * The persisted firmware used to live under one global key, written on any WHOOP connect and read back + * for whichever strap was active. On a single-strap install those are the same strap, which is why it + * went unnoticed; with two straps paired it reports the OTHER strap's firmware — a WHOOP 5/MG showing a + * 4.0's 41.17.6.0 because the 4.0 connected last. + * + * The rule, in order: + * - [live] wins: it came from THIS connection's handshake. + * - [perDevice] next: this device's own persisted value, from a previous connection. + * - [legacyGlobal] ONLY when [pairedCount] is 1. The old global key cannot say which strap it belongs + * to, so it is trustworthy exactly when there is only one strap it could have come from. This keeps a + * single-strap install reading correctly across the upgrade instead of showing "unknown" until the + * next connect; a multi-strap install refuses it, which is the bug. + * - otherwise null — "not known yet" is the honest answer, and better than another strap's number. + * + * Pure so the attribution is unit-tested without prefs, a strap, or a registry. Swift twin: + * `FirmwareAttribution.resolve`. + */ +internal fun resolveFirmware( + live: String?, + perDevice: String?, + legacyGlobal: String?, + pairedCount: Int, +): String? = live?.takeIf { it.isNotBlank() } + ?: perDevice?.takeIf { it.isNotBlank() } + ?: legacyGlobal?.takeIf { it.isNotBlank() && pairedCount == 1 } + +/** + * The per-device preference key for a persisted firmware string. + * + * Keyed on the BLE peripheral address rather than the registry id: the BLE client knows the address it + * connected to, and resolving it to a registry row there would repeat the mis-mapping #1527 fixed for + * `lastSeen` (stamping "the active row" records a sighting of a strap that was never connected). The + * registry row carries the same address, so the read side resolves without guessing. + * + * Returns null for a blank address, so a caller with no address writes nothing rather than writing to a + * key that belongs to no device. + */ +internal fun firmwarePrefKey(peripheralId: String?): String? = + peripheralId?.trim()?.takeIf { it.isNotEmpty() }?.let { "noop.lastFirmware.${it.lowercase()}" } 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 f4c1cf8285..5da6fdbace 100644 --- a/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt +++ b/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt @@ -5842,7 +5842,10 @@ class WhoopBleClient( if (_state.value.strapFirmware != fw) { _state.update { it.copy(strapFirmware = fw) } // Persist so the debug export can name the firmware offline (state clears on disconnect). - runCatching { NoopPrefs.setLastFirmware(context, fw) } + // Keyed by the address THIS connection used, not globally: a second strap would otherwise + // overwrite the first's value and both would report whichever connected last - a 5/MG showing + // a 4.0's 41.17.6.0. + runCatching { NoopPrefs.setFirmwareFor(context, lastDeviceAddress, fw) } } } val respCmd = parsed.parsed["resp_cmd"] as? String diff --git a/android/app/src/main/java/com/noop/testcentre/AndroidDiagnostics.kt b/android/app/src/main/java/com/noop/testcentre/AndroidDiagnostics.kt index 4d54843a05..688434ffcb 100644 --- a/android/app/src/main/java/com/noop/testcentre/AndroidDiagnostics.kt +++ b/android/app/src/main/java/com/noop/testcentre/AndroidDiagnostics.kt @@ -71,7 +71,23 @@ object AndroidDiagnostics { else -> reportedModel(context)?.displayName ?: "unknown (paired, family not yet detected)" }, ) - add("Firmware: ${com.noop.ui.NoopPrefs.lastFirmware(context) ?: "unknown (connect to record)"}") + // #1633 follow-up: the ACTIVE device's own firmware. This read used to be the bare global key, + // so a two-strap log named whichever strap connected last - a 5/MG log reporting a 4.0's + // 41.17.6.0. resolveFirmware falls back to the legacy key only when one device is paired. + val fwRows = runCatching { + (context.applicationContext as? com.noop.NoopApplication)?.deviceRegistry?.all().orEmpty() + }.getOrDefault(emptyList()) + val fwActive = runCatching { + (context.applicationContext as? com.noop.NoopApplication)?.deviceRegistry?.activeDeviceId() + }.getOrNull() + val fwRow = fwRows.firstOrNull { it.id == fwActive } + val fw = com.noop.ble.resolveFirmware( + live = null, + perDevice = com.noop.ui.NoopPrefs.firmwareFor(context, fwRow?.peripheralId), + legacyGlobal = com.noop.ui.NoopPrefs.lastFirmware(context), + pairedCount = fwRows.size, + ) + add("Firmware: ${fw ?: "unknown (connect to record)"}") val syncSec = com.noop.ui.NoopPrefs.lastSyncAt(context) add("Last sync: ${if (syncSec > 0L) relTime(System.currentTimeMillis() - syncSec * 1000L) else "never"}") // #57: write-health. "Last sync" fires even on an empty/failed offload, so distinguish "rows @@ -105,7 +121,10 @@ object AndroidDiagnostics { // checked against. Name the whole set instead. val invRows = runCatching { (context.applicationContext as? com.noop.NoopApplication)?.deviceRegistry?.all().orEmpty() - .map { InventoryRow(it.id, it.brand, it.model, it.status, it.lastSeenAt) } + .map { + InventoryRow(it.id, it.brand, it.model, it.status, it.lastSeenAt, + com.noop.ui.NoopPrefs.firmwareFor(context, it.peripheralId)) + } }.getOrDefault(emptyList()) val invActive = runCatching { (context.applicationContext as? com.noop.NoopApplication)?.deviceRegistry?.activeDeviceId() diff --git a/android/app/src/main/java/com/noop/testcentre/DeviceInventory.kt b/android/app/src/main/java/com/noop/testcentre/DeviceInventory.kt index 4e0fa7bb6d..3daa3d6663 100644 --- a/android/app/src/main/java/com/noop/testcentre/DeviceInventory.kt +++ b/android/app/src/main/java/com/noop/testcentre/DeviceInventory.kt @@ -7,6 +7,7 @@ internal data class InventoryRow( val model: String, val status: String, val lastSeenAt: Long, + val firmware: String? = null, ) /** @@ -48,6 +49,7 @@ internal fun deviceInventoryLines( return listOf(head) + ordered.map { r -> val marker = if (r.id == activeId) "ACTIVE" else r.status val seen = if (r.lastSeenAt > 0L) relTime((nowSec - r.lastSeenAt) * 1000L) else "never" - " device id=${r.id} status=$marker brand=${r.brand} model=${r.model} lastSeen=$seen" + " device id=${r.id} status=$marker brand=${r.brand} model=${r.model} lastSeen=$seen" + + " fw=${r.firmware ?: "unknown"}" } } diff --git a/android/app/src/main/java/com/noop/ui/DevicesScreen.kt b/android/app/src/main/java/com/noop/ui/DevicesScreen.kt index 5aeb81dc19..f499f3c0b0 100644 --- a/android/app/src/main/java/com/noop/ui/DevicesScreen.kt +++ b/android/app/src/main/java/com/noop/ui/DevicesScreen.kt @@ -258,10 +258,18 @@ fun DevicesScreen( // WHOOP-only: `noop.lastFirmware` is written solely from a WHOOP handshake, so a non-WHOOP // active device (Oura/FTMS) must NOT inherit it. Single-key, so on a multi-WHOOP install a // not-yet-connected active strap can briefly show the other strap's build until it republishes. - liveFirmware = if (device.status == DeviceStatus.active.name) - (live.strapFirmware - ?: if (device.brand.equals("WHOOP", ignoreCase = true)) NoopPrefs.lastFirmware(context) else null) - else null, + // #1633 follow-up: resolve against THIS device, never the last strap to connect. The old + // fallback read one global key, so with two straps paired the 5/MG reported the 4.0's + // firmware. The legacy key is still honoured when exactly one device is paired - then it + // cannot belong to anything else - so a single-strap install does not regress to 'unknown'. + liveFirmware = com.noop.ble.resolveFirmware( + live = if (device.status == DeviceStatus.active.name) live.strapFirmware else null, + perDevice = if (device.brand.equals("WHOOP", ignoreCase = true)) + NoopPrefs.firmwareFor(context, device.peripheralId) else null, + legacyGlobal = if (device.status == DeviceStatus.active.name && + device.brand.equals("WHOOP", ignoreCase = true)) NoopPrefs.lastFirmware(context) else null, + pairedCount = all.size, + ), // Historical record layout from the current backfill, distinct from strap firmware. liveHistoryLayout = if (device.status == DeviceStatus.active.name && live.connected) live.historyLayoutVersion else null, diff --git a/android/app/src/main/java/com/noop/ui/MainActivity.kt b/android/app/src/main/java/com/noop/ui/MainActivity.kt index ae06965ef1..94cde90f66 100644 --- a/android/app/src/main/java/com/noop/ui/MainActivity.kt +++ b/android/app/src/main/java/com/noop/ui/MainActivity.kt @@ -1265,6 +1265,20 @@ object NoopPrefs { if (fw.isNullOrBlank()) remove(KEY_LAST_FIRMWARE) else putString(KEY_LAST_FIRMWARE, fw) }.apply() } + + /** This device's own persisted firmware, keyed by BLE address - see [com.noop.ble.firmwarePrefKey]. + * Null when nothing was ever recorded for that device. */ + fun firmwareFor(context: Context, peripheralId: String?): String? = + com.noop.ble.firmwarePrefKey(peripheralId)?.let { of(context).getString(it, null) } + + /** Record a firmware string against the device it came from. A blank address writes nothing rather + * than writing to a key that belongs to no device. */ + fun setFirmwareFor(context: Context, peripheralId: String?, fw: String?) { + val key = com.noop.ble.firmwarePrefKey(peripheralId) ?: return + of(context).edit().apply { + if (fw.isNullOrBlank()) remove(key) else putString(key, fw) + }.apply() + } } /** diff --git a/android/app/src/test/java/com/noop/ble/FirmwareAttributionTest.kt b/android/app/src/test/java/com/noop/ble/FirmwareAttributionTest.kt new file mode 100644 index 0000000000..555feaede8 --- /dev/null +++ b/android/app/src/test/java/com/noop/ble/FirmwareAttributionTest.kt @@ -0,0 +1,56 @@ +package com.noop.ble + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Test + +/** + * Pins firmware attribution. Swift twin: `FirmwareAttributionTests`. + * + * The reported bug: a WHOOP 5/MG showed 41.17.6.0 — the 4.0's firmware — because the persisted value + * lived under ONE global key, written on any WHOOP connect and read back for whichever strap was active. + */ +class FirmwareAttributionTest { + + @Test + fun `live wins over everything`() { + assertEquals("50.1.0.0", resolveFirmware("50.1.0.0", "49.0.0.0", "41.17.6.0", 2)) + } + + @Test + fun `this device's own persisted value beats the legacy global`() { + assertEquals("50.1.0.0", resolveFirmware(null, "50.1.0.0", "41.17.6.0", 2)) + } + + @Test + fun `the legacy global is REFUSED when more than one device is paired`() { + // The exact reported bug: two straps, no per-device value yet for the active one, and the global + // key holds the other strap's firmware. "unknown" is correct; 41.17.6.0 is not. + assertNull(resolveFirmware(null, null, "41.17.6.0", 2)) + } + + @Test + fun `the legacy global is honoured for a single-device install`() { + // It cannot have come from anything else, so a single-strap install does not regress to + // "unknown" on upgrade before the next connect. + assertEquals("41.17.6.0", resolveFirmware(null, null, "41.17.6.0", 1)) + } + + @Test + fun `blank values are treated as absent, not as an answer`() { + assertEquals("41.17.6.0", resolveFirmware("", " ", "41.17.6.0", 1)) + assertNull(resolveFirmware("", "", "", 1)) + } + + @Test + fun `the pref key is per-device and case-insensitive on the address`() { + assertEquals("noop.lastFirmware.f1:d4:f7:24:53:de", firmwarePrefKey("F1:D4:F7:24:53:DE")) + assertEquals(firmwarePrefKey("f1:d4:f7:24:53:de"), firmwarePrefKey("F1:D4:F7:24:53:DE")) + } + + @Test + fun `no address means no key, so nothing is written to a key owned by no device`() { + assertNull(firmwarePrefKey(null)) + assertNull(firmwarePrefKey(" ")) + } +} diff --git a/android/app/src/test/java/com/noop/testcentre/DeviceInventoryTest.kt b/android/app/src/test/java/com/noop/testcentre/DeviceInventoryTest.kt index 4584526cfe..f58afedab0 100644 --- a/android/app/src/test/java/com/noop/testcentre/DeviceInventoryTest.kt +++ b/android/app/src/test/java/com/noop/testcentre/DeviceInventoryTest.kt @@ -23,8 +23,9 @@ class DeviceInventoryTest { } } - private fun row(id: String, status: String, seen: Long, model: String = "WHOOP 4.0") = - InventoryRow(id, "WHOOP", model, status, seen) + private fun row(id: String, status: String, seen: Long, model: String = "WHOOP 4.0", + fw: String? = null) = + InventoryRow(id, "WHOOP", model, status, seen, fw) @Test fun `an empty registry says so rather than printing a bare header`() { @@ -46,8 +47,8 @@ class DeviceInventoryTest { assertEquals( listOf( "Devices: 2 registered (1 active, 1 paired, 0 archived)", - " device id=my-whoop status=ACTIVE brand=WHOOP model=WHOOP 5.0 / MG lastSeen=3h 10m ago", - " device id=whoop-B status=paired brand=WHOOP model=WHOOP 4.0 lastSeen=10m ago", + " device id=my-whoop status=ACTIVE brand=WHOOP model=WHOOP 5.0 / MG lastSeen=3h 10m ago fw=unknown", + " device id=whoop-B status=paired brand=WHOOP model=WHOOP 4.0 lastSeen=10m ago fw=unknown", ), lines, ) @@ -56,7 +57,7 @@ class DeviceInventoryTest { @Test fun `a never-seen row reads never rather than a huge duration`() { val lines = deviceInventoryLines(listOf(row("whoop-C", "paired", 0L)), null, 100_000L, rel) - assertEquals(" device id=whoop-C status=paired brand=WHOOP model=WHOOP 4.0 lastSeen=never", lines[1]) + assertEquals(" device id=whoop-C status=paired brand=WHOOP model=WHOOP 4.0 lastSeen=never fw=unknown", lines[1]) } @Test From 27066f06637843f140fe2139af0ae7ce86b1eb22 Mon Sep 17 00:00:00 2001 From: Fanboynz Date: Wed, 26 Aug 2026 13:42:35 +1200 Subject: [PATCH 2/4] protocol: say WHY a 5/MG hello decoded no firmware MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Attribution alone cannot make a firmware appear. The 5/MG version is read as four bytes at pay[93] behind a `pay[93] == 50` guard, and the decoder's own comment says both are anchored to a SINGLE capture at 50.38.1.0: the name+token region ahead of the version is fixed-width "on this firmware", re-verifying the offset across firmwares is left as future work, and the guards deliberately fail closed. So a 5/MG that does not match reports nothing — and from a log, two very different causes look identical: - the byte at 93 is not 50: a different generation marker, offset probably still right; - the name+token region is a different width, so byte 93 is NOT the version and the offset has MOVED. This is the case the decoder comment warns about, and the one a user cannot diagnose. Emit the evidence that separates them: payload length, the byte actually at 93, where the printable-ASCII name run ended (the width that shifts everything after it), and a hex window across the region the version should occupy. A capture from an undecoded strap then locates the real offset without another round trip. The guard is NOT loosened. Widening it blindly would render unrelated bytes as a version, which is worse than "unknown" because it looks authoritative — the same reasoning as leaving BEAT_ACCURACY_MIN_FRACTION alone in #1632. Logged once per connection on both platforms (the value is stable), cleared on the same per-connection boundary each already uses: Android's reset(), Swift's `family` didSet. The decoder half is PURE and lives in WhoopProtocol / Framing, so unlike most of this cluster it is covered by tests that run on Linux: 5 twinned cases each, including a truncated payload that must clamp rather than trap, and a payload ending exactly at the window start. Verified: 638 WhoopProtocol tests green on Linux; 1031 Android tests green; the diagnostic diffed Kotlin-vs-Swift across five cases and byte-identical; doc_comment_lint and i18n_audit clean. The app-side logging is app-target on Swift and needs app-build. --- .../FirmwareGateDiagnostic.swift | 35 ++++++++++++ .../Sources/WhoopProtocol/Interpreter.swift | 5 ++ .../FirmwareGateDiagnosticTests.swift | 49 +++++++++++++++++ Strand/BLE/FrameRouter.swift | 14 ++++- .../main/java/com/noop/ble/WhoopBleClient.kt | 15 +++++ .../noop/protocol/FirmwareGateDiagnostic.kt | 39 +++++++++++++ .../main/java/com/noop/protocol/Framing.kt | 5 ++ .../protocol/FirmwareGateDiagnosticTest.kt | 55 +++++++++++++++++++ 8 files changed, 216 insertions(+), 1 deletion(-) create mode 100644 Packages/WhoopProtocol/Sources/WhoopProtocol/FirmwareGateDiagnostic.swift create mode 100644 Packages/WhoopProtocol/Tests/WhoopProtocolTests/FirmwareGateDiagnosticTests.swift create mode 100644 android/app/src/main/java/com/noop/protocol/FirmwareGateDiagnostic.kt create mode 100644 android/app/src/test/java/com/noop/protocol/FirmwareGateDiagnosticTest.kt diff --git a/Packages/WhoopProtocol/Sources/WhoopProtocol/FirmwareGateDiagnostic.swift b/Packages/WhoopProtocol/Sources/WhoopProtocol/FirmwareGateDiagnostic.swift new file mode 100644 index 0000000000..8c338ff9c4 --- /dev/null +++ b/Packages/WhoopProtocol/Sources/WhoopProtocol/FirmwareGateDiagnostic.swift @@ -0,0 +1,35 @@ +import Foundation + +/// Why a WHOOP 5/MG hello carried no firmware version, in enough detail to re-derive the offset. +/// +/// The version is read as four bytes at `pay[93]`, guarded by `pay[93] == 50` ("5.0 generation"). Both +/// the offset and the guard are anchored to a SINGLE capture at firmware 50.38.1.0, and the decoder's own +/// comment says so: the name+token region ahead of the version is fixed-width *on that firmware*, and +/// re-verifying the offset across firmwares is explicitly left as future work. The guards fail closed, so +/// a strap that does not match simply reports nothing. +/// +/// That silence has two very different causes and no way to tell them apart from a log: +/// - the byte at 93 is not 50 — a different generation marker, offset probably still right; +/// - the name+token region is a different width, so byte 93 is not the version at all and the offset has +/// MOVED. This is the one the decoder comment warns about, and the one a user cannot diagnose. +/// +/// So the line carries what distinguishes them: the payload length, the byte actually at 93, where the +/// printable-ASCII device name ended (the run whose width shifts everything after it), and a hex window +/// spanning the region the version should sit in. A capture from an undecoded strap then locates the real +/// offset without another round trip. +/// +/// Deliberately does NOT loosen the guard. Widening it blindly would render unrelated bytes as a version, +/// which is worse than "unknown" because it looks authoritative. +/// +/// Pure, and the window is clamped to the payload, so a short or malformed frame cannot trap. Kotlin +/// twin: `com.noop.protocol.firmwareGateDiagnostic`. +public func firmwareGateDiagnostic(payload: [UInt8], nameEndIndex: Int) -> String { + let at93 = payload.count > 93 ? String(payload[93]) : "n/a" + let lo = min(88, payload.count) + let hi = min(101, payload.count) + let window = lo < hi + ? payload[lo..= 97, pay[93] == 50 { fb.parsed["fw_version"] = .string("\(pay[93]).\(pay[94]).\(pay[95]).\(pay[96])") + } else { + // The guards fail closed by design, which left a strap reporting no firmware with no way to + // say WHY - a different generation byte and a MOVED offset look identical from a log. Carry + // the evidence instead; see `firmwareGateDiagnostic`. + fb.parsed["fw_gate"] = .string(firmwareGateDiagnostic(payload: pay, nameEndIndex: i)) } } } diff --git a/Packages/WhoopProtocol/Tests/WhoopProtocolTests/FirmwareGateDiagnosticTests.swift b/Packages/WhoopProtocol/Tests/WhoopProtocolTests/FirmwareGateDiagnosticTests.swift new file mode 100644 index 0000000000..657bd66577 --- /dev/null +++ b/Packages/WhoopProtocol/Tests/WhoopProtocolTests/FirmwareGateDiagnosticTests.swift @@ -0,0 +1,49 @@ +import XCTest +@testable import WhoopProtocol + +/// Pins the 5/MG firmware-gate diagnostic. Kotlin twin: `FirmwareGateDiagnosticTest`. +/// +/// The decoder reads the version at pay[93] behind a `pay[93] == 50` guard, both anchored to a single +/// 50.38.1.0 capture. The guards fail closed, so a strap that does not match reports nothing — and a +/// different generation byte is indistinguishable from a MOVED offset unless the line says which. +final class FirmwareGateDiagnosticTests: XCTestCase { + + private func payload(count: Int, at93: UInt8) -> [UInt8] { + var p = (0.. 93 { p[93] = at93 } + return p + } + + func testReportsTheByteItActuallySawAndTheExpectedOne() { + let line = firmwareGateDiagnostic(payload: payload(count: 128, at93: 51), nameEndIndex: 27) + XCTAssertTrue(line.contains("at93=51 expected=50"), line) + XCTAssertTrue(line.contains("len=128"), line) + } + + func testCarriesTheNameEndBecauseThatIsWhatMovesTheOffset() { + // The version sits after the name+token region, so where the printable-ASCII name run ended is + // the number that lets a reader re-derive a shifted offset. + let line = firmwareGateDiagnostic(payload: payload(count: 128, at93: 51), nameEndIndex: 31) + XCTAssertTrue(line.contains("nameEnd=31"), line) + } + + func testTheHexWindowSpansTheRegionTheVersionShouldOccupy() { + let line = firmwareGateDiagnostic(payload: payload(count: 128, at93: 51), nameEndIndex: 27) + XCTAssertTrue(line.contains("hex[88..<101]="), line) + // 13 bytes, two hex chars each. + let hex = line.components(separatedBy: "hex[88..<101]=").last ?? "" + XCTAssertEqual(hex.count, 26, hex) + } + + func testAShortPayloadCannotTrapAndSaysSo() { + // A malformed/truncated hello must not crash the decoder; the window clamps and at93 reports n/a. + let line = firmwareGateDiagnostic(payload: [1, 2, 3], nameEndIndex: 2) + XCTAssertTrue(line.contains("at93=n/a"), line) + XCTAssertTrue(line.contains("len=3"), line) + } + + func testAPayloadEndingExactlyAtTheWindowStartYieldsAnEmptyWindow() { + let line = firmwareGateDiagnostic(payload: Array(repeating: 0, count: 88), nameEndIndex: 20) + XCTAssertTrue(line.hasSuffix("hex[88..<88]="), line) + } +} diff --git a/Strand/BLE/FrameRouter.swift b/Strand/BLE/FrameRouter.swift index deafe71b71..7186afea37 100644 --- a/Strand/BLE/FrameRouter.swift +++ b/Strand/BLE/FrameRouter.swift @@ -18,12 +18,16 @@ public final class FrameRouter { // #900: a fresh connection is a fresh capture session — re-arm the per-command raw-frame dump so // each connect can re-capture the disputed COMMAND_RESPONSE prefix once. `family` is set fresh per // connection by BLEManager (connectCore), so this is the per-session reset hook. - didSet { rawDumpedRespCmds.removeAll() } + didSet { rawDumpedRespCmds.removeAll(); loggedFirmwareGate = nil } } /// #900: resp command names (e.g. "GET_BATTERY_LEVEL(26)") whose raw COMMAND_RESPONSE frame has already /// been dumped this connection. The provenance dump fires once per command per session so a 4.0's /// per-poll battery reads don't flood the strap log. Reset when `family` is set at connect. + /// #1634: last firmware-gate line logged, so a stable per-connection value is not repeated on every + /// hello. Cleared alongside the other per-connection routing state. + private var loggedFirmwareGate: String? + private var rawDumpedRespCmds: Set = [] public init(state: LiveState) { @@ -112,6 +116,14 @@ public final class FrameRouter { // Persist so the debug export can name the firmware offline (state clears on disconnect). UserDefaults.standard.set(fw, forKey: "noop.lastFirmware") } + + // #1634: the 5/MG hello decoded no firmware. The guards fail closed by design, so this is the + // only place that can say WHY - a different generation byte vs a MOVED offset. Logged once per + // connection (the value is stable), so a capture from an undecoded strap carries the evidence. + if let gate = parsed.parsed["fw_gate"]?.stringValue, loggedFirmwareGate != gate { + loggedFirmwareGate = gate + state.append(log: gate, domain: .connection) + } // Advertising-name replies (WHOOP 4.0 / Harvard). GET (cmd 76) carries the current name in // its payload; SET (cmd 77) carries only a result byte. The schema has no field decode for // either, so pull them straight from the frame bytes. The COMMAND_RESPONSE inner is 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 5da6fdbace..29e1350c7d 100644 --- a/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt +++ b/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt @@ -2185,6 +2185,10 @@ class WhoopBleClient( /// `batterySource(familyEstablished, connectedFamily)` is the correct order): seeing this true then /// establishes happens-before for the [connectedFamily] write that precedes it at discovery. Swapping /// the two parameters would silently drop that guarantee. + /** #1634: last firmware-gate line logged, so a stable per-connection value is not repeated on every + * hello. Cleared in [reset] with the rest of the per-connection state. */ + @Volatile private var loggedFirmwareGate: String? = null + @Volatile private var familyEstablished = false /// The family actually discovered on the connected peripheral. Drives family-aware frame @@ -5847,6 +5851,16 @@ class WhoopBleClient( // a 4.0's 41.17.6.0. runCatching { NoopPrefs.setFirmwareFor(context, lastDeviceAddress, fw) } } + + // #1634: the 5/MG hello decoded no firmware. The guards fail closed by design, so this is the + // only place that can say WHY - a different generation byte vs a MOVED offset. Logged once per + // connection (the value is stable), so a capture from an undecoded strap carries the evidence. + (parsed.parsed["fw_gate"] as? String)?.let { gate -> + if (loggedFirmwareGate != gate) { + loggedFirmwareGate = gate + log(gate) + } + } } val respCmd = parsed.parsed["resp_cmd"] as? String val result = parsed.parsed["result"] as? String @@ -8289,6 +8303,7 @@ class WhoopBleClient( didBond = false connectHandshakeDone = false familyEstablished = false // the next link re-establishes it at service discovery + loggedFirmwareGate = null seq.set(0) writeQueue.clear() cccdQueue.clear() diff --git a/android/app/src/main/java/com/noop/protocol/FirmwareGateDiagnostic.kt b/android/app/src/main/java/com/noop/protocol/FirmwareGateDiagnostic.kt new file mode 100644 index 0000000000..1279948cbc --- /dev/null +++ b/android/app/src/main/java/com/noop/protocol/FirmwareGateDiagnostic.kt @@ -0,0 +1,39 @@ +package com.noop.protocol + +/** + * Why a WHOOP 5/MG hello carried no firmware version, in enough detail to re-derive the offset. + * + * The version is read as four bytes at `pay[93]`, guarded by `pay[93] == 50` ("5.0 generation"). Both the + * offset and the guard are anchored to a SINGLE capture at firmware 50.38.1.0, and the decoder's own + * comment says so: the name+token region ahead of the version is fixed-width *on that firmware*, and + * re-verifying the offset across firmwares is explicitly left as future work. The guards fail closed, so + * a strap that does not match simply reports nothing. + * + * That silence has two very different causes and no way to tell them apart from a log: + * - the byte at 93 is not 50 — a different generation marker, offset probably still right; + * - the name+token region is a different width, so byte 93 is not the version at all and the offset has + * MOVED. This is the one the decoder comment warns about, and the one a user cannot diagnose. + * + * So the line carries what distinguishes them: the payload length, the byte actually at 93, where the + * printable-ASCII device name ended (the run whose width shifts everything after it), and a hex window + * spanning the region the version should sit in. A capture from an undecoded strap then locates the real + * offset without another round trip. + * + * Deliberately does NOT loosen the guard. Widening it blindly would render unrelated bytes as a version, + * which is worse than "unknown" because it looks authoritative. + * + * Pure, and the window is clamped to the payload, so a short or malformed frame cannot trap. Swift twin: + * `WhoopProtocol.firmwareGateDiagnostic`. + */ +fun firmwareGateDiagnostic(payload: ByteArray, nameEndIndex: Int): String { + val at93 = if (payload.size > 93) (payload[93].toInt() and 0xFF).toString() else "n/a" + val lo = minOf(88, payload.size) + val hi = minOf(101, payload.size) + val window = if (lo < hi) { + payload.copyOfRange(lo, hi).joinToString("") { "%02x".format(it.toInt() and 0xFF) } + } else { + "" + } + return "fw gate: no version decoded — len=${payload.size} at93=$at93 expected=50" + + " nameEnd=$nameEndIndex hex[$lo..<$hi]=$window" +} diff --git a/android/app/src/main/java/com/noop/protocol/Framing.kt b/android/app/src/main/java/com/noop/protocol/Framing.kt index d5d203cbbd..9045b5f6ef 100644 --- a/android/app/src/main/java/com/noop/protocol/Framing.kt +++ b/android/app/src/main/java/com/noop/protocol/Framing.kt @@ -376,6 +376,11 @@ object Framing { if (pay.size >= 97 && (pay[93].toInt() and 0xFF) == 50) { parsed["fw_version"] = "${pay[93].toInt() and 0xFF}.${pay[94].toInt() and 0xFF}." + "${pay[95].toInt() and 0xFF}.${pay[96].toInt() and 0xFF}" + } else { + // The guards fail closed by design, which left a strap reporting no firmware with no way to + // say WHY - a different generation byte and a MOVED offset look identical from a log. Carry + // the evidence instead; see [firmwareGateDiagnostic]. + parsed["fw_gate"] = firmwareGateDiagnostic(pay, i) } } } diff --git a/android/app/src/test/java/com/noop/protocol/FirmwareGateDiagnosticTest.kt b/android/app/src/test/java/com/noop/protocol/FirmwareGateDiagnosticTest.kt new file mode 100644 index 0000000000..112253049e --- /dev/null +++ b/android/app/src/test/java/com/noop/protocol/FirmwareGateDiagnosticTest.kt @@ -0,0 +1,55 @@ +package com.noop.protocol + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * Pins the 5/MG firmware-gate diagnostic. Swift twin: `FirmwareGateDiagnosticTests`. + * + * The decoder reads the version at pay[93] behind a `pay[93] == 50` guard, both anchored to a single + * 50.38.1.0 capture. The guards fail closed, so a strap that does not match reports nothing — and a + * different generation byte is indistinguishable from a MOVED offset unless the line says which. + */ +class FirmwareGateDiagnosticTest { + + private fun payload(count: Int, at93: Int): ByteArray { + val p = ByteArray(count) { (it % 256).toByte() } + if (count > 93) p[93] = at93.toByte() + return p + } + + @Test + fun `reports the byte it actually saw and the expected one`() { + val line = firmwareGateDiagnostic(payload(128, 51), 27) + assertTrue(line, line.contains("at93=51 expected=50")) + assertTrue(line, line.contains("len=128")) + } + + @Test + fun `carries the name end because that is what moves the offset`() { + // The version sits after the name+token region, so where the printable-ASCII name run ended is + // the number that lets a reader re-derive a shifted offset. + assertTrue(firmwareGateDiagnostic(payload(128, 51), 31).contains("nameEnd=31")) + } + + @Test + fun `the hex window spans the region the version should occupy`() { + val line = firmwareGateDiagnostic(payload(128, 51), 27) + assertTrue(line, line.contains("hex[88..<101]=")) + assertEquals(26, line.substringAfter("hex[88..<101]=").length) + } + + @Test + fun `a short payload cannot trap and says so`() { + // A malformed/truncated hello must not crash the decoder; the window clamps and at93 reports n/a. + val line = firmwareGateDiagnostic(byteArrayOf(1, 2, 3), 2) + assertTrue(line, line.contains("at93=n/a")) + assertTrue(line, line.contains("len=3")) + } + + @Test + fun `a payload ending exactly at the window start yields an empty window`() { + assertTrue(firmwareGateDiagnostic(ByteArray(88), 20).endsWith("hex[88..<88]=")) + } +} From eefd1bb766529c5f865fac4f7527bde8d5f2aed4 Mon Sep 17 00:00:00 2001 From: Fanboynz Date: Wed, 26 Aug 2026 13:46:06 +1200 Subject: [PATCH 3/4] protocol: the Android gate line was nested where it could never fire MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-review. I placed the fw_gate logging INSIDE the `(fw_version ?: fw_harvard)?.let { fw -> … }` block — the branch that only runs when a firmware WAS decoded. fw_gate exists only when one was NOT, so the two conditions are mutually exclusive: the line could never be emitted. The whole diagnostic was inert on Android. Nothing caught it. It compiled, and all five twinned formatter tests passed — they exercise the string, not where it is called from. The Swift twin was already correct (its check is a sibling of the `if let fw` block, not nested inside it), so the byte-parity diff passed too while one platform silently produced nothing. Moved out to the same sibling position Swift uses. Also confirmed while re-reviewing: both gates sit inside 5/MG-only paths (respCmd == 145 / cmd == 145, GET_HELLO), so a WHOOP 4.0 — which reports firmware through REPORT_VERSION_INFO as fw_harvard — never reaches them and cannot emit a spurious "no version decoded" line on every connect. compileFullDebugKotlin clean; protocol tests green; doc_comment_lint clean. --- android/app/src/main/java/com/noop/ble/WhoopBleClient.kt | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) 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 29e1350c7d..52912e2f6c 100644 --- a/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt +++ b/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt @@ -5852,14 +5852,15 @@ class WhoopBleClient( runCatching { NoopPrefs.setFirmwareFor(context, lastDeviceAddress, fw) } } + } + // #1634: the 5/MG hello decoded no firmware. The guards fail closed by design, so this is the // only place that can say WHY - a different generation byte vs a MOVED offset. Logged once per // connection (the value is stable), so a capture from an undecoded strap carries the evidence. (parsed.parsed["fw_gate"] as? String)?.let { gate -> - if (loggedFirmwareGate != gate) { - loggedFirmwareGate = gate - log(gate) - } + if (loggedFirmwareGate != gate) { + loggedFirmwareGate = gate + log(gate) } } val respCmd = parsed.parsed["resp_cmd"] as? String From 51d44472630a6deeb862a13a287fa3f93f20bc38 Mon Sep 17 00:00:00 2001 From: Fanboynz Date: Wed, 26 Aug 2026 13:56:28 +1200 Subject: [PATCH 4/4] diagnostics: bind the registry rows before counting them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit app-build failed both legs on one error: DebugDataDiagnostics.swift:131: cannot find 'invDevices' in scope I read the registry inline — `((try? invRegistry.all()) ?? []).map { … }` — and then referenced `invDevices.count` for the paired-count rule, but no such binding ever existed. Bound it before the map. Worth recording how this got through: every local check passed. The pure helpers are byte-parity diffed, the Kotlin side compiles and its tests run, and the WhoopProtocol half has 638 tests green on Linux. None of that touches this file, which needs macOS. app-build is the only thing that compiles it, and it did its job on the first run. Swept the same block for any other name used but never bound: none. doc_comment_lint clean. --- Strand/System/DebugDataDiagnostics.swift | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Strand/System/DebugDataDiagnostics.swift b/Strand/System/DebugDataDiagnostics.swift index ae92d97d16..4287e75d27 100644 --- a/Strand/System/DebugDataDiagnostics.swift +++ b/Strand/System/DebugDataDiagnostics.swift @@ -114,7 +114,11 @@ enum DebugDataDiagnostics { // the same position it holds on Android. if let invStore = await repo.storeHandle() { let invRegistry = DeviceRegistryStore(dbQueue: invStore.registryWriter) - let invRows = ((try? invRegistry.all()) ?? []).map { + // Bound before the map so the rule below has a count to test. Reading `.all()` inline left + // nothing to reference, which app-build caught and no local check could — this file needs + // macOS to compile. + let invDevices = (try? invRegistry.all()) ?? [] + let invRows = invDevices.map { // Firmware resolved by the same rule as the Devices card: this device's own persisted // value when there is one, and the LEGACY global key only when a single device is paired // (it cannot have come from anything else). Apple does not yet write the per-device key —