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

Support templating MQTT triggers #45614

Merged
merged 15 commits into from
Feb 8, 2021

Conversation

emontnemery
Copy link
Contributor

@emontnemery emontnemery commented Jan 27, 2021

Proposed change

Support templating MQTT triggers

The main driver is to allow blueprint inputs to define an MQTT topic or payload to trigger on.

Introduce limited templates which don't have any access to the state dependent template filters and functions such as states etc.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example automation:

Example where MQTT trigger's topic is templated.

trigger_variables:
  page: !input page
  object: !input object
  target_entity: !input target_entity
  haspentity: !input plate
  node: {{ haspentity.split(".")[1].split("_touch")[0] }}
  state_topic: '{{ "hasp/" ~ node ~ "/state/p" ~ page ~ "b" ~ object ~ "" }}'
trigger:
  - platform: mqtt
    topic: '{{ state_topic }}'
  - platform: state
    entity_id: !input target_entity
    attribute: brightness
  - platform: state
    entity_id: !input plate
    attribute: status
    from:
    to: 'available'

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

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

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

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@probot-home-assistant
Copy link

Hey there @home-assistant/core, mind taking a look at this pull request as its been labeled with an integration (mqtt) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

@probot-home-assistant
Copy link

Hey there @home-assistant/core, mind taking a look at this pull request as its been labeled with an integration (automation) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

_LOGGER.warning(
"Use of '%s' is not supported in limited templates", name
)
return ""
Copy link
Member

Choose a reason for hiding this comment

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

Should it raise?

Copy link
Contributor Author

@emontnemery emontnemery Jan 27, 2021

Choose a reason for hiding this comment

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

Not sure, maybe better to raise to entirely block the template from rendering?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to break it and not have it attach the trigger.

Copy link
Member

Choose a reason for hiding this comment

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

Because triggers are a pain to debug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to raise, it gets really noisy though:

2021-01-27 20:52:58 ERROR (MainThread) [homeassistant.components.automation.hasp_control_slider_object] Error setting up trigger HASP Control Slider Object
Traceback (most recent call last):
  File "/mnt/d/development/github/home-assistant_fork/homeassistant/helpers/template.py", line 357, in async_render
    render_result = compiled.render(kwargs)
  File "/mnt/d/development/github/home-assistant_fork/venv/lib/python3.8/site-packages/jinja2/environment.py", line 1090, in render
    self.environment.handle_exception()
  File "/mnt/d/development/github/home-assistant_fork/venv/lib/python3.8/site-packages/jinja2/environment.py", line 832, in handle_exception
    reraise(*rewrite_traceback_stack(source=source))
  File "/mnt/d/development/github/home-assistant_fork/venv/lib/python3.8/site-packages/jinja2/_compat.py", line 28, in reraise
    raise value.with_traceback(tb)
  File "<template>", line 1, in top-level template code
  File "/mnt/d/development/github/home-assistant_fork/venv/lib/python3.8/site-packages/jinja2/sandbox.py", line 462, in call
    return __context.call(__obj, *args, **kwargs)
  File "/mnt/d/development/github/home-assistant_fork/homeassistant/helpers/template.py", line 1366, in warn_unsupported
    raise TemplateError(f"Use of '{name}' is not supported in limited templates")
homeassistant.exceptions.TemplateError: str: Use of 'states' is not supported in limited templates

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/mnt/d/development/github/home-assistant_fork/homeassistant/components/mqtt/trigger.py", line 47, in async_attach_trigger
    topic = topic.async_render(variables, limited=True)
  File "/mnt/d/development/github/home-assistant_fork/homeassistant/helpers/template.py", line 359, in async_render
    raise TemplateError(err) from err
homeassistant.exceptions.TemplateError: TemplateError: str: Use of 'states' is not supported in limited templates

Copy link
Member

Choose a reason for hiding this comment

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

We should catch HomeAssistantError and then not print the stack trace. That will help :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, fixed in eeac975

@aderusha
Copy link

aderusha commented Feb 4, 2021

I'm not sure if this is the place to do this, but... is it possible to extend this concept to other triggers?

I'm working on the HASP project mentioned in the example used above. As shown in the example, there are several trigger conditions which are derived from the input entity name. Like the MQTT trigger example used, these do not rely on template evaluations that would ever change (due to state or whatever), the result of the template is 100% determined by the inputs selected when the user deploys the template.

For a concrete example: the HASP project makes use of a set of 4 MQTT lights which are used as a way to store 4x RGB values and to provide the user a front-end UI to select colors. Currently, I am using MQTT to trigger on the state of those values. For reasons, those values are retained, which means I'm doing a lot of triggering on automation reload etc. What would work much better would be triggering on the entity itself, like this:

trigger_variables:
  haspentity: !input plate
  node: {{ haspentity.split(".")[1].split("_touch")[0] }}
  foregroundentity: {{ "light." ~ node ~ "_foreground }}
trigger:
  - platform: state
    entity_id: foregroundentity

@emontnemery
Copy link
Contributor Author

emontnemery commented Feb 4, 2021

A comment on the particular example:
There's no guarantee that the entity_id is fixed, the user is free to change it, or it may be already reserved so homeassistant will make up a new one <entity_id>-n.

If your problem is that the MQTT messages are retained, then don't retain them :)

On a more general note, some additional templating of other triggers than MQTT seems useful thanks to blueprints.
In particular event triggers, as discussed here: home-assistant/architecture/issues/466

@aderusha
Copy link

aderusha commented Feb 4, 2021

That brings up another problem w/r/t templates in blueprints. As you've correctly noted, we are making some assumptions about the related entities based on the entity name. Those assumptions may be invalid for the reasons you mention. The HASP device utilizes MQTT discovery to create a set of entities that are linked to a common device. What would be best would be if we could have the user select the device, and then via templates find the associated entities.

To my knowledge, the template engine has no ability to turn a device_id into an entity name. That would be required in order to accomplish the MQTT trigger scheme outlined in the original example you provided if we were selecting based on the device instead of one of the generated entities. If there was some means of find entities related to a selected device, there should be no ambiguity of the resulting entity names but... that then puts us back into the position of needing to be able to use those names as triggers for not-MQTT-related things.

So, until we have some way to select a device and turn that device into entity names, we're instead having the user select one of the associated entities, stripping a device name out of the entity, and then running with that for MQTT topics.

Finally, thanks so much for your work on this and also congratulations on the new job!

@Adminiuga
Copy link
Contributor

@emontnemery can you please help me understand a bit, how this would work? Paulus redirected me to this PR for my use case and frankly my understanding of how "templating" is implemented under the hood is still lacking.
With your PR, in blueprint definition does this

trigger:
  - platform: mqtt
    topic: '{{ state_topic }}'

becomes a "static" topic once the automation is instantiated? i.e. the trigger for automation is no longer a template?

To provide some context to my question, I have the following use case:
in a regular automation we could define a "template" trigger like

automation:
  - id: some_id
    trigger:
      - platform: state
        entity_id: input_boolean.toggle_1
        to: "on"
        for: "{{ states('input_datetime.max_on_time') }}"
     action:
 ...

and I just wanted the input_datetime.max_on_time to be a blue print !input but even with this PR it doesn't work and I'm trying to understand why?
In other words, I need something like a "pre-processor", where trigger templates are kept as templates, but only "inputs" are evaulated/processed?

@Adminiuga
Copy link
Contributor

@balloob any reason we shouldn't expose trigger_variables to state trigger templating? e.g. with the following changes

--- a/homeassistant/components/homeassistant/triggers/state.py
+++ b/homeassistant/components/homeassistant/triggers/state.py
@@ -6,7 +6,13 @@ from typing import Any, Dict, Optional
 import voluptuous as vol
 
 from homeassistant import exceptions
-from homeassistant.const import CONF_ATTRIBUTE, CONF_FOR, CONF_PLATFORM, MATCH_ALL
+from homeassistant.const import (
+    CONF_ATTRIBUTE,
+    CONF_FOR,
+    CONF_PLATFORM,
+    CONF_VARIABLES,
+    MATCH_ALL,
+)
 from homeassistant.core import CALLBACK_TYPE, HassJob, HomeAssistant, State, callback
 from homeassistant.helpers import config_validation as cv, template
 from homeassistant.helpers.event import (
@@ -151,6 +157,8 @@ async def async_attach_trigger(
                 "to_state": to_s,
             }
         }
+        if automation_info:
+            variables.update(automation_info.get(CONF_VARIABLES, {}))
 
         try:
             period[entity] = cv.positive_time_period(

My use case of the following blueprint works as prefer it to work:

input:
  max_on_time:
    name: Max time to run
    description: Maximum time to run when turned on automatically
    selector:
      entity:
        domain: input_datetime

trigger_variables:
  trigger_variables:
  max_on_time_entity: !input "max_on_time"

trigger:
  - platform: state
    entity_id: !input "fan_switch"
    to: "on"
    for: "{{ states(max_on_time_entity) }}"

?

I don't mean it to be the part of this PR, was going to submit another PR if this approach is approved.
Haven't looked into other trigger types, but perhaps numeric_value trigger could benefit from the same.

@emontnemery
Copy link
Contributor Author

@aderusha:

we're instead having the user select one of the associated entities, stripping a device name out of the entity, and then running with that for MQTT topics

That seems a little bit weird, shouldn't your automation work with the entity's state or services, which will then convert to and from MQTT messages?

@Adminiuga What you propose is being prepared, but we need this PR to be merged first to have all the plumbing in place.

@@ -61,6 +61,14 @@ def valid_subscribe_topic(value: Any) -> str:
return value


def valid_subscribe_topic_template(value: Any) -> Any:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def valid_subscribe_topic_template(value: Any) -> Any:
def valid_subscribe_topic_template(value: Any) -> Union[str, template.Template]:

Is it a good idea to return different types, especially since Template has an optimization if it's static and so has hardly any overhead?

Copy link
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

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

🎉 🎉 🎉 🎉 🎉

@emontnemery emontnemery merged commit 047f167 into home-assistant:dev Feb 8, 2021
**kwargs: Any,
) -> Any:
"""Render given template.

This method must be run in the event loop.

If limited is True, the template is not allowed to access any function or filter depending on hass or the state machine.
Copy link
Member

Choose a reason for hiding this comment

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

It's nice to break long strings around 88 characters.

@aderusha
Copy link

aderusha commented Feb 8, 2021

we're instead having the user select one of the associated entities, stripping a device name out of the entity, and then running with that for MQTT topics

That seems a little bit weird, shouldn't your automation work with the entity's state or services, which will then convert to and from MQTT messages?

The HASP project makes extensive use of MQTT-discovered Home Assistant entities in order to allow for "native" controls and interfaces so users can use home-assistant constructs to interact with the device. On boot, the device publishes discovery messages for a binary sensor (of class "connectivity", used to track if the HASP is online and connected to Home Assistant), a light object (to control the backlight on/off/dim), a sensor object (stores info about the HASP, IP address, if an update is available, etc), and 4 RGB lights which are used for theme support (these are really handy as they allow the user to interact with colors in a home assistant-friendly way).

So, one HASP device == 7 entities, all connected via the device registry. The Home Assistant UI offers a really nice way to pull all this stuff together. However, to the best of my knowledge, there is no way to expose these relationships via templates. Let's say I have an automation which displays something on the screen. I want that automation to implement themes, so I need to figure out if the binary_sensor is available (don't send messages to a HASP that isn't online), and then send each of the 4 RGB light values to each of the 4 theme elements. To do this, I'm making the (poor) assumption that everything has entity names that line up. I'd really rather not do this, but the alternative is either A) making the user select 5 different entities with every blueprint they deploy, or B) figuring out a way, via blueprints or templates or whatever, to have the user select one of the devices and then somehow leverage the device registry to identify associated devices. Currently, I don't think this is possible via automations, and the "select the correct 5 things in order" hasn't tested out great with users.

So... that's why we're doing what we're doing. And also why I'm completely derailing this PR thread! Sorry everyone :D

@emontnemery
Copy link
Contributor Author

@aderusha let's stop derailing this PR thread then :)
Please ping me on discord instead, I'm @emontnemery there too.

@github-actions github-actions bot locked and limited conversation to collaborators Feb 9, 2021
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.

6 participants