Newbie Vorschläge

[Phill]

Hallo,
ich habe mich neu als Developer eingeschrieben, da ich aktiv am Projekt mitgestalten wollte. Bei der Anwendung und Einrichtung von FHEM für unser Haus und bei der Entwicklung meines Moduls Talk2Fhem sind mir Sachen aufgefallen, die ich hier mal darlegen wollte.

Bevor ich zu den Themen einen Patch erstelle, wollte ich erst mal fragen ob es überhaupt gewünscht ist patches für den Hauptprozess fhem.pl einzureichen, oder ob sowas nur für ein Majorrelease was ist. Sind keine großen Sachen aber folgendes halte ich für suboptimal.

1. Beim ändern und anlegen von Attributen lässt sich ja in AttrFn der Attributwert beeinflussen. Das funktioniert aber nur wenn der Wert auch existiert. Wird kein Attributwert angegeben wird der Wert immer auf 1 gesetzt. Habe das Problem mal kurz überflogen aber ich denke dass in dem Übergebenen Array an die Funktion, das Feld für den Wert noch nicht existiert.
Dachte eigentlich ein

Code Auswählen Erweitern

push(@_, $wert)würde helfen, macht es aber nicht.
Oder gibt es dafür ein workaround?

2. Warum sind eigentlich bei den globalen Funktionen InternalVal, ReadingsVal, ReadingsNum usw. der default-Parameter nicht Alternativ? Ich sehe eigentlich bei 99% aller Beispiele immer nur einen Leeren String dort eingetragen! Ich meine es steht ja sogar im Kommentar:

Functions used to make fhem-oneliners more readable

Wäre es nicht noch mehr Readable wenn der Letzte Parameter optional wäre und je nach funktion dann ein Sinniger Wert zurückgegeben wird. 0 oder Leerzeichen
Ich würde ein Standardzeichen auch einem undef bevorzugen weil es sonst im skalaren Kontext immer eine Warnung gibt.

So das war jetzt mal mein Einstieg, bitte, um des Motivationswillen, nicht zu sehr drauf hauen.

Danke

[rudolfkoenig]

Wird kein Attributwert angegeben wird der Wert immer auf 1 gesetzt.

Ja, und ich will dieses Verhalten nicht mehr aendern. Mag sein, dass ich das heute anders machen wuerde, aber das wird an vielen Stellen verwendet, und ich will nicht alles umstellen.

Warum sind eigentlich bei den globalen Funktionen InternalVal, ReadingsVal, ReadingsNum usw. der default-Parameter nicht Alternativ?

Ich meine, es gibt keine sinnvolle Voreinstellung, und damit wird der Benutzer gezwungen, eins zu waehlen.
Und man koennte dann kein undef als Voreinstellung spezifizieren. Ich habe aber nicht wirklich Probleme, es auf Optional zu aendern.

Wg. dem Patch: es hat nichts mit major-Release zu tun, sowas ist nur als "Grundlage fuer Installation" zu sehen.

[Phill]

Ja, und ich will dieses Verhalten nicht mehr aendern. Mag sein, dass ich das heute anders machen wuerde, aber das wird an vielen Stellen verwendet, und ich will nicht alles umstellen.

Das der Wert auf 1 gesetzt wird ist ja auch gut und soll nicht geändert werden, ich meine dass man in dem Fall über AttrFn den Attribut Wert nicht beeinflussen kann, was ja dem Modul an sich gewährt wird, nur eben nicht, wenn der Wert leer ist.

[betateilchen]

Ja, und ich will dieses Verhalten nicht mehr aendern.

Allerdings wäre ich auch SEHR dafür, dass dieser oft Fehler/Fehlverhalten erzeugende Unfug endlich mal geändert wird. Genau wie ein paar Andere Dinge im Attributhandling.

Beispiel:

Rufe das Popup zum Ändern des Attributes "room" auf und entferne dort alle Haken. Da würde ich erwarten, dass das Attribut komplett verschwindet, wenn kein Raum mehr ausgewählt ist.

Aber nein: das Attribut bekommt danach tatsächlich den Wert 1 zugewiesen und das Device befindet sich danach im room=1 Und in meiner Raumliste  links existiert plötzlich ein neuer Raum mit dem Namen "1" den ich nie haben wollte.

