WLED 0.15+ always resets GPIO 0 to become a button pin (two new bugs)

[Arcitec]

What happened?

The ability to have more than 2 buttons has introduced a bug in WLED's pin handling.

• The WLED_MAX_BUTTONS default array length is dynamic and is 2 for ESP8266 and 4 for ESP32. It can also be edited by the compiler flag to become even larger if wanted.
• WLED only gives 2 default values for BTNPIN and BTNTYPE. Which means there's some unspecified values in the array on ESP32:

#define BTNPIN 0,-1
#define BTNTYPE BTN_TYPE_PUSH,BTN_TYPE_NONE

• C++ arrays default all unspecified values to 0. So btnPin[4] = {2,3} becomes {2,3,0,0} automatically. So for example, WLED's default values for an ESP32 binary will look like this:

int8_t btnPin[4] = {0, -1, 0, 0};
byte buttonType[4] = {2, 0, 0, 0};

• Bug 1: The WLED code that is supposed to fix this at the first launch in cfg.cpp: if (fromFS) { never executes. That code is supposed to set GPIO = -1 and Type = NONE for every button pin whose pre-defined firmware setting for the GPIO was either -1 or the type was NONE. Basically a "reset all unused pins". But that code never executes!
• This means that all of the unused/unspecified button pins all stay defined as "GPIO 0 type NONE (0)".
• Bug 2: There's a second bug in the button initialization code: It activates and takes over ownership of ALL "defined" (>= 0) button GPIO pins even if their type is set to NONE (Disabled). So it steals/allocates all "disabled" button pins too. If a button setting is "disabled", it should not steal ownership of them!
• And because the button initialization code runs after the LED initialization, it means that GPIO 0 is always overwritten to become a button pin instead of an LED pin on all ESP32 devices.
• The most important bug is that the settings cleanup code never executes. But the "pins are taken over by the button handler even if their type is set to NONE (Disabled)" is a nasty bug too.

There is a temporary workaround via compiler flags: By manually specifying every array index on ESP32 builds:

-D WLED_MAX_BUTTONS=4
-D BTNPIN=0,-1,-1,-1
-D BTNTYPE=2,0,0,0

To Reproduce Bug

x

Expected Behavior

x

Install Method

Binary from WLED.me

What version of WLED?

0.15 and newer

Which microcontroller/board are you seeing the problem on?

ESP32

Relevant log/trace output

Anything else?

No response

Code of Conduct

• I agree to follow this project's Code of Conduct

[Arcitec]

I've been digging around the issue tracker now. There has been previous discussion of this and some talks about fixes, but it didn't have a good summary of the actual bugs this causes, so it got lost in the noise.

• Closed issue: Talks about unset button pins becoming 0 by default, but didn't realize that it was a big problem causing LED outputs to be overwritten. BTNPIN compile time option sets unused buttons to GPIO 0 #3953
• Closed pull request: Attempted to "fix" it by hardcoding a list of -1 values, which was not a good solution since the length of the button array can be changed dynamically. Fix default button pin assignment for esp32 #4074
• Stale pull request: Outdated code and inappropriate solution which attempted to add a "init to -1" in the wrong place in the code: Fix default pin assignement for unassigned buttons, take 2 #4098

The last pull request above actually talks about the cfg.cpp solution.

However, if we start digging into v0.15.1.beta2, we see some interesting changes in WLED:

https://github.com/wled/WLED/tree/v0.15.1.beta2

WLED/wled00/cfg.cpp

Lines 292 to 299 in 6572efb

	// new install/missing configuration (button 0 has defaults)
	if (fromFS) {
	// relies upon only being called once with fromFS == true, which is currently true.
	for (size_t s = 0; s < WLED_MAX_BUTTONS; s++) {
	if (buttonType[s] == BTN_TYPE_NONE || btnPin[s] < 0 || !PinManager::allocatePin(btnPin[s], false, PinOwner::Button)) {
	btnPin[s] = -1;
	buttonType[s] = BTN_TYPE_NONE;
	}

There's newly added code which is supposed to set the buttons to GPIO = -1, Type = NONE (0) if either GPIO was -1 or Type was NONE (0).

Edit: I see that the fix was added 7 months ago in this commit: da6d64e

@PaoloTK told me on Discord that the new code doesn't run. He said that if we add a debug print statement to the highlighted fromFS code block, it actually never runs, so maybe the fix doesn't work?

Here's what to do next:

1. Check if the highlighted cfg settings fix above actually runs and actually works. It's supposed to clear the unused pins in config on first startup after a fresh USB flash.
2. Also fix the button assignment code so that it doesn't take over "Disabled (Type == NONE)" button GPIOs! :) Because users don't expect that behavior. WLED is currently setting the pin owner to "button" for every GPIO in the button config UI, even when their type is set to "disabled".
3. If both bugs are fixed, we can close this and all the other linked PRs and issues.

[PaoloTK]

@PaoloTK told me on Discord that the new code doesn't run. He said that if we add a debug print statement to the highlighted fromFS code block, it actually never runs, so maybe the fix doesn't work?

To clarify, this was my experience when testing while working on the previous 2 PRs. It could have been fixed since then, but I have been unable to test.

[blazoncek]

I've finally identified the issue. It is an early return from EEPROM handling code (that's since become obsolete).

[blazoncek]

@Arcitec how familiar are you with git patches? I can provide a patch for testing but is unfortunately based on my fork's stable branch.
You'll need to add remote to your local git repo, checkout my branch and apply patch on top of that. Then do the testing.

[github-actions]

Hey! This issue has been open for quite some time without any new comments now. It will be closed automatically in a week if no further activity occurs.
Thank you for using WLED! ✨

[netmindz]

Has this been resolved now @blazoncek ?

[blazoncek]

Has this been resolved now?

Once #4757 is merged.
However I have not received any confirmation or other feedback on the PR. Neither form @Arcitec nor @PaoloTK or anyone else.

[DrTHC]

This is happening again in latest version

[DedeHai]

@DrTHC not for me. can you detail out on how to reproduce it?