Make the R22 deep-data unlock actually reversible: a disable sequence with a mandatory read-back (#174)

[vishk23]

The defect

The Settings copy has said since #174 that the R22 unlock is "reversible (it only changes which data the strap chooses to emit)", and Whoop5Config's header repeated it. That is true about the hardware and false about the app: NOOP shipped an enable button and nothing that writes the flags back. The toggle has never written anything in either direction — it gates sends — so turning it off silently left all sixteen flags set on the strap. The only ways out were the official WHOOP app or a factory reset.

This adds the undo, and verifies it by reading the strap rather than by believing an ack.

What it writes

Whoop5Config.disableR22Sequence — the same sixteen keys, same order as enableR22Sequence, each carrying featureFlagOffValue = ASCII '0' (0x30). Master flag first, so a run interrupted by a disconnect leaves the strap nearer off than it started, and the two sequences stay trivially diffable.

On the off value, precisely. This is the one inferred byte, and the inference is stronger than a guess and weaker than a measurement, so it is worth stating exactly:

Confirmed	0x30 is this firmware's off value in the device-config namespace. setBroadcastHr(false) has written it to whoop_live_hr_in_adv_ind_pkt via SET_DEVICE_CONFIG_VALUE(119) since #181, hardware-validated against a Garmin Edge 840. enable_raw_data_w_ecg independently reads '0' on an MG whose ECG is idle.
Why it transfers	The two namespaces are one wire idiom: key name ASCII NUL-padded to 32 bytes, single ASCII-digit value at offset 32, [0x01] + inner b3 prefix, written with response. Only the opcode (0x77 vs 0x78) and body length (33 vs 40) differ. deviceConfigBody's own doc spells the convention out as "an ASCII digit, '1' = 0x31 / '0' = 0x30".
Why writes can't be one-way	Writes through 120 demonstrably change stored state: running enableR22Sequence moved enable_sig12 from '2' (0x32) to '1' (0x31), confirmed by a GET_FF_VALUE(128) read before and after. A key that can be written is a key that can be rewritten.
Not established	No feature flag has ever been observed holding '0'. Every 128 read ever taken returned '1' or '2', and those sixteen readings are a round-trip of NOOP's own writes. The namespaces are proven separate at the verb level — a key asked through the wrong verb answers FAILURE — so the evidence above is an argument by shared convention, not a measurement of this opcode.

And two pieces of counter-evidence kept in view rather than smoothed over: disable_pip_r26_packets is written '2' despite being named disable_*, and enable_sig12 was corrected '2' → '1' from a real capture (#423) — so the official app picks between '1' and '2' per flag rather than using one canonical "true". Tri-state semantics stay unestablished, and nothing here assumes them.

So the run tests the byte instead of asserting it

R22DisableReport is staged rather than a blanket sixteen writes:

1. Probe — write '0' to ONE flag, read it back with GET_FF_VALUE(128).
2. Gate — if the strap did not stop reporting the old value, STOP. Fifteen keys untouched, and the report says '0' is not how this firmware clears a flag. That is a publishable answer, not a failure.
3. Clear — only on a good probe, write the remaining fifteen.
4. Verify — read all sixteen back and tabulate enable wrote X → now Y.

The probe key is enable_sig12: the only key with a hardware demonstration that a write to it moves stored state, so a failure there is attributable to the value rather than to the key or the verb. Every other key would leave that ambiguity open. It is also the flag with least to lose — undocumented effect, gates no stream NOOP reads.

Read-back separates three outcomes a boolean "did it work" would blur:

Read-back	Meaning
value '0'	cleared — the key exists and holds the off value
FAILURE(0)	unset — no stored value at all. Nothing predicts this; reported as its own state
value unchanged	unchanged — the write was refused or no-oped

Enumeration cannot substitute for the read. '0' is a stored value, not an absence — six of the seven keys the 115/116 device-config walk returned read '0' and were still enumerated — so a cleared key stays listed rather than dropping out. And the 117/118 feature-flag walk carries no value field at all ([revision][index][validKey][key]), so it can only answer presence. 128 is the only verb that can verify a clear, and it answered cleanly for all sixteen flags on a 5/MG.

The ack is never the proof. Every write's COMMAND_RESPONSE is recorded and not believed — SELECT_WRIST returns SUCCESS for a no-op and FAILURE for a real mutation on this firmware. A SUCCESS ack whose read-back did not move renders as unchanged, never as success. Discipline owed to #907 / #891.

On disable_pip_r26_packets, the one line a reviewer should push back on

Fifteen keys read as "enable/tune X" and one reads as "disable X", so writing the same byte to all sixteen looks wrong at a glance. It is deliberate. This sequence is defined as the undo of the enable sequence — clear every flag it set — not as "set each key to its semantic opposite". Those are different operations and only the first is well defined here: inverting per key would require knowing what '1' and '2' each select, and this key is the evidence we do not.

So writing '0' to it is expected to stop PIP R26 packets being suppressed — i.e. let them flow again, which is the pre-R22 behaviour and the correct restoration. The polarity inversion is real, but it lives in what the key means, not in which byte undoes it. It is called out in the report so nobody is surprised that one key's "off" is another feature's "on". A test pins it.

What this does NOT do

It does not restore a snapshot. NOOP has never read these values before writing them, so the strap's pre-NOOP state is unknown and unrecoverable. If the firmware's own default for some key is '1' rather than unset, this writes a value the factory never used. The honest claim is "clears the sixteen flags NOOP set". Making the stronger claim true needs a read-before-write snapshot on the enable path — worth doing, deliberately out of scope here.

Also in this PR

• FeatureFlagWriteGate tightens the send allowlist for opcode 120. The clause it replaces was opcode-only (command == .setConfig && deepDataEnabled), so any feature-flag key with any value travelled for as long as the deep-data opt-in happened to be on. It is now key- and value-aware: the sixteen R22 keys, each only for its own enable value or the off value. Exactly the weakness Make the ECG gate hypothesis testable: an opt-in enable_raw_data_w_ecg write with a mandatory read-back (#891, #103) #907 closed on opcode 119, and the disable path doubles the number of values going through 120, so it is tightened rather than widened. One pure predicate the send path itself consults, so the tests prove it about the real wire path.
• .onChange on the toggle, offering the disable instead of silently writing nothing. Broadcast HR has had one since [android] Today HR chart: arbitrary x-axis times, no time in the drag readout #181; this one was missing.
• The card said "15" while the sequence carried 16 (enable_sig12, WHOOP 5.0 raw HCI capture — where to submit? (docs reference #174, which doesn't exist) #103), so it declared success one flag early and named a count that had drifted. Threshold and number now both come from enableR22Sequence.count.
• A disable run's acks no longer tick the enable counter upward — both go through SET_CONFIG, so without the guard turning R22 off incremented "Strap accepted N/16 R22 flags".
• An interrupted run is rendered, not dropped. Unlike the read-only probes it has already written, so the user is told how far it got and which keys are still set.

Verification

Swift swift test (Packages/WhoopProtocol)	454 tests, 0 failures (24 new)
Android testFullDebugUnitTest	3226 tests, 0 failures, 392 classes — counted from the JUnit XML, not the wrapper exit code (25 new)
Android assembleFullDebug	clean
macOS Strand / iOS NOOPiOS	both build (app-build.yml is disabled, so this was run locally)
Tools/i18n_audit.py --ci	passes; new copy translated in every focus locale and in it/ru/zh-Hans/zh-Hant so the extra-locale ratchet does not move

The pure report is order-dependent and CoreBluetooth-free, so the whole verdict table — including the case where the strap refuses '0' — is covered without hardware.

Not verified on hardware, and deliberately not claimed:

1. Whether the strap accepts '0' for a feature flag. This is what the probe stage measures on first run. A rejection is reported as a finding, with fifteen keys untouched.
2. Whether clearing the flags makes the strap's R22 behaviour revert. The report says what the strap stores. Whether the deep records stop needs a wear-and-sync afterwards — watch the type-0x2F deep-buffer capture. Both caveats are stated in the UI copy and in the rendered report rather than papered over.

How to run it

Settings → Experimental · WHOOP 5 / MG → Turn deep data back off. Or flip the Unlock WHOOP 5/MG deep data (R22) switch off and accept the prompt. Needs the full encrypted bond; unlike the enable it does not require the strap worn — the on-wrist gate exists because the R22 stream is on-wrist only, and turning a feature off should not require strapping the device back on.

Refs #174 (the original unlock), #890 / #907 (the read-back-not-the-ack pattern), #103 / #423 (the 16th flag and its value correction), #181 (the hardware-validated 0x30 precedent).

[ryanbr]

The staged probe is the right design, and tightening opcode 120 from "any key, any value while the
opt-in is on" to key- and value-aware is worth the PR on its own. Thanks for building both app targets —
nothing in CI covers them.

One blocker: the toggle-off path can't work.

deepDataEnabled is @AppStorage bound straight to the pref, so flipping the switch off writes false
immediately. .onChange then raises the dialog, and the confirm button calls disableWhoop5DeepData() —
which opens with guard PuffinExperiment.deepDataEnabled. The pref is already false, so it returns at
the guard.

The user is shown "NOOP can write the off value to all 16 now and read each one back", taps Clear
flags on strap, and nothing happens. The only trace is a strap-log line telling them to turn the toggle
back on. Android is identical (if (!puffinExperiment.isDeepDataEnabled) at WhoopBleClient:5728).

That is the same shape as the defect this PR fixes: UI promising an undo that never runs. It also makes
the second flow in "How to run it" dead, and the button path is the only one that works.

The allowlist already has the pattern for the fix — it admits the read-back opcode on
r22DisableRun != nil rather than on the pref. Doing the same for opcode 120 restricted to
featureFlagOffValue would let a run in flight clear the flags regardless of the toggle, which is
exactly when clearing them is wanted. Then the guard in disableWhoop5DeepData() can go.

One nit, not blocking. In nextStep(), a key list without the probe key sets probePassed = true and
writes all sixteen unprobed. Unreachable today — both call sites use the no-arg init — but it is
fail-open on a write path whose whole safety argument is the probe, and probePassed = true records a
probe that never ran. Refusing would be truer to the design.

Also needs a rebase; main has moved several times today.

The evidence discipline is the best part of this and I would not want it lost in the fix: the "Not
established" row, keeping the disable_pip_r26_packets polarity and the '2' → '1' correction in
view rather than smoothing them, and choosing the one probe key with a hardware demonstration behind it
so a failure is attributable to the value.

[vishk23]

You are right, and the diagnosis was exact. I reproduced it before changing anything: SettingsView.swift:262 — the "Clear flags on strap" button, the one raised by flipping the toggle off — is dead, while SettingsView.swift:1527 "Turn deep data back off" works, and it works only because it renders while the pref is still true. So the feature half-worked, and the half that worked was the one a user is least likely to find. Pushed the fix, rebased onto 49830442.

The gate. admitsSend is split into two predicates, taking the shape the allowlist already used for the read-back rather than inventing one:

predicate	opcode	key	value	gate
admitsEnableWrite	120	one of the sixteen	that key's own enable value	deep-data opt-in
admitsDisableWrite	120	one of the sixteen	featureFlagOffValue only	r22DisableRun != nil

Then the guard PuffinExperiment.deepDataEnabled in disableWhoop5DeepData() is gone, both platforms, and advanceR22Disable()'s own fail-closed re-check now asks the disable predicate — it would otherwise have blocked at exactly the same place the guard did.

One thing I want to flag because it is a change beyond what you asked for: I also stopped admitsEnableWrite admitting the off value. Keeping the old "enable value or off value" rule on the pref-gated clause would have left the off value reachable under the opt-in alone, which is a narrower version of the same "any value while the opt-in is on" looseness this PR set out to close. Splitting on value as well as gate makes the invariant exact instead of approximate: an off value on the wire means a disable run is in flight. One predicate admitting either value under either gate cannot state that. testTheTwoDirectionsAreDisjointOnValue pins it in both directions, per key.

Gating on the run is also strictly narrower than the pref in the direction that matters for a write path: with the opt-in merely left switched on, no off value can reach the wire at all unless a run is genuinely walking its plan.

A second half of the defect, which fell out of fixing the first. The disable report rendered inside if deepDataEnabled (Swift SettingsView, Kotlin SettingsScreen). So even with the gate fixed, the commonest route to a run — flip the switch off, confirm — would have run correctly and shown the user nothing: no progress line, no read-back table, no dismiss button, because the pref is already false while the run is in flight. Both are hoisted out of that block. The report is about what is on the strap, which outlives the app's opt-in.

I do not think I would have found that one without your diagnosis; it is the same pref, one layer up.

The fail-open nit — fixed, and thank you for not letting it go as "unreachable". nextStep() now refuses a key list without the probe key: stopped, every key skipped, and a new probeUnavailable verdict distinct from probeInconclusive (there the probe ran and answered nothing; here it could not run at all). Your framing is the right one — probePassed = true was recording a probe that never ran, so a report could have claimed a passed gate on a run that never had one, which is worse than the unprobed writes.

Verified

• swift test WhoopProtocol: 472 tests, 0 failures (R22DisableTests 31 of them, up from 24).
• Kotlin testFullDebugUnitTest: 3253 tests, 0 failures, 0 errors, 5 skipped across 394 classes, read from the JUnit XML rather than the wrapper exit code. R22DisableTest is 32 of them.
• Both app targets built again, since nothing in CI does: xcodebuild BUILD SUCCEEDED for Strand (macOS) and NOOPiOS (iOS Simulator).
• Tools/i18n_audit.py --ci upstream/main green.

New tests worth naming: testOffValueWritesAreAdmittedWithTheOptInOffWhileARunIsInFlight is the direct regression test for this defect — it asserts the off value is admitted with deepDataOptIn: false, which is the actual state at the moment the user confirms. aKeyListWithoutTheProbeKeyRefusesToWriteAnything and theDefaultKeyListStillPlansTheProbe cover the nit from both sides. Kotlin twins for all of them.

Still not established, unchanged by any of this: whether '0' is how this firmware clears a feature flag. That is what the probe stage is for, and it remains unrun on hardware.

[vishk23]

Rebased onto ad378ce1 and green. The requested fix landed in a9713deb before this merge — the R22 undo is gated on THE RUN, not the pref, so turning the switch off no longer decides on its own whether the strap gets the clearing write.

The conflict this merge resolved was purely a tail-append collision: #973 appended a haptics string to all six Android catalogues at the same place this branch appends the R22-disable strings. Both sides kept, and then the thing that makes "keep both" safe or unsafe actually checked — every file re-parsed as XML and every name counted (1752/1752/1752/1752/1751/1709, zero duplicates in any locale). A duplicated name silently drops the earlier string, so it is verified rather than assumed.

./gradlew testFullDebugUnitTest: 3263 tests, 0 failures (5 skipped). Tools/i18n_audit.py --ci clean. Ready for a re-look whenever you have a moment.