[v2] HID: uclogic: Add support for XP-PEN Deco LW
Commit Message
The XP-PEN Deco LW is a UGEE v2 device with a frame with 8 buttons.
Its pen has 2 buttons, supports tilt and pressure.
It can be connected by USB cable or using a USB Bluetooth dongle to use
it in wireless mode. When it is connected using the dongle, the device
battery is used to power it.
Its vendor, product and version are identical to the Deco L. The only
difference reported by its firmware is the product name.
In order to add support for battery reporting, add a new HID descriptor
and a quirk to detect the wireless version of the tablet.
Link: https://github.com/DIGImend/digimend-kernel-drivers/issues/635
Tested-by: Mia Kanashi <chad@redpilled.dev>
Tested-by: Andreas Grosse <andig.mail@t-online.de>
Signed-off-by: José Expósito <jose.exposito89@gmail.com>
---
v1 -> v2: https://lore.kernel.org/linux-input/20221028081656.GA19766@elementary/T/
- Barnabás Pőcze: Use strstarts() instead of a custom implementation
---
drivers/hid/hid-uclogic-params.c | 73 ++++++++++++++++++++++++++++++++
drivers/hid/hid-uclogic-rdesc.c | 34 +++++++++++++++
drivers/hid/hid-uclogic-rdesc.h | 7 +++
3 files changed, 114 insertions(+)
Comments
Hello!
Before device sends a battery report, you can notice garbage values in a power supply capacity.
You can see this by running
`watch -n0.1 cat /sys/class/power_supply/hid-28bd-935-battery/uevent`
and then connecting the tablet.
You can set it to a value here, but i think this is a problem in the global hid driver?
Hi Mia,
Thanks for testing :D
On Sat, Oct 29, 2022 at 03:28:10AM +0300, Mia Kanashi wrote:
> Hello!
>
> Before device sends a battery report, you can notice garbage values in a power supply capacity.
>
> You can see this by running
> `watch -n0.1 cat /sys/class/power_supply/hid-28bd-935-battery/uevent`
> and then connecting the tablet.
>
> You can set it to a value here, but i think this is a problem in the global hid driver?
That shouldn't be problematic, because the charging status should be
initially set to unknown and change to charging/discharging [1] once
the device sends the first report.
Your desktop environment shouldn't display the battery percentage
while the status is unknown, so the initial values are ignored.
Here is my output of my XP-Pen Deco SW before receiving the battery
percentage:
POWER_SUPPLY_NAME=hid-28bd-933-battery
POWER_SUPPLY_TYPE=Battery
POWER_SUPPLY_PRESENT=1
POWER_SUPPLY_ONLINE=1
POWER_SUPPLY_MODEL_NAME=UGTABLET Deco Pro SW
POWER_SUPPLY_SCOPE=Device
And after:
POWER_SUPPLY_NAME=hid-28bd-933-battery
POWER_SUPPLY_TYPE=Battery
POWER_SUPPLY_PRESENT=1
POWER_SUPPLY_ONLINE=1
POWER_SUPPLY_CAPACITY=99
POWER_SUPPLY_MODEL_NAME=UGTABLET Deco Pro SW
POWER_SUPPLY_STATUS=Discharging
POWER_SUPPLY_SCOPE=Device
Jose
[1] Actually it should be set to discharging until this gets merged:
https://lore.kernel.org/linux-input/20221028181849.23157-1-jose.exposito89@gmail.com/T/
On 29 October 2022 10:58:32 EEST, "José Expósito" <jose.exposito89@gmail.com> wrote:
>Hi Mia,
>
>Thanks for testing :D
>
>On Sat, Oct 29, 2022 at 03:28:10AM +0300, Mia Kanashi wrote:
>> Hello!
>>
>> Before device sends a battery report, you can notice garbage values in a power supply capacity.
>>
>> You can see this by running
>> `watch -n0.1 cat /sys/class/power_supply/hid-28bd-935-battery/uevent`
>> and then connecting the tablet.
>>
>> You can set it to a value here, but i think this is a problem in the global hid driver?
>
>That shouldn't be problematic, because the charging status should be
>initially set to unknown and change to charging/discharging [1] once
>the device sends the first report.
>
>Your desktop environment shouldn't display the battery percentage
>while the status is unknown, so the initial values are ignored.
>
>Here is my output of my XP-Pen Deco SW before receiving the battery
>percentage:
>
> POWER_SUPPLY_NAME=hid-28bd-933-battery
> POWER_SUPPLY_TYPE=Battery
> POWER_SUPPLY_PRESENT=1
> POWER_SUPPLY_ONLINE=1
> POWER_SUPPLY_MODEL_NAME=UGTABLET Deco Pro SW
> POWER_SUPPLY_SCOPE=Device
>
>And after:
>
> POWER_SUPPLY_NAME=hid-28bd-933-battery
> POWER_SUPPLY_TYPE=Battery
> POWER_SUPPLY_PRESENT=1
> POWER_SUPPLY_ONLINE=1
> POWER_SUPPLY_CAPACITY=99
> POWER_SUPPLY_MODEL_NAME=UGTABLET Deco Pro SW
> POWER_SUPPLY_STATUS=Discharging
> POWER_SUPPLY_SCOPE=Device
>
>Jose
>
>[1] Actually it should be set to discharging until this gets merged:
> https://lore.kernel.org/linux-input/20221028181849.23157-1-jose.exposito89@gmail.com/T/
Hello, for me it first looks like this:
POWER_SUPPLY_NAME=hid-28bd-935-battery
POWER_SUPPLY_TYPE=Battery
POWER_SUPPLY_PRESENT=1
POWER_SUPPLY_ONLINE=1
POWER_SUPPLY_MODEL_NAME=Hanvon Ugee Deco LW
POWER_SUPPLY_SCOPE=Device
Then it looks like this for a very brief (or not so brief) period of time, it also isn't always there, i suggest trying plugging it in multiple times:
POWER_SUPPLY_NAME=hid-28bd-935-battery
POWER_SUPPLY_TYPE=Battery
POWER_SUPPLY_PRESENT=1
POWER_SUPPLY_ONLINE=1
POWER_SUPPLY_CAPACITY=160
POWER_SUPPLY_MODEL_NAME=Hanvon Ugee Deco LW
POWER_SUPPLY_STATUS=Discharging
POWER_SUPPLY_SCOPE=Device
Here value is 160, obvious garbage, it can have any other value depending on your luck.
And then after that it looks like this:
POWER_SUPPLY_NAME=hid-28bd-935-battery
POWER_SUPPLY_TYPE=Battery
POWER_SUPPLY_PRESENT=1
POWER_SUPPLY_ONLINE=1
POWER_SUPPLY_CAPACITY=40
POWER_SUPPLY_MODEL_NAME=Hanvon Ugee Deco LW
POWER_SUPPLY_STATUS=Discharging
POWER_SUPPLY_SCOPE=Device
>[1] Actually it should be set to discharging until this gets merged:
> https://lore.kernel.org/linux-input/20221028181849.23157-1-jose.exposito89@gmail.com/T/
But i also currently applied this ^ patch, i will try testing without it and then report.
>>[1] Actually it should be set to discharging until this gets merged:
>> https://lore.kernel.org/linux-input/20221028181849.23157-1-jose.exposito89@gmail.com/T/
>
>But i also currently applied this ^ patch, i will try testing without it and then report.
Tested without it, same issue.
So yeah it seems that hid-input driver can set supply status to discharging before setting a battery capacity?
Hi!
On Sat, Oct 29, 2022 at 04:55:21PM +0300, Mia Kanashi wrote:
> >>[1] Actually it should be set to discharging until this gets merged:
> >> https://lore.kernel.org/linux-input/20221028181849.23157-1-jose.exposito89@gmail.com/T/
> >
> >But i also currently applied this ^ patch, i will try testing without it and then report.
>
> Tested without it, same issue.
> So yeah it seems that hid-input driver can set supply status to discharging before setting a battery capacity?
Very good catch. I managed to reproduce it using the USB dongle. I
didn't notice it before because I was running upower after connecting
the device, which isn't fast enough. However, using watch as you
suggested makes the issue pretty noticeable.
The problem is that the battery is fetched when the USB dongle is
connected. However, the tablet might not be paired at that point.
To explain it with the actual code:
if (dev->battery_status != HID_BATTERY_REPORTED &&
!dev->battery_avoid_query) {
value = hidinput_query_battery_capacity(dev);
^ Here the battery is fetched, but because the tabled
is not paired and this function returns garbage
if (value < 0)
return value;
dev->battery_capacity = value;
dev->battery_status = HID_BATTERY_QUERIED;
^ Now the battery is set as queried
}
if (dev->battery_status == HID_BATTERY_UNKNOWN)
val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
else
val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
^ And therefore the battery is reported
Thankfully, there is already a flag (battery_avoid_query) used to solve
the same issue on stylus. The battery percentage is unknown until the
stylus is in proximity.
So I guess I could use the same flag to avoid this problem.
I'll add a fix in a second revision of this patch.
Thanks!!
>The problem is that the battery is fetched when the USB dongle is
>connected. However, the tablet might not be paired at that point.
Big thanks for the explanation, i've looked at this code before but couldn't see exactly what's wrong.
On Sat, 2022-10-29 at 16:55 +0200, José Expósito wrote:
> Hi!
>
> On Sat, Oct 29, 2022 at 04:55:21PM +0300, Mia Kanashi wrote:
> > > > [1] Actually it should be set to discharging until this gets
> > > > merged:
> > > >
> > > > https://lore.kernel.org/linux-input/20221028181849.23157-1-jose.exposito89@gmail.com/T/
> > >
> > > But i also currently applied this ^ patch, i will try testing
> > > without it and then report.
> >
> > Tested without it, same issue.
> > So yeah it seems that hid-input driver can set supply status to
> > discharging before setting a battery capacity?
>
> Very good catch. I managed to reproduce it using the USB dongle. I
> didn't notice it before because I was running upower after connecting
> the device, which isn't fast enough. However, using watch as you
> suggested makes the issue pretty noticeable.
>
> The problem is that the battery is fetched when the USB dongle is
> connected. However, the tablet might not be paired at that point.
>
> To explain it with the actual code:
>
>
> if (dev->battery_status != HID_BATTERY_REPORTED &&
> !dev->battery_avoid_query) {
> value = hidinput_query_battery_capacity(dev);
> ^ Here the battery is fetched, but because the tabled
> is not paired and this function returns garbage
> if (value < 0)
> return value;
>
> dev->battery_capacity = value;
> dev->battery_status = HID_BATTERY_QUERIED;
> ^ Now the battery is set as queried
> }
>
> if (dev->battery_status == HID_BATTERY_UNKNOWN)
> val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
> else
> val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
> ^ And therefore the battery is reported
>
>
> Thankfully, there is already a flag (battery_avoid_query) used to
> solve
> the same issue on stylus. The battery percentage is unknown until the
> stylus is in proximity.
>
> So I guess I could use the same flag to avoid this problem.
>
> I'll add a fix in a second revision of this patch.
UPower will also respect the POWER_SUPPLY_PROP_PRESENT property, if
that's useful.
@@ -18,6 +18,7 @@
#include "usbhid/usbhid.h"
#include "hid-ids.h"
#include <linux/ctype.h>
+#include <linux/string.h>
#include <asm/unaligned.h>
/**
@@ -1211,6 +1212,69 @@ static int uclogic_params_ugee_v2_init_frame_mouse(struct uclogic_params *p)
return rc;
}
+/**
+ * uclogic_params_ugee_v2_has_battery() - check whether a UGEE v2 device has
+ * battery or not.
+ * @hdev: The HID device of the tablet interface.
+ *
+ * Returns:
+ * True if the device has battery, false otherwise.
+ */
+static bool uclogic_params_ugee_v2_has_battery(struct hid_device *hdev)
+{
+ /* The XP-PEN Deco LW vendor, product and version are identical to the
+ * Deco L. The only difference reported by their firmware is the product
+ * name. Add a quirk to support battery reporting on the wireless
+ * version.
+ */
+ if (hdev->vendor == USB_VENDOR_ID_UGEE &&
+ hdev->product == USB_DEVICE_ID_UGEE_XPPEN_TABLET_DECO_L) {
+ struct usb_device *udev = hid_to_usb_dev(hdev);
+
+ if (strstarts(udev->product, "Deco LW"))
+ return true;
+ }
+
+ return false;
+}
+
+/**
+ * uclogic_params_ugee_v2_init_battery() - initialize UGEE v2 battery reporting.
+ * @hdev: The HID device of the tablet interface, cannot be NULL.
+ * @p: Parameters to fill in, cannot be NULL.
+ *
+ * Returns:
+ * Zero, if successful. A negative errno code on error.
+ */
+static int uclogic_params_ugee_v2_init_battery(struct hid_device *hdev,
+ struct uclogic_params *p)
+{
+ int rc = 0;
+
+ if (!hdev || !p)
+ return -EINVAL;
+
+ /* Some tablets contain invalid characters in hdev->uniq, throwing a
+ * "hwmon: '<name>' is not a valid name attribute, please fix" error.
+ * Use the device vendor and product IDs instead.
+ */
+ snprintf(hdev->uniq, sizeof(hdev->uniq), "%x-%x", hdev->vendor,
+ hdev->product);
+
+ rc = uclogic_params_frame_init_with_desc(&p->frame_list[1],
+ uclogic_rdesc_ugee_v2_battery_template_arr,
+ uclogic_rdesc_ugee_v2_battery_template_size,
+ UCLOGIC_RDESC_UGEE_V2_BATTERY_ID);
+ if (rc)
+ return rc;
+
+ p->frame_list[1].suffix = "Battery";
+ p->pen.subreport_list[1].value = 0xf2;
+ p->pen.subreport_list[1].id = UCLOGIC_RDESC_UGEE_V2_BATTERY_ID;
+
+ return rc;
+}
+
/**
* uclogic_params_ugee_v2_init() - initialize a UGEE graphics tablets by
* discovering their parameters.
@@ -1334,6 +1398,15 @@ static int uclogic_params_ugee_v2_init(struct uclogic_params *params,
if (rc)
goto cleanup;
+ /* Initialize the battery interface*/
+ if (uclogic_params_ugee_v2_has_battery(hdev)) {
+ rc = uclogic_params_ugee_v2_init_battery(hdev, &p);
+ if (rc) {
+ hid_err(hdev, "error initializing battery: %d\n", rc);
+ goto cleanup;
+ }
+ }
+
output:
/* Output parameters */
memcpy(params, &p, sizeof(*params));
@@ -1035,6 +1035,40 @@ const __u8 uclogic_rdesc_ugee_v2_frame_mouse_template_arr[] = {
const size_t uclogic_rdesc_ugee_v2_frame_mouse_template_size =
sizeof(uclogic_rdesc_ugee_v2_frame_mouse_template_arr);
+/* Fixed report descriptor template for UGEE v2 battery reports */
+const __u8 uclogic_rdesc_ugee_v2_battery_template_arr[] = {
+ 0x05, 0x01, /* Usage Page (Desktop), */
+ 0x09, 0x07, /* Usage (Keypad), */
+ 0xA1, 0x01, /* Collection (Application), */
+ 0x85, UCLOGIC_RDESC_UGEE_V2_BATTERY_ID,
+ /* Report ID, */
+ 0x75, 0x08, /* Report Size (8), */
+ 0x95, 0x02, /* Report Count (2), */
+ 0x81, 0x01, /* Input (Constant), */
+ 0x05, 0x84, /* Usage Page (Power Device), */
+ 0x05, 0x85, /* Usage Page (Battery System), */
+ 0x09, 0x65, /* Usage Page (AbsoluteStateOfCharge), */
+ 0x75, 0x08, /* Report Size (8), */
+ 0x95, 0x01, /* Report Count (1), */
+ 0x15, 0x00, /* Logical Minimum (0), */
+ 0x26, 0xff, 0x00, /* Logical Maximum (255), */
+ 0x81, 0x02, /* Input (Variable), */
+ 0x75, 0x01, /* Report Size (1), */
+ 0x95, 0x01, /* Report Count (1), */
+ 0x15, 0x00, /* Logical Minimum (0), */
+ 0x25, 0x01, /* Logical Maximum (1), */
+ 0x09, 0x44, /* Usage Page (Charging), */
+ 0x81, 0x02, /* Input (Variable), */
+ 0x95, 0x07, /* Report Count (7), */
+ 0x81, 0x01, /* Input (Constant), */
+ 0x75, 0x08, /* Report Size (8), */
+ 0x95, 0x07, /* Report Count (7), */
+ 0x81, 0x01, /* Input (Constant), */
+ 0xC0 /* End Collection */
+};
+const size_t uclogic_rdesc_ugee_v2_battery_template_size =
+ sizeof(uclogic_rdesc_ugee_v2_battery_template_arr);
+
/* Fixed report descriptor for Ugee EX07 frame */
const __u8 uclogic_rdesc_ugee_ex07_frame_arr[] = {
0x05, 0x01, /* Usage Page (Desktop), */
@@ -161,6 +161,9 @@ extern const size_t uclogic_rdesc_v2_frame_dial_size;
/* Device ID byte offset in v2 frame dial reports */
#define UCLOGIC_RDESC_V2_FRAME_DIAL_DEV_ID_BYTE 0x4
+/* Report ID for tweaked UGEE v2 battery reports */
+#define UCLOGIC_RDESC_UGEE_V2_BATTERY_ID 0xba
+
/* Fixed report descriptor template for UGEE v2 pen reports */
extern const __u8 uclogic_rdesc_ugee_v2_pen_template_arr[];
extern const size_t uclogic_rdesc_ugee_v2_pen_template_size;
@@ -177,6 +180,10 @@ extern const size_t uclogic_rdesc_ugee_v2_frame_dial_template_size;
extern const __u8 uclogic_rdesc_ugee_v2_frame_mouse_template_arr[];
extern const size_t uclogic_rdesc_ugee_v2_frame_mouse_template_size;
+/* Fixed report descriptor template for UGEE v2 battery reports */
+extern const __u8 uclogic_rdesc_ugee_v2_battery_template_arr[];
+extern const size_t uclogic_rdesc_ugee_v2_battery_template_size;
+
/* Fixed report descriptor for Ugee EX07 frame */
extern const __u8 uclogic_rdesc_ugee_ex07_frame_arr[];
extern const size_t uclogic_rdesc_ugee_ex07_frame_size;