Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

extend optimistic state timeout to 10 seconds #10735

Closed

Conversation

marcelveldt
Copy link
Member

@marcelveldt marcelveldt commented Nov 30, 2021

Proposed change

Discussed many times already in forums, discord etc.
Some integrations and/or devices are just a bit slow to update, for example some Z-Wave devices.

You send a call service to turn on a light but the new state only arrives seconds later.
This causes a weird "flip flop" in the UI. You turn on the toggle, 2 seconds later it turns off again and a few seconds later it will turn on again. For most people this will mean they will probably go wild toggling the entity as they do not understand why it toggled back in the UI.

Some attempts have been made to try to fix this in the integrations but that just feels hacky.
Not to say that this approach is better but it is way more simpler and works for all integrations at once.

This extends the optimistic timeout from the current 2 seconds to 10 seconds.
In other words: This will give the integration up to 10 seconds to report back the actual/updated state.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

prevents "flip flopping" for slow integrations/devices
@balloob
Copy link
Member

balloob commented Nov 30, 2021

10 seconds is too long to "lie". We should explore some indicator that it's pending.

@marcelveldt
Copy link
Member Author

10 seconds is too long to "lie". We should explore some indicator that it's pending.

The pending indicator is even better approach.

  1. Turn on light
  2. Set "pending" variable
  3. Have some kind of visual feedback about this pending state (e.g. disable the control)
  4. Reset pending state after the timeout OR when state update arrives for this entity

Easiest approach for first version would be to tie disabled state of the control to the pending state.

AND/OR have an entity attribute with the update delay, defaulting to 2 seconds and can be overridden at entity-level.

@balloob
Copy link
Member

balloob commented Nov 30, 2021

The downside with disabling is that is what we do when the entity is unavailable.

@marcelveldt
Copy link
Member Author

Hmmm, yeah you are right. So some kind of "loading indicator" would be better perhaps.
I'll try if I can create something like that. Is there already some kind of in-place loading indicator used in the frontend to re-use for this ?

@balloob
Copy link
Member

balloob commented Nov 30, 2021

We don't have something like this for toggles. For buttons we have a "spinner on button" although I am not sure if we still use it?

We should let @matthiasdebaat take a look too.

@matthiasdebaat
Copy link
Collaborator

matthiasdebaat commented Dec 1, 2021

The UX pattern of a Switch state that it immediately activate or deactivate something. 10 seconds is in that case way to long. The virtual switch is like a real world one. If you tap it and it doesn’t show immediate result you get confused. And maybe tap it again. So technically the switch should only toggle when there is really something happening. That is in our situation not nice, so I think the current two seconds is a good solution.

This is my suggestion: When the users click the switch, it toggles like we do now. When the integration doesn’t report back after 2 seconds, we replace the switch with a spinner. I want to keep the UI as clean as possible for the 80% most common situation.

@marcelveldt
Copy link
Member Author

marcelveldt commented Dec 2, 2021

That sounds like a very good solution @matthiasdebaat. I'm not sure about numbers but I think we're talking about a small amount of integrations/devices that do not report back within 2 seconds.
In fact I once had a few of those devices and when both I and my wife got frustrated I ditched them to the trash.

Anyways this issue gets reported many times on the forums and issue trackers so would be nice to fix.
I'll have a look at the spinner approach. Shouldn't be hard code-wise but maybe you can point me at an already existing spinner to (re)use for this purpose ?

Schermafbeelding 2021-12-02 om 16 03 32

@balloob
Copy link
Member

balloob commented Dec 2, 2021

That video is an example from the docs about what not to do 😝

@marcelveldt
Copy link
Member Author

Yeah, I was just referencing that small spinner icon/animation :-)

@bramkragten
Copy link
Member

src/components/ha-circular-progress.ts

@kpine
Copy link
Contributor

kpine commented Dec 3, 2021

This approach works for people who are watching the UI. It doesn't solve the problem for those using voice commands (e.g. Alexa) though.

@marcelveldt
Copy link
Member Author

marcelveldt commented Dec 3, 2021

This approach works for people who are watching the UI. It doesn't solve the problem for those using voice commands (e.g. Alexa) though.

What do you mean? Does the voice assistant give feedback that setting the new value failed ?

EDIT: Confirmed by reading the forum post. The voice assistant responds that the command failed while the light actually turned on/off. So the voice assistant (or any other listener for the command) should also retrieve a temporary optimistic state.

@marcelveldt marcelveldt marked this pull request as draft December 3, 2021 19:44
@marcelveldt
Copy link
Member Author

Converted to draft as this needs more discussion

@marcelveldt
Copy link
Member Author

marcelveldt commented Dec 3, 2021

Given the response of @kpine that this also has impact to the voice assistants this needs more thought and can't be handled in the UI entirely.

Idea:

  1. Entity attribute with an optimistic_state timeout (or a bool and have a static timeout).
  2. After a command has been executed set some flag that the state is optimistic
  3. Set a timeout to cancel the optimistic state if a new state did not come in within the timespan
  4. UI can act on the optimistic flag by showing the spinner

Or something like that.

@marcelveldt
Copy link
Member Author

BTW: Do we have any idea which integrations actually have this issue ?
I'm aware of Z-wave but are there any others that are slow with their state reporting ?
I quickly browsed some integrations this week and I spotted some that actually report the set state without actually relying on the actual state from the device.

@mwolter805
Copy link

Hello, I started an issue related to this PR a few months ago home-assistant/core#59379 and wondering if the change in 2022.2 home-assistant/core#64621 is related to or resolves this? Thanks!

@blhoward2
Copy link

It would be nice to have this fixed. What needs to be worked out RE the voice assistants?

@marcelveldt
Copy link
Member Author

marcelveldt commented Feb 24, 2022

It would be nice to have this fixed. What needs to be worked out RE the voice assistants?

Imo the only way to fix this is at core level (for all integrations):

  1. Send a command to a device/light
  2. Update the state and mark it as optimistic
  3. Expect a state update to happen from the device within X seconds
  4. If a state is being marked as optimistic, show this in some visual way to the user and prevent a new command until the confirmation is there.
  5. State update received from device --> remove optimistic mark and update state
  6. Timeout expired (no state received from device) --> remove optimistic mark and restore state

This is an architecture decision. Speaking of making things easier and reorganizing stuff in HA I think this fits there exactly. This is a good example where the user experience differs completely from the technical one. A user doesn't care at all if a state update was retrieved for a device/light. All they see is "why the heck did my command not work ?"

Created an issue for this in the architecture repo:
home-assistant/architecture#740

@bouwew
Copy link

bouwew commented Mar 9, 2022

10 seconds is too long to "lie". We should explore some indicator that it's pending.

The pending indicator is even better approach.

The Simple Thermostat card shows this in a nice way: when changing the setpoint the color of the setpoint numbers turn red. Once the same value comes back from the climate entity then the color of the numbers goes back to normal.

@bouwew
Copy link

bouwew commented Mar 9, 2022

BTW: Do we have any idea which integrations actually have this issue ? I'm aware of Z-wave but are there any others that are slow with their state reporting ? I quickly browsed some integrations this week and I spotted some that actually report the set state without actually relying on the actual state from the device.

The Plugwise integration has this issue after fully implementing use of the DUC, see home-assistant/core#67687

@github-actions
Copy link

github-actions bot commented Jun 7, 2022

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@github-actions github-actions bot added the stale label Jun 7, 2022
@bramkragten bramkragten removed the stale label Jun 7, 2022
@bramkragten
Copy link
Member

Gonna close this, there is currently a architecture discussion going to fix this on a larger scale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants