-
-
Notifications
You must be signed in to change notification settings - Fork 28.6k
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
Completely local control of entities via Alexa #2942
Conversation
Woah, was not expecting this! Thanks for contributing. There's a few structural issues which i'll point out in line comments momentarily. |
"""Activate the alexa_local_control component.""" | ||
from flask import Flask | ||
|
||
app = Flask(__name__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a specific reason you are using Flask over the built in http
component? http
should provide everything you need, like complex URL routing. I don't see any other requirements that you are using Flask for that http
wouldn't be able to handle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a look at the http
component and I didn't see the ability to run on a separate port, which is required by the Hue bridge API as far as I can tell, because integrating with the HASS API isn't possible as some of the endpoints overlap (again, as far as I can tell). I also chose Flask because the original code in ha-local-echo used it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@balloob will need to weigh in. I think that at a minimum we would want to use something lighter weight than Flask. At most, I believe we should support either multiple http
(one for each port) or even better, a workaround to route requests based on User-Agent or specific IP address, so that we can have a single HTTP which serves the existing routes as well as the Hue routes, but only serve the Hue routes to Alexa.
This is going to need tests. |
Yep, I agree! It's reasonably complex, so I'll add some tests over the next day or so when I have time to do so. |
""" | ||
Support for local control of entities through Alexa. | ||
|
||
A sample configuration entry is given below: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This entire block needs to be removed and put into docs instead, including the license.
"""
Support for local control of entities through Alexa.
For more details about this component, please refer to the documentation at
https://home-assistant.io/components/alexa_local_control/
"""
I'd like to come up with a better name for this. Maybe just |
We also need to be mindful of the Alexa/Hue limit to 49 devices total. UPDATE: It's 49 devices per hub. It would be extra special extra credit, but you could fake multiple hubs if you have more than 49 entities exposed. |
Indeed. I'm not sure what the best way to go about this is, especially since the ability to expose entities by default is there (how do you choose which ones to expose by default if the user has >49 devices?). My first thought would be to expose manually marked ones first, then arbitrarily choose default-exposed entities until we reach 49, but that obviously isn't particularly consistent. Telling users they have to manually mark which entities to expose if they exceed 49 exposed-by-default entities could also work, but that's not very user friendly. Thoughts from others? |
I'd rather just have users manually specific up to 49 devices to expose. We also should filter out real Hue devices by default to remove duplicates. I believe there's a function in core or loader that lets you determine what platform created an entity. That check should be run against all entities in the |
|
||
""" | ||
|
||
DESCRIPTION_XML_RESPONSE = """<?xml version="1.0" encoding="UTF-8" ?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move this to line 282 since it's not a constant that other components would need to get at.
I think that |
Thinking more about the overlapping routes issue, I do think it would be best to only accept requests from the Alexa On a similar topic of the component name, I think it might be better to remove the Alexa branding from this, since I could see a bunch of different use cases for this, like with the new Logi Pop buttons. Maybe just name this |
|
||
# Note that the double newline at the end of | ||
# this string is required per the SSDP spec | ||
resp_template = """HTTP/1.1 200 OK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to do this with the built in Flask/http
tools. I believe this is just a list of a bunch of headers and the 200 code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The UPNP listener needs special socket options to listen to multicast packets, which I don't believe Flask or http
supports.
We should not use Flask. Instead, use the internal HTTP framework. To initialize a web server, use this code: https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/http.py#L112-L133 Once you have a server, you can register views on it, like you would in normal Home Assistant land. You can pass a shared dictionary as constructor to the views so that they share the token registry. |
requires_auth = False | ||
|
||
def __init__(self, hass): | ||
"""Initializ the instance of the view.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initialize
Let's rename this to |
|
||
|
||
# pylint: disable=too-few-public-methods | ||
class Config(object): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine, but I assume it's needed because PyLint complained about too many locals or something like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Config class is mainly so I can share the parsed configuration between the few views I have (I assume that's what you're referring to).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that works too, thanks for clarifying :)
if 'devicetype' not in data: | ||
return self.Response("devicetype not specified", status=400) | ||
|
||
json_response = [{'success': {'username': '12345678901234567890'}}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we only ever allow one username why not make the /api/<username>/lights
route /api/12345678901234567890/lights
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose we could do that, this method itself is a holdover from ha-local-echo, I could also just generate a random string, which would be more in line with how the real Hue bridge works, based on my reading the Hue API docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, we can leave it as is in the interest of getting this merged before the cutoff. However, in the future, it should randomly generate a string and store that string in a flat file so that only actually authorized usernames can get through.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that mean we'll want to emulate how the real Hue bridge works with regards to authentication, where, in order to get a username, the user has to click a (virtual) button in the HASS interface in a specified timeout? The real bridge requires you to hit a physical button on the bridge when a consumer of the API wants a username.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No we don't have to go that far, but I do want a way for someone to be able to remove a username from that flat file to disable it if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All right, sounds good. I'll be sure to make a note of doing that after merging the initial code.
"""Determine if an entity should be exposed on the emulated bridge.""" | ||
config = self.config | ||
|
||
if ('view' in entity.attributes) and (entity.attributes['view']): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about
if entity.attributes.get('view') is not None:
# Ignore entities that are views
return False
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that should work, another holdover from ha-local-echo I didn't change.
Once all my line comments are fixed up this should be good to merge, but I want a second pair of eyes to look so @Teagan42 will be later tonight. It also still needs docs. |
Great! I just pushed commits that should fix everything but the username stuff, and I'm in the process of writing documentation now, should be done fairly soon. |
I created a PR for home-assistant.github.io: home-assistant/home-assistant.io#845. Please let me know if anything needs to be changed or added; I imagine the sidebar is one thing that needs to be looked at. |
btw ping @blocke, just want to make sure he knows this is being added :) |
# Get whether or not entities should be exposed by default, or if only | ||
# explicitly marked ones will be exposed | ||
self.expose_by_default = conf.get(CONF_EXPOSE_BY_DEFAULT) | ||
if not isinstance(self.expose_by_default, bool): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.expose_by_default = conf.get(CONF_EXPOSE_BY_DEFAULT, DEFAULT_EXPOSE_BY_DEFAULT
Thanks! 🐬 🍪 💯 |
@mgbowen So a few things to improve this component now that it's merged:
def map_brightness_to_speed(self, brightness: int):
"""Map a 0-255 brightness value to speed."""
if brightness > 0 and brightness < 85:
return SPEED_LOW
elif brightness > 85 and brightness < 170:
return SPEED_MED
elif brightness > 170 and brightness < 255:
return SPEED_HIGH I've got nothing else for the moment, it's working fantastically so far! Thanks for your hard work on this!!! |
Sounds good. I'm moving apartments this weekend, so I might not be able to get to it for a bit, but I'll try hopefully before 0.28!
|
@mgbowen For sure, thank you for your hard work. +1 to @robbiet480 suggestion. I've protected the API via an iptables rules to allow only the HASS IP and Alexa to connect it, but would be nice to have an authorization set for it. I also bought to test an Osram Lightify Garden lights and when using the emulated_hue via voice command it works (it toggle the lights on/off), however Alexa returns an error message It is working for other platform and devices. I'll dig into to it a little bit later this week. Thank you again! |
Just a heads up that I updated my list. |
Description:
This PR adds the ability to natively discover and control devices via Alexa (Amazon Echo, Dot, or Tap). It accomplishes this by emulating a Philips Hue bridge, which Alexa has native, local support for, presenting entities as lights. Entities that can be turned on (or off) or have their brightness set are supported; this includes lights, scripts, and other devices. It also allows for exposing entities in configurable domains by default, or for only exposing entities that are specifically marked.
This will complete this Pivotal story.
A lot of this code is based off ha-local-echo. Thanks to Bruce Locke!
Python is not a language I use often, and this is my first foray into developing for Home Assistant, so please let me know if there's anything that can be improved! I also have not gotten around to writing tests for this, but I can in the next few days. I can also add documentation to home-assistant.io over the next few days.
Example entry for
configuration.yaml
(if applicable):PR for home-assistant.github.io: home-assistant/home-assistant.io#845
Checklist:
If user exposed functionality or configuration variables are added/changed:
If code communicates with devices, web services, or a:
tox
run successfully. Your PR cannot be merged unless tests passREQUIREMENTS
variable (example).requirements_all.txt
by runningscript/gen_requirements_all.py
..coveragerc
.If the code does not interact with devices:
tox
run successfully. Your PR cannot be merged unless tests pass