DIGImend / digimend-kernel-drivers

DIGImend graphics tablet drivers for the Linux kernel
GNU General Public License v2.0
1.17k stars 173 forks source link

add support for Huion Inspiroy 2 #679

Closed tequeter closed 5 months ago

tequeter commented 7 months ago

This adds support for the Inspiroy 2 series, M (H951P) and L (H1061P).

Everything works, but you'll need some userland setup to make the group keys actually change the mappings of the other buttons and wheel.

I've got the product ID of the M version from OpenTabletDriver/OpenTabletDriver#2927, but it would be nice to get confirmation that it actually works. Likewise, support for the S version would be easy to add if someone could provide the product ID.

Closes #676.

tequeter commented 7 months ago

@demigirlboss, could you try out my branch and report if it works for your H951P?

Thanks!

Pixaurora commented 7 months ago

I had some trouble installing it (ultimately I had to run modprobe hid_uclogic after installing it, but didn't know what name to use), but I can confirm that it works with my H951P that I received in the mail today! Thank you for your contribution

JoseExposito commented 5 months ago

Thanks a lot @tequeter, I squashed and merged your patches and they'll be available in v13.

By the way, libwacom (the library used by KDE and GNOME settings) has a concept of "NumModes" for rings, dials and touch strips: https://github.com/linuxwacom/libwacom/blob/master/data/wacom.example#L207-L234

You could do something similar with buttons to fully support you tablet without additional hacks and directly from system settings. You might need to add some KDE or GNOME code as well.

tequeter commented 5 months ago

Now, this is great news :-) Thank you!

NumModes: oh, interesting! I added it to my notes and will investigate that whenever I get to revisit my setup.

JoseExposito commented 5 months ago

Yes, you'll need to add some code to libwacom to get them working of course, but I think that's the right place. I'll suggest to ask the maintainers before implementing it. They are very responsive and will help you to understand what's needed and if libwacom is the right place.

I think there is a problem with your patch though. The descriptor used for the group buttons uses the same IDs that are used for the main button. Unless I didn't understand which interface is sending which buttons, I believe you have two BTN_0, two BTN_1, etc. Sorry for not noticing it earlier :S

You can test it with sudo libinput record. In order for libwacom to know if you pressed the main buttons or the button group, you'll need to assign them different codes.

tequeter commented 5 months ago

Oh! In my current hackish script, I was relying on the input device. I.e.

xsetwacom --set 'HUION Huion Tablet_H1061P Group Buttons pad' Button 1 '...'
vs.
xsetwacom --set 'HUION Huion Tablet_H1061P Pad pad' Button  8 '...'

Are you saying I won't be able to filter by device when I'll try to integrate with the NumModes feature?

The DeviceMatch in wacom.example seems to be able to do that, but then the modes defined in the Group Buttons device might not influence the Pad buttons?

Do note that the tablet is sending non-overlapping values:

That could change at the whim of the manufacturer though, so I preferred to send the new 0xe3 subreport through that new "Group Buttons" device.

Here are the libinput recordings, for reference: libinput-record-group-buttons.txt libinput-record-pad-buttons.txt

NB: I forgot most of what I learnt to write this patch in the three months that elapsed since then, so bear with me :-)

JoseExposito commented 4 months ago

Ah! Forget about what I said, the recording is 100% correct, there is no need to make any change. I was worried about button overlapping but the tablet sends the right values.

whot commented 4 months ago

Are you saying I won't be able to filter by device when I'll try to integrate with the NumModes feature?

The NumModes in libwacom merely says "this device supports N hardware modes". This was implemented ~12y ago for the Wacom Intuos 4/5 series. Those have a button inside the ring and pressing the button will toggle the LED that signals which mode you're in. That feature has been present on a few devices since, including the Cintiq 24HD devices which had two independent sets of mode buttons - one left, one right. And, iirc, had 3 buttons so specificially switch to mode 1/2/3 as opposed to cycling through.

Other than that it's just a software feature - libinput exposes "pad mode groups" and you can query those to figure out if a ring/dial/strip/button is a member of those. And it'll tell you which button is the mode switch button, all that comes from libwacom. But events-wise, nothing happens at that level, it's handled in the compositor (e.g. mutter) which will e.g. assign different events to the ring based on the current mode.

The kernel toggles the LED for us, that's sort-of hardcoded in the wacom driver, mostly because any other approach was way too complicated for near-zero gain.

But anyway, libinput works so that it basically says "pad button event for button 2!" and then you can call libinput_event_tablet_pad_get_mode_group() for that event and you can then decide "well, button 2 in mode 4 means blah, so I'm going to do blah". That's all in the compositor, where it's implemented of course.

tequeter commented 4 months ago

Wow, thanks a lot for the detailed explanation!

tequeter commented 4 months ago

Re: your comment in #690

I admit from the quick hacking I did I'm not 100% how the group mode buttons device is supposed to work but my gut feeling (having implemented this for wacom devices 10y ago) is that having a separate kernel device here is not ideal.

The H1061P sends this data (as per a usbmon capture):

G1 - 08 e3 01 01 01 00
G2 - 08 e3 01 01 02 00
G3 - 08 e3 01 01 04 00
P1 - 08 e0 01 01 08 00
P2 - 08 e0 01 01 10 00
P3 - 08 e0 01 01 20 00
P4 - 08 e0 01 01 40 00
P5 - 08 e0 01 01 80 00
P6 - 08 e0 01 01 00 01
P7 - 08 e0 01 01 00 02
P8 - 08 e0 01 01 00 04

(G - group button pressed, P - regular pad button pressed.)

On this device and firmware, they could all be sent through the Pad device, as they are fortunately non-overlapping.

This seemed a fragile assumption though, so I opted to route the contents of the 0xe3 subreport through a dedicated "Group Buttons" input device.

My patch adds it to all tablets that fall in the uclogic_params_pen_init_v2() case, and not just the Inspiroy 2 ones, because:

  1. It'll work out of the box for the future Huion devices that send extra buttons through that 0xe3 subreport.
  2. That code already unconditionally exposes Touch Strip/Ring and Dial devices (which, I assumed, was done with the same rationale).

I'm not sure if it answers your question, though. Can you expand on what seems wrong?

whot commented 4 months ago

I'm not sure if it answers your question, though. Can you expand on what seems wrong?

There's nothing objectively wrong per se on the kernel side of things, but it makes userspace a fair bit more complicated. To explain that, two things are important:

The first part is simply historical. Many years ago pads and pens were actually on the same kernel event node but that changed probably a decade ago (i don't remember when, doesn't matter anyway). So for the last years we had: one event node for tablet tools, one event node for button/ring/dial/strips, one event node for touch (if any).

libwacom, libinput and GNOME are very much based around that. I don't know the details of KDE and wlroots-based compositors but I suspect they too had most of their testing done with (or copied from) the setup we've had for years.

There is code that can associate the pad to the tool (we have users with e.g. a Cintiq and an Intuos so this is important), but that code probably only handles a single pad device. libinput assigns "device groups" to make it easier for the caller to figure out which devices are part of the same physical device but again it comes down to whether this is handled.

The uclogic driver already diverges from this by having a separate event node for the dial. That didn't matter too much because we haven't handled it but the next version of libwacom/libinput will support this, and mutter support is in the works. I know that if the REL_WHEEL shows up on the pad node it'll work with current libinput but on a separate node? Not sure but it will need updates in libinput and everything above.

For the group buttons the issue is the same: right now nothing can handle buttons of a pad split across several event nodes, so by moving them into another event node everything needs updating. So for the kernel driver its a few LOC but it'll be hundreds of man-hours to get everyone else up-to-date for this[^1]. Compared to the kernel effort of routing this through the same event node this feels like an imbalance.

Another thing I can see after looking at the Inspiroy 2 L - the group buttons here are all marked with icons (can't quite tell what they're supposed to mean though). For Wacom devices with similar buttons we've gone the route of key events: KEY_CONTROLPANEL, KEY_ONSCREEN_KEYBOARD, KEY_BUTTONCONFIG are the ones currently in use (Cintiq Pro 13). The idea here was that these aren't buttons in the traditional sense but rather buttons with fixed meaning. They should, by default, map to things like "open settings" and while remapping is possible having them as generic button is wrong. Looking at this tablet I'd say we have a similar argument?

Ideally and to match the Wacom driver behaviour we'd thus have:

Longer term this means that uclogic devices get to benefit from all the other work we are/have been putting in for Wacom devices, at what looks like a fairly small cost to the kernel driver efforts.


[^1] Historically at least there was a huge delay between "tablet released with new features" and "tablet features are supported" because effectively we had less than a handful of developers dealing with this. So if we want this properly integrated we're looking at, possibly, years.

JoseExposito commented 4 months ago

So for the last years we had: one event node for tablet tools, one event node for button/ring/dial/strips, one event node for touch (if any).

There are 2 main issues implementing this in the kernel:

To introduce this change I'd need to refactor quite a lot of the driver code first... And it is something I've been avoiding because I find some parts of the current code difficult to follow. And most important, I can not test the refactored code because I don't have all the required hardware (unless I spend a few hundreds of euros to be able to work for free :stuck_out_tongue_closed_eyes:).

There are already a few KUnit tests covering some code paths of the driver. I guess I could start growing those as a first step.

That didn't matter too much because we haven't handled it but the next version of libwacom/libinput will support this, and mutter support is in the works.

Ping me when you need a review and testing on that MR.

If we move the dial to the pad node, would it affect your current MR? As far as I know, if we merge the event nodes now, we won't break any user-space tools (at least none of the "popular" ones). However, breaking Mutter would be considered a regression.

tequeter commented 4 months ago

On group buttons:

Huion's group buttons are not the same as Wacom's touch keys, IMHO. Instead, they work like the mode buttons that you described earlier.

Huion's 3 group buttons are meant to remap the pad controls when pressed, thus providing 3 virtual wheels and 24 virtual pad buttons instead of the physical 1 and 8 (see the linked video). Each of the group buttons has a firmware-driven LED that stays on until another group button is pressed.

The first icon suggests it'll enable bindings associated with painting, the third icon would be for controlling a media player, and your guess is as good as mine for the second one :-) I suspect users allocate them as they see fit, though.

On mapping the group buttons as keys instead:

Questions:

On sending the group buttons through the Pad device (as keys or buttons):

Everything you said about being compatible with the rest of the stack! And it would also make the Inspiroy kernel support code simpler, which is always a big plus.

If going with the buttons, do you think overlapping Group/Pad button IDs in a future device or firmware can become a (likely enough) issue, though?

If so, I suspect the uclogic code would need special-casing problematic device/firmware combinations and remapping the buttons IDs in the events so they don't overlap. Would that complexity be acceptable?

If going with the keys instead, same question if the group semantics change in a future tablet model.

mattie20 commented 4 months ago

Tequeter you are right about the group buttons. If you're curious, attached are the default mappings for the group buttons 1 - paintbrush, 2 - settings, and 3 - media in Huions own driver.

I have one and have been trying to get libwacom to work with it, I think @whot might find this line to be helpful for e144f0a / 9f03c25. This is what mine is named and it seems to work. Gnome will recognize it and I can do everything except map the buttons.

DeviceMatch=usb|256c|0067||HUION Huion Tablet_H951P;



b1_paint_icon b2_settings_icon b3_media_icon

whot commented 4 months ago

ah, thanks for the video, that makes things easier to understand (I only have the S which doesn't have those buttons).

This behaviour matches the Wacom Cintiq 24HDT which had three mode buttons next to the ring to toggle directly into one of three modes. IOW there's precedence, except for the indecipherable icons :)

And it means we can ignore my comment regarding treating them like wacom's touch keys, these are proper mode buttons.

If the group buttons were translated to key events, would that still integrate well with the Modes feature?

No, it wouldn't. Key events are terrible for integration purposes because they cannot easily be undone or reliably detected. It's much better to have the tablet send a specific unique event, then emulate key events where it's needed (compositor or even in the app). GTK for example has a GtkPadController which allows you mapping the ring into a GAction so there's even less code needed in the client. But we can ignore that particular headache for these buttons anyway, they're not touch keys.

If going with the buttons, do you think overlapping Group/Pad button IDs in a future device or firmware can become a (likely enough) issue, though?

It's too hard to guess for me, I haven't dealt with the kernel devices directly so I don't exactly know the details of the code. My guess would be "unlikely" because it'll make their driver more complicated too but that's really just a guess, I don't know how these are handled in Windows.

If so, I suspect the uclogic code would need special-casing problematic device/firmware combinations and remapping the buttons IDs in the events so they don't overlap. Would that complexity be acceptable?

You already need to add the PIDs one-by-one and you already have special-cases so having one for shifting buttons shouldn't be too hard. Again, that's speaking from a position with no knowledge of the kernel driver details.

whot commented 4 months ago
DeviceMatch=usb|256c|0067||HUION Huion Tablet_H951P;

@mattie20 can you send a PR for this to libwacom please? That device has its own PID so we can merge that, we don't need the new uniq firmware match.

Thanks for the image, interestingly: on my Inspiroy 2 S (no group buttons and only 6 buttons total) the default mapping is BIE and Ctrl+S, Space and Ctrl+Alt+Z. The "Zoom in/out" is implemented as Ctrl = and Ctrl - so you need to hope it's implemented by the app in question.

tequeter commented 4 months ago

@JoseExposito , do you have advice on where and how to route the group buttons, or know someone who might?

To summarize: H951P and H1061P have "group buttons" which are actually "mode buttons" in Wacom-speak. The tablets send them in a subreport distinct from the regular pad buttons.

Options:

  1. Expose them on a distinct "Group Buttons" device. While future-proof on the kernel side, it's going to be a nightmare to integrate with userland portions of the stack. This is the current behavior as of this PR.
  2. Route them to the Pad device unchanged. This would both simplify the kernel side and integrate nicely with userland, but risk hard-to-solve breakage if future tablets use the same IDs for group and pad buttons.
  3. Something else, like routing them to the Pad device preemptively remapped to some higher IDs (what would be a safe range)?

I think it's a significant decision, because rechanging the interface later would break users' configs and/or require synchronized libwacom updates AFAIU.

Yet, I lack the experience supporting device drivers for 3rd party manufacturers to make this call alone.

whot commented 4 months ago

@spbnick may/would/should know :smile:

Best of my understanding from a fairly short look at the driver: this driver sits "on top" of the hid core drivers in the kernel. What it does is primarily modifying the report descriptors and reports before the kernel sees them, but the actual input event handling, input device creation, etc. is left to the hid-core (hid-generic?) kernel drivers.

It seems to do this by creating HID Application Collections for new HID Reports which the kernel splits into separate event nodes (That's why you have "Touch Strip", "Fingers", "Pad", etc devices). The driver calls these "subreports" and the report descriptor bytes for those are in hid-uclogic-rdesc.c and they get added to the struct uclogic_params and eventually compiled into one report descriptor in uclogic_params_get_desc().

So basically: kernel calls uclogic_probe, this driver creates a new rdesc and lets the kernel do the rest.

The subreports are linked via the report ID (subreport->value), when an event (==report) comes in, uclogic_raw_event checks if the report ID matches any of the subreport values and then mangles the data in the report according to the driver data (uclogic_raw_event_pen()).

Is this roughly correct?

Now, the event node per-report is the HID_QUIRK_MULTI_INPUT. If we change that to HID_QUIRK_INPUT_PER_APP we instead get an event node per application collection (I think, cc @bentiss).

Then we could group the various bits together into a single application collection instead and.. things should just fall into place? Not 100% if you can have an AC split across multiple report IDs though.

Doing this makes my Dial event node disappear and its REL_WHEEL axes move to the Keypad device, so it's somewhat promising :)

diff --git hid-uclogic-core.c hid-uclogic-core.c
index 6d3bbec6336f..f490f3c5c343 100644
--- hid-uclogic-core.c
+++ hid-uclogic-core.c
@@ -174,7 +174,7 @@ static int uclogic_probe(struct hid_device *hdev,
         * libinput requires the pad interface to be on a different node
         * than the pen, so use QUIRK_MULTI_INPUT for all tablets.
         */
-       hdev->quirks |= HID_QUIRK_MULTI_INPUT;
+       hdev->quirks |= HID_QUIRK_INPUT_PER_APP;
        hdev->quirks |= HID_QUIRK_HIDINPUT_FORCE;
 #ifdef HID_QUIRK_NO_EMPTY_INPUT
        hdev->quirks |= HID_QUIRK_NO_EMPTY_INPUT;
diff --git hid-uclogic-rdesc.c hid-uclogic-rdesc.c
index c26bfb139c53..a65c6c25959c 100644
--- hid-uclogic-rdesc.c
+++ hid-uclogic-rdesc.c
@@ -660,7 +660,7 @@ const size_t uclogic_rdesc_v2_pen_template_size =
 #define UCLOGIC_RDESC_FRAME_BUTTONS_BYTES(_id, _size) \
        0x05, 0x01,     /*  Usage Page (Desktop),               */ \
        0x09, 0x07,     /*  Usage (Keypad),                     */ \
-       0xA1, 0x01,     /*  Collection (Application),           */ \
+       0xA1, 0x02,     /*  Collection (Logical),           */ \
        0x85, (_id),    /*      Report ID (_id),                */ \
        0x14,           /*      Logical Minimum (0),            */ \
        0x25, 0x01,     /*      Logical Maximum (1),            */ \

edit: the collection change isn't necessary

spbnick commented 4 months ago

The description of the driver inner workings is roughly correct, yes, AFAIR. The HID_QUIRK_INPUT_PER_APP seems to be a recent addition, I'm not sure how it would work exactly. As is the sad state of input in Linux, you could only tell how it would work if you read the userspace driver code (there are no docs, basically). It could break user configuration too, in theory, but again, I don't know what HID_QUIRK_INPUT_PER_APP will do exactly.

whot commented 4 months ago

The HID_QUIRK_INPUT_PER_APP seems to be a recent addition,

Relatively recently (compared to DIGIMEND :): kernel 4.18.

AIUI HID_QUIRK_MULTI_INPUT splits/groups event nodes per report ID, HID_QUIRK_MULTI_INPUT splits/groups events node per application collection. FTR I'm not proposing to change all devices since we can't test for breakage but it might be worth considering enabling that for all devices from now on to get slightly better behaviour?

Having said that: I spent 2h today to write a new (my first) bpf-based input driver and ended up with this draft PR for udev-hid-bpf and I get what looks like a perfectly functioning single-event-node pad device. Haven't looked at the pen yet but this looks like a really viable path, doubly so if huion keeps adding new pids for devices instead of re-using them.

@tequeter if you can get me the hid-recorder output for the M/L devices (all buttons and the wheel) I can try to add those - note these need to be the ones without hid-uclogic, i.e. as the device sends them OOTB.

JoseExposito commented 4 months ago

The HID_QUIRK_INPUT_PER_APP seems to be a recent addition, Relatively recently (compared to DIGIMEND :): kernel 4.18.

Debian stable (v12) ships kernel 6.1. Even CentOS Stream 8 ships kernel 4.18. In my opinion, we should be fine using it.

AIUI HID_QUIRK_MULTI_INPUT splits/groups event nodes per report ID, HID_QUIRK_MULTI_INPUT splits/groups events node per application collection. FTR I'm not proposing to change all devices since we can't test for breakage but it might be worth considering enabling that for all devices from now on to get slightly better behaviour?

That could be complicated as, due to shared PIDs across devices and vendors, we don't know which devices we support. It won't be possible to create a list of old devices. Neither a list of new devices, they might just work out of the box if they used the right VIP/PID.

I can try to open a draft PR modifying the required HID descriptors and test with different tablets. Let's see if I have some time this weekend ;)

However, reducing the number of interfaces might break user config. I'm not sure if it is safe to introduce this change.

JoseExposito commented 4 months ago

The changed end up being quiet simple. The implications of the change are not. But at leas it provides a good way to test if having all devices in the same node help userspace somehow: https://github.com/DIGImend/digimend-kernel-drivers/pull/693

tequeter commented 4 months ago

tequeter if you can get me the hid-recorder output for the M/L devices (all buttons and the wheel) I can try to add those - note these need to be the ones without hid-uclogic, i.e. as the device sends them OOTB.

Here are the recordings for the 3 devices exposed by the tablet without uclogic present.

h1061p-0.hid.txt h1061p-1.hid.txt h1061p-2-gb1.hid.txt - with group button 1 active (paintbrush). h1061p-2-gb2.hid.txt - with group button 2 active (settings). h1061p-2-gb3.hid.txt - with group button 3 active (media).

The group buttons do not send anything, they just rebind the pad buttons to send different key sequences. I did a separate capture with each group button active, for clarity.

Each time, I pressed the buttons from top to bottom, then rolled the wheel up and down.

whot commented 4 months ago

Thanks for the recordings!

The group buttons do not send anything, they just rebind the pad buttons to send different key sequences.

That actually means we cannot use a BPF program as-is, we need something to switch the tablet into raw mode. We have some plans for that, but meanwhile - can you get me the recordings after the tablet is switched to table mode, but without uclogic?

https://github.com/whot/huion-switcher should do the trick, run it once, then hid-record the buttons/dials/etc. Works on my Intuos 2 S, so let's hope it does for you too.

tequeter commented 4 months ago

Not necessarily: the tablet sends unique key events for the pad buttons and wheel when the different group buttons are activated.

So you could just let the tablet take care of the group/mode switching (it already manages the related LEDs) and consider your input to be 24 virtual pad buttons and 3 virtual wheels. I expect most users to be happy with the group buttons working as group buttons and not want to do something fancy when they are pressed.

I don't know how it'll play with the rest of the stack, though.

In any case, here is a recording in raw mode (huion-switcher worked mighty fine). I pressed the group buttons from top to bottom, then the pad buttons likewise, then rolled the wheel up and down.

h1061p-0-raw.hid.txt

NB that in raw mode the events are sent on the first device.

whot commented 4 months ago

@tequeter: Thanks for the recording. I now mostly finished the support for my device udev-hid-bpf!85. If you have the time it should be relatively simple to do the same for your device.

Changes you would need (aside from the PID near the top) is in fixed_rdesc_vendor: line 318: UsagePage_Button: my device has 6 buttons, so we have a UsageRange from 1-6 (BTN_0 to BTN_5) and it needs 6 bits + 2 padding bytes (ReportCount(2) + Input(Const)).

your device has 11 buttons that come through as bitmask, so if you change to UsageRange_i8(0x01, 0xb), ReportCount(11) followed by 5 padding bytes. This gives 16 bits in the report for buttons so the wheel byte becomes byte 6 in your report. You'll need to adjust the code after line 431 ((data[0] == VENDOR_REPORT_ID) accordingly.

But otherwise you should be able to copy this one as-is and have it work. This probably sounds a lot more complicated than it is too ;)

Once you have udev-hid-bpf, the bits you'll need to test (aside from the huion switcher)

$ meson compile -C builddir

# reload/replace the BPF - change the 0066 PID
$ sudo ./build/udev-hid-bpf --verbose add --replace /sys/bus/hid/devices/0003:256C:0066.00* - build/src/bpf/testing/10-Huion__Inspiroy-2-S.bpf.o

# to see any bpf_printk
$ sudo cat /sys/kernel/debug/tracing/trace_pipe

No re-plugging needed but if you do don't forget to run the huion switcher again.

Should be possible to just re-use the same bpf for all three of S/M/L since there's not much difference. Maybe I'll get to this next week if you don't beat me to it.