Add initialization logic to switch type inputs for correct boot-up behaviour#5374
Add initialization logic to switch type inputs for correct boot-up behaviour#5374
Conversation
Added boot-up state initialization logic to switch type inputs.
WalkthroughRefactors the handleSwitch function in button.cpp to introduce caching of the current switch state and restructure the switch-state machine logic, using waitTime as an explicit initialization and debounce state controller with three defined states before processing edge detection. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
KuglicsL
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/button.cpp`:
- Line 136: Remove the unreachable "break;" statements that follow a "return" in
button.cpp (the redundant break; lines shown in the diff); locate the
switch/case or control blocks where a return immediately precedes a break and
delete those break statements to avoid unreachable-code warnings and cleanup the
code.
🧹 Nitpick comments (1)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@wled00/button.cpp`: - Line 136: Remove the unreachable "break;" statements that follow a "return" in button.cpp (the redundant break; lines shown in the diff); locate the switch/case or control blocks where a return immediately precedes a break and delete those break statements to avoid unreachable-code warnings and cleanup the code.wled00/button.cpp (1)
136-136: Dead code:breakafterreturnis unreachable.The comments say "redundant, left as guard," but these statements are truly unreachable — no compiler or runtime path can execute them. They may trigger compiler warnings (e.g.,
-Wunreachable-code). Consider removing them.Proposed fix
++buttons[b].waitTime; // change initialization stage to debouncing return; // will have to debounce later, might as well return now - break; // redundant, left as guard case 1: // debouncingreturn; - break; // redundant, left as guard case 2: // the switch is initializedAlso applies to: 150-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/button.cpp` at line 136, Remove the unreachable "break;" statements that follow a "return" in button.cpp (the redundant break; lines shown in the diff); locate the switch/case or control blocks where a return immediately precedes a break and delete those break statements to avoid unreachable-code warnings and cleanup the code.
|
thanks for contributing. if (buttons[b].waitTime == 0) {
buttons[b].waitTime++;
buttons[b].pressedBefore = isButtonPressed(b);;
} |
This would skip the debouncing. My example. The board is supplied from 12V -> SMPS -> MCU 3.3V. It also has optocoupler inputs triggered by >=10V, driven from the 12V supply. If the 12V ramp up is slow, the MCU can start up, read switch input and save it as bootup state before 12V reaches the trigger level for the optocoupler stage. Having the initial debounce also helps if your input has a large external filter cap, only pulled up by internal pullup - it gives the cap time to be charged by the large MCU pullup resistance. Also debounce in general is used against noise spikes from bad wiring and unstable supply practices. E.g. WLED hardware installed in cars could suffer the most from the lack of it. Overall I think the slightly more complicated solution is more foolproof and has minimal impact on the handleSwitch() function's speed and size. |
|
debounce as the name sais is to "debounce" a switch, it should not be used to compensate for all sorts of bad hardware design. At boot a switch will be stable and can not bounce. if power is unstable at boot, there is a compile flag for such setups. |
|
I agree about the bad HW design part, but 99% of WLED users have no knowledge what a complier even is. If they encounter an issue like this, they will just find a workaround or downgrade their project. Increasing stability is always welcome especially in unpredictable end user environments. Debouncing buttons in all states (even initial) keeps industry best practices in sight and gives button handling the most stability. It has low added size (based on amap: button.cpp 256873 -> 267622) and very low added runtime cycles after initialization is done (maybe two extra comparsions in switch - case). I'd like if others chipped in with their ideas, other than that I rest my case. |
|
if you want to have debounce for each state change that is fine with me but it then should be done properly without code duplication. |
|
startup delay logic aside: |
|
On the code duplication note: I think the whole button handling could use a makeover to move debouncing from individual handleButton/handleSwitch debouncing to a combined debouncer. The short/long press differentiation then could be handled in their own separate state machine. Right now, the debounce threshold is referenced in three different places, and it is not possible to implement initial state debouncing without duplication.
That is correct - I presumed the suggested behaviour should be the default based on the comments in the source code. We would have two types:
If this sounds like a useful feature, I might sift through the source and try implementing it. In the meantime, this PR is a dead end. |
yea, that is what I was hinting at. piling fixes upon a bad base makes it worse over time (and is probably how it got the way it is). We try to improve things in baby steps.
I am not sure this is the best way but it might be, I am not using toggle switches except for tests, all I can say that to me the current implementation is a bit confusing on when it actually switches a preset. Also PIR sensor uses the same code if I read that right, current implementation is already edge driven(?). You are right that the whole code could use a better, unified approach. |
|
Current implementation based on my understanding:
After looking at the source some more, there's a few more weird behaviours in there. If a long strip is currently updating, both analog and digital button refresh time is tied to ANALOG_BTN_READ_CYCLE (250ms). Why not have a dedicated define for that frequency, and why is it this large? The only difference between a PIR switch and a normal SWITCH is inverted reading and a unique MQTT topic (/motion) for the PIR sensor. I believe the overall cleanest solution would be to rework the button handling to individually configurable entities, rather than pre-configured input types and a global pullup toggle checkbox. Obviously this approach introduces a lot of new possibilities for interfacing on the user side. It is totally feasible to make this change backwards compatible, and selectable presets (BUTTON, SWITCH, PIR) would allow users to easily select old button types in the new system. If you think this is not over the top, I could start working on a large, backwards compatible overhaul in my free time. Let me know what you think. |
|
without checking code: the list you have there should be pretty much how it is implemented right now. or which are the differences (apart from the code being messy) |
|
[COMMON GLOBAL OPTIONS] [COMMON INDIVIDUAL OPTIONS] [MOMENTARY SWITCH GLOBAL OPTIONS] [TOGGLE SWITCH INDIVIDUAL OPTIONS] Example structure: //shared variables between all configurable buttons
class ConfigurableInput {
int8_t pin; // -1 if uninitialized, positive otherwise
uint8_t type; // cfginput type
bool inverted; // if true, input logic level is inverted
String MQTTEventTopic; // individual configurable MQTT topic for each input
uint8_t pullDirection; // none/up/down
uint8_t prevState; // previous value for debouncing
unsigned long lastTransition; // last transition time for debouncing
unsigned long debounceTime; // debouncing time threshold
uint8_t presetEventPrimary; // preset to call on primary event (short press for button, positive edge for simple switch, any edge for toggle switch)
uint8_t presetEventSecondary; // preset to call on secondary event (long press for button, negative edge for simple switch)
};
class MomentarySwitch: public ConfigurableInput {
unsigned long longPressThreshold; // minimum duration for long press
unsigned long doublePressInterval; // maximum interval for double press
};
class ToggleSwitch: public ConfigurableInput {
bool edgeMode; // if true, boot-up state does not matter
};
class TouchSwitch: public ConfigurableInput {
uint8_t touchThreshold; // touch button threshold level
};
class AnalogSwitch: public ConfigurableInput {
uint16_t turnOffThreshold; // global brightness mode turnoff value
uint16_t filteredValue; // IIR filtered analog value
uint8_t filterCoeff; // 0 - 100 IIR filter coefficient
};So basically this would move lots of compile time options into the runtime configurable territory. |
2 participants
Added boot-up state initialization logic to switch type inputs.
This ensures switch type inputs can be in any state at boot-up without erroneously calling Off->On state change macros.
The initialization logic debounces the input state, initializes button state based on the filtered value and prints debug information.
The changes were tested on an ESP32.
Summary by CodeRabbit