Skip to content

Snips sounds #13746

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

Merged
balloob merged 8 commits into from
Apr 9, 2018
Merged

Snips sounds #13746

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7bef9db
Added feedback sound configuration
todschmidt Apr 7, 2018
2bb36ba
Added feedback sound configuration
todschmidt Apr 7, 2018
11b4dcc
Cleaned up feedback off
todschmidt Apr 7, 2018
fb00907
Cleaned up whitespace
todschmidt Apr 7, 2018
26258a5
Moved feedback pus to helper funx
todschmidt Apr 7, 2018
a9caf7e
Async
todschmidt Apr 9, 2018
3753a25
Used async_mock_service for tests
todschmidt Apr 9, 2018
3a597fd
Lint
balloob Apr 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 58 additions & 13 deletions homeassistant/components/snips.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
For more details about this component, please refer to the documentation at
https://home-assistant.io/components/snips/
"""
import asyncio
import json
import logging
from datetime import timedelta
Expand All @@ -19,11 +18,18 @@

CONF_INTENTS = 'intents'
CONF_ACTION = 'action'
CONF_FEEDBACK = 'feedback_sounds'
CONF_PROBABILITY = 'probability_threshold'
CONF_SITE_IDS = 'site_ids'

SERVICE_SAY = 'say'
SERVICE_SAY_ACTION = 'say_action'
SERVICE_FEEDBACK_ON = 'feedback_on'
SERVICE_FEEDBACK_OFF = 'feedback_off'

INTENT_TOPIC = 'hermes/intent/#'
FEEDBACK_ON_TOPIC = 'hermes/feedback/sound/toggleOn'
FEEDBACK_OFF_TOPIC = 'hermes/feedback/sound/toggleOff'

ATTR_TEXT = 'text'
ATTR_SITE_ID = 'site_id'
Expand All @@ -34,7 +40,12 @@
_LOGGER = logging.getLogger(__name__)

CONFIG_SCHEMA = vol.Schema({
DOMAIN: {}
DOMAIN: vol.Schema({
vol.Optional(CONF_FEEDBACK, default=False): cv.boolean,

@balloob balloob Apr 7, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shouldn't this be configurable per site id?

@todschmidt todschmidt Apr 7, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Figured that was enough of an edge case it wasn't worth complicating the config for. They can turn on or off a site via the service but I can't imagine there are many cases where anyone wants to do that.

@balloob balloob Apr 7, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it make sense to limit this functionality to just the service? You don't need to change it all the time?

@todschmidt todschmidt Apr 7, 2018

Copy link
Copy Markdown
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 think so, it's pretty minor to add it and I think a lot of users will like to set it here, especially considering snips doesn't have the option yet.

@todschmidt todschmidt Apr 7, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Especially for something like hassio where you reboot and it's nice to have it back on if you want it.

vol.Optional(CONF_PROBABILITY, default=0): vol.Coerce(float),
vol.Optional(CONF_SITE_IDS, default=['default']):
vol.All(cv.ensure_list, [cv.string]),
}),
}, extra=vol.ALLOW_EXTRA)

INTENT_SCHEMA = vol.Schema({
Expand All @@ -57,21 +68,36 @@
vol.Optional(ATTR_SITE_ID, default='default'): str,
vol.Optional(ATTR_CUSTOM_DATA, default=''): str
})

SERVICE_SCHEMA_SAY_ACTION = vol.Schema({
vol.Required(ATTR_TEXT): str,
vol.Optional(ATTR_SITE_ID, default='default'): str,
vol.Optional(ATTR_CUSTOM_DATA, default=''): str,
vol.Optional(ATTR_CAN_BE_ENQUEUED, default=True): cv.boolean,
vol.Optional(ATTR_INTENT_FILTER): vol.All(cv.ensure_list),
})
SERVICE_SCHEMA_FEEDBACK = vol.Schema({
vol.Optional(ATTR_SITE_ID, default='default'): str
})


@asyncio.coroutine
def async_setup(hass, config):
async def async_setup(hass, config):
"""Activate Snips component."""
@asyncio.coroutine
def message_received(topic, payload, qos):
def set_feedback(site_ids, state):

@balloob balloob Apr 8, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since this is called from inside async context, plase add @callback decorator and prefix name with async_

"""Set Feedback sound state."""
site_ids = (site_ids if site_ids
else config[DOMAIN].get(CONF_SITE_IDS))
topic = (FEEDBACK_ON_TOPIC if state
else FEEDBACK_OFF_TOPIC)
for site_id in site_ids:
payload = json.dumps({'siteId': site_id})
hass.components.mqtt.async_publish(
FEEDBACK_ON_TOPIC, None, qos=0, retain=False)

@balloob balloob Apr 7, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What are you doing here? Is this to clear the retained message?

@todschmidt todschmidt Apr 7, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes

@balloob balloob Apr 8, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shouldn't you clear the retained message on the off topic if setting the on topic?

@todschmidt todschmidt Apr 8, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Off isn't set to be retained since that is the default if snips restarts Edit: Doh, I see what you mean I didn't have it set before, fixed.

hass.components.mqtt.async_publish(
topic, payload, qos=1, retain=True)

set_feedback(None, config[DOMAIN].get(CONF_FEEDBACK))

@balloob balloob Apr 8, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think this should have a default. If no value is given, we should not send any messages. That way it can be controlled by the outside.

@todschmidt todschmidt Apr 8, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, was back and forth there and could see it either way but ok.

@todschmidt todschmidt Apr 8, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK, there has to be a better way to do this. I think it should be if nothing is configured we don't send anything, if false is configured we do. But this seems hacky but how to test for not set?

    if not config[DOMAIN].get(CONF_FEEDBACK, 'no_conf') == 'no_conf':
        async_set_feedback(None, config[DOMAIN].get(CONF_FEEDBACK))


async def message_received(topic, payload, qos):
"""Handle new messages on MQTT."""
_LOGGER.debug("New intent: %s", payload)

Expand All @@ -81,6 +107,13 @@ def message_received(topic, payload, qos):
_LOGGER.error('Received invalid JSON: %s', payload)
return

if (request['intent']['probability']
< config[DOMAIN].get(CONF_PROBABILITY)):

@balloob balloob Apr 8, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

default for probability is 0. So if none is configured, nothing will ever pass? Better to not have a probability default and just check if probability is None or intentProb < probability

@todschmidt todschmidt Apr 8, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Probability (editL was: default) is checked for greater than, so 0 matches all. It's a lower limit not upper.

@balloob balloob Apr 9, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The config option defaults to 0.
Snips probability is between 0 and 1.

If not configured, probability defaults to 0.

So you're checking here things like if 0.8 < 0. Which will always be false?

Also, if you know a key exists, prefer to use square brackets over get

@todschmidt todschmidt Apr 9, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

0.8 < 0 is false yes, so we accept the intent. If I set the default to 1 then we would by default not accept any intents.

@balloob balloob Apr 9, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, I see it now. My bad.

_LOGGER.warning("Intent below probaility threshold %s < %s",
request['intent']['probability'],
config[DOMAIN].get(CONF_PROBABILITY))
return

try:
request = INTENT_SCHEMA(request)
except vol.Invalid as err:
Expand All @@ -97,7 +130,7 @@ def message_received(topic, payload, qos):
slots[slot['slotName']] = {'value': resolve_slot_values(slot)}

try:
intent_response = yield from intent.async_handle(
intent_response = await intent.async_handle(
hass, DOMAIN, intent_type, slots, request['input'])
if 'plain' in intent_response.speech:
snips_response = intent_response.speech['plain']['speech']
Expand All @@ -115,11 +148,10 @@ def message_received(topic, payload, qos):
mqtt.async_publish(hass, 'hermes/dialogueManager/endSession',
json.dumps(notification))

yield from hass.components.mqtt.async_subscribe(
await hass.components.mqtt.async_subscribe(
INTENT_TOPIC, message_received)

@asyncio.coroutine
def snips_say(call):
async def snips_say(call):
"""Send a Snips notification message."""
notification = {'siteId': call.data.get(ATTR_SITE_ID, 'default'),
'customData': call.data.get(ATTR_CUSTOM_DATA, ''),
Expand All @@ -129,8 +161,7 @@ def snips_say(call):
json.dumps(notification))
return

@asyncio.coroutine
def snips_say_action(call):
async def snips_say_action(call):
"""Send a Snips action message."""
notification = {'siteId': call.data.get(ATTR_SITE_ID, 'default'),
'customData': call.data.get(ATTR_CUSTOM_DATA, ''),
Expand All @@ -144,12 +175,26 @@ def snips_say_action(call):
json.dumps(notification))
return

async def feedback_on(call):
"""Turn feedback sounds on."""
set_feedback(call.data.get(ATTR_SITE_ID), True)

async def feedback_off(call):
"""Turn feedback sounds off."""
set_feedback(call.data.get(ATTR_SITE_ID), False)

hass.services.async_register(
DOMAIN, SERVICE_SAY, snips_say,
schema=SERVICE_SCHEMA_SAY)
hass.services.async_register(
DOMAIN, SERVICE_SAY_ACTION, snips_say_action,
schema=SERVICE_SCHEMA_SAY_ACTION)
hass.services.async_register(
DOMAIN, SERVICE_FEEDBACK_ON, feedback_on,
schema=SERVICE_SCHEMA_FEEDBACK)
hass.services.async_register(
DOMAIN, SERVICE_FEEDBACK_OFF, feedback_off,
schema=SERVICE_SCHEMA_FEEDBACK)

return True

Expand Down
Loading