Add Lightweight Preset Scheduling #4772
Conversation
WalkthroughA scheduling system was implemented, allowing events to trigger presets at specified times based on a JSON schedule file. Supporting code includes a new header, schedule parsing and checking logic, and integration with the main setup and loop routines. The web interface was updated to support uploading and downloading the schedule JSON. The file upload handler was enhanced to safely update the schedule JSON by writing to a temporary file before replacing the original and immediately applying the new schedule. Changes
✨ Finishing Touches
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: 5
🧹 Nitpick comments (2)
wled00/schedule.h (1)
8-17: Consider documenting the struct fields.While the struct design is compact and memory-efficient, consider adding brief comments to clarify the purpose of each field, especially
repeatMaskwhich uses bit manipulation.struct ScheduleEvent { - uint8_t startMonth; - uint8_t startDay; - uint8_t endMonth; - uint8_t endDay; - uint8_t repeatMask; - uint8_t hour; - uint8_t minute; - uint8_t presetId; + uint8_t startMonth; // 1-12, or 0 if not used + uint8_t startDay; // 1-31 + uint8_t endMonth; // 1-12, or 0 if not used + uint8_t endDay; // 1-31 + uint8_t repeatMask; // bitmask: bit 0=Sunday, bit 1=Monday, etc. + uint8_t hour; // 0-23 + uint8_t minute; // 0-59 + uint8_t presetId; // preset to apply };wled00/schedule.cpp (1)
83-83: Consider reducing JSON buffer size for memory efficiency.A 4096-byte buffer may be excessive for the expected schedule JSON size. Consider reducing it or making it configurable based on available memory.
- DynamicJsonDocument doc(4096); + DynamicJsonDocument doc(2048); // Should be sufficient for 32 events
📜 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(2 hunks)wled00/wled.h(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
wled00/wled.cpp (1)
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.
🧬 Code Graph Analysis (2)
wled00/wled.cpp (2)
wled00/schedule.h (2)
checkSchedule(20-20)loadSchedule(19-19)wled00/schedule.cpp (4)
checkSchedule(31-73)checkSchedule(31-31)loadSchedule(75-104)loadSchedule(75-75)
wled00/schedule.cpp (1)
wled00/schedule.h (1)
loadSchedule(19-19)
🔇 Additional comments (6)
wled00/wled.h (1)
69-69: LGTM - Clean integration of scheduling header.The inclusion of
schedule.his properly placed and enables the scheduling functionality declared in the new header file.wled00/wled.cpp (2)
57-57: LGTM - Proper integration of schedule checking.The placement of
checkSchedule()afterhandleTime()ensures that the current time is available before checking scheduled events. This is the appropriate location in the main loop.
531-531: LGTM - Appropriate initialization of schedule system.Loading the schedule during setup ensures that scheduled events are available from startup. The placement is correct within the setup sequence.
wled00/data/settings_time.htm (1)
181-185: LGTM - Clean UI integration for schedule management.The new section provides necessary functionality for users to upload and download schedule JSON files. The file input restriction to
.jsonfiles and the backup download link are good UX practices.wled00/schedule.h (1)
6-6: Good choice for embedded systems.The limit of 32 schedule events strikes a good balance between functionality and memory usage for embedded devices.
wled00/schedule.cpp (1)
14-26: LGTM - Correct date range logic with year-end handling.The
isTodayInRange()function correctly handles date ranges that span across year boundaries (e.g., December to February). The logic properly distinguishes between same-year ranges and cross-year ranges.
Add error handling for the schedule.json Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wled00/schedule.cpp(1 hunks)wled00/wled_server.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wled00/schedule.cpp
🧰 Additional context used
🧠 Learnings (1)
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.
🔇 Additional comments (2)
wled00/wled_server.cpp (2)
183-200: LGTM: Good filename normalization and special case handling.The filename normalization ensures consistency, and the special handling for
/schedule.jsonusing a temporary file is a good practice for atomic updates.
223-232: LGTM: Proper handling of other file types.The logic for handling configuration files, palette files, and other uploads is well-structured and maintains existing functionality.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
wled00/schedule.cpp (1)
66-72: Fix debug statement placement.The debug statement on line 71 executes for every event regardless of whether it matches, creating misleading log output. This was flagged in previous reviews but remains unfixed.
if (match) { applyPreset(e.presetId); DEBUG_PRINTF_P(PSTR("[Schedule] Applying preset %u at %02u:%02u\n"), e.presetId, hr, min); + DEBUG_PRINTF_P(PSTR("[Schedule] Checked event %u: match=%d\n"), i, match); } - DEBUG_PRINTF_P(PSTR("[Schedule] Checked event %u: match=%d\n"), i, match);
🧹 Nitpick comments (1)
wled00/schedule.cpp (1)
86-86: Consider stack usage for embedded systems.A 4096-byte
DynamicJsonDocumenton the stack may be too large for ESP8266/ESP32 environments, potentially causing stack overflow.Consider using a smaller buffer size or allocating on heap:
- DynamicJsonDocument doc(4096); + DynamicJsonDocument doc(2048); // Smaller bufferOr validate if 4096 bytes is actually needed based on expected schedule file sizes.
📜 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(14 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- wled00/schedule.h
- wled00/wled_server.cpp
🧰 Additional context used
🧠 Learnings (1)
wled00/schedule.cpp (1)
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.
🔇 Additional comments (3)
wled00/schedule.cpp (3)
1-11: LGTM: Clean file structure and appropriate includes.The file header, includes, and global variable definitions are well-structured and appropriate for the scheduling functionality.
13-25: LGTM: Correct implementation for year-spanning date ranges.The
isTodayInRangefunction correctly handles both normal date ranges (within the same year) and year-spanning ranges (e.g., November to February). The logic properly accounts for the different comparison conditions needed for each case.
30-40: Improve anti-duplicate triggering mechanism.The current anti-duplicate logic using
thisMinute = timeinfo->tm_min + timeinfo->tm_hour * 60has a potential issue: it resets at midnight (when the calculation wraps from 1439 back to 0), which could cause missed triggers or double triggers around midnight transitions.Consider using a more robust approach:
- int thisMinute = timeinfo->tm_min + timeinfo->tm_hour * 60; + // Use a combination of day and minute to handle midnight rollover + int thisDayMinute = (timeinfo->tm_yday * 1440) + (timeinfo->tm_hour * 60) + timeinfo->tm_min;Or alternatively, store the last triggered time as a timestamp and compare against a minimum interval.
Likely an incorrect or invalid review comment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
wled00/schedule.cpp (2)
13-25: Add comments to clarify the date range logic.The
isTodayInRangefunction correctly handles both normal date ranges and ranges that span year boundaries, but the logic could benefit from clearer documentation.Consider adding comments to explain the two cases:
bool isTodayInRange(uint8_t sm, uint8_t sd, uint8_t em, uint8_t ed, uint8_t cm, uint8_t cd) { + // Case 1: Range doesn't span year boundary (e.g., March 15 to June 20) if (sm < em || (sm == em && sd <= ed)) { return (cm > sm || (cm == sm && cd >= sd)) && (cm < em || (cm == em && cd <= ed)); } + // Case 2: Range spans year boundary (e.g., November 15 to February 10) else { return (cm > sm || (cm == sm && cd >= sd)) || (cm < em || (cm == em && cd <= ed)); } }
111-117: Consider more precise day validation.The current validation allows days 1-31 for all months, which means invalid dates like February 30th would pass validation. While this may be acceptable for the scheduling use case, consider if more precise validation is needed.
If precise date validation is desired, you could add month-specific day validation:
+ // More precise day validation + uint8_t maxDays[] = {31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31}; + if (sd > maxDays[sm-1] || ed > maxDays[em-1]) { + DEBUG_PRINTF_P(PSTR("[Schedule] Invalid day for month in event %u, skipping\n"), numScheduleEvents); + continue; + }However, the current validation is likely sufficient for most practical scheduling scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/schedule.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
wled00/schedule.cpp (1)
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.
🔇 Additional comments (3)
wled00/schedule.cpp (3)
1-12: Good modular structure and clean setup.The header includes, file path definition, and global variables are properly organized and follow good practices for embedded systems.
30-72: Excellent performance optimization and duplicate prevention.The
checkSchedulefunction now correctly avoids the performance issue of loading the schedule file every minute by using a staticlastMinutevariable for duplicate prevention. The time validation and event matching logic is well-implemented.
74-130: Well-implemented loading function with proper safety measures.The
loadSchedulefunction correctly addresses all the major concerns from previous reviews:
- Uses proper JSON buffer locking for thread safety
- Validates JSON field ranges before assignment
- Has comprehensive error handling with proper cleanup
- Closes files before releasing locks
The validation ranges are appropriate for the use case.
|
thanks for contributing. |
|
Please revert all unnecessary changes and then I'll have a look. |
|
I have made a new, clean implementation and this PR is now superseded PR #4775. |
Disclaimer: I have basically no experience with doing this kind of thing, I'm just good a hooking ai up to stuff, so I was able to teach myself enough to get by.
This PR introduces a lightweight scheduling system that applies WLED presets automatically based on time and date. The goal is to enable users to automate WLED behavior without needing external automations or integrations.
Purpose
This allows users to:
It works purely offline, and is simple enough to be used by less technical users via a JSON file (as in people not running home assistant or similar).
How it works
- The schedule system loads this file on startup and... every minute (TODO: Make the load schedule only trigger on upload and startup [schedule.cpp loadSchedule() function])Now only loads on startup and upload [37a536a]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)
Testing
Limitations / Future Work
and (TODO: Make the load schedule only trigger on upload and boot [schedule.cpp loadSchedule() function])Notes for maintainers
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Enhancements