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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Google Nest Device Access / SDM integration #41119

Closed

Conversation

allenporter
Copy link
Contributor

@allenporter allenporter commented Oct 3, 2020

Breaking change

Proposed change

Create an integration with for the Google Nest Device Access program, using the Smart Device Management API. This is fairly bones with a single sensor platform for temperature and humidity sensors, using a polling API call using the pypi library google-nest-sdm. Authentication with Nest happens via OAuth and requires a decent amount of custom setup following the Google Nest Device Access instructions (requires a fee, creating a project id, and setting up oauth client credentials to get a client id and secret). Future planned enhancements include using a pull API, handling camera events, and controlling climate devices.

I have not yet added documentation for the integration, but the change contains a README.md for early developer instructions.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml
google_nest:
  project_id: <your project id>
  client_id: <your client id>
  client_secret: <your client secret>

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

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.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 馃 Silver
  • 馃 Gold
  • 馃弳 Platinum

To help with the load of incoming pull requests:

Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

This should update or existing nest integration, and not implement a new domain.

Dev automation moved this from Needs review to Review in progress Oct 5, 2020
@allenporter
Copy link
Contributor Author

allenporter commented Oct 5, 2020

In the documentation pull request the question was asked: Why a new integration? Below is some context for thatrationale.
Given the explicit guidance to go with the existing domain, will change course to do that, but leaving the context here:

Background:
Compared to the existing nest API that is not accepting new users, the new API has some differences:

  • Has a different data model focused on traits rather than devices
  • Provides a fewer fields/device properties of the old API
  • Supports fewer devices overall (e.g. no CO-2 sensor, no Protect).
  • Uses different authentication endpoints, etc.
  • Device Access Program also requires a US$5 fee,

Nothing too surprising here though, as different APIs obviously will have differences.

Approach: Separate integrations:
This request proposes a new integration that follows the latest recommendations from the developer docs including a simple oauth setup flow with little customization, data polling libraries, etc. This approach optimizes for simplicity within the integration itself, pushing the divergence between authentication, APIs, supported devices, up a level to the community. This may have down sides that the community has to pay additional cost of knowing that two separate integrations exist (e.g. documentation overhead, etc), but leaves each standalone integration simpler.
Assumption: If the old Nest API is fully turned down, the old integration can be fully deleted.

Alternative Approach: Update existing integration:
An alternative can be add pivot points within the existing nest integration to support the new APIs. This could appear to the end user as a separate configuration option, or set of configuration options. Here are some considerations:

  • Configuration: This path would require additional code/validation to (1) indicate that some configuration options are not supported in the new API and (2) disable functionality while it is developed, since we prefer to evolve integrations in smaller pull requests rather than a single change to support all functionality. Over time, it may be difficult to tell which functionality is for the old API, and new API, but that complexity would remain within the integration itself.

  • Testing Plan: This is a good example of the power of unit tests, and presumably one big reason Home Assistant has a good culture around testing (observation as a newcomer). This is a case where owning a device is not enough to do development on an integration, and instead you're really integrating with an API.

    • Automated test coverage: The existing API has test coverage for the configuration flow, though none of the existing platforms. Before proceeding down the path of adding a split point, it would probably make sense to add more test coverage for the existing code to make sure that it does not break. Increasing test coverage is a good thing to do anyway (more valuable to the community), though little value for an integration that does not accept new users.
    • Manual Testing: I own Nest devices, but am not able to use the API for the existing integration since it is restricted. Implication: To develop in nest would mean that you need to both be an authorized user of the old API (registered before a certain date) and have paid the device registration fee for the new API.
    • Manual Testing Alternative: more coordination in the community: e.g. I go find a friend that uses the old API and get them to test it out before every commit.

Alternative Approach: Replace existing integration: The existing nest API is not accepting new integrations, but still works for anyone who was already authorized to use it. Assumption: This is a non-starter as many members of the community would be upset by this given the above differences in the API (and the US$5 fee!)

Alternative Approach: Custom Component: Another alternative could be to develop this as an entirely separate custom component, so that the complexity of diverging Nest APIs does not get introduced into the Home Assistant core. My impression is that Nest devices are popular enough that it is worth the effort to do the upfront design work to make this a core integration. I suspect that doing more development in a silo will also make review more difficult later. I am not pursuing this, but it is still a viable option.

Python Library API:
There was a similar decision point for the Python Library that talks to the API. Should the existing python-nest library be extended to work with the Device Access program?

When examining the existing python-nest library, it seemed the data models diverged a decent amount, and even slight differences between authentication models seemed to add up in making the code a little more complex to support both APIs. Reviewing the Home Assistant docs on Python Library: Modelling Data it seemed simplest to follow the guidance there to "expose the data from the API in the same structure as that the API offers it" and "API libraries should try to do as little as possible.". That is good guidance, as following the documented best practice produced a very lean and simple library that is easy to follow.

A compatibility layer here could theoretically abstract away the API differences, but I think the APIs differ enough that leaning into the recommended approach makes more sense here.
Note: diverging at the Python Library API is a separate decision point and a single Home Assistant integration can use multiple APIs, or a single python package can support multiple APIs.
Assumption: There will likely not be new investment in python-nest or new investment in improving the existing/old Nest API functionality with Home Assistant.
Assumption: Introduction of a new Nest API likely means there will soon be a deprecation plan for the old Nest API

Some user discussion:
There was some discussion in the community with some folks weighing in on this topic with reasons for both approaches from a user perspective.

@allenporter
Copy link
Contributor Author

allenporter commented Oct 6, 2020

I'm scoping out more of what it would take to update nest to support the new API. Here are some additional considerations that need to be taken into account to support two different APIs in a single integration:

  • Differences in API support: Many services, devices, and attributes within a device would not be supported:

    • SET_ETA service
    • SET_AWAY_MODE service
    • CANCEL_ETA
    • Camera "turn_on" and "turn off"
    • Nest Home: away, security_state
    • Nest Thermostat: fan, is_using_emergency_heat, is_locked, present_state
    • Nest Protect: Not supported at all
  • Both APIs: Users have expressed interest in using both APIs at once, given the differences in functionality between each API. Supporting this would add another level of complexity to the configuration schema. e.g. imagine configuring which devices/entities should use which API would be very complex or duplicative of all options. (Would want to avoid having the same device twice)

  • Platforms: Any entity would need code inside to support both APIs, or every platform has two code paths with duplicate API logic and duplicate entities with differences. Many constants for managing/fliter entities would be duplicated within the platform, and need to indicate which platform it is for.

  • Documentation: Either have two entirely separate sections of the documentation (one for each API) or go through everything and comment on if it is available in the new API or not. Setup instructions for configuring accounts will be duplicated/different, as well as additional new sections for configuring cloud / pubusb.

  • Naming: Many of the names currently used by nest have different naming schemes in the new API. For compatibility maybe it makes sense to stick with the old naming, though it just means more potential rot. Adapting to the existing naming may make a migration path for users simpler, if the new API happens to support their use case. Stuff like 'leaf' vs 'manual_eco' get odd.

Question: Is it possible to have a single split point at the start of the integration, then load an entirely different set of platforms depending on a configuration setting? e.g. load sensory.py for old API and sensor_foo.py for new API? Using separate files (for everything but init) could be a path towards managing the complexity of having two separate API integrations within nest

(Maybe I can try and find someone to work through some of this in more detail or an early review to understand preferred approaches, as I think its going to be a pretty painful review otherwise)

@frenck
Copy link
Member

frenck commented Oct 7, 2020

In the end, the old API will not be supported. No new users can migrate to it and its a matter of time now. The nest integration should be replaced (including the library as you suggested), not a new one added.

@allenporter
Copy link
Contributor Author

I think the question is: Over what time frame?

My assumption is that Home Assistant cares about keeping these existing users happy. I am too new to the community to make that judgement call, though I am happy to do the work.

@WisdomWolf
Copy link

If I may weigh in as a fellow software engineer and current user of Home Assistant with a Nest Thermostat on the old API, I think that @allenporter has the right idea. While its true that the old API will eventually be fully deprecated, I imagine that is a year or more away. Creating this as a new integration allows both old and new integrations to co-exist without logistical nightmares. The old (let's call it legacy) integration could then be independently deprecated in the future when appropriate and user's directed to configure the new API prior to removing the legacy one. At that point the new integration would become the defacto standard and both would no longer be supported. That allows for a clean migration path allowing new users to get setup, existing users to not be forced into convolution, and more advanced/picky users to be satisfied with access to both in the meantime.

@mrdarrengriffin
Copy link

I must agree with @WisdomWolf. I too am a software engineer and things like this always come back around in a nasty way. Having a completely new model of the Nest integration using the existing nest: integration will cause a lot of confusion. Imagine if someone was having a hard time understanding how to manually implement the new integration, they would resort to the forums and look at outdated references so the nest: YAML.

As much as it would be nice to keep the current integration, maybe using something to differentiate the old from the new wouldn't hurt.

@WisdomWolf
Copy link

In the end, the old API will not be supported. No new users can migrate to it and its a matter of time now. The nest integration should be replaced (including the library as you suggested), not a new one added.

I would argue that adding a new integration is in line with the eventual goal of replacement and not at all at odds with that goal. If you think about the idea of replacement with physical objects, you first must have a new one to replace the old. I intend to replace my current vehicle with a Tesla, but I need to ensure that at no point do I have no vehicle. So I will first buy the Tesla prior to the end of my current lease and then after some time my existing vehicle will be removed from my garage and will effectively have been replaced by the newer Tesla.

@balloob
Copy link
Member

balloob commented Oct 7, 2020

My first idea would be to tell people to just migrate to the new one. But less data + $5 is a non-starter for that.

So the best option is that we keep it as 1 integration. Just because it is 1 integration, we don't need to have the code be intertwined. We can have 2 entity classes. NestLegacyClimateEntity and NestClimateEntity. And we can keep helpers for the 2 Nest APIs also in different files.

# __init__.py
async def async_setup_entry(hass, entry):
    if "SDM" not in entry.data:
        return await async_setup_legacy_entry(hass, entry)
# climate.py
from .climate_dsm import async_setup_dsm_entry
from .climate_legacy import async_setup_legacy_entry

async def async_setup_entry(hass, entry):
    if "SDM" not in entry.data:
        return await async_setup_legacy_entry(hass, entry)

    return await async_setup_dsm_entry(hass, entry)

And in manifest.json you can specify multiply Python packages.

@allenporter allenporter marked this pull request as draft October 7, 2020 19:20
@allenporter allenporter closed this Oct 8, 2020
Dev automation moved this from Review in progress to Cancelled Oct 8, 2020
@mrdarrengriffin
Copy link

Why was this closed?

@balloob balloob removed the invalid label Oct 8, 2020
@allenporter
Copy link
Contributor Author

I think it closed automatically when I reset my branch to address the comments. I'm fairly new to github, so apologies. Rest assured I'm still working on this, just rewriting some bits.

I'll still go with a "one platform at a time" approach. Stay tuned.

@snicker
Copy link

snicker commented Oct 8, 2020

I'm with @balloob here; the existing integration can handle legacy and new approaches in one integration; understanding which users are on could be the complication/challenge. Adding the integration through UI/YAML should allow the user to choose which they are using (and also allow for the addition of two instances of the Nest integration (for users of Nest protect since that does not seem to be present on the new API). All existing integrations could be assumed to be Legacy when updating the integration/core version of HA.

@allenporter
Copy link
Contributor Author

Yeah the split within the integration via separate files was my best thought for how to manage the complexity. Thanks for spelling it out balloob, I agree it should be possible.

I don't yet see how to support both integrations at once giving my limited understanding of how config works. Can you have the same integration twice with two separate configs? If so, a pointer would help me.

(Just want to make sure you aren't saying we want extra a degrees of freedom within the config to support both with one config stanza. )

@balloob
Copy link
Member

balloob commented Oct 9, 2020

To make it possible to set up two config flows, we should make the YAML config parameters different for the new client ID/secret, ie sdm_client_id and sdm_client_secret.

@allenporter
Copy link
Contributor Author

OK, that is what I was afraid of in Both APIs above I spoke to above. I imagine something like this in the end:

nest:
  client_id: CLIENT_ID
  client_secret: CLIENT_SECRET
  structure:
    - Vacation
  sensors:
    monitored_conditions:
      - 'temperature'
      - 'target'
  sdm_project_id: PROJECT_ID
  sdm_client_id: CLIENT_ID
  sdm_client_secret: CLIENT_SECRET
  sdm_structure:
    - Primary
  sdm_sensors:
    monitored_conditions:
      - 'temperature'
      - 'humidity'

I'm happy to do this, but may be hesitant to go super configurable.

@allenporter
Copy link
Contributor Author

I will definitely start with only supporting just new or old but not both concurrently.

@ammmze
Copy link

ammmze commented Nov 10, 2020

In the end, the old API will not be supported. No new users can migrate to it and its a matter of time now. The nest integration should be replaced (including the library as you suggested), not a new one added.

I realize this is already closed and things have already moved forward with th coexisting, but FWIW (and also as a software engineer), it seems google refers to the ecosystem as "Google Nest" (see screenshots) rather than just Nest. This helps them to separate it (slightly, but nonetheless it is different) from the original company. So IMO having it a separate integration under the name google_nest is appropriate and would help separate the 2 services and allowing the old to die off when necessary.

Screenshot_20201109-213840
Screenshot_20201109-213918

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Nov 10, 2020
@balloob
Copy link
Member

balloob commented Nov 10, 2020

This has already been resolved and released.

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

Successfully merging this pull request may close these issues.

None yet

8 participants