falk0069 / hue-upnp

Philips Hue UPNP and HTTP emulator (works with the Android Hue app and Logitech Harmony Home Remotes)
30 stars 10 forks source link

Updates to work with Amazon Echo, and removed a lot of globals. #5

Closed jimboca closed 8 years ago

jimboca commented 8 years ago

Andy,

This has changes to resolve #2 along with a lot of changes to make the code easier to call for another python program.

This will not be my final version, but I wanted to show you the changes now to see what you think. I have verified this works with my Harmony Hub, Amazon Echo (Alexa), and the Hue Android App. I did not yet very the execution of the current hue-upnp-helper script actually runs, but it will be simple to fix if there is an issue, but the script will change since it passes the device name instead of number, but I can change to pass the number if you prefer.

Hopefully the many changes don't freak you out :)

Let me know what you think, and I'll try to get a closer to final version in the next couple days, but it may not be until the weekend.

falk0069 commented 8 years ago

Jim,

I spent a couple hours today reviewing the code and testing it out. I think I understand almost everything you did and it looks great. I did run into a few errors while testing it that I hope we can correct. Before trying to fix them myself, I want to see what you think first. I also have a few requests / questions I'd like to make.

1) What is ISY? Is this a different home automation device?

2) For the 'failed' responses can we set it as 'error' instead of 'failed'? Here is a link to the Hue API documentation: http://www.developers.meethue.com/documentation/error-messages (you might need to register to read)

3) For all http responses can we use '\r\n' (i.e. CRLF) for newlines? This is the recommended http line terminator: http://www.w3.org/Protocols/rfc2616/rfc2616-sec19.html

4) I attempted to make the hue-upnp-helper.sh script work by setting this: DEVICES = [ hue_upnp_helper_handler('PC WOL'), hue_upnp_helper_handler('Wemo Outlet'), hue_upnp_helper_handler('Wemo Light'), ]

but hue_upnp_helper_handler() defines this (3 args): set(self,cmd,value)

So, I see this error when trying to turn something on/off: Exception happened during processing of request from ('192.168.1.206', 33284) ... dst = DEVICES[deviceNum].set(parsedContent) TypeError: set() takes exactly 3 arguments (2 given)

We probably could make hue_upnp_helper_handler() more like isy_rest_handler() to fix, but I wanted to make sure that is what you think as well. Or maybe we could make just one function and pass in a 'type' (script vs. isy).

5) Lastly, I'd like it if we could move all configurable items to the top of the script for easy location (or maybe eventually an optional config file?). For now could we move ISY_IP, ISY_USERNAME, ISY_PASSWORD, DEVICES to the top?

Let me know what you think.

Thanks

--Andy

jimboca commented 8 years ago

Andy,

Sorry for the delayed reply, I've got to many projects going on!

1) The ISY is the best home automation controller IMO https://www.universal-devices.com/residential/isy994izw-series/

2) Done

3) It looks like that is what is done, unless I missed some? Most responses use the send_json method which is passed just newlines and converts all newlines to '\r\n'.

4) I am working on this. I think the best way would be to create a default super class that handles parsing the data and calling the proper methods which the other classes can override. Let me know if you agree.

5) Yes, I'm thinking there should be a separate config file. I was thinking to do that originally but never did.

i will try to get this all finished today.

Thanks, Jim

jimboca commented 8 years ago

The requested changes are pushed. Let me know what you think. I wanted to move the DEVICES settings to the config file as well, but I was not able to do that since it references methods defined in hueUpnp.py.

falk0069 commented 8 years ago

Hi Jim,

I finally had a chance to run through your new code make updates I still needed. The json parser is awesome. This also makes it easy to add as many devices as we want. So just a few notes on the additional updates I did: 1) I moved everything to the config file that I thought might be useful 2) I had to modify the external program calls since I always got 'Error'. Turns out the 'Popen' does not return the exit code. You need to call .communicate and then .returncode. Also, the exit code is zero on success, so I had to return the opposite. 3) I also how troubles turn ON devices since it was only sending the BRI command. So, I updated to code to send both the ON and BRI commands if BRI != 0. 4) I added in a beginning default state for ON and BRI in the config, since I've have other users request this.

I think that was about it.

By the way, I just got an Echo and Alexa successful found all the devices.

Thanks for your help.

--Andy

jimboca commented 8 years ago

Hi Andy,

Yes, I saw that merge was done, thanks for going thru it all, I know it was a lot of changes so I really appreciate you pulling it back in. Also great to hear you got an Echo and it worked out for you! I will update to your version in my system and make sure it all still works. One thing I still need to fix is that I would like it to figure out the IP address for the LOCATION, I already have code to do that, so I'll try to port it to hue-upnp when I have time.

On Sat, Dec 5, 2015 at 9:35 AM falk0069 notifications@github.com wrote:

Hi Jim,

I finally had a chance to run through your new code make updates I still needed. The json parser is awesome. This also makes it easy to add as many devices as we want. So just a few notes on the additional updates I did: 1) I moved everything to the config file that I thought might be useful 2) I had to modify the external program calls since I always got 'Error'. Turns out the 'Popen' does not return the exit code. You need to call .communicate and then .returncode. Also, the exit code is zero on success, so I had to return the opposite. 3) I also how troubles turn ON devices since it was only sending the BRI command. So, I updated to code to send both the ON and BRI commands if BRI != 0. 4) I added in a beginning default state for ON and BRI in the config, since I've have other users request this.

I think that was about it.

By the way, I just got an Echo and Alexa successful found all the devices.

Thanks for your help.

--Andy

— Reply to this email directly or view it on GitHub https://github.com/falk0069/hue-upnp/pull/5#issuecomment-162226914.

jimboca commented 8 years ago

I'm finally back from vacation and looking at this. I think I need to change how the config works a little so I can modify the config from the calling program. I'm going to move the 'import hueUpnp_config' inside the main section and pass the config as an argument to run. Does that sound OK?

falk0069 commented 8 years ago

Passing the config as an argument sounds like a good option. It would be nice if we could default it to the original config if the user omit it, though. -Andy On Jan 3, 2016 1:02 PM, "jimboca" notifications@github.com wrote:

I'm finally back from vacation and looking at this. I think I need to change how the config works a little so I can modify the config from the calling program. I'm going to move the 'import hueUpnp_config' inside the main section and pass the config as an argument to run. Does that sound OK?

— Reply to this email directly or view it on GitHub https://github.com/falk0069/hue-upnp/pull/5#issuecomment-168531067.

jimboca commented 8 years ago

I've got it working, although it does not supply the default, but the caller just needs to import and pass in the global. I will send a merge request.