bxparks / AceButton

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

Fetching button-state after reboot #21

Closed ImkereiWetzel closed 5 years ago

ImkereiWetzel commented 5 years ago

Hey, first I'd like to thank you for this wonderful piece of code.

In some places (doc, sourcecode) you mention the intention of not trigger on events that already occured before the button is created. What would be your proposal for getting these events, e.g. for behaviour like "if you press the Start button and switch the device on, you will get into a special configuration menu"

Currently I do it the following way:

void setup() {
...
if (!once_after_reboot)
{
    if (button.getLastButtonState()) 
    { 
       showSpecialSetup();
    }
    once_after_reboot = true;
}
...
}
bxparks commented 5 years ago

Hi, Glad you find the library useful.

I definitely understand what you are trying to do, and it's a useful feature. However, I didn't design the AceButton library for this use-case. Mostly because in my mind, an "event" is a change in the button state, and in your case, there is no change in state of the button.

Looking at your code, I have to wonder, does it actually work? Because before AceButton::check() is called for the first time, getLastButtonState() should return kButtonStateUnknown (i.e. 2). If you look at the documentation for getLastButtonState, it says: "This is a tri-state function. It may return HIGH, LOW or kButtonStateUnknown to indicate that the last state is not known." So this method does not return a boolean value, it returns an integer. Amusing to me, I wrote the next sentence to say: "This method is not for public consumption, it is exposed only for testing purposes. Consider it to be a private method." So you shouldn't be using it. :-)

Currently, the library does not have a method that returns "what is the current state of the button". But it is easy to get information by calling digitalRead() on the button pin directly. Can you use this instead?

ImkereiWetzel commented 5 years ago

Hey, you are indeed right. It only works if I check via "check()" beforehand. Thats in the omitted code, I thought it gets clearer without it, to make myself clear.

Sure, I could also do it by pulling the pin-state in, but in my programming courses we always had the so called lamda transition, which could also fire just like so :-) and thats something maybe not only me would like to "enable" with some flags...

bxparks commented 5 years ago

Right, lambda transition, I agree that's conceptually cleaner. But keep in mind that it costs CPU time and memory to support that. Some people want to use AceButton for lots of buttons (like 50 or 100 buttons). I think that means those lambda transitions would cause at least num(buttons) events to fire as soon as the first check() method is called. You could add a flag to suppress those. But that means that the check() method is paying a runtime cost of checking for that flag on every call, even though these events would only fire at the start of the program. There's also the cost added code complexity and unit testing. The code for implementing this feature adds to program memory consumption, which starts to add up when we have only 30kB (Arduino Micro) or 32kB (Nano). If calling digitalRead() at setup() is a simple and reasonable solution, then I'm tempted to leave it at that.

bxparks commented 5 years ago

So, debating myself, there are 2 things I don't like about asking people to call digitalRead() directly:

  1. The binding of the button to its pin number is duplicated, once in AceButton::init(), and once again in calling digitalRead() in the setup(), and
  2. The logic for interpreting HIGH or LOW values as "pressed" or "released" is determined by the wiring of the button, is embedded in the AceButton class, and shouldn't need to be duplicated in the code surrounding the digitalRead() method.

I have a proposal: How about I add a AceButton::isReleasedRaw() method that reads the button directly (using digitalRead()) and returns true or false whether the button is released. This is the analog to the existing AceButton::isReleased(uint8_t buttonState). The implementation of isReleasedRaw() would be simple and inlined:

bool isReleasedRaw() {
  return isReleased(mButtonConfig->readButton(mPin));
}

The recommended usage of this method would be exactly your use-case, to read the state of the button just after startup of the device.

bxparks commented 5 years ago

I would also be agreeable to naming this method isPressedRaw() or isPressedDirect(), since that's probably the more common use-case. (I don't remember why I named the other one isReleased(uint8_t). Maybe because there was already an internal method named isPressed().)

ImkereiWetzel commented 5 years ago
    Regardless of the naming that would be a clean solution

BrM

    Outlook für iOS beziehen

On Wed, Feb 27, 2019 at 2:09 AM +0100, "Brian Park" notifications@github.com wrote:

I would also be agreeable to naming this method isPressedRaw() or isPressedDirect(), since that's probably the more common use-case. (I don't remember why I named the other one isReleased(uint8_t). Maybe because there was already an internal method named isPressed().)

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

bxparks commented 5 years ago

I implemented an AceButton::isPressedRaw() method on the develop branch. Do you want to take a look?

ImkereiWetzel commented 5 years ago

Works perfect. Thank you.