jcarbaugh / python-roku

Screw remotes. Control your Roku with Python.
https://pypi.python.org/pypi/roku
BSD 3-Clause "New" or "Revised" License
288 stars 94 forks source link

Add in two new methods for iterable volume control. #55

Open atlc opened 4 years ago

atlc commented 4 years ago

This allows you to jump volume incrementally by passing an integer argument to either volume_down_by or volume_up_by.

This functionality can also be extended to ChannelUp and ChannelDown by defining 'channel_up_by': 'ChannelUp' and 'channel_down_by': 'ChannelDown' in the ITERABLE_FUNCTIONS, but I don't have a TV tuner to test those commands.

jcarbaugh commented 4 years ago

@atlc thanks so much for the contribution! I'd like to find a way to reuse the existing volume_up and volume_down commands, if possible.

https://github.com/jcarbaugh/python-roku/pull/55/files#diff-2a352684b33ccb00ba030ce51c0200c7R180

Instead of ITERABLE_FUNCTIONS on the line above, I'm wondering if it could just be elif name in ("volume_up", "volume_down"): and then just parse an int of the number of times to invoke the command from args[0] if it exists. The int can default to 1 if no arg is passed, so it retains the current behavior.

What do you think?

jcarbaugh commented 4 years ago

@atlc also, I'm happy to either let you investigate or I'll merge and take a stab at it myself, whichever you prefer. I'm prepping for a 4.0 release that I want to get out tomorrow night, so we can include this feature if we get it ready by then. Thanks!

atlc commented 4 years ago

Hey @jcarbaugh, thanks for looking everything over! The reason why I decided to create a separate dict for the iterable functions is because that behavior is extensible to the channel up, channel down, left, right, down, up, and back functions and I thought that would be the most acceptable format. I am not a Python developer so I am not all that familiar with development conventions so I really appreciate the feedback.

I'll take a stab at implementing it the second way, with the functionality extended to all the directional inputs, and the channel directions!

jcarbaugh commented 4 years ago

Having that defined list instead of just being in the elif is compelling. Thinking about it more broadly, the volume up and down makes sense to repeat, but really any command could be repeated. What if call took an optional repeat parameter that would just invoke the given command multiple times? There would have to be a very short sleep between each call of the command to make sure that the previous one finished executing on the Roku.

atlc commented 4 years ago

Really, adding the logic to an extra argument for the call method does make the most sense and is the most programmatic approach. Definitely agree with you there.

I won't have any time to work on this tomorrow unfortunately, so if you want to wait until after the release I can finish up implementing that Tuesday! Otherwise, I've got the rest of the PR up to date with all the other suggestions before the suggestion of shifting the functionality to call.

PS - Thank you so much for making this awesome project, it's so nice not having to hunt for the remote and a laptop is so much harder to lose! 👍

metagrapher commented 4 years ago

Having that defined list instead of just being in the elif is compelling. Thinking about it more broadly, the volume up and down makes sense to repeat, but really any command could be repeated. What if call took an optional repeat parameter that would just invoke the given command multiple times? There would have to be a very short sleep between each call of the command to make sure that the previous one finished executing on the Roku.

I actually just wrote a simple button press function in my own app that does exactly that. The logic may be reusable here:

def button_press(self, button, x=1):  
        if self.rc==[]: self.discover() # check for a connected roku object  
        if button not in self.rc.commands: return False # if the call is not a button command, return  
        for i in range(x):  
            try:  
                getattr(self.rc, button)()  
            except:   
                # re-iterate on exception, since this is almost always a temporary connection exception  
                # any other exceptions would have been caught already in self.discovery()  
                self.button_press(button, x-i) # subtract the current iteration from x arg to keep count  

I did consider adding in an arbitrary sleep in there, which I actually have done on some other commands which I wrapped. Some commands don't seem to need a sleep, while others would go far too quickly for the screen to catch up. I've found in practice that repeated forward and reverse commands can be strung together, but if you do this with more than say 4 then the interface can get a little laggy and must be /reset/ with a roku.play() command twice in a row (pause, unpause).

All in all, "to sleep or not to sleep" may be a decision for the caller, but I'm not sure. Something about argument overload and protecting the user from themselves?

metagrapher commented 4 years ago

Hrm. Looking at the code in core.py I'm not actually sure that my code will help at all. In that case, all I can offer is my recent experience both with using a time.sleep(1) and without: (pre-apologies for any reiteration)

I've found it's useful to string together several up, down, left, or right commands, as the system is very responsive to this. As well, a literal command can be thrown in, as may be the case in automating a search, for example.

Repeated forward, reverse, or replay commands tend to not work very well, with the worst offender being the replay command. You can witness this for yourself with a normal IR remote control: the system goes bonkers.

I also found that when sending several forward commands (in order to modulate speed), sometimes the interface would get stuck on the heads up screen (the one with the play button and timeline) and the time would get all jumpy. It seemed to be an interface display issue and not a functional issue, as it didn't affect playback and was remedied by sending play() twice.

I ran into this as I was writing a method to specify a time amount to skip forward or back, based on the knowledge that commercials tend to be specific lengths of time (none of which match a 20 second skip time, btw) I found that by timing the forward and reverse speeds at various multipliers, in combination with time.sleep calls, I was able to get surprisingly accurate skip timings. If there's interest, I may make a pull request to add that functionality to core (for now it lives in my wrapper.)