Skip to content
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.

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

Fmegen/3.3.0.0 - Adding VeryHigh Overview Display Option #3678

Open
wants to merge 14 commits into
base: dev
Choose a base branch
from

Conversation

vanmegen
Copy link

@vanmegen vanmegen commented Jan 2, 2025

This is analog to the nightscout Very High setting which displays bg values higher than a configurable value in a different color.
I did set the default to 400, but it can be set to 240 to be a similar threshold as nightscout uses.

(in the following screenshot red dots mark values higher than 240)
image

I have added this to the overview settings
image

as well as to the onboarding wizzard
image

@MilosKozak
Copy link
Contributor

MilosKozak commented Jan 2, 2025

I kept it without "very high" to match dexcom setting
But I'm open to discussion about this

@olorinmaia
Copy link
Contributor

I prefer keeping red color for lows.

But, default value is set to 400 mg/dl which translates to about 22 mmol/l so I think this is a win / win for everyone. This new setting can be set so high that it won't have affect on the visuals in most cases as we rarely go above 13 mmol\l. Then I can just set it to e.g. 20 mmol/l and it will display like before. Or if users want it lower they have the freedom to do so.

So I think this will be a good change, regardless if you prefer it like before, or want red color for very high.

Copy link

sonarqubecloud bot commented Jan 3, 2025

@vanmegen
Copy link
Author

vanmegen commented Jan 3, 2025

@olorinmaia

So I think this will be a good change, regardless if you prefer it like before, or want red color for very high.
Thanks for the comments!

@MilosKozak

I kept it without "very high" to match dexcom setting But I'm open to discussion about this
The reason I added this to aaps is that this display format is one of the defaults in nightscout and quite useful to get at-a-glance information about the current range. like "green" == all good, "yellow" == not-great-not-terrible, and "red" == bad.

here is the corresponding nightscout screenshot
image

in nightscout, you can set this via environment variable
image

I agree that this should be the "high" externally (i.e., in the UI), however, internally, the range above 180 is already named "high_value", which is why I used the very_high_value names instead.

should I do any change to the PR?

@Philoul
Copy link
Contributor

Philoul commented Jan 4, 2025

Hum, if we want to be fully consistant, we should also review Watch graph (to include the "Very High color") and watchfaces colors for Very High BG...
So probably a bit more work to do.
I should also have to update a bit Custom Watchface code to keep compatibility with Custom Watchfaces V1 for BG color management

@Philoul
Copy link
Contributor

Philoul commented Jan 4, 2025

If this proposal is merged, maybe it's better to manage Watch update for color consistancy in a different PR 🤔

@koelewij
Copy link

koelewij commented Jan 7, 2025

As @olorinmaia already mentioned, setting the "Very High" threshold to a very high value is basically allowing you to disable it would in my opinion, not bother me to implement this.

@vanmegen
Copy link
Author

vanmegen commented Jan 8, 2025

Hi @Philoul

If this proposal is merged, maybe it's better to manage Watch update for color consistancy in a different PR 🤔
I would suggest so too as I dont have a physical watch for testing,

@vanmegen
Copy link
Author

vanmegen commented Jan 8, 2025

Hi @koelewij,

As @olorinmaia already mentioned, setting the "Very High" threshold to a very high value is basically allowing you to disable it would in my opinion, not bother me to implement this.
thanks you for your feedback

@Philoul
Copy link
Contributor

Philoul commented Jan 8, 2025

@vanmegen I will manage watch update (with Phone app impact for data communication) on my side.

  • I will have to manage compatibility with v1 and v2 custom watchfaces
  • don't know if complications are impacted or not

@vanmegen
Copy link
Author

vanmegen commented Jan 9, 2025

Hi @Philoul

@vanmegen I will manage watch update (with Phone app impact for data communication) on my side.
Awesome! thank you very much

@MilosKozak
what do you think? Is it worth merging?

kind regards,
friedel

@vanmegen
Copy link
Author

Hi @Philoul @koelewij, @MilosKozak ,
I am wondering what would be the next step?

In particular, who would need to approve the PR?
Is there a process I should be aware of (discord)?

kind regards,
Friedel

@olorinmaia
Copy link
Contributor

@MilosKozak approves all PR, and all else who can try to test. I don't think it's likely for more changes to get merged for 3.3 right now. But that's up to Milos to decide.

@olorinmaia
Copy link
Contributor

olorinmaia commented Feb 17, 2025

Would it make sense to add threshold for very low also? To align it completely with NS. Low is yellow color and very low is red.

# Conflicts:
#	core/keys/src/main/kotlin/app/aaps/core/keys/UnitDoubleKey.kt
@vanmegen
Copy link
Author

@olorinmaia

Would it make sense to add threshold for very low also? To align it completely with NS. Low is yellow color and very low is red.
good point. I updated the PR to include a "very_low" configurable setting as well.

@vanmegen
Copy link
Author

screenshot
image

@olorinmaia
Copy link
Contributor

I'm testing this now and seem to be working good. I also tested it on AAPSClient. I can confirm that the "Range of visualization" set on AAPS syncs to AAPSClient for the new parameters also.

Thumbs up from me.

@vanmegen
Copy link
Author

vanmegen commented Mar 2, 2025

@Philoul
I had some time to add the veryHigh and veryLow settings to the watchfaces as well
could you please review this part of the PR if this is what you had in mind?

here are some screenshots:
(the main changes are the graph colors as well as the BG reading that now includes the very* color as well.
note that I set the High setting to 180 and the veryHigh setting to 200)

image
image
image

@Philoul
Copy link
Contributor

Philoul commented Mar 3, 2025

@vanmegen I made a quick analysis only, and current proposal within your latest commit will break some feature within CustomWatchface v1 and v2 (dynData and dynPref).

You should keep sgvLevel and manage color within watchfaces with this value (extend with -2 and +2 for veryLow and veryHigh).
VeryHigh and veryLow colors seems to be hardcoded, but in CWF they should be available as parameter for customization with dedicated keys.

CWF code should also be adapted to keep compatibility with all previous CWF (including advanced feature like dynData and dynPref where sgvLevel is also managed, with new min/max values).

@Philoul
Copy link
Contributor

Philoul commented Mar 3, 2025

Wear In screenshots VeryHigh and VeryLow horizontal lines are missing (I only see one yellow line on the top and one red line on the bottom of the graphs)

@@ -1481,8 +1485,10 @@ class DataHandlerMobile @Inject constructor(
avgDeltaDetailed = glucoseStatus?.let { deltaStringDetailed(it.shortAvgDelta, it.shortAvgDelta * Constants.MGDL_TO_MMOLL, units) } ?: "--",
sgvLevel = if (glucoseValue.recalculated > highLine) 1L else if (glucoseValue.recalculated < lowLine) -1L else 0L,
Copy link
Contributor

Choose a reason for hiding this comment

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

sgvLevel should be updated to return values from 2L (veryHigh) to -2L (VeryLow)

Copy link
Author

Choose a reason for hiding this comment

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

Did that

Copy link

sonarqubecloud bot commented Mar 3, 2025

@vanmegen
Copy link
Author

vanmegen commented Mar 3, 2025

Hi @Philoul,

Wear In screenshots VeryHigh and VeryLow horizontal lines are missing (I only see one yellow line on the top and one red line on the bottom of the graphs)
I think I addressed your issues/suggestions.

  • sgvLevel rather than raw value
  • lines now appear in graphs
  • bg value is colored as well
  • colors can be used from settings

as for this comment:

CWF code should also be adapted to keep compatibility with all previous CWF (including advanced feature like dynData and dynPref where sgvLevel is also managed, with new min/max values)
I was not sure where to change the ranges. can you point me to the code?

image
image
image

hope that catched all comments

@Philoul
Copy link
Contributor

Philoul commented Mar 3, 2025

Sorry, previous comment deleted by mistake
It's better but not enough with latest commit (default images not managed, sgvLevel dynamic values for external data...)
Let me propose you update for CustomWatchface code 😉

@Philoul
Copy link
Contributor

Philoul commented Mar 3, 2025

@vanmegen I just made a PR on your working branch with my proposal for CustomWatchface: vanmegen#1

  • I increased CustomWatchface version with a minor update (from 2.0 to 2.1) because you have a high level of compatibility (a CWF v2.1 will work with a plugin V2 without the HighColor management only...
  • I included several comment to explain you the different modifications between your proposal and mine but you were very close with your latest commit, even in variable naming ;-)

Don't forget to test my proposal (of course I know very well CWF code so it should work fine, but didn't test anything because watch is connected to my loop phone only, and didn't install your PR...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants