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

Split cpu module #2114

Merged
merged 12 commits into from
Oct 17, 2023
Merged

Split cpu module #2114

merged 12 commits into from
Oct 17, 2023

Conversation

mmhat
Copy link
Contributor

@mmhat mmhat commented Apr 14, 2023

This PR contains three different modules that were derived from the original cpu module: cpu_frequency, cpu_usage and load.
The reasoning behind that split is that it is impossible to define states that severe all three purposes equally well, e.g. say I want a different color if the cpu usage goes above 50 percent and a different format if the load goes above 10.
As another improvement the load module now also shows the 5 minute and the 15 minute average.

@Alexays
Copy link
Owner

Alexays commented Jul 18, 2023

Hey!
Sorry for the delay, can you move common parts of these modules in shared files to avoid duplication?

@mmhat
Copy link
Contributor Author

mmhat commented Jul 29, 2023

@Alexays I deduplicated the code; I didn't move the shared code to own files though, the cpu module uses the functions of the cpu_frequency, cpu_usage and load module. Is that ok? If not, where should those shared files live? In utils?

@Alexays
Copy link
Owner

Alexays commented Sep 11, 2023

There's some errors in freebsd tests, can you check?

The module provides the three system load averages. This is an
improvement compared what you can do with the cpu module: cpu
only provides the one minute sample and the state of the cpu module is
derived from the cpu usage which messes up the formating of the load
average. Also, at least on modern Linux systems, the load of a system
takes much more than the cpu utilization into account and it should
therefore live in a separate module.
@mmhat
Copy link
Contributor Author

mmhat commented Sep 13, 2023

@Alexays I fixed the freebsd code and the CI is green again. Can you approve the workflows?

static std::vector<float> frequencies;
if (frequencies.empty()) {
spdlog::warn(
"cpu/bsd: parseCpuFrequencies is not implemented, expect garbage in {*_frequency}");
Copy link
Contributor

Choose a reason for hiding this comment

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

Chase module rename and maybe slightly deduplicate wording

Suggested change
"cpu/bsd: parseCpuFrequencies is not implemented, expect garbage in {*_frequency}");
"cpu_frequency: not implemented on BSDs, expect garbage in {*_frequency}");

@@ -99,6 +99,17 @@ waybar::AModule* waybar::Factory::makeModule(const std::string& name) const {
if (ref == "cpu") {
return new waybar::modules::Cpu(id, config_[name]);
}
#if defined(HAVE_CPU_LINUX)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe remove the conditional as the module code is still compiled in. cpu is like a superset, so cpu_frequency printing different warning is confusing:

 $ waybar
 [...]
-[2023-10-04 09:23:30.090] [warning] cpu_frequency: not implemented on BSDs, expect garbage in {*_frequency}
+[2023-10-04 10:05:32.888] [warning] module cpu_frequency: Unknown module: cpu_frequency

@Alexays Alexays merged commit 6b73e2a into Alexays:master Oct 17, 2023
@Alexays
Copy link
Owner

Alexays commented Oct 17, 2023

Sorry again for the delay, do you think it's possible to do another PR to update man and Github Wiki?
Thanks again!

@mmhat
Copy link
Contributor Author

mmhat commented Oct 18, 2023

Nice, thank you @Alexays ! Yes, I can do that.

@mmhat mmhat deleted the split-cpu-module branch October 18, 2023 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants