All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Sebastian Kapfer <sebastian_kapfer@gmx.net>
Cc: Linux Input ML <linux-input@vger.kernel.org>
Subject: Re: [PATCH] 9-byte Alps, revisited
Date: Thu, 19 Nov 2009 16:34:34 -0800	[thread overview]
Message-ID: <20091120003433.GA21522@core.coreip.homeip.net> (raw)
In-Reply-To: <1258675669.5044.540.camel@sardelle.necksus.de>

Hi Sebastian,

On Fri, Nov 20, 2009 at 01:07:49AM +0100, Sebastian Kapfer wrote:
> 
> Hi Dmitry,
> 
> thank you for your reply.  It is much cleaner now!  I had to make a few
> changes though:
> 
> 1. As posted, the rearranged patch doesn't work properly.

Kinda expected that ;) That's why I asked to try it out.

>  One has to
> retain the sign bits of the PS/2 subpacket.
> 

Yes, indeed.

> 2. I've also pulled the checking of byte0 before the demuxing of the
> PS/2 subpacket.  I think it's safer to desync early if the data is bad.

It does not matter - byte0 does not change when you are receiving bytes 1
through 5 - if you already checked then it is still good.

> 
> 3. The hardware is very broken:  Touchpad and trackpoint share button
> state.  This means that you can trigger this sequence of events:
> 
> 	<user presses button on trackpoint>
> 	3byte:  left down  --> reported to "dev2"
> 	<user moves pointer with touchpad>
> 	6byte:  left down  --> reported to "dev"
> 	<user releases button on trackpoint>
> 	3byte:  left up    --> reported to "dev2"
> 
> The result is that dev has a stuck mouse button, and in X11 the mouse
> button stays down.  That's why I explicitly cloned button events to both
> devices in my first patch. 
> 
> However, this created a different problem:  If the user hammered at the
> mouse button very quickly, the events would be processed out of order,
> e.g.
> 
> 	touch_press touch_release stick_press stick_release
> 
> instead of
> 
> 	touch_press stick_press touch_release stick_release
> 
> As a result of this, a double click was detected in X11.
> 
> I have added logic that assigns each button down event to only one of
> the devices, and also avoids hanging buttons.  This is activated by a
> new flag.
> 

How about we just don't report button presses on the device representing
the stick? This is how Synaptics touchpads with pass-through ports work
(all buttons belong to the touchpad) and it seems to be working very well.

> (I'm pretty sure the shared button state is true for most if not all
> Alps dualpoints, but I made a separate flag out of it for now).
> 
> 4. I've noticed the applied patch for the 4-button Alps device. Is it
> really intended that one of the buttons fires both a BTN_x event and a
> BTN_MIDDLE event?  I don't think so, even tough they share the same bit
> in the packet.  BTN_MIDDLE should never be emitted from a device with
> the ALPS_FOUR_BUTTONS flag IMHO.  I haven't changed this, never having
> seen such a unit.

There is another patch that clears BTN_MIDDLE on the ones that have 4
directional button so input core will deliver either one or the other to
the clients.

> 
> 5. There remains a slight conceptual problem.  The distinction between
> 6-byte and 9-byte packets is not possible only looking at the first 6
> bytes.  (This is a protocoll issue.  We have scrutinized every bit now,
> the protocol just seems to be broken in this regard.)
> 
> This means that if the user triggers a 6-byte message while holding all
> three buttons down (e.g. hold buttons and move pointer via touchpad), we
> run into de-sync.
> 
> We can't solve this without knowing the message size in the driver.  I
> have no idea if we can somehow get this info out of the PS/2 layer.
> Dmitry, do you have any insight into this?

I had another version of the patch that looked at the 7th byte before
deciding if it was standard or interleaved packet instead of using
ALPS_PS2_INTERLEAVD flag but I decided it was too complex. If the device
in E6x00 indeed has 3 buttons then I can ressurect it.

> 
> I still vote strongly for applying the patch, since this is a mostly
> cosmetic problem that never occurs in practical work whereas in the
> current state my touchpad is unusable.
>

Hmm, it is too late for .32 but maybe we can respin it for stable oncde
we hammer out the functionality.

-- 
Dmitry

  reply	other threads:[~2009-11-20  0:34 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-11-15 19:42 [PATCH] Alps dualpoint touchpads losing sync [buttons fixed too] Sebastian Kapfer
2009-11-18  9:00 ` Dmitry Torokhov
2009-11-20  0:07   ` [PATCH] 9-byte Alps, revisited Sebastian Kapfer
2009-11-20  0:34     ` Dmitry Torokhov [this message]
2009-11-20  0:55       ` Sebastian Kapfer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20091120003433.GA21522@core.coreip.homeip.net \
    --to=dmitry.torokhov@gmail.com \
    --cc=linux-input@vger.kernel.org \
    --cc=sebastian_kapfer@gmx.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.