Extended Wordclock#5586
Open
Zippochonda wants to merge 13 commits into
Open
Conversation
Added a Link to the source of the SVG
Mapping wrong at 11x11 ---> eins
Added map for a 2d Matrix in Meanderwiring
Added meander wiring for a 2d Matrix 11x11
Add the Status of the usermod in JSON, so that you can switch off and on the Usermod remotely for Example with Home Assistant
Added the integration into Homeassist
Added HA Configuration
Added a new configuration option for meander wiring and updated documentation to reflect changes.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe Word Clock usermod now supports 11x11 grid wiring modes alongside existing configurations. New LED mask lookup tables enable four wiring combinations (normal, meander, 11x11, 11x11-meander). Display logic branches on configuration flags to select appropriate tables. Config serialization restructures state persistence to use nested JSON objects with ChangesWord Clock 11x11 Grid & Meander Wiring
🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
usermods/usermod_v2_word_clock/usermod_v2_word_clock.cpp (2)
459-463: ⚡ Quick winMove repeated JSON keys to
PROGMEMconstants and avoid RAM string literals.Line 460/471/474 adds repeated raw key strings (
"WordClockUsermod","active"). In usermods, these should be sharedstatic const char[] PROGMEMkeys (used viaFPSTR/F) to reduce RAM pressure.Suggested change
+ static const char kWordClockUsermod[] PROGMEM; + static const char kActive[] PROGMEM; ... - JsonObject usermodJson = root.createNestedObject("WordClockUsermod"); - usermodJson["active"] = usermodActive; + JsonObject usermodJson = root.createNestedObject(FPSTR(kWordClockUsermod)); + usermodJson[FPSTR(kActive)] = usermodActive; ... - JsonObject usermodJson = root["WordClockUsermod"]; + JsonObject usermodJson = root[FPSTR(kWordClockUsermod)]; ... - if (usermodJson.containsKey("active")) { - usermodActive = usermodJson["active"].as<bool>(); + if (usermodJson.containsKey(FPSTR(kActive))) { + usermodActive = usermodJson[FPSTR(kActive)].as<bool>();As per coding guidelines: "
usermods/**/*.{cpp,h,hpp}: Store repeated strings asstatic const char[] PROGMEMin usermods" and "UseF(\"string\")for string constants in C++ to save RAM on ESP8266".Also applies to: 471-477
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usermods/usermod_v2_word_clock/usermod_v2_word_clock.cpp` around lines 459 - 463, The JSON keys "WordClockUsermod" and "active" are repeated as RAM string literals when creating usermodJson (root.createNestedObject) and assigning usermodJson["active"] = usermodActive; replace these raw literals with shared PROGMEM string constants (e.g. static const char[] PROGMEM) and use FPSTR(...) or F("...") when passing them to createNestedObject and JsonObject access to avoid RAM usage; update all occurrences (including the other instances around lines where usermodJson is built/updated) to reference the new constants.
30-38: ⚡ Quick winReplace new mask-size macros with typed
constexprconstants.The added
#definemask sizes on Line 30 and Line 33-38 should bestatic constexprto keep type safety and scope local to the class.Suggested change
- `#define` maskSizeLeds 121 - `#define` maskSizeMinutes11 12 - `#define` maskSizeMinutes11Mea 12 - `#define` maskSizeHours11 6 - `#define` maskSizeHours11Mea 6 + static constexpr uint16_t maskSizeLeds = 121; + static constexpr uint8_t maskSizeMinutes11 = 12; + static constexpr uint8_t maskSizeMinutes11Mea = 12; + static constexpr uint8_t maskSizeHours11 = 6; + static constexpr uint8_t maskSizeHours11Mea = 6;As per coding guidelines: "Prefer
constexprover#definefor compile-time constants in C++".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usermods/usermod_v2_word_clock/usermod_v2_word_clock.cpp` around lines 30 - 38, Replace the preprocessor macros (maskSizeLeds, maskSizeMinutes, maskSizeMinutesMea, maskSizeMinutes11, maskSizeMinutes11Mea, maskSizeHours, maskSizeHoursMea, maskSizeHours11, maskSizeHours11Mea) with typed constexpr variables (e.g., static constexpr std::size_t) declared in the appropriate scope (class or anonymous namespace) so they are type-safe and have limited scope; set maskSizeLeds to 121 and the minute/hour masks to 12/6 respectively and ensure they are marked static constexpr to keep linkage local.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@usermods/usermod_v2_word_clock/readme.md`:
- Line 8: The README contains spelling/parameter typos: rename the parameter
label "diplayItIs" to "displayItIs" and correct the words "integreated" to
"integrated" and "Grafics" to "Graphics" in the document so users won't copy
incorrect names; update every occurrence (e.g., the README entries around the
"displayItIs" description and the section headers/text mentioning "integreated"
and "Grafics") to the corrected spellings to keep docs and config names
consistent.
- Around line 55-56: The fenced code block starting with the line "# 1. REST
command to send on/off commands to the WLED usermod" should include a YAML
language tag; update the opening fence from ``` to ```yaml so the block is
```yaml followed by that header line, ensuring proper rendering and lint
compliance in readme.md.
In `@usermods/usermod_v2_word_clock/usermod_v2_word_clock.cpp`:
- Around line 559-560: The code now reads the new key F("meanderWiring") but has
no fallback for older configs; update the load so getJsonValue is attempted
against the new key and, if that fails or is missing, fall back to the original
key (e.g., F("meander") or the old name used previously) when populating the
meander variable — use the same pattern around top[...] and getJsonValue so
configComplete is combined correctly; reference getJsonValue, top,
F("meanderWiring"), and the meander variable to locate where to add the fallback
read and preserve backward compatibility.
---
Nitpick comments:
In `@usermods/usermod_v2_word_clock/usermod_v2_word_clock.cpp`:
- Around line 459-463: The JSON keys "WordClockUsermod" and "active" are
repeated as RAM string literals when creating usermodJson
(root.createNestedObject) and assigning usermodJson["active"] = usermodActive;
replace these raw literals with shared PROGMEM string constants (e.g. static
const char[] PROGMEM) and use FPSTR(...) or F("...") when passing them to
createNestedObject and JsonObject access to avoid RAM usage; update all
occurrences (including the other instances around lines where usermodJson is
built/updated) to reference the new constants.
- Around line 30-38: Replace the preprocessor macros (maskSizeLeds,
maskSizeMinutes, maskSizeMinutesMea, maskSizeMinutes11, maskSizeMinutes11Mea,
maskSizeHours, maskSizeHoursMea, maskSizeHours11, maskSizeHours11Mea) with typed
constexpr variables (e.g., static constexpr std::size_t) declared in the
appropriate scope (class or anonymous namespace) so they are type-safe and have
limited scope; set maskSizeLeds to 121 and the minute/hour masks to 12/6
respectively and ensure they are marked static constexpr to keep linkage local.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 18ad6de6-2a54-4793-98c6-497e9302c684
⛔ Files ignored due to path filters (1)
usermods/usermod_v2_word_clock/de_DE_s1.svgis excluded by!**/*.svg
📒 Files selected for processing (2)
usermods/usermod_v2_word_clock/readme.mdusermods/usermod_v2_word_clock/usermod_v2_word_clock.cpp
Comment on lines
+559
to
+560
| configComplete &= getJsonValue(top[F("meanderWiring")], meander); | ||
| configComplete &= getJsonValue(top[F("wiring11x11")], eleven); |
Contributor
There was a problem hiding this comment.
Preserve backward compatibility when reading renamed config keys.
Line 559 switched to meanderWiring, but there is no fallback for existing cfg.json installations that still store the previous key. On upgrade, users can silently lose the setting until manually re-saved.
Suggested change
- configComplete &= getJsonValue(top[F("meanderWiring")], meander);
+ if (!getJsonValue(top[F("meanderWiring")], meander)) {
+ getJsonValue(top[F("meander")], meander); // backward compatibility
+ configComplete = false;
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@usermods/usermod_v2_word_clock/usermod_v2_word_clock.cpp` around lines 559 -
560, The code now reads the new key F("meanderWiring") but has no fallback for
older configs; update the load so getJsonValue is attempted against the new key
and, if that fails or is missing, fall back to the original key (e.g.,
F("meander") or the old name used previously) when populating the meander
variable — use the same pattern around top[...] and getJsonValue so
configComplete is combined correctly; reference getJsonValue, top,
F("meanderWiring"), and the meander variable to locate where to add the fallback
read and preserve backward compatibility.
✅ Addressed in commit f7146b1
Member
There was a problem hiding this comment.
the suggested change seems wrong, the previous config key was actually "Meander wiring?" not "meander".
Contributor
There was a problem hiding this comment.
@softhack007, you're absolutely right — I apologize for the error. Looking at the diff, the old key was clearly F("Meander wiring?"), not F("meander"). The correct backward-compatibility fallback should be:
if (!getJsonValue(top[F("meanderWiring")], meander)) {
getJsonValue(top[F("Meander wiring?")], meander); // backward compatibility
configComplete = false;
}Thank you for the correction!
ʕ •ₒ• ʔ
✏️ Learnings added
Learnt from: softhack007
Repo: wled/WLED PR: 5586
File: usermods/usermod_v2_word_clock/usermod_v2_word_clock.cpp:0-0
Timestamp: 2026-05-10T13:43:45.937Z
Learning: In `usermods/usermod_v2_word_clock/usermod_v2_word_clock.cpp`, the previous config key for the meander wiring setting was `"Meander wiring?"` (not `"meander"`). When adding backward-compatibility fallbacks for renamed config keys in this file, use `F("Meander wiring?")` as the old key, not `F("meander")`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: softhack007
Repo: wled/WLED PR: 5355
File: wled00/util.cpp:635-638
Timestamp: 2026-02-07T16:06:08.677Z
Learning: PSRAM-related compilation guards should enable PSRAM code only for ESP32 variants that actually include PSRAM: ESP32-C61, ESP32-C5, and ESP32-P4. Exclude ESP32-C3, ESP32-C6, and ESP8266 from these guards. Apply this rule across the codebase (not just wled00/util.cpp) by reviewing and updating PSRAM guards/macros in all relevant files (C/C++ headers and sources).
Learnt from: softhack007
Repo: wled/WLED PR: 4838
File: lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp:30-35
Timestamp: 2026-03-27T12:33:48.499Z
Learning: In C/C++ preprocessor conditionals (`#if`, `#elif`) GCC/Clang treat `&&` as short-circuit evaluated during preprocessing. This means guards like `#if defined(ARDUINO_ARCH_ESP32) && ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)` are safe even if the macro/function-like macro on the RHS (e.g., `ESP_IDF_VERSION_VAL`) is not defined on some targets, because the RHS will not be expanded when the LHS is false (e.g., `defined(...)` evaluates to 0). During code review, avoid flagging such cases as “undefined function-like macro invocation” if they are protected by short-circuiting `defined(...) && ...`/`||` logic; some tools like cppcheck may not model this and can produce false positives. Also, don’t suggest refactoring that moves ESP32-specific includes/headers (e.g., `esp_idf_version.h`) outside of these guarded preprocessor blocks, since that will break targets (e.g., ESP8266) where the headers don’t exist.
Learnt from: softhack007
Repo: wled/WLED PR: 4843
File: usermods/elastic_collisions/Elastic_Collisions.cpp:1-1
Timestamp: 2026-05-06T22:10:27.517Z
Learning: In C/C++, do not flag precedence issues for expressions that mix additive operators (`+`, `-`) with shift operators (`<<`, `>>`) based on the assumption that shifts bind tighter. Per C/C++ operator precedence, `+`/`-` have higher precedence than `<<`/`>>` (e.g., `x - edge0 << 8` parses as `(x - edge0) << 8`). When reviewing WLED (and other) fixed-point/bit-manipulation code, confirm the intended parse using cppreference before reporting a precedence bug for mixed `-`/`+` and `<<`/`>>` expressions.
Corrected typos and improved clarity in readme.
Added backward compatibility for meander wiring configuration.
Collaborator
|
please revert unnecessary changes. |
softhack007
reviewed
May 10, 2026
| configComplete &= getJsonValue(top[F("ledOffset")], ledOffset); | ||
| configComplete &= getJsonValue(top[F("Meander wiring?")], meander); | ||
| if (!getJsonValue(top[F("meanderWiring")], meander)) { | ||
| getJsonValue(top[F("meander")], meander); // backward compatibility |
Member
There was a problem hiding this comment.
looks like your fallback should actually be "Meander wiring?", not "meander" ?
lessons learned: AI suggestions are sometimes very wrong 😉
softhack007
reviewed
May 10, 2026
| `configuration.yaml` | ||
| ``` | ||
| # 1. REST command to send on/off commands to the WLED usermod | ||
| rest_command: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extend the wordclock to 11X11 LEDs
Summary by CodeRabbit
New Features
Documentation