Add Philips Hue Adapter #99

hobinjk · 2017-06-14T17:29:13Z

It exposes each light as an onOffSwitch.

There's currently an issue in the first run where the bridge pairs then shows only one device which is missing the id key required for being saved as a thing.

This should be ready once that issue is fixed.

hobinjk · 2017-06-15T14:46:13Z

I figure out the issue! It turns out that discoverLights would keep adding new lights which would in turn cause handleNewThing to occur for the first light to be added.

@dhylands This should be good to go whenever you're available

dhylands

I had an issue when I initially paired the hub. One of 3 lights showed up, but I when I clicked Save, nothing happened. When I cancelled out of adding, and re-entered then my 3 hue bulbs showed up and the save worked as expected.

dhylands · 2017-06-15T20:09:37Z

adapters/philips-hue/philips-hue-adapter.js

@@ -0,0 +1,233 @@
+/**
+ *
+ * PhilipsHueAdapter - an adapter for controlling TP-Link Smart Lights


Typo? (reference to TP-Link?)

dhylands · 2017-06-15T20:15:51Z

adapters/philips-hue/index.js

+'use strict';
+
+var PhilipsHueAdapter = require('./philips-hue-adapter');
+var rp = require('request-promise-native');


request (a dependency of request-promise-native) appears to be missing from package.json

tip: When adding lots of new stuff, I generally like to clone the repository into another directory and verify that it runs from a fresh clone.

That helps to identify missing dependencies as well as files you may have forgotten to add to the PR (one of my favorite tricks).

Thanks for the heads-up! I assumed incorrectly that request-promise-native would have request as a dependency instead of a peer dependency.

dhylands · 2017-06-15T20:57:27Z

adapters/philips-hue/philips-hue-adapter.js

+    this.pairing = true;
+    var endTime = Date.now() + timeoutSeconds * 1000;
+
+    var attemptPairing = () => {


I guess I don't understand why you create a function and then call it rather than just put the contents inline?

dhylands · 2017-06-15T20:58:42Z

adapters/philips-hue/philips-hue-adapter.js

+        return storage.init();
+      }).then(() => {
+        return storage.getItem(KNOWN_BRIDGE_USERNAMES);
+      }).then(knownBridgeUsernames => {


Shouldn't knownBridgeNames have parens around it?

I personally believe the code looks cleaner without them, especially in cases like catch(e => {}). I can add them here, but is there a style guide I should be following?

For me, I guess it was more a case of not realizing that you could even do it that way.

The only style guide that we really have is the suggestions from the linter, and this passed the linter. @benfrancis may have an opinion, but otherwise I think its fine.

dhylands · 2017-06-15T20:59:37Z

adapters/philips-hue/philips-hue-adapter.js

+    var attemptPairing = () => {
+      this.pair().then(username => {
+        this.username = username;
+        return this.discoverLights();


It also isn't clear to me why you're returning anything when the return value isn't used?
I've seen this being used before - and I'm probably just not getting something.

dhylands · 2017-06-15T21:01:37Z

adapters/philips-hue/philips-hue-adapter.js

+        }
+        knownBridgeUsernames[this.bridge.id] = this.username;
+        return storage.setItem(KNOWN_BRIDGE_USERNAMES, knownBridgeUsernames);
+      }).catch(e => {


I find it useful to do console.error(e) this then prints a complete backtrace when e is an error. When you add anything else (like in the line below) then it prints the string representation of e which doesn't include the backtrace.

dhylands · 2017-06-15T21:21:57Z

adapters/philips-hue/philips-hue-adapter.js

+
+  /**
+   * Perform a single attempt at pairing
+   * @return {Promise} Resolved with username if pairing succeeds


Is this paring the hub? or pairing a light?

Pairing a hub then discovering lights, I updated the comment to be a bit more clear

hobinjk · 2017-06-16T15:45:06Z

The problem where 1 light shows up and can't be added is related to the way that startPairing works. I'm trying to write a test to demonstrate this, but the behavior I've found is that only one device can be added during an instance of pairing. It gets this one device from models/action.js handling the 'pair'.

AdapterManager.addNewThing().then(function(thing) {
  Things.handleNewThing(thing);
  action.status = 'completed';
})...

Once the Hue hub is paired and can create lights, the mechanism that causes those multiple devices to show up is the NewThingsController.ws or NewThingsController.get handlers. These don't function in the first run because they each only run getNewThings once at the beginning when the hue hub is still unpaired.

I don't really know how to fix this issue, although I could probably fix the issue causing the one device that shows up to be unable to be saved.

hobinjk · 2017-06-16T17:37:08Z

There were two issues in play. First, the "Send Thing instead of description" allows the user to save Things that are dynamically added while the Add New Things menu is open.

Second, the Things/Actions models now properly work together to communicate all newly available Things instead of just the first that appears. Actions was using the Promise resolution as its only source of new things, meaning that all of the pairing attempts would "race" to be the first to resolve the Promise. The winning Thing would then be sent via handleNewThing to the Add New Things menu. I replaced this one-shot approach with a listener attached to AdapterManager's thing-added event that calls handleNewThing.

I'm now looking at how to write a test that will demonstrate this behavior.

(there was a bit of delay after I spilled half a cup of tea on my computer causing its display to short out and its keyboard to die, but the passage of time is a perfect panacea)

hobinjk · 2017-06-16T17:41:27Z

@benfrancis I think you probably have some opinions on the movement of handleNewThing (at least that's what git blame tells me :P). Is this the proper approach to take?

hobinjk · 2017-06-16T21:00:05Z

Since the changes to AdapterManager, NewThingsController, and the Actions model are pretty far removed from the Philips Hue adapter I've moved them to #103

benfrancis · 2017-06-19T13:20:32Z

Only being able to pair one "Thing" at a time was actually an intentional design decision. IIRC this is because the push button commissioning method used by ZigBee and Z-Wave creates a security vulnerability in that the pairing mechanism allows any device to connect to the hub while in pairing mode without any form of authentication. A malicious device could try to pair with the hub while it is in pairing mode and go unnoticed by the user as they add the device they intended to add.

By locking this down to only allow one device to be paired at a time it makes it much more obvious to the user when a device was added unintentionally. However, with the UI we ended up with this does get confusing because you can still have multiple devices displayed in the "add device" dialog and "pairing" and "adding" have become separate user interactions. So maybe its right to re-visit this decision.

I guess this becomes more of an issue when you pair the gateway with another hub which may have multiple devices already connected to it. I personally don't like the idea of a "hub of hubs" and think we should focus on supporting standards like ZigBee, Z-Wave and Bluetooth, but I know Dave feels differently about this. I'm sure there will be demand for a Phillips Hue adapter and the encryption used by the Zigbee Light Link profile makes it tricky to support the bulbs directly. Regardless, our pairing UI is probably going to need some refinement to support the quirks of the various pairing mechanisms used by devices!

See my review in #103 for the specifics around the changes to /new_things

hobinjk · 2017-06-19T14:08:00Z

Okay, so if I understand correctly the ZigBee and Z-wave startPairing tell the adapter to permit any device to pair with it.

The interpretation of startPairing that I use with the Philips Hue is that the adapter begins attempting to get an API key from the hub. Notably, this then requires the user to press the physical button on top of the hub to allow the acquiring of the API key. I definitely agree that there's room for a clearer separation of pairing adapters and adding devices, which would also solve the problem of how to prompt a user to press the hub's button if and only if they are attempting to pair a Philips Hue hub.

dhylands · 2017-06-19T16:43:01Z

ZigBee always allows multiple devices to join for the duration that the controller is pairing. What the adapter does is that it uses the first one to resolve the promise. Remaining devices that get added are just added to the adapter manager and will be visible the next time you enter the Add Thing screen.

I think that having the new devices be posted via a web socket would allow them to appear in real-time.

dhylands · 2017-06-19T16:51:02Z

I also think that we need to think of a way that the adapter can tell the UI that it needs user input.

For Zigbee, adding a new device doesn't require pressing a button. If the adapter is in pariing mode and you plug in an unparied device, it will automatically get added with no button press required.

For Hue, it would be nice to prompt the user that the hub needs to be paired (perhaps at the very beginning of the add process).

For ZWave, you actually need to press a button on the device, and if the user was used to the ZigBee behaviour of not having to press a button, then they might not realize that they're supposed to press a button.

benfrancis · 2017-06-19T16:55:49Z

Yeah it doesn't really make sense as currently implemented. The original intention was that adding and pairing would be the same thing and you could only add/pair one device at a time. The reality turned out to be different as the underlying protocols allow multiple devices to be paired and we just don't show them, which is much worse. "Adding" (to the database) is currently a separate interaction to "pairing" (with the dongle) and we currently have no UI for "unpairing", only "removing".

This can definitely be improved but I'm hesitant to try to make major changes to the way it works a week before release. I'd suggest we fix the bugs we find (like the one James seems to have found, and the fact we don't show all the devices that are paired) and hold off adding new features (including new adapters that require major changes to the core) until the next release.

As an aside, I think it was someone from Develco who suggested only allowing one ZigBee device to pair at a time, as a mitigation for using push button commissioning without requiring the entry of a serial number in the UI as an extra security measure.

codecov-io · 2017-06-23T20:20:01Z

Codecov Report

Merging #99 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master     #99   +/-   ##
======================================
  Coverage    82.6%   82.6%           
======================================
  Files          30      30           
  Lines         920     920           
======================================
  Hits          760     760           
  Misses        160     160

Impacted Files	Coverage Δ
config/test.js	`100% <ø> (ø)`	⬆️
config/default.js	`100% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 050e2db...dff1ca4. Read the comment docs.

andrenatal · 2017-09-22T20:03:53Z

@hobinjk what's the current status of this PR? Would we be able to demo this feature too?

hobinjk · 2017-09-22T20:53:41Z

@andrenatal I've spun it off into https://github.com/hobinjk/philips-hue-adapter and the current plan is to use the adapter API once it's finished although for now I've been manually adding the adapter to config/default.js

hobinjk force-pushed the philips-hue-adapter branch 2 times, most recently from 7e6a446 to 7ec6560 Compare June 15, 2017 14:37

hobinjk force-pushed the philips-hue-adapter branch from b574c60 to 6445650 Compare June 15, 2017 17:19

dhylands reviewed Jun 15, 2017

View reviewed changes

hobinjk force-pushed the philips-hue-adapter branch from 522172f to c14533c Compare June 16, 2017 20:55

hobinjk mentioned this pull request Jun 16, 2017

/new_things improvements #103

Closed

hobinjk force-pushed the philips-hue-adapter branch from c14533c to 7769e4a Compare June 23, 2017 20:14

hobinjk added 8 commits June 23, 2017 16:15

Add Philips Hue Adapter

ad751bb

Add SSDP-based discovery to Philips Hue Adapter

96825e4

Disable Philips Hue adapter in test config

4a95f67

Add missing dependency on request

ae0956e

Fix typo

897cccb

Refactor attemptPairing to method

97a8530

Clarify documentation

a75c7ea

Use console.error without string description

dff1ca4

hobinjk force-pushed the philips-hue-adapter branch from 7769e4a to dff1ca4 Compare June 23, 2017 20:16

hobinjk closed this Sep 22, 2017

mrstegeman added this to Done in WebThings Gateway Apr 10, 2019

mrstegeman removed this from Done in WebThings Gateway Apr 10, 2019

mrstegeman added this to In progress in WebThings Gateway via automation Apr 10, 2019

mrstegeman moved this from In progress to Done in WebThings Gateway Apr 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Philips Hue Adapter #99

Add Philips Hue Adapter #99

hobinjk commented Jun 14, 2017

hobinjk commented Jun 15, 2017

dhylands left a comment

dhylands Jun 15, 2017

dhylands Jun 15, 2017

hobinjk Jun 16, 2017 •

edited

dhylands Jun 15, 2017

dhylands Jun 15, 2017

hobinjk Jun 16, 2017

dhylands Jun 16, 2017

dhylands Jun 15, 2017

dhylands Jun 15, 2017

dhylands Jun 15, 2017

hobinjk Jun 16, 2017

hobinjk commented Jun 16, 2017

hobinjk commented Jun 16, 2017 •

edited

hobinjk commented Jun 16, 2017

hobinjk commented Jun 16, 2017

benfrancis commented Jun 19, 2017

hobinjk commented Jun 19, 2017

dhylands commented Jun 19, 2017

dhylands commented Jun 19, 2017

benfrancis commented Jun 19, 2017

codecov-io commented Jun 23, 2017 •

edited

andrenatal commented Sep 22, 2017

hobinjk commented Sep 22, 2017

Add Philips Hue Adapter #99

Add Philips Hue Adapter #99

Conversation

hobinjk commented Jun 14, 2017

hobinjk commented Jun 15, 2017

dhylands left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hobinjk Jun 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hobinjk commented Jun 16, 2017

hobinjk commented Jun 16, 2017 • edited

hobinjk commented Jun 16, 2017

hobinjk commented Jun 16, 2017

benfrancis commented Jun 19, 2017

hobinjk commented Jun 19, 2017

dhylands commented Jun 19, 2017

dhylands commented Jun 19, 2017

benfrancis commented Jun 19, 2017

codecov-io commented Jun 23, 2017 • edited

Codecov Report

andrenatal commented Sep 22, 2017

hobinjk commented Sep 22, 2017

hobinjk Jun 16, 2017 •

edited

hobinjk commented Jun 16, 2017 •

edited

codecov-io commented Jun 23, 2017 •

edited