FreeBSD adding temperature sensors (WIP) #1350

amanusk · 2018-10-20T06:49:51Z

The pull requests adds sensors_temperatures() capability for FreeBSD. This requires coretemp kernel module.

This is a "WIP" pull request to make sure this is on the right track.

@giampaolo, some questions:

In this implementation, the C code only returns a single tuple with current and max, while creating the dictionary is handled in python. This makes it easier to iterate over the number of cpus, although this can also be done in C. I believe the number of errors can be smaller this way, and the number of sysctlbyname calls does not change.
Should psutil_sensors_temperatures be named something else?
Is there a good open source tool to test pep7 compliance?

Current implementation returns the following when running scripts/temperatures.py

coretemp
    Core 0               46 °C (high = 105 °C, critical = 105 °C)
    Core 1               46 °C (high = 105 °C, critical = 105 °C)
    Core 2               48 °C (high = 105 °C, critical = 105 °C)
    Core 3               48 °C (high = 105 °C, critical = 105 °C)
    Core 4               46 °C (high = 105 °C, critical = 105 °C)
    Core 5               46 °C (high = 105 °C, critical = 105 °C)
    Core 6               43 °C (high = 105 °C, critical = 105 °C)
    Core 7               43 °C (high = 105 °C, critical = 105 °C)

* Add temperature sensors to freeBSD

giampaolo

Thanks Alex! This really is awesome work!
I added some comments inline. Other than that it would be nice to have some tests. In details what I would do is:

in test_bsd.py run sysctl cmdline tool, get the numbers of "dev.cpu.%d.temperature" entries and assert they are equal to len(psutil.sensors_temperatures())
for every psutil.sensors_temperatures() check "current" against "dev.cpu.%d.temperature" with a 10 units tolerance or something and "max" against "dev.cpu.%d.coretemp.tjmax" (no tolerance)

This requires coretemp kernel module.

What does this mean? Is it something you have to install? If so it may be worth mentioning it into doc.

Is there a good open source tool to test pep7 compliance?

Not that I'm aware of but it's worth having such a tool integrated in our workflow. For now I created #1351 to keep track of it.

giampaolo · 2018-10-20T08:57:52Z

psutil/arch/freebsd/specific.c

+        goto error;
+    tjmax = SYSCTLTEMP(tjmax);
+
+


giampaolo · 2018-10-20T09:04:05Z

psutil/arch/freebsd/specific.c

+
+    sprintf(sensor, "dev.cpu.%d.coretemp.tjmax", core);
+    if (sysctlbyname(sensor, &tjmax, &size, NULL, 0))
+        goto error;


Considering we can have a valid "current" temp I think it's better to ignore this error and set "max" temp to 0. Then from Python you can check if it's 0 and turn it into None.

giampaolo · 2018-10-20T09:06:31Z

psutil/_psbsd.py

+        num_cpus = cpu_count_logical()
+        try:
+            for cpu in range(num_cpus):
+                current, tjmax = cext.sensors_temperatures(cpu)


I think it's better to catch NotImplementedError here instead of against the whole for loop. That way if there are temperatures for certain CPUs only (instead of all of them) we can at least return a partial result.

Also high looks like a better name than tjmax because it reflects the namedtuple field name.

giampaolo · 2018-10-20T09:07:17Z

psutil/arch/freebsd/specific.c

+ * Return temperature information.
+ */
+PyObject *
+psutil_sensors_temperatures(PyObject *self, PyObject *args) {


I would rename this to psutil_sensors_cpu_temperature

giampaolo · 2018-10-20T09:09:16Z

psutil/_psbsd.py

@@ -432,6 +432,22 @@ def sensors_battery():
            secsleft = minsleft * 60
        return _common.sbattery(percent, secsleft, power_plugged)

+    def sensors_temperatures():
+        """Return systemp temperatures"""


I would rephrase this as: "Return CPU cores temperatures if available, else an empty dict."

giampaolo · 2018-10-20T09:15:53Z

psutil/_psbsd.py

+    def sensors_temperatures():
+        """Return systemp temperatures"""
+        ret = dict()
+        ret["coretemp"] = list()


This is not correct because if there are no temperatures you will return a {"coretemp": []} dict which will evaluate to True. Instead define ret as:

ret = collections.defaultdict(list)

...and when you add elements to it do:

ret["coretemp"].append(...)

Finally, at the end of the function transform it to a plain dict with:

return dict(ret)

giampaolo · 2018-10-20T09:18:57Z

psutil/arch/freebsd/specific.c

+
+
+/*
+ * Return temperature information.


"Return temperature information for a given CPU core number."

giampaolo · 2018-10-20T09:19:29Z

psutil/_psutil_bsd.c

@@ -981,6 +981,8 @@ PsutilMethods[] = {
 #if defined(PSUTIL_FREEBSD)
    {"sensors_battery", psutil_sensors_battery, METH_VARARGS,
     "Return battery information."},
+    {"sensors_temperatures", psutil_sensors_temperatures, METH_VARARGS,
+     "Return temperatures information."},


"Return temperature information for a given CPU core number."

giampaolo · 2018-10-20T09:35:46Z

psutil/_psbsd.py

+        try:
+            for cpu in range(num_cpus):
+                current, tjmax = cext.sensors_temperatures(cpu)
+                name = "Core {}".format(cpu)


Please use name = "Core %s" % cpu instead in order to be consistent with the rest of code base.

giampaolo · 2018-10-20T09:37:50Z

psutil/arch/freebsd/specific.c

@@ -31,6 +31,7 @@
 #define PSUTIL_TV2DOUBLE(t)    ((t).tv_sec + (t).tv_usec / 1000000.0)
 #define PSUTIL_BT2MSEC(bt) (bt.sec * 1000 + (((uint64_t) 1000000000 * (uint32_t) \
        (bt.frac >> 32) ) >> 32 ) / 1000000)
+#define SYSCTLTEMP(t) ((int)((t + 270)) - 3000) / 10


What's the logic here? Where did you take this? Are we returning Celsius? If there's some source code of reference it would be worth mentioning it here as a comment.

Yeah.. this bit kinda looks like a hack.. Explanation:

Calling sysctlbyname returns an integer. Usually something like this: 3121.

Always ending with 1, and starts with 3 for the temperatures I have observed.

By comparing this with sysctl calls, I was able to determine that the actual temperature is the middle 2 digits (in this case 12) with and offset of 27, so in this case: 39.

I wasn't able go find official documentation of this, so if you or a FreeBSD expert have a better idea of how to get actual values, that would be great.

Link to coretemp implementation:
https://github.com/freebsd/freebsd/blob/master/sys/dev/coretemp/coretemp.c

I see. It looks like coretemp.c implements the conversion logic like this:
https://github.com/freebsd/freebsd/blob/b3f6f192fe65a06ad95d7d6eef0c58a789e7326f/sys/dev/coretemp/coretemp.c#L372-L375
Ideally I think it would be better to do the same but practically I'm not sure if the same logic can be applied to your code.
I'm CC-ing @glebius, @sunpoet, @kostikbel just in case they know more about this.

https://www.intel.com/content/dam/www/public/us/en/documents/manuals/64-ia-32-architectures-software-developer-vol-3b-part-2-manual.pdf

Paragraph 14.8 there. Let me read myself :) and come with review .

Thanks @glebius.
Here's how netdata implements the conversion logic:
https://github.com/netdata/netdata/blob/545d07eb78222a80124e9187e88921013ee403ea/collectors/freebsd.plugin/freebsd_sysctl.c#L520

Okay, I reviewed and I don't understand why do you want to do conversion in the python module? The sysctl already returns Celsius. Looks like on your box it returns some strange data. What version of FreeBSD do you use @amanusk and what is the hardware?

@glebius, Thanks for you help with this.
freebsd-version -k returns 11.2-RELEASE

System is Thinkpad T430, with i7-3740QM

Running sysctl dev.cpu.0.temperature in Shell returns: dev.cpu.0.temperature: 42.0C which is indeed in Celsius. The bit where I need to convert is line 1030 in the discussed file (sorry for not knowing how to link line). It returns a 4 digit integer as explained in this conversion.

amanusk · 2018-10-20T11:36:24Z

@giampaolo, Thanks for the review. I wanted to make sure this is feasible before continuing work on this. (At least no licensing issues 😄 )

Other than that it would be nice to have some tests

Sure!

What does this mean? Is it something you have to install? If so it may be worth mentioning it into doc.

I am not much of an expert in FreeBSD myself, I got the idea for this PR from here: https://www.cyberciti.biz/faq/freebsd-determine-processor-cpu-temperature-command/

The kernel module needs to be enabled, but there is no need for additional installations. Definitely worth mentioning in the Doc.

amanusk · 2018-10-20T17:08:36Z

psutil/arch/freebsd/specific.c

+    if (! PyArg_ParseTuple(args, "i", &core))
+        return NULL;
+    sprintf(sensor, "dev.cpu.%d.temperature", core);
+    if (sysctlbyname(sensor, &current, &size, NULL, 0))


@glebius: This call returns a 4 digit integer, as explained.

I reproduced that. Looks strange indeed. Investigating.

I see. The sysctl has special format. It reports in deciKelvin. Here is example parser:

https://github.com/freebsd/freebsd/blob/master/sbin/sysctl/sysctl.c#L790

Just copy math from there. Your math is almost correct.

amanusk · 2018-11-02T16:06:53Z

@giampaolo, sorry for the long delay.
I have implemented the changes and the tests.

giampaolo · 2018-11-03T14:06:03Z

Thanks a lot Alex! Merging.

@EccoTheFlintstone

* origin/master: (182 commits) giampaolo#1394 / windows / process exe(): convert errno 0 into ERROR_ACCESS_DENIED; errno 0 occurs when the Python process runs in 'Virtual Secure Mode' pre-release fix win num_handles() test update readme fix giampaolo#1111: use a lock to make Process.oneshot() thread safe pdate HISTORY giampaolo#1373: different approach to oneshot() cache (pass Process instances around - which is faster) use PROCESS_QUERY_LIMITED_INFORMATION also for username() Linux: refactor _parse_stat_file() and return a dict instead of a list (+ maintainability) fix giampaolo#1357: do not expose Process' memory_maps() and io_counters() methods if not supported by the kernel giampaolo#1376 Windows: check if variable is NULL before free()ing it enforce lack of support for Win XP fix giampaolo#1370: improper usage of CloseHandle() may lead to override the original error code resulting in raising a wrong exception update HISTORY (Windows) use PROCESS_QUERY_LIMITED_INFORMATION access rights (giampaolo#1376) update HISTORY revert 5398c48; let's do it in a separate branch giampaolo#1111 make Process.oneshot() thread-safe sort HISTORY give CREDITS to @EccoTheFlintstone for giampaolo#1368 fix ionice set not working on windows x64 due to LENGTH_MISMATCH (giampaolo#1368) make flake8 happy give CREDITS to @amanusk for giampaolo#1369 / giampaolo#1352 and update doc Add CPU frequency support for FreeBSD (giampaolo#1369) giampaolo#1359: add test case for cpu_count(logical=False) against lscpu utility disable false positive mem test on travis + osx fix PEP8 style mistakes give credits to @koenkooi for giampaolo#1360 Fix giampaolo#1354 [Linux] disk_io_counters() fails on Linux kernel 4.18+ (giampaolo#1360) giampaolo#1350: give credits to @amanusk FreeBSD adding temperature sensors (WIP) (giampaolo#1350) pre release sensors_temperatures() / linux: convert defaultdict to dict fix giampaolo#1004: Process.io_counters() may raise ValueError fix giampaolo#1307: [Linux] disk_partitions() does not honour PROCFS_PATH refactor hasattr() checks as global constants giampaolo#1197 / linux / cpu_freq(): parse /proc/cpuinfo in case /sys/devices/system/cpu fs is not available fix giampaolo#1277 / osx / virtual_memory: 'available' and 'used' memory were not calculated properly travis / osx: set py 3.6 travis: disable pypy; se py 3.7 on osx skip test on PYPY + Travis fix travis fix giampaolo#715: do not print exception on import time in case cpu_times() fails. fix different travis failures give CREDITS for giampaolo#1320 to @truthbk [aix] improve compilation on AIX, better support for gcc/g++ + fix cpu metrics (giampaolo#1320) give credits to @alxchk for giampaolo#1346 (sunOS) Fix giampaolo#1346 (giampaolo#1347) giampaolo#1284, giampaolo#1345 - give credits to @amanusk Add parsing for /sys/class/thermal (giampaolo#1345) Fix decoding error in tests catch UnicodeEncodeError on print() use memory tolerance in occasionally failing test Fix random 0xC0000001 errors when querying for Connections (giampaolo#1335) Correct capitalization of PyPI (giampaolo#1337) giampaolo#1341: move open() utilities/wrappers in _common.py Refactored ps() function in test_posix (giampaolo#1341) fix giampaolo#1343: document Process.as_dict() attrs values giampaolo#1332 - update HISTORY make psutil_debug() aware of PSUTIL_DEBUG (giampaolo#1332) also include PYPY (or try to :P) travis: add python 3.7 build add download badge remove failing test assertions remove failing test make test more robust pre release pre release set version to 5.4.7 OSX / SMC / sensors: revert giampaolo#1284 (giampaolo#1325) setup.py: add py 3.7 fix giampaolo#1323: [Linux] sensors_temperatures() may fail with ValueError fix failing linux tests giampaolo#1321 add unit tests giampaolo#1321: refactoring make disk_io_counters more robust (giampaolo#1324) fix typo Fix DeprecationWarning: invalid escape sequence (giampaolo#1318) remove old test update is_storage_device() docstring fix giampaolo#1305 / disk_io_counters() / Linux: assume SECTOR_SIZE is a fixed 512 giampaolo#1313 remove test which no longer makes sense disk_io_counters() - linux: mimic iostat behavior (giampaolo#1313) fix wrong reference link in doc disambiguate TESTFN for parallel testing fix giampaolo#1309: add STATUS_PARKED constant and fix STATUS_IDLE (both on linux) give CREDITS to @sylvainduchesne for giampaolo#1294 retain GIL when querying connections table (giampaolo#1306) Update index.rst (giampaolo#1308) fix giampaolo#1279: catch and skip ENODEV in net_if_stat() appveyor: retire 3.5, add 3.7 revert file renaming of macos files; get them back to 'osx' prefix winmake: add upload-wheels cmd Rename OSX to macOS (giampaolo#1298) apveyor: reset py 3.4 and remove 3.7 (not available yet) try to fix occasional children() failure on Win: https://ci.appveyor.com/project/giampaolo/psutil/build/job/je3qyldbb86ff66h appveyor: remove py 3.4 and add 3.7 giampaolo#1284: give credits to @amanusk + some minor adjustments little refactoring Osx temps (giampaolo#1284) ...

FreeBSD adding temperature sensors

2d1bb43

* Add temperature sensors to freeBSD

giampaolo requested changes Oct 20, 2018

View reviewed changes

amanusk commented Oct 20, 2018

View reviewed changes

Add tests for sensors_temperatures on freeBSD

703f3dc

giampaolo merged commit bb5d032 into giampaolo:master Nov 3, 2018

giampaolo added a commit that referenced this pull request Nov 3, 2018

#1350: give credits to @amanusk

7ddfa9a

giampaolo added the freebsd label Nov 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FreeBSD adding temperature sensors (WIP) #1350

FreeBSD adding temperature sensors (WIP) #1350

amanusk commented Oct 20, 2018

giampaolo left a comment •

edited

Loading

giampaolo Oct 20, 2018

giampaolo Oct 20, 2018

giampaolo Oct 20, 2018 •

edited

Loading

giampaolo Oct 20, 2018

giampaolo Oct 20, 2018

giampaolo Oct 20, 2018

giampaolo Oct 20, 2018

giampaolo Oct 20, 2018

giampaolo Oct 20, 2018

giampaolo Oct 20, 2018

amanusk Oct 20, 2018

giampaolo Oct 20, 2018

glebius Oct 20, 2018

giampaolo Oct 20, 2018

glebius Oct 20, 2018

amanusk Oct 20, 2018

amanusk commented Oct 20, 2018

amanusk Oct 20, 2018

glebius Oct 20, 2018

glebius Oct 20, 2018

amanusk commented Nov 2, 2018

giampaolo commented Nov 3, 2018

FreeBSD adding temperature sensors (WIP) #1350

FreeBSD adding temperature sensors (WIP) #1350

Conversation

amanusk commented Oct 20, 2018

giampaolo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giampaolo Oct 20, 2018 • edited Loading

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

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

amanusk commented Oct 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanusk commented Nov 2, 2018

giampaolo commented Nov 3, 2018

giampaolo left a comment •

edited

Loading

giampaolo Oct 20, 2018 •

edited

Loading