Allow changing level or tilt while blind is moving #1549
Conversation
| """Stop the device if in motion.""" | ||
| await self._e_stop.send_value(value=True, collector=collector) | ||
| """Stop the device if in motion. Use only when command_processing_lock is held.""" | ||
| await self.stop(collector=collector) | ||
|
|
||
| def is_state_change(self, **kwargs: Any) -> bool: |
There was a problem hiding this comment.
This could be made aware of target positions
There was a problem hiding this comment.
Ich habe das mal versucht - so richtig glücklich war ich mit dem Ergebnis nicht. Das Problem ist, dass sowohl die einzelnen Methoden (wie open, close, open_tilt, vent, ...) als auch is_state_change unabhängig voneinander bestimmen, wie Target Level (und Target Tilt Level) abhängig von der auszuführenden Aktion zu setzen sind. Wenn hier noch eine Überprüfung von target_level und target_tilt_level dazukommt, wird es schnell sehr unübersichtlich.
Aus meiner Sicht wäre es sinnvoll, das Ableiten der Ziellevel und die Überprüfung, ob für gegebene Ziellevel ein neues Kommando gesendet werden muss, klar zu separieren. Dann kann man das Ableiten der Ziellevel auch an einer Stelle tun. Das wäre aber auch in nem eigenen PR ganz gut aufgehoben - was meinst du, @SukramJ?
|
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 Daher wird in #1512 statt eines Locks die 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. |
Das hab ich natürlich getan (siehe PR-Beschreibung) 😉
Das habe ich als allererstes getan. Wenn |
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 Das Problem ist, dass Das heißt, das lesen und aktualisieren von |
|
Danke für deine Erklärung.
Die |
Previously, the test case would succeed even when a command queue was being used. Since that leads to bugs on a real CCU (where the time for sending a command is not negligible), this commit introduces a similar communication delay to the test. At the same time, this allows us to reduce the number of iterations that this test needs to run in order to be reasonably sure that a race condition is absent.
|
Wann können wir mit einem Merge und neuen Release rechnen? Würde das gerne testen. |
Wenn es soweit ist. |
|
Ist sonst noch irgendwas offen? |
Codecov ReportAttention: Patch coverage is
❗ 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 ☔ View full report in Codecov by Sentry. |
|
Dann gehe ich auch davon aus das PR #1512 nicht benötigt wird. Oder? |
|
@sleiner Dann kann der PR gemerged werden? |
|
@SukramJ Jetzt müssen wir auf 1.62 warten richtig ? |
@Jack77777777: |
|
Geschlossene PR sind kein Diskussionsforum! |
|
Siehe #1506 (reply in thread) |
Addresses #1506.
Test setup:
53f70f7