-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Bridge discovery #19994
Bridge discovery #19994
Conversation
Tests are still missing (as well as some potential HA entities). |
5a59627
to
a04c876
Compare
@Koenkk Unfortunately, I need help with the tests again. Edit: OK, I get it now, they are partially covered. Not sure what to do about line |
a04c876
to
7fbbfc4
Compare
Open issues:
|
First of all, many thanks for this!
Left a comment regarding this on the code
I don't see another place to use it currently, let's leave it in
Currently not, but the current handling is fine. |
187534b
to
ae39663
Compare
So apart from updating/expanding the list of discoverable entities, only the issue of the device identifier to use is still open. I've left a reply on the code comment, but I can recap here: If the coordinator address is only changed under very specific circumstances (like when restoring a backup to a new coordinator), I think we should be fine with using it as the identifier, as the discovery entries for all devices will be recreated on startup and point to the new bridge. My preference for this is based on that it is an actual device identifier and not just a configuration setting like the base topic. |
lib/extension/homeassistant.ts
Outdated
object_id: 'connection_state', | ||
mockProperties: [], | ||
discovery_payload: { | ||
name: 'Connection 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.
I don't think this sensor is necessary. It's basically the availability of the bridge, something which can already be retrieved from the other sensors (we also don't have a separate availability sensor for each device)
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.
I think it is actually useful (but as a binary_sensor
instead of a sensor
like it was in the package.yaml
) for automations in HA. Yes, you could build your own from individual ZigBee devices, but which would you "watch"? Having a binary sensor for online status of hub-style devices is pretty common.
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.
I've changed the sensor
to a binary_sensor
.
Agree with your solution, left a few more comments, once those are solved this can be merged. |
ae39663
to
afc6363
Compare
I've cleaned the list of entities somewhat and added a few more that seemed useful (either from the |
Thanks again! I think all the manual entities can be removed from the docs now? |
Mostly, I think. Some of the input helper entities are not included yet or can't be replaced with MQTT entities because they need more complex automation logic. I'll have a go at a PR to remove those that are now superfluous. |
in connection with that PR I noticed these errors When all the Z2M configuration is in the entities part of the attributes
|
@leroyloren I assume you have "legacy_entity_attributes" enabled? |
yes |
@Koenkk Is this a configuration you want to support? If so, the code for adding the legacy entity attributes will need special handling (probably by not adding any entity attributes automatically at all, or possibly by filtering). I can provide a PR. I also saw this during initial development, but since it does not technically break HA, I chose to view it as an incentive for people to not enable the legacy entity attributes anymore. |
@mundschenk-at no, @leroyloren please disable the legacy option |
@mundschenk-at i love this new feature. thank you! this makes some of my custom mqtt entities obsolet (restart, permit join) can you add also a device counter sensor to the bridge? currently i have this custom entity mqtt:
sensor:
- unique_id: mqtt_z2m_device_count
name: "Zigbee2MQTT Device Counter"
state_topic: "zigbee2mqtt/bridge/devices"
state_class: total
value_template: "{{ value_json | count }}"
availability:
- topic: "zigbee2mqtt/bridge/state"
value_template: "{{ value_json.state }}"
payload_available: "online" and a automation to notify me if some of the stupid aqara devices left the network again alias: System - Z2M Devices change
description: ""
trigger:
- platform: state
entity_id:
- sensor.z2m_devices
for:
hours: 0
minutes: 0
seconds: 0
condition:
- condition: state
entity_id: binary_sensor.zigbee2mqtt_bridge_connection_state
state: online
for:
hours: 0
minutes: 5
seconds: 0
action:
- service: notify.mobile_app_XY
data:
title: Z2M
message: Device Counter changed
mode: single |
@b2un0 I'll look into this. |
command_topic: `${baseTopic}/request/permit_join`, | ||
payload_on: 'true', |
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.
Hello, great work! Maybe it's worth adding a 255s timeout there, as is done in the Z2M frontend? I think it's more secure. @Koenkk
command_topic: `${baseTopic}/request/permit_join`, | |
payload_on: 'true', | |
command_topic: `${baseTopic}/request/permit_join`, | |
payload_on: '{"value": true, "time": 255}', |
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.
Since this is a home automation system, it's as "secure" as you make your automations. There is a more involved discussion in #20468 (comment) (with wildly differing opinions, I don't see a clear consensus either way).
getDiscoverKey
method.availability
payload for certain entities.Permit join
(switch
)Restart
(button
)Log level
(select
)Connection state
(binary_sensor
)Network map
(sensor
)Permit join timeout
(sensor
)Restart required
(binary_sensor
)Version
(sensor
)Coordinator version
(sensor
)