[Refactoring] MySensors-Module

[rudolfkoenig]

Bin von der Praezision beeindruckt.
Ich meine zuletzt gelesen zu haben, dass Zeit "analog" ist (nicht in Quanten zerteilt), insofern plausibel

[Beta-User]

Na ja, über den effektiven Grad der Präzision mag man streiten, vermutlich müßte eine Schleife her, die das vielfach wiederholt, um alle Zufälligkeiten rauszufischen... Das dürfte nur am Ende wenig bringen, denn viel mehr wie eine Tendenz auf Frage, ob die Node "gut" zu erreichen ist, läßt sich darüber m.E. sowieso kaum rausfinden . Mir reicht es jedenfalls, wenn ich über diesen Wert nicht nur über die Geschwindigkeit des Einfärbens des Readings sehen kann, dass es einigermaßen schnell geht . Zeit für die Formatierung und weitere Überlegungen ist später noch, wenn man das wirklich braucht/will...

Für Vorschläge, wie man so eine "Pingzeit" "normierter" darstellt, bin ich aber wie immer offen .

Interessanter werden wohl ein paar der anderen Auswertungen, insbesondere zu RSSI und Extended_DEBUG...


Ein anderer Punkt, den ich aber erst irgendwann später angehen will: Man hat über die Sketche die Möglichkeit, einzelnen "ChildID's" Bezeichnungen zuzuweisen. ChildID's sind sowas wie "Kanäle" (kann alles mögliche sein: ein Relais, ein Temperaturfühler, alle Daten von einem BME280 (die man aber auch "vereinzeln" und unter separaten ChildID's versenden kann)). Nachdem ich jetzt die Vorzüge kennengelernt habe, die "split"-Geräte teilweise haben, wäre es ein nettes Gimmick, die heute als "Einheitsdevice" aufgebauten MYSENSORS_DEVICE-Geräte irgendwie splitten zu können. Muß mich da auch erst eindenken, aber falls jemand eine spontane Eingebung hat: Gerne...
(Denke über ein Attribut am IO nach und/oder eine Art "bridgeRegex" am Device).

Aber: eines nach dem anderen...

[RichardCZ]

Für Vorschläge, wie man so eine "Pingzeit" "normierter" darstellt, bin ich aber wie immer offen  .

Zu ping herüberschiel:

$ ping fhem.de
PING fhem.de (88.99.31.202) 56(84) bytes of data.
64 bytes from fhem.de (88.99.31.202): icmp_seq=1 ttl=53 time=15.1 ms
64 bytes from fhem.de (88.99.31.202): icmp_seq=2 ttl=53 time=17.0 ms
64 bytes from fhem.de (88.99.31.202): icmp_seq=3 ttl=53 time=15.1 ms
64 bytes from fhem.de (88.99.31.202): icmp_seq=4 ttl=53 time=15.4 ms


1/100stel ms - ist doch ausreichend.
Wiederholung um zu sehen ob es Varianzen gibt (nicht um besser einen Durschschnitt abschätzen zu können)
Und noch ein paar andere Dinge wie lost oder dupe Pakete



Spontane EIngebung wird in den nächsten 14 Tagen folgen, denn heute wird meine Mysensors Grabbelkiste geliefert.

[Beta-User]

Spontane EIngebung wird in den nächsten 14 Tagen folgen, denn heute wird meine Mysensors Grabbelkiste geliefert.

Da bin ich ja mal gespannt. Hast du jetzt nur RS485-Bauteile drin oder auch "Funker"?

1/100stel ms - ist doch ausreichend.
Wiederholung um zu sehen ob es Varianzen gibt (nicht um besser einen Durschschnitt abschätzen zu können)
Und noch ein paar andere Dinge wie lost oder dupe Pakete

ms-Angaben finde ich gut. Gestern hatte ich mich erst mal gefreut und euch an meinem allerersten Erfolgserlebnis zu dem Thema teilhaben lassen, da hatte ich erst mal noch gar keine konkrete Vorstellung, was da überhaupt kommt...

Was lost+dupe Pakete angeht, bin ich mal auf die Vorschläge gespannt. Ich gebe dabei aber zu bedenken, dass wir mit RS485 "Exotenhardware" betreiben und man das mMn. nicht so einfach auf Funknetzwerke übertragen kann. Das "Standardsetup" zu MySensors dürfte in vielen Fällen sein, dass es "einfache" Sensorik ist, deren Arduinos im Regelfall schlafen, ohne das dem Controller (=FHEM) mitzuteilen. Die wachen dann irgendwann auf, messen und versenden ihre Meßwerte per Funk, um dann wieder "leise" einzuschlafen. Diese Art Nodes kann man gar nicht anpingen oder abfragen... (deswegen gibt's diese timeout-Attribute: sendet die Node _von sich aus_ auch einen heartbeat, kann man u.a. darüber überwachen, ob die "aktiv" ist oder erwartete Nachrichten ausbleiben - sonst bräuchte man dazu externe Module).

(Dafür liefert RS485 "super-aussagefähige" RSSI-Werte zurück, you'll see). Und das "extended-Debug-Zeug" hat mWn. bisher außer mir max. eine Handvoll Leute je ausgetestet...

[Beta-User]

Und noch ein update im ersten Thread.

Jetzt gibt es "ms"-Ausgaben und Rückmeldungen zu noch ein paar mehr Anfragen. Vermutlich ist die Formatierung der Debug- und RSSI-Rückmeldungen nicht optimal, aber ich habe grade keine Nodes zum Testen .

Wenn sich die Downloadzahlen (=Tester?) nicht wesentlich erhöhen, lasse ich das jetzt noch eine Weile beim mir so laufen und checke das Gesamtpaket dann vermutlich am WE ein. perlcritic@MYSENSORS_DEVICE liefert bei -3 zwar noch ein paar Punkte, aber da warte ich dann auf Verbesserungsvorschläge von kundiger Seite...

[RichardCZ]

Code Auswählen Erweitern

*{'main::MYSENSORS_Initialize'} = *{'Initialize'};
->
sub main::MYSENSORS_Initialize { goto &Initialize };


letzteres schmiert wesentlich weniger in der Symboltabelle herum.

Ist dann das no strict überhaupt notwendig?



Bei den Uses immer erst die Systemmodule, dann die Projektmodule, dann die eigenen Module

use GPUtils qw(:all);
use Device::MySensors::Constants qw(:all);
use Device::MySensors::Message qw(:all);

use DevIo;

use Exporter ('import');
->

use Exporter ('import');

use DevIo;
use GPUtils qw(:all);

use Device::MySensors::Constants qw(:all);
use Device::MySensors::Message qw(:all);


Das ist robuster wenn irgendjemand irgendwo im Namespace rumschmiert. Und wenn das irgendwo auf die Schnauze fliegt, dann sind echt einige Exkremente am dampfen.
Und vielleicht innerhalb der Gruppen alphabetisch sortiert zur Übersicht.



  if ($init_done) {
    return Start($hash);
  } else {
    return;
  }

Das geht in 2 Zeilen - und sauberer/übersichtlicher.

return "Need at least one parameters"  <- *parameter



Puh - das ist ja gut gemeint:

Code Auswählen Erweitern

return "Unknown argument $a[1], choose one of " . join(" ", map {@{$sets{$_}} ? $_.':'.join ',', @{$sets{$_}} : $_} sort keys %sets)  if(!defined($sets{$a[1]}));

aber

Code Auswählen Erweitern

return "Unknown argument $a[1], choose one of "
    . join(" ", map {
        @{$sets{$_}} ? $_
                       . ':'
                       . join ',', @{$sets{$_}}
                     : $_
                   } sort keys %sets)
    if (!defined($sets{$a[1]}));


Das hat sicher die Grenze zum Einzeiler längst überschritten. (also auch meine aufgedrüselte version, die ja auch ein Einzeiler ist nur optisch formatiert)

Code Auswählen Erweitern

   sendMessage($hash,radioId => 0, childId => 0, cmd => C_INTERNAL, ack => 0, subType => I_INCLUSION_MODE, payload => $value eq 'on' ? 1 : 0);

Bitte keine Angst vor hübscher Formatierung (=Lesbarkeit):

Code Auswählen Erweitern

sendMessage($hash,
            radioId => 0,
            childId => 0,
            cmd     => C_INTERNAL,
            ack     => 0,
            subType => I_INCLUSION_MODE,
            payload => $value eq 'on' ? 1 : 0
        );

wobei hier ja eigentlich zwei Hashrefs übergeben werden sollten.


10_MYSENSORS_DEVICE

if ($command =~ m{time|reboot|clear|flash|fwType}xms) { # for speed reasons?

No, for brevity reasons. Und vergiss die \A \z Anker nicht - die da hoffentlich hingehören.

Code Auswählen Erweitern

if ($command eq "fwType") {
        if (@values == 1) {
          my ($type) = @values;
          if ($type =~ m{^\d*$}xms) {
            readingsSingleUpdate($hash, 'FW_TYPE', $type, 1);
          } else {
            return "fwType must be numeric";
          }
        }
        return;
      }

Der Reihe nach:

* Die Firmwareversion kann auch ein Leerer String sein? Nein? Siehste.
* Mongolische Ziffern möglich? Nein? Siehste.

Also ich würde es ja so machen:

Code Auswählen Erweitern

if ($command eq 'fwType') {
    my $type = shift @values // return;
    return "fwType must be numeric" if ($type !~ m{^[0-9]+$}xms); # ja ne is klar :-)
    return readingsSingleUpdate($hash, 'FW_TYPE', $type, 1);
}


Aber eigentlich sollte man es so machen:

https://forum.fhem.de/index.php?topic=88878.15 
https://wiki.fhem.de/wiki/MySensors_Starter_Guide
("Zwischenzeitlich wird ein update über den jeweiligen Transport-Layer (OTA) von HFEM unterstützt[5].") - was ist bitte HFEM?

Code Auswählen Erweitern

if ($command eq 'fwType') {
    my $type = shift @values // return;
    return "fwType must be numeric, but got >$type<." if ($type !~ m{^[0-9]{2,20}$}xms);  # begrenzt vielleicht? Keine Major.Minor?
    return readingsSingleUpdate($hash, 'FW_TYPE', $type, 1);
}


[Beta-User]

Anbei mal ein Satz mehr oder weniger ungetesteter Versionen, wird etwas Zeit benötigen, dass ich das dem "live-Härtetest" unterziehen kann, vorab gibt's aber noch ein paar Kleinigkeiten zu klären...

Code Auswählen Erweitern


sub main::MYSENSORS_Initialize { goto &Initialize };


Done. Dann kann man auch auf strict/refs verzichten.

Unklar: Was vars angeht. Das klappt mit den EXPORT-Variablen aber nur, wenn ich ein "our" davorschreibe (das hattest du irgendwann mal vorgeschlagen). Leider ist mir nur nicht so ganz klar, wo die Unterschiede liegen (bzw. wo ich das DAM-gerecht nachlesen kann...). Es ist daher jetzt in der "our"-Variante drin, mal sehen, ob mir das beim live-Test doch aus irgendwelchen Gründen um die Ohren fliegt...

Bei den Uses immer erst die Systemmodule, dann die Projektmodule, dann die eigenen Module [...]
Und vielleicht innerhalb der Gruppen alphabetisch sortiert zur Übersicht.

Dürfte jetzt passen...

Das geht in 2 Zeilen - und sauberer/übersichtlicher. [...]
return "Need at least one parameters"  <- *parameter

Ok, wir kommen langsam aber sicher zu den "Feinheiten"... Hab's da eleminiert, wo es mit grade vor der Flinte war, aber ohne Ansprucha uf Vollständigkeit...

Das hat sicher die Grenze zum Einzeiler längst überschritten. (also auch meine aufgedrüselte version, die ja auch ein Einzeiler ist nur optisch formatiert) [...]
Bitte keine Angst vor hübscher Formatierung (=Lesbarkeit):

Na ja, manche ererbten Juwelen sind halt wie sie sind... Habe die jetzt auch in den Fällen etwas abgestaubt, wo es mir beim Vorbeigehen ins Auge gespungen war .

wobei hier ja eigentlich zwei Hashrefs übergeben werden sollten.

Was will mir "eigentlich" sagen? Ändern? Also den hash vorher bilden und dann als solchen der Funktion übergeben? Irgendwo zentral ablegen und dann darauf verweisen? Weiß noch nicht so recht, habe etwas Sorge, dass es am Ende dann eher noch unübersichtlicher werden würde...

No, for brevity reasons. Und vergiss die \A \z Anker nicht - die da hoffentlich hingehören.

Hab' die Anker auch noch an ein "paar" anderen Stellen verteilt, dto. für die \s+-Geschichte samt Derivaten...

* Die Firmwareversion kann auch ein Leerer String sein? Nein? Siehste.
* Mongolische Ziffern möglich? Nein? Siehste.
[...]Aber eigentlich sollte man es so machen:[...]
Keine Major.Minor?

Danke für den Vorschlag, überzeugend...

Den Link interpretiere ich mal als diese Frage: "Warum nicht einfach auf eine bestimmte firmware-Datei verweisen und dazu einen upload-Befehl generieren, wie sonst in FHEM üblich?"

Nun ja, es gibt für MySensors eine Art "Muster-Controller", über den einige User früher ihre updates gemacht haben (also außerhalb FHEM). Der sieht diese Art der Verwaltung der firmwares und ein "auto-update-feature" vor, das eben über eine csv-Datei gesteuert wird und eine formalen Aufbau hat, was auch die Frage nach der Nummerierung beantwortet: dort sind afaik nur natürliche Zahlen vorgesehen, siehe https://www.mysensors.org/about/fota#launch-your-fota-update.

An sich wäre es wirklich mehr "FHEM-like", das händisch anzuschubsen, aber das würde das "auto-update-feature" brechen, daher habe ich den Aufwand bisher nicht getrieben und tendiere auch weiter dazu, das so zu lassen, wie es ist...



Noch was ganz anderes:

Soweit ich das überblicke, werden die drei Funktionen in Message.pm nur von 00_MYSENSORS.pm benötigt. Tendiere daher dazu, die schlicht und ergreifend da reinzubasteln...?
Deine Meinung?

Wenn man das dann vollständig machen wollte, wäre das auch mit Constants.pm die Frage, aber die wird eben tatsächlich von beiden Seiten her genutzt. Aber auch da wäre es vermutlich nicht so viel aufwand, das in die 00 mit reinzuwursteln.
Auch hier: Deine Meinung?

[Beta-User]

Live-Test steht noch aus, aber:

Anbei MYSENSORS/MYSENSORS_DEVICE, die ohne  Constants.pm und Message.pm in /lib auskommen... Ist jetzt beides in 00_MYSENSORS.pm integriert.

[Beta-User]

So, live-Test läuft - sieht soweit erst mal gut aus .

Die Module finden sich zum testen für "Freiwillige" jetzt wieder im ersten Post hier.

Das dispatchen mit "constant" klappt jetzt auch, aus http://neilb.org/reviews/constants.html (21 ways to use constant) habe ich den Trick geklaut, CONSTANT() zu verwenden. Hat aber bestimmt einen Pferdefuß...(?)

Wie auch immer, bei Gelegenheit sollte man vermutlich die "Startroutine" noch von eventbasiert auf timerbasiert umbauen, aber Rom und so... (@Richard: das mit der Startroutine ist ein spezielles FHEM-Thema, wir kommen ein andermal darauf zurück).

Das mit dem EXPORT scheint ein spezielles Thema zu sein, das ich noch nicht verstehe, aber auch da: Rom!

Jetzt mault perlcritic immer noch manches auf Level -3 an, aber für's erste wäre ich (evtl. bis auf das Initialisierungsthema) geneigt, den Stand aus dem ersten Post erst mal einzufrieren und das ganze dann in ein paar Tagen "as is" ins svn zu packen.

[Wzut]

Darf ich jetzt auch mal Klugscheissen ?
So wie dein Sub Set anfängt hatte ich es auch fast in jedem Modul , das @_ und das "böse" @a und dann die Teile int(@a) , $a[1], usw durchgekaut
Inzwischen steht bei mir da

Code Auswählen Erweitern

sub Set {

    my $hash = shift;
    my $name = shift;
    my $cmd  = shift // return "set $name needs at least one argument !";
    my $value = shift // '';

und warum gleich so früh if(!defined($sets{$a[1]})); ? Willst du mit aller Gewalt gleich raus wenn das FHEMWEB Fragezeichen kommt ?
lass es doch bei den den paar if commands einfach durchlaufen und gib ganz am Schluss immer das unknow aus

[Beta-User]

Darf ich jetzt auch mal Klugscheissen ?

...immer wieder gerne .

So wie dein Sub Set anfängt hatte ich es auch fast in jedem Modul , das @_ und das "böse" @a und dann die Teile int(@a) , $a[1], usw durchgekaut
Inzwischen steht bei mir da

...vielleicht sollte ich mir das @a/b nochmal vornehmen .
Und die ganzen shift's erweitern... (aber irgendwo hatte ich zu shift vs. (....)=@_ neulich gelesen: Wenn man das ganze nur intern aufruft, darf man auch etwas weniger "failsafe" programmieren....)

und warum gleich so früh if(!defined($sets{$a[1]})); ? Willst du mit aller Gewalt gleich raus wenn das FHEMWEB Fragezeichen kommt ?
lass es doch bei den den paar if commands einfach durchlaufen und gib ganz am Schluss immer das unknow aus

Ähm, ich tu' mich mit dem Teil des Codes auch immer schwer, wenn da so viele Referenzierungen drin sind, aber mMn. hat das hier nichts mit dem roten Fragezeichen zu tun, sondern es geht darum, ob der betreffende set-Command überhaupt vorhanden ist. Ist er das nicht, macht es keinen Sinn, weiterzumachen, sondern es dürfte eher wichtig sein, hier - as is - aufzuhören . Das erfüllt also mMn. genau den Zweck, den das shift oben ggf. auch gehabt hätte: failsafe's Programmieren...

[Wzut]

ok, dann vergleich mal das Ende von matchClient mit dem Ende von matchChan76GWClient ein paar Zeilen tiefer.
return $found if $found;
return;
habe ich mir inzwischen abgewönht und mache es wie in matchClient.

Log3($hash->{NAME}, 4, "$hash->{NAME}: matched firmware config request to hash $found, name: $found->{NAME}") if $found;

Steht da im Log wirklich was lesbares für $found ($found->{NAME} leuchtet mir ein ) , aber $found ist doch $defs{$d} ?
und ich fand die $hash->{NAME} immer gruselig da du in der foreach eh my $name = $hash->{NAME} hast kannst das doch auch vorziehen und dann nur noch $name verwenden.
Im Gegenzug hätte ich eher $clientname eingespart da nur 1x verwendet und direkt ins  AttrVal gepackt. 

[RichardCZ]

und warum gleich so früh if(!defined($sets{$a[1]})); ? Willst du mit aller Gewalt gleich raus wenn das FHEMWEB Fragezeichen kommt ?

Ob das jetzt in dem konkreten Fall so sein muss oder nicht, lasse ich mal dahingestellt, aber prinzipiell ja - sollte man so früh wie
möglich rausspringen wenn sich vergebliche Liebesmüh' abzeichnet.

Also anstelle

Code Auswählen Erweitern

sub x {
  my blah = shift;
  if (defined blah) {
    wurschtel
  }
  return;
}

eben

Code Auswählen Erweitern

sub x {
  my blah = shift // return;
  # Here we know blah MUST be defined
  # and we spare ourselves an if-Einrückung
  wurschtel

  return;
}

Es ist sogar wünschenswert mal einen if-Ast zu negieren und vorab "abzufackeln",
wenn man sich Einrückungen größerer Mengen Code spart.

Code Auswählen Erweitern

sub x {
  my blah = shift;

  if (mords && logic || function) {
   x-zeilen ellenlanges
   gewurschtel
  }

  return;
}
->

Code Auswählen Erweitern

sub x {
  my blah = shift;

  if (! (mords && logic || function)) { # bzw. De-Morgan
    return;
  }

  x-zeilen ellenlanges
  gewurschtel

  return;
}

[Beta-User]

Boah, ihr wollt's aber wirklich wissen...

"Komisch", dass man merkt, dass matchChan76GWClient eine der ersten komplexeren Funktionen war, die ich mühsam und mit viel Hokuspokus selbst anhand der Vorlage zusammengeschustert habe, da  sind noch ein paar Ausgaben im verbose 4 drin gewesen, die wirklich niemandem weitergeholfen hätten, die ich aber brauchte, um erst mal selber zu verstehen, was da in etwa abgeht ...

Hab's jetzt moderat angepaßt und hoffe, dass es dann auch noch funktioniert, wenn es wirklich mal einer brauchen sollte (das ist eine sehr spezielle Spezialfunktion, die man nur ernsthaft testen kann, wenn man mit der passenden Hardware rumoperiert oder sehr clever ist, was das Programmieren von Testsoftware angeht ).

Ansonsten habe ich set in die Richtung umgebaut wie vorgeschlagen, den Start timerbasiert verzögert (und notifyfn in MYSENSORS ganz ausgebaut), noch ein paar "each" verjagt und den Rest hoffentlich nicht kaputtgemacht .

Dateien zum Testen wie immer im ersten Post.

[Beta-User]

Ich weiß noch nicht wie ich es geschafft habe - vermutlich dunkle Experimente mit Mysensors (ohne Codeänderungen jedoch) - aber
[...]
Best guess ist, dass ich irgendwo was hardwaremäßig noch nicht richtig konfiguriert habe.

Code Auswählen Erweitern

MYSENSOR_0 erscheint.
Zumindest sind solche undefines vorher nicht aufgetaucht im Log.

Vorab mal OT: Das "zuckt" ja zumindest schon mal. Gratuliere!
Was diese "spezielle" Node angeht, solltest du dir nicht die großen Gedanken machen, das ist im wesentlichen (meistens(!)) unnützes "Beiwerk"; diese sind "0"-er Devices bei mir alle in einem "unused-devices"-Raum (ich habe mehrere GW's definiert).

K.A., wie deine Dunklen Experimente aussahen, aber bei mir erscheinen weder früher noch jetzt derartige Einträge und "trim" konnte ich im Quellcode auch nicht finden...

Ok. ShuttersControl schafft das vielleicht noch nicht, aber so ein MySensors/* kommt sicher bald hin.

Nett, dass du das dort schreibst, hier aber meine entsprechenden Anfragen nicht mal mit einem "warte noch" kommentierst (falls wir über dieselben Code-Teile sprechen (?)). Den Mehrwert der Aufsplittung wollte man mir "am anderen Ort" auch nicht erläutern (ich sehe da heute das erste Mal einen vielleicht nachvollziehbaren inhaltlichen Punkt...), und ehrlich gesagt ist mir diese "one in all"-Lösung mit den nur noch zwei Modulen/Dateien sehr viel sympatischer. Aber vielleicht schaffst du es ja, mir den Sinn dieser Aktion an sich und der Testerei im speziellen an einem Modul (=MySensors) zu erläutern, dessen Funktionalitäten ich halbwegs verinnerlicht habe...