KraigM / homebridge-wink

Wink hub plugin for HomeBridge
54 stars 37 forks source link

Wink Modularization #11

Closed pdlove closed 8 years ago

pdlove commented 8 years ago

I used the WinkLockAccessory as a base class of how to work around the issue of the wink-js missing most of the group types. The one downside is that it currently doesn't support that "instant" lock update when triggering a lock. I then pulled each accessory type out into it's own file for easier management. Currently it covers Lights, Locks, Garage Door and basic Binary Switches. I also listed all available device types to work as a simple checklist and reveal what still hasn't been done. With this framework, it makes it very simple to add new device types.

Installation is as simple as placing the index.js and accessories folder into an existing installation's home bridge-wink folder for testing.

AppleTechy commented 8 years ago

@pdlove For garage door will it still pick up MYQ garage doors or only GoControl? Since we already have a plugin for the MYQ it might be easier for the user if there was a way to limit what brand garage door accessories showed up. This way there is less confusion on which is running through the Wink plugin and which is actually running directly to MYQ API. Thoughts?

pdlove commented 8 years ago

@AppleTechy It will use any garage door recognized by Wink because it communicates with Wink and not directly to the garage door. Personally, I use Wink to control my MyQ because I'm too lazy to install the API. Wink is my single source for control. I do have an update I need to do to fix an issue with the garage door not updating correctly. I can see about adding a configuration option to the config.json for MyQ suppression if that is desired.

AppleTechy commented 8 years ago

@pdlove I have noticed when using Wink there is a significant delay from when you send the command to when it starts to open the garage door vs when sending it directly from the MYQ app which is almost instant. To me the MYQ app is much quicker therefore more convenient to use. You should try and use the MYQ app and wink app to compare the time difference between the garage door starting to open because for you it might be much faster or on mine something might have gotten out of sync and is causing it to be so slow. Anyways I would love to hear if you also notice a major difference in speed between using the 2. I would love to possibly see some MYQ suppression that way I don't accidentally try and send the command via the Wink with the delay when I would prefer to send it via MYQ plugin. Thanks for all your hard work to make this plugin better!

KraigM commented 8 years ago

That might be related to the same issue I have with locks. Which Im guessing has to do with the cloud api vs local api, hence #8. Its just a guess but its my best guess.

AppleTechy commented 8 years ago

@KraigM This delay is noticeable when doing the tests through their respective mobile apps. Haven't done any testing using homebridge.

pdlove commented 8 years ago

I'm going to add two configuration options to the file. One will allow you to suppress devices by type and the other by name. I experience a 5 second delay with using the wink app which is fine for me because I tap the button on my watch when I turn into my driveway and I have a very long driveway. I have known this delay to jump up to 30 seconds at times. @KraigM The biggest issue with using the local API is that you have to severe the connection to the cloud since Wink updated and closed that hole.

KraigM commented 8 years ago

There is about a 7 second delay by the wink app itself on my lock

pdlove commented 8 years ago

@KraigM I have several changes I've made that finish up the device types outlined here and adds powerstrips, smoke alarms and by tomorrow will also include thermostats the requested window sensors. Is it possible for me to update this pull request or will I have to close it and start a new one? I'm also going to get the feature I just mentioned added in over the next few days. Can I add new commits to this pull request or will I have to close it and open a new one?

AppleTechy commented 8 years ago

@pdlove @KraigM I found one site that said you didn't have to serve the connection to the cloud but you needed a specific file but the article was from little over a year ago. The issue is everything so far I have seen is to get LAN API is to reset your Hub and then root it which would be a pain for some of us to re-add all of our devices back to the hub.

AppleTechy commented 8 years ago

@pdlove Will you be able to control Ge Outlink Smart Outlets in the new revision of the plugin?

pdlove commented 8 years ago

@AppleTechy Yes. The one I'm committing later this week also sets the In Use flag based on the power consumption and returns it as an outlet instead of a light.

KraigM commented 8 years ago

@pdlove you just have to push to the same branch and it should auto include them. You can also add [#11] to the beginning of you commit messages to ensure they show as being linked to this PR

AppleTechy commented 8 years ago

@pdlove Alright, thanks for all your work on this plugin!

AppleTechy commented 8 years ago

@pdlove you should create a Slack account on our team. Make talking much easier rather than going back and forth on here.

pdlove commented 8 years ago

@AppleTechy On the LAN API, the update they did that bricked most of the hubs temporarily made the hub not easy to root. It has to be done now my soldering a serial connector on that connects to a Raspberry Pi or similar. No problem on the plugin work. I'm anxious to see it completed. I'm actually adding in a lot of the home bridge functions that I don't know that Apple even uses yet.

KraigM commented 8 years ago

@pdlove As far as code review, one initial note, why didn't you have them all inherit from a common class?

Also, why did you remove all the logging?

KraigM commented 8 years ago

@pdlove Also, why did you add name to the id. If someone changes the display name of any device, HomeKit would see it as a completely new device and they would lose all configurations.

KraigM commented 8 years ago

@pdlove You also need to be sure to include fixes done in the main repo that are missing. I think you got at least some of them, but just make sure you get them all.

pdlove commented 8 years ago

@KraigM Good point. That's something I did while testing because I was having trouble getting a unique ID at the time. I'll yank the name from it. On the fixes, I'll double-check the commits from since I forked and make sure they're there. I thought I got everything, but it never hurts to go over it.

The only thing I was still having trouble with on this one was the fix on locks that polled for the update every few seconds until it changed. I've got my issues on that resolved now and have that as part of my next commit.

pdlove commented 8 years ago

I feel really good about the stability and completeness of my latest commit. Have a look and let me know if I failed to address any concerns.

pdlove commented 8 years ago

I'm going to submit a fix for the light color soon. Don't merge this into the branch until after that please.

KraigM commented 8 years ago

Ok Im going to look through your updates now

KraigM commented 8 years ago

@pdlove Are you just working on the lightbulb atm or do you have other framework changes you are working on and/or haven't pushed? I have a thought on how to modularize it a little more that I was going to code real quick, but don't want to deal with a bunch of conflicts

pdlove commented 8 years ago

I just submitted to my fork the light fix. It's "light" on code changes.... sorry, couldn't help myself.

KraigM commented 8 years ago

/facepalm lol. Cool, Im going to have some changes shortly for you to look over.

pdlove commented 8 years ago

@KraigM I created another branch on my fork that contains changes that allow devices to dynamically add to the home bridge implementation so that you don't have to reboot it when adding devices. It will also have the ability to remove devices removed from Wink. I'm trying to also find a way to simply disable the device from HomeKit when it isn't connected. For example, if the dumb switch is off on a GE Link light. I just can't figure out yet how to flag it as disabled. I'll get there. I look forward to seeing your changes. Once we have this perfect, I'm going to take the framework over to a smart things module.

KraigM commented 8 years ago

@pdlove What do you mean by a SmartThings module?

pdlove commented 8 years ago

@KraigM You know... not wink, but smart things... the samsung equivalent. I have both. The SmartThings hubs allows quicker reaction to motion detectors and changes, but I've got enough Wink stuff I can't just switch. :) The pain of technology. It's a separate project altogether. I just want to get the platform modularization right here first because I'm not very thrilled with what I've seen on other projects because they have a single huge index.js file which is left over from how home bridge was before they spun off the plugins.

KraigM commented 8 years ago

@pdlove Oh do you mean you want to similarly modularize the Smarthings homebridge module?

KraigM commented 8 years ago

@pdlove I just did a pretty massive restructure (going along with the separation you already did).

I pushed these changes in [#11] Changed all the accessories to inherit from a newly extracted W…

KraigM commented 8 years ago

Check it out and let me know what you think, if you see any issues I missed, etc

KraigM commented 8 years ago

Also just pushed some fixes to some code warnings and a reformat of all the code.

You should try out WebStorm. They have an awesome debugger, not to mention code complete, warnings on code mistakes, navigating references, code clean, etc. It comes with a 30 day trial, but at the end of they trial you just have to restart the app every 30 mins (annoy-a-ware). We should be able to eventually get some free open source licenses.

pdlove commented 8 years ago

I'll try it out. I've been using Visual Studio Code and have been fairly impressed that Microsoft has actually done a decent job supporting node.js. I've been using some other site for "beautification". I'll take a look at your changes and provide some feedback. When I took a brief look last night while feeding the baby, it looked like you had done what I originally tried to do but at the time I had no concept of inheritance or scope in node and couldn't make it do what I thought it should.

pdlove commented 8 years ago

Looks great. I left a few comments. I'm going to update mine from your branch.

KraigM commented 8 years ago

I responded to the one inline and Im fixing the other

pdlove commented 8 years ago

K. Looking good. I'm good with your branch as having everything needed and requested. I think the only outstanding request that can be fulfilled is to add the siren accessory. Other than that, the local API concept and my "Dynamic Register/Unregister" of a device, neither of which can be completed this weekend.

KraigM commented 8 years ago

I just posted another inline response to one of your comments I missed

KraigM commented 8 years ago

@pdlove Cool. So unless you have any further comments or any issues pop up in your own testing, I'm satisfied with calling this done and I'm ready to release it as 1.0.0. I've tested it with my lock (which is all I can test) and it works like a champ. So let me know when you have tested it and when you are satisfied with closing/releasing this, and we will make it happen.

pdlove commented 8 years ago

I'll be done in about a half hour. I'd like to go ahead and add the siren before we perform the 1.0 release. I'll pulled from your branch and will update this pull request.

pdlove commented 8 years ago

I'm done. Everything works great and Sirens and AC fan speed have been added. I think everything looks great and I'm excited to get this into everyone's hands.

pdlove commented 8 years ago

@KraigM Thank you for helping my first github contributing experience be a positive one. :+1:

pdlove commented 8 years ago

@KraigM Will you update the Readme file with the proper Installation and Upgrade Instructions?

KraigM commented 8 years ago

Released under 1.0.0/1.0.1

rudders commented 8 years ago

If you get the dynamic loading stuff figured out that would be an awesome improvement to homebridge! Would be great if you could share your logic on the slack plugins channel if you get it to work. https://homebridgeteam.slack.com/messages/plugins/

pdlove commented 8 years ago

How do I get invited to there? I've got it working in test, but it requires changes that I'm not feeling good about yet. Basically, there is a function on the Server I created that allows me to add devices to the bridge. Once done, I initiate a restart of the Homebridge server. The next time the HomeKit executes any command, it then sees the new device(s). I have a fork of hap-nodejs and homekit that demonstrate the needed changes. I'm just not satisfied with what I currently have to do to get access to the Server object. If I can find a way to get that right, I'm going to start submitting pull requests.

AppleTechy commented 8 years ago

@pdlove You have to create a slack account under our team. Create an account and you should been fine. I believe it actually open to everyone. Use the link provided on the main homebridge page or here is the same link http://homebridge-slackin.herokuapp.com/

rudders commented 8 years ago

@pdlove restarting homebridge is a pretty brute force solution. My concern with this is that if some devices are offline during one of these restarts then people's HomeKit configurations could get borked if they are not discovered. I thought you'd founds the secret sauce of HomeKit;) be keen to watch your progress though.

pdlove commented 8 years ago

@rudders Not homebridge but the http server portion. The point is to make it useful to the Wemo group as well. I added a republish function to HAPServer.js that closes the http listener on HAP-nodejs and reopens it with the new device setup as well as stops/restarts the advertisement. I don't see my commit at the moment. I'll outline this better in the morning under [#14].