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

update to new hue-js interface, storing username token in config file #14

Merged
merged 5 commits into from
Sep 22, 2016
Merged

Conversation

mattbucci
Copy link
Contributor

Before merging bahamas10/hue.js#1 and bahamas10/hue.js#2 must be merged

@mattbucci mattbucci mentioned this pull request Sep 15, 2016
@TwizzyDizzy
Copy link

@mattbucci actually, when cloning your master of hue-cli (commit 0026cfa), I'm still getting errors - though different ones:

When using hue search:

dgram.js:445
    throw errnoException(err, 'getsockname');
    ^

Error: getsockname EINVAL
    at exports._errnoException (util.js:1026:11)
    at Socket.address (dgram.js:445:11)
    at next (/opt/node-v6.4.0-linux-armv7l/lib/node_modules/hue-cli/node_modules/hue.js/lib/Discoverer.js:27:25)
    at Object.module.exports [as discover] (/opt/node-v6.4.0-linux-armv7l/lib/node_modules/hue-cli/node_modules/hue.js/lib/Discoverer.js:21:5)
    at Object.<anonymous> (/opt/node-v6.4.0-linux-armv7l/lib/node_modules/hue-cli/hue-cli.js:265:9)
    at Module._compile (module.js:556:32)
    at Object.Module._extensions..js (module.js:565:10)
    at Module.load (module.js:473:32)
    at tryModuleLoad (module.js:432:12)
    at Function.Module._load (module.js:424:3)

When using hue -H 192.168.1.205 register:

please go and press the link button on your base station
failed to pair to Hue Base Station 192.168.1.205

/opt/node-v6.4.0-linux-armv7l/lib/node_modules/hue-cli/hue-cli.js:251
    throw err;
    ^
Error
    at Object.<anonymous> (/opt/node-v6.4.0-linux-armv7l/lib/node_modules/hue-cli/node_modules/hue.js/lib/helpers.js:7:22)
    at Module._compile (module.js:556:32)
    at Object.Module._extensions..js (module.js:565:10)
    at Module.load (module.js:473:32)
    at tryModuleLoad (module.js:432:12)
    at Function.Module._load (module.js:424:3)
    at Module.require (module.js:483:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (/opt/node-v6.4.0-linux-armv7l/lib/node_modules/hue-cli/node_modules/hue.js/lib/Hue.js:5:20)
    at Module._compile (module.js:556:32)

NodeJS version: 6.4.0 (armv7).

@bahamas10
Copy link
Owner

I will test this this weekend when I have access to a newer bridge, thanks!

Also, I created a (terribly named) PR a long time ago with good intentions, but it has unfortunately stagnated. Either way, it may be worth incorporating hue-sdk (my own creation) into this module instead of the seemingly abandoned hue.js.

@mattbucci
Copy link
Contributor Author

@TwizzyDizzy assuming you dropped in my version of hue.js as well? Seems odd though that it's saying something about a socket error. I'll test with my version when I get home.

I'm on node v6.5.0 x86 osx

@mattbucci
Copy link
Contributor Author

mattbucci commented Sep 15, 2016

@bahamas10 I'd be glad to help out in anyway I can, even if that means refactoring to a new hue sdk or updating your hue sdk package to be compatible with the new auth flow

@mattbucci
Copy link
Contributor Author

mattbucci commented Sep 15, 2016

@TwizzyDizzy try changing your package.json to point to mattbucci/master for hue-js, run npm update and see if you have the same problem. My fixes aren't merged to his master yet.

@TwizzyDizzy
Copy link

TwizzyDizzy commented Sep 15, 2016

Yeah, done that. But still... same error (though now just upraded to nodejs 6.6.0 that was released roughly an hour ago).

If you have anything I should test, don't hesitate to hit me up :)

Cheers
Thomas

@mattbucci
Copy link
Contributor Author

@TwizzyDizzy I'll break out the raspi today and see if I can reproduce it, I've got the newer hub btw

@mattbucci
Copy link
Contributor Author

mattbucci commented Sep 16, 2016

@TwizzyDizzy unfortunately my raspi seems dead so you're gonna have to test some untested stuff 👍

So your first error seems to be a common issue with calling sever.address before the port has finished binding....now I can't see why this would ever get executed synchronously but perhaps @bahamas10 would know as it looks like he was the last to touch that.

I would edit the file /opt/node-v6.4.0-linux-armv7l/lib/node_modules/hue-cli/node_modules/hue.js/lib/Discoverer.js changing lines 19-24 from. Your log clearly shows that line 21 is executed

if (client.bind.length) {
    client.bind();
    next(); // this fails because bind hasn't completed yet, not sure bind.length is being checked but it's clearly not working
  } else {
    client.bind(next);
  }

to

    client.bind(next);

Then report back. (For the sake of the reader I'm referencing the docs located here https://nodejs.org/api/dgram.html#dgram_socket_bind_port_address_callback)

As for your second error check out http://192.168.1.205/debug/clip.html and run through http://www.developers.meethue.com/documentation/getting-started to see if you can figure anything else out. are you running a new hub or an old hub? I'm on the new hub but your error seems not to be related to any code but rather a failed response of the hub. Maybe it's an older version and we have to account for both? dunno. You could also try adding a console.log(err) above line 251 of /opt/node-v6.4.0-linux-armv7l/lib/node_modules/hue-cli/hue-cli.js and let us know what the error is that hue is returning

@TwizzyDizzy
Copy link

TwizzyDizzy commented Sep 16, 2016

Mhhh.. that kind of debugging seems to me to be rather cumbersome. If you like, I'd provide you with an unprivileged (read normal user) SSH account on my raspberry so you could test/debug as you like?

Send me your pubkey to [...]@[...] (redacted, after access was granted) if you want.

As for you change above: this lead to an EADDRINUSE error.

I'm running the v2 hub.

Cheers
Thomas

@mattbucci
Copy link
Contributor Author

mattbucci commented Sep 16, 2016

I see where we got confused, @bahamas10 already fixed that issue in his dave branch. I merged that fix of his into my master so that it's a complete working implementation of hue.js and now discovery is working as expected:

mattbucci@behring5:~/hue-cli$ node hue-cli.js search
1 stations found

1: 192.168.1.205

@TwizzyDizzy
Copy link

Hi @mattbucci,

alright, since you can't physically press the register button on my bridge, I'm going to test the register functionality when I get home later (around 7PM, CEST). Should I test with the version in your home directory or your github master?

Cheers
Thomas

@mattbucci
Copy link
Contributor Author

they should both be the same now, but I'll add a console log to the one in
my home dir so let's use that one

On Fri, Sep 16, 2016 at 2:33 AM, Thomas Dalichow notifications@github.com
wrote:

Hi @mattbucci https://github.com/mattbucci,

alright, since you can't physically press the register button on my
bridge, I'm going to test the register functionality when I get home later
(around 7PM, CEST). Should I test with the version in your home directory
or your github master?

Cheers
Thomas


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#14 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ADIcs0Yq4oM1ik7EAawwcIejZwr2-laEks5qqmJPgaJpZM4J9mkv
.

@TwizzyDizzy
Copy link

TwizzyDizzy commented Sep 16, 2016

Just tested the register function. It worked! As did the light functions and everything else hue-cli shall achieve :)

(for readers, this is debugging sample code and not to be used by you. Please refer to README.md instead)

mattbucci@behring5:~$ node /home/mattbucci/hue-cli/hue-cli.js -H 192.168.1.205 register
please go and press the link button on your base station
Hue Base Station paired!
username: 4x55Sy492M3hfUCSNZKE8K-mBUwaXH-z8JvTW8-c
config file written to /home/mattbucci/.hue.json

Cheers
Thomas

@jgladch
Copy link

jgladch commented Sep 20, 2016

Can we get this merged, plz? 🎊

@mattbucci
Copy link
Contributor Author

mattbucci commented Sep 21, 2016

@jgladch @yadomi if you'd like to test this now please do the following and report back and let us know if it works. Will be much more helpful!

I'm sure he'll be much more comfortable merging if we have more people verifying it's fixed 👍

@bahamas10
Copy link
Owner

hey everyone, thanks for all the debugging on this and interest in the project overall... i'm looking at the code now and will update this thread when everything is merged and working :D

config.username = resp[0].success.username;

// writing config file
var s = JSON.stringify(config, null, 2);
Copy link
Owner

@bahamas10 bahamas10 Sep 21, 2016

Choose a reason for hiding this comment

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

I didn't realize hue-cli had as large of a community as it did (thanks everyone!) so I'm interested in gathering insight from everyone. My only concern with this is, when a user runs hue register it will clobber their existing config file without passing the --save option. Admittedly, the --save function currently operates in a less-than-ideal way.

My current proposal would be to drop the --save functionality completely, and make it so hue register will automatically write out the config IF ONE DOES NOT ALREADY EXIST. If one exists, hue register should print a warning, dump the current config, and exit non-zero, and leave it up to the user to remove it before registering a new bridge. What do you all think of that?

Either way though, I'm currently 👍 on this change and appreciate the small diff!

addendum: if you all like this idea, i wouldn't mind merging this as-is and making the code do what i described in a second commit

@TwizzyDizzy
Copy link

Hi @bahamas10

My current proposal would be to drop the --save functionality completely, and make it so hue register will automatically write out the config IF ONE DOES NOT ALREADY EXIST. If one exists, hue register should print a warning, dump the current config, and exit non-zero, and leave it up to the user to remove it before registering a new bridge. What do you all think of that?

👍 seems to be the wisest course of action!

@yadomi
Copy link

yadomi commented Sep 21, 2016

@mattbucci damn, i'm too late. I've made #15 but it seem you're faster than me :)

@mattbucci
Copy link
Contributor Author

@yadomi I reviewed your work, funny we both chose to work on it at the same time 👍, Seems like I can do a bit of code cleanup if process.exit is preferred over throw

@yadomi
Copy link

yadomi commented Sep 21, 2016

@mattbucci yes that's funny, we've made almost the same change 👍
for the throw thing, I think for a CLI tool, it's more friendly to display a nice error message than throw the raw error to the console.

btw, should I close my PR (#15) since this one do almost the same things ?

@mattbucci
Copy link
Contributor Author

@yadomi yeh that's fine. please close your PR and review mine. I'll make any changes needed (changing throw to process.exit(1)) and anything else you find and hopefully we can get this merged later today.

@yadomi
Copy link

yadomi commented Sep 21, 2016

@mattbucci this PR looks OK to me 🚀

@bahamas10 bahamas10 merged commit 43239de into bahamas10:master Sep 22, 2016
@bahamas10
Copy link
Owner

thank you everyone! I've merged this pr and have published it to npm

$ npm publish
+ hue-cli@0.2.0

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

Successfully merging this pull request may close these issues.

5 participants