Only perform discovery on existing devices

[cdjackson]

Currently the binding performs discovery on all devices - even those that are already Things. This is because the discovery service has no way to know if a device is already discovered and added as a Thing.

Since devices may (re)join the network at any time we either need to add devices when discovery is started, or add support for the OH background discovery concept.

We also need to handle one other case which is if Things are deleted, but not removed from the network. In this case, if we don't add all known things during discovery, we will loose devices completely.

[falmanna]

Does it make sense to check on the existing things using thingRegistery when discovering existing devices after starting a scan?

if (node.getNetworkAddress() == 0 || thingRegistry.stream().anyMatch(thing -> thing.getUID().toString()
      .toLowerCase().trim().contains(node.getIeeeAddress().toString().toLowerCase().trim()))) {
      continue;
}

I've made this change and it seems to be working, but I am not sure if I missed something as I have little knowledge about the internals of the ZigBee binding.

[cdjackson]

It's not allowed for the discovery service to interface with the thingRegistry. This is what we used to have in the past, but it was removed some time ago.

[cdjackson]

Sorry - to be clearer - what I meant was this was originally part of ESH - I think it was called the "extended discovery service" and this feature was removed from the core a while back.

[falmanna]

what I meant was this was originally part of ESH

Yes, I remember but I am not following up with the framework development. I just went through the original removal issue and found that you've discussed this specific issue there but didn't report about the suggestion. Apparently you were to try getting the existing things from the BridgeHandler without accessing the framework from the DiscoveryService, so something like this should be possible?

if (node.getNetworkAddress() == 0 || coordinator.getThing().getThings().stream().anyMatch(thing -> 
           thing.getUID().toString().toLowerCase().trim().contains(node.getIeeeAddress().toString().toLowerCase().trim()))) {
     continue;
}

[cdjackson]

I initially thought this was reasonable, however I don't think it will work properly. The problem is that you are making an assumption that the uid contains the IEEE address, and this isn't correct.

You might be able to find a similar way to get the IEEE address properly as it is contained in the thing configuration - I've not looked to see if that's available.

[falmanna]

Initially, this should work I guess.

if (node.getNetworkAddress() == 0 || (coordinator.getThing().getThings().stream()
        .anyMatch(thing -> ((String) thing.getConfiguration()
                .get(ZigBeeBindingConstants.CONFIGURATION_MACADDRESS)).toLowerCase().trim()
                        .contains(node.getIeeeAddress().toString().toLowerCase().trim())
                && node.isDiscovered()))) {
    continue;
}

I've added node.isDiscovered() since some devices (mostly sensors) might have Node has not completed discovery state. Please let me know if this make sense.

We can also improve performance by creating a one-time set of existing things' addresses, but then we will need to listen for added and removed things.

[cdjackson]

Thanks. If you've tested this, then please feel free to provide a PR. Please have a think if this can be re-written slightly to remove some of the trim and toLowerCase type methods - they are not all required since (for example) IeeeAddress will only ever return an upper case string of exactly the correct length - you could also use this for the configuration as well (ie construct an IeeeAddress instance...

eg -:

get(ZigBeeBindingConstants.CONFIGURATION_MACADDRESS)).toLowerCase().trim()
                        .contains(node.getIeeeAddress().toString().toLowerCase().trim())

could become something like -:

new IeeeAddress(get(ZigBeeBindingConstants.CONFIGURATION_MACADDRESS)).equals(node.getIeeeAddress())

I probably wouldn't worry too much about performance - this should be fast enough given that it only happens during discovery, which is a very occasional activity - I'd be more worried if it was running every second or so. I'd be tempted to focus on readability in this instance.

[falmanna]

Thanks, will create a PR after further testing