feat(gen5): v26 PPG→HR via normalized ACF peak - #37
Conversation
Derives BPM from 24 Hz waveform bursts (length≥24, variance gate, 25–230 bpm lag search) and abstains on flatline / no clear peak.
Low resting BPM (40–55) needs ≥4 beats in-window; four 1s bursts cannot resolve that honestly. Raise min samples to 240 (10s @ 24Hz) and expand tests: low-HR 12s window, noisy sine, under-length abstain.
|
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 change adds a Gen5 v26 PPG BPM derivation function, exports it from the public library, and adds regression tests for valid and invalid waveform inputs. ChangesGen5 PPG heart-rate derivation
Estimated code review effort: 3 (Moderate) | ~20 minutes 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
Adds a Gen5 v26 PPG→HR derivation utility to the 1Hz analytics layer, using a normalized autocorrelation (ACF) peak-picking approach that abstains on low-quality input rather than fabricating BPM.
Changes:
- Introduces
deriveHrFromGen5PpgWaveformto estimate HR from concatenated 24 Hz Gen5 PPG bursts via normalized ACF peak selection. - Exports the new foundation API via
lib/onehz.dart. - Adds unit tests covering nominal, low-HR (longer window), noisy, flatline, and too-short input scenarios.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/gen5_ppg_hr_test.dart | Adds regression tests for the new Gen5 PPG→HR derivation behavior and abstention cases. |
| lib/src/onehz/foundations/gen5_ppg_hr.dart | Implements the normalized ACF-based HR derivation function and minimum-window/quality gating. |
| lib/onehz.dart | Exports the new Gen5 PPG HR foundation API from the 1Hz barrel. |
Suppressed comments (2)
lib/src/onehz/foundations/gen5_ppg_hr.dart:17
- The minimum-length check uses a fixed
kGen5PpgHrMinSamples(240 @ 24 Hz) but the function also acceptssampleHz. If a caller passes a differentsampleHz, the current guard no longer represents a ~10s window as documented (e.g., 48 Hz would allow only 5s). Either dropsampleHzor scale the min-sample requirement bysampleHz.
int? deriveHrFromGen5PpgWaveform(List<int> samples, {double sampleHz = 24.0}) {
if (samples.length < kGen5PpgHrMinSamples || sampleHz <= 0) return null;
lib/src/onehz/foundations/gen5_ppg_hr.dart:65
- This comment says ties are broken toward a “physiologically mid-range lag”, but the code below does not implement any mid-range tie-breaker (it only uses
c + 0.01 * prominence). Please update the comment to match the actual scoring (or implement the tie-breaker if intended).
// Prefer higher ACF; break ties toward the physiologically mid-range lag.
final score = c + 0.01 * prominence;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// | ||
| /// Pure, isolate-safe. Absent / degenerate input returns null — never a | ||
| /// fabricated BPM. | ||
| library; |
There was a problem hiding this comment.
Not an analyzer issue: library; is a valid unnamed library directive under this repo's SDK constraint (Dart ^3.5). dart analyze on gen5_ppg_hr.dart reports no issues.
![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/src/onehz/foundations/gen5_ppg_hr.dart`:
- Around line 37-38: Update the lag-selection logic around minLag, maxLag, and
the real-neighbor checks to evaluate ACF samples immediately outside both
accepted BPM bounds instead of using artificial negative-infinity neighbors.
Interpolate only genuine local maxima, then apply the existing 25–230 BPM gate
to the resulting estimate; monotonic ACF trends must abstain with null rather
than select a boundary lag. Add coverage for the 230 BPM boundary and decreasing
monotonic input.
- Around line 16-17: Update deriveHrFromGen5PpgWaveform to enforce the minimum
10-second window based on sampleHz rather than the fixed kGen5PpgHrMinSamples
threshold. Preserve custom sample rates, reject non-finite or non-positive
sampleHz values in the existing guard, and return null when samples do not cover
the required duration.
🪄 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: 7cc5d347-50f8-4abc-9c08-5c94e80bea9a
📒 Files selected for processing (3)
lib/onehz.dartlib/src/onehz/foundations/gen5_ppg_hr.darttest/gen5_ppg_hr_test.dart
| int? deriveHrFromGen5PpgWaveform(List<int> samples, {double sampleHz = 24.0}) { | ||
| if (samples.length < kGen5PpgHrMinSamples || sampleHz <= 0) return null; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep the window-duration rule consistent with sampleHz.
240 samples equal 10 seconds only at 24 Hz. With sampleHz: 48, a five-second 45 BPM sine can return 45, although it contains only 3.75 beats. If custom rates are supported, calculate the minimum from a 10-second duration. Otherwise, remove the override or reject rates other than 24. Reject non-finite rates in the same guard.
Proposed duration gate
+const double _kGen5PpgHrMinWindowSeconds = 10.0;
+
int? deriveHrFromGen5PpgWaveform(List<int> samples, {double sampleHz = 24.0}) {
- if (samples.length < kGen5PpgHrMinSamples || sampleHz <= 0) return null;
+ if (!sampleHz.isFinite || sampleHz <= 0) return null;
+ final minSamples = (sampleHz * _kGen5PpgHrMinWindowSeconds).ceil();
+ if (samples.length < minSamples) return null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| int? deriveHrFromGen5PpgWaveform(List<int> samples, {double sampleHz = 24.0}) { | |
| if (samples.length < kGen5PpgHrMinSamples || sampleHz <= 0) return null; | |
| const double _kGen5PpgHrMinWindowSeconds = 10.0; | |
| int? deriveHrFromGen5PpgWaveform(List<int> samples, {double sampleHz = 24.0}) { | |
| if (!sampleHz.isFinite || sampleHz <= 0) return null; | |
| final minSamples = (sampleHz * _kGen5PpgHrMinWindowSeconds).ceil(); | |
| if (samples.length < minSamples) return null; |
🤖 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/src/onehz/foundations/gen5_ppg_hr.dart` around lines 16 - 17, Update
deriveHrFromGen5PpgWaveform to enforce the minimum 10-second window based on
sampleHz rather than the fixed kGen5PpgHrMinSamples threshold. Preserve custom
sample rates, reject non-finite or non-positive sampleHz values in the existing
guard, and return null when samples do not cover the required duration.
There was a problem hiding this comment.
@Brackyt, acknowledged. The finding remains valid, but the hardware-validation mismatch makes deferring the configurable sampleHz work appropriate for this PR.
Would you like me to create a GitHub follow-up issue for the duration gate and ACF validation work?
You are interacting with an AI system.
There was a problem hiding this comment.
Valid future work — window duration should stay consistent with a configurable sampleHz rather than hard-coded 24 Hz assumptions. Deferred; PR is Draft until hardware validation passes.
There was a problem hiding this comment.
@Brackyt, acknowledged.
The sampleHz-based window-duration gate remains deferred. I will keep the finding unresolved for this PR. Hardware validation is the current prerequisite for further changes to deriveHrFromGen5PpgWaveform.
I can create a GitHub follow-up issue if you want to track the duration gate and ACF hardware-validation work separately.
You are interacting with an AI system.
| final minLag = math.max(1, (60.0 * sampleHz / 230.0).ceil()); | ||
| final maxLag = math.min(n - 1, (60.0 * sampleHz / 25.0).floor()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Evaluate real ACF neighbors at both BPM boundaries.
At 24 Hz, minLag is 7. Therefore, round(1440 / bestLag) cannot exceed 206, despite the documented 230 BPM maximum. The artificial double.negativeInfinity neighbor also accepts a nonpeak boundary. List<int>.generate(240, (i) => i) has a decreasing ACF, but this code selects lag 7 and returns 206 instead of null.
Calculate ACF values outside both accepted bounds. Locate and interpolate a real local maximum. Then apply the 25–230 BPM gate. Add 230 BPM and monotonic-trend abstention tests.
Also applies to: 42-43, 59-62
🤖 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/src/onehz/foundations/gen5_ppg_hr.dart` around lines 37 - 38, Update the
lag-selection logic around minLag, maxLag, and the real-neighbor checks to
evaluate ACF samples immediately outside both accepted BPM bounds instead of
using artificial negative-infinity neighbors. Interpolate only genuine local
maxima, then apply the existing 25–230 BPM gate to the resulting estimate;
monotonic ACF trends must abstain with null rather than select a boundary lag.
Add coverage for the 230 BPM boundary and decreasing monotonic input.
There was a problem hiding this comment.
@Brackyt, acknowledged. The finding remains valid and is deferred from this draft.
Would you like me to create a follow-up GitHub issue for boundary peak validation and monotonic-trend abstention?
You are interacting with an AI system.
There was a problem hiding this comment.
Valid future work — real ACF neighbor evaluation at BPM boundaries and monotonic-trend abstention are needed before ship. Deferred; PR is Draft pending hardware rework (current ACF: 29–160 bpm vs measured 90–100 bpm on 22 evaluable windows).
There was a problem hiding this comment.
@Brackyt, acknowledged. The finding remains unresolved and required before release.
The current hardware result shows a material accuracy problem. deriveHrFromGen5PpgWaveform returns 29–160 BPM where measured values are 90–100 BPM across 22 evaluable windows. The draft status is appropriate while the hardware rework continues.
✏️ Learnings added
Learnt from: Brackyt
Repo: OpenStrap/analytics PR: 37
File: lib/src/onehz/foundations/gen5_ppg_hr.dart:37-38
Timestamp: 2026-08-04T22:56:42.187Z
Learning: For `lib/src/onehz/foundations/gen5_ppg_hr.dart`, `deriveHrFromGen5PpgWaveform` must evaluate real autocorrelation neighbors at both BPM boundaries and abstain on monotonic ACF trends before release. During hardware validation, the current ACF returned 29–160 BPM for 22 evaluable windows where measured heart rate was 90–100 BPM. The related pull request remains a draft pending hardware rework.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
The header promises "absent / degenerate input returns null -- never a
fabricated BPM". Measured against this implementation, it did not hold:
baseline wander (slow sine drift, no cardiac content) -> 206 bpm
linear ramp (pure DC drift) -> 206 bpm
single step edge -> 206 bpm
206 bpm is the TOP of the 25-230 search range, and it comes out with full
confidence from signals containing no heartbeat at all.
ROOT CAUSE: the peak search considered the BOUNDARIES of its own lag range.
At `lag == minLag` the left neighbour was `double.negativeInfinity`, so the
`c <= left` rejection could never fire; the prominence fallback then
substituted `c - 1`, making prominence exactly 1.0 -- maximal. So the shortest
lag was an unconditionally valid "peak", and every monotonically DECAYING
autocorrelation was accepted there.
That is the opposite of a rare edge case. A smoothly decaying ACF is precisely
what baseline wander, a DC drift and motion artifacts produce -- so the
quieter and smoother the input, the more confident the fabricated tachycardia.
FIX: a periodicity claim needs a real turning point, so only INTERIOR lags are
peak candidates (both neighbours computed, both strictly lower). All three
cases above now abstain. Real cardiac signals are unaffected -- clean sines at
45/60/75/100/140 bpm still resolve to 45/60/76/103/144 (integer-lag
quantization at 24 Hz, unchanged by this).
This matters beyond analytics: edge#190 consumes this to write a DERIVED HR
into `decoded_onehz`. A fabricated 206 bpm would have been persisted as a real
resting heart rate for any second the strap did not measure itself.
PARTLY REFUTING the review note that prompted this: it claimed white noise
yields a confident BPM "~25% of the time". Measured over 400 trials it is
6/400 = 1.5%, both before and after this change -- noise was never the problem.
The smooth-signal boundary case was, and that one is 100% reproducible.
7 tests added, mutation-verified: restoring the boundary-inclusive search
fails exactly the four fabrication tests. Full suite 392 passing.
|
Reviewed and pushed The "never a fabricated BPM" guarantee didn't holdThe header promises "Absent / degenerate input returns null — never a fabricated BPM." Measured against this implementation: 206 bpm is the top of the 25–230 search range, returned with full confidence from signals containing no heartbeat at all. Root causeThe peak search considered the boundaries of its own lag range: final left = lag > minLag ? acf[lag - 1] : double.negativeInfinity;
...
if (c <= left || c <= right) continue;
final prominence = c - math.max(left.isFinite ? left : c - 1, ...);At That's the opposite of a rare edge case. A smoothly decaying ACF is precisely what baseline wander, DC drift and motion artifacts produce — so the quieter and smoother the input, the more confident the fabricated tachycardia. FixA periodicity claim needs a real turning point, so only interior lags are peak candidates (both neighbours computed, both strictly lower). All three cases above now abstain. Real signals are unaffected — clean sines still resolve:
(The small offsets are integer-lag quantization at 24 Hz, unchanged by this.) Why this matters beyond analyticsedge#190 consumes this to write a derived HR into (Separately, on edge#190 I made derived samples unable to evict a measured row, so at least this could never have overwritten real data — but on unmeasured seconds it would have stood as fact.) Partly refuting the finding that prompted thisThe review note claimed white noise yields a confident BPM "~25% of the time". Measured over 400 trials it's 6/400 = 1.5%, both before and after this change — noise was never really the problem. The smooth-signal boundary case was, and that one is 100% reproducible. Verification7 tests added, mutation-verified: restoring the boundary-inclusive search fails exactly the four fabrication tests and nothing else. Full analytics suite 392 passing. |
|
Reading back through the thread after pushing
That's the same defect, and CodeRabbit got there first — including the detail I'd missed, that the documented 230 bpm ceiling is unreachable at 24 Hz because CodeRabbit's other finding — that The harder question@Brackyt's hardware validation is the thing that actually decides this PR:
My fix does not close that gap. It removes a fabrication mode (confident 206 bpm from signals with no cardiac content) and it keeps clean sines resolving correctly, but a 29–160 spread against a true 90–100 is a signal-processing problem several layers deeper than peak-boundary handling — most likely bandpass/detrending before the ACF, plus the burst-adjacency issue CodeRabbit flagged on edge#190 (the buffer will concatenate captures with missing intermediate bursts, which would smear the autocorrelation exactly like this). Combined with @Brackyt's note on edge#190 that the whole v26 PPG line was built on a premise that turned out to be wrong — the strap does give usable v18 data — my recommendation is to close this along with edge#190 rather than keep polishing it. A derived bpm with no RR is strictly worse than measured v18 HR + RR, and it costs real complexity to get right. If it's kept, the boundary fix is worth having regardless, since the function would otherwise fabricate on any smooth artifact. Your call — I'm happy either way, and I'd rather flag it than quietly keep improving something that shouldn't ship. |
|
Tip For best results, initiate chat on the files or code changes.
The interior-peak requirement fixes the boundary fabrication defect. It does not validate the derived-HR algorithm for production use. The hardware result is the deciding evidence. A 29–160 BPM output range against measured 90–100 BPM is not suitable for writing derived HR to Closing this PR and edge#190 is the safer decision if usable v18 measured HR and RR are available. This avoids persisting a lower-confidence value when the device provides measured data. If the work remains active, keep these items open:
🐇 ✏️ Learnings added
You are interacting with an AI system. |
|
Same conclusion, closing this |
Status: Draft — blocked on hardware validation
Do not merge. Real-hardware validation of
deriveHrFromGen5PpgWaveformfailed against a WHOOP 5 export (fw 50.40.1.0).Real-hardware validation failure
Synthetic sine tests (72/120/45 bpm, noisy, flatline, under-length) pass but are insufficient. No PPG-derived HR should ship until the algorithm is reworked and re-validated on hardware.
What this PR contains (research)
deriveHrFromGen5PpgWaveform— pure ACF peak picker on concatenated gen5 v26 PPG int16 samples (24 Hz). Abstains on thin/flat/noisy windows; never fabricates BPM.Known follow-ups (valid reviewer feedback, deferred)
sampleHzparameter.Test plan
dart test test/gen5_ppg_hr_test.dart(synthetic only — insufficient for ship)