Wifi management via wpa_supplicant over dBus #279
Conversation
![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 enhances the rs-matter project's capability to manage Wifi networks on Linux systems, particularly for embedded environments. It transitions the example applications from a simulated network control to a fully functional integration with the wpa_supplicant D-Bus service, enabling real Matter BLE commissioning over Wifi. The changes involve substantial refactoring of network control logic, introduction of a dedicated IP stack management layer, and improvements to D-Bus communication proxies.
Highlights
- Real Wifi Management: The
onoff_light_btexample now leverageswpa_supplicantover D-Bus anddhclientto provide actual Wifi network management capabilities, moving beyond a previously 'fake' network controller implementation. wpa_supplicantIntegration: A newWpaSuppCtlmodule has been introduced to facilitate D-Bus communication with thewpa_supplicantdaemon, enabling network scanning, connection, and robust state management for Wifi networks.- IP Stack Control Abstraction: An
IpStackCtltrait was defined to abstract IP layer management, with a concrete Unix-specificDhClientCtlimplementation added. This allows the system to configure network interfaces (e.g., via DHCP and SLAAC) using standard Linux commands likedhclientandip. - Enhanced Wifi Diagnostics: The
WpaSuppCtlnow implements theWifiDiagtrait, providing real-time diagnostic information such as BSSID, RSSI, channel number, and band for the connected Wifi network. - D-Bus Proxy Refinements: Existing
zbusproxy definitions forwpa_supplicantinterfaces and properties have been corrected and refined, ensuring more accurate and reliable D-Bus communication.
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
This pull request is a significant step towards real-world Wifi management on Linux by integrating with wpa_supplicant over D-Bus. The changes are extensive, touching example code, network state management, and the core wpa_supplicant controller implementation. The introduction of DhClientCtl for IP stack management is a practical approach for embedded systems.
Overall, the implementation is comprehensive, but I've identified several critical issues related to potential panics, an infinite loop, and incorrect state handling that should be addressed before merging. I've also included suggestions to improve robustness and maintainability.
Here is a summary of my feedback:
- Critical Issues: There are several places where
.unwrap()is used on operations that can fail (e.g., UTF-8 conversion of non-guaranteed UTF-8 byte slices like SSIDs/passphrases), which will lead to panics. There's also a potential infinite loop when waiting for a D-Bus signal and a logic bug in state updates. - High-Severity Issues: The
dropimplementation performs blocking async operations, which is unsafe and can lead to deadlocks. Additionally, there are missing checks for command exit statuses and slice lengths that could cause panics or unexpected behavior. - Maintainability: I've suggested improvements for error handling and code readability in a few places to make the code more robust and easier to understand.
Please review the detailed comments on the specific lines of code. Addressing these points will greatly improve the quality and stability of this new functionality.
|
Adding myself so it shows up on my github dashboard, feel free to nudge me when you're ready |
…ple take the interface as a cmdline param
|
@gmarcosb Since you wanted to take a look - this is now ready for review. |
…ary and zbus proc-macros generate slow-to-compile code
|
@gmarcosb Do you still plan to look into this? |
| @@ -378,3 +570,59 @@ impl From<zbus::Error> for NetCtlError { | |||
| NetCtlError::Other(value.into()) | |||
| } | |||
| } | |||
|
|
|||
| // See https://github.com/project-chip/connectedhomeip/blob/cd5fec9ba9be0c39f3c11f67d57b18b6bb2b4289/src/platform/Linux/ConnectivityManagerImpl.cpp#L1937 | |||
| fn band_and_channel(freq: u32) -> (WiFiBandEnum, u16) { | |||
There was a problem hiding this comment.
Can we get some uint tests [run in CI] for some of the things that we can test w/o needing a network connection, like this guy & ipv6.rs?
Understood some things we won't be able to test because of the needed backends, although if we could test with some fakes it would be ideal...
Matter C++ code is not-so-great at tests, would be nice to make sure the rust stuff has at least some basic coverage
(TBD: Not yet marked ready for review as I need to test the latest round of changes.)This PR is completing the not-really-working-for-typechecking-only
wpa_suppmodule that was part of the original zbus-enablement PR #271With it, we are now able to talk to the
wpa_supplicantLinux daemon over dBus, and thus do a Matter BLE commissioning on Linux, which is managing the system's Wifi networks for real.Note that while having support for driving
wpa_supplicantdirectly is good to have inrs-matter(Matter C++ SDK has it as well and that's their primary way of driving Wifi networks for Linux), I expect that the upcoming NetworkManager-over-dBus support to be the solution for Wifi networks management on Linux, including for (most) embedded use cases except the tiniest ones where NM is not installed.