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

Rename smbIsAlwaysOff to smbIsScheduledOff (alpha) #120

Merged
merged 9 commits into from
May 1, 2024

Conversation

MikePlante1
Copy link
Contributor

@MikePlante1 MikePlante1 commented Apr 17, 2024

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 :

  • Removed checking smbIsOff from defaultProfile
  • Added checking for smbIsScheduledOff to allSettingsDefault
  • Removed checking isf and cr from allSettingsDefault

Do not merge until nightscout/trio-oref#20 is merged, since the changes to determine-basal.js in this Oi PR are reflected in that Oiref PR.

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.
@@ -1,5 +1,5 @@
<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
<model type="com.apple.IDECoreDataModeler.DataModel" documentVersion="1.0" lastSavedToolsVersion="21754" systemVersion="22F82" minimumToolsVersion="Automatic" sourceLanguage="Swift" userDefinedModelVersionIdentifier="">
<model type="com.apple.IDECoreDataModeler.DataModel" documentVersion="1.0" lastSavedToolsVersion="22757" systemVersion="23E224" minimumToolsVersion="Automatic" sourceLanguage="Swift" userDefinedModelVersionIdentifier="">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what this change is about or if it's needed as it seems it was altered automatically.

@MikePlante1 MikePlante1 changed the title Rename smbIsAlwaysOff to smbIsScheduledOff (Draft) Rename smbIsAlwaysOff to smbIsScheduledOff Apr 17, 2024
@MikePlante1
Copy link
Contributor Author

MikePlante1 commented Apr 17, 2024

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

Refactored by @polscm32 for readability.
Logic changes by @MikePlante1 :
+ Removed checking `smbIsOff` from `defaultProfile`
+ Added checking for `smbIsScheduledOff` to `allSettingsDefault`
+ Removed checking `isf` and `cr` from `allSettingsDefault`

Co-Authored-By: polscm32 <107251660+polscm32@users.noreply.github.com>
@MikePlante1 MikePlante1 changed the title (Draft) Rename smbIsAlwaysOff to smbIsScheduledOff Rename smbIsAlwaysOff to smbIsScheduledOff Apr 18, 2024
@MikePlante1 MikePlante1 mentioned this pull request Apr 18, 2024
@MikePlante1 MikePlante1 requested a review from dnzxy April 18, 2024 05:03
@bjornoleh
Copy link
Contributor

bjornoleh commented Apr 18, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

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.

Displays time range in label when SMBs are scheduled off in profile override.
For example, if the range is set from 22:00-6:00 it would display
⃠ 22-6
@MikePlante1
Copy link
Contributor Author

MikePlante1 commented Apr 18, 2024

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.

When `Disable SMBs` or `Schedule When SMBs are Off` is the only thing changed.

Note, this commit does not fix that changes to SMB minutes are not displayed in the label.
@MikePlante1
Copy link
Contributor Author

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
Copy link
Contributor

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

@bjornoleh
Copy link
Contributor

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
Copy link
Member

"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
Copy link
Contributor

"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
Copy link
Member

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

@bjornoleh
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

@bjornoleh
Copy link
Contributor

@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
Copy link
Contributor Author

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

Merge pull request #2 from nightscout/alpha
@MikePlante1 MikePlante1 changed the title Rename smbIsAlwaysOff to smbIsScheduledOff Rename smbIsAlwaysOff to smbIsScheduledOff (alpha) Apr 30, 2024
MikePlante1 and others added 3 commits April 30, 2024 13:11
Includes open-iaps-oref PR 20 and PR 17 (console error fix)

 Last commits:
+5774155 Merge pull request 20 from MikePlante1/disable_smb_schedule
+e3a8d73 Merge pull request 17 from MikePlante1/console_error_refactor
+a29d9ce rename smbIsAlwaysOff to smbIsScheduledOff
+d898eb1 fix threshold console.error

Co-authored-by: Mike Plante <mikeplante1@gmail.com>
Update oref0 for #120 Rename smbIsAlwaysOff to smbIsScheduledOff
@bjornoleh
Copy link
Contributor

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

@bjornoleh bjornoleh merged commit aa55271 into nightscout:alpha May 1, 2024
@MikePlante1 MikePlante1 mentioned this pull request May 3, 2024
@MikePlante1 MikePlante1 deleted the disable-smb-schedule branch September 18, 2024 01:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants