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

Google assistant: add blinds trait for covers #22336

Merged
merged 30 commits into from Mar 30, 2019

Conversation

@giefca
Copy link
Contributor

commented Mar 23, 2019

Description:

Add support for the Blinds Trait to Google Assistant:
https://developers.google.com/actions/smarthome/traits/openclose

Covers use the blinds trait instead of the onoff and brightness trait. Voice commands are more naturals.
The only downside is that we lose the ability to control covers within the Google Home App.

Related issue (if applicable): fixes #

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

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.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

giefca added some commits Mar 23, 2019

@homeassistant

This comment has been minimized.

Copy link

commented Mar 23, 2019

Hi @giefca,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@homeassistant homeassistant added cla-signed and removed cla-needed labels Mar 23, 2019

giefca added some commits Mar 24, 2019

giefca added some commits Mar 24, 2019

@pergolafabio

This comment has been minimized.

Copy link

commented Mar 24, 2019

i am wondering if you can also initiate a STOP command?

@robbiet480

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

Looks like there are some test issues.

Show resolved Hide resolved tests/components/google_assistant/__init__.py Outdated
Show resolved Hide resolved tests/components/google_assistant/__init__.py Outdated
Show resolved Hide resolved tests/components/google_assistant/__init__.py Outdated
Show resolved Hide resolved tests/components/google_assistant/__init__.py Outdated
Show resolved Hide resolved tests/components/google_assistant/__init__.py Outdated
Show resolved Hide resolved tests/components/google_assistant/__init__.py Outdated
Show resolved Hide resolved tests/components/google_assistant/__init__.py Outdated
Show resolved Hide resolved tests/components/google_assistant/__init__.py Outdated
Show resolved Hide resolved tests/components/google_assistant/__init__.py Outdated
Show resolved Hide resolved tests/components/google_assistant/__init__.py Outdated
@giefca

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

@pergolafabio no the trait doesn't support it

I'm trying to fix it to pass tests. It's the first time I contribute to a project on github. My modification was working well in my environment, I thought it would be easier to submit it.

@giefca giefca force-pushed the giefca:dev branch from c3e8986 to 0567a06 Mar 25, 2019

Show resolved Hide resolved tests/components/google_assistant/__init__.py Outdated
Show resolved Hide resolved tests/components/google_assistant/__init__.py Outdated
Show resolved Hide resolved tests/components/google_assistant/__init__.py Outdated
Show resolved Hide resolved tests/components/google_assistant/__init__.py Outdated
Show resolved Hide resolved homeassistant/components/google_assistant/trait.py Outdated
Show resolved Hide resolved homeassistant/components/google_assistant/trait.py Outdated
@balloob

This comment has been minimized.

Copy link
Member

commented Mar 30, 2019

Looks great ! I will tag it for 0.91 so that people can start using it faster 👍

Marked it as a breaking change because people have to use different voice commands now to control their covers.

@balloob balloob merged commit b04fd08 into home-assistant:dev Mar 30, 2019

4 of 11 checks passed

Build Error Workflow: Build Error
Details
ci/circleci: Build Error Your tests failed on CircleCI
Details
home-assistant.home-assistant Build #20190329.4 failed
Details
home-assistant.home-assistant (Lint) Lint failed
Details
home-assistant.home-assistant (Test Python35) Test Python35 failed
Details
home-assistant.home-assistant (Test Python36) Test Python36 failed
Details
home-assistant.home-assistant (Test Python37) Test Python37 failed
Details
Hound No violations found. Woof!
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on dev at 92.934%
Details

@ghost ghost removed the in progress label Mar 30, 2019

balloob added a commit that referenced this pull request Apr 1, 2019

Google assistant: add blinds trait for covers (#22336)
* Update const.py

* Update smart_home.py

* Update trait.py

* Update test_trait.py

* Update smart_home.py

* Update test_trait.py

* Update trait.py

* Update trait.py

* Update test_trait.py

* Update test_trait.py

* Update __init__.py

* Update test_trait.py

* Change email

* Trying to correct CLA

* Update __init__.py

* Update trait.py

* Update trait.py

* Update trait.py

* Update trait.py

* Update __init__.py

* Update test_trait.py

* Update test_google_assistant.py

* Update trait.py

* Update trait.py

* Update test_trait.py

* Update test_trait.py

@balloob balloob referenced this pull request Apr 3, 2019

Merged

0.91.0 #22688

@dummptyhummpty

This comment has been minimized.

Copy link

commented Apr 4, 2019

@balloob I was happy to use this today, but noticed that the list of supported domains in the docs needs to be updated to reflect that it's no longer using brightness: https://www.home-assistant.io/components/google_assistant/

@pergolafabio

This comment has been minimized.

Copy link

commented Apr 4, 2019

So did you try it already? What alternative words can you say for open/close?

@dummptyhummpty

This comment has been minimized.

Copy link

commented Apr 4, 2019

@pergolafabio Yeah, we're pretty used to saying open and close so that's all we say. You can also say "Hey Google! Set the kitchen shade to 50%." and it will say "Ok, opening the kitchen shade to 50%".

@pergolafabio

This comment has been minimized.

Copy link

commented Apr 4, 2019

i was hoping you could also say up/down , we dont use those open/close in dutch language :)

@michaelarnauts

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

@pergolafabio "open/sluit de zonnewering" is pretty natural for me, and even "doe de zonnewering omhoog" worked fine.

@giefca is there a way so it also works from the Google Home app?

@pergolafabio

This comment has been minimized.

Copy link

commented Apr 4, 2019

@michaelarnauts ah ok interesting , so "zonnewering omhoog" "zonnewering omlaag" is working?
dont think its working from the home app 'yet' , its a feature request : actions-on-google/smart-home-nodejs#302

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Apr 4, 2019

@balloob

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

Chat on the forums please 👍

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.