[PATCH] Make ati_remote button repeat sensitivity soft-configurable

[PATCH] Make ati_remote button repeat sensitivity soft-configurable

[Karl Pickett]

The ati_remote driver is a little too sensitive for my wife... if you
do anything but barely tap the button you can get multiple events
reported.  We prefer a more conservative no-repeat setting.  This is a
pretty trivial patch which just makes one hard coded value soft
configurable and does not change the default.

--- linux-source-2.6.20/drivers/usb/input/ati_remote.c.kjp
2006-12-07 22:50:56.000000000 -0500
+++ linux-source-2.6.20/drivers/usb/input/ati_remote.c  2007-04-03
20:19:24.000000000 -0400
@@ -119,7 +119,9 @@
  * and we have to take this into account for an accurate repeat
  * behaviour.
  */
-#define FILTER_TIME    60 /* msec */
+#define FILTER_TIME    60 /* millisec between each repeat */
+
+#define FILTER_MAX     5 /* number of repeats to ignore */

 static unsigned long channel_mask;
 module_param(channel_mask, ulong, 0644);
@@ -133,6 +135,11 @@
 module_param(repeat_filter, int, 0644);
 MODULE_PARM_DESC(repeat_filter, "Repeat filter time, default = 60 msec");

+static int repeat_filter_max = FILTER_MAX;
+module_param(repeat_filter_max, int, 0644);
+MODULE_PARM_DESC(repeat_filter_max,
+       "Number of repeats to ignore, default = 5");
+
 #define dbginfo(dev, format, arg...) do { if (debug) dev_info(dev ,
format , ## arg); } while (0)
 #undef err
 #define err(format, arg...) printk(KERN_ERR format , ## arg)
@@ -515,7 +522,7 @@
                ati_remote->old_jiffies = jiffies;

                if (ati_remote->repeat_count > 0 &&
-                   ati_remote->repeat_count < 5)
+                   ati_remote->repeat_count < repeat_filter_max)
                        return;




-- 
Karl Pickett
The most wasted of all days is one without laughter.—E.E. Cummings

Re: [PATCH] Make ati_remote button repeat sensitivity soft-configurable

[Dmitry Torokhov]

Hi Karl,

On Tuesday 03 April 2007 20:34, Karl Pickett wrote:
> The ati_remote driver is a little too sensitive for my wife... if you
> do anything but barely tap the button you can get multiple events
> reported.  We prefer a more conservative no-repeat setting.  This is a
> pretty trivial patch which just makes one hard coded value soft
> configurable and does not change the default.
> 

Makes sense. Can I please have a Signed-off-by: ... line from you so
I can apply the patch to my tree?

Thanks!

-- 
Dmitry

Re: [PATCH] Make ati_remote button repeat sensitivity soft-configurable

[Éric Piel]

04/04/2007 02:34 AM, Karl Pickett wrote/a écrit:
> The ati_remote driver is a little too sensitive for my wife... if you
> do anything but barely tap the button you can get multiple events
> reported.  We prefer a more conservative no-repeat setting.  This is a
> pretty trivial patch which just makes one hard coded value soft
> configurable and does not change the default.
This default value is set to 300 ms. On my Xserver, the default value is 
660 ms and by default in my distrib it's set to 500 ms. So, indeed, the 
default value of the ati_remote is quite small. Maybe you could increase 
FILTER_MAX to 10 (= 600 ms) in order to have something saner?

That was my two cents :-)
See you,
Eric

Re: [PATCH] Make ati_remote button repeat sensitivity soft-configurable

[Vincent Vanackere]

On 4/4/07, Éric Piel <Eric.Piel@tremplin-utc.net> wrote:

> This default value is set to 300 ms. On my Xserver, the default value is
> 660 ms and by default in my distrib it's set to 500 ms. So, indeed, the
> default value of the ati_remote is quite small. Maybe you could increase
> FILTER_MAX to 10 (= 600 ms) in order to have something saner?

Well, this value was chosen because it matches what the hardware does
: as explained in the comments, the hardware generates exactly 5
events for the first keypress (spaced by ~50ms if I recall correctly).

At least on my hardware (=> Vendor=0bc7 ProdID=0004 Rev= 1.00), the
current default value match exactly what I'm expecting (never had the
problem : "if you do anything but barely tap the button you can get
multiple events reported").
Just a thought : perhaps some models are more sensitive than others
(and so the default should be made dependent on the exact remote
model)... what is your hardware id ?

Best regards,

Vincent

Re: [PATCH] Make ati_remote button repeat sensitivity soft-configurable

[Karl Pickett]

Here is some more output...

[300950.438977] drivers/usb/input/ati_remote.c: Registered USB driver
ATI/X10 RF USB Remote Control v. 2.2.1
[300950.445902] drivers/usb/input/ati_remote.c: Weird data, len=1 ff
54 1f 60 13 20 ... (I only get that once, at startup)

Bus 001 Device 002: ID 0bc7:0004 X10 Wireless Technology, Inc. X10 Receiver
Device Descriptor:
  bLength                18
  bDescriptorType         1
  bcdUSB               1.10
  bDeviceClass            0 (Defined at Interface level)
  bDeviceSubClass         0
  bDeviceProtocol         0
  bMaxPacketSize0         8
  idVendor           0x0bc7 X10 Wireless Technology, Inc.
  idProduct          0x0004 X10 Receiver
  bcdDevice            1.00
  iManufacturer           1
  iProduct                2
  iSerial                 0
  bNumConfigurations      1

This is me holding down a button with debug on:
Apr  4 09:27:32 karl-desktop kernel: [346509.808778] ati_remote
1-1:1.0: channel 0x06; data 54,1f; index 41; keycode 106
Apr  4 09:27:32 karl-desktop kernel: [346509.856726] ati_remote
1-1:1.0: channel 0x06; data 54,1f; index 41; keycode 106
Apr  4 09:27:32 karl-desktop kernel: [346509.896685] ati_remote
1-1:1.0: channel 0x06; data 54,1f; index 41; keycode 106
Apr  4 09:27:32 karl-desktop kernel: [346509.944634] ati_remote
1-1:1.0: channel 0x06; data 54,1f; index 41; keycode 106
Apr  4 09:27:32 karl-desktop kernel: [346509.984587] ati_remote
1-1:1.0: channel 0x06; data 54,1f; index 41; keycode 106
Apr  4 09:27:32 karl-desktop kernel: [346510.032540] ati_remote
1-1:1.0: channel 0x06; data 54,1f; index 41; keycode 106
Apr  4 09:27:32 karl-desktop kernel: [346510.072499] ati_remote
1-1:1.0: channel 0x06; data 54,1f; index 41; keycode 106
Apr  4 09:27:32 karl-desktop kernel: [346510.120453] ati_remote
1-1:1.0: channel 0x06; data 54,1f; index 41; keycode 106
Apr  4 09:27:32 karl-desktop kernel: [346510.160414] ati_remote
1-1:1.0: channel 0x06; data 54,1f; index 41; keycode 106
Apr  4 09:27:32 karl-desktop kernel: [346510.208361] ati_remote
1-1:1.0: channel 0x06; data 54,1f; index 41; keycode 106
Apr  4 09:27:32 karl-desktop kernel: [346510.248318] ati_remote
1-1:1.0: channel 0x06; data 54,1f; index 41; keycode 106
Apr  4 09:27:32 karl-desktop kernel: [346510.296264] ati_remote
1-1:1.0: channel 0x06; data 54,1f; index 41; keycode 106
Apr  4 09:27:32 karl-desktop kernel: [346510.336222] ati_remote
1-1:1.0: channel 0x06; data 54,1f; index 41; keycode 106
Apr  4 09:27:32 karl-desktop kernel: [346510.384175] ati_remote
1-1:1.0: channel 0x06; data 54,1f; index 41; keycode 106
Apr  4 09:27:32 karl-desktop kernel: [346510.424135] ati_remote
1-1:1.0: channel 0x06; data 54,1f; index 41; keycode 106
Apr  4 09:27:33 karl-desktop kernel: [346510.472085] ati_remote
1-1:1.0: channel 0x06; data 54,1f; index 41; keycode 106

That's .23 seconds between the first and 6th event.  A little quick :)
 I think the ideal fix would be a second timestamp that keeps track of
when the first event happened... not just the last event.  That way
you can set a real "don't repeat until after .5 seconds" option.







On 4/4/07, Vincent Vanackere <vincent.vanackere@gmail.com> wrote:
> On 4/4/07, Éric Piel <Eric.Piel@tremplin-utc.net> wrote:
>
> > This default value is set to 300 ms. On my Xserver, the default value is
> > 660 ms and by default in my distrib it's set to 500 ms. So, indeed, the
> > default value of the ati_remote is quite small. Maybe you could increase
> > FILTER_MAX to 10 (= 600 ms) in order to have something saner?
>
> Well, this value was chosen because it matches what the hardware does
> : as explained in the comments, the hardware generates exactly 5
> events for the first keypress (spaced by ~50ms if I recall correctly).
>
> At least on my hardware (=> Vendor=0bc7 ProdID=0004 Rev= 1.00), the
> current default value match exactly what I'm expecting (never had the
> problem : "if you do anything but barely tap the button you can get
> multiple events reported").
> Just a thought : perhaps some models are more sensitive than others
> (and so the default should be made dependent on the exact remote
> model)... what is your hardware id ?
>
> Best regards,
>
> Vincent
>


-- 
Karl Pickett
The most wasted of all days is one without laughter.—E.E. Cummings

Re: [PATCH] Make ati_remote button repeat sensitivity soft-configurable

[Karl Pickett]

Would this be more acceptable?  I haven't compiled or tested yet..but
you can see what I'm trying to do.. make the delay time based instead
of implementation/count based...

Signed-off-by: Karl Pickett <karl.pickett@gmail.com>

--- ./ati_remote.c.kjp	2007-04-04 10:56:20.000000000 -0400
+++ ./ati_remote.c	2007-04-04 11:13:02.000000000 -0400
@@ -120,6 +120,7 @@
  * behaviour.
  */
 #define FILTER_TIME     60 /* msec */
+#define REPEAT_DELAY    500 /* msec */

 static unsigned long channel_mask;
 module_param(channel_mask, ulong, 0644);
@@ -133,6 +134,11 @@
 module_param(repeat_filter, int, 0644);
 MODULE_PARM_DESC(repeat_filter, "Repeat filter time, default = 60 msec");

+static int repeat_delay = REPEAT_DELAY;
+module_param(repeat_delay, int, 0644);
+MODULE_PARM_DESC(repeat_delay, "Delay before sending repeats, "
+                                "default = " #REPEAT_DELAY " msec");
+
 #define dbginfo(dev, format, arg...) do { if (debug) dev_info(dev ,
format , ## arg); } while (0)
 #undef err
 #define err(format, arg...) printk(KERN_ERR format , ## arg)
@@ -175,6 +181,7 @@
         unsigned long old_jiffies;
         unsigned long acc_jiffies;  /* handle acceleration */
         unsigned int repeat_count;
+        unsigned long first_jiffies;

         char name[NAME_BUFSIZE];
         char phys[NAME_BUFSIZE];
@@ -501,21 +508,26 @@
         }

         if (ati_remote_tbl[index].kind == KIND_FILTERED) {
+                unsigned long now = jiffies;
+
                 /* Filter duplicate events which happen "too close"
together. */
                 if (ati_remote->old_data[0] == data[1] &&
                     ati_remote->old_data[1] == data[2] &&
-                    time_before(jiffies, ati_remote->old_jiffies +
msecs_to_jiffies(repeat_filter))) {
+                    time_before(now, ati_remote->old_jiffies +
+                            msecs_to_jiffies(repeat_filter))) {
                         ati_remote->repeat_count++;
                 } else {
                         ati_remote->repeat_count = 0;
+                        ati_remote->first_jiffies = now;
                 }

                 ati_remote->old_data[0] = data[1];
                 ati_remote->old_data[1] = data[2];
-                ati_remote->old_jiffies = jiffies;
+                ati_remote->old_jiffies = now;

                 if (ati_remote->repeat_count > 0 &&
-                    ati_remote->repeat_count < 5)
+                                time_before(now, ati_remote->first_jiffies +
+                                        msecs_to_jiffies(repeat_delay)))
                         return;


-- 
Karl Pickett
The most wasted of all days is one without laughter.—E.E. Cummings

Re: [PATCH] Make ati_remote button repeat sensitivity soft-configurable

[Vincent Vanackere]

[-- Attachment #1: Type: text/plain, Size: 647 bytes --]

On 4/4/07, Karl Pickett <karl.pickett@gmail.com> wrote:
> Would this be more acceptable?  I haven't compiled or tested yet..but
> you can see what I'm trying to do.. make the delay time based instead
> of implementation/count based...

Well, I was going to submit a similar patch so I'd say that's the
right approach ;-)
(I just think the check on repeat_count < 5 must be kept as is
prevents the user to get the spurious events even if repeat_delay is
set too low).

I'm attaching a very small adaptation of your patch (re-added the
repeat_count check and a small comment, compile & run-time tested).
Works fine for me...

Best regards,

Vincent

[-- Attachment #2: atiremote.diff --]
[-- Type: text/x-patch, Size: 2220 bytes --]

--- drivers/usb/input/ati_remote.c.orig	2007-04-04 22:05:10.000000000 +0200
+++ drivers/usb/input/ati_remote.c	2007-04-04 22:35:59.000000000 +0200
@@ -120,6 +120,7 @@
  * behaviour.
  */
 #define FILTER_TIME	60 /* msec */
+#define REPEAT_DELAY    500 /* msec */
 
 static unsigned long channel_mask;
 module_param(channel_mask, ulong, 0644);
@@ -133,6 +134,10 @@
 module_param(repeat_filter, int, 0644);
 MODULE_PARM_DESC(repeat_filter, "Repeat filter time, default = 60 msec");
 
+static int repeat_delay = REPEAT_DELAY;
+module_param(repeat_delay, int, 0644);
+MODULE_PARM_DESC(repeat_delay, "Delay before sending repeats, default = 500 msec");
+
 #define dbginfo(dev, format, arg...) do { if (debug) dev_info(dev , format , ## arg); } while (0)
 #undef err
 #define err(format, arg...) printk(KERN_ERR format , ## arg)
@@ -174,6 +179,8 @@
 	unsigned char old_data[2];  /* Detect duplicate events */
 	unsigned long old_jiffies;
 	unsigned long acc_jiffies;  /* handle acceleration */
+        unsigned long first_jiffies;
+
 	unsigned int repeat_count;
 
 	char name[NAME_BUFSIZE];
@@ -501,21 +508,31 @@
 	}
 
 	if (ati_remote_tbl[index].kind == KIND_FILTERED) {
+		unsigned long now = jiffies;
+
 		/* Filter duplicate events which happen "too close" together. */
 		if (ati_remote->old_data[0] == data[1] &&
 		    ati_remote->old_data[1] == data[2] &&
-		    time_before(jiffies, ati_remote->old_jiffies + msecs_to_jiffies(repeat_filter))) {
+		    time_before(now, ati_remote->old_jiffies + 
+				     msecs_to_jiffies(repeat_filter))) {
 			ati_remote->repeat_count++;
 		} else {
 			ati_remote->repeat_count = 0;
+			ati_remote->first_jiffies = now;
 		}
 
 		ati_remote->old_data[0] = data[1];
 		ati_remote->old_data[1] = data[2];
-		ati_remote->old_jiffies = jiffies;
+		ati_remote->old_jiffies = now;
 
+		/* Ensure we skip at least the 4 first duplicate events (generated
+		 * by a single keypress), and continue skipping until repeat_delay
+		 * msecs have passed 
+		 */
 		if (ati_remote->repeat_count > 0 &&
-		    ati_remote->repeat_count < 5)
+		    (ati_remote->repeat_count < 5 ||
+		     time_before(now, ati_remote->first_jiffies + 
+				      msecs_to_jiffies(repeat_delay))))
 			return;
 
 

Re: [PATCH] Make ati_remote button repeat sensitivity soft-configurable

[Éric Piel]

04/04/2007 10:52 PM, Vincent Vanackere wrote/a écrit:
:
> I'm attaching a very small adaptation of your patch (re-added the
> repeat_count check and a small comment, compile & run-time tested).
> Works fine for me...

> 
> --- drivers/usb/input/ati_remote.c.orig	2007-04-04 22:05:10.000000000 +0200
> +++ drivers/usb/input/ati_remote.c	2007-04-04 22:35:59.000000000 +0200
:
> @@ -174,6 +179,8 @@
>  	unsigned char old_data[2];  /* Detect duplicate events */
>  	unsigned long old_jiffies;
>  	unsigned long acc_jiffies;  /* handle acceleration */
> +        unsigned long first_jiffies;
    ^^^^^^^^ space warning!

Sorry for being annoying ;-)
Eric

Re: [PATCH] Make ati_remote button repeat sensitivity soft-configurable

[Vincent Vanackere]

[-- Attachment #1: Type: text/plain, Size: 318 bytes --]

On 4/4/07, Éric Piel <Eric.Piel@tremplin-utc.net> wrote:
> 04/04/2007 10:52 PM, Vincent Vanackere wrote/a écrit:
> > +        unsigned long first_jiffies;
>     ^^^^^^^^ space warning!
>
> Sorry for being annoying ;-)
> Eric
>

Indeed you are    8-)
... new version attached ...

Best regards,

Vincent

[-- Attachment #2: atiremote.diff --]
[-- Type: text/x-patch, Size: 2213 bytes --]

--- drivers/usb/input/ati_remote.c.orig	2007-04-04 22:05:10.000000000 +0200
+++ drivers/usb/input/ati_remote.c	2007-04-04 23:06:33.000000000 +0200
@@ -120,6 +120,7 @@
  * behaviour.
  */
 #define FILTER_TIME	60 /* msec */
+#define REPEAT_DELAY    500 /* msec */
 
 static unsigned long channel_mask;
 module_param(channel_mask, ulong, 0644);
@@ -133,6 +134,10 @@
 module_param(repeat_filter, int, 0644);
 MODULE_PARM_DESC(repeat_filter, "Repeat filter time, default = 60 msec");
 
+static int repeat_delay = REPEAT_DELAY;
+module_param(repeat_delay, int, 0644);
+MODULE_PARM_DESC(repeat_delay, "Delay before sending repeats, default = 500 msec");
+
 #define dbginfo(dev, format, arg...) do { if (debug) dev_info(dev , format , ## arg); } while (0)
 #undef err
 #define err(format, arg...) printk(KERN_ERR format , ## arg)
@@ -174,6 +179,8 @@
 	unsigned char old_data[2];  /* Detect duplicate events */
 	unsigned long old_jiffies;
 	unsigned long acc_jiffies;  /* handle acceleration */
+	unsigned long first_jiffies;
+
 	unsigned int repeat_count;
 
 	char name[NAME_BUFSIZE];
@@ -501,21 +508,31 @@
 	}
 
 	if (ati_remote_tbl[index].kind == KIND_FILTERED) {
+		unsigned long now = jiffies;
+
 		/* Filter duplicate events which happen "too close" together. */
 		if (ati_remote->old_data[0] == data[1] &&
 		    ati_remote->old_data[1] == data[2] &&
-		    time_before(jiffies, ati_remote->old_jiffies + msecs_to_jiffies(repeat_filter))) {
+		    time_before(now, ati_remote->old_jiffies + 
+				     msecs_to_jiffies(repeat_filter))) {
 			ati_remote->repeat_count++;
 		} else {
 			ati_remote->repeat_count = 0;
+			ati_remote->first_jiffies = now;
 		}
 
 		ati_remote->old_data[0] = data[1];
 		ati_remote->old_data[1] = data[2];
-		ati_remote->old_jiffies = jiffies;
+		ati_remote->old_jiffies = now;
 
+		/* Ensure we skip at least the 4 first duplicate events (generated
+		 * by a single keypress), and continue skipping until repeat_delay
+		 * msecs have passed 
+		 */
 		if (ati_remote->repeat_count > 0 &&
-		    ati_remote->repeat_count < 5)
+		    (ati_remote->repeat_count < 5 ||
+		     time_before(now, ati_remote->first_jiffies + 
+				      msecs_to_jiffies(repeat_delay))))
 			return;
 
 

Re: [PATCH] Make ati_remote button repeat sensitivity soft-configurable

[Karl Pickett]

On 4/4/07, Vincent Vanackere <vincent.vanackere@gmail.com> wrote:
> On 4/4/07, Éric Piel <Eric.Piel@tremplin-utc.net> wrote:
> > 04/04/2007 10:52 PM, Vincent Vanackere wrote/a écrit:
> > > +        unsigned long first_jiffies;
> >     ^^^^^^^^ space warning!
> >
> > Sorry for being annoying ;-)
> > Eric
> >
>
> Indeed you are    8-)
> ... new version attached ...
>
> Best regards,
>
> Vincent
>
>

Works great for me.  I can now give it a hard press and not cause a
repeat.  It's nice that you can  just set repeat_delay to 0 to get the
identical previous behavior, in case anybody was in love with that.

The spaces were probably my fault, I sent it from a machine with
expandtab on (don't ask :)



-- 
Karl Pickett
The most wasted of all days is one without laughter.—E.E. Cummings