Make the v20 optical byte map executable: name its eleven fields, CRC-gate the decoder - #895
Merged
Merged
Conversation
… (#423) The v20 (2,140-byte) byte layout is established and published in #423, but nothing in the tree could read a field out of one. Whoop5RawOptical had the shape right -- five 422-byte blocks, a 21-byte head, two 200-byte reading slots -- while keeping the head as an opaque byte blob and never verifying the frame's checksum. Name and type the eleven fields inside the head (sample_count, source_a, drive_a, source_b, drive_b, detector_a_select, range_a, offset_a, detector_b_select, range_b, offset_b), using exactly the neutral names from the public write-up so the code and the published record agree. They consume the 21 bytes with no residue. rawHeader still round-trips the raw bytes, and a test asserts it reproduces them -- the named view is an interpretation OF those bytes, not a replacement for them. CRC-gate decode. The old comment said callers 'should also use the normal envelope CRC verification path' and then did not; its own shape test decoded an all-zero buffer with three magic bytes set. decode now verifies the envelope and both checksums before reading any payload field, per the BLE safety contract. Correct the CRC input range on the way: it is [8:2136], not [26:2136] as the earlier #423 comment recorded -- from the record-class byte, not the start of the block tiling. Corroborated at a different frame size by @digitalerdude's v26 result (CRC-32 over bytes 8..83), so byte 8 is the frame-family rule. The useful consequence is that this is not a v20-specific CRC at all but the ordinary WHOOP 5 envelope rule already in the tree, so the decoder calls verifyFrame / Framing.frameCrcOk rather than growing its own. Header bytes 6:7, listed as unidentified in the write-up, are that envelope's CRC16-Modbus over [0:6]. Add r20_optical_oracle.json, byte-identical across Swift and Android, in the decoder_oracle idiom: two real captured records whose expected values were generated by an independent third implementation (Tools/gen_r20_oracle.py), so neither platform can agree with the other by construction. Covers the full per-field decode, the [25,0,0,25,25] pattern, sign extension at negative values, saturation at exactly 2^19-1, the CRC range, and five single-bit corruptions that must each be rejected. Both suites also carry an opt-in WHOOP_R20_CORPUS test that runs the decoder over a whole capture; against the 29,203-record corpus behind #423 both report 29203/29203 decoded, 0 invariant mismatches. Not wired into storage or analytics: no production consumer reads these fields. Refs #423.
ryanbr
added a commit
that referenced
this pull request
Aug 18, 2026
…ackfill Re-cut of #369 (by @vishk23) against current main. That branch was 385 commits behind and BOTH of its migration slots had been taken since: GRDB v32-apple-step-hour collided with v32-rr-src-channel (main is at v37) and Room 26 collided with MIGRATION_25_26 (main is at 30). Renumbered to the current tail rather than mutating shipped migrations. The daily Apple Health import flattens iPhone steps to one appleDaily.steps total, so an hour the phone spent dead or on a desk is invisible - the day just reads low. This adds an hourly table beside it: - GRDB v38-apple-step-hour + Room MIGRATION_30_31 (@database version 31, SCHEMA_VERSION 31): additive CREATE TABLE only, appleStepHour(deviceId, ts, steps), PK (deviceId, ts) like every other per-sample table. - Store API upsertAppleStepHours/appleStepHours mirroring OuraRawStore, with an idempotent ON CONFLICT upsert keyed on the natural key. - HKStatisticsCollectionQuery at hour interval over the same sync window, same anchor and error handling as the existing daily collect(), plus a one-time 90-day backfill flagged only after rows actually land (HealthKit returns EMPTY rather than an error when step access is denied, so gating on no-throw alone would burn the one-time widen on a deny-then-grant). - appleStepHour registered in deviceScopedTables so Forget-device wipes it. - Kotlin schema twin + AppleStepHourMigrationTest, and both byte-identical schema_oracle.json copies updated (roomVersion 31, v38 migration, table entry). Fixes on top of the original: - Corrected the documented claim about what a missing hour means. Only hours with a step sample produce a row, so an absent hour is "no steps recorded" - a dead phone AND a still hour both look identical. Step data carries no separate "was recording" signal, so the doc now says a gap is evidence, not proof, and a consumer must not label it "phone off". - Refreshed the stale renumbering commentary and the "v27 store" header left over from the earlier renumber. - Added a test pinning SCHEMA_VERSION to the migration target so the two cannot drift apart (the #1410 failure mode). SCOPE: plumbing only, no UI reads the table yet - same precedent as #346/#895. Android carries the schema but no importer writes it (HealthKit has no Android analogue), exactly as the older appleDaily table already does, so .noopbak stays byte-identical. Re-review also found the Android Forget-device path incomplete: appleStepHour was registered in the Swift deviceScopedTables (whose test enumerates every deviceId-keyed table automatically) but NOT in the hand-maintained Android twin in DeviceRegistryDao. Android never writes the table, but a .noopbak restored FROM iOS carries its rows, so "delete this device's data" — and in particular "Remove Apple Health imported data", which is exactly this data — left the hourly step history behind. Added the delete + re-key queries, wired both call sites, and extended the fan-out guard test to pin appleStepHour. Validation: full Android suite 4037 tests / 489 classes green (the fan-out guard caught the gap above before this landed), including SchemaOracleTest (compares Room KSP-generated schema against the oracle) and the new migration test. test (WhoopStore) green in CI — that covers the v38 migration, the store API, the GRDB-side oracle and the automatic deviceScopedTables coverage assertion. HealthKitBridge is app-target Swift, validated by app-build. Co-authored-by: vishk23 <119831996+vishk23@users.noreply.github.com>
ryanbr
added a commit
that referenced
this pull request
Aug 18, 2026
…ackfill (#1429) Re-cut of #369 (by @vishk23) against current main. That branch was 385 commits behind and BOTH of its migration slots had been taken since: GRDB v32-apple-step-hour collided with v32-rr-src-channel (main is at v37) and Room 26 collided with MIGRATION_25_26 (main is at 30). Renumbered to the current tail rather than mutating shipped migrations. The daily Apple Health import flattens iPhone steps to one appleDaily.steps total, so an hour the phone spent dead or on a desk is invisible - the day just reads low. This adds an hourly table beside it: - GRDB v38-apple-step-hour + Room MIGRATION_30_31 (@database version 31, SCHEMA_VERSION 31): additive CREATE TABLE only, appleStepHour(deviceId, ts, steps), PK (deviceId, ts) like every other per-sample table. - Store API upsertAppleStepHours/appleStepHours mirroring OuraRawStore, with an idempotent ON CONFLICT upsert keyed on the natural key. - HKStatisticsCollectionQuery at hour interval over the same sync window, same anchor and error handling as the existing daily collect(), plus a one-time 90-day backfill flagged only after rows actually land (HealthKit returns EMPTY rather than an error when step access is denied, so gating on no-throw alone would burn the one-time widen on a deny-then-grant). - appleStepHour registered in deviceScopedTables so Forget-device wipes it. - Kotlin schema twin + AppleStepHourMigrationTest, and both byte-identical schema_oracle.json copies updated (roomVersion 31, v38 migration, table entry). Fixes on top of the original: - Corrected the documented claim about what a missing hour means. Only hours with a step sample produce a row, so an absent hour is "no steps recorded" - a dead phone AND a still hour both look identical. Step data carries no separate "was recording" signal, so the doc now says a gap is evidence, not proof, and a consumer must not label it "phone off". - Refreshed the stale renumbering commentary and the "v27 store" header left over from the earlier renumber. - Added a test pinning SCHEMA_VERSION to the migration target so the two cannot drift apart (the #1410 failure mode). SCOPE: plumbing only, no UI reads the table yet - same precedent as #346/#895. Android carries the schema but no importer writes it (HealthKit has no Android analogue), exactly as the older appleDaily table already does, so .noopbak stays byte-identical. Re-review also found the Android Forget-device path incomplete: appleStepHour was registered in the Swift deviceScopedTables (whose test enumerates every deviceId-keyed table automatically) but NOT in the hand-maintained Android twin in DeviceRegistryDao. Android never writes the table, but a .noopbak restored FROM iOS carries its rows, so "delete this device's data" — and in particular "Remove Apple Health imported data", which is exactly this data — left the hourly step history behind. Added the delete + re-key queries, wired both call sites, and extended the fan-out guard test to pin appleStepHour. Validation: full Android suite 4037 tests / 489 classes green (the fan-out guard caught the gap above before this landed), including SchemaOracleTest (compares Room KSP-generated schema against the oracle) and the new migration test. test (WhoopStore) green in CI — that covers the v38 migration, the store API, the GRDB-side oracle and the automatic deviceScopedTables coverage assertion. HealthKitBridge is app-target Swift, validated by app-build. Co-authored-by: vishk23 <119831996+vishk23@users.noreply.github.com>
ryanbr
added a commit
that referenced
this pull request
Aug 18, 2026
Build numbers only. MARKETING_VERSION / versionName stay 10.1.1 — the staging line is one above the latest release (v10.1.0) and holds there until 10.1.1 actually ships. versionCode 341 -> 342 and CURRENT_PROJECT_VERSION 223 -> 224, both +1 past the previous staging build so a sideloaded update installs in place rather than being refused as a downgrade. Picks up since the last testing build (f2271cd): backup build provenance (#1422), the CTL/ATL/TSB training-load engine (#1423) and its Trends card (#1425), sleep-vs-wake HR contrast (#1424), the v20 optical decoder + CRC gate (#895), the Sleep UI split (#1426), the WHOOP4 clock-correlation fix (#1427), the settings waist-row layout + copy fix (#1428), hourly Apple step import (#1429), Low refresh (#1430), and Power saving moving into More with a 35% ceiling (#1431).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The v20 (2,140-byte) optical record's byte layout is established and published in #423 — but nothing in the tree can read a field out of one.
Whoop5RawOptical(from #546) gets the shape right: five 422-byte blocks, a 21-byte head, two 200-byte reading slots. It keeps the head as an opaque[UInt8]blob and never verifies the frame's checksum.This PR makes the layout executable: the eleven fields inside the head are named and typed, and the decoder is CRC-gated. Decoder and tests only — no storage, no analytics, no consumers. It should be reviewable and useful on its own.
1. The eleven fields (block-relative)
The 21-byte head is fully accounted for —
1+1+2+1+2+1+4+2+1+4+2 = 21, no residue. Both detector groups are 7 bytes with the identical internal shapeselect:1, range:4, offset:2, which is what makes the 7-byte stride visible in a raw hex dump of block 0.sample_countrange_asource_aoffset_adrive_adetector_b_selectsource_brange_bdrive_boffset_bdetector_a_selectreadings_a/readings_bNames are exactly the neutral ones from the #423 write-up, so the code and the public record agree. They describe what each field is observed to do and assert nothing about registers, units, or wavelength. The raw 21 bytes are still preserved —
rawHeaderround-trips them, and a test asserts it reproduces the on-wire bytes. The named view is an interpretation of those bytes, not a replacement for them.2. Correction: the CRC input range is
[8:2136], not[26:2136]If you built anything against the earlier number, it rejects every real record. My 2026-07-17 comment in #423 (and the body of #546) said the five blocks fill
[26:2136]"where the CRC32 sits". The tiling is right; the CRC range was wrong.It starts at the record-class byte. This matches @digitalerdude's independent v26 finding earlier in the thread — CRC-32 over bytes
8..83validating for all 1,080 of their records at a completely different frame size. Same frame family, same rule, so byte 8 is general rather than a v20 quirk. That cross-format agreement is what makes me confident enough to correct the record rather than just patch a constant.And the useful consequence: this is not a v20-specific CRC at all — it is the ordinary WHOOP 5 envelope rule the repo already implements.
@2:3declares 2132, so the envelope's payload span[8, declaredLength+8-4)is[8, 2136). The decoder therefore callsverifyFrame(_:family:.whoop5)/Framing.frameCrcOkrather than growing its own checksum.Small new finding while wiring that up: header bytes
6:7— listed as "unidentified" in the #423 table — are the envelope's CRC16-Modbus over bytes[0:6], holding 29,203/29,203. They read as a constant0xB3B5in that capture only because bytes 0–5 are constant. One fewer unknown byte, and it means the whole record sits inside the already-known envelope.3. CRC gating (the actual defect being fixed)
The previous decoder documented that callers "should also use the normal envelope CRC verification path" and then didn't. Its own shape test built an all-zero 2,140-byte buffer with three magic bytes set and decoded it successfully. Bad bytes could become data.
decodenow verifies the envelope and both checksums before reading a single payload field, per the BLE safety contract. Consequences, all intended:parseFrame's field annotation andWhoop5OpticalExperimentAnalyzernow skip corrupt records instead of annotating/analysing them.OpticalExperimentAnalysisTestsre-seals after it mutates a header byte.4. Readings: signed 20-bit, already sign-extended on the wire
The containers arrive sign-extended into 4 bytes, so a plain signed 32-bit read is the entire decode — no masking, no re-extension from bit 19. That is measured, not assumed: across all 4,380,450 populated containers in the corpus the fourth byte is only ever
0x00(4,057,474) or0xFF(322,976) and never anything else, and decoded values span[-148,044, 524,287].Worth stating because the wrong version is not obviously wrong from the numbers: masking to 20 bits and zero-extending, or reading the container unsigned, turns the real sample
-22,101(wireab a9 ff ff) into1,048,491or4,294,945,195. Both look like plausible ADC counts. A golden vector pins that exact value.The positive rail is exactly
2^19-1 = 524,287— hit and never exceeded — sosampleMin/sampleMaxare exposed and asserted rather than left as folklore.5. Tests
A shared
r20_optical_oracle.json, byte-identical inPackages/WhoopProtocol/Tests/WhoopProtocolTests/Resources/andandroid/app/src/test/resources/, same idiom asdecoder_oracle.json(#869, #889). Two real captured records; the expected values were generated by an independent third implementation of the layout (Tools/gen_r20_oracle.py, plainstruct.unpack+zlib.crc32), so Swift and Kotlin each have to reproduce them rather than agreeing with each other by construction. A test asserts the two copies stay identical, and the generator writes both at once.Covered on both platforms: full per-field decode of all five blocks; the
[25, 0, 0, 25, 25]pattern withsample_count == 0yielding no readings (not 50 zeros); block 3 confirmed as the drive-off dark control; negative readings sign-extending; saturation exactly at2^19-1; the CRC range asserted directly, including that the retracted[26:2136]does not match; and five single-bit corruptions — in a reading, insample_count, indrive_a, in the CRC trailer, and in the declared length — each of which must be rejected. The generator proves each mutation really does break a checksum, so the fixture can never assert "rejected" for a frame that is in fact valid.Results (both run on this branch's head):
swift testinPackages/WhoopProtocol: 422 tests, 0 failures, of which 10 areWhoop5RawOpticalTests../gradlew :app:testFullDebugUnitTest: 3187 tests, 1 failed, 5 skipped.Whoop5RawOpticalTestis 9 tests, 0 failures, 0 skipped, read out of the JUnit XML rather than trusted from the exit code.On that one Kotlin failure — it is pre-existing on main and unrelated to this PR.
com.noop.data.DeepCaptureMigrationTest.repositoryInsertV18Aux_insertsThenPrunesfails withexpected:<my-whoop> but was:<null>. #888 made the v18-aux retention sweep amortized —WhoopRepository.insertnow prunes only oncebanked >= V18_AUX_PRUNE_EVERY_ROWS(10,000) — but the test still inserts a single row and asserts the prune fired, so it cannot pass. Nothing incom.noop.datareferencesWhoop5RawOptical. It goes unnoticed because Kotlin unit tests don't run on PRs. Happy to send that as a separate one-line PR if you'd like it.6. Full-corpus verification
Both suites carry an opt-in test that runs the decoder over an entire deep-buffer capture and asserts the published invariants on every record — the block pattern,
drive_ainside its six-value set,drive_b == 2 * drive_a, offsets multiples of 800, reserved zero, reading counts matchingsample_count, every sample inside the signed 20-bit domain, and the decoded timestamp matching the recorder's ownstrap_ts.It skips when the variable is unset (the capture is far too large to commit), so CI is unaffected. Against the 29,203-record WHOOP 5.0/MG capture behind #423:
Two golden vectors prove the offsets; this proves they hold at scale, on both platforms, which is the claim #423 actually makes.
7. Scope, and what is deliberately not claimed
Not wired into storage or analytics. No production consumer reads any of these fields. Persistence and downstream analytics for this record are being designed separately; keeping them out means this can be reviewed as what it is — a byte-layout decoder with vectors.
Unchanged from #546, and restated in the source: the two slots under one head belong to one shared measurement configuration and must not be labelled as two wavelengths.
drive_*is named only because a block with both drive fields at zero produces a dark measurement with no pulse (block 3). Its units are not established, whatsource_*values{1,2,3,4}select is unknown, and nothing in this corpus identifies an emission band.Everything here was measured from NOOP's own BLE captures via the #454 recorder — offsets, widths, endianness, value sets and counts are all reproducible from the capture alone.
Thanks to @digitalerdude, whose public capture and v26 CRC-from-byte-8 result are what corroborate §2 across two frame sizes.
Refs #423.