Simpler TZ #5286
Simpler TZ #5286
Conversation
WalkthroughRefactors timezone storage into PROGMEM tuples with label strings, adds Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)wled00/**/*.cpp📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
🧠 Learnings (11)📓 Common learnings📚 Learning: 2025-08-31T03:38:14.114ZApplied to files:
📚 Learning: 2025-12-01T07:01:16.949ZApplied to files:
📚 Learning: 2026-01-13T03:07:13.582ZApplied to files:
📚 Learning: 2025-10-10T18:34:06.550ZApplied to files:
📚 Learning: 2026-01-01T13:18:14.738ZApplied to files:
📚 Learning: 2025-11-14T13:37:11.994ZApplied to files:
📚 Learning: 2025-08-28T08:09:20.630ZApplied to files:
📚 Learning: 2025-08-26T11:51:21.817ZApplied to files:
📚 Learning: 2025-11-14T13:37:30.955ZApplied to files:
📚 Learning: 2025-11-27T06:33:11.436ZApplied to files:
🪛 Cppcheck (2.19.0)wled00/ntp.cpp[error] 20-20: There is an unknown macro here somewhere. Configuration is required. If PROGMEM is a macro then please configure it. (unknownMacro) ⏰ 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). (17)
🔇 Additional comments (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
wled00/data/settings_time.htm (1)
88-96: Minor: Unnecessary post-increment on loop index.On line 92,
o.value = i++;uses post-increment butiis already provided byforEachas the correct zero-based index and isn't used afterward. The increment has no effect here.Suggested fix
function TZ() { zones.forEach((n,i)=>{ let o = cE("option"); - o.value = i++; + o.value = i; o.text = n; d.Sf["TZ"].appendChild(o); }); }wled00/ntp.cpp (1)
189-200: Consider reserving String capacity to reduce memory fragmentation.The function builds a JSON string through repeated concatenation in a loop, which causes multiple reallocations on ESP platforms with limited RAM. With ~25 timezones averaging ~15 characters each plus delimiters, the final string is approximately 500 bytes. Reserving capacity upfront would reduce heap fragmentation.
Suggested optimization
String getTZNamesJSONString() { String names = "["; + names.reserve(512); // approximate final size to reduce reallocations for (size_t i = 0; i < countof(TZ_TABLE); i++) { // the following is shorter code than sprintf() names += '"'; names += FPSTR(std::get<0>(TZ_TABLE[i])); names += '"'; names += ','; } names.setCharAt(names.length()-1, ']'); // replace last comma with bracket return names; }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
wled00/data/settings_time.htmwled00/fcn_declare.hwled00/ntp.cppwled00/wled.hwled00/xml.cpp
🧰 Additional context used
📓 Path-based instructions (5)
wled00/**/!(html_*)*.h
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for non-generated C++ header files (.h)
Files:
wled00/fcn_declare.hwled00/wled.h
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/settings_time.htm
wled00/data/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
wled00/data/**: When modifying web UI files, runnpm run buildto regenerate embedded headers before any firmware build
For web UI changes, edit files only under wled00/data (not firmware or generated files)
Files:
wled00/data/settings_time.htm
wled00/data/settings*.htm
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Name settings pages as settings*.htm within the web UI
Files:
wled00/data/settings_time.htm
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/ntp.cppwled00/xml.cpp
🧠 Learnings (14)
📓 Common learnings
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Learnt from: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.949Z
Learning: In WLED PR #5134, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
📚 Learning: 2025-04-26T19:19:07.600Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4658
File: wled00/const.h:140-141
Timestamp: 2025-04-26T19:19:07.600Z
Learning: In WLED, the WLED_MAX_PANELS macro is intentionally defined as a fixed constant value (18) with no redefinition mechanism, making it "unoverridable" - there's no need for a static assertion to check its maximum value.
Applied to files:
wled00/wled.h
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/html_*.h : DO NOT edit generated embedded web header files (wled00/html_*.h)
Applied to files:
wled00/wled.h
📚 Learning: 2025-08-28T08:09:20.630Z
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Applied to files:
wled00/wled.h
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Applied to files:
wled00/wled.h
📚 Learning: 2025-11-14T13:37:11.994Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Applied to files:
wled00/wled.h
📚 Learning: 2025-08-31T03:38:14.114Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.114Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Applied to files:
wled00/wled.h
📚 Learning: 2025-11-16T19:40:46.260Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4926
File: wled00/FX.cpp:4727-4730
Timestamp: 2025-11-16T19:40:46.260Z
Learning: WLED AuroraWave (wled00/FX.cpp): wave_start and wave_end intentionally use int16_t; segments longer than 32k LEDs are not supported (bounded by MAX_LEDS), so widening to 32-bit is unnecessary.
Applied to files:
wled00/wled.h
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/html_*.h : Always commit generated html_*.h files along with related source changes
Applied to files:
wled00/wled.h
📚 Learning: 2025-09-16T18:08:42.848Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1176-1187
Timestamp: 2025-09-16T18:08:42.848Z
Learning: In WLED finalizeInit() bus creation (wled00/FX_fcn.cpp), intentionally allowing memory overruns when bus configurations exceed MAX_LED_MEMORY is a deliberate design choice. The trade-off prioritizes creating buses with reduced LED counts over completely failing to create buses, which would cause no LED output and UI failures. This approach forces users to update configurations after migrating to version 0.16 while maintaining basic functionality.
Applied to files:
wled00/wled.h
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
Repo: wled/WLED PR: 4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Applied to files:
wled00/wled.h
📚 Learning: 2025-10-20T09:41:50.374Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4995
File: wled00/FX.cpp:5209-5212
Timestamp: 2025-10-20T09:41:50.374Z
Learning: In WLED 2D effects, SEGMENT.length() returns the physical pixel count (upper bound), while SEG_W * SEG_H can be smaller depending on mapping (mirroring/grouping). For per-cell state buffers (e.g., mode_2Dgameoflife in wled00/FX.cpp), allocate using SEGMENT.length() to ensure sufficient capacity and avoid reallocations, while bounding indexing by cols*rows.
Applied to files:
wled00/wled.h
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, verify that file operations (especially file.open()) respect LittleFS filename limitations. Assume default WLED configuration with LittleFS default filename limit of 255 bytes. Do not assume extreme configuration values like WLED_MAX_SEGNAME_LEN = 512 which would not be standard configurations.
Applied to files:
wled00/wled.h
📚 Learning: 2025-08-29T00:26:15.808Z
Learnt from: ksedgwic
Repo: wled/WLED PR: 4883
File: usermods/usermod_v2_skystrip/rest_json_client.h:6-14
Timestamp: 2025-08-29T00:26:15.808Z
Learning: WLED uses a vendored ArduinoJson library (version 6) located at "src/dependencies/json/ArduinoJson-v6.h" which is included through wled.h. Usermods should not directly include ArduinoJson headers but instead rely on wled.h for ArduinoJson symbols. The standard pattern is to include wled.h and use JsonObject, JsonArray, DynamicJsonDocument, etc. without additional includes.
Applied to files:
wled00/wled.h
🧬 Code graph analysis (2)
wled00/fcn_declare.h (1)
wled00/ntp.cpp (2)
getTZNamesJSONString(189-200)getTZNamesJSONString(189-189)
wled00/xml.cpp (1)
wled00/ntp.cpp (2)
getTZNamesJSONString(189-200)getTZNamesJSONString(189-189)
🪛 Cppcheck (2.19.0)
wled00/ntp.cpp
[error] 20-20: There is an unknown macro here somewhere. Configuration is required. If PROGMEM is a macro then please configure it.
(unknownMacro)
⏰ 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 (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 (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32S3_wroom2)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32c3dev_qio)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
🔇 Additional comments (10)
wled00/fcn_declare.h (1)
194-194: LGTM!The new function declaration is correctly placed in the ntp.cpp section and follows the existing declaration patterns in this file.
wled00/data/settings_time.htm (3)
11-11: LGTM!The
zonesarray is correctly initialized with a placeholder value that will be overwritten by the server-injected JSON fromgetTZNamesJSONString().
27-27: LGTM!The call to
TZ()is correctly placed afterFC()to ensure the form is initialized before populating the timezone select options.
153-153: LGTM!The empty select element will be populated at runtime by the
TZ()function, replacing the previous static hardcoded options.wled00/wled.h (1)
273-276: LGTM!The guarded redefinition of
countofensures consistent macro definition across the codebase. This is a good practice for array bounds checking and aligns with the PR's goal of simplifying timezone handling by usingcountof(TZ_TABLE)instead of a separate constant. Based on learnings, this improves maintainability by avoiding hardcoded array bounds.wled00/xml.cpp (1)
547-547: LGTM!The timezone names JSON is correctly injected before other time settings, ensuring the
zonesarray is defined whenTZ()is called in the client-side JavaScript. The temporaryStringfromgetTZNamesJSONString()is safely consumed byprintf_Pbefore destruction.wled00/ntp.cpp (4)
19-44: LGTM!The PROGMEM string constants are correctly defined as static file-local constants for flash storage, which is the standard pattern for ESP8266/ESP32 to save RAM. The static analysis warning about
PROGMEMbeing an unknown macro is a false positive—it's provided by the Arduino framework.
46-172: Verify PROGMEM behavior withstd::tupleon ESP platforms.The PR author mentioned uncertainty about whether storing timezone strings in PROGMEM works correctly. Using
std::tuplewithPROGMEMis non-trivial—while the string constants themselves are correctly in PROGMEM, accessingstd::get<0>(TZ_TABLE[i])to retrieve theconst char*may not behave as expected since the pointer value itself is stored in PROGMEM and needs to be read withpgm_read_ptr()rather than direct access.The current
FPSTR(std::get<0>(TZ_TABLE[i]))ingetTZNamesJSONString()assumes the pointer can be accessed directly. On ESP8266, this may cause issues because the pointer value is in flash and must be read specially.Please verify on ESP8266 that the timezone names display correctly in the settings UI. If they show garbage or crash, the pointer access needs adjustment:
// Potential fix if direct access fails on ESP8266: const char* name; memcpy_P(&name, &std::get<0>(TZ_TABLE[i]), sizeof(name)); names += FPSTR(name);
175-187: LGTM!The
updateTimezone()function correctly usescountof(TZ_TABLE)for bounds checking andmemcpy_Pwithstd::get<1/2>to copyTimeChangeRulestructs from PROGMEM.
16-17: LGTM!Using a defined constant
TZ_INIT(255) for the uninitialized state is clearer than a magic number.
|
@coderabbitai please implement suggestions. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@blazoncek, let me fetch the review comments from this PR to see what suggestions need to be implemented. 🏁 Script executed: Length of output: 20482 @blazoncek, I found two suggestions from the review that need to be implemented:
Let me implement these changes: [plan_code_changes] Task: Implement review suggestions
🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @wled00/data/settings_time.htm:
- Around line 87-95: In the TZ() function, remove the unnecessary post-increment
on the forEach index: replace the o.value assignment that uses i++ with the
plain index i (e.g., o.value = i) so the callback parameter isn't mutated; keep
the rest of the logic (creating option element, setting o.text, and appending to
d.Sf["TZ"]) unchanged.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wled00/data/settings_time.htmwled00/xml.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- wled00/xml.cpp
🧰 Additional context used
📓 Path-based instructions (3)
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/settings_time.htm
wled00/data/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
wled00/data/**: When modifying web UI files, runnpm run buildto regenerate embedded headers before any firmware build
For web UI changes, edit files only under wled00/data (not firmware or generated files)
Files:
wled00/data/settings_time.htm
wled00/data/settings*.htm
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Name settings pages as settings*.htm within the web UI
Files:
wled00/data/settings_time.htm
🧠 Learnings (1)
📓 Common learnings
Learnt from: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.949Z
Learning: In WLED PR #5134, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
🔇 Additional comments (2)
wled00/data/settings_time.htm (2)
11-11: LGTM! Initial placeholder value.The zones array is initialized with a placeholder and will be populated with the full timezone list from the backend (via
getTZNamesJSONString()) beforeTZ()is invoked.
152-152: LGTM! Dynamic population approach.The timezone select is now empty and will be populated at runtime by the
TZ()function. This aligns with the PR objective to simplify timezone handling by moving from hardcoded options to dynamic population.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @wled00/ntp.cpp:
- Around line 189-201: In getTZNamesJSONString() the code calls
name.reserve(512) but the variable is declared as names, causing a compile
error; change the call to use names.reserve(512) so the reserve is applied to
the correct String (names) and the function compiles correctly.
- Around line 175-187: In updateTimezone(), tzCurrent is set to the original
currentTimezone instead of the validated tz_table_entry after bounds-checking;
change the assignment so tzCurrent = tz_table_entry to reflect the clamped index
used to select TZ_TABLE (locate function updateTimezone and replace the
tzCurrent assignment accordingly).
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wled00/data/settings_time.htmwled00/ntp.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- wled00/data/settings_time.htm
🧰 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/ntp.cpp
🧠 Learnings (1)
📓 Common learnings
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.949Z
Learning: In WLED PR #5134, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, 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.
🪛 Cppcheck (2.19.0)
wled00/ntp.cpp
[error] 20-20: There is an unknown macro here somewhere. Configuration is required. If PROGMEM is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: wled_build / Build Environments (esp32dev_debug)
- GitHub Check: wled_build / Build Environments (usermods)
🔇 Additional comments (2)
wled00/ntp.cpp (2)
46-172: LGTM: TZ_TABLE refactored to use tuples.The migration from
std::pairtostd::tuplewith timezone name strings is well-structured. The PROGMEM attribute on the array is correct, and the data will be accessed properly viamemcpy_PinupdateTimezone().
19-44: Fix critical typo on line 191:name.reserve(512)should benames.reserve(512)The variable is declared as
names(plural) on line 190, but line 191 referencesname(singular), which will cause a compilation error.Additionally, storing the entire
TZ_TABLEarray with thePROGMEMattribute is problematic. While individual PROGMEM string declarations (lines 20-44) are fine, applyingPROGMEMto astd::tuplearray containingTimeChangeRulestructs can cause compilation issues with complex data types. Consider removing thePROGMEMattribute from theTZ_TABLEdeclaration (line 46) and keeping only the individual string pointers in PROGMEM.⛔ Skipped due to learnings
Learnt from: softhack007 Repo: wled/WLED PR: 0 File: :0-0 Timestamp: 2025-11-14T13:37:11.994Z Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.Learnt from: softhack007 Repo: wled/WLED PR: 0 File: :0-0 Timestamp: 2025-11-14T13:37:30.955Z Learning: In WLED code reviews, verify that file operations (especially file.open()) respect LittleFS filename limitations. Assume default WLED configuration with LittleFS default filename limit of 255 bytes. Do not assume extreme configuration values like WLED_MAX_SEGNAME_LEN = 512 which would not be standard configurations.Learnt from: softhack007 Repo: wled/WLED PR: 0 File: :0-0 Timestamp: 2025-11-14T13:37:11.994Z Learning: In WLED code reviews, file operations (especially file.open()) should be checked to ensure they respect LittleFS filename limitations. The default LittleFS filename limit is 255 bytes (LFS_NAME_MAX). Reviews should assume default WLED configuration defines and not extreme edge-case values (e.g., WLED_MAX_SEGNAME_LEN = 512 would not be standard). File paths should be validated to stay within the 255-byte limit.Learnt from: softhack007 Repo: wled/WLED PR: 0 File: :0-0 Timestamp: 2025-11-14T13:37:30.955Z Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.Learnt from: KrX3D Repo: wled/WLED PR: 4585 File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.h:121-136 Timestamp: 2025-04-27T09:37:28.415Z Learning: Using PROGMEM for the seven-segment font array (umSSDRNumbers) in the WLED SSDR usermod causes compilation problems, so it should be left as a regular array.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
wled00/ntp.cpp (2)
191-191: Consider increasing reserve size for safety margin.While 512 bytes may be sufficient for the current 25 timezones (estimated ~550 bytes), increasing the reservation to 1024 would provide a comfortable margin and prevent potential reallocation if timezone names or count increase in the future.
♻️ Suggested increase to reserve size
- names.reserve(512); + names.reserve(1024);
189-201: Use consistent PROGMEM access pattern withmemcpy_Pfor clarity.Line 195's direct use of
std::get<0>(TZ_TABLE[i])extracts a pointer from the PROGMEM tuple. While this works due to PROGMEM memory-mapping on ESP8266/ESP32, lines 183-184 use the more explicitmemcpy_Ppattern for accessing TZ_TABLE elements. For consistency and clarity, consider applying the same pattern:const char* name_ptr; memcpy_P(&name_ptr, &std::get<0>(TZ_TABLE[i]), sizeof(name_ptr)); names += FPSTR(name_ptr);
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/ntp.cpp
🧰 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/ntp.cpp
🧠 Learnings (4)
📓 Common learnings
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Learnt from: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.949Z
Learning: In WLED PR #5134, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
📚 Learning: 2025-12-01T07:01:16.949Z
Learnt from: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.949Z
Learning: In WLED PR #5134, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
Applied to files:
wled00/ntp.cpp
📚 Learning: 2025-10-10T18:34:06.550Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.
Applied to files:
wled00/ntp.cpp
📚 Learning: 2026-01-01T13:18:14.738Z
Learnt from: softhack007
Repo: wled/WLED PR: 5251
File: usermods/user_fx/user_fx.cpp:225-227
Timestamp: 2026-01-01T13:18:14.738Z
Learning: In WLED effects, when computing time differences between unsigned long timestamps (e.g., `strip.now`, `lastBumpUpdate`), always cast the subtraction result to `int` before converting to `float` to ensure correct behavior on ESP32-C3 (RISC-V). Use pattern: `float(int(timestamp1 - timestamp2))` instead of `float(timestamp1 - timestamp2)`.
Applied to files:
wled00/ntp.cpp
🪛 Cppcheck (2.19.0)
wled00/ntp.cpp
[error] 20-20: There is an unknown macro here somewhere. Configuration is required. If PROGMEM is a macro then please configure it.
(unknownMacro)
⏰ 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 (usermods)
- GitHub Check: wled_build / Build Environments (esp32dev_debug)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
🔇 Additional comments (3)
wled00/ntp.cpp (3)
20-44: LGTM! PROGMEM string declarations follow best practices.The timezone name strings are correctly declared as PROGMEM constants following the standard Arduino/ESP pattern.
Note: The static analysis warning about the PROGMEM macro is a false positive—PROGMEM is a standard platform macro.
46-172: Clean refactoring of TZ_TABLE structure.The migration from
std::pairtostd::tuplewith integrated timezone names simplifies maintenance. The structure is well-organized and each entry is clearly documented with timezone abbreviations and UTC offsets.
179-184: Correct PROGMEM access pattern for TimeChangeRule structs.The use of
memcpy_Pwith&std::get<1>and&std::get<2>is the proper way to read structs from PROGMEM tuples. The bounds checking withcountof(TZ_TABLE)is also a good improvement over hardcoded constants.
|
@blazoncek I'm wondering about the added value (technical) of this change 🤔 you are basicially moving the TZ names out of the HTM and into the existing array. Does it save flash space? Is it faster? |
|
Really? Adding new time zones in one place instead of two without needing to keep track of zone IDs is no added value? Yes, the flash use is slightly reduced too. Comments like this (and the one about creating offending usermods) keep affirming me that my place is no longer with WLED team and that my contributions are no longer welcome. Next time just close the PR, I will be less offended. |
|
looks like a solid change. There is only one thing I would add: since the list is now enumerated automatically, changing the order or adding entries will break backwards compatibility. A comment stating that the order must not be changed and entries can only be added at the end should make that clear. |
3 participants
Makes adding new timezones simpler by only needing to add new timezone to one array.
I've tried to put strings into PROGMEM but am not sure if that works. Would appreciate @willmmiles to take a look.
Summary by CodeRabbit
Refactor
New Features
✏️ Tip: You can customize this high-level summary in your review settings.