nightscout / Trio

MIT License
64 stars 259 forks source link

Rename smbIsAlwaysOff to smbIsScheduledOff (alpha) #120

Closed MikePlante1 closed 3 months ago

MikePlante1 commented 4 months ago

Also rewords "Last Hour SMBs are Off" to "First Hour SMBs are Resumed" to hopefully be a little clearer, and hides "Schedule when SMBs are Off" switch when "Disable SMBs" is switched on instead of disabling it when "Disable SMBs" is switched off.

Fix and refactor unChanged() for Profile Overrides Refactored by @polscm32 for readability. Logic changes by @MikePlante1 :

Do not merge until https://github.com/nightscout/open-iaps-oref/pull/20 is merged, since the changes to determine-basal.js in this Oi PR are reflected in that Oiref PR.

MikePlante1 commented 4 months ago

Renamed to Draft as I fix the toggles so it allows enacting an override when the only thing toggled on is Schedule when SMBs are Off

EDIT: good to go now

bjornoleh commented 4 months ago

The scheduling of SMBs work as expected.

Tested: setting disable and resume times with resume one hour after disabling. SMBs were issued before and after , but not during the SMB disabled timeframe.

Did not test setting resume hours < disable hours.

Please note, the profile status no longer displays the ø sign to indicate that SMBs are disabled. Likely, this is still looking for the previously used variable name.

MikePlante1 commented 4 months ago

Please note, the profile status no longer displays the ø sign to indicate that SMBs are disabled. Likely, this is still looking for the previously used variable name.

Good catch. It has to do with the changing of the toggle logic. You used to have to toggle on Disable SMBs in order for Schedule When SMBs are Off, but that doesn't make sense to me, so now in order for Schedule When SMBs are Off to be used you have to toggle off Disable SMBs because if Disable SMBs is toggled on then you're disabling SMBs all the time, not just for the scheduled times.

I'll look into adding something like ø 22-6 if Schedule when SMBS are Off is toggled on and set to 22:00-6:00.

bjornoleh commented 4 months ago

That’s great. @kylmcw mentioned something about a comma , sign in the profile string that doesn’t look too good. Perhaps you’ll catch that as well.

MikePlante1 commented 4 months ago

Please note, the profile status no longer displays the ø sign to indicate that SMBs are disabled. Likely, this is still looking for the previously used variable name.

Just added a commit to address this. To keep this PR more focused, I did not look into the comma discrepancy.

MikePlante1 commented 4 months ago

If you have one, please voice your opinion on changing the Disable SMBs toggle string: 👍 Disable SMBs 👎 Disable SMBs Always 😄 Always Disable SMBs 🎉 Disable All SMBs 😕 Something else...

bjornoleh commented 4 months ago

The comma issue is resolved now. I tried all combinations I could think of.

bjornoleh commented 4 months ago

If you have one, please voice your opinion on changing the Disable SMBs toggle string: 👍 Disable SMBs 👎 Disable SMBs Always 😄 Always Disable SMBs 🎉 Disable All SMBs 😕 Something else...

The OpenAPS setting for SMB is called "Enable SMB Always", so perhaps "Disable SMB Always" fits well?

scottleibrand commented 4 months ago

"Enable SMB Always" means SMB is enabled (allowed) any time, regardless of how long it has been since carb entry, or what COB is.

"Disable SMB Always" would imply that SMB is always disabled at any time, regardless of how long it has been since carb entry, or what COB is. I don't think that's what your setting does (24x7 disable, overriding everything else)?

Can someone describe which settings you have for enabling and disabling SMB, and what they do? That should make it more apparent what the names should be.

bjornoleh commented 4 months ago

"Enable SMB Always" means SMB is enabled (allowed) any time, regardless of how long it has been since carb entry, or what COB is.

"Disable SMB Always" would imply that SMB is always disabled at any time, regardless of how long it has been since carb entry, or what COB is. I don't think that's what your setting does (24x7 disable, overriding everything else)?

Can someone describe which settings you have for enabling and disabling SMB, and what they do? That should make it more apparent what the names should be.

SMBs are fully disabled when this setting is on, for the duration the profile (similar to a Loop override). So I think it is accurate enough. When the profile ends, SMBs will be enabled if the main settings allows. The duration can be timed or indefinite.

scottleibrand commented 4 months ago

So, “SMBs temporarily disabled”? I think “always” implies a more permanent setting.

bjornoleh commented 4 months ago

So, “SMBs temporarily disabled”? I think “always” implies a more permanent setting.

This is a setting that is configured when defining a profile/override, so the temporary nature is implied. I could agree though, but the name also needs to be seen in context with the alternative scheduled disabling of SMBs that is also an option. This will disable SMB during set hours of the day (some find it useful to disable during the night). If not scheduled, SMBs are off till the end of the profile/override.

Perhaps @MikePlante1 has screenshots, I don’t have this build on my phone right now.

bjornoleh commented 4 months ago

Ok, I had a screenshot after all:

image

The “Schedule when SMBs are off” toggle is hidden when “Disable SMBs” is enabled, hence it’s useful to hint at the possibility to disable on schedule.

JeremyStorring commented 3 months ago

Changes here look good, was a bit concerned about the profiles view changing but the changes make sense (and reading comments here helped)

bjornoleh commented 3 months ago

@MikePlante1, this works as expected in the app, and is technically good to go. Which way do you lean regarding the name of the Disable SMBs toggle string?

MikePlante1 commented 3 months ago

@MikePlante1, this works as expected in the app, and is technically good to go. Which way do you lean regarding the name of the Disable SMBs toggle string?

Thanks for that reminder. I went ahead and pushed "Always Disable SMBs"

bjornoleh commented 3 months ago

Merging this into alpha now. Only changes in the last commit is the proper copying of oref0 files including readable js source code.