70_Klafs.pm Code Review

Begonnen von xasher, 20 Mai 2022, 11:22:48

Vorheriges Thema - Nächstes Thema

xasher

Hallo zusammen,

ich habe für meine Saunasteuerung ein Modul gebaut und habe inzwischen einen Stand erreicht, wo ich einen erfahrenen Programmierer bitten möchte drüberzuschauen.
Aktuell ist die Version nicht als Package konzipiert. Da kenne ich mich (derzeit) noch nicht aus.
Ich habe das ganze schon mal im Forum bekannt gegeben: https://forum.fhem.de/index.php?topic=127701.msg1222018#msg1222018 (ok, ist nicht so verbreitet die Sauna)
Mit Beta-User konnte ich schon viele Verbesserungen durchführen - vielen Dank an dich an dieser Stelle.

So konnte das Passwort in das set überführt werden. Danke an CoolTux!
Außerdems konnten die Readings stark vereinfacht werden.

An dieser Stelle würde ich das gerne öffentlich diskutieren.

Besten Dank schon mal vorab

Viele Grüße
Alex

Beta-User

Hi  :) ,

vorab mal Danke, dass du die Anregung aufgegriffen hast!

Anbei mal eine erste "Rückmeldung" (nochmal der Hinweis: ich kann das nur dahingehend checken, ob es ohne Fehler lädt!). Vorschlag wäre, du machst dazu mal ein "diff -u" zu deiner Fassung, dann können wir die Unterschiede vielleicht am einfachsten durchgehen und es ist dann auch für andere leichter, den entsprechenden Code-Schnipsel zu verstehen und ggf. auch ihre Meinung dazu zu sagen. Bitte erforderlichenfalls dann die betreffende Funktion komplett in Code-Tags zeigen.

Sachen, die (jetzt und für dich) selbstverständlich/selbsterklärend sind, kannst du gerne einfach umsetzen/übernehmen, das ist im diff nicht mehr so interessant. Was "Quotes" angeht, sollten wir diese nach und nach bereinigen. Was nicht extrapoliert werden muss, kann in einfache Quotes oder ein "q", siehe (u.A.) https://www.perlmonks.org/?node_id=401006...

Da ist jedenfalls ein "bunter Strauß" von Sachen drin, von denen ich als nicht soooo erfahrener Perl-Anwender annehme, dass die auch für andere User und Gelegenheits-Maintainer interessant sind ;) .

Grüße, Beta-User
Server: HP-elitedesk@Debian 12, aktuelles FHEM@ConfigDB | CUL_HM (VCCU) | MQTT2: MiLight@ESP-GW, BT@OpenMQTTGw | MySensors: seriell, v.a. 2.3.1@RS485 | ZWave | ZigBee@deCONZ | SIGNALduino | MapleCUN | RHASSPY
svn: u.a MySensors, Weekday-&RandomTimer, Twilight,  div. attrTemplate-files

xasher

Hallo Beta-User

vielen Dank für deine Antwort. Ich habe schon ein ersten Blick drübergeworfen. Sind mal wieder interessante Dinge dabei.
Das Wochenende ist leider etwas voll - bin aber dran - alsbald möglich.

Viele Grüße
Alex

Beta-User

Moin,

lass dir Zeit...

Vielleicht noch ein Hinweis wegen des Vorschlags, die Nach-Initialisierung nicht per NotifyFn() zu machen, sondern timer-basiert: In https://forum.fhem.de/index.php/topic,24658.msg1219913.html#msg1219913 findest du meine Überarbeitung von VALVES, und im ersten Beitrag die Vergleichsversion. Da ist u.A. auch dieser Punkt geändert...

Grüße und ein angenehmes WE,

Beta-User
Server: HP-elitedesk@Debian 12, aktuelles FHEM@ConfigDB | CUL_HM (VCCU) | MQTT2: MiLight@ESP-GW, BT@OpenMQTTGw | MySensors: seriell, v.a. 2.3.1@RS485 | ZWave | ZigBee@deCONZ | SIGNALduino | MapleCUN | RHASSPY
svn: u.a MySensors, Weekday-&RandomTimer, Twilight,  div. attrTemplate-files

xasher

Guten Morgen,

ich habe jetzt die Teile übernommen und übernommen, soweit es mir klar war.
Im Detail bedeutet das:

Zeilennummern bezieht sich auf die Version von Beta-User. Bei der aktuell angehängten Version kann es abweichen:

144: Raum vorbelegen, fand ich nett, da Unsorted schon recht voll ist. Kann man aber auch lassen. Es bringt der Sache nichts
152: Vorbelegung der Hashwerte im Define: Dachte, ich hätte mal Perl Fehlermeldungen bekommen nach dem Define. Seitdem ist es drin
431: RemainTime formatiert eingefügt, ging verloren

Darüberhinaus:
Log3 mit Klammern versehen
Klafs_connected mit Bulk übernommen

Dann zu den Schleifen. Du hast aus den foreach Schleifen for Schleifen gemacht. Kannst du nochmals erklären, warum wir das machen? Ich bin da vmtl nicht ganz firm drin. Das hatte ich mal aus einem Beispiel übernommen und es hat funktioniert.  ;D

Dann unten hast du noch ein Kommentar:
if ( $devtype eq 'KLAFS') { #Beta-User: KLAFS oder Klafs....?

Ja, gute Frage nun. In einer ersten Version hatte ich $hash->{TYPE} = 'KLAFS'; drinstehen. Das wurde aber mal entfernt. Wie kommt das jetzt zustande?
Vermutlich durch meine Definition in Zeile 1:
define mySauna KLAFS
attr mySauna disable 0
attr mySauna genericDeviceType switch
attr mySauna interval 30
attr mySauna pin 0000
attr mySauna room Homekit,Klafs
attr mySauna saunaid ab0c123d-ef4g-5h67-8ij9-k0l12mn34op5
attr mySauna siriName Sauna
attr mySauna username user
attr mySauna verbose 4


Grüße
Alex

Beta-User

Moin zurück.

Ein paar kurze Anmerkungen:
Zitat von: xasher am 23 Mai 2022, 08:23:34
144: Raum vorbelegen, fand ich nett, da Unsorted schon recht voll ist. Kann man aber auch lassen. Es bringt der Sache nichts
An sich ist das schon nett, hat aber ein paar Haken:
- direktes "Rummalen" in Standard-Hashes sollte man vermeiden, für Attribute nutzt man eben CommandAttr(), wenn man was setzen will. Will man aber @define-time eigentlich nicht.
- Wenn "unsorted" voll ist, ist das "dein Problem". Ist (wie manches, das wir jetzt gleich diskutieren) eine Philosophie-Frage: Ich nutze "unsorted" eher als Übergangsort und finde es eigentlich ganz ok, wenn alles dort landet, was ich nicht explizit irgendwohin geschoben habe. Das mache ich dann nämlich in der Regel mit allem, was nicht temporär ist, und nicht gebraucht wird, bekommt ein "ignore", bevor es in den Raum Steuerung->unused geht...

Zitat
152: Vorbelegung der Hashwerte im Define: Dachte, ich hätte mal Perl Fehlermeldungen bekommen nach dem Define. Seitdem ist es drin
Dass damit Fehlermeldungen unterdrückt werden sollen, war mir fast klar. Die Frage ist: Warum kamen die? Antwort: Direkter Zugriff auf den Device-Hash. Lösung: Methodenzugriff verwenden (InternalVal()) und einen default mitgeben...

Zitat
Du hast aus den foreach Schleifen for Schleifen gemacht. Kannst du nochmals erklären, warum wir das machen? Ich bin da vmtl nicht ganz firm drin. Das hatte ich mal aus einem Beispiel übernommen und es hat funktioniert.  ;D
Die Frage ist auch hier weniger, ob es funktioniert (tut es beides), sondern ob man an einer Schreibweise festhält, die nach meinem Kenntnisstand einfach nur noch eine unnötige Indirektion verursacht. Wenn man weiß, dass es funktional auf's selbe rausläuft, kann man sich diese paar Zeichen sparen ::) ...

Zitat
Dann unten hast du noch ein Kommentar:
if ( $devtype eq 'KLAFS') { #Beta-User: KLAFS oder Klafs....?
Du hast in deinem Device-Hash beide Varianten drinstehen. Der Sinn der unterschiedlichen Schreibweise hat sich mir nur noch nicht erschlossen. Das hat nichts mit der Frage zu Tun, wie man auf den Wert dann sinnvollerweise zugreift.
Server: HP-elitedesk@Debian 12, aktuelles FHEM@ConfigDB | CUL_HM (VCCU) | MQTT2: MiLight@ESP-GW, BT@OpenMQTTGw | MySensors: seriell, v.a. 2.3.1@RS485 | ZWave | ZigBee@deCONZ | SIGNALduino | MapleCUN | RHASSPY
svn: u.a MySensors, Weekday-&RandomTimer, Twilight,  div. attrTemplate-files

xasher

Hi Beta-User,

also,
1) Raum vorbelegen ist draußen. Das kann dann jeder selber machen
2) Vorbelegung der Hashwerte ist weitgehend draußen. in Zeile 229 bekomme ich einen Fehler. Ich habe in 228 das eingefügt "$hash->{Klafs}->{LoginFailures} //= '';"
3) Ich habe auf For-Schleifen umgestellt.
4) Das mit den Hashwerten ist mir noch nicht ganz klar. Stimmt, ich hatte mal $hash->{KLAFS} und $hash->{Klafs} gemischt im Einsatz. Aktuell aber nur diese Variante: $hash->{Klafs}

Soweit dazu. Ich habe aber gerade noch etwas Streß mit der Sub Klafs_Connected. Du hast meine Ursprungsversion um die Variable $notUseBulk erweitert:

#Beta-User
sub Klafs_CONNECTED {
    my $hash = shift // return;
    my $set  = shift;
    my $notUseBulk = shift;

    if ($set) {
      $hash->{Klafs}->{CONNECTED} = $set;

      if ( $notUseBulk ) {
          readingsSingleUpdate($hash,'state',$set,1) if $set eq ReadingsVal($hash->{NAME},'state','');
      } else {
        readingsBulkUpdate($hash,'state',$set) if $set eq ReadingsVal($hash->{NAME},'state','');
      }
      return;
    }
    return 'disabled' if $hash->{Klafs}->{CONNECTED} eq 'disabled';
    return 1 if $hash->{Klafs}->{CONNECTED} eq 'connected';
        return 0;
}


#Xasher: Ursprüngliche Version
sub Klafs_CONNECTED {
    my $hash = shift // return;
    my $set  = shift;
    if ($set) {
      $hash->{Klafs}->{CONNECTED} = $set;

      if (!defined($hash->{READINGS}->{state}->{VAL}) || $hash->{READINGS}->{state}->{VAL} ne $set) {
                      readingsSingleUpdate($hash,"state",$set,1);
      }
      return;
    } else {
      if ($hash->{Klafs}->{CONNECTED} eq 'disabled') {
        return 'disabled';
      }
      elsif ($hash->{Klafs}->{CONNECTED} eq 'connected')
      {
        return 1;
      } else {
        return 0;
      }
    }
}


Das bringt laut Log > 10 Refreshs in der Sekunde:
2022.05.23 11:18:20 4: mySauna - Update with device: ab0c123d-ef4g-5h67-8ij9-k0l12mn34op5
2022.05.23 11:18:20 3: mySauna - notify DoUpdate...
2022.05.23 11:18:20 4: mySauna - Update with device: ab0c123d-ef4g-5h67-8ij9-k0l12mn34op5
2022.05.23 11:18:20 3: mySauna - notify DoUpdate...
2022.05.23 11:18:20 4: mySauna - Update with device: ab0c123d-ef4g-5h67-8ij9-k0l12mn34op5
2022.05.23 11:18:20 3: mySauna - notify DoUpdate...
2022.05.23 11:18:20 4: mySauna - Update with device: ab0c123d-ef4g-5h67-8ij9-k0l12mn34op5
2022.05.23 11:18:20 3: mySauna - notify DoUpdate...
2022.05.23 11:18:20 4: mySauna - Update with device: ab0c123d-ef4g-5h67-8ij9-k0l12mn34op5
2022.05.23 11:18:20 3: mySauna - notify DoUpdate...
2022.05.23 11:18:20 4: mySauna - Update with device: ab0c123d-ef4g-5h67-8ij9-k0l12mn34op5
2022.05.23 11:18:20 3: mySauna - notify DoUpdate...
2022.05.23 11:18:20 4: mySauna - Update with device: ab0c123d-ef4g-5h67-8ij9-k0l12mn34op5
2022.05.23 11:18:20 3: mySauna - notify DoUpdate...
2022.05.23 11:18:21 4: mySauna - Update with device: ab0c123d-ef4g-5h67-8ij9-k0l12mn34op5
2022.05.23 11:18:21 3: mySauna - notify DoUpdate...
2022.05.23 11:18:21 4: mySauna - Update with device: ab0c123d-ef4g-5h67-8ij9-k0l12mn34op5
2022.05.23 11:18:21 3: mySauna - notify DoUpdate...
2022.05.23 11:18:21 4: mySauna - Update with device: ab0c123d-ef4g-5h67-8ij9-k0l12mn34op5
2022.05.23 11:18:21 3: mySauna - notify DoUpdate...


Ich kapiers gerade nicht, warum das passiert

Ich hänge mal die aktuelle Version dran. Vielleicht sollten wir uns auf die beziehen. Deine Anmerkungen sollten drin sein - bis auf Klafs_CONNECTED. Diff ist langsam unübersichtlich.

Grüße,
Alex

Beta-User

Zitat von: xasher am 23 Mai 2022, 11:43:23
Ich kapiers gerade nicht, warum das passiert
Klingt danach, als wäre deine NotifyFn() irgendwie suboptimal (wobei da TYPE noch groß drin ist, was mir seltsam vorkommt, aber der log-Eintrag liest sich so)...
Zitat von: Beta-User am 21 Mai 2022, 10:10:09

Vielleicht noch ein Hinweis wegen des Vorschlags, die Nach-Initialisierung nicht per NotifyFn() zu machen, sondern timer-basiert: In https://forum.fhem.de/index.php/topic,24658.msg1219913.html#msg1219913 findest du meine Überarbeitung von VALVES, und im ersten Beitrag die Vergleichsversion. Da ist u.A. auch dieser Punkt geändert...
Als Sofortmaßnahme würde ich erst mal empfehlen, in Define das hier aufzunehmen ;) :
setNotifyDev($hash,'global');
Braucht aber ein ziemlich aktuelles fhem.pl!
Server: HP-elitedesk@Debian 12, aktuelles FHEM@ConfigDB | CUL_HM (VCCU) | MQTT2: MiLight@ESP-GW, BT@OpenMQTTGw | MySensors: seriell, v.a. 2.3.1@RS485 | ZWave | ZigBee@deCONZ | SIGNALduino | MapleCUN | RHASSPY
svn: u.a MySensors, Weekday-&RandomTimer, Twilight,  div. attrTemplate-files

xasher

Zitat von: Beta-User am 23 Mai 2022, 12:02:56
Klingt danach, als wäre deine NotifyFn() irgendwie suboptimal

Ich habe den Code mal angeschaut und versuche das zu interpretieren. Du hast
1) NotifyFn rausgenommen
2)  eine Ergänzung im Define gemacht:
    InternalTimer(gettimeofday() + AttrVal($name,'valvesInitialDelay',61), 'VALVES_GetUpdate', $hash, 0) if !$init_done;
    notifyRegexpChanged($hash, 'global',1);
    VALVES_GetUpdate($hash) if $init_done && !AttrVal($name,'disable',0);

Interpretiert heißt das in etwa: Timer setzen mit dem Attribut 'valvesInitialDelay' bzw 61 Sekunden, wenn nichts dfiniert ist und wenn die Initialisierung noch nicht abgeschlossen ist.
Oder was heißt "if !$init_done;"
Das kann ich mir nicht herleiten: notifyRegexpChanged($hash, 'global',1);
Letzte Zeile:  VALVES_GetUpdate($hash) if $init_done && !AttrVal($name,'disable',0);
Update, wenn Initalisierung abgeschlossen und das Device nicht disabled ist.

Grüße
Alex

Beta-User

Zitat von: xasher am 23 Mai 2022, 12:57:13
Ich habe den Code mal angeschaut und versuche das zu interpretieren. Du hast
1) NotifyFn rausgenommen
2)  eine Ergänzung im Define gemacht:
    InternalTimer(gettimeofday() + AttrVal($name,'valvesInitialDelay',61), 'VALVES_GetUpdate', $hash, 0) if !$init_done;
    notifyRegexpChanged($hash, 'global',1);
    VALVES_GetUpdate($hash) if $init_done && !AttrVal($name,'disable',0);

Es gibt noch eine relevante Änderung in dem Zusammenhang; die findet sich in Attr (u.A. bei disable)*.

Zitat
Interpretiert heißt das in etwa: Timer setzen mit dem Attribut 'valvesInitialDelay' bzw 61 Sekunden, wenn nichts dfiniert ist und wenn die Initialisierung noch nicht abgeschlossen ist.
Oder was heißt "if !$init_done;"
Genau. Bei Einlesen der DEF bei FHEM-Start erst mal 61 Sekunden delay rein (das Attribut gibt es da noch nicht); wird später ein define/defmod angewiesen, wird der Attributwert genommen. *Der Timer wird modifiziert, wenn dann das Attribut gesetzt wird (meine ich).

Zitat
Das kann ich mir nicht herleiten: notifyRegexpChanged($hash, 'global',1);
Ich mir auch nicht mehr; ist ein Rest aus den Versuchen, ob es ohne NotifyFn() klappt (die wird dadurch deaktiviert, siehe https://wiki.fhem.de/wiki/DevelopmentModuleAPI#notifyRegexpChanged).

Zitat
Letzte Zeile:  VALVES_GetUpdate($hash) if $init_done && !AttrVal($name,'disable',0);
Update, wenn Initalisierung abgeschlossen und das Device nicht disabled ist.
Genau. Ist das Device disabled (oder wird es später) wird kein Timer gesetzt bzw. der Timer gelöscht. Daher wird bei Löschen von disable bzw. 0-Setzen dann auch wieder der Timer gestartet...

=> NotifyFn() nicht mehr erforderlich...

Dein Problem (und auch das von VALVES in der Ausgangsversion) war, dass bei ALLEN Events diese NotifyFn() aufgerufen wurde, also jeder Temperatur-Änderung auf dem Mond, ...
Die 10x/Sek. sind übrigens recht viel, wobei mir nicht klar ist, ob da auch noch eine Art Selbsttriggerung eine Rolle gespielt hatte. Du solltest mal sehen, wie viele Events durch den Event-Monitor durchrauschen und das ggf. auch eingrenzen.
Server: HP-elitedesk@Debian 12, aktuelles FHEM@ConfigDB | CUL_HM (VCCU) | MQTT2: MiLight@ESP-GW, BT@OpenMQTTGw | MySensors: seriell, v.a. 2.3.1@RS485 | ZWave | ZigBee@deCONZ | SIGNALduino | MapleCUN | RHASSPY
svn: u.a MySensors, Weekday-&RandomTimer, Twilight,  div. attrTemplate-files

xasher

Hallo,

also, ich hab notify rausgenommen und es jetzt so mal probiert.
Scheint auf den ersten Eindruck gut auszusehen, was denkst du - zumindest was du so im Quelltext siehst?

Änderungen:
Klafs_connect in deiner Version drin
NotifyFN raus und Timer ins Define gelegt.

Gruß
Alex

Beta-User

Zitat von: xasher am 23 Mai 2022, 13:14:25
Scheint auf den ersten Eindruck gut auszusehen, was denkst du - zumindest was du so im Quelltext siehst?
Um sicherzugehen, dass du den Hinweis wahrgenommen hattest:
- attr disable braucht ggf. eine Ergänzung;
- was das NotifyFn() auf sich selbst sollte, hatte ich nicht geprüft, weil eigentlich dieser Zweig nicht hätte durchlaufen werden dürfen. Es kann aber sein, dass man jetzt bei bestimmten Readings (vor dem Setzen!) schauen muss, ob es eine Änderung geben wird und dann ggf. entsprechend darauf reagieren...

Das Zugrunde liegende Problem könnte uU. auch anders gelöst werden, bitte mal den UBUS-Thread auf "ReadyFn()" durchsuchen (bzw. https://wiki.fhem.de/wiki/DevelopmentModuleIntro#X_Ready). Aber Achtung bevor du da was einbaust: Die wird uU. noch deutlich häufiger aufgerufen wie NotifyFn() - du solltest vorher wissen, wie man da einen Timeout einbaut!
Server: HP-elitedesk@Debian 12, aktuelles FHEM@ConfigDB | CUL_HM (VCCU) | MQTT2: MiLight@ESP-GW, BT@OpenMQTTGw | MySensors: seriell, v.a. 2.3.1@RS485 | ZWave | ZigBee@deCONZ | SIGNALduino | MapleCUN | RHASSPY
svn: u.a MySensors, Weekday-&RandomTimer, Twilight,  div. attrTemplate-files

xasher

Da haben sich unsere Nachrichten überschnitten mit dem disable im AttrFn

Was ich schon ab Zeile 85 habe, ist:
        if( $attrName eq "disable" ) {
          if( $cmd eq "set" and $attrVal eq "1" ) {
              RemoveInternalTimer($hash);
              readingsSingleUpdate ( $hash, "state", "disable", 1 );
              Log3 ($name, 3, "$name - disabled");
          }

          elsif( $cmd eq "del" ) {
              readingsSingleUpdate ( $hash, "state", "active", 1 );
              Log3 ($name, 3, "$name - enabled");
          }
         
        }


Wenn ich es in VALVES richtig sehe:

    if ($attrName eq 'disable') {
        RemoveInternalTimer($hash) if $cmd ne 'del';
        InternalTimer(gettimeofday(), \&VALVES_GetUpdate, $hash, 0) if $cmd eq 'del' || !$attrVal && $init_done;
        return;
    }


Dann machst du bei Disable einen RemoveInternalTimer und ein GetUpdate bei aktivem Modul bzw. wenn disable gelöscht bzw. wenn Initialisierung fertig ist.
Dann mit einem return zurück.

Für mich müsste es dann doch ähnlich aussehen?!
        if( $attrName eq "disable" ) {
          RemoveInternalTimer($hash) if $cmd ne 'del';
          InternalTimer(gettimeofday(), \&Klafs_DoUpdate, $hash, 0) if $cmd eq 'del' || !$attrVal && $init_done;
          return;
        }elsif( $attrName eq "username" ) {...


Grüße
Alex

Beta-User

 :) Sollte passen.

Anmerkungen:
- "Quotes"...? Immer doppelte eliminieren, wenn keine Evaluierung erforderlich ist und du was anfaßt...
- elsif() nach return; ?
- die "Übersetzung" ist nicht ganz korrekt:
if $cmd eq 'del' || !$attrVal && $init_done;bedeutet eher: Mache das update, wenn das Attribut gelöscht oder nach der Initialisierung auf "0" gestellt wird.
Server: HP-elitedesk@Debian 12, aktuelles FHEM@ConfigDB | CUL_HM (VCCU) | MQTT2: MiLight@ESP-GW, BT@OpenMQTTGw | MySensors: seriell, v.a. 2.3.1@RS485 | ZWave | ZigBee@deCONZ | SIGNALduino | MapleCUN | RHASSPY
svn: u.a MySensors, Weekday-&RandomTimer, Twilight,  div. attrTemplate-files

xasher

Zitat von: Beta-User am 23 Mai 2022, 14:05:50
- "Quotes"...? Immer doppelte eliminieren, wenn keine Evaluierung erforderlich ist und du was anfaßt...
Stimmt, habs natürlich geändert!

Zitat von: Beta-User am 23 Mai 2022, 14:05:50
- elsif() nach return; ?
Habs rausgenommen, kommt ja unten.

Gibt es aus deiner Sicht noch grobe Schnitzer bzw. Einwände gegen eine Veröffentlichung?

Grüße
Alex