LOOP-1487: add basal rates informational screen + editor#15
Conversation
nhamming
left a comment
nhamming
left a comment
There was a problem hiding this comment.
I think the pod specific parameters should be taken from the plugin, not made redundant here. Is this possible?
| switch prescription.pump { | ||
| case .dash: | ||
| // ANNA TODO: make this (1, 600) once guardrail bug is resolved | ||
| return (0...600).map { round(Double($0) / Double(1/0.05) * 100) / 100 } |
There was a problem hiding this comment.
Shouldn't this be pulled from Pod.swift so that a change there will cause a change here?
There was a problem hiding this comment.
Agreed, though I wasn't sure about whether the design of the submodules would make it so that TidepoolService would be unable (or at least not supposed to) have a dependency on DashKit - @darinkrauss do you have any thoughts? When I build I get a circular dependency warning
There was a problem hiding this comment.
@novalegra TidepoolService cannot have a dependency on DashKit as it is (eventually) intended to be used by DIY Loop and DashKit is really only Tidepool Loop only.
There was a problem hiding this comment.
@nhamming How should we move forward with this? I know from conversations with Pete that the images for devices are another pump-specific asset that we'll need to correctly provide, but are currently hard coded. I could duplicate Pod.swift and add that file to TidepoolService so the therapy settings flow has access to the data.
There was a problem hiding this comment.
Can we make these arguments to the init here? Is there a point in top-level Loop where the flow is constructed where these arguments can be passed in?
There was a problem hiding this comment.
The other snag is that pump setup happens after onboarding, right? So we wouldn't even have an instantiated PumpManager at the Loop level.. really we need access to instance-level PumpManager requirements in a static context—which is mostly doable (save for deliverable increments differences in MDT x23 and x22 in DIY), but would take some refactoring.
We need to know the pump from the prescription, then have a way to query the appropriate PumpManager at the top-level—which is a tricky flow of data since top-level Loop shouldn't have knowledge of prescriptions. 🤔
I don't think this is an easy architecture question to address. I'd recommend leaving these hard-coded for now with TODOs, and raising this item as a discussion topic during dev sync this week (or perhaps once Pete is back).
There was a problem hiding this comment.
Plus the user will eventually able to change the pump type within the settings flow... But with an enum for the pump types/managers, you could implement functions based on that. I'll add the topic to dev sync
There was a problem hiding this comment.
We really need to give some thought on how we will enable the Tidepool Loop workflows while also not confusing the DIY Loop integration. I'm a bit concerned about trying to include both in TidepoolService plugin.
There was a problem hiding this comment.
@novalegra I'm good with the TODO and discussing at the dev sync to find a way forward.
| private var maximumBasalScheduleEntryCount: Int { | ||
| switch prescription.pump { | ||
| case .dash: | ||
| return 24 |
There was a problem hiding this comment.
Similar comment about redundant information here and in Pod.swift.
| private var syncBasalRateSchedule: Int { | ||
| switch prescription.pump { | ||
| case .dash: | ||
| return 24 |
There was a problem hiding this comment.
Similar comment about redundant information here and in Pod.swift.
novalegra
requested review from
darinkrauss,
nhamming and
rickpasetto
and removed request for
mpangburn
darinkrauss
left a comment
darinkrauss
left a comment
There was a problem hiding this comment.
Other than the various comments about pump dependencies that we need to figure out, LGTM! 
| switch prescription.pump { | ||
| case .dash: | ||
| // ANNA TODO: make this (1, 600) once guardrail bug is resolved | ||
| return (0...600).map { round(Double($0) / Double(1/0.05) * 100) / 100 } |
There was a problem hiding this comment.
We really need to give some thought on how we will enable the Tidepool Loop workflows while also not confusing the DIY Loop integration. I'm a bit concerned about trying to include both in TidepoolService plugin.
nhamming
left a comment
nhamming
left a comment
There was a problem hiding this comment.
Unblocking given the discussion and plan on how to move forward.
Updated translations from Lokalise on Sat Aug 19 15:10:08 CDT 2023
4 participants
https://tidepool.atlassian.net/browse/LOOP-1487