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

OpenGarage support #7338

Merged
merged 7 commits into from May 2, 2017

Conversation

Projects
None yet
7 participants
@swbradshaw
Copy link
Contributor

swbradshaw commented Apr 27, 2017

Description:

This adds a cover platform for the Open Source WiFi garage opener, OpenGarage. This is similar to the Garadget platform, but has a few differences:

  • Cost. These are ESP8266 based devices that a DIY person can build and flash for under $10. For the non-tech user, they can be purchased for $50 USD.
  • Local. This operates on your local network, and does not connect to a cloud server.
  • Distance Sensor. Open/Close status is determined by a distance sensor. In addition to door status, this also allows you to create a sensor in HA that can tell if your vehicle is in the garage or not. (See documentation)

I started with the garadget.py code and created this platform based on that.

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

Example entry for configuration.yaml (if applicable):

    cover:
     platform: opengarage
     covers:
         garage:
           host:  192.168.1.12
           device_key: opendoor
           name:  Left Garage Door

Checklist:

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

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

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • 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:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.
OpenGarage.io support
Cleaned up component and ran lint checking
@mention-bot

This comment has been minimized.

Copy link

mention-bot commented Apr 27, 2017

@swbradshaw, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @fabaff and @happyleavesaoc to be potential reviewers.

def stop_cover(self):
"""Stop the door where it is."""
self._state_before_move = None
self._state = STATE_STOPPED

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Apr 27, 2017

trailing whitespace

def open_cover(self):
"""Open the cover."""
if self._state not in [STATE_OPEN, STATE_OPENING]:
self._state_before_move = self._state #close

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Apr 27, 2017

at least two spaces before inline comment
inline comment should start with '# '

def close_cover(self):
"""Close the cover."""
if self._state not in [STATE_CLOSED, STATE_CLOSING]:
self._state_before_move = self._state #open

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Apr 27, 2017

at least two spaces before inline comment
inline comment should start with '# '

return self._state in [STATE_CLOSED, STATE_OPENING]


def _start_watcher(self, command):

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Apr 27, 2017

too many blank lines (2)

self._name = args[CONF_NAME]
self.device_id = args['device_id']
self._devicekey = args[CONF_DEVICEKEY]
self._state = STATE_UNKNOWN

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Apr 27, 2017

trailing whitespace

@swbradshaw swbradshaw changed the title WIP: New cover platform: OpenGarage.io support New cover platform: OpenGarage.io support Apr 27, 2017

vol.Required(CONF_HOST): cv.string,
vol.Optional(CONF_PORT, default=DEFAULT_PORT): cv.port,
vol.Required(CONF_DEVICEKEY): cv.string,
vol.Optional(CONF_NAME): cv.string

This comment has been minimized.

Copy link
@fabaff

fabaff Apr 28, 2017

Member

vol.Required should go first then vol.Optional, preferably alphabetically ordered. Make it easier to keep the documentation in sync.

Platform for the opengarage.io cover component.
For more details about this platform, please refer to the documentation
https://home-assistant.io/components/cover.opengarage

This comment has been minimized.

Copy link
@fabaff

fabaff Apr 28, 2017

Member

/ is missing at the end.

def setup_platform(hass, config, add_devices, discovery_info=None):
"""Setup OpenGarage covers."""
covers = []
devices = config.get(CONF_COVERS, {})

This comment has been minimized.

Copy link
@fabaff

fabaff Apr 28, 2017

Member

No default needed here as CONF_COVERS is required and can't be empty.

covers = []
devices = config.get(CONF_COVERS, {})

_LOGGER.debug(devices)

This comment has been minimized.

Copy link
@fabaff

fabaff Apr 28, 2017

Member

Please remove that.

class OpenGarageCover(CoverDevice):
"""Representation of a OpenGarage cover."""

# pylint: disable=no-self-use, too-many-instance-attributes

This comment has been minimized.

Copy link
@fabaff

fabaff Apr 28, 2017

Member

too-many-instance-attributes is globally disabled already.

url = '{}/jc'.format(
self.opengarage_url
)
ret = requests.get(url)

This comment has been minimized.

Copy link
@fabaff

fabaff Apr 28, 2017

Member

Please add a timeout to all requests.

"""Get latest status."""
url = '{}/jc'.format(
self.opengarage_url
)

This comment has been minimized.

Copy link
@fabaff

fabaff Apr 28, 2017

Member

I guess that url fits on one line.

@fabaff fabaff changed the title New cover platform: OpenGarage.io support OpenGarage.io support Apr 28, 2017

@swbradshaw

This comment has been minimized.

Copy link
Contributor Author

swbradshaw commented Apr 28, 2017

@fabaff I've made the changes you requested. Let me know if you see anything else. Thanks!

@swbradshaw swbradshaw changed the title OpenGarage.io support OpenGarage support Apr 29, 2017

"""Start watcher."""
_LOGGER.debug("Starting Watcher for command: %s ", command)
if self._unsub_listener_cover is None:
self._unsub_listener_cover = track_utc_time_change(

This comment has been minimized.

Copy link
@balloob

balloob Apr 30, 2017

Member

So I am kinda confused here.

  • You register a listener that fires every second. Since you don't know where you are in the second, it means that you schedule an update to be fired between 0.001 and 0.999 seconds. That seems kinda arbitrary.
  • On first fire, you call schedule_update_ha_state, the listener gets removed in the update

Home Assistant will already call the update method after it calls a service because should_poll is returning True.

So why is this "start_watcher" necessary?

This comment has been minimized.

Copy link
@balloob

balloob Apr 30, 2017

Member

It seems like you want to keep calling update while the state is in opening/closing. Why not have this logic inside the update method ? When update is done, if certain state, schedule another sync.

Also use track_point_in_time so that you don't have to unsub anything.

@swbradshaw

This comment has been minimized.

Copy link
Contributor Author

swbradshaw commented Apr 30, 2017

@balloob The watcher did what you described - updated the status every second when the status is opening/closing. That said, I went ahead and removed it. It doesn't provide much value, and in the case that the door were to get stuck while opening/closing, the watcher would keep running, which isn't ideal. Also, its worth mentioning that I didn't write this code - it's verbatim from the Garadget cover.

self._available = True

# Lets try to get the configured name if not provided.
try:

This comment has been minimized.

Copy link
@balloob

balloob Apr 30, 2017

Member

This seems a duplication of the update method. Just call add_devices(covers, True) in your setup method and we'll call update right there. You can then go ahead in the update method to load the name if it is None.

self.signal = status.get('rssi')
self.dist = status.get('dist')
self._available = True
except (requests.exceptions.ConnectionError,

This comment has been minimized.

Copy link
@balloob

balloob Apr 30, 2017

Member

You can just catch RequestException which is the parent of all Requests exceptions.

dict(reason=ex))
self._state = STATE_OFFLINE
except KeyError as ex:
_LOGGER.warning('OpenGarage device %(device)s seems to be offline',

This comment has been minimized.

Copy link
@balloob

balloob Apr 30, 2017

Member

A KeyError doesn't mean it's offline, it means that you requested a key on a dictionary that didn't hold that key. However, this can never happen because you use .get() which will never raise this error. You can remove this block.

self._state_before_move = self._state
self._state = STATE_CLOSING
ret = self._push_button()
return ret.get('result') == 1

This comment has been minimized.

Copy link
@balloob

balloob Apr 30, 2017

Member

there is no need to return anything

self._state_before_move = self._state
self._state = STATE_OPENING
ret = self._push_button()
return ret.get('result') == 1

This comment has been minimized.

Copy link
@balloob

balloob Apr 30, 2017

Member

No need to return anything

self._state_before_move = None
self._state = STATE_STOPPED
ret = self._push_button()
return ret.get('result') == 1

This comment has been minimized.

Copy link
@balloob

balloob Apr 30, 2017

Member

No need to return anything


@property
def should_poll(self):
"""No polling needed for a demo cover."""

This comment has been minimized.

Copy link
@balloob

balloob Apr 30, 2017

Member

Stale comment. The default for polling is already True so you can just remove this whole property.

@balloob

balloob approved these changes May 2, 2017

All comments addressed

@balloob balloob merged commit f4f06af into home-assistant:dev May 2, 2017

4 checks passed

cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.07%) to 93.458%
Details
hound No violations found. Woof!
@balloob

This comment has been minimized.

Copy link
Member

balloob commented May 2, 2017

Thanks! 🐬

@balloob balloob referenced this pull request May 5, 2017

Merged

0.44 #7444

@home-assistant home-assistant locked and limited conversation to collaborators Aug 12, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.