nightscout / Trio

Trio - an automated insulin delivery system for iOS based on the OpenAPS algorithm with adaptations.
https://docs.diy-trio.org/en/latest/
MIT License
81 stars 418 forks source link

Enact Bolus has no sanity check #187

Closed LiroyvH closed 1 month ago

LiroyvH commented 4 months ago

The field to enter a bolus doesn't appear to have a proper two-way sanity check. Whilst it checks for a max bolus, it doesn't check for a minimum bolus nor does it respect the Bolus Increment. This means you can enter impossible values such as 0.0001U. This makes the app crash immediately after running FaceID. Moreover, for the 2nd issue, if your Bolus Increment is set to 0.5: you can still try to bolus 0.025 or, as another example, 1.025 - whilst neither ought to be possible with 0.5 increments. Ideally, minimum bolus is tied to bolus increment.

Medtronic 754, Trio Dev, iOS 17.5 and lower.

Sjoerd-Bo3 commented 3 months ago

hey 👋 - silence for 30 days 🤐 ... anybody? triage is required!

LiroyvH commented 3 months ago

As a quick solution, just fast thinking out loud (generally a bad idea in the weekend): I think you can check if its a valid request by doing "Requested Bolus / Bolus Increment". It has to result in a whole number to be valid.

Eg: if the result of the calculation (Requested Bolus / Bolus Increment) is not an integer, then the request is invalid and should not be processed. This works not only for a valid minimum, but will work to check the validity of any bolus with the fixed increment. (Of course alongside variables such as maxBolus) After all, 5.05 with an increment of 0.1 won't be valid either, but would stay in bounds when only looking at minimum/maximum bolus.

github-actions[bot] commented 2 months ago

hey 👋 - silence for 30 days 🤐 ... anybody? triage is required!

LiroyvH commented 2 months ago

Adding comment to (hopefully) avoid auto-closure of the still existing bug.

I think the solution is in my comment above, which will provide a sanity check all across the board (currently missing). If nobody sees an obvious issue with the proposal, this might be one of those small things I can code myself.

Sjoerd-Bo3 commented 1 month ago

Adding comment to (hopefully) avoid auto-closure of the still existing bug.

I think the solution is in my comment above, which will provide a sanity check all across the board (currently missing). If nobody sees an obvious issue with the proposal, this might be one of those small things I can code myself.

I would say go for it! We are busy at the moment. And if you can make this, that would be great!

LiroyvH commented 1 month ago

@Sjoerd-Bo3 Ok, but does the proposed method to verify it look good? I'd like at least one person other than me to ponder over the solution and whether or not I overlooked anything before spending time on attempting to code it. :P It looks good and pretty watertight to me, but a 2nd check/check for caveats would be nice.

aug0211 commented 1 month ago

@LiroyvH your approach sounds reasonable. I think this is similar to using the % mod function.

Could look something like bolus % bolus increment (must = 0).

MikePlante1 commented 1 month ago

Here's where to check for valid bolus sizes depending on your Medtronic model: https://github.com/loopandlearn/MinimedKit/blob/f11abde5e2eea2cbf7ac80f3f4bc4bc6e7f6de56/MinimedKit/Models/PumpModel.swift#L114

LiroyvH commented 1 month ago

@aug0211 Thnx!

@MikePlante1 Hmm I think that's more relevant for the selection itself under Settings -> Preferences -> Bolus Increment. I'm not currently looking in to validating that one (if it currently isn't), just validating that the bolus that's being requested to be enacted by the user matches a.) the absolute minimum (so essentially 1 * the configured increment), b.) is compatible with the set bolus increment in the "Enact bolus" window (so you cannot enact 5.05 if your increment is set to 0.1).

Sjoerd-Bo3 commented 1 month ago

@aug0211 Thnx!

@MikePlante1 Hmm I think that's more relevant for the selection itself under Settings -> Preferences -> Bolus Increment. I'm not currently looking in to validating that one (if it currently isn't), just validating that the bolus that's being requested to be enacted by the user matches a.) the absolute minimum (so essentially 1 * the configured increment), b.) is compatible with the set bolus increment in the "Enact bolus" window (so you cannot enact 5.05 if your increment is set to 0.1).

