michielpost / Q42.HueApi

C# helper library to talk to the Philips Hue bridge
MIT License
412 stars 114 forks source link

Bridge discovery approaches #198

Closed Indigo744 closed 4 years ago

Indigo744 commented 4 years ago

Hello,

I have been using your lib since several days, and I must say I'm very pleased with the result. Thank you so much for your work on it.

The only thing I have trouble with is bridge discovery on the network.

Philipps provides a nice guidance as how to discover a Hue bridge:

  1. Use UPNP
  2. Use N-UPNP (uses https://discovery.meethue.com)
  3. Use mDNS
  4. Use network scan

Looking at the source code of HttpBridgeLocator, I clearly see that only the approach 2 (NUPNP) is supported by this lib.

When looking in the issues of this repo, I came across some issues talking about UPNP discovery (#3 and #151) and referencing the SSDPBridgeLocator class.

However, it seems it was deprecated in 3.8.1 without a single explanation on why. So now I am quite confused.

My questions are:

  1. Why did you deprecate SSDPBridgeLocator? Was it not working? Should we use something else?
  2. Do you plan to support more way of discovering the bridge? Especially for non-internet environment?

Thank you for your time.

michielpost commented 4 years ago

The SSDPBridgeLocator used to work, but only on the .Net 4.5 framework. It was deprecated because this library moved to netstandard. I've tried to get it to work on .netstandard, but without success. With some changes the code compiled, but it never found any bridges anymore. So because I couldn't get it to work, it was removed.

If somebody creates a PR with a working UPNP solution, I'd be happy to include it in the library again., but I'm not planning to work on it myself.

There are also some other libraries that have more knowledge about this subject, for example:

Maybe it's easier to do the bridge discovery using those libraries, or ask their owners how to implement bridge discovery. If you have working code using one of those libraries it would also be helpful to share it here.

Indigo744 commented 4 years ago

Thank you for your reply. I'll try to look into it and eventually make a PR.

Would you accept PR regarding other bridge discoveries? Like mDNS or network scan? How would you prefer these new locating approaches to be defined? In other class? Project?

michielpost commented 4 years ago

Yes, a PR with bridge discovery using whatever method is welcome :) The bridge discovery should implement the 'IBridgeLocator' interface: https://github.com/Q42/Q42.HueApi/blob/master/src/Q42.HueApi/Interfaces/IBridgeLocator.cs

And the code should be in a class library with at least netstandard2.0 support. If no additional NuGet package references are needed, it can be included in the main Q42.HueApi class library. But if the bridge discovery uses another external library, it's best to create a new project for example Q42.HueApi.BridgeDiscovery.

Indigo744 commented 4 years ago

Alright. I'll keep you posted 😸

Indigo744 commented 4 years ago

When the last PR lands, I would love to have a Nuget release please 😺

Then I'll share here an example of an implementation of a robust bridge discovery using all of these locators.

d8ahazard commented 4 years ago

Yes, I'm also waiting for this to land on NuGet. :D

With the current .13 package, was something subsequently fixed that may explain this issue?:

System.AggregateException: One or more errors occurred. (Only one usage of each socket address (protocol/network address/port) is normally permitted.) ---> System.Net.Sockets.SocketException (10048): Only one usage of each socket address (protocol/network address/port) is normally permitted. at System.Net.Sockets.Socket.UpdateStatusAfterSocketErrorAndThrowException(SocketError error, String callerName) at System.Net.Sockets.Socket.DoBind(EndPoint endPointSnapshot, SocketAddress socketAddress) at System.Net.Sockets.Socket.Bind(EndPoint localEP) at Q42.HueApi.SsdpBridgeLocator.LocateBridgesAsync(TimeSpan timeout) --- End of inner exception stack trace --- at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions) at System.Threading.Tasks.Task1.GetResultCore(Boolean waitCompletionNotification) at System.Threading.Tasks.Task1.get_Result() at HueDream.Hue.HueBridge.FindBridges() in D:\DEV\HueDream\Hue\HueBridge.cs:line 158

I'm calling reuseaddress on all of my socket instances, and I see you're setting it in the SsdpBridgeLocator I'm calling at line 158 in my code, so I'm guessing that I'm just outdated yet.

Indigo744 commented 4 years ago

@d8ahazard thanks for reporting this issue.

After investigating it, I found a way to fix and improve the locators. @michielpost I'll submit a new PR after the MDNS one lands.

michielpost commented 4 years ago

Version 3.14.0 has been submitted to NuGet.org and should be there shortly. Thanks for all the PRs!

Indigo744 commented 4 years ago

Here is a gist with some approaches I came up with: https://gist.github.com/Indigo744/fd55456077450bce1382cf1f957b6b59

It is inspired from the Philips documentation flowcharts: image

N-UPNP, MDNS and SSDP are run in parallel for better performance.

michielpost commented 4 years ago

@Indigo744 do you think it would be a good idea to include your HueBridgeDiscovery examples in the library?

Indigo744 commented 4 years ago

If you want, I have no problem with that 😺

michielpost commented 4 years ago

Added the HueBridgeDiscvoery to Q42.HueApi: https://github.com/Q42/Q42.HueApi/pull/208