workouts: record one heart-rate sample a second in a manual workout - #2416
Conversation
A manual workout scores its Effort -- live on the card and saved with the workout -- from the heart-rate samples it records, and both platforms recorded more than one for the same second. iOS runs captureWorkoutSample from two @published sinks (heartRate and rr), so a second in which the rate moves arrives twice; Android runs it on every LiveState emission that carries a heart rate. StrainScorer credits a zero gap with a full second (sampleDurationsMinutes), so each repeat counted as another second of effort: twenty minutes of 100-160 bpm with every second arriving twice scored 42.86 instead of 35.3. Calories integrate over the real gaps and were not affected. A second that already has its sample now takes no other (iOS ActiveWorkout.recordSample, the same guard inline on Android), which also stops a repeat rescoring the workout and re-saving its snapshot. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ed; upstream parity red on main; four-change build shipping
ryanbr
left a comment
There was a problem hiding this comment.
Thanks @UtkuDenizAltiok, and welcome. The diagnosis is right and I checked it rather than taking it on trust: sampleDurationsMinutes credits a zero gap with fallbackSampleMin, a whole second (StrainScorer.swift:228), so a second that arrives twice really is counted twice. Mutation-checking the guard and restoring the file byte-identical by sha256 is exactly the right way to show a test earns its keep.
Your parity note is also correct, and worth more than a footnote: the scheduled Parity Governance run did fail on 971d0d9f1 (run 35818771138), on test_checked_in_inventory_and_baseline_match_current_sources and test_checked_metadata_is_compact_v3_and_expands_losslessly. That is main, not this branch, and it is mine to repair. Nothing for you to do about it here.
One change before merge.
The dedupe also drops a peak
Both platforms return before the peak is folded in:
guard w.recordSample(HRSample(ts: ..., bpm: hr)) else { return }
w.peakHr = max(w.peakHr, hr)if (w.samples.lastOrNull()?.ts == ts) return
...
peakHr = s.maxOf { it.bpm }The two sinks fire because the rate moved, so the refused reading is frequently a DIFFERENT bpm for that second, and by your own analysis that is most seconds of a workout. Today both readings reach peakHr; after this change only the first of each second does, and a within-second high is discarded rather than deduplicated. The effect is bounded by the within-second variation, but it is a behaviour change, and the body lists Effort as the only thing that moves.
Keeping the perf win and the peak is a small edit: on a repeated second fold the bpm into peakHr and return before the rescore and the snapshot, rather than returning first. Then the guard does what its name says, one sample a second, and nothing else changes.
Smaller
samples.last compares only the final element, so it dedupes a repeat but not an out-of-order arrival. Fine for a monotonic 1 Hz stream, worth a word in the doc.
The Kotlin guard is inline and so has no test. You are right that keeping it inline avoids a new parity identity, and I would not trade that away for coverage; just noting the Android side rests on review alone.
Review on ryanbr#2416: the sinks fire because the rate moved, so the reading refused for a repeated second is often a different bpm, and returning before the peak discarded a within-second high. A repeat now folds into peakHr (iOS ActiveWorkout.recordSample; Android inline) and only the peak is published for it, with no rescore and no snapshot. Both platforms also saved the workout's maxHr from the samples alone, so the saved max takes the folded peak too (iOS ActiveWorkout.savedPeak, Android maxOf(samples max, w.peakHr)), and Android now grows the live peak by comparison instead of recomputing it from the samples, which would drop the fold at the next second. recordSample's doc says it compares only the last sample: a repeat, not an out-of-order arrival. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review on ryanbr#2416: the sinks fire because the rate moved, so the reading refused for a repeated second is often a different bpm, and returning before the peak discarded a within-second high. A repeat now folds into peakHr (iOS ActiveWorkout.recordSample; Android inline) and only the peak is published for it, with no rescore and no snapshot. Both platforms also saved the workout's maxHr from the samples alone, so the saved max takes the folded peak too (iOS ActiveWorkout.savedPeak, Android maxOf(samples max, w.peakHr)), and Android now grows the live peak by comparison instead of recomputing it from the samples, which would drop the fold at the next second. recordSample's doc says it compares only the last sample: a repeat, not an out-of-order arrival. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…2417 ryanbr#2418 opened; build 6adaf45
…swered in code
|
Thanks for checking it properly. You're right about the peak, and it went one step further than the live number: both platforms also saved the workout's max from the samples alone, so a refused within-second high was missing from the saved row too. Fixed in fb774dd:
New test |
The iOS stress check-in appended the live R-R intervals to its buffer every time ingestHR ran, and ingestHR runs from two published sinks, the heart rate and the R-R. A sink runs inside willSet, so the handler reads the packet before the one being written, and a packet whose heart rate also moved reached it twice: its intervals entered rrBuf twice and the detector's slow baseline advanced on every call rather than every packet. Driven through a real LiveState in the standard-HR write order, 21 packets came out as 800, 800, 801, 802, 802, 803, 804, 804. RRPacketCursor states for a non-view consumer the rule RRPacketObserver already gives views, take a packet once keyed on rrSeq, and evaluateStress asks it first. setRRIntervals assigns rr and then bumps rrSeq, so live.rr always belongs to live.rrSeq whichever sink is running, and a packet the heart-rate sink never carries is taken on the next packet's R-R sink instead. rrSeq is incremented before any sink can observe it, so a real packet never carries the cursor's initial 0. Android runs the same detector once per history offload on rrRecent and is unchanged. The platforms still differ in how often they evaluate, per packet here and per offload there, which this leaves alone deliberately. Raised alongside #2416, the same two sinks double-recording manual-workout samples. Authored by @UtkuDenizAltiok.
What this PR does
A manual workout (Start workout) records the live heart rate into its sample list, and its Effort — live on the card and saved with the workout — is scored from that list. Both platforms recorded more than one sample for the same second:
captureWorkoutSampleruns from two@Publishedsinks inAppModel(live.$heartRateandlive.$rr, AppModel.swift ~258). On a WHOOP the standard-HR path sets the R-R intervals on every packet and the heart rate whenever it changes (BLEManager.swift ~5511–5516), so a second in which the rate moves — most seconds of exercise — arrives twice, with onets.captureWorkoutSampleruns on everyLiveStateemission that carries a heart rate (AppViewModel.kt ~895), i.e. whenever any field of the live state changes, not only the rate.StrainScorer.sampleDurationsMinutescredits each sample with the gap to the next one and a zero gap with a full second (fallbackSampleMin), so each repeated second counted as one more second of effort. Calories are not affected:estimateBoutCaloriesintegrates over the real gaps, where a repeat adds nothing.Now a second that already has its sample takes no other, on both platforms, so the workout is scored from one reading a second — which is what the stream is. A repeat also no longer rescores the workout or re-saves its crash snapshot.
Type of change
How it was tested
WorkoutSampleOncePerSecondTests(StrandTests), 2 tests on the realAppModel.ActiveWorkoutandStrainScorer: a repeated second is refused; twenty minutes of 100→160 bpm with every second arriving twice now scores exactly the Effort of the plain once-a-second stream, where the repeats, kept, scored 42.86 instead of 35.3. With the guard removed fromrecordSampleboth tests fail (the repeated second is kept; 42.86 ≠ 35.3), and the file restored byte-identical (sha256).3f3cfc19(on266a8702): WhoopStore 611 · StrandAnalytics 2048 · StrandImport 327 · doc lint · i18n · macOSStrandTests2,109 (the only failures are the two locale-dependentTodayCarryOverTests, which fail on cleanmaintoo) · iOS build. No new warnings in the changed files.main@266a8702(twin-map authority drift; the scheduled Parity Governance run failed on971d0d9fthis morning). This PR adds no Swift or Kotlin twin; on the previous base,94a71b04, all three passed with this change.captureWorkoutSample(no new function, so no new parity identity).AppViewModelhas no JVM test harness, so the Kotlin side is covered by review and by Android CI on the fork (run 35813899964 on the pre-rebase head, green; the rebase changed none of these lines).Checklist
android/(Android CI on the fork)docs/CONTRIBUTING.mdStrand.xcodeproj/) or any secrets/keystoresRelated issues
Refs #950 (per-sample durations in Effort)
🤖 Generated with Claude Code