-
-
Notifications
You must be signed in to change notification settings - Fork 4k
New usermod for a 6-digit 7-Segment Clock with Countdown to date #5289
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdds a new WLED usermod implementing a six‑digit, two‑separator seven‑segment overlay for clock and countdown displays, including mask rendering, time/target handling, JSON/MQTT/config integration, and full Usermod lifecycle hooks. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @usermods/7_seg_clock_countdown/7_seg_clock_countdown.cpp:
- Around line 329-337: The current branch handling large day counts (the remDays
> 99 block using setDigitChar and setDigitInt) misrenders remDays >= 1000; add a
new branch that checks remDays >= 1000 before the remDays > 99 check and cap or
format those values (for example, display a capped "999d" or a special overflow
indicator like "OVR" or ">99d") by setting the four left digits via
setDigitChar/setDigitInt and the unit with setDigitChar('d'); ensure the logic
uses remDays >= 1000 as the new condition and leaves the existing remDays > 99
branch for 3-digit values.
- Around line 252-254: The code computes absDiff using abs(diff) which can
truncate int64_t; replace abs(diff) with a 64-bit-safe absolute call (e.g.,
llabs(diff) or std::abs<long long>(diff) / std::abs(static_cast<long
long>(diff))) so absDiff correctly holds the absolute value of the int64_t diff;
update the usage around the diff, countingUp, and absDiff variables accordingly.
- Around line 572-601: When handling MQTT updates for
targetYear/targetMonth/targetDay/targetHour/targetMinute the code sets the
individual variables but never calls validateTarget(), leaving targetUnix stale;
after each assignment to targetYear, targetMonth, targetDay, targetHour, or
targetMinute (the blocks shown) call validateTarget(true) to force recomputation
of targetUnix and related state so the countdown reflects the new values.
- Around line 552-564: Subscription and handler use different topic formats:
update SevenSegClockCountdown::onMqttConnect to subscribe with the device/topic
separator and wildcard (e.g., combine mqttDeviceTopic, a "/" separator,
MQTT_Topic and "/#") so it actually receives all 7seg subtopics, and update
SevenSegClockCountdown::onMqttMessage to check the same fully-qualified prefix
(e.g., String(mqttDeviceTopic) + "/" + MQTT_Topic) instead of "/7seg/"; ensure
both use the same separator convention and that the subscribe uses a trailing
"/#" wildcard to capture child topics.
In @usermods/7_seg_clock_countdown/7_seg_clock_countdown.h:
- Around line 218-221: clampVal currently returns uint8_t which truncates values
>=256 (breaking year clamping for targetYear); change clampVal's return type to
int (or make it a templated function) and update its declaration/definition
(static int clampVal(int v, int lo, int hi) or template<typename T> static T
clampVal(T v, T lo, T hi)) so it preserves full-range integers, and ensure any
call sites (e.g., where clampVal is used to clamp targetYear) accept the
adjusted return type.
🧹 Nitpick comments (7)
usermods/7_seg_clock_countdown/7_seg_clock_countdown.h (3)
1-3: Consider removing standalone<stdint.h>include.In WLED usermods,
wled.h(included in the .cpp file) already provides standard integer types. The static analysis error aboutstdint.hnot being found is a false positive in the WLED build context, but the include is likely redundant.Suggested change
#pragma once -#include <stdint.h> +// Types provided by wled.h in the implementation file
58-77: Consider computing digit bases from constants for maintainability.The hardcoded base indices (0, 35, 80, 115, 160, 195) are correct for the default geometry but will break if
LEDS_PER_SEGorSEP_LEDSchanges. Per the README, users must manually update these values. Consider computing them dynamically. Based on learnings, magic numbers should be replaced with expressions using defined constants.Example dynamic computation
static uint16_t digitBase(uint8_t d) { // Layout: D0 | D1 | SEP1 | D2 | D3 | SEP2 | D4 | D5 constexpr uint16_t D = LEDS_PER_DIGIT; // 35 constexpr uint16_t S = SEP_LEDS; // 10 switch (d) { case 0: return 0; case 1: return D; case 2: return 2*D + S; case 3: return 3*D + S; case 4: return 4*D + 2*S; case 5: return 5*D + 2*S; default: return 0; } }
105-106: Typo in variable names: "Seperator" → "Separator".
SeperatorOnandSeperatorOffare misspelled. Since these names appear in JSON state, config, and MQTT topics, this typo will persist in the public API. Consider fixing before the first release.usermods/7_seg_clock_countdown/7_seg_clock_countdown.cpp (2)
125-148: Separator logic is confusing; consider simplifying.The current logic:
SeperatorOn && SeperatorOff→ blink,SeperatorOnonly → on,SeperatorOffonly → off, neither → blink. The naming and conditions are counterintuitive. Consider using a single enum (BLINK,ON,OFF) or renaming toseparatorMode.
293-311: Consider usingelse iffor mutually exclusive conditions.Lines 293 and 302 have mutually exclusive conditions (
absDiff < 60andabsDiff >= 60) but are separateifblocks. Usingelse ifwould make the exclusivity explicit.usermods/7_seg_clock_countdown/README.md (2)
7-8: Use markdown link syntax for URL.Per markdownlint, bare URLs should be formatted as links for accessibility and consistent styling.
Suggested fix
- Active development happens in the WLED fork by DereIBims: - - https://github.com/DereIBims/WLED + - [github.com/DereIBims/WLED](https://github.com/DereIBims/WLED)
15-15: Remove spaces inside code span.The code span
` ddd d`has leading spaces inside the backticks, which markdownlint flags. If the leading space is intentional to show display padding, consider using a non-breaking space or a clearer format.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
usermods/7_seg_clock_countdown/7_seg_clock_countdown.cppusermods/7_seg_clock_countdown/7_seg_clock_countdown.husermods/7_seg_clock_countdown/README.mdusermods/7_seg_clock_countdown/library.json
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: willmmiles
Repo: wled/WLED PR: 4623
File: usermods/word-clock-matrix/word-clock-matrix.cpp:332-332
Timestamp: 2025-03-29T01:22:54.617Z
Learning: In the WLED project, hardcoded usermod IDs (like 500 in the WordClockMatrix's getId() method) are intentionally used as part of a strategy to avoid modifying core code. There are plans to remove numeric usermod IDs entirely in the future.
Learnt from: freakintoddles2
Repo: wled/WLED PR: 4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Learnt from: DedeHai
Repo: wled/WLED PR: 4923
File: wled00/FX.cpp:4883-4901
Timestamp: 2025-09-12T17:29:43.826Z
Learning: In WLED’s web UI, only one slider value (e.g., SEGMENT.intensity or SEGMENT.custom1) changes at a time; code relying on this may use simplified change guards, though presets/JSON can still update multiple fields atomically.
Learnt from: 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: 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: KrX3D
Repo: wled/WLED PR: 4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.h:121-137
Timestamp: 2025-04-27T10:06:22.545Z
Learning: In the Seven Segment Display Reloaded usermod, the dimension mismatch between the default array (umSSDRNumbers[11][7]) and the override (umSSDRNumbers[11][10] = umSSDR_NUMBERS) is intentional by design, allowing for different use cases with varying numbers of segments per digit.
📚 Learning: 2025-09-02T19:42:57.475Z
Learnt from: freakintoddles2
Repo: wled/WLED PR: 4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Applied to files:
usermods/7_seg_clock_countdown/README.mdusermods/7_seg_clock_countdown/7_seg_clock_countdown.husermods/7_seg_clock_countdown/7_seg_clock_countdown.cpp
📚 Learning: 2025-03-29T01:22:54.617Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4623
File: usermods/word-clock-matrix/word-clock-matrix.cpp:332-332
Timestamp: 2025-03-29T01:22:54.617Z
Learning: In the WLED project, hardcoded usermod IDs (like 500 in the WordClockMatrix's getId() method) are intentionally used as part of a strategy to avoid modifying core code. There are plans to remove numeric usermod IDs entirely in the future.
Applied to files:
usermods/7_seg_clock_countdown/README.mdusermods/7_seg_clock_countdown/7_seg_clock_countdown.husermods/7_seg_clock_countdown/7_seg_clock_countdown.cpp
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Applied to files:
usermods/7_seg_clock_countdown/README.mdusermods/7_seg_clock_countdown/7_seg_clock_countdown.cpp
📚 Learning: 2025-10-20T09:38:51.997Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4995
File: wled00/FX.cpp:5223-5226
Timestamp: 2025-10-20T09:38:51.997Z
Learning: WLED matrices: each dimension (SEG_W, SEG_H) is limited to ≤255; 256 or larger per side is not supported/feasible on ESP32, so effects should assume per-dimension max 255.
Applied to files:
usermods/7_seg_clock_countdown/README.md
📚 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:
usermods/7_seg_clock_countdown/README.mdusermods/7_seg_clock_countdown/7_seg_clock_countdown.cpp
📚 Learning: 2025-04-27T10:06:22.545Z
Learnt from: KrX3D
Repo: wled/WLED PR: 4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.h:121-137
Timestamp: 2025-04-27T10:06:22.545Z
Learning: In the Seven Segment Display Reloaded usermod, the dimension mismatch between the default array (umSSDRNumbers[11][7]) and the override (umSSDRNumbers[11][10] = umSSDR_NUMBERS) is intentional by design, allowing for different use cases with varying numbers of segments per digit.
Applied to files:
usermods/7_seg_clock_countdown/README.mdusermods/7_seg_clock_countdown/7_seg_clock_countdown.husermods/7_seg_clock_countdown/7_seg_clock_countdown.cpp
📚 Learning: 2025-04-27T09:37:28.415Z
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.
Applied to files:
usermods/7_seg_clock_countdown/7_seg_clock_countdown.cpp
📚 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:
usermods/7_seg_clock_countdown/library.json
🧬 Code graph analysis (2)
usermods/7_seg_clock_countdown/7_seg_clock_countdown.h (1)
usermods/7_seg_clock_countdown/7_seg_clock_countdown.cpp (52)
ensureMaskSize(28-32)ensureMaskSize(28-28)clearMask(35-38)clearMask(35-35)setRangeOn(41-49)setRangeOn(41-41)getHundredths(53-77)getHundredths(53-53)drawClock(115-149)drawClock(115-115)revertMode(81-89)revertMode(81-81)SaveMode(92-100)SaveMode(92-92)setMode(103-111)setMode(103-103)drawCountdown(249-374)drawCountdown(249-249)setDigitInt(152-170)setDigitInt(152-152)setDigitChar(173-189)setDigitChar(173-173)setSeparator(192-197)setSeparator(192-192)setSeparatorHalf(200-207)setSeparatorHalf(200-200)applyMaskToStrip(210-219)applyMaskToStrip(210-210)validateTarget(223-245)validateTarget(223-223)setup(16-19)setup(16-16)loop(22-24)loop(22-22)handleOverlayDraw(378-400)handleOverlayDraw(378-378)addToJsonInfo(404-443)addToJsonInfo(404-404)addToJsonState(446-462)addToJsonState(446-446)readFromJsonState(465-510)readFromJsonState(465-465)addToConfig(514-528)addToConfig(514-514)readFromConfig(531-548)readFromConfig(531-531)onMqttConnect(552-557)onMqttConnect(552-552)onMqttMessage(560-633)onMqttMessage(560-560)getId(637-637)getId(637-637)
usermods/7_seg_clock_countdown/7_seg_clock_countdown.cpp (2)
wled00/bus_manager.h (1)
start(79-83)usermods/7_seg_clock_countdown/7_seg_clock_countdown.h (5)
digitBase(58-77)LETTER_MASK(141-216)sep1Base(78-78)sep2Base(79-79)clampVal(218-221)
🪛 Clang (14.0.6)
usermods/7_seg_clock_countdown/7_seg_clock_countdown.h
[error] 2-2: 'stdint.h' file not found
(clang-diagnostic-error)
🪛 LanguageTool
usermods/7_seg_clock_countdown/README.md
[grammar] ~10-~10: Ensure spelling is correct
Context: ... - https://github.com/DereIBims/WLED A usermod that renders a six‑digit, two‑separator...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~117-~117: Ensure spelling is correct
Context: ... enabled - showClock - showCountdown - SeperatorOn - SeperatorOff - Numeric keys (payload: integer): - tar...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~150-~150: Ensure spelling is correct
Context: ...d "false"/"0". ## UI integration - The usermod adds a compact block to the “Info” scre...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
usermods/7_seg_clock_countdown/README.md
8-8: Bare URL used
(MD034, no-bare-urls)
15-15: Spaces inside code span elements
(MD038, no-space-in-code)
🔇 Additional comments (9)
usermods/7_seg_clock_countdown/7_seg_clock_countdown.h (2)
141-215: Verify behavior for character 'x' in countdown display.The
drawCountdown()method usessetDigitChar(_, 'x')as a placeholder, but 'x' is not handled inLETTER_MASK()and falls through to the default mask (A, D, G segments lit). If this is intentional placeholder behavior, consider adding an explicit case or a comment. If 'x' should display differently (e.g., blank), add the appropriate case.
112-117: Member initialization useslocalTimebefore it's valid.
targetYear = year(localTime)etc. are evaluated at static construction time, whenlocalTimeis likely 0 (epoch 1970). WhilereadFromConfig()provides fallback defaults, the in-class initializers may produce unexpected values. Consider using fixed defaults (e.g.,2025) or deferring initialization.usermods/7_seg_clock_countdown/7_seg_clock_countdown.cpp (4)
7-8: Include path uses local header correctly.The include of
"7_seg_clock_countdown.h"follows the expected pattern for WLED usermods.
53-77: LGTM: Hundredths calculation with proper clamping.The logic correctly computes sub-second hundredths for smooth countdown display, handles both countdown and count-up modes, and clamps the result to 0–99.
378-400: Overlay draw logic is correct.The alternating display logic using
localTime / periodto switch between clock and countdown is clean and functional.
637-640: Usermod registration follows WLED conventions.The static instance and
REGISTER_USERMODmacro usage follows the standard WLED usermod pattern. The hardcoded ID0x22B8is acceptable per project learnings about usermod IDs.usermods/7_seg_clock_countdown/library.json (1)
10-11: Verify ESP8266 compatibility.The manifest only targets
espressif32. If this usermod also supports ESP8266, addespressif8266to the platforms list. If ESP32-only is intentional (e.g., due to memory constraints), consider documenting this in the README.usermods/7_seg_clock_countdown/README.md (2)
49-51: Core tweak requirement may cause maintenance burden.Requiring users to modify
wled00/FX.cpp::mode_static()to returnFRAMETIMEis fragile—it may be overwritten by WLED updates and requires users to maintain a patched fork. Consider documenting this as optional (hundredths may lag slightly) or exploring alternative approaches.
1-170: Comprehensive and well-structured documentation.The README thoroughly documents panel geometry, overlay mechanism, build instructions, runtime configuration, JSON/MQTT APIs, and limitations. This is excellent for user onboarding.
|
thanks for contributing. |
|
To be honest I didn't consider extending the existing mod. But no problem if you don't want to merge it to the official code base, then I'll just keep it available in my fork :) |
There was a problem hiding this 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
🤖 Fix all issues with AI agents
In `@usermods/7_seg_clock_countdown/README.md`:
- Around line 60-62: Fix the README typos where the documented option names are
misspelled as "SeperatorOn" and "SeperatorOff": change them to the correct
"SeparatorOn" and "SeparatorOff" to match the code (references: SeparatorOn,
SeparatorOff) and update the JSON and MQTT examples that use those keys so they
use the correct spelling as well.
♻️ Duplicate comments (1)
usermods/7_seg_clock_countdown/7_seg_clock_countdown.cpp (1)
492-497: MQTT message handling prefix mismatch - incoming topics don't start with/.The subscription (line 487) is correct:
{mqttDeviceTopic}/7seg/#. However, the message handler builds a prefix with a leading/(line 494), which won't match incoming topics.For example:
- Subscribed topic:
wled/mystrip/7seg/#- Incoming message topic:
wled/mystrip/7seg/enabled- Built prefix:
/wled/mystrip/← leading slash causes mismatchProposed fix
bool SevenSegClockCountdown::onMqttMessage(char *topic, char *payload) { String topicStr = String(topic); - String prefix = "/"; prefix += mqttDeviceTopic; prefix += "/"; + String prefix = String(mqttDeviceTopic) + "/" + MQTT_Topic + "/"; if (!topicStr.startsWith(prefix)) return false; - String subTopic = topicStr.substring(strlen(prefix.c_str())); + String subTopic = topicStr.substring(prefix.length());
🧹 Nitpick comments (5)
usermods/7_seg_clock_countdown/7_seg_clock_countdown.h (2)
111-116: Initialization fromlocalTimeat class construction may be unreliable.Member initializers
targetYear = year(localTime),targetMonth = month(localTime),targetDay = day(localTime)execute at static construction time, before WLED initializeslocalTime. This may result in garbage or epoch values. These fields are subsequently overwritten byreadFromConfig(), so this is likely benign, but consider initializing to safe defaults (e.g.,2025,1,1) for clarity.Suggested default initialization
- int targetYear = year(localTime); - uint8_t targetMonth = month(localTime); - uint8_t targetDay = day(localTime); + int targetYear = 2025; + uint8_t targetMonth = 1; + uint8_t targetDay = 1;
77-78: Consider computing separator bases from geometry constants.
sep1Base()andsep2Base()return magic numbers (70, 150) that must manually match the digit/separator layout. Computing these fromLEDS_PER_DIGITandSEP_LEDSwould eliminate manual synchronization when geometry changes.Derived separator bases
- static constexpr uint16_t sep1Base() { return 70; } - static constexpr uint16_t sep2Base() { return 150; } + // sep1 after digits 0,1: 2 * LEDS_PER_DIGIT = 70 + static constexpr uint16_t sep1Base() { return VERY_FIRST_LED + 2 * LEDS_PER_DIGIT; } + // sep2 after digits 2,3 and sep1: 4 * LEDS_PER_DIGIT + SEP_LEDS = 150 + static constexpr uint16_t sep2Base() { return VERY_FIRST_LED + 4 * LEDS_PER_DIGIT + SEP_LEDS; }usermods/7_seg_clock_countdown/7_seg_clock_countdown.cpp (2)
254-262: Potential display issue for count-up between 60-99 minutes.When
absDiff >= 60(seconds), the display showsxxMM:SS. However, the minute calculationabsDiff / 600at line 257 computes tens of minutes (since 600 = 60 seconds × 10), but this only works correctly ifabsDiff < 6000(100 minutes). Since the code falls back to clock after 3600 seconds (line 218), this is safe, but the logic is subtle.Consider adding a comment clarifying that this branch only executes when
60 <= absDiff < 3600.
110-124: Separator logic is functional but confusing.The combination of
SeparatorOnandSeparatorOffflags is counterintuitive:
- Both
trueor bothfalse→ blinking- Only
SeparatorOntrue → always on- Only
SeparatorOfftrue → always offA single enum or mode field (e.g.,
separatorMode: { BLINK, ALWAYS_ON, ALWAYS_OFF }) would be clearer. This is a design observation—no change required if you prefer the current approach.usermods/7_seg_clock_countdown/README.md (1)
50-51: Document the core modification requirement clearly.The required
mode_static()change inFX.cppis a significant modification to WLED core. Consider noting that this change affects all WLED users of the build, not just this usermod, and may have minor performance implications (continuous rendering even for static effects).Is this core change acceptable for the WLED project? Maintainers may prefer the usermod work without core modifications, even if hundredths updates are less smooth.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
usermods/7_seg_clock_countdown/7_seg_clock_countdown.cppusermods/7_seg_clock_countdown/7_seg_clock_countdown.husermods/7_seg_clock_countdown/README.md
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: willmmiles
Repo: wled/WLED PR: 4623
File: usermods/word-clock-matrix/word-clock-matrix.cpp:332-332
Timestamp: 2025-03-29T01:22:54.617Z
Learning: In the WLED project, hardcoded usermod IDs (like 500 in the WordClockMatrix's getId() method) are intentionally used as part of a strategy to avoid modifying core code. There are plans to remove numeric usermod IDs entirely in the future.
Learnt from: freakintoddles2
Repo: wled/WLED PR: 4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Learnt from: KrX3D
Repo: wled/WLED PR: 4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.h:121-137
Timestamp: 2025-04-27T10:06:22.545Z
Learning: In the Seven Segment Display Reloaded usermod, the dimension mismatch between the default array (umSSDRNumbers[11][7]) and the override (umSSDRNumbers[11][10] = umSSDR_NUMBERS) is intentional by design, allowing for different use cases with varying numbers of segments per digit.
📚 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:
usermods/7_seg_clock_countdown/README.mdusermods/7_seg_clock_countdown/7_seg_clock_countdown.cpp
📚 Learning: 2025-03-29T01:22:54.617Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4623
File: usermods/word-clock-matrix/word-clock-matrix.cpp:332-332
Timestamp: 2025-03-29T01:22:54.617Z
Learning: In the WLED project, hardcoded usermod IDs (like 500 in the WordClockMatrix's getId() method) are intentionally used as part of a strategy to avoid modifying core code. There are plans to remove numeric usermod IDs entirely in the future.
Applied to files:
usermods/7_seg_clock_countdown/README.mdusermods/7_seg_clock_countdown/7_seg_clock_countdown.husermods/7_seg_clock_countdown/7_seg_clock_countdown.cpp
📚 Learning: 2025-09-02T19:42:57.475Z
Learnt from: freakintoddles2
Repo: wled/WLED PR: 4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Applied to files:
usermods/7_seg_clock_countdown/README.mdusermods/7_seg_clock_countdown/7_seg_clock_countdown.husermods/7_seg_clock_countdown/7_seg_clock_countdown.cpp
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Applied to files:
usermods/7_seg_clock_countdown/README.mdusermods/7_seg_clock_countdown/7_seg_clock_countdown.cpp
📚 Learning: 2025-10-20T09:38:51.997Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4995
File: wled00/FX.cpp:5223-5226
Timestamp: 2025-10-20T09:38:51.997Z
Learning: WLED matrices: each dimension (SEG_W, SEG_H) is limited to ≤255; 256 or larger per side is not supported/feasible on ESP32, so effects should assume per-dimension max 255.
Applied to files:
usermods/7_seg_clock_countdown/README.md
📚 Learning: 2026-01-13T03:07:13.582Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5293
File: usermods/user_fx/user_fx.cpp:167-171
Timestamp: 2026-01-13T03:07:13.582Z
Learning: In WLED effects (usermods/user_fx/user_fx.cpp and similar), when storing future timestamps, assignments like `stored_time = now + delay` are correct and handle uint32_t wraparound properly. Only comparison statements need to use subtraction-based elapsed-time checks (e.g., `(now - stored_time) >= delay`). Do not flag assignment statements as needing fixes for wraparound handling.
Applied to files:
usermods/7_seg_clock_countdown/README.mdusermods/7_seg_clock_countdown/7_seg_clock_countdown.husermods/7_seg_clock_countdown/7_seg_clock_countdown.cpp
📚 Learning: 2025-04-27T10:06:22.545Z
Learnt from: KrX3D
Repo: wled/WLED PR: 4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.h:121-137
Timestamp: 2025-04-27T10:06:22.545Z
Learning: In the Seven Segment Display Reloaded usermod, the dimension mismatch between the default array (umSSDRNumbers[11][7]) and the override (umSSDRNumbers[11][10] = umSSDR_NUMBERS) is intentional by design, allowing for different use cases with varying numbers of segments per digit.
Applied to files:
usermods/7_seg_clock_countdown/README.mdusermods/7_seg_clock_countdown/7_seg_clock_countdown.husermods/7_seg_clock_countdown/7_seg_clock_countdown.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:
usermods/7_seg_clock_countdown/7_seg_clock_countdown.husermods/7_seg_clock_countdown/7_seg_clock_countdown.cpp
📚 Learning: 2025-04-27T09:37:28.415Z
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.
Applied to files:
usermods/7_seg_clock_countdown/7_seg_clock_countdown.cpp
📚 Learning: 2025-11-16T19:40:46.260Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4926
File: wled00/FX.cpp:4727-4730
Timestamp: 2025-11-16T19:40:46.260Z
Learning: WLED AuroraWave (wled00/FX.cpp): wave_start and wave_end intentionally use int16_t; segments longer than 32k LEDs are not supported (bounded by MAX_LEDS), so widening to 32-bit is unnecessary.
Applied to files:
usermods/7_seg_clock_countdown/7_seg_clock_countdown.cpp
📚 Learning: 2025-09-02T19:36:49.513Z
Learnt from: freakintoddles2
Repo: wled/WLED PR: 4904
File: wled00/FX.cpp:3901-3913
Timestamp: 2025-09-02T19:36:49.513Z
Learning: In FX.cpp mode_percent time-based options (% of Minute/Hour/Day), do not guard against invalid RTC; showing a full bar with unsynced time is intentional to avoid confusing UX.
Applied to files:
usermods/7_seg_clock_countdown/7_seg_clock_countdown.cpp
📚 Learning: 2025-04-18T22:27:58.634Z
Learnt from: KrX3D
Repo: wled/WLED PR: 4237
File: usermods/INA219_v2/INA219_v2.cpp:265-276
Timestamp: 2025-04-18T22:27:58.634Z
Learning: When implementing MQTT message handling in WLED usermods, use `strstr(topic, "/specific/path")` instead of `strcmp_P(topic, PSTR("/specific/path"))` to properly match topics that include the device prefix. The full MQTT topic typically follows the pattern `<mqttDeviceTopic>/specific/path`.
Applied to files:
usermods/7_seg_clock_countdown/7_seg_clock_countdown.cpp
📚 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:
usermods/7_seg_clock_countdown/7_seg_clock_countdown.cpp
🪛 LanguageTool
usermods/7_seg_clock_countdown/README.md
[grammar] ~38-~38: Ensure spelling is correct
Context: .... ## How it works (overlay mask) - The usermod builds a per‑LED mask (vector of 0/1). ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~151-~151: Ensure spelling is correct
Context: ...d "false"/"0". ## UI integration - The usermod adds a compact block to the “Info” scre...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (6)
usermods/7_seg_clock_countdown/7_seg_clock_countdown.h (1)
140-213: LGTM!The
LETTER_MASKfunction provides comprehensive character-to-segment mapping with a sensible fallback. The bitmask approach is clean and efficient.usermods/7_seg_clock_countdown/7_seg_clock_countdown.cpp (4)
215-217: LGTM - int64_t absolute value handled correctly.The manual absolute value computation
(diff < 0) ? -diff : diffcorrectly handles 64-bit values without truncation.
278-284: LGTM - overflow handling for ≥1000 days.The display now correctly shows "o999d " for countdowns exceeding 999 days, addressing the previous overflow concern.
388-403: LGTM - JSON state serialization.State serialization is complete and correctly uses the
7segnamespace.
561-566: LGTM - Usermod registration.The static instance and
REGISTER_USERMODpattern follows WLED conventions. The hardcoded ID0x22B8is consistent with the project's usermod ID strategy. Based on learnings, hardcoded usermod IDs are intentionally used in WLED.usermods/7_seg_clock_countdown/README.md (1)
1-171: LGTM - Comprehensive documentation.The README provides thorough documentation covering features, configuration, JSON/MQTT APIs, and limitations. The separator typo aside, this is well-written and will help users configure the usermod.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| - `SeperatorOn` (bool): Force separators on in clock mode | ||
| - `SeperatorOff` (bool): Force separators off in clock mode | ||
| - Both true or both false → blinking separators once per second |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: "Seperator" should be "Separator" to match code.
The README documents SeperatorOn and SeperatorOff, but the code uses SeparatorOn and SeparatorOff. This mismatch will cause JSON/MQTT API calls using the documented names to fail silently.
Fix typos in README
-- `SeperatorOn` (bool): Force separators on in clock mode
-- `SeperatorOff` (bool): Force separators off in clock mode
+- `SeparatorOn` (bool): Force separators on in clock mode
+- `SeparatorOff` (bool): Force separators off in clock modeAlso update JSON examples at lines 82-83, and MQTT examples at lines 119-120, 135-136.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - `SeperatorOn` (bool): Force separators on in clock mode | |
| - `SeperatorOff` (bool): Force separators off in clock mode | |
| - Both true or both false → blinking separators once per second | |
| - `SeparatorOn` (bool): Force separators on in clock mode | |
| - `SeparatorOff` (bool): Force separators off in clock mode | |
| - Both true or both false → blinking separators once per second |
🤖 Prompt for AI Agents
In `@usermods/7_seg_clock_countdown/README.md` around lines 60 - 62, Fix the
README typos where the documented option names are misspelled as "SeperatorOn"
and "SeperatorOff": change them to the correct "SeparatorOn" and "SeparatorOff"
to match the code (references: SeparatorOn, SeparatorOff) and update the JSON
and MQTT examples that use those keys so they use the correct spelling as well.
my usermod for the 7 Segment clock/countdown.
Viedeos and photos can be found in the Discord-channel "showcase"
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.