Remote Bolus Dev by gestrich · Pull Request #1508 · LoopKit/Loop

gestrich · 2021-05-23T14:15:44Z

This PR includes changes to the Loop module to support remote/bolus in dev. See the workspace PR for more details on the overall effort. Comments added inline for review.

ps2

Thanks Bill, could you address the comments? Otherwise, this is looking great! Thanks!

ps2 · 2021-08-31T01:52:29Z


        loopAppManager.initialize(windowProvider: self, launchOptions: launchOptions)
        loopAppManager.launch()
+        UIApplication.shared.registerForRemoteNotifications()


This is already happening in NotificationManager; we should probably only do this once.

@ps2 This is part of the proposed fix outlined in 1536 (among others). From what I can tell, we only fetch the token via registerForRemoteNotifications during onboarding. Since we can't persist the token per iOS guidelines, it will be available in-memory only and not be available for later upload after an app reboot. Here's the cases where that could pose an issue:

User chooses to setup Nightscout after onboarding.

User goes offline during onboarding process.

I think attempting to fetch it on each startup would be good protection against the above scenarios -- it will always be available to in memory and uploaded to Nightscout when needed.

If that seems reasonable, I agree we don't need it in both places. How about I remove it from onboarding? Or are there other considerations I'm not thinking of?

I updated this PR in this way. Please let me know if any any questions on that.

ps2 · 2021-08-31T01:53:54Z

                case .cancelTemporaryOverride:
                    log.default("Canceling temporary override from remote command")
                    loopManager.settings.scheduleOverride = nil
+                case .bolusEntry(let bolusValue):


We should trigger a user notification when this happens. Or is the push notification including foreground text?

I've added notifications to indicate success and failure once the remote notification is processed. That should handle the foreground notification and allow for better troubleshooting for both foreground and background states. I've added screenshots and more discussion in Zulip https://loop.zulipchat.com/#narrow/stream/144182-development/topic/Remote.20bolus.20.2F.20remote.20carbs

ps2 · 2021-08-31T01:54:10Z

+                        log.default("No max bolus detected. Aborting...")
+                    }
+                case .carbsEntry(let newEntry):
+                    log.default("Adding carbs entry.")


A local notification should happen with this too.

gestrich · 2021-09-03T20:09:48Z

@@ -375,6 +375,8 @@
 		89FE21AD24AC57E30033F501 /* Collection.swift in Sources */ = {isa = PBXBuildFile; fileRef = 89FE21AC24AC57E30033F501 /* Collection.swift */; };
 		A90EF53C25DEF06200F32D61 /* PluginManager.swift in Sources */ = {isa = PBXBuildFile; fileRef = C16DA84122E8E112008624C2 /* PluginManager.swift */; };
 		A90EF54425DEF0A000F32D61 /* OSLog.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4374B5EE209D84BE00D17AA8 /* OSLog.swift */; };


Base32 and OneTimePassword are the new git submodules that are linked in the workspace. I believe they need added to the Loop project, and to be embedded, in order for things to work.

gestrich · 2021-09-03T20:11:03Z

                    loopManager.settings.scheduleOverride = nil
+                case .bolusEntry(let bolusAmount):
+                    log.default("Enacting remote bolus entry: %{public}@", String(describing: bolusAmount))
+


Add explicit guards for the various error conditions on a remote bolus. In there is an error, that will be reported back as a user local notification.

gestrich · 2021-09-03T20:12:24Z

+                        log.default("Remote bolus higher than maximum. Aborting...")
+                        return
+                    }
+


When enacting the bolus here, we may get a specialized error from the enactBolus API which will also be presented in a local notification.... otherwise we show a local notification with a success message.

gestrich · 2021-09-03T20:12:48Z

+                        } else {
+                            NotificationManager.sendRemoteBolusNotification(amount: bolusAmount)
+                        }
+                    }


Same paradigm here for remote carb entries.

gestrich · 2021-09-03T20:13:57Z

+                        log.default("Invalid carb entry amount. Aborting...")
+                        return
+                    }
+


Added this check that we don't exceed the max carb entry amount.

gestrich · 2021-09-03T20:14:34Z

+                        log.default("Carbs higher than maximum. Aborting...")
+                        return
+                    }
+


Finally we attempt to add the carbs, and also show an error if that fails (unlikely - would be Core Data issue it seems)

gestrich · 2021-09-03T20:16:12Z

    }

    // MARK: - Remote Notification
+


Pete - Since you looked over this before I realized I should have this feature flag check. At least that is what is done in dev now.

gestrich · 2021-09-03T20:17:41Z

@@ -324,10 +332,13 @@ extension LoopAppManager: UNUserNotificationCenterDelegate {
        case LoopNotificationCategory.bolusFailure.rawValue,
             LoopNotificationCategory.pumpBatteryLow.rawValue,
             LoopNotificationCategory.pumpExpired.rawValue,


New notifications for remote bolus/carbs. More on that below.

gestrich · 2021-09-03T20:20:06Z

@@ -798,7 +798,7 @@ extension LoopDataManager {
                                      maximumBasalRatePerHour: loopSettings.maximumBasalRatePerHour,
                                      maximumBolus: loopSettings.maximumBolus,
                                      suspendThreshold: loopSettings.suspendThreshold,


This was discussed in #1536... But basically loopSettings doeesn't store the deviceToken so this will be nil. We may want to just remove that property altogether to avoid the confusion but I'd need to verify that is ok.

gestrich · 2021-09-03T20:21:17Z

@@ -67,14 +67,6 @@ extension NotificationManager {
        center.requestAuthorization(options: authOptions) { (granted, error) in
            UNUserNotificationCenter.current().getNotificationSettings { settings in
                completion(settings.authorizationStatus)


Discussed in #1536 but need to make sure this is called on each startup since we are not persisting it anywhere. Moved to LoopAppManager above.

gestrich · 2021-09-03T20:22:26Z

@@ -156,6 +148,85 @@ extension NotificationManager {
            UNUserNotificationCenter.current().add(request)
        }
    }


Helpers to send the new local notifications.

gestrich · 2021-09-03T20:24:02Z

@@ -361,6 +361,21 @@ extension RemoteDataServicesManager {

 }



Check our data services for one to validate our notification here. As a future enhancement, data services like Nightscout could provide some kind of identifier in the push payload that maps to the service. That would allow Loop to dispatch the notification to the corresponding service rather than sending it to all the services like this.

gestrich · 2021-09-03T20:25:02Z

 import Foundation
 import LoopKit
+import HealthKit



I don't believe this was being used from what I can tell. But it looks like the right structure to add new remote command errors like I needed for remote bolus/carbs.

gestrich · 2021-09-03T20:25:40Z

+    case exceedsMaxBolus
+    case exceedsMaxCarbs
+    case invalidCarbs
+


I'm using this description in the local notifications that show.

gestrich · 2021-09-03T20:26:52Z

            self = .cancelTemporaryOverride
+        }  else if let bolusValue = notification["bolus-entry"] as? Double {
+            self = .bolusEntry(bolusValue)
+        } else if let carbsValue = notification["carbs-entry"] as? Double {


Jose had this original comment on the default absorption value. I hadn't dug in but it doesn't seem critical now either.

ps2 · 2022-03-09T19:20:00Z

Token management has been resolved in dev, I believe. This PR needs to be updated to resolve conflicts with the latest dev. I'll see if I can land the NightScoutService soon.

gestrich · 2022-03-10T01:17:06Z

@ps2 conflicts + token workarounds have been removed.

ps2 · 2022-03-14T17:17:59Z

Merged these changes in 5b25c83. Forgot to squash, so they're all separate in the log. :-/

gestrich changed the title ~~Dev otp~~ Remote Bolus Dev May 23, 2021

gestrich mentioned this pull request May 23, 2021

Remote Bolus Dev LoopKit/LoopWorkspace#12

Closed

6 tasks

gestrich force-pushed the dev_otp branch from ed08a0d to cd4873a Compare May 23, 2021 15:36

gestrich force-pushed the dev_otp branch 3 times, most recently from 5b2e804 to 9efa688 Compare June 12, 2021 19:50

gestrich force-pushed the dev_otp branch 6 times, most recently from 2a07601 to c8d6c9a Compare June 20, 2021 15:59

Remote Bolus in Dev

bdcac9c

gestrich force-pushed the dev_otp branch from c8d6c9a to bdcac9c Compare June 20, 2021 16:12

bill-foreflight added 3 commits August 10, 2021 07:41

Merge dev into HEAD

5b7839e

Move Base32 to submodule

c0eef26

Merge dev upstream into dev_otp

7bf3f78

gestrich force-pushed the dev_otp branch from 9cc7f70 to 7bf3f78 Compare August 24, 2021 15:40

bill-foreflight added 2 commits August 29, 2021 08:48

Don't persist the device token LoopKit#1536

2129ddf

Register for remote notifications on startup

6200877

ps2 requested changes Aug 31, 2021

View reviewed changes

gestrich added 3 commits September 3, 2021 12:54

Local notifications for remote bolus/carb success and failure

54f5ef5

Remove redundant remote notification registration

ec6f32d

Remove absorption text which won't fit in push notification preview

75eb15d

gestrich commented Sep 3, 2021

View reviewed changes

Comment thread Loop/Managers/LoopAppManager.swift

gestrich commented Sep 3, 2021

View reviewed changes

gestrich marked this pull request as ready for review September 3, 2021 20:27

gestrich requested a review from ps2 September 3, 2021 20:27

gestrich and others added 4 commits September 3, 2021 16:48

Fix removed comment

b2d2b97

Merge dev into HEAD

d3bd000

Merge dev into HEAD

68c0168

Merge dev into HEAD

faf5acd

gestrich added 2 commits March 9, 2022 19:45

Merge dev into dev_otp

7cc0700

Remove Nightscout token management hack since fixed in dev.

a7c837b

ps2 mentioned this pull request Mar 13, 2022

Test and update of Remote Bolus for Loop #1634

Merged

ps2 closed this Mar 14, 2022

Uh oh!

Conversation

gestrich commented May 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ps2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gestrich Aug 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gestrich Sep 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gestrich Sep 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gestrich Sep 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ps2 commented Mar 9, 2022

Uh oh!

gestrich commented Mar 10, 2022

Uh oh!

ps2 commented Mar 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gestrich commented May 23, 2021 •

edited

Loading

gestrich Aug 31, 2021 •

edited

Loading

gestrich Sep 3, 2021 •

edited

Loading

gestrich Sep 3, 2021 •

edited

Loading

gestrich Sep 3, 2021 •

edited

Loading