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

Beta-User

Na ja, ich sehe das Modul nicht in Aktion...

Auf die ärgsten Themen seit der ersten Version sind wir m.E. eingegangen.

Prinzipiell würde ich mal noch empfehlen,
- die "sehr repetitiven" Codestellen anzusehen, ob sich da nicht manches vereinfachen läßt? (20x am Stück ReadingsVal-Wertezuweisungen mit anschließender Concatenation?) Richt nach (vorläufiges! Stichwort) "encode_json" (und die Werte vorab per Schleife in einen Hash packen);
- decode_json und encode_json enthalten eine Transformation nach utf8. Kann sein, dass du das hier brauchst, aber wenn nicht, könntest du das noch auf use JSON() umstellen und z.B. sowas hier verwenden:
if ( !eval { $decoded  = JSON->new->decode($content) ; 1 } ) {
Siehe dazu auch den "Unicode first aid"-Thread in Development.
- Die Reading-Inhalte ("3 Tage") sind teilweise auch eingedeutscht. Ich bin ein Fan von Internationalisierung... Könnte man hier mit einem "verschachtelten Hash" lösen und dabei die language-Einstellungen in "global" beachten.

- Auch das "elsif"-Gedöns bei der Sprache wäre einfacher in einem Hash untergebracht.

Nun ja, und einpacken wäre nach wie vor nicht verkehrt... Aber Rom und so ::) .
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,

Sprache und Standby-Time habe ich noch vereinfacht.
Das Modul ist eingecheckt - sollte Morgen in der Verteilung drin sind.

Dir ganz herzlichen Dank für deine Zeit. Das Modul ist nun so viel besser als es ursprünglich war.

Grüße
Alex

Beta-User

 :) Danke für deine Rückmeldung.

Ich werd' vermutlich nochmal über den Code schauen, eine Sache ist mir aber auf die Schnelle aufgefallen: In der "en"-Commandref hatte ich schon "id="-Anker verteilt. Die sind aus irgendwelchen Gründen wieder rausgefallen...

Das solltest du bei Gelegenheit insgesamt umstellen.
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 zusammen,

ich muss mein Modul 70_Klafs.pm nochmals in den Review geben. Ich habe folgende Fehlersituation.

Wenn ich mein Modul definiere:
define mySauna KLAFS

Dann funktioniert meine FritzBox Verbindung nicht mehr. Die Fritzbox Config kommt nach der Saunadefinition in fhem.cfg (Vielleicht ist das relevant)

Es liegt an Zeile 37
use FHEM::Core::Authentication::Passwords qw(:ALL);
und Zeile 120
$hash->{helper}->{passObj} = FHEM::Core::Authentication::Passwords->new($hash->{TYPE});

Ich verwende die Passwortroute von CoolTux, aber dann geht die Fritzbox nicht mehr. Es kommt ständig Checking API

Da mache ich also etwas falsch. Könnt ihr mir helfen?

Besten Dank und viele Grüße
Alex

Beta-User

Weiß nicht recht, eigentlich dürfte das nicht die tiefere Ursache sein.

Wo ich aber auf alle Fälle Probleme sehe: Du erzeugst Events beim Starten von FHEM (in define ohne init_done-Prüfung), und liest das Passwort aus dem $hash, der ggf. zu diesem Zeitpunkt noch gar nicht angelegt ist (aber in $type stehen würde). Soweit ich mich entsinne, ist die Empfehlung von CoolTux, $name zu verwenden, und dann eine RenameFn() für den Fall eines rename vorzusehen.

Die BlockingGet sind uu. auch zu überdenken.

PS: Der Rest meiner Hinweise scheint nicht so sehr auf fruchtbaren Boden gefallen zu sein, angefangen von dem id-Thema für die Commandref...
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. Also, das mit deinen Hinweisen und fruchtbaren Boden soll nicht meine Undankbarkeit widerspiegeln. Es hat auch oft damit zu tun, dass ich es nicht richtig verstehe und man vermutlich immer weiter optimieren kann.

Aber das Problem kommt definitiv in Verbindung mit dem Modul von CoolTux und meinem Modul. Ich finde das Problem müsste man als erstes beheben.
Vorneweg, es liegt nicht an CoolTux.

Hintergrund, dass das Fritzbox Modul nicht funktioniert kommt schon bei der Einbindung hier:
use FHEM::Core::Authentication::Passwords qw(:ALL);

Das CoolTux Hilfmodul verwenden noch zwei andere Module in FHEM. Ich habe mir das SoftliqCloud-Modul angeschaut, da ich ebenfalls den Wasserenthärter besitze.

Bei beiden Packagen folgt eine Packagedefinition


package FHEM::Gruenbeck::SoftliqCloud;

oder
package FHEM::UBUS_CLIENT; ## no critic "Package declaration"

Wenn ich bei mir so ein Eintrage mache
package FHEM::Klafs;

funktioniert ebenfalls das Modul der Fritzbox wieder. Aber es kommt dann zu Folgefehler bei mir im Modul.

Da kenne ich mich definitiv derzeit nicht aus. Liegt es daran, dass dann das Modul - wie sagt man - zum Package wird?

Viele Grüße
Alex

Beta-User

Ja, damit nimmst du das Modul aus dem "main"-Namespace. An sich finde ich das sehr sinnvoll, du musst dann halt das "drumrum" auch sauber machen und v.a. die aus fhme.pl etc. erforderlichen Funktionen ebenfalls importieren.

Im Moment dürfte das Modul entweder schon nicht eingebunden werden (initialize kann nicht adressiert werden), oder es gibt andere "undefined subroutines".

Ob das Passwort-Helferlein auch für ungepackagte Module geht, kann ich nicht sagen.

Der Übergang ist gar nicht soooo schwer, siehe z.B. als aktuelles Beispiel:
https://svn.fhem.de/trac/changeset/25877/

Aktuell meine ich, dass es für dich vielleicht sinnvoll wäre, mal in die TvHeadend-reloaded zu schauen, das ist mAn. relativ ähnlich wie das, was du hier machst.
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 zusammnen,

ich habe am Wochenende das verlinkte Beispiel angesehen und auch TVHeadend.pm. Ich möchte niemand nerven, komm aber an einer Stelle nicht weiter. Vielleicht hilft ein kleiner Fingerzeig, damit es weitergeht...

Was ich inzwischen gemacht habe

Packagedefinition eingefügt:
package FHEM::Klafs;

Die GPUtils importiert:
use GPUtils qw(:all);

Die Namen wie z.B. Klafs_Initialize in Initalize umbenannt und die benötigten Funktionen importiert:
BEGIN {

  GP_Import(qw(
    readingsBeginUpdate
    readingsBulkUpdate
    readingsEndUpdate
    readingsSingleUpdate
    Log3
    defs
    init_done
    InternalTimer
    strftime
    RemoveInternalTimer
    readingFnAttributes
    AttrVal
    notifyRegexpChanged
    ReadingsVal
    HttpUtils_NonblockingGet
    HttpUtils_BlockingGet
  ))
};


Beim Start kommt noch ein Killeraufruf:
Undefined subroutine &main::Klafs called at fhem.pl line 3486.

Ich tu mir da schwer, da der Fehler nicht auf mein Modul referenziert. Kann mir nochmals jemand eine Herangehensweise zeigen, wo ich genau suchen muss. Wo hängt es im Moment?

Ich hänge das Modul mal im aktuellen Stand an.

VG
Alex

Beta-User

#23
Die Zeile gehört in den Kontext InternalTimer().

Aus dem Modul kann ich das auch nicht direkt ablesen, würde aber empfehlen, alle InternalTimer-Aufrufe auf Referenzierungen umzustellen:

War:
InternalTimer(gettimeofday(), 'FHEM::Klafs::Klafs_DoUpdate', $hash, 0) if $cmd eq 'del' || !$attrVal && $init_done;
Soll:
InternalTimer(gettimeofday(), \&Klafs_DoUpdate, $hash, 0) if $cmd eq 'del' || !$attrVal && $init_done;

(Da waren auch noch ein paar Aufrufe ohne Package-Adresse, die allerdings m.E. nicht für den Absturz verantwortlich sind).

EDIT: das ist noch ein InternalTimer, der auf $self referenziert. Der dürfte es sein (die se caller-Analysen kannst du m.E. rauswerfen, sehe keinen Sinn darin).
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

Ja, genau das mit $self wars.

Aktueller Stand mit dem Modul. Es läuft im Namespace von Fhem. Fritzboxmodul läuft jetzt auch wieder.
ID in der Hilfe ist auch vorhanden.

Augrund der Schwere des Fehlers würde ich morgen mal das vorhandene Modul in Fhem ersetzen. Ich teste es heute noch.
Dann ist es auf jeden Fall mal besser.

Besten Dank dir für deine Zeit.

VG
Alex

Beta-User

Ähm, habe grade gesehen, dass die neue Fassung mit package main; beginnt, und dann in diesem Kontext use strikt/warnings gesetzt wird... Das sollte nicht so bleiben ;) .
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 Mühe. Wie gesagt, ich habe mich auch am Modul 69_SoftliqCloud.pm orientiert...
Ich habe mir daher nochmals TV-Headend Reloaded runtergeladen und auch das package main; gestrichen und package FHEM::Klafs; an den Anfang gesetzt.

Viele Grüße
Alex

Beta-User

Danke für's Aufgreifen.

Den Rest habe ich mir nicht angesehen, aber der angesprochene Teil paßt, warnings&Co sind wieder aktiv :) .
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