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

Lazy discovery on demand #215

Merged
merged 4 commits into from
Feb 13, 2018

Conversation

syssi
Copy link
Collaborator

@syssi syssi commented Feb 12, 2018

I want to reduce the number of discovery messages of specific devices (Philips Lights).

@yawor
Copy link
Contributor

yawor commented Feb 12, 2018

@syssi in the async code I've started working on (not much time recently to move it forward) I'm running discovery independently from the requests. It runs on timer every minute or so. There's also an async lock which keeps the command from running if library is waiting for discovery response.
I can push my initial async code to a PR but there's not much there yet.

@syssi
Copy link
Collaborator Author

syssi commented Feb 12, 2018

@rytilahti The lazy device discovery works fine for philips lights without drawbacks so far. Are you happy with the order of the init parameters?

@rytilahti
Copy link
Owner

rytilahti commented Feb 12, 2018

That looks fine for me (the "force discovery" was merely a hack to make it work after some FW update), but let me run this for a day or so on all my devices just to be sure. I'm not sure about the parameter order, but I think the lazy discovery should be set as True per default. Those devices with problems can be forced to do it every time.

@yawor iirc some miio protocol implementation (maybe it was the javascript one) used to do updates every 180 or 200 seconds. I'm unsure if they had any data to back that up though. Having an initial PR for async sounds good though! In a couple of weeks homeassistant will drop python 3.4 support, which probably will lead the way for dropping it also from this library (if @syssi is not too much against it :-)).

@rytilahti rytilahti merged commit deb072c into rytilahti:master Feb 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants