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
Conversation
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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@guillaume1410 what "KO" means ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
"""Initialize the cover.""" | ||
self.hass = hass | ||
self.ryobi_door = ryobi_door | ||
self._name = 'ryobi_gdo' + ryobi_door.get_device_id() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use string formatting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
"""Close the cover.""" | ||
_LOGGER.info("Closing garage door") | ||
self.ryobi_door.close_device() | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to return.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
"""Open the cover.""" | ||
_LOGGER.info("Opening garage door") | ||
self.ryobi_door.open_device() | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dito
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
for device_id in devices: | ||
my_door = ryobi_door(username, password, device_id) | ||
if my_door.get_api_key() is True: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
device_to_add = True | ||
else: | ||
_LOGGER.error("%s not in your devices. Check conf", device_id) | ||
if device_to_add is True: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want an immediate update then add True
as second parameter to add_devices()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
def open_cover(self, **kwargs): | ||
"""Open the cover.""" | ||
_LOGGER.info("Opening garage door") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too verbose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
def close_cover(self, **kwargs): | ||
"""Close the cover.""" | ||
_LOGGER.info("Closing garage door") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too verbose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to make sure that's a list. use something like vol.All(cv.ensure_list, [cv.string])
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
For more details about this platform, please refer to the documentation | ||
https://home-assistant.io/components/cover.ryobi_gdo/ | ||
""" | ||
from datetime import timedelta |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'datetime.timedelta' imported but unused
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hass
isn't used.
@property | ||
def is_closed(self): | ||
"""Return if the cover is closed.""" | ||
if self._door_state == STATE_UNKNOWN: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
* Initial component for Ryobi cover * Initial component for Ryobi cover * Adding Ryobi cover * Adding Ryobi cover * Adding Ryobi cover * Minor changes * Remove import
Hi fabaff. |
Still works for me, although occasionally a restart will cause the initial entity add to fail, but another restart fixes it. |
Is this project dead? I attempted to install this cover as indicated in the documentation and got
|
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):Checklist:
tox
. Your PR cannot be merged unless tests passIf user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
REQUIREMENTS
variable (example).requirements_all.txt
by runningscript/gen_requirements_all.py
..coveragerc
.If the code does not interact with devices: