| Message ID | 20060206221652.GB3989@localhost.localdomain |
|---|---|
| State | New |
| Headers |
Received: from gw01.mail.saunalahti.fi ([195.197.172.115]) by www.linuxtv.org with esmtp (Exim 4.50) id 1F6EfH-00007B-3y for vdr@linuxtv.org; Mon, 06 Feb 2006 23:16:55 +0100 Received: from localhost (YZKCCXIX.dsl.saunalahti.fi [85.76.51.20]) by gw01.mail.saunalahti.fi (Postfix) with ESMTP id 77A271134C7; Tue, 7 Feb 2006 00:16:52 +0200 (EET) Date: Tue, 7 Feb 2006 00:16:52 +0200 From: Marko =?iso-8859-1?B?TeRrZWzk?= <marko.makela@hut.fi> To: VDR Mailing List <vdr@linuxtv.org> Subject: Re: [vdr] Key-repeat or release magic in RCU drivers Message-ID: <20060206221652.GB3989@localhost.localdomain> References: <43DCE06E.6010109@cadsoft.de> <4DF107C1C9%linux@youmustbejoking.demon.co.uk> <20060130085313.GC3583@localhost.localdomain> <200602011951.04437@orion.escape-edv.de> <20060201212815.GA3554@localhost.localdomain> <4DF28F9D66%linux@youmustbejoking.demon.co.uk> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="G4iJoqBmSsgzjUCe" Content-Disposition: inline In-Reply-To: <4DF28F9D66%linux@youmustbejoking.demon.co.uk> Organization: Helsinki University of Technology User-Agent: Mutt/1.5.11+cvs20060126 Content-Transfer-Encoding: 7bit X-BeenThere: vdr@linuxtv.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: VDR Mailing List <vdr@linuxtv.org> List-Id: VDR Mailing List <vdr.linuxtv.org> List-Unsubscribe: <http://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr>, <mailto:vdr-request@linuxtv.org?subject=unsubscribe> List-Archive: <http://www.linuxtv.org/pipermail/vdr> List-Post: <mailto:vdr@linuxtv.org> List-Help: <mailto:vdr-request@linuxtv.org?subject=help> List-Subscribe: <http://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr>, <mailto:vdr-request@linuxtv.org?subject=subscribe> X-List-Received-Date: Mon, 06 Feb 2006 22:16:55 -0000 Status: O X-Status: X-Keywords: X-UID: 7733 |
Commit Message
Marko Mäkelä
Feb. 6, 2006, 10:16 p.m. UTC
On Wed, Feb 01, 2006 at 09:56:40PM +0000, Darren Salt wrote: > I demand that Marko Mäkelä may or may not have written... > > > On Wed, Feb 01, 2006 at 07:51:04PM +0100, Oliver Endriss wrote: > [snip] > >> (1) The repeat timer of the input layer must be turned off, and the > >> natural repeat rate of the RC5 frames should be used. Anything else > >> will result in inaccurate operation. > > >> (2) The key-up timeout should be set to 280ms or higher (>= 2x repeat > >> rate). This will allow for one missing RC5 frame which may happen due > >> to transmission errors. > > > Thanks, this sounds exactly what I'm looking for! > > > The 280 ms you mention matches the #define UP_TIMEOUT (HZ*7/25) in > > /usr/src/linux-2.6.14.3/drivers/media/dvb/ttpci/av7110_ir.c on my vdr box. > > I'll see if I'm smart enough to port the code to the cx88 driver. > > linux/drivers/media/common/ir-functions.c would seem to be the right place > for that code... I guess you meant ir-common.c, after all. I made a quick&dirty patch to ir-common.c and cx88-input.c that maps each incoming RC5 frame to a key-press or a key-repeat event. The key-release timeout is set to 280 ms. I tried blocking the IR beam for a couple of seconds while holding the Volume+ button down. After I unblocked the beam, the repeat events would continue. Thanks to the patch, it is now possible to reach any line on long listings such as the EPG menu, by holding the Down button and releasing it when the selection mark reaches the line. The only drawback in my patch is that it is sometimes hard to press a repeating key (e.g., Channel+) quickly enough to only generate a key-press event without any key-repeat. After all, it might be good to discard the first key-repeat event. Do av7110 users have this problem? It'd be nice if this patch could be polished and submitted to the kernel. Are there any v4l-dvb developers on this list? Marko
Comments
Marko Mäkelä wrote: > On Wed, Feb 01, 2006 at 09:56:40PM +0000, Darren Salt wrote: > > I demand that Marko Mäkelä may or may not have written... > > > > > On Wed, Feb 01, 2006 at 07:51:04PM +0100, Oliver Endriss wrote: > > [snip] > > >> (1) The repeat timer of the input layer must be turned off, and the > > >> natural repeat rate of the RC5 frames should be used. Anything else > > >> will result in inaccurate operation. > > > > >> (2) The key-up timeout should be set to 280ms or higher (>= 2x repeat > > >> rate). This will allow for one missing RC5 frame which may happen due > > >> to transmission errors. > > > > > Thanks, this sounds exactly what I'm looking for! > > > > > The 280 ms you mention matches the #define UP_TIMEOUT (HZ*7/25) in > > > /usr/src/linux-2.6.14.3/drivers/media/dvb/ttpci/av7110_ir.c on my vdr box. > > > I'll see if I'm smart enough to port the code to the cx88 driver. > > > > linux/drivers/media/common/ir-functions.c would seem to be the right place > > for that code... > > I guess you meant ir-common.c, after all. > > I made a quick&dirty patch to ir-common.c and cx88-input.c that maps each > incoming RC5 frame to a key-press or a key-repeat event. The key-release > timeout is set to 280 ms. I tried blocking the IR beam for a couple of > seconds while holding the Volume+ button down. After I unblocked the beam, > the repeat events would continue. > > Thanks to the patch, it is now possible to reach any line on long listings > such as the EPG menu, by holding the Down button and releasing it when the > selection mark reaches the line. > > The only drawback in my patch is that it is sometimes hard to press a > repeating key (e.g., Channel+) quickly enough to only generate a key-press > event without any key-repeat. After all, it might be good to discard the > first key-repeat event. Do av7110 users have this problem? No, this problem is handled by input_repeat_key/delay_timer_finished magic. ;-) The driver discards all keypresses until the delay timer of the input layer has expired. Btw, this delay may be adjusted by user space tools. > It'd be nice if this patch could be polished and submitted to the kernel. > Are there any v4l-dvb developers on this list? Darren posted a patch on the v4l/dvb lists. Maybe you could join your efforts and submit a patch against the current hg repository. Oliver
I demand that Oliver Endriss may or may not have written... > Marko Mäkelä wrote: >> On Wed, Feb 01, 2006 at 09:56:40PM +0000, Darren Salt wrote: >>> I demand that Marko Mäkelä may or may not have written... >>>> On Wed, Feb 01, 2006 at 07:51:04PM +0100, Oliver Endriss wrote: >>> [snip] >>>>> (1) The repeat timer of the input layer must be turned off, and the >>>>> natural repeat rate of the RC5 frames should be used. Anything else >>>>> will result in inaccurate operation. >>>>> (2) The key-up timeout should be set to 280ms or higher (>= 2x repeat >>>>> rate). This will allow for one missing RC5 frame which may happen >>>>> due to transmission errors. >>>> Thanks, this sounds exactly what I'm looking for! >>>> The 280 ms you mention matches the #define UP_TIMEOUT (HZ*7/25) in >>>> /usr/src/linux-2.6.14.3/drivers/media/dvb/ttpci/av7110_ir.c on my vdr >>>> box. I'll see if I'm smart enough to port the code to the cx88 driver. >>> linux/drivers/media/common/ir-functions.c would seem to be the right >>> place for that code... >> I guess you meant ir-common.c, after all. Both, really: it was renamed fairly recently. >> I made a quick&dirty patch to ir-common.c and cx88-input.c that maps each >> incoming RC5 frame to a key-press or a key-repeat event. [...] I've altered ir-functions.c:ir_input_keydown() here to make this easier: there should no longer any need for other modules to alter ir->keypressed. If you have a new keypress, call ir_input_nokey() then ir_input_keydown(), else call ir_input_keydown() only and the patched code will notice that it's for the same key and cause a repeat event instead. [snip] >> The only drawback in my patch is that it is sometimes hard to press a >> repeating key (e.g., Channel+) quickly enough to only generate a key-press >> event without any key-repeat. After all, it might be good to discard the >> first key-repeat event. I have logic for that currently in budget-ci, but it really belongs in ir-common. >> Do av7110 users have this problem? > No, this problem is handled by input_repeat_key/delay_timer_finished magic. > ;-) > The driver discards all keypresses until the delay timer of the input layer > has expired. Btw, this delay may be adjusted by user space tools. Hmm. So the standard repeat code should be used everywhere...? [snip] >> It'd be nice if this patch could be polished and submitted to the kernel. >> Are there any v4l-dvb developers on this list? > Darren posted a patch on the v4l/dvb lists. Maybe you could join your > efforts and submit a patch against the current hg repository. FWIW, my current patch sets are linked from <URL:http://www.youmustbejoking.demon.co.uk/progs.linux.html#dvb> And we need to take this to the v4l list :-)
On Tue, Feb 07, 2006 at 01:17:56AM +0000, Darren Salt wrote: > >> I made a quick&dirty patch to ir-common.c and cx88-input.c that maps each > >> incoming RC5 frame to a key-press or a key-repeat event. [...] > > I've altered ir-functions.c:ir_input_keydown() here to make this easier: > there should no longer any need for other modules to alter ir->keypressed. If > you have a new keypress, call ir_input_nokey() then ir_input_keydown(), else > call ir_input_keydown() only and the patched code will notice that it's for > the same key and cause a repeat event instead. This sounds reasonable. However, how do you distinguish rapid keypresses from repeat? In RC5 code, there is a toggle bit that changes state at a new keypress. In other codes, the repeat code may differ from the code of the first keypress. I think that ir_input_keydown() or equivalent function should take the repeat flag as a parameter. The function should never convert a repeat code to a keypress or vice versa. Two examples: (1) If I press the '1' key two times very quickly, I want VDR to see '11'. In your scheme, I'm afraid it will produce '1' and '1'|kRepeat, and VDR will ignore the second keypress. (2) When I press the '1' button close to the maximum range of the remote (which is not much with the Hauppauge receiver that is driven out of spec), the driver should deliver the events 12022020 instead of 12012010, so that VDR will switch to channel 1 and not 111. > FWIW, my current patch sets are linked from > <URL:http://www.youmustbejoking.demon.co.uk/progs.linux.html#dvb> > > And we need to take this to the v4l list :-) Sure. Marko
I demand that Marko Mäkelä may or may not have written... > On Tue, Feb 07, 2006 at 01:17:56AM +0000, Darren Salt wrote: >>>> I made a quick&dirty patch to ir-common.c and cx88-input.c that maps >>>> each incoming RC5 frame to a key-press or a key-repeat event. [...] >> I've altered ir-functions.c:ir_input_keydown() here to make this easier: >> there should no longer any need for other modules to alter ir->keypressed. >> If you have a new keypress, call ir_input_nokey() then ir_input_keydown(), >> else call ir_input_keydown() only and the patched code will notice that >> it's for the same key and cause a repeat event instead. > This sounds reasonable. However, how do you distinguish rapid keypresses > from repeat? In RC5 code, there is a toggle bit that changes state at a > new keypress. In other codes, the repeat code may differ from the code of > the first keypress. There's a toggle bit: bit 5 of the second byte. > I think that ir_input_keydown() or equivalent function should take the > repeat flag as a parameter. No need. If you want a keypress not to be flagged as a repeat, call ir_input_nokey() first. For a key to be flagged as being repeated, don't call ir_input_nokey() between two successive calls to ir_input_keydown(). > The function should never convert a repeat code to a keypress or vice > versa. Two examples: > (1) If I press the '1' key two times very quickly, I want VDR to see '11'. > In your scheme, I'm afraid it will produce '1' and '1'|kRepeat, and VDR > will ignore the second keypress. That won't be a problem unless interrupts are being missed. AFAICT (printk), that isn't happening. > (2) When I press the '1' button close to the maximum range of the remote > (which is not much with the Hauppauge receiver that is driven out of spec), > the driver should deliver the events 12022020 instead of 12012010, Meaning what? > so that VDR will switch to channel 1 and not 111. Well... it'll generate a repeat for each received signal (assuming that the key is held down) and it may time out, causing the next received signal to be treated as a key-down event. With the /other/ scheme (using the input layer's own repeat handler), you will get repeats - and maybe a few more 1s (although I'm enforcing lower limits on the repeat/delay times, and the timeout is shorter than the repeat time). [snip]
On Tue, Feb 07, 2006 at 06:24:02PM +0000, Darren Salt wrote: > >> I've altered ir-functions.c:ir_input_keydown() here to make this easier: > >> there should no longer any need for other modules to alter ir->keypressed. > >> If you have a new keypress, call ir_input_nokey() then ir_input_keydown(), > >> else call ir_input_keydown() only and the patched code will notice that > >> it's for the same key and cause a repeat event instead. > > > This sounds reasonable. However, how do you distinguish rapid keypresses > > from repeat? In RC5 code, there is a toggle bit that changes state at a > > new keypress. In other codes, the repeat code may differ from the code of > > the first keypress. > > There's a toggle bit: bit 5 of the second byte. Yes, in the RC5 code there is, just like I wrote. > > The function should never convert a repeat code to a keypress or vice > > versa. Two examples: > > > (1) If I press the '1' key two times very quickly, I want VDR to see '11'. > > In your scheme, I'm afraid it will produce '1' and '1'|kRepeat, and VDR > > will ignore the second keypress. > > That won't be a problem unless interrupts are being missed. AFAICT (printk), > that isn't happening. The RC5 repeat rate is 113.8 ms. If I press the same button twice within that time frame, it does happen. I must admit that this seems to happen with my own patch as well. I'll have to add some debug code to display the RC5 frames to see whether the toggle bit really is toggling. > > (2) When I press the '1' button close to the maximum range of the remote > > (which is not much with the Hauppauge receiver that is driven out of spec), > > the driver should deliver the events 12022020 instead of 12012010, > > Meaning what? 0=key-release 1=key-press 2=key-repeat I see that I should have picked another button than '1', to avoid confusing it with the number of the key-press event. > > so that VDR will switch to channel 1 and not 111. > > Well... it'll generate a repeat for each received signal (assuming that the > key is held down) and it may time out, causing the next received signal to be > treated as a key-down event. With 240 ms key-release timeout, it will time out if two RC5 frames are lost. If you hold the RCU in the wrong angle or far away from the receiver and press the button longer to ensure that the event will be noticed, you will end up getting multiple keypress events instead of one keypress and multiple repeat events. > With the /other/ scheme (using the input layer's own repeat handler), you > will get repeats - and maybe a few more 1s (although I'm enforcing lower > limits on the repeat/delay times, and the timeout is shorter than the repeat > time). I tested patch 3326 of the hg/v4l-dvb tree with your patches dvb-ir-20060206.patch.tar.gz applied. I changed the key-release timeout in cx88-input.c from 120 to 240 ms. Few keys were recognized. Among the working keys were the number keys, although they were shifted by 1 (e.g., '6' would be reported as '7'). The input layer's own repeat handler was not disabled, that is, I think there were too many repeat events. I couldn't figure out how budget-ci.c disables the repeat timer. I was unable to use this driver, because one of the cx88 modules failed to load and the computer crashed after some time. Marko
diff -rup linux-2.6.15.2/drivers/media/common/ir-common.c linux-2.6.15.2-cx88_input/drivers/media/common/ir-common.c --- linux-2.6.15.2/drivers/media/common/ir-common.c 2006-01-31 08:25:07.000000000 +0200 +++ linux-2.6.15.2-cx88_input/drivers/media/common/ir-common.c 2006-02-06 23:46:12.000000000 +0200 @@ -298,7 +298,7 @@ static void ir_input_key_event(struct in } dprintk(1,"%s: key event code=%d down=%d\n", dev->name,ir->keycode,ir->keypressed); - input_report_key(dev,ir->keycode,ir->keypressed); + input_event(dev, EV_KEY, ir->keycode, ir->keypressed); input_sync(dev); } @@ -343,13 +343,12 @@ void ir_input_keydown(struct input_dev * ir->keypressed = 0; ir_input_key_event(dev,ir); } - if (!ir->keypressed) { - ir->ir_key = ir_key; - ir->ir_raw = ir_raw; - ir->keycode = keycode; - ir->keypressed = 1; - ir_input_key_event(dev,ir); - } + /* same RC5 code -> it is a repeated key */ + ir->keypressed = ir->ir_raw == ir_raw ? 2 : 1; + ir->ir_key = ir_key; + ir->ir_raw = ir_raw; + ir->keycode = keycode; + ir_input_key_event(dev,ir); } /* -------------------------------------------------------------------------- */ diff -rup linux-2.6.15.2/drivers/media/video/cx88/cx88-input.c linux-2.6.15.2-cx88_input/drivers/media/video/cx88/cx88-input.c --- linux-2.6.15.2/drivers/media/video/cx88/cx88-input.c 2006-01-31 08:25:07.000000000 +0200 +++ linux-2.6.15.2-cx88_input/drivers/media/video/cx88/cx88-input.c 2006-02-06 23:04:39.000000000 +0200 @@ -289,6 +289,11 @@ MODULE_PARM_DESC(ir_debug, "enable debug #define ir_dprintk(fmt, arg...) if (ir_debug) \ printk(KERN_DEBUG "%s IR: " fmt , ir->core->name , ##arg) +static void input_repeat_key(unsigned long data) +{ + /* dummy routine to disable autorepeat in the input driver */ +} + /* ---------------------------------------------------------------------- */ static void cx88_ir_handle_key(struct cx88_IR *ir) @@ -472,6 +477,7 @@ int cx88_ir_init(struct cx88_core *core, /* all done */ input_register_device(ir->input); + ir->input->timer.function = input_repeat_key; return 0; } @@ -572,7 +578,7 @@ void cx88_ir_irq(struct cx88_core *core) if ((ircode & 0xfffff000) != 0x3000) break; ir_input_keydown(ir->input, &ir->ir, ircode & 0x3f, ircode); - ir->release = jiffies + msecs_to_jiffies(120); + ir->release = jiffies + msecs_to_jiffies(280); break; }