Temp fix for #32875 (network commissioning cluster check for SSID on Thread feature) #32884
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
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
arkq,
bzbarsky-apple,
carol-apple,
cecille,
chshu,
chulspro,
cliffamzn,
Damian-Nordic and
dhrishi
tcarmelveilleux
requested changes
Apr 6, 2024
There was a problem hiding this comment.
- Change description does not indicate what is being changed, or why, only links to an issue number, making tracing harder. Please describe the change.
- Change is removing a check that was deemed correct by spec, and spec interpretation should be validated with TSG before merging.
Note that TC-CNET-4.22 was missing the fix for TH == null (https://github.com/CHIP-Specifications/chip-test-plans/pull/3843), see TC-CNET-4.22 (https://github.com/CHIP-Specifications/chip-test-plans/blob/master/src/cluster/network_commissioning.adoc#4215-tc-cnet-422-thread-verification-for-scannetworks-command-dut-server), so step 3 implies that the test should pass. Given that the code at play was present at SVE, and nobody failed that test, it begs the question about whether the test was run at all!
chrisdecenzo
changed the title
s-mcclain
approved these changes
Apr 8, 2024
|
PR #32884: Size comparison from f46cdf3 to cbac1d7 Decreases (7 builds for efr32, esp32, nrfconnect, stm32)
Full report (22 builds for bl602, bl702, bl702l, cc32xx, efr32, esp32, mbed, nrfconnect, stm32)
|
tcarmelveilleux
approved these changes
Apr 9, 2024
robszewczyk
approved these changes
Apr 9, 2024
Damian-Nordic
approved these changes
Apr 9, 2024
jmartinez-silabs
pushed a commit
to jmartinez-silabs/connectedhomeip
that referenced
this pull request
Apr 17, 2024
…for SSID on Thread feature) (project-chip#32884) * Temp fix for 32875 * Clarify comments
jmartinez-silabs
added a commit
that referenced
this pull request
Apr 17, 2024
…Thread feature) (#32884) (#33032) * Temp fix for 32875 * Clarify comments Co-authored-by: chrisdecenzo <[email protected]>
jmartinez-silabs
added a commit
to SiliconLabs/matter
that referenced
this pull request
May 8, 2024
…ix for project-chip#32875 (network commissioning cluster check for SSID on Thread feature) (project-chip#32884) Merge in WMN_TOOLS/matter from cherry-pick/remove_no_ssid_check to RC_2.3.0-1.3 Squashed commit of the following: commit 29c1eb165d6e322bb106ef200950c57485118b52 Author: chrisdecenzo <[email protected]> Date: Tue Apr 9 08:27:01 2024 -0700 Temp fix for project-chip#32875 (network commissioning cluster check for SSID on Thread feature) (project-chip#32884) * Temp fix for 32875 * Clarify comments
rcasallas-silabs
pushed a commit
to rcasallas-silabs/connectedhomeip
that referenced
this pull request
Jun 20, 2024
…ix for project-chip#32875 (network commissioning cluster check for SSID on Thread feature) (project-chip#32884) Merge in WMN_TOOLS/matter from cherry-pick/remove_no_ssid_check to RC_2.3.0-1.3 Squashed commit of the following: commit 29c1eb165d6e322bb106ef200950c57485118b52 Author: chrisdecenzo <[email protected]> Date: Tue Apr 9 08:27:01 2024 -0700 Temp fix for project-chip#32875 (network commissioning cluster check for SSID on Thread feature) (project-chip#32884) * Temp fix for 32875 * Clarify comments
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.
Fixes #32875
As part of network commissioning cluster cleanup in 1.3, a check was added to the scan networks handler which checks for the SSID argument on a Thread device and returns an error when found.
There is confusing text in the spec which states that non-WiFi implementations should ignore this argument when null, and so the new check appears to not be conformant to that. But regardless, a popular commissioner was found to be passing a null SSID argument and this change breaks Thread device commissioning for users with that commissioner.
The spec needs to be clarified on this point, and the commissioner needs to be fixed. The commissioner fix is in the works but could take months to fully roll out to customers. In the meantime, this PR comments out the SSID check and a separate issue #32887 has been created to track that work.
Also, I have confirmed that this fix addresses the problem in #32875 (eg. no other fixes are needed).