Merge lp:~oif-team/frame/semi-mt-add-touches into lp:frame

[Chase Douglas]

Add the correct number of additional fingers for semi-mt devices. This allows up to three touch gestures on semi-mt synaptics trackpads. I can now perform some unity gestures :).

The new contact has a unique ID and is always placed at the center of the current bounding box. This ensures calculations based on the geometric center of the touches are unaffected.

[Henrik Rydberg (rydberg)]

I like the idea, just some comments on the implementation:

The EV_KEY code could be broken out into its own function, like for EV_ABS events.
Also, the number of active contacts needs to be set also for one touch, etc.

Reusing older touch ids could work, but note that the one chosen is likely to have been
used recently. Also, the tracking id range of the device should be respected. An alternative
is to pick an offset somewhere in the middle of the given tracking range.

review: Needs Fixing

[Chase Douglas (chasedouglas)]

I'll split out the key code into a separate function.

I think I can set the number of semi-mt touches to 1 by watching BTN_TOOL_FINGER. I had forgotten about its usage being equivalent to BTN_TOOL_*TAP. Note that it's not really necessary, however, since the number of semi-mt touches has an effect only when there are three or more touches.

I'm not sure what the best resolution to the touch ids issue is. It's true that the current solution will choose a recently used touch id, but this shouldn't really be an issue because the utouch-frame "protocol" tells the client when the touch has ended, right? The real issue is that we will be trampling on top of the touch id range reported by the kernel, but there's no way around this without creating a separate tracking id implementation in utouch-frame.

We could choose an id that's half way around the ring if you consider the tracking ids as a circular ring buffer. This would still be ~2^15 touches away, so there's low likelyhood of a tracking id collision. Would that be alright with you?

[Henrik Rydberg (rydberg)]

Sounds all good, thanks!

[lp:~oif-team/frame/semi-mt-add-touches]

32. By Chase Douglas on 2011-04-04

Split out EV_KEY handling and add support for BTN_TOOL_FINGER

[Chase Douglas (chasedouglas)]

I've added all the changes except for the tracking id value change. The issue is that the tracking IDs from utouch-frame don't have a boundary. If you're using the mtdev backend there is the range from the kernel, but this isn't exposed to the utouch-frame clients. As such, the only boundaries are that the tracking ID fits inside an int (32 bits) and is not equal to -1.

Henrik, how do you want to resolve this issue? Should we assume the valid tracking ID range is 0 to 2^32 - 2, 0 to 2^31 - 1 (all positive values of an int), or something else? I'll note that the XI 2.1 spec says it's an unsigned int that can take any value, if that's of any help in determining the best resolution here.

[Henrik Rydberg (rydberg)]

Having utouch-frame expose the tracking id range seems like a reasonable and simple solution.

[lp:~oif-team/frame/semi-mt-add-touches]

33. By Chase Douglas on 2011-04-13

Provide the range of touch IDs and decouple ID from touch active state

The kernel provides a range of touch IDs for each device, and XI 2.1 will
have its own range as well. The tracking ID from the kernel is set to -1
when the touch goes inactive, but -1 (as an unsigned int) is a valid
value for XI 2.1. To make things work in both environments, the tracking
ID range is provided in the touch surface, and the active state has been
decoupled from the tracking ID value. The backend (mtdev or XI 2.1) must
now set the active value of the touch contact.

Also, to provide for the full range of XI 2.1 touch IDs, the ID value has
been changed to unsigned int.

34. By Chase Douglas on 2011-04-13

Set the touch ID half way around the ID range to be safe

[Chase Douglas (chasedouglas)]

Please review the last two commits which I hope will resolve all outstanding issues.

Thanks!

[Henrik Rydberg (rydberg)]

Thanks for the changes, some comments:

* I would prefer clean patches before the changes on top.

* I (again) regret admitting to changing the original kernel protocol to allow zero as a tracking id... :-)

* The semantic change regarding id is fine per se, but needs a lot of thought and checks, because it is, indeed, an ABI break. I will have to get back to you on this one.

* The logic for number of fingers still seems to not handle the full range.

review: Needs Fixing

[Henrik Rydberg (rydberg)]

Regarding the tracking id range, I think the proper solution is to restrict the id range in the X server, using the exact same semantics as the kernel. The kernel protocol relies on there being a value outside the valid range, and using a different semantics in the X server obviously creates problems.

The original intent with the X tracking id was to mimic the kernel behavior for those kernel drivers with no or poor implementation of kernel tracking ids. Since then, the kernel code for tracking has been centralized, so all type B devices behave exactly the same. Maybe the special handling in X can be dropped now?

[Henrik Rydberg (rydberg)]

Following more IRC discussions, it is alright for utouch-frame to use the pair (active, id) instead of just id to codify touch status and identifier. Combined with the given id range, the api works for both mtdev and XI2.1.

For the next round, the users of utouch-frame (grail, mtview, tools) needs to be checked for special usage of id (in particular id == -1), since the proposed changes will introduce different semantics for non-active touch ids.

[Chase Douglas (chasedouglas)]

> Thanks for the changes, some comments:
>
> * I would prefer clean patches before the changes on top.

This is really hard to do with bzr and is just not how it's meant to be used. I've gone with this approach because it's easiest for me and allows one to see how review responses are addressed in the merge request review.

> * The semantic change regarding id is fine per se, but needs a lot of thought
> and checks, because it is, indeed, an ABI break. I will have to get back to
> you on this one.

Is this completely resolved now, or are there any other issues?

> * The logic for number of fingers still seems to not handle the full range.

I believe they are handled appropriately now. The evdev semi-mt number of touches is only provided through BTN_TOOL_FINGER, BTN_TOOL_DOUBLETAP, BTN_TOOL_TRIPLETAP, and BTN_TOOL_QUADTAP. What else is there to add?

Thanks!

[Henrik Rydberg (rydberg)]

> Is this completely resolved now, or are there any other issues?

Yep, outstanding issues, if any, should be dealt with in the users of this package instead.

> I believe they are handled appropriately now.

Right, it does look right for one finger too now. It would not hurt to simplify the logic a bit, though.

[lp:~oif-team/frame/semi-mt-add-touches]

35. By Chase Douglas on 2011-04-25

Clean up semi-mt touch count logic

By watching for BTN_TOUCH == 0 we can drop any logic handling for when
other semi-mt touch count events are sent with value 0.

[Chase Douglas (chasedouglas)]

> > I believe they are handled appropriately now.
>
> Right, it does look right for one finger too now. It would not hurt to
> simplify the logic a bit, though.

I had one idea on how the logic should work, and it was needed because I originally didn't handle BTN_TOUCH. The simplified logic in this last patch is a little easier to comprehend.

I hope this is good enough to merge now. Stephen noted a lot of people asking this past weekend for higher touch gestures in synaptics trackpads. With it merged we can at least give them the daily ppa to test with, and we can work on releasing the work and pushing it to the utouch stable ppa.

[Stephen M. Webb (bregma)]

Even I understand the simplified logic and I see expected data coming from the test tools when tested against actual hardware.

review: Approve

[Henrik Rydberg (rydberg)]

Found two problems with the current code - lin e 163, missing setting of active variable, and in the identifier code, possible wrap of values, resulting in close by use of identifiers. In addition, I would prefer if the semi-mt code was broken out in its own branch.

I have created a merge request to this branch, fixing the problems above.

[lp:~oif-team/frame/semi-mt-add-touches]

36. By Chase Douglas on 2011-04-26

Merge improvements from Henrik

[Chase Douglas (chasedouglas)]

Thanks Henrik! I merged your changes into the branch.

[lp:~oif-team/frame/semi-mt-add-touches]

37. By Henrik Rydberg on 2011-04-27

Merging in semi-mt-addons2 fixes

[Henrik Rydberg (rydberg)]

I think this branch is ready to be merged now.

review: Approve

=== modified file 'include/utouch/frame.h'
--- include/utouch/frame.h	2011-02-21 12:16:00 +0000
+++ include/utouch/frame.h	2011-04-27 12:16:41 +0000
@@ -94,6 +94,8 @@
 	float mapped_max_x;
 	float mapped_max_y;
 	float mapped_max_pressure;
+	unsigned int min_id;
+	unsigned int max_id;
 };
 
 #define UTOUCH_TOOL_FINGER	0
@@ -129,7 +131,7 @@
 	const struct utouch_contact *prev;
 	int active;
 	int slot;
-	int id;
+	unsigned int id;
 	int tool_type;
 	float x;
 	float y;

=== modified file 'src/frame-impl.h'
--- src/frame-impl.h	2011-02-15 14:01:51 +0000
+++ src/frame-impl.h	2011-04-27 12:16:41 +0000
@@ -35,6 +35,7 @@
 	struct utouch_frame *next;
 	int *evmap;
 	float map[9];
+	unsigned int semi_mt_num_active;
 };
 
 #endif

=== modified file 'src/frame-mtdev.c'
--- src/frame-mtdev.c	2011-02-24 14:47:40 +0000
+++ src/frame-mtdev.c	2011-04-27 12:16:41 +0000
@@ -92,6 +92,13 @@
 	s->use_distance = evemu_has_event(dev, EV_ABS, ABS_MT_DISTANCE);
 #endif
 
+	if (evemu_has_event(dev, EV_ABS, ABS_MT_TRACKING_ID)) {
+		s->min_id = evemu_get_abs_minimum(dev, ABS_MT_TRACKING_ID);
+		s->max_id = evemu_get_abs_maximum(dev, ABS_MT_TRACKING_ID);
+	} else {
+		s->min_id = MT_ID_MIN;
+		s->max_id = MT_ID_MAX;
+	}
 
 	s->min_x = evemu_get_abs_minimum(dev, ABS_MT_POSITION_X);
 	s->min_y = evemu_get_abs_minimum(dev, ABS_MT_POSITION_Y);
@@ -190,7 +197,45 @@
 		t->tool_type = ev->value;
 		return 1;
 	case ABS_MT_TRACKING_ID:
-		t->id = ev->value;
+		if (ev->value == -1)
+			t->active = 0;
+		else {
+			t->id = ev->value;
+			t->active = 1;
+		}
+		return 1;
+	default:
+		return 0;
+	}
+}
+
+static int handle_key_event(utouch_frame_handle fh,
+			    const struct input_event *ev)
+{
+	/* EV_KEY events are only used for semi-mt touch count */
+	if (!fh->surface->is_semi_mt)
+		return 0;
+
+	if (ev->code == BTN_TOUCH && ev->value == 0) {
+		fh->semi_mt_num_active = 0;
+		return 1;
+	}
+
+	if (ev->value == 0)
+		return 0;
+
+	switch (ev->code) {
+	case BTN_TOOL_FINGER:
+		fh->semi_mt_num_active = 1;
+		return 1;
+	case BTN_TOOL_DOUBLETAP:
+		fh->semi_mt_num_active = 2;
+		return 1;
+	case BTN_TOOL_TRIPLETAP:
+		fh->semi_mt_num_active = 3;
+		return 1;
+	case BTN_TOOL_QUADTAP:
+		fh->semi_mt_num_active = 4;
 		return 1;
 	default:
 		return 0;
@@ -212,6 +257,8 @@
 		f = utouch_frame_sync(fh, get_evtime_ms(ev));
 	else if (ev->type == EV_ABS)
 		handle_abs_event(fh, ev);
+	else if (ev->type == EV_KEY)
+		handle_key_event(fh, ev);
 
 	return f;
 }

=== modified file 'src/frame-xi2.c'
--- src/frame-xi2.c	2011-02-22 15:32:00 +0000
+++ src/frame-xi2.c	2011-04-27 12:16:41 +0000
@@ -27,6 +27,7 @@
 #include <errno.h>
 #include <math.h>
 #include <string.h>
+#include <limits.h>
 #include <X11/Xlib.h>
 
 static Atom transform_atom;
@@ -130,6 +131,8 @@
 	char *name;
 	int i;
 
+	s->min_id = 0;
+	s->max_id = UINT_MAX;
 	s->phys_width = 100;
 	s->phys_height = 65;
 	s->phys_pressure = 10;
@@ -306,8 +309,10 @@
 		for (i = 0; i < nbit; i++)
 			if (XIMaskIsSet(mask, i))
 				handle_event(fh, slot, i, value[num++]);
-		if (ev->evtype == XI_TouchEnd)
-			utouch_frame_get_current_slot(fh)->id = -1;
+		if (ev->evtype != XI_TouchBegin)
+			utouch_frame_get_current_slot(fh)->active = 1;
+		else if (ev->evtype == XI_TouchEnd)
+			utouch_frame_get_current_slot(fh)->active = 0;
 		return sync_xi2(fh, ev->time);
 	case XI_PropertyEvent:
 		prop_ev = (XIPropertyEvent *)ev;

=== modified file 'src/frame.c'
--- src/frame.c	2011-02-21 12:16:00 +0000
+++ src/frame.c	2011-04-27 12:16:41 +0000
@@ -90,7 +90,7 @@
 			goto out;
 		slots[i] = s;
 		s->slot = i;
-		s->id = -1;
+		s->active = 0;
 	}
 
 	return slots;
@@ -223,12 +223,13 @@
 
 	for (i = 0; i < fh->num_slots; i++) {
 		t = fh->next->slots[i];
-		if (t->id == id)
+		if (t->active && t->id == id)
 			return utouch_frame_set_current_slot(fh, i);
 	}
 	for (i = 0; i < fh->num_slots; i++) {
 		t = fh->next->slots[i];
-		if (t->id < 0) {
+		if (!t->active) {
+			t->active = 1;
 			t->id = id;
 			return utouch_frame_set_current_slot(fh, i);
 		}
@@ -264,7 +265,7 @@
 	struct utouch_surface *s = fh->surface;
 	const struct utouch_contact *ap = a->prev;
 
-	a->active = b->id != -1;
+	a->active = b->active;
 	a->id = b->id;
 	a->tool_type = b->tool_type;
 	a->x = b->x;
@@ -300,7 +301,8 @@
 static int detect_addrem(const struct utouch_contact *a,
 			 const struct utouch_contact *b)
 {
-	return a->id != b->id || a->tool_type != b->tool_type;
+	return a->id != b->id || a->tool_type != b->tool_type ||
+	       a->active != b->active;
 }
 
 static int detect_mod(const struct utouch_contact *a,
@@ -324,6 +326,34 @@
 	return tv.tv_usec / ms + tv.tv_sec * ms;
 }
 
+static void set_semi_mt_touches(utouch_frame_handle fh)
+{
+	struct utouch_frame *next = fh->next;
+	struct utouch_contact *a = next->slots[0];
+	struct utouch_contact *b = next->slots[1];
+	struct utouch_surface *s = fh->surface;
+	/* use touch ids half way around the ID range */
+	unsigned int id = a->id + (s->max_id - s->min_id) / 2;
+	float x = (a->x + b->x) / 2;
+	float y = (a->y + b->y) / 2;
+	int i;
+
+	for (i = 2; i < fh->semi_mt_num_active; i++) {
+		struct utouch_contact *c = next->slots[i];
+
+		memset(c, 0, sizeof(struct utouch_contact));
+		c->active = 1;
+		if (id > s->max_id)
+			id = s->min_id;
+		c->id = id++;
+		c->x = x;
+		c->y = y;
+	}
+
+	for (i = fh->semi_mt_num_active; i < fh->num_slots; i++)
+		next->slots[i]->active = 0;
+}
+
 const struct utouch_frame *utouch_frame_sync(utouch_frame_handle fh,
 					     utouch_frame_time_t time)
 {
@@ -338,6 +368,9 @@
 	frame->num_active = 0;
 	dt = frame->time - prev->time;
 
+	if (fh->surface->is_semi_mt)
+		set_semi_mt_touches(fh);
+
 	for (i = 0; i < fh->num_slots; i++) {
 		struct utouch_contact *p = frame->slots[i];
 		struct utouch_contact *q = next->slots[i];