Add support for setting fan speed for D5 Next #499

aleksamagicka · 2022-08-28T17:58:53Z

Add support for setting pump and fan speed for the D5 Next pump in the aquacomputer.py driver.

I had to use the crcmod library to calculate CRC-16/USB checksums, because the control HID report (that contains all the settings for a device) has one at the end and it needs to be updated when the settings are changed. Please let me know if I missed adding it somewhere.

Also I wanted to ask if it's OK to utilize the hwmon driver for writing stuff to the device. I don't really see than being done in other drivers, so maybe --use-hwmon can be passed in here to let the user go through the kernel driver?

Related: #439

Checklist:

Adhere to the development process
Conform to the style guide
Verify that the changes work as expected on real hardware
Add automated tests cases
Verify that all (other) automated tests (still) pass
Update the README and other applicable documentation pages
Update the liquidctl.8 Linux/Unix/Mac OS man page
Update or add applicable docs/*guide.md device guides
Submit relevant data, scripts or dissectors to https://github.com/liquidctl/collected-device-data

New CLI flag?

Adjust the completion scripts in extra/completions/

New device?

Regenerate extra/linux/71-liquidctl.rules (instructions in the file header)
Add entry to the README's supported device list with applicable notes (at least en)

New driver?

Document the protocol in docs/developer/protocol/

jonasmalacofilho · 2022-09-18T06:06:36Z

Sorry again for the delay.

Also I wanted to ask if it's OK to utilize the hwmon driver for writing stuff to the device.

Yes, and it should actually be preferred (when there's a hwmon driver).

I don't really see than being done in other drivers,

That's mostly just because hwmon support in liquidctl is new and not yet complete, but also because not all current hwmon drivers expose PWM control (e.g. Kraken X2, because of device/protocol issues, or Corsair RMi/HXi, due to safety concerns, IIRC).

so maybe --use-hwmon can be passed in here to let the user go through the kernel driver?

No, I think it should be the other way around, just as with get_status(): the hwmon driver is used by default, unless --direct-access is passed.

Besides being more consistent, this minimizes the chances of putting either the device or the kernel driver in a inconsistent/unexpected state.

aleksamagicka · 2022-09-18T09:57:12Z

Great, thanks. I'll rework it to use hwmon first, if possible.

Offsets from: aleksamagicka/aquacomputer_d5next-hwmon@5e3ae6f

…) and hwmon access

…t mode

aleksamagicka · 2022-09-18T13:21:35Z

Writing to hwmon is now implemented in the aquacomputer driver, please take a look.

jonasmalacofilho

Thanks!

jonasmalacofilho · 2022-09-19T02:43:44Z

docs/aquacomputer-d5next-guide.md

+Valid channel values on the D5 Next are `pump` and `fan`. The hwmon driver is always used first, if it's available and
+exposes `pwmX` and `pwmX_enable` attributes. You can pass in `--direct-access` to bypass it and communicate with the
+device directly.


I don't think it's necessary to talk about hwmon here, especially since the very next section is about it (and how to bypass it).

jonasmalacofilho · 2022-09-19T02:46:09Z

liquidctl.8

@@ -236,7 +236,7 @@ Internal data used by some drivers.
 .SS Aquacomputer Farbwerk 360
 .SS Aquacomputer Octo
 .SS Aquacomputer Quadro
-Cooling channels: (D5 Next, Octo, Quadro:) not yet supported; (Farbwerk 360:) not applicable.
+Cooling channels: (D5 Next): supported, (Octo, Quadro:) not yet supported; (Farbwerk 360:) not applicable.


Suggested change

Cooling channels: (D5 Next): supported, (Octo, Quadro:) not yet supported; (Farbwerk 360:) not applicable.

Cooling channels: (D5 Next): \fIfan\fR, \fIpump\fR; (Octo, Quadro:) not yet supported; (Farbwerk 360:) not applicable.

jonasmalacofilho · 2022-09-19T02:50:15Z

liquidctl/driver/aquacomputer.py

+            _LOGGER.info(
+                "bound to %s kernel driver, writing fixed speed to hwmon", self._hwmon.driver
+            )
+            return self._set_fixed_speed_hwmon(channel, duty)


This will fail if the hwmon driver is present, but doesn't expose the necessary PWM attributes.

I think we should instead automatically fallback to direct access in that case.

jonasmalacofilho · 2022-09-19T02:51:04Z

liquidctl/driver/aquacomputer.py

+            _LOGGER.warning(
+                "directly writing fixed speed despite %s kernel driver", self._hwmon.driver
+            )


Related to the previous comment, in my opinion a warning is only necessary if we're re-implementing (and possibly conflicting with) something the kernel driver actually supports/exposes.

jonasmalacofilho · 2022-09-19T02:54:42Z

liquidctl/driver/hwmon.py

+    def write_str(self, name, value):
+        (self.path / name).write_text(value)


For symmetry, I think a write_int API would be better, and move the str() conversion within it.

On a related note, it would also probably make sense to rename get_int to read_int, also for symmetry.

Since renaming get_int to read_int touches more drivers than aquacomputer.py, do you want me to create a separate PR right after this one or do it here?

You're right, that would be better.

jonasmalacofilho · 2022-09-19T02:57:10Z

tests/test_aquacomputer.py

-def test_d5next_set_fixed_speeds_not_supported(mockD5NextDevice):
-    with pytest.raises(NotSupportedByDriver):
-        mockD5NextDevice.set_fixed_speed("fan", 42)
+def test_d5next_set_fixed_speeds_directly(mockD5NextDevice):


Similarly to test_d5next_get_status_directly, this should also test whether --direct-access is honored, even if a (complete) hwmon driver is present.

But I do see how this might be getting a bit repetitive/annoying.

jonasmalacofilho · 2022-09-19T02:58:17Z

tests/test_aquacomputer.py

+    assert pump_report.data[0x95:0x98] == [0, 32, 208]  # 0, <8400>
+
+
+def test_d5next_set_fixed_speeds_hwmon(mockD5NextDevice, tmp_path):


It might be wise to also test whether the fallback to direct mode when the PWM attributes are missing.

jonasmalacofilho · 2022-09-19T03:31:21Z

Since crcmod is stable but not actively maintained, let's exceptionally pin its required version to 1.7.

# on setup.cfg, .github/workflows/*.yml, README.md, etc:
crcmod == 1.7

I usually don't do this type of thing in order to avoid issues with distro-supplied packages. But in this case we really don't expect to see a new version, and I'm not super comfortable leaving a highly popular but potentially¹ neglected dependency unpinned.

I don't know the crcmod developers. It's perfectly possible that they are active and super careful, and that crcmod simply hasn't needed any change in the last decade. However, I couldn't immediately assert that's indeed the case, and pinning this one dependency should be nearly painless... after all, it hasn't received any intentional updates in more than a decade. ↩

…ct access if they aren't

aleksamagicka · 2022-09-19T07:52:44Z

Thanks for the review, things you mentioned should be fixed now, except the get_int to read_int conversion.

jonasmalacofilho · 2022-09-21T16:53:46Z

Thanks!

aleksamagicka marked this pull request as ready for review August 29, 2022 07:43

aleksamagicka and others added 9 commits September 18, 2022 12:32

D5 Next PWM support

b9d58bd

Offsets from: aleksamagicka/aquacomputer_d5next-hwmon@5e3ae6f

Update man page to note that D5 Next fans are now controllable

bab92a1

Switch from crccheck to crcmod and add it to readme and setup.cfg

1a76872

Missed adding crcmod in Github workflows

e3f2fc0

Prevent values of over 100% from being accepted

bf5a065

Update protocol docs to explain new feature report

c16516b

Update guide docs for D5 Next

5955e0a

Updated tests for setting fixed speeds for D5 Next

96d1347

Use fan_ctrl_offset when it's already available

15edba3

aleksamagicka force-pushed the d5next-pwm branch from 87852f1 to 15edba3 Compare September 18, 2022 10:34

aleksamagicka and others added 14 commits September 18, 2022 12:54

Split set_fixed_speed into separate functions for direct (implemented…

18ca1ce

…) and hwmon access

Add write_int function to HwmonDevice

ed215fc

Implement passing fixed PWM duty to hwmon

ba4abca

Use clamp function from utils for clamping duty

4bc3831

Added missing comma

e863c16

Fix pwm_duty calculation (it's in centi-percent)

8c70b97

Rename write_int in HwmonDevice to write_str

8b80477

Check if hwmon attributes are available; set channel to direct percen…

1cb3849

…t mode

Explain hwmon usage in D5 Next docs

92d7413

Wrap a very long paragraph in D5 Next docs

4316bfe

Specify hwmon driver name in error message

0dbd8d2

Add test for writing fixed speed through hwmon driver

48e1719

Reformatted tests using black

98f21ba

Pass in duty unchanged and let functions convert it if needed

3f134d8

jonasmalacofilho reviewed Sep 19, 2022

View reviewed changes

aleksamagicka and others added 9 commits September 19, 2022 08:19

Removed extra hwmon note from docs

fa9dbf1

Fix manpage noting D5 Next fan control capabilities

071f715

Convert write_str to write_int

28f8cb5

Fix crcmod version to 1.7

53465d5

Check earlier if hwmon attributes are available and fall back to dire…

656dcd5

…ct access if they aren't

Reformat main driver

3180f3a

Test if direct_access is honored despite hwmon driver being present

5a9bef5

Test that fallback to direct access works in case hwmon is incomplete

aea2085

Swap fan and pump report order to follow pwm1, pwm2

00a8d29

Expand comments regarding PWM support for D5 Next

f43f203

jonasmalacofilho merged commit a81c9b5 into liquidctl:main Sep 21, 2022

aleksamagicka deleted the d5next-pwm branch September 21, 2022 17:47

aleksamagicka mentioned this pull request Sep 21, 2022

Rename get_int to read_int in HwmonDevice #507

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for setting fan speed for D5 Next #499

Add support for setting fan speed for D5 Next #499

aleksamagicka commented Aug 28, 2022 •

edited

Loading

jonasmalacofilho commented Sep 18, 2022

aleksamagicka commented Sep 18, 2022

aleksamagicka commented Sep 18, 2022

jonasmalacofilho left a comment

jonasmalacofilho Sep 19, 2022

jonasmalacofilho Sep 19, 2022

jonasmalacofilho Sep 19, 2022

jonasmalacofilho Sep 19, 2022

jonasmalacofilho Sep 19, 2022

aleksamagicka Sep 19, 2022

jonasmalacofilho Sep 20, 2022

aleksamagicka Sep 21, 2022

jonasmalacofilho Sep 19, 2022

jonasmalacofilho Sep 19, 2022

jonasmalacofilho commented Sep 19, 2022

aleksamagicka commented Sep 19, 2022

jonasmalacofilho commented Sep 21, 2022

	Cooling channels: (D5 Next): supported, (Octo, Quadro:) not yet supported; (Farbwerk 360:) not applicable.
	Cooling channels: (D5 Next): \fIfan\fR, \fIpump\fR; (Octo, Quadro:) not yet supported; (Farbwerk 360:) not applicable.

		def write_str(self, name, value):
		(self.path / name).write_text(value)

		assert pump_report.data[0x95:0x98] == [0, 32, 208] # 0, <8400>


		def test_d5next_set_fixed_speeds_hwmon(mockD5NextDevice, tmp_path):

Add support for setting fan speed for D5 Next #499

Add support for setting fan speed for D5 Next #499

Conversation

aleksamagicka commented Aug 28, 2022 • edited Loading

jonasmalacofilho commented Sep 18, 2022

aleksamagicka commented Sep 18, 2022

aleksamagicka commented Sep 18, 2022

jonasmalacofilho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasmalacofilho commented Sep 19, 2022

Footnotes

aleksamagicka commented Sep 19, 2022

jonasmalacofilho commented Sep 21, 2022

aleksamagicka commented Aug 28, 2022 •

edited

Loading