add more macro/timer slots #5140
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughReplaces fixed-size, index-based timers with a dynamic vector-backed Timer model, adds timer management APIs and migration paths, updates load/save and runtime trigger logic to use the vector, and implements a dynamic web UI to add/remove/reorder timers while exposing platform max timer limits. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas needing extra attention:
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
![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/wled.h (1)
821-829: Consider defining constants for timer array sizes.The timer arrays now use two different sizes: 18 for timerHours/Minutes/Macro/Weekday and 16 for timerMonth/Day/DayEnd. This design is intentional (sunrise/sunset at indices 16/17 don't need date ranges), but the hardcoded values throughout the codebase increase maintenance burden.
Based on learnings, consider defining constants like:
#define WLED_MAX_TIMERS 18 #define WLED_MAX_TIMER_DATE_RANGES 16This would make future expansion easier and reduce risk of inconsistencies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
wled00/cfg.cpp(4 hunks)wled00/data/settings_time.htm(2 hunks)wled00/ntp.cpp(2 hunks)wled00/set.cpp(2 hunks)wled00/wled.h(1 hunks)wled00/wled_eeprom.cpp(1 hunks)wled00/xml.cpp(1 hunks)
🧰 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/ntp.cppwled00/set.cppwled00/cfg.cppwled00/wled_eeprom.cppwled00/xml.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_time.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_time.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_time.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
🧠 Learnings (15)
📓 Common learnings
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/wled_eeprom.cpp:0-0
Timestamp: 2025-09-01T10:26:17.959Z
Learning: In WLED PR #4876, the DMXStartLED EEPROM backward compatibility issue was partially addressed by keeping it at address 2550 and reading it as a 16-bit value, with DMXChannelsValue array moved to addresses 2552-2566. This maintains compatibility with pre-0.11 EEPROM layouts for DMXStartLED, though legacy "Set to 255" (code 6) configurations may still need migration logic.
Learnt from: DedeHai
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1176-1187
Timestamp: 2025-09-16T18:08:42.848Z
Learning: In WLED finalizeInit() bus creation (wled00/FX_fcn.cpp), intentionally allowing memory overruns when bus configurations exceed MAX_LED_MEMORY is a deliberate design choice. The trade-off prioritizes creating buses with reduced LED counts over completely failing to create buses, which would cause no LED output and UI failures. This approach forces users to update configurations after migrating to version 0.16 while maintaining basic functionality.
Learnt from: blazoncek
Repo: wled/WLED PR: 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: DedeHai
Repo: wled/WLED PR: 4926
File: wled00/FX.cpp:4727-4730
Timestamp: 2025-11-16T19:40:46.260Z
Learning: WLED AuroraWave (wled00/FX.cpp): wave_start and wave_end intentionally use int16_t; segments longer than 32k LEDs are not supported (bounded by MAX_LEDS), so widening to 32-bit is unnecessary.
Learnt from: DedeHai
Repo: wled/WLED PR: 4793
File: wled00/file.cpp:481-513
Timestamp: 2025-07-31T18:21:49.868Z
Learning: In WLED, essential configuration files that require backup have short, controlled names (like "/cfg.json", "/presets.json") that are well within a 32-character buffer limit. The file naming is controlled by developers, making buffer overflow in backup filename construction highly unlikely.
Learnt from: DedeHai
Repo: wled/WLED PR: 4923
File: wled00/FX.cpp:4883-4901
Timestamp: 2025-09-12T17:29:43.826Z
Learning: In WLED’s web UI, only one slider value (e.g., SEGMENT.intensity or SEGMENT.custom1) changes at a time; code relying on this may use simplified change guards, though presets/JSON can still update multiple fields atomically.
Learnt from: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.
📚 Learning: 2025-11-16T19:40:46.260Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4926
File: wled00/FX.cpp:4727-4730
Timestamp: 2025-11-16T19:40:46.260Z
Learning: WLED AuroraWave (wled00/FX.cpp): wave_start and wave_end intentionally use int16_t; segments longer than 32k LEDs are not supported (bounded by MAX_LEDS), so widening to 32-bit is unnecessary.
Applied to files:
wled00/cfg.cppwled00/wled.h
📚 Learning: 2025-10-10T18:34:06.550Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.
Applied to files:
wled00/cfg.cppwled00/wled.h
📚 Learning: 2025-09-01T10:26:17.959Z
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/wled_eeprom.cpp:0-0
Timestamp: 2025-09-01T10:26:17.959Z
Learning: In WLED PR #4876, the DMXStartLED EEPROM backward compatibility issue was partially addressed by keeping it at address 2550 and reading it as a 16-bit value, with DMXChannelsValue array moved to addresses 2552-2566. This maintains compatibility with pre-0.11 EEPROM layouts for DMXStartLED, though legacy "Set to 255" (code 6) configurations may still need migration logic.
Applied to files:
wled00/wled_eeprom.cppwled00/wled.h
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/html_*.h : DO NOT edit generated embedded web header files (wled00/html_*.h)
Applied to files:
wled00/wled_eeprom.cppwled00/wled.h
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Applied to files:
wled00/wled.h
📚 Learning: 2025-11-14T13:37:11.994Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Applied to files:
wled00/wled.h
📚 Learning: 2025-08-28T08:09:20.630Z
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Applied to files:
wled00/wled.h
📚 Learning: 2025-08-31T03:38:14.114Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.114Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Applied to files:
wled00/wled.h
📚 Learning: 2025-04-26T12:51:04.993Z
Learnt from: KrX3D
Repo: wled/WLED 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.
Applied to files:
wled00/wled.h
📚 Learning: 2025-08-29T00:26:15.808Z
Learnt from: ksedgwic
Repo: wled/WLED PR: 4883
File: usermods/usermod_v2_skystrip/rest_json_client.h:6-14
Timestamp: 2025-08-29T00:26:15.808Z
Learning: WLED uses a vendored ArduinoJson library (version 6) located at "src/dependencies/json/ArduinoJson-v6.h" which is included through wled.h. Usermods should not directly include ArduinoJson headers but instead rely on wled.h for ArduinoJson symbols. The standard pattern is to include wled.h and use JsonObject, JsonArray, DynamicJsonDocument, etc. without additional includes.
Applied to files:
wled00/wled.h
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/data/** : For web UI changes, edit files only under wled00/data (not firmware or generated files)
Applied to files:
wled00/wled.h
📚 Learning: 2025-09-02T19:42:57.475Z
Learnt from: freakintoddles2
Repo: wled/WLED PR: 4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Applied to files:
wled00/wled.h
📚 Learning: 2025-09-16T18:08:42.848Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1176-1187
Timestamp: 2025-09-16T18:08:42.848Z
Learning: In WLED finalizeInit() bus creation (wled00/FX_fcn.cpp), intentionally allowing memory overruns when bus configurations exceed MAX_LED_MEMORY is a deliberate design choice. The trade-off prioritizes creating buses with reduced LED counts over completely failing to create buses, which would cause no LED output and UI failures. This approach forces users to update configurations after migrating to version 0.16 while maintaining basic functionality.
Applied to files:
wled00/wled.h
📚 Learning: 2025-06-27T06:04:44.652Z
Learnt from: DedeHai
Repo: wled/WLED PR: 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.
Applied to files:
wled00/wled.h
🧬 Code graph analysis (2)
wled00/ntp.cpp (1)
wled00/src/dependencies/time/Time.cpp (4)
hour(49-52)hour(49-49)minute(72-75)minute(72-72)
wled00/xml.cpp (1)
wled00/util.cpp (4)
printSetFormValue(114-116)printSetFormValue(114-114)printSetFormValue(121-123)printSetFormValue(121-121)
⏰ 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). (13)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32c3dev_qio)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32S3_wroom2)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
🔇 Additional comments (9)
wled00/wled_eeprom.cpp (1)
240-246: LGTM - Consistent initialization for extended timer range.The new loop correctly initializes timers 8-17 by clearing the enabled bit when no macro is set, mirroring the existing logic for timers 0-7 at line 238. This ensures consistent state for timers that are stored in cfg.json rather than EEPROM.
wled00/set.cpp (1)
548-577: LGTM - Timer parsing correctly expanded to 18 entries.The key construction logic properly handles both single-digit (0-9) and double-digit (10-17) indices:
- Buffer size increased to 5 characters to accommodate "H10" through "H17"
- Null terminator placement is correct for both cases
- Month/Day processing correctly limited to i<16 since sunrise/sunset (indices 16/17) don't use date ranges
wled00/ntp.cpp (2)
384-396: LGTM - Timer loop correctly expanded to 16 entries.The regular timer processing loop now handles indices 0-15, matching the size of the timerMonth/timerDay/timerDayEnd arrays. The date range check at line 391 correctly accesses these arrays within bounds.
398-423: LGTM - Sunrise/sunset macros relocated to indices 16/17.The sunrise and sunset macro handling has been moved from indices 8/9 to 16/17, consistent with the expanded timer array design. This maintains backward compatibility for the 8 new regular timer slots (8-15).
wled00/xml.cpp (1)
588-610: LGTM - Timer value rendering correctly extended.The key construction matches set.cpp logic. Hours are correctly limited to i<16 since indices 16/17 (sunrise/sunset) use a fixed value of 255 set by hidden HTML inputs. Month/Day/DayEnd rendering is also correctly limited to i<16.
wled00/cfg.cpp (2)
685-718: Verify sunrise/sunset JSON parsing behavior.The logic at line 686 jumps the iterator to 16 when encountering
hour==255, which marks sunrise/sunset entries. This assumes:
- Regular timers (0-15) always appear before sunrise/sunset in JSON
- Sunrise/sunset entries don't have date ranges
This works for configs generated by WLED itself, but malformed external JSON with out-of-order entries could cause issues. Consider adding bounds checking or documenting the expected JSON format.
The post-processing cleanup loop (lines 716-718) is a good defensive addition that ensures consistency regardless of input.
1215-1231: LGTM - Serialization correctly extended to 18 timers.The output loop correctly iterates over all 18 timer indices. Date range objects are only created for indices 0-15, matching the array sizes for timerMonth/timerDay/timerDayEnd.
wled00/data/settings_time.htm (2)
30-61: LGTM - Timer UI correctly expanded.The BTa function now renders 16 regular timer rows (indices 0-15) plus dedicated sunrise/sunset rows at indices 16/17. The sunrise/sunset rows correctly use hidden inputs with value="255" for the hour field.
78-85: LGTM - Form serialization correctly extended.The Wd function now processes all 18 timer entries, correctly serializing the weekday checkbox states to hidden inputs before form submission.
|
appreciate your effort. without looking too much into this: doesn't your update break sunrise/sunset configurations of existing setups? |
|
FYI there was a PR that implemented entirely new approach to timed presets. It lacked UI though. |
|
So to clarify... Yes, this current implementation does break sunrise/sunset configs as it currently stands. It can be modified to preserve those, and just skip those 2 for regular macro configs. I'm not sure what is different about timed presets, but I can certainly give those a look. A truly "final" version of the macro system would be to allow users to select sunrise/sunset for any macro, and allow them to add more macros at will, making them basically unlimited. I know there are issues with strongly typed languages not being happy with open-ended arrays. Although converting to vectors may make this a lot easier as there will only be one open-ended array. But if that seems like a good idea, I can certainly look into doing that. I'm going to try converting to vectors tonight and see how that pans out with open-ended macros. |
|
I took a look at #4789 and that's basically the same. I can implement those changes here, or extend that PR, or both. I'll try my previous change here (vectors in an open-ended array), and pull down that PR and see how it compares locally and see how they are different. I'll also implement a UI that expands as needed, which I thought of doing, but didn't want to get too crazy in a proof of concept. |
|
Keep in mind RAM constrained ESP8266. |
|
Ok, I have completely rewritten this using the ideas from that other PR as well as my own ideas. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
wled00/const.h (1)
56-86: WLED_MAX_TIMERS choices are reasonable and match the new vector pathPer‑platform limits (16 on ESP8266, 64 on ESP32‑class) are a sensible balance between flexibility and RAM usage, and they’re properly wired into parsing/serialization elsewhere. If we ever raise
WLED_MAX_TIMERSabove 99, we’ll need to revisit the timer field‑name construction inset.cpp(it currently assumes at most two decimal digits).wled00/xml.cpp (1)
588-605: Timer UI serialization matches the new Timer layout but depends on JS decodingweekdayscorrectlyThe SUBPAGE_TIME block now cleanly reflects the vector model and
WLED_MAX_TIMERS, and emittingaddTimerRow(...)fromtimersis consistent with the JSON encoding (packedweekdaysbitfield plus full date range). Please double‑check thataddTimerRowinsettings_time.htminterprets theweekdaysargument as “bit0 = enabled, bits1–7 = DOW mask” to avoid mismatches between stored state and the checkboxes.wled00/set.cpp (1)
548-612: Dynamic timer parsing is sound; consider guarding against minute overflow and future index growthThe new loop correctly:
- Clears existing timers, walks indices 0…
WLED_MAX_TIMERS-1, and skips deleted rows via theT*presence check.- Reconstructs the packed weekdays byte and date range, delegating validation to
addTimerand compacting afterwards.Two small improvement points:
- Read
minuteinto a wider type (e.g.int minuteVal = request->arg(k).toInt();then cast toint8_twhen callingaddTimer) to avoid silent wrap if someone posts out‑of‑range values outside the UI.- The index→name mapping assumes at most two decimal digits; if
WLED_MAX_TIMERSis ever raised above 99, switching tosnprintf(k, sizeof(k), "T%d", ti)(and similarly for the other fields) would future‑proof this.wled00/cfg.cpp (1)
1226-1251: Vector-based timer serialization is consistent and avoids emitting empty entriesThe
serializeConfigside now:
- Iterates
::timersand skips pure “empty” timers (preset==0 && hour==0 && minute==0).- Writes
en,hour,min,macro, anddow(asweekdays >> 1) plus date range only forisRegular()timers.This matches the deserialization logic and keeps the on-disk representation compact while preserving disabled-but-configured timers (preset 0, nonzero time,
en=0). That’s a good balance between correctness and size.wled00/ntp.cpp (3)
581-584: Reconsider preset 0 validation logic.The validation skips timers with
preset == 0 && (weekdays & 0x01), but per learnings, a timer with preset 0 has no effect regardless of the enabled flag. This check may be overly strict and could skip legitimate disabled timers that should be migrated.Consider either:
- Allowing these timers through (they're harmless since preset 0 has no effect)
- Or documenting why they should be explicitly skipped during migration
Based on learnings: Timer with preset 0 is effectively disabled regardless of enable bit.
697-704: Sunrise/sunset migration may skip valid timers.The migration checks
timerMacro[8] != 0 || timerMinutes[8] != 0, but a sunrise/sunset timer with both values at 0 but valid weekdays could still be intentional (e.g., trigger at exact sunrise/sunset time on specific days). Though rare, this could skip valid timer configurations.Consider also checking weekdays:
- if (timerMacro[8] != 0 || timerMinutes[8] != 0) { + if (timerMacro[8] != 0 || timerMinutes[8] != 0 || timerWeekday[8] != 0) { addTimer(timerMacro[8], TIMER_HOUR_SUNRISE, timerMinutes[8], timerWeekday[8]); } - if (timerMacro[9] != 0 || timerMinutes[9] != 0) { + if (timerMacro[9] != 0 || timerMinutes[9] != 0 || timerWeekday[9] != 0) { addTimer(timerMacro[9], TIMER_HOUR_SUNSET, timerMinutes[9], timerWeekday[9]); }
654-666: compactTimers() logic may need weekdays check.The function removes timers where
preset == 0 && hour == 0 && minute == 0, but doesn't check weekdays. A timer with weekdays set but all other fields at 0 would not be removed, potentially leaving partially-configured timers in the vector.Consider including weekdays in the empty timer check:
for (size_t i = 0; i < timers.size();) { const Timer& t = timers[i]; - if (t.preset == 0 && t.hour == 0 && t.minute == 0) { + if (t.preset == 0 && t.hour == 0 && t.minute == 0 && t.weekdays == 0) { timers.erase(timers.begin() + i); } else { ++i; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
wled00/cfg.cpp(2 hunks)wled00/const.h(2 hunks)wled00/data/settings_time.htm(3 hunks)wled00/fcn_declare.h(1 hunks)wled00/ntp.cpp(4 hunks)wled00/set.cpp(3 hunks)wled00/wled.h(2 hunks)wled00/wled_eeprom.cpp(1 hunks)wled00/xml.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- wled00/wled_eeprom.cpp
🧰 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.cppwled00/ntp.cpp
wled00/**/!(html_*)*.h
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for non-generated C++ header files (.h)
Files:
wled00/const.hwled00/fcn_declare.hwled00/wled.h
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_time.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_time.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_time.htm
🧠 Learnings (26)
📓 Common learnings
Learnt from: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.913Z
Learning: In WLED PR #5134, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Learnt from: DedeHai
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1176-1187
Timestamp: 2025-09-16T18:08:42.848Z
Learning: In WLED finalizeInit() bus creation (wled00/FX_fcn.cpp), intentionally allowing memory overruns when bus configurations exceed MAX_LED_MEMORY is a deliberate design choice. The trade-off prioritizes creating buses with reduced LED counts over completely failing to create buses, which would cause no LED output and UI failures. This approach forces users to update configurations after migrating to version 0.16 while maintaining basic functionality.
📚 Learning: 2025-12-01T07:01:16.913Z
Learnt from: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.913Z
Learning: In WLED PR #5134, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
Applied to files:
wled00/set.cppwled00/const.hwled00/xml.cppwled00/data/settings_time.htmwled00/fcn_declare.hwled00/wled.hwled00/cfg.cppwled00/ntp.cpp
📚 Learning: 2025-10-10T18:34:06.550Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.
Applied to files:
wled00/set.cppwled00/wled.h
📚 Learning: 2025-08-28T08:09:20.630Z
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Applied to files:
wled00/set.cppwled00/const.hwled00/data/settings_time.htmwled00/wled.h
📚 Learning: 2025-11-14T13:37:11.994Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Applied to files:
wled00/set.cppwled00/const.hwled00/data/settings_time.htmwled00/wled.h
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Applied to files:
wled00/set.cppwled00/const.hwled00/data/settings_time.htmwled00/wled.h
📚 Learning: 2025-12-01T07:01:16.914Z
Learnt from: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.914Z
Learning: In WLED timer system, even if a timer is enabled (enable bit set in timerWeekday), it will have no effect if the preset selected (timerMacro) is 0, because there can never be a preset with ID 0.
Applied to files:
wled00/set.cppwled00/data/settings_time.htmwled00/wled.hwled00/ntp.cpp
📚 Learning: 2025-11-16T19:40:46.260Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4926
File: wled00/FX.cpp:4727-4730
Timestamp: 2025-11-16T19:40:46.260Z
Learning: WLED AuroraWave (wled00/FX.cpp): wave_start and wave_end intentionally use int16_t; segments longer than 32k LEDs are not supported (bounded by MAX_LEDS), so widening to 32-bit is unnecessary.
Applied to files:
wled00/set.cppwled00/const.hwled00/wled.h
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/html_*.h : DO NOT edit generated embedded web header files (wled00/html_*.h)
Applied to files:
wled00/set.cppwled00/const.hwled00/fcn_declare.hwled00/wled.h
📚 Learning: 2025-11-30T15:29:00.711Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4456
File: usermods/deep_sleep/deep_sleep.cpp:224-230
Timestamp: 2025-11-30T15:29:00.711Z
Learning: In WLED, `touchThreshold` is a global variable defined in wled00/wled.h and is accessible to all usermods. It stores the touch sensitivity threshold for touch-capable GPIO pins on ESP32 variants and should not be flagged as undefined when used in usermods.
Applied to files:
wled00/set.cpp
📚 Learning: 2025-04-26T19:19:07.600Z
Learnt from: blazoncek
Repo: wled/WLED PR: 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.
Applied to files:
wled00/const.h
📚 Learning: 2025-02-19T12:43:34.200Z
Learnt from: blazoncek
Repo: wled/WLED 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.
Applied to files:
wled00/const.h
📚 Learning: 2025-09-16T18:08:42.848Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1176-1187
Timestamp: 2025-09-16T18:08:42.848Z
Learning: In WLED finalizeInit() bus creation (wled00/FX_fcn.cpp), intentionally allowing memory overruns when bus configurations exceed MAX_LED_MEMORY is a deliberate design choice. The trade-off prioritizes creating buses with reduced LED counts over completely failing to create buses, which would cause no LED output and UI failures. This approach forces users to update configurations after migrating to version 0.16 while maintaining basic functionality.
Applied to files:
wled00/const.hwled00/data/settings_time.htm
📚 Learning: 2025-02-19T12:43:34.199Z
Learnt from: blazoncek
Repo: wled/WLED 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.
Applied to files:
wled00/const.h
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
Repo: wled/WLED PR: 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.
Applied to files:
wled00/const.h
📚 Learning: 2025-10-20T09:38:51.997Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4995
File: wled00/FX.cpp:5223-5226
Timestamp: 2025-10-20T09:38:51.997Z
Learning: WLED matrices: each dimension (SEG_W, SEG_H) is limited to ≤255; 256 or larger per side is not supported/feasible on ESP32, so effects should assume per-dimension max 255.
Applied to files:
wled00/const.h
📚 Learning: 2025-09-18T03:17:30.107Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1183-1194
Timestamp: 2025-09-18T03:17:30.107Z
Learning: In WLED finalizeInit() bus creation (wled00/FX_fcn.cpp), digitalCount is intentionally incremented even for placeholder buses to maintain consistent resource mapping across all bus configurations.
Applied to files:
wled00/const.h
📚 Learning: 2025-09-21T20:44:09.502Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4928
File: wled00/bus_wrapper.h:1248-1275
Timestamp: 2025-09-21T20:44:09.502Z
Learning: In WLED bus_wrapper.h, the _useParallelI2S flag is used for selecting the correct function pointer (between parallel I2S and non-parallel I2S bus types) rather than for changing memory calculations. Both parallel and non-parallel I2S configurations have the same memory requirements, so memUsage() correctly doesn't differentiate based on this flag.
Applied to files:
wled00/const.h
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/data/settings_time.htm
📚 Learning: 2025-11-27T06:33:11.436Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5109
File: wled00/FX.cpp:3174-3343
Timestamp: 2025-11-27T06:33:11.436Z
Learning: WLED Ants effect (wled00/FX.cpp): The author prefers the current velocity initialization using hw_random16(1000, 5000)/5000.0f, resulting in an effective range of ~3.6–10.0 (with VELOCITY_MIN=2.0, VELOCITY_MAX=10.0), and wants the code kept as-is with comments updated to document this behavior. Avoid suggesting changes to span the full 2.0–10.0 range in future reviews.
Applied to files:
wled00/data/settings_time.htm
📚 Learning: 2025-09-02T19:42:57.475Z
Learnt from: freakintoddles2
Repo: wled/WLED PR: 4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Applied to files:
wled00/data/settings_time.htmwled00/wled.h
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, verify that file operations (especially file.open()) respect LittleFS filename limitations. Assume default WLED configuration with LittleFS default filename limit of 255 bytes. Do not assume extreme configuration values like WLED_MAX_SEGNAME_LEN = 512 which would not be standard configurations.
Applied to files:
wled00/data/settings_time.htm
📚 Learning: 2025-09-18T03:17:30.107Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1183-1194
Timestamp: 2025-09-18T03:17:30.107Z
Learning: In WLED bus management code, dead code branches in error handling (like unreachable else breaks) are intentionally preserved to accommodate potential future API changes, even when currently unreachable.
Applied to files:
wled00/data/settings_time.htm
📚 Learning: 2025-11-14T13:37:11.994Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, file operations (especially file.open()) should be checked to ensure they respect LittleFS filename limitations. The default LittleFS filename limit is 255 bytes (LFS_NAME_MAX). Reviews should assume default WLED configuration defines and not extreme edge-case values (e.g., WLED_MAX_SEGNAME_LEN = 512 would not be standard). File paths should be validated to stay within the 255-byte limit.
Applied to files:
wled00/data/settings_time.htm
📚 Learning: 2025-08-31T03:38:14.114Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.114Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Applied to files:
wled00/wled.h
📚 Learning: 2025-09-01T10:26:17.959Z
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/wled_eeprom.cpp:0-0
Timestamp: 2025-09-01T10:26:17.959Z
Learning: In WLED PR #4876, the DMXStartLED EEPROM backward compatibility issue was partially addressed by keeping it at address 2550 and reading it as a 16-bit value, with DMXChannelsValue array moved to addresses 2552-2566. This maintains compatibility with pre-0.11 EEPROM layouts for DMXStartLED, though legacy "Set to 255" (code 6) configurations may still need migration logic.
Applied to files:
wled00/wled.h
🧬 Code graph analysis (4)
wled00/set.cpp (1)
wled00/ntp.cpp (6)
clearTimers(643-646)clearTimers(643-643)addTimer(577-632)addTimer(577-579)compactTimers(654-666)compactTimers(654-654)
wled00/fcn_declare.h (1)
wled00/ntp.cpp (12)
addTimer(577-632)addTimer(577-579)removeTimer(635-640)removeTimer(635-635)clearTimers(643-646)clearTimers(643-643)getTimerCount(649-651)getTimerCount(649-649)compactTimers(654-666)compactTimers(654-654)migrateTimersFromArrays(672-723)migrateTimersFromArrays(672-672)
wled00/cfg.cpp (1)
wled00/ntp.cpp (6)
clearTimers(643-646)clearTimers(643-643)addTimer(577-632)addTimer(577-579)migrateTimersFromArrays(672-723)migrateTimersFromArrays(672-672)
wled00/ntp.cpp (2)
wled00/src/dependencies/time/Time.cpp (4)
hour(49-52)hour(49-49)minute(72-75)minute(72-72)wled00/presets.cpp (2)
applyPreset(127-134)applyPreset(127-127)
⏰ 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). (20)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32S3_wroom2)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32c3dev_qio)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp32dev_debug)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
🔇 Additional comments (10)
wled00/fcn_declare.h (1)
207-247: Timer struct and constants look consistent with the new timer modelThe hour sentinels, weekday encoding, and helper methods align with the addTimer/checkTimers logic and give a compact 8‑byte representation without obvious correctness issues. Using
isEnabled()to gate on both enable bit andpreset != 0is a good fit for the existing “preset 0 is inert” semantics.wled00/set.cpp (1)
300-310: Comment-only change around touch buttons; logic unchangedThe SOC_TOUCH_VERSION_2 block still preserves the intended control flow (
elseguarded by#ifdef) and behavior for S2/S3 touch pins; only the comment text changed. No functional impact.wled00/wled.h (1)
819-835: Global timer state split (vector + legacy arrays) is reasonable; confirm remaining array users are migration-onlyIntroducing
std::vector<Timer> timersalongsidelastTimerMinutematches the new dynamic timer design, and keeping the legacytimer*arrays with explicit “migration” comments is a good way to preserve backward compatibility.Please verify that:
- All runtime scheduling logic (e.g.
checkTimers, NTP triggers) now uses thetimersvector exclusively, and- The legacy arrays are only read by migration/EEPROM code paths (
migrateTimersFromArraysetc.), so they can’t drift out of sync with the vector during normal operation.This will avoid subtle bugs where an old code path still reads the arrays after migration.
wled00/cfg.cpp (1)
682-731: Timer deserialization logic is clean for the new format; confirm legacy upgrade path handles all existing configsThe new
timers.inshandling correctly rebuilds the vector with hour/min/macro, packed weekdays, and optional date ranges, delegating validation toaddTimer.However, the legacy fallback assumes
timerMacro/Hours/Minutesare already populated beforedeserializeConfig()runs. Since those arrays are no longer filled from JSON in this file, on upgrade from older firmware that only persisted timers viacfg.json,hasLegacyDatamay stay false and existing timers could silently disappear.Confirm that for all supported upgrade paths, either:
- Old configs already use the new
timers.insshape being parsed here, or- The legacy arrays are populated from EEPROM or an earlier JSON reader before this deserializer runs.
If neither applies, an explicit decoder for the previous JSON timer layout is needed instead of relying solely on the array fallback. Existing timer behavior across upgrades is critical to preserve.
wled00/ntp.cpp (1)
384-444: Vector-based timer iteration implemented correctly.The refactored timer checking logic properly handles sunrise/sunset/regular timer types, applies offset-based triggering for sun-based timers, and correctly filters by weekday and date range.
wled00/data/settings_time.htm (5)
23-30: Clean refactor to classList-based visibility toggle.The updated expand() function correctly uses CSS classes for showing/hiding timer details, improving maintainability.
144-176: Timer type switching implemented correctly.The function properly toggles UI visibility and validation ranges when switching between regular and sunrise/sunset timer types.
322-353: CSS styling for new timer UI looks good.The styles properly support the dynamic timer list with appropriate visual feedback for deleted timers, reordering controls, and visibility states.
437-441: Clear UI controls and documentation.The "Add Timer" button and updated help text provide good user guidance for the new timer interface.
249-254: Verify server initializes timers via addTimerRow() calls.The FC() function is now a placeholder that relies on the server to populate timers by calling
addTimerRow()for each timer during settings script execution. Confirm the settings script (loaded at line 13) contains the necessary timer initialization logic.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
wled00/data/settings_time.htm (1)
218-252: Timer reordering now works with submission.The reordering functions correctly manipulate DOM order, and the updated
Wd()function reindexes form fields based on visual DOM order before submission. This addresses the previous review concern about reordering not affecting submission order.
🧹 Nitpick comments (2)
wled00/data/settings_time.htm (1)
33-34: Consider using a defined constant for maxTimers.The
maxTimersvariable defaults to 64, but the comment says it will be overridden by the server. This value should matchWLED_MAX_TIMERSdefined in the backend. Consider documenting or validating this linkage.Based on learnings, magic numbers should be replaced with meaningful constants where possible.
-var maxTimers = 64; // Default, will be overridden by server +var maxTimers = 64; // Default, should match WLED_MAX_TIMERS from serverAlso verify the server actually sends this value to override the default.
wled00/ntp.cpp (1)
699-709: Sunrise/sunset migration includes weekday-only timers.The conditions
timerWeekday[8] != 0andtimerWeekday[9] != 0will migrate timers that have the enable bit set but no preset. Per learnings, these timers have no effect since preset 0 doesn't exist. This is harmless but may create unnecessary timer entries.Consider tightening the condition to only migrate meaningful timers:
// Migrate sunrise timer (index 8) - // Include weekdays check to catch timers with only weekday configuration - if (timerMacro[8] != 0 || timerMinutes[8] != 0 || timerWeekday[8] != 0) { + // Only migrate if there's a preset or offset configured + if (timerMacro[8] != 0 || timerMinutes[8] != 0) { addTimer(timerMacro[8], TIMER_HOUR_SUNRISE, timerMinutes[8], timerWeekday[8]); } // Migrate sunset timer (index 9) - // Include weekdays check to catch timers with only weekday configuration - if (timerMacro[9] != 0 || timerMinutes[9] != 0 || timerWeekday[9] != 0) { + // Only migrate if there's a preset or offset configured + if (timerMacro[9] != 0 || timerMinutes[9] != 0) { addTimer(timerMacro[9], TIMER_HOUR_SUNSET, timerMinutes[9], timerWeekday[9]); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
wled00/cfg.cpp(2 hunks)wled00/data/settings_time.htm(3 hunks)wled00/ntp.cpp(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
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_time.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_time.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_time.htm
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/ntp.cppwled00/cfg.cpp
🧠 Learnings (15)
📓 Common learnings
Learnt from: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.913Z
Learning: In WLED PR #5134, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
📚 Learning: 2025-12-01T07:01:16.913Z
Learnt from: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.913Z
Learning: In WLED PR #5134, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
Applied to files:
wled00/data/settings_time.htmwled00/ntp.cppwled00/cfg.cpp
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/html_*.h : DO NOT edit generated embedded web header files (wled00/html_*.h)
Applied to files:
wled00/data/settings_time.htm
📚 Learning: 2025-12-01T07:01:16.914Z
Learnt from: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.914Z
Learning: In WLED timer system, even if a timer is enabled (enable bit set in timerWeekday), it will have no effect if the preset selected (timerMacro) is 0, because there can never be a preset with ID 0.
Applied to files:
wled00/data/settings_time.htmwled00/ntp.cpp
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/data/settings_time.htm
📚 Learning: 2025-11-14T13:37:11.994Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Applied to files:
wled00/data/settings_time.htmwled00/ntp.cpp
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Applied to files:
wled00/data/settings_time.htm
📚 Learning: 2025-08-28T08:09:20.630Z
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Applied to files:
wled00/data/settings_time.htm
📚 Learning: 2025-10-10T18:34:06.550Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.
Applied to files:
wled00/data/settings_time.htmwled00/ntp.cpp
📚 Learning: 2025-09-02T19:42:57.475Z
Learnt from: freakintoddles2
Repo: wled/WLED PR: 4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Applied to files:
wled00/data/settings_time.htm
📚 Learning: 2025-11-27T06:33:11.436Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5109
File: wled00/FX.cpp:3174-3343
Timestamp: 2025-11-27T06:33:11.436Z
Learning: WLED Ants effect (wled00/FX.cpp): The author prefers the current velocity initialization using hw_random16(1000, 5000)/5000.0f, resulting in an effective range of ~3.6–10.0 (with VELOCITY_MIN=2.0, VELOCITY_MAX=10.0), and wants the code kept as-is with comments updated to document this behavior. Avoid suggesting changes to span the full 2.0–10.0 range in future reviews.
Applied to files:
wled00/data/settings_time.htm
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, verify that file operations (especially file.open()) respect LittleFS filename limitations. Assume default WLED configuration with LittleFS default filename limit of 255 bytes. Do not assume extreme configuration values like WLED_MAX_SEGNAME_LEN = 512 which would not be standard configurations.
Applied to files:
wled00/data/settings_time.htm
📚 Learning: 2025-09-18T03:17:30.107Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1183-1194
Timestamp: 2025-09-18T03:17:30.107Z
Learning: In WLED bus management code, dead code branches in error handling (like unreachable else breaks) are intentionally preserved to accommodate potential future API changes, even when currently unreachable.
Applied to files:
wled00/data/settings_time.htm
📚 Learning: 2025-11-14T13:37:11.994Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, file operations (especially file.open()) should be checked to ensure they respect LittleFS filename limitations. The default LittleFS filename limit is 255 bytes (LFS_NAME_MAX). Reviews should assume default WLED configuration defines and not extreme edge-case values (e.g., WLED_MAX_SEGNAME_LEN = 512 would not be standard). File paths should be validated to stay within the 255-byte limit.
Applied to files:
wled00/data/settings_time.htm
📚 Learning: 2025-09-16T18:08:42.848Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1176-1187
Timestamp: 2025-09-16T18:08:42.848Z
Learning: In WLED finalizeInit() bus creation (wled00/FX_fcn.cpp), intentionally allowing memory overruns when bus configurations exceed MAX_LED_MEMORY is a deliberate design choice. The trade-off prioritizes creating buses with reduced LED counts over completely failing to create buses, which would cause no LED output and UI failures. This approach forces users to update configurations after migrating to version 0.16 while maintaining basic functionality.
Applied to files:
wled00/data/settings_time.htm
🧬 Code graph analysis (2)
wled00/ntp.cpp (1)
wled00/src/dependencies/time/Time.cpp (4)
hour(49-52)hour(49-49)minute(72-75)minute(72-72)
wled00/cfg.cpp (1)
wled00/ntp.cpp (6)
clearTimers(642-645)clearTimers(642-642)addTimer(577-631)addTimer(577-579)migrateTimersFromArrays(675-728)migrateTimersFromArrays(675-675)
⏰ 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). (14)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32c3dev_qio)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32S3_wroom2)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32dev_debug)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
🔇 Additional comments (10)
wled00/cfg.cpp (3)
682-716: New format timer deserialization looks correct.The logic properly:
- Clears existing timers before loading
- Extracts enabled bit and weekdays from the
dowfield- Handles sunrise/sunset vs regular timer distinction via hour check against
TIMER_HOUR_SUNSET- Delegates validation to
addTimer()
1280-1303: Serialization logic is consistent with deserialization.The code correctly:
- Uses
::timersto disambiguate from localtimersJSON object- Skips meaningless timers (preset=0, hour=0, minute=0)
- Only serializes date range for regular timers via
timer.isRegular()
768-782: Legacy array migration fallback is appropriate.The code correctly checks all 10 legacy array indices for any meaningful data before triggering migration. This ensures existing configurations are preserved during the upgrade path.
wled00/data/settings_time.htm (2)
178-216: Timer deletion logic correctly handles count and submission.The implementation now:
- Decrements
timerCountto allow adding new timers (addressing past review feedback)- Sets field values to 0 to mark as deleted
- Uses CSS class to visually indicate deletion
- Wd() properly skips deleted rows during submission
42-142: Timer row creation handles all timer types correctly.The function properly:
- Validates against
maxTimerslimit- Distinguishes sunrise (255), sunset (254), and regular timers
- Correctly extracts enabled bit (bit 0) and weekdays (bits 1-7)
- Hides hour input and date range for sunrise/sunset timers
- Creates proper form field names for backend processing
wled00/ntp.cpp (5)
384-445: Timer checking logic is well-structured.The refactored
checkTimers()correctly:
- Iterates the vector-based timers
- Checks enabled state before processing
- Handles sunrise/sunset timers with offset calculation
- Validates weekday and date range before triggering
- Applies preset via
applyPreset()The debug output provides good visibility into timer operations.
605-617: Month/day validation correctly handles legacy 0 values.The validation now accepts 0 as a valid value for months and days (representing "use default"), which addresses the previous review concern about migration failures. The actual default values (monthStart=1, monthEnd=12, etc.) are applied in
isTodayInDateRange()at lines 350-352.
652-669: Timer compaction logic is reasonable.The function removes timers that have no meaningful configuration (no preset, no time) and whose weekdays are either all cleared (0) or all set (255, the default). The
shrink_to_fit()call is appropriate for ESP8266/ESP32 memory management.
234-238: Formatting adjustment is acceptable.This is a minor whitespace change in the NTP response validation that doesn't affect functionality.
500-500: Good use of named constant for sunset validation.Replacing the magic number with
SUNSET_MAXimproves code readability and maintainability. Based on learnings, this is the preferred approach for numeric literals with domain meaning.
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 (1)
wled00/cfg.cpp (1)
744-744: Default weekdays value may incorrectly enable legacy timers.The default
weekdays = 255sets all bits including bit 0 (enabled flag). For legacy timers wheretimerWeekdayJsonis null or missing, this will mark them as enabled even if they weren't originally.Consider defaulting to a value that preserves the legacy "all days" semantic without auto-enabling:
- uint8_t weekdays = timerWeekdayJson.isNull() ? 255 : (timerWeekdayJson[i] | 255); + // Legacy format: dow in bits 1-7, enabled bit needs to be set explicitly + // Default to all days (0xFE) without enabled flag, then set enabled based on having a preset + uint8_t dow = timerWeekdayJson.isNull() ? 127 : (timerWeekdayJson[i] | 127); + uint8_t weekdays = (dow << 1) | ((preset != 0) ? 1 : 0);Alternatively, verify the legacy format's weekday encoding to ensure this default is appropriate. Based on learnings, existing configurations should work correctly after migration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/cfg.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/cfg.cpp
🧠 Learnings (6)
📓 Common learnings
Learnt from: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.913Z
Learning: In WLED PR #5134, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
📚 Learning: 2025-12-01T07:01:16.913Z
Learnt from: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.913Z
Learning: In WLED PR #5134, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
Applied to files:
wled00/cfg.cpp
📚 Learning: 2025-11-14T13:37:11.994Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Applied to files:
wled00/cfg.cpp
📚 Learning: 2025-08-28T08:09:20.630Z
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Applied to files:
wled00/cfg.cpp
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Applied to files:
wled00/cfg.cpp
📚 Learning: 2025-10-10T18:34:06.550Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.
Applied to files:
wled00/cfg.cpp
🧬 Code graph analysis (1)
wled00/cfg.cpp (1)
wled00/ntp.cpp (6)
clearTimers(642-645)clearTimers(642-642)addTimer(577-631)addTimer(577-579)migrateTimersFromArrays(675-728)migrateTimersFromArrays(675-675)
⏰ 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). (3)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (usermods)
🔇 Additional comments (3)
wled00/cfg.cpp (3)
686-716: LGTM! New timer format deserialization is well-structured.The new vector-based format loading correctly:
- Clears existing timers before loading
- Properly reconstructs the weekdays byte with the enabled flag in bit 0
- Uses
hour < TIMER_HOUR_SUNSET(254) to correctly identify regular timers for date range extraction- Delegates validation to
addTimer()
772-786: LGTM! Legacy array migration fallback is correct.The fallback properly checks for data in legacy arrays and delegates to
migrateTimersFromArrays()which correctly handles the 10-slot layout (0-7 regular, 8 sunrise, 9 sunset) per the relevant code snippet.
1284-1307: Timer serialization requires verification of isRegular() consistency.The serialization appears correct for:
- Using
::timersto reference the global vector- Extracting day-of-week bits via right-shift (removing the enabled flag)
- Conditional inclusion of date ranges
However, verification is needed to confirm that
Timer::isRegular()aligns with the deserialization checkhour < TIMER_HOUR_SUNSETto ensure consistency across sunrise/sunset and regular timer handling.
|
Ok, I've built this and tested on my ESP32 and it seems to be working great after some superficial tests and minor fixes. I'll run some deeper tests around uploading older config files, adding too many timers, deleting and re-ordering in the same save, etc. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
wled00/data/settings_time.htm (4)
93-99: Replace magic number with a defined constant.The hardcoded
64formaxTimersshould be replaced with a constant defined in the backend or passed from the server. The comment at line 85 notes "will be overridden by server," but if the server doesn't provide this value, the hardcoded 64 will be used.Based on learnings, magic numbers should be replaced with defined constants. Consider having the server explicitly provide
maxTimersvia the settings script and removing the hardcoded default, or document that 64 is the platform-specific default.
206-206: Use strict equality comparison.The condition uses loose equality (
==) which can lead to unexpected type coercion. SincehourSelect.valueis a string from a DOM element, use strict equality with explicit type conversion.- var isSpecial = (hourSelect.value == 254 || hourSelect.value == 255); + var isSpecial = (parseInt(hourSelect.value) === TIMER_TYPE_SUNSET || parseInt(hourSelect.value) === TIMER_TYPE_SUNRISE);
394-400: Use strict equality and named constants.Similar to line 206, this uses loose equality (
==) for type comparison. Additionally, the magic number0here represents "Regular" timer type, while 254 and 255 represent special types.- if (hourSelect.value == 0) { + if (parseInt(hourSelect.value) === 0) { // Regular timer - use the hour input value } else {Or define a constant for regular timer type as well.
93-199: Consider validating timer count consistency on page load.When the page loads and timers are populated (via
FC()or serverGetV()calls),timerCountis incremented for eachaddTimerRow()call. However, if the server sends more timers thanmaxTimers, or if there's a mismatch, the UI could end up in an inconsistent state.Consider adding a check in
FC()or after server population:// After all timers are loaded if (timerCount > maxTimers) { console.warn(`Loaded ${timerCount} timers but max is ${maxTimers}`); // Optionally show warning to user }This helps catch configuration issues early during development and testing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/data/settings_time.htm(5 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
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_time.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_time.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_time.htm
🧠 Learnings (15)
📓 Common learnings
Learnt from: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.913Z
Learning: In WLED PR #5134, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
📚 Learning: 2025-12-01T07:01:16.913Z
Learnt from: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.913Z
Learning: In WLED PR #5134, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
Applied to files:
wled00/data/settings_time.htm
📚 Learning: 2025-12-01T07:01:16.914Z
Learnt from: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.914Z
Learning: In WLED timer system, even if a timer is enabled (enable bit set in timerWeekday), it will have no effect if the preset selected (timerMacro) is 0, because there can never be a preset with ID 0.
Applied to files:
wled00/data/settings_time.htm
📚 Learning: 2025-09-12T17:29:43.826Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4923
File: wled00/FX.cpp:4883-4901
Timestamp: 2025-09-12T17:29:43.826Z
Learning: In WLED’s web UI, only one slider value (e.g., SEGMENT.intensity or SEGMENT.custom1) changes at a time; code relying on this may use simplified change guards, though presets/JSON can still update multiple fields atomically.
Applied to files:
wled00/data/settings_time.htm
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/data/settings_time.htm
📚 Learning: 2025-11-14T13:37:11.994Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Applied to files:
wled00/data/settings_time.htm
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Applied to files:
wled00/data/settings_time.htm
📚 Learning: 2025-08-28T08:09:20.630Z
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Applied to files:
wled00/data/settings_time.htm
📚 Learning: 2025-10-10T18:34:06.550Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.
Applied to files:
wled00/data/settings_time.htm
📚 Learning: 2025-09-02T19:42:57.475Z
Learnt from: freakintoddles2
Repo: wled/WLED PR: 4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Applied to files:
wled00/data/settings_time.htm
📚 Learning: 2025-11-27T06:33:11.436Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5109
File: wled00/FX.cpp:3174-3343
Timestamp: 2025-11-27T06:33:11.436Z
Learning: WLED Ants effect (wled00/FX.cpp): The author prefers the current velocity initialization using hw_random16(1000, 5000)/5000.0f, resulting in an effective range of ~3.6–10.0 (with VELOCITY_MIN=2.0, VELOCITY_MAX=10.0), and wants the code kept as-is with comments updated to document this behavior. Avoid suggesting changes to span the full 2.0–10.0 range in future reviews.
Applied to files:
wled00/data/settings_time.htm
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, verify that file operations (especially file.open()) respect LittleFS filename limitations. Assume default WLED configuration with LittleFS default filename limit of 255 bytes. Do not assume extreme configuration values like WLED_MAX_SEGNAME_LEN = 512 which would not be standard configurations.
Applied to files:
wled00/data/settings_time.htm
📚 Learning: 2025-09-18T03:17:30.107Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1183-1194
Timestamp: 2025-09-18T03:17:30.107Z
Learning: In WLED bus management code, dead code branches in error handling (like unreachable else breaks) are intentionally preserved to accommodate potential future API changes, even when currently unreachable.
Applied to files:
wled00/data/settings_time.htm
📚 Learning: 2025-11-14T13:37:11.994Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, file operations (especially file.open()) should be checked to ensure they respect LittleFS filename limitations. The default LittleFS filename limit is 255 bytes (LFS_NAME_MAX). Reviews should assume default WLED configuration defines and not extreme edge-case values (e.g., WLED_MAX_SEGNAME_LEN = 512 would not be standard). File paths should be validated to stay within the 255-byte limit.
Applied to files:
wled00/data/settings_time.htm
📚 Learning: 2025-09-16T18:08:42.848Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1176-1187
Timestamp: 2025-09-16T18:08:42.848Z
Learning: In WLED finalizeInit() bus creation (wled00/FX_fcn.cpp), intentionally allowing memory overruns when bus configurations exceed MAX_LED_MEMORY is a deliberate design choice. The trade-off prioritizes creating buses with reduced LED counts over completely failing to create buses, which would cause no LED output and UI failures. This approach forces users to update configurations after migrating to version 0.16 while maintaining basic functionality.
Applied to files:
wled00/data/settings_time.htm
⏰ 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). (11)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32S3_wroom2)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32c3dev_qio)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32dev_debug)
🔇 Additional comments (5)
wled00/data/settings_time.htm (5)
275-291: Verify timer pairing integrity during reordering.The reorder functions assume each timer consists of exactly 2 consecutive rows (main + details). If a row is ever missing or the DOM structure becomes inconsistent (e.g., through manual manipulation or a bug in addTimerRow), the sibling navigation could select the wrong rows or fail silently.
Consider adding validation to ensure
row2andprevRow1belong to the correct timer before moving, or add defensive checks:// In moveTimerUp after line 286: if (!prevRow1 || prevRow1.id.indexOf('timer') !== 0) return; // Not a timer rowAlternatively, verify through testing that the two-row structure is always maintained.
382-389: Weekday bit calculation appears correct.The weekday bit manipulation correctly packs the enable checkbox (W{i}0) into bit 0 and the seven weekday checkboxes (W{i}1-7) into bits 1-7, matching the format expected by the backend.
The logic properly reconstructs the weekday value by iterating through all 8 checkboxes and building the bit field with powers of 2.
311-361: LGTM! Good migration path for timer data formats.The
FC()function properly handles three scenarios: moderntimerDataarray format, legacy separate arrays, and server-driven population viaGetV(). The type validation and empty timer filtering in legacy mode are well-implemented.The fallback chain ensures backward compatibility while supporting the new dynamic timer system.
16-16: Verify preset loading completes before settings populate.Since
loadPresets()is asynchronous andloadJS('/settings/s.js?p=5', ...)is called immediately after, there's a potential race condition. Ifs.jstriggersFC()or functions that depend on presets before the fetch completes, preset selects may not populate correctly.The implementation includes
presetsLoadedchecks (lines 153, 444) as guards, but confirm that the UI properly handles cases where the fetch is slow. Consider whether additional coordination is needed to ensures.jsexecution waits for preset initialization.
472-510: The indentation standard cited in the review comment is incorrect. According to the WLED project's actual practices, web UI files under wled00/data use 2-space indentation (spaces, not tabs). The CSS styles in this file should follow that convention rather than tabs.Likely an incorrect or invalid review comment.
| var isSunrise = (hour === 255); | ||
| var isSunset = (hour === 254); | ||
| var isSpecial = isSunrise || isSunset; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace magic numbers 255 and 254 with named constants.
The special timer type values 255 (sunrise) and 254 (sunset) are magic numbers repeated throughout the code (lines 104-106, 134-136, 206, 225, 397-398). These should be defined as named constants at the top of the script for maintainability.
Based on learnings, add named constants:
+ var TIMER_TYPE_SUNRISE = 255;
+ var TIMER_TYPE_SUNSET = 254;
var pJson = {}; // Store presets
var presetsLoaded = false;Then replace all occurrences:
- var isSunrise = (hour === 255);
- var isSunset = (hour === 254);
+ var isSunrise = (hour === TIMER_TYPE_SUNRISE);
+ var isSunset = (hour === TIMER_TYPE_SUNSET);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In wled00/data/settings_time.htm around lines 104-106 (and also update
occurrences at 134-136, 206, 225, 397-398), the code uses magic numbers 255 and
254 to indicate sunrise and sunset; define named constants (e.g., const
TIMER_SUNRISE = 255; const TIMER_SUNSET = 254;) near the top of the script and
replace all literal uses of 255 and 254 throughout the file with those
constants, ensuring any boolean checks (isSunrise/isSunset/isSpecial) and
comparisons use the new names and that no other numeric meanings are changed.
|
A hefty update that will take time to digest. |
|
I agree on the "dont like" points. Keeping the legacy arrays in RAM is wasteful and not acceptable. Also if there was a way to make it "back and forth" compatible would be great so people dont loose their schedules when downgrading, not sure its feasible. |
|
I agree this is a large update, and I have no issues making edits and getting this right. I can make edits to ensure the config-stored JSON timer array is the same as the older version of config-stored JSON timer array, which will make the "back and forth" possible. The only issue with that is that the old versions don't handle more than 8 timers (+2 for sun timers). So any extra will probably be silently discarded if anybody ever downgrades or moves their settings between controllers on different versions. I'm not sure anything can be done about that. Also, the new timers have the sun timers intermixed with the "regular" timers, so getting those back into their proper indexes is also not possible, unless we make efforts to do so when storing in the config file, which breaks the ordering. There are also the EEPROM-stored arrays that are not JSON, and have a different layout. Those can also be back-ported on downgrade by saving a config file and then the older version will take the config file and load those values into the EEPROM as they currently do. So those aren't a huge issue unless I'm missing something. Regarding keeping legacy code around, that was to allow the migration for EEPROM-stored values. If timers are no longer stored that way, and are stored in config files, then that can be removed. I can also try to tighten up the code by removing comments and white space and making the code leaner. I can also remove some of the UI improvements and revert those back to number inputs. I'll try making a config file with more than 8 timers in it, and downgrade my ESP32 and load them in to see what happens, but I don't have high hopes that it will work. I'm pretty sure it will silently discard any extras and the sun timers will break. |
thats great!
unavoidable so not a big deal.
I am not sure any device uses EEPROM still, the whole EEPROM stuff may be deprecated. I would not worry about it too much. Try to get the code increase down as much as possible, it's fine if there are no added bells and whistles. |
|
EEPROM has been deprecated before 0.15 release and last time it was used was in 0.9. |
|
Yeah, I did some digging and discovered that the EEPROM storage was converted to JSON (with the current format I still use, which makes back and forth within a reasonable version possible) in 2020. I've removed all the code that dealt with converting from old formats, and also removed the old timer arrays that are no longer relevant. I can also remove the old I also tightened up the code, removed all the comments, whitespace, etc. and removed all the UI bells and whistles. I looked for any other dead code regarding timers and macros, and was only able to find a few small spots in the |
|
|
good progress, the UI does not work right though: I get "time:1 An invalid form control with name='H2' is not focusable." when adding sunrise/sunset. Got the same error before saving it without any changes after uploading. Also adding a timer and not changing anything (i.e. enabled, 0,0,0) does remove it again when saving. Is that intentional? Not saying this needs changing, just checking for potential bugs. what I was not able to check: does it allow multiple sunrise/sunset presets? |
|
Looks much better now. Clean and efficient. |
I'll take a look at this. I haven't done much more than superficial testing yet.
Yes. Setting the preset to 0 (and hour and minute) deletes the timer. If this is not wanted, I can remove it, but I feel like there should be a way to remove timers.
It should... and they follow dates, so all my house timers (Halloween, Christmas, etc) can be set to start at sunset. There is no "global" sunrise or sunset timer anymore, they all fallow schedules (unless you don't set a date, and then it's enabled everyday) (or should be, I'll do some deeper testing when I can) |
Thank you :)
Not sure what you mean... Ahh, you mean for the dropdowns? I'll take a look and see what can be done with those. But I tried to make it as small as possible, but if you liked the dropdowns, I can re-implement that... |
They are useful, but keep code as small as possible. |
good point. setting it all to zero to remove may be a bit unintuitive. check how its done in LED config for buttons: a small x button to the right that sets it all to zero and effectively removing it when saving. small addition for a better UX. |
|
FYI, I'm going out of town this weekend, so I'll work on this when I get back. Just letting you know so you don't think I bailed. |
3 participants
This is not a PR in the strict sense, but more a request for conversation.
There have been a few mentions in the Discord channels about increasing the number of macro slots, and the responses given there were not to my satisfaction. So I went ahead and tried it on my controller.
It works just fine, but there are a few concerns/questions, notably, there are some choices that were made prior to this PR that affect how this change affects end-users. Mostly stemming from the indexes the sunrise and sunset macros have. Those macros were placed at the end of the array and the macro addition can be done in a few ways:
Anywho, this PR adds 8 more macro slots, and has been tested and works great. The only issue that I can see is the backward incompatibility for saved configs, and the issue I found of not being able to save the macros in the EEPROM. The macros save just fine in the internal config file, and load from there after startup and run just fine as well on my ESP32.
I don't know how this will affect any other controller as I only have the ESP32, and this does increase the size of the config file, so, it may be too big for smaller controllers.
Please let me know if this is a bad idea for any other reason, but as I see it right now, I can't find any glaring issues against it.
I will also happily modify the code to use any of the above 3 options for adding the macros to the array, option #1 being my preference, but #2 being a good workaround. And I may do that soon anyway.
NOTE: AI was used to help me with this PR.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.