FHEM Forum

Hallo Reinerlein,

ich bin über die Feiertage nun zu einem ersten recht ausführlichen Testen der SONOS Module gekommen.
Vorab: Hut ab für die viele Entwicklungsarbeit, es passiert da wirklich eine Menge im Hintergrund auf Protokollebene und die Einbindung in die FHEM Welt ist wirklich großartig gelungen.

Ich hoffe du fasst dies hier nicht als Minderung der Wertschätzung deiner Arbeit auf, denn das soll es nicht sein. Ich würde gerne dabei unterstützen die Qualität der Module noch weiter zu verbessern und auf ein einheitliches Niveau mit anderen FHEM Modulen im Multimedia/AV Bereich zu heben, damit diese in Teilen vergleichbar werden.

Dazu habe ich einige Punkte zusammengekritzelt. Es ist zugegebenermaßen so viel geworden, dass ich hoffe, dass du mir nachsehen kannst, sollte etwas zu knapp oder grammatikalisch fehlerhaft formuliert sein. Ich fände es prima, wenn wir uns darüber hier austauschen könnten  :) 

Allgemein habe ich folgende offenen Punkte für mich gefunden:

   
• Attribut "webname" der jeweils angesprochenen FHEMWEB Instanz wird nicht beachtet (z.B. bei Verwendung von Proxy; /fhem ist auch statisch in Readings enthalten, bspw. in currentAlbumArtURL, nextAlbumArtURL)

   
• set/get Befehle werden in FHEM normalerweise klein geschrieben. Zumindest sollte Groß/Kleinschreibung nicht unterschieden werden (lc() in Perl verwenden). Bei einem Befehlnamen, der aus mehreren Wörtern besteht, ist der erste Teil klein, danach der Anfangsbuchstabe je Wort groß (z.B. pauseAll, stopAll)

   
• für die Groß-/Kleinschreibung von Readings gilt entsprechend das gleiche wie für set/get Befehle (ist teilweise auch so, aber nicht durchgängig)

   
• die get/set Befehle, welche sowohl in SONOS als auch SONOSPLAYER enthalten sind, sollten einheitlich benannt sein, wenn sie die gleiche Funktion übernehmen (z.B. pause statt pauseAll, stop statt stopAll in 00_SONOS.pm). Das ermöglicht z.B. eine bessere Einbindung in structure-Devices. Es ergibt sich aus dem angesprochenen FHEM Device, ob nur ein SONOSPLAYER Gerät einzeln oder das gesamte SONOS System angesprochen wird.

   
• set/get-Befehle, die kein Argument erwarten, sollten mit :noArg im Code versehen werden, damit in FHEM Web direkt ersichtlich ist, dass nichts weiter erwartet wird.

   
• wenn eine direkte Verbindung zwischen einem set-Befehl und einem Reading besteht, sollte der set-Befehl exakt mit dem Namen des Readings übereinstimmen. Außerdem sollte sich der Wert des Readings direkt als Eingabe für den set-Befehl verwenden lassen. FHEM kann bietet dann automatisch entsprechende Widgets für's Handling an (bzw. mit kleinen Kniffen im Returncode bei Aufruf von "set DEVICE ?", hast du teilweise schon drin; treble, bass etc fehlt z.B. noch).

   
• bei Readings, die eine direkte Verbindung zum set-Befehl haben, sollte als TRUE/FALSE Value nicht 1/0 verwendet werden, sondern besser on/off, damit es freundlicher in der Nutzung des set-Befehls ist (z.B. loudness, mute, repeat, shuffle).

   
• die Werte von Readings werden zumeist eher in Kleinschreibung dargestellt (mit nur sinnvollen Ausnahmen). Bspw. "present" statt "Present", "on" statt "On" usw.

   
• die Benennung jener Attribute, welche sich auf Readings beziehen, sollten mit dem Readingsnamen beginnen und dann gefolgt von ihrer Nutzung (z.B. volumeMin statt minVolume, volumeMax statt maxVolume)

   
• das Attribut "stateVariable" erscheint mir überflüssig; man kann mit dem FHEM-weiten Attribut "stateFormat" das gleiche erreichen und der Nutzer bleibt zusätzlich flexibel in der Anpassung, was er als STATE für das Device angezeigt haben will. Entweder du verzichtest auf das Reading "state" und setzt stateFormat=presence oder (besser) du orientierst dich mehr am FHEM-AV-Standard.
  Ich würde presence auf "present" oder "absent" setzen und "state" abhängig davon entsprechend auf absent/play/stop oder was sonst noch Sinn macht für das, was Sonos gerade tut. Vermutlich das, was du unter transportState anzeigst. dieses Attribut könnte dann ggf. wegfallen.

   
• die Bennung des Readings "location" finde ich etwas unglücklich gewählt, da diese Bezeichnung zumeist eher mit Bezug auf Geo-Koordinaten o.ä. belegt ist. Besser (und sinnvoller) fände ich ein Reading "ipAddress" oder ähnliches, wo dann auch nicht die ganze URL drin steht, sondern eben nur die IP. Der Vollständigkeit halber könnte man den verwendeten Port unter den Internals darstellen

   
• FHEM-AV-Kompatibilität: mute/muteT, volume/volumeD/volumeU

   
• Log3 Levels sind häufig zu hoch (bzw. nummerisch zu niedrig :-)): Debug-Nachrichten sollten >3 haben. Aktuell werden offensichtlich zu Debugging-Zwecken gedachte Meldungen als Level 0 oder 1 ausgegeben, was zu einem übergroßen Logfile führt. Auch sollte optimalerweise am Anfang immer der Name des FHEM-Gerätes stehen, welches den Log-Eintrag erzeugt (aktuell steht dort nur SONOS0 oder SONOS1 ohne wirklichen Bezug).
  Ich selbst erzeuge Debug-Meldungen, welche quasi "raw" sind, mit Level5 und verarbeitete Meldungen mit Level4. Level3 sind nur einmalige Meldungen beim Booten und die Wiedergabe von set-Befehlen, sonst nichts. Level 0-2 verwende ich IIRC überhaupt nicht.

Einige Dinge wären dagegen "nice to have" und sind lediglich eine Anregung:

   
• SONOSPLAYER sollte ein alias-Attribut gesetzt haben, z.B. damit statt Sonos_Raumname nur Raumname dort steht oder PLAY:1, PLAY:3, PLAY:5 ...
• SONOSPLAYER sollte ein devIconState Attribut gesetzt haben, um ein Icon statt Text für den Status anzuzeigen (z.B. "appeared:rc_GREEN .*:rc_STOP")
• Bei der Nutzung von "speak" wäre es prima, wenn ein aktuelles Playback automatisch angehalten und es nach abspielen der Nachricht anschließend wieder fortgesetzt würde. Dies würde eine direkte Nutzung des Speak Befehls ermöglichen, bspw. in Bestandscode bei der Nutzung anderer FHEM-AV-kompatibler Geräte.

Dann wären da noch einige allgemeine Fragen, die sich mir auftaten:

   
• sollte der Default für characterDecoding nicht besser UTF-8 sein?
• wofür ist pingType gedacht bzw. was ist dessen Default-Value? Die Nutzung von "syn" schien bei mir FHEM sehr häufig und sehr lang zu blockieren. Ohne das Attribut habe ich dies nicht (so stark) beobachten können.
• mir ist spontan ohne großartig zu schauen nicht ganz klar, weshalb du Readings über userReadings erzeugst, statt das ganze Handling intern in deinem Modul zu handhaben.

Letztendlich noch einen Blick auf den Bereich Sicherheit:

   
• wird bei Nutzung des Proxy sichergestellt, dass man darüber nicht einfach beliebige URLs aufrufen kann, sondern nur zu den Sonos Geräten selbst und optimalerweise dort auch nur zu bestimmten URIs zum Zwecke des Artwork Abrufs?

Ich möchte gerne anbieten, dass ich bei der Verbesserung in den genannten Punkten unterstütze, sofern du das möchtest. Sei es durch Patches, Tests oder einfach nur Beistand  ;) 






Gruß
Julian

Hi Loredo,

danke für die Ideen und Hinweise. Bei einigen hatte ich mir auch schon Gedanken gemacht, bei anderen eher weniger :)

Ich versuche mal zu den Themen was zu schreiben:

• Thema webname: Das kann ich leider nicht beachten, da im SubProzess ja noch gar kein Aufruf über die Oberfläche gemacht wird. Erst wenn der Anwender das ganze mit seinem Browser aufruft, ist bekannt, über welche Instanz er das tut. Dann sind die Readings allerdings bereits gesetzt.
  Das einzige, was ich tun kann, wäre ein eigenes Attribut dafür anzubieten, damit man das sozusagen etwas beeinflussen kann.
• Kleingeschrieben getter/setter: Die sind jetzt alle Case-Insensitive
• Groß-/Kleinschreibung der Readings: Das hatte mal das System, dass die Titelinformationen mit "current", "info" und "next" beginnend in klein beginnen, und die anderen (damals Hauptsächlich "Systeminfos" wie Versionsnummer usw.) groß. Das ist bis auf drei/vier Readings, die eigentlich groß gehören auch immer noch so.
  Nur kann man das natürlich nur mit erheblichem Aufwand bei den Benutzern (und auch auf meiner Seite) anpassen, weswegen ich das so lassen würde.
• Einheitliche Setter bei SONOS und SONOSPLAYER: Ich habe jetzt die beiden ("PauseAll" und "StopAll") dupliziert zu "Pause" und "Stop".
• Getter/Setter ohne Argument wurden entsprechend markiert, und haben auf der Oberfläche kein Eingabefeld mehr.
  Des Weitern habe ich den anderen Gettern/Settern soweit möglich eine Auswahlliste spendiert
• Direkt-Verbindung zwischen Setter und Reading: Soweit ich das sehen konnte, gab es nur bei "Icon" das Problem. Diesen Setter habe ich in "RoomIcon" umbenannt, da ich davon ausgehe, dass das sowieso kaum einer verwendet (zumindest nicht automatisiert).
• Thema True/False: Man konnte schon immer für "1" oder "0" auch "on" oder "off" angeben. Das war nur nicht dokumentiert. Ich habe die Auswahllisten der entsprechenden Setter nun mit "on"/"off" gefüllt.
• Die Werte der Readings sind zumeist direkt die Daten von den Playern. Das einzige selbst beschriebene Reading ist "Presence", und das möchte ich jetzt nicht mehr anders schreiben, da das sehr oft verwendet werden dürfte (z.B. überall im Modul selbst).
• Benennung der Attribute die sich auf Readings beziehen: Hier ist die interne Logik Auslöser für die Schreibweise. Es gibt z.B. auch die Attribute "minVolumeHeadphone" usw., die durch Aneinanderkettung im Code geprüft werden.
  Auch hier: Es dürfte oft verwendet werden, und würde einen Haufen Arbeit beim Umbenennen nach sich ziehen.
• Das Attribut "stateVariable": Es wurde geschaffen, als es die Möglichkeit in Fhem selber noch lange nicht gab. Jetzt dürfte es einigermaßen überflüssig sein, ausser das es wahrscheinlich etwas schneller verarbeitet wird, was aber nicht so wichtig sein dürfte. Aber es stört ja auch nicht. Die neue Variante kannst du ja auch verwenden...
• Das Reading "location": Es ist aus dem UPnP-Part übernommen, dort heißt das so. Im Reading steht die Position des Device-Beschreibungsdokuments. Man kann es verwenden (und ich tue das intern auch reichlich), um selber eine Verbindung zum Player aufzubauen.
• Fhem-AV-Kompatibilität: Da habe ich nicht drauf geachtet, zumal diese auch nach dem Sonos-Modul geschrieben wurde. Dazu habe ich schon mehrfach meine Meinung in irgendwelchen Posts kundgetan. Ich betrachte solch eine "Vereinheitlichung" als nicht relevant. Es wird in meinen Augen kein Nutzen erzielt. Wenn man ein Modul neu entwickelt, ist sowas sicher hilfreich um zumindest mal ähnliche Namen zu haben, aber bestehendes darauf anpassen... davon halte ich nicht viel...
• Loglevel: Nun ja, jedem steht es frei, den Verbose-Level auf 0 zu setzen. Dann hast du nur ein paar wenige Zeilen im Log. Die Zahlen hinter dem Wort SONOS beziehen sich auf die Thread-Nummer, die da loggt. Ein Device kann ich nur selten angeben, da im SubProzess solche Unterscheidungen nicht wichtig sind. Im Fhem-Log bezieht es sich schon wegen der Verwendung des Standard-Logging-Mechanismusses auf die korrekten Devices...
• Attribut "alias" für Sonosplayer: Bei der Standarderzeugung der Playerdevice wird dieses nun mit erzeugt. Aber natürlich kannst du es dir ja auch nachträglich setzen.
• Das mit dem "devStateIcon" habe ich noch nie verwendet. Da es aber ein Standard-Fhem-Attribut ist, solltest du es doch wie überall setzen können, oder?
• Zum Thema Speak: Hast du es denn schon mal verwendet :)? Das passiert natürlich, sonst hätte die Funktion ja keinen Sinn... Du kannst auch selber eine MP3-Datei temporär abspielen lassen (mit "PlayURITemp"), dort wird nach Beendigung auch der alte Zustand wieder hergestellt. Pass nur auf, wenn du Streams übergibst. Dann kommt er natürlich nicht wieder zurück zum alten Musikstück...
• Das CharacterDecoding bezieht sich auf das Decoding (das "De" beachten) der Informationen vom Player. Diese werden vom Player vorgegeben, und wurde von mir nur zur besseren Erforschbarkeit eingebaut. Normalerweise musst du das nie setzen...
• Thema pingtype: Die verschiedenen Varianten funktionieren unterschiedlich gut auf verschiedenen Systemen. Deshalb gibt es eine Auswahl dazu. Den geringsten Aufwand hat das System mit "syn", da keine Antwort des Empfängers abgewartet wird, sondern nur geprüft wird, ob eine Route zur ARP-Adresse des Players existiert (das beantwortet der Switch direkt selber). Leider haben gute Switches damit ein Problem, da diese einen langen ARP-Cache haben, und somit Devices immer noch als Aktiv gemeldet werden, obwohl diese bereits aus sind.
  Ich verwende gerne "icmp", das ist das Standard-Ping-Verfahren, benötigt allerdings root-Rechte für den ausführenden Prozess (hier Fhem).
• Thema userReadings: Diese benötigen Zeit in der Verarbeitung, sodass ich dem Anwender die Möglichkeit lassen will, diese auch einfach wieder zu entfernen. Viele nutzen die Funktionalität der abfragbaren Listen nicht. Die können an dieser Stelle optimieren, bzw. das auch so anpassen, wie sie es brauchen...
• Thema Proxy: Ich habe es so gebaut, dass der Durchgriff nur auf die Zoneplayer (und auf die Spotify-Cover-API) erfolgen kann. Und das auch nur, wenn man das Attribut "generateProxyAlbumArtURLs" aktiviert hat. Da wird aber nichts weiter mehr in der Player-URL beschränkt. Wenn man da also keinen Zugriff möchte, sollte man den aus lassen, bzw. weiter vorne einen Schuz sicherstellen.
  Ausserdem gibt es noch die Möglichkeit Standard-Cover auszuliefern. Das sind diese Platzhalter-Bildchen vom Sonos-Controller. Diese sind fest kodiert im Modul...

Soo... einiges geschrieben :)
Leider kann ich nicht alles anpassen. Das Ding ist ja schließlich in Benutzung. Aber wo es einfach geht, ist es jetzt eingecheckt.

Grüße
Reinerlein

Hi Reinerlein,


merci für die prompte Antwort, da kam ich jetzt noch gar nicht hinterher ;-)

Zitat von: Reinerlein am 02 Januar 2015, 18:17:53
Thema webname: Das kann ich leider nicht beachten, da im SubProzess ja noch gar kein Aufruf über die Oberfläche gemacht wird. Erst wenn der Anwender das ganze mit seinem Browser aufruft, ist bekannt, über welche Instanz er das tut. Dann sind die Readings allerdings bereits gesetzt.
Das einzige, was ich tun kann, wäre ein eigenes Attribut dafür anzubieten, damit man das sozusagen etwas beeinflussen kann.

Beeinflussen allein genügt ja nicht, sonst wäre man ja auf eine bestimmte FHEM Instanz festgelegt (gesetz dem Fall man möchte beim Zugriff auf die eine Instanz vermeiden, dass Inhalte auch über die andere geladen werden). Klassisches Beispiel ist die Nutzung eines Reverse-Proxy, wie man ihn üblicherweise zwecks Absicherung und besserer Integration vor FHEM installiert. generateProxyAlbumArtURLs wird damit obligatorisch. Mir ist nicht klar weshalb du im Proxy-Bereich nicht dynamisch die URL generierst und sie stattdessen statisch im Reading festlegst? Mein Vorschlag wäre, im Reading das Prefix /fhem wegzulassen und zur Laufzeit, wenn das Proxy-Modul angesprochen wird, aus attr{global}{webname} vorne ranzustellen.

Zitat von: Reinerlein am 02 Januar 2015, 18:17:53
Kleingeschrieben getter/setter: Die sind jetzt alle Case-Insensitive

Ich weiß, es ist wie Korinthen kacken, aber die über FHEMWEB angezeigten getter/setter sollten dann ebenfalls der Kleinschreibungsregel folgen. Nun da im Code nicht mehr casesensitiv gearbeitet wird, gibts da auch keine Probleme für Bestandsinstallationen. Das sorgt für eine einheitliche Sortierreihenfolge aller getter/setter im Menü und somit wäre das Verhalten dann tatsächlich so, wie es bei anderen Modulen auch der Fall ist (Stichwort Vergleichbarkeit, Nutzergewohnheiten etc.)

Zitat von: Reinerlein am 02 Januar 2015, 18:17:53
Groß-/Kleinschreibung der Readings: Das hatte mal das System, dass die Titelinformationen mit "current", "info" und "next" beginnend in klein beginnen, und die anderen (damals Hauptsächlich "Systeminfos" wie Versionsnummer usw.) groß. Das ist bis auf drei/vier Readings, die eigentlich groß gehören auch immer noch so.
Nur kann man das natürlich nur mit erheblichem Aufwand bei den Benutzern (und auch auf meiner Seite) anpassen, weswegen ich das so lassen würde.

Ich biete mich da gerne an das anzupassen. Es ist auch mehr eine optische Sache so wie oben auch genannt (Sortierreihenfolge etc.).
Programmatisch dürfte es keinen Unterschied für Bestandsinstallationen machen, da die Readings nicht casesensitiv sind soweit ich weiß.

Zitat von: Reinerlein am 02 Januar 2015, 18:17:53
Einheitliche Setter bei SONOS und SONOSPLAYER: Ich habe jetzt die beiden ("PauseAll" und "StopAll") dupliziert zu "Pause" und "Stop".

Aus Convenincegründen würde ich die pauseAll und stopAll in FHEMWEB ausblenden und nur zur Erhaltung der Kompatibilität im Programmcode behalten (kommt wieder den Nutzergewohnheiten zugute).

Zitat von: Reinerlein am 02 Januar 2015, 18:17:53
Direkt-Verbindung zwischen Setter und Reading: Soweit ich das sehen konnte, gab es nur bei "Icon" das Problem. Diesen Setter habe ich in "RoomIcon" umbenannt, da ich davon ausgehe, dass das sowieso kaum einer verwendet (zumindest nicht automatisiert).

Um das nicht automatisierte geht es hier ja auch :-)
Es geht darum den aktuellen Wert in FHEMWEB vorausgewählt zu bekommen. Ist jetzt ja auch so, prima.

Zitat von: Reinerlein am 02 Januar 2015, 18:17:53
Thema True/False: Man konnte schon immer für "1" oder "0" auch "on" oder "off" angeben. Das war nur nicht dokumentiert. Ich habe die Auswahllisten der entsprechenden Setter nun mit "on"/"off" gefüllt.

Hier und dort fehlt es wohl noch. Nur die Auswahllisten anzupassen genügt hier allerdings nicht, auch die Readings müssen dann on/off statt 1/0 ausgeben, damit die Vorauswahl entsprechend funktioniert.

Zitat von: Reinerlein am 02 Januar 2015, 18:17:53
Die Werte der Readings sind zumeist direkt die Daten von den Playern. Das einzige selbst beschriebene Reading ist "Presence", und das möchte ich jetzt nicht mehr anders schreiben, da das sehr oft verwendet werden dürfte (z.B. überall im Modul selbst).

Ich schau mal, dass ich da einen Patch für schreibe. Wenn die Readings-Namen in Befehlen an SONOS wiederverwendet werden, sind diese dann case-insensitive?

Zitat von: Reinerlein am 02 Januar 2015, 18:17:53
Benennung der Attribute die sich auf Readings beziehen: Hier ist die interne Logik Auslöser für die Schreibweise. Es gibt z.B. auch die Attribute "minVolumeHeadphone" usw., die durch Aneinanderkettung im Code geprüft werden.
Auch hier: Es dürfte oft verwendet werden, und würde einen Haufen Arbeit beim Umbenennen nach sich ziehen.

Hm, das ist jetzt unglücklich. Da das Modul im SVN ja neu ist, wäre jetzt die richtige Zeit das noch zu ändern, bevor es mehr gefestigt ist. Ich finde es aus Sicht der Nutzergewohnheiten (kenne ich ein AV Modul, kann ich auch andere direkt in den gleichen Funktionen bedienen) sehr wichtig, dass es hier einheitlich wird.
Ich schaue auch hier mal, ob ich bei Gelegenheit einen Patch bereitstellen kann (ggf. inkl. Rückwärtskompatibilität).

Zitat von: Reinerlein am 02 Januar 2015, 18:17:53
Das Attribut "stateVariable": Es wurde geschaffen, als es die Möglichkeit in Fhem selber noch lange nicht gab. Jetzt dürfte es einigermaßen überflüssig sein, ausser das es wahrscheinlich etwas schneller verarbeitet wird, was aber nicht so wichtig sein dürfte. Aber es stört ja auch nicht. Die neue Variante kannst du ja auch verwenden...

