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.

Sign up for GitHub

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

solve ratio problem #12

Merged
merged 4 commits into from
Oct 10, 2023
Merged

solve ratio problem #12

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e9ea77b
solve ratio problem
bktech2021 Oct 9, 2023
b53271f
no fan -> no fans
bktech2021 Oct 9, 2023
acfa829
discover_gpus cross-platform issue
bktech2021 Oct 10, 2023
4d83f6c
small fixes
bktech2021 Oct 10, 2023
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
17 changes: 12 additions & 5 deletions src/app.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,9 @@ pub fn run(
selected_gpu = usize::from(n - 1)
}
KeyCode::Char('p') => {
// re-scan pci tree to let driver discover new devices (only works as sudo)
//commented out because discover_gpus not implemented
Copy link
Owner

@alphastrata alphastrata Oct 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is implemented https://docs.rs/nvml-wrapper/latest/nvml_wrapper/struct.Nvml.html#method.discover_gpus, it's probably not available on the platform (laptop gpus).

We need a better way to conditionally show/not-show this for sure.

You can actually see it working on a multi-gpu system here #6 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was not able to compile, i will look again

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

/* snip

# Platform Support

    Only supports Linux.
    */
    // TODO: constructor for default pci_infos ^
    // Checked against local
    // Tested
    #[cfg(target_os = "linux")]
    #[doc(alias = "nvmlDeviceDiscoverGpus")]
    pub fn discover_gpus(&self, pci_info: PciInfo) -> Result<(), NvmlError> {
        let sym = nvml_sym(self.lib.nvmlDeviceDiscoverGpus.as_ref())?;

        unsafe { nvml_try(sym(&mut pci_info.try_into()?)) }
    }

/* snip */

as it writes, this function is only for linux, im testing it on windows. so i will try to not show "p to rescan" on windows

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, I too should've gone deeper in the docs before snarking, soz.


/* // re-scan pci tree to let driver discover new devices (only works as sudo)
match nvml.discover_gpus(PciInfo {
bus: 0,
bus_id: "".into(),
Expand All @@ -174,7 +176,7 @@ pub fn run(
gpu_list = crate::gpu::try_init_gpus(&nvml, lh)?;
if selected_gpu >= gpu_list.len() {
selected_gpu = 0;
}
} */
}
_ => {}
}
Expand All @@ -198,9 +200,14 @@ fn draw_fan_speed<'d>(gpu: &GpuInfo<'d>) -> Gauge<'d> {
.map(|u| u as f64)
.sum::<f64>()
/ temps as f64;

let percentage = (avg / 100.).clamp(0., 1.0);
let label = format!("{:.1}%", avg);
let percentage = match avg.is_nan() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is defnitely no issue with the code -- I'm thinking that maybe we should just not run the call to draw_fan-speed at all if we're on a fan-less system, because we're checking this every time the loop runs.

Wanna hoist something out side the loop to where we can check if we get values then just never run this code if we don't have to?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i will convert it to something like Result<Gauge, Error>, if error is no fans, it will get there is no fans. and, you can implement other errors in future.

true => 0.0,
false => avg
};
let label = match avg.is_nan() {
true => "No fans".to_string(),
false => format!("{:.1}%", avg)
};
let spanned_label = Span::styled(label, Style::new().white().bold().bg(Color::Black));

Gauge::default()
alphastrata marked this conversation as resolved.
Show resolved Hide resolved
Expand Down