Undocumented change in mqtt cover in 0.82.0 & open/closed position does not work

bruxy70 commented 5 years ago

Home Assistant release with the issue: 0.82.0

Last working Home Assistant release (if known): 0.81.6

Operating environment (Hass.io/Docker/Windows/etc.): Hass.io

Component/platform: https://www.home-assistant.io/components/cover.mqtt/

Description of problem: My mqtt cover returns current position from 0 (cover open - all the way up) to 100 (cover closed - all the way down) in mqtt topic e.g. "klara/cover1/state". For this, I have configured state_topic: "klara/cover1/state" It was probably not correct, but it worked.

It seems that in 0.82.0 this was changed to a more correct set-up, where _statetopic should only report "open" and "closed", and there is new topic _positiontopic for position updates. But there are few issues with that:

After the update, the configuration needs to be changed by changing the _statetopic to _positiontopic. But this was not mentioned in the release documentation! Without the change, hassio returns the following error, the covers will not be created, lovelace will render an error with yellow background.

Invalid config for [cover.mqtt]: Set position topic must be set together with get position topic
The documentation on https://www.home-assistant.io/components/cover.mqtt/ has been updated by adding the topic, but it is contradicting itself. The _statetopics says

Use only if not using get_position_topic. State topic can only read open/close state.

Whilst the _positiontopic says

…Always in favor if used together with state_topic.

Also, the error and manual refers to _get_positiontopic, but the position is called _positiontopic.

The _stateopen and _stateclosed configuration also need to be changed to _positionopen and _positionclosed. This is also not mentioned in the release documentation!
Finally, the controls for the covers stopped working at the open/closed position. When the cover is in the middle it works fine - the up arrow will roll it up and down arrow will close it. BUT, when it reaches the closed position, it will disable the up arrow, not the down arrow. So it cannot be open anymore (except by the position slider, that works). And vice versa on the open position. Changing _positionopen and _positionclosed does not have any effect to this.
The _positiontopic did not work prior to 0.82.0, so there was no easy way to set it up before the upgrade. And the upgrade did not change it, so there is no easy way to do that - upgrade, don't panic, change config, restart. Or change it up front, but then it will only work after the upgrade. The check configuration add-on did not see the error either.

Problem-relevant configuration.yaml entries and (fill out even if it seems unimportant):

cover:
  - platform: mqtt
    name: "Klára dveře"
    command_topic: "klara/cover1/set"
    position_topic: "klara/cover1/state"
    set_position_topic:  "klara/cover1/position"
    payload_open: "open"
    payload_close: "close"
    payload_stop: "stop"
    position_open: 0
    position_closed: 100
    retain: true
    qos: 1

Traceback (if applicable):

Additional information:

dgomes commented 5 years ago

Since this was introduced by https://github.com/home-assistant/home-assistant/pull/16148

Maybe @pszafer can shed some light to this issue.

pszafer commented 5 years ago

Don't know if I can write anything to release documentation. I made this change and documentation for it. True about get_position_topic in state_topic section. I'd add PR for this.

State topic doesn't have anything anymore with getting/setting positon of cover using integer. It can be used only for open/close purposes. Nothing integer I'd say.

Finally, the controls for the covers stopped working at the open/closed position. When the cover is in the middle it works fine - the up arrow will roll it up and down arrow will close it. BUT, when it reaches the closed position, it will disable the up arrow, not the down arrow. So it cannot be open anymore (except by the position slider, that works). And vice versa on the open position. Changing position_open >and position_closed does not have any effect to this.

What is the state of not working covers while closed/open in /dev-state site?

bruxy70 commented 5 years ago

When the cover is closed (all the way down), the state is:

state: open State attributes: { "current_position": 100, "friendly_name": "Klára dveře", "supported_features": 15 } Whilst the configuration says
position_open: 0
position_closed: 100
On the web control, the up arrow is grayed, the stop and down arrow black.

When the cover is open (all the way up). the state is:

state: closed State attributes: { "current_position": 0, "friendly_name": "Klára dveře", "supported_features": 15 }

pszafer commented 5 years ago

@bruxy70 I made PR for this issue. Position is working properly, but I set state as 0 - closed, 100 - open. Didn't noticed it before. Sorry. If you want to, you can edit this mqtt cover file locally for now and check if it is working properly. Just change

self._state = self._position == 0

to

self._state = self._position == self._position_closed

bruxy70 commented 5 years ago

Thanks, that was quick :) I am not really sure how to change the file inside the docker image. I will wait for the next release if that is ok?

pszafer commented 5 years ago

That's ok. You can wait. Hope it's gonna be working all right for you.

bruxy70 commented 5 years ago

Ok, i put the mqtt.py changed in #18407 to custom_componnents/cover and restarted hassio, I guess that should do the trick.

Unfortunately, it does not work.

It did change the attributes so on position 0 it says open and on position 100 it says closed. But it still disables the down arrow instead of up arrow on position 0, and it disables up arrow instead of up arrow on position 100.

It also started to act weird - if I take the position slide to 100, it will go to position 0 instead. Then I tried to take it to position somewhere in the middle, but it went all the way to position 100. And it got stuck there. When I try to go to position 0, it moves the slider, but the cover does not move. And the up arrow is disabled.

So I will remove the custom component and restart again.

bruxy70 commented 5 years ago

Did some further testing (did not want to move the covers too much when everybody slept ;)

When I call cover.set_cover_position with position 0, the cover goes to 100. With position 100 it goes to 0. With position 20 it goes to 80. I did that from the devekoper tools/services,

(Interestingly, the slider shows the correct position, so when I move the cover slider to the right, it starts moving left and vice versa).

When I call service mqtt_publish instead of cover.set_position, the cover behaves correctly, so I do not think it is an issue with the cover. So when I call it with parameters

{
 "topic": "klara/cover1/position",
 "payload": 100
}

It goes to 100. 80 goes to 80 etc.

If I set

    position_open: 100
    position_closed: 0

(which is not correct), cover.set_cover_position works correctly, but then the state is closed when it should be open and vice versa.

And the issue with the disabled arrows at the 0 and 100 position is not solved in any scenario, whether the open_position/closed is 0/100 or 100/0

This actually works this way in 0.82.0 as well as with the new cover/mqtt.py. The only difference it makes is that it correctly reports state as closed on 100 and open on 0.

bruxy70 commented 5 years ago

@pszafer, I think I found the bug. The lines 462, 463 (in find_percentage_in_range) and 486,487 (in find_in_range_from_percent) they are:

            max_range = self._position_open
            min_range = self._position_closed

Assuming that open is greater than closed. Then on lines 467 and 492 it calculates current_range = max_range - min_range Which would be negative if closed position is greater than open.

So, on my local drive, I have changed this to this:

            if self._position_open > self._position_closed:
                max_range = self._position_open
                min_range = self._position_closed
            else:
                max_range = self._position_closed
                min_range = self._position_open

And the positioning works the way it should! (or you can do somehing more inteligent, I am not really a programmer ;) )

I think there is the same bug for tilt btw.

Said that, this still does not solve the issue that the wrong arrow gets disabled - when I reach 100, it disables the up arrow and leaves the down arrow. It needs to be the other way around,

pszafer commented 5 years ago

@bruxy70 about arrows I have no idea for now. Tests are ok, but tests might be wrong.

About find_percentage_in_range and find_in_range_from_percent, I think it is okay the way it was. At least for version <0.82. Slider is working from 0 - 100% where left side is close position - 0, right side is open position 100%.

Regular range:

open = 100
close = 0
Slider sends - 39% - is simple 39% it should look like this:

find_percentage_in_range =

max_range = 100
min_range = 0
position = 39
current_range = max_range - min_range = 100
offset_position = position - min_range = 39 - 0 = 39
position_percentage = round(float(offset_position) / current_range * 100.0) = 39/100*100 = 39

find_in_range_from_percent =

max_range = 100
min_range = 0
percentage = 39
offset = min_range = 0
current_range = max_range - min_range = 100
position = round(current_range * (percentage / 100.0)) = 100 * (39/100) = 39
position += offset = 39 + 0 = 39

Inverted range:

open = 0
close = 100

Slider sends 39%, HA should send 61% to your cover, but return 39% for frontend.

find_percentage_in_range =

max_range = 0
min_range = 100
position = 39
current_range = max_range - min_range = -100
offset_position = position - min_range = 39 - 100 = -61
position_percentage = round(float(offset_position) / current_range * 100.0) = -61/-100*100 = 61

find_in_range_from_percent =

max_range = 0
min_range = 100
percentage = 39
offset = min_range = 100
current_range = max_range - min_range = -100
position = round(current_range * (percentage / 100.0)) = -100 * (39/100) = -39
position += offset = -39 + 100 = 61

I made gist with one change, it might be this which is messing up for you. Can you check it out? https://gist.github.com/pszafer/f2891584e878f747321be3cc45a6fec5

I agree about tilt. I think tilt is messed up, but it was like this before. I didn't change anything to tilt.

bruxy70 commented 5 years ago

I do not understand why HA should send 61 if position is 39. The way it worked before is: 100 is fully deployed (fully covered, all the way down) 0 is open (hidden, not visible, all the way up - 0) so 20% is 20% rolled out. Why would I say I want 80% cover of I want 20? I thought it makes sense

With this gist, it seems to work from the UI - if I put the position to 100, it really goes to 100. But the cover actually goes back to 0.

Also, the idea that max is smaller than min makes no sense. And negative distance makes no sense neither. I don't get it.

pszafer commented 5 years ago

For HA frontend 100 is open and 0 is closed. This is open:

Imagine that you have multiple covers and some of them are working normal way and some inverted. Then slider would work different way for each cover. It has to be working same way for all types of covers.

bruxy70 commented 5 years ago

What is then the purpose of being able to configure position_open and position_closed, if it is fixed to 100 and 0? Ok, will try to re-map the values on the cover itself. Thanks for help. V

pszafer commented 5 years ago

I'm not saying that values are fixed in the home assistant. Values are "fixed" in slider only because slider sends percentage from 0 to 100, where 0 supposed to be closed and 100 is fully open.

pszafer commented 5 years ago

@bruxy, you shouldn't need to remap anything. One thing maybe you should accept that in slider mode you'd need use left side (0% slider) to close, right side (100% slider) to open. You can check out if MQTT is working properly for you with this PR: https://github.com/home-assistant/home-assistant/pull/18456

home-assistant / core

Undocumented change in mqtt cover in 0.82.0 & open/closed position does not work #18383