Aus FHEM Sicht ist das Modul eigentlich neu, weil es neu im SVN als offizielles Modul angeboten wird. Ich bin kein Fan davon Altlasten gleich von Beginn an drin zu lassen, vor allem wenn sie Funktionen doppeln, die in FHEM zentral bereitgestellt sind. Die Nutzergewohnheiten sollten hier höher priorisiert werden. Nutzer kennen von anderen Modulen her stateFormat schon, daher sollte es auch hier keine verwirrende Dopplung geben. Außerdem: Sollte sich an stateFormat einmal etwas ändern, müsstest du das aus Konsistenzgründen in stateVariable nachziehen. Das macht für mich keinen Sinn.

Zitat von: Reinerlein am 02 Januar 2015, 18:17:53
Das Reading "location": Es ist aus dem UPnP-Part übernommen, dort heißt das so. Im Reading steht die Position des Device-Beschreibungsdokuments. Man kann es verwenden (und ich tue das intern auch reichlich), um selber eine Verbindung zum Player aufzubauen.

Ok, das verstehe ich. Dann wünsche ich mir ein zusätzliches Reading, welches nur die IP Adresse beinhaltet. Man mag das Reading location ja direkt für die Abfrage des einen XMLs nutzen können, aber schon für das Schicken von Befehlen muss ich doch den String schon wieder selbst auseinandernehmen und anders zusammensetzen (machst du sicherlich im Code auch so). Da wäre es besser, die zentralen Bestandteile als Readings einzeln bereitzustellen, damit nicht jeder Nutzer in seinem myUtils Code den String erst selbst auseinanderfieseln muss (wieder das Stichwort Convenience).

Zitat von: Reinerlein am 02 Januar 2015, 18:17:53
Fhem-AV-Kompatibilität: Da habe ich nicht drauf geachtet, zumal diese auch nach dem Sonos-Modul geschrieben wurde. Dazu habe ich schon mehrfach meine Meinung in irgendwelchen Posts kundgetan. Ich betrachte solch eine "Vereinheitlichung" als nicht relevant. Es wird in meinen Augen kein Nutzen erzielt. Wenn man ein Modul neu entwickelt, ist sowas sicher hilfreich um zumindest mal ähnliche Namen zu haben, aber bestehendes darauf anpassen... davon halte ich nicht viel...

Ich finde es wichtig, Nutzergewohnheiten zu respektieren und ihm die Möglichkeit zu geben, einmal gelerntes auch auf andere Module anzuwenden. Es macht für mich aus Nutzersicht keinen Sinn, dass die gleiche Funktion bei jedem Modul anders heißt, obwohl sie das gleiche tut. Da muss ich jedes Mal erst (einige länger als andere) in der Commandref studieren nur um zu schauen, wie jetzt das Äquivalent zu einem Befehl ist, den ich von einem anderen Modul schon kenne.
Es ist einfach eine Frage der Nutzerfreundlichkeit. Als netten Nebeneffekt kann man zudem Automatisierungscode, den man in seiner myUtils Datei schon für ein anderes AV Gerät geschrieben hat, dann ganz einfach für das Sonos Modul wiederverwenden (oder gar als generalisierte Funktion definieren, ohne dass man auf spezielle Eigenheiten einiger Geräte Rücksicht nehmen müsste).

Zitat von: Reinerlein am 02 Januar 2015, 18:17:53
Loglevel: Nun ja, jedem steht es frei, den Verbose-Level auf 0 zu setzen. Dann hast du nur ein paar wenige Zeilen im Log. Die Zahlen hinter dem Wort SONOS beziehen sich auf die Thread-Nummer, die da loggt. Ein Device kann ich nur selten angeben, da im SubProzess solche Unterscheidungen nicht wichtig sind. Im Fhem-Log bezieht es sich schon wegen der Verwendung des Standard-Logging-Mechanismusses auf die korrekten Devices...

Dann mache ich wohl was falsch.
Der Standardloglevel einer neuen FHEM Installation ist 3. Alle Module, die ich kenne, geben in diesem Level keinerlei Debug- oder unnötige Info-Meldungen aus, ohne dass vorher eine Nutzeraktion stattgefunden hätte.
Aber selbst wenn ich bei allen Sonosplayer Geräten und dem Sonos Gerät verbose=0 setze, bekomme ich noch solche unnötigen Ausgaben wie diese hier:


https://gist.github.com/jpawlowski/c416de56a6e7131338e9


Dies ist jetzt mal das Logfile, welches lediglich bisher in diesem Jahr durch Sonos entstanden ist. Gewaltig!


Ich kann dort nicht erkennen, dass der Name eines FHEM Devices dort erwähnt würde oder dass die Ausgaben gar meinem explizit gesetzten Verbose-Level entsprechen.


So nervig es ist und so unnötig es dir aus funktionaler Sicht erscheinen mag: Dem FHEM Standard entspricht das hier nicht.

Zitat von: Reinerlein am 02 Januar 2015, 18:17:53
Das mit dem "devStateIcon" habe ich noch nie verwendet. Da es aber ein Standard-Fhem-Attribut ist, solltest du es doch wie überall setzen können, oder?

Klar, geht auch. War nur ein Vorschlag, wie du deine automatisch angelegten Sonos Devices in den Standardsettings noch verbessern könntest (again, more convenient )

Zitat von: Reinerlein am 02 Januar 2015, 18:17:53
Zum Thema Speak: Hast du es denn schon mal verwendet? Das passiert natürlich, sonst hätte die Funktion ja keinen Sinn... Du kannst auch selber eine MP3-Datei temporär abspielen lassen (mit "PlayURITemp"), dort wird nach Beendigung auch der alte Zustand wieder hergestellt. Pass nur auf, wenn du Streams übergibst. Dann kommt er natürlich nicht wieder zurück zum alten Musikstück...

Ich habe das natürlich schon ausprobiert und war wie gesagt erstaunt, dass es eben nicht so wie erwartet funktioniert hat.
Im Logfile stand dann immer sowas:



2015.01.01 16:08:04 3: SONOS1: Temporary playing of "/RINCON_B8E93722EB1001400_MR_Speak.mp3" must wait, because another playing is in work...



Aber selbst wenn ich dann manuell die Wiedergabe gestoppt habe wurde das File nicht abgespielt. Das scheint dann wohl irgendwie ein Bug zu sein, bei dem ich nicht weiß wie ich die Ursache herausfinde

Zitat von: Reinerlein am 02 Januar 2015, 18:17:53
Thema pingtype: Die verschiedenen Varianten funktionieren unterschiedlich gut auf verschiedenen Systemen. Deshalb gibt es eine Auswahl dazu. Den geringsten Aufwand hat das System mit "syn", da keine Antwort des Empfängers abgewartet wird, sondern nur geprüft wird, ob eine Route zur ARP-Adresse des Players existiert (das beantwortet der Switch direkt selber). Leider haben gute Switches damit ein Problem, da diese einen langen ARP-Cache haben, und somit Devices immer noch als Aktiv gemeldet werden, obwohl diese bereits aus sind.
Ich verwende gerne "icmp", das ist das Standard-Ping-Verfahren, benötigt allerdings root-Rechte für den ausführenden Prozess (hier Fhem).

FHEM als root laufen zu lassen kommt bei einer ernsthaften Installation IMHO halt nicht in Frage, da es die Sicherheit zu stark gefährdet.
Ich hatte mit gesetzten pingtype generell das Problem, dass FHEM häufig blockiert war. Nicht dass Geräte noch da gewesen wären, die an waren.
Ich schalte auch meine Sonos nie aus, brauche das also eigentlich nicht. Was ich mir ursprünglich erhofft hatte war, dass die Erkennung der Sonos Geräte nach dem Neustart von FHEM schneller ginge. Das dauert bei mir oftmals bis zu 5 oder 10 Minuten, bis die Geräte wieder angesprochen werden können.

Zitat von: Reinerlein am 02 Januar 2015, 18:17:53
Thema userReadings: Diese benötigen Zeit in der Verarbeitung, sodass ich dem Anwender die Möglichkeit lassen will, diese auch einfach wieder zu entfernen. Viele nutzen die Funktionalität der abfragbaren Listen nicht. Die können an dieser Stelle optimieren, bzw. das auch so anpassen, wie sie es brauchen...

Ah, ok. Die Intention ist gut, nutzerfreundlicher fände ich es allerdings das über ein einfaches Attribut an-/abschaltbar zu haben.

Zitat von: Reinerlein am 02 Januar 2015, 18:17:53
Thema Proxy: Ich habe es so gebaut, dass der Durchgriff nur auf die Zoneplayer (und auf die Spotify-Cover-API) erfolgen kann. Und das auch nur, wenn man das Attribut "generateProxyAlbumArtURLs" aktiviert hat. Da wird aber nichts weiter mehr in der Player-URL beschränkt. Wenn man da also keinen Zugriff möchte, sollte man den aus lassen, bzw. weiter vorne einen Schuz sicherstellen.

Ok. Ich habe etwas hin und her probiert und es scheint wirklich nicht alles durchgereicht zu werden. Denke das langt und für den Zugriff auf FHEM übers Internet per Reverse-Proxy ist ohnehin eine Nutzerauthentifizierung erforderlich.








Gruß
Julian

Hi Julian,

ok, ich werde das mal durchgehen... aber etwas vorweg:
Das Modul ist nicht neu. Es dürfte sogar das älteste Multimedia-Modul in Fhem sein (Initial Release mit den ersten, für mich lauffähigen, Funktionalitäten war im Januar 2013, also etwa zwei Jahre her).

Das bedeutet, dass es viele Menschen gibt, die das Modul auch solange verwenden, und ihren Code entsprechend aufgebaut haben.
Alle Änderungen müssen damit eindeutig abwärtskompatibel sein! Deshalb z.B. auch die Verdoppelung der Pause-Anweisung.

Ich möchte nicht, dass irgendwelche Readings (die meiner Meinung nach Case-Sensitive sind, da es Perl-Hashes sind) umbenannt werden, nur um irgendwelchen ästhetischen Ansprüchen zu genügen. Das gleiche gilt für die Anzeige der Befehle. Das ist schließlich zum einen Geschmackssache, und zum anderen viel Anpassungsaufwand...

Die Anpassung der Readings im Quellcode ist sehr(!) weitreichend und hat zunächst mal einen nicht überschaubaren Prüfaufwand zur Folge. Des weiteren natürlich die Folgen bei den Anwendern...
Ich möchte diesen Aufwand eigentlich weder in der Entwicklung noch auf Anwenderseite haben.

Aber schon mal konkret zu einigen Punkten, die ich ad hoc beantworten kann:
Das Logging: Du kannst am Sonos-Device das Verbose-Attribut auf 0 setzen. Das sollte eigentlich gehen, ansonsten ist das ein Bug. Deine Logausgaben habe ja alle den Level 1-3 gehabt. Die einzigen 0-er Logs waren Fehlermeldungen, dass der Port bereits belegt war (vermutlich nach einem Shutdown-Restart oder Rereadcfg), und er deswegen warten muss. Das sollte auf dem Level auch rauskommen.
Debug-Ausgaben kommen im Normalfall ab Level 4 und 5, wobei es dort noch eine Unterscheidung gibt, da bei 5 erhebliche Ausgaben erfolgen...

On/Off-Setter: Das habe ich wieder umgestellt auf 0/1. Da es im Code sehr oft verwendet (if ReadingsVal()), und in vielen eigenen Codeschnipsel so verwendet wird, ist die 0 oder 1 im Reading wichtig. Die Auswahl sollte dementsprechend passend die Zahlen anbieten. In eigenen Aufrufen kann man natürlich weiterhin on/off verwenden...

Reading-Patch: Was möchtest du denn da patchen? Das Wort "appeared" bzw. "disappeared" sollte wegen der Abwärtskompatibilität erhalten bleiben. Sonst kannst du dir da ja auch ein userReading zu machen...

Zu der location: Das braucht ein Anwender niemals. Auch ich verwende es nur, um das Beschreibungsdokument zu laden, und die IP-Adresse für den pingCheck zu verwenden. Nicht vergessen: Das läuft über UPnP. Keine direkte IP-Kommunikation im Modul selbst, alles per UPnP-Proxy gekapselt.
Der Ping ist nur zum Feststellen von abwesenden Playern, da diese beim Abschalten nix mehr senden (obwohl UPnP das vorsehen würde).
Diese Prüfung kannst du mit dem pingType auf "none" abschalten. Dann erfährst du aber nie, ob ein Player weg ist, und du erhältst u.U. Fehlermeldungen beim Ausführen eines Befehls.
Ob es schlau ist Fhem als root laufen zu lassen, muss jeder selbst entscheiden. Ich für meinen Teil betrachte das als absolut problemfrei, da auf meiner Fhem-Maschine nichts anderes läuft...
Wenn man das nicht will, kann man die anderen Ping-Verfahren ausprobieren, oder es halt abschalten...

Zu dem userReading bzgl. LastActionResult: Wenn die Komponenten einmal angelegt sind, werden sie vom Modul nicht mehr neu erzeugt. Du kannst also anpassen und löschen was du möchtest. Das initiale Anlegen ist nur eine Grundvorlage, und kann/sollte von den Anwendern angepasst werden, das bedeutet, dass man für das Erzeugen dieses userReading eigentlich auch kein Attribut braucht...

Zu dem Speak: Der Mechanismus im Hintergrund verwendet den Befehl "PlayURITemp", dieser kann nur einmal gleichzeitig pro Player gestartet werden. Hast du da vielleicht etwas in der Richtung zuvor gestartet?

Zu dem Rest schauen wir mal... Das ist ja auch ein Diskussionsforum :)

Grüße
Reinerlein