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

Always save current position if payload is numeric value #16148

Merged
merged 7 commits into from Nov 1, 2018

Conversation

Projects
None yet
5 participants
@pszafer
Contributor

pszafer commented Aug 23, 2018

Currently if payload is even with var state_open or state_close HA save only position open or close without saving current_position.
Without current_position value, slider is hidden from HA frontend.
I changed this condition, so if payload is numeric, current_position will be always saved even if cover is fully open or closed.

Documentation PR: home-assistant/home-assistant.io#7279

Checklist:

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

This comment has been minimized.

Contributor

pszafer commented Sep 5, 2018

@MartinHjelmare can you check it and if ok merge?

@MartinHjelmare

I'm thinking this is correct. The position will be saved for binary logical payloads 0 and 1 too. Is that a problem?


if payload == self._state_open:
is_numeric_payload = payload.isnumeric()
if payload == self._state_open and not is_numeric_payload:

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 5, 2018

Member

It is easier to read if we put the numeric condition first in the expression.

@MartinHjelmare

Please revert the vertical order of statements change.

if int(payload) > 0:
self._state = False
else:
self._state = True
self._position = int(payload)
elif payload == self._state_open and not is_numeric_payload:

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 5, 2018

Member

No, I meant to change just this line so that we start by checking if payload is numerical.

if int(payload) > 0:
self._state = False
else:
self._state = True
self._position = int(payload)
elif payload == self._state_open and not is_numeric_payload:
self._state = False
elif payload == self._state_closed and not is_numeric_payload:

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 5, 2018

Member

Same as above.

@pszafer

This comment has been minimized.

Contributor

pszafer commented Sep 5, 2018

Done.

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Sep 5, 2018

What do you think about my question?

@pszafer

This comment has been minimized.

Contributor

pszafer commented Sep 5, 2018

if it is numeric it is better payload being saved as than HA UI allow to change cover position with slider.

Documentation and warning is clear about that:

Payload is not True, False, or integer (0-100): %s", payload)

One thing I can think of, which might be a problem if there is somebody who want's for some reason inverted numbers.
For now it is only: Open > 0, Close == 0.
It's not gonna work if somebody wants Close == 100, Open < 100.

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Sep 6, 2018

I'm not sure we understand each other. The problem I'm seeing with the new logic, is that if a user wants to use payload 0 for closed state and payload 1 for open state, those will now fall only into the third case as numeric and update the position. State will still match the 0 and 1 payloads but the slider always range between 0 - 100 so then it will look weird with 1 being fully opened.

Do you see what I'm thinking?

Can you test this case, to see if it actually will occur or if there's some other logic in place that avoids this?

@pszafer

This comment has been minimized.

Contributor

pszafer commented Sep 6, 2018

I understand you. I think this is not according to all docs, that cover position should be represented as percentage.
Here is some discussion about inverted option: stefanbode/Sonoff-Tasmota#53

Maybe safer would be something like this:

if payload == self._state_open:
  self._state = False
elif payload == self._state_closed:
  self._state = True
if payload.isnumeric() and 0 <= int(payload) <= 100:
  self._position = int(payload)
@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Sep 6, 2018

From the cover base entity class:

@property
def current_cover_position(self):
    """Return current position of cover.
    None is unknown, 0 is closed, 100 is fully open.
    """
    pass

The later suggestion would be a breaking change for users that only send a numeric payload and rely on existing logic that says that 0 is closed and above 0 is open.

@pszafer

This comment has been minimized.

Contributor

pszafer commented Sep 6, 2018

So let's leave like it is now or maybe you'd prefer something like this:

payload_saved = False
if payload == self._state_open:
  self._state = False
  payload_saved = True
elif payload == self._state_closed:
  self._state = True
  payload_saved = True
if payload.isnumeric() and 0 <= int(payload) <= 100:
  self._position = int(payload)
  if not payload_saved:
     if int(payload) > 0:
        self._state = False
     else:
        self._state = True
@pszafer

This comment has been minimized.

Contributor

pszafer commented Sep 15, 2018

@MartinHjelmare what do you think about last proposition?

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Sep 15, 2018

I think you should test live what happens in the mentioned case.

@pszafer

This comment has been minimized.

Contributor

pszafer commented Sep 20, 2018

Ok, I tested it and it doesn't break anything if user set such option:

  state_open: "1"
  state_closed: "0"

But I don't like it... because it is only workaround.
If somebody has inverted cover he has to click the other way in web ui in slider like this:

  state_open: "0"
  state_closed: "50"

I changed this function once again.
I assumed that position value should be integer between with boundaries between open/close value.

Ended up with function like below.
I can push it if you think that it is good idea.

With below function we calculate percentage position if open != 100 and close != 0 and than we can handle inverted situation (if close is higher number than open).

   @callback
        def state_message_received(topic, payload, qos):
            """Handle new MQTT state messages."""
            if self._template is not None:
                payload = self._template.async_render_with_possible_json_value(
                    payload)
            payload_saved = False
            numeric_states = (payload.isnumeric() and
                              self._state_open.isnumeric() and
                              self._state_closed.isnumeric())

            if payload == self._state_open:
                self._state = False
                payload_saved = True
            elif payload == self._state_closed:
                self._state = True
                payload_saved = True
            if numeric_states:
                open_state = int(self._state_open)
                closed_state = int(self._state_closed)
                payload = int(payload)
                if ((open_state <= payload <= closed_state) or
                   (open_state >= payload >= closed_state)):
                    if (open_state == 100 and closed_state == 0):
                        percent_payload = payload
                    else:
                        """Formula:((value - min) / (max - min)) * 100 """
                        percent_payload = (abs(((payload - open_state)
                                           / (closed_state - open_state))*100))
                    self._position = percent_payload
                    if not payload_saved:
                        if percent_payload > 0:
                            self._state = False
                        else:
                            self._state = True
                    payload_saved = True
            if not payload_saved:
                if numeric_states:
                    if (open_state < closed_state):
                        _LOGGER.warning(
                            "Payload is not integer between (%s - %s): %s",
                            self._state_closed, self._state_open, payload)
                    elif (open_state == closed_state):
                        _LOGGER.warning(
                            "Open and closed values cannot be equal (%s - %s)",
                            self._state_open, self._state_closed)
                    else:
                        _LOGGER.warning(
                            "Payload is not integer between (%s - %s): %s",
                            self._state_open, self._state_closed, payload)
                else:
                    _LOGGER.warning(
                        "Payload is not True, False or Integer: %s", payload)
                return
            self.async_schedule_update_ha_state()

Hope it makes sense for you.

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Sep 20, 2018

How does the slider for position react when open and closed states are 1 and 0?

@pszafer

This comment has been minimized.

Contributor

pszafer commented Sep 20, 2018

Position close:
image

Position open:
image

But clicking slider sends value between 0 - 100.
What I'm thinking is to change function async def async_set_cover_position
and calculate proper integer .position
eg if somebody have 20 for open and 0 for close and click in slider for 50, it should send 10.
Am I right about that?

@pszafer

This comment has been minimized.

Contributor

pszafer commented Sep 22, 2018

@MartinHjelmare I made changes as I was thinking yesterday.
Now it is the same logic for tilt (it was already done before) and positioning (both reading and setting).
So if I have

open: 1
close: 0

and I click on slider 41% it will send close command as closest integer is 0 etc.

If your initial states for open/close aren't numeric it will send percentage number between 0 - 100% like it was before.
For any other number it will find_in_range_from_percent and send proper value

@pszafer pszafer force-pushed the pszafer:mqtt_cover branch from 7dea4a9 to 9e15b26 Sep 26, 2018

@MartinHjelmare MartinHjelmare requested a review from home-assistant/core Sep 26, 2018

@balloob

This comment has been minimized.

Member

balloob commented Sep 27, 2018

I think that we should not overload a topic to be able to handle both. We should instead have a new topic that can take the current position.

@pszafer

This comment has been minimized.

Contributor

pszafer commented Sep 27, 2018

What do you mean by both?
Topic is handling only position/payload and we are interpreting this position to state.

Would you like to have something like this below?

###VERSION 1
- platform: mqtt 
  name: "TEST" 
  command_topic: "cmnd/cover_test/backlog" 
  state_topic: "stat/cover_test/SHUTTER1" 
  availability_topic: "tele/cover_test/LWT" 
  set_position_topic: "cmnd/cover_test/shutterposition" 
  payload_open: "shutteropen" 
  payload_close: "shutterclose" 
  payload_stop: "shutterstop" 
  state_open: "100" //or OPEN  <----
  state_closed: "0" // or CLOSE <------
  payload_open: "100" <-------
  payload_closed: "0" <--------
  payload_available: "Online" 
  payload_not_available: "Offline" 
  optimistic: false

vs

###VERSION 2
- platform: mqtt 
  name: "TEST" 
  command_topic: "cmnd/cover_test/backlog" 
  state_topic: "stat/cover_test/SHUTTER1"
  position_topic: "stat/cover_test/SHUTTER1POSTITION" <--------
  availability_topic: "tele/cover_test/LWT" 
  set_position_topic: "cmnd/cover_test/shutterposition" 
  payload_open: "shutteropen" 
  payload_close: "shutterclose" 
  payload_stop: "shutterstop" 
  state_open: "100"  //or OPEN <--------
  state_closed: "0" // or CLOSE <--------
  payload_open: 100 <--------
  payload_closed: 0 <--------
  payload_available: "Online" 
  payload_not_available: "Offline" 
  optimistic: false

one advantage of such idea, would be that we can check if somebody is giving us integer as payload_open and payload_closed.
Can you explain why do you think we should make such change?

@@ -1,7 +1,7 @@
"""The tests for the MQTT cover platform."""
import unittest

import homeassistant.components.cover as cover
from homeassistant.components import cover, mqtt

This comment has been minimized.

@houndci-bot

houndci-bot Oct 1, 2018

'homeassistant.components.mqtt' imported but unused

@@ -45,6 +45,7 @@
SUPPORT_OPEN = 1
SUPPORT_CLOSE = 2
SUPPORT_SET_POSITION = 4
SUPPORT_GET_POSITION = 6

This comment has been minimized.

@balloob

balloob Oct 1, 2018

Member

We can't change the entity model.

This comment has been minimized.

@pszafer

pszafer Oct 1, 2018

Contributor

Ok, will remove that.
In which supported_features should I put get_position? SUPPORT_SET_POSITION or OPEN_CLOSE_FEATURES?

This comment has been minimized.

@balloob

balloob Oct 2, 2018

Member

It's not needed to add it as a feature.

@balloob

This comment has been minimized.

Member

balloob commented Oct 1, 2018

We need to make sure with any option that it serves a single purpose and one purpose only.

We should also make sure that options don't conflict with one another, and if they do, they should not be allowed to be used together.

So if you want to support a current position payload, first off, let's make sure PR is limited to just that functionality. Then to add support for a current_position topic, it is not allowed to be used together with the state topic. It's one or the other. Then the value of current_position topic is what should be returned from the current_cover_position property. No checks, besides conversion to a number, no transformations.

@pszafer

This comment has been minimized.

Contributor

pszafer commented Oct 1, 2018

@balloob I can do this, but if I understand you right, it will break all existing mqtt covers implementation, eg. Tasmota forks etc. By break I mean usage of get_position.
User would have to subscribe to same topic for open/close and get_position. Is it ok?
I made it this way that we can just use state_topic same time as position_topic.
If you confirm that I understand you correctly, I'll try to do that tomorrow after work.

Example:

state_topic: 'state-topic'
position_topic: 'position-topic'
state_open: 100
state_closed: 0
position_open: 100
position_closed: 0

With such config and not using state/position together I don't see room for assumption that state_open <= current_state <= state_closed, so cover would have to always report state_open or state_closed value for one function and position for another.

Second, what do you think about my last question find_percentage_in_range? I don't have tilt so I'm not sure, but for me this inverted condition is messed up.

@balloob

This comment has been minimized.

Member

balloob commented Oct 2, 2018

This PR needs to focus, so tilt is out of scope for this PR.

What do you mean with subscribe to same topic for state and position? The user should either configure a position or a state topic, not both, as the state can be derived from the position itself?

@pszafer

This comment has been minimized.

Contributor

pszafer commented Oct 2, 2018

This PR needs to focus, so tilt is out of scope for this PR.

What do you mean with subscribe to same topic for state and position? The user should either configure a position or a state topic, not both, as the state can be derived from the position itself?

All the time I'm talking about backward compability, because I imagine that in future developers of mqtt covers like Tasmota can change it behaviours and send such mqtt topics:

pub state-topic -m OPEN
pub position-topic -m 60

But for now we have at least 3 things to consider:

  1. If user has implementation that has state_topic:
state_topic: 'state-topic'
state_open: 100
state_closed: 0

It would be easiest for user to just add:

state_topic: 'state-topic'
position_topic: 'state-topic'
state_open: 100
state_closed: 0
position_open: 100
position_closed: 0

And then in mqtt cover we should check if state_topic and position_topic is the same and then set the state according to position. It should be done by get_position function.
And this would follow your idea to configure either state or position topic as one could be derived fron another.

  1. If user has position <1-99> and OPEN/CLOSE states, but still by the shared topic...
state_topic: 'state-topic'
position_topic: 'state-topic'
state_open: OPEN
state_closed: CLOSE
position_open: 100
position_closed: 0

I imagine that in this situation get_state function should set position if state_open == payload

  1. Above situations should have inverted (open-0, close-100) options (which is kind of transformation value/percentage ) and allow user to have non percentage integers.
    If we don't do that slider in HA UI will be working different for regular/inverted covers (left will close for regular and left will open for inverted).

Hope you understand what I'm talking about.

@balloob

This comment has been minimized.

Member

balloob commented Oct 9, 2018

I think I understand and I disagree. A topic should have a single purpose. Either receiving OPEN/CLOSE or it is receiving a number. We should allow the user to configure either a position topic or a state topic but not both.

@pszafer pszafer force-pushed the pszafer:mqtt_cover branch from 946ebde to 6556cc7 Oct 24, 2018

@pszafer

This comment has been minimized.

Contributor

pszafer commented Oct 24, 2018

@balloob I think I did it the way you see it. From tomorrow I'll be testing it in my house.

@balloob

This comment has been minimized.

Member

balloob commented Oct 30, 2018

Looks good. Ok to merge when conflicts resolved.

@pszafer pszafer force-pushed the pszafer:mqtt_cover branch from 1be70de to abf59cb Oct 30, 2018

@pszafer pszafer referenced this pull request Oct 30, 2018

Merged

Cover get position topic #7279

0 of 2 tasks complete
@MartinHjelmare

Great!

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Nov 1, 2018

Can be merged when build passes.

@balloob balloob merged commit c3e3f66 into home-assistant:dev Nov 1, 2018

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 93.064%
Details

@wafflebot wafflebot bot removed the in progress label Nov 1, 2018

@balloob

This comment has been minimized.

Member

balloob commented Nov 1, 2018

🎉

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