| Message ID | 1281803367-2590-1-git-send-email-rydberg@euromail.se |
|---|---|
| State | Accepted |
| Delegated to: | Leann Ogasawara |
| Headers | show |
On Sat, 2010-08-14 at 17:29 +0100, Henrik Rydberg wrote: > The ten-finger 3M screen M2256PW has a kernel driver already > (hid-3m-pct), but with a couple of glitches: > > 1. The one-touch down/up, which emits BTN_TOUCH events, sometimes does > not work properly. This results in buttons getting stuck in the > button-down state in the window manager. > > 2. The action after touch-up in one spot followed by a touch-down in > another spot sometimes causes a sequence of strange finger-tracing > events along the perimeter of the rectangle between the two spots. The > root of the problem lies in how the kernel inteprets the HID hybrid > event type which is used by the 3M device [1]. > > While a proper HID solution is being worked out upstream, this patch > simplifies the driver logic such that BTN_TOUCH and ABS_X/Y events are > emitted properly, in effect papering over the worst problems. Suggested > for inclusion in Maverick. > > [1] Suggested by Stephane Chatty, who also confirms the observed problems. > > Signed-off-by: Henrik Rydberg <rydberg@euromail.se> > --- > drivers/hid/hid-3m-pct.c | 47 +++++++++------------------------------------ > 1 files changed, 10 insertions(+), 37 deletions(-) > > diff --git a/drivers/hid/hid-3m-pct.c b/drivers/hid/hid-3m-pct.c > index 2a0d56b..fc57c0c 100644 > --- a/drivers/hid/hid-3m-pct.c > +++ b/drivers/hid/hid-3m-pct.c > @@ -125,9 +125,7 @@ static int mmm_input_mapped(struct hid_device *hdev, struct hid_input *hi, > */ > static void mmm_filter_event(struct mmm_data *md, struct input_dev *input) > { > - struct mmm_finger *oldest = 0; > - bool pressed = false, released = false; > - int i; > + int i, index = -1; > > /* > * we need to iterate on all fingers to decide if we have a press > @@ -149,45 +147,20 @@ static void mmm_filter_event(struct mmm_data *md, struct input_dev *input) > input_event(input, EV_ABS, ABS_MT_TOUCH_MINOR, > wide ? f->h : f->w); > input_mt_sync(input); > - /* > - * touchscreen emulation: maintain the age rank > - * of this finger, decide if we have a press > - */ > - if (f->rank == 0) { > - f->rank = ++(md->num); > - if (f->rank == 1) > - pressed = true; > - } > - if (f->rank == 1) > - oldest = f; > - } else { > - /* this finger took off the screen */ > - /* touchscreen emulation: maintain age rank of others */ > - int j; > - > - for (j = 0; j < 10; ++j) { > - struct mmm_finger *g = &md->f[j]; > - if (g->rank > f->rank) { > - g->rank--; > - if (g->rank == 1) > - oldest = g; > - } > - } > - f->rank = 0; > - --(md->num); > - if (md->num == 0) > - released = true; > + > + if (index < 0) > + index = i; > } > f->valid = 0; > } > > /* touchscreen emulation */ > - if (oldest) { > - if (pressed) > - input_event(input, EV_KEY, BTN_TOUCH, 1); > - input_event(input, EV_ABS, ABS_X, oldest->x); > - input_event(input, EV_ABS, ABS_Y, oldest->y); > - } else if (released) { > + if (index >= 0) { > + struct mmm_finger *f = &md->f[index]; > + input_event(input, EV_KEY, BTN_TOUCH, 1); > + input_event(input, EV_ABS, ABS_X, f->x); > + input_event(input, EV_ABS, ABS_Y, f->y); > + } else { > input_event(input, EV_KEY, BTN_TOUCH, 0); > } > } > -- > 1.7.1 Won't this emit a button touch down and ABS_{X,Y} event every time any finger moves? I can think of many ways this could break things, including the ABS_{X,Y} values flittering back and forth between all fingers touching the screen. This would make dragging and dropping a nightmare if you accidentally or intentionally touch more than one finger to the display at a time. Is the one-touch button down/up issue caused by driver logic or hardware issues? I wonder if a better solution would be single-touch emulation by only sending BTN_TOUCH and ABS_{X,Y} events on the first touch started and not sending these events on the second touch started when the first is released. -- Chase
On 08/15/2010 07:42 PM, Chase Douglas wrote: > On Sat, 2010-08-14 at 17:29 +0100, Henrik Rydberg wrote: >> The ten-finger 3M screen M2256PW has a kernel driver already >> (hid-3m-pct), but with a couple of glitches: >> >> 1. The one-touch down/up, which emits BTN_TOUCH events, sometimes does >> not work properly. This results in buttons getting stuck in the >> button-down state in the window manager. >> >> 2. The action after touch-up in one spot followed by a touch-down in >> another spot sometimes causes a sequence of strange finger-tracing >> events along the perimeter of the rectangle between the two spots. The >> root of the problem lies in how the kernel inteprets the HID hybrid >> event type which is used by the 3M device [1]. >> >> While a proper HID solution is being worked out upstream, this patch >> simplifies the driver logic such that BTN_TOUCH and ABS_X/Y events are >> emitted properly, in effect papering over the worst problems. Suggested >> for inclusion in Maverick. >> >> [1] Suggested by Stephane Chatty, who also confirms the observed problems. >> >> Signed-off-by: Henrik Rydberg <rydberg@euromail.se> [...] > > Won't this emit a button touch down and ABS_{X,Y} event every time any > finger moves? No. The input subsystem only emits changes to states. Moreover, only the position of the first used slot is reported. Other used slots have no effect at all. > I can think of many ways this could break things, > including the ABS_{X,Y} values flittering back and forth between all > fingers touching the screen. This would make dragging and dropping a > nightmare if you accidentally or intentionally touch more than one > finger to the display at a time. This has no effect when running the MT stack in userspace, since multi-finger touches produce no output. Besides, the problem can only present itself if the device switches contact identity on the slots. > Is the one-touch button down/up issue caused by driver logic or hardware > issues? As stated in the pretext, the kernel hid implementation is doing the wrong thing when receiving a hybrid MT event from the device, leading to spurious SYN_REPORT events. Whether this also breaks the current state logic is yet to be investigated. The simplified state logic presented in this patch is resilient to the spurious SYN_REPORT events, and therefore works properly. > I wonder if a better solution would be single-touch emulation by only > sending BTN_TOUCH and ABS_{X,Y} events on the first touch started and > not sending these events on the second touch started when the first is > released. As stated in the pretext, the patch is a temporary solution until a proper solution involving the hid core has been found. The logic you describe is basically what Stephane Chatty implemented, and which this patch temporarily removes. Henrik
On Sun, 2010-08-15 at 22:32 +0200, Henrik Rydberg wrote: > On 08/15/2010 07:42 PM, Chase Douglas wrote: > > > On Sat, 2010-08-14 at 17:29 +0100, Henrik Rydberg wrote: > >> The ten-finger 3M screen M2256PW has a kernel driver already > >> (hid-3m-pct), but with a couple of glitches: > >> > >> 1. The one-touch down/up, which emits BTN_TOUCH events, sometimes does > >> not work properly. This results in buttons getting stuck in the > >> button-down state in the window manager. > >> > >> 2. The action after touch-up in one spot followed by a touch-down in > >> another spot sometimes causes a sequence of strange finger-tracing > >> events along the perimeter of the rectangle between the two spots. The > >> root of the problem lies in how the kernel inteprets the HID hybrid > >> event type which is used by the 3M device [1]. > >> > >> While a proper HID solution is being worked out upstream, this patch > >> simplifies the driver logic such that BTN_TOUCH and ABS_X/Y events are > >> emitted properly, in effect papering over the worst problems. Suggested > >> for inclusion in Maverick. > >> > >> [1] Suggested by Stephane Chatty, who also confirms the observed problems. > >> > >> Signed-off-by: Henrik Rydberg <rydberg@euromail.se> > > [...] > > > > > Won't this emit a button touch down and ABS_{X,Y} event every time any > > finger moves? > > > No. The input subsystem only emits changes to states. Moreover, only the > position of the first used slot is reported. Other used slots have no effect at all. Yep, I misread this the first time. I still think I see an issue though. Say you touch one finger to the screen, so a BTN_TOUCH down and ABS_{X,Y} is emitted for the finger. You intend to use this touch to drag something on the screen. Now, BTN_TOUCH down is emitted on every frame of data coming from the device. This seems weird, but we'll leave that aside for now. Next, you touch a second finger to the screen. I'll assume it's index in the array of touches is higher, so the first finger is still driving the ABS_{X,Y}. Now, you lift up the first finger. BTN_TOUCH never goes up because single-touch emulation shifts immediately to the second finger. All of a sudden your dragged item is dragged across the screen to somewhere else. That seems like bad behavior to me. > > I wonder if a better solution would be single-touch emulation by only > > sending BTN_TOUCH and ABS_{X,Y} events on the first touch started and > > not sending these events on the second touch started when the first is > > released. > > > As stated in the pretext, the patch is a temporary solution until a proper > solution involving the hid core has been found. The logic you describe is > basically what Stephane Chatty implemented, and which this patch temporarily > removes. The original code switches the BTN_TOUCH and ABS_{X,Y} to the oldest touch when the current oldest touch is removed. After your patch, this jumping around will still occur. That seems wrong to me, but this is a murky area. There isn't a generally accepted mode of handling single-touch legacy support in multitouch drivers (not that I'm aware of at least, you may know more than I :). Thus, I'm wondering how well it would work if we only sent these single-touch events for the first touch, and no events for any further touches until they are all removed. This would allow single-touch drag when multiple fingers are down to behave right, in my opinion, such that the end of the finger doing the dragging causes dragging to cease. Of course, this is all an issue of MT -> ST translation, and hopefully all apps will become MT aware and not depend on ABS_{X,Y} when ABS_MT_* are available :). -- Chase
On 08/15/2010 11:28 PM, Chase Douglas wrote: > On Sun, 2010-08-15 at 22:32 +0200, Henrik Rydberg wrote: >> On 08/15/2010 07:42 PM, Chase Douglas wrote: >> >>> On Sat, 2010-08-14 at 17:29 +0100, Henrik Rydberg wrote: >>>> The ten-finger 3M screen M2256PW has a kernel driver already >>>> (hid-3m-pct), but with a couple of glitches: >>>> >>>> 1. The one-touch down/up, which emits BTN_TOUCH events, sometimes does >>>> not work properly. This results in buttons getting stuck in the >>>> button-down state in the window manager. >>>> >>>> 2. The action after touch-up in one spot followed by a touch-down in >>>> another spot sometimes causes a sequence of strange finger-tracing >>>> events along the perimeter of the rectangle between the two spots. The >>>> root of the problem lies in how the kernel inteprets the HID hybrid >>>> event type which is used by the 3M device [1]. >>>> >>>> While a proper HID solution is being worked out upstream, this patch >>>> simplifies the driver logic such that BTN_TOUCH and ABS_X/Y events are >>>> emitted properly, in effect papering over the worst problems. Suggested >>>> for inclusion in Maverick. >>>> >>>> [1] Suggested by Stephane Chatty, who also confirms the observed problems. >>>> >>>> Signed-off-by: Henrik Rydberg <rydberg@euromail.se> >> >> [...] >>> >> >>> Won't this emit a button touch down and ABS_{X,Y} event every time any >>> finger moves? >> >> >> No. The input subsystem only emits changes to states. Moreover, only the >> position of the first used slot is reported. Other used slots have no effect at all. > > Yep, I misread this the first time. > > I still think I see an issue though. Say you touch one finger to the > screen, so a BTN_TOUCH down and ABS_{X,Y} is emitted for the finger. You > intend to use this touch to drag something on the screen. Now, BTN_TOUCH > down is emitted on every frame of data coming from the device. This > seems weird, but we'll leave that aside for now. No. Since the input core only emits changes, there is one BTN_TOUCH event when the first finger touches the surface, and one BTN_TOUCH event when the last finger leaves the surface. > Next, you touch a > second finger to the screen. I'll assume it's index in the array of > touches is higher, so the first finger is still driving the ABS_{X,Y}. > > Now, you lift up the first finger. BTN_TOUCH never goes up because > single-touch emulation shifts immediately to the second finger. All of a > sudden your dragged item is dragged across the screen to somewhere else. > That seems like bad behavior to me. For a touchpad, the finger configuration change is detected and the internal device pointer repositioned, so the actual pointer does not move. For a touch screen, you expect the pointer to be below the single finger on the surface. When the touches are separated in time, this is natural. When the touches fall within the same frame, it is still true, but seems odd. We are looking at the breakdown of the pointer concept, which is quite far beyond scope of this patch. >>> I wonder if a better solution would be single-touch emulation by only >>> sending BTN_TOUCH and ABS_{X,Y} events on the first touch started and >>> not sending these events on the second touch started when the first is >>> released. >> >> >> As stated in the pretext, the patch is a temporary solution until a proper >> solution involving the hid core has been found. The logic you describe is >> basically what Stephane Chatty implemented, and which this patch temporarily >> removes. > > The original code switches the BTN_TOUCH and ABS_{X,Y} to the oldest > touch when the current oldest touch is removed. After your patch, this > jumping around will still occur. That seems wrong to me, but this is a > murky area. See comment above. > There isn't a generally accepted mode of handling > single-touch legacy support in multitouch drivers (not that I'm aware of > at least, you may know more than I :). Thus, I'm wondering how well it > would work if we only sent these single-touch events for the first > touch, and no events for any further touches until they are all removed. > This would allow single-touch drag when multiple fingers are down to > behave right, in my opinion, such that the end of the finger doing the > dragging causes dragging to cease. These are all interesting thoughts, but perhaps best left to another forum. > Of course, this is all an issue of MT -> ST translation, and hopefully > all apps will become MT aware and not depend on ABS_{X,Y} when ABS_MT_* > are available :). Pointer emulation is looking more and more like a gesture, doesn't it. ;-) Cheers, Henrik
On Mon, 2010-08-16 at 08:55 +0200, Henrik Rydberg wrote: > On 08/15/2010 11:28 PM, Chase Douglas wrote: > > > On Sun, 2010-08-15 at 22:32 +0200, Henrik Rydberg wrote: > >> On 08/15/2010 07:42 PM, Chase Douglas wrote: > >> > >>> On Sat, 2010-08-14 at 17:29 +0100, Henrik Rydberg wrote: > >>>> The ten-finger 3M screen M2256PW has a kernel driver already > >>>> (hid-3m-pct), but with a couple of glitches: > >>>> > >>>> 1. The one-touch down/up, which emits BTN_TOUCH events, sometimes does > >>>> not work properly. This results in buttons getting stuck in the > >>>> button-down state in the window manager. > >>>> > >>>> 2. The action after touch-up in one spot followed by a touch-down in > >>>> another spot sometimes causes a sequence of strange finger-tracing > >>>> events along the perimeter of the rectangle between the two spots. The > >>>> root of the problem lies in how the kernel inteprets the HID hybrid > >>>> event type which is used by the 3M device [1]. > >>>> > >>>> While a proper HID solution is being worked out upstream, this patch > >>>> simplifies the driver logic such that BTN_TOUCH and ABS_X/Y events are > >>>> emitted properly, in effect papering over the worst problems. Suggested > >>>> for inclusion in Maverick. > >>>> > >>>> [1] Suggested by Stephane Chatty, who also confirms the observed problems. > >>>> > >>>> Signed-off-by: Henrik Rydberg <rydberg@euromail.se> Based on the comments in this thread, I feel confident that this is likely the best we can do in the short time frame before Maverick is released, and really does help paper over a real issue with the HID stack as it currently exists. Because we are one of the first distros really attacking multitouch screens, getting as many working drivers as possible at this early stage is important, even if our changes are not long term solutions that would be accepted by upstream. So: Acked-by: Chase Douglas <chase.douglas@canonical.com>
Applied to Maverick linux master. Thanks, Leann On Sat, 2010-08-14 at 17:29 +0100, Henrik Rydberg wrote: > The ten-finger 3M screen M2256PW has a kernel driver already > (hid-3m-pct), but with a couple of glitches: > > 1. The one-touch down/up, which emits BTN_TOUCH events, sometimes does > not work properly. This results in buttons getting stuck in the > button-down state in the window manager. > > 2. The action after touch-up in one spot followed by a touch-down in > another spot sometimes causes a sequence of strange finger-tracing > events along the perimeter of the rectangle between the two spots. The > root of the problem lies in how the kernel inteprets the HID hybrid > event type which is used by the 3M device [1]. > > While a proper HID solution is being worked out upstream, this patch > simplifies the driver logic such that BTN_TOUCH and ABS_X/Y events are > emitted properly, in effect papering over the worst problems. Suggested > for inclusion in Maverick. > > [1] Suggested by Stephane Chatty, who also confirms the observed problems. > > Signed-off-by: Henrik Rydberg <rydberg@euromail.se> > --- > drivers/hid/hid-3m-pct.c | 47 +++++++++------------------------------------ > 1 files changed, 10 insertions(+), 37 deletions(-) > > diff --git a/drivers/hid/hid-3m-pct.c b/drivers/hid/hid-3m-pct.c > index 2a0d56b..fc57c0c 100644 > --- a/drivers/hid/hid-3m-pct.c > +++ b/drivers/hid/hid-3m-pct.c > @@ -125,9 +125,7 @@ static int mmm_input_mapped(struct hid_device *hdev, struct hid_input *hi, > */ > static void mmm_filter_event(struct mmm_data *md, struct input_dev *input) > { > - struct mmm_finger *oldest = 0; > - bool pressed = false, released = false; > - int i; > + int i, index = -1; > > /* > * we need to iterate on all fingers to decide if we have a press > @@ -149,45 +147,20 @@ static void mmm_filter_event(struct mmm_data *md, struct input_dev *input) > input_event(input, EV_ABS, ABS_MT_TOUCH_MINOR, > wide ? f->h : f->w); > input_mt_sync(input); > - /* > - * touchscreen emulation: maintain the age rank > - * of this finger, decide if we have a press > - */ > - if (f->rank == 0) { > - f->rank = ++(md->num); > - if (f->rank == 1) > - pressed = true; > - } > - if (f->rank == 1) > - oldest = f; > - } else { > - /* this finger took off the screen */ > - /* touchscreen emulation: maintain age rank of others */ > - int j; > - > - for (j = 0; j < 10; ++j) { > - struct mmm_finger *g = &md->f[j]; > - if (g->rank > f->rank) { > - g->rank--; > - if (g->rank == 1) > - oldest = g; > - } > - } > - f->rank = 0; > - --(md->num); > - if (md->num == 0) > - released = true; > + > + if (index < 0) > + index = i; > } > f->valid = 0; > } > > /* touchscreen emulation */ > - if (oldest) { > - if (pressed) > - input_event(input, EV_KEY, BTN_TOUCH, 1); > - input_event(input, EV_ABS, ABS_X, oldest->x); > - input_event(input, EV_ABS, ABS_Y, oldest->y); > - } else if (released) { > + if (index >= 0) { > + struct mmm_finger *f = &md->f[index]; > + input_event(input, EV_KEY, BTN_TOUCH, 1); > + input_event(input, EV_ABS, ABS_X, f->x); > + input_event(input, EV_ABS, ABS_Y, f->y); > + } else { > input_event(input, EV_KEY, BTN_TOUCH, 0); > } > } > -- > 1.7.1 > >
diff --git a/drivers/hid/hid-3m-pct.c b/drivers/hid/hid-3m-pct.c index 2a0d56b..fc57c0c 100644 --- a/drivers/hid/hid-3m-pct.c +++ b/drivers/hid/hid-3m-pct.c @@ -125,9 +125,7 @@ static int mmm_input_mapped(struct hid_device *hdev, struct hid_input *hi, */ static void mmm_filter_event(struct mmm_data *md, struct input_dev *input) { - struct mmm_finger *oldest = 0; - bool pressed = false, released = false; - int i; + int i, index = -1; /* * we need to iterate on all fingers to decide if we have a press @@ -149,45 +147,20 @@ static void mmm_filter_event(struct mmm_data *md, struct input_dev *input) input_event(input, EV_ABS, ABS_MT_TOUCH_MINOR, wide ? f->h : f->w); input_mt_sync(input); - /* - * touchscreen emulation: maintain the age rank - * of this finger, decide if we have a press - */ - if (f->rank == 0) { - f->rank = ++(md->num); - if (f->rank == 1) - pressed = true; - } - if (f->rank == 1) - oldest = f; - } else { - /* this finger took off the screen */ - /* touchscreen emulation: maintain age rank of others */ - int j; - - for (j = 0; j < 10; ++j) { - struct mmm_finger *g = &md->f[j]; - if (g->rank > f->rank) { - g->rank--; - if (g->rank == 1) - oldest = g; - } - } - f->rank = 0; - --(md->num); - if (md->num == 0) - released = true; + + if (index < 0) + index = i; } f->valid = 0; } /* touchscreen emulation */ - if (oldest) { - if (pressed) - input_event(input, EV_KEY, BTN_TOUCH, 1); - input_event(input, EV_ABS, ABS_X, oldest->x); - input_event(input, EV_ABS, ABS_Y, oldest->y); - } else if (released) { + if (index >= 0) { + struct mmm_finger *f = &md->f[index]; + input_event(input, EV_KEY, BTN_TOUCH, 1); + input_event(input, EV_ABS, ABS_X, f->x); + input_event(input, EV_ABS, ABS_Y, f->y); + } else { input_event(input, EV_KEY, BTN_TOUCH, 0); } }
The ten-finger 3M screen M2256PW has a kernel driver already (hid-3m-pct), but with a couple of glitches: 1. The one-touch down/up, which emits BTN_TOUCH events, sometimes does not work properly. This results in buttons getting stuck in the button-down state in the window manager. 2. The action after touch-up in one spot followed by a touch-down in another spot sometimes causes a sequence of strange finger-tracing events along the perimeter of the rectangle between the two spots. The root of the problem lies in how the kernel inteprets the HID hybrid event type which is used by the 3M device [1]. While a proper HID solution is being worked out upstream, this patch simplifies the driver logic such that BTN_TOUCH and ABS_X/Y events are emitted properly, in effect papering over the worst problems. Suggested for inclusion in Maverick. [1] Suggested by Stephane Chatty, who also confirms the observed problems. Signed-off-by: Henrik Rydberg <rydberg@euromail.se> --- drivers/hid/hid-3m-pct.c | 47 +++++++++------------------------------------ 1 files changed, 10 insertions(+), 37 deletions(-)