From fee1486db6fcbed0713ddf6484e89d7fbfc484ed Mon Sep 17 00:00:00 2001 From: "Luke D. Jones" Date: Sun, 17 Jul 2022 12:18:55 +1200 Subject: [PATCH] Correctly save multizone config --- CHANGELOG.md | 2 + daemon/src/ctrl_aura/config.rs | 105 ++++++++++++++++-- daemon/src/ctrl_aura/controller.rs | 164 ++++++++++++++++++++--------- daemon/src/ctrl_aura/zbus.rs | 2 +- daemon/src/error.rs | 6 ++ rog-aura/src/usb.rs | 4 +- 6 files changed, 225 insertions(+), 58 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 63153fa4..2323d0d6 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/daemon/src/ctrl_aura/config.rs b/daemon/src/ctrl_aura/config.rs index 7e7d9543..e86a2f0d 100644 --- a/daemon/src/ctrl_aura/config.rs +++ b/daemon/src/ctrl_aura/config.rs @@ -16,6 +16,7 @@ pub struct AuraConfig { pub current_mode: AuraModeNum, pub builtins: BTreeMap, pub multizone: Option>>, + pub multizone_on: bool, pub enabled: HashSet, } @@ -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(); - tmp.insert(*effect.mode(), vec![effect]); - self.multizone = Some(tmp); + multi.insert(*effect.mode(), vec![effect]); } + } 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); + } +} diff --git a/daemon/src/ctrl_aura/controller.rs b/daemon/src/ctrl_aura/controller.rs index 734bbb39..c08a88c0 100644 --- a/daemon/src/ctrl_aura/controller.rs +++ b/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_mode(mode) - .map_err(|e| error!("CtrlKbdLedTask: {e}")) - .ok(); - } + lock.write_current_config_mode() + .map_err(|e| error!("CtrlKbdLedTask: {e}")) + .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>); impl crate::Reloadable for CtrlKbdLedReloader { fn reload(&mut self) -> Result<(), RogError> { - if let Ok(mut ctrl) = self.0.try_lock() { - let current = ctrl.config.current_mode; - if let Some(mode) = ctrl.config.builtins.get(¤t).cloned() { - ctrl.do_command(mode).ok(); - } - + if let Ok(ctrl) = self.0.try_lock() { + ctrl.write_current_config_mode()?; 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() { - return Err(RogError::MissingFunction( - "All keyboard features missing, you may require a v5.11 series kernel or newer" - .into(), - )); + if led_node.is_none() { + return Err(RogError::NoAuraKeyboard); } if bright_node.is_none() { @@ -301,7 +283,6 @@ impl CtrlKbdLed { let set: Vec = 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> { - self.set_and_save(mode) + /// Set an Aura effect if the effect mode or zone is supported. + /// + /// 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]) -> 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) { - self.write_mode(data)?; + if self.config.builtins.contains_key(&next) { 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" + ); + } } diff --git a/daemon/src/ctrl_aura/zbus.rs b/daemon/src/ctrl_aura/zbus.rs index 79f33447..16e23b7d 100644 --- a/daemon/src/ctrl_aura/zbus.rs +++ b/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()); diff --git a/daemon/src/error.rs b/daemon/src/error.rs index 14f254ab..7c71ebde 100644 --- a/daemon/src/error.rs +++ b/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"), } } } diff --git a/rog-aura/src/usb.rs b/rog-aura/src/usb.rs index b4acc550..749db528 100644 --- a/rog-aura/src/usb.rs +++ b/rog-aura/src/usb.rs @@ -25,7 +25,7 @@ pub const fn aura_brightness_bytes(brightness: u8) -> [u8; 17] { /// booting. /// /// # Bits for 0x19b6 keyboard model -/// +/// /// ```text /// byte 4 in the USB packet is for keyboard + logo power states /// default is on, `ff` @@ -51,7 +51,7 @@ pub const fn aura_brightness_bytes(brightness: u8) -> [u8; 17] { /// | 0001 | 0000 | 10 | lightbar shtdn off | bit 5 | /// /// # Bits for older 0x1866 keyboard model -/// +/// /// Keybord and Light zone require Awake /// | byte 1 | byte 2 | byte 3 | | | /// | 1 | 2 | 3 | 4 | 5 | 6 | function | hex |