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

MQTT Cover Fix Assumed State #14672

Merged
merged 1 commit into from
May 29, 2018

Conversation

OttoWinter
Copy link
Member

Description:

Usually, if an MQTT platform has an optimistic mode, it means that assumed_state will also be True. This is usually used in cases where the actual state of a device (such as a cover) is unknown and causes the front-end to show the up AND down actions.

Right now, if optimistic is False, only one action is shown as seen below. However, if optimistic is True, ideally both actions should be enabled.

screen shot 2018-05-28 at 22 28 35

Related issue (if applicable): fixes #

Example entry for configuration.yaml (if applicable):

cover:
  - platform: mqtt
    optimistic: True
    # ...

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@OttoWinter
Copy link
Member Author

OttoWinter commented May 28, 2018

(In my case, this would preferably be in a bug-fix release such as 0.70.1, as this is a pretty important part of the new cover component for esphomeyaml in the next version and since this is actually a bug fix. Of course I understand if this should rather fall into the usual full release cycle)

@fabaff fabaff added this to the 0.70.1 milestone May 28, 2018
@dgomes
Copy link
Contributor

dgomes commented May 28, 2018

LGTM

Would the considerations I made in #14401 and #14151 also apply to the cover ?

@OttoWinter
Copy link
Member Author

@dgomes Probably, yes. But I think in the case of the cover it's not that important, since the frontend doesn't seem to show the state if in optimistic mode (the up/down buttons are there to show the state)

Also there are probably few use-cases where one needs the underlying state from templates/automations, so I don't think I'll do that in this PR.

@dgomes
Copy link
Contributor

dgomes commented May 28, 2018

well the obvious use case would be to avoid open/close the cover when the cover is already open/closed

@OttoWinter
Copy link
Member Author

The cover component (as well as the other MQTT integrations I believe) explicitly do not prevent sending a command like open the cover twice.

Besides, the whole idea of the optimistic parameter is that you do not know the actual state of the device (here cover). The problem is that if we try to prevent users from doing something like opening a cover twice, then that would also mean to only show one of the UP/DOWN buttons in the front-end.

Even with the recorder we cannot always know the actual state of the cover, only do an approximation of the state.

@dgomes
Copy link
Contributor

dgomes commented May 28, 2018

My understanding of optimistic is different:

  • optimistic means that you think you know the state even when you don't have any sensor to support.
  • your optimistic interpretation leads to a nihilist situation in which any state goes, so the cover is actually in all states at once.

@OttoWinter
Copy link
Member Author

OttoWinter commented May 28, 2018

Well the Home Assistant property we're declaring is called assumed_state after all.

Also, consider this: I have a cover, which is optimistic as it does not have any sensor reporting the actual state of the cover. Then:

  1. I open the cover through the frontend. After this Home Assistant will only show the close button as Home Assistant now thinks the cover is open.
  2. Home Assistant shuts down due to an update/reboot or whatever.
  3. During this down period I manually close the cover through a push button I installed. The cover is now closed.
  4. Home Assistant starts up again and thinks through the smart recorder recovery that the cover is open.
  5. After a while I try to open the cover again through Home Assistant. But: As Home Assistant thinks the cover is open it only displays close button. So I can't open the cover through Home Assistant -> bad UX

The same would happen in a more frequent scenario:

  1. I press the open cover button, because of optimistic mode Home Assistant now thinks the cover is immediately open.
  2. While the cover is opening, I decide that that wasn't what I wanted and press the stop button again. Home Assistant still thinks the cover is open.
  3. I attempt to undo my action and close the cover again, but again Home Assistant only shows the open button.

Of course the latter scenario could be handled manually by setting assumed state to true once a stop happens, but that would just make things more complicated and once again a semi-bad UX imo.

To clarify, sure we can recover the state from the recorder, no problem. But we should definitely not try to prevent the user from opening a cover twice (by setting assumed_state to something else than optimistic)

@dgomes
Copy link
Contributor

dgomes commented May 28, 2018

  • upon stop the state should be intermediate (all buttons available UP & DOWN)

It's true that a human can always mess around with the state, but thats why it's optimistic (we expects that no one does and everything is in sync)

How to recover from a closed cover that optimistically we think is open ?

  • make the stop button always available (see above, when you press stop)

@balloob
Copy link
Member

balloob commented May 29, 2018

So I feel like we failed by naming the option optimistic for MQTT and not assumed. Because optimistic != assumed.

Optimistic: when sending the command, assume it succeeds and update the local cache of the state. Just being optimistic doesn't mean perse that we don't get a state reported, it might just be very slow.

Assumed: we never get a state reported. Be optimistic when sending commands.

@balloob balloob merged commit fcb60d4 into home-assistant:dev May 29, 2018
balloob pushed a commit that referenced this pull request May 31, 2018
@balloob balloob mentioned this pull request May 31, 2018
girlpunk pushed a commit to girlpunk/home-assistant that referenced this pull request Sep 4, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Sep 5, 2018
@OttoWinter OttoWinter deleted the cover-assumed-state branch September 29, 2018 10:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants