Fixing pairing failure, where auto commissioner, when used back to back will fail pairings for devices on the network already#43251
Conversation
…ck will fail pairings for devices on the network already
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request correctly addresses a state leakage issue in the AutoCommissioner class where the mNeedsNetworkSetup flag was not being reset during the commissioning cleanup process. This fix is essential for ensuring that back-to-back commissioning attempts are reliable and do not inherit state from previous sessions. The implementation follows the prevailing coding style of the file. I have suggested resetting a few additional state-tracking flags in the same function to further improve the robustness of the cleanup logic and prevent similar issues in the future.
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the AutoCommissioner where the mNeedsNetworkSetup flag was not being reset in the CleanupCommissioning() method, causing back-to-back pairing attempts to fail when the AutoCommissioner is reused. The flag is used to determine whether network setup is needed during commissioning (set when transport type is BLE, NFC, or WiFiPAF), and if not cleared, could cause incorrect commissioning flow for subsequent devices.
Changes:
- Added reset of
mNeedsNetworkSetupflag tofalseinAutoCommissioner::CleanupCommissioning()
|
PR #43251: Size comparison from 7f6dd32 to ca1f22f Full report (31 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43251 +/- ##
==========================================
- Coverage 53.73% 53.73% -0.01%
==========================================
Files 1530 1530
Lines 105861 105866 +5
Branches 13312 13312
==========================================
Hits 56884 56884
- Misses 48977 48982 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Summary
The mNeedsNetworkSetup flag never Gets Reset in CleanupCommissioning()
Location: connectedhomeip/src/controller/AutoCommissioner.cpp:781-790
void AutoCommissioner::CleanupCommissioning()
{
ResetNetworkAttemptType();
mPAI.Free();
mDAC.Free();
mCommissioneeDeviceProxy = nullptr;
mOperationalDeviceProxy = OperationalDeviceProxy();
mDeviceCommissioningInfo = ReadCommissioningInfo();
mNeedsDST = false;
// ❌ mNeedsNetworkSetup is NOT reset here!
}
This may be possible given that there was a previous pairing attempt to a device over BLE, which might have setup mNeedsNetworkSetup.
Testing
Ran through manual testing of pairing of two devices.
Readability checklist
The checklist below will help the reviewer finish PR review in time and keep the
code readable:
descriptive
“When in Rome…”
rule (coding style)
See: Pull Request Guidelines