Skip to content

fix(ble): stop labelling a WHOOP 5/MG as WHOOP 4.0 - #2750

Open
christophersarmiento wants to merge 2 commits into
ryanbr:mainfrom
christophersarmiento:fix/whoop-model-stamp-and-dis-model-number
Open

christophersarmiento wants to merge 2 commits into
ryanbr:mainfrom
christophersarmiento:fix/whoop-model-stamp-and-dis-model-number

Conversation

@christophersarmiento

Copy link
Copy Markdown

What this PR does

A WHOOP 5.0 / MG install showed WHOOP 4.0 as its model on the Devices card. As a result, DeviceFamily.forRegistryModel treated the strap as a 4.0 everywhere: the label, the skin-temp raw→°C scale, and copy that applies only to a 4.0 ("no steps over BLE on a 4.0"). Two bugs combined to cause this.

1. #716 stamp fired on any scan result (Apple + Android)

  • The seeded my-whoop row's placeholder model "WHOOP" was replaced on the first discovery callback of any kind, before whoopGattScanDecision ran, using whichever family the scan was targeting.
  • The auto-scan tries 4.0 first. An unsupported advertiser heard in that window (here service 1150) caused the row to be stamped "WHOOP 4.0" before the 5/MG leg ran.
  • Field log:
    [09:57:10] Scanning for WHOOP 4.0…
    [09:57:10] Updated device model from "WHOOP" to "WHOOP 4.0" (#716)
    [09:57:10] Discovered <name> (rssi -41) — WHOOP PUFFIN service 1150 detected but unsupported; NOOP will not connect or send commands.
    [09:57:18] No WHOOP 4.0 found yet — trying WHOOP 5.0 / MG
    
  • The stamp now runs only when the scan has accepted a strap for the selected family and is about to connect: after the scan decision, the present-scan return and the preferred-peripheral filter. Both platforms get the same change.

2. DIS model number was not used to resolve the variant on Apple

  • A real MG reports serial prefix MGB and hardware revision WS50_r03, which match neither Whoop5Variant heuristic. Its DIS 0x2A24 Model Number reads MG.
  • Whoop5Variant.from already accepts modelNumber: (Found a repository that might help with Whoop 5 sensor decoding #520), and Android already passes it. BLEManager only logged 2A24, so the variant stayed .unknown and reconcileModelFromAttestation never upgraded a mislabelled row.
  • BLEManager now stores 2A24 as disModelNumber, clears it on teardown, and passes it to both Whoop5Variant.from call sites, matching WhoopBleClient.
  • The attestation correction now relabels an existing 4.0 row on the next connect. A positively identified MG also now enables the existing MG-only gates (isMG), as Android already does.

Type of change

  • Bug fix
  • New feature
  • Refactor / cleanup
  • Documentation
  • CI / tooling

How it was tested

  • Hardware (iOS, WHOOP MG, FW 50.37.1.0):
    • Before the change, a clean install stamped WHOOP 4.0 and logged variant=—.
    • With this branch built and installed on an iPhone (NOOPiOS, iOS 27), the Devices card reads WHOOP 5.0 / MG.
  • macOS Strand scheme: xcodebuild … build succeeded. NOOPiOS: built and installed on device.
  • Tools/doc_comment_lint.py: OK.
  • Android: not compiled locally (no JDK 17 on this machine). That change is a relocation of the existing stamp block into a helper called from the connect point, so it relies on android.yml. Not exercised on an Android device.

Checklist

  • Swift package tests pass for any package I touched (no package changes)
  • Android unit tests pass if I touched android/ (relying on CI, see above)
  • No new build warnings introduced
  • UI changes use only StrandDesign tokens (no UI changes)
  • No hardcoded hex frame bytes; protocol facts live in the schema / decoders
  • Follows the conventions in docs/CONTRIBUTING.md
  • I did not commit generated output (Strand.xcodeproj/) or any secrets/keystores

Related issues

Refs #716
Refs #520

…cted

The ryanbr#716 stamp replaced the seeded "WHOOP" model on the first scan result
of any kind, using whichever family the scan happened to target. The
auto-scan tries WHOOP 4.0 first, so an unsupported advertiser heard in
that window (e.g. service 1150) labelled a 5/MG install "WHOOP 4.0"
before the 5/MG leg ran, and forRegistryModel then treated the strap as
a 4.0 (Devices label, skin-temp scale, 4.0-only copy).

Move the stamp to the point where the scan has accepted a strap for the
selected family and is about to connect, on both platforms.

Refs ryanbr#716
… Apple

Store DIS 0x2A24 and pass it to Whoop5Variant.from, as Android already
does. A real MG reports serial prefix "MGB" and hardware revision
"WS50_r03", which match neither heuristic, but its model number reads
"MG". Without it the variant stayed unknown, so the attestation
correction never relabelled a 5/MG that was registered as a 4.0.

Refs ryanbr#520
@ryanbr

ryanbr commented Oct 13, 2026

Copy link
Copy Markdown
Owner

Approved the workflow runs, so Android CI will now actually compile the Kotlin half for you.

Traced both halves against the tree and the design holds up. Two things I checked that I want to record here because they are easy to doubt: the 2A24 read cannot leak onto a 4.0 (the DIS read is gated selectedModel.deviceFamily != .whoop4 at BLEManager.swift:5209 and again in shouldReadDisUnbonded, so readDisExtras never runs there), and the repair path for an already mislabelled install is genuinely reachable, because selectedModel is .persisted at BLEManager.swift:1278 rather than derived from the registry row, so a row reading "WHOOP 4.0" does not gate its own DIS read shut.

One hole, though, and it needs a different call site.

The stamp no longer fires on the reconnect path

didDiscover / onScanResult is not the only road to a connect. The comment at BLEManager.swift:6608 says so outright: the connectCore paths (retrieveConnectedPeripherals / retrievePeripherals) reach didConnect without passing through discovery. Those are BLEManager.swift:1741 (preferred peripheral) and :1756 (last known), both retrieve by identifier and connect directly. Android has the same shape at WhoopBleClient.kt:4398, adp.getRemoteDevice(address) into connectGatt(autoConnect = true), the standing connect at :4386, which never reaches onScanResult.

On those routes the seeded row keeps its "WHOOP" placeholder. And the attestation cannot clean up after it:

// Packages/WhoopProtocol/Sources/WhoopProtocol/DeviceFamily.swift
static func forRegistryModel(_ model: String?) -> DeviceFamily {
    switch model {
    case "4.0", "WHOOP 4.0": return .whoop4
    default: return .whoop5          // "WHOOP" lands here
    }
}

reconcileModelFromAttestation guards on forRegistryDevice(...) == .whoop4 (BLEManager.swift:5407), so a placeholder row resolves .whoop5 and the guard returns early. Kotlin's forRegistryModel is identical, so the twin is equally blind.

Net effect: a WHOOP 4.0 whose first successful connect arrives by retrieve-and-connect rather than scan-and-connect keeps model "WHOOP", which resolves to .whoop5, which is the wrong skin-temp raw to degree-C scale (#938) and no generation on the card. That is the state #716 was filed about, and the old always-fires stamp happened to cover it. A 5/MG is untouched by this, because the placeholder resolves correctly for it by luck, which is why MG hardware testing would not surface it.

Suggested change

Call stampSeededModelIfNeeded() from didConnect instead of didDiscover, and from the Kotlin connect point rather than onScanResult. That is the one choke point every route reaches, and it keeps the property that makes this PR correct: it still cannot fire on an unsupported advertiser heard during the auto-scan's 4.0 leg, because nothing is connecting at that moment.

To be precise about what that costs: at didConnect the family is the persisted selectedModel, not a value just confirmed from an advertisement. That is the same value the stamp already writes, so moving the call does not weaken the signal, it only widens the set of connects that get stamped at all. The DIS attestation remains the corrector for a persisted family that was wrong.

Smaller notes

  • noteWhoop5VariantFromDIS's doc comment at BLEManager.swift:5289 still reads "Diagnostic only: nothing gates on it yet". It already called reconcileModelFromAttestation and adoptWhoopSerialIdentity, and now that 2A24 makes .mg actually resolvable it also drives isMG, which reaches DeviceConfigWriteGate.swift:183 (ecgGateOptIn && isMG). Worth correcting in this PR, since this is the change that makes the old wording wrong.
  • The Kotlin helper sets modelStamped = true before the pairedDevices() lookup, so a lookup that finds nothing burns the one-shot for the process. Swift sets the flag only after a successful write and so retries on the next connect. Pre-existing divergence, but it is inside the new function body now.
  • No tests on either side. The stamp's correctness is positional now, so the honest pin is a source tripwire asserting the call sits after the scan decision (or at the connect point, if you take the above), on both platforms. There is prior art in the tree for that shape of test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants