eclipse-archived / smarthome

Eclipse SmartHome™ project
https://www.eclipse.org/smarthome/
Eclipse Public License 2.0
865 stars 781 forks source link

Bridge Thing offline, but child things stay online #5552

Closed jsjames closed 6 years ago

jsjames commented 6 years ago

In my openhab binding (pentair), I have a serial bridge with several child things. If there is a setup error for the bridge, I set the status of the bridge to offline (updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, msg)). This is reflected correctly in the paperUI. However, all the child things still go ONLINE. I had thought the framework took care of not allow child things to go online if their corresponding bridge is OFFLINE. Can someone confirm or do I need to manually manage this part of the lifecycle in the binding?

Thanks, Jeff

J-N-K commented 6 years ago

Your thing handler should take care of the thing‘s status in „bridgeStatusChanged“.

sjsf commented 6 years ago

The implementation in BaseThingHandler.bridgeStatusChanged() should by default do exactly what you want:

https://github.com/eclipse/smarthome/blob/936287de21b7a6baabcf969899b9b62d2faac0bf/bundles/core/org.eclipse.smarthome.core.thing/src/main/java/org/eclipse/smarthome/core/thing/binding/BaseThingHandler.java#L672-L680

If you didn't override this method in your thing handlers and it's not working as expected, then this needs to be looked into more closely for any potential bugs.

jsjames commented 6 years ago

I don't have bridgeStatusChanged overloaded. But it appears that if the Bridge sets status to OFFLINE in its initialize function, bridgeStatusChanged is never called. In my case, that is what is happening.

In looking at some other code in other another binding (dcsalarm), it looks like in the initialize function for the child things, they automatically set the child status to OFFLINE. Then, they implement bridgeStatusChanged to determine when the bridge goes online and at that point initialize the child thing.

Assuming this is how it should work by design, I can implement the same in my binding?

sjsf commented 6 years ago

Ah, I see. I checked the ThingManager code and indeed you hit a weak spot there. The bridge obviously needs to be initialized first, before the thing handlers are created. So they cannot be possible to inform them because they simply don't exist at that point in time.

However, when the ThingManager then turns to the thing handler, it only checks whether they are initialized:

https://github.com/eclipse/smarthome/blob/5b746e3084ca2803400549bd5467a11c7116d92b/bundles/core/org.eclipse.smarthome.core.thing/src/main/java/org/eclipse/smarthome/core/thing/internal/ThingManager.java#L520-L526

It doesn't care whether the bridge is online or not. The question now is whether it would help if the ThingManager would call the child thing handler's initialize() only once the bridge becomes ONLINE. Under the assumption that the bridge is required in order to successfully communicate with the device, I think this would make sense.

Nevertheless, the child handle must be able to cope with the situation that the communication breaks down at any time anyways.

So, TL;DR: No, I don't think this is good to implement this logic in the binding. It puts the same burden on all bindings. This is something which the framework should take care of. IMHO this is a bug.

kaikreuzer commented 6 years ago

I do not agree that this is a bug in the framework - we require ThingHandlers to always set a ThingStatus within the initialize() method. If the pentair binding currently sets it to ONLINE no-matter-what, then this is clearly a bug in the binding.

As every binding must have some logic of how to determine a correct thing status (and by this obviously also reflecting the bridge status), this logic should best be called from initialize as well.

Not calling initialize at all by the framework is imho no valid approach as it must be absolutely valid to perform some handler logic upon startup even if there isn't any communication with the remote device possible. Note that the method is meant to initialize the handler, not the remote device.

triller-telekom commented 6 years ago

Note that the method (initialize) is meant to initialize the handler, not the remote device.

I agree with this and maybe we want to consider, or at least think about, implementing a initialization method for the device as well, in the context of https://github.com/eclipse/smarthome/issues/3484. But this is out of scope for this issue...

So this makes me think whether we should move this issue to https://github.com/openhab/openhab2-addons where the pentair binding is located. There the binding can be fixed to properly handle the ThingStatus. WDYT?

htreu commented 6 years ago

Assuming this is how it should work by design, I can implement the same in my binding?

@jsjames please continue with the fix of this issue in your binding. The discussion here shows that the handler initialization is responsible for setting the correct status. For child thing handlers this might be even UNKNOWN in case you are not able to decide the correct status right away.