Give commissionees at least ScanMaxTimeSeconds when asking for network scans. #40711
+55
−17
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
…k scans. We were just doing the 30-second default instead of checking what ScanMaxTimeSeconds is.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the commissioning process to properly respect the ScanMaxTimeSeconds attribute when performing network scans, instead of using a hardcoded 30-second default timeout.
- Adds reading of the
ScanMaxTimeSecondsattribute during commissioning info collection - Implements proper timeout handling for network scan operations based on the retrieved values
- Fixes a typo in an error message
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/controller/CommissioningDelegate.h | Adds maxScanTime field to NetworkClusterInfo struct to store scan timeout values |
| src/controller/CHIPDeviceController.cpp | Implements reading of ScanMaxTimeSeconds attribute and fixes error message typo |
| src/controller/AutoCommissioner.cpp | Uses the retrieved scan timeout values for kScanNetworks commissioning stage |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![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
This pull request correctly implements the use of ScanMaxTimeSeconds from the commissionee for network scans, replacing the previous hardcoded timeout. The changes are logical and well-contained. I have one suggestion to improve maintainability by refactoring duplicated code in src/controller/CHIPDeviceController.cpp.
|
PR #40711: Size comparison from f1631b6 to bde2d56 Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40711 +/- ##
==========================================
+ Coverage 50.71% 50.73% +0.01%
==========================================
Files 1355 1356 +1
Lines 99303 99358 +55
Branches 12877 12873 -4
==========================================
+ Hits 50366 50409 +43
- Misses 48937 48949 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jmartinez-silabs
approved these changes
Aug 23, 2025
| } | ||
| return return_err; | ||
| } | ||
|
|
||
| CHIP_ERROR DeviceCommissioner::ParseNetworkCommissioningTimeouts(NetworkClusterInfo & networkInfo, const char * networkType) |
There was a problem hiding this comment.
Do we need the network type string ? It's used for logging info. Is it safe to use if a non null terminated buffer is passed
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.
We were just doing the 30-second default instead of checking what ScanMaxTimeSeconds is.
Testing
Manual for now; we have no good way to test things with specific ScanMaxTimeSeconds values.