Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
dl9sec opened this issue Jun 25, 2019 · 1 comment
Closed

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

dl9sec opened this issue Jun 25, 2019 · 1 comment

Comments

@dl9sec
Copy link

dl9sec commented Jun 25, 2019

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
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants