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

Fix unavailable property for wemo switch #13106

Merged
merged 3 commits into from Mar 12, 2018

Conversation

Projects
None yet
4 participants
@balloob
Copy link
Member

commented Mar 12, 2018

Description:

This will mark Wemo switches as unavailable if they haven't been able to update.

Related issue (if applicable): #12935

Example entry for configuration.yaml (if applicable):

wemo:

Checklist:

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

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2018

@arsaboo I've tested this PR locally and it works. Could you also test this?

Testing steps:

  • Start HASS and have it find your Wemo switch
  • Disconnect Wemo switch
  • HASS should mark Wemo switch as unavailable
  • Plug Wemo switch back in
  • HASS should make Wemo switch available again
@MartinHjelmare
Copy link
Member

left a comment

Adding polling also adds calls to async_update after every service call. The lock as currently placed will not protect from unnecessary calls to _update.

Side note, this module would benefit from a clean up according to our current patterns and practices. Eg where/when callbacks should be registered.

@@ -199,9 +201,33 @@ def turn_off(self, **kwargs):
self.wemo.off()
self.schedule_update_ha_state()

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Mar 12, 2018

Member

This should probably be removed, since we have callbacks for state change called from pywemo.

Wemo switch is unreachable. If update goes through, it will be made
available again.
"""
if self._update_lock.locked():

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Mar 12, 2018

Member

This doesn't protect _update from being called excessively both from callback and poll.

@arsaboo

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2018

Tested this and it works as expected 👍

After taking the Wemo Insight out, it shows unavailable in about 5-7 seconds. After I plug it back in, the switch becomes available as soon as the network is established (typically about 30 seconds or so).

@@ -65,17 +65,28 @@ def __init__(self, device):

wemo = get_component('wemo')
wemo.SUBSCRIPTION_REGISTRY.register(self.wemo)
wemo.SUBSCRIPTION_REGISTRY.on(self.wemo, None, self._update_callback)
wemo.SUBSCRIPTION_REGISTRY.on(

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Mar 12, 2018

Member

Move this to async_added_to_hass.

updated = self.wemo.subscription_update(_type, _params)
self._update(force_update=(not updated))

if not hasattr(self, 'hass'):

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Mar 12, 2018

Member

This can be removed after the move of callback registration to async_added_to_hass.

@balloob

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2018

Addressed the first round of comments by @MartinHjelmare . (Not the ones that arrived while I was typing this)

I agree that it needs a big cleanup. This is one of the first integrations that was added to Home Assistant. The fact that a call to update() can block up to 5 minutes is something I added to pywemo back in the day… I wouldn't do that today anymore. However, I want to fix the available thing right now, maybe one day an async rewrite of pywemo is in order.

@balloob

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2018

Addressed 2nd round of comments 👍

I did not get rid of the global for wemo because I don't have devices to test the light/binary sensor platforms. (and it's out of scope for this PR)

@MartinHjelmare
Copy link
Member

left a comment

Looks good!

@balloob balloob added this to the 0.64.4 milestone Mar 12, 2018

@balloob balloob merged commit 8d8b07a into dev Mar 12, 2018

6 checks passed

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

@balloob balloob deleted the wemo-unavailable branch Mar 12, 2018

balloob added a commit that referenced this pull request Mar 12, 2018

Fix unavailable property for wemo switch (#13106)
* Fix unavailable property for wemo switch

* Have subscriptions respect the lock

* Move subscription callback to added to hass section

@balloob balloob referenced this pull request Mar 12, 2018

Merged

0.65.4 #13149

@home-assistant home-assistant locked and limited conversation to collaborators Jul 26, 2018

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.