-
Notifications
You must be signed in to change notification settings - Fork 638
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
Button #defs are +1 of the corresponding log numbering #1972
Comments
But every other user facing indexing is 0-based? MQTT topic, Web, Terminal commands etc. Only place that uses 1-based is headers, for example BUTTON1_RELAY=1 depends that we use RELAY1_PIN=something. I swear there was a commit or changelog note somewhere referencing why it is like that... |
As this isn't a real problem and modifying this logic is obviously unacceptable, IMO the solution could be add appropriate notes in the wiki and near the defines, so one reading BUTTON1 or RELAY1 knows they actually are represented as button #0 and relay #0 and in the code they are button[0] and relay[0]. That's really only 2 cents or less :) |
To ease newcomers entry, another small code readability improvement could be use GPIOx defines (instead of raw numbers) wherever a PIN is defined, mainly in hardware.h , general.h , light.h |
Yeah, that is true, thank you for pointing that out. It is implied, but never explicitly spelled out that relay #id and preprocessor values are one number off from each other. I am not sure about the GPIOx usefulness, since ESP SDK uses pin numbering as-is. One notable exception is wemos / nodemcu with D1,D2, etc. numbers, but those are mapped by the esp8266/Arduino variants |
Hi,
correct me if I'm wrong.
The log shows button numbering corresponding to the C code vector index, i.e.:
the button #0 in the C vector index is BUTTON1 in the #define
While that's obvious to the seasoned C devs and for who looks carefully at the source code, this appears as inconsistent and may confuse newcomers looking at the log for devices with more than one button.
Hope this helps someone,
Piero
The text was updated successfully, but these errors were encountered: