Migrate fix for PumpManager: Use pump limit from pumpManager instead of UI #266

dnzxy · 2024-06-03T06:39:37Z

Migrating this important fix over to Trio.

Quoting @bastiaanv, the original author:

What is the problem

I will start by saying: This is mainly a Dana pump problem, but the problem is the following:

The LoopKit syncDeliveryLimits allows the PumpManager to return DeliveryLimits which are also provided by the parameters. When using Loop (v3.3.0, but this behaviour is since v1.1.0), a dev could return a different then provided and the returned will be stored instead of the provided one. [Trio] doesn't do that currently.

Code proof of this behaviour: https://github.com/LoopKit/LoopKit/blob/dev/LoopKitUI/Views/Settings%20Editors/DeliveryLimitsEditor.swift#L348

Why do we need it

We need this, because Dana pumps don't allow delivery limits to be set, only to be retrieved. This means that the DanaKit should either:

Reject every syncDeliveryLimits command (which leads to an invalid maxBolus and maxBasal since there are never stored)

Or ignore the provided parameters and just fetch the actual limits from the pump and return those

The last one gives the user the best UX :)

What is the impact

Nothing (or at least the behaviour is the same as with Loop). The code has been tested on the Dana branch

When looking at the code from OmniBLE and OmniKit (link), the parameter is directly returned.

When looking at the code from MinimedKit (link), the actual values stored on the pump is (set first and then) returned.

Relates to #263

FreeAPS/Sources/Modules/PumpSettingsEditor/PumpSettingsEditorProvider.swift

Sjoerd-Bo3

LGTM (code and compared to previous PR)

bjornoleh · 2024-06-03T17:26:36Z

Does this PR also solve the issue with Medtronic pumps which are limited to 25 U max, unlike pods that can deliver 30 U max? (ref issue 247, but I won't link that issue until I understand if this PR is relevant there)

bastiaanv · 2024-06-03T18:50:46Z

On Medtronic it would, since the settings are fetched after setting. See: https://github.com/LoopKit/MinimedKit/blob/main/MinimedKit/PumpManager/MinimedPumpManager.swift#L1418

On Omnipods it wouldnt, since the parameter is returned and not fetched from the pump. See: https://github.com/LoopKit/OmniBLE/blob/dev/OmniBLE/PumpManager/OmniBLEPumpManager.swift#L2106

bjornoleh · 2024-06-03T18:55:55Z

Thanks, then I think we can say that #247 will be closed with the merging of this PR!

marionbarker · 2024-06-03T20:29:05Z

Test Results:

I got a build failure - see Configuration 2 details.

The behavior with the PR (modified so it would build) was different - no error message but the Pump Settings screen showed the pump maxBolus after silent failure instead of showing the attempted value.

Configuration 1:

MDT 515 pump (initial maxBolus 7, maxBasal 0.9)
Trio, commit 14c7643
- Trio settings: maxBolus 10, maxBasal 2
- attach pump
- (just as an aside) unlike loop, the pump can be attached even though the maxBolus and maxBasal on pump are less than Trio settings
- Following connection, pump settings match Trio pump settings
- Modify Trio settings: maxBolus 5, maxBasal 0.9 and save on Pump - done successfully
- Modify Trio settings: maxBolus 30 and save on Pump - fails silently; the Pump Settings screen continues to show 30 U until leaving and returning to the screen

The Xcode debug log reports:

Save delivery limit settings failed: command(MinimedKit.PumpOpsError.pumpError(Max setting exceeded))
Type: Error | Timestamp: 2024-06-03 11:53:24.222964-07:00 | Process: FreeAPS | Library: MinimedKit | Subsystem: com.randallknutson.OmniBLE | Category: MinimedPumpManager | TID: 0x18acfa

Configuration 2:

MDT 515 pump (initial maxBolus 5, maxBasal 0.9) from prior Trio setting
Trio, commit 14c7643 with PR 266 patch applied - fails to build (see below for error and work-around).
Now when attempting to set maxBolus to 30 and save on pump, it still fails silently but it does not show 30 U on pump settings screen - leaves it at 5 U.

The Xcode debug log reports:

Save delivery limit settings failed: command(MinimedKit.PumpOpsError.pumpError(Max setting exceeded))
Type: Error | Timestamp: 2024-06-03 13:16:25.633035-07:00 | Process: FreeAPS | Library: MinimedKit | Subsystem: com.randallknutson.OmniBLE | Category: MinimedPumpManager | TID: 0x191915

Build Failure

Switched to dnzxy branch directly instead of using a patch.

commit: a06cd43 (HEAD -> migrate-pump-limit, dnzxy/migrate-pump-limit) Add missing parameter for save function

screenshot of build failure:

work-around - not sure what this does - added a dummy print statement after the case statement with the error.

dnzxy · 2024-06-03T20:33:16Z

That's what you get when doing this via the GH GUI in the morning before leaving for a work trip. Apologies. Should build fine now. Please check again @marionbarker .

marionbarker · 2024-06-03T20:39:28Z

Please - it should be a rule that no PR is created if you haven't built the change.

I spent a lot of time assuming it was me

Configuration 2: repeated

dbeb61f (HEAD -> migrate-pump-limit, dnzxy/migrate-pump-limit) Address build failure by removing duplicate case
same results

In my opinion - if Save on Pump fails - it should give an error message to the user, not just restore the Pump Settings screen to what was there before you made the request.

Co-authored-by: Bastiaan Verhaar <bastiaanv@users.noreply.github.com>

dnzxy · 2024-06-03T20:55:16Z

Apologies, again.
I simply wanted to migrate this sooner rather than later and not have it sit around. Typical copy-paste issue.

In my opinion - if Save on Pump fails - it should give an error message to the user, not just restore the Pump Settings screen to what was there before you made the request.

This is a 1:1 migration of the initial PR, I haven't changed or touched any error handling.

Force-pushed commit 05d2efb is merely cleaning up commit history for my own stupidity and oversight.

marionbarker · 2024-06-03T21:01:48Z

Test with Dana

This needs more work - see Configuration 4 - need to see an error message, not a spinning icon.

Configuration 3:

switch to feat/dana with latest dev from DanaKit:

updates to DanaKit:

* DanaKit 489c310...3642042 (5):
  > fix: Fix typo
  > i18n: Fix translations with appEnv
  > Merge remote-tracking branch 'loopandlearn/enable_appName' into HEAD
  > fix: fix minor errors
  > fix: Several fixes & features (see iAPS for more details)

Test: Attach the dana-simulator to this build of Trio:

Attempt to modify Pump Setting by changing maxBolus from 4 to 5
Silent failure - have to leave and return to pump settings screen to see it is still at 4

Xcode debug log:

DanaKitPumpManager.swift - syncDeliveryLimits(limits:completion:)#1064: Skipping sync delivery limits (not supported by dana). Fetching current settings
Type: Info | Timestamp: 2024-06-03 13:46:49.187844-07:00 | Process: FreeAPS | Library: DanaKit | Subsystem: com.randallknutson.DanaKit | Category: DanaKitPumpManager | TID: 0x1940f9

Configuration 4:

Created a patch from git diff alpha migrate-pump-limit
Apply patch to current feat/dev (with updated DanaKit)
same error as before - but Trio displays the spinning saving icon until you leave and resume.

dnzxy · 2024-06-03T21:09:56Z

I'm sorry, I really cannot follow what you are applying on top of what and updating where. There is no Configuration 4 in your last comment. I'm assuming it's the 2nd "Configuration 3"?

According to @bastiaanv , this PR should work on top of latest feat/dana (which should be based on latest dev) + PR #263 applied too. alpha is in-sync with dev as they have not converged yet and the branch was recently created.

The compile error you had listed earlier was due to me not removing a line that should have been removed and be solved. Building fine on my end here now.

bastiaanv · 2024-06-03T21:12:03Z

The silent error might be related to the simulator instead of the PR @marionbarker

Could you check the output of the simulator, there is a big chance that it gave the Unsupported error there.

The xcode log is to be correct. I want to let the dev know we cannot set the setting, only fetch

dnzxy · 2024-06-03T21:22:08Z

@bastiaanv could you clarify the flow of data here real quick for me.

User sets setting -> sent to pump -> tries to set it -> in either case it returns to pump manager, i.e. the app, with set settings (on pump) -> if nil (unsuccessful), it sets the entered settings again (?) in UI. (edit to add) i.e. this means silently failing as reported in #247; doesn't it?

Is this correct and intended like so?

LiroyvH · 2024-06-03T21:31:51Z

@bjornoleh Reading over this and how I think it functions (and mega reservation there: I'm not sure I understand :P), how does this fix 247 for Medtronic pumps that allow values higher than 25U (when manually set on the pump)? Is that because it'll try to set, fail, but then read from pumpManager, sees its set to 50U (or whatever ;)) anyway and thus sets that as the maxBolus value?

If that is how it works, I do think one thing may be important (unless I misunderstood how this works): if it fails to set the value the user asks, it shouldn't automatically assume that what's in the pump is what the user wants; rather it should error out if the desired value (entered in app) does not match the value obtained from the pump. This prevents the app from being set to a value the user did not intend it to be set to.

Scenario: let's say a user wants to set a 10U MaxBolus (as guardrail), but this cannot be set because the Dana pump doesn't allow remote updating the value. The Dana itself is set to 25U. If Trio then automagically updates the value from 10U to 25U because that's what's in the pump: the user may inadvertently end up with a value that is more than twice higher than what they intended to set. The same with Medtronic: user tries to set a value higher than 25U, let's say 40U. This cannot be set remotely, so it'll fail. The Medtronic pump has the value of 40U manually set in it by the user. In this scenario, if the pump is polled: the 40U entered by the user and the 40U on the pump will match so it's fine. But if the pump is set to 35U: it should error out as its not the value the user desired to be set and it cannot be set remotely.

If that makes sense. Sorry if I misunderstood what this PR is supposed to do, this is what I made of it.

bastiaanv · 2024-06-03T21:31:58Z

Kinda. The function doesnt return nil, but always return .success (with the applied values) or .failure (for when something goes wrong).

The silent fail is a danakit bug, where the simulator doesnt return anything and the write command doesnt have a timeout to make sure the cycle is completed. A situation which is only possible by a poor simulator implementation 😅

If something goes wrong (aka .failure is hit), I think trio reads the settings again and restores those values, but I didnt check that flow

marionbarker · 2024-06-03T21:32:55Z

The silent error might be related to the simulator instead of the PR @marionbarker

Could you check the output of the simulator, there is a big chance that it gave the Unsupported error there.

The xcode log is to be correct. I want to let the dev know we cannot set the setting, only fetch

The simulator says:

2024-06-03T14:31:48-07:00 INFO: Sending OPCODE_ENCRYPTION__PUMP_CHECK - Data: paUO8RDluF2jowOcxSaSxSYgXlpa
2024-06-03T14:31:48-07:00 INFO: Sending OPCODE_ENCRYPTION__TIME_INFORMATION - Data: paUD8RGq2TlaWg==
Doing second lvl decryption
2024-06-03T14:31:48-07:00 INFO: Sending OPCODE_ETC__KEEP_CONNECTION - Data: AA==
Doing second lvl decryption
2024-06-03T14:31:48-07:00 INFO: Sending OPCODE_REVIEW__INITIAL_SCREEN_INFORMATION - Data: AAAAAPpddAAAZGQAAAAAAA==
2024-06-03T14:32:08-07:00 INFO: Sending OPCODE_ENCRYPTION__PUMP_CHECK - Data: paUO8RDluF2jowOcxSaSxSYgXlpa
2024-06-03T14:32:08-07:00 INFO: Sending OPCODE_ENCRYPTION__TIME_INFORMATION - Data: paUD8RGq2TlaWg==
Doing second lvl decryption
2024-06-03T14:32:08-07:00 INFO: Sending OPCODE_ETC__KEEP_CONNECTION - Data: AA==
Doing second lvl decryption
2024-06-03T14:32:08-07:00 INFO: Sending OPCODE_REVIEW__INITIAL_SCREEN_INFORMATION - Data: AAAAAPpddAAAZGQAAAAAAA==
Doing second lvl decryption
2024-06-03T14:32:08-07:00 ERROR: UNIMPLEMENTED COMMAND: 103

dnzxy · 2024-06-03T21:38:16Z

Kinda. The function doesnt return nil, but always return .success (with the applied values) or .failure (for when something goes wrong).

The silent fail is a danakit bug, where the simulator doesnt return anything and the write command doesnt have a timeout to make sure the cycle is completed. A situation which is only possible by a poor simulator implementation 😅

If something goes wrong (aka .failure is hit), I think trio reads the settings again and restores those values, but I didnt check that flow

Thanks for clarifying. I had assumed nil because of the nullish coalesce that's being used within that closure's switch-case.

bastiaanv · 2024-06-03T21:38:26Z

The silent error might be related to the simulator instead of the PR @marionbarker

Could you check the output of the simulator, there is a big chance that it gave the Unsupported error there.

The xcode log is to be correct. I want to let the dev know we cannot set the setting, only fetch

The simulator says:


2024-06-03T14:31:48-07:00 INFO: Sending OPCODE_ENCRYPTION__PUMP_CHECK - Data: paUO8RDluF2jowOcxSaSxSYgXlpa

2024-06-03T14:31:48-07:00 INFO: Sending OPCODE_ENCRYPTION__TIME_INFORMATION - Data: paUD8RGq2TlaWg==

Doing second lvl decryption

2024-06-03T14:31:48-07:00 INFO: Sending OPCODE_ETC__KEEP_CONNECTION - Data: AA==

Doing second lvl decryption

2024-06-03T14:31:48-07:00 INFO: Sending OPCODE_REVIEW__INITIAL_SCREEN_INFORMATION - Data: AAAAAPpddAAAZGQAAAAAAA==

2024-06-03T14:32:08-07:00 INFO: Sending OPCODE_ENCRYPTION__PUMP_CHECK - Data: paUO8RDluF2jowOcxSaSxSYgXlpa

2024-06-03T14:32:08-07:00 INFO: Sending OPCODE_ENCRYPTION__TIME_INFORMATION - Data: paUD8RGq2TlaWg==

Doing second lvl decryption

2024-06-03T14:32:08-07:00 INFO: Sending OPCODE_ETC__KEEP_CONNECTION - Data: AA==

Doing second lvl decryption

2024-06-03T14:32:08-07:00 INFO: Sending OPCODE_REVIEW__INITIAL_SCREEN_INFORMATION - Data: AAAAAPpddAAAZGQAAAAAAA==

Doing second lvl decryption

2024-06-03T14:32:08-07:00 ERROR: UNIMPLEMENTED COMMAND: 103

Ah yes, so that is a mistake on my part. To fetch the max basal and max bolus from the pump, I need to issue two commands which arent implemented on the simulator. So testing on the dana simulator is useless and doesnt reflect how the fix/feature works

dnzxy · 2024-06-03T21:39:28Z

So testing on the dana simulator is useless and doesnt reflect how the fix/feature works

How can we verify this works then to merge it?

bastiaanv · 2024-06-03T21:43:07Z

@LiroyvH This PR makes Trio behave the same way as Loop does now. When the command was succesful, use the value from the pumpManager instead of the user input. DanaKit will always return something else than the user input, so that is why this is important for the Dana project. But the pumpManager itself should return a error if the input isnt supported (or return the current limits). This last part is besides this PR. This PR only makes Trio behave the same way Loop does now

marionbarker · 2024-06-03T21:56:29Z

I understand adding an alert may be out of scope of this PR. But should be added to future work.

Trio is silent with a real Medtronic pump. By silent, I mean the Pump Settings screen returns to prior value but doesn't say it cannot save.

Loop doesn't report an error for too large a value because the guardrails will not allow you to send anything greater than what is allowed (by Loop). For my MDT 515, the max is 25 U (same as the guardrails).

If the rileylink is powered off, the modal alert that RL can't be found is prompt.
If the pump is out of range of the RL, takes longer but an Unable to Save alert is displayed.

marionbarker

I believe this solves one problem and other problems can be solved later.
Tested for MDT - the Pump Settings restore to value matching what is on the pump after attempting to save an invalid value.

dnzxy · 2024-06-03T22:03:31Z

Loop doesn't report an error for too large a value because the guardrails will not allow you to send anything greater than what is allowed (by Loop). For my MDT 515, the max is 25 U (same as the guardrails).

I think this is exactly what should not be hard-set by the app but through the pump looking at #247 and at @LiroyvH 's comment.

Thanks for your rigid testing :)

