fix(gen5): SET_CLOCK revision byte + honest v18 decode - #188
Conversation
Protocol's strict gravity gate rejected every v18 record in a hardware export (fw 50.40.1.0) while v20/v26 deep buffers dominated offload. Recover HR/RR via a hardware fallback with alternate unix offset and honest absent accel when gravity fails validation.
Drop the misaligned unix@6 fallback that could invent timestamps, and prepend revision1 on gen5 SET_CLOCK/GET_CLOCK only (WHOOP 4 path unchanged). Hardware showed Invalid revision when body[0] was the epoch low byte; a later connect with the 9-byte form correlated with drift=0.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughGen5 v18 historical records now support lenient timestamp, heart-rate, RR, and gravity decoding. Gen5 clock commands use revision-prefixed payloads. One-Hz persistence rejects unsupported R10-lite records and preserves valid Gen4 and Gen5 samples. ChangesGen5 v18 BLE and persistence support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Gen5HistoricalIngestion
participant decodeGen5HistoricalSample
participant StrictDecoder
participant sampleFromGen5V18Lenient
Gen5HistoricalIngestion->>decodeGen5HistoricalSample: historical record and wall-clock timestamp
decodeGen5HistoricalSample->>StrictDecoder: strict historical decode
StrictDecoder-->>decodeGen5HistoricalSample: sample or failure
decodeGen5HistoricalSample->>sampleFromGen5V18Lenient: revision 18 fallback
sampleFromGen5V18Lenient-->>Gen5HistoricalIngestion: lenient sample or null
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Updates the Gen5 (WHOOP 5) BLE clock and v18 historical decode paths in edge’s BLE ingestion layer to match observed hardware behavior and avoid timestamp fabrication, while keeping Gen4 behavior unchanged.
Changes:
- Add Gen5
SET_CLOCK/GET_CLOCKpayload builders that prependrevision1, and use them inBleEngine.setClock()/getClock(). - Add a “honest” lenient v18 historical decode path that only recovers HR/RR when
unix@7is plausible, and never falls back to a misaligned timestamp read. - Add unit tests covering the revision-prefixed clock payloads and the lenient v18 decode behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| lib/ble/ble_engine.dart | Adds Gen5 clock revision-prefixed payloads and a lenient-but-honest v18 decode fallback, wired into Gen5 historical ingestion. |
| test/gen5_v18_hardware_lenient_test.dart | Adds tests pinning the Gen5 clock payload revision byte and v18 timestamp/lenient-decode behavior against captured evidence. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/ble/ble_engine.dart`:
- Around line 87-90: Replace the fixed 2030 upper bound in gen5V18UnixPlausible
with a rolling wall-time-aware future limit while preserving the existing lower
bound. Add or update boundary coverage to verify the current-time-based cutoff
without relying on a date that will become invalid after January 1, 2030.
In `@test/gen5_v18_hardware_lenient_test.dart`:
- Around line 65-91: Update the success fixture in the test around
sampleFromGen5V18Lenient to exercise production ingestion through
decodeGen5HistoricalSample instead, asserting the expected recovered sample. Set
a nonzero valid RR interval, and adjust the sensor values so the fixture is
rejected specifically by the dynamic-acceleration gate while remaining otherwise
valid; preserve the expected timestamp, heart rate, and null acceleration
assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b4208874-445f-4946-a211-acddbd916641
📒 Files selected for processing (2)
lib/ble/ble_engine.darttest/gen5_v18_hardware_lenient_test.dart
|
@Brackyt Nice work on the hardware bring-up — the v18 unix@7 call and the clock fix are both solid. Couple of things before this can land: Could you move the v26 PPG→HR into its own PR? The v18 + clock commits were clean and green on their own, and this needs an analytics PR first anyway — Build:
For the PPG→HR PR, when you get to it:
Still open from last round:
Happy to help with any of these if useful. |
Gen5 v18/lenient samples lack gen4 optics so R24 decode fails and decoded_onehz stayed empty — fall back to the BLE-preferred Sample when tsEpoch > 0, excluding gen4 R10-lite hr-only records. Align gen5V18UnixFromInner with RecordGate's isPlausibleUnix(wallNow) so implausible timestamps archive instead of silently dropping after decode. Restore git-sourced analytics lock (cbbe06a).
d692d07 to
0ab4e4a
Compare
|
Thanks for the thorough review — agreed on the split. Updated the branch: This PR (#188) now contains only:
Stripped from this branch (was
Follow-up PRs (opening separately):
Will note in the edge PPG PR that analytics must merge first (pin to OpenStrap/analytics SHA — CI won't resolve until then). Happy to iterate on any of the deferred items. |
|
I kinda let my AI agent do the 3 MRs and didn't have time to check it all. Might have some issue. I'm currently testing all of it and fix issues I find |
Parse raw hex and gate Gen4 R10-lite records before accepting a complete preferred sample into decoded_onehz. Exercise lenient v18 recovery through decodeGen5HistoricalSample with HR+RR in hardware tests.
Cover strict dynamic-accel rejection with valid gravity through the production decodeGen5HistoricalSample path.
Strict parseGen5Historical accepts high dynamic accel when gravity is valid; lenient fallback is exercised via the gravity-gate case only.
Prove _decodeOneHzSample rejects Gen4 R10-lite bytes before the hasDecodedOneHz early return, not only when preferred is HR-only.
|
So the v26 PPG stuff was created because I thought the whoop wasn't giving usable data. That was dumb. Either way, we have these 2 other MRs that could be revisited later if needed (I don't think so, but i kept them in draft in case). I'll test tonight if the v18 stuff works great for sleep/readiness and give results. Known remaining areas:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/gen5_decoded_onehz_persistence_test.dart`:
- Line 46: Update the assignment to inner[15] so the clamped rrMs.length value
is explicitly converted to an int before assigning it as a byte.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ee64f30b-3974-42d8-9a64-50e030304c77
📒 Files selected for processing (4)
lib/ble/ble_engine.dartlib/data/db.darttest/gen5_decoded_onehz_persistence_test.darttest/gen5_v18_hardware_lenient_test.dart
R10-lite reject must parse hex first, but hexToBytes failure should fall through to preferred/hasDecodedOneHz — not return null. Restores decoded_onehz for fixture inserts with placeholder hex. Also fix clamp→int in the gen5 persistence helper.
|
CI fix pushed: All 14
Gen5 suites were already green; the failing suites now pass locally. Waiting on Actions re-run. |
P0. `sampleFromGen5V18Lenient` correctly abstains on a gravity vector that
fails the magnitude gate (ax/ay/az stay null) while keeping HR/RR -- but
`decoded_onehz.ax/ay/az` are REAL NOT NULL, so `_queueDecodedOneHz` writes
`decoded.ax ?? 0` and the substrate loader reads `?? 0` back. "We did not
measure this" becomes "the wrist was at exactly (0,0,0)".
That is not an inert default. `zAngle(0,0,0)` is exactly 0.0 in Dart --
atan2(0,0) is 0.0, not NaN -- so a run of absent seconds has a perfectly
CONSTANT z-angle, which is the van Hees immobility criterion satisfied
maximally. Measured against the pinned analytics: 8 h of (0,0,0) scores 28501
immobile seconds and `vanHeesSleepWindow.present == true`, i.e. a fabricated
~7.9 h night, fully staged, out of data that does not exist. This PR's own
commit message notes the strict gate rejected EVERY v18 record on fw
50.40.1.0, so for that firmware it is the ordinary case, not a corner.
Exact (0,0,0) is an unambiguous ABSENT marker rather than a reading: gravity
always has magnitude ~1 g and every decoder that emits a vector gates on
`magSq >= 0.25`. So `Substrate.accelPresentAt` / `accelPresentFraction` can
recover the distinction the NOT NULL column erased, with no schema change --
and it heals rows already persisted.
TWO OTHER SENTINELS WERE TRIED AND REJECTED, both measured rather than assumed:
* NaN fails OPEN. The rule asks "did the angle change by >= threshold", and
every comparison against NaN is false, so it never trips -- NaN scores the
SAME 28501 immobile seconds as zeros. Pinned in a test so nobody "fixes"
this that way later.
* Omitting the seconds does not work either: `immobilityMask` is a pure
index-wise angle rule with no timestamp/gap awareness (unlike `nap.dart`'s
`stillAt`, which does check `absAt(k) - absAt(k-1) == 1`), so it just joins
across the hole.
Since analytics has no validity input to be told any of this, the honest move
at this layer is not to let absent accel ANCHOR a window: below
`kMinAccelCoverageForVanHees` the accel-led path is skipped entirely and the
day falls through to the EXISTING HR-led fallback, which is already the
low-confidence degraded mode for exactly this situation. Half rather than
something tiny, because van Hees picks the LONGEST immobile block and absent
seconds are maximally immobile, so a mostly-absent window would reliably hand
the answer to the missing data.
The complete fix belongs upstream -- `immobilityMask` should take an optional
validity mask and mark invalid seconds via the `immobileUnknown` machinery it
already has. Noted for analytics; this keeps the P0 off the field meanwhile.
P1, same commit: lenient records bypassed `raw_archive`. A partial decode is
not a full decode -- HR/RR are kept but the gravity bytes the strict gate
rejected were discarded the moment we ACK'd the trim, with `raw_records` gone
and `decoded_onehz` having nowhere to put a null accel. They are now archived
as well as sampled, in the same safe-trim transaction, so a future decoder can
recover what this one could not. Extracted `_archiveHistoricalFrame` so both
the undecodable and partial paths share one implementation.
7 tests added; the 6 pre-existing failures in notification_dedupe_test also
reproduce on origin/main unmodified and are unrelated.
|
Reviewed this and pushed one commit ( P0 — absent gravity is read back as perfect stillness
That is not an inert default: A constant z-angle is the van Hees immobility criterion satisfied maximally. Measured against the pinned analytics — 8 h of A fabricated, fully-staged night out of data that does not exist. Your own commit message notes the strict gate rejected every v18 record on fw 50.40.1.0 — so on that firmware this is the ordinary case, not a corner. Two fixes I tried and rejected — measured, not assumedWorth recording, because both look obviously correct on paper:
Both are pinned in tests so nobody "fixes" it that way later. What I did insteadExact Since analytics has no validity input to be told any of this, the honest move at this layer is not to let absent accel anchor a window: below The complete fix belongs upstream: P1 — lenient records bypassed
|
…ats)
P0. The measured-vs-derived guard was per-CONNECTION and evaporated at exactly
the moment it was needed.
`_gen5MeasuredRecTs` is an in-memory Set, cleared in `_teardownSession` and
never seeded from `decoded_onehz`. After a reconnect it is empty while the rows
are still on disk, so a re-delivered v26 burst derives an HR for a second that
already has a measured v18 row. `decoded_onehz` is INSERT-OR-REPLACE on
UNIQUE(rec_ts), so the derived row WINS -- and `_queueOrphanGuard` then deletes
the evicted counter's `decoded_rr` beats. A derived sample carries no beats of
its own, so the net trade is a measured HR plus a whole second of beat-to-beat
intervals for an inferred bpm. `decoded_rr` is the durable RR store; that is
irrecoverable.
Seeding the set from the DB on connect would not really fix it -- the set would
have to hold every second ever measured. Provenance belongs on the datum, so:
* `Sample.derived` marks an INFERRED HR (today only the v26 PPG ACF path).
* `_queueDecodedOneHz` honours it: derived rows use INSERT OR IGNORE and SKIP
the orphan guard, so an existing row for that second simply stands. Nothing
is evicted, so nothing is stranded.
Measured-vs-measured is deliberately untouched -- "newest wins" is still right
there (the strap counter resets on reboot), and a test pins that.
This also de-fangs the u16-index-as-global-PK concern: a v26 burst index that
collides with a real counter now loses the insert instead of overwriting a
measured row.
The vacuous test, confirmed and replaced. `decodeGen5HistoricalSample --
measured v18 clobber guard` passed with the guard line DELETED, because no
`ppgBuf` was supplied so the derived path abstained for want of samples no
matter what the guard did. It now primes a buffer with an ACF-resolvable
waveform and asserts BOTH directions: the derived path is genuinely reachable
for that fixture when the second is unclaimed, and abstains when it is claimed.
Re-ran the mutation afterwards -- it now fails, as it should.
Not addressed here, deliberately: absent gravity persisted as `?? 0`. That is
the same seam as OpenStrap#188 and is fixed there (`Substrate.accelPresentAt`); doing it
again here would just conflict.
5 tests added/reworked, each mutation-verified. Suite 1076 passing; the 6
failures in notification_dedupe_test are pre-existing and reproduce on
origin/main unmodified.
|
Great find and fix |
|
Thanks @Brackyt. One thing worth calling out from your own list of known issues:
You had this exactly right, and it's more serious than "potentially". Measured against the pinned analytics, 8 h of That's On the rest of your list, for whatever it's worth:
CodeRabbit's two findings on this PR (the 2030 cutoff, and exercising the strict→lenient path through |
|
Correction to my earlier note on this PR, and it's good news. I described the 6 They are a time bomb, not a standing breakage. The suite builds date-prefixed dedupe keys from a hardcoded
Proved it by substituting today's date into the unmodified file on Practical impact here: this PR's CI cannot go green until #207 merges, regardless of its own content. Sorry for the noise — "pre-existing and unrelated" was accurate but undersold that it was actively blocking you. |
|
Thanks @abdulsaheel, agree on the zero-accel → van Hees fabrication; Quick update on the other items from that earlier known-issues list (they’ve moved to separate PRs, so “not wired” is outdated on those):
On the |
Summary
Hardware bring-up against a real WHOOP 5 (fw 50.40.1.0, Android) from discussion #178.
Scope: v18 decode + clock + db persistence only. PPG/v26 ingest is deferred to draft follow-up PRs (analytics #37 + edge #190) — real-hardware validation of the new PPG algorithm failed (see below).
What landed (WHOOP 4 clock path unchanged):
SET_CLOCK/GET_CLOCKprependrevision1so the strap no longer treats the epoch low byte as the command revision (Invalid revisionon console).decoded_onehzpersistence fix: gen5 v18 samples now fall back to the BLE-preferredSamplewhen R24 decode fails (excluding gen4 R10-lite hr-only records). This was the root cause of empty substrate on device.RecordGate:gen5V18UnixFromInnernow usesisPlausibleUnix(unix, wallNow)— implausible timestamps abstain at decode →raw_archive, not silent post-decode drop.cbbe06aon OpenStrap/analytics;pubspec.lockis git-sourced (nopath:override).Hardware evidence (fw 50.40.1.0, ~12.3 min wear)
decoded_onehzdecoded_rrNo sleep/readiness claim from ~14 minutes of wear. Lenient v18 null acceleration is currently stored as
(0,0,0)indecoded_onehz— correcting that requires an absence representation, derivation handling, and an algo-version bump (tracked separately; this PR does not claim sleep/motion correctness).Deferred (draft research PRs — do not merge)
Test plan
flutter test test/gen5_v18_hardware_lenient_test.dart test/gen5_decoded_onehz_persistence_test.dart test/gen5_sample_mapping_test.dartdart analyzeon changed filespubspec.lock—openstrap_analyticsissource: git+ OpenStrap URL +cbbe06adecoded_onehz9→741,decoded_rr0→167 over ~12.3 min9B, rev=1; reconnect showedClock correlated … drift=0Invalid revisionfor0x92after HELLOSummary by CodeRabbit
New Features
Bug Fixes