JChristensen / JC_Button

Arduino library to debounce button switches, detect presses, releases, and long presses
GNU General Public License v3.0
425 stars 102 forks source link

UpDown.ino: Non-working usage of min()/max()... #22

Closed dl9sec closed 5 years ago

dl9sec commented 5 years ago

Hi Jack,

thank you very much for your efforts programming this library.

Maybe you recognized, that i modified your lib for an ESP-32 and its capacitive touch buttons (see https://github.com/dl9sec/JC_CapButton). While testing the lib I came across a problem with the UpDown example, which did not work neither with tactile buttons, nor with my capacitive touch buttons (the counter always shows 0). First I suspected the ESP-32 architecture, but it was much easier.

Inside the state machine you use the following code:

case INCR:                              // increment the counter
    count = min(count++, MAX_COUNT);    // but not more than the specified maximum
    STATE = WAIT;
    break;

case DECR:                              // decrement the counter
    count = max(count--, MIN_COUNT);    // but not less than the specified minimum
    STATE = WAIT;
    break;

The reason is the usage of min() and max() where you do the increment/decrement math inside the function call. The Arduino reference explicitly warns about that usage (see https://www.arduino.cc/reference/en/language/functions/math/min/). And the Arduino guys are right... ;-D

Changing the code to

case INCR:                              // increment the counter
    count++;
    count = min(count, MAX_COUNT);    // but not more than the specified maximum
    STATE = WAIT;
    break;

case DECR:                              // decrement the counter
    count--;
    count = max(count, MIN_COUNT);    // but not less than the specified minimum
    STATE = WAIT;
    break;

makes the example work like a charm (also with my capacitive touch buttons).

The good news: I tested your library succesfully on an ESP-32, so you could extend the "architectures" in "library.properties" "with "esp32" (and i am sure on esp8266 too) :-)

Regards, Thorsten

JChristensen commented 5 years ago

@dl9sec Hello Thorsten, thanks very much for catching this!