[fixed] Dispatch-Funktion bei Signalduino u. IT V3 mit freezes

[Beta-User]

Wenn ich den Code in der dispatch Routine der fhem.pl richtig verstehe, dann müsste eigentlich die Rückgabe eines Leerstrings ausreichen.

Nach nochmaligem Blick in den Code stimme ich dem zu.

Dies kann beim Signalduino nicht passieren.
Da gibts eine Protokollliste wo jedes Protokoll eine IDnr hat und jede IDnr ist eindeutig zu einem Modul zugeordnet.

Soweit klar, was Signalduino angeht. Prinzipiell sind aber andere IO's denkbar (angefangen bei CUL), so dass das Ergebnis nicht zwingend dasselbe sein muss (vermutlich hier aber keine Abweichungen bestehen).

Was den Mechanismus an sich angeht, ist der vermutlich mit einer der ältesten Teile von FHEM. Da wäre ich mit Änderungen eher vorsichtig, auch wenn das notfalls Nacharbeit an den Client-Modulen bedeutet...

[rudolfkoenig]

Hier treffen mAn mehrere Probleme aufeinaner.

#1 Ich habe nicht damit gerechnet, dass ein Modul kaputte Messages weitergibt.
#2 Bei der erneuten Suche nach verfuegbaren Modulen werden auch bereits geladene (und damit schon befragte) Module erneut gefragt (d.h. ParseFn wird aufgerufen), und clientArray wird sinnlos geloescht.
#3 Bei vielen Clients ist computeClientArray ohne weitere Optimierung leider langsam: Im Fall von SIGNALduino bedeutet das 565 Module * 35 Clients => 19775 Pruefungen.

Zu den Loesungen dieser Probleme:
#1. Ich meine das IT Modul sollte bei kaputten Daten "" (Leerstring) zurueckliefern. Wenn irgendwer ein IT_DOKTOR Modul baut, dann kann man das IT Modul wieder zurueckdrehen.
#2. Habe ich gerade gefixt. Das sollte computeClientArray in diesen Faellen vermeiden, bleibt aber weiterhin eine sinnlose Schleife ueber 29 matchList Eintraegen, falls das IT Modul nicht geaendert wird.
#3. Module mit vielen Clients sollten ClientsKeepOrder setzen und "versprechen", dass der Clients Eintrag alle Modulnamen komplett und in der richtigen Parsefn-Aufrufreihenfolge enthaelt. Im Fall von SIGNALduino reduziert das den Erstellungsaufwand von clientArray von 19775 auf 35.

[Ralf9]

Ich habe nicht damit gerechnet, dass ein Modul kaputte Messages weitergibt.
#1. Ich meine das IT Modul sollte bei kaputten Daten "" (Leerstring) zurueckliefern

Das kommt bei vielen Clientmodulen vor, daß bei den vom IO weitergereichten Nachrichten auch kaputte dabei sind, da die Prüfung meistens erst im Clientmodul erfolgt. 
Ja das IT Modul sollte bei kaputten Daten einen (Leerstring) zurückliefern.
Bis dies auch im svn geändert ist, kann noch etwas dauern, aber dies ist ein anderes Thema (siehe unten)
Es gibt vermutlich auch noch andere Module wo es geändert werden sollte. Z.B. das 10_SOMFY Modul
https://svn.fhem.de/trac/browser/trunk/fhem/FHEM/10_SOMFY.pm#L559

#3. Module mit vielen Clients sollten ClientsKeepOrder setzen und "versprechen", dass der Clients Eintrag alle Modulnamen komplett und in der richtigen Parsefn-Aufrufreihenfolge enthaelt. Im Fall von SIGNALduino reduziert das den Erstellungsaufwand von clientArray von 19775 auf 35.

Da muß demnach dann "$hash->{ClientsKeepOrder}" definiert werden und dann müssen die match- und clientliste die selbe Reihenfolge haben.
Darf das " :" für einen Zeilenumbruch drinbleiben?

Code Auswählen Erweitern

my %matchListSIGNALduino = (
     "1:IT"               => "^i......",  
     "2:CUL_TCM97001"     => "^s[A-Fa-f0-9]+",
     "3:SD_RSL"           => "^P1#[A-Fa-f0-9]{8}",
     "5:CUL_TX"           => "^TX..........",         
     "6:SD_AS"            => "^P2#[A-Fa-f0-9]{7,8}",
     "4:OREGON"           => "^(3[8-9A-F]|[4-6][0-9A-F]|7[0-8]).*",
...

my $clientsSIGNALduino = ":IT:"
."CUL_TCM97001:"
."SD_RSL:"
."CUL_TX:"
."SD_AS:"
."OREGON:"
...
." :" # Zeilenumbruch
...



Beim Signalduino gibt's eine Protokollliste wo bei jedem Protokoll Id Eintrag das eindeutig zugeordnete Clientmodul drinsteht.
Da müsste es doch auch möglich sein vor dem Dispatch Aufruf, das der Protokoll Id zugeordnete Clientmodul in den $hash->{".clientArray"} zu schreiben, oder übersehe ich da was?
z.B.
"i400015" hat die Protooll Id 3 und in der Protokollliste steht, daß bei der ID 3 das Clientmodul "IT" verwendet wird

Code Auswählen Erweitern

wenn $dmsg = "i400015" und $m = "IT"

my @a = ();
if ($modules{$m}{LOADED}) {
  push @a, $m
}
$hash->{".clientArray"} = \@a;
Dispatch($hash, $dmsg, \%addvals);



Der maintainer für die Module CUL_TCM97001 und IT ist @bjoernh, er schaut in letzter Zeit nur recht sporatisch ins Forum rein. Sein letzter Login war am 13 April 2021
Ich hab Ihn mal per email angeschrieben (die komplette email schicke ich Euch per pm)

> Hallo Björn,
>
> bist Du in fhem nicht mehr aktiv?
>
> Ich habe Dir übers fhem Forum eine persönliche Nachricht geschrieben

prinzipiell schon.

Ich komme bloß gerade nicht dazu mich um das Modul zu kümmern.
...
Wenn Du willst, können wir auch gerne mit Rudolf bzgl. der parallelen
Pflege des Moduls antriggern.

Ich schau mal, dass ich mir das bei Gelegenheit anschaue, muss aber erst
noch einiges für den Verein aufarbeiten.

Die Frage ist ob es noch Sinn macht, daß er weiterhin der maintainer ist.

Wenn mir jemand hilft, könnte ich auch den maintainer für die Module machen

Gruß Ralf

[rudolfkoenig]

Da muß demnach dann "$hash->{ClientsKeepOrder}" definiert werden und dann müssen die match- und clientliste die selbe Reihenfolge haben.
Darf das " :" für einen Zeilenumbruch drinbleiben?

Die Reihenfolge entspricht eigentlich dem ORDER, das ist die zweistellige Zahl vor dem Client-Modulnamen. Ist aber vmtl. in deinem Fall irrelevant.
Zeilenumbruch stoert nicht wirklich, die Komplexitaet waechst von 29 auf 31, immer noch besser als 17k+.

Dein Vorschlag fuer .clientArray ist vmtl. richtig, allerdings will ich nicht versprechen, dass das in der Zukunft sich nicht mehr aendert => bitte nicht weiter optimieren.

Falls Du den Maintainer uebernehmen willst, mein OK hast Du.

[Sidey]

#1 Ich habe nicht damit gerechnet, dass ein Modul kaputte Messages weitergibt.

Du meinst damit, dass das physische Modul Daten an ein logisches Modul weitergibt, welches es eventuell nicht verarbeiten kann?

#3 Bei vielen Clients ist computeClientArray ohne weitere Optimierung leider langsam: Im Fall von SIGNALduino bedeutet das 565 Module * 35 Clients =>
19775 Pruefungen.

Ich blicke hier gerade nicht durch, wieso computeClientArray überhaupt so oft aufgerufen wird. Das Ergebnis dürfte sich für einen hash doch nie ändern, solange der clientArray nicht geändert wird.
Wie dem aber auch sei, eine simple Optimierung liegt im Vergleich 2000% schneller, wenn wir anstatt einer Regex einfach ein "eq" operator verwenden
:

Code Auswählen Erweitern

                       
if($m eq $re) {
  push @a, $m if($modules{$m}{Match});
  last;
}


regexMatch   21.3/s           --         -96%
equalCompare  502/s        2252%           --

Falls die Regex irgendwie wichtig ist, was ich nicht annehme, dann könnte man sicherlich auch deutlich schneller werden, wenn sie einmalig und nicht bei jedem durchlauf erzeugt wird, aber das sprengt hier den Rahmen, hatte ich aber schon mal an anderer Stelle angeregt.

Zu den Loesungen dieser Probleme:
#1. Ich meine das IT Modul sollte bei kaputten Daten "" (Leerstring) zurueckliefern. Wenn irgendwer ein IT_DOKTOR Modul baut, dann kann man das IT Modul wieder zurueckdrehen.

Damit wird die Verarbeitung für andere Module ja unterbunden. Bislang habe ich das so verstanden, dass ein Modul entweder den Namen der Definition zurück liefert oder ein UNDEFINED... für autocreate.
In allen anderen Fällen, muss doch ein undef geliefert werden, damit dispatch überhaupt in der Lage ist an weitere Module verteilen zu können.

#3. Module mit vielen Clients sollten ClientsKeepOrder setzen und "versprechen", dass der Clients Eintrag alle Modulnamen komplett und in der richtigen Parsefn-Aufrufreihenfolge enthaelt. Im Fall von SIGNALduino reduziert das den Erstellungsaufwand von clientArray von 19775 auf 35.

Ich habe eine grobe Ahnung was das bewirkt, bzw. dass es bewirkt dass keine Sortierung vorgenommen wird, allerdings wüsste ich nicht wie sich die "richtige" ParseFn-Aufrufreihenfolge definiert.
Eine Aufschlussreiche Dokumentation habe ich dazu leider auch nicht finden können und so bin ich mir da unsicher, was das für einen Effekt zwischen die Datenübergabe zwischen physischem und logischem Modul hat.


Grüße Sidey

[rudolfkoenig]

Du meinst damit, dass das physische Modul Daten an ein logisches Modul weitergibt, welches es eventuell nicht verarbeiten kann?

Natuerlich nicht, das soll das physische Modul doch gar nicht beurteilen koennen.
Es geht darum, dass das logische Modul (hier IT) kaputte Messages "an den Naechsten" weitergibt, und hier kein Naechster gibt.

Ich blicke hier gerade nicht durch, wieso computeClientArray überhaupt so oft aufgerufen wird.

Wegen dem Bug in Dispatch (aka #2):
- das IT Modul meint, ein anderes Modul soll das kaputte Paket bearbeiten
- Dispatch sucht daraufhin matchList nach nach diesem "anderen" Modul durch
- findet IT (=>FEHLER)
- ruft IT->ParseFn nochmal auf (jaja, weiss ich, schon gut).
- da jetzt ja ein "weiteres" Modul geladen wurde, muss clientArray neu berechnet werden.
Das habe ich gefixt.

... wenn wir anstatt einer Regex einfach ein "eq" operator verwenden

Das ist klar, leider "verspricht" die urspuengliche Client Verarbeitung ist, dass es regexp sein darf.
Wenn man ClientsKeepOrder setzt, dann sagt das Modul, es ist kein Regexp, und die Reihenfolge ist auch ok => da wird die Berechnung auch deutlich schneller durchgefuehrt.
Urspruenglich war ClientsKeepOrder fuer dynamische / per Attribut setzbare Modulreihenfolgen gedacht, passt aber hier auch perfekt.

[Beta-User]

Kurze Rückmeldung, da man mich betr. der Maintainerfrage (mit) per pm angeschrieben hatte: Ich habe da keine Aktien drin; falls jemand was zum "Einpacken" wissen will, helfe ich gerne, soweit die Kenntnisse reichen (was nicht übermäßig viel ist).

Da alle (potentiellen) CUL/Signalduino-Maintainer grade hier so einträchtig versammelt sind, vielleicht eine Bitte eher aus der User-Perspektive: Das Feld "verfügbare Versionen von firmwares und Modulen" (und deren sinnvolle Kombinationsmöglichkeiten) erscheint mir ziemlich undurchsichtig. Falls man das in diesem Zuge irgendwie (kommunikativ oder) im Code wieder was vereinfachen könnte, fände vermutlich nicht nur ich das klasse!

[Sidey]

Es geht darum, dass das logische Modul (hier IT) kaputte Messages "an den Naechsten" weitergibt, und hier kein Naechster gibt.

Da habe ich ein abweichendes Verständnis. Das IT Modul (und das Versrändniss trifft auch auf fast alle anderen logischen Module zu) gibt eine Rückmeldung an die Dispatch Funktion ob ParseFN die Meldung verarbeiten konnte.
Das logische Modul gibt somit keinerlei Daten an ein anderes logisches Modul sondern überlässt dies der Dispatch Funktion (wo es auch hingehört).
Die Funktion ist auch praktisch, weil es ja durchaus möglich ist, dass es ein anderes logisches Modul gibt, welches diese Daten verarbeiten kann.

Wenn das logische Modul zweifelsfrei feststellen kann, dass etwas nicht weiter verarbeitet werden muss, dann könnte man vielleicht ein [STOP] zurückgeben. Damit wäre dann zumindest eher erkennbar dass es hier nicht weitergeht. Die Bewertung Entscheidung in ein logisches Modul zu legen skaliert aber meines Erachtens nicht in Kombination mit dem aktuellen Maintainer Vorgehen.

Das ist klar, leider "verspricht" die urspuengliche Client Verarbeitung ist, dass es regexp sein darf.

Wäre zu einfach gewesen. Ich kann mich gerne an einen Patch setzen, der zumindest die regex so optimiert, dass diese weniger häufig compiliert werden muss. Das würde dann grundsätzlich allen zugute kommen.
Die Variante mit der Regex sollten wir vielleicht mal auf deprecated setzen. Ein Modulauthor sollte schon wissen, an welche logischen Module er was übergeben möchte und in der MatchList muss er es ja in der Regel auch hinterlegen.

Wenn man ClientsKeepOrder setzt, dann sagt das Modul, es ist kein Regexp, und die Reihenfolge ist auch ok => da wird die Berechnung auch deutlich schneller durchgefuehrt.
Urspruenglich war ClientsKeepOrder fuer dynamische / per Attribut setzbare Modulreihenfolgen gedacht, passt aber hier auch perfekt.

Bedeutet, die richtige Reihenfolge gibt das physiche Modul vor, da gibt es kein festgelegtes "richtig" oder "falsch" ?
In diesem konkreten Fall, ist das Zusätzliche Internal die effizienteste und am einfachsten umzusetzende Variante.

[rudolfkoenig]

Wenn das logische Modul zweifelsfrei feststellen kann, dass etwas nicht weiter verarbeitet werden muss, dann könnte man vielleicht ein [STOP] zurückgeben.

GIbts schon, und ist ein Leerstring.

[Sidey]

GIbts schon, und ist ein Leerstring.

Das hatte ich ehrlich gesagt durchaus schon gesehen aber eigentlich immer als "Fehlerhaft" angesehen.

PS: Also mit einer Codezeile mehr könnten wir so die 700% beschleunigen.

[Ralf9]

Mir ist nicht klar welche Auswirkungen es hat, wenn die Reihenfolge vom $hash->{Clients} nicht passt.

Das ist wahrscheinlich erst interessant, wenn es in der matchList für eine $dmsg mehrere Clientmodule gibt die matchen können, dann sollte die clientlist die gleiche Reihenfolge haben wie die matchlist.
Wenn es aber wie beim Signalduino für jede $dmsg in der matchlist nur einen Eintrag gibt bei dem die regex match, dürfte die Reihenfolge in der Clientliste eigentlich egal sein.
Es liese sich wahrscheinlich noch ein klein wenig optimieren, wenn in der Clientlist die Module für Sensoren weiter vorne stehen und die Module für Funkrolladen und Funksteckdosen weiter hinten.

[Ralf9]

Ich habs mir nochmals angeschaut, demnach wird ohne ClientsKeepOrder in der sub computeClientArray das .clientArray alphabetisch sortiert.
Demnach ist die Parsefn-Aufrufreihenfolge die alphabetische Sortierung.

#3. Module mit vielen Clients sollten ClientsKeepOrder setzen und "versprechen", dass der Clients Eintrag alle Modulnamen komplett und in der richtigen Parsefn-Aufrufreihenfolge enthaelt

Mir ist nicht klar warum das so wichtig ist, daß in der $hash->{Clients} die Einträge alphabetisch sortiert sind.


Da beim Dispatch die matchlist alphabetisch sortiert wird, ist es wahrscheinlich besser bei der Nummerierung 1-9 eine 0 davor zu schreiben

Anstatt so

Code Auswählen Erweitern

my %matchListSIGNALduino = (
     "1:IT"         
     "2:CUL_TCM97001"
     "3:SD_RSL"
     "5:CUL_TX"     
     "6:SD_AS"     
     "4:OREGON"     
     "7:Hideki"
     "9:CUL_FHTTK"
     "10:SD_WS07"
     "11:SD_WS09"

dann so

Code Auswählen Erweitern

my %matchListSIGNALduino = (
     "01:IT"         
     "02:CUL_TCM97001"
     "03:SD_RSL"
     "05:CUL_TX"     
     "06:SD_AS"     
     "04:OREGON"     
     "07:Hideki"
     "09:CUL_FHTTK"
     "10:SD_WS07"
     "11:SD_WS09"

[rudolfkoenig]

Die Reihenfolge geht nach ORDER, das ist die zweistellige Nummer vor dem Modulnamen. Ist de facto irrelevant.

Wenn Signalduino in der Client Liste alle Module vollständig benennt, dann reicht den o.g. ClientsKeepOrder zu setzen.

[Sidey]

Wenn Signalduino in der Client Liste alle Module vollständig benennt, dann reicht den o.g. ClientsKeepOrder zu setzen.

Die sind alle gesetzt. Man könnte die Clientliste vor dem Dispatch Aufruf sogar auf die relevanten reduzieren.


Bist Du an einer CPU Optimierten computeClientArray interessiert?

[rudolfkoenig]

Man könnte die Clientliste vor dem Dispatch Aufruf sogar auf die relevanten reduzieren.

Das ist nicht sinnvoll, da die Liste nur nach Laden neuer Module berechnet wird. Aus dem Gleichen Grund ist eine Optimierung nur begrenzt wichtig. Aber wenn es nich wesentlich länger oder unleserlich wird: warum nicht.