search

cyberman54 / ESP32-Paxcounter

Wifi & BLE driven passenger flow metering with cheap ESP32 boards
https://cyberman54.github.io/ESP32-Paxcounter/
Other
1.73k stars 405 forks source link

Confusing definitions of `BUTTON_PULLUP` and `BUTTON_PULLDOWN` #752

Closed lenaschimmel closed 2 years ago

lenaschimmel commented 3 years ago

The way the button hardware is defined seems ambiguous to me. In main.c there is a check for BUTTON_PULLUP, while button.c checks for BUTTON_PULLDOWN:

So it seems to me that one should always define BUTTON_PULLUP or BUTTON_PULLDOWN. When defining both, or defining none, the output on startup and the actual behavior do not match, but there's no error or warning. Five of the hal/*.hheaders define BUTTON_PULLUP, none of the files define BUTTON_PULLDOWN.

I noticed that the button functionality uses the Arduino lib SimpleButton by @spacehuhn, which has:

Button, which needs an external pull up or pull down resistor ButtonPullup, which enables the integrated pull up resistor

So it would be consistent to use only the definition BUTTON_PULLUP and get rid of BUTTON_PULLDOWN.

(I can also make a PR for this, but since I'm completely new to Arduino and ESP32 development, first I'd like to hear if I'm on the right track.)

cyberman54 commented 3 years ago

Yes, that's right, we should get rid of BUTTON_PULLDOWN here. You're welcome to create a PR for this.

Generally, the SimpleButton Arduino lib should be changed by a more popular, better maintained lib. Any suggestions would be welcome.

cyberman54 commented 2 years ago

fixed by https://github.com/cyberman54/ESP32-Paxcounter/pull/884