Properly handle commissioning error paths #16882
Merged
Conversation
This file contains 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
Also callback with full peer id for folks that want that.
The current failsafe expiry doesn't actually re-start the advertising because the pase connection stops the commissioning window timer. Instead, stop the commissining window timer when we get a commissioning complete. Don't use the OnSessionEstablishmentError function for failsafe timer expiry because it sends a callback about pairing that does not apply here.
pullapprove
bot
requested review from
anush-apple,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
erjiaqing,
franck-apple,
gjc13,
harimau-qirex,
hawk248 and
harsha-rajendran
msandstedt
reviewed
Apr 2, 2022
Most commands don't set it - the last one that did was the failsafe.
|
PR #16882: Size comparison from a208c3d to 3f2d51b Increases above 0.2%:
Increases (31 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
Decreases (3 builds for cc13x2_26x2)
Full report (31 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
Co-authored-by: Boris Zbarsky <[email protected]>
bzbarsky-apple
approved these changes
Apr 4, 2022
|
PR #16882: Size comparison from a208c3d to 727c636 Increases above 0.2%:
Increases (31 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
Decreases (3 builds for cc13x2_26x2)
Full report (31 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
msandstedt
approved these changes
Apr 4, 2022
tcarmelveilleux
approved these changes
Apr 4, 2022
|
PR #16882: Size comparison from a208c3d to 9388830 Increases above 0.2%:
Increases (31 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
Decreases (3 builds for cc13x2_26x2)
Full report (31 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
chencheung
pushed a commit
to chencheung/connectedhomeip
that referenced
this pull request
Apr 6, 2022
* Don't re-establish PASE if we have a connection. * Fix incorrect error return for pase with no pairing delegate * Add commissioning status update callback Also callback with full peer id for folks that want that. * Disarm failsafe immediately if we get 0 timeout. * Fix event handling in commissioning window manager. The current failsafe expiry doesn't actually re-start the advertising because the pase connection stops the commissioning window timer. Instead, stop the commissining window timer when we get a commissioning complete. Don't use the OnSessionEstablishmentError function for failsafe timer expiry because it sends a callback about pairing that does not apply here. * Fail re-send if the session is expired. * Disarm failsafe and kill pase connection for early failures * Clear event on BLE commissioning. * Restyled by clang-format * Restyled by autopep8 * Add ExpireAllPASEPairings function to session manager Use in CommissioningWindowManager * Restyled by clang-format * Address review comments * Boris' reliable transmission fix. * Revert back to using > 0 for breadcrumb. Most commands don't set it - the last one that did was the failsafe. * manually cancel timer. * Apply suggestions from code review Co-authored-by: Boris Zbarsky <[email protected]> * Add API documentation * Move log line down. * Restyled by clang-format * Boris' fix for darwin tests. Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Boris Zbarsky <[email protected]>
chencheung
pushed a commit
to chencheung/connectedhomeip
that referenced
this pull request
Apr 6, 2022
* Don't re-establish PASE if we have a connection. * Fix incorrect error return for pase with no pairing delegate * Add commissioning status update callback Also callback with full peer id for folks that want that. * Disarm failsafe immediately if we get 0 timeout. * Fix event handling in commissioning window manager. The current failsafe expiry doesn't actually re-start the advertising because the pase connection stops the commissioning window timer. Instead, stop the commissining window timer when we get a commissioning complete. Don't use the OnSessionEstablishmentError function for failsafe timer expiry because it sends a callback about pairing that does not apply here. * Fail re-send if the session is expired. * Disarm failsafe and kill pase connection for early failures * Clear event on BLE commissioning. * Restyled by clang-format * Restyled by autopep8 * Add ExpireAllPASEPairings function to session manager Use in CommissioningWindowManager * Restyled by clang-format * Address review comments * Boris' reliable transmission fix. * Revert back to using > 0 for breadcrumb. Most commands don't set it - the last one that did was the failsafe. * manually cancel timer. * Apply suggestions from code review Co-authored-by: Boris Zbarsky <[email protected]> * Add API documentation * Move log line down. * Restyled by clang-format * Boris' fix for darwin tests. Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Boris Zbarsky <[email protected]>
andrei-menzopol
pushed a commit
to andrei-menzopol/connectedhomeip
that referenced
this pull request
Apr 14, 2022
* Don't re-establish PASE if we have a connection. * Fix incorrect error return for pase with no pairing delegate * Add commissioning status update callback Also callback with full peer id for folks that want that. * Disarm failsafe immediately if we get 0 timeout. * Fix event handling in commissioning window manager. The current failsafe expiry doesn't actually re-start the advertising because the pase connection stops the commissioning window timer. Instead, stop the commissining window timer when we get a commissioning complete. Don't use the OnSessionEstablishmentError function for failsafe timer expiry because it sends a callback about pairing that does not apply here. * Fail re-send if the session is expired. * Disarm failsafe and kill pase connection for early failures * Clear event on BLE commissioning. * Restyled by clang-format * Restyled by autopep8 * Add ExpireAllPASEPairings function to session manager Use in CommissioningWindowManager * Restyled by clang-format * Address review comments * Boris' reliable transmission fix. * Revert back to using > 0 for breadcrumb. Most commands don't set it - the last one that did was the failsafe. * manually cancel timer. * Apply suggestions from code review Co-authored-by: Boris Zbarsky <[email protected]> * Add API documentation * Move log line down. * Restyled by clang-format * Boris' fix for darwin tests. Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Boris Zbarsky <[email protected]>
This was referenced Apr 19, 2022
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.
Problem
There was little error handling in the commissioner. This changes the commissioner to clear the failsafe on error, which will notify the device to begin accepting pase connections again. Also fixes the handling of the pase session - it will be closed if commissioning errors before adding the network, but will be retained if network setup was in progress.
Also adds some additional callbacks to give the delegate more insight into the commissioning processes and to facilitate testing.
Apologies for the large PR - I have separated the individual steps into separate commits to facilitate review.
Change overview
Testing
New cirque tests to cover error cases, previous tests cover success case.