[WiFiPAF] Add support for NAN-USD commands over hostapd/wpa_supplicant control interface #41522
Conversation
…t control interface This change enables a Commissioner to leverage the command-based control interface of hostapd for WiFi-PAF commissioning, as an alternative to the D-Bus interface, which is exclusive to wpa_supplicant. With this change, a Linux-based access point should be able to act as Commissioner or Commissioner Proxy with NAN-USD capabilities. OpenWRT is an example of a Linux distribution which could now take advantage of hostapd support for WiFi-PAF commissioning, since it doesn't normally run wpa_supplicant. * In order to accomodate for the two different implementations (D-Bus and control interface), a generic WiFiPAFDriver class was created, for providing a single interface to low-level WiFi-PAF methods to ConnectivityManagerImpl. * The D-Bus WiFi-PAF implementation by NXP was moved to the WiFiDriverDbus derived class. * A WiFiPAFDriverCtrlIface class implements WiFiPAFDriver using wpa_ctrl_* functions from hostap repository, which was added as a submodule. It's the same command interface used by the hostapd_cli application. See https://w1.fi/wpa_supplicant/devel/wpa__ctrl_8h.html for reference. * WiFiPAFDriver has private methods for logic that is shared by both WiFiPAFDbus and WiFiPAFCtrlIface. * Two build arguments have been added: ** chip_device_config_enable_wifipaf_ctrl_iface: enables the build of WiFiDriverCtrlIface instead of WiFiDriverDbus. Defaults to "false". ** chip_device_config_enable_wifipaf_hostapd: used for disabling Wi-Fi management by ConnectivityManagerImpl. Must be disabled when hostapd is managing Wi-Fi interfaces. Defaults to "false". * WiFiPAFDriverCtrlIface is compatible with both wpa_supplicant and hostapd: ** Enable only chip_device_config_enable_wifipaf_ctrl_iface if running wpa_supplicant. ** Enable both chip_device_config_enable_wifipaf_ctrl_iface and chip_device_config_enable_wifipaf_hostapd if running hostapd.
|
|
![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 introduces a significant and well-structured refactoring to support WiFi-PAF commissioning over the hostapd/wpa_supplicant control interface, in addition to the existing D-Bus interface. The abstraction of WiFi-PAF operations into a WiFiPAFDriver base class with two separate implementations is a clean approach. My review has identified a critical issue in the shutdown logic of the new control interface driver that could lead to resource leaks and incorrect behavior, a high-severity issue regarding data-endianness that could cause interoperability problems, and a medium-severity suggestion to improve performance in data serialization. Overall, this is a valuable addition that extends WiFi-PAF support to more platforms.
| wpa_ctrl_close(ctrl); | ||
| wpa_ctrl_close(cmd_ctrl); |
There was a problem hiding this comment.
The wpa_ctrl handles ctrl and cmd_ctrl are closed here, but they are shared for the lifetime of the WiFiPAFDriverCtrlIface instance. Closing them in Shutdown will cause subsequent operations to fail. These handles should be closed in the destructor of WiFiPAFDriverCtrlIface instead. Please also add a destructor to WiFiPAFDriverCtrlIface in WiFiPAFDriverCtrlIface.h to handle this, for example:
~WiFiPAFDriverCtrlIface() override
{
if (ctrl) wpa_ctrl_close(ctrl);
if (cmd_ctrl) wpa_ctrl_close(cmd_ctrl);
}
~WiFiPAFDriverCtrlIface() override
{
if (ctrl) wpa_ctrl_close(ctrl);
if (cmd_ctrl) wpa_ctrl_close(cmd_ctrl);
}| << std::setw(2) << static_cast<int>(PafPublish_ssi.DevOpCode) << std::setw(4) << htons(PafPublish_ssi.DevInfo) | ||
| << std::setw(4) << htons(PafPublish_ssi.ProductId) << std::setw(4) << htons(PafPublish_ssi.VendorId) << " freq_list="; |
There was a problem hiding this comment.
The use of htons() here to convert uint16_t fields to network byte order before creating the hex string for the SSI (Service Specific Information) is inconsistent with the D-Bus implementation (WiFiPAFDriverDbus.cpp), which directly uses the struct's memory layout. On little-endian systems, this will result in different byte orders for the SSI between the two implementations, leading to interoperability issues. Please ensure the byte order is consistent across both implementations. If network byte order (big-endian) is required by the wpa_supplicant/hostapd control interface, then the D-Bus implementation should also be updated to ensure data is sent in network byte order.
| std::ostringstream oss_ssi; | ||
| for (size_t i = 0; i < msgBuf->DataLength(); ++i) | ||
| { | ||
| oss_ssi << std::hex << std::setw(2) << std::setfill('0') << static_cast<int>(msgBuf->Start()[i]); | ||
| } | ||
| auto ssi_str = oss_ssi.str(); |
There was a problem hiding this comment.
This loop for converting the message buffer to a hex string using std::ostringstream is inefficient. The Matter codebase provides a more performant utility chip::Encoding::BytesToHex for this purpose. Using it would improve performance and maintain consistency with the rest of the codebase.
For example, you could replace these lines with:
std::vector ssi_hex_buffer(msgBuf->DataLength() * 2 + 1);
ReturnErrorOnFailure(chip::Encoding::BytesToHex(msgBuf->Start(), msgBuf->DataLength(), ssi_hex_buffer.data(), ssi_hex_buffer.size()));
auto ssi_str = std::string(ssi_hex_buffer.data());
std::vector<char> ssi_hex_buffer(msgBuf->DataLength() * 2 + 1);
ReturnErrorOnFailure(chip::Encoding::BytesToHex(msgBuf->Start(), msgBuf->DataLength(), ssi_hex_buffer.data(), ssi_hex_buffer.size()));
auto ssi_str = std::string(ssi_hex_buffer.data());
.gitmodules
Outdated
| recursive = true | ||
| [submodule "third_party/hostap/repo"] | ||
| path = third_party/hostap/repo | ||
| url = https://w1.fi/hostap.git |
There was a problem hiding this comment.
This seems to give a 404 to me.
This should probably have a platforms guard to linux.
There was a problem hiding this comment.
Also wondering who w1.fi actually is ... why is this not some github project ? https://who.is/whois/w1.fi does not make it very clear....
There was a problem hiding this comment.
Probably this has to be added by a git submodule add as well as our CI seems to complain that checkout does not work
There was a problem hiding this comment.
Thank you for your comments @andy31415 , indeed I had not committed the actual submodule. About the repo URL, it is valid for git clone, but not suitable for loading with a web browser.
src/platform/Linux/BUILD.gn
Outdated
| "SystemPlatformConfig.h", | ||
| "SystemTimeSupport.cpp", | ||
| "WiFiPAFDriver.h", | ||
| "WiFiPAFDriver.cpp", |
There was a problem hiding this comment.
Should this be conditional on some "enable WiFi PAF" setting instead? Do we expect every linux distribution to support this?
There was a problem hiding this comment.
I agree, this should be guarded somehow. Will fix it.
src/platform/Linux/BUILD.gn
Outdated
| } | ||
|
|
||
| include_dirs = [ | ||
| "${chip_root}/third_party/hostap/repo/src/common", |
There was a problem hiding this comment.
This should be done via dependencies: if you depend on hostap, this include should be added in.
src/platform/Linux/BUILD.gn
Outdated
| } | ||
| } | ||
|
|
||
| source_set("wpa_ctrl") { |
There was a problem hiding this comment.
this should be inside the hostap BUILD.gn, so that I can depend on hostap and not have a separate source set here.
|
PR #41522: Size comparison from c87ece5 to f2a6a96 Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
PR #41522: Size comparison from 63df482 to 4e7aebf Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
5 participants
Summary
This change enables a Commissioner to leverage the command-based control interface of hostapd for WiFi-PAF commissioning, as an alternative to the D-Bus interface, which is exclusive to wpa_supplicant. With this change, a Linux-based access point should be able to act as Commissioner or Commissioner Proxy with NAN-USD capabilities. OpenWRT is an example of a Linux distribution which could now take advantage of hostapd support for WiFi-PAF commissioning, since it doesn't normally run wpa_supplicant.
In order to accomodate for the two different implementations (D-Bus and control interface), a generic WiFiPAFDriver class was created, for providing a single interface to low-level WiFi-PAF methods to ConnectivityManagerImpl.
The D-Bus WiFi-PAF implementation by NXP was moved to the WiFiDriverDbus derived class.
A WiFiPAFDriverCtrlIface class implements WiFiPAFDriver using wpa_ctrl_* functions from hostap repository, which was added as a submodule. It's the same command interface used by the hostapd_cli application. See https://w1.fi/wpa_supplicant/devel/wpa__ctrl_8h.html for reference.
WiFiPAFDriver has private methods for logic that is shared by both WiFiPAFDbus and WiFiPAFCtrlIface.
Two build arguments have been added:
of WiFiDriverDbus. Defaults to "false".
ConnectivityManagerImpl. Must be disabled when hostapd is managing Wi-Fi interfaces.
Defaults to "false".
WiFiPAFDriverCtrlIface is compatible with both wpa_supplicant and hostapd:
Enable only chip_device_config_enable_wifipaf_ctrl_iface if running wpa_supplicant.
Enable both chip_device_config_enable_wifipaf_ctrl_iface and
chip_device_config_enable_wifipaf_hostapd if running hostapd.
Testing
Environment:
AP: 8devices Habanero DVK running OpenWRT v24.10.0
Device: RPi 4b running Ubuntu 24.04 + Atheros AR9271 USB adapter
Steps:
AP:
$./chip-tool pairing wifipaf-wifi 1 TestSSID test-passphrase 20202021 3840 --freq 2412
Device:
$ sudo ./chip-all-clusters-app --wifi --wifipaf freq_list=2412
Expected result: the Raspberry Pi device should be successfully commissioned by the chip-tool commissioner running on the Access Point.