Basic Leakshield integration

[main--]

Based on the discussion in #37 and the reverse-engineering work by @looncraz, here is a very basic integration with Leakshield. Right now it only reads the pressure value. I tried to keep the implementation very similar to highflownext, so the mbar pressure value is reported as fan RPM right now. The value is also divided by 10 because the device reports 10x mbar. This information loss is unfortunate of course, but I personally feel that since the decimal digit fluctuates a lot anyway, the usability benefit of dropping it outweighs the very minor loss of accuracy here. I wish there was a better way to report this though. Perhaps (ab)using the voltage or temperature sensor types would be a better fit here since they offer a decimal point?

[aleksamagicka]

Thanks!

In the rest of the driver on such occasions the third digit is included and the unit is denoted in the label (for example, 7e1235e for flow rate or 4796714 for conductivity). New data is read every two seconds anyway, so it's not jumping around a lot. Aside from that, code looks fine to me.

There are no float values in this driver, the 25.5 you can for example see as a temperature from sensors is actually 25500 internally - same with voltages etc. As far as I'm aware, FPU operations in the kernel are only available on special occasions, on some architectures, and require being inbetween kernel_fpu_begin() and kernel_fpu_end(), so that's only for some special code. Besides, all Aquacomputer devices report the values as non float numbers. And no, we can't abuse temp or voltage inputs for this kind of stuff, it would never be accepted into mainline :) RPMs are fine for that, though, as long as they're documented.

[main--]

Thank you for the fast response. What I meant is that given a value of e.g. 345.6 mbar, instead of dividing the raw value by 10 (to produce 345), I could just multiply by 100 (to produce 345600) and return that as e.g. a temperature value. That should cause sensors to display the correct value (345.6) with no accuracy loss (and of course no floating point operations in the kernel), right?

To be honest I don't understand why RPM specifically should be blessed as the unit that is allowed represent arbitrary other units (especially given that it can't represent fixed-point decimals, which temperatures and voltages apparently can). But if I understand correctly, then this rule is set by the kernel maintainers?

As I explained above, I would really love a solution here which doesn't require the user to divide by ten when looking at the data. But if there's no way to make it work under the current rules, I completely understand of course.

[aleksamagicka]

RPM is kind of the catch-all that's accepted in mainline for these kinds of readings (like highflownext, that was accepted with no comments). Temp, voltage, current are all more specific and we can't pass in pressure readings or something else to them. The thing with RPM is that it leaves the value untouched by formatting. That leaves us with the following:

• Divide by 10 and mark as mbar. A decimal place is lost, but the reading is in milli units already and at that level we can probably bring into consideration the sensitivity of the sensor(s), so the loss isn't as big as, say, showing only L/h when dL/h is available.
• Instead of dividing, multiply by 100 and get microbar (μ), so that 345.6mbar becomes 345600microbar and you retain the decimal place.

I'll leave it you to decide which one you want to implement here, I'm OK with both.

[looncraz]

I, for one, would prefer µbar over a truncated mbar.  The sensors on the Leakshield are quite accurate and you can watch as the tenths change in response to temperature change or smaller leaks, allowing you to do more with the information. There really needs to be a truly generic value type to handle these scenarios given how common they seem to be, but that's a battle for another forum. My pressure is currently 360.2~360.3 mbar, it has dropped from 360.6~360.8 this morning. Truncating that decimal point would have just shown a stagnant number all day, but I can see that it's dropped slowly as the water warmed up or a little air managed to get in.. information that's particularly welcome, if not technically strictly needed at this point. Thanks!

…

--The loon

On 06/10/2022 11.42, Aleksa Savić wrote: RPM is kind of the catch-all that's accepted in mainline for these kinds of readings (like highflownext, that was accepted with no comments). Temp, voltage, current are all more specific and we can't pass in pressure readings or something else to them. The thing with RPM is that it leaves the value untouched by formatting. That leaves us with the following: * Divide by 10 and mark as mbar. A decimal place is lost, but the reading is in milli units already and at that level we can probably bring into consideration the sensitivity of the sensor(s), so the loss isn't as big as, say, showing only L/h when dL/h is available. * Instead of dividing, multiply by 100 and get microbar (μ), so that 345.6mbar becomes 345600microbar and you retain the decimal place. I'll leave it you to decide which one you want to implement here, I'm OK with both. — Reply to this email directly, view it on GitHub <#41 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEZLK7VI777HZ5B7GK4KFZ3WB36OTANCNFSM6AAAAAAQ6ADGAI>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[aleksamagicka]

