search

KsenijaS / krakenx

Python script to control NZXT cooler Kraken X52/X62 in Linux
GNU General Public License v2.0
181 stars 20 forks source link

Switch to the liquidctl driver #27

Closed jonasmalacofilho closed 5 years ago

jonasmalacofilho commented 6 years ago

We have decided in #11 to switch to the liquidctl driver. I'm opening this issue so that we can track that effort.

Current snapshot: jonasmalacofilho:switch-to-liquidctl-driver (diff)

Obviously we need to:

@KsenijaS has pointed out that we also need to:

I also think we should do a couple of releases for PyPI:

EDIT: @jneumaier had already thought about and done the release : )

jonasmalacofilho commented 6 years ago

By the way, who of us will actually implement this? : )

I can do it of course, but maybe @jneumaier can take this on as well?

By the way, I will prepare a 1.1.0 release of liquidctl (with a few extra color modes and maybe some other small features), so that krakenx can just import it via setup.py/PyPI.

jonasmalacofilho commented 5 years ago

I started working on the initial driver switch, see jonasmalacofilho:switch-to-liquidctl-driver.

Note: you need a current 'master' version of liquidctl, while I finish preparations for the upcoming release.

jneumaier commented 5 years ago

@jonasmalacofilho Sorry for the late reply. I hoped to find some time to help. To be honest it might have taken me quite some time to find this "easy solution" in the end. The code is really comprehensible now.

Integrates very well and works fine already. Only the logo/ring/sync separation needs some more work from what I tested. Maybe an easy solution would be to always set ring only and separately set text color if a value is given (except for MODE_SOLID_ALL). And "Write only specified values for fan, pump and color schemes #21" would be nice later on. Makes no sense that I always overwrite my profiles playing around with colors.

I would be fine to harmonize the mode names a little (Police, Spinner, Chaser) - no idea which names make more sense, guess the krakenx ones are made up? :-) Would make it easier to maintain the documentation (we could add aliases but mode name changes would be fine in my opinion).

jonasmalacofilho commented 5 years ago

Maybe an easy solution would be to always set ring only and separately set text color if a value is given.

That's a good idea. But there are cases where syncing both channels might also be desired (e.g. breathing).

In the end, I think we really need a --color_channel/-ch option, and I just added that to the branch.

This should fix #22 and allow us to enable all other remaining color modes. But, at the same time, we are already somewhat moving towards the adoption of #21: two separate calls would be necessary in those cases, in this changes how the UI is used. But I'm fine with it... I agree with you that having it

overwrite my profiles playing around with colors"

is not optimal. Also, I change fan and pump speeds often; I don't want to think about the RGB all the time.

jneumaier commented 5 years ago

I moved protocol.md and added a note about the driver in there. For now I would be fine with this. You may add further adjustments to the documentation or just mark the task as done :-)

jonasmalacofilho commented 5 years ago

@jneumaier thanks!

Though I still need to expose (and document) the new color modes.

jonasmalacofilho commented 5 years ago

Rebased and updated jonasmalacofilho/switch-to-liquidctl-driver (diff):

@jneumaier can you take a look on the naming and missing documentation?

KsenijaS commented 5 years ago

Hi Jonas,

Thank you for making those changes! They will make the code much more readable. I have a few comments though:

  1. I wouldn't replace MODE_SOLID with COLOR_MODES[1]; line 44 is not readable at all. It's not obvious what COLOR_MODES[1] is, nor is it obvious in COLOR_MODES's definition that the order of modes would change what the code does. Maybe consider defining a MODE_SOLID mode outside COLOR_MODES, inserting it to COLOR_MODES at the right index, and referring to it directly?

  2. Using the string "super-" to denote that text color can be changed on a mode is error prone and unreadable. First, it's not obvious in a mode's definition that prefixing its name with "super-" does anything functional. Second, it's not obvious what "super-" means. I think it'd be better if this were a third field in the mode namedtuple, and if it were called something more declarative (e.g. custom_text_color or something)

I wanna hear what you think.

jonasmalacofilho commented 5 years ago

Thanks, you were right, those were ugly.

  1. Did basically that.

  2. While super does mean something in the driver – a mode where LEDs are referenced individually – the code was indeed unnecessarily convoluted. Fixed that by adding a new uses_text_color field to the Mode tuple.

By the way, while strange, most modes don't actually do anything with the supplied text color. IIRC one other (non "super") mode used it, but I decided to hid that in liquidctl/the driver and keep things simple. CAM doesn't expose that either, and selecting the appropriate channel will always give better results.

jneumaier commented 5 years ago

Thanks a lot @jonasmalacofilho

How should we proceed here?

I have a lot of time at the first weekend in February and will finally help a little here. Sorry for the delay.

Feel free to edit this post.