bxparks / AceButton

An adjustable, compact, event-driven button library for Arduino that debounces and dispatches events to a user-defined event handler.
MIT License
385 stars 37 forks source link

Public method to supress long kEventLongPressed #67

Closed BAXTER001 closed 3 years ago

BAXTER001 commented 3 years ago

Wonderful Library! I'm using AceButton to detect button presses on a rotary encoder that also has press functionality, because of this I consider rotations of the encoder to cancel the fact the button is counting up to being 'long pressed' exposing a public:

void AceButton::cancelLongPress() {
  clearPressed();
}

Would seem to be a reasonable modification unless you can see some horrible pitfall or better way of expressing it?

bxparks commented 3 years ago

Hi, I'm having trouble following, probably because I've never used a rotary encoder. I don't really know how it works, and what it is usually used for. Can you provide more details, particularly, how the thing is wired to the pins?

BAXTER001 commented 3 years ago

It's a component that can be either twisted to give 'I've rotated one click in this direction' or pressed like a standard button:

image

Only the normally open push button pins are tied into AceButton, the rest is dealt with by another library (https://github.com/PaulStoffregen/Encoder)

So for my use case I could add a lot of functions to a single encoder for example in a video player:

Clockwise rotation = Skip backwards.
Anti-clockwise rotation = Skip forwards.
Clockwise rotation when pressed = Volume Up.
Anti-clockwise rotation when pressed = Volume Down.
Click = Pause.
Long press = Stop and exit.

The trouble arises when I'm using holding it down and twisting it clockwise for the 'Volume Up' command, the library doesn't know anything about the rotation and to it it's just a button being held counting up to the long press timeout, but I want to cancel the long press countdown as soon as I detect rotation in either direction, currently AceButton doesn't expose anything I can see to say 'Okay cancel the current long press countdown I don't need the long press event.'

bxparks commented 3 years ago

Got it, the https://github.com/PaulStoffregen/Encoder link was helpful, particularly the information about quadrature encoding. I see 3 pins in the photo. I assume 2 of the pins are for the rotary encoding, and one of the pins is the push-button, so there must be a 4th pin, the ground pin, hidden behind in the photo?

Second question: Do these rotary switch have mechanical notching, so that each click increments or decrements the internal counter? Or are do they turn smoothly? It also seems to me that the Encoder.read() returns the value of the internal counter. So if you didn't poll the read() method fast enough, and you turned the knob really quickly, you could potentially skip numbers, say from 100 to 110 on successive polling of read(). Is that right? Because the interrupt will cause the Encoder to handle the signal and increment the internal counter.

So putting it all together, you want the LongPress event to fire only when the knob is not being rotated. Or, the other way you phrased it, you want the LongPress event to be disabled while the knob is being rotated. But because the Encoder library is handling the interrupts for you internally, the only way for you to know that you are turning the knob is by detecting the fact that the value returned by Encoder.read() has changed between 2 successive reads.

And finally, if you push down and rotate to change the volume, then continue to hold down the button, it seems like you do not want the LongPress event to fire, yes? I think you want to push, rotate to change the volume, then release the knob, then push again for a LongPress.

Let me think about this for a bit...

bxparks commented 3 years ago

Oh, another question: Why does the rotary encoder library ignore debouncing? If the pins are attached to interrupts, wouldn't there be spurious increment/decrement counts due to the bouncing of the contacts? Or is it because rotary encoders use optical detectors (maybe magnetic detectors?), instead of mechanical switches?

BAXTER001 commented 3 years ago

I don't think we need to focus too closely on the idea of encoders, it's simply the fact that I wish to cancel an in-progress long-press timeout, the specifics of what the other event is it could be pretty much anything.

Looks like the developer simply didn't implement denouncing, but generally they are mechanical contacts and there are forks that do including internal debouncing.

bxparks commented 3 years ago

It was important for me to understand how the rotary encoder actually worked, and how the encoder library is to be used, to determine if the proposed solution would work. Because I want to avoid adding a AceButton::cancelLongPress() method to my library. There is currently no other method that does something similar. It creates a new code path that I would have to write tests for. It makes refactoring my code in the future more difficult, without breaking backwards compatibility. If there is a way to accomplish what you want without changing either library, I think that's the solution we should consider first.

I think you can solve your problem by creating a thin wrapper or "controller" object that manages the interacting states and events between these two types of buttons:

1) Let AceButton fire off a LongPress event into the controller object, regardless whether you are rotating the knob. 2) Wire up the Encoder so that the rotation values are sent into the controller object. I don't know exactly where that happens, but I assume that you are doing an Encoder.read() operation in the loop() function. 3) Inside the controller object, keep track of whether or not the knob was turned within the last 1-2 seconds (configurable). 4) When the controller object gets the LongPress event from AceButton, check to see if the knob was turned within the last 1-2 seconds. (a) If the knob was turned, then eat the LongPress event and do nothing. (b) If the knob was not turned, pass along the LongPress event to implement your "stop and exit" function.

The basic idea here is that you keep AceButton and the Encoder library isolated and independent, so that they don't pollute the internal workings of each other. This makes each library easier to test and validate that they are working properly. Then your controller object combines the states and events of the two libraries in a way that makes sense for your particular application. Does this help?

bxparks commented 3 years ago

It occurs to me that you probably want your controller to handle things slightly differently whether your knob is being turned while pressed, versus whether it's being turned while not pressed, and whether that affects the handling of the LongPressed event, or how much delay you need to use. You may need to use the Pressed, Released and LongReleased events from AceButton. If you are familiar with Finite State Machines, writing out the FSM for your combo rotary/push button may be helpful.

BAXTER001 commented 3 years ago

Yeah an fsm transitioning based on my knob events and the Released event is probably the best option If I didn't want to patch my local copy of AceButton. Thanks for your consideration.

bxparks commented 3 years ago

Good luck! For what it's worth, I think this is the right approach. You might have been able to hack something into AceButton to get something working. But it's also might lead you into debugging the internal states of AceButton, coupled with whatever events are triggered by the Encoder library. Worse case, things work 95% of the time, but in 5% of time, something doesn't quite work with some strange combination of button presses or knob twists, and you pull your hair out trying to debug the combined system. With the FSM layered on top of both libraries, you can test the FSM in complete isolation. And if you leave AceButton untouched, you can be sure that it is 100% tested. I think I tested every possible code path and event coming out of that library. Once you modify it, you lose that guarantee.

BAXTER001 commented 3 years ago

That makes sense, keep the extensibility on my side of the divide, thanks again for your time.