Thanks for the info, then I'd endorse the second option as well. It's great to hear that the sensor is reliable, I don't have a leakshield so it reminded me of flow sensors on other devices, which aren't particularly accurate as far as I understand.

[looncraz]

There are two sensors uses for the pressure, they appear to be combined and adjusted to create the single pressure value:     Press 1      : 380.20 mbar     Press 2      : 380.15 mbar     Average      : 380.1  mbar     Correction   : -19.8  mbar     Adjusted     : 360.3  mbar     Temperature  : 31.36 C     Press Change :  0.0 ml/min     Volume change:    2 ml/hour     Volume change:    2 ml/hour (two entries)     Filled       : 293 ml     Reservoir    : 490 ml So the pressure reading is (Press_1 + Press_2 + Correction + Correction ) / 2 and is then truncated (well, probably done in pure integer, so every * 100) (38020 + 38015 + -1980 + -1980) / 2 = 36037.5 = 360.3 I've seen the Average - Correction not line up every time, but this formula seems to explain the values every time. With access to the data, I would saw we could provide one extra decimal place that the Adjusted field doesn't have. Just my thoughts :p .. the temperature, BTW, aligns decently well with my coolant temperature... typically within 1.5C, currently reading higher by 1.2C.

…

--The loon

On 06/10/2022 12.02, Aleksa Savić wrote: Thanks for the info, then I'd endorse the second option as well. It's great to hear that the sensor is reliable, I don't have a leakshield so it reminded me of flow sensors on other devices, which aren't particularly accurate as far as I understand. — Reply to this email directly, view it on GitHub <#41 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEZLK7VV5B2RKOIARNUI2JDWB4AZFANCNFSM6AAAAAAQ6ADGAI>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[main--]

Based on the feedback here I switched the values to µbar. It's ugly but I guess it will have to do. I also added the two temperature sensors, reservoir fill measurements, and min/target/max pressure readings.

I also added pump speed and flow values which are normally supposed to be sent by the Aquacomputer Service to the Leakshield for a more accurate calculation of the minimum and target pressure. I have reverse-engineered the USB packet that is used to transmit this data, but right now it's just a python script. I feel like it would be cool to offer a write interface (i.e. aqc_write) for these two values, so that a userland daemon could read these values from a pump and a flow sensor and then feed them back into Leakshield, both through hwmon. What do you think?

The main issue I can see is that it uses a bulk transfer on endpoint 2, while everything else runs through HID on endpoint 1. I'm not sure how to approach this, since it seems to me like the kernel currently creates a separate instance of the entire driver for individual device endpoints. But I may be wrong, since I'm not very familiar with USB.

[aleksamagicka]

Can you please share the Python script for USB transfers? On other devices, aquasuite also uses bulk transfers for setting software temp sensor values, which is something I've been meaning to look into.

[main--]

Sure!

import usb.core
import crcmod.predefined
cf = crcmod.predefined.mkCrcFun('crc-16-usb')

dev = usb.core.find(idVendor=0x0c70, idProduct=0xf014)
# interesting: don't need to detach kernel driver for this

# 04 - unknown
# 12 00 - pump rpm
# 03 e9 - flow in dL/h
# rest is unknown. maybe slots for virtual sensor inputs?
# 7fff always means n/a
b = bytes.fromhex('04120003e97fff7fff7fff7fff7fff7fff7fff7fff7fff7fff7fff7fff7fff7fff03000000000000000000000000000000')
# append checksum
bb = b + cf(b).to_bytes(2, 'big')
dev.write(2, bb)

[aleksamagicka]

Thanks. The kernel driver does not need to be detached because it supports hidraw for raw access. (That's how the aquacomputer driver in liquidctl is able to bypass it.)

7FFF makes for one temp sensor with an unknown value, so the sensors start with the first 7FFF. That can be documented, as we don't expose that functionality yet. Speaking of it, I found these functions in an old driver for sending USB reports:

/* Unlock the urb so we can reuse it */
static void g110_ep1_urb_completion(struct urb *urb)
{
	struct hid_device *hdev = urb->context;
	struct g110_data *data = hid_get_g110data(hdev);
	struct input_dev *idev = data->input_dev;
	int i;

	for (i = 0; i < 8; i++)
		g110_handle_key_event(data, idev, 24+i, data->ep1keys[0]&(1<<i));

	input_sync(idev);

	usb_submit_urb(urb, GFP_ATOMIC);
}

static int g110_ep1_read(struct hid_device *hdev)
{
	struct usb_interface *intf;
	struct usb_device *usb_dev;
	struct g110_data *data = hid_get_g110data(hdev);

	struct usb_host_endpoint *ep;
	unsigned int pipe;
	int retval = 0;

	/* Get the usb device to send the image on */
	intf = to_usb_interface(hdev->dev.parent);
	usb_dev = interface_to_usbdev(intf);

	pipe = usb_rcvintpipe(usb_dev, 0x01);
	ep = (usb_pipein(pipe) ? usb_dev->ep_in : usb_dev->ep_out)[usb_pipeendpoint(pipe)];

	if (unlikely(!ep))
		return -EINVAL;

	usb_fill_int_urb(data->ep1_urb, usb_dev, pipe, data->ep1keys, 2,
			 g110_ep1_urb_completion, NULL, 10);
	data->ep1_urb->context = hdev;
	data->ep1_urb->actual_length = 0;

	retval = usb_submit_urb(data->ep1_urb, GFP_KERNEL);

	return retval;
}

That driver is for some Logitech keyboards (g110 references), but it's getting a usb_dev reference from hdev, which we can do as well. Then, it creates a pipe (we'd probably use usb_sndbulkpipe()) and instead of creating an interrupt URB, usb_fill_bulk_urb() seems like the way to go.

This code is from 12 years ago, and looks like some stuff changed in the meantime - instead of setting the ep1_urb->context manually, it can be passed in to usb_fill_bulk_urb() and retrieved in the completion callback. Since all this would presumably be triggered from aqc_write(), we'd need to have a wait_for_completion() call there to wait until the write is complete, as usb_submit_urb() returns immediately.

Regarding the virtual temp sensors (generally, not only with the Leakshield): there's a way to read them from the usual sensor reports (you can try to find their offset there), and when setting one of them (which should work the same as for Leakshield, without the preceding special parameters), the rest must be preserved and reconstructed from before. Those temp sensors expire after 10-15 seconds, but if a user or userspace app is setting them one by one through the driver, this needs to be done. (This is for later.)

[main--]

Thanks a lot for your help. I have switched the implementation over to fan_min/max/target. I also managed to send the USB updates. It turns out that usb_bulk_msg really simplifies the whole process a lot.

During my testing, it seems to work fine! The only weird part in my current implementation is that if you set e.g. a flow rate once and then never again, every subsequent write to the pump RPM will repeatedly send your old flow rate to the device (and vice versa). And also it's obviously not perfect that if you want to just send both values, two different USB packets will be sent. I'm not sure if there is an easy fix for this; it seems acceptable for now.

Another comment I would make is that sensors currently unfortunately does not surface target RPM. However, the target RPM value appears to be informational only (i.e. does not trigger any actions like min/max do) so this also seems acceptable to me.

[aleksamagicka]

Great work! Thanks to you, we have that figured out in the driver now. Setting virt temp sensors is done similarly, for which now there is a reference on how to implement. Some comments regarding the code are enclosed, but nothing major.

The flow rate being stale is not our problem, it's up to userspace to supply it. Sending two packets is OK, we're not spamming the device or anything, and if userspace wants to send one report, they are free to do so without the driver.

Didn't know that about target RPM, but it does make sense - it's intended for giving a setpoint to the device and it's not very important that it's shown (unlike real RPM/value to compare).

[aleksamagicka]

LGTM! I'll resolve the formatting (just running it through Lindent).

[hp-pepster]

Hi, thank you all for your great work. I'm using your driver build from git but somehow the value for the flow rate from the highflownext does not seem to work properly if I pass it to the leakshield.
I read the value in dL/h, e.g. 700. If I pass that value to the leakshield by writing that value to /fan3_input, but the value for the target pressure does not change and the small display on the leakshield shows only 700 for the flow rate.

If I use the aquasuite via a VM to feed the value, the target pressure changes. The leakshield also prints the flow rate in the small display correctly, like 70 l/h. Am I doing something wrong or is this maybe a "feature" in the driver?

[aleksamagicka]

I don't have a Leakshield myself, so I unfortunately can't be of much help in direct testing. #41 (comment) specifies an example of the USB report that's being sent to the Leakshield; there it says that the flow value should indeed be in dL/h, which concides with how other Aquacomputer devices represent that.

Can you perhaps capture the packets sent to the device when changing stuff in Aquasuite? You'd be looking for an USB bulk transfer on endpoint 2 whose contents start with 04, and some that follow if they're relevant. From there you can see what Aquasuite is generating.

Edit: or try writing 7000 and other variations to fan3_input, and see what happens.

[main--]

I can confirm that the value is shown on the display without a unit. This is indeed weird and I missed that during my testing. However, I do observe the minimum pressure changing in response to me setting different flow values, so it seems like something is at least working. 🤷

I don't understand what the missing piece here could be, given that the packet I captured (in the comment above) was sent directly by aquasuite and did result in the device correctly displaying the flow rate (i.e. with units).

[aleksamagicka]

Are there more packets being sent by Aquasuite before/after this one? Asking because for the fan controllers (d5 next, octo, etc.) after changing something aquasuite is sending a second HID report, always with the same value. Everything seems to work fine without it, but we send it in the driver anyway. Maybe they're sending something else here as well?

[hp-pepster]

I'm also sending the pump speed to the leakshield, which leads to a way lower target pressure, do you send that as well?
Maybe some of my values are of interest without Aquasuit.

Pump speed: ~ 2590 rpm
Flow rate: ~ 690 dl/h
Minimum pressure: 107 mbar
Target pressure: 157 mbar
Current pressure: ~259 mbar
Coolant temperature: 29.5 °C

@aleksamagicka I never captured living USB packets but I will try my best.

[main--]

Just looked back at my packet captures and found a detail that I had missed earlier! It turns out that byte 34 is actually set to 0x0C when a flow rate is sent (and 0 otherwise). I will send a PR to fix this.

[aleksamagicka]

@main--: Great! Does that make it show the flow rate correctly?

@hp-pepster: If the mentioned byte is what needs to be set, you won't need to (for the flow rate issue). For reference, here's a nice tutorial for that.

[hp-pepster]

@aleksamagicka thank you for the link, maybe I can contribute more to this project if I can capture some data from the aquacomputer devices. Just let me know if I can help. I've got a leakshield, a d5next and a highflownext. If someone is interested, I use python + mariadb + grafana for the data visualisation.
@main-- good finding, thank you for your effort.

[aleksamagicka]

@aleksamagicka thank you for the link, maybe I can contribute more to this project if I can capture some data from the aquacomputer devices. Just let me know if I can help. I've got a leakshield, a d5next and a highflownext.

Since you have a Leakshield and a High Flow Next, perhaps you can take a look at #26 and #27. No pressure, of course, if you want to try and get stuck somewhere, I'll help as much as I can.

[aleksamagicka]

@main--, I'd like to prepare a patch for adding support for Leakshield sensors to the mainline kernel. Can you please write your name and email so I can credit you in the patch (and future ones regarding the Leakshield with your code)?

For an example of how it's going to be used, please take a look at the Originally-from: tag in one of the previous patches I've sent.

[main--]

Please credit me as: Noah Bergbauer <[email protected]>

[aleksamagicka]

Thank you! You'll be CCed on the patches when I send them.