Maybe as a nice to have extra: automaticcally round up or down? With a bumper animation or something?

MikePlante1 commented 1 month ago

@LiroyvH After testing this, I think I see what you're saying. With my Medtronic 722, when I try to bolus less than 0.1 (so 0.01, 0.09, etc) the app closes and doesn't send a bolus. So the "enact bolus" button should just be disabled if less than the minimum bolus. If the bolus isn't possible but is above the minimum, it just rounds down, so there's nothing we need to fix about that. For example, entering 0.19 U will result in a 0.1U bolus.

Really, "Bolus Increment" should be removed as a preference and everything should just be checked against the selected pump's limitations.

Sjoerd-Bo3 commented 1 month ago

@LiroyvH After testing this, I think I see what you're saying. With my Medtronic 722, when I try to bolus less than 0.1 (so 0.01, 0.09, etc) the app closes and doesn't send a bolus. So the "enact bolus" button should just be disabled if less than the minimum bolus. If the bolus isn't possible but is above the minimum, it just rounds down, so there's nothing we need to fix about that. For example, entering 0.19 U will result in a 0.1U bolus.

Really, "Bolus Increment" should be removed as a preference and everything should just be checked against the selected pump's limitations.

It will be removed in the next iteration of settings (already is)

LiroyvH commented 1 month ago

@MikePlante1 Yup. I think you can actually get it to do increment of 0.025 depending on the model. So less than 0.1 is possible if the pump supports it. But higher is also possible. For example, because of my insulin requirements I've even ran with 0.5 increments at some point. (Though with SMB that wasn't ideal, so its back to 0.1 now (despite supporting 0.025 :P))

If the bolus isn't possible but is above the minimum, it just rounds down, so there's nothing we need to fix about that. For example, entering 0.19 U will result in a 0.1U bolus.

Not sure if I agree with that being the right solution, that's a near 50% reduction of administered insulin in that example. Though of course, if you're bolusing 5.19 and it goes down to 5.1 its much less extreme. But there are users that don't need a whole lot of insulin that may be affected - I think a warning would be in order there. ("You entered 0.19, but your bolus increment is 0.1. Would you like to enact \<rounded (to closest valid) number>? \<enact> \<cancel>")

Really, "Bolus Increment" should be removed as a preference and everything should just be checked against the selected pump's limitations.

It will be removed in the next iteration of settings (already is)

I'd reconsider that or otherwise at least make it possible to override this in another way. Validating against the selected pump limitations is a very good idea, but removing the ability for the user to change the increment up and down I wouldn't do. We shouldn't assume users always just want 0.1 or always want the bare-minimum the pump supports. (I mean, I did do 0.025 for fun, and it wasn't so funny on the Apple Watch I can tell you.). There's a reason all pumps allow you to change this in their settings or PDM.

LiroyvH commented 1 month ago

Oh I'll add to that: I do think this setting is in the wrong location. It should imho be in the Pump Settings, not the app preferences.

MikePlante1 commented 1 month ago

I can see a case for a setting to be added for "Bolus Increment" under "Watch" in settings. Is there another reason to set a higher increment than what the pump is capable of?

There's a reason all pumps allow you to change this in their settings or PDM.

I'm not sure what you mean by this. The only relatable setting I can think of on my 722 is "Easy Bolus" for the up/down quick bolus buttons on the pump. That's not really the same thing, though.

LiroyvH commented 1 month ago

Yes, higher auto-increments for insensitive people (couldn't resist making that pun :P).

Incidentally, assuming the pump is set to what it is minimally capable of may lead to issues. If the app assumes 0.025 based on the pump minimum but the pump is set to 0.1: you might start running in to issues when the math doesn't work out. Must make sure users know this, especially existing ones if this behaviour suddenly changes whilst the pump's settings remain being what they are. The default is higher than its lowest for the 754 for example IIRC.

MikePlante1 commented 1 month ago

I only have a 722 which doesn't have a scroll rate to test.

So for the 754, it looks like you can set Scroll Rate to 0.025 U, 0.05 U, or 0.1 U. If you set Scroll Rate to 0.1 U on your pump, and set Bolus increment to 0.025 U in Trio, what happens when you try to give a manual bolus in Trio of:

A: 0.05 B: 0.125 C: 0.175

If you could do that and provide a log it might help us figure this out. I could be wrong, but I don't think Loop or AAPS has a bolus increment setting, so I'd think we should be able to get Trio to work correctly without requiring the user to set this as well.

LiroyvH commented 1 month ago

@MikePlante1 I cannot properly test that because the bolus window only allows 2 decimals. :') However, with SR at 0.1 the pump does appear to accept 0.05!

Yeah I hope at least in Trio when it's unfortunately removed that it remains being possible to set this as an override in the debug options (or middleware); and of course at least gets a way to set a different step size in the Watch-settings. The 0.1 is already horrible, but 0.025 is an absolute hell: 40 taps on the +/clicks with the scroll wheel to get to 1 single unit. For people like me who can have to request 25U: that's 1000 f-ing taps/clicks haha! (I'd sooner set this to 0.5 or 1.0 step size.)

dsut4392 commented 1 month ago

@MikePlante1 I've never tried to see if adjusting the scroll rate setting on the pump affects what you can deliver via a bolus from Loop or Trio, I would have assumed that does not. Important also to remember that the bolus increment on the 754 pumps isn't static, but depends on the bolus size, e.g. you can bolus 0.025 increments up to 0.975U and it will deliver them, but send a bolus of 1.025 to the pump it will only deliver 1U. Send a bolus of below 0.025U and it will fail silently, below 1 U any bolus is rounded down to the nearest 0.025, above 1U it's rounded down to the nearest 0.05 etc. I did a bunch of testing of this on one of @dnzxy's branches last week.

dnzxy commented 1 month ago

It will be automatically setting this to the smallest bolus increment, which is 0.05 for pods, 0.1 for x22 models, and Dana, and hard-set 0.1 for x23 models. Feel free to adjust code to get to a larger bolus increment. The issue here is that LoopKit only exposes a range and we cannot set a range as allowed bolus sizes. That’s the bolus increment set by the pump manager I am talking here, which is used for various calculations in the app and the algo. The bolus increment stepper for the watch is a different thing. Given that the watch app will be completely re-done, feel free to chime in when the time comes.

dnzxy commented 1 month ago

By the way, this is being sanity checked in the backend and this is sanity checked before it’s send to the backend in the BolusState model (even in 0.2.0).

dsut4392 commented 1 month ago

I wonder if for x23 or x54 models you should hard code it to 0.05 instead of 0.1? The only times 0.05 increments won't be delivered (and will get rounded down by the pump) is for boluses over 10U, in which case you're talking at most a 0.5% under-delivery compared to what was sent, completely insignificant.

dnzxy commented 1 month ago

That’s entirely possible to do, I was told 0.1 would be the sanest choice. We can also do 0.05 👍 the question is how insignificant is that and does the overwhelming majority of people not regularly dose above 10 U? Children, sure, but adults?

dsut4392 commented 1 month ago

My thinking is that for people who are very sensitive and want the smallest possible increment, the difference between 0.1 and 0.05 may matter. For everyone else, it's unlikely to make any difference one way or the other, because in most cases whenever Trio is going to deliver an SMB it's going to be bigger than 0.05 anyway. I suppose there's likely to be a small increase in the number of SMBs overall when Trio is able to deliver 0.05 but not 0.1, which could present a battery hit. But on an MDT pump this isn't as critical, it's not like your pump is going to die a screaming death like pods do. And philosophically, if we're choosing to default to the smallest increment possible with pods, then we should at least aim for the smallest increment reliably possible with all pumps rather than choosing a different value.

dnzxy commented 1 month ago

Sane argumentation. Initially, we were just setting the smallest available increment for all pumps but I was told that 0.025 really is not a good value to use for reasons. So we went with the "sibling" MDT increment, 0.1, which was also chosen by @bastiaanv for DanaKit.

Ideally, what we need, is for LoopKit device modules/drivers to properly expose the user selected bolus increment and/or, if dynamic based on bolus size, the bolus dose specific increment.

This increment is not just used for SMB calculation within oref, it’s technically also used to calculate scheduled basal for TDD. That seldom runs (only really in the presence of red loops), but it‘s still part of the equation and pump dose pulses are (usually 😂) the best way to calculate insulin.

MikePlante1 commented 1 month ago

That’s entirely possible to do, I was told 0.1 would be the sanest choice. We can also do 0.05 👍 the question is how insignificant is that and does the overwhelming majority of people not regularly dose above 10 U? Children, sure, but adults?

What problems does 0.025 U cause?

I’ll say U-200 is not an option for me with my 722 because that would mean a 0.2 U increment.

And I very rarely bolus > 10 U at a time.

dnzxy commented 1 month ago

0.025 increments are only used for super small boli and we‘d be setting it in the app for use across all instances where it is necessary to pass in the bolus increment. Anyhow, I asked, and I was told 0.025 is not the one to use and to use 0.1 (and now, 0.05 was also suggested).

dsut4392 commented 1 month ago

@MikePlante1 If we set 0.025 as the bolus increment, any time Trio tries to deliver a bolus (manual or SMB) over 1U with an 0.025 increment it will get rounded down to the nearest 0.05. I guess it's not a significant amount as long as the history correctly tracks what has actually been delivered. At the moment the history screen is only showing two decimal places, and not rounding correctly with a 0.025U bolus shown as 0.02U, and a 0.175U bolus shown as 0.18U (in main). Hopefully oref is getting the insulin delivery straight from the pump event and this rounding is only a problem in the UI?

dsut4392 commented 1 month ago

IMG_9725 IMG_9726

MikePlante1 commented 1 month ago

a 0.175U bolus shown as 0.18U (in main). Hopefully oref is getting the insulin delivery straight from the pump event and this rounding is only a problem in the UI?

Could you upload a log so I can check how oref processes that?

dnzxy commented 1 month ago

With Trio 0.2.0 you can check what is send to the algorithm (it's the pump history, and that is on file). The actually dosed amount is entered amount -> rounded to nearest allowed bolus increment by LoopKit device layer -> sent to pump.

The bolus increment for 1.0.0 will be automatically set based on the pump model you onboard in the app, meaning the bolus increment setting will no longer be required for users to use or enter. We can further discuss what the best value for x23 / x54 models is so set this to, other channels are better suited for that discussion.

The treatment view should contain a check for minimal bolus, as was the OP's initial intention with this issue ticket. There is a known issue for entered amount < lowest allowed bolus amount (@marionbarker has that on her list I believe) causing weird outcomes, that will need addressing (probably not by the Trio team). All bolus rounding is done viaLoopKit's device layer implementation of allowed bolus rounding; there is no custom Trio logic.


Edit to add this, as I think the bolus increment setting is extremely confusing because of its naming. The setting is only used for 3 things in Trio

  1. calculating TDD (currently still happening in trio-oref, courtesy of iAPS v2.3.3) -> will change when we adopt "vanilla oref"
  2. rounding the "proposed" SMB dosage in trio-oref which is then sent to the app backend for further processing (i.e., sending it through LoopKit submodule for rounding before it is sent to the pump)
  3. determining the "steps" for the bolus amount for the watch app when you turn the crown

(Technically a fourth thing: formatting the displayed number in history, for bolus progress, etc. Nothing dosing-specific)

Every single insulin dose that ever goes from Trio to the pump goes through the respective LoopKit submodule, and all the LoopKit submodules offer a rounding method that rounds the Trio-given amount to the nearest allowed pump bolusing amount. This "processed" and rounded number is then used as dosing amount and sent to the pump for actual dosing.

This is why this setting leads to a TON of confusion and should not be exposed to the user, hence we will automatically set it when you onboard a pump in Trio for v1.0.0.

dsut4392 commented 1 month ago

IMG_9728 FWIW, Loop displays the actual bolus issued up to 3 decimal places, it doesn't round the value in the insulin delivery screen. Sorry for taking this OT.

LiroyvH commented 1 month ago

@dnzxy Almost sounds like the dependency on LoopKit is introducing annoying limitations. But I'm sure that's a part of the pros vs cons equation. The discussion between having to set the lowest, 0.05 or 0.1 sounds rather arbitrary and that its unclear what the implications are and what the sane settings are, but maybe I'm misinterpreting that.

Anyway:

The bolus increment stepper for the watch is a different thing. Given that the watch app will be completely re-done, feel free to chime in when the time comes.

Since they're directly related, I'm not sure if its really a different thing. Currently you can set the increment to for example 0.5 and going from 0 to 25 is "only" 50 clicks with the scroll wheel which is quite doable. But even with a 0.05 rather than 0.025 increment, going to 25U is still 500 taps/clicks with the scroll wheel - which is taking a horrible amount of time to achieve. Ideally, this is really fixed before that time if the Watch - prior to a new app/settings - relies on the forced Bolus Increment the same way it relies on that setting now. Or at the very least it should be possible to override the setting to mitigate the problems it introduces. Then you don't expose it in the settings, but there's a route for people to make the app do what they want rather than have the app dictate what they must be using at all times.

dnzxy commented 1 month ago

They‘re currently directly related. Since the watch app will need a complete make over, it does not have to anymore. No work other than critical bug addressing will be done on 0.2.0, so let’s revisit this, with 1.0.0.

LiroyvH commented 1 month ago

@dnzxy Not sure I understand, but the setting didn't disappear for 0.2.0 (at least its still available here). So isn't it relevant to discuss for 1.0.0 already (which I thought was the next proposed version) to ensure it's properly implemented all across the board?

dnzxy commented 1 month ago

In the work that is already happening for what will be 1.0.0, the setting is no longer there (see it will auto-set when onboarding a pump), hence all the discussion of watch app implications are only 0.2.0 relevant, since the setting still exists for that version of the app, because for 1.0.0 we‘re also looking to modernize the watch app, which is a whole different conversation, feel free to bring your wishes for a customizable bolus increment picker / step count up then.

LiroyvH commented 1 month ago

I really don't understand what the advantage is of bringing it up after it becomes a problem rather than before it becomes a problem, especially if it's not going to be an issue in 0.2.0; but ok I'll open a new issue then.

dnzxy commented 1 month ago

The issue you had addressed here is addressed in the current dev work, it was very good of you to address it here, the discussion it opened up helped with tweaking a few things.

Either keep this open and revisit the watch thing once the watch work actually starts, or do a new issue 😊 Not sure why it always has to be so confrontational, the points you raised were sensible and good to be addressed.

LiroyvH commented 1 month ago

@dnzxy No I think I'm just at a loss here as to what it is you want. The issue was for a sanity check, this went to the Bolus increment directive (unfortunately) being removed entirely and nothing about the sanity check issues other than your comment that its being sanity checked now after the changes. Then when addressing the issues removing the MaxBolus directive causes: I'm supposed to not get in to that but instead wait for 1.0.0 to be released before raising the issues. Sounds to me like the issue was supposed to be closed as it's resolved and a new one should be opened once 1.0.0 is out there... As such, I closed the issue as resolved.

dnzxy commented 1 month ago

Yeah I’m at a loss at to what you are talking about, sorry. MaxBolus directive removed entirely - what?! This was about a sanity check that was confused to have something to do with the bolus increment setting. It doesn’t really, but the discussion led to a few good points. All well and worth it. I asked you repeatedly to address this for 1.0.0, i.e., when it is testable, and to address it when the watch app you kept focusing on is hopefully redone and your wish of a customizable stepper can be addressed. That’s all :)

I‘m sorry, if this caused confusion.

LiroyvH commented 1 month ago

Yeah not MaxBolus (that was another issue), Bolus Increment.

Either way: the issue was indeed about a sanity check in the Enact Bolus-window, but based on those comments about removing bolus increments altogether, the alleged proposed solution that came paired with that and your comment that there is a sanity check now: it seems to be resolved, somehow. The only follow-up I made was that forcing users to a very low bolus increment without any way to change it will cause significant issues with the current Watch App. No more no less.

So is the enact bolus bug fixed (in dev/future release) or not...? If it is, then I'm really not sure why this issue has to stay open as it's completed. If it isn't solved, then I really don't understand the line of comments anymore. It can be re-opened and I'll just go with the flow. The Watch issue that will be introduced if not addressed can be setup in another issue I guess, though I'd prefer to do that before it's actually a problem rather than only fixing it once its broken. That is all.