Merge branch 'fluke/profiles' into 'main'

fix: Profile select by name is correctly choosing

Closes #96 and #100

See merge request asus-linux/asusctl!68

CHANGELOG.md

@@ -7,6 +7,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## [Unreleased]
 ### Changed
 - Fix missing CLI command help for some supported options
+- Fix incorrectly selecting profile by name, where the active profile was being copied to the selected profile
 
 # [3.7.1] - 2021-06-11
 ### Changed

asusctl/src/main.rs

@@ -14,7 +14,6 @@ use rog_types::{
     gfx_vendors::{GfxRequiredUserAction, GfxVendors},
     supported::{
         FanCpuSupportedFunctions, LedSupportedFunctions, RogBiosSupportedFunctions,
-        SupportedFunctions,
     },
 };
 use std::{env::args, path::Path};
@@ -133,12 +132,6 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
 
     let supported = dbus.proxies().supported().get_supported_functions()?;
 
-    if parsed.help {
-        print_supported_help(&supported, &parsed);
-        println!("\nSee https://asus-linux.org/faq/ for additional help");
-        std::process::exit(1);
-    }
-
     if parsed.version {
         println!("  asusctl v{}", env!("CARGO_PKG_VERSION"));
         println!(" rog-dbus v{}", rog_dbus::VERSION);
@@ -234,53 +227,6 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
     Ok(())
 }
 
-/// Print the root help for base commands if the commands are supported
-fn print_supported_help(supported: &SupportedFunctions, parsed: &CliStart) {
-    // As help option don't work with `parse_args_default`
-    // we will call `parse_args_default_or_exit` instead
-    let usage: Vec<String> = parsed.self_usage().lines().map(|s| s.to_string()).collect();
-    for line in usage.iter().filter(|line| {
-        if line.contains("--fan-mode") && !supported.fan_cpu_ctrl.stock_fan_modes {
-            return false;
-        }
-        if line.contains("--chg-limit") && !supported.charge_ctrl.charge_level_set {
-            return false;
-        }
-        true
-    }) {
-        println!("{}", line);
-    }
-
-    // command strings are in order of the struct
-    let commands: Vec<String> = CliCommand::usage().lines().map(|s| s.to_string()).collect();
-    println!("\nCommands available");
-    for line in commands.iter().filter(|line| {
-        if line.contains("profile") {
-            return supported.fan_cpu_ctrl.stock_fan_modes || supported.fan_cpu_ctrl.fan_curve_set;
-        }
-        if line.contains("led-mode") {
-            return supported.keyboard_led.stock_led_modes.is_empty();
-        }
-        if line.contains("bios") {
-            return supported.rog_bios_ctrl.dedicated_gfx_toggle
-                || supported.rog_bios_ctrl.post_sound_toggle;
-        }
-        if line.contains("anime") {
-            return supported.anime_ctrl.0;
-        }
-        false
-    }) {
-        println!("{}", line);
-    }
-
-    if !supported.fan_cpu_ctrl.stock_fan_modes {
-        println!("Note: Fan mode control is not supported by this laptop");
-    }
-    if !supported.charge_ctrl.charge_level_set {
-        println!("Note: Charge control is not supported by this laptop");
-    }
-}
-
 fn do_gfx(
     dbus: &RogDbusClient,
     supported: &RogBiosSupportedFunctions,
@@ -489,12 +435,21 @@ fn handle_profile(
     }
 
     let mut set_profile = false;
-    let mut profile;
+    let mut profile = Profile::default();
     if cmd.create {
-        profile = Profile::default();
         set_profile = true;
-    } else {
+    } else if let Some(ref name) = cmd.profile {
-        profile = dbus.proxies().profile().active_data()?;
+        let profiles = dbus.proxies().profile().all_profile_data()?;
+        for p in profiles {
+            if p.name == *name {
+                profile = p;
+                break;
+            }
+        }
+        if profile.name != *name {
+            println!("The requested profile doesn't exist, you may need to create it");
+            std::process::exit(-1);
+        }
     }
 
     if let Some(turbo) = cmd.turbo {

daemon/src/config_aura.rs

@@ -90,7 +90,7 @@ impl AuraConfig {
         let mut buf = String::new();
         if let Ok(read_len) = file.read_to_string(&mut buf) {
             if read_len == 0 {
-                return AuraConfig::create_default(&mut file, &supported_led_modes);
+                return AuraConfig::create_default(&mut file, supported_led_modes);
             } else {
                 if let Ok(data) = serde_json::from_str(&buf) {
                     return data;
@@ -109,7 +109,7 @@ impl AuraConfig {
                 panic!("Please remove {} then restart asusd", AURA_CONFIG_PATH);
             }
         }
-        AuraConfig::create_default(&mut file, &supported_led_modes)
+        AuraConfig::create_default(&mut file, supported_led_modes)
     }
 
     fn create_default(file: &mut File, support_data: &LaptopLedData) -> Self {

daemon/src/ctrl_gfx/controller.rs

@@ -8,7 +8,6 @@ use logind_zbus::{
     ManagerProxy, SessionProxy,
 };
 use rog_types::gfx_vendors::{GfxPower, GfxRequiredUserAction, GfxVendors};
-use std::iter::FromIterator;
 use std::{io::Write, ops::Add, path::Path, time::Instant};
 use std::{process::Command, thread::sleep, time::Duration};
 use std::{str::FromStr, sync::mpsc};
@@ -79,20 +78,20 @@ impl CtrlGraphics {
                 match dev.vendor()? {
                     0x1002 => {
                         info!("GFX: {}: AMD graphics", dev.id());
-                        amd.push(GraphicsDevice::new(dev.id().to_owned(), functions(&dev)));
+                        amd.push(GraphicsDevice::new(dev.id().to_owned(), functions(dev)));
                     }
                     0x10DE => {
                         info!("GFX: {}: NVIDIA graphics", dev.id());
                         dev.set_runtime_pm(sysfs_class::RuntimePowerManagement::On)?;
-                        nvidia.push(GraphicsDevice::new(dev.id().to_owned(), functions(&dev)));
+                        nvidia.push(GraphicsDevice::new(dev.id().to_owned(), functions(dev)));
                     }
                     0x8086 => {
                         info!("GFX: {}: Intel graphics", dev.id());
-                        intel.push(GraphicsDevice::new(dev.id().to_owned(), functions(&dev)));
+                        intel.push(GraphicsDevice::new(dev.id().to_owned(), functions(dev)));
                     }
                     vendor => {
                         info!("GFX: {}: Other({:X}) graphics", dev.id(), vendor);
-                        other.push(GraphicsDevice::new(dev.id().to_owned(), functions(&dev)));
+                        other.push(GraphicsDevice::new(dev.id().to_owned(), functions(dev)));
                     }
                 }
             }
@@ -265,13 +264,13 @@ impl CtrlGraphics {
         let unbinds = devices.iter().map(|dev| dev.unbind());
         // Remove NVIDIA graphics devices and their functions
         let removes = devices.iter().map(|dev| dev.remove());
-        Result::from_iter(unbinds.chain(removes))
+        unbinds.chain(removes).collect::<Result<_, _>>()
             .map_err(|err| RogError::Command("device unbind error".into(), err))
     }
 
     fn unbind_only(devices: &[GraphicsDevice]) -> Result<(), RogError> {
         let unbinds = devices.iter().map(|dev| dev.unbind());
-        Result::from_iter(unbinds)
+        unbinds.collect::<Result<_, _>>()
             .map_err(|err| RogError::Command("device unbind error".into(), err))
     }
 
@@ -462,7 +461,7 @@ impl CtrlGraphics {
                     for driver in NVIDIA_DRIVERS.iter() {
                         Self::do_driver_action(driver, "rmmod")?;
                     }
-                    Self::unbind_only(&devices)?;
+                    Self::unbind_only(devices)?;
                     Self::do_driver_action("vfio-pci", "modprobe")?;
                 } else {
                     return Err(GfxError::VfioDisabled.into());
@@ -478,7 +477,7 @@ impl CtrlGraphics {
                 for driver in NVIDIA_DRIVERS.iter() {
                     Self::do_driver_action(driver, "rmmod")?;
                 }
-                Self::unbind_remove_nvidia(&devices)?;
+                Self::unbind_remove_nvidia(devices)?;
             }
         }
         Ok(())

daemon/src/ctrl_gfx/system.rs

@@ -83,7 +83,7 @@ impl GraphicsDevice {
                     Ok(driver) => {
                         info!("{}: Unbinding {}", driver.id(), func.id());
                         unsafe {
-                            driver.unbind(&func).map_err(|err| {
+                            driver.unbind(func).map_err(|err| {
                                 error!("gfx unbind: {}", err);
                                 err
                             })?;
@@ -109,7 +109,7 @@ impl GraphicsDevice {
                     Ok(driver) => {
                         info!("{}: Binding {}", driver.id(), func.id());
                         unsafe {
-                            driver.bind(&func).map_err(|err| {
+                            driver.bind(func).map_err(|err| {
                                 error!("gfx bind: {}", err);
                                 err
                             })?;

daemon/src/ctrl_leds/controller.rs

@@ -370,7 +370,7 @@ impl CtrlKbdLed {
 
             self.config.read();
             if let Some(data) = self.config.builtins.get(&next) {
-                self.write_mode(&data)?;
+                self.write_mode(data)?;
                 self.config.current_mode = next;
             }
             self.config.write();
@@ -381,7 +381,7 @@ impl CtrlKbdLed {
 
     #[inline]
     fn write_mode(&self, mode: &AuraEffect) -> Result<(), RogError> {
-        if !self.supported_modes.standard.contains(&mode.mode()) {
+        if !self.supported_modes.standard.contains(mode.mode()) {
             return Err(RogError::NotSupported);
         }
         let bytes: [u8; LED_MSG_LEN] = mode.into();

daemon/src/ctrl_profiles/controller.rs

@@ -1,6 +1,6 @@
 use crate::error::RogError;
 use crate::{config::Config, GetSupported};
-use log::info;
+use log::{info, warn};
 use rog_profiles::profiles::Profile;
 use rog_types::supported::FanCpuSupportedFunctions;
 use std::sync::Arc;
@@ -23,12 +23,12 @@ impl GetSupported for CtrlFanAndCpu {
 }
 
 impl crate::Reloadable for CtrlFanAndCpu {
+    /// Fetcht he active profile and use that to set all related components up
     fn reload(&mut self) -> Result<(), RogError> {
         if let Ok(mut cfg) = self.config.clone().try_lock() {
             let active = cfg.active_profile.clone();
             if let Some(existing) = cfg.power_profiles.get_mut(&active) {
                 existing.set_system_all()?;
-                cfg.write();
             }
         }
         Ok(())
@@ -38,31 +38,38 @@ impl crate::Reloadable for CtrlFanAndCpu {
 impl CtrlFanAndCpu {
     pub fn new(config: Arc<Mutex<Config>>) -> Result<Self, RogError> {
         Profile::get_fan_path()?;
-        info!("Device has thermal throttle control");
+        info!("Device has fan control available");
         Ok(CtrlFanAndCpu { config })
     }
 
     /// Toggle to next profile in list
     pub(super) fn do_next_profile(&mut self) -> Result<(), RogError> {
         if let Ok(mut config) = self.config.clone().try_lock() {
+            // Read first just incase the user has modified the config before calling this
             config.read();
 
-            let mut i = config
+            let mut toggle_index = config
                 .toggle_profiles
                 .binary_search(&config.active_profile)
                 .unwrap_or(0)
                 + 1;
-            if i >= config.toggle_profiles.len() {
+            if toggle_index >= config.toggle_profiles.len() {
-                i = 0;
+                toggle_index = 0;
             }
 
-            let profile = config.toggle_profiles[i].clone();
+            let profile = config.toggle_profiles[toggle_index].clone();
 
             if let Some(existing) = config.power_profiles.get(&profile) {
                 existing.set_system_all()?;
                 config.active_profile = existing.name.clone();
                 config.write();
-                info!("Profile was changed to: {}", profile);
+                info!("Profile was changed to: {}", &profile);
+            } else {
+                warn!(
+                    "toggle_profile {} does not exist in power_profiles",
+                    &profile
+                );
+                return Err(RogError::MissingProfile(profile.to_string()));
             }
         }
         Ok(())
@@ -70,17 +77,25 @@ impl CtrlFanAndCpu {
 
     pub(super) fn set_active(&mut self, profile: &str) -> Result<(), RogError> {
         if let Ok(mut config) = self.config.clone().try_lock() {
+            // Read first just incase the user has modified the config before calling this
             config.read();
             if let Some(existing) = config.power_profiles.get(profile) {
                 existing.set_system_all()?;
                 config.active_profile = existing.name.clone();
                 config.write();
                 info!("Profile was changed to: {}", profile);
+            } else {
+                warn!(
+                    "toggle_profile {} does not exist in power_profiles",
+                    profile
+                );
+                return Err(RogError::MissingProfile(profile.to_string()));
             }
         }
         Ok(())
     }
 
+    /// Create a new profile if the requested name doesn't exist, or modify existing
     pub(super) fn new_or_modify(&mut self, profile: &Profile) -> Result<(), RogError> {
         if let Ok(mut config) = self.config.clone().try_lock() {
             config.read();

daemon/src/ctrl_profiles/zbus.rs

@@ -139,7 +139,7 @@ impl FanAndCpuZbus {
         if let Ok(ctrl) = self.inner.try_lock() {
             if let Ok(cfg) = ctrl.config.clone().try_lock() {
                 if let Some(profile) = cfg.power_profiles.get(&cfg.active_profile) {
-                    self.notify_profile(&profile)
+                    self.notify_profile(profile)
                         .unwrap_or_else(|err| warn!("{}", err));
                 }
             }

daemon/src/error.rs

@@ -8,7 +8,6 @@ use crate::ctrl_gfx::error::GfxError;
 
 #[derive(Debug)]
 pub enum RogError {
-    ParseFanLevel,
     ParseVendor,
     ParseLed,
     MissingProfile(String),
@@ -36,7 +35,6 @@ impl fmt::Display for RogError {
     // This trait requires `fmt` with this exact signature.
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match self {
-            RogError::ParseFanLevel => write!(f, "Parse profile error"),
             RogError::ParseVendor => write!(f, "Parse gfx vendor error"),
             RogError::ParseLed => write!(f, "Parse LED error"),
             RogError::MissingProfile(profile) => write!(f, "Profile does not exist {}", profile),

rog-aura/src/sequencer.rs

@@ -39,7 +39,7 @@ impl Sequences {
 
     pub fn iter(&self) -> ActionIterator {
         ActionIterator {
-            actions: &self,
+            actions: self,
             next_idx: 0,
         }
     }

rog-profiles/src/error.rs

@@ -6,7 +6,6 @@ use rog_fan_curve::CurveError;
 #[derive(Debug)]
 pub enum ProfileError {
     ParseFanLevel,
-    MissingProfile(String),
     Path(String, std::io::Error),
     Read(String, std::io::Error),
     Write(String, std::io::Error),
@@ -23,9 +22,6 @@ impl fmt::Display for ProfileError {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match self {
             ProfileError::ParseFanLevel => write!(f, "Parse profile error"),
-            ProfileError::MissingProfile(profile) => {
-                write!(f, "Profile does not exist {}", profile)
-            }
             ProfileError::Path(path, error) => write!(f, "Path {}: {}", path, error),
             ProfileError::Read(path, error) => write!(f, "Read {}: {}", path, error),
             ProfileError::Write(path, error) => write!(f, "Write {}: {}", path, error),