Skip to content

Conversation

@PaoloTK
Copy link
Contributor

@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
Contributor

If you want to do it, do it correctly.
This is the place for this kind of initialisation:
https://github.com/Aircoookie/WLED/blob/3615ab535b3ebfb77a175a4cb2949d0a4a516143/wled00/cfg.cpp#L255-L269

@PaoloTK
Copy link
Contributor Author

PaoloTK commented Aug 11, 2024

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

https://github.com/Aircoookie/WLED/blob/3615ab535b3ebfb77a175a4cb2949d0a4a516143/wled00/cfg.cpp#L255-L269

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
@netmindz netmindz changed the base branch from 0_15 to main December 16, 2024 13:33
@netmindz
Copy link
Member

Can one of the reviewers please approve if this it ready to merge (and cherry pick to the release branch) ?

@blazoncek
Copy link
Contributor

My language may not be the best but my intentions are always good.

Although this PR may be a workaround, I've managed to pinpoint the cause of the issue (incorrect button assignment at 1st boot).
The same problem is present at bus initialisation and that has been awkwardly solved by placing it in finalizeInit(). I'll try to provide correct fix for both issues.

@DedeHai
Copy link
Collaborator

DedeHai commented May 19, 2025

@blazoncek did you fix this elsewhere? can this be closed?

@blazoncek
Copy link
Contributor

I have unlimited and truly variable button support with correct 1st boot initialisation ready in my fork.
Unfortunately it will require my other open PRs to be merged before I can open a PR with a fix. This is due to the fact it is not only a fix but also enhancement.

Copy link
Contributor

@blazoncek blazoncek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though this fix should be ok, I would prefer #4757 being implemented instead.

@PaoloTK
Copy link
Contributor Author

PaoloTK commented Oct 7, 2025

Even though this fix should be ok, I would prefer #4757 being implemented instead.

Same

@netmindz netmindz closed this Nov 8, 2025
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.

5 participants