nate/fix/LOOP-1927/manual-glucose-entry #256

nhamming · 2020-09-16T17:05:44Z

https://tidepool.atlassian.net/browse/LOOP-1927

allow manual glucose entry without carb entry

rickpasetto

LGTM! (I can't wait until I write some unit tests for this 🤷 )

… be display correctly

nhamming · 2020-09-18T14:15:44Z

@ps2 I made a slight modification to the DeviceDataManager when it retrieves a glucose sensorState. Love to get your initial thoughts on this.

Also see tidepool-org/LoopKit#244

ps2

LGTM. I have several suggestions in here, that I think would improve things, but none of them are critical.

ps2 · 2020-09-18T14:51:17Z

Loop Status Extension/StatusViewController.swift

+
+        if view.bounds.width < 375 {
+            // need to adjust for narrow display
+            hudView.adjustViewsForNarrowDisplay = true


Can't the hud adjust its view itself if it's not given enough room? Adjust views depending on the size given would seem to be the view's responsibility, not a user of the view.

Overlooked this comment. Will do.

ps2 · 2020-09-18T15:00:09Z

Loop/Models/ManualGlucoseState.swift

+import Foundation
+import LoopKit
+
+struct ManualGlucoseState: SensorDisplayable {


ManualGlucoseState -> ManualGlucoseDisplayable? And SensorDisplayable -> GlucoseDisplayable? The latter is LoopKit, I know, and maybe we do it later, but if this protocol is used to display both sensor and manual glucose data, the name could use updating.

I'll definitely update GlucoseDisplayable now. I was confused when writing the code and happy to take this suggestion.

ps2 · 2020-09-18T15:04:11Z

Loop/Managers/DeviceDataManager.swift

+    func sensorState(for glucose: GlucoseSampleValue?) -> SensorDisplayable? {
+        if let glucose = glucose, glucose.wasUserEntered {
+            // the CGM manager needs to determine the glucoseValueType for a manual glucose based on its managed glucose thresholds
+            let glucoseValueType = (cgmManager as? CGMManagerUI)?.glucoseValueType(for: glucose)


It's probably not right that we're asking the CGM to categorize the manual glucose value. It suggest that maybe glucose categorization should be a Loop concern. What do we do if we don't have a CGM? If this is too big, let's not tackle it here, but I think this is a sign we need to refactor glucose range categorization.

@ps2 The scenario when the CGM is not available was reported to design. They agree that ideally Loop manages the user glucose thresholds. However, they don't have a big concern with the style of the manually entered glucose being normal in the scenario when no CGM is setup. However, they would like to confirm there is no risk in a build at some point.

ps2 · 2020-09-18T15:05:18Z

Loop/Managers/DeviceDataManager.swift


-    var sensorState: SensorDisplayable? {
-        return cgmManager?.sensorState
+    func sensorState(for glucose: GlucoseSampleValue?) -> SensorDisplayable? {


sensorState -> glucoseDisplay?

ps2 · 2020-09-18T15:52:27Z

LoopUI/ViewModel/CGMStatusHUDViewModel.swift


        glucoseValueTintColor = sensor?.glucoseValueType?.glucoseColor ?? .label
-        glucoseTrendTintColor = sensor?.glucoseValueType?.trendColor ?? .glucoseTintColor
+        glucoseTrendTintColor = isManualGlucose ? .destructive : sensor?.glucoseValueType?.trendColor ?? .glucoseTintColor


Hmm, when the sensor is in a warmup state, and the user enters a manual glucose value, there is not really any "error" that needs attending to. I guess this is a question for design/product. It seems like communicating the value as a manual entry is important, but painting it in error/warning colors when everything is operating as expected and user attention is not required is not great.

I understand that when the CGM is failing in some way, we don't want to paint over the failing state with a manual glucose entry that makes everything look ok.

@ps2 yes, I'm working with Matt to determine all the possible states. Currently the known states include a red !, red or glucose colour +, and glucose colour <clock>, but we are still exploring.

nhamming · 2020-09-21T17:53:17Z

@ps2 Ready for a formal review. With the naming update and the expanded use cases to consider how to display a manual glucose entry, this has become a much larger than expected PR.

As for the logic when displaying a manual glucose entry, the value displays as expected, but the associated icon displays the current status highlight icon instead of a trend. If there is no status highlight (possible in development, but should never occur in prod), the icon displays ?.

rickpasetto

Mostly nits, but other than that LGTM 💭

rickpasetto · 2020-09-21T19:50:42Z

Loop Status Extension/StatusViewController.swift

+
+        if view.bounds.width < 375 {
+            // need to adjust for narrow display
+            hudView.adjustViewsForNarrowDisplay = true


rickpasetto · 2020-09-21T19:54:49Z

Loop/Managers/DeviceDataManager.swift


-    var sensorState: SensorDisplayable? {
-        return cgmManager?.sensorState
+    func glucoseDisplay(for glucose: GlucoseSampleValue?) -> GlucoseDisplayable? {


[nit] this responsibility seems odd here in DeviceDataManager. I would have expected this to live in a ViewModel somewhere (maybe CGMStatusHUDViewModel?)

I believe it's here because of the interaction with cgmManager. And the interaction with cgmManager is the not-great part, imo; we need to eventually make Loop be able to evaluate glucoseRangeCategories independently of a CGM; it's not right that the CGM is evaluating ranges for the fingerstick value. I think once we have resolved that, this can go somewhere that makes more sense.

ps2

LGTM; just have the one comment about making the view adjust itself rather than the client, which seems natural to me, but I won't hold this up for. And the other comment about glucose range thresholding not really belonging to the CGM, but that's probably best addressed later.

ps2 · 2020-09-21T20:14:24Z

Loop/Managers/DeviceDataManager.swift


-    var sensorState: SensorDisplayable? {
-        return cgmManager?.sensorState
+    func glucoseDisplay(for glucose: GlucoseSampleValue?) -> GlucoseDisplayable? {


I believe it's here because of the interaction with cgmManager. And the interaction with cgmManager is the not-great part, imo; we need to eventually make Loop be able to evaluate glucoseRangeCategories independently of a CGM; it's not right that the CGM is evaluating ranges for the fingerstick value. I think once we have resolved that, this can go somewhere that makes more sense.

… displays

nhamming · 2020-09-22T17:12:16Z

@ps2 I added the feature flag and switch for DIY to allow Loop to categorize the glucose range instead of the CGM Manager.

ps2

LGTM. I do think the (currently hardcoded) categorizer for DIY Loop should be used for both CGM and Fingerstick glucose values, but I can tackle this on the DIY side as well, later, if you want to get this in.

ps2 · 2020-09-22T18:05:16Z

Loop/Managers/DeviceDataManager.swift

-        return cgmManager?.sensorState
+    func glucoseDisplay(for glucose: GlucoseSampleValue?) -> GlucoseDisplayable? {
+        if let glucose = glucose, glucose.wasUserEntered {
+            guard FeatureFlags.cgmManagerCategorizeManualGlucoseRangeEnabled else {


I don't think categorization would differ based on manual/cgm values, so we should have one categorization routine for each. In this case, we are splitting between Tidepool Loop, which uses the Dexcom CGMManager categorization routine, and DIY Loop, which will use a hardcoded categorization routine for now. But eventually, they will use the same routine in Loop (which might coordinate with the CGMManager in some ways, but the settings and config will be in Loop).

In essence, I'm suggesting that this temporary hardcoded categorization be used for both cgm and fingerstick glucose values if the flag is turned on.

Good to know. I'll add this now.

allow manual glucose entry without carb entry

8a38b70

nhamming requested a review from rickpasetto September 16, 2020 17:05

rickpasetto approved these changes Sep 16, 2020

View reviewed changes

nhamming added 3 commits September 16, 2020 16:09

the manual glucose entry displayed in the CGM pill is never overridden

0997628

Merge branch 'dev' into nate/fix/LOOP-1927/manual-glucose-entry

f85bf26

manually entered glucose needs interpretation from the CGM manager to…

e45842d

… be display correctly

nhamming requested a review from ps2 September 18, 2020 14:12

nhamming added 2 commits September 18, 2020 11:37

Merge branch 'dev' into nate/fix/LOOP-1927/manual-glucose-entry

4f3e896

update to glucose icon display if manual glucose entry

85ff522

ps2 approved these changes Sep 18, 2020

View reviewed changes

ps2 reviewed Sep 18, 2020

View reviewed changes

updating the display of a manual glucose entry within the CGM pill

7efb717

nhamming requested review from ps2 and rickpasetto September 21, 2020 17:53

rickpasetto approved these changes Sep 21, 2020

View reviewed changes

ps2 approved these changes Sep 21, 2020

View reviewed changes

nhamming added 5 commits September 22, 2020 08:51

Merge branch 'dev' into nate/fix/LOOP-1927/manual-glucose-entry

188a34a

the status bar determines when it should adjust its layout for narrow…

fefdcb9

… displays

updated tests

2dce93d

clean up

2d5d36a

added hardcoded glucose range categorization when feature is disabled

51b99f0

nhamming requested a review from ps2 September 22, 2020 15:31

ps2 approved these changes Sep 22, 2020

View reviewed changes

DIY categorization glucose range for both manual and CGM values

82056ff

nhamming merged commit 97f6c73 into dev Sep 22, 2020

nhamming deleted the nate/fix/LOOP-1927/manual-glucose-entry branch September 22, 2020 21:06

nate/fix/LOOP-1927/manual-glucose-entry #256

nate/fix/LOOP-1927/manual-glucose-entry #256

Uh oh!

Conversation

nhamming commented Sep 16, 2020

Uh oh!

rickpasetto left a comment

Choose a reason for hiding this comment

Uh oh!

nhamming commented Sep 18, 2020

Uh oh!

ps2 left a comment

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!

nhamming commented Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rickpasetto left a comment

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!

ps2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nhamming commented Sep 22, 2020

Uh oh!

ps2 left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nhamming commented Sep 21, 2020 •

edited

Loading