bridgeHandlerInitialized() behaviour has changed

kgoderis commented 8 years ago

I am in the process of upgrading some of my earlier PR to the latest APIs. I have noticed that the behaviour of bridgeHandlerInitialized has changed:

bridgeHandlerInitialized is not called upon Things that are initialized after that their Bridge came ONLINE. This used to be different in the past, with bridgeHandlerInitialized being called irrespective of the time of initialisation of the Bridge.

Is it now up to the Thing to verify whether its Bridge is initialised? e.g. call thingIsInitialized() on the Bridge?

Furthermore, in the IDE, when deliberately delaying the initialisation of the Bridge, I noticed that bridgeHandlerInitialized is called twice on the Things belonging to the Bridge, when the initialisation is continued on the Bridge. Is that normal? Should Things now keep track themselves if bridgeHandlerInitialized got called on them?

Tx Karel

kaikreuzer commented 8 years ago

@sbussweiler Could you comment?

sbussweiler commented 8 years ago

It's NOT up to the Thing to verify whether the Bridge has been intitialized or not. The ThingHandler will be notified about it. I will check it tomorrow in detail and give you feedback.

kgoderis commented 8 years ago

ok - FYI, I reproduce this via the IDE where I can time or sequence the initialisation of the ThingHandler and the BridgeHandler. I have some logs if you wan to, but if the Bridge is up and running before the Thing, then the method never got called on the Thing....

sbussweiler commented 8 years ago

There is a test case which tests this scenario, see here.

Anyway, I will test it tomorrow manually. Some logs will be good as well.

kaikreuzer commented 8 years ago

Didn't we also have a discussion with @marcelrv regarding replacing bridgeHandlerInitialized by something like bridgeStatusChanged or the like? I cannot find a link to this discussion right now, but maybe this should be considered here as well.

cdjackson commented 8 years ago

This discussion was on the forum - possibly linked to the thing lifecycle discussion… I was reading through this a week or so back as I see the same issue that Karel has and had to put a hack into my bridge to call the bridgeHandlerInitialized method when it came online…

On 23 Feb 2016, at 16:22, Kai Kreuzer notifications@github.com wrote:

Didn't we also have a discussion with @marcelrv https://github.com/marcelrv regarding replacing bridgeHandlerInitialized by something like bridgeStatusChanged or the like? I cannot find a link to this discussion right now, but maybe this should be considered here as well.

— Reply to this email directly or view it on GitHub https://github.com/eclipse/smarthome/issues/1075#issuecomment-187771115.

kgoderis commented 8 years ago

@sbussweiler should you not separate the test it into 2 different tests, with Bridge followed by Thing, and Thing followed by Bridge apart?

Anyways, the code of the (KNX) binding worked in the old snapshot, now it behaves somewhat differently. FYI, switch between the 2 scenarios in the IDE by letting the safeCall run past 5000ms.

marcelrv commented 8 years ago

@kaikreuzer yes, the discussion was here: https://www.eclipse.org/forums/index.php/t/1073429/

So the PR that changed the behavior was probably https://github.com/eclipse/smarthome/pull/769 as that one made changes to the lifecycle.

kgoderis commented 8 years ago

From the discussion on the forum:

"ThingHandler.bridgeHandlerInitialized() is called when the status of the bridge has been changed from INITIALIZING to ONLINE/OFFLINE (not for each ONLINE/OFFLINE status change). The status ONLINE/OFFLINE is set by the binding developer. The status change from INITIALIZING to ONLINE/OFFLINE signals the framework that the handler has been initialized. If the thing is a bridge, the child-things will be notified via ThingHandler.bridgeHandlerInitialized() about the bridge initialisation."

Well, if this is true, then it is logical that bridgeHandlerInitialized is not called for Things that are initialised after that the bridge came online. So, cfr that same thread, we either have to foresee some specific Bridge/Thing notification interface/mechanism, or make the ThingHandler EventSubscribers to listen for events on the bus, which IMO is a bit overkill for "intra-binding" stuff. Both of these things should then be provided in the BaseThingHandler....

In my use case (KNX), it is the bridge that makes the connection to the KNX bus, and thus, child things only come online when the connection to the physical knx bridge is successfully set up. So, it needs a kind of OFFLINE/ONLINE notification.

sbussweiler commented 8 years ago

"ThingHandler.bridgeHandlerInitialized() is called when the status of the bridge has been changed from INITIALIZING to ONLINE/OFFLINE (not for each ONLINE/OFFLINE status change). The status ONLINE/OFFLINE is set by the binding developer. The status change from INITIALIZING to ONLINE/OFFLINE signals the framework that the handler has been initialized. If the thing is a bridge, the child-things will be notified via ThingHandler.bridgeHandlerInitialized() about the bridge initialisation."

Thats exactly what the ThingManager does in line 190.

In addition to that, ThingHandler.bridgeHandlerInitialized() is also called for Things that are initialized after the Bridge has been initialized. The ThingManager does it in line 197.

An automated test case tests both scenarios. I also reproduced the expected behavior manually and it works. Maybe it's an timing issue because you mentioned that I should do something like safeCall with more than 5000ms?

Either we spend more time in analysis why it did not work with your plugin or we focus a complete new solution. We can remove the methods bridgeHandlerInitialized() and bridgeHandlerDisposed() from the ThingHandler interface and introduce a new one, e.g. bridgeStatusChanged(). This method is called for every Bridge status change. The BaseThingHandler gets an empty method implementation. A concrete ThingHandler can implement the method and can prompt any kind of process, triggered by a Bridge status change.

What do you think, @kgoderis and @marcelrv?

kgoderis commented 8 years ago

I will have a second look at the KNX binding, but I seem not to be alone with this case. Something fishy must be going on despite the successful test cases. I will also study the code and change it a bit in my repo to compile some TRACE output

kgoderis commented 8 years ago

@sbussweiler @kaikreuzer Ok - This is something that I do not get. In the new IDE setup, even when the plugins are added from all the repo's incl ESH,the runtime suprisingly still refers to the TP definition and not the plugin's themselves. In my case, the TP was pointing to 0.8.0.201602151840, which is not the latest. I was under the impression that the TP would be updated/reload automatically, but this is not the case apparently.

When doing a "reload" of the TP, it was set to 0.8.0.201602231715, and suddenly the issue with bridgeHandlerInitialized() went away. The PR #769 mentioned here above was merged on Jan 21st, which is from before the TP of Feb 15th (on which I have the issue). Since that date there were only minimal changes to ThingManager.java (https://github.com/eclipse/smarthome/commit/25b7d3f94224e2f86d623fec9d1011aaef00eb25 and https://github.com/eclipse/smarthome/commit/77ef13af851be6ba0bc3bee0d8d96e7f5bfaa6f0), I am puzzled as to what made the problem go away.

sbussweiler commented 8 years ago

Your mentioned PR 25b7d3f moved the ThingHandler initialization into a separate thread and fixed your problem with a long running initialization. Before this change a long running ThingHandler.initialize() call returned with a TimeoutException and ThingHandler.bridgeHandlerInitialized() was never called.

kaikreuzer commented 8 years ago

So @kgoderis no issue left with the bridgeHandlerInitialized() call then?

@kgoderis & @marcelrv What about @sbussweiler's suggestion to implement a new bridgeStatusChanged() method? Imho we should go for it (while keeping bridgeHandlerInitialized() in place to guarantee backward compatibility).

kgoderis commented 8 years ago

Good suggestion because I also have a few bindings with a bespoke listener interface to just deal with this

cdjackson commented 8 years ago

I would also be very keen to see this implemented...

sbussweiler commented 8 years ago

@kaikreuzer, @kgoderis and @cdjackson please have a look on PR #1142.

kaikreuzer commented 8 years ago

Thanks a lot @sbussweiler, so I close this issue now.

marcelrv commented 8 years ago

thanks @sbussweiler that helps a lot of bindings!

cdjackson commented 8 years ago

Can I clarify how this is now meant to work...

My understanding is that we should always get the bridgeHandlerInitialized() method called, even when a thing is added after the bridge is online? Is this correct? This was @kgoderis initial comment in this issue.

Now with the new bridgeStatusChanged method, do I also expect to see this being called for a newly added thing? eg if my bridge is online, and I add a new thing, I would expect to be called with bridgeStatusChanged(ONLINE)? Is this correct or should I continue to use both the bridgeStatusChanged and bridgeHandlerInitialized methods?

Currently, I'm seeing neither of these methods being called when I add a new thing in Z-Wave. Previously I had a function in my bridge to handle this by calling bridgeHandlerInitialized in the things when the bridge status changed - Ive now removed this as I expected the framework would now do it for me?

If the framework is meant to make these calls, then one point to note that may contribute is I change the thing type almost immediately on adding a new thing. I'm not sure if this is contributing - I would assume not as the thing gets re-created after its type changes (right?).

Can someone confirm how this is meant to work - the lifecycle documentation doesn't show these calls during initialisation, and only says that we are notified of changes - it doesn't mention if it is always called.

sbussweiler commented 8 years ago

My understanding is that we should always get the bridgeHandlerInitialized() method called, even when a thing is added after the bridge is online? Is this correct? This was @kgoderis initial comment in this issue.

This is correct. If a thing has been initialized (ThingHandler.initialize() method is called, thing status is ONLINE/OFFLINE) after bridge initialization, ThingHandler.bridgeHandlerInitialized() should be called.

Now with the new bridgeStatusChanged method, do I also expect to see this being called for a newly added thing? eg if my bridge is online, and I add a new thing, I would expect to be called with bridgeStatusChanged(ONLINE)? Is this correct

This is not correct. A thing has been added, but the status of the bridge is still the same. Due to the fact that the status of the bridge has not been changed bridgeStatusChanged() is not called.

or should I continue to use both the bridgeStatusChanged and bridgeHandlerInitialized methods?

... based on the mentioned behavior you have to use both methods. If this behavior is not applicable, we have to discuss it.

the lifecycle documentation doesn't show these calls during initialisation, and only says that we are notified of changes

We should extend our documentation.

cdjackson commented 8 years ago

If a thing has been initialized (ThingHandler.initialize() method is called, thing status is ONLINE/OFFLINE)

Ok - this might be the issue. Currently I assume that the thing isn't ONLINE until the bridge handler is initialised, so we have a "chicken and egg" situation... The framework doesn't call the bridgeHandlerInitialized method because the thing is not online, and I don't put the thing online until bridgeHandlerInitialized has been called. I do it this way as I need to access methods in the bridge before I can find the state of the thing...

I guess there are two solutions here...

One is to set the thing to OFFLINE in the initialize method - then the framework will call my bridgeHandlerInitialized method? Is that correct?
Second would be for the framework to always call bridgeHandlerInitialized even if the thing is not ONLINE or OFFLINE (which is what I had previously assumed).

This probably should be documented too :)

This is not correct. A thing has been added, but the status of the bridge is still the same

Fine - that's just my misunderstanding - I thought that the new method was going to have the same functionality, or replace the bridgeHandlerInitialized method. No problem.

based on the mentioned behavior you have to use both methods. If this behavior is not applicable, we have to discuss it.

It's fine - just so long as the documentation on the lifecycle is clear...

sbussweiler commented 8 years ago

One is to set the thing to OFFLINE in the initialize method - then the framework will call my bridgeHandlerInitialized method? Is that correct?

That's correct. This is how you can proceed and to solve the issue in the binding. Means, in your implementation ThingHandler.initialize() is called but you don't change the thing status?

The method ThingHandler.initialize() is called in order to initialize the thing and only things in status ONLINE / OFFLINE are considered as "initialized". The documentation says that in this method the handler has to set the status to ONLINE resp. OFFLINE.

cdjackson commented 8 years ago

Means, in your implementation ThingHandler.initialize() is called but you don't change the thing status?

Correct. I assume that the device is still initialising, and as I'm not allowed to set INITIALIZING state only ONLINE/OFFLINE) I leave it like this until I can find the state (which requires the bridge). I'll change this and set to OFFLINE - this might solve a few issues :)

Thanks for the clarifications....

sbussweiler commented 8 years ago

Okay.

I will update the lifecycle documentation with the mentioned issues ;)

sbussweiler commented 8 years ago

@cdjackson, I talked again this morning with @kaikreuzer about the bridge initialization and bridge status change handling. We came up with another, third proposal:

bridgeHandlerInitialized and bridgeHandlerDisposed() will be removed from ThingHandler interface soon
bridgeStatusChanged() works as before; only already added things are notified about bridge status changes.
In the ThingHandler.initialize() method every Binding can ask for the bridge status. If the status of your bridge is ONLINE you can proceed. If the bridge status is OFFLINE you have to wait until bridgeStatusChanged(ONLINE) has been called in order to proceed.

What do you think?

cdjackson commented 8 years ago

This sounds like what I'm doing now but without setting the status to ONLINE in the initialize method. I guess what you mean is that you will also call bridgeStatusChanged if status is INITIALIZING? Is that right?

Yesterday I added the setStatus(ONLINE) to my initialize and this works ok. I then wait until the bridgeStatusChanged is called before continuing the initialisation.

Can I ask why you come up with this new idea? Is it to allow the status to remain in INITIALIZING?

sbussweiler commented 8 years ago

This sounds like what I'm doing now but without setting the status to ONLINE in the initialize method. I guess what you mean is that you will also call bridgeStatusChanged if status is INITIALIZING? Is that right?

The behavior of bridgeStatusChanged() will not change. The framework calls for each bridge status change (ONLINE resp. OFFLINE) the method bridgeStatusChanged().

Can I ask why you come up with this new idea? Is it to allow the status to remain in INITIALIZING?

It's not allowed to remain the status in INITIALIZING. In your case set the status to OFFLINE.

As I mentioned the behavior of bridgeStatusChanged() is the same. The only think I noticed is that bridgeHandlerInitialized() is no longer needed. Every thing handler can ask it's bridge about the bridge status. If a thing handler must wait for the bridge before continuing the initialization, the handler can set the status to OFFLINE. If bridgeStatusChanged() with bridge status ONLINE is called, the handler can continue the initialization and change the thing status to ONLINE.

Yesterday I added the setStatus(ONLINE) to my initialize and this works ok. I then wait until the bridgeStatusChanged is called before continuing the initialisation.

That's exactly my proposed procedure from above with one exception: set the thing status to OFFLINE.

cdjackson commented 8 years ago

It's not allowed to remain the status in INITIALIZING. In your case set the status to OFFLINE.

This is what I do now...

The only think I noticed is that bridgeHandlerInitialized() is no longer needed.

Ok - I think this is fine for me. This is what I previously did anyway, but I took this out and let the bridgeHandlerInitialized be called after setting status to OFFLINE. I don't mind adding this back if this method is deprecated.

So, either is fine by me...

maggu2810 commented 8 years ago

@sbussweiler I hope you have a few minutes to answer my questions :wink:

A device is accessed by an HTTP connection and needs a login. Currently the initialize of the thing is implemented this way:

updateStatus(ThingStatus.INITIALIZING);
check configuration parameters, if there is something wrong: updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_PENDING, "missing / wrong ..."); and return
at last queue a future task that handles the login
return
if the async login is handled correctly, the thing status is set to online and I queue a future task that reads the information over HTTP from the device
after the read succeeded the channels are updated and another read is queued (some time later / refresh interval)
if something went wrong (login / read values), the updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, "..."); is called and a login is queued (so, see above, if this succeeds the status is set to online again)

So, I assume there is not all correctly done as intended or is it okay (at least the updateStatus(ThingStatus.INITIALIZING); should perhaps be removed). Could you give me some tips?

sbussweiler commented 8 years ago

Sure, will try my best ;-)

updateStatus(ThingStatus.INITIALIZING);

As you already mentioned, updateStatus(ThingStatus.INITIALIZING) should be removed. It's not allowed to set this status in the binding. The framework updates the status to INITIALIZING when ThingHandler.initialize() is called.

check configuration parameters, if there is something wrong: updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_PENDING, "missing / wrong ..."); and return

If there are some missing configuration parameters (config parameters which are marked es "required") ThingHandler.initialize() is never called. The framework sets the status to UNINITIALIZED.HANDLER_CONFIGURATION_PENDING. I would recommend to mark your parameters user/password/token as required, so you don't have to check for such "missing" parameters.

If the parameters are just "wrong", update the status in ThingHandler.initialize() to OFFLINE.CONFIGURATION_ERROR (note that OFFLINE.CONFIGURATION_PENDING is an not allowed status combination, see Thing Status).

at last queue a future task that handles the login

If the parameters are "correct" you can do it. If not use ThingHandler.handleConfigurationUpdate() to update your configuration and try to establish the connection again. After the login is handled correctly, set the status to ONLINE.

if something went wrong (login / read values), the updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, "..."); is called and a login is queued (so, see above, if this succeeds the status is set to online again)

That's the way. If something went wrong, update the status to OFFLINE.COMMUNICATION_ERROR.

maggu2810 commented 8 years ago

Thanks @sbussweiler

eclipse-archived / smarthome

bridgeHandlerInitialized() behaviour has changed #1075