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

Add Home Connect integration #29214

Merged
merged 61 commits into from
May 5, 2020

Conversation

DavidMStraub
Copy link
Contributor

Description:

This adds an integration to support home appliances (ovens, fridges, hobs, hoods, dryers, washing machines, dish washers) following the Home Connect standard. These are mostly devices by Bosch and related brands (Siemens, Neff, ...).

The integration uses the official REST API (docs) and, after an initial data fetch, works by subscribing to an SSE stream (in a separate thread) for each appliance, i.e. it work by "cloud push".

I developed the underlying Python library homeconnect myself (repo here).

The HA integration was already announced by me last year in the forums and I received a lot of feedback there and in the development repository. The custom component is also available via HACS.

While most of the reported issues in the past had to do with the OAuth2 authentication flow, I have now completely rewritten that part using the awesome new official Home Assistant OAuth2 work flow that is also used by Somfy and it works very well.

I feel the integration has now reached a level of maturity and reliability for me to attempt a PR. Having it as an official integration could allow for big improvements in the future:

  • It could leverage the new account linking service
  • Home Assistant (or Nabu Casa) could apply for enhanced permissions, e.g. switching on ovens remotely, which unfortunately is disabled for normal developer accounts.

Apologies for submitting such a large chunk of code, but it didn't seem reasonable to me to remove parts that are already part of the public custom component.

Pull request with documentation for home-assistant.io: home-assistant/home-assistant.io#11336

Example entry for configuration.yaml (if applicable):

homeconnect:
  client_id: CLIENT_ID
  client_secret: CLIENT_SECRET

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

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

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

  • The manifest file has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

Copy link
Member

@springstan springstan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some small things to get the reviewing process started :)

homeassistant/components/homeconnect/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/homeconnect/api.py Outdated Show resolved Hide resolved
homeassistant/components/homeconnect/api.py Outdated Show resolved Hide resolved
@MartinHjelmare MartinHjelmare changed the title New integration: Home Connect Add new Home Connect integration Dec 1, 2019
Copy link
Member

@springstan springstan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for implementing my suggestions! :)

return unload_ok


@Throttle(SCAN_INTERVAL)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For initial contribution this is ok.

In the future, you might want to put this into a data class that sends a signal when new data is available. Now it can take a while before new data is adopted by entities.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately the API doesn't push info about new devices. One has to query them and then listen to a separate SSE endpoint for each of them for state updates.

To be honest, I blatantly copied/adapted this function from Somfy without a very deep understanding 😬

devices = []
for app in appl:
if app.type == "Dryer":
device = Dryer(app)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a future PR, we have a great little helper for this.

from homeassistant.util.decorator import Registry

MODELS = Registry()

@MODELS.register("dryer")
class Dryer(DeviceWithDoor, DeviceWithPrograms):
    …

for app in appl:
    model = MODELS.get(app.type)

    if model is None:
        _LOGGER.warning(…)
        continue

    device = model(app)
    …


async def async_step_user(self, user_input=None):
"""Handle a flow start."""
if self.hass.config_entries.async_entries(DOMAIN):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why limit it to 1 installation? It looks like the code can handle multiple config entries ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I never actually thought about this, I will look into it!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be sure I understood correctly: I can just get rid of async_step_user altogether and that's it, right? Did that in 1b88efb.

def get_entities():
entities = []
data = hass.data[DOMAIN]
for device_dict in data.get(DEVICES, []):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a bug as you write to hass.data[DOMAIN]conig_entry.entry_id] in the component.

data = hass.data[DOMAIN]
hc_api = data[entry.entry_id]
try:
data[DEVICES] = await hass.async_add_executor_job(hc_api.get_devices)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should make a wrapper class that holds hc_api and the latest device data, and store that in hass.data[DOMAIN][entry.entry_id] so you can support multiple entries.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right!
Since the only other data that I was storing was the list of devices, I simply added it as a property of the existing class now in
41b6c9d .

@mountainsandcode
Copy link
Contributor

mountainsandcode commented Dec 26, 2019

Hi @DavidMStraub, is this "ready" for testing? Happy to give it a go and report back in case it is :) Thanks for the effort for this one - will be very useful!

@springstan
Copy link
Member

@mountainsandcode I think you can test it by installing it via HACS as a custom component ✌

@balloob
Copy link
Member

balloob commented Jan 4, 2020

@DavidMStraub what is the status of this PR ?

@DavidMStraub
Copy link
Contributor Author

@mountainsandcode @balloob Apologies for the silence and for not having addressed all of the useful comments (thanks for them!). Apart from the holiday break, this was mostly due to an outstanding issue in the underlying library that appears to be causing the SSE stream from the API to break and leaving the devices in a stale state:
DavidMStraub/homeconnect#3

As noted by @springstan, there are thankfully many people testing it and several have reported the problem. I wanted to fix this issue before finalizing the changes to the PR, but haven't found the cause yet. (The SSE code (source) uses requests.get(..., stream=True) under the hood which is supposed to automatically reconnect, and I don't understand why it doesn't. Could it somehow be that Home Assistant closes the thread if no data is received for some time?)

Anyway, I am definitely commited to bringing this to the finish line. I will address the remaining comments in the next 3-4 days and let you know if I manage to fix this.

@balloob
Copy link
Member

balloob commented Jan 5, 2020

requests.get does not automatically reconnect.

@DavidMStraub
Copy link
Contributor Author

@MartinHjelmare please disregard that I just re-requested a review. There have been requested changes that I had overlooked. Will need to adress them first.

homeassistant/components/home_connect/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/home_connect/switch.py Outdated Show resolved Hide resolved
homeassistant/components/home_connect/switch.py Outdated Show resolved Hide resolved
homeassistant/components/home_connect/switch.py Outdated Show resolved Hide resolved
homeassistant/components/home_connect/switch.py Outdated Show resolved Hide resolved
homeassistant/components/home_connect/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/home_connect/switch.py Outdated Show resolved Hide resolved
homeassistant/components/home_connect/switch.py Outdated Show resolved Hide resolved
from .entity import HomeConnectEntity

_LOGGER = logging.getLogger(__name__)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should set PARELLEL_UPDATES constant to limit or not limit number of parallel updates for all entities within this platform. We should do this for all platforms.

https://developers.home-assistant.io/docs/integration_fetching_data#request-parallelism

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that all entities have async_update, is this still necessary? Of course the SSE threads are parallel, but this is unrelated to PARELLEL_UPDATES I suppose.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's no I/O in the update and we use async_update we don't need to set parallel updates. It will default to no limit.

A future PR could refactor and drop the use of async_update and instead do the processing inside _update_callback and call self.async_write_ha_state at the end. This would remove the extra scheduling on the event loop that we don't need.

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

@MartinHjelmare MartinHjelmare merged commit 86d410d into home-assistant:dev May 5, 2020
DavidMStraub added a commit to DavidMStraub/homeassistant-homeconnect that referenced this pull request May 5, 2020
@lock lock bot locked and limited conversation to collaborators May 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants