Repository navigation
WhoopProtocol: model v20 (2140 B) optical record as a neutral 5-block structure - #546
Conversation
fd9513e to
53ce4cd
Compare
|
Reviewed, and this is a better model than what it replaces. The structural claim holds — I verified it independently against the real captured buffer rather than taking the description's word for it:
That last point is the argument for merging. The previous decode listed those ten constants; this explains them. It also dissolves the 25-vs-50 question rather than patching it — the slot is 50 wide and the count is in the record — which subsumes #545 properly. It resolves a real latent bug, too. Reviewing my own (now superseded) Android decoder I'd flagged that if the header byte is genuinely a sample count, then hard-coding 25 is wrong and a hypothetical VerificationThe Kotlin half needed running, because Android CI does not run it:
Swift and Kotlin decoders agree: same constants, same structure, and the count bounds are equivalent ( Follow-ups (none blocking)1. The 2. The deleted RE notes actually corroborate this model — worth restoring rather than dropping. The diff removes the paragraph citing the LED-current field @28 and per-photodiode offset DACs @38/@45 (and the "RE notes only, NOT authoritative" hedge + #423 pointer), while strengthening "the six active channels are INFERRED to be optical" into "The v20 record is optical". That direction is backwards on its own — but the substantive point is that those notes support you. Under this PR's own 21-byte split, block 0 sits at 26 with shared metadata @27–32, chan0's descriptor @33–39 and chan1's @40–46. So:
One drive current shared by the block, one offset DAC per channel — exactly what "one measurement configuration, two detector/readout channels" predicts. That's independent corroboration of the 3. Minor: On naming: Nice piece of reverse engineering — the stride model is the kind of thing that makes the next layout easier rather than just this one correct. |
…ce (#546 follow-up) Two points from the #546 review, now that it's merged. 1. Whoop5RawImu length-gate parity. #546 tightened the Swift decoder's gate from `>=` to `== bufferLength` so the 1244-B IMU and 2140-B optical decoders can't cross-misfire, but the Kotlin twin still read `f.size < bufferLength` (accepting anything >= 1244). Its own KDoc already claimed "exact length", so the code now matches both the doc and Swift: `f.size != bufferLength`. The only caller (PuffinDeepBufferLog) already gates exact length, so this is a no-op on real frames — it just closes the same cross-misfire the Swift change did. Also adds the Kotlin twin of #546's Swift over-length test (a valid buffer + 1 trailing byte must be rejected) — it pins the fix and fails on the old `<` gate. 2. Restore the RE evidence #546 dropped. The superseded decoder cited issue #423's config-header fields — an LED-current @28 and per-photodiode offset DACs @38/@45 — which #546 removed while strengthening "inferred to be optical" into "is optical". Those notes CORROBORATE the new paired-block model: under the 21-byte split, @28 falls in a block's shared bytes (@27..32) and @38/@45 fall in its two 7-byte channel descriptors (@33..39, @40..46) — one LED per block, one detector per channel, which aligns with "two readout channels share one measurement config". Re-added to both Whoop5RawOptical docs (Swift + Kotlin twin), hedged as a non-authoritative line of evidence for the detector/readout reading rather than two wavelengths. Not included: the `sensor_channel_samples` = max(block counts) naming point (review item 3). Tested (fully — pure package + Kotlin, no app-target/CI gap): swift test (WhoopProtocol) -> 289/289 ./gradlew testFullDebugUnitTest --tests "com.noop.protocol.*" -> 164/164
…ce (#546 follow-up) (#577) Two points from the #546 review, now that it's merged. 1. Whoop5RawImu length-gate parity. #546 tightened the Swift decoder's gate from `>=` to `== bufferLength` so the 1244-B IMU and 2140-B optical decoders can't cross-misfire, but the Kotlin twin still read `f.size < bufferLength` (accepting anything >= 1244). Its own KDoc already claimed "exact length", so the code now matches both the doc and Swift: `f.size != bufferLength`. The only caller (PuffinDeepBufferLog) already gates exact length, so this is a no-op on real frames — it just closes the same cross-misfire the Swift change did. Also adds the Kotlin twin of #546's Swift over-length test (a valid buffer + 1 trailing byte must be rejected) — it pins the fix and fails on the old `<` gate. 2. Restore the RE evidence #546 dropped. The superseded decoder cited issue #423's config-header fields — an LED-current @28 and per-photodiode offset DACs @38/@45 — which #546 removed while strengthening "inferred to be optical" into "is optical". Those notes CORROBORATE the new paired-block model: under the 21-byte split, @28 falls in a block's shared bytes (@27..32) and @38/@45 fall in its two 7-byte channel descriptors (@33..39, @40..46) — one LED per block, one detector per channel, which aligns with "two readout channels share one measurement config". Re-added to both Whoop5RawOptical docs (Swift + Kotlin twin), hedged as a non-authoritative line of evidence for the detector/readout reading rather than two wavelengths. Not included: the `sensor_channel_samples` = max(block counts) naming point (review item 3). Tested (fully — pure package + Kotlin, no app-target/CI gap): swift test (WhoopProtocol) -> 289/289 ./gradlew testFullDebugUnitTest --tests "com.noop.protocol.*" -> 164/164
…ce (ryanbr#546 follow-up) (ryanbr#577) Two points from the ryanbr#546 review, now that it's merged. 1. Whoop5RawImu length-gate parity. ryanbr#546 tightened the Swift decoder's gate from `>=` to `== bufferLength` so the 1244-B IMU and 2140-B optical decoders can't cross-misfire, but the Kotlin twin still read `f.size < bufferLength` (accepting anything >= 1244). Its own KDoc already claimed "exact length", so the code now matches both the doc and Swift: `f.size != bufferLength`. The only caller (PuffinDeepBufferLog) already gates exact length, so this is a no-op on real frames — it just closes the same cross-misfire the Swift change did. Also adds the Kotlin twin of ryanbr#546's Swift over-length test (a valid buffer + 1 trailing byte must be rejected) — it pins the fix and fails on the old `<` gate. 2. Restore the RE evidence ryanbr#546 dropped. The superseded decoder cited issue ryanbr#423's config-header fields — an LED-current @28 and per-photodiode offset DACs @38/@45 — which ryanbr#546 removed while strengthening "inferred to be optical" into "is optical". Those notes CORROBORATE the new paired-block model: under the 21-byte split, @28 falls in a block's shared bytes (@27..32) and @38/@45 fall in its two 7-byte channel descriptors (@33..39, @40..46) — one LED per block, one detector per channel, which aligns with "two readout channels share one measurement config". Re-added to both Whoop5RawOptical docs (Swift + Kotlin twin), hedged as a non-authoritative line of evidence for the detector/readout reading rather than two wavelengths. Not included: the `sensor_channel_samples` = max(block counts) naming point (review item 3). Tested (fully — pure package + Kotlin, no app-target/CI gap): swift test (WhoopProtocol) -> 289/289 ./gradlew testFullDebugUnitTest --tests "com.noop.protocol.*" -> 164/164
#546 gave the 2140-byte layout-v20 record a NEUTRAL structure — five 422-byte blocks, each with a 21-byte header, two 200-byte readout slots and a reserved byte — deliberately without names, because nothing in the bytes says red, IR, green or ambient. This adds the harness that could earn those names. Phase marking in Settings (Advanced > Experimental, shown only when frame recording is already on) appends one line to the existing puffin-deepbuffers.jsonl. An offline CLI (whoop-optical-experiment) plus OpticalExperimentAnalysis reads it back, attributes each v20 frame to a phase, and reports per-phase block activation, per-header-byte behaviour, per-channel ADC statistics and adjacent-phase deltas. It sends NOTHING to the strap, and that is structural rather than a policy: markWhoop5OpticalPhase reaches PuffinDeepBufferLog.appendOpticalPhase, which is a file append. No peripheral, no characteristic, no command path on that route — verified, not asserted. Attribution uses the strap timestamp inside each frame, never log-arrival time, because deep buffers land during the connect-time offload burst minutes after the seconds they describe. experiment_end is excluded from the phase list but retained in markers, so it bounds the final phase without becoming one — endUnixTs indexes the full marker list while phaseIndex searches only real phases. The frame.baseTs == strap_ts check gates a malformed envelope without substituting for frame content. The CLI drops the first 10 s after each marker by default; the library defaults to 0, which is the right split — a library should not silently discard data. The only modification to existing code is a clean extraction: the soft-cap rotation was lifted verbatim into a private append(_:) so the marker path shares it. Phase markers therefore inherit the existing rotation rather than growing the file unbounded. Everything else is additive. Claims no wavelength and computes no SpO2 or blood pressure. The questions it asks are the narrow answerable ones — which blocks respond to contact, darkness, room light, pressure, paced breathing; whether blocks 1 and 2 ever acquire; which header bytes move with which condition; whether the two readout slots behave as paired detectors. Those have to be settled before anyone can honestly claim a wavelength, let alone a saturation. Swift only, on purpose: the deep-buffer lane (#423) has no Android counterpart, so there is nothing there for a marker to mark. The JSONL and report format are the contract, not the Swift. i18n done properly. #850 landed hours earlier and put it/ru/zh-Hans/zh-Hant behind a ratcheting allowance; all four new strings are translated into all EIGHT locales with the allowance file untouched. Raising it would have passed CI just as green. Verified: 10/10 CI green. Not validated on hardware and cannot be — the harness exists precisely because nobody has a labelled capture yet. The app-target half is compiled and tested locally only, as app-build.yml is disabled.
…ce (ryanbr#546 follow-up) (ryanbr#577) Two points from the ryanbr#546 review, now that it's merged. 1. Whoop5RawImu length-gate parity. ryanbr#546 tightened the Swift decoder's gate from `>=` to `== bufferLength` so the 1244-B IMU and 2140-B optical decoders can't cross-misfire, but the Kotlin twin still read `f.size < bufferLength` (accepting anything >= 1244). Its own KDoc already claimed "exact length", so the code now matches both the doc and Swift: `f.size != bufferLength`. The only caller (PuffinDeepBufferLog) already gates exact length, so this is a no-op on real frames — it just closes the same cross-misfire the Swift change did. Also adds the Kotlin twin of ryanbr#546's Swift over-length test (a valid buffer + 1 trailing byte must be rejected) — it pins the fix and fails on the old `<` gate. 2. Restore the RE evidence ryanbr#546 dropped. The superseded decoder cited issue ryanbr#423's config-header fields — an LED-current @28 and per-photodiode offset DACs @38/@45 — which ryanbr#546 removed while strengthening "inferred to be optical" into "is optical". Those notes CORROBORATE the new paired-block model: under the 21-byte split, @28 falls in a block's shared bytes (@27..32) and @38/@45 fall in its two 7-byte channel descriptors (@33..39, @40..46) — one LED per block, one detector per channel, which aligns with "two readout channels share one measurement config". Re-added to both Whoop5RawOptical docs (Swift + Kotlin twin), hedged as a non-authoritative line of evidence for the detector/readout reading rather than two wavelengths. Not included: the `sensor_channel_samples` = max(block counts) naming point (review item 3). Tested (fully — pure package + Kotlin, no app-target/CI gap): swift test (WhoopProtocol) -> 289/289 ./gradlew testFullDebugUnitTest --tests "com.noop.protocol.*" -> 164/164
ryanbr#546 gave the 2140-byte layout-v20 record a NEUTRAL structure — five 422-byte blocks, each with a 21-byte header, two 200-byte readout slots and a reserved byte — deliberately without names, because nothing in the bytes says red, IR, green or ambient. This adds the harness that could earn those names. Phase marking in Settings (Advanced > Experimental, shown only when frame recording is already on) appends one line to the existing puffin-deepbuffers.jsonl. An offline CLI (whoop-optical-experiment) plus OpticalExperimentAnalysis reads it back, attributes each v20 frame to a phase, and reports per-phase block activation, per-header-byte behaviour, per-channel ADC statistics and adjacent-phase deltas. It sends NOTHING to the strap, and that is structural rather than a policy: markWhoop5OpticalPhase reaches PuffinDeepBufferLog.appendOpticalPhase, which is a file append. No peripheral, no characteristic, no command path on that route — verified, not asserted. Attribution uses the strap timestamp inside each frame, never log-arrival time, because deep buffers land during the connect-time offload burst minutes after the seconds they describe. experiment_end is excluded from the phase list but retained in markers, so it bounds the final phase without becoming one — endUnixTs indexes the full marker list while phaseIndex searches only real phases. The frame.baseTs == strap_ts check gates a malformed envelope without substituting for frame content. The CLI drops the first 10 s after each marker by default; the library defaults to 0, which is the right split — a library should not silently discard data. The only modification to existing code is a clean extraction: the soft-cap rotation was lifted verbatim into a private append(_:) so the marker path shares it. Phase markers therefore inherit the existing rotation rather than growing the file unbounded. Everything else is additive. Claims no wavelength and computes no SpO2 or blood pressure. The questions it asks are the narrow answerable ones — which blocks respond to contact, darkness, room light, pressure, paced breathing; whether blocks 1 and 2 ever acquire; which header bytes move with which condition; whether the two readout slots behave as paired detectors. Those have to be settled before anyone can honestly claim a wavelength, let alone a saturation. Swift only, on purpose: the deep-buffer lane (ryanbr#423) has no Android counterpart, so there is nothing there for a marker to mark. The JSONL and report format are the contract, not the Swift. i18n done properly. ryanbr#850 landed hours earlier and put it/ru/zh-Hans/zh-Hant behind a ratcheting allowance; all four new strings are translated into all EIGHT locales with the allowance file untouched. Raising it would have passed CI just as green. Verified: 10/10 CI green. Not validated on hardware and cannot be — the harness exists precisely because nobody has a labelled capture yet. The app-target half is compiled and tested locally only, as app-build.yml is disabled.
Replaces the inline v20 (2140 B) historical decode with a dedicated, neutral
Whoop5RawOpticaldecoder (Swift + Kotlin) that models the record's actual repeated structure. Supersedes #545 — it subsumes that 50→25 sample-count fix by reading the count from the record instead of hard-coding it.The structure (verified on all 29,203 captured records)
The v20 body is five repeated 422-byte measurement blocks at
26 + 422·i(26/448/870/1292/1714 — exactly filling[26:2136], where the CRC32 sits). Each block:Header byte 0 is the shared sample count for both slots. Block counts are always
[25, 0, 0, 25, 25]; active slots carry 25 sign-extended i32 readings (~25 Hz), slots 25–49 are zero padding, reserved bytes are zero. The 21-byte header splits[count:1][shared:6][chan0:7][chan1:7]and is preserved as raw metadata — no register meanings asserted.Why this replaces the old model
The previous decode treated the six active slots as six independent channels. They're really three measurement blocks of two slots each. This matters semantically: the two slots under one block header are a detector/readout pair for one measurement configuration, not two different LED wavelengths. The decoder therefore exposes neutral
channel_b<block>_<slot>names and asserts no color — a defensible model until a labeled optical experiment establishes wavelength identity. (Analog Devices' MAX86171 is a structural cross-check for "one measurement → a PPG1/PPG2 detector pair"; it is not claimed to be the actual silicon.)Scope
Instrumentation/diagnostics only, no production consumers —
extractHistoricalStreamsreads only v18 keys, and the raw 2140-B buffer is persisted separately. The decoder is lossless (reconstructs the full 21-byte header) and strictly gated (== 2140, envelope + inner type + version,sampleCount <= 50keeps every read in-bounds). Also tightensWhoop5RawImuto an exact-length gate (>=→==) so the two same-type decoders can't cross-misfire.Tests
WhoopProtocolsuite green (289 passing). Real captured-frame test asserts[25,0,0,25,25], the exact 21-byte headers, paired-channel sample counts, and that the interpreter exposes nored/irkeys; plus strict shape/count-gate tests.RE background for the structure is tracked in #423.