Bug bei der Verwendung von TimeSeries in fhem.pl

Begonnen von mumpitzstuff, 04 November 2018, 23:54:34

Vorheriges Thema - Nächstes Thema

mumpitzstuff

Wenn man sich einen event-aggregator z. B. wie folgt anlegt:

attr Revolt_5d31 event-aggregator power::none:median:120,energy::none:median:120,avgpower::none:median:120

dann kommt es öfter zu folgendem Fehler im Logfile:

2018.11.01 11:58:07 1: PERL WARNING: Argument "" isn't numeric in numeric ge (>=) at FHEM/TimeSeries.pm line 265.
2018.11.01 11:58:07 1: stacktrace:
2018.11.01 11:58:07 1:     main::__ANON__                      called by FHEM/TimeSeries.pm (265)
2018.11.01 11:58:07 1:     TimeSeries::elapsed                 called by fhem.pl (4719)
2018.11.01 11:58:07 1:     main::readingsBulkUpdate            called by ./FHEM/19_Revolt.pm (138)
2018.11.01 11:58:07 1:     main::Revolt_Parse                  called by fhem.pl (3795)
2018.11.01 11:58:07 1:     main::Dispatch                      called by ./FHEM/00_CUL.pm (948)
2018.11.01 11:58:07 1:     main::CUL_Parse                     called by ./FHEM/00_CUL.pm (832)
2018.11.01 11:58:07 1:     main::CUL_Read                      called by fhem.pl (3599)
2018.11.01 11:58:07 1:     main::CallFn                        called by fhem.pl (726)


Die Ursache ist eigentlich ganz simpel. In fhem.pl wird ein TimeSeries Objekt erzeugt und dabei ein Parameter autoreset übergeben.

      my (undef,$duration,$method,$function,$holdTime) = split(":", $v[0], 5);
      my $ts;
      if(defined($readings->{".ts"})) {
        $ts= $readings->{".ts"};
      } else {
        require "TimeSeries.pm";
        $ts= TimeSeries->new( { method => $method,
                                autoreset => $duration,
                                holdTime => $holdTime } );


Dummerweise wird hier nicht geprüft, ob $duration nicht eventuell ein Leerstring ist und dieser Leerstring wird dann übergeben. Innerhalb von TimeSeries wird autoreset aber nur mit defined überprüft, was dann in der elapsed Methode von TimeSeries zum Fehler führt. Man müsste in fhem.pl $defined also in ein undef umwandeln, falls defined ein Leerstring enthält.
Man müsste daher diesen Code:

        require "TimeSeries.pm";
        $ts= TimeSeries->new( { method => $method,
                                autoreset => $duration,
                                holdTime => $holdTime } );


durch eventuell sowas ersetzen:

        require "TimeSeries.pm";
        $ts= TimeSeries->new( { method => $method,
                                autoreset => (looks_like_number($duration) ? $duration : undef),
                                holdTime => $holdTime } );


Ich bin mir nicht ganz sicher, aber eventuell müsste man für holdtime sowas ähnliches auch machen? Ist bei mir immer gesetzt, deshalb habe ich keine Ahnung...

mumpitzstuff

Besteht kein Interesse das Problem zu beheben? Soll/Kann ich noch etwas beisteuern?

CoolTux

Interessant finde ich das sonst noch keiner sowas gemeldet hat. Scheint eher selten gebraucht zu werden.
Ich stupse es aber mal an da ich den "Fehler" Codetechn. mir erklären/nachvollziehen kann.
Du musst nicht wissen wie es geht! Du musst nur wissen wo es steht, wie es geht.
Support me to buy new test hardware for development: https://www.paypal.com/paypalme/MOldenburg
My FHEM Git: https://git.cooltux.net/FHEM/
Das TuxNet Wiki:
https://www.cooltux.net

mumpitzstuff

Es gab dazu 2-3 Erwähnungen im Forum. Wenn man nach timeseries sucht, wird man fündig.

mumpitzstuff

Kann bitte der folgende Patch aufgenommen werden? Er verhindert die hier beschriebenen Warnings:

fhem/fhem.pl | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fhem/fhem.pl b/fhem/fhem.pl
index d8dccc86a..9f3e23a60 100755
--- a/fhem/fhem.pl
+++ b/fhem/fhem.pl
@@ -4874,8 +4874,8 @@ readingsBulkUpdate($$$@)
       } else {
         require "TimeSeries.pm";
         $ts= TimeSeries->new( { method => $method,
-                                autoreset => $duration,
-                                holdTime => $holdTime } );
+                                autoreset => (looks_like_number($duration) ? $duration : undef),
+                                holdTime => (looks_like_number($holdTime) ? $holdTime : undef) } );
         $readings->{".ts"}= $ts;
         # access from command line:
         # { $defs{"myClient"}{READINGS}{"myValue"}{".ts"}{max} }

rudolfkoenig


Dr. Boris Neubert

Please revert.

$duration und $holdTime müssen Zahlen sein, sonst hat der User was falsch gemacht und dafür sind die Warnings da. Das erschließt sich mir zunächst aus der Commandref zu event-aggregator (von mir ;-).

Wir müssen nur an der richtigen Stelle prüfen, ob diese Werte gesetzt sind. Wenn nicht, ist was anderes im Code falsch, weil man mit undef nicht rechnen kann. Ich weiß aber nicht, ob ich da so bald dazukomme.
Globaler Moderator, Developer, aktives Mitglied des FHEM e.V. (Marketing, Verwaltung)
Bitte keine unaufgeforderten privaten Nachrichten!

rudolfkoenig

ZitatPlease revert.
Erledigt.

Zitat$duration und $holdTime müssen Zahlen sein, sonst hat der User was falsch gemacht und dafür sind die Warnings da.
Gerne, aber wieso stoert dann der Patch?

Dr. Boris Neubert

Zitat von: rudolfkoenig am 24 Januar 2020, 18:57:36
Erledigt.
Gerne, aber wieso stoert dann der Patch?

Danke.

Ich bin mir nicht sicher, ob der Patch stört. Dass müsste frickelpiet nun morgen nach dem Update testen.

Mich stört der Patch, weil er das Problem nicht lösen kann.
Globaler Moderator, Developer, aktives Mitglied des FHEM e.V. (Marketing, Verwaltung)
Bitte keine unaufgeforderten privaten Nachrichten!

mumpitzstuff

Stimmt. Darum wollte ich mich kümmern, habe aufgrund meiner Kinder leider für gar nichts mehr Zeit. Die Warnungen waren zwar weg, die Funktion leider auch, wenn :: anstatt :0: verwendet wurde. Tut mir leid.

Dr. Boris Neubert

Konnte die Finger nicht davonlassen.

Kann mal bitte jemand probieren, in fhem.pl die Zeile 4866

      my (undef,$duration,$method,$function,$holdTime) = split(":", $v[0], 5);


nach

      my (undef,$duration,$method,$function,$holdTime) = map { $_ eq "" ? undef : $_ } split(":", $v[0], 5);


zu ändern und berichten, ob es geht?

Ungetestet!

Globaler Moderator, Developer, aktives Mitglied des FHEM e.V. (Marketing, Verwaltung)
Bitte keine unaufgeforderten privaten Nachrichten!