FreeBSD: add support for CPU frequency and temperature #436

cgzones · 2020-12-28T18:42:03Z

No description provided.

adriaandegroot · 2021-02-03T13:07:25Z

DisplayOptionsPanel.c

-   Panel_add(super, (Object*) CheckItem_newByRef("Also show CPU temperature (requires libsensors)", &(settings->showCPUTemperature)));
+   #ifdef BUILD_WITH_CPU_TEMP
+   Panel_add(super, (Object*) CheckItem_newByRef(
+   #ifdef HTOP_LINUX


This one (#ifdef) is inconsistent with the next case (#if defined), it'd be easier to read if both used the same preprocessor directive.

You mean:

Suggested change

#ifdef HTOP_LINUX

#if defined(HTOP_LINUX)

adriaandegroot · 2021-02-03T13:08:49Z

DisplayOptionsPanel.c

+                                                 "Also show CPU temperature (requires libsensors)",
+   #elif defined(HTOP_FREEBSD)
+                                                 "Also show CPU temperature",
+   #endif


If neither is defined, you'll get a peculiar compile error because of the missing parameter; I realise that the whole point is to have one or the other when BUILD_WITH_CPU_TEMP is set, but for future-proofing (e.g. OpenBSD support) you might have #else #error Unsupported HTOP OS setting or something like that.

adriaandegroot · 2021-02-03T13:12:54Z

freebsd/FreeBSDProcessList.c

+      if (pl->settings->showCPUTemperature) {
+         int temperature;
+         size_t len = sizeof(temperature);
+         char mibBuffer[32];


Probably worth mentioning that you need a buffer long enough for the dev.cpu..temperature string plus the length of the CPU number (so 21, plus 1 for NUL .. I suppose nobody has CPU identifiers running to 9 digits.

Yes and no. You actually need to provide enough space, that even for the maximum permissible CPU number (int32_t) you can still fit this into the buffer. Thus this is intentionally a few bytes larger than it ordinarily needs to be. There are other places in htop over-sizing certain buffers for similar reasons. You'll get the reasoning too, if you use -Wformat=2 with GCC, which also checks those sizes, IIRC.

I think what I meant was "a comment here saying '32 is big enough for the fixed string plus %d expands to max. 9 digits so it is sufficient' would be nice" because I did go back to count whether the buffer was large enough.

adriaandegroot · 2021-02-03T14:11:41Z

The code from @cgzones compiles, and it hits the right code paths.

I have 13-CURRENT from a month or so back, with an AMD Ryzen 3700X (8C16T). sysctl dev.cpu does not show any temperature data, but there is a freq sub-key. I may be missing a kernel module to do the temperature measurement.

Unfortunately, the CPU percentage and temperature and speed display black-on-black on my FreeBSD system; it's kind of hard to read. For whatever reason, Linux shows me light-grey on black (this is of course strongly dependent on the terminal that is in use). Regardless, copy-paste shows me it shows this on my FreeBSD machine:

  2[||||||||||||||||||||||||                                               27.0% N/A N/A]

For CPU2, that seems plausible except that neither temp -- see above, I don't have that information -- nor frequency is being displayed.

adriaandegroot · 2021-02-03T14:22:16Z

After some debugging -- and changing my terminal's color-scheme -- I get

 1[|                                                                1.0% 3600MHz 2345°C]
 2[||||||||                                                         8.7%  101MHz 2345°C]
 3[|                                                                0.5%  102MHz 2345°C]
 4[||                                                               0.9%  103MHz 2345°C]

Here temps and speeds are (obviously!) bogus, but I chased the various error paths in the code. Temperature 2345 means that the sysctlbyname() call returned -1: that is expected, since I don't have any temperature data. Speed 10x means that the sysctlbyname() call returned -1: looking over my sysctl dev.cpu output, I see that only CPU 0 has a frequency setting and the rest are missing.

So: modulo "my system does not provide all the information that htop is looking for" it seems to work.

adriaandegroot · 2021-02-03T14:38:13Z

After kldload amdtemp I do have suitable temperature readings. Note that frequency information is only available for CPU0; I asked a friend with a TR and an Intel system and they had temperatures (on TR, with the amdtemp kernel module) and also only a single CPU frequency available.

Overall: LGTM.

BenBE · 2021-02-03T14:57:03Z

Any suggestion what would be needed to have the temperature readings properly? I somewhere saw some documentation regarding the kernel returning multiples of 0.1 Kelvin.

As the question came up on Twitter:
What's missing is, mostly:

People to test with real hardware (we currently only have VMs available, thus no values that are reported for testing)
Suggestions for improvements to make the readings sensible: I'd go duck and cover quite fast, if the 2350°C was the real CPU temperature. Also I'd start asking myself how the CPU survived to display that value …

Thanks a lot for this feedback.

RhodiumToad · 2021-02-03T16:59:30Z

I tested on real hardware: FreeBSD 11.4-STABLE, 4-core Intel Skylake (i5-6500 CPU @ 3.20GHz), with cpufreq (built into kernel) and coretemp (loaded as a module) enabled (but powerd not running, so the frequency is fixed).

What I see is that htop correctly displays the same data exposed via sysctl (which means that the freq is shown only for CPU 0, the others show as N/A, while the temp is shown individually for each core).

RhodiumToad · 2021-02-03T17:05:42Z

Also I should note: some people have had build failures as a result of having the ncurses port installed (rather than using the base system ncurses) - this should be fixable with CPPFLAGS="-isystem /usr/local/include" (and possibly also LDFLAGS="-L/usr/local/lib") as additional args to ./configure (when building inside the ports framework, this should be handled automatically).

BenBE · 2021-02-03T17:14:27Z

Also I should note: some people have had build failures as a result of having the ncurses port installed (rather than using the base system ncurses) - this should be fixable with CPPFLAGS="-isystem /usr/local/include" (and possibly also LDFLAGS="-L/usr/local/lib") as additional args to ./configure (when building inside the ports framework, this should be handled automatically).

Does it make sense to check for this in configure and include these additional arguments automatically when the ncurses port is detected in contrast to version from the base system?

RhodiumToad · 2021-02-03T17:18:33Z

Does it make sense to check for this in configure and include these additional arguments automatically when the ncurses port is detected in contrast to version from the base system?

No, the ports framework is already doing this for you.

BenBE · 2021-02-03T17:28:08Z

Does it make sense to check for this in configure and include these additional arguments automatically when the ncurses port is detected in contrast to version from the base system?

No, the ports framework is already doing this for you.

Then I'm wondering why it's not picking things up through the configure. Can you investigate from configure.ac?

debdrup · 2021-02-03T18:14:10Z

I've also tested on real hardware (a ThinkPad T420 with an i5-2520 @ 2.5GHz) running FreeBSD 12.2-RELEASE, with the cpufreq and coretemp kernel modules loaded.
It shows both the temperatures of all cores and the frequency of the first core (which is expected, since cpufreq isn't per-CPU).
It also correctly displays the powerstepped temperature, when powerd(++) is running.

RhodiumToad · 2021-02-03T18:43:37Z

Then I'm wondering why it's not picking things up through the configure. Can you investigate from configure.ac?

Oh, sorry if I wasn't clear - the reported failures were from people building outside the ports system. If you want that to work out of the box when ports ncurses is installed, then you'd need to fix it somehow, but I don't know the best way of doing that without possibly interfering with the ports build.

cgzones added the FreeBSD 👹 FreeBSD related issues label Dec 28, 2020

BenBE approved these changes Dec 29, 2020

View reviewed changes

BenBE added this to the 3.0.6 milestone Jan 11, 2021

cgzones force-pushed the freebsd branch 2 times, most recently from 5391990 to 84a128c Compare January 30, 2021 22:48

cgzones mentioned this pull request Jan 30, 2021

Frequency and temperature in Freebsd #419

Closed

adriaandegroot reviewed Feb 3, 2021

View reviewed changes

FreeBSD: add support for CPU frequency and temperature

fd2a0cf

cgzones force-pushed the freebsd branch from 84a128c to fd2a0cf Compare February 5, 2021 15:32

ginggs marked this pull request as ready for review March 18, 2021 11:59

ginggs merged commit ee97916 into htop-dev:master Mar 18, 2021

cgzones deleted the freebsd branch March 19, 2021 13:41

BenBE mentioned this pull request May 5, 2021

please add temperature of core to head of monitoring screen #621

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FreeBSD: add support for CPU frequency and temperature #436

FreeBSD: add support for CPU frequency and temperature #436

cgzones commented Dec 28, 2020

adriaandegroot Feb 3, 2021

BenBE Feb 3, 2021

adriaandegroot Feb 3, 2021

adriaandegroot Feb 3, 2021

BenBE Feb 3, 2021

adriaandegroot Feb 3, 2021

adriaandegroot commented Feb 3, 2021

adriaandegroot commented Feb 3, 2021

adriaandegroot commented Feb 3, 2021

BenBE commented Feb 3, 2021

RhodiumToad commented Feb 3, 2021

RhodiumToad commented Feb 3, 2021 •

edited

Loading

BenBE commented Feb 3, 2021

RhodiumToad commented Feb 3, 2021

BenBE commented Feb 3, 2021

debdrup commented Feb 3, 2021 •

edited

Loading

RhodiumToad commented Feb 3, 2021

FreeBSD: add support for CPU frequency and temperature #436

FreeBSD: add support for CPU frequency and temperature #436

Conversation

cgzones commented Dec 28, 2020

adriaandegroot Feb 3, 2021

Choose a reason for hiding this comment

BenBE Feb 3, 2021

Choose a reason for hiding this comment

adriaandegroot Feb 3, 2021

Choose a reason for hiding this comment

adriaandegroot Feb 3, 2021

Choose a reason for hiding this comment

BenBE Feb 3, 2021

Choose a reason for hiding this comment

adriaandegroot Feb 3, 2021

Choose a reason for hiding this comment

adriaandegroot commented Feb 3, 2021

adriaandegroot commented Feb 3, 2021

adriaandegroot commented Feb 3, 2021

BenBE commented Feb 3, 2021

RhodiumToad commented Feb 3, 2021

RhodiumToad commented Feb 3, 2021 • edited Loading

BenBE commented Feb 3, 2021

RhodiumToad commented Feb 3, 2021

BenBE commented Feb 3, 2021

debdrup commented Feb 3, 2021 • edited Loading

RhodiumToad commented Feb 3, 2021

RhodiumToad commented Feb 3, 2021 •

edited

Loading

debdrup commented Feb 3, 2021 •

edited

Loading