10_ZWave.pm aufräumen?

Begonnen von A.Harrenberg, 17 Juli 2016, 16:03:55

Vorheriges Thema - Nächstes Thema

A.Harrenberg

Hallo Rudi,

da ich mich ja nun schon eine ganze Weile mit der Datei auseinandersetze weiß ich zwar mittlerweile recht gut "Bescheid" wo was ist, ingesamt merkt man der Datei aber an das sie historisch gewachsen ist und das dort mehrere "Köche" drin gerührt haben. Ich selbst bin da auch nicht das beste Beispiel für Konsistenz, oder sogar das Paradebeispiel von Inkosistenz, meine Funktionen sehen auch alle verschieden aus ,-(

Bei der ALARM Klasse habe ich jetzt zumindest mal versucht die Namen der Funktionen einfach nur aus der Klasse und dem hexcode des Befehls abzuleiten und die Funktionen dann entsprechend sortiert anzulegen. Ich finde das recht übersichtlich und man erkennt die zusammengehörigkeit der Funktionen zur jeweiligen Klasse eben sehr gut.

Insgesamt gibt es ja verschiedenste Arten von Funktionen in der Datei:

  • Implementierungsfunktionen für Klassen (get/set/report)
  • Hilfsfunktionen für die jeweiligen Klassen die als Unterfunktionen aufgerufen werden (z.B. die eigenltiche Verschlüsselung bei Security)
  • Controllerfunktionen
  • Funktionen zum Senden mit Sendstack
  • Funktionen zum allgemeinen Parsen von Antworten
  • ...
DIese Funktionen sind momentan mMn etwas willkürlich verstreut und namentlich auch nicht immer so eindeutig.

Ich würde hier gerne versuchen ein wenig "aufzuräumen"...
Als einem ersten Schritt würde ich das "Sortieren" der Funktionen nach Klassenfunktionen, Controllerfunktionen, Sende/Empfangsfunktionen etc. anstreben, dann das
Umbenennen der Klassenfunktionen im Stil der ALARM Klasse. In diesem Zuge könnte man dann auch mal etwas genauer schauen welche Versionen von den einzelnen Klassen eigentlich unterstützt werden und dies in der Wiki nachtragen bzw. das in der %zwave_classVersion berücksichtigen falls nötig. An die Controllerfunktionalitäten oder das Senden/Empfangen würde ich im ersten Schritt mal nicht rangehen wollen...

Hälst Du das für sinnvoll?

Gruß,
Andreas.
FB 7360, Homematic und ZWave
Support for ZWave-SECURITY

rudolfkoenig

Wenn Du Spass daran hast, kannst du es gerne machen.
Ich bitte dich aber, die Patches stueckweise zu machen. Also erst Umorganization, dann Umbenennung.

A.Harrenberg

Hallo Rudi,

anbei mal der Patch für eine erte Aufräumaktion.

Ich würde da später eventuell in einem zweiten Schritt noch versuchen noch weiter gruppieren, das wird mir aber jetzt zu viel auf einmal...
Funktional habe ich (hoffentlich) nichts geändert, ich habe mich aber an %zwave_classes (und ein paar anderen Variable) vergriffen und dort die Klammerung/Einrückung mal einheitlich gemacht.

Mir ist auch eine Sub (ZWave_getPic) aufgefallen die anscheinend nirgends aufgerufen wird, habe sie erst mal auskommentiert.

Gruß,
Andreas.
FB 7360, Homematic und ZWave
Support for ZWave-SECURITY

krikan

Zitat von: A.Harrenberg am 24 Juli 2016, 08:47:25
Mir ist auch eine Sub (ZWave_getPic) aufgefallen die anscheinend nirgends aufgerufen wird, habe sie erst mal auskommentiert.
Hallo Andreas!
Wird mMn in 00_ZWDongle.pm (Zeile 181) aufgerufen.
Gruß, Christian

A.Harrenberg

Hi,

Ja, Du hast Recht. Hatte nicht daran gedacht das hier auch "zwischen" den Modulen zugegriffen wird.
Wobei man in dem Fall dann überlegen kann ob die Sub dann nicht besser in das andere Modul geschoben werden sollte.

Gruß,
Andreas.
FB 7360, Homematic und ZWave
Support for ZWave-SECURITY

rudolfkoenig

Hallo Andreas,


ich fuerchte, das wird noch 2 Wochen dauern, da ich unterwegs bin, und mit den mobilen Geraeten Patches einzuspielen nicht so einfach funktioniert, wie ich es geplant habe. ZWave_getPic greift auf interne Modulldaten zu, wird aber im ZWDongle fuer die neue Neighborlist-Anzeige mit Bildern benoetigt.


Gruss, Rudi

A.Harrenberg

Hi Rudi,
Zitat von: rudolfkoenig am 24 Juli 2016, 14:38:06
ich fuerchte, das wird noch 2 Wochen dauern, da ich unterwegs bin, und mit den mobilen Geraeten Patches einzuspielen nicht so einfach funktioniert, wie ich es geplant habe. ZWave_getPic greift auf interne Modulldaten zu, wird aber im ZWDongle fuer die neue Neighborlist-Anzeige mit Bildern benoetigt.
ok, dann mache ich da erst mal nicht weiter (und die ZWave_getPic bleibt dann auch im Modul) und warte.

Dann schaue ich mir noch mal SECURITY an, da wollte ich auch noch was einbauen um zu verhindern das Befehle auf dem Security-Stack liegen bleiben wenn Nonce bzw Nonce-Request verlorengehen. Und das Thema Firmwareupdate gibt es ja auch noch... ,-)

Gruß,
Andreas.
FB 7360, Homematic und ZWave
Support for ZWave-SECURITY

rudolfkoenig

Hallo Andreas,

dieser Patch ist sehr gross (190k), hat in etwa die Groesse der Originaldatei, d.h. im Schnitt wurde jede zweite Zeile verschoben oder modifiziert. Ich habe damit zwei Probleme:
- ich kann bei dieser Groesse nicht mehr pruefen, ob es Seiteneffekte hat.
- svn blame kann man ab sofort fuer die alten Sachen nur mit einem deutlichen Mehraufwand verwenden: erst neue Version mit svn blame pruefen, falls als Aenderung diese Umformatierung angezeigt wird (was fuer 50% der Zeilen zutrifft), dann die alte Version auschecken, und svn blame darauf starten.

Mir ist die Lage der einzelnen Funktionen egal: die Datei ist zu gross, um alles auf einmal zu sehen, und dem Editor ist bei der Suche egal, wo die Funktionen sind. Auch die Struktur-Umformatierung ist mir (angesicht der Nachteile) nicht so wichtig: "richtig schoen" kriegt man es eh nicht hin, entweder zu viel Platz verschenkt, oder unschoen umgebrochen.

Demgegenueber steht, dass Du dich mit diesem Code besser fuehlst, und das will ich nicht ignorieren.

Deswegen, bevor ich es einchecke:
- wurde hier ausser Umformatieren / umsortieren was gemacht?
- ist dir der Mehraufwand beim svn blame das Umsortieren/Umformatieren Wert?

Gruss,
  Rudi

A.Harrenberg

Hallo Rudi,
Zitat von: rudolfkoenig am 10 August 2016, 20:59:18
Demgegenueber steht, dass Du dich mit diesem Code besser fuehlst, und das will ich nicht ignorieren.

Deswegen, bevor ich es einchecke:
- wurde hier ausser Umformatieren / umsortieren was gemacht?
- ist dir der Mehraufwand beim svn blame das Umsortieren/Umformatieren Wert?
also funktional habe ich in der Version nichts (bewusst) geändert.
Mit svn blame habe ich noch nie gearbeitet, sagt mir also erst einmal nichts, ich schaue mir nachher mal an was das ist und wovon Du da eigentlich redest... ,-) Den Aufwand kann ich also nicht einschätzen.
So wie ich das lese wäre es Dir lieber es bliebe wie es ist...

Was wäre denn mit folgender Vorgehensweise:
- Die Umformatierung für %zwave_classes wird übernommen, der Rest bleibt erst einmal so wie bisher
Ich finde das so doch etwas aufgeräumter und durch die "extra Kommas" macht man beim Einfügen von neuen Zeilen weniger Fehler.

Class-Funktionen würde ich dann nach-und-nach umsortieren/umbenennen wenn ich neue Funktionen in der jeweiligen Klasse implementiere.

Würde das den Aufwand für svn blame in Grenzen halten oder kame das am Ende auf das gleiche raus? (habe wie gesagt noch nie damit gearbeitet...)

Gruß,
Andreas.
FB 7360, Homematic und ZWave
Support for ZWave-SECURITY

A.Harrenberg

Hallo Rudi,

habe jetzt mal ganz rudimentär geschaut was "svn blame" macht. Problem ist ja wohl das es sich an Zeilennummern orientiert und bei so einer Verschiebeaktion dann Amok läuft...
Ich verstehe Deinen Einwand dahingehend jetzt und kann nachvollziehen das damit die "Nachverfolgbarkeit" der Änderungen quasi unterbrochen wird.

Sieht für mich jetzt irgendwie nach einer "Loose-Loose" Situation aus ,-(

Damit sind dann aber stückweise Verschiebeaktionen auch nicht besser, nach einer Weile müsste das dann ja für "blame" genauso enden oder sehe ich das falsch.
Hmpf, nun weiß ich auch nicht weiter.

Gruß,
Andreas.

FB 7360, Homematic und ZWave
Support for ZWave-SECURITY

rudolfkoenig

svn blame ist schon praktisch, habe es gestern auch verwenden muessen, siehe https://forum.fhem.de/index.php/topic,56495.msg480446.html#msg480446

ZitatSieht für mich jetzt irgendwie nach einer "Loose-Loose" Situation aus ,-(
In der Tat, deswegen wollte ich wissen, wie wichtig fuer dich das Umformatieren ist. Leider ist mir das Problem mit svn blame erst bei der Ansicht der Aenderungen eingefallen, das muss ich mir fuer die Zukunft merken. Ich stehe immer noch zu der Aussage: falls du meinst, es ist Wert, dann checke ich es ein.
Fuer nachtraeglich Jammern bin ich nicht bekannt :)

A.Harrenberg

Hallo Rudi,
Zitat von: rudolfkoenig am 12 August 2016, 08:18:16
svn blame ist schon praktisch, habe es gestern auch verwenden muessen, siehe https://forum.fhem.de/index.php
sehe ich auch so...

Zitat von: rudolfkoenig am 12 August 2016, 08:18:16
Ich stehe immer noch zu der Aussage: falls du meinst, es ist Wert, dann checke ich es ein.
Fuer nachtraeglich Jammern bin ich nicht bekannt :)
Darüber muss ich noch mit mir selbst diskutieren. ;-) Momentan bin ich da völlig unentschlossen.

Gruß,
Andreas.

FB 7360, Homematic und ZWave
Support for ZWave-SECURITY

A.Harrenberg

Hallo Rudi,

das ist eine schwierige Entscheidung...

Ich habe noch ein wenig weiter nach "Blame" (oder besser "Praise") gegoogelt... Umformatieren und Umsortieren bringt das auf jeden Fall durcheinander, da führt kein Weg dran vorbei. ;-(

a.) die Änderungen an %zwave_classes finde ich zwar übersichtlicher, sind aber nicht wirklich wichtig -> Vorschlag: NICHT ändern
b.) das Sortieren der modul-internen Funktionen ist schwierig und in meinem Patch auch noch nicht wirklich final/fertig -> Vorschlag: NICHT ändern

c.) die ZWave-Klassenbefehle würde ich aber schon gerne geordnet am Stück haben, das macht es einfacher die Klassen zu erweitern etc. Mein finaler Idealzustand wäre eigentlich eine strikte Trennung der ZWave-Klassenbefehle, der Controllerbefehle und der Sendestacks. Leider würde das Umsortieren der Klassen immer noch eine Menge Codezeilen in SVN-Blame betreffen.

Was würde denn passieren wenn die ZWave-Klassen in eine eigene Datei ausgelagert werden würden? Wäre das ohne weiteres möglich oder gibt das dann ein Krampf mit modul-internen Daten auf die man nicht zugreifen kann und die dann übergeben werden müssen? Was passiert eigentlich mit "blame" wenn man ganze Passagen löscht?

Es ist und bleibt eine "Loose-Loose" Situation, ich würde hier aber lieber nichts voreilig durchführen was wir nachher bereuen, auch wenn Du nicht nachträglich jammerst, ich aber vielleicht schon...

Vielleicht kannst Du ja noch mal Deine Meinung zu der Trennung der Klassenbefehle und zu der extra-Datei schreiben.

Gruß,
Andreas.
FB 7360, Homematic und ZWave
Support for ZWave-SECURITY

rudolfkoenig

svn praise gefaellt mir, habs bisher nicht gekannt :)

ZitatWäre das ohne weiteres möglich oder gibt das dann ein Krampf mit modul-internen Daten auf die man nicht zugreifen kann und die dann übergeben werden müssen?
Wir muessten die "Modul-globalen" Variablen, auf die wir in der anderen Datei zugreifen wollen, statt mit "my" mit "use vars qw()" definieren. Als Krampf wuerde ich das nicht bezeichnen. Ich habe mit der Trennung kein Problem, auch wenn ich das selbst nicht in Angriff nehmen werde. Aber einen Patch nehme ich.

A.Harrenberg

Hallo Rudi,

ok, dann würde ich sagen das ich mal versuche die ganzen Klassenfunktionen auszulagern und dann schaue ob sich da irgendwelche Effekte ergeben die wir jetzt nicht im Blick haben.

Falls das dann ohne Nebenwirkungen funktionieren sollte würde ich einen Patch machen bei dem ich die Funktionen aus der 10_ZWave.pm rauslösche ohne den Rest zu sortieren und die Funktionen dann zB. in eine ZWave_Classes.pm auslagert sind.

Dabei würden dann aber auch ALLE "praise" Kommentare zu den Klassen verlorengehen... Die sind mMn aber nicht so wichtig wie die Kommentare der modul-funktionen. Außerdem plane ich sowieso nach und nach durch alle Klassen zu gehen und die auf einen aktuellen Stand zu bringen. Dabei werden dann genügend neue Kommentare in "blame" erzeugt. "praise" ist für Deinen Code reserviert ,-)

Gruß,
Andreas.
FB 7360, Homematic und ZWave
Support for ZWave-SECURITY

A.Harrenberg

Hallo Rudi,

könntest Du mit "svn copy FHEM/10_ZWave.pm FHEM/ZWave_Classes.pm" eine Kopie der aktuellen Datei in SVN erstellen? Die neue Datei "erbt" dabei anscheinend die Historie der Original-datei. Wenn ich dann in der Originaldatei alle Klassen lösche und in der neuen alles was nicht-Klassen betrifft, dann wäre in einem ersten Schritt erstmal die Historie mit blame noch erhalten. Das Umsortieren in ZWave_Classes.pm bringt dann zwar alles durcheinander, es wäre aber anscheinend immerhin noch vorhanden.
Soweit die Theorie...

Gruß,
Andreas.
FB 7360, Homematic und ZWave
Support for ZWave-SECURITY

A.Harrenberg

Hallo Rudi,

Zitat von: rudolfkoenig am 13 August 2016, 13:12:33
Wir muessten die "Modul-globalen" Variablen, auf die wir in der anderen Datei zugreifen wollen, statt mit "my" mit "use vars qw()" definieren.
soll das mit "use vars qw()" oder mit "our" gemacht werden? So wie ich das verstehe ist "use vars qw" deprecated und "our" erfüllte den selben Zweck?

Gruß,
Andreas.
FB 7360, Homematic und ZWave
Support for ZWave-SECURITY

A.Harrenberg

Hallo Rudi,

schon der erste Nebeneffekt...

Was passiert mit der Doku? Die Funktionen in z.B. ZWave_Classes.pm zu spezifizieren und die dazugerhörige Dokumentation in 10_ZWave.pm zu machen ist irgendwie sehr unschön... Soweit ich das mit der Commandref verstehe werden aber nur "offizielle" Module (also alles was [0-9][0-9]_<name>.pm heißt) hinsichtlich des Dok-Blocks ausgewertet oder gibt es da eine Möglichkeit auch die Doku von solchen Supportfunktionesblöcken einzubinden? Funktioniert dann noch "Device specific help"? Wahrscheinlich nicht, oder...

Gruß,
Andreas.
FB 7360, Homematic und ZWave
Support for ZWave-SECURITY

rudolfkoenig

Die Geschichte mit our vs. "use vars qw()" habe ich auch gelesen, ein erstes Experiment funktionierte bei mir aber nicht, und ich habe es nicht weiterverfolgt. Wenn es bei dir mit our klappt, nur zu.

Doku gibts bisher nur in den 10_XX.pm Dateien, bei einer Trennung muessten wir also zwei Dateien pflegen. Ich habe auch keine Idee, wie man das ohne Andere zu stoeren aendern koennte.

rudolfkoenig

Zitatkönntest Du mit "svn copy FHEM/10_ZWave.pm FHEM/ZWave_Classes.pm" eine Kopie der aktuellen Datei in SVN erstellen?
Kann ich gerne machen, wenn ich es soll. Nach deiner letzten Meldung bin ich mir nicht mehr sicher.

A.Harrenberg

Hi Rudi,

ich auch nicht, irgendwie scheint immer was dagegen zu sprechen das aufzuräumen... Und ich hab' ehrlich gesagt keine Lust mich mit Einschränkungen wie Code / Doku getrennt rumzuärgern. Das wäre für mich schlimmer als "unaufgeräumt"... Ich bin da gerade ein wenig frustriert...

Momentan würde ich sagen es bleibt so wie es ist und wenn ich dann mal an die Klassen rangehe verschiebe ich die einzeln und nacheinander, so wie sie gerade bearbeitet werden.

Gruß,
Andreas.
FB 7360, Homematic und ZWave
Support for ZWave-SECURITY

A.Harrenberg

Hallo Rudi,

anbei ein kleiner Patch für BASIC V2. Der Report kann in V2 zwei zusätzliche Parameter senden.

Ich habe die "Erkennung" restriktiver gemacht, also nur 1 paramter oder 3 parameter... Doku ist angepasst.

Ich werde mich mal weiter durch die Klassen wühlen und versuchen das alles auf den neuesten Stand zu bringen, wird aber dauern...

Gruß,
Andreas.
FB 7360, Homematic und ZWave
Support for ZWave-SECURITY

rudolfkoenig