Oura: fix 0x5D hrv_event decode — it's (u8 hr_bpm, u8 rmssd_ms) 5-min pairs, not (u16 time, int8, int8)

[pipiche38]

Problem (related to issue #728)

NOOP decoded the 0x5D hrv_event as a 4-byte (u16 time_ms, int8 b1, int8 b2) stride and carried the fields raw (units-neutral, never fed to scoring). Cross-checking open_oura's decode_hrv and a real overnight capture shows the true layout is a run of (u8 avg HR bpm, u8 avg RMSSD ms) pairs, one per 5-min bucket. The old framing:

• garbled the first (hr, rmssd) byte-pair into a bogus time_ms,
• sign-flipped the RMSSD byte (int8, so 131 read as -125),
• and only b1 accidentally landed on a real HR byte.

Validation (real device)

On a captured 22-Jul night, NOOP's stored b1 values were 50, 52, 53, 52, 47, 54, 59, 52 — textbook sleeping HR, matching the #511 IBI-derived median of 54 bpm. Reconstructing the bogus time_ms (33842 = 0x8432) back to bytes gives [50, 132] — exactly the (hr, rmssd) of the first pair. So b1 was right by accident; time_ms and b2's sign were wrong.

Change (byte-identical Swift/Kotlin twins)

• OuraProtocol: OuraHRV now carries (index, hrBpm, rmssdMs); decodeHRV walks 2-byte unsigned (hr, rmssd) pairs and returns nil on an empty or odd-length body (never a partial pair). Golden tests use real overnight bytes (32 84 32 83) plus an odd-length-nil case. oura-decode CLI updated.
• WhoopStore / data: the OURA_HRV event payload is now honestly labelled hr_bpm / rmssd_ms / pair_index — the byte→unit scaling is pinned now, so the old "raw, no rmssd_ms" honesty caveat no longer applies. Keys/values identical on both platforms.
• docs/OURA_PROTOCOL.md §6.9 corrected, with the decode-correction note.

Scope

Pure decoder-correctness fix, one concern. It's also the precursor to #728: the ring's own overnight HR + RMSSD are now available honestly — feeding avgHrv directly and giving a ground-truth HR to validate #728's IBI-derived hrSample against.
(0x5D alone is too sparse — ~40–96 HR/night — to satisfy the pipeline's ≥200 HR gate, so #728 still needs the dense IBI-derived series.)

Verification

• swift test OuraProtocol + WhoopStore green (corrected 0x5D golden + mapping tests).
• macOS Strand BUILD SUCCEEDED; Android compileFullDebugKotlin OK.
• Android unit-test run is blocked only by the unrelated Skin Temp, Wrong Strap Model #716 test-fake gap (PR Android: unblock testFullDebugUnitTest — implement DeviceRegistryDao.setModel in the three fake DAOs #725); the Kotlin 0x5D tests mirror the passing Swift ones and run once Android: unblock testFullDebugUnitTest — implement DeviceRegistryDao.setModel in the three fake DAOs #725 lands.

[ryanbr]

Reviewed. The decode fix is correct and well-evidenced — I re-derived your reasoning independently rather than taking it on trust:

• 0x8432 = 33842, and little-endian bytes [0x32, 0x84] = (50, 132), so the bogus time_ms really was the first (hr, rmssd) pair.
• 131 as int8 is -125, matching the sign-flip you describe.
• The mechanism is visible in the old code: i8(v) = v.toByte().toInt() applied to a payload that is an IntArray of unsigned 0–255 values, so any RMSSD above 127 went negative. Both your golden values (132, 131) are in that range.
• b1 landing on a real HR byte was indeed accidental, and 50–54 bpm is consistent with the fix(oura): correct 0x60/0x80 IBI decoders to the ring's real byte layout #511 IBI-derived median.

Parity is genuine. I suspected a signedness divergence at first — Swift does Int(b[i]) while Kotlin does a bare b[i] — but OuraRecord.payload is an IntArray holding unsigned bytes, so both sides yield 0–255. Correct on both platforms, and the odd-length-nil guard matches.

Two things before merge.

1. Stale header docs, both platforms

The inline comments were updated but the file-level contract docs still describe the old layout:

• Packages/WhoopStore/Sources/WhoopStore/OuraStreamMapping.swift:38 — .hrv (0x5D HRV tag, raw int8 b1/b2)``
• android/app/src/main/java/com/noop/data/OuraStreamMapping.kt:21-23 — "carrying ITS RAW decoded fields (time_ms/b1/b2) ONLY, never a fabricated rmssd_ms (the int8 b1/b2 byte->ms scale is not Tier-A…)"

The Kotlin one now contradicts the code it documents: it forbids exactly the rmssd_ms the mapping emits. A reader following it would conclude the change violates the honest-data invariant, when in fact the invariant is satisfied because the scaling is now pinned. Worth correcting symmetrically, since these are the docs someone reads instead of the code.

2. Only one 5-minute bucket per record actually persists

@Entity(tableName = "event", primaryKeys = ["deviceId", "ts", "kind"]), and the mapping gives every pair the same ts from anchor(ringTimestamp) — the bucket is distinguished only by pair_index inside the payload, which is not part of the key. So N pairs from one record collide and all but one are dropped on insert.

This is pre-existing (the old code also emitted N events at one ts), so it is not a regression. But the reframing makes it consequential: the payload now presents itself as the ring's per-5-min HR/RMSSD series, and the PR body offers it as the precursor to #728 with "the ring's own overnight HR + RMSSD are now available honestly". As stored, only one bucket per record is retained, so that reads as more than is kept.

Your own comment anticipates the gap — "buckets are ~5 min apart (consumer applies the offset)" — but no consumer applies it, and the sleepPhase case a few lines below in the same file shows why it has to happen before the event is minted:

30 s spacing makes every code a distinct (deviceId, ts, kind) row; the earlier provisional ts + index offset is gone (it would double-shift reconstructed codes). The raw 2-bit code persists unchanged; index (position within the wire record) is kept for audit.

So sleep phases get distinct timestamps from OuraHypnogramAssembler upstream of the mapping, and index is explicitly audit-only. HRV has no equivalent, so pair_index cannot do the job the payload implies.

Either is fine by me:

• apply the 5-minute offset at mapping time so each bucket is its own row (closest to what the payload now claims, and what Live Oura ring: overnight vitals (skin temp / RHR / HRV) never surface — banked IBI is stored but no hrSample is derived, so the nightly pipeline scores the night NULL #728 would want), or
• keep it as-is and say plainly in the doc comment that only one bucket per record survives storage, so nobody builds on a series that is not there.

Minor

Your caveat about the Android unit-test run is still accurate — #725 is open, so the Kotlin 0x5D tests cannot run yet. No objection to landing ahead of it given the Swift twins pass and the Kotlin code is a line-for-line mirror; worth re-running once #725 merges.

Nice piece of work — the validation against a real overnight, with the arithmetic shown, is what made this quick to check.

[ryanbr]

Anchoring the two findings from my summary comment to the lines they concern, so they are actionable in place. The decode fix itself is correct — verified independently — and neither of these blocks the layout correction.