[mqtt] Simplify homeassistant thing types, and use AbstractStorageBasedTypeProvider #16600

ccutrer · 2024-04-01T16:05:11Z

This is a rather large PR, but is built of a few commits with essentially two distinct goals. I have not tested them individually, and there are lots of interactions between the two goals, which is why I did not disentangle them.

This first goal is to simplify and optimize thing and channel types for mqtt.homeassistant. This fixes #14295. This in itself has two user visible changes:

Thing types have not actually changed, but thing UIDs have. Auto Discovery will no longer use the per-thing thing type in the thing ID, but will use the "base" thing type (available in the binding's XML) of mqtt:homeassistant in its ID. In practice, newly discovered things will get UIDs such as mqtt:homeassistant:mosquitto:awair-element-63611 instead of mqtt:homeassistant_awair_2Delement_2D63611:mosquitto:awair_2Delement_2D63611 (unnecessary escaping has also been removed). Existing things will not change their UID. I'm not sure if this is considered a breaking change, since existing things will not change at all. But if you were to delete the thing, and re-add it from discovery, it would end up with a different ID. This new behavior matches how mqtt.homie works.
Channel IDs have been simplified. First, if a component has no name (in the Home Assistant configuration), it will not generate a channel group at all. Second, the channel group name is now only the component's name, instead of including the entire thing ID in the channel group, it is now only the component name (which already has to be unique within a Home Assistant device, so not a problem there). The channel group type (which is unseen by the user) remains the fully unique id, which includes the thing id. Third, if a component only has a single channel, it also doesn't create a channel group, just a single channel for that component without a group.
For this change, I did a bit of hoop-jumping to pass a property through from auto-discovery, so that existing things will not have their channels change, only newly discovered and created things will use this new channel type. Again, I'm not sure if this counts as a breaking change. If it does, I'm possibly inclined to not use this hinky property, and simply apply the new channel IDs to all things. An example change is from mqtt:homeassistant_zigbee2mqtt_5F0x8cf681fffe32e58e:mosquitto:zigbee2mqtt_5F0x8cf681fffe32e58e:0x8cf681fffe32e58e_5Fbattery_5Fzigbee2mqtt#sensor to mqtt:homeassistant:mosquitto:zigbee2mqtt_5F0x8cf681fffe32e58e:sensor_battery

The second goal is relatively simple - use AbstractStorageBasedTypeProvider. Because the thing and channel group types are dynamic, they don't exist in the binding's XML. But since openHAB 4.0, if a thing type isn't available at startup, it will leave the thing in an unknown state for two minutes before it initializes it. By using AbstractStorageBasedTypeProvider, the dynamic types will be persisted to JSON storage, and thus available immediately at startup, avoiding the two minute delay. This fixes #15577

Instead, use state descriptions for the varying parts, which means that there is not an explosion of channel types for every single Home Assistant channel, which can cause performance issues if you have a lot of devices/channels Signed-off-by: Cody Cutrer <cody@cutrer.us>

This has two important impacts, and one not-as-important-but-still- beneficial impact: * ThingTypes are not created for Things that are never actually created, but only discovered, reducing bloat. * Avoids warnings about things types not being in the registry * Thing IDs end up like mqtt:homeassistant:broker:mydevice instead of mqtt:homeassistant_mydevice:broker:mydevice Note that already-created things will maintain their original ID, and it's only new things created from auto-discovery that will have the new, shorter thing IDs. Signed-off-by: Cody Cutrer <cody@cutrer.us>

This is triggered by a property only set by discovery, so will only apply to newly created things. Channel groups are now named after simply the component type and object_id, if possible, and without additional encoding, instead off encoding the unique_id. Most devices will encode the component type and device identifiers into the unique_id, and of arbitrary format, so it is far more complicated than necessary, when a channel group is already only within the scope of the thing/device. Many component types that can only ever have one channel (button, switch, sensor, select, scene, etc.) will not be in a channel group anymore. In this case their channel id is what their group id would have been, without the repetitiveness of "sensor_<unique_id>#sensor". Signed-off-by: Cody Cutrer <cody@cutrer.us>

Includes both homie and homeassistant dynamic things. It also fixes all ordering issues in both bindings in order to be consistent in how types are persisted: * For Homie, Nodes (channel groups) and Properties (channels) are ordered in the way they are defined in $nodes and $properties * For Home Assistant, Components are ordered by label. This includes both single channel components that are not in a channel group, as well as channel groups. We also ensure that on the Thing itself non-grouped channels consistently sort before grouped channels. Signed-off-by: Cody Cutrer <cody@cutrer.us>

Signed-off-by: Cody Cutrer <cody@cutrer.us>

...ing.mqtt.generic/src/main/java/org/openhab/binding/mqtt/generic/MqttChannelTypeProvider.java

...c/main/java/org/openhab/binding/mqtt/homeassistant/internal/component/AbstractComponent.java

....binding.mqtt.homie/src/main/java/org/openhab/binding/mqtt/homie/internal/homie300/Node.java

...c/main/java/org/openhab/binding/mqtt/homeassistant/internal/component/AbstractComponent.java

antroids · 2024-04-03T10:26:01Z

...c/main/java/org/openhab/binding/mqtt/homeassistant/internal/component/AbstractComponent.java

     */
-    public AbstractComponent(ComponentFactory.ComponentConfiguration componentConfiguration, Class<C> clazz) {
+    public AbstractComponent(ComponentFactory.ComponentConfiguration componentConfiguration, Class<C> clazz,
+            boolean newStyleChannels, boolean singleChannelComponent) {


Is this possible to move newStyleChannels into haID?

before I spend time making changes here, I'd like feedback on how we want to address backwards compatibility:

as is - newly discovered things get the shorter channels, already-existing things don't. file-based things don't (which... I don't even know if file-based Home Assistant things really work, so might not matter)

an explicit configuration on the thing to opt in (means can be used for file-based things, or if you have to re-create your things for some reason) - but means it's an additional step, so most people won't ever know about it. and not really useful for re-creating things, since the thing uid will always be the shorter version, even if you choose the longer channel ids.

an explicit configuration on the thing to opt out (means can be used for file-based things, or if you have to re-create your things for some reason) - but means it will break all existing things causing issues for existing users anyway. and not really useful for re-creating things, since the thing uid will always be the shorter version, even if you choose the longer channel ids.

just pull off the band aid and call a breaking change - all existing things get the newer shorter ids, and no way to go back. definitely simplifies the code to not need to plumb this through, or to have to have two branches

For the as-is option, i think this is elegant. One question, how long would you support the deprecated convention? One or two releases? When this comes with a clear eol date and an upgrade notice (distro repo) it is probably my favorite.

Imho opt-in/opt-out is only making it more complex and not solving any issue in the ugprade path.

The last option is clear, convenient for the developers, but disruptive to the end user.

So all in all, my vote would go to the first option.

antroids · 2024-04-03T10:40:43Z

...c/main/java/org/openhab/binding/mqtt/homeassistant/internal/component/AbstractComponent.java

+            if (newStyleChannels && this.singleChannelComponent) {
+                groupId = haID.component;
+            } else {
+                groupId = "";


Should not it be null as in the org.openhab.core.thing.ChannelUID.getGroupId?

It can't be null, since it's not just used as part of a channel id, but also for other uses in the thing handler that don't allow null. Perhaps when I have more time I'll check if I can change those all to allow nullable.

antroids · 2024-04-03T10:49:25Z

...c/main/java/org/openhab/binding/mqtt/homeassistant/internal/component/AbstractComponent.java

+    protected ComponentChannel.Builder buildChannel(String channelID, ChannelTypeUID channelTypeUID, Value valueState,
+            String label, ChannelStateUpdateListener channelStateUpdateListener) {
+        if (singleChannelComponent) {
+            channelID = groupId;
+        }
+        return new ComponentChannel.Builder(this, channelID, channelTypeUID, valueState, label,
+                channelStateUpdateListener);
    }


Currently, there are no restrictions on which UIDs can be used as channelTypeUID, but the types are determined. Could you create a build method for each channel type. The next step would be abstract builders with factory methods specific for channel types.

Can you please elaborate here? I'm not sure what you mean about types being determined.

lolodomo · 2024-04-06T16:22:16Z

@ccutrer : did you notice the build failed ?

ccutrer · 2024-04-06T16:23:34Z

I did. And I've gotten to the point that I can reproduce the failures locally, but have not yet had time to troubleshoot them (or address @antroids comments).

Signed-off-by: Cody Cutrer <cody@cutrer.us>

ccutrer

I've addressed (most) the review comments, but not looked at itests. The suggested changes have also caused (non-test) test failures - I think related to some of the foreach changes, possibly due to needing to mock different methods. I don't have time today to look into any of that, but will when I can

ccutrer · 2024-04-18T21:40:06Z

...c/main/java/org/openhab/binding/mqtt/homeassistant/internal/component/AbstractComponent.java

     */
-    public AbstractComponent(ComponentFactory.ComponentConfiguration componentConfiguration, Class<C> clazz) {
+    public AbstractComponent(ComponentFactory.ComponentConfiguration componentConfiguration, Class<C> clazz,
+            boolean newStyleChannels, boolean singleChannelComponent) {


before I spend time making changes here, I'd like feedback on how we want to address backwards compatibility:

as is - newly discovered things get the shorter channels, already-existing things don't. file-based things don't (which... I don't even know if file-based Home Assistant things really work, so might not matter)

an explicit configuration on the thing to opt in (means can be used for file-based things, or if you have to re-create your things for some reason) - but means it's an additional step, so most people won't ever know about it. and not really useful for re-creating things, since the thing uid will always be the shorter version, even if you choose the longer channel ids.

an explicit configuration on the thing to opt out (means can be used for file-based things, or if you have to re-create your things for some reason) - but means it will break all existing things causing issues for existing users anyway. and not really useful for re-creating things, since the thing uid will always be the shorter version, even if you choose the longer channel ids.

just pull off the band aid and call a breaking change - all existing things get the newer shorter ids, and no way to go back. definitely simplifies the code to not need to plumb this through, or to have to have two branches

ccutrer · 2024-04-18T21:48:51Z

...c/main/java/org/openhab/binding/mqtt/homeassistant/internal/component/AbstractComponent.java

+    protected ComponentChannel.Builder buildChannel(String channelID, ChannelTypeUID channelTypeUID, Value valueState,
+            String label, ChannelStateUpdateListener channelStateUpdateListener) {
+        if (singleChannelComponent) {
+            channelID = groupId;
+        }
+        return new ComponentChannel.Builder(this, channelID, channelTypeUID, valueState, label,
+                channelStateUpdateListener);
    }


Can you please elaborate here? I'm not sure what you mean about types being determined.

ccutrer · 2024-04-18T21:50:49Z

...c/main/java/org/openhab/binding/mqtt/homeassistant/internal/component/AbstractComponent.java

+            if (newStyleChannels && this.singleChannelComponent) {
+                groupId = haID.component;
+            } else {
+                groupId = "";


It can't be null, since it's not just used as part of a channel id, but also for other uses in the thing handler that don't allow null. Perhaps when I have more time I'll check if I can change those all to allow nullable.

ccutrer added 4 commits April 1, 2024 09:35

ccutrer requested a review from antroids as a code owner April 1, 2024 16:05

ccutrer added 2 commits April 1, 2024 10:37

fix spotless warning

3d71b9d

Signed-off-by: Cody Cutrer <cody@cutrer.us>

address itests compilation errors

76c3b9b

Signed-off-by: Cody Cutrer <cody@cutrer.us>

lolodomo added the enhancement An enhancement or new feature for an existing add-on label Apr 3, 2024

antroids requested changes Apr 3, 2024

View reviewed changes

address review comments

b54d5da

Signed-off-by: Cody Cutrer <cody@cutrer.us>

ccutrer commented Apr 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mqtt] Simplify homeassistant thing types, and use AbstractStorageBasedTypeProvider #16600

[mqtt] Simplify homeassistant thing types, and use AbstractStorageBasedTypeProvider #16600

ccutrer commented Apr 1, 2024

antroids Apr 3, 2024

ccutrer Apr 18, 2024

lsiepel May 20, 2024 •

edited

antroids Apr 3, 2024

ccutrer Apr 18, 2024 •

edited

antroids Apr 3, 2024

ccutrer Apr 18, 2024

lolodomo commented Apr 6, 2024

ccutrer commented Apr 6, 2024

ccutrer left a comment

ccutrer Apr 18, 2024

ccutrer Apr 18, 2024

ccutrer Apr 18, 2024 •

edited

[mqtt] Simplify homeassistant thing types, and use AbstractStorageBasedTypeProvider #16600

Are you sure you want to change the base?

[mqtt] Simplify homeassistant thing types, and use AbstractStorageBasedTypeProvider #16600

Conversation

ccutrer commented Apr 1, 2024

antroids Apr 3, 2024

Choose a reason for hiding this comment

ccutrer Apr 18, 2024

Choose a reason for hiding this comment

lsiepel May 20, 2024 • edited

Choose a reason for hiding this comment

antroids Apr 3, 2024

Choose a reason for hiding this comment

ccutrer Apr 18, 2024 • edited

Choose a reason for hiding this comment

antroids Apr 3, 2024

Choose a reason for hiding this comment

ccutrer Apr 18, 2024

Choose a reason for hiding this comment

lolodomo commented Apr 6, 2024

ccutrer commented Apr 6, 2024

ccutrer left a comment

Choose a reason for hiding this comment

ccutrer Apr 18, 2024

Choose a reason for hiding this comment

ccutrer Apr 18, 2024

Choose a reason for hiding this comment

ccutrer Apr 18, 2024 • edited

Choose a reason for hiding this comment

lsiepel May 20, 2024 •

edited

ccutrer Apr 18, 2024 •

edited

ccutrer Apr 18, 2024 •

edited