removed ThingLinkManager and with it the auto-linking feature and "Simple Mode"

[kaikreuzer]

@ghys has added great support in the new UI for link management without requiring this feature, which caused a lot of problems in 2.x.

Signed-off-by: Kai Kreuzer kai@openhab.org

[wborn]

LGTM and thanks!

[openhab-bot]

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

https://community.openhab.org/t/channels-not-responding/96712/11

[splatch]

FYI - I was looking for the calls of channelLinked and channelUnlinked methods in 3.x APIs and now they're gone. ThingHandler is still permitted to have these, but they never get called.

[kaikreuzer]

@splatch That's interesting. I guess you are right, I removed a bit too much with this PR - I thought that the class only deals with "Thing links", while it also covered parts of the "Item links". Will see how to best revert that part...

[splatch]

I managed to get notifications back via registry listener, however I have impression that it is not a valid way. First of all - it is not attached to Thing and its handler lifecycle but to the link presence.

@Component(immediate = true)
public class ChannelLinkNotifier implements RegistryChangeListener<ItemChannelLink> {

    private final Logger logger = LoggerFactory.getLogger(ChannelLinkNotifier.class);
    private final ItemChannelLinkRegistry itemChannelLinkRegistry;
    private final ThingRegistry thingRegistry;

    @Activate
    public ChannelLinkNotifier(@Reference ItemChannelLinkRegistry itemChannelLinkRegistry, @Reference ThingRegistry thingRegistry) {
        this.itemChannelLinkRegistry = itemChannelLinkRegistry;
        this.thingRegistry = thingRegistry;

        // registry do not dispatch notifications about existing links to listeners
        itemChannelLinkRegistry.getAll().forEach(this::added);
        itemChannelLinkRegistry.addRegistryChangeListener(this);
    }

    @Override
    public void added(ItemChannelLink element) {
        ChannelUID channelUID = element.getLinkedUID();
        ThingUID thingUID = channelUID.getThingUID();

        call(thingUID, handler -> handler.channelLinked(channelUID), "channelLinked");
    }

    @Override
    public void removed(ItemChannelLink element) {
        ChannelUID channelUID = element.getLinkedUID();
        ThingUID thingUID = channelUID.getThingUID();

        call(thingUID, handler -> handler.channelUnlinked(channelUID), "channelUnlinked");
    }

    protected void call(ThingUID thingUID, Consumer<ThingHandler> consumer, String method) {
        // Nullability checks can't understand Java 8 Optional semantics.. in 2020! What a shame!
        Thing thing = thingRegistry.get(thingUID);
        if (thing != null) {
            ThingHandler handler = thing.getHandler();
            if (handler != null && ThingHandlerHelper.isHandlerInitialized(handler)) {
                consumer.accept(handler);
            } else {
                logger.info("Skipping notification to thing {} handler {} method call, as it is not initialized",
                        thingUID, method);
            }
        }
    }

    @Override
    public void updated(ItemChannelLink oldElement, ItemChannelLink element) {
        removed(oldElement);
        added(element);
    }
}

[kaikreuzer]

@splatch I quite like your ChannelLinkNotifier - what problems do you see with it that you call it "not a valid way"? Basing it on the lifecycle of the links sounds like a good approach to me.
Would you want to create a PR with that?

[splatch]

@kaikreuzer from what I have observed it is not called when thing/thing handler is restarted. This leaves a gap in case of configuration updates. It gets triggered only if link is recreated. Essentially this listener needs to be a combined one.

[kaikreuzer]

It gets triggered only if link is recreated.

Hm, right, I remember. The ThingLinkManager was implemented as a tracker, i.e. it pushed ALL linked channels to newly appearing listeners (thing handlers). This was mainly for the initialization and results in a "REFRESH" command being sent for every linked channel at startup (by the default implementation of channelLinked).

Thinking about it, I really wonder if we need to keep that behavior in place. It would somehow make more sense to me to only call ´channelLinked` on active handlers and not for all already linked channels at startup. I haven't really experienced any bindings that rely on this behavior in any way, but I also cannot tell, what might break if we leave it away. But there's also the chance that it even improves the overall behavior as far less REFRESH commands will be flying around during startup...

@openhab/core-maintainers What are your thoughts on this? Shall we dare to change this behaviour?

[cdjackson]

Why not remove the default implementation which creates the REFRESH command, but leave the method. This allows bindings to know when channels are linked since this information is useful so we know if we need to maintain the channel (eg to initialise polling), but removes your concern about too many REFRESH commands on startup, which I agree is possibly not nice.

[kaikreuzer]

The default implementation definitely has to stay as it is required for any active handler - I am very certain that many of not most bindings will break if we remove it and I also do not see a reason for it to be removed.
The discussion here is merely around calling channelLinked at startup time of a handler, see the comments above.

[cdjackson]

I didn't mean to actually remove the method itself, I meant just to remove the call to REFRESH - that would allow bindings to implement a refresh if they need, but not force it.

Anyway, that aside I did misread this - I thought it was about removing the call completely which would definitely not be good. I'm ok with it not being called on startup.

[wborn]

What are your thoughts on this?

It's worth a try. I haven't looked at the code, but if handlers only implement refresh commands and not channellinked calls the data may not be properly refreshed.

[splatch]

I use it in our bacnet v2 binding to start polling for data so I rely on old behavior. I see no issues migrating to new behavior, once it is defined.
Cause most of the bindings do not have own persistence and rely on configuration delivered by framework it makes sense for framework to care about handler lifecycle too. Making this functionality unrelated to thing handler will force bindings, which rely on presence of links, to do additional work.

[cweitkamp]

I think we can omit the REFRESH commands. I cannot remember a binding which relies on that. Just the opposite it would be much easier to handle REFRESH commands for a binding developer because he has not to be aware of the flood during restart and does not have to put in workarounds to handle them.

On the other hand changing the channelLinked feature could may result in some unexpected behavior. A quick search for it reveals nearly 47 places. Do not know if all them are relevant or even necessary - but a small risk will remain.

[kaikreuzer]

@cweitkamp Just to clarify again: As mentioned above, the suggestion here is merely to change the behaviour at handler startup and omit sending REFRESH commands for every linked channel.
The behavior AFTER startup will stay exactly as it is today, nothing will change about channelLinked being called whenever new links are created.

[openhab-bot]

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

https://community.openhab.org/t/moving-from-oh-this-is-no-rant/112194/42

[openhab-bot]

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

https://community.openhab.org/t/zwave-polling-unlinked-channels/152554/3