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

Adding ryobi garage door opener #14618

Merged
merged 7 commits into from May 26, 2018

Conversation

Projects
None yet
7 participants
@guillaume1410
Contributor

guillaume1410 commented May 24, 2018

Description:

Adding support for ryobi garage door opener

Related issue (if applicable): fixes #

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

Example entry for configuration.yaml (if applicable):

cover:
  - platform: ryobi_gdo
    username: user@domain.com
    password: 1kdkdfjk233
    device_id:
      - 12345

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

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

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

  • [ X] New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

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

This comment has been minimized.

Member

fabaff commented May 25, 2018

There is no need to open a new PR for the changes. The changes can be pushed to the existing branch. This makes it hard to keep track of the review.

Success of #14134.

_LOGGER.debug("Credentials OK, api key retrieved")
auth_ok = True
else:
_LOGGER.error("Credentials KO, no api key retrieved")

This comment has been minimized.

@alkuzad

This comment has been minimized.

@guillaume1410

guillaume1410 May 26, 2018

Contributor

haha. KO reverse of OK. Possibly a bad professional habit... anyway changed the code and message

def __init__(self, hass, ryobi_door):
"""Initialize the cover."""
self.hass = hass

This comment has been minimized.

@fabaff

fabaff May 25, 2018

Member

Not used.

This comment has been minimized.

@guillaume1410

guillaume1410 May 25, 2018

Contributor

done

"""Initialize the cover."""
self.hass = hass
self.ryobi_door = ryobi_door
self._name = 'ryobi_gdo' + ryobi_door.get_device_id()

This comment has been minimized.

@fabaff

fabaff May 25, 2018

Member

Use string formatting.

This comment has been minimized.

@guillaume1410

guillaume1410 May 25, 2018

Contributor

done

"""Close the cover."""
_LOGGER.info("Closing garage door")
self.ryobi_door.close_device()
return

This comment has been minimized.

@fabaff

fabaff May 25, 2018

Member

No need to return.

This comment has been minimized.

@guillaume1410

guillaume1410 May 25, 2018

Contributor

done

"""Open the cover."""
_LOGGER.info("Opening garage door")
self.ryobi_door.open_device()
return

This comment has been minimized.

@fabaff

This comment has been minimized.

@guillaume1410

guillaume1410 May 25, 2018

Contributor

done

for device_id in devices:
my_door = ryobi_door(username, password, device_id)
if my_door.get_api_key() is True:

This comment has been minimized.

@fabaff

fabaff May 25, 2018

Member

Check if it's False and return. This way you can eliminate auth_ok as there is no need to proceed if the credentials are wrong.

Guard clauses make the code more readable because there is less indent and nesting.

This comment has been minimized.

@guillaume1410

guillaume1410 May 25, 2018

Contributor

done

device_to_add = True
else:
_LOGGER.error("%s not in your devices. Check conf", device_id)
if device_to_add is True:

This comment has been minimized.

@fabaff

fabaff May 25, 2018

Member

Remove device_to_add and check covers instead. Or can be removed if the guard clauses ensure that this point only can be reached if it's all good.

This comment has been minimized.

@guillaume1410

guillaume1410 May 25, 2018

Contributor

done

self.ryobi_door = ryobi_door
self._name = 'ryobi_gdo' + ryobi_door.get_device_id()
self.ryobi_door.update()
self._door_state = ryobi_door.get_door_status()

This comment has been minimized.

@fabaff

fabaff May 25, 2018

Member

If you want an immediate update then add True as second parameter to add_devices().

This comment has been minimized.

@guillaume1410

guillaume1410 May 25, 2018

Contributor

done

def open_cover(self, **kwargs):
"""Open the cover."""
_LOGGER.info("Opening garage door")

This comment has been minimized.

@fabaff

fabaff May 25, 2018

Member

Too verbose.

This comment has been minimized.

@guillaume1410

guillaume1410 May 25, 2018

Contributor

done

def close_cover(self, **kwargs):
"""Close the cover."""
_LOGGER.info("Closing garage door")

This comment has been minimized.

@fabaff

fabaff May 25, 2018

Member

Too verbose.

This comment has been minimized.

@guillaume1410

guillaume1410 May 25, 2018

Contributor

done

PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
vol.Required(CONF_USERNAME): cv.string,
vol.Required(CONF_PASSWORD): cv.string,
vol.Required(CONF_DEVICE_ID): [cv.string],

This comment has been minimized.

@fabaff

fabaff May 25, 2018

Member

You need to make sure that's a list. use something like vol.All(cv.ensure_list, [cv.string]).

This comment has been minimized.

@guillaume1410

guillaume1410 May 25, 2018

Contributor

done

guillaume1410 added some commits May 26, 2018

@guillaume1410 guillaume1410 referenced this pull request May 26, 2018

Merged

Initial documentation for Ryobi cover #5424

2 of 2 tasks complete
For more details about this platform, please refer to the documentation
https://home-assistant.io/components/cover.ryobi_gdo/
"""
from datetime import timedelta

This comment has been minimized.

@houndci-bot

houndci-bot May 26, 2018

'datetime.timedelta' imported but unused

@fabaff

fabaff approved these changes May 26, 2018

@fabaff fabaff merged commit c425afe into home-assistant:dev May 26, 2018

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 94.018%
Details
@MartinHjelmare

Where is the source for the package py_ryobi_gdo hosted? Is it not on github?

class RyobiCover(CoverDevice):
"""Representation of a ryobi cover."""
def __init__(self, hass, ryobi_door):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 26, 2018

Member

hass isn't used.

@property
def is_closed(self):
"""Return if the cover is closed."""
if self._door_state == STATE_UNKNOWN:

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 26, 2018

Member

Remove this. It's not adding anything and unknown state is handled by the base entity class.

for device_id in devices:
my_door = ryobi_door(username, password, device_id)
_LOGGER.debug("Getting the API key")

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 26, 2018

Member

There's too much of debug logging in the module, without giving any specific info in the log messages. I'd remove everything but the one about adding devices.

iMicknl added a commit to iMicknl/home-assistant that referenced this pull request May 31, 2018

Adding ryobi garage door opener (home-assistant#14618)
* Initial component for Ryobi cover

* Initial component for Ryobi cover

* Adding Ryobi cover

* Adding Ryobi cover

* Adding Ryobi cover

* Minor changes

* Remove import

@balloob balloob referenced this pull request Jun 8, 2018

Merged

0.71.0 #14876

@mcnutter1

This comment has been minimized.

mcnutter1 commented Jul 16, 2018

Can we add the states of opening and closing as well, I built a custom Ryobi GDO component and can share the states if needed.

There is also an attribute for a sensor of the battery level of each GDO as well which is a nice to have to monitor the battery level.

cyberjacob pushed a commit to cyberjacob/home-assistant that referenced this pull request Sep 4, 2018

Adding ryobi garage door opener (home-assistant#14618)
* Initial component for Ryobi cover

* Initial component for Ryobi cover

* Adding Ryobi cover

* Adding Ryobi cover

* Adding Ryobi cover

* Minor changes

* Remove import
@guillaume1410

This comment has been minimized.

Contributor

guillaume1410 commented Oct 20, 2018

Hi fabaff.
This code doesn't work anymore and I'm not able to make it work again. Seems like something has change on the ryobi side.
I would like the component to be removed but I don't know how to do.

@mcnutter1

This comment has been minimized.

mcnutter1 commented Oct 20, 2018

Still works for me, although occasionally a restart will cause the initial entity add to fail, but another restart fixes it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment