Bridge Child Thing Discovery

[tmrobert8]

Problem Discovery works great right now for bridges but discovering things within the bridge is a bit complicated. The consensus design pattern seems to be something like (with alot of variations):

1. The ThingHandlerFactory creates a ThingHandler
2. The ThingHandlerFactory creates a DiscoveryService passing the handler - the service then adds a listener or sets something in the handler to be triggered - then the discovery service is registered.
3. The handler, generally during initialization, triggers the discovery service to start
4. The discovery service scans the bridge and updates the inbox with results.

While it works - there's three downsides to this approach:

1. From a design standpoint, it's a bit of a mess. The handler factory is creating a discovery service (seems out of scope), the discovery service has a link to a specific handler (ugh), multiple discovery services can be created (since they are unique to a handler), a handler is doing something to trigger a discovery service (way out of scope), etc, etc
2. My biggest issue is that discovery generally only happens during initialization and there is no way to retrigger the discovery without restarting the binding (that's a more general statement as it depends a bit on the specific implementation details of the author). That makes it very hard for the USER to rediscover things in the bridge (especially if the bridge is fairly dynamic in adding things) - it's not simply a press and play scenario for them.
3. Lastly, there is about a bunch of different variations to the approach. Would be nice to standardize on a single approach.

What I'd do Rather than complaining, thought I'd try to offer a starting point to the discussion. This is a starting point only - not a full implementation. Would rather hear if its a viable alternative before hashing out any details...

What I'd like to do is create a "BridgeDiscoveryParticipant" (or something better named!) that is similar to Mdns/UpnpDiscoveryParticipant but whose SOURCE is the ThingRegistry itself (rather than MDNS/UPNP).

Basically it would something like:

public interface BridgeDiscoveryParticipant {
  public ThingTypeUID getBridgeID();
  public Set<ThingTypeUID> getSupportedThingTypeUIDs();
  public DiscoveryResult[] createResults(Bridge bridge); // not sure what we should pass here
  .. probably some other management type of functions ..
}

The BridgeDiscoveryService implementation would:

1. When a participant is discovered, we query the ThingRegistry for the associated bridge and if found, call back the createResults to get any DiscoveryResult(s) for the bridge
2. A listener is added to the ThingRegistry to listen for Things to be added/removed from the registry. If the bridge is added, we call createResults. If removed, we remove the associated discovery results
3. On an active scan (startScan) - we simply get the bridge for each participant and if found, call createResults to rediscover the children

The discovery service of course would have to deal with all the other management type of issues - I just wanted to talk about the overall approach at this point.

Benefits

1. We standardize the approach to discovering child items
2. Simplifies the design process since the discovery participant is the ONLY class concerned with discovery of child items (not sprinkled across three different classes)
3. Biggest benefit, the USER can simply trigger an active scan at any time and those results would appear in their inbox.

What do you all think?

Tim

[maggu2810]

I agree with the points you complain about. There should be some better option provided by the framework. Without having a look at the implementation I cannot judge if the approach fits the most common needs.

Should a demonstration be implemented for e.g. the Hue binding so we have a visible example how the implementation looks like and check them for some drawbacks?

[tmrobert8]

@maggu2810 @triller-telekom Sorry it took me awhile to get back to you on this. I don't know much about hue so I posted an example on my own binding (NEEO).

Background The NEEO Brain is a hub that contains zero to many rooms, each room contains zero to many devices. After discovering the brain, I'd like the rooms to immediately be discovered (and placed in the inbox) and when adding a room, I'd like the devices to immediately be discovered.

Sample implementation ThingDiscoveryParticipant is the interface that a binding would need to implement and will be notified when a thing should be evaluated

ThingDiscoveryService is the implementation of the discovery service that will monitor things and call the ThingDiscoveryParticipant

NeeoRoomDiscovery is the NEEO room discovery implementation of ThingDiscoveryService NeeoDeviceDiscovery is the same implementation but for devices

This is fairly straightforward. The ThingDiscoveryService adds a listener to the ThingRegistry and when a new thing is added, calls the ThingDiscoveryParticipant with the thing. The ThingDiscoveryParticipant will return null (or an empty collection) if no action should be taken on the thing or a collection of discovery results if one or more inbox items should be placed.

Please note I've already tested this on my development box and it works great.

Design Issues

1. Bridge vs Thing In my small world, I'd only need to be looking at bridges (not things). However, I err'd on the side of caution and assume others may need to discover things based on a thing (rather than a bridge). I'm not sure what everyone thinks - but if you feel strongly that only bridges should be evaluated - it would be a simple process to filter for bridges only (as opposed to things) and of course rename the implementation. We could actually support both I guess by having BridgeDiscoveryParticipant and a ThingDiscoveryParticipant depending on what someone wants - but that may be to complex and either one or the other we should go with. Thoughts?

2. Online vs Any You'll notice my implementation does not care if the thing is online or not. Both the NEEO and my sony binding don't have to be online to discover child things. But again, I don't know if that's my narrow view. If we think an online status is important - I can see the following options: a. Leave the status up to the implementation. In other words, don't address it and assume the implementor of ThingDiscoveryParticipant will handle whatever logic they need. b. Maybe add a 'isOnlineOnly' to the interface so the implementation writer simply needs to return a boolean to have their implementation called with only online things (and presumably when a thing changes to online). Would require alot more work but could be helpful if most implementations would need an online status c. Only call implementations when the thing goes online. Makes sense to only add stuff to the inbox if the thing is online. Same work as B but no added interface method and the assumption to only call when online.

Between the options - I'd only thing A or C would be the best. Either leave it up to the writer or assume most would only want online status and force it. Thoughts?

3. Filtering The implementation simply sends every thing added to every participant - I could potentially only call each participant based on whether the thing binding ID has a same binding ID from the support types list. Not sure if it's worth the effort since there isn't likely to be a performance issue - but you tell me..

Lastly - not real happy with the names but those are easy to change! There's also some smaller implementation issues and documentation issues - but those would be addressed in the eventual PR

[triller-telekom]

Thank you for thinking about a new concept to simplify the discovery process!

A couple of points that I noticed while thinking about it:

1. Things cannot have other Things as a child, that's the purpose of Bridges. A Bridge is defined as a Thing which acts a gateway to other Things, i.e. it provides the physical access in terms of a USB stick or the credentials to a webservice. So we only need this for Bridges.
2. Before calling discoverThings(participant, thing); you should check if the participant supports the type of the thing (well bridge after you have changed is as mentioned above). So maybe you should rename public Set<ThingTypeUID> getSupportedThingTypeUIDs(); in the Participant to public boolean supportsThingType(ThingType type); so its easier to check. The HandlerFactory already do it this way too.
3. Instead of querying the ThingRegistry like this: thingRegistry.getAll() you could use thingRegistry.stream() and filter for Bridges directly.
4. To answer your question "can we have participants before a thing registry is set? I don't think we can but need someone to answer that..." - You cannot have participants before the ThingRegistry is set, because you have defined the registry as a mandatory static reference. So after all static references are satisfied activate() will be called and your service is activated. At this point the participants might appear.
5. Please do not catch Exception directly. catch the specific ones.
6. The `@NonNullByDefault({}) should be written directly in front of the type and not in front of the visibility modifier, see https://www.eclipse.org/smarthome/documentation/development/conventions.html#null-annotations

Regarding the naming: How about ChildDiscoveryService for the service running in the framework and ChildDiscoveryServiceParticipant for the interface? Because what you discover are children of a Bridge, well technically you discover Things or ´Bridges, but only those which exist as a child of aBridge`.

I would be happy to see a PR here at the smarthome project which includes my points from above so we can discuss your idea further. its easier to discuss and comment in the code.

[tmrobert8]

Will do those suggestions - the only comment I had was for 1. That's really the heart of my question - do you think there is a need to ONLY discover child things (in which case you look at bridges only) or would there be a need to discover neighbor things. In other words, do you think there would ever be a case where a thing could discover another thing under the same bridge (begs the question why evaluating the bridge didn't find it)?

