refactor(rog-platform, asusd): simplify sysfs attribute parsing and error handling

asusd/src/asus_armoury.rs

@@ -65,12 +65,7 @@ impl AsusArmouryAttribute {
     }
 
     fn resolve_i32_value(refreshed: Option<i32>, cached: &AttrValue) -> i32 {
-        refreshed
-            .or(match cached {
-                AttrValue::Integer(i) => Some(*i),
-                _ => None,
-            })
-            .unwrap_or(-1)
+        refreshed.or_else(|| cached.as_i32()).unwrap_or(-1)
     }
 
     pub async fn emit_limits(&self, connection: &Connection) -> Result<(), RogError> {
@@ -291,18 +286,17 @@ impl AsusArmouryAttribute {
         match self.attr.current_value() {
             Ok(value) => {
                 self.logged_read_error.store(false, Ordering::Relaxed);
-                if matches!(value, AttrValue::Integer(_)) {
+                if value.as_i32().is_some() {
                     attrs.push("current_value".to_string());
                 }
             }
-            Err(e) => {
-                if !self.logged_read_error.swap(true, Ordering::Relaxed) {
-                    error!(
-                        "Firmware attribute '{}' is not supported or failed to read: {e:?}",
-                        self.attr.name()
-                    );
-                }
+            Err(e) if !self.logged_read_error.swap(true, Ordering::Relaxed) => {
+                error!(
+                    "Firmware attribute '{}' is not supported or failed to read: {e:?}",
+                    self.attr.name()
+                );
             }
+            _ => {}
         }
         attrs
     }
@@ -332,8 +326,8 @@ impl AsusArmouryAttribute {
             let mut config = self.config.lock().await;
             let tuning = config.select_tunings(power_plugged == 1, profile);
             if let Some(tune) = tuning.group.get_mut(&self.name()) {
-                if let AttrValue::Integer(i) = self.attr.default_value() {
-                    *tune = *i;
+                if let Some(i) = self.attr.default_value().as_i32() {
+                    *tune = i;
                 }
             }
             if tuning.enabled {
@@ -394,8 +388,8 @@ impl AsusArmouryAttribute {
                     return Ok(*tune);
                 }
             }
-            if let AttrValue::Integer(i) = self.attr.default_value() {
-                return Ok(*i);
+            if let Some(i) = self.attr.default_value().as_i32() {
+                return Ok(i);
             }
             return Err(fdo::Error::Failed(
                 "Could not read current value".to_string(),
@@ -415,7 +409,7 @@ impl AsusArmouryAttribute {
 
         if let Ok(value) = self.attr.current_value() {
             self.logged_read_error.store(false, Ordering::Relaxed);
-            if let AttrValue::Integer(i) = value {
+            if let Some(i) = value.as_i32() {
                 return Ok(i);
             }
         }
@@ -673,8 +667,8 @@ pub async fn set_config_or_default(
                             error!("Failed to set {}: {e}", <&str>::from(name));
                         })
                         .ok();
-                    if let AttrValue::Integer(i) = default {
-                        tuning.group.insert(name, *i);
+                    if let Some(i) = default.as_i32() {
+                        tuning.group.insert(name, i);
                         info!(
                             "Set default tuning config for {} = {:?}",
                             <&str>::from(name),

rog-aura/src/aura_detection.rs

@@ -133,13 +133,18 @@ impl LedSupportFile {
             if file.is_empty() {
                 warn!("{} is empty", ASUS_LED_MODE_USER_CONF);
             } else {
-                if let Ok(mut tmp) = ron::from_str::<LedSupportFile>(&file) {
-                    data.0.append(&mut tmp.0);
+                match ron::from_str::<LedSupportFile>(&file) {
+                    Ok(mut tmp) => {
+                        data.0.append(&mut tmp.0);
+                        info!(
+                            "Loaded user-defined LED support data from {}",
+                            ASUS_LED_MODE_USER_CONF
+                        );
+                    }
+                    Err(e) => {
+                        error!("Could not deserialise {}: {}", ASUS_LED_MODE_USER_CONF, e);
+                    }
                 }
-                info!(
-                    "Loaded user-defined LED support data from {}",
-                    ASUS_LED_MODE_USER_CONF
-                );
             }
         }
         // Load and append the default LED support data

rog-platform/src/asus_armoury.rs

@@ -13,15 +13,9 @@ use crate::error::PlatformError;
 const BASE_DIR: &str = "/sys/class/firmware-attributes/asus-armoury/attributes/";
 
 fn read_i32(path: &Path) -> Result<i32, PlatformError> {
-    if let Ok(mut f) = File::open(path) {
-        let mut buf = String::new();
-        f.read_to_string(&mut buf)?;
-        buf.trim()
-            .parse::<i32>()
-            .map_err(|_| PlatformError::ParseNum)
-    } else {
-        Err(PlatformError::ParseNum)
-    }
+    read_string(path)?
+        .parse::<i32>()
+        .map_err(|_| PlatformError::ParseNum)
 }
 
 fn read_string(path: &Path) -> Result<String, PlatformError> {
@@ -41,6 +35,22 @@ pub enum AttrValue {
     None,
 }
 
+impl AttrValue {
+    pub fn as_i32(&self) -> Option<i32> {
+        match self {
+            Self::Integer(val) => Some(*val),
+            _ => None,
+        }
+    }
+}
+
+impl From<String> for AttrValue {
+    fn from(val: String) -> Self {
+        val.parse::<i32>()
+            .map(AttrValue::Integer)
+            .unwrap_or_else(|_| AttrValue::String(val))
+    }
+}
 #[derive(Debug, Default, Clone)]
 pub struct Attribute {
     name: String,
@@ -64,16 +74,7 @@ impl Attribute {
 
     /// Read the `current_value` directly from the attribute path
     pub fn current_value(&self) -> Result<AttrValue, PlatformError> {
-        match read_string(&self.base_path.join("current_value")) {
-            Ok(val) => {
-                if let Ok(int) = val.parse::<i32>() {
-                    Ok(AttrValue::Integer(int))
-                } else {
-                    Ok(AttrValue::String(val))
-                }
-            }
-            Err(e) => Err(e),
-        }
+        read_string(&self.base_path.join("current_value")).map(AttrValue::from)
     }
 
     pub fn base_path_exists(&self) -> bool {
@@ -146,27 +147,26 @@ impl Attribute {
     fn read_base_values(
         base_path: &Path,
     ) -> (AttrValue, AttrValue, AttrValue, AttrValue, AttrValue) {
-        let default_value = match read_string(&base_path.join("default_value")) {
-            Ok(val) => {
-                if let Ok(int) = val.parse::<i32>() {
-                    AttrValue::Integer(int)
-                } else {
-                    AttrValue::String(val)
-                }
-            }
-            Err(_) => AttrValue::None,
-        };
+        let default_value = read_string(&base_path.join("default_value"))
+            .map(AttrValue::from)
+            .unwrap_or_default();
 
         let possible_values = match read_string(&base_path.join("possible_values")) {
-            Ok(val) => {
-                if let Ok(int) = val.parse::<i32>() {
-                    AttrValue::Integer(int)
-                } else if val.contains(';') {
-                    AttrValue::EnumInt(val.split(';').filter_map(|s| s.parse().ok()).collect())
-                } else {
-                    AttrValue::EnumStr(val.split(';').map(|s| s.to_string()).collect())
+            Ok(val) => match val.parse::<i32>() {
+                Ok(int) => AttrValue::Integer(int),
+                Err(_) => {
+                    let tokens: Vec<&str> = val.split(';').collect();
+                    if let Ok(ints) = tokens
+                        .iter()
+                        .map(|s| s.parse::<i32>())
+                        .collect::<Result<Vec<i32>, _>>()
+                    {
+                        AttrValue::EnumInt(ints)
+                    } else {
+                        AttrValue::EnumStr(tokens.into_iter().map(String::from).collect())
+                    }
                 }
-            }
+            },
             Err(_) => AttrValue::None,
         };
 
@@ -566,4 +566,77 @@ mod tests {
         }
         attr.set_current_value(&val).unwrap();
     }
+
+    struct TestDir(PathBuf);
+
+    impl TestDir {
+        fn new(name: &str) -> Self {
+            let dir = std::env::temp_dir().join(format!("{name}_{}", std::process::id()));
+            let _ = std::fs::remove_dir_all(&dir);
+            std::fs::create_dir_all(&dir).expect("failed to create test dir");
+            Self(dir)
+        }
+
+        fn path(&self) -> &Path {
+            &self.0
+        }
+
+        fn join(&self, path: &str) -> PathBuf {
+            self.0.join(path)
+        }
+    }
+
+    impl Drop for TestDir {
+        fn drop(&mut self) {
+            let _ = std::fs::remove_dir_all(&self.0);
+        }
+    }
+
+    #[test]
+    fn test_possible_values_parsing() {
+        let test_dir = TestDir::new("test_possible_values_parsing");
+        let possible_path = test_dir.join("possible_values");
+
+        // 1. All integer tokens
+        std::fs::write(&possible_path, "0;1;2\n").expect("Failed to write possible_values");
+        let (_, possible, _, _, _) = Attribute::read_base_values(test_dir.path());
+        assert_eq!(possible, AttrValue::EnumInt(vec![0, 1, 2]));
+
+        // 2. All string tokens
+        std::fs::write(&possible_path, "Disabled;Enabled\n")
+            .expect("Failed to write possible_values");
+        let (_, possible, _, _, _) = Attribute::read_base_values(test_dir.path());
+        assert_eq!(
+            possible,
+            AttrValue::EnumStr(vec![
+                "Disabled".to_string(),
+                "Enabled".to_string()
+            ])
+        );
+
+        // 3. Mixed string and int tokens (preserves all tokens without dropping non-integers)
+        std::fs::write(&possible_path, "0;Disabled;2\n").expect("Failed to write possible_values");
+        let (_, possible, _, _, _) = Attribute::read_base_values(test_dir.path());
+        assert_eq!(
+            possible,
+            AttrValue::EnumStr(vec![
+                "0".to_string(),
+                "Disabled".to_string(),
+                "2".to_string()
+            ])
+        );
+
+        // 4. Single integer token
+        std::fs::write(&possible_path, "42\n").expect("Failed to write possible_values");
+        let (_, possible, _, _, _) = Attribute::read_base_values(test_dir.path());
+        assert_eq!(possible, AttrValue::Integer(42));
+
+        // 5. Single string token
+        std::fs::write(&possible_path, "Performance\n").expect("Failed to write possible_values");
+        let (_, possible, _, _, _) = Attribute::read_base_values(test_dir.path());
+        assert_eq!(
+            possible,
+            AttrValue::EnumStr(vec!["Performance".to_string()])
+        );
+    }
 }

rog-platform/src/cpu.rs

@@ -6,7 +6,7 @@ use zbus::zvariant::{OwnedValue, Type, Value};
 
 use crate::error::{PlatformError, Result};
 use crate::platform::PlatformProfile;
-use crate::{read_attr_string, to_device};
+use crate::{read_attr_string, read_sysfs_parsed, to_device};
 
 const ATTR_AVAILABLE_GOVERNORS: &str = "cpufreq/scaling_available_governors";
 const ATTR_GOVERNOR: &str = "cpufreq/scaling_governor";
@@ -317,21 +317,16 @@ pub fn get_cpu_temp() -> f32 {
             if let Ok(name) = std::fs::read_to_string(path.join("name")) {
                 let name = name.trim();
                 if name == "k10temp" || name == "coretemp" || name == "zenpower" {
-                    if let Ok(temp_str) = std::fs::read_to_string(path.join("temp1_input")) {
-                        if let Ok(temp_val) = temp_str.trim().parse::<f32>() {
-                            return temp_val / 1000.0;
-                        }
+                    if let Some(temp_val) = read_sysfs_parsed::<f32>(path.join("temp1_input")) {
+                        return temp_val / 1000.0;
                     }
                 }
             }
         }
     }
-    if let Ok(temp_str) = std::fs::read_to_string("/sys/class/thermal/thermal_zone0/temp") {
-        if let Ok(temp_val) = temp_str.trim().parse::<f32>() {
-            return temp_val / 1000.0;
-        }
-    }
-    0.0
+    read_sysfs_parsed::<f32>("/sys/class/thermal/thermal_zone0/temp")
+        .map(|t| t / 1000.0)
+        .unwrap_or(-1.0)
 }
 
 pub fn get_cpu_frequency_mhz() -> f32 {
@@ -342,11 +337,9 @@ pub fn get_cpu_frequency_mhz() -> f32 {
             let name = entry.file_name().to_string_lossy().into_owned();
             if name.starts_with("cpu") && name[3..].chars().all(|c| c.is_ascii_digit()) {
                 let freq_path = entry.path().join("cpufreq/scaling_cur_freq");
-                if let Ok(freq_str) = std::fs::read_to_string(freq_path) {
-                    if let Ok(freq_khz) = freq_str.trim().parse::<f32>() {
-                        total_freq += freq_khz / 1000.0;
-                        count += 1;
-                    }
+                if let Some(freq_khz) = read_sysfs_parsed::<f32>(freq_path) {
+                    total_freq += freq_khz / 1000.0;
+                    count += 1;
                 }
             }
         }
@@ -368,7 +361,7 @@ pub fn get_cpu_frequency_mhz() -> f32 {
     if count > 0 {
         total_freq / count as f32
     } else {
-        0.0
+        -1.0
     }
 }