-
-
Notifications
You must be signed in to change notification settings - Fork 326
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
Add governor information #1057
base: master
Are you sure you want to change the base?
Add governor information #1057
Conversation
Signed-off-by: Patrick José Pereira <[email protected]>
3231833
to
31dce84
Compare
Signed-off-by: Patrick José Pereira <[email protected]>
Signed-off-by: Patrick José Pereira <[email protected]>
Signed-off-by: Patrick José Pereira <[email protected]>
Signed-off-by: Patrick José Pereira <[email protected]>
Signed-off-by: Patrick José Pereira <[email protected]>
Signed-off-by: Patrick José Pereira <[email protected]>
…mExt Signed-off-by: Patrick José Pereira <[email protected]>
31dce84
to
6b48f22
Compare
@GuillaumeGomez it's ready to review, it appears that the CI error is not from this patch |
@@ -232,6 +232,9 @@ fn interpret_input(input: &str, sys: &mut System) -> bool { | |||
sys.global_cpu_info().frequency() | |||
); | |||
} | |||
"governor" => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not displaying this information as part of the CPUs directly?
/// ); | ||
/// println!("{}%", s.global_cpu_info().cpu_usage()); | ||
/// ``` | ||
fn governor(&self) -> GovernorKind; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this from SystemExt
, it should only be on CpuExt
.
/// println!("{}", cpu.governor()); | ||
/// } | ||
/// ``` | ||
fn governor(&self) -> GovernorKind; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should return an Option
in case this information hasn't been retrieved yet.
A few things to change but also: I won't merge this PR until this feature is implemented for all supported systems. |
Is this truly necessary ? Can't we make it an optional and have others OSs implementing it and allowing not having support ? I don't have access to all OSs, and that's open source.. others may add this functionality if it's necessary, I'm just creating the background to support it. |
Yes it is.
The only way to have something optional is in case an OS doesn't support this feature. Otherwise it might very well never be implemented. It also forces to add special handling in tests, meaning more code to maintain.
I do everything with VMs. You can always ask help and see if others are interested into this feature and willing to help you implement it. |
Signed-off-by: Patrick José Pereira [email protected]