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

Allow MQTT device based auto discovery #109030

Merged
merged 59 commits into from
May 29, 2024
Merged

Allow MQTT device based auto discovery #109030

merged 59 commits into from
May 29, 2024

Conversation

jbouwh
Copy link
Contributor

@jbouwh jbouwh commented Jan 28, 2024

Proposed change

The current MQTT discovery model only allows to setup per component, if a device has multiple entities to published, the device context, availability and origin information needs to be duplicated.

This PR reduces IO overhead in the paho client on processing discovery packages: The baseline was 10000 packets, with device discovery it 6250 packet reads so a 37.5% reduction in I/O. Processing less I/O reduced the paho client run time by ~18%. The grouped discovery reduced the run time in the HA client by ~12%.

This PR adds a way to discover all components for a device with one discovery. The device, availability and origin mapping is only submitted once. Also the following options are allowed in the shared context:

  • state_topic
  • command_topic
  • encoding
  • qos

Except for device and origin options, supported shared options can be overridden in the component config.

Discovery updates and removal is fully supported.

The device based discovery coexists with the component based discovery, for 18 months. We start sending deprecation warnings after 6 months for the single component schema.

Note that some code and schemas that are related to device, availability and origin validation was moved to a new module schemas.py.

Example

The example below is of a device based auto discovery that supplies a sensor and binary_sensor.

Discovery topic:

homeassistant/device/test123/config

Payload:

{
  "dev": {
    "identifiers": [
      "mysensors123"
    ]
  },
  "avty": {
    "topic": "avty-topic"
  },
  "cmp": {
    "bins1": {
      "platform": "binary_sensor",
      "name": "Beer",
      "state_topic": "test-topic"
    },
    "sens1": {
      "platform": "sensor",
      "name": "Milk",
      "state_topic": "test-topic"
    }
  },
  "o": {
    "name": "My org",
    "sw": "bla2mqtt"
  },
  "qos": 0
}

Note that a required platform option is added to identify the component platform. The keys under the components (abbreviated cmp), are treated as object_id and are used to create a unique device discovery_id. In this case the discover_id's are test123 bins1 and test123 sens1.

In case a node_id is specified (e.g. node123) in the device discovery topic ...

homeassistant/device/node123/test123/config

... this will become part of the object_id.
The discovery_id 's become test123 node123 bins1 and test123 node123 sens1.

Migration from single topic to device based discovery

To allow a smooth migration from single topic discovery to device based discovery, the discovery_id for an mqtt item must be the same. Migration is supported of the single topic id has a node_id and a object_id. After migration the object id moves inside the discovery payload and the previous node_id becomes the new object_id of the device discovery topic.

Example (device automation):

Discovery topic single: homeassistant/device_automation/0AFFD2/bla/config
Discovery id: 0AFFD2 bla
Discovery payload single:

{
  "automation_type": "trigger",
  "device": {
    "identifiers": [
      "0AFFD2"
    ]
  },
  "o": {
    "name": "foobar"
  },
  "payload": "short_press",
  "topic": "foobar/triggers/button1",
  "type": "button_short_press",
  "subtype": "button_1"
}

Migrated:

Discovery topic device: homeassistant/device/0AFFD2/config
Discovery id: 0AFFD2 bla
Discovery payload device:

{
  "device": {
    "identifiers": [
      "0AFFD2"
    ]
  },
  "o": {
    "name": "foobar"
  },
  "cmp": {
    "bla": {
      "automation_type": "trigger",
      "payload": "short_press",
      "topic": "foobar/triggers/button1",
      "type": "button_short_press",
      "subtype": "button_1",
      "platform": "device_automation"
    }
  }
}

If the new device discovery payload has the same discovery_id and comes after the single discovery payload. Home Assistant will publish None (retained) to the single discovery payload to remove it.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

@home-assistant
Copy link

Hey there @emontnemery, mind taking a look at this pull request as it has been labeled with an integration (mqtt) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of mqtt can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign mqtt Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

@corporategoth
Copy link

What would a sample payload look like?

@jbouwh
Copy link
Contributor Author

jbouwh commented Jan 29, 2024

What would a sample payload look like?

Have a look at the tests. I'll update the PR docs btw.

@jbouwh
Copy link
Contributor Author

jbouwh commented Jan 29, 2024

What would a sample payload look like?

Have a look at the tests. I'll update the PR docs btw.

Example added in PR documentation

@jbouwh
Copy link
Contributor Author

jbouwh commented Jan 29, 2024

@Koenkk can you comment if this would help in your opinion?

@bieniu
Copy link
Member

bieniu commented Jan 29, 2024

I haven't looked at the code yet because I'm on vacation, so I have a few questions...

  • will the configuration for individual components be the same as before? i.e. will all configuration options for entities be available?
  • will individual entities be able to have separate availability topics or always those inherited from the device?
  • I wonder if there will be an easy way to migrate entities already configured to the new schema 🤔

I assume the old way of configuring will remain? Removing it (even in a few years) would be a massive breaking change because a lot of different integrations/devices use MQTT discovery.

@jbouwh
Copy link
Contributor Author

jbouwh commented Jan 29, 2024

I haven't looked at the code yet because I'm on vacation, so I have a few questions...

  • will the configuration for individual components be the same as before? i.e. will all configuration options for entities be available?

Yes

  • will individual entities be able to have separate availability topics or always those inherited from the device?

Yes, but in the current implementation the central config overrides the individual settings (maybe I should change this for availability)

  • I wonder if there will be an easy way to migrate entities already configured to the new schema 🤔

The device entry en entity id's should not change, but it would be good to add a test that proves how a migration is possible,

I assume the old way of configuring will remain? Removing it (even in a few years) would be a massive breaking change because a lot of different integrations/devices use MQTT discovery.

Yes, they can be used together, it is not a breaking change

@corporategoth
Copy link

@jbouwh It would be nice if a common command and state topic could be specified (device-wide), and overridden on a per-entity basis.

In Z2M, the command and state topic is identical for every entity on that device, there is no reason to specify a per-entity state/command topic. The code is already setup to extract a specific json attribute from the state MQTT message.

So being able to specify a default state/command topic used by all entities (unless overridden) will reduce the size of this message drastically. Not sure if message size matters, but presumably smaller messages are quicker to transmit/process.

@jbouwh
Copy link
Contributor Author

jbouwh commented Jan 29, 2024

@jbouwh It would be nice if a common command and state topic could be specified (device-wide), and overridden on a per-entity basis.

In Z2M, the command and state topic is identical for every entity on that device, there is no reason to specify a per-entity state/command topic. The code is already setup to extract a specific json attribute from the state MQTT message.

So being able to specify a default state/command topic used by all entities (unless overridden) will reduce the size of this message drastically. Not sure if message size matters, but presumably smaller messages are quicker to transmit/process.

And entity based override would make sense for availability. The origin and device config should be central.

In the current code the central config overrides the entity specific config, but can be changed easily.

I want to limit the amount of supported parameters in the device context for this PR. So a common state_topic is not something I want to add here yet. May be in a feature update.

Edit: Added state_topic, command_topic, qos and encoding as shared options that can be overridden in the entity config.

@bieniu
Copy link
Member

bieniu commented Jan 29, 2024

Thanks @jbouwh Tomorrow I'll have access to my dev env, so I will play with Shellies Discovery and this PR over the next few days and come back to you.

@corporategoth
Copy link

corporategoth commented Jan 29, 2024

That would make sense for availability. The origin and device config should be central.

In the current code the central config overrides the entity specific config, but can be changed easily.

I was more thinking a top level section on this JSON (ie. same level as 'dev', 'cmp', 'o') of:

"def": {
  "state_topic": "some-topic",
  "command_topic": "some-cmd-topic",
}

Then the code simply does the check if cmp.entity.state_topic is defined, use it, otherwise use def.state_topic. Then you don't need to repeat the state topic for every entity in the JSON. It's a small optimization, not a hill I'll die on, but one that will cut down the size of the message.

Note: I'm thinking of my devices that produce 80+ entities ... so you're reducing 80+ duplicate declarations of the state/command topic.

@jbouwh
Copy link
Contributor Author

jbouwh commented Jan 29, 2024

That would make sense for availability. The origin and device config should be central.
In the current code the central config overrides the entity specific config, but can be changed easily.

I was more thinking a top level section on this JSON (ie. same level as 'dev', 'cmp', 'o') of:

"def": {
  "state_topic": "some-topic",
  "command_topic": "some-cmd-topic",
}

Then the code simply does the check if cmp.entity.state_topic is defined, use it, otherwise use def.state_topic. Then you don't need to repeat the state topic for every entity in the JSON. It's a small optimization, not a hill I'll die on, but one that will cut down the size of the message.

Note: I'm thinking of my devices that produce 80+ entities ... so you're reducing 80+ duplicate declarations of the state/command topic.

Ah, okay. At the moment the shared options are at root level of the json. I made it it easy to expand shared options in the future. I am out sure this should be in a mapping too. The problem with options like state_topic is that not all entity platforms support that topic, and we do not just want to insert the shared option in the component config.

@Koenkk
Copy link
Contributor

Koenkk commented Jan 29, 2024

This would be really useful for z2m indeed, currently z2m subscribes to Home Assistant discovery topic on startup which generate a huge amount of messages and degrades performance because of this (Koenkk/zigbee2mqtt#20648)

@jbouwh
Copy link
Contributor Author

jbouwh commented Jan 29, 2024

Ah, okay. At the moment the shared options are at root level of the json. I made it it easy to expand shared options in the future. I am out sure this should be in a mapping too. The problem with options like state_topic is that not all entity platforms support that topic, and we do not just want to insert the shared option in the component config.

After all, it seems it makes sense to have some more shared options added, like state_topic or command_topic. It would reduce the size of the config message.

@mundschenk-at
Copy link

mundschenk-at commented Jan 29, 2024

I think for Z2M it would also be beneficial if the <node_id>/<object_id> segmentation was possible like with entity-based discovery, allowing for <discovery_prefix>/device/<node_id>/<object_id>/config and targeted subscription:

homeassistant/device/my_bridge_123/device_123/config
homeassistant/device/my_bridge_123/device_456/config
...

@jbouwh
Copy link
Contributor Author

jbouwh commented Jan 29, 2024

I think for Z2M it would also be beneficial if the <node_id>/<object_id> segmentation was possible like with entity-based discovery, allowing for <discovery_prefix>/device/<node_id>/<object_id>/config and targeted subscription:

homeassistant/device/my_bridge_123/device_123/config homeassistant/device/my_bridge_123/device_456/config ...

With the current PR implementation you can use the node_id too if that is desired. The key under cmp is added as an extra key to generate the discovery_id for the component. This is done to be able to find the siblings for a device, If a node_id is set in the discovery topic, then that will be used too as explained in the PR documentation.

@mundschenk-at
Copy link

Sorry, I missed the part below the fold (after the example payload). All good!

@jbouwh jbouwh marked this pull request as ready for review May 29, 2024 08:34
@bdraco
Copy link
Member

bdraco commented May 29, 2024

Looks good. Running one last profile now.

@bdraco
Copy link
Member

bdraco commented May 29, 2024

Big hitter on the profile is still schema, but I don't think we can do much about that right now

Screenshot 2024-05-28 at 11 04 46 PM

@bdraco
Copy link
Member

bdraco commented May 29, 2024

Mixins still comes up high but much much better with all the other changes

Screenshot 2024-05-28 at 11 05 03 PM

@jbouwh jbouwh merged commit 585892f into dev May 29, 2024
38 checks passed
@jbouwh jbouwh deleted the jbouwh-mqtt-device-discovery branch May 29, 2024 09:12
@bdraco
Copy link
Member

bdraco commented Jun 3, 2024

@jbouwh Asked for a bit of a summary of the performance changes here. Sadly I has already deleted the screenshots, but the after profile screenshot is here: #109030 (comment)

Reposted as
333959149-07ab213a-1990-467b-ba7e-d3d9597f8683

The baseline was 10000 packets, with device discovery it 6250 packet reads so a 37.5% reduction in I/O. Processing less I/O reduced the paho client run time by ~18%. The grouped discovery reduced the run time in the HA client by ~12%.

@bdraco
Copy link
Member

bdraco commented Jun 3, 2024

Also that reminds me we have some useless threading locks we can get rid of since everything is running in the main thread now.

@jbouwh
Copy link
Contributor Author

jbouwh commented Jun 3, 2024

Also that reminds me we have some useless threading locks we can get rid of since everything is running in the main thread now.

Nice

@emontnemery
Copy link
Contributor

@bdraco, @jbouwh this PR adds a complete new schema for discovering MQTT entities, and we will have to support both schemas indefinitely. A reduction of 12% is not nearly enough to motivate that IMO. Unless I misunderstand the message above, I think we should revert this PR before we release HA Core 2024.6.

@home-assistant home-assistant unlocked this conversation Jun 3, 2024
@bdraco
Copy link
Member

bdraco commented Jun 3, 2024

Not sure it really moves the needle (or is much different), but the overall combined runtime reduction between the paho and ha client code is little bit more than that at @ ~16%

The performance improvement in HA wasn't what pushed this over the top for me though. During testing we kept having the MQTT client drop messages because it would overwhelm the queue. While we were able to increased the maximum number of queued messages for the addon, thats only solves it for a subset of users. Without a path to reducing the number of messages (discovery or otherwise), MQTT isn't going to be reliable at high volume since HA will not be able to keep up with the broker.

Everything fell apart at ~1900 entities before (1960 was the magic number on my system that was always reliable). Its also reasonable to say that we will only support a maximum of ~1800 entities and anything beyond that is not supported.

y0gi44 pushed a commit to y0gi44/home-assistant-core that referenced this pull request Jun 3, 2024
We call this 100000s of times if there are many subscriptions

home-assistant#109030 (comment)
jbouwh added a commit that referenced this pull request Jun 3, 2024
jbouwh added a commit that referenced this pull request Jun 3, 2024
Revert "Allow MQTT device based auto discovery (#109030)"

This reverts commit 585892f.
@bdraco
Copy link
Member

bdraco commented Jun 3, 2024

#118757 Is the replacement

frenck pushed a commit that referenced this pull request Jun 4, 2024
Revert "Allow MQTT device based auto discovery (#109030)"

This reverts commit 585892f.
dgomes pushed a commit to dgomes/home-assistant that referenced this pull request Jun 4, 2024
Revert "Allow MQTT device based auto discovery (home-assistant#109030)"

This reverts commit 585892f.
@github-actions github-actions bot locked and limited conversation to collaborators Jun 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants