A reimplementation of time-controlled presets (scalable). (I suck at HTML/JavaScript, so I need help) #4789
Conversation
## Walkthrough
The changes migrate the timer system from fixed-size arrays to a dynamic vector-based approach throughout the codebase. Timer management, serialization, deserialization, and UI handling are refactored to support up to 32 timers plus separate sunrise/sunset timers, with validation and more flexible data structures. The user interface for timer presets is now dynamic, allowing users to add or remove presets interactively and manage sunrise/sunset presets separately.
## Changes
| File(s) | Change Summary |
|-------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| wled00/cfg.cpp | Refactored timer serialization/deserialization to use a vector-based system; replaced fixed arrays, clarified enabled state encoding, and improved date range handling; separated sunrise/sunset timers. |
| wled00/data/settings_time.htm | Converted time presets UI from a static table to a dynamic, user-expandable list supporting up to 32 presets; added separate Sunrise/Sunset presets section with dedicated controls and processing logic. |
| wled00/fcn_declare.h | Added timer management function declarations, constants, a new Timer struct with helper methods, and an external vector of Timer objects; increased max presets to 32; defined special sunrise/sunset constants. |
| wled00/ntp.cpp | Replaced legacy timer arrays with a vector; added thread-safe timer management functions with validation; synchronized vector to legacy arrays; updated timer checking logic to use vector and support sunrise/sunset timers. |
| wled00/set.cpp | Refactored timer handling in settings to clear and add timers via new functions; added validation; supported separate sunrise/sunset timers with offsets; expanded timer indexing beyond 10 fixed timers. |
| wled00/wled.h | Expanded global timer arrays from 10 to 34 elements to accommodate 32 regular timers plus sunrise/sunset timers; updated initialization accordingly. |
## Estimated code review effort
🎯 4 (Complex) | ⏱️ ~40 minutes
- Complexity: The changes span core logic, UI, and configuration handling, introducing a new data structure and validation logic, and updating multiple interconnected modules. The review requires careful attention to data migration, backward compatibility, and UI/logic correctness.
## Suggested reviewers
- DedeHaiNote ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
wled00/cfg.cpp (1)
708-711: Consider simplifying the enabled bit logic.The current implementation shifts weekdays and then conditionally sets the enabled bit. This could be simplified and made more readable:
- uint8_t weekdays = timer[F("dow")] | 0; - weekdays <<= 1; // shift to make room for enabled bit - bool enabled = timer["en"] | false; - if (enabled) weekdays |= 0x01; // set enabled bit + uint8_t weekdays = (timer[F("dow")] | 0) << 1; // shift weekdays to upper 7 bits + if (timer["en"] | false) weekdays |= 0x01; // set enabled bit if timer is enabledwled00/data/settings_time.htm (1)
241-242: Use more specific button IDs to avoid conflictsThe button IDs "+" and "-" are too generic and might conflict with other page elements.
- <button type="button" id="+" onclick="addTimePreset()">+</button> - <button type="button" id="-" onclick="removeLastPreset()">-</button> + <button type="button" id="btnAddTimer" onclick="addTimePreset()">+</button> + <button type="button" id="btnRemoveTimer" onclick="removeLastPreset()">-</button>Also update the references in
updateButtonStates():function updateButtonStates() { - gId("+").style.display = (currentPresetCount<maxTimePresets) ? "inline":"none"; - gId("-").style.display = (currentPresetCount>1) ? "inline":"none"; + gId("btnAddTimer").style.display = (currentPresetCount<maxTimePresets) ? "inline":"none"; + gId("btnRemoveTimer").style.display = (currentPresetCount>1) ? "inline":"none"; }wled00/ntp.cpp (2)
414-420: Consider rejecting invalid values instead of silently constrainingThe validation silently constrains invalid month/day values instead of rejecting them, which could hide user input errors.
- // Validate month range - monthStart = constrain(monthStart, 1, 12); - monthEnd = constrain(monthEnd, 1, 12); - - // Validate day range - dayStart = constrain(dayStart, 1, 31); - dayEnd = constrain(dayEnd, 1, 31); + // Validate month range + if (monthStart < 1 || monthStart > 12 || monthEnd < 1 || monthEnd > 12) { + DEBUG_PRINTLN(F("Error: Invalid month range")); + return; + } + + // Validate day range + if (dayStart < 1 || dayStart > 31 || dayEnd < 1 || dayEnd > 31) { + DEBUG_PRINTLN(F("Error: Invalid day range")); + return; + }
489-489: Document the date range encoding formatThe encoding of month start/end into a single byte is not documented, which could confuse future maintainers.
Add a comment explaining the encoding:
// Date range info (only for regular timers) + // Encode monthStart in upper 4 bits, monthEnd in lower 4 bits timerMonth[regularTimerCount] = (timer.monthStart << 4) | (timer.monthEnd & 0x0F);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
wled00/cfg.cpp(2 hunks)wled00/data/settings_time.htm(3 hunks)wled00/fcn_declare.h(1 hunks)wled00/ntp.cpp(3 hunks)wled00/set.cpp(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
wled00/cfg.cpp (1)
wled00/ntp.cpp (4)
clearTimers(429-432)clearTimers(429-429)addTimer(383-426)addTimer(383-385)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (usermods)
🔇 Additional comments (8)
wled00/set.cpp (1)
554-591: Well-implemented timer validation and migration.The validation logic properly handles:
- Different constraints for regular vs sunrise/sunset timers
- Minute field as offset for sunrise/sunset (-59 to 59)
- Weekday bits limited to 7 bits
- Date ranges only for regular timers
The use of
constrain()for safe value clamping and the extended ASCII indexing (0-9, A-F) for supporting 16 timers are good design choices.wled00/data/settings_time.htm (4)
31-44: Well-structured dynamic table generationThe refactored
BTa()function properly implements dynamic timer generation with good separation of concerns. The minimum timer count of 1 ensures the UI is always usable.
76-86: Robust implementation of timer additionThe
addTimePreset()function correctly enforces the maximum timer limit and properly updates the DOM and UI state.
88-102: Correct handling of timer removalThe function properly maintains the minimum timer count and correctly removes both the main and detail rows for each timer.
109-118: Successful adaptation to dynamic timer countThe
Wd()function correctly usescurrentPresetCountfor iteration and properly constructs the weekday bitmask for each timer.wled00/ntp.cpp (3)
547-587: Well-refactored timer checking logicThe modified
checkTimers()function correctly handles all timer types with proper validation and debugging. The use of Timer methods provides clean abstraction.
510-534: Clean getter function implementationsThe timer count functions are well-implemented and appropriate for the expected small dataset size.
387-390: No undefined constant –maxTimePresetsis declared inwled00/fcn_declare.h
The reference tomaxTimePresetsis valid; it’s defined as// wled00/fcn_declare.h const uint8_t maxTimePresets = 16;Please disregard the “undefined constant” concern. The remaining validation suggestions still apply:
- Sunrise/sunset minute validation
Allows values from –59 to 59 but does not check against actual sunrise/sunset times. Consider stricter bounds or explicit rejection of unrealistic values.- Month/day validation
Usesconstrain(), which silently corrects out-of-range inputs. To catch invalid parameters, consider rejecting values outside acceptable ranges instead of auto-clamping.Likely an incorrect or invalid review comment.
|
Now that I’ve slept, I can write more info. |
|
I fixed everything i could think of, but still cant get the UI to populate correctly |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
wled00/set.cpp (1)
640-641: Same preset validation issue as sunrise timer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
wled00/cfg.cpp(2 hunks)wled00/data/settings_time.htm(4 hunks)wled00/fcn_declare.h(1 hunks)wled00/ntp.cpp(3 hunks)wled00/set.cpp(1 hunks)wled00/wled.h(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- wled00/wled.h
🚧 Files skipped from review as they are similar to previous changes (2)
- wled00/fcn_declare.h
- wled00/cfg.cpp
🧰 Additional context used
🧠 Learnings (2)
wled00/ntp.cpp (4)
Learnt from: blazoncek
PR: #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: #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: blazoncek
PR: #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: KrX3D
PR: #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 (3)
Learnt from: blazoncek
PR: #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: #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: #4482
File: wled00/udp.cpp:343-347
Timestamp: 2025-02-19T12:41:22.676Z
Learning: The setName() method in WLED's Segment class (FX_fcn.cpp) handles all necessary validation internally, including:
- Null pointer checks
- Length validation against WLED_MAX_SEGNAME_LEN
- Memory allocation/reallocation
No additional validation or name clearing is needed before calling setName().
🔇 Additional comments (11)
wled00/set.cpp (2)
590-591: Verify the logic for adding timers with preset 0.The condition allows adding a timer when
preset == 0if the enabled bit is set. This might lead to unexpected behavior if preset 0 has special meaning (e.g., default action or disabled state).Consider whether this is intentional or if the condition should be:
-if (preset > 0 || (weekdays & 0x01)) { +if (preset > 0 && (weekdays & 0x01)) {
645-681: Well-structured regular timer processing.The implementation properly validates all inputs and skips invalid entries. Good use of
constrain()to ensure values are within valid ranges.wled00/ntp.cpp (5)
13-52: Excellent thread safety implementation.The dual approach using FreeRTOS mutex for ESP32 and interrupt disabling for single-core platforms is well-designed. The RAII pattern ensures proper cleanup.
424-474: Robust timer validation and memory protection.Excellent implementation with comprehensive validation, memory growth protection, and appropriate error messages. The differentiated validation for regular timers vs sunrise/sunset offsets is particularly well done.
538-540: Clever month range encoding.Good use of bit packing to store both monthStart and monthEnd in a single byte. This maintains backward compatibility while extending functionality.
565-594: Clean and thread-safe helper functions.Well-designed API with focused, single-purpose functions. Consistent use of thread safety throughout.
596-652: Well-refactored timer checking logic.Clean separation of timer types with appropriate handling for each. The sunrise/sunset offset calculation is correctly implemented.
wled00/data/settings_time.htm (4)
11-30: Proper initialization of timer count from server data.The implementation correctly determines the existing timer count by checking for valid weekday values, which should resolve UI population issues.
72-115: Excellent data preservation in row generation.The function properly extracts and preserves all timer fields from existing data, which should prevent data loss during UI updates.
134-139: Verify row removal logic for detail rows.The function removes the last 2 rows assuming they're a main/detail pair. Ensure the detail row is always present or check if it exists before removal to prevent removing the wrong rows.
Consider checking row count or class names to ensure you're removing the correct rows.
150-160: Correctly updated to handle dynamic timer count.The function properly iterates over all active timers and correctly constructs the weekday bitmask including the enabled bit.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
wled00/set.cpp (1)
595-643: Duplicate validation logic - consider the same refactoring.The sunset timer processing uses identical validation patterns as the sunrise timer. The same refactoring suggestion applies here to reduce code duplication.
🧹 Nitpick comments (1)
wled00/set.cpp (1)
545-593: Consider refactoring sunrise timer processing for better maintainability.The sunrise timer processing logic is correct and includes thorough validation, but it's quite verbose. The validation pattern is repeated for each parameter with similar structure.
Consider extracting a helper function to reduce code duplication:
+// Helper function for parameter validation with debug logging +template<typename T> +T validateTimerParam(const String& value, T minVal, T maxVal, const char* paramName, const char* timerType) { + T rawValue = static_cast<T>(value.toInt()); + if (rawValue < minVal || rawValue > maxVal) { + DEBUG_PRINTF("%s timer: Invalid %s %d, constraining to range %d-%d\n", + timerType, paramName, (int)rawValue, (int)minVal, (int)maxVal); + } + return constrain(rawValue, minVal, maxVal); +}Then use it like:
-int presetRaw = request->arg("SR_T").toInt(); -if (presetRaw < 0 || presetRaw > 250) { - DEBUG_PRINTF("Sunrise timer: Invalid preset %d, constraining to range 0-250\n", presetRaw); -} -uint8_t preset = constrain(presetRaw, 0, 250); +uint8_t preset = validateTimerParam(request->arg("SR_T"), 0, 250, "preset", "Sunrise");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/set.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
wled00/set.cpp (3)
Learnt from: blazoncek
PR: #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: #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: #4482
File: wled00/udp.cpp:343-347
Timestamp: 2025-02-19T12:41:22.676Z
Learning: The setName() method in WLED's Segment class (FX_fcn.cpp) handles all necessary validation internally, including:
- Null pointer checks
- Length validation against WLED_MAX_SEGNAME_LEN
- Memory allocation/reallocation
No additional validation or name clearing is needed before calling setName().
🧬 Code Graph Analysis (1)
wled00/set.cpp (1)
wled00/ntp.cpp (4)
clearTimers(477-481)clearTimers(477-477)addTimer(425-474)addTimer(425-427)
🔇 Additional comments (5)
wled00/set.cpp (5)
542-543: LGTM: Clean timer initialization approach.The approach of clearing all existing timers first and then rebuilding from form data ensures a clean state and prevents stale timer entries from persisting.
646-681: Robust regular timer processing with good validation.The regular timer processing logic is well-implemented with:
- Proper existence checks for timer entries
- Validation of empty/zero preset values (addressing the past review comment)
- Appropriate parameter constraints for regular timers (0-23 hours, 0-59 minutes)
- Correct weekday bit masking (0x7F for 7 bits vs 0xFF for sunrise/sunset)
651-654: Excellent implementation of the improved timer existence check.This properly addresses the previous review comment by checking both for argument existence AND meaningful content. The validation correctly skips empty or zero preset values before processing the timer.
667-668: Good differentiation between regular and sunrise/sunset timer constraints.The code correctly applies different validation rules:
- Regular timers: 7-bit weekdays mask (0x7F) since they don't need the enabled bit
- Sunrise/sunset timers: 8-bit weekdays (0xFF) to preserve the enabled bit
- Regular timers: 0-59 minutes vs sunrise/sunset: -59 to +59 offset
This demonstrates proper understanding of the different timer types and their requirements.
647-648: maxTimePresets value and loop bounds validatedmaxTimePresets is consistently defined as 32 in wled00/fcn_declare.h and mirrored in data/settings_time.htm. All loops in set.cpp (and related code) iterate from 0 to 31, matching the intended 32 regular timers. No adjustments required.
…onor the UI “enabled” flag
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
I know this is not fully ready for review (to merge) but its is ready for someone else to look at (it is beyond my ability's) |
There was a problem hiding this comment.
I am wondering what will happen when users upgrade their WLED and have all 10 timed presets used? I do not see an upgrade code.
I'm also wondering why did you choose to add semaphoring to timer logic? IMO that's an overkill not needed in this particular case.
I did not look at JS/HTML code thoroughly since the underlying C/C++ code needs more work.
| } | ||
|
|
||
| // Load sunrise/sunset timers from separate "sunrise_sunset" object | ||
| JsonObject sunriseSunset = tm["sunrise_sunset"]; |
There was a problem hiding this comment.
No need to have such long key, as this is not a UI element. Use short keys (not necessarily descriptive) like "ss".
Also. Why would you limit yourself to just one "sunrise" and "sunset" event?
You should be able to schedule different sunrise and sunset events as well.
| for (JsonObject timer : timers) { | ||
| if (it > 9) break; |
There was a problem hiding this comment.
Where do you handle legacy configuration? Consider upgrade path.
| @@ -200,9 +311,132 @@ <h3>Button actions</h3> | |||
| </tbody> | |||
| </table> | |||
| <a href="https://kno.wled.ge/features/macros/#analog-button" target="_blank">Analog Button setup</a> | |||
| <h3>Sunrise/Sunset Presets</h3> | |||
| <div style="display: inline-block;"> | |||
There was a problem hiding this comment.
Please use classes instead of style properties.
| bool hasSunsetTimer(); | ||
|
|
||
| // Timer constants | ||
| const uint8_t maxTimePresets = 32; |
There was a problem hiding this comment.
Use constexpr or #define if this is pure constant so that compiler can optimize.
| const uint8_t TIMER_HOUR_SUNRISE = 255; // Special value for sunrise timer | ||
| const uint8_t TIMER_HOUR_SUNSET = 254; // Special value for sunset timer | ||
|
|
||
| // External timer declarations |
There was a problem hiding this comment.
These should go into .cpp file.
|
|
||
| // WARNING: may cause errors in sunset calculations on ESP8266, see #3400 | ||
| // building with `-D WLED_USE_REAL_MATH` will prevent those errors at the expense of flash and RAM | ||
|
|
||
| // Dynamic timer storage with thread safety | ||
| std::vector<Timer> timers; |
There was a problem hiding this comment.
Use static as this vector should not be accessible from elsewhere. You added wrappers.
| applyPreset(timerMacro[i]); | ||
| TimerCriticalSection lock; | ||
| for (const auto& timer : timers) { | ||
| if (!timer.isEnabled() || timer.preset == 0) continue; |
| WLED_GLOBAL byte timerHours[] _INIT_N(({ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 })); | ||
| WLED_GLOBAL int8_t timerMinutes[] _INIT_N(({ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 })); | ||
| WLED_GLOBAL byte timerMacro[] _INIT_N(({ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 })); | ||
| WLED_GLOBAL byte timerHours[34] _INIT_N(({ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 })); |
There was a problem hiding this comment.
This is hard for ESP8266, consuming a lot of precious RAM.
I'm trying to make it easy to add lots more scheduled presets, and I'm burnt out after coding for 15 hours, so I don't feel like writing a detailed pr description.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes