Re: [PATCH 7/9 v2] Input: synaptics - improved 2->3 finger transition handling

[Daniel Kurtz]

On Thu, Jul 28, 2011 at 10:07 AM, Chase Douglas
<chase.douglas@xxxxxxxxxxxxx> wrote:
> On 07/27/2011 06:00 PM, Daniel Kurtz wrote:
>> On Thu, Jul 28, 2011 at 5:13 AM, Chase Douglas
>> <chase.douglas@xxxxxxxxxxxxx> wrote:
>> [...]
>>>>>
>>>>>> Would you prefer an implementation that continued to report count (via
>>>>>> BTN_TOUCH*) correctly, but dropped down to 0 or 1 MT-B slots when for
>>>>>> these cases where it could not determine the correct position or track_id
>>>>>> to report?
>>>>>
>>>>> That may be doable, but I would prefer to just assume that tracking ids
>>>>> are not valid when (tracked touches > reported touches).
>>>>
>>>> Userspace is free to make this assumption, of course, but, in fact,
>>>> the image sensor trackpad actually does a pretty good job of tracking
>>>> the fingers - it just has serious issues reporting them!
>>>> Since a track_id change is how userspace is told that the identity of
>>>> the reported finger is changing, if the track_id of a finger position
>>>> datum is unknowable, I'd rather just discard it in the kernel than
>>>> report it to userspace with the wrong track_id.
>>>> Otherwise, the heuristics used in the userspace finger tracking
>>>> algorithms would need to be overly aggressively tuned to handle this
>>>> known error cases:
>>>>   2->3 and 3->2 finger transitions look like 2nd finger motion,
>>>> instead of reported finger changes.
>>>>
>>>>>
>>>>>> It seems like it would be more work for userspace to handle this new way
>>>>>> than the simulated number-of-touch transitions, where the transient
>>>>>> states are all normal valid states.
>>>>>
>>>>> This harkens back to my earlier statements where I said this new
>>>>> Synaptics protocol is worse than the previous one :).
>>>>>
>>>>> I agree that the implementation you gave here might be trickier for
>>>>> userspace, so I'd rather table it unless you feel that the "tracking ids
>>>>> are meaningless!" solution won't work. If you think there are problems
>>>>> with that approach, we can re-evaluate.
>>>>>
>>>>> Thanks!
>>>>>
>>>>> -- Chase
>>>>
>>>> Yes, I feel there are problems with this approach, as I tried to explain above.
>>>> Can you explain why you 'continuation gestures' can't handle 1->2->3
>>>> finger transitions looking like 1->2->1->3, and 3->2->3 looking like
>>>> 3->2->0->3?
>>>>
>>>> I think the only real point left to decide is what BTN_* events should
>>>> signify during these rare transition states:
>>>>   (a) the actually number of fingers on the pad,
>>>>   (b) the number of fingers being reported via the slots
>>>>
>>>> The current patchset does (a).
>>>> We could do (b), if that would get these patches accepted sooner :)
>>>
>>> I was thinking that the current patchset does (b). I think (a) is
>>> better, and if it really works that way then I'm fine with it. It's hard
>>> for me to keep track of the flow of the logic across the patches :).
>>
>> Argh, my bad.  You are correct.  Current patchset does (b)!
>> It reports the number of active slots, not the number of touches.
>>
>> In any case, I really don't know why you need (a).  We are talking
>> about some degenerate transitions here.  Your userspace driver should
>> work just fine with the (b) driver, it just loses some really
>> complicated continued gestures for hardware that can't support them.
>
> To give userspace incorrect information about the number of touches on
> the device is always bad. Lets say the degenerate case is when you go
> from two touches to three (maybe Synaptics doesn't do this, but someone
> else might). When the user performs a three finger tap, we'd see two
> touches down, two lifted up, three touches down, three lifted up in
> short succession. Userspace is gonna get pretty confused about that :).
>
> (Please don't make me try to come up with a use case we already have in
> Unity that would be broken for Synaptics due to this. I could probably
> find one, but it would take quite a bit of thinking. :)

Just to be clear:

Debouncing num-fingers at the start of a tap works just fine.
For a 3f-tap, you would see one of:
  0->1->2->3
  0->2->3
  0->3

Not:
  0->1->2->0->3
  0->2->0->3

You only see 2->0->3 if there had previously been 3 fingers down, and
some of those fingers are removed:
  0->3->0*->2*->0*->3*
  0->3->0*->1*

Where the "*" states are when mt_state_lost = true.
So, with method (b), you might see 3->0->2->0 if the release of the
other two fingers was really slow (longer than 37.5 ms), or, more
likely on a tap, just 3->0.

In any case, I don't think we are making progress arguing (a) vs. (b).
Let me implement method (a), and upload for review.
I agree that it makes some sense to always accurately report number of
touches with the BTN_*, independent of number of slots.

A true MT-B userspace app would always do the right thing with the
slots, legacy apps can always do the right thing in legacy mode, and a
hybrid app get do 2-touch multitouch & use BTN_* to determine total
number of touches.

Was there anything else I should add/change while I'm at it?

You mention documentation below, was there something in particular
that you'd like to see documented better?  Where?

-Dan

>
>>> That said, merging this patchset as is effectively means that the number
>>> of slots is completely decoupled from the number of touches on the
>>> device. Previously, one could say that the number of touches on the
>>> device was equal to the number of open slots or more if all slots were
>>> open. With this approach, we could have 0 slots open during a transition
>>> when there are still touches down.
>>>
>>> While the distinction makes sense for these synaptics devices, I don't
>>> want the semantics to hold for full multitouch devices. Otherwise, we
>>> would have to add many more BTN_*TAPs. If we go this route, we must have
>>> a way to tell userspace that this is a special device where the number
>>> of active touches can only be determined from BTN_*TAP. Thus, we would
>>> need a property for this exception to normal behavior.
>>
>> Henrik & Dmitry roughly suggested "do not define a new property; let
>> userspace detect max BTN_*TAP > ABS_MT_SLOT.max to indicate that
>> BTN_*TAP carries the total number of touches".  I wish they would
>> chime in on this patchset...
>
> I think it sets a really bad precedent to obey the stated protocol in
> most cases, but disregard it in others if specific conditions are met,
> which are not documented and are not presented in a clear manner to
> userspace. At the *very least*, this change would need documentation to
> go in at the same time, but I strongly believe a property is merited here.
>
> -- Chase
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/