-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improved unacknowledged command recovery #47
Improved unacknowledged command recovery #47
Conversation
New resolveUnacknowledgedCommand() attempts a GetStatusCommand to resolve any unacknowledged command or throws .unacknowledgedCommandPending on failure. Have all PodCommSession funcs call resolveUnacknowledgedCommand() instead of failing if called with a pending unacknowledged command.
… code Fix deactivatePod() logic error to correctly handle an unacknowledged command Simplify logging by not including the calling function's name
Initial ReviewThe pair of PR, OmniKit #47 and OmniBLE PR 139: Improved unacknowledged command recovery, will be reviewed and tested in tandem. SummaryFor every instance in which the If the recovery succeeds, then the app continues without alerting the user. This should provide a smoother user experience. Code Review
Build Test
TestingMost of the testing will be done with OmniBLE using the rPi simulator. After that testing is completed and reported in the OmniBLE PR, an Eros pod will be tested. These test results will be provided in future comments. |
SimilarityThe PR changes for OmniBLE PR 139 and OmniKit PR 47 are identical. If one works, they both should work. Test:Repeat a subset of Test: Part 2 from OmniBLE PR 139 using an Eros pod. The fact that this subset behaved the same with a real Eros pod as it did with the rPi DASH simulator indicates this code works for OmniKit. Test MethodAdjust the test patch used for OmniBLE to work with OmniKit. This patch uses two private vars Because this test is done with a real pod (not a pod simulator) the test order is modified from the OmniBLE testing. Test comm errorsInitial configuration:
Pair, Prime and Insert with new Pod
2-message commands Test the commands below:
Do not test the rest of the items from OmniBLE Test: Part 2. These tests are not needed to approve this PR.
Final ConfigurationBuild again on the iPhone 7 connected to the Eros pod without the testing patch. Let it run to pod completion.This does not need to complete in order to approve this PR. But may as well let it run. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Status
Code review: Approve
Similar to OmniBLE, so limited testing is OK
Test: Part 2: Success
I approve this PR.
Fix for LoopKit/Loop#2275
Use new tryToResolvePendingCommand() function that try to use getStatus() to resolve a
pending unacknowledged command in most PodCommSession functions instead of just
failing if called with a pending unacknowledged command. Reworked logic to prevent
possible incorrect unacknowledged command handling during pod setup & deactivation.