Replies: 1 comment
|
going through this now — verified the races, they're real (and stopWorkout is actually worse than you said, it never calls disableLiveStreams at all right now, nothing recomputes owners when a workout ends). my calls on the 4:
architecture-wise: wantHr/wantImu reconciler is the right shape, matches how ble_state.dart/sync_policy.dart already work (pure policy class, diff desired vs applied). one thing to add — needs a link-generation id so a stale completion from a replaced connection can't stomp the current one. we already do that informally in a couple places, just make it explicit here. go ahead and build it. |
0 replies
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Status: approved and implementation-ready
The maintainer verified the races and approved the independent
wantHr/wantImureconciler, including an explicit link-generation guard. This discussion is now the complete implementation contract for the live HR/IMU ownership PR. The final policy and boundaries below supersede the earlier open questions.The PR may be implemented directly from this body. Optical opcode cleanup, live R-R correction, and the design of a reliable movement-reminder sampling scheduler remain separate concerns; they must not be folded into or used to block the ownership fix.
Why discuss this before a PR?
Edge currently treats a foreground connection as an implicit request for both
realtime HR and the 100 Hz Gen5 IMU stream. Real features need different
combinations, and the existing enable/disable operations can race. The
technical need for explicit ownership is clear, but changing passive step and
movement-reminder behavior is a product decision, so I would like to agree on
the policy before implementing it.
The two related small fixes—Gen5 optical opcode 107/108 cleanup and the live
R-R calculation issue—remain separate from this proposal.
What Edge does today
On Edge
mainatd4d4cde:openSession()unconditionally restores the full live bundle after foreground READY;
enableLiveStreams()means opcode 3 HR ON plus opcode 106 IMU ON;
is off or HR-only; and
_hasLiveConsumeris a single background-policy boolean. It does notrepresent HR and IMU owners independently.
The streams do have consumers, but foreground connection state is broader than
those consumers and currently hides their different requirements.
Verified consumer map
The gait list is
running,trail_running,walking,hiking,dog_walking,treadmillandcross_country. All other workouts currentlydiscard workout step accumulation from the live IMU.
The Gen5 on-chip historical step counter is a whole-day fallback, not the
highest-priority source. Current code uses it only when the resolved windowed
live/phone total is zero:
That distinction matters when deciding whether passive live IMU collection is
worth retaining.
Confirmed transition races
A stale OFF can defeat a new owner
_stopBreathingStreams()startsdisableLiveStreams()without awaiting ortracking it. The engine continues to report
liveEnabled == trueuntil theOFF sequence finishes.
This allows the following order:
The background downgrade already has a special future/settle mechanism for
this exact race, but breathing teardown is outside it.
The last owner can leave without disabling the stream
Breathing records only whether it happened to perform the original enable. If
it starts while a workout or foreground connection already has streams on, it
records no ownership. When the other feature and then breathing leave, no
single path recomputes the complete remaining owner set. HR and IMU can remain
enabled, with the keepalive continuing to re-arm IMU.
The engine also sets its live flags before enable writes have succeeded. That
is a separate reason the applied state should not double as desired state.
Proposed shape
Use two independently computed desired bits:
This represents OFF, HR-only, IMU-only and HR+IMU. A three-state
off / HR-only / fullmodel cannot represent movement reminders without HR andwould unnecessarily arm IMU for breathing and non-gait workouts.
One serialized/coalescing reconciler should:
Suggested Gen5 policy
The iOS row preserves Edge's current documented strategy: inbound HR
notifications are intentionally used to make background execution more likely.
This is an Edge platform policy, not a claim of measured guarantee or official
WHOOP parity.
Resolved implementation contract
The maintainer decisions are final for this ownership PR:
activeMovementReminderSamplingowner for bounded sample windows. The window scheduler is not part of this PR: sampling only inside bounded windows cannot prove that movement did not happen between them, so this PR must not invent a cadence or make a new uninterrupted-stillness claim. Until a separately validated scheduler can observe the gaps, no standing movement-reminder IMU owner is created.Required ownership model
Compute desired state from explicit consumers, never from foreground connection state:
activeMovementReminderSamplingandpassiveStrapStepSamplingOptIndefault to false. The model must represent all four states: OFF, HR-only, IMU-only and HR+IMU.Required transition semantics
Required acceptance tests for this PR
Use delayed and failing fake writes to cover:
Non-goals
All reactions