Button.when_held(function_ref, seconds)

[martinohanlon]

I'm looking for some pointers... I think it would be a good to add Button.when_held which would fire a function when a button has been held down for a period of time. Its achievable by timing when_pressed and when_released but its a function I use a lot to determine between a short press and a long press. Presumably it would also be a good idea to have a wait_for _held(seconds, timeout=None)?

Providing you agree Im happy to make the change and issue a pull but could I get some pointers as to where the change should be made as Im struggling to find my path through the modules structure.

[bennuttall]

I like the idea - could be handy. I'm not sure how it would work as you can't set up multiple events for different lengths of time, you'd have to wrap it in something, and then timeout on release...?

[bennuttall]

Any ideas @waveform80? Would this be possible?

[waveform80]

Erm ... yeah, it's certainly possible - the question is how to do it reasonably safely (it'll necessarily involve a background thread to note when a time-threshold has elapsed) which then throws up interesting questions like "is the threshold configurable after construction", and "if the threshold is configurable, what happens if someone changes it while the held-thread is executing after a button press but before event firing", etc.

I'll put my thinking cap on and have a play with this but it'll be a little while yet - I want to plough through a few more 1.2 tickets first.

[bennuttall]

Ok let's leave it for now - there are plenty more to do for 1.2 (I just tagged a few more). Though as before, we can postpone a few if necessary.

[bcroston]

I might be adding this to RPi.GPIO in the next few months.

[lurch]

What if the thread itself wasn't configurable after construction, but the thread only gets created (if the user has set up a when_held callback) when the button is pressed? Would creating and destroying threads on each button press and release (only for buttons where when_held callbacks have been added) add too much overhead?

[waveform80]

It's an interesting idea but if you create lots of threads you've got to be careful about join()ing them again or you quite quickly exhaust the thread limit of the process (in other words, it's certainly do-able but one needs still needs to be a little careful).

[waveform80]

Sadly, even if it were added to RPi.GPIO natively I doubt we could take advantage of that; with pins now abstracted into their own hierarchy we'd have a choice if RPi.GPIO supports "when_held" but everyone else doesn't: add it to the pin abstraction and then implement an emulation for it in the RPIO and native pin modules (and pigpiod and potentially wiringpi in future), or just implement it ourselves at the device level (probably in DigitalInputDevice). Frankly I suspect we'll wind up going the latter route simply because it's less code.

[martinohanlon]

I needed this function, so I have created a 'HoldableButton' class which inherits from Button.

from gpiozero import Button
from threading import Timer

class HoldableButton(Button):
    def __init__(self, pin=None, pull_up=True, bounce_time=None, hold_time=1, repeat=False): 

        super(HoldableButton, self).__init__(pin, pull_up, bounce_time)

        # Set Button when_pressed and when_released to call local functions
        # cant use super() as it doesn't support setters
        Button.when_pressed.fset(self, self._when_button_pressed)
        Button.when_released.fset(self, self._when_button_released)

        self._when_held = None
        self._when_pressed = None
        self._when_released = None

        self.hold_time = hold_time
        self.repeat = repeat
        self._held_timer = None

    #override button when_pressed and when_released
    @property
    def when_pressed(self):
        return self._when_pressed

    @when_pressed.setter
    def when_pressed(self, value):
        self._when_pressed = value

    @property
    def when_released(self):
        return self._when_released

    @when_released.setter
    def when_released(self, value):
        self._when_released = value

    @property
    def when_held(self):
        return self._when_held

    @when_held.setter
    def when_held(self, value):
        self._when_held = value

    def _when_button_pressed(self):
        if self._when_pressed != None:
            self._when_pressed()

        self._start_hold()

    def _when_button_released(self):
        if self._when_released != None:
            self.when_released()

        self._stop_hold()

    def _start_hold(self):
        if self._when_held != None:
            self._held_timer = Timer(self.hold_time, self._button_held)
            self._held_timer.start()

    def _stop_hold(self):
        if self._held_timer != None:
            self._held_timer.cancel()

    def _button_held(self):
        self._when_held()
        if self.repeat == True and self.is_pressed == True:
            self._start_hold()

if __name__ == "__main__":
    from time import sleep
    def button_pressed():
        print("pressed")
    def button_released():
        print("released")
    def button_held():
        print("held")
    pin = 26
    but = HoldableButton(pin, hold_time = 0.5, repeat = False)    
    but.when_held = button_held
    but.when_pressed = button_pressed
    but.when_released = button_released
    while True:
        sleep(0.1)

This feels like a more 'appropriate' implementation, rather than complicating Button which should itself be simple. @bennuttall @waveform80 - Thoughts?

[waveform80]

Interesting stuff - this could definitely form the basis of a holdable button implementation. Only one thing's immediately jumped out at me: if the user assigns a long-running when_pressed handler (e.g. just for the sake of argument sleep(2)) then _start_hold won't get called until that returns. Ideally _start_hold and _stop_hold need to be called at the top of the internal handlers. I'll have to have a think about any race conditions a bit later (sorry - trying to battle Scribus to finish some picademy worksheets at the mo - argh!)

[martinohanlon]

Good spot @waveform80.

[lurch]

I wonder if it'd be worth also adding a self._press_time = None attribute to __init__, setting it to time.time() inside _when_button_pressed, and then modifying the callback logic inside _when_button_released to do

if self._when_released != None:
    try:
        self.when_released(time.time() - self._press_time)
    except TypeError:
        self.when_released()
    finally:
        self._press_time = None

(while leaving all the other Timer stuff in there too) and then the caller could choose to use either def button_released() or def button_released(held_for)
?

Hmmmm... although I guess my modification above is 'incompatible' with the logic of https://github.com/RPi-Distro/python-gpiozero/blob/master/gpiozero/input_devices.py#L153 - I can't quite tell how _wrap_callback (which @martinohanlon isn't using here - should he be?) is supposed to cope with extra callback args. Would it simply be a case of modifying the def button_released(held_for) signature to def button_released(held_for=None) ? Or would it need to be def button_released(which_button, held_for=None) ? (Or would both work?)

[waveform80]

@lurch - interesting query: good catch on _wrap_callback - that should be used so that handlers can either have no (mandatory positional) arguments or one (mandatory positional) argument which is set to the sending object. To handle extra params in handlers, we'd need to tweak _wrap_callback - my hunch is it's doable but I'd have to have a think about it (sorry - too much picademy stuff I need to remember right now - I'll have to come back to this).

