Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MDNSDiscoveryService support for detection of multiple things per mdns service #3677

Closed
debug-richard opened this issue Jul 2, 2023 · 24 comments
Labels
enhancement An enhancement or new feature of the Core

Comments

@debug-richard
Copy link

The current implementation of MDNSDiscoveryService/MDNSDiscoveryParticipant only allows one thing to be reported per MDNS service, but an MDNS service can provide multiple things.

This is needed for a plugin I am developing, I will submit a pull request soon.

@debug-richard debug-richard added the enhancement An enhancement or new feature of the Core label Jul 2, 2023
debug-richard added a commit to debug-richard/openhab-core that referenced this issue Jul 2, 2023
…loses openhab#3677)

Adds the optional methods createResults(...) and getThingUIDs(...) to the MDNSDiscoveryParticipant interface.
They return an ArrayList<DiscoveryResult/ThingUID> containing the discovered things.
Bindings implementing this interfaces should return null on createResult(...)/getThingUID(...) to avoid double detection.

Signed-off-by: Richard Schleich <[email protected]>
debug-richard added a commit to debug-richard/openhab-core that referenced this issue Jul 2, 2023
…loses openhab#3677)

Adds the optional methods createResults(...) and getThingUIDs(...) to the MDNSDiscoveryParticipant interface.
They return an ArrayList<DiscoveryResult/ThingUID> containing the discovered things.
Bindings implementing this interfaces should return null on createResult(...)/getThingUID(...) to avoid double detection.

Signed-off-by: Richard Schleich <[email protected]>
debug-richard added a commit to debug-richard/openhab-core that referenced this issue Jul 2, 2023
…loses openhab#3677)

Adds the optional methods createResults(...) and getThingUIDs(...) to the MDNSDiscoveryParticipant interface.
They return an ArrayList<DiscoveryResult/ThingUID> containing the discovered things.
Bindings implementing these interfaces should return null on createResult(...)/getThingUID(...) to avoid double detection.

Signed-off-by: Richard Schleich <[email protected]>
@morph166955
Copy link

I'm confused by what this is doing. I have several bindings which generates a new thing for each device discovered. For example, the androidtv binding generates a thing for each of the Nvidia Shields on my network, and for each protocol it detects (2 per device). How is this different from that?

@debug-richard
Copy link
Author

Your example is 1 device = 2 different mdns service types providing 2 different things
In my case it is 1 device = 1 mdns service type providing X things

A mdns service is based on a service type (_http._tcp.local), a port, a hostname and optional attributes.
In my case the service is http and provides multiple things via rest api (/api/device/1/, /api/device/2/ /api/device/X/).
In the current state I would need to announce them on different ports or via different service types which is not the way how mdns service should be used.

@morph166955
Copy link

morph166955 commented Jul 3, 2023

Ah, ok that makes sense. So in your case, rather than returning a single DiscoveryResult from createResult, you want to return an array of results.

That said, wouldn't it be more appropriate to define as a Bridge at that point rather than multiple Things? If you are connecting to the same device, on the same port, routing traffic through the Bridge and having a set of Things under the Bridge seems more appropriate. In that case, you would reply with a DiscoveryResult of a Bridge, and then you can add the things to the inbox as it makes sense at the binding level.

Check out...

https://github.com/morph166955/openhab-addons/blob/ring/bundles/org.openhab.binding.ring/src/main/java/org/openhab/binding/ring/internal/discovery/RingDiscoveryService.java

Basically on the ring binding there is an account which is created. Everything runs through the account. Then, the account sends back a list of devices through the API. We add those to a devices list. Then through DiscoveryService we add those devices to the Inbox so they can be configured. Ultimately, even when something happens on the devices, the commands are still sent through the account to the other side.

@debug-richard
Copy link
Author

The discovered things are actually bridges :P
Writing a (discovery) bridge for bridges that support things seem like a bit too much overhead.
This patch changes ~15 lines of logic code and is fully backwards compatible.

@morph166955
Copy link

Sorry, not following. What is the point of creating multiple bridges that go to the same device, same port, and use the same base protocol (http)?

@debug-richard
Copy link
Author

