[1.3.40] Fast channel switching resets

Message ID 43DCB8B2.10104@cadsoft.de
State New
Headers

Commit Message

Klaus Schmidinger Jan. 29, 2006, 12:44 p.m. UTC
  Luca Olivetti wrote:
> En/na Luca Olivetti ha escrit:
> 
> 
>>>Ok, I see in lirc.c that the REPEATDELAY is 350 (used not only to start
>>>the repeat but also to detect that there's no key pressed to generate a
>>>release), changing it to 150 and using a delay of 200 in interface.c
>>>seems to solve it (but still I'm not sure it is completely reliable).
>>
>>It's not reliable.
>>Maybe, instead of just raising the delay in interface (which kinda
>>defeats the purpose of not waiting) cDisplayChannel should remember that
>>a key repeat is in effect and just ignore kNone until a key release or a
>>timeout.
> 
> 
> I just tried to do that with a quick and dirty hack (in order to avoid
> recompiling everything and plugins, instead of adding a boolean member I
> "overloaded" number) and it seems to work fine (with the original
> timeouts in lirc and interface).

Based on your suggestion I have implemented a repeat timeout in cRemote,
so it should work with any kind of remote control, and for all kinds
of menus.

Please try the attached patch (against VD 1.3.40) and run it with
the original interface.c.

Originally I didn't want to release a new version of VDR today,
but since this repeat thing is rather problematic, and there is
also a serious problem with the version of EPG events after a
restart, I'm thinking of making a new release later today. So
if somebody could confirm that this patch actually works, that
would be nice.

Klaus
  

Comments

Joachim Wilke Jan. 29, 2006, 1:13 p.m. UTC | #1
2006/1/29, Klaus Schmidinger <Klaus.Schmidinger@cadsoft.de>:
> Originally I didn't want to release a new version of VDR today,
> but since this repeat thing is rather problematic, and there is
> also a serious problem with the version of EPG events after a
> restart, I'm thinking of making a new release later today. So
> if somebody could confirm that this patch actually works, that
> would be nice.

I tried it with success. When pressing the up-key on channel 1, VDR
switches to channel 2, keeps showing this program while counting up
the channel numbers. When releasing the up-key, VDR switches to the
final channel.

Regards,
Joachim.
  
Klaus Schmidinger Jan. 29, 2006, 1:15 p.m. UTC | #2
Joachim Wilke wrote:
> 2006/1/29, Klaus Schmidinger <Klaus.Schmidinger@cadsoft.de>:
> 
>>Originally I didn't want to release a new version of VDR today,
>>but since this repeat thing is rather problematic, and there is
>>also a serious problem with the version of EPG events after a
>>restart, I'm thinking of making a new release later today. So
>>if somebody could confirm that this patch actually works, that
>>would be nice.
> 
> 
> I tried it with success. When pressing the up-key on channel 1, VDR
> switches to channel 2, keeps showing this program while counting up
> the channel numbers. When releasing the up-key, VDR switches to the
> final channel.

Ok, that's how it's supposed to work.

The first channel switch can't be suppressed, because at that
time it's not yet known that there will be repeated key presses.

Klaus
  
Luca Olivetti Jan. 29, 2006, 2:21 p.m. UTC | #3
En/na Klaus Schmidinger ha escrit:

> 
> Originally I didn't want to release a new version of VDR today,
> but since this repeat thing is rather problematic, and there is
> also a serious problem with the version of EPG events after a
> restart, I'm thinking of making a new release later today. So
> if somebody could confirm that this patch actually works, that
> would be nice.

I'm afraid it's not reliable enough with the default REPEATDELAY of 350
in lirc.c (sometimes cDIsplayChannels still gets a kNone before the
k_Release).
Changing REPEATDELAY to 250 seems to work better, but I don't think is
the definitive solution.

Bye
  
Klaus Schmidinger Jan. 29, 2006, 2:28 p.m. UTC | #4
Luca Olivetti wrote:
> En/na Klaus Schmidinger ha escrit:
> 
> 
>>Originally I didn't want to release a new version of VDR today,
>>but since this repeat thing is rather problematic, and there is
>>also a serious problem with the version of EPG events after a
>>restart, I'm thinking of making a new release later today. So
>>if somebody could confirm that this patch actually works, that
>>would be nice.
> 
> 
> I'm afraid it's not reliable enough with the default REPEATDELAY of 350
> in lirc.c (sometimes cDIsplayChannels still gets a kNone before the
> k_Release).
> Changing REPEATDELAY to 250 seems to work better, but I don't think is
> the definitive solution.

REPEATDELAY is the time from the first keypress until the repeat
function actually kicks in. I'm afraid I don't see how this would
have an effect here.

Ok, if the events from LIRC are actually more than 250ms apart,
then skipping one could result in the repeatDelay timeout to
expire. So maybe you should rather try increasing REPEATTIMEOUT
in remote.c. I'd say a value of 1000 should really be enough for any
remote control (after all, a repeat function is supposed to be _fast_ ;-).

Klaus
  
Luca Olivetti Jan. 29, 2006, 2:44 p.m. UTC | #5
En/na Klaus Schmidinger ha escrit:

> REPEATDELAY is the time from the first keypress until the repeat
> function actually kicks in. I'm afraid I don't see how this would
> have an effect here.

It's also used as a timeout to detect the release:
           else {
              printf("%d\n",LastTime.Elapsed());
              if (FirstTime.Elapsed() < REPEATDELAY)
                 continue; // repeat function kicks in after a short delay
              repeat = true;
              timeout = REPEATDELAY; <<<<<<<
              }

Bye
  
Luca Olivetti Jan. 29, 2006, 2:50 p.m. UTC | #6
En/na Luca Olivetti ha escrit:
> En/na Klaus Schmidinger ha escrit:
> 
>> REPEATDELAY is the time from the first keypress until the repeat
>> function actually kicks in. I'm afraid I don't see how this would
>> have an effect here.
> 
> It's also used as a timeout to detect the release:
>            else {
>               printf("%d\n",LastTime.Elapsed());
>               if (FirstTime.Elapsed() < REPEATDELAY)
>                  continue; // repeat function kicks in after a short delay
>               repeat = true;
>               timeout = REPEATDELAY; <<<<<<<

note that I added the printf here to see the time between repeats
(that's around 110ms with my remote and lirc version)

Bye
  
Klaus Schmidinger Jan. 29, 2006, 3:04 p.m. UTC | #7
Luca Olivetti wrote:
> En/na Luca Olivetti ha escrit:
> 
>>En/na Klaus Schmidinger ha escrit:
>>
>>
>>>REPEATDELAY is the time from the first keypress until the repeat
>>>function actually kicks in. I'm afraid I don't see how this would
>>>have an effect here.
>>
>>It's also used as a timeout to detect the release:
>>           else {
>>              printf("%d\n",LastTime.Elapsed());
>>              if (FirstTime.Elapsed() < REPEATDELAY)
>>                 continue; // repeat function kicks in after a short delay
>>              repeat = true;
>>              timeout = REPEATDELAY; <<<<<<<
> 
> 
> note that I added the printf here to see the time between repeats
> (that's around 110ms with my remote and lirc version)

So what about setting REPEATTIMEOUT in remote.c to 1000?
Does that help?

Klaus
  
Luca Olivetti Jan. 29, 2006, 3:19 p.m. UTC | #8
En/na Klaus Schmidinger ha escrit:
ded the printf here to see the time between repeats
>> (that's around 110ms with my remote and lirc version)
> 
> So what about setting REPEATTIMEOUT in remote.c to 1000?
> Does that help?

I think that REPEATTIMEOUT should be at least double than REPEATDELAY
(in lirc, I don't know other remotes).
So I tried with the original REPEATDELAY 350 and REPEATTIMEOUT 700 as
well as REPEATDELAY 250 and REPEATTIMEOUT 500. Both combinations work,
so I'd say that a REPEATTIMEOUT of 1000 should definitely work (unless
there's some remote that takes more than 500 ms to detect a release
after a repeat).

Bye
  

Patch

--- remote.h	2006/01/01 14:00:50	1.32
+++ remote.h	2006/01/29 12:27:08
@@ -23,6 +23,7 @@ 
   static eKeys keys[MaxKeys];
   static int in;
   static int out;
+  static cTimeMs repeatTimeout;
   static cRemote *learning;
   static char *unknownCode;
   static cMutex mutex;
--- remote.c	2006/01/15 15:17:40	1.48
+++ remote.c	2006/01/29 12:28:21
@@ -19,10 +19,12 @@ 
 // --- cRemote ---------------------------------------------------------------
 
 #define INITTIMEOUT 10000 // ms
+#define REPEATTIMEOUT 500 // ms
 
 eKeys cRemote::keys[MaxKeys];
 int cRemote::in = 0;
 int cRemote::out = 0;
+cTimeMs cRemote::repeatTimeout;
 cRemote *cRemote::learning = NULL;
 char *cRemote::unknownCode = NULL;
 cMutex cRemote::mutex;
@@ -163,9 +165,11 @@ 
          eKeys k = keys[out];
          if (++out >= MaxKeys)
             out = 0;
+         if ((k & k_Repeat) != 0)
+            repeatTimeout.Set(REPEATTIMEOUT);
          return k;
          }
-      else if (!WaitMs || !keyPressed.TimedWait(mutex, WaitMs)) {
+      else if (!WaitMs || !keyPressed.TimedWait(mutex, WaitMs) && repeatTimeout.TimedOut()) {
          if (learning && UnknownCode) {
             *UnknownCode = unknownCode;
             unknownCode = NULL;