Commander Core: Add fixed speed support #405
Conversation
|
Still working on documentation, tests, and updating collected device data, but everything else is ready for feedback. |
There was a problem hiding this comment.
The implementation looks great. I especially like how you were able to only change the mode of the specified channel, without storing anything on the liquidctl side.
Just one comment on the documentation, and a nitpick on the casing of a status attribute name.
docs/corsair-commander-core-guide.md
Outdated
| ├── AIO LED count 29 | ||
| ├── RGB port 1 LED count 8 | ||
| ├── RGB port 2 LED count 8 | ||
| ├── RGB port 3 LED count N/A | ||
| ├── RGB port 4 LED count N/A | ||
| ├── RGB port 5 LED count N/A | ||
| ├── RGB port 6 LED count N/A | ||
| ├── AIO Pump Yes |
There was a problem hiding this comment.
Should AIO pump become just AIO?
There was a problem hiding this comment.
I think that makes sense. Or maybe even AIO port.
Also, since these are all booleans, maybe AIO port connected or AIO port in use? (And similarly for the fan ports).
docs/corsair-commander-core-guide.md
Outdated
| Note: There is a hardware minimum speed when setting a fixed duty cycle. Anything below that point just runs at the minimum. | ||
| This means that 0 is not stopped and a lot of the lower speeds mean the same speed. |
There was a problem hiding this comment.
In this case, I think it's better to explicitly state the allowed range. Something like:
The firmware limits the minimum pump and fan speeds in fixed mode: for the pump, it is {}%; for the fans, it is {}%.
There was a problem hiding this comment.
So iCUE, limits between 0% and 20%. I can set it to 0% or anything 20% and higher. I have two different types of fans now ML and LL, when set to 0% the ML's will stop and the LL's will not. So I'll have to test the differences more thoroughly.
As for the pump, iCUE limits it to 3 preset settings. Quiet, Balanced, Extreme, and Variable Speed. I will figure out what exactly those are and test lower ones to see how exactly the pump responds.
There was a problem hiding this comment.
Oh, I see. In that case, let's leave them unrestrained for now.
There was a problem hiding this comment.
I rewrote that note so it makes more sense.
| @@ -99,7 +114,27 @@ def set_speed_profile(self, channel, profile, **kwargs): | |||
| raise NotSupportedByDriver | |||
|
|
|||
| def set_fixed_speed(self, channel, duty, **kwargs): | |||
There was a problem hiding this comment.
Any thoughts on displaying the fan/pump mode and target fixed speed (if set) that are set here in the status message? Maybe potentially under --verbose? If so is there a device that does this already or format ideas?
This can be another PR, I just had the idea.
There was a problem hiding this comment.
That would be nice.
For the pump/fan mode, take a look at the Commander Pro and Smart Device (V1): those specific devices only report PWM or DC, but you could extend that idea for whether their running in fixed PWM or on-board profile modes. IIRC, they use enums with a suitable __str__() implementation for the pretty/human output.
For the target duty cycle (if set), take a look at the Corsair Platinum coolers.
That was actually a goal I had because I really don't want it to change speed and mess up the fan curve when I switch between windows and linux unless I specifically tell it to. Protocol made it decently easy, just have to add the read and modify the data. |
|
Thanks! |
Adds pump and fan detection to initialize.
Adds support for setting a fixed percentage as the speed to the pump and each fan.
Related: #218
Checklist:
liquidctl.8Linux/Unix/Mac OS man pagedocs/*guide.mddevice guidesNew CLI flag?
extra/completions/New device?
extra/linux/71-liquidctl.rules(instructions in the file header)en)New driver?
docs/developer/protocol/