This is because the devices behind the bridges use a different type of bus for communication and the logical structure works perfectly.
But the discussion is becoming binding centered.

An MDNS service can technically announce several things, so this option should be supported.

@morph166955
Copy link

morph166955 commented Jul 10, 2023

I think you're missing what I'm saying. In your example:

A mdns service is based on a service type (_http._tcp.local), a port, a hostname and optional attributes.
In my case the service is http and provides multiple things via rest api (/api/device/1/, /api/device/2/ /api/device/X/).

Different REST APIs are accessed through a single http service from a single port. I would see a manual config for this looking something like:

Bridge mybinding:httpAPI:myBridge "Controller" [ ip="192.168.1.1", port="80" ] {
	Thing device myDevice1 "API Device 1" [ deviceId=1 ]
	Thing device myDevice2 "API Device 2" [ deviceId=2 ]
        ...
	Thing device myDeviceX "API Device X" [ deviceId=X ]
}

So in this case, the only thing that has to be found via mdns is the Bridge. Once the Bridge is configured, it would then add the individual devices to the inbox directly for creation. There is no reason to discover multiple devices from the same mdns advertisement.

@debug-richard
Copy link
Author

So instead of detecting and returning the bridges directly in the MDNSDiscoveryParticipant of the binding, should I create an additional binding that detects a meta-bridge from the MDNSDiscoveryParticipant that is only used to announce the bridges?

This is more of a workaround than a good solution in my view.

@morph166955
Copy link

morph166955 commented Jul 11, 2023

No, this is all handled in one binding. I would suggest you take a look at the UniFi binding as a conceptual example of Bridges and Things. The MDNS would return the single Bridge. Once the user adds it, the Bridge populates the inbox with all of the available Things directly. The purpose of a Bridge is to reduce the overall complexity by maintaining one connection to the API rather than an individual connection for each. In many cases, when the APIs are from a service provider, they are also rate limited so this helps prevent flooding as well.

@debug-richard
Copy link
Author

The thing with the additional binding was wrong but it still requires an additional bridge which I don't like.

@wborn
Copy link
Member

wborn commented Jul 23, 2023

This is needed for a plugin I am developing, I will submit a pull request soon.

Do you already have a link to the code? What is the binding for?
I agree that it doesn't make much sense to support this when the add-on modelling is incorrect.

@debug-richard
Copy link
Author

Ok guys, I looked at the suggested implementations.

To give you a rough idea of my structure:

Bridge mybinding:httpAPI:myBridgeModule "IO Module Slot 1" [ ip="192.168.1.1", port="80", slot="1" ] {
	Thing device ioport1 "IO Port 1" [ mode="analog" ]
	Thing device ioport2 "IO Port 2" [ mode="digital" ]
    ...
	Thing device ioportX "IO Port X" [ mode="output" ]
}

Bridge mybinding:httpAPI:myBridgeModule "IO Module Slot 2" [ ip="192.168.1.1", port="80", slot="2" ] {
	Thing device ioport1 "IO Port 1" [ mode="analog" ]
	Thing device ioport2 "IO Port 2" [ mode="digital" ]
    ...
	Thing device ioportX "IO Port X" [ mode="output" ]
}

Bridge mybinding:httpAPI:myBridgeModule "IO Module Slot 1" [ ip="192.168.1.2", port="80", slot="1" ] {
	Thing device ioport1 "IO Port 1" [ mode="analog" ]
	Thing device ioport2 "IO Port 2" [ mode="digital" ]
    ...
	Thing device ioportX "IO Port X" [ mode="output" ]
}

Bridge mybinding:httpAPI:myBridgeModule "IO Module Slot 3" [ ip="192.168.1.2", port="80", slot="3" ] {
	Thing device ioport1 "IO Port 1" [ mode="analog" ]
	Thing device ioport2 "IO Port 2" [ mode="digital" ]
    ...
	Thing device ioportX "IO Port X" [ mode="output" ]
}

Basically, there can be multiple hosts that can contain multiple modules with multiple ports.
Theoretically, I could add another bridge for the host, but that doesn't really solve my problem, since discovery is always done via a discovery service (please tell me if I am wrong).

The proposed UniFi and Ring implementations both do not implement MDNSDiscoveryParticipant.
They implement AbstractDiscoveryService which allows to call thingDiscovered(...) multiple times to return the discovered things.

And that's the point, I implemented the AbstractDiscoveryService first, but it only works by brute-forcing IP addresses, which is not my preferred solution but the binding basically works with this implementation.

Now I tried to rewrite it with the MDNSDiscoveryParticipant, but this doesn't provide the necessary interfaces to handle multiple things per host.

@morph166955
Copy link

morph166955 commented Jul 29, 2023

What if you moved slot to the thing level configuration as well. That would resolve it. So under a thing you could have [ slot="1", mode="digital" ]. You would definitely have a different bridge for different hosts, IPs.

@wborn
Copy link
Member

wborn commented Jul 29, 2023

You can also model different slots per IP/port in one Thing by using a channel group for each slot.

@debug-richard
Copy link
Author

debug-richard commented Jul 29, 2023

This is not possible because the ports share a power management via the bridge.
The things have also multiple channels attached (voltage, current, various interrupt triggers).

But this has nothing to do with the mdns discovery problem.

The issue related question is, how (else) do I discover multiple things from a mdns host?

@morph166955
Copy link

I think I see the confusion. The short answer is, you don't. Once you add the bridge from mdns, the bridge can add all of the individual things tied to it directly to the inbox. Mdns is not required at that point. The ring binding does this as an example. Once we add the bridge, which is the connection to the ring cloud, we get a json back from the cloud with information about all the ring devices. We simply parse that and add inbox items for each one.

@debug-richard
Copy link
Author

Do you mean this code ?

@morph166955
Copy link

Yes that section of code deals with that.

@debug-richard
Copy link
Author

So when you add the binding, the detection does nothing.
When you add the bridge, it discovers things from the API and stores them in the RingDeviceRegistry instance.
Now when (background) detection is running, it fetches things from the registry and populates the inbox.
Do I understand this correctly?

@morph166955
Copy link

morph166955 commented Jul 29, 2023

Yes that's correct. In ring we have to put in credentials so it's manual. In your case I'd add the bridge from mdns and then let the bridge add the rest of the devices to the inbox as things attached to the bridge.

@debug-richard
Copy link
Author

Ok, this is a possible solution.

But I have to say that the two cases are different, in the case of MDNS you don't need credentials to log in so you can detect them directly, while the ring binding needs to have them before it can do anything.

But eventually we have to make a decision.
My patch is quite small, it adds optional interfaces and doesn't break anything.
I believe it is suitable for the MDNS case, but I don't know if there are any openhab api guidelines for our particular problems I'm touching on here.

I want to push my binding soon so any maintainers (@wborn, @morph166955), please merge it our close the PR soon so I know what to do.

thank you anyway

@morph166955
Copy link

So, I'm not a decision maker here by any stretch, but my opinion would be to detect the bridge and add the things after as we've discussed. Many bindings are structured this way already. This is how OH is structured for bridges and things. It's not "proper" to create multiple things or bridges for the same interface point. We used to do that before bridges, we don't anymore. Each mdns advertisement should in theory create a single thing or bridge. If there are multiple ports or something like that on the same device, then each would have a separate advertisement as well which mitigates this.

I would also suggest you open the PR for your binding and leave it as a work in progress draft. This would allow for better collaboration as we could look closer at the specific use case and comment directly.

@kaikreuzer
Copy link
Member

This issue looks as if it is in a deadlock...

You can also model different slots per IP/port in one Thing by using a channel group for each slot.

I would agree with @wborn that this might be the best modelling when looking at your example.
Also, @morph166955 has a point that ideally, every mDNS announced service should correspond to one thing.

My patch is quite small, it adds optional interfaces and doesn't break anything.

That's fair, but if this patch opens up a path that might invite people to do an inappropriate modelling for their binding, we might rather want to avoid such a situation. And as @wborn pointed out already: If there is no use for the feature, there is no point in adding it in the first place.

@kaikreuzer
Copy link
Member

As #3678 has been closed, let's close this issue as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature of the Core
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants