UPNP don't notify embedded child devices by default

[andrewfg]

See #4712

Signed-off-by: Andrew Fiddian-Green [email protected]

[andrewfg]

Ping @lolodomo ..

Note: I wrote the code, but I do not have any time for testing, so can you please have a look at it yourself.

[andrewfg]

@lo92fr if your addon discovery service participant wants to be notified about embedded child devices then it will have to override the notifyChildDevices() method.

[mherwege]

See my question: #4712 (comment)

[andrewfg]

@mherwege either we have to modify..

• some bindings to prevent them seeing unwanted children (e.g. Sonos) (opt out model), or
• other bindings to explicitly allow them to see wanted children (opt in model)

On balance I think the opt in model is safer.

[mherwege]

@mherwege either we have to modify..

• some bindings to prevent them seeing unwanted children (e.g. Sonos) (opt out model), or
• other bindings to explicitly allow them to see wanted children (opt in model)

On balance I think the opt in model is safer.

For upnpcontrol, which is a generic binding, you would want to see the embedded devices. That would mean the Sonos embedded media server and renderer would be discovered. I believe that is what @lolodomo wanted to prevent. It is easy enough to not have them discovered by the Sonos binding (opt-in or opt-out doesn’t matter). But you can’t avoid discovering it with the upnpcontrol binding.

[andrewfg]

@mherwege I think you misunderstand this. The OH Core service will ALWAYS discover both root and embedded devices. The bindings respective discovery participants will by default not receive the NOTIFICATIONS for embedded devices. And it you want your addon's disco participant to receive those notifications then you need to make a one liner override of its notifyChildDevices() method.

[mherwege]

@mherwege I think you misunderstand this. The OH Core service will ALWAYS discover both root and embedded devices. The bindings respective discovery participants will by default not receive the NOTIFICATIONS for embedded devices. And it you want your addon's disco participant to receive those notifications then you need to make a one liner override of its notifyChildDevices() method.

I think I understand this well enough. Again: you would want to override this for the upnpcontrol binding because that binding would want to see the embedded devices for many devices (e.g. Denon has their media player as an embedded device). Therefore it will discover the embedded Sonos media server and media renderer. You cannot avoid that for upnpcontrol. That’s why I asked @lolodomo what binding discovered these embedded devices. Was it the sonos binding or the upnpcontrol binding?

[lolodomo]

That’s why I asked @lolodomo what binding discovered these embedded devices. Was it the sonos binding or the upnpcontrol binding?

The Sonos binding.
The upnpcontrol binding is not installed.

[andrewfg]

@lolodomo I opened this PR to resolve your problem from #4712 .. but apparently you are not too much concerned about that any more?? => So shall I close this one?

[lolodomo]

Of course I am interested. What is expected from me? If this is a review, I can probably do it but I am not a maintainer for the core framework.

[kaikreuzer]

I cannot judge whether this feature is required, but if the binding maintainers ask for it, I'll happily merge it.
Wrt opt-in/opt-out: For backward compatibility reasons, I would think it is better to go for an opt-out, so that the existing behavior is not impacted, wdyt @andrewfg?

[lolodomo]

Code looks good to me.

[lolodomo]

Once merged, I will tell you if Sonos discovery is fixed. But we already know that it will.

[andrewfg]

We may need to adjust that other binding that does require discovery of embedded children.

—-

EDIT specifically https://github.com/openhab/openhab-addons/blob/main/bundles/org.openhab.binding.upnpcontrol/src/main/java/org/openhab/binding/upnpcontrol/internal/discovery/UpnpControlDiscoveryParticipant.java should override notifyChildDevices() and return true..