Backport: eager connect watchdog for InPlay/iPhone-16 wedges - #141
Conversation
Backport of #133 from next-dev, squashed. Mitigates BLE connection wedges on InPlay-firmware DASH pods paired with an iPhone 16 (any variant) or 17e: a connect that stalls with no callback is torn down and retried rather than left to time out, and the pod settings view gains a persistent notice explaining the slower connections. Also carries the later fixes from that branch: the O5 fresh-advert connect, which cut foreground connects on Omnipod 5 from about six seconds to under one; the foreground-state seed, so a pump manager built after the app has already become active does not idle-disconnect between commands; and the test-beeps button no longer gating on hasConnection. Two conflicts against this line, both because dev predates the Pod Keep Alive rework (#125). Resolved by keeping dev's accessor and taking the eager-connect logic: - shouldHoldConnection keeps podType.isDash && Storage.shared.podKeepAlive.value.keepsPodConnectedInBackground rather than podKeepAliveKeepsConnectedInBackground - the pairing auto-enable keeps Storage.shared.podKeepAlive.value while adopting the consolidated UIDevice.hasPossibleInPlayBLEIssues Does not build against LoopWorkspace/dev as it stands, for reasons that predate this change -- see the pull request.
5df3bb2 to
5a7350a
Compare
|
Rebased onto current Conflicts, and how they were resolvedBoth were the same shape: #142 introduced newer accessors on 1.
if isAppForeground { return true }
if let peripheral = keepAlivePeripheral, shouldUseEagerConnect(for: peripheral) { return true }
return podKeepAliveKeepsConnectedInBackgroundByte-identical to 2. A genuine split — took each side's newer half, which is again what if self.usingInPlayPod == true && UIDevice.hasPossibleInPlayBLEIssues { // ours (consolidated helper)
if self.state.podKeepAlive == .disabled { // dev's, from #142
The original blocker is goneThis PR was opened noting it couldn't build until Build
That required one unrelated fix: Still a draft: hardware testing on an InPlay pod + iPhone 16/17e is the remaining gap. |
itsmojo
left a comment
itsmojo
left a comment
There was a problem hiding this comment.
Merge looks good and basic functionality checks out on a non-iPhone 16/17e. Unfortunately I am unable to perform any real testing of the new eager connect functionality without having an iPhone that exhibits the problematic behavior with an InPlay BLE DASH pod.
setBLEHeartbeatRequest branched on podType.mayUseRileyLink, which is true for DASH as well as Eros because DASH can use a RileyLink under the Pod Keep Alive option. A DASH pod on direct BLE with Pod Keep Alive off therefore took the RileyLink branch and only set timerTickEnabled on a RileyLink provider that has no devices. provideHeartbeat was never assigned and BlePodComms.setHeartbeatRequest was never called, so the delayed-connect probe that produces periodic background wakes was never scheduled. issueHeartbeatIfNeeded() also gates on provideHeartbeat, so even a probe that did fire would not have notified Loop. Loop was requesting the heartbeat correctly -- DeviceDataManager sets pumpManagerMustProvideBLEHeartbeat when the CGM cannot provide one -- and the request was logged and then dropped. With no wake source the app stayed suspended in the background until something else woke it. Seen as Loop Failure alerts after gaps of 20 to 264 minutes that end the instant the app is foregrounded; a report from an affected DASH pod shows provideHeartbeat: false moments after a setBLEHeartbeatRequest log line. Only reachable when the CGM cannot supply the heartbeat itself, so it needs a remote or networked CGM such as Nightscout. A BLE Dexcom provides its own heartbeat and masks it. Branch on whether a RileyLink is actually in use instead: Eros always, DASH only under the RileyLink Pod Keep Alive option. The other mayUseRileyLink call sites want the "may" semantics and are unchanged.
|
Added a second commit (0f3295f) for a separate background-wake bug found while testing this branch on a DASH pod with Nightscout Remote CGM.
Loop was asking correctly: It needs a CGM that cannot supply its own heartbeat, so remote/networked CGMs like Nightscout — a BLE Dexcom masks it entirely. That is presumably why it has gone unreported. The fix branches on whether a RileyLink is actually in use (Eros always; DASH only under the RileyLink Pod Keep Alive option) rather than whether one is possible. I checked the other six This is orthogonal to the watchdog, so happy to split it into its own PR against Separately, one thing in this branch worth a look. The new |
TestThis test applies the eager-connect backport to iAPS.
ConfigurationUse SE 2nd gen phone running iOS 18 using Nightscout as a CGM (no heartbeat) and OmnipodKit Test Narrative
Updating OmnipodKit to include PR 141This is starting from iAPS directory, dev branch, where each submodule has a remote pointing to the original repository with a unique branch name. This is the first commit from 141 (I forgot about the second one, but that only affects the app staying awake when the CGM does not have a heartbeat. I built to the test phone and confirmed the DASH selection paired with an rPi DASH pod. At this point I remembered there are 2 commits, so I added the next one: I rebuilt, unplugged the phone from Xcode and locked the phone at 14:04 PDT |
The zombie check in didDiscover treats a pairable pod heard while .connecting with no watchdog as a stale connect and cancels it. That is right for a connect abandoned by an earlier attempt, but it cannot tell that case from a connect we issued moments ago: a peripheral keeps advertising until the link is actually established, and CoreBluetooth reports .connecting from the instant connect() is called, so an advert inside the first second or two is the normal case. discoverPods pre-connects any already-known disconnected pod and then starts the discovery scan. timedConnect only arms the watchdog when shouldUseEagerConnect is true, which needs an affected iPhone model, so on any other phone that pre-connect is unsupervised. The scan hears the pod ~0.8s later at state=.connecting, the zombie branch cancels it, the teardown timer reissues timedConnect (again unsupervised), the next advert ~1.5s later cancels that, and pairing loops at the advert interval without ever completing. Two Xcode captures from an iPhone SE pairing an Omnipod 5 show 62 and 104 consecutive cycles of [pairing] heard pod ... pairable=true state=1 [pairing] ... stuck in .connecting with no watchdog -- cancelling zombie connect [pairing] reconnecting to pairable pod ... after zombie teardown Pairing with a pod that is not yet a known device takes the state=0 path and races the same window: it worked in earlier testing because the ~1.1s connect landed before the next advert. Stamp when the current pairing connect was issued and only treat a .connecting pod as a zombie once that connect is older than pairingConnectZombieSeconds (default 5s, past a healthy connect and inside the pairing budget). The stamp is separate from connectRequestedAt, which is deliberately kept across retries for the latency metric, and is cleared on didConnect and didFailToConnect.
|
@marionbarker — your two Trio captures ( What the logs show. Both captures are the same loop, every ~1.6s, forever (62 cycles in one, 104 in the other): Mechanism. Why Loop and iAPS paired for you. Timing, not immunity. Those started with a pod that was not yet a known device, so no pre-connect; the first advert arrived at Fix (5a32565). Stamp when the current pairing connect was issued and only treat a Retest. I suspect the current pod is the one left half-paired by the SPS2 failure, so a fresh pod is the right test regardless. On the fixed build the log should have zero The zombie branch is also in |
…istic A "zombie" was a pending connect left behind by a pairing attempt that gave up without cancelling. The didDiscover branch tried to detect one from an advert heard while the pod was .connecting, but that is equally true of a connect issued a second ago, and on phones where the watchdog does not arm it cancelled every in-flight pairing connect and looped (see 5a32565, which only narrowed the misfire). Remove the guess and close the source instead: - endPodDiscovery cancels any .connecting peripheral. The previous autoConnectIDs exemption is what leaked the orphans: discovery adds the pod to autoConnectIDs before ending, so any later abandoned connect to a pod that had once been found was never cancelled. Only a .connected paired pod keeps the exemption. - discoverPods cancels any unsupervised .connecting peripheral before issuing its own connects, so during discovery the only pending connect is ours and the discovery timeout owns it. This also covers a connect left pending across an app suspend or kill, which comes back through state restoration already .connecting. didDiscover goes back to connecting only when .disconnected. The pairingConnectIssuedAt stamp and pairingConnectZombieSeconds from 5a32565 go with the heuristic.
|
Pushed d70d7b8, which supersedes 5a32565 — Pete pointed out the right fix, and it is simpler. A zombie is only ever a pending connect that some path issued and then abandoned without So d70d7b8 removes the heuristic (and the 5s gate and its state with it) and closes the source:
|
3 participants
Backport of #133 to the
devline, squashed to a single commit.What it brings
Mitigation for BLE connection wedges on InPlay-firmware DASH pods paired with an iPhone 16 (any variant) or 17e: a connect that stalls with no callback is torn down and retried rather than left to time out, plus a persistent notice in pod settings explaining the slower connections and a detail screen behind it.
It also carries the later fixes from that branch:
didBecomeActive, soisAppForegroundstayed false and the link was idle-disconnected ~4s after each command while the user was on screen.hasConnection.Conflicts
Rebased onto
devat2c603f7. Two conflicts, both with #142 (the Pod Keep Alive backport), which introduced newer accessors that this branch predated.Neither resolution is a judgment call — each reproduces exactly what
next-devalready carries, that being the lineage where both changes coexist.1.
BluetoothManager.shouldHoldConnection— kept dev'spodKeepAliveKeepsConnectedInBackgroundaccessor and layered the eager-connect early return above it:Byte-identical to
next-dev.2.
OmniPumpManagerpairing auto-enable — a genuine split; took each side's newer half, which is again whatnext-devhas:Storage.shared.podKeepAlive.valueno longer appears anywhere in the tree. The backport removes dev's localOmniPumpManager.iPhoneWithPossibleInPlayIssuesand shipsOmnipodKit/Common/UIDevice.swiftwith the consolidated helper, so there is no leftover duplicate — zero references remain to the old name.Verification
** BUILD SUCCEEDED **, exit 0 — LoopWorkspace scheme, iPhone 17 simulator, LoopWorkspacedevwith this branch checked out for OmnipodKit.One unrelated fix was required to get there:
devstill has theusesRileyLinkbreak that #146 fixed onmain, soOmnipodKit/devcannot currently build for any simulator destination, with or without this PR. Raised separately as #148. With that applied, this branch builds clean — it was the only failure.Not exercised on hardware. Testing on an InPlay pod with an iPhone 16 or 17e is the remaining gap, which is why this stays a draft.
Note on the earlier "does not build against LoopWorkspace/dev" section
That section is obsolete and has been removed. It described
OmnipodKit/devrequiringPumpHeartbeatRequest, a LoopKit type the pinned LoopKit did not define. That has since landed via LoopKit #599 —LoopWorkspace/devnow pins LoopKita5beee96, which defines it. The dependency is satisfied and this PR is no longer blocked on a LoopKit bump.