[lurch]

I've just been thinking about the 'repeat' aspect of this and how it interacts with 'hold_time'... (similar to what @waveform80 was talking about in #115 (comment) )

If the user sets a hold_time of e.g. 5 seconds and then sets repeat to True, if they hold the button down for 16 seconds, then the when_held callback will be called three times (at 5, 10 and 15 seconds into the button-hold).
However if the user again holds the button down for 16 seconds, but after 4.5 seconds the hold_time gets changed from 5 to 10 seconds, then the when_held callback will only be called twice (at 5 and 15 seconds). Whereas if the hold_time was changed from 5 seconds to 10 seconds after 5.5 seconds, then the when_held callback will be again called twice, but this time at 5 and 10 seconds.
Is this the intended behaviour? I wonder if it might make sense for any change to hold_time to only affect the next button-hold sequence, and not the current button-hold sequence (if you see what I mean?)
I guess one way of doing that would be to modify _stop_hold to:

def _stop_hold(self):
    if self._held_timer != None:
        self._held_timer.cancel()
        self._held_timer = None

(which might be worth doing anyway?), and then modify _start_hold to:

def _start_hold(self):
    if self._when_held != None:
        if self._held_timer != None:
            hold_time = self._held_timer.interval
        else:
            hold_time = self.hold_time
        self._held_timer = Timer(hold_time, self._button_held)
        self._held_timer.start()

?

And I wonder if a similar argument could be made for the repeat attribute? I guess if each _held_timer instance held its 'own copy' of repeat (e.g. by storing it as an extra attribute on the Timer object itself) then each button-hold sequence could e.g. set its self._held_timer.repeat to False after two repeats (so that each button-hold sequence would only call the when_held callback a maximum of three times), without affecting the 'class level' value of repeat. (Again, if you see what I mean!)

Apologies if I'm over-complicating things, but it's always worth considering edge-cases.

