[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