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

feat: add interval option #34

Merged
merged 2 commits into from
Aug 19, 2024
Merged

feat: add interval option #34

merged 2 commits into from
Aug 19, 2024

Conversation

narugit
Copy link
Owner

@narugit narugit commented Aug 14, 2024

About

  • Added an interval option (-i) to configure the millisecond delay between sensor value readings.
  • Updated documentation for M2 Mac users, recommending the use of -i and -n options to obtain more stable and accurate temperature readings.

@narugit narugit force-pushed the feature/add_interval_option branch from 4d2fdc5 to 10bd8e1 Compare August 14, 2024 04:52
@narugit
Copy link
Owner Author

narugit commented Aug 14, 2024

Hi @kyleyannelli
Could you try this branch in your M2 Mac?

@kyleyannelli
Copy link
Contributor

kyleyannelli commented Aug 15, 2024

@narugit I like the -i option. -i25 -n1000 works great on my machine most of the time. However, this morning I quickly tried the branch when there was nothing but my terminal open. I found that less load on the CPU make a quicker interval work better. I was getting consistent results with -i20 rather than -i25. After launching Firefox & playing a video in the background, -i25 was now more consistent. I didn't have time to test whether or not it was a coincidence. Regardless, maybe the interval lower limit should be lowered to 10 or 15 ms? I imagine this will vary from machine to machine and environment to environment. Either way we are trying to tame a seemingly unpredictable sensor. Perhaps it's fine as it is for now...

Edit: If you'd like I will have more time over the weekend to make a test that can just try every combination of interval by steps of 10 and every retry by steps of 100 to see which combo yields the best results. I'm quite confident with my current values, but I can't help but be a bit curious about the possibility of a better combo.

@narugit
Copy link
Owner Author

narugit commented Aug 15, 2024

@kyleyannelli
Thanks for trying out different settings.
It seems like CPU load does have an impact. As you mentioned, it might be a good idea to lower the interval limit to around 10ms.

As a separate idea from the interval option, how about adding an option (-f, --fail-soft) that saves the most recent valid sensor value in /tmp/?
If the smctemp command fails to get a valid sensor reading at some point, it can refer to the last valid sensor value stored in /tmp/.

@kyleyannelli
Copy link
Contributor

That sounds good to me!

I'm currently running smctemp in a loop through a system service. Saving the last non zero value in a text file so I can just run cat /tmp/smctemp.val.txt to display in my tmux bar.

README.md Outdated
For M2 Macs, using the `-n`, `-i`, and `-f` options can help obtain more stable sensor values.
Try tuning these options to get better results.

The recommended settings are `-i25 -n40 -f` (See also https://github.com/narugit/smctemp/issues/32#issuecomment-2287304793).
Copy link
Owner Author

Choose a reason for hiding this comment

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

Hi @kyleyannelli
I'd like to add recommended settings for M2 user, so could you provide your recommended settings with -f?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @narugit
After messing around a bit I'm going to recommend -i25 -n180 -f. The lower retries just don't hit even when the CPU is under load. I think 25ms and 180 maximum retries work well enough to get a result while the max wait time is under 5 seconds.

Copy link
Owner Author

Choose a reason for hiding this comment

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

@kyleyannelli
Thank you for checking! I've updated the recommended settings.

@narugit narugit force-pushed the feature/add_interval_option branch from 17cb9ae to 38a671c Compare August 19, 2024 00:20
@narugit narugit merged commit 62ace53 into main Aug 19, 2024
2 checks passed
@narugit narugit deleted the feature/add_interval_option branch August 19, 2024 00:22
@narugit narugit mentioned this pull request Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants