[PATCH evdev 5/5] Keep the states of multitouch events

Benjamin Tissoires tissoire at cena.fr
Tue Apr 13 02:34:47 PDT 2010


>> --- a/src/evdev.c
>> +++ b/src/evdev.c
>> @@ -556,12 +556,10 @@ EvdevProcessAbsoluteMotionEvent(InputInfoPtr pInfo, struct input_event *ev)
>>
>>       if (ev->code<  ABS_MT_TOUCH_MAJOR)
>>           pEvdev->vals[pEvdev->axis_map[ev->code]] = value;
>> -    else if (pEvdev->current_num_multitouch<  EVDEV_MAX_TOUCHPOINTS) {
>> -        /* MT value ->  store it at the first available place */
>> -        pEvdev->vals[pEvdev->axis_map[ev->code] +
>> -                        pEvdev->current_num_multitouch * pEvdev->mt_num_valuators] = value;
>> -    } else
>> -        return; /* mt-event, but not enough place to store it */
>> +    else
>> +        /* multitouch event */
>> +        pEvdev->mt_current_vals[pEvdev->axis_map[ev->code] -
>> +                pEvdev->mt_first_axis] = value;
>
> that took me a bit to understand so I think a comment (and exhaustive commit
> message) would definitely be in order here. If I understand this correctly,
>
> mt_current_vals only constains the values of the _current_ touchpoint. these
> values are copied out at MT_SYNC and they are then overwritten at the next
> touchpoint. correct?
>

you're right

>
>>       if (ev->code == ABS_X)
>>           pEvdev->abs |= ABS_X_VALUE;
>> @@ -692,6 +690,7 @@ EvdevProcessSyncEvent(InputInfoPtr pInfo, struct input_event *ev)
>>       int num_v = 0, first_v = 0;
>>       int v[MAX_VALUATORS];
>>       EvdevPtr pEvdev = pInfo->private;
>> +    int i;
>>
>>       EvdevProcessValuators(pInfo, v,&num_v,&first_v);
>>
>> @@ -704,7 +703,17 @@ EvdevProcessSyncEvent(InputInfoPtr pInfo, struct input_event *ev)
>>       pEvdev->num_queue = 0;
>>       pEvdev->abs = 0;
>>       pEvdev->rel = 0;
>> -    pEvdev->current_num_multitouch = 0;
>> +
>> +    if (pEvdev->flags&  EVDEV_MULTITOUCH) {
>> +        pEvdev->current_num_multitouch = 0;
>> +
>> +        /* clean up known multitouch */
>> +        for (i = 0; i<  EVDEV_MAX_TOUCHPOINTS; i++) {
>> +            if (!pEvdev->known_touches[i].used)
>> +                pEvdev->known_touches[i].id = -1;
>> +            pEvdev->known_touches[i].used = FALSE;
>> +        }
>> +    }
>>   }
>>
>>   /**
>> @@ -714,7 +723,48 @@ static void
>>   EvdevProcessMTSyncReport(InputInfoPtr pInfo, struct input_event *ev)
>>   {
>>       EvdevPtr pEvdev = pInfo->private;
>> -    pEvdev->current_num_multitouch++;
>> +    EvdevTouchPtr pTouch = NULL;
>> +
>> +    /* find the previously associated MT */
>> +    if (pEvdev->axis_map[ABS_MT_TRACKING_ID]>  -1) {
>> +        /* the tracking ID is given by the device */
>> +        int i;
>> +        int id = pEvdev->mt_current_vals[
>> +            pEvdev->axis_map[ABS_MT_TRACKING_ID] - pEvdev->mt_first_axis];
>
> something strikes me as odd here. Long term I don't think we should be
> assigning ABS_MT_TRACKING_ID an axis mapping and maybe special treat it.
> because if I read this correctly, you're relying on the mapping as an
> indicator of whether it's there.
> Instead, I'd rather you detect it separately and then claim in the log that
> "no tracking ID, can't do multitouch" or so.

The detection of ABS_MT_TRACKING_ID is not very well chosen I agree 
(should rely on a flag). I made the patch this way in order to be able 
to track touches if the device does not send the trackingID (in a later 
patch), but as I read it, I think I should have made it better.

The solution you give after (return if no trackingID) is much simpler 
and won't interfere with later patches.

>
>> +
>> +        /* Get a TouchDataRec to keep the state of the MT-event */
>> +        /* first, find if the id was previously assigned */
>> +        for (i = 0; i<  EVDEV_MAX_TOUCHPOINTS; i++) {
>> +            if (pEvdev->known_touches[i].id == id) {
>> +                pTouch =&pEvdev->known_touches[i];
>> +                break;
>> +            }
>> +        }
>> +
>> +        if (!pTouch) {
>> +            /* the TouchDataRec was not found, look at an unassigned one */
>> +            for (i = 0; i<  EVDEV_MAX_TOUCHPOINTS; i++) {
>> +                if (pEvdev->known_touches[i].id<= 0) {
>> +                    pTouch =&pEvdev->known_touches[i];
>> +                    pTouch->id = id;
>> +                    break;
>> +                }
>> +            }
>> +        }
>> +
>> +        if (pTouch) {
>> +            pTouch->used = TRUE;
>> +            pTouch->position = pEvdev->current_num_multitouch; /* currently as mask for valuators is non available */
>> +        }
>> +    }
>> +
>> +    if (pTouch) {
>> +        memcpy(pEvdev->vals + pEvdev->mt_first_axis +
>> +                pTouch->position * pEvdev->mt_num_valuators,
>> +                pEvdev->mt_current_vals,
>> +                sizeof(int) * pEvdev->mt_num_valuators);
>> +        pEvdev->current_num_multitouch++;
>> +    }
>
> this one can move inside the if. or we just return early if
> [ABS_MT_TRACKING_ID]>  -1.
>
>>   }
>>
>>   /**
>> @@ -2091,6 +2141,7 @@ EvdevPreInit(InputDriverPtr drv, IDevPtr dev, int flags)
>>       const char *device, *str;
>>       int num_calibration = 0, calibration[4] = { 0, 0, 0, 0 };
>>       EvdevPtr pEvdev;
>> +    int i;
>>
>>       if (!(pInfo = xf86AllocateInput(drv, 0)))
>>   	return NULL;
>> @@ -2188,6 +2239,12 @@ EvdevPreInit(InputDriverPtr drv, IDevPtr dev, int flags)
>>           return NULL;
>>       }
>>
>> +    /* mt-related */
>
> you can just skip this comment.
>
>> +    for (i = 0; i<  EVDEV_MAX_TOUCHPOINTS; i++) {
>> +        pEvdev->known_touches[i].id = 0;
>> +        pEvdev->known_touches[i].used = FALSE;
>> +    }
>> +
>>       EvdevAddDevice(pInfo);
>>
>>       if (pEvdev->flags&  EVDEV_BUTTON_EVENTS)
>> diff --git a/src/evdev.h b/src/evdev.h
>> index 0dba366..9b5c1cd 100644
>> --- a/src/evdev.h
>> +++ b/src/evdev.h
>> @@ -116,6 +116,13 @@ typedef struct {
>>       int val;		/* State of the key/button; pressed or released. */
>>   } EventQueueRec, *EventQueuePtr;
>>
>> +/* Touch record to store if a touch was presviously reported */
>
> typo
>
>> +typedef struct {
>> +    int id; /* the tracking ID of the touch (>0 means touch alive) */
>> +    BOOL used; /* true if seen in the current frame (between two SYNC_EVENT) */
>
> maybe "is_present" instead? or "is_active"?
>
>> +    int position; /* the position in the collection of touches */
>> +} EvdevTouchRec, *EvdevTouchPtr;
>> +
>>   typedef struct {
>>       const char *device;
>>       int grabDevice;         /* grab the event device? */
>> @@ -199,6 +206,10 @@ typedef struct {
>>       unsigned int mt_first_axis;
>>       unsigned int mt_num_valuators;
>>       unsigned int current_num_multitouch;
>> +
>> +    /* a list of the current touches */
>> +    EvdevTouchRec known_touches[EVDEV_MAX_TOUCHPOINTS];
>
> please add an mt_ prefix.
>
>> +    int mt_current_vals[ABS_CNT - ABS_MT_TOUCH_MAJOR];
>>   } EvdevRec, *EvdevPtr;
>>
>>   /* Event posting functions */
>> --
>> 1.6.6.1
>
> Cheers,
>    Peter
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel


More information about the xorg-devel mailing list