nzxt-kraken3 improvements

aleksamagicka commented 1 year ago

Received my X53 yesterday (#21), so here is the first batch of improvements for nzxt-kraken3. Notable changes:

Added second product ID
Added pump duty percent as a fan output
Moved commands and their lengths to static variables
Commands are now being sent through an internal buffer, which is protected by a mutex
Commands now match liquidctl behaviour and are expanded with zeroes up to 64 bytes
Exposed firmware version through debugfs
Reformatted code to 100 chars

I'll look into PWM and curve reading/writing next. I've named some variables X53_... for now, once we get to a Z series device, we can make them general and differentiate between the devices in kraken3_probe() and elsewhere easily.

jonasmalacofilho commented 1 year ago

Thanks! I'll take a look in the next few days, as soon as I can.

aleksamagicka commented 1 year ago

Thanks for the invite, accepted! Just pushed PWM fixed duty setting as well.

aleksamagicka commented 1 year ago

I've ordered the Kraken Z53, which I'll receive next week, so I'll implement support for it as well.

aleksamagicka commented 1 year ago

I've been investigating writing curves through the driver. For reference, the ltc4222 is a very clear example of how to use and define attributes. My current understanding is:

That there is no way to read the curve from the device
That it has a fixed number of points relating to temps in the range of 20C to 59C? (correct me if I'm wrong here). Either way, there's a fixed number of points for which to set duty.

Just putting this out there before deciding on if it's worth implementing that.

jonasmalacofilho commented 1 year ago

For reference, the ltc4222 is a very clear example of how to use and define attributes.

What's wrong with temp[1-*]_auto_point[1-*]_pwm / temp[1-*]_auto_point[1-*]_temp, except not resulting in a very ergonomic or always efficient interface? (And assuming I intuitively pick the right pair or attributes).

And, for what it's worth, I think custom attributes are generally frowned upon in hwmon.

My current understanding is:

That there is no way to read the curve from the device

AFAIK, correct.

That it has a fixed number of points relating to temps in the range of 20C to 59C? (correct me if I'm wrong here). Either way, there's a fixed number of points for which to set duty.

Correct.[^1]

[^1]: The first suggestion of the 20°C–59°C range is: https://github.com/liquidctl/liquidctl/issues/78#issuecomment-606675627. Tests since seem to confirm that that's right.

aleksamagicka commented 1 year ago

Nothing wrong with that, that's what I'd want to use here. I mentioned that driver because I saw attribute defs and inits there without too much clutter, which is how the temp curves are defined as far as I know right now. (Looking at an old aquacomputer driver PR for fan curves, I see similar definitions, but I haven't played with that all yet.)

Will definitely look into it at some point, but most likely Z53 support will be coming sooner as that's easier. For curves pwm_enable will also have to be added to be able to switch between direct pwm and curves.

On Mon, Oct 3, 2022, 10:39 AM Jonas Malaco @.***> wrote:

For reference, the ltc4222 https://elixir.bootlin.com/linux/v6.0-rc7/source/drivers/hwmon/ltc4222.c is a very clear example of how to use and define attributes.

What's wrong with temp[1-]_auto_point[1-]_pwm / temp[1-]_auto_point[1-]_temp, except not resulting in a very ergonomic or always efficient interface? (And assuming I intuitively pick the right pair or attributes).

And, for what it's worth, I think custom attributes are generally frowned upon in hwmon.

My current understanding is:

That there is no way to read the curve from the device

AFAIK, correct.

That it has a fixed number of points relating to temps in the range of 20C to 59C? (correct me if I'm wrong here). Either way, there's a fixed number of points for which to set duty.

Correct.1 <#m_2708271608043856359m-8361878339550448626_user-content-fn-1-4af270f3b0bc5bcf2923da73d631e69f> Footnotes

1.

The first suggestion of the 20°C–59°C range is: liquidctl/liquidctl#78 (comment) https://github.com/liquidctl/liquidctl/issues/78#issuecomment-606675627. Tests since seem to confirm that that's right. ↩ <#m_2708271608043856359m-8361878339550448626_user-content-fnref-1-4af270f3b0bc5bcf2923da73d631e69f>

— Reply to this email directly, view it on GitHub https://github.com/liquidctl/liquidtux/pull/22#issuecomment-1265117258, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACQGRIMZKN7VJED3XEVR6QDWBKLSJANCNFSM6AAAAAAQXWWXLE . You are receiving this because you authored the thread.Message ID: @.***>

aleksamagicka commented 1 year ago

Z53 sensor support is now in, no PWM yet:

z53-hid-3-a
Adapter: HID adapter
Pump speed:     870 RPM
Pump duty [%]:   20 RPM
Fan speed:        0 RPM
Fan duty [%]:     0 RPM
Coolant temp:   +23.7°C

amezin commented 1 year ago

40x copy-paste looks scary. Isn't it easier to build the attribute array(s) in init function at this point? Statically allocating them, and filling in module's init is likely the simplest approach. Although I don't know what's preferred in the upstream (maybe even the copy-paste method with static initialization is the preferred one).

aleksamagicka commented 1 year ago

I've been looking at this, this and this and saw a lot of definitions similar to mine. As far as I know, directly creating sysfs attributes is discouraged (and these macros exist).

aleksamagicka commented 1 year ago

Curves should be working fine and I encourage everyone to test, if possible.

One thing I'm not completely sure about is writing 1 to pwmX_enable - in other words, switching from curve mode to direct PWM duty mode, as we can right now only mark the fan curve as disabled, because the direct PWM duty is not cached. Setting PWM directly also brings it to the same state, so I think that the current behaviour is OK - just set the direct PWM duty if you want it, and you can always reactivate the curve by writing 2. Thoughts?

Edit: perhaps the following is better:

Setting PWM directly -> pwmX_enable is 1
Applying the curve -> pwmX_enable is 2 by writing to it
Setting a curve point -> pwmX_enable currently resets to 1, but it should be 0, since we now don't have control of the device in a way that we know, and neither have we set a direct PWM value to it last

We'll still need to disallow writing 1 to it because we don't cache the last fixed duty value.

aleksamagicka commented 1 year ago

Side note, the Z53 pump is so much quieter than the X53. With both being next to me on the desk, the X53 has a much more pronounced humming/raspy sound. A few times I had to look at the sensors output to check if the Z53 was even running at full speed because I couldn't hear it.

aleksamagicka commented 1 year ago

Good point on the multi-line comment style, I haven't ever sent a patch with those to Guenter and I had no idea that the net subsystem was special beyond the 80 char line limit.

I haven't yet tested this with liquidctl, but I plan to once we figure out the pwmX_enable behavior (which is now up to me to implement according to comments) and fix other stuff. I'll definitely send a PR for liquidctl so that it can use the now expanded capabilities of this once it's merged.

I'll work on this, but perhaps not immediately.

jonasmalacofilho commented 1 year ago

Aside: with these improvements, liquidctl should automatically start using this driver for reading status info from X models. But the Z models will require a few changes (there). And fan/pump control using hwmon has yet to be implemented there.

aleksamagicka commented 1 year ago

I'll test liquidctl next, and work on adding hwmon curve and Z53 support to it.

jonasmalacofilho commented 1 year ago

Thanks!

liquidctl / liquidtux

nzxt-kraken3 improvements #22