From 8332649e45f43c7df8db2336554db4cfdae8ff63 Mon Sep 17 00:00:00 2001 From: "NGnius (Graham)" Date: Mon, 16 Jan 2023 18:46:40 -0500 Subject: [PATCH] Disallow setting max < min and min > max because the kernel now does too --- backend/src/api/cpu.rs | 14 ++++- backend/src/api/gpu.rs | 16 ++++- backend/src/api/handler.rs | 121 +++++++++++++++++++++++++----------- src/components/cpus.tsx | 124 +++++++++++++++++++------------------ src/components/gpu.tsx | 40 ++++++------ 5 files changed, 194 insertions(+), 121 deletions(-) diff --git a/backend/src/api/cpu.rs b/backend/src/api/cpu.rs index 02e2f1e..1c4d35f 100644 --- a/backend/src/api/cpu.rs +++ b/backend/src/api/cpu.rs @@ -159,8 +159,18 @@ pub fn set_clock_limits( if let Some(&Primitive::F64(index)) = params_in.get(0) { if let Some(&Primitive::F64(min)) = params_in.get(1) { if let Some(&Primitive::F64(max)) = params_in.get(2) { - setter(index as usize, MinMax {min: min as u64, max: max as u64}); - vec![min.into(), max.into()] + let safe_max = if max < min { + min + } else { + max + }; + let safe_min = if min > max { + max + } else { + min + }; + setter(index as usize, MinMax {min: safe_min as u64, max: safe_max as u64}); + vec![safe_min.into(), safe_max.into()] } else { vec!["set_clock_limits missing parameter 2".into()] } diff --git a/backend/src/api/gpu.rs b/backend/src/api/gpu.rs index e23ba85..819eb89 100644 --- a/backend/src/api/gpu.rs +++ b/backend/src/api/gpu.rs @@ -78,11 +78,21 @@ pub fn set_clock_limits( move |params_in: super::ApiParameterType| { if let Some(&Primitive::F64(min)) = params_in.get(0) { if let Some(&Primitive::F64(max)) = params_in.get(1) { + let safe_max = if max < min { + min + } else { + max + }; + let safe_min = if min > max { + max + } else { + min + }; setter(MinMax { - min: min as _, - max: max as _, + min: safe_min as _, + max: safe_max as _, }); - vec![(min as u64).into(), (max as u64).into()] + vec![(safe_min as u64).into(), (safe_max as u64).into()] } else { vec!["set_clock_limits missing parameter 1".into()] } diff --git a/backend/src/api/handler.rs b/backend/src/api/handler.rs index e7672e6..2b9cd60 100644 --- a/backend/src/api/handler.rs +++ b/backend/src/api/handler.rs @@ -32,7 +32,8 @@ pub enum BatteryMessage { } impl BatteryMessage { - fn process(self, settings: &mut dyn TBattery) { + fn process(self, settings: &mut dyn TBattery) -> bool { + let dirty = self.is_modify(); match self { Self::SetChargeRate(rate) => settings.charge_rate(rate), Self::GetChargeRate(cb) => cb(settings.get_charge_rate()), @@ -43,6 +44,12 @@ impl BatteryMessage { Self::ReadChargeDesign(cb) => cb(settings.read_charge_design()), Self::ReadCurrentNow(cb) => cb(settings.read_current_now()), } + dirty + } + + /// Message instructs the driver to modify settings + fn is_modify(&self) -> bool { + matches!(self, Self::SetChargeRate(_) | Self::SetChargeMode(_)) } } @@ -59,7 +66,8 @@ pub enum CpuMessage { } impl CpuMessage { - fn process(self, settings: &mut dyn TCpus) { + fn process(self, settings: &mut dyn TCpus) -> bool { + let dirty = self.is_modify(); // NOTE: "cpu" refers to the Linux kernel definition of a CPU, which is actually a hardware thread // not to be confused with a CPU chip, which usually has multiple hardware threads (cpu cores/threads) in the chip match self { @@ -129,6 +137,18 @@ impl CpuMessage { cb(result); } } + dirty + } + + /// Message instructs the driver to modify settings + fn is_modify(&self) -> bool { + matches!(self, + Self::SetCpuOnline(_, _) + | Self::SetCpusOnline(_) + | Self::SetClockLimits(_, _) + | Self::SetCpuGovernor(_, _) + | Self::SetCpusGovernor(_) + ) } } @@ -142,7 +162,8 @@ pub enum GpuMessage { } impl GpuMessage { - fn process(self, settings: &mut dyn TGpu) { + fn process(self, settings: &mut dyn TGpu) -> bool { + let dirty = self.is_modify(); match self { Self::SetPpt(fast, slow) => settings.ppt(fast, slow), Self::GetPpt(cb) => cb(settings.get_ppt()), @@ -151,6 +172,15 @@ impl GpuMessage { Self::SetSlowMemory(val) => *settings.slow_memory() = val, Self::GetSlowMemory(cb) => cb(*settings.slow_memory()), } + dirty + } + + fn is_modify(&self) -> bool { + matches!(self, + Self::SetPpt(_, _) + | Self::SetClockLimits(_) + | Self::SetSlowMemory(_) + ) } } @@ -161,12 +191,18 @@ pub enum GeneralMessage { } impl GeneralMessage { - fn process(self, settings: &mut dyn TGeneral) { + fn process(self, settings: &mut dyn TGeneral) -> bool { + let dirty = self.is_modify(); match self { Self::SetPersistent(val) => *settings.persistent() = val, Self::GetPersistent(cb) => cb(*settings.persistent()), Self::GetCurrentProfileName(cb) => cb(settings.get_name().to_owned()), } + dirty + } + + fn is_modify(&self) -> bool { + matches!(self, Self::SetPersistent(_)) } } @@ -178,43 +214,47 @@ pub struct ApiMessageHandler { impl ApiMessageHandler { pub fn process_forever(&mut self, settings: &mut Settings) { while let Ok(msg) = self.intake.recv() { - self.process(settings, msg); + let mut dirty = self.process(settings, msg); while let Ok(msg) = self.intake.try_recv() { - self.process(settings, msg); + dirty |= self.process(settings, msg); } - // run on_set - if let Err(e) = settings.on_set() { - log::error!("Settings on_set() err: {}", e); - } - // do callbacks - for func in self.on_empty.drain(..) { - func(()); - } - // save - log::debug!("api_worker is saving..."); - let is_persistent = *settings.general.persistent(); - let save_path = crate::utility::settings_dir() - .join(settings.general.get_path().clone()); - if is_persistent { - let settings_clone = settings.json(); - let save_json: SettingsJson = settings_clone.into(); - unwrap_maybe_fatal(save_json.save(&save_path), "Failed to save settings"); - log::debug!("Saved settings to {}", save_path.display()); - } else { - if save_path.exists() { - if let Err(e) = std::fs::remove_file(&save_path) { - log::warn!("Failed to delete persistent settings file {}: {}", save_path.display(), e); - } else { - log::debug!("Deleted persistent settings file {}", save_path.display()); - } - } else { - log::debug!("Ignored save request for non-persistent settings"); + if dirty { + // run on_set + if let Err(e) = settings.on_set() { + log::error!("Settings on_set() err: {}", e); } + // do callbacks + for func in self.on_empty.drain(..) { + func(()); + } + // save + log::debug!("api_worker is saving..."); + let is_persistent = *settings.general.persistent(); + let save_path = crate::utility::settings_dir() + .join(settings.general.get_path().clone()); + if is_persistent { + let settings_clone = settings.json(); + let save_json: SettingsJson = settings_clone.into(); + unwrap_maybe_fatal(save_json.save(&save_path), "Failed to save settings"); + log::debug!("Saved settings to {}", save_path.display()); + } else { + if save_path.exists() { + if let Err(e) = std::fs::remove_file(&save_path) { + log::warn!("Failed to delete persistent settings file {}: {}", save_path.display(), e); + } else { + log::debug!("Deleted persistent settings file {}", save_path.display()); + } + } else { + log::debug!("Ignored save request for non-persistent settings"); + } + } + } else { + log::debug!("Skipping callbacks for non-modify handled message(s)"); } } } - pub fn process(&mut self, settings: &mut Settings, message: ApiMessage) { + pub fn process(&mut self, settings: &mut Settings, message: ApiMessage) -> bool { match message { ApiMessage::Battery(x) => x.process(settings.battery.as_mut()), ApiMessage::Cpu(x) => x.process(settings.cpus.as_mut()), @@ -224,13 +264,18 @@ impl ApiMessageHandler { if let Err(e) = settings.on_resume() { log::error!("Settings on_resume() err: {}", e); } + false } - ApiMessage::WaitForEmptyQueue(callback) => self.on_empty.push(callback), + ApiMessage::WaitForEmptyQueue(callback) => { + self.on_empty.push(callback); + false + }, ApiMessage::LoadSettings(path, name) => { match settings.load_file(path.into(), name, false) { Ok(success) => log::info!("Loaded settings file? {}", success), Err(e) => log::warn!("Load file err: {}", e), } + true } ApiMessage::LoadMainSettings => { match settings.load_file( @@ -241,9 +286,11 @@ impl ApiMessageHandler { Ok(success) => log::info!("Loaded main settings file? {}", success), Err(e) => log::warn!("Load file err: {}", e), } + true } ApiMessage::LoadSystemSettings => { settings.load_system_default(); + true }, ApiMessage::GetLimits(cb) => { cb(super::SettingsLimits { @@ -252,6 +299,7 @@ impl ApiMessageHandler { gpu: settings.gpu.limits(), general: settings.general.limits(), }); + false }, ApiMessage::GetProvider(name, cb) => { cb(match &name as &str { @@ -259,7 +307,8 @@ impl ApiMessageHandler { "cpu" | "cpus" => settings.cpus.provider(), "gpu" => settings.gpu.provider(), _ => settings.general.provider(), - }) + }); + false } } } diff --git a/src/components/cpus.tsx b/src/components/cpus.tsx index cbb14a5..586200c 100644 --- a/src/components/cpus.tsx +++ b/src/components/cpus.tsx @@ -138,24 +138,24 @@ export class Cpus extends Component<{}, CpuState> { description={tr("Set bounds on clock speed")} onChange={(value: boolean) => { if (value) { - if ((get_value(LIMITS_INFO) as backend.SettingsLimits).cpu.cpus[0].clock_min_limits != null) { - set_value(CLOCK_MIN_CPU, (get_value(LIMITS_INFO) as backend.SettingsLimits).cpu.cpus[0].clock_min_limits!.min); - } - if ((get_value(LIMITS_INFO) as backend.SettingsLimits).cpu.cpus[0].clock_max_limits != null) { - set_value(CLOCK_MAX_CPU, (get_value(LIMITS_INFO) as backend.SettingsLimits).cpu.cpus[0].clock_max_limits!.max); - } - syncPlebClockToAdvanced(); - reloadGUI("CPUFreqToggle"); + if ((get_value(LIMITS_INFO) as backend.SettingsLimits).cpu.cpus[0].clock_min_limits != null) { + set_value(CLOCK_MIN_CPU, (get_value(LIMITS_INFO) as backend.SettingsLimits).cpu.cpus[0].clock_min_limits!.min); + } + if ((get_value(LIMITS_INFO) as backend.SettingsLimits).cpu.cpus[0].clock_max_limits != null) { + set_value(CLOCK_MAX_CPU, (get_value(LIMITS_INFO) as backend.SettingsLimits).cpu.cpus[0].clock_max_limits!.max); + } + syncPlebClockToAdvanced(); + reloadGUI("CPUFreqToggle"); } else { - set_value(CLOCK_MIN_CPU, null); - set_value(CLOCK_MAX_CPU, null); - for (let i = 0; i < total_cpus; i++) { - backend.resolve(backend.unsetCpuClockLimits(i), (_idc: any[]) => {}); - } - backend.resolve(backend.waitForComplete(), (_: boolean) => { - reloadGUI("CPUUnsetFreq"); - }); - syncPlebClockToAdvanced(); + set_value(CLOCK_MIN_CPU, null); + set_value(CLOCK_MAX_CPU, null); + for (let i = 0; i < total_cpus; i++) { + backend.resolve(backend.unsetCpuClockLimits(i), (_idc: any[]) => {}); + } + backend.resolve(backend.waitForComplete(), (_: boolean) => { + reloadGUI("CPUUnsetFreq"); + }); + syncPlebClockToAdvanced(); } }} /> @@ -172,20 +172,21 @@ export class Cpus extends Component<{}, CpuState> { onChange={(freq: number) => { backend.log(backend.LogLevel.Debug, "Min freq slider is now " + freq.toString()); const freqNow = get_value(CLOCK_MIN_CPU); - if (freq != freqNow) { - set_value(CLOCK_MIN_CPU, freq); - for (let i = 0; i < total_cpus; i++) { - backend.resolve(backend.setCpuClockLimits(i, freq, get_value(CLOCK_MAX_CPU)), - (limits: number[]) => { - set_value(CLOCK_MIN_CPU, limits[0]); - set_value(CLOCK_MAX_CPU, limits[1]); - syncPlebClockToAdvanced(); + const maxNow = get_value(CLOCK_MAX_CPU); + if (freq != freqNow && ((maxNow != null && freq > maxNow) || maxNow == null)) { + set_value(CLOCK_MIN_CPU, freq); + for (let i = 0; i < total_cpus; i++) { + backend.resolve(backend.setCpuClockLimits(i, freq, get_value(CLOCK_MAX_CPU)), + (limits: number[]) => { + set_value(CLOCK_MIN_CPU, limits[0]); + set_value(CLOCK_MAX_CPU, limits[1]); + syncPlebClockToAdvanced(); + }); + } + backend.resolve(backend.waitForComplete(), (_: boolean) => { + reloadGUI("CPUMinFreq"); }); - } - backend.resolve(backend.waitForComplete(), (_: boolean) => { - reloadGUI("CPUMinFreq"); - }); - reloadGUI("CPUMinFreqImmediate"); + reloadGUI("CPUMinFreqImmediate"); } }} />} @@ -202,20 +203,21 @@ export class Cpus extends Component<{}, CpuState> { onChange={(freq: number) => { backend.log(backend.LogLevel.Debug, "Max freq slider is now " + freq.toString()); const freqNow = get_value(CLOCK_MAX_CPU); - if (freq != freqNow) { - set_value(CLOCK_MAX_CPU, freq); - for (let i = 0; i < total_cpus; i++) { - backend.resolve(backend.setCpuClockLimits(i, get_value(CLOCK_MIN_CPU), freq), - (limits: number[]) => { - set_value(CLOCK_MIN_CPU, limits[0]); - set_value(CLOCK_MAX_CPU, limits[1]); - syncPlebClockToAdvanced(); + const minNow = get_value(CLOCK_MIN_CPU); + if (freq != freqNow && ((minNow != null && freq > minNow) || minNow == null)) { + set_value(CLOCK_MAX_CPU, freq); + for (let i = 0; i < total_cpus; i++) { + backend.resolve(backend.setCpuClockLimits(i, get_value(CLOCK_MIN_CPU), freq), + (limits: number[]) => { + set_value(CLOCK_MIN_CPU, limits[0]); + set_value(CLOCK_MAX_CPU, limits[1]); + syncPlebClockToAdvanced(); + }); + } + backend.resolve(backend.waitForComplete(), (_: boolean) => { + reloadGUI("CPUMaxFreq"); }); - } - backend.resolve(backend.waitForComplete(), (_: boolean) => { - reloadGUI("CPUMaxFreq"); - }); - reloadGUI("CPUMaxFreqImmediate"); + reloadGUI("CPUMaxFreqImmediate"); } }} />} @@ -301,15 +303,15 @@ export class Cpus extends Component<{}, CpuState> { onChange={(freq: number) => { backend.log(backend.LogLevel.Debug, "Min freq slider for " + advancedCpu.toString() + " is now " + freq.toString()); const freqNow = get_value(CLOCK_MIN_MAX_CPU)[advancedCpuIndex] as MinMax; - if (freq != freqNow.min) { - backend.resolve(backend.setCpuClockLimits(advancedCpuIndex, freq, freqNow.max!), - (limits: number[]) => { - const clocks = get_value(CLOCK_MIN_MAX_CPU) as MinMax[]; - clocks[advancedCpuIndex].min = limits[0]; - clocks[advancedCpuIndex].max = limits[1]; - set_value(CLOCK_MIN_MAX_CPU, clocks); - reloadGUI("CPUMinFreq"); - }); + if (freq != freqNow.min && ((freqNow.max != null && freqNow.max > freq) || freqNow.max == null)) { + backend.resolve(backend.setCpuClockLimits(advancedCpuIndex, freq, freqNow.max!), + (limits: number[]) => { + const clocks = get_value(CLOCK_MIN_MAX_CPU) as MinMax[]; + clocks[advancedCpuIndex].min = limits[0]; + clocks[advancedCpuIndex].max = limits[1]; + set_value(CLOCK_MIN_MAX_CPU, clocks); + reloadGUI("CPUMinFreq"); + }); } }} />} @@ -326,15 +328,15 @@ export class Cpus extends Component<{}, CpuState> { onChange={(freq: number) => { backend.log(backend.LogLevel.Debug, "Max freq slider for " + advancedCpu.toString() + " is now " + freq.toString()); const freqNow = get_value(CLOCK_MIN_MAX_CPU)[advancedCpuIndex] as MinMax; - if (freq != freqNow.max) { - backend.resolve(backend.setCpuClockLimits(advancedCpuIndex, freqNow.min!, freq), - (limits: number[]) => { - const clocks = get_value(CLOCK_MIN_MAX_CPU) as MinMax[]; - clocks[advancedCpuIndex].min = limits[0]; - clocks[advancedCpuIndex].max = limits[1]; - set_value(CLOCK_MIN_MAX_CPU, clocks); - reloadGUI("CPUMaxFreq"); - }); + if (freq != freqNow.max && ((freqNow.min != null && freq > freqNow.min) || freqNow.min == null)) { + backend.resolve(backend.setCpuClockLimits(advancedCpuIndex, freqNow.min!, freq), + (limits: number[]) => { + const clocks = get_value(CLOCK_MIN_MAX_CPU) as MinMax[]; + clocks[advancedCpuIndex].min = limits[0]; + clocks[advancedCpuIndex].max = limits[1]; + set_value(CLOCK_MIN_MAX_CPU, clocks); + reloadGUI("CPUMaxFreq"); + }); } }} />} diff --git a/src/components/gpu.tsx b/src/components/gpu.tsx index bcd9121..e101aeb 100644 --- a/src/components/gpu.tsx +++ b/src/components/gpu.tsx @@ -123,11 +123,11 @@ export class Gpu extends Component<{}> { } reloadGUI("GPUFreqToggle"); } else { - set_value(CLOCK_MIN_GPU, null); - set_value(CLOCK_MAX_GPU, null); - backend.resolve(backend.unsetGpuClockLimits(), (_: any[]) => { - reloadGUI("GPUUnsetFreq"); - }); + set_value(CLOCK_MIN_GPU, null); + set_value(CLOCK_MAX_GPU, null); + backend.resolve(backend.unsetGpuClockLimits(), (_: any[]) => { + reloadGUI("GPUUnsetFreq"); + }); } }} /> @@ -144,13 +144,14 @@ export class Gpu extends Component<{}> { onChange={(val: number) => { backend.log(backend.LogLevel.Debug, "GPU Clock Min is now " + val.toString()); const valNow = get_value(CLOCK_MIN_GPU); - if (val != valNow) { - backend.resolve(backend.setGpuClockLimits(val, get_value(CLOCK_MAX_GPU)), - (limits: number[]) => { - set_value(CLOCK_MIN_GPU, limits[0]); - set_value(CLOCK_MAX_GPU, limits[1]); - reloadGUI("GPUMinClock"); - }); + const maxNow = get_value(CLOCK_MAX_GPU); + if (val != valNow && ((maxNow != null && val < maxNow) || maxNow == null)) { + backend.resolve(backend.setGpuClockLimits(val, get_value(CLOCK_MAX_GPU)), + (limits: number[]) => { + set_value(CLOCK_MIN_GPU, limits[0]); + set_value(CLOCK_MAX_GPU, limits[1]); + reloadGUI("GPUMinClock"); + }); } }} />} @@ -167,13 +168,14 @@ export class Gpu extends Component<{}> { onChange={(val: number) => { backend.log(backend.LogLevel.Debug, "GPU Clock Max is now " + val.toString()); const valNow = get_value(CLOCK_MAX_GPU); - if (val != valNow) { - backend.resolve(backend.setGpuClockLimits(get_value(CLOCK_MIN_GPU), val), - (limits: number[]) => { - set_value(CLOCK_MIN_GPU, limits[0]); - set_value(CLOCK_MAX_GPU, limits[1]); - reloadGUI("GPUMaxClock"); - }); + const minNow = get_value(CLOCK_MIN_GPU); + if (val != valNow && ((minNow != null && val > minNow) || minNow == null)) { + backend.resolve(backend.setGpuClockLimits(get_value(CLOCK_MIN_GPU), val), + (limits: number[]) => { + set_value(CLOCK_MIN_GPU, limits[0]); + set_value(CLOCK_MAX_GPU, limits[1]); + reloadGUI("GPUMaxClock"); + }); } }} />}