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
--- 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; }
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.
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
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.
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.
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
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
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.
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
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 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.
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
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
Andreas Brugger wrote:
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?
Skip() sets the start day to the following day after StartTime(). Until now this was fine for VDR, because Skip() is only used for timers that have no start day set. For timers that have a start day, Skip() did weired things because of the sometimes wrong StartTime() for these timers.
The original patch did not attempt to fix Skip(), it just fixes the wrong calculation of StartTime(). Skip() could be fixed by this additional patch: (untested!) --- timers.c.orig 2006-05-20 18:50:49.000000000 +0200 +++ timers.c 2006-05-22 21:22:37.364563392 +0200 @@ -560,6 +560,7 @@ void cTimer::Skip(void) { day = IncDay(SetTime(StartTime(), 0), 1); + day = SetTime(StartTime(), 0); SetEvent(NULL); }
However, with the original patch you can be quite sure that after Skip() the StartTime() will match the day mask, even if the start day does not. (and you can always set the start day to a non-recording day manually)
A variant of the problematic loop in Enigma may be to Skip() until StartTime() instead of Day() matches the day mask - this should be instantly true with the patch, and will be true after some loops without the patch. (StartTime() however will then be falsely at midnight.) And add a failsafe that terminates after some 100 loops. ;)
Cheers,
Udo
Udo Richter wrote:
The original patch did not attempt to fix Skip(), it just fixes the wrong calculation of StartTime(). Skip() could be fixed by this additional patch: (untested!)
Well, that was really untested. This one *is* tested:
--- timers.c.orig 2006-05-20 18:50:49.000000000 +0200 +++ timers.c 2006-05-22 22:19:02.134688704 +0200 @@ -560,6 +560,9 @@ void cTimer::Skip(void) { day = IncDay(SetTime(StartTime(), 0), 1); + startTime = 0; + day = SetTime(StartTime(), 0); + startTime = 0; SetEvent(NULL); }
Cheers,
Udo
Udo Richter wrote:
Udo Richter wrote:
The original patch did not attempt to fix Skip(), it just fixes the wrong calculation of StartTime(). Skip() could be fixed by this additional patch: (untested!)
Well, that was really untested. This one *is* tested:
Thanks a lot for looking at this problem, Udo. So, is it my understanding that Enigma does not need to change if we use the original "starttime" patch you posted in conjunction with this other new one?
Best Regards.
C.Y.M wrote:
Thanks a lot for looking at this problem, Udo. So, is it my understanding that Enigma does not need to change if we use the original "starttime" patch you posted in conjunction with this other new one?
The two patches (my original and my last) fix minor issues with VDR. The bogus Enigma loop may terminate again with these two patches, but I wouldn't bet on it. You can try it if you want.
Rewriting the Enigma loop as I suggested would be a good idea anyway, to make it a bit more robust.
Cheers,
Udo
Udo Richter schrieb:
C.Y.M wrote:
Thanks a lot for looking at this problem, Udo. So, is it my understanding that Enigma does not need to change if we use the original "starttime" patch you posted in conjunction with this other new one?
The two patches (my original and my last) fix minor issues with VDR. The bogus Enigma loop may terminate again with these two patches, but I wouldn't bet on it. You can try it if you want.
Rewriting the Enigma loop as I suggested would be a good idea anyway, to make it a bit more robust.
C.Y.M. maybe you could test this fix and if it works I will include it into the next version of Enigma.
Udo, do you know any side-effect of scripts or something like that if the directory is renamed? I didn't test this patch due to some concerns that there maybe some unknown side-effects ...
Andreas Brugger wrote:
Udo, do you know any side-effect of scripts or something like that if the directory is renamed?
ummmm... what? Sorry, don't get it. My patches have nothing to do with file names or directory names. And if you're hunting Enigma bugs, sorry, my VDR doesn't have the OSD memory to run Enigma.
Cheers,
Udo
Udo Richter schrieb:
Andreas Brugger wrote:
Udo, do you know any side-effect of scripts or something like that if the directory is renamed?
ummmm... what? Sorry, don't get it. My patches have nothing to do with file names or directory names.
Ah sorry, that was an other patch that changed the directory-name.
And if you're hunting Enigma bugs, sorry, my VDR doesn't have the OSD memory to run Enigma.
BTW, you don't need a modded card to use Enigma. That is an old myth. ;-) The OSD-size is limited though. You have to use a max. height of 470 pixels or so.
Stone schrieb:
C.Y.M. maybe you could test this fix and if it works I will include it into the next version of Enigma.What is the fix? I see the section of the code that Udo is talking about, but I am not really sure how it is supposed to be.
Best Regards.
You should change a part of void cText2SkinStatus::UpdateEvents(void) in status.c so it looks like that:
if (!dummy.IsSingleEvent()) // add 4 additional rep. timer { int j = 0; do { j++; // just to avoid a endless loop dummy.Skip(); dummy.Matches(); // Refresh start- and end-time } while (!dummy.DayMatches(dummy.StartTime()) && (j < 7)); }
This works here. Also with the official release of Klaus.
Udo Richter wrote:
Udo Richter wrote:
The original patch did not attempt to fix Skip(), it just fixes the wrong calculation of StartTime(). Skip() could be fixed by this additional patch: (untested!)
Well, that was really untested. This one *is* tested:
--- timers.c.orig 2006-05-20 18:50:49.000000000 +0200 +++ timers.c 2006-05-22 22:19:02.134688704 +0200 @@ -560,6 +560,9 @@ void cTimer::Skip(void) { day = IncDay(SetTime(StartTime(), 0), 1);
- startTime = 0;
- day = SetTime(StartTime(), 0);
- startTime = 0; SetEvent(NULL);
}
I'm afraid this might have a nasty side effect. Imagine the following scenario:
- a weekly timer that records on Mondays at 20:00 - at Monday, 19:00 the user decides to skip today's recording, so he presses the red button on the timer - some time later he decides to make this timer also record on Tuesdays, so he edits the timer and sets the day flag for Tuesday by pressing '2'
The way it is now, the timer would record as the user expects it to. With your patch he would also have to manually set the "first day" correctly, otherwise the additional Tuesday recording wouldn't take place.
I think I prefer to leave cTimer::Skip() as it is, and only adopt your original patch.
Klaus
Klaus Schmidinger wrote:
Udo Richter wrote:
The original patch did not attempt to fix Skip(), it just fixes the wrong calculation of StartTime(). Skip() could be fixed by this additional patch: (untested!)
I'm afraid this might have a nasty side effect. Imagine the following scenario:
- a weekly timer that records on Mondays at 20:00
- at Monday, 19:00 the user decides to skip today's recording, so he presses the red button on the timer
- some time later he decides to make this timer also record on Tuesdays, so he edits the timer and sets the day flag for Tuesday by pressing '2'
I have no problem if Skip() continues to mean 'skip next recording' and sets day to one day later. With the fixed calculation of StartTime(), its not a big difference in user experience, and using Skip() to calculate future timers isn't the main goal here. (and can be done by either looping Skip() or smart use of Matches(t))
However, I would suggest to keep at least one startTime = 0; in Skip(), so the cached startTime is properly invalidated.
Cheers,
Udo
Udo Richter wrote:
Klaus Schmidinger wrote:
Udo Richter wrote:
The original patch did not attempt to fix Skip(), it just fixes the wrong calculation of StartTime(). Skip() could be fixed by this additional patch: (untested!)
I'm afraid this might have a nasty side effect. Imagine the following scenario:
- a weekly timer that records on Mondays at 20:00
- at Monday, 19:00 the user decides to skip today's recording, so he presses the red button on the timer
- some time later he decides to make this timer also record on Tuesdays, so he edits the timer and sets the day flag for Tuesday by pressing '2'
I have no problem if Skip() continues to mean 'skip next recording' and sets day to one day later. With the fixed calculation of StartTime(), its not a big difference in user experience, and using Skip() to calculate future timers isn't the main goal here. (and can be done by either looping Skip() or smart use of Matches(t))
However, I would suggest to keep at least one startTime = 0; in Skip(), so the cached startTime is properly invalidated.
Ok, that certainly makes sense.
Klaus
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
-
Andreas Brugger -
C.Y.M -
Christian Wieninger -
Klaus Schmidinger -
Stone -
Udo Richter