Correctly save multizone config

CHANGELOG.md

@@ -5,6 +5,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ## [Unreleased ]
+### Changed
+- Fixed save and restore of multizone LED settings
 
 ## [4.2.0] - 2022-07-16
 ### Added

daemon/src/ctrl_aura/config.rs

@@ -16,6 +16,7 @@ pub struct AuraConfig {
     pub current_mode: AuraModeNum,
     pub builtins: BTreeMap<AuraModeNum, AuraEffect>,
     pub multizone: Option<BTreeMap<AuraModeNum, Vec<AuraEffect>>>,
+    pub multizone_on: bool,
     pub enabled: HashSet<AuraControl>,
 }
 
@@ -26,6 +27,7 @@ impl Default for AuraConfig {
             current_mode: AuraModeNum::Static,
             builtins: BTreeMap::new(),
             multizone: None,
+            multizone_on: false,
             enabled: HashSet::from([
                 AuraControl::BootLogo,
                 AuraControl::BootKeyb,
@@ -123,27 +125,35 @@ impl AuraConfig {
             .unwrap_or_else(|err| error!("Could not write config: {}", err));
     }
 
-    /// Multipurpose, will accept AuraEffect with zones and put in the correct store
+    /// Set the mode data, current mode, and if multizone enabled.
+    ///
+    /// Multipurpose, will accept AuraEffect with zones and put in the correct store.
     pub fn set_builtin(&mut self, effect: AuraEffect) {
+        self.current_mode = effect.mode;
         match effect.zone() {
             AuraZone::None => {
                 self.builtins.insert(*effect.mode(), effect);
+                self.multizone_on = false;
             }
             _ => {
                 if let Some(multi) = self.multizone.as_mut() {
                     if let Some(fx) = multi.get_mut(effect.mode()) {
                         for fx in fx.iter_mut() {
-                            if fx.mode == effect.mode {
+                            if fx.zone == effect.zone {
                                 *fx = effect;
-                                break;
+                                return;
                             }
                         }
+                        fx.push(effect);
                     } else {
-                        let mut tmp = BTreeMap::new();
+                        multi.insert(*effect.mode(), vec![effect]);
-                        tmp.insert(*effect.mode(), vec![effect]);
-                        self.multizone = Some(tmp);
                     }
+                } else {
+                    let mut tmp = BTreeMap::new();
+                    tmp.insert(*effect.mode(), vec![effect]);
+                    self.multizone = Some(tmp);
                 }
+                self.multizone_on = true;
             }
         }
     }
@@ -155,3 +165,86 @@ impl AuraConfig {
         None
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::AuraConfig;
+    use rog_aura::{AuraEffect, AuraModeNum, AuraZone, Colour};
+
+    #[test]
+    fn set_multizone_4key_config() {
+        let mut config = AuraConfig::default();
+
+        let mut effect = AuraEffect::default();
+        effect.colour1 = Colour(0xff, 0x00, 0xff);
+        effect.zone = AuraZone::Key1;
+        config.set_builtin(effect);
+
+        assert!(config.multizone.is_some());
+
+        let mut effect = AuraEffect::default();
+        effect.colour1 = Colour(0x00, 0xff, 0xff);
+        effect.zone = AuraZone::Key2;
+        config.set_builtin(effect);
+
+        let mut effect = AuraEffect::default();
+        effect.colour1 = Colour(0xff, 0xff, 0x00);
+        effect.zone = AuraZone::Key3;
+        config.set_builtin(effect);
+
+        let mut effect = AuraEffect::default();
+        effect.colour1 = Colour(0x00, 0xff, 0x00);
+        effect.zone = AuraZone::Key4;
+        let effect_clone = effect.clone();
+        config.set_builtin(effect);
+        // This should replace existing
+        config.set_builtin(effect_clone);
+
+        let res = config.multizone.unwrap();
+        let sta = res.get(&AuraModeNum::Static).unwrap();
+        assert_eq!(sta.len(), 4);
+        assert_eq!(sta[0].colour1, Colour(0xff, 0x00, 0xff));
+        assert_eq!(sta[1].colour1, Colour(0x00, 0xff, 0xff));
+        assert_eq!(sta[2].colour1, Colour(0xff, 0xff, 0x00));
+        assert_eq!(sta[3].colour1, Colour(0x00, 0xff, 0x00));
+    }
+
+    #[test]
+    fn set_multizone_multimode_config() {
+        let mut config = AuraConfig::default();
+
+        let mut effect = AuraEffect::default();
+        effect.zone = AuraZone::Key1;
+        config.set_builtin(effect);
+
+        assert!(config.multizone.is_some());
+
+        let mut effect = AuraEffect::default();
+        effect.zone = AuraZone::Key2;
+        effect.mode = AuraModeNum::Breathe;
+        config.set_builtin(effect);
+
+        let mut effect = AuraEffect::default();
+        effect.zone = AuraZone::Key3;
+        effect.mode = AuraModeNum::Comet;
+        config.set_builtin(effect);
+
+        let mut effect = AuraEffect::default();
+        effect.zone = AuraZone::Key4;
+        effect.mode = AuraModeNum::Pulse;
+        config.set_builtin(effect);
+
+        let res = config.multizone.unwrap();
+        let sta = res.get(&AuraModeNum::Static).unwrap();
+        assert_eq!(sta.len(), 1);
+
+        let sta = res.get(&AuraModeNum::Breathe).unwrap();
+        assert_eq!(sta.len(), 1);
+
+        let sta = res.get(&AuraModeNum::Comet).unwrap();
+        assert_eq!(sta.len(), 1);
+
+        let sta = res.get(&AuraModeNum::Pulse).unwrap();
+        assert_eq!(sta.len(), 1);
+    }
+}

daemon/src/ctrl_aura/controller.rs

@@ -9,7 +9,7 @@ use crate::{
 use async_trait::async_trait;
 use log::{error, info, warn};
 use logind_zbus::manager::ManagerProxy;
-use rog_aura::usb::AuraControl;
+use rog_aura::{usb::AuraControl, AuraZone};
 use rog_aura::{
     usb::{LED_APPLY, LED_SET},
     AuraEffect, LedBrightness, LED_MSG_LEN,
@@ -106,11 +106,9 @@ impl CtrlTask for CtrlKbdLedTask {
                 lock.set_brightness(lock.config.brightness)
                     .map_err(|e| error!("CtrlKbdLedTask: {e}"))
                     .ok();
-                if let Some(mode) = lock.config.builtins.get(&lock.config.current_mode) {
+                lock.write_current_config_mode()
-                    lock.write_mode(mode)
+                    .map_err(|e| error!("CtrlKbdLedTask: {e}"))
-                        .map_err(|e| error!("CtrlKbdLedTask: {e}"))
+                    .ok();
-                        .ok();
-                }
             } else if start {
                 info!("CtrlKbdLedTask saving last brightness");
                 Self::update_config(&mut lock)
@@ -154,15 +152,6 @@ impl CtrlTask for CtrlKbdLedTask {
                 }
             })
             .detach();
-
-        // let inner = self.inner.clone();
-        // self.repeating_task(500, executor, move || loop {
-        //     if let Ok(ref mut lock) = inner.try_lock() {
-        //         Self::update_config(lock).unwrap();
-        //         break;
-        //     }
-        // })
-        // .await;
         Ok(())
     }
 }
@@ -171,12 +160,8 @@ pub struct CtrlKbdLedReloader(pub Arc<Mutex<CtrlKbdLed>>);
 
 impl crate::Reloadable for CtrlKbdLedReloader {
     fn reload(&mut self) -> Result<(), RogError> {
-        if let Ok(mut ctrl) = self.0.try_lock() {
+        if let Ok(ctrl) = self.0.try_lock() {
-            let current = ctrl.config.current_mode;
+            ctrl.write_current_config_mode()?;
-            if let Some(mode) = ctrl.config.builtins.get(&current).cloned() {
-                ctrl.do_command(mode).ok();
-            }
-
             ctrl.set_power_states(&ctrl.config)
                 .map_err(|err| warn!("{err}"))
                 .ok();
@@ -211,11 +196,8 @@ impl CtrlKbdLed {
 
         let bright_node = Self::get_kbd_bright_path();
 
-        if led_node.is_none() && bright_node.is_none() {
+        if led_node.is_none() {
-            return Err(RogError::MissingFunction(
+            return Err(RogError::NoAuraKeyboard);
-                "All keyboard features missing, you may require a v5.11 series kernel or newer"
-                    .into(),
-            ));
         }
 
         if bright_node.is_none() {
@@ -301,7 +283,6 @@ impl CtrlKbdLed {
         let set: Vec<AuraControl> = config.enabled.iter().map(|v| *v).collect();
         let bytes = AuraControl::to_bytes(&set);
 
-        // Quite ugly, must be a more idiomatic way to do
         let message = [
             0x5d, 0xbd, 0x01, bytes[0], bytes[1], 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
         ];
@@ -351,8 +332,24 @@ impl CtrlKbdLed {
         ))
     }
 
-    pub(crate) fn do_command(&mut self, mode: AuraEffect) -> Result<(), RogError> {
+    /// Set an Aura effect if the effect mode or zone is supported.
-        self.set_and_save(mode)
+    ///
+    /// On success the aura config file is read to refresh cached values, then the effect is
+    /// stored and config written to disk.
+    pub(crate) fn set_effect(&mut self, effect: AuraEffect) -> Result<(), RogError> {
+        if !self.supported_modes.standard.contains(&effect.mode) {
+            return Err(RogError::AuraEffectNotSupported);
+        } else if effect.zone != AuraZone::None
+            && !self.supported_modes.multizone.contains(&effect.zone)
+        {
+            return Err(RogError::AuraEffectNotSupported);
+        }
+
+        self.write_mode(&effect)?;
+        self.config.read(); // refresh config if successful
+        self.config.set_builtin(effect);
+        self.config.write();
+        Ok(())
     }
 
     /// Should only be used if the bytes you are writing are verified correct
@@ -366,10 +363,10 @@ impl CtrlKbdLed {
                     .map_err(|err| RogError::Write("write_bytes".into(), err));
             }
         }
-        Err(RogError::NotSupported)
+        Err(RogError::NoAuraNode)
     }
 
-    /// Write an effect block
+    /// Write an effect block. This is for per-key
     #[inline]
     fn _write_effect(&mut self, effect: &[Vec<u8>]) -> Result<(), RogError> {
         if self.flip_effect_write {
@@ -385,19 +382,6 @@ impl CtrlKbdLed {
         Ok(())
     }
 
-    /// Used to set a builtin mode and save the settings for it
-    ///
-    /// This needs to be universal so that settings applied by dbus stick
-    #[inline]
-    fn set_and_save(&mut self, mode: AuraEffect) -> Result<(), RogError> {
-        self.config.read();
-        self.write_mode(&mode)?;
-        self.config.current_mode = *mode.mode();
-        self.config.set_builtin(mode);
-        self.config.write();
-        Ok(())
-    }
-
     #[inline]
     pub(super) fn toggle_mode(&mut self, reverse: bool) -> Result<(), RogError> {
         let current = self.config.current_mode;
@@ -424,9 +408,9 @@ impl CtrlKbdLed {
             let next = self.supported_modes.standard[idx];
 
             self.config.read();
-            if let Some(data) = self.config.builtins.get(&next) {
+            if self.config.builtins.contains_key(&next) {
-                self.write_mode(data)?;
                 self.config.current_mode = next;
+                self.write_current_config_mode()?;
             }
             self.config.write();
         }
@@ -436,9 +420,6 @@ impl CtrlKbdLed {
 
     #[inline]
     fn write_mode(&self, mode: &AuraEffect) -> Result<(), RogError> {
-        if !self.supported_modes.standard.contains(mode.mode()) {
-            return Err(RogError::NotSupported);
-        }
         let bytes: [u8; LED_MSG_LEN] = mode.into();
         self.write_bytes(&bytes)?;
         self.write_bytes(&LED_SET)?;
@@ -446,4 +427,89 @@ impl CtrlKbdLed {
         self.write_bytes(&LED_APPLY)?;
         Ok(())
     }
+
+    #[inline]
+    fn write_current_config_mode(&self) -> Result<(), RogError> {
+        if self.config.multizone_on {
+            let mode = self.config.current_mode;
+            if let Some(multizones) = self.config.multizone.as_ref() {
+                if let Some(set) = multizones.get(&mode) {
+                    for mode in set {
+                        self.write_mode(mode)?;
+                    }
+                }
+            }
+        } else {
+            let mode = self.config.current_mode;
+            if let Some(effect) = self.config.builtins.get(&mode) {
+                self.write_mode(effect)?;
+            }
+        }
+
+        Ok(())
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use rog_aura::{AuraEffect, AuraModeNum, AuraZone, Colour};
+
+    use crate::{ctrl_aura::config::AuraConfig, laptops::LaptopLedData};
+
+    use super::CtrlKbdLed;
+
+    #[test]
+    fn check_set_mode_errors() {
+        // Checking to ensure set_mode errors when unsupported modes are tried
+        let config = AuraConfig::default();
+        let supported_modes = LaptopLedData {
+            prod_family: "".into(),
+            board_names: vec![],
+            standard: vec![AuraModeNum::Static],
+            multizone: vec![],
+            per_key: false,
+        };
+        let mut controller = CtrlKbdLed::new(supported_modes, config).unwrap();
+
+        let mut effect = AuraEffect::default();
+        effect.colour1 = Colour(0xff, 0x00, 0xff);
+        effect.zone = AuraZone::None;
+
+        // This error comes from write_bytes because we don't have a keyboard node stored
+        assert_eq!(
+            controller
+                .set_effect(effect.clone())
+                .unwrap_err()
+                .to_string(),
+            "No Aura keyboard node found"
+        );
+
+        effect.mode = AuraModeNum::Laser;
+        assert_eq!(
+            controller
+                .set_effect(effect.clone())
+                .unwrap_err()
+                .to_string(),
+            "Aura efect not supported"
+        );
+
+        effect.mode = AuraModeNum::Static;
+        effect.zone = AuraZone::Key2;
+        assert_eq!(
+            controller
+                .set_effect(effect.clone())
+                .unwrap_err()
+                .to_string(),
+            "Aura efect not supported"
+        );
+
+        controller.supported_modes.multizone.push(AuraZone::Key2);
+        assert_eq!(
+            controller
+                .set_effect(effect.clone())
+                .unwrap_err()
+                .to_string(),
+            "No Aura keyboard node found"
+        );
+    }
 }

daemon/src/ctrl_aura/zbus.rs

@@ -103,7 +103,7 @@ impl CtrlKbdLedZbus {
     ) {
         let mut led = None;
         if let Ok(mut ctrl) = self.0.try_lock() {
-            match ctrl.do_command(effect) {
+            match ctrl.set_effect(effect) {
                 Ok(_) => {
                     if let Some(mode) = ctrl.config.builtins.get(&ctrl.config.current_mode) {
                         led = Some(mode.clone());

daemon/src/error.rs

@@ -23,6 +23,9 @@ pub enum RogError {
     Io(std::io::Error),
     Zbus(zbus::Error),
     ChargeLimit(u8),
+    AuraEffectNotSupported,
+    NoAuraKeyboard,
+    NoAuraNode,
 }
 
 impl fmt::Display for RogError {
@@ -48,6 +51,9 @@ impl fmt::Display for RogError {
             RogError::Io(detail) => write!(f, "std::io error: {}", detail),
             RogError::Zbus(detail) => write!(f, "Zbus error: {}", detail),
             RogError::ChargeLimit(value) => write!(f, "Invalid charging limit, not in range 20-100%: {}", value),
+            RogError::AuraEffectNotSupported => write!(f, "Aura efect not supported"),
+            RogError::NoAuraKeyboard => write!(f, "No supported Aura keyboard"),
+            RogError::NoAuraNode => write!(f, "No Aura keyboard node found"),
         }
     }
 }