[RFC][PATCH] General axis valuator support

[Matt Helsley]

On Sun, Jan 11, 2009 at 8:01 PM, Peter Hutterer <peter.hutterer at who-t.net> wrote:
> Some comments purely related to your comments, not your code. I won't be
> able to look at your code today.
>
> On Sun, Jan 11, 2009 at 07:37:49PM -0800, Matt Helsley wrote:
>> Currently X, Y, and PRESSURE are special case axes in xf86-input-evdev.
>> This patch supports axes in a more general way. There are a few open
>> questions and TODOs:
>>
>> TODO:
>>       Add REL axes. Since we currently limit evdev devices to
>>               exclusively REL or ABS axes we can reuse the same vals
>>               buffer in the device structure.
>
> This is actually a bit tricky. If we allow rel and abs for the same axis, we
> have to convert all relative events to absolute ones in the driver. Reason
> being that once the abs axes are set up, the server caps at the ranges.

Based on this and the XI2 protocol spec discussion it looks like each axis has a
separate mode (Absolute, Relative). I wasn't expecting that -- I figured it
would apply to the "whole device".

> This is made slightly more complicated by the fact that a few devices (some
> popular mouse/keyboard combos) announce an ABS_X range of 256/256, so any
> relative X axis movement would get scaled to that range.

For those we could look at the corresponding axis in rel_bitmask and, if it exists,
we don't set up any corresponding abs axis in the server. Then we ignore any
input_events for ABS_<axis>. Of course we're then assuming that events from the two
axes are in fact related to the same user action...

Perhaps the really clever thing to do would be to compare the values coming in
during a "sample" input period. If the REL_ and ABS_ values correspond then we
know the axes are related.

Unless there's something totally obvious I'm missing I don't think it's a problem
we can cleanly solve.

>>       In theory there can be more than MAX_VALUATORS axes. Often there
>>               will be less than MAX_VALUATORS axes. This either fails
>>               to fully support a device or wastes a space. Should
>>               probably switch to dynamically-allocated values but this
>>               means we need an UnInit function.
>
> MAX_VALUATORS is the maximum the server supports, so we don't need to worry
> about the driver here until the server increases that value.

OK. I'll leave the dynamic allocation parts for later (if ever).

>>       Needs testing. Currently I've only got devices that
>>               support up to three axes. A nice Wacom tablet that
>>               supports tilt might be able to test more axes with
>>               some small testing-specific kernel/X driver hacks.
>>               Any other ideas for testing?
>
> look at the test/ directory in the evdev tree. uinput makes it really easy to
> create fake devices with a hideous number of axes, etc.

OK. I haven't had a look at these yet but it seems like an excellent use of uinput!

>> QUESTIONS:
>>       Is this the only code that needs to test and count bits or can
>>               we make these pieces common somehow?
>
> You mean in X.org? no, the synaptics driver (and I think the joystick driver)
> do it too.

OK. A later patch perhaps..

>>       There are still locations that special-case X and Y:
>>               1. TOUCHPAD dx, dy calculation
>>               2. Axis-swapping (would require an axis-mapping)
>>               3. Axis-inversion
>>               4. Calibration
>>
>>       The swap, scale, inversion, and calibration transforms look like
>>               they are done in the wrong order; we wind up using Y's
>>               min and max to invert X if we've swapped the axes. I'd
>>               think we'd want to swap later if not last...
>
> yes, you're right, for absolute coords anyway. This definitly needs to be
> fixed.
>
> However, the order is important and the reason why it is that way is that you
> are guaranteed that InvertX will *always* invert the horizontal axis, even
> when the axes are swapped. I'd like to keep that behaviour.

OK, makes sense.

One other thing that occured to me: the EV_SYN events are largely ignored. This
means that a "pair" of X,Y movements are reported with as two X events --
movement along X then movement along Y for example -- rather than one, correct?
This is likely unnoticeable since the relative distance between these points are
likely too small -- especially on reliable and modern devices. However it still seems
like these input events should be accumulated in the pEvdev device and only
Post'ed when we see an EV_SYN. Has anyone ever tried this at some point?

>>       Should there be a set_valuators function?
>
> you mean to change the range after a client request? yes.
>

I also seem to recall seeing something about "default" or "initial" values also being
set through that interface. The purpose of those still mystifies me.

>>       Is the maximum number of axes MAX_VALUATORS or MAX_VALUATORS/6
>>               or can there be more? Digging through some of the core
>>               X bits left me unclear on these points...
>
> MAX_VALUATORS.
> MAX_VALUATORS/6 is the number of valuator events, each containing 6 valuators.
>
>> Some of these questions and TODOs could be left until later patches in
>> order to simplify review. It would be nice to know which of the TODOs
>> folks would prefer to see in separate patches. For example, perhaps
>> converting the REL axes should be a separate patch.
>
> The smaller the patches, the easier they are to review. Patches are cheap, so
> group any logical change into one patch.
>
> Just to give you an indication, in your last patch, I would have split the
> ABS_X change in the initial condition for the "for" loop into a separate
> patch

Wow, that's much more fine-grained than I expected.

Thanks!

Cheers,
    -Matt Helsley