[hue] Fix NUPnP discovery

[jlaur]

Fixes #14852
Regression of #11842

[jlaur]

Please consider this for being backported to 3.4.x.

[lolodomo]

LGTM

[andrewfg]

@jlaur / @lolodomo as with all ongoing PRs for this binding, your #14871 is merged into my #13570 .. however I have a question for you both concerning that..

Background: when mDNS discovery is used, then the discovery service checks if the respective bridge supports API v2 (it makes an API v2 GET call which return 200 OK if API v2 is supported). And if it is supported it puts an API v2 thing in the inbox (and NOT a legacy API v1 thing). Reason is that for newly discovered things it is better for the user to start with the new technology and not the legacy.

Now that you have re-enabled NUPnP discovery via this PR, the (legacy) mechanism once more puts a legacy thing in the inbox. So now if the bridge supports API v2, the mDNS discovery puts a v2 thing in the inbox, and the legacy NUPnP discovery puts a second (legacy) thing in the inbox. The risk of this is that the user might click on the legacy bridge thing, and start creating a whole system based mistakenly on legacy stuff.

There are two possible solutions..

1. Either I do the same with NUPnP discovery as I have done with mDNS i.e. it checks if the discovered thing supports API v2 and depending on the result it puts either a v2 or a legacy thing in the inbox.
2. Or I disable the NUPnP discovery again..
3. Or I do nothing..

I suppose 1. is the right answer, but perhaps you can confirm? Note in case of 1. if the bridge really does not support API v2 then a legacy thing will anyway be placed in the inbox (via either mDNS or NUPnP or both).

[lolodomo]

In fact, I am not sure what is and what was the interest of the NUPnP cloud discovery ?

[lolodomo]

By the way, we need to include again the UPnP discovery class (removed in OH3) as there is no more discovery of old bridge with the current code. I hope the old bridge will still expose itself through UPnP.

[jlaur]

1. Either I do the same with NUPnP discovery as I have done with mDNS i.e. it checks if the discovered thing supports API v2 and depending on the result it puts either a v2 or a legacy thing in the inbox.
2. Or I disable the NUPnP discovery again..

It was not disabled, it was broken - and by that of course indirectly disabled.

3. Or I do nothing..

I suppose 1. is the right answer, but perhaps you can confirm? Note in case of 1. if the bridge really does not support API v2 then a legacy thing will anyway be placed in the inbox (via either mDNS or NUPnP or both).

See #14852 (comment) - asking the same question.

I don't understand why we have an implementation of cloud discovery when the binding works locally and we have local discovery options. So I would opt for completely removing the NUPnP discovery, unless someone steps up and explains why there would be a need for this.

[andrewfg]

In the meantime I made this commit but we can revert it later if you wish..

don't understand why we have an implementation of cloud discovery

Good point.

The following is not directly related to discovery, but may nevertheless be relevant..

Currently there are two ways to authenticate OH on the bridge -- namely locally (press button on hub) to get an application key, and via cloud V1 OAuth2 authentication. Philips / Signify implies that a local application key is not very secure, and they imply that better security can be had from using OAuth2 cloud authentication. Notwithstanding this, Philips / Signify have just announced that V1 OAuth2 will be discontinued on July 1st 2023 and say 'in the future there will be another way to authorize on the V2 API'. So, I interpret all of this to mean that Philips / Signify is planning to introduce some API v2 OAuth2 cloud based work flow with a username and password whereby the application key would probably become time limited in some way. This all just for info..

https://developers.meethue.com/v1-oauth2-to-be-disabled-on-july-1-2023/

EDIT: 4yi I have not implemented API v2 OAuthV2 in my PR because, I don't really understand how OAuth2 works :(

[kaikreuzer]

I don't understand why we have an implementation of cloud discovery when the binding works locally and we have local discovery options.

Afair the history, the reason for the introduction was that it there were quite some users where local UPnP discovery didn't work as expected - be it due to different local subnets, blocked UPnP services IPv6 or whatsoever. So the as the cloud discovery was offered by Philips (and the hue app was using it as a fallback as well), this was also implemented in openHAB as a secondary discovery option.

[jlaur]

Afair the history, the reason for the introduction was that it there were quite some users where local UPnP discovery didn't work as expected - be it due to different local subnets, blocked UPnP services IPv6 or whatsoever. So the as the cloud discovery was offered by Philips (and the hue app was using it as a fallback as well), this was also implemented in openHAB as a secondary discovery option.

Thanks for sharing this, @kaikreuzer. So we should keep this.

[lolodomo]

I am going to restore UPnP discovery for old bridges but I would like to do it only for old bridges.
Here is the data exposed by my hue bridge through UPnP:

What field should I consider to filter only old bridges ? The year in "Model name" ? Model number ? Something from the serial number ?
Or should I filter nothing because all recent bridges do not expose themself through UPnP ?

[andrewfg]

I will run Device Spy on mine, and let you know what I get.

[andrewfg]

Here you go. There seem to be quite a few potential markers that you could use.

[lolodomo]

Ok so UPnP is still available for recent bridges. In that case, I have to apply a filter.
"Model number" seems to be the ideal field. WDYT if I ignore bridges when the model number starts with "B" ?

[lolodomo]

I submitted a PR. Of course it is tested and working with a bridge V1. And it is expected to not discover bridge V2.

I can see that the model id retrieved from API is BSB001 in my case (the modelId property is updated when the binding starts) while "Model number" in UPnP discovery is a different value.
For more recent bridges, it looks like BSB002 is probably the value in both places.

[andrewfg]

There might be problems if they would release a new bridge with a model number not starting with B. So perhaps do a Regex search for a year in model name, and only search if it is below 2015?
Assuming BSB001 was 2012 and BSB002 was 2015.

[lolodomo]

There might be problems if they would release a new bridge with a model number not starting with B. So perhaps do a Regex search for a year in model name, and only search if it is below 2015?
Assuming BSB001 was 2012 and BSB002 was 2015.

Done, I believe it is a better option.

[jlaur]

So the as the cloud discovery was offered by Philips (and the hue app was using it as a fallback as well), this was also implemented in openHAB as a secondary discovery option.

Getting back to NUPnP as "secondary" discovery option, I noticed that the label for the discovery result changed back and forth when scanning (XXXXXX is last 6 characters of the serial number):

• Philips Hue - XXXXXX (192.168.0.251)
• Philips Hue (192.168.0.251)

I added some more logging:

2023-05-01 21:54:54.859 [INFO ] [al.discovery.HueBridgeNupnpDiscovery] - Hue bridge discovered by NUPnP
2023-05-01 21:54:56.027 [INFO ] [ry.HueBridgeMDNSDiscoveryParticipant] - Hue bridge discovered by mDNS

So it seems that the two discovery results are fighting. It shouldn't be a real issue except the flickering caused by the label switching. It should be possible to make the labels consistent, I'll have a look at that.

FYI @kaikreuzer, @lolodomo, @andrewfg

[andrewfg]

flickering caused by the label switching. It should be possible to make the labels consistent

Good point. Obviously the most important thing is the representationProperty which must be identical otherwise othwerwise one gets two things 'flickering' rather than one thing with its label 'flickering..

[lolodomo]

flickering caused by the label switching. It should be possible to make the labels consistent

Good point. Obviously the most important thing is the representationProperty which must be identical otherwise othwerwise one gets two things 'flickering' rather than one thing with its label 'flickering..

But I believe the configuration parameters should also be the same ?
And even with identical label and configuration parameters, I guess it will trigger an update of the thing (if the thing was added from inbox without changing its id. So two discoveries will trigger two thing updates.

[andrewfg]

believe the configuration parameters should also be the same
guess it will trigger an update of the thing (if the thing was added from inbox without changing its id)

Hmm. I don't think so -> https://www.openhab.org/docs/developer/bindings/thing-xml.html#representation-property-and-discovery

EDIT: oops -- I see this "and both the properties and the configuration parameters of existing Things"

[andrewfg]

@jlaur there is unfortunately still a bug here. As you know I synchronized your PR into my own API v2 PR, and two of the people who are running that version reported that the nupnp discovery crashes with the stack trace shown below.

The issue is that the getBridgeList() method calls doGetRequest() and this does not return HTTP 200 'Ok' containing a valid JSON content, but instead returns HTTP 429 'too many requests' and no JSON content. And that in turn causes the Objects.requireNonNull(..) check to fail.

One of the two users also cross checked it by opening the https://discovery.meethue.com/ link in his browser (from inside his LAN), and you can see the 429 reported by the browser.

12:56:53.733 [WARN ] [ommon.WrappedScheduledExecutorService] - Scheduled runnable ended with an exception:
java.lang.NullPointerException: null
        at java.util.Objects.requireNonNull(Objects.java:208) ~[?:?]
        at org.openhab.binding.hue.internal.discovery.HueBridgeNupnpDiscovery.getBridgeList(HueBridgeNupnpDiscovery.java:146) ~[?:?]
        at org.openhab.binding.hue.internal.discovery.HueBridgeNupnpDiscovery.discoverHueBridges(HueBridgeNupnpDiscovery.java:75) ~[?:?]
        at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:539) ~[?:?]
        at java.util.concurrent.FutureTask.run(FutureTask.java:264) ~[?:?]
        at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:304) ~[?:?]
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136) ~[?:?]
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635) ~[?:?]
        at java.lang.Thread.run(Thread.java:833) ~[?:?]

[openhab-bot]

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

https://community.openhab.org/t/hue-binding-with-added-support-for-api-v2/146648/10

[jlaur]

there is unfortunately still a bug here

Thanks, fixed in #14989.