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

Add Bridge discovery #13

Closed
ZeroOne3010 opened this issue Nov 8, 2019 · 10 comments
Closed

Add Bridge discovery #13

ZeroOne3010 opened this issue Nov 8, 2019 · 10 comments

Comments

@ZeroOne3010
Copy link
Owner

Bridge discovery should be added. The Philips documentation is somewhat internally inconsistent, but the preferred order seems to be approximately like this:

  1. N-UPnP
  2. mDNS
  3. UPnP
  4. IP scan

Implement at least one of those.

@JasperPluijmers
Copy link

I think for this protocol this is fine, but for UPnP for example different bridges could present themselves at different times, a more asynchronous setup would be nice.
It would be more logical of there would be a continuing (for as long as the discover process is going on) listening process with callbacks or implementable methods like onBridgeFound(Bridge bridge).
(sorry of not completely clear, I will try to make something)

@JasperPluijmers
Copy link

Sorry for ghosting, I made what I meant in a pull request. It is not very pretty and doesn't have any tests or comments yet. What do you think about it?

@bwalsh0
Copy link

bwalsh0 commented Jan 5, 2020

I found the N-UPnP method to be the most straightforward. I have it working alongside your library, I might expand it and create a pull request if I can find the time.

You need to get the JSON response from https://discovery.meethue.com/, supports multiple Bridges. Response is one JSON Array with one object for each Bridge. Note the precautions mentioned on Hue's webpage.

Should run on a background thread, I have used Asynctask below. I'm also using OkHttp3, but HttpsUrlConnection works fine as well. This code just returns the first Bridge in the JSON Array, it's pretty bare but I hope it helps guide people in the right direction.

// ...AsyncTask<Void, Void, String> {
//
@Override
        protected String doInBackground(Void... voids) {
        // Establish connection
            OkHttpClient.Builder client = new OkHttpClient.Builder();
            Request request = new Request.Builder()
                    .url("https://discovery.meethue.com/")
                    .build();

            Response response = null;
            String result = null;
            try {
                response = client.build().newCall(request).execute();
                if (!response.isSuccessful()) {
                    return "";
                } else {
                    result = response.body().string();
                }
            } catch (IOException e) {
                e.printStackTrace();
            }

      // Parse JSON response
            try {
                JSONArray json = new JSONArray(result);
                String bridgeIp, bridgeId;

                for (int i = 0; i < json.length(); ++i) {
                    JSONObject bridge = json.getJSONObject(i);
                    bridgeId = bridge.getString("id");
                    bridgeIp = bridge.getString("internalipaddress");
                    Log.e(TAG, "Id: " + bridgeId + "\n" + "IP: " + bridgeIp);
                    return bridgeIp;          // Return IP address of first Bridge
                }
            } catch (JSONException e) {
                e.printStackTrace();
            }
            return "";
        }

Fields are as follows, if at least one Bridge is found:

id: <string> (always present)
internalipaddress: <string> (always present)
macaddress: <string> (might be empty/null)
name: <string> (might be empty/null)

@JasperPluijmers
Copy link

The problem is for the N-UPnP discoverer you need to have connection with internet, this is not always possible.

@ZeroOne3010
Copy link
Owner Author

Thanks @JasperPluijmers and @bwalsh0! I pushed Jasper's code into a new branch called async-discoverer and made a couple of minor edits. Development can continue in the branch. Some thoughts:

  • A UPnPDiscovererTestRun class would be useful: this would be a class in the test project that would actually execute the UPnP discoverer. See NUPnPDiscovererTestRun for comparison. This is not an automatic test, but one could execute it separately to test the functionality with their real setup.
  • Some automatic unit tests would be good too.
  • It would seem like a good idea to prevent the UPnPDiscoverer from running indefinitely if nobody calls the stop method. The documentation (https://developers.meethue.com/develop/application-design-guidance/hue-bridge-discovery/) says that the best practice is to wait for a maximum of 5 seconds.
  • @bwalsh0, I'm not sure if you noticed, but I have already implemented the N-UPnP discoverer. :) It's just using a different interface than Jasper's UPnP discoverer...
  • ...which reminds me, they should both use the same asynchronous interface.

@bwalsh0
Copy link

bwalsh0 commented Jan 7, 2020

@ZeroOne3010, Didn't realize NUPnPDiscoverer was already implemented, will transition to it since your class is much more complete. Thank you for your work on this library.

ZeroOne3010 added a commit that referenced this issue Jan 12, 2020
Adding a short wait (TimeUnit.MILLISECONDS.sleep(100L);) after the
instantiation of a new MulticastSocket makes the code much more reliable
by allowing some time for the port to actually get ready.
ZeroOne3010 added a commit that referenced this issue Jan 15, 2020
This seems less complicated than the original version with the abstract
class. Also, this should be more in line with how the synchronous
discovery works so that eventually both methods can be unified behind
a single interface.
@ZeroOne3010
Copy link
Owner Author

OK, I think I've mostly completed the bullet points I outlined above, save for automatic unit tests or integration tests, as the interface has been changing so rapidly. Note to self from the documentation:

In case the application gets a non-empty response from the bridge, multiple bridges may have been found. Always validate information from these bridges before giving the user the option to select one bridge from the list for further control. There are two ways to check the connection with the bridge and retrieve some basic information from that bridge.

...so that still needs to be done.

@ZeroOne3010
Copy link
Owner Author

I would still like to add mDNS discovery as well, but let's see if it'll be included in the first version or not. The IP scan seems like an overkill after all those other options, I don't really fancy doing that.

@ZeroOne3010
Copy link
Owner Author

Whew... So, a few weeks ago I realized the UPnP discoverer wasn't working reliably at all and have been banging my head against the wall since. Today I finally figured it out: the SSDP message required quotation marks around one of the values..: cc1aea3 🙄 In other words, progress is being made, but not as quickly as I would've liked.

@ZeroOne3010
Copy link
Owner Author

Added an integration test for the UPnP discoverer and merged it all to master. 💪 UPnP and NUPnP are now supported. I'm going to go ahead and close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants