liquidctl / liquidtux

Linux kernel hwmon drivers for AIO liquid coolers and other devices
Other
79 stars 14 forks source link

[WIP] Add driver for aquastream ULTIMATE #4

Closed grmat closed 2 years ago

grmat commented 4 years ago

Hi,

building upon your work, I've put together a driver for the Aqua Computer aquastream ULTIMATE. This device is also USB HID based and supports different sensors, so it's quite similar to yours.

This is WIP state and clearly needs cleanup, however, I'd be interested in your thoughts if you got the time and motivation. And also take the chance to ask some questions.

This is the output I currently get:

# sensors aquastream_ultimate-hid-3-1
aquastream_ultimate-hid-3-1
Adapter: HID adapter
Pump voltage:  12.05 V
Fan voltage:   11.54 V
Pump RPM:     3399 RPM
Fan RPM:       739 RPM
internal:      +24.9°C
temp2:             N/A
temp3:             N/A
temp4:             N/A
temp5:         +24.9°C
Pump power:     2.91 W
Fan power:    550.00 mW
Pump current: 242.00 mA
Fan current:   48.00 mA

I've added labels because as you can see, some values are read for both the pump and the connected fans. There's also a target value for the pump RPM, but I intend the driver to be read-only in v1. I haven't implemented flow values yet (they don't fit in hwmon's types) and wasn't able to figure out the whole report.

I'd like to ask you how your plans are with this project. Do you intend in sending patches upstream? Do you have immediate TODOs, planned features or any other goals?

Regards, and thanks for the work

jonasmalacofilho commented 3 years ago

Hi @grmat,

I'm very sorry for not answering you sooner.

From a glance, I think your driver is as good as mine. But it also has the same problem as mine, which is a data race between the raw_event handler and sysfs reads: even if it appears to work on our x86-64 machines, that's (AFAIK) undefined behavior.

After that's fixed (and I also need to clean up my code), I think we should submit them upstream. Before I thought read only (that is, incomplete) drivers would not be accepted, according to the hwmon documentation, but taking a look at the latest HID+HWMON drivers that were merged, it doesn't appear to be a deal breaker after all.

In fact, we can use these new upstream HID+HWMON drivers as a reference for how to solve our data races. IIRC they are drivers for Corsair RMi/HXi power supplies and some model of Corsair AIO or fan hub.

amezin commented 2 years ago

@jonasmalacofilho @grmat Aquacomputer has a whole family of devices, and their own "bus", and seem to be more focused on monitoring and fan control, rather than RGB, so this is very interesting. Unfortunately, I don't own any of their hardware.

Regarding data races: memcpy() is most likely wrong, but (natural) aligned stores are considered atomic in Linux kernel code, AFAIK. So just copying data from packed to a non-packed struct in raw_event should solve all issues (also it would be nice to have temp_* as an array). And if every field is a uint16_t, I can imagine a few tricks to avoid having two similar struct definitions...

Although a read-only driver isn't that useful, yep.

And a reply 1,5 years later most likely isn't that useful too :)

jonasmalacofilho commented 2 years ago

Closing as this has stalled. Additionally, these days it probably makes more sense to add support for this device to the mainline aquacomputer_d5next driver (kernel 5.15 or later).