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

Add Freebox device tracker #12727

Merged
merged 3 commits into from Jun 5, 2018

Conversation

@stilllman
Contributor

stilllman commented Feb 26, 2018

Description:

This pull request adds a device tracker platform for the Freebox, the server/router of the french FAI Free, along the lines of PR #10702. It is based on freepybox, but it necessitates some changes that have not been released yet.

The platform can be set up automatically through discovery (once this netdisco commit will be released), or configured as usual.

There is some documentation in the header of the file, I can make that a full documentation page in home-assistant.github.io if need be.

Example entry for configuration.yaml (if applicable):

device_tracker:
  - platform: freebox
    host: foobar.fbox.fr
    port: 1234

Checklist:

  • The code change is tested and works locally.

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.
homeassistant/components/device_tracker/freebox.py Outdated
import homeassistant.helpers.config_validation as cv
from homeassistant.helpers.event import track_time_interval
from homeassistant.components.device_tracker import (

This comment has been minimized.

@houndci-bot

houndci-bot Feb 26, 2018

'homeassistant.components.device_tracker.DOMAIN' imported but unused

@stilllman

This comment has been minimized.

Contributor

stilllman commented Feb 26, 2018

I've run tox in my local repo, but I have no output so I'm not sure it's actually testing anything...

I have some questions regarding this PR:

First of all, is it ok to make this pull request to initiate discussions even if it depends on a third-party library version that is not released yet, or should I have waited?

Secondly, the Freebox requires a manual intervention from the user to authorize applications, pretty much like the Hue bridge does: the first time an application requests access to the server, the user must authorize it directly on the device. What would be the correct way to handle this? Do I need to make a custom component so that the user can configure the access from the home page, as is done for Hue for instance, or is it acceptable to simply give instructions in the documentation (for starters)?

About the automatic setup through discovery, I was wondering if it is something desirable for device trackers? Device tracking can be seen as a bit intrusive, is it a good idea to start doing it without user intervention? If not, what would be the correct approach to notify the user that we discovered a Freebox device, along with the host and port to use in her configuration file if she wishes to set it up?

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Mar 3, 2018

WIP PRs are OK.

You can use the configurator component to ask the user to perform actions to facilitate login or setup. Look at eg the August component.

Another alternative is the new config entry flow. See #12830 for hue. This flow is very new and might be considered experimental. It also requires test coverage if you implement it.

The config flow approach will also handle discovery and let the user dismiss a discovered device if its not wanted to be setup as an integration.

The user can turn off discovery component if it's not wanted. It's also possible to configure it to ignore certain platforms.
https://home-assistant.io/components/discovery/

@ylecuyer

This comment has been minimized.

ylecuyer commented Mar 4, 2018

Is there a way to beta test ?

@stilllman

This comment has been minimized.

Contributor

stilllman commented Mar 4, 2018

@MartinHjelmare The new config flow seems to be exactly what I had in mind, I'll read through the related PRs and see if I can implement it.

@ylecuyer Beta testing would be much welcomed, but for now it requires a bit of manual intervention to install the unreleased-yet dependencies. You will need to install freepybox with the latest changes, either from the upstream repo, or from my fork where the version number has already been bumped to 0.0.3. If you want to test discovery, you will need a version of netdisco with this commit, or you can rollback the netdisco version number to 1.2.4 and use manual configuration. Please let me know if you have any questions or feedback!

@stilllman

This comment has been minimized.

Contributor

stilllman commented Apr 27, 2018

It's been a while since I reached out to the maintainer of Freepybox without response, so I decided to fork his work in an asynchronous library, which will be more suited to integration in Hass. I've released it on PyPI (aiofreepybox) and I've modified the component to make it asynchronous as well.

However, I still have an error I'm not sure how to solve: my log repeatedly indicates that "async_update_info was never awaited". As far as I understand, it should be called and awaited by async_track_time_interval (or another function down the line), right? I also have another error that may or may not be related:

2018-04-28 01:34:16 ERROR (MainThread) [homeassistant.core] Error doing job: Exception in callback _ProactorReadPipeTransport._loop_reading(<_OverlappedF...ed result=b''>)
Traceback (most recent call last):
File "c:\python36\lib\asyncio\events.py", line 145, in _run
self._callback(*self._args)
File "c:\python36\lib\asyncio\proactor_events.py", line 188, in _loop_reading
self._closing)
AssertionError
2018-04-28 01:34:16 INFO (MainThread) [homeassistant.core] Bus:Handling <Event system_log_event[L]: timestamp=1524872056.4453623, level=ERROR, message=Error doing job: Exception in callback _ProactorReadPipeTransport._loop_reading(<_OverlappedF...ed result=b''>), exception=Traceback (most recent call last):
File "c:\python36\lib\asyncio\events.py", line 145, in _run
self._callback(*self._args)
File "c:\python36\lib\asyncio\proactor_events.py", line 188, in _loop_reading
self._closing)
AssertionError
, source=c:\users\luc_t_000\projects\home-assistant\homeassistant\core.py>

I have this error even when I disable the Freebox device tracker, so I'm not sure it comes from my code, but since it seems to relate to the asyncio loop, I'm wondering if it could be related to the other error. Any insight? FYI, I'm currently working on Windows with Python 3.6.4.

@stilllman

This comment has been minimized.

Contributor

stilllman commented Apr 29, 2018

I've tested my code on Ubuntu with Python 3.5.3: I no longer have the error in asyncio, but I still have the "coroutine was never awaited" one, so I guess I'm doing something wrong with async_track_time_interval.

@ylecuyer

This comment has been minimized.

ylecuyer commented Apr 30, 2018

Shouldn't async_update_info be decorated with @asyncio.coroutine ?

If you search for async_track_time_interval they follow this pattern

homeassistant/components/device_tracker/freebox.py Outdated
}
api_version = 'v1' # Use the lowest working version.
fbx = Freepybox(

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Apr 30, 2018

Member

Do we need to create a new instance of Freepybox at every update? Can't we instantiate this in init and save it as an instance attribute?

This comment has been minimized.

@stilllman

stilllman May 3, 2018

Contributor

Sure, I'll do that. Do you see any problem if I leave the session opening/closing in the update function though? Doing so, we don't have to deal explicitely with lost connection, auth token refresh etc.

homeassistant/components/device_tracker/freebox.py Outdated
for device in hosts
if device['active']]
asyncio.gather(*[self.async_see(mac=d.id, host_name=d.name)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Apr 30, 2018

Member

The future that this returns is never awaited.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Apr 30, 2018

Member

Since we're not interested in the result of the future, ie the return value of async_see, I suggest you use asyncio.wait instead of asyncio.gather.

This comment has been minimized.

@stilllman

stilllman May 3, 2018

Contributor

Right, fixed

homeassistant/components/device_tracker/freebox.py Outdated
await fbx.open(self.host, self.port)
try:
hosts = await fbx.lan.get_hosts_list()
finally:

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Apr 30, 2018

Member

Why do we need the try... finally?

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Apr 30, 2018

Member

If getting the hosts blows up, nothing in the following normal flow will execute, including awaiting the closing task.

This comment has been minimized.

@stilllman

stilllman May 3, 2018

Contributor

The goal was to "fire and forget" the closing task when an exception occurs (push the async task to the event loop, but without waiting for it to finish before rethrowing the exception), and "fire and await" in the normal flow, but I may have misunderstood how this works (this is my first time developing with asyncio).

Perhaps I should wrap this in an async context manager, I'll need to check how to do that properly.

homeassistant/components/device_tracker/freebox.py Outdated
def __init__(self, hass, config, async_see):
"""Initialize the scanner."""
from aiofreepybox import Freepybox
import asyncio

This comment has been minimized.

@houndci-bot

houndci-bot May 3, 2018

'asyncio' imported but unused

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 4, 2018

Member

asyncio is part of standard library so should be imported at the top of the module.

homeassistant/components/device_tracker/freebox.py Outdated
def __init__(self, hass, config, async_see):
"""Initialize the scanner."""
from aiofreepybox import Freepybox
import asyncio

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 4, 2018

Member

asyncio is part of standard library so should be imported at the top of the module.

homeassistant/components/device_tracker/freebox.py Outdated
"""Initialize the scanner."""
from aiofreepybox import Freepybox
import asyncio
import socket

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 4, 2018

Member

Same as above.

homeassistant/components/device_tracker/freebox.py Outdated
"""Check the Freebox for devices."""
_LOGGER.info('Scanning devices')
import asyncio

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 4, 2018

Member

Move to top of module.

homeassistant/components/device_tracker/freebox.py Outdated
await self.fbx.open(self.host, self.port)
try:
hosts = await self.fbx.lan.get_hosts_list()
finally:

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 4, 2018

Member

If we expect an exception here, maybe we should try to catch it, if we know what it is and if it will happen often?

"""
Support for device tracking through Freebox routers.
This tracker keeps track of the devices connected to the configured Freebox.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 4, 2018

Member

Move the rest of the docstring from this line to the documentation. Replace this line with a link to the documentation. See other platforms for the correct link format.

@stilllman

This comment has been minimized.

Contributor

stilllman commented May 4, 2018

@MartinHjelmare Thanks for the reviews, all good points! I will be on holidays for the next week, I'll make the changes when I'll be back.

However, I still don't understand why async_update_info is never awaited, should I decorate it with @asyncio.coroutine instead of using async def, like @ylecuyer proposed? Looking at the code of async_track_time_interval, it seems to me that both versions should be supported, but then again I'm no asyncio expert.

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented May 4, 2018

No, we should use async def from now on when defining coroutine functions.

Are you still getting that warning after the latest changes?

@stilllman

This comment has been minimized.

Contributor

stilllman commented May 16, 2018

@MartinHjelmare After investigating a bit, I think I understand what the problem is. The Throttle decorator wraps the coroutine function in a non-coroutine function (and rightfully so, asyncio.iscoroutinefunction returns False for bound methods decorated with @Throttle), so async_add_job ends up running it in an executor instead of wrapping it in a Task. I'll add a non-decorated method that calls the throttled one to work around this, unless you have a better idea?

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented May 17, 2018

Since you're scheduling the update from this module, I suggest simply taking the max of configured scan interval and min time between updates, and use that as update interval. And remove the throttle.

@stilllman stilllman referenced this pull request May 22, 2018

Merged

Add documentation for the Freebox device tracker #5415

1 of 2 tasks complete
@stilllman

This comment has been minimized.

Contributor

stilllman commented May 22, 2018

Documentation added in home-assistant/home-assistant.io#5415.

homeassistant/components/device_tracker/freebox.py Outdated
except HttpRequestError:
_LOGGER.exception('Failed to scan devices')
last_results = [_build_device(device)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 23, 2018

Member

Put this and the see call within an else: here. These calls should not be done if the error happens.

homeassistant/components/device_tracker/freebox.py Outdated
for device in hosts
if device['active']]
await asyncio.wait([self.async_see(mac=d.id, host_name=d.name)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare May 23, 2018

Member

Add a check if last_results is truthy before doing this.

@stilllman stilllman changed the title from [WIP] Add Freebox device tracker to Add Freebox device tracker May 23, 2018

@syssi

syssi approved these changes Jun 5, 2018

@syssi syssi merged commit 1036394 into home-assistant:dev Jun 5, 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 remained the same at 94.004%
Details
@syssi

This comment has been minimized.

Member

syssi commented Jun 5, 2018

@balloob balloob referenced this pull request Jun 22, 2018

Merged

0.72 #15088

MizterB added a commit to MizterB/home-assistant that referenced this pull request Aug 11, 2018

Add Freebox device tracker (home-assistant#12727)
* Add a device tracker for Freebox routers

* Automatic setup of Freebox device tracker based on discovery

* Make the Freebox device tracker asynchronous
@gsemet

This comment has been minimized.

gsemet commented Aug 30, 2018

Hi ! This component works great :) @stilllman have you tried to add internet status support (sensor I guess?), in order to see which state is the connection (1,2 ... to 6), connexion uptime, current speed dl/up,... ?

@stilllman

This comment has been minimized.

Contributor

stilllman commented Aug 30, 2018

Hey @gsemet! I have considered making a full-fledged component instead of just a device tracker, with support for toggling wifi, getting list of missed calls, etc., but I wanted to see if there was a need in the community before doing so :)

I won't start working on this before at least a few weeks though, I just had a happy event in my life that will keep me quite busy! In the meantime, how about creating a feature request with the features you would like to see in the component? You can take a look at the Freebox API to see what would be feasible or not.

@gsemet

This comment has been minimized.

gsemet commented Aug 30, 2018

Congrat for this happy event :) mine is expected by the end of this year, so I may find some time to implement the freebox status + some action switchs ;)

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Aug 30, 2018

@stilllman stilllman deleted the stilllman:freebox-device-tracker branch Oct 9, 2018

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