Add support for Apple M2

[enen92]

Hey @narugit thanks for resurrecting the old smctemp project.
This PR adds support for Apple M2 (my laptop) (8 cores - 4 performance + 4 efficiency). Also adds support for other models based on the sensors app open-source code.
Closes #11

Also adds an option to poll for the temperature values for n times until a given value is returned (special case for some apple m2 models)

Mac Model:

TG Pro details:

smctemp:

[narugit]

@joshuataylor
I don't have M2, so could you kindly check this feature branch in your M2 environment?
If it works, we can close #11 .

[narugit]

@enen92
Could you kindly explain how you obtained the sensor information in M2?
For example, how do you know Tp0n indicates CPU performance core 4 temperature？

[joshuataylor]

Alright so getCPUModel() returns "Apple M2 Max" for me, willing to bet Ultra and other marketing names/product names such as Ultra etc will be "Apple M2 Ultra".

Also thinking the same for M1? But the else should catch it. Maybe in future some sort of case statement to check the first 8 characters or so?

Maybe it's best to check that it starts with "Apple M2"?

Not sure of how to do this properly in C++, but changing it to the following fixed it sometimes for me:

  // Apple M2 CPU Models begin with "Apple M2", then have the product name such as Max/Ultra after it
  if (getCPUModel().substr(0, 8) == "Apple M2") {

It'll return like "36.1" sometimes, then 4.2.. is it picking a value at random?

[enen92]

Alright so getCPUModel() returns "Apple M2 Max" for me, willing to bet Ultra and other marketing names/product names such as Ultra etc will be "Apple M2 Ultra".

Also thinking the same for M1? But the else should catch it. Maybe in future some sort of case statement to check the first 8 characters or so?

Maybe it's best to check that it starts with "Apple M2"?

Not sure of how to do this properly in C++, but changing it to the following fixed it sometimes for me:

  // Apple M2 CPU Models begin with "Apple M2", then have the product name such as Max/Ultra after it
  if (getCPUModel().substr(0, 8) == "Apple M2") {

It'll return like "36.1" sometimes, then 4.2.. is it picking a value at random?

I reworked it a bit on the last commit, it should now be detected. Note that those models have a lot more cpu cores (8 performance cores + 4 efficiency cores) so there are some sensors that are not picked up in the calculation.
I also added a commented block to add them later (I have no way to reverse engineer this/no hardware).
The average calculation should fix the "double" problem but a lot of sensors are missing for those models.

[enen92]

@enen92 Could you kindly explain how you obtained the sensor information in M2? For example, how do you know Tp0n indicates CPU performance core 4 temperature？

I got this from the stats app: https://github.com/exelban/stats/blob/135eacec905a8d1d43259660f1c4a85a1838d376/Modules/Sensors/values.swift#L253-L262

Also checked against TG pro that the values are pretty similar so I guess the match is right

[joshuataylor]

Awesome thanks, I'll check it and and double check the sensor values. I'll share here once I know more.

[joshuataylor]

exelban/stats#1286 (comment)

Also from my observations it looks really all over the place. TG Pro must have figured out which sensors to use... 😕

[enen92]

exelban/stats#1286 (comment)

Also from my observations it looks really all over the place. TG Pro must have figured out which sensors to use... 😕

Anyway, thanks for the link. I removed those definitions from the comments to avoid confusion (I also don't know if they are an exact match). As this tool only computes the average temperature I'd say it's fine.

[narugit]

@enen92
Thank you for quick fix!
I've verified working well on Intel chip and M1 chip.

[narugit]

@joshuataylor
Thanks for trying!
Now that the feature branch is fixed, could you check if it works again on your Apple M2 Max?

[joshuataylor]

Works randomly, but looks like these bytes randomly return the wrong value. Maybe merge this, then figure out the correct sensors for the Max?

  Tp05  [flt ]  4.6 (bytes: 89 41 94 40)
  Tp09  [flt ]  6.7 (bytes: 66 66 D6 40)
  Tp0D  [flt ]  4.6 (bytes: 89 41 94 40)
  Tp0f  [flt ]  6.7 (bytes: 66 66 D6 40)
  Tp0j  [flt ]  4.6 (bytes: 89 41 94 40)

[joshuataylor]

[stats]https://github.com/exelban/stats) has the same issue, they disable the Max by default it seems.

TG Pro:

[enen92]

Yeah in sensors (last version) they seem to discard the values if lower than 10:

exelban/stats@14e29c4#diff-ce3fe5eb966ca7c489e6b4db8d0d3a2441f0f8425779776f243611f9630cdafcR124

However the use-case for this app is different than smctemp. Sensors store state and display it to the user.
Smctemp pools the registers if requested by the user. If using this from the terminal (using watch or similar) maybe you should consider storing the last value or only display it if higher than the threshold.

Or maybe we should consider a threshold on smctemp and only consider the values on those CPU sensors valid if above that limit. Worst case scenario it will return 0 and you can take appropriate measures on the client side

[enen92]

Please wait a bit while I rework this. Maybe we can add some option to poll for values until one is valid.

[narugit]

@enen92
Your idea of providing the option for polling is great, and I appreciate it.

I'd like to suggest using the option -n instead of -c.
I am considering phasing out the -c option in the future. When running the smctemp command without any options, I plan to show Celsius temperature values in default.

[enen92]

@enen92 Your idea of providing the option for polling is great, and I appreciate it.

I'd like to suggest using the option -n instead of -c. I am considering phasing out the -c option in the future. When running the smctemp command without any options, I plan to show Celsius temperature values in default.

What if in the future the tool supports more classes than just the CPU temperature? For example, the GPU? (which I have in the queue)

[narugit]

How about something like this? (t is type of t)

• -t [cpu,gpu,...]: Show temperature of specified one (Default is CPU temperature)

[narugit]

Ah,,

I've just realized now that you're suggesting whether it's possible to allow setting the polling frequency independently for each sensor when multiple sensors are specified.

[narugit]

@enen92
Even in the future, when the capability to specify multiple sensors becomes available, I believe there's no need to allow specifying the polling frequency for each sensor individually. It should suffice to have just one option for the polling frequency.

Since the polling frequency represents the maximum number of attempts, as long as we can retrieve a valid value within that specified number of attempts, there shouldn't be any issues.

For instance, if one wishes to configure the CPU temperature to poll 5 times and the GPU temperature 2 times, simply setting -n5 should suffice. This assumes that the GPU can likely obtain the value within 2 attempts. I believe this approach can meet this user's requirements.

I would appreciate hearing your opinion.

[enen92]

Hum, I think I misunderstood the request first (thought you meant to use ./smctemp -n5 to get the cpu temp with 5 attempts). In your last comment I think you actually meant ./smctemp -c -n5. If that is the case, I agree - we can still define an operation (CPU) but an additional option for attempts (while not limiting future extensions).
If so, I'll try to have a crack at it tomorrow, already too late here :)

[narugit]

@enen92
Actually, -c doesn't refer to CPU; it stands for Celsius temperature. I apologize for any confusion caused by having both "C"PU and "C"elsius in the same sentence.

smctemp/README.md

Line 23 in da1fe79

-c : list CPU temperatures (Celsius)

And as you've correctly stated, the expectation behavior after the merging of this PR is to use the command smctemp -c -n5 to get the cpu temp (in Celsius) with 5 attempts.

[enen92]

@enen92 Actually, -c doesn't refer to CPU; it stands for Celsius temperature. I apologize for any confusion caused by having both "C"PU and "C"elsius in the same sentence.

smctemp/README.md

Line 23 in da1fe79

-c : list CPU temperatures (Celsius)

And as you've correctly stated, the expectation behavior after the merging of this PR is to use the command smctemp -c -n5 to get the cpu temp (in Celsius) with 5 attempts.

Should be done in the last push.
Actually -c in the code sets the operation to read the CPU values so I thought it was dedicated for the CPU. If introducing support for other units maybe it's not a bad idea to introduce (-C uppercase or -F) in the future.

[narugit]

@enen92

If introducing support for other units maybe it's not a bad idea to introduce (-C uppercase or -F) in the future.

Thank you for the great idea. Sounds great!

[narugit]

Thank you for your quick modification!
I've checked the behavior on my Intel and M1 mac and it works well:)
It is the same behavior as before on both Intel and M1 platforms with -c, while also allowing the use of the -n option.

[narugit]

@joshuataylor
Could you please check the behavior is as you expected on your M2 Max?

[enen92]

Ping @joshuataylor

[joshuataylor]

Apologies for the delay, I missed the @ and the follow up here! 🤦
Everything seems to be good on my end! 🎉

[narugit]

@joshuataylor
Thank you for confirming!

@enen92
Thank you for a great PR 🎉