danielperna84 / hahomematic

Python 3 Interface for Home Assistant to interact with HomeMatic devices
MIT License
130 stars 22 forks source link

HmIP-BBL: Setting cover position and tilt position at the same time is buggy #1503

Closed sleiner closed 5 months ago

sleiner commented 5 months ago

I agree to the following

The problem

When changing both cover position ("Behanghöhe") and tilt ("Lamellenposition") at roughly the same time through the Home Assistant cover.set_cover_position and `cover.set_cover_tilt_position, the blind stutters for a bit and either the new position or the new tilt are applied, but not both.

What version of HomematicIP (local) has the issue?

1.59.0

What was the last working version of HomematicIP (local)?

No response

What type of installation are you running?

Home Assistant OS

What type of installation are you running for your homematic backend?

RaspberryMatic Standalone

Which version of your homematic backend are you running?

3.75.6.20240316

What hardware are you running for your system?

CCU3

Which config details do you use

Which interfaces do you use?

Diagnostics information (no logs here)

No response

Log file extract. Anything in the logs that might be useful for us? The log (Setting/System/Logs -> load full log) is the best source to support trouble shooting!

No response

Additional information

How to reproduce

  1. Set the blind blind position to cover position ("Behanghöhe") 10% and tilt ("Lamellenposition") 10%, via one of these methods:
    • CCU Web UI
    • Set cover position in the Home Assistant dashboard, wait until the blind has arrived, then set tilt position
    • Via the homematicip_local.set_device_value, set COMBINED_PARAMETER to L=10,L2=10
  2. Create a Home Assistant scene which sets both cover position and tilt to 20%.
  3. Activate the scene.

Expected: Blind applies a cover position of 20% and a tilt of 20% *Actual behaviour: Blind stutters a bit and applies a cover position of 10% and a tilt of 20%.

Potential cause for the bug

When the scene is activated, Home Assistant calls cover.set_position and cover.set_tilt_position in rapid succession. After checking the cover implementation in hahomematic, I suppose that this results in two API calls to the CCU and thus two calls to CeIpBlind._set_level(). Even when the level or tilt level parameters are None, they are defaulted to the current position so that combined parameter is likely set twice with these values:

  1. L=20,L2=10 (resulting from cover.set_position)
  2. L=10,L2=20 (resulting from cover.set_tilt_position)

Ideally, setting them like this should work:

  1. L=20
  2. L2=20

Unfortunately, it does not - as I have tried using some CCU programs (without any Home Assistant involvement at all). Likewise, this does also not work:

  1. L=20,L1=101
  2. L=101,L2=20

The only variant I found that actually worked was:

  1. L=20 (setting the position when the blind is not moving. As the blind is in a steady, setting only either L or L2 is fine.
  2. L=20,L2=20 (setting the tilt position while the blind is already moving - note that we explicitly need to mention the target L even though it is not yet reached)
SukramJ commented 5 months ago

Ich kannst den Service set_cover_combined_position verwenden, da von Haus aus keine kombinierte Parameterübertragung unterstützt.

Eine weitere Verbesserung wurde hier vorgeschlagen, aber da vom Benutzer keine Rückmeldung mehr kam hab ich das Theme nicht weiter verfolgt. Vielleicht ist was Relevantes dabei.

Da ich selber keine cover besitze bin ich auf entsprechende Zuarbeit angewiesen.

Ideally, setting them like this should work:

Should ist mir egal. Bitte teste das mit dem combined_parameter in der CCU und melde eine funktionierende Variante zurück. cover ist mir zu sensibel, und wird von zu vielen Benutzern verwendet, als das ich an der Stelle rumexperimentieren werde.

SukramJ commented 5 months ago

Vielleicht auch hilfreich: #560

sleiner commented 5 months ago

Hi @SukramJ,

vielen Dank für die superschnelle Antwort!

Ich kannst den Service set_cover_combined_position verwenden, da von Haus aus keine kombinierte Parameterübertragung unterstützt.

Danke für den Hinweis! :-) Das ist mir bewusst; für Automatisierungen oder Skripte würde ich das sowieso bevorzugen, schon allein um durch nur einen Funkbefehl den Duty Cycle niedrig zu halten. Ich fände es trotzdem erstrebenswert, die "Home Assistant native" Variante zum Laufen zu bringen, da in manchen Bereichen nur die nativen Services verwendet werden können und nicht die Homematic-spezifischen, zum Beispiel:

Ideally, setting them like this should work:

Should ist mir egal. Bitte teste das mit dem combined_parameter in der CCU und melde eine funktionierende Variante zurück.

Das habe ich getan, es hat nicht funktioniert. Genauso die Variante aus #1269. Das hatte ich in meinem ursprünglichen Report wohl nicht gut formuliert. Um es nochmal anders und hoffentlich klarer zu formulieren:

Wenn gerade cover.set_position mit einer Position von 20 aufgerufen wurde und die Jalousie noch fährt, könnte cover.set_tilt_position für eine Lamellenposition von 20 den combined parameter folgendermaßen setzen:

Wenn du möchtest, kann ich einen Pull Request vorbereiten. Oder hast du vielleicht noch eine andere Idee?

SukramJ commented 5 months ago

Ich fände es trotzdem erstrebenswert, die "Home Assistant native" Variante zum Laufen zu bringen

Viel Erfolg. Sauber formuliert könntest Du natürlich auch hier sinnvolle Änderungen in HA erwirken.

Wenn du möchtest, kann ich einen Pull Request vorbereiten.

Bitte gerne, hier muss ich aber auf entsprechend angepasste bzw. erweiterte Unittests bestehen, da ich das ganze ansonsten später, aufgrund fehlender eigener Geräte, nicht weiter pflegen kann.

Alternativ wird ein entsprechender und möglichst vollständiger Testplan benötigt, und um die Implementierung würde ich mich kümmern Meine Erwartung wäre hier, das alle relevanten Parameterkombinationen gegen die CCU getestet werden (kein should). Parameter -> erwartetes Ergebnis. Das Testen muss über einen combined_parameter, oder wie der Parameter auch in der CCU WebUI heißt, erfolgen.

Sollwert aus dem letzten Service-Call angeben und nicht die aktuelle Position

Das hört sich ganz schlecht an, und sowas würde ich glaube ich auch nicht aktzeptieren. Was ist denn, wenn von einer anderen App aus geändert wird? Die Zustandsänderung sollte immer vom aktuellen Zustand ableitbar sein. Die Lösung sollte so ein fach wie möglich sein, denn ich muss das später weiter pflegen!!!