[PATCH] ClickPad support v4
Chris Bagwell
chris at cnpbagwell.com
Fri Dec 17 11:18:42 PST 2010
OK, I've re-reviewed patch and I've decided I understand what its
trying to do now. Most my original comments still apply but I've
added new ones.
First, I need to confirm intent of patch is this:
* Create a rectangle defined by {Top|Bottom|Left|Right}Edge that
excludes button area in attempt to cause cursor not to move when in
that area.
* Allow edge scrolling, circular scrolling, and tap gestures; but not
cursor movement; to work even when outside this smaller area.
Can you confirm this is intent? Once I know then I can provide better
feedback and maybe even re-send updated patches.
We have Area*Edge defined to reduce touchpad area for movement and
gestures but it sounds like clickpads may need a new setting to
define area that stops only movement but allows gestures?
The definition of {Top|Bottom|Left|Right}Edge is a little fuzzy to me.
Maybe its OK for "movement" only control. Peter, what do you think?
More below.
On Wed, Dec 8, 2010 at 1:55 AM, Yan Li <yan.i.li at intel.com> wrote:
> This patch adds the support for Synaptics Clickpad devices.
> It requires the change in Linux kernel synaptics input driver, found in
> https://patchwork.kernel.org/patch/92435/
> The kernel patch is already included in 2.6.34 and later releases.
>
> When the kernel driver sets only the left-button bit evbit and no
> multi-finger is possible, Clickpad mode is activated. In this mode,
> the bottom touch area is used as button emulations. Clicking at the
> bottom-left, bottom-center and bottom-right zone corresponds to a left,
> center and right click.
>
> v2->v3: Fix the mis-detection of Clickpad device with double-tap feature
> (e.g. MacBook)
> Fix one forgotten spacing issue Peter suggested
>
> v3->v4: Ported to HEAD by Yan Li for MeeGo, also added ClickPad
> description to man page.
>
> Signed-off-by: Takashi Iwai <tiwai at suse.de>
> Signed-off-by: Yan Li <yan.i.li at intel.com>
> ---
> man/synaptics.man | 8 ++++++
> src/eventcomm.c | 7 +++++
> src/synaptics.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++-
> src/synapticsstr.h | 2 +
> 4 files changed, 88 insertions(+), 1 deletions(-)
>
> diff --git a/man/synaptics.man b/man/synaptics.man
> index 3f1ca9d..25f1115 100644
> --- a/man/synaptics.man
> +++ b/man/synaptics.man
> @@ -56,6 +56,14 @@ Pressure dependent motion speed.
> .IP \(bu 4
> Run-time configuration using shared memory. This means you can change
> parameter settings without restarting the X server.
> +.IP \(bu 4
> +Synaptics ClickPad support: ClickPad is a new kind of device from
> +Synaptics that has no visible physical keys. Instead, the whole board
> +is clickable and the device sends out BTN_MIDDLE only. It's the
> +driver's duty to judge whether the click is a left or right one
> +according to finger location. If the driver detects that the touchpad
> +has only one button, the ClickPad mode will be activated and handles
> +the action correctly.
> .LP
> Note that depending on the touchpad firmware, some of these features
> might be available even without using the synaptics driver. Note also
> diff --git a/src/eventcomm.c b/src/eventcomm.c
> index faa66ab..7da5a40 100644
> --- a/src/eventcomm.c
> +++ b/src/eventcomm.c
> @@ -269,6 +269,13 @@ event_query_axis_ranges(LocalDevicePtr local)
> }
>
> xf86Msg(X_PROBED, "%s: buttons:%s\n", local->name, buf);
> +
> + /* clickpad device reports only the single left button mask */
> + if (priv->has_left && !priv->has_right && !priv->has_middle && !priv->has_double) {
> + priv->is_clickpad = TRUE;
> + xf86Msg(X_INFO, "%s: is Clickpad device\n", local->name);
> + }
> +
> }
> }
>
> diff --git a/src/synaptics.c b/src/synaptics.c
> index 53c3685..2e5f8ae 100644
> --- a/src/synaptics.c
> +++ b/src/synaptics.c
> @@ -506,6 +506,18 @@ static void set_default_parameters(LocalDevicePtr local)
> vertResolution = priv->resy;
> }
>
> + /* Clickpad mode -- bottom area is used as buttons */
> + if (priv->is_clickpad) {
> + int button_bottom;
> + /* Clickpad devices usually the button area at the bottom, and
> + * its size seems ca. 20% of the touchpad height no matter how
> + * large the pad is.
> + */
> + button_bottom = priv->maxy - (abs(priv->maxy - priv->miny) * 20) / 100;
> + if (button_bottom < b && button_bottom >= t)
> + b = button_bottom;
> + }
> +
> /* set the parameters */
> pars->left_edge = xf86SetIntOption(opts, "LeftEdge", l);
> pars->right_edge = xf86SetIntOption(opts, "RightEdge", r);
> @@ -2153,6 +2165,59 @@ handle_clickfinger(SynapticsParameters *para, struct SynapticsHwState *hw)
> }
> }
>
> +/* clickpad event handling */
> +static void
> +HandleClickpad(LocalDevicePtr local, struct SynapticsHwState *hw, edge_type edge)
> +{
> + SynapticsPrivate *priv = (SynapticsPrivate *) (local->private);
> + SynapticsParameters *para = &priv->synpara;
> +
> + if (edge & BOTTOM_EDGE) {
> + /* button area */
> + int width = priv->maxx - priv->minx;
> + int left_button_x, right_button_x;
> +
> + /* left and right clickpad button ranges;
> + * the gap between them is interpreted as a middle-button click
> + */
> + left_button_x = width * 2 / 5 + priv->minx;
> + right_button_x = width * 3 / 5 + priv->minx;
> +
> + /* clickpad reports only one button, and we need
> + * to fake left/right buttons depending on the touch position
> + */
> + if (hw->left) { /* clicked? */
> + hw->left = 0;
> + if (hw->x < left_button_x)
> + hw->left = 1;
> + else if (hw->x > right_button_x)
> + hw->right = 1;
> + else
> + hw->middle = 1;
> + }
> +
> + /* Don't move pointer position in the button area during clicked,
> + * except for horiz/vert scrolling is enabled.
> + *
> + * The synaptics driver tends to be pretty sensitive. This hack
> + * is to avoid that the pointer moves slightly and misses the
> + * poistion you aimed to click.
> + *
> + * Also, when the pointer movement is reported, the dragging
> + * (with a sort of multi-touching) doesn't work well, too.
> + */
> + if (hw->left || !(para->scroll_edge_horiz ||
> + ((edge & RIGHT_EDGE) && para->scroll_edge_vert)))
> + hw->z = 0; /* don't move pointer */
> +
OK, I think this code is basically pointing out a bug unrelated to
clickpads. It seems only needed to me because we must not be
discarding values from touchpad that fall outside
{Top|Bottom|Left|Right}Edge. I only know for sure we discard those
outside Area*Edge.
Or is it trying to say HW sends X/Y values outside click area but
somehow affected by click? I know synaptics tracks first finger
during double touch but does adjust maybe 10% towards second finger
during that touch. But that jump is handled elsewhere when it sees
DOUBLETAP sent.
Something I'm missing here.
> + } else if (hw->left) {
Indentation issues make this hard to follow.
> + /* dragging */
> + hw->left = priv->prev_hw.left;
> + hw->right = priv->prev_hw.right;
> + hw->middle = priv->prev_hw.middle;
> + }
> + priv->prev_hw = *hw;
> +}
>
> /* Update the hardware state in shared memory. This is read-only these days,
> * nothing in the driver reads back from SHM. SHM configuration is a thing of the past.
> @@ -2344,6 +2409,12 @@ HandleState(LocalDevicePtr local, struct SynapticsHwState *hw)
> if (para->touchpad_off == 1)
> return delay;
>
> + edge = edge_detection(priv, hw->x, hw->y);
> +
This needs the "if (inside_active_area)" check to keep definition in
man page of Area*Edge.
Thats all for now.
Chris
> + /* Clickpad handling for button area */
> + if (priv->is_clickpad)
> + HandleClickpad(local, hw, edge);
> +
> inside_active_area = is_inside_active_area(priv, hw->x, hw->y);
>
> /* now we know that these _coordinates_ aren't in the area.
> @@ -2373,7 +2444,6 @@ HandleState(LocalDevicePtr local, struct SynapticsHwState *hw)
>
> /* no edge or finger detection outside of area */
> if (inside_active_area) {
> - edge = edge_detection(priv, hw->x, hw->y);
> finger = SynapticsDetectFinger(priv, hw);
> }
>
> diff --git a/src/synapticsstr.h b/src/synapticsstr.h
> index 658721c..5e4090e 100644
> --- a/src/synapticsstr.h
> +++ b/src/synapticsstr.h
> @@ -235,6 +235,8 @@ typedef struct _SynapticsPrivateRec
> Bool has_pressure; /* device reports pressure */
> Bool has_width; /* device reports finger width */
> Bool has_scrollbuttons; /* device has physical scrollbuttons */
> + Bool is_clickpad; /* is Clickpad device (one-button) */
> + struct SynapticsHwState prev_hw; /* previous h/w state (for clickpad) */
>
> enum TouchpadModel model; /* The detected model */
> } SynapticsPrivate;
> --
> 1.7.2.3
>
>
> --
> Best regards,
> Li, Yan
>
> MeeGo Team, Opensource Technology Center, SSG, Intel
> Office tel.: +86-10-82171695 (inet: 8-758-1695)
> OpenPGP key: 5C6C31EF
> IRC: yanli on network irc.freenode.net
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
>
More information about the xorg-devel
mailing list