haruny / adt-pulse-mqtt

ADT Pulse Bridge for Home Assistant
MIT License
10 stars 5 forks source link

Discussion: add-on codebase #32

Open digitalcraig opened 4 years ago

digitalcraig commented 4 years ago

I'm happy to work together and settle on a single codebase for the add-on as well as the standalone Docker container version. I'm not very experienced at the Github workflow, but it's supposed to lend itself to this kind of collaboration. I'm learning as I go, for example, to create branches to make my updates and utilize pull requests to merge them back into master once they full tested and functional. I have set up automated builds on Docker Hub so that when I commit changes they get built and ready to be pulled.

The only reason I keep updating my repo is because it was the original from which several others have forked from and Github maintains that lineage. I was trying to keep things from getting circular. I was under the impression that forks usually contribute their code back upstream as well as pull down updates. It's a bit more complicated because of the way the addon metadata file has to be pointed to a Github repo (though I think it can be configured to pull a pre-built image from Docker Hub instead), but I'm happy to try to figure out a solution to prevent conflicts. I just happened to have some time to devote today towards a fix. The only thing that matters to me is that a working solution can be maintained.

Truthfully, I always thought of this project as temporary workaround until either I or some else got around to writing a native Home Assistant component in Python. There is an active project by @rsnodgrass which is available through HACS which seems to have become a focal point of discussion on the Home Assistant Community Forums thread anyway. Perhaps we should consider putting all of our support behind that?

Let me know your thoughts.

haruny commented 4 years ago

Hey Craig, thanks for reaching out. I share your goal, having something working and maintainable. Currently, this JS version fits my needs and committed to maintaining it as long as possible. I saw the py version but it's not as convenient for me to drive Smartthings/Home Assistant at the same time. Maybe I should look into it further. I kind of have a unique home setup. HA is my dashboard but all device connectivity (zwave/zigbee) is driven via Smartthings.

At any rate, happy to merge everything back to your repo if it'll be any convenience to you. I have really no claims here. I just happened to improve your work on top of the original adt js library and made it known to the community. Maybe we should ask people on the Forum thread if they like to keep using this one or we should merge all efforts to the py version. Not even sure rsnodgrass needs or accepting help.

jrdean commented 4 years ago

I can't thank both of you enough. I started using HA in January. It took me until February to figure out there was a way to integrate my ADT sensors into HA. I originally used the @rsnodgrass adt project through HACS. It was not usable for me because there would be up to a 30 second delay. This made motion detectors not usable for turning on and off lights and it would completely miss doors being opened and closed. On a whim I decided to try adt-pulse-mqtt and was blown away. It never misses door sensors and motion is quick enough to turn on a light in a dark hallway when I approach it; 5 seconds max. The initial setup of adt-pulse-mqtt was a bit intimidating for me, but I figured it out in 1-2 hours.

Attached is a screen shot of my logs imported into Google Sheets showing all of the missed events from the HACS version vs this mqtt project. I see no reason to ever go back to the HACS version and I'm pleased to hear that you intend to continue to maintain it. Screen Shot 2020-03-21 at 10 08 46 PM

haruny commented 4 years ago

@jrdean Glad to hear it's working for you :) adt-pulse-mqtt is mimicking exactly like your browser would work if you had logged into portal.adtpulse.com. If the site is up and available, then at-pulse-mqtt will work. It has a max 5 second delay. Like a browser, not rocket science and it works ;) I didn't play around HACS. Saw that it's component based and it meant too much fiddling to make it work with my setup. Therefore I skipped. I find mqtt convenient enough for whatever purpose I need.

digitalcraig commented 4 years ago

Thanks for the feedback, @jrdean . I haven't tried the HACS add-on so I had no idea how well it worked. I can see that since we are essentially scraping the Pulse website and the events need to caught in real-time, putting those events into the much longer Home Assistant update loop/event system might cause delays. That doesn't mean there isn't a way for it to go faster, but I have not been able to devote time I would need to understand how the Home Assistant architecture functions and it's changing constantly.

As far as the code base, I propose we split into the two separate repos each with both of us as contributors along with anyone else interested in helping. One would be aimed at maintaining the core Node docker container and one as a stub for the add-on. The add-on would serve as a stable endpoint for anyone to add to their installation and get consistent updates. The add-on versioning would be tied to a specific tag of the container on Docker Hub so that new versions can be tested and rolled out in a controlled way.

There is some documentation here which explains how to configure the add-on to pull images rather than build them locally. We can also look at how Frenck structured the Community Add-ons repos for inspiration. I believe there is a stub for the repo with a stub for each add-on, but then each add-on has it's own repo.

When I set out to build this solution using bits and pieces of other partial projects I found, I had no idea anyone would actually want to use it except me. I'm happy to keep it going and make improvements as needed. It's a great learning opportunity and I want to contribute something back.

haruny commented 4 years ago

I like the separation idea. Would it be simpler to maintain adppulse as a node package standalone repo? I think for all other purposes with docker or hassio or SmartThings we can have separate repos that everyone contribute. This way we can tag from a certain version as you suggested.

digitalcraig commented 4 years ago

That sounds like a good way to go.. The original NPM package I started with is still there and on github with only an update to change the version of tough-cookie in 3 years. Now that I understand better how github works, it would make sense for me to properly fork that repo (instead of just giving credit in the source code), bring in all of the changes from the past 3 years, and publish it as a new package on NPM which handles the Pulse portal communication. This would give others the opportunity to utilize it for other applications such as the Homebridge plugin.

Another repo for a new module, also published on NPM, would depend on the pulse package and handle the interface to MQTT. The config options would stay the same and work exactly the same way.

My existing repo would have the Docker file that pulls everything from NPM to build and publish arm and amd64 images to Docker Hub for a functional standalone bridge. At that point, you could convert your repo to have a similar Docker file where it builds locally as well or just points to the image already on Docker Hub. The second option is the preferred way per the add-on developer documentation.

How much of the SmartThings support is needed on the MQTT publishing side (server.jsp) and how much is done by the groovy code? I know there was at least one piece which publishes to a different topic and remaps some of the status values. Could that be done with some configuration options and pull the groovy code out to it's own repo? I have no problem leaving it in, I'm thinking along the lines of making it more generic for broader support of other applications.

Anywhere along this chain, anyone can fork their own copy, make changes, and use pull requests to submit their changes into the original repo. Of course, adding maintainers would allow them to also review, comment, and merge pull requests. The pull request allows for review and discussion before merging changes.

How does this sound?

rsnodgrass commented 4 years ago

@jrdean, for the ADT Pulse HACS version, you could have also configured in YAML the ADT Pulse refresh_interval down to 5 seconds (or even lower). The HACS ADT Pulse integration defaulted the refresh interval to 30 seconds to be nice to ADT Pulse's cloud service and not DDoS the service. However, since adt-pulse-mqtt defaults to 5 seconds, I just adjusted the default down to 5 seconds as well.

For Home Assistant users, whether the HACS or MQTT version is used, the event loop is still involved in status updates (since Home Assistant has to receive the message from MQTT in order to process and store into its database). Currently, HACS uses the HA thread pool to do its updates as its http client library is not a fully asynchronous. The pyadtpulse library for interacting with ADT's Pulse service is separate from the Home Assistant implementation, so this can be used by other Python projects (gives it a stronger case of the community supporting it over time).

However, please use whichever version works best for your use case! MQTT version has plenty of other benefits, such as working with any non-Home Assistant integrations you may have.

haruny commented 4 years ago

I checked. There seems to be multiple adtpulse npm repos, but they are quite dated. Don't know if the original authors are active to accept/merge our support quickly. I suggest we give @kevinmhickey a shout out to see if he can let us contribute to his original work. Which is basis of our current work. He also submitted an npm package for it. If he's OK. Let's use that as that.

Let me propose the following structure/architecture. Please let me know about your thoughts.

Am I missing anything?

digitalcraig commented 4 years ago

I have a fork of kevinmhickey/adt-pulse and I did contact him originally about fixing the things I found that didn't work. He didn't respond (my pull request is still open) and I wasn't so sure about making an npm package of my own at the proof-of-concept stage so I bypassed that and went straight to making the changes I needed and creating a Docker container for a Hassio add-on. If you think it's necessary to try to contact him again, I'm fine with that as well.

There are really four pieces which each build on the previous:

I think structuring this way ensures there are no conflicts when updates need to be made. I do think that whatever the names and where they reside, we should try to keep a consistent location for the add-on repo. Most users are using haruny/adt-pulse-mqtt and it makes sense to convert that as I described. Each of three previous pieces should get new repos and digitalcraig/adt-pulse-mqtt deprecated.

I'm okay the SmartThings support or deprecating in future versions, however you want to proceed there. It's small accommodation to support it.

haruny commented 4 years ago

I'm ok with your plan. 4 repos. If @kevinmhickey is not responding, let's keep ours as is.

digitalcraig commented 4 years ago

If we want these repos to have their own namespace instead assigned to one of our personal accounts, it looks like we can create a Github organization which can have multiple co-owners. Teams can be formed within that organization and codeowners assigned so that pull requests get proper review. It's free for open source.

haruny commented 4 years ago

sure. we can do that.

digitalcraig commented 4 years ago

Do you have any thoughts a namespace for the repos? It's probably not a good idea to use ADT Pulse or any form of it. I doubt ADT would take notice of a single repo which is arguably descriptive of what it it does, but we shouldn't tempt fate by using their trademarks.

haruny commented 4 years ago

Good Idea not to use ADT in the name but I'd suggest it should be still descriptive for the folks who may find it via search engines.

alitaha792003 commented 1 year ago

Question. Did the ADT smarttings work now?