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

Osx temps #1284

Merged
merged 12 commits into from
Jun 26, 2018
Merged

Osx temps #1284

merged 12 commits into from
Jun 26, 2018

Conversation

amanusk
Copy link
Collaborator

@amanusk amanusk commented May 26, 2018

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.

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.

This is great stuff. Thank. Some comments inline but overall it looks good. Can you just paste the output of python scripts/sensors.py? I'd like to see how this looks like.

psutil/_psosx.py Outdated
label = ''
high = values['high']
if high == int(0):
high = 105
Copy link
Owner

Choose a reason for hiding this comment

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

looks like this requires an explanation; why is 105 hard coded? why int(0)? Also, both current and high should be floats for consistency

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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:
Copy link
Owner

Choose a reason for hiding this comment

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

It doesn't look like this line can raise OSError or IOError

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, will remove

{"sensors_battery", psutil_sensors_battery, METH_VARARGS,
"Return battery information."},
{"sensors_fans", psutil_sensors_fans, METH_VARARGS,
"Return fan spped information."},
Copy link
Owner

Choose a reason for hiding this comment

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

typo

Py_DECREF(py_cpu_temp);
Py_DECREF(py_cpu_temp_current);
Py_DECREF(py_cpu_temp_high);
return py_retdict;
Copy link
Owner

Choose a reason for hiding this comment

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

I think it's easier if you use a list instead of a dict, and append values as you find them;

if (PyDict_SetItemString(py_cpu_temp, "high", py_cpu_temp_high)) {
goto error;
}
if (PyDict_SetItemString(py_retdict, "TC0F", py_cpu_temp)) {
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 "TC0F"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

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 CPU die temperature? Also, what if the system has more than one CPU?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

@amanusk amanusk May 26, 2018

Choose a reason for hiding this comment

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

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
Copy link
Owner

Choose a reason for hiding this comment

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

since you can't retrieve critical from C then you could just do critical = high

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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
Copy link
Owner

Choose a reason for hiding this comment

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

doesn't this return temperatures about (1) CPU only?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right, it could be possible to add additional sensors (via SMC reads) in the future

@amanusk
Copy link
Collaborator Author

amanusk commented May 26, 2018

Output of scripts/sensors.py

Fan 0
    Fans:
        Fan 0                2162 RPM
Fan 1
    Fans:
        Fan 1                2001 RPM
TC0F
    Temperatures:
        TC0F                 42.34°C (high=105°C, critical=105°C)
Battery:
    charge:     66.0%
    left:       8:23:00
    status:     discharging
    plugged in: no

* 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
@amanusk
Copy link
Collaborator Author

amanusk commented May 26, 2018

Some notes on the latest commit,
Regarding our discussion,

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?

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 TC0C, and Core 1 is TC1C (according to documentation and other sources). I can not guess how would this be interpolated for 2 CPUs. Also, there is no clear indication or SMC key for number of CPUs.
(BTW, per-core results can be added quite easily to sensors_temperatures in the future).

I think there should be a function in _psutil_osx.c per sensors, so that an error when trying to obtain some value will not lead to no results at all on systems where some SMC key is not supported.

I think it's easier if you use a list instead of a dict, and append values as you find them;

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

looks like this requires an explanation; why is 105 hard coded?

Please leave your feedback on this, do you prefer critical and high be left as None if no sensor is available?

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.

@JMY1000
Copy link

JMY1000 commented May 26, 2018

To my best knowledge, there are no official Apple systems with 2 CPUs that have ever been produced

MacPro1,1-5,1 all supported 2 CPUs.
All Xserve models supported 2 CPUs.
PowerMac3,6-11,2 supported 2 CPUs.

In addition, I am not sure there are SMC keys for a second CPU.

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.

Also, there is no clear indication or SMC key for number of CPUs.

Even if there isn't (though given there are sensors, that can be inferred pretty easily), other methods exist. For example, system_profiler SPHardwareDataType | grep 'Number of Processors:' | sed 's/^.*: //'.

There's still a bunch of sensors that are missing though. For example, on my MacBookPro14,3:

Th1H => NB/CPU/GPU HeatPipe 1 Proximity
Ts0S => Memory Bank Proximity
TG0D => GPU 0 Die
TG0P => GPU 0 Proximity
TM0P => Memory Slot Proximity
TPCD => Platform Controller Hub Die
TW0P => AirPort Proximity

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.

@JMY1000
Copy link

JMY1000 commented May 27, 2018

See my repo for an outline of what I'm talking about.

@amanusk
Copy link
Collaborator Author

amanusk commented Jun 2, 2018

@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 ??. From my comparison with Intel power gadget, the key TC0F had the best correlation with the temperature reported by the tool. On the other hand TC0D was not present at all on my system. Do you know what ?? are?

@JMY1000
Copy link

JMY1000 commented Jun 2, 2018

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

@giampaolo
Copy link
Owner

giampaolo commented Jun 14, 2018

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

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?

@amanusk
Copy link
Collaborator Author

amanusk commented Jun 14, 2018

@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.
There should be other sensors for per-core temperature.

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?
To my knowledge count_cpus return the number of physical or logical cores, but does not give information on number of sockets.
For example /proc/cpuinfo reports processor : 0 and processor : 1 for systems where 2 CPUs are installed

@giampaolo
Copy link
Owner

giampaolo commented Jun 14, 2018

I think psutil.cpu_count(logical=False) should give what you want. On OSX that is calculated via sysctlbyname("hw.physicalcpu"), which you can already use:

psutil/psutil/_psutil_osx.c

Lines 510 to 522 in d43ee30

/*
* Return the number of physical CPUs in the system.
*/
static PyObject *
psutil_cpu_count_phys(PyObject *self, PyObject *args) {
int num;
size_t size = sizeof(int);
if (sysctlbyname("hw.physicalcpu", &num, &size, NULL, 0))
Py_RETURN_NONE; // mimic os.cpu_count()
else
return Py_BuildValue("i", num);
}

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

@JMY1000
Copy link

JMY1000 commented Jun 14, 2018

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

@giampaolo
Copy link
Owner

@JMY1000 that is cool and is exactly what is needed. Can you paste the output of python scripts/sensors.py to show what info you are able to retrieve?

@amanusk
Copy link
Collaborator Author

amanusk commented Jun 14, 2018

I have added per-core report, the result of scripts/sensors.py is now (dual core system):

Cores                          
    Temperatures:              
        Core0                39.0°C (high=None°C, critical=None°C)                                                             
        Core1                37.0°C (high=None°C, critical=None°C)                                                             
CPUdie                         
    Temperatures:              
        CPUdie               41.55°C (high=93.0°C, critical=93.0°C)                                                            
Fan0                           
    Fans:                      
        Fan0                 0 RPM 

I have used a for loop over the cores, as suggested.

I think psutil.cpu_count(logical=False) should give what you want. On OSX that is calculated via sysctlbyname("hw.physicalcpu"), which you can already use:

What I meant is for a system like this 2-CPU-MB to report 2 CPUs, while psutil.cpu_count(logical=False) reports cores. On a system like this there could be 24 cores, 12 cores per CPU, and 48 thread (logical=True)

Anyway, if the implementation of @JMY1000 is better and more flexible, you should go with it.

if there was enough interest for me to keep going

@JMY1000 I am quite interested :)

@giampaolo
Copy link
Owner

What does CPUdie represent? If we're able to retrieve per-core temps I suppose that is useless.

@amanusk
Copy link
Collaborator Author

amanusk commented Jun 14, 2018

The sensors measures the overall CPU die temperature in a different location.
Similar to how on Linux I get

coretemp
    Physical id 0        58.0 °C (high = 87.0 °C, critical = 105.0 °C)
    Core 0               57.0 °C (high = 87.0 °C, critical = 105.0 °C)
    Core 1               55.0 °C (high = 87.0 °C, critical = 105.0 °C)

It is equivalent to Physical id 0

@amanusk
Copy link
Collaborator Author

amanusk commented Jun 15, 2018

Anything else I can do/change/contribute?

@JMY1000
Copy link

JMY1000 commented Jun 15, 2018

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

@amanusk
Copy link
Collaborator Author

amanusk commented Jun 16, 2018

@giampaolo, @JMY1000, do you think it would be better if I remove CPUdie for now, and leave per-core temperature only, as well as the fan speeds, and merge @JMY1000's PR when it is ready?

@JMY1000
Copy link

JMY1000 commented Jun 16, 2018

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

@giampaolo
Copy link
Owner

@amanusk perhaps you can integrate @JMY1000 changes in this PR? Is your OSX able to provide other temperatures/sensors other than CPU and fans?

@amanusk
Copy link
Collaborator Author

amanusk commented Jun 20, 2018

Yes, there are others.
It seems not all systems have the same SMC keys available though.

They SMC keys are roughly divided by their purpose. For example, keys related to CPU are of the format TC<X><X> . SMC keys for battery are TB<X><X> etc.
I can add functions that probe the different sensors according to their region, so that sensors.temperatures() returns a dictionary were the key is (for example) CPU and the values is a list of all SMC keys that were successfully probed.
Example (of what it can look like):

CPU                          
    Temperatures:              
        Core0                39.0°C (high=None°C, critical=None°C)                                                             
        Core1                37.0°C (high=None°C, critical=None°C)                                                         
        CPUdie               41.55°C (high=93.0°C, critical=93.0°C)   
        CPUproxibity      41.55°C (high=93.0°C, critical=93.0°C)   
....
HDD
       Temperatures:           
                 HDDProximity (39.0°C (high=None°C, critical=None°C))

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 CPU Die that you have asked about). All the SMC keys will be hard coded in smc.h

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.

@amanusk
Copy link
Collaborator Author

amanusk commented Jun 20, 2018

I have added some changes to the PR,
The result of ./scripts/sensors.py on my MacMini:

Fans                  
    Fans:             
        Fan0                 1800 RPM        
HDD                   
    Temperatures:     
        HDD 1 unkonwn        35.0°C (high=None°C, critical=None°C)                        
CPU                   
    Temperatures:     
        Core0                38.0°C (high=None°C, critical=None°C)                        
        Core1                38.0°C (high=None°C, critical=None°C)                        
        CPU 0 Proximity      38.875°C (high=None°C, critical=None°C)                      
        CPU TC0E             40.55078125°C (high=None°C, critical=None°C)                 
        CPU TC0F             40.91796875°C (high=None°C, critical=None°C)     

And on a friends MacBook Pro:

Fans
    Fans:
        Fan1                 2009 RPM
        Fan0                 2158 RPM
Batteries
    Temperatures:
        TB0T                 31.19921875°C (high=None°C, critical=None°C)
        TB1T                 31.19921875°C (high=None°C, critical=None°C)
        TB2T                 30.296875°C (high=None°C, critical=None°C)
CPU
    Temperatures:
        Core1                53.0°C (high=None°C, critical=None°C)
        Core2                52.0°C (high=None°C, critical=None°C)
        Core3                53.0°C (high=None°C, critical=None°C)
        Core4                53.0°C (high=None°C, critical=None°C)
        CPU 0 Proximity      47.5°C (high=None°C, critical=None°C)
        CPU TC0E             56.921875°C (high=None°C, critical=None°C)
        CPU TC0F             58.94921875°C (high=None°C, critical=None°C)
Battery:
    charge:     82.0%
    left:       5:25:00
    status:     discharging
    plugged in: no

I have made some changes to make it easier to add more sensors (SMC keys) in the future.
Some justifications:

  • I have taken some labels from iStats and some from the mentioned website

  • I tried to determine the exact number of CPUs/GPUs/Fans where I could, where I couldn't I assumed the highest number mentioned in iStats (for example TB2T for battery, etc.)

  • I do not know what TC0F REALLY stands for, what I do know, is that it is the closest measurement to Intel power gadget on both systems.

  • I only had 2 systems to test this on. Anyone with other, different systems, is welcome to suggest changes.

  • Regarding Travis: Some of the builds seem to fail, regardless of my changes (e.g, some times python 2.6 fails, and sometimes they all pass). Is this normal?

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.

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?


if (py_retlist == NULL)
return NULL;
smc_key* iter_key = smc_keys;
Copy link
Owner

Choose a reason for hiding this comment

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

missing parentheses?

psutil/_psosx.py Outdated
if rawlist is not None:
for batt in rawlist:
ret["Batteries"].append((batt[0], batt[1], None, None))
except(SystemError):
Copy link
Owner

Choose a reason for hiding this comment

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

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):
Copy link
Owner

Choose a reason for hiding this comment

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

same here

psutil/_psosx.py Outdated
try:
for key, value in cext.sensors_fans().items():
ret["Fans"].append(_common.sfan(key, int(value)))
except (SystemError):
Copy link
Owner

Choose a reason for hiding this comment

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

same here

if (PyList_Append(py_retlist, py_tuple)) {
goto error;
}
Py_XDECREF(py_tuple);
Copy link
Owner

Choose a reason for hiding this comment

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

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)

if (py_retlist == NULL)
return NULL;
if (sysctlbyname("hw.packages", &num_cpus, &size, NULL, 0))
Py_RETURN_NONE;
Copy link
Owner

Choose a reason for hiding this comment

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

Missing parentheses. Also shouldn't you return py_retlist; instead?

@giampaolo
Copy link
Owner

Also, is there no way to get high/critical values on OSX? Do iStat or other similar tools provide such a value?

@amanusk
Copy link
Collaborator Author

amanusk commented Jun 21, 2018

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:

--- CPU Stats ---             
CPU temp:               38.13°C                              

--- Fan Stats ---             
Total fans in system:   1                                    
Fan 0 speed:            1800 RPM                             

--- Battery Stats ---         
No battery on system          

--- Extra Stats ---           
Tm0p Misc (clock chip) Proximity temp:35.88°C                
Tm0P Unknown temp:      35.88°C                              
Tp0C Unknown temp:      37.19°C                              
Ts0G Unknown temp:      49.0°C                               
TA0p Ambient temperature temp:37.25°C                        
TA0P Ambient temperature temp:37.25°C                        
TA0V Unknown temp:      22.92°C                              
TA1p Ambient temperature temp:37.13°C                        
TA1P Ambient temperature temp:37.13°C                        
TA2p Unknown temp:      40.25°C                              
TA2P Unknown temp:      40.25°C                              
TCGc PECI GPU temp:     41.0°C                               
TCGC PECI GPU temp:     41.0°C                               
TCPG  temp:             98.0°C                               
TCSc PECI SA temp:      37.0°C                               
TCSC PECI SA temp:      37.0°C                               
TCTD Unknown temp:      0.0°C                                
TCXc PECI CPU temp:     40.02°C                              
TCXr Unknown temp:      -59.98°C                             
TCXC PECI CPU temp:     40.02°C                              
TC0c  temp:             38.0°C                               
TC0p  temp:             38.13°C                              
TC0C CPU 0 Core temp:   38.0°C                               
TC0E CPU 0 ?? temp:     40.08°C                              
TC0F CPU 0 ?? temp:     39.66°C                              
TC0G CPU 0 ?? temp:     93.0°C                               
TC0J CPU 0 ?? temp:     -0.44°C                              
TC0P CPU 0 Proximity temp:38.13°C                            
TC1c  temp:             36.0°C                               
TC1C Core 1 temp:       36.0°C                               
TH0a Unknown temp:      -127.0°C                             
TH0b Unknown temp:      -127.0°C                             
TH0c Unknown temp:      -127.0°C                             
TH0A Unknown temp:      -127.0°C                             
TH0B Unknown temp:      -127.0°C                             
TH0C Unknown temp:      -127.0°C                             
TH0F Unknown temp:      -127.0°C                             
TH0O Unknown temp:      -7.0°C                               
TH0P Harddisk 0 Proximity temp:-7.0°C                        
TH0R Unknown temp:      -127.0°C                             
TH1a Unknown temp:      35.0°C                               
TH1b Unknown temp:      35.0°C                               
TH1c Unknown temp:      35.0°C                               
TH1A Unknown temp:      -127.0°C                             
TH1B Unknown temp:      -127.0°C                             
TH1C Unknown temp:      -127.0°C                             
TH1F Unknown temp:      -127.0°C                             
TH1O Unknown temp:      9.0°C                                
TH1P Unknown temp:      -127.0°C                             
TH1R Unknown temp:      -127.0°C                             
TM0p Unknown temp:      36.75°C                              
TM0P Memory Slot Proximity temp:36.75°C                      
TM0V Unknown temp:      37.3°C                               
TPCd Unknown temp:      42.0°C                               
TPCD Platform Controller Hub Die temp:42.0°C                 
TS0V Unknown temp:      33.47°C                              
TW0p Unknown temp:      36.88°C                              
TW0P AirPort Proximity temp:36.88°C         

As you can see, many keys are marked as Unknown or ??.
Some show obliviously wrong temperatures (such as -127)
There is TC0G CPU 0 ?? temp: 93.0°C which on my system shows a constant temperature of 93, and could be either high or critical, but it could also be completely unrelated, I can only guess.

This is also part of the reason I have split up the code into several functions, one for CPU/GPU/ etc.
It is possible that in the future someone will confirm that this is indeed high, and it will require slightly different handling of some particular keys, and not simply bunching them together in a long list and going over all of them.

Squash fan speed functions into one
Remove TC1F and TC0F SMC keys
@amanusk
Copy link
Collaborator Author

amanusk commented Jun 26, 2018

Commited changes based on your latest review.
Please check if I haven't missed anything

@giampaolo
Copy link
Owner

Looks good. Merging. Thanks for all the hard work.

@giampaolo giampaolo merged commit 196802d into giampaolo:master Jun 26, 2018
@giampaolo
Copy link
Owner

For the record, if either @amanusk or @JMY1000 want to give it a try, adding a brand new psutil.sensors_voltages() function should be relatively easy by using these keys:
#1284 (comment)
The same should be done on Linux (I can probably do that).

@JMY1000
Copy link

JMY1000 commented Jun 27, 2018

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

@giampaolo
Copy link
Owner

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",
}

@JMY1000
Copy link

JMY1000 commented Jun 27, 2018

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

@JMY1000
Copy link

JMY1000 commented Jun 28, 2018

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

@giampaolo
Copy link
Owner

You mean you're not able to compile psutil from sources? Did you install XCode?

@JMY1000
Copy link

JMY1000 commented Jun 29, 2018

@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 AttributeError: module 'psutil_osx' has no attribute 'sensor_temperatures' on line 221 of _psosx.py. I've got Xcode installed, but I'm using Clion and PyCharm as my IDEs.

@giampaolo
Copy link
Owner

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.

@JMY1000
Copy link

JMY1000 commented Jun 29, 2018

@giampaolo I'm working in my own fork which hasn't had those changes merged in yet, so that's not the issue.

@giampaolo
Copy link
Owner

giampaolo commented Jun 29, 2018 via email

@JMY1000
Copy link

JMY1000 commented Jun 29, 2018

@giampaolo No luck.

@giampaolo
Copy link
Owner

I'm working in my own fork which hasn't had those changes merged in yet, so that's not the issue.

Wait a sec. This is exactly the issue. =)
You should merge latest psutil master in your branch.

@JMY1000
Copy link

JMY1000 commented Jul 1, 2018

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

@giampaolo
Copy link
Owner

giampaolo commented Jul 1, 2018 via email

@JMY1000
Copy link

JMY1000 commented Jul 1, 2018

@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
Copy link
Owner

AttributeError: module 'psutil_osx' has no attribute 'sensor_temperatures' mans the C extension (_psutil_osx.c) does not expose a "sensors_temperatures" function. E.g. this is how current (updated master) C extension exposes a "smc_get_temperature" function:
https://github.com/giampaolo/psutil/blob/master/psutil/_psutil_osx.c#L2041
Your branch should do something similar.
If you want to update your fork with latest changes (which I recommend) here's how you do it:
https://help.github.com/articles/syncing-a-fork/

@JMY1000
Copy link

JMY1000 commented Jul 1, 2018

@giampaolo Gotcha, didn't realize they needed to be centrally registered. Thanks!

@JMY1000
Copy link

JMY1000 commented Jul 2, 2018

@giampaolo @amanusk Sorry to tag both of you, but the blame 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?

@giampaolo
Copy link
Owner

giampaolo commented Jul 3, 2018 via email

giampaolo added a commit that referenced this pull request Aug 13, 2018
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
giampaolo added a commit that referenced this pull request Aug 13, 2018
* 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
giampaolo added a commit that referenced this pull request Oct 1, 2018
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)
  ...
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.

None yet

3 participants