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