Aquacomputer D5 Next sensor reading support #482
Conversation
|
Awesome, thanks for working on this!
Small note: in this case, I think it'd be more appropriate to raise
Not yet. Also, while getting the kernel version should be relatively straightforward, that wouldn't tell us the full picture, as the user could be running an out-of-tree driver. Do you know how we might get information about the module? I haven't done a proper review yet, but at first glance the patch looks very looks good. |
Thanks.
Yes, that's a concern as well. I'm not yet that intimate with liquidctl, but if I'm not mistaken I saw some drivers accepting custom CLI flags. Maybe that could be used to override assumptions the code would make about the kernel version it detects. I'll have to explore and think about it more, because there's features piling up at the kernel driver repo that I have yet to send in. But purely for the sensor stuff for now, it should be a clear cut of what kernel supports what. I have a table in the readme to track that, aside from WIP branches for some other devices that I'm not going to send here just yet. |
I think that could get quite messy quite fast. A think a combination of us implementing graceful degradation and the user having the option to use (On a related note, for super hack level or only-enable-if-you-known-what-you're-doing options we also have |
(Which I've just discovered)
…ut so it's accessible
Not sure how I missed this one
…d finish hwmon tests
| plus_5v_voltage = ("+5V voltage", self._hwmon.get_int("in2_input") * 1e-3, "V") | ||
| sensor_readings.append(plus_5v_voltage) | ||
|
|
||
| if self._hwmon.has_attribute("in3_input"): |
There was a problem hiding this comment.
Should a warning be produced if this is not available?
There was a problem hiding this comment.
Yeah, I think so. That attribute is useful, and our other drivers emit warnings in similar situations:
_LOGGER.warning('some attributes cannot be read from %s kernel driver', self._hwmon.module)I don't think the warning needs to be more specific than that, even though here it's just one attribute. In the future there might be more (or at least different) attributes missing, and I don't think most users need to care about the specifics (while this is an useful attribute, everything else is still quite functional without it).
That said, one downside of the non-specific warning is that users will need to check the documentation and/or try --direct-access just to see what they might be missing. But that's a general problem with all of our (hwmon-aware) drivers, so this probably isn't the best place to consider it.
There was a problem hiding this comment.
Added that, thanks.
README.md
Outdated
| @@ -125,6 +125,7 @@ The notes are sorted alphabetically, major (upper case) notes before minor | |||
| | AIO liquid cooler | [NZXT Kraken Z53, Z63, Z73](docs/kraken-x3-z3-guide.md) | USB & USB HID | <sup>_p_</sup> | | |||
| | DDR4 DRAM | [Corsair Vengeance RGB](docs/ddr4-guide.md) | SMBus | <sup>_Uax_</sup> | | |||
| | DDR4 DRAM | [Generic DDR4 temperature sensor](docs/ddr4-guide.md) | SMBus | <sup>_Uax_</sup> | | |||
| | Fan/LED controller | [Aquacomputer D5 Next](docs/aquacomputer-d5next-guide.md) | USB HID | <sup>_ehn_</sup> | | |||
There was a problem hiding this comment.
It's category is more of a "watercooling pump/fan controller", since it only controls it's own LEDs... Is the existing text OK or should I write it differently?
There was a problem hiding this comment.
I think "Fan/pump controller" might be a reasonably appropriate and short name, and it can also replace the "Fan/AIO controller" category used for the NZXT H1 V2.
There was a problem hiding this comment.
Thanks, marked both as that.
|
Just saw that you marked the PR as ready for review, thanks! I'll try to do it this weekend. |
There was a problem hiding this comment.
Hi,
I really appreciate the attention to detail, the PR is super clean. I left a few notes, but they are very minor.
Regarding the remaining applicable items in the checklist, you can leave updating the man page to me, as for now that'll just be a placeholder, and I already need to add those to some other devices.
But can you also try to document the protocol (as much as currently implemented by the driver) in docs/developer/protocol/*?
| sensor_readings = [] | ||
|
|
||
| # Read temp sensor values | ||
| for idx, temp_sensor_offset in enumerate(self._device_info["temp_sensors"]): |
There was a problem hiding this comment.
This is probably just as easy to read as is, if not more, but a reminder that you could also do something like:
| for idx, temp_sensor_offset in enumerate(self._device_info["temp_sensors"]): | |
| for label, offset in zip(self._device_info["temp_sensors_label"], self._device_info["temp_sensors"]): |
...here and in the similar loops bellow.
There was a problem hiding this comment.
Thanks, I've been aware of zip() before, but I never had an opportunity to use it (in Python or elsewhere). I converted that loop to use it, since it fits in nicely.
There was a problem hiding this comment.
Let me start by saying sorry for the noise!
I think it's (more) important to be consistent throughout the file. And while it's possible to use zip in the other loops too, it'll probably look a bit unwieldy when more than one label is needed per iteration. Honestly, even in this loop, I think I prefer the look of your original version, thanks to the shorter and more uniform lines.
I also thought that there could be a performance penalty in doing the extra indexed (and checked) access, but I can't actually see that in a quick microbenchmark, at least not with Python 3.10.[1]
So I might revert this back before merging the PR, unless you want to try using zip on the other loops.
Again, sorry!
[1] (EDIT) With small lists it makes no difference, but if the lists were larger enumerate+[idx] could have a moderate to significant (negative) impact:
$ SIZE=2
$ python -m timeit -s "values = list(range(0, $SIZE))" "for a, b in zip(values, values): pass"
2000000 loops, best of 5: 130 nsec per loop
$ python -m timeit -s "values = list(range(0, $SIZE))" "for idx, a in enumerate(values): b = values[idx]"
2000000 loops, best of 5: 125 nsec per loop
$ SIZE=10
$ python -m timeit -s "values = list(range(0, $SIZE))" "for a, b in zip(values, values): pass"
1000000 loops, best of 5: 244 nsec per loop
$ python -m timeit -s "values = list(range(0, $SIZE))" "for idx, a in enumerate(values): b = values[idx]"
1000000 loops, best of 5: 317 nsec per loop
$ SIZE=100
$ python -m timeit -s "values = list(range(0, $SIZE))" "for a, b in zip(values, values): pass"
200000 loops, best of 5: 1.49 usec per loop
$ python -m timeit -s "values = list(range(0, $SIZE))" "for idx, a in enumerate(values): b = values[idx]"
100000 loops, best of 5: 2.41 usec per loop
$ SIZE=1000
$ python -m timeit -s "values = list(range(0, $SIZE))" "for a, b in zip(values, values): pass"
20000 loops, best of 5: 13.7 usec per loop
$ python -m timeit -s "values = list(range(0, $SIZE))" "for idx, a in enumerate(values): b = values[idx]"
10000 loops, best of 5: 28.9 usec per loopThere was a problem hiding this comment.
No problem, I'll revert it now to make it easier.
There was a problem hiding this comment.
Just saw the edit - off the top of my head there will be 8-10 max register offsets (for Octo), so we're good.
|
Stuff you commented on should now be resolved, please take a look. I'll add a protocol description later during the day, hopefully. I've written about it before, so I'll use it to speed up the process as it's good enough for the sensor report aspect. (The control/configuration report has been reverse engineered a bit more throughly, and that needs to be updated.) |
Thank you!
Yes, that's good enough, thanks! |
This reverts commit 9759b6b. # Conflicts: # liquidctl/driver/aquacomputer.py
|
I've pushed the docs and added serial number retrieval to initialization. I've made the protocol docs general, because the devices are similar and I'll be extending it in future PRs once this one is merged. |
|
Merged, thank you!
Yeah, that makes sense. |
|
Err, I just noticed an issue. You're overriding the Probing is not supposed to result in any reads or writes, and in the case of USB HIDs (and other USB devices), that properly should only return the USB Related to it, there something else I noticed but had so far left for later: Unless you end up (in the next devices) needing the firmware version and serial number outside of We also currently don't expose a general firmware version property for all devices. While it's fine to do have it (when, like here, it' seems stable and cheap to compute), it's also not necessary. |
Sorry, that was accidental, I didn't realize there was an existing function with that same name. Is it OK to rename it so the true one is not overriden?
Makes sense, I'll move parsing to their respective functions. I'll send a PR tomorrow as it's getting late here. |
Yeah, I missed it too.
In the case of the serial number I think it's probably better to not have it as public property. IIRC we don't have it on any other drivers, and for those specifically interested in it, it would be a way to bypass calling (Besides |
As promised in #438, here is the first PR for adding support for Aquacomputer devices to liquidctl. This PR adds a new driver called
aquacomputer, and it currently supports reading sensor values of the D5 Next watercooling pump.Aquacomputer devices share same layouts of sensor substructures/groups in their HID reports, so adding sensor reading support for other devices, such as Octo, Quadro, Farbwerk, Farbwerk 360 should be trivial. (Just need to specify the offsets in device specific data and write some cases for non-general sensors.) They are all supported by the partially mainlined driver that I maintain.
Some advanced features of the devices are still being investigated and reverse engineered, and additionally as far as I know nobody has looked at the RGB controls yet. The kernel driver supports sensor reading and directly controlling the PWM inputs, where available.
Marking this PR as draft for now, as I have to check more boxes down below. I've formatted the code using black (haven't used it before), so hopefully it looks as it should.
And a question for the future: the kernel driver support for devices varies between kernel versions. Is there a clean way to get the kernel version from liquidctl already?
Related: #439
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/