[RFC][PATCH] General axis valuator support
Peter Hutterer
peter.hutterer at who-t.net
Sun Jan 11 20:01:57 PST 2009
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.
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.
> 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.
> 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.
> I'd like to move EvdevCacheCompare() above EvdevProbe() -- it
> means we can get rid of all but the GRAB ioctls() from
> EvdevProbe().
sounds good.
> Put back the button-as-ABS_PRESSURE test.
>
> 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.
> 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.
> Should there be a switch_mode function someday?
yes.
> Should there be a set_valuators function?
you mean to change the range after a client request? yes.
> 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
Cheers,
Peter
> Index: xf86-input-evdev/src/evdev.c
> ===================================================================
> --- xf86-input-evdev.orig/src/evdev.c
> +++ xf86-input-evdev/src/evdev.c
> @@ -55,10 +55,35 @@
> #ifndef MAXDEVICES
> #include <inputstr.h> /* for MAX_DEVICES */
> #define MAXDEVICES MAX_DEVICES
> #endif
>
> +#define TestBit(bit, array) (array[(bit) / LONG_BITS]) & (1L << ((bit)
> % LONG_BITS))
> +
> +static size_t _CountBits(int x)
> +{
> + x = x - ((x >> 1) & 0x55555555); /* 01010101... */
> + x = (x & 0x33333333) + ((x >> 2) & 0x33333333); /* 00110011... */
> + x = x + (x >> 4) & 0x0f0f0f0f;/* 0000111100001111... */
> + return (x * 0x01010101) >> 24;
> +}
> +
> +static size_t CountBits(unsigned long *array, size_t nlongs)
> +{
> + unsigned int i;
> + size_t count;
> +
> + for (i = 0; i < nlongs; i++) {
> + unsigned long x = array[i];
> +
> + count += _CountBits(x);
> + if (sizeof(long) > sizeof(int))
> + count += _CountBits(x >> (sizeof(int)*8));
> + }
> + return count;
> +}
> +
> /* 2.4 compatibility */
> #ifndef EVIOCGRAB
> #define EVIOCGRAB _IOW('E', 0x90, int)
> #endif
>
> @@ -375,24 +400,14 @@ EvdevReadInput(InputInfoPtr pInfo)
> break;
> }
> break;
>
> case EV_ABS:
> - switch (ev.code) {
> - case ABS_X:
> - pEvdev->abs_x = value;
> - abs = 1;
> - break;
> - case ABS_Y:
> - pEvdev->abs_y = value;
> - abs = 1;
> - break;
> - case ABS_PRESSURE:
> - pEvdev->abs_p = value;
> - abs = 1;
> - break;
> - }
> + if (ev.code > ABS_MAX)
> + break;
> + pEvdev->vals[pEvdev->axis_map[ev.code]] = value;
> + abs = 1;
> break;
>
> case EV_KEY:
> /* don't repeat mouse buttons */
> if (ev.code >= BTN_MOUSE && ev.code < KEY_OK)
> @@ -442,21 +457,21 @@ EvdevReadInput(InputInfoPtr pInfo)
> break;
> }
> }
>
> /* convert to relative motion for touchpads */
> - if (pEvdev->flags & EVDEV_TOUCHPAD) {
> + if (abs && (pEvdev->flags & EVDEV_TOUCHPAD)) {
> abs = 0;
> if (pEvdev->tool) { /* meaning, touch is active */
> - if (pEvdev->old_x != -1)
> - dx = pEvdev->abs_x - pEvdev->old_x;
> - if (pEvdev->old_y != -1)
> - dy = pEvdev->abs_y - pEvdev->old_y;
> - pEvdev->old_x = pEvdev->abs_x;
> - pEvdev->old_y = pEvdev->abs_y;
> + if (pEvdev->old_vals[0] != -1)
> + dx = pEvdev->vals[0] - pEvdev->old_vals[0];
> + if (pEvdev->old_vals[1] != -1)
> + dy = pEvdev->vals[1] - pEvdev->old_vals[1];
> + pEvdev->old_vals[0] = pEvdev->vals[0];
> + pEvdev->old_vals[1] = pEvdev->vals[1];
> } else {
> - pEvdev->old_x = pEvdev->old_y = -1;
> + pEvdev->old_vals[0] = pEvdev->old_vals[1] = -1;
> }
> }
>
> if (dx != 0 || dy != 0) {
> if (pEvdev->swap_axes) {
> @@ -479,32 +494,39 @@ EvdevReadInput(InputInfoPtr pInfo)
> * pEvdev->digi here, lets us ignore that event. pEvdev is
> * initialized to 1 so devices that doesn't use this scheme still
> * just works.
> */
> if (abs && pEvdev->tool) {
> - int v[3];
> - v[0] = (pEvdev->swap_axes) ? pEvdev->abs_y : pEvdev->abs_x;
> - v[1] = (pEvdev->swap_axes) ? pEvdev->abs_x : pEvdev->abs_y;
> - v[2] = pEvdev->abs_p;
> + int v[MAX_VALUATORS];
> +
> + memcpy(v, pEvdev->vals, sizeof(int)*pEvdev->num_vals);
> + if (pEvdev->swap_axes) {
> + v[0] ^= v[1];
> + v[1] ^= v[0];
> + v[0] ^= v[1];
> + }
>
> if (pEvdev->flags & EVDEV_CALIBRATED)
> {
> v[0] = xf86ScaleAxis(v[0],
> - pEvdev->max_x, pEvdev->min_x,
> + pEvdev->absinfo[ABS_X].maximum,
> + pEvdev->absinfo[ABS_X].minimum,
> pEvdev->calibration.max_x,
> pEvdev->calibration.min_x);
> v[1] = xf86ScaleAxis(v[1],
> - pEvdev->max_y, pEvdev->min_y,
> + pEvdev->absinfo[ABS_Y].maximum,
> + pEvdev->absinfo[ABS_Y].minimum,
> pEvdev->calibration.max_y,
> pEvdev->calibration.min_y);
> }
>
> if (pEvdev->invert_x)
> - v[0] = pEvdev->max_x - (v[0] - pEvdev->min_x);
> + v[0] = (pEvdev->absinfo[ABS_X].maximum - v[0] +
> + pEvdev->absinfo[ABS_X].minimum);
> if (pEvdev->invert_y)
> - v[1] = pEvdev->max_y - (v[1] - pEvdev->min_y);
> + v[1] = (pEvdev->absinfo[ABS_Y].maximum - v[1] +
> + pEvdev->absinfo[ABS_Y].minimum);
>
> - xf86PostMotionEventP(pInfo->dev, TRUE, 0,
> - 2 + (pEvdev->has_pressure ? 1 : 0), v);
> + xf86PostMotionEventP(pInfo->dev, TRUE, 0, pEvdev->num_vals, v);
> }
> }
>
> #define TestBit(bit, array) (array[(bit) / LONG_BITS]) & (1L << ((bit)
> % LONG_BITS))
>
> @@ -884,76 +906,64 @@ EvdevAddKeyClass(DeviceIntPtr device)
> static int
> EvdevAddAbsClass(DeviceIntPtr device)
> {
> InputInfoPtr pInfo;
> EvdevPtr pEvdev;
> - struct input_absinfo absinfo_x, absinfo_y, absinfo_p;
> - int num_valuators = 2;
> + int num_axes, i, axis;
>
> pInfo = device->public.devicePrivate;
> pEvdev = pInfo->private;
>
> - if (ioctl(pInfo->fd,
> - EVIOCGABS(ABS_X), &absinfo_x) < 0) {
> - xf86Msg(X_ERROR, "ioctl EVIOCGABS failed: %s\n", strerror(errno));
> - return !Success;
> - }
> + if (!(pEvdev->bitmask & EV_ABS))
> + return !Success;
> + num_axes = CountBits(pEvdev->abs_bitmask, NLONGS(ABS_MAX));
> + if (num_axes < 1)
> + return !Success;
> + pEvdev->num_vals = num_axes;
> + memset(pEvdev->vals, 0, num_axes*sizeof(int));
> + memset(pEvdev->old_vals, -1, num_axes*sizeof(int));
>
> - if (ioctl(pInfo->fd,
> - EVIOCGABS(ABS_Y), &absinfo_y) < 0) {
> - xf86Msg(X_ERROR, "ioctl EVIOCGABS failed: %s\n", strerror(errno));
> - return !Success;
> - }
> -
> - pEvdev->min_x = absinfo_x.minimum;
> - pEvdev->max_x = absinfo_x.maximum;
> - pEvdev->min_y = absinfo_y.minimum;
> - pEvdev->max_y = absinfo_y.maximum;
> -
> - if (pEvdev->has_pressure &&
> - ioctl(pInfo->fd, EVIOCGABS(ABS_PRESSURE), &absinfo_p) < 0) {
> - xf86Msg(X_ERROR, "ioctl EVIOCGABS ABS_PRESSURE failed: %s\n",
> - strerror(errno));
> - return !Success;
> - }
> -
> - if (pEvdev->has_pressure) {
> - num_valuators++;
> - pEvdev->max_p = absinfo_p.maximum;
> - pEvdev->min_p = absinfo_p.minimum;
> - }
> -
> -
> - if (!InitValuatorClassDeviceStruct(device, num_valuators,
> + if (!InitValuatorClassDeviceStruct(device, num_axes,
> #if GET_ABI_MAJOR(ABI_XINPUT_VERSION) < 3
> GetMotionHistory,
> #endif
> GetMotionHistorySize(),
> Absolute))
> return !Success;
> -
> - /* X valuator */
> - xf86InitValuatorAxisStruct(device, 0, pEvdev->min_x, pEvdev->max_x,
> - 10000, 0, 10000);
> - xf86InitValuatorDefaults(device, 0);
> -
> - /* Y valuator */
> - xf86InitValuatorAxisStruct(device, 1, pEvdev->min_y, pEvdev->max_y,
> - 10000, 0, 10000);
> - xf86InitValuatorDefaults(device, 1);
> -
> - if (pEvdev->has_pressure) {
> - xf86InitValuatorAxisStruct(device, 2, pEvdev->min_p,
> pEvdev->max_p,
> + for (axis = ABS_X; axis <= ABS_MAX; axis++) {
> + pEvdev->axis_map[axis] = -1;
> + if (!TestBit(axis, pEvdev->abs_bitmask))
> + continue;
> + pEvdev->axis_map[axis] = i;
> + xf86InitValuatorAxisStruct(device, i,
> + pEvdev->absinfo[axis].minimum,
> + pEvdev->absinfo[axis].maximum,
> 10000, 0, 10000);
> - xf86InitValuatorDefaults(device, 2);
> + xf86InitValuatorDefaults(device, i);
> + pEvdev->old_vals[i] = -1;
> + i++;
> }
>
> xf86MotionHistoryAllocate(pInfo);
>
> if (!InitPtrFeedbackClassDeviceStruct(device, EvdevPtrCtrlProc))
> return !Success;
>
> - pInfo->flags |= XI86_POINTER_CAPABLE;
> + if ((TestBit(ABS_X, pEvdev->abs_bitmask) &&
> + TestBit(ABS_Y, pEvdev->abs_bitmask)) ||
> + (TestBit(ABS_RX, pEvdev->abs_bitmask) &&
> + TestBit(ABS_RY, pEvdev->abs_bitmask)) ||
> + (TestBit(ABS_HAT0X, pEvdev->abs_bitmask) &&
> + TestBit(ABS_HAT0Y, pEvdev->abs_bitmask)) ||
> + (TestBit(ABS_HAT1X, pEvdev->abs_bitmask) &&
> + TestBit(ABS_HAT1Y, pEvdev->abs_bitmask)) ||
> + (TestBit(ABS_HAT2X, pEvdev->abs_bitmask) &&
> + TestBit(ABS_HAT2Y, pEvdev->abs_bitmask)) ||
> + (TestBit(ABS_HAT3X, pEvdev->abs_bitmask) &&
> + TestBit(ABS_HAT3Y, pEvdev->abs_bitmask)) ||
> + (TestBit(ABS_TILT_X, pEvdev->abs_bitmask) &&
> + TestBit(ABS_TILT_Y, pEvdev->abs_bitmask)))
> + pInfo->flags |= XI86_POINTER_CAPABLE;
>
> return Success;
> }
>
> static int
> @@ -1393,35 +1403,25 @@ EvdevProbe(InputInfoPtr pInfo)
> }
>
> if (TestBit(ABS_X, abs_bitmask) && TestBit(ABS_Y, abs_bitmask)) {
> xf86Msg(X_INFO, "%s: Found x and y absolute axes\n",
> pInfo->name);
> pEvdev->flags |= EVDEV_ABSOLUTE_EVENTS;
> - if (!pEvdev->has_pressure && TestBit(BTN_TOUCH, key_bitmask)) {
> + if (!TestBit(ABS_PRESSURE, pEvdev->abs_bitmask) &&
> + TestBit(BTN_TOUCH, key_bitmask)) {
> if (num_buttons) {
> xf86Msg(X_INFO, "%s: Found absolute touchpad\n",
> pInfo->name);
> pEvdev->flags |= EVDEV_TOUCHPAD;
> - pEvdev->old_x = pEvdev->old_y = -1;
> + memset(pEvdev->old_vals, -1, sizeof(int)*pEvdev->num_vals);
> } else {
> xf86Msg(X_INFO, "%s: Found absolute touchscreen\n",
> pInfo->name);
> pEvdev->flags |= EVDEV_TOUCHSCREEN;
> pEvdev->flags |= EVDEV_BUTTON_EVENTS;
> }
> }
> has_axes = TRUE;
> }
>
> - if (TestBit(ABS_PRESSURE, abs_bitmask)) {
> - struct input_absinfo absinfo_p;
> -
> - /* More than two pressure levels indicate it's not a button */
> - if (ioctl(pInfo->fd,
> - EVIOCGABS(ABS_PRESSURE), &absinfo_p) == 0) {
> - if ((absinfo_p.maximum - absinfo_p.minimum) > 1)
> - pEvdev->has_pressure = TRUE;
> - }
> - }
> -
> for (i = 0; i < BTN_MISC; i++)
> if (TestBit(i, key_bitmask))
> break;
>
> if (i < BTN_MISC) {
> @@ -1431,11 +1431,11 @@ EvdevProbe(InputInfoPtr pInfo)
> }
>
> if (has_axes && num_buttons) {
> pInfo->flags |= XI86_POINTER_CAPABLE | XI86_SEND_DRAG_EVENTS |
> XI86_CONFIGURED;
> - if (pEvdev->has_pressure) {
> + if (TestBit(ABS_PRESSURE, pEvdev->abs_bitmask)) {
> xf86Msg(X_INFO, "%s: Configuring as tablet\n", pInfo->name);
> pInfo->type_name = XI_TABLET;
> } else {
> xf86Msg(X_INFO, "%s: Configuring as mouse\n", pInfo->name);
> pInfo->type_name = XI_MOUSE;
> @@ -1517,11 +1517,10 @@ EvdevPreInit(InputDriverPtr drv, IDevPtr
> /*
> * We initialize pEvdev->tool to 1 so that device that doesn't use
> * proximity will still report events.
> */
> pEvdev->tool = 1;
> - pEvdev->has_pressure = FALSE;
>
> device = xf86CheckStrOption(dev->commonOptions, "Device", NULL);
> if (!device) {
> xf86Msg(X_ERROR, "%s: No device specified.\n", pInfo->name);
> xf86DeleteInput(pInfo, 0);
> Index: xf86-input-evdev/src/evdev.h
> ===================================================================
> --- xf86-input-evdev.orig/src/evdev.h
> +++ xf86-input-evdev/src/evdev.h
> @@ -76,19 +76,22 @@ typedef struct {
>
> typedef struct {
> const char *device;
> int grabDevice; /* grab the event device? */
> int screen;
> - int min_x, min_y, max_x, max_y, min_p, max_p;
> - int abs_x, abs_y, abs_p, old_x, old_y;
> +
> + int num_vals;
> + char axis_map[max(ABS_CNT, REL_CNT)]; /* Map evdev <axis> to index
> */
> + int old_vals[MAX_VALUATORS]; /* Translate absolute inputs to
> relative */
> + int vals[MAX_VALUATORS];
> +
> int flags;
> int tool;
> int buttons; /* number of buttons */
> BOOL swap_axes;
> BOOL invert_x;
> BOOL invert_y;
> - BOOL has_pressure;
>
> /* XKB stuff has to be per-device rather than per-driver */
> int noXkb;
> #ifdef XKB
> char *xkb_rules;
More information about the xorg
mailing list