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

Improve monitorField target value calculation #287

Merged
merged 1 commit into from
Mar 19, 2024

Conversation

xak2000
Copy link
Contributor

@xak2000 xak2000 commented Mar 17, 2024

The monitorField target value calculation now uses heuristic method to convert the target value to the appropriate unit of measurement, that RyzenAdj uses to return value in.

E.g. fast_limit will be divided by 1000, as RyzenAdj sets the value in milliwatts (e.g. 25000), while returns the value in watts (e.g. 25). But tctl_temp will not be divided by 1000, as RyzenAdj sets the value in degrees Celsius (e.g. 85) and returns the value also in degrees Celsius (e.g. 85).

Fixes #286

@Falcosc I'm not sure why did you select exactly value 2000 for this heuristic detection, so feel free to improve the commit message with the explanation.

The `monitorField` target value calculation now uses heuristic method to convert the target value to the appropriate unit of
measurement, that RyzenAdj uses to return value in.

E.g. `fast_limit` will be divided by 1000, as RyzenAdj sets the value in milliwatts (e.g. 25000), while returns the value in watts (e.g. 25). But `tctl_temp` will not be divided by 1000, as RyzenAdj sets the value in degrees Celsius (e.g. 85) and returns the value also in degrees Celsius (e.g. 85).

Fixes FlyGoat#286
@FlyGoat
Copy link
Owner

FlyGoat commented Mar 17, 2024

@Falcosc What’s your opinion on this PR?
I’m planning to make a release very soon, should I include those stuff?

Thanks

@Falcosc
Copy link
Collaborator

Falcosc commented Mar 17, 2024

Thanks @xak2000
Looks good. Let's wait 2 days just to be sure since I can't test it anymore.
My AMD work notebook has an antivirus software which does not like our ring0 stuff, so I can't use the monitoring part anymore and just waste CPU cycles by applying always without checking if needed or not.

2000 is the highest value I can think of, since nobody is using below 3 Watt or 3 Amps
So technical 2999, but this would look strange :)
So if we have values which are not milli, we could support up to 2000

@FlyGoat you can merge as soon as you want to create the release, or I will merge in 2 days.

@Falcosc Falcosc marked this pull request as ready for review March 17, 2024 14:52
@xak2000
Copy link
Contributor Author

xak2000 commented Mar 17, 2024

@Falcosc Ok, thanks for the explanation.

The good part is: even if in some corner cases the if will falsely assume wrong multiplier, the script should still work as before because it doesn't directly use monitorFieldAdjTarget to compare the read value with. The real value that RyzenAdd reads just after write (monitorFieldAdjResult) will be used anyway (though, with a warning).

@FlyGoat
Copy link
Owner

FlyGoat commented Mar 17, 2024

@Falcosc I’ll leave it to you as I'm dumb to those stuff :-)

@Falcosc Falcosc merged commit 6dce42a into FlyGoat:master Mar 19, 2024
2 checks passed
@xak2000 xak2000 deleted the patch-1 branch March 20, 2024 15:02
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.

readjustService: unnecessarily multiplies tctl-temp by 0.001
3 participants