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

Hubitat smarthings merge #70

Open
wants to merge 63 commits into
base: master
Choose a base branch
from

Conversation

jp0550
Copy link

@jp0550 jp0550 commented Aug 14, 2018

Works between Hubitat and Smartthings with just an import change needed at the top of the piston source file.

  • Adds supports for local fuel streams for both Hubitat (enabled by default) and Smartthings (disabled by default). A container app is made for each fuel stream with a size limit of 95KB. This should eliminate reliance on the custom fuel stream servers and allow users to manage their fuel streams themselves. Import functionality can be added to pull data in from the old servers later if needed.

  • Add option for custom endpoint for different dashboard urls.

  • Add option for disabling piston logging in location events. (Significantly slows down Hubitat)

  • Enables command overrides/overloads. "push" in Hubitat has two methods "push()" with no button number and "push(number)" with a button number. This is also used to fix the "flash" virtual command since there is also a native "flash" command in Hubitat that conflicts with it.

If this is merged the fuel stream PR can be ignored as this supports both local and external fuel streams.

ajayjohn and others added 30 commits March 25, 2018 15:11
Remove hubUID restrictions and fix saveState error
Also, disable piston execution logging ( slowing down events page ) and
change local requests to use http methods instead of sendHubCommand for
faster processing.
@idpaterson
Copy link
Collaborator

Amazing work, thank you for moving forward with the merge. I will set this up for minions to test both sides of the integration (a few of us have Hubitat) and hopefully we'll be able to push it through in the next release within a few weeks.

@idpaterson
Copy link
Collaborator

It looks like there will be a small bug fix release before this goes into testing. You're on deck for testing once that small release is deployed.

@jp0550
Copy link
Author

jp0550 commented Aug 22, 2018

Sounds good! There probably be another commit to this to iron out some issues I found, and to handle a fw change from Hubitat.

@idpaterson
Copy link
Collaborator

I've had good results with this pattern for the HubAction and Protocol classes. May not be the most idiomatic way to accomplish it but is this something you can run with?

private static Class HubActionClass() {
    try {
        return 'physicalgraph.device.HubAction' as Class
    } catch(all) {
        return 'hubitat.device.HubAction' as Class
    }
}
sendHubCommand(HubActionClass().newInstance(requestParams, null, [callback: localHttpRequestHandler]))

Since it is used everywhere to distinguish between ST and HE I would also love to see an abstraction of the hubUID test. Is there a good way to use an alternate self-documenting name like isHubitat?

@jp0550
Copy link
Author

jp0550 commented Aug 26, 2018

Sure thing, I'll pull it in and give it a try next chance I get, along with changing hubUID references.

@jp0550
Copy link
Author

jp0550 commented Aug 26, 2018

This one should be ready to go. There's a lot of performance improvements here for Hubitat and for the first time I was seeing timings lower than SmartThings. Looks like the biggest slowdown in that system was the state size so I've reduced it wherever possible mostly with the stats and logs limits which are now limited to 10 and 50 with a setting that can be changed per piston if the user wants more. Also I removed atomic state loading for both systems after getRuntimeData if the piston didn't have to wait at a semaphore which helped as well.

If you get a chance, let me know what you think and if you see any changes that should be made.

@idpaterson
Copy link
Collaborator

Excellent! Should I include a warning when importing pistons to a Hubitat instance, something along the lines of "Unlike the computing cloud that runs pistons on SmartThings, your Hubitat Elevation hub runs all of your pistons. Please import and test in small batches to avoid overloading your hub." Perhaps if they are importing more than say 20 pistons? Not the best wording since the running pistons are what will overload the hub rather than the import, but if you have any other performance guidelines I think the import feature will be an important place for this reminder.

@jp0550
Copy link
Author

jp0550 commented Aug 27, 2018

I think that's a great idea as I can see users coming to Hubitat using this tool to pull in their ST pistons. I'll get together a list of performance guidelines and send them to you.

@jp0550
Copy link
Author

jp0550 commented Aug 29, 2018

Biggest things in Hubitat as far as performance are below:

  1. After creating a piston go into the Hubitat apps section, click the newly created piston, and hit done. Child apps in Hubitat can't (yet) create the settings by themselves so it can cache devices. You'll get a warning in the logs until you do so, and the piston will run around 150ms slower as it grabs devices from the parent on each execution.

  2. Having multiple pistons reference the same event creates slowdowns as two pistons have to spin up to handle that event. It's better to put multiple statements referencing the event into one piston for situations where you have an event like motion triggering multiple actions.

  3. Write pistons keeping performance in mind following some of the guidelines from this thread: https://community.webcore.co/t/conditions-and-triggers-the-difference/164

I have some changes coming soon to fix timeToday in Hubitat. You can't create a time variable right now with something like '04:00PM'. It will just return the current time.

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.

3 participants