[martinohanlon]

I think your over complicating the problem. A typical use case would be a volume up button with a very short hold time so you don't have to press a button 100 times to get from 0% to 100%, its unlikely that properties would be changed at run time.

However from my perspective it works as intended. At initiation (ie the button being pressed) the properties in place should be used, changing behaviour part way through an execution would be confusing.

[lurch]

A typical use case would be a volume up button with a very short hold time so you don't have to press a button 100 times to get from 0% to 100%

That sort of ties in with what I was saying about repeat being stored per-hold-sequence. Once the volume got to 100%, then the volume-up-hold-button timer could turn off its repeat attribute, to prevent additional volume-up callbacks from being fired (e.g. maybe you flash an LED each time the volume increases by a step). And similarly the volume-down-hold-button timer could turn off its repeat attribute once the volume got down to 0%.
But I've not given it a great deal of thought, so there's probably easier (less complicated) ways to achieve that ;-)

[martinohanlon]

I've made a couple of changes to my HoldableButton class, most notably adding a is_held property. I've put it on a gist - https://gist.github.com/martinohanlon/20cee570d6ea4ca0b7ad

[lurch]

It looks like if the _when_held callback hasn't been set, then is_held will never get set to True? Shouldn't is_held be set regardless of whether _when_held has been set or not?

[martinohanlon]

@lurch good spot... I hadnt thought of that. I'll make a change.

Edit - change made!

[lurch]

Edit - change made!

Cool! It's only a minor point, but I guess _button_held could potentially be re-ordered to:

    def _button_held(self):
        self._is_held = True
        if self._when_held is not None:
            if self.repeat and self.is_pressed:
                self._start_hold()
            self._when_held()

i.e. the repeat attribute has little effect when _when_held is None.

And your idea of a is_held property has given me an idea that my earlier suggestion could also be modified to add a pressed_time property i.e.:

    @property
    def pressed_time(self):
        if self.is_pressed:
            return time.time() - self._press_time
        else:
            return 0

[bennuttall]

@waveform80 is this one for v1.2 or v1.3?

[waveform80]

I think 1.2 - it's looking good to me I just haven't had 5 minutes spare to play/torture it myself yet. One quick question: is there any particular reason this shouldn't just be in the Button class (I can't remember if I mentioned this before). I'm also need to test @lurch's suggestions involving repetition as that's quite an important use-case (the holding volume up/down button is an excellent example that probably ought to become a recipe albeit with a PWMLED instead of actual volume).

Let me polish off these tests, get the robot-keyboard example up to scratch (that'll take me up to 5, then I've got baby-sitting duties) then I'll try and look at this.

[martinohanlon]

No reason why it couldn't go in the Button class. My initial views were that it might be worth keeping Button simple and inheriting from Button made the implementation simpler, but no strong views, from a interface perspective adding this capability to Button doesn't change its existing simple interface providing you default hold_time and repeat in the constructor.

A few of @lurch 's suggestion have been incorporated in the class I've been using in my project, see the gist.

I did ponder making hold_time and repeat private properties as it seemed an odd use case to change the hold_time and repeat after you had initialised the object, if they are kept public, I would leave the implementation as is, as the implementer can decide whether the values of held_time and repeat can be updated based on whether the button is held down.

[bennuttall]

What's the status of this? Are we waiting for a PR from @martinohanlon?

[waveform80]

I'm going to look into this tomorrow now I've gotten most of the other tickets done. As it happens I think the use-case of changing hold_time is actually going to be rather common: consider how keyboards handle repetition. There's a long initial delay, then a shorter inter-repeat delay (to avoid accidental repetition). If people want to emulate that (e.g. for holding a volume-up button) their first thought is going to be something like:

from gpiozero import Button

volume_up = Button(4, hold_time=0.5)

def volume_up_held():
    volume_up.hold_time = 0.1
    # turn the music up!

def volume_up_released():
    volume_up.hold_time = 0.5

volume_up.when_held = volume_up_held
volume_up.when_released = volume_up_released

