search

apwelsh / hubitat

MIT License
26 stars 12 forks source link

Made some Changes #6

Closed JonoPorter closed 4 years ago

JonoPorter commented 4 years ago

I've made some changes to fix a few things I found annoying or not working.

here are my changes: https://github.com/JonoPorter/Roku-Tv

apwelsh commented 4 years ago

I have not looked at this device drive in a while. I will see what you did. Thanks.

Armand --Sent from iPhone

On Sep 14, 2019, at 3:07 PM, Jonathan Porter notifications@github.com wrote:

 I've made some changes to fix a few things I found annoying or not working.

here are my changes: https://github.com/JonoPorter/Roku-Tv

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

apwelsh commented 4 years ago

I have taken some time to look into the asynchttpGet and the sendHubCommand method I am using presently. They are actually both async. sendHubCommand follows the standard driver interface of sending all the various responses to a single parse() method, whereas the asynchttpGet requires a parser per call (or at least to pass a parse to the call). I am not sure the performance is all the different. I am curious if you can elaborate on how using async was faster.

Also, Iooking at your fork, I see that you removed the Music Player capability. Understand that what I was working on with my driver is Alexa integration. Music Player defines the standard commands that I implemented on the TV that the TV capability does not. It also open the door for a future ability for alexa to pass a streaming content URL to the device to kick off any Roku channel for playing said content. -- something to consider when modifying the drivers.

That said, I like your changes, they are very clean, and well thought out. Good work!

JonoPorter commented 4 years ago

Mainly I was frustrated when I had my whole house shut down command it would take over 30 seconds before it would start turning off other devices. And that issue went away when I removed the Rokus from the command. So I started looking to see if there was anything wrong with the code. When I switch over the calls to use the async http request this issue went away, as it was near instant afterwards. I think its less to do with blocking vs async calls and more to do that maybe hubitat's handling of the sendHubCommand may have issues. That is why I commented out the wake up call, I plan to look into that one some more. So through experimentation I would say its faster. I also prefer async calls for my own work.

as for the removing Music player I was playing with the capabilities to see if alexa would recognize the rokus as something that would handle the play/pause wording. I did not succeed and I forgot to revert my changes. This would be in the woops category. I'll have to look into it again and clean it up.

apwelsh commented 4 years ago

Very cool. I will check in with the deva to see how the command bus works. It is likely that the HE’s command BUS is synchronized so that although the calls are async, the responses are processed synchronously. This would explain some lagging I have observed in the past. Maybe I will add the asynhttpGet technique, and leave the parse. I want to try and ensure that I can receive messages that I did not explicitly ask for, so that HE can continue to deliver messages to the device should they have a need.

Regarding Alexa integration, the problem there is that HE only exports sensors, dimmers, color bulbs and switches. It used to be only lights, and now they added sensors. I have asked to support the TV type but it just isn’t something they are looking into just yet.

Based on this discussion, I am going to see how much of your changes I can integrate. I would really like to meet as many of the use cases as possible, and you additional child switches are not a bad idea based on the way you implemented it. In fact, I think I will expand on this further to allow selecting any button to add as a child device. I also learned I need a way to remove installed child devices without having to navigate to them, so I will work on that too.

I would like to merge these together into a single driver again if we can come to a consensus on final features.

By the way, I will be remove the MAC address detection in favor of the built-in command that gets the Mac by IP, so that it can be a smoother operation. Keep an eye out for my changes and let me know what you think.
Also, and management app is in the works (as soon as I finish my Advanced Hue Integration app which will bring hue scenes to HE).

JonoPorter commented 4 years ago

Getting rid of the MAC input and just having IP would be sweet. That was also on my list to look into but its a long list.

apwelsh commented 4 years ago

The Mac is computed so you don’t need to enter it. It is needed for wake on lan. There is a function that I just learned about that will get it from the IP address. Also, I will be removing both, when I move the code to an App that finds the TV devices automatically.

apwelsh commented 4 years ago

Was there a certain reason you change turning-on and turning-off to on/off with undo events? I am merging some of the code changes, and curious if there was an issue with turning-on/turning-off states that I am not aware of.

JonoPorter commented 4 years ago

The reason was the Alexa was waiting for those events to turn to on or off. since they often never left the turning-on/turning-off the alexa would claim the roku is not responding. So I made it just be in off or on states, but with the state being updated after the async call.

apwelsh commented 4 years ago

Thanks. I think you addressed this in the community too. Anyways, I switched over to async and see it is definitely getting a response faster now. And that’s makes sense. I did not even consider that there are a lot more refresh events with the single parser. Thanks for the tip. When I was creating this driver, the async methods were not out yet, and the performance was not an issue. Also the turning-on and turning-off states were a norm in Smartthings, so I carried it over. But your findings make sense. Alexa only recognizes on/off, and hubitat only recognizes on/off, so I will change it to on/off too, but I won't change the state until the sync callback is received to avoid flip-flip triggering of rules.

apwelsh commented 4 years ago

I have implemented the breadth of your changes in my driver, though my implementation differs. I kept the auto manage features, and kept the HDMI port management. I allow adding any control button as a child device. The reason I had not in the past, was that I figured the complexity to use them through Alexa or Google home is too great to justify having them whereas you can call the keyPress(key) custom command from Rule Machine rules, and figured this would be the primary automation point. Now the user can choose. This extends the ability to create a remote control panel in SharpTools too, since each button is a momentary switch in this configuration.