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

[upnpcontrol] Send periodic keep alive #17976

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

digitaldan
Copy link
Contributor

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

@digitaldan digitaldan requested a review from mherwege as a code owner December 24, 2024 21:31
@lsiepel lsiepel added the bug An unexpected problem or unintended behavior of an add-on label Dec 24, 2024
Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

Besides the single comment, LGTM.

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

…hab/binding/upnpcontrol/internal/handler/UpnpRendererHandler.java

Co-authored-by: lsiepel <[email protected]>
Signed-off-by: Dan Cunningham <[email protected]>
@digitaldan
Copy link
Contributor Author

Thanks, i would suggest backporting this one.

@mherwege
Copy link
Contributor

mherwege commented Dec 25, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of an add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[upnpcontrol] Device goes offline with device not registered error
3 participants