Repository navigation
Alarm: defer the re-arm-on-bond call so it doesn't race the connect handshake (#34) - #340
Conversation
…andshake (ryanbr#34) state.bonded is set from inside BLEManager's connect-handshake continuation, and Combine delivered the bonded sink synchronously on that same call stack — so armStrapAlarm()'s SET_CLOCK/SET_ALARM_TIME/GET_ALARM_TIME burst landed in the middle of the handshake, ahead of its own clock-set and before the cmd-notify characteristic was resubscribed post-bond. A v8.6.2 strap log (issue ryanbr#34) confirmed the result: the alarm's GET_ALARM_TIME readback got no reply at all. Deferring one runloop turn lets the handshake finish first, so the alarm arms on a settled, fully-subscribed link. No BLE bytes/commands change, only order.
|
Reviewed — the diagnosis is right and well-evidenced (the dropped GET_ALARM_TIME readback in the #34 log is the tell), and the parity call checks out: Android arms the strap alarm too ( One suggested refinement to the fix itself. There's already a precise signal for "handshake settled": Suggested fix: arm the strap alarm off that same "handshake complete" signal rather than off raw Either way this is a BLE-timing change that compile + unit tests can't validate — it needs an on-device retest on a real WHOOP 4.0 before #34 is called fixed. Thanks for the sharp write-up, including catching that the "720 days in the future" line was the separate |
…anbr#34) Review feedback on ryanbr#340: a one-turn `.main.async` defer breaks the synchronous nesting inside BLEManager's connect-handshake continuation, but doesn't guarantee the cmd-notify characteristic has actually confirmed subscribed by the time the deferred alarm arm sends GET_ALARM_TIME — that confirmation (didUpdateNotificationStateFor) can land over a later delegate callback, past a single runloop turn. Replaces the defer with a precise signal: LiveState.connectSettled, a counter BLEManager bumps only once BOTH connectHandshakeDone (hello + SET_CLOCK sent) AND the cmd-notify characteristic's subscription is CONFIRMED active are true - whichever lands second. AppModel's alarm re-arm sink now keys off `live.$connectSettled` instead of `live.$bonded`, so SET_ALARM_TIME/ GET_ALARM_TIME always go out on a link whose reply channel is confirmed live. WHOOP 5/MG (which never sends GET_ALARM_TIME and stays behind the Experimental toggle) signals settled directly off its own handshake completion, preserving its prior re-arm-on-bond timing unchanged.
|
Addressed in 2f6dc01 — replaced the You're right that a single runloop turn only guarantees leaving the nested WHOOP 5/MG (Experimental, never sends GET_ALARM_TIME) signals settled directly off its own handshake completion, so its re-arm timing is unchanged — it doesn't wait on the WHOOP4-only cmd-notify gate. Rebuilt + retested: macOS Still needs the on-device retest from the #34 reporter — this remains a BLE-timing change compile/unit tests can't validate on their own. |
…andshake (#34) (ryanbr#340) * Alarm: defer the re-arm-on-bond call so it doesn't race the connect handshake (#34) state.bonded is set from inside BLEManager's connect-handshake continuation, and Combine delivered the bonded sink synchronously on that same call stack — so armStrapAlarm()'s SET_CLOCK/SET_ALARM_TIME/GET_ALARM_TIME burst landed in the middle of the handshake, ahead of its own clock-set and before the cmd-notify characteristic was resubscribed post-bond. A v8.6.2 strap log (issue #34) confirmed the result: the alarm's GET_ALARM_TIME readback got no reply at all. Deferring one runloop turn lets the handshake finish first, so the alarm arms on a settled, fully-subscribed link. No BLE bytes/commands change, only order. * Alarm: gate the re-arm-on-bond on connect-settled, not raw bonded (#34) Review feedback on ryanbr#340: a one-turn `.main.async` defer breaks the synchronous nesting inside BLEManager's connect-handshake continuation, but doesn't guarantee the cmd-notify characteristic has actually confirmed subscribed by the time the deferred alarm arm sends GET_ALARM_TIME — that confirmation (didUpdateNotificationStateFor) can land over a later delegate callback, past a single runloop turn. Replaces the defer with a precise signal: LiveState.connectSettled, a counter BLEManager bumps only once BOTH connectHandshakeDone (hello + SET_CLOCK sent) AND the cmd-notify characteristic's subscription is CONFIRMED active are true - whichever lands second. AppModel's alarm re-arm sink now keys off `live.$connectSettled` instead of `live.$bonded`, so SET_ALARM_TIME/ GET_ALARM_TIME always go out on a link whose reply channel is confirmed live. WHOOP 5/MG (which never sends GET_ALARM_TIME and stays behind the Experimental toggle) signals settled directly off its own handshake completion, preserving its prior re-arm-on-bond timing unchanged.
…andshake (ryanbr#34) (ryanbr#340) * Alarm: defer the re-arm-on-bond call so it doesn't race the connect handshake (ryanbr#34) state.bonded is set from inside BLEManager's connect-handshake continuation, and Combine delivered the bonded sink synchronously on that same call stack — so armStrapAlarm()'s SET_CLOCK/SET_ALARM_TIME/GET_ALARM_TIME burst landed in the middle of the handshake, ahead of its own clock-set and before the cmd-notify characteristic was resubscribed post-bond. A v8.6.2 strap log (issue ryanbr#34) confirmed the result: the alarm's GET_ALARM_TIME readback got no reply at all. Deferring one runloop turn lets the handshake finish first, so the alarm arms on a settled, fully-subscribed link. No BLE bytes/commands change, only order. * Alarm: gate the re-arm-on-bond on connect-settled, not raw bonded (ryanbr#34) Review feedback on ryanbr#340: a one-turn `.main.async` defer breaks the synchronous nesting inside BLEManager's connect-handshake continuation, but doesn't guarantee the cmd-notify characteristic has actually confirmed subscribed by the time the deferred alarm arm sends GET_ALARM_TIME — that confirmation (didUpdateNotificationStateFor) can land over a later delegate callback, past a single runloop turn. Replaces the defer with a precise signal: LiveState.connectSettled, a counter BLEManager bumps only once BOTH connectHandshakeDone (hello + SET_CLOCK sent) AND the cmd-notify characteristic's subscription is CONFIRMED active are true - whichever lands second. AppModel's alarm re-arm sink now keys off `live.$connectSettled` instead of `live.$bonded`, so SET_ALARM_TIME/ GET_ALARM_TIME always go out on a link whose reply channel is confirmed live. WHOOP 5/MG (which never sends GET_ALARM_TIME and stays behind the Experimental toggle) signals settled directly off its own handshake completion, preserving its prior re-arm-on-bond timing unchanged.
Summary
armStrapAlarm()was re-armed on every (re)bond via a Combine sink onstate.bondedthat fires synchronously, nested inside BLEManager's own connect-handshake continuation (the bonding-confirm write'sdidWriteValueFor). That put the alarm's SET_CLOCK/SET_ALARM_TIME/GET_ALARM_TIME burst in the middle of that continuation — ahead of the handshake's own clock-set, and before the cmd-notify characteristic was resubscribed post-bond.DispatchQueue.main.async) so the connect handshake finishes first — clock set once, notify channel confirmed active — before the alarm arms on a settled link. No BLE command bytes change, only send order.Diagnosis detail (for reference): the same log also showed a "strap clock 720 days in the future" warning, which I first read as a second strap-side clock fault. That turned out to be a decode bug in that build's
dataRangeNewestUnix(fixed-grid scan straddling the real timestamp) — already fixed independently in #307/#308, unrelated to this fix. The actual current-time bytes in the raw GET_DATA_RANGE frame decode correctly, so there's no evidence of a genuinely broken strap RTC in that session.Test plan
xcodegen generate && xcodebuild -project Strand.xcodeproj -scheme Strand -destination 'platform=macOS' CODE_SIGNING_ALLOWED=NO build— BUILD SUCCEEDEDxcodebuild -project Strand.xcodeproj -scheme NOOPiOS -destination 'generic/platform=iOS Simulator' CODE_SIGNING_ALLOWED=NO build— BUILD SUCCEEDED (AppModel.swift is shared with iOS)xcodebuild … test -only-testing:StrandTests/SmartAlarmWeekdayTests -only-testing:StrandTests/AlarmReadbackDecodeTests— all pass (one unrelated locale-dependent failure reproduced and cleared with-testLanguage en -testRegion US)This is a pure ordering/timing change (no protocol bytes, no schema, no analytics) so it's not a cross-platform parity item; Android's equivalent re-arm (
AppViewModel.kt) is already dispatched via a coroutineStateFlowcollector decoupled from the BLE callback thread, so it doesn't have this specific synchronous-nesting failure mode.