Osx temps #1284
Osx temps #1284
Conversation
psutil/_psosx.py
Outdated
| label = '' | ||
| high = values['high'] | ||
| if high == int(0): | ||
| high = 105 |
There was a problem hiding this comment.
looks like this requires an explanation; why is 105 hard coded? why int(0)? Also, both current and high should be floats for consistency
There was a problem hiding this comment.
Since Apples SMC keys are poorly documented, I have not found a good way to always be able to retrieve high and critical, should these be left as None in this case?
psutil/_psosx.py
Outdated
| for key, value in cext.sensors_fans().items(): | ||
| try: | ||
| ret[key].append(_common.sfan(key, int(value))) | ||
| except (OSError, IOError) as err: |
There was a problem hiding this comment.
It doesn't look like this line can raise OSError or IOError
There was a problem hiding this comment.
Right, will remove
psutil/_psutil_osx.c
Outdated
| {"sensors_battery", psutil_sensors_battery, METH_VARARGS, | ||
| "Return battery information."}, | ||
| {"sensors_fans", psutil_sensors_fans, METH_VARARGS, | ||
| "Return fan spped information."}, |
psutil/_psutil_osx.c
Outdated
| Py_DECREF(py_cpu_temp); | ||
| Py_DECREF(py_cpu_temp_current); | ||
| Py_DECREF(py_cpu_temp_high); | ||
| return py_retdict; |
There was a problem hiding this comment.
I think it's easier if you use a list instead of a dict, and append values as you find them;
psutil/_psutil_osx.c
Outdated
| if (PyDict_SetItemString(py_cpu_temp, "high", py_cpu_temp_high)) { | ||
| goto error; | ||
| } | ||
| if (PyDict_SetItemString(py_retdict, "TC0F", py_cpu_temp)) { |
There was a problem hiding this comment.
TC0F is the best SMC key that I have found to correlate to the CPU die temperature (and is the key read in smc.c), but again, there is not official documentation for this. This can be change to CPU die or left as is. More SMC keys could be added in the future.
There was a problem hiding this comment.
What's CPU die temperature? Also, what if the system has more than one CPU?
There was a problem hiding this comment.
This https://app.assembla.com/spaces/fakesmc/wiki/Known_SMC_Keys is the best source I have found the documents this. There CPU die is marked as TC0D, but this does not exist on both systems I have tested. TC0F is best correlated to the temperature reported by the Intel power gadget
There was a problem hiding this comment.
It's a pity this is so poorly documented. :(
My concern is if we can't get per-CPU temp then I'm not sure what a single CPU temp value should represent on a multi CPU platform. The average of all CPUs temperatures?
There was a problem hiding this comment.
Applying same logic as per-core temperature, in multi-CPU systems there should be an equivalent TC\dF key per CPU, I'll see if I can make a loop over all that can be detected. But I do not have access to a multi-CPU system. (Also I am not sure that such a system even exists, and whether Apple themselves addressed this issue)
psutil/_psosx.py
Outdated
| if high and not critical: | ||
| critical = high | ||
| elif critical and not high: | ||
| high = critical |
There was a problem hiding this comment.
since you can't retrieve critical from C then you could just do critical = high
There was a problem hiding this comment.
Sure. Copied this from _pslinux.py
psutil/_psosx.py
Outdated
| @@ -212,6 +214,31 @@ def disk_partitions(all=False): | |||
| # ===================================================================== | |||
|
|
|||
|
|
|||
| def sensors_temperatures(): | |||
| """Return hardware (CPU and others) temperatures as a dict | |||
There was a problem hiding this comment.
doesn't this return temperatures about (1) CPU only?
There was a problem hiding this comment.
You are right, it could be possible to add additional sensors (via SMC reads) in the future
|
Output of |
* Fix typo (fan speed) * Add try, except in case on system fans are detected * Change function name to cpu_die_temperature to better reflect effect
|
Some notes on the latest commit,
I am unsure how to approach the multi-CPU configuration. To my best knowledge, there are no official Apple systems with 2 CPUs that have ever been produced (maybe some Hackintosh). In addition, I am not sure there are SMC keys for a second CPU. For example, Core 0 temperature is obtained by the key I think there should be a function in
If it is all the same to you, I would prefer to leave this as a dictionary, as this this not obtained from the same source as in Linux
Please leave your feedback on this, do you prefer I know some of the decisions here are not perfect, as there is no official documentation for SMC keys to my best knowledge. But perhaps some of the issues could be improved with the help of the community and cross referencing other systems. |
MacPro1,1-5,1 all supported 2 CPUs.
There are, as other monitoring apps can grab the temperatures for both sets of CPUs. I have both Xserves and PowerMac G5s that I'm willing to test with.
Even if there isn't (though given there are sensors, that can be inferred pretty easily), other methods exist. For example, There's still a bunch of sensors that are missing though. For example, on my MacBookPro14,3: I think rather than (outside of CPU temperatures) the best approach is to scan for sensors rather than use a series of hard coded values, in the same vein as iStats. I'll see about working on a fork of @amanusk's repo. |
|
See my repo for an outline of what I'm talking about. |
|
@giampaolo, how would you like to approach this? clearly there are many sensors, some documented and some not. Should the focus be on CPU sensors first? @JMY1000, in your list there are several keys mared as |
|
@amanusk I don't. The numbers were scavenged from other projects, primarily iStats, XRG, and this site. Only iStats lists those keys, but they also list the question marks. |
Yes, I think having at least CPU temperatures is OK and we can ignore other sensors for now (although it would be nice to have them as well). To my understanding the PR as it stands still doesn't take multiple CPU cores into account though, so we (I) are not sure what the returned value represents on multi core systems (my best guess is it represents the first CPU). @JMY1000 how does your branch differ from this PR? What extra info can you get? Are there blockers? |
|
@giampaolo, I understand your concern now. The current PR takes into account a system with a single CPU (e.g a single i7), which could be a multi-core. The sensor probably (based on comparison with other programs) gives the an average temperature of all the cores, or is measured in proximity to the CPU die. I thought you were concerned with systems that have more that one socket (i.e. 2x i7), then yes, the PR only addresses the CPU in socket 0. By the way, does psutil have or should it have a way to return the number of sockets? |
|
I think Lines 510 to 522 in d43ee30 Perhaps you can use that in a for loop and do "TC0F", ""TC1F" etc.?According to iStats source code it appears my assumption is correct:https://github.com/Chris911/iStats/blob/09b159f85a9481b59f347a37259f6d272f65cc05/lib/iStats/smc.rb#L56-L66 |
|
@giampaolo A massive amount of sensors, including (but not limited to) per-core temperature for multiple CPUs, GPUs, heatsinks, HDDs, memory, and PSUs. The basic idea is that my system is far more flexible and expandable than the current PR since it grabs the values from any number of sensors of your choosing. I'm not sure why you mean by blockers. If you mean things in the way of development, I've still got to figure out how to properly set up the testing suite and my dev environment to best work, but I've mostly been waiting to see if there was enough interest for me to keep going. |
|
@JMY1000 that is cool and is exactly what is needed. Can you paste the output of |
|
I have added per-core report, the result of I have used a for loop over the cores, as suggested.
What I meant is for a system like this 2-CPU-MB to report Anyway, if the implementation of @JMY1000 is better and more flexible, you should go with it.
@JMY1000 I am quite interested :) |
|
What does |
|
The sensors measures the overall CPU die temperature in a different location. It is equivalent to |
|
Anything else I can do/change/contribute? |
|
@giampaolo I'll need to do some more development work before I can do that, but since there's interest, I'm happy to continue. @amanusk If you're interested in helping me, I'm all for it. |
|
@giampaolo, @JMY1000, do you think it would be better if I remove |
|
@amanusk IMO I'd favor just working on my PR, but if we want to get what's working in quick, let's keep CPU die. |
|
Yes, there are others. They SMC keys are roughly divided by their purpose. For example, keys related to CPU are of the format The labels can be whatever it says on this site http://web.archive.org/web/20140714090133/http://www.parhelia.ch:80/blog/statics/k3_keys.html, (which is also where they list If what you want a mechanism to dynamically probe all the possible SMC keys that could be available on each machine, I am not sure how to build it, and you should wait for @JMY1000 to finish his PR. |
|
I have added some changes to the PR, And on a friends MacBook Pro: I have made some changes to make it easier to add more sensors (SMC keys) in the future.
|
There was a problem hiding this comment.
I have made some changes to make it easier to add more sensors (SMC keys) in the future.
Very good work. This looks promising. I added a bunch of inline comments. Couldn't some or all functions using SMCGetTemperature be grouped in a single one? It appears there is room for code reuse / refactoring.
Also, since you know the list of all possible key/value strings upfront ("TC0", etc.) why don't you just iterate over all of them and continue when SMCGetTemperature(key) fails?
Actually you may even consider maintaining the key/value mapping in pure python, and just expose SMCGetTemperature in C (which you will call as cext.SMCGetTemperature(key)). Such a C function should either return a string (temperature in Celsius) in case of success
or raise KeyError in case of failure. From Python you'll just catch TypeError and continue instead of populating the returning dict. Makes sense?
Regarding Travis: Some of the builds seem to fail.
Sorry about that. Just ignore them, they are unrelated.
PS: could you please use 4 spaces indentation in C code?
psutil/_psutil_osx.c
Outdated
|
|
||
| if (py_retlist == NULL) | ||
| return NULL; | ||
| smc_key* iter_key = smc_keys; |
psutil/_psosx.py
Outdated
| if rawlist is not None: | ||
| for batt in rawlist: | ||
| ret["Batteries"].append((batt[0], batt[1], None, None)) | ||
| except(SystemError): |
There was a problem hiding this comment.
Why? In general you want to let the underlying C extension raise exceptions (e.g. OSError). In this case if cext.battery_temperatures() fails because the SMC key does not exist then the C extension should simply return an empty list or dict
psutil/_psosx.py
Outdated
| if rawlist is not None: | ||
| for temp in rawlist: | ||
| ret["HDD"].append((temp[0], temp[1], None, None)) | ||
| except(SystemError): |
psutil/_psosx.py
Outdated
| try: | ||
| for key, value in cext.sensors_fans().items(): | ||
| ret["Fans"].append(_common.sfan(key, int(value))) | ||
| except (SystemError): |
| if (PyList_Append(py_retlist, py_tuple)) { | ||
| goto error; | ||
| } | ||
| Py_XDECREF(py_tuple); |
There was a problem hiding this comment.
Suggestion: these lines (from py_tuple to here) are recurring and can be put in a utility function like int _append_pair(PyObject *py_retlist, char key, char value)
psutil/_psutil_osx.c
Outdated
| if (py_retlist == NULL) | ||
| return NULL; | ||
| if (sysctlbyname("hw.packages", &num_cpus, &size, NULL, 0)) | ||
| Py_RETURN_NONE; |
There was a problem hiding this comment.
Missing parentheses. Also shouldn't you return py_retlist; instead?
|
Also, is there no way to get high/critical values on OSX? Do iStat or other similar tools provide such a value? |
Your guess is as good as mine. Here is an example of iStats on my system: As you can see, many keys are marked as This is also part of the reason I have split up the code into several functions, one for CPU/GPU/ etc. |
Squash fan speed functions into one Remove TC1F and TC0F SMC keys
|
Commited changes based on your latest review. |
|
Looks good. Merging. Thanks for all the hard work. |
|
For the record, if either @amanusk or @JMY1000 want to give it a try, adding a brand new |
|
@giampaolo macOS doesn't natively expose CPU voltages, even for things like the CPU. AFAIK the only way to get this info is by installing the Intel Power Gadget kext and reading from it, but having a closed-source kext as a dependency (even optional) just doesn't seem worth it to me. Battery and other smaller stuff might be possible, but that's a pretty limited and boring application IMO. |
|
Mmm... are you sure? I thought it was possible given the following keys: voltage_keys = {
"VC0C": "CPU Core 1",
"VC1C": "CPU Core 2",
"VC2C": "CPU Core 3",
"VC3C": "CPU Core 4",
"VC4C": "CPU Core 5",
"VC5C": "CPU Core 6",
"VC6C": "CPU Core 7",
"VC7C": "CPU Core 8",
"VV1R": "CPU VTT",
"VG0C": "GPU Core",
"VM0R": "Memory",
"VN1R": "PCH",
"VN0C": "MCH",
"VD0R": "Mainboard S0 Rail",
"VD5R": "Mainboard S5 Rail",
"VP0R": "12V Rail",
"Vp0C": "12V Vcc",
"VV2S": "Main 3V",
"VR3R": "Main 3.3V",
"VV1S": "Main 5V",
"VH05": "Main 5V",
"VV9S": "Main 12V",
"VD2R": "Main 12V",
"VV7S": "Auxiliary 3V",
"VV3S": "Standby 3V",
"VV8S": "Standby 5V",
"VeES": "PCIe 12V",
"VBAT": "Battery",
"Vb0R": "CMOS Battery",
} |
|
@giampaolo Let me take a look. It's possible I was confusing it with CPU speed, since that's one of the other things that's exposed through Power Gadget. |
|
@giampaolo I know this isn't strictly the place, but I'm having trouble linking the built C files to Python. I've been looking here and here, but I'm still not quite getting it. Are there project specific directions I should be following? |
|
You mean you're not able to compile psutil from sources? Did you install XCode? |
|
@giampaolo It compiles fine, but Python doesn't seem to be able to see the compiled files properly or something. It ends up erroring with |
|
I see. That is probably due to files renaming (from osx to macos) in https://github.com/giampaolo/psutil/pull/1298/files. On a second though that is probably too disruptive as a change. I reverted it in 1b09b5f. Please retry. |
|
@giampaolo I'm working in my own fork which hasn't had those changes merged in yet, so that's not the issue. |
|
Try “make clean” and “make uninstall”
On Fri, 29 Jun 2018 at 19:00, JMY1000 ***@***.***> wrote:
@giampaolo <https://github.com/giampaolo> I'm working in my own fork
which hasn't had those changes merged in yet, so that's not the issue.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1284 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAplLNJouce5Fw9Oo8rc0XJEO9xyCstzks5uBl1HgaJpZM4UO15u>
.
--
Giampaolo - http://grodola.blogspot.com
|
|
@giampaolo No luck. |
Wait a sec. This is exactly the issue. =) |
|
@giampaolo Why would that be an issue? I'm happy to merge in the latest branch if that fixes something, but there'd be a bunch of merge conflicts anyways that I'd rather resolve later, so unless there's a specific issue, I'd prefer not to. |
|
This PR got merged into master so if you want to provide a new one adding
functionality you should start from where master stands at this point.
On Sun, 1 Jul 2018 at 06:45, JMY1000 ***@***.***> wrote:
@giampaolo <https://github.com/giampaolo> Why would that be an issue? I'm
happy to merge in the latest branch if that fixes something, but there'd be
a bunch of merge conflicts anyways that I'd rather resolve later, so unless
there's a specific issue, I'd prefer not to.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1284 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAplLBk9GBZR5PerZ_9cg6sfx3hx1uqGks5uCFPegaJpZM4UO15u>
.
--
Giampaolo - http://grodola.blogspot.com
|
|
@giampaolo I was planning on merging after I completed my work just prior to my PR in case you merge any other conflicting changes into master in the mean time. I'm pretty sure the issue is unrelated to fact that I haven't merged in the most recent stuff. |
|
|
|
@giampaolo Gotcha, didn't realize they needed to be centrally registered. Thanks! |
|
@giampaolo @amanusk Sorry to tag both of you, but the blame is ambiguous as to who the author is. After several calls to It doesn't seem to consistently fail on a single value or at a singular time. I don't know what's causing this, but it doesn't immediately appear to be something in what I wrote. The memory address shifts around, but not every run. Any ideas what might be happening? |
|
We can’t argue what you may have did wrong in your branch: there is no PR
for it, no git diff to look/comment at and your code is not even in sync
with master so we can’t really discuss what you are doing. I suggest once
again to restart from an updated master and submit a PR (even a “test” PR
not meant not be merged) which can be discussed and commented. Also, since
it seems you aim at doing everything in C (and I agree with that) before
working on voltages it may make sense to first refactor code and submit a
PR which declares the temperature SMC keys in C (instead of python) so that SMCOpen()
is called only once.
On Tue, 3 Jul 2018 at 01:37, JMY1000 ***@***.***> wrote:
@giampaolo <https://github.com/giampaolo> @amanusk
<https://github.com/amanusk> Sorry to tag both of you, but the blame
<https://github.com/giampaolo/psutil/blame/master/psutil/arch/osx/smc.c>
is ambiguous as to who the author is. After several calls to
SMCGetTemperature(), SMCOpen() (called inside of SMCGetTemperature())
errors out with the following error:
test_sensors(19918,0x7fff9d644340) malloc: *** error for object 0x7fbd53c00348: incorrect checksum for freed object - object was probably modified after being freed.
*** set a breakpoint in malloc_error_break to debug
It doesn't seem to consistently fail on a single value or at a singular
time. I don't know what's causing this, but it doesn't immediately appear
to be something in what I wrote. The memory address shifts around, but not
every run. Any ideas what might be happening?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1284 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAplLItmsbxli44dwyx7_hXBh1svlSunks5uCq66gaJpZM4UO15u>
.
--
Giampaolo - http://grodola.blogspot.com
|
It turns out code contributed in PR #1284 was using parts of a source code released by Apple [1] which uses GPL license which is incompatible with psutil's license (BSD). [1] https://gist.github.com/edvakf/4049362
* setup.py: add py 3.7 * Revert OSX sensors code due to copyright constraints It turns out code contributed in PR #1284 was using parts of a source code released by Apple [1] which uses GPL license which is incompatible with psutil's license (BSD). [1] https://gist.github.com/edvakf/4049362
* 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) ...
This addresses the discussion #371,
This is mainly based on the work of @wiggin15, I have mainly changed it to work similarly to the current API in Linux.
I had a chance to test this on 2 machines.
I have also tested how this will integrate with my own project s-tui-#49
Please leave feedback so I can do the work to integrate this while I have access to hardware.