[PATCH xf86-input-evdev 2/2 v2] Add support for masked valuators
Chase Douglas
chase.douglas at canonical.com
Sat Jan 22 13:26:35 PST 2011
On 01/05/2011 10:33 PM, Peter Hutterer wrote:
> On Wed, Jan 05, 2011 at 03:27:06PM -0500, Chase Douglas wrote:
>> With the X server now supporting masked valuators for XI2, enable
>> support in X evdev.
>>
>> This kills backwards compatibility with X Input ABI < 12
>
> thanks! we need some extra configure checks to prevent building against
> older servers, and once we do that, all current ABI_XINPUT checks can go
> too.
I've added a commit to do this.
>> /* convert to relative motion for touchpads */
>> if (pEvdev->abs_queued && (pEvdev->flags & EVDEV_RELATIVE_MODE)) {
>> if (pEvdev->in_proximity) {
>> - if (pEvdev->old_vals[0] != -1)
>> - pEvdev->delta[REL_X] = pEvdev->vals[0] - pEvdev->old_vals[0];
>> - if (pEvdev->old_vals[1] != -1)
>> - pEvdev->delta[REL_Y] = pEvdev->vals[1] - pEvdev->old_vals[1];
>> - if (pEvdev->abs_queued & ABS_X_VALUE)
>> - pEvdev->old_vals[0] = pEvdev->vals[0];
>> - if (pEvdev->abs_queued & ABS_Y_VALUE)
>> - pEvdev->old_vals[1] = pEvdev->vals[1];
>> + if (valuator_mask_isset(pEvdev->mask, 0) &&
>> + valuator_mask_isset(pEvdev->oldMask, 0))
>> + pEvdev->delta[REL_X] = valuator_mask_get(pEvdev->mask, 0) -
>> + valuator_mask_get(pEvdev->oldMask, 0);
>> + if (valuator_mask_isset(pEvdev->mask, 1) &&
>> + valuator_mask_isset(pEvdev->oldMask, 1))
>> + pEvdev->delta[REL_Y] = valuator_mask_get(pEvdev->mask, 1) -
>> + valuator_mask_get(pEvdev->oldMask, 1);
>> + if (valuator_mask_isset(pEvdev->mask, 0))
>> + valuator_mask_set(pEvdev->oldMask, 0,
>> + valuator_mask_get(pEvdev->mask, 0));
>> + if (valuator_mask_isset(pEvdev->mask, 1))
>> + valuator_mask_set(pEvdev->oldMask, 1,
>> + valuator_mask_get(pEvdev->mask, 1));
>
> please rearrange this into
>
> if (valuator_mask_isset(mask, 0))
> {
> if (valuator_mask_isset(oldmask, 0)
> delta = ...
> valuator_mask_set(oldMask)
> }
Done.
>> + for (i = 0; i <= 1; i++) {
>> + int val;
>>
>> - if (pEvdev->flags & EVDEV_CALIBRATED)
>> - {
>> - v[0] = xf86ScaleAxis(v[0],
>> - pEvdev->absinfo[ABS_X].maximum,
>> - pEvdev->absinfo[ABS_X].minimum,
>> - pEvdev->calibration.max_x, pEvdev->calibration.min_x);
>> - v[1] = xf86ScaleAxis(v[1],
>> - pEvdev->absinfo[ABS_Y].maximum,
>> - pEvdev->absinfo[ABS_Y].minimum,
>> - pEvdev->calibration.max_y, pEvdev->calibration.min_y);
>> - }
>> + if (!valuator_mask_isset(pEvdev->mask, i))
>> + continue;
>>
>> - if (pEvdev->invert_x)
>> - v[0] = (pEvdev->absinfo[ABS_X].maximum - v[0] +
>> - pEvdev->absinfo[ABS_X].minimum);
>> - if (pEvdev->invert_y)
>> - v[1] = (pEvdev->absinfo[ABS_Y].maximum - v[1] +
>> - pEvdev->absinfo[ABS_Y].minimum);
>> + val = valuator_mask_get(pEvdev->mask, i);
>
> val here has the same value as unswapped_x/y, so you can skip those two and
> just work with val.
val is retrieved during the loop. The first time through the loop this
would work. But then the x value will change for the second loop. Thus,
I use val as a temporary variable.
>> - *num_v = pEvdev->num_vals;
>> - *first_v = 0;
>> + if (pEvdev->swap_axes)
>> + val = xf86ScaleAxis((i == 0 ? unswapped_y : unswapped_x),
>> + pEvdev->absinfo[i].maximum,
>> + pEvdev->absinfo[i].minimum,
>> + pEvdev->absinfo[1 - i].maximum,
>> + pEvdev->absinfo[1 - i].minimum);
>> +
>> + if (pEvdev->flags & EVDEV_CALIBRATED)
>> + val = xf86ScaleAxis(val, pEvdev->absinfo[i].maximum,
>> + pEvdev->absinfo[i].minimum,
>> + (i == 0 ? pEvdev->calibration.max_x :
>> + pEvdev->calibration.max_y),
>> + (i == 0 ? pEvdev->calibration.min_x :
>> + pEvdev->calibration.min_y));
>
> please, no two-line ternery conditions in function calls. just use a temp
> variable here. mind you, if you want to get really fancy you can change
> calibration into a struct { int min; int max; } calibration[2]. but I'm not
> sure how much that gains us here besides saving one line.
I added temporary variables.
>> +
>> + if ((i == 0 && pEvdev->invert_x) || (i == 1 && pEvdev->invert_y))
>> + val = (pEvdev->absinfo[i].maximum - val +
>> + pEvdev->absinfo[i].minimum);
>> +
>> + valuator_mask_set(pEvdev->mask, i, val);
>> + }
>> }
>> }
>>
>> @@ -518,11 +502,15 @@ EvdevProcessProximityState(InputInfoPtr pInfo)
>> int prox_state = 0;
>> int i;
>>
>> + /* Does this device have any proximity keys? */
>
> s/keys/axes/
Done.
>> pEvdev->num_vals = num_axes;
>> - memset(pEvdev->vals, 0, num_axes * sizeof(int));
>> - memset(pEvdev->old_vals, -1, num_axes * sizeof(int));
>> + if (num_axes > 0) {
>> + pEvdev->mask = valuator_mask_new(num_axes);
>> + if (!pEvdev->mask) {
>> + xf86Msg(X_ERROR, "%s: failed to allocate valuator mask.\n",
>> + device->name);
>> + return !Success;
>
> shouldn't this be goto out as well?
Sure.
>> + }
>> + pEvdev->oldMask = valuator_mask_new(num_axes);
>> + if (!pEvdev->oldMask) {
>> + xf86Msg(X_ERROR, "%s: failed to allocate old valuator mask.\n",
>> + device->name);
>
> imo, one error message for both mask and oldMask is enough (mind you, if we
> fail here we probably topple soon anyway).
Yeah, I've simplified it.
>> @@ -1488,6 +1510,11 @@ EvdevAddRelClass(DeviceIntPtr device)
>> free(atoms);
>>
>> return Success;
>> +
>> +out:
>> + free(pEvdev->mask);
>> + pEvdev->mask = NULL;
>> + return !Success;
>> }
>
> please split the "goto out" parts into a separate patch that the valuator
> mask patch only contains valuator mask portions.
Done.
>>
>> static int
>> @@ -1759,6 +1786,9 @@ EvdevProc(DeviceIntPtr device, int what)
>> close(pInfo->fd);
>> pInfo->fd = -1;
>> }
>> + free(pEvdev->mask);
>> + free(pEvdev->oldMask);
>> + free(pEvdev->proxMask);
>
> seeing this, I just realised we need a valuator_mask_free() in the server,
> otherwise the whole point of making the valuator mask opague goes away and
> we're stuck with a single memory area for the mask. patch is out.
I've switched this now.
>> diff --git a/src/evdev.h b/src/evdev.h
>> index f640fdd..6af145f 100644
>> --- a/src/evdev.h
>> +++ b/src/evdev.h
>> @@ -121,8 +121,9 @@ typedef struct {
>>
>> int num_vals; /* number of valuators */
>> int axis_map[max(ABS_CNT, REL_CNT)]; /* Map evdev <axis> to index */
>> - int vals[MAX_VALUATORS];
>> - int old_vals[MAX_VALUATORS]; /* Translate absolute inputs to relative */
>> + ValuatorMask *mask;
>> + ValuatorMask *oldMask;
>> + ValuatorMask *proxMask;
>
> this is possibly bikeshedding, but why not keep using the names vals and
> old_vals? the type changes, but it's still used as a valuator array (more or
> less). please add some comments as to what values the various masks hold, I
> have to admit the proximity mask confused me a bit.
>
> also, evdev is trying to follow a foo_bar naming convention internally where
> possible.
Ok, fixed up.
Thanks for the review! I will send the patches out shortly.
-- Chase
More information about the xorg-devel
mailing list