-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
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
Fix KNX telegram device trigger not firing after integration reload #107388
Conversation
Hey there @Julius2342, @marvin-w, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
@@ -99,6 +99,8 @@ def async_call_trigger_action(telegram: TelegramDict) -> None: | |||
{"trigger": {**trigger_data, **telegram}}, | |||
) | |||
|
|||
return knx.telegrams.async_listen_telegram( | |||
async_call_trigger_action, name="KNX device trigger call" | |||
return async_dispatcher_connect( |
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.
Side note: Every device trigger needs to be based off a regular trigger like an event or integration specific trigger. Device triggers are just a device abstraction over a regular trigger and are not allowed to be standalone. I don't see any integration specific triggers in the KNX integration. See eg webostv integration for how to create an integration specific trigger.
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.
Yes, Knx doesn't have any integration specific triggers. This was never mentioned in a review, nor could I find anything about that in the developer documentations. I'll try to have a look into in when I have some time left. Are these always looked up in a <integration>/trigger.py
file?
Until then, I think this bugfix should still be merged as it doesn't change any behaviour on the current device trigger except fixing integration reloads.
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.
Yes, the module should be named trigger.py
.
My comment is a side note for the future.
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.
It says here that device automations are built on top of core concepts and are not exposing new functionality.
Device Automations provide users with a device-centric layer on top of the core concepts of Home Assistant.
Device automations are not exposing extra functionality but are a way for users to not have to learn new concepts.
https://developers.home-assistant.io/docs/device_automation_index
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.
Ah thanks. So I can add a trigger like knx.telegram
? That would be awesome.
I initially implemented this to avoid using HA events for the same effect, to not spam the event bus (and db) with unneeded Knx telegram information (~1-5 average, max 50 per second). I guess I can filter the integration specific trigger the same way we are doing for the device trigger, so this should be fine. (And not require any yaml as events would 😬)
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.
Triggers are always available to be setup in automations via YAML if the user so wishes. Maybe that's not what you mean?
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.
Or right, that's not what I mean. We currently have Events, but users would need to configure address filters in yaml for it to work (to not spam).
With a trigger I think we could provide a form (/yaml) directly in the automation builder for this
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.
Nice! Looks good to me!
Proposed change
Previously the custom device trigger was not working after an integration reload since its callback HassJob was removed with the integration. (in
core/homeassistant/components/knx/telegrams.py
Line 103 in 3d4d828
Now it is loosely coupled with
async_dispatcher_connect
in the device triggersasync_attach_trigger
andasync_dispatcher_send
which just happens for every telegram. So reloads don't affect this anymore.Type of change
Additional information
Checklist
ruff format homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.To help with the load of incoming pull requests: