andig / homebridge-fritz

Homebridge platform for Fritz!Box router and supported DECT devices
MIT License
74 stars 22 forks source link

Restrict characteristic values for thermostat to be valid and enforce HomeKit thermostat behavior #82

Closed Supereg closed 4 years ago

Supereg commented 4 years ago

This PR makes the following changes. It enforces the behavior of a thermostat as it is defined by the HomeKit Accessory Protocol spec. This means the thermostat supports exact two states: heating and off (as the fritz thermostats are only able to heat and definitely can't support cooling). Also this PR properly defines the minimum and maximum temperatures the fritz thermostat is able to support (from 8-28 degree Celsius). This PR also improves data collection for thermostats on first startup.

Any feedback is appreciated :)

~ Andi (lol)

andig commented 4 years ago

Could I ask you to park this until #80 has landed?

Supereg commented 4 years ago

No problem for me. I'm using my modified version anyways. One thing which is missing here is that the thermostat reports the current heating state correctly (CurrentHeatingCoolingState characteristic) I'm not really informed about the possibilities of the fritz api. Is there a way to query the actual state of the valve of the thermostat in order to check if the thermostat ist actually heating or on idle. This information is represented by the CurrentHeatingCoolingState characteristic. This characteristic currently only reports what was set in the TargetHeatingCoolingState characteristic. Thats probably the only thing which is missing in this PR.

andig commented 4 years ago

Would appreciate if you could take a look at #80 anyway.

The key part is always executing the callback early and only do the actual device transaction later/async. Reason being that otherwise- especially in scenarios with high number of devices- it would lead to timeouts.

Supereg commented 4 years ago

Would appreciate if you could take a look at #80 anyway.

The key part is always executing the callback early and only do the actual device transaction later/async. Reason being that otherwise- especially in scenarios with high number of devices- it would lead to timeouts.

This behavior seems to be already enforced, isn't it? I noticed that you immediately return the callback without waiting for the actual value to return. And I didn't change anything about this behavior 🤔

Supereg commented 4 years ago

No problem for me. I'm using my modified version anyways. One thing which is missing here is that the thermostat reports the current heating state correctly (CurrentHeatingCoolingState characteristic) I'm not really informed about the possibilities of the fritz api. Is there a way to query the actual state of the valve of the thermostat in order to check if the thermostat ist actually heating or on idle. This information is represented by the CurrentHeatingCoolingState characteristic. This characteristic currently only reports what was set in the TargetHeatingCoolingState characteristic. Thats probably the only thing which is missing in this PR.

Regarding this issue. I think the only way to somewhat support the behavior wanted by HomeKit is to return the CurrentHeatingCoolingState based on the current/target temperature. Don't know how things get handled if you set an temperature offset.

andig commented 4 years ago

I think the only way to somewhat support the behavior wanted by HomeKit is to return the CurrentHeatingCoolingState based on the current/target temperature.

Which is intended by the current implementation. If current temp = "ON" temp, then state is "ON" etc. Afaik there is no way to retrieve the current state from the FB.

Supereg commented 4 years ago

I think the only way to somewhat support the behavior wanted by HomeKit is to return the CurrentHeatingCoolingState based on the current/target temperature.

Which is intended by the current implementation. If current temp = "ON" temp, then state is "ON" etc. Afaik there is no way to retrieve the current state from the FB.

The current temperature returns 'on' when you set the thermostat to maximum temperature in the fritz UI. The thermostat then fully opens the valve with no temperature check. Otherwise the valve gets opened based on the current temperature.

andig commented 4 years ago

@Supereg #80 is merged. Would be great if you could re-assess and rebase.

Supereg commented 4 years ago

Rebased onto master; also changed CurrentHeatingCoolingState calculations to be dependent on the current/target temperature

Supereg commented 4 years ago

Removed accidentally added IDE files and also improved updating the CurrentHeatingCoolingState when setting a new targetTemperature. The PR is now fully working and ready for review

andig commented 4 years ago

Ich versuche es mal auf deutsch- passt das? @jngmrks: ein Test Deinerseits wäre grossartig.

andig commented 4 years ago

@Supereg bitte schau Dir die Kommentare mal ein. Ich verstehe bei einigen der Änderungen nicht wie/warum sie mit der PR Beschreibung zusammen hängen.

jngmrks commented 4 years ago

Ich finde das Interessant und genau so wollte ich das darmals 😅. Da hatten wir mal drüber geschrieben. Den Status heizen und kühlen in Abhängigkeit von der Temperatur und der Zieltemperatur.

Ich würde das gerne Testen, hab aber leider keinen Plan wie? 🤔

andig commented 4 years ago

Ich finde das Interessant und genau so wollte ich das darmals 😅. Da hatten wir mal drüber geschrieben. Den Status heizen und kühlen in Abhängigkeit von der Temperatur und der Zieltemperatur.

Ich würde das gerne Testen, hab aber leider keinen Plan wie? 🤔

Du kannst per npm Deinen Wunwschbranch- in dem Fall aus dem Repo von Supereg installieren.

jngmrks commented 4 years ago

Ok, also ich teste das ganze gerade mal, aber hab da gleich die ersten "Probleme". Sagen wir ich habe 20 Grad und das Thermostat steht auf 15 Grad, dann ist das Thermostat mit 20 Grad Grün und der Status "Heizen: 15 Grad". Alles gut. Stelle ich dann auf 21 Grad wird die neue Zieltemperatur zwar übernommen, aber es bleibt bei 20 Grad Grün. Starte ich die App dann neu ändert sich der Status auf "Auf 21 Grad heizen" und das Thermostat zeigt 20 Grad Orange. So sollte es eigentlich gleich nach dem setzen der neuen Temperatur sein.

Das Thermostat lässt sich nur noch zwischen Heizen und Aus steuern. Perfekt, jedoch stellt AUS das Thermostat auf ZU anstatt auf die Spartemperatur. Ich finde das vom Grundgedanken ja richtig, macht genau das was man auswählt. Ich weiß aber nicht ob das alle so sehen?

Die Zeitsteuerung funktioniert weiterhin wie erwartet, aber das Schaltverhalten sollte dringend angepasst werden, damit man auch sofort sieht was los ist und nicht erst nach einem Neustart der App.

Supereg commented 4 years ago

Supereg bitte schau Dir die Kommentare mal ein. Ich verstehe bei einigen der Änderungen nicht wie/warum sie mit der PR Beschreibung zusammen hängen.

Jo werde später das alles nochmal klar und detailliert erläutern. Hab aktuell grad aber leider die Zeit dazu nicht. Das plugin hier verfolgt grundsätzlich eine andere Strategie und diese versuche ich, wie @jngmrks bereits feststellte, neu zu überdenken beziehungsweise zur Diskussion zu stellen.

Supereg commented 4 years ago

Okay. Um nun mal alle deine Fragen im Allgemeinen zu behandeln (spezifische Sachen werde ich dann bei den jeweiligen Fragen ergänzen). Zur Vollständigkeit halber werde ich alles relativ detailliert ausführen, damit wirklich alle Informationen hier vollständig formuliert sind.

HomeKit

HomeKit Thermostate funktionieren wie folgt (Wohlgemerkt Thermostate. Die Kühlungsfunktion wird hier logischerweise ausgeschlossen). Über die TargetTemperature wird trivialer weiße die gewünschte Raumtemperatur gesteuert, sowie über das characteristic TargetHeatingCoolingState (in unserem Fall ausschließlicher Wertebereich OFF und HEAT) die gewünschte "Aktion". Über das read-only characteristic CurrentHeatingCoolingState gibt das Thermostat wie der Name sagt den aktuellen Zustand zurück (Im Endeffekt den Zustand des Ventils). Somit gibt es für uns drei verschiedene Zustandskombinationen zu betrachten:

Natürlich gibt es auch noch die mögliche Kombination mit Target = OFF; Current = HEAT die im Endeffekt einen Übergangszustand darstellt und für dieses Plugin nicht wirklich umsetzbar ist und somit rausfällt.

Fritz Thermostate und mapping auf HomeKit

Ich glaube euch ist soweit klar, wie Fritz Thermostate Funktionieren. Jedoch zur Vollständigkeit. Fritz Thermostate verfügen über einen TargetTemperature Wertebereich von 8 bis 28 Grad Celsius. Stellt man die Heizstufe bei einer TargetTemperature von 8 Grad ein weiteres mal runter, so versetzt sich das Thermostat in den OFF Zustand (komplett ausgeschaltet; HomeKit Äquivalent wäre hier Target = OFF; Current = OFF). Stellt man die Heizstufe bei einer TargetTemperature von 28 Grad ein weiteres mal rauf, so versetzt sich das Thermostat in den ONZustand, was ein dauerhaftes, TargetTemperature unabhängiges heizen hervorruft (Dies ist von HomeKit nicht darstellbar; ich hab das auf 28 Grad; Target = HEAT; Current = HEAT übersetzt). Für alle Werte innerhalb der 8 bis 28 Range regelt das Thermostat selbstständig, je nach aktueller Temperatur ob es heizen soll. Da Fritz keine Information über den Zustand des Ventils führt müssen wir uns das irgendwie selber zusammenrechen. Dafür hab ich ganz einfach einen größer gleich Vergleich von CurrentTemperature und TargetTemperature eingeführt für die Feststellung des CurrentHeatingCoolingState, sofern eben TargetHeatingCoolingState nicht ausgeschalten ist.


Nachtmodus und Komfortmodus

Wie dir auffällt ist hier noch kein einziges Wort über Nachtmodus bzw. Komfortmodus gefallen, einfach deswegen weil dies nicht repräsentierter ist durch HomeKit bzw. HomeKit dafür nichts festlegt. Wie am Beispiel der Eve Thermos kann entweder der herstelle über seine eigenen App Funktionen wie Zeitpläne hinzufügen oder der Nutzer kann Zeitbasierte Temperaturänderungen über HomeKit Automationen einstellen (Oder eben die Fritz Seite). Ich finde es auch keine gute Idee diese Funktionen auf die von uns nichtbenutzten Status zu mappen (AUTO und COOLING), weil das eher zu Verständnisproblemen und anderen Konflikten führt. Sollte jemand Zeitpläne über die Fritz Komfort- und Nachttemperaturen einstellen werden diese ja auch durch die TargetTemperature zur jeweiligen Zeit repräsentiert.

Zusammenfassung

Was ich mit diesem PR versucht habe ist, das Verhalten, dass man als langjähriger HomeKit Nutzer erwartet zu etablieren. Am Ende des Tages ist es wahrscheinlich Präferenz(?). Ich halte halt nicht viel von so hacky Sachen, wenn man irgendwelche Funktionen versucht auf andere Funktionen zu mappen, die nie dafür bestimmt ware oder einfach keinen Sinn machen.

Supereg commented 4 years ago

Um weiter auszuführen. Ich finde die aktuelle Code base des Thermostat services katastrophal und äußerst unleserlich (meine Meinung). Vor allem die setValues innerhalb der set listener, die extra den Context setzten, damit nicht alles aufgerufen wird. Auch das Verhalten immer erst mit dem Callback zu antworten und dann einen später irgendwann die aktuellen Daten hinterher zu schicken, wenn sie ankommen erschafft mehr Probleme als es löst. Ich hab mich mal in die Fritz Smart Home Sachen ein wenig tiefer eingelesen und es scheint so, dass die FritzBox eh nur hin und wieder den Status des Thermostats updatet. Warum also bei jedem get request eine Anfrage senden. Ich könnte mir auch ein Konzept vorstellen wo der update Mechanismus komplett von den get listener entkoppelt wird. Aber so tiefgreifende Änderungen wollte ich nicht direkt mit dem PR vornehmen. Mir ist es erstmal wichtig die Diskussion darum anzuregen und beschriebene Funktionsweise umzusetzen. Am Ende des Tages ist es für mich nicht wichtig wie viel davon umgesetzt wird. Für mich ist es okay mit meinem eigenen Fork zu fahren und die nötigsten Änderung aus dem upstream zu ziehen. Ich versuche nur allen anderen Anwendern, die nach mir kommen (und vllt auch wirklich nur Anwender sind und keine Programmierer) eine aus meiner persönlichen Sicht bessere experience zu liefern.

Ich bin mal auf euer Feedback und eure Sichtweisen gespannt. Ich weiß auch nicht wie die angerissenen Änderungen mit den anderen Geräten funktionieren, die von dem Plugin unterstützt werden. Ich berufe mich hier einzig und allein auf die Fritz Thermostate, weil dies auch die einzigen Geräte sind, die ich in meinem Haushalt supporten muss. Von den anderen habe ich bisher keine Ahnung.

Supereg commented 4 years ago

Ok, also ich teste das ganze gerade mal, aber hab da gleich die ersten "Probleme". Sagen wir ich habe 20 Grad und das Thermostat steht auf 15 Grad, dann ist das Thermostat mit 20 Grad Grün und der Status "Heizen: 15 Grad". Alles gut. Stelle ich dann auf 21 Grad wird die neue Zieltemperatur zwar übernommen, aber es bleibt bei 20 Grad Grün. Starte ich die App dann neu ändert sich der Status auf "Auf 21 Grad heizen" und das Thermostat zeigt 20 Grad Orange. So sollte es eigentlich gleich nach dem setzen der neuen Temperatur sein.

Daran hab ich eigentlich gedacht, aber ich sehe gerade, dass ich das glaub ich komplett vergessen habe zu implementieren. Ja genau, dass könnte man gleich noch verbessern, sodass der Zustandsübergang unmittelbar ausgeführt wird. EDIT: Ist jetzt implementiert. Ich bitte darum nochmal zu testen, ob das nun alles nach deinen Erwartungen diesbezüglich funktioniert :)

Das Thermostat lässt sich nur noch zwischen Heizen und Aus steuern. Perfekt, jedoch stellt AUS das Thermostat auf ZU anstatt auf die Spartemperatur. Ich finde das vom Grundgedanken ja richtig, macht genau das was man auswählt. Ich weiß aber nicht ob das alle so sehen?

Das ist unter anderem das Verhalten, das ich mit diesem PR versuche zu etablieren. Wie das die Allgemeinheit so sieht, keine Ahnung. Ich persönlich sehe das jedoch als das einzig logische Verhalten. Zeitplänen für die Spartemperatur können ja trotzdem gesetzt werden oder mittels HomeKit Automationen o.ä. selbst gesetzt werden. Aber wenn ich mein Thermostat Ausschalten will dann soll es halt auch aus sein 🤷🏽‍♂️

Die Zeitsteuerung funktioniert weiterhin wie erwartet, aber das Schaltverhalten sollte dringend angepasst werden, damit man auch sofort sieht was los ist und nicht erst nach einem Neustart der App.

Mit Schaltverhalten meinst du das im ersten Absatz?

jngmrks commented 4 years ago

Ich sehe das genauso wenn Aus dann will ich auch das Aus ist. Ja genau, mit dem Schaltverhalten meinte ich den Status ob Heizen oder Kühlen, ich teste nochmal mit der neuen Version :).

Ein Problem bleibt. Ein solcher Sturm an Anfragen das gar nichts mehr geht. 🤨. Wenn alle Anfragen durch sind, dann läufts, vorher lässt sich nichts steuern, das trifft auch auf das Intervall zu. Wenn da alles abgefragt wird geht gar nichts mehr. Eingestellte Temperaturen werden wieder auf den vorherigen Wert gesetzt und eingeschaltete Thermostate wieder ausgeschaltet. Ein totales Durcheinander 😐. Richtig Schlimm wird's erst wenn man während dem Start die App öffnet. Dann werden alle Anfragen nochmal gestartet. So ein Durchlauf dauert ca. 1 Minute bei mir. (Davon sind 20 Sekunden nur die Batteriestand abfragen). Erst nach dieser gefühlten Minute kann ich etwas ändern. Vorher springt es immer wieder zurück. Das Problem hatte ich vorher mit der #80 von @andig nicht (mehr).

Hat das Intervall überhaupt noch eine Relevanz? Die App fragt sowieso jedes mal von neuem bei der Box an? Ich habs mal auf 1800 Sekunden gestellt und konnte keinen Nachtteil erkennen.

Die Wechsel von Heizen und Kühlen funktionieren jetzt meistens, aber leider nicht immer. Oft bleibt es dann Orange, egal ob ich eine höhere oder niedrigere Temperatur einstelle, irgendwo muss da noch was fehlen.

andig commented 4 years ago

Moin! Erstmal vielen Dank für die Zeit die Du Dir hier für die Verbesserung des Plugin nimmst! Ich selbst habe es seit neuer Heizung nicht mehr im Einsatz sondern bin auf solches Feedback angewiesen. Das aktuelle Vrehalten ist nach verschiedenen Useriterationen entstanden- deshalb bitte ich um Vertändnis wenn ich es nicht ad-hoc ohne es wirklich zu verstehen ändern möchte.

Wie dir auffällt ist hier noch kein einziges Wort über Nachtmodus bzw. Komfortmodus gefallen, einfach deswegen weil dies nicht repräsentierter ist durch HomeKit bzw. HomeKit dafür nichts festlegt. Wie am Beispiel der Eve Thermos kann entweder der herstelle über seine eigenen App Funktionen wie Zeitpläne hinzufügen oder der Nutzer kann Zeitbasierte Temperaturänderungen über HomeKit Automationen einstellen (Oder eben die Fritz Seite). Ich finde es auch keine gute Idee diese Funktionen auf die von uns nichtbenutzten Status zu mappen (AUTO und COOLING), weil das eher zu Verständnisproblemen und anderen Konflikten führt. Sollte jemand Zeitpläne über die Fritz Komfort- und Nachttemperaturen einstellen werden diese ja auch durch die TargetTemperature zur jeweiligen Zeit repräsentiert.

D.h. die Empfehlung wäre eigentlich die FritzPläne zu deaktivieren während ich versucht hatte sie irgendwie darzustellen?

Was ich mit diesem PR versucht habe ist, das Verhalten, dass man als langjähriger HomeKit Nutzer erwartet zu etablieren. Am Ende des Tages ist es wahrscheinlich Präferenz(?). Ich halte halt nicht viel von so hacky Sachen, wenn man irgendwelche Funktionen versucht auf andere Funktionen zu mappen, die nie dafür bestimmt ware oder einfach keinen Sinn machen.

Nachvollziehbar!

Um weiter auszuführen. Ich finde die aktuelle Code base des Thermostat services katastrophal und äußerst unleserlich (meine Meinung). Vor allem die setValues innerhalb der set listener, die extra den Context setzten, damit nicht alles aufgerufen wird.

Tatsächlich war mir update neu, ich kannte das einfach nicht. Daher meine Frage danach. Ich würde deshalb gerne die Chance nutzen (neuer PR?) hier und komplett von set auf update umzusteigen wo möglich. Würde mich über PR freuen.

Auch das Verhalten immer erst mit dem Callback zu antworten und dann einen später irgendwann die aktuellen Daten hinterher zu schicken, wenn sie ankommen erschafft mehr Probleme als es löst.

Jaein. Der Aufhänger war die epische Installation von @jngmrks. Dort werden mehrere Räume per Szene geschaltet was zu einem derartigen API-Sturm führt dass Homekit aufgrund Timeouts von 5 Sekunden die Aktionen als fehlerhaft anzeigt. Da müsste einmalig gemeinsames Verständnis her wie das ggf. anders gelöst werden könnte, dann habe ich auch nichts gegen abweichende Lösungsansätze.

... Ich könnte mir auch ein Konzept vorstellen wo der update Mechanismus komplett von den get listener entkoppelt wird. Aber so tiefgreifende Änderungen wollte ich nicht direkt mit dem PR vornehmen. Mir ist es erstmal wichtig die Diskussion darum anzuregen und beschriebene Funktionsweise umzusetzen.

Ich hatte eine ähnlcihe Idee, da ich das Plugin aber nicht ehr aktiv nutze (und damit auch nicht mehr teste) hab ich lieber die Finger davon gelassen. Können wir aber gerne anpacken.

Am Ende des Tages ist es für mich nicht wichtig wie viel davon umgesetzt wird. Für mich ist es okay mit meinem eigenen Fork zu fahren und die nötigsten Änderung aus dem upstream zu ziehen.

Ich versuche nur allen anderen Anwendern, die nach mir kommen (und vllt auch wirklich nur Anwender sind und keine Programmierer) eine aus meiner persönlichen Sicht bessere experience zu liefern.

Ich würde mich freuen hier möglichst viel mitzunehmen.

Ich bin mal auf euer Feedback und eure Sichtweisen gespannt. Ich weiß auch nicht wie die angerissenen Änderungen mit den anderen Geräten funktionieren, die von dem Plugin unterstützt werden. Ich berufe mich hier einzig und allein auf die Fritz Thermostate, weil dies auch die einzigen Geräte sind, die ich in meinem Haushalt supporten muss. Von den anderen habe ich bisher keine Ahnung.

Also aus meiner Sicht verstanden. Mein Vorschlag:

Klingt gut?

andig commented 4 years ago

ping @Supereg ich hoffe ich habe nix Falsches gesagt?

Supereg commented 4 years ago

sorry, ist schon wieder alles in den hintersten Gehirnzellen verschwunden. Zeit ist momentan etwas rar bei mir 😅

Supereg commented 4 years ago

Ich schau, dass ich am Wochenende mich vllt dazu komme mich damit erneut zu beschäftigen. Ich würde jetzt erstmal ein einem neuen PR versuchen auf updateValue umzusteigen und eventuell das direkte Anfrage des Status rauszunehmen und nur über die Intervallsteuerung zu gehen. Danach werd ich mich dann hiermit wieder auseinandersetzen, denn die Fehler die du beschreibst @jngmrks (mit zurücksetzten etc) liegen eher dem zugrunde und waren vorher nur nicht wirklich sichtbar 🤔

Ergänzung der README- Du hast soviel Grundlagen beschrieben, die sollten da unbedingt rein!

Und keine Ahnung was du von mir erwartest, dass ich da rein schreibe 😅 Nicht unbedingt meine liebste Aufgabe READMEs zu verfassen 😂

andig commented 4 years ago

Nicht unbedingt meine liebste Aufgabe READMEs zu verfassen 😂

Meine auch nicht :)

Und keine Ahnung was du von mir erwartest, dass ich da rein schreibe 😅

Die grundsätzliche Funktionalität des Thermostats sollte rein- nix Wildes. Bevor der nächste PR es mangels Doku wieder ändern will ;)

jngmrks commented 4 years ago

Du meinst Quasi alles auf Cache Basis? Und dann beim Interval die Werte ändern? Das wäre natürlich genial. Das würde die ewigen Wartezeiten komplett eliminieren, aber auch neue Probleme verursachen. Es sei denn dies würde rein nur für die Thermostate gemacht. Vor allem fürs Screenscraping der Akkustände... Das dauert am längsten.

Supereg commented 4 years ago

Genau, dass halt bei einer Abfrage durch HomeKit direkt und ausschließlich die cached value zurückgegeben wird (was auch ganz nebenbei das erwartete verhalten des HAP für bridges ist). Und der cache wird halt in einem sinnvollen (🙃) Abstand geupdated.

Welche Probleme genau? Ich kenn aktuell halt nur die Codebase für die Thermostat Accessoires, keine Ahnung ob das für die anderen Fritz Produkte auch funktioniert.

..., aber auch neue Probleme verursachen. Es sei denn dies würde rein nur für die Thermostate gemacht. Vor allem fürs Screenscraping der Akkustände... Das dauert am längsten.

Pls explain

jngmrks commented 4 years ago

Naja wenn das für die schaltbaren Steckdosen auch eingeführt wird und sagen wir mal ich hätte einen Intervall von 2 Minuten, dann würde es im schlimmsten Fall eben 2 Minuten dauern bis die Steckdose eingeschaltet wird. Im Umkehrschluss würde ich dann auch erst in 2 Minuten sehen das die Steckdose eingeschaltet wurde ;). Bei den Thermostaten wäre mir das ziemlich egal, die werden ja eh nur alle 15? Minuten aktualisiert.

Supereg commented 4 years ago

Ja gut, da könnte man dann schauen, dass man entweder dann für Steckdosen das Intervall runtersetzt oder zusätzlich je nachdem wie alt der gecached wert ist man zusätzlich ne Anfrage mitschickt. Hat man halt wieder das Problem, dass man wahrscheinlich direkt antwortet und danach erst irgendwann die Anfrage zurückkommt

jngmrks commented 4 years ago

Also nochmal zum Verständnis, du möchtest bei einer Änderung am Thermostat an HomeKit sofort ein Ok und die neue Temperatur zurücksenden. -> Ok. Aber willst du auch Zeitgleich der FritzBox die Änderung mitteilen oder erst mit dem Interval? Ich verstehe das dann so, das im Cache der neue Wert steht, aber eigentlich müsste die FritzBox ja die Änderung sofort mitgeteilt bekommen damit dann beim nächsten Cache-Update der Wert schon stimmt!?

Supereg commented 4 years ago

Änderungen werden logischerweise sofort weitergesendet. Es geht hier hauptsächlich um die get anfragen durch HomeKit, da die tendenziell häufiger auftreten.

jngmrks commented 4 years ago

Ja die brauchen bei mir gesamt an die 45 Sekunden (das längste davon ist das ScreenScraping welches übrigens immer doppelt ausgeführt wird) das könnte man dann auch auf eine Anfrage zusammenfassen.

Mir gefällt diese Cache-Idee sehr gut und ich denke echt das dies auch das normale Verhalten sein sollte. Dadurch keinerlei Timeouts mehr und ein extrem performantes HomeKit. Jetzt updated ja ein Gerät nach dem anderen...)

Supereg commented 4 years ago

Was genau braucht 45 Sekunden? Eine einzige Anfrage für ein einziges Gerät 😳 Und was ist ScreenScraping, bzw in dem Zusammenhang?

jngmrks commented 4 years ago

Nein, ein Durchlauf aller Thermostate und Steckdosen braucht ca. 45 Sekunden. 7 Steckdosen und 5 Thermostate. Das ScreenScraping wird für den Akkustand der Thermostate benötigt, da der Akkustand kein Bestandteil der Fritz API ist.

@andig -> getBatteryLevel und getStatusLowBattery verwenden den gleichen FB Api Call. Dieser kommt durch ScreenScraping zustande.

andig commented 4 years ago

@Supereg ich denke hier geht es ein bisschen durcheinander. Für den Batteriestand gibts kein API, daher wird der aus der HTML Oberfläche der FB "rausextrahiert"- das nennt man Screen Scraping. Wenn da was doppelt aufgerufen wird ist es ein separates Issue und hat in diesem Issue nichts zu suchen.

andig commented 4 years ago

getBatteryLevel und getStatusLowBattery verwenden den gleichen FB Api Call. Dieser kommt durch ScreenScraping zustande.

Ja, das könnte man vmtl. cachen und z.B. den alten Wert nehmen falls aktueller als xyz. Dafür bitte separates Issue, hier gehts um Thermostate!

jngmrks commented 4 years ago

Eben, deswegen schreib Ichs ja hier die Abfrage ist ja von den Thermostaten, da ein Cache dafür eingeführt werden soll. Wenn beide aus einem Aufruf kommen, dann kann man sich die zusätzliche Anfrage ja sparen. Bzw. beide Anfragen aus einem Cache-Eintrag holen.

andig commented 4 years ago

Was fehlt denn hier konkret noch oder wäre der PR so rund dass er rein kann?

Supereg commented 4 years ago

So wie ich es verstanden habe, funktioniert das hier noch nicht so reibungslose. Vom aktuellen Plan her hätte ich erst #91 gemerged. Und dann den PR hier noch angepasst auf das neue Verhalten.

andig commented 4 years ago

Rebase benötigt (git rebase restirct-valid-values), alternativ- damit es bei jgnmrks keine Probleme gibt- git merge.

Supereg commented 4 years ago

Jo werd das später mal nen rebase auf master machen.

Supereg commented 4 years ago

Okay, ist jetzt auf den master rebased. Bei meinen Test funktioniert alles so weit. Vllt könntest du @jngmrks nochmal schauen, wie das mit deinem Ansturm an Geräten läuft.

Dann liegt es nur noch an euch, in wie fern ihr hier genanntes Verhalten etablieren wollt oder nicht.

Die README sollte vllt noch angepasst werden, die veralteten Hinweise raus und vllt ein paar neue rein.

jngmrks commented 4 years ago

Ich befürworte die Änderung auf Heizung ein/aus und dem Status Heizen und Kühlen wie in der restirct-valid-values. Selbiges wie Tag/Nacht kann man auch mit der Automation erzielen, sogar Standortbasierend.

Jedoch bleibt noch ein "BUG". Siehe Video: https://jueng.de/1572216252.mp4 Sporadisch ändert sich der Status von Heizen und Kühlen nicht in Abhängigkeit der Temperatur. Der Status aktualisiert sich erst wenn man die App abschießt.

jngmrks commented 4 years ago

Bis auf den kleinen Bug der sporadisch auftritt und ich nicht genau feststellen kann was diesen auslöst, wäre meiner Meinung nach alles zur Veröffentlichung bereit.

Supereg commented 4 years ago

Sporadisch ändert sich der Status von Heizen und Kühlen nicht in Abhängigkeit der Temperatur.

Okay werd mich da nochmal dran setzten. Also du meinst, wenn du die Temperatur einstellst, wird der CurrentHeatingCoolingState nicht (immer?) korrekt geupdated?

jngmrks commented 4 years ago

Sporadisch ändert sich der Status von Heizen und Kühlen nicht in Abhängigkeit der Temperatur.

Okay werd mich da nochmal dran setzten. Also du meinst, wenn du die Temperatur einstellst, wird der CurrentHeatingCoolingState nicht (immer?) korrekt geupdated?

Ja, siehe hier: https://jueng.de/1572216252.mp4

Supereg commented 4 years ago

Also der case wird im Code soweit eigentlich behandelt. Ich hätte die Vermutung, dass genau in dem Moment der update vom Interval dazwischenfunkt und die Änderung direkt wieder überschreibt. Wobei das nicht erklärt, warum die Temperatur dann nicht auch überschrieben wird. Kannst du versuchen das zu reproduzieren und mir circa davon ein debug log output schicken. Da dass ja nicht immer auftritt muss es ja irgendwie sowas sein. Du könntest auch mal testen was passiert, wenn du die Home app offen lässt und den Interval abwartest, also so circa ne Minute warten.

Supereg commented 4 years ago

Gibts nen Grund warum deine anderen Thermostate rot hinterlegt sind 😅

jngmrks commented 4 years ago

Ja Batteriestand 1%, aber da halten die immer noch ca. 1 Jahr

appliko commented 4 years ago

Hi, did you recheck if „off“ function mode is working after latest changes ? It is not working to me