feat(temperature): Add `zone-type` setting #2752

xphoniex · 2022-06-22T06:53:26Z

What type of PR is this? (check all applicable)

Feature
Bug Fix

Description

CPU's thermal-zone changes on every boot on my machine, sometimes it's 1 and other times it's 2. Others also have had this issue and resorted to scripts updating the thermal-zone before starting polybar.

This can simply be fixed using a zone-type config option:

If you look at the content in /sys/class/thermal/thermal_zone%zone%/type, you'll get the name of the sensor and you can easily deduct zone from that. For example for Intel CPUs, you should set zone-type = x86_pkg_temp and you'll be set.

Related Issues & Documents

Ref #2572

Documentation (check all applicable)

This PR requires changes to the Wiki documentation (describe the changes)

xphoniex · 2022-06-30T10:39:16Z

polite reminder @patrick96 , can we merge this?

patrick96 · 2022-07-01T04:17:38Z

I'm not around right now, so can't really review this. Will review once I'm back and have time. But general idea looks good. Except that you just try the first 10 zones instead of listing files in the folder and iterating through those.

xphoniex · 2022-07-02T04:41:50Z

I'm not around right now, so can't really review this. Will review once I'm back and have time. But general idea looks good. Except that you just try the first 10 zones instead of listing files in the folder and iterating through those.

Okay, cool.

Re iterations, rewrote it to use INT_MAX (here), this is simpler than listing the files (no allocation needed, and no need to then extract zone # from the file string).

I have a few useful contributions which I can open PR for once this is merged.

patrick96 · 2022-07-09T10:40:21Z

I'm not the biggest fan of iterating until no file is found. Is it guaranteed that all thermal_zoneX appear in order without gaps?

Also, the error message for when no zone is found is very confusing:

The file '/sys/class/thermal/thermal_zone1/type' does not exist

The error message should tell the user which zone type it failed to find (and maybe even list all available zone types).

I have a few useful contributions which I can open PR for once this is merged.

Please open a discussion topic for those. That way we can see how those will fit into polybar.

xphoniex · 2022-07-09T16:45:31Z

I'm not the biggest fan of iterating until no file is found. Is it guaranteed that all thermal_zoneX appear in order without gaps?

Also, the error message for when no zone is found is very confusing:
The file '/sys/class/thermal/thermal_zone1/type' does not exist
The error message should tell the user which zone type it failed to find (and maybe even list all available zone types).

There's this in kernel, but still not sure about gaps as there are ways to unregister sensors.

I suppose you've seen this issue before so I implemented what you suggested, check the diff please.

xphoniex · 2022-07-23T17:46:20Z

reminder

codecov · 2022-07-25T10:03:05Z

Codecov Report

Merging #2752 (379499a) into master (b5764c8) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

❗ Current head 379499a differs from pull request most recent head 0ba3557. Consider uploading reports for the commit 0ba3557 to get more accurate results

@@            Coverage Diff             @@
##           master    #2752      +/-   ##
==========================================
- Coverage   13.52%   13.49%   -0.03%     
==========================================
  Files         152      152              
  Lines       11396    11417      +21     
==========================================
  Hits         1541     1541              
- Misses       9855     9876      +21

Flag	Coverage Δ
unittests	`13.49% <0.00%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/modules/temperature.cpp	`0.00% <0.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us.

patrick96

Please also mention this feature in the ¢hangelog.

patrick96 · 2022-07-25T10:05:13Z

src/modules/temperature.cpp

+      }
+
+      if (!zone_found) {
+        throw module_error("zone-type '" + m_zone_type +  "' was not found, available zone types:" + available_zones);


Suggested change

throw module_error("zone-type '" + m_zone_type + "' was not found, available zone types:" + available_zones);

throw module_error("zone-type '" + m_zone_type + "' was not found, available zone types: " + available_zones);

https://github.com/polybar/polybar/pull/2752/files#diff-4ecfc5580e380c6cc659596ee5bb12d8f8df8ceea72dae9559e1494503d54691R42

patrick96 · 2022-07-25T10:07:20Z

src/modules/temperature.cpp

+      for (auto &z: zone_list) {
+        string zone_file = string_util::replace(PATH_THERMAL_ZONE_TYPE, "%zone%", to_string(z));
+        string z_zone_type = string_util::strip_trailing_newline(file_util::contents(zone_file));
+        available_zones += " " + z_zone_type;


Minor: Instead of manually constructing the string, we could store the types in a vector and finally construct the string using. string_util::join(available_zones, ", ")

https://github.com/polybar/polybar/pull/2752/files#diff-4ecfc5580e380c6cc659596ee5bb12d8f8df8ceea72dae9559e1494503d54691R42

patrick96 · 2022-07-25T10:10:03Z

src/modules/temperature.cpp

+      vector<string> thermal_list = file_util::list_files("/sys/class/thermal/");
+      for (auto &thermal: thermal_list) {
+        string zone_str = regex_replace(thermal, std::regex("thermal_zone([0-9]+)"), string("$1"), std::regex_constants::format_no_copy);
+        if (!zone_str.empty()) {
+          zone_list.push_back(stoi(zone_str));
+        }
+      }


Optional, but could make this code cleaner: Instead of extracting the zone number, we could file_util::glob for /sys/class/thermal/thermal_zone* and just add /type for getting the type and /temp to get m_path, the zone number isn't really needed for the module itself.

Pick whatever solution you like better.

I kept the zone number as an effort to keep behavior consistent for when there are multiple sensors with the same name. We know we'll always pick the one with lowest number, which depending on hardware and kernel may or may not be random.

Do you still want me to change this?

Oh that's a good point. I think sorting the paths should be sufficient to keep consistency (even if it doesn't guarantee that the lowest number is picked).

It's up to you, whichever solution you like better

https://github.com/polybar/polybar/pull/2752/files#diff-4ecfc5580e380c6cc659596ee5bb12d8f8df8ceea72dae9559e1494503d54691R28

Signed-off-by: xphoniex <dj.2dixx@gmail.com>

xphoniex · 2022-07-30T12:13:12Z

Please also mention this feature in the ¢hangelog.

https://github.com/polybar/polybar/blob/0ba355760950b0b5932f46c82cac8858eb3b55a9/CHANGELOG.md

patrick96 · 2022-07-30T15:03:17Z

Looks very good now. I'll not be at my computer for a few weeks, so I'll have to do the final review + testing once I get back.

xphoniex · 2022-08-13T15:16:09Z

bump to keep the PR in my Recent activity

patrick96 · 2022-08-21T17:25:47Z

Thanks!

xphoniex force-pushed the xphoniex/add-zone-type-to-temperature-mod branch from eb369e4 to 5b96d0f Compare July 2, 2022 04:37

xphoniex force-pushed the xphoniex/add-zone-type-to-temperature-mod branch from 5b96d0f to 6ced7ce Compare July 9, 2022 16:40

xphoniex force-pushed the xphoniex/add-zone-type-to-temperature-mod branch from 6ced7ce to 379499a Compare July 9, 2022 16:48

patrick96 mentioned this pull request Jul 25, 2022

Hardware settings auto-detection #2572

Open

patrick96 requested changes Jul 25, 2022

View reviewed changes

feat(temperature): Add zone-type setting

0ba3557

Signed-off-by: xphoniex <dj.2dixx@gmail.com>

xphoniex force-pushed the xphoniex/add-zone-type-to-temperature-mod branch from 379499a to 0ba3557 Compare July 30, 2022 12:09

patrick96 approved these changes Aug 21, 2022

View reviewed changes

patrick96 added this to the 3.7.0 milestone Aug 21, 2022

patrick96 merged commit 6ccecbf into polybar:master Aug 21, 2022

xphoniex deleted the xphoniex/add-zone-type-to-temperature-mod branch August 21, 2022 17:29

patrick96 mentioned this pull request Nov 5, 2023

Version 3.7.0 #3032

Merged

24 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(temperature): Add `zone-type` setting #2752

feat(temperature): Add `zone-type` setting #2752

xphoniex commented Jun 22, 2022 •

edited by patrick96

Loading

xphoniex commented Jun 30, 2022

patrick96 commented Jul 1, 2022

xphoniex commented Jul 2, 2022

patrick96 commented Jul 9, 2022

xphoniex commented Jul 9, 2022 •

edited

Loading

xphoniex commented Jul 23, 2022

codecov bot commented Jul 25, 2022 •

edited

Loading

patrick96 left a comment

patrick96 Jul 25, 2022

xphoniex Jul 30, 2022

patrick96 Jul 25, 2022

xphoniex Jul 30, 2022

patrick96 Jul 25, 2022

xphoniex Jul 28, 2022

patrick96 Jul 28, 2022

xphoniex Jul 30, 2022

xphoniex commented Jul 30, 2022

patrick96 commented Jul 30, 2022

xphoniex commented Aug 13, 2022

patrick96 commented Aug 21, 2022

	throw module_error("zone-type '" + m_zone_type + "' was not found, available zone types:" + available_zones);
	throw module_error("zone-type '" + m_zone_type + "' was not found, available zone types: " + available_zones);

feat(temperature): Add zone-type setting #2752

feat(temperature): Add zone-type setting #2752

Conversation

xphoniex commented Jun 22, 2022 • edited by patrick96 Loading

What type of PR is this? (check all applicable)

Description

Related Issues & Documents

Documentation (check all applicable)

xphoniex commented Jun 30, 2022

patrick96 commented Jul 1, 2022

xphoniex commented Jul 2, 2022

patrick96 commented Jul 9, 2022

xphoniex commented Jul 9, 2022 • edited Loading

xphoniex commented Jul 23, 2022

codecov bot commented Jul 25, 2022 • edited Loading

Codecov Report

patrick96 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xphoniex commented Jul 30, 2022

patrick96 commented Jul 30, 2022

xphoniex commented Aug 13, 2022

patrick96 commented Aug 21, 2022

feat(temperature): Add `zone-type` setting #2752

feat(temperature): Add `zone-type` setting #2752

xphoniex commented Jun 22, 2022 •

edited by patrick96

Loading

xphoniex commented Jul 9, 2022 •

edited

Loading

codecov bot commented Jul 25, 2022 •

edited

Loading