[upnpcontrol] Send periodic keep alive

[digitaldan]

This sends a periodic search packet to the remote upnp device as part of the thing's refresh cycle. This search request acts as a keep alive mechanism for devices that loose registration.
Fixes #16638

[lsiepel]

Besides the single comment, LGTM.

Will wait for @mherwege to look at it before merging. Do you recommend to backport?

[digitaldan]

Thanks, i would suggest backporting this one.

[mherwege]

I am a bit in doubt about this solution. The binding so far refrained from explicit M-SEARCH calls and relied on the upnp service for this. And you state this issue also appears with Sonos. Would it then not be better to solve this in the transport, like here: https://github.com/openhab/openhab-core/blob/1f9ba2b0eea9daf70515cd36b53c3fe1d2f1e54d/bundles/org.openhab.core.io.transport.upnp/src/main/java/org/openhab/core/io/transport/upnp/internal/UpnpIOServiceImpl.java#L434. Maybe @wborn has an opinion on this.
I am still OK to solve it in the binding if the other options don’t work out, but there are 2 underlying layers (upnp transport and service) that in my opinion should be responsible for the stability of the connection.

[digitaldan]

I'm not familiar at all with the UPNP backend, but i'll try taking a look, may take a little bit.

[digitaldan]

@mherwege just looking at the code you referenced and how its called from this binding, what does "GetCurrentConnectionIDs" mean in this code?

https://github.com/digitaldan/openhab-addons/blob/06b361c13904f6ea12e42ff594a23ed0df702314/bundles/org.openhab.binding.upnpcontrol/src/main/java/org/openhab/binding/upnpcontrol/internal/handler/UpnpHandler.java#L620

[mherwege]

This is a standard uPnP action that is always implemented on all of the different services involved with media. It should always return something. So that’s why it has been chosen for the polling. If it fails, something is wrong with the connection. As this polling is done anyway, it consider this a potential place to add the M-SEARCH code.

[andrewfg]

@digitaldan as mentioned in openhab/openhab-core#4527 my proposal is to apply a (temporary) fix in the binding that does NOT rely on any eventual change in OH core.

Furthermore -- instead of your current code -- I would suggest to try one or more of the following code lines either a) one time only during your handler factory initialization, or b) periodically analogous to what you are doing in your current code.

upnpService.getRegistry().addListener(this); 

// and/or
upnpService.getControlPoint().search(); 

// and/or
upnpService.getControlPoint().search(new RootDeviceHeader()); 

[digitaldan]

Furthermore -- instead of your current code

I can do that, my only thought with the current code was to target a specific device vs triggering a search request for any device jUPnP might care about which would cause some noise for other bindings that may be listening for (new) device activity (like Sonos?) Was trying to be nice and keep the collateral noise to just devices used in the binding.

If this is not a problem (and i don't think it would be) , then i'm happy to switch to using your suggestion ;-)

[andrewfg]

@digitaldan to make your search more specific you could use the following. And instead of searching every one minute, you could perhaps check the device.getIdentity().getMaxAgeSeconds() value and send even fewer requests. (The max age expiry is usually between 5 and 30 minutes).

upnpService.getControlPoint().search(new UDNHeader(udn));

[digitaldan]

perhaps check the device.getIdentity().getMaxAgeSeconds()

I like that idea, i'll look into it.

[andrewfg]

I like that idea, i'll look into it.

@digitaldan for the time being you could borrow my code for this addon.