Lightweight Scheduling System Proposal (Feedback Requested) #4775
Conversation
WalkthroughA scheduling feature was introduced, allowing users to upload, download, and manage a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Suggested reviewers
Note ⚡️ Unit Test Generation - BetaCodeRabbit's unit test generation is now available in Beta! Automatically generate comprehensive unit tests for your code changes, ensuring better test coverage and catching edge cases you might miss. Our AI analyzes your code structure and creates tests that follow best practices and your project's testing patterns. Learn more here, or just try it under ✨ Finishing Touches. ✨ 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
wled00/schedule.cpp (1)
87-147: Consider making the JSON buffer size configurable.The hardcoded 4096-byte buffer on line 99 might be limiting for complex schedules. Consider defining it as a constant for easier maintenance.
Add a constant definition after the
SCHEDULE_FILEdefinition:#define SCHEDULE_FILE "/schedule.json" +#define SCHEDULE_JSON_BUFFER_SIZE 4096Then use it in the function:
- DynamicJsonDocument doc(4096); + DynamicJsonDocument doc(SCHEDULE_JSON_BUFFER_SIZE);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
wled00/data/settings_time.htm(1 hunks)wled00/schedule.cpp(1 hunks)wled00/schedule.h(1 hunks)wled00/wled.cpp(3 hunks)wled00/wled_server.cpp(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
wled00/wled_server.cpp (1)
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.
wled00/wled.cpp (2)
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: 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.
🧬 Code Graph Analysis (1)
wled00/wled.cpp (2)
wled00/schedule.h (2)
checkSchedule(27-27)loadSchedule(24-24)wled00/schedule.cpp (4)
checkSchedule(33-83)checkSchedule(33-33)loadSchedule(87-147)loadSchedule(87-87)
🔇 Additional comments (4)
wled00/data/settings_time.htm (1)
181-185: No action needed:uploadFileis definedThe
uploadFile(fileObj, name)function is implemented inwled00/data/common.js, so the upload functionality is already available in the loaded scripts.wled00/wled.cpp (1)
5-5: LGTM! Well-integrated scheduling functionality.The scheduling system is properly integrated:
- Header inclusion is correct
checkSchedule()is appropriately placed afterhandleTime()to ensure time is updated before checkingloadSchedule()is correctly called after filesystem initializationAlso applies to: 58-58, 419-420
wled00/schedule.h (1)
1-27: LGTM! Well-structured header file.The header file is properly designed with:
- Modern
#pragma onceguard- Clear documentation for all struct fields
- Appropriate use of fixed-width integer types
- Reasonable limit of 32 schedule events
wled00/wled_server.cpp (1)
178-277: Excellent implementation of atomic schedule upload!The enhanced upload handling for
/schedule.jsonis well-designed:
- Uses temporary file to prevent corruption during upload
- Implements atomic replacement with proper fallback logic
- Includes comprehensive error handling and meaningful response messages
- Immediately loads the new schedule ensuring changes take effect
|
My $0.02: this should be implemented as a usermod. I have no objections to including such a module in standard builds (like audioreactive), but from a code architectural standpoint, I think it's better to avoid adding more code to the core when the module framework provides APIs with sufficient power. (In fact, I am pushing to migrate existing core features to modules in the future - see cases like #4868). |
|
I definitely don’t know what would work best from an architectural standpoint, but from a user perspective, I’d really like this scheduling feature to be available by default. Most users aren’t going to compile WLED themselves just to add a usermod, so having it built-in makes it way more accessible, that is why I made this. That said, I’m open to ideas on how to implement it better while still making this easy for everyone to use. |
|
I'll ask again: why not extend the already existing scheduler and make that more versatile instead of adding another one? |
|
Honestly, I'm not sure what’s best here. My main goal was to keep things simple and clean, but as its my first time doing anything like this is i didn't quite understand how the old way worked >///< (I've been figuring that out), I implemented a new way which could certainly be worse. I’m definitely open to ideas on extending the existing scheduler if that’s preferred. |
|
To add to the discussion: Enhancing current implementation (using vectors instead of fixed array with 10 elemts) would be better. The only added complexity would be date scheduling. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
wled00/schedule.cpp(1 hunks)wled00/schedule.h(1 hunks)wled00/wled_server.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- wled00/wled_server.cpp
- wled00/schedule.h
🧰 Additional context used
🧠 Learnings (1)
wled00/schedule.cpp (2)
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: 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`.
🔇 Additional comments (5)
wled00/schedule.cpp (5)
1-8: LGTM!The file header and includes are properly structured and include all necessary dependencies for the scheduling functionality.
9-13: LGTM!Good use of
std::vectorfor dynamic storage as requested in previous reviews, and reasonable buffer size allocation.
15-29: LGTM!The date range logic correctly handles both normal ranges and year-end wrap-around cases. The implementation properly checks if the current date falls within the specified start and end date range.
83-104: LGTM!Proper file handling with existence checks, JSON buffer locking for thread safety, and appropriate error handling for JSON parsing.
32-79: Fix the date range check condition.The condition
if (e.startMonth)on line 68 will skip events scheduled for January since January is month 1, but the check only proceeds ifstartMonthis non-zero. Events with January start dates will be incorrectly skipped.Apply this fix:
- // Otherwise check if current date is within start and end date range - if (e.startMonth) { + // Check if current date is within start and end date range (when specified) + if (e.startMonth > 0) {Additionally, consider improving the time validation guard:
- if (now < 100000) return; // Invalid or uninitialized time guard + if (now < 946684800) return; // Invalid time guard (before year 2000)Likely an incorrect or invalid review comment.
|
@blazoncek I fixed the problems you mentioned. |
|
That's better. When that is done, replace existing schedules. |
|
I had a quick look. Some things here look pretty nice. Thanks for taking the time to work on this. But I agree with dedehai. Why not improve the existing scheduler instead of adding a 2nd? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
wled00/data/settings_time.htm (1)
123-163: Consider a more robust DOM manipulation approach.While the current reindexing implementation works, the heavy reliance on regex string replacement in innerHTML could be fragile if HTML structure changes. Consider using more targeted DOM API methods for updating specific attributes.
For example, instead of regex replacement:
-row.innerHTML = row.innerHTML.replace(new RegExp(`name="W${oldIndex}"`, 'g'), `name="W${newIndex}"`) +row.querySelectorAll(`[name="W${oldIndex}"]`).forEach(el => el.name = `W${newIndex}`)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/data/settings_time.htm(4 hunks)
🔇 Additional comments (7)
wled00/data/settings_time.htm (7)
11-12: LGTM! Reasonable configuration values.The maximum of 16 presets and initial count of 3 (sunrise, sunset, permanent) provides good balance between functionality and resource usage.
29-43: Excellent refactoring for maintainability.The BTa() function is now much cleaner and more maintainable using the generatePresetRow() helper. The dynamic generation logic correctly handles different preset types.
45-77: Well-designed helper function with proper type handling.The generatePresetRow() function effectively handles different preset types and generates clean HTML. The input constraints are appropriate (negative minutes for sunrise/sunset, positive for regular presets).
79-91: Solid implementation with proper validation.The addTimePreset() function correctly validates against the maximum limit, safely manipulates the DOM, and maintains UI consistency by updating button states.
93-121: Safe removal operations with proper validation.Both removal functions correctly protect built-in presets and maintain DOM consistency through reindexing. The user-friendly error messages provide clear feedback.
165-176: Clean button state management.The updateButtonStates() function correctly manages add/remove button states with proper null checks and logical conditions.
306-338: Server-side JSON validation confirmedschedule.cpp implements robust checks:
- Uses a fixed-size DynamicJsonDocument and handles deserializeJson errors.
- Validates all event fields (month, day, hour, minute, repeat count, preset ID) against allowable ranges, skipping any invalid entries.
No further action required.
| function FC() | ||
| { | ||
| for(i=0;i<10;i++) | ||
| for(i=0;i<currentPresetCount;i++) | ||
| { | ||
| let wd = gId("W"+i).value; | ||
| let wd = gId("W"+i); | ||
| if (!wd) continue; // Skip if preset doesn't exist | ||
| wd = wd.value; | ||
| for(j=0;j<8;j++) { | ||
| gId("W"+i+j).checked=wd>>j&1; | ||
| let checkbox = gId("W"+i+j); | ||
| if (checkbox) checkbox.checked=wd>>j&1; | ||
| } | ||
| if ((wd&254) != 254 || (i<8 && (gN("M"+i).value != 1 || gN("D"+i).value != 1 || gN("P"+i).value != 12 || gN("E"+i).value != 31))) { | ||
| expand(gId("CB"+i),i); //expand macros with custom DOW or date range set | ||
| if ((wd&254) != 254 || (i<8 && (gN("M"+i) && gN("D"+i) && gN("P"+i) && gN("E"+i) && | ||
| (gN("M"+i).value != 1 || gN("D"+i).value != 1 || gN("P"+i).value != 12 || gN("E"+i).value != 31)))) { | ||
| let cbElement = gId("CB"+i); | ||
| if (cbElement) expand(cbElement,i); //expand macros with custom DOW or date range set | ||
| } | ||
| } | ||
| updateAddButtonState(); | ||
| } | ||
| function Wd() | ||
| { | ||
| a = [0,0,0,0,0,0,0,0,0,0]; | ||
| for (i=0; i<10; i++) { | ||
| a = []; | ||
| for (i=0; i<currentPresetCount; i++) { | ||
| a[i] = 0; | ||
| m=1; | ||
| for(j=0;j<8;j++) { a[i]+=gId(("W"+i)+j).checked*m; m*=2;} | ||
| gId("W"+i).value=a[i]; | ||
| for (j=0; j<8; j++) { | ||
| let checkbox = gId("W"+i+j); | ||
| if (checkbox && checkbox.checked) a[i]+=m; | ||
| m*=2; | ||
| } | ||
| let hiddenInput = gId("W"+i); | ||
| if (hiddenInput) hiddenInput.value = a[i]; | ||
| } |
There was a problem hiding this comment.
Good adaptation to dynamic presets with minor issue.
The FC() and Wd() functions are correctly updated to handle dynamic presets with proper null checks. However, line 194 calls updateAddButtonState() which doesn't exist - should this be updateButtonStates()?
-updateAddButtonState();
+updateButtonStates();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function FC() | |
| { | |
| for(i=0;i<10;i++) | |
| for(i=0;i<currentPresetCount;i++) | |
| { | |
| let wd = gId("W"+i).value; | |
| let wd = gId("W"+i); | |
| if (!wd) continue; // Skip if preset doesn't exist | |
| wd = wd.value; | |
| for(j=0;j<8;j++) { | |
| gId("W"+i+j).checked=wd>>j&1; | |
| let checkbox = gId("W"+i+j); | |
| if (checkbox) checkbox.checked=wd>>j&1; | |
| } | |
| if ((wd&254) != 254 || (i<8 && (gN("M"+i).value != 1 || gN("D"+i).value != 1 || gN("P"+i).value != 12 || gN("E"+i).value != 31))) { | |
| expand(gId("CB"+i),i); //expand macros with custom DOW or date range set | |
| if ((wd&254) != 254 || (i<8 && (gN("M"+i) && gN("D"+i) && gN("P"+i) && gN("E"+i) && | |
| (gN("M"+i).value != 1 || gN("D"+i).value != 1 || gN("P"+i).value != 12 || gN("E"+i).value != 31)))) { | |
| let cbElement = gId("CB"+i); | |
| if (cbElement) expand(cbElement,i); //expand macros with custom DOW or date range set | |
| } | |
| } | |
| updateAddButtonState(); | |
| } | |
| function Wd() | |
| { | |
| a = [0,0,0,0,0,0,0,0,0,0]; | |
| for (i=0; i<10; i++) { | |
| a = []; | |
| for (i=0; i<currentPresetCount; i++) { | |
| a[i] = 0; | |
| m=1; | |
| for(j=0;j<8;j++) { a[i]+=gId(("W"+i)+j).checked*m; m*=2;} | |
| gId("W"+i).value=a[i]; | |
| for (j=0; j<8; j++) { | |
| let checkbox = gId("W"+i+j); | |
| if (checkbox && checkbox.checked) a[i]+=m; | |
| m*=2; | |
| } | |
| let hiddenInput = gId("W"+i); | |
| if (hiddenInput) hiddenInput.value = a[i]; | |
| } | |
| function FC() | |
| { | |
| for(i=0;i<currentPresetCount;i++) | |
| { | |
| let wd = gId("W"+i); | |
| if (!wd) continue; // Skip if preset doesn't exist | |
| wd = wd.value; | |
| for(j=0;j<8;j++) { | |
| let checkbox = gId("W"+i+j); | |
| if (checkbox) checkbox.checked=wd>>j&1; | |
| } | |
| if ((wd&254) != 254 || (i<8 && (gN("M"+i) && gN("D"+i) && gN("P"+i) && gN("E"+i) && | |
| (gN("M"+i).value != 1 || gN("D"+i).value != 1 || gN("P"+i).value != 12 || gN("E"+i).value != 31)))) | |
| { | |
| let cbElement = gId("CB"+i); | |
| if (cbElement) expand(cbElement,i); //expand macros with custom DOW or date range set | |
| } | |
| } | |
| - updateAddButtonState(); | |
| + updateButtonStates(); | |
| } | |
| function Wd() | |
| { | |
| a = []; | |
| for (i=0; i<currentPresetCount; i++) { | |
| a[i] = 0; | |
| m=1; | |
| for (j=0; j<8; j++) { | |
| let checkbox = gId("W"+i+j); | |
| if (checkbox && checkbox.checked) a[i]+=m; | |
| m*=2; | |
| } | |
| let hiddenInput = gId("W"+i); | |
| if (hiddenInput) hiddenInput.value = a[i]; | |
| } |
🤖 Prompt for AI Agents
In wled00/data/settings_time.htm around lines 177 to 209, the function FC()
calls updateAddButtonState() on line 194, but this function is not defined.
Replace this call with updateButtonStates() to fix the undefined function error
and ensure the correct button state update logic is executed.
|
i am working on a new implementation |
|
Looks promising! |
|
superseded by #4789 |
Note: This is a cleaner reimplementation of the scheduling system originally proposed in PR #4772. It replaces that version with improved structure and maintainability.
Disclaimer: This is a conceptual implementation intended to look at improved approaches to scheduling in WLED. It primarily serves as a foundation on how best to evolve or replace the existing timer system.
What this PR Does
Introduces a lightweight, modular scheduling system supporting up to 32 schedule events (vs. the legacy ~10 timer slots).
Each schedule event supports:
Time-based preset triggers (hour + minute)
Optional date ranges (start/end month and day)
Weekly repeat bitmasks for specific weekdays
Scheduling data is stored and loaded from a simple JSON file (schedule.json), making it easier to backup and edit outside the device.
The scheduler runs in the main loop, applying presets once per minute when conditions match.
Comparison to Existing Timer System
Why Some Features Are Not Included Yet
This scheduler focuses on being lightweight and efficient to make it feasible as a default built-in feature, rather than a usermod. As such, it currently:
The aim is to provide a clean, minimal core that covers the most common scheduling needs with minimal RAM and CPU overhead.
Thoughts on Combining with the Existing Timer System
Feedback on how to best handle this would be helpful.
Technical details
How it works
JSON Formating
[ { "sm": 7, "sd": 4, "em": 7, "ed": 4, "r": 0, "h": 20, "m": 30, "p": 5 }, { "sm": 0, "sd": 0, "em": 0, "ed": 0, "r": 34, "h": 7, "m": 15, "p": 2 } ]Explanation:
First entry: Runs preset 5 on July 4th at 20:30 every year.
Second entry: Runs preset 2 every Monday and Friday at 07:15.
(Repeat bitmask 34 = 0b100010 = Monday and Friday)
Summary by CodeRabbit
New Features
Bug Fixes