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

Add parsing for /sys/class/thermal #1345

Merged
merged 5 commits into from
Oct 1, 2018
Merged

Add parsing for /sys/class/thermal #1345

merged 5 commits into from
Oct 1, 2018

Conversation

amanusk
Copy link
Collaborator

@amanusk amanusk commented Sep 27, 2018

This pull request addresses #1310.

Basically, in case no files were found that match /sys/class/hwmon, which should have more information, look at /sys/class/thermal

When running python scripts/temperatures.py, (after "hiding" /sys/class/hwmon ) it returns:
On Thinkpad T430 + Ubuntu:

x86_pkg_temp
    x86_pkg_temp         53.0 °C (high = None °C, critical = None °C)

acpitz
    acpitz               46.0 °C (high = 103.0 °C, critical = 103.0 °C)

On raspberry pi 3 + Raspbian:

cpu-thermal
    cpu-thermal          46.16 °C (high = None °C, critical = None °C)

Need to add unit tests for this

In case no files are found in /sys/class/hwmon, check for temperature
indicating sensors in /sys/class/thermal
Copy link
Owner

@giampaolo giampaolo left a comment

Choose a reason for hiding this comment

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

Nice work. I added some inline comments.

psutil/_pslinux.py Outdated Show resolved Hide resolved
psutil/_pslinux.py Outdated Show resolved Hide resolved
psutil/_pslinux.py Outdated Show resolved Hide resolved
trip_paths = glob.glob(base + '/trip_point*')
trip_points = [os.path.basename(p) for p in trip_paths]
trip_points = sorted(set(["_".join(x.split('_')[0:3])
for x in trip_points]))
Copy link
Owner

Choose a reason for hiding this comment

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

Mmmm... I have mixed feelings about this. You make 2 for loops here and another 1 later for a total of 3. I wonder if there's a way to reduce the number of loops.

Add Fallback for cat when getting high/critical
Copy link
Collaborator Author

@amanusk amanusk left a comment

Choose a reason for hiding this comment

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

@giampaolo, applied your requested changes.

I would like to ask for your help in writing a test for this case.
My understanding from this:

with mock.patch(patch_point, side_effect=open_mock):
            with mock.patch('glob.glob',
                            return_value=['/sys/class/hwmon/hwmon0/temp1']):

Is that glob.glob is replaces, and always returns /sys/class/hwmon/hwmon0/temp1.
In order to test this case, we need it to return [ ] the first time, and something like ['/sys/class/thermal/thermal_zone0'] the second time.
I unfortunately don't know how to do this.

Or is there a completely different approach?

psutil/_pslinux.py Outdated Show resolved Hide resolved
continue

trip_paths = glob.glob(base + '/trip_point*')
trip_points = set(['_'.join(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Squashed 2 for loops into one

Fallback=None)
elif trip_type == 'high':
high = cat(os.path.join(base, trip_point + "_temp"),
Fallback=None)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added Fallback

@giampaolo
Copy link
Owner

Nice work!

I would like to ask for your help in writing a test for this case.

Sure. Not tested, just pseudo code, but it should give you an idea:

def glob_mock(path):
    if path == ...:
        return ...
    else:
        return ...

with mock.patch('glob.glob', create=True, side_effect=glob_mock):
    ...

The tests check for correct return value when there is not
/sys/class/hwmon directory, but /sys/class/thermal/ is present
@amanusk
Copy link
Collaborator Author

amanusk commented Sep 28, 2018

Sure. Not tested, just pseudo code, but it should give you an idea

Thanks. It works. Added the test.

Copy link
Owner

@giampaolo giampaolo left a comment

Choose a reason for hiding this comment

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

Thanks. I think it's better to split the two tests though that exercise the 2 glob.glob branches separately. Can you do that?

@@ -1466,6 +1466,8 @@ def test_emulate_eio_error(self):
def open_mock(name, *args, **kwargs):
if name.endswith("_input"):
raise OSError(errno.EIO, "")
if name.endswith("temp"):
Copy link
Owner

Choose a reason for hiding this comment

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

elif

@amanusk
Copy link
Collaborator Author

amanusk commented Oct 1, 2018

Ok, split the tests for /sys/class/hwmon and /sys/class/thermal

Copy link
Owner

@giampaolo giampaolo left a comment

Choose a reason for hiding this comment

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

Excellent job. Thanks. Merging.

@giampaolo giampaolo merged commit 13ef73c into giampaolo:master Oct 1, 2018
giampaolo added a commit that referenced this pull request Oct 1, 2018
@amanusk
Copy link
Collaborator Author

amanusk commented Oct 1, 2018

Thanks!
I would like to try and implement sensors_temperatures() for FreeBSD. Would this be interesting? Should I open a new issue for this?

@giampaolo
Copy link
Owner

giampaolo commented Oct 1, 2018

Yeah, that would be great to have (and needs new issue or direct PR). But did you investigate it already whether it is possible at all?

@amanusk
Copy link
Collaborator Author

amanusk commented Oct 1, 2018

I think it should be. For example:
sysctl -n dev.cpu.0.temperature returns the cpu temperature, and sysctl -n dev.cpu.0.coretemp.tjmax should be the critical temperature.
I assume these can be called from C code, similarly to how sensors_battery() is implemented.

@giampaolo
Copy link
Owner

Oh nice! Can you paste the output of sysctl -a | grep temp? I'm curious if there are other temperatures other than for CPU. It looks like CPU temps are numbered so perhaps a progressive loop (1, 2, 3, ...) is needed until sysctl() returns an error or something. OSX is similar.

And yes, that can easily be done in C via sysctlbyname syscall. FreeBSD code already does that extensively (grep for it).

nlevitt added a commit to nlevitt/psutil that referenced this pull request Apr 9, 2019
* 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)
  ...
@rforro
Copy link

rforro commented Mar 11, 2020

There is a bug if you trying to get the CPU temperature and you are also using an external 1-Wire temperature sensor.

I'm running Raspberry zero with Raspbian Buster (Linux version 4.19.97+ (dom@buildbot) (gcc version 4.9.3 (crosstool-NG crosstool-ng-1.22.0-88-g8460611))

The command ll /sys/class/hwmon/hwmon* shows 2 temperature sensors one is the external 1-Wire and the internal, which we want to read:

/sys/class/hwmon/hwmon0 -> ../../devices/w1_bus_master1/28-0316a5e4f7ff/hwmon/hwmon0
/sys/class/hwmon/hwmon1 -> ../../devices/platform/soc/soc:firmware/raspberrypi-hwmon/hwmon/hwmon1

1-Wire ll /sys/class/hwmon/hwmon0/:

device -> ../../../28-0316a5e4f7ff
name
power
subsystem -> ../../../../../class/hwmon
temp1_input
uevent

Internal ll /sys/class/hwmon/hwmon1/:

device -> ../../../raspberrypi-hwmon
in0_lcrit_alarm
name
power
subsystem -> ../../../../../../../class/hwmon
uevent

As you can see the external 1-Wire includes temp1_input but the internal one don't and that's the problem. As far as I understand the code looks at first into /sys/class/hwmon/

psutil/psutil/_pslinux.py

Lines 1204 to 1213 in ea4887e

basenames = glob.glob('/sys/class/hwmon/hwmon*/temp*_*')
# CentOS has an intermediate /device directory:
# https://github.com/giampaolo/psutil/issues/971
# https://github.com/nicolargo/glances/issues/1060
basenames.extend(glob.glob('/sys/class/hwmon/hwmon*/device/temp*_*'))
basenames.extend(glob.glob(
'/sys/devices/platform/coretemp.*/hwmon/hwmon*/temp*_*'))
basenames = sorted(set([x.split('_')[0] for x in basenames]))
for base in basenames:

and after that if no temperature data has been found looks at /sys/class/thermal/:

psutil/psutil/_pslinux.py

Lines 1247 to 1249 in ea4887e

# Indication that no sensors were detected in /sys/class/hwmon/
if not basenames:
basenames = glob.glob('/sys/class/thermal/thermal_zone*')

But because the external 1-Wire provides temp1_input the code never reaches the part where is read /sys/class/thermal/ including cpu temperature.

@giampaolo
Copy link
Owner

With covid and everything I don't have time. Please make a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants