kevinkk525 / pysmartnode

Micropython Smarthome framework
MIT License
116 stars 22 forks source link

Pushbutton long press function triggers on first normal button press #22

Closed SpotlightKid closed 4 years ago

SpotlightKid commented 4 years ago

I believe there is a small logic error in the abutton.Pushbutton.buttoncheck coro:

ticks_diff(ticks_ms, ticks_change) returns an erroneous result on the first button press, because t_change has not been changed from None to a ticks value. This causes the long press function to be triggered directly, when the button is pressed for the first time (i.e. before the release and even before the long press timeout has passed).

The solution is, I believe to set t_change before the while loop is entered to a sufficiently high number, e.g.

t_change = time.ticks_ms() + time.ticks_add(0, -1)

See the example for http://docs.micropython.org/en/latest/library/utime.html#utime.ticks_add

# Find out TICKS_MAX used by this port
print(ticks_add(0, -1))
kevinkk525 commented 4 years ago

Interesting. In my tests (when I implemented it) I did not find any bugs and even though I am using the library with long press actions (which typically reset my devices), it didn't throw any errors or unintended activations either. However, I would expect time.ticks_diff(0,None) to throw an exception, which it doesn't, it somehow calculates with None.. So that's a bit weird.

Were you able to reproduce the problem or is it "just" a code analysis?

Your proposed solution could work around the issue but it wouldn't need t_change to be TICKS_MAX, it would be enough to set t_change to the current time.ticks_ms() because the difference from the current time has to be bigger than self.long_press_ms for it to trigger.

SpotlightKid commented 4 years ago

Turns out I was using the the Pushbutton class wrongly. I didn't realize that __init__ already schedules a buttoncheck task, so I scheduled it again, which triggered the observed behaviour:

import uasyncio as aio

from pyb import Pin
from abutton import Pushbutton

def cb():
    print("Long press triggered.")

async def button_coro(pin):
    print('Button coro started.')
    btn = Pushbutton(Pin(pin, Pin.IN, Pin.PULL_UP), suppress=True)
    btn.long_func(cb)

    print("Entering button coro loop.")
    await btn.buttoncheck()
    print('Display coro done')

async def main():
    print("Starting button coro...")
    button_task = aio.create_task(button_coro('A6'))

    print("Entering wait loop.")
    while True:
        await aio.sleep(1)

aio.run(main())

The correct way would presumably be just:

import uasyncio as aio

from pyb import Pin
from abutton import Pushbutton

def cb():
    print("Long press triggered.")

async def main():
    btn = Pushbutton(Pin('A6', Pin.IN, Pin.PULL_UP), suppress=True)
    btn.long_func(cb)

    print("Entering wait loop.")
    while True:
        await aio.sleep(1)

aio.run(main())

I find it a bit odd, that the buttoncheck task is scheduled before you have a chance to attach the callback(s). I'll probably remove that in the copy of abutton.py in my project and start the coro manually.

Anyway, seems this can be closed as invalid, though it might still be prudent to set t_change correctly at the start of the loop.

kevinkk525 commented 4 years ago

This library was built to be a drop-in replacement for Peter Hinch's aswitch library: https://github.com/peterhinch/micropython-async/blob/master/aswitch.py Therefore his documentation about the usage is valid for my library. (The goal was to reduce coroutine usage and work around a cancellation bug in the old uasyncio v2 version. Now that is not important anymore with v3)

I find it a bit odd, that the buttoncheck task is scheduled before you have a chance to attach the callback(s). I'll probably remove that in the copy of abutton.py in my project and start the coro manually.

Where's the problem with that? The task isn't executed until you yield to the scheduler, so until then you can safely modify the callbacks without any button checks being executed.

I might still give it some thoughts to check if there is a possibility of running into an error if t_change is None.

SpotlightKid commented 4 years ago

Where's the problem with that?

Well, it's imaginable (though probably unusual) that the scheduler is already running, when Pushbutton is instantiated, isn't it?

Apart from that, it's just a matter of preference / object semantics, I guess. I think it's unexpected for an object initialization call to do that kind of thing, i.e. potentially trigger some ongoing task. But I realize that this is done elsewhere too, e.g. blah = SomeService(host, port) triggering a connection.

kevinkk525 commented 4 years ago

Yes the scheduler might already be running, which is mostly the case in my applications. But the task still doesn't get executed until the current task yields (calls await asyncio.sleep(x)) so you would still be safe to change everything in your current coroutine/task.

Hmm I guess it does depend on preferences and application design.