Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

Pass DiscoveryServiceCallback also to DiscoveryParticipants #5526

Conversation

triller-telekom
Copy link
Contributor

DiscoveryParticipants may also implement the ExtendedDiscoveryService
interface to obtain a DiscoveryServiceCallback.

Fixes #4630

Signed-off-by: Stefan Triller stefan.triller@telekom.de

DiscoveryParticipants may also implement the ExtendedDiscoveryService
interface to obtain a DiscoveryServiceCallback.

Fixes eclipse-archived#4630

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
Copy link
Contributor

@htreu htreu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment inline and one question: If this is for detecting duplicates, isn't there already the `representatoionProperty' on which the persistent inbox detects them? Or do I miss something here?

@@ -147,14 +152,24 @@ private void scan() {

@Reference(cardinality = ReferenceCardinality.MULTIPLE, policy = ReferencePolicy.DYNAMIC)
protected void addMDNSDiscoveryParticipant(MDNSDiscoveryParticipant participant) {
if (participant instanceof ExtendedDiscoveryService) {
((ExtendedDiscoveryService) participant).setDiscoveryServiceCallback(discoveryServiceCallback);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doubt that this will work: when the participants are added to the MDNSDiscoveryService the discoveryServiceCallback is not necessarily set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is true, but therefore I have the loop over all participants when the callback is set.

This code here is for the case that later at runtime a participant is added, so it receives the callback as well.

@triller-telekom
Copy link
Contributor Author

To answer your question: With the callback you can access previous results and do whatever you like with them, that depends on the use-case. But you are right, for detecting duplicates we have the representationProperty.

@maggu2810
Copy link
Contributor

Can you give us some examples of concrete use cases?
I just wonder why it has not been necessary before.

@tmrobert8
Copy link
Contributor

tmrobert8 commented May 10, 2018

Use case - if the configuration contains an IP Address and that IP address changes, you need to update the prior result.

EDIT: same applies really to any configuration option (port, etc) - but ip address is easy to illustrate

@kaikreuzer
Copy link
Contributor

@tmrobert8 That is a perfect example why we do NOT want to introduce this feature - because people believe they have to do such things. Instead, the framework already takes care of updating existing inbox results, when you re-discover the same Thing UID but with changed configuration parameters.

@htreu
Copy link
Contributor

htreu commented May 14, 2018

We should reflect this in the docs. The mechanism of updating current inbox results and existing things is not part of the discovery service description. Also the ExtendedDiscoveryService is explicitly mentioned to get hold of existing results/things.

@tmrobert8
Copy link
Contributor

I agree with @htreu - I never realized the inbox item is automatically updated and that should documented.

@kaikreuzer
Copy link
Contributor

Who is volunteering to do so?

@tmrobert8
Copy link
Contributor

I'd have no issues documenting this once it's been merged

@kaikreuzer
Copy link
Contributor

Maybe I have missed something, but as far as I understood, the documentation would actually document, why this PR here is NOT needed, so I don't see why we would have to wait for it to be merged...

@tmrobert8
Copy link
Contributor

tmrobert8 commented May 14, 2018

@kaikreuzer
No - my fault. I thought you meant to document (ie javadoc) the existing classes to mention that inbox items will automatically be updated with the new results based on the thing UID (I don't believe that's documented right now)

As for this PR - I still think it's valuable to have (unless you tell me there is a better way!). My comments about updating the config apply to the existing thing. Example: let's say I have an MDNS item (the NEEO brain) that I have added to my system. If the NEEO Brain acquires a new IP address, the MDNS discovery will see the new IP address and I can update the thing's configuration directly. Same applies to Sony when the stupid TV changes port numbers and the UPNP discovery is notified of the change.

@triller-telekom
Copy link
Contributor Author

@tmrobert8 What prevents you from doing this

If the NEEO Brain acquires a new IP address, the MDNS discovery will see the new IP address and I can update the thing's configuration directly

without this PR? You simply create a new DiscoveryResult with the same ThingUID and your existing Thing will get the new IP address.

@tmrobert8
Copy link
Contributor

@triller-telekom
Never knew that (ie DiscoveryResult will update existing thing's configuration). I have no idea what the use case is then for the PR if it updates both the inbox and existing thing items...

htreu pushed a commit to htreu/smarthome that referenced this pull request May 15, 2018
Addresses eclipse-archived#5526-

Signed-off-by: Henning Treu <henning.treu@telekom.de>
@htreu
Copy link
Contributor

htreu commented May 15, 2018

Please see #5583 for an updated documentation.

With this I think we can close this one.

@htreu htreu closed this May 15, 2018
@triller-telekom triller-telekom deleted the extendedDiscoveryParticipants branch May 16, 2018 16:25
sjsf pushed a commit that referenced this pull request May 28, 2018
Addresses #5526
Signed-off-by: Henning Treu <henning.treu@telekom.de>
ermartens pushed a commit to ermartens/smarthome that referenced this pull request Jun 15, 2018
Addresses eclipse-archived#5526
Signed-off-by: Henning Treu <henning.treu@telekom.de>
@openhab-bot
Copy link
Contributor

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

https://community.openhab.org/t/best-practice-in-bundle-when-both-autodiscover-and-manual-thing-setup-is-used/93565/3

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ExtendedDiscoveryService not available for MDNSDiscoveryParticipant
6 participants