Bug 26584

Patches tend to be more quickly processed when mailed to xorg-devel
as the output from git format-patch, as described at:
http://www.x.org/wiki/Development/Documentation/SubmittingPatches

Created attachment 33338 [details] [review]
git formatted diff against a7460d35743b7343a6dcce13f14cfd28428385bf

Comment on attachment 33325 [details] [review]
Patch solving the issue.

Diff against source release 1.3.0

Thanks for the patch, looks good. I do wonder if it would be simpler to do something like this:

static KeySym aiptek_map[256] = {
 // all already defined keysyms
}

Since NoSymbol is equivalent to 0L and that won't change this would give us the same result but make the patch and the code easier to read by not having lines of NoSymbol.

Yes it's really simpler (but I was also somewhat mirroring the wacom driver behaviour).

But, after some further testing, I found another bug, around the macroKey assignment.Line 915:
/*
 * Any other EV_KEY event is assumed to be
 * the pressing of a macro key from the tablet.
 */
default:
{
    ++eventsInMessage;
    common->currentValues.macroKey = event->value;
}
break;

I suppose macroKey should be equal to event->code (and not value).

Also, using the 256 entries table, we can probably directly map linux kernel key code to X.Org values. (and not looping over possible kernel values and assigning them to X.Org values).

I will try to come with a better patch today.

> --- Comment #5 from Olivier Samyn <olivier@oleastre.be>  2010-02-16 23:16:14 PST ---
> Yes it's really simpler (but I was also somewhat mirroring the wacom driver
> behaviour).

that should probably get fixed in the wacom driver too.

> But, after some further testing, I found another bug, around the macroKey
> assignment.Line 915:
> /*
>  * Any other EV_KEY event is assumed to be
>  * the pressing of a macro key from the tablet.
>  */
> default:
> {
>     ++eventsInMessage;
>     common->currentValues.macroKey = event->value;
> }
> break;
> 
> I suppose macroKey should be equal to event->code (and not value).

yes, that makes sense. please provide a separate patch for this though to
ease bisectability and traceability.

> Also, using the 256 entries table, we can probably directly map linux kernel
> key code to X.Org values. (and not looping over possible kernel values and
> assigning them to X.Org values).

well, it's not quite that easy. the kernel sends keycodes that are then
mapped into keysyms by the client. X keycodes are essentially kernel keycode
+ 8, so that's easy enough provided the user has the evdev ruleset (which is
standard these days on linux).

where it does become tricky though is that users configure thinking of
keysyms, not keycodes. so a user says "I want to map Q to K", even though
"Q" is only KEY_Q on a qwerty layout. so some keycode to keysym lookup may
be needed (the wacom driver still does that for this reason).
tbh, I don't know enough about the aiptek driver to know if that applies in
this case, but please be aware of the above - you'd risk wasting time
otherwise.

From the kernel source code, the aiptek driver translates some status bits to a fixed set of kernel KEY_* key events.

What I plan to do is:
- have two static key arrays that defines the direct key mapping (keep current linux_inputDevice_keyMap and a slightly modified aiptek_map)
- add a new static 256 elements array (call it aiptek_key_map) that will define the real key, all elements initialized to 0.
- Modify xf86AiptekPlug and add a loop that initialize aiptek_key_map with 
the following rule: aiptek_key_map[linux_inputDevice_keyMap[i]]=aiptek_map[i]
- Modify xf86AiptekSendEvents to map directly kernel event to X.Org one. Event keysym = aiptek_key_map[common->currentValues.macroKey]

Patch will follow...

Created attachment 33376 [details] [review]
Uses event-code instead of event->value

Patch series to make the tablet keys working.

Created attachment 33377 [details] [review]
Uses direct event code to keycode mapping for XI ABI >=7

Created attachment 33378 [details] [review]
Simplify keycode to keysym mapping

I added three separate patches to simplify the current kernel keycode to keysym mapping as discussed before.

Everything seems to work, except I do not get any keyboard event in xev. I'm probably missing something, but can not figure it out.

I will continue investigating, but some help is welcome.

(In reply to comment #8)
> Created an attachment (id=33376) [details]
> Uses event-code instead of event->value

applied, thanks.(In reply to comment #9)

> Created an attachment (id=33377) [details]
> Uses direct event code to keycode mapping for XI ABI >=7

this one isn't correct, if you change to 8+i instead of 7+i you offset pre-ABI 7 mapping by one too, don't you?(In reply to comment #10)

> Created an attachment (id=33378) [details]
> Simplify keycode to keysym mapping

i think this one needs a few more comments to explain what it does.

+            for (loop = 0; loop < linux_inputDevice_keyMap_len; ++loop)

this block - couldn't you just memcpy this? you can also use ARRAY_LENGTH (copy from evdev) to avoid the _len variable. I'm not sure how needed this approach is anyway - I think you could just leave the current array in place but fix it at [256] in size so the remainers are filled with NoSymbol.


also, the map itself looks quite suspicious - XK_F2 for keycode 9 (KEY_ESC), this is rather odd and would likely return weird results. what are the actual keycodes you get from the kernel? Are they standardised through the event api? if so, you could leave the map as empty anyway, because the actual map is kept by xkb.

Created attachment 33465 [details] [review]
Correct linux keymap to keysym index loop

This one corrects the loop that was iterating over the linux key map but testing for a match only with the first map element ([0] instead of [i]).

Created attachment 33466 [details] [review]
Change index offset for generated key events

(In reply to comment #12)
> this one isn't correct, if you change to 8+i instead of 7+i you offset pre-ABI
> 7 mapping by one too, don't you?(In reply to comment #10)

The internal linux map starts with KEY_F1 (index 0 in the map), that maps to XK_F1 that's the 9th element of the aiptek_map (index 8 in the map).

So, I suppose adding 8 is the intended behavior (or is there some magic trick in the xf86PostKeyEvent function that adds one ? ).

(In reply to comment #14)
> The internal linux map starts with KEY_F1 (index 0 in the map), that maps to
> XK_F1 that's the 9th element of the aiptek_map (index 8 in the map).

oh, right, sorry. I went with the assumption that the event we get has the value from the kernel (e.g. KEY_ESC) not the index into the array.

both patches merged and pushed, thanks.

Created attachment 33514 [details] [review]
Initialize the keymap with 256 items

This solves the actual bug in the simplest way. 

Xorg does not crash anymore, but no key event is received by xev.

Created attachment 33515 [details] [review]
Add keyboard feedback struct initialization

A simple call to InitKbdFeedbackClassDeviceStruct seems to make the aiptek driver recognized as a keyboard event emitter.

So this makes aiptek generated keyboard events visible to xev.

Thanks, applied and pushed.
As I've added in the commit message for the second patch, this only fixes the issue for X servers 1.6 and earlier. X server 1.7 has a different initialization method that - afaict - is currently not employed at all by the aiptek driver. So under 1.7, the driver has no key symbols at all.

Just letting you know about this so you're not surprised when you update :)
Please feel free to send fixes for this case as well, I really appreciate your work here.

Given that this bug is fixed now (I think?), it's probably best to close it and open another one for the 1.7 issue though.

Created attachment 33516 [details] [review]
Generate only key press or key release depending on kernel event.

Without this patch, the driver generates two key events (hard coded press and release events) per physical key press on the tablet. 

This patch keeps track of the kernel event value that contains 1 if key pressed and 0 if released; and reuse that value to generate the X event.

Cross posting, I just added a small patch for double event generation.

And yes, it solves issues with xserver 1.6 (as I'm still running ubuntu 9.10).

But I plan to install this week the next version, and makes the driver working with xserver 1.7 (and I hope the patches will be merged downstream).

Also, there is still an issue with the generated keysyms.

Although the keycode is the one emitted by the driver, the received keysym seems to come from the evdev keymap (and not the specific one installed by the aiptek driver).

Basically this closes this bug, and I will open a new one for the keysym issue and another one for xserver 1.7 support.

(In reply to comment #19)
> Created an attachment (id=33516) [details]
> Generate only key press or key release depending on kernel event.
> 
> Without this patch, the driver generates two key events (hard coded press and
> release events) per physical key press on the tablet. 
> 
> This patch keeps track of the kernel event value that contains 1 if key pressed
> and 0 if released; and reuse that value to generate the X event.

for server 1.6 and later you also need to filter out autorepeat - the server will do that for you. See the evdev code at 
http://cgit.freedesktop.org/xorg/driver/xf86-input-evdev/tree/src/evdev.c#n299

or does the driver do its own filtering anyway? i noticed the previousValues.pressed so if you have the automatic filtering based on the event value, you might be able to get rid of the previousValues.pressed part? (just guessing again)


(In reply to comment #20)
> Basically this closes this bug, and I will open a new one for the keysym issue
> and another one for xserver 1.7 support.

thanks, I'll comment on that bug then, because atm I'm confused what server version this issue occurs ;) 

Follow up for xinput ABI > 7 posted as bug #29543

sorry for the late answer. Is this still a problem, do you still have this device and are you still using it?
aiptek is barely maintained and tested, it might be beneficial to get the driver supported properly by the kernel and then use evdev or the wacom driver for this device

Short of getting this into kernel-land for evdev, is there anything wrong with 
the attached patch?  If not, let's just get the patch committed and close this 
out.

Closing per #29543