Add support for MySensors cover #3512

OttoWinter · 2016-09-25T12:55:43Z

Description:
Addition of support for MySensors cover.

Related issue (if applicable): Not applicable

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#986

Example entry for configuration.yaml (if applicable):
Not applicable.

Checklist:

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

Documentation added/updated in home-assistant.io

If code communicates with devices, web services, or a:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
[N/A] New dependencies have been added to the REQUIREMENTS variable (example).
[N/A] New dependencies are only imported inside functions that use them (example).
[N/A] New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
[N/A] New files were added to .coveragerc.

If the code does not interact with devices:

[N/A] Local tests with tox run successfully. Your PR cannot be merged unless tests pass
[N/A] Tests have been added to verify that the new code works.

The MySensors documentation doesn’t specify when sending a V_PERCENTAGE is allowed.

balloobbot · 2016-09-25T12:56:22Z

homeassistant/components/mysensors.py

@@ -158,7 +158,7 @@ def gw_start(event):
            'No devices could be setup as gateways, check your configuration')
        return False

-    for component in 'sensor', 'switch', 'light', 'binary_sensor', 'climate':
+    for component in 'sensor', 'switch', 'light', 'binary_sensor', 'climate', 'cover':


farcy v1.1

80: E501 line too long (86 > 79 characters)

MartinHjelmare

Three comments and also add set_cover_position back in. Mysensors do specify V_PERCENTAGE for cover and even if it didn't, we would want to use that, and we are free to do so.

Thanks for contributing!

MartinHjelmare · 2016-09-25T14:44:35Z

homeassistant/components/cover/mysensors.py

+        pres = gateway.const.Presentation
+        set_req = gateway.const.SetReq
+        map_sv_types = {
+            pres.S_COVER: [set_req.V_UP],


Use V_PERCENTAGE instead cause you use that to check state.

MartinHjelmare · 2016-09-25T15:04:05Z

homeassistant/components/cover/mysensors.py

+    def is_closed(self):
+        """Return True if cover is closed."""
+        set_req = self.gateway.const.SetReq
+        return self._values.get(set_req.V_PERCENTAGE) == '0'


Compare to 0 ie an integer.

MartinHjelmare · 2016-09-25T15:10:18Z

homeassistant/components/cover/mysensors.py

+        for value_type, value in child.values.items():
+            _LOGGER.debug(
+                '%s: value_type %s, value = %s', self._name, value_type, value)
+            self._values[value_type] = value


Check for V_PERCENTAGE and force that value to an integer.

This reverts commit d78cb3a.

OttoWinter · 2016-09-25T15:34:23Z

My problem with set_cover_position was that in the home-assistant webinterface it would show a slider for the cover position even though it was not supported by my MySensors node (I'm using end-stoppers, so setting the position with percentages is not possible with my setup).

I don't know why/when this slider shows up, but I suspect it is because the set_cover_position function exists in the MySensorsCover class. Since I wasn't able to find a way to get around this issue (by manually checking the node whether it supports setting the cover position with percentages), I dropped the method.

Maybe you have an idea how to solve this?

MartinHjelmare

You made a good point. I think we can solve it and support both devices with set position and without, by adding V_STATUS as a possible state report. The user has to then decide to either use V_PERCENTAGE or V_STATUS as value type for reporting state.

It's the property current_position that makes the position slider show or not. If the former is None the slider is not shown.

MartinHjelmare · 2016-09-25T17:40:58Z

homeassistant/components/cover/mysensors.py

+        pres = gateway.const.Presentation
+        set_req = gateway.const.SetReq
+        map_sv_types = {
+            pres.S_COVER: [set_req.V_PERCENTAGE],


Add V_STATUS.

MartinHjelmare · 2016-09-25T17:46:25Z

homeassistant/components/cover/mysensors.py

+        for value_type, value in child.values.items():
+            _LOGGER.debug(
+                '%s: value_type %s, value = %s', self._name, value_type, value)
+            if value_type == set_req.V_PERCENTAGE:


Move this check and behavior to the update method in MysensorsDeviceEntity in the mysensors component, but use V_DIMMER instead to don't break version 1.4. Then you don't need to overwrite update here.

MartinHjelmare · 2016-09-25T17:50:32Z

homeassistant/components/cover/mysensors.py

+    def is_closed(self):
+        """Return True if cover is closed."""
+        set_req = self.gateway.const.SetReq
+        return self._values.get(set_req.V_PERCENTAGE) == 0


Check if V_PERCENTAGE is in self._values, if so return as before, if not fall back to get V_STATUS and compare that to STATE_OFF.

MartinHjelmare · 2016-09-25T17:52:06Z

homeassistant/components/cover/mysensors.py

+            self.node_id, self.child_id, set_req.V_UP, 1)
+        if self.gateway.optimistic:
+            # Optimistically assume that cover has changed state.
+            self._values[set_req.V_PERCENTAGE] = 100


Check for V_PERCENTAGE, before setting it. Add a check for V_STATUS and set that if present.

MartinHjelmare · 2016-09-25T17:52:25Z

homeassistant/components/cover/mysensors.py

+            self.node_id, self.child_id, set_req.V_DOWN, 1)
+        if self.gateway.optimistic:
+            # Optimistically assume that cover has changed state.
+            self._values[set_req.V_PERCENTAGE] = 0


Same as above.

OttoWinter · 2016-09-25T18:50:24Z

As V_PERCENTAGE is just an alias for V_DIMMER, just as V_STATUS is an alias for V_LIGHT, theoretically MySensors v1.4 could be supported, but the value_type names wouldn't be very appropriate.

Should the version check (if float(gateway.protocol_version) < 1.5: continue) be replaced by

if float(gateway.protocol_version) >= 1.5:
    map_sv_types.update({
        pres.S_COVER: [set_req.V_DIMMER, set_req.V_LIGHT],
    })

?

MartinHjelmare · 2016-09-25T21:25:06Z

We could make it easy for us and just use the old types, V_DIMMER and V_LIGHT, and remove any specific version check. Downside is that if the old types are removed from future mysensors versions we'll have to update the code to not break. I suggest doing it like this:

pres = gateway.const.Presentation
set_req = gateway.const.SetReq
map_sv_types = {
    pres.S_COVER: [set_req.V_DIMMER, set_req.V_LIGHT],
}
if float(gateway.protocol_version) >= 1.5:
    map_sv_types.update({
        pres.S_COVER: [set_req.V_PERCENTAGE, set_req.V_STATUS],
    })

MartinHjelmare · 2016-09-25T21:27:19Z

homeassistant/components/mysensors.py

@@ -337,7 +338,8 @@ def update(self):
            _LOGGER.debug(
                "%s: value_type %s, value = %s", self._name, value_type, value)
            if value_type in (set_req.V_ARMED, set_req.V_LIGHT,
-                              set_req.V_LOCK_STATUS, set_req.V_TRIPPED):
+                              set_req.V_LOCK_STATUS, set_req.V_TRIPPED,
+                              set_req.V_DIMMER):


This should not be here, but in a separate check. The value for V_DIMMER should just be forced to an int, not changed to a home assistant state. Use an elif statement.

My bad. Must have overlooked that.

`V_PERCENTAGE` and `V_DIMMER` are aliases just like `V_STATUS` and `V_LIGHT`, so the code inside `MySensorsCover` doesn’t need to be updated.

MartinHjelmare · 2016-09-25T22:11:56Z

Sorry, I forgot about all the rest of the types in the device class, V_PERCENTAGE and V_STATUS. We don't have access to those when specifying version 1.4 in the config, so now we either have to check for both old types, V_DIMMER and V_LIGHT and new types, V_PERCENTAGE and V_STATUS, at all the places we access the types, or we scrap trying to be portable. If we still want to port this to version 1.4 it might be easiest still just to use the old types everywhere, since they are aliases for the new types.

Sorry for going back and forth. What do you think?

OttoWinter · 2016-09-25T22:36:49Z

Not a problem. I wasn't aware the v1.5 SetReq is not available within a v1.4 gateway, but it makes sense now.

I think using the old V_TYPES in code for compatibility's sake is just fine.

MartinHjelmare · 2016-09-25T22:52:09Z

Unrelated failure of flaky test. I'll close and reopen to rerun Travis.

MartinHjelmare · 2016-09-25T23:39:00Z

@Streberpower If you're happy, I'm happy now. 🐬 Ok, to merge?

OttoWinter · 2016-09-26T05:07:10Z

Great, yes.

MartinHjelmare · 2016-09-26T06:56:00Z

I talked with @balloob and we said that we'll hold on merging new features until after 0.29. Hopefully that can go out soon. There's an issue with segfaults that is holding it up.

sytone · 2016-09-26T13:59:26Z

Can you do a exists checks on the old types and if not there use the new?

Sent from Outlook for iPhone

On Sun, Sep 25, 2016 at 2:25 PM -0700, "Martin Hjelmare" notifications@github.com wrote:

We could make it easy for us and just use the old types, V_DIMMER and V_LIGHT, and remove any specific version check. Downside is that if the old types are removed from future mysensors versions we'll have to update the code to not break. I suggest doing it like this:

pres = gateway.const.Presentation
set_req = gateway.const.SetReq
map_sv_types = {
    pres.S_COVER: [set_req.V_DIMMER, set_req.V_LIGHT],
}
if float(gateway.protocol_version) >= 1.5:
    map_sv_types.update({
        pres.S_COVER: [set_req.V_PERCENTAGE, set_req.V_STATUS],
    })

You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
#3512 (comment)

OttoWinter · 2016-09-26T14:16:08Z

Then you'd have to check which one of the V_TYPES (V_DIMMER or V_PERCENTAGE) is available in the MySensorsCover class too, which would just be a mess.

For example, consider the is_closed property. Right now it is

def is_closed(self):
    """Return True if cover is closed."""
    set_req = self.gateway.const.SetReq
    if set_req.V_DIMMER in self._values:
        return self._values.get(set_req.V_DIMMER) == 0
    else:
        return self._values.get(set_req.V_LIGHT) == STATE_OFF

But if one were checking which V_TYPE is defined, it'd be something along the lines of

def is_closed(self):
    """Return True if cover is closed."""
    set_req = self.gateway.const.SetReq
    if set_req.V_DIMMER in self._values:
        try:
            return self._values.get(set_req.V_DIMMER) == 0
        except AttributeError:
            return self._values.get(set_req.V_PERCENTAGE) == 0
    else:
        try:
            return self._values.get(set_req.V_LIGHT) == STATE_OFF
        except AttributeError:
            return self._values.get(set_req.V_STATUS) == STATE_OFF

Why bother? MySensors probably won't drop the old V_TYPES anytime soon since there's no real cost in maintaining these and many tutorials out there use the old ones. And if MySensors do, it would be an easy fix anyway. Home Assistant is already using the old V_TYPE all around the code base.

OttoWinter added 4 commits September 25, 2016 11:56

Added support for MySensors cover device

1a1f95e

Fixed set_req not defined

0ccb9f6

Fixed V_PERCENTAGE is str

3e36aea

Removed set_cover_position

d78cb3a

The MySensors documentation doesn’t specify when sending a V_PERCENTAGE is allowed.

OttoWinter mentioned this pull request Sep 25, 2016

Create component page for mysensors based cover home-assistant/home-assistant.io#986

Merged

balloobbot reviewed Sep 25, 2016

View reviewed changes

OttoWinter changed the title ~~My sensors cover~~ Added support for MySensors cover Sep 25, 2016

OttoWinter changed the title ~~Added support for MySensors cover~~ Add support for MySensors cover Sep 25, 2016

OttoWinter added 2 commits September 25, 2016 15:02

Fixed homeassistant/components/mysensors.py line too long

4dc8f0d

Fixed lint ATTR_POSITION imported but unused

7a3af03

MartinHjelmare requested changes Sep 25, 2016

View reviewed changes

OttoWinter added 3 commits September 25, 2016 17:22

Use V_PERCENTAGE for MySensors cover

2492f29

Revert "Removed set_cover_position"

0566200

This reverts commit d78cb3a.

Fix set_req, ATTR_POSITION not defined

d8ed976

MartinHjelmare requested changes Sep 25, 2016

View reviewed changes

Added support for non-exactly positionable covers

feb7c40

MartinHjelmare requested changes Sep 25, 2016

View reviewed changes

OttoWinter added 2 commits September 25, 2016 23:38

Fixed V_PERCENTAGE cast to bool

a7d1717

Ported MySensors cover back to v1.4

5f8d921

`V_PERCENTAGE` and `V_DIMMER` are aliases just like `V_STATUS` and `V_LIGHT`, so the code inside `MySensorsCover` doesn’t need to be updated.

Fixed v1.5 V_TYPES not in in v1.4 gateway SetReq

b54b736

MartinHjelmare closed this Sep 25, 2016

MartinHjelmare reopened this Sep 25, 2016

MartinHjelmare approved these changes Sep 25, 2016

View reviewed changes

MartinHjelmare merged commit d5f8aa5 into home-assistant:dev Sep 30, 2016

home-assistant locked and limited conversation to collaborators Mar 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Add support for MySensors cover #3512

Add support for MySensors cover #3512

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Add support for MySensors cover #3512

Add support for MySensors cover #3512

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!