bjornoleh · 2024-06-08T21:32:25Z

@Sjoerd-Bo3 , I think you already approved this PR before some later commit. Would you take another look? Thanks!

Sjoerd-Bo3 requested review from Sjoerd-Bo3, JeremyStorring, bjornoleh, MikePlante1 and avouspierre June 3, 2024 06:50

Sjoerd-Bo3 added the pending review Pull Request by outside contributor that is pending review label Jun 3, 2024

Sjoerd-Bo3 assigned dnzxy Jun 3, 2024

Sjoerd-Bo3 requested changes Jun 3, 2024

View reviewed changes

FreeAPS/Sources/Modules/PumpSettingsEditor/PumpSettingsEditorProvider.swift Show resolved Hide resolved

dnzxy requested a review from Sjoerd-Bo3 June 3, 2024 14:00

Sjoerd-Bo3 previously approved these changes Jun 3, 2024

View reviewed changes

bjornoleh linked an issue Jun 3, 2024 that may be closed by this pull request

Allow more general pump dependent settings for maximum bolus and basal rates #247

Open

bjornoleh removed their request for review June 3, 2024 19:23

dnzxy dismissed Sjoerd-Bo3’s stale review via dbeb61f June 3, 2024 20:32

Use pump limit from pumpManager instead of UI

05d2efb

Co-authored-by: Bastiaan Verhaar <bastiaanv@users.noreply.github.com>

marionbarker approved these changes Jun 3, 2024

View reviewed changes

LiroyvH mentioned this pull request Jun 3, 2024

Allow more general pump dependent settings for maximum bolus and basal rates #247

Open

marionbarker mentioned this pull request Jun 4, 2024

[Bug?] Bolus reduced without warning #271

Closed

bastiaanv mentioned this pull request Jun 6, 2024

Miscellaneous issues with dana-simulator bastiaanv/dana-simulator#2

Closed

Sjoerd-Bo3 mentioned this pull request Jun 8, 2024

Pump Settings screen returns to prior value but doesn't say it cannot save #286

Closed

Sjoerd-Bo3 self-requested a review June 8, 2024 21:56

Sjoerd-Bo3 approved these changes Jun 8, 2024

View reviewed changes

Sjoerd-Bo3 merged commit 8a682e3 into nightscout:alpha Jun 8, 2024
1 check passed

This was referenced Jun 10, 2024

Trio Release 0.2.0 #2

Merged

(Draft) Merge dev into main MikePlante1/Trio#5

Closed

dnzxy deleted the migrate-pump-limit branch June 21, 2024 20:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate fix for PumpManager: Use pump limit from pumpManager instead of UI #266

Migrate fix for PumpManager: Use pump limit from pumpManager instead of UI #266

dnzxy commented Jun 3, 2024

Sjoerd-Bo3 left a comment

bjornoleh commented Jun 3, 2024

bastiaanv commented Jun 3, 2024

bjornoleh commented Jun 3, 2024

marionbarker commented Jun 3, 2024 •

edited

Loading

dnzxy commented Jun 3, 2024

marionbarker commented Jun 3, 2024

dnzxy commented Jun 3, 2024 •

edited

Loading

marionbarker commented Jun 3, 2024 •

edited

Loading

dnzxy commented Jun 3, 2024 •

edited

Loading

bastiaanv commented Jun 3, 2024

dnzxy commented Jun 3, 2024 •

edited

Loading

LiroyvH commented Jun 3, 2024

bastiaanv commented Jun 3, 2024 •

edited

Loading

marionbarker commented Jun 3, 2024

dnzxy commented Jun 3, 2024 •

edited

Loading

bastiaanv commented Jun 3, 2024

dnzxy commented Jun 3, 2024

bastiaanv commented Jun 3, 2024

marionbarker commented Jun 3, 2024

marionbarker left a comment

dnzxy commented Jun 3, 2024

bjornoleh commented Jun 8, 2024

Migrate fix for PumpManager: Use pump limit from pumpManager instead of UI #266

Migrate fix for PumpManager: Use pump limit from pumpManager instead of UI #266

Conversation

dnzxy commented Jun 3, 2024

What is the problem

Why do we need it

What is the impact

Sjoerd-Bo3 left a comment

Choose a reason for hiding this comment

bjornoleh commented Jun 3, 2024

bastiaanv commented Jun 3, 2024

bjornoleh commented Jun 3, 2024

marionbarker commented Jun 3, 2024 • edited Loading

Test Results:

Configuration 1:

Configuration 2:

Build Failure

dnzxy commented Jun 3, 2024

marionbarker commented Jun 3, 2024

Configuration 2: repeated

dnzxy commented Jun 3, 2024 • edited Loading

marionbarker commented Jun 3, 2024 • edited Loading

Test with Dana

Configuration 3:

Test: Attach the dana-simulator to this build of Trio:

Configuration 4:

dnzxy commented Jun 3, 2024 • edited Loading

bastiaanv commented Jun 3, 2024

dnzxy commented Jun 3, 2024 • edited Loading

LiroyvH commented Jun 3, 2024

bastiaanv commented Jun 3, 2024 • edited Loading

marionbarker commented Jun 3, 2024

dnzxy commented Jun 3, 2024 • edited Loading

bastiaanv commented Jun 3, 2024

dnzxy commented Jun 3, 2024

bastiaanv commented Jun 3, 2024

marionbarker commented Jun 3, 2024

marionbarker left a comment

Choose a reason for hiding this comment

dnzxy commented Jun 3, 2024

bjornoleh commented Jun 8, 2024

marionbarker commented Jun 3, 2024 •

edited

Loading

dnzxy commented Jun 3, 2024 •

edited

Loading

marionbarker commented Jun 3, 2024 •

edited

Loading

dnzxy commented Jun 3, 2024 •

edited

Loading

dnzxy commented Jun 3, 2024 •

edited

Loading

bastiaanv commented Jun 3, 2024 •

edited

Loading

dnzxy commented Jun 3, 2024 •

edited

Loading