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

[dnzxy]

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

[bjornoleh]

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]

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]

Thanks, then I think we can say that https://github.com/nightscout/Trio/issues/247 will be closed with the merging of this PR!

[marionbarker]

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 14c76432
  • 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 14c76432 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: a06cd43c (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]

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]

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

• dbeb61fc (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.

[dnzxy]

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]

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]

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]

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]

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

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

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]

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]

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]

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]

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]

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

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.

[dnzxy]

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]

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