Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use HID_QUIRK_HIDINPUT_FORCE to fix #550 #553

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tornaria
Copy link

@tornaria tornaria commented Aug 4, 2021

We must force HIDINPUT, since after ec5c16d some devices are of class HID_DG_DIGITIZER, which is (incorrectly?) not considered as input by macro IS_INPUT_APPLICATION().

With this quirk all devices are forced to go through hidinput_connect() to be configured as input devices.

For instance, XP-PEN G430S was broken by ec5c16d and it works now.

I also included a second (cosmetic) patch which makes devices of class HID_DG_DIGITIZER to use suffix "Digitizer" in the name of the device (used for dmesg, libinput, xinput,...)

We must force HIDINPUT, since after ec5c16d some devices are of class
HID_DG_DIGITIZER, which is (incorrectly?) not considered as input by
macro IS_INPUT_APPLICATION().

With this quirk all devices are forced to go through hidinput_connect()
to be configured as input devices.

For instance, XP-PEN G430S was broken by ec5c16d and it works now.
This is cosmetic, affects the name of the device in dmesg, libinput and
xinput.
@nic3-14159
Copy link

Here's some background to what this patch fixes. There seems to be a few XP-PEN tablets (confirmed on G430s and STAR 06) that no longer work with the DIGImend drivers after commit ec5c16d. After some investigation, we believe the source of the issue is the IS_INPUT_APPLICATION macro in include/linux/hid.h in the kernel tree.

/* Applications from HID Usage Tables 4/8/99 Version 1.1 */
/* We ignore a few input applications that are not widely used */
#define IS_INPUT_APPLICATION(a) \
		(((a >= HID_UP_GENDESK) && (a <= HID_GD_MULTIAXIS)) \
		|| ((a >= HID_DG_PEN) && (a <= HID_DG_WHITEBOARD)) \
		|| (a == HID_GD_SYSTEM_CONTROL) || (a == HID_CP_CONSUMER_CONTROL) \
		|| (a == HID_GD_WIRELESS_RADIO_CTLS))

By changing the HID descriptor usage from 0x02 (Pen) to 0x01 (Digitizer), the macro evaluates to false, as the digitizer usage leads to HID_DG_DIGITIZER being passed to the macro, which does not pass a >= HID_DG_PEN.

This macro is used in drivers/hid/hid-input.c, in the function hidinput_connect():

if (!force) {
	for (i = 0; i < hid->maxcollection; i++) {
		struct hid_collection *col = &hid->collection[i];
		if (col->type == HID_COLLECTION_APPLICATION ||
				col->type == HID_COLLECTION_PHYSICAL)
			if (IS_INPUT_APPLICATION(col->usage))
				break;
	}

	if (i == hid->maxcollection)
		return -1;
}

We believe the reason it only seems to be affecting XP-PEN tablets is that the hid-uclogic-params.c code does not add a HID descriptor for the frame on these models, either because the tablet does not have frame buttons or the frame is not currently supported. The frame descriptors use the Keypad usage from the Generic Desktop page, which passes the IS_INPUT_APPLICATION macro. It seems like the above code snippet passes if at least one usage passes the macro, so without the frame descriptor, the function returns early and does not configure the input layer. Based on this, it seems likely that any tablet with a fixed descriptor that does not contain a usage which passes the macro will also experience similar issues.

Using HID_QUIRK_HIDINPUT_FORCE as suggested by @tornaria leads to the force flag being set to true, bypassing the check. A more permanant fix would be to change HID_DG_PEN to HID_DG_DIGITIZER in the IS_INPUT_APPLICATION macro, as the assumption that the digitizer input application is not widely used no longer seems to be correct after the usages were changed in the DIGImend code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants