[linux] Add WiFi support in ConnectivityManager. #1975
Conversation
yufengwangca
requested review from
andy31415,
anush-apple,
BroderickCarlin,
chrisdecenzo,
hawk248,
jelderton,
robszewczyk,
saurabhst and
woody-apple
as code owners
* Add WiFi support to ConnectivityManager to manage wpa_supplicant using GDBus. * Restyled by whitespace * Restyled by clang-format
This is used to fix the following compile issue from CI. (Did not see this error on local build) 2020-08-05T03:31:18.9931835Z In file included from ../../../../src/include/platform/ConnectivityManager.h:263, The root cause is those header files need to be exist during compile phase not linking phase, that means any file which include header file <platform/CHIPDeviceLayer.h> need to see those DBus header files available before compile. In theory, we should allow any source files within CHIP stack to include CHIPDeviceLayer.h, how to guarantee those DBus header files are pre-generated before any cpp/hpp file is processed since they are also generated during make phase? For different codegen version, it should generate the same API prototypes, we need to update those files only when we update the corresponding xml file. Unless we can make those DBus header files get generated in config phase instead of make phase, otherwise, checking in those auto-generated header files are the safest solution. I would rather use this simple solution to prevent all flaky build sequence errors. |
| mWpaSupplicant.state = GDBusWpaSupplicant::WPA_NOT_CONNECTED; | ||
| } | ||
|
|
||
| if (err != NULL) |
There was a problem hiding this comment.
Don't need to check for NULL here as g_error_free() already does.
There was a problem hiding this comment.
I prefer always check NULL before calling all g_error_free() even it handles NULL case, this pattern is used throughout gio source code. Suppose there are 0.001 chance of error, then we can prevent 99.99% unnecessary function call.
| } | ||
| else | ||
| { | ||
| if (err != NULL) |
There was a problem hiding this comment.
Suggest removing this branch.
Can it really happen that |err| is null here?
If it can't happen,
assert(err);
If it can happen,
ChipLogProgress(DeviceLayer, "wpa_supplicant: failed to create wpa_supplicant1 interface proxy %s: %s",
mWpaSupplicant.interfacePath, err ? err->message : "unknown error");
There was a problem hiding this comment.
Ack, in theory, err should never be NULL if iface is NULL, but lets not assume that.
| { | ||
| mWpaSupplicant.iface = iface; | ||
| mWpaSupplicant.state = GDBusWpaSupplicant::WPA_INTERFACE_CONNECTED; | ||
| ChipLogProgress(DeviceLayer, "wpa_supplicant: connected to wpa_supplicant interface proxy: %p", iface); |
| } | ||
| else | ||
| { | ||
| if (err != NULL) |
There was a problem hiding this comment.
Suggest removing this branch (see above)
| } | ||
| } | ||
|
|
||
| if (err != NULL) |
There was a problem hiding this comment.
see comments above
| mWpaSupplicant.state = GDBusWpaSupplicant::WPA_GOT_INTERFACE_PATH; | ||
| ChipLogProgress(DeviceLayer, "wpa_supplicant: WiFi interface added: %s", mWpaSupplicant.interfacePath); | ||
|
|
||
| wpa_fi_w1_wpa_supplicant1_interface_proxy_new_for_bus(G_BUS_TYPE_SYSTEM, G_DBUS_PROXY_FLAGS_NONE, "fi.w1.wpa_supplicant1", |
There was a problem hiding this comment.
Can we move the interface string to a constant?
namespace {
const char kWpaSupplicantInterface[] = "fi.w1.wpa_supplicant1";
}
| } | ||
| else | ||
| { | ||
| if (err != NULL) |
There was a problem hiding this comment.
Suggest removing this branch.
There was a problem hiding this comment.
See comment above
| mWpaSupplicant.state = GDBusWpaSupplicant::WPA_NOT_CONNECTED; | ||
| } | ||
|
|
||
| if (err != NULL) |
There was a problem hiding this comment.
See comment above
We're already generating CHIPVersion.h, maybe take a look at that? It shouldn't be flaky. |
Yes, CHIPVersion.h is generated during config phase instead of make phase |
|
After piloting, I found I am still not able to use the way to pre-generate CHIPVersion.h to generate GDBus header files. We use the following syntax to generate CHIPVersion.h during config phase. include_HEADERS = .PHONY: force That means we force generate CHIPVersion.h for all targets. But we should generate GDBus headers file only if Linux device layer is selected and GIO package are installed & enabled on the host platform. This check is also done during config phase and the result is stored to variable CONFIG_WIFI_PLATFORM_WPA. From the config log, I found the header file generating process is triggered before variable CONFIG_WIFI_PLATFORM_WPA gets assigned. So the GDBus header files are never get generated with the following syntax. if CONFIG_WIFI_PLATFORM_WPA If I remove condition check "if CONFIG_WIFI_PLATFORM_WPA", then GDBus header files are pre-generated as expected, but I believe this behavior is not what we want since we mandate all target platforms handle those GDBus stuff. It looks like checking in those auto-generate header files is still the only viable solution right now. |
Ok, thanks for that info. It seems like we are running into either a limitation or a lack of expertise with the automake system. |
|
Size increase report for "nrf-example-build"
Full report output |
|
Size increase report for "nrfconnect-example-build"
Full report output |
|
Size increase report for "linux-example-build"
Full report output |
|
Size increase report for "esp32-example-build"
Full report output |
|
Size increase report for "gn_nrf-example-build"
Full report output |
|
Size increase report for "gn_linux-example-build"
Full report output |
Merge in WMN_TOOLS/matter from feature/slc_cli_updates to silabs_slc_1.3 Squashed commit of the following: commit 49afede4bacfe0d0cb37026056e7028e3353d795 Author: Sarthak Shaha <[email protected]> Date: Thu Jun 13 23:52:40 2024 +0530 updated zap to latest
Problem
The Linux device layer needs a ConnectivityMgr that handles a single, primary WiFi interface.
Summary of Changes