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
ZHA - add events for remote like devices #18493
Conversation
hass.data[DISCOVERY_KEY] = hass.data.get(DISCOVERY_KEY, {}) | ||
import homeassistant.components.zha.const as zha_const |
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 can be imported at the top of the 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.
There is a design issue w/ the modules that causes circular imports. I have it corrected in another branch and can fix this in a subsequent PR if that is ok.
@@ -177,8 +180,8 @@ def device_removed(self, device): | |||
async def async_device_initialized(self, device, join): | |||
"""Handle device joined and basic information discovered (async).""" | |||
import zigpy.profiles | |||
import homeassistant.components.zha.event as zha_event |
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.
See above.
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.
There is a design issue w/ the modules that causes circular imports. I have it corrected in another branch and can fix this in a subsequent PR if that is ok.
@@ -177,8 +180,8 @@ def device_removed(self, device): | |||
async def async_device_initialized(self, device, join): | |||
"""Handle device joined and basic information discovered (async).""" | |||
import zigpy.profiles | |||
import homeassistant.components.zha.event as zha_event | |||
import homeassistant.components.zha.const as zha_const |
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.
Please move this 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.
There is a design issue w/ the modules that causes circular imports. I have it corrected in another branch and can fix this in a subsequent PR if that is ok.
_LOGGER.debug( | ||
"remotes config path: %s Is path: %s", | ||
remotes_config_path, | ||
os.path.isfile(remotes_config_path) |
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 doing I/O in an async context. That's not allowed.
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.
any suggestions on how to handle? Should I just load this at the top of the module and not in the function?
{ZHA: {}, ZLL: {}} | ||
) | ||
|
||
remote_devices = load_json(remotes_config_path) |
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.
What are we doing here and what does it have to do with events? Looks like another feature, so should not be part of this PR.
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.
without this nothing sends events. This is a registry so users can opt certain devices in to sending events. It is described in the PR description.
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.
Why does it need to be configurable?
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.
There are 100’s of zigbee devices and they don’t all follow defined specs. Also, there is no hard rule for what devices should use / need events. The alternative is a HA PR for every particular device that people want events from.
@MartinHjelmare Thanks a bunch for the help here and the comments. I think I have a much more elegant solution as a result that ditches the registration file. Closing this and I will open a new PR that references this one so that I can get your feedback. Thanks again! |
The original motivation for this came from comments in #14795. I then opened PR #14902 which I closed due to it getting stale and hard to rebase easily. There was good feedback in that PR and I believe I managed to address most of the comments. That being said, I have changed direction a bit. This PR adds support for events for remote like devices. It also avoids breaking changes by leaving the original entities intact and functional.
This is opt in only. There is a new config file
zha-remotes.json
that has this shape:This allows users to opt into this functionality without needing HA releases to "remap" devices.