On Thu, Nov 9, 2023 at 12:19 AM David Revoy <davidrevoy@xxxxxxxxxxxxxx> wrote: > > > BTW, David, were you able to do a revert of 276e14e6c3? > > I'm sorry Benjamin: I did some research on how to build a kernel [1], on how to revert a commit (easy, I know a bit of Git), and started following it step by step. Result: I failed and concluded that it probably requires too much computer knowledge compared to what I can do now. I'm afraid I won't be able to build a custom kernel for testing. No worries. And I'm actually happy, because you definitely fit into the HID-BPF model where I want to fix a user's device without requiring kernel compilation, and fixing the device in a reliable way that we can do the general fix without impacting the reporter. Cheers, Benjamin > > [1] https://docs.fedoraproject.org/en-US/quick-docs/kernel-build-custom/#_building_a_vanilla_upstream_kernel > > > On Tuesday, November 7th, 2023 at 08:59, Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx> wrote: > > > > On Mon, Nov 6, 2023 at 9:06 PM Illia Ostapyshyn > > ostapyshyn@xxxxxxxxxxxxxxxxxxx wrote: > > > > > On 11/6/23 17:59, Benjamin Tissoires wrote: > > > > > > > If the pen has 2 buttons, and an eraser side, it would be a serious > > > > design flow for XPPEN to report both as eraser. > > > > > > > > Could you please use sudo hid-recorder from hid-tools[1] on any kernel > > > > version and send us the logs here? > > > > I'll be able to replay the events locally, and understand why the > > > > kernel doesn't work properly. > > > > > > > > And if there is a design flaw that can be fixed, we might even be able > > > > to use hid-bpf to change it :) > > > > > > My wild guess is that XP-Pen 16 Artist Pro reports an Eraser usage > > > without Invert for the upper button and Eraser with Invert for the > > > eraser tip. A device-specific driver could work with that, but there > > > seems to be no way to incorporate two different erasers (thus, allowing > > > userspace to map them to different actions arbitrarily) in the generic > > > driver currently. > > > > > > That's exactly why I want to see the exact event flow. We can not do > > "wild guesses" unfortunately (not meaning any offenses). > > And I am very suspicious about the fact that the stylus reports 2 > > identical erasers. Because in the past David seemed to be able to have > > 2 distincts behaviors for the 2 "buttons" (physical button and eraser > > tail). > > > > > > Generally speaking, relying on X to fix your hardware is going to be a > > > > dead end. When you switch to wayland, you'll lose all of your fixes, > > > > which isn't great. > > > > > > > AFAIU, the kernel now "merges" both buttons, which is a problem. It > > > > seems to be a serious regression. This case is also worrying because I > > > > added regression tests on hid, but I don't have access to all of the > > > > various tablets, so I implemented them from the Microsoft > > > > specification[0]. We need a special case for you here. > > > > > > The issue preventing David from mapping HID_DG_ERASER to BTN_STYLUS2 is > > > that the hidinput_hid_event is not compatible with hidinput_setkeycode. > > > If usage->code is no longer BTN_TOUCH after remapping, it won't be > > > released when Eraser reports 0. A simple fix is: > > > > > > I must confess, being the one who refactored everything, I still don't > > believe this is as simple as it may seem. I paged out all of the > > special cases, and now, without seeing the event flow I just can not > > understand why this would fix the situation. > > > > And BTW, if you have a tool affected by 276e14e6c3, I'd be curious to > > get a hid-recorder sample for it so I can get regression tests for it. > > > > > --- a/drivers/hid/hid-input.c > > > +++ b/drivers/hid/hid-input.c > > > @@ -1589,7 +1589,7 @@ void hidinput_hid_event(struct hid_device *hid, > > > struct hid_field field, struct > > > / value is off, tool is not rubber, ignore */ > > > return; > > > else if (*quirks & HID_QUIRK_NOINVERT && > > > - !test_bit(BTN_TOUCH, input->key)) { > > > + !test_bit(usage->code, input->key)) { > > > > > > I don't want to be rude, but this feels very much like black magic, > > especially because there is a comment just below and it is not > > updated. So either the explanation was wrong, or it's not explaining > > the situation (I also understand that this is not a formal submission, > > so maybe that's the reason why the comment is not updated). > > > > > /* > > > * There is no invert to release the tool, let hid_input > > > * send BTN_TOUCH with scancode and release the tool after. > > > > > > This change alone fixes David's problem and the right-click mapping in > > > hwdb works again. However, the tool switches to rubber for the remapped > > > eraser (here BTN_STYLUS2) events, both for devices with and without > > > Invert. This does no harm but is not useful either. A cleaner solution > > > for devices without Invert would be to omit the whole tool switching > > > logic in this case: > > > > > > @@ -1577,6 +1577,9 @@ void hidinput_hid_event(struct hid_device *hid, > > > struct hid_field *field, struct > > > > > > switch (usage->hid) { > > > case HID_DG_ERASER: > > > + if (*quirks & HID_QUIRK_NOINVERT && usage->code != BTN_TOUCH) > > > + break; > > > + > > > report->tool_active |= !!value; > > > > > > Remapping Invert does not work anyway as the Invert tool is hardcoded in > > > hidinput_hid_event. Even worse, I guess (not tested) trying to do so > > > would mask BTN_TOOL_RUBBER from dev->keybit and could cause weird > > > behavior similar to one between 87562fcd1342 and 276e14e6c3. This > > > raises the question: should users be able to remap Invert after all? > > > > > > The kernel is supposed to transfer what the device is. So if it says > > this is an eraser, we should not try to change it. Users can then > > tweak their own device if they wish through hid-bpf or through > > libinput quirks, but when you install a fresh kernel without tweaks, > > we should be as accurate as possible. > > > > My main concern is that now we have a device which exports 2 different > > interactions as being the same. So either the firmware is wrong, and > > we need to quirk it, or the kernel is wrong and merges both, and this > > needs fixes as well. > > > > Once every interaction on the device gets its own behavior, userspace > > can do whatever they want. It's not the kernel's concern anymore. > > > > BTW, David, were you able to do a revert of 276e14e6c3? > > > > Cheers, > > Benjamin >
