Venetian blind support for proprietary Fibaro FGRM-222 Roller Shutter devices #24405

ChristianKuehnel · 2019-06-08T10:10:20Z

Breaking Change:

Should not break anything

Description:

The Fibaro FGR-222/FGRM-2222 ZWave roller shutter devices have a proprietary command class to support setting the tilt angle of venetian blinds (= type of window cover). This PR adds the support to HA for this. This allows the user to set the height of the blinds and the tilt angle separately. It can be used to create nice scenes, e.g. blinds down and angle at 30% so that there is still some light coming.

⚠️ This PR depends on a change in the open-zwave library: home-assistant/open-zwave#6

Related issue (if applicable): fixes #

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#<home-assistant.io PR number goes here>

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

zwave:
  device_config:
    cover.mycover:
      tilt_open_position: 60

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist

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

Documentation added/updated in home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
Untested files have been added to .coveragerc.

If the code does not interact with devices:

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

ghost · 2019-06-08T10:10:26Z

Hey there @home-assistant/z-wave, mind taking a look at this pull request as its been labeled with a integration (zwave) you are listed as a codeowner for? Thanks!

This is a automatic comment generated by codeowners-mention to help ensure issues and pull requests are seen by the right people.

homeassistant/components/zwave/cover.py

andre-richter · 2019-06-08T11:41:54Z

Awesome, looking forward so much for this to land 🎉

homeassistant/components/zwave/cover.py

andre-richter · 2019-06-09T09:31:23Z

I propose to make it configurable.

Hardcode to a default of 99 or 50 (49?), and let the user be able to configure an override. Somebody other than us might have a third opinion about what open means, we never know.

Even in my case I might use this feature to set "open" to around 90, because fully open allows the light to bounce off the slat and directly into my eye in some situations, which is inconvenient. Whereas a very tiny downwards angle already prevents that but still has the "open" feeling.

Because of that, I would be fine with whatever hardcoding you go for.

ChristianKuehnel · 2019-06-09T13:53:25Z

OK, I added a parameter (see config example above).

andre-richter · 2019-06-09T16:21:27Z

Thanks for the update!

For lack of pushing rights to your branch, leaving this diff here (should be better readable than text comments in this case):

diff --git a/homeassistant/components/zwave/cover.py b/homeassistant/components/zwave/cover.py
index 3542df9..b1fe772 100644
--- a/homeassistant/components/zwave/cover.py
+++ b/homeassistant/components/zwave/cover.py
@@ -2,7 +2,7 @@
 import logging
 from homeassistant.core import callback
 from homeassistant.components.cover import (
-    DOMAIN, SUPPORT_OPEN, SUPPORT_CLOSE, ATTR_POSITION)
+    DOMAIN, SUPPORT_OPEN, SUPPORT_CLOSE, ATTR_POSITION, ATTR_TILT_POSITION)
 from homeassistant.components.cover import CoverDevice
 from homeassistant.helpers.dispatcher import async_dispatcher_connect
 from . import (
@@ -252,17 +252,17 @@ class FibaroFGRM222(ZwaveRollershutter):
         else:
             # Limit the range to [0-99], as this what that the ZWave command
             # accepts.
-            tilt_position = max(0, min(99, kwargs.get(ATTR_POSITION)))
+            tilt_position = max(0, min(99, kwargs.get(ATTR_TILT_POSITION)))
             _LOGGER.debug("setting tilt to %d", tilt_position)
             self._value_tilt.data = tilt_position
 
     def open_cover_tilt(self, **kwargs):
         """Set slats to horizontal position."""
-        self.set_cover_tilt_position(position=self._open_tilt_position)
+        self.set_cover_tilt_position(tilt_position=self._open_tilt_position)
 
     def close_cover_tilt(self, **kwargs):
         """Close the slats."""
-        self.set_cover_tilt_position(position=0)
+        self.set_cover_tilt_position(tilt_position=0)
 
     def set_cover_position(self, **kwargs):
         """Move the roller shutter to a specific position.

My tests:

Slider working.
Open/Close buttons working.
tilt_open_position config parameter working.

ChristianKuehnel · 2019-06-10T14:20:00Z

I applied the patch @andre-richter supplied. I can't test this right now...

andre-richter · 2019-06-10T17:13:01Z

@home-assistant/z-wave, I have independently verified the functionality of this PR on my Fibaro controllers. The code works for @ChristianKuehnel as well, hence two real-life tests in total.

Feel free to start your review :)

BR,
Andre

andre-richter · 2019-06-14T09:15:41Z

@balloob, sorry to bother you on this one. It's not visible to us who is in the @home-assitant/z-wave group. Since it seems like this PR is getting little attention, wanted ask you if you could ping/delegate somebody for review or tell us who to talk to?

Thanks,
Andre

andre-richter · 2019-06-24T16:28:53Z

Pinging again since this is now over two weeks old.

@dmulcahey @Adminiuga please let us know how we can help you to drive this to closure.

Thanks,
Andre

Adminiuga · 2019-06-24T17:32:33Z

@a-andre hrm, I really have 0 exposure to the ZWave component, more on the ZHA/Zigbee side. From my perspective nothing jumps out, but again, have no zwave exposure.

@cgarwood any chance you could take a look at this PR?

Adminiuga · 2019-06-24T17:33:19Z

@ChristianKuehnel can you please rebase it? showing conflicts with the current dev.

ChristianKuehnel · 2019-06-24T18:21:25Z

@ChristianKuehnel can you please rebase it? showing conflicts with the current dev.

I just squashed the changes and rebased them on dev.

cgarwood · 2019-06-25T02:09:02Z

I don't have a lot of experience in the zwave quirks/workarounds or covers, but the python looks fine to me. I can't speak for the required openzwave patch though.

andre-richter · 2019-06-25T04:56:44Z

I can't speak for the required openzwave patch though

I would argue that QA/review on this one has already been done by the openzwave guys themselves, since the respective patch is already upstream in openzwave master.

andre-richter · 2019-07-05T19:44:10Z

Ping =/

Guys believe me I am not comfortable nagging here all the time. But we still got no idea who would be responsible for zwave code in the homeassistant team at the moment.

I tried to ping @pvizeli on the corresponding zwave patch but he seems busy.

Best,
Andre

MartinHjelmare · 2019-07-09T02:55:33Z

homeassistant/components/zwave/cover.py

+        self._open_tilt_position = 50  # type: int
+        if open_tilt_position is not None:
+            self._open_tilt_position = open_tilt_position
+        _LOGGER.debug('open_tilt_position: %s', open_tilt_position)


We don't want side effects, eg logging, in init method.

ChristianKuehnel · 2019-07-09T07:43:37Z

@MartinHjelmare I just removed the init statement.

So the rest is fine and can be merged?
Can you also do the review for the related change home-assistant/open-zwave#6 to the Z-Wave library?

MartinHjelmare · 2019-07-09T08:23:24Z

Someone that knows the zwave integration should review here.

andre-richter · 2019-07-09T08:49:29Z

@MartinHjelmare Could you do us a big favor and reach out in your internal channels who currently owns this?

We tried to ping all people that, from the outside, looked responsible, but no luck so far in finding someone who wants to own this.

Thanks a lot,
Andre

andre-richter · 2019-09-01T13:06:38Z

Rebased on top of 0.98.1:

https://github.com/andre-richter/home-assistant/tree/0.98.1_fibaro_222_tilt

MartinHjelmare · 2019-09-07T08:46:52Z

I think we should close this PR, since it depends on another PR that hasn't been merged yet. If the dependency is merged a new PR can be opened.

andre-richter · 2019-09-07T09:28:40Z

Any chance to get more traction on the other PR? Its been ignored for 3 months straight now :(

And it's not even a complicated patch that needs much review, because it is just a cherry-pick from the master open-zwave repo where the code has been reviewed by open-zwave maintainers and is merged already.

cgarwood · 2019-09-07T13:43:40Z

Still can't speak for the HA ozw1.4 fork, but I'm in the process of updating the HA zwave component for OZW1.6 which should re-align HA with the OZW master. Downside is I'm still a few weeks out most likely from having the updated component ready to go.

MartinHjelmare · 2019-09-07T14:50:35Z

Let's close this for now. As soon as the library is ready we can accept this again.

andre-richter · 2019-09-07T16:08:06Z

1.6 sounds good. Let's wait for that 👍

ChristianKuehnel · 2019-09-16T14:03:36Z

The library changes was just merged: home-assistant/open-zwave#6

Not sure if we need to wait for a release or if the master branch is being used...

@ChristianKuehnel

The Fibaro FGR-222/FGRM-2222 ZWave roller shutter devices have a proprietary command class to support setting the tilt angle of venetian blinds (= type of window cover). This PR adds the support to HA for this. This allows the user to set the height of the blinds and the tilt angle separately. Original patch by @ChristianKuehnel #24405.

ChristianKuehnel requested a review from a team as a code owner June 8, 2019 10:10

homeassistant added cla-signed integration: zwave labels Jun 8, 2019

ChristianKuehnel force-pushed the fibaro2019 branch from 6a25e98 to 45d02d9 Compare June 8, 2019 10:19

frenck mentioned this pull request Jun 8, 2019

Fixes linter warning in ZHA sensor #24406

Merged

9 tasks

ChristianKuehnel requested review from Adminiuga and dmulcahey as code owners June 8, 2019 10:23

andre-richter reviewed Jun 8, 2019

View reviewed changes

homeassistant/components/zwave/cover.py Outdated Show resolved Hide resolved

homeassistant/components/zwave/cover.py Show resolved Hide resolved

homeassistant/components/zwave/cover.py Outdated Show resolved Hide resolved

andre-richter reviewed Jun 8, 2019

View reviewed changes

homeassistant/components/zwave/cover.py Outdated Show resolved Hide resolved

andre-richter mentioned this pull request Jun 22, 2019

ManufacturerProprietary for Fibaro FGRM-222 home-assistant/open-zwave#6

Merged

Venetien blind support for Fibaro FGRM-222

926a8c2

ChristianKuehnel force-pushed the fibaro2019 branch from 59f380a to 926a8c2 Compare June 24, 2019 18:20

fixed line length

b3ad7b9

MartinHjelmare reviewed Jul 9, 2019

View reviewed changes

removed logging from __init__

3e17e27

MartinHjelmare added this to Needs review in Dev Jul 23, 2019

MartinHjelmare added the second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. label Sep 7, 2019

project-bot bot moved this from Needs review to Second opinion wanted in Dev Sep 7, 2019

MartinHjelmare closed this Sep 7, 2019

Dev automation moved this from Second opinion wanted to Cancelled Sep 7, 2019

MartinHjelmare removed the second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. label Sep 7, 2019

andre-richter mentioned this pull request Dec 8, 2019

Venetian blind support for proprietary Fibaro FGRM-222 Roller Shutter devices #29701

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Venetian blind support for proprietary Fibaro FGRM-222 Roller Shutter devices #24405

Venetian blind support for proprietary Fibaro FGRM-222 Roller Shutter devices #24405

ChristianKuehnel commented Jun 8, 2019 •

edited

ghost commented Jun 8, 2019

andre-richter commented Jun 8, 2019

andre-richter commented Jun 9, 2019 •

edited

ChristianKuehnel commented Jun 9, 2019

andre-richter commented Jun 9, 2019

ChristianKuehnel commented Jun 10, 2019

andre-richter commented Jun 10, 2019

andre-richter commented Jun 14, 2019

andre-richter commented Jun 24, 2019

Adminiuga commented Jun 24, 2019

Adminiuga commented Jun 24, 2019

ChristianKuehnel commented Jun 24, 2019

cgarwood commented Jun 25, 2019

andre-richter commented Jun 25, 2019

andre-richter commented Jul 5, 2019

MartinHjelmare Jul 9, 2019

ChristianKuehnel commented Jul 9, 2019

MartinHjelmare commented Jul 9, 2019

andre-richter commented Jul 9, 2019 •

edited

andre-richter commented Sep 1, 2019

MartinHjelmare commented Sep 7, 2019

andre-richter commented Sep 7, 2019 •

edited

cgarwood commented Sep 7, 2019

MartinHjelmare commented Sep 7, 2019

andre-richter commented Sep 7, 2019

ChristianKuehnel commented Sep 16, 2019

Venetian blind support for proprietary Fibaro FGRM-222 Roller Shutter devices #24405

Venetian blind support for proprietary Fibaro FGRM-222 Roller Shutter devices #24405

Conversation

ChristianKuehnel commented Jun 8, 2019 • edited

Breaking Change:

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

ghost commented Jun 8, 2019

andre-richter commented Jun 8, 2019

andre-richter commented Jun 9, 2019 • edited

ChristianKuehnel commented Jun 9, 2019

andre-richter commented Jun 9, 2019

ChristianKuehnel commented Jun 10, 2019

andre-richter commented Jun 10, 2019

andre-richter commented Jun 14, 2019

andre-richter commented Jun 24, 2019

Adminiuga commented Jun 24, 2019

Adminiuga commented Jun 24, 2019

ChristianKuehnel commented Jun 24, 2019

cgarwood commented Jun 25, 2019

andre-richter commented Jun 25, 2019

andre-richter commented Jul 5, 2019

MartinHjelmare Jul 9, 2019

Choose a reason for hiding this comment

ChristianKuehnel commented Jul 9, 2019

MartinHjelmare commented Jul 9, 2019

andre-richter commented Jul 9, 2019 • edited

andre-richter commented Sep 1, 2019

MartinHjelmare commented Sep 7, 2019

andre-richter commented Sep 7, 2019 • edited

cgarwood commented Sep 7, 2019

MartinHjelmare commented Sep 7, 2019

andre-richter commented Sep 7, 2019

ChristianKuehnel commented Sep 16, 2019

ChristianKuehnel commented Jun 8, 2019 •

edited

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

andre-richter commented Jun 9, 2019 •

edited

andre-richter commented Jul 9, 2019 •

edited

andre-richter commented Sep 7, 2019 •

edited