Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft enabling wifipaf on sample apps #34716

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Draft enabling wifipaf on sample apps #34716

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5c821dd
Draft enabling wifipaf on sample apps
gladystonfranca Aug 1, 2024
eda0be9
Fixing issue
gladystonfranca Aug 1, 2024
2b49379
Update src/platform/device.gni
andy31415 Aug 1, 2024
d0c4fd3
Update src/platform/device.gni
andy31415 Aug 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/platform/device.gni
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ declare_args() {
chip_fake_platform = false

# Include wifi-paf to commission the device or not
chip_device_config_enable_wifipaf = false
chip_device_config_enable_wifipaf = true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a reason to have wifipaf as a default?
Otherwise we should just change what the TH builds, like I see https://github.com/project-chip/connectedhomeip/blob/master/integrations/docker/images/chip-cert-bins/Dockerfile#L177

You could add a wifi-paf variant to build things like linux-x64-all-clusters-ipv6only-wifi-paf.

To get that variant working you add the variant in https://github.com/project-chip/connectedhomeip/blob/master/scripts/build/build/targets.py#L187 and make https://github.com/project-chip/connectedhomeip/blob/master/scripts/build/builders/host.py#L331 support some form of flag that makes the gn argument take some value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andy31415 I don't have answer for that if it should be default or not. I just realized support was not there. Thank you for your quick answer Andrei. I will raise up this question and get back here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andy31415 we are going to use the chip-cert-bins in the TH image for testing , also the way the test plan is written , it uses the same sample app for validating the commissioning flow for BLE , Wifi and on network , so I guess we would have to enable it by default ?
Let me start a conversation with other folks in slack as well

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that you also need a patched wpa_supplicant version to actually have working PAF support I don't think it's helpful to enable this by default. Is the TH not able to pass build arguments to the binaries it builds?

Copy link
Contributor

@ksperling-apple ksperling-apple Aug 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I also think in general it's unhelpful to treat current_os == "linux" as "you're building for development and want to enable every option under the sun". I've been thinking about adding some option like matter_enable_recommended that can be defaulted to true and then be used as the default choice for other options, so that in scenarios where I want to explicitly selected what gets turned on I can just set that one option to false, without having to see what other RPC or tracing or feature xyz options have been added recently and are now defaulting to True because I happen to build on Linux.)

}

if (chip_device_platform == "auto") {
Expand Down
Loading