70_Klafs.pm Code Review

[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:

Code: [Auswählen]

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 .

[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.

[xasher]

Hallo zusammen,

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

Wenn ich mein Modul definiere:

Code: [Auswählen]

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

Code: [Auswählen]

use FHEM::Core::Authentication::Passwords qw(:ALL);und Zeile 120

Code: [Auswählen]

$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...

[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:

Code: [Auswählen]

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

Code: [Auswählen]

package FHEM::Gruenbeck::SoftliqCloud;
oder

Code: [Auswählen]

package FHEM::UBUS_CLIENT; ## no critic "Package declaration"
Wenn ich bei mir so ein Eintrage mache

Code: [Auswählen]

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.

[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:

Code: [Auswählen]

package FHEM::Klafs;
Die GPUtils importiert:

Code: [Auswählen]

use GPUtils qw(:all);
Die Namen wie z.B. Klafs_Initialize in Initalize umbenannt und die benötigten Funktionen importiert:

Code: [Auswählen]

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:

Code: [Auswählen]

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]

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:

Code: [Auswählen]

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

Code: [Auswählen]

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).

[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 .

[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 .