Improve thing initialization and enable thing-type updates

[J-N-K]

Closes #1924

This

• moves config description URI to type-base class
• decouples the thing manager from bundle loading
• makes sure that all thing/channel-types and config descriptions are available when the thing is initialized
• enables thing type updates

Supersedes #3191

Signed-off-by: Jan N. Klug github@klug.nrw

[jlaur]

@J-N-K - do you know if this will also fix this issue that I'm seeing from time to time:

2023-01-21 09:31:08.326 [WARN ] [core.thing.internal.ThingManagerImpl] - No config description for 'thing-type:energidataservice:service' found when normalizing configuration for 'energidataservice:service:energidataservice'. This is probably a bug.

It sounds very much like it, just seeking confirmation.

[J-N-K]

Yes.

[kaikreuzer]

FTR: I have started reviewing this PR today, but I will need a few more days, since it is a lot of code and it is touching the core mechanisms - but so far all looks good. 👍

[kaikreuzer]

Many thanks, @J-N-K, this looks overall pretty good!
Please find my review comments below. I went through the code, but didn't yet do any extensive testing, though - but I guess that can be done once merged, we should just not do it right before releasing a milestone.

[kaikreuzer]

Might it make sense to make this configurable for the user?

[J-N-K]

I'm not sure. In my original version I did not have this at all, but while testing I realized that the systeminfo binding does something really strange: It has a seperate thing-type for each thing with a ThingTypeUID systeminfo:computer-mba-jan-wifi_janessa_me where the last part is the id-part of the ThingUID. The factory reports "supported" for every ThingTypeUID that starts with systeminfo:computer-. The ThingType itself added dynamically to the ThingTypeRegistry during the initialization of the thing, so at the time the ThingManager checks if everything needed to initialize the thing is present in the registries it fails. IMO this is a faulty behavior of the binding, if it dynamically creates ThingTypes it should add those to the registry and make sure that they are present when a factory reports they are supported. WDYT?

[kaikreuzer]

Yes, I agree that this sounds like a pretty weird behavior. Did you find out why at all different types are created for all things? That does not seem to make much sense...

[mherwege]

The reason for this in the systeminfo binding to have thing types by thing is that thing specific channel groups are added. In my understanding, this has to be done on the thing type level. E.g. a system with 5 harddisks will get 5 storage channel groups created dynamically. This cannot be defined upfront across all things.
I am open for alternative suggestions for this if it avoids complexity in core. There are probably other bindings doing this for the same reason though.

[J-N-K]

Digitalstrom, Homematic, MQTT, Netatmo and Systeminfo use ThingTypeProvider, I did not check in which way. IMO having different thing-types is fine (also your specific case should be handled by dynamic channel addition), my problem is that you report support for a given ThingTypeUID but you don't add the ThingType to the ThingTypeRegistry.

[kaikreuzer]

The issue might be that the specific thing types can only be determined/defined as soon as thing handlers become active - and then it feels too late for adding them to the ThingTypeRegistry.
But let's not try to solve this here as part of this PR, but rather live with the implementation you have done. If this causes issues or we have good ideas how to refactor the system info binding, we can any time address this and simplify the logic.

[kaikreuzer]

Might it make sense to put the update folder also underneath OH-INF as this is where all other metadata resides?

[kaikreuzer]

Another thought, looking at our OH-INF folder: So far, all metadata that binding developers are expected to deliver are XML files (and property files, but those are typically generated).

From a consistency point of view: Might it make sense to also switch to XML for the update information and provide a proper XSD for them as well?

[J-N-K]

I agree on moving to the OH-INF folder, that avoids future collisions.

I'm not so sure regarding the XML (although I see the benefit of validating the content with the XSD and having proper documentation). The reason is that parsing XML with XStream is really a PITA. If we can agree on using Jakarta XML, I'm fine with it.

[J-N-K]

<?xml version="1.0" encoding="UTF-8" standalone="yes" ?>
<update:update-descriptions xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
	xmlns:update="https://openhab.org/schemas/update-description/v1.0.0"
	xsi:schemaLocation="https://openhab.org/schemas/update-description/v1.0.0 https://openhab.org/schemas/update-description-1.0.0.xsd">

	<thing-type uid="testBinding:testThingTypeAdd">
		<instruction-set targetVersion="1">
			<add-channel id="testChannel1">
				<channelTypeUid>testBinding:testChannelTypeId</channelTypeUid>
			</add-channel>
			<add-channel id="testChannel2">
				<channelTypeUid>testBinding:testChannelTypeId</channelTypeUid>
				<label>Test Label</label>
			</add-channel>
			<add-channel id="testChannel3">
				<channelTypeUid>testBinding:testChannelTypeId</channelTypeUid>
				<label>Test Label</label>
				<description>Test Description</description>
			</add-channel>
		</instruction-set>
	</thing-type>

	<thing-type uid="testBinding:testThingTypeRemove">
		<instruction-set targetVersion="1">
			<remove-channel id="testChannel"/>
		</instruction-set>
	</thing-type>

	<thing-type uid="testBinding:testThingTypeUpdate">
		<instruction-set targetVersion="1">
			<update-channel id="testChannel">
				<channelTypeUid>testBinding:testChannelNewTypeId</channelTypeUid>
				<label>New Test Label</label>
			</update-channel>
		</instruction-set>
	</thing-type>

</update:update-descriptions>

WDYT?

[kaikreuzer]

Much more boilerplate, but that's what comes with XML. I like the fact that we can easily check it against XSDs.

I agree that parsing with XStream is no fun, I'll have a look at your Jakarta implementation.

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/openhab-4-0-snapshot-discussion/142322/152

[kaikreuzer]

Wow, you're incredibly fast - thanks!
I think you have addressed all my comments and I am very happy about the result - so let's wait for the build to succeed and we can merge. 👍

[wborn]

Looks like these changes break the add-ons build, see:

• https://ci.openhab.org/job/openHAB-Addons/985/
• /~https://github.com/openhab/openhab-addons/actions/runs/4196737262/jobs/7278112750

If I revert these changes locally the tests succeed again. Can you have a look @J-N-K?

[wborn]

A more descriptive title would also be nice for the release notes and if it is resolved you can claim the bounty. 😉

[J-N-K]

Looks like these changes break the add-ons build, see:

• https://ci.openhab.org/job/openHAB-Addons/985/
• /~https://github.com/openhab/openhab-addons/actions/runs/4196737262/jobs/7278112750

If I revert these changes locally the tests succeed again. Can you have a look @J-N-K?

See openhab/openhab-addons#14431

[jlaur]

@J-N-K - I guess I have to be the one asking the stupid question. 😊 I understand from openhab/openhab-addons#14447 (comment) that update instructions are needed when adding new channels. What is the reason that this cannot be auto-detected? Performance? I can understand more complex scenarios needing instructions, but it seems to me that if a channel doesn't exist in jsondb, but is declared in the binding, then it must be added? I haven't looked into the jsondb data model as I'm personally using files, so there might be a simple explanation.

[J-N-K]

There are different reason:

• A missing channel (in the thing, existing in the XML) could have been removed by the thing itself (because it is not available). This is the reverse action of dynamically adding channels (which is more common) but still a valid option.
• An existing channel (in the thing, but missing in the XML) could have been dynamically added by the thing itself or be an extensible channel that has been added by the user.
• A channel could change (e.g. the channel-type changes from deconz:switch to system:power, the label and/or description could change).

We have tried for five years to find a solution that autodetects such changes (there have been several attempts here and in ESH), but there have always been issues (which are not absolute corner-cases) where an automatic solution failed. Especially the first two points make it very hard and the solution with the update instruction is far more fail-safe - it requires a deliberate action of the developer to change existing things.

[jlaur]

@J-N-K - I see, thanks for the summary/additional context.

[lolodomo]

We have now to promote this new feature before merging changes in add-ons.
But I assume it remains only an option ?

This feature only concerns things created with MainUI and of course not things defined in things file; I am right ?

[J-N-K]

Technically it is ab "option". But IMO there should be policy to upgrade everything that is possible to make a smoother user experience.

[lolodomo]

@J-N-K : is there a special syntax to mention the adding or removal of a thing configuration parameter ? Or is it just unnecessary to consider that ?

[J-N-K]

It is unnecessary because that depends on the config description and config descriptions are always build when the bundle loads/reloads, so they are automatically updated. The database only stores the config values, so the worst thing that could happen is a value that is no longer used.

For managed things the channels are stored in the database and the thing description is only used when creating the thing and from then on only "restored" from the database (as opposed to the file-base things, there the thing is always "deleted" and "created" when the file is unloaded/loaded).

[lolodomo]

Ok.
And if there is a change of the channel type (change of UID), is it necessary to provide something or not ?

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/how-to-notify-user-about-deprecations-properly/145533/5

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/how-to-implement-thing-upgrades/148290/2