So I want to put some rigorous testing into edge cases of that (I have a sneaking suspicion there's a race condition in there still, and I think I might be able to make a slightly simpler implementation that doesn't ghost all the properties by overriding _fire_events, but we'll see).

[waveform80]

I've had a play with this today and here's some thoughts and I'll PR an alternate implementation in a mo...

It's a rather different design to @martinohanlon's original gist and incorporates some thoughts that came up after @lurch's notes above. Martin's design is good in that it affords predictable, precise repeat intervals by spawning a new thread each time a hold-wait needs to be undertaken. Unfortunately, this means it's possible (even likely in some scenarios, which I'll get to below) to have multiple when_held handlers running simultaneously. This isn't particularly newbie friendly (bear in mind that new programmers do horrid things like using global state all over the place and not thinking about thread interactions).

Let's consider a long running when_held handler (let's say it takes a second) with a relatively short hold_time (let's say 0.5 seconds). This is what will happen with the proposed implementation:

0.0    0.5    1.0    1.5    2.0    Time
+------------------------------------->

+-------------+
|             |
|  when_held  |    Thread1
|             |
+-+-----------+
  |
  |    +-------------+
  |    |             |
  +---->  when_held  |   Thread2
       |             |
       +-+-----------+
         |
         |    +-------------+
         |    |             |
         +---->  when_held  |   Thread3
              |             |
              +-------------+

Note the handlers are nicely fired on the 0.0, 0.5, 1.0, etc. boundaries, but that each is effectively a new thread. Provided there's no global state or inter-thread interactions that's fine, but if there are, things are going to blow up in interesting and unpredictable ways (multi-threaded debugging is always horrid and not something we want to force down people's throats too quickly).

If anyone's wondering how likely this sort of scenario is, here's an example from my reams of picamera e-mail. A picamera user wanted to implement something like the burst mode on his hand-held camera so that pressing the button would snap a picture, but holding it would repeatedly capture images. Bear in mind that the camera has to be a global or shared object in this scenario, and that high quality still capture typically takes somewhere in the region of 1 second with the Pi's camera module (due to mode switching). It should be easy to see how this would lead to a crash with the model above.

So, can we come up with a better model? Well ... I've got a friendlier model, but while it's easier I hesitate to say it's actually "better". In this model we fire up a single thread when the button is pressed. This thread waits on the hold time, fires the when_held event, then loops. The advantage is that a single thread handles all the events so no interactions are possible (there is still a possibility of overlap between when_held and when_released - I could mitigate that with a bit more time). The disadvantage is no more nice precise timing - the hold-time occurs between when_held executions rather than simultaneously. In other words it looks like this:

0.0    0.5    1.0    1.5    2.0    2.5    3.0    3.5         Time
+--------------------------------------------------------------->

+-------------+      +-------------+      +-------------+
|             |      |             |      |             |
|  when_held  +------>  when_held  +------>  when_held  |  Thread1
|             |      |             |      |             |
+-------------+      +-------------+      +-------------+

+------------------------------------------->
             Button held

If the event handler is fast enough there'll be precious little difference to Martin's model but obviously longer handlers (like a camera capture) result in substantial differences (in other words, it's probably worth documenting if we go with this model). Still, it works nicely with the camera example, and with the use-case of changing the hold time while the handler's running (simple in this model as it just applies to the next waiting time after the handler's finished).

Comments?

[martinohanlon]

As long as when_held and is_held are implemented im cool.
On 7 Apr 2016 22:43, "Dave Jones" notifications@github.com wrote:

I've had a play with this today and here's some thoughts and I'll PR an
alternate implementation in a mo...

It's a rather different design to @martinohanlon
https://github.com/martinohanlon's original gist and incorporates some
thoughts that came up after @lurch https://github.com/lurch's notes
above. Martin's design is good in that it affords predictable, precise
repeat intervals by spawning a new thread each time a hold-wait needs to be
undertaken. Unfortunately, this means it's possible (even likely in some
scenarios, which I'll get to below) to have multiple when_held handlers
running simultaneously. This isn't particularly newbie friendly (bear in
mind that new programmers do horrid things like using global state all over
the place and not thinking about thread interactions).

Let's consider a long running when_held handler (let's say it takes a
second) with a relatively short hold_time (let's say 0.5 seconds). This
is what will happen with the proposed implementation:

0.0 0.5 1.0 1.5 2.0 Time
+------------------------------------->

+-------------+
| |
| when_held | Thread1
| |
+-+-----------+
|
| +-------------+
| | |
+----> when_held | Thread2
| |
+-+-----------+
|
| +-------------+
| | |
+----> when_held | Thread3
| |
+-------------+

Note the handlers are nicely fired on the 0.0, 0.5, 1.0, etc. boundaries,
but that each is effectively a new thread. Provided there's no global state
or inter-thread interactions that's fine, but if there are, things are
going to blow up in interesting and unpredictable ways (multi-threaded
debugging is always horrid and not something we want to force down people's
throats too quickly).

If anyone's wondering how likely this sort of scenario is, here's an
example from my reams of picamera e-mail. A picamera user wanted to
implement something like the burst mode on his hand-held camera so that
pressing the button would snap a picture, but holding it would repeatedly
capture images. Bear in mind that the camera has to be a global or shared
object in this scenario, and that high quality still capture typically
takes somewhere in the region of 1 second with the Pi's camera module (due
to mode switching). It should be easy to see how this would lead to a crash
with the model above.

So, can we come up with a better model? Well ... I've got a friendlier
model, but while it's easier I hesitate to say it's actually "better". In
this model we fire up a single thread when the button is pressed. This
thread waits on the hold time, fires the when_held event, then loops. The
advantage is that a single thread handles all the events so no interactions
are possible (there is still a possibility of overlap between when_held
and when_released - I could mitigate that with a bit more time). The
disadvantage is no more nice precise timing - the hold-time occurs
between when_held executions rather than simultaneously. In other words
it looks like this:

0.0 0.5 1.0 1.5 2.0 2.5 3.0 3.5 Time
+--------------------------------------------------------------->

+-------------+ +-------------+ +-------------+
| | | | | |
| when_held +------> when_held +------> when_held | Thread1
| | | | | |
+-------------+ +-------------+ +-------------+

+------------------------------------------->
Button held

If the event handler is fast enough there'll be precious little difference
to Martin's model but obviously longer handlers (like a camera capture)
result in substantial differences (in other words, it's probably worth
documenting if we go with this model). Still, it works nicely with the
camera example, and with the use-case of changing the hold time while the
handler's running (simple in this model as it just applies to the next
waiting time after the handler's finished).

Comments?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#115 (comment)

[waveform80]

Doh - I forgot is_held. I'll just bung that in and then push it.

[martinohanlon]

That made me laugh...
On 7 Apr 2016 22:47, "Dave Jones" notifications@github.com wrote:

Doh - I forgot is_held. I'll just bung that in and then push it.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#115 (comment)

[waveform80]

There we go, it's in PR #260

[waveform80]

Eurgh, and I've forgotten to properly implement hold_repeat as well ... bah. Oh well, that'll have to wait until tomorrow - I've got a horribly early start so I've gotta head off now but I'll finish it off tomorrow.

[lurch]

Any scope for adding the pressed_time idea I had? Or is that something that it'd be better for the user to implement themselves using when_pressed and when_released, rather than adding extra bloat to the API?

[waveform80]

Yeah, why not - it's simple enough and this is the "Button" class after all (not some abstract parent) so let's throw in all the functionality we want.

[waveform80]

Actually, now I've looked at - that's kinda useful in a whole series of scenarios (not just buttons) ... I might just rename it active_time and stuff it in EventsMixin (then rename it pressed_time in the Button class).

[lurch]

Cool! :-D

[lurch]

So, what about my other idea of the when_activated callback optionally being sent the time that the device was inactive for, and the when_deactivated callback optionally being sent the time that the device was active for? Or would that involve too big a modification to _wrap_callback at this stage?

[waveform80]

I think I'll pass on that one for now. It's a little too big a change for 1.2 and I've got plenty of work to do on the test suite today (got a couple of persistent failures on the Pi and the real-pin PWM tests are being extremely annoying with their segfaults). Worth re-visiting in 1.3 though.

[waveform80]

Huh... just noticed the codecov stuff doesn't seem to be working either (was just checking what coverage was missing for mixins)

[waveform80]

Okay, let's merge this to let people play with it - I'll do another PR for the tests in a bit.