Skip to content
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

Add basic support for IKEA Fyrtur blinds #26659

Merged
merged 10 commits into from Sep 22, 2019

Conversation

@ggravlingen
Copy link
Contributor

commented Sep 15, 2019

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:

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.
@project-bot project-bot bot added this to Needs review in Dev Sep 15, 2019
@MartinHjelmare MartinHjelmare moved this from Needs review to Incoming in Dev Sep 15, 2019

_LOGGER = logging.getLogger(__name__)

IKEA = "IKEA of Sweden"

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Sep 15, 2019

Member

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

This comment has been minimized.

Copy link
@ggravlingen

ggravlingen Sep 15, 2019

Author Contributor

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


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

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Sep 15, 2019

Member

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

This comment has been minimized.

Copy link
@ggravlingen

ggravlingen Sep 15, 2019

Author Contributor

Thanks, removed it.

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

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Sep 15, 2019

Member

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

This comment has been minimized.

Copy link
@ggravlingen

ggravlingen Sep 15, 2019

Author Contributor

Fixed and changed in all files.

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2019

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

This comment has been minimized.

Copy link
Member

commented Sep 15, 2019

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

black --fast homeassistant
@ggravlingen

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2019

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

This comment has been minimized.

Copy link

commented Sep 16, 2019

How can I test this?

@ggravlingen

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

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

@robbinonline

This comment has been minimized.

Copy link

commented Sep 16, 2019

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

@ggravlingen

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

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

This comment has been minimized.

Copy link

commented Sep 16, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

No, only cover.py.

@ggravlingen

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2019

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

@robbinonline

This comment has been minimized.

Copy link

commented Sep 17, 2019

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

@ggravlingen

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2019

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

@robbinonline

This comment has been minimized.

Copy link

commented Sep 17, 2019

Really appreciate it, looking forward to the updated code!

@ggravlingen

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2019

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

@robbinonline

This comment has been minimized.

Copy link

commented Sep 17, 2019

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

Schermafbeelding 2019-09-17 om 11 41 51

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2019

@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

This comment has been minimized.

Copy link

commented Sep 17, 2019

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

Schermafbeelding 2019-09-17 om 15 51 01

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.

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

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Sep 17, 2019

Member

Add a return False here.

This comment has been minimized.

Copy link
@ggravlingen

ggravlingen Sep 17, 2019

Author Contributor

Done

@ggravlingen

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2019

@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?

Copy link
Member

left a comment

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 Outdated Show resolved Hide resolved
@ggravlingen

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2019

@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

This comment has been minimized.

Copy link
Member

commented Sep 19, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2019

@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 ggravlingen referenced this pull request Sep 19, 2019
0 of 9 tasks complete
@MartinHjelmare MartinHjelmare moved this from Reviewer approved to Incoming in Dev Sep 19, 2019
ggravlingen added 5 commits Sep 15, 2019
@ggravlingen

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2019

Argh, I rebased and messed up 🤦‍♂

@ggravlingen

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2019

I'll fix this later tonight

ggravlingen added 4 commits Sep 20, 2019
@ggravlingen

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2019

@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:

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

@ggravlingen ggravlingen changed the title WIP: Add basic support for IKEA Fyrtur blinds Add basic support for IKEA Fyrtur blinds Sep 22, 2019
def __init__(self, cover, api, gateway_id):
"""Initialize a cover."""
self._api = api
self._unique_id = f"cover-{gateway_id}-{cover.id}"

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Sep 22, 2019

Member

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

This comment has been minimized.

Copy link
@ggravlingen

ggravlingen Sep 22, 2019

Author Contributor

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?

image

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Sep 22, 2019

Member

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.

This comment has been minimized.

Copy link
@ggravlingen

ggravlingen Sep 22, 2019

Author Contributor

That makes sense, thanks for the explanation

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

This comment has been minimized.

Copy link
Member

commented Sep 22, 2019

Can be merged when build passes.

@ggravlingen

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2019

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

@MartinHjelmare MartinHjelmare merged commit 49fef9a into home-assistant:dev Sep 22, 2019
11 checks passed
11 checks passed
CI Build #20190922.66 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch Coverage not affected.
Details
codecov/project 94.07% (target 90%)
Details
Dev automation moved this from Reviewer approved to Done Sep 22, 2019
@lock lock bot locked and limited conversation to collaborators Sep 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
6 participants
You can’t perform that action at this time.