From 0a55c8c06d22aa6faa8a9228d5e17cf41cbbe5a6 Mon Sep 17 00:00:00 2001 From: Akrm Al-Hakimi Date: Fri, 12 Dec 2025 16:47:10 -0500 Subject: [PATCH 1/6] fix(#124): return error and halt operation on `forget()` --- nmrs/src/connection.rs | 36 +++--------------------------------- nmrs/src/constants.rs | 9 --------- 2 files changed, 3 insertions(+), 42 deletions(-) diff --git a/nmrs/src/connection.rs b/nmrs/src/connection.rs index b0c894b3..02c1f2f6 100644 --- a/nmrs/src/connection.rs +++ b/nmrs/src/connection.rs @@ -128,37 +128,7 @@ pub(crate) async fn forget(conn: &Connection, ssid: &str) -> Result<()> { && decode_ssid_or_empty(&bytes) == ssid { debug!("Disconnecting from active network: {ssid}"); - let dev_proxy: zbus::Proxy<'_> = zbus::proxy::Builder::new(conn) - .destination("org.freedesktop.NetworkManager")? - .path(dev_path.clone())? - .interface("org.freedesktop.NetworkManager.Device")? - .build() - .await?; - - match dev_proxy.call_method("Disconnect", &()).await { - Ok(_) => debug!("Disconnect call succeeded"), - Err(e) => warn!("Disconnect call failed: {e}"), - } - - debug!("About to enter wait loop..."); - for i in 0..retries::FORGET_MAX_RETRIES { - Delay::new(timeouts::forget_poll_interval()).await; - match dev.state().await { - Ok(current_state) => { - debug!("Wait loop {i}: device state = {current_state}"); - if current_state == device_state::DISCONNECTED - || current_state == device_state::UNAVAILABLE - { - debug!("Device reached disconnected state"); - break; - } - } - Err(e) => { - warn!("Failed to get device state in wait loop {i}: {e}"); - break; - } - } - } + disconnect_wifi_and_wait(conn, dev_path).await?; debug!("Wait loop completed"); } } @@ -251,7 +221,7 @@ pub(crate) async fn forget(conn: &Connection, ssid: &str) -> Result<()> { /// /// Calls the Disconnect method on the device and polls until the device /// state becomes Disconnected or Unavailable, or the retry limit is reached. -pub(crate) async fn disconnect_wifi_device( +pub(crate) async fn disconnect_wifi_and_wait( conn: &Connection, dev_path: &OwnedObjectPath, ) -> Result<()> { @@ -355,7 +325,7 @@ async fn ensure_disconnected( } } - disconnect_wifi_device(conn, wifi_device).await?; + disconnect_wifi_and_wait(conn, wifi_device).await?; } Ok(()) diff --git a/nmrs/src/constants.rs b/nmrs/src/constants.rs index 15992e78..7c541419 100644 --- a/nmrs/src/constants.rs +++ b/nmrs/src/constants.rs @@ -13,13 +13,10 @@ pub mod device_type { /// NetworkManager device state constants pub mod device_state { - // pub const UNMANAGED: u32 = 10; pub const UNAVAILABLE: u32 = 20; pub const DISCONNECTED: u32 = 30; - // pub const PREPARE: u32 = 40; pub const CONFIG: u32 = 50; pub const ACTIVATED: u32 = 100; - // pub const DEACTIVATING: u32 = 110; pub const FAILED: u32 = 120; } @@ -45,7 +42,6 @@ pub mod timeouts { pub const DISCONNECT_FINAL_DELAY_MS: u64 = 500; pub const CONNECTION_POLL_INTERVAL_MS: u64 = 500; pub const SCAN_WAIT_SECONDS: u64 = 3; - pub const FORGET_POLL_INTERVAL_MS: u64 = 200; pub fn disconnect_poll_interval() -> Duration { Duration::from_millis(DISCONNECT_POLL_INTERVAL_MS) @@ -62,10 +58,6 @@ pub mod timeouts { pub fn scan_wait() -> Duration { Duration::from_secs(SCAN_WAIT_SECONDS) } - - pub fn forget_poll_interval() -> Duration { - Duration::from_millis(FORGET_POLL_INTERVAL_MS) - } } /// Retry count constants @@ -74,7 +66,6 @@ pub mod retries { pub const CONNECTION_MAX_RETRIES: u32 = 40; pub const CONNECTION_CONFIG_STUCK_THRESHOLD: u32 = 15; pub const CONNECTION_STUCK_CHECK_START: u32 = 10; - pub const FORGET_MAX_RETRIES: u32 = 20; pub const WIFI_READY_MAX_RETRIES: u32 = 20; } From 7526b2a90edca81bee3e70f16407c998dfd59226 Mon Sep 17 00:00:00 2001 From: Akrm Al-Hakimi Date: Fri, 12 Dec 2025 17:37:24 -0500 Subject: [PATCH 2/6] fix(#123): preserve secured flag across all APs --- nmrs/src/models.rs | 14 ++++++++++++++ nmrs/src/scan.rs | 10 +--------- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/nmrs/src/models.rs b/nmrs/src/models.rs index 3ce203a0..1f88eeb8 100644 --- a/nmrs/src/models.rs +++ b/nmrs/src/models.rs @@ -425,6 +425,20 @@ impl WifiSecurity { } } +impl Network { + pub fn merge_ap(&mut self, other: &Network) { + if other.strength.unwrap_or(0) > self.strength.unwrap_or(0) { + self.strength = other.strength; + self.frequency = other.frequency; + self.bssid = other.bssid.clone(); + } + + self.secured |= other.secured; + self.is_psk |= other.is_psk; + self.is_eap |= other.is_eap; + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/nmrs/src/scan.rs b/nmrs/src/scan.rs index 7d8d33c4..409fe91e 100644 --- a/nmrs/src/scan.rs +++ b/nmrs/src/scan.rs @@ -86,17 +86,9 @@ pub(crate) async fn list_networks(conn: &Connection) -> Result> { // Deduplicate: use (SSID, frequency) as key, keep strongest signal for (ssid, frequency, new_net) in all_networks { - let strength = new_net.strength.unwrap_or(0); networks .entry((ssid, frequency)) - .and_modify(|n| { - if strength > n.strength.unwrap_or(0) { - *n = new_net.clone(); - } - if new_net.secured { - n.secured = true; - } - }) + .and_modify(|n| n.merge_ap(&new_net)) .or_insert(new_net); } From 53ee964acb3b524b0394ec69d17d32e632a3c023 Mon Sep 17 00:00:00 2001 From: Akrm Al-Hakimi Date: Fri, 12 Dec 2025 18:01:34 -0500 Subject: [PATCH 3/6] refactor(#46): event-driven D-Bus signals instead of polling --- Cargo.lock | 38 ++- nmrs/Cargo.toml | 9 +- nmrs/src/connection.rs | 168 ++++++------- nmrs/src/constants.rs | 48 ++-- nmrs/src/device.rs | 53 +++-- nmrs/src/lib.rs | 16 +- nmrs/src/models.rs | 325 +++++++++++++++++++++++++- nmrs/src/proxies.rs | 175 -------------- nmrs/src/proxies/access_point.rs | 49 ++++ nmrs/src/proxies/active_connection.rs | 75 ++++++ nmrs/src/proxies/device.rs | 64 +++++ nmrs/src/proxies/main_nm.rs | 52 +++++ nmrs/src/proxies/mod.rs | 34 +++ nmrs/src/proxies/wireless.rs | 37 +++ nmrs/src/state_wait.rs | 285 +++++++++++++++++++--- 15 files changed, 1083 insertions(+), 345 deletions(-) delete mode 100644 nmrs/src/proxies.rs create mode 100644 nmrs/src/proxies/access_point.rs create mode 100644 nmrs/src/proxies/active_connection.rs create mode 100644 nmrs/src/proxies/device.rs create mode 100644 nmrs/src/proxies/main_nm.rs create mode 100644 nmrs/src/proxies/mod.rs create mode 100644 nmrs/src/proxies/wireless.rs diff --git a/Cargo.lock b/Cargo.lock index fde8b54c..1926e867 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -449,6 +449,21 @@ dependencies = [ "winapi", ] +[[package]] +name = "futures" +version = "0.3.31" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "65bc07b1a8bc7c85c5f2e110c476c7389b4554ba72af57d8445ea63a576b0876" +dependencies = [ + "futures-channel", + "futures-core", + "futures-executor", + "futures-io", + "futures-sink", + "futures-task", + "futures-util", +] + [[package]] name = "futures-channel" version = "0.3.31" @@ -456,6 +471,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2dff15bf788c671c1934e366d07e30c1814a8ef514e1af724a602e8a2fbe1b10" dependencies = [ "futures-core", + "futures-sink", ] [[package]] @@ -506,16 +522,16 @@ dependencies = [ ] [[package]] -name = "futures-task" +name = "futures-sink" version = "0.3.31" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f90f7dce0722e95104fcb095585910c0977252f286e354b5e3bd38902cd99988" +checksum = "e575fab7d1e0dcb8d0c7bcf9a63ee213816ab51902e6d244a95819acacf1d4f7" [[package]] -name = "futures-timer" -version = "3.0.3" +name = "futures-task" +version = "0.3.31" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f288b0a4f20f9a56b5d1da57e2227c661b7b16168e2f72365f57b63326e29b24" +checksum = "f90f7dce0722e95104fcb095585910c0977252f286e354b5e3bd38902cd99988" [[package]] name = "futures-util" @@ -523,9 +539,13 @@ version = "0.3.31" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9fa08315bb612088cc391249efdc3bc77536f16c91f6cf495e6fbe85b20a4a81" dependencies = [ + "futures-channel", "futures-core", + "futures-io", "futures-macro", + "futures-sink", "futures-task", + "memchr", "pin-project-lite", "pin-utils", "slab", @@ -933,7 +953,7 @@ dependencies = [ name = "nmrs" version = "0.4.0" dependencies = [ - "futures-timer", + "futures", "log", "serde", "thiserror", @@ -1479,13 +1499,13 @@ checksum = "06abde3611657adf66d383f00b093d7faecc7fa57071cce2578660c9f1010821" [[package]] name = "uuid" -version = "1.18.1" +version = "1.19.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2f87b8aa10b915a06587d0dec516c282ff295b475d94abf425d62b57710070a2" +checksum = "e2e054861b4bd027cd373e18e8d8d8e6548085000e41290d95ce0c373a654b4a" dependencies = [ "getrandom 0.3.3", "js-sys", - "serde", + "serde_core", "wasm-bindgen", ] diff --git a/nmrs/Cargo.toml b/nmrs/Cargo.toml index 5c5c5fb9..1c3d14c1 100644 --- a/nmrs/Cargo.toml +++ b/nmrs/Cargo.toml @@ -13,10 +13,11 @@ categories = ["api-bindings", "asynchronous"] log = "0.4.29" zbus = "5.11.0" zvariant = "5.7.0" -serde = { version = "1", features = ["derive"] } -thiserror = "2.0.16" -futures-timer = "3.0.3" -uuid = { version = "1.18.1", features = ["v4"] } +serde = { version = "1.0.228", features = ["derive"] } +thiserror = "2.0.17" +uuid = { version = "1.19.0", features = ["v4"] } +tokio = { version = "1.48.0", features = ["time", "sync"] } +futures = "0.3.31" [dev-dependencies] tokio = { version = "1.48.0", features = ["rt-multi-thread", "macros", "sync", "time"] } \ No newline at end of file diff --git a/nmrs/src/connection.rs b/nmrs/src/connection.rs index 02c1f2f6..ba441ef8 100644 --- a/nmrs/src/connection.rs +++ b/nmrs/src/connection.rs @@ -1,16 +1,16 @@ -use futures_timer::Delay; use log::{debug, error, info, warn}; use std::collections::HashMap; +use tokio::time::sleep; use zbus::Connection; use zvariant::OwnedObjectPath; use crate::Result; use crate::connection_settings::{delete_connection, get_saved_connection_path}; -use crate::constants::{device_state, device_type, retries, timeouts}; +use crate::constants::{device_state, device_type, timeouts}; use crate::models::{ConnectionError, ConnectionOptions, WifiSecurity}; use crate::network_info::current_ssid; use crate::proxies::{NMAccessPointProxy, NMDeviceProxy, NMProxy, NMWirelessProxy}; -use crate::state_wait::wait_for_connection_state; +use crate::state_wait::{wait_for_connection_activation, wait_for_device_disconnect}; use crate::utils::decode_ssid_or_empty; use crate::wifi_builders::build_wifi_connection; @@ -76,14 +76,10 @@ pub(crate) async fn connect(conn: &Connection, ssid: &str, creds: WifiSecurity) build_and_activate_new(conn, &nm, &wifi_device, &specific_object, ssid, creds).await?; } } - let dev_proxy = NMDeviceProxy::builder(conn) - .path(wifi_device.clone())? - .build() - .await?; - debug!("Waiting for connection to complete..."); - wait_for_connection_state(&dev_proxy).await?; - info!("Connection request for '{ssid}' submitted successfully"); + // Connection activation is now handled within connect_via_saved() and + // build_and_activate_new() using signal-based monitoring + info!("Successfully connected to '{ssid}'"); Ok(()) } @@ -219,8 +215,10 @@ pub(crate) async fn forget(conn: &Connection, ssid: &str) -> Result<()> { /// Disconnects a Wi-Fi device and waits for it to reach disconnected state. /// -/// Calls the Disconnect method on the device and polls until the device -/// state becomes Disconnected or Unavailable, or the retry limit is reached. +/// Calls the Disconnect method on the device and waits for the `StateChanged` +/// signal to indicate the device has reached Disconnected or Unavailable state. +/// This is more efficient than polling and responds immediately when the +/// device disconnects. pub(crate) async fn disconnect_wifi_and_wait( conn: &Connection, dev_path: &OwnedObjectPath, @@ -230,6 +228,13 @@ pub(crate) async fn disconnect_wifi_and_wait( .build() .await?; + // Check if already disconnected + let current_state = dev.state().await?; + if current_state == device_state::DISCONNECTED || current_state == device_state::UNAVAILABLE { + debug!("Device already disconnected"); + return Ok(()); + } + let raw: zbus::proxy::Proxy = zbus::proxy::Builder::new(conn) .destination("org.freedesktop.NetworkManager")? .path(dev_path.clone())? @@ -237,26 +242,16 @@ pub(crate) async fn disconnect_wifi_and_wait( .build() .await?; + debug!("Sending disconnect request"); let _ = raw.call_method("Disconnect", &()).await; - for _ in 0..retries::DISCONNECT_MAX_RETRIES { - Delay::new(timeouts::disconnect_poll_interval()).await; - match dev.state().await { - Ok(s) if s == device_state::DISCONNECTED || s == device_state::UNAVAILABLE => { - break; - } - Ok(_) => continue, - Err(e) => return Err(e.into()), - } - } + // Wait for disconnect using signal-based monitoring + wait_for_device_disconnect(&dev).await?; - Delay::new(timeouts::disconnect_final_delay()).await; + // Brief stabilization delay + sleep(timeouts::stabilization_delay()).await; - match dev.state().await { - Ok(s) if s == device_state::DISCONNECTED || s == device_state::UNAVAILABLE => Ok(()), - Ok(s) => Err(ConnectionError::Stuck(format!("{s}"))), - Err(e) => Err(e.into()), - } + Ok(()) } /// Finds the first Wi-Fi device on the system. @@ -333,10 +328,10 @@ async fn ensure_disconnected( /// Attempts to connect using a saved connection profile. /// -/// Activates the saved connection. If activation succeeds but the device -/// ends up in a disconnected state (indicating invalid saved settings), -/// deletes the saved connection and creates a fresh one with the provided -/// credentials. +/// Activates the saved connection and monitors the activation state using +/// D-Bus signals. If activation fails (device disconnects or enters failed +/// state), deletes the saved connection and creates a fresh one with the +/// provided credentials. /// /// This handles cases where saved passwords are outdated or corrupted. async fn connect_via_saved( @@ -359,38 +354,38 @@ async fn connect_via_saved( active_conn.as_str() ); - Delay::new(timeouts::disconnect_final_delay()).await; - - let dev_check = NMDeviceProxy::builder(conn) - .path(wifi_device.clone())? - .build() - .await?; - - let check_state = dev_check.state().await?; - - if check_state == device_state::DISCONNECTED { - warn!("Connection activated but device stuck in Disconnected state"); - warn!("Saved connection has invalid settings, deleting and retrying"); - - let _ = nm.deactivate_connection(active_conn).await; - - let _ = delete_connection(conn, saved.clone()).await; - - let opts = ConnectionOptions { - autoconnect: true, - autoconnect_priority: None, - autoconnect_retries: None, - }; - - let settings = build_wifi_connection(ap.as_str(), creds, &opts); - - debug!("Creating fresh connection with corrected settings"); - nm.add_and_activate_connection(settings, wifi_device.clone(), ap.clone()) - .await - .map_err(|e| { - error!("Fresh connection also failed: {e}"); - e - })?; + // Wait for connection activation using signal-based monitoring + match wait_for_connection_activation(conn, &active_conn).await { + Ok(()) => { + debug!("Saved connection activated successfully"); + } + Err(e) => { + warn!("Saved connection activation failed: {e}"); + warn!("Deleting saved connection and retrying with fresh credentials"); + + let _ = nm.deactivate_connection(active_conn).await; + let _ = delete_connection(conn, saved.clone()).await; + + let opts = ConnectionOptions { + autoconnect: true, + autoconnect_priority: None, + autoconnect_retries: None, + }; + + let settings = build_wifi_connection(ap.as_str(), creds, &opts); + + debug!("Creating fresh connection with corrected settings"); + let (_, new_active_conn) = nm + .add_and_activate_connection(settings, wifi_device.clone(), ap.clone()) + .await + .map_err(|e| { + error!("Fresh connection also failed: {e}"); + e + })?; + + // Wait for the fresh connection to activate + wait_for_connection_activation(conn, &new_active_conn).await?; + } } } @@ -408,12 +403,16 @@ async fn connect_via_saved( let settings = build_wifi_connection(ap.as_str(), creds, &opts); - nm.add_and_activate_connection(settings, wifi_device.clone(), ap.clone()) + let (_, active_conn) = nm + .add_and_activate_connection(settings, wifi_device.clone(), ap.clone()) .await .map_err(|e| { error!("Fresh connection also failed: {e}"); e })?; + + // Wait for the fresh connection to activate + wait_for_connection_activation(conn, &active_conn).await?; } } @@ -424,7 +423,8 @@ async fn connect_via_saved( /// /// Builds connection settings from the provided credentials, ensures the /// device is disconnected, then calls AddAndActivateConnection to create -/// and activate the connection in one step. +/// and activate the connection in one step. Monitors activation using +/// D-Bus signals for immediate feedback on success or failure. async fn build_and_activate_new( conn: &Connection, nm: &NMProxy<'_>, @@ -445,38 +445,39 @@ async fn build_and_activate_new( ensure_disconnected(conn, nm, wifi_device).await?; - match nm + let (_, active_conn) = match nm .add_and_activate_connection(settings, wifi_device.clone(), ap.clone()) .await { - Ok(_) => debug!("add_and_activate_connection() succeeded"), + Ok(paths) => { + debug!( + "add_and_activate_connection() succeeded, active connection: {}", + paths.1.as_str() + ); + paths + } Err(e) => { error!("add_and_activate_connection() failed: {e}"); return Err(e.into()); } - } - - Delay::new(timeouts::disconnect_poll_interval()).await; + }; - let dev_proxy = NMDeviceProxy::builder(conn) - .path(wifi_device.clone())? - .build() - .await?; + debug!("Waiting for connection activation using signal monitoring..."); - let initial_state = dev_proxy.state().await?; - debug!("Dev state after build_and_activate_new(): {initial_state:?}"); - debug!("Waiting for connection to complete..."); - wait_for_connection_state(&dev_proxy).await?; + // Wait for connection activation using the ActiveConnection signals + wait_for_connection_activation(conn, &active_conn).await?; - info!("Connection request for '{ssid}' submitted successfully"); + info!("Connection to '{ssid}' activated successfully"); Ok(()) } /// Triggers a Wi-Fi scan and finds the target access point. /// -/// Requests a scan, waits for it to complete, then searches for an -/// access point matching the target SSID. +/// Requests a scan, waits briefly for results, then searches for an +/// access point matching the target SSID. The wait time is shorter than +/// polling-based approaches since we just need the scan to populate +/// initial results. async fn scan_and_resolve_ap( conn: &Connection, wifi: &NMWirelessProxy<'_>, @@ -487,7 +488,8 @@ async fn scan_and_resolve_ap( Err(e) => warn!("Scan request failed: {e}"), } - Delay::new(timeouts::scan_wait()).await; + // Brief wait for scan results to populate + sleep(timeouts::scan_wait()).await; debug!("Scan wait complete"); let ap = find_ap(conn, wifi, ssid).await?; diff --git a/nmrs/src/constants.rs b/nmrs/src/constants.rs index 7c541419..1bc06df6 100644 --- a/nmrs/src/constants.rs +++ b/nmrs/src/constants.rs @@ -15,9 +15,7 @@ pub mod device_type { pub mod device_state { pub const UNAVAILABLE: u32 = 20; pub const DISCONNECTED: u32 = 30; - pub const CONFIG: u32 = 50; pub const ACTIVATED: u32 = 100; - pub const FAILED: u32 = 120; } /// WiFi security flag constants @@ -34,39 +32,41 @@ pub mod wifi_mode { pub const AP: u32 = 3; } -/// Timeout and delay constants (in milliseconds) +/// Timeout constants for signal-based waiting. +/// +/// These timeouts are used with D-Bus signal monitoring instead of polling. +/// They define how long to wait for state transitions before giving up. pub mod timeouts { use std::time::Duration; - pub const DISCONNECT_POLL_INTERVAL_MS: u64 = 300; - pub const DISCONNECT_FINAL_DELAY_MS: u64 = 500; - pub const CONNECTION_POLL_INTERVAL_MS: u64 = 500; - pub const SCAN_WAIT_SECONDS: u64 = 3; + /// Maximum time to wait for Wi-Fi device to become ready (60 seconds). + /// + /// Used after enabling Wi-Fi to wait for the hardware to initialize. + const WIFI_READY_TIMEOUT_SECS: u64 = 60; - pub fn disconnect_poll_interval() -> Duration { - Duration::from_millis(DISCONNECT_POLL_INTERVAL_MS) - } + /// Time to wait after requesting a scan before checking results (2 seconds). + /// + /// While we could use signals for scan completion, a short delay is + /// sufficient for now, and simpler for most use cases. + const SCAN_WAIT_SECS: u64 = 2; - pub fn disconnect_final_delay() -> Duration { - Duration::from_millis(DISCONNECT_FINAL_DELAY_MS) - } + /// Brief delay after state transitions to allow NetworkManager to stabilize. + const STABILIZATION_DELAY_MS: u64 = 100; - pub fn connection_poll_interval() -> Duration { - Duration::from_millis(CONNECTION_POLL_INTERVAL_MS) + /// Returns the Wi-Fi ready timeout duration. + pub fn wifi_ready_timeout() -> Duration { + Duration::from_secs(WIFI_READY_TIMEOUT_SECS) } + /// Returns the scan wait duration. pub fn scan_wait() -> Duration { - Duration::from_secs(SCAN_WAIT_SECONDS) + Duration::from_secs(SCAN_WAIT_SECS) } -} -/// Retry count constants -pub mod retries { - pub const DISCONNECT_MAX_RETRIES: u32 = 10; - pub const CONNECTION_MAX_RETRIES: u32 = 40; - pub const CONNECTION_CONFIG_STUCK_THRESHOLD: u32 = 15; - pub const CONNECTION_STUCK_CHECK_START: u32 = 10; - pub const WIFI_READY_MAX_RETRIES: u32 = 20; + /// Returns a brief stabilization delay. + pub fn stabilization_delay() -> Duration { + Duration::from_millis(STABILIZATION_DELAY_MS) + } } /// Signal strength thresholds for bar display diff --git a/nmrs/src/device.rs b/nmrs/src/device.rs index 8b7f1e5a..0fe92e2a 100644 --- a/nmrs/src/device.rs +++ b/nmrs/src/device.rs @@ -1,14 +1,17 @@ //! Network device enumeration and control. //! //! Provides functions for listing network devices, checking Wi-Fi state, -//! and enabling/disabling Wi-Fi. +//! and enabling/disabling Wi-Fi. Uses D-Bus signals for efficient state +//! monitoring instead of polling. +use log::debug; use zbus::Connection; use crate::Result; -use crate::constants::{retries, timeouts}; -use crate::models::{ConnectionError, Device, DeviceState, DeviceType}; +use crate::constants::device_type; +use crate::models::{ConnectionError, Device, DeviceState}; use crate::proxies::{NMDeviceProxy, NMProxy}; +use crate::state_wait::wait_for_wifi_device_ready; /// Lists all network devices managed by NetworkManager. /// @@ -47,25 +50,43 @@ pub(crate) async fn list_devices(conn: &Connection) -> Result> { /// Waits for a Wi-Fi device to become ready for operations. /// -/// Polls until a Wi-Fi device reaches either Disconnected or Activated state, -/// indicating it's ready for scanning or connection operations. This is useful -/// after enabling Wi-Fi, as the device may take time to initialize. +/// Uses D-Bus signals to efficiently wait until a Wi-Fi device reaches +/// either Disconnected or Activated state, indicating it's ready for +/// scanning or connection operations. This is useful after enabling Wi-Fi, +/// as the device may take time to initialize. /// /// Returns `WifiNotReady` if no Wi-Fi device becomes ready within the timeout. pub(crate) async fn wait_for_wifi_ready(conn: &Connection) -> Result<()> { - for _ in 0..retries::WIFI_READY_MAX_RETRIES { - let devices = list_devices(conn).await?; - for dev in devices { - if dev.device_type == DeviceType::Wifi - && (dev.state == DeviceState::Disconnected || dev.state == DeviceState::Activated) - { - return Ok(()); - } + let nm = NMProxy::new(conn).await?; + let devices = nm.get_devices().await?; + + // Find the Wi-Fi device + for dev_path in devices { + let dev = NMDeviceProxy::builder(conn) + .path(dev_path.clone())? + .build() + .await?; + + if dev.device_type().await? != device_type::WIFI { + continue; } - futures_timer::Delay::new(timeouts::scan_wait()).await; + + debug!("Found Wi-Fi device, waiting for it to become ready"); + + // Check current state first + let current_state = dev.state().await?; + let state = DeviceState::from(current_state); + + if state == DeviceState::Disconnected || state == DeviceState::Activated { + debug!("Wi-Fi device already ready"); + return Ok(()); + } + + // Wait for device to become ready using signal-based monitoring + return wait_for_wifi_device_ready(&dev).await; } - Err(ConnectionError::WifiNotReady) + Err(ConnectionError::NoWifiDevice) } /// Enables or disables Wi-Fi globally. diff --git a/nmrs/src/lib.rs b/nmrs/src/lib.rs index ada0659e..3a85906c 100644 --- a/nmrs/src/lib.rs +++ b/nmrs/src/lib.rs @@ -35,6 +35,17 @@ //! specific variants for common failures like authentication errors, timeouts, //! and missing devices. //! +//! # Signal-Based State Monitoring +//! +//! This crate uses D-Bus signals for efficient state monitoring instead of polling. +//! When connecting to a network, the library subscribes to NetworkManager's +//! `StateChanged` signals to detect connection success or failure immediately, +//! rather than polling device state in a loop. This provides: +//! +//! - Faster response times (immediate notification vs polling delay) +//! - Lower CPU usage (no spinning loops) +//! - Better error messages with specific failure reasons +//! //! # Logging //! //! This crate uses the [`log`](https://docs.rs/log) facade for logging. To see @@ -63,8 +74,9 @@ pub mod wifi_builders; // Re-exported public API pub use models::{ - ConnectionError, ConnectionOptions, Device, DeviceState, DeviceType, EapMethod, EapOptions, - Network, NetworkInfo, Phase2, StateReason, WifiSecurity, reason_to_error, + ActiveConnectionState, ConnectionError, ConnectionOptions, ConnectionStateReason, Device, + DeviceState, DeviceType, EapMethod, EapOptions, Network, NetworkInfo, Phase2, StateReason, + WifiSecurity, connection_state_reason_to_error, reason_to_error, }; pub use network_manager::NetworkManager; diff --git a/nmrs/src/models.rs b/nmrs/src/models.rs index 1f88eeb8..a0f266b3 100644 --- a/nmrs/src/models.rs +++ b/nmrs/src/models.rs @@ -2,6 +2,164 @@ use serde::{Deserialize, Serialize}; use std::fmt::{Display, Formatter}; use thiserror::Error; +/// NetworkManager active connection state. +/// +/// These values represent the lifecycle states of an active connection +/// as reported by the NM D-Bus API. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ActiveConnectionState { + /// Connection state is unknown. + Unknown, + /// Connection is activating (connecting). + Activating, + /// Connection is fully activated (connected). + Activated, + /// Connection is deactivating (disconnecting). + Deactivating, + /// Connection is fully deactivated (disconnected). + Deactivated, + /// Unknown state code not mapped to a specific variant. + Other(u32), +} + +impl From for ActiveConnectionState { + fn from(code: u32) -> Self { + match code { + 0 => Self::Unknown, + 1 => Self::Activating, + 2 => Self::Activated, + 3 => Self::Deactivating, + 4 => Self::Deactivated, + v => Self::Other(v), + } + } +} + +impl Display for ActiveConnectionState { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + match self { + Self::Unknown => write!(f, "unknown"), + Self::Activating => write!(f, "activating"), + Self::Activated => write!(f, "activated"), + Self::Deactivating => write!(f, "deactivating"), + Self::Deactivated => write!(f, "deactivated"), + Self::Other(v) => write!(f, "unknown state ({v})"), + } + } +} + +/// NetworkManager active connection state reason codes. +/// +/// These values indicate why an active connection transitioned to its +/// current state. Use `ConnectionStateReason::from(code)` to convert +/// from the raw u32 values returned by NetworkManager signals. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ConnectionStateReason { + /// The reason is unknown. + Unknown, + /// No specific reason. + None, + /// User disconnected. + UserDisconnected, + /// Device disconnected. + DeviceDisconnected, + /// The NetworkManager service stopped. + ServiceStopped, + /// IP configuration was invalid. + IpConfigInvalid, + /// Connection timed out while activating. + ConnectTimeout, + /// Service start timed out. + ServiceStartTimeout, + /// Service failed to start. + ServiceStartFailed, + /// No secrets (password) were provided. + NoSecrets, + /// Login/authentication failed. + LoginFailed, + /// The connection was removed. + ConnectionRemoved, + /// A dependency failed. + DependencyFailed, + /// Device realization failed. + DeviceRealizeFailed, + /// Device was removed. + DeviceRemoved, + /// Unknown reason code not mapped to a specific variant. + Other(u32), +} + +impl From for ConnectionStateReason { + fn from(code: u32) -> Self { + match code { + 0 => Self::Unknown, + 1 => Self::None, + 2 => Self::UserDisconnected, + 3 => Self::DeviceDisconnected, + 4 => Self::ServiceStopped, + 5 => Self::IpConfigInvalid, + 6 => Self::ConnectTimeout, + 7 => Self::ServiceStartTimeout, + 8 => Self::ServiceStartFailed, + 9 => Self::NoSecrets, + 10 => Self::LoginFailed, + 11 => Self::ConnectionRemoved, + 12 => Self::DependencyFailed, + 13 => Self::DeviceRealizeFailed, + 14 => Self::DeviceRemoved, + v => Self::Other(v), + } + } +} + +impl Display for ConnectionStateReason { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + match self { + Self::Unknown => write!(f, "unknown"), + Self::None => write!(f, "none"), + Self::UserDisconnected => write!(f, "user disconnected"), + Self::DeviceDisconnected => write!(f, "device disconnected"), + Self::ServiceStopped => write!(f, "service stopped"), + Self::IpConfigInvalid => write!(f, "IP configuration invalid"), + Self::ConnectTimeout => write!(f, "connection timed out"), + Self::ServiceStartTimeout => write!(f, "service start timed out"), + Self::ServiceStartFailed => write!(f, "service start failed"), + Self::NoSecrets => write!(f, "no secrets (password) provided"), + Self::LoginFailed => write!(f, "login/authentication failed"), + Self::ConnectionRemoved => write!(f, "connection was removed"), + Self::DependencyFailed => write!(f, "dependency failed"), + Self::DeviceRealizeFailed => write!(f, "device realization failed"), + Self::DeviceRemoved => write!(f, "device was removed"), + Self::Other(v) => write!(f, "unknown reason ({v})"), + } + } +} + +/// Converts a connection state reason code to a specific `ConnectionError`. +/// +/// Maps authentication-related failures to `AuthFailed`, timeout issues to `Timeout`, +/// and other failures to the appropriate variant. +pub fn connection_state_reason_to_error(code: u32) -> ConnectionError { + let reason = ConnectionStateReason::from(code); + match reason { + // Authentication failures + ConnectionStateReason::NoSecrets | ConnectionStateReason::LoginFailed => { + ConnectionError::AuthFailed + } + + // Timeout failures + ConnectionStateReason::ConnectTimeout | ConnectionStateReason::ServiceStartTimeout => { + ConnectionError::Timeout + } + + // IP configuration failures (often DHCP) + ConnectionStateReason::IpConfigInvalid => ConnectionError::DhcpFailed, + + // All other failures + _ => ConnectionError::ConnectionFailed(reason), + } +} + /// NetworkManager device state reason codes. /// /// These values come from the NM D-Bus API and indicate why a device @@ -200,10 +358,14 @@ pub enum ConnectionError { #[error("no saved connection for network")] NoSavedConnection, - /// A general connection failure with a reason code from NetworkManager. + /// A general connection failure with a device state reason code. #[error("connection failed: {0}")] Failed(StateReason), + /// A connection activation failure with a connection state reason. + #[error("connection activation failed: {0}")] + ConnectionFailed(ConnectionStateReason), + /// Invalid UTF-8 encountered in SSID. #[error("invalid UTF-8 in SSID: {0}")] InvalidUtf8(#[from] std::str::Utf8Error), @@ -637,4 +799,165 @@ mod tests { "connection failed: carrier changed" ); } + + #[test] + fn active_connection_state_from_u32() { + assert_eq!( + ActiveConnectionState::from(0), + ActiveConnectionState::Unknown + ); + assert_eq!( + ActiveConnectionState::from(1), + ActiveConnectionState::Activating + ); + assert_eq!( + ActiveConnectionState::from(2), + ActiveConnectionState::Activated + ); + assert_eq!( + ActiveConnectionState::from(3), + ActiveConnectionState::Deactivating + ); + assert_eq!( + ActiveConnectionState::from(4), + ActiveConnectionState::Deactivated + ); + assert_eq!( + ActiveConnectionState::from(99), + ActiveConnectionState::Other(99) + ); + } + + #[test] + fn active_connection_state_display() { + assert_eq!(format!("{}", ActiveConnectionState::Unknown), "unknown"); + assert_eq!( + format!("{}", ActiveConnectionState::Activating), + "activating" + ); + assert_eq!(format!("{}", ActiveConnectionState::Activated), "activated"); + assert_eq!( + format!("{}", ActiveConnectionState::Deactivating), + "deactivating" + ); + assert_eq!( + format!("{}", ActiveConnectionState::Deactivated), + "deactivated" + ); + assert_eq!( + format!("{}", ActiveConnectionState::Other(42)), + "unknown state (42)" + ); + } + + #[test] + fn connection_state_reason_from_u32() { + assert_eq!( + ConnectionStateReason::from(0), + ConnectionStateReason::Unknown + ); + assert_eq!(ConnectionStateReason::from(1), ConnectionStateReason::None); + assert_eq!( + ConnectionStateReason::from(2), + ConnectionStateReason::UserDisconnected + ); + assert_eq!( + ConnectionStateReason::from(3), + ConnectionStateReason::DeviceDisconnected + ); + assert_eq!( + ConnectionStateReason::from(6), + ConnectionStateReason::ConnectTimeout + ); + assert_eq!( + ConnectionStateReason::from(9), + ConnectionStateReason::NoSecrets + ); + assert_eq!( + ConnectionStateReason::from(10), + ConnectionStateReason::LoginFailed + ); + assert_eq!( + ConnectionStateReason::from(99), + ConnectionStateReason::Other(99) + ); + } + + #[test] + fn connection_state_reason_display() { + assert_eq!(format!("{}", ConnectionStateReason::Unknown), "unknown"); + assert_eq!( + format!("{}", ConnectionStateReason::NoSecrets), + "no secrets (password) provided" + ); + assert_eq!( + format!("{}", ConnectionStateReason::LoginFailed), + "login/authentication failed" + ); + assert_eq!( + format!("{}", ConnectionStateReason::ConnectTimeout), + "connection timed out" + ); + assert_eq!( + format!("{}", ConnectionStateReason::Other(123)), + "unknown reason (123)" + ); + } + + #[test] + fn connection_state_reason_to_error_auth_failures() { + // NoSecrets and LoginFailed map to AuthFailed + assert!(matches!( + connection_state_reason_to_error(9), + ConnectionError::AuthFailed + )); + assert!(matches!( + connection_state_reason_to_error(10), + ConnectionError::AuthFailed + )); + } + + #[test] + fn connection_state_reason_to_error_timeout() { + // ConnectTimeout and ServiceStartTimeout map to Timeout + assert!(matches!( + connection_state_reason_to_error(6), + ConnectionError::Timeout + )); + assert!(matches!( + connection_state_reason_to_error(7), + ConnectionError::Timeout + )); + } + + #[test] + fn connection_state_reason_to_error_dhcp() { + // IpConfigInvalid maps to DhcpFailed + assert!(matches!( + connection_state_reason_to_error(5), + ConnectionError::DhcpFailed + )); + } + + #[test] + fn connection_state_reason_to_error_generic() { + // Other reasons map to ConnectionFailed + match connection_state_reason_to_error(2) { + ConnectionError::ConnectionFailed(reason) => { + assert_eq!(reason, ConnectionStateReason::UserDisconnected); + } + _ => panic!("expected ConnectionError::ConnectionFailed"), + } + } + + #[test] + fn connection_failed_error_display() { + assert_eq!( + format!( + "{}", + ConnectionError::ConnectionFailed(ConnectionStateReason::NoSecrets) + ), + "connection activation failed: no secrets (password) provided" + ); + } } diff --git a/nmrs/src/proxies.rs b/nmrs/src/proxies.rs deleted file mode 100644 index ee731532..00000000 --- a/nmrs/src/proxies.rs +++ /dev/null @@ -1,175 +0,0 @@ -//! D-Bus proxy traits for NetworkManager interfaces. -//! -//! These traits define the NetworkManager D-Bus API surface used by this crate. -//! The `zbus::proxy` macro generates proxy implementations that handle -//! D-Bus communication automatically. -//! -//! # NetworkManager D-Bus Structure -//! -//! - `/org/freedesktop/NetworkManager` - Main NM object -//! - `/org/freedesktop/NetworkManager/Devices/*` - Device objects -//! - `/org/freedesktop/NetworkManager/AccessPoint/*` - Access point objects -//! - `/org/freedesktop/NetworkManager/Settings` - Connection settings - -use std::collections::HashMap; -use zbus::{Result, proxy}; -use zvariant::OwnedObjectPath; - -/// Proxy for NetworkManager device interface. -/// -/// Provides access to device properties like interface name, type, state, -/// and the reason for state transitions. -#[proxy( - interface = "org.freedesktop.NetworkManager.Device", - default_service = "org.freedesktop.NetworkManager" -)] -pub trait NMDevice { - /// The network interface name (e.g., "wlan0"). - #[zbus(property)] - fn interface(&self) -> Result; - - /// Device type as a numeric code (2 = Wi-Fi). - #[zbus(property)] - fn device_type(&self) -> Result; - - /// Current device state (100 = activated, 120 = failed). - #[zbus(property)] - fn state(&self) -> Result; - - /// Whether NetworkManager manages this device. - #[zbus(property)] - fn managed(&self) -> Result; - - /// The kernel driver in use. - #[zbus(property)] - fn driver(&self) -> Result; - - /// Current state and reason code for the last state change. - #[zbus(property)] - fn state_reason(&self) -> Result<(u32, u32)>; -} - -/// Proxy for the main NetworkManager interface. -/// -/// Provides methods for listing devices, managing connections, -/// and controlling Wi-Fi state. -#[proxy( - interface = "org.freedesktop.NetworkManager", - default_service = "org.freedesktop.NetworkManager", - default_path = "/org/freedesktop/NetworkManager" -)] -pub trait NM { - /// Returns paths to all network devices. - fn get_devices(&self) -> zbus::Result>; - - /// Whether Wi-Fi is globally enabled. - #[zbus(property)] - fn wireless_enabled(&self) -> zbus::Result; - - /// Enable or disable Wi-Fi globally. - #[zbus(property)] - fn set_wireless_enabled(&self, value: bool) -> zbus::Result<()>; - - /// Paths to all active connections. - #[zbus(property)] - fn active_connections(&self) -> zbus::Result>; - - /// Creates a new connection and activates it simultaneously. - /// - /// Returns paths to both the new connection settings and active connection. - fn add_and_activate_connection( - &self, - connection: HashMap<&str, HashMap<&str, zvariant::Value<'_>>>, - device: OwnedObjectPath, - specific_object: OwnedObjectPath, - ) -> zbus::Result<(OwnedObjectPath, OwnedObjectPath)>; - - /// Activates an existing saved connection. - fn activate_connection( - &self, - connection: OwnedObjectPath, - device: OwnedObjectPath, - specific_object: OwnedObjectPath, - ) -> zbus::Result; - - /// Deactivates an active connection. - fn deactivate_connection(&self, active_connection: OwnedObjectPath) -> zbus::Result<()>; -} - -/// Proxy for wireless device interface. -/// -/// Extends the base device interface with Wi-Fi specific functionality -/// like scanning and access point enumeration. -#[proxy( - interface = "org.freedesktop.NetworkManager.Device.Wireless", - default_service = "org.freedesktop.NetworkManager" -)] -pub trait NMWireless { - /// Returns paths to all visible access points. - fn get_all_access_points(&self) -> Result>; - - /// Requests a Wi-Fi scan. Options are usually empty. - fn request_scan(&self, options: HashMap>) -> Result<()>; - - /// Signal emitted when a new access point is discovered. - #[zbus(signal)] - fn access_point_added(&self, path: OwnedObjectPath); - - /// Signal emitted when an access point is no longer visible. - #[zbus(signal)] - fn access_point_removed(&self, path: OwnedObjectPath); - - /// Path to the currently connected access point ("/" if none). - #[zbus(property)] - fn active_access_point(&self) -> Result; - - /// Current connection bitrate in Kbit/s. - #[zbus(property)] - fn bitrate(&self) -> Result; -} - -/// Proxy for access point interface. -/// -/// Provides information about a visible Wi-Fi network including -/// SSID, signal strength, security capabilities, and frequency. -#[proxy( - interface = "org.freedesktop.NetworkManager.AccessPoint", - default_service = "org.freedesktop.NetworkManager" -)] -pub trait NMAccessPoint { - /// SSID as raw bytes (may not be valid UTF-8). - #[zbus(property)] - fn ssid(&self) -> Result>; - - /// Signal strength as percentage (0-100). - #[zbus(property)] - fn strength(&self) -> Result; - - /// BSSID (MAC address) of the access point. - #[zbus(property)] - fn hw_address(&self) -> Result; - - /// General capability flags (bit 0 = privacy/WEP). - #[zbus(property)] - fn flags(&self) -> Result; - - /// WPA security flags (PSK, EAP, etc.). - #[zbus(property)] - fn wpa_flags(&self) -> Result; - - /// RSN/WPA2 security flags. - #[zbus(property)] - fn rsn_flags(&self) -> Result; - - /// Operating frequency in MHz. - #[zbus(property)] - fn frequency(&self) -> Result; - - /// Maximum supported bitrate in Kbit/s. - #[zbus(property)] - fn max_bitrate(&self) -> Result; - - /// Wi-Fi mode (1 = adhoc, 2 = infrastructure, 3 = AP). - #[zbus(property)] - fn mode(&self) -> Result; -} diff --git a/nmrs/src/proxies/access_point.rs b/nmrs/src/proxies/access_point.rs new file mode 100644 index 00000000..b17580b8 --- /dev/null +++ b/nmrs/src/proxies/access_point.rs @@ -0,0 +1,49 @@ +//! NetworkManager Access Point proxy. + +use zbus::{Result, proxy}; + +/// Proxy for access point interface. +/// +/// Provides information about a visible Wi-Fi network including +/// SSID, signal strength, security capabilities, and frequency. +#[proxy( + interface = "org.freedesktop.NetworkManager.AccessPoint", + default_service = "org.freedesktop.NetworkManager" +)] +pub trait NMAccessPoint { + /// SSID as raw bytes (may not be valid UTF-8). + #[zbus(property)] + fn ssid(&self) -> Result>; + + /// Signal strength as percentage (0-100). + #[zbus(property)] + fn strength(&self) -> Result; + + /// BSSID (MAC address) of the access point. + #[zbus(property)] + fn hw_address(&self) -> Result; + + /// General capability flags (bit 0 = privacy/WEP). + #[zbus(property)] + fn flags(&self) -> Result; + + /// WPA security flags (PSK, EAP, etc.). + #[zbus(property)] + fn wpa_flags(&self) -> Result; + + /// RSN/WPA2 security flags. + #[zbus(property)] + fn rsn_flags(&self) -> Result; + + /// Operating frequency in MHz. + #[zbus(property)] + fn frequency(&self) -> Result; + + /// Maximum supported bitrate in Kbit/s. + #[zbus(property)] + fn max_bitrate(&self) -> Result; + + /// Wi-Fi mode (1 = adhoc, 2 = infrastructure, 3 = AP). + #[zbus(property)] + fn mode(&self) -> Result; +} diff --git a/nmrs/src/proxies/active_connection.rs b/nmrs/src/proxies/active_connection.rs new file mode 100644 index 00000000..a98d3bab --- /dev/null +++ b/nmrs/src/proxies/active_connection.rs @@ -0,0 +1,75 @@ +//! NetworkManager Active Connection proxy. + +use zbus::{Result, proxy}; +use zvariant::OwnedObjectPath; + +/// Proxy for active connection interface. +/// +/// Provides access to the state of an active (in-progress or established) +/// network connection. Use this to monitor connection activation progress +/// and detect failures with specific reason codes. +/// +/// # Signals +/// +/// The `StateChanged` signal is emitted when the connection activation state +/// changes. Use `receive_activation_state_changed()` to get a stream of state changes: +/// +/// ```ignore +/// let mut stream = active_conn_proxy.receive_activation_state_changed().await?; +/// while let Some(signal) = stream.next().await { +/// let args = signal.args()?; +/// match args.state { +/// 2 => println!("Connection activated!"), +/// 4 => println!("Connection failed: reason {}", args.reason), +/// _ => {} +/// } +/// } +/// ``` +#[proxy( + interface = "org.freedesktop.NetworkManager.Connection.Active", + default_service = "org.freedesktop.NetworkManager" +)] +pub trait NMActiveConnection { + /// Current state of the active connection. + /// + /// Values: + /// - 0: Unknown + /// - 1: Activating + /// - 2: Activated + /// - 3: Deactivating + /// - 4: Deactivated + #[zbus(property)] + fn state(&self) -> Result; + + /// Path to the connection settings used for this connection. + #[zbus(property)] + fn connection(&self) -> Result; + + /// Path to the specific object (e.g., access point) used for this connection. + #[zbus(property)] + fn specific_object(&self) -> Result; + + /// Connection identifier (usually the SSID for Wi-Fi). + #[zbus(property)] + fn id(&self) -> Result; + + /// Connection UUID. + #[zbus(property)] + fn uuid(&self) -> Result; + + /// Paths to devices using this connection. + #[zbus(property)] + fn devices(&self) -> Result>; + + /// Signal emitted when the connection activation state changes. + /// + /// The method is named `activation_state_changed` to avoid conflicts with + /// the `state` property's change stream. Use `receive_activation_state_changed()` + /// to subscribe to this signal. + /// + /// Arguments: + /// - `state`: The new connection state (see `ActiveConnectionState`) + /// - `reason`: The reason for the state change (see `ConnectionStateReason`) + #[zbus(signal, name = "StateChanged")] + fn activation_state_changed(&self, state: u32, reason: u32); +} diff --git a/nmrs/src/proxies/device.rs b/nmrs/src/proxies/device.rs new file mode 100644 index 00000000..dab79cc5 --- /dev/null +++ b/nmrs/src/proxies/device.rs @@ -0,0 +1,64 @@ +//! NetworkManager Device proxy. + +use zbus::{Result, proxy}; + +/// Proxy for NetworkManager device interface. +/// +/// Provides access to device properties like interface name, type, state, +/// and the reason for state transitions. +/// +/// # Signals +/// +/// The `StateChanged` signal is emitted whenever the device state changes. +/// Use `receive_device_state_changed()` to get a stream of state change events: +/// +/// ```ignore +/// let mut stream = device_proxy.receive_device_state_changed().await?; +/// while let Some(signal) = stream.next().await { +/// let args = signal.args()?; +/// println!("New state: {}, Old state: {}, Reason: {}", +/// args.new_state, args.old_state, args.reason); +/// } +/// ``` +#[proxy( + interface = "org.freedesktop.NetworkManager.Device", + default_service = "org.freedesktop.NetworkManager" +)] +pub trait NMDevice { + /// The network interface name (e.g., "wlan0"). + #[zbus(property)] + fn interface(&self) -> Result; + + /// Device type as a numeric code (2 = Wi-Fi). + #[zbus(property)] + fn device_type(&self) -> Result; + + /// Current device state (100 = activated, 120 = failed). + #[zbus(property)] + fn state(&self) -> Result; + + /// Whether NetworkManager manages this device. + #[zbus(property)] + fn managed(&self) -> Result; + + /// The kernel driver in use. + #[zbus(property)] + fn driver(&self) -> Result; + + /// Current state and reason code for the last state change. + #[zbus(property)] + fn state_reason(&self) -> Result<(u32, u32)>; + + /// Signal emitted when device state changes. + /// + /// The method is named `device_state_changed` to avoid conflicts with the + /// `state` property's change stream. Use `receive_device_state_changed()` + /// to subscribe to this signal. + /// + /// Arguments: + /// - `new_state`: The new device state code + /// - `old_state`: The previous device state code + /// - `reason`: The reason code for the state change + #[zbus(signal, name = "StateChanged")] + fn device_state_changed(&self, new_state: u32, old_state: u32, reason: u32); +} diff --git a/nmrs/src/proxies/main_nm.rs b/nmrs/src/proxies/main_nm.rs new file mode 100644 index 00000000..677a79b1 --- /dev/null +++ b/nmrs/src/proxies/main_nm.rs @@ -0,0 +1,52 @@ +//! Main NetworkManager proxy. + +use std::collections::HashMap; +use zbus::proxy; +use zvariant::OwnedObjectPath; + +/// Proxy for the main NetworkManager interface. +/// +/// Provides methods for listing devices, managing connections, +/// and controlling Wi-Fi state. +#[proxy( + interface = "org.freedesktop.NetworkManager", + default_service = "org.freedesktop.NetworkManager", + default_path = "/org/freedesktop/NetworkManager" +)] +pub trait NM { + /// Returns paths to all network devices. + fn get_devices(&self) -> zbus::Result>; + + /// Whether Wi-Fi is globally enabled. + #[zbus(property)] + fn wireless_enabled(&self) -> zbus::Result; + + /// Enable or disable Wi-Fi globally. + #[zbus(property)] + fn set_wireless_enabled(&self, value: bool) -> zbus::Result<()>; + + /// Paths to all active connections. + #[zbus(property)] + fn active_connections(&self) -> zbus::Result>; + + /// Creates a new connection and activates it simultaneously. + /// + /// Returns paths to both the new connection settings and active connection. + fn add_and_activate_connection( + &self, + connection: HashMap<&str, HashMap<&str, zvariant::Value<'_>>>, + device: OwnedObjectPath, + specific_object: OwnedObjectPath, + ) -> zbus::Result<(OwnedObjectPath, OwnedObjectPath)>; + + /// Activates an existing saved connection. + fn activate_connection( + &self, + connection: OwnedObjectPath, + device: OwnedObjectPath, + specific_object: OwnedObjectPath, + ) -> zbus::Result; + + /// Deactivates an active connection. + fn deactivate_connection(&self, active_connection: OwnedObjectPath) -> zbus::Result<()>; +} diff --git a/nmrs/src/proxies/mod.rs b/nmrs/src/proxies/mod.rs new file mode 100644 index 00000000..0a526b2b --- /dev/null +++ b/nmrs/src/proxies/mod.rs @@ -0,0 +1,34 @@ +//! D-Bus proxy traits for NetworkManager interfaces. +//! +//! These traits define the NetworkManager D-Bus API surface used by this crate. +//! The `zbus::proxy` macro generates proxy implementations that handle +//! D-Bus communication automatically. +//! +//! # NetworkManager D-Bus Structure +//! +//! - `/org/freedesktop/NetworkManager` - Main NM object +//! - `/org/freedesktop/NetworkManager/Devices/*` - Device objects +//! - `/org/freedesktop/NetworkManager/AccessPoint/*` - Access point objects +//! - `/org/freedesktop/NetworkManager/ActiveConnection/*` - Active connection objects +//! - `/org/freedesktop/NetworkManager/Settings` - Connection settings +//! +//! # Signal-based State Monitoring +//! +//! This crate uses D-Bus signals for efficient state monitoring instead of polling: +//! - `NMDevice::StateChanged` - Emitted when device state changes +//! - `NMActiveConnection::StateChanged` - Emitted when connection activation state changes +//! +//! Use the generated `receive_device_state_changed()` and `receive_activation_state_changed()` +//! methods to get signal streams. + +mod access_point; +mod active_connection; +mod device; +mod main_nm; +mod wireless; + +pub use access_point::NMAccessPointProxy; +pub use active_connection::NMActiveConnectionProxy; +pub use device::NMDeviceProxy; +pub use main_nm::NMProxy; +pub use wireless::NMWirelessProxy; diff --git a/nmrs/src/proxies/wireless.rs b/nmrs/src/proxies/wireless.rs new file mode 100644 index 00000000..f3d43c3c --- /dev/null +++ b/nmrs/src/proxies/wireless.rs @@ -0,0 +1,37 @@ +//! NetworkManager Wireless Device proxy. + +use std::collections::HashMap; +use zbus::{Result, proxy}; +use zvariant::OwnedObjectPath; + +/// Proxy for wireless device interface. +/// +/// Extends the base device interface with Wi-Fi specific functionality +/// like scanning and access point enumeration. +#[proxy( + interface = "org.freedesktop.NetworkManager.Device.Wireless", + default_service = "org.freedesktop.NetworkManager" +)] +pub trait NMWireless { + /// Returns paths to all visible access points. + fn get_all_access_points(&self) -> Result>; + + /// Requests a Wi-Fi scan. Options are usually empty. + fn request_scan(&self, options: HashMap>) -> Result<()>; + + /// Signal emitted when a new access point is discovered. + #[zbus(signal)] + fn access_point_added(&self, path: OwnedObjectPath); + + /// Signal emitted when an access point is no longer visible. + #[zbus(signal)] + fn access_point_removed(&self, path: OwnedObjectPath); + + /// Path to the currently connected access point ("/" if none). + #[zbus(property)] + fn active_access_point(&self) -> Result; + + /// Current connection bitrate in Kbit/s. + #[zbus(property)] + fn bitrate(&self) -> Result; +} diff --git a/nmrs/src/state_wait.rs b/nmrs/src/state_wait.rs index e277a7e0..ab47402f 100644 --- a/nmrs/src/state_wait.rs +++ b/nmrs/src/state_wait.rs @@ -1,51 +1,274 @@ -//! Connection state monitoring. +//! Connection state monitoring using D-Bus signals. //! -//! Provides functions to wait for device state transitions during -//! connection establishment, with proper error mapping for failures. +//! Provides functions to wait for device and connection state transitions +//! using NetworkManager's signal-based API instead of polling. This approach +//! is more efficient and provides faster response times. +//! +//! # Signal-Based Monitoring +//! +//! Instead of polling device state in a loop, these functions subscribe to +//! D-Bus signals that NetworkManager emits when state changes occur: +//! +//! - `NMDevice.StateChanged` - Emitted when device state changes +//! - `NMActiveConnection.StateChanged` - Emitted when connection activation state changes +//! +//! This provides a few benefits: +//! - Immediate response to state changes (no polling delay) +//! - Lower CPU usage (no spinning loops) +//! - More reliable; at least in the sense that we won't miss rapid state transitions. +//! - Better error messages with specific failure reasons -use futures_timer::Delay; +use futures::StreamExt; +use log::{debug, warn}; +use std::time::Duration; +use tokio::time::timeout; +use zbus::Connection; use crate::Result; -use crate::constants::{device_state, retries, timeouts}; -use crate::models::{ConnectionError, StateReason, reason_to_error}; -use crate::proxies::NMDeviceProxy; +use crate::constants::{device_state, timeouts}; +use crate::models::{ + ActiveConnectionState, ConnectionError, ConnectionStateReason, connection_state_reason_to_error, +}; +use crate::proxies::{NMActiveConnectionProxy, NMDeviceProxy}; -/// Waits for a device to reach the activated state or fail. -/// -/// Polls the device state until activation succeeds, fails, or times out. -/// Returns a structured error indicating the specific failure reason. -pub(crate) async fn wait_for_connection_state(dev: &NMDeviceProxy<'_>) -> Result<()> { - let mut config_stuck = 0; +/// Default timeout for connection activation (30 seconds). +const CONNECTION_TIMEOUT: Duration = Duration::from_secs(30); + +/// Default timeout for device disconnection (10 seconds). +const DISCONNECT_TIMEOUT: Duration = Duration::from_secs(10); - for i in 0..retries::CONNECTION_MAX_RETRIES { - Delay::new(timeouts::connection_poll_interval()).await; +/// Waits for an active connection to reach the activated state. +/// +/// Monitors the connection activation process by subscribing to the +/// `StateChanged` signal on the active connection object. This provides +/// more detailed error information than device-level monitoring. +/// +/// # Arguments +/// +/// * `conn` - The D-Bus connection +/// * `active_conn_path` - Path to the active connection object +/// +/// # Errors +/// +/// Returns an error if: +/// - The connection enters the `Deactivated` state (with the specific failure reason) +/// - The timeout expires before activation completes +/// - A D-Bus communication error occurs +/// +/// # Example +/// +/// ```ignore +/// let (_, active_conn) = nm.add_and_activate_connection(settings, device, ap).await?; +/// wait_for_connection_activation(&conn, &active_conn).await?; +/// ``` +pub(crate) async fn wait_for_connection_activation( + conn: &Connection, + active_conn_path: &zvariant::OwnedObjectPath, +) -> Result<()> { + let active_conn = NMActiveConnectionProxy::builder(conn) + .path(active_conn_path.clone())? + .build() + .await?; - let raw = dev.state().await?; + // Check current state first + let current_state = active_conn.state().await?; + let state = ActiveConnectionState::from(current_state); + debug!("Current active connection state: {state}"); - if raw == device_state::ACTIVATED { + match state { + ActiveConnectionState::Activated => { + debug!("Connection already activated"); return Ok(()); } + ActiveConnectionState::Deactivated => { + warn!("Connection already deactivated"); + return Err(ConnectionError::ConnectionFailed( + ConnectionStateReason::Unknown, + )); + } + _ => {} + } + + // Subscribe to state change signals + let mut stream = active_conn.receive_activation_state_changed().await?; + debug!("Subscribed to ActiveConnection StateChanged signal"); + + // Wait for state change with timeout + let result = timeout(CONNECTION_TIMEOUT, async { + while let Some(signal) = stream.next().await { + match signal.args() { + Ok(args) => { + let new_state = ActiveConnectionState::from(args.state); + let reason = ConnectionStateReason::from(args.reason); - if raw == device_state::FAILED { - return Err(match dev.state_reason().await { - Ok((_, code)) => reason_to_error(code), - Err(_) => ConnectionError::Failed(StateReason::Unknown), - }); + debug!("Active connection state changed to: {new_state} (reason: {reason})"); + + match new_state { + ActiveConnectionState::Activated => { + debug!("Connection activation successful"); + return Ok(()); + } + ActiveConnectionState::Deactivated => { + debug!("Connection activation failed: {reason}"); + return Err(connection_state_reason_to_error(args.reason)); + } + _ => { + // Still in progress (Activating/Deactivating) + } + } + } + Err(e) => { + warn!("Failed to parse StateChanged signal args: {e}"); + } + } } + // Stream ended unexpectedly + Err(ConnectionError::Stuck("signal stream ended".into())) + }) + .await; + + match result { + Ok(inner) => inner, + Err(_) => { + warn!( + "Connection activation timed out after {:?}", + CONNECTION_TIMEOUT + ); + Err(ConnectionError::Timeout) + } + } +} + +/// Waits for a device to reach the disconnected state using D-Bus signals. +/// +/// Used when disconnecting from a network to ensure the device has fully +/// released the connection before attempting a new one. +/// +/// # Arguments +/// +/// * `dev` - The device proxy to monitor +/// +/// # Errors +/// +/// Returns an error if: +/// - The timeout expires before disconnection completes +/// - A D-Bus communication error occurs +pub(crate) async fn wait_for_device_disconnect(dev: &NMDeviceProxy<'_>) -> Result<()> { + // Check current state first + let current_state = dev.state().await?; + debug!("Current device state for disconnect: {current_state}"); + + if current_state == device_state::DISCONNECTED || current_state == device_state::UNAVAILABLE { + debug!("Device already disconnected"); + return Ok(()); + } + + // Subscribe to state change signals + let mut stream = dev.receive_device_state_changed().await?; + debug!("Subscribed to device StateChanged signal for disconnect"); + + // Wait for disconnect with timeout + let result = timeout(DISCONNECT_TIMEOUT, async { + while let Some(signal) = stream.next().await { + match signal.args() { + Ok(args) => { + let new_state = args.new_state; + debug!("Device state during disconnect: {new_state}"); - if raw == device_state::CONFIG { - config_stuck += 1; - if config_stuck > retries::CONNECTION_CONFIG_STUCK_THRESHOLD { - return Err(ConnectionError::Stuck("config".into())); + if new_state == device_state::DISCONNECTED + || new_state == device_state::UNAVAILABLE + { + debug!("Device reached disconnected state"); + return Ok(()); + } + } + Err(e) => { + warn!("Failed to parse StateChanged signal args: {e}"); + } } - } else { - config_stuck = 0; } + Err(ConnectionError::Stuck("signal stream ended".into())) + }) + .await; - if i > retries::CONNECTION_STUCK_CHECK_START && raw == device_state::DISCONNECTED { - return Err(ConnectionError::Stuck("disconnected".into())); + match result { + Ok(inner) => inner, + Err(_) => { + // Check final state - might have reached target during the last moments + let final_state = dev.state().await?; + if final_state == device_state::DISCONNECTED || final_state == device_state::UNAVAILABLE + { + Ok(()) + } else { + warn!("Disconnect timed out, device still in state: {final_state}"); + Err(ConnectionError::Stuck(format!("state {final_state}"))) + } } } +} + +/// Waits for a Wi-Fi device to be ready (Disconnected or Activated state). +/// +/// Used after enabling Wi-Fi to wait for the device to initialize before +/// performing operations like scanning. +/// +/// # Arguments +/// +/// * `dev` - The device proxy to monitor +/// +/// # Errors +/// +/// Returns `WifiNotReady` if the device doesn't become ready within the timeout. +pub(crate) async fn wait_for_wifi_device_ready(dev: &NMDeviceProxy<'_>) -> Result<()> { + // Check current state first + let current_state = dev.state().await?; + debug!("Current device state for ready check: {current_state}"); + + if current_state == device_state::DISCONNECTED || current_state == device_state::ACTIVATED { + debug!("Device already ready"); + return Ok(()); + } + + // Subscribe to state change signals + let mut stream = dev.receive_device_state_changed().await?; + debug!("Subscribed to device StateChanged signal for ready check"); + + let ready_timeout = timeouts::wifi_ready_timeout(); + + let result = timeout(ready_timeout, async { + while let Some(signal) = stream.next().await { + match signal.args() { + Ok(args) => { + let new_state = args.new_state; + debug!("Device state during ready wait: {new_state}"); - Err(ConnectionError::Timeout) + if new_state == device_state::DISCONNECTED + || new_state == device_state::ACTIVATED + { + debug!("Device is now ready"); + return Ok(()); + } + } + Err(e) => { + warn!("Failed to parse StateChanged signal args: {e}"); + } + } + } + Err(ConnectionError::WifiNotReady) + }) + .await; + + match result { + Ok(inner) => inner, + Err(_) => { + // Check final state + let final_state = dev.state().await?; + if final_state == device_state::DISCONNECTED || final_state == device_state::ACTIVATED { + Ok(()) + } else { + warn!("Wi-Fi device not ready after timeout, state: {final_state}"); + Err(ConnectionError::WifiNotReady) + } + } + } } From dda4875ab4cf542e689a593c31bab823461d75f3 Mon Sep 17 00:00:00 2001 From: Akrm Al-Hakimi Date: Fri, 12 Dec 2025 18:04:47 -0500 Subject: [PATCH 4/6] chore: update CHANGELOG and nix flake --- CHANGELOG.md | 24 ++++ Cargo.lock | 7 + nmrs/Cargo.toml | 2 +- nmrs/src/connection.rs | 24 +++- nmrs/src/state_wait.rs | 286 ++++++++++++++++++----------------------- package.nix | 2 +- 6 files changed, 176 insertions(+), 169 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1aca99db..91ca4e77 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,30 @@ # Changelog ## [Unreleased] +### Changed +- Core: Refactored `connect()` to use event-driven D-Bus signals instead of polling ([#46](https://github.com/cachebag/nmrs/issues/46)) + - Subscribes to `NMDevice.StateChanged` signal for immediate disconnect detection + - Subscribes to `NMActiveConnection.StateChanged` for connection activation monitoring + - Uses `tokio::time::timeout` for proper timeout handling + - Replaced `futures-timer` polling with signal streams + - Faster response times and lower CPU usage + +### Added +- Core: `ActiveConnectionState` enum for connection lifecycle states ([#46](https://github.com/cachebag/nmrs/issues/46)) +- Core: `ConnectionStateReason` enum with detailed failure reasons (NoSecrets, LoginFailed, ConnectTimeout, etc.) ([#46](https://github.com/cachebag/nmrs/issues/46)) +- Core: `connection_state_reason_to_error()` for mapping activation failures to typed errors ([#46](https://github.com/cachebag/nmrs/issues/46)) +- Core: `NMActiveConnection` D-Bus proxy for monitoring connection activation ([#46](https://github.com/cachebag/nmrs/issues/46)) +- Core: Signal-based wait helpers (`wait_for_connection_activation`, `wait_for_device_disconnect`, `wait_for_wifi_device_ready`) ([#46](https://github.com/cachebag/nmrs/issues/46)) + +### Fixed +- Core: `forget()` now verifies final device state if disconnect wait fails - proceeds to deletion only if device is confirmed disconnected, otherwise returns error ([#124](https://github.com/cachebag/nmrs/issues/124)) +- Core: `list_networks()` now preserves security flags when deduplicating APs - previously a secured AP could be overwritten by an unsecured AP with stronger signal ([#123](https://github.com/cachebag/nmrs/issues/123)) +- Core: Fixed race condition in signal-based state waiting - now subscribes to signals before checking current state to avoid missing rapid transitions +- Core: Fixed UI freeze when using nmrs from GTK apps - replaced tokio-specific timeout with runtime-agnostic `futures-timer` that works with any async runtime (glib, tokio, etc.) + +### Removed +- Core: Removed `futures-timer` dependency - replaced with tokio + D-Bus signals ([#46](https://github.com/cachebag/nmrs/issues/46)) + ## [0.4.0-beta] - 2025-12-11 ### **Breaking Changes** - **nmrs**: Expanded `ConnectionError` enum with new variants (`AuthFailed`, `SupplicantConfigFailed`, `SupplicantTimeout`, `DhcpFailed`, `Timeout`, `Stuck`, `NoWifiDevice`, `WifiNotReady`, `NoSavedConnection`, `Failed(StateReason)`) - exhaustive matches will need a wildcard ([#82](https://github.com/cachebag/nmrs/issues/82)) diff --git a/Cargo.lock b/Cargo.lock index 1926e867..d4c844c3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -533,6 +533,12 @@ version = "0.3.31" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f90f7dce0722e95104fcb095585910c0977252f286e354b5e3bd38902cd99988" +[[package]] +name = "futures-timer" +version = "3.0.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f288b0a4f20f9a56b5d1da57e2227c661b7b16168e2f72365f57b63326e29b24" + [[package]] name = "futures-util" version = "0.3.31" @@ -954,6 +960,7 @@ name = "nmrs" version = "0.4.0" dependencies = [ "futures", + "futures-timer", "log", "serde", "thiserror", diff --git a/nmrs/Cargo.toml b/nmrs/Cargo.toml index 1c3d14c1..6b31d236 100644 --- a/nmrs/Cargo.toml +++ b/nmrs/Cargo.toml @@ -16,8 +16,8 @@ zvariant = "5.7.0" serde = { version = "1.0.228", features = ["derive"] } thiserror = "2.0.17" uuid = { version = "1.19.0", features = ["v4"] } -tokio = { version = "1.48.0", features = ["time", "sync"] } futures = "0.3.31" +futures-timer = "3.0" [dev-dependencies] tokio = { version = "1.48.0", features = ["rt-multi-thread", "macros", "sync", "time"] } \ No newline at end of file diff --git a/nmrs/src/connection.rs b/nmrs/src/connection.rs index ba441ef8..d3551c1d 100644 --- a/nmrs/src/connection.rs +++ b/nmrs/src/connection.rs @@ -1,6 +1,6 @@ +use futures_timer::Delay; use log::{debug, error, info, warn}; use std::collections::HashMap; -use tokio::time::sleep; use zbus::Connection; use zvariant::OwnedObjectPath; @@ -124,8 +124,22 @@ pub(crate) async fn forget(conn: &Connection, ssid: &str) -> Result<()> { && decode_ssid_or_empty(&bytes) == ssid { debug!("Disconnecting from active network: {ssid}"); - disconnect_wifi_and_wait(conn, dev_path).await?; - debug!("Wait loop completed"); + if let Err(e) = disconnect_wifi_and_wait(conn, dev_path).await { + warn!("Disconnect wait failed: {e}"); + let final_state = dev.state().await?; + if final_state != device_state::DISCONNECTED + && final_state != device_state::UNAVAILABLE + { + error!( + "Device still connected (state: {final_state}), cannot safely delete" + ); + return Err(ConnectionError::Stuck(format!( + "disconnect failed, device in state {final_state}" + ))); + } + debug!("Device confirmed disconnected, proceeding with deletion"); + } + debug!("Disconnect phase completed"); } } } @@ -249,7 +263,7 @@ pub(crate) async fn disconnect_wifi_and_wait( wait_for_device_disconnect(&dev).await?; // Brief stabilization delay - sleep(timeouts::stabilization_delay()).await; + Delay::new(timeouts::stabilization_delay()).await; Ok(()) } @@ -489,7 +503,7 @@ async fn scan_and_resolve_ap( } // Brief wait for scan results to populate - sleep(timeouts::scan_wait()).await; + Delay::new(timeouts::scan_wait()).await; debug!("Scan wait complete"); let ap = find_ap(conn, wifi, ssid).await?; diff --git a/nmrs/src/state_wait.rs b/nmrs/src/state_wait.rs index ab47402f..d9af1c65 100644 --- a/nmrs/src/state_wait.rs +++ b/nmrs/src/state_wait.rs @@ -18,10 +18,11 @@ //! - More reliable; at least in the sense that we won't miss rapid state transitions. //! - Better error messages with specific failure reasons -use futures::StreamExt; +use futures::{FutureExt, StreamExt, select}; +use futures_timer::Delay; use log::{debug, warn}; +use std::pin::pin; use std::time::Duration; -use tokio::time::timeout; use zbus::Connection; use crate::Result; @@ -42,25 +43,6 @@ const DISCONNECT_TIMEOUT: Duration = Duration::from_secs(10); /// Monitors the connection activation process by subscribing to the /// `StateChanged` signal on the active connection object. This provides /// more detailed error information than device-level monitoring. -/// -/// # Arguments -/// -/// * `conn` - The D-Bus connection -/// * `active_conn_path` - Path to the active connection object -/// -/// # Errors -/// -/// Returns an error if: -/// - The connection enters the `Deactivated` state (with the specific failure reason) -/// - The timeout expires before activation completes -/// - A D-Bus communication error occurs -/// -/// # Example -/// -/// ```ignore -/// let (_, active_conn) = nm.add_and_activate_connection(settings, device, ap).await?; -/// wait_for_connection_activation(&conn, &active_conn).await?; -/// ``` pub(crate) async fn wait_for_connection_activation( conn: &Connection, active_conn_path: &zvariant::OwnedObjectPath, @@ -70,7 +52,11 @@ pub(crate) async fn wait_for_connection_activation( .build() .await?; - // Check current state first + // Subscribe to signals FIRST to avoid race condition + let mut stream = active_conn.receive_activation_state_changed().await?; + debug!("Subscribed to ActiveConnection StateChanged signal"); + + // Check current state - if already terminal, return immediately let current_state = active_conn.state().await?; let state = ActiveConnectionState::from(current_state); debug!("Current active connection state: {state}"); @@ -89,72 +75,56 @@ pub(crate) async fn wait_for_connection_activation( _ => {} } - // Subscribe to state change signals - let mut stream = active_conn.receive_activation_state_changed().await?; - debug!("Subscribed to ActiveConnection StateChanged signal"); - - // Wait for state change with timeout - let result = timeout(CONNECTION_TIMEOUT, async { - while let Some(signal) = stream.next().await { - match signal.args() { - Ok(args) => { - let new_state = ActiveConnectionState::from(args.state); - let reason = ConnectionStateReason::from(args.reason); + // Wait for state change with timeout (runtime-agnostic) + let mut timeout_delay = pin!(Delay::new(CONNECTION_TIMEOUT).fuse()); - debug!("Active connection state changed to: {new_state} (reason: {reason})"); - - match new_state { - ActiveConnectionState::Activated => { - debug!("Connection activation successful"); - return Ok(()); - } - ActiveConnectionState::Deactivated => { - debug!("Connection activation failed: {reason}"); - return Err(connection_state_reason_to_error(args.reason)); - } - _ => { - // Still in progress (Activating/Deactivating) + loop { + select! { + _ = timeout_delay => { + warn!("Connection activation timed out after {:?}", CONNECTION_TIMEOUT); + return Err(ConnectionError::Timeout); + } + signal_opt = stream.next() => { + match signal_opt { + Some(signal) => { + match signal.args() { + Ok(args) => { + let new_state = ActiveConnectionState::from(args.state); + let reason = ConnectionStateReason::from(args.reason); + debug!("Active connection state changed to: {new_state} (reason: {reason})"); + + match new_state { + ActiveConnectionState::Activated => { + debug!("Connection activation successful"); + return Ok(()); + } + ActiveConnectionState::Deactivated => { + debug!("Connection activation failed: {reason}"); + return Err(connection_state_reason_to_error(args.reason)); + } + _ => {} + } + } + Err(e) => { + warn!("Failed to parse StateChanged signal args: {e}"); + } } } - } - Err(e) => { - warn!("Failed to parse StateChanged signal args: {e}"); + None => { + return Err(ConnectionError::Stuck("signal stream ended".into())); + } } } } - // Stream ended unexpectedly - Err(ConnectionError::Stuck("signal stream ended".into())) - }) - .await; - - match result { - Ok(inner) => inner, - Err(_) => { - warn!( - "Connection activation timed out after {:?}", - CONNECTION_TIMEOUT - ); - Err(ConnectionError::Timeout) - } } } /// Waits for a device to reach the disconnected state using D-Bus signals. -/// -/// Used when disconnecting from a network to ensure the device has fully -/// released the connection before attempting a new one. -/// -/// # Arguments -/// -/// * `dev` - The device proxy to monitor -/// -/// # Errors -/// -/// Returns an error if: -/// - The timeout expires before disconnection completes -/// - A D-Bus communication error occurs pub(crate) async fn wait_for_device_disconnect(dev: &NMDeviceProxy<'_>) -> Result<()> { - // Check current state first + // Subscribe to signals FIRST to avoid race condition + let mut stream = dev.receive_device_state_changed().await?; + debug!("Subscribed to device StateChanged signal for disconnect"); + let current_state = dev.state().await?; debug!("Current device state for disconnect: {current_state}"); @@ -163,64 +133,56 @@ pub(crate) async fn wait_for_device_disconnect(dev: &NMDeviceProxy<'_>) -> Resul return Ok(()); } - // Subscribe to state change signals - let mut stream = dev.receive_device_state_changed().await?; - debug!("Subscribed to device StateChanged signal for disconnect"); - - // Wait for disconnect with timeout - let result = timeout(DISCONNECT_TIMEOUT, async { - while let Some(signal) = stream.next().await { - match signal.args() { - Ok(args) => { - let new_state = args.new_state; - debug!("Device state during disconnect: {new_state}"); - - if new_state == device_state::DISCONNECTED - || new_state == device_state::UNAVAILABLE - { - debug!("Device reached disconnected state"); - return Ok(()); - } - } - Err(e) => { - warn!("Failed to parse StateChanged signal args: {e}"); + // Wait for disconnect with timeout (runtime-agnostic) + let mut timeout_delay = pin!(Delay::new(DISCONNECT_TIMEOUT).fuse()); + + loop { + select! { + _ = timeout_delay => { + // Check final state - might have reached target during the last moments + let final_state = dev.state().await?; + if final_state == device_state::DISCONNECTED || final_state == device_state::UNAVAILABLE { + return Ok(()); + } else { + warn!("Disconnect timed out, device still in state: {final_state}"); + return Err(ConnectionError::Stuck(format!("state {final_state}"))); } } - } - Err(ConnectionError::Stuck("signal stream ended".into())) - }) - .await; - - match result { - Ok(inner) => inner, - Err(_) => { - // Check final state - might have reached target during the last moments - let final_state = dev.state().await?; - if final_state == device_state::DISCONNECTED || final_state == device_state::UNAVAILABLE - { - Ok(()) - } else { - warn!("Disconnect timed out, device still in state: {final_state}"); - Err(ConnectionError::Stuck(format!("state {final_state}"))) + signal_opt = stream.next() => { + match signal_opt { + Some(signal) => { + match signal.args() { + Ok(args) => { + let new_state = args.new_state; + debug!("Device state during disconnect: {new_state}"); + + if new_state == device_state::DISCONNECTED + || new_state == device_state::UNAVAILABLE + { + debug!("Device reached disconnected state"); + return Ok(()); + } + } + Err(e) => { + warn!("Failed to parse StateChanged signal args: {e}"); + } + } + } + None => { + return Err(ConnectionError::Stuck("signal stream ended".into())); + } + } } } } } /// Waits for a Wi-Fi device to be ready (Disconnected or Activated state). -/// -/// Used after enabling Wi-Fi to wait for the device to initialize before -/// performing operations like scanning. -/// -/// # Arguments -/// -/// * `dev` - The device proxy to monitor -/// -/// # Errors -/// -/// Returns `WifiNotReady` if the device doesn't become ready within the timeout. pub(crate) async fn wait_for_wifi_device_ready(dev: &NMDeviceProxy<'_>) -> Result<()> { - // Check current state first + // Subscribe to signals FIRST to avoid race condition + let mut stream = dev.receive_device_state_changed().await?; + debug!("Subscribed to device StateChanged signal for ready check"); + let current_state = dev.state().await?; debug!("Current device state for ready check: {current_state}"); @@ -229,45 +191,45 @@ pub(crate) async fn wait_for_wifi_device_ready(dev: &NMDeviceProxy<'_>) -> Resul return Ok(()); } - // Subscribe to state change signals - let mut stream = dev.receive_device_state_changed().await?; - debug!("Subscribed to device StateChanged signal for ready check"); - let ready_timeout = timeouts::wifi_ready_timeout(); - - let result = timeout(ready_timeout, async { - while let Some(signal) = stream.next().await { - match signal.args() { - Ok(args) => { - let new_state = args.new_state; - debug!("Device state during ready wait: {new_state}"); - - if new_state == device_state::DISCONNECTED - || new_state == device_state::ACTIVATED - { - debug!("Device is now ready"); - return Ok(()); - } - } - Err(e) => { - warn!("Failed to parse StateChanged signal args: {e}"); + let mut timeout_delay = pin!(Delay::new(ready_timeout).fuse()); + + loop { + select! { + _ = timeout_delay => { + // Check final state + let final_state = dev.state().await?; + if final_state == device_state::DISCONNECTED || final_state == device_state::ACTIVATED { + return Ok(()); + } else { + warn!("Wi-Fi device not ready after timeout, state: {final_state}"); + return Err(ConnectionError::WifiNotReady); } } - } - Err(ConnectionError::WifiNotReady) - }) - .await; - - match result { - Ok(inner) => inner, - Err(_) => { - // Check final state - let final_state = dev.state().await?; - if final_state == device_state::DISCONNECTED || final_state == device_state::ACTIVATED { - Ok(()) - } else { - warn!("Wi-Fi device not ready after timeout, state: {final_state}"); - Err(ConnectionError::WifiNotReady) + signal_opt = stream.next() => { + match signal_opt { + Some(signal) => { + match signal.args() { + Ok(args) => { + let new_state = args.new_state; + debug!("Device state during ready wait: {new_state}"); + + if new_state == device_state::DISCONNECTED + || new_state == device_state::ACTIVATED + { + debug!("Device is now ready"); + return Ok(()); + } + } + Err(e) => { + warn!("Failed to parse StateChanged signal args: {e}"); + } + } + } + None => { + return Err(ConnectionError::WifiNotReady); + } + } } } } diff --git a/package.nix b/package.nix index 76e6c730..66b883f4 100644 --- a/package.nix +++ b/package.nix @@ -19,7 +19,7 @@ rustPlatform.buildRustPackage { src = ./.; - cargoHash = "sha256-Z538+q/Af7nshS8G8mPV7aGfTd1XiGKjYljNf9FW3HA="; + cargoHash = "sha256-a3b2BGru8qmi/9Vmt5bUt/8PHrPe8GZbfFAng6Ce+C0="; nativeBuildInputs = [ pkg-config From 7664f943bcf0c8a456231761108d8a85eea8a7bc Mon Sep 17 00:00:00 2001 From: Akrm Al-Hakimi Date: Fri, 12 Dec 2025 23:34:52 -0500 Subject: [PATCH 5/6] fix(cont #46): replace polling with signal-based monitoring and fix GTK population issues and freezing --- CHANGELOG.md | 27 ++++------- nmrs-gui/src/ui/header.rs | 2 +- nmrs-gui/src/ui/mod.rs | 34 ++++++++++++-- nmrs/src/lib.rs | 1 + nmrs/src/network_manager.rs | 37 +++++++++++++++ nmrs/src/network_monitor.rs | 89 +++++++++++++++++++++++++++++++++++++ package.nix | 2 +- 7 files changed, 169 insertions(+), 23 deletions(-) create mode 100644 nmrs/src/network_monitor.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 91ca4e77..498d4c7f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,28 +2,19 @@ ## [Unreleased] ### Changed -- Core: Refactored `connect()` to use event-driven D-Bus signals instead of polling ([#46](https://github.com/cachebag/nmrs/issues/46)) - - Subscribes to `NMDevice.StateChanged` signal for immediate disconnect detection - - Subscribes to `NMActiveConnection.StateChanged` for connection activation monitoring - - Uses `tokio::time::timeout` for proper timeout handling - - Replaced `futures-timer` polling with signal streams - - Faster response times and lower CPU usage +- Core: Refactored connection monitoring from polling to event-driven D-Bus signals for faster response times and lower CPU usage ([#46](https://github.com/cachebag/nmrs/issues/46)) +- Core: Replaced `tokio` with `futures-timer` for runtime-agnostic async support (fixes GTK/glib compatibility) ### Added -- Core: `ActiveConnectionState` enum for connection lifecycle states ([#46](https://github.com/cachebag/nmrs/issues/46)) -- Core: `ConnectionStateReason` enum with detailed failure reasons (NoSecrets, LoginFailed, ConnectTimeout, etc.) ([#46](https://github.com/cachebag/nmrs/issues/46)) -- Core: `connection_state_reason_to_error()` for mapping activation failures to typed errors ([#46](https://github.com/cachebag/nmrs/issues/46)) -- Core: `NMActiveConnection` D-Bus proxy for monitoring connection activation ([#46](https://github.com/cachebag/nmrs/issues/46)) -- Core: Signal-based wait helpers (`wait_for_connection_activation`, `wait_for_device_disconnect`, `wait_for_wifi_device_ready`) ([#46](https://github.com/cachebag/nmrs/issues/46)) +- Core: `ActiveConnectionState` and `ConnectionStateReason` enums for detailed connection status tracking ([#46](https://github.com/cachebag/nmrs/issues/46)) +- Core: `monitor_network_changes()` API for real-time network list updates via D-Bus signals +- Core: `NetworkManager` is now `Clone` ### Fixed -- Core: `forget()` now verifies final device state if disconnect wait fails - proceeds to deletion only if device is confirmed disconnected, otherwise returns error ([#124](https://github.com/cachebag/nmrs/issues/124)) -- Core: `list_networks()` now preserves security flags when deduplicating APs - previously a secured AP could be overwritten by an unsecured AP with stronger signal ([#123](https://github.com/cachebag/nmrs/issues/123)) -- Core: Fixed race condition in signal-based state waiting - now subscribes to signals before checking current state to avoid missing rapid transitions -- Core: Fixed UI freeze when using nmrs from GTK apps - replaced tokio-specific timeout with runtime-agnostic `futures-timer` that works with any async runtime (glib, tokio, etc.) - -### Removed -- Core: Removed `futures-timer` dependency - replaced with tokio + D-Bus signals ([#46](https://github.com/cachebag/nmrs/issues/46)) +- Core: `forget()` now verifies device is disconnected before deleting saved connections ([#124](https://github.com/cachebag/nmrs/issues/124)) +- Core: `list_networks()` preserves security flags when deduplicating APs ([#123](https://github.com/cachebag/nmrs/issues/123)) +- Core: Fixed race condition in signal subscription where rapid state changes could be missed +- GUI: Fixed UI freeze when connecting/forgetting networks ## [0.4.0-beta] - 2025-12-11 ### **Breaking Changes** diff --git a/nmrs-gui/src/ui/header.rs b/nmrs-gui/src/ui/header.rs index 57b3dd23..a2dbab46 100644 --- a/nmrs-gui/src/ui/header.rs +++ b/nmrs-gui/src/ui/header.rs @@ -312,7 +312,7 @@ pub async fn refresh_networks( is_scanning.set(false); } -fn clear_children(container: >k::Box) { +pub fn clear_children(container: >k::Box) { let mut child = container.first_child(); while let Some(widget) = child { child = widget.next_sibling(); diff --git a/nmrs-gui/src/ui/mod.rs b/nmrs-gui/src/ui/mod.rs index 7cc09ad7..ced4f584 100644 --- a/nmrs-gui/src/ui/mod.rs +++ b/nmrs-gui/src/ui/mod.rs @@ -115,7 +115,7 @@ pub fn build_ui(app: &Application) { }; let ctx = Rc::new(networks::NetworksContext { - nm, + nm: nm.clone(), on_success, status: status_clone.clone(), stack: stack_clone.clone(), @@ -123,9 +123,37 @@ pub fn build_ui(app: &Application) { details_page, }); - let header = - header::build_header(ctx, &list_container_clone, is_scanning_clone, &win_clone); + let header = header::build_header( + ctx.clone(), + &list_container_clone, + is_scanning_clone.clone(), + &win_clone, + ); vbox_clone.prepend(&header); + + // TODO: Re-enable network monitoring with proper UI integration + // Currently disabled because it needs access to full context for row creation + /* + // Start background network monitoring for live updates + let nm_monitor = nm.clone(); + let list_container_monitor = list_container_clone.clone(); + let is_scanning_monitor = is_scanning_clone; + let ctx_monitor = ctx.clone(); + + glib::MainContext::default().spawn_local(async move { + let _ = nm_monitor.monitor_network_changes(move || { + let ctx = ctx_monitor.clone(); + let list_container = list_container_monitor.clone(); + let is_scanning = is_scanning_monitor.clone(); + + glib::MainContext::default().spawn_local(async move { + if !is_scanning.get() { + header::refresh_networks(ctx, &list_container, &is_scanning).await; + } + }); + }).await; + }); + */ } Err(err) => { status_clone.set_text(&format!("Failed to initialize: {err}")); diff --git a/nmrs/src/lib.rs b/nmrs/src/lib.rs index 3a85906c..3adc1c83 100644 --- a/nmrs/src/lib.rs +++ b/nmrs/src/lib.rs @@ -62,6 +62,7 @@ mod connection_settings; mod constants; mod device; mod network_info; +mod network_monitor; mod proxies; mod scan; mod state_wait; diff --git a/nmrs/src/network_manager.rs b/nmrs/src/network_manager.rs index 60e0a8c1..68e27809 100644 --- a/nmrs/src/network_manager.rs +++ b/nmrs/src/network_manager.rs @@ -6,12 +6,14 @@ use crate::connection_settings::{get_saved_connection_path, has_saved_connection use crate::device::{list_devices, set_wifi_enabled, wait_for_wifi_ready, wifi_enabled}; use crate::models::{Device, Network, NetworkInfo, WifiSecurity}; use crate::network_info::{current_connection_info, current_ssid, show_details}; +use crate::network_monitor; use crate::scan::{list_networks, scan_networks}; /// High-level interface to NetworkManager over D-Bus. /// /// Provides methods for listing devices, scanning networks, connecting, /// and managing saved connections. +#[derive(Clone)] pub struct NetworkManager { conn: Connection, } @@ -98,4 +100,39 @@ impl NetworkManager { pub async fn forget(&self, ssid: &str) -> Result<()> { forget(&self.conn, ssid).await } + + /// Monitors Wi-Fi network changes in real-time. + /// + /// Subscribes to D-Bus signals for access point additions and removals + /// on all Wi-Fi devices. Invokes the callback whenever the network list + /// changes, enabling live UI updates without polling. + /// + /// This function runs indefinitely until an error occurs. Run it in a + /// background task. + /// + /// # Example + /// + /// ```no_run + /// # use nmrs::NetworkManager; + /// # async fn example() -> nmrs::Result<()> { + /// let nm = NetworkManager::new().await?; + /// + /// // Spawn monitoring task + /// glib::MainContext::default().spawn_local({ + /// let nm = nm.clone(); + /// async move { + /// nm.monitor_network_changes(|| { + /// println!("Networks changed!"); + /// }).await + /// } + /// }); + /// # Ok(()) + /// # } + /// ``` + pub async fn monitor_network_changes(&self, callback: F) -> Result<()> + where + F: Fn() + 'static, + { + network_monitor::monitor_network_changes(&self.conn, callback).await + } } diff --git a/nmrs/src/network_monitor.rs b/nmrs/src/network_monitor.rs new file mode 100644 index 00000000..c856f9a5 --- /dev/null +++ b/nmrs/src/network_monitor.rs @@ -0,0 +1,89 @@ +//! Real-time network monitoring using D-Bus signals. +//! +//! Provides functionality to monitor access point changes (additions/removals) +//! in real-time without needing to poll. This enables live UI updates. + +use futures::stream::{Stream, StreamExt}; +use log::{debug, warn}; +use std::pin::Pin; +use zbus::Connection; + +use crate::Result; +use crate::constants::device_type; +use crate::models::ConnectionError; +use crate::proxies::{NMDeviceProxy, NMProxy, NMWirelessProxy}; + +/// Monitors access point changes on all Wi-Fi devices. +/// +/// Subscribes to `AccessPointAdded` and `AccessPointRemoved` signals on all +/// wireless devices. When any signal is received, invokes the callback to +/// notify the caller that the network list has changed. +/// +/// This function runs indefinitely until an error occurs or the connection +/// is lost. Run it in a background task. +/// +/// # Example +/// +/// ```ignore +/// let nm = NetworkManager::new().await?; +/// nm.monitor_network_changes(|| { +/// println!("Network list changed, refresh UI!"); +/// }).await?; +/// ``` +pub async fn monitor_network_changes(conn: &Connection, callback: F) -> Result<()> +where + F: Fn() + 'static, +{ + let nm = NMProxy::new(conn).await?; + let devices = nm.get_devices().await?; + + // Use dynamic dispatch to handle different signal stream types + let mut streams: Vec>>> = Vec::new(); + + // Subscribe to signals on all Wi-Fi devices + for dev_path in devices { + let dev = NMDeviceProxy::builder(conn) + .path(dev_path.clone())? + .build() + .await?; + + if dev.device_type().await? != device_type::WIFI { + continue; + } + + let wifi = NMWirelessProxy::builder(conn) + .path(dev_path.clone())? + .build() + .await?; + + let added_stream = wifi.receive_access_point_added().await?; + let removed_stream = wifi.receive_access_point_removed().await?; + + // Box both streams as trait objects + streams.push(Box::pin(added_stream.map(|_| ()))); + streams.push(Box::pin(removed_stream.map(|_| ()))); + + debug!("Subscribed to AP signals on device: {dev_path}"); + } + + if streams.is_empty() { + warn!("No Wi-Fi devices found to monitor"); + return Err(ConnectionError::NoWifiDevice); + } + + debug!( + "Monitoring {} signal streams for network changes", + streams.len() + ); + + // Merge all streams and listen for any signal + let mut merged = futures::stream::select_all(streams); + + while let Some(_signal) = merged.next().await { + debug!("Network change detected"); + callback(); + } + + warn!("Network monitoring stream ended unexpectedly"); + Err(ConnectionError::Stuck("monitoring stream ended".into())) +} diff --git a/package.nix b/package.nix index 66b883f4..fb354e3b 100644 --- a/package.nix +++ b/package.nix @@ -19,7 +19,7 @@ rustPlatform.buildRustPackage { src = ./.; - cargoHash = "sha256-a3b2BGru8qmi/9Vmt5bUt/8PHrPe8GZbfFAng6Ce+C0="; + cargoHash = "sha256-vTNK0VmmrpJRqQ50OvYyrhH7ygbTrS1d6u/228pglvE="; nativeBuildInputs = [ pkg-config From 95df02e5b5d2c37054c15e83dee1202068179553 Mon Sep 17 00:00:00 2001 From: Akrm Al-Hakimi Date: Fri, 12 Dec 2025 23:50:24 -0500 Subject: [PATCH 6/6] chore: bump version --- nmrs-gui/Cargo.toml | 2 +- nmrs/Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/nmrs-gui/Cargo.toml b/nmrs-gui/Cargo.toml index 5b59fe35..68f07ec8 100644 --- a/nmrs-gui/Cargo.toml +++ b/nmrs-gui/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "nmrs-gui" -version = "0.4.0" +version = "0.5.0" edition = "2024" [dependencies] diff --git a/nmrs/Cargo.toml b/nmrs/Cargo.toml index 6b31d236..5d0dec61 100644 --- a/nmrs/Cargo.toml +++ b/nmrs/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "nmrs" -version = "0.4.0" +version = "0.5.0" edition = "2024" description = "A Rust library for NetworkManager over D-Bus" license = "MIT"