All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] Input: synaptics - use firmware data for Cr-48
@ 2012-07-18 10:22 Chung-yih Wang
  2012-07-18 15:38 ` Chase Douglas
  0 siblings, 1 reply; 19+ messages in thread
From: Chung-yih Wang @ 2012-07-18 10:22 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Henrik Rydberg, Daniel Kurtz, Chase Douglas, JJ Ding,
	linux-input, linux-kernel, Chung-yih Wang

The profile sensor clickpad in a Cr-48 Chromebook does a reasonable job of
tracking individual fingers. This tracking isn't perfect, but, experiments
show that it works better than just passing "semi-mt" data to userspace,
and making userspace try to deduce where the fingers are given a bounding box.

This patch tries to report correct two-finger positions instead of the
{(min_x, min_y), (max_x, max_y)} for profile sensor clickpads on Cr-48
chromebooks. Note that this device's firmware always reports the higher
(smaller y) finger in the "sgm" packet, and the lower (larger y) finger in the
"agm" packet. Thus, when a new finger arrives on the pad, the kernel driver
uses a simple Euclidean distance measure to deduce which of the two new fingers
should keep the tracking ID of the previous single finger. Similarly, when one
finger is removed, the same measure is used to determine which finger remained
on the pad.

Signed-off-by: Chung-yih Wang <cywang@chromium.org>
---
 drivers/input/mouse/synaptics.c |   93 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 93 insertions(+), 0 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index d5b390f..0215c7c 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -45,6 +45,7 @@
 #define YMIN_NOMINAL 1408
 #define YMAX_NOMINAL 4448
 
+static bool cr48_profile_sensor;
 
 /*****************************************************************************
  *	Stuff we need even when we do not want native Synaptics support
@@ -1003,6 +1004,75 @@ static void synaptics_image_sensor_process(struct psmouse *psmouse,
 	priv->agm_pending = false;
 }
 
+static int synaptics_distsq(const struct input_mt_slot *slot,
+			    const struct synaptics_hw_state *hw)
+{
+	int slot_x = input_mt_get_value(slot, ABS_MT_POSITION_X);
+	int slot_y = input_mt_get_value(slot, ABS_MT_POSITION_Y);
+	int dx = hw->x - slot_x;
+	int dy = synaptics_invert_y(hw->y) - slot_y;
+	return dx * dx + dy * dy;
+}
+
+static bool synaptics_is_sgm_slot(const struct input_mt_slot *slot,
+				  const struct synaptics_hw_state *sgm,
+				  const struct synaptics_hw_state *agm)
+{
+	return (synaptics_distsq(slot, sgm) < synaptics_distsq(slot, agm));
+}
+
+static int synaptics_get_sgm_slot(const struct input_mt_slot *slots,
+				  const struct synaptics_hw_state *sgm)
+{
+	int distsq_slot0 = synaptics_distsq(&slots[0], sgm);
+	int distsq_slot1 = synaptics_distsq(&slots[1], sgm);
+	return (distsq_slot0 < distsq_slot1 ? 0 : 1);
+}
+
+static void synaptics_profile_sensor_process(struct psmouse *psmouse,
+					     struct synaptics_hw_state *sgm,
+					     int num_fingers)
+{
+	struct input_dev *dev = psmouse->dev;
+	struct synaptics_data *priv = psmouse->private;
+	struct synaptics_hw_state *agm = &priv->agm;
+	struct synaptics_mt_state mt_state;
+
+	/* Initialize using current mt_state (as updated by last agm) */
+	mt_state = agm->mt_state;
+
+	if (num_fingers >= 2) {
+		/* Get previous sgm slot if exists */
+		int sgm_slot = (mt_state.count != 0) ? mt_state.sgm : 0;
+		if (mt_state.count == 1) {
+			const struct input_mt_slot *mt = &dev->mt[sgm_slot];
+			if (!synaptics_is_sgm_slot(mt, sgm, agm))
+				sgm_slot = 1 - sgm_slot;
+		}
+		synaptics_report_slot(dev, sgm_slot, sgm);
+		synaptics_report_slot(dev, 1 - sgm_slot, agm);
+		synaptics_mt_state_set(&mt_state, num_fingers,
+				       sgm_slot, 1 - sgm_slot);
+	} else if (num_fingers == 1) {
+		int sgm_slot = (mt_state.count != 0) ? mt_state.sgm : 0;
+		if (mt_state.count >= 2)
+			sgm_slot = synaptics_get_sgm_slot(dev->mt, sgm);
+		synaptics_report_slot(dev, sgm_slot, sgm);
+		synaptics_report_slot(dev, 1 - sgm_slot, NULL);
+		synaptics_mt_state_set(&mt_state, 1, sgm_slot, -1);
+	} else {
+		synaptics_report_slot(dev, 0, NULL);
+		synaptics_report_slot(dev, 1, NULL);
+		synaptics_mt_state_set(&mt_state, 0, -1, -1);
+	}
+	/* Store updated mt_state */
+	priv->mt_state = agm->mt_state = mt_state;
+	/* Send the number of fingers reported by touchpad itself. */
+	input_mt_report_finger_count(dev, mt_state.count);
+	synaptics_report_buttons(psmouse, sgm);
+	input_sync(dev);
+}
+
 /*
  *  called for each full received packet from the touchpad
  */
@@ -1066,6 +1136,12 @@ static void synaptics_process_packet(struct psmouse *psmouse)
 		finger_width = 0;
 	}
 
+	if (cr48_profile_sensor) {
+		synaptics_profile_sensor_process(psmouse, &hw, num_fingers);
+		return;
+	}
+
+
 	if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c))
 		synaptics_report_semi_mt_data(dev, &hw, &priv->agm,
 					      num_fingers);
@@ -1227,6 +1303,9 @@ static void set_input_params(struct input_dev *dev, struct synaptics_data *priv)
 					ABS_MT_POSITION_Y);
 	}
 
+	if (cr48_profile_sensor)
+		input_set_abs_params(dev, ABS_MT_PRESSURE, 0, 255, 0, 0);
+
 	if (SYN_CAP_PALMDETECT(priv->capabilities))
 		input_set_abs_params(dev, ABS_TOOL_WIDTH, 0, 15, 0, 0);
 
@@ -1422,10 +1501,24 @@ static const struct dmi_system_id __initconst olpc_dmi_table[] = {
 	{ }
 };
 
+static const struct dmi_system_id __initconst cr48_dmi_table[] = {
+#if defined(CONFIG_DMI) && defined(CONFIG_X86)
+	{
+		/* Cr-48 Chromebook (Codename Mario) */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "IEC"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Mario"),
+		},
+	},
+#endif
+	{ }
+};
+
 void __init synaptics_module_init(void)
 {
 	impaired_toshiba_kbc = dmi_check_system(toshiba_dmi_table);
 	broken_olpc_ec = dmi_check_system(olpc_dmi_table);
+	cr48_profile_sensor = dmi_check_system(cr48_dmi_table);
 }
 
 static int __synaptics_init(struct psmouse *psmouse, bool absolute_mode)
-- 
1.7.7.3


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] Input: synaptics - use firmware data for Cr-48
  2012-07-18 10:22 [PATCH v2] Input: synaptics - use firmware data for Cr-48 Chung-yih Wang
@ 2012-07-18 15:38 ` Chase Douglas
       [not found]   ` <CAM2ehZbftDja6CBGjhL3Jp+30DtYJj+8_4e=_wWcj3pCDGD7AA@mail.gmail.com>
  0 siblings, 1 reply; 19+ messages in thread
From: Chase Douglas @ 2012-07-18 15:38 UTC (permalink / raw)
  To: Chung-yih Wang
  Cc: Dmitry Torokhov, Henrik Rydberg, Daniel Kurtz, JJ Ding,
	linux-input, linux-kernel

On 07/18/2012 03:22 AM, Chung-yih Wang wrote:
> The profile sensor clickpad in a Cr-48 Chromebook does a reasonable job of
> tracking individual fingers. This tracking isn't perfect, but, experiments
> show that it works better than just passing "semi-mt" data to userspace,
> and making userspace try to deduce where the fingers are given a bounding box.
>
> This patch tries to report correct two-finger positions instead of the
> {(min_x, min_y), (max_x, max_y)} for profile sensor clickpads on Cr-48
> chromebooks. Note that this device's firmware always reports the higher
> (smaller y) finger in the "sgm" packet, and the lower (larger y) finger in the
> "agm" packet. Thus, when a new finger arrives on the pad, the kernel driver
> uses a simple Euclidean distance measure to deduce which of the two new fingers
> should keep the tracking ID of the previous single finger. Similarly, when one
> finger is removed, the same measure is used to determine which finger remained
> on the pad.

Can it keep track of the touches as you rotate them past each other in 
both the X and Y axes? If not, then it should remain a semi-mt device. 
Even if you can guess which touch is which when a second touch is added, 
you will lose track of it when the user attempts to perform a rotation.

Semi-mt is our only mechanism for telling userspace that the device 
can't accurately tell us about rotations. We could create a new device 
property to say: "This device kinda sorta tells us enough info usually 
to know where two touches are initially." I don't think the effort is 
worth it though. What is the point of providing the exact locations on a 
trackpad if they can't be used for rotation?

-- Chase

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] Input: synaptics - use firmware data for Cr-48
       [not found]   ` <CAM2ehZbftDja6CBGjhL3Jp+30DtYJj+8_4e=_wWcj3pCDGD7AA@mail.gmail.com>
@ 2012-07-19  6:42       ` Dmitry Torokhov
  2012-07-19 13:14     ` Henrik Rydberg
  2012-07-19 16:16       ` Chase Douglas
  2 siblings, 0 replies; 19+ messages in thread
From: Dmitry Torokhov @ 2012-07-19  6:42 UTC (permalink / raw)
  To: Chung-Yih Wang (王崇懿)
  Cc: Chase Douglas, Henrik Rydberg, Daniel Kurtz, JJ Ding,
	linux-input, linux-kernel

Hi Chung-Yih,

On Thu, Jul 19, 2012 at 11:02:23AM +0800, Chung-Yih Wang (王崇懿) wrote:
> Hi Chase,
> 
>    Thanks for your great comments. You are right, it is impossible to get
>  correct finger tracking if both fingers are moving. However, we think it
> still worth to have the firmware tracking of the fingers as they could
> perform well for most one-stationary-one-moving cases. This will be good
> enough for the one-stationary-one-moving gestures we want to provide on
> Cr-48. And that's why we want to make the patch specific to Cr-48.
> 

First of all if the patch makes sense for Cr-48 then other boxes using
the same touchpad with the same sensor would also benefit from it.
However it is still not clear to me what coordinates are being reported
by the device? You say that it always reports finger with smaller Y in
SGM packet, but what about X coordinate? Could it be that the most
benefit from your patch is because it reports proper slot from 2->1
finger transitions?

BTW, you seem to have butchered single-touch protocol reporting for your
device.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] Input: synaptics - use firmware data for Cr-48
@ 2012-07-19  6:42       ` Dmitry Torokhov
  0 siblings, 0 replies; 19+ messages in thread
From: Dmitry Torokhov @ 2012-07-19  6:42 UTC (permalink / raw)
  To: Chung-Yih Wang (王崇懿)
  Cc: Chase Douglas, Henrik Rydberg, Daniel Kurtz, JJ Ding,
	linux-input, linux-kernel

Hi Chung-Yih,

On Thu, Jul 19, 2012 at 11:02:23AM +0800, Chung-Yih Wang (王崇懿) wrote:
> Hi Chase,
> 
>    Thanks for your great comments. You are right, it is impossible to get
>  correct finger tracking if both fingers are moving. However, we think it
> still worth to have the firmware tracking of the fingers as they could
> perform well for most one-stationary-one-moving cases. This will be good
> enough for the one-stationary-one-moving gestures we want to provide on
> Cr-48. And that's why we want to make the patch specific to Cr-48.
> 

First of all if the patch makes sense for Cr-48 then other boxes using
the same touchpad with the same sensor would also benefit from it.
However it is still not clear to me what coordinates are being reported
by the device? You say that it always reports finger with smaller Y in
SGM packet, but what about X coordinate? Could it be that the most
benefit from your patch is because it reports proper slot from 2->1
finger transitions?

BTW, you seem to have butchered single-touch protocol reporting for your
device.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] Input: synaptics - use firmware data for Cr-48
       [not found]   ` <CAM2ehZbftDja6CBGjhL3Jp+30DtYJj+8_4e=_wWcj3pCDGD7AA@mail.gmail.com>
  2012-07-19  6:42       ` Dmitry Torokhov
@ 2012-07-19 13:14     ` Henrik Rydberg
  2012-07-19 16:16       ` Chase Douglas
  2 siblings, 0 replies; 19+ messages in thread
From: Henrik Rydberg @ 2012-07-19 13:14 UTC (permalink / raw)
  To: Chung-Yih Wang (王崇懿)
  Cc: Chase Douglas, Dmitry Torokhov, Daniel Kurtz, JJ Ding,
	linux-input, linux-kernel

>    Thanks for your great comments. You are right, it is impossible to get
>  correct finger tracking if both fingers are moving. However, we think it
> still worth to have the firmware tracking of the fingers as they could
> perform well for most one-stationary-one-moving cases. This will be good
> enough for the one-stationary-one-moving gestures we want to provide on
> Cr-48. And that's why we want to make the patch specific to Cr-48.

If one finger is stationary, it is trivial to find out where the other
finger is using the available semi-mt data.

The general feeling about this patch is that it is very similar to
where we started off two years ago. The problems we saw then led to
the implementation of the semi-mt protocol. I doubt things have
changed much since then.

Thanks,
Henrik

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] Input: synaptics - use firmware data for Cr-48
       [not found]   ` <CAM2ehZbftDja6CBGjhL3Jp+30DtYJj+8_4e=_wWcj3pCDGD7AA@mail.gmail.com>
@ 2012-07-19 16:16       ` Chase Douglas
  2012-07-19 13:14     ` Henrik Rydberg
  2012-07-19 16:16       ` Chase Douglas
  2 siblings, 0 replies; 19+ messages in thread
From: Chase Douglas @ 2012-07-19 16:16 UTC (permalink / raw)
  To: "Chung-Yih Wang (王崇懿)"
  Cc: Dmitry Torokhov, Henrik Rydberg, Daniel Kurtz, JJ Ding,
	linux-input, linux-kernel

On 07/18/2012 08:02 PM, Chung-Yih Wang (王崇懿) wrote:
> Hi Chase,
>
>     Thanks for your great comments. You are right, it is impossible to
> get  correct finger tracking if both fingers are moving. However, we
> think it still worth to have the firmware tracking of the fingers as
> they could perform well for most one-stationary-one-moving cases. This
> will be good enough for the one-stationary-one-moving gestures we want
> to provide on Cr-48. And that's why we want to make the patch specific
> to Cr-48.

Can you provide more details on what you are attempting to accomplish? 
The only thing you can't do easily with semi-mt is:

* Rotations (but the hardware can't do it anyway, so this is moot)
* Pinch directions at 45 degree angles (i.e. (pinch up and left, down 
and right) or (pinch down and left, up and right))

You should be able to do everything else with semi-mt, such as pinch 
horizontally vs pinch vertically, movement, etc.

The pinch directions at 45 degree angles could be possible with the 
Cr-48 trackpad, but I would want to know that there's a real use case 
for it before undertaking an effort to support it.

-- Chase

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] Input: synaptics - use firmware data for Cr-48
@ 2012-07-19 16:16       ` Chase Douglas
  0 siblings, 0 replies; 19+ messages in thread
From: Chase Douglas @ 2012-07-19 16:16 UTC (permalink / raw)
  To: "Chung-Yih Wang (王崇懿)"
  Cc: Dmitry Torokhov, Henrik Rydberg, Daniel Kurtz, JJ Ding,
	linux-input, linux-kernel

On 07/18/2012 08:02 PM, Chung-Yih Wang (王崇懿) wrote:
> Hi Chase,
>
>     Thanks for your great comments. You are right, it is impossible to
> get  correct finger tracking if both fingers are moving. However, we
> think it still worth to have the firmware tracking of the fingers as
> they could perform well for most one-stationary-one-moving cases. This
> will be good enough for the one-stationary-one-moving gestures we want
> to provide on Cr-48. And that's why we want to make the patch specific
> to Cr-48.

Can you provide more details on what you are attempting to accomplish? 
The only thing you can't do easily with semi-mt is:

* Rotations (but the hardware can't do it anyway, so this is moot)
* Pinch directions at 45 degree angles (i.e. (pinch up and left, down 
and right) or (pinch down and left, up and right))

You should be able to do everything else with semi-mt, such as pinch 
horizontally vs pinch vertically, movement, etc.

The pinch directions at 45 degree angles could be possible with the 
Cr-48 trackpad, but I would want to know that there's a real use case 
for it before undertaking an effort to support it.

-- Chase
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] Input: synaptics - use firmware data for Cr-48
  2012-07-19 16:16       ` Chase Douglas
@ 2012-07-19 17:05         ` Daniel Kurtz
  -1 siblings, 0 replies; 19+ messages in thread
From: Daniel Kurtz @ 2012-07-19 17:05 UTC (permalink / raw)
  To: Chase Douglas
  Cc: "Chung-Yih Wang (王崇懿)",
	Dmitry Torokhov, Henrik Rydberg, JJ Ding, linux-input,
	linux-kernel

On Fri, Jul 20, 2012 at 12:16 AM, Chase Douglas
<chase.douglas@canonical.com> wrote:
>
> On 07/18/2012 08:02 PM, Chung-Yih Wang (王崇懿) wrote:
>>
>> Hi Chase,
>>
>>     Thanks for your great comments. You are right, it is impossible to
>> get  correct finger tracking if both fingers are moving. However, we
>> think it still worth to have the firmware tracking of the fingers as
>> they could perform well for most one-stationary-one-moving cases. This
>> will be good enough for the one-stationary-one-moving gestures we want
>> to provide on Cr-48. And that's why we want to make the patch specific
>> to Cr-48.
>
>
> Can you provide more details on what you are attempting to accomplish?

The Cr-48 profile sensor is also a Clickpad.  Thus, one of the most
common user gestures is to click the pad with one finger (to start a
selection) and then swipe a second (almost always "upper") finger
across the pad to extend the selection (or move a selected object).

The semi-mt approach is breaking down when the swiping finger moves
horizontally and "crosses over top" of a lower "stationary" finger.
For example, if a user clicks their finger in the bottom center of the
pad, and while using a second finger to horizontally extend a
selection, the fingers cross in the X direction.

As the moving finger approaches the same X coordinate as the
stationary finger, the reported position of the bottom finger will
start to move significantly towards the upper finger due to a
"pulling" affect of the profile sensor.  Thus, that bottom, stationary
finger starts to move towards the finger that is actually moving.
Eventually, when the moving finger gets close enough to the stationary
finger, the reported X coordinate of the two fingers becomes the same
- the two fingers' reported X coordinates "merge".  As the moving
finger continues to move towards, over and past the lower finger, the
reported position of the two fingers moves together, until the moving
finger gets significantly far enough away (in X) from the lower
finger, at which point the lower finger's reported position starts
moving back to its actual position.

Due to this effect, when using semi-mt, it is very difficult to know
at which point the "finger pattern" of the bounding box changes; in
other words, when the fingers change from "BottomLeft / TopRight" to
"BottomRight / TopLeft".  It can be approximated by assuming that the
"finger pattern" changes when the merged X coordinate passes over the
original starting point of the bottom finger.  However, this
approximation only holds when the bottom finger is perfectly
stationary.  In the real world, the bottom finger rolls/wiggles or
otherwise moves, causing the actual crossing point to change.  Or, as
is also likely, the lower finger is already being pulled when the
upper finger starts moving, so its reported position is already not at
the correct crossing point.  Thus, using semi-mt with this profile
sensor clickpad, we have not been able to generate smooth pointer
motion when an upper finger crosses a lower finger.

What we found, though, is that the firmware does do a much better job
of tracking such horizontal crosses.  Therefore, we would like to get
this raw data from the kernel.  It is trivial for userspace to convert
the raw finger position data to a bounding box format for use with
other gestures.

In any case, we really aren't that familiar with how other "synaptics
semi-mt-compatible" touchpads work.  Using this patch may or may not
be better than semi-mt.  Or, their firmware may or may not follow the
same "always report upper finger in sgm" rule (we have seen other
Synaptics trackpads that do "always report oldest finger in sgm").
This is why we chose to isolate this change to just one hardware type,
so that the change, which improves the experience of a Cr-48 user, has
no deleterious affect on users of any other hardware.

If others try this method with their hardware and it works for them,
then great!   We would be happy to help review additional patches that
extend it to additional systems.

-Daniel

> The only thing you can't do easily with semi-mt is:
>
> * Rotations (but the hardware can't do it anyway, so this is moot)
> * Pinch directions at 45 degree angles (i.e. (pinch up and left, down and
> right) or (pinch down and left, up and right))
>
> You should be able to do everything else with semi-mt, such as pinch
> horizontally vs pinch vertically, movement, etc.
>
> The pinch directions at 45 degree angles could be possible with the Cr-48
> trackpad, but I would want to know that there's a real use case for it
> before undertaking an effort to support it.
>
> -- Chase

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] Input: synaptics - use firmware data for Cr-48
@ 2012-07-19 17:05         ` Daniel Kurtz
  0 siblings, 0 replies; 19+ messages in thread
From: Daniel Kurtz @ 2012-07-19 17:05 UTC (permalink / raw)
  To: Chase Douglas
  Cc: "Chung-Yih Wang (王崇懿)",
	Dmitry Torokhov, Henrik Rydberg, JJ Ding, linux-input,
	linux-kernel

On Fri, Jul 20, 2012 at 12:16 AM, Chase Douglas
<chase.douglas@canonical.com> wrote:
>
> On 07/18/2012 08:02 PM, Chung-Yih Wang (王崇懿) wrote:
>>
>> Hi Chase,
>>
>>     Thanks for your great comments. You are right, it is impossible to
>> get  correct finger tracking if both fingers are moving. However, we
>> think it still worth to have the firmware tracking of the fingers as
>> they could perform well for most one-stationary-one-moving cases. This
>> will be good enough for the one-stationary-one-moving gestures we want
>> to provide on Cr-48. And that's why we want to make the patch specific
>> to Cr-48.
>
>
> Can you provide more details on what you are attempting to accomplish?

The Cr-48 profile sensor is also a Clickpad.  Thus, one of the most
common user gestures is to click the pad with one finger (to start a
selection) and then swipe a second (almost always "upper") finger
across the pad to extend the selection (or move a selected object).

The semi-mt approach is breaking down when the swiping finger moves
horizontally and "crosses over top" of a lower "stationary" finger.
For example, if a user clicks their finger in the bottom center of the
pad, and while using a second finger to horizontally extend a
selection, the fingers cross in the X direction.

As the moving finger approaches the same X coordinate as the
stationary finger, the reported position of the bottom finger will
start to move significantly towards the upper finger due to a
"pulling" affect of the profile sensor.  Thus, that bottom, stationary
finger starts to move towards the finger that is actually moving.
Eventually, when the moving finger gets close enough to the stationary
finger, the reported X coordinate of the two fingers becomes the same
- the two fingers' reported X coordinates "merge".  As the moving
finger continues to move towards, over and past the lower finger, the
reported position of the two fingers moves together, until the moving
finger gets significantly far enough away (in X) from the lower
finger, at which point the lower finger's reported position starts
moving back to its actual position.

Due to this effect, when using semi-mt, it is very difficult to know
at which point the "finger pattern" of the bounding box changes; in
other words, when the fingers change from "BottomLeft / TopRight" to
"BottomRight / TopLeft".  It can be approximated by assuming that the
"finger pattern" changes when the merged X coordinate passes over the
original starting point of the bottom finger.  However, this
approximation only holds when the bottom finger is perfectly
stationary.  In the real world, the bottom finger rolls/wiggles or
otherwise moves, causing the actual crossing point to change.  Or, as
is also likely, the lower finger is already being pulled when the
upper finger starts moving, so its reported position is already not at
the correct crossing point.  Thus, using semi-mt with this profile
sensor clickpad, we have not been able to generate smooth pointer
motion when an upper finger crosses a lower finger.

What we found, though, is that the firmware does do a much better job
of tracking such horizontal crosses.  Therefore, we would like to get
this raw data from the kernel.  It is trivial for userspace to convert
the raw finger position data to a bounding box format for use with
other gestures.

In any case, we really aren't that familiar with how other "synaptics
semi-mt-compatible" touchpads work.  Using this patch may or may not
be better than semi-mt.  Or, their firmware may or may not follow the
same "always report upper finger in sgm" rule (we have seen other
Synaptics trackpads that do "always report oldest finger in sgm").
This is why we chose to isolate this change to just one hardware type,
so that the change, which improves the experience of a Cr-48 user, has
no deleterious affect on users of any other hardware.

If others try this method with their hardware and it works for them,
then great!   We would be happy to help review additional patches that
extend it to additional systems.

-Daniel

> The only thing you can't do easily with semi-mt is:
>
> * Rotations (but the hardware can't do it anyway, so this is moot)
> * Pinch directions at 45 degree angles (i.e. (pinch up and left, down and
> right) or (pinch down and left, up and right))
>
> You should be able to do everything else with semi-mt, such as pinch
> horizontally vs pinch vertically, movement, etc.
>
> The pinch directions at 45 degree angles could be possible with the Cr-48
> trackpad, but I would want to know that there's a real use case for it
> before undertaking an effort to support it.
>
> -- Chase
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] Input: synaptics - use firmware data for Cr-48
  2012-07-19 17:05         ` Daniel Kurtz
@ 2012-07-19 17:34           ` Chase Douglas
  -1 siblings, 0 replies; 19+ messages in thread
From: Chase Douglas @ 2012-07-19 17:34 UTC (permalink / raw)
  To: Daniel Kurtz
  Cc: "Chung-Yih Wang (王崇懿)",
	Dmitry Torokhov, Henrik Rydberg, JJ Ding, linux-input,
	linux-kernel

On 07/19/2012 10:05 AM, Daniel Kurtz wrote:
> On Fri, Jul 20, 2012 at 12:16 AM, Chase Douglas
> <chase.douglas@canonical.com> wrote:
>>
>> On 07/18/2012 08:02 PM, Chung-Yih Wang (王崇懿) wrote:
>>>
>>> Hi Chase,
>>>
>>>      Thanks for your great comments. You are right, it is impossible to
>>> get  correct finger tracking if both fingers are moving. However, we
>>> think it still worth to have the firmware tracking of the fingers as
>>> they could perform well for most one-stationary-one-moving cases. This
>>> will be good enough for the one-stationary-one-moving gestures we want
>>> to provide on Cr-48. And that's why we want to make the patch specific
>>> to Cr-48.
>>
>>
>> Can you provide more details on what you are attempting to accomplish?
>
> The Cr-48 profile sensor is also a Clickpad.  Thus, one of the most
> common user gestures is to click the pad with one finger (to start a
> selection) and then swipe a second (almost always "upper") finger
> across the pad to extend the selection (or move a selected object).
>
> The semi-mt approach is breaking down when the swiping finger moves
> horizontally and "crosses over top" of a lower "stationary" finger.
> For example, if a user clicks their finger in the bottom center of the
> pad, and while using a second finger to horizontally extend a
> selection, the fingers cross in the X direction.
>
> As the moving finger approaches the same X coordinate as the
> stationary finger, the reported position of the bottom finger will
> start to move significantly towards the upper finger due to a
> "pulling" affect of the profile sensor.  Thus, that bottom, stationary
> finger starts to move towards the finger that is actually moving.
> Eventually, when the moving finger gets close enough to the stationary
> finger, the reported X coordinate of the two fingers becomes the same
> - the two fingers' reported X coordinates "merge".  As the moving
> finger continues to move towards, over and past the lower finger, the
> reported position of the two fingers moves together, until the moving
> finger gets significantly far enough away (in X) from the lower
> finger, at which point the lower finger's reported position starts
> moving back to its actual position.
>
> Due to this effect, when using semi-mt, it is very difficult to know
> at which point the "finger pattern" of the bounding box changes; in
> other words, when the fingers change from "BottomLeft / TopRight" to
> "BottomRight / TopLeft".  It can be approximated by assuming that the
> "finger pattern" changes when the merged X coordinate passes over the
> original starting point of the bottom finger.  However, this
> approximation only holds when the bottom finger is perfectly
> stationary.  In the real world, the bottom finger rolls/wiggles or
> otherwise moves, causing the actual crossing point to change.  Or, as
> is also likely, the lower finger is already being pulled when the
> upper finger starts moving, so its reported position is already not at
> the correct crossing point.  Thus, using semi-mt with this profile
> sensor clickpad, we have not been able to generate smooth pointer
> motion when an upper finger crosses a lower finger.
>
> What we found, though, is that the firmware does do a much better job
> of tracking such horizontal crosses.  Therefore, we would like to get
> this raw data from the kernel.  It is trivial for userspace to convert
> the raw finger position data to a bounding box format for use with
> other gestures.
>
> In any case, we really aren't that familiar with how other "synaptics
> semi-mt-compatible" touchpads work.  Using this patch may or may not
> be better than semi-mt.  Or, their firmware may or may not follow the
> same "always report upper finger in sgm" rule (we have seen other
> Synaptics trackpads that do "always report oldest finger in sgm").
> This is why we chose to isolate this change to just one hardware type,
> so that the change, which improves the experience of a Cr-48 user, has
> no deleterious affect on users of any other hardware.
>
> If others try this method with their hardware and it works for them,
> then great!   We would be happy to help review additional patches that
> extend it to additional systems.

Ok, that's very helpful to know, thanks for providing such a detailed 
response!

So the problem is that you want to support the kernel providing two 
touches, but the touch locations are only valid if they do not cross in 
the Y axis. That's a very nuanced property of the device, one that is 
only specific to certain touchpads from one manufacturer.

I understand the usefulness of this functionality, but I also worry 
about proliferating the number of properties for devices (there are only 
32 bits we can use, IIRC). I see four options off the top of my head:

* Don't do anything, leave it as SEMI_MT. Obviously this would suck, but 
it is an option.

* Make the trackpad advertise itself as *not* SEMI_MT. This would be 
broken, however, if the user performs a rotation where the touches cross 
in the Y axis. I feel this is plain wrong according to the stated 
protocol documentation and previous behavior, so I don't want to do this.

* Add a new device property (INVALID_Y_AXIS_CROSSING?) that describes 
the exact behavior of this device. I would be ok with this if everyone 
else is, but only because proper clickpad behavior (which I consider 
very importand) is broken without this knowledge.

* Leave the device as SEMI_MT, but provide the real locations, and allow 
userspace to determine the device vendor/model/etc. If userspace knows 
that a specific device behaves in a specific way, it can do its own 
quirking handling. Given the specificity of this behavior to only some 
devices of one brand, this would be my suggested resolution to the issue.

-- Chase

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] Input: synaptics - use firmware data for Cr-48
@ 2012-07-19 17:34           ` Chase Douglas
  0 siblings, 0 replies; 19+ messages in thread
From: Chase Douglas @ 2012-07-19 17:34 UTC (permalink / raw)
  To: Daniel Kurtz
  Cc: "Chung-Yih Wang (王崇懿)",
	Dmitry Torokhov, Henrik Rydberg, JJ Ding, linux-input,
	linux-kernel

On 07/19/2012 10:05 AM, Daniel Kurtz wrote:
> On Fri, Jul 20, 2012 at 12:16 AM, Chase Douglas
> <chase.douglas@canonical.com> wrote:
>>
>> On 07/18/2012 08:02 PM, Chung-Yih Wang (王崇懿) wrote:
>>>
>>> Hi Chase,
>>>
>>>      Thanks for your great comments. You are right, it is impossible to
>>> get  correct finger tracking if both fingers are moving. However, we
>>> think it still worth to have the firmware tracking of the fingers as
>>> they could perform well for most one-stationary-one-moving cases. This
>>> will be good enough for the one-stationary-one-moving gestures we want
>>> to provide on Cr-48. And that's why we want to make the patch specific
>>> to Cr-48.
>>
>>
>> Can you provide more details on what you are attempting to accomplish?
>
> The Cr-48 profile sensor is also a Clickpad.  Thus, one of the most
> common user gestures is to click the pad with one finger (to start a
> selection) and then swipe a second (almost always "upper") finger
> across the pad to extend the selection (or move a selected object).
>
> The semi-mt approach is breaking down when the swiping finger moves
> horizontally and "crosses over top" of a lower "stationary" finger.
> For example, if a user clicks their finger in the bottom center of the
> pad, and while using a second finger to horizontally extend a
> selection, the fingers cross in the X direction.
>
> As the moving finger approaches the same X coordinate as the
> stationary finger, the reported position of the bottom finger will
> start to move significantly towards the upper finger due to a
> "pulling" affect of the profile sensor.  Thus, that bottom, stationary
> finger starts to move towards the finger that is actually moving.
> Eventually, when the moving finger gets close enough to the stationary
> finger, the reported X coordinate of the two fingers becomes the same
> - the two fingers' reported X coordinates "merge".  As the moving
> finger continues to move towards, over and past the lower finger, the
> reported position of the two fingers moves together, until the moving
> finger gets significantly far enough away (in X) from the lower
> finger, at which point the lower finger's reported position starts
> moving back to its actual position.
>
> Due to this effect, when using semi-mt, it is very difficult to know
> at which point the "finger pattern" of the bounding box changes; in
> other words, when the fingers change from "BottomLeft / TopRight" to
> "BottomRight / TopLeft".  It can be approximated by assuming that the
> "finger pattern" changes when the merged X coordinate passes over the
> original starting point of the bottom finger.  However, this
> approximation only holds when the bottom finger is perfectly
> stationary.  In the real world, the bottom finger rolls/wiggles or
> otherwise moves, causing the actual crossing point to change.  Or, as
> is also likely, the lower finger is already being pulled when the
> upper finger starts moving, so its reported position is already not at
> the correct crossing point.  Thus, using semi-mt with this profile
> sensor clickpad, we have not been able to generate smooth pointer
> motion when an upper finger crosses a lower finger.
>
> What we found, though, is that the firmware does do a much better job
> of tracking such horizontal crosses.  Therefore, we would like to get
> this raw data from the kernel.  It is trivial for userspace to convert
> the raw finger position data to a bounding box format for use with
> other gestures.
>
> In any case, we really aren't that familiar with how other "synaptics
> semi-mt-compatible" touchpads work.  Using this patch may or may not
> be better than semi-mt.  Or, their firmware may or may not follow the
> same "always report upper finger in sgm" rule (we have seen other
> Synaptics trackpads that do "always report oldest finger in sgm").
> This is why we chose to isolate this change to just one hardware type,
> so that the change, which improves the experience of a Cr-48 user, has
> no deleterious affect on users of any other hardware.
>
> If others try this method with their hardware and it works for them,
> then great!   We would be happy to help review additional patches that
> extend it to additional systems.

Ok, that's very helpful to know, thanks for providing such a detailed 
response!

So the problem is that you want to support the kernel providing two 
touches, but the touch locations are only valid if they do not cross in 
the Y axis. That's a very nuanced property of the device, one that is 
only specific to certain touchpads from one manufacturer.

I understand the usefulness of this functionality, but I also worry 
about proliferating the number of properties for devices (there are only 
32 bits we can use, IIRC). I see four options off the top of my head:

* Don't do anything, leave it as SEMI_MT. Obviously this would suck, but 
it is an option.

* Make the trackpad advertise itself as *not* SEMI_MT. This would be 
broken, however, if the user performs a rotation where the touches cross 
in the Y axis. I feel this is plain wrong according to the stated 
protocol documentation and previous behavior, so I don't want to do this.

* Add a new device property (INVALID_Y_AXIS_CROSSING?) that describes 
the exact behavior of this device. I would be ok with this if everyone 
else is, but only because proper clickpad behavior (which I consider 
very importand) is broken without this knowledge.

* Leave the device as SEMI_MT, but provide the real locations, and allow 
userspace to determine the device vendor/model/etc. If userspace knows 
that a specific device behaves in a specific way, it can do its own 
quirking handling. Given the specificity of this behavior to only some 
devices of one brand, this would be my suggested resolution to the issue.

-- Chase
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] Input: synaptics - use firmware data for Cr-48
  2012-07-19 17:34           ` Chase Douglas
  (?)
@ 2012-07-19 18:44           ` Henrik Rydberg
       [not found]             ` <CAM2ehZaLeJsxCOkqLv9jSko9y3Awix1jjobfTo5WQj8rcrYquA@mail.gmail.com>
  -1 siblings, 1 reply; 19+ messages in thread
From: Henrik Rydberg @ 2012-07-19 18:44 UTC (permalink / raw)
  To: Chase Douglas
  Cc: Daniel Kurtz, "Chung-Yih Wang (王崇懿)",
	Dmitry Torokhov, JJ Ding, linux-input, linux-kernel

> I understand the usefulness of this functionality, but I also worry
> about proliferating the number of properties for devices (there are
> only 32 bits we can use, IIRC). I see four options off the top of my
> head:
> 
> * Don't do anything, leave it as SEMI_MT. Obviously this would suck,
> but it is an option.
> 
> * Make the trackpad advertise itself as *not* SEMI_MT. This would be
> broken, however, if the user performs a rotation where the touches
> cross in the Y axis. I feel this is plain wrong according to the
> stated protocol documentation and previous behavior, so I don't want
> to do this.
> 
> * Add a new device property (INVALID_Y_AXIS_CROSSING?) that
> describes the exact behavior of this device. I would be ok with this
> if everyone else is, but only because proper clickpad behavior
> (which I consider very importand) is broken without this knowledge.
> 
> * Leave the device as SEMI_MT, but provide the real locations, and
> allow userspace to determine the device vendor/model/etc. If
> userspace knows that a specific device behaves in a specific way, it
> can do its own quirking handling. Given the specificity of this
> behavior to only some devices of one brand, this would be my
> suggested resolution to the issue.

A fifth option is to quirk the driver to remove the pulling effect
from the reported bounding box, such that the simple userspace scheme
for determining the position of the moving finger actually works.

