Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

other: if in a non-D0 state, short-circuit further logic #1355

Merged
merged 5 commits into from
Dec 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Bug Fixes

- [#1314](https://github.com/ClementTsang/bottom/pull/1314): Fix fat32 mounts not showing up in macOS.
- [#1355](https://github.com/ClementTsang/bottom/pull/1355): Reduce chances of non-D0 devices waking up due to temperature checks on Linux.

## [0.9.6] - 2023-08-26

Expand Down
2 changes: 1 addition & 1 deletion src/app/data_harvester/nvidia.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ pub fn get_nvidia_vecs(

temp_vec.push(TempHarvest {
name: name.clone(),
temperature,
temperature: Some(temperature),
});
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/app/data_harvester/temperature.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use crate::app::Filter;
#[derive(Default, Debug, Clone)]
pub struct TempHarvest {
pub name: String,
pub temperature: f32,
pub temperature: Option<f32>,
}

#[derive(Clone, Debug, Copy, PartialEq, Eq, Default)]
Expand Down
77 changes: 42 additions & 35 deletions src/app/data_harvester/temperature/linux.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use anyhow::Result;
use hashbrown::{HashMap, HashSet};

use super::{is_temp_filtered, TempHarvest, TemperatureType};
use crate::app::Filter;
use crate::{app::Filter, utils::error::BottomError};

const EMPTY_NAME: &str = "Unknown";

Expand All @@ -24,7 +24,7 @@ fn read_temp(path: &Path) -> Result<f32> {
Ok(fs::read_to_string(path)?
.trim_end()
.parse::<f32>()
.map_err(|e| crate::utils::error::BottomError::ConversionError(e.to_string()))?
.map_err(|e| BottomError::ConversionError(e.to_string()))?
/ 1_000.0)
}

Expand Down Expand Up @@ -131,15 +131,40 @@ fn finalize_name(
None => label,
},
(Some(name), None) => name,
(None, None) => match &fallback_sensor_name {
Some(sensor_name) => sensor_name.clone(),
(None, None) => match fallback_sensor_name {
Some(sensor_name) => sensor_name.to_owned(),
None => EMPTY_NAME.to_string(),
},
};

counted_name(seen_names, candidate_name)
}

/// Whether the temperature should *actually* be read during enumeration.
/// Will return false if the state is not D0/unknown, or if it does not support `device/power_state`.
#[inline]
fn is_device_awake(path: &Path) -> bool {
// Whether the temperature should *actually* be read during enumeration.
// Set to false if the device is in ACPI D3cold.
// Documented at https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-devices-power_state
let device = path.join("device");
let power_state = device.join("power_state");
if power_state.exists() {
if let Ok(state) = fs::read_to_string(power_state) {
let state = state.trim();
// The zenpower3 kernel module (incorrectly?) reports "unknown", causing this check
// to fail and temperatures to appear as zero instead of having the file not exist.
//
// Their self-hosted git instance has disabled sign up, so this bug cant be reported either.
state == "D0" || state == "unknown"
} else {
true
}
} else {
true
}
}

/// Get temperature sensors from the linux sysfs interface `/sys/class/hwmon` and
/// `/sys/devices/platform/coretemp.*`. It returns all found temperature sensors, and the number
/// of checked hwmon directories (not coretemp directories).
Expand Down Expand Up @@ -178,29 +203,15 @@ fn hwmon_temperatures(temp_type: &TemperatureType, filter: &Option<Filter>) -> H
for file_path in dirs {
let sensor_name = read_to_string_lossy(file_path.join("name"));

// Whether the temperature should *actually* be read during enumeration.
// Set to false if the device is in ACPI D3cold.
//
// If it is false, then the temperature will be set to 0.0 later down the line.
let should_read_temp = {
// Documented at https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-devices-power_state
let device = file_path.join("device");
let power_state = device.join("power_state");
if power_state.exists() {
if let Ok(state) = fs::read_to_string(power_state) {
let state = state.trim();
// The zenpower3 kernel module (incorrectly?) reports "unknown", causing this check
// to fail and temperatures to appear as zero instead of having the file not exist.
//
// Their self-hosted git instance has disabled sign up, so this bug cant be reported either.
state == "D0" || state == "unknown"
} else {
true
}
} else {
true
if !is_device_awake(&file_path) {
if let Some(sensor_name) = sensor_name {
temperatures.push(TempHarvest {
name: sensor_name,
temperature: None,
});
}
};
continue;
}

if let Ok(dir_entries) = file_path.read_dir() {
// Enumerate the devices temperature sensors
Expand Down Expand Up @@ -272,19 +283,15 @@ fn hwmon_temperatures(temp_type: &TemperatureType, filter: &Option<Filter>) -> H
let name = finalize_name(hwmon_name, sensor_label, &sensor_name, &mut seen_names);

if is_temp_filtered(filter, &name) {
let temp_celsius = if should_read_temp {
if let Ok(temp) = read_temp(&temp_path) {
temp
} else {
continue;
}
let temp_celsius = if let Ok(temp) = read_temp(&temp_path) {
temp
} else {
0.0
continue;
};

temperatures.push(TempHarvest {
name,
temperature: temp_type.convert_temp_unit(temp_celsius),
temperature: Some(temp_type.convert_temp_unit(temp_celsius)),
});
}
}
Expand Down Expand Up @@ -329,7 +336,7 @@ fn add_thermal_zone_temperatures(

temperatures.push(TempHarvest {
name,
temperature: temp_type.convert_temp_unit(temp_celsius),
temperature: Some(temp_type.convert_temp_unit(temp_celsius)),
});
}
}
Expand Down
6 changes: 3 additions & 3 deletions src/app/data_harvester/temperature/sysinfo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ pub fn get_temperature_data(
if is_temp_filtered(filter, &name) {
temperature_vec.push(TempHarvest {
name,
temperature: temp_type.convert_temp_unit(component.temperature()),
temperature: Some(temp_type.convert_temp_unit(component.temperature())),
});
}
}
Expand All @@ -36,11 +36,11 @@ pub fn get_temperature_data(
if let Some(temp) = temp.as_temperature() {
temperature_vec.push(TempHarvest {
name,
temperature: match temp_type {
temperature: Some(match temp_type {
TemperatureType::Celsius => temp.celsius(),
TemperatureType::Kelvin => temp.kelvin(),
TemperatureType::Fahrenheit => temp.fahrenheit(),
},
}),
});
}
}
Expand Down
11 changes: 9 additions & 2 deletions src/bin/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,14 @@ use crossterm::{
use tui::{backend::CrosstermBackend, Terminal};

use bottom::{
args,
canvas::{self, canvas_styling::CanvasStyling},
check_if_terminal, cleanup_terminal, create_collection_thread, create_input_thread,
create_or_get_config,
data_conversion::*,
handle_key_event_or_break, handle_mouse_event,
options::*,
*,
panic_hook, read_config, try_drawing, update_data, BottomEvent,
};

// Used for heap allocation debugging purposes.
Expand All @@ -38,7 +42,10 @@ fn main() -> Result<()> {

#[cfg(feature = "logging")]
{
if let Err(err) = init_logger(log::LevelFilter::Debug, std::ffi::OsStr::new("debug.log")) {
if let Err(err) = bottom::init_logger(
log::LevelFilter::Debug,
Some(std::ffi::OsStr::new("debug.log")),
) {
println!("Issue initializing logger: {err}");
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/data_conversion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ impl ConvertedData {
data.temp_harvest.iter().for_each(|temp_harvest| {
self.temp_data.push(TempWidgetData {
sensor: KString::from_ref(&temp_harvest.name),
temperature_value: temp_harvest.temperature.ceil() as u64,
temperature_value: temp_harvest.temperature.map(|temp| temp.ceil() as u64),
temperature_type,
});
});
Expand Down
57 changes: 42 additions & 15 deletions src/utils/logging.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@ pub static OFFSET: once_cell::sync::Lazy<time::UtcOffset> = once_cell::sync::Laz

#[cfg(feature = "logging")]
pub fn init_logger(
min_level: log::LevelFilter, debug_file_name: &std::ffi::OsStr,
min_level: log::LevelFilter, debug_file_name: Option<&std::ffi::OsStr>,
) -> Result<(), fern::InitError> {
fern::Dispatch::new()
let dispatch = fern::Dispatch::new()
.format(|out, message, record| {
let offset_time = {
let utc = time::OffsetDateTime::now_utc();
Expand All @@ -43,35 +43,39 @@ pub fn init_logger(
message
))
})
.level(min_level)
.chain(fern::log_file(debug_file_name)?)
.apply()?;
.level(min_level);

if let Some(debug_file_name) = debug_file_name {
dispatch.chain(fern::log_file(debug_file_name)?).apply()?;
} else {
dispatch.chain(std::io::stdout()).apply()?;
}

Ok(())
}

#[macro_export]
macro_rules! c_debug {
macro_rules! error {
($($x:tt)*) => {
#[cfg(feature = "logging")]
{
log::debug!($($x)*)
log::error!($($x)*)
}
};
}

#[macro_export]
macro_rules! c_error {
macro_rules! warn {
($($x:tt)*) => {
#[cfg(feature = "logging")]
{
log::error!($($x)*)
log::warn!($($x)*)
}
};
}

#[macro_export]
macro_rules! c_info {
macro_rules! info {
($($x:tt)*) => {
#[cfg(feature = "logging")]
{
Expand All @@ -81,17 +85,17 @@ macro_rules! c_info {
}

#[macro_export]
macro_rules! c_log {
macro_rules! debug {
($($x:tt)*) => {
#[cfg(feature = "logging")]
{
log::log!($($x)*)
log::debug!($($x)*)
}
};
}

#[macro_export]
macro_rules! c_trace {
macro_rules! trace {
($($x:tt)*) => {
#[cfg(feature = "logging")]
{
Expand All @@ -101,11 +105,34 @@ macro_rules! c_trace {
}

#[macro_export]
macro_rules! c_warn {
macro_rules! log {
($($x:tt)*) => {
#[cfg(feature = "logging")]
{
log::warn!($($x)*)
log::log!(log::Level::Trace, $($x)*)
}
};
($level:expr, $($x:tt)*) => {
#[cfg(feature = "logging")]
{
log::log!($level, $($x)*)
}
};
}

#[cfg(test)]
mod test {

#[cfg(feature = "logging")]
#[test]
fn test_logging_macros() {
super::init_logger(log::LevelFilter::Trace, None)
.expect("initializing the logger should succeed");

error!("This is an error.");
warn!("This is a warning.");
info!("This is an info.");
debug!("This is a debug.");
info!("This is a trace.");
}
}
20 changes: 12 additions & 8 deletions src/widgets/temperature_table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use crate::{
#[derive(Clone, Debug)]
pub struct TempWidgetData {
pub sensor: KString,
pub temperature_value: u64,
pub temperature_value: Option<u64>,
pub temperature_type: TemperatureType,
}

Expand All @@ -37,13 +37,17 @@ impl ColumnHeader for TempWidgetColumn {

impl TempWidgetData {
pub fn temperature(&self) -> KString {
let temp_val = self.temperature_value.to_string();
let temp_type = match self.temperature_type {
TemperatureType::Celsius => "°C",
TemperatureType::Kelvin => "K",
TemperatureType::Fahrenheit => "°F",
};
concat_string!(temp_val, temp_type).into()
match self.temperature_value {
Some(temp_val) => {
let temp_type = match self.temperature_type {
TemperatureType::Celsius => "°C",
TemperatureType::Kelvin => "K",
TemperatureType::Fahrenheit => "°F",
};
concat_string!(temp_val.to_string(), temp_type).into()
}
None => "N/A".to_string().into(),
}
}
}

Expand Down