Snips sounds by todschmidt · Pull Request #13746 · home-assistant/core

todschmidt · 2018-04-07T19:22:52Z

Description:

Added configuration to enable feedback sounds.
Added probability threshold

Related issue (if applicable): fixes #N/A

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

Example entry for `configuration.yaml` (if applicable):

snips:
  probability_threshold: 0.40
  feedback_sounds: true
  site_ids:
    - default
    - remote

Checklist:

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

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

Documentation added/updated in home-assistant.github.io

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

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:

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

houndci-bot · 2018-04-07T20:25:36Z

+    assert result
+    await hass.async_block_till_done()
+
+    assert len(events) == 1 


trailing whitespace

balloob · 2018-04-07T20:53:32Z

+        for site_id in config[DOMAIN].get(CONF_SITE_IDS):
+            payload = json.dumps({'siteId': site_id})
+            hass.components.mqtt.async_publish(
+                FEEDBACK_ON_TOPIC, None, qos=0, retain=False)


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

balloob · 2018-04-07T20:54:12Z

+        site_ids = ([call.data.get(ATTR_SITE_ID)]
+                    if call.data.get(ATTR_SITE_ID)
+                    else config[DOMAIN].get(CONF_SITE_IDS))
+        for site_id in site_ids:


Extract this into helper methods please.

balloob · 2018-04-07T20:54:34Z

+            hass.components.mqtt.async_publish(
+                FEEDBACK_ON_TOPIC, payload, qos=1, retain=True)
+    else:
+        for site_id in config[DOMAIN].get(CONF_SITE_IDS):


Convert to helper method.

balloob · 2018-04-07T21:20:33Z

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


Shouldn't this be configurable per site id?

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.

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

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.

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

balloob · 2018-04-08T01:06:16Z

+        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)


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

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.

balloob · 2018-04-08T01:07:07Z

    """Activate Snips component."""
-    @asyncio.coroutine
-    def message_received(topic, payload, qos):
+    def set_feedback(site_ids, state):


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

balloob · 2018-04-08T01:07:24Z

+            hass.components.mqtt.async_publish(
+                topic, payload, qos=1, retain=True)
+
+    set_feedback(None, config[DOMAIN].get(CONF_FEEDBACK))


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.

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

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))

balloob · 2018-04-08T01:52:43Z

            return

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


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

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

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

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.

Ah, I see it now. My bad.

balloob · 2018-04-09T03:30:22Z

+            hass.components.mqtt.async_publish(
+                topic, payload, qos=int(state), retain=state)
+
+    if not config[DOMAIN].get(CONF_FEEDBACK, 'no_conf') == 'no_conf':


this is weird… instead try this

if CONF_FEEDBACK in config[DOMAIN]: async_set_feedback(None, config[DOMAIN][CONF_FEEDBACK])

balloob · 2018-04-09T03:33:44Z

+    events = []
+
+    @callback
+    def record_event(event):


We have a helper for this, it's called async_mock_service and can be imported from tests.common

* Added feedback sound configuration * Added feedback sound configuration * Cleaned up feedback off * Cleaned up whitespace * Moved feedback pus to helper funx * Async * Used async_mock_service for tests * Lint

todschmidt added 2 commits April 7, 2018 15:15

Added feedback sound configuration

7bef9db

Added feedback sound configuration

2bb36ba

homeassistant added cla-signed integration: snips labels Apr 7, 2018

todschmidt mentioned this pull request Apr 7, 2018

Update snips.markdown home-assistant/home-assistant.io#5133

Closed

2 tasks

Cleaned up feedback off

11b4dcc

homeassistant added the cla-signed label Apr 7, 2018

houndci-bot reviewed Apr 7, 2018

View reviewed changes

balloob reviewed Apr 7, 2018

View reviewed changes

todschmidt added 2 commits April 7, 2018 17:21

Cleaned up whitespace

fb00907

Moved feedback pus to helper funx

26258a5

balloob reviewed Apr 8, 2018

View reviewed changes

Async

a9caf7e

balloob reviewed Apr 9, 2018

View reviewed changes

todschmidt and others added 2 commits April 9, 2018 09:56

Used async_mock_service for tests

3753a25

Lint

3a597fd

balloob approved these changes Apr 9, 2018

View reviewed changes

balloob merged commit e593117 into home-assistant:dev Apr 9, 2018

balloob mentioned this pull request Apr 27, 2018

0.68 #14124

Merged

home-assistant locked and limited conversation to collaborators Jul 26, 2018

Uh oh!

Conversation

todschmidt commented Apr 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

todschmidt Apr 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

todschmidt Apr 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

todschmidt Apr 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balloob Apr 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

todschmidt commented Apr 7, 2018 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):

todschmidt Apr 8, 2018 •

edited

Loading

todschmidt Apr 8, 2018 •

edited

Loading

todschmidt Apr 9, 2018 •

edited

Loading

balloob Apr 9, 2018 •

edited

Loading