Commander Core: Add support for the Commander Core XT#478
Commander Core: Add support for the Commander Core XT#478jonasmalacofilho merged 1 commit intoliquidctl:mainfrom
Conversation
jonasmalacofilho
left a comment
There was a problem hiding this comment.
Overall it's looks very good, thanks for being thorough. Can you make the two adjustments I pointed out?
Thanks again!
@ParkerMc, does it also look good to you?
docs/corsair-commander-core-guide.md
Outdated
| Valid channel values are `pump`, `fanN`, where 1 <= N <= 6 is the fan number, and | ||
| `fans`, to simultaneously configure all fans. | ||
| Valid channel values on the Core (non-XT) are `pump`, `fanN`, where 1 <= N <= 6 is the fan number. | ||
| On the Core XT, the `pump` channel is not present and fanN starts at 0. The `fans` channel can be |
There was a problem hiding this comment.
In liquidctl we prefer to always to number related channels starting at one, as those will be user-facing names.
There was a problem hiding this comment.
Ah, didn't realize that. I've updated the change to use 1-based indexing for user-facing names.
| def test_parse_channels_commander_core(): | ||
| """This test will go through and thoroughly test CommanderCore._parse_channels so we don't have to in other tests""" | ||
| tests = [ | ||
| core = CommanderCore(MockCommanderCoreDevice(), 'Corsair Commander Core (experimental)', True) | ||
| corext = CommanderCore(MockCommanderCoreDevice(), 'Corsair Commander Core (experimental)', False) | ||
|
|
||
| core_tests = [ | ||
| ('pump', [0]), ('fans', [1, 2, 3, 4, 5, 6]), | ||
| ('fan1', [1]), ('fan2', [2]), ('fan3', [3]), ('fan4', [4]), ('fan5', [5]), ('fan6', [6]) | ||
| ] | ||
| ] | ||
|
|
||
| corext_tests = [ | ||
| ('fans', [0, 1, 2, 3, 4, 5, 6]), | ||
| ('fan0', [0]), ('fan1', [1]), ('fan2', [2]), ('fan3', [3]), ('fan4', [4]), ('fan5', [5]), ('fan6', [6]) | ||
| ] | ||
|
|
||
| for (val, answer) in tests: | ||
| assert list(CommanderCore._parse_channels(val)) == answer | ||
| # Run Core (non-XT) tests | ||
| for (val, answer) in core_tests: | ||
| assert list(core._parse_channels(val)) == answer | ||
|
|
||
| # Run Core XT tests | ||
| for (val, answer) in corext_tests: | ||
| assert list(corext._parse_channels(val)) == answer |
There was a problem hiding this comment.
It would be better to split this into two tests, one for each variant.
This way, we get a better "at a glance" diagnostic if one of them ever fails, plus they are also better suited if, in the future, the implementations have to diverge more.
There was a problem hiding this comment.
Sure, makes sense. Updated.
README.md
Outdated
| | DDR4 DRAM | [Generic DDR4 temperature sensor](docs/ddr4-guide.md) | SMBus | <sup>_Uax_</sup> | | ||
| | Fan/LED controller | [Corsair Commander Pro](docs/corsair-commander-guide.md) | USB HID | <sup>_h_</sup> | | ||
| | Fan/LED controller | [Corsair Commander Core](docs/corsair-commander-core-guide.md) | USB HID | <sup>_ep_</sup> | | ||
| | Fan/LED controller | [Corsair Commander Core, Core XT](docs/corsair-commander-core-guide.md) | USB HID | <sup>_ep_</sup> | |
There was a problem hiding this comment.
Forgot one.
This temporarily needs to be split into two entries, so that the one for the Core XT can be tagged with n ("Newly supported and requires git").
Looks good to me! I think the changes Jonas mentioned will take care of everything. |
|
Actually, |
Update the Commander Core to recognize the Commander Core XT Product ID, and update channel/temperature naming across the driver to account for the lack of an AIO on this variant of the device. Other than this, the protocol appears identical and at least fan speed readout and control have been tested by me.
db2167a to
f417aff
Compare
Sure thing, I've added some updates to that file. |
Awesome! If you would like to work on some more support for the core/core XT, throw a message in the discord and I can get you up to speed on what I know. I just don't have the time to implement it myself unfortunately. |
|
Thanks! |
|
confirming latest git picks up the device: |
SWEET :D :D :D |
so the device has 6 corsair 4-pin fan led hookups, and 1 3-pin corsair strip led hookup. i don't have any of the former, but i am using 40 leds on the latter. it doesn't look like it shows up here, though. firmware version is correct! |
Can you show us the full output of the same command with |
It has 7 RGB ports listed in the initialize command. With only three pins, it may not be able to detect the number of LEDs. If you do |
|
as requested: |
|
The debugging does not show that any other RGB device is connected. As a double check if you want send that again(with |
Update the Commander Core to recognize the Commander Core XT Product ID,
and update channel/temperature naming across the driver to account for
the lack of an AIO on this variant of the device. Other than this, the
protocol appears identical and at least fan speed readout and control
have been tested by me.
Fixes:
Closes:
Related:
#410
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/