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

Refactor Google Assistant #12959

Merged
merged 15 commits into from Mar 8, 2018

Conversation

Projects
None yet
7 participants
@balloob
Copy link
Member

commented Mar 7, 2018

Description:

We tried to get our Google Assistant integration certified by Google and we ran into a couple of issues #12935.

When I was looking into fixing the bugs, I realized that the code could benefit from a major cleaning. I've followed a similar model as we used for Alexa and have structured all the logic based on the traits that Google supports.

Because of this, I have removed a few hacks:

  • It is no longer possible to override the domain that Home Assistant uses for an entity. This was bound to go wrong when we would test supported features for different domains.
  • Removed support for disguising temperature sensors as thermostats. We should follow the traits that Google offer us and not offer things that will only work half.

I'm running the old tests on the new code without modification. Will add some new tests for the new code on top of that too with as goal to replace the old tests too. The old tests test too many things at once, making it difficult to test specific code paths.

To refactor:

  • SYNC
  • QUERY
  • EXECUTE

To add:

  • More error checking and reporting
  • More tests

This refactor will open up the way to easily implement a ton more traits easily. Especially excited about the Toggles trait that can turn things on/off.

Fixed bugs:

  • We will now correctly report the new state as a response to an execute command
  • We will raise error when trying to set temperature out of range of the device

CC @philk

Breaking change: Google Assistant has removed the type option for entity configuration and no longer allows sensors to disguise as thermostats.

Related issue (if applicable): #12935

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io#<home-assistant.github.io PR number goes here>

Checklist:

  • The code change is tested and works locally.

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.

balloob added some commits Mar 7, 2018

@OttoWinter
Copy link
Member

left a comment

Great 👍 I think if people still want custom device type they can use templates anyway.


async def execute(self, hass, command, params):
"""Execute a color spectrum command."""
hex_value = "{0:x}".format(params['color']['spectrumRGB']).zfill(6)

This comment has been minimized.

Copy link
@OttoWinter

OttoWinter Mar 7, 2018