If you don't think so (and I can't think of any examples), then bridge filtering is the way to go. I just don't want to artificially put a limitation because I can't foresee a situation where a thing could potentially discover another thing under the same bridge. The argument would be again, why didn't you find the additional thing when you discovered the bridge.

Do you think that would ever be a possibility?

BTW - as for 5 - that was a straight copy for the UpnpDiscoveryService. I think they are catching exception because they are trapping any participant from preventing any other participant from executing (isolating). There are no defined exceptions to catch. Let me know what you want to do (we either ignore catching exceptions, continue to catch the generic exception or create a defined DiscoveryException to catch [which won't address isolation but could be useful for writers]).

[triller-telekom]

I think the case that a thing discovers a "neighbour thing" on the same Bridge is artificial and also speaks against the concept of Bridges and child Things. So we only need the discovery service for Bridges.

Good catch that the UpnpDiscoveryService is also doing it wrong :)

But let's focus on getting your discovery service right first. In general for framework code that calls arbitrary external code, such the one of a participant from a binding, should use our SafeCaller infrastructure. This guards not only against Exceptions but also against jobs which run too long.

https://github.com/eclipse/smarthome/blob/491ecd19d3487cf256b69d4b36f0dc0750f50c3d/bundles/core/org.eclipse.smarthome.core/src/main/java/org/eclipse/smarthome/core/net/NetUtil.java#L573-L577

One other important thing which is missing in your approach is the "reverse direction". For now you have solved the issue that in the HandlerFactory you have to create a DiscoveryService and pass over the BridgeHandler. This is one direction.

What I mean by "reverse direction" is that during the normal communication of the BridgeHandler with the real physical entity it might notice that a new device exists. For example in Philips Hue you can send a command to turn on a bulb to the Hue-gateway and it returns not only that the bulb is on, but also a list of devices which are connected to it. This "reverse direction" is a bit tricky though...

So what is missing in your approach is that the BridgeHandler can notify your participant about a newly discovered device. For sure you could define an interface so the participant can register itself on the ThingHandler, but how will that look like? What is the data structure that you pass tot he listerner? It is very binding specific on which kind of data the handler has and wants to pass to the participant. All I can think about it a HashMap<String, Object> to pass any data. However, this is prune to (type) errors :( In the current implementation we do not have this general interface and thus bindings specify their own like https://github.com/eclipse/smarthome/blob/master/extensions/binding/org.eclipse.smarthome.binding.hue/src/main/java/org/eclipse/smarthome/binding/hue/handler/LightStatusListener.java

Also a registerListener() and unregisterListener() on the BridgeHandler interface it not nice since not all bindings have a participant and would still have to implement those methods. A better way to solve this dilemma would be to have these methods encapsulated in a BridgeHandlerCallback, similar to the ThingHandlerCallback which we now pass tot he handlers. So a binding developer can simply call callback.deviceDiscovered(myMap).

One other point which I just stumbled across is the name Participant which somehow collides with the UpnpDiscoveryParticipant which has the method DiscoveryResult createResult(RemoteDevice device) that just converts the data from RemoteDevice into a DiscoveryResult which is a matter of milliseconds, versus your proposed Participant which might even trigger a long running scan. So the ChildDiscoveryServiceParticipant has to be renamed somehow to maybe ChildDiscoveryServiceModule? Suggestions for a better name are welcome :)

[tmrobert8]

@tilmankamp Thanks - I'll take the comments and create a PR for them - we can discuss further at that point. Feel free to post any other thoughts between now and then and I'll see if we can include them in the initial PR

[triller-telekom]

Thanks for creating the PR: However the "reverse direction" issue is still to discuss. I would like to hear the opinion of the committers @kaikreuzer @maggu2810 @sjka or @htreu about this:

So what is missing in your approach is that the BridgeHandler can notify your participant about a newly discovered device. For sure you could define an interface so the participant can register itself on the ThingHandler, but how will that look like? What is the data structure that you pass tot he listerner? It is very binding specific on which kind of data the handler has and wants to pass to the participant. All I can think about it a HashMap<String, Object> to pass any data. However, this is prune to (type) errors :(

[tmrobert8]

Just a thought (and not sure if it's good). What if we change the signature (not suggesting these names - just for illustration) to void bridgeDiscovered(bridge, discoveryCallback) where discoveryCallback looks like public interface DiscoveryCallback { void thingDiscovered(DiscoveryResult); }

That way if it's one way only - you just simply call the 'thingDiscovered' method for each result and in the case of your hue (which I haven't look at the code) - you'd do something like (psuedo code):

public void bridgeDiscovered(bridge, discoveryCallback) {
          foreach(thing discovered) { 
             discoveryCallback.thingDiscovered(myResult)
             if lightBridge (or whatever) then
                 lightBridge.addListener(new listener() {
                    void onLightAdded(whatever) {
                       discoveryCallback.thingDiscovered(myNewThing);
                    }
                 });

   }

That way the thing or thinghandler has no knowledge of discovery, all the discovery items are actually in the discovery implementaion and your discovery implementation can add whatever listener is needed and have a way to callback to add discovery results?

Let me know if my poor attempt at psuedo code is unclear!

Tim

[triller-telekom]

@tmrobert8 Thank you for this suggestion, but I am trying to follow your pseudo code but I guess there is something missing, which is why I do not understand it so far. Let me summarize what I understand:

We do have the following classes/interfaces involved:

ChildDiscoveryServiceModuleImpl implements ChildDiscoveryServiceModule:

public void bridgeDiscovered(bridge, discoveryCallback) {
          foreach(CHILD discovered) { 
             discoveryCallback.thingDiscovered(myResult)
             if lightBridge (or whatever) then
                 lightBridge.addListener(new listener() {
                    void onLightAdded(whatever) {
                       discoveryCallback.thingDiscovered(myNewThing);
                    }
                 });
   }

ChildDiscoveryService

//if a thing was added to the thingRegistry
public void added(Thing thing) {
       if(! (Thing instanceof Bridge) {return;}
        for (ChildDiscoveryServiceModule module : modules) {
           if(module.supportsBridgeThingType(thing.getUID()) {
                module.bridgeDiscovered(thing, CALLBACK);
           }
        }
    }

My question is: Where does the CALLBACK come from? Maybe I have also mixed up where you want to place which method... Maybe you can sketch which method should be placed i which interface/class.

We do have the interface ChildDiscoveryServiceModule, to be implemented by the binding and the class ChildDiscoveryService running as a service inside the framework.

[tmrobert8]

Was pretty poor psuedo code I know. Here's actual code (non-tested and could be prettied up but will get the point):

Child stuff (still need to rename the directory from thing to child):

https://github.com/tmrobert8/smarthome/tree/ThingDiscovery/bundles/config/org.eclipse.smarthome.config.discovery.thing/src/main/java/org/eclipse/smarthome/config/discovery/child

My very quick Hue implementation (no idea if it works, can absolutely be recoded in a better fashion and probably has some issues (like potentially adding the listener more than once) but hopefully get's the general approach right to talk about): https://github.com/tmrobert8/smarthome/blob/ThingDiscovery/extensions/binding/org.eclipse.smarthome.binding.hue/src/main/java/org/eclipse/smarthome/binding/hue/internal/discovery/HueLightChildDiscoveryImpl.java

[triller-telekom]

Thank you for implementing a proof of concept.

Unfortunately I found a big issue in it: Things which are added to the ThingRegistry must not have a handler yet! I did not know that before trying your code either. Background of this is that the ThingRegistry has the "metadata" about a Thing and the ThingManager creates a handler for them later.

So by the time you get notified that a Thing was added to the ThingRegistry, i.e. through a click on the approve button in the inbox of paperui, you do not have a handler on the Thing yet.

So unfortunately there seems to be an architectural issue in this new discovery idea.

For your Neeo binding I would thus suggest to implement it like it is done in the hue binding, i.e. create a discovery service from the handlerfactory and pass the bridgehandler to it and register the discovery service as a listener on your handler in case it finds something. This way we will get it merged soon, because this discussion here might take longer :(

However, we can further pursue the idea of improving the state of the art discovery process because it is for sure not optimal.

Does it make sense to you?

[kaikreuzer]

Sorry, I didn't follow this discussion in detail yet, but it feels a bit as if you are creating an alternative discovery service framework, which has many overlaps. E.g. the ChildDiscoveryCallback is pretty similar to the methods that AbstractDiscoveryService provides.

Taking a step back, all that you are requesting is actually to be able to declare a certain bridge handler being a DiscoveryService (i.e. giving it the possibility to call methods like the ones in ChildDiscoveryCallback). If a bridge handler were an OSGi service, this could be done straight away, but unfortunately that isn't the case. The downside of making the bridge handler being a discovery service at the same time would be worse separation of concerns as one class would deal with both aspects. But if we want to avoid this, we do not have any other chance than going for additional listener interfaces and imho this is the part that brings the most (for the developer unexpected) complexity that you are trying to solve - your current approach cannot simplify this part at all. I am therefore not really clear on the right way for simplification; if we change something fundamentally here (through new framework services/infrastructure etc.), it should imho really have a clear benefit and not just make stuff slightly simpler/different...

[tmrobert8]

@triller-telekom Didn't realize that myself and you are correct - would be a fatal flaw to this design (even though in practice a handler is assigned since the neeo one is working just fine - but I'm sure there is circumstances that will break that). BTW - going to switch the neeo stuff to the 'hue' way shortly to get it off our hands) but I agree we need to discuss this further because I feel like alot of the points @kaikreuzer makes applies to the current situation as well.

@kaikreuzer The implementation is built on the discovery service (just using the thing registry as a source). Was not an alternative or parallel design. However - as @triller-telekom pointed out - the approach can't be used because a handler isn't applied

Here's the two major issues I have with the current 'hue' way of doing things:

1. Way out of scope for a thing handler factory to be creating/starting a discovery service as a side effect.
2. A thing represents a physical device and a thing handler represents the communication between that device and ESH. I believe that device discovery (which is an ESH concept and has no real world analog with the device) is out of scope for the thing handler. To @triller-telekom point about bi-directional, at most a thing handler should provide an event to trigger a discovery service to scan (the thing handler should not be doing the scanning itself).
3. If you look at the class references - it's too complicated, not obvious and hard for a new person to pickup because the numerous reference (factory->discovery service<->thing handler plus ESH reference to support). A complicated class reference always points to issues in the design that should be corrected (ie simplifed).

Clearly the thing/bridge level is not correct and but it seems the ThingHandler level is correct. What if we created a ThingHandlerRegistry (as an OSGI service) that will track the thinghandlers as they are created/disposed and provides a listener for changes. We can then implement something similar (which is an abstractservicediscovery implementation) to what we are talking about but using the ThingHandlerRegistry as a source? Just thinking out loud here...

[kaikreuzer]

1. I wouldn't call it out of scope. The HandlerFactory is currently THE place to deal with OSGi services - be it requiring some that need to be passed to handlers (like e.g. the shared httpClient) or be it registering services, because a binding wants to offer them (like a discovery service for a certain bridge where we know that we can do discovery).

2. Not really out of scope either. As you say, the thing handler contains the code for the technical communication with the device. So naturally, it is the thing handler, who has all the means of figuring out if there is anything new available and often this is pretty intertwined with the "normal" communication it is doing (like e.g. for hue or for bindings that receive push events from the device). The current setup tries to nonetheless keep the discovery code separated from the handler code by simply providing the handler to the discovery service, but not introducing any dependency from the handler to the discovery at all.

A ThingHandlerRegistry does not make much sense. Nobody should be interested in accessing handlers and in their lifecycle directly. We have the ThingManagerImpl that does the handling of all the handlers already. Imho the handler factory is just the right place as it is right now: You have the exact moment when the handler gets created and you even know that for this specific handler that is being created, you want to also register a discovery service (which is rather the exception than the norm). So the best approach might actually be to simplify the way this is done in the handler factory. E.g. by letting the bridge handler implement a specific marker interface and the BaseThingHandlerFactory then automatically deals with the creation of a suitable discovery service for it or something.... Also just thinking out loud here.

[tmrobert8]

Marker interface is an interesting suggestion. Off the top of my head (and with TERRIBLE names)... Let's say we had some marker interface: public interface ThingHandlerDiscoverySource {}

That a thing handler can implement: public class MyThingHandler extends BaseThingHandlerFactory implements ThingHandlerDiscoverySource { ... }

Now let's say we had a paired interface:

public interface ThingHandlerDiscoveryProvider<T extends ThingHandlerDiscoverySource> { 
   void discover(T handler, DiscoveryCallback callback)
}

the callback would simply be (like above - allowing you to discover stuff immediately or based on a listener for something on the handler):

public interface DiscoveryCallback {
   void thingDiscovered(DiscoverResult result);
   void thingRemoved(ThingUID thingUID);
}

The discovery implementation would be like

public class MyThingHandlerDiscovery implements ThingHandlerDiscoveryProvider<MyThingHandler> { 
   public void discover(MyThingHandler handler, DiscoveryCallback callback) {
      do discovery stuff based on handler and use callback to give results
   }
}

Now - to link them up. The BaseThingHandlerFactory would, after creating the thing handler...

if (handler instanceof ThingHandlerDiscoverySource) {
   ... do discovery stuff
}

(note: the remove handler would then do the removal stuff

do discovery stuff Yes - I wrote that vaguely because here is where we get into some decisions before we can talk implementation details

1. We could have an implementation of the AbstractDiscoveryService that would take the ThingHandlerDiscoverySource, find the paired implementation (via reflection or what not) and handle all the interaction between the two. This would mimic the HUE way of things in that we create one discovery service for every one handler and the handler would dynamically start the discovery service.
2. (not sure if I have good wording here since I'm still fairly new to OSGi) We could have an generic implementation of AbstractDiscoveryService (OSGi service) that has all the ThingHandlerDiscoveryProviders added to it (via OSGi services) and the BaseThingHandlerFactory get's a reference to that implementation. The BaseThingHandler then on creating a handler - passes that handler to the discovery service, which then looks up the related ThingHandlerDiscoveryProvider and makes the necessary calls to do discovery. That way we have a single discovery provider that can handle multiple thing handlers (a one to many relationship versus the first option which is one to one).

I'm really not sure if I explained myself well here - if you kinda agree with the approach but would like more details - maybe I can mock this up to look at for further discussion

or am I off on what you are suggesting?

[lolodomo]

I have the feeling that we have a simple solution consisting in enhancing the BaseThingHandlerFactory with a new method protected @Nullable AbstractDiscoveryService createDiscoveryService(ThingHandler thingHandler). By default, the method will return null. The bindings wanting a discovery service attached to a thing will override this method in their handler factory and instantiate the discovery service. Then the updated BaseThingHandlerFactory will deal with service registration and service activation. So this will mainly simplify the stuff with registration that all bindings have currently to implement in their handler factory.

I can propose the change with its use in the Hue binding for example.

[lolodomo]

My solution was really cool because it simplifies the binding code but after finishing to implement it, I discovered that it introduces a dependency cycle between o.e.s.config.discovery and o.e.s.core.thing because I need to use AbstractDiscoveryService in BaseThingHandlerFactory. Is there an easy way to avoid this cycle ?

[htreu]

I guess we do not want a dependency from o.e.s.core.thing onto o.e.s.config.discovery. The BaseThingHandlerFactory may provide a new method registerThingDiscovery and a new interface ThingDiscovery which is implemented by AbstractDiscoveryService. In registerThingDiscovery the service could be registered with bundleContext.registerService("org.eclipse.smarthome.config.discovery.DiscoveryService", ... but that looks like a hack to me.