Add basic support for IKEA Fyrtur blinds #26659

ggravlingen · 2019-09-15T09:59:06Z

Description:

This PR adds basic support for the IKEA Fyrtur window blinds.

Caveat: I don't have one myself, so I need to find someone to test the code. I suggest this to be done before reviewing the code.

Related issue (if applicable): fixes #

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

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

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.

MartinHjelmare · 2019-09-15T10:09:00Z

homeassistant/components/tradfri/cover.py

+
+_LOGGER = logging.getLogger(__name__)
+
+IKEA = "IKEA of Sweden"


We use the IKEA constant in many platforms. We can move it to const.py in the package.

Hmm, looking more closely at it, I'm not sure it's being used at all. I'll remove it from all files.

MartinHjelmare · 2019-09-15T10:13:06Z

homeassistant/components/tradfri/cover.py

+
+    def set_cover_position(self, **kwargs):
+        """Move the cover to a specific position."""
+        if ATTR_POSITION in kwargs:


We don't need to check for this. It's validated by the service schema already.

Thanks, removed it.

MartinHjelmare · 2019-09-15T10:13:47Z

homeassistant/components/tradfri/cover.py

+    @callback
+    def _async_start_observe(self, exc=None):
+        """Start observation of cover."""
+        from pytradfri.error import PytradfriError


Please move 3rd party imports to the top of the module.

Fixed and changed in all files.

ggravlingen · 2019-09-15T10:37:18Z

I've posted in the HA forums where adding support has been requested for and asked for someone to do a test against a real blind.

MartinHjelmare · 2019-09-15T11:32:56Z

Make sure to run black from the project root.
https://developers.home-assistant.io/blog/2019/07/31/black.html

black --fast homeassistant

ggravlingen · 2019-09-15T17:49:46Z

Thanks, I have a sneaking feeling there was something I'd forgotten in the process! Mind if I add a link to that blog post to the development checklist?

robbinonline · 2019-09-16T13:52:24Z

How can I test this?

ggravlingen · 2019-09-16T18:44:57Z

@robbinonline: clone my branch, run the docker script and start HASS. Pair the gateway as usual and the blind should be there.

robbinonline · 2019-09-16T19:20:08Z

Hmm that sounds like a big project. Not possible to install it like a custom component or update the changed files?

ggravlingen · 2019-09-16T19:30:16Z

I’m afraid I don’t know how that works. You need to update pytradfri to 6.3.0 and add a file to the tradfri folder but if that is possible as a custom component is beyond me.

The dockerfile ia dockerfile.dev (in my repo, not the official one) if you feel adventurous.

robbinonline · 2019-09-16T20:11:54Z

I have updated pytradfri to 6.3.0 and change .coveragerc and add cover.py. Do I also need the other files for the blinds?

ggravlingen · 2019-09-16T20:28:01Z

No, only cover.py.

ggravlingen · 2019-09-17T03:11:49Z

@robbinonline woke up this morning realizing I’ve forgotten to change one file. 🤦‍♂️ Will fix and post here when done.

robbinonline · 2019-09-17T06:21:51Z

Ah thats hopefully the reason its not working here, try it for hours yesterday 😅

ggravlingen · 2019-09-17T06:29:46Z

Really sorry about that 🙈 Its been a while since I last built something för HA.

robbinonline · 2019-09-17T06:37:07Z

Really appreciate it, looking forward to the updated code!

ggravlingen · 2019-09-17T07:53:21Z

@robbinonline the forgotten change has been added to the code now. You also need to update the __init__.py file.

robbinonline · 2019-09-17T09:44:59Z

@ggravlingen Allright update the code, and now the blinds shows up in the Integrations, only with the state: entity not available

Checking the logs:

Error doing job: Task exception was never retrieved
11:35 components/cover/__init__.py (ERROR) - message first occured at 11:35 and shows up 2 times

Error doing job: Task exception was never retrieved
Traceback (most recent call last):
  File "/usr/src/homeassistant/homeassistant/helpers/entity_platform.py", line 408, in _async_add_entity
    await entity.async_update_ha_state()
  File "/usr/src/homeassistant/homeassistant/helpers/entity.py", line 266, in async_update_ha_state
    self._async_write_ha_state()
  File "/usr/src/homeassistant/homeassistant/helpers/entity.py", line 300, in _async_write_ha_state
    state = self.state
  File "/usr/src/homeassistant/homeassistant/components/cover/__init__.py", line 211, in state
    closed = self.is_closed
  File "/usr/src/homeassistant/homeassistant/components/cover/__init__.py", line 264, in is_closed
    raise NotImplementedError()
NotImplementedError

Did I miss something?

ggravlingen · 2019-09-17T12:40:12Z

@robbinonline you did nothing wrong at all and this is the reason we're testing 😄 It appears I needed to add a is_closed method. I've done so now but it might still be the case that it's not working and I need to add more methods that are required for the blinds to show up.

robbinonline · 2019-09-17T13:51:27Z

Allright, update the cover.py, now the blinds appear :)

But the controls don't work it only reads the state, and only updated the state by example how far are the blinds open after a restart.

MartinHjelmare · 2019-09-17T14:18:47Z

homeassistant/components/tradfri/cover.py

+        """Return if the cover is closed or not."""
+        if self.current_cover_position == 0:
+            return True
+


Add a return False here.

ggravlingen · 2019-09-17T14:45:32Z

@robbinonline that's a bit of progress then that we can track current state :) No errors in the log?

I've also adressed the comment of @MartinHjelmare but don't know if that will do anything to make changing the state work.

@MartinHjelmare sorry about spamming this PR. I can ping you when it's working if you want to turn off notifications for this PR?

MartinHjelmare

Don't worry about notifying me. I'm happy to see the progress. It's also easier to review small chunks at a time. 👍

homeassistant/components/tradfri/cover.py

MartinHjelmare · 2019-09-18T19:38:52Z

Has someone tested the observation method in pytradfri with the blinds?

ggravlingen · 2019-09-18T19:41:03Z

@MartinHjelmare good point, I'll go back to pytradfri to find someone to test. This would have been so much easier if I'd had a blind. :)

ggravlingen · 2019-09-19T06:31:17Z

@MartinHjelmare I made some changes to pytradfri to see if we can fix the error that's starting to pop up with the new gateway firmware (#26673). Hence the version bump to 6.3.1.

MartinHjelmare · 2019-09-19T06:42:45Z

It would be better to put the library bump in a separate PR since we then could include it in a patch release since it's potentially a fix for a problem.

ggravlingen · 2019-09-19T07:19:43Z

@MartinHjelmare I agree and I will do so as I reckon it will take a bit longer to sort out observation.

There are a few people who are up and running testing this branch, including a few who experience the bug, so I figured it'd be an easy way of testing to just bump the version here.

ggravlingen · 2019-09-20T05:57:04Z

Argh, I rebased and messed up 🤦‍♂

ggravlingen · 2019-09-20T06:05:28Z

I'll fix this later tonight

ggravlingen · 2019-09-22T11:59:51Z

@MartinHjelmare when you feel the code is in line with HA standards, this one is good to go. I got confirmation on the forums that it works as expected:

Source: https://community.home-assistant.io/t/please-add-support-for-blinds-activated-in-the-ikea-integration/137253/38

MartinHjelmare · 2019-09-22T16:43:54Z

homeassistant/components/tradfri/cover.py

+    def __init__(self, cover, api, gateway_id):
+        """Initialize a cover."""
+        self._api = api
+        self._unique_id = f"cover-{gateway_id}-{cover.id}"


The entity registry is aware of the integration domain and platform domain of the entity, so adding cover isn't necessary.

I'll remove it but just a double check as I saw this in light and also noticed who made the change, so I figured it wasn't put there randomly?

The light platform has two kinds of entities, light and group. If there are more than one type of entity in a platform and the rest of the id is the same between the two kinds, we need to add a differentiator.

I don't know if this is actually the case for the light platform, ie if group.id and light.id are the same.

That makes sense, thanks for the explanation

MartinHjelmare · 2019-09-22T20:09:00Z

Can be merged when build passes.

ggravlingen · 2019-09-22T20:26:18Z

Thank you for the advice during review and to all the testers 🎉🙏

homeassistant added the cla-signed label Sep 15, 2019

probot-home-assistant bot added integration: tradfri new-platform labels Sep 15, 2019

project-bot bot added this to Needs review in Dev Sep 15, 2019

MartinHjelmare moved this from Needs review to Incoming in Dev Sep 15, 2019

MartinHjelmare requested changes Sep 15, 2019

View reviewed changes

Dev automation moved this from Incoming to Review in progress Sep 15, 2019

This was referenced Sep 15, 2019

Update tradfri.markdown home-assistant/home-assistant.io#10373

Merged

Ikea smart blinds home-assistant-libs/pytradfri#242

Closed

MartinHjelmare reviewed Sep 17, 2019

View reviewed changes

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

MartinHjelmare reviewed Sep 17, 2019

View reviewed changes

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

ggravlingen mentioned this pull request Sep 19, 2019

Bump TRADFRI #26731

Merged

9 tasks

MartinHjelmare moved this from Reviewer approved to Incoming in Dev Sep 19, 2019

ggravlingen added 5 commits September 20, 2019 07:52

Add basic support for IKEA Fyrtur blinds

d999df6

Update coveragerc

145ac1f

Fix typo

432c4d1

Fix typos

bade109

Update following review

1e63786

ggravlingen added 4 commits September 20, 2019 19:01

Fix incorrect rebase

61bc890

Fix error

66fa529

Update to new format of unique id

32bb104

Add cover

a05d661

ggravlingen changed the title ~~WIP: Add basic support for IKEA Fyrtur blinds~~ Add basic support for IKEA Fyrtur blinds Sep 22, 2019

MartinHjelmare reviewed Sep 22, 2019

View reviewed changes

MartinHjelmare moved this from Incoming to Review in progress in Dev Sep 22, 2019

Remove reference to cover in unique id

ae4bbe4

MartinHjelmare approved these changes Sep 22, 2019

View reviewed changes

Dev automation moved this from Review in progress to Reviewer approved Sep 22, 2019

MartinHjelmare merged commit 49fef9a into home-assistant:dev Sep 22, 2019

Dev automation moved this from Reviewer approved to Done Sep 22, 2019

lock bot locked and limited conversation to collaborators Sep 23, 2019


		_LOGGER = logging.getLogger(__name__)

		IKEA = "IKEA of Sweden"

Add basic support for IKEA Fyrtur blinds #26659

Add basic support for IKEA Fyrtur blinds #26659

Conversation

ggravlingen commented Sep 15, 2019 • edited

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggravlingen commented Sep 15, 2019

MartinHjelmare commented Sep 15, 2019

ggravlingen commented Sep 15, 2019

robbinonline commented Sep 16, 2019

ggravlingen commented Sep 16, 2019

robbinonline commented Sep 16, 2019

ggravlingen commented Sep 16, 2019

robbinonline commented Sep 16, 2019

ggravlingen commented Sep 16, 2019

ggravlingen commented Sep 17, 2019

robbinonline commented Sep 17, 2019

ggravlingen commented Sep 17, 2019

robbinonline commented Sep 17, 2019

ggravlingen commented Sep 17, 2019

robbinonline commented Sep 17, 2019 • edited by MartinHjelmare

ggravlingen commented Sep 17, 2019

robbinonline commented Sep 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggravlingen commented Sep 17, 2019 • edited

MartinHjelmare left a comment

Choose a reason for hiding this comment

MartinHjelmare commented Sep 18, 2019

ggravlingen commented Sep 18, 2019

ggravlingen commented Sep 19, 2019

MartinHjelmare commented Sep 19, 2019

ggravlingen commented Sep 19, 2019

ggravlingen commented Sep 20, 2019

ggravlingen commented Sep 20, 2019

ggravlingen commented Sep 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinHjelmare Sep 22, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinHjelmare commented Sep 22, 2019

ggravlingen commented Sep 22, 2019

ggravlingen commented Sep 15, 2019 •

edited

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

robbinonline commented Sep 17, 2019 •

edited by MartinHjelmare

ggravlingen commented Sep 17, 2019 •

edited

MartinHjelmare Sep 22, 2019 •

edited