Skip to content

Add fan control wrappers #60

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

Merged
merged 1 commit into from
Mar 18, 2025
Merged

Add fan control wrappers #60

merged 1 commit into from
Mar 18, 2025

Conversation

codifryed
Copy link
Contributor

This adds wrappers for:

nvmlDeviceSetDefaultFanSpeed_v2
nvmlDeviceSetFanSpeed_v2

Tested working on multiple systems.

Resolves #59

@brayniac brayniac merged commit be1dfba into rust-nvml:main Mar 18, 2025
brayniac added a commit that referenced this pull request Mar 18, 2025
brayniac added a commit that referenced this pull request Mar 18, 2025
@brayniac
Copy link
Contributor

@codifryed - I mistakenly approved and merged this without realizing this introduced some build failures. Sorry about that. If this is still relevant, can I get you to rebase the changes on main and submit a new PR?

@raum-dellamorte
Copy link

The build failure appears to be that this created a duplicate of this commit adding fan control, #77. The functions set_fan_speed and set_default_fan_speed are identical in both this PR and #77. Close as duplicate?

I used this PR in my personal project. Yay! I can update nvfanservice to use the official crate! Or at least the official repo.

@codifryed
Copy link
Contributor Author

codifryed commented Mar 19, 2025

Indeed, #77 (a later semi-duplicate) adds these methods plus one more to set the fan policy. I'd say #77 is preferred because of that and the unwrapped_functions update, with one exception: my branch added tests for these methods. Since these methods modify the device, they are not run by default so I don't know if they're actually wanted or not. (seems superfluous)

@brayniac I'm happy to add those test "stubs" in a new PR if you think it's a good idea, otherwise we can consider these changes already merged.

Thank you for getting around to these PRs. Now that I know it's possible, I will make more. :)

Edit: &mut self is another difference I now see. I assume mut is desired for methods that change device settings. That was something I wasn't 100% sure about originally.

@brayniac
Copy link
Contributor

@codifryed - I agree the tests may be of limited use given that they are not run. While I'd happily take a PR that adds them for completeness, I'm not sure if they really serve much benefit. I think we can consider this closed by #77

I'm trying to catch up on outstanding PRs and will publish a release soon. Thanks again!

@ilya-zlobintsev
Copy link
Contributor

I'm sorry - I completely forgot to check existing PRs before making #77 🤦

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.

Fan Control Wrappers
4 participants