Closed dnzxy closed 3 weeks ago
I tried with iphone 8 + RPI Dash simulator with last code :
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.
Noticed the transition problem - which I thought was wrong. Deleted all apps on phone with shared app group and started fresh, same problem.
Not fresh install, various branches had been build in the past, but:
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).
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.
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
dev
(old setup) to PR code (new setup) -> pump is keptdev
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.
@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):
With the recent changes introduced by @kskandis I can now report the following:
I can now successfully
dnzxy:pump-loss-fix-migration
dev
and rebuilddev
-> successfully paired pod on dev
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.
Success for Omnipod DASH Comments:
This comment has gotten pretty long. Do the Medtronic testing in a new comment.
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)
Try again building dev over fix without deleting first, then return to fix
MDT Test #1 a success. Will do the rest of the MDT testing later.
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.
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.
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...
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:
dev
8a6e8f1
dev
again8a6e8f1
to dev
Add Pump
and when tapped guides me through the initial setup of the Omnipod DASH (or any other pump of my choosing)*** Waiting for the next command ***
and Listening for commands
feature -> dev
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.
- 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)
While running the DASH testing to first suspend and then resume above:
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?
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:
Success. This app works as expected.
If the debug: delete pump row is removed, the only consequence is for multiple round trips:
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.
pump-loss-fix-migration
is at commit 8a6e8f18 (still has the delete pump
button)Test Trio dev
to pump-loss-fix-migration
with other Shared-App Group apps on phone
Test again with latest commit (comment out the debug: delete pump button).
Success
This continues directly from last comment I made in this PR.
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:
Thank you to @kskandis and @avouspierre for the great collaboration.
Addresses and solves #231