Variable button count (up to 32) #4757
Conversation
- adds ability to configure variable number of buttons during runtime - fixes #4692
WalkthroughReplaces legacy parallel button arrays with a Button struct and a dynamic std::vector, updates core button handling, config (serialize/deserialize), web UI, XML output, and multiple usermods to use the new buttons vector and member accesses. No public API signatures changed. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
wled00/data/settings_leds.htm (1)
563-583: Consider renaming theofffunction for clarity.The
offfunction name in the onclick handler is misleading as it sets the pin value to -1 rather than turning something off. Consider renaming it toclearPinorresetPinfor better clarity.Apply this diff to improve function naming:
-<span style="cursor: pointer;" onclick="off('BT${s}')"> ✕</span><br></div>`; +<span style="cursor: pointer;" onclick="clearPin('BT${s}')"> ✕</span><br></div>`;Also update the function definition at line 12:
-function off(n) { gN(n).value = -1;} +function clearPin(n) { gN(n).value = -1; } // Reset pin to unused statewled00/cfg.cpp (1)
412-438: Minor: Clarify comment about button configuration skip.The comment at line 425 could be more precise about why button configuration is skipped.
Apply this diff to clarify the comment:
-continue; // pin not available or invalid, skip configuring this GPIO +continue; // button type is none or pin not available, skip configurationwled00/wled.h (1)
646-658: Consider optimizing struct member ordering for better packing.The current member ordering might not be optimally packed due to alignment requirements. Consider reordering members by size for potentially better memory usage:
struct Button { unsigned long pressedTime; // time button was pressed unsigned long waitTime; // time to wait for next button press - int8_t pin; // pin number - struct { - uint8_t type : 6; // button type (push, long, double, etc.) - bool pressedBefore : 1; // button was pressed before - bool longPressed : 1; // button was long pressed - }; uint8_t macroButton; // macro/preset to call on button press uint8_t macroLongPress; // macro/preset to call on long press uint8_t macroDoublePress; // macro/preset to call on double press + int8_t pin; // pin number + struct { + uint8_t type : 6; // button type (push, long, double, etc.) + bool pressedBefore : 1; // button was pressed before + bool longPressed : 1; // button was long pressed + };This groups larger members together and could reduce padding between members, though the actual benefit depends on the compiler and target architecture.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
usermods/EXAMPLE/usermod_v2_example.cpp(1 hunks)usermods/audioreactive/audio_reactive.cpp(1 hunks)usermods/multi_relay/multi_relay.cpp(3 hunks)usermods/pixels_dice_tray/pixels_dice_tray.cpp(2 hunks)usermods/usermod_v2_four_line_display_ALT/usermod_v2_four_line_display_ALT.cpp(1 hunks)wled00/button.cpp(14 hunks)wled00/cfg.cpp(2 hunks)wled00/const.h(1 hunks)wled00/data/settings_leds.htm(6 hunks)wled00/set.cpp(3 hunks)wled00/wled.h(1 hunks)wled00/xml.cpp(3 hunks)
🧰 Additional context used
🧠 Learnings (8)
usermods/EXAMPLE/usermod_v2_example.cpp (1)
Learnt from: willmmiles
PR: wled/WLED#4623
File: usermods/word-clock-matrix/word-clock-matrix.cpp:332-332
Timestamp: 2025-03-29T01:22:54.617Z
Learning: In the WLED project, hardcoded usermod IDs (like 500 in the WordClockMatrix's getId() method) are intentionally used as part of a strategy to avoid modifying core code. There are plans to remove numeric usermod IDs entirely in the future.
wled00/const.h (11)
Learnt from: blazoncek
PR: wled/WLED#4658
File: wled00/const.h:140-141
Timestamp: 2025-04-26T19:19:07.600Z
Learning: In WLED, the WLED_MAX_PANELS macro is intentionally defined as a fixed constant value (18) with no redefinition mechanism, making it "unoverridable" - there's no need for a static assertion to check its maximum value.
Learnt from: blazoncek
PR: wled/WLED#4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.200Z
Learning: In WLED, maximum segment name length varies by platform:
- ESP8266: 32 characters (WLED_MAX_SEGNAME_LEN = 32)
- ESP32: 64 characters (WLED_MAX_SEGNAME_LEN = 64)
This platform difference can cause truncation when syncing longer names from ESP32 to ESP8266. Additionally, the WLED UI has limitations regarding modified maximum segment name lengths.
Learnt from: blazoncek
PR: wled/WLED#4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: DedeHai
PR: wled/WLED#4682
File: wled00/FX.cpp:8997-9005
Timestamp: 2025-05-09T18:43:15.355Z
Learning: In the WLED codebase, SEGMENT.custom3 is always constrained to the range 0-31 and will not exceed this range.
Learnt from: KrX3D
PR: wled/WLED#4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.h:121-137
Timestamp: 2025-04-27T10:06:22.545Z
Learning: In the Seven Segment Display Reloaded usermod, the dimension mismatch between the default array (umSSDRNumbers[11][7]) and the override (umSSDRNumbers[11][10] = umSSDR_NUMBERS) is intentional by design, allowing for different use cases with varying numbers of segments per digit.
Learnt from: DedeHai
PR: wled/WLED#4682
File: wled00/FX.cpp:9016-9039
Timestamp: 2025-05-09T18:48:21.296Z
Learning: WLED does not support matrices large enough (≥1024 px) for 32-bit overflow in dx*dx + dy*dy to be a practical concern in particle system effects.
Learnt from: KrX3D
PR: wled/WLED#4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.cpp:430-435
Timestamp: 2025-04-28T20:51:29.773Z
Learning: In WLED, `bri` is a global variable used for brightness control.
Learnt from: KrX3D
PR: wled/WLED#4664
File: wled00/syslog.cpp:50-51
Timestamp: 2025-04-26T12:51:04.993Z
Learning: In the WLED codebase, syslog configuration variables (syslogHost, syslogPort, syslogProtocol, syslogFacility, syslogSeverity) are defined as global variables in wled.h and are accessed directly in the SyslogPrinter implementation, rather than being class members.
Learnt from: KrX3D
PR: wled/WLED#4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.h:121-136
Timestamp: 2025-04-27T09:37:28.415Z
Learning: Using PROGMEM for the seven-segment font array (umSSDRNumbers) in the WLED SSDR usermod causes compilation problems, so it should be left as a regular array.
wled00/data/settings_leds.htm (8)
Learnt from: KrX3D
PR: wled/WLED#4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.cpp:430-435
Timestamp: 2025-04-28T20:51:29.773Z
Learning: In WLED, `bri` is a global variable used for brightness control.
Learnt from: blazoncek
PR: wled/WLED#4658
File: wled00/const.h:140-141
Timestamp: 2025-04-26T19:19:07.600Z
Learning: In WLED, the WLED_MAX_PANELS macro is intentionally defined as a fixed constant value (18) with no redefinition mechanism, making it "unoverridable" - there's no need for a static assertion to check its maximum value.
Learnt from: blazoncek
PR: wled/WLED#4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.200Z
Learning: In WLED, maximum segment name length varies by platform:
- ESP8266: 32 characters (WLED_MAX_SEGNAME_LEN = 32)
- ESP32: 64 characters (WLED_MAX_SEGNAME_LEN = 64)
This platform difference can cause truncation when syncing longer names from ESP32 to ESP8266. Additionally, the WLED UI has limitations regarding modified maximum segment name lengths.
Learnt from: DedeHai
PR: wled/WLED#4682
File: wled00/FX.cpp:8997-9005
Timestamp: 2025-05-09T18:43:15.355Z
Learning: In the WLED codebase, SEGMENT.custom3 is always constrained to the range 0-31 and will not exceed this range.
Learnt from: blazoncek
PR: wled/WLED#4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
Learnt from: DedeHai
PR: wled/WLED#4682
File: wled00/FX.cpp:9016-9039
Timestamp: 2025-05-09T18:48:21.296Z
Learning: WLED does not support matrices large enough (≥1024 px) for 32-bit overflow in dx*dx + dy*dy to be a practical concern in particle system effects.
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
wled00/xml.cpp (11)
Learnt from: blazoncek
PR: wled/WLED#4658
File: wled00/const.h:140-141
Timestamp: 2025-04-26T19:19:07.600Z
Learning: In WLED, the WLED_MAX_PANELS macro is intentionally defined as a fixed constant value (18) with no redefinition mechanism, making it "unoverridable" - there's no need for a static assertion to check its maximum value.
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: blazoncek
PR: wled/WLED#4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Learnt from: blazoncek
PR: wled/WLED#4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Learnt from: DedeHai
PR: wled/WLED#4682
File: wled00/FX.cpp:8997-9005
Timestamp: 2025-05-09T18:43:15.355Z
Learning: In the WLED codebase, SEGMENT.custom3 is always constrained to the range 0-31 and will not exceed this range.
Learnt from: KrX3D
PR: wled/WLED#4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.h:121-136
Timestamp: 2025-04-27T09:37:28.415Z
Learning: Using PROGMEM for the seven-segment font array (umSSDRNumbers) in the WLED SSDR usermod causes compilation problems, so it should be left as a regular array.
Learnt from: blazoncek
PR: wled/WLED#4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs by falling back to the main segment ID, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Learnt from: blazoncek
PR: wled/WLED#4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.200Z
Learning: In WLED, maximum segment name length varies by platform:
- ESP8266: 32 characters (WLED_MAX_SEGNAME_LEN = 32)
- ESP32: 64 characters (WLED_MAX_SEGNAME_LEN = 64)
This platform difference can cause truncation when syncing longer names from ESP32 to ESP8266. Additionally, the WLED UI has limitations regarding modified maximum segment name lengths.
Learnt from: blazoncek
PR: wled/WLED#4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like `if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))` is unnecessary and could be problematic, as it bypasses proper size verification.
Learnt from: KrX3D
PR: wled/WLED#4664
File: wled00/syslog.cpp:50-51
Timestamp: 2025-04-26T12:51:04.993Z
Learning: In the WLED codebase, syslog configuration variables (syslogHost, syslogPort, syslogProtocol, syslogFacility, syslogSeverity) are defined as global variables in wled.h and are accessed directly in the SyslogPrinter implementation, rather than being class members.
wled00/set.cpp (10)
Learnt from: blazoncek
PR: wled/WLED#4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Learnt from: blazoncek
PR: wled/WLED#4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Learnt from: KrX3D
PR: wled/WLED#4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.h:121-136
Timestamp: 2025-04-27T09:37:28.415Z
Learning: Using PROGMEM for the seven-segment font array (umSSDRNumbers) in the WLED SSDR usermod causes compilation problems, so it should be left as a regular array.
Learnt from: blazoncek
PR: wled/WLED#4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like `if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))` is unnecessary and could be problematic, as it bypasses proper size verification.
Learnt from: blazoncek
PR: wled/WLED#4700
File: wled00/wled_server.cpp:409-414
Timestamp: 2025-05-26T16:09:34.325Z
Learning: In wled00/wled_server.cpp, the OTA update access control logic intentionally allows OTA updates from different subnets when otaSameSubnet is true AND a PIN is set. This was a conscious design decision by blazoncek to provide flexibility for remote OTA updates with PIN protection, though they acknowledged it may not be the optimal security model.
Learnt from: blazoncek
PR: wled/WLED#4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs by falling back to the main segment ID, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Learnt from: blazoncek
PR: wled/WLED#4658
File: wled00/const.h:140-141
Timestamp: 2025-04-26T19:19:07.600Z
Learning: In WLED, the WLED_MAX_PANELS macro is intentionally defined as a fixed constant value (18) with no redefinition mechanism, making it "unoverridable" - there's no need for a static assertion to check its maximum value.
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: DedeHai
PR: wled/WLED#4750
File: usermods/audioreactive/audio_reactive.cpp:311-321
Timestamp: 2025-06-27T06:04:44.652Z
Learning: In WLED's AudioReactive usermod, FFT buffer memory (valFFT, windowFFT) is allocated once during initialization and never deallocated, following typical embedded system design where core functionality buffers persist for the application lifetime. Raw buffer pointers for aligned allocations don't need to be tracked in the current implementation since deallocation is never performed.
wled00/cfg.cpp (4)
Learnt from: blazoncek
PR: wled/WLED#4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like `if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))` is unnecessary and could be problematic, as it bypasses proper size verification.
Learnt from: KrX3D
PR: wled/WLED#4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.h:121-136
Timestamp: 2025-04-27T09:37:28.415Z
Learning: Using PROGMEM for the seven-segment font array (umSSDRNumbers) in the WLED SSDR usermod causes compilation problems, so it should be left as a regular array.
Learnt from: blazoncek
PR: wled/WLED#4700
File: wled00/wled_server.cpp:409-414
Timestamp: 2025-05-26T16:09:34.325Z
Learning: In wled00/wled_server.cpp, the OTA update access control logic intentionally allows OTA updates from different subnets when otaSameSubnet is true AND a PIN is set. This was a conscious design decision by blazoncek to provide flexibility for remote OTA updates with PIN protection, though they acknowledged it may not be the optimal security model.
Learnt from: blazoncek
PR: wled/WLED#4658
File: wled00/const.h:140-141
Timestamp: 2025-04-26T19:19:07.600Z
Learning: In WLED, the WLED_MAX_PANELS macro is intentionally defined as a fixed constant value (18) with no redefinition mechanism, making it "unoverridable" - there's no need for a static assertion to check its maximum value.
wled00/button.cpp (6)
Learnt from: KrX3D
PR: wled/WLED#4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.h:121-136
Timestamp: 2025-04-27T09:37:28.415Z
Learning: Using PROGMEM for the seven-segment font array (umSSDRNumbers) in the WLED SSDR usermod causes compilation problems, so it should be left as a regular array.
Learnt from: blazoncek
PR: wled/WLED#4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Learnt from: blazoncek
PR: wled/WLED#4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Learnt from: KrX3D
PR: wled/WLED#4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.cpp:430-435
Timestamp: 2025-04-28T20:51:29.773Z
Learning: In WLED, `bri` is a global variable used for brightness control.
Learnt from: KrX3D
PR: wled/WLED#4237
File: usermods/INA219_v2/INA219_v2.cpp:265-276
Timestamp: 2025-04-18T22:27:58.634Z
Learning: When implementing MQTT message handling in WLED usermods, use `strstr(topic, "/specific/path")` instead of `strcmp_P(topic, PSTR("/specific/path"))` to properly match topics that include the device prefix. The full MQTT topic typically follows the pattern `<mqttDeviceTopic>/specific/path`.
Learnt from: blazoncek
PR: wled/WLED#4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs by falling back to the main segment ID, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
wled00/wled.h (11)
Learnt from: blazoncek
PR: wled/WLED#4658
File: wled00/const.h:140-141
Timestamp: 2025-04-26T19:19:07.600Z
Learning: In WLED, the WLED_MAX_PANELS macro is intentionally defined as a fixed constant value (18) with no redefinition mechanism, making it "unoverridable" - there's no need for a static assertion to check its maximum value.
Learnt from: KrX3D
PR: wled/WLED#4664
File: wled00/syslog.cpp:50-51
Timestamp: 2025-04-26T12:51:04.993Z
Learning: In the WLED codebase, syslog configuration variables (syslogHost, syslogPort, syslogProtocol, syslogFacility, syslogSeverity) are defined as global variables in wled.h and are accessed directly in the SyslogPrinter implementation, rather than being class members.
Learnt from: KrX3D
PR: wled/WLED#4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.cpp:430-435
Timestamp: 2025-04-28T20:51:29.773Z
Learning: In WLED, `bri` is a global variable used for brightness control.
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: blazoncek
PR: wled/WLED#4700
File: wled00/wled_server.cpp:409-414
Timestamp: 2025-05-26T16:09:34.325Z
Learning: In wled00/wled_server.cpp, the OTA update access control logic intentionally allows OTA updates from different subnets when otaSameSubnet is true AND a PIN is set. This was a conscious design decision by blazoncek to provide flexibility for remote OTA updates with PIN protection, though they acknowledged it may not be the optimal security model.
Learnt from: blazoncek
PR: wled/WLED#4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Learnt from: blazoncek
PR: wled/WLED#4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Learnt from: KrX3D
PR: wled/WLED#4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.h:121-136
Timestamp: 2025-04-27T09:37:28.415Z
Learning: Using PROGMEM for the seven-segment font array (umSSDRNumbers) in the WLED SSDR usermod causes compilation problems, so it should be left as a regular array.
Learnt from: DedeHai
PR: wled/WLED#4682
File: wled00/FX.cpp:8997-9005
Timestamp: 2025-05-09T18:43:15.355Z
Learning: In the WLED codebase, SEGMENT.custom3 is always constrained to the range 0-31 and will not exceed this range.
Learnt from: DedeHai
PR: wled/WLED#4750
File: usermods/audioreactive/audio_reactive.cpp:311-321
Timestamp: 2025-06-27T06:04:44.652Z
Learning: In WLED's AudioReactive usermod, FFT buffer memory (valFFT, windowFFT) is allocated once during initialization and never deallocated, following typical embedded system design where core functionality buffers persist for the application lifetime. Raw buffer pointers for aligned allocations don't need to be tracked in the current implementation since deallocation is never performed.
🧬 Code Graph Analysis (4)
usermods/usermod_v2_four_line_display_ALT/usermod_v2_four_line_display_ALT.cpp (3)
usermods/EXAMPLE/usermod_v2_example.cpp (2)
b(312-327)b(312-312)usermods/pixels_dice_tray/pixels_dice_tray.cpp (2)
b(462-520)b(462-462)usermods/usermod_v2_four_line_display_ALT/usermod_v2_four_line_display.h (1)
b(256-256)
usermods/EXAMPLE/usermod_v2_example.cpp (2)
usermods/pixels_dice_tray/pixels_dice_tray.cpp (2)
b(462-520)b(462-462)usermods/audioreactive/audio_reactive.cpp (2)
b(1527-1538)b(1527-1527)
usermods/pixels_dice_tray/pixels_dice_tray.cpp (3)
usermods/EXAMPLE/usermod_v2_example.cpp (2)
b(312-327)b(312-312)usermods/audioreactive/audio_reactive.cpp (2)
b(1527-1538)b(1527-1527)wled00/button.cpp (2)
isButtonPressed(93-122)isButtonPressed(93-93)
usermods/audioreactive/audio_reactive.cpp (2)
usermods/EXAMPLE/usermod_v2_example.cpp (2)
b(312-327)b(312-312)usermods/pixels_dice_tray/pixels_dice_tray.cpp (2)
b(462-520)b(462-462)
🔇 Additional comments (27)
wled00/const.h (2)
97-97: LGTM: Increased button support for ESP8266 platforms.The increase from 2 to 10 maximum buttons for ESP8266 platforms aligns well with the PR objective of supporting variable button count while being reasonable for memory-constrained devices.
99-99: LGTM: Increased button support for other platforms.The increase from 4 to 32 maximum buttons perfectly matches the PR objective of supporting "up to 32" buttons on more capable platforms.
usermods/EXAMPLE/usermod_v2_example.cpp (1)
316-320: LGTM: Updated to use structured button data.The change from
buttonType[b]tobuttons[b].typecorrectly adopts the new unified button management structure. The logic for filtering button types remains identical, ensuring no behavioral changes while improving code consistency across the codebase.usermods/usermod_v2_four_line_display_ALT/usermod_v2_four_line_display_ALT.cpp (1)
752-757: LGTM: Consistent adoption of structured button data.The migration from
buttonType[b]tobuttons[b].typefollows the same pattern as other usermods, maintaining identical filtering logic while adopting the new unified button management structure. This change supports the dynamic button system introduced by the PR.usermods/audioreactive/audio_reactive.cpp (1)
1533-1533: LGTM! Correct implementation of the button data structure refactor.This change properly replaces the global
buttonType[b]array access with the new unifiedbuttons[b].typestructure, maintaining the same logical functionality while supporting the variable button count feature.wled00/xml.cpp (3)
276-285: LGTM! Updated bLimits function call with new maximum button count.The addition of
WLED_MAX_BUTTONSto thebLimitsJavaScript function call correctly reflects the increased button limit and aligns with the variable button count feature.
388-391: LGTM! Clean transition to unified button structure.The range-based loop over the
buttonsvector with direct access to button properties (button.pin,button.type) is a clean improvement over separate array iterations. This maintains the same output format while using the new structured approach.
564-567: LGTM! Consistent button macro handling with unified structure.The loop correctly accesses button macro properties through the unified
buttonsvector, maintaining the same functionality while improving code organization.usermods/multi_relay/multi_relay.cpp (1)
564-639: LGTM! Successful migration to unified button structure.All button state references have been correctly updated from separate global arrays to the unified
buttons[b]struct fields. The logic for button handling, debouncing, long press detection, and double press detection remains unchanged, ensuring compatibility while improving code organization.usermods/pixels_dice_tray/pixels_dice_tray.cpp (1)
464-520: LGTM! Proper integration with unified button structure.The usermod correctly updates button type checks to use
buttons[b].typewhile maintaining its own local state arrays for button handling. The button state management logic (press/release detection, long press, double press) remains intact, ensuring continued functionality with the new button structure.wled00/button.cpp (5)
18-37: LGTM! Button action functions correctly updated.The
shortPressActionfunction properly accesses button macros through the unified structure (buttons[b].macroButton) while maintaining the same logic and MQTT publishing behavior.
93-122: LGTM! Button state detection properly migrated.The
isButtonPressedfunction correctly uses the unified button structure to access pin and type information. Touch threshold handling and all button type logic remain intact.
124-163: LGTM! Switch handling successfully refactored.The
handleSwitchfunction properly accesses all button state fields through the unified structure while preserving the switch detection logic, debouncing, and MQTT publishing functionality.
267-267: LGTM! Dynamic loop bounds using buttons vector size.Using
buttons.size()instead of a fixedWLED_MAX_BUTTONSconstant properly supports the variable button count feature and prevents iteration over uninitialized button slots.
290-362: LGTM! Momentary button logic successfully migrated.The complex momentary button handling logic (press detection, long press, double press, debouncing) has been correctly updated to use the unified button structure while preserving all timing logic and state management functionality.
wled00/data/settings_leds.htm (3)
9-9: LGTM! Proper initialization of button limits.The
maxBTvariable is correctly initialized and updated through thebLimitsfunction parameter.Also applies to: 45-45, 54-54
584-590: LGTM! Button removal logic is properly implemented.The function correctly prevents removing the last button and updates the UI state through
btnBtn().
849-855: LGTM! Clean UI integration for button management.The button container and controls are properly integrated into the form with appropriate visibility management.
wled00/cfg.cpp (3)
356-357: LGTM! Proper pin cleanup before clearing buttons.Good practice to deallocate all button pins before clearing the vector to prevent resource leaks.
363-388: LGTM! Comprehensive ESP32 button validation.Excellent validation for ESP32-specific button types with clear error messages and proper touch interrupt configuration.
993-1002: LGTM! Proper button configuration serialization.The serialization correctly iterates through the buttons vector and outputs all button properties.
wled00/set.cpp (4)
131-139: LGTM! Proper button pin cleanup with touch interrupt handling.The code correctly deallocates button pins and handles ESP32 S2/S3 touch interrupt detachment.
280-284: LGTM! Correct button vector management.The code properly handles both adding new buttons and updating existing ones based on the index.
327-331: Verify: Button cleanup loop preserves index 0.The loop condition
i > 0means button at index 0 will never be removed even if unused. Is this intentional to always keep at least one button slot in the configuration?If this is intentional, consider adding a comment to clarify:
-// we should remove all unused buttons from the vector +// remove all unused buttons from the vector (except index 0 to maintain at least one slot)
535-544: LGTM! Proper macro assignment through button struct.The code correctly updates button macros using the new struct members with proper identifier generation for indices >= 10.
wled00/wled.h (2)
646-669: Well-structured refactoring of button data!The consolidation of button properties into a single
Buttonstruct is a significant improvement over multiple parallel arrays. The use of bitfields for compact storage is appropriate for embedded systems.
659-670: Excellent implementation of dynamic button management!The constructor properly initializes all members with sensible defaults, and the transition from fixed arrays to
std::vector<Button>enables the runtime-configurable button count feature described in the PR objectives.
|
Are we comfortable with having variable buttons? |
|
the code looks ok, I have to agree with the rabbit on the struct packing though. |
GPIO0 can be overridden by BTNPIN and only one button is now default (type push button). Coderabbit does not understand everything. 😉 |
|
no it does not, and it is nitpicking but you could drop the inner struct and use all uint8_t/int8_t and it would still leave space for one uint8_t for future use. the struct is 32bit aligned. or add a comment, that there are 3 padding bytes. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
wled00/data/settings_leds.htm (1)
603-623: Call pinDropdowns() after adding/removing buttons to keep pin conflict logic correctaddBtn/remBtn append/remove raw fields, but pin conflict checks and reservation text use select.pin. Without converting new inputs via pinDropdowns(), conflicts for newly added buttons are missed.
Add pinDropdowns() after DOM updates:
function addBtn(i,p,t) { var b = gId("btns"); var c = b.innerHTML; var s = chrID(i); c += `<div id="btn${i}">#${i} GPIO: <input type="number" name="BT${s}" onchange="UI()" min="-1" max="${d.max_gpio}" class="xs" value="${p}">`; ... b.innerHTML = c; + pinDropdowns(); btnBtn(); UI(); } function remBtn() { var b = gId("btns"); if (b.children.length <= 1) return; b.lastElementChild.remove(); + pinDropdowns(); btnBtn(); UI(); }Also applies to: 624-630
wled00/set.cpp (1)
276-329: Do not allocate pins for disabled buttons; guard parsing with hasArg()Two fixes needed:
- Skip allocation when type is BTN_TYPE_NONE (prevents disabled entries claiming GPIOs).
- Only process indices present in the form; otherwise empty args default to 0 and cause spurious allocations.
Apply:
- for (int i = 0; i < WLED_MAX_BUTTONS; i++) { + for (int i = 0; i < WLED_MAX_BUTTONS; i++) { int offset = i < 10 ? '0' : 'A' - 10; char bt[4] = "BT"; bt[2] = offset+i; bt[3] = 0; // button pin (use A,B,C,... if WLED_MAX_BUTTONS>10) char be[4] = "BE"; be[2] = offset+i; be[3] = 0; // button type (use A,B,C,... if WLED_MAX_BUTTONS>10) - int hw_btn_pin = request->arg(bt).toInt(); - if (i >= buttons.size()) buttons.emplace_back(hw_btn_pin, request->arg(be).toInt()); // add button to vector - else { - buttons[i].pin = hw_btn_pin; - buttons[i].type = request->arg(be).toInt(); - } + // Stop when the next indexed fields are absent + if (!request->hasArg(bt) || !request->hasArg(be)) break; + const int hw_btn_pin = request->arg(bt).length() ? request->arg(bt).toInt() : -1; + const uint8_t hw_btn_type = request->arg(be).toInt(); + if (i >= (int)buttons.size()) buttons.emplace_back(-1, BTN_TYPE_NONE); // ensure slot exists + buttons[i].type = hw_btn_type; + // Disabled buttons must not claim pins + if (buttons[i].type == BTN_TYPE_NONE || hw_btn_pin < 0) { + buttons[i].pin = -1; + continue; + } + buttons[i].pin = hw_btn_pin; if (buttons[i].pin >= 0 && PinManager::allocatePin(buttons[i].pin, false, PinOwner::Button)) { #ifdef ARDUINO_ARCH_ESP32 // ESP32 only: check that button pin is a valid gpio - if ((buttons[i].type == BTN_TYPE_ANALOG) || (buttons[i].type == BTN_TYPE_ANALOG_INVERTED)) { + if ((buttons[i].type == BTN_TYPE_ANALOG) || (buttons[i].type == BTN_TYPE_ANALOG_INVERTED)) { if (digitalPinToAnalogChannel(buttons[i].pin) < 0) { // not an ADC analog pin DEBUG_PRINTF_P(PSTR("PIN ALLOC error: GPIO%d for analog button #%d is not an analog pin!\n"), buttons[i].pin, i); PinManager::deallocatePin(buttons[i].pin, PinOwner::Button); buttons[i].type = BTN_TYPE_NONE; } else { analogReadResolution(12); // see #4040 } - } else if ((buttons[i].type == BTN_TYPE_TOUCH || buttons[i].type == BTN_TYPE_TOUCH_SWITCH)) { + } else if ((buttons[i].type == BTN_TYPE_TOUCH || buttons[i].type == BTN_TYPE_TOUCH_SWITCH)) { if (digitalPinToTouchChannel(buttons[i].pin) < 0) { // not a touch pin DEBUG_PRINTF_P(PSTR("PIN ALLOC error: GPIO%d for touch button #%d is not an touch pin!\n"), buttons[i].pin, i); PinManager::deallocatePin(buttons[i].pin, PinOwner::Button); buttons[i].type = BTN_TYPE_NONE; } #ifdef SOC_TOUCH_VERSION_2 // ESP32 S2 and S3 have a fucntion to check touch state but need to attach an interrupt to do so - else touchAttachInterrupt(buttons[i].pin, touchButtonISR, touchThreshold << 4); // threshold on Touch V2 is much higher (1500 is a value given by Espressif example, I measured changes of over 5000) + else touchAttachInterrupt(buttons[i].pin, touchButtonISR, touchThreshold << 4); // threshold on Touch V2 is much higher (1500 is a value given by Espressif example, I measured changes of over 5000) #endif - } else + } else #endif { // regular buttons and switches if (disablePullUp) { pinMode(buttons[i].pin, INPUT); } else { #ifdef ESP32 pinMode(buttons[i].pin, buttons[i].type==BTN_TYPE_PUSH_ACT_HIGH ? INPUT_PULLDOWN : INPUT_PULLUP); #else pinMode(buttons[i].pin, INPUT_PULLUP); #endif } } } else { buttons[i].pin = -1; buttons[i].type = BTN_TYPE_NONE; } }This prevents disabled entries from reserving GPIOs and avoids creating spurious button slots when the form doesn’t include them.
🧹 Nitpick comments (1)
wled00/wled.h (1)
649-673: Avoid mixed-type bitfields; use explicit bytes or pack predictablyMixing uint8_t and bool bitfields in the same struct can defeat packing, vary per-compiler, and waste RAM. Prefer explicit bytes or a packed bitmask.
Example refactor:
- struct { - uint8_t type : 6; - bool pressedBefore : 1; - bool longPressed : 1; - }; + uint8_t type; // lower 6 bits used + uint8_t flags; // bit0: pressedBefore, bit1: longPressedAccessors:
- pressedBefore = flags & 0x01
- longPressed = flags & 0x02
Or, if you keep bitfields, add a packing/alignment note to document expected padding.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
usermods/audioreactive/audio_reactive.cpp(1 hunks)wled00/cfg.cpp(2 hunks)wled00/const.h(1 hunks)wled00/data/settings_leds.htm(6 hunks)wled00/set.cpp(3 hunks)wled00/wled.h(2 hunks)wled00/xml.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- usermods/audioreactive/audio_reactive.cpp
- wled00/const.h
🧰 Additional context used
📓 Path-based instructions (5)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/set.cppwled00/xml.cppwled00/cfg.cpp
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/settings_leds.htm
wled00/data/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
wled00/data/**: When modifying web UI files, runnpm run buildto regenerate embedded headers before any firmware build
For web UI changes, edit files only under wled00/data (not firmware or generated files)
Files:
wled00/data/settings_leds.htm
wled00/data/settings*.htm
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Name settings pages as settings*.htm within the web UI
Files:
wled00/data/settings_leds.htm
wled00/**/!(html_*)*.h
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for non-generated C++ header files (.h)
Files:
wled00/wled.h
🧬 Code graph analysis (3)
wled00/set.cpp (3)
wled00/pin_manager.cpp (6)
isPinAllocated(182-187)isPinAllocated(182-182)deallocatePin(28-42)deallocatePin(28-28)allocatePin(148-178)allocatePin(148-148)usermods/audioreactive/audio_reactive.cpp (1)
ARDUINO_ARCH_ESP32(554-1707)wled00/button.cpp (2)
touchButtonISR(397-400)touchButtonISR(397-397)
wled00/wled.h (1)
wled00/bus_manager.h (1)
vector(168-168)
wled00/cfg.cpp (2)
wled00/pin_manager.cpp (4)
deallocatePin(28-42)deallocatePin(28-28)allocatePin(148-178)allocatePin(148-148)wled00/button.cpp (2)
touchButtonISR(397-400)touchButtonISR(397-397)
🔇 Additional comments (6)
wled00/data/settings_leds.htm (1)
889-895: Reminder: run UI embed build after web changesAfter editing files under wled00/data, regenerate embedded headers before firmware build.
- Run: npm run build
wled00/wled.h (1)
299-303: Verify defaulting button to GPIO0 across boardsBTNPIN=0 and BTNTYPE=BTN_TYPE_PUSH will try to allocate GPIO0 on first boot. On some ESP32 boards GPIO0 is used for RMII clock or boot straps. PinManager should block reserved pins, but please confirm this default is desired with Ethernet targets and your supported boards.
wled00/xml.cpp (3)
293-304: bLimits propagation and max buttons wiring LGTMPassing WLED_MAX_BUTTONS into bLimits aligns the UI with runtime limits.
407-410: Buttons rendering via vector LGTMIterating buttons to emit addBtn(i,pin,type) is correct and keeps UI in sync with runtime config.
583-586: Macro rows rendering via vector LGTMaddRow wiring from button macro fields is consistent with the new model.
wled00/cfg.cpp (1)
405-430: Default-from-FS path: avoid allocating when default type is NONEIf BTNTYPE resolves to BTN_TYPE_NONE, skip allocation and insert a disabled entry.
- for (size_t s = 0; s < WLED_MAX_BUTTONS && s < numPins; s++) { - type = defTypes[s < numTypes ? s : numTypes - 1]; // use last known type to set current type if types less than pins - if (type == BTN_TYPE_NONE || defPins[s] < 0 || !PinManager::allocatePin(defPins[s], false, PinOwner::Button)) { + for (size_t s = 0; s < WLED_MAX_BUTTONS && s < numPins; s++) { + type = defTypes[s < numTypes ? s : numTypes - 1]; + if (type == BTN_TYPE_NONE || defPins[s] < 0 || !PinManager::allocatePin(defPins[s], false, PinOwner::Button)) { if (buttons.size() == 0) buttons.emplace_back(-1, BTN_TYPE_NONE); // add disabled button to vector (so we have at least one button defined) continue; // pin not available or invalid, skip configuring this GPIO }Likely an incorrect or invalid review comment.
|
did a quick test and there are some bugs:
|
Summary by CodeRabbit
New Features
Improvements
Refactor