Port over pump loss fix via LoopKit/Loop#1702

[dnzxy]

Ports over LoopKit/Loop#1702 to fix the loss of pumps and CGMs by no longer storing them as UserDefaults entry but via binary plist files. Storage of CGM info was already done via PR #15 back before Trio was opened for beta. This PR also migrates information storage for pump information.

Quoting from Loop PR:

It's an attempt to fix the issue […] where Loop appears to revert to an old version of device state.
Migration is supported coming from previous versions of Loop that used UserDefaults, but reverting to a previous version of Loop will mean loss of Pump, CGM […] as the previous version of Loop will not know to load the data from the binary plist files.

Thank you to @kskandis and @avouspierre for the great collaboration.

Addresses and solves #231

[avouspierre]

I tried with iphone 8 + RPI Dash simulator with last code :

• Complete new pump installation + OK : ✅
• kill the app after a complete installation - reopen OK with the pod : ✅
• kill the app during the installation of a new pod -after pod insertion - : Message "Finish setup" > Ok (after restarting with pod -q only) : ✅
• kill the app during the installation of a new pod -after pod pair - : Message "Finish setup" > Ok (after restarting with pod -q only) : ✅

[marionbarker]

Problems with PR:

I think we are missing something. I believe this should remember the pump when going from dev to the new branch (but the reverse should not be true.) (Error 1)

Also, with the PR in place, deleting the insulin delivery system doesn't seem to get rid of it. (Error 2) This is for Omnipod ONLY. Did not happen for Medtronic.

Configuration

• iPhone SE
• dev is commit: 14c7643
• pump-loss-fix-migration is commit: c9e747a

Noticed the transition problem - which I thought was wrong.
Deleted all apps on phone with shared app group and started fresh, same problem.

Testing - DASH simulator

• build dev, attach rPi DASH simulator
• (Error 1): build dnzxy:pump-loss-fix-migration, no pump seen in main screen
• build dev, pod is there again
  • deactivate pod
  • main screen shows no pod
• build dnzxy:pump-loss-fix-migration
  • (Error 1): no pump seen in main screen
  • add Omnipod DASH pump
  • connect to rPi DASH simulator
  • build dev over app, it says no pod (doesn't loose the pump selection)
    • I expect this direction to lose the pod
  • Return to dnzxy:pump-loss-fix-migration, connects to the rPi with no issues
  • Deactivate pod, switch to other insulin delivery
  • (Error 2): but main screen still shows no pod, even though tapping on pump shows no pump is attached

Testing - Medtronic

• build dev, attach medtronic (MDT 515 pump)
• (Error 1): build dnzxy:pump-loss-fix-migration, no pump seen in main screen
• build dev, MDT 515 is there again and still connected.
  • confirm that removing Medtronic pump shows no pump icon in Main screen
• build dnzxy:pump-loss-fix-migration, no pump seen in main screen
  • add MDT 515
  • (Error 2 - does NOT happen for MDT pump): delete MDT pump, main screen shows add pump

[bjornoleh]

Succesful test:

Not fresh install, various branches had been build in the past, but:

• Build dev 14c7643
• Attach old, expired Dash pod (NXP)
• Build pump-loss-fix-migration
• Dash pod was connected and worked as it should, except that it had gone silent. I expect this to be related to the fact that the pod expired in 2021, and unrelated to this PR

Testing in reverse, going from pump-loss-fix-migration to dev, the pod was lost (and the sim pump had been automatically selected for some reason).

[kskandis]

Thank you for your thorough testing!

Also, with the PR in place, deleting the insulin delivery system doesn't seem to get rid of it. (Error 2) This is for Omnipod ONLY. Did not happen for Medtronic.

This issue pre-exists in dev, too, so it isn't related to this PR. We should open a separate issue for it?

For other issues, I think once this PR is installed on the iPhone, the nlist pump state persists and will be the first choice as the method of retrieval. So changing pump state on a Dev installation will not be supported once this PR is installed. Is this the way Loop also works? Loop does have ResetLoopManager which gets called from LoopAppManager on launch. We could use this to clear nlists documents for testing.

[dnzxy]

Can only join @kskandis and thank you all for rigid testing (and even burning pods). Thank you! To address the feedback and question:

The expected behaviour should be

1. move from dev (old setup) to PR code (new setup) -> pump is kept
2. move from PR code to dev or any branch that has old setup -> pump is lost

It looks like that @marionbarker seems to have lost the pod (rPi) on both cases, while @bjornoleh's test showed the expected behaviour. I'm unsure how to proceed here.

@marionbarker can you please check if this may be a UI glitch, where the home screen shows no pump ("Add pump"), but when you tap it, it is actually already connected? I have experienced this behaviour with the in-app simulator before and reported it on Discord (no issue so far); just to make sure if the actual pairing is lost, or if the UI just is not updated properly.

[bjornoleh]

Failure during testing:

• Built Trio dev 14c7643 from scratch to a different phone (SE2), with no Loop, iAPS, Trio or xDrip4iOS apps installed
• Parired NXP Dash pod
• Built pump-loss-fix-migration on top
  • lost the pod

[dnzxy]

@kskandis and I will be looking at this more closely and be working towards a solution hopefully later today.

I think it's fair to summarize: we've encountered two issues here (which we will look at):

1. Pump is lost when going FROM UserDefault stored pump pairing TO binary file storage pump pairing
   a. Loss of pump pairing upon REVERSAL from binary file storage to UserDefaults storage is expected and deemed okay.
2. After pump removal, the UI shows "No pod" (= no paired device) but should actually show "Add pump" (=no selected insulin delivery method)

[dnzxy]

With the recent changes introduced by @kskandis I can now report the following:

I can now successfully

• pair pod on current feature branch dnzxy:pump-loss-fix-migration
• switch back to dev and rebuild
• pump is lost now (expected)
• pair new pod to phone while on dev -> successfully paired pod on dev
• switch to feature branch dnzxy:pump-loss-fix-migration -> rebuild -> pump still connected (expected; fixes found error 1 by Marion and Bjørn)

I've force closed, restarted phone, rebuild feature branch multiple times, pod stays paired. Ultimately, pod persists going from dev -> feature; is lost feature -> dev; works as designed.

Tested with rPi pod simulator on an iPhone 11 running iOS 16.7.

There is also a (temporary ❓ ) debug feature to clean the binary files for paired pumps hidden under the debug toggle. This cleans out the files; may be useful for testing this PR and possibly beyond.

[marionbarker]

Status

Success for Omnipod DASH
Comments:

• transition from dev to to pump-loss-fix-migration works as expected
• however, when you return to dev from pump-loss-fix-migration two things happen:
  • pump is lost and must be added back (this is expected)
  • subsequent transition from dev to pump-loss-fix-migration loses the pod unless you delete the app before returning to dev - in that case, expected smooth transition works

This comment has gotten pretty long. Do the Medtronic testing in a new comment.

Configuration

• pump-loss-fix-migration is commit 8a6e8f1 - call this fix for short
• dev is commit 14c7643 (same as yesterday)
• iPhone SE (yesterday wiped out all programs with shared app group, only Trio exists on phone)

Testing - DASH

Test 1: Success: pod maintained going from dev to fix

• initial condition: dev with rPi DASH paired
• build fix: rPi is still connected (bolus command worked)
• NOTE - come back to this later in test 4

Test 2: Success: pod lost going from fix to dev

• build dev:
  • As expected - lose pump:
  • Trio says Error: pump not set
  • main screen says Add Pump
  • add pump and it works (for dev)

Test 3A: Fail: pod NOT maintained going from dev to fix

• return to fix and see that pod is maintained
• but says comms are lost and I'm not able to restore
  • See Test 4
• discard pod and pair fresh

Test 3B: Success - fix works for all pod commands

• now test every pod command before moving forward
  • pair and insert (interrupt each and rebuild with successful behavior)
  • suspend, resume
  • set and cancel temp basal
  • modify confidence reminders and notifications
  • all pod diagnotics
  • bolus
  • loop is green

Test 4: Success: Try the dev to fix transition again

When this was done for Loop, you had to delete the app to go backwards.
Delete Trio (no other apps with shared app group are on the phone)

• build dev and import settings from NS
• attach rPi DASH
• check that all functions work (see Test 3B)
• build fix on phone, pod is still there and communications seem to work
• test every pod command

Test 5: repeat the dev - fix test

Try again building dev over fix without deleting first, then return to fix

• pod is lost as expected - main screen says add pump
• pair rPi DASH
• This time, ^C the rPi before the rebuild.
• build fix over dev
• restart rPi and the issue seen in Test 3A persists.

[marionbarker]

Status

MDT Test #1 a success. Will do the rest of the MDT testing later.

Configuration

• pump-loss-fix-migration is commit 8a6e8f1 - call this fix for short
• dev is commit 14c7643 (same as yesterday)
• iPhone SE - no app on phone (all programs with shared app group previously deleted)

Testing - Medtronic

Test 1: Success: MDT maintained going from dev to fix

• initial condition: build dev onto blank phone
• add Nightscout and import settings
• pair with MDT 515 pump
• wait for green loop, suspend/resume delivery and bolus
• build fix over dev
• maintains connection to MDT
• bolus, suspend/resume, green loop

Need to take a break. I'll do the return to dev testing for MDT later. I'll leave the test phone running closed loop with MDT and fix branch running.

[kskandis]

Thanks for your detailed comments and testing!!

however, when you return to dev from pump-loss-fix-migration two things happen:

• pump is lost and must be added back (this is expected)
• subsequent transition from dev to pump-loss-fix-migration loses the pod unless you delete the app before returning to dev - in that case, expected smooth transition works

In the case where you are moving from PR to Dev, you should use the new Settings - Debug Option "Delete Pump State nlist" Button to remove the persistent nlist doc. Otherwise when you re-install the PR, it will still exist and the PR will use that (rather than the legacy, Dev pump settings) for the pump setttings.

I'm very sorry if I did not explain the Debug Option fully. This is why we need this option, if only temporarily for testing.

[avouspierre]

In the case where you are moving from PR to Dev, you should use the new Settings - Debug Option "Delete Pump State nlist" Button to remove the persistent nlist doc. Otherwise when you re-install the PR, it will still exist and the PR will use that (rather than the legacy, Dev pump settings) for the pump setttings.

Tested with simulator with success. But probably to remove when merged because no reason to open this option...avoid to delete plist associated with a pump without any reason...

[dnzxy]

Status

Success for Omnipod DASH Comments:

• transition from dev to to pump-loss-fix-migration works as expected

• however, when you return to dev from pump-loss-fix-migration two things happen:

  • pump is lost and must be added back (this is expected)
  • subsequent transition from dev to pump-loss-fix-migration loses the pod unless you delete the app before returning to dev - in that case, expected smooth transition works

I cannot reproduce this behaviour.

Here's what I did:

• Clean phone, no other self-built app on phone, fresh iPhone state (reset to factory settings)
• Checkout Trio dev
• Build to test device
• Pair pod using rPi pod simulator
• Pairing process successful
• Play test beeps to verify
• Checkout feature-branch on commit 8a6e8f1
• Rebuild from Xcode to phone (NOTE: pod was still paired, not removed, no nothing!)
• Pod is still paired just fine, UI showing reservoir and battery
• Play test beeps to verify connection
• See screenshot for rPi logs
  
• Checkout dev again
• Rebuild to test phone -> going from feature-branch commit 8a6e8f1 to dev
• Pod is completely lost
• UI shows Add Pump and when tapped guides me through the initial setup of the Omnipod DASH (or any other pump of my choosing)
• rPi shows it's still *** Waiting for the next command *** and Listening for commands

---

• I did not (!) have to use the debug reset functionality.
• I did not have to delete the app on the test phone to get to a state where the pod connection is lost upon building - feature -> dev
• This worked for me out-of-the-box.

How do we want to proceed with the PR? I agree with Pierre, that the debug option should only be kept for testing and must be removed for proper use of the app – nuking pod details is just too big of a risk even when hidden behind the debug toggle.

[kskandis]

• I did not (!) have to use the debug reset functionality.

You only need to use the debug reset functionality if you return back to Dev after installing PR, and then, re-installing PR, so multiple round trips of installation which would a user would not do. I know confusing!

Dev -> PR -> Dev -> PR, so multiple tests. If Debug nlist Reset is NOT performed, the 2nd PR install will not see the 2nd Dev's pump. It will see the 1st PR's pump because nlist is still active!

So install Dev (add pod) -> install PR (see Dev's Pod) -> Tap Debug nlist Reset -> install Dev (Add Pump is displayed, add a pump) -> install PR (see Dev's pump)

[marionbarker]

Summary - Success

• Thanks for the information about the debug option
• Used that prior to building dev over fix, this resolved the requirement to delete the app before building dev
  • See Medtronic Test 2 and Omnipod Test 6 for successful demonstration

Configuration

• pump-loss-fix-migration is commit 8a6e8f1 - call this fix for short
• dev is commit 14c7643
• iPhone SE - only Trio on phone

Testing - Medtronic (continued)

Test 2: build dev on top of fix

• while running fix, use debug option to delete pump state nlist
• build dev over fix
• pump is gone main screen shows add pump (as expected)
• add MDT back
• confirm MDT is working, suspend, resume, bolus, green loop

Test 3: transition from dev to fix

• now build fix over dev again
• pump is still there but is showing signal loss (not uncommon for MDT)
• these functions work: bolus, suspend, resume
• wait to see if signal loss indicator gets restored over time - takes 2 cycles, but green loop does get resolved.

Testing - DASH, part 2

Test 6 (for DASH): build dev over fix and then return to fix - revisited

• start with fix (from Medtronic test immediately preceding this one)
• delete medtronic pump
• add DASH and pair rPi
• exercise the Omnipod functions - accidentally got into the state that needs OmniBLE fix (see ### OmniBLE Test below)
• restore proper operation, confirm all commands work
• while running fix, use debug option to delete pump state nlist
• build dev over fix - lose pod and pump selection as expected
• attach new rPi pod to dev, make sure it works
• build fix over dev, pod is still there and it works correctly
• verify all pod functions are good, have a green loop

OmniBLE Test for OmniBLE PR 125

While running the DASH testing to first suspend and then resume above:

• got into a state where pod state of rPi does not match what Trio thinks
• time stamp is just before 13:00 PDT
  • hx="1724b075240a1d08001f100000000fff0000"
  • extended_bolus_active = False, immediate_bolus_active = False, temp_basal_active = False, basal_active = False
  • but Trio thinks pod resumed successfully and is running scheduled basal of 0.7 U/hr
  • modify the OmniBLE submodule to point at improved_delivery_status
  • rebuild the app
  • it now comes up saying insulin suspended - this is correct! (Add this log to OmniBLE PR 123 as proof this fix works)
  • upload the log file and csv file under that PR to demonstrate the code worked as expected.

[marionbarker]

I approve this as is. There is some question as to whether the debug option to delete (nlist or plist) should be left in. I have no strong opinion on this.

I tested for DASH and Medtronic and both worked as expected.

[bjornoleh]

It seems like this PR is coming together now.

Question: I had different experiences transitioning from dev to fix depending on having either the all previously installed, or more likely having other apps with the shared app group (xdrip4iOS and possibly other instances of Trio or iAPS). Will the fix work correctly also in these cases now?

[marionbarker]

It seems like this PR is coming together now.

Question: I had different experiences transitioning from dev to fix depending on having either the all previously installed, or more likely having other apps with the shared app group (xdrip4iOS and possibly other instances of Trio or iAPS). Will the fix work correctly also in these cases now?

@bjornoleh I think your testing was before the most recent updates which fixed a problem. However, I did not test using more than one app on a phone (after the fix) so am doing that below:

Summary

Success. This app works as expected.

If the debug: delete pump row is removed, the only consequence is for multiple round trips:

• dev to fix will work as expected (pump is maintained)
• fix back to dev will work as expected (pump is lost. Must choose pump type and connect. Pod will be lost)
• second trip from dev to fix (pump is kept but pod is lost because there already is a saved plist for the old pod, must discard pod and pair new)

Recommendation

In my opinion, the debug: delete pump row should be removed.

Other options are to comment out the code or label the row differently and shift it all the way down to the bottom of the menu.

Testing configuration:

• iPhone SE running 17.5.1
• Trio dev branch on phone (commit aeb4ee1)
• build iAPS dev on phone (commit 191e2b1c)
• build Loop-dev on phone (commit c4b4588)
• Trio branch pump-loss-fix-migration is at commit 8a6e8f1 (still has the delete pump button)

Test Details:

• Trio-dev has rPi DASH attached (rPi 4)
• New build of iAPS-dev
  • use default settings except for units and IOB, add Nightscout CGM and a different rPi as DASH simulator (rPi 3b)
• New build of Loop-dev
  • use default settings (press on hold green loop when onboarding to skip), add Nightscout CGM and the MDT 515 as the pump

Status

• All 3 apps on the phone are running at the same time
• Wait until all 3 apps show green loop (takes a while for MDT, that is normal)

Testing

Test Trio dev to pump-loss-fix-migration with other Shared-App Group apps on phone

• build fix over dev, works as expected (rPi 4 continues to communicate)
• build dev over fix (without using the debug button to delete the pump)
  • Main screen indicates Add pump (as expected)
  • Connect to the rPi (4) again
• Now build fix over dev (expect it to have lost signal because I did not previously delete plist)
  • This is what happens, I cannot communicate with the rPi 4 because the previous plist remains)

[marionbarker]

Test again with latest commit (comment out the debug: delete pump button).

Status

Success

Configuration

This continues directly from last comment I made in this PR.

• Trio branch pump-loss-fix-migration is at commit 3ac4c31 (delete pump button is commented out)

Testing

• Trio has rPi-4 DASH attached with earlier commit of fix branch
• build new commit of fix branch, no issue, pod still is communicating
  • deactivate pod from Trio
• Delete MDT pump from Loop
• Add MDT pump to Trio
  • wait for green loop
• Build dev over fix (expect pump to be lost) - success - pump is lost but don't add the pump back
• Build fix over dev - success (pump is still there because the information in plist is still valid for MDT)
  • Confirm I can suspend/resume the MDT

[marionbarker]

Tested before the final formatting change.

[marionbarker]

This modification was requested because Xcode was auto-formatting the files to make the comments line up and that resulted in changes in the local clone with respect to GitHub.

Code review of final commit.

• This is only a change to the format for some comment lines.
• There is no need to retest functionality of the code.
• I built with Xcode and confirmed there are no modifications to files automtically made by Xcode

[bjornoleh]

Approving based on initial testing (partial success) and review of Marion's successful testing of the last changes after my tests with actual pods.

Verified that the app builds without uncommitted changes.