98_RandomTimer.pm Code Review und packages

[RichardCZ]

@Beta-User - dieses no warnings smartmatch-operator ... das ist notwendig oder ist das ein Überbleibsel? Ich konnte jetzt auf den ersten Blick nirgends eine Verwendung von ~~ entdecken.

[Beta-User]

Zu finden in der sub Attr ab Zeile 177. (Oder habe ich die Frage falsch interpretiert?)

Ich hänge auch mal eine etwas aktualisierte Fassung vom WDT an, da wir grade dabei sind...
Da gibt es auch 4 "eval"-Warnungen.

- Über Zeile 461 denke ich grade nach, bin aber noch nicht zu einem Ergebnis gekommen, was das eigentlich für einen Zweck hat.

- Zeilen 514 und 525 sehen mir danach aus, also könnte das ein Fall für die Umwandlung in unbenannte sub's sein, aber sicher bin ich mir nicht.

=> Vielleicht kann du als Experte das jeweils auf einen Blick beantworten, bevor ich lange rumteste?

- Zeile 1015 sieht mir wie ein Wiedergänger von der RT-Problematik aus, hier allerdings noch "garniert" durch die Ersetzung typischer Parameter. => Plaster?

Ggf. hast du damit dann auch etwas mehr "Spielmaterial" für einen Vorschlag für eine "zentrale eval-Alternative"...?

[rudolfkoenig]

Die "zentrale eval-Alternative" heisst AnalyzePerlCommand.
Der Haken: neben eval macht es etliche potentiell ueberfluessige Sachen ($hour,$we etc setzen, Variablen wie EVTPARTx zu uebergeben), ist also langsamer, als ein einfaches eval.
Das Gute: wenn das eval ein WARNING generiert, findet man mit stacktrace raus, wo das Problem aufgetreten ist, und was gerade dabei evaluiert wurde.

[RichardCZ]

Zu finden in der sub Attr ab Zeile 177. (Oder habe ich die Frage falsch interpretiert?)

Uh - ne ich seh's schon.

Code (perl) Auswählen Erweitern

sub Attr {
    my ($cmd, $name, $attrName, $attrVal) = @_;

    my $hash = $defs{$name};

    if ($attrName eq 'switchmode') {
        setSwitchmode($hash, $attrVal);
    }

    if ($attrName =~ m{\A disable(Cond)? \z}xms) {
        # Pull the schaltening before, because offturning wanted when disable instead of teutonian comments
        RmInternalTimer("Exec", $hash);
        MkInternalTimer("Exec", time()+1, \&Exec, $hash, 0);
    }
   
    if ($attrName eq 'offState') {
        my ($offRegex, $offReading) = split ' ', $attrVal, 2;
        $hash->{helper}{offRegex}   = $offRegex;
        $hash->{helper}{offReading} = $offReading // 'state';
    }
 
    return;
}


Dann sollte die nowarnings smartwatch Geschichte auch wegkönnen.

Ich hänge auch mal eine etwas aktualisierte Fassung vom WDT an, da wir grade dabei sind...
Da gibt es auch 4 "eval"-Warnungen.

- Über Zeile 461 denke ich grade nach, bin aber noch nicht zu einem Ergebnis gekommen, was das eigentlich für einen Zweck hat.

Was der Code für einen Sinn hat, habe ich auch noch nicht begriffen, aber da sollte jedenfalls ein Block-Eval gehen,

- Zeilen 514 und 525 sehen mir danach aus, also könnte das ein Fall für die Umwandlung in unbenannte sub's sein, aber sicher bin ich mir nicht.

Sinn wieder mal dahingestellt, auch das geht mit einem Block-Eval (Compilezeit)

- Zeile 1015 sieht mir wie ein Wiedergänger von der RT-Problematik aus, hier allerdings noch "garniert" durch die Ersetzung typischer Parameter. => Plaster?

Ggf. hast du damit dann auch etwas mehr "Spielmaterial" für einen Vorschlag für eine "zentrale eval-Alternative"...?

Also WeekdayTimer_FensterOffen ist eine richtige Baustelle. Es würde mir helfen, wenn ich ein paar Beispiele sehen würde für den Inhalt von $verzoegerteAusfuehrungCond bevor irgendwas damit gemacht wird. Also schon zu dem Zeitpunkt an dem es aus
AttrVal($hash->{NAME}, "delayedExecutionCond", "0");
geholt wurde.

Die "zentrale eval-Alternative" heisst AnalyzePerlCommand.
Der Haken: neben eval macht es etliche potentiell ueberfluessige Sachen ($hour,$we etc setzen, Variablen wie EVTPARTx zu uebergeben), ist also langsamer, als ein einfaches eval.
Das Gute: wenn das eval ein WARNING generiert, findet man mit stacktrace raus, wo das Problem aufgetreten ist, und was gerade dabei evaluiert wurde.

Unabhängig von der kleinen Performance Penalty sollte man es dann trotzdem verwenden. So fehlt z.B. in dem o.g. WDT Code an Zeile 1015 jedwede $@ Behandlung bzw. ein Abfangen potentieller Fehler. Somit ist das eine Stelle, wo man FHEM hart "zur Landung zwingen" könnte.

[Beta-User]

Hmm, also für die RT-eval-Geschichte sähe das dann mit AnalyzePerlCommand so aus, oder?
(hätte ich ja auch selbst drauf kommen können..., Analy.*muß dann auch nach GP_Import).

Code Auswählen Erweitern

sub isDisabled {
   my $hash = shift;

   my $disable = AttrVal($hash->{NAME}, "disable", 0 );
   return $disable if($disable);

   my $disableCond = AttrVal($hash->{NAME}, "disableCond", "nf" );
   if ($disableCond eq "nf") {
     return 0;
   }
   
   return AnalyzePerlCommand($hash, $disableCond);
   
   ##seitherige Fassung auskommentiert:
   #else {
   #   $disable = eval ( $disableCond ); ## no critic 'eval' # not working when written as eval  { $disableCond };
   #   if ($@) {
   #      $@ =~ s/\n/ /gx;
   #      Log3 ($hash, 3, "[$hash->{NAME}] ERROR: " . $@ . " EVALUATING " . $disableCond);
   #   }
   #   return $disable;
   #}
}

Wenn das da klappt, sollte es dann 1:1 auch für den WDT zu verwenden sein, von daher sehe ich mal davon ab, dazu ein Beispiel zu liefern.

Das mit dem smartmatch-Ersatz folgt dann noch, und da und vorallem auch bei dem Block-call fehlt mir noch der Durchblick, wie ich das ggf. sinnvoll testen könnte...
Vermutlich macht es am meisten Sinn, das "für Mutige" zum Testen anzubieten, aber bei meinen diesbezüglichen Anfragen (auch für die wenigen neuen features da) war in der Vergangenheit wenig feedback zu bekommen. Ggf. würde ich das - wenn fertig und ggf. auch nochmal von euch kritisch beäugt - auch "auf Verdacht" ins svn schieben.

ABER (bzgl. direkter Veröffentlichung@svn): Es wäre nicht sooo toll, wenn wir hier Aufräumarbeiten anschieben und das erste, was die User davon sehen, wären dann unmotivierte Abstürze oder unerklärliche neue Verhaltensweisen. Für den Fall dass, hoffe ich auf viele Helfer, die schnell mit reparieren...

(@CoolTux: Das "Pflaster" beim Package-Namen darf bleiben, auch wenn ich mich damit vermutlich für die "Hall of fame" disqualifiziere . Und wenn es dann läuft, machen wir ggf. auch noch eine "Schönheitoperation" betr. die Optik, wenn es denn sein muß. Aber wir beschränken uns dann darauf und trennen es strikt von funktionalen Themen .)

Wenn wir hier beim RT durch sein sollten, würde ich mich ggf. direkt noch an WDT machen, ABER: auch das Teil ist ein "Erbstück", das schon durch ein paar Hände gegangen ist, und die verbliebenen "Level-3-Muher" kann ich zwar nachvollziehen, aber die zu lösen ist ungleich schwieriger als das Löschen von ein paar Prototypes...

[Wzut]

@Beta-User, warum 

Code Auswählen Erweitern

my $disable = AttrVal($hash->{NAME}, "disable", 0 );
   return $disable if($disable);
und nicht :

Code Auswählen Erweitern

return if (AttrNum($hash->{NAME}, "disable", 0 ));

BTW : Mit dem Attr disable habe ich früher auch nur so gemacht, bis mir Module unter kamen die zusätlich noch ein set disable unterstützen.
Vorteil : mit dem set kann man auch mal eben schnell temporär das Modul lahmlegen ohne config Änderung (Stichwort rotes Fragezeichen).
I.d.R. zieht das set <name> disable auch gleich state mit , das ReadingsVal( $hash->{NAME}, "state", 'disabled' ) müsste dann natürlich oben noch als oder dazu.
 

[Beta-User]

...das scheint jedenfalls bei ersten Tests zu klappen, anbei daher nochmal eine aktualisierte Fassung samt Gesamt-patch...

Hoffe, so wird ein Schuh draus, und an diesem Muster können sich ggf. dann auch andere orientieren.

Die elegantere Fassung mit

Code Auswählen Erweitern

return if (AttrNum($hash->{NAME}, "disable", 0 ));ist eingebaut, über den 2. Aspekt muß ich bei Gelegenheit mal nachdenken. Ist zwar doof, dass man das disable bei diesen alten Modulen teils nur via Attribut setzen kann, aber hier ist es so, dass sich der RT ja je nach Attributauswertung auch auf "disable" setzt. Fürchte, da wird dann eine unbeabsichte (logische) "Schleife" an der falschen Stelle geschlossen (?). Wenn ich da aber für die "psitive Auswertung" des Attributs jetzt mit einem anderen "state"-Inhalt reinfunke, ist das Risiko groß, dass sich da direkt jemand beschwert, weil er das irgendwie auswertet... (?)

[rudolfkoenig]

Die elegante Version ist mAn ist IsDisabled($hash->{NAME}), sie prueft auch nach disabledForIntervals.

[Beta-User]

Die elegante Version ist mAn ist IsDisabled($hash->{NAME}), sie prueft auch nach disabledForIntervals.

Kurze Rückmeldung dazu:

Nette Idee, ich werde versuchen, das feature einzubauen.
Das hat aber einen "kleinen" Haken: Auch die Evaluierung der DisableCondition führt zu einem "inactive" (sofern zutreffend) oder einer Aktivierung für diesen Tag bei Tageswechsel. Irgendwie muß man daher m.E. die Fälle auch dauerhaft auseinanderhalten können, jedenfalls, wenn man bei der Gelegenheit auch noch ein "set xy inactive" bzw. active zulassen möchte und disableForInternals nicht dazu führen soll, dass wirklich nur für den Zeitraum "still" ist... Will heißen: Dauert noch etwas, ich muß das gedanklich erst nochmal in den ganzen Varianten durchspielen. Vermutlich muß ich ein Reading generieren, ob (nur) die Evaluierung inactive ergibt oder ob es eine User-Anweisung ist (=dauerhaft, bis zur willentlichen Wieder-Aktivierung).