Skip to content

Remote PR Set #3 #2009

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 15 commits into from
Jul 2, 2023
Merged

Remote PR Set #3 #2009

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
10e58b7
Remote handling rearchitecture
gestrich Apr 22, 2023
89f2293
Move remote background handling to ServicesManager.
gestrich Jun 22, 2023
36b0c94
Add delegate for ServicesManager
gestrich Jun 22, 2023
fb6a375
Remove remote details from DeviceDataManager
gestrich Jun 22, 2023
e3a2627
Remove remote terms from ServicesManagerDelegate api
gestrich Jun 22, 2023
47fa03b
Respond only to loopFinished notifications
gestrich Jun 22, 2023
82b05a7
Remove unused method
gestrich Jun 22, 2023
c598114
Rename method
gestrich Jun 23, 2023
1826af8
Update names
gestrich Jun 23, 2023
deb021e
Update names
gestrich Jun 23, 2023
b4209dc
Remove more remote references in DeviceDataManager
gestrich Jun 24, 2023
cca2741
Remove Remote 2.0 parts
gestrich Jun 24, 2023
5988cdb
Update enact override method name
gestrich Jun 24, 2023
52f143e
Move validation to ServicesManager.
gestrich Jun 25, 2023
b30bef9
Move bolus validation to ServicesManager
gestrich Jul 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 0 additions & 32 deletions Loop.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,12 +354,6 @@
A98556852493F901000FD662 /* AlertStore+SimulatedCoreData.swift in Sources */ = {isa = PBXBuildFile; fileRef = A98556842493F901000FD662 /* AlertStore+SimulatedCoreData.swift */; };
A987CD4924A58A0100439ADC /* ZipArchive.swift in Sources */ = {isa = PBXBuildFile; fileRef = A987CD4824A58A0100439ADC /* ZipArchive.swift */; };
A999D40624663D18004C89D4 /* PumpManagerError.swift in Sources */ = {isa = PBXBuildFile; fileRef = A999D40524663D18004C89D4 /* PumpManagerError.swift */; };
A99A114229A581F4007919CE /* BolusAction.swift in Sources */ = {isa = PBXBuildFile; fileRef = A99A114129A581F4007919CE /* BolusAction.swift */; };
A99A114429A5829A007919CE /* CarbAction.swift in Sources */ = {isa = PBXBuildFile; fileRef = A99A114329A5829A007919CE /* CarbAction.swift */; };
A99A114629A582A2007919CE /* OverrideAction.swift in Sources */ = {isa = PBXBuildFile; fileRef = A99A114529A582A2007919CE /* OverrideAction.swift */; };
A99A114E29A5879D007919CE /* BolusActionTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = A99A114B29A5879C007919CE /* BolusActionTests.swift */; };
A99A114F29A5879D007919CE /* CarbActionTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = A99A114C29A5879C007919CE /* CarbActionTests.swift */; };
A99A115029A5879D007919CE /* OverrideActionTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = A99A114D29A5879C007919CE /* OverrideActionTests.swift */; };
A9A056B324B93C62007CF06D /* CriticalEventLogExportView.swift in Sources */ = {isa = PBXBuildFile; fileRef = A9A056B224B93C62007CF06D /* CriticalEventLogExportView.swift */; };
A9A056B524B94123007CF06D /* CriticalEventLogExportViewModel.swift in Sources */ = {isa = PBXBuildFile; fileRef = A9A056B424B94123007CF06D /* CriticalEventLogExportViewModel.swift */; };
A9A63F8E246B271600588D5B /* NSTimeInterval.swift in Sources */ = {isa = PBXBuildFile; fileRef = 439897341CD2F7DE00223065 /* NSTimeInterval.swift */; };
Expand Down Expand Up @@ -1339,12 +1333,6 @@
A98556842493F901000FD662 /* AlertStore+SimulatedCoreData.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "AlertStore+SimulatedCoreData.swift"; sourceTree = "<group>"; };
A987CD4824A58A0100439ADC /* ZipArchive.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ZipArchive.swift; sourceTree = "<group>"; };
A999D40524663D18004C89D4 /* PumpManagerError.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PumpManagerError.swift; sourceTree = "<group>"; };
A99A114129A581F4007919CE /* BolusAction.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BolusAction.swift; sourceTree = "<group>"; };
A99A114329A5829A007919CE /* CarbAction.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CarbAction.swift; sourceTree = "<group>"; };
A99A114529A582A2007919CE /* OverrideAction.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = OverrideAction.swift; sourceTree = "<group>"; };
A99A114B29A5879C007919CE /* BolusActionTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = BolusActionTests.swift; sourceTree = "<group>"; };
A99A114C29A5879C007919CE /* CarbActionTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = CarbActionTests.swift; sourceTree = "<group>"; };
A99A114D29A5879C007919CE /* OverrideActionTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = OverrideActionTests.swift; sourceTree = "<group>"; };
A9A056B224B93C62007CF06D /* CriticalEventLogExportView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CriticalEventLogExportView.swift; sourceTree = "<group>"; };
A9A056B424B94123007CF06D /* CriticalEventLogExportViewModel.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CriticalEventLogExportViewModel.swift; sourceTree = "<group>"; };
A9B607AF247F000F00792BE4 /* UserNotifications+Loop.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "UserNotifications+Loop.swift"; sourceTree = "<group>"; };
Expand Down Expand Up @@ -2810,19 +2798,6 @@
A99A114029A581D6007919CE /* Remote */ = {
isa = PBXGroup;
children = (
A99A114129A581F4007919CE /* BolusAction.swift */,
A99A114329A5829A007919CE /* CarbAction.swift */,
A99A114529A582A2007919CE /* OverrideAction.swift */,
);
path = Remote;
sourceTree = "<group>";
};
A99A114A29A58789007919CE /* Remote */ = {
isa = PBXGroup;
children = (
A99A114B29A5879C007919CE /* BolusActionTests.swift */,
A99A114C29A5879C007919CE /* CarbActionTests.swift */,
A99A114D29A5879C007919CE /* OverrideActionTests.swift */,
);
path = Remote;
sourceTree = "<group>";
Expand All @@ -2838,7 +2813,6 @@
A9E6DFED246A0460005B1A1C /* Models */ = {
isa = PBXGroup;
children = (
A99A114A29A58789007919CE /* Remote */,
A9DFAFB224F0415E00950D1E /* CarbBackfillRequestUserInfoTests.swift */,
A963B279252CEBAE0062AA12 /* SetBolusUserInfoTests.swift */,
A9DFAFB424F048A000950D1E /* WatchHistoricalCarbsTests.swift */,
Expand Down Expand Up @@ -3868,7 +3842,6 @@
C17824A01E19CF9800D9D25C /* GlucoseThresholdTableViewController.swift in Sources */,
4372E487213C86240068E043 /* SampleValue.swift in Sources */,
437CEEE41CDE5C0A003C8C80 /* UIImage.swift in Sources */,
A99A114229A581F4007919CE /* BolusAction.swift in Sources */,
C1201E2C23ECDBD0002DA84A /* WatchContextRequestUserInfo.swift in Sources */,
1D49795824E7289700948F05 /* ServicesViewModel.swift in Sources */,
1D4A3E2D2478628500FD601B /* StoredAlert+CoreDataClass.swift in Sources */,
Expand Down Expand Up @@ -3929,7 +3902,6 @@
1D6B1B6726866D89009AC446 /* AlertPermissionsChecker.swift in Sources */,
4F08DE8F1E7BB871006741EA /* CollectionType+Loop.swift in Sources */,
A9F703772489D8AA00C98AD8 /* PersistentDeviceLog+SimulatedCoreData.swift in Sources */,
A99A114629A582A2007919CE /* OverrideAction.swift in Sources */,
E9B080B1253BDA6300BAD8F8 /* UserDefaults+LoopIntents.swift in Sources */,
C1AF062329426300002C1B19 /* ManualGlucoseEntryRow.swift in Sources */,
C148CEE724FD91BD00711B3B /* DeliveryUncertaintyAlertManager.swift in Sources */,
Expand Down Expand Up @@ -3977,7 +3949,6 @@
A9C62D842331700E00535612 /* DiagnosticLog+Subsystem.swift in Sources */,
895FE0952201234000FCF18A /* OverrideSelectionViewController.swift in Sources */,
C1EF747228D6A44A00C8C083 /* CrashRecoveryManager.swift in Sources */,
A99A114429A5829A007919CE /* CarbAction.swift in Sources */,
A9F66FC3247F451500096EA7 /* UIDevice+Loop.swift in Sources */,
439706E622D2E84900C81566 /* PredictionSettingTableViewCell.swift in Sources */,
430D85891F44037000AF2D4F /* HUDViewTableViewCell.swift in Sources */,
Expand Down Expand Up @@ -4149,12 +4120,10 @@
buildActionMask = 2147483647;
files = (
A9DF02CB24F72B9E00B7C988 /* CriticalEventLogTests.swift in Sources */,
A99A114F29A5879D007919CE /* CarbActionTests.swift in Sources */,
B44251B3252350CE00605937 /* ChartAxisValuesStaticGeneratorTests.swift in Sources */,
1D80313D24746274002810DF /* AlertStoreTests.swift in Sources */,
C1777A6625A125F100595963 /* ManualEntryDoseViewModelTests.swift in Sources */,
C16B984026B4898800256B05 /* DoseEnactorTests.swift in Sources */,
A99A115029A5879D007919CE /* OverrideActionTests.swift in Sources */,
A9A63F8E246B271600588D5B /* NSTimeInterval.swift in Sources */,
A9DFAFB324F0415E00950D1E /* CarbBackfillRequestUserInfoTests.swift in Sources */,
A963B27A252CEBAE0062AA12 /* SetBolusUserInfoTests.swift in Sources */,
Expand All @@ -4174,7 +4143,6 @@
B4CAD8792549D2540057946B /* LoopCompletionFreshnessTests.swift in Sources */,
1D8D55BC252274650044DBB6 /* BolusEntryViewModelTests.swift in Sources */,
A91E4C2124F867A700BE9213 /* StoredAlertTests.swift in Sources */,
A99A114E29A5879D007919CE /* BolusActionTests.swift in Sources */,
1DA7A84224476EAD008257F0 /* AlertManagerTests.swift in Sources */,
A91E4C2324F86F1000BE9213 /* CriticalEventLogExportManagerTests.swift in Sources */,
E9C58A7324DB4A2700487A17 /* LoopDataManagerTests.swift in Sources */,
Expand Down
155 changes: 10 additions & 145 deletions Loop/Managers/DeviceDataManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,6 @@ final class DeviceDataManager {
overrideHistory: overrideHistory,
insulinDeliveryStore: doseStore.insulinDeliveryStore
)


settingsManager.remoteDataServicesManager = remoteDataServicesManager

Expand All @@ -416,7 +415,10 @@ final class DeviceDataManager {
alertManager: alertManager,
analyticsServicesManager: analyticsServicesManager,
loggingServicesManager: loggingServicesManager,
Copy link
Contributor Author

@gestrich gestrich Jun 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@ps2 ps2 Jun 18, 2023

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@gestrich gestrich Jun 19, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@gestrich gestrich Jun 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@ps2 ps2 Jun 19, 2023

Choose a reason for hiding this comment

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

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".

Copy link
Contributor Author

@gestrich gestrich Jun 19, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@gestrich gestrich Jun 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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:

  1. 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.
  2. 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)
  3. Move the background task code, all in support of remote, from DeviceDataManager to ServicesManager.
  4. Move handleRemoteNotification from DeviceDataManager to ServicesManager.

Screenshot 2023-06-22 at 5 55 54 AM

Copy link
Contributor Author

@gestrich gestrich Jun 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

remoteDataServicesManager: remoteDataServicesManager
remoteDataServicesManager: remoteDataServicesManager,
settingsManager: settingsManager,
servicesManagerDelegate: loopManager,
servicesManagerDosingDelegate: self
)

let criticalEventLogs: [CriticalEventLog] = [settingsManager.settingsStore, glucoseStore, carbStore, dosingDecisionStore, doseStore, deviceLog, alertManager.alertStore]
Expand Down Expand Up @@ -1339,6 +1341,7 @@ extension DeviceDataManager: LoopDataManagerDelegate {
self.crashRecoveryManager.dosingFinished()
}
}

}

extension Notification.Name {
Expand All @@ -1347,152 +1350,14 @@ extension Notification.Name {
static let PumpEventsAdded = Notification.Name(rawValue: "com.loopKit.notification.PumpEventsAdded")
}

// MARK: - Remote Notification Handling
extension DeviceDataManager {

func handleRemoteNotification(_ notification: [String: AnyObject]) {
Task {
let backgroundTask = await beginBackgroundTask(name: "Remote Data Upload")
await handleRemoteNotification(notification)
await endBackgroundTask(backgroundTask)
}
}

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

defer {
log.default("Remote Notification: Finished handling")
}

guard FeatureFlags.remoteCommandsEnabled else {
log.error("Remote Notification: Remote Commands not enabled.")
return
}

let command: RemoteCommand
do {
command = try await remoteDataServicesManager.commandFromPushNotification(notification)
} catch {
log.error("Remote Notification: Parse Error: %{public}@", String(describing: error))
return
}

await handleRemoteCommand(command)
}

func handleRemoteCommand(_ command: RemoteCommand) async {

log.default("Remote Notification: Handling command %{public}@", String(describing: command))

switch command.action {
case .temporaryScheduleOverride(let overrideAction):
do {
try command.validate()
try await handleOverrideAction(overrideAction)
} catch {
log.error("Remote Notification: Override Action Error: %{public}@", String(describing: error))
}
case .cancelTemporaryOverride(let overrideCancelAction):
do {
try command.validate()
try await handleOverrideCancelAction(overrideCancelAction)
} catch {
log.error("Remote Notification: Override Action Cancel Error: %{public}@", String(describing: error))
}
case .bolusEntry(let bolusAction):
do {
try command.validate()
try await handleBolusAction(bolusAction)
} catch {
await NotificationManager.sendRemoteBolusFailureNotification(for: error, amount: bolusAction.amountInUnits)
log.error("Remote Notification: Bolus Action Error: %{public}@", String(describing: error))
}
case .carbsEntry(let carbAction):
do {
try command.validate()
try await handleCarbAction(carbAction)
} catch {
await NotificationManager.sendRemoteCarbEntryFailureNotification(for: error, amountInGrams: carbAction.amountInGrams)
log.error("Remote Notification: Carb Action Error: %{public}@", String(describing: error))
}
}
}

//Remote Overrides

func handleOverrideAction(_ action: OverrideAction) async throws {
let remoteOverride = try action.toValidOverride(allowedPresets: loopManager.settings.overridePresets)
await activateRemoteOverride(remoteOverride)
}

func handleOverrideCancelAction(_ action: OverrideCancelAction) async throws {
await activateRemoteOverride(nil)
}

func activateRemoteOverride(_ remoteOverride: TemporaryScheduleOverride?) async {
loopManager.mutateSettings { settings in settings.scheduleOverride = remoteOverride }
await remoteDataServicesManager.triggerUpload(for: .overrides)
}

//Remote Bolus

func handleBolusAction(_ action: BolusAction) async throws {
let validBolusAmount = try action.toValidBolusAmount(maximumBolus: loopManager.settings.maximumBolus)
try await self.enactBolus(units: validBolusAmount, activationType: .manualNoRecommendation)
await remoteDataServicesManager.triggerUpload(for: .dose)
self.analyticsServicesManager.didBolus(source: "Remote", units: validBolusAmount)
}

//Remote Carb Entry

func handleCarbAction(_ action: CarbAction) async throws {
let candidateCarbEntry = try action.toValidCarbEntry(defaultAbsorptionTime: carbStore.defaultAbsorptionTimes.medium,
minAbsorptionTime: LoopConstants.minCarbAbsorptionTime,
maxAbsorptionTime: LoopConstants.maxCarbAbsorptionTime,
maxCarbEntryQuantity: LoopConstants.maxCarbEntryQuantity.doubleValue(for: .gram()),
maxCarbEntryPastTime: LoopConstants.maxCarbEntryPastTime,
maxCarbEntryFutureTime: LoopConstants.maxCarbEntryFutureTime
)

let _ = try await addRemoteCarbEntry(candidateCarbEntry)
await remoteDataServicesManager.triggerUpload(for: .carb)
}

//Can't add this concurrency wrapper method to LoopKit due to the minimum iOS version
func addRemoteCarbEntry(_ carbEntry: NewCarbEntry) async throws -> StoredCarbEntry {
return try await withCheckedThrowingContinuation { continuation in
carbStore.addCarbEntry(carbEntry) { result in
switch result {
case .success(let storedCarbEntry):
self.analyticsServicesManager.didAddCarbs(source: "Remote", amount: carbEntry.quantity.doubleValue(for: .gram()))
continuation.resume(returning: storedCarbEntry)
case .failure(let error):
continuation.resume(throwing: error)
}
}
}
}

//Background Uploads
// MARK: - ServicesManagerDosingDelegate

extension DeviceDataManager: ServicesManagerDosingDelegate {

func beginBackgroundTask(name: String) async -> UIBackgroundTaskIdentifier? {
var backgroundTask: UIBackgroundTaskIdentifier?
backgroundTask = await UIApplication.shared.beginBackgroundTask(withName: name) {
guard let backgroundTask = backgroundTask else {return}
Task {
await UIApplication.shared.endBackgroundTask(backgroundTask)
}

self.log.error("Background Task Expired: %{public}@", name)
}

return backgroundTask
func deliverBolus(amountInUnits: Double) async throws {
try await enactBolus(units: amountInUnits, activationType: .manualNoRecommendation)
}

func endBackgroundTask(_ backgroundTask: UIBackgroundTaskIdentifier?) async {
guard let backgroundTask else {return}
await UIApplication.shared.endBackgroundTask(backgroundTask)
}
}

// MARK: - Critical Event Log Export
Expand Down
2 changes: 1 addition & 1 deletion Loop/Managers/LoopAppManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,7 @@ class LoopAppManager: NSObject {
guard let notification = notification else {
return false
}
deviceDataManager?.handleRemoteNotification(notification)
deviceDataManager?.servicesManager.handleRemoteNotification(notification)
return true
}

Expand Down
Loading