For instance, consider the simple algorithm "the slowest corner is the
stationary finger". As long as this is true, the behavior will be
smooth. If the sensor data clutters this behavior, it shows up in the
driver as a mismatch between the point as computed above and the
better estimate available in the driver. For frames where this
happens, one can simply alter the bounding box slightly so that the
algorithm works.

It should be possible to formulate such a scheme in a couple of lines.

Henrik

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] Input: synaptics - use firmware data for Cr-48
       [not found]             ` <CAM2ehZaLeJsxCOkqLv9jSko9y3Awix1jjobfTo5WQj8rcrYquA@mail.gmail.com>
@ 2012-07-20  7:25                 ` Henrik Rydberg
  0 siblings, 0 replies; 19+ messages in thread
From: Henrik Rydberg @ 2012-07-20  7:25 UTC (permalink / raw)
  To: Chung-Yih Wang (王崇懿)
  Cc: Chase Douglas, Daniel Kurtz, Dmitry Torokhov, JJ Ding,
	linux-input, linux-kernel

On Fri, Jul 20, 2012 at 11:14:25AM +0800, Chung-Yih Wang (王崇懿) wrote:
> From our experiments, the assumption of "the slowest corner is the
> stationary finger" is not always true. That is the major reason we want to
> report the firmware data instead of semi-mt.

Oh, but that was precisely the point; the reason it does not hold true
is due to sensor and discretization errors. If we can improve upon
this situation, we get a better model of reality.

> The problem here will be how
> to remove the pulling effect, we measured the pulling effect before and can
> not get a good result as there could be IIR in firmware as well. It seems
> not an easy job to remove the pulling effect cleanly.

Probably a simple filter will work. If the bounding box is moving too
fast for the tracked point to stay in the right corner, the solution
is to use a smaller time step. In practise, keeping the tracked point
as a state in the driver, and updating the bounding box using box ->
(1 - m) box + m box_new. If the tracked point is in the right corner,
let m = 1. If not, choose a smaller m.

> > > * Add a new device property (INVALID_Y_AXIS_CROSSING?) that
> > > describes the exact behavior of this device. I would be ok with this
> > > if everyone else is, but only because proper clickpad behavior
> > > (which I consider very importand) is broken without this knowledge.
> >
> Sounds good to me(but I would rather to have INVALID_CROSSING instead,
> depending on the relative finger positions,  it could still have wrong
> tracking either in X or Y axis crossing)

Propagating information about various sensor defects to userspace
sounds horrid to me. The sooner we can forget about these devices, the
better.

> > > * Leave the device as SEMI_MT, but provide the real locations, and
> > > allow userspace to determine the device vendor/model/etc. If
> > > userspace knows that a specific device behaves in a specific way, it
> > > can do its own quirking handling. Given the specificity of this
> > > behavior to only some devices of one brand, this would be my
> > > suggested resolution to the issue.
> >
> 
> A bit confused here, do you mean we report the real locations instead of
> bounding box the current driver have? I am not quite sure if this will
> affect other existing works in userspace for this semi-mt driver.

I am not entirely opposed to this solution, but I would much rather
see an attempt to improve the bounding box in the driver, since such a
solution could be useful for other devices as well.

Thanks,
Henrik

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] Input: synaptics - use firmware data for Cr-48
@ 2012-07-20  7:25                 ` Henrik Rydberg
  0 siblings, 0 replies; 19+ messages in thread
From: Henrik Rydberg @ 2012-07-20  7:25 UTC (permalink / raw)
  To: Chung-Yih Wang (王崇懿)
  Cc: Chase Douglas, Daniel Kurtz, Dmitry Torokhov, JJ Ding,
	linux-input, linux-kernel

On Fri, Jul 20, 2012 at 11:14:25AM +0800, Chung-Yih Wang (王崇懿) wrote:
> From our experiments, the assumption of "the slowest corner is the
> stationary finger" is not always true. That is the major reason we want to
> report the firmware data instead of semi-mt.

Oh, but that was precisely the point; the reason it does not hold true
is due to sensor and discretization errors. If we can improve upon
this situation, we get a better model of reality.

> The problem here will be how
> to remove the pulling effect, we measured the pulling effect before and can
> not get a good result as there could be IIR in firmware as well. It seems
> not an easy job to remove the pulling effect cleanly.

Probably a simple filter will work. If the bounding box is moving too
fast for the tracked point to stay in the right corner, the solution
is to use a smaller time step. In practise, keeping the tracked point
as a state in the driver, and updating the bounding box using box ->
(1 - m) box + m box_new. If the tracked point is in the right corner,
let m = 1. If not, choose a smaller m.

> > > * Add a new device property (INVALID_Y_AXIS_CROSSING?) that
> > > describes the exact behavior of this device. I would be ok with this
> > > if everyone else is, but only because proper clickpad behavior
> > > (which I consider very importand) is broken without this knowledge.
> >
> Sounds good to me(but I would rather to have INVALID_CROSSING instead,
> depending on the relative finger positions,  it could still have wrong
> tracking either in X or Y axis crossing)

Propagating information about various sensor defects to userspace
sounds horrid to me. The sooner we can forget about these devices, the
better.

> > > * Leave the device as SEMI_MT, but provide the real locations, and
> > > allow userspace to determine the device vendor/model/etc. If
> > > userspace knows that a specific device behaves in a specific way, it
> > > can do its own quirking handling. Given the specificity of this
> > > behavior to only some devices of one brand, this would be my
> > > suggested resolution to the issue.
> >
> 
> A bit confused here, do you mean we report the real locations instead of
> bounding box the current driver have? I am not quite sure if this will
> affect other existing works in userspace for this semi-mt driver.

I am not entirely opposed to this solution, but I would much rather
see an attempt to improve the bounding box in the driver, since such a
solution could be useful for other devices as well.

Thanks,
Henrik
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] Input: synaptics - use firmware data for Cr-48
  2012-07-20  7:25                 ` Henrik Rydberg
@ 2012-07-20  9:03                   ` Daniel Kurtz
  -1 siblings, 0 replies; 19+ messages in thread
From: Daniel Kurtz @ 2012-07-20  9:03 UTC (permalink / raw)
  To: Henrik Rydberg
  Cc: Chung-Yih Wang (王崇懿),
	Chase Douglas, Dmitry Torokhov, JJ Ding, linux-input,
	linux-kernel

On Fri, Jul 20, 2012 at 3:25 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> On Fri, Jul 20, 2012 at 11:14:25AM +0800, Chung-Yih Wang (王崇懿) wrote:
>> From our experiments, the assumption of "the slowest corner is the
>> stationary finger" is not always true. That is the major reason we want to
>> report the firmware data instead of semi-mt.
>
> Oh, but that was precisely the point; the reason it does not hold true
> is due to sensor and discretization errors. If we can improve upon
> this situation, we get a better model of reality.
>
>> The problem here will be how
>> to remove the pulling effect, we measured the pulling effect before and can
>> not get a good result as there could be IIR in firmware as well. It seems
>> not an easy job to remove the pulling effect cleanly.
>
> Probably a simple filter will work. If the bounding box is moving too
> fast for the tracked point to stay in the right corner, the solution
> is to use a smaller time step. In practise, keeping the tracked point
> as a state in the driver, and updating the bounding box using box ->
> (1 - m) box + m box_new. If the tracked point is in the right corner,
> let m = 1. If not, choose a smaller m.
>
>> > > * Add a new device property (INVALID_Y_AXIS_CROSSING?) that
>> > > describes the exact behavior of this device. I would be ok with this
>> > > if everyone else is, but only because proper clickpad behavior
>> > > (which I consider very importand) is broken without this knowledge.
>> >
>> Sounds good to me(but I would rather to have INVALID_CROSSING instead,
>> depending on the relative finger positions,  it could still have wrong
>> tracking either in X or Y axis crossing)
>
> Propagating information about various sensor defects to userspace
> sounds horrid to me. The sooner we can forget about these devices, the
> better.

Not providing the userspace driver with enough information to give
users the best experience possible sounds horrid to me.  It turns out
that using a bounding box with fixed "[(min_x, min_y), (max_x,
max_y)]", and no per-finger pressure information, instead of the
coordinates and pressures provided by the firmware, is throwing away
useful data that could be used by the userspace driver.

What we would like is a way to tell userspace what the firmware
originally intended, but with a caveat that the firmware can't be 100%
trusted.  And, since this is for a relatively small class of hardware,
to do it in a way that doesn't consume precious resources, like
additional input properties.

>
>> > > * Leave the device as SEMI_MT, but provide the real locations, and
>> > > allow userspace to determine the device vendor/model/etc. If
>> > > userspace knows that a specific device behaves in a specific way, it
>> > > can do its own quirking handling. Given the specificity of this
>> > > behavior to only some devices of one brand, this would be my
>> > > suggested resolution to the issue.
>> >

This is essentially what this patch does.  It sets the SEMI_MT flag to
indicate that the kernel data cannot be totally trusted, and then
provides real MT-B (including per-finger pressures), instead of a
fixed bounding box.  It leaves it to userspace to treat the two slots
worth of coordinates as a bounding box or as actual fingers using its
own heuristics.  By limiting to only one hardware type (using DMI),
any breakage caused by this alternative use of the SEMI_MT flag is
limited.

Hopefully it is clear what we are trying to accomplish.  I don't see
how we can make a bounding box, even an improved bounding box, work.
Perhaps Henrik has a really good idea, but I haven't been able to
figure out what he is suggesting.

Thank you for the nice discussion and alternative suggestions about
better ways of doing this!

-Daniel

>>
>> A bit confused here, do you mean we report the real locations instead of
>> bounding box the current driver have? I am not quite sure if this will
>> affect other existing works in userspace for this semi-mt driver.
>
> I am not entirely opposed to this solution, but I would much rather
> see an attempt to improve the bounding box in the driver, since such a
> solution could be useful for other devices as well.
>
> Thanks,
> Henrik

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] Input: synaptics - use firmware data for Cr-48
@ 2012-07-20  9:03                   ` Daniel Kurtz
  0 siblings, 0 replies; 19+ messages in thread
From: Daniel Kurtz @ 2012-07-20  9:03 UTC (permalink / raw)
  To: Henrik Rydberg
  Cc: Chung-Yih Wang (王崇懿),
	Chase Douglas, Dmitry Torokhov, JJ Ding, linux-input,
	linux-kernel

On Fri, Jul 20, 2012 at 3:25 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> On Fri, Jul 20, 2012 at 11:14:25AM +0800, Chung-Yih Wang (王崇懿) wrote:
>> From our experiments, the assumption of "the slowest corner is the
>> stationary finger" is not always true. That is the major reason we want to
>> report the firmware data instead of semi-mt.
>
> Oh, but that was precisely the point; the reason it does not hold true
> is due to sensor and discretization errors. If we can improve upon
> this situation, we get a better model of reality.
>
>> The problem here will be how
>> to remove the pulling effect, we measured the pulling effect before and can
>> not get a good result as there could be IIR in firmware as well. It seems
>> not an easy job to remove the pulling effect cleanly.
>
> Probably a simple filter will work. If the bounding box is moving too
> fast for the tracked point to stay in the right corner, the solution
> is to use a smaller time step. In practise, keeping the tracked point
> as a state in the driver, and updating the bounding box using box ->
> (1 - m) box + m box_new. If the tracked point is in the right corner,
> let m = 1. If not, choose a smaller m.
>
>> > > * Add a new device property (INVALID_Y_AXIS_CROSSING?) that
>> > > describes the exact behavior of this device. I would be ok with this
>> > > if everyone else is, but only because proper clickpad behavior
>> > > (which I consider very importand) is broken without this knowledge.
>> >
>> Sounds good to me(but I would rather to have INVALID_CROSSING instead,
>> depending on the relative finger positions,  it could still have wrong
>> tracking either in X or Y axis crossing)
>
> Propagating information about various sensor defects to userspace
> sounds horrid to me. The sooner we can forget about these devices, the
> better.

Not providing the userspace driver with enough information to give
users the best experience possible sounds horrid to me.  It turns out
that using a bounding box with fixed "[(min_x, min_y), (max_x,
max_y)]", and no per-finger pressure information, instead of the
coordinates and pressures provided by the firmware, is throwing away
useful data that could be used by the userspace driver.

What we would like is a way to tell userspace what the firmware
originally intended, but with a caveat that the firmware can't be 100%
trusted.  And, since this is for a relatively small class of hardware,
to do it in a way that doesn't consume precious resources, like
additional input properties.

>
>> > > * Leave the device as SEMI_MT, but provide the real locations, and
>> > > allow userspace to determine the device vendor/model/etc. If
>> > > userspace knows that a specific device behaves in a specific way, it
>> > > can do its own quirking handling. Given the specificity of this
>> > > behavior to only some devices of one brand, this would be my
>> > > suggested resolution to the issue.
>> >

This is essentially what this patch does.  It sets the SEMI_MT flag to
indicate that the kernel data cannot be totally trusted, and then
provides real MT-B (including per-finger pressures), instead of a
fixed bounding box.  It leaves it to userspace to treat the two slots
worth of coordinates as a bounding box or as actual fingers using its
own heuristics.  By limiting to only one hardware type (using DMI),
any breakage caused by this alternative use of the SEMI_MT flag is
limited.

Hopefully it is clear what we are trying to accomplish.  I don't see
how we can make a bounding box, even an improved bounding box, work.
Perhaps Henrik has a really good idea, but I haven't been able to
figure out what he is suggesting.

Thank you for the nice discussion and alternative suggestions about
better ways of doing this!

-Daniel

>>
>> A bit confused here, do you mean we report the real locations instead of
>> bounding box the current driver have? I am not quite sure if this will
>> affect other existing works in userspace for this semi-mt driver.
>
> I am not entirely opposed to this solution, but I would much rather
> see an attempt to improve the bounding box in the driver, since such a
> solution could be useful for other devices as well.
>
> Thanks,
> Henrik
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] Input: synaptics - use firmware data for Cr-48
  2012-07-20  9:03                   ` Daniel Kurtz
  (?)
@ 2012-07-20 13:03                   ` Henrik Rydberg
  -1 siblings, 0 replies; 19+ messages in thread
From: Henrik Rydberg @ 2012-07-20 13:03 UTC (permalink / raw)
  To: Daniel Kurtz
  Cc: Chung-Yih Wang (王崇懿),
	Chase Douglas, Dmitry Torokhov, JJ Ding, linux-input,
	linux-kernel

> > Propagating information about various sensor defects to userspace
> > sounds horrid to me. The sooner we can forget about these devices, the
> > better.
> 
> Not providing the userspace driver with enough information to give
> users the best experience possible sounds horrid to me.

The question was whether we should add those to the long-lived input
interface or not, sorry if that sounded like a rant.

> It turns out
> that using a bounding box with fixed "[(min_x, min_y), (max_x,
> max_y)]", and no per-finger pressure information, instead of the
> coordinates and pressures provided by the firmware, is throwing away
> useful data that could be used by the userspace driver.

So far, we have provided the baseline of reliable data from similar
devices.

> What we would like is a way to tell userspace what the firmware
> originally intended, but with a caveat that the firmware can't be 100%
> trusted.  And, since this is for a relatively small class of hardware,
> to do it in a way that doesn't consume precious resources, like
> additional input properties.

Input properties are not a precious resource, there is no limit on the
bitmask values or anything like that, but there is no rush to add new
ones.

> >> > > * Leave the device as SEMI_MT, but provide the real locations, and
> >> > > allow userspace to determine the device vendor/model/etc. If
> >> > > userspace knows that a specific device behaves in a specific way, it
> >> > > can do its own quirking handling. Given the specificity of this
> >> > > behavior to only some devices of one brand, this would be my
> >> > > suggested resolution to the issue.
> >> >
> 
> This is essentially what this patch does.

I am interpreting Chase's suggestion as simply reporting the raw
values instead of min/max.

> It sets the SEMI_MT flag to
> indicate that the kernel data cannot be totally trusted, and then
> provides real MT-B (including per-finger pressures), instead of a
> fixed bounding box.  It leaves it to userspace to treat the two slots
> worth of coordinates as a bounding box or as actual fingers using its
> own heuristics.  By limiting to only one hardware type (using DMI),
> any breakage caused by this alternative use of the SEMI_MT flag is
> limited.

So it seems there is no need to add logic to the driver, only change
one line from min/max to raw data for this particular hardware. That
would solve your problem, yes?

> Hopefully it is clear what we are trying to accomplish.  I don't see
> how we can make a bounding box, even an improved bounding box, work.
> Perhaps Henrik has a really good idea, but I haven't been able to
> figure out what he is suggesting.

I understand you are not interested in looking into this, and your
main objective is quite clear.

Thanks,
Henrik

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] Input: synaptics - use firmware data for Cr-48
  2012-07-20  9:03                   ` Daniel Kurtz
  (?)
  (?)
@ 2012-07-20 18:31                   ` Chase Douglas
  2012-07-27 10:40                     ` Daniel Kurtz
  -1 siblings, 1 reply; 19+ messages in thread
From: Chase Douglas @ 2012-07-20 18:31 UTC (permalink / raw)
  To: Daniel Kurtz
  Cc: Henrik Rydberg,
	"Chung-Yih Wang (王崇懿)",
	Dmitry Torokhov, JJ Ding, linux-input, linux-kernel

On 07/20/2012 02:03 AM, Daniel Kurtz wrote:
>>>>> * Leave the device as SEMI_MT, but provide the real locations, and
>>>>> allow userspace to determine the device vendor/model/etc. If
>>>>> userspace knows that a specific device behaves in a specific way, it
>>>>> can do its own quirking handling. Given the specificity of this
>>>>> behavior to only some devices of one brand, this would be my
>>>>> suggested resolution to the issue.
>
> This is essentially what this patch does.  It sets the SEMI_MT flag to
> indicate that the kernel data cannot be totally trusted, and then
> provides real MT-B (including per-finger pressures), instead of a
> fixed bounding box.  It leaves it to userspace to treat the two slots
> worth of coordinates as a bounding box or as actual fingers using its
> own heuristics.  By limiting to only one hardware type (using DMI),
> any breakage caused by this alternative use of the SEMI_MT flag is
> limited.

So I was worried that you were trying to remove the SEMI_MT flag, and I 
apologise for not looking closely enough to notice that wasn't the case. 
The documentation for the flag says:

"""
Some touchpads, most common between 2008 and 2011, can detect the 
presence of multiple contacts without resolving the individual 
positions; only the number of contacts and a rectangular shape is known. 
For such touchpads, the semi-mt property should be set.

Depending on the device, the rectangle may enclose all touches, like a 
bounding box, or just some of them, for instance the two most recent 
touches. The diversity makes the rectangle of limited use, but some 
gestures can normally be extracted from it.
"""

Since the documentation doesn't say the data must be provided as min/max 
values, this patch actually appears to be perfectly fine as is.

My next question is: how are you going to tell from userspace if the 
hardware actually provides correct data? IIRC, it was decided that we 
wouldn't provide sysfs nodes for the device IDs.

-- Chase

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] Input: synaptics - use firmware data for Cr-48
  2012-07-20 18:31                   ` Chase Douglas
@ 2012-07-27 10:40                     ` Daniel Kurtz
  0 siblings, 0 replies; 19+ messages in thread
From: Daniel Kurtz @ 2012-07-27 10:40 UTC (permalink / raw)
  To: Chase Douglas
  Cc: Henrik Rydberg,
	"Chung-Yih Wang (王崇懿)",
	Dmitry Torokhov, JJ Ding, linux-input, linux-kernel

On Sat, Jul 21, 2012 at 2:31 AM, Chase Douglas
<chase.douglas@canonical.com> wrote:
>
> On 07/20/2012 02:03 AM, Daniel Kurtz wrote:
>>>>>>
>>>>>> * Leave the device as SEMI_MT, but provide the real locations, and
>>>>>> allow userspace to determine the device vendor/model/etc. If
>>>>>> userspace knows that a specific device behaves in a specific way, it
>>>>>> can do its own quirking handling. Given the specificity of this
>>>>>> behavior to only some devices of one brand, this would be my
>>>>>> suggested resolution to the issue.
>>
>>
>> This is essentially what this patch does.  It sets the SEMI_MT flag to
>> indicate that the kernel data cannot be totally trusted, and then
>> provides real MT-B (including per-finger pressures), instead of a
>> fixed bounding box.  It leaves it to userspace to treat the two slots
>> worth of coordinates as a bounding box or as actual fingers using its
>> own heuristics.  By limiting to only one hardware type (using DMI),
>> any breakage caused by this alternative use of the SEMI_MT flag is
>> limited.
>
>
> So I was worried that you were trying to remove the SEMI_MT flag, and I apologise for not looking closely enough to notice that wasn't the case. The documentation for the flag says:
>
> """
> Some touchpads, most common between 2008 and 2011, can detect the presence of multiple contacts without resolving the individual positions; only the number of contacts and a rectangular shape is known. For such touchpads, the semi-mt property should be set.
>
> Depending on the device, the rectangle may enclose all touches, like a bounding box, or just some of them, for instance the two most recent touches. The diversity makes the rectangle of limited use, but some gestures can normally be extracted from it.
> """
>
> Since the documentation doesn't say the data must be provided as min/max values, this patch actually appears to be perfectly fine as is.
>
> My next question is: how are you going to tell from userspace if the hardware actually provides correct data? IIRC, it was decided that we wouldn't provide sysfs nodes for the device IDs.
>

Excellent question.  We haven't solved this in any elegant way.  When
building images for this particular hardware platform, we set a flag
in our user-space touchpad driver.  It then knows to process this
device's data as "non-bounding box semi-mt".

-Daniel

> -- Chase
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2012-07-27 10:40 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-07-18 10:22 [PATCH v2] Input: synaptics - use firmware data for Cr-48 Chung-yih Wang
2012-07-18 15:38 ` Chase Douglas
     [not found]   ` <CAM2ehZbftDja6CBGjhL3Jp+30DtYJj+8_4e=_wWcj3pCDGD7AA@mail.gmail.com>
2012-07-19  6:42     ` Dmitry Torokhov
2012-07-19  6:42       ` Dmitry Torokhov
2012-07-19 13:14     ` Henrik Rydberg
2012-07-19 16:16     ` Chase Douglas
2012-07-19 16:16       ` Chase Douglas
2012-07-19 17:05       ` Daniel Kurtz
2012-07-19 17:05         ` Daniel Kurtz
2012-07-19 17:34         ` Chase Douglas
2012-07-19 17:34           ` Chase Douglas
2012-07-19 18:44           ` Henrik Rydberg
     [not found]             ` <CAM2ehZaLeJsxCOkqLv9jSko9y3Awix1jjobfTo5WQj8rcrYquA@mail.gmail.com>
2012-07-20  7:25               ` Henrik Rydberg
2012-07-20  7:25                 ` Henrik Rydberg
2012-07-20  9:03                 ` Daniel Kurtz
2012-07-20  9:03                   ` Daniel Kurtz
2012-07-20 13:03                   ` Henrik Rydberg
2012-07-20 18:31                   ` Chase Douglas
2012-07-27 10:40                     ` Daniel Kurtz

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.