Die Liste dieser unsinnigen Ereignisse im Attributhandling liesse sich beliebig fortführen. Aber ich habe gerade keine Lust, mich schon wieder darüber aufzuregen.

Aber immerhin beruhigt es mich sehr, dass ich nicht alleine mit meiner Meinung zum Thema Attribute und deren Verhalten bin.

[Phill]

Ich wollte gerade zu dem Thema einen fhem.pl Patchvorschlag machen, da stelle ich fest, dass eben diese oben angesprochenen Funktion gar nicht mehr möglich ist in der neusten Version aus SVN.

Code Auswählen Erweitern

2817c2797
<     my $val = $attrVal;
---
>     my $val = $a[2];
Hab ich was verpasst?

[betateilchen]

vielleicht das hier? https://forum.fhem.de/index.php/topic,83414.0.html

[betateilchen]

Das mit der Änderung für append und delete ist übrigens mal wieder so ein typischer, nicht zu Ende gedachter Schnellschuss, der bei configDB nicht funktionieren kann, wenn jemand versucht, solche Zeilen mit -a und -d in die Konfiguration zu schreiben.

Man sollte zumindest irgendwo dokumentieren, dass diese zusätzlichen Parameter nur im "Online Modus" innerhalb einer laufenden FHEM Installation verwendet werden dürfen.

(Gleiches Problem übrigens bei "define -temporary", das funktioniert auch nicht bei configDB)

[Benni]

Da gibt es doch noch einiges mehr, was nur im "Online-Modus" sinnvoll ist, bzw. anders herum auch nur während der Initialisierungsphase.
Vielleicht sollte man hier generell die Möglichkeit einer Art Attributierung schaffen um Kommandos bereits Modulseitig entsprechend zu kennzeichnen und dann jeweils während der einzelnen Phasen/Systemzustände passend darauf zu reagieren.

Nichts desto trotz gehört es auf jeden Fall in der Commandref dokumentiert.

Gruß Benni.

[rudolfkoenig]

@betateilchen:

bei configDB nicht funktionieren kann, wenn jemand versucht, solche Zeilen mit -a und -d in die Konfiguration zu schreiben.

Warum soll jemand -a oder -d in die Konfiguration schreiben wollen?
Vermutlich habe ich etwas nicht verstanden: kannst du es bitte anders formulieren?

Da würde ich erwarten, dass das Attribut komplett verschwindet, wenn kein Raum mehr ausgewählt ist.

Und ich finde das Verhalten bei "attr global stacktrace" oder "attr global mseclog" natuerlich.
Was soll passieren, wenn man kein vaue spezifiziert? In deinem Fall waere loeschen konsequent, das wuerde aber vielerorts Probleme verursachen. Eine Fehlermeldung waere in deinem Fall auch nicht wesentlich eleganter.
Dieses Verhalten gibt es seit in fhem (bzw. fhz1000.pl) Attribute gibt, ich habs jetzt aber endlich auch dokumentiert.


@Phill: die Ursache von Aenderungen kannst du bei meinen Aenderungen mit "svn blame fhem.pl" gefolgt von "svn log fhem.pl" rauskriegen. 99% sind mit einer Hinweis auf die Diskussion im Forum versehen.

[Phill]

Also das scheint wirklich beim hinzufügen der Parameter "-a" und "-d" passiert zu sein, das die modulseitige Attributänderung komplett nicht mehr möglich ist. Mein vorschlag:

Code Auswählen Erweitern

Index: fhem.pl
===================================================================
--- fhem.pl (Revision 16044)
+++ fhem.pl (Arbeitskopie)
@@ -2808,15 +2808,16 @@
       $attrVal = "-" if($attrName eq "logfile");
       $attrVal = 5   if($attrName eq "verbose");
     }
-    $ret = CallFn($sdev, "AttrFn", "set", @a);
+
+    $attrVal = 1 if(!defined($attrVal));
+
+    $ret = CallFn($sdev, "AttrFn", "set", $_[0], $attrName, $attrVal);
     if($ret) {
       push @rets, $ret;
       next;
     }

-    my $val = $attrVal;
-    $val = 1 if(!defined($val));
-    $attr{$sdev}{$attrName} = $val;
+    $attr{$sdev}{$attrName} = $attrVal;

     if($attrName eq "IODev") {
       if(!$attrVal || !defined($defs{$attrVal})) {
@@ -2838,9 +2839,9 @@
       return $err if($err);
       evalStateFormat($hash);
     }
-    addStructChange("attr", $sdev, "$attrName $val")
-        if(!defined($oVal) || $oVal ne $val);
-    DoTrigger("global", "ATTR $sdev $attrName $val", 1) if($init_done);
+    addStructChange("attr", $sdev, "$attrName $attrVal")
+        if(!defined($oVal) || $oVal ne $attrVal);
+    DoTrigger("global", "ATTR $sdev $attrName $attrVal", 1) if($init_done);

   }


Dadurch ist die Funktion wieder gegeben, der Wert 1 wird bei leerem Attribut vor AttrFn gesetzt, was meiner Ansicht eh wichtig ist und dadurch kann das Modul auch leere Attributwerte beeinflussen, und die zwischenvariabel $val ist sowieso obsolet seit dem Patch.

Ich rede übrigens die ganze Zeit hier von DevelopmentModuleIntro:

Zusätzlich ist es möglich auch übergebene Attributwerte zu verändern bzw. zu korrigieren, indem man im Parameterarray @_ den ursprünglichen Wert anpasst. Dies erfolgt im Beispiel über die Modifikation des Wertes mit Index 3 (entspricht dem 4. Element) im Parameterarray, also $_[3].

[betateilchen]

Dieses Verhalten gibt es seit in fhem (bzw. fhz1000.pl) Attribute gibt, ich habs jetzt aber endlich auch dokumentiert.

Davon dass dieses kranke Verhalten jetzt dokumentiert wurde, wird es auch nicht besser. Es bleibt krank und völlig unlogisch.

@betateilchen:Warum soll jemand -a oder -d in die Konfiguration schreiben wollen?

Dafür gibt es keinen sinnvollen Grund, das weiß ich genau so gut wie Du. Aber auch Du kennst die FHEM Nutzer, die oft nicht wissen was sie tun. Die schreiben manchmal sogar set-Befehle in die fhem.cfg, einfach "weil es geht". Viele Anwender können nicht unterscheiden, dass eine laufende Konfiguration nicht immer zu 100% dem entsprechen muss, was in der Konfiguration steht. Und das kann man den Anwendern auch überhaupt nicht zum Vorwurf machen, weil für dieses Verständnis sehr tiefgreifende Kenntnisse über das Gesamtkonstrukt FHEM und dessen internen Funktionsweisen notwendig sind. Die kann und muss man bei Anwendern nicht voraussetzen, aber man sollte die möglichen Konsequenzen daraus berücksichtigen.

Aber was in letzter Zeit immer mehr passiert: Bewährte, einfache und logische Dinge werden völlig unnötig verkompliziert.

Beispielsweise wird die Syntax

Code Auswählen Erweitern


<cmd> <device> <param> <optionalParameter>

in der Form:
define <device> TYPE <optionalParameter>
attr <device> attrName attrValue


auf die man sich jahrelang blind verlassen konnte und die völlig simpel, logisch und verständlich war, plötzlich grundlegend geändert in

Code Auswählen Erweitern


<cmd> [<option>] <device> <param> <optionalParameter>


ohne dass es dazu jemals eine Ankündigung oder gar eine Diskussion über mögliche Folgen gegeben hätte.

[rudolfkoenig]

@Phill: ich habe deinen Patch etwas erweitert uebernommen.

@betateilchen: ich wuerde "ploetzlich" durch schleichend ersetzen: define hat seit laengerem -ignoreErr und -temporary, list hat -r. Bei der ausgebliebenen Diskussionen gebe ich dir Recht, allerdings habe ich nicht damit gerechnet, dass es Probleme verursacht, oder solche Emotionen freisetzt.

[betateilchen]

Für mich ist das überhaupt kein emotionales Thema.

[Phill]

Ähm Achtung, da fehlt jetzt ein Parameter
Vorher:

Code Auswählen Erweitern

$ret = CallFn($sdev, "AttrFn", "set", @_);
Meins:

Code Auswählen Erweitern

$ret = CallFn($sdev, "AttrFn", "set", $_[0], $attrName, $attrVal);
Jetzt:

Code Auswählen Erweitern

$ret = CallFn($sdev, "AttrFn", "set", $attrName, $attrVal);
@_ hat doch 3 Elemente. Da fehlt nochmal der Name denn in der AttrFn kommt der Name nicht mehr an, dafür irgendein FHEMWEB hash. ?

Code Auswählen Erweitern

@a = split(" ", $param, 3) if($param);

Ich denke so sollte es aussehen:

Code Auswählen Erweitern

Index: fhem.pl
===================================================================
--- fhem.pl (Revision 16047)
+++ fhem.pl (Arbeitskopie)
@@ -2808,7 +2808,7 @@
       $attrVal = "-" if($attrName eq "logfile");
       $attrVal = 5   if($attrName eq "verbose");
     }
-    $ret = CallFn($sdev, "AttrFn", "set", $attrName, $attrVal);
+    $ret = CallFn($sdev, "AttrFn", "set", $sdev, $attrName, $attrVal);
     if($ret) {
       push @rets, $ret;
       next;


[Markus Bloch]

Da haste recht, müsste dann aber eher so lauten:

Code Auswählen Erweitern

$ret = CallFn($sdev, "AttrFn", "set", $sdev, $attrName, $attrVal);

Du meintest wohl eher $a[0] von @a. Dieser enthält aber unter Umständen keinen Definitionsnamen, sondern die verwendete devspec. Die Variable $sdev trägt den jeweils aufgelösten Definitionsnamen.

Gruß
Markus

[Phill]

Ja richtig war ein copy paste fehler hatte wahrscheinlich zu lange gebraucht den beitrag zu korregieren.   

[rudolfkoenig]

Ich habe jetzt

Code Auswählen Erweitern

$ret = CallFn($sdev, "AttrFn", "set", $sdev, $attrName, $attrVal);
eingecheckt. $a[0] statt $sdev waere auch gegangen, weil paar Zeilen weiter oben $a[0]=$sdev stand, ist aber irritierend.


Wegen der dokumentierten Aenderung der Attributwerte:
- bin traurig drueber, dass es dokumentiert wurde, weil AttrFn meiner Ansicht nach die Attribute nicht aendern soll. Sonst kann sich der Endanwender auf nichts mehr verlassen.
- kann jemand bitte erklaeren, wozu es sinnvoll ist?
- ist jetzt wieder moeglich (getestet)
- bis vor kurzem haette ich geschworen, dass Strings als Value an subs uebergeben werden, scheint aber nicht der Fall zu sein. Wenn jemand mich hier aufklaeren kann...

[Phill]

Ganz einfach, in Perl sind alle Funktionsübergaben immer eine Referenz.
Nur wird halt meißtens am Anfang gleich dereferenziert.

Code Auswählen Erweitern

my $deref = $_[0];

Und die Sinnhaftigkeit der Änderbarkeit, sehe ich darin, dass ja nicht nur ein Syntaxcheck sondern auch eine Syntaxkorrektur vorgenommen werden kann. Und jetzt auch die Möglichkeit besteht einen Standardwert Modulseitig zu definieren wenn nichts angegeben ist.

[Markus Bloch]

@_ ist ein Alias auf die Übergabeparameter. Das ist ein Standardfeature von Perl Subroutinen. Wenn man @_ verändert, verändert man die Variable, welche an die Subroutine übergeben wurde. In Perl 6 geht dies so direkt nicht mehr, da muss man schreibbare Parameter explizit aktivieren.

[betateilchen]

Und jetzt auch die Möglichkeit besteht einen Standardwert Modulseitig zu definieren wenn nichts angegeben ist.

Herr, schmeiß Hirn! Schmeiß meinetwegen irgendwas, aber ziele bitte gut, damit solcher Unfug irgendwann aufhört.

Sonst kann sich der Endanwender auf nichts mehr verlassen.

Seit wann kommt es denn DARAUF an?

[Phill]

Herr, schmeiß Hirn! Schmeiß meinetwegen irgendwas, aber ziele bitte gut, damit solcher Unfug irgendwann aufhört.

He, sei nett bitte. Das ist doch nur die Konsequenz daraus, dass sich am Standardwert 1 nichts mehr ändern wird. Und das ist eben nicht für jedes Modul was.

Wenn man es jetzt weiterspinnt, sollte das Attribut eigentlich nicht angelegt werden wenn die Funktion undef setzt.

[rudolfkoenig]

Ganz einfach, in Perl sind alle Funktionsübergaben immer eine Referenz.

Mag sein, aber ich kann mich daran erinnern, dass es nicht so war. Ich hatte mal explizit \$ uebergeben muessen, damit der Rechner mit meinem 300MB Strings nicht platzt.  Zugegeben, das ist schon etwas laenger her.

Das ist doch nur die Konsequenz daraus, dass sich am Standardwert 1 nichts mehr ändern wird.

Sehe ich ganz anders: das ist nur so, weil ich was Falsches angenommen habe, und der Feature dokumentiert und verwendet wird. Meiner Ansicht nach sollte man eine Fehlermeldung liefern, und nicht an dem Wert herumdoktern.

[betateilchen]

Meiner Ansicht nach sollte man eine Fehlermeldung liefern, und nicht an dem Wert herumdoktern.

Attribute gehören dem Benutzer. (diese Aussage stammt nicht von mir, sondern von Rudi!) Und der Benutzer soll sich bitte auch darum kümmern, und nicht irgendwelche undurchschaubaren Mechanismen in irgendwelchen Modulen. Das gilt für mich sowohl für automatisches Umbenennen, noch mehr aber für das "automatische" Verändern von Attributwerten oder das Zuweisen von "1" an jeder noch so sinnlosen Stelle.

Aber offenbar wird der Kreis der FHEM Entwickler, die nur noch danach streben, hier ihren Egoismus auszuleben, leider immer größer. Um die Benutzer geht es doch in den meisten Fällen überhaupt nicht mehr. Dieses Phänomen hat sich leider in den letzter Jahren immer stärker ausgeprägt. Vor drei Jahren hätte Rudi einen solchen Unfug wie jetzt hier im Thread vorgeschlagen niemals zugelassen, geschweige denn auch noch selbst umgesetzt.

Das ist der Punkt, der mich persönlich am meisten enttäuscht - inzwischen lässt sich offenbar jeder Mist in FHEM "wünschen" und die Kritikfähigkeit gegenüber solchen Änderungen ist gegen NULL gesunken. Diese persönliche Enttäuschung ist der einzige emotionale Punkt, der mich im Moment beschäftigt.

Wir - und damit meine ich ALLE Entwickler hier in FHEM - sind gerade auf dem besten Weg, FHEM zu einem für normale Anwender nicht mehr verständlichen und unbeherrschbaren Moloch zu entwickeln. Und das alles nur, um irgendwelche persönliche Ideen auf dem Rücken der Benutzer umzusetzen. Ob die Mehrzahl (!) der Benutzer davon tatsächlch irgendeinen Nutzen haben, interessiert nicht mehr sonderlich.

Gute Nacht.

[Phill]

Guten morgen,

ich kann mir nicht ankreiden lassen das ich hier irgendwelche Egoismen durchsetzen will oder mir hier irgendwas gewünscht hätte. Ich habe lediglich auf eine Unstimmigkeit hingewiesen, und war daran interessiert diese Aufzuklären. Wenn das dazu geführt hätte die Änderung zu unterbinden, hätte ich das genauso gut geheißen, denn es wäre das Resultat eines Entwicklungsprozesses und Sachlichkeit gewesen. Und daran bin ich interessiert. Nicht an rumstänkern und Unfähigkeit zu attestieren.

Ich bin auf jeden Fall bei dir wenn es darum geht Wildwuchs zu unterbinden, denn so ein großes Projekt funktioniert nicht ohne Standards und da mangelt es leider in FHEM.

Grüße

[Phill]

Ich hätte da noch eine Bitte! Und zwar ist mein Modul Talk2Fhem mittlerweile in einem echt zufriedenstellenden Stadium, und ich würde es gerne offiziell über SVN verfügbar machen. Jetzt habe ich von Rudolf König wegen der Schreibrechte die Antwort erhalten, dass sich das Modul noch ein Entwickler bezüglich Qualitätsanforderungen/Coaching anschauen soll.
Wer könnte sich dem mal annehmen?

https://forum.fhem.de/index.php?topic=80960.msg730163#msg730163

Grüße

[Markus Bloch]

Ich hab mir mal dein Modul durchgeschaut. Folgende Sachen sind mir aufgefallen:

- Das Lesen und Schreiben von Dateien (hier dein Configfile) führst du direkt via open()/close() durch. Hier bitte die Funktionen FileRead() und FileWrite() benutzen (https://wiki.fhem.de/wiki/DevelopmentModuleAPI#Daten_dauerhaft_schreiben.2Flesen).
Hintergrund: Nutzer die configDB benutzen können die Datei in ihre configDB übernehmen. Die Dateien werden dann direkt aus der configDB gelesen.

- Zeile 432 bitte ändern in:

Code Auswählen Erweitern

$hash->{helper}{phrase} = undef;
Hintergrund: Nach Möglichkeit immer mit den übergebenen Parametern direkt arbeiten, Da dies die offizielle Schnittstelle ist.

- Den Aufruf von AnalyzeCommandChain() im Rahmen von Talk2Fhem_Set bitte den Client-Hash mit übergeben:

Code Auswählen Erweitern

my $fhemres = AnalyzeCommandChain ($hash->{CL}, $fhemcmd) unless (IsDisabled($name));
Hintergrund: Nur so wird geprüft, ob der jeweilige User-Client berechtigt ist, diesen Befehl auszuführen (allowed-Definition)

- Die folgende Zeile dürfte so nicht funktionieren:

Code Auswählen Erweitern

notifyRegexpChanged($hash, join ",",@{$$hash{helper}{notifiers}});
Hintergrund: notifyRegexpChanged() wird ein notify-Ausdruck übergeben. Dies ist ein regulärer Ausdruck. Bei regulären Ausdrücken wird nicht mit einem Komma ver-ODER-t sondern mit einer Pipe. Wenn Du also eine Auflistung an Definitionsnamen hast, auf welche Du in deiner NotifyFn hören willst, müsstest du diese hier mit einer Pipe joinen.

Ansonsten würde ich dir empfehlen deinen Code nochmal sauber einzurücken, da man momentan absolut nicht erkennen kann wo welcher Block beginnt/aufhört. Das erleichtert Dir das Lesen deines eigenen Codes ungemein, wenn du ihn eine längere Zeit nicht mehr anfässt.

Viele Grüße

Markus

[Phill]

Vielen Dank für die Hinweise, habe sie berücksichtigt.

- Den Aufruf von AnalyzeCommandChain() im Rahmen von Talk2Fhem_Set bitte den Client-Hash mit übergeben:

Code Auswählen Erweitern

my $fhemres = AnalyzeCommandChain ($hash->{CL}, $fhemcmd) unless (IsDisabled($name));
Hintergrund: Nur so wird geprüft, ob der jeweilige User-Client berechtigt ist, diesen Befehl auszuführen (allowed-Definition)

Hab ich auch mal so übernommen, muss aber sagen, ich verstehe es nicht. Aus dem Development WIKI werde ich nicht wirklich schlau. Und bei ein paar Versuchen, war $hash->{CL} immer undef. Gibt es da irgendwo noch weitere Informationen zu? Ich habe nichts gefunden. Sollte hier der Verursacher des Set Befehls drin stehen? Also: FHEMWEB, telnet, usw?

Grüße
 

[Markus Bloch]

Ja genau, hier wird der Client-Hash hinterlegt, welcher den Befehl initiiert hat. Im Wiki ist das so direkt nicht beschrieben. Das kann man nur aus dem Quellcode von fhem.pl direkt entnehmen:

Code Auswählen Erweitern

sub
CommandSet($$)
{
  my ($cl, $param) = @_;
  my @a = split("[ \t][ \t]*", $param);
  return "Usage: set <name> <type-dependent-options>\n$namedef" if(int(@a)<1);

  my @rets;
  foreach my $sdev (devspec2array($a[0], $cl)) {

    $a[0] = $sdev;
    $defs{$sdev}->{CL} = $cl if($defs{$sdev});
    my $ret = DoSet(@a);
    delete $defs{$sdev}->{CL} if($defs{$sdev});
    push @rets, $ret if($ret);

  }
  return join("\n", @rets);
}