WHOOP MG: decode the "Labrador" ECG packets, plus a gated hand-run turn-on probe (default OFF) - #896
WHOOP MG: decode the "Labrador" ECG packets, plus a gated hand-run turn-on probe (default OFF)#896vishk23 wants to merge 8 commits into
Conversation
The MG carries ECG electrodes in its clasp (a plain 5.0 does not — Whoop5Variant
already tells them apart). Its ECG subsystem is "Labrador" in the protocol tables
and is its own realtime data type, not an R-numbered StrapSensorData layout:
a FILTERED stream and a RAW stream the strap persists for later offload.
Adds the pure decoder for both, plus the command payload/frame builders and the
turn-on report that classifies what a strap answers. No transport, no UI, no
storage — this commit is decoder-only and covered entirely by `swift test`.
The four command NUMBERS were already in our own CommandNumber table from the
whoomp/goose work. The packet field layouts and the {revision, arg, padding}
payload shape are protocol facts re-derived from static analysis of the vendor's
iOS client, attributed in ATTRIBUTION.md under the existing decompiled-app
doctrine. No implementation is copied.
PROTOCOL.md §9.1 records the mapping as a WORKING HYPOTHESIS and says why it
isn't settled: §6 lists FIVE ECG/HeartKey names against these four codes
(ECG_SEND_RAW is unaccounted for), 139 isn't contiguous with 123–125, and the
table is 4.0-derived while 5/MG is known to remap opcodes (MAVERICK answers
SET_CLOCK at 146, not 10). §6 keeps its "exact codes unconfirmed" caveat rather
than losing it to this change.
What is deliberately NOT asserted, because nothing attests it:
- the packet TYPE byte these arrive under (no capture exists, and PacketType
has no Labrador entry) — so the decoder works on a payload and ships a
structural triage for finding the type empirically;
- the WristSelection raw values (right-first is an inference off the enum
order, and SELECT_WRIST is a persistent write, so the inference is labelled
everywhere it surfaces);
- the heartKeyProgress "timed out" sentinel — 0...100 is a percentage and any
other byte is carried raw rather than renamed into an unproven state;
- the ECG sample unit and scale — hence filteredECGDataRaw, no µV conversion;
- the raw blob's bytes-per-sample, which is count ÷ numberOfECGSamples and so
is NOT on the wire: decodeRaw takes the width explicitly, and
rawBytesPerSampleCandidates enumerates what a buffer admits instead of
guessing. An ambiguous buffer refuses to decode.
Fails closed throughout: a short header, a numberOfECGSamples the buffer cannot
hold, truncated leads-off arrays, and a width whose product OR sum would overflow
the offset math all return nil rather than a partial decode or a trap. Kotlin
additionally range-checks list elements, because List<Int> can express values
Swift's [UInt8] cannot and the two must not diverge. Frame-level entry points
verify BOTH puffin CRCs before any field is read.
Whoop5EcgProbe separates the one gating question the client cannot answer —
whether firmware device-flags refuse the feature — from the COMMAND_RESPONSE
result code at frame[12]: UNSUPPORTED(3) means the opcode is not implemented,
FAILURE(0) is the signature most consistent with a device-flag block, and
all-SUCCESS with zero packets arriving is that block as a silent no-op. Silence
alone is never read as a block, because a mid-flight sync looks identical. The
attested result codes outrank the packet count deliberately: the count comes from
a shape heuristic that ordinary traffic can trip, so it yields a hedged
"candidate" verdict and never an unqualified "not blocked".
NOOP is not a medical device. The strap's embedded rhythm classifier ships its
verdict in every packet; this decodes the byte and nothing more. It is
unvalidated instrumentation, never a measurement and never a diagnosis — a test
asserts the report never names a classifier result at all.
UNVERIFIED ON HARDWARE: no strap has been asked whether it honours these
commands. Every fixture here is synthetic and pins the structural contract a
capture cannot change; none of it claims a capture exists.
Kotlin twin per the parity contract, same fixtures, same expected outputs.
Verification: swift test 393 passed / 0 failed (47 new);
./gradlew testFullDebugUnitTest --tests com.noop.protocol.Whoop5EcgTest
38 passed / 0 failed; full Android suite BUILD SUCCESSFUL.
Wires the decoder from the previous commit to the strap: the four Labrador
opcodes, an allowlist clause, and a user-initiated probe behind the existing
Experimental/diagnostic surface. Default OFF, MG-only, never automatic.
NOT VERIFIED ON HARDWARE. No strap has been asked whether it honours these
commands, so per the BLE safety contract this half is proposed for review, not
claimed working. The decoder commit beneath it stands alone without this.
Safety, per docs/CONTRIBUTING.md §BLE safety contract:
- All four commands are safe and reversible. Three are data-stream toggles;
SELECT_WRIST writes one persistent wrist value that is re-writable at any
time. None wipes data, reflashes, ship-modes, force-trims or power-cycles,
so the curated-safe-subset rule holds. The destructive firmware-load family
is absent from WhoopCommand entirely, so the sender cannot form those bytes.
- send() gains TWO guards, not one: ECG opcodes are dropped outright unless the
family is 5/MG (the 4.0 branch has no allowlist of its own, so the invariant
has to live in send() rather than resting on callers), and then again unless
the Experimental opt-in is on AND the strap has POSITIVELY attested itself an
MG over DIS. Whoop5Variant.unknown is not MG, so an unidentified strap stays
gated off.
- The new INBOUND path is CRC-gated before any byte is read (contract §2). The
probe's verdict is its whole output, and one flipped bit at frame[12] would
turn a healthy SUCCESS into "LIKELY blockedByDeviceFlags".
- The connect handshake is untouched. Nothing is sent on connect, on bond, or
on any timer — every byte follows a tap and a confirmation.
- Both inbound-driven collections (candidates AND unsolicited steps) are
capped, so a chatty stream cannot grow the report or the strap log unbounded.
SELECT_WRIST gets its own step, its own screen and its own confirmation rather
than a button inside the start flow. It is the only command here that changes
strap state outliving the session, AND its right=0/left=1 mapping is inferred
from the client enum's order rather than verified — so the sheet says both
plainly. A sheet, not a second dialog: presenting one confirmationDialog from
another races, and the warning is the point of the step and must be readable.
Turning the feature off must never be gated behind the switch that turned it on.
ecgStopCapture() stays reachable with the opt-in already off (ecgStopOverride,
scoped to one synchronous call and only able to carry OFF values); flipping the
Settings toggle off also tells the strap to stop; and because that send needs a
live MG link, an ecgMayBeRunning latch keeps the Devices "Stop" control offered
after the opt-in is gone rather than stranding a strap that may still be
streaming. The Settings path passes reportsResult: false so switching a setting
off can't pop the Devices result sheet from another screen.
Non-medical framing is carried at every surface that could be read as a result:
the Settings caption, the action dialog, and a line pinned above the report text.
The strap's classifier byte is logged as a NUMBER, never its token name — a strap
log is a shareable artefact and no line in it should read like a finding.
The probe answers the device-flag question empirically: it settles each command's
COMMAND_RESPONSE outcome, counts frames passing the structural triage over a 30s
window, and logs candidates plus the raw bytes of the first two, so a plain
strap-log export is enough to redo the decode offline. No new storage table; the
existing probe/strap-log path carries it.
Probe state, the ecgMayBeRunning latch's link assumptions, and the resolved
variant are all cleared on disconnect so a stale gate cannot unlock anything for
whatever connects next, and the key joins PuffinExperiment.fiveMGGatedKeys so a
4.0 ↔ 5/MG switch turns it off.
Verification: macOS (Strand) and iOS (NOOPiOS) both build — Strand/BLE and the
UI are app-target Swift with no default CI coverage, so these local builds are
the verification. swift test 393 passed / 0 failed. Android untouched by this
commit (no ECG app layer there yet — decoder parity only).
…ct-honesty # Conflicts: # docs/PROTOCOL.md
…not support
A SELECT_WRIST-only run on a WHOOP 5 MG rendered "Accepted but SILENT — every
command returned SUCCESS yet no ECG packet arrived in 30s. Consistent with a
device-flag block applied as a silent no-op." No data-generation command was
sent, so zero packets was the correct and expected outcome. A second wrist-only
run whose write came back FAILURE(0) rendered "LIKELY blockedByDeviceFlags".
Both verdicts read silence, or a refusal, as evidence about the question the
probe exists to answer without checking whether the run had asked the ECG data
path for anything. That manufactures support for hypothesis (e) of #891 — the
one the issue says deserves the least trust precisely because it is the string
the code was written to print. The OFF path had the same defect: three toggles
sent with their OFF arguments get three SUCCESSes and the silence they asked
for, and reported a block.
Scoped rather than special-cased on opcode 123. Every Step now records whether
its command, WITH THE ARGUMENT IT WAS SENT WITH, could make the strap emit
realtime ECG data — a new pure Whoop5Ecg.requestsRealtimeData(cmd:arg:) beside
the decoders, mirrored in Kotlin. SELECT_WRIST starts nothing on either
argument; a toggle sent with 0 asks for silence; RAW_SAVE names flash rather
than a live channel, so a realtime window cannot observe it either way; an
unsolicited reply's argument is unknowable. false can only weaken a verdict, so
an omission fails safe, and the field has no default so every construction site
must state it.
acceptedButSilent now needs a data request the strap acknowledged with SUCCESS;
blockedByDeviceFlagsLikely needs the refused command to have asked for data.
Three honest verdicts replace the over-claims: noDataRequested ("NOT A TEST of
whether ECG is blocked"), dataRequestNotAccepted, and commandRefused. The report
annotates each step with why it does or does not bear on the question.
Whoop5EcgProbe is now mirrored as com.noop.protocol.Whoop5EcgProbe. The earlier
"not mirrored, Android has no ECG app layer" reasoning is wrong for this type:
it decides whether a null result gets published as evidence of a firmware block.
Merge resolution with #893: the four ECG opcodes leave Android's CommandNumber
SENDER enum. They were there only to label a COMMAND_RESPONSE, which #893's
read-only CommandNames table now does without making them constructible, and
Android sends none of them. Whoop5EcgTest pins the same name<->code mapping
against CommandNames instead.
swift test 479 passed; gradlew testFullDebugUnitTest 3243 tests, 1 pre-existing
failure (DeepCaptureMigrationTest.repositoryInsertV18Aux_insertsThenPrunes, red
on main since #888, fixed by #897), verified from the JUnit XML.
Refs #891.
The defectTwo runs on a real WHOOP 5 MG tonight sent only and, on the run where the same wrist write came back Both are wrong, and wrong in the direction that costs most. Zero ECG packets after a run that asked for no ECG data is the correct and expected outcome; a firmware refusing one configuration write says nothing about whether ECG generation is gated. That manufactures support for hypothesis (e) of #891 — the one the issue explicitly says deserves the least trust, because it is the string the code was written to print. Pasted into the thread unqualified it would have corrupted the conclusion. The OFF path had the same defect: The fixNot a special case on opcode 123. Every step now records whether the command — with the argument it was sent with — could have made the strap emit realtime ECG data, via a new pure predicate
The two verdicts that interpret silence or refusal are then scoped:
The report now annotates every step with why it does or does not bear on the question ( Kotlin twin
Merge resolution with #893This branch predates #893 and listed the four ECG opcodes in Android's Verification
New tests cover: the predicate itself (opcode × argument, both platforms); wrist-only SUCCESS; wrist-only FAILURE; the OFF sequence; raw-save alone; toggles sent but unacknowledged; toggles sent and silent; toggles sent with packets; and an invariant sweep asserting that no run without a data request can produce either affirmative block claim, across every reachable outcome code. Refs #891. |
ryanbr
left a comment
ryanbr
left a comment
There was a problem hiding this comment.
Reviewed. The safety construction is careful and the honesty about what is unverified is exactly right. Findings below, starting with the one that is not yours.
The failing Android test is a regression from #888, and you are right to disown it
Confirmed, with the mechanism. #888 made the v18-aux prune amortized — WhoopRepository now sweeps only when banked >= V18_AUX_PRUNE_EVERY_ROWS (10 000). The Swift side was updated to suit: DeepCaptureChannelsTests injects the threshold (v18AuxPruneEveryRows: 1, 7, 5_000) so the tests still exercise a sweep. The Kotlin twin was not. repositoryInsertV18Aux_insertsThenPrunes inserts one row and asserts pruneV18Aux fired, which the amortized path cannot do.
So it is red on main and has been since #888, invisible because android.yml is disabled. Nothing to do with this branch. Being fixed separately — the Kotlin threshold is a const where Swift's is an injectable parameter, so the fix is to make it injectable rather than to loosen the assertion.
The CONFLICTING label is stale — no rebase needed
merge-tree against current main returns clean, and a real test merge produced no conflicted files. GitHub had not recomputed after #902/#903 landed. Ignore it.
Two CI failures, both small and real
doc-comments:android/app/src/main/java/com/noop/protocol/Whoop5Ecg.kt:363— stacked doc block, the one below it wins. (This one catches everybody; it caught me repeatedly this month.)i18n: 17 new literals absent from the catalog, all inDevicesView/SettingsView.
The i18n one is worth more than a lint fix here. Every non-medical disclaimer is in that list — the Settings caption, the action-dialog body, "Unvalidated instrumentation, not a medical measurement and not a diagnosis", and the wrist sheet's "may set the opposite wrist" warning. The PR's safety argument is that the framing is carried at every surface that could read as a result; that argument only holds in English until these are extracted. Given #902 just moved the notification surface into the catalog, this is the one place I would not merge with a follow-up promise.
The change I would actually ask for: tell the operator to close the circuit
leadsOn is decoded and printed in the triage line — good, because it is the discriminator for #891's hypothesis (a), the one free explanation for that null result. But nothing in the start dialog tells the user to hold the opposite-hand contact for the window. A single-lead ECG needs the circuit closed; the strap is one electrode and the wearer's other hand is the second.
As it stands the likely run is: tap Start, wear the strap normally, get leadsOn=0 and no frames, and read that as "the commands did nothing" — reproducing #891 exactly, and pointing at the device-flag hypothesis when the cause was the operator. One line of copy in the confirmation ("hold the metal clasp with your other hand for the whole window") turns this from a probe that likely re-derives the null into one that actually tests (a). Worth stating in the report too when leadsOn=0 on every frame: "leads never closed — rerun holding the clasp" is a different verdict from "no data".
Scope: Android gets the decoder but cannot run the probe
The transport, the allowlist clause, the UI and the settings toggle are Apple-only; Android gets Whoop5Ecg.kt + Whoop5EcgProbe.kt and no way to drive them. That is a defensible split, but CLAUDE.md asks for both platforms or an explicit reason, and the PR body reads as though the second commit covers both. Worth a sentence — particularly since #891's ask is aimed at MG owners generally, and half of them cannot answer it from this.
What is right
- Attribution is the correct shape: facts re-derived and credited, no vendor code or literals, and the unattestable fields (wrist enum values, timeout sentinel, sample unit, packet type byte) carried raw rather than named.
Whoop5EcgTestsstates that every fixture is synthetic and says why inventing a capture would be worse — the practice #903 just asked for, done better than the doc asks.- Removing
SELECT_WRISTfrom the Kotlin sender enum and citing #893's label table is the right call, and it narrows the sender surface instead of widening it. Verified nothing in the Kotlin tree references it and no test inCommandCatalogueTestasserts on 123, so the removal is clean. - Two gates on
send()rather than one, with the family check placed insend()because the 4.0 branch has no allowlist of its own — that is the right place for the invariant. - The commit split is the right shape. Commit 1 stands on its own: it makes a captured log decodable offline, which is useful whether or not anyone ever gets a strap to emit one.
One thing to keep visible
The four opcodes are still an unconfirmed name↔code mapping — docs/PROTOCOL.md now says so explicitly after #893, since the table's own "five consecutive codes" claim did not survive contact with the table. A SUCCESS from a strap is not evidence the command means what the name says, and the 5/MG demonstrably remaps opcodes into the high space. The probe is the right way to find out; it is worth the report saying that a SUCCESS on 124/125/139 confirms only that the opcode was accepted.
|
Opened #904 for the Mirrors what the Swift half of #888 already did: Your diagnosis was exact — nothing on this branch goes near it. |
…y is (#888) (#904) `DeepCaptureMigrationTest.repositoryInsertV18Aux_insertsThenPrunes` has been failing on main since #888. It inserts one aux row and asserts pruneV18Aux fired, which the amortised sweep cannot do — that PR changed the sweep to run once per V18_AUX_PRUNE_EVERY_ROWS (10 000) rows rather than on every batch. The Swift half of #888 handled this: StreamStore.insert takes v18AuxRetentionRows and v18AuxPruneEveryRows as parameters, and DeepCaptureChannelsTests passes 1, 7 and 5 000 to exercise the sweep, the deferral and the counter reset. The Kotlin twin kept reading the companion constants, so no test could observe the sweep without inserting ten thousand rows, and the one test that tried was left asserting the pre-#888 behaviour. android.yml is disabled, so nothing reported it. Found by @vishk23 while running the suite for #896, and correctly identified there as pre-existing rather than theirs. WhoopRepository.insert now takes both as defaulted parameters, mirroring the Swift signature. Production callers pass neither and get the shipped constants; every existing call site passes exactly two arguments, so appending defaulted parameters leaves them binding as before. Also brings the Kotlin coverage up to what Swift has had since #888: the fixed test now states why it needs a threshold of 1, and two more pin the amortisation itself — that three rows against a 5 000-row budget sweep nothing, and that two rows per batch against a budget of 3 sweep exactly once because the counter resets. Those are the twins of testRetentionSweepIsDeferredBelowTheThreshold, testRetentionSweepRunsOnceTheThresholdIsCrossed and testTheAmortisationCounterResetsAfterEachSweep. Verified: the three expected sweep counts were re-derived by simulating the exact gate (banked accumulates per device, sweeps at >= budget, resets only on a successful sweep) rather than by arithmetic in my head; all call sites checked for positional-argument binding; doc_comment_lint passes. Not compiled — Gradle cannot run on this host (aapt2 is x86-64 only) and no CI job builds Android, which is the same gap that let the regression sit on main for this long.
… doc
Two Android-only follow-ups to the #893 merge resolution. Neither changes
behaviour and neither sends a byte.
SELECT_WRIST (123) is restored to CommandNumber. Dropping the three
TOGGLE_LABRADOR codes was right — this branch would have ADDED them, which is
the widening #893 declined — but 123 was not this branch's to remove. It has
been in that enum since the initial commit, predates the ECG work, and is
referenced by no Kotlin caller, so removing it changed the curated send surface
for a reason unrelated to decoding ECG packets. With it back, the sender enum is
byte-identical to main's: this branch now adds nothing to it and removes nothing
from it. theMgEcgTogglesAreNamedButNotSendable is unaffected — it asserts
124/125/139, not 123.
Whoop5Ecg.kt carried two stacked doc blocks on plausibleFilteredFrame, so the
first bound to nothing and Dokka kept only the second. The orphaned one was the
caveat that matters ("a HEURISTIC, not a classifier: nothing downstream may
treat a hit as proof"). Tools/doc_comment_lint.py fails on it, and the
doc-comments CI job is currently red for exactly this. Moved to
plausibleFilteredPayload, which had no doc at all and is where the Swift twin
carries the same text, so Swift/Kotlin doc parity is restored too.
|
Two Android-only follow-ups to the #893 merge resolution above, pushed as
|
… ones for a human
The i18n gate was red because all 17 new ECG literals were absent from
Strand/Resources/Localizable.xcstrings entirely. They are now extracted, so
"no new un-extracted literals" is green — but the fix is deliberately NOT
uniform across them.
Thirteen are functional UI copy: menu items, buttons, the wrist-picker title,
the probe-result header, the "listening" status line, and the two technical
caveats (that SELECT_WRIST persists on the strap, and that the left/right
mapping is inferred from the vendor app's enum order rather than attested).
Those are translated into all eight shipped locales, following the terminology
the sibling #103/#761 probe strings already established — de "EKG", es/fr/it/
pt-PT "ECG", ru "ЭКГ", zh "心电图/心電圖"; strap as de "Strap", es "pulsera",
fr "bracelet", it "fascia", pt-PT "bracelete", ru "браслет", zh "手环/手環" —
and matching each locale's register (fr formal "vous", the rest informal).
Cross-references quote the localized UI labels ("Geräte"/"Dispositivos"/
"Appareils", "Puffin-Frames in eine Datei aufzeichnen") so they can be found
on screen. All eight locales are required, not just the focus four: the
non-focus allowance in Tools/i18n_extra_locale_baseline.txt sits at exactly
zero headroom, so an untranslated key there breaks the ratcheting gate too.
Three are medical safety copy — "not a medical device", "not a diagnosis",
"see a doctor if you have symptoms". Those are left untranslated on purpose.
A disclaimer that drifts in meaning across four languages is a worse outcome
than one shown in English, and none of these can be composed from the 39
disclaimers already translated in this catalog; they make claims specific to
the ECG probe. They are added as catalog entries with a comment naming them as
needing a native speaker, and with NO localizations, so the runtime falls back
to the complete English source rather than rendering a blank or a guess.
The gate therefore stays red, and it now names exactly what is missing:
missing=3 in each focus locale, and +3 over the allowance in each of it/ru/
zh-Hans/zh-Hant. That residue is the three safety strings and nothing else.
Tools/i18n_extra_locale_baseline.txt is deliberately not raised to absorb them
— that file ratchets down only.
The red
|
…e (#891)
The scoping fix folded into #896 made the two silence-interpreting verdicts
unreachable unless the run actually asked the ECG data path for something. It
fixed REACHABILITY and left the WORDING alone, and the wording is independently
wrong:
Consistent with a device-flag block applied as a silent no-op.
That names `blockedByDeviceFlags`. Checkable in this tree: there is no
DEVICE_FLAG opcode anywhere in whoop_protocol.json's CommandNumber table -- no
verb reads such a flag, none writes one -- nothing on main implements it in
either language, and the identifier appears only as prose in #896's own
Whoop5EcgProbe header and one docs/PROTOCOL.md line calling it a "firmware
WhoopDeviceFlag layer". It is a client-side construct: never transmitted to a
strap, not a strap capability gate. The probe reaches its verdict from result
codes plus silence and never reads any flag, so it is in no position to
attribute the silence to one.
#891 then tested the leading NAMED firmware-side candidate on hardware -- wrote
enable_raw_data_w_ecg = '1', confirmed the read-back through
GET_DEVICE_CONFIG_VALUE(121), and ran the toggles with the electrodes held for
the full 30 s -- and still saw zero packets. So the flag-block reading is not
where the evidence points, and it was the probe's own vocabulary that kept
lending it weight, including in the opening post of #891.
Changes, mirrored across both platforms:
- `blockedByDeviceFlagsLikely` -> `dataRequestRefused`, and the headline now
says the reply carries THAT the firmware refused, never WHY.
- `acceptedButSilent` drops the causal clause and instead names the five
explanations that fit the same silence: banked to flash rather than streamed,
a wrong opcode mapping, no start verb among three TOGGLE_* commands, an
entitlement gate, an open electrode circuit.
- `opcodeUnsupported` no longer says "not a device-flag block" -- denying the
claim still puts the vocabulary in front of the reader.
- docs/PROTOCOL.md §9.1 rewritten: records the correction and the #891
falsification rather than describing a firmware WhoopDeviceFlag layer.
Tests pin it by enumerating every Verdict CASE rather than every input, so a new
case cannot be added with the old vocabulary and slip through on the grounds
that nothing reaches it. A second test asserts the silent verdict still names
the alternatives, so removing a false cause does not leave the report mute.
The general rule, since it outlives this probe: a verdict may name an
OBSERVATION, never a MECHANISM. An instrument that sees only result codes and
packet counts has no license to use nouns beyond those.
Verification:
- swift test in Packages/WhoopProtocol: 481 tests, 0 failures
- ./gradlew testFullDebugUnitTest --tests "com.noop.protocol.Whoop5EcgProbeTest":
18 tests, 0 failures
…ct (#914) The #103 probe's no-answer verdict printed one sentence — "neither GET_FF_VALUE(128) nor GET_DEVICE_CONFIG_VALUE(121) is served by this firmware" — for three runs that establish three different things, and for one that establishes the opposite. Two timeouts got it. But `BLEManager.send` can return without transmitting at all: there is an early return when `cmdCharacteristic` is missing, and another in the 5/MG allowlist else-branch. So a run in which nothing ever reached the strap rendered a claim about what the strap's firmware serves. The file's own header names answered / rejected as UNSUPPORTED / silent as three distinct outcomes and then the verdict collapsed silence into the same factual claim as a refusal. One refusal got it for both verbs. The `||` meant 128 answering UNSUPPORTED(3) while 121 timed out printed "neither ... is served — rejected as UNSUPPORTED". 121 was never refused; it was never heard from. An undecodable reply got the bare "is not served" sentence. A CRC or envelope failure is affirmative evidence the strap DID transmit, which is the opposite of what that sentence says. The sentence is now per-verb and tied to the evidence that produced it: UNSUPPORTED is "refused by firmware (UNSUPPORTED)", a timeout is "served no reply in Ns — unconfirmed" (the window is now recorded, not just traced), an undecodable reply is "replied but the frame did not decode — unconfirmed", and an untried verb is "not asked". Only UNSUPPORTED is the firmware answering, so the strong "not served by this firmware" wording survives in exactly one case: the firmware refused BOTH verbs itself, where the `||` is now an `&&`. Same defect class as the ECG probe fix on #896: a verdict asserting a conclusion the run's own inputs could not support. Mirrored byte-for-byte in com.noop.protocol.DeviceConfigReadProbe; the golden cross-platform report test is unaffected because it exercises the answered path. swift test 428 passed, 0 failures; gradlew testFullDebugUnitTest 3199 tests, 0 failures, 5 skipped, verified from the JUnit XML (the DeepCaptureMigrationTest failure noted in #897 is fixed on current main and passes here). Refs #103. Co-authored-by: Fanboynz <mp3geek@gmail.com>
`FeatureFlagProbeReport.verdict` returned
strap announced N flag(s) but named none
whenever `keys` was empty and the strap had answered 117 — with no check
that a single SEND_NEXT_FF(118) reply had ever been decoded. That case is
reachable and is the likely first-contact result on a 5/MG:
`probeFeatureFlags()` sends 117, `noteStart` records the count,
`sendFeatureFlagStep(.sendNextFeatureFlag)` goes out, the 8s timer fires,
`noteTimeout(command: 118)` writes a trace line, and the report renders.
Zero replies were read, yet the headline asserted a fact about the strap's
key list — blaming the strap for our own timeout.
That is the conclusion someone pastes into #103 as evidence the enumerate
verb answers but serves nothing, when the run established no such thing.
It also contradicts the discipline the branch two conditions above states
outright and applies correctly to `skipped > 0`: never report our
limitation as the strap's behaviour. Same defect class as the ECG probe
verdict fixed in #896 — a verdict asserting a conclusion the run's own
inputs could not support.
`steps` already counts decoded SEND_NEXT replies, so the gate is
`steps == 0`, and the walk having produced no reply is reported as
inconclusive. `stopReason` renders directly beneath and names which of
the two causes it was (timeout, or a 118 reply that failed to decode),
so the one branch covers both without over-claiming either.
Also stops the verdict restating an implausible announced count as bare
fact. `noteStart` already marks a count outside 1…128 in the trace, but
the verdict is the line that gets quoted; it now carries the same doubt.
A plausible count renders exactly as before, so real reports are
unchanged.
The Swift test that pinned the old string as intended behaviour asserted
it after `noteStart` + `noteTimeout(118)` — the buggy path itself. Split
into the two cases it was conflating: no 118 reply (inconclusive) versus
a 118 reply that walked to the end marker carrying no name, which is a
genuine fact about the strap and still says "named none" plainly.
Kotlin twin updated in step; the rendered strings stay byte-identical
across platforms.
Verified: Packages/WhoopProtocol `swift test` 427 tests, 0 failures.
`./gradlew testFullDebugUnitTest` (JDK 17) 3198 tests, 0 failures,
5 skipped, counted from app/build/test-results/*.xml on a forced
`--rerun`. Both new tests confirmed to fail against the unfixed source
(Swift 4 assertions, Kotlin 2 tests) before the fix was restored.
Co-authored-by: Fanboynz <mp3geek@gmail.com>
There was a problem hiding this comment.
Re-reviewed. doc-comments passes now, and the red Android test turned out to be #888's — fixed by #904. Three things left.
The CI failure is the disclaimers. 16 new strings, 13 translated. The three left in English are the "not a medical device" warnings. So on a German or French phone the whole ECG screen translates except the safety text. Just needs the translations.
Nobody is told to hold the clasp. The only mention is the Settings blurb saying where the electrodes are. #891 asks other MG owners to run this — you knew to hold thumb and index on the indents, they won't. They'll get leads=0, no data, and report "nothing happened", which sends the thread back to the wrong hypothesis. One line in the start dialog fixes it, plus a separate result line when the leads never close.
blockedByDeviceFlags is still in here — the phrase you retired in #891. Your fix is already written in vishk23#11. Worth pulling in rather than merging something you've since disowned.
Needs a rebase too, but only the strings file, so it's the same pass as the translations.
…o review asks Three of the four things the 07-27 review left open. The fourth — the 24 human translations of the medical-safety copy — is deliberately NOT touched here. **blockedByDeviceFlags.** The fix was already written on the fork (#11) and is folded in: `473c724e`. The phrase now appears only where it is being RETIRED — assertions that no verdict may print it, and the doc paragraphs explaining that it is a CLIENT-SIDE construct no command in `whoop_protocol.json` carries. Nothing asserts it any more. **Nobody is told to hold the clasp.** Two additions, on both platforms: * The start dialog now carries the run instruction — hold the two clasp indents with the fingers of the OTHER hand for the whole capture, because the MG measures across the wrist electrode AND the clasp, so until it is held the circuit is open and the capture returns zero whatever the firmware does. It is appended to the existing safety string rather than added as a new key, so it costs no new translation debt and reaches the human translator in the same pass; the translator comment now names it as a run instruction that must survive translation. * `Whoop5EcgProbe.report` gains a result line when zero packets arrived AND the run did ask for data. It is phrased as a QUESTION about the run, never a finding: lead state is not on the wire, so the report says it CANNOT tell an open circuit from a strap that ignored the command. Claiming the leads were open would be the same manufactured-cause error as the retired device-flag wording. Silent when packets arrived, and silent when nothing asked for data (that case already has its own sentence). Twin wording in Kotlin, byte-identical. **The strings-file rebase.** `Localizable.xcstrings` was the only conflict, and it was resolved as a 3-way SEMANTIC merge on the `strings` dict rather than textually — keeping both sides of an .xcstrings conflict duplicates top-level keys and JSON parsing hides it. base 3274, ours 3290, theirs 3294 -> merged 3310; ours-add 16, theirs-add 20, theirs-edit 31, conflicts 0, duplicate keys at any nesting level 0. **What is still open, and why nothing here changes it.** The i18n gate is red on exactly 3 strings x 8 locales = 24 translations of "not a medical device / not a diagnosis / see a doctor". Those are left in English on purpose: a disclaimer that drifts in meaning is worse than one shown in English, and the absent localizations make the runtime fall back to the complete English source. They need a native speaker, not a machine. Verified: WhoopProtocol 500/500 (including the two new electrode-question tests), android `./gradlew testFullDebugUnitTest` 3289 tests / 0 failures (5 skipped, the two Kotlin twins confirmed present in the JUnit XML), Tools/i18n_audit.py --ci unchanged at the same 24 (this merge adds no new gap), and the macOS app target BUILD SUCCEEDED (DevicesView.swift is app-target Swift that no CI job compiles).
|
Rebased, and two of your three points are now done. The third is the one I want to ask you about rather than guess at. Done
The clasp. You were right that this was the difference between a useful null and a misread one. Two places, both platforms:
The rebase. Verified: WhoopProtocol 500/500, The one left, and a questionThe red check is exactly 3 strings x 8 locales = 24 translations: "not a medical device", "not a diagnosis", "see a doctor". I have deliberately not machine-translated them, and I would rather not — a disclaimer that drifts in meaning is worse than one shown in English, and the absent localizations already make the runtime fall back to the complete English source. So the PR is blocked on finding 8 native speakers, which is not a thing I can schedule. Would you rather this were split? The decoder half — That turns a PR blocked on translators into a mergeable one plus a parked one, and it means #891 can point people at a decoder that is already on main. If you would rather keep it whole and wait for the translations, that is fine too — say which and I will do it. I did not want to split someone else's review in half without asking. |
2 participants
The MG carries ECG electrodes in its clasp; a plain 5.0 does not, and
Whoop5Variantalready tells the two apart. Its ECG subsystem is "Labrador" in the protocol tables — its own realtime data type, not an R-numberedStrapSensorDatalayout, with a FILTERED stream and a RAW stream the strap persists for later offload.Two commits, deliberately separable.
1.
protocol(mg)— decoder onlyPure decoder for both streams, plus the command payload/frame builders and the turn-on report that classifies what a strap answers. No transport, no UI, no storage. Swift
WhoopProtocol+ Kotlin twin, covered entirely by unit tests.The four command NUMBERS were already in our
CommandNumbertable from the whoomp/goose work. The packet field layouts and the{revision, arg, padding}payload shape are protocol facts re-derived from static analysis of the vendor's iOS client, attributed inATTRIBUTION.mdunder the existing decompiled-app entry.This commit stands alone. If the transport half below is not wanted, this one is still useful on its own — it is what lets a captured strap log be decoded offline.
2.
ble(mg)— gated, hand-run probe, default OFFWires the decoder to the strap: the four Labrador opcodes, an allowlist clause, and a user-initiated probe behind the existing Experimental/diagnostic surface. MG-only, never automatic.
NOT VERIFIED ON HARDWARE. No strap has been asked whether it honours these commands, so per the BLE safety contract this half is proposed for review, not claimed working.
Against
docs/CONTRIBUTING.md§BLE safety contract:SELECT_WRISTwrites one persistent wrist value that is re-writable at any time. None wipes data, reflashes, ship-modes, force-trims or power-cycles. The destructive firmware-load family is absent fromWhoopCommandentirely, so the sender cannot form those bytes.send()gains two guards, not one: ECG opcodes are dropped outright unless the family is 5/MG (the 4.0 branch has no allowlist of its own, so the invariant lives insend()rather than resting on callers), and again unless the Experimental opt-in is on and the strap has positively attested itself an MG over DIS.Whoop5Variant.unknownis not MG, so an unidentified strap stays gated off.frame[12]would turn a healthy SUCCESS into "LIKELY blockedByDeviceFlags", and the verdict is the probe's whole output.ecgMayBeRunninglatch's link assumptions, and the resolved variant are cleared on disconnect; the key joinsPuffinExperiment.fiveMGGatedKeysso a 4.0 ↔ 5/MG switch turns it off.SELECT_WRISTgets its own step, screen and confirmation rather than a button inside the start flow: it is the only command here that changes strap state outliving the session, and its right=0/left=1 mapping is inferred from the client enum's order rather than verified — the sheet says both plainly.Turning the feature off is never gated behind the switch that turned it on.
ecgStopCapture()stays reachable with the opt-in already off, flipping the Settings toggle off also tells the strap to stop, and anecgMayBeRunninglatch keeps the Devices "Stop" control offered rather than stranding a strap that may still be streaming.Non-medical framing is carried at every surface that could read as a result: the Settings caption, the action dialog, and a line pinned above the report text. The strap's classifier byte is logged as a number, never its token name — a strap log is a shareable artefact and no line in it should read like a finding. Related: #752 (whether arrhythmia notifications are in scope) — this PR deliberately produces no notification and no finding, only a decode.
What the probe answers
Whether the device-flag question is empirical: it settles each command's
COMMAND_RESPONSEoutcome, counts frames passing the structural triage over a 30 s window, and logs candidates plus the raw bytes of the first two — so a plain strap-log export is enough to redo the decode offline. No new storage table; the existing probe/strap-log path carries it.Verification
Rebased on current
main.swift testinPackages/WhoopProtocol: 462 tests, 0 failures./gradlew testFullDebugUnitTest: 3218 tests, 5 skipped, 1 failure —DeepCaptureMigrationTest.repositoryInsertV18Aux_insertsThenPrunes, which also fails on plainmainwith none of this branch applied (verified on a clean checkout of2ff2af18:7 tests completed, 1 failed). It is a pre-existing Kotlin-side regression from Amortize the v18 aux retention sweep instead of running it per insert batch #888, not something this branch touches — nothing here goes nearWhoopRepository/v18AuxSample. Filed separately.xcodebuild -scheme Strand -destination 'platform=macOS' test: 964 tests, 1 skipped, 0 failures (** TEST SUCCEEDED **)Strand/BLEand the UI are app-target Swift with no default CI coverage, so the local macOS/iOS builds are the verification for that half.