Wifi management via NetworkManager over dBus #281
Conversation
…ple take the interface as a cmdline param
…ary and zbus proc-macros generate slow-to-compile code
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @ivmarkov, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly upgrades the network capabilities of the rs-matter project by integrating real Wifi management for Linux systems. It provides flexible options for interacting with common network daemons like NetworkManager and wpa_supplicant, enhancing the example applications to demonstrate full Wifi connectivity and diagnostics. The changes involve new D-Bus integration modules, refactored network control logic, and improved IP stack management.
Highlights
- Real Wifi Management Integration: This PR replaces the previous 'fake' network communication cluster with actual Wifi management capabilities for Linux-based Matter devices. It introduces support for both NetworkManager and wpa_supplicant D-Bus services, enabling real-world Wifi scanning, connection, and diagnostics.
- Configurable Example Application: The
onoff_light_btexample has been updated to allow users to choose their preferred Wifi management backend (NetworkManager or wpa_supplicant) and specify the network interface via command-line arguments, making the example more flexible and practical. - NetworkManager D-Bus Implementation: A new
NetMgrCtlcomponent has been added, providing a comprehensive interface to interact with NetworkManager over D-Bus. This includes functionality for discovering available networks, connecting to them, and retrieving detailed Wifi diagnostic information. - Enhanced wpa_supplicant Support: The existing
WpaSuppCtlhas been significantly refactored and improved. It now features more robust Wifi scanning with retries, better handling of security types, and a clear separation of concerns for IP stack management. - Abstracted IP Stack Control: A new
IpStackCtltrait and itsDhClientCtlimplementation have been introduced. This allows for flexible management of the IP layer (e.g., DHCP, IPv6 link-local addressing) independently, particularly crucial when using wpa_supplicant which doesn't handle IP configuration directly. - D-Bus Proxy Updates: Numerous updates have been made to the
zbusproxy definitions for both NetworkManager and wpa_supplicant. These changes ensure accurate D-Bus interface and property mappings, including explicit property names anddefault_servicedeclarations for improved reliability.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The code changes introduce Wifi management via NetworkManager over dBus. The changes are well-structured, but there are a few areas for improvement, including a critical issue regarding the use of block_on in Drop implementations, some code duplication that could be refactored, and a potential bug in the RSSI value conversion.
| #[derive(Debug, Eq, PartialEq, Clone, Hash)] | ||
| struct WifiConnInfo { | ||
| connected: bool, | ||
| security: WiFiSecurityBitmap, | ||
| ssid: Vec<u8>, | ||
| bssid: Vec<u8>, | ||
| band: WiFiBandEnum, | ||
| channel: u16, | ||
| rssi: i8, | ||
| } | ||
|
|
||
| impl WifiConnInfo { | ||
| /// Create a new `WifiConnInfo` from the given `NetworkScanInfo::Wifi`. | ||
| fn new(scan_info: &NetworkScanInfo, connected: bool) -> Self { | ||
| let NetworkScanInfo::Wifi { | ||
| security, | ||
| ssid, | ||
| bssid, | ||
| channel, | ||
| band, | ||
| rssi, | ||
| } = scan_info | ||
| else { | ||
| // Not possible, because `WpaSupp` only produces scan info of type `Wifi` | ||
| unreachable!(); | ||
| }; | ||
|
|
||
| Self { | ||
| connected, | ||
| security: *security, | ||
| ssid: ssid.to_vec(), | ||
| bssid: bssid.to_vec(), | ||
| band: *band, | ||
| channel: *channel, | ||
| rssi: *rssi, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // TODO: Unify with the same method in `wpa_supp` | ||
| // See https://github.com/project-chip/connectedhomeip/blob/cd5fec9ba9be0c39f3c11f67d57b18b6bb2b4289/src/platform/Linux/ConnectivityManagerImpl.cpp#L1937 | ||
| fn band_and_channel(freq: u32) -> (WiFiBandEnum, u16) { | ||
| let mut band = WiFiBandEnum::V2G4; | ||
|
|
||
| let channel = if freq <= 931 { | ||
| if freq >= 916 { | ||
| ((freq - 916) * 2) - 1 | ||
| } else if freq >= 902 { | ||
| (freq - 902) * 2 | ||
| } else if freq >= 863 { | ||
| (freq - 863) * 2 | ||
| } else { | ||
| 1 | ||
| } | ||
| } else if freq <= 2472 { | ||
| (freq - 2412) / 5 + 1 | ||
| } else if freq == 2484 { | ||
| 14 | ||
| } else if (3600..=3700).contains(&freq) { | ||
| // Note: There are not many devices supports this band, and this band contains rational frequency in MHz, need to figure out | ||
| // the behavior of wpa_supplicant in this case. | ||
| band = WiFiBandEnum::V3G65; | ||
| 0 | ||
| } else if (5035..=5945).contains(&freq) || freq == 5960 || freq == 5980 { | ||
| band = WiFiBandEnum::V5G; | ||
| (freq - 5000) / 5 | ||
| } else if freq >= 5955 { | ||
| band = WiFiBandEnum::V6G; | ||
| (freq - 5950) / 5 | ||
| } else if freq >= 58000 { | ||
| band = WiFiBandEnum::V60G; | ||
|
|
||
| // Note: Some channel has the same center frequency but different bandwidth. Should figure out wpa_supplicant's behavior in | ||
| // this case. Also, wpa_supplicant's frequency property is uint16 infact. | ||
| match freq { | ||
| 58_320 => 1, | ||
| 60_480 => 2, | ||
| 62_640 => 3, | ||
| 64_800 => 4, | ||
| 66_960 => 5, | ||
| 69_120 => 6, | ||
| 59_400 => 9, | ||
| 61_560 => 10, | ||
| 63_720 => 11, | ||
| 65_880 => 12, | ||
| 68_040 => 13, | ||
| _ => 0, | ||
| } | ||
| } else { | ||
| // Unknown channel | ||
| 0 | ||
| }; | ||
|
|
||
| (band, channel as u16) | ||
| } |
There was a problem hiding this comment.
The WifiConnInfo struct and the band_and_channel function are duplicated in both rs-matter/src/transport/network/wifi/nm.rs and rs-matter/src/transport/network/wifi/wpa_supp.rs.
This duplication increases the maintenance burden and the risk of inconsistencies. These shared components should be extracted into a common module, for example, rs-matter/src/transport/network/wifi/util.rs, and then imported by both nm.rs and wpa_supp.rs.
The TODO on line 519 already acknowledges this for band_and_channel.
| rssi: bss_info | ||
| .strength() | ||
| .await? | ||
| .min(i8::MIN as _) | ||
| .max(i8::MAX as _) as i8, |
There was a problem hiding this comment.
The strength property from NetworkManager is a u8 representing signal strength as a percentage (0-100). The code currently casts this value directly to an i8 to be used as the rssi.
This is incorrect, as RSSI in the Matter specification (and generally in Wi-Fi) is expected to be in dBm, which is typically a negative value (e.g., -30 to -90). A value of 127 is reserved to indicate an "unknown" RSSI. This direct cast will result in incorrect and misleading RSSI values being reported by the device.
The value should be converted from a percentage to a dBm value. While an exact formula is not always available, an approximation could be used, or if NetworkManager cannot provide a dBm value, it might be better to report the RSSI as unknown (127).
| let wpa_supp = NetCtlWithStatusImpl::new( | ||
| &net_ctl_state, | ||
| WpaSuppCtl::new(&connection, &if_name, DhClientCtl::new(&if_name, true)), | ||
| ); | ||
| let nm = NetCtlWithStatusImpl::new(&net_ctl_state, NetMgrCtl::new(&connection, &if_name)); | ||
|
|
||
| // Assemble our Data Model handler by composing the predefined Root Endpoint handler with the On/Off handler | ||
| let dm_handler = dm_handler(&matter, &on_off, &net_ctl, &networks); | ||
| let wpa_supp_dm_handler = dm_handler(&matter, &on_off, &wpa_supp, &networks); | ||
| let nm_dm_handler = dm_handler(&matter, &on_off, &nm, &networks); | ||
|
|
||
| // Create a default responder capable of handling up to 3 subscriptions | ||
| // All other subscription requests will be turned down with "resource exhausted" | ||
| let responder = DefaultResponder::new(&matter, &buffers, &subscriptions, dm_handler); | ||
| let wpa_supp_responder = | ||
| DefaultResponder::new(&matter, &buffers, &subscriptions, wpa_supp_dm_handler); | ||
| let nm_responder = DefaultResponder::new(&matter, &buffers, &subscriptions, nm_dm_handler); | ||
|
|
||
| // Run the responder with up to 4 handlers (i.e. 4 exchanges can be handled simultaneously) | ||
| // Clients trying to open more exchanges than the ones currently running will get "I'm busy, please try again later" | ||
| let mut respond = pin!(responder.run::<4, 4>()); | ||
| let mut respond = pin!(async { | ||
| if use_wpa_supp { | ||
| wpa_supp_responder.run::<4, 4>().await | ||
| } else { | ||
| nm_responder.run::<4, 4>().await | ||
| } | ||
| }); |
There was a problem hiding this comment.
There is some code duplication in setting up the dm_handler and responder for the wpa_supplicant and NetworkManager paths. While the async block cleverly handles the different future types, the setup code for both paths is almost identical.
To make the code more DRY, you could move the object construction and setup logic inside the if/else branches within the async block. This would avoid creating unused objects and make the logic clearer.
let mut respond = pin!(async {
if use_wpa_supp {
let wpa_supp = NetCtlWithStatusImpl::new(
&net_ctl_state,
WpaSuppCtl::new(&connection, &if_name, DhClientCtl::new(&if_name, true)),
);
let dm_handler = dm_handler(&matter, &on_off, &wpa_supp, &networks);
let responder =
DefaultResponder::new(&matter, &buffers, &subscriptions, dm_handler);
responder.run::<4, 4>().await
} else {
let nm =
NetCtlWithStatusImpl::new(&net_ctl_state, NetMgrCtl::new(&connection, &if_name));
let dm_handler = dm_handler(&matter, &on_off, &nm, &networks);
let responder =
DefaultResponder::new(&matter, &buffers, &subscriptions, dm_handler);
responder.run::<4, 4>().await
}
});
NOTE: #279 needs to be merged first as this PR is on top of it.
This PR is very similar in spirit to #279 in that it brings Wifi connectivity management for (embedded) Linux, however based on the NetworkManager daemon, over dBus and using the (pure Rust) zbus NM bindings we have in the meantime.
There are two topics which are not addressed by this PR and are left to subsequent PRs:
NetifDiagrs-matter trait)We do have a "standard"
NetifDiagimpl already which works on any Unix OS -UnixNetifs.However - and when using NetworkManager specifically - we can do better in that we don't need
UnixNetifsbecause NetworkManager itself provides a dBus API for enumerating the details of all Linux/Unix network interfaces available in the OS.It is not like urgent to implement this, but why not?
Unlike
wpa_supplicantwhich does not have any persistence (that is, aside from/etcconf files), NetworkManager does have a built-in Connection Persistence API, available over dBus.The above is a bit at odds with the built-in rs-matter Wifi/Thread connections' persistence we provide in the form of the
WirelessNetworksstruct, which implements thenet_comm::Networkstrait pfrs-matter.Given that NetworkManager has its own persistence, we have three choices how to deal with it:
Continue using
WirelessNetworksand thus rely on the built-in rs-matter wireless networks persistence. Whatever Wifi connection is created in NetworkManager - it is considered transient and is removed when the dBus connection to NetworkManager is dropped (actually, when theNetCtltrait impl is dropped) => this is what this PR implements currentlyDon't use the built-in rs-matter
WirelessNetworksimpl, and implement a new wireless networks persistence, which relies squarely on the persistence mechanism of NetworkManager. This has pros and cons, in that wireless networks need to only be persisted once commissioning is over, while NetworkManager seems not to allow using a Wifi connection which is not persisted inside it yet, so we have to see how to implement this exactly, and whether this is a good ideaUse both the NetworkManager peristence and the built-in
WirelessNetworksstruct and keep those in sync. Seems like the worst of both worlds, I'm mentioning just for completeness