ZHA cover device support by billyburly · Pull Request #30639 · home-assistant/core

billyburly · 2020-01-10T06:02:28Z

Description:

Support for ZHA cover devices. Allows control of IKEA Fyrtur blinds via the zha integration

Related issue (if applicable): fixes #

Pull request with documentation for home-assistant.io : home-assistant/home-assistant.io#11695

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

homeassistant · 2020-01-10T06:02:30Z

Hi @billyburly,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

probot-home-assistant · 2020-01-10T06:02:38Z

Hey there @dmulcahey, @Adminiuga, mind taking a look at this pull request as its been labeled with a integration (zha) you are listed as a codeowner for? Thanks!

Adminiuga

Call cluster commands as methods of the channel.

billyburly · 2020-01-10T18:52:28Z

@Adminiuga added a check to see if the commands were successful

Adminiuga · 2020-01-10T19:55:03Z

Hrm, I have no ideas why codecov fails.

Adminiuga

@dmulcahey this looks fine to me. But wouldn't mind a second set of eyes.

dmulcahey · 2020-01-10T23:21:32Z

Awesome, I’ll peek at this in detail in the morning.

Adminiuga · 2020-01-10T23:57:53Z

I think if you cover a couple more lines like in here https://codecov.io/gh/home-assistant/home-assistant/compare/d25aa1f1830f1d3fad42dde3f9bdfae2ad98b7f2...0567474b9473e0c06ee40a3323aae41d54b1d5f1/diff#D4-92 it should pass the coverage test.

Adminiuga · 2020-01-11T00:00:39Z

and check https://github.com/home-assistant/home-assistant/blob/74a198e37b9fc3d189d307cc5396609513d9e0e6/tests/components/zha/test_sensor.py#L86 something similar should cover line 38 of async_setup_entry()

Adminiuga · 2020-01-11T00:21:52Z

install pre-commit . saves some time ;)

dmulcahey · 2020-01-11T15:00:40Z

I think if you cover a couple more lines like in here https://codecov.io/gh/home-assistant/home-assistant/compare/d25aa1f1830f1d3fad42dde3f9bdfae2ad98b7f2...0567474b9473e0c06ee40a3323aae41d54b1d5f1/diff#D4-92 it should pass the coverage test.

@billyburly You need to add a test to cover the uncovered branch in async_get_state lines 170 and 171. That should get you past the coverage check.

dmulcahey · 2020-01-11T15:05:02Z

This looks good to me. Thanks for the contribution! This can be merged after the coverage check passes.

Adminiuga · 2020-01-12T02:24:40Z

@billyburly wanna give billyburly#1 a try?

I still don't fully understand, why with

    with patch(
        "homeassistant.components.zha.core.channels.ZigbeeChannel.get_attribute_value",
        return_value=coro_mock(100)
    ) as get_attr_mock:

The mock gets called two times, but return 100 only the 1st time, so async_get_state() still was getting None for the attribute value.

Update ZHA cover tests coverage.

Adminiuga · 2020-01-12T03:42:27Z

Thank you!

billyburly · 2020-01-12T03:51:09Z

@Adminiuga thanks for the help with the test

MartinHjelmare

Some suggestions for the future.

MartinHjelmare · 2020-01-12T04:02:54Z

+        """Open the window cover."""
+        res = await self._cover_channel.up_open()
+        if isinstance(res, list) and res[1] is Status.SUCCESS:
+            self.async_set_state(STATE_OPENING)


With this info we can implement is_opening cover entity property.

https://github.com/home-assistant/home-assistant/blob/4dc39492a57c505c1915e679f06198204458763c/homeassistant/components/cover/__init__.py#L220-L223

MartinHjelmare · 2020-01-12T04:03:43Z

+        """Close the window cover."""
+        res = await self._cover_channel.down_close()
+        if isinstance(res, list) and res[1] is Status.SUCCESS:
+            self.async_set_state(STATE_CLOSING)


With this info we can implement is_closing cover entity property.

https://github.com/home-assistant/home-assistant/blob/4dc39492a57c505c1915e679f06198204458763c/homeassistant/components/cover/__init__.py#L225-L228

MartinHjelmare · 2020-01-12T04:04:36Z

+        """Stop the window cover."""
+        res = await self._cover_channel.stop()
+        if isinstance(res, list) and res[1] is Status.SUCCESS:
+            self._state = STATE_OPEN if self._current_position > 0 else STATE_CLOSED


Use async_set_state?

MartinHjelmare · 2020-01-12T04:05:46Z

+
+    async def async_set_cover_position(self, **kwargs):
+        """Move the roller shutter to a specific position."""
+        new_pos = kwargs.get(ATTR_POSITION)


Position is required.

new_pos = kwargs[ATTR_POSITION]

ZHA cover device support

d64fc37

billyburly requested review from Adminiuga and dmulcahey as code owners January 10, 2020 06:02

homeassistant added the cla-needed label Jan 10, 2020

probot-home-assistant Bot added has-tests integration: zha new-platform labels Jan 10, 2020

probot-home-assistant Bot assigned Adminiuga and dmulcahey Jan 10, 2020

homeassistant added cla-signed and removed cla-needed labels Jan 10, 2020

billyburly mentioned this pull request Jan 10, 2020

Add cover to ZHA supported devices home-assistant/home-assistant.io#11695

Merged

billyburly added 4 commits January 10, 2020 01:16

flake8

b5301a1

flake8, black

dc4f8ec

isort

a655508

pylint

67428fd

Adminiuga requested changes Jan 10, 2020

View reviewed changes

Comment thread homeassistant/components/zha/core/channels/closures.py Outdated

billyburly added 5 commits January 10, 2020 10:32

more test

9b2ec59

use zigpy provided functions

e3509fe

black

5860e8b

handle command errors, better state handling

e748042

black

0567474

Adminiuga approved these changes Jan 10, 2020

View reviewed changes

Adminiuga self-requested a review January 10, 2020 19:55

Adminiuga approved these changes Jan 10, 2020

View reviewed changes

more test

374148e

lint

81cd67a

dmulcahey approved these changes Jan 11, 2020

View reviewed changes

Update ZHA cover tests coverage.

13fc42f

Merge pull request #1 from Adminiuga/tests/cover-tests

9663f9b

Update ZHA cover tests coverage.

Adminiuga merged commit 4d64172 into home-assistant:dev Jan 12, 2020

MartinHjelmare reviewed Jan 12, 2020

View reviewed changes

lock Bot locked and limited conversation to collaborators Jan 13, 2020

Uh oh!

Conversation

billyburly commented Jan 10, 2020

Description:

Checklist:

Uh oh!

homeassistant commented Jan 10, 2020

Uh oh!

probot-home-assistant Bot commented Jan 10, 2020

Uh oh!

Adminiuga left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

billyburly commented Jan 10, 2020

Uh oh!

Adminiuga commented Jan 10, 2020

Uh oh!

Adminiuga left a comment

Choose a reason for hiding this comment

Uh oh!

dmulcahey commented Jan 10, 2020

Uh oh!

Adminiuga commented Jan 10, 2020

Uh oh!

Adminiuga commented Jan 11, 2020

Uh oh!

Adminiuga commented Jan 11, 2020

Uh oh!

dmulcahey commented Jan 11, 2020

Uh oh!

dmulcahey commented Jan 11, 2020

Uh oh!

Adminiuga commented Jan 12, 2020

Uh oh!

Adminiuga commented Jan 12, 2020

Uh oh!

billyburly commented Jan 12, 2020

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare Jan 12, 2020

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare Jan 12, 2020

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare Jan 12, 2020

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare Jan 12, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants