Remote PR Set #3 #2009

gestrich · 2023-06-10T14:54:57Z

This is part of a collection of foundational PRs that need to merge at the same time.

Loop
LoopKit
NightscoutService
TidepoolService

Remote Delegation Update

The remote command flow has been refactored to use delegation from Loop to enact commands, rather than the recently merged type system.

Remote Errors in Nightscout

Support is added for uploading remote errors from Loop to Nightscout as a note. This was discussed in Zulip several months ago. This is not the same as our future remote 2.0 work that will track command status in Mongo. It is instead a stopgap to provide remote feedback to caregivers when commands are rejected, until we can roll out Remote 2.0. In addition, the rearchitecture mentioned above made it tricky to maintain the current level of in-app error feedback, so this comes at a convenient time.

Remote 2.0 Preview

For terminology, let's refer to the pre-work we've been merging as "Foundational" and the future implementation of remote 2.0 just as "Remote 2.0".

This set of PRs is different from the prior, as it includes the Remote 2.0 features in addition to the foundational. But I plan to remove these Remote 2.0 parts just prior to merge. PR comments point out what parts are Remote 2.0 so it should be clear. You could either ignore these sections or view them for context on what is coming later. Note these Remote 2.0 changes are not ready for merge so you will see some TODOs in these sections.

Once you approve of the parts that are intended for merge, I'll remove the Remote 2.0 parts.

So why did I leave the Remote 2.0 features for the initial review? It can be time consuming to tease apart the Foundational parts from Remote 2.0 for a PR as they are very connected. If there were significant change requests once you reviewed this, that could lead to a lot of rework to Remote 2.0 things. So it is more time efficient to keep these components together until you get through your first pass.

gestrich · 2023-06-10T14:57:43Z

Loop/Managers/DeviceDataManager.swift

@@ -409,7 +408,8 @@ final class DeviceDataManager {
            alertManager: alertManager,
            analyticsServicesManager: analyticsServicesManager,
            loggingServicesManager: loggingServicesManager,


This allows the DeviceDataManager to handle actions for the ServicesManager -- via a new RemoteCommandHandler protocol. When a plugin sends an action to the ServicesManager, it needs to get that action to the DeviceDataManager.

DeviceDataManager already exposes methods for dosing. Does it need separate methods for "remote" dosing? If so, why?

I'm wondering how ServicesManager can communicate with DeviceDataManager, assuming DeviceDataManager is still where dosing/settings things will live. Right now there is not a reference I see going that direction. So this delegate provides a bridge for the ServicesManagerManager to talk to the DeviceDataManager to enact doses, change settings, etc.

Is it the delegate name of RemoteActionDelegate that I should change? Like DeviceDataManagerDelegate?

Or were you thinking these remote things be relocated to RemoteDataServicesManager? (dosing, settings)? (That's another place where the reference goes from DeviceDataManager -> RemoteDataServicesManager but not the other way around. )

We also need to think about where incoming push notifications are handled as well, if we are moving all these, as those need to be propogated back to the ServicesManager too.

ServicesManager will still need to communicate with DeviceDataManager for dosing. But it should only be for dosing. I don't think DeviceDataManager needs to know about anything "remote".

Ok I'll look at keeping the delegate, as we still need a bridge between these classes, but I'll rename it -- maybe ServicesManagerDelegate (I had the delegation relationship backwards in my prior comment).

Then I'll remove as much of the remote things from DeviceDataManager into the ServicesManager or RemoteDataServicesManager.

Do you want these changes as part of this PR set or a future one? This change seems big enough that it may be easier to test/review separately from all this. Whatever you prefer.

I documented the relationships between these managers to get the big picture of how this update will fit. A few notes on the plans for this update:

The RemoteActionDelegate, implemented by DeviceDataManager in this PR, will be renamed to ServicesManagerDelegate (red line in diagram). That will be for dosing/settings updates. I'll rename the delegate methods to drop handleRemote from prefix.

Update my dosing/settings error messages in DeviceDataManager to remove any 'Remote' phraseology to be consistent with the rest of this rearchitecture. I plan to not change anything else in those methods, including remote trigger uploads and analytics uploads (will stay in DeviceDataManager)

Move the background task code, all in support of remote, from DeviceDataManager to ServicesManager.

Move handleRemoteNotification from DeviceDataManager to ServicesManager.

@ps2 I've made the changes that I describe in the last comment and pushed to this branch. Let me know if this is what you are looking for.

Note there is a still the RemoteActionDelegate (not represented in this diagram) that lives between the RemoteDataServices and ServicesManager. Then there is the new delegate between ServicesManager and DeviceDataManager which has no references to "Remote" like you mentioned.

I have not adjusted the naming in RemoteActionDelegate per your comments in the LoopKit PR. I'll look at that separately. I think that is the last of your feedback thus far.

Loop/Managers/DeviceDataManager.swift

gestrich · 2023-06-10T18:15:31Z

Loop/Managers/DeviceDataManager.swift

    }

-    func handleRemoteCommand(_ command: RemoteCommand) async {
+    //Remote Overrides
+


With the removal of the OverrideAction type, this validation logic needed moved into here.

gestrich · 2023-06-10T18:17:29Z

Loop/Managers/DeviceDataManager.swift

@@ -1443,30 +1442,140 @@ extension DeviceDataManager {
        await remoteDataServicesManager.triggerUpload(for: .overrides)
    }



Moved from OverrideAction

gestrich · 2023-06-10T18:28:14Z

Loop/Managers/DeviceDataManager.swift

+            }
+        }
+    }
+
    //Remote Bolus



Moved from BolusAction

gestrich · 2023-06-10T18:31:58Z

Loop/Managers/DeviceDataManager.swift

+            case .exceedsMaxBolus:
+                return NSLocalizedString("Exceeds maximum allowed bolus in settings", comment: "Remote command error description: bolus exceeds maximum bolus in settings.")
+            }
+        }
    }

    //Remote Carb Entry



Move from CarbAction

gestrich · 2023-06-10T18:32:23Z

Loop/Managers/LoopDataManager.swift

@@ -903,6 +903,8 @@ extension LoopDataManager {
        }

        updateRemoteRecommendation()


REMOTE 2.0: WILL REMOVE PRE-MERGE

Loop/Managers/DeviceDataManager.swift

gestrich · 2023-06-10T18:35:57Z

Loop/Managers/NotificationManager.swift

@@ -79,27 +79,6 @@ extension NotificationManager {

    // MARK: - Notifications



Getting rid of the these parsing/invalid error notifications for now as I think is desired -- this logic is now removed from the Loop domain with this rearchicture so it is tricky to present anyways. All of these will show in Nightscout as notes though.

gestrich · 2023-06-10T18:37:37Z

Loop/Managers/RemoteDataServicesManager.swift

+        let service = try serviceForPushNotification(notification)
+        return try await service.handleRemoteNotification(notification)
+    }
+


REMOTE 2.0: WILL REMOVE PRE-MERGE

Referring to just processPendingRemoteCommands

gestrich · 2023-06-10T18:39:40Z

Loop/Managers/RemoteDataServicesManager.swift

-        let serviceIdentifier = notification["serviceIdentifier"] as? String ?? defaultServiceIdentifier
-        return remoteDataServices.first(where: {$0.serviceIdentifier == serviceIdentifier})
-    }
-


This method is now removed as we will not fetch a command from the plugin now with the new delegation flow.

We are not removing push notifications for commands, right? Just adding periodic fetch as a backup, for when push does not work.

Push is still the most timely way to wake Loop to handle a remote command, but it may be not 100% reliable.

Yeah push notifications are not being removed with these updates or Remote 2.0.

gestrich · 2023-06-10T18:44:16Z

Loop/Managers/ServicesManager.swift

@@ -201,6 +205,40 @@ extension ServicesManager: ServiceDelegate {
        log.default("Service with identifier '%{public}@' deleted", service.serviceIdentifier)
        removeActiveService(service)
    }
+


The ServiceDelegate protocol conforms now to the RemoteCommandHandler and these methods are required.

gestrich · 2023-06-10T18:47:52Z

Loop/Managers/ServicesManager.swift

+    func handleRemoteOverrideCancel() async throws {
+        try await remoteCommandHandler?.handleRemoteOverrideCancel()
+    }
+


These are the 2 notifications we still show for Loop domain issues/success -- Carbs an Bolus. The error is propogated back to the plugin so it can still do what it needs to (right now log as NS note)

gestrich · 2023-06-10T21:45:43Z

Loop/View Controllers/CarbEntryViewController.swift

@@ -405,8 +405,8 @@ final class CarbEntryViewController: LoopChartsTableViewController, Identifiable
                        cell.datePicker.preferredDatePickerStyle = .wheels
                    }
                #endif


Use new constants for these

gestrich · 2023-06-17T16:23:20Z

Loop/Managers/LoopDataManager.swift

@@ -2204,6 +2206,14 @@ protocol LoopDataManagerDelegate: AnyObject {
    ///   - units: The recommended bolus in U
    /// - Returns: a supported bolus volume in U. The volume returned should be the nearest deliverable volume.
    func roundBolusVolume(units: Double) -> Double


REMOTE 2.0: WILL REMOVE PRE-MERGE

gestrich · 2023-06-17T16:25:41Z

Loop/Managers/NotificationManager.swift

@@ -161,11 +140,11 @@ extension NotificationManager {
    }

    @MainActor


These are the only in-app failure notifications I left behind -- for bolus/carb specific issues (i.e. not parsing errors or OTP erorrs -- things like "The bolus is too big").

We may decide we don't want these either but I left them for now as they were pretty specific errors.

Things like "the bolus is too big" is something the caregiver ideally should be told at the time of entering the command.

Yeah the caregiver app does prevent really large values from being entered (not sure about Nightscout) but doesn't yet have support for honoring the Looper's dosing maximums.

gestrich · 2023-06-17T16:27:00Z

Loop/Managers/RemoteDataServicesManager.swift

+            }
+        }
+    }
+


This here should really be the same - it just got moved around I believe.

gestrich · 2023-06-17T16:28:07Z

Loop/Managers/ServicesManager.swift

+            throw error
+        }
+    }
+


REMOTE 2.0: WILL REMOVE PRE-MERGE

gestrich · 2023-06-17T16:28:21Z

LoopTests/Managers/LoopDataManagerDosingTests.swift

@@ -27,6 +27,8 @@ class MockDelegate: LoopDataManagerDelegate {
        self.recommendation = automaticDose.recommendation
        completion(error)
    }


REMOTE 2.0: WILL REMOVE PRE-MERGE

gestrich · 2023-06-17T16:30:03Z

LoopTests/Models/Remote/BolusActionTests.swift

@@ -1,101 +0,0 @@
-//


Unfortunately these tests I recently added are not easily supported now with these changes. I may try to add these back with some architecture improvements later.

ps2 · 2023-06-18T15:26:04Z

Loop/Managers/DeviceDataManager.swift

-        } catch {
-            log.error("Remote Notification: Parse Error: %{public}@", String(describing: error))
-            return
+    func processPendingRemoteCommands() {


As much as we can, "remote" handling responsibility/concerns should be isolated to RemoteDataServicesManager/ServicesManager. I'm not sure DeviceDataManager needs to change at all.

ps2 · 2023-06-18T16:47:17Z

Loop/Managers/DeviceDataManager.swift

@@ -1357,84 +1366,74 @@ extension Notification.Name {
 }

 // MARK: - Remote Notification Handling
-extension DeviceDataManager {
+
+extension DeviceDataManager: RemoteActionDelegate {

    func handleRemoteNotification(_ notification: [String: AnyObject]) {


I think this should probably be moved to ServicesManager.

Use notification to detect loop completion. Move handleRemoteNotification to ServicesManager

ps2

I think this is closer to landing, but if possible, I still think some further re-arranging of responsibilities would keep things more organized. See the other comments for details. If you really want to land this now, we could, and tackle my comments as a later refactoring. Let me know.

ps2 · 2023-06-24T22:29:59Z

Loop/Managers/DeviceDataManager.swift

@@ -416,7 +415,8 @@ final class DeviceDataManager {
            alertManager: alertManager,
            analyticsServicesManager: analyticsServicesManager,
            loggingServicesManager: loggingServicesManager,
-            remoteDataServicesManager: remoteDataServicesManager
+            remoteDataServicesManager: remoteDataServicesManager,
+            servicesManagerDelegate: self


Dosing (eventually) needs to be delegated to DeviceDataManager, but overrides, carb, settings, are more the domain of LoopDataManager. And we may want to even run dosing through LoopDataManager, as a bolus command received mid-loop is something to consider.

ps2 · 2023-06-24T22:44:29Z

Loop/Managers/DeviceDataManager.swift

+    func deliverCarbs(amountInGrams: Double, absorptionTime: TimeInterval?, foodType: String?, startDate: Date?) async throws {
+
+        let absorptionTime = absorptionTime ?? carbStore.defaultAbsorptionTimes.medium
+        if absorptionTime < LoopConstants.minCarbAbsorptionTime || absorptionTime > LoopConstants.maxCarbAbsorptionTime {


I think validation and error reporting for remote commands should remain in ServiceManager. Local commands tend to be validated directly in the local UIs before being commanded.

Move ServiceManager delegation to LoopDataManager

ps2

LGTM. Just a note with one further small change if you want to tackle it now. Otherwise, I'll plan on merging this soon.

ps2 · 2023-06-30T14:31:33Z

Loop/Managers/DeviceDataManager.swift


-        defer {
-            log.default("Remote Notification: Finished handling")
+        guard amountInUnits > 0 else {


This validation could go into remote services manager, along with the error definitions. These are errors to report to the remote services, I think. Won't block on this, though.

This will put a dependency in ServicesManger on LoopSettings which sounds like you are ok with. I'll go ahead and make that change pre-merge. More contextual notes on the last changes are here https://loop.zulipchat.com/#narrow/stream/358458-Loop-Caregiver-App/topic/Merge.20Strategies/near/369827210

I've made these changes here and don't have any other planned changes. Let me know if anything is needed or merge if you are ready.

gestrich commented Jun 10, 2023

View reviewed changes

Loop/Managers/DeviceDataManager.swift Show resolved Hide resolved

gestrich commented Jun 10, 2023

View reviewed changes

Loop/Managers/DeviceDataManager.swift Show resolved Hide resolved

gestrich commented Jun 10, 2023

View reviewed changes

Loop/Managers/DeviceDataManager.swift Outdated

}

}

}

//Remote Bolus

Copy link

Contributor Author

gestrich Jun 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved from BolusAction

gestrich commented Jun 10, 2023

View reviewed changes

Loop/Managers/DeviceDataManager.swift Outdated Show resolved Hide resolved

gestrich commented Jun 10, 2023

View reviewed changes

gestrich force-pushed the feature/2023/06/bg/remote-command-service-refactor branch from b1597ca to 1bbf0a1 Compare June 17, 2023 15:51

gestrich commented Jun 17, 2023

View reviewed changes

Loop/Managers/ServicesManager.swift

throw error

}

}

Copy link

Contributor Author

gestrich Jun 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REMOTE 2.0: WILL REMOVE PRE-MERGE

gestrich commented Jun 17, 2023

View reviewed changes

This was referenced Jun 17, 2023

Remote PR Set #3 LoopKit/TidepoolService#13

Merged

Remote PR Set #3 LoopKit/LoopKit#488

Merged

Remote PR Set #3 ps2/NightscoutService#41

Merged

gestrich marked this pull request as ready for review June 17, 2023 16:54

Remote handling rearchitecture

10e58b7

ps2 reviewed Jun 18, 2023

View reviewed changes

gestrich force-pushed the feature/2023/06/bg/remote-command-service-refactor branch from 1bbf0a1 to 10e58b7 Compare June 19, 2023 09:33

gestrich added 11 commits June 22, 2023 17:50

Move remote background handling to ServicesManager.

89f2293

Use notification to detect loop completion. Move handleRemoteNotification to ServicesManager

Add delegate for ServicesManager

36b0c94

Remove remote details from DeviceDataManager

fb6a375

Remove remote terms from ServicesManagerDelegate api

e3a2627

Respond only to loopFinished notifications

47fa03b

Remove unused method

82b05a7

Rename method

c598114

Update names

1826af8

Update names

deb021e

Remove more remote references in DeviceDataManager

b4209dc

Remove Remote 2.0 parts

cca2741

ps2 reviewed Jun 24, 2023

View reviewed changes

gestrich added 2 commits June 24, 2023 19:49

Update enact override method name

5988cdb

Move validation to ServicesManager.

52f143e

Move ServiceManager delegation to LoopDataManager

ps2 approved these changes Jun 30, 2023

View reviewed changes

Move bolus validation to ServicesManager

b30bef9

ps2 merged commit 475fa61 into LoopKit:dev Jul 2, 2023

		@@ -1443,30 +1442,140 @@ extension DeviceDataManager {
		await remoteDataServicesManager.triggerUpload(for: .overrides)
		}

		@@ -903,6 +903,8 @@ extension LoopDataManager {
		}

		updateRemoteRecommendation()

		@@ -79,27 +79,6 @@ extension NotificationManager {

		// MARK: - Notifications

		@@ -161,11 +140,11 @@ extension NotificationManager {
		}

		@MainActor

Remote PR Set #3 #2009

Remote PR Set #3 #2009

Uh oh!

Conversation

gestrich commented Jun 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Remote Delegation Update

Remote Errors in Nightscout

Remote 2.0 Preview

Uh oh!

gestrich Jun 10, 2023 • 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!

gestrich Jun 19, 2023 • 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!

gestrich Jun 22, 2023 • 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 Jun 22, 2023 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gestrich Jun 10, 2023 • 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 Jun 10, 2023 • 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 Jun 10, 2023 • 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!

gestrich Jun 10, 2023 • 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!

gestrich Jun 17, 2023 • 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

gestrich commented Jun 10, 2023 •

edited

Loading

gestrich Jun 10, 2023 •

edited

Loading

gestrich Jun 19, 2023 •

edited

Loading

gestrich Jun 22, 2023 •

edited

Loading

gestrich Jun 22, 2023 •

edited

Loading

gestrich Jun 10, 2023 •

edited

Loading

gestrich Jun 10, 2023 •

edited

Loading

gestrich Jun 10, 2023 •

edited

Loading

gestrich Jun 10, 2023 •

edited

Loading

gestrich Jun 17, 2023 •

edited

Loading

gestrich Jul 1, 2023 •

edited

Loading