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 Alexa API, fix thermostats #17969

Merged
merged 3 commits into from Oct 30, 2018

Conversation

Projects
None yet
6 participants
@bitglue
Member

bitglue commented Oct 29, 2018

Description:

Includes some significant refactoring, and some bug fixes. For easier review they are done as separate commits. See the individual commit messages for more detail.

fixes #15580, fixes #15183

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
@houndci-bot

Some files could not be reviewed due to errors:

Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/linters/.local/bin/flake8", line 7, in 
    from flake8.main.cli import main
ModuleNotFoundError: No module named 'flake8'

@bitglue bitglue referenced this pull request Oct 29, 2018

Closed

Issue/fix alexa climate device not responding #17950

1 of 3 tasks complete
self.namespace = self._directive[API_HEADER]['namespace']
self.name = self._directive[API_HEADER]['name']
self.payload = self._directive[API_PAYLOAD]
self.has_endpoint = API_ENDPOINT in self._directive

This comment has been minimized.

@balloob

balloob Oct 29, 2018

Member

So for 1 device being multiple entities in Alexa, we should be able to solve that with our new Device Registry too. If integrations are configured using config entries, they can now register to what device they belong.

)
response = directive.error()
except _AlexaError as e:
# TODO does some logging need to happen here?

This comment has been minimized.

@balloob

balloob Oct 29, 2018

Member

I want to attach a logger function to the config object.

@balloob

Looks great!

If only you could have published this 8 hours earlier, you could have saved me some time 😉

@balloob balloob closed this Oct 29, 2018

@wafflebot wafflebot bot removed the in progress label Oct 29, 2018

@balloob balloob reopened this Oct 29, 2018

@wafflebot wafflebot bot added the in progress label Oct 29, 2018

@balloob

This comment has been minimized.

Member

balloob commented Oct 29, 2018

re-opening because travis seems confused

@bitglue bitglue force-pushed the bitglue:alexa_updates branch from 835f94d to 88f89ad Oct 29, 2018

@bitglue bitglue requested a review from home-assistant/core as a code owner Oct 29, 2018

@houndci-bot

Some files could not be reviewed due to errors:

Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/linters/.local/bin/flake8", line 7, in 
    from flake8.main.cli import main
ModuleNotFoundError: No module named 'flake8'

@bitglue bitglue force-pushed the bitglue:alexa_updates branch from 88f89ad to 2c2e876 Oct 29, 2018

@houndci-bot

Some files could not be reviewed due to errors:

Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/linters/.local/bin/flake8", line 7, in 
    from flake8.main.cli import main
ModuleNotFoundError: No module named 'flake8'

@bitglue bitglue force-pushed the bitglue:alexa_updates branch from 2c2e876 to b050f59 Oct 29, 2018

@houndci-bot

Some files could not be reviewed due to errors:

Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/linters/.local/bin/flake8", line 7, in 
    from flake8.main.cli import main
ModuleNotFoundError: No module named 'flake8'
@@ -228,7 +228,10 @@ def async_handle_message(hass, cloud, handler_name, payload):
def async_handle_alexa(hass, cloud, payload):
"""Handle an incoming IoT message for Alexa."""
if not cloud.alexa_enabled:
return alexa.turned_off_response(payload)
# pylint: disable=protected-access
directive = alexa._AlexaDirective(payload)

This comment has been minimized.

@pvizeli

pvizeli Oct 29, 2018

Member

Instead to access a protected function (look like a design error), can we make a public function or helper function for it?

This comment has been minimized.

@bitglue

bitglue Oct 29, 2018

Member

I couldn't think of a use case for doing this outside of this one thing. Seems to me like a reasonable thing to do, as this particular function has a legitimate reason for needing to reach into the low-level details of the Alexa API. More of an exception than the norm.

This comment has been minimized.

@bitglue

bitglue Oct 29, 2018

Member

Alternately, maybe it's better to respond to this request in a way that doesn't require knowledge of the alexa API? Then we avoid any coupling entirely. For example, maybe just an HTTP 404 with a simple text body which links to instructions for enabling alexa cloud or something. I don't use cloud so I don't really know if that would be any different from a UX perspective.

This comment has been minimized.

@balloob

balloob Oct 29, 2018

Member

this is being routed directly back to Alexa, so it needs to be in the Alexa format.

This comment has been minimized.

@bitglue

bitglue Oct 29, 2018

Member

OK, how about we just pass an enabled parameter to async_handle_message. Then smart_home.py doesn't need to expose any low-level details, nor does iot.py need to care.

bitglue added some commits Oct 26, 2018

Refactor Alexa API to use objects for requests
This introduces _AlexaDirective to stand in for the previous model of passing
basic dict and list data structures to and from handlers. This gives a more
expressive platform for functionality common to most or all handlers.

I had two use cases in mind:

1) Most responses should include current properties. In the case of locks and
thermostats, the response must include the properties or Alexa will give the
user a vague error like "Hmm, $device is not responding." Locks currently work,
but thermostats do not. I wanted a way to automatically include properties in
all responses. This is implemented in a subsequent commit.

2) The previous model had a 1:1 mapping between Alexa endpoints and Home
Assistant entities. This works most of the time, but sometimes it's not so
great. For example, my Z-wave thermostat shows as three devices in Alexa: one
for the temperature sensor, one for the heat, and one for the AC. I'd like to
merge these into one device from Alexa's perspective. I believe this will be
facilitated with the `endpoint` attribute on `_AlexaDirective`.
Include properties in all Alexa responses
The added _AlexaResponse class provides a richer vocabulary for handlers.

Among that vocabulary is .merge_context_properties(), which is invoked
automatically for any request directed at an endpoint. This adds all supported
properties to the response as recommended by the Alexa API docs, and in some
cases (locks, thermostats at least) the user will get an error "Hmm, $device is
not responding" if properties are not provided in the response.

@bitglue bitglue force-pushed the bitglue:alexa_updates branch from b050f59 to 083f7f2 Oct 29, 2018

@houndci-bot

Some files could not be reviewed due to errors:

Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/linters/.local/bin/flake8", line 7, in 
    from flake8.main.cli import main
ModuleNotFoundError: No module named 'flake8'

@bitglue bitglue merged commit e167930 into home-assistant:dev Oct 30, 2018

5 checks passed

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

@wafflebot wafflebot bot removed the in progress label Oct 30, 2018

@bitglue bitglue deleted the bitglue:alexa_updates branch Oct 30, 2018

@Thunderbird2086

This comment has been minimized.

Thunderbird2086 commented Oct 31, 2018

@bitglue Thanks for the change.
However, there is bug.
When I said that 'Alexa, set to heat', my AC turned on as heat mode, but Alexa said 'it's off'.

@bitglue bitglue referenced this pull request Oct 31, 2018

Merged

Report correct thermostat mode to Alexa #18053

2 of 2 tasks complete

pvizeli added a commit that referenced this pull request Oct 31, 2018

Report correct thermostat mode to Alexa (#18053)
We were erroneously reporting the _previous_ mode. So if the thermostat was off
and the user asks, "Alexa, set the thermostat to heat", the thermostat would be
set to heat but Alexa would respond, "The thermostat is off."

Bug caught by @Thunderbird2086 at
#17969 (comment)

@balloob balloob referenced this pull request Nov 9, 2018

Merged

0.82 #18335

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment