Fix calculating starttime of repeated timers with start date

Message ID 44705D02.7020505@gmx.de
State New
Headers

Commit Message

Udo Richter May 21, 2006, 12:28 p.m. UTC
  Hi list,

Referring to 
http://www.vdr-portal.de/board/thread.php?postid=468928#post468928 , 
here's a patch that fixes calculation of startTime in case that they
(1) are weekly timers
(2) have a start day set
(3) have next scheduled recording more than 7 days in the future

In this case, the timer will be sorted in the timer list at the start 
day, even if the start day doesn't match the weekday mask. (this can be 
down to two days from now for a timer that will record in 8 days)

Since the fix is at a rather sensitive code piece, it could use some 
review and testing. The change only affects repeated timers with start 
date set.

The fix does two things: First, the scan loop for matching timer events 
is shifted from now (or whatever t is) to the scheduled start day. That 
way the loop should always find a match.

Second, the startTime safety catchup is changed to a time that is really 
one week in the future - 'day' may be earlier. However, I don't think 
that this catch is triggered at all any more.

Cheers,

Udo
  

Comments

C.Y.M May 21, 2006, 1:39 p.m. UTC | #1
Udo Richter wrote:
> Hi list,
> 
> Referring to
> http://www.vdr-portal.de/board/thread.php?postid=468928#post468928 ,
> here's a patch that fixes calculation of startTime in case that they
> (1) are weekly timers
> (2) have a start day set
> (3) have next scheduled recording more than 7 days in the future
> 

With this patch applied, If I manually create a single event timer from the
schedule, then goto the timer menu and edit the timer to reoccur on a specific
weekday (by pressing the "0" key), VDR locks up and crashes when I goto modify
the timer.


BR.
  
Udo Richter May 21, 2006, 2:37 p.m. UTC | #2
C.Y.M wrote:
> With this patch applied, If I manually create a single event timer from the
> schedule, then goto the timer menu and edit the timer to reoccur on a specific
> weekday (by pressing the "0" key), VDR locks up and crashes when I goto modify
> the timer.

Sorry, cant reproduce this. I've set several timers, edited them on 
several ways, switched to repeated, repeated with start date and back, 
but no crashes so far.

When exactly does it crash? On leaving the timer edit dialog with ok?

When changing the timer to a repeating timer, do you set a start date?

If you use a skin: Does the crash happen with default skin?

Can you provide a backtrace?

Cheers,

Udo
  
C.Y.M May 21, 2006, 2:54 p.m. UTC | #3
Udo Richter wrote:
> C.Y.M wrote:
>> With this patch applied, If I manually create a single event timer
>> from the
>> schedule, then goto the timer menu and edit the timer to reoccur on a
>> specific
>> weekday (by pressing the "0" key), VDR locks up and crashes when I
>> goto modify
>> the timer.
> 
> Sorry, cant reproduce this. I've set several timers, edited them on
> several ways, switched to repeated, repeated with start date and back,
> but no crashes so far.
> 
> When exactly does it crash? On leaving the timer edit dialog with ok?

When I change the single event timer to a repeating timer and press OK, it goes
back to the timers menu.  From there, the OSD just locks up and doesnt respond
any more.
> 
> When changing the timer to a repeating timer, do you set a start date?

The original timer that I created is just a single event timer.
> 
> If you use a skin: Does the crash happen with default skin?

Yes, I'm using Enigma.  The skin doesnt seem to make any difference though.
> 
> Can you provide a backtrace?

I would provide a backtrace, but vdr is not actually segfaulting. VDR just locks
up and doesn't respond until I restart it.
> 

BR.
  
C.Y.M May 21, 2006, 3:31 p.m. UTC | #4
>> When exactly does it crash? On leaving the timer edit dialog with ok?
> 
> When I change the single event timer to a repeating timer and press OK, it goes
> back to the timers menu.  From there, the OSD just locks up and doesnt respond
> any more.
>> When changing the timer to a repeating timer, do you set a start date?
> 
> The original timer that I created is just a single event timer.
>> If you use a skin: Does the crash happen with default skin?
> 
> Yes, I'm using Enigma.  The skin doesnt seem to make any difference though.
>> Can you provide a backtrace?
> 
> I would provide a backtrace, but vdr is not actually segfaulting. VDR just locks
> up and doesn't respond until I restart it.
> 

I wonder if the timer conflict checking built into the epgsearch plugin is what
is causing it to lock up with this change in VDR. I will need to test more but I
dont really have much time at the moment.

Best Regards.
  
Udo Richter May 21, 2006, 3:53 p.m. UTC | #5
C.Y.M wrote:
> When I change the single event timer to a repeating timer and press OK, it goes
> back to the timers menu.  From there, the OSD just locks up and doesnt respond
> any more.

What really confuses me about this is that the patch behaves exactly as 
before unless a start day is set. And since you don't set a start day, 
what triggers this?

> I wonder if the timer conflict checking built into the epgsearch plugin is what
> is causing it to lock up with this change in VDR. 

I also have epgsearch running, so it should strike for me too. Plus, I 
don't think that epgsearch is triggered directly on timer change.

Cheers,

Udo
  
Klaus Schmidinger May 21, 2006, 4 p.m. UTC | #6
Udo Richter wrote:
> C.Y.M wrote:
>> When I change the single event timer to a repeating timer and press 
>> OK, it goes
>> back to the timers menu.  From there, the OSD just locks up and doesnt 
>> respond
>> any more.
> 
> What really confuses me about this is that the patch behaves exactly as 
> before unless a start day is set. And since you don't set a start day, 
> what triggers this?
> 
>> I wonder if the timer conflict checking built into the epgsearch 
>> plugin is what
>> is causing it to lock up with this change in VDR. 
> 
> I also have epgsearch running, so it should strike for me too. Plus, I 
> don't think that epgsearch is triggered directly on timer change.

Just my 2ct: I think your fix is ok and solves the problem very well.
Can't see why VDR would choke on it.

Klaus
  
C.Y.M May 21, 2006, 4:05 p.m. UTC | #7
Udo Richter wrote:
> C.Y.M wrote:
>> When I change the single event timer to a repeating timer and press
>> OK, it goes
>> back to the timers menu.  From there, the OSD just locks up and doesnt
>> respond
>> any more.
> 
> What really confuses me about this is that the patch behaves exactly as
> before unless a start day is set. And since you don't set a start day,
> what triggers this?


Isn't a "start day" set when a single event timer is created? The original timer
that is created is just set to a single day event. Then I edit that timer to
recur every Monday. After I accept the timer modification to recur every Monday,
thats when VDR freezes. Hm, I wonder what is different.

Best Regards.
  
Christian Wieninger May 21, 2006, 6 p.m. UTC | #8
Am Sonntag, 21. Mai 2006 17:53 schrieb Udo Richter:
> I also have epgsearch running, so it should strike for me too. Plus, I
> don't think that epgsearch is triggered directly on timer change.

yes, the search timer update is only triggered within the time intervall 
specified by setup or when manually starting it from within the plugin. 
BTW: til now epgsearch checks for timer conflicts using the timeline plugin. 
The next release will probably have its own timer conflict check.

BR,

Christian
  
Udo Richter May 21, 2006, 7:19 p.m. UTC | #9
C.Y.M wrote:
> Isn't a "start day" set when a single event timer is created? The original timer
> that is created is just set to a single day event. Then I edit that timer to
> recur every Monday. 

When the timer changes to repeated, the start day (in 
seconds-since-epoch) is set to 0 at the same moment. A good hint: 
Repeated timers with start day != 0 will have a '!' in front of the 
timer. This would also indicate weired things stored into that field.

The edit dialog operates on a copy, and the copy overwrites the actual 
timer in one big copy operation.

> After I accept the timer modification to recur every Monday,
> thats when VDR freezes. Hm, I wonder what is different.

One thing you can try: When VDR freezes, connect gdb to it with 
--pid=xxx and do the usual thread apply all bt. That way we should at 
least see where it is frozen.

Cheers,

Udo
  
C.Y.M May 22, 2006, 2:33 a.m. UTC | #10
Udo Richter wrote:
> C.Y.M wrote:
>> Isn't a "start day" set when a single event timer is created? The
>> original timer
>> that is created is just set to a single day event. Then I edit that
>> timer to
>> recur every Monday. 
> 
> When the timer changes to repeated, the start day (in
> seconds-since-epoch) is set to 0 at the same moment. A good hint:
> Repeated timers with start day != 0 will have a '!' in front of the
> timer. This would also indicate weired things stored into that field.
> 
> The edit dialog operates on a copy, and the copy overwrites the actual
> timer in one big copy operation.
> 
>> After I accept the timer modification to recur every Monday,
>> thats when VDR freezes. Hm, I wonder what is different.
> 
> One thing you can try: When VDR freezes, connect gdb to it with
> --pid=xxx and do the usual thread apply all bt. That way we should at
> least see where it is frozen.
> 

Udo, I was wrong about the skin not making a difference.  When I am in Enigma,
thats when it crashes.  If I just use the default STNG skin, its fine.  There is
some schedule checking in the Enigma skin, I'm sure thats what is causing it.
It crashes when I go into the main menu (I think editing the timer was just a
coincidence).  So, it appears that Enigma needs the fix.

Best Regards.
  
C.Y.M May 22, 2006, 2:58 a.m. UTC | #11
C.Y.M wrote:
> Udo Richter wrote:
>> C.Y.M wrote:
>>> Isn't a "start day" set when a single event timer is created? The
>>> original timer
>>> that is created is just set to a single day event. Then I edit that
>>> timer to
>>> recur every Monday. 
>> When the timer changes to repeated, the start day (in
>> seconds-since-epoch) is set to 0 at the same moment. A good hint:
>> Repeated timers with start day != 0 will have a '!' in front of the
>> timer. This would also indicate weired things stored into that field.
>>
>> The edit dialog operates on a copy, and the copy overwrites the actual
>> timer in one big copy operation.
>>
>>> After I accept the timer modification to recur every Monday,
>>> thats when VDR freezes. Hm, I wonder what is different.
>> One thing you can try: When VDR freezes, connect gdb to it with
>> --pid=xxx and do the usual thread apply all bt. That way we should at
>> least see where it is frozen.
>>
> 
> Udo, I was wrong about the skin not making a difference.  When I am in Enigma,
> thats when it crashes.  If I just use the default STNG skin, its fine.  There is
> some schedule checking in the Enigma skin, I'm sure thats what is causing it.
> It crashes when I go into the main menu (I think editing the timer was just a
> coincidence).  So, it appears that Enigma needs the fix.
> 

The problem I am experiencing with this starttime patch has to do with a new
function in the text2skin plugin that was introduced by Enigma.

struct tEvent : public cListObject

This is added to text2skin by the following patch: text2skin_extensions-0.8.diff

Enigma will list the upcoming timers in the main menu and thats what sends vdr
into a lockup.

Best Regards.
  
Udo Richter May 22, 2006, 5:05 p.m. UTC | #12
C.Y.M wrote:
> The problem I am experiencing with this starttime patch has to do with a new
> function in the text2skin plugin that was introduced by Enigma.
> 
> Enigma will list the upcoming timers in the main menu and thats what sends vdr
> into a lockup.

I *think* I found the reason in this code piece from status.c, 
cText2SkinStatus::UpdateEvents():

if (!dummy.IsSingleEvent()) // add 4 additional rep. timer
{
     do
     {
         dummy.Skip();
         dummy.Matches(); // Refresh start- and end-time
     } while (!dummy.DayMatches(dummy.Day()));
}

With my patch, StartTime() no longer points to days that are no 
recording days, and as a consequence, Skip() will correctly skip one 
week forward each time, to the day after the next recording. The above 
loop seems to expect Skip() to jump in 1-day steps and expects the timer 
to match the day mask finally. Since Skip() jumps to the day after the 
next timer, the day mask never matches.

The loop can now probably be avoided completely, since Skip() and 
StartTime() will deliver all future timers directly.

Cheers,

Udo
  
Andreas Brugger May 22, 2006, 6:41 p.m. UTC | #13
Udo Richter schrieb:

> C.Y.M wrote:
>
>> The problem I am experiencing with this starttime patch has to do 
>> with a new
>> function in the text2skin plugin that was introduced by Enigma.
>>
>> Enigma will list the upcoming timers in the main menu and thats what 
>> sends vdr
>> into a lockup.
>
>
> I *think* I found the reason in this code piece from status.c, 
> cText2SkinStatus::UpdateEvents():
>
> if (!dummy.IsSingleEvent()) // add 4 additional rep. timer
> {
>     do
>     {
>         dummy.Skip();
>         dummy.Matches(); // Refresh start- and end-time
>     } while (!dummy.DayMatches(dummy.Day()));
> }
>
> With my patch, StartTime() no longer points to days that are no 
> recording days, and as a consequence, Skip() will correctly skip one 
> week forward each time, to the day after the next recording. The above 
> loop seems to expect Skip() to jump in 1-day steps and expects the 
> timer to match the day mask finally. Since Skip() jumps to the day 
> after the next timer, the day mask never matches.
>
> The loop can now probably be avoided completely, since Skip() and 
> StartTime() will deliver all future timers directly.

Shouldn't Skip() actually skip to the next day, where the timer should 
record and not to the day after?
I know the VDR's Skip() does not, but I think a "corrected" version 
should or am I missing something here?

Bye,
Andreas Brugger
  

Patch

--- vdr-1.4.0-orig/timers.c	2006-05-20 18:50:49.000000000 +0200
+++ vdr-1.4.0/timers.c	2006-05-20 18:50:54.000000000 +0200
@@ -347,7 +347,7 @@ 
      }
   else {
      for (int i = -1; i <= 7; i++) {
-         time_t t0 = IncDay(t, i);
+         time_t t0 = IncDay(day ? max(day, t) : t, i);
          if (DayMatches(t0)) {
             time_t a = SetTime(t0, begin);
             time_t b = a + length;
@@ -359,7 +359,7 @@ 
             }
          }
      if (!startTime)
-        startTime = day; // just to have something that's more than a week in the future
+        startTime = IncDay(t, 7); // just to have something that's more than a week in the future
      else if (!Directly && (t > startTime || t > day + SECSINDAY + 3600)) // +3600 in case of DST change
         day = 0;
      }