The channelLinked method is not called after restart

[robnielsen]

If a channel is dynamically added to a thing, on restart of openHAB the channelLinkedmethod doesn't get called after initial linking of the item to the channel.

If items are defined in a .items file, the channelLinked method will get called if the file is edited or touched after OH3 is up and running.

This is a regression from OH2 and is similar to https://github.com/openhab/openhab-core/issues/1596

[wborn]

What does isLinked(channelUID) return for these dynamic channels in the initialize method?

If it's true the handler will not receive the channelLinked event because the handler is still initializing and the ChannelLinkNotifier added in #1634 will only send the event to initialized handlers.

[robnielsen]

What does isLinked(channelUID) return for these dynamic channels in the initialize method?

It is returning truefor the dynamic channels that are linked to an item and falsefor the ones that are not.

[robnielsen]

This can be easily reproduced with the following, you do not need to have an Insteon device.

1. Install Insteon binding
2. Add Insteon Network bridge and set the port to a value such as /tcp/foo:9761
3. Add Insteon Device, by selecting the bridge above, address 11.22.33, product key F00.00.01
4. Edit Insteon Device that was just created and link a new item to the Dimmer channel.

Now you should have an Insteon network, and Insteon device and a Item linked to the Insteon device.

From the openHAB console run the commands insteon display_devices and insteon display_channels, you should see the device you just defined and the channel that is linked to the item:

openhab> insteon display_devices
insteon:device:74611007c4 address = 11.22.33 productKey = F00.00.01 channels = beep, onLevel, fastOnOff, ledBrightness, lastHeardFrom, ledOnOff, rampRate, dimmer, manualChange
openhab> insteon display_channels
insteon:device:74611007c4:dimmer feature = dimmer parameters = {}
openhab>

Stop openHAB and start openHAB. From the console, run the same commands and you should see something like:

openhab> insteon display_devices
insteon:device:74611007c4 address = 11.22.33 productKey = F00.00.01 channels = beep, onLevel, fastOnOff, ledBrightness, lastHeardFrom, ledOnOff, rampRate, dimmer, manualChange
openhab> insteon display_channels
openhab> 

The data in display_devices is added in the initialize() method, and the data in display_channels is added in the channelLinked(channelUID) method.

[wborn]

The behavior was changed because for most handlers that are initialized calling channelLinked would result in an unnecessary REFRESH command. See https://github.com/openhab/openhab-core/pull/1385#issuecomment-641558762 I think it should be possible to fix it in the handler by executing the same logic based on what isLinked(channelUID) returns in the initialize method?

[robnielsen]

@wborn, yes the handler could be modified as you described. But this is a breaking change in functionality from OH2 to OH3, and more than likely will be a difficult track down when channelLinked is called sometimes and sometimes it isn't.

[wborn]

We did not know how breaking the change would be so it seems this is the first case. If there are only a couple of bindings impacted I think the breaking change still makes sense and we should add some docs to those methods to document when they are called. WDYT @openhab/core-maintainers ?

[kaikreuzer]

I'd agree with @wborn.

[robnielsen]

@openhab/core-maintainers, If the decision is made to keep this breaking change, please reach out to the code owners of the impacted bindings. A search for the channelLinkedmethod results in 46 handlers in 32 bindings that override the channelLinked method.

[robnielsen]

I tried @wborn suggestion and it works. Specifically, I added something like:

getThing().getChannels().forEach(channel -> {
    if (isLinked(channel.getUID())) {
        channelLinked(channel.getUID());
    }
});

I hope the decision is to fix this, not fixing it feels wrong. Also, at a minimum, the channelLinked javadoc will need to be updated to indicate that this might not get called and can not be relied on.

[robnielsen]

I don't think this has anything to do with the dynamically added channels. There is almost a minute delay between starting the first binding to the third one:

2020-10-20 08:13:33.288 [INFO ] [me.event.ThingStatusInfoChangedEvent] - Thing 'astro:sun:home' changed from UNINITIALIZED to INITIALIZING
2020-10-20 08:13:40.088 [INFO ] [me.event.ThingStatusInfoChangedEvent] - Thing 'ecobee:account:account' changed from UNINITIALIZED to INITIALIZING
2020-10-20 08:14:31.072 [INFO ] [me.event.ThingStatusInfoChangedEvent] - Thing 'insteon:network:home' changed from UNINITIALIZED to INITIALIZING
2020-10-20 08:14:33.604 [INFO ] [me.event.ThingStatusInfoChangedEvent] - Thing 'netatmo:netatmoapi:home' changed from UNINITIALIZED to INITIALIZING

Insteon does depend on the openhab-transport-serial feature.

[lolodomo]

Would it be possible to create an issue to list and follow all bindings that should be checked due to the change in the core framework?

[cdjackson]

It seems like a bad idea to leave this method only working at certain times - doesn't it? I've not followed all the links to work out the reason this was changed, but looking at the comment from @wborn above, it seems to be related to preventing extra REFRESH commands during startup?

I've also not looked at how all this is implemented, so please forgive my ignorance, but maybe this should be written so that these methods (ie the REFRESH command, and the channelLinked method) should simply be called after the handler is initialised - ie do nothing beforehand, but once it initialises, then do both things? Would this be a suitable way to ensure that bindings always get called, and there are not multiple refreshes, and everything is done in a defined order?

[robnielsen]

@cdjackson, I agree. The workaround I had to add to the Insteon binding is to bascially call the channelLinked method for linked channels in the initialize method. Then in the channelLinked method, I have a check to see if it's been called before, and if it has it immediately returns.

[lolodomo]

Just reading the code of the Astro binding, I already know that this binding is partially broken by this change ! Some refresh jobs will no more be started at OH startup and some channels will no more be updated. @clinique : I know that you are well aware about the Astro binding, could you have a look and see if there is an alternative ? I already imagine an alternative would be to start the jobs anyway (even if the user has no linked channels requiring this refresh jobs) ?

[lolodomo]

@clinique: the Netatmo binding is also impacted, the list of "measurable channels" will no more be built ! There is something I do no more understand in the code, as MEASURABLE_CHANNELS is an empty list, measuredChannels will always be empty ?

[lolodomo]

After more investigation and testing, the hue binding is not impacted. We have the polling jobs run regularly which are setting the channels. So the binding does not rely on channelLinked to nitialize the channels.

[lolodomo]

Thinking about it, the change could affect any binding relying on channelLinked to trigger the refresh of a channel. Even if a binding does not override channelLinked, it could be affected.

[cweitkamp]

It looks like the avmfritz binding is affected too. At least I am not able to get the Call monitor running in OH3 which afair depends on this feature too.

[MHerbst]

As far as I can see, the Homematic binding is also affected. It implements the channelLinked method to request the current values of all linked channels at startup.

[lolodomo]

@cdjackson : does the Z-Wave binding rely on call to channelLinked to initialize the channels ?

[mckennajp]

This bug is a blocker for me migrating to OH3. The Z-Wave binding polling does not work on my configuration because ZWaveThingHandler::channelLinked is not getting called on startup.

[kaikreuzer]

To me this is not a bug but a feature - imho, OH2 was buggy by calling this method unnecessarily often (and yes, as a result of that, a couple of bindings unfortunately decided to rely on that behavior).

It should be pretty easy for any thing handler to iterate over the list of linked channels within the initialize() method. This will be in any case much more efficient than relying on a dozen independent calls of channelLinked(). I'd thus strongly suggest to fix the concerned bindings.

Wrt the docs, they state "This method is only called, if the thing has been initialized" - it nowhere states that it is called retro-actively for any event that happened before initialization. But yes, if this can be misinterpreted (especially driven through the OH2 behavior), we can add a more precise explanation there.

[robnielsen]

@kaikreuzer, are you planning on reaching out to the owners of bindings that use channelLinked? Hopefully, you will. I also suggested this above.

[cdjackson]

OH2 was buggy by calling this method unnecessarily often

Maybe, but that's not relevant. If I understand correctly, the only buggy part is that the REFRESH command got called too many times. I think we can agree that shouldn't be the case, but that is not related to this issue.

a couple of bindings unfortunately decided to rely on that behavior

You make out like binding developers are doing something wrong, but in the past, even if not now, this was the case that this that this call WAS meant to be called after initialisation -:

https://github.com/eclipse-archived/smarthome/pull/1890

It should be pretty easy for any thing handler to iterate over the list of linked channels within the initialize() method.

True, but that requires a change to the way things work, and as we see, it has broken things, and adding code to loads of bindings which doesn't seem sensible.

Wrt the docs, they state "This method is only called, if the thing has been initialized" - it nowhere states that it is called retro-actively for any event that happened before initialization

Maybe it doesn't now, but as I showed above - it looks like it used to - you even implemented this yourself.

[kaikreuzer]

Maybe it doesn't now, but as I showed above - it looks like it used to - you even implemented this yourself.

Good catch - so it was indeed documented in the developer guide that it works that way.

Well, if everyone prefers to keep it that way, I can be easily convinced as well. The problem remains that I wouldn't know how to implement it and we would need somebody to do it. (Note that it was not removed and hence could be re-added, but it disappeared by a major refactoring that cannot be undone.)

The simpler solution might therefore still be to adapt the bindings that now have a problem, because the solution is pretty simple in that case.

are you planning on reaching out to the owners of bindings that use channelLinked?

It is probably much easier if a volunteer goes through the code and identifies places that might have an issue. I'd assume that for the majority of relevant bindings, but reports are already there and possibly fixed, since quite some people are testing OH3 in their environments already.

@openhab/core-maintainers Any preference on whether to close this as "won't fix" and try to fix impacted bindings or rather trying to find a fix for it?

[cdjackson]

The problem remains that I wouldn't know how to implement it

Isn't it as simple as iterating over the channels when the thing status changes from !ONLINE to ONLINE, and then calling channelLinked() - exactly as you already suggested above?

[lolodomo]

As I understood, everything is frozen in 7 days and the release is planned in 15 days. I can't imagine OH3 to be released with a "broken" Z-Wave binding ! What is the final plan ?

[kaikreuzer]

Isn't it as simple as iterating over the channels

Yes, I think it is - but I wouldn't know a good and safe place in the core code (and do not have the time to figure that out) and as mentioned above, I'd think it would be the sub-optimal solution for bindings.

@cdjackson I would hence very much appreciate if you'd do this fix in the Z-Wave handler code, so that the risk of broken Z-Wave support can be mitigated.

[kaikreuzer]

@cdjackson Not sure if you have seen my last message. Is this something you could and would do? If not, we need a plan B and our time is running out quickly.

[cdjackson]

Sorry - I had not seen this. My email was down for a few days and I lost a lot of mail (and 2 servers!).

I’m probably not going to have the chance to look at this for the next week or so - if someone else can come up with a PR, that would be good.

[kaikreuzer]

I could try to do a PR, but I cannot do any testing, so that would be far from ideal. Do we have anyone that is a Z-Wave user and could try to do an according PR against https://github.com/openhab/org.openhab.binding.zwave?

[cdjackson]

I will try and find some time to look at this for ZWave and ZigBee on Thursday or Friday.

[vak-git]

It looks like the MCP23017 binding is affected too in OH3. I think channelLinked calls would need to be added to initialize() method of this binding.

[openhab-bot]

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

https://community.openhab.org/t/oh3-channellinked-and-channelunlinked-don-t-get-called/128745/2

[J-N-K]

After think a while about this, I doubt it's correct to call channelLinked on the !ONLINE to ONLINE transition. This would result in a call every time the thing status changes and that's not what I would expect. So maybe a better place would be when transitioning from INITIALIZING to an initialized state (UNKNOWN, ONLINE, OFFLINE). This should happen online once.

A good place would IMO be ThingManagerImpl.ThingHandleCallback.statusUpdated with a check if it is the correct transition.

[openhab-bot]

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

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