[Refactoring] MySensors-Module

[Beta-User]

Hallo zusammen,

anbei mal ein Satz aktualisierter MySensors-Module. Habe die auch auf meinem Hauptsystem zum Testen laufen, weiß aber noch nicht, ob das alles stressfrei ist, weil doch einiges umgemodelt wurde.

Es ist einiges an perlcritic-Anregungen drin verarbeitet, aber ob das schon Bronzezeit ist...?

Was auch ging, war die "überflüssigen" Teile aus dem list zu verbannen, sehr schön!

Die verbliebenen perlcritic-Themen werden wohl etwas länger dauern, da ich die Anmerkungen zum Teil (noch) nicht verstehe und erst etwas Zeit (und Lust...) brauchen werde, um mich da einzudenken, was jeweils gemeint sein könnte.

Manche "Merkwürdigkeiten" - v.a. rund um Constants.pm - habe ich nicht wegbekommen, insbesondere geht es nicht so einfach, den EXPORT-Teil einfach nach vorne zu ziehen oder die Doppelungen in EXPORT_OK rauszunehmen... Im Moment habe ich noch keine Ahnung, wieso das so ist.

Zumindest habe ich die "Initialize"-Geschichte bei "Device" jetzt auch im Griff . Immer noch schlammig, aber hoffentlich mit dem wenigsten "Drumrum"...

Ich poste das vorrangig, damit die MySensors-Leute einen Ort haben, um die zu testenden Versionen zu bekommen, aber wenn wer mag, kann er gerne Kommentare abgeben oder Vorschläge zum "Entzerren" der "high complexity"-Codeteile liefern.

Grüße,
Beta-User

Nachtrag:
Anbei nur noch 00_MYSENSORS.pm und 10_MYSENSORS_DEVICE.pm, die beiden anderen braucht man seit der Fassung vom 09.05.2020 nicht mehr!

[zap]

Frage: welchen Vorteil hat

Code Auswählen Erweitern

$command eq ... and do {

gegenüber dem klassischen if elsif Konstrukt?

[RichardCZ]

Frage: welchen Vorteil hat

Code Auswählen Erweitern

$command eq ... and do {

gegenüber dem klassischen if elsif Konstrukt?

gar keinen.

           $key eq "altitude" and do {
...
            };

in Massen z.B. in 95_Astro

->

if ($key eq 'altitude') {
...
}

Da wollte jemand einfach nur mit logical shortcuts experimentieren.

[Beta-User]

Nun ja, war halt da und hat funktioniert...
perlcritic mosert auch nicht - im Gegensatz zu der elsif-Kaskade, die ich an anderer Stelle eingebaut habe. Die sollte man aber auch irgendwie eleganter als "nur" mit "if" lösen können...

[RichardCZ]

Der Punkt ist ja, dass "and do" weniger elegant ist als ein if.
Es ist ja äquivalent zu einer Ansammlung von ifs - keine if-elsif. Wenn man das also ersetzt, meckert perlcritic da auch nicht.

Logische Shortcuts sind eine tolle Sache - in Ausdrücken bei denen ein Wahrheitswert rauskullern soll:

if (mordsfunc(x) && $fast == 1) {...

kann man u.U. erheblich beschleunigen (wenn z.B. $fast == 1 oft nicht zutrifft), indem man die Sache einfach umdreht:

if ($fast == 1 && mordsfunc(x)) {...

dann wird mordsfunc(x) nämlich gar nicht ausgeführt.

Auch in if-elsif Ketten, oder tabular Ternaries... sollte man die wahrscheinlicheren und leicht abzufragenden Fälle
als erstes abhandeln. Im Notfall hilft ein Profiler oder einfache Prints.

[Sidey]

If else Schlangen lassen sich teilweise ganz gut mit Dispatch Tables ablösen.

Geht einfach, wenn es nur eine Variable zum Entscheiden gibt. Hat man mehrere kann man ggf. noch geschickt kombinieren oder mehrere Einträge auf den gleichen Code verweisen lassen.


Grüße Sidey

[Beta-User]

So,
zumindest ein kleiner weiterer update in der Sache...

Ein paar der "COMMAND_HANDLER"-Geschichten sind eliminiert, auch wenn ich "weniger elegant" jetzt nicht unbedingt als "dringenden Handlungsbedarf" übersetzen würde (?).

Etwas "Bauchweh" hatte ich mit den Block-grep-Modifikationen in Zeilen 191/194, aber zumindest ist mir das Testsystem nicht abgeschmiert beim rereadcfg... Scheint also zu passen. Damit wären wir mit überschaubarem Morphiumpflastereinsatz bei diesem Teilstück @perlcritic-level -3 .

If else Schlangen lassen sich teilweise ganz gut mit Dispatch Tables ablösen.

Klingt interessant, aber irgendwie habe ich Schwierigkeiten, das Stichwort, dieses Fragment

So bearbeitet, kann man das eventuell später noch weiter zu

Code Auswählen Erweitern

my %dispatch = (
    'ThermostatState' => \&_handleDies,
    'WallThermostat' => \&_handleDas,
...
);


und den Code z.B. in diesen Zeilen gedanklich zu einer spontanen Vollsynthese mit

Code Auswählen Erweitern

      MESSAGE_TYPE: {
        $type == C_PRESENTATION and do {
          onPresentationMsg($hash,$msg);
          last;
        };
        $type == C_SET and do {
          onSetMsg($hash,$msg);
          last;
        };
        $type == C_REQ and do {
          onRequestMsg($hash,$msg);
          last;
        };
        $type == C_INTERNAL and do {
          onInternalMsg($hash,$msg);
          last;
        };
        $type == C_STREAM and do {
          onStreamMsg($hash,$msg);
          last;
        };
      }
zu bewegen.

Sollte das am Ende einfach nur so sein?!?:

Code Auswählen Erweitern

my %dispatch = (
    'C_PRESENTATION' => \&onPresentationMsg($hash,$msg),
    'C_SET'              => \&onSetMsg($hash,$msg),
    'C_REQ'             => \&onRequestMsg($hash,$msg),
    'C_INTERNAL' => \&onInternalMsg($hash,$msg),
    'C_STREAM'   => \&onStreamMsg($hash,$msg),
);


(Aber wie dann abschubsen? Das ist doch erst mal ein "einfacher Hash", man müßte also erst "$dispatch{$type}" abfragen, bevor da was zurückkommt? Sorry, immer noch DAM).

Grüße,

Beta-User

[Sidey]

Hi Beta-User,

Du hast das mit den Dispatch Tables verstanden.
Ich habe das im 00_Signalduino schon so für get und set umgesetzt, weil mir das doch zu unübersichtlich wurde.

Die Referenz auf die Sub rufst du dann so auf:

Code Auswählen Erweitern

$dispatch{$Action}->($hash,$msg);

Hier auch ein paar Beispiele.
https://www.perlmonks.org/?node_id=456530

Grüße Sidey

[RichardCZ]

Sollte das am Ende einfach nur so sein?!?:

Code Auswählen Erweitern

my %dispatch = (
    'C_PRESENTATION' => \&onPresentationMsg($hash,$msg),
    'C_SET'           => \&onSetMsg($hash,$msg),
    'C_REQ'             => \&onRequestMsg($hash,$msg),
    'C_INTERNAL' => \&onInternalMsg($hash,$msg),
    'C_STREAM'   => \&onStreamMsg($hash,$msg),
);

Aber wie dann abschubsen?

Sidey hat eigentlich schon alle Zutaten beschrieben. Wenn Du es konkret willst,
ich würde es so schreiben:

Code Auswählen Erweitern

my $dispatch = {
    C_PRESENTATION => \&onPresentationMsg,
    C_SET          => \&onSetMsg,
    C_REQ          => \&onRequestMsg,
    C_INTERNAL     => \&onInternalMsg,
    C_STREAM       => \&onStreamMsg,
};

$dispatch->{$type}->($hash, $msg);


[Wzut]

Ich habe das im 00_Signalduino schon so für get und set umgesetzt

na also meine Gebete wurde erhört, gleich mal abschreiben gehen.....

[Beta-User]

Ähm, also irgendwie habe ich da jetzt so ziemlich alle Varianten durchgespielt...

Am "längsten lebt" Richards Variante, aber die zieht dann nach Eingang der ersten von einer Node gesendeten Meldung (vermutlich heartbeat=C_INTERNAL)  FHEM komplett in den Abgrund mit einem freundlichen Hinweis auf "Can't use an undefined value as a subroutine reference at ..." (Verweis auf die Zeile mit "$dispatch->{$type}->($hash, $msg);")
Die meisten anderen Kombinationen von %/$, Quotes, Klammern und -> verhindern das Starten der Module... Jetzt habe ich grade keine Lust mehr, dem weiter auf den Grund zu gehen (das Testen mache ich mit dem Hauptsystem) und kann auch einen Typo nicht ausschließen.

[RichardCZ]

Ähm, also irgendwie habe ich da jetzt so ziemlich alle Varianten durchgespielt...

Am "längsten lebt" Richards Variante, aber die zieht dann nach Eingang der ersten von einer Node gesendeten Meldung (vermutlich heartbeat=C_INTERNAL)  FHEM komplett in den Abgrund mit einem freundlichen Hinweis auf "Can't use an undefined value as a subroutine reference at ..." (Verweis auf die Zeile mit "$dispatch->{$type}->($hash, $msg);")
Die meisten anderen Kombinationen von %/$, Quotes, Klammern und -> verhindern das Starten der Module... Jetzt habe ich grade keine Lust mehr, dem weiter auf den Grund zu gehen (das Testen mache ich mit dem Hauptsystem) und kann auch einen Typo nicht ausschließen.

Wie wäre es mal den Code zu zeigen? Ich habe in solchen Fällen auch keine Lust auf Wahrsagerei.

Fehlermeldung besagt eigentlich nur, dass  $dispatch->{$type} undefiniert ist. M.a.W. Du bekommst einen $type rein, der nicht im Hash drin ist.

Code Auswählen Erweitern

ref $dispatch->{$type} eq 'CODE' ? $dispatch->{$type}->($hash, $msg) : Log("Irgendjemand versucht mir >$type< reinzudrücken");


[Sidey]

Du kannst auch einfach den commit in dein Repo hauen.
Dort kann man auch konkret Hilfestellung geben

[Beta-User]

Bitte etwas Geduld, ich hatte gestern nur keine Lust/Reserven mehr, das vollständige Bild zu liefern, und testen kann man mMn. nur, wenn man Hardware hat (oder das irgendwie aufwändig simmuliert).

(Der Code ist im wesentlichen aber genau das, was hier im ersten Beitrag verfügbar ist, ich habe dann gestern nur die 9 Zeilen von Richard wieder gegen die ursprüngliche MESSAGE_TYPE-Variante rückgetauscht...)

Bei Gelegenheit werde ich mal den Vorschlag mit dem Vorabtest auf den richtigen ref einbauen. Ist irgendwie seltsam, denn lt. Doku unter https://www.mysensors.org/download/serial_api_20#command sollte es wirklich nur diese $type geben; kann aber natürlich sein, dass da sehr viel mehr garbage über die Serielle Schnittstelle reinkommt, als ich bisher dachte...

Fortsetzung folgt .

(Und der Tipp mit dem Test war in etwa der Schubs, den ich brauchte, btw.. Mehr Kaffeesatzleserei war gar nicht meine Intention, es ging eher darum, rechtzeitig zu melden, dass "Houston" zu informieren ist, bevor Wzut da von "einfacher Gebetserhörung" ausgeht).

[Wzut]

bevor Wzut da von "einfacher Gebetserhörung" ausgeht

Was , wie ? also ich bin glücklich, bei mir läuft zum einen die "einfache" Variante von Richard also auch die Luxusversion %get / %set von Sidey aus 00_SIGNALduino
D.h. ich kann nun von Fall zu Fall und je nach Modul unterscheiden ob die einfache Version reicht oder ob es die aufgebohrte sein muß.