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

Bridge discovery #19994

Merged
merged 6 commits into from
Dec 6, 2023
Merged

Bridge discovery #19994

merged 6 commits into from
Dec 6, 2023

Conversation

mundschenk-at
Copy link
Contributor

@mundschenk-at mundschenk-at commented Dec 3, 2023

  • Adds discovery for the Z2M Bridge. Fixes [Feature request]: Auto-discovery for bridge/controller #19877.
  • Simplifies the getDiscoverKey method.
  • Adds support for disabling the availability payload for certain entities.
  • Adds the following entities for the bridge:
    • Control:
      • Permit join (switch)
      • Restart (button)
    • Config:
      • Log level (select)
    • Diagnostic:
      • Connection state (binary_sensor)
      • Network map (sensor)
      • Permit join timeout (sensor)
      • Restart required (binary_sensor)
      • Version (sensor)
      • Coordinator version (sensor)

@mundschenk-at mundschenk-at marked this pull request as draft December 3, 2023 01:10
@mundschenk-at
Copy link
Contributor Author

Tests are still missing (as well as some potential HA entities).

@mundschenk-at mundschenk-at force-pushed the bridge_discovery branch 4 times, most recently from 5a59627 to a04c876 Compare December 3, 2023 13:51
@mundschenk-at
Copy link
Contributor Author

mundschenk-at commented Dec 3, 2023

@Koenkk Unfortunately, I need help with the tests again. I have got no clue why those three lines (89, 1396, 1560) are shown as uncovered.

Edit: OK, I get it now, they are partially covered. Not sure what to do about line 89 except ignore it, but I think I can work something out for the other two.

@mundschenk-at mundschenk-at marked this pull request as ready for review December 3, 2023 15:36
@mundschenk-at
Copy link
Contributor Author

Open issues:

  • Instead of putting the Coordinator Type into the hw_version field, we could set it as the model. I already changed it to that and then changed it back because having the Z2M version as sw_version (listed by HA as Firmware version) didn't look right.
  • Would a data model for the Bridge be useful elsewhere (similar to Device and Group)?
  • I am currently treating the CoordinatorVersion.meta object as containing the specific set of subfields that are provided by the ZH adapter classes, but formally that's not guaranteed. Do have any plans to make this structure an actual part of the interface/type definition?

test/homeassistant.test.js Outdated Show resolved Hide resolved
test/homeassistant.test.js Outdated Show resolved Hide resolved
@Koenkk
Copy link
Owner

Koenkk commented Dec 3, 2023

First of all, many thanks for this!

Instead of putting the Coordinator Type into the hw_version field, we could set it as the model. I already changed it to that and then changed it back because having the Z2M version as sw_version (listed by HA as Firmware version) didn't look right.

Left a comment regarding this on the code

Would a data model for the Bridge be useful elsewhere (similar to Device and Group)?

I don't see another place to use it currently, let's leave it in homeassistant.ts and move it when needed elsewhere.

I am currently treating the CoordinatorVersion.meta object as containing the specific set of subfields that are provided by the ZH adapter classes, but formally that's not guaranteed. Do have any plans to make this structure an actual part of the interface/type definition?

Currently not, but the current handling is fine.

@mundschenk-at mundschenk-at force-pushed the bridge_discovery branch 2 times, most recently from 187534b to ae39663 Compare December 4, 2023 20:59
@mundschenk-at
Copy link
Contributor Author

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.

object_id: 'connection_state',
mockProperties: [],
discovery_payload: {
name: 'Connection state',
Copy link
Owner

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)

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@Koenkk
Copy link
Owner

Koenkk commented Dec 5, 2023

, only the issue of the device identifier to use is still open.

Agree with your solution, left a few more comments, once those are solved this can be merged.

@mundschenk-at
Copy link
Contributor Author

I've cleaned the list of entities somewhat and added a few more that seemed useful (either from the package.yaml or on its own).

@Koenkk Koenkk merged commit f11eb24 into Koenkk:dev Dec 6, 2023
8 checks passed
@Koenkk
Copy link
Owner

Koenkk commented Dec 6, 2023

Thanks again! I think all the manual entities can be removed from the docs now?

@mundschenk-at
Copy link
Contributor Author

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.

@leroyloren
Copy link

in connection with that PR I noticed these errors

When all the Z2M configuration is in the entities part of the attributes

State attributes for switch.zigbee2mqtt_bridge_permit_join exceed maximum size of 16384 bytes. This can cause database performance issues; Attributes will not be stored State attributes for sensor.zigbee2mqtt_bridge_permit_join_timeout exceed maximum size of 16384 bytes. This can cause database performance issues; Attributes will not be stored State attributes for sensor.zigbee2mqtt_bridge_version exceed maximum size of 16384 bytes. This can cause database performance issues; Attributes will not be stored State attributes for select.zigbee2mqtt_bridge_log_level exceed maximum size of 16384 bytes. This can cause database performance issues; Attributes will not be stored

obrazek
dfgdf

@mundschenk-at
Copy link
Contributor Author

@leroyloren I assume you have "legacy_entity_attributes" enabled?

@leroyloren
Copy link

yes

@mundschenk-at
Copy link
Contributor Author

mundschenk-at commented Dec 19, 2023

@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.

@Koenkk
Copy link
Owner

Koenkk commented Dec 19, 2023

@mundschenk-at no, @leroyloren please disable the legacy option

@b2un0
Copy link
Sponsor

b2un0 commented Jan 2, 2024

@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

@mundschenk-at
Copy link
Contributor Author

@b2un0 I'll look into this.

Comment on lines +1897 to +1898
command_topic: `${baseTopic}/request/permit_join`,
payload_on: 'true',
Copy link
Contributor

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

Suggested change
command_topic: `${baseTopic}/request/permit_join`,
payload_on: 'true',
command_topic: `${baseTopic}/request/permit_join`,
payload_on: '{"value": true, "time": 255}',

Copy link
Contributor Author

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants