13
295
Fork
You've already forked yambar
74

modules: add temperature module #144

Open
aacebedo wants to merge 2 commits from aacebedo/yambar:temp-module into master
pull from: aacebedo/yambar:temp-module
merge into: dnkl:master
dnkl:master
dnkl:releases/1.11
dnkl:releases/1.10
dnkl:releases/1.9
dnkl:releases/1.8
dnkl:releases/1.7
dnkl:releases/1.6
dnkl:releases/1.5
dnkl:releases/1.4
dnkl:releases/1.3
dnkl:releases/1.2
dnkl:releases/1.1
dnkl:releases/1.0
Contributor
Copy link

This module displays the temperature from the thermal zones of the machine

This module displays the temperature from the thermal zones of the machine
modules: add temp module
Some checks failed
continuous-integration/woodpecker the build failed
9bbe57a306
dnkl left a comment
Copy link

Nice little module, thanks :)

Note that there's also a CI failure that needs to be fixed.

Nice little module, thanks :) Note that there's also a CI failure that needs to be fixed.
CHANGELOG.md Outdated
@ -18,6 +18,7 @@
(https://codeberg.org/dnkl/yambar/issues/139).
* mem: a module handling system memory monitoring
* cpu: a module offering cpu usage monitoring
* temp: a module displaying the temperature of the thermal zones in the machine.
Owner
Copy link

I'd suggest renaming the module (filename, in documentation - i.e. everywhere) to temperature, to avoid any confusion with e.g. temporary.

I'd suggest renaming the module (filename, in documentation - i.e. everywhere) to `temperature`, to avoid any confusion with e.g. _temporary_.
Author
Contributor
Copy link

corrected

corrected
Author
Contributor
Copy link

I let some variable named temp (read_temp or enum name for instance). Do you want it to be replaced by temperature too ? (I find temperature being quiet long)

I let some variable named temp (read_temp or enum name for instance). Do you want it to be replaced by temperature too ? (I find temperature being quiet long)
Owner
Copy link

No, that's fine, keep it that way.

No, that's fine, keep it that way.
dnkl marked this conversation as resolved
@ -0,0 +26,4 @@
| unit
: string
: no
: Unit of the temperature. It can be Celsius (C) or Farenheit (F) (default=C)
Owner
Copy link

I suggest allowing both the short versions, and the long versions:

  • C
  • Celsuis
  • F
  • Fahrenheit
I suggest allowing both the short versions, and the long versions: * C * Celsuis * F * Fahrenheit
Author
Contributor
Copy link

corrected

corrected
@ -0,0 +42,4 @@
interval: 1000
thermal_zone: 0
content:
string: {text: "{temp}C"}
Owner
Copy link

Let's get fancy and use °C (DEGREE CELSIUS) instead of a simple "C"

Let's get fancy and use _°C_ (DEGREE CELSIUS) instead of a simple "C"
Author
Contributor
Copy link

Corrected

Corrected
dnkl marked this conversation as resolved
modules/temp.c Outdated
@ -0,0 +78,4 @@
break;
case TEMP_UNIT_FAHRENHEIT:
*temp = read_temp;
*temp = (*temp * (9 / 5)) + 32;
Owner
Copy link

The 9 / 5 part will be done as an integer division, thus always resulting in 1. This means the compiler will optimize this expression to *temp + 32, which obviously isn't correct.

You need to use floats here instead: 9.0 / 5.0.

The `9 / 5` part will be done as an integer division, thus always resulting in `1`. This means the compiler will optimize this expression to `*temp + 32`, which obviously isn't correct. You need to use floats here instead: `9.0 / 5.0`.
Author
Contributor
Copy link

My bad I thought the type would have bin infered from the variable type but forgot the parenthesis would have been evaluated before.

Corrected

My bad I thought the type would have bin infered from the variable type but forgot the parenthesis would have been evaluated before. Corrected
dnkl marked this conversation as resolved
modules/temp.c Outdated
@ -0,0 +82,4 @@
break;
default:
goto exit;
break;
Owner
Copy link

break not necessary here

`break` not necessary here
Author
Contributor
Copy link

corrected by simplify it

corrected by simplify it
dnkl marked this conversation as resolved
modules/temp.c Outdated
@ -0,0 +114,4 @@
return exposable;
}
#include <pthread.h>
Owner
Copy link

Remove this :)

Remove this :)
Author
Contributor
Copy link

Corrected

Corrected
dnkl marked this conversation as resolved
modules/temp.c Outdated
@ -0,0 +221,4 @@
static const struct attr_info attrs[] = {
{"interval", false, &conf_verify_interval},
{"thermal_zone", true, &conf_verify_unsigned},
{"unit", false, &conf_verify_unit},
Owner
Copy link

There's a conf_verify_enum() that I think would be more appropriate here (and maybe in the mem/cpu modules as well?)

See the i3 module for an example.

There's a `conf_verify_enum()` that I think would be more appropriate here (and maybe in the mem/cpu modules as well?) See the `i3` module for an example.
Author
Contributor
Copy link

Corrected

Corrected
dnkl marked this conversation as resolved
aacebedo force-pushed temp-module from f1fa539bb7
All checks were successful
continuous-integration/woodpecker the build was successful
to 9a4062db7c
All checks were successful
continuous-integration/woodpecker the build was successful
2021年12月29日 15:52:22 +01:00 Compare
aacebedo changed title from (削除) modules: add temp module (削除ここまで) to modules: add temperature module 2021年12月29日 15:55:18 +01:00
dnkl left a comment
Copy link

See comments below.

See comments below.
@ -0,0 +44,4 @@
thermal_zone: 0
content:
string: {text: "{temperature}°C (DEGREE CELSIUS)"}
```
Owner
Copy link

I think we can skip the "(DEGREE CELSIUS)" part... yes? I just mentioned it because it's the name of the Unicode codepoint.

I think we can skip the "(DEGREE CELSIUS)" part... yes? I just mentioned it because it's the name of the Unicode codepoint.
@ -0,0 +97,4 @@
}
struct tag_set tags = {
.tags = (struct tag *[]){tag_new_int(mod, "temperature", round(temperature))},
Owner
Copy link

Wouldn't a float tag be better here?

If it's about presentation, i.e. to be able to display it without decimals, perhaps we should add an :int formatter? Then you could do {string: {text: "{temperature:int}"}}.

Wouldn't a float tag be better here? If it's about presentation, i.e. to be able to display it without decimals, perhaps we should add an `:int` formatter? Then you could do `{string: {text: "{temperature:int}"}}`.
Owner
Copy link

Or even better, a way to specify the number of decimals to use. Perhaps {tag:.1}, for 1 decimal?

Or even better, a way to specify the number of decimals to use. Perhaps `{tag:.1}`, for 1 decimal?
Author
Contributor
Copy link

In fact I was thinking about improving the current system to specify a format and was wondering if you would be ok with it.

Should we let a way to change the entire format string when printing the values ? Or shall we limit the format to the precision ?

I think we could have a security issue if the format given by the user in the tag is directly used in the sprintf function in the *_as_string functions.

For float I am wondering if the %g format specifier is able to add the decimal part depending on the value of it (for .00 it shall not appear).

In fact I was thinking about improving the current system to specify a format and was wondering if you would be ok with it. Should we let a way to change the entire format string when printing the values ? Or shall we limit the format to the precision ? I think we could have a security issue if the format given by the user in the tag is directly used in the sprintf function in the `*_as_string` functions. For float I am wondering if the %g format specifier is able to add the decimal part depending on the value of it (for .00 it shall not appear).
Owner
Copy link

I don't think allowing a generic printf format string is a good idea. One, we must make sure it's limited to a single argument/value. And two, we need to parse/recognize the formatter, to know whether to call as_int(), as_float() or as_string().

However, a limited set of printf formatters is a good idea. Then we could, optionally, deprecate/remove the current hex and oct formatters.

So, perhaps the right solution is to accept integer, float and string formatters, but not pass them directly to sprintf()? For example, the user shouldn't have to use the correct length modifier. I.e. %d should be enough for an integer (and not %ld).

I think what we'll end up with is:

  • Integer formatters: decimal, hex, octal (d, u, o, x, X)
  • Float formatters: e, E, f, F, g, G
  • String formatters: s
  • Flag characters1
  • Field width
  • Precision modifiers
I don't think allowing a generic printf format string is a good idea. One, we must make sure it's limited to a single argument/value. And two, we need to parse/recognize the formatter, to know whether to call `as_int()`, `as_float()` or `as_string()`. However, a limited set of printf formatters **is** a good idea. Then we could, optionally, deprecate/remove the current `hex` and `oct` formatters. So, perhaps the right solution is to accept integer, float and string formatters, but not pass them directly to `sprintf()`? For example, the user shouldn't have to use the correct length modifier. I.e. `%d` should be enough for an integer (and not `%ld`). I think what we'll end up with is: * Integer formatters: decimal, hex, octal (d, u, o, x, X) * Float formatters: e, E, f, F, g, G * String formatters: s * Flag characters1 * Field width * Precision modifiers
Collaborator
Copy link

Now that we have a way of specifying how many digits a float should use, I believe temperature should just be a normal float.

Now that we have a way of specifying how many digits a float should use, I believe `temperature` should just be a normal `float`.
Owner
Copy link

I agree :)

I agree :)
Collaborator
Copy link

I have been revisiting this, since currently the temperature module is the only thing left in my configuration that's still not a built-in module.

Looking at this implementation, @aacebedo is reading from /sys/class/thermal/thermal_zone*/temp. The issue I have with that is that the thermal_zone only specifies a sort of mysterious "temperature", that we can't easily associate with anything.

I would like to propose that instead, we implement the temperature module based on the hardware monitors: /sys/class/hwmon/hwmon*/.

Those directories contain a lot of things that will be much more helpful to the end user, imo, such as:

  • The name of which device is doing the measurement (so you can easily distinguish between cpu, gpu, nvme, etc.
  • The critical temperature of each sensor.

This lets us easily offer to the user a lot more configuration. For example:

  • we can use a conditions to only get the sensors we want.
  • we can calculate how close a sensor is to its critical temperature, and offer it as a percentage (we must take care in the implementation, since this can technically go over 100%)
  • we can calculate which sensor is the closest to its critical temperature, and offer it as a special tag, so people can chose only to display the most heated sensor at a time (I really want this hehehe)
  • we can get the fans speed through fan*_input*
  • we can get the amount of power being drawn with power*_average*

And probably more stuff I am not aware of.

At this point, however, I am not sure we should call this new module temperature. Perhaps we should call it hwmon, for "hardware monitor".

What do you think, @dnkl?

  • Note these depend on the driver actually offering us that information, which, not all of them do.
I have been revisiting this, since currently the temperature module is the only thing left in my configuration that's still not a built-in module. Looking at this implementation, @aacebedo is reading from `/sys/class/thermal/thermal_zone*/temp`. The issue I have with that is that the `thermal_zone` only specifies a sort of mysterious "temperature", that we can't easily associate with anything. I would like to propose that instead, we implement the temperature module based on the hardware monitors: `/sys/class/hwmon/hwmon*/`. Those directories contain a lot of things that will be much more helpful to the end user, imo, such as: * The **name** of which device is doing the measurement (so you can easily distinguish between `cpu`, `gpu`, `nvme`, etc. * The **critical temperature** of each sensor. This lets us easily offer to the user a lot more configuration. For example: * we can use a `conditions` to only get the sensors we want. * we can calculate how close a sensor is to its critical temperature, and offer it as a percentage (we must take care in the implementation, since this can technically go over 100%) * we can calculate which sensor is the closest to its critical temperature, and offer it as a special tag, so people can chose only to display the most heated sensor at a time (I really want this hehehe) * we can get the fans speed through `fan*_input`\* * we can get the amount of power being drawn with `power*_average`\* And probably more stuff I am not aware of. At this point, however, I am not sure we should call this new module `temperature`. Perhaps we should call it `hwmon`, for "hardware monitor". What do you think, @dnkl? ------- * Note these depend on the driver actually offering us that information, which, not all of them do.
Collaborator
Copy link

I found that in the elixir linux documentation, of all places, we have a pretty good description of everything we can get with hwmon. We don't have to implement everything, to be sure (especially since I personally wouldn't even be able to test everything), but it is good to know nevertheless.

EDIT: ok, I also finally found the official kernel documentation as well: https://www.kernel.org/doc/Documentation/hwmon/sysfs-interface

I found that in the [elixir linux documentation](https://elixir.bootlin.com/linux/v6.2-rc1/source/Documentation/hwmon/sysfs-interface.rst), of all places, we have a pretty good description of everything we can get with hwmon. We don't have to implement everything, to be sure (especially since I personally wouldn't even be able to test everything), but it is good to know nevertheless. ---- EDIT: ok, I also finally found the official kernel documentation as well: https://www.kernel.org/doc/Documentation/hwmon/sysfs-interface
Collaborator
Copy link

Apologies for spaming this thread, but I've cooked up a very simple example just to show what the output could look like. Here's what I got in my machine:

nvme
	temp1_alarm: 0.000000
	temp1_crit: 84850.000000
	temp1_input: 45850.000000
	temp1_label: 0.000000
	temp1_max: 79850.000000
	temp1_min: -40150.000000
	temp2_input: 59850.000000
	temp2_label: 0.000000
	temp2_max: 65261848.000000
	temp2_min: -273150.000000
	temp3_input: 45850.000000
	temp3_label: 0.000000
	temp3_max: 65261848.000000
	temp3_min: -273150.000000
acpitz
	temp1_crit: 20800.000000
	temp1_input: 16800.000000
	temp2_crit: 119000.000000
	temp2_input: 27800.000000
coretemp
	temp1_crit: 100000.000000
	temp1_crit_alarm: 0.000000
	temp1_input: 33000.000000
	temp1_label: 0.000000
	temp1_max: 84000.000000
	temp2_crit: 100000.000000
	temp2_crit_alarm: 0.000000
	temp2_input: 33000.000000
	temp2_label: 0.000000
	temp2_max: 84000.000000
	temp3_crit: 100000.000000
	temp3_crit_alarm: 0.000000
	temp3_input: 32000.000000
	temp3_label: 0.000000
	temp3_max: 84000.000000
	temp4_crit: 100000.000000
	temp4_crit_alarm: 0.000000
	temp4_input: 33000.000000
	temp4_label: 0.000000
	temp4_max: 84000.000000
	temp5_crit: 100000.000000
	temp5_crit_alarm: 0.000000
	temp5_input: 31000.000000
	temp5_label: 0.000000
	temp5_max: 84000.000000
	temp6_crit: 100000.000000
	temp6_crit_alarm: 0.000000
	temp6_input: 33000.000000
	temp6_label: 0.000000
	temp6_max: 84000.000000
	temp7_crit: 100000.000000
	temp7_crit_alarm: 0.000000
	temp7_input: 31000.000000
	temp7_label: 0.000000
	temp7_max: 84000.000000
amdgpu
	temp1_crit: 94000.000000
	temp1_crit_hyst: -273150.000000
	temp1_input: 58000.000000
	temp1_label: 0.000000

I used a regex to only get the temperature information. Of course, implementing this well and correctly would be actually a lot more work than the module being proposed in this PR, but, overall, I still think it is worth it (note that i.e. in my silly example temp*_label is being read as a float, not a string, like it should).

Apologies for spaming this thread, but I've cooked up a very simple example just to show what the output could look like. Here's what I got in my machine: ``` nvme temp1_alarm: 0.000000 temp1_crit: 84850.000000 temp1_input: 45850.000000 temp1_label: 0.000000 temp1_max: 79850.000000 temp1_min: -40150.000000 temp2_input: 59850.000000 temp2_label: 0.000000 temp2_max: 65261848.000000 temp2_min: -273150.000000 temp3_input: 45850.000000 temp3_label: 0.000000 temp3_max: 65261848.000000 temp3_min: -273150.000000 acpitz temp1_crit: 20800.000000 temp1_input: 16800.000000 temp2_crit: 119000.000000 temp2_input: 27800.000000 coretemp temp1_crit: 100000.000000 temp1_crit_alarm: 0.000000 temp1_input: 33000.000000 temp1_label: 0.000000 temp1_max: 84000.000000 temp2_crit: 100000.000000 temp2_crit_alarm: 0.000000 temp2_input: 33000.000000 temp2_label: 0.000000 temp2_max: 84000.000000 temp3_crit: 100000.000000 temp3_crit_alarm: 0.000000 temp3_input: 32000.000000 temp3_label: 0.000000 temp3_max: 84000.000000 temp4_crit: 100000.000000 temp4_crit_alarm: 0.000000 temp4_input: 33000.000000 temp4_label: 0.000000 temp4_max: 84000.000000 temp5_crit: 100000.000000 temp5_crit_alarm: 0.000000 temp5_input: 31000.000000 temp5_label: 0.000000 temp5_max: 84000.000000 temp6_crit: 100000.000000 temp6_crit_alarm: 0.000000 temp6_input: 33000.000000 temp6_label: 0.000000 temp6_max: 84000.000000 temp7_crit: 100000.000000 temp7_crit_alarm: 0.000000 temp7_input: 31000.000000 temp7_label: 0.000000 temp7_max: 84000.000000 amdgpu temp1_crit: 94000.000000 temp1_crit_hyst: -273150.000000 temp1_input: 58000.000000 temp1_label: 0.000000 ``` I used a regex to only get the temperature information. Of course, implementing this well and correctly would be actually a lot more work than the module being proposed in this PR, but, overall, I still think it is worth it (note that i.e. in my silly example temp*_label is being read as a float, not a string, like it should).
Owner
Copy link

@Horus I agree, this would be super nice to have. Can you open a new issue for this? Instead of hijacking this PR...

hwmon seems like a reasonable name. I haven't looked at the details, of what it can offer, but all your suggestions make sense so far.

@Horus I agree, this would be super nice to have. Can you open a new issue for this? Instead of hijacking this PR... `hwmon` seems like a reasonable name. I haven't looked at the details, of what it can offer, but all your suggestions make sense so far.
First-time contributor
Copy link

Hi there. Any update on this? I was hoping to add a temperature module to my yambar.

Hi there. Any update on this? I was hoping to add a temperature module to my yambar.
Contributor
Copy link

In case anyone is interested, I created a temperature module for Yambar: yambar-temperature.

It's very basic and just polls the temperatures using libsensors.

In case anyone is interested, I created a temperature module for Yambar: [`yambar-temperature`](https://github.com/Delgan/yambar-temperature). It's very basic and just polls the temperatures using `libsensors`.
Contributor
Copy link

This looks sensible to me. One thing I'd request on top of the existing review is to just expose temperature_f and temperature_c tags instead of a configuration to flip between them. If you're interested, feel free to submit to maybar.

This looks sensible to me. One thing I'd request on top of the existing review is to just expose `temperature_f` and `temperature_c` tags instead of a configuration to flip between them. If you're interested, feel free to submit to [`maybar`](https://codeberg.org/mathstuf/maybar).
All checks were successful
continuous-integration/woodpecker the build was successful
This pull request has changes conflicting with the target branch.
  • CHANGELOG.md
  • modules/meson.build
  • plugin.c
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u temp-module:aacebedo-temp-module
git switch aacebedo-temp-module

Merge

Merge the changes and update on Forgejo.
git switch master
git merge --no-ff aacebedo-temp-module
git switch aacebedo-temp-module
git rebase master
git switch master
git merge --ff-only aacebedo-temp-module
git switch aacebedo-temp-module
git rebase master
git switch master
git merge --no-ff aacebedo-temp-module
git switch master
git merge --squash aacebedo-temp-module
git switch master
git merge --ff-only aacebedo-temp-module
git switch master
git merge aacebedo-temp-module
git push origin master
Sign in to join this conversation.
No reviewers
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
6 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
dnkl/yambar!144
Reference in a new issue
dnkl/yambar
No description provided.
Delete branch "aacebedo/yambar:temp-module"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?