From 2a1f36c4d4daffa398cae95c05d46fabd98e492b Mon Sep 17 00:00:00 2001 From: vishk23 <119831996+vishk23@users.noreply.github.com> Date: Wed, 29 Jul 2026 00:40:24 -0400 Subject: [PATCH 1/2] Separate the two enumeration terminators: don't stop a key walk on validKey=0 (#761) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `FeatureFlagProbe.swift` says the record layout is "Unverified on 5/MG. The hardware result behind this layout is a WHOOP 4.0 with an R19-era key", and `isExhausted` was `!validKey || index == 0xFF` — a disjunction inherited from that same unverified layout. On a WHOOP 5 MG (WS50_r03) the two have never been separated on the wire: - 117/118 served sixteen replies, ALL validKey=1 with decodable names, indices 1-10 then 13-18. Neither terminator was ever served; the walk ended because the CLIENT stopped asking at the announced count. - 115/116 ended on one reply carrying index=255 AND validKey=0 together. Both conditions fired on the same frame. So `validKey = 0` may equally mark an EMPTY or RETIRED SLOT with the list continuing past it. If it does, every published enumeration is truncated at the first hole — the same failure #874's isSkippable exists to prevent, one condition over. The walk now stops only on index == 0xFF. A validKey=0 entry is recorded, stepped over, and the next-record verb sent again; what comes back separates the readings, and `terminatorFinding` states which was observed. Client-side bounds: 8 consecutive empty slots, a repeated index during such a run (parked cursor - evidence for the terminator reading), the announced count plus 4, and the existing 128-reply cap. Each names itself in a new `stopCode`, so a walk that ended on our arithmetic can never read as one the strap ended. Also: raw record bytes of every reply are logged beside the parsed fields (and the whole frame when a reply fails to decode); the announced count now carries BOTH the u16 LE and the single-byte reading, because every count seen so far had a zero high byte and does not distinguish them; and the walk is namespace-parameterised so 115/116 goes through the identical rules. still counted and stepped over, never reported as the strap's behaviour. Read-only throughout: no new opcode, no allowlist change, the SET verbs (119/120) are still never formed on this path. An explicit UNSUPPORTED(3) refusal now ends the walk on its own named stopCode, on the START reply (the driver reads hasStopped rather than stepping to the next verb) and on any NEXT reply. swift test: 441 passed / 0 failures (FeatureFlagProbeTests 27 -> 43). --- .../WhoopProtocol/FeatureFlagProbe.swift | 502 +++++++++++++-- .../FeatureFlagProbeTests.swift | 433 ++++++++++++- Strand/BLE/BLEManager.swift | 13 +- .../main/java/com/noop/ble/WhoopBleClient.kt | 13 +- .../com/noop/protocol/FeatureFlagProbe.kt | 576 +++++++++++++++--- .../com/noop/protocol/FeatureFlagProbeTest.kt | 473 +++++++++++++- docs/PROTOCOL.md | 19 +- 7 files changed, 1832 insertions(+), 197 deletions(-) diff --git a/Packages/WhoopProtocol/Sources/WhoopProtocol/FeatureFlagProbe.swift b/Packages/WhoopProtocol/Sources/WhoopProtocol/FeatureFlagProbe.swift index 30cad0e336..2f63770f5c 100644 --- a/Packages/WhoopProtocol/Sources/WhoopProtocol/FeatureFlagProbe.swift +++ b/Packages/WhoopProtocol/Sources/WhoopProtocol/FeatureFlagProbe.swift @@ -75,6 +75,111 @@ public enum FeatureFlagProbe { /// a mis-anchored record, not a key. public static let maxKeyLength = 32 + /// START_DEVICE_CONFIG_KEY_EXCHANGE (115 / 0x73) — the device-config twin of 117. Read-only. + public static let startDeviceConfigKeyExchangeCmd: UInt8 = 115 + /// SEND_NEXT_DEVICE_CONFIG (116 / 0x74) — the device-config twin of 118. Read-only. + public static let sendNextDeviceConfigCmd: UInt8 = 116 + + /// How many consecutive `validKey = 0` replies the walk steps over before it gives up on the + /// EMPTY-SLOT reading (see `NextResponse.isEmptySlot`). + /// + /// Calibrated on the one 117/118 walk this project has: that strap announced 16 keys and served + /// indices **1–10 then 13–18** — a two-slot hole it handled itself, without ever sending + /// `validKey = 0`. A firmware that instead exposes its holes would show them as runs of that size, + /// so eight is four times the largest hole ever observed and still a hard bound: the empty-slot + /// experiment can cost at most eight extra round-trips beyond where the walk used to stop. + public static let maxConsecutiveEmptySlots = 8 + + /// Extra `SEND_NEXT` replies the walk takes AFTER the strap's announced count is satisfied, so the + /// strap — not our arithmetic — gets to end its own list. + /// + /// This exists because of what the announced count actually did on hardware: the 117/118 walk stopped + /// at exactly `steps == count`, having never seen `index = 0xFF` **or** `validKey = 0`. Every entry + /// came back valid and named. So the list was not observed to end; the client stopped asking. The + /// same strap served indices up to **18** while announcing **16**, which is the calibration: an + /// allowance of four is twice that observed excess, and still bounded by `maxFlags`. + public static let countOvershootAllowance = 4 + + /// Ceiling on record bytes rendered as hex in one trace line. Records are short (`revision`, `index`, + /// `validKey`, a ≤32-byte name, padding), so anything past this is already evidence that the record + /// is not the documented layout — and the byte COUNT is always reported even when the tail is elided, + /// so the finding survives the elision. + public static let maxRawHexBytes = 64 + + // MARK: - Namespaces + + /// One enumerate pair — the opcodes the walk drives and the words the report prints for them. + /// + /// The protocol's own `CommandNumber` table names two symmetric enumerate pairs (117/118 for feature + /// flags, 115/116 for device config). Both are walked by the SAME code here, so a fix to the walk's + /// terminator handling cannot apply to one namespace and miss the other. + /// + /// Reusing this decoder for 115/116 ASSUMES the two share a record layout — an inference from the + /// naming symmetry in that table, not an observation. It fails closed: a mismatch surfaces as + /// `.truncated` or as a count `countIsPlausible` rejects. + public struct Namespace: Equatable, Sendable { + public let startCmd: UInt8 + public let nextCmd: UInt8 + public let startLabel: String + public let nextLabel: String + /// Report title, e.g. `FEATURE-FLAG ENUMERATION PROBE`. + public let title: String + /// What the namespace is called in prose, e.g. `feature-flag`. + public let noun: String + /// What ONE entry is called in prose, e.g. `flag`. + public let entryNoun: String + /// Heading for the collected list, e.g. `Flags`. + public let listHeading: String + public init(startCmd: UInt8, nextCmd: UInt8, startLabel: String, nextLabel: String, + title: String, noun: String, entryNoun: String, listHeading: String) { + self.startCmd = startCmd + self.nextCmd = nextCmd + self.startLabel = startLabel + self.nextLabel = nextLabel + self.title = title + self.noun = noun + self.entryNoun = entryNoun + self.listHeading = listHeading + } + } + + /// 117/118 — the feature-flag key list. + public static let featureFlagNamespace = Namespace( + startCmd: startKeyExchangeCmd, nextCmd: sendNextFlagCmd, + startLabel: "START_FF_KEY_EXCHANGE", nextLabel: "SEND_NEXT_FF", + title: "FEATURE-FLAG ENUMERATION PROBE", noun: "feature-flag", entryNoun: "flag", + listHeading: "Flags") + + /// 115/116 — the device-config key list. + public static let deviceConfigNamespace = Namespace( + startCmd: startDeviceConfigKeyExchangeCmd, nextCmd: sendNextDeviceConfigCmd, + startLabel: "START_DEVICE_CONFIG_KEY_EXCHANGE", nextLabel: "SEND_NEXT_DEVICE_CONFIG", + title: "DEVICE-CONFIG ENUMERATION PROBE", noun: "device-config", entryNoun: "config key", + listHeading: "Keys") + + /// Why a walk stopped. A STRING reason reads well in a log and a CODE can be asserted on, and both + /// are always set together: a truncated walk that reports no reason at all is precisely how a partial + /// key list gets mistaken for a complete one. + public enum StopCode: String, Equatable, Sendable { + /// The strap served `index = 0xFF`. The one terminator this project has ever seen unambiguously. + case endMarker + /// A `validKey = 0` reply repeated the previous reply's index: the cursor is parked, so on this + /// firmware `validKey = 0` behaves as a terminator rather than as an empty slot. + case emptySlotCursorParked + /// `maxConsecutiveEmptySlots` consecutive `validKey = 0` replies, with the index still advancing. + case emptySlotRunCap + /// `maxFlags` replies. A client-side bound: says nothing about where the strap's list ends. + case stepCap + /// The announced count plus `countOvershootAllowance` was spent. Also a client-side bound. + case announcedCountOvershoot + /// The strap answered nothing inside the probe's window. + case timeout + /// The strap refused the verb (result `UNSUPPORTED`). + case unsupported + /// A reply did not decode. Our limitation, and labelled as ours. + case parseFailure + } + // MARK: - Decoded responses /// Why a frame was not decoded. Distinct cases so the probe can say *what* went wrong in the log @@ -98,15 +203,36 @@ public enum FeatureFlagProbe { public let resultCode: Int? /// Record `revision` — the layout version the firmware answers with. public let revision: Int - /// `numberOfFeatureFlags` as reported by the strap. NOT trusted as a loop bound on its own. + /// `numberOfFeatureFlags` read as u16 LE (`record[1] | record[2] << 8`). NOT trusted as a loop + /// bound on its own, and NOT the only defensible reading of those bytes — see `singleByteCount`. public let count: Int - public init(resultCode: Int?, revision: Int, count: Int) { + /// The raw record bytes this response was decoded from, kept so the report can print them. + /// Parsed fields are a claim about the layout; these bytes are the evidence for it. + public let record: [UInt8] + public init(resultCode: Int?, revision: Int, count: Int, record: [UInt8] = []) { self.resultCode = resultCode self.revision = revision self.count = count + self.record = record } /// True when the count is inside the range a real key list could plausibly occupy. public var countIsPlausible: Bool { count > 0 && count <= FeatureFlagProbe.maxFlags } + + /// The same field read as a SINGLE byte — i.e. on the reading where `record[2]` is padding + /// rather than the count's high byte. + /// + /// The layout this decoder implements calls the field u16 LE. Nothing observed here settles that: + /// every count seen so far has had `record[2] == 0x00`, where a u16 read and a single-byte read + /// return the SAME number, so no capture yet distinguishes them. Both are therefore carried, and + /// the report says plainly when they agree (no evidence either way) and when they differ (which + /// is itself the finding). Guessing one and discarding the other is how a walk gets bounded by a + /// number nobody checked. + public var singleByteCount: Int? { record.count >= 2 ? Int(record[1]) : nil } + /// The byte a u16 reading treats as the count's high half. `0x00` means the two readings agree. + public var countHighByte: Int? { record.count >= 3 ? Int(record[2]) : nil } + /// True when the u16 and single-byte readings return the same number, i.e. this reply is silent + /// on the field's width. + public var countReadingsAgree: Bool { countHighByte == 0 } } /// Decoded `SEND_NEXT_FF` reply. @@ -119,21 +245,45 @@ public enum FeatureFlagProbe { public let validKey: Bool /// The key name, when one decodes as printable ASCII; nil when the entry carries none. public let key: String? - public init(resultCode: Int?, revision: Int, index: Int, validKey: Bool, key: String?) { + /// The raw record bytes this response was decoded from, kept so the report can print them. + public let record: [UInt8] + public init(resultCode: Int?, revision: Int, index: Int, validKey: Bool, key: String?, + record: [UInt8] = []) { self.resultCode = resultCode self.revision = revision self.index = index self.validKey = validKey self.key = key + self.record = record } - /// The STRAP said stop: it returned the 0xFF end marker, or flagged the entry as not a real key. - /// Both are the firmware's own signal, so both are trusted. + + /// The strap's UNAMBIGUOUS end marker: `index = 0xFF`. + /// + /// This used to also mean `validKey = 0`, on the reading that the firmware sets that flag to + /// signal the end of its list. That reading is not established. The file's own header note says + /// the record layout is **unverified on 5/MG** — it was derived from a WHOOP 4.0 with an R19-era + /// key list — and the terminator semantics came with the layout. + /// + /// What a 5/MG (WS50_r03) actually served, on the only walks this project has: + /// + /// - 117/118: sixteen replies, every one `validKey = 1` with a decodable name, indices 1–10 then + /// 13–18. The strap handled its own two-slot hole internally. **Neither** `validKey = 0` **nor** + /// `index = 0xFF` was ever served — the walk ended because the client stopped asking at the + /// announced count. + /// - 115/116: the final reply carried `index = 255` **and** `validKey = 0` **together**. Both + /// conditions fired on the same frame, so that run cannot say which one the firmware meant. /// - /// Deliberately does NOT include `key == nil`. That is OUR parser declining a name — a byte outside - /// printable ASCII, or one longer than `maxKeyLength` — and it says nothing about whether the strap - /// has more entries. Treating it as a terminator meant one undecodable name threw away every entry - /// after it: a list of forty with a bad byte at seven yielded six keys. See `isSkippable`. - public var isExhausted: Bool { !validKey || index == 0xFF } + /// So on this hardware the disjunction has never been separated. If `validKey = 0` in fact marks + /// an EMPTY or RETIRED SLOT with the list continuing past it, then stopping there truncates — + /// the identical failure `isSkippable` exists to prevent, one condition over, and one that would + /// silently shorten every published key list including this project's own. `isEmptySlot` is that + /// case, and the walk now steps over it and records what comes next. + public var isExhausted: Bool { index == 0xFF } + + /// `validKey = 0` WITHOUT the 0xFF end marker — the ambiguous case, and the one this probe exists + /// to resolve. Recorded, stepped over, and bounded by `maxConsecutiveEmptySlots`; what the strap + /// serves next is the evidence that separates "end of list" from "empty slot". + public var isEmptySlot: Bool { !validKey && index != 0xFF } /// The firmware calls this a real key but the name did not decode. Record it and KEEP WALKING. /// @@ -148,19 +298,30 @@ public enum FeatureFlagProbe { /// Decode a `START_FF_KEY_EXCHANGE` COMMAND_RESPONSE. CRC-gated: a frame whose checksums fail is /// rejected before any field is read. - public static func parseStart(frame: [UInt8], family: DeviceFamily) -> Result { - switch record(frame: frame, family: family, expecting: startKeyExchangeCmd) { + /// + /// `expecting` defaults to 117 and exists so the DEVICE-CONFIG twin + /// `START_DEVICE_CONFIG_KEY_EXCHANGE` (115) is decoded by this same code — the reuse contract + /// documented on `Namespace`. + public static func parseStart(frame: [UInt8], family: DeviceFamily, + expecting: UInt8 = startKeyExchangeCmd) + -> Result { + switch record(frame: frame, family: family, expecting: expecting) { case .failure(let f): return .failure(f) case .success(let r): guard r.record.count >= 3 else { return .failure(.truncated) } let count = Int(r.record[1]) | (Int(r.record[2]) << 8) - return .success(StartResponse(resultCode: r.resultCode, revision: Int(r.record[0]), count: count)) + return .success(StartResponse(resultCode: r.resultCode, revision: Int(r.record[0]), + count: count, record: r.record)) } } /// Decode a `SEND_NEXT_FF` COMMAND_RESPONSE. CRC-gated like `parseStart`. - public static func parseNext(frame: [UInt8], family: DeviceFamily) -> Result { - switch record(frame: frame, family: family, expecting: sendNextFlagCmd) { + /// `expecting` defaults to 118 and carries the same reuse contract: 116 + /// (`SEND_NEXT_DEVICE_CONFIG`) walks the device-config namespace through this decoder. + public static func parseNext(frame: [UInt8], family: DeviceFamily, + expecting: UInt8 = sendNextFlagCmd) + -> Result { + switch record(frame: frame, family: family, expecting: expecting) { case .failure(let f): return .failure(f) case .success(let r): // revision + index are the minimum: the 0xFF end marker arrives with nothing after it. @@ -170,7 +331,7 @@ public enum FeatureFlagProbe { let validKey = r.record.count >= 3 ? r.record[2] != 0 : false let key = r.record.count >= 4 ? asciiKey(Array(r.record[3...])) : nil return .success(NextResponse(resultCode: r.resultCode, revision: revision, index: index, - validKey: validKey, key: key)) + validKey: validKey, key: key, record: r.record)) } } @@ -223,6 +384,21 @@ public enum FeatureFlagProbe { return out.isEmpty ? nil : String(decoding: out, as: UTF8.self) } + /// Lower-case space-separated hex, for putting the RAW bytes of a reply in the log next to the + /// fields decoded from them. + /// + /// Parsed output alone has repeatedly turned out to be insufficient evidence here: a claim about the + /// record layout cannot be re-checked, or contradicted, from the fields that layout produced. Over + /// `maxRawHexBytes` the tail is elided and the true length is printed, so the elision itself is + /// visible and a suspiciously long record still reads as one. + public static func hex(_ bytes: [UInt8]) -> String { + if bytes.isEmpty { return "(empty)" } + let shown = bytes.prefix(maxRawHexBytes) + var out = shown.map { String(format: "%02x", $0) }.joined(separator: " ") + if bytes.count > maxRawHexBytes { out += " … (\(bytes.count) bytes total)" } + return out + } + /// 5/MG result-code label (0 FAILURE / 1 SUCCESS / 2 PENDING / 3 UNSUPPORTED), matching /// `BodyLocationProbe`. Unknown codes keep their number rather than being coerced. static func resultLabel(_ code: Int) -> String { @@ -247,86 +423,195 @@ public struct FeatureFlagProbeReport: Equatable, Sendable { /// Which family the probe ran against (labels only). public let family: DeviceFamily + /// Which enumerate pair this walk drives — 117/118 or 115/116. The walk logic is identical; only + /// the opcodes and the words differ, so neither namespace can drift from the other's terminator rules. + public let namespace: FeatureFlagProbe.Namespace /// Key names collected, in the order the strap reported them. public private(set) var keys: [String] = [] /// Trace lines: one per reply plus any failure notes. public private(set) var trace: [String] = [] /// SEND_NEXT replies seen. Bounds the walk (see `noteNext`). public private(set) var steps = 0 - /// The count the strap reported for `START_FF_KEY_EXCHANGE`, when it answered. + /// The count the strap reported for the START verb, when it answered. public private(set) var reportedCount: Int? + /// The same count read as a single byte (`record[1]`), when the record carried one. + public private(set) var reportedCountSingleByte: Int? + /// `record[2]` — the byte a u16 reading treats as the count's high half. + public private(set) var reportedCountHighByte: Int? /// Result code of the START reply on 5/MG (nil on 4.0, or before it lands). public private(set) var startResult: Int? /// Entries the strap flagged as real keys whose NAME did not decode, and which the walk stepped over /// rather than stopping at. Surfaced in the report so a dump with holes never reads as a complete list. public private(set) var skipped = 0 + /// Replies carrying `validKey = 0` WITHOUT the 0xFF end marker, which the walk stepped over. + public private(set) var emptySlots = 0 + /// True once any `validKey = 0` reply arrived without the 0xFF marker. + public private(set) var sawEmptySlot = false + /// True once `index = 0xFF` arrived. + public private(set) var sawEndMarker = false + /// True when the 0xFF reply ALSO carried `validKey = 0` — i.e. both terminator conditions fired on + /// one frame, and that run cannot say which one the firmware meant. This is what a 5/MG served on + /// 115/116, and the reason the two have never been separated. + public private(set) var endMarkerAlsoCarriedInvalidFlag = false + /// Replies the strap served AFTER the first `validKey = 0`. Any value above zero is the decisive + /// observation: the list did not end there. + public private(set) var repliesAfterFirstEmptySlot = 0 + /// Key names collected after the first `validKey = 0` — keys a walk that stopped there would have lost. + public private(set) var keysAfterFirstEmptySlot = 0 + /// Replies after the first `validKey = 0` that the FIRMWARE flagged as real entries, whether or not + /// this parser could decode their names. + /// + /// This — not the decoded-key count — is what makes the empty-slot reading decisive. #874's rule + /// applies to the conclusion as much as to the walk: if the strap named an entry past the hole and our + /// ASCII filter declined it, the list still continued, and reporting "inconclusive" there would be our + /// parser's limitation deciding a question about the firmware. + public private(set) var validEntriesAfterFirstEmptySlot = 0 + /// Replies taken past the strap's announced count, under `countOvershootAllowance`. + public private(set) var repliesPastAnnouncedCount = 0 /// Set once the walk stopped for a reason we can name. public private(set) var stopReason: String? + /// The same reason as a code, so a caller (or a test) can branch on it rather than on prose. + public private(set) var stopCode: FeatureFlagProbe.StopCode? + /// True once the walk has a reason to stop. The BLE driver consults this after EVERY reply, so a + /// refusal recorded on the START reply ends the run instead of being stepped over into the next verb. + public var hasStopped: Bool { stopCode != nil } + /// The index the previous reply carried, for spotting a cursor that stopped advancing. + private var lastIndex: Int? + /// Length of the current unbroken run of `validKey = 0` replies, reset by any valid entry. + private var consecutiveEmptySlots = 0 - public init(family: DeviceFamily) { self.family = family } + public init(family: DeviceFamily, + namespace: FeatureFlagProbe.Namespace = FeatureFlagProbe.featureFlagNamespace) { + self.family = family + self.namespace = namespace + } /// Record the START reply. public mutating func noteStart(_ r: FeatureFlagProbe.StartResponse) { reportedCount = r.count + reportedCountSingleByte = r.singleByteCount + reportedCountHighByte = r.countHighByte startResult = r.resultCode - var line = "START_FF_KEY_EXCHANGE(117) → revision=\(r.revision) count=\(r.count)" + var line = "\(namespace.startLabel)(\(namespace.startCmd)) → revision=\(r.revision) count=\(r.count)" if let c = r.resultCode { line += " result=\(FeatureFlagProbe.resultLabel(c))(\(c))" } + line += " raw=\(FeatureFlagProbe.hex(r.record))" if !r.countIsPlausible { line += " ⚠︎ count outside 1…\(FeatureFlagProbe.maxFlags) — treated as unknown, the walk still stops on the strap's own end marker" } trace.append(line) + if let c = r.resultCode, c == 3 { + stopReason = "strap refused \(namespace.startLabel)(\(namespace.startCmd)) with UNSUPPORTED(3)" + stopCode = .unsupported + } } /// Record one SEND_NEXT reply. Returns true when the walk should continue. + /// + /// The decision order IS the experiment. `index = 0xFF` ends the walk, because that marker is the one + /// thing a strap has served here unambiguously. `validKey = 0` on its own does NOT end it: the walk + /// steps over the entry, sends the next-record verb again, and records what comes back — which is the + /// only way to tell "end of list" from "empty slot" apart. Everything past that point is a + /// CLIENT-side bound (`maxConsecutiveEmptySlots`, `maxFlags`, the announced count plus its + /// overshoot), and each one names itself in `stopCode` so a walk that ended on our arithmetic can + /// never be read as a walk the strap ended. @discardableResult public mutating func noteNext(_ r: FeatureFlagProbe.NextResponse) -> Bool { steps += 1 - var line = "SEND_NEXT_FF(118) → index=\(r.index) validKey=\(r.validKey)" + if sawEmptySlot { repliesAfterFirstEmptySlot += 1 } + var line = "\(namespace.nextLabel)(\(namespace.nextCmd)) → index=\(r.index) validKey=\(r.validKey)" if let k = r.key { line += " key=\"\(k)\"" } if let c = r.resultCode { line += " result=\(FeatureFlagProbe.resultLabel(c))(\(c))" } + line += " raw=\(FeatureFlagProbe.hex(r.record))" trace.append(line) + + // 0. An explicit refusal. UNSUPPORTED(3) says the firmware does not serve this verb, so nothing + // in the record is meaningful and there is nothing to walk. Checked FIRST, before any field is + // acted on, and named as the strap's answer rather than as one of our bounds. + if let c = r.resultCode, c == 3 { + stopReason = "strap refused \(namespace.nextLabel)(\(namespace.nextCmd)) with UNSUPPORTED(3)" + stopCode = .unsupported + return false + } + + // 1. The strap's own unambiguous end marker. Nothing overrides this. if r.isExhausted { - if r.index == 0xFF { - stopReason = "cursor exhausted (index 0xFF)" - } else { - // `validKey = 0` is documented as an end marker alongside 0xFF, and it is the firmware's - // own flag, so it is trusted here. But nothing observed so far rules out the other - // reading — that it marks an EMPTY SLOT and the list continues past it. If so this stops - // on the first hole, which is the same truncation `isSkippable` exists to prevent, one - // condition over. It cannot be settled without a strap, so instead of guessing we make - // the discrepancy loud: stopping here well short of the announced count is exactly the - // evidence that would settle it. - stopReason = "firmware reported validKey=false" - if let count = reportedCount, count > steps { - stopReason = "firmware reported validKey=false at index \(r.index) after \(steps) of " - + "\(count) announced entries — if validKey=0 marks an empty slot rather than the " - + "end of the list, the remainder was NOT walked (see #872 review)" - } + sawEndMarker = true + if !r.validKey { + endMarkerAlsoCarriedInvalidFlag = true + trace[trace.count - 1] += " (index=0xFF AND validKey=0 on the same reply — both terminator conditions at once)" } + stopReason = "cursor exhausted (index 0xFF)" + stopCode = .endMarker return false } - if r.isSkippable { - // Our decode declined the name; the strap still says the entry is real and may have more after - // it. Count it so a partial dump describes itself instead of looking complete. - skipped += 1 - trace[trace.count - 1] += " (name did not decode — skipped, walk continues)" + + // 2. `validKey = 0` with no end marker: the ambiguous case. Step over it and keep asking. + if r.isEmptySlot { + let parked = (lastIndex == r.index) + lastIndex = r.index + emptySlots += 1 + consecutiveEmptySlots += 1 + let runLength = consecutiveEmptySlots + sawEmptySlot = true + trace[trace.count - 1] += " (validKey=0 without the 0xFF marker — treated as an EMPTY SLOT, walk continues)" + if parked { + // The cursor did not move. A firmware that means "end of list" parks there and repeats + // itself, so this is the observation that settles the ambiguity the other way — and it + // costs two round-trips rather than a full cap's worth. + stopReason = "validKey=0 repeated at index \(r.index) without advancing — the cursor is " + + "parked, so on this firmware validKey=0 IS a terminator" + stopCode = .emptySlotCursorParked + return false + } + if runLength >= FeatureFlagProbe.maxConsecutiveEmptySlots { + stopReason = "\(runLength) consecutive validKey=0 replies (cap " + + "\(FeatureFlagProbe.maxConsecutiveEmptySlots)) — a CLIENT-side bound, not the strap's" + stopCode = .emptySlotRunCap + return false + } + } else { + lastIndex = r.index + consecutiveEmptySlots = 0 + if sawEmptySlot { validEntriesAfterFirstEmptySlot += 1 } + if r.isSkippable { + // Our decode declined the name; the strap still says the entry is real and may have more + // after it. Count it so a partial dump describes itself instead of looking complete. + skipped += 1 + trace[trace.count - 1] += " (name did not decode — skipped, walk continues)" + } + if let k = r.key, !keys.contains(k) { + keys.append(k) + if sawEmptySlot { keysAfterFirstEmptySlot += 1 } + } } - if let k = r.key, !keys.contains(k) { keys.append(k) } - // Bound the walk on REPLIES, not on distinct keys: a firmware whose cursor never advances would - // repeat one name forever, and a key-count bound would never stop writing 118 to the strap. + + // 3. Bound the walk on REPLIES, not on distinct keys: a firmware whose cursor never advances would + // repeat one name forever, and a key-count bound would never stop writing the verb to the strap. if steps >= FeatureFlagProbe.maxFlags { stopReason = "safety cap of \(FeatureFlagProbe.maxFlags) replies reached" + stopCode = .stepCap return false } - if let count = reportedCount, count > 0, count <= FeatureFlagProbe.maxFlags, steps >= count { - stopReason = "walked the \(count) entries the strap announced" - return false + // 4. The announced count, PLUS an overshoot, so the strap gets to end its own list. On the one + // 117/118 walk this project has, `steps >= count` was the only thing that stopped it — no marker, + // no validKey=0 — which means that list was never observed to end at all. + if let count = reportedCount, count > 0, count <= FeatureFlagProbe.maxFlags { + if steps > count { repliesPastAnnouncedCount = steps - count } + if steps >= count + FeatureFlagProbe.countOvershootAllowance { + stopReason = "walked the \(count) entries the strap announced plus " + + "\(FeatureFlagProbe.countOvershootAllowance) more — a CLIENT-side bound; the strap " + + "never sent an end marker, so the list is not known to end here" + stopCode = .announcedCountOvershoot + return false + } } return true } - /// Record a reply that could not be decoded. - public mutating func noteFailure(_ f: FeatureFlagProbe.ParseFailure, command: Int) { + /// Record a reply that could not be decoded. `frame`, when supplied, is logged in full: a frame that + /// failed its CRC is still the only evidence of what the strap actually put on the wire. + public mutating func noteFailure(_ f: FeatureFlagProbe.ParseFailure, command: Int, + frame: [UInt8] = []) { let why: String switch f { case .crc: why = "CRC failed — frame rejected (never decoded)" @@ -334,27 +619,105 @@ public struct FeatureFlagProbeReport: Equatable, Sendable { case .wrongCommand: why = "COMMAND_RESPONSE for a different command" case .truncated: why = "record too short for the documented layout" } - trace.append("cmd \(command) reply not decoded: \(why)") - stopReason = stopReason ?? why + var line = "cmd \(command) reply not decoded: \(why)" + if !frame.isEmpty { line += " raw frame=\(FeatureFlagProbe.hex(frame))" } + trace.append(line) + if stopReason == nil { + stopReason = why + stopCode = .parseFailure + } } /// Record the strap answering nothing at all within the probe's window. public mutating func noteTimeout(command: Int, seconds: Int) { trace.append("no COMMAND_RESPONSE for opcode \(command) within \(seconds)s") - stopReason = stopReason ?? "strap served no reply to opcode \(command) within \(seconds)s" + if stopReason == nil { + stopReason = "strap served no reply to opcode \(command) within \(seconds)s" + stopCode = .timeout + } + } + + // MARK: - Findings + + /// What this run established about the two terminator conditions — the question the probe exists to + /// answer, stated in the report rather than left for a reader to reconstruct from the trace. + public var terminatorFinding: String { + if sawEmptySlot && (validEntriesAfterFirstEmptySlot > 0 || sawEndMarker) { + var s = "DECISIVE — validKey=0 is an EMPTY/RETIRED SLOT on this firmware, not the end of the " + s += "list: the strap served \(repliesAfterFirstEmptySlot) further repl(ies) after the first " + s += "validKey=0" + if validEntriesAfterFirstEmptySlot > 0 { + s += ", \(validEntriesAfterFirstEmptySlot) of them flagged validKey=1" + } + if keysAfterFirstEmptySlot > 0 { s += ", naming \(keysAfterFirstEmptySlot) more key(s)" } + if sawEndMarker { s += ", and ended on the index=0xFF marker" } + s += ". A walk that stopped on validKey=0 would have been truncated here." + return s + } + if stopCode == .emptySlotCursorParked { + return "DECISIVE — validKey=0 is a TERMINATOR on this firmware: the next request returned the " + + "same index with validKey=0 again, so the cursor is parked and there is nothing past it." + } + if sawEmptySlot { + return "INCONCLUSIVE — validKey=0 was served and the walk continued past it, but a client-side " + + "bound ended the run before the strap did. Nothing here separates 'end of list' from " + + "'empty slot'." + } + if sawEndMarker && endMarkerAlsoCarriedInvalidFlag { + return "AMBIGUOUS — the walk ended on a reply carrying index=0xFF AND validKey=0 together, so " + + "both terminator conditions fired at once and this run cannot say which one the firmware " + + "meant." + } + if sawEndMarker { + return "index=0xFF ended the walk with validKey still true on the same reply, so the 0xFF " + + "marker alone terminates. validKey=0 was never served, so it is untested here." + } + return "NO TERMINATOR OBSERVED — neither validKey=0 nor index=0xFF was served in \(steps) " + + "repl(ies); the walk ended on a CLIENT-side bound, so the strap's list is not known to end " + + "where this report stops." + } + + /// What the announced count actually says, including the reading it does not settle. + public var countFinding: String? { + guard let count = reportedCount else { return nil } + var s = "u16 LE read = \(count)" + if let single = reportedCountSingleByte, let high = reportedCountHighByte { + s += "; single-byte read = \(single) (high byte " + + String(format: "0x%02x", high) + ")" + s += high == 0 + ? " — the two readings AGREE, so this reply does not establish the field's width" + : " — the two readings DISAGREE; the walk uses the u16 value and plausibility-checks it" + } + s += ". Keys yielded: \(keys.count)" + if skipped > 0 { s += " (+\(skipped) undecodable)" } + if emptySlots > 0 { s += " (+\(emptySlots) empty slot(s))" } + let accounted = keys.count + skipped + emptySlots + if accounted != count { + s += " — MISMATCH against the announced \(count)" + } + if repliesPastAnnouncedCount > 0 { + s += ". The strap kept answering \(repliesPastAnnouncedCount) repl(ies) past its own announced " + + "count." + } + return s } /// The full copyable report. public func render() -> String { let fam = family == .whoop5 ? "WHOOP 5/MG" : "WHOOP 4.0" - var sb = "#761 FEATURE-FLAG ENUMERATION PROBE — \(fam)\n" - sb += "Read-only: START_FF_KEY_EXCHANGE(117) + SEND_NEXT_FF(118). No value is written; " + var sb = "#761 \(namespace.title) — \(fam)\n" + sb += "Read-only: \(namespace.startLabel)(\(namespace.startCmd)) + " + sb += "\(namespace.nextLabel)(\(namespace.nextCmd)). No value is written; " sb += "SET_FF_VALUE(120) and SET_DEVICE_CONFIG_VALUE(119) are never sent from this path.\n" sb += "\nVerdict: \(verdict)\n" if let stopReason { sb += "Stopped: \(stopReason)\n" } - sb += "\nFlags reported by the strap (\(keys.count)" + if let stopCode { sb += "Stop code: \(stopCode.rawValue)\n" } + sb += "Terminator: \(terminatorFinding)\n" + if let countFinding { sb += "Announced count: \(countFinding)\n" } + sb += "\n\(namespace.listHeading) reported by the strap (\(keys.count)" if let reportedCount { sb += " of \(reportedCount) announced" } if skipped > 0 { sb += ", \(skipped) name(s) did not decode and were skipped" } + if emptySlots > 0 { sb += ", \(emptySlots) validKey=0 slot(s) stepped over" } sb += "):\n" if keys.isEmpty { sb += " (none)\n" @@ -371,16 +734,20 @@ public struct FeatureFlagProbeReport: Equatable, Sendable { /// verdict is the line that gets pasted into an issue, and restating a number the probe itself /// distrusts as bare fact is the same over-claim in a smaller place. A plausible count renders /// exactly as before, so the common report is unchanged. + /// + /// The noun comes from `namespace`, not a literal: #913 wrote this when the probe walked 117/118 only, + /// so "flag(s)" was the only possibility. This PR walks 115/116 through the same code, where the + /// entries are config keys — a hardcoded "flag(s)" would mislabel every device-config report. private var announcedFlags: String { let n = reportedCount ?? 0 let plausible = n > 0 && n <= FeatureFlagProbe.maxFlags - return plausible ? "\(n) flag(s)" : "an implausible \(n) flag(s)" + return plausible ? "\(n) \(namespace.entryNoun)(s)" : "an implausible \(n) \(namespace.entryNoun)(s)" } /// One-line summary of what the probe established. public var verdict: String { if let startResult, startResult == 3 { - return "opcode 117 REJECTED by firmware (UNSUPPORTED) — this strap does not serve the enumerate verb" + return "opcode \(namespace.startCmd) REJECTED by firmware (UNSUPPORTED) — this strap does not serve the enumerate verb" } if keys.isEmpty && reportedCount == nil { return "no usable reply — the enumerate path is unconfirmed on this firmware" @@ -390,7 +757,7 @@ public struct FeatureFlagProbeReport: Equatable, Sendable { // would carry into #103. Same class as `isSkippable`: never report our limitation as the strap's // behaviour. if keys.isEmpty && skipped > 0 { - return "strap named \(skipped) flag(s), none of which decoded as printable ASCII within " + return "strap named \(skipped) \(namespace.entryNoun)(s), none of which decoded as printable ASCII within " + "\(FeatureFlagProbe.maxKeyLength) chars — this is our parser rejecting them, NOT the " + "strap serving blanks; see the trace for the raw replies" } @@ -401,15 +768,24 @@ public struct FeatureFlagProbeReport: Equatable, Sendable { // timer firing on the first 118. What the strap would have named is unknown, and the report has // to say unknown. `stopReason`, rendered directly under this line, names which of the two it was. if keys.isEmpty && steps == 0 { - return "strap announced \(announcedFlags); no SEND_NEXT_FF(118) reply was decoded — " - + "the key list was never read (inconclusive)" + // Namespace-aware for the same reason as `announcedFlags`: #913 could name SEND_NEXT_FF(118) + // literally because 117/118 was the only walk; a 115/116 run must say SEND_NEXT_DEVICE_CONFIG(116). + return "strap announced \(announcedFlags); no \(namespace.nextLabel)(\(namespace.nextCmd)) " + + "reply was decoded — the key list was never read (inconclusive)" } if keys.isEmpty { + // Both halves survive the merge with #913: its doubt about an implausible announced count + // (`announcedFlags`), and this PR's namespace-aware noun. Taking either side alone would drop + // the other — #913's version hardcodes "flag(s)" and would mislabel a device-config walk. return "strap announced \(announcedFlags) but named none" } - if skipped > 0 { - return "enumerated \(keys.count) feature-flag key name(s); \(skipped) further name(s) did not decode" + var s = "enumerated \(keys.count) \(namespace.noun) key name(s)" + if skipped > 0 { s += "; \(skipped) further name(s) did not decode" } + // A walk that ended on OUR bound is not a complete list, and the one-line summary is the line that + // gets pasted into an issue. Say it here, not only four lines further down. + if stopCode == .stepCap || stopCode == .announcedCountOvershoot || stopCode == .emptySlotRunCap { + s += " — INCOMPLETE: the walk ended on a client-side bound, not on the strap's own end marker" } - return "enumerated \(keys.count) feature-flag key name(s)" + return s } } diff --git a/Packages/WhoopProtocol/Tests/WhoopProtocolTests/FeatureFlagProbeTests.swift b/Packages/WhoopProtocol/Tests/WhoopProtocolTests/FeatureFlagProbeTests.swift index c37a34098f..4d57278253 100644 --- a/Packages/WhoopProtocol/Tests/WhoopProtocolTests/FeatureFlagProbeTests.swift +++ b/Packages/WhoopProtocol/Tests/WhoopProtocolTests/FeatureFlagProbeTests.swift @@ -145,21 +145,133 @@ final class FeatureFlagProbeTests: XCTestCase { XCTAssertEqual(report.stopReason, "cursor exhausted (index 0xFF)") } - func testValidKeyFalseStopsTheWalkEvenWithBytesAfterIt() { - // validKey=0 with a plausible-looking name after it: the firmware's own flag wins. + /// `validKey = 0` alone is NOT classified as the strap's end marker any more — it is an empty slot, + /// and the walk steps over it. The entry itself is still never collected as a key. + func testValidKeyFalseIsAnEmptySlotNotTheEndMarker() { + // validKey=0 with a plausible-looking name after it: still not a key, still not the end. let record: [UInt8] = [0x01, 0x04, 0x00] + keyBytes("stale_buffer_leftover") let frame = whoop4Response(cmd: 118, payload: payload(result: 1, record: record)) guard case .success(let r) = FeatureFlagProbe.parseNext(frame: frame, family: .whoop4) else { return XCTFail("expected a decoded NEXT response") } XCTAssertFalse(r.validKey) - XCTAssertTrue(r.isExhausted) + XCTAssertFalse(r.isExhausted, "only index=0xFF is the strap's unambiguous end marker") + XCTAssertTrue(r.isEmptySlot) var report = FeatureFlagProbeReport(family: .whoop4) - XCTAssertFalse(report.noteNext(r)) - XCTAssertEqual(report.stopReason, "firmware reported validKey=false") + XCTAssertTrue(report.noteNext(r), "validKey=0 without 0xFF must not end the walk") + XCTAssertNil(report.stopReason) + XCTAssertEqual(report.emptySlots, 1) XCTAssertTrue(report.keys.isEmpty, "an invalid entry must never be collected as a flag") } + /// The whole experiment, in one walk: the strap serves `validKey = 0` mid-list, the walk keeps asking, + /// and the strap names MORE keys before ending on 0xFF. That sequence is decisive — under the old rule + /// this list would have been reported as one key long. + func testWalkContinuesPastValidKeyZeroAndCollectsTheKeysAfterIt() { + var report = FeatureFlagProbeReport(family: .whoop5) + report.noteStart(FeatureFlagProbe.StartResponse(resultCode: 1, revision: 1, count: 5, + record: [0x01, 0x05, 0x00])) + func next(_ index: Int, valid: Bool, _ key: String?) -> FeatureFlagProbe.NextResponse { + FeatureFlagProbe.NextResponse(resultCode: 1, revision: 1, index: index, validKey: valid, + key: key, record: [0x01, UInt8(index), valid ? 1 : 0]) + } + XCTAssertTrue(report.noteNext(next(1, valid: true, "enable_r22_packets"))) + XCTAssertTrue(report.noteNext(next(2, valid: false, nil)), "an empty slot must not end the walk") + XCTAssertTrue(report.noteNext(next(3, valid: false, nil)), "nor must a second one") + XCTAssertTrue(report.noteNext(next(4, valid: true, "enable_sig12"))) + XCTAssertFalse(report.noteNext(next(0xFF, valid: false, nil))) + + XCTAssertEqual(report.keys, ["enable_r22_packets", "enable_sig12"]) + XCTAssertEqual(report.emptySlots, 2) + XCTAssertEqual(report.keysAfterFirstEmptySlot, 1) + XCTAssertEqual(report.stopCode, .endMarker) + let finding = report.terminatorFinding + XCTAssertTrue(finding.hasPrefix("DECISIVE — validKey=0 is an EMPTY/RETIRED SLOT"), finding) + XCTAssertTrue(finding.contains("naming 1 more key(s)"), finding) + XCTAssertTrue(report.render().contains("2 validKey=0 slot(s) stepped over")) + } + + /// #874's rule reaches the CONCLUSION too. The strap flagged an entry past the hole as real; our ASCII + /// filter declined its name. The list still continued, so the finding must still be decisive — reading + /// "inconclusive" here would be our parser deciding a question about the firmware. + func testAnUndecodableEntryAfterAnEmptySlotIsStillDecisive() { + var report = FeatureFlagProbeReport(family: .whoop5) + report.noteStart(FeatureFlagProbe.StartResponse(resultCode: 1, revision: 1, count: 30, + record: [0x01, 0x1E, 0x00])) + XCTAssertTrue(report.noteNext(FeatureFlagProbe.NextResponse( + resultCode: 1, revision: 1, index: 1, validKey: false, key: nil, record: [0x01, 0x01, 0x00]))) + XCTAssertTrue(report.noteNext(FeatureFlagProbe.NextResponse( + resultCode: 1, revision: 1, index: 2, validKey: true, key: nil, + record: [0x01, 0x02, 0x01, 0xDE, 0xAD]))) + XCTAssertEqual(report.keys, [], "no name decoded, so no key is invented") + XCTAssertEqual(report.keysAfterFirstEmptySlot, 0) + XCTAssertEqual(report.validEntriesAfterFirstEmptySlot, 1) + let finding = report.terminatorFinding + XCTAssertTrue(finding.hasPrefix("DECISIVE — validKey=0 is an EMPTY/RETIRED SLOT"), finding) + XCTAssertTrue(finding.contains("1 of them flagged validKey=1"), finding) + XCTAssertFalse(finding.contains("naming"), finding) + } + + /// The other decisive outcome, and the one that costs two round-trips: the strap repeats the same + /// index with `validKey = 0`. A parked cursor has nothing past it, so `validKey = 0` really is a + /// terminator on that firmware — and the report says so in those words rather than assuming it. + func testRepeatedEmptySlotAtTheSameIndexIsReportedAsATerminator() { + var report = FeatureFlagProbeReport(family: .whoop5) + report.noteStart(FeatureFlagProbe.StartResponse(resultCode: 1, revision: 1, count: 9, + record: [0x01, 0x09, 0x00])) + let empty = FeatureFlagProbe.NextResponse(resultCode: 1, revision: 1, index: 7, validKey: false, + key: nil, record: [0x01, 0x07, 0x00]) + XCTAssertTrue(report.noteNext(empty), "the first empty slot is stepped over") + XCTAssertFalse(report.noteNext(empty), "a parked cursor ends the walk") + XCTAssertEqual(report.stopCode, .emptySlotCursorParked) + XCTAssertTrue(report.terminatorFinding.hasPrefix("DECISIVE — validKey=0 is a TERMINATOR"), + report.terminatorFinding) + XCTAssertEqual(report.steps, 2, "settling this must cost two round-trips, not a full cap") + } + + /// A firmware that answers `validKey = 0` forever with an ADVANCING index cannot run the walk away: + /// the consecutive-empty cap stops it, and names itself a CLIENT-side bound so the run is never read + /// as a complete list. + func testAnUnendingRunOfEmptySlotsStopsAtTheConsecutiveCap() { + var report = FeatureFlagProbeReport(family: .whoop4) + var index = 0 + var sent = 0 + while report.noteNext(FeatureFlagProbe.NextResponse( + resultCode: 1, revision: 1, index: index, validKey: false, key: nil, + record: [0x01, UInt8(index), 0x00])) { + index += 1 + sent += 1 + XCTAssertLessThan(sent, FeatureFlagProbe.maxConsecutiveEmptySlots + 2, "walk must not run away") + } + XCTAssertEqual(report.steps, FeatureFlagProbe.maxConsecutiveEmptySlots) + XCTAssertEqual(report.stopCode, .emptySlotRunCap) + let why = report.stopReason ?? "" + XCTAssertTrue(why.contains("a CLIENT-side bound, not the strap's"), why) + XCTAssertTrue(report.terminatorFinding.hasPrefix("INCONCLUSIVE"), report.terminatorFinding) + } + + /// A valid entry resets the run, so scattered holes cost nothing against the cap. + func testAValidEntryResetsTheConsecutiveEmptySlotRun() { + var report = FeatureFlagProbeReport(family: .whoop4) + func empty(_ i: Int) -> FeatureFlagProbe.NextResponse { + FeatureFlagProbe.NextResponse(resultCode: 1, revision: 1, index: i, validKey: false, key: nil, + record: [0x01, UInt8(i), 0x00]) + } + func valid(_ i: Int, _ k: String) -> FeatureFlagProbe.NextResponse { + FeatureFlagProbe.NextResponse(resultCode: 1, revision: 1, index: i, validKey: true, key: k, + record: [0x01, UInt8(i), 0x01]) + } + for i in 0..<(FeatureFlagProbe.maxConsecutiveEmptySlots - 1) { + XCTAssertTrue(report.noteNext(empty(i))) + } + XCTAssertTrue(report.noteNext(valid(20, "hr_ch_switching"))) + for i in 21..<(21 + FeatureFlagProbe.maxConsecutiveEmptySlots - 1) { + XCTAssertTrue(report.noteNext(empty(i)), "the run restarted at the valid entry") + } + XCTAssertNil(report.stopCode) + XCTAssertEqual(report.keys, ["hr_ch_switching"]) + } + func testNonPrintableNameIsNotReportedAsAKey() { let record: [UInt8] = [0x01, 0x02, 0x01, 0xDE, 0xAD, 0xBE, 0xEF] let frame = whoop4Response(cmd: 118, payload: payload(result: 1, record: record)) @@ -198,30 +310,75 @@ final class FeatureFlagProbeTests: XCTestCase { "a dump with holes must describe itself rather than look complete") } - /// `validKey = 0` is trusted as an end marker, but the other reading — an EMPTY SLOT with the list - /// continuing past it — is not ruled out by anything observed so far. Stopping well short of the - /// announced count is the evidence that would settle it, so the report has to say so loudly rather - /// than reporting a confident short list. - func testStoppingOnValidKeyFalseShortOfTheAnnouncedCountIsFlagged() { - var report = FeatureFlagProbeReport(family: .whoop4) - report.noteStart(FeatureFlagProbe.StartResponse(resultCode: 1, revision: 1, count: 40)) + /// The reply a WHOOP 5 MG actually served to end its 115/116 walk: `index = 255` and `validKey = 0` + /// on the SAME frame. Both terminator conditions fired at once, so that run cannot say which one the + /// firmware meant — and the report must say exactly that instead of picking one. + func testEndMarkerCarryingValidKeyZeroIsReportedAsAmbiguous() { + var report = FeatureFlagProbeReport(family: .whoop5, + namespace: FeatureFlagProbe.deviceConfigNamespace) + report.noteStart(FeatureFlagProbe.StartResponse(resultCode: 1, revision: 1, count: 7, + record: [0x01, 0x07, 0x00])) XCTAssertTrue(report.noteNext(FeatureFlagProbe.NextResponse( - resultCode: 1, revision: 1, index: 0, validKey: true, key: "enable_r22_packets"))) + resultCode: 1, revision: 1, index: 1, validKey: true, key: "enable_rfid", + record: [0x01, 0x01, 0x01]))) XCTAssertFalse(report.noteNext(FeatureFlagProbe.NextResponse( - resultCode: 1, revision: 1, index: 1, validKey: false, key: nil))) + resultCode: 1, revision: 1, index: 0xFF, validKey: false, key: nil, + record: [0x01, 0xFF, 0x00, 0x00]))) - let why = try! XCTUnwrap(report.stopReason) - XCTAssertTrue(why.contains("2 of 40 announced entries"), why) - XCTAssertTrue(why.contains("the remainder was NOT walked"), why) + XCTAssertTrue(report.endMarkerAlsoCarriedInvalidFlag) + XCTAssertEqual(report.stopCode, .endMarker) + XCTAssertTrue(report.terminatorFinding.hasPrefix("AMBIGUOUS"), report.terminatorFinding) + XCTAssertTrue(report.terminatorFinding.contains("both terminator conditions fired at once"), + report.terminatorFinding) + XCTAssertTrue(report.render().contains("index=0xFF AND validKey=0 on the same reply")) } - /// …and when the count was fully walked, the plain reason stands — no false alarm. - func testValidKeyFalseAtTheAnnouncedEndIsNotFlagged() { - var report = FeatureFlagProbeReport(family: .whoop4) - report.noteStart(FeatureFlagProbe.StartResponse(resultCode: 1, revision: 1, count: 1)) + /// The 0xFF marker with `validKey` still true separates the two conditions the other way, and the + /// report has to be equally careful there: 0xFF alone terminated, and validKey=0 stayed untested. + func testEndMarkerWithValidKeyTrueSaysValidKeyZeroIsUntested() { + var report = FeatureFlagProbeReport(family: .whoop5) + report.noteStart(FeatureFlagProbe.StartResponse(resultCode: 1, revision: 1, count: 2, + record: [0x01, 0x02, 0x00])) + XCTAssertTrue(report.noteNext(FeatureFlagProbe.NextResponse( + resultCode: 1, revision: 1, index: 1, validKey: true, key: "enable_r22_packets", + record: [0x01, 0x01, 0x01]))) XCTAssertFalse(report.noteNext(FeatureFlagProbe.NextResponse( - resultCode: 1, revision: 1, index: 0, validKey: false, key: nil))) - XCTAssertEqual(report.stopReason, "firmware reported validKey=false") + resultCode: 1, revision: 1, index: 0xFF, validKey: true, key: nil, + record: [0x01, 0xFF, 0x01]))) + XCTAssertFalse(report.endMarkerAlsoCarriedInvalidFlag) + XCTAssertTrue(report.terminatorFinding.contains("validKey=0 was never served, so it is untested"), + report.terminatorFinding) + } + + /// What the 117/118 walk on a 5/MG actually did: sixteen valid, named entries and NO terminator of + /// either kind. The walk used to stop dead at the announced count and read as a complete list. It now + /// asks a bounded number of further times, and — whatever comes back — reports in the verdict itself + /// that a client-side bound ended the run. + func testWalkOvershootsTheAnnouncedCountSoTheStrapCanEndItsOwnList() { + var report = FeatureFlagProbeReport(family: .whoop5) + report.noteStart(FeatureFlagProbe.StartResponse(resultCode: 1, revision: 1, count: 3, + record: [0x01, 0x03, 0x00])) + var sent = 0 + var keepGoing = true + while keepGoing { + sent += 1 + keepGoing = report.noteNext(FeatureFlagProbe.NextResponse( + resultCode: 1, revision: 1, index: sent, validKey: true, key: "key_\(sent)", + record: [0x01, UInt8(sent), 0x01])) + } + XCTAssertEqual(sent, 3 + FeatureFlagProbe.countOvershootAllowance, + "the strap gets \(FeatureFlagProbe.countOvershootAllowance) replies past its own count") + XCTAssertEqual(report.stopCode, .announcedCountOvershoot) + XCTAssertEqual(report.repliesPastAnnouncedCount, FeatureFlagProbe.countOvershootAllowance) + XCTAssertTrue(report.verdict.contains("INCOMPLETE: the walk ended on a client-side bound"), + report.verdict) + XCTAssertTrue(report.terminatorFinding.hasPrefix("NO TERMINATOR OBSERVED"), + report.terminatorFinding) + // …and the count line reports the arithmetic that makes it a finding. + let count = try! XCTUnwrap(report.countFinding) + XCTAssertTrue(count.contains("Keys yielded: 7"), count) + XCTAssertTrue(count.contains("MISMATCH against the announced 3"), count) + XCTAssertTrue(count.contains("kept answering 4 repl(ies) past its own announced count"), count) } /// The verdict must never blame the strap for our own decode. A firmware whose names all fail our @@ -341,10 +498,16 @@ final class FeatureFlagProbeTests: XCTestCase { guard case .success(let n) = FeatureFlagProbe.parseNext(frame: frame, family: .whoop5) else { return XCTFail("next \(i)") } - let keepGoing = report.noteNext(n) - XCTAssertEqual(keepGoing, i < names.count - 1, "the announced count bounds the walk") + XCTAssertTrue(report.noteNext(n), "the announced count no longer ends the walk by itself") + } + // The strap's own end marker — the only thing that ends a walk cleanly. + let end = whoop5Response(cmd: 118, payload: payload(result: 1, record: [0x01, 0xFF, 0x00, 0x00])) + guard case .success(let last) = FeatureFlagProbe.parseNext(frame: end, family: .whoop5) else { + return XCTFail("end marker") } + XCTAssertFalse(report.noteNext(last)) XCTAssertEqual(report.keys, names) + XCTAssertEqual(report.stopCode, .endMarker) let text = report.render() XCTAssertTrue(text.contains("#761 FEATURE-FLAG ENUMERATION PROBE — WHOOP 5/MG")) XCTAssertTrue(text.contains("Read-only")) @@ -352,11 +515,220 @@ final class FeatureFlagProbeTests: XCTestCase { XCTAssertTrue(text.contains("Flags reported by the strap (3 of 3 announced)")) XCTAssertTrue(text.contains(" 1. enable_r22_packets")) XCTAssertTrue(text.contains(" 3. wear_detect_bias")) - XCTAssertTrue(text.contains("walked the 3 entries the strap announced")) + XCTAssertTrue(text.contains("Stop code: endMarker")) XCTAssertTrue(text.contains("START_FF_KEY_EXCHANGE(117) → revision=1 count=3 result=SUCCESS(1)")) XCTAssertTrue(text.contains("SEND_NEXT_FF(118) → index=0 validKey=true key=\"enable_r22_packets\"")) } + // MARK: - Raw bytes + + /// Requirement of the whole exercise: the RAW record bytes of every reply are in the report, not only + /// the fields parsed out of them. Earlier findings here had to be re-run because only parsed output + /// survived, and a parsed field cannot contradict the layout that produced it. + func testEveryReplyLogsItsRawRecordBytes() { + var report = FeatureFlagProbeReport(family: .whoop5) + let start = whoop5Response(cmd: 117, payload: payload(result: 1, record: [0x01, 0x02, 0x00])) + guard case .success(let s) = FeatureFlagProbe.parseStart(frame: start, family: .whoop5) else { + return XCTFail("start") + } + report.noteStart(s) + XCTAssertEqual(s.record, [0x01, 0x02, 0x00], "the parse keeps the bytes it decoded from") + let next = whoop5Response(cmd: 118, payload: payload(result: 1, record: [0x01, 0x00, 0x01, 0x78, 0x00])) + guard case .success(let n) = FeatureFlagProbe.parseNext(frame: next, family: .whoop5) else { + return XCTFail("next") + } + XCTAssertTrue(report.noteNext(n)) + let text = report.render() + XCTAssertTrue(text.contains("raw=01 02 00"), text) + XCTAssertTrue(text.contains("raw=01 00 01 78 00"), text) + } + + /// A reply that failed to decode is the one whose raw bytes matter most, so the whole frame goes in. + func testUndecodedReplyLogsTheWholeRawFrame() { + var report = FeatureFlagProbeReport(family: .whoop4) + report.noteFailure(.crc, command: 118, frame: [0xAA, 0x05, 0x00, 0x2B, 0x24]) + XCTAssertTrue(report.render().contains("raw frame=aa 05 00 2b 24"), report.render()) + XCTAssertEqual(report.stopCode, .parseFailure) + } + + /// The hex is bounded so one absurd record cannot flood the log — but the true LENGTH is always + /// printed, because an over-long record is itself evidence that the layout is wrong. + func testOverLongRawRecordIsElidedButItsLengthIsKept() { + let long = [UInt8](repeating: 0xAB, count: FeatureFlagProbe.maxRawHexBytes + 10) + let rendered = FeatureFlagProbe.hex(long) + XCTAssertTrue(rendered.hasSuffix("… (\(FeatureFlagProbe.maxRawHexBytes + 10) bytes total)"), rendered) + XCTAssertEqual(FeatureFlagProbe.hex([]), "(empty)") + } + + // MARK: - The announced count + + /// The count is read as u16 LE, and every count seen so far has had a zero high byte — where a + /// single-byte read returns the SAME number. So no capture distinguishes the two readings, and the + /// report says that rather than asserting a field width nothing has established. + func testCountCarriesBothReadingsAndSaysWhenTheyAgree() { + let frame = whoop5Response(cmd: 117, payload: payload(result: 1, record: [0x01, 0x10, 0x00])) + guard case .success(let r) = FeatureFlagProbe.parseStart(frame: frame, family: .whoop5) else { + return XCTFail("start") + } + XCTAssertEqual(r.count, 16) + XCTAssertEqual(r.singleByteCount, 16) + XCTAssertEqual(r.countHighByte, 0) + XCTAssertTrue(r.countReadingsAgree) + var report = FeatureFlagProbeReport(family: .whoop5) + report.noteStart(r) + let finding = try! XCTUnwrap(report.countFinding) + XCTAssertTrue(finding.contains("u16 LE read = 16; single-byte read = 16 (high byte 0x00)"), finding) + XCTAssertTrue(finding.contains("the two readings AGREE"), finding) + } + + /// A nonzero high byte is the only thing that separates them, and it is a finding either way: on the + /// u16 reading the list is enormous, on the single-byte reading `record[2]` is padding. + func testNonZeroHighByteIsReportedAsADisagreement() { + let frame = whoop5Response(cmd: 117, payload: payload(result: 1, record: [0x01, 0x07, 0x02])) + guard case .success(let r) = FeatureFlagProbe.parseStart(frame: frame, family: .whoop5) else { + return XCTFail("start") + } + XCTAssertEqual(r.count, 0x0207) + XCTAssertEqual(r.singleByteCount, 7) + XCTAssertFalse(r.countReadingsAgree) + XCTAssertFalse(r.countIsPlausible, "an implausible u16 must not become a loop bound") + var report = FeatureFlagProbeReport(family: .whoop5) + report.noteStart(r) + let finding = try! XCTUnwrap(report.countFinding) + XCTAssertTrue(finding.contains("single-byte read = 7 (high byte 0x02)"), finding) + XCTAssertTrue(finding.contains("the two readings DISAGREE"), finding) + } + + /// Announced-count-versus-yielded is itself a result. A strap that announces sixteen and names + /// fourteen has two entries nobody has accounted for, and the report has to raise it rather than + /// print a tidy short list. + func testAnnouncedCountVersusKeysYieldedMismatchIsReported() { + var report = FeatureFlagProbeReport(family: .whoop5) + report.noteStart(FeatureFlagProbe.StartResponse(resultCode: 1, revision: 1, count: 16, + record: [0x01, 0x10, 0x00])) + XCTAssertTrue(report.noteNext(FeatureFlagProbe.NextResponse( + resultCode: 1, revision: 1, index: 1, validKey: true, key: "enable_r22_packets", + record: [0x01, 0x01, 0x01]))) + XCTAssertFalse(report.noteNext(FeatureFlagProbe.NextResponse( + resultCode: 1, revision: 1, index: 0xFF, validKey: true, key: nil, + record: [0x01, 0xFF, 0x01]))) + let finding = try! XCTUnwrap(report.countFinding) + XCTAssertTrue(finding.contains("Keys yielded: 1"), finding) + XCTAssertTrue(finding.contains("MISMATCH against the announced 16"), finding) + } + + /// …and when everything is accounted for — keys plus skipped plus empty slots — there is no alarm. + func testFullyAccountedCountIsNotFlaggedAsAMismatch() { + var report = FeatureFlagProbeReport(family: .whoop5) + report.noteStart(FeatureFlagProbe.StartResponse(resultCode: 1, revision: 1, count: 3, + record: [0x01, 0x03, 0x00])) + _ = report.noteNext(FeatureFlagProbe.NextResponse(resultCode: 1, revision: 1, index: 1, + validKey: true, key: "a", record: [0x01, 0x01, 0x01])) + _ = report.noteNext(FeatureFlagProbe.NextResponse(resultCode: 1, revision: 1, index: 2, + validKey: true, key: nil, record: [0x01, 0x02, 0x01])) + _ = report.noteNext(FeatureFlagProbe.NextResponse(resultCode: 1, revision: 1, index: 3, + validKey: false, key: nil, record: [0x01, 0x03, 0x00])) + let finding = try! XCTUnwrap(report.countFinding) + XCTAssertFalse(finding.contains("MISMATCH"), finding) + XCTAssertTrue(finding.contains("Keys yielded: 1 (+1 undecodable) (+1 empty slot(s))"), finding) + } + + // MARK: - The device-config namespace (115/116) + + /// The SAME walk drives the device-config pair, so the terminator rules cannot be corrected in one + /// namespace and left wrong in the other. Decoded end-to-end from 115/116 frames. + func testDeviceConfigNamespaceWalksThroughTheSameCorrectedRules() { + var report = FeatureFlagProbeReport(family: .whoop5, + namespace: FeatureFlagProbe.deviceConfigNamespace) + let start = whoop5Response(cmd: 115, payload: payload(result: 1, record: [0x01, 0x02, 0x00])) + guard case .success(let s) = FeatureFlagProbe.parseStart(frame: start, family: .whoop5, + expecting: 115) else { + return XCTFail("115 start") + } + report.noteStart(s) + // An empty slot first — under the old rule the walk would have ended here with zero keys. + let hole = whoop5Response(cmd: 116, payload: payload(result: 1, record: [0x01, 0x02, 0x00, 0x00])) + guard case .success(let h) = FeatureFlagProbe.parseNext(frame: hole, family: .whoop5, + expecting: 116) else { + return XCTFail("116 hole") + } + XCTAssertTrue(report.noteNext(h)) + let named = whoop5Response(cmd: 116, payload: payload( + result: 1, record: [0x01, 0x03, 0x01] + keyBytes("enable_raw_data_w_ecg"))) + guard case .success(let n) = FeatureFlagProbe.parseNext(frame: named, family: .whoop5, + expecting: 116) else { + return XCTFail("116 named") + } + XCTAssertTrue(report.noteNext(n)) + XCTAssertEqual(report.keys, ["enable_raw_data_w_ecg"], + "the key AFTER the empty slot is exactly what the old rule discarded") + + let text = report.render() + XCTAssertTrue(text.contains("#761 DEVICE-CONFIG ENUMERATION PROBE — WHOOP 5/MG"), text) + XCTAssertTrue(text.contains("START_DEVICE_CONFIG_KEY_EXCHANGE(115) + SEND_NEXT_DEVICE_CONFIG(116)"), text) + XCTAssertTrue(text.contains("Keys reported by the strap (1 of 2 announced"), text) + XCTAssertTrue(text.contains("SEND_NEXT_DEVICE_CONFIG(116) → index=3"), text) + } + + /// An explicit refusal is the strap's answer, not one of our bounds, and it must end the walk with its + /// own name — on the START reply (where the driver reads `hasStopped` instead of stepping to 118) and + /// on a NEXT reply alike. + func testUnsupportedRefusalEndsTheWalkWithItsOwnStopCode() { + var start = FeatureFlagProbeReport(family: .whoop5) + XCTAssertFalse(start.hasStopped) + start.noteStart(FeatureFlagProbe.StartResponse(resultCode: 3, revision: 0, count: 0, + record: [0x00, 0x00, 0x00])) + XCTAssertTrue(start.hasStopped, "a refused START must not step on to the next verb") + XCTAssertEqual(start.stopCode, .unsupported) + XCTAssertTrue(start.render().contains("Stop code: unsupported")) + + var mid = FeatureFlagProbeReport(family: .whoop5) + mid.noteStart(FeatureFlagProbe.StartResponse(resultCode: 1, revision: 1, count: 4, + record: [0x01, 0x04, 0x00])) + XCTAssertTrue(mid.noteNext(FeatureFlagProbe.NextResponse( + resultCode: 1, revision: 1, index: 1, validKey: true, key: "enable_r22_packets", + record: [0x01, 0x01, 0x01]))) + XCTAssertFalse(mid.noteNext(FeatureFlagProbe.NextResponse( + resultCode: 3, revision: 0, index: 0, validKey: false, key: nil, record: [0x00, 0x00, 0x00]))) + XCTAssertEqual(mid.stopCode, .unsupported) + XCTAssertEqual(mid.emptySlots, 0, "a refusal is never counted as an empty slot") + XCTAssertTrue(mid.stopReason?.contains("UNSUPPORTED(3)") == true, mid.stopReason ?? "") + } + + /// The two platforms must classify the same reply the same way, or a strap log means different things + /// on either side. These are the exact constants the walk is bounded by; the Kotlin twin + /// (`walkBoundsMatchTheSwiftTwin`) asserts the same numbers and the same stop-code spellings. + func testWalkBoundsMatchTheKotlinTwin() { + XCTAssertEqual(FeatureFlagProbe.maxConsecutiveEmptySlots, 8) + XCTAssertEqual(FeatureFlagProbe.countOvershootAllowance, 4) + XCTAssertEqual(FeatureFlagProbe.maxRawHexBytes, 64) + XCTAssertEqual(FeatureFlagProbe.maxFlags, 128) + XCTAssertEqual(FeatureFlagProbe.StopCode.endMarker.rawValue, "endMarker") + XCTAssertEqual(FeatureFlagProbe.StopCode.emptySlotCursorParked.rawValue, "emptySlotCursorParked") + XCTAssertEqual(FeatureFlagProbe.StopCode.emptySlotRunCap.rawValue, "emptySlotRunCap") + XCTAssertEqual(FeatureFlagProbe.StopCode.stepCap.rawValue, "stepCap") + XCTAssertEqual(FeatureFlagProbe.StopCode.announcedCountOvershoot.rawValue, "announcedCountOvershoot") + XCTAssertEqual(FeatureFlagProbe.StopCode.timeout.rawValue, "timeout") + XCTAssertEqual(FeatureFlagProbe.StopCode.unsupported.rawValue, "unsupported") + XCTAssertEqual(FeatureFlagProbe.StopCode.parseFailure.rawValue, "parseFailure") + } + + /// The 115/116 frames must not decode as 117/118 by accident — the opcode is checked, both ways. + func testTheTwoNamespacesDoNotDecodeEachOthersFrames() { + let cfg = whoop5Response(cmd: 116, payload: payload(result: 1, record: [0x01, 0x00, 0x01])) + XCTAssertEqual(FeatureFlagProbe.parseNext(frame: cfg, family: .whoop5), .failure(.wrongCommand)) + let ff = whoop5Response(cmd: 118, payload: payload(result: 1, record: [0x01, 0x00, 0x01])) + XCTAssertEqual(FeatureFlagProbe.parseNext(frame: ff, family: .whoop5, expecting: 116), + .failure(.wrongCommand)) + XCTAssertEqual(FeatureFlagProbe.deviceConfigNamespace.startCmd, 115) + XCTAssertEqual(FeatureFlagProbe.deviceConfigNamespace.nextCmd, 116) + // Still read-only: the SET verbs appear in neither namespace. + for ns in [FeatureFlagProbe.featureFlagNamespace, FeatureFlagProbe.deviceConfigNamespace] { + XCTAssertFalse([119, 120].contains(Int(ns.startCmd))) + XCTAssertFalse([119, 120].contains(Int(ns.nextCmd))) + } + } + func testRepeatedKeysCannotDriveAnUnboundedWalk() { // A firmware whose cursor never advances repeats one entry: the reply-count cap must stop it. var report = FeatureFlagProbeReport(family: .whoop4) @@ -417,14 +789,17 @@ final class FeatureFlagProbeTests: XCTestCase { Verdict: enumerated 1 feature-flag key name(s) Stopped: cursor exhausted (index 0xFF) + Stop code: endMarker + Terminator: AMBIGUOUS — the walk ended on a reply carrying index=0xFF AND validKey=0 together, so both terminator conditions fired at once and this run cannot say which one the firmware meant. + Announced count: u16 LE read = 2; single-byte read = 2 (high byte 0x00) — the two readings AGREE, so this reply does not establish the field's width. Keys yielded: 1 — MISMATCH against the announced 2 Flags reported by the strap (1 of 2 announced): 1. enable_r22_packets Exchange: - START_FF_KEY_EXCHANGE(117) → revision=1 count=2 result=SUCCESS(1) - SEND_NEXT_FF(118) → index=0 validKey=true key="enable_r22_packets" result=SUCCESS(1) - SEND_NEXT_FF(118) → index=255 validKey=false result=SUCCESS(1) + START_FF_KEY_EXCHANGE(117) → revision=1 count=2 result=SUCCESS(1) raw=01 02 00 + SEND_NEXT_FF(118) → index=0 validKey=true key="enable_r22_packets" result=SUCCESS(1) raw=01 00 01 65 6e 61 62 6c 65 5f 72 32 32 5f 70 61 63 6b 65 74 73 00 00 00 00 00 00 00 00 00 00 + SEND_NEXT_FF(118) → index=255 validKey=false result=SUCCESS(1) raw=01 ff 00 00 00 00 00 (index=0xFF AND validKey=0 on the same reply — both terminator conditions at once) """ XCTAssertEqual(report.render(), golden) diff --git a/Strand/BLE/BLEManager.swift b/Strand/BLE/BLEManager.swift index aaad54f415..df03d82cff 100644 --- a/Strand/BLE/BLEManager.swift +++ b/Strand/BLE/BLEManager.swift @@ -2822,9 +2822,18 @@ public final class BLEManager: NSObject, ObservableObject { switch FeatureFlagProbe.parseStart(frame: frame, family: family) { case .success(let r): featureFlagReport?.noteStart(r) - sendFeatureFlagStep(.sendNextFeatureFlag) + // A firmware that REFUSED 117 has nothing to enumerate; sending 118 anyway would spend a + // round-trip to learn what the refusal already said. `hasStopped` is the report's own + // named reason, so the driver never has to re-derive one. + if featureFlagReport?.hasStopped == true { + finishFeatureFlagProbe() + } else { + sendFeatureFlagStep(.sendNextFeatureFlag) + } case .failure(let f): - featureFlagReport?.noteFailure(f, command: Int(awaiting)) + // Pass the frame: a reply that failed to decode is the one whose RAW bytes matter most, + // and a report that kept only the parsed fields cannot be re-examined later. + featureFlagReport?.noteFailure(f, command: Int(awaiting), frame: frame) finishFeatureFlagProbe() } return 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 be009f1544..deb92ba83b 100644 --- a/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt +++ b/android/app/src/main/java/com/noop/ble/WhoopBleClient.kt @@ -3489,9 +3489,18 @@ class WhoopBleClient( val start = parsed.value if (start != null) { featureFlagReport?.noteStart(start) - sendFeatureFlagStep(CommandNumber.SEND_NEXT_FF) + // A firmware that REFUSED 117 has nothing to enumerate; sending 118 anyway would spend a + // round-trip to learn what the refusal already said. hasStopped is the report's own named + // reason, so the driver never has to re-derive one. + if (featureFlagReport?.hasStopped == true) { + finishFeatureFlagProbe() + } else { + sendFeatureFlagStep(CommandNumber.SEND_NEXT_FF) + } } else { - featureFlagReport?.noteFailure(parsed.failure!!, awaiting) + // Pass the frame: a reply that failed to decode is the one whose RAW bytes matter most, + // and a report that kept only the parsed fields cannot be re-examined later. + featureFlagReport?.noteFailure(parsed.failure!!, awaiting, frame) finishFeatureFlagProbe() } return diff --git a/android/app/src/main/java/com/noop/protocol/FeatureFlagProbe.kt b/android/app/src/main/java/com/noop/protocol/FeatureFlagProbe.kt index 93eef6ded4..3afa05f7f9 100644 --- a/android/app/src/main/java/com/noop/protocol/FeatureFlagProbe.kt +++ b/android/app/src/main/java/com/noop/protocol/FeatureFlagProbe.kt @@ -63,6 +63,111 @@ object FeatureFlagProbe { /** Longest key name accepted from a reply (the SET side NUL-pads names to 32 bytes). */ const val MAX_KEY_LENGTH = 32 + /** START_DEVICE_CONFIG_KEY_EXCHANGE (115 / 0x73) — the device-config twin of 117. Read-only. */ + const val START_DEVICE_CONFIG_KEY_EXCHANGE_CMD = 115 + + /** SEND_NEXT_DEVICE_CONFIG (116 / 0x74) — the device-config twin of 118. Read-only. */ + const val SEND_NEXT_DEVICE_CONFIG_CMD = 116 + + /** + * How many consecutive `validKey = 0` replies the walk steps over before it gives up on the + * EMPTY-SLOT reading (see [NextResponse.isEmptySlot]). + * + * Calibrated on the one 117/118 walk this project has: that strap announced 16 keys and served + * indices **1–10 then 13–18** — a two-slot hole it handled itself, without ever sending + * `validKey = 0`. A firmware that instead exposes its holes would show them as runs of that size, so + * eight is four times the largest hole ever observed and still a hard bound. + */ + const val MAX_CONSECUTIVE_EMPTY_SLOTS = 8 + + /** + * Extra SEND_NEXT replies the walk takes AFTER the strap's announced count is satisfied, so the + * strap — not our arithmetic — gets to end its own list. The 117/118 walk stopped at exactly + * `steps == count`, having seen neither `index = 0xFF` nor `validKey = 0`; the same strap served + * indices up to 18 while announcing 16, and four is twice that observed excess. + */ + const val COUNT_OVERSHOOT_ALLOWANCE = 4 + + /** + * Ceiling on record bytes rendered as hex in one trace line. The byte COUNT is printed even when the + * tail is elided, so a suspiciously long record still reads as one. + */ + const val MAX_RAW_HEX_BYTES = 64 + + /** + * One enumerate pair — the opcodes the walk drives and the words the report prints for them. + * + * The protocol's own `CommandNumber` table names two symmetric enumerate pairs (117/118 for feature + * flags, 115/116 for device config). Both are walked by the SAME code here, so a fix to the walk's + * terminator handling cannot apply to one namespace and miss the other. Reusing this decoder for + * 115/116 ASSUMES the two share a record layout — an inference from that table's naming symmetry, + * not an observation. It fails closed. + */ + data class Namespace( + val startCmd: Int, + val nextCmd: Int, + val startLabel: String, + val nextLabel: String, + /** Report title, e.g. `FEATURE-FLAG ENUMERATION PROBE`. */ + val title: String, + /** What the namespace is called in prose, e.g. `feature-flag`. */ + val noun: String, + /** What ONE entry is called in prose, e.g. `flag`. */ + val entryNoun: String, + /** Heading for the collected list, e.g. `Flags`. */ + val listHeading: String, + ) + + /** 117/118 — the feature-flag key list. */ + val FEATURE_FLAG_NAMESPACE = Namespace( + startCmd = START_KEY_EXCHANGE_CMD, nextCmd = SEND_NEXT_FLAG_CMD, + startLabel = "START_FF_KEY_EXCHANGE", nextLabel = "SEND_NEXT_FF", + title = "FEATURE-FLAG ENUMERATION PROBE", noun = "feature-flag", entryNoun = "flag", + listHeading = "Flags", + ) + + /** 115/116 — the device-config key list. */ + val DEVICE_CONFIG_NAMESPACE = Namespace( + startCmd = START_DEVICE_CONFIG_KEY_EXCHANGE_CMD, nextCmd = SEND_NEXT_DEVICE_CONFIG_CMD, + startLabel = "START_DEVICE_CONFIG_KEY_EXCHANGE", nextLabel = "SEND_NEXT_DEVICE_CONFIG", + title = "DEVICE-CONFIG ENUMERATION PROBE", noun = "device-config", entryNoun = "config key", + listHeading = "Keys", + ) + + /** + * Why a walk stopped. A STRING reason reads well in a log and a CODE can be asserted on, and both are + * always set together: a truncated walk that reports no reason at all is precisely how a partial key + * list gets mistaken for a complete one. [code] matches the Swift `StopCode` raw values exactly. + */ + enum class StopCode(val code: String) { + /** The strap served `index = 0xFF`. The one terminator this project has seen unambiguously. */ + END_MARKER("endMarker"), + + /** + * A `validKey = 0` reply repeated the previous reply's index: the cursor is parked, so on this + * firmware `validKey = 0` behaves as a terminator rather than as an empty slot. + */ + EMPTY_SLOT_CURSOR_PARKED("emptySlotCursorParked"), + + /** [MAX_CONSECUTIVE_EMPTY_SLOTS] consecutive `validKey = 0` replies, index still advancing. */ + EMPTY_SLOT_RUN_CAP("emptySlotRunCap"), + + /** [MAX_FLAGS] replies. A client-side bound: says nothing about where the strap's list ends. */ + STEP_CAP("stepCap"), + + /** The announced count plus [COUNT_OVERSHOOT_ALLOWANCE] was spent. Also a client-side bound. */ + ANNOUNCED_COUNT_OVERSHOOT("announcedCountOvershoot"), + + /** The strap answered nothing inside the probe's window. */ + TIMEOUT("timeout"), + + /** The strap refused the verb (result `UNSUPPORTED`). */ + UNSUPPORTED("unsupported"), + + /** A reply did not decode. Our limitation, and labelled as ours. */ + PARSE_FAILURE("parseFailure"), + } + /** COMMAND_RESPONSE packet type (36 / 0x24). */ private const val COMMAND_RESPONSE_TYPE = 36 @@ -73,9 +178,39 @@ object FeatureFlagProbe { enum class ParseFailure { CRC, ENVELOPE, WRONG_COMMAND, TRUNCATED } /** Decoded `START_FF_KEY_EXCHANGE` reply. */ - data class StartResponse(val resultCode: Int?, val revision: Int, val count: Int) { + data class StartResponse( + val resultCode: Int?, + val revision: Int, + /** + * `numberOfFeatureFlags` read as u16 LE (`record[1] | record[2] shl 8`). NOT trusted as a loop + * bound on its own, and NOT the only defensible reading of those bytes — see [singleByteCount]. + */ + val count: Int, + /** + * The raw record bytes this response was decoded from, kept so the report can print them. + * Parsed fields are a claim about the layout; these bytes are the evidence for it. + */ + val record: List = emptyList(), + ) { /** True when the count is inside the range a real key list could plausibly occupy. */ val countIsPlausible: Boolean get() = count > 0 && count <= MAX_FLAGS + + /** + * The same field read as a SINGLE byte — i.e. on the reading where `record[2]` is padding rather + * than the count's high byte. + * + * The layout this decoder implements calls the field u16 LE. Nothing observed here settles that: + * every count seen so far has had `record[2] == 0x00`, where a u16 read and a single-byte read + * return the SAME number. Both are therefore carried, and the report says plainly when they + * agree (no evidence either way) and when they differ (which is itself the finding). + */ + val singleByteCount: Int? get() = if (record.size >= 2) record[1].toInt() and 0xFF else null + + /** `record[2]` — the byte a u16 reading treats as the count's high half. */ + val countHighByte: Int? get() = if (record.size >= 3) record[2].toInt() and 0xFF else null + + /** True when the two readings return the same number, i.e. this reply is silent on the width. */ + val countReadingsAgree: Boolean get() = countHighByte == 0 } /** Decoded `SEND_NEXT_FF` reply. */ @@ -86,18 +221,39 @@ object FeatureFlagProbe { val index: Int, val validKey: Boolean, val key: String?, + /** The raw record bytes this response was decoded from, kept so the report can print them. */ + val record: List = emptyList(), ) { /** - * The STRAP said stop: it returned the 0xFF end marker, or flagged the entry as not a real key. - * Both are the firmware's own signal, so both are trusted. + * The strap's UNAMBIGUOUS end marker: `index = 0xFF`. + * + * This used to also mean `validKey = 0`, on the reading that the firmware sets that flag to + * signal the end of its list. That reading is not established. This file's own header note says + * the record layout is UNVERIFIED on 5/MG — it was derived from a WHOOP 4.0 with an R19-era key + * list — and the terminator semantics came with the layout. + * + * What a 5/MG (WS50_r03) actually served, on the only walks this project has: * - * Deliberately does NOT include `key == null`. That is OUR parser declining a name — a byte - * outside printable ASCII, or one longer than [MAX_KEY_LENGTH] — and it says nothing about - * whether the strap has more entries. Treating it as a terminator meant one undecodable name - * threw away every entry after it: a list of forty with a bad byte at seven yielded six keys. - * See [isSkippable]. + * - 117/118: sixteen replies, every one `validKey = 1` with a decodable name, indices 1–10 then + * 13–18. The strap handled its own two-slot hole internally. Neither `validKey = 0` nor + * `index = 0xFF` was ever served — the walk ended because the client stopped asking at the + * announced count. + * - 115/116: the final reply carried `index = 255` AND `validKey = 0` together. Both conditions + * fired on the same frame, so that run cannot say which one the firmware meant. + * + * So on this hardware the disjunction has never been separated. If `validKey = 0` in fact marks + * an EMPTY or RETIRED SLOT with the list continuing past it, then stopping there truncates — the + * identical failure [isSkippable] exists to prevent, one condition over. [isEmptySlot] is that + * case, and the walk now steps over it and records what comes next. + */ + val isExhausted: Boolean get() = index == 0xFF + + /** + * `validKey = 0` WITHOUT the 0xFF end marker — the ambiguous case, and the one this probe exists + * to resolve. Recorded, stepped over, and bounded by [MAX_CONSECUTIVE_EMPTY_SLOTS]; what the + * strap serves next is the evidence that separates "end of list" from "empty slot". */ - val isExhausted: Boolean get() = !validKey || index == 0xFF + val isEmptySlot: Boolean get() = !validKey && index != 0xFF /** * The firmware calls this a real key but the name did not decode. Record it and KEEP WALKING. @@ -113,19 +269,40 @@ object FeatureFlagProbe { /** One decode outcome: exactly one of [value] / [failure] is non-null. */ data class Parsed(val value: T?, val failure: ParseFailure?) - /** Decode a `START_FF_KEY_EXCHANGE` COMMAND_RESPONSE. CRC-gated. */ - fun parseStart(frame: ByteArray, family: DeviceFamily): Parsed { - val e = extract(frame, family, START_KEY_EXCHANGE_CMD) + /** + * Decode a `START_FF_KEY_EXCHANGE` COMMAND_RESPONSE. CRC-gated. + * + * [expecting] defaults to 117 and exists so the DEVICE-CONFIG twin + * `START_DEVICE_CONFIG_KEY_EXCHANGE` (115) is decoded by this same code — the reuse contract + * documented on [Namespace]. + */ + fun parseStart( + frame: ByteArray, + family: DeviceFamily, + expecting: Int = START_KEY_EXCHANGE_CMD, + ): Parsed { + val e = extract(frame, family, expecting) e.failure?.let { return Parsed(null, it) } val r = e.value!! if (r.record.size < 3) return Parsed(null, ParseFailure.TRUNCATED) val count = (r.record[1].toInt() and 0xFF) or ((r.record[2].toInt() and 0xFF) shl 8) - return Parsed(StartResponse(r.resultCode, r.record[0].toInt() and 0xFF, count), null) + return Parsed( + StartResponse(r.resultCode, r.record[0].toInt() and 0xFF, count, r.record.toList()), + null, + ) } - /** Decode a `SEND_NEXT_FF` COMMAND_RESPONSE. CRC-gated like [parseStart]. */ - fun parseNext(frame: ByteArray, family: DeviceFamily): Parsed { - val e = extract(frame, family, SEND_NEXT_FLAG_CMD) + /** + * Decode a `SEND_NEXT_FF` COMMAND_RESPONSE. CRC-gated like [parseStart]. [expecting] defaults to 118 + * and carries the same reuse contract: 116 (`SEND_NEXT_DEVICE_CONFIG`) walks the device-config + * namespace through this decoder. + */ + fun parseNext( + frame: ByteArray, + family: DeviceFamily, + expecting: Int = SEND_NEXT_FLAG_CMD, + ): Parsed { + val e = extract(frame, family, expecting) e.failure?.let { return Parsed(null, it) } val r = e.value!! // revision + index are the minimum: the 0xFF end marker arrives with nothing after it. @@ -134,7 +311,7 @@ object FeatureFlagProbe { val index = r.record[1].toInt() and 0xFF val validKey = if (r.record.size >= 3) r.record[2].toInt() != 0 else false val key = if (r.record.size >= 4) asciiKey(r.record.copyOfRange(3, r.record.size)) else null - return Parsed(NextResponse(r.resultCode, revision, index, validKey, key), null) + return Parsed(NextResponse(r.resultCode, revision, index, validKey, key, r.record.toList()), null) } private class Extracted(val record: ByteArray, val resultCode: Int?) @@ -177,6 +354,25 @@ object FeatureFlagProbe { return if (out.isEmpty()) null else out.toString() } + /** + * Lower-case space-separated hex, for putting the RAW bytes of a reply in the log next to the fields + * decoded from them. + * + * Parsed output alone has repeatedly turned out to be insufficient evidence here: a claim about the + * record layout cannot be re-checked, or contradicted, from the fields that layout produced. Over + * [MAX_RAW_HEX_BYTES] the tail is elided and the true length is printed, so the elision itself is + * visible and a suspiciously long record still reads as one. + */ + fun hex(bytes: List): String { + if (bytes.isEmpty()) return "(empty)" + var out = bytes.take(MAX_RAW_HEX_BYTES).joinToString(" ") { "%02x".format(it.toInt() and 0xFF) } + if (bytes.size > MAX_RAW_HEX_BYTES) out += " … (${bytes.size} bytes total)" + return out + } + + /** [hex] over a raw frame. */ + fun hex(bytes: ByteArray): String = hex(bytes.toList()) + /** 5/MG result-code label, matching the body-location probe's table. */ fun resultLabel(code: Int): String = when (code) { 0 -> "FAILURE" @@ -192,7 +388,14 @@ object FeatureFlagProbe { * the strap log both show. Pure and order-dependent (start → next… → finish). Twin of Swift * `FeatureFlagProbeReport`; [render] is byte-identical across platforms. */ -class FeatureFlagProbeReport(private val family: DeviceFamily) { +class FeatureFlagProbeReport( + private val family: DeviceFamily, + /** + * Which enumerate pair this walk drives — 117/118 or 115/116. The walk logic is identical; only the + * opcodes and the words differ, so neither namespace can drift from the other's terminator rules. + */ + val namespace: FeatureFlagProbe.Namespace = FeatureFlagProbe.FEATURE_FLAG_NAMESPACE, +) { private val _keys = mutableListOf() /** Key names collected, in the order the strap reported them. */ @@ -210,6 +413,14 @@ class FeatureFlagProbeReport(private val family: DeviceFamily) { var reportedCount: Int? = null private set + /** The same count read as a single byte (`record[1]`), when the record carried one. */ + var reportedCountSingleByte: Int? = null + private set + + /** `record[2]` — the byte a u16 reading treats as the count's high half. */ + var reportedCountHighByte: Int? = null + private set + /** Result code of the START reply on 5/MG (null on 4.0, or before it lands). */ var startResult: Int? = null private set @@ -221,89 +432,228 @@ class FeatureFlagProbeReport(private val family: DeviceFamily) { var skipped: Int = 0 private set + /** Replies carrying `validKey = 0` WITHOUT the 0xFF end marker, which the walk stepped over. */ + var emptySlots: Int = 0 + private set + + /** True once any `validKey = 0` reply arrived without the 0xFF marker. */ + var sawEmptySlot: Boolean = false + private set + + /** True once `index = 0xFF` arrived. */ + var sawEndMarker: Boolean = false + private set + + /** + * True when the 0xFF reply ALSO carried `validKey = 0` — i.e. both terminator conditions fired on one + * frame, and that run cannot say which one the firmware meant. This is what a 5/MG served on 115/116. + */ + var endMarkerAlsoCarriedInvalidFlag: Boolean = false + private set + + /** + * Replies the strap served AFTER the first `validKey = 0`. Any value above zero is the decisive + * observation: the list did not end there. + */ + var repliesAfterFirstEmptySlot: Int = 0 + private set + + /** Key names collected after the first `validKey = 0` — keys a walk that stopped there would lose. */ + var keysAfterFirstEmptySlot: Int = 0 + private set + + /** + * Replies after the first `validKey = 0` that the FIRMWARE flagged as real entries, whether or not + * this parser could decode their names. + * + * This — not the decoded-key count — is what makes the empty-slot reading decisive. #874's rule applies + * to the conclusion as much as to the walk: if the strap named an entry past the hole and our ASCII + * filter declined it, the list still continued, and reporting "inconclusive" there would be our + * parser's limitation deciding a question about the firmware. + */ + var validEntriesAfterFirstEmptySlot: Int = 0 + private set + + /** Replies taken past the strap's announced count, under [FeatureFlagProbe.COUNT_OVERSHOOT_ALLOWANCE]. */ + var repliesPastAnnouncedCount: Int = 0 + private set + /** Set once the walk stopped for a reason we can name. */ var stopReason: String? = null private set + /** The same reason as a code, so a caller (or a test) can branch on it rather than on prose. */ + var stopCode: FeatureFlagProbe.StopCode? = null + private set + + /** + * True once the walk has a reason to stop. The BLE driver consults this after EVERY reply, so a + * refusal recorded on the START reply ends the run instead of being stepped over into the next verb. + */ + val hasStopped: Boolean get() = stopCode != null + + /** The index the previous reply carried, for spotting a cursor that stopped advancing. */ + private var lastIndex: Int? = null + + /** Length of the current unbroken run of `validKey = 0` replies, reset by any valid entry. */ + private var consecutiveEmptySlots = 0 + /** Record the START reply. */ fun noteStart(r: FeatureFlagProbe.StartResponse) { reportedCount = r.count + reportedCountSingleByte = r.singleByteCount + reportedCountHighByte = r.countHighByte startResult = r.resultCode - var line = "START_FF_KEY_EXCHANGE(117) → revision=${r.revision} count=${r.count}" + var line = "${namespace.startLabel}(${namespace.startCmd}) → revision=${r.revision} count=${r.count}" r.resultCode?.let { line += " result=${FeatureFlagProbe.resultLabel(it)}($it)" } + line += " raw=${FeatureFlagProbe.hex(r.record)}" if (!r.countIsPlausible) { line += " ⚠︎ count outside 1…${FeatureFlagProbe.MAX_FLAGS} — treated as unknown, the walk " + "still stops on the strap's own end marker" } _trace.add(line) + if (r.resultCode == 3) { + stopReason = "strap refused ${namespace.startLabel}(${namespace.startCmd}) with UNSUPPORTED(3)" + stopCode = FeatureFlagProbe.StopCode.UNSUPPORTED + } } - /** Record one SEND_NEXT reply. Returns true when the walk should continue. */ + /** + * Record one SEND_NEXT reply. Returns true when the walk should continue. + * + * The decision order IS the experiment. `index = 0xFF` ends the walk, because that marker is the one + * thing a strap has served here unambiguously. `validKey = 0` on its own does NOT end it: the walk + * steps over the entry, sends the next-record verb again, and records what comes back — the only way + * to tell "end of list" from "empty slot" apart. Everything past that point is a CLIENT-side bound, + * and each one names itself in [stopCode] so a walk that ended on our arithmetic can never be read as + * a walk the strap ended. + */ fun noteNext(r: FeatureFlagProbe.NextResponse): Boolean { steps += 1 - var line = "SEND_NEXT_FF(118) → index=${r.index} validKey=${r.validKey}" + if (sawEmptySlot) repliesAfterFirstEmptySlot += 1 + var line = "${namespace.nextLabel}(${namespace.nextCmd}) → index=${r.index} validKey=${r.validKey}" r.key?.let { line += " key=\"$it\"" } r.resultCode?.let { line += " result=${FeatureFlagProbe.resultLabel(it)}($it)" } + line += " raw=${FeatureFlagProbe.hex(r.record)}" _trace.add(line) + + // 0. An explicit refusal. UNSUPPORTED(3) says the firmware does not serve this verb, so nothing in + // the record is meaningful and there is nothing to walk. Checked FIRST, before any field is acted + // on, and named as the strap's answer rather than as one of our bounds. + if (r.resultCode == 3) { + stopReason = "strap refused ${namespace.nextLabel}(${namespace.nextCmd}) with UNSUPPORTED(3)" + stopCode = FeatureFlagProbe.StopCode.UNSUPPORTED + return false + } + + // 1. The strap's own unambiguous end marker. Nothing overrides this. if (r.isExhausted) { - if (r.index == 0xFF) { - stopReason = "cursor exhausted (index 0xFF)" - } else { - // `validKey = 0` is documented as an end marker alongside 0xFF, and it is the firmware's - // own flag, so it is trusted here. But nothing observed so far rules out the other - // reading — that it marks an EMPTY SLOT and the list continues past it. If so this stops - // on the first hole, which is the same truncation [isSkippable] exists to prevent, one - // condition over. It cannot be settled without a strap, so instead of guessing we make - // the discrepancy loud: stopping here well short of the announced count is exactly the - // evidence that would settle it. - stopReason = "firmware reported validKey=false" - val count = reportedCount - if (count != null && count > steps) { - stopReason = "firmware reported validKey=false at index ${r.index} after $steps of " + - "$count announced entries — if validKey=0 marks an empty slot rather than the " + - "end of the list, the remainder was NOT walked (see #872 review)" - } + sawEndMarker = true + if (!r.validKey) { + endMarkerAlsoCarriedInvalidFlag = true + _trace[_trace.size - 1] = _trace[_trace.size - 1] + + " (index=0xFF AND validKey=0 on the same reply — both terminator conditions at once)" } + stopReason = "cursor exhausted (index 0xFF)" + stopCode = FeatureFlagProbe.StopCode.END_MARKER return false } - if (r.isSkippable) { - // Our decode declined the name; the strap still says the entry is real and may have more after - // it. Count it so a partial dump describes itself instead of looking complete. - skipped += 1 + + // 2. `validKey = 0` with no end marker: the ambiguous case. Step over it and keep asking. + if (r.isEmptySlot) { + val parked = lastIndex == r.index + lastIndex = r.index + emptySlots += 1 + consecutiveEmptySlots += 1 + val runLength = consecutiveEmptySlots + sawEmptySlot = true _trace[_trace.size - 1] = _trace[_trace.size - 1] + - " (name did not decode — skipped, walk continues)" + " (validKey=0 without the 0xFF marker — treated as an EMPTY SLOT, walk continues)" + if (parked) { + // The cursor did not move. A firmware that means "end of list" parks there and repeats + // itself, so this is the observation that settles the ambiguity the other way — and it + // costs two round-trips rather than a full cap's worth. + stopReason = "validKey=0 repeated at index ${r.index} without advancing — the cursor is " + + "parked, so on this firmware validKey=0 IS a terminator" + stopCode = FeatureFlagProbe.StopCode.EMPTY_SLOT_CURSOR_PARKED + return false + } + if (runLength >= FeatureFlagProbe.MAX_CONSECUTIVE_EMPTY_SLOTS) { + stopReason = "$runLength consecutive validKey=0 replies (cap " + + "${FeatureFlagProbe.MAX_CONSECUTIVE_EMPTY_SLOTS}) — a CLIENT-side bound, not the strap's" + stopCode = FeatureFlagProbe.StopCode.EMPTY_SLOT_RUN_CAP + return false + } + } else { + lastIndex = r.index + consecutiveEmptySlots = 0 + if (sawEmptySlot) validEntriesAfterFirstEmptySlot += 1 + if (r.isSkippable) { + // Our decode declined the name; the strap still says the entry is real and may have more + // after it. Count it so a partial dump describes itself instead of looking complete. + skipped += 1 + _trace[_trace.size - 1] = _trace[_trace.size - 1] + + " (name did not decode — skipped, walk continues)" + } + r.key?.let { + if (!_keys.contains(it)) { + _keys.add(it) + if (sawEmptySlot) keysAfterFirstEmptySlot += 1 + } + } } - r.key?.let { if (!_keys.contains(it)) _keys.add(it) } - // Bound the walk on REPLIES, not on distinct keys: a firmware whose cursor never advances would - // repeat one name forever, and a key-count bound would never stop writing 118 to the strap. + + // 3. Bound the walk on REPLIES, not on distinct keys: a firmware whose cursor never advances would + // repeat one name forever, and a key-count bound would never stop writing the verb to the strap. if (steps >= FeatureFlagProbe.MAX_FLAGS) { stopReason = "safety cap of ${FeatureFlagProbe.MAX_FLAGS} replies reached" + stopCode = FeatureFlagProbe.StopCode.STEP_CAP return false } + // 4. The announced count, PLUS an overshoot, so the strap gets to end its own list. On the one + // 117/118 walk this project has, `steps >= count` was the only thing that stopped it — no marker, + // no validKey=0 — which means that list was never observed to end at all. val count = reportedCount - if (count != null && count > 0 && count <= FeatureFlagProbe.MAX_FLAGS && steps >= count) { - stopReason = "walked the $count entries the strap announced" - return false + if (count != null && count > 0 && count <= FeatureFlagProbe.MAX_FLAGS) { + if (steps > count) repliesPastAnnouncedCount = steps - count + if (steps >= count + FeatureFlagProbe.COUNT_OVERSHOOT_ALLOWANCE) { + stopReason = "walked the $count entries the strap announced plus " + + "${FeatureFlagProbe.COUNT_OVERSHOOT_ALLOWANCE} more — a CLIENT-side bound; the strap " + + "never sent an end marker, so the list is not known to end here" + stopCode = FeatureFlagProbe.StopCode.ANNOUNCED_COUNT_OVERSHOOT + return false + } } return true } - /** Record a reply that could not be decoded. */ - fun noteFailure(f: FeatureFlagProbe.ParseFailure, command: Int) { + /** + * Record a reply that could not be decoded. [frame], when supplied, is logged in full: a frame that + * failed its CRC is still the only evidence of what the strap actually put on the wire. + */ + fun noteFailure(f: FeatureFlagProbe.ParseFailure, command: Int, frame: ByteArray = ByteArray(0)) { val why = when (f) { FeatureFlagProbe.ParseFailure.CRC -> "CRC failed — frame rejected (never decoded)" FeatureFlagProbe.ParseFailure.ENVELOPE -> "not a COMMAND_RESPONSE envelope" FeatureFlagProbe.ParseFailure.WRONG_COMMAND -> "COMMAND_RESPONSE for a different command" FeatureFlagProbe.ParseFailure.TRUNCATED -> "record too short for the documented layout" } - _trace.add("cmd $command reply not decoded: $why") - if (stopReason == null) stopReason = why + var line = "cmd $command reply not decoded: $why" + if (frame.isNotEmpty()) line += " raw frame=${FeatureFlagProbe.hex(frame)}" + _trace.add(line) + if (stopReason == null) { + stopReason = why + stopCode = FeatureFlagProbe.StopCode.PARSE_FAILURE + } } /** Record the strap answering nothing at all within the probe's window. */ fun noteTimeout(command: Int, seconds: Int) { _trace.add("no COMMAND_RESPONSE for opcode $command within ${seconds}s") - if (stopReason == null) stopReason = "strap served no reply to opcode $command within ${seconds}s" + if (stopReason == null) { + stopReason = "strap served no reply to opcode $command within ${seconds}s" + stopCode = FeatureFlagProbe.StopCode.TIMEOUT + } } /** @@ -317,14 +667,83 @@ class FeatureFlagProbeReport(private val family: DeviceFamily) { get() { val n = reportedCount ?: 0 val plausible = n > 0 && n <= FeatureFlagProbe.MAX_FLAGS - return if (plausible) "$n flag(s)" else "an implausible $n flag(s)" + return if (plausible) "$n ${namespace.entryNoun}(s)" + else "an implausible $n ${namespace.entryNoun}(s)" + } + + /** + * What this run established about the two terminator conditions — the question the probe exists to + * answer, stated in the report rather than left for a reader to reconstruct from the trace. + */ + val terminatorFinding: String + get() { + if (sawEmptySlot && (validEntriesAfterFirstEmptySlot > 0 || sawEndMarker)) { + var s = "DECISIVE — validKey=0 is an EMPTY/RETIRED SLOT on this firmware, not the end of " + + "the list: the strap served $repliesAfterFirstEmptySlot further repl(ies) after the " + + "first validKey=0" + if (validEntriesAfterFirstEmptySlot > 0) { + s += ", $validEntriesAfterFirstEmptySlot of them flagged validKey=1" + } + if (keysAfterFirstEmptySlot > 0) s += ", naming $keysAfterFirstEmptySlot more key(s)" + if (sawEndMarker) s += ", and ended on the index=0xFF marker" + s += ". A walk that stopped on validKey=0 would have been truncated here." + return s + } + if (stopCode == FeatureFlagProbe.StopCode.EMPTY_SLOT_CURSOR_PARKED) { + return "DECISIVE — validKey=0 is a TERMINATOR on this firmware: the next request returned " + + "the same index with validKey=0 again, so the cursor is parked and there is nothing " + + "past it." + } + if (sawEmptySlot) { + return "INCONCLUSIVE — validKey=0 was served and the walk continued past it, but a " + + "client-side bound ended the run before the strap did. Nothing here separates 'end of " + + "list' from 'empty slot'." + } + if (sawEndMarker && endMarkerAlsoCarriedInvalidFlag) { + return "AMBIGUOUS — the walk ended on a reply carrying index=0xFF AND validKey=0 " + + "together, so both terminator conditions fired at once and this run cannot say which " + + "one the firmware meant." + } + if (sawEndMarker) { + return "index=0xFF ended the walk with validKey still true on the same reply, so the 0xFF " + + "marker alone terminates. validKey=0 was never served, so it is untested here." + } + return "NO TERMINATOR OBSERVED — neither validKey=0 nor index=0xFF was served in $steps " + + "repl(ies); the walk ended on a CLIENT-side bound, so the strap's list is not known to " + + "end where this report stops." + } + + /** What the announced count actually says, including the reading it does not settle. */ + val countFinding: String? + get() { + val count = reportedCount ?: return null + var s = "u16 LE read = $count" + val single = reportedCountSingleByte + val high = reportedCountHighByte + if (single != null && high != null) { + s += "; single-byte read = $single (high byte 0x%02x".format(high) + ")" + s += if (high == 0) { + " — the two readings AGREE, so this reply does not establish the field's width" + } else { + " — the two readings DISAGREE; the walk uses the u16 value and plausibility-checks it" + } + } + s += ". Keys yielded: ${_keys.size}" + if (skipped > 0) s += " (+$skipped undecodable)" + if (emptySlots > 0) s += " (+$emptySlots empty slot(s))" + if (_keys.size + skipped + emptySlots != count) s += " — MISMATCH against the announced $count" + if (repliesPastAnnouncedCount > 0) { + s += ". The strap kept answering $repliesPastAnnouncedCount repl(ies) past its own " + + "announced count." + } + return s } /** One-line summary of what the probe established. */ val verdict: String get() { if (startResult == 3) { - return "opcode 117 REJECTED by firmware (UNSUPPORTED) — this strap does not serve the enumerate verb" + return "opcode ${namespace.startCmd} REJECTED by firmware (UNSUPPORTED) — this strap does not serve the enumerate verb" } if (_keys.isEmpty() && reportedCount == null) { return "no usable reply — the enumerate path is unconfirmed on this firmware" @@ -334,40 +753,57 @@ class FeatureFlagProbeReport(private val family: DeviceFamily) { // a reader would carry into #103. Same class as [FeatureFlagProbe.NextResponse.isSkippable]: // never report our limitation as the strap's behaviour. if (_keys.isEmpty() && skipped > 0) { - return "strap named $skipped flag(s), none of which decoded as printable ASCII within " + + return "strap named $skipped ${namespace.entryNoun}(s), none of which decoded as printable ASCII within " + "${FeatureFlagProbe.MAX_KEY_LENGTH} chars — this is our parser rejecting them, NOT " + "the strap serving blanks; see the trace for the raw replies" } // Same discipline one condition over: with no 118 reply decoded, the walk never asked for a // single name, so "named none" would blame the strap for OUR timeout (or our parse failure). // [steps] counts decoded SEND_NEXT replies, so `steps == 0` is exactly "the key list was - // never read" — the reachable case being `probeFeatureFlags()` getting its 117 answer and - // then the 8s timer firing on the first 118. What the strap would have named is unknown, and - // the report has to say unknown. [stopReason], rendered directly under this line, names - // which of the two it was. + // never read" — the reachable case being the probe getting its START answer and then the 8s + // timer firing on the first SEND_NEXT. What the strap would have named is unknown, and the + // report has to say unknown. [stopReason], rendered directly under this line, names which of + // the two it was. (#913, kept through this PR's rebase: the opcode names are now taken from + // [namespace] so a 115/116 walk does not report itself as SEND_NEXT_FF(118).) if (_keys.isEmpty() && steps == 0) { - return "strap announced $announcedFlags; no SEND_NEXT_FF(118) reply was decoded — " + - "the key list was never read (inconclusive)" + return "strap announced $announcedFlags; no ${namespace.nextLabel}(${namespace.nextCmd}) " + + "reply was decoded — the key list was never read (inconclusive)" + } + if (_keys.isEmpty()) { + // Both halves survive the merge with #913: its doubt about an implausible announced count + // ([announcedFlags]), and this PR's namespace-aware noun. + return "strap announced $announcedFlags but named none" } - if (_keys.isEmpty()) return "strap announced $announcedFlags but named none" - if (skipped > 0) { - return "enumerated ${_keys.size} feature-flag key name(s); $skipped further name(s) did not decode" + var s = "enumerated ${_keys.size} ${namespace.noun} key name(s)" + if (skipped > 0) s += "; $skipped further name(s) did not decode" + // A walk that ended on OUR bound is not a complete list, and the one-line summary is the line + // that gets pasted into an issue. Say it here, not only four lines further down. + if (stopCode == FeatureFlagProbe.StopCode.STEP_CAP || + stopCode == FeatureFlagProbe.StopCode.ANNOUNCED_COUNT_OVERSHOOT || + stopCode == FeatureFlagProbe.StopCode.EMPTY_SLOT_RUN_CAP + ) { + s += " — INCOMPLETE: the walk ended on a client-side bound, not on the strap's own end marker" } - return "enumerated ${_keys.size} feature-flag key name(s)" + return s } /** The full copyable report (byte-identical to the Swift `render()`). */ fun render(): String { val fam = if (family == DeviceFamily.WHOOP5) "WHOOP 5/MG" else "WHOOP 4.0" val sb = StringBuilder() - sb.append("#761 FEATURE-FLAG ENUMERATION PROBE — $fam\n") - sb.append("Read-only: START_FF_KEY_EXCHANGE(117) + SEND_NEXT_FF(118). No value is written; ") + sb.append("#761 ${namespace.title} — $fam\n") + sb.append("Read-only: ${namespace.startLabel}(${namespace.startCmd}) + ") + sb.append("${namespace.nextLabel}(${namespace.nextCmd}). No value is written; ") sb.append("SET_FF_VALUE(120) and SET_DEVICE_CONFIG_VALUE(119) are never sent from this path.\n") sb.append("\nVerdict: $verdict\n") stopReason?.let { sb.append("Stopped: $it\n") } - sb.append("\nFlags reported by the strap (${_keys.size}") + stopCode?.let { sb.append("Stop code: ${it.code}\n") } + sb.append("Terminator: $terminatorFinding\n") + countFinding?.let { sb.append("Announced count: $it\n") } + sb.append("\n${namespace.listHeading} reported by the strap (${_keys.size}") reportedCount?.let { sb.append(" of $it announced") } if (skipped > 0) sb.append(", $skipped name(s) did not decode and were skipped") + if (emptySlots > 0) sb.append(", $emptySlots validKey=0 slot(s) stepped over") sb.append("):\n") if (_keys.isEmpty()) { sb.append(" (none)\n") diff --git a/android/app/src/test/java/com/noop/protocol/FeatureFlagProbeTest.kt b/android/app/src/test/java/com/noop/protocol/FeatureFlagProbeTest.kt index 86bede8bec..0e5c8c43ba 100644 --- a/android/app/src/test/java/com/noop/protocol/FeatureFlagProbeTest.kt +++ b/android/app/src/test/java/com/noop/protocol/FeatureFlagProbeTest.kt @@ -125,18 +125,142 @@ class FeatureFlagProbeTest { assertEquals("cursor exhausted (index 0xFF)", report.stopReason) } - @Test fun validKeyFalseStopsTheWalkEvenWithBytesAfterIt() { + /** + * `validKey = 0` alone is NOT classified as the strap's end marker any more — it is an empty slot, + * and the walk steps over it. The entry itself is still never collected as a key. + */ + @Test fun validKeyFalseIsAnEmptySlotNotTheEndMarker() { val record = byteArrayOf(0x01, 0x04, 0x00) + keyBytes("stale_buffer_leftover") val frame = whoop4Response(118, payload(1, record)) val r = FeatureFlagProbe.parseNext(frame, DeviceFamily.WHOOP4).value!! assertFalse(r.validKey) - assertTrue(r.isExhausted) + assertFalse("only index=0xFF is the strap's unambiguous end marker", r.isExhausted) + assertTrue(r.isEmptySlot) val report = FeatureFlagProbeReport(DeviceFamily.WHOOP4) - assertFalse(report.noteNext(r)) - assertEquals("firmware reported validKey=false", report.stopReason) + assertTrue("validKey=0 without 0xFF must not end the walk", report.noteNext(r)) + assertNull(report.stopReason) + assertEquals(1, report.emptySlots) assertTrue(report.keys.isEmpty()) } + /** + * The whole experiment, in one walk: the strap serves `validKey = 0` mid-list, the walk keeps asking, + * and the strap names MORE keys before ending on 0xFF. That sequence is decisive — under the old rule + * this list would have been reported as one key long. + */ + @Test fun walkContinuesPastValidKeyZeroAndCollectsTheKeysAfterIt() { + val report = FeatureFlagProbeReport(DeviceFamily.WHOOP5) + report.noteStart(FeatureFlagProbe.StartResponse(1, 1, 5, listOf(0x01, 0x05, 0x00))) + fun next(index: Int, valid: Boolean, key: String?) = FeatureFlagProbe.NextResponse( + 1, 1, index, valid, key, + listOf(0x01, index.toByte(), if (valid) 0x01 else 0x00), + ) + assertTrue(report.noteNext(next(1, true, "enable_r22_packets"))) + assertTrue("an empty slot must not end the walk", report.noteNext(next(2, false, null))) + assertTrue("nor must a second one", report.noteNext(next(3, false, null))) + assertTrue(report.noteNext(next(4, true, "enable_sig12"))) + assertFalse(report.noteNext(next(0xFF, false, null))) + + assertEquals(listOf("enable_r22_packets", "enable_sig12"), report.keys) + assertEquals(2, report.emptySlots) + assertEquals(1, report.keysAfterFirstEmptySlot) + assertEquals(FeatureFlagProbe.StopCode.END_MARKER, report.stopCode) + val finding = report.terminatorFinding + assertTrue(finding, finding.startsWith("DECISIVE — validKey=0 is an EMPTY/RETIRED SLOT")) + assertTrue(finding, finding.contains("naming 1 more key(s)")) + assertTrue(report.render().contains("2 validKey=0 slot(s) stepped over")) + } + + /** + * #874's rule reaches the CONCLUSION too. The strap flagged an entry past the hole as real; our ASCII + * filter declined its name. The list still continued, so the finding must still be decisive — reading + * "inconclusive" here would be our parser deciding a question about the firmware. + */ + @Test fun anUndecodableEntryAfterAnEmptySlotIsStillDecisive() { + val report = FeatureFlagProbeReport(DeviceFamily.WHOOP5) + report.noteStart(FeatureFlagProbe.StartResponse(1, 1, 30, listOf(0x01, 0x1E, 0x00))) + assertTrue( + report.noteNext( + FeatureFlagProbe.NextResponse(1, 1, 1, false, null, listOf(0x01, 0x01, 0x00)), + ), + ) + assertTrue( + report.noteNext( + FeatureFlagProbe.NextResponse( + 1, 1, 2, true, null, listOf(0x01, 0x02, 0x01, 0xDE.toByte(), 0xAD.toByte()), + ), + ), + ) + assertEquals("no name decoded, so no key is invented", emptyList(), report.keys) + assertEquals(0, report.keysAfterFirstEmptySlot) + assertEquals(1, report.validEntriesAfterFirstEmptySlot) + val finding = report.terminatorFinding + assertTrue(finding, finding.startsWith("DECISIVE — validKey=0 is an EMPTY/RETIRED SLOT")) + assertTrue(finding, finding.contains("1 of them flagged validKey=1")) + assertFalse(finding, finding.contains("naming")) + } + + /** + * The other decisive outcome, and the one that costs two round-trips: the strap repeats the same index + * with `validKey = 0`. A parked cursor has nothing past it, so `validKey = 0` really is a terminator on + * that firmware — and the report says so in those words rather than assuming it. + */ + @Test fun repeatedEmptySlotAtTheSameIndexIsReportedAsATerminator() { + val report = FeatureFlagProbeReport(DeviceFamily.WHOOP5) + report.noteStart(FeatureFlagProbe.StartResponse(1, 1, 9, listOf(0x01, 0x09, 0x00))) + val empty = FeatureFlagProbe.NextResponse(1, 1, 7, false, null, listOf(0x01, 0x07, 0x00)) + assertTrue("the first empty slot is stepped over", report.noteNext(empty)) + assertFalse("a parked cursor ends the walk", report.noteNext(empty)) + assertEquals(FeatureFlagProbe.StopCode.EMPTY_SLOT_CURSOR_PARKED, report.stopCode) + assertTrue( + report.terminatorFinding, + report.terminatorFinding.startsWith("DECISIVE — validKey=0 is a TERMINATOR"), + ) + assertEquals("settling this must cost two round-trips, not a full cap", 2, report.steps) + } + + /** + * A firmware that answers `validKey = 0` forever with an ADVANCING index cannot run the walk away: the + * consecutive-empty cap stops it, and names itself a CLIENT-side bound so the run is never read as a + * complete list. + */ + @Test fun anUnendingRunOfEmptySlotsStopsAtTheConsecutiveCap() { + val report = FeatureFlagProbeReport(DeviceFamily.WHOOP4) + var index = 0 + var sent = 0 + while (report.noteNext( + FeatureFlagProbe.NextResponse(1, 1, index, false, null, listOf(0x01, index.toByte(), 0x00)), + ) + ) { + index += 1 + sent += 1 + assertTrue("walk must not run away", sent < FeatureFlagProbe.MAX_CONSECUTIVE_EMPTY_SLOTS + 2) + } + assertEquals(FeatureFlagProbe.MAX_CONSECUTIVE_EMPTY_SLOTS, report.steps) + assertEquals(FeatureFlagProbe.StopCode.EMPTY_SLOT_RUN_CAP, report.stopCode) + val why = report.stopReason!! + assertTrue(why, why.contains("a CLIENT-side bound, not the strap's")) + assertTrue(report.terminatorFinding, report.terminatorFinding.startsWith("INCONCLUSIVE")) + } + + /** A valid entry resets the run, so scattered holes cost nothing against the cap. */ + @Test fun aValidEntryResetsTheConsecutiveEmptySlotRun() { + val report = FeatureFlagProbeReport(DeviceFamily.WHOOP4) + fun empty(i: Int) = FeatureFlagProbe.NextResponse(1, 1, i, false, null, listOf(0x01, i.toByte(), 0x00)) + fun valid(i: Int, k: String) = + FeatureFlagProbe.NextResponse(1, 1, i, true, k, listOf(0x01, i.toByte(), 0x01)) + + for (i in 0 until FeatureFlagProbe.MAX_CONSECUTIVE_EMPTY_SLOTS - 1) { + assertTrue(report.noteNext(empty(i))) + } + assertTrue(report.noteNext(valid(20, "hr_ch_switching"))) + for (i in 21 until 21 + FeatureFlagProbe.MAX_CONSECUTIVE_EMPTY_SLOTS - 1) { + assertTrue("the run restarted at the valid entry", report.noteNext(empty(i))) + } + assertNull(report.stopCode) + assertEquals(listOf("hr_ch_switching"), report.keys) + } + @Test fun nonPrintableNameIsNotReportedAsAKey() { val record = byteArrayOf(0x01, 0x02, 0x01, 0xDE.toByte(), 0xAD.toByte(), 0xBE.toByte(), 0xEF.toByte()) val frame = whoop4Response(118, payload(1, record)) @@ -170,28 +294,316 @@ class FeatureFlagProbeTest { } /** - * `validKey = 0` is trusted as an end marker, but the other reading — an EMPTY SLOT with the list - * continuing past it — is not ruled out by anything observed so far. Stopping well short of the - * announced count is the evidence that would settle it, so the report has to say so loudly rather - * than reporting a confident short list. + * The reply a WHOOP 5 MG actually served to end its 115/116 walk: `index = 255` and `validKey = 0` on + * the SAME frame. Both terminator conditions fired at once, so that run cannot say which one the + * firmware meant — and the report must say exactly that instead of picking one. */ - @Test fun stoppingOnValidKeyFalseShortOfTheAnnouncedCountIsFlagged() { - val report = FeatureFlagProbeReport(DeviceFamily.WHOOP4) - report.noteStart(FeatureFlagProbe.StartResponse(1, 1, 40)) - assertTrue(report.noteNext(FeatureFlagProbe.NextResponse(1, 1, 0, true, "enable_r22_packets"))) - assertFalse(report.noteNext(FeatureFlagProbe.NextResponse(1, 1, 1, false, null))) + @Test fun endMarkerCarryingValidKeyZeroIsReportedAsAmbiguous() { + val report = FeatureFlagProbeReport(DeviceFamily.WHOOP5, FeatureFlagProbe.DEVICE_CONFIG_NAMESPACE) + report.noteStart(FeatureFlagProbe.StartResponse(1, 1, 7, listOf(0x01, 0x07, 0x00))) + assertTrue( + report.noteNext( + FeatureFlagProbe.NextResponse(1, 1, 1, true, "enable_rfid", listOf(0x01, 0x01, 0x01)), + ), + ) + assertFalse( + report.noteNext( + FeatureFlagProbe.NextResponse( + 1, 1, 0xFF, false, null, listOf(0x01, 0xFF.toByte(), 0x00, 0x00), + ), + ), + ) - val why = report.stopReason!! - assertTrue(why, why.contains("2 of 40 announced entries")) - assertTrue(why, why.contains("the remainder was NOT walked")) + assertTrue(report.endMarkerAlsoCarriedInvalidFlag) + assertEquals(FeatureFlagProbe.StopCode.END_MARKER, report.stopCode) + assertTrue(report.terminatorFinding, report.terminatorFinding.startsWith("AMBIGUOUS")) + assertTrue( + report.terminatorFinding, + report.terminatorFinding.contains("both terminator conditions fired at once"), + ) + assertTrue(report.render().contains("index=0xFF AND validKey=0 on the same reply")) + } + + /** + * The 0xFF marker with `validKey` still true separates the two conditions the other way, and the + * report has to be equally careful there: 0xFF alone terminated, and validKey=0 stayed untested. + */ + @Test fun endMarkerWithValidKeyTrueSaysValidKeyZeroIsUntested() { + val report = FeatureFlagProbeReport(DeviceFamily.WHOOP5) + report.noteStart(FeatureFlagProbe.StartResponse(1, 1, 2, listOf(0x01, 0x02, 0x00))) + assertTrue( + report.noteNext( + FeatureFlagProbe.NextResponse( + 1, 1, 1, true, "enable_r22_packets", listOf(0x01, 0x01, 0x01), + ), + ), + ) + assertFalse( + report.noteNext( + FeatureFlagProbe.NextResponse(1, 1, 0xFF, true, null, listOf(0x01, 0xFF.toByte(), 0x01)), + ), + ) + assertFalse(report.endMarkerAlsoCarriedInvalidFlag) + assertTrue( + report.terminatorFinding, + report.terminatorFinding.contains("validKey=0 was never served, so it is untested"), + ) } - /** …and when the count was fully walked, the plain reason stands — no false alarm. */ - @Test fun validKeyFalseAtTheAnnouncedEndIsNotFlagged() { + /** + * What the 117/118 walk on a 5/MG actually did: sixteen valid, named entries and NO terminator of + * either kind. The walk used to stop dead at the announced count and read as a complete list. It now + * asks a bounded number of further times, and — whatever comes back — reports in the verdict itself + * that a client-side bound ended the run. + */ + @Test fun walkOvershootsTheAnnouncedCountSoTheStrapCanEndItsOwnList() { + val report = FeatureFlagProbeReport(DeviceFamily.WHOOP5) + report.noteStart(FeatureFlagProbe.StartResponse(1, 1, 3, listOf(0x01, 0x03, 0x00))) + var sent = 0 + var keepGoing = true + while (keepGoing) { + sent += 1 + keepGoing = report.noteNext( + FeatureFlagProbe.NextResponse( + 1, 1, sent, true, "key_$sent", listOf(0x01, sent.toByte(), 0x01), + ), + ) + } + assertEquals(3 + FeatureFlagProbe.COUNT_OVERSHOOT_ALLOWANCE, sent) + assertEquals(FeatureFlagProbe.StopCode.ANNOUNCED_COUNT_OVERSHOOT, report.stopCode) + assertEquals(FeatureFlagProbe.COUNT_OVERSHOOT_ALLOWANCE, report.repliesPastAnnouncedCount) + assertTrue( + report.verdict, + report.verdict.contains("INCOMPLETE: the walk ended on a client-side bound"), + ) + assertTrue(report.terminatorFinding, report.terminatorFinding.startsWith("NO TERMINATOR OBSERVED")) + val count = report.countFinding!! + assertTrue(count, count.contains("Keys yielded: 7")) + assertTrue(count, count.contains("MISMATCH against the announced 3")) + assertTrue(count, count.contains("kept answering 4 repl(ies) past its own announced count")) + } + + // MARK: raw bytes + + /** + * Requirement of the whole exercise: the RAW record bytes of every reply are in the report, not only + * the fields parsed out of them. Earlier findings here had to be re-run because only parsed output + * survived, and a parsed field cannot contradict the layout that produced it. + */ + @Test fun everyReplyLogsItsRawRecordBytes() { + val report = FeatureFlagProbeReport(DeviceFamily.WHOOP5) + val start = whoop5Response(117, payload(1, byteArrayOf(0x01, 0x02, 0x00))) + val s = FeatureFlagProbe.parseStart(start, DeviceFamily.WHOOP5).value!! + report.noteStart(s) + assertEquals(listOf(0x01, 0x02, 0x00), s.record) + val next = whoop5Response(118, payload(1, byteArrayOf(0x01, 0x00, 0x01, 0x78, 0x00))) + val n = FeatureFlagProbe.parseNext(next, DeviceFamily.WHOOP5).value!! + assertTrue(report.noteNext(n)) + val text = report.render() + assertTrue(text, text.contains("raw=01 02 00")) + assertTrue(text, text.contains("raw=01 00 01 78 00")) + } + + /** A reply that failed to decode is the one whose raw bytes matter most, so the whole frame goes in. */ + @Test fun undecodedReplyLogsTheWholeRawFrame() { val report = FeatureFlagProbeReport(DeviceFamily.WHOOP4) - report.noteStart(FeatureFlagProbe.StartResponse(1, 1, 1)) - assertFalse(report.noteNext(FeatureFlagProbe.NextResponse(1, 1, 0, false, null))) - assertEquals("firmware reported validKey=false", report.stopReason) + report.noteFailure( + FeatureFlagProbe.ParseFailure.CRC, 118, + byteArrayOf(0xAA.toByte(), 0x05, 0x00, 0x2B, 0x24), + ) + assertTrue(report.render(), report.render().contains("raw frame=aa 05 00 2b 24")) + assertEquals(FeatureFlagProbe.StopCode.PARSE_FAILURE, report.stopCode) + } + + /** + * The hex is bounded so one absurd record cannot flood the log — but the true LENGTH is always + * printed, because an over-long record is itself evidence that the layout is wrong. + */ + @Test fun overLongRawRecordIsElidedButItsLengthIsKept() { + val long = List(FeatureFlagProbe.MAX_RAW_HEX_BYTES + 10) { 0xAB.toByte() } + val rendered = FeatureFlagProbe.hex(long) + assertTrue( + rendered, + rendered.endsWith("… (${FeatureFlagProbe.MAX_RAW_HEX_BYTES + 10} bytes total)"), + ) + assertEquals("(empty)", FeatureFlagProbe.hex(emptyList())) + } + + // MARK: the announced count + + /** + * The count is read as u16 LE, and every count seen so far has had a zero high byte — where a + * single-byte read returns the SAME number. So no capture distinguishes the two readings, and the + * report says that rather than asserting a field width nothing has established. + */ + @Test fun countCarriesBothReadingsAndSaysWhenTheyAgree() { + val frame = whoop5Response(117, payload(1, byteArrayOf(0x01, 0x10, 0x00))) + val r = FeatureFlagProbe.parseStart(frame, DeviceFamily.WHOOP5).value!! + assertEquals(16, r.count) + assertEquals(16, r.singleByteCount) + assertEquals(0, r.countHighByte) + assertTrue(r.countReadingsAgree) + val report = FeatureFlagProbeReport(DeviceFamily.WHOOP5) + report.noteStart(r) + val finding = report.countFinding!! + assertTrue(finding, finding.contains("u16 LE read = 16; single-byte read = 16 (high byte 0x00)")) + assertTrue(finding, finding.contains("the two readings AGREE")) + } + + /** + * A nonzero high byte is the only thing that separates them, and it is a finding either way: on the + * u16 reading the list is enormous, on the single-byte reading `record[2]` is padding. + */ + @Test fun nonZeroHighByteIsReportedAsADisagreement() { + val frame = whoop5Response(117, payload(1, byteArrayOf(0x01, 0x07, 0x02))) + val r = FeatureFlagProbe.parseStart(frame, DeviceFamily.WHOOP5).value!! + assertEquals(0x0207, r.count) + assertEquals(7, r.singleByteCount) + assertFalse(r.countReadingsAgree) + assertFalse("an implausible u16 must not become a loop bound", r.countIsPlausible) + val report = FeatureFlagProbeReport(DeviceFamily.WHOOP5) + report.noteStart(r) + val finding = report.countFinding!! + assertTrue(finding, finding.contains("single-byte read = 7 (high byte 0x02)")) + assertTrue(finding, finding.contains("the two readings DISAGREE")) + } + + /** + * Announced-count-versus-yielded is itself a result. A strap that announces sixteen and names one has + * fifteen entries nobody has accounted for, and the report has to raise it rather than print a tidy + * short list. + */ + @Test fun announcedCountVersusKeysYieldedMismatchIsReported() { + val report = FeatureFlagProbeReport(DeviceFamily.WHOOP5) + report.noteStart(FeatureFlagProbe.StartResponse(1, 1, 16, listOf(0x01, 0x10, 0x00))) + assertTrue( + report.noteNext( + FeatureFlagProbe.NextResponse( + 1, 1, 1, true, "enable_r22_packets", listOf(0x01, 0x01, 0x01), + ), + ), + ) + assertFalse( + report.noteNext( + FeatureFlagProbe.NextResponse(1, 1, 0xFF, true, null, listOf(0x01, 0xFF.toByte(), 0x01)), + ), + ) + val finding = report.countFinding!! + assertTrue(finding, finding.contains("Keys yielded: 1")) + assertTrue(finding, finding.contains("MISMATCH against the announced 16")) + } + + /** …and when everything is accounted for — keys plus skipped plus empty slots — there is no alarm. */ + @Test fun fullyAccountedCountIsNotFlaggedAsAMismatch() { + val report = FeatureFlagProbeReport(DeviceFamily.WHOOP5) + report.noteStart(FeatureFlagProbe.StartResponse(1, 1, 3, listOf(0x01, 0x03, 0x00))) + report.noteNext(FeatureFlagProbe.NextResponse(1, 1, 1, true, "a", listOf(0x01, 0x01, 0x01))) + report.noteNext(FeatureFlagProbe.NextResponse(1, 1, 2, true, null, listOf(0x01, 0x02, 0x01))) + report.noteNext(FeatureFlagProbe.NextResponse(1, 1, 3, false, null, listOf(0x01, 0x03, 0x00))) + val finding = report.countFinding!! + assertFalse(finding, finding.contains("MISMATCH")) + assertTrue(finding, finding.contains("Keys yielded: 1 (+1 undecodable) (+1 empty slot(s))")) + } + + // MARK: the device-config namespace (115/116) + + /** + * The SAME walk drives the device-config pair, so the terminator rules cannot be corrected in one + * namespace and left wrong in the other. Decoded end-to-end from 115/116 frames. + */ + @Test fun deviceConfigNamespaceWalksThroughTheSameCorrectedRules() { + val report = FeatureFlagProbeReport(DeviceFamily.WHOOP5, FeatureFlagProbe.DEVICE_CONFIG_NAMESPACE) + val start = whoop5Response(115, payload(1, byteArrayOf(0x01, 0x02, 0x00))) + report.noteStart(FeatureFlagProbe.parseStart(start, DeviceFamily.WHOOP5, 115).value!!) + // An empty slot first — under the old rule the walk would have ended here with zero keys. + val hole = whoop5Response(116, payload(1, byteArrayOf(0x01, 0x02, 0x00, 0x00))) + assertTrue(report.noteNext(FeatureFlagProbe.parseNext(hole, DeviceFamily.WHOOP5, 116).value!!)) + val named = whoop5Response( + 116, payload(1, byteArrayOf(0x01, 0x03, 0x01) + keyBytes("enable_raw_data_w_ecg")), + ) + assertTrue(report.noteNext(FeatureFlagProbe.parseNext(named, DeviceFamily.WHOOP5, 116).value!!)) + assertEquals( + "the key AFTER the empty slot is exactly what the old rule discarded", + listOf("enable_raw_data_w_ecg"), report.keys, + ) + + val text = report.render() + assertTrue(text, text.contains("#761 DEVICE-CONFIG ENUMERATION PROBE — WHOOP 5/MG")) + assertTrue(text, text.contains("START_DEVICE_CONFIG_KEY_EXCHANGE(115) + SEND_NEXT_DEVICE_CONFIG(116)")) + assertTrue(text, text.contains("Keys reported by the strap (1 of 2 announced")) + assertTrue(text, text.contains("SEND_NEXT_DEVICE_CONFIG(116) → index=3")) + } + + /** The 115/116 frames must not decode as 117/118 by accident — the opcode is checked, both ways. */ + @Test fun theTwoNamespacesDoNotDecodeEachOthersFrames() { + val cfg = whoop5Response(116, payload(1, byteArrayOf(0x01, 0x00, 0x01))) + assertEquals( + FeatureFlagProbe.ParseFailure.WRONG_COMMAND, + FeatureFlagProbe.parseNext(cfg, DeviceFamily.WHOOP5).failure, + ) + val ff = whoop5Response(118, payload(1, byteArrayOf(0x01, 0x00, 0x01))) + assertEquals( + FeatureFlagProbe.ParseFailure.WRONG_COMMAND, + FeatureFlagProbe.parseNext(ff, DeviceFamily.WHOOP5, 116).failure, + ) + assertEquals(115, FeatureFlagProbe.DEVICE_CONFIG_NAMESPACE.startCmd) + assertEquals(116, FeatureFlagProbe.DEVICE_CONFIG_NAMESPACE.nextCmd) + // Still read-only: the SET verbs appear in neither namespace. + for (ns in listOf(FeatureFlagProbe.FEATURE_FLAG_NAMESPACE, FeatureFlagProbe.DEVICE_CONFIG_NAMESPACE)) { + assertFalse(listOf(119, 120).contains(ns.startCmd)) + assertFalse(listOf(119, 120).contains(ns.nextCmd)) + } + } + + /** + * An explicit refusal is the strap's answer, not one of our bounds, and it must end the walk with its + * own name — on the START reply (where the driver reads [FeatureFlagProbeReport.hasStopped] instead of + * stepping to 118) and on a NEXT reply alike. + */ + @Test fun unsupportedRefusalEndsTheWalkWithItsOwnStopCode() { + val start = FeatureFlagProbeReport(DeviceFamily.WHOOP5) + assertFalse(start.hasStopped) + start.noteStart(FeatureFlagProbe.StartResponse(3, 0, 0, listOf(0x00, 0x00, 0x00))) + assertTrue("a refused START must not step on to the next verb", start.hasStopped) + assertEquals(FeatureFlagProbe.StopCode.UNSUPPORTED, start.stopCode) + assertTrue(start.render().contains("Stop code: unsupported")) + + val mid = FeatureFlagProbeReport(DeviceFamily.WHOOP5) + mid.noteStart(FeatureFlagProbe.StartResponse(1, 1, 4, listOf(0x01, 0x04, 0x00))) + assertTrue( + mid.noteNext( + FeatureFlagProbe.NextResponse( + 1, 1, 1, true, "enable_r22_packets", listOf(0x01, 0x01, 0x01), + ), + ), + ) + assertFalse( + mid.noteNext( + FeatureFlagProbe.NextResponse(3, 0, 0, false, null, listOf(0x00, 0x00, 0x00)), + ), + ) + assertEquals(FeatureFlagProbe.StopCode.UNSUPPORTED, mid.stopCode) + assertEquals("a refusal is never counted as an empty slot", 0, mid.emptySlots) + assertTrue(mid.stopReason!!, mid.stopReason!!.contains("UNSUPPORTED(3)")) + } + + /** + * The two platforms must classify the same reply the same way, or a strap log means different things + * on either side. These are the exact constants the walk is bounded by. + */ + @Test fun walkBoundsMatchTheSwiftTwin() { + assertEquals(8, FeatureFlagProbe.MAX_CONSECUTIVE_EMPTY_SLOTS) + assertEquals(4, FeatureFlagProbe.COUNT_OVERSHOOT_ALLOWANCE) + assertEquals(64, FeatureFlagProbe.MAX_RAW_HEX_BYTES) + assertEquals(128, FeatureFlagProbe.MAX_FLAGS) + assertEquals("endMarker", FeatureFlagProbe.StopCode.END_MARKER.code) + assertEquals("emptySlotCursorParked", FeatureFlagProbe.StopCode.EMPTY_SLOT_CURSOR_PARKED.code) + assertEquals("emptySlotRunCap", FeatureFlagProbe.StopCode.EMPTY_SLOT_RUN_CAP.code) + assertEquals("stepCap", FeatureFlagProbe.StopCode.STEP_CAP.code) + assertEquals("announcedCountOvershoot", FeatureFlagProbe.StopCode.ANNOUNCED_COUNT_OVERSHOOT.code) + assertEquals("timeout", FeatureFlagProbe.StopCode.TIMEOUT.code) + assertEquals("unsupported", FeatureFlagProbe.StopCode.UNSUPPORTED.code) + assertEquals("parseFailure", FeatureFlagProbe.StopCode.PARSE_FAILURE.code) } /** @@ -299,7 +711,7 @@ class FeatureFlagProbeTest { assertEquals(FeatureFlagProbe.ParseFailure.CRC, FeatureFlagProbe.parseStart(ByteArray(0), DeviceFamily.WHOOP4).failure) } - @Test fun fullWalkRendersEveryKeyAndStopsOnTheAnnouncedCount() { + @Test fun fullWalkRendersEveryKeyAndStopsOnTheEndMarker() { val report = FeatureFlagProbeReport(DeviceFamily.WHOOP5) val start = whoop5Response(117, payload(1, byteArrayOf(0x01, 0x03, 0x00))) report.noteStart(FeatureFlagProbe.parseStart(start, DeviceFamily.WHOOP5).value!!) @@ -308,9 +720,13 @@ class FeatureFlagProbeTest { val record = byteArrayOf(0x01, i.toByte(), 0x01) + keyBytes(name) val frame = whoop5Response(118, payload(1, record)) val n = FeatureFlagProbe.parseNext(frame, DeviceFamily.WHOOP5).value!! - assertEquals(i < names.size - 1, report.noteNext(n)) + assertTrue("the announced count no longer ends the walk by itself", report.noteNext(n)) } + // The strap's own end marker — the only thing that ends a walk cleanly. + val end = whoop5Response(118, payload(1, byteArrayOf(0x01, 0xFF.toByte(), 0x00, 0x00))) + assertFalse(report.noteNext(FeatureFlagProbe.parseNext(end, DeviceFamily.WHOOP5).value!!)) assertEquals(names, report.keys) + assertEquals(FeatureFlagProbe.StopCode.END_MARKER, report.stopCode) val text = report.render() assertTrue(text.contains("#761 FEATURE-FLAG ENUMERATION PROBE — WHOOP 5/MG")) assertTrue(text.contains("Read-only")) @@ -318,7 +734,7 @@ class FeatureFlagProbeTest { assertTrue(text.contains("Flags reported by the strap (3 of 3 announced)")) assertTrue(text.contains(" 1. enable_r22_packets")) assertTrue(text.contains(" 3. wear_detect_bias")) - assertTrue(text.contains("walked the 3 entries the strap announced")) + assertTrue(text.contains("Stop code: endMarker")) assertTrue(text.contains("START_FF_KEY_EXCHANGE(117) → revision=1 count=3 result=SUCCESS(1)")) assertTrue(text.contains("SEND_NEXT_FF(118) → index=0 validKey=true key=\"enable_r22_packets\"")) } @@ -372,14 +788,17 @@ class FeatureFlagProbeTest { Verdict: enumerated 1 feature-flag key name(s) Stopped: cursor exhausted (index 0xFF) + Stop code: endMarker + Terminator: AMBIGUOUS — the walk ended on a reply carrying index=0xFF AND validKey=0 together, so both terminator conditions fired at once and this run cannot say which one the firmware meant. + Announced count: u16 LE read = 2; single-byte read = 2 (high byte 0x00) — the two readings AGREE, so this reply does not establish the field's width. Keys yielded: 1 — MISMATCH against the announced 2 Flags reported by the strap (1 of 2 announced): 1. enable_r22_packets Exchange: - START_FF_KEY_EXCHANGE(117) → revision=1 count=2 result=SUCCESS(1) - SEND_NEXT_FF(118) → index=0 validKey=true key="enable_r22_packets" result=SUCCESS(1) - SEND_NEXT_FF(118) → index=255 validKey=false result=SUCCESS(1) + START_FF_KEY_EXCHANGE(117) → revision=1 count=2 result=SUCCESS(1) raw=01 02 00 + SEND_NEXT_FF(118) → index=0 validKey=true key="enable_r22_packets" result=SUCCESS(1) raw=01 00 01 65 6e 61 62 6c 65 5f 72 32 32 5f 70 61 63 6b 65 74 73 00 00 00 00 00 00 00 00 00 00 + SEND_NEXT_FF(118) → index=255 validKey=false result=SUCCESS(1) raw=01 ff 00 00 00 00 00 (index=0xFF AND validKey=0 on the same reply — both terminator conditions at once) """.trimIndent() assertEquals(golden, report.render()) diff --git a/docs/PROTOCOL.md b/docs/PROTOCOL.md index 85be882044..e6a0d4936c 100644 --- a/docs/PROTOCOL.md +++ b/docs/PROTOCOL.md @@ -547,10 +547,21 @@ the same `pay[2]` record start `GET_BATTERY_LEVEL` and `GET_CLOCK` already decod | 117 `START_FF_KEY_EXCHANGE` | `revision u8` · `numberOfFeatureFlags u16 LE` · padding | | 118 `SEND_NEXT_FF` | `revision u8` · `index u8` · `validKey u8` · `key` (ASCII, NUL-terminated) · padding | -The walk stops on the strap's own end marker (`validKey = 0`, or `index = 0xFF`), on the announced count, -or on a hard cap of 128 replies — and each 118 is only sent after the previous reply lands. Both CRCs are -verified before any field is read; a failed CRC, a non-COMMAND_RESPONSE type, or a short record ends the -walk with a named reason instead of a decode. Driven by `BLEManager.probeFeatureFlags()` / +**The two terminator conditions are not interchangeable, and are separated deliberately.** The walk stops +on `index = 0xFF` — the one end marker a strap has served here unambiguously. `validKey = 0` on its own +does NOT stop it: that could equally mark an EMPTY or RETIRED SLOT with the list continuing past it, and +the record layout above is derived from a WHOOP 4.0 and **unverified on 5/MG**. Neither reading is +established, because on the walks this project has, the two have never been separated on the wire: the +117/118 walk on a WS50_r03 served sixteen replies that were all `validKey = 1` with no `0xFF` at all, +and its 115/116 walk ended on a single reply carrying `index = 255` **and** `validKey = 0` together. So a +`validKey = 0` entry is recorded, stepped over, and the next record verb is sent again — what comes back +separates the two readings, and the report states which it observed. Past that the bounds are all +CLIENT-side and each names itself in the report's `Stop code:` line: 8 consecutive `validKey = 0` replies, +a repeated index during such a run (a parked cursor — evidence for the terminator reading), the announced +count plus 4, or a hard cap of 128 replies. Each next-record request is only sent after the previous reply +lands. Both CRCs are verified before any field is read; a failed CRC, a non-COMMAND_RESPONSE type, or a +short record ends the walk with a named reason instead of a decode, and the RAW record bytes of every +reply are logged beside the fields decoded from them. Driven by `BLEManager.probeFeatureFlags()` / `WhoopBleClient.probeFeatureFlags()` (user-triggered, Test Centre → Connection, both families) and allowlisted for 5/MG framing **only while a probe is in flight**; parsed + rendered by the pure `FeatureFlagProbe` / `FeatureFlagProbeReport` twins (Swift↔Kotlin byte-parity, unit-tested on synthetic From f7f373407bede7d05a815880a9c789efe50b262c Mon Sep 17 00:00:00 2001 From: vishk23 <119831996+vishk23@users.noreply.github.com> Date: Wed, 29 Jul 2026 00:25:54 -0400 Subject: [PATCH 2/2] Narrow the parked-cursor verdict to what the walk observes (#761, #103) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The DECISIVE terminator string claimed "the cursor is parked and there is nothing past it". The run observes only that the cursor does not advance past that point via this verb. Two firmwares emit the identical frame: - end of list — the cursor parks at a sentinel, nothing follows; - a cursor that advances only on a valid record — an empty or retired slot parks it, validKey=0 is a SLOT, the walk is stuck, and keys may sit behind the hole. The second is exactly the reading this PR exists to make testable, so the probe was printing, as DECISIVE, the conclusion it was written to question — and that is the line a reader pastes into an issue as settled. The DECISIVE label stays, because something IS decided; only the claim narrows. The same over-claim was restated in three other places and all four are fixed: the StopCode doc comment on both platforms, and both test doc comments. New assertions pin the narrowing in both directions rather than trusting the wording to stay: the finding must contain "the cursor does not advance past it" and the non-separability sentence, and must NOT contain "there is nothing past it". Widening it back now fails the suite. The EMPTY/RETIRED SLOT branch is untouched — more names arriving after a validKey=0 is direct evidence, and "a walk that stopped here would have been truncated" is what was observed. --- .../WhoopProtocol/FeatureFlagProbe.swift | 16 ++++++++++--- .../FeatureFlagProbeTests.swift | 17 ++++++++++++-- .../com/noop/protocol/FeatureFlagProbe.kt | 16 +++++++++---- .../com/noop/protocol/FeatureFlagProbeTest.kt | 23 +++++++++++++++++-- 4 files changed, 61 insertions(+), 11 deletions(-) diff --git a/Packages/WhoopProtocol/Sources/WhoopProtocol/FeatureFlagProbe.swift b/Packages/WhoopProtocol/Sources/WhoopProtocol/FeatureFlagProbe.swift index 2f63770f5c..5442ac93b5 100644 --- a/Packages/WhoopProtocol/Sources/WhoopProtocol/FeatureFlagProbe.swift +++ b/Packages/WhoopProtocol/Sources/WhoopProtocol/FeatureFlagProbe.swift @@ -163,8 +163,10 @@ public enum FeatureFlagProbe { public enum StopCode: String, Equatable, Sendable { /// The strap served `index = 0xFF`. The one terminator this project has ever seen unambiguously. case endMarker - /// A `validKey = 0` reply repeated the previous reply's index: the cursor is parked, so on this - /// firmware `validKey = 0` behaves as a terminator rather than as an empty slot. + /// A `validKey = 0` reply repeated the previous reply's index: the cursor did not advance, so + /// this walk cannot reach anything beyond that point. Note what this does NOT establish — a + /// firmware whose cursor parks on an empty slot emits the identical frame, so "the list ends + /// here" and "the walk is stuck on a hole" are not separable by this observation. case emptySlotCursorParked /// `maxConsecutiveEmptySlots` consecutive `validKey = 0` replies, with the index still advancing. case emptySlotRunCap @@ -655,8 +657,16 @@ public struct FeatureFlagProbeReport: Equatable, Sendable { return s } if stopCode == .emptySlotCursorParked { + // The DECISIVE label is kept, narrowed to what the run actually decides. "There is nothing + // past it" is a step beyond the observation, and TWO firmwares emit this identical frame: + // a list that genuinely ends at a parked sentinel, and a cursor that advances only on a valid + // record — where validKey=0 is a SLOT, the walk is stuck on a hole, and keys may sit behind + // it. The second is precisely the reading this probe exists to make testable, so printing the + // first as decided would hand a reader the conclusion the probe was written to question. return "DECISIVE — validKey=0 is a TERMINATOR on this firmware: the next request returned the " - + "same index with validKey=0 again, so the cursor is parked and there is nothing past it." + + "same index with validKey=0 again, so the cursor does not advance past it, so this walk " + + "cannot see anything beyond. Whether the list truly ends here is not separable from a " + + "firmware whose cursor parks on an empty slot." } if sawEmptySlot { return "INCONCLUSIVE — validKey=0 was served and the walk continued past it, but a client-side " diff --git a/Packages/WhoopProtocol/Tests/WhoopProtocolTests/FeatureFlagProbeTests.swift b/Packages/WhoopProtocol/Tests/WhoopProtocolTests/FeatureFlagProbeTests.swift index 4d57278253..4b21687df5 100644 --- a/Packages/WhoopProtocol/Tests/WhoopProtocolTests/FeatureFlagProbeTests.swift +++ b/Packages/WhoopProtocol/Tests/WhoopProtocolTests/FeatureFlagProbeTests.swift @@ -213,8 +213,11 @@ final class FeatureFlagProbeTests: XCTestCase { } /// The other decisive outcome, and the one that costs two round-trips: the strap repeats the same - /// index with `validKey = 0`. A parked cursor has nothing past it, so `validKey = 0` really is a - /// terminator on that firmware — and the report says so in those words rather than assuming it. + /// index with `validKey = 0`. What that decides is that the cursor does not advance, so this walk + /// cannot see past the point — NOT that the list ends there. A firmware whose cursor parks on an + /// empty slot emits the identical frame, and that is the reading this whole probe exists to make + /// testable, so the verdict must not print it as settled. Both halves are pinned below: the DECISIVE + /// label stays, and the over-claim must not come back. func testRepeatedEmptySlotAtTheSameIndexIsReportedAsATerminator() { var report = FeatureFlagProbeReport(family: .whoop5) report.noteStart(FeatureFlagProbe.StartResponse(resultCode: 1, revision: 1, count: 9, @@ -226,6 +229,16 @@ final class FeatureFlagProbeTests: XCTestCase { XCTAssertEqual(report.stopCode, .emptySlotCursorParked) XCTAssertTrue(report.terminatorFinding.hasPrefix("DECISIVE — validKey=0 is a TERMINATOR"), report.terminatorFinding) + // The narrowing, pinned so it cannot be quietly widened back. "There is nothing past it" is the + // conclusion a reader would paste into an issue as settled, and it is the one this probe exists + // to question — two firmwares emit this identical frame. + XCTAssertTrue(report.terminatorFinding.contains("the cursor does not advance past it"), + report.terminatorFinding) + XCTAssertTrue(report.terminatorFinding.contains( + "not separable from a firmware whose cursor parks on an empty slot"), + report.terminatorFinding) + XCTAssertFalse(report.terminatorFinding.contains("there is nothing past it"), + "the walk observes a stalled cursor, not an empty tail: \(report.terminatorFinding)") XCTAssertEqual(report.steps, 2, "settling this must cost two round-trips, not a full cap") } diff --git a/android/app/src/main/java/com/noop/protocol/FeatureFlagProbe.kt b/android/app/src/main/java/com/noop/protocol/FeatureFlagProbe.kt index 3afa05f7f9..21f138ddab 100644 --- a/android/app/src/main/java/com/noop/protocol/FeatureFlagProbe.kt +++ b/android/app/src/main/java/com/noop/protocol/FeatureFlagProbe.kt @@ -144,8 +144,10 @@ object FeatureFlagProbe { END_MARKER("endMarker"), /** - * A `validKey = 0` reply repeated the previous reply's index: the cursor is parked, so on this - * firmware `validKey = 0` behaves as a terminator rather than as an empty slot. + * A `validKey = 0` reply repeated the previous reply's index: the cursor did not advance, so + * this walk cannot reach anything beyond that point. Note what this does NOT establish — a + * firmware whose cursor parks on an empty slot emits the identical frame, so "the list ends + * here" and "the walk is stuck on a hole" are not separable by this observation. */ EMPTY_SLOT_CURSOR_PARKED("emptySlotCursorParked"), @@ -690,9 +692,15 @@ class FeatureFlagProbeReport( return s } if (stopCode == FeatureFlagProbe.StopCode.EMPTY_SLOT_CURSOR_PARKED) { + // The DECISIVE label is kept, narrowed to what the run actually decides. "There is nothing + // past it" is a step beyond the observation, and TWO firmwares emit this identical frame: + // a list that genuinely ends at a parked sentinel, and a cursor that advances only on a + // valid record — where validKey=0 is a SLOT, the walk is stuck on a hole, and keys may sit + // behind it. The second is precisely the reading this probe exists to make testable. return "DECISIVE — validKey=0 is a TERMINATOR on this firmware: the next request returned " + - "the same index with validKey=0 again, so the cursor is parked and there is nothing " + - "past it." + "the same index with validKey=0 again, so the cursor does not advance past it, so " + + "this walk cannot see anything beyond. Whether the list truly ends here is not " + + "separable from a firmware whose cursor parks on an empty slot." } if (sawEmptySlot) { return "INCONCLUSIVE — validKey=0 was served and the walk continued past it, but a " + diff --git a/android/app/src/test/java/com/noop/protocol/FeatureFlagProbeTest.kt b/android/app/src/test/java/com/noop/protocol/FeatureFlagProbeTest.kt index 0e5c8c43ba..901cc764e4 100644 --- a/android/app/src/test/java/com/noop/protocol/FeatureFlagProbeTest.kt +++ b/android/app/src/test/java/com/noop/protocol/FeatureFlagProbeTest.kt @@ -202,8 +202,10 @@ class FeatureFlagProbeTest { /** * The other decisive outcome, and the one that costs two round-trips: the strap repeats the same index - * with `validKey = 0`. A parked cursor has nothing past it, so `validKey = 0` really is a terminator on - * that firmware — and the report says so in those words rather than assuming it. + * with `validKey = 0`. What that decides is that the cursor does not advance, so this walk cannot see + * past the point — NOT that the list ends there. A firmware whose cursor parks on an empty slot emits + * the identical frame, and that is the reading this whole probe exists to make testable, so the + * verdict must not print it as settled. Both halves are pinned below. */ @Test fun repeatedEmptySlotAtTheSameIndexIsReportedAsATerminator() { val report = FeatureFlagProbeReport(DeviceFamily.WHOOP5) @@ -216,6 +218,23 @@ class FeatureFlagProbeTest { report.terminatorFinding, report.terminatorFinding.startsWith("DECISIVE — validKey=0 is a TERMINATOR"), ) + // The narrowing, pinned so it cannot be quietly widened back. "There is nothing past it" is the + // conclusion a reader would paste into an issue as settled, and it is the one this probe exists + // to question — two firmwares emit this identical frame. + assertTrue( + report.terminatorFinding, + report.terminatorFinding.contains("the cursor does not advance past it"), + ) + assertTrue( + report.terminatorFinding, + report.terminatorFinding.contains( + "not separable from a firmware whose cursor parks on an empty slot", + ), + ) + assertFalse( + "the walk observes a stalled cursor, not an empty tail: ${report.terminatorFinding}", + report.terminatorFinding.contains("there is nothing past it"), + ) assertEquals("settling this must cost two round-trips, not a full cap", 2, report.steps) }