Correctly handle nonretryable pairing faults (#3)#57
Open
itsmojo wants to merge 4 commits into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reopened copy of OmnipodKit PR #52 & #56 that were both closed during repo rework
While testing an early "five" version of the pod sim which incorrectly gave responses with a DASH product id (4) instead of as an O5 product id (5), I found that the UI pairing code ended up doing an auto-retry on what should have been a fatal pod is incompatible error that this operation actually succeeded due to the various things done that were done to make the pairing code more robust on retries. But this resulted in the supposedly fatal failure never being seen by the user. This artificial example actually demonstrated an issue that any pod setup failure such as activation took too long or any pod fault was currently handled incorrectly and allowed to auto-retry and then the user is offered a chance to retry that shouldn't have been even presented.
As turns out, there were a number of related mistakes. One is that the PodCommsState enum .fault state was using podState.fault to test for a faulted pod (which is for only normal fault codes) and not the more extensive podState.isFaulted (which also handles the pseudo faults for activation taking too long and incompatible pod). Fixing this definition means that the DetailedStatus returned for this state had to be re-declared as an Optional since there is no DetailedStatus for the activationTimeout and podIncompatible pseudo faults which are based on the SetupProgress/PodProgressStatus enums.
Additionally, pairAndPrime needs to use different return types for recoverable vs non-recoverable failures so the UI can know how to proceed. Previously all sessions errors were being treated as recoverable communications errors. Instead, podState?.isFaulted == true can be used to test for any faulted pod to return a non-recoverable deviceState error in these cases. The PairPodViewModel needed to updates its testing for non-recoverable errors and faulted pods. And finally, the PaidPodView had some logic issues for dealing with non-recoverable errors. Previously for these cases it would display both a red "Deactivate Pod" button as well as Blue "Abort" button which ended up doing the same thing to go the deactivate pod view.
Again, note that this example pod failure is for an artificial example that should never happen with real pods, but pod faults certainly do happen during pairing. However, it is worth noting that actual pod faults occurring during pairing is doesn't really occur all that often. Unfortunately I couldn't get my pod_frontend_simulator to work today and so I was unable to test using induced fake pod faults during pairing.
Removing the redundant red Deactivate Pod button, on a fake pod fault during pairing you'd see the following.
But since the Abort operation for a pod fault is destructive, this button should really be shown in red which is what this PR now does.