Enable Wi-Fi Management only when required #39172
Open
+53
−39
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
* Postpone to start Wi-Fi Management while needed Signed-off-by: Lo,Chin-Ran <[email protected]>
crlonxp
changed the title
|
PR #39172: Size comparison from a9afc79 to cd0e4d0 Increases above 0.2%:
Full report (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
andy31415
requested changes
May 25, 2025
There was a problem hiding this comment.
https://github.com/project-chip/connectedhomeip/actions/runs/15237267578/job/42852799995?pr=39172 still shows 20+ seconds of runtime for checks (instead of the expected 3 seconds):
...
TV_ChannelCluster - Starting test
INFO WARNING:root:TAG configurator::cluster::attribute::quality was not handled/recognized at src/app/zap-templates/zcl/data-model/chip/thread-network-diagnostics-cluster.xml:131:37
INFO WARNING:root:TAG configurator::cluster::command::describedConform was not handled/recognized at src/app/zap-templates/zcl/data-model/chip/actions-cluster.xml:109:25
INFO WARNING:root:TAG configurator::cluster::command::quality was not handled/recognized at src/app/zap-templates/zcl/data-model/chip/webrtc-requestor-cluster.xml:39:36
INFO WARNING:root:TAG configurator::cluster::attribute::describedConform was not handled/recognized at src/app/zap-templates/zcl/data-model/chip/bridged-device-basic-information-cluster.xml:84:25
INFO TV_ChannelCluster - Completed in 22.44 seconds
INFO TV_ContentLauncherCluster - Starting test
INFO WARNING:root:TAG configurator::cluster::attribute::quality was not handled/recognized at src/app/zap-templates/zcl/data-model/chip/thread-network-diagnostics-cluster.xml:131:37
INFO WARNING:root:TAG configurator::cluster::command::describedConform was not handled/recognized at src/app/zap-templates/zcl/data-model/chip/actions-cluster.xml:109:25
INFO WARNING:root:TAG configurator::cluster::command::quality was not handled/recognized at src/app/zap-templates/zcl/data-model/chip/webrtc-requestor-cluster.xml:39:36
INFO WARNING:root:TAG configurator::cluster::attribute::describedConform was not handled/recognized at src/app/zap-templates/zcl/data-model/chip/bridged-device-basic-information-cluster.xml:84:25
INFO TV_ContentLauncherCluster - Completed in 21.99 seconds
INFO TV_KeypadInputCluster - Starting test
INFO WARNING:root:TAG configurator::cluster::attribute::quality was not handled/recognized at src/app/zap-templates/zcl/data-model/chip/thread-network-diagnostics-cluster.xml:131:37
INFO WARNING:root:TAG configurator::cluster::command::describedConform was not handled/recognized at src/app/zap-templates/zcl/data-model/chip/actions-cluster.xml:109:25
INFO WARNING:root:TAG configurator::cluster::command::quality was not handled/recognized at src/app/zap-templates/zcl/data-model/chip/webrtc-requestor-cluster.xml:39:36
INFO WARNING:root:TAG configurator::cluster::attribute::describedConform was not handled/recognized at src/app/zap-templates/zcl/data-model/chip/bridged-device-basic-information-cluster.xml:84:25
INFO TV_KeypadInputCluster - Completed in 21.99 seconds
INFO TV_LowPowerCluster - Starting test
...
andy31415
reviewed
May 25, 2025
|
PR #39172: Size comparison from a9afc79 to 6e124e1 Increases above 0.2%:
Full report (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #39172: Size comparison from 9af93fd to 97834a3 Full report (7 builds for cc32xx, qpg, stm32, tizen)
|
|
PR #39172: Size comparison from 9af93fd to 25d46ab Full report (27 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, nrfconnect, qpg, stm32, telink, tizen)
|
Not enabling Wi-Fi-PAF in the test example Signed-off-by: Lo,Chin-Ran <[email protected]>
|
PR #39172: Size comparison from 9af93fd to 44b8e2e Increases above 0.2%:
Full report (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
andy31415
reviewed
May 26, 2025
| @@ -38,7 +38,7 @@ jobs: | |||
|
|
|||
| strategy: | |||
| matrix: | |||
| build_variant: [no-ble-no-shell-tsan-clang] | |||
| build_variant: [no-ble-no-wifipaf-no-shell-tsan-clang] | |||
There was a problem hiding this comment.
This means that we still try to get PAF as a default but make tests fast?
Why is WifiPAF a default if it is slow? Default to false seems correct then.
There was a problem hiding this comment.
Do you mean to set chip_device_config_enable_wifipaf=true as default? It should be enabled as default to be accepted as the new feature, shouldn't it?
From the log we got, if we enable it by default, the qr-code has both wifi-paf and onnetwork supported. If wifi-paf is packed, it will start Wi-Fi management. But I suspect the test environment does not have it ready that it always failed and take a long time to do it.
There was a problem hiding this comment.
I mean chip_device_config_enable_wifipaf=false
There was a problem hiding this comment.
Yes, we still hope to keep PAF enabled in the SDK and make test faster (or said, back to normal)
Making test slow is unexpected. (The reason is mentioned above) We hope to fix the problems, instead of just disabling it.
There was a problem hiding this comment.
It should be enabled as default to be accepted as the new feature, shouldn't it?
Enabling by default in SDK is NOT NEEDED.
You can have a feature accepted at SVE without it being enable by default in SVE, otherwise all features would be enabled all the time, even features that may conflict between each other.
There was a problem hiding this comment.
If the feature is disabled by default, all tests and builds under PAF are also disabled.
However, the specific test case (no-ble-noshell-tsan-clang) does not require PAF.
This PR addresses the issue by adding a flag to disable PAF for this test case. This is expected as the test case only runs on-network commissioning
There was a problem hiding this comment.
My concern is that this shows a significant speed regression if we enable it by default, which even if we disable it in CI we will still have developers run defaults and have their workflow disabled.
I do not want a 4x slowdown for my local tests because I just use defaults.
Please do the reverse: disable PAF by default and enable the PAF flag specifically where it is needed instead of doing "enable PAF by default and disable where we see slowdowns". This is more friendly for developers that do not test PAF features as enabling PAF seems to have performance sideffects.
andy31415
reviewed
May 26, 2025
| @@ -126,6 +123,12 @@ declare_args() { | |||
| declare_args() { | |||
| # Enable Joint Fabric features | |||
| chip_device_config_enable_joint_fabric = false | |||
|
|
|||
| # Include wifi-paf to commission the device or not | |||
| # This is a feature of Wi-Fi spec that it can be enabled if wifi is enabled | |||
There was a problem hiding this comment.
why do we insist on having this default to true on linux?
There was a problem hiding this comment.
It should be default to true to be accepted as a new feature, shouldn't it? I remember that's the reason why this feature was rejected in Matter 1.4 few months ago.
There was a problem hiding this comment.
I would not expect compile defaults to make a difference in feature acceptance. You may need to modify the certification binaries to enable it by default. We already change some defaults in https://github.com/project-chip/connectedhomeip/blob/master/integrations/docker/images/chip-cert-bins/Dockerfile#L150 (we set platform mdns instead of minmdns and we disable ipv4)
There was a problem hiding this comment.
If this PR can fix the problems, could you let me know why will we still prefer leaving it disabled? If you suspect there are new unknown problems ahead, could you let me know what other tests should we check? Thank you.
There was a problem hiding this comment.
I would like to have it disabled because local developers just compile things with defaults in most cases even if CI varies arguments. I do not want developers to experience a 4x slowdown in test performance because they enable a feature they do not use or test. developers or CI tests that do test PAF can enable it explicitly.
Or alternative would be to find a way to bypass that slowdown. I am unclear still what is going on, is it that we have "PAF enabled, so PAF shows in QR code so PAF startup is slow"? We have to either find a way for "defaults do not slow down things unless this is really used" or not have it as a default.
|
Chin-ran’s PR was tested on his side that would bypass that slowdown and results in “defaults do not slow down things unless this is really used”.
Xiaolong
From: Andrei Litvin ***@***.***>
Sent: Tuesday, May 27, 2025 7:32 AM
To: project-chip/connectedhomeip ***@***.***>
Cc: Xiaolong Huang ***@***.***>; Assign ***@***.***>
Subject: Re: [project-chip/connectedhomeip] Enable Wi-Fi Management only when required (PR #39172)
WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros.
@andy31415 commented on this pull request.
________________________________
In src/platform/device.gni<#39172 (comment)>:
@@ -126,6 +123,12 @@ declare_args() {
declare_args() {
# Enable Joint Fabric features
chip_device_config_enable_joint_fabric = false
+
+ # Include wifi-paf to commission the device or not
+ # This is a feature of Wi-Fi spec that it can be enabled if wifi is enabled
I would like to have it disabled because local developers just compile things with defaults in most cases even if CI varies arguments. I do not want developers to experience a 4x slowdown in test performance because they enable a feature they do not use or test. developers or CI tests that do test PAF can enable it explicitly.
Or alternative would be to find a way to bypass that slowdown. I am unclear still what is going on, is it that we have "PAF enabled, so PAF shows in QR code so PAF startup is slow"? We have to either find a way for "defaults do not slow down things unless this is really used" or not have it as a default.
—
Reply to this email directly, view it on GitHub<#39172 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/A7XEZOB2O3YN5JSZF63X4KL3ARZNRAVCNFSM6AAAAAB53UZFECVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDQNZRGM2DEOJVG4>.
You are receiving this because you were assigned.Message ID: ***@***.******@***.***>>
|
|
@andy31415 Chin-ran’s PR was tested on his side that would bypass that slowdown and results in “defaults do not slow down things unless this is really used”. Do you still see any slow down for tests with the PR outside Wi-Fi commissioning? If so, I think the resolution is good. |
| # This is a feature of Wi-Fi spec that it can be enabled if wifi is enabled | ||
| # and the supplicant can support. | ||
| chip_device_config_enable_wifipaf = | ||
| chip_enable_wifi && chip_device_platform == "linux" |
There was a problem hiding this comment.
This should be MANUALLY enabled when desired, not "all linux unless overridden to false".
It seems that CI had to be updated in this PR ... this seems to indicate that slowdown was probably not removed completely. |
|
@crlonxp @xiaolonghca to be specific on my changes requested: I am asking to not slow down things that are not related to PAF. CI is one obvious thing (adding 1.5-2h of CI time for no additional test coverage seems clearly wrong) however this is also about local development. Locally I tested and master with/without BLE on my cloud VM works within 4 seconds roughly (added ble test to see if ble options make a difference and they do not):
I did recompile on a separate shell that compiled all variants (i.e. the variant change DID take effect even if no compile step is shown). However then I applied this PR and the time went to 24 seconds
This is the bit that you have to fix in order to make "paf enabled by default" acceptable. Until then, PAF should be disabled by default since it is an optional feature that seems to significantly impact chip-tool somehow. |
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.
Testing
Environment:
AP: Asus RT-N66U, running at chnl#1
commissioner: Rpi + NetGear, Inc. A6210
commissionee#1: rpi + Linksys AE6000
commissionee#2: imx93+iw612
commands:
[commissionee]
$ ./chip-all-clusters-app --wifi --wifipaf freq_list=2412
[commissioner]
$ ./chip-tool pairing code-wifi 1 n_m_2g nxp12345 MT:-24J0M3810KA0648G00 --freq 2412
or
$ ./chip-tool pairing wifipaf-wifi 1 n_m_2g nxp12345 20202021 3840 --freq 2412