Split sub-bindings into dedicated features

[lsiepel]

I like to point to this PR and proceed discussing the solution here.

modbus, bluetooth and mqtt have multiple forms of subbindings. The current structure has some pro's and con's. Maybe the optimal solutaion has changed over time now the amount of bindings have grown.

Helpfull infomration is here: openhab/openhab-addons#18793 (comment)

I like to invite both @openhab/add-ons-maintainers and @openhab/core-maintainers to join the discussion.

[ccutrer]

Also, cc @cdjackson @cpmeister @ssalonen as what appears to be maintainers of the core bits of Bluetooth and ModBus.

I'll jump right in:

Right now I'm thinking that it no longer makes sense to have sub-bindings at all. For MQTT there are only a handful, but the Home Assistant one is quite large (and getting larger, with an upcoming dependency on GraalVM/Python), which many users that use the generic MQTT binding don't care to install. I also feel it can be confusing for users for many of these bindings because Bluetooth, ModBus, and MQTT are all essentially transport layers that users may not know or care that their devices are using. For example, when using Falcon Player binding (mqtt.fpp), a user won't have any thought for "should I connect via MQTT or Rest API". For Bluetooth and ModBus bindings there are sooo many sub-bindings that the thing type list when manually adding a thing for them is very long.

So... my proposal is this:

• Sub-bindings become their own addon (get a dedicated addon.xml). Devices that only have a single possible transport (like FPP, SunSpec) just get moved into a top-level org.openhab.binding.fpp, org.openhab.binding.sunspec, etc. Devices that you might connect via multiple transports (like AirThings), or that are inextricably connected to the transport as part of their identity can stay under their parent binding umbrella, but still with a dedicated addon.xml, and a new addon ID: org.openhab.binding.bluetooth.airthings with addon id bluetooth.airthings; org.openhab.binding.bluetooth.bluez.
• Due to binding id changing, thing type IDs will change. All of the bindings will need to account for this in their ThingHandlerFactories, advertising support for both old-style and new-style ids: Home Assistant will claim support for mqtt:homeassistant and homeassistant:device. The ThingHandler can additionally use changeThingType to dynamically change existing things to the new thing type id to avoid needing to keep duplicate thing type definitions. Perhaps we can add some infrastructure to core to make such transitions easier? Maybe not necessary.
• Collapse the Generic MQTT Binding (org.openhab.binding.mqtt.generic) into the MQTT Broker Binding (org.openhab.binding.mqtt) for a few reasons:
  • Several of the sub-bindings depend on the generic binding as well as the broker binding.
  • Even as the primary maintainer of mqtt, mqtt.generic, mqtt.espmilighthub, mqtt.homie, and mqtt.homeassistant I have a hard time keeping straight what belongs in mqtt and what belongs in mqtt.generic. Mostly I don't put too much thought into it because I know the end user only sees "MQTT Binding" and gets both bundles with the current setup.
  • The most popular -- by far -- sub-binding of MQTT is the generic binding, so this means nothing will change for these users, including not needing to manually selecting any more addons to be installed when they upgrade (sub-bindings that are split out will not get installed automatically).
• The bridge types will stay in the "base" binding: MQTT Broker will be in org.openhab.binding.mqtt, ModBus TCP and Serial bridges will be in org.openhab.binding.modbus. I'm less sure how to handle this with Bluetooth, since at the moment it seems like every thing type has to be aware of all bridge types, and the bridges are implemented in specific sub-bindings. Maybe this means org.openhab.binding.bluetooth.bluegiga, org.openhab.binding.bluetooth.bluez, and org.openhab.binding.bluetooth.roaming remain as part of the primary Bluetooth binding, but as separate bundles only for maintainability purposes.
• We need to ensure that cross-binding bridges are allowed, so that a device in binding homeassistant is able to specify a bridge from the mqtt binding. This might take some core changes (especially in the Schema for thing types) to be able to declare this properly, and then some front end changes so that the UI can show the proper list of existing brokers in the broker dropdown when creating a thing.
• Sub-bindings will reference their dependent binding in their feature.xml with a <feature> tag. This means that installing "Home Assistant" addon from the addon store should automatically install -- and mark as installed -- "MQTT" addon. But this needs to be verified - it might take some front end changes and possibly some core changes to make sure this works correctly. Additionally, the UI should detect this state, and issue an additional warning if you attempt to uninstall "MQTT" while any of its dependents are installed.

With all of that said, once any core and/or UI dependencies are worked out, this doesn't have to be a big-bang change. Individual sub-bindings can be broken out one at a time. I volunteer espmilighthub as the first tribute, since it's a relatively simple binding, not particularly popular in terms of user base, and I'm a maintainer of.

Please give feedback on any of this proposal - things you think will work well, things that won't, things that need additional fleshing out, potential pitfalls I might not have anticipated, etc.!

[lsiepel]

As i see 4 rockets and no comment, i think this proposal is as good as it gets. But feedback from @holgerfriedrich , @rlkoshak, @jlaur and anyone else who can add value to the discussion would be appreciated.

As we are not that far from feature freeze, i wonder what to do. I would like to get this into oh5 as that would make backporting easier, but there is some time pressure. wdyt @ccutrer ? We could also do mqtt in oh5 and modbus / bluetooth in 5.1

[ssalonen]

I do agree with your assessment on modbus. They are all separate devices. Afaik, apart from the main bundle, all the subbindings are very device specific, and therefore not relevant to everybody. As you said, protocol is just implementation detail

[ccutrer]

As we are not that far from feature freeze, i wonder what to do. I would like to get this into oh5 as that would make backporting easier, but there is some time pressure. wdyt @ccutrer ? We could also do mqtt in oh5 and modbus / bluetooth in 5.1

I don't know that we could get even MQTT into OH5. We're 2 weeks out from feature freeze. As far as even just MQTT, I feel it's much more important to get the GraalPy PR(s) in first, since they bring several bug fixes, especially since current versions of Zigbee2MQTT cause a ton of log spam without these fixes. I can't predict if I'll have much time before then. I also worry that splitting the bindings will take additional coordination with core and/or UIs that will slow down implementation. Not to mention the potential for unforeseen issues that can't really be found until stuff is merged and a full distro snapshot build is available.

Even for reviewers' available time, I think it would be more valuably spent getting say the ZWaveJS binding and the aforementioned GraalPy PRs in.

[lsiepel]

You are probably right. Let’s focus on those. Nonetheless I would appreciate more feedback from others so we can focus on getting this in 5.1

[rkoshak]

As i see 4 rockets and no comment, i think this proposal is as good as it gets. But feedback from @holgerfriedrich , @rlkoshak, @jlaur and anyone else who can add value to the discussion would be appreciated.

Unfortunately my usernames are different in Github and on the forum. It's a long story.

I don't really have anything to add here. This doesn't really seem like it has a really-big impact on the end users. I think splitting each sub-binding out into its own binding would be an improvement overall.

I only ask that upgradeTool be updated to install the missing add-ons during the upgrade to ensure this doesn't become a big breaking change. Though I can see good arguments for not doing that and making them install just those bindings they need, not surprising users with stuff that breaks is probably better. I think you could even look at the Things to figure out the minimum set of these new broken out bindings to install so extra bindings may not even need to be installed.

Everything else is code/developer facing and I deliberately try not to talk about that stuff. It's not my place as a non-developer contributor.

[lsiepel]

As i see 4 rockets and no comment, i think this proposal is as good as it gets. But feedback from @holgerfriedrich , @rlkoshak, @jlaur and anyone else who can add value to the discussion would be appreciated.

Unfortunately my usernames are different in Github and on the forum. It's a long story.

I don't really have anything to add here. This doesn't really seem like it has a really-big impact on the end users. I think splitting each sub-binding out into its own binding would be an improvement overall.

I only ask that upgradeTool be updated to install the missing add-ons during the upgrade to ensure this doesn't become a big breaking change. Though I can see good arguments for not doing that and making them install just those bindings they need, not surprising users with stuff that breaks is probably better. I think you could even look at the Things to figure out the minimum set of these new broken out bindings to install so extra bindings may not even need to be installed.

Everything else is code/developer facing and I deliberately try not to talk about that stuff. It's not my place as a non-developer contributor.

It does have some user facing changes. Thing Type Id's might change, lists of Thing's might be smaller.

[rkoshak]

It does have some user facing changes. Thing Type Id's might change, lists of Thing's might be smaller.

Yes, but they are not big changes. upgradeTool should make all the changes necessary for managed config users (i.e. install the new separate add-on and change the appropriate Thing IDs) automatically as part of the upgrade so for managed config users this isn't even a breaking change. File based users signed up to manually manage their config anyway so they'll have to edit their files manually based on the release notes.

For example, if a managed config user is using the Home Assistant MQTT sub-binding, upgradeTool will see that upon upgrade, install the new split off Home Assistant MQTT binding, and change the Thing IDs of the appropriate Things as necessary.

Even without upgradeTool handling this, I don't think the changes are that big where I have any arguments against doing it. It seems like a good idea overall.

[holgerfriedrich]

I don't have a strong opinion on this topic; that is why I did not comment on it before. I have not been involved in the development of those aggregated add-ons, so I have no previous experience with that.

Maybe I can add just a few things that came to my mind:

If we split up and have a dependency on the "base" add-on, this will cause issues with manual plugin installation (as dropping jars in the addon folder always ignores those dependencies). They need to be installed separately. This affects not too many users; most use the bundled versions.

Splitting up would at least reduce the size of the installed add-ons. This is likely only a problem for MQTT, where I have already noticed the size when I was looking into the graalvm issues. Having 100 MB jars might be annoying, but not too bad - as we anyway require 64-bit systems for OH5.

Another pro could be that the plugin list would be closer to the real device that a user wants to add. In OH4, for example, users did not get a result when searching for homie. They needed to find out that the MQTT add-on would do the job. (This might have changed in the meantime, as there were improvements made in this area).

If you think this restructuring brings significant benefit that exceeds the effort, then let's go for it.

[lsiepel]

I do think this would be better in several ways. @ccutrer i think we can continue now

[ccutrer]

If we split up and have a dependency on the "base" add-on, this will cause issues with manual plugin installation (as dropping jars in the addon folder always ignores those dependencies). They need to be installed separately. This affects not too many users; most use the bundled versions.

This will not cause any new issues for manual plugin installation - the sub-bindings have always been separate JARs. It's only the Karaf feature that's aggregated. And I've never been able to get a JAR installation in the addons folder to work correctly if I was trying to install an updated version of just a sub-binding, since it would conflict with the same bundle from the installed feature. I've always just used bundle update in the Karaf console to update the specific bundle I was working on. I think I could probably get things to work now in the addons folder by putting every part of the aggregate binding in there, but why would I? With a split up binding, you could install the base binding in the UI, and then only add the sub-binding JAR you want in the addons folder, and it should work (though you may need to adjust start levels).

I do think this would be better in several ways. @ccutrer i think we can continue now

👍

I'll start working on it when I have time, but for sure won't send a submittable PR until after 5.0 is released.

[lsiepel]

Cool!