hahomematic
hahomematic copied to clipboard
danielperna84
Allow changing level or tilt while blind is moving
Addresses #1506.
Test setup:
- HMIP-BBL (firmware version 1.10.6)
- RaspberryMatic 3.75.7.20240420
- custom_homematic
53f70f7
Ich gehe mal davon aus, das du den Code nicht gegen ein Gerät getestet hast, das sollte aber dein Ziel sein.
Die Verwendung des Locks halte ich für unnötig, da der betroffene Codeabschnitt nur eine Ausführungszeit im Millisekunden bereich hat. Wenn der collector alle Daten gesammelt hat wird ein Befehl an die CCU gesendet, und nicht auf ein Ergebnis gewartet. Selbst wenn der Lock eine Befehlsausführung abwarten würde, sollte der Stop Befehl nicht auf eine Lockfreigabe warten müssen, sondern immer sofort ausgeführt werden.
Daher wird in #1512 statt eines Locks die command_queue (FIFO) verwendet und mit wait_for_callback innerhalb des definierten Timeouts auf ein Ergebnis gewartet. Die Ergebnisse werden in der empfangenen Reihenfolge abgearbeitet, und die Verarbeitung kann jederzeit unterbrochen werden. Würde mich freuen, wenn Du das auch testen könntest.
Egal wie es weitergeht solltest Du als nächste deine lokale Entwicklungsumgebung soweit ertüchtigen, dass Du eine Codeänderung mit HA gegen ein echtes Gerät schicken kannst. Wenn Du dabei Hilfe brauchst, dann melde dich. Ich werde allerdings über das verlängerte Wochenende wohl nicht erreichbar sein.
Ich gehe mal davon aus, das du den Code nicht gegen ein Gerät getestet hast, das sollte aber dein Ziel sein.
Das hab ich natürlich getan (siehe PR-Beschreibung) 😉
Daher wird in https://github.com/danielperna84/hahomematic/pull/1512 statt eines Locks die
command_queue(FIFO) verwendet und mitwait_for_callbackinnerhalb des definierten Timeouts auf ein Ergebnis gewartet. Die Ergebnisse werden in der empfangenen Reihenfolge abgearbeitet, und die Verarbeitung kann jederzeit unterbrochen werden. Würde mich freuen, wenn Du das auch testen könntest.
Das habe ich als allererstes getan. Wenn command_queue und wait_for_callback aktiv sind, tritt z.B. beim Aktivieren von Szenen diese Race Condition zuverlässig auf. Da werden halt zwei Service Calls direkt hintereinander. Das ergibt aus meiner Sicht schon Sinn, dass in der Zwischenzeit nicht asynchron ein Netzwerkrequest an die CCU rausgehen kann. Hier sind wir dann im Sub-Millisekundenbereich.
Das hab ich natürlich getan (siehe PR-Beschreibung) 😉
Für mich geht das da nicht draus hervor.
Die Frage ist allerdings über welche Racecondition wir hier sprechen, und da könnte das Verständnisproblem sein.
Ich will dafür sorgen das die Befehle gegen das Gerät nacheinander, und erst nach Abschluss der Vorgängeraktion ausgeführt werden, aber auch jederzeit gestoppt werden können.
Du willst dafür sorgen, das nicht parallel an neuen Befehlen im HA Code gearbeitet wird, und so wiedersprüchliche Befehle für die CCU entstehen. Richtig?
Weil mir aufgefallen ist, dass diese Race Condition nicht offensichtlich ist, hier nochmal eine Erklärung:
Das Problem ist nicht, dass zwei Requests an die CCU sehr kurz nacheinander abgesendet werden und der zweite vom ersten überholt würde. Genau dieses Problem würde die command_queue ja lösen.
Das Problem ist, dass _set_level via _target_level und _target_tilt_level auf den command cache zugreift. Wenn also _set_level ein zweites Mal betreten wird, ohne dass der command cache schon mit den Daten des vorherigen Calls aktualisiert wurde, sind _target_level und _target_tilt_level potenziell falsch. In dem Fall sind dann die Ziellevel im zweiten Kommando tatsächlich schon falsch.
Das heißt, das lesen und aktualisieren von _target_level und _target_tilt_level muss "am Stück" passieren, das ist das, was das Lock schützen soll. Da _target_level und _target_tilt_level aus dem Command Cache kommen, und der erst beim Senden des tatsächlichen Kommandos aktualisiert wird, habe ich das Senden des Kommandos auch mit in die "gelockte" Region reingenommen. Hast Du eine Idee, wie man das noch anders oder kleinteiliger hinbekommen würde, @SukramJ?
Danke für deine Erklärung.
Genau dieses Problem würde die command_queue ja lösen.
Die command_queue ist in diesem PR aber deaktiviert.
Wann können wir mit einem Merge und neuen Release rechnen? Würde das gerne testen.
Wann können wir mit einem Merge und neuen Release rechnen?
Wenn es soweit ist.
Ist sonst noch irgendwas offen? Bei dir funktioniert mit den Geräten alles wie erwartet?
Codecov Report
Attention: Patch coverage is 92.68293% with 3 lines in your changes are missing coverage. Please review.
Project coverage is 88.47%. Comparing base (
ec5d62b) to head (457f5a4). Report is 77 commits behind head on devel.
| Files | Patch % | Lines |
|---|---|---|
| hahomematic/platforms/custom/cover.py | 92.68% | 3 Missing :warning: |
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@ Coverage Diff @@
## devel #1549 +/- ##
==========================================
- Coverage 90.42% 88.47% -1.95%
==========================================
Files 49 52 +3
Lines 6037 6430 +393
==========================================
+ Hits 5459 5689 +230
- Misses 578 741 +163
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@SukramJ Jetzt müssen wir auf 1.62 warten richtig ?
@Jack77777777: Freue dich nicht zu früh bzgl. meines CCA-Blueprints. Ich habe zwar in der aktuelle Version ein konfigurierbares Tilt-Delay ergänzt, aber ich kann die Positionserkennung nicht anhand des Attributes 'current_tilt_position' durchführen. Daher weiß ich auch nicht, ob dieser PR uns dabei hilft.
Siehe https://github.com/danielperna84/hahomematic/discussions/1506#discussioncomment-9449053