Bug 21832

Found the problem.  My mouse is sending EV_ABS with event code 40:

event type=EV_KEY, code=272, value=1
event type=EV_ABS, code=40, value=0
event type=EV_REL, code=0, value=2
event type=EV_REL, code=1, value=5
event type=EV_SYN, code=0, value=0

Why?  Who knows!  Every time there's a

  event type=EV_ABS, code=40, value=0

though it's followed by

  event type=EV_ABS, code=40, value=2

so perhaps there is some Deeper Meaning.  Checking input.h shows that code 40 means ABS_MISC, which provides no additional information.

The real problem appears to be that the current code doesn't support mixed relative and absolute devices:

    /* We don't allow relative and absolute axes on the same device. Reason
       Reason being that some devices (MS Optical Desktop 2000) register both
       rel and abs axes for x/y.
       The abs axes register min/max, this min/max then also applies to the
       relative device (the mouse) and caps it at 0..255 for both axis.
       So unless you have a small screen, you won't be enjoying it much.

        FIXME: somebody volunteer to fix this.
     */
    if (pEvdev->flags & EVDEV_RELATIVE_EVENTS)
	EvdevAddRelClass(device);
    else if (pEvdev->flags & EVDEV_ABSOLUTE_EVENTS)
        EvdevAddAbsClass(device);

Except that it does support them mixed, sort of.  EvdevProcessEvent will happily accept EV_ABS events, attempt to stuff them in the "vals" array, and set the "abs" flag.

        case EV_ABS:
            if (ev->code > ABS_MAX)
                break;
            pEvdev->vals[pEvdev->axis_map[ev->code]] = value;
            if (ev->code == ABS_X)
                abs |= ABS_X_VALUE;
            else if (ev->code == ABS_Y)
                abs |= ABS_Y_VALUE;
            else
                abs |= ABS_VALUE;
            break;

Then with the next EV_SYN we process the data as if they were absolute in the first place, and call xf86PostMotionEventP() with "is_absolute" set to TRUE.

So the driver queries the mouse for the relative and absolute events it will generate (and my mouse correctly reports that it will send ABS_MISC).  Then it initializes just the EV_REL portion.  Then it accepts and processes EV_ABS events that it never set itself up to handle.  The minimal solution appears to be:

   * Process EV_REL events in EvdevProcessEvent if and only if the EVDEV_RELATIVE_EVENTS flag is set.  Ignore them otherwise.

   * Process EV_ABS events in EvdevProcessEvent if and only if the EVDEV_ABSOLUTE_EVENTS flag is set _and_ the EVDEV_RELATIVE_EVENTS flag is not set.  Ignore them otherwise.

Created attachment 26047 [details] [review]
Patch to ignore REL, ABS events when driver not configured for them

This patch fixes the problem on my machine.

+  if (EVDEV_RELATIVE_EVENTS || ! (pEvdev->flags & EVDEV_ABSOLUTE_EVENTS))

the first bit doesn't make sense, shouldn't that be (.. flags & RELATIVE..)?
Also, the second condition should be enough. If not, then we need to change
the flag setting code.

please remove the space between the ! and the condition too.

Other than that, looks good. Please reattach the modified version as a
git-formatted patch (see
http://wiki.x.org/wiki/Development/Documentation/SubmittingPatches)

Created attachment 26050 [details] [review]
Git-format patch to ignore REL, ABS events when driver not configured for them

Yeah, the first bit is broken.  But it does need both parts.  Look at the initialization code:

    if (pEvdev->flags & EVDEV_RELATIVE_EVENTS)
        EvdevAddRelClass(device);
    else if (pEvdev->flags & EVDEV_ABSOLUTE_EVENTS)
        EvdevAddAbsClass(device);

It's completely legal for a device to support neither relative nor absolute events.  The ABS handlers won't get set up if EVDEV_RELATIVE_EVENTS is set, and they won't get set up if EVDEV_ABSOLUTE_EVENTS is not set.  We should ignore the ABS events in either situation.

Added git-format patch.

> --- Comment #4 from Derek Upham <sand@blarg.net>  2009-05-21 00:28:51 PST ---
> Created an attachment (id=26050)
>  --> (http://bugs.freedesktop.org/attachment.cgi?id=26050)
> Git-format patch to ignore REL, ABS events when driver not configured for them

ack, looks good to me.

> Yeah, the first bit is broken.  But it does need both parts.  Look at the
> initialization code:
> 
>     if (pEvdev->flags & EVDEV_RELATIVE_EVENTS)
>         EvdevAddRelClass(device);
>     else if (pEvdev->flags & EVDEV_ABSOLUTE_EVENTS)
>         EvdevAddAbsClass(device);

You are right. This makes me wonder though if we should just unset the
EVDEV_ABSOLUTE_EVENTS flag at this point in the code. This way we require a
single condition in your patch and we only have one point where this hack is
needed (and documented). In the future, when we manage to figure out how to
support both types of axes there's only one point in the code we have to
worry about.

Created attachment 26063 [details] [review]
Derek's patch amended with Peter's suggestion from comment #6

We shouldn't unset the flag there.  We should change EvdevProbe so that it never enters the current

  if (has_abs_axes) { ... }

block if we have already set up for relative events. That way the device is never in an inconsistent state.  The function is already doing something similar for pure scroll wheels, near the bottom.

The simplest approach would be to never enter the for (...) {...} loop that tests bits if "has_rel_axes" is set.  Or we have special code after the loop that checks "has_rel_axes", sets "has_abs_axes" to false, and then sends an X_INFO message: "already set up for relative motion, ignoring absolute motion events".

I disagree, simply for the reason that it would split the hack over two different locations again, making it harder to identify and fix in the future. If we were to skip the has_abs part, then we'd have to have a comment there to refer to the other location and vice versa.

You're right that we should have X_INFO output, I'll add that to your patch before the flag is unset.

*** Bug 21891 has been marked as a duplicate of this bug. ***

Pushed as 0a3657d2ee62f4086e9687218cb33835ba61a0b3. Thanks for the patch.