feat(gen5): v26 PPG-derived HR ingest (algo v52) - #190
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.
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).
Wire gen5 v26 PPG bursts into Sample ingest via analytics ACF HR (b3e7b88): adjacent 12-burst buffer, u16@3 counter for v26, always archive v26 hex, never clobber measured v18 at the same rec_ts.
|
Important Review skippedDraft detected. 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:
📝 WalkthroughWalkthroughThe BLE engine adds Gen5 v18 lenient decoding, Gen5 v26 PPG-derived heart rate, generation-aware clock commands, archival, and connection-state cleanup. Database persistence excludes Gen4 R10-lite records from ChangesGen5 historical decoding and derivation
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant BLEHistoricalDrain
participant BleEngine
participant Analytics
participant ArchiveSink
participant Database
BLEHistoricalDrain->>BleEngine: ingest Gen5 historical frame
BleEngine->>Analytics: derive HR from concatenated v26 PPG samples
Analytics-->>BleEngine: return HR or abstain
BleEngine->>ArchiveSink: archive v26 PPG hex
BleEngine->>Database: persist decoded sample
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
Regenerate pubspec.lock without pubspec_overrides so CI resolves openstrap_analytics from git (b3e7b88), not path.
There was a problem hiding this comment.
🟡 Not ready to approve
A newly added test contains a Dart compile error, and the v18 clobber guard appears connection-scoped which can still allow v26-derived samples to overwrite existing measured rows in decoded_onehz across reconnections.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Adds Gen5 (WHOOP 5) support for deriving per-second HR from v26 PPG bursts and persisting the resulting 1 Hz substrate, while preserving raw archival and re-deriving analytics via an algo version bump.
Changes:
- Wire Gen5 v26 PPG bursts into historical ingest with a rolling adjacency-gated burst buffer and ACF-based derived HR (RR intentionally empty).
- Adjust decoded substrate persistence so Gen5 v18/lenient samples (no Gen4 optics) can still land in
decoded_onehz, while keeping Gen4 R10-lite excluded. - Repin
openstrap_analyticsto the commit that addsderiveHrFromGen5PpgWaveform, bumpkAlgoVersionto 52, and expand/restore Gen5 mapping + persistence tests.
File summaries
| File | Description |
|---|---|
| test/gen5_v18_hardware_lenient_test.dart | Adds hardware-evidence regression tests for Gen5 clock payload revisions and strict-vs-lenient v18 unix handling. |
| test/gen5_sample_mapping_test.dart | Expands mapping tests for Gen5 v20/v21 null mapping, v26 PPG→derived HR behavior, burst-buffer adjacency rules, and measured-v18 clobber guard. |
| test/gen5_decoded_onehz_persistence_test.dart | Adds DB-level persistence tests ensuring Gen5 v18 samples persist into decoded_onehz via preferred fallback and keeping R10-lite excluded. |
| pubspec.yaml | Repins openstrap_analytics to the commit that introduces v26 PPG HR derivation. |
| pubspec.lock | Updates the resolved analytics git ref to match the new pin. |
| lib/data/db.dart | Updates _decodeOneHzSample to allow preferred Gen5 v18/lenient samples to populate decoded_onehz even without full optics, while excluding Gen4 R10-lite. |
| lib/compute/derivation_engine.dart | Bumps kAlgoVersion to 52 with changelog notes tied to the new Gen5 v26-derived HR substrate. |
| lib/ble/ble_engine.dart | Implements Gen5 v26 PPG burst buffering + derived HR sample mapping, always-archive behavior for v26, and Gen5 clock payload revision handling. |
Review details
- Files reviewed: 7/8 changed files
- Comments generated: 3
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| import 'package:openstrap_analytics/onehz.dart' | ||
| show deriveHrFromGen5PpgWaveform, kGen5PpgHrMinSamples; |
There was a problem hiding this comment.
Fixed in 63c40f0 — the unused kGen5PpgHrMinSamples import was removed. This PR is now Draft pending hardware validation of the PPG algorithm.
| /// Seconds that already have a measured v18 sample this connection — PPG | ||
| /// derivation must not REPLACE those rows in `decoded_onehz`. | ||
| final Set<int> _gen5MeasuredRecTs = <int>{}; |
There was a problem hiding this comment.
Valid future work. _gen5MeasuredRecTs is in-session only; durable measured-v18 precedence across reconnects needs a persisted guard (e.g. query decoded_onehz before emitting PPG Sample). Deferred — this PR is Draft until PPG HR passes hardware validation.
| final raw = RawRecord( | ||
| counter: counter, | ||
| packetType: PacketType.historicalData, | ||
| hex: '2f18' '00' * 20, |
There was a problem hiding this comment.
Not a compile error under this repo's SDK: '2f18' '00' * 20 is valid Dart string repetition and dart analyze passes on the test file. Stale reviewer claim.
Use the analytics min-window constant so the import is load-bearing and thin burst concatenations abstain before ACF.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 205-214: Update the burst adjacency logic in
lib/ble/ble_engine.dart lines 205-214 to accept only the expected next
burstIndex within the same second, while allowing only the protocol-defined
reset transition at the next second; clear the buffer for skipped or invalid
transitions. Add regression cases in test/gen5_sample_mapping_test.dart lines
156-178 covering a skipped index and an invalid next-second index transition.
- Around line 309-317: Preserve measured HR precedence across reconnects by
enforcing the measured-row check during durable persistence, not solely through
the connection-local set in the Gen5 decoder around sampleFromGen5PpgWaveform.
Update the persistence path around the teardown/set handling at
lib/ble/ble_engine.dart lines 951-953 so reconnects consult durable measured
records and never replace them with derived PPG HR. In
test/gen5_sample_mapping_test.dart lines 180-198, supply at least
kGen5PpgHrMinSamples, verify derivation succeeds without the measured timestamp,
then verify the guard abstains; add persisted-row/reconnect coverage including
idempotence and synchronization behavior.
- Around line 2179-2193: Update the isGen5V26 handling in the PPG derivation
flow so archive-only records use a separate historical ingestion method that
increments records and recordsThisOffload before returning when sample is null.
Keep onHistoricalArchive for v26 archives paired with a stored sample, and apply
the same distinction at the corresponding alternate flow.
In `@lib/data/db.dart`:
- Around line 1868-1902: Update _decodeOneHzSample to decode raw.hex before
checking preferred, returning null immediately when
_isGen4R10LiteHistorical(bytes) is true. Only then allow a complete preferred
sample to return, while preserving the existing decoder and fallback behavior
for non-R10-lite records. Add a regression test covering an R10-lite raw record
with a complete preferred sample and verifying it is rejected.
In `@test/gen5_decoded_onehz_persistence_test.dart`:
- Line 46: Update the assignment to inner[15] so it uses an integer-valued
expression instead of int.clamp, preserving the 0–4 bounds and satisfying
Uint8List’s integer element type.
🪄 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: 95503003-bd4a-4eef-8e9e-d2773faa6d1f
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
lib/ble/ble_engine.dartlib/compute/derivation_engine.dartlib/data/db.dartpubspec.yamltest/gen5_decoded_onehz_persistence_test.darttest/gen5_sample_mapping_test.darttest/gen5_v18_hardware_lenient_test.dart
| if (parsed is Gen5PpgWaveform) { | ||
| ppgBuf?.add( | ||
| unix: parsed.unix, | ||
| burstIndex: parsed.burstIndex, | ||
| wave: parsed.ppgWaveform, | ||
| ); | ||
| if (measuredRecTs?.contains(parsed.unix) ?? false) return null; | ||
| final samples = ppgBuf?.concatenated() ?? parsed.ppgWaveform; | ||
| return sampleFromGen5PpgWaveform(parsed, samples); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve measured HR across connection boundaries.
_gen5MeasuredRecTs protects only measured samples decoded during the current connection. Teardown clears the set. A later connection can therefore persist derived PPG HR at a timestamp that already contains measured v18 HR. The decoded store uses replacement by rec_ts, so this can overwrite the measured row.
lib/ble/ble_engine.dart#L309-L317: enforce measured-over-derived precedence in durable persistence, not only in decoder memory.lib/ble/ble_engine.dart#L951-L953: do not use a connection-local set as the only source of measured-row precedence.test/gen5_sample_mapping_test.dart#L180-L198: provide at leastkGen5PpgHrMinSamplesthroughppgBuf, first prove that derivation succeeds without the measured timestamp, and then prove that the guard causes abstention. Add a persisted-row or reconnect regression test.
As per coding guidelines, regression tests must cover idempotence and synchronization behavior.
📍 Affects 2 files
lib/ble/ble_engine.dart#L309-L317(this comment)lib/ble/ble_engine.dart#L951-L953test/gen5_sample_mapping_test.dart#L180-L198
🤖 Prompt for 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.
In `@lib/ble/ble_engine.dart` around lines 309 - 317, Preserve measured HR
precedence across reconnects by enforcing the measured-row check during durable
persistence, not solely through the connection-local set in the Gen5 decoder
around sampleFromGen5PpgWaveform. Update the persistence path around the
teardown/set handling at lib/ble/ble_engine.dart lines 951-953 so reconnects
consult durable measured records and never replace them with derived PPG HR. In
test/gen5_sample_mapping_test.dart lines 180-198, supply at least
kGen5PpgHrMinSamples, verify derivation succeeds without the measured timestamp,
then verify the guard abstains; add persisted-row/reconnect coverage including
idempotence and synchronization behavior.
Source: Coding guidelines
There was a problem hiding this comment.
@Brackyt, acknowledged. The finding remains valid and is out of scope for this PR.
The draft body tracks the follow-up. #188 and validated analytics must unblock it before implementation. If you want separate issue tracking, I can create a GitHub issue with the reconnect and durable-persistence acceptance criteria.
You are interacting with an AI system.
There was a problem hiding this comment.
Valid future work — agree measured HR should win durably, not just for the current BLE session. Tracked as a follow-up when/if PPG ingest is reworked and re-validated. PR converted to Draft.
There was a problem hiding this comment.
@Brackyt, acknowledged. The durable measured-v18 precedence requirement remains valid and is tracked as future work.
The Draft status is appropriate while PPG ingestion and analytics validation remain pending. If you want, I can create a GitHub follow-up issue with reconnect, durable-persistence, idempotence, and synchronization acceptance criteria.
🐇
You are interacting with an AI system.
| if (isGen5V26) { | ||
| final archive = ArchiveRecord( | ||
| counter: counter, | ||
| hex: _innerHex(frame.inner), | ||
| packetType: frame.inner.isNotEmpty ? frame.inner[0] : 0, | ||
| capturedAt: DateTime.now().millisecondsSinceEpoch, | ||
| reason: 'gen5_v26_ppg', | ||
| ); | ||
| final d = _drain; | ||
| if (d != null) { | ||
| d.onHistoricalArchive(archive); | ||
| } else { | ||
| unawaited(onArchiveRecord?.call(archive) ?? Future<void>.value()); | ||
| } | ||
| if (sample == null) return; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Count archive-only v26 records as historical records.
When PPG derivation abstains, Line 2193 returns without calling onHistoricalRecord. The preceding onHistoricalArchive call does not increment records or recordsThisOffload.
A drain that contains only abstaining v26 records can therefore appear empty. This can produce incorrect sync reports and continuation decisions.
Use a separate archive-only ingestion method that increments the historical record counters. Keep onHistoricalArchive for archives paired with a stored sample.
Proposed fix
- if (d != null) {
- d.onHistoricalArchive(archive);
+ if (d != null) {
+ if (sample == null) {
+ d.onArchiveOnlyHistoricalRecord(archive);
+ } else {
+ d.onHistoricalArchive(archive);
+ }
...
+ void onArchiveOnlyHistoricalRecord(ArchiveRecord a) {
+ records++;
+ recordsThisOffload++;
+ _lastProgressAt = DateTime.now();
+ onHistoricalArchive(a);
+ }As per coding guidelines, maintain one historical ingestion path for each concern.
Also applies to: 3726-3735
🤖 Prompt for 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.
In `@lib/ble/ble_engine.dart` around lines 2179 - 2193, Update the isGen5V26
handling in the PPG derivation flow so archive-only records use a separate
historical ingestion method that increments records and recordsThisOffload
before returning when sample is null. Keep onHistoricalArchive for v26 archives
paired with a stored sample, and apply the same distinction at the corresponding
alternate flow.
Source: Coding guidelines
There was a problem hiding this comment.
@Brackyt, acknowledged. The finding remains valid and is deferred while this PR is a draft.
When this work resumes, archive-only v26 records should increment the historical offload counters before the early return.
You are interacting with an AI system.
There was a problem hiding this comment.
Valid — archive-only v26 rows should count toward offload progress. Deferred with this PR's Draft status; not blocking #188.
There was a problem hiding this comment.
@Brackyt, acknowledged. The archive-only v26 offload-accounting issue remains valid and is deferred. It does not block #188.
Would you like me to create a follow-up GitHub issue for this work?
You are interacting with an AI system.
…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.
|
Reviewed and pushed P0 — the measured-vs-derived guard evaporates at exactly the moment it's needed
So a re-delivered v26 burst derives an HR for a second that already has a measured v18 row.
Note the neighbouring Why I didn't just seed the setTo be correct it would have to hold every second ever measured. Provenance belongs on the datum instead:
Measured-vs-measured is deliberately untouched — "newest wins" is still correct there (the strap counter resets on reboot), and there's a test pinning that so this guard can't quietly regress it. This also de-fangs the u16-index-as-global-PK concern: a v26 burst index colliding with a real counter now loses its insert instead of overwriting a measured row. The clobber-guard test was vacuous — confirmed, not assumedI mutated it before rewriting it. With the guard line deleted: It passed because no The two new P0 tests are mutation-verified the same way: disabling the Not addressed here, deliberatelyAbsent gravity persisted as VerificationSuite 1076 passing / 6 failing; the 6 are |
|
I am not sure this PR is needed @abdulsaheel |
|
@Brackyt — thanks for saying so directly, and I think you're right. My honest recommendation is close this, and analytics#37 with it. Three independent things point the same way: 1. Your own premise is gone. You said the v26 PPG work existed because you thought the strap wasn't giving usable data, and that turned out not to be the case. If v18 delivers measured HR and RR, then deriving a bpm from PPG autocorrelation buys an inferior number — no RR, so no HRV — for a lot of moving parts. 2. It doesn't work on real hardware. Your own validation on analytics#37: ACF returns 29–160 bpm where measured is 90–100, across 22 evaluable windows. I found and fixed a separate fabrication bug there (a decaying ACF was accepted at the range boundary and reported as a confident 206 bpm), but that fix does not close the 29–160 gap. That's a deeper signal-processing problem, and fixing it is real work with no clear payoff given point 1. 3. Both bots independently found the P0 I fixed here. Copilot and CodeRabbit each flagged that If you close itNothing is lost that matters. The one piece with value independent of v26 is the principle that a derived value must never evict a measured one — worth remembering if any inferred-HR source is ever added, but not worth keeping a branch open for. Still-open bot findings, for the recordCodeRabbit's "enforce actual adjacency between buffered PPG bursts" is real and unaddressed (the buffer accepts any larger I'd rather not polish any of that on a branch you've said was a mistake. Your call — happy to close it out, or to keep going if you'd still like v26 working. |
|
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. |
|
Same conclusion, closing this |
Status: Draft — blocked on hardware validation
Do not merge. This PR depends on #188 (v18 persistence + unix gate) and a reworked/validated OpenStrap/analytics#37.
Real-hardware validation failure
Evaluated against export
openstrap_export_1785880255630.db(fw 50.40.1.0):Synthetic sine tests pass but are insufficient. PPG-derived HR must not ship until the algorithm is reworked and re-validated on hardware.
What this PR contains (research)
Sampleingest via normalized ACF HR (empty RR — no HRV claim).Gen5PpgBurstBuffer: capacity 12, adjacency gate (same-second or +1 s unix + monotonicburstIndex)._counterFromInner: u16@3 for v26 (not u32-inflated into v18 key space).reason: gen5_v26_ppg) even when a derived Sample is emitted.rec_ts(_gen5MeasuredRecTs).kAlgoVersion52 bump citing analytics pinb3e7b88624e4cbb6a0ab2dee6715446f19feb775.Known follow-ups (valid reviewer feedback, deferred)
_gen5MeasuredRecTs).Test plan
flutter test test/gen5_sample_mapping_test.dart test/gen5_decoded_onehz_persistence_test.dart test/gen5_v18_hardware_lenient_test.dart