From 09a38dd136bf38a5af8b68f7a95912d6493f6e1d Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Thu, 15 May 2025 17:05:14 +0300 Subject: [PATCH 01/40] core: battery limit management --- src/cli/debug.rs | 2 +- src/config/load.rs | 21 ++++- src/config/types.rs | 8 +- src/cpu.rs | 189 +++++++++++++++++++++++++++++++++++++++++++- src/daemon.rs | 10 --- src/engine.rs | 44 ++++++++++- src/main.rs | 18 +++++ src/util/error.rs | 2 + 8 files changed, 276 insertions(+), 18 deletions(-) diff --git a/src/cli/debug.rs b/src/cli/debug.rs index c34565d..a36018d 100644 --- a/src/cli/debug.rs +++ b/src/cli/debug.rs @@ -13,7 +13,7 @@ pub fn run_debug(config: &AppConfig) -> Result<(), Box> { println!("Version: {}", env!("CARGO_PKG_VERSION")); // Current date and time - let now = SystemTime::now(); + let _now = SystemTime::now(); // Prefix with underscore to indicate intentionally unused let formatted_time = chrono::Local::now().format("%Y-%m-%d %H:%M:%S"); println!("Timestamp: {formatted_time}"); diff --git a/src/config/load.rs b/src/config/load.rs index f0fedce..1414557 100644 --- a/src/config/load.rs +++ b/src/config/load.rs @@ -68,7 +68,7 @@ pub fn load_config_from_path(specific_path: Option<&str>) -> Result Result { let toml_app_config = toml::from_str::(&contents).map_err(ConfigError::TomlError)?; + // Handle inheritance of values from global to profile configs + let mut charger_profile = toml_app_config.charger.clone(); + let mut battery_profile = toml_app_config.battery.clone(); + + // If profile-specific battery thresholds are not set, inherit from global config + if charger_profile.battery_charge_thresholds.is_none() { + charger_profile.battery_charge_thresholds = toml_app_config.battery_charge_thresholds; + } + + if battery_profile.battery_charge_thresholds.is_none() { + battery_profile.battery_charge_thresholds = toml_app_config.battery_charge_thresholds; + } + // Convert AppConfigToml to AppConfig Ok(AppConfig { - charger: ProfileConfig::from(toml_app_config.charger), - battery: ProfileConfig::from(toml_app_config.battery), - battery_charge_thresholds: toml_app_config.battery_charge_thresholds, + charger: ProfileConfig::from(charger_profile), + battery: ProfileConfig::from(battery_profile), + global_battery_charge_thresholds: toml_app_config.battery_charge_thresholds, ignored_power_supplies: toml_app_config.ignored_power_supplies, poll_interval_sec: toml_app_config.poll_interval_sec, daemon: DaemonConfig { diff --git a/src/config/types.rs b/src/config/types.rs index 3f4f8ed..83c51b5 100644 --- a/src/config/types.rs +++ b/src/config/types.rs @@ -13,6 +13,7 @@ pub struct ProfileConfig { pub max_freq_mhz: Option, pub platform_profile: Option, pub turbo_auto_settings: Option, + pub battery_charge_thresholds: Option<(u8, u8)>, } impl Default for ProfileConfig { @@ -26,6 +27,7 @@ impl Default for ProfileConfig { max_freq_mhz: None, // no override platform_profile: None, // no override turbo_auto_settings: Some(TurboAutoSettings::default()), + battery_charge_thresholds: None, } } } @@ -36,7 +38,8 @@ pub struct AppConfig { pub charger: ProfileConfig, #[serde(default)] pub battery: ProfileConfig, - pub battery_charge_thresholds: Option<(u8, u8)>, // (start_threshold, stop_threshold) + #[serde(rename = "battery_charge_thresholds")] + pub global_battery_charge_thresholds: Option<(u8, u8)>, // (start_threshold, stop_threshold) pub ignored_power_supplies: Option>, #[serde(default = "default_poll_interval_sec")] pub poll_interval_sec: u64, @@ -92,6 +95,7 @@ pub struct ProfileConfigToml { pub min_freq_mhz: Option, pub max_freq_mhz: Option, pub platform_profile: Option, + pub battery_charge_thresholds: Option<(u8, u8)>, } #[derive(Deserialize, Debug, Clone, Default)] @@ -118,6 +122,7 @@ impl Default for ProfileConfigToml { min_freq_mhz: None, max_freq_mhz: None, platform_profile: None, + battery_charge_thresholds: None, } } } @@ -175,6 +180,7 @@ impl From for ProfileConfig { max_freq_mhz: toml_config.max_freq_mhz, platform_profile: toml_config.platform_profile, turbo_auto_settings: Some(TurboAutoSettings::default()), + battery_charge_thresholds: toml_config.battery_charge_thresholds, } } } diff --git a/src/cpu.rs b/src/cpu.rs index ac8092c..188ff70 100644 --- a/src/cpu.rs +++ b/src/cpu.rs @@ -1,7 +1,8 @@ use crate::core::{GovernorOverrideMode, TurboSetting}; use crate::util::error::ControlError; use core::str; -use std::{fs, io, path::Path, string::ToString}; +use log::debug; +use std::{fs, io, path::{Path, PathBuf}, string::ToString}; pub type Result = std::result::Result; @@ -342,3 +343,189 @@ pub fn get_governor_override() -> Option { None } } + +/// Set battery charge thresholds to protect battery health +/// +/// This sets the start and stop charging thresholds for batteries that support this feature. +/// Different laptop vendors implement battery thresholds in different ways, so this function +/// attempts to handle multiple implementations (Lenovo, ASUS, etc.). +/// +/// The thresholds determine at what percentage the battery starts charging (when below start_threshold) +/// and at what percentage it stops (when it reaches stop_threshold). +/// +/// # Arguments +/// +/// * `start_threshold` - The battery percentage at which charging should start (typically 0-99) +/// * `stop_threshold` - The battery percentage at which charging should stop (typically 1-100) +/// +pub fn set_battery_charge_thresholds(start_threshold: u8, stop_threshold: u8) -> Result<()> { + // Validate threshold values + if start_threshold >= stop_threshold { + return Err(ControlError::InvalidValueError(format!( + "Start threshold ({}) must be less than stop threshold ({})", + start_threshold, stop_threshold + ))); + } + + if stop_threshold > 100 { + return Err(ControlError::InvalidValueError(format!( + "Stop threshold ({}) cannot exceed 100%", + stop_threshold + ))); + } + + // Known sysfs paths for battery threshold control by vendor + let threshold_paths = vec![ + // Standard sysfs paths (used by Lenovo and some others) + ThresholdPathPattern { + description: "Standard", + start_path: "charge_control_start_threshold", + stop_path: "charge_control_end_threshold", + }, + // ASUS-specific paths + ThresholdPathPattern { + description: "ASUS", + start_path: "charge_control_start_percentage", + stop_path: "charge_control_end_percentage", + }, + // Huawei-specific paths + ThresholdPathPattern { + description: "Huawei", + start_path: "charge_start_threshold", + stop_path: "charge_stop_threshold", + }, + ]; + + let power_supply_path = Path::new("/sys/class/power_supply"); + if !power_supply_path.exists() { + return Err(ControlError::NotSupported( + "Power supply path not found, battery threshold control not supported".to_string() + )); + } + + let entries = fs::read_dir(power_supply_path).map_err(|e| { + if e.kind() == io::ErrorKind::PermissionDenied { + ControlError::PermissionDenied(format!( + "Permission denied accessing power supply directory: {}", + power_supply_path.display() + )) + } else { + ControlError::Io(e) + } + })?; + + let mut supported_batteries = Vec::new(); + + // Scan all power supplies for battery threshold support + for entry in entries.flatten() { + let ps_path = entry.path(); + let name = entry.file_name().into_string().unwrap_or_default(); + + // Skip non-battery devices + if !is_battery(&ps_path)? { + continue; + } + + // Try each threshold path pattern for this battery + for pattern in &threshold_paths { + let start_threshold_path = ps_path.join(&pattern.start_path); + let stop_threshold_path = ps_path.join(&pattern.stop_path); + + if start_threshold_path.exists() && stop_threshold_path.exists() { + // Found a battery with threshold support + supported_batteries.push(SupportedBattery { + name: name.clone(), + pattern: pattern.clone(), + path: ps_path.clone(), + }); + + // Found a supported pattern, no need to check others for this battery + break; + } + } + } + + if supported_batteries.is_empty() { + return Err(ControlError::NotSupported( + "No batteries with charge threshold control support found".to_string() + )); + } + + // Apply thresholds to all supported batteries + let mut errors = Vec::new(); + let mut success_count = 0; + + for battery in supported_batteries { + let start_path = battery.path.join(&battery.pattern.start_path); + let stop_path = battery.path.join(&battery.pattern.stop_path); + + // Attempt to set both thresholds + match ( + write_sysfs_value(&start_path, &start_threshold.to_string()), + write_sysfs_value(&stop_path, &stop_threshold.to_string()) + ) { + (Ok(_), Ok(_)) => { + debug!("Set {}-{}% charge thresholds for {} battery '{}'", + start_threshold, stop_threshold, battery.pattern.description, battery.name); + success_count += 1; + }, + (start_result, stop_result) => { + let mut error_msg = format!("Failed to set thresholds for {} battery '{}'", + battery.pattern.description, battery.name); + + if let Err(e) = start_result { + error_msg.push_str(&format!(": start threshold error: {}", e)); + } + if let Err(e) = stop_result { + error_msg.push_str(&format!(": stop threshold error: {}", e)); + } + + errors.push(error_msg); + } + } + } + + if success_count > 0 { + // As long as we successfully set thresholds on at least one battery, consider it a success + if !errors.is_empty() { + debug!("Partial success setting battery thresholds: {}", errors.join("; ")); + } + Ok(()) + } else { + Err(ControlError::WriteError(format!( + "Failed to set charge thresholds on any battery: {}", + errors.join("; ") + ))) + } +} + +/// Helper struct for battery charge threshold path patterns +#[derive(Clone)] +struct ThresholdPathPattern { + description: &'static str, + start_path: &'static str, + stop_path: &'static str, +} + +/// Helper struct for batteries with threshold support +struct SupportedBattery { + name: String, + pattern: ThresholdPathPattern, + path: PathBuf, +} + +/// Check if a power supply entry is a battery +fn is_battery(path: &Path) -> Result { + let type_path = path.join("type"); + + if !type_path.exists() { + return Ok(false); + } + + let ps_type = fs::read_to_string(&type_path) + .map_err(|_| ControlError::ReadError(format!("Failed to read {}", type_path.display())))? + .trim() + .to_string(); + + Ok(ps_type == "Battery") +} diff --git a/src/daemon.rs b/src/daemon.rs index b755226..e7b579a 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -202,16 +202,6 @@ pub fn run_daemon(mut config: AppConfig, verbose: bool) -> Result<(), Box LevelFilter { - match log_level { - LogLevel::Error => LevelFilter::Error, - LogLevel::Warning => LevelFilter::Warn, - LogLevel::Info => LevelFilter::Info, - LogLevel::Debug => LevelFilter::Debug, - } -} - /// Write current system stats to a file for --stats to read fn write_stats_file(path: &str, report: &SystemReport) -> Result<(), std::io::Error> { let mut file = File::create(path)?; diff --git a/src/engine.rs b/src/engine.rs index c83d2f2..99cd2f8 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -1,7 +1,7 @@ use crate::config::{AppConfig, ProfileConfig}; use crate::core::{OperationalMode, SystemReport, TurboSetting}; use crate::cpu::{self}; -use crate::util::error::EngineError; +use crate::util::error::{ControlError, EngineError}; use log::{debug, info}; /// Determines the appropriate CPU profile based on power status or forced mode, @@ -92,6 +92,12 @@ pub fn determine_and_apply_settings( cpu::set_platform_profile(profile)?; } + // Apply battery charge thresholds if configured + apply_battery_charge_thresholds( + selected_profile_config.battery_charge_thresholds, + config.global_battery_charge_thresholds, + )?; + debug!("Profile settings applied successfully."); Ok(()) @@ -186,3 +192,39 @@ fn manage_auto_turbo(report: &SystemReport, config: &ProfileConfig) -> Result<() Err(e) => Err(EngineError::ControlError(e)), } } + +/// Apply battery charge thresholds from configuration +fn apply_battery_charge_thresholds( + profile_thresholds: Option<(u8, u8)>, + global_thresholds: Option<(u8, u8)>, +) -> Result<(), EngineError> { + // Try profile-specific thresholds first, fall back to global thresholds + let thresholds = profile_thresholds.or(global_thresholds); + + if let Some((start_threshold, stop_threshold)) = thresholds { + info!("Setting battery charge thresholds: {start_threshold}-{stop_threshold}%"); + match cpu::set_battery_charge_thresholds(start_threshold, stop_threshold) { + Ok(()) => { + debug!("Successfully set battery charge thresholds"); + Ok(()) + } + Err(e) => { + // If the battery doesn't support thresholds, log but don't fail + if matches!(e, ControlError::NotSupported(_)) { + debug!("Battery charge thresholds not supported: {e}"); + Ok(()) + } else { + // For permission errors, provide more helpful message + if matches!(e, ControlError::PermissionDenied(_)) { + debug!("Permission denied setting battery thresholds - requires root privileges"); + } + Err(EngineError::ControlError(e)) + } + } + } + } else { + // No thresholds configured, this is not an error + debug!("No battery charge thresholds configured"); + Ok(()) + } +} diff --git a/src/main.rs b/src/main.rs index 9dfd67e..cf3a4de 100644 --- a/src/main.rs +++ b/src/main.rs @@ -77,6 +77,13 @@ enum Commands { }, /// Set ACPI platform profile SetPlatformProfile { profile: String }, + /// Set battery charge thresholds to extend battery lifespan + SetBatteryThresholds { + /// Percentage at which charging starts (when below this value) + start_threshold: u8, + /// Percentage at which charging stops (when it reaches this value) + stop_threshold: u8, + }, } fn main() { @@ -358,6 +365,17 @@ fn main() { } Some(Commands::SetPlatformProfile { profile }) => cpu::set_platform_profile(&profile) .map_err(|e| Box::new(e) as Box), + Some(Commands::SetBatteryThresholds { + start_threshold, + stop_threshold, + }) => { + info!( + "Setting battery thresholds: start at {}%, stop at {}%", + start_threshold, stop_threshold + ); + cpu::set_battery_charge_thresholds(start_threshold, stop_threshold) + .map_err(|e| Box::new(e) as Box) + } Some(Commands::Daemon { verbose }) => daemon::run_daemon(config, verbose), Some(Commands::Debug) => cli::debug::run_debug(&config), None => { diff --git a/src/util/error.rs b/src/util/error.rs index 3ee0a86..63a6628 100644 --- a/src/util/error.rs +++ b/src/util/error.rs @@ -4,6 +4,7 @@ use std::io; pub enum ControlError { Io(io::Error), WriteError(String), + ReadError(String), InvalidValueError(String), NotSupported(String), PermissionDenied(String), @@ -24,6 +25,7 @@ impl std::fmt::Display for ControlError { match self { Self::Io(e) => write!(f, "I/O error: {e}"), Self::WriteError(s) => write!(f, "Failed to write to sysfs path: {s}"), + Self::ReadError(s) => write!(f, "Failed to read sysfs path: {s}"), Self::InvalidValueError(s) => write!(f, "Invalid value for setting: {s}"), Self::NotSupported(s) => write!(f, "Control action not supported: {s}"), Self::PermissionDenied(s) => { From 099e5c4ba626dfc6085d50f604dd458bac67b8a9 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Thu, 15 May 2025 17:06:30 +0300 Subject: [PATCH 02/40] docs: mention battery management; update sample config --- README.md | 28 +++++++++++++++++++ src/config/load.rs | 4 +-- src/cpu.rs | 67 +++++++++++++++++++++++++++------------------- src/engine.rs | 6 +++-- 4 files changed, 73 insertions(+), 32 deletions(-) diff --git a/README.md b/README.md index 88ebe6a..2ddf235 100644 --- a/README.md +++ b/README.md @@ -111,6 +111,26 @@ sudo superfreq set-min-freq 1200 --core-id 0 sudo superfreq set-max-freq 2800 --core-id 1 ``` +### Battery Management + +```bash +# Set battery charging thresholds to extend battery lifespan +sudo superfreq set-battery-thresholds 40 80 # Start charging at 40%, stop at 80% +``` + +Battery charging thresholds help extend battery longevity by preventing constant +charging to 100%. Different laptop vendors implement this feature differently, +but Superfreq attempts to support multiple vendor implementations including: + +- Lenovo ThinkPad/IdeaPad (Standard implementation) +- ASUS laptops +- Huawei laptops +- Other devices using the standard Linux power_supply API + +Note that battery management is sensitive, and that your mileage may vary. +Please open an issue if your vendor is not supported, but patches would help +more than issue reports, as supporting hardware _needs_ hardware. + ## Configuration Superfreq uses TOML configuration files. Default locations: @@ -139,6 +159,8 @@ platform_profile = "performance" # Min/max frequency in MHz (optional) min_freq_mhz = 800 max_freq_mhz = 3500 +# Optional: Profile-specific battery charge thresholds (overrides global setting) +# battery_charge_thresholds = [40, 80] # Start at 40%, stop at 80% # Settings for when on battery power [battery] @@ -149,6 +171,12 @@ epb = "balance_power" platform_profile = "low-power" min_freq_mhz = 800 max_freq_mhz = 2500 +# Optional: Profile-specific battery charge thresholds (overrides global setting) +# battery_charge_thresholds = [60, 80] # Start at 60%, stop at 80% (more conservative) + +# Global battery charging thresholds (applied to both profiles unless overridden) +# Start charging at 40%, stop at 80% - extends battery lifespan +battery_charge_thresholds = [40, 80] # Daemon configuration [daemon] diff --git a/src/config/load.rs b/src/config/load.rs index 1414557..72c9c0c 100644 --- a/src/config/load.rs +++ b/src/config/load.rs @@ -85,12 +85,12 @@ fn load_and_parse_config(path: &Path) -> Result { // Handle inheritance of values from global to profile configs let mut charger_profile = toml_app_config.charger.clone(); let mut battery_profile = toml_app_config.battery.clone(); - + // If profile-specific battery thresholds are not set, inherit from global config if charger_profile.battery_charge_thresholds.is_none() { charger_profile.battery_charge_thresholds = toml_app_config.battery_charge_thresholds; } - + if battery_profile.battery_charge_thresholds.is_none() { battery_profile.battery_charge_thresholds = toml_app_config.battery_charge_thresholds; } diff --git a/src/cpu.rs b/src/cpu.rs index 188ff70..10a7acb 100644 --- a/src/cpu.rs +++ b/src/cpu.rs @@ -2,7 +2,11 @@ use crate::core::{GovernorOverrideMode, TurboSetting}; use crate::util::error::ControlError; use core::str; use log::debug; -use std::{fs, io, path::{Path, PathBuf}, string::ToString}; +use std::{ + fs, io, + path::{Path, PathBuf}, + string::ToString, +}; pub type Result = std::result::Result; @@ -357,7 +361,7 @@ pub fn get_governor_override() -> Option { /// /// * `start_threshold` - The battery percentage at which charging should start (typically 0-99) /// * `stop_threshold` - The battery percentage at which charging should stop (typically 1-100) -/// +/// pub fn set_battery_charge_thresholds(start_threshold: u8, stop_threshold: u8) -> Result<()> { // Validate threshold values if start_threshold >= stop_threshold { @@ -369,7 +373,7 @@ pub fn set_battery_charge_thresholds(start_threshold: u8, stop_threshold: u8) -> if stop_threshold > 100 { return Err(ControlError::InvalidValueError(format!( - "Stop threshold ({}) cannot exceed 100%", + "Stop threshold ({}) cannot exceed 100%", stop_threshold ))); } @@ -386,7 +390,7 @@ pub fn set_battery_charge_thresholds(start_threshold: u8, stop_threshold: u8) -> ThresholdPathPattern { description: "ASUS", start_path: "charge_control_start_percentage", - stop_path: "charge_control_end_percentage", + stop_path: "charge_control_end_percentage", }, // Huawei-specific paths ThresholdPathPattern { @@ -399,7 +403,7 @@ pub fn set_battery_charge_thresholds(start_threshold: u8, stop_threshold: u8) -> let power_supply_path = Path::new("/sys/class/power_supply"); if !power_supply_path.exists() { return Err(ControlError::NotSupported( - "Power supply path not found, battery threshold control not supported".to_string() + "Power supply path not found, battery threshold control not supported".to_string(), )); } @@ -415,22 +419,22 @@ pub fn set_battery_charge_thresholds(start_threshold: u8, stop_threshold: u8) -> })?; let mut supported_batteries = Vec::new(); - + // Scan all power supplies for battery threshold support for entry in entries.flatten() { let ps_path = entry.path(); let name = entry.file_name().into_string().unwrap_or_default(); - + // Skip non-battery devices if !is_battery(&ps_path)? { continue; } - + // Try each threshold path pattern for this battery for pattern in &threshold_paths { let start_threshold_path = ps_path.join(&pattern.start_path); let stop_threshold_path = ps_path.join(&pattern.stop_path); - + if start_threshold_path.exists() && stop_threshold_path.exists() { // Found a battery with threshold support supported_batteries.push(SupportedBattery { @@ -438,57 +442,64 @@ pub fn set_battery_charge_thresholds(start_threshold: u8, stop_threshold: u8) -> pattern: pattern.clone(), path: ps_path.clone(), }); - + // Found a supported pattern, no need to check others for this battery break; } } } - + if supported_batteries.is_empty() { return Err(ControlError::NotSupported( - "No batteries with charge threshold control support found".to_string() + "No batteries with charge threshold control support found".to_string(), )); } - + // Apply thresholds to all supported batteries let mut errors = Vec::new(); let mut success_count = 0; - + for battery in supported_batteries { let start_path = battery.path.join(&battery.pattern.start_path); let stop_path = battery.path.join(&battery.pattern.stop_path); - + // Attempt to set both thresholds match ( write_sysfs_value(&start_path, &start_threshold.to_string()), - write_sysfs_value(&stop_path, &stop_threshold.to_string()) + write_sysfs_value(&stop_path, &stop_threshold.to_string()), ) { (Ok(_), Ok(_)) => { - debug!("Set {}-{}% charge thresholds for {} battery '{}'", - start_threshold, stop_threshold, battery.pattern.description, battery.name); + debug!( + "Set {}-{}% charge thresholds for {} battery '{}'", + start_threshold, stop_threshold, battery.pattern.description, battery.name + ); success_count += 1; - }, + } (start_result, stop_result) => { - let mut error_msg = format!("Failed to set thresholds for {} battery '{}'", - battery.pattern.description, battery.name); - + let mut error_msg = format!( + "Failed to set thresholds for {} battery '{}'", + battery.pattern.description, battery.name + ); + if let Err(e) = start_result { error_msg.push_str(&format!(": start threshold error: {}", e)); } if let Err(e) = stop_result { error_msg.push_str(&format!(": stop threshold error: {}", e)); } - + errors.push(error_msg); } } } - + if success_count > 0 { // As long as we successfully set thresholds on at least one battery, consider it a success if !errors.is_empty() { - debug!("Partial success setting battery thresholds: {}", errors.join("; ")); + debug!( + "Partial success setting battery thresholds: {}", + errors.join("; ") + ); } Ok(()) } else { @@ -517,15 +528,15 @@ struct SupportedBattery { /// Check if a power supply entry is a battery fn is_battery(path: &Path) -> Result { let type_path = path.join("type"); - + if !type_path.exists() { return Ok(false); } - + let ps_type = fs::read_to_string(&type_path) .map_err(|_| ControlError::ReadError(format!("Failed to read {}", type_path.display())))? .trim() .to_string(); - + Ok(ps_type == "Battery") } diff --git a/src/engine.rs b/src/engine.rs index 99cd2f8..b1ddb38 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -200,7 +200,7 @@ fn apply_battery_charge_thresholds( ) -> Result<(), EngineError> { // Try profile-specific thresholds first, fall back to global thresholds let thresholds = profile_thresholds.or(global_thresholds); - + if let Some((start_threshold, stop_threshold)) = thresholds { info!("Setting battery charge thresholds: {start_threshold}-{stop_threshold}%"); match cpu::set_battery_charge_thresholds(start_threshold, stop_threshold) { @@ -216,7 +216,9 @@ fn apply_battery_charge_thresholds( } else { // For permission errors, provide more helpful message if matches!(e, ControlError::PermissionDenied(_)) { - debug!("Permission denied setting battery thresholds - requires root privileges"); + debug!( + "Permission denied setting battery thresholds - requires root privileges" + ); } Err(EngineError::ControlError(e)) } From 587d6a070fdb38720a73ad8a1978dd3003435f99 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Thu, 15 May 2025 17:28:56 +0300 Subject: [PATCH 03/40] treewide: apply clippy lints; remove dead code --- src/cpu.rs | 8 ++++---- src/monitor.rs | 18 ++---------------- 2 files changed, 6 insertions(+), 20 deletions(-) diff --git a/src/cpu.rs b/src/cpu.rs index 10a7acb..69ef10a 100644 --- a/src/cpu.rs +++ b/src/cpu.rs @@ -432,8 +432,8 @@ pub fn set_battery_charge_thresholds(start_threshold: u8, stop_threshold: u8) -> // Try each threshold path pattern for this battery for pattern in &threshold_paths { - let start_threshold_path = ps_path.join(&pattern.start_path); - let stop_threshold_path = ps_path.join(&pattern.stop_path); + let start_threshold_path = ps_path.join(pattern.start_path); + let stop_threshold_path = ps_path.join(pattern.stop_path); if start_threshold_path.exists() && stop_threshold_path.exists() { // Found a battery with threshold support @@ -460,8 +460,8 @@ pub fn set_battery_charge_thresholds(start_threshold: u8, stop_threshold: u8) -> let mut success_count = 0; for battery in supported_batteries { - let start_path = battery.path.join(&battery.pattern.start_path); - let stop_path = battery.path.join(&battery.pattern.stop_path); + let start_path = battery.path.join(battery.pattern.start_path); + let stop_path = battery.path.join(battery.pattern.stop_path); // Attempt to set both thresholds match ( diff --git a/src/monitor.rs b/src/monitor.rs index 259e872..811f3e9 100644 --- a/src/monitor.rs +++ b/src/monitor.rs @@ -48,7 +48,7 @@ pub fn get_system_info() -> SystemInfo { } #[derive(Debug, Clone, Copy)] -struct CpuTimes { +pub struct CpuTimes { user: u64, nice: u64, system: u64, @@ -57,8 +57,6 @@ struct CpuTimes { irq: u64, softirq: u64, steal: u64, - guest: u64, - guest_nice: u64, } impl CpuTimes { @@ -147,18 +145,6 @@ fn read_all_cpu_times() -> Result> { parts[8] )) })?, - guest: parts[9].parse().map_err(|_| { - SysMonitorError::ProcStatParseError(format!( - "Failed to parse guest time: {}", - parts[9] - )) - })?, - guest_nice: parts[10].parse().map_err(|_| { - SysMonitorError::ProcStatParseError(format!( - "Failed to parse guest_nice time: {}", - parts[10] - )) - })?, }; cpu_times_map.insert(core_id, times); } @@ -288,7 +274,7 @@ pub fn get_cpu_core_info( None } else { let usage = 100.0 * (1.0 - (idle_diff as f32 / total_diff as f32)); - Some(usage.max(0.0).min(100.0)) // clamp between 0 and 100 + Some(usage.clamp(0.0, 100.0)) // clamp between 0 and 100 } }; From 36807f66c44683f5e68d55893888db5fa0535f2f Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Thu, 15 May 2025 20:06:24 +0300 Subject: [PATCH 04/40] core: move batter logic out of cpu module; better hw detection --- src/battery.rs | 256 ++++++++++++++++++++++++++++++++++++++++++++ src/cli/debug.rs | 5 +- src/config/load.rs | 4 - src/config/types.rs | 12 +-- src/cpu.rs | 213 ++---------------------------------- src/engine.rs | 149 +++++++++++++++----------- src/main.rs | 6 +- src/monitor.rs | 99 ++++++++++++++++- src/util/error.rs | 2 - 9 files changed, 458 insertions(+), 288 deletions(-) create mode 100644 src/battery.rs diff --git a/src/battery.rs b/src/battery.rs new file mode 100644 index 0000000..4dd9be3 --- /dev/null +++ b/src/battery.rs @@ -0,0 +1,256 @@ +use crate::util::error::ControlError; +use log::{debug, warn}; +use std::{ + fs, io, + path::{Path, PathBuf}, +}; + +pub type Result = std::result::Result; + +/// Represents a pattern of path suffixes used to control battery charge thresholds +/// for different device vendors. +#[derive(Clone)] +pub struct ThresholdPathPattern { + pub description: &'static str, + pub start_path: &'static str, + pub stop_path: &'static str, +} + +/// Represents a battery that supports charge threshold control +pub struct SupportedBattery { + pub name: String, + pub pattern: ThresholdPathPattern, + pub path: PathBuf, +} + +/// Set battery charge thresholds to protect battery health +/// +/// This sets the start and stop charging thresholds for batteries that support this feature. +/// Different laptop vendors implement battery thresholds in different ways, so this function +/// attempts to handle multiple implementations (Lenovo, ASUS, etc.). +/// +/// The thresholds determine at what percentage the battery starts charging (when below `start_threshold`) +/// and at what percentage it stops (when it reaches `stop_threshold`). +/// +/// # Arguments +/// +/// * `start_threshold` - The battery percentage at which charging should start (typically 0-99) +/// * `stop_threshold` - The battery percentage at which charging should stop (typically 1-100) +/// +/// # Errors +/// +/// Returns an error if: +/// - The thresholds are invalid (start >= stop or stop > 100) +/// - No power supply path is found +/// - No batteries with threshold support are found +/// - Failed to set thresholds on any battery +pub fn set_battery_charge_thresholds(start_threshold: u8, stop_threshold: u8) -> Result<()> { + validate_thresholds(start_threshold, stop_threshold)?; + + let power_supply_path = Path::new("/sys/class/power_supply"); + if !power_supply_path.exists() { + return Err(ControlError::NotSupported( + "Power supply path not found, battery threshold control not supported".to_string(), + )); + } + + let supported_batteries = find_supported_batteries(power_supply_path)?; + if supported_batteries.is_empty() { + return Err(ControlError::NotSupported( + "No batteries with charge threshold control support found".to_string(), + )); + } + + apply_thresholds_to_batteries(&supported_batteries, start_threshold, stop_threshold) +} + +/// Validates that the threshold values are in acceptable ranges +fn validate_thresholds(start_threshold: u8, stop_threshold: u8) -> Result<()> { + if start_threshold >= stop_threshold { + return Err(ControlError::InvalidValueError(format!( + "Start threshold ({start_threshold}) must be less than stop threshold ({stop_threshold})" + ))); + } + + if stop_threshold > 100 { + return Err(ControlError::InvalidValueError(format!( + "Stop threshold ({stop_threshold}) cannot exceed 100%" + ))); + } + + Ok(()) +} + +/// Finds all batteries in the system that support threshold control +fn find_supported_batteries(power_supply_path: &Path) -> Result> { + let entries = fs::read_dir(power_supply_path).map_err(|e| { + if e.kind() == io::ErrorKind::PermissionDenied { + ControlError::PermissionDenied(format!( + "Permission denied accessing power supply directory: {}", + power_supply_path.display() + )) + } else { + ControlError::Io(e) + } + })?; + + let mut supported_batteries = Vec::new(); + for entry in entries.flatten() { + let ps_path = entry.path(); + if is_battery(&ps_path)? { + if let Some(battery) = find_battery_with_threshold_support(&ps_path) { + supported_batteries.push(battery); + } + } + } + + if supported_batteries.is_empty() { + warn!("No batteries with charge threshold support found"); + } else { + debug!( + "Found {} batteries with threshold support", + supported_batteries.len() + ); + for battery in &supported_batteries { + debug!( + "Battery '{}' supports {} threshold control", + battery.name, battery.pattern.description + ); + } + } + + Ok(supported_batteries) +} + +/// Write a value to a sysfs file +fn write_sysfs_value(path: impl AsRef, value: &str) -> Result<()> { + let p = path.as_ref(); + fs::write(p, value).map_err(|e| { + let error_msg = format!("Path: {:?}, Value: '{}', Error: {}", p.display(), value, e); + if e.kind() == io::ErrorKind::PermissionDenied { + ControlError::PermissionDenied(error_msg) + } else { + ControlError::WriteError(error_msg) + } + }) +} + +/// Identifies if a battery supports threshold control and which pattern it uses +fn find_battery_with_threshold_support(ps_path: &Path) -> Option { + let threshold_paths = vec![ + ThresholdPathPattern { + description: "Standard", + start_path: "charge_control_start_threshold", + stop_path: "charge_control_end_threshold", + }, + ThresholdPathPattern { + description: "ASUS", + start_path: "charge_control_start_percentage", + stop_path: "charge_control_end_percentage", + }, + ThresholdPathPattern { + description: "Huawei", + start_path: "charge_start_threshold", + stop_path: "charge_stop_threshold", + }, + // ThinkPad-specific, sometimes used in addition to standard paths + ThresholdPathPattern { + description: "ThinkPad", + start_path: "charge_start_threshold", + stop_path: "charge_stop_threshold", + }, + // Framework laptop support + // FIXME: This needs actual testing. I inferred this behaviour from some + // Framework-specific code, but it may not be correct. + ThresholdPathPattern { + description: "Framework", + start_path: "charge_behaviour_start_threshold", + stop_path: "charge_behaviour_end_threshold", + }, + ]; + + for pattern in &threshold_paths { + let start_threshold_path = ps_path.join(pattern.start_path); + let stop_threshold_path = ps_path.join(pattern.stop_path); + if start_threshold_path.exists() && stop_threshold_path.exists() { + return Some(SupportedBattery { + name: ps_path.file_name()?.to_string_lossy().to_string(), + pattern: pattern.clone(), + path: ps_path.to_path_buf(), + }); + } + } + None +} + +/// Applies the threshold settings to all supported batteries +fn apply_thresholds_to_batteries( + batteries: &[SupportedBattery], + start_threshold: u8, + stop_threshold: u8, +) -> Result<()> { + let mut errors = Vec::new(); + let mut success_count = 0; + + for battery in batteries { + let start_path = battery.path.join(battery.pattern.start_path); + let stop_path = battery.path.join(battery.pattern.stop_path); + + match ( + write_sysfs_value(&start_path, &start_threshold.to_string()), + write_sysfs_value(&stop_path, &stop_threshold.to_string()), + ) { + (Ok(()), Ok(())) => { + debug!( + "Set {}-{}% charge thresholds for {} battery '{}'", + start_threshold, stop_threshold, battery.pattern.description, battery.name + ); + success_count += 1; + } + (start_result, stop_result) => { + let mut error_msg = format!( + "Failed to set thresholds for {} battery '{}'", + battery.pattern.description, battery.name + ); + if let Err(e) = start_result { + error_msg.push_str(&format!(": start threshold error: {e}")); + } + if let Err(e) = stop_result { + error_msg.push_str(&format!(": stop threshold error: {e}")); + } + errors.push(error_msg); + } + } + } + + if success_count > 0 { + if !errors.is_empty() { + debug!( + "Partial success setting battery thresholds: {}", + errors.join("; ") + ); + } + Ok(()) + } else { + Err(ControlError::WriteError(format!( + "Failed to set charge thresholds on any battery: {}", + errors.join("; ") + ))) + } +} + +/// Determines if a power supply entry is a battery +fn is_battery(path: &Path) -> Result { + let type_path = path.join("type"); + + if !type_path.exists() { + return Ok(false); + } + + let ps_type = fs::read_to_string(&type_path) + .map_err(|_| ControlError::ReadError(format!("Failed to read {}", type_path.display())))? + .trim() + .to_string(); + + Ok(ps_type == "Battery") +} diff --git a/src/cli/debug.rs b/src/cli/debug.rs index a36018d..1011463 100644 --- a/src/cli/debug.rs +++ b/src/cli/debug.rs @@ -5,7 +5,7 @@ use crate::monitor; use std::error::Error; use std::fs; use std::process::{Command, Stdio}; -use std::time::{Duration, SystemTime}; +use std::time::Duration; /// Prints comprehensive debug information about the system pub fn run_debug(config: &AppConfig) -> Result<(), Box> { @@ -13,7 +13,6 @@ pub fn run_debug(config: &AppConfig) -> Result<(), Box> { println!("Version: {}", env!("CARGO_PKG_VERSION")); // Current date and time - let _now = SystemTime::now(); // Prefix with underscore to indicate intentionally unused let formatted_time = chrono::Local::now().format("%Y-%m-%d %H:%M:%S"); println!("Timestamp: {formatted_time}"); @@ -246,7 +245,7 @@ fn check_and_print_sysfs_path(path: &str, description: &str) { fn is_systemd_service_active(service_name: &str) -> Result> { let output = Command::new("systemctl") .arg("is-active") - .arg(format!("{}.service", service_name)) + .arg(format!("{service_name}.service")) .stdout(Stdio::piped()) // capture stdout instead of letting it print .stderr(Stdio::null()) // redirect stderr to null .output()?; diff --git a/src/config/load.rs b/src/config/load.rs index 72c9c0c..470a99d 100644 --- a/src/config/load.rs +++ b/src/config/load.rs @@ -68,9 +68,7 @@ pub fn load_config_from_path(specific_path: Option<&str>) -> Result Result { Ok(AppConfig { charger: ProfileConfig::from(charger_profile), battery: ProfileConfig::from(battery_profile), - global_battery_charge_thresholds: toml_app_config.battery_charge_thresholds, ignored_power_supplies: toml_app_config.ignored_power_supplies, - poll_interval_sec: toml_app_config.poll_interval_sec, daemon: DaemonConfig { poll_interval_sec: toml_app_config.daemon.poll_interval_sec, adaptive_interval: toml_app_config.daemon.adaptive_interval, diff --git a/src/config/types.rs b/src/config/types.rs index 83c51b5..52ef5c9 100644 --- a/src/config/types.rs +++ b/src/config/types.rs @@ -38,19 +38,11 @@ pub struct AppConfig { pub charger: ProfileConfig, #[serde(default)] pub battery: ProfileConfig, - #[serde(rename = "battery_charge_thresholds")] - pub global_battery_charge_thresholds: Option<(u8, u8)>, // (start_threshold, stop_threshold) pub ignored_power_supplies: Option>, - #[serde(default = "default_poll_interval_sec")] - pub poll_interval_sec: u64, #[serde(default)] pub daemon: DaemonConfig, } -const fn default_poll_interval_sec() -> u64 { - 5 -} - // Error type for config loading #[derive(Debug)] pub enum ConfigError { @@ -225,6 +217,10 @@ impl Default for DaemonConfig { } } +const fn default_poll_interval_sec() -> u64 { + 5 +} + const fn default_adaptive_interval() -> bool { false } diff --git a/src/cpu.rs b/src/cpu.rs index 69ef10a..62d0dd1 100644 --- a/src/cpu.rs +++ b/src/cpu.rs @@ -1,12 +1,7 @@ use crate::core::{GovernorOverrideMode, TurboSetting}; use crate::util::error::ControlError; use core::str; -use log::debug; -use std::{ - fs, io, - path::{Path, PathBuf}, - string::ToString, -}; +use std::{fs, io, path::Path, string::ToString}; pub type Result = std::result::Result; @@ -170,8 +165,7 @@ pub fn set_epp(epp: &str, core_id: Option) -> Result<()> { } pub fn set_epb(epb: &str, core_id: Option) -> Result<()> { - // EPB is often an integer 0-15. Ensure `epb` string is valid if parsing. - // For now, writing it directly as a string. + // EPB is often an integer 0-15. let action = |id: u32| { let path = format!("/sys/devices/system/cpu/cpu{id}/cpufreq/energy_performance_bias"); if Path::new(&path).exists() { @@ -249,11 +243,13 @@ pub fn set_platform_profile(profile: &str) -> Result<()> { /// /// # Errors /// -/// Returns [`ControlError::NotSupported`] if: -/// - The file `/sys/firmware/acpi/platform_profile_choices` does not exist. -/// - The file `/sys/firmware/acpi/platform_profile_choices` is empty. +/// # Returns /// -/// Returns [`ControlError::PermissionDenied`] if the file `/sys/firmware/acpi/platform_profile_choices` cannot be read. +/// - [`ControlError::NotSupported`] if: +/// - The file `/sys/firmware/acpi/platform_profile_choices` does not exist. +/// - The file `/sys/firmware/acpi/platform_profile_choices` is empty. +/// +/// - [`ControlError::PermissionDenied`] if the file `/sys/firmware/acpi/platform_profile_choices` cannot be read. /// pub fn get_platform_profiles() -> Result> { let path = "/sys/firmware/acpi/platform_profile_choices"; @@ -347,196 +343,3 @@ pub fn get_governor_override() -> Option { None } } - -/// Set battery charge thresholds to protect battery health -/// -/// This sets the start and stop charging thresholds for batteries that support this feature. -/// Different laptop vendors implement battery thresholds in different ways, so this function -/// attempts to handle multiple implementations (Lenovo, ASUS, etc.). -/// -/// The thresholds determine at what percentage the battery starts charging (when below start_threshold) -/// and at what percentage it stops (when it reaches stop_threshold). -/// -/// # Arguments -/// -/// * `start_threshold` - The battery percentage at which charging should start (typically 0-99) -/// * `stop_threshold` - The battery percentage at which charging should stop (typically 1-100) -/// -pub fn set_battery_charge_thresholds(start_threshold: u8, stop_threshold: u8) -> Result<()> { - // Validate threshold values - if start_threshold >= stop_threshold { - return Err(ControlError::InvalidValueError(format!( - "Start threshold ({}) must be less than stop threshold ({})", - start_threshold, stop_threshold - ))); - } - - if stop_threshold > 100 { - return Err(ControlError::InvalidValueError(format!( - "Stop threshold ({}) cannot exceed 100%", - stop_threshold - ))); - } - - // Known sysfs paths for battery threshold control by vendor - let threshold_paths = vec![ - // Standard sysfs paths (used by Lenovo and some others) - ThresholdPathPattern { - description: "Standard", - start_path: "charge_control_start_threshold", - stop_path: "charge_control_end_threshold", - }, - // ASUS-specific paths - ThresholdPathPattern { - description: "ASUS", - start_path: "charge_control_start_percentage", - stop_path: "charge_control_end_percentage", - }, - // Huawei-specific paths - ThresholdPathPattern { - description: "Huawei", - start_path: "charge_start_threshold", - stop_path: "charge_stop_threshold", - }, - ]; - - let power_supply_path = Path::new("/sys/class/power_supply"); - if !power_supply_path.exists() { - return Err(ControlError::NotSupported( - "Power supply path not found, battery threshold control not supported".to_string(), - )); - } - - let entries = fs::read_dir(power_supply_path).map_err(|e| { - if e.kind() == io::ErrorKind::PermissionDenied { - ControlError::PermissionDenied(format!( - "Permission denied accessing power supply directory: {}", - power_supply_path.display() - )) - } else { - ControlError::Io(e) - } - })?; - - let mut supported_batteries = Vec::new(); - - // Scan all power supplies for battery threshold support - for entry in entries.flatten() { - let ps_path = entry.path(); - let name = entry.file_name().into_string().unwrap_or_default(); - - // Skip non-battery devices - if !is_battery(&ps_path)? { - continue; - } - - // Try each threshold path pattern for this battery - for pattern in &threshold_paths { - let start_threshold_path = ps_path.join(pattern.start_path); - let stop_threshold_path = ps_path.join(pattern.stop_path); - - if start_threshold_path.exists() && stop_threshold_path.exists() { - // Found a battery with threshold support - supported_batteries.push(SupportedBattery { - name: name.clone(), - pattern: pattern.clone(), - path: ps_path.clone(), - }); - - // Found a supported pattern, no need to check others for this battery - break; - } - } - } - - if supported_batteries.is_empty() { - return Err(ControlError::NotSupported( - "No batteries with charge threshold control support found".to_string(), - )); - } - - // Apply thresholds to all supported batteries - let mut errors = Vec::new(); - let mut success_count = 0; - - for battery in supported_batteries { - let start_path = battery.path.join(battery.pattern.start_path); - let stop_path = battery.path.join(battery.pattern.stop_path); - - // Attempt to set both thresholds - match ( - write_sysfs_value(&start_path, &start_threshold.to_string()), - write_sysfs_value(&stop_path, &stop_threshold.to_string()), - ) { - (Ok(_), Ok(_)) => { - debug!( - "Set {}-{}% charge thresholds for {} battery '{}'", - start_threshold, stop_threshold, battery.pattern.description, battery.name - ); - success_count += 1; - } - (start_result, stop_result) => { - let mut error_msg = format!( - "Failed to set thresholds for {} battery '{}'", - battery.pattern.description, battery.name - ); - - if let Err(e) = start_result { - error_msg.push_str(&format!(": start threshold error: {}", e)); - } - if let Err(e) = stop_result { - error_msg.push_str(&format!(": stop threshold error: {}", e)); - } - - errors.push(error_msg); - } - } - } - - if success_count > 0 { - // As long as we successfully set thresholds on at least one battery, consider it a success - if !errors.is_empty() { - debug!( - "Partial success setting battery thresholds: {}", - errors.join("; ") - ); - } - Ok(()) - } else { - Err(ControlError::WriteError(format!( - "Failed to set charge thresholds on any battery: {}", - errors.join("; ") - ))) - } -} - -/// Helper struct for battery charge threshold path patterns -#[derive(Clone)] -struct ThresholdPathPattern { - description: &'static str, - start_path: &'static str, - stop_path: &'static str, -} - -/// Helper struct for batteries with threshold support -struct SupportedBattery { - name: String, - pattern: ThresholdPathPattern, - path: PathBuf, -} - -/// Check if a power supply entry is a battery -fn is_battery(path: &Path) -> Result { - let type_path = path.join("type"); - - if !type_path.exists() { - return Ok(false); - } - - let ps_type = fs::read_to_string(&type_path) - .map_err(|_| ControlError::ReadError(format!("Failed to read {}", type_path.display())))? - .trim() - .to_string(); - - Ok(ps_type == "Battery") -} diff --git a/src/engine.rs b/src/engine.rs index b1ddb38..67aa8b6 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -1,8 +1,39 @@ +use crate::battery; use crate::config::{AppConfig, ProfileConfig}; use crate::core::{OperationalMode, SystemReport, TurboSetting}; use crate::cpu::{self}; use crate::util::error::{ControlError, EngineError}; -use log::{debug, info}; +use log::{debug, info, warn}; + +/// Try applying a CPU feature and handle common error cases. Centralizes the where we +/// previously did: +/// 1. Try to apply a feature setting +/// 2. If not supported, log a warning and continue +/// 3. If other error, propagate the error +fn try_apply_feature( + feature_name: &str, + value_description: &str, + apply_fn: F, +) -> Result<(), EngineError> +where + F: FnOnce() -> Result, +{ + info!("Setting {feature_name} to '{value_description}'"); + + match apply_fn() { + Ok(_) => Ok(()), + Err(e) => { + if matches!(e, ControlError::NotSupported(_)) { + warn!( + "{feature_name} setting is not supported on this system. Skipping {feature_name} configuration." + ); + Ok(()) + } else { + Err(EngineError::ControlError(e)) + } + } + } +} /// Determines the appropriate CPU profile based on power status or forced mode, /// and applies the settings using functions from the `cpu` module. @@ -17,6 +48,8 @@ pub fn determine_and_apply_settings( "Governor override is active: '{}'. Setting governor.", override_governor.trim() ); + + // Apply the override governor setting - validation is handled by set_governor cpu::set_governor(override_governor.trim(), None)?; } @@ -35,11 +68,15 @@ pub fn determine_and_apply_settings( } } else { // Determine AC/Battery status - // If no batteries, assume AC power (desktop). - // Otherwise, check the ac_connected status from the (first) battery. - // XXX: This relies on the setting ac_connected in BatteryInfo being set correctly. - let on_ac_power = - report.batteries.is_empty() || report.batteries.first().is_some_and(|b| b.ac_connected); + // For desktops (no batteries), we should always use the AC power profile + // For laptops, we check if any battery is present and not connected to AC + let on_ac_power = if report.batteries.is_empty() { + // No batteries means desktop/server, always on AC + true + } else { + // At least one battery exists, check if it's on AC + report.batteries.first().is_some_and(|b| b.ac_connected) + }; if on_ac_power { info!("On AC power, selecting Charger profile."); @@ -53,7 +90,19 @@ pub fn determine_and_apply_settings( // Apply settings from selected_profile_config if let Some(governor) = &selected_profile_config.governor { info!("Setting governor to '{governor}'"); - cpu::set_governor(governor, None)?; + // Let set_governor handle the validation + if let Err(e) = cpu::set_governor(governor, None) { + // If the governor is not available, log a warning + if matches!(e, ControlError::InvalidValueError(_)) + || matches!(e, ControlError::NotSupported(_)) + { + warn!( + "Configured governor '{governor}' is not available on this system. Skipping." + ); + } else { + return Err(e.into()); + } + } } if let Some(turbo_setting) = selected_profile_config.turbo { @@ -63,40 +112,56 @@ pub fn determine_and_apply_settings( debug!("Managing turbo in auto mode based on system conditions"); manage_auto_turbo(report, selected_profile_config)?; } - _ => cpu::set_turbo(turbo_setting)?, + _ => { + try_apply_feature("Turbo boost", &format!("{turbo_setting:?}"), || { + cpu::set_turbo(turbo_setting) + })?; + } } } if let Some(epp) = &selected_profile_config.epp { - info!("Setting EPP to '{epp}'"); - cpu::set_epp(epp, None)?; + try_apply_feature("EPP", epp, || cpu::set_epp(epp, None))?; } if let Some(epb) = &selected_profile_config.epb { - info!("Setting EPB to '{epb}'"); - cpu::set_epb(epb, None)?; + try_apply_feature("EPB", epb, || cpu::set_epb(epb, None))?; } if let Some(min_freq) = selected_profile_config.min_freq_mhz { - info!("Setting min frequency to '{min_freq} MHz'"); - cpu::set_min_frequency(min_freq, None)?; + try_apply_feature("min frequency", &format!("{min_freq} MHz"), || { + cpu::set_min_frequency(min_freq, None) + })?; } if let Some(max_freq) = selected_profile_config.max_freq_mhz { - info!("Setting max frequency to '{max_freq} MHz'"); - cpu::set_max_frequency(max_freq, None)?; + try_apply_feature("max frequency", &format!("{max_freq} MHz"), || { + cpu::set_max_frequency(max_freq, None) + })?; } if let Some(profile) = &selected_profile_config.platform_profile { - info!("Setting platform profile to '{profile}'"); - cpu::set_platform_profile(profile)?; + try_apply_feature("platform profile", profile, || { + cpu::set_platform_profile(profile) + })?; } - // Apply battery charge thresholds if configured - apply_battery_charge_thresholds( - selected_profile_config.battery_charge_thresholds, - config.global_battery_charge_thresholds, - )?; + // Set battery charge thresholds if configured + if let Some((start_threshold, stop_threshold)) = + selected_profile_config.battery_charge_thresholds + { + if start_threshold < stop_threshold && stop_threshold <= 100 { + info!("Setting battery charge thresholds: {start_threshold}-{stop_threshold}%"); + match battery::set_battery_charge_thresholds(start_threshold, stop_threshold) { + Ok(()) => debug!("Battery charge thresholds set successfully"), + Err(e) => warn!("Failed to set battery charge thresholds: {e}"), + } + } else { + warn!( + "Invalid battery threshold values: start={start_threshold}, stop={stop_threshold}" + ); + } + } debug!("Profile settings applied successfully."); @@ -192,41 +257,3 @@ fn manage_auto_turbo(report: &SystemReport, config: &ProfileConfig) -> Result<() Err(e) => Err(EngineError::ControlError(e)), } } - -/// Apply battery charge thresholds from configuration -fn apply_battery_charge_thresholds( - profile_thresholds: Option<(u8, u8)>, - global_thresholds: Option<(u8, u8)>, -) -> Result<(), EngineError> { - // Try profile-specific thresholds first, fall back to global thresholds - let thresholds = profile_thresholds.or(global_thresholds); - - if let Some((start_threshold, stop_threshold)) = thresholds { - info!("Setting battery charge thresholds: {start_threshold}-{stop_threshold}%"); - match cpu::set_battery_charge_thresholds(start_threshold, stop_threshold) { - Ok(()) => { - debug!("Successfully set battery charge thresholds"); - Ok(()) - } - Err(e) => { - // If the battery doesn't support thresholds, log but don't fail - if matches!(e, ControlError::NotSupported(_)) { - debug!("Battery charge thresholds not supported: {e}"); - Ok(()) - } else { - // For permission errors, provide more helpful message - if matches!(e, ControlError::PermissionDenied(_)) { - debug!( - "Permission denied setting battery thresholds - requires root privileges" - ); - } - Err(EngineError::ControlError(e)) - } - } - } - } else { - // No thresholds configured, this is not an error - debug!("No battery charge thresholds configured"); - Ok(()) - } -} diff --git a/src/main.rs b/src/main.rs index cf3a4de..dc9bf6a 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,3 +1,4 @@ +mod battery; mod cli; mod config; mod conflict; @@ -370,10 +371,9 @@ fn main() { stop_threshold, }) => { info!( - "Setting battery thresholds: start at {}%, stop at {}%", - start_threshold, stop_threshold + "Setting battery thresholds: start at {start_threshold}%, stop at {stop_threshold}%" ); - cpu::set_battery_charge_thresholds(start_threshold, stop_threshold) + battery::set_battery_charge_thresholds(start_threshold, stop_threshold) .map_err(|e| Box::new(e) as Box) } Some(Commands::Daemon { verbose }) => daemon::run_daemon(config, verbose), diff --git a/src/monitor.rs b/src/monitor.rs index 811f3e9..75ff1ce 100644 --- a/src/monitor.rs +++ b/src/monitor.rs @@ -2,6 +2,7 @@ use crate::config::AppConfig; use crate::core::{BatteryInfo, CpuCoreInfo, CpuGlobalInfo, SystemInfo, SystemLoad, SystemReport}; use crate::cpu::get_logical_core_count; use crate::util::error::SysMonitorError; +use log::debug; use std::{ collections::HashMap, fs, @@ -360,7 +361,7 @@ fn get_fallback_temperature(hw_path: &Path) -> Option { pub fn get_all_cpu_core_info() -> Result> { let initial_cpu_times = read_all_cpu_times()?; - thread::sleep(Duration::from_millis(250)); // Interval for CPU usage calculation + thread::sleep(Duration::from_millis(250)); // interval for CPU usage calculation let final_cpu_times = read_all_cpu_times()?; let num_cores = get_logical_core_count() @@ -503,7 +504,7 @@ pub fn get_battery_info(config: &AppConfig) -> Result> { } } } else if name.starts_with("AC") || name.contains("ACAD") || name.contains("ADP") { - // fallback for type file missing + // Fallback for type file missing if let Ok(online) = read_sysfs_value::(ps_path.join("online")) { if online == 1 { overall_ac_connected = true; @@ -513,6 +514,12 @@ pub fn get_battery_info(config: &AppConfig) -> Result> { } } + // No AC adapter detected but we're on a desktop system + // Default to AC power for desktops + if !overall_ac_connected { + overall_ac_connected = is_likely_desktop_system(); + } + for entry in fs::read_dir(power_supply_path)? { let entry = entry?; let ps_path = entry.path(); @@ -524,6 +531,12 @@ pub fn get_battery_info(config: &AppConfig) -> Result> { if let Ok(ps_type) = read_sysfs_file_trimmed(ps_path.join("type")) { if ps_type == "Battery" { + // Skip peripheral batteries that aren't real laptop batteries + if is_peripheral_battery(&ps_path, &name) { + debug!("Skipping peripheral battery: {name}"); + continue; + } + let status_str = read_sysfs_file_trimmed(ps_path.join("status")).ok(); let capacity_percent = read_sysfs_value::(ps_path.join("capacity")).ok(); @@ -564,9 +577,91 @@ pub fn get_battery_info(config: &AppConfig) -> Result> { } } } + + // If we found no batteries but have power supplies, we're likely on a desktop + if batteries.is_empty() && overall_ac_connected { + debug!("No laptop batteries found, likely a desktop system"); + } + Ok(batteries) } +/// Check if a battery is likely a peripheral (mouse, keyboard, etc) not a laptop battery +fn is_peripheral_battery(ps_path: &Path, name: &str) -> bool { + // Common peripheral battery names + if name.contains("mouse") + || name.contains("keyboard") + || name.contains("trackpad") + || name.contains("gamepad") + || name.contains("controller") + || name.contains("headset") + || name.contains("headphone") + { + return true; + } + + // Small capacity batteries are likely not laptop batteries + if let Ok(energy_full) = read_sysfs_value::(ps_path.join("energy_full")) { + // Most laptop batteries are at least 20,000,000 µWh (20 Wh) + // Peripheral batteries are typically much smaller + if energy_full < 10_000_000 { + // 10 Wh in µWh + return true; + } + } + + // Check for model name that indicates a peripheral + if let Ok(model) = read_sysfs_file_trimmed(ps_path.join("model_name")) { + if model.contains("bluetooth") || model.contains("wireless") { + return true; + } + } + + false +} + +/// Determine if this is likely a desktop system rather than a laptop +fn is_likely_desktop_system() -> bool { + // Check for DMI system type information + if let Ok(chassis_type) = fs::read_to_string("/sys/class/dmi/id/chassis_type") { + let chassis_type = chassis_type.trim(); + + // Chassis types: + // 3=Desktop, 4=Low Profile Desktop, 5=Pizza Box, 6=Mini Tower + // 7=Tower, 8=Portable, 9=Laptop, 10=Notebook, 11=Hand Held, 13=All In One + // 14=Sub Notebook, 15=Space-saving, 16=Lunch Box, 17=Main Server Chassis + match chassis_type { + "3" | "4" | "5" | "6" | "7" | "15" | "16" | "17" => return true, // desktop form factors + "9" | "10" | "14" => return false, // laptop form factors + _ => {} // Unknown, continue with other checks + } + } + + // Check CPU power policies, desktops often don't have these + let power_saving_exists = Path::new("/sys/module/intel_pstate/parameters/no_hwp").exists() + || Path::new("/sys/devices/system/cpu/cpufreq/conservative").exists(); + + if !power_saving_exists { + return true; // likely a desktop + } + + // Check battery-specific ACPI paths that laptops typically have + let laptop_acpi_paths = [ + "/sys/class/power_supply/BAT0", + "/sys/class/power_supply/BAT1", + "/proc/acpi/battery", + ]; + + for path in &laptop_acpi_paths { + if Path::new(path).exists() { + return false; // Likely a laptop + } + } + + // Default to assuming desktop if we can't determine + true +} + pub fn get_system_load() -> Result { let loadavg_str = read_sysfs_file_trimmed("/proc/loadavg")?; let parts: Vec<&str> = loadavg_str.split_whitespace().collect(); diff --git a/src/util/error.rs b/src/util/error.rs index 63a6628..51c3162 100644 --- a/src/util/error.rs +++ b/src/util/error.rs @@ -49,7 +49,6 @@ pub enum SysMonitorError { ReadError(String), ParseError(String), ProcStatParseError(String), - NotAvailable(String), } impl From for SysMonitorError { @@ -67,7 +66,6 @@ impl std::fmt::Display for SysMonitorError { Self::ProcStatParseError(s) => { write!(f, "Failed to parse /proc/stat: {s}") } - Self::NotAvailable(s) => write!(f, "Information not available: {s}"), } } } From 45b6672c641ff1d5a7f52fb7ee202acc461995c1 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Thu, 15 May 2025 20:10:23 +0300 Subject: [PATCH 05/40] battery: avoid manual flatten; capture each Err --- src/battery.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/battery.rs b/src/battery.rs index 4dd9be3..645eaa1 100644 --- a/src/battery.rs +++ b/src/battery.rs @@ -95,7 +95,14 @@ fn find_supported_batteries(power_supply_path: &Path) -> Result e, + Err(e) => { + warn!("Failed to read power-supply entry: {e}"); + continue; + } + }; let ps_path = entry.path(); if is_battery(&ps_path)? { if let Some(battery) = find_battery_with_threshold_support(&ps_path) { From b9cce7b634a520ac58587626998261ea05022bb1 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Thu, 15 May 2025 20:32:59 +0300 Subject: [PATCH 06/40] config: prefer a named struct over tuple for battery conf --- src/config/load.rs | 3 ++- src/config/types.rs | 19 +++++++++++++------ src/engine.rs | 7 ++++--- 3 files changed, 19 insertions(+), 10 deletions(-) diff --git a/src/config/load.rs b/src/config/load.rs index 470a99d..9679dc0 100644 --- a/src/config/load.rs +++ b/src/config/load.rs @@ -86,7 +86,8 @@ fn load_and_parse_config(path: &Path) -> Result { // If profile-specific battery thresholds are not set, inherit from global config if charger_profile.battery_charge_thresholds.is_none() { - charger_profile.battery_charge_thresholds = toml_app_config.battery_charge_thresholds; + charger_profile.battery_charge_thresholds = + toml_app_config.battery_charge_thresholds.clone(); } if battery_profile.battery_charge_thresholds.is_none() { diff --git a/src/config/types.rs b/src/config/types.rs index 52ef5c9..32c7c38 100644 --- a/src/config/types.rs +++ b/src/config/types.rs @@ -1,6 +1,12 @@ // Configuration types and structures for superfreq use crate::core::TurboSetting; -use serde::Deserialize; +use serde::{Deserialize, Serialize}; + +#[derive(Deserialize, Serialize, Debug, Clone)] +pub struct BatteryChargeThresholds { + pub start: u8, + pub stop: u8, +} // Structs for configuration using serde::Deserialize #[derive(Deserialize, Debug, Clone)] @@ -13,7 +19,8 @@ pub struct ProfileConfig { pub max_freq_mhz: Option, pub platform_profile: Option, pub turbo_auto_settings: Option, - pub battery_charge_thresholds: Option<(u8, u8)>, + #[serde(skip_serializing_if = "Option::is_none")] + pub battery_charge_thresholds: Option, } impl Default for ProfileConfig { @@ -87,7 +94,8 @@ pub struct ProfileConfigToml { pub min_freq_mhz: Option, pub max_freq_mhz: Option, pub platform_profile: Option, - pub battery_charge_thresholds: Option<(u8, u8)>, + #[serde(skip_serializing_if = "Option::is_none")] + pub battery_charge_thresholds: Option, } #[derive(Deserialize, Debug, Clone, Default)] @@ -96,10 +104,9 @@ pub struct AppConfigToml { pub charger: ProfileConfigToml, #[serde(default)] pub battery: ProfileConfigToml, - pub battery_charge_thresholds: Option<(u8, u8)>, + #[serde(skip_serializing_if = "Option::is_none")] + pub battery_charge_thresholds: Option, pub ignored_power_supplies: Option>, - #[serde(default = "default_poll_interval_sec")] - pub poll_interval_sec: u64, #[serde(default)] pub daemon: DaemonConfigToml, } diff --git a/src/engine.rs b/src/engine.rs index 67aa8b6..401d849 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -147,9 +147,10 @@ pub fn determine_and_apply_settings( } // Set battery charge thresholds if configured - if let Some((start_threshold, stop_threshold)) = - selected_profile_config.battery_charge_thresholds - { + if let Some(thresholds) = &selected_profile_config.battery_charge_thresholds { + let start_threshold = thresholds.start; + let stop_threshold = thresholds.stop; + if start_threshold < stop_threshold && stop_threshold <= 100 { info!("Setting battery charge thresholds: {start_threshold}-{stop_threshold}%"); match battery::set_battery_charge_thresholds(start_threshold, stop_threshold) { From 44fff2e273b631691da6650cab4bec7cf1dea7c5 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Thu, 15 May 2025 20:48:44 +0300 Subject: [PATCH 07/40] battery: treshold patterns to a constant array --- src/battery.rs | 60 +++++++++++++++++++++++--------------------------- 1 file changed, 27 insertions(+), 33 deletions(-) diff --git a/src/battery.rs b/src/battery.rs index 645eaa1..15823a9 100644 --- a/src/battery.rs +++ b/src/battery.rs @@ -16,6 +16,32 @@ pub struct ThresholdPathPattern { pub stop_path: &'static str, } +// Threshold patterns +const THRESHOLD_PATTERNS: &[ThresholdPathPattern] = &[ + ThresholdPathPattern { + description: "Standard", + start_path: "charge_control_start_threshold", + stop_path: "charge_control_end_threshold", + }, + ThresholdPathPattern { + description: "ASUS", + start_path: "charge_control_start_percentage", + stop_path: "charge_control_end_percentage", + }, + // Combine Huawei and ThinkPad since they use identical paths + ThresholdPathPattern { + description: "ThinkPad/Huawei", + start_path: "charge_start_threshold", + stop_path: "charge_stop_threshold", + }, + // Framework laptop support + ThresholdPathPattern { + description: "Framework", + start_path: "charge_behaviour_start_threshold", + stop_path: "charge_behaviour_end_threshold", + }, +]; + /// Represents a battery that supports charge threshold control pub struct SupportedBattery { pub name: String, @@ -144,39 +170,7 @@ fn write_sysfs_value(path: impl AsRef, value: &str) -> Result<()> { /// Identifies if a battery supports threshold control and which pattern it uses fn find_battery_with_threshold_support(ps_path: &Path) -> Option { - let threshold_paths = vec![ - ThresholdPathPattern { - description: "Standard", - start_path: "charge_control_start_threshold", - stop_path: "charge_control_end_threshold", - }, - ThresholdPathPattern { - description: "ASUS", - start_path: "charge_control_start_percentage", - stop_path: "charge_control_end_percentage", - }, - ThresholdPathPattern { - description: "Huawei", - start_path: "charge_start_threshold", - stop_path: "charge_stop_threshold", - }, - // ThinkPad-specific, sometimes used in addition to standard paths - ThresholdPathPattern { - description: "ThinkPad", - start_path: "charge_start_threshold", - stop_path: "charge_stop_threshold", - }, - // Framework laptop support - // FIXME: This needs actual testing. I inferred this behaviour from some - // Framework-specific code, but it may not be correct. - ThresholdPathPattern { - description: "Framework", - start_path: "charge_behaviour_start_threshold", - stop_path: "charge_behaviour_end_threshold", - }, - ]; - - for pattern in &threshold_paths { + for pattern in THRESHOLD_PATTERNS { let start_threshold_path = ps_path.join(pattern.start_path); let stop_threshold_path = ps_path.join(pattern.stop_path); if start_threshold_path.exists() && stop_threshold_path.exists() { From ded671296af5e5b05d8b95a08dd1dffd4f47f130 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Thu, 15 May 2025 21:03:07 +0300 Subject: [PATCH 08/40] engine: treat `InvalidValueError` same as `NotSupported` --- src/engine.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/engine.rs b/src/engine.rs index 401d849..f4b27ac 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -23,7 +23,9 @@ where match apply_fn() { Ok(_) => Ok(()), Err(e) => { - if matches!(e, ControlError::NotSupported(_)) { + if matches!(e, ControlError::NotSupported(_)) + || matches!(e, ControlError::InvalidValueError(_)) + { warn!( "{feature_name} setting is not supported on this system. Skipping {feature_name} configuration." ); From 759ba2a10ac4db2895ebec3a97f3aa42ff7558b8 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Thu, 15 May 2025 21:07:05 +0300 Subject: [PATCH 09/40] battery: tighten treshhold validation --- src/battery.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/battery.rs b/src/battery.rs index 15823a9..5fb41d4 100644 --- a/src/battery.rs +++ b/src/battery.rs @@ -92,6 +92,11 @@ pub fn set_battery_charge_thresholds(start_threshold: u8, stop_threshold: u8) -> /// Validates that the threshold values are in acceptable ranges fn validate_thresholds(start_threshold: u8, stop_threshold: u8) -> Result<()> { + if start_threshold == 0 || stop_threshold == 0 { + return Err(ControlError::InvalidValueError( + "Thresholds must be greater than 0%".to_string(), + )); + } if start_threshold >= stop_threshold { return Err(ControlError::InvalidValueError(format!( "Start threshold ({start_threshold}) must be less than stop threshold ({stop_threshold})" From b215afc9ddc8ac91355fb68589024728507181b7 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Thu, 15 May 2025 21:46:57 +0300 Subject: [PATCH 10/40] config: streamline treshold validation --- src/battery.rs | 30 +++++------------------------- src/config/mod.rs | 11 ++++------- src/config/types.rs | 28 ++++++++++++++++++++++++++++ 3 files changed, 37 insertions(+), 32 deletions(-) diff --git a/src/battery.rs b/src/battery.rs index 5fb41d4..4eb9ebb 100644 --- a/src/battery.rs +++ b/src/battery.rs @@ -1,4 +1,4 @@ -use crate::util::error::ControlError; +use crate::{config::types::BatteryChargeThresholds, util::error::ControlError}; use log::{debug, warn}; use std::{ fs, io, @@ -71,7 +71,9 @@ pub struct SupportedBattery { /// - No batteries with threshold support are found /// - Failed to set thresholds on any battery pub fn set_battery_charge_thresholds(start_threshold: u8, stop_threshold: u8) -> Result<()> { - validate_thresholds(start_threshold, stop_threshold)?; + // Validate thresholds using `BatteryChargeThresholds` + let thresholds = BatteryChargeThresholds::new(start_threshold, stop_threshold) + .map_err(|e| ControlError::InvalidValueError(e))?; let power_supply_path = Path::new("/sys/class/power_supply"); if !power_supply_path.exists() { @@ -87,29 +89,7 @@ pub fn set_battery_charge_thresholds(start_threshold: u8, stop_threshold: u8) -> )); } - apply_thresholds_to_batteries(&supported_batteries, start_threshold, stop_threshold) -} - -/// Validates that the threshold values are in acceptable ranges -fn validate_thresholds(start_threshold: u8, stop_threshold: u8) -> Result<()> { - if start_threshold == 0 || stop_threshold == 0 { - return Err(ControlError::InvalidValueError( - "Thresholds must be greater than 0%".to_string(), - )); - } - if start_threshold >= stop_threshold { - return Err(ControlError::InvalidValueError(format!( - "Start threshold ({start_threshold}) must be less than stop threshold ({stop_threshold})" - ))); - } - - if stop_threshold > 100 { - return Err(ControlError::InvalidValueError(format!( - "Stop threshold ({stop_threshold}) cannot exceed 100%" - ))); - } - - Ok(()) + apply_thresholds_to_batteries(&supported_batteries, thresholds.start, thresholds.stop) } /// Finds all batteries in the system that support threshold control diff --git a/src/config/mod.rs b/src/config/mod.rs index b386b52..0a20a83 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -1,9 +1,6 @@ +pub mod load; +pub mod types; pub mod watcher; -// Re-export all configuration types and functions -pub use self::load::*; -pub use self::types::*; - -// Internal organization of config submodules -mod load; -mod types; +pub use load::*; +pub use types::*; diff --git a/src/config/types.rs b/src/config/types.rs index 32c7c38..742ff78 100644 --- a/src/config/types.rs +++ b/src/config/types.rs @@ -1,6 +1,7 @@ // Configuration types and structures for superfreq use crate::core::TurboSetting; use serde::{Deserialize, Serialize}; +use std::convert::TryFrom; #[derive(Deserialize, Serialize, Debug, Clone)] pub struct BatteryChargeThresholds { @@ -8,6 +9,33 @@ pub struct BatteryChargeThresholds { pub stop: u8, } +impl BatteryChargeThresholds { + pub fn new(start: u8, stop: u8) -> Result { + if start == 0 || stop == 0 { + return Err("Thresholds must be greater than 0%".to_string()); + } + if start >= stop { + return Err(format!( + "Start threshold ({start}) must be less than stop threshold ({stop})" + )); + } + if stop > 100 { + return Err(format!("Stop threshold ({stop}) cannot exceed 100%")); + } + + Ok(Self { start, stop }) + } +} + +impl TryFrom<(u8, u8)> for BatteryChargeThresholds { + type Error = String; + + fn try_from(values: (u8, u8)) -> Result { + let (start, stop) = values; + Self::new(start, stop) + } +} + // Structs for configuration using serde::Deserialize #[derive(Deserialize, Debug, Clone)] pub struct ProfileConfig { From 620dc48be0cc7411927d5316db79b3bf138f431a Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Thu, 15 May 2025 22:01:45 +0300 Subject: [PATCH 11/40] engine: apply governor override via `try_apply_feature` --- src/engine.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/engine.rs b/src/engine.rs index f4b27ac..afa4aba 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -51,8 +51,10 @@ pub fn determine_and_apply_settings( override_governor.trim() ); - // Apply the override governor setting - validation is handled by set_governor - cpu::set_governor(override_governor.trim(), None)?; + // Apply the override governor setting + try_apply_feature("override governor", override_governor.trim(), || { + cpu::set_governor(override_governor.trim(), None) + })?; } let selected_profile_config: &ProfileConfig; From 7c34ef5aa0de96af05ee7d84d8b356a29f978fb2 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Thu, 15 May 2025 22:07:15 +0300 Subject: [PATCH 12/40] engine: try to handle systems with multiple batteries --- src/battery.rs | 2 +- src/engine.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/battery.rs b/src/battery.rs index 4eb9ebb..2f7c43a 100644 --- a/src/battery.rs +++ b/src/battery.rs @@ -73,7 +73,7 @@ pub struct SupportedBattery { pub fn set_battery_charge_thresholds(start_threshold: u8, stop_threshold: u8) -> Result<()> { // Validate thresholds using `BatteryChargeThresholds` let thresholds = BatteryChargeThresholds::new(start_threshold, stop_threshold) - .map_err(|e| ControlError::InvalidValueError(e))?; + .map_err(ControlError::InvalidValueError)?; let power_supply_path = Path::new("/sys/class/power_supply"); if !power_supply_path.exists() { diff --git a/src/engine.rs b/src/engine.rs index afa4aba..fa50124 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -78,8 +78,8 @@ pub fn determine_and_apply_settings( // No batteries means desktop/server, always on AC true } else { - // At least one battery exists, check if it's on AC - report.batteries.first().is_some_and(|b| b.ac_connected) + // Check if any battery reports AC connected + report.batteries.iter().any(|b| b.ac_connected) }; if on_ac_power { From 705fc6c46d0bc893f9c343f9e0ed55f60c95af6a Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Thu, 15 May 2025 22:20:02 +0300 Subject: [PATCH 13/40] battery: use a reference to `SupportedBattery` instead of clones --- src/battery.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/battery.rs b/src/battery.rs index 2f7c43a..3fa35e4 100644 --- a/src/battery.rs +++ b/src/battery.rs @@ -43,9 +43,9 @@ const THRESHOLD_PATTERNS: &[ThresholdPathPattern] = &[ ]; /// Represents a battery that supports charge threshold control -pub struct SupportedBattery { +pub struct SupportedBattery<'a> { pub name: String, - pub pattern: ThresholdPathPattern, + pub pattern: &'a ThresholdPathPattern, pub path: PathBuf, } @@ -93,7 +93,7 @@ pub fn set_battery_charge_thresholds(start_threshold: u8, stop_threshold: u8) -> } /// Finds all batteries in the system that support threshold control -fn find_supported_batteries(power_supply_path: &Path) -> Result> { +fn find_supported_batteries(power_supply_path: &Path) -> Result>> { let entries = fs::read_dir(power_supply_path).map_err(|e| { if e.kind() == io::ErrorKind::PermissionDenied { ControlError::PermissionDenied(format!( @@ -154,14 +154,14 @@ fn write_sysfs_value(path: impl AsRef, value: &str) -> Result<()> { } /// Identifies if a battery supports threshold control and which pattern it uses -fn find_battery_with_threshold_support(ps_path: &Path) -> Option { +fn find_battery_with_threshold_support(ps_path: &Path) -> Option> { for pattern in THRESHOLD_PATTERNS { let start_threshold_path = ps_path.join(pattern.start_path); let stop_threshold_path = ps_path.join(pattern.stop_path); if start_threshold_path.exists() && stop_threshold_path.exists() { return Some(SupportedBattery { name: ps_path.file_name()?.to_string_lossy().to_string(), - pattern: pattern.clone(), + pattern, path: ps_path.to_path_buf(), }); } @@ -171,7 +171,7 @@ fn find_battery_with_threshold_support(ps_path: &Path) -> Option], start_threshold: u8, stop_threshold: u8, ) -> Result<()> { From b11bad74d5af58c87354c1ca243a1d4f4e33b7ec Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Thu, 15 May 2025 22:54:02 +0300 Subject: [PATCH 14/40] battery: perform permission checks upfront --- src/battery.rs | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/src/battery.rs b/src/battery.rs index 3fa35e4..ec390cb 100644 --- a/src/battery.rs +++ b/src/battery.rs @@ -82,6 +82,14 @@ pub fn set_battery_charge_thresholds(start_threshold: u8, stop_threshold: u8) -> )); } + // Check if the power supply directory is writable + // This helps identify permission issues in containerized environments early + if !is_path_writable(power_supply_path) { + return Err(ControlError::PermissionDenied( + "Power supply path exists but is not writable. This may occur in containerized environments.".to_string(), + )); + } + let supported_batteries = find_supported_batteries(power_supply_path)?; if supported_batteries.is_empty() { return Err(ControlError::NotSupported( @@ -240,3 +248,24 @@ fn is_battery(path: &Path) -> Result { Ok(ps_type == "Battery") } + +/// Check if a directory is writable by attempting to open a temporary file for writing +fn is_path_writable(path: &Path) -> bool { + use std::fs::OpenOptions; + + // Try to create a temporary file to check write permissions + // If we're in a container with a read-only mount, this will fail + let temp_file_path = path.join(".superfreq_write_test"); + let result = OpenOptions::new() + .write(true) + .create(true) + .open(&temp_file_path); + + // Clean up the temporary file if we created it + if result.is_ok() { + let _ = fs::remove_file(temp_file_path); + true + } else { + false + } +} From 90fd077a6736517136baffc03c1d93e8f56471d5 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Thu, 15 May 2025 22:58:47 +0300 Subject: [PATCH 15/40] config: make `ProfileConfig` serialisable --- src/config/types.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/config/types.rs b/src/config/types.rs index 742ff78..988170f 100644 --- a/src/config/types.rs +++ b/src/config/types.rs @@ -37,7 +37,7 @@ impl TryFrom<(u8, u8)> for BatteryChargeThresholds { } // Structs for configuration using serde::Deserialize -#[derive(Deserialize, Debug, Clone)] +#[derive(Deserialize, Serialize, Debug, Clone)] pub struct ProfileConfig { pub governor: Option, pub turbo: Option, @@ -67,7 +67,7 @@ impl Default for ProfileConfig { } } -#[derive(Deserialize, Debug, Default, Clone)] +#[derive(Deserialize, Serialize, Debug, Default, Clone)] pub struct AppConfig { #[serde(default)] pub charger: ProfileConfig, @@ -212,7 +212,7 @@ impl From for ProfileConfig { } } -#[derive(Deserialize, Debug, Clone)] +#[derive(Deserialize, Serialize, Debug, Clone)] pub struct DaemonConfig { #[serde(default = "default_poll_interval_sec")] pub poll_interval_sec: u64, @@ -261,7 +261,7 @@ const fn default_adaptive_interval() -> bool { } const fn default_min_poll_interval_sec() -> u64 { - 1 + 1 } const fn default_max_poll_interval_sec() -> u64 { From 6fe322272eeb60d0a37a7e04fcc99f93c604a12e Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Thu, 15 May 2025 23:20:08 +0300 Subject: [PATCH 16/40] core: validate tresholds at the CLI level --- src/config/types.rs | 6 +++--- src/core.rs | 4 ++-- src/main.rs | 30 +++++++++++++++++++++++++----- 3 files changed, 30 insertions(+), 10 deletions(-) diff --git a/src/config/types.rs b/src/config/types.rs index 988170f..500320f 100644 --- a/src/config/types.rs +++ b/src/config/types.rs @@ -154,7 +154,7 @@ impl Default for ProfileConfigToml { } } -#[derive(Deserialize, Debug, Clone)] +#[derive(Deserialize, Serialize, Debug, Clone)] pub struct TurboAutoSettings { #[serde(default = "default_load_threshold_high")] pub load_threshold_high: f32, @@ -230,7 +230,7 @@ pub struct DaemonConfig { pub stats_file_path: Option, } -#[derive(Deserialize, Debug, Clone, Copy, PartialEq, Eq)] +#[derive(Deserialize, Serialize, Debug, Clone, Copy, PartialEq, Eq)] pub enum LogLevel { Error, Warning, @@ -261,7 +261,7 @@ const fn default_adaptive_interval() -> bool { } const fn default_min_poll_interval_sec() -> u64 { - 1 + 1 } const fn default_max_poll_interval_sec() -> u64 { diff --git a/src/core.rs b/src/core.rs index 2be64ff..76dc940 100644 --- a/src/core.rs +++ b/src/core.rs @@ -1,8 +1,8 @@ use clap::ValueEnum; -use serde::Deserialize; +use serde::{Deserialize, Serialize}; use std::fmt; -#[derive(Debug, Clone, Copy, PartialEq, Eq, Deserialize, ValueEnum)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Deserialize, Serialize, ValueEnum)] pub enum TurboSetting { Always, // turbo is forced on (if possible) Auto, // system or driver controls turbo diff --git a/src/main.rs b/src/main.rs index dc9bf6a..3755929 100644 --- a/src/main.rs +++ b/src/main.rs @@ -370,11 +370,31 @@ fn main() { start_threshold, stop_threshold, }) => { - info!( - "Setting battery thresholds: start at {start_threshold}%, stop at {stop_threshold}%" - ); - battery::set_battery_charge_thresholds(start_threshold, stop_threshold) - .map_err(|e| Box::new(e) as Box) + // Basic validation to provide proper error messages at the CLI level + if start_threshold >= stop_threshold { + error!( + "Start threshold ({start_threshold}) must be less than stop threshold ({stop_threshold})" + ); + Err(Box::new(ControlError::InvalidValueError(format!( + "Start threshold ({start_threshold}) must be less than stop threshold ({stop_threshold})" + ))) as Box) + } else if stop_threshold > 100 { + error!("Stop threshold ({stop_threshold}) cannot exceed 100%"); + Err(Box::new(ControlError::InvalidValueError(format!( + "Stop threshold ({stop_threshold}) cannot exceed 100%" + ))) as Box) + } else if start_threshold == 0 || stop_threshold == 0 { + error!("Thresholds must be greater than 0%"); + Err(Box::new(ControlError::InvalidValueError( + "Thresholds must be greater than 0%".to_string(), + )) as Box) + } else { + info!( + "Setting battery thresholds: start at {start_threshold}%, stop at {stop_threshold}%" + ); + battery::set_battery_charge_thresholds(start_threshold, stop_threshold) + .map_err(|e| Box::new(e) as Box) + } } Some(Commands::Daemon { verbose }) => daemon::run_daemon(config, verbose), Some(Commands::Debug) => cli::debug::run_debug(&config), From 7b6602ad39c0df9670fea622561d2bb22f34760a Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 16 May 2025 00:01:38 +0300 Subject: [PATCH 17/40] docs: mention profile-specific tresholds taking precedence --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 2ddf235..b9f3662 100644 --- a/README.md +++ b/README.md @@ -176,6 +176,7 @@ max_freq_mhz = 2500 # Global battery charging thresholds (applied to both profiles unless overridden) # Start charging at 40%, stop at 80% - extends battery lifespan +# NOTE: Profile-specific thresholds (in [charger] or [battery] sections) take precedence over this global setting battery_charge_thresholds = [40, 80] # Daemon configuration From d8f609fdefb5817c6233d38086e31cb0a0bb4b3c Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 16 May 2025 00:02:09 +0300 Subject: [PATCH 18/40] {core,battery}: better input validation; path, permission and sanity checks - battery: add path existence and writability check for threshold support - cpu: validate governor and EPP values, and add frequency validation - engine: validate turbo auto settings for proper configuration - main: add validation for minimum and maximum frequency inputs --- src/battery.rs | 19 ++++- src/cpu.rs | 194 ++++++++++++++++++++++++++++++++++++++++++++++++- src/engine.rs | 40 +++++++--- src/main.rs | 67 +++++++++++++++-- 4 files changed, 303 insertions(+), 17 deletions(-) diff --git a/src/battery.rs b/src/battery.rs index ec390cb..f167ab0 100644 --- a/src/battery.rs +++ b/src/battery.rs @@ -161,12 +161,29 @@ fn write_sysfs_value(path: impl AsRef, value: &str) -> Result<()> { }) } +/// Safely check if a path exists and is writable +fn path_exists_and_writable(path: &Path) -> bool { + if !path.exists() { + return false; + } + + // Try to open the file with write access to verify write permission + match fs::OpenOptions::new().write(true).open(path) { + Ok(_) => true, + Err(_) => false, + } +} + /// Identifies if a battery supports threshold control and which pattern it uses fn find_battery_with_threshold_support(ps_path: &Path) -> Option> { for pattern in THRESHOLD_PATTERNS { let start_threshold_path = ps_path.join(pattern.start_path); let stop_threshold_path = ps_path.join(pattern.stop_path); - if start_threshold_path.exists() && stop_threshold_path.exists() { + + // Ensure both paths exist and are writable before considering this battery supported + if path_exists_and_writable(&start_threshold_path) + && path_exists_and_writable(&stop_threshold_path) + { return Some(SupportedBattery { name: ps_path.file_name()?.to_string_lossy().to_string(), pattern, diff --git a/src/cpu.rs b/src/cpu.rs index 62d0dd1..fb35a9a 100644 --- a/src/cpu.rs +++ b/src/cpu.rs @@ -79,6 +79,15 @@ where } pub fn set_governor(governor: &str, core_id: Option) -> Result<()> { + // Validate the governor is available on this system + if !is_governor_valid(governor)? { + return Err(ControlError::InvalidValueError(format!( + "Governor '{}' is not available on this system. Valid governors: {}", + governor, + get_available_governors()?.join(", ") + ))); + } + let action = |id: u32| { let path = format!("/sys/devices/system/cpu/cpu{id}/cpufreq/scaling_governor"); if Path::new(&path).exists() { @@ -93,6 +102,32 @@ pub fn set_governor(governor: &str, core_id: Option) -> Result<()> { core_id.map_or_else(|| for_each_cpu_core(action), action) } +/// Check if the provided governor is available in the system +fn is_governor_valid(governor: &str) -> Result { + let governors = get_available_governors()?; + Ok(governors.contains(&governor.to_string())) +} + +/// Get available CPU governors from the system +fn get_available_governors() -> Result> { + // We'll check cpu0's available governors as they're typically the same across cores + let path = "/sys/devices/system/cpu/cpu0/cpufreq/scaling_available_governors"; + + if !Path::new(path).exists() { + return Err(ControlError::NotSupported( + "Could not determine available governors".to_string(), + )); + } + + let content = fs::read_to_string(path) + .map_err(|e| ControlError::ReadError(format!("Failed to read available governors: {e}")))?; + + Ok(content + .split_whitespace() + .map(ToString::to_string) + .collect()) +} + pub fn set_turbo(setting: TurboSetting) -> Result<()> { let value_pstate = match setting { TurboSetting::Always => "0", // no_turbo = 0 means turbo is enabled @@ -153,6 +188,16 @@ fn try_set_per_core_boost(value: &str) -> Result { } pub fn set_epp(epp: &str, core_id: Option) -> Result<()> { + // Validate the EPP value against available options + let available_epp = get_available_epp_values()?; + if !available_epp.contains(&epp.to_string()) { + return Err(ControlError::InvalidValueError(format!( + "Invalid EPP value: '{}'. Available values: {}", + epp, + available_epp.join(", ") + ))); + } + let action = |id: u32| { let path = format!("/sys/devices/system/cpu/cpu{id}/cpufreq/energy_performance_preference"); if Path::new(&path).exists() { @@ -164,8 +209,37 @@ pub fn set_epp(epp: &str, core_id: Option) -> Result<()> { core_id.map_or_else(|| for_each_cpu_core(action), action) } +/// Get available EPP values from the system +fn get_available_epp_values() -> Result> { + let path = "/sys/devices/system/cpu/cpu0/cpufreq/energy_performance_available_preferences"; + + if !Path::new(path).exists() { + // If the file doesn't exist, fall back to a default set of common values + // This is safer than failing outright, as some systems may allow these values + // even without explicitly listing them + return Ok(vec![ + "default".to_string(), + "performance".to_string(), + "balance_performance".to_string(), + "balance_power".to_string(), + "power".to_string(), + ]); + } + + let content = fs::read_to_string(path).map_err(|e| { + ControlError::ReadError(format!("Failed to read available EPP values: {e}")) + })?; + + Ok(content + .split_whitespace() + .map(ToString::to_string) + .collect()) +} + pub fn set_epb(epb: &str, core_id: Option) -> Result<()> { - // EPB is often an integer 0-15. + // Validate EPB value - should be a number 0-15 or a recognized string value + validate_epb_value(epb)?; + let action = |id: u32| { let path = format!("/sys/devices/system/cpu/cpu{id}/cpufreq/energy_performance_bias"); if Path::new(&path).exists() { @@ -177,7 +251,54 @@ pub fn set_epb(epb: &str, core_id: Option) -> Result<()> { core_id.map_or_else(|| for_each_cpu_core(action), action) } +fn validate_epb_value(epb: &str) -> Result<()> { + // EPB can be a number from 0-15 or a recognized string + // Try parsing as a number first + if let Ok(value) = epb.parse::() { + if value <= 15 { + return Ok(()); + } else { + return Err(ControlError::InvalidValueError(format!( + "EPB numeric value must be between 0 and 15, got {}", + value + ))); + } + } + + // If not a number, check if it's a recognized string value + let valid_strings = [ + "performance", + "balance-performance", + "balance-power", + "power", + // Alternative forms + "balance_performance", + "balance_power", + ]; + + if valid_strings.contains(&epb) { + Ok(()) + } else { + Err(ControlError::InvalidValueError(format!( + "Invalid EPB value: '{}'. Must be a number 0-15 or one of: {}", + epb, + valid_strings.join(", ") + ))) + } +} + pub fn set_min_frequency(freq_mhz: u32, core_id: Option) -> Result<()> { + // Check if the new minimum frequency would be greater than current maximum + if let Some(id) = core_id { + validate_min_frequency(id, freq_mhz)?; + } else { + // Check for all cores + let num_cores = get_logical_core_count()?; + for id in 0..num_cores { + validate_min_frequency(id, freq_mhz)?; + } + } + let freq_khz_str = (freq_mhz * 1000).to_string(); let action = |id: u32| { let path = format!("/sys/devices/system/cpu/cpu{id}/cpufreq/scaling_min_freq"); @@ -191,6 +312,17 @@ pub fn set_min_frequency(freq_mhz: u32, core_id: Option) -> Result<()> { } pub fn set_max_frequency(freq_mhz: u32, core_id: Option) -> Result<()> { + // Check if the new maximum frequency would be less than current minimum + if let Some(id) = core_id { + validate_max_frequency(id, freq_mhz)?; + } else { + // Check for all cores + let num_cores = get_logical_core_count()?; + for id in 0..num_cores { + validate_max_frequency(id, freq_mhz)?; + } + } + let freq_khz_str = (freq_mhz * 1000).to_string(); let action = |id: u32| { let path = format!("/sys/devices/system/cpu/cpu{id}/cpufreq/scaling_max_freq"); @@ -203,6 +335,66 @@ pub fn set_max_frequency(freq_mhz: u32, core_id: Option) -> Result<()> { core_id.map_or_else(|| for_each_cpu_core(action), action) } +fn read_sysfs_value_as_u32(path: &str) -> Result { + if !Path::new(path).exists() { + return Err(ControlError::NotSupported(format!( + "File does not exist: {path}" + ))); + } + + let content = fs::read_to_string(path) + .map_err(|e| ControlError::ReadError(format!("Failed to read {path}: {e}")))?; + + content + .trim() + .parse::() + .map_err(|e| ControlError::ReadError(format!("Failed to parse value from {path}: {e}"))) +} + +fn validate_min_frequency(core_id: u32, new_min_freq_mhz: u32) -> Result<()> { + let max_freq_path = format!("/sys/devices/system/cpu/cpu{core_id}/cpufreq/scaling_max_freq"); + + if !Path::new(&max_freq_path).exists() { + return Ok(()); + } + + let max_freq_khz = read_sysfs_value_as_u32(&max_freq_path)?; + let new_min_freq_khz = new_min_freq_mhz * 1000; + + if new_min_freq_khz > max_freq_khz { + return Err(ControlError::InvalidValueError(format!( + "Minimum frequency ({} MHz) cannot be higher than maximum frequency ({} MHz) for core {}", + new_min_freq_mhz, + max_freq_khz / 1000, + core_id + ))); + } + + Ok(()) +} + +fn validate_max_frequency(core_id: u32, new_max_freq_mhz: u32) -> Result<()> { + let min_freq_path = format!("/sys/devices/system/cpu/cpu{core_id}/cpufreq/scaling_min_freq"); + + if !Path::new(&min_freq_path).exists() { + return Ok(()); + } + + let min_freq_khz = read_sysfs_value_as_u32(&min_freq_path)?; + let new_max_freq_khz = new_max_freq_mhz * 1000; + + if new_max_freq_khz < min_freq_khz { + return Err(ControlError::InvalidValueError(format!( + "Maximum frequency ({} MHz) cannot be lower than minimum frequency ({} MHz) for core {}", + new_max_freq_mhz, + min_freq_khz / 1000, + core_id + ))); + } + + Ok(()) +} + /// Sets the platform profile. /// This changes the system performance, temperature, fan, and other hardware replated characteristics. /// diff --git a/src/engine.rs b/src/engine.rs index fa50124..7001871 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -1,5 +1,5 @@ use crate::battery; -use crate::config::{AppConfig, ProfileConfig}; +use crate::config::{AppConfig, ProfileConfig, TurboAutoSettings}; use crate::core::{OperationalMode, SystemReport, TurboSetting}; use crate::cpu::{self}; use crate::util::error::{ControlError, EngineError}; @@ -177,6 +177,9 @@ fn manage_auto_turbo(report: &SystemReport, config: &ProfileConfig) -> Result<() // Get the auto turbo settings from the config, or use defaults let turbo_settings = config.turbo_auto_settings.clone().unwrap_or_default(); + // Validate the complete configuration to ensure it's usable + validate_turbo_auto_settings(&turbo_settings)?; + // Get average CPU temperature and CPU load let cpu_temp = report.cpu_global.average_temperature_celsius; @@ -202,14 +205,6 @@ fn manage_auto_turbo(report: &SystemReport, config: &ProfileConfig) -> Result<() } }; - // Validate the configuration to ensure it's usable - if turbo_settings.load_threshold_high <= turbo_settings.load_threshold_low { - return Err(EngineError::ConfigurationError( - "Invalid turbo auto settings: high threshold must be greater than low threshold" - .to_string(), - )); - } - // Decision logic for enabling/disabling turbo let enable_turbo = match (cpu_temp, avg_cpu_usage) { // If temperature is too high, disable turbo regardless of load @@ -262,3 +257,30 @@ fn manage_auto_turbo(report: &SystemReport, config: &ProfileConfig) -> Result<() Err(e) => Err(EngineError::ControlError(e)), } } + +fn validate_turbo_auto_settings(settings: &TurboAutoSettings) -> Result<(), EngineError> { + // Validate load thresholds + if settings.load_threshold_high <= settings.load_threshold_low { + return Err(EngineError::ConfigurationError( + "Invalid turbo auto settings: high threshold must be greater than low threshold" + .to_string(), + )); + } + + // Validate range of load thresholds (should be 0-100%) + if settings.load_threshold_high > 100.0 || settings.load_threshold_low < 0.0 { + return Err(EngineError::ConfigurationError( + "Invalid turbo auto settings: load thresholds must be between 0% and 100%".to_string(), + )); + } + + // Validate temperature threshold (realistic range for CPU temps in Celsius) + if settings.temp_threshold_high <= 0.0 || settings.temp_threshold_high > 110.0 { + return Err(EngineError::ConfigurationError( + "Invalid turbo auto settings: temperature threshold must be between 0°C and 110°C" + .to_string(), + )); + } + + Ok(()) +} diff --git a/src/main.rs b/src/main.rs index 3755929..e5f6fba 100644 --- a/src/main.rs +++ b/src/main.rs @@ -357,15 +357,70 @@ fn main() { cpu::set_epb(&epb, core_id).map_err(|e| Box::new(e) as Box) } Some(Commands::SetMinFreq { freq_mhz, core_id }) => { - cpu::set_min_frequency(freq_mhz, core_id) - .map_err(|e| Box::new(e) as Box) + // Basic validation for reasonable CPU frequency values + if freq_mhz == 0 { + error!("Minimum frequency cannot be zero"); + Err(Box::new(ControlError::InvalidValueError( + "Minimum frequency cannot be zero".to_string(), + )) as Box) + } else if freq_mhz > 10000 { + // Extremely high value unlikely to be valid + error!("Minimum frequency ({freq_mhz} MHz) is unreasonably high"); + Err(Box::new(ControlError::InvalidValueError(format!( + "Minimum frequency ({freq_mhz} MHz) is unreasonably high" + ))) as Box) + } else { + cpu::set_min_frequency(freq_mhz, core_id) + .map_err(|e| Box::new(e) as Box) + } } Some(Commands::SetMaxFreq { freq_mhz, core_id }) => { - cpu::set_max_frequency(freq_mhz, core_id) - .map_err(|e| Box::new(e) as Box) + // Basic validation for reasonable CPU frequency values + if freq_mhz == 0 { + error!("Maximum frequency cannot be zero"); + Err(Box::new(ControlError::InvalidValueError( + "Maximum frequency cannot be zero".to_string(), + )) as Box) + } else if freq_mhz > 10000 { + // Extremely high value unlikely to be valid + error!("Maximum frequency ({freq_mhz} MHz) is unreasonably high"); + Err(Box::new(ControlError::InvalidValueError(format!( + "Maximum frequency ({freq_mhz} MHz) is unreasonably high" + ))) as Box) + } else { + cpu::set_max_frequency(freq_mhz, core_id) + .map_err(|e| Box::new(e) as Box) + } + } + Some(Commands::SetPlatformProfile { profile }) => { + // Get available platform profiles and validate early if possible + match cpu::get_platform_profiles() { + Ok(available_profiles) => { + if !available_profiles.contains(&profile) { + error!( + "Invalid platform profile: '{}'. Available profiles: {}", + profile, + available_profiles.join(", ") + ); + Err(Box::new(ControlError::InvalidProfile(format!( + "Invalid platform profile: '{}'. Available profiles: {}", + profile, + available_profiles.join(", ") + ))) as Box) + } else { + info!("Setting platform profile to '{}'", profile); + cpu::set_platform_profile(&profile) + .map_err(|e| Box::new(e) as Box) + } + } + Err(_) => { + // If we can't get profiles (e.g., feature not supported), pass through to the function + // which will provide appropriate error + cpu::set_platform_profile(&profile) + .map_err(|e| Box::new(e) as Box) + } + } } - Some(Commands::SetPlatformProfile { profile }) => cpu::set_platform_profile(&profile) - .map_err(|e| Box::new(e) as Box), Some(Commands::SetBatteryThresholds { start_threshold, stop_threshold, From eb97689bc7a941cb760fa12c154ff871f77416a6 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 16 May 2025 00:18:46 +0300 Subject: [PATCH 19/40] battery: simplify file permission check and improve threshold inheritance --- src/battery.rs | 5 +---- src/config/load.rs | 17 ++++++++++------- src/cpu.rs | 8 +++----- src/main.rs | 10 +++++----- 4 files changed, 19 insertions(+), 21 deletions(-) diff --git a/src/battery.rs b/src/battery.rs index f167ab0..4d05bbc 100644 --- a/src/battery.rs +++ b/src/battery.rs @@ -168,10 +168,7 @@ fn path_exists_and_writable(path: &Path) -> bool { } // Try to open the file with write access to verify write permission - match fs::OpenOptions::new().write(true).open(path) { - Ok(_) => true, - Err(_) => false, - } + fs::OpenOptions::new().write(true).open(path).is_ok() } /// Identifies if a battery supports threshold control and which pattern it uses diff --git a/src/config/load.rs b/src/config/load.rs index 9679dc0..663f72e 100644 --- a/src/config/load.rs +++ b/src/config/load.rs @@ -84,14 +84,17 @@ fn load_and_parse_config(path: &Path) -> Result { let mut charger_profile = toml_app_config.charger.clone(); let mut battery_profile = toml_app_config.battery.clone(); - // If profile-specific battery thresholds are not set, inherit from global config - if charger_profile.battery_charge_thresholds.is_none() { - charger_profile.battery_charge_thresholds = - toml_app_config.battery_charge_thresholds.clone(); - } + // Clone global battery_charge_thresholds once if it exists + if let Some(global_thresholds) = toml_app_config.battery_charge_thresholds { + // Apply to charger profile if not already set + if charger_profile.battery_charge_thresholds.is_none() { + charger_profile.battery_charge_thresholds = Some(global_thresholds.clone()); + } - if battery_profile.battery_charge_thresholds.is_none() { - battery_profile.battery_charge_thresholds = toml_app_config.battery_charge_thresholds; + // Apply to battery profile if not already set + if battery_profile.battery_charge_thresholds.is_none() { + battery_profile.battery_charge_thresholds = Some(global_thresholds); + } } // Convert AppConfigToml to AppConfig diff --git a/src/cpu.rs b/src/cpu.rs index fb35a9a..075e520 100644 --- a/src/cpu.rs +++ b/src/cpu.rs @@ -257,12 +257,10 @@ fn validate_epb_value(epb: &str) -> Result<()> { if let Ok(value) = epb.parse::() { if value <= 15 { return Ok(()); - } else { - return Err(ControlError::InvalidValueError(format!( - "EPB numeric value must be between 0 and 15, got {}", - value - ))); } + return Err(ControlError::InvalidValueError(format!( + "EPB numeric value must be between 0 and 15, got {value}" + ))); } // If not a number, check if it's a recognized string value diff --git a/src/main.rs b/src/main.rs index e5f6fba..de2cbcc 100644 --- a/src/main.rs +++ b/src/main.rs @@ -396,7 +396,11 @@ fn main() { // Get available platform profiles and validate early if possible match cpu::get_platform_profiles() { Ok(available_profiles) => { - if !available_profiles.contains(&profile) { + if available_profiles.contains(&profile) { + info!("Setting platform profile to '{profile}'"); + cpu::set_platform_profile(&profile) + .map_err(|e| Box::new(e) as Box) + } else { error!( "Invalid platform profile: '{}'. Available profiles: {}", profile, @@ -407,10 +411,6 @@ fn main() { profile, available_profiles.join(", ") ))) as Box) - } else { - info!("Setting platform profile to '{}'", profile); - cpu::set_platform_profile(&profile) - .map_err(|e| Box::new(e) as Box) } } Err(_) => { From 8252f0d74e83ef744ced822e4c6b333bbf0ea6c5 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 16 May 2025 00:42:51 +0300 Subject: [PATCH 20/40] config: better validation of battery charge threshold; update error handling --- src/battery.rs | 9 +++++++-- src/config/types.rs | 24 +++++++++++++++--------- 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/src/battery.rs b/src/battery.rs index 4d05bbc..296096f 100644 --- a/src/battery.rs +++ b/src/battery.rs @@ -72,8 +72,13 @@ pub struct SupportedBattery<'a> { /// - Failed to set thresholds on any battery pub fn set_battery_charge_thresholds(start_threshold: u8, stop_threshold: u8) -> Result<()> { // Validate thresholds using `BatteryChargeThresholds` - let thresholds = BatteryChargeThresholds::new(start_threshold, stop_threshold) - .map_err(ControlError::InvalidValueError)?; + let thresholds = + BatteryChargeThresholds::new(start_threshold, stop_threshold).map_err(|e| match e { + crate::config::types::ConfigError::ValidationError(msg) => { + ControlError::InvalidValueError(msg) + } + _ => ControlError::InvalidValueError(format!("Invalid battery threshold values: {e}")), + })?; let power_supply_path = Path::new("/sys/class/power_supply"); if !power_supply_path.exists() { diff --git a/src/config/types.rs b/src/config/types.rs index 500320f..1c606eb 100644 --- a/src/config/types.rs +++ b/src/config/types.rs @@ -3,24 +3,28 @@ use crate::core::TurboSetting; use serde::{Deserialize, Serialize}; use std::convert::TryFrom; -#[derive(Deserialize, Serialize, Debug, Clone)] +#[derive(Deserialize, Serialize, Debug, Clone, PartialEq, Eq)] pub struct BatteryChargeThresholds { pub start: u8, pub stop: u8, } impl BatteryChargeThresholds { - pub fn new(start: u8, stop: u8) -> Result { + pub fn new(start: u8, stop: u8) -> Result { if start == 0 || stop == 0 { - return Err("Thresholds must be greater than 0%".to_string()); - } - if start >= stop { - return Err(format!( - "Start threshold ({start}) must be less than stop threshold ({stop})" + return Err(ConfigError::ValidationError( + "Thresholds must be greater than 0%".to_string(), )); } + if start >= stop { + return Err(ConfigError::ValidationError(format!( + "Start threshold ({start}) must be less than stop threshold ({stop})" + ))); + } if stop > 100 { - return Err(format!("Stop threshold ({stop}) cannot exceed 100%")); + return Err(ConfigError::ValidationError(format!( + "Stop threshold ({stop}) cannot exceed 100%" + ))); } Ok(Self { start, stop }) @@ -28,7 +32,7 @@ impl BatteryChargeThresholds { } impl TryFrom<(u8, u8)> for BatteryChargeThresholds { - type Error = String; + type Error = ConfigError; fn try_from(values: (u8, u8)) -> Result { let (start, stop) = values; @@ -85,6 +89,7 @@ pub enum ConfigError { TomlError(toml::de::Error), NoValidConfigFound, HomeDirNotFound, + ValidationError(String), } impl From for ConfigError { @@ -106,6 +111,7 @@ impl std::fmt::Display for ConfigError { Self::TomlError(e) => write!(f, "TOML parsing error: {e}"), Self::NoValidConfigFound => write!(f, "No valid configuration file found."), Self::HomeDirNotFound => write!(f, "Could not determine user home directory."), + Self::ValidationError(s) => write!(f, "Configuration validation error: {s}"), } } } From 09427ccc9acb2a7f1c50fe7b114016676942e337 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 16 May 2025 01:05:37 +0300 Subject: [PATCH 21/40] config: better serialization in config structs --- src/config/types.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/config/types.rs b/src/config/types.rs index 1c606eb..7de13b7 100644 --- a/src/config/types.rs +++ b/src/config/types.rs @@ -119,7 +119,7 @@ impl std::fmt::Display for ConfigError { impl std::error::Error for ConfigError {} // Intermediate structs for TOML parsing -#[derive(Deserialize, Debug, Clone)] +#[derive(Deserialize, Serialize, Debug, Clone)] pub struct ProfileConfigToml { pub governor: Option, pub turbo: Option, // "always", "auto", "never" @@ -132,7 +132,7 @@ pub struct ProfileConfigToml { pub battery_charge_thresholds: Option, } -#[derive(Deserialize, Debug, Clone, Default)] +#[derive(Deserialize, Serialize, Debug, Clone, Default)] pub struct AppConfigToml { #[serde(default)] pub charger: ProfileConfigToml, @@ -286,7 +286,7 @@ const fn default_stats_file_path() -> Option { None } -#[derive(Deserialize, Debug, Clone)] +#[derive(Deserialize, Serialize, Debug, Clone)] pub struct DaemonConfigToml { #[serde(default = "default_poll_interval_sec")] pub poll_interval_sec: u64, From 2256cf25f9848d6094aa66d41a10a52ea23a87e5 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 16 May 2025 01:27:43 +0300 Subject: [PATCH 22/40] cpu: better governor validation --- src/cpu.rs | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/cpu.rs b/src/cpu.rs index 075e520..7b1b1bf 100644 --- a/src/cpu.rs +++ b/src/cpu.rs @@ -80,11 +80,14 @@ where pub fn set_governor(governor: &str, core_id: Option) -> Result<()> { // Validate the governor is available on this system - if !is_governor_valid(governor)? { + // This returns both the validation result and the list of available governors + let (is_valid, available_governors) = is_governor_valid(governor)?; + + if !is_valid { return Err(ControlError::InvalidValueError(format!( "Governor '{}' is not available on this system. Valid governors: {}", governor, - get_available_governors()?.join(", ") + available_governors.join(", ") ))); } @@ -103,9 +106,18 @@ pub fn set_governor(governor: &str, core_id: Option) -> Result<()> { } /// Check if the provided governor is available in the system -fn is_governor_valid(governor: &str) -> Result { +/// Returns a tuple of (is_valid, available_governors) to avoid redundant file reads +fn is_governor_valid(governor: &str) -> Result<(bool, Vec)> { let governors = get_available_governors()?; - Ok(governors.contains(&governor.to_string())) + + // Convert input governor to lowercase for case-insensitive comparison + let governor_lower = governor.to_lowercase(); + + // Convert all available governors to lowercase for comparison + let governors_lower: Vec = governors.iter().map(|g| g.to_lowercase()).collect(); + + // Check if the lowercase governor is in the lowercase list + Ok((governors_lower.contains(&governor_lower), governors)) } /// Get available CPU governors from the system From 18141d22e160c8b280098215a470aaab0daf3fe9 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 16 May 2025 01:33:38 +0300 Subject: [PATCH 23/40] config: improve EPP value validation --- src/cpu.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cpu.rs b/src/cpu.rs index 7b1b1bf..b17bb59 100644 --- a/src/cpu.rs +++ b/src/cpu.rs @@ -202,7 +202,7 @@ fn try_set_per_core_boost(value: &str) -> Result { pub fn set_epp(epp: &str, core_id: Option) -> Result<()> { // Validate the EPP value against available options let available_epp = get_available_epp_values()?; - if !available_epp.contains(&epp.to_string()) { + if !available_epp.iter().any(|v| v.eq_ignore_ascii_case(epp)) { return Err(ControlError::InvalidValueError(format!( "Invalid EPP value: '{}'. Available values: {}", epp, From e8d7d1ab86a169cb6582ff6f95a328a7fddf54b7 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 16 May 2025 01:42:08 +0300 Subject: [PATCH 24/40] cpu: prevent possible overflow in frequency calculations --- src/cpu.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/cpu.rs b/src/cpu.rs index b17bb59..1932efe 100644 --- a/src/cpu.rs +++ b/src/cpu.rs @@ -309,7 +309,10 @@ pub fn set_min_frequency(freq_mhz: u32, core_id: Option) -> Result<()> { } } - let freq_khz_str = (freq_mhz * 1000).to_string(); + // XXX: We use u64 for the intermediate calculation to prevent overflow + let freq_khz = u64::from(freq_mhz) * 1000; + let freq_khz_str = freq_khz.to_string(); + let action = |id: u32| { let path = format!("/sys/devices/system/cpu/cpu{id}/cpufreq/scaling_min_freq"); if Path::new(&path).exists() { @@ -333,7 +336,10 @@ pub fn set_max_frequency(freq_mhz: u32, core_id: Option) -> Result<()> { } } - let freq_khz_str = (freq_mhz * 1000).to_string(); + // XXX: Use a u64 here as well. + let freq_khz = u64::from(freq_mhz) * 1000; + let freq_khz_str = freq_khz.to_string(); + let action = |id: u32| { let path = format!("/sys/devices/system/cpu/cpu{id}/cpufreq/scaling_max_freq"); if Path::new(&path).exists() { From d3f2442ccca5edf29727ab1d6dcbb5fc2762d581 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 16 May 2025 01:52:16 +0300 Subject: [PATCH 25/40] config: add valid EPB strings; better validation --- src/cpu.rs | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/src/cpu.rs b/src/cpu.rs index 1932efe..539aee1 100644 --- a/src/cpu.rs +++ b/src/cpu.rs @@ -5,6 +5,16 @@ use std::{fs, io, path::Path, string::ToString}; pub type Result = std::result::Result; +// Valid EPB string values +const VALID_EPB_STRINGS: &[&str] = &[ + "performance", + "balance-performance", + "balance_performance", // alternative form + "balance-power", + "balance_power", // alternative form + "power", +]; + // Write a value to a sysfs file fn write_sysfs_value(path: impl AsRef, value: &str) -> Result<()> { let p = path.as_ref(); @@ -106,7 +116,7 @@ pub fn set_governor(governor: &str, core_id: Option) -> Result<()> { } /// Check if the provided governor is available in the system -/// Returns a tuple of (is_valid, available_governors) to avoid redundant file reads +/// Returns a tuple of (`is_valid`, `available_governors`) to avoid redundant file reads fn is_governor_valid(governor: &str) -> Result<(bool, Vec)> { let governors = get_available_governors()?; @@ -275,24 +285,18 @@ fn validate_epb_value(epb: &str) -> Result<()> { ))); } - // If not a number, check if it's a recognized string value - let valid_strings = [ - "performance", - "balance-performance", - "balance-power", - "power", - // Alternative forms - "balance_performance", - "balance_power", - ]; - - if valid_strings.contains(&epb) { + // If not a number, check if it's a recognized string value. + // This is using case-insensitive comparison + if VALID_EPB_STRINGS + .iter() + .any(|valid| valid.eq_ignore_ascii_case(epb)) + { Ok(()) } else { Err(ControlError::InvalidValueError(format!( "Invalid EPB value: '{}'. Must be a number 0-15 or one of: {}", epb, - valid_strings.join(", ") + VALID_EPB_STRINGS.join(", ") ))) } } From 2a5c00e3a7a2c0046bf8a36e186e71a1e02e8011 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 16 May 2025 02:04:00 +0300 Subject: [PATCH 26/40] battery: skip writable check for power supply path It wouldn't even work, lol. --- src/battery.rs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/battery.rs b/src/battery.rs index 296096f..f393f25 100644 --- a/src/battery.rs +++ b/src/battery.rs @@ -87,13 +87,8 @@ pub fn set_battery_charge_thresholds(start_threshold: u8, stop_threshold: u8) -> )); } - // Check if the power supply directory is writable - // This helps identify permission issues in containerized environments early - if !is_path_writable(power_supply_path) { - return Err(ControlError::PermissionDenied( - "Power supply path exists but is not writable. This may occur in containerized environments.".to_string(), - )); - } + // XXX: Skip checking directory writability since /sys is a virtual filesystem + // Individual file writability will be checked by find_battery_with_threshold_support let supported_batteries = find_supported_batteries(power_supply_path)?; if supported_batteries.is_empty() { From cd68ffd054417d045aa9d6a5c44b78486f559f20 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 16 May 2025 02:13:44 +0300 Subject: [PATCH 27/40] battery: remove writable path checker --- src/battery.rs | 21 --------------------- src/cpu.rs | 2 -- src/util/error.rs | 4 ---- 3 files changed, 27 deletions(-) diff --git a/src/battery.rs b/src/battery.rs index f393f25..4c1f632 100644 --- a/src/battery.rs +++ b/src/battery.rs @@ -262,24 +262,3 @@ fn is_battery(path: &Path) -> Result { Ok(ps_type == "Battery") } - -/// Check if a directory is writable by attempting to open a temporary file for writing -fn is_path_writable(path: &Path) -> bool { - use std::fs::OpenOptions; - - // Try to create a temporary file to check write permissions - // If we're in a container with a read-only mount, this will fail - let temp_file_path = path.join(".superfreq_write_test"); - let result = OpenOptions::new() - .write(true) - .create(true) - .open(&temp_file_path); - - // Clean up the temporary file if we created it - if result.is_ok() { - let _ = fs::remove_file(temp_file_path); - true - } else { - false - } -} diff --git a/src/cpu.rs b/src/cpu.rs index df18e83..9f82f09 100644 --- a/src/cpu.rs +++ b/src/cpu.rs @@ -1,7 +1,6 @@ use crate::core::{GovernorOverrideMode, TurboSetting}; use crate::util::error::ControlError; use core::str; -use std::path::PathBuf; use std::{fs, io, path::Path, string::ToString}; pub type Result = std::result::Result; @@ -140,7 +139,6 @@ fn get_available_governors() -> Result> { let path = "/sys/devices/system/cpu/cpu0/cpufreq/scaling_available_governors"; if !Path::new(path).exists() { - return Err(ControlError::NotSupported( "Could not determine available governors".to_string(), )); diff --git a/src/util/error.rs b/src/util/error.rs index 6ba476f..b52480b 100644 --- a/src/util/error.rs +++ b/src/util/error.rs @@ -11,7 +11,6 @@ pub enum ControlError { InvalidProfile(String), InvalidGovernor(String), ParseError(String), - ReadError(String), PathMissing(String), } @@ -47,9 +46,6 @@ impl std::fmt::Display for ControlError { Self::ParseError(s) => { write!(f, "Failed to parse value: {s}") } - Self::ReadError(s) => { - write!(f, "Failed to read sysfs path: {s}") - } Self::PathMissing(s) => { write!(f, "Path missing: {s}") } From 3d490db734e79554d1243c0406bb7d2eee261546 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 16 May 2025 02:19:15 +0300 Subject: [PATCH 28/40] battery: better threshold setting logic --- src/battery.rs | 79 +++++++++++++++++++++++++++++++++++--------------- 1 file changed, 56 insertions(+), 23 deletions(-) diff --git a/src/battery.rs b/src/battery.rs index 4c1f632..a3c3d3f 100644 --- a/src/battery.rs +++ b/src/battery.rs @@ -161,6 +161,19 @@ fn write_sysfs_value(path: impl AsRef, value: &str) -> Result<()> { }) } +/// Read a value from a sysfs file +fn read_sysfs_value(path: impl AsRef) -> Result { + let p = path.as_ref(); + fs::read_to_string(p).map_err(|e| { + let error_msg = format!("Path: {:?}, Error: {}", p.display(), e); + if e.kind() == io::ErrorKind::PermissionDenied { + ControlError::PermissionDenied(error_msg) + } else { + ControlError::ReadError(error_msg) + } + }).map(|s| s.trim().to_string()) +} + /// Safely check if a path exists and is writable fn path_exists_and_writable(path: &Path) -> bool { if !path.exists() { @@ -203,37 +216,57 @@ fn apply_thresholds_to_batteries( for battery in batteries { let start_path = battery.path.join(battery.pattern.start_path); let stop_path = battery.path.join(battery.pattern.stop_path); - - match ( - write_sysfs_value(&start_path, &start_threshold.to_string()), - write_sysfs_value(&stop_path, &stop_threshold.to_string()), - ) { - (Ok(()), Ok(())) => { - debug!( - "Set {}-{}% charge thresholds for {} battery '{}'", - start_threshold, stop_threshold, battery.pattern.description, battery.name - ); - success_count += 1; - } - (start_result, stop_result) => { - let mut error_msg = format!( - "Failed to set thresholds for {} battery '{}'", - battery.pattern.description, battery.name - ); - if let Err(e) = start_result { - error_msg.push_str(&format!(": start threshold error: {e}")); + + // Read current thresholds in case we need to restore them + let current_start = read_sysfs_value(&start_path).ok(); + let current_stop = read_sysfs_value(&stop_path).ok(); + + // Write stop threshold first (must be >= start threshold) + let stop_result = write_sysfs_value(&stop_path, &stop_threshold.to_string()); + + // Only proceed to set start threshold if stop threshold was set successfully + if let Ok(()) = stop_result { + let start_result = write_sysfs_value(&start_path, &start_threshold.to_string()); + + match start_result { + Ok(()) => { + debug!( + "Set {}-{}% charge thresholds for {} battery '{}'", + start_threshold, stop_threshold, battery.pattern.description, battery.name + ); + success_count += 1; } - if let Err(e) = stop_result { - error_msg.push_str(&format!(": stop threshold error: {e}")); + Err(e) => { + // Start threshold failed, try to restore the previous stop threshold + if let Some(prev_stop) = ¤t_stop { + let restore_result = write_sysfs_value(&stop_path, prev_stop); + if let Err(re) = restore_result { + warn!( + "Failed to restore previous stop threshold for battery '{}': {}. Battery may be in an inconsistent state.", + battery.name, re + ); + } else { + debug!("Restored previous stop threshold ({}) for battery '{}'", prev_stop, battery.name); + } + } + + errors.push(format!( + "Failed to set start threshold for {} battery '{}': {}", + battery.pattern.description, battery.name, e + )); } - errors.push(error_msg); } + } else if let Err(e) = stop_result { + errors.push(format!( + "Failed to set stop threshold for {} battery '{}': {}", + battery.pattern.description, battery.name, e + )); } } if success_count > 0 { if !errors.is_empty() { - debug!( + warn!( "Partial success setting battery thresholds: {}", errors.join("; ") ); From df5ae64c44985aa84c99c538fd631a9ce942f25d Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 16 May 2025 02:20:09 +0300 Subject: [PATCH 29/40] cpu: add alternative EPP string formats for better compat --- src/cpu.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/cpu.rs b/src/cpu.rs index 9f82f09..f3a0bbd 100644 --- a/src/cpu.rs +++ b/src/cpu.rs @@ -246,7 +246,9 @@ fn get_available_epp_values() -> Result> { "default".to_string(), "performance".to_string(), "balance_performance".to_string(), + "balance_performance".replace('_', "-"), "balance_power".to_string(), + "balance_power".replace('_', "-"), "power".to_string(), ]); } From 7d28d12c1cf22bcbae2dbbefcbdadfb3f36c94c2 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 16 May 2025 02:20:43 +0300 Subject: [PATCH 30/40] cpu: update governor override path --- src/cpu.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cpu.rs b/src/cpu.rs index f3a0bbd..53e4cc0 100644 --- a/src/cpu.rs +++ b/src/cpu.rs @@ -487,7 +487,7 @@ pub fn get_platform_profiles() -> Result> { } /// Path for storing the governor override state -const GOVERNOR_OVERRIDE_PATH: &str = "/etc/superfreq/governor_override"; +const GOVERNOR_OVERRIDE_PATH: &str = "/etc/xdg/superfreq/governor_override"; /// Force a specific CPU governor or reset to automatic mode pub fn force_governor(mode: GovernorOverrideMode) -> Result<()> { From c1d81b687cce1ffcaadf40f83b67160f23735942 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 16 May 2025 03:03:35 +0300 Subject: [PATCH 31/40] battery: deduplicate frequency validation logic --- src/battery.rs | 35 ++++++++++++++++++++--------------- src/main.rs | 46 ++++++++++++++++++++++++---------------------- 2 files changed, 44 insertions(+), 37 deletions(-) diff --git a/src/battery.rs b/src/battery.rs index a3c3d3f..d555bd5 100644 --- a/src/battery.rs +++ b/src/battery.rs @@ -164,14 +164,16 @@ fn write_sysfs_value(path: impl AsRef, value: &str) -> Result<()> { /// Read a value from a sysfs file fn read_sysfs_value(path: impl AsRef) -> Result { let p = path.as_ref(); - fs::read_to_string(p).map_err(|e| { - let error_msg = format!("Path: {:?}, Error: {}", p.display(), e); - if e.kind() == io::ErrorKind::PermissionDenied { - ControlError::PermissionDenied(error_msg) - } else { - ControlError::ReadError(error_msg) - } - }).map(|s| s.trim().to_string()) + fs::read_to_string(p) + .map_err(|e| { + let error_msg = format!("Path: {:?}, Error: {}", p.display(), e); + if e.kind() == io::ErrorKind::PermissionDenied { + ControlError::PermissionDenied(error_msg) + } else { + ControlError::ReadError(error_msg) + } + }) + .map(|s| s.trim().to_string()) } /// Safely check if a path exists and is writable @@ -216,18 +218,18 @@ fn apply_thresholds_to_batteries( for battery in batteries { let start_path = battery.path.join(battery.pattern.start_path); let stop_path = battery.path.join(battery.pattern.stop_path); - + // Read current thresholds in case we need to restore them let current_start = read_sysfs_value(&start_path).ok(); let current_stop = read_sysfs_value(&stop_path).ok(); - + // Write stop threshold first (must be >= start threshold) let stop_result = write_sysfs_value(&stop_path, &stop_threshold.to_string()); - + // Only proceed to set start threshold if stop threshold was set successfully - if let Ok(()) = stop_result { + if matches!(stop_result, Ok(())) { let start_result = write_sysfs_value(&start_path, &start_threshold.to_string()); - + match start_result { Ok(()) => { debug!( @@ -246,10 +248,13 @@ fn apply_thresholds_to_batteries( battery.name, re ); } else { - debug!("Restored previous stop threshold ({}) for battery '{}'", prev_stop, battery.name); + debug!( + "Restored previous stop threshold ({}) for battery '{}'", + prev_stop, battery.name + ); } } - + errors.push(format!( "Failed to set start threshold for {} battery '{}': {}", battery.pattern.description, battery.name, e diff --git a/src/main.rs b/src/main.rs index de2cbcc..471b18e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -358,17 +358,9 @@ fn main() { } Some(Commands::SetMinFreq { freq_mhz, core_id }) => { // Basic validation for reasonable CPU frequency values - if freq_mhz == 0 { - error!("Minimum frequency cannot be zero"); - Err(Box::new(ControlError::InvalidValueError( - "Minimum frequency cannot be zero".to_string(), - )) as Box) - } else if freq_mhz > 10000 { - // Extremely high value unlikely to be valid - error!("Minimum frequency ({freq_mhz} MHz) is unreasonably high"); - Err(Box::new(ControlError::InvalidValueError(format!( - "Minimum frequency ({freq_mhz} MHz) is unreasonably high" - ))) as Box) + if let Err(e) = validate_freq(freq_mhz, "Minimum") { + error!("{e}"); + Err(e) } else { cpu::set_min_frequency(freq_mhz, core_id) .map_err(|e| Box::new(e) as Box) @@ -376,17 +368,9 @@ fn main() { } Some(Commands::SetMaxFreq { freq_mhz, core_id }) => { // Basic validation for reasonable CPU frequency values - if freq_mhz == 0 { - error!("Maximum frequency cannot be zero"); - Err(Box::new(ControlError::InvalidValueError( - "Maximum frequency cannot be zero".to_string(), - )) as Box) - } else if freq_mhz > 10000 { - // Extremely high value unlikely to be valid - error!("Maximum frequency ({freq_mhz} MHz) is unreasonably high"); - Err(Box::new(ControlError::InvalidValueError(format!( - "Maximum frequency ({freq_mhz} MHz) is unreasonably high" - ))) as Box) + if let Err(e) = validate_freq(freq_mhz, "Maximum") { + error!("{e}"); + Err(e) } else { cpu::set_max_frequency(freq_mhz, core_id) .map_err(|e| Box::new(e) as Box) @@ -497,3 +481,21 @@ fn init_logger() { debug!("Logger initialized with RUST_LOG={env_log}"); }); } + +/// Validate CPU frequency input values +fn validate_freq(freq_mhz: u32, label: &str) -> Result<(), Box> { + if freq_mhz == 0 { + error!("{label} frequency cannot be zero"); + Err(Box::new(ControlError::InvalidValueError(format!( + "{label} frequency cannot be zero" + ))) as Box) + } else if freq_mhz > 10000 { + // Extremely high value unlikely to be valid + error!("{label} frequency ({freq_mhz} MHz) is unreasonably high"); + Err(Box::new(ControlError::InvalidValueError(format!( + "{label} frequency ({freq_mhz} MHz) is unreasonably high" + ))) as Box) + } else { + Ok(()) + } +} From 622f4f6f32ffae4fd89a42d0197b5c20947c01df Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 16 May 2025 03:06:59 +0300 Subject: [PATCH 32/40] engine: simplify error handling --- src/engine.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/engine.rs b/src/engine.rs index 7001871..791fa5a 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -23,14 +23,13 @@ where match apply_fn() { Ok(_) => Ok(()), Err(e) => { - if matches!(e, ControlError::NotSupported(_)) - || matches!(e, ControlError::InvalidValueError(_)) - { + if matches!(e, ControlError::NotSupported(_)) { warn!( "{feature_name} setting is not supported on this system. Skipping {feature_name} configuration." ); Ok(()) } else { + // Propagate all other errors, including InvalidValueError Err(EngineError::ControlError(e)) } } From 8f860424f915746ac02b78fa42a201e4e2530a21 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 16 May 2025 03:11:05 +0300 Subject: [PATCH 33/40] battery: better validation for battery charge thresholds To accommodate vendors that use 0 to mean "charge immediately" or "disable start threshold," we modified the validation logic to allow start to be 0 *as long as* start is less than stop and stop is less than OR equal to 100 --- src/config/types.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/config/types.rs b/src/config/types.rs index 7de13b7..10e0a9c 100644 --- a/src/config/types.rs +++ b/src/config/types.rs @@ -11,9 +11,9 @@ pub struct BatteryChargeThresholds { impl BatteryChargeThresholds { pub fn new(start: u8, stop: u8) -> Result { - if start == 0 || stop == 0 { + if stop == 0 { return Err(ConfigError::ValidationError( - "Thresholds must be greater than 0%".to_string(), + "Stop threshold must be greater than 0%".to_string(), )); } if start >= stop { From 4e03679209dd0bf824b60d7226909155b68cea18 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 16 May 2025 03:18:50 +0300 Subject: [PATCH 34/40] cli: use Clap's value parser for cheaper validation --- src/main.rs | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/src/main.rs b/src/main.rs index 471b18e..0b0f1ff 100644 --- a/src/main.rs +++ b/src/main.rs @@ -12,7 +12,7 @@ mod util; use crate::config::AppConfig; use crate::core::{GovernorOverrideMode, TurboSetting}; use crate::util::error::ControlError; -use clap::Parser; +use clap::{Parser, value_parser}; use env_logger::Builder; use log::{debug, error, info}; use std::sync::Once; @@ -81,8 +81,10 @@ enum Commands { /// Set battery charge thresholds to extend battery lifespan SetBatteryThresholds { /// Percentage at which charging starts (when below this value) + #[clap(value_parser = value_parser!(u8).range(0..=99))] start_threshold: u8, /// Percentage at which charging stops (when it reaches this value) + #[clap(value_parser = value_parser!(u8).range(1..=100))] stop_threshold: u8, }, } @@ -409,7 +411,7 @@ fn main() { start_threshold, stop_threshold, }) => { - // Basic validation to provide proper error messages at the CLI level + // We only need to check if start < stop since the range validation is handled by Clap if start_threshold >= stop_threshold { error!( "Start threshold ({start_threshold}) must be less than stop threshold ({stop_threshold})" @@ -417,16 +419,6 @@ fn main() { Err(Box::new(ControlError::InvalidValueError(format!( "Start threshold ({start_threshold}) must be less than stop threshold ({stop_threshold})" ))) as Box) - } else if stop_threshold > 100 { - error!("Stop threshold ({stop_threshold}) cannot exceed 100%"); - Err(Box::new(ControlError::InvalidValueError(format!( - "Stop threshold ({stop_threshold}) cannot exceed 100%" - ))) as Box) - } else if start_threshold == 0 || stop_threshold == 0 { - error!("Thresholds must be greater than 0%"); - Err(Box::new(ControlError::InvalidValueError( - "Thresholds must be greater than 0%".to_string(), - )) as Box) } else { info!( "Setting battery thresholds: start at {start_threshold}%, stop at {stop_threshold}%" From e6b7f3eb34701e84c1875caeb4c8b4b6efd884a5 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 16 May 2025 03:32:41 +0300 Subject: [PATCH 35/40] battery: case-insensitive matching for peripherals --- src/monitor.rs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/monitor.rs b/src/monitor.rs index b4d4021..0b3f7fb 100644 --- a/src/monitor.rs +++ b/src/monitor.rs @@ -604,14 +604,17 @@ pub fn get_battery_info(config: &AppConfig) -> Result> { /// Check if a battery is likely a peripheral (mouse, keyboard, etc) not a laptop battery fn is_peripheral_battery(ps_path: &Path, name: &str) -> bool { + // Convert name to lowercase once for case-insensitive matching + let name_lower = name.to_lowercase(); + // Common peripheral battery names - if name.contains("mouse") - || name.contains("keyboard") - || name.contains("trackpad") - || name.contains("gamepad") - || name.contains("controller") - || name.contains("headset") - || name.contains("headphone") + if name_lower.contains("mouse") + || name_lower.contains("keyboard") + || name_lower.contains("trackpad") + || name_lower.contains("gamepad") + || name_lower.contains("controller") + || name_lower.contains("headset") + || name_lower.contains("headphone") { return true; } From 6c2fc6652fc14bc3a678d7ad39ab4b9762d6127c Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 16 May 2025 03:46:54 +0300 Subject: [PATCH 36/40] monitor: optimize path search for CPU frequency directory --- src/monitor.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/monitor.rs b/src/monitor.rs index 0b3f7fb..80605ff 100644 --- a/src/monitor.rs +++ b/src/monitor.rs @@ -399,11 +399,13 @@ pub fn get_cpu_global_info(cpu_cores: &[CpuCoreInfo]) -> CpuGlobalInfo { eprintln!("Warning: {e}"); 0 }); - let path = (0..core_count) - .map(|i| PathBuf::from(format!("/sys/devices/system/cpu/cpu{i}/cpufreq/"))) - .find(|path| path.exists()); - if let Some(test_path_buf) = path { - cpufreq_base_path_buf = test_path_buf; + + for i in 0..core_count { + let test_path = PathBuf::from(format!("/sys/devices/system/cpu/cpu{i}/cpufreq/")); + if test_path.exists() { + cpufreq_base_path_buf = test_path; + break; // Exit the loop as soon as we find a valid path + } } } From a3137ff2d0cc62005aac3437ba488d6d5114e51f Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 16 May 2025 04:03:08 +0300 Subject: [PATCH 37/40] battery: bettery error handling in sysfs value reading --- src/battery.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/battery.rs b/src/battery.rs index d555bd5..99be305 100644 --- a/src/battery.rs +++ b/src/battery.rs @@ -167,10 +167,12 @@ fn read_sysfs_value(path: impl AsRef) -> Result { fs::read_to_string(p) .map_err(|e| { let error_msg = format!("Path: {:?}, Error: {}", p.display(), e); - if e.kind() == io::ErrorKind::PermissionDenied { - ControlError::PermissionDenied(error_msg) - } else { - ControlError::ReadError(error_msg) + match e.kind() { + io::ErrorKind::PermissionDenied => ControlError::PermissionDenied(error_msg), + io::ErrorKind::NotFound => { + ControlError::PathMissing(format!("Path '{}' does not exist", p.display())) + } + _ => ControlError::ReadError(error_msg), } }) .map(|s| s.trim().to_string()) @@ -220,7 +222,6 @@ fn apply_thresholds_to_batteries( let stop_path = battery.path.join(battery.pattern.stop_path); // Read current thresholds in case we need to restore them - let current_start = read_sysfs_value(&start_path).ok(); let current_stop = read_sysfs_value(&stop_path).ok(); // Write stop threshold first (must be >= start threshold) From 3d4d3b8075d1b55e556d29978d99249dff48bd7c Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 16 May 2025 04:42:21 +0300 Subject: [PATCH 38/40] battery: refactor sysfs handler into util module --- src/battery.rs | 97 ++++++++++++++--------------------------------- src/util/mod.rs | 1 + src/util/sysfs.rs | 80 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 109 insertions(+), 69 deletions(-) create mode 100644 src/util/sysfs.rs diff --git a/src/battery.rs b/src/battery.rs index 99be305..9beb05e 100644 --- a/src/battery.rs +++ b/src/battery.rs @@ -1,4 +1,4 @@ -use crate::{config::types::BatteryChargeThresholds, util::error::ControlError}; +use crate::{config::types::BatteryChargeThresholds, util::error::ControlError, util::sysfs}; use log::{debug, warn}; use std::{ fs, io, @@ -148,66 +148,6 @@ fn find_supported_batteries(power_supply_path: &Path) -> Result, value: &str) -> Result<()> { - let p = path.as_ref(); - fs::write(p, value).map_err(|e| { - let error_msg = format!("Path: {:?}, Value: '{}', Error: {}", p.display(), value, e); - if e.kind() == io::ErrorKind::PermissionDenied { - ControlError::PermissionDenied(error_msg) - } else { - ControlError::WriteError(error_msg) - } - }) -} - -/// Read a value from a sysfs file -fn read_sysfs_value(path: impl AsRef) -> Result { - let p = path.as_ref(); - fs::read_to_string(p) - .map_err(|e| { - let error_msg = format!("Path: {:?}, Error: {}", p.display(), e); - match e.kind() { - io::ErrorKind::PermissionDenied => ControlError::PermissionDenied(error_msg), - io::ErrorKind::NotFound => { - ControlError::PathMissing(format!("Path '{}' does not exist", p.display())) - } - _ => ControlError::ReadError(error_msg), - } - }) - .map(|s| s.trim().to_string()) -} - -/// Safely check if a path exists and is writable -fn path_exists_and_writable(path: &Path) -> bool { - if !path.exists() { - return false; - } - - // Try to open the file with write access to verify write permission - fs::OpenOptions::new().write(true).open(path).is_ok() -} - -/// Identifies if a battery supports threshold control and which pattern it uses -fn find_battery_with_threshold_support(ps_path: &Path) -> Option> { - for pattern in THRESHOLD_PATTERNS { - let start_threshold_path = ps_path.join(pattern.start_path); - let stop_threshold_path = ps_path.join(pattern.stop_path); - - // Ensure both paths exist and are writable before considering this battery supported - if path_exists_and_writable(&start_threshold_path) - && path_exists_and_writable(&stop_threshold_path) - { - return Some(SupportedBattery { - name: ps_path.file_name()?.to_string_lossy().to_string(), - pattern, - path: ps_path.to_path_buf(), - }); - } - } - None -} - /// Applies the threshold settings to all supported batteries fn apply_thresholds_to_batteries( batteries: &[SupportedBattery<'_>], @@ -222,14 +162,14 @@ fn apply_thresholds_to_batteries( let stop_path = battery.path.join(battery.pattern.stop_path); // Read current thresholds in case we need to restore them - let current_stop = read_sysfs_value(&stop_path).ok(); + let current_stop = sysfs::read_sysfs_value(&stop_path).ok(); // Write stop threshold first (must be >= start threshold) - let stop_result = write_sysfs_value(&stop_path, &stop_threshold.to_string()); + let stop_result = sysfs::write_sysfs_value(&stop_path, &stop_threshold.to_string()); // Only proceed to set start threshold if stop threshold was set successfully if matches!(stop_result, Ok(())) { - let start_result = write_sysfs_value(&start_path, &start_threshold.to_string()); + let start_result = sysfs::write_sysfs_value(&start_path, &start_threshold.to_string()); match start_result { Ok(()) => { @@ -242,7 +182,7 @@ fn apply_thresholds_to_batteries( Err(e) => { // Start threshold failed, try to restore the previous stop threshold if let Some(prev_stop) = ¤t_stop { - let restore_result = write_sysfs_value(&stop_path, prev_stop); + let restore_result = sysfs::write_sysfs_value(&stop_path, prev_stop); if let Err(re) = restore_result { warn!( "Failed to restore previous stop threshold for battery '{}': {}. Battery may be in an inconsistent state.", @@ -294,10 +234,29 @@ fn is_battery(path: &Path) -> Result { return Ok(false); } - let ps_type = fs::read_to_string(&type_path) - .map_err(|_| ControlError::ReadError(format!("Failed to read {}", type_path.display())))? - .trim() - .to_string(); + let ps_type = sysfs::read_sysfs_value(&type_path).map_err(|e| { + ControlError::ReadError(format!("Failed to read {}: {}", type_path.display(), e)) + })?; Ok(ps_type == "Battery") } + +/// Identifies if a battery supports threshold control and which pattern it uses +fn find_battery_with_threshold_support(ps_path: &Path) -> Option> { + for pattern in THRESHOLD_PATTERNS { + let start_threshold_path = ps_path.join(pattern.start_path); + let stop_threshold_path = ps_path.join(pattern.stop_path); + + // Ensure both paths exist and are writable before considering this battery supported + if sysfs::path_exists_and_writable(&start_threshold_path) + && sysfs::path_exists_and_writable(&stop_threshold_path) + { + return Some(SupportedBattery { + name: ps_path.file_name()?.to_string_lossy().to_string(), + pattern, + path: ps_path.to_path_buf(), + }); + } + } + None +} diff --git a/src/util/mod.rs b/src/util/mod.rs index a91e735..0aa2927 100644 --- a/src/util/mod.rs +++ b/src/util/mod.rs @@ -1 +1,2 @@ pub mod error; +pub mod sysfs; diff --git a/src/util/sysfs.rs b/src/util/sysfs.rs new file mode 100644 index 0000000..e1776e5 --- /dev/null +++ b/src/util/sysfs.rs @@ -0,0 +1,80 @@ +use crate::util::error::ControlError; +use std::{fs, io, path::Path}; + +/// Write a value to a sysfs file with consistent error handling +/// +/// # Arguments +/// +/// * `path` - The file path to write to +/// * `value` - The string value to write +/// +/// # Errors +/// +/// Returns a `ControlError` variant based on the specific error: +/// - `ControlError::PermissionDenied` if permission is denied +/// - `ControlError::PathMissing` if the path doesn't exist +/// - `ControlError::WriteError` for other I/O errors +pub fn write_sysfs_value(path: impl AsRef, value: &str) -> Result<(), ControlError> { + let p = path.as_ref(); + + fs::write(p, value).map_err(|e| { + let error_msg = format!("Path: {:?}, Value: '{}', Error: {}", p.display(), value, e); + match e.kind() { + io::ErrorKind::PermissionDenied => ControlError::PermissionDenied(error_msg), + io::ErrorKind::NotFound => { + ControlError::PathMissing(format!("Path '{}' does not exist", p.display())) + } + _ => ControlError::WriteError(error_msg), + } + }) +} + +/// Read a value from a sysfs file with consistent error handling +/// +/// # Arguments +/// +/// * `path` - The file path to read from +/// +/// # Returns +/// +/// Returns the trimmed contents of the file as a String +/// +/// # Errors +/// +/// Returns a `ControlError` variant based on the specific error: +/// - `ControlError::PermissionDenied` if permission is denied +/// - `ControlError::PathMissing` if the path doesn't exist +/// - `ControlError::ReadError` for other I/O errors +pub fn read_sysfs_value(path: impl AsRef) -> Result { + let p = path.as_ref(); + fs::read_to_string(p) + .map_err(|e| { + let error_msg = format!("Path: {:?}, Error: {}", p.display(), e); + match e.kind() { + io::ErrorKind::PermissionDenied => ControlError::PermissionDenied(error_msg), + io::ErrorKind::NotFound => { + ControlError::PathMissing(format!("Path '{}' does not exist", p.display())) + } + _ => ControlError::ReadError(error_msg), + } + }) + .map(|s| s.trim().to_string()) +} + +/// Safely check if a path exists and is writable +/// +/// # Arguments +/// +/// * `path` - The file path to check +/// +/// # Returns +/// +/// Returns true if the path exists and is writable, false otherwise +pub fn path_exists_and_writable(path: &Path) -> bool { + if !path.exists() { + return false; + } + + // Try to open the file with write access to verify write permission + fs::OpenOptions::new().write(true).open(path).is_ok() +} From 5b347eb3e237d34e3e5374039cce4ae72c8cdfd7 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 16 May 2025 05:00:41 +0300 Subject: [PATCH 39/40] cpu: scan all CPUs for available governors; cleanup --- src/cpu.rs | 72 +++++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 60 insertions(+), 12 deletions(-) diff --git a/src/cpu.rs b/src/cpu.rs index 53e4cc0..d9f53dd 100644 --- a/src/cpu.rs +++ b/src/cpu.rs @@ -135,22 +135,70 @@ fn is_governor_valid(governor: &str) -> Result<(bool, Vec)> { /// Get available CPU governors from the system fn get_available_governors() -> Result> { - // We'll check cpu0's available governors as they're typically the same across cores - let path = "/sys/devices/system/cpu/cpu0/cpufreq/scaling_available_governors"; + let cpu_base_path = Path::new("/sys/devices/system/cpu"); - if !Path::new(path).exists() { - return Err(ControlError::NotSupported( - "Could not determine available governors".to_string(), - )); + // First try the traditional path with cpu0. This is the most common case + // and will usually catch early, but we should try to keep the code to handle + // "edge" cases lightweight, for the (albeit smaller) number of users that + // run Superfreq on unusual systems. + let cpu0_path = "/sys/devices/system/cpu/cpu0/cpufreq/scaling_available_governors"; + if Path::new(cpu0_path).exists() { + let content = fs::read_to_string(cpu0_path).map_err(|e| { + ControlError::ReadError(format!("Failed to read available governors from cpu0: {e}")) + })?; + + let governors: Vec = content + .split_whitespace() + .map(ToString::to_string) + .collect(); + + if !governors.is_empty() { + return Ok(governors); + } } - let content = fs::read_to_string(path) - .map_err(|e| ControlError::ReadError(format!("Failed to read available governors: {e}")))?; + // If cpu0 doesn't have the file or it's empty, scan all CPUs + // This handles heterogeneous systems where cpu0 might not have cpufreq + if let Ok(entries) = fs::read_dir(cpu_base_path) { + for entry in entries.flatten() { + let path = entry.path(); + let file_name = entry.file_name(); + let name = match file_name.to_str() { + Some(name) => name, + None => continue, + }; - Ok(content - .split_whitespace() - .map(ToString::to_string) - .collect()) + // Skip non-CPU directories + if !name.starts_with("cpu") + || name.len() <= 3 + || !name[3..].chars().all(char::is_numeric) + { + continue; + } + + let governor_path = path.join("cpufreq/scaling_available_governors"); + if governor_path.exists() { + match fs::read_to_string(&governor_path) { + Ok(content) => { + let governors: Vec = content + .split_whitespace() + .map(ToString::to_string) + .collect(); + + if !governors.is_empty() { + return Ok(governors); + } + } + Err(_) => continue, // try next CPU if this one fails + } + } + } + } + + // If we get here, we couldn't find any valid governors list + Err(ControlError::NotSupported( + "Could not determine available governors on any CPU".to_string(), + )) } pub fn set_turbo(setting: TurboSetting) -> Result<()> { From d0cc9e4fb3ca75078fcb20dd133bbdafe10ab255 Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Fri, 16 May 2025 05:10:01 +0300 Subject: [PATCH 40/40] cpu: reduce allocations for EPP values --- src/cpu.rs | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/src/cpu.rs b/src/cpu.rs index d9f53dd..eeb4dfa 100644 --- a/src/cpu.rs +++ b/src/cpu.rs @@ -15,6 +15,17 @@ const VALID_EPB_STRINGS: &[&str] = &[ "power", ]; +// EPP (Energy Performance Preference) string values +const EPP_FALLBACK_VALUES: &[&str] = &[ + "default", + "performance", + "balance-performance", + "balance_performance", // alternative form with underscore + "balance-power", + "balance_power", // alternative form with underscore + "power", +]; + // Write a value to a sysfs file fn write_sysfs_value(path: impl AsRef, value: &str) -> Result<()> { let p = path.as_ref(); @@ -288,17 +299,9 @@ fn get_available_epp_values() -> Result> { if !Path::new(path).exists() { // If the file doesn't exist, fall back to a default set of common values - // This is safer than failing outright, as some systems may allow these values + // This is safer than failing outright, as some systems may allow these values │ // even without explicitly listing them - return Ok(vec![ - "default".to_string(), - "performance".to_string(), - "balance_performance".to_string(), - "balance_performance".replace('_', "-"), - "balance_power".to_string(), - "balance_power".replace('_', "-"), - "power".to_string(), - ]); + return Ok(EPP_FALLBACK_VALUES.iter().map(|&s| s.to_string()).collect()); } let content = fs::read_to_string(path).map_err(|e| {