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

Adding RoomHinting to GoogleAssistant to allow for room annotations. #12598

Merged
merged 1 commit into from
Feb 22, 2018
Merged

Adding RoomHinting to GoogleAssistant to allow for room annotations. #12598

merged 1 commit into from
Feb 22, 2018

Conversation

jeremydk
Copy link
Contributor

@jeremydk jeremydk commented Feb 22, 2018

Description:

[Not for merge yet] Added the roomHint attribute to the devices return for the QUERY intent for Google Assistant. According to the documentation available from Google, this will allow disambiguation between devices with similar names based on a provided room. While this can currently be achieved within the Google Assistant application by associating exposed devices into a "Room", each user needs to do this locally on their app.

However, the attribute doesn't appear to be lit up yet on the Google side, even though it's in the documentation. I've reached out to developer support at this point, and I'm waiting to hear back. I'll update the PR as required based on what I hear back.

Submitting at this point in order to allow for review.

Docs from Google:

https://developers.google.com/actions/smarthome/create-app

roomHint: String. Optional. If the partner's cloud configuration includes placing devices in rooms, the name of the room can be provided here; the Assistant will attempt to align the room with those in the HomeGraph.

https://developers.google.com/actions/smarthome/faq

Q: Do device names need to be unique?

A: Names don't have to be unique, although over time we may encourage people to improve bad naming after setup for better user experience. If a user has two lamps in your house called “lamp”, where one has roomHint as “bedroom”, the other as “living room”, saying “OK Google, turn on the living room lamp” will only turn on the one that’s in the living room.

Related issue (if applicable): fixes #

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io#

Docs pending. Holding off for any required changes first.

Example entry for configuration.yaml (if applicable):

entity_config:
  light.bedroom_lamp:
    expose: True
    room: bedroom
    name: Lamp

Checklist:

  • [ x] The code change is tested and works locally.

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

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

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

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.

@homeassistant
Copy link
Contributor

Hi @jeremydk,

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!

@jeremydk jeremydk changed the title [No Merge] Adding RoomHinting to GoogleAssistant to allow for room annotations. [WIP] Adding RoomHinting to GoogleAssistant to allow for room annotations. Feb 22, 2018
@OttoWinter
Copy link
Member

While I do understand the need for this, I don't think we should put the room: config item in the customize: section. First, it might seem like a good idea to put it there, because it might also be used by other components. But in the end 1. I think this would only be used by Google Assistant anyway and 2. if users see the room: in customize:, they might think Home Assistant does some magic and uses room: for example for grouping entities in the front-end, which it does not.

I think the entity_config section of google_assistant would be more appropriate for room:.

@jeremydk
Copy link
Contributor Author

I came to the same conclusion, and the room annotation is in the entity_config section for google_assistant currently. I apologise if that wasn't clear from my, admittedly very trimmed, yaml sample.

@OttoWinter
Copy link
Member

OttoWinter commented Feb 22, 2018

Oh, I'm very very sorry about that and I want to apologize. I should have seriously checked the code before posting the comment. ❤️

Copy link
Contributor

@philk philk left a comment

Choose a reason for hiding this comment

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

IIRC they say somewhere in the docs that roomHint doesn't do anything yet (which, aside from laziness, is why I didn't bother adding it) but I'm still totally fine with this. Thanks!

(Don't forget the doc PR)

@balloob balloob changed the title [WIP] Adding RoomHinting to GoogleAssistant to allow for room annotations. Adding RoomHinting to GoogleAssistant to allow for room annotations. Feb 22, 2018
@balloob balloob merged commit f899ce8 into home-assistant:dev Feb 22, 2018
@balloob balloob mentioned this pull request Feb 22, 2018
@jeremydk
Copy link
Contributor Author

Thanks -- I'll call out in the documentation that this isn't currently enabled on the Google side -- Support got back to me and confirmed that my calls are correct, and it's an issue on their end.

@jeremydk jeremydk deleted the roomHint branch February 23, 2018 18:41
@home-assistant home-assistant locked and limited conversation to collaborators May 29, 2018
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.

5 participants