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

FreeBSD adding temperature sensors (WIP) #1350

Merged
merged 2 commits into from
Nov 3, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions psutil/_psbsd.py
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,22 @@ def sensors_battery():
secsleft = minsleft * 60
return _common.sbattery(percent, secsleft, power_plugged)

def sensors_temperatures():
"""Return systemp temperatures"""
Copy link
Owner

Choose a reason for hiding this comment

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

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

ret = dict()
ret["coretemp"] = list()
Copy link
Owner

Choose a reason for hiding this comment

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

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)

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

@giampaolo giampaolo Oct 20, 2018

Choose a reason for hiding this comment

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

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.

name = "Core {}".format(cpu)
Copy link
Owner

Choose a reason for hiding this comment

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

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

ret["coretemp"].append(
_common.shwtemp(name, current, tjmax, tjmax))
except NotImplementedError:
return None

return ret


# =====================================================================
# --- other system functions
Expand Down
2 changes: 2 additions & 0 deletions psutil/_psutil_bsd.c
Original file line number Diff line number Diff line change
Expand Up @@ -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."},
Copy link
Owner

Choose a reason for hiding this comment

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

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

#endif

// --- others
Expand Down
36 changes: 36 additions & 0 deletions psutil/arch/freebsd/specific.c
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@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.

#ifndef _PATH_DEVNULL
#define _PATH_DEVNULL "/dev/null"
#endif
Expand Down Expand Up @@ -1010,3 +1011,38 @@ psutil_sensors_battery(PyObject *self, PyObject *args) {
PyErr_SetFromErrno(PyExc_OSError);
return NULL;
}


/*
* Return temperature information.
Copy link
Owner

Choose a reason for hiding this comment

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

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

*/
PyObject *
psutil_sensors_temperatures(PyObject *self, PyObject *args) {
Copy link
Owner

Choose a reason for hiding this comment

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

I would rename this to psutil_sensors_cpu_temperature

int current;
int tjmax;
int core;
char sensor[26];
size_t size = sizeof(current);

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I reproduced that. Looks strange indeed. Investigating.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

goto error;
current = SYSCTLTEMP(current);

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

Choose a reason for hiding this comment

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

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.

tjmax = SYSCTLTEMP(tjmax);


Copy link
Owner

Choose a reason for hiding this comment

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

extra line

return Py_BuildValue("ii", current, tjmax);

error:
if (errno == ENOENT)
PyErr_SetString(PyExc_NotImplementedError, "no temperature sensors");
else
PyErr_SetFromErrno(PyExc_OSError);
return NULL;
}
1 change: 1 addition & 0 deletions psutil/arch/freebsd/specific.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,5 @@ PyObject* psutil_virtual_mem(PyObject* self, PyObject* args);
PyObject* psutil_cpu_stats(PyObject* self, PyObject* args);
#if defined(PSUTIL_FREEBSD)
PyObject* psutil_sensors_battery(PyObject* self, PyObject* args);
PyObject* psutil_sensors_temperatures(PyObject* self, PyObject* args);
#endif