Member
hex_value = "{0:06x}".format(params['color']['spectrumRGB'])
'thermostatHumidityAmbient': 54.0
}
assert len(devices) == 3
assert devices['climate.heatpump'] == {

This comment has been minimized.

Copy link
@balloob

balloob Mar 7, 2018

Author Member

Same tests except broken up in testing per device. That way the error logs are more readable.

Also removed the 'on' attribute as that is part of the OnOff trait that is not enabled for climate devices.

"command": "action.devices.commands.ColorAbsolute",
"params": {
"color": {
"spectrumRGB": 16711680,

This comment has been minimized.

Copy link
@balloob

balloob Mar 7, 2018

Author Member

Removed this test as it violates the spec:

According to Google, a light is either in Color spectrum or color temperature mode. With spectrum being able to do all colors, temperature mode is just for whites.

https://developers.google.com/actions/smarthome/traits/colortemperature#device-states

Since a given light is in spectrum OR temperature mode, this object includes the current color settings in the relevant mode.

@@ -415,13 +362,14 @@ def test_execute_request(hass_fixture, assistant_client):
body = yield from result.json()
assert body.get('requestId') == reqid
commands = body['payload']['commands']
assert len(commands) == 8
assert len(commands) == 6

This comment has been minimized.

Copy link
@balloob

balloob Mar 7, 2018

Author Member

New implementation will group all successful commands for 1 device in a single result object.


ceiling = hass_fixture.states.get('light.ceiling_lights')
assert ceiling.state == 'off'

kitchen = hass_fixture.states.get('light.kitchen_lights')
assert kitchen.attributes.get(light.ATTR_COLOR_TEMP) == 476

This comment has been minimized.

Copy link
@balloob

balloob Mar 7, 2018

Author Member

This was part of the removed test.



@asyncio.coroutine
def test_determine_service():

This comment has been minimized.

Copy link
@balloob

balloob Mar 7, 2018

Author Member

This test no longer worked. Will be covered by the new trait tests.

@balloob balloob referenced this pull request Mar 7, 2018

Closed

Google Assistant: report device as offline if unavailable #12938

2 of 2 tasks complete

balloob added some commits Mar 7, 2018

@balloob balloob changed the title WIP: Refactor Google Assistant Refactor Google Assistant Mar 7, 2018

@home-assistant home-assistant deleted a comment from houndci-bot Mar 7, 2018

@home-assistant home-assistant deleted a comment from houndci-bot Mar 7, 2018

balloob added some commits Mar 7, 2018

@balloob

This comment has been minimized.

Copy link
Member Author

commented Mar 8, 2018

I've tested the code locally and verified that it works.

balloob added some commits Mar 8, 2018

homeassistant/components/cloud/__init__.py Outdated
@@ -175,7 +174,7 @@ def should_expose(entity):
"""If an entity should be exposed."""
return conf['filter'](entity.entity_id)

self._gactions_config = ga_sh.Config(
self._gactions_config = ga_s.Config(

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Mar 8, 2018

undefined name 'ga_s'

homeassistant/components/cloud/__init__.py Outdated
@@ -175,7 +174,7 @@ def should_expose(entity):
"""If an entity should be exposed."""
return conf['filter'](entity.entity_id)

self._gactions_config = ga_sh.Config(
self._gactions_config = ga_s.Config(

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Mar 8, 2018

undefined name 'ga_s'

@balloob balloob force-pushed the refactor-google-assistant branch to eba2925 Mar 8, 2018

@home-assistant home-assistant deleted a comment from houndci-bot Mar 8, 2018

@home-assistant home-assistant deleted a comment from houndci-bot Mar 8, 2018

@balloob balloob merged commit 9b1a75a into dev Mar 8, 2018

6 checks passed

WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.05%) to 93.594%
Details
hound No violations found. Woof!

@balloob balloob deleted the refactor-google-assistant branch Mar 8, 2018

@balloob balloob referenced this pull request Mar 9, 2018

Merged

0.65 #12995

@balloob balloob removed the new-platform label Mar 9, 2018

@bmesuere

This comment has been minimized.

Copy link

commented Mar 10, 2018

@balloob I have actual lights that are switches in home assistant (due to how the rfxcom component works). I used the override to let them show up as lights in Google assistant and enable queries such as "turn on all living room lights". What is the recommended way to handle this case now?

@arsaboo

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2018

@bmesuere I have a switch that controls a light. I have named it Driveway light and I can turn it on/off by saying turn on/off driveway light. I am thinking you can still add it to a group or try out light.group.

@balloob

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2018

light.group will actually not work as it is limited to lights. The reason for removing it is that all of Google Assistant works by testing against supported_features property. By overriding the domain, we pass switch supported features to light supported features and things start going weird. It's not up to the Google Assistant to support shortcomings in other integrations. Instead, consider using a light.template to make a switch be represented as a light. If you want the sensor as thermostat feature back, use the generic thermostat.

@CWSpear

This comment has been minimized.

Copy link

commented May 8, 2018

@balloob how do templates work in this way? For example, how can I make a remote behave as a switch so I can use it to turn on/off my TVs with Google Assistant?

@arsaboo

This comment has been minimized.

Copy link
Contributor

commented May 8, 2018

@CWSpear Use template switch, for example.

Also, please don't use merged PRs for discussions. Open a new issue (if it is an issue) or post in forums for configuration questions.

@CWSpear

This comment was marked as spam.

Copy link

commented May 8, 2018

@arsaboo I tried and didn't get any good answers. In fact, the only response I got was point here. And here got me an answer in 13 minutes...

But I will try and keep it in mind for next time. Anyway, that link helped, and I think I've got it figured out. Thanks!

@home-assistant home-assistant locked as resolved and limited conversation to collaborators May 8, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.