Repository navigation
Arm the realtime HR stream while the Breathe screen is visible - #2036
Conversation
BreatheScreen displayed AppViewModel.bpm and consumed live R-R data but never took a realtime want, so with Breathe the only live-HR surface the stream was off and the BPM hero / RMSSD / R-R readouts froze. Take one ref-count for the screen's visible lifetime (Android DisposableEffect, balanced 1:1; iOS/macOS startRealtimeHR/stopRealtimeHR in onAppear/onDisappear, mirroring LiveWorkoutView) and release it on disposal. The want is unconditional, not gated on a running session, since the BPM hero and R-R readouts render before any session starts.
|
Exact-head comment on #2036 head 5ea0564. KEEP Breathe-screen realtime HR want for visible lifetime (iOS startRealtimeHR/stopRealtimeHR + Android requestRealtimeHr/releaseRealtimeHr), matching Live screens. Breathe showed live bpm/R-R but never armed the ref-counted stream, so with Breathe as sole HR surface samples froze (WHOOP 5/MG repro). CHECK: no double-want via controllerBox.prepare; no tests in +10/-1 diff. Author kiesstein not kvnloo. No competing PR from me. |
ryanbr
left a comment
There was a problem hiding this comment.
Reviewed at 5ea0564. The bug is real and this is the right shape of fix. Waiting only on the app
build and Android suite, which were still running when I looked.
Confirmed the screen really was reading a stream nobody had armed. Neither surface asked for it
before this: grep RealtimeHR finds nothing in BreathingView.swift on main, and nothing in
BreatheScreen.kt. Yet both consume live beats. Swift ingests them at .onRRPackets(live) and shows
model.bpm plus an RMSSD readout tile; Kotlin collects viewModel.live.rrPackets() into the rolling
buffer that feeds Hrv.rmssd and the coherence card. So on a WHOOP 5 or MG, which only streams while
armed, the flagship biofeedback screen sat on "No data" and a dash unless the wearer happened to have
opened Live first. That also explains why it would have looked intermittent rather than broken.
I checked the ref-counting rather than trusting the naming, because the two sides are spelled
differently. They match. Swift's startRealtimeHR / stopRealtimeHR hold realtimeWanters and
Kotlin's requestRealtimeHr / releaseRealtimeHr hold the same counter, both arm on the 0 to 1 edge,
both call resetSmoothing there and only there, both clamp the release at zero, and each documents
the other as its twin. So the comment's claim that navigating to Live will not stop the stream out
from under this screen is accurate on both platforms, and a stale BPM cannot survive the transition
on one platform while being blanked on the other.
The balance is clean in this screen specifically. Nothing else in the Breathe path touches the
count: BiofeedbackController never calls either method, and LiveSessionRunner, which does, is not
on this path and no-ops its own release when it was never running. So the appear and disappear pair
is the only party, which is what makes a single increment safe here.
On arming while merely visible rather than while running, which is the one judgement call in the
change: I think you have it right, and it is worth saying why out loud given how much attention strap
drain has had. The BPM hero and the RMSSD tile are on screen before a session starts, and gating the
arm on running would leave a wearer looking at two dashes deciding the feature is broken, which is
the report this started from. The cost is bounded the way Live's is, by a foreground screen the user
opened deliberately, and the lightweight HR recording is unaffected either way.
Both platforms in one change, using the existing primitive on each rather than inventing a second
mechanism. Nothing to change from my side. Thanks @kiesstein.
What this PR does
The Breathe screen displays AppViewModel.bpm (smoothed, live-only) and consumes live R-R data, but never took a realtime want. With Breathe as the only live-HR surface on screen the reference-counted stream was never armed, so ingestHr() received no fresh samples and the BPM hero / RMSSD / R-R readouts froze on whatever the last arming screen left behind. Reproduced on a WHOOP 5/MG. BreathingView.swift had the twin omission (parity contract).
This change takes one ref-count for the Breathe screen's visible lifetime and releases it on disposal:
The want is unconditional, not gated on running, since the BPM hero and R-R readouts render before any session starts.
Why it's safe:
Type of change
How it was tested
Hardware (WHOOP 5/MG): stale BPM reproduced before the fix; after the fix, opening Breathe from a non-live screen arms the realtime stream and the BPM hero updates while the screen is visible.
Android (automated):
No new automated tests: there is no Compose UI test scaffolding in the repo (no androidTest/, no Robolectric), and AppViewModel requires a NoopApplication and isn't constructable in the plain-JVM unit-test setup. The ref-count logic itself is unchanged.
Not yet run: macOS/iOS app-target builds (need a Mac — app-build.yml is disabled and swift-packages doesn't compile Strand/) and the iOS/macOS strap leg of the same matrix.
Checklist