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

[hue] Implementing "[discovery.upnp] Devices may apply a grace period" #9985

Merged
merged 7 commits into from
Feb 9, 2021

Conversation

andrewfg
Copy link
Contributor

@andrewfg andrewfg commented Jan 29, 2021

BACKGROUND

The JUPnP library strictly follows the UPnP specification insofar as if a device fails to send its next 'ssdp:alive' notification within its declared 'maxAge' period, it is immediately considered to be gone. The Philips Hue can sometimes be late in sending its 'ssdp:alive' notifications even though it has not really gone offline. This means that the Hue Bridge is repeatedly removed from, and (re)added to, the InBox. There may be up to fifty such removals and (re)addings per day. This leads to confusion in the UI, and repeated logger messages.

SOLUTION PART A - CORE

I have made a Pull Request for a change in discovery.upnp that allows bindings to specify an additional grace period that the core UPnP discovery service shall wait before it removes a Thing from the Inbox. The change in [discovery.upnp] comprises an extension to the UpnpDiscoveryParticipant interface adding a new default int getRemovalGracePeriodSeconds() method. The change in [discovery.upnp] is implemented so that any existing binding inherits the default method, and its discovery behaviour is thus not changed.

SOLUTION PART B - HUE BINDING

This (HUE) binding overrides the default int getRemovalGracePeriodSeconds() method, and this asks for an additional grace period, which the core UPnP discovery processes will now honour.

REFERENCE
openhab/openhab-core#2144

Signed-off-by: Andrew Fiddian-Green software@whitebear.ch

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
@cweitkamp cweitkamp linked an issue Jan 30, 2021 that may be closed by this pull request
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
@andrewfg andrewfg added rebuild Triggers Jenkins PR build and removed awaiting other PR Depends on another PR labels Feb 6, 2021
@cweitkamp cweitkamp added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Feb 6, 2021
Copy link
Contributor

@cweitkamp cweitkamp left a comment

Choose a reason for hiding this comment

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

Build is green 👍🏼 . Two remarks.

wrt the configurationPid: we are using bunding.hue for factories and discovery.hue for discovery services. But in this case the second Pid is used for HueBridgeNupnpDiscovery. Don't know if it is really used over there or if we can remove it from that class and assing it to the UPnP discovery. That'll be a streamlined solution compared to other bindings.

We might ask @ghys if he has an idea how we can handle configuration settings for discovery services from the UI in the future.

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
@andrewfg
Copy link
Contributor Author

andrewfg commented Feb 7, 2021

ask @ghys if he has an idea how we can handle configuration settings for discovery services from the UI in the future

It seems to me that the way I have done it, is the solution to the problem. We could even cite the code from this binding in the respective documentation (.md) file.

@andrewfg
Copy link
Contributor Author

andrewfg commented Feb 7, 2021

We could even cite the code from this binding in the respective documentation (.md) file.

Oops. Actually I did that already!
openhab/openhab-docs#1478

@andrewfg
Copy link
Contributor Author

andrewfg commented Feb 7, 2021

we are using binding.hue for factories and discovery.hue for discovery services

@cweitkamp in case you missed it, the point of using the binding.hue config file in this case, (rather than the discovery.hue file), is that the binding.hue config file is read/write editable directly in the UI (whereas discovery.hue requires an external text editor).

And IMHO it doesn't matter how many classes in the same binding get config values from the same config file, as the respective name/value pairs won't clash.

image

@cpmeister cpmeister self-assigned this Feb 9, 2021
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
@cpmeister cpmeister merged commit 18e028a into openhab:main Feb 9, 2021
@cpmeister cpmeister added this to the 3.1 milestone Feb 9, 2021
@andrewfg
Copy link
Contributor Author

Many thanks @cpmeister

J-N-K pushed a commit to J-N-K/openhab-addons that referenced this pull request Feb 10, 2021
openhab#9985)

* [hue] implement PR openhab#2144 in openhab.core
* [hue] add binding configuration parameter; return long

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
@openhab-bot
Copy link
Collaborator

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

https://community.openhab.org/t/org-openhab-binding-hue-internal-discovery-huebridgediscoveryparticipant-threw-an-exception/117247/4

cweitkamp pushed a commit that referenced this pull request Feb 19, 2021
* [hue] extra null check

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
themillhousegroup pushed a commit to themillhousegroup/openhab2-addons that referenced this pull request May 10, 2021
openhab#9985)

* [hue] implement PR openhab#2144 in openhab.core
* [hue] add binding configuration parameter; return long

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: John Marshall <john.marshall.au@gmail.com>
themillhousegroup pushed a commit to themillhousegroup/openhab2-addons that referenced this pull request May 10, 2021
* [hue] extra null check

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: John Marshall <john.marshall.au@gmail.com>
thinkingstone pushed a commit to thinkingstone/openhab-addons that referenced this pull request Nov 7, 2021
openhab#9985)

* [hue] implement PR openhab#2144 in openhab.core
* [hue] add binding configuration parameter; return long

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
thinkingstone pushed a commit to thinkingstone/openhab-addons that referenced this pull request Nov 7, 2021
* [hue] extra null check

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this pull request May 5, 2022
openhab#9985)

* [hue] implement PR openhab#2144 in openhab.core
* [hue] add binding configuration parameter; return long

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this pull request May 5, 2022
* [hue] extra null check

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
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 for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[hue] Hue bridge frequently added to the inbox
5 participants