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

Fix default pin assignement for unassigned buttons, take 2 #4098

Open
wants to merge 1 commit into
base: 0_15
Choose a base branch
from

Conversation

PaoloTK
Copy link

@PaoloTK PaoloTK commented Aug 11, 2024

Follow up on #3953
Fill btnPin array with -1 unless corresponding member of buttonType array has been set to a value != 0 (BTN_TYPE_NONE).
0.14 branch had a similar implementation where it filled all elements except the first with -1, but since in 0.15 you can define multiple button pins at compile time it was removed.
Because of this change it is no longer necessary to specify the second pin and type on the default BTNPIN and BTNTYPE defs.

Credit to @softhack007 for the idea.

@blazoncek
Copy link
Collaborator

If you want to do it, do it correctly.
This is the place for this kind of initialisation:

WLED/wled00/cfg.cpp

Lines 255 to 269 in 3615ab5

// new install/missing configuration (button 0 has defaults)
if (fromFS) {
// relies upon only being called once with fromFS == true, which is currently true.
uint8_t s = 0;
if (pinManager.allocatePin(btnPin[0], false, PinOwner::Button)) { // initialized to #define value BTNPIN, or zero if not defined(!)
++s; // do not clear default button if allocated successfully
}
for (; s<WLED_MAX_BUTTONS; s++) {
btnPin[s] = -1;
buttonType[s] = BTN_TYPE_NONE;
macroButton[s] = 0;
macroLongPress[s] = 0;
macroDoublePress[s] = 0;
}
}

@PaoloTK
Copy link
Author

PaoloTK commented Aug 11, 2024

If you want to do it, do it correctly. This is the place for this kind of initialisation:

WLED/wled00/cfg.cpp

Lines 255 to 269 in 3615ab5

// new install/missing configuration (button 0 has defaults)
if (fromFS) {
// relies upon only being called once with fromFS == true, which is currently true.
uint8_t s = 0;
if (pinManager.allocatePin(btnPin[0], false, PinOwner::Button)) { // initialized to #define value BTNPIN, or zero if not defined(!)
++s; // do not clear default button if allocated successfully
}
for (; s<WLED_MAX_BUTTONS; s++) {
btnPin[s] = -1;
buttonType[s] = BTN_TYPE_NONE;
macroButton[s] = 0;
macroLongPress[s] = 0;
macroDoublePress[s] = 0;
}
}

While as always I truly appreciate the help and guidance, there's no need to be rude to people trying to help, so you could have done without the first sentence.

I spent a long time testing the code block you linked and could never get it to trigger for reasons beyond my understanding. I asked in the Discord and got no answers. So I did the next best thing I could think of, found where that behavior was implemented in the previous release and placed the code there.

If you look at the block you quoted, if it was to trigger the fix I implemented should not be needed since one of the conditions to set btnPin to -1 is buttonType[s] == BTN_TYPE_NONE.

If the block above should always be triggered on first boot and it's not due to a bug, it's obviously better to figure out why that is rather than patch the code somewhere else, so I'm willing to look into that instead.
However, fixing this bug is actually quite crucial since if it isn't (and when someone raised the issue in #3953 it was marked "won't fix") once 0.15 ships ALL configurations that use pin 0 for LED output on ESP32 will break. I tried implementing a quick fix for that instead previously but was shut down.

@softhack007 softhack007 added this to the 0.15.1 candidate milestone Sep 28, 2024
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

Successfully merging this pull request may close these issues.

3 participants