Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings
/WLEDPublic

add more macro/timer slots#5140

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Draft
benjamw wants to merge19 commits intowled:main
base:main
Choose a base branch
Loading
frombenjamw:more_macros
Draft

Conversation

@benjamw
Copy link
Contributor

@benjamwbenjamw commentedDec 1, 2025•
edited by coderabbitaibot
Loading

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:

  1. Move the sunrise/sunset macros to the start of the array, giving them indexes 0 and 1. This will allow future expansion without further affecting users. Although it will affect users who update. This is the method I most agree with, but do agree there are backwards incompatibilities which are bad.
  2. Keep them where they are, at indexes 8 and 9, keeping everything backward compatible, but causing weirdness for counting the expanded macros.
  3. Move them to the end, keeping with the current method, giving them new indexes 16 and 17. This is arguably the worst method, causing issues now, as well as in the future when more expansion may be added. But this is the choice that was made in this PR because I didn't really think it through before I made the changes and was just going for proof of concept.

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

    • Fully redesigned, dynamic timer system: add/remove/reorder up to platform limits, per‑timer enable, regular vs sunrise/sunset types, weekday selection, date ranges, Order column and “+ Add Timer” UI.
    • Preset integration: remote presets loading and preset selects in timer configuration.
    • Platform-specific max timers exposed to UI.

  • Bug Fixes

    • Robust migration from legacy timers, skips empty/disabled entries, preserves sunrise/sunset, compacts storage, and improves serialization/deserialization and UI sync.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitaibot commentedDec 1, 2025•
edited
Loading

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the.coderabbit.yaml file in this repository. To trigger a single review, invoke the@coderabbitai review command.

You can disable this status message by setting thereviews.review_status tofalse in the CodeRabbit configuration file.

Walkthrough

Replaces 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

Cohort / File(s)Summary
Core declarations & limits
wled00/fcn_declare.h,wled00/const.h,wled00/wled.h		AddsTimer struct and helpers, constantsTIMER_HOUR_SUNRISE/TIMER_HOUR_SUNSET, prototypes for timer management (addTimer,removeTimer,clearTimers,getTimerCount,compactTimers,migrateTimersFromArrays), globalstd::vector<Timer> timers,lastTimerMinute,improvActive,improvError, andWLED_MAX_TIMERS (ESP8266:16, others:64). Legacy arrays retained for migration.
Configuration serialization & migration
wled00/cfg.cpp,wled00/xml.cpp		cfg.cpp now reads/writes timers from a JSONtimersArray vector format with format detection and fallbacks to legacy JSON array and in-memory legacy arrays; migration logic added.xml.cpp emits timer rows from the timers vector, exposesmaxTimers to JS, and outputs a blank row if none.
Form parsing & server-side timer rebuild
wled00/set.cpp		Replaces fixed 10-entry parsing with dynamic indexed parsing up toWLED_MAX_TIMERS, reads fields only when present, builds timers viaaddTimer(...), and callscompactTimers() after rebuild.
Runtime trigger logic & timer management
wled00/ntp.cpp		Replaces per-array trigger checks with iteration overtimers, unified shouldTrigger flow (regular vs sunrise/sunset), emits debug when timers fire, and adds migration, validation, and timer lifecycle helpers (add/remove/clear/compact/migrate).
Web UI (timer editor)
wled00/data/settings_time.htm,wled00/xml.cpp		Replaces static timer table with dynamic JS-managed rows:loadPresets,populatePresetSelects,updatePresetSelect,addTimerRow,updateTimerType,removeTimerRow,moveTimerUp,moveTimerDown; supports sunrise/sunset types, weekday bitfields, date ranges, reorder/delete UX,timerCount andmaxTimers.
EEPROM notes
wled00/wled_eeprom.cpp		Adds comments clarifying timers 8/9 (sunrise/sunset) are stored in cfg.json and that legacy arrays are initialized; no functional EEPROM change.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Areas needing extra attention:

  • Migration detection and correctness between new JSONtimersArray, legacy JSON array, and in-memory legacy arrays (wled00/cfg.cpp,migrateTimersFromArrays).
  • Serialization/deserialization details: weekday bitfield, sunrise/sunset sentinel hour encoding, and regular timers' month/day ranges.
  • Consistent enforcement ofWLED_MAX_TIMERS across JS exposure, form parsing (set.cpp), storage, and runtime.
  • Runtime trigger semantics inntp.cpp, minute-edge behavior, and sunrise/sunset handling.
  • UI ↔ backend field naming, ordering and deletion/reorder behavior (wled00/data/settings_time.htm,wled00/xml.cpp,wled00/set.cpp).

Suggested reviewers

  • DedeHai
  • blazoncek

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check nameStatusExplanationResolution
Docstring Coverage⚠️ WarningDocstring coverage is 77.78% which is insufficient. The required threshold is 80.00%.You can run@coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check nameStatusExplanation
Description Check✅ PassedCheck skipped - CodeRabbit’s high-level summary is enabled.
Title check✅ PassedThe title 'add more macro/timer slots' aligns with the primary objective to expand timer and macro storage capacity using a new vector-based system with dynamic limits.

Thanks for usingCodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment@coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitaicoderabbitaibot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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:

#defineWLED_MAX_TIMERS18#defineWLED_MAX_TIMER_DATE_RANGES16

This would make future expansion easier and reduce risk of inconsistencies.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and betweenb5f13e4 and583fdd0.

📒 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.cpp
  • wled00/set.cpp
  • wled00/cfg.cpp
  • wled00/wled_eeprom.cpp
  • wled00/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 build to 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: softhack007Repo: wled/WLED PR: 0File: :0-0Timestamp: 2025-11-14T13:37:11.994ZLearning: 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-sgRepo: wled/WLED PR: 4876File: wled00/xml.cpp:0-0Timestamp: 2025-08-28T08:09:20.630ZLearning: 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: softhack007Repo: wled/WLED PR: 0File: :0-0Timestamp: 2025-11-14T13:37:30.955ZLearning: 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-sgRepo: wled/WLED PR: 4876File: wled00/wled_eeprom.cpp:0-0Timestamp: 2025-09-01T10:26:17.959ZLearning: 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: DedeHaiRepo: wled/WLED PR: 4939File: wled00/FX_fcn.cpp:1176-1187Timestamp: 2025-09-16T18:08:42.848ZLearning: 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: blazoncekRepo: wled/WLED PR: 4658File: wled00/const.h:140-141Timestamp: 2025-04-26T19:19:07.600ZLearning: 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: DedeHaiRepo: wled/WLED PR: 4926File: wled00/FX.cpp:4727-4730Timestamp: 2025-11-16T19:40:46.260ZLearning: 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: DedeHaiRepo: wled/WLED PR: 4793File: wled00/file.cpp:481-513Timestamp: 2025-07-31T18:21:49.868ZLearning: 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: DedeHaiRepo: wled/WLED PR: 4923File: wled00/FX.cpp:4883-4901Timestamp: 2025-09-12T17:29:43.826ZLearning: 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: DedeHaiRepo: wled/WLED PR: 4997File: wled00/e131.cpp:33-44Timestamp: 2025-10-10T18:34:06.550ZLearning: 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: DedeHaiRepo: wled/WLED PR: 4926File: wled00/FX.cpp:4727-4730Timestamp: 2025-11-16T19:40:46.260ZLearning: 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.cpp
  • wled00/wled.h
📚 Learning: 2025-10-10T18:34:06.550Z
Learnt from: DedeHaiRepo: wled/WLED PR: 4997File: wled00/e131.cpp:33-44Timestamp: 2025-10-10T18:34:06.550ZLearning: 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
  • wled00/wled.h
📚 Learning: 2025-09-01T10:26:17.959Z
Learnt from: mval-sgRepo: wled/WLED PR: 4876File: wled00/wled_eeprom.cpp:0-0Timestamp: 2025-09-01T10:26:17.959ZLearning: 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.cpp
  • wled00/wled.h
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CRRepo: wled/WLED PR: 0File: .github/copilot-instructions.md:0-0Timestamp: 2025-10-05T15:24:05.545ZLearning: Applies to wled00/html_*.h : DO NOT edit generated embedded web header files (wled00/html_*.h)

Applied to files:

  • wled00/wled_eeprom.cpp
  • wled00/wled.h
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007Repo: wled/WLED PR: 0File: :0-0Timestamp: 2025-11-14T13:37:30.955ZLearning: 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: softhack007Repo: wled/WLED PR: 0File: :0-0Timestamp: 2025-11-14T13:37:11.994ZLearning: 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-sgRepo: wled/WLED PR: 4876File: wled00/xml.cpp:0-0Timestamp: 2025-08-28T08:09:20.630ZLearning: 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: BobLoeffler68Repo: wled/WLED PR: 4891File: wled00/FX.cpp:3333-3349Timestamp: 2025-08-31T03:38:14.114ZLearning: 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: KrX3DRepo: wled/WLED PR: 4664File: wled00/syslog.cpp:50-51Timestamp: 2025-04-26T12:51:04.993ZLearning: 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: ksedgwicRepo: wled/WLED PR: 4883File: usermods/usermod_v2_skystrip/rest_json_client.h:6-14Timestamp: 2025-08-29T00:26:15.808ZLearning: 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: CRRepo: wled/WLED PR: 0File: .github/copilot-instructions.md:0-0Timestamp: 2025-10-05T15:24:05.545ZLearning: 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: freakintoddles2Repo: wled/WLED PR: 4904File: wled00/FX.cpp:3915-3930Timestamp: 2025-09-02T19:42:57.475ZLearning: 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: DedeHaiRepo: wled/WLED PR: 4939File: wled00/FX_fcn.cpp:1176-1187Timestamp: 2025-09-16T18:08:42.848ZLearning: 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: DedeHaiRepo: wled/WLED PR: 4750File: usermods/audioreactive/audio_reactive.cpp:311-321Timestamp: 2025-06-27T06:04:44.652ZLearning: 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 encounteringhour==255, which marks sunrise/sunset entries. This assumes:

  1. Regular timers (0-15) always appear before sunrise/sunset in JSON
  2. 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.

@DedeHai
Copy link
Collaborator

appreciate your effort. without looking too much into this: doesn't your update break sunrise/sunset configurations of existing setups?
The way this should be implemented is the same as with other recent changes, like palettes or buttons: use vectors so its future proof or the next request will just be "I need more than 16 macros, my plan requires 24". Now whether this is feasible without breaking backwards compatibility I am not sure, but if compatibility is broken, it better be the "final" version.

@blazoncek
Copy link
Collaborator

FYI there was a PR that implemented entirely new approach totimed presets. It lacked UI though.
It might be better to revisit that as it opened quite a few new possibilities.

@benjamw
Copy link
ContributorAuthor

benjamw commentedDec 1, 2025•
edited
Loading

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.
If you wish to make this the "final" version with minimal disruption, the proper way to do it IMHO would be to move those sunrise/sunset configs to indexes 0 and 1, and put all the macro configs after it, which would ultimately make those infinite.

I'm not sure what is different abouttimed 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 forany 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 can also add a check to see if the macros at index 8 and 9 (the current sun macros) are being used and have settings, and move those as needed when being read from the config file. That may make updating painless for users as they won't have any idea that it was moved, and it will just work as it always has.

I'm going to try converting to vectors tonight and see how that pans out with open-ended macros.

@benjamw
Copy link
ContributorAuthor

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.

@blazoncek
Copy link
Collaborator

Keep in mind RAM constrained ESP8266.

@benjamw
Copy link
ContributorAuthor

Ok, I have completely rewritten this using the ideas from that other PR as well as my own ideas.

Copy link
Contributor

@coderabbitaicoderabbitaibot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (7)
wled00/const.h (1)

56-86:WLED_MAX_TIMERS choices are reasonable and match the new vector path

Per‑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 raiseWLED_MAX_TIMERS above 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 decodingweekdays correctly

The SUBPAGE_TIME block now cleanly reflects the vector model andWLED_MAX_TIMERS, and emittingaddTimerRow(...) fromtimers is consistent with the JSON encoding (packedweekdays bitfield plus full date range). Please double‑check thataddTimerRow insettings_time.htm interprets theweekdays argument 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 growth

The 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 toaddTimer and compacting afterwards.

Two small improvement points:

  • Readminute into a wider type (e.g.int minuteVal = request->arg(k).toInt(); then cast toint8_t when 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; ifWLED_MAX_TIMERS is 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 entries

TheserializeConfig side now:

  • Iterates::timers and skips pure “empty” timers (preset==0 && hour==0 && minute==0).
  • Writesen,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 withpreset == 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:

  1. Allowing these timers through (they're harmless since preset 0 has no effect)
  2. 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 checkstimerMacro[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 wherepreset == 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

📥 Commits

Reviewing files that changed from the base of the PR and between583fdd0 and9740a29.

📒 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.cpp
  • wled00/xml.cpp
  • wled00/cfg.cpp
  • wled00/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.h
  • wled00/fcn_declare.h
  • wled00/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 build to 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: blazoncekRepo: wled/WLED PR: 5140File: wled00/data/settings_time.htm:66-76Timestamp: 2025-12-01T07:01:16.913ZLearning: 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: softhack007Repo: wled/WLED PR: 0File: :0-0Timestamp: 2025-11-14T13:37:11.994ZLearning: 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-sgRepo: wled/WLED PR: 4876File: wled00/xml.cpp:0-0Timestamp: 2025-08-28T08:09:20.630ZLearning: 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: DedeHaiRepo: wled/WLED PR: 4939File: wled00/FX_fcn.cpp:1176-1187Timestamp: 2025-09-16T18:08:42.848ZLearning: 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: blazoncekRepo: wled/WLED PR: 5140File: wled00/data/settings_time.htm:66-76Timestamp: 2025-12-01T07:01:16.913ZLearning: 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.cpp
  • wled00/const.h
  • wled00/xml.cpp
  • wled00/data/settings_time.htm
  • wled00/fcn_declare.h
  • wled00/wled.h
  • wled00/cfg.cpp
  • wled00/ntp.cpp
📚 Learning: 2025-10-10T18:34:06.550Z
Learnt from: DedeHaiRepo: wled/WLED PR: 4997File: wled00/e131.cpp:33-44Timestamp: 2025-10-10T18:34:06.550ZLearning: 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.cpp
  • wled00/wled.h
📚 Learning: 2025-08-28T08:09:20.630Z
Learnt from: mval-sgRepo: wled/WLED PR: 4876File: wled00/xml.cpp:0-0Timestamp: 2025-08-28T08:09:20.630ZLearning: 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.cpp
  • wled00/const.h
  • wled00/data/settings_time.htm
  • wled00/wled.h
📚 Learning: 2025-11-14T13:37:11.994Z
Learnt from: softhack007Repo: wled/WLED PR: 0File: :0-0Timestamp: 2025-11-14T13:37:11.994ZLearning: 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.cpp
  • wled00/const.h
  • wled00/data/settings_time.htm
  • wled00/wled.h
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007Repo: wled/WLED PR: 0File: :0-0Timestamp: 2025-11-14T13:37:30.955ZLearning: 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.cpp
  • wled00/const.h
  • wled00/data/settings_time.htm
  • wled00/wled.h
📚 Learning: 2025-12-01T07:01:16.914Z
Learnt from: blazoncekRepo: wled/WLED PR: 5140File: wled00/data/settings_time.htm:66-76Timestamp: 2025-12-01T07:01:16.914ZLearning: 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.cpp
  • wled00/data/settings_time.htm
  • wled00/wled.h
  • wled00/ntp.cpp
📚 Learning: 2025-11-16T19:40:46.260Z
Learnt from: DedeHaiRepo: wled/WLED PR: 4926File: wled00/FX.cpp:4727-4730Timestamp: 2025-11-16T19:40:46.260ZLearning: 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.cpp
  • wled00/const.h
  • wled00/wled.h
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CRRepo: wled/WLED PR: 0File: .github/copilot-instructions.md:0-0Timestamp: 2025-10-05T15:24:05.545ZLearning: Applies to wled00/html_*.h : DO NOT edit generated embedded web header files (wled00/html_*.h)

Applied to files:

  • wled00/set.cpp
  • wled00/const.h
  • wled00/fcn_declare.h
  • wled00/wled.h
📚 Learning: 2025-11-30T15:29:00.711Z
Learnt from: DedeHaiRepo: wled/WLED PR: 4456File: usermods/deep_sleep/deep_sleep.cpp:224-230Timestamp: 2025-11-30T15:29:00.711ZLearning: 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: blazoncekRepo: wled/WLED PR: 4658File: wled00/const.h:140-141Timestamp: 2025-04-26T19:19:07.600ZLearning: 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: blazoncekRepo: wled/WLED PR: 4482File: wled00/udp.cpp:147-149Timestamp: 2025-02-19T12:43:34.200ZLearning: 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: DedeHaiRepo: wled/WLED PR: 4939File: wled00/FX_fcn.cpp:1176-1187Timestamp: 2025-09-16T18:08:42.848ZLearning: 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.h
  • wled00/data/settings_time.htm
📚 Learning: 2025-02-19T12:43:34.199Z
Learnt from: blazoncekRepo: wled/WLED PR: 4482File: wled00/udp.cpp:147-149Timestamp: 2025-02-19T12:43:34.199ZLearning: 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: netmindzRepo: wled/WLED PR: 4728File: wled00/FX.h:378-378Timestamp: 2025-06-15T09:59:52.720ZLearning: 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: blazoncekRepo: wled/WLED PR: 4995File: wled00/FX.cpp:5223-5226Timestamp: 2025-10-20T09:38:51.997ZLearning: 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: willmmilesRepo: wled/WLED PR: 4939File: wled00/FX_fcn.cpp:1183-1194Timestamp: 2025-09-18T03:17:30.107ZLearning: 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: DedeHaiRepo: wled/WLED PR: 4928File: wled00/bus_wrapper.h:1248-1275Timestamp: 2025-09-21T20:44:09.502ZLearning: 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: DedeHaiRepo: wled/WLED PR: 4798File: wled00/FX.cpp:7531-7533Timestamp: 2025-08-26T11:51:21.817ZLearning: 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: BobLoeffler68Repo: wled/WLED PR: 5109File: wled00/FX.cpp:3174-3343Timestamp: 2025-11-27T06:33:11.436ZLearning: 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: freakintoddles2Repo: wled/WLED PR: 4904File: wled00/FX.cpp:3915-3930Timestamp: 2025-09-02T19:42:57.475ZLearning: 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
  • wled00/wled.h
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007Repo: wled/WLED PR: 0File: :0-0Timestamp: 2025-11-14T13:37:30.955ZLearning: 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: willmmilesRepo: wled/WLED PR: 4939File: wled00/FX_fcn.cpp:1183-1194Timestamp: 2025-09-18T03:17:30.107ZLearning: 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: softhack007Repo: wled/WLED PR: 0File: :0-0Timestamp: 2025-11-14T13:37:11.994ZLearning: 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: BobLoeffler68Repo: wled/WLED PR: 4891File: wled00/FX.cpp:3333-3349Timestamp: 2025-08-31T03:38:14.114ZLearning: 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-sgRepo: wled/WLED PR: 4876File: wled00/wled_eeprom.cpp:0-0Timestamp: 2025-09-01T10:26:17.959ZLearning: 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 model

The hour sentinels, weekday encoding, and helper methods align with the addTimer/checkTimers logic and give a compact 8‑byte representation without obvious correctness issues. UsingisEnabled() to gate on both enable bit andpreset != 0 is a good fit for the existing “preset 0 is inert” semantics.

wled00/set.cpp (1)

300-310:Comment-only change around touch buttons; logic unchanged

The SOC_TOUCH_VERSION_2 block still preserves the intended control flow (else guarded 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-only

Introducingstd::vector<Timer> timers alongsidelastTimerMinute matches 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 thetimers vector exclusively, and
  • The legacy arrays are only read by migration/EEPROM code paths (migrateTimersFromArrays etc.), 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 configs

The newtimers.ins handling correctly rebuilds the vector with hour/min/macro, packed weekdays, and optional date ranges, delegating validation toaddTimer.

However, the legacy fallback assumestimerMacro/Hours/Minutes are 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,hasLegacyData may stay false and existing timers could silently disappear.

Confirm that for all supported upgrade paths, either:

  • Old configs already use the newtimers.ins shape being parsed here, or
  • The legacy arrays are populated from EEPROM or an earlier JSON readerbefore 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 callingaddTimerRow() for each timer during settings script execution. Confirm the settings script (loaded at line 13) contains the necessary timer initialization logic.

Copy link
Contributor

@coderabbitaicoderabbitaibot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 updatedWd() 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.

ThemaxTimers variable defaults to 64, but the comment says it will be overridden by the server. This value should matchWLED_MAX_TIMERS defined 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 server

Also 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 conditionstimerWeekday[8] != 0 andtimerWeekday[9] != 0 will 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

📥 Commits

Reviewing files that changed from the base of the PR and between9740a29 and5cde8b8.

📒 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 build to 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.cpp
  • wled00/cfg.cpp
🧠 Learnings (15)
📓 Common learnings
Learnt from: blazoncekRepo: wled/WLED PR: 5140File: wled00/data/settings_time.htm:66-76Timestamp: 2025-12-01T07:01:16.913ZLearning: 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: softhack007Repo: wled/WLED PR: 0File: :0-0Timestamp: 2025-11-14T13:37:11.994ZLearning: 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: blazoncekRepo: wled/WLED PR: 5140File: wled00/data/settings_time.htm:66-76Timestamp: 2025-12-01T07:01:16.913ZLearning: 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
  • wled00/ntp.cpp
  • wled00/cfg.cpp
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CRRepo: wled/WLED PR: 0File: .github/copilot-instructions.md:0-0Timestamp: 2025-10-05T15:24:05.545ZLearning: 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: blazoncekRepo: wled/WLED PR: 5140File: wled00/data/settings_time.htm:66-76Timestamp: 2025-12-01T07:01:16.914ZLearning: 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
  • wled00/ntp.cpp
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHaiRepo: wled/WLED PR: 4798File: wled00/FX.cpp:7531-7533Timestamp: 2025-08-26T11:51:21.817ZLearning: 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: softhack007Repo: wled/WLED PR: 0File: :0-0Timestamp: 2025-11-14T13:37:11.994ZLearning: 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
  • wled00/ntp.cpp
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007Repo: wled/WLED PR: 0File: :0-0Timestamp: 2025-11-14T13:37:30.955ZLearning: 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-sgRepo: wled/WLED PR: 4876File: wled00/xml.cpp:0-0Timestamp: 2025-08-28T08:09:20.630ZLearning: 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: DedeHaiRepo: wled/WLED PR: 4997File: wled00/e131.cpp:33-44Timestamp: 2025-10-10T18:34:06.550ZLearning: 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
  • wled00/ntp.cpp
📚 Learning: 2025-09-02T19:42:57.475Z
Learnt from: freakintoddles2Repo: wled/WLED PR: 4904File: wled00/FX.cpp:3915-3930Timestamp: 2025-09-02T19:42:57.475ZLearning: 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: BobLoeffler68Repo: wled/WLED PR: 5109File: wled00/FX.cpp:3174-3343Timestamp: 2025-11-27T06:33:11.436ZLearning: 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: softhack007Repo: wled/WLED PR: 0File: :0-0Timestamp: 2025-11-14T13:37:30.955ZLearning: 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: willmmilesRepo: wled/WLED PR: 4939File: wled00/FX_fcn.cpp:1183-1194Timestamp: 2025-09-18T03:17:30.107ZLearning: 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: softhack007Repo: wled/WLED PR: 0File: :0-0Timestamp: 2025-11-14T13:37:11.994ZLearning: 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: DedeHaiRepo: wled/WLED PR: 4939File: wled00/FX_fcn.cpp:1176-1187Timestamp: 2025-09-16T18:08:42.848ZLearning: 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 thedow field
  • Handles sunrise/sunset vs regular timer distinction via hour check againstTIMER_HOUR_SUNSET
  • Delegates validation toaddTimer()

1280-1303:Serialization logic is consistent with deserialization.

The code correctly:

  • Uses::timers to disambiguate from localtimers JSON object
  • Skips meaningless timers (preset=0, hour=0, minute=0)
  • Only serializes date range for regular timers viatimer.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:

  • DecrementstimerCount to 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 againstmaxTimers limit
  • 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 refactoredcheckTimers() 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 viaapplyPreset()

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 inisTodayInDateRange() 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). Theshrink_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 withSUNSET_MAX improves 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>
Copy link
Contributor

@coderabbitaicoderabbitaibot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
wled00/cfg.cpp (1)

744-744:Default weekdays value may incorrectly enable legacy timers.

The defaultweekdays = 255 sets all bits including bit 0 (enabled flag). For legacy timers wheretimerWeekdayJson is 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

📥 Commits

Reviewing files that changed from the base of the PR and between5cde8b8 and492972e.

📒 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: blazoncekRepo: wled/WLED PR: 5140File: wled00/data/settings_time.htm:66-76Timestamp: 2025-12-01T07:01:16.913ZLearning: 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: softhack007Repo: wled/WLED PR: 0File: :0-0Timestamp: 2025-11-14T13:37:11.994ZLearning: 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: blazoncekRepo: wled/WLED PR: 5140File: wled00/data/settings_time.htm:66-76Timestamp: 2025-12-01T07:01:16.913ZLearning: 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: softhack007Repo: wled/WLED PR: 0File: :0-0Timestamp: 2025-11-14T13:37:11.994ZLearning: 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-sgRepo: wled/WLED PR: 4876File: wled00/xml.cpp:0-0Timestamp: 2025-08-28T08:09:20.630ZLearning: 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: softhack007Repo: wled/WLED PR: 0File: :0-0Timestamp: 2025-11-14T13:37:30.955ZLearning: 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: DedeHaiRepo: wled/WLED PR: 4997File: wled00/e131.cpp:33-44Timestamp: 2025-10-10T18:34:06.550ZLearning: 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
  • Useshour < TIMER_HOUR_SUNSET (254) to correctly identify regular timers for date range extraction
  • Delegates validation toaddTimer()

772-786:LGTM! Legacy array migration fallback is correct.

The fallback properly checks for data in legacy arrays and delegates tomigrateTimersFromArrays() 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::timers to 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 thatTimer::isRegular() aligns with the deserialization checkhour < TIMER_HOUR_SUNSET to ensure consistency across sunrise/sunset and regular timer handling.

@benjamwbenjamw changed the titleadd 8 more macro slotsadd more macro/timer slotsDec 2, 2025
converted preset entry to select box for ease of use
@benjamw
Copy link
ContributorAuthor

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.

Copy link
Contributor

@coderabbitaicoderabbitaibot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (4)
wled00/data/settings_time.htm (4)

93-99:Replace magic number with a defined constant.

The hardcoded64 formaxTimers should 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 providemaxTimers via 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.value is 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 number0 here 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 (viaFC() or serverGetV() calls),timerCount is 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 inFC() or after server population:

// After all timers are loadedif(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

📥 Commits

Reviewing files that changed from the base of the PR and between383440c andacf0153.

📒 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 build to 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: blazoncekRepo: wled/WLED PR: 5140File: wled00/data/settings_time.htm:66-76Timestamp: 2025-12-01T07:01:16.913ZLearning: 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: softhack007Repo: wled/WLED PR: 0File: :0-0Timestamp: 2025-11-14T13:37:11.994ZLearning: 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: blazoncekRepo: wled/WLED PR: 5140File: wled00/data/settings_time.htm:66-76Timestamp: 2025-12-01T07:01:16.913ZLearning: 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: blazoncekRepo: wled/WLED PR: 5140File: wled00/data/settings_time.htm:66-76Timestamp: 2025-12-01T07:01:16.914ZLearning: 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: DedeHaiRepo: wled/WLED PR: 4923File: wled00/FX.cpp:4883-4901Timestamp: 2025-09-12T17:29:43.826ZLearning: 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: DedeHaiRepo: wled/WLED PR: 4798File: wled00/FX.cpp:7531-7533Timestamp: 2025-08-26T11:51:21.817ZLearning: 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: softhack007Repo: wled/WLED PR: 0File: :0-0Timestamp: 2025-11-14T13:37:11.994ZLearning: 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: softhack007Repo: wled/WLED PR: 0File: :0-0Timestamp: 2025-11-14T13:37:30.955ZLearning: 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-sgRepo: wled/WLED PR: 4876File: wled00/xml.cpp:0-0Timestamp: 2025-08-28T08:09:20.630ZLearning: 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: DedeHaiRepo: wled/WLED PR: 4997File: wled00/e131.cpp:33-44Timestamp: 2025-10-10T18:34:06.550ZLearning: 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: freakintoddles2Repo: wled/WLED PR: 4904File: wled00/FX.cpp:3915-3930Timestamp: 2025-09-02T19:42:57.475ZLearning: 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: BobLoeffler68Repo: wled/WLED PR: 5109File: wled00/FX.cpp:3174-3343Timestamp: 2025-11-27T06:33:11.436ZLearning: 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: softhack007Repo: wled/WLED PR: 0File: :0-0Timestamp: 2025-11-14T13:37:30.955ZLearning: 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: willmmilesRepo: wled/WLED PR: 4939File: wled00/FX_fcn.cpp:1183-1194Timestamp: 2025-09-18T03:17:30.107ZLearning: 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: softhack007Repo: wled/WLED PR: 0File: :0-0Timestamp: 2025-11-14T13:37:11.994ZLearning: 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: DedeHaiRepo: wled/WLED PR: 4939File: wled00/FX_fcn.cpp:1176-1187Timestamp: 2025-09-16T18:08:42.848ZLearning: 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 ensurerow2 andprevRow1 belong 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 row

Alternatively, 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.

TheFC() function properly handles three scenarios: moderntimerData array 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.

SinceloadPresets() is asynchronous andloadJS('/settings/s.js?p=5', ...) is called immediately after, there's a potential race condition. Ifs.js triggersFC() or functions that depend on presets before the fetch completes, preset selects may not populate correctly.

The implementation includespresetsLoaded checks (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.js execution 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 use2-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.

Comment on lines +104 to +106
varisSunrise=(hour===255);
varisSunset=(hour===254);
varisSpecial=isSunrise||isSunset;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

🛠️ Refactor suggestion |🟠 Major

Replace magic numbers 255 and 254 with named constants.

The special timer type values255 (sunrise) and254 (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 updateoccurrences at 134-136, 206, 225, 397-398), the code uses magic numbers 255 and254 to indicate sunrise and sunset; define named constants (e.g., constTIMER_SUNRISE = 255; const TIMER_SUNSET = 254;) near the top of the script andreplace all literal uses of 255 and 254 throughout the file with thoseconstants, ensuring any boolean checks (isSunrise/isSunset/isSpecial) andcomparisons use the new names and that no other numeric meanings are changed.

@blazoncek
Copy link
Collaborator

A hefty update that will take time to digest.
On first glance there are two things I don't like: special code to handle legacy (when will it be removed?) and code size increase.

@DedeHai
Copy link
Collaborator

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.
Code increase is 3.5k which is not acceptable. Keep it as lightweight as possible.

@DedeHaiDedeHai marked this pull request as draftDecember 2, 2025 08:09
@benjamw
Copy link
ContributorAuthor

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.

@DedeHai
Copy link
Collaborator

which will make the "back and forth" possible

thats great!

So any extra will probably be silently discarded if anybody ever downgrades or moves their settings between controllers on different versions.

unavoidable so not a big deal.

There are also the EEPROM-stored arrays

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.

benjamw reacted with thumbs up emoji

@blazoncek
Copy link
Collaborator

EEPROM has been deprecated before 0.15 release and last time it was used was in 0.9.

@benjamw
Copy link
ContributorAuthor

benjamw commentedDec 3, 2025•
edited
Loading

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 oldwled_eeprom.cpp file to make room for this if requested. But I totally understand if that's waiting for a deprecation period to end.

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 theeeprom file.

@DedeHai
Copy link
Collaborator

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?

@blazoncek
Copy link
Collaborator

Looks much better now. Clean and efficient.
If you still need presets (as in 1st implementation), you do not need to fetch them via HTTP request but can check local storage which keeps a copy.

@benjamw
Copy link
ContributorAuthor

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.

I'll take a look at this. I haven't done much more than superficial testing yet.

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.

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.

what I was not able to check: does it allow multiple sunrise/sunset presets?

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)

@benjamw
Copy link
ContributorAuthor

benjamw commentedDec 3, 2025•
edited
Loading

Looks much better now. Clean and efficient.

Thank you :)

If you still need presets (as in 1st implementation), you do not need to fetch them via HTTP request but can check local storage which keeps a copy.

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...

@blazoncek
Copy link
Collaborator

if you liked the dropdowns

They are useful, but keep code as small as possible.

benjamw reacted with thumbs up emoji

@DedeHai
Copy link
Collaborator

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.

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.

benjamw reacted with thumbs up emoji

@benjamw
Copy link
ContributorAuthor

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.

@benjamw
Copy link
ContributorAuthor

Ok, I re-added the preset dropdowns and made that as small as possible. I also sorted them to match the main display (pretty closely, some punctuation, like underscore, is sorted differently).

I also fixed theAn invalid form control with name='H2' is not focusable issue. Let me know if you still get this, because I haven't seen it. And if you do, can you provide your cfg.json file for debugging?

I also fixed an annoyance of mine where when swapping between Sunrise/Sunset/Regular, any hour that was set in Regular would disappear when going back to Regular. Very annoying for users who are just playing around, twiddling all the knobs.

@DedeHai
Copy link
Collaborator

I like the dropdown approach in general but it does break the layout as the field is too wide with long preset names.
also there is no delete button and the selection to delete does not work.
there is also a bug when adding mulitple timers: it wont respect the checkmarks.
the sorting of the list is unnecessary IMHO. The main UI can also be sorted by number so it wont match.

please do thorough testing also on mobile.

benjamw reacted with thumbs up emoji

@benjamw
Copy link
ContributorAuthor

I see the mobile issue where preset names are too long. Unfortunately this is a result of the Macro Button options being in a table and fixing it would either require reverting those sections back to numerical inputs, or adding some javascript/css that would allow the table to collapse into blocks with the screen size.

I can easily revert the inputs, but converting to collapsible blocks would require quite a bit of HTML/CSS/JS updates that would drastically change the layout and/or quickly eat up the size budget.

I've also adjusted the timer clean up logic to delete any timer with preset = 0, so the delete dropdown option should work now.

I'm not sure what was meant by "there is also a bug when adding mulitple timers: it wont respect the checkmarks."... does that mean that the checkmarks are not showing as expected (on or off), or that timers that are enabled are not working, or timers that are disabled are triggering? I can't replicate any of those states.

I've also added the option to select which sort method to use.

I've updated the HTML/CSS to use flex boxes just to see what that looked like.

There is a bug where the Sunset/Sunrise setting is not saved anymore with the new HTML that I'll look at tomorrow.

@blazoncek
Copy link
Collaborator

adding some javascript/css

CSS:text-overflow:ellipsis; See:https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Properties/text-overflow

collapsible blocks would require quite a bit of HTML/CSS/JS

Not really, just a class toggle. I.e.element.classList.toggle("hide"); CSS does the rest.

Overall, nice progress. 👍

@benjamw
Copy link
ContributorAuthor

collapsible blocks would require quite a bit of HTML/CSS/JS

Not really, just a class toggle. I.e.element.classList.toggle("hide"); CSS does the rest.

Sorry I was typing fast and not very clear. I didn't mean collapse and hide, I meant collapse and wrap down the page. Tables can't flow with smaller screens. I switched to use flex boxes and individual cards for each for both buttons and timers, so those can more easily flow down the page on smaller screens.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@blazoncekblazoncekblazoncek left review comments

@coderabbitaicoderabbitai[bot]coderabbitai[bot] left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@benjamw@DedeHai@blazoncek
[8]ページ先頭
©2009-2025 Movatter.jp