Add citybikes platform

[aronsky]

Description:

(Better, async version of #7914. Github won't allow me to re-open the old PR since I rebased it on a newer dev branch)

CityBikes is an open API platform, that provides data about bike sharing systems around the world. It allows real-time monitoring of bike availability at bike sharing stations.

This platform connects the CityBikes API to home assistant, opening up the possibilities for advanced automations, such as recommending the user a bike sharing station based on the amount of bikes available, upon leaving the house.

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

Example entry for configuration.yaml (if applicable):

sensor:
  - platform: citybikes
    radius: 500

Checklist:

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

• Documentation added/updated in home-assistant.github.io

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][ex-requir]).
• New dependencies are only imported inside functions that use them ([example][ex-import]).
• New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
• New files were added to .coveragerc.

[mention-bot]

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

[balloob]

Instead of making this a boolean, make it an asyncio.Event.

Inside the Network, you can do

self.ready = asyncio.Event(loop=hass.loop)

# when network ready
self.ready.set()

And inside here, instead of sleeping, do this:

yield from network.ready.wait()

[aronsky]

The event seems like an elegant solution (I was thinking about using a dispatcher at first), but having a ready property allows me to check whether an update is necessary in the async_update method of a station. But that's not critical, and the event solution really looks more straightforward, so I'll go with that.

[balloob]

Looks awesome! Had a few minor comments.

[balloob]

Use cv.latitude and cv.longitude for validating coordinates.

[balloob]

Please do not use global variables. You can store data in hass.data, a dictionary for exactly this purpose.

[aronsky]

I assume I should use the DOMAIN as the top level key?

[balloob]

Yes, or another key of choice (but have it include the domain)

[balloob]

Why are you adding a job instead of doing it during the setup phase of the platform ? Because of the way Home Assistant works, it will still wait for this taks to finish, but because it is not under your setup method, we can't skip it if it takes too long.

[aronsky]

Since the setup requires communication with the remote API, I want it to be a non-blocking task - otherwise, the start up of Home Assistant is blocked until this is set up.

Furthermore, if network connectivity is unavailable on start-up (for instance, after a power outage, the Home Assistant instance might boot up before the Internet router), I'd like the platform to perform additional attempts at setting itself up, by scheduling the setup routine on failure. If I'm doing that, I thought I might as well start with the scheduling from the beginning, and avoid blocking while I'm at it.

[balloob]

Any task scheduled during setup will still block Home Assistant. Please don't do this.

[aronsky]

I see, I had no idea it would still block the initialization. I changed it for now - but is there a proper way to have the initialization done asynchronously, as well as retry on network failure?

[balloob]

If you have a network failure and want Home Assistant to retry in a bit, you can raise PlatformNotReady. Home Assistant will then try again in 30 seconds (with auto-backoff till 5 minutes)

[balloob]

(this was just added, might need to rebase)

[balloob]

What about a server error ?

[aronsky]

I took the list of handled exceptions from another component and a library for CityBikes (which, BTW, I failed to convert to async in a usable way, I might write and release a standalone async version later).

I'll add handling of non-200 responses.

[aronsky]

It seems that aiohttp.ClientError is an umbrella exception to all the possible errors that can happen when performing an HTTP request, including any server errors: http://aiohttp.readthedocs.io/en/stable/client_reference.html#client-exceptions

[balloob]

What about a server error?

[aronsky]

It seems that aiohttp.ClientError is an umbrella exception to all the possible errors that can happen when performing an HTTP request, including any server errors: http://aiohttp.readthedocs.io/en/stable/client_reference.html#client-exceptions

[balloob]

This cannot happen because you validate the response schema?

[aronsky]

It can happen in the beginning, when _station_data is None. Anyway, I changed it to use get with a default specifier.

[balloob]

Please drop the else: and just return

[balloob]

This shouldn't happen because you validate the response data

[aronsky]

Agreed, just need to check that _station_data is not empty (which it is, before the first state update).

[balloob]

You're the only consumer of this object, there is no really need to wrap properties around the instance variables for uid, stations and ready.

[aronsky]

Since I changed it as requested, and the comment is now hidden as outdated, I'll ask it here as well: is there a proper way to have the initialization done asynchronously, as well as retry on network failure?

[balloob]

All setups are already done in parallel.

Raise PlatformNotReady and Home Assistant will automatically retry.

You can also choose to initialize only at EVENT_HOMEASSISTANT_START but that goes against the idea of a setup method.

How long does the setup take ?

[aronsky]

Awesome, the new retry mechanism sounds like exactly what I need. I will do that. Hope the rebase doesn't break the PR, like the last time :)

The startup requires, in the expected common use case, 2 web requests. So with a good Internet connection, it shouldn't take more than a few seconds. Currently, I'm using an async timeout of 5 seconds for requests, so it will be at most 10 seconds.

[balloob]

Looks like your rebase is including commits that are not yours.

When you start a new feature, always make sure that you start from a fresh copy of the dev branch from our repo. Not from your own repo, as that one is not automatically kept up to date.

[aronsky]

I will reread the git documentation to avoid this problem in the future. Is there anything that can be done to fix it in this constellation? Or should I open a new PR?

[balloob]

It's easy to mess up Git, happens to me occasionally too.

I would go for this approach https://xkcd.com/1597/

[aronsky]

OK, looks like I managed to push the correct version without any other commits, and without a new PR. Hurray!

The linting error is not in my code, it's in the dev branch on which I'm based.

[balloob]

Top