cTimer::StartTime() gibt inkonsistente Zeiten zurück.

kls · July 5, 2025 at 5:04 PM New

Quote from MarkusE

Also wenn ein Plugin einen wiederholenden Timer, bei dem der erste Tag der Aufzeichnung am 1.8.2025 sein soll, mit Matches(2027) aufruft, verändert das den Timer und die erste Aufzeichnung ist schon am ersten passenden Wochentag, und nicht erst am 1.8.2025.

Aufrufe von Matches() mit einer Zeit in der Zukunft sind künftig nicht mehr zulässig. Es wird eine Fehlermeldung geben und day wird nicht verändert.

Anbei mein aktueller Stand der Änderungen (ersetzt den Diff aus #37).
Die Version -03-w ist ohne Whitespace-Unterschiede, damit man die eigentlichen Änderungen besser sehen kann. Zum Patchen bitte die Version ohne "-w" verwenden.

Plugins sollten ohne Änderung weiter funktionieren wie bisher, allerdings kann es "deprecated"-Meldungen beim Übersetzen geben (je nachdem, welche Funktionen verwendet werden) und Log-Meldungen zur Laufzeit, über die man Stellen finden kann, die geändert werden sollten. Es wäre nett, wenn ihr diesen Patch mit möglichst vielen Plugins übersetzen würdet, um zu sehen, ob es irgendwo nicht klappt.

Der voraussichtliche HISTORY-Eintrag:

Code

- Reworked cTimer::Matches():
  + Calculating the raw start/stop time of a timer has been moved to the new function
    cTimer::CalcStartStopTime(). If a plugin calls cTimer::Matches() with Directly==true,
    it should use cTimer::CalcStartStopTime() instead. If it calls cTimer::Matches() with
    Directly==false, it should call cTimer::Matches(time_t t, int Margin) instead.
  + The versions of cTimer::Matches() with 'bool Directly' are deprecated. Existing calls
    with Directly==true are redirected to cTimer::CalcStartStopTime(), and a log message
    is issued.
  + cTimer::Matches() now reports an error if it is called with a time parameter that is
    not the current time.
  + The "first day" parameter of a repeating timer is now only reset if cTimer::Matches()
    is called with t==0 and Margin==0 and it has been exceeded.
  + Access to the cached cTimer::start-/stopTime members is now protected via a mutex.

Display More

WARNUNG: Diesen Patch bitte nur unter kontrollierten Bedingungen benutzen! Nicht für den produktiven Einsatz verwenden!

MarkusE #40 werde ich gesondert behandeln.

MarkusE · July 6, 2025 at 10:11 AM New

Ich habe mir mal die Verwendungen von StartTime() in VDR angeschaut.

Ich denke, wir brauchen eine neue Methode VpsTime():

Code

time_t cTimer::VpsTime(void) const {
// this timer records all VPS events where event-Vps() == VpsTime()
// for VPS timers, use this in the timer detail view UI (user needs to know the VPS time)

  if (HasFlags(tfVps)) {
     time_t startTime, stopTime;
     CalcStartStopTime(startTime, stopTime);
     return startTime;
  } else {
    return 0;
  }
}

Display More

Und die Methode StartTime() würde ich so schreiben:

Code

time_t cTimer::StartTime(void) const {
// At this time VDR needs a device for this timer
// Use cases: should we shutdown VDR, or is the StartTime() of the next timer to near?
//            wake up VDR early enough before StartTime() of next timer
//            is there a timer conflict?
//            ...

  if (HasFlags(tfVps) && event && event->Vps()) {
    return event->StartTime() - Setup.VpsMargin;
  } else {
     time_t startTime, stopTime;
     CalcStartStopTime(startTime, stopTime);
     return startTime;
  }
}

Display More

cocobrother · July 6, 2025 at 11:35 AM New

Klingt für mich gut.

kls · July 6, 2025 at 3:51 PM New

StartTime() liefert die Zeit, wann der Timer startet. Das ist entweder die vom Benutzer eingegebene Zeit (für normale Timer, hier ist sie auch exakt), oder die Startzeit des dem Timer zugeordneten Events (für VPS-Timer, hier hängt sie davon ab, wie sorgfältig der Sender das handhabt). Hier Setup.VpsMargin mit ins Spiel zu bringen wäre nicht richtig, denn das wird an der Aufrufstelle nicht erwartet.

Ausserdem werden die Zeiten in den startTime/stopTime Members gecached, weil sie z.B. beim Sortieren der Timer oft hintereinander gebraucht werden, sich aber normalerweise nur selten ändern. Mit deiner vorgeschlagenen Änderung würde dieses Caching verschwinden. Gut, das kann man machen, ist vielleicht bei den heutigen CPU-Geschwindigkeiten kein Thema mehr. Hätte auch den Vorteil, dass es keinen Mutex braucht. Muss ich mir noch überlegen...

Noch was: Ich wollte diese Änderung eigentlich möglichst so machen, dass sich für Plugins funktionell nichts ändert, um zu vermeiden, dass es einen "Bruch" gibt (daher auch die ganzen verschiedenen Matches()-Versionen). Wenn ein Plugin bisher Matches() mit Directly==true aufgerufen hat und sich darauf verließ, dass danach StartTime() die "rohe" StartZeit liefert, dann würde das nach deiner Änderung nicht mehr funktionieren. Ob das ein großes Problem wäre, kann ich schlecht beurteilen.

MarkusE · July 6, 2025 at 4:30 PM New

kls ,

O.K., dann ändern wir StartTime(void) nicht.

Wir prüfen aber an jeder Stelle, an der StartTime() verwendet wird, ob die Zeit benötigt wird, ab der der Timer ein Device braucht. Und falls ja, und der Timer ein VPS Timer ist, ziehen wir an dieser Stelle noch Setup.VpsMargin von StartTime() ab. Ev. machen wir dafür eine neue Methode z.B cTimers::TimeDeviceRequired().

Wir dokumentieren das entsprechend in VDR, so dass auch Plugin Entwickler den Code anpassen können.

VDR intern müsste z.B. shutdown.c angepasst werden, damit VDR auch bei VPS Timern rechtzeitig aufwacht.

MarkusE · July 6, 2025 at 4:39 PM New

Ich sehe gerade, in eTimerMatch cTimer::Matches(const cEvent *Event, int *Overlap) const :

Code

     if (UseVps) {
        if (startTime == Event->Vps()) {
           overlap = FULLMATCH;
           if (Event->IsRunning())
              overlap += 200;
           else if (Event->RunningStatus() != SI::RunningStatusNotRunning)
              overlap += 100;
           }
        }

Hier muss if (startTime == Event->Vps()) korrigiert werden. Start time ist nicht die VPS Zeit des Timers. Ich denke, eine Methode, die die VPS Zeit eines Timers zurückgibt, würde helfen. Siehe oben.

kls · July 6, 2025 at 4:53 PM New

Quote from MarkusE

Wir prüfen aber an jeder Stelle, an der StartTime() verwendet wird, ob die Zeit benötigt wird, ab der der Timer ein Device braucht.

Dafür ist eigentlich der Margin Parameter bei Matches() gedacht:

///<If Margin is given, it is subtracted from
///< the timer's actual start time. This can be used to check whether the timer will start within the next Margin seconds.

kls · July 6, 2025 at 4:54 PM New

Quote from MarkusE

Hier muss if (startTime == Event->Vps()) korrigiert werden.

Schau ich mir nach Abschluss der aktuellen Änderung an.

MarkusE · July 6, 2025 at 5:09 PM New

> Dafür ist eigentlich der Margin Parameter bei Matches() gedacht

Und wenn StartTime() aufgerufen wird, und nicht Matches()?

Ohne Anspruch auf Vollständigkeit: In shutdown.c wird Timer->StartTime() in den Zeilen 174, 218, 240 verwendet. An keiner dieser Stellen wird Matches() verwendet. An jeder dieser Stellen ist bei VPS Timern die Korrektur um Setup.VpsMargin notwendig.

Die Frage ist, welche Zeit ist bei VPS Timern relevant? Die Zeit, ab der VDR vermutlich Daten auf die Festplatte schreiben wird? Oder die Zeit, ab der VDR an sein muss, und ein Device benötigt, das auf den entsprechenden Transponder getuned sein muss?

kls · July 6, 2025 at 5:15 PM New

Hier wäre dann die aus meiner Sicht abschließende Fassung des Patches (wieder mit und ohne Whitespace-Unterschiede).

Das Ziel war, das in #1 geschilderte Problem zu lösen, ohne dabei die Funktionalität bestehender Plugins zu gefährden. Gleichzeitig wird dadurch die Semantik von cTimer::Matches() vereinfacht, da der Parameter Directly verschwindet.

Es wäre schön, wenn jemand testen könnte, ob mit dem Patch Plugins, die cTimer verwenden, problemlos übersetzt werden können und ggf. die erwähnten Compile- bzw. Log-Meldungen bringen.

Code

- Reworked cTimer::Matches():
  + Calculating the raw start/stop time of a timer has been moved to the new function
    cTimer::CalcStartStopTime(). If a plugin calls cTimer::Matches() with Directly==true,
    it should use cTimer::CalcStartStopTime() instead. If it calls cTimer::Matches() with
    Directly==false, it should call cTimer::Matches(time_t t, int Margin) instead.
  + The versions of cTimer::Matches() with 'bool Directly' are deprecated. Existing calls
    with Directly==true are redirected to cTimer::CalcStartStopTime(), and a log message
    is issued.
  + cTimer::Matches() now reports an error if it is called with a time parameter that is
    not the current time.
  + The "first day" parameter of a repeating timer is now only reset if cTimer::Matches()
    is called with t==0 and Margin==0 and it has been exceeded.
  + Access to the cached cTimer::start-/stopTime members is now protected via a mutex.
  + Plugins that use cTimer should continue to work as before. However, the author should
    react accordingly to compile time and log messages regarding these modifications.
    To test whether the plugin code will compile once the deprecated functions are
    removed in a future version, the macro DEPRECATED_TIMER_MATCHES can be set to 0 in
    timers.h.

Display More

kls · July 6, 2025 at 5:20 PM New

Quote from MarkusE

Ohne Anspruch auf Vollständigkeit: In shutdown.c wird Timer->StartTime() in den Zeilen 174, 218, 240 verwendet. An keiner dieser Stellen wird Matches() verwendet. An jeder dieser Stellen ist bei VPS Timern die Korrektur um Setup.VpsMargin notwendig.

Schau ich mir an.

Quote from MarkusE

Die Frage ist, welche Zeit ist bei VPS Timern relevant? Die Zeit, ab der VDR vermutlich Daten auf die Festplatte schreiben wird? Oder die Zeit, ab der VDR an sein muss, und ein Device benötigt, das auf den entsprechenden Transponder getuned sein muss?

Die Änderung, um die es in diesem Thread geht, bezieht sich allein darauf, dass (wie du zu Recht bemängelt hast) cTimer::StartTime() unterschiedliche Zeiten liefert, je nachdem, mit welchen Parametern cTimer::Matches() aufgerufen wurde. Falls nicht noch ein Bug gefunden wird, halte ich dieses Problem mit dem Patch aus #50 für erledigt. Weitere Probleme würde ich dann gerne separat in eigenen Threads behandeln.

FireFly · July 6, 2025 at 7:32 PM New

Quote from MarkusE

In shutdown.c wird Timer->StartTime() in den Zeilen 174, 218, 240 verwendet. An keiner dieser Stellen wird Matches() verwendet. An jeder dieser Stellen ist bei VPS Timern die Korrektur um Setup.VpsMargin notwendig.

siehe RE: VDR version 2.7.6 freigegeben

kls · July 6, 2025 at 8:52 PM New

Quote from FireFly

siehe RE: VDR version 2.7.6 freigegeben

Ich sehe gerade, dass ich mir deinen Post schon vorgemerkt habe.

kls · July 8, 2025 at 9:19 AM New

Quote from MarkusE

Bei wiederholenden Timern würde ich aber anstelle von...

Das wäre dann so, oder?

Diff

--- timers.c    2025/07/06 15:06:55     5.26
+++ timers.c    2025/07/08 07:17:49
@@ -594,7 +594,7 @@
          if (DayMatches(t0)) {
             time_t a = SetTime(t0, begin);
             if (length < 0)
-               t0 = IncDay(d, i + 1);
+               t0 = d + (i + 1) * SECSINDAY; // length shall always be the same, even on DST change
             time_t b = SetTime(t0, end);
             if ((!day || a >= day) && t < b) {
                startTime = a;

Display More

MarkusE · July 8, 2025 at 4:00 PM New

Quote from kls

Das wäre dann so, oder?

Diff

--- timers.c    2025/07/06 15:06:55     5.26
+++ timers.c    2025/07/08 07:17:49
@@ -594,7 +594,7 @@
          if (DayMatches(t0)) {
             time_t a = SetTime(t0, begin);
             if (length < 0)
-               t0 = IncDay(d, i + 1);
+               t0 = d + (i + 1) * SECSINDAY; // length shall always be the same, even on DST change
             time_t b = SetTime(t0, end);
             if ((!day || a >= day) && t < b) {
                startTime = a;

Display More

Hatte ich so geschrieben, muss mich aber selbst korrigieren.

Die stopTime sollte aus der startTime + Länge des Timers berechnet werden. Damit werden Fehler vermieden, die entstehen, falls z.B. i = 5 ist und zwischen i = 1 und i = 2 ein Wechsel zwischen Sommer-/Winterzeit stattfindet.

Also

Code

 time_t b;
 if (length >= 0) b = a + length;
 else b = a + SECSINDAY + length;

kls · July 8, 2025 at 4:20 PM New

Oder vielleicht so:

Diff

--- timers.c    2025/07/06 15:06:55     5.26
+++ timers.c    2025/07/08 14:16:05
@@ -593,9 +593,9 @@
          time_t t0 = IncDay(d, i);
          if (DayMatches(t0)) {
             time_t a = SetTime(t0, begin);
+            time_t b = a + length;
             if (length < 0)
-               t0 = IncDay(d, i + 1);
-            time_t b = SetTime(t0, end);
+               b += SECSINDAY;
             if ((!day || a >= day) && t < b) {
                startTime = a;
                stopTime = b;

Display More

kls · July 10, 2025 at 6:52 PM New

Quote from MarkusE

Ich denke, wir brauchen eine neue Methode VpsTime()

Ich habe das jetzt mal eingebaut und in cTimer::Expired() verwendet.
Ich denke, die Funktion braucht einen time_t Parameter, damit man bei wiederholenden Timern auch eine zukünftige VPS-Zeit ermitteln kann. tfVps abzufragen halte ich hier für unnötig, da man an der Aufrufstelle sowieso in einem VPS-Kontext sein wird.

Siehst du noch weitere Stellen, an denen diese Funktion verwendet werden sollte?

MarkusE · July 10, 2025 at 7:46 PM New

Ich würde es noch in void cTimer::Skip(void) verwenden:

Code

void cTimer::Skip(void)                                                                                                                             
{                                                                                                                                                   
  cMutexLock MutexLock(&mutex);                                                                                                                     
  day = IncDay(SetTime(VpsTime(), 0), 1);                                                                                                     
  startTime = 0;                                                                                                                                
  SetEvent(NULL);                                                                                                                                   
}

Grund: event->StartTime() kann größer als VpsTime() sein. Wenn z.B. VpsTime() == 23:59 Uhr ist und event->StartTime() == 00:01 Uhr)ist und IncDay(SetTime(event->StartTime(), 0), 1); verwendet wird, werden 2 Tage übersprungen.

In bool cTimer::Expired(void) würde ich time_t Vps = VpsTime(); vor die for(const cEvent *e = FirstEvent; e; e = Schedule->Events()->Next(e) Schleife schreiben.

kls · July 10, 2025 at 9:10 PM New

Quote from MarkusE

Ich würde es noch in void cTimer::Skip(void) verwenden

Stimmt.

Quote from MarkusE

In bool cTimer::Expired(void) würde ich time_t Vps = VpsTime(); vor die for(const cEvent *e = FirstEvent; e; e = Schedule->Events()->Next(e) Schleife schreiben.

Sorry, du hattest das hier ja auch schon richtig so gemacht. Hab's korrigiert.

cTimer::StartTime() gibt inkonsistente Zeiten zurück.

Participate now!

Users Viewing This Thread