[knx] Skip already discovered Things during device discovery

[holgerfriedrich]

Prevents reconfiguration of imported Things when discovery runs. Previously, existing Things were rediscovered and reconfigured to default connection type.

Fixes #19824

[kaikreuzer]

Afair, discovery services should not have a dependency on the thing registry.

Anyhow, I do not think this is the right approach, see also my comment at #19824.

[holgerfriedrich]

@kaikreuzer If I am not allowed to use the ThingRegistry, is there another was to find out if the Thing has been added before?

The other option would be to remove all initial settings.
If I leave out info which could have been changed by a user in the config, only serialNumber would remain.
Even the connection type cannot be detected without further measures, because it is either TUNNEL or ROUTER in the finder result; if secure or not, I cannot tell without further measures. That is why we default to the more common unsecure versions.

Right now, it has useful initial settings for the config, and we would loose that if the content always gets overwritten:

TUNNEL
thingDiscovered(DiscoveryResultBuilder.create(uid).withLabel(response.getDevice().getName())
.withProperty("serialNumber", serial).withProperty("type", "TUNNEL")
.withProperty("ipAddress", "" + response.getControlEndpoint().endpoint().getAddress().getHostAddress())
.withProperty("port", "" + response.getControlEndpoint().endpoint().getPort())
.withRepresentationProperty("serialNumber").build());

ROUTER
thingDiscovered(DiscoveryResultBuilder.create(uid)
.withLabel(response.getDevice().getName() + " (router mode)")
.withProperty("serialNumber", serial + "-r").withProperty("type", "ROUTER")
.withProperty("ipAddress", "224.0.23.12")
.withProperty("port", "" + response.getControlEndpoint().endpoint().getPort())
.withRepresentationProperty("serialNumber").build());

Could you elaborate a bit about why ThingRegistry should not be used here? Do you expect startup issues as ThingRegistry then needs to be initialized before this code is executed?

[kaikreuzer]

Could you elaborate a bit about why ThingRegistry should not be used here?

I don't remember the details atm, but yes, from the design the discovery was meant to be independent of the registries to keep a clean dependency tree and not introduce any circular dependencies.

Also, I would strive for a more general solution that rather works through declaration and not through code.
As just commented on #19824: I could imagine that we introduce a kind of opt-out for a thing type of the automatic syncs. In that case, a binding would only have to add a flag to the thing type description and that's it. The rest would be handled in the core Inbox logic. This would very easily allow any other binding developer to use the feature without having to write code like you did in this PR, which is much more complex and possibly error-prone when implemented by various developers in slightly different ways.

I understand that you would love to see this solved for 5.1, but I'm afraid we ran out of time for that.
Maybe a simple and quick temporary solution would be to remove the representation property from the created inbox result. Afaik, this is used to link an inbox result to the existing thing and if it is not set, the sync mechanism should not chime in.
Would you want to give this a try? We could possibly still squeeze this in, even after the RC today.

[holgerfriedrich]

@kaikreuzer I wrote this while you typed your answer above, so it is now partly outdated - I just want it to be mentioned.

For me it looks like we have two options to go from here.
Either we implement some kind of veto mechanism in core that prevents updates if the veto flag is added. This could be on property level or even on full DiscoveryResult level. For the use-case required here, it would be even fine to update as long as the Thing is still in the Inbox. Once it is configured, the updates should be ignored.

Or we shift this logic to the bindings. Then we end up like here where we need access to the ThingRegistry. We could provide access to it via the AbstractDiscoveryService as we do for the LocaleProvider and TranslationProvider.
Though, I seem to miss the concept behind the architecture.

[holgerfriedrich]

Maybe a simple and quick temporary solution would be to remove the representation property from the created inbox result. Afaik, this is used to link an inbox result to the existing thing and if it is not set, the sync mechanism should not chime in.

Yes, I will give it a try.

[lolodomo]

The dependency with thing registry also shocked me when I saw it this morning.

In that case, a binding would only have to add a flag to the thing type description and that's it. The rest would be handled in the core Inbox logic. This would very easily allow any other binding developer to use the feature without having to write code like you did in this PR

I like the idea.

[holgerfriedrich]

Maybe a simple and quick temporary solution would be to remove the representation property from the created inbox result. Afaik, this is used to link an inbox result to the existing thing and if it is not set, the sync mechanism should not chime in.

I tried, it was not successful. I removed all Things, also from Inbox. Started over. Added the new things, reconfigured security, waited for Thing online, restarted. No success.
I have alternatively removed the property setting the connection type, see #19839. This comes with drawbacks (see other PR), but avoids using the ThingRegistry.

IMHO we should squeeze in one of the two PRs and go for a more general solution involving core after release.

The general solution involving core will most likely also help for the issues @jlaur was mentioning before.

[kaikreuzer]

Many thanks for testing so quickly @holgerfriedrich! Pity that it did not seem to work out, although I am really wondering how the matching works in that case...

If you can live with #19839 until we do a first 5.1 patch release, I would suggest to go with that. It seems to be the least invasive fix we can do right now.

[holgerfriedrich]

I think will need to live with whatever we merge now for the full 5.1 life cycle.
If we go with the core solution and modify the thing type description as you proposed above, this will likely not be backported to 5.1.
I prefer #19828 over #19839 because I don't like that users need to reconfigure the newly added Things to resolve the config error. For most users this will mean to find the Thing and set TUNNEL (or just click save) to make it work. For me this feels like a step back from what we had before.

But yes, the risk might be lower with #19839. And maybe we can find a way to check/probe if security is configured. This would be even better. Something for the next patch release, maybe.

There are 2 PRs out, which one to pick is your call now :-)

[kaikreuzer]

There are 2 PRs out, which one to pick is your call now :-)

I would actually love to hear a preference from any @openhab/add-ons-maintainers here.

[kaikreuzer]

Ok, seems nobody else dares to take any decision here either.
So let me suggest the following: We merge this PR here for 5.1 as a temporary solution with the guarantee that you remove the code afterwards again, when we have a proper solution. I will build a RC2 tonight, so we can test the KNX binding tomorrow morning with it to really make sure that we do not have any issues with circular dependencies.

[kaikreuzer]

Unfortunately, we are not getting a green snapshot build right now (see #19841), which I would have loved to test today.
To be honest, without it, I am not feeling well about merging this PR and would rather tend to #19839...

[kaikreuzer]

Closing in favor of #19839 - to keep the risk as low as possible and not cause any issues with the GA release tomorrow. Thanks for your understanding. 🙏

[jlaur]

Maybe a simple and quick temporary solution would be to remove the representation property from the created inbox result. Afaik, this is used to link an inbox result to the existing thing and if it is not set, the sync mechanism should not chime in.

Unfortunately I didn't have time to jump into this yesterday, so I'm quite late to the party: Actually the mechanism that makes discovery modify existing managed Thing configuration seems to be based on the generated ThingID, not the representation property. That's one of the things that confused me while testing #19516.

I would actually love to hear a preference from any @openhab/add-ons-maintainers here.

Is there anything left to discuss today? I've seen a few PR's briefly, so I'm guessing this is now settled?

[holgerfriedrich]

No, all fine, thanks @jlaur.
We just had two options to proceed.
The handling of (re-)discovery needs to be handled in core anyway, but not as part of 5.1.