From 1a258aaed86b47422542138eef8a46dfafc28a6e Mon Sep 17 00:00:00 2001 From: Viet Anh Nguyen Date: Mon, 20 Jul 2026 10:00:36 +0700 Subject: [PATCH 1/2] fix(fan-curve): keep the watchdog alive at a steady temperature The firmware watchdog is one-shot: it counts down from the last fan command and hands the fan back to thinkpad_acpi when it reaches zero. arm_fan_watchdog() was called only inside the 'level changed' branch, which reads as correct but inverts the actual risk -- a curve sitting at a steady temperature issues no fan commands at all. So roughly 30s after the temperature settled, the firmware silently reclaimed the fan. last_level still matched the target, so nothing ever rewrote it, and the UI went on emitting fan-curve-update every 2s showing the curve as active. The failure is invisible and the steady state is the common case, not an edge case. Re-arms on a timer at half the watchdog interval, which leaves a full loop tick of slack so one delayed iteration cannot lose the fan. Cleared alongside last_level wherever the curve stops steering, so re-enabling arms immediately rather than inheriting a stale deadline. Verified by mutation: forcing watchdog_due() to false reproduces the old behaviour and both tests fail, one reporting 30s without a re-arm. --- src-tauri/src/fan_curve.rs | 100 ++++++++++++++++++++++++++++++++++++- 1 file changed, 98 insertions(+), 2 deletions(-) diff --git a/src-tauri/src/fan_curve.rs b/src-tauri/src/fan_curve.rs index cbe57ed..a0cc836 100755 --- a/src-tauri/src/fan_curve.rs +++ b/src-tauri/src/fan_curve.rs @@ -1,6 +1,6 @@ use serde::{Deserialize, Serialize}; use std::sync::{Arc, Mutex}; -use std::time::Duration; +use std::time::{Duration, Instant}; use tauri::{AppHandle, Emitter, Manager}; use tauri_plugin_store::StoreExt; use tokio::time::sleep; @@ -225,7 +225,8 @@ fn get_cpu_temperature() -> Result { /// /// Shared with the helper script's whitelist, which only accepts this exact /// value. thinkpad_acpi's watchdog is one-shot and only rearms when it receives -/// a fan command, so it is re-armed on every level change rather than once. +/// a fan command, so it is re-armed on a timer — see [`watchdog_due`] for why +/// re-arming on level change alone was not enough. use crate::fan_control::FAN_WATCHDOG_SECS; /// Hand the fan back to firmware control. @@ -249,6 +250,21 @@ async fn arm_fan_watchdog() { let _ = write_fan_command(&format!("watchdog {}", FAN_WATCHDOG_SECS)).await; } +/// Whether the firmware watchdog is due to be re-armed. +/// +/// The watchdog is one-shot: it counts down from the last fan command and hands +/// the fan back to the firmware when it reaches zero. Re-arming only on level +/// change looks right, but a curve sitting at a steady temperature issues no fan +/// commands at all — so the fan silently reverted to automatic control roughly +/// [`FAN_WATCHDOG_SECS`] after the temperature settled, which is the *common* +/// case rather than an edge case. `last_level` still matched the target, so +/// nothing rewrote it and the UI went on reporting the curve as active. +/// +/// Re-armed at half the interval so one slow or skipped tick cannot expire it. +fn watchdog_due(since_last_arm: Duration) -> bool { + since_last_arm >= Duration::from_secs((FAN_WATCHDOG_SECS / 2) as u64) +} + /// Synchronous counterpart to [`restore_fan_to_auto`], for the app exit handler. /// /// Runs unconditionally on shutdown: once this process is gone nothing is left @@ -334,6 +350,7 @@ async fn write_fan_command(command: &str) -> Result<(), String> { pub async fn fan_curve_background_task(app: AppHandle) { let state = app.state::(); let mut last_level: Option = None; + let mut last_armed: Option = None; let mut error_count = 0; let mut permission_error_reported = false; const MAX_ERRORS: i32 = 5; @@ -359,6 +376,7 @@ pub async fn fan_curve_background_task(app: AppHandle) { restore_fan_to_auto().await; } last_level = None; + last_armed = None; permission_error_reported = false; continue; } @@ -380,6 +398,7 @@ pub async fn fan_curve_background_task(app: AppHandle) { eprintln!("[Fan Curve] Temperature unreadable — returning fan to auto"); restore_fan_to_auto().await; last_level = None; + last_armed = None; let _ = app.emit_to( "main", "fan-curve-error", @@ -408,6 +427,7 @@ pub async fn fan_curve_background_task(app: AppHandle) { last_level = Some(target_level); permission_error_reported = false; arm_fan_watchdog().await; + last_armed = Some(Instant::now()); } Err(e) => { eprintln!("[Fan Curve] Failed to set fan speed: {}", e); @@ -425,6 +445,11 @@ pub async fn fan_curve_background_task(app: AppHandle) { } } } + } else if last_level.is_some() && last_armed.is_none_or(|t| watchdog_due(t.elapsed())) { + // Holding a level still counts as steering the fan, so the watchdog + // has to be kept alive even though nothing is being changed. + arm_fan_watchdog().await; + last_armed = Some(Instant::now()); } // Always emit temperature and current level to frontend for live UI updates @@ -446,6 +471,77 @@ pub async fn fan_curve_background_task(app: AppHandle) { mod tests { use super::*; + /// The regression this exists for: the curve re-armed the watchdog only when + /// the level changed, so a machine sitting at a steady temperature issued no + /// fan commands and the firmware took the fan back after FAN_WATCHDOG_SECS. + /// + /// The loop ticks every 2s, so this asserts the re-arm lands with room to + /// spare rather than on the exact boundary. + #[test] + fn watchdog_is_rearmed_well_before_the_firmware_gives_up() { + let expiry = Duration::from_secs(FAN_WATCHDOG_SECS as u64); + + assert!( + !watchdog_due(Duration::from_secs(0)), + "no re-arm needed immediately after arming" + ); + + let due_at = (1..=FAN_WATCHDOG_SECS as u64) + .map(Duration::from_secs) + .find(|d| watchdog_due(*d)) + .expect("must become due before the firmware watchdog expires"); + + assert!( + due_at < expiry, + "re-arm becomes due at {:?} but the firmware gives up at {:?}", + due_at, + expiry + ); + + // At least one full 2s tick has to fit between "due" and "expired", + // otherwise a single slow iteration loses the fan. + assert!( + expiry - due_at >= Duration::from_secs(2), + "only {:?} of slack between due ({:?}) and expiry ({:?}) - one \ + delayed tick would let the watchdog fire", + expiry - due_at, + due_at, + expiry + ); + } + + /// A level held across many ticks is the case that used to silently stop + /// steering the fan, so walk the actual loop cadence rather than a single + /// duration. + #[test] + fn holding_one_level_still_keeps_the_watchdog_alive() { + const TICK: u64 = 2; + let mut since_arm = Duration::from_secs(0); + let mut rearms = 0; + + // Five minutes at a dead-steady temperature: no level change, ever. + for _ in 0..(300 / TICK) { + since_arm += Duration::from_secs(TICK); + if watchdog_due(since_arm) { + rearms += 1; + since_arm = Duration::from_secs(0); + } + assert!( + since_arm < Duration::from_secs(FAN_WATCHDOG_SECS as u64), + "watchdog went {:?} without a re-arm - the fan would have \ + reverted to firmware control", + since_arm + ); + } + + assert!( + rearms >= 9, + "expected roughly one re-arm per {}s over 5 minutes, got {}", + FAN_WATCHDOG_SECS / 2, + rearms + ); + } + #[test] fn test_calculate_fan_level() { let points = vec![ From 5c0bb06fc22065406685e8c58fa19e3081fd09be Mon Sep 17 00:00:00 2001 From: Viet Anh Nguyen Date: Mon, 20 Jul 2026 10:07:33 +0700 Subject: [PATCH 2/2] fix(fan-curve): say when a computed level was not applied The update event carried only fan_level, taken from last_level.unwrap_or(target_level). When every write fails -- no helper installed, /proc/acpi/ibm/fan not writable -- last_level stays None and the event reported the level the curve *wanted*, byte-identical to one it had actually set. The fan-curve-error toast fires once, guarded by permission_error_reported, and is easy to miss or dismiss. After that the panel looked correct indefinitely while the curve had never touched the fan. Adds a controlling flag alongside the level. The panel appends "(not applied)" and tints the figure when it is false. Compared with === false so an event without the field still reads as controlling. --- src-tauri/src/fan_curve.rs | 10 +++++++++- src/js/fanCurve.js | 11 ++++++++--- src/styles/fan.css | 7 +++++++ 3 files changed, 24 insertions(+), 4 deletions(-) diff --git a/src-tauri/src/fan_curve.rs b/src-tauri/src/fan_curve.rs index a0cc836..6e1db7f 100755 --- a/src-tauri/src/fan_curve.rs +++ b/src-tauri/src/fan_curve.rs @@ -452,7 +452,14 @@ pub async fn fan_curve_background_task(app: AppHandle) { last_armed = Some(Instant::now()); } - // Always emit temperature and current level to frontend for live UI updates + // Always emit temperature and current level to frontend for live UI updates. + // + // `controlling` says whether that level was actually applied. When every + // write is failing — no helper installed, /proc not writable — last_level + // stays None and this reported the level it *wanted*, indistinguishable + // from one it had set. The fan-curve-error toast fires once and is easy + // to miss or dismiss, after which the display looked correct forever. + let controlling = last_level.is_some(); let display_level = last_level.unwrap_or(target_level); if let Err(e) = app.emit_to( "main", @@ -460,6 +467,7 @@ pub async fn fan_curve_background_task(app: AppHandle) { serde_json::json!({ "temperature": temp, "fan_level": display_level, + "controlling": controlling, }), ) { eprintln!("[Fan Curve] Failed to emit event: {}", e); diff --git a/src/js/fanCurve.js b/src/js/fanCurve.js index d699770..e8d5c09 100755 --- a/src/js/fanCurve.js +++ b/src/js/fanCurve.js @@ -64,12 +64,17 @@ export async function startCurveMode() { const { listen } = window.__TAURI__.event; if (!window.fanCurveUnlisten) { window.fanCurveUnlisten = await listen('fan-curve-update', (event) => { - const { temperature, fan_level } = event.payload; + const { temperature, fan_level, controlling } = event.payload; currentTemp = temperature; - // Update UI + // Update UI. `controlling` is false when the backend computed this level + // but could not apply it, which otherwise looks identical to a level it + // did apply — the error toast fires once and is easy to miss. document.getElementById('curve-current-temp').textContent = `${temperature}°C`; - document.getElementById('curve-target-speed').textContent = `Level ${fan_level}`; + const speedEl = document.getElementById('curve-target-speed'); + speedEl.textContent = + controlling === false ? `Level ${fan_level} (not applied)` : `Level ${fan_level}`; + speedEl.classList.toggle('curve-not-applied', controlling === false); drawCurve(); }); diff --git a/src/styles/fan.css b/src/styles/fan.css index 7970fc3..580097a 100644 --- a/src/styles/fan.css +++ b/src/styles/fan.css @@ -621,6 +621,13 @@ color: var(--text-primary); } +/* The curve computed this level but could not apply it — usually no fan helper + installed. Without this the figure is indistinguishable from one that took + effect, and the accompanying error toast only appears once. */ +.curve-info-value.curve-not-applied { + color: var(--power-color); +} + .fan-curve-help, .curve-help { padding: 12px;