Skip to content

burst_index and layout_marker are decoded and then destroyed — the only two decoder fields with no banked home (extends #845 to v26/v20-21) #979

Closed

Description

@vishk23

burst_index (v26) and layout_marker (v20/21) are decoded on every matching frame, persisted nowhere, and not reconstructable from anything that is persisted — so they're destroyed when the strap frees history on offload-ack.

This is the same argument #845 makes for the v18 fields, applied to the two frame families #845 doesn't cover. I checked all 44 keys the Interpreter emits into parsed against every production consumer on both platforms; these are the only two with no banked home.

Why these two are different from the other unread fields

Most unread decoder output is recoverable, which is why it's fine:

Key Producer Recoverable from
spo2_candidate_82 Interpreter.swift:525 banked aux_byte_82 (Streams.swift:381), and Streams.swift:401-403 says so explicitly
onwrist Interpreter.swift:495 nibble of @81; whole byte banked as sleep_state_byte → sleepStateSample.rawByte
wake_quality Interpreter.swift:497 same byte
motion_wear_quality Interpreter.swift:442 same byte as activity_class, which is persisted (Streams.swift:399)

These two aren't:

  • burst_index — Interpreter.swift:609, fb.add(21, 1, "burst_index", "ppg", …). v26 isn't v18, so there's no V18AuxSlot for it, and ppgWaveformSample stores only (ts, samples). Production readers: 0. Only references are Whoop5PpgWaveformTests.swift:49,70.
  • layout_marker — Interpreter.swift:666, fb.add(10, 1, "layout_marker", "meta", …). The v20/v21 layout discriminator (0x80 vs 0x81 in the fixtures). Production readers: 0. Only references are Whoop5HistoricalV2021Tests.swift:67,131,159.

Both surface in the whoop-decode CLI, which iterates parsed.fields (Packages/WhoopProtocol/Sources/whoop-decode/main.swift:211) — useful for inspecting a capture you already have, but it doesn't make the app's path live.

Parity note

Neither has a Kotlin twin. burst_index appears in android/.../protocol/HistoricalStreams.kt:459 only inside a comment explaining what @21 is; nothing decodes it. layout_marker has no Kotlin reference at all. So a v26/v20 capture from an Android user is missing both fields even in the frame inspector.

Why it might be worth two lines

burst_index is the per-burst counter (PR#553) — the thing that tells you whether two v26 frames belong to the same 40 s burst or different ones. Whoop5PpgWaveformTests.swift:56 uses exactly that to distinguish two bursts ~19 min apart. Any future work over ppgWaveformSample (a better HR estimator, HRV-from-PPG — the reasons Database.swift:592-606 gives for keeping those rows at all) has to segment by burst, and right now the rows it would run over don't carry the burst boundary. That seems like the one field where "we kept the samples but dropped the thing that groups them" actually bites.

layout_marker is cheaper to argue about — one byte, and the layout is re-derivable from the frame if you still have the frame. But you don't: the strap frees it.

Suggested fix

Either add two slots alongside the existing aux banking, or drop them from the decoder so the inventory in #845 stays honest about what's kept. I'd do burst_index and leave layout_marker, but I don't have a strong view. Happy to send a PR for whichever.

Two small things noticed alongside, not worth their own issues:

  • Packages/WhoopProtocol/Tests/WhoopProtocolTests/DeepCaptureChannelsTests.swift:285-292 names its set persistedElsewhere and asserts each slot "already has a durable home". That's not true for spo2_candidate_82 or motion_wear_quality — both are derivable from a banked byte, neither is persisted. The behaviour is right, only the label overstates; it briefly convinced me SpO2 was being stored. recoverableFromABankedField would read truer.
  • Tools/ and tools/ are two distinct trees at the repo root (git ls-tree main Tools/linux-capture/ returns nothing; the linux-capture files are under lowercase tools/). On a case-insensitive macOS checkout they merge into one visible directory, so a macOS contributor can add a file to what they see as Tools/linux-capture/ and have it recorded under the other tree. Nothing is broken today — docs consistently say lowercase — but it's a trap worth a normalization commit.

Environment: fork of main at af44563c; all file:line refs against that tree.

Activity

  1. ryanbr commented on Aug 1, 2026

    @ryanbr
    Owner

    Checked every reference against current main — your fork's line numbers still land exactly, and all of it holds:

    • burst_index at Interpreter.swift:609, layout_marker at :666, zero production readers on either platform.
    • The recoverability table is right, and Streams.swift:401-404 already says so in its own words: motion_wear_quality@63 "is the same byte as activity_class", spo2_candidate_82 "is a gated 70-100 view of auxByte82 and is recoverable from the raw byte". SleepStateSample.rawByte carries the whole @81 byte, which covers onwrist and wake_quality.
    • PpgWaveformSample really is just (ts, samples) — no slot to put a burst id in.
    • Kotlin: burst_index appears only inside a comment at HistoricalStreams.kt:459; layout_marker has no Kotlin reference at all.

    burst_index — agreed, and the strongest argument is one the code already makes. Database.swift's v27 migration note says the waveform table exists so "a BETTER estimator, HRV-from-PPG, or a waveform viewer can later run over the ORIGINAL samples rather than the derived bpm", and tells the next person not to clean the reader up as dead code because "the rows are the point".

    We added a migration, a new table and a packed BLOB specifically to keep those samples for future segmentation work — and then drop the one byte that says where the bursts begin. Every use that table names has to segment by burst first. Please do send that PR.

    layout_marker — I'd leave it too, for a reason slightly different from yours. Its information content is not really destroyed: the record decoded correctly under a chosen layout, so which layout it was is implied by the banked fields themselves. What's lost is forensic — "which marker did this record carry" — which only matters when chasing a layout bug, and then you want the frame anyway.

    But I'd rather not drop it from the decoder either: it earns its keep in whoop-decode when inspecting a capture you still have. So the third option is to keep decoding it, not bank it, and say in the inventory that it is deliberately unbanked and why. That keeps #845 honest without losing the inspector.

    Both side notes are right, and one is worse than you flagged.

    persistedElsewhere in DeepCaptureChannelsTests.swift — correct, and recoverableFromABankedField does read truer. Two of the seventeen entries (spo2_candidate_82, motion_wear_quality) are derivable from a banked byte rather than stored, and the assertion message asserting each "already has a durable home" is what misleads. The behaviour is fine; only the label lies.

    Tools/ vs tools/ — confirmed as two distinct trees, 50 files and 27 files, with linux-capture under the lowercase one. I checked for actual colliding paths and there are none, so your "nothing is broken today" is right rather than lucky. Worth adding that CI itself runs on macos-15, so every CI checkout is already merging 77 files into one visible directory — the trap is live on the machine that validates the repo, not just on a contributor's laptop. A normalization commit is worth doing on its own.

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions