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.

Sign up for GitHub

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

Jump to bottom

Google assistant: add blinds trait for covers #22336

Merged
merged 30 commits into from Mar 30, 2019
Merged

Google assistant: add blinds trait for covers #22336

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
850d8ea
Update const.py
giefca Mar 23, 2019
996bff1
Update smart_home.py
giefca Mar 23, 2019
5e4ed41
Update trait.py
giefca Mar 23, 2019
9685db2
Update test_trait.py
giefca Mar 23, 2019
49cd7b9
Update smart_home.py
giefca Mar 23, 2019
2f6ba02
Update test_trait.py
giefca Mar 23, 2019
959dfd5
Update trait.py
giefca Mar 24, 2019
fb20c50
Update trait.py
giefca Mar 24, 2019
862b4b5
Update test_trait.py
giefca Mar 24, 2019
898e568
Update test_trait.py
giefca Mar 24, 2019
9beac91
Update __init__.py
giefca Mar 24, 2019
862bde6
Update test_trait.py
giefca Mar 24, 2019
5393314
Change email
giefca Mar 25, 2019
0567a06
Trying to correct CLA
giefca Mar 25, 2019
2c33b91
Update __init__.py
giefca Mar 26, 2019
524ff98
Update trait.py
giefca Mar 26, 2019
6704a1b
Update trait.py
giefca Mar 26, 2019
94d94d2
Update trait.py
giefca Mar 26, 2019
d914123
Update trait.py
giefca Mar 26, 2019
8107850
Update __init__.py
giefca Mar 28, 2019
441e83f
Update test_trait.py
giefca Mar 28, 2019
70e25a6
Update test_google_assistant.py
giefca Mar 28, 2019
b76cc1c
Update trait.py
giefca Mar 28, 2019
8e18725
Merge pull request #1 from giefca/giefca-patch-1
giefca Mar 28, 2019
c175f31
Merge pull request #3 from giefca/giefca-patch-2
giefca Mar 28, 2019
66f7c63
Merge pull request #4 from giefca/giefca-patch-3
giefca Mar 28, 2019
d003e25
Merge pull request #2 from giefca/giefca-patch-1-1
giefca Mar 28, 2019
c0b5056
Update trait.py
giefca Mar 28, 2019
64c6572
Update test_trait.py
giefca Mar 29, 2019
eb481a5
Update test_trait.py
giefca Mar 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions homeassistant/components/google_assistant/const.py
View file
Open in desktop
Expand Up @@ -29,6 +29,7 @@
TYPE_FAN = PREFIX_TYPES + 'FAN'
TYPE_THERMOSTAT = PREFIX_TYPES + 'THERMOSTAT'
TYPE_LOCK = PREFIX_TYPES + 'LOCK'
TYPE_BLINDS = PREFIX_TYPES + 'BLINDS'

SERVICE_REQUEST_SYNC = 'request_sync'
HOMEGRAPH_URL = 'https://homegraph.googleapis.com/'
Expand Down
4 changes: 2 additions & 2 deletions homeassistant/components/google_assistant/smart_home.py
View file
Open in desktop
Expand Up @@ -31,7 +31,7 @@
from . import trait
from .const import (
TYPE_LIGHT, TYPE_LOCK, TYPE_SCENE, TYPE_SWITCH, TYPE_VACUUM,
TYPE_THERMOSTAT, TYPE_FAN, TYPE_CAMERA,
TYPE_THERMOSTAT, TYPE_FAN, TYPE_CAMERA, TYPE_BLINDS,
CONF_ALIASES, CONF_ROOM_HINT,
ERR_FUNCTION_NOT_SUPPORTED, ERR_PROTOCOL_ERROR, ERR_DEVICE_OFFLINE,
ERR_UNKNOWN_ERROR,
Expand All @@ -45,7 +45,7 @@
DOMAIN_TO_GOOGLE_TYPES = {
camera.DOMAIN: TYPE_CAMERA,
climate.DOMAIN: TYPE_THERMOSTAT,
cover.DOMAIN: TYPE_SWITCH,
cover.DOMAIN: TYPE_BLINDS,
fan.DOMAIN: TYPE_FAN,
group.DOMAIN: TYPE_SWITCH,
input_boolean.DOMAIN: TYPE_SWITCH,
Expand Down
84 changes: 70 additions & 14 deletions homeassistant/components/google_assistant/trait.py
View file
Open in desktop
Expand Up @@ -48,6 +48,7 @@
TRAIT_LOCKUNLOCK = PREFIX_TRAITS + 'LockUnlock'
TRAIT_FANSPEED = PREFIX_TRAITS + 'FanSpeed'
TRAIT_MODES = PREFIX_TRAITS + 'Modes'
TRAIT_OPENCLOSE = PREFIX_TRAITS + 'OpenClose'

PREFIX_COMMANDS = 'action.devices.commands.'
COMMAND_ONOFF = PREFIX_COMMANDS + 'OnOff'
Expand All @@ -66,6 +67,7 @@
COMMAND_LOCKUNLOCK = PREFIX_COMMANDS + 'LockUnlock'
COMMAND_FANSPEED = PREFIX_COMMANDS + 'SetFanSpeed'
COMMAND_MODES = PREFIX_COMMANDS + 'SetModes'
COMMAND_OPENCLOSE = PREFIX_COMMANDS + 'OpenClose'

TRAITS = []

Expand Down Expand Up @@ -128,8 +130,6 @@ def supported(domain, features):
"""Test if state is supported."""
if domain == light.DOMAIN:
return features & light.SUPPORT_BRIGHTNESS
if domain == cover.DOMAIN:
return features & cover.SUPPORT_SET_POSITION
if domain == media_player.DOMAIN:
return features & media_player.SUPPORT_VOLUME_SET

Expand All @@ -149,11 +149,6 @@ def query_attributes(self):
if brightness is not None:
response['brightness'] = int(100 * (brightness / 255))

elif domain == cover.DOMAIN:
position = self.state.attributes.get(cover.ATTR_CURRENT_POSITION)
if position is not None:
response['brightness'] = position

elif domain == media_player.DOMAIN:
level = self.state.attributes.get(
media_player.ATTR_MEDIA_VOLUME_LEVEL)
Expand All @@ -173,12 +168,6 @@ async def execute(self, command, data, params):
ATTR_ENTITY_ID: self.state.entity_id,
light.ATTR_BRIGHTNESS_PCT: params['brightness']
}, blocking=True, context=data.context)
elif domain == cover.DOMAIN:
await self.hass.services.async_call(
cover.DOMAIN, cover.SERVICE_SET_COVER_POSITION, {
ATTR_ENTITY_ID: self.state.entity_id,
cover.ATTR_POSITION: params['brightness']
}, blocking=True, context=data.context)
elif domain == media_player.DOMAIN:
await self.hass.services.async_call(
media_player.DOMAIN, media_player.SERVICE_VOLUME_SET, {
Expand Down Expand Up @@ -251,10 +240,10 @@ def supported(domain, features):
return domain in (
group.DOMAIN,
input_boolean.DOMAIN,
cover.DOMAIN,
Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand why we should keep covers in the 'OnOff' trait.

Copy link
Contributor

Choose a reason for hiding this comment

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

Its really just a personal preference kind of thing. I saw the code for it was left in, in which case the tests need to remain, and I kind of like the ability to tell google to turn off everything and not have to switch to close or lock, etc. Can probably be removed if we want to be consistent though and probably should just be handled with the group domain and supported in the cover element of ha.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would agree with @giefca here. If I say turn everything off, one would expect switches and lights to turn off, not curtains to close and locks to lock I think.

switch.DOMAIN,
fan.DOMAIN,
light.DOMAIN,
cover.DOMAIN,
media_player.DOMAIN,
)

Expand Down Expand Up @@ -1047,3 +1036,70 @@ async def execute(self, command, data, params):
ATTR_ENTITY_ID: self.state.entity_id,
media_player.ATTR_INPUT_SOURCE: source
}, blocking=True, context=data.context)


@register_trait
class OpenCloseTrait(_Trait):
"""Trait to open and close a cover.
https://developers.google.com/actions/smarthome/traits/openclose
"""

name = TRAIT_OPENCLOSE
commands = [
COMMAND_OPENCLOSE
]

@staticmethod
def supported(domain, features):
"""Test if state is supported."""
if domain == cover.DOMAIN:
return features & cover.SUPPORT_SET_POSITION
else:
return domain in (cover.DOMAIN)

def sync_attributes(self):
"""Return opening direction."""
return {
'openDirection': ['UP']
Copy link
Contributor

Choose a reason for hiding this comment

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

The 'openDirection' attribute is not required so we might not want to assume the 'Up' value until we plan on implementing that attribute in 'Cover'.

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 are right

Copy link
Member

Choose a reason for hiding this comment

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

This should be based on the device class. If it's not optional, let's leave it out for now.

Copy link

Choose a reason for hiding this comment

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

@giefca
if i want to use up/down can instead of open/close
it it enough to add that line : 'openDirection': ['UP']
line 1050 ?
i prefer to speak with words up/down

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if your cover is closed when it is down you shouldn't have to add anything

Copy link

Choose a reason for hiding this comment

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

i am using the TCP command line cover, i dont know the state at all of my blinds
https://www.home-assistant.io/components/cover.command_line/
just wanted to ask , how to change the code, so i can say up/down
open/close isnt a good dutch translation for us, we dont say open close cover , we say up/down for covers ... so up/down is in dutch (for covers) the way we open / close a cover :)

}

def query_attributes(self):
"""Return state query attributes."""
domain = self.state.domain
response = {}

if domain == cover.DOMAIN:
position = self.state.attributes.get(cover.ATTR_CURRENT_POSITION)
if position is not None:
response['openPercent'] = position
response['on'] = self.state.state != cover.STATE_CLOSED
response['online'] = True
Copy link
Contributor

Choose a reason for hiding this comment

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

HA should be sending the online value on its own I believe

Copy link
Contributor

Choose a reason for hiding this comment

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

I also couldn't find anywhere that mentioned if the 'on' trait actually does what you are thinking here. I think this is more for devices that need to be woken up before they do anything and not for the open/close state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 'on' trait was an old line in early dev and should not be here anymore.
I will correct the online value.

Copy link
Member

Choose a reason for hiding this comment

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

We always add online upstream in our code, this line is not necessary.

else:
response['on'] = self.state.state != cover.STATE_CLOSED
response['online'] = True
Copy link
Member

Choose a reason for hiding this comment

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

drop this line.

if self.state.state != cover.STATE_CLOSED:
response['openPercent'] = 100
else:
response['openPercent'] = 0

return response
giefca marked this conversation as resolved.
Show resolved Hide resolved

async def execute(self, command, data, params):
"""Execute an Open, close, Set position command."""
domain = self.state.domain

if domain == cover.DOMAIN:
if params['openPercent'] == 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

Also we probably don't need to route 'openPercent' 100% and 0% to open and close service calls respectively as they just forward to the set position service anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to route 0% and 100% to open and close service because google assistant always pass 'openPercent' even if no percentage is specified. It's a problem for covers that do not support set_cover_position service.

Copy link
Member

Choose a reason for hiding this comment

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

Does the trait support blinds/garage doors that do not support setting a position?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it does support garage doors, that's the purpose of these conditions. Without routing 0% and 100% to close and open services the trait can't control garage doors. However if a percentage is specified in the voice command, it will call the set_cover_position service even if the cover doesn't support it and the door won't move.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just changed it. Now a garage door will open if closed and close if opened, even if a percentage is specified in the voice command.

await self.hass.services.async_call(cover.DOMAIN, cover.SERVICE_OPEN_COVER, {
giefca marked this conversation as resolved.
Show resolved Hide resolved
ATTR_ENTITY_ID: self.state.entity_id
Copy link

Choose a reason for hiding this comment

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

continuation line missing indentation or outdented

}, blocking=True, context=data.context)
Copy link

Choose a reason for hiding this comment

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

continuation line missing indentation or outdented

elif params['openPercent'] == 100:
await self.hass.services.async_call(cover.DOMAIN, cover.SERVICE_CLOSE_COVER, {
giefca marked this conversation as resolved.
Show resolved Hide resolved
ATTR_ENTITY_ID: self.state.entity_id
Copy link

Choose a reason for hiding this comment

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

continuation line missing indentation or outdented

}, blocking=True, context=data.context)
Copy link

Choose a reason for hiding this comment

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

continuation line missing indentation or outdented

else:
await self.hass.services.async_call(
cover.DOMAIN, cover.SERVICE_SET_COVER_POSITION, {
ATTR_ENTITY_ID: self.state.entity_id,
cover.ATTR_POSITION: params['openPercent']
}, blocking=True, context=data.context)
46 changes: 25 additions & 21 deletions tests/components/google_assistant/__init__.py
View file
Open in desktop
@@ -1,3 +1,5 @@


"""Tests for the Google Assistant integration."""

DEMO_DEVICES = [{
Expand Down Expand Up @@ -28,17 +30,17 @@
'willReportState':
False
}, {
'id':
Copy link
Member

Choose a reason for hiding this comment

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

Please revert this change

'switch.decorative_lights',
'name': {
'name': 'Decorative Lights'
},
'traits': [
'action.devices.traits.OnOff'
],
'type': 'action.devices.types.SWITCH',
'willReportState':
False
'id':
'switch.decorative_lights',
'name': {
'name': 'Decorative Lights'
},
'traits': [
'action.devices.traits.OnOff'
],
'type': 'action.devices.types.SWITCH',
'willReportState':
False
}, {
'id':
'light.ceiling_lights',
Expand Down Expand Up @@ -93,9 +95,9 @@
'name': 'Living Room Window'
},
'traits':
['action.devices.traits.OnOff', 'action.devices.traits.Brightness'],
['action.devices.traits.OnOff','action.devices.traits.OpenClose'],
giefca marked this conversation as resolved.
Show resolved Hide resolved
'type':
'action.devices.types.SWITCH',
'action.devices.types.BLINDS',
'willReportState':
False
}, {
Expand All @@ -105,34 +107,36 @@
'name': 'Hall Window'
},
'traits':
['action.devices.traits.OnOff', 'action.devices.traits.Brightness'],
['action.devices.traits.OnOff','action.devices.traits.OpenClose'],
giefca marked this conversation as resolved.
Show resolved Hide resolved
'type':
'action.devices.types.SWITCH',
'action.devices.types.BLINDS',
'willReportState':
False
}, {
'id': 'cover.garage_door',
'name': {
'name': 'Garage Door'
},
'traits': ['action.devices.traits.OnOff'],
'type': 'action.devices.types.SWITCH',
'traits': ['action.devices.traits.OnOff','action.devices.traits.OpenClose'],
giefca marked this conversation as resolved.
Show resolved Hide resolved
'type':
'action.devices.types.BLINDS',
'willReportState': False
}, {
'id': 'cover.kitchen_window',
'name': {
'name': 'Kitchen Window'
},
'traits': ['action.devices.traits.OnOff'],
'type': 'action.devices.types.SWITCH',
'traits': ['action.devices.traits.OnOff','action.devices.traits.OpenClose'],
giefca marked this conversation as resolved.
Show resolved Hide resolved
'type':
'action.devices.types.BLINDS',
'willReportState': False
}, {
'id': 'group.all_covers',
'name': {
'name': 'all covers'
},
'traits': ['action.devices.traits.OnOff'],
'type': 'action.devices.types.SWITCH',
'traits': ['action.devices.traits.OpenClose'],
'type': 'action.devices.types.BLINDS',
'willReportState': False
}, {
'id':
Expand Down
1 change: 1 addition & 0 deletions tests/components/google_assistant/test_google_assistant.py
View file
Open in desktop
Expand Up @@ -123,6 +123,7 @@ def test_sync_request(hass_fixture, assistant_client, auth_header):
body = yield from result.json()
assert body.get('requestId') == reqid
devices = body['payload']['devices']
print([dev['id'] for dev in devices])
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to remove print from the final version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed

assert (
sorted([dev['id'] for dev in devices])
== sorted([dev['id'] for dev in DEMO_DEVICES]))
Expand Down
96 changes: 29 additions & 67 deletions tests/components/google_assistant/test_trait.py
View file
Open in desktop
Expand Up @@ -83,33 +83,6 @@ async def test_brightness_light(hass):
}


async def test_brightness_cover(hass):
"""Test brightness trait support for cover domain."""
assert trait.BrightnessTrait.supported(cover.DOMAIN,
cover.SUPPORT_SET_POSITION)

trt = trait.BrightnessTrait(hass, State('cover.bla', cover.STATE_OPEN, {
cover.ATTR_CURRENT_POSITION: 75
}), BASIC_CONFIG)

assert trt.sync_attributes() == {}

assert trt.query_attributes() == {
'brightness': 75
}

calls = async_mock_service(
hass, cover.DOMAIN, cover.SERVICE_SET_COVER_POSITION)
await trt.execute(
trait.COMMAND_BRIGHTNESS_ABSOLUTE, BASIC_DATA,
{'brightness': 50})
assert len(calls) == 1
assert calls[0].data == {
ATTR_ENTITY_ID: 'cover.bla',
cover.ATTR_POSITION: 50
}


async def test_brightness_media_player(hass):
"""Test brightness trait support for media player domain."""
assert trait.BrightnessTrait.supported(media_player.DOMAIN,
Expand Down Expand Up @@ -358,46 +331,6 @@ async def test_onoff_light(hass):
}


async def test_onoff_cover(hass):
Copy link
Contributor

Choose a reason for hiding this comment

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

Might want to keep the open & close trait test for cover if we are leaving in the code for it, which I am happy you did btw.

Copy link
Member

@balloob balloob Mar 28, 2019 •
edited

Choose a reason for hiding this comment

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

No. We should remove the on/off functionality for covers completely (as this PR does).

"""Test OnOff trait support for cover domain."""
assert trait.OnOffTrait.supported(cover.DOMAIN, 0)

trt_on = trait.OnOffTrait(hass, State('cover.bla', cover.STATE_OPEN),
BASIC_CONFIG)

assert trt_on.sync_attributes() == {}

assert trt_on.query_attributes() == {
'on': True
}

trt_off = trait.OnOffTrait(hass, State('cover.bla', cover.STATE_CLOSED),
BASIC_CONFIG)

assert trt_off.query_attributes() == {
'on': False
}

on_calls = async_mock_service(hass, cover.DOMAIN, cover.SERVICE_OPEN_COVER)
await trt_on.execute(
trait.COMMAND_ONOFF, BASIC_DATA,
{'on': True})
assert len(on_calls) == 1
assert on_calls[0].data == {
ATTR_ENTITY_ID: 'cover.bla',
}

off_calls = async_mock_service(hass, cover.DOMAIN,
cover.SERVICE_CLOSE_COVER)
await trt_on.execute(
trait.COMMAND_ONOFF, BASIC_DATA,
{'on': False})
assert len(off_calls) == 1
assert off_calls[0].data == {
ATTR_ENTITY_ID: 'cover.bla',
}


async def test_onoff_media_player(hass):
"""Test OnOff trait support for media_player domain."""
assert trait.OnOffTrait.supported(media_player.DOMAIN, 0)
Expand Down Expand Up @@ -1119,3 +1052,32 @@ async def test_modes(hass):
'entity_id': 'media_player.living_room',
'source': 'media'
}


async def test_openclose_cover(hass):
"""Test brightness trait support for cover domain."""
Copy link
Member

Choose a reason for hiding this comment

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

fix comment

assert trait.OpenCloseTrait.supported(cover.DOMAIN,
cover.SUPPORT_SET_POSITION)

trt = trait.OpenCloseTrait(hass, State('cover.bla', cover.STATE_OPEN, {
cover.ATTR_CURRENT_POSITION: 75
}), BASIC_CONFIG)

assert trt.sync_attributes() == {'openDirection': ['UP']}

assert trt.query_attributes() == {
'openPercent': 75,
'on': True,
'online': True
giefca marked this conversation as resolved.
Show resolved Hide resolved
}

calls = async_mock_service(
hass, cover.DOMAIN, cover.SERVICE_SET_COVER_POSITION)
await trt.execute(
trait.COMMAND_OPENCLOSE, BASIC_DATA,
{'openPercent': 50})
assert len(calls) == 1
assert calls[0].data == {
ATTR_ENTITY_ID: 'cover.bla',
cover.ATTR_POSITION: 50
}