-
-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
Add websock command to query device for triggers #24044
Conversation
})) | ||
|
||
|
||
async def async_trigger(hass, config, action, automation_info): |
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.
Name it async_attach_trigger
. What is config vs automation_info ? Shouldn't we just have hass, trigger_config, action
?
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.
Ok, but I leave it for now so the new trigger can be tested without any changes to the existing automation framework.
def _domain_validator(config): | ||
"""Validate it is a valid domain or platform.""" | ||
try: | ||
platform = importlib.import_module( |
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 really ugly, and it won't work with custom integrations either. We should use the async_get_integration
method instead. However, that's async. Maybe just drop this validator ?
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.
Yeah, the problem was that async_get_integration
is async. I removed the validator for now.
try: | ||
integration = await async_get_integration(hass, domain) | ||
except IntegrationNotFound: | ||
_LOGGER.exception('Integration %s not found', domain) |
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.
We should never use exception
unless it's an unexpected exception. This one we know why it happens (but shouldn't). I suggest we log a warning
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.
Fixed.
|
||
async def async_get_device_automation_triggers(hass, device_id): | ||
"""List device triggers.""" | ||
device_registry = await hass.helpers.device_registry.async_get_registry() |
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.
Run both at the same time: dev_reg, ent_reg = await asyncio.gather(…)
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.
Fixed.
for entity in entities: | ||
domains.add(split_entity_id(entity.entity_id)[0]) | ||
|
||
for domain in domains: |
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 run all of these tasks at the same time using asyncio.gather
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.
Fixed.
# Trigger when light is turned on | ||
CONF_PLATFORM: 'device', | ||
CONF_DOMAIN: DOMAIN, | ||
CONF_TYPE: 'turn_on', |
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 create constants for turn_on
and turn_off
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.
Fixed.
vol.Required(CONF_PLATFORM): 'device', | ||
vol.Optional(CONF_DEVICE_ID): str, | ||
vol.Required(CONF_DOMAIN): DOMAIN, | ||
vol.Required(CONF_ENTITY_ID): cv.entity_ids, |
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 should be single?
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.
Fixed.
state_config = { | ||
state.CONF_ENTITY_ID: config[CONF_ENTITY_ID], | ||
state.CONF_TO: to_state | ||
} |
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.
Include a FROM
or else a color update will trigger it too.
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.
fixed.
I think that overall this looks great. For translations, would we do something like |
@balloob thanks for reviewing! All fixed now + tests added. |
Description:
Add websock command to query device for triggers.
Triggers can be provided by the integration that provided the device or the entity integrations that the device has entities with.
As an example of the latter, this PR also adds
turn_on
andturn_off
triggers tolight
entities.TODO: Add an example of the former.
This PR is intended as initial step towards home-assistant/architecture#178
To test, add to
configuration.yaml
:device_automation:
Example websock exchange:
The returned trigger can be used in an automation:
Checklist:
tox
. Your PR cannot be merged unless tests passIf the code does not interact with devices: