[PATCH 0/4] HID: magicmouse: improve scrolling

[PATCH 0/4] HID: magicmouse: improve scrolling

[Chase Douglas]

I love my Magic Mouse, and Michael Poole did a great job creating a new
driver for it in Linux. However, the scrolling behavior in Linux has
been a pet peeve of mine. I am proposing a few patches to change the
behavior in the following ways:

* Scroll on entire surface of mouse instead of an artificial "middle"
  area.
* Fix up a scroll state math issue (trivial).
* Add a param for enabling scroll acceleration, but disable it by
  default. It's unique to magicmouse I believe, and isn't intuitive.
* Add a param for scroll speed (i.e. distance a touch needs to travel on
  the surface to produce a WHEEL event). Clean up acceleration code as
  well because it is tightly integrated with the speed logic.

After these four patches, setting scroll_speed to 48, and setting
ConstantDeceleration to 2 in xorg.conf, I have found my mouse to be much
better.

Thanks,

-- Chase

 drivers/hid/hid-magicmouse.c |   49 +++++++++++++++++++++++++++++------------
 1 files changed, 34 insertions(+), 15 deletions(-)

[PATCH 1/4] HID: magicmouse: scroll on entire surface, not just middle of mouse

[Chase Douglas]

Previously, scroll events only occurred when the user moved a touch
along the middle of the touch surface. This is unintuitive for a normal
user who is not aware of this. The device has a uniform surface, so the
distinction is artificial. This change removes the touch area check for
a scroll event, which replicates the OS X behavior.

Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
---
 drivers/hid/hid-magicmouse.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index f10d56a..cd70635 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -160,10 +160,9 @@ static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tda
 	msc->touches[id].size = misc & 63;
 
 	/* If requested, emulate a scroll wheel by detecting small
-	 * vertical touch motions along the middle of the mouse.
+	 * vertical touch motions.
 	 */
-	if (emulate_scroll_wheel &&
-	    middle_button_start < x && x < middle_button_stop) {
+	if (emulate_scroll_wheel) {
 		static const int accel_profile[] = {
 			256, 228, 192, 160, 128, 96, 64, 32,
 		};
-- 
1.7.0.4

[PATCH 2/4] HID: magicmouse: properly account for scroll movement in state

[Chase Douglas]

Before this change, sequential scroll events would take a variable
amount of movement due to incorrect accounting. This change ensures all
scroll movements require a deterministic touch movement for an action to
occur.

Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
---
 drivers/hid/hid-magicmouse.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index cd70635..171ed0c 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -183,7 +183,8 @@ static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tda
 		case TOUCH_STATE_DRAG:
 			step = step / accel_profile[msc->scroll_accel];
 			if (step != 0) {
-				msc->touches[id].scroll_y = y;
+				msc->touches[id].scroll_y -=
+					step * accel_profile[msc->scroll_accel];
 				msc->scroll_jiffies = now;
 				input_report_rel(input, REL_WHEEL, step);
 			}
-- 
1.7.0.4

Re: [PATCH 2/4] HID: magicmouse: properly account for scroll movement in state

[Michael Poole]

Chase Douglas writes:

> Before this change, sequential scroll events would take a variable
> amount of movement due to incorrect accounting. This change ensures all
> scroll movements require a deterministic touch movement for an action to
> occur.
>
> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>

My apologies for the delay in reviewing this one.  I understand now, and
agree it's the right thing to do here.

Acked-by: Michael Poole <mdpoole@troilus.org>

(For the rest of the list, I acked the other three patches in this
series via private email earlier.)

Michael Poole

> ---
>  drivers/hid/hid-magicmouse.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
> index cd70635..171ed0c 100644
> --- a/drivers/hid/hid-magicmouse.c
> +++ b/drivers/hid/hid-magicmouse.c
> @@ -183,7 +183,8 @@ static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tda
>  		case TOUCH_STATE_DRAG:
>  			step = step / accel_profile[msc->scroll_accel];
>  			if (step != 0) {
> -				msc->touches[id].scroll_y = y;
> +				msc->touches[id].scroll_y -=
> +					step * accel_profile[msc->scroll_accel];
>  				msc->scroll_jiffies = now;
>  				input_report_rel(input, REL_WHEEL, step);
>  			}

Re: [PATCH 2/4] HID: magicmouse: properly account for scroll movement in state

[Chase Douglas]

On Wed, 2010-06-09 at 21:25 -0400, Michael Poole wrote:
> Chase Douglas writes:
> 
> > Before this change, sequential scroll events would take a variable
> > amount of movement due to incorrect accounting. This change ensures all
> > scroll movements require a deterministic touch movement for an action to
> > occur.
> >
> > Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> 
> My apologies for the delay in reviewing this one.  I understand now, and
> agree it's the right thing to do here.
> 
> Acked-by: Michael Poole <mdpoole@troilus.org>
> 
> (For the rest of the list, I acked the other three patches in this
> series via private email earlier.)
> 
> Michael Poole

Jiri,

What's the status on the rest of these patches that need to be added in?

Thanks,

-- Chase

Re: [PATCH 2/4] HID: magicmouse: properly account for scroll movement in state

[Jiri Kosina]

On Wed, 16 Jun 2010, Chase Douglas wrote:

> > > Before this change, sequential scroll events would take a variable
> > > amount of movement due to incorrect accounting. This change ensures all
> > > scroll movements require a deterministic touch movement for an action to
> > > occur.
> > >
> > > Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> > 
> > My apologies for the delay in reviewing this one.  I understand now, and
> > agree it's the right thing to do here.
> > 
> > Acked-by: Michael Poole <mdpoole@troilus.org>
> > 
> > (For the rest of the list, I acked the other three patches in this
> > series via private email earlier.)
> > 
> > Michael Poole
> 
> Jiri,
> 
> What's the status on the rest of these patches that need to be added in?

I wasn't aware of any magicmouse patches waiting for me ... if there are 
things that are destined to be applied on top of what we currently have, 
please be so kind and send them to me again.

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

[PATCH 3/4] HID: magicmouse: disable and add module param for scroll acceleration

[Chase Douglas]

Scroll acceleration is unique to the magicmouse driver, and is
unintuitive to a user who is unaware of the functionality. Thus, disable
it by default, but add a module parameter to enable it for power users
who want it.

Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
---
 drivers/hid/hid-magicmouse.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 171ed0c..f44aaf2 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -30,6 +30,10 @@ static bool emulate_scroll_wheel = true;
 module_param(emulate_scroll_wheel, bool, 0644);
 MODULE_PARM_DESC(emulate_scroll_wheel, "Emulate a scroll wheel");
 
+static bool scroll_acceleration = false;
+module_param(scroll_acceleration, bool, 0644);
+MODULE_PARM_DESC(scroll_acceleration, "Accelerate sequential scroll events");
+
 static bool report_touches = true;
 module_param(report_touches, bool, 0644);
 MODULE_PARM_DESC(report_touches, "Emit touch records (otherwise, only use them for emulation)");
@@ -177,7 +181,9 @@ static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tda
 		switch (tdata[7] & TOUCH_STATE_MASK) {
 		case TOUCH_STATE_START:
 			msc->touches[id].scroll_y = y;
-			msc->scroll_accel = min_t(int, msc->scroll_accel + 1,
+			if (scroll_acceleration)
+				msc->scroll_accel = min_t(int,
+						msc->scroll_accel + 1,
 						ARRAY_SIZE(accel_profile) - 1);
 			break;
 		case TOUCH_STATE_DRAG:
-- 
1.7.0.4

[PATCH 4/4] HID: magicmouse: add param for scroll speed

[Chase Douglas]

The new scroll_speed param takes an integer value from 0 to 63, where 0
is slowest and 63 is fastest. The default of 32 remains the same. This
parameter also affects scroll acceleration linearly.

A second part of this change is a tightly coupled modification to the
scroll acceleration. Previously, scroll acceleration could be reset
without lifting the scroll finger. This is rather unintuitive and hard
to control in the case where a user wants faster scrolling, but wants to
hold the scroll touch for longer than a moment.

Note that scroll acceleration levels are now 1-7, where 7 is slowest. In
the previous implementation, there were 8 levels defined, but it was
impossible to start at the slowest level. In order to keep the default
scroll speed unchanged, only 7 levels are used now.

Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
---
 drivers/hid/hid-magicmouse.c |   43 +++++++++++++++++++++++++++--------------
 1 files changed, 28 insertions(+), 15 deletions(-)

diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index f44aaf2..fe0c760 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -30,6 +30,17 @@ static bool emulate_scroll_wheel = true;
 module_param(emulate_scroll_wheel, bool, 0644);
 MODULE_PARM_DESC(emulate_scroll_wheel, "Emulate a scroll wheel");
 
+static unsigned int scroll_speed = 32;
+static int param_set_scroll_speed(const char *val, struct kernel_param *kp) {
+	unsigned long speed;
+	if (!val || strict_strtoul(val, 0, &speed) || speed > 63)
+		return -EINVAL;
+	scroll_speed = speed;
+	return 0;
+}
+module_param_call(scroll_speed, param_set_scroll_speed, param_get_uint, &scroll_speed, 0644);
+MODULE_PARM_DESC(scroll_speed, "Scroll speed, value from 0 (slow) to 63 (fast)");
+
 static bool scroll_acceleration = false;
 module_param(scroll_acceleration, bool, 0644);
 MODULE_PARM_DESC(scroll_acceleration, "Accelerate sequential scroll events");
@@ -54,6 +65,8 @@ MODULE_PARM_DESC(report_undeciphered, "Report undeciphered multi-touch state fie
 #define TOUCH_STATE_START 0x30
 #define TOUCH_STATE_DRAG  0x40
 
+#define SCROLL_ACCEL_DEFAULT 7
+
 /**
  * struct magicmouse_sc - Tracks Magic Mouse-specific data.
  * @input: Input device through which we report events.
@@ -145,7 +158,7 @@ static void magicmouse_emit_buttons(struct magicmouse_sc *msc, int state)
 	input_report_key(msc->input, BTN_RIGHT, state & 2);
 
 	if (state != last_state)
-		msc->scroll_accel = 0;
+		msc->scroll_accel = SCROLL_ACCEL_DEFAULT;
 }
 
 static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tdata)
@@ -167,30 +180,28 @@ static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tda
 	 * vertical touch motions.
 	 */
 	if (emulate_scroll_wheel) {
-		static const int accel_profile[] = {
-			256, 228, 192, 160, 128, 96, 64, 32,
-		};
 		unsigned long now = jiffies;
 		int step = msc->touches[id].scroll_y - y;
 
-		/* Reset acceleration after half a second. */
-		if (time_after(now, msc->scroll_jiffies + HZ / 2))
-			msc->scroll_accel = 0;
-
 		/* Calculate and apply the scroll motion. */
 		switch (tdata[7] & TOUCH_STATE_MASK) {
 		case TOUCH_STATE_START:
 			msc->touches[id].scroll_y = y;
-			if (scroll_acceleration)
-				msc->scroll_accel = min_t(int,
-						msc->scroll_accel + 1,
-						ARRAY_SIZE(accel_profile) - 1);
+
+			/* Reset acceleration after half a second. */
+			if (scroll_acceleration && time_before(now,
+						msc->scroll_jiffies + HZ / 2))
+				msc->scroll_accel = max_t(int,
+						msc->scroll_accel - 1, 1);
+			else
+				msc->scroll_accel = SCROLL_ACCEL_DEFAULT;
+
 			break;
 		case TOUCH_STATE_DRAG:
-			step = step / accel_profile[msc->scroll_accel];
+			step /= (64 - (int)scroll_speed) * msc->scroll_accel;
 			if (step != 0) {
-				msc->touches[id].scroll_y -=
-					step * accel_profile[msc->scroll_accel];
+				msc->touches[id].scroll_y -= step *
+					(64 - scroll_speed) * msc->scroll_accel;
 				msc->scroll_jiffies = now;
 				input_report_rel(input, REL_WHEEL, step);
 			}
@@ -351,6 +362,8 @@ static int magicmouse_probe(struct hid_device *hdev,
 		return -ENOMEM;
 	}
 
+	msc->scroll_accel = SCROLL_ACCEL_DEFAULT;
+
 	msc->quirks = id->driver_data;
 	hid_set_drvdata(hdev, msc);
 
-- 
1.7.0.4