Corsair Commander Core: Add support for speed curve profiles#687
Corsair Commander Core: Add support for speed curve profiles#687jonasmalacofilho merged 1 commit intoliquidctl:mainfrom agireud:corsair_commander_core
Conversation
|
I only took a quick glance but looks around what I expected. I'm glad my guess with the read more command worked out. Thanks for finishing the implmentation! |
|
@ParkerMc turns out I spoke too soon! My previous implementation was flawed but I was able to figure out the protocol and get everything fully functional. The read commands ended up looking quite different though. |
agireud
changed the title
agireud
changed the title
agireud
changed the title
jonasmalacofilho
left a comment
jonasmalacofilho
left a comment
There was a problem hiding this comment.
Looks good to me except for the small nit. I'll fix it on my end and merge it.
Thanks for the PR (and for the poke on Discord)!
| _New in 1.9.0: fixed duty cycles are now supported._<br> | ||
| _Changed in 1.11.0: the Corsair Commander Core XT is now supported._<br> | ||
| _Changed in 1.12.0: the Corsair Commander ST is now supported._<br> | ||
| _New in 1.13.1: speed curve profiles are now supported._<br> |
There was a problem hiding this comment.
Nit: I've generally opted to keep "New in" at the top of the sections describing the new feature. And as such they apply to the entire section, and need no further description.
In this case, it would be:
_New in 1.9.0._<br>
for the fixed speeds and
_New in git._<br>
for the newly suported speed curves.
To be honest, it may be the case that some of the issues have indeed been solved with a8be683 (PR #687). However, there still seems to be a long list of remaining issues, and I think we're starting to lose track of what works and what doesn't. This partially reverts commits 6426a06. Related: #520, #583, #598, #623, #705 Related: PR #687
|
Are the drivers installed automatically or do I need to add them to my system? |
4 participants
Continues where #454 left off and implements support for speed curve configuration for the pump/fans in the Corsair Commander Core driver. Much of this is based off the initial work by @ParkerMc (thank you!) but has deviated significantly from the protocol documented in the original merge request.
Two areas are of particular interest:
_CMD_READhas been split into 3 separate read commands (_CMD_READ_INITIAL,_CMD_READ_MORE,_CMD_READ_FINAL) which are necessary to continue reading data from the device when the data exceeds the device->host packet size.The addition of
_CMD_WRITE_MOREwhich allows writing data to the device when the data exceeds the host->device packet size. (Based off the work @ParkerMc did for OpenRGB).I am currently running this on my Gentoo Linux system without any hiccups and the fan duties are adjusting appropriately based on temperature readings in all cases I've tested. The Corsair Commander Core is running the latest v2.11.221 firmware.
I've verified all existing tests are passing and also added two new tests for speed curve profiles. If anyone is available to aid with further testing that would be helpful.
Note: Missing from this implementation is the ability to specify which temperature sensor is utilized by a speed curve profile, but extending support should be straight forward once this gets merged. Currently it is hard coded to utilize the built-in water temperature sensor.
Closes: #454, #447
Related: #218, #520, #623
Checklist:
docs/*guide.mddevice guides, with ["new/changed in" notes]liquidctl.8Linux/Unix/Mac OS man pagedocs/developer/protocolNew CLI flag?
extra/completions/New device?
extra/linux/71-liquidctl.rules(instructions in the file header)gitMRLVNew driver?
docs/developer/protocol/