Usermod: Add SkyStrip weather forecast usermod #4883
Conversation
WalkthroughAdds a new SkyStrip usermod: time-series SkyModel, generic interfaces, utilities, a REST JSON client, OpenWeatherMap data source, five LED visualization views (Cloud, Wind, Temperature, 24‑hour Delta, Test Pattern), README/FAQ, and build manifest; all include configuration, debug hooks, and view/source lifecycle handling. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 24
🧹 Nitpick comments (31)
usermods/usermod_v2_skystrip/util.cpp (1)
6-22: Optional: consider FastLED’s CHSV->CRGB for speed/flash size.If FastLED is available in this TU, its integer HSV conversion avoids floats on MCUs.
usermods/usermod_v2_skystrip/rest_json_client.h (1)
16-24: Document ownership/lifetime of returned JSON and rate-limit behavior.Callers should not retain the pointer across calls; clarify in comment.
Apply:
- // Returns pointer to internal document on success, nullptr on failure. + // Returns a pointer to an internal document on success (invalidated by the next call), nullptr on failure.usermods/usermod_v2_skystrip/interfaces.h (3)
27-31: Mark return values as [[nodiscard]] to prevent accidental dropsfetch() and checkhistory() results are easy to ignore. Add [[nodiscard]].
- virtual std::unique_ptr<ModelType> fetch(std::time_t now) = 0; + [[nodiscard]] virtual std::unique_ptr<ModelType> fetch(std::time_t now) = 0; @@ - virtual std::unique_ptr<ModelType> checkhistory(std::time_t now, std::time_t oldestTstamp) = 0; + [[nodiscard]] virtual std::unique_ptr<ModelType> checkhistory(std::time_t now, std::time_t oldestTstamp) = 0;
35-37: Prefer const char over std::string for name() on MCUs*Avoids small heap churn and code size on ESP targets. This is an API change across implementers.
- virtual std::string name() const = 0; + virtual const char* name() const = 0;And similarly in IDataViewT (see below).
49-51: Apply same name() return-type change for views- virtual std::string name() const = 0; + virtual const char* name() const = 0;usermods/usermod_v2_skystrip/temperature_view.h (1)
3-7: Remove redundant include to speed up buildsForward declaration is sufficient; drop the heavy skymodel.h include from the header.
#include "interfaces.h" -#include "skymodel.h" class SkyModel;usermods/usermod_v2_skystrip/delta_view.h (1)
3-7: Remove redundant include to speed up buildsForward declare SkyModel; avoid including skymodel.h in the header.
#include "interfaces.h" -#include "skymodel.h" class SkyModel;usermods/usermod_v2_skystrip/wind_view.h (2)
3-7: Remove redundant include to speed up buildsHeader doesn’t need the full SkyModel definition.
#include "interfaces.h" -#include "skymodel.h" class SkyModel;
13-16: Align with proposed const char name() interface (if adopted)*If interfaces.h switches to const char*, update this signature accordingly.
- std::string name() const override { return "WV"; } + const char* name() const override { return "WV"; }usermods/usermod_v2_skystrip/cloud_view.h (2)
3-7: Drop unnecessary include to reduce compile couplingYou both include and forward-declare SkyModel. Keep the forward declaration and remove the include to speed up builds and cut dependencies.
-#include "skymodel.h" +// forward-declared below to reduce coupling
24-27: Initialize segId_ to a safe default in the constructorPlease ensure CloudView() sets segId_ to the disabled sentinel (e.g., -1) so view() early-outs until configured.
If not already done in cloud_view.cpp, I can provide a one-line ctor init.
usermods/usermod_v2_skystrip/test_pattern_view.h (1)
3-7: Prefer forward declaration; remove heavy header includeYou include skymodel.h and also forward-declare SkyModel. Keep the forward declaration and drop the include to reduce compile time.
-#include "skymodel.h" +// forward-declared below to reduce couplingusermods/usermod_v2_skystrip/usermod_v2_skystrip.h (3)
41-41: Minor style: extraneous semicolon inside method bodyTrailing semicolon after return is unnecessary.
- uint16_t getId() override { return USERMOD_ID_SKYSTRIP; }; + uint16_t getId() override { return USERMOD_ID_SKYSTRIP; }
44-45: Make accessor const-correctisEnabled doesn’t mutate state; mark it const.
- inline bool isEnabled() { return enabled_; } + inline bool isEnabled() const { return enabled_; }
23-23: Remove unused fieldlastLoop_in usermods/usermod_v2_skystrip/usermod_v2_skystrip.h:23 to reclaim RAM on MCUs. If you need it for future functionality, annotate it with[[maybe_unused]]instead.usermods/usermod_v2_skystrip/rest_json_client.cpp (3)
30-35: Insecure TLS; consider pinning or cert storesetInsecure() disables certificate validation. If feasible, support certificate/fingerprint pinning or a lightweight trust store behind a config flag.
Do you want a follow-up patch to add SHA-256 fingerprint pinning for api.openweathermap.org?
20-21: Store constant debug strings in flashWrap constant strings with F() to reduce RAM usage.
- DEBUG_PRINTLN("SkyStrip: RestJsonClient::getJson: RATE LIMITED"); + DEBUG_PRINTLN(F("SkyStrip: RestJsonClient::getJson: RATE LIMITED"));- DEBUG_PRINTLN(F("SkyStrip: RestJsonClient::getJson: trouble initiating request")); + DEBUG_PRINTLN(F("SkyStrip: RestJsonClient::getJson: trouble initiating request"));Also applies to: 40-41
15-16: Guard against null/empty URLAvoid undefined behavior on bad input.
DynamicJsonDocument* RestJsonClient::getJson(const char* url) { + if (!url || !url[0]) return nullptr;usermods/usermod_v2_skystrip/test_pattern_view.cpp (2)
85-85: Store ctor debug string in flashUse F() to keep the string literal out of RAM.
- DEBUG_PRINTLN("SkyStrip: TP::CTOR"); + DEBUG_PRINTLN(F("SkyStrip: TP::CTOR"));
91-131: Optional: mark unused parameter to silence warningsmodel is currently unused; add (void)model; to document intent.
void TestPatternView::view(time_t now, SkyModel const &model, int16_t dbgPixelIndex) { + (void)model;usermods/usermod_v2_skystrip/skymodel.h (2)
41-42: Use a compact type for precip type (enum/uint8_t) instead of floating valueThese are discrete 0–3 codes; storing as float wastes space and invites accidental fractional values. Consider a dedicated struct or pack into uint8_t.
34-34: Keep time_t usage consistentMixing std::time_t and time_t is needless divergence in this header; use one consistently (the rest of the code uses time_t).
Apply:
- std::time_t lcl_tstamp{0}; // update timestamp from our clock + time_t lcl_tstamp{0}; // update timestamp from our clockusermods/usermod_v2_skystrip/skymodel.cpp (1)
76-87: Remove or mark unused parameter now in invalidate_historynow is unused; mark it to silence warnings or drop the param.
Apply:
-void SkyModel::invalidate_history(time_t now) { +void SkyModel::invalidate_history(time_t /*now*/) {usermods/usermod_v2_skystrip/delta_view.cpp (1)
88-96: De-duplicate lastDebug; two separate statics defeat throttling and waste RAMDeclare once before the loop and reuse in both debug blocks.
Apply:
@@ - for (uint16_t i = 0; i < len; ++i) { + static time_t lastDebug = 0; + for (uint16_t i = 0; i < len; ++i) { @@ - if (dbgPixelIndex >= 0) { - static time_t lastDebug = 0; + if (dbgPixelIndex >= 0) { if (now - lastDebug > 1 && i == dbgPixelIndex) { @@ - if (dbgPixelIndex >= 0) { - static time_t lastDebug = 0; + if (dbgPixelIndex >= 0) { if (now - lastDebug > 1 && i == dbgPixelIndex) {Also applies to: 100-115, 136-152
usermods/usermod_v2_skystrip/usermod_v2_skystrip.cpp (1)
119-125: Skip overlay draw when disabled or OFFAvoid unnecessary per-frame work and accidental writes when off/disabled.
Apply:
void SkyStrip::handleOverlayDraw() { - // this happens a hundred times a second + // this happens a hundred times a second + if (!enabled_ || offMode) return; time_t now = skystrip::util::time_now_utc(); for (auto &view : views_) { view->view(now, *model_, dbgPixelIndex_); } }usermods/usermod_v2_skystrip/cloud_view.cpp (2)
103-106: Comment and constant disagree.kNightVMax is equal to kDayVMax, but the comment says “slightly higher night maximum”. Align the comment or adjust the value.
-// slightly higher night maximum so low clouds are more visible -constexpr float kNightVMax= 0.40f; +// night maximum equals day; adjust to taste if you want more low-cloud visibility +constexpr float kNightVMax= 0.40f;
115-124: Silent skip on missing cloud data leads to black pixels.When
estimateCloudAtfails, the loopcontinues and leaves pixels unchanged for that position, which can create stale colors on subsequent frames with differentlen. Consider explicitly writing black or a “no data” color.usermods/usermod_v2_skystrip/open_weather_map_source.h (1)
33-43: Clarify units and defaults in comments for config fields.Document
units=imperialassumption incomposeApiUrland whetherintervalSec_bounds are enforced elsewhere.usermods/usermod_v2_skystrip/temperature_view.cpp (1)
141-149: Marker blending dims near markers; confirm UX.
color_blend(col, 0, blend)darkens toward black; if the intent was to brighten markers, blend toward a marker color or boost V instead.Alternative:
- col = color_blend(col, 0, blend); + const uint32_t mark = skystrip::util::hsv2rgb(0.f, 0.f, 0.85f); // bright white + col = color_blend(col, mark, blend);Also applies to: 151-155
usermods/usermod_v2_skystrip/util.h (2)
24-31: Local time formatting hack is sound; add a brief note.Using
gmtime_ron pre-offset seconds avoids TZ/DST complexities in WLED; consider a one-line comment to prevent future “fixes.”- gmtime_r(&local_sec, &tmLocal); // local_sec is already local seconds + gmtime_r(&local_sec, &tmLocal); // Intentionally using gmtime_r: local_sec already includes UTC→local offset.
41-73:estimateAtignoresstepparameter.Either document why
stepis unused or remove it from the signature to avoid confusion. If future smoothing uses it, add a TODO.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (25)
usermods/usermod_v2_skystrip/FAQ.md(1 hunks)usermods/usermod_v2_skystrip/cloud_view.cpp(1 hunks)usermods/usermod_v2_skystrip/cloud_view.h(1 hunks)usermods/usermod_v2_skystrip/delta_view.cpp(1 hunks)usermods/usermod_v2_skystrip/delta_view.h(1 hunks)usermods/usermod_v2_skystrip/interfaces.h(1 hunks)usermods/usermod_v2_skystrip/library.json(1 hunks)usermods/usermod_v2_skystrip/open_weather_map_source.cpp(1 hunks)usermods/usermod_v2_skystrip/open_weather_map_source.h(1 hunks)usermods/usermod_v2_skystrip/readme.md(1 hunks)usermods/usermod_v2_skystrip/rest_json_client.cpp(1 hunks)usermods/usermod_v2_skystrip/rest_json_client.h(1 hunks)usermods/usermod_v2_skystrip/skymodel.cpp(1 hunks)usermods/usermod_v2_skystrip/skymodel.h(1 hunks)usermods/usermod_v2_skystrip/temperature_view.cpp(1 hunks)usermods/usermod_v2_skystrip/temperature_view.h(1 hunks)usermods/usermod_v2_skystrip/test_pattern_view.cpp(1 hunks)usermods/usermod_v2_skystrip/test_pattern_view.h(1 hunks)usermods/usermod_v2_skystrip/usermod_v2_skystrip.cpp(1 hunks)usermods/usermod_v2_skystrip/usermod_v2_skystrip.h(1 hunks)usermods/usermod_v2_skystrip/util.cpp(1 hunks)usermods/usermod_v2_skystrip/util.h(1 hunks)usermods/usermod_v2_skystrip/wind_view.cpp(1 hunks)usermods/usermod_v2_skystrip/wind_view.h(1 hunks)wled00/const.h(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/{*.cpp,!(html_*)*.h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ source and header files
Files:
wled00/const.h
🧠 Learnings (1)
📚 Learning: 2025-03-29T01:22:54.617Z
Learnt from: willmmiles
PR: wled/WLED#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:
wled00/const.h
🧬 Code graph analysis (13)
usermods/usermod_v2_skystrip/wind_view.h (1)
usermods/usermod_v2_skystrip/wind_view.cpp (9)
WindView(36-41)view(43-108)view(43-43)addToConfig(110-112)addToConfig(110-110)appendConfigData(114-119)appendConfigData(114-114)readFromConfig(121-127)readFromConfig(121-122)
usermods/usermod_v2_skystrip/util.cpp (1)
usermods/usermod_v2_skystrip/util.h (1)
c(122-129)
usermods/usermod_v2_skystrip/open_weather_map_source.h (2)
usermods/usermod_v2_skystrip/open_weather_map_source.cpp (15)
OpenWeatherMapSource(124-135)fetch(197-286)fetch(197-197)checkhistory(288-355)checkhistory(288-288)reload(357-365)reload(357-357)addToConfig(137-144)addToConfig(137-137)readFromConfig(146-187)readFromConfig(146-148)composeApiUrl(189-195)composeApiUrl(189-189)geocodeOWM(368-406)geocodeOWM(368-370)usermods/usermod_v2_skystrip/usermod_v2_skystrip.cpp (4)
addToConfig(128-148)addToConfig(128-128)readFromConfig(192-226)readFromConfig(192-192)
usermods/usermod_v2_skystrip/interfaces.h (7)
usermods/usermod_v2_skystrip/open_weather_map_source.cpp (6)
fetch(197-286)fetch(197-197)checkhistory(288-355)checkhistory(288-288)reload(357-365)reload(357-357)usermods/usermod_v2_skystrip/temperature_view.cpp (2)
view(65-176)view(65-66)usermods/usermod_v2_skystrip/temperature_view.h (1)
view(14-24)usermods/usermod_v2_skystrip/test_pattern_view.cpp (2)
view(91-131)view(91-92)usermods/usermod_v2_skystrip/test_pattern_view.h (1)
view(13-22)usermods/usermod_v2_skystrip/wind_view.cpp (2)
view(43-108)view(43-43)usermods/usermod_v2_skystrip/wind_view.h (1)
view(13-22)
usermods/usermod_v2_skystrip/test_pattern_view.h (2)
usermods/usermod_v2_skystrip/test_pattern_view.cpp (9)
TestPatternView(82-89)view(91-131)view(91-92)addToConfig(133-141)addToConfig(133-133)appendConfigData(143-148)appendConfigData(143-143)readFromConfig(150-179)readFromConfig(150-151)usermods/usermod_v2_skystrip/usermod_v2_skystrip.cpp (6)
addToConfig(128-148)addToConfig(128-128)appendConfigData(150-189)appendConfigData(150-150)readFromConfig(192-226)readFromConfig(192-192)
usermods/usermod_v2_skystrip/temperature_view.h (4)
usermods/usermod_v2_skystrip/temperature_view.cpp (9)
TemperatureView(58-63)view(65-176)view(65-66)addToConfig(178-180)addToConfig(178-178)appendConfigData(182-187)appendConfigData(182-182)readFromConfig(189-195)readFromConfig(189-190)usermods/usermod_v2_skystrip/test_pattern_view.cpp (8)
view(91-131)view(91-92)addToConfig(133-141)addToConfig(133-133)appendConfigData(143-148)appendConfigData(143-143)readFromConfig(150-179)readFromConfig(150-151)usermods/usermod_v2_skystrip/wind_view.cpp (8)
view(43-108)view(43-43)addToConfig(110-112)addToConfig(110-110)appendConfigData(114-119)appendConfigData(114-114)readFromConfig(121-127)readFromConfig(121-122)usermods/usermod_v2_skystrip/usermod_v2_skystrip.cpp (6)
addToConfig(128-148)addToConfig(128-128)appendConfigData(150-189)appendConfigData(150-150)readFromConfig(192-226)readFromConfig(192-192)
usermods/usermod_v2_skystrip/usermod_v2_skystrip.cpp (6)
usermods/usermod_v2_skystrip/usermod_v2_skystrip.h (2)
SkyStrip(17-51)setup(35-41)usermods/usermod_v2_skystrip/util.h (1)
time_now_utc(13-13)usermods/usermod_v2_skystrip/temperature_view.cpp (8)
view(65-176)view(65-66)addToConfig(178-180)addToConfig(178-178)appendConfigData(182-187)appendConfigData(182-182)readFromConfig(189-195)readFromConfig(189-190)usermods/usermod_v2_skystrip/test_pattern_view.cpp (8)
view(91-131)view(91-92)addToConfig(133-141)addToConfig(133-133)appendConfigData(143-148)appendConfigData(143-143)readFromConfig(150-179)readFromConfig(150-151)usermods/usermod_v2_skystrip/wind_view.cpp (8)
view(43-108)view(43-43)addToConfig(110-112)addToConfig(110-110)appendConfigData(114-119)appendConfigData(114-114)readFromConfig(121-127)readFromConfig(121-122)usermods/usermod_v2_skystrip/open_weather_map_source.cpp (4)
addToConfig(137-144)addToConfig(137-137)readFromConfig(146-187)readFromConfig(146-148)
usermods/usermod_v2_skystrip/test_pattern_view.cpp (5)
usermods/usermod_v2_skystrip/test_pattern_view.h (2)
TestPatternView(8-29)view(13-22)usermods/usermod_v2_skystrip/temperature_view.cpp (8)
view(65-176)view(65-66)addToConfig(178-180)addToConfig(178-178)appendConfigData(182-187)appendConfigData(182-182)readFromConfig(189-195)readFromConfig(189-190)usermods/usermod_v2_skystrip/util.cpp (2)
hsv2rgb(6-22)hsv2rgb(6-6)usermods/usermod_v2_skystrip/util.h (1)
blinkDebug(139-146)usermods/usermod_v2_skystrip/usermod_v2_skystrip.cpp (6)
addToConfig(128-148)addToConfig(128-128)appendConfigData(150-189)appendConfigData(150-150)readFromConfig(192-226)readFromConfig(192-192)
usermods/usermod_v2_skystrip/open_weather_map_source.cpp (1)
usermods/usermod_v2_skystrip/usermod_v2_skystrip.cpp (4)
addToConfig(128-148)addToConfig(128-128)readFromConfig(192-226)readFromConfig(192-192)
usermods/usermod_v2_skystrip/usermod_v2_skystrip.h (5)
usermods/usermod_v2_skystrip/usermod_v2_skystrip.cpp (19)
SkyStrip(32-41)setup(43-69)setup(43-43)loop(71-117)loop(71-71)handleOverlayDraw(119-125)handleOverlayDraw(119-119)addToConfig(128-148)addToConfig(128-128)appendConfigData(150-189)appendConfigData(150-150)readFromConfig(192-226)readFromConfig(192-192)showBooting(228-237)showBooting(228-228)doneBooting(239-244)doneBooting(239-239)reloadSources(246-252)reloadSources(246-246)usermods/usermod_v2_skystrip/open_weather_map_source.cpp (4)
addToConfig(137-144)addToConfig(137-137)readFromConfig(146-187)readFromConfig(146-148)usermods/usermod_v2_skystrip/temperature_view.cpp (6)
addToConfig(178-180)addToConfig(178-178)appendConfigData(182-187)appendConfigData(182-182)readFromConfig(189-195)readFromConfig(189-190)usermods/usermod_v2_skystrip/test_pattern_view.cpp (6)
addToConfig(133-141)addToConfig(133-133)appendConfigData(143-148)appendConfigData(143-143)readFromConfig(150-179)readFromConfig(150-151)usermods/usermod_v2_skystrip/wind_view.cpp (6)
addToConfig(110-112)addToConfig(110-110)appendConfigData(114-119)appendConfigData(114-114)readFromConfig(121-127)readFromConfig(121-122)
usermods/usermod_v2_skystrip/util.h (1)
usermods/usermod_v2_skystrip/util.cpp (2)
hsv2rgb(6-22)hsv2rgb(6-6)
usermods/usermod_v2_skystrip/wind_view.cpp (4)
usermods/usermod_v2_skystrip/util.h (5)
clamp01(34-36)estimateSpeedAt(83-86)estimateDirAt(87-90)estimateGustAt(91-94)blinkDebug(139-146)usermods/usermod_v2_skystrip/wind_view.h (2)
WindView(8-27)view(13-22)usermods/usermod_v2_skystrip/util.cpp (2)
hsv2rgb(6-22)hsv2rgb(6-6)usermods/usermod_v2_skystrip/usermod_v2_skystrip.cpp (6)
addToConfig(128-148)addToConfig(128-128)appendConfigData(150-189)appendConfigData(150-150)readFromConfig(192-226)readFromConfig(192-192)
usermods/usermod_v2_skystrip/temperature_view.cpp (3)
usermods/usermod_v2_skystrip/util.h (6)
clamp01(34-36)lerp(39-39)current_offset(16-22)estimateTempAt(75-78)estimateDewPtAt(79-82)blinkDebug(139-146)usermods/usermod_v2_skystrip/temperature_view.h (2)
TemperatureView(8-29)view(14-24)usermods/usermod_v2_skystrip/util.cpp (2)
hsv2rgb(6-22)hsv2rgb(6-6)
🪛 LanguageTool
usermods/usermod_v2_skystrip/readme.md
[grammar] ~3-~3: Ensure spelling is correct
Context: # SkyStrip This usermod displays the weather forecast on severa...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~3-~3: There might be a mistake here.
Context: ...forecast on several parallel LED strips. It currently includes Cloud, Wind, Tempe...
(QB_NEW_EN)
[grammar] ~13-~13: There might be a mistake here.
Context: ...ermap.org/api/one-call-3). The skystrip module makes one API call per hour plus ...
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ...our plus 24 calls when initially started up; it should remain comfortably under t...
(QB_NEW_EN)
[grammar] ~15-~15: There might be a mistake here.
Context: ...ly under the free-tier limit of 1000 per day. Enter the latitude and longitude f...
(QB_NEW_EN)
[grammar] ~18-~18: There might be a mistake here.
Context: ...tude for the desired forecast. There are several ways to do this: 1. Enter the la...
(QB_NEW_EN)
[grammar] ~21-~21: There might be a mistake here.
Context: ...LatitudeandLongitude` config fields. 2. Enter a combined lat/long string in the ...
(QB_NEW_EN)
usermods/usermod_v2_skystrip/FAQ.md
[grammar] ~4-~4: There might be a mistake here.
Context: ...e. Each view maps weather data onto hue, saturation, and value (brightness) along...
(QB_NEW_EN)
[grammar] ~11-~11: There might be a mistake here.
Context: ...le value scales with probability. In the absence of precipitation, hue differenti...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ... differentiates day from night: daylight clouds appear pale yellow, nighttime clo...
(QB_NEW_EN)
[grammar] ~13-~13: There might be a mistake here.
Context: ...llow, nighttime clouds desaturate toward white. For clouds, saturation is low and...
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ...on is low and value grows with coverage, keeping even thin clouds visible. Thus, ...
(QB_NEW_EN)
[grammar] ~15-~15: There might be a mistake here.
Context: ...le. Thus, a bright blue pixel highlights likely rain, whereas a soft yellow glow ...
(QB_NEW_EN)
[grammar] ~22-~22: There might be a mistake here.
Context: ...des for diagonal winds. Saturation rises with gustiness—calm breezes stay washed ...
(QB_NEW_EN)
[grammar] ~23-~23: There might be a mistake here.
Context: ...stay washed out while strong gusts drive the color toward full intensity. Value s...
(QB_NEW_EN)
[grammar] ~24-~24: There might be a mistake here.
Context: ...ensity. Value scales with wind strength, boosting brightness as the highest of su...
(QB_NEW_EN)
[grammar] ~25-~25: There might be a mistake here.
Context: ...s the highest of sustained speed or gust approaches 50 mph (or equivalent). For e...
(QB_NEW_EN)
[grammar] ~26-~26: There might be a mistake here.
Context: ...nt). For example, a saturated blue pixel indicates gusty north winds, while a dim...
(QB_NEW_EN)
[grammar] ~27-~27: There might be a mistake here.
Context: ...nds, while a dim pastel green suggests a gentle westerly breeze. The mapping bet...
(QB_NEW_EN)
[grammar] ~32-~32: There might be a mistake here.
Context: ...ed as: | Direction | Hue (°) | Color | |-----------|---------|--------| | N ...
(QB_NEW_EN)
[grammar] ~33-~33: There might be a mistake here.
Context: ...olor | |-----------|---------|--------| | N | 240 | Blue | | NE ...
(QB_NEW_EN)
[grammar] ~34-~34: There might be a mistake here.
Context: ...------| | N | 240 | Blue | | NE | 300 | Purple | | E ...
(QB_NEW_EN)
[grammar] ~35-~35: There might be a mistake here.
Context: ...lue | | NE | 300 | Purple | | E | 30 | Orange | | SE ...
(QB_NEW_EN)
[grammar] ~36-~36: There might be a mistake here.
Context: ...urple | | E | 30 | Orange | | SE | 45 | Gold | | S ...
(QB_NEW_EN)
[grammar] ~37-~37: There might be a mistake here.
Context: ...range | | SE | 45 | Gold | | S | 60 | Yellow | | SW ...
(QB_NEW_EN)
[grammar] ~38-~38: There might be a mistake here.
Context: ...old | | S | 60 | Yellow | | SW | 90 | Lime | | W ...
(QB_NEW_EN)
[grammar] ~39-~39: There might be a mistake here.
Context: ...ellow | | SW | 90 | Lime | | W | 120 | Green | | NW ...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ...ime | | W | 120 | Green | | NW | 180 | Cyan | | N ...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ...reen | | NW | 180 | Cyan | | N | 240 | Blue | ## Te...
(QB_NEW_EN)
[grammar] ~47-~47: There might be a mistake here.
Context: ...adient: deep blues near 14 °F transition through cyan and green to warm yellows a...
(QB_NEW_EN)
[grammar] ~48-~48: There might be a mistake here.
Context: ... to warm yellows at 77 °F and reds above 100 °F. Saturation reflects humidity via dew‑p...
(QB_NEW_EN)
[grammar] ~49-~49: There might be a mistake here.
Context: ...humidity via dew‑point spread; muggy air produces soft desaturated colors, wherea...
(QB_NEW_EN)
[grammar] ~50-~50: There might be a mistake here.
Context: ...ted colors, whereas dry air yields vivid tones. Value is fixed at mid‑brightness,...
(QB_NEW_EN)
[grammar] ~51-~51: There might be a mistake here.
Context: ...rightness, but local time markers (e.g., noon, midnight) temporarily darken pixel...
(QB_NEW_EN)
[grammar] ~58-~58: There might be a mistake here.
Context: ...g: | Temp (°F) | Hue (°) | Color | |-----------|---------|------------| | ≤...
(QB_NEW_EN)
[grammar] ~59-~59: There might be a mistake here.
Context: ... | |-----------|---------|------------| | ≤14 | 240 | Deep blue | | 3...
(QB_NEW_EN)
[grammar] ~60-~60: There might be a mistake here.
Context: ...--| | ≤14 | 240 | Deep blue | | 32 | 210 | Blue-cyan | | 5...
(QB_NEW_EN)
[grammar] ~61-~61: There might be a mistake here.
Context: ... | | 32 | 210 | Blue-cyan | | 50 | 180 | Cyan | | 6...
(QB_NEW_EN)
[grammar] ~62-~62: There might be a mistake here.
Context: ... | | 50 | 180 | Cyan | | 68 | 150 | Green-cyan | | 7...
(QB_NEW_EN)
[grammar] ~63-~63: There might be a mistake here.
Context: ... | | 68 | 150 | Green-cyan | | 77 | 60 | Yellow | | 9...
(QB_NEW_EN)
[grammar] ~64-~64: There might be a mistake here.
Context: ...n | | 77 | 60 | Yellow | | 95 | 30 | Orange | | ≥...
(QB_NEW_EN)
[grammar] ~65-~65: There might be a mistake here.
Context: ... | | 95 | 30 | Orange | | ≥100 | 0 | Red | #...
(QB_NEW_EN)
[grammar] ~77-~77: There might be a mistake here.
Context: ...dim blue pixel therefore means a slight cool‑down with more moisture, while a bright satur...
(QB_NEW_EN)
[grammar] ~78-~78: There might be a mistake here.
Context: ...e a bright saturated red indicates rapid warming coupled with drying. Approximat...
(QB_NEW_EN)
[grammar] ~83-~83: There might be a mistake here.
Context: ...(Color) | | Humidity | Saturation | |-------------|-------------| |------...
(QB_NEW_EN)
[grammar] ~84-~84: There might be a mistake here.
Context: ...--------| |------------|------------| | Cooling | Blue tones | | More ...
(QB_NEW_EN)
[grammar] ~85-~85: There might be a mistake here.
Context: ... tones | | More humid | Low/Pastel | | Steady | Green | | Stabl...
(QB_NEW_EN)
[grammar] ~86-~86: There might be a mistake here.
Context: ...n | | Stable | Medium | | Warming | Yellow→Red | | Drier...
(QB_NEW_EN)
🔇 Additional comments (12)
wled00/const.h (1)
202-202: Confirm need for a new core usermod ID; avoid churn if not required.If SkyStrip is intended as an in-repo/official usermod, 59 here is fine. If not, consider using a self-defined ID inside the usermod (e.g., ≥500) to avoid touching core, per past practice and planned deprecation of numeric IDs. Please confirm intended status.
usermods/usermod_v2_skystrip/library.json (1)
1-4: LGTM: minimal manifest with libArchive disabled.Matches how other WLED usermods avoid link-time archiving.
usermods/usermod_v2_skystrip/readme.md (1)
18-28: Polish list formatting and examples for consistency.Improve readability of the config list and punctuation.
[ suggest_nitpick ]
Apply:-Enter the latitude and longitude for the desired forecast. There are -several ways to do this: -1. Enter the latitude and longitude as signed floating point numbers - in the `Latitude` and `Longitude` config fields. -2. Enter a combined lat/long string in the `Location` field, examples: -- `54.9352° S, 67.6059° W` -- `-54.9352, -67.6059` -- `-54.9352 -67.6059` -- `S54°42'7", W67°40'33"` -3. Enter a geo location string like `oakland,ca,us` in the `Location` field. +Enter the latitude and longitude for the desired forecast. You can: +1. Enter signed floating-point values in the `Latitude` and `Longitude` fields. +2. Enter a combined lat/long string in the `Location` field, for example: + - `54.9352° S, 67.6059° W` + - `-54.9352, -67.6059` + - `-54.9352 -67.6059` + - `S54°42'7", W67°40'33"` +3. Enter a geo-location string (e.g., `oakland,ca,us`) in the `Location` field.usermods/usermod_v2_skystrip/rest_json_client.h (1)
31-34: Ensure doc_ is constructed with capacity; otherwise deserialization will fail.Verify constructor uses a member-initializer:
doc_(MAX_JSON_SIZE). If not, add it.Add (in RestJsonClient.cpp):
RestJsonClient::RestJsonClient() : lastFetchMs_(0) , doc_(MAX_JSON_SIZE) {}usermods/usermod_v2_skystrip/FAQ.md (1)
20-28: Verify doc ↔ code mapping for wind hueConfirm that hueFromDir() (code) matches these documented direction→hue values (e.g., N=240°, E≈30°, S≈60°, W≈120°). If not, update either the function or this section to avoid user confusion.
usermods/usermod_v2_skystrip/open_weather_map_source.cpp (2)
31-47: urlEncode: keep '+' encoding consistent with space handlingOWM accepts %20; existing code is fine. If you later allow '+', ensure both encode/decode paths agree. No change required now.
197-206: Throttle logic and URL composition look correctFetch interval gating and timemachine URL formation are correct for OWM 3.0 endpoints.
Also applies to: 288-301
usermods/usermod_v2_skystrip/wind_view.cpp (2)
36-41: Good constructor and debug buffer init.
79-85: Brightness floor applies even for gust-only nonzero; verify this is desired.
valusesmax(spd, gst)with a floor kMinV>0, so any nonzero gust produces visible output even if sustained wind is zero. Confirm design; otherwise gate onspdor blend.usermods/usermod_v2_skystrip/temperature_view.cpp (1)
28-56: Nice, readable temp→hue ramp.Stop table and lerp are clear and will be easy to tune.
usermods/usermod_v2_skystrip/util.h (2)
110-135: applySaturation preserves luminance correctly; LGTM.
139-146: Blink debug usesmillis()/1000square wave; good for visibility.No changes needed.
- Avoid leaving segments permanently frozen; restore previous freeze state - Fix negative modulo in local-time marker logic - Handle invalid segments safely - Clarify duplicate "N" table row - Fix country normalizeLocation bug - Don't include the API key in logging - Tighten wording; avoid hardcoding OWM plan limits - Fix rate-limit reset logic - Harden rate-limit check and keep informative logging - Add network timeout to avoid long blocking on bad links - Check non-2xx status codes before parsing - Right-size JSON buffer per target - Fix potential buffer overrun in emitSeriesMDHM - Use float instead of double for DataPoint.value - Restore seg.freeze after rendering (and avoid freezing on zero-length) - use PROGMEM to reduce RAM use - Sanitize HSV inputs - Clamp HSV values in conversion to RGB - Clarify SAFETY_DELAY_MS type - Clamp and wrap HSV inputs to prevent overflow/underflow artifacts - Normalize wind direction to [0,360) before mapping to hue - fixed compiler warning about shadowed variables - delay initial checkhistory after fetch
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (3)
usermods/usermod_v2_skystrip/FAQ.md (1)
32-43: Remove duplicate “N” row and fix table column countThe wind-direction table lists “N” twice, and the trailing “(wraps around)” creates a 4th column that breaks markdownlint. Drop the final “N” row and add a separate note below the table.
| NW | 180 | Cyan | -| N | 240 | Blue | (wraps around) + +Note: Hues wrap at 360°, so “N” repeats at the boundary.usermods/usermod_v2_skystrip/rest_json_client.h (1)
6-9: Include ArduinoJson explicitly (don’t rely on transitive include)DynamicJsonDocument is used in the public API; add the header to avoid brittle build dependencies.
#include <WiFiClient.h> #include <WiFiClientSecure.h> #include "wled.h" +#include <ArduinoJson.h>usermods/usermod_v2_skystrip/open_weather_map_source.cpp (1)
70-85: LGTM: “,USA” → “,US” normalization fixedThe off-by-one/terminator bug is resolved; this now truncates correctly.
🧹 Nitpick comments (9)
usermods/usermod_v2_skystrip/util.h (1)
57-60: Unused parameter “step” in estimateAt“step” is unused and may trigger warnings. Either remove it or mark as intentionally unused.
-bool estimateAt(const Series &v, time_t t, double step, double &out) { +bool estimateAt(const Series &v, time_t t, double /*step*/, double &out) {usermods/usermod_v2_skystrip/rest_json_client.h (2)
16-23: Document lifetime and non-reentrancy of returned JSON pointerClarify that the returned DynamicJsonDocument* is owned by the client and is invalidated on the next getJson() call.
26-33: Expose/read a socket timeout; ensure it’s applied on WiFiClient before begin()Prevent hangs on bad networks. Per team learning, set timeouts on WiFiClient, not via HTTPClient::setTimeout(). Consider a setter or ctor arg to control it.
usermods/usermod_v2_skystrip/temperature_view.cpp (1)
32-42: Align temperature→hue stops with docs or update docsThe stops (e.g., 68°F → 138.8°, 95°F → 18.8°, 104°F → 0°) differ from FAQ’s example mapping (68→150, 95→30, ≥100→0). Either adjust the constants or note the intentional calibration drift.
usermods/usermod_v2_skystrip/rest_json_client.cpp (1)
56-66: Guard against oversized payloads vs. JSON doc capacityIf Content-Length is known and exceeds doc_ capacity, abort early to avoid partial/failed parses and wasted work.
int len = http_.getSize(); DEBUG_PRINTF("SkyStrip: RestJsonClient::getJson: expecting up to %d bytes, free heap before deserialization: %u\n", len, ESP.getFreeHeap()); + if (len > 0) { + const size_t cap = doc_.capacity(); + if ((size_t)len > cap) { + http_.end(); + DEBUG_PRINTF("SkyStrip: RestJsonClient::getJson: response too large (%d > %u)\n", len, (unsigned)cap); + return nullptr; + } + } doc_.clear();usermods/usermod_v2_skystrip/cloud_view.cpp (1)
98-106: Fix comment/value mismatch for night brightnessComment says “slightly higher night maximum” but both day and night VMax are 0.40. Either adjust the value or the comment.
- // slightly higher night maximum so low clouds are more visible - constexpr float kNightVMax= 0.40f; + // slightly higher night maximum so low clouds are more visible + constexpr float kNightVMax= 0.45f; // or update comment if 0.40 is intendedusermods/usermod_v2_skystrip/delta_view.cpp (1)
88-96: Use signed loop index to match len typelen is int; using uint16_t for i can underflow/overflow comparisons on large segments. Use int for consistency.
- for (uint16_t i = 0; i < len; ++i) { + for (int i = 0; i < len; ++i) {usermods/usermod_v2_skystrip/open_weather_map_source.cpp (2)
217-224: Consider moving lastFetch_ update after a successful GETUpdating lastFetch_ before network I/O rate-limits retries on transient failures. If you want faster recovery on failures, update lastFetch_ only on success (keep lastHistFetch_ as-is). If avoiding API thrash is more important, keep current behavior.
- lastFetch_ = now; - lastHistFetch_ = now; // history fetches should wait + // Defer lastFetch_ until we confirm success (optional strategy) + // lastHistFetch_ = now; // keep history delay if desired
309-312: Minor: redundant lastHistFetch_ assignmentlastHistFetch_ is set both before and after fetch; keep one (the stagger-after line here is enough).
- // Stagger history fetch to avoid back-to-back GETs in same loop iteration - // and reduce risk of watchdog resets. Enforce at least 15s before history. - lastHistFetch_ = skystrip::util::time_now_utc(); + // Stagger history fetch; enforce min delay in checkhistory() + lastHistFetch_ = skystrip::util::time_now_utc();(If you keep this, remove the earlier assignment at Line 223.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
usermods/usermod_v2_skystrip/FAQ.md(1 hunks)usermods/usermod_v2_skystrip/cloud_view.cpp(1 hunks)usermods/usermod_v2_skystrip/delta_view.cpp(1 hunks)usermods/usermod_v2_skystrip/open_weather_map_source.cpp(1 hunks)usermods/usermod_v2_skystrip/readme.md(1 hunks)usermods/usermod_v2_skystrip/rest_json_client.cpp(1 hunks)usermods/usermod_v2_skystrip/rest_json_client.h(1 hunks)usermods/usermod_v2_skystrip/skymodel.cpp(1 hunks)usermods/usermod_v2_skystrip/skymodel.h(1 hunks)usermods/usermod_v2_skystrip/temperature_view.cpp(1 hunks)usermods/usermod_v2_skystrip/test_pattern_view.cpp(1 hunks)usermods/usermod_v2_skystrip/usermod_v2_skystrip.cpp(1 hunks)usermods/usermod_v2_skystrip/util.cpp(1 hunks)usermods/usermod_v2_skystrip/util.h(1 hunks)usermods/usermod_v2_skystrip/wind_view.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- usermods/usermod_v2_skystrip/wind_view.cpp
- usermods/usermod_v2_skystrip/test_pattern_view.cpp
- usermods/usermod_v2_skystrip/util.cpp
- usermods/usermod_v2_skystrip/skymodel.h
- usermods/usermod_v2_skystrip/usermod_v2_skystrip.cpp
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-29T00:22:33.137Z
Learnt from: ksedgwic
PR: wled/WLED#4883
File: usermods/usermod_v2_skystrip/rest_json_client.cpp:40-46
Timestamp: 2025-08-29T00:22:33.137Z
Learning: For ESP32/ESP8266 HTTP requests in WLED, set timeouts on the underlying WiFiClient (using client.setTimeout()) before calling http_.begin(), rather than using HTTPClient::setTimeout(). This pattern is used successfully in the Klipper usermod to prevent connection hangs.
Applied to files:
usermods/usermod_v2_skystrip/rest_json_client.cppusermods/usermod_v2_skystrip/rest_json_client.h
📚 Learning: 2025-06-07T15:58:42.579Z
Learnt from: willmmiles
PR: wled/WLED#4712
File: wled00/FX_fcn.cpp:992-993
Timestamp: 2025-06-07T15:58:42.579Z
Learning: In WLED segments, the backing pixel buffer is always allocated to the full physical segment size, but effect rendering only uses a subset when mirror or grouping are enabled. For 2D segments, this subset is vWidth() * vHeight(), and for 1D segments it's vLength(). Color processing operations like fade_out should iterate over this rendering subset, not the full allocated buffer.
Applied to files:
usermods/usermod_v2_skystrip/temperature_view.cpp
📚 Learning: 2025-04-30T05:41:03.633Z
Learnt from: blazoncek
PR: wled/WLED#4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like `if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))` is unnecessary and could be problematic, as it bypasses proper size verification.
Applied to files:
usermods/usermod_v2_skystrip/temperature_view.cpp
🧬 Code graph analysis (7)
usermods/usermod_v2_skystrip/rest_json_client.cpp (1)
usermods/usermod_v2_skystrip/rest_json_client.h (1)
RestJsonClient(16-38)
usermods/usermod_v2_skystrip/open_weather_map_source.cpp (4)
usermods/usermod_v2_skystrip/util.h (3)
c(135-142)s(14-20)time_now_utc(26-26)usermods/usermod_v2_skystrip/usermod_v2_skystrip.cpp (2)
readFromConfig(192-226)readFromConfig(192-192)usermods/usermod_v2_skystrip/skymodel.cpp (2)
invalidate_history(76-87)invalidate_history(76-76)usermods/usermod_v2_skystrip/rest_json_client.cpp (4)
getJson(15-66)getJson(15-15)resetRateLimit(10-13)resetRateLimit(10-10)
usermods/usermod_v2_skystrip/temperature_view.cpp (4)
usermods/usermod_v2_skystrip/util.h (6)
lerp(52-52)current_offset(29-35)s(14-20)estimateTempAt(88-91)estimateDewPtAt(92-95)blinkDebug(152-159)usermods/usermod_v2_skystrip/temperature_view.h (2)
TemperatureView(8-29)view(14-24)usermods/usermod_v2_skystrip/util.cpp (2)
hsv2rgb(6-32)hsv2rgb(6-6)usermods/usermod_v2_skystrip/usermod_v2_skystrip.cpp (6)
addToConfig(128-148)addToConfig(128-128)appendConfigData(150-189)appendConfigData(150-150)readFromConfig(192-226)readFromConfig(192-192)
usermods/usermod_v2_skystrip/cloud_view.cpp (5)
usermods/usermod_v2_skystrip/cloud_view.h (2)
CloudView(8-27)view(13-22)usermods/usermod_v2_skystrip/util.h (6)
current_offset(29-35)estimateCloudAt(108-111)estimatePrecipTypeAt(112-115)estimatePrecipProbAt(116-119)blinkDebug(152-159)s(14-20)usermods/usermod_v2_skystrip/util.cpp (2)
hsv2rgb(6-32)hsv2rgb(6-6)usermods/usermod_v2_skystrip/usermod_v2_skystrip.cpp (6)
addToConfig(128-148)addToConfig(128-128)appendConfigData(150-189)appendConfigData(150-150)readFromConfig(192-226)readFromConfig(192-192)usermods/usermod_v2_skystrip/skymodel.cpp (2)
invalidate_history(76-87)invalidate_history(76-76)
usermods/usermod_v2_skystrip/delta_view.cpp (5)
usermods/usermod_v2_skystrip/util.h (3)
lerp(52-52)blinkDebug(152-159)s(14-20)usermods/usermod_v2_skystrip/delta_view.h (2)
DeltaView(8-27)view(13-22)usermods/usermod_v2_skystrip/cloud_view.cpp (8)
view(39-199)view(39-39)addToConfig(201-203)addToConfig(201-201)appendConfigData(205-210)appendConfigData(205-205)readFromConfig(212-218)readFromConfig(212-213)usermods/usermod_v2_skystrip/util.cpp (2)
hsv2rgb(6-32)hsv2rgb(6-6)usermods/usermod_v2_skystrip/usermod_v2_skystrip.cpp (6)
addToConfig(128-148)addToConfig(128-128)appendConfigData(150-189)appendConfigData(150-150)readFromConfig(192-226)readFromConfig(192-192)
usermods/usermod_v2_skystrip/skymodel.cpp (1)
usermods/usermod_v2_skystrip/skymodel.h (1)
SkyModel(16-47)
usermods/usermod_v2_skystrip/rest_json_client.h (1)
usermods/usermod_v2_skystrip/rest_json_client.cpp (5)
RestJsonClient(5-8)getJson(15-66)getJson(15-15)resetRateLimit(10-13)resetRateLimit(10-10)
🪛 LanguageTool
usermods/usermod_v2_skystrip/FAQ.md
[grammar] ~4-~4: There might be a mistake here.
Context: ...e. Each view maps weather data onto hue, saturation, and value (brightness) along...
(QB_NEW_EN)
[grammar] ~11-~11: There might be a mistake here.
Context: ...le value scales with probability. In the absence of precipitation, hue differenti...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ... differentiates day from night: daylight clouds appear pale yellow, nighttime clo...
(QB_NEW_EN)
[grammar] ~13-~13: There might be a mistake here.
Context: ...llow, nighttime clouds desaturate toward white. For clouds, saturation is low and...
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ...on is low and value grows with coverage, keeping even thin clouds visible. Thus, ...
(QB_NEW_EN)
[grammar] ~15-~15: There might be a mistake here.
Context: ...le. Thus, a bright blue pixel highlights likely rain, whereas a soft yellow glow ...
(QB_NEW_EN)
[grammar] ~22-~22: There might be a mistake here.
Context: ...des for diagonal winds. Saturation rises with gustiness—calm breezes stay washed ...
(QB_NEW_EN)
[grammar] ~23-~23: There might be a mistake here.
Context: ...stay washed out while strong gusts drive the color toward full intensity. Value s...
(QB_NEW_EN)
[grammar] ~24-~24: There might be a mistake here.
Context: ...ensity. Value scales with wind strength, boosting brightness as the highest of su...
(QB_NEW_EN)
[grammar] ~25-~25: There might be a mistake here.
Context: ...s the highest of sustained speed or gust approaches 50 mph (or equivalent). For e...
(QB_NEW_EN)
[grammar] ~26-~26: There might be a mistake here.
Context: ...nt). For example, a saturated blue pixel indicates gusty north winds, while a dim...
(QB_NEW_EN)
[grammar] ~27-~27: There might be a mistake here.
Context: ...nds, while a dim pastel green suggests a gentle westerly breeze. The mapping bet...
(QB_NEW_EN)
[grammar] ~32-~32: There might be a mistake here.
Context: ...ed as: | Direction | Hue (°) | Color | |-----------|---------|--------| | N ...
(QB_NEW_EN)
[grammar] ~33-~33: There might be a mistake here.
Context: ...olor | |-----------|---------|--------| | N | 240 | Blue | | NE ...
(QB_NEW_EN)
[grammar] ~34-~34: There might be a mistake here.
Context: ...------| | N | 240 | Blue | | NE | 300 | Purple | | E ...
(QB_NEW_EN)
[grammar] ~35-~35: There might be a mistake here.
Context: ...lue | | NE | 300 | Purple | | E | 30 | Orange | | SE ...
(QB_NEW_EN)
[grammar] ~36-~36: There might be a mistake here.
Context: ...urple | | E | 30 | Orange | | SE | 45 | Gold | | S ...
(QB_NEW_EN)
[grammar] ~37-~37: There might be a mistake here.
Context: ...range | | SE | 45 | Gold | | S | 60 | Yellow | | SW ...
(QB_NEW_EN)
[grammar] ~38-~38: There might be a mistake here.
Context: ...old | | S | 60 | Yellow | | SW | 90 | Lime | | W ...
(QB_NEW_EN)
[grammar] ~39-~39: There might be a mistake here.
Context: ...ellow | | SW | 90 | Lime | | W | 120 | Green | | NW ...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ...ime | | W | 120 | Green | | NW | 180 | Cyan | | N ...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ...reen | | NW | 180 | Cyan | | N | 240 | Blue | (wraps ...
(QB_NEW_EN)
[grammar] ~47-~47: There might be a mistake here.
Context: ...adient: deep blues near 14 °F transition through cyan and green to warm yellows a...
(QB_NEW_EN)
[grammar] ~48-~48: There might be a mistake here.
Context: ... to warm yellows at 77 °F and reds above 100 °F. Saturation reflects humidity via dew‑p...
(QB_NEW_EN)
[grammar] ~49-~49: There might be a mistake here.
Context: ...humidity via dew‑point spread; muggy air produces soft desaturated colors, wherea...
(QB_NEW_EN)
[grammar] ~50-~50: There might be a mistake here.
Context: ...ted colors, whereas dry air yields vivid tones. Value is fixed at mid‑brightness,...
(QB_NEW_EN)
[grammar] ~51-~51: There might be a mistake here.
Context: ...rightness, but local time markers (e.g., noon, midnight) temporarily darken pixel...
(QB_NEW_EN)
[grammar] ~58-~58: There might be a mistake here.
Context: ...g: | Temp (°F) | Hue (°) | Color | |-----------|---------|------------| | ≤...
(QB_NEW_EN)
[grammar] ~59-~59: There might be a mistake here.
Context: ... | |-----------|---------|------------| | ≤14 | 240 | Deep blue | | 3...
(QB_NEW_EN)
[grammar] ~60-~60: There might be a mistake here.
Context: ...--| | ≤14 | 240 | Deep blue | | 32 | 210 | Blue-cyan | | 5...
(QB_NEW_EN)
[grammar] ~61-~61: There might be a mistake here.
Context: ... | | 32 | 210 | Blue-cyan | | 50 | 180 | Cyan | | 6...
(QB_NEW_EN)
[grammar] ~62-~62: There might be a mistake here.
Context: ... | | 50 | 180 | Cyan | | 68 | 150 | Green-cyan | | 7...
(QB_NEW_EN)
[grammar] ~63-~63: There might be a mistake here.
Context: ... | | 68 | 150 | Green-cyan | | 77 | 60 | Yellow | | 9...
(QB_NEW_EN)
[grammar] ~64-~64: There might be a mistake here.
Context: ...n | | 77 | 60 | Yellow | | 95 | 30 | Orange | | ≥...
(QB_NEW_EN)
[grammar] ~65-~65: There might be a mistake here.
Context: ... | | 95 | 30 | Orange | | ≥100 | 0 | Red | #...
(QB_NEW_EN)
[grammar] ~77-~77: There might be a mistake here.
Context: ...dim blue pixel therefore means a slight cool‑down with more moisture, while a bright satur...
(QB_NEW_EN)
[grammar] ~78-~78: There might be a mistake here.
Context: ...e a bright saturated red indicates rapid warming coupled with drying. Approximat...
(QB_NEW_EN)
[grammar] ~83-~83: There might be a mistake here.
Context: ...(Color) | | Humidity | Saturation | |-------------|-------------| |------...
(QB_NEW_EN)
[grammar] ~84-~84: There might be a mistake here.
Context: ...--------| |------------|------------| | Cooling | Blue tones | | More ...
(QB_NEW_EN)
[grammar] ~85-~85: There might be a mistake here.
Context: ... tones | | More humid | Low/Pastel | | Steady | Green | | Stabl...
(QB_NEW_EN)
[grammar] ~86-~86: There might be a mistake here.
Context: ...n | | Stable | Medium | | Warming | Yellow→Red | | Drier...
(QB_NEW_EN)
usermods/usermod_v2_skystrip/readme.md
[grammar] ~3-~3: Ensure spelling is correct
Context: # SkyStrip This usermod displays the weather forecast on severa...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~3-~3: There might be a mistake here.
Context: ...forecast on several parallel LED strips. It currently includes Cloud, Wind, Tempe...
(QB_NEW_EN)
[grammar] ~17-~17: There might be a mistake here.
Context: ...tude for the desired forecast. There are several ways to do this: 1. Enter the la...
(QB_NEW_EN)
[grammar] ~20-~20: There might be a mistake here.
Context: ...LatitudeandLongitude` config fields. 2. Enter a combined lat/long string in the ...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
usermods/usermod_v2_skystrip/FAQ.md
42-42: Table pipe style
Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
(MD055, table-pipe-style)
42-42: Table column count
Expected: 3; Actual: 4; Too many cells, extra data will be missing
(MD056, table-column-count)
🔇 Additional comments (7)
usermods/usermod_v2_skystrip/util.h (1)
38-44: Confirm local-time formatting behavior across DST changesfmt_local applies the current offset to any utc_ts, which can be off for timestamps spanning a DST boundary. If acceptable, add a brief comment; otherwise, consider deriving offset for the specific utc_ts.
usermods/usermod_v2_skystrip/readme.md (1)
12-16: LGTM: OWM plan wording is future-proofedCall pattern described without hardcoded quotas; casing consistent. No further changes needed.
usermods/usermod_v2_skystrip/temperature_view.cpp (2)
85-86: Nice: RAII freeze guard prevents sticky freeze on early returnsThe FreezeGuard resolves the previous freeze-leak issue.
90-129: DST edge case: markerWeight uses a single offset for a 48h horizonIf a DST change occurs within the horizon, marker placement will be off by 1h around the transition. Acceptable if out-of-scope; otherwise compute offset for t.
usermods/usermod_v2_skystrip/skymodel.cpp (1)
106-149: LGTM: robust streamed debug emissionSafe snprintf usage, capacity checks, and chunked writes look solid.
usermods/usermod_v2_skystrip/cloud_view.cpp (1)
69-96: LGTM: normalized TOD math and marker detectionNormalization to [0,DAY) and wrap-aware nearTOD logic look correct.
usermods/usermod_v2_skystrip/open_weather_map_source.cpp (1)
389-393: LGTM: reload() correctly forces immediate eligibilityAdjusting lastFetch_ by interval enables immediate refresh.
- Polish README list formatting and examples for consistency - Improve FAQ - Mark variable as unused - Improve comments to clarify semantics - Improve temperature mapping documentation in the README
- Don't define USERMOD_ID_SKYSTRIP globally - Use signed loop index to match len type - Expose/read a socket timeout; ensure it’s applied on WiFiClient before begin() - Guard against oversized payloads vs. JSON doc capacity - Avoid wraparound trick in RestJsonClient ctor; initialize via reset for clarity - Make RestJsonClient non-copyable/non-movable - Preserve tail data in SkyModel mergeSeries - Fix wind-direction interpolation (circular wrap-around bug)
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
usermods/usermod_v2_skystrip/util.h (1)
138-145: Do not interpolate categorical precipitation “type”; use nearest sample.Linear interpolation yields invalid in‑between types.
Apply within this file:
-inline bool estimatePrecipTypeAt(const SkyModel &m, time_t t, double step, - double &out) { - return estimateAt(m.precip_type_forecast, t, step, out); -} +inline bool estimatePrecipTypeAt(const SkyModel &m, time_t t, double /*step*/, + double &out) { + // Nearest-neighbor over time (categorical) + const auto &v = m.precip_type_forecast; + if (v.empty()) return false; + if (t < v.front().tstamp - GRACE_SEC) return false; + if (t > v.back().tstamp + GRACE_SEC) return false; + if (t <= v.front().tstamp) { out = v.front().value; return true; } + if (t >= v.back().tstamp) { out = v.back().value; return true; } + for (size_t i = 1; i < v.size(); ++i) { + if (t <= v[i].tstamp) { + const auto &a = v[i-1], &b = v[i]; + const time_t mid = a.tstamp + (b.tstamp - a.tstamp)/2; + out = (t < mid) ? a.value : b.value; + return true; + } + } + return false; +}If you prefer a reusable helper, I can factor this into estimateAtNearest() near estimateAt().
🧹 Nitpick comments (10)
usermods/usermod_v2_skystrip/readme.md (1)
4-4: Normalize view names for consistency (“24‑Hour Delta”, “Test Pattern”).Use consistent hyphenation/casing across README and FAQ.
Apply:
-It currently includes Cloud, Wind, Temperature, 24 Hour Delta, and TestPattern views. +It currently includes Cloud, Wind, Temperature, 24‑Hour Delta, and Test Pattern views.usermods/usermod_v2_skystrip/util.h (1)
38-44: Ensure fmt_local always NUL-terminates on failure.Guard zero-length and strftime() returning 0.
Apply:
inline void fmt_local(char *out, size_t n, time_t utc_ts, const char *fmt = "%m-%d %H:%M") { - const time_t local_sec = utc_ts + current_offset(); + if (n == 0) return; + out[0] = '\0'; + const time_t local_sec = utc_ts + current_offset(); struct tm tmLocal; gmtime_r(&local_sec, &tmLocal); // local_sec is already local seconds - strftime(out, n, fmt, &tmLocal); + if (strftime(out, n, fmt, &tmLocal) == 0) out[n-1] = '\0'; }usermods/usermod_v2_skystrip/rest_json_client.cpp (2)
64-66: Remove redundant 204 check; already covered by non‑2xx guard.Keeps condition minimal and accurate.
Apply:
- if (code <= 0 || code < 200 || code >= 300 || code == 204) { + if (code <= 0 || code < 200 || code >= 300) {
50-56: Also set connect timeout on ESP32 to avoid long TCP handshakes.HTTPClient on ESP32 supports setConnectTimeout(); no-op on ESP8266.
Apply:
// Begin request - if (client) { + if (client) { // Apply socket (Stream) timeout before using HTTPClient. client->setTimeout(socketTimeoutMs_); } +#if defined(ARDUINO_ARCH_ESP32) + http_.setConnectTimeout((int)socketTimeoutMs_); +#endif if (!http_.begin(*client, url)) {usermods/usermod_v2_skystrip/open_weather_map_source.cpp (6)
49-68: Redactor: optionally handle mixed-case “appid”If upstream ever changes param case, this misses redaction. Consider case-insensitive search when available.
Minimal tweak on POSIX targets:
- const char* p = strstr(in, "appid="); + const char* p = strstr(in, "appid="); + #if defined(__GLIBC__) || defined(__APPLE__) || defined(__unix__) + if (!p) p = strcasestr(in, "appid="); + #endifIf portability is a concern, keep as-is.
70-84: normalizeLocation fix looks good; update the commentYou now only trim spaces (commas are preserved). Update the comment for accuracy.
Apply:
- // trim spaces and commas + // trim spaces (preserve commas)
166-207: Config UX: avoid one-shot geocode failure “sticky” stateIf geocoding fails due to transient network issues, location_ remains unchanged and will not be retried until user edits it. Consider retrying after backoff when running, or clearing lastLocation_ on failure.
I can propose a small backoff/jitter state if desired.
209-215: Make units configurable instead of hard-coded imperialHard-coding imperial will be wrong for most users outside the US and can conflict with WLED’s global unit setting.
Apply minimal change now and add a helper outside this hunk:
- (void)snprintf(buf, len, - "%s/data/3.0/onecall?exclude=minutely,daily,alerts&units=imperial&lat=%.6f&lon=%.6f&appid=%s", - apiBase_.c_str(), latitude_, longitude_, apiKey_.c_str()); + (void)snprintf(buf, len, + "%s/data/3.0/onecall?exclude=minutely,daily,alerts&units=%s&lat=%.6f&lon=%.6f&appid=%s", + apiBase_.c_str(), unitsStr(), latitude_, longitude_, apiKey_.c_str());Add this helper in the class (or file-static) and a config key if you prefer:
// outside selected lines const char* OpenWeatherMapSource::unitsStr() const { // TODO: read from config or global WLED setting return "imperial"; }
316-324: History window boundary conditionThe check skips fetching when oldestTstamp == now - 24h. If you want to include exactly the 24h lookback, use strict inequality.
Apply:
- if (oldestTstamp <= now - HISTORY_SEC) return nullptr; + if (oldestTstamp < now - HISTORY_SEC) return nullptr;
397-438: Geocode behavior and rate-limit resets
- Good: normalize, encode, limit=5, accept exactly one match.
- Consider: if multiple matches, optionally pick best by country code when the query ends with “,US”.
I can add a minimal disambiguation: prefer matches where “country == 'US'” when query ends with “,US”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
usermods/usermod_v2_skystrip/FAQ.md(1 hunks)usermods/usermod_v2_skystrip/cloud_view.cpp(1 hunks)usermods/usermod_v2_skystrip/delta_view.cpp(1 hunks)usermods/usermod_v2_skystrip/open_weather_map_source.cpp(1 hunks)usermods/usermod_v2_skystrip/readme.md(1 hunks)usermods/usermod_v2_skystrip/rest_json_client.cpp(1 hunks)usermods/usermod_v2_skystrip/rest_json_client.h(1 hunks)usermods/usermod_v2_skystrip/skymodel.cpp(1 hunks)usermods/usermod_v2_skystrip/temperature_view.cpp(1 hunks)usermods/usermod_v2_skystrip/test_pattern_view.cpp(1 hunks)usermods/usermod_v2_skystrip/usermod_v2_skystrip.h(1 hunks)usermods/usermod_v2_skystrip/util.h(1 hunks)usermods/usermod_v2_skystrip/wind_view.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- usermods/usermod_v2_skystrip/wind_view.cpp
- usermods/usermod_v2_skystrip/usermod_v2_skystrip.h
- usermods/usermod_v2_skystrip/delta_view.cpp
- usermods/usermod_v2_skystrip/skymodel.cpp
- usermods/usermod_v2_skystrip/temperature_view.cpp
- usermods/usermod_v2_skystrip/test_pattern_view.cpp
- usermods/usermod_v2_skystrip/cloud_view.cpp
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-29T00:22:33.137Z
Learnt from: ksedgwic
PR: wled/WLED#4883
File: usermods/usermod_v2_skystrip/rest_json_client.cpp:40-46
Timestamp: 2025-08-29T00:22:33.137Z
Learning: For ESP32/ESP8266 HTTP requests in WLED, set timeouts on the underlying WiFiClient (using client.setTimeout()) before calling http_.begin(), rather than using HTTPClient::setTimeout(). This pattern is used successfully in the Klipper usermod to prevent connection hangs.
Applied to files:
usermods/usermod_v2_skystrip/rest_json_client.cppusermods/usermod_v2_skystrip/rest_json_client.h
📚 Learning: 2025-08-29T00:22:33.137Z
Learnt from: ksedgwic
PR: wled/WLED#4883
File: usermods/usermod_v2_skystrip/rest_json_client.cpp:40-46
Timestamp: 2025-08-29T00:22:33.137Z
Learning: The HTTPClient::setTimeout() method in ESP32/ESP8266 may not prevent hanging in all scenarios, particularly during connection establishment or DNS resolution phases.
Applied to files:
usermods/usermod_v2_skystrip/rest_json_client.cpp
📚 Learning: 2025-08-29T00:26:15.770Z
Learnt from: ksedgwic
PR: wled/WLED#4883
File: usermods/usermod_v2_skystrip/rest_json_client.h:6-14
Timestamp: 2025-08-29T00:26:15.770Z
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/usermod_v2_skystrip/rest_json_client.cppusermods/usermod_v2_skystrip/rest_json_client.h
📚 Learning: 2025-08-29T00:26:15.770Z
Learnt from: ksedgwic
PR: wled/WLED#4883
File: usermods/usermod_v2_skystrip/rest_json_client.h:6-14
Timestamp: 2025-08-29T00:26:15.770Z
Learning: In WLED projects, ArduinoJson.h is not directly included via #include <ArduinoJson.h> - the ArduinoJson symbols are made available through the WLED build system and wled.h transitive includes, so explicitly adding #include <ArduinoJson.h> is not necessary and may not work.
Applied to files:
usermods/usermod_v2_skystrip/rest_json_client.h
🧬 Code graph analysis (1)
usermods/usermod_v2_skystrip/util.h (1)
usermods/usermod_v2_skystrip/util.cpp (2)
hsv2rgb(8-34)hsv2rgb(8-8)
🪛 LanguageTool
usermods/usermod_v2_skystrip/FAQ.md
[grammar] ~4-~4: There might be a mistake here.
Context: ...e. Each view maps weather data onto hue, saturation, and value (brightness) along...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ...le value scales with probability. In the absence of precipitation, hue differenti...
(QB_NEW_EN)
[grammar] ~13-~13: There might be a mistake here.
Context: ... differentiates day from night: daylight clouds appear pale yellow, nighttime clo...
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ...llow, nighttime clouds desaturate toward white. For clouds, saturation is low and...
(QB_NEW_EN)
[grammar] ~15-~15: There might be a mistake here.
Context: ...on is low and value grows with coverage, keeping even thin clouds visible. Thus, ...
(QB_NEW_EN)
[grammar] ~16-~16: There might be a mistake here.
Context: ...le. Thus, a bright blue pixel highlights likely rain, whereas a soft yellow glow ...
(QB_NEW_EN)
[grammar] ~24-~24: There might be a mistake here.
Context: ...des for diagonal winds. Saturation rises with gustiness—calm breezes stay washed ...
(QB_NEW_EN)
[grammar] ~25-~25: There might be a mistake here.
Context: ...stay washed out while strong gusts drive the color toward full intensity. Value s...
(QB_NEW_EN)
[grammar] ~26-~26: There might be a mistake here.
Context: ...ensity. Value scales with wind strength, boosting brightness as the highest of su...
(QB_NEW_EN)
[grammar] ~27-~27: There might be a mistake here.
Context: ...s the highest of sustained speed or gust approaches 50 mph (or equivalent). For e...
(QB_NEW_EN)
[grammar] ~28-~28: There might be a mistake here.
Context: ...nt). For example, a saturated blue pixel indicates gusty north winds, while a dim...
(QB_NEW_EN)
[grammar] ~29-~29: There might be a mistake here.
Context: ...nds, while a dim pastel green suggests a gentle westerly breeze. The mapping bet...
(QB_NEW_EN)
[grammar] ~34-~34: There might be a mistake here.
Context: ...ed as: | Direction | Hue (°) | Color | |-----------|---------|--------| | N ...
(QB_NEW_EN)
[grammar] ~35-~35: There might be a mistake here.
Context: ...olor | |-----------|---------|--------| | N | 240 | Blue | | NE ...
(QB_NEW_EN)
[grammar] ~36-~36: There might be a mistake here.
Context: ...------| | N | 240 | Blue | | NE | 300 | Purple | | E ...
(QB_NEW_EN)
[grammar] ~37-~37: There might be a mistake here.
Context: ...lue | | NE | 300 | Purple | | E | 30 | Orange | | SE ...
(QB_NEW_EN)
[grammar] ~38-~38: There might be a mistake here.
Context: ...urple | | E | 30 | Orange | | SE | 45 | Gold | | S ...
(QB_NEW_EN)
[grammar] ~39-~39: There might be a mistake here.
Context: ...range | | SE | 45 | Gold | | S | 60 | Yellow | | SW ...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ...old | | S | 60 | Yellow | | SW | 90 | Lime | | W ...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ...ellow | | SW | 90 | Lime | | W | 120 | Green | | NW ...
(QB_NEW_EN)
[grammar] ~42-~42: There might be a mistake here.
Context: ...ime | | W | 120 | Green | | NW | 180 | Cyan | | N ...
(QB_NEW_EN)
[grammar] ~43-~43: There might be a mistake here.
Context: ...reen | | NW | 180 | Cyan | | N | 240 | Blue | (wraps ...
(QB_NEW_EN)
[grammar] ~53-~53: There might be a mistake here.
Context: ...nd reds at ~104 °F and above. Saturation reflects humidity via dew‑point spread; ...
(QB_NEW_EN)
[grammar] ~54-~54: There might be a mistake here.
Context: ...point spread; muggy air produces softer, desaturated colors, whereas dry air yiel...
(QB_NEW_EN)
[grammar] ~55-~55: There might be a mistake here.
Context: ...y air yields vivid tones. Value is fixed at mid‑brightness, but local time marker...
(QB_NEW_EN)
[grammar] ~56-~56: There might be a mistake here.
Context: ...ocal time markers (e.g., noon, midnight) temporarily darken pixels to mark time. ...
(QB_NEW_EN)
[grammar] ~57-~57: There might be a mistake here.
Context: ...ark time. A bright orange‑red pixel thus signifies hot, dry conditions around 95 ...
(QB_NEW_EN)
[grammar] ~58-~58: There might be a mistake here.
Context: ... around 95 °F, whereas a pale cyan pixel indicates a cool, humid day near 50 °F. ...
(QB_NEW_EN)
[grammar] ~63-~63: There might be a mistake here.
Context: ...: | Temp (°F) | Hue (°) | Color | |-----------|---------|-------------| | ...
(QB_NEW_EN)
[grammar] ~64-~64: There might be a mistake here.
Context: ... | |-----------|---------|-------------| | ≤14 | 234.9 | Deep blue | | ...
(QB_NEW_EN)
[grammar] ~65-~65: There might be a mistake here.
Context: ...-| | ≤14 | 234.9 | Deep blue | | 32 | 207.0 | Blue/cyan | | ...
(QB_NEW_EN)
[grammar] ~66-~66: There might be a mistake here.
Context: ... | | 32 | 207.0 | Blue/cyan | | 50 | 180.0 | Cyan | | ...
(QB_NEW_EN)
[grammar] ~67-~67: There might be a mistake here.
Context: ... | | 50 | 180.0 | Cyan | | 68 | 138.8 | Greenish | | ...
(QB_NEW_EN)
[grammar] ~68-~68: There might be a mistake here.
Context: ... | | 68 | 138.8 | Greenish | | 77 | 60.0 | Yellow | | ...
(QB_NEW_EN)
[grammar] ~69-~69: There might be a mistake here.
Context: ... | | 77 | 60.0 | Yellow | | 86 | 38.8 | Orange | | ...
(QB_NEW_EN)
[grammar] ~70-~70: There might be a mistake here.
Context: ... | | 86 | 38.8 | Orange | | 95 | 18.8 | Orange‑red | | ...
(QB_NEW_EN)
[grammar] ~71-~71: There might be a mistake here.
Context: ... | | 95 | 18.8 | Orange‑red | | ≥104 | 0.0 | Red | ...
(QB_NEW_EN)
[grammar] ~83-~83: There might be a mistake here.
Context: ...dim blue pixel therefore means a slight cool‑down with more moisture, while a bright satur...
(QB_NEW_EN)
[grammar] ~84-~84: There might be a mistake here.
Context: ...e a bright saturated red indicates rapid warming coupled with drying. Approximat...
(QB_NEW_EN)
[grammar] ~89-~89: There might be a mistake here.
Context: ...tributes: | Temperature | Hue (Color) | |-------------|-------------| | Cooling ...
(QB_NEW_EN)
[grammar] ~90-~90: There might be a mistake here.
Context: ... (Color) | |-------------|-------------| | Cooling | Blue tones | | Steady ...
(QB_NEW_EN)
[grammar] ~91-~91: There might be a mistake here.
Context: ...---------| | Cooling | Blue tones | | Steady | Green | | Warming ...
(QB_NEW_EN)
[grammar] ~92-~92: There might be a mistake here.
Context: ...e tones | | Steady | Green | | Warming | Yellow→Red | | Humidit...
(QB_NEW_EN)
[grammar] ~95-~95: There might be a mistake here.
Context: ...llow→Red | | Humidity | Saturation | |------------|------------| | More humid...
(QB_NEW_EN)
[grammar] ~96-~96: There might be a mistake here.
Context: ...Saturation | |------------|------------| | More humid | Low/Pastel | | Stable ...
(QB_NEW_EN)
[grammar] ~97-~97: There might be a mistake here.
Context: ...-----------| | More humid | Low/Pastel | | Stable | Medium | | Drier ...
(QB_NEW_EN)
[grammar] ~98-~98: There might be a mistake here.
Context: ...Low/Pastel | | Stable | Medium | | Drier | High/Vivid | ## Test Pa...
(QB_NEW_EN)
usermods/usermod_v2_skystrip/readme.md
[grammar] ~3-~3: Ensure spelling is correct
Context: # SkyStrip This usermod displays the weather forecast on severa...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~3-~3: There might be a mistake here.
Context: ...forecast on several parallel LED strips. It currently includes Cloud, Wind, Tempe...
(QB_NEW_EN)
[grammar] ~18-~18: There might be a mistake here.
Context: ...n the Latitude and Longitude fields. 2. Enter a combined lat/long string in the ...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
usermods/usermod_v2_skystrip/FAQ.md
44-44: Table pipe style
Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
(MD055, table-pipe-style)
44-44: Table column count
Expected: 3; Actual: 4; Too many cells, extra data will be missing
(MD056, table-column-count)
🔇 Additional comments (4)
usermods/usermod_v2_skystrip/rest_json_client.h (1)
39-53: LGTM — header layout and platform guards look solid.Non-copyable class, sized JSON buffer per target, and socket-timeout plumbing are appropriate.
usermods/usermod_v2_skystrip/open_weather_map_source.cpp (3)
217-226: Throttle timing logic LGTMUpdate of lastFetch_/lastHistFetch_ before network to prevent thrash is correct.
248-270: Sunrise/sunset fallback may invert day/night around boundariesUsing numeric limits as sentinels is fine, but inferring day/night from current.icon’s “n” can be wrong near twilight. If easy, prefer “current” has “sunrise/sunset” or derive from “daily[0]” when present.
Would you like me to add a fallback that checks daily[0].sunrise/sunset when current lacks them?
279-308: Wind gust optionality and POP range
- wind_gust is often absent; as() returns 0.0 which is fine—just confirming this is intended.
- OpenWeatherMap “pop” is 0..1. Ensure downstream views expect 0..1, not 0..100.
I can scan the repo and patch any 0..100 assumptions if needed.
|
Thanks for your contribution, this is a really elaborate UM! |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
usermods/usermod_v2_skystrip/FAQ.md (1)
34-46: Remove duplicated “N” row in wind-direction tableThe table lists “N” twice; keep a single entry to avoid confusion.
| NW | 180 | Cyan | -| N | 240 | Blue | Note: Hues wrap at 360°, so “N” repeats at the boundary.
🧹 Nitpick comments (5)
usermods/usermod_v2_skystrip/FAQ.md (5)
22-33: Add a brief units note (degrees vs 0–255 hue; mph vs metric)Clarify hue units and speed scaling so users can translate to WLED’s hue (0–255) and km/h.
Value scales with wind strength, boosting brightness as the highest of sustained speed or gust approaches 50 mph (or equivalent). For example, a saturated blue pixel indicates gusty north winds, while a dim pastel green suggests a gentle westerly breeze. + +Note on units: Hues are in degrees (0–360). WLED’s 0–255 hue maps via + hue8 = round(hue_deg × 255 / 360). +Wind speed scaling here references mph; use your preferred unit and +equivalent max when interpreting brightness.
75-75: Hyphenate section title for consistencyUse “24-hour” (hyphenated) to match common style elsewhere.
-## 24 Hour Delta View (DV) +## 24-hour Delta View (DV)
8-18: Specify numeric scaling for “value scales with …”Readers can better interpret brightness if ranges are stated (e.g., probability 0–100% → V 32–255; delta magnitude clamp). Document the exact maps used, or note defaults.
Also applies to: 75-86
1-7: Add a short Gallery with photos/GIFsA visual preview addresses recent feedback and makes the mapping intuitive.
# SkyStrip Interpretation Guide This FAQ explains how to read the various HSV-based views of the `usermod_v2_skystrip` module. Each view maps weather data onto hue, saturation, and value (brightness) along the LED strip. +### Gallery +<!-- Replace with real images/GIFs --> +- Cloud View — daytime clouds vs. rain +- Wind View — direction sweep +- Temperature View — cold→hot gradient +- Delta View — warming vs. cooling examples +- Test Pattern — orientation check
101-110: Add “Hardware/Platform notes” (flash/RAM, outputs, tested MCUs)Document practical limits and tested configs (as requested in PR discussion): number of parallel outputs, typical segment count/length, RAM/flash impact, and any platform restrictions.
LED orientation. +## Hardware/Platform notes +- Tested MCUs/boards: (list) +- Typical LED count/segments and parallel outputs supported: (values) +- Approx. flash and RAM overhead of this usermod: (sizes) +- Network/API usage (OpenWeatherMap): request interval and payload size +- Known limits or platform restrictions (e.g., ESP8266/ESP32 differences)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
usermods/usermod_v2_skystrip/FAQ.md(1 hunks)usermods/usermod_v2_skystrip/open_weather_map_source.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- usermods/usermod_v2_skystrip/open_weather_map_source.cpp
🧰 Additional context used
🪛 LanguageTool
usermods/usermod_v2_skystrip/FAQ.md
[grammar] ~4-~4: There might be a mistake here.
Context: ...e. Each view maps weather data onto hue, saturation, and value (brightness) along...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ...le value scales with probability. In the absence of precipitation, hue differenti...
(QB_NEW_EN)
[grammar] ~13-~13: There might be a mistake here.
Context: ... differentiates day from night: daylight clouds appear pale yellow, nighttime clo...
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ...llow, nighttime clouds desaturate toward white. For clouds, saturation is low and...
(QB_NEW_EN)
[grammar] ~15-~15: There might be a mistake here.
Context: ...on is low and value grows with coverage, keeping even thin clouds visible. Thus, ...
(QB_NEW_EN)
[grammar] ~16-~16: There might be a mistake here.
Context: ...le. Thus, a bright blue pixel highlights likely rain, whereas a soft yellow glow ...
(QB_NEW_EN)
[grammar] ~24-~24: There might be a mistake here.
Context: ...des for diagonal winds. Saturation rises with gustiness—calm breezes stay washed ...
(QB_NEW_EN)
[grammar] ~25-~25: There might be a mistake here.
Context: ...stay washed out while strong gusts drive the color toward full intensity. Value s...
(QB_NEW_EN)
[grammar] ~26-~26: There might be a mistake here.
Context: ...ensity. Value scales with wind strength, boosting brightness as the highest of su...
(QB_NEW_EN)
[grammar] ~27-~27: There might be a mistake here.
Context: ...s the highest of sustained speed or gust approaches 50 mph (or equivalent). For e...
(QB_NEW_EN)
[grammar] ~28-~28: There might be a mistake here.
Context: ...nt). For example, a saturated blue pixel indicates gusty north winds, while a dim...
(QB_NEW_EN)
[grammar] ~29-~29: There might be a mistake here.
Context: ...nds, while a dim pastel green suggests a gentle westerly breeze. The mapping bet...
(QB_NEW_EN)
[grammar] ~34-~34: There might be a mistake here.
Context: ...ed as: | Direction | Hue (°) | Color | |-----------|---------|--------| | N ...
(QB_NEW_EN)
[grammar] ~35-~35: There might be a mistake here.
Context: ...olor | |-----------|---------|--------| | N | 240 | Blue | | NE ...
(QB_NEW_EN)
[grammar] ~36-~36: There might be a mistake here.
Context: ...------| | N | 240 | Blue | | NE | 300 | Purple | | E ...
(QB_NEW_EN)
[grammar] ~37-~37: There might be a mistake here.
Context: ...lue | | NE | 300 | Purple | | E | 30 | Orange | | SE ...
(QB_NEW_EN)
[grammar] ~38-~38: There might be a mistake here.
Context: ...urple | | E | 30 | Orange | | SE | 45 | Gold | | S ...
(QB_NEW_EN)
[grammar] ~39-~39: There might be a mistake here.
Context: ...range | | SE | 45 | Gold | | S | 60 | Yellow | | SW ...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ...old | | S | 60 | Yellow | | SW | 90 | Lime | | W ...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ...ellow | | SW | 90 | Lime | | W | 120 | Green | | NW ...
(QB_NEW_EN)
[grammar] ~42-~42: There might be a mistake here.
Context: ...ime | | W | 120 | Green | | NW | 180 | Cyan | | N ...
(QB_NEW_EN)
[grammar] ~43-~43: There might be a mistake here.
Context: ...reen | | NW | 180 | Cyan | | N | 240 | Blue | Note: ...
(QB_NEW_EN)
[grammar] ~53-~53: There might be a mistake here.
Context: ...nd reds at ~104 °F and above. Saturation reflects humidity via dew‑point spread; ...
(QB_NEW_EN)
[grammar] ~54-~54: There might be a mistake here.
Context: ...point spread; muggy air produces softer, desaturated colors, whereas dry air yiel...
(QB_NEW_EN)
[grammar] ~55-~55: There might be a mistake here.
Context: ...y air yields vivid tones. Value is fixed at mid‑brightness, but local time marker...
(QB_NEW_EN)
[grammar] ~56-~56: There might be a mistake here.
Context: ...ocal time markers (e.g., noon, midnight) temporarily darken pixels to mark time. ...
(QB_NEW_EN)
[grammar] ~57-~57: There might be a mistake here.
Context: ...ark time. A bright orange‑red pixel thus signifies hot, dry conditions around 95 ...
(QB_NEW_EN)
[grammar] ~58-~58: There might be a mistake here.
Context: ... around 95 °F, whereas a pale cyan pixel indicates a cool, humid day near 50 °F. ...
(QB_NEW_EN)
[grammar] ~63-~63: There might be a mistake here.
Context: ...: | Temp (°F) | Hue (°) | Color | |-----------|---------|-------------| | ...
(QB_NEW_EN)
[grammar] ~64-~64: There might be a mistake here.
Context: ... | |-----------|---------|-------------| | ≤14 | 234.9 | Deep blue | | ...
(QB_NEW_EN)
[grammar] ~65-~65: There might be a mistake here.
Context: ...-| | ≤14 | 234.9 | Deep blue | | 32 | 207.0 | Blue/cyan | | ...
(QB_NEW_EN)
[grammar] ~66-~66: There might be a mistake here.
Context: ... | | 32 | 207.0 | Blue/cyan | | 50 | 180.0 | Cyan | | ...
(QB_NEW_EN)
[grammar] ~67-~67: There might be a mistake here.
Context: ... | | 50 | 180.0 | Cyan | | 68 | 138.8 | Greenish | | ...
(QB_NEW_EN)
[grammar] ~68-~68: There might be a mistake here.
Context: ... | | 68 | 138.8 | Greenish | | 77 | 60.0 | Yellow | | ...
(QB_NEW_EN)
[grammar] ~69-~69: There might be a mistake here.
Context: ... | | 77 | 60.0 | Yellow | | 86 | 38.8 | Orange | | ...
(QB_NEW_EN)
[grammar] ~70-~70: There might be a mistake here.
Context: ... | | 86 | 38.8 | Orange | | 95 | 18.8 | Orange‑red | | ...
(QB_NEW_EN)
[grammar] ~71-~71: There might be a mistake here.
Context: ... | | 95 | 18.8 | Orange‑red | | ≥104 | 0.0 | Red | ...
(QB_NEW_EN)
[grammar] ~83-~83: There might be a mistake here.
Context: ...dim blue pixel therefore means a slight cool‑down with more moisture, while a bright satur...
(QB_NEW_EN)
[grammar] ~84-~84: There might be a mistake here.
Context: ...e a bright saturated red indicates rapid warming coupled with drying. Approximat...
(QB_NEW_EN)
[grammar] ~89-~89: There might be a mistake here.
Context: ...tributes: | Temperature | Hue (Color) | |-------------|-------------| | Cooling ...
(QB_NEW_EN)
[grammar] ~90-~90: There might be a mistake here.
Context: ... (Color) | |-------------|-------------| | Cooling | Blue tones | | Steady ...
(QB_NEW_EN)
[grammar] ~91-~91: There might be a mistake here.
Context: ...---------| | Cooling | Blue tones | | Steady | Green | | Warming ...
(QB_NEW_EN)
[grammar] ~92-~92: There might be a mistake here.
Context: ...e tones | | Steady | Green | | Warming | Yellow→Red | | Humidit...
(QB_NEW_EN)
[grammar] ~95-~95: There might be a mistake here.
Context: ...llow→Red | | Humidity | Saturation | |------------|------------| | More humid...
(QB_NEW_EN)
[grammar] ~96-~96: There might be a mistake here.
Context: ...Saturation | |------------|------------| | More humid | Low/Pastel | | Stable ...
(QB_NEW_EN)
[grammar] ~97-~97: There might be a mistake here.
Context: ...-----------| | More humid | Low/Pastel | | Stable | Medium | | Drier ...
(QB_NEW_EN)
[grammar] ~98-~98: There might be a mistake here.
Context: ...Low/Pastel | | Stable | Medium | | Drier | High/Vivid | ## Test Pa...
(QB_NEW_EN)
🔇 Additional comments (1)
usermods/usermod_v2_skystrip/FAQ.md (1)
51-59: No changes needed: temperature→hue stops in FAQ.md match implementationVerified that the hue values in FAQ.md (lines 65–72) exactly correspond to the entries in kStopsF of temperature_view.cpp (lines 34–41); no update required.
That sounds like a great idea!
I've been considering adding user-configurable per-view color settings, maybe that is what you mean?
Will do, it definitely is sensitive to available heap ... I'll check out the link you referenced |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
usermods/usermod_v2_skystrip/FAQ.md (1)
34-45: Remove duplicated “N” row in wind-direction table.“N” appears twice; keep a single entry and keep the wrap note below the table.
| NW | 180 | Cyan | -| N | 240 | Blue |
🧹 Nitpick comments (5)
usermods/usermod_v2_skystrip/readme.md (3)
13-15: Align startup API call count (24 vs 25).Two different numbers are documented for the initial burst. Pick one and use it consistently.
If “up to 24” is correct, apply:
- SkyStrip makes 25 API calls to the + SkyStrip makes up to 24 API calls to the [OpenWeatherMap One Call API](https://openweathermap.org/api/one-call-3) - when it first starts running and one API call per hour after that. + when it first starts running and one API call per hour after that.Also applies to: 37-39
17-25: Document precedence between Location vs Latitude/Longitude.The code prefers an updated Location string to derive lat/long, but clears Location if lat/long change directly. Add a short note so users know which field “wins.”
Apply after the numbered list:
+Note: If you edit both fields, the Location string takes precedence and will +update Latitude/Longitude. If you change Latitude/Longitude directly without +changing Location, the Location field is cleared.
41-42: Unify list marker style (markdownlint MD004).Top-level list uses “-” but these sub-items use “*”.
- * RAM: +2080 bytes - * Flash: +153,812 bytes + - RAM: +2080 bytes + - Flash: +153,812 bytesusermods/usermod_v2_skystrip/usermod_v2_skystrip.cpp (2)
29-29: Make the delay constant constexpr.Tiny win and communicates intent; no runtime cost.
-const uint32_t SAFETY_DELAY_MS = 10u * 1000u; +constexpr uint32_t SAFETY_DELAY_MS = 10u * 1000u;
34-41: Use explicit std::make_unique instead of ::make_unique
Replace all instances of ::make_unique with std::make_unique to avoid relying on a global using-declaration (bus_manager.h) and improve clarity.- sources_.push_back(::make_unique<OpenWeatherMapSource>()); - model_ = ::make_unique<SkyModel>(); - views_.push_back(::make_unique<CloudView>()); - views_.push_back(::make_unique<WindView>()); - views_.push_back(::make_unique<TemperatureView>()); - views_.push_back(::make_unique<DeltaView>()); - views_.push_back(::make_unique<TestPatternView>()); + sources_.push_back(std::make_unique<OpenWeatherMapSource>()); + model_ = std::make_unique<SkyModel>(); + views_.push_back(std::make_unique<CloudView>()); + views_.push_back(std::make_unique<WindView>()); + views_.push_back(std::make_unique<TemperatureView>()); + views_.push_back(std::make_unique<DeltaView>()); + views_.push_back(std::make_unique<TestPatternView>());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
usermods/usermod_v2_skystrip/FAQ.md(1 hunks)usermods/usermod_v2_skystrip/open_weather_map_source.cpp(1 hunks)usermods/usermod_v2_skystrip/readme.md(1 hunks)usermods/usermod_v2_skystrip/usermod_v2_skystrip.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- usermods/usermod_v2_skystrip/open_weather_map_source.cpp
🧰 Additional context used
🧬 Code graph analysis (1)
usermods/usermod_v2_skystrip/usermod_v2_skystrip.cpp (9)
usermods/usermod_v2_skystrip/usermod_v2_skystrip.h (2)
SkyStrip(19-53)setup(37-43)usermods/usermod_v2_skystrip/util.h (2)
time_now_utc(26-26)s(14-20)usermods/usermod_v2_skystrip/cloud_view.h (1)
view(13-22)usermods/usermod_v2_skystrip/delta_view.h (1)
view(13-22)usermods/usermod_v2_skystrip/temperature_view.h (1)
view(14-24)usermods/usermod_v2_skystrip/test_pattern_view.h (1)
view(13-22)usermods/usermod_v2_skystrip/wind_view.h (1)
view(13-22)usermods/usermod_v2_skystrip/open_weather_map_source.cpp (4)
addToConfig(203-210)addToConfig(203-203)readFromConfig(212-257)readFromConfig(212-214)usermods/usermod_v2_skystrip/skymodel.cpp (2)
invalidate_history(81-92)invalidate_history(81-81)
🪛 LanguageTool
usermods/usermod_v2_skystrip/readme.md
[grammar] ~3-~3: Ensure spelling is correct
Context: # SkyStrip This usermod displays the weather forecast on severa...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~3-~3: There might be a mistake here.
Context: ...forecast on several parallel LED strips. It currently includes Cloud, Wind, Tempe...
(QB_NEW_EN)
[grammar] ~28-~28: There might be a mistake here.
Context: ..../FAQ.md) for more information on how to interpret the forecast views. ## Hardwa...
(QB_NEW_EN)
[grammar] ~34-~34: There might be a mistake here.
Context: ...music-addressable-led-strip-controller). - I use a display consisting of 4 parallel...
(QB_NEW_EN)
[grammar] ~39-~39: There might be a mistake here.
Context: ...ng and one API call per hour after that. - Based on comparisons with a baseline bui...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ...ons with a baseline build SkyStrip uses: * RAM: +2080 bytes * Flash: +153,812 byt...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ...uild SkyStrip uses: * RAM: +2080 bytes * Flash: +153,812 bytes
(QB_NEW_EN)
[grammar] ~42-~42: There might be a mistake here.
Context: ...M: +2080 bytes * Flash: +153,812 bytes
(QB_NEW_EN)
usermods/usermod_v2_skystrip/FAQ.md
[grammar] ~4-~4: There might be a mistake here.
Context: ...e. Each view maps weather data onto hue, saturation, and value (brightness) along...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ...le value scales with probability. In the absence of precipitation, hue differenti...
(QB_NEW_EN)
[grammar] ~13-~13: There might be a mistake here.
Context: ... differentiates day from night: daylight clouds appear pale yellow, nighttime clo...
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ...llow, nighttime clouds desaturate toward white. For clouds, saturation is low and...
(QB_NEW_EN)
[grammar] ~15-~15: There might be a mistake here.
Context: ...on is low and value grows with coverage, keeping even thin clouds visible. Thus, ...
(QB_NEW_EN)
[grammar] ~16-~16: There might be a mistake here.
Context: ...le. Thus, a bright blue pixel highlights likely rain, whereas a soft yellow glow ...
(QB_NEW_EN)
[grammar] ~24-~24: There might be a mistake here.
Context: ...des for diagonal winds. Saturation rises with gustiness—calm breezes stay washed ...
(QB_NEW_EN)
[grammar] ~25-~25: There might be a mistake here.
Context: ...stay washed out while strong gusts drive the color toward full intensity. Value s...
(QB_NEW_EN)
[grammar] ~26-~26: There might be a mistake here.
Context: ...ensity. Value scales with wind strength, boosting brightness as the highest of su...
(QB_NEW_EN)
[grammar] ~27-~27: There might be a mistake here.
Context: ...s the highest of sustained speed or gust approaches 50 mph (or equivalent). For e...
(QB_NEW_EN)
[grammar] ~28-~28: There might be a mistake here.
Context: ...nt). For example, a saturated blue pixel indicates gusty north winds, while a dim...
(QB_NEW_EN)
[grammar] ~29-~29: There might be a mistake here.
Context: ...nds, while a dim pastel green suggests a gentle westerly breeze. The mapping bet...
(QB_NEW_EN)
[grammar] ~34-~34: There might be a mistake here.
Context: ...ed as: | Direction | Hue (°) | Color | |-----------|---------|--------| | N ...
(QB_NEW_EN)
[grammar] ~35-~35: There might be a mistake here.
Context: ...olor | |-----------|---------|--------| | N | 240 | Blue | | NE ...
(QB_NEW_EN)
[grammar] ~36-~36: There might be a mistake here.
Context: ...------| | N | 240 | Blue | | NE | 300 | Purple | | E ...
(QB_NEW_EN)
[grammar] ~37-~37: There might be a mistake here.
Context: ...lue | | NE | 300 | Purple | | E | 30 | Orange | | SE ...
(QB_NEW_EN)
[grammar] ~38-~38: There might be a mistake here.
Context: ...urple | | E | 30 | Orange | | SE | 45 | Gold | | S ...
(QB_NEW_EN)
[grammar] ~39-~39: There might be a mistake here.
Context: ...range | | SE | 45 | Gold | | S | 60 | Yellow | | SW ...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ...old | | S | 60 | Yellow | | SW | 90 | Lime | | W ...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ...ellow | | SW | 90 | Lime | | W | 120 | Green | | NW ...
(QB_NEW_EN)
[grammar] ~42-~42: There might be a mistake here.
Context: ...ime | | W | 120 | Green | | NW | 180 | Cyan | | N ...
(QB_NEW_EN)
[grammar] ~43-~43: There might be a mistake here.
Context: ...reen | | NW | 180 | Cyan | | N | 240 | Blue | Note: ...
(QB_NEW_EN)
[grammar] ~53-~53: There might be a mistake here.
Context: ...nd reds at ~104 °F and above. Saturation reflects humidity via dew‑point spread; ...
(QB_NEW_EN)
[grammar] ~54-~54: There might be a mistake here.
Context: ...point spread; muggy air produces softer, desaturated colors, whereas dry air yiel...
(QB_NEW_EN)
[grammar] ~55-~55: There might be a mistake here.
Context: ...y air yields vivid tones. Value is fixed at mid‑brightness, but local time marker...
(QB_NEW_EN)
[grammar] ~56-~56: There might be a mistake here.
Context: ...ocal time markers (e.g., noon, midnight) temporarily darken pixels to mark time. ...
(QB_NEW_EN)
[grammar] ~57-~57: There might be a mistake here.
Context: ...ark time. A bright orange‑red pixel thus signifies hot, dry conditions around 95 ...
(QB_NEW_EN)
[grammar] ~58-~58: There might be a mistake here.
Context: ... around 95 °F, whereas a pale cyan pixel indicates a cool, humid day near 50 °F. ...
(QB_NEW_EN)
[grammar] ~63-~63: There might be a mistake here.
Context: ...: | Temp (°F) | Hue (°) | Color | |-----------|---------|-------------| | ...
(QB_NEW_EN)
[grammar] ~64-~64: There might be a mistake here.
Context: ... | |-----------|---------|-------------| | ≤14 | 234.9 | Deep blue | | ...
(QB_NEW_EN)
[grammar] ~65-~65: There might be a mistake here.
Context: ...-| | ≤14 | 234.9 | Deep blue | | 32 | 207.0 | Blue/cyan | | ...
(QB_NEW_EN)
[grammar] ~66-~66: There might be a mistake here.
Context: ... | | 32 | 207.0 | Blue/cyan | | 50 | 180.0 | Cyan | | ...
(QB_NEW_EN)
[grammar] ~67-~67: There might be a mistake here.
Context: ... | | 50 | 180.0 | Cyan | | 68 | 138.8 | Greenish | | ...
(QB_NEW_EN)
[grammar] ~68-~68: There might be a mistake here.
Context: ... | | 68 | 138.8 | Greenish | | 77 | 60.0 | Yellow | | ...
(QB_NEW_EN)
[grammar] ~69-~69: There might be a mistake here.
Context: ... | | 77 | 60.0 | Yellow | | 86 | 38.8 | Orange | | ...
(QB_NEW_EN)
[grammar] ~70-~70: There might be a mistake here.
Context: ... | | 86 | 38.8 | Orange | | 95 | 18.8 | Orange‑red | | ...
(QB_NEW_EN)
[grammar] ~71-~71: There might be a mistake here.
Context: ... | | 95 | 18.8 | Orange‑red | | ≥104 | 0.0 | Red | ...
(QB_NEW_EN)
[grammar] ~83-~83: There might be a mistake here.
Context: ...dim blue pixel therefore means a slight cool‑down with more moisture, while a bright satur...
(QB_NEW_EN)
[grammar] ~84-~84: There might be a mistake here.
Context: ...e a bright saturated red indicates rapid warming coupled with drying. Approximat...
(QB_NEW_EN)
[grammar] ~89-~89: There might be a mistake here.
Context: ...tributes: | Temperature | Hue (Color) | |-------------|-------------| | Cooling ...
(QB_NEW_EN)
[grammar] ~90-~90: There might be a mistake here.
Context: ... (Color) | |-------------|-------------| | Cooling | Blue tones | | Steady ...
(QB_NEW_EN)
[grammar] ~91-~91: There might be a mistake here.
Context: ...---------| | Cooling | Blue tones | | Steady | Green | | Warming ...
(QB_NEW_EN)
[grammar] ~92-~92: There might be a mistake here.
Context: ...e tones | | Steady | Green | | Warming | Yellow→Red | | Humidit...
(QB_NEW_EN)
[grammar] ~95-~95: There might be a mistake here.
Context: ...llow→Red | | Humidity | Saturation | |------------|------------| | More humid...
(QB_NEW_EN)
[grammar] ~96-~96: There might be a mistake here.
Context: ...Saturation | |------------|------------| | More humid | Low/Pastel | | Stable ...
(QB_NEW_EN)
[grammar] ~97-~97: There might be a mistake here.
Context: ...-----------| | More humid | Low/Pastel | | Stable | Medium | | Drier ...
(QB_NEW_EN)
[grammar] ~98-~98: There might be a mistake here.
Context: ...Low/Pastel | | Stable | Medium | | Drier | High/Vivid | ## Test Pa...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
usermods/usermod_v2_skystrip/readme.md
41-41: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
42-42: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
usermods/usermod_v2_skystrip/readme.md (1)
33-36: Add a short descriptive label so the link isn’t required for context.If the product page moves, readers still know what hardware you used.
-- SkyStrip was developed/tested using the - [Athom esp32-based LED strip controller](https://www.athom.tech/blank-1/wled-esp32-rf433-music-addressable-led-strip-controller). +- SkyStrip was developed/tested using the Athom ESP32-based addressable LED strip controller + (ESP32, RF433, WLED-ready): + https://www.athom.tech/blank-1/wled-esp32-rf433-music-addressable-led-strip-controller
🧹 Nitpick comments (8)
usermods/usermod_v2_skystrip/readme.md (8)
4-4: Standardize view naming and hyphenation.Use consistent casing and hyphenation for view names.
-It currently includes Cloud, Wind, Temperature, 24 Hour Delta, and TestPattern views. +It currently includes Cloud, Wind, Temperature, 24‑hour Delta, and Test Pattern views.
12-16: Avoid duplicating the API call pattern. Keep one canonical note.You state the call pattern in “Configuration” and again in “Hardware/Platform notes.” Remove the later repetition to reduce drift.
- - SkyStrip makes 25 API calls to the - [OpenWeatherMap One Call API](https://openweathermap.org/api/one-call-3) - when it first starts running and one API call per hour after that.Also applies to: 42-45
35-41: Replace deep vendor product link with vendor‑agnostic description.That long, parameterized URL is fragile. Describe the strip clearly; keep docs future‑proof.
-- Display used for development: four - [WS2815 LED strips](https://www.superlightingled.com/dc12v-ws2815-upgraded-ws2812b-1m-144-leds-individually-addressable-digital-led-strip-lights-dual-signal-wires-waterproof-dream-color-programmable-5050-rgb-flexible-led-ribbon-light-p-2134:fd57dd8a8ac1ee0e78f5493a35b28792.html) - , each 1 m long, - 12 V, 5050 RGB, 144 LEDs/m, individually addressable with - dual-signal (backup) line; arranged side‑by‑side (physically - parallel). Any equivalent WS281x‑compatible strip of similar density - should work; adjust power and wiring accordingly. +- Display used for development: four WS2815 12 V 5050 RGB LED strips, + 1 m each, 144 LEDs/m, individually addressable with dual‑signal (backup) line; + arranged side‑by‑side (physically parallel). Any equivalent WS281x‑compatible + strip of similar density should work; adjust power and wiring accordingly.
45-47: Fix markdown list‑style to satisfy MD004 (dash style).Current list uses asterisks; the rest of the doc uses dashes.
- * RAM: +2080 bytes - * Flash: +153,812 bytes + - RAM: +2080 bytes + - Flash: +153,812 bytes
8-9: Provide a concrete PlatformIO example.Small snippet reduces setup friction.
Add `usermod_v2_skystrip` to `custom_usermods` in your PlatformIO environment. + +Example (platformio.ini): + +```ini +[env:esp32dev] +platform = espressif32 +framework = arduino +board = esp32dev +build_flags = + -D USERMOD_SkyStrip=1 + -D WLED_USE_MY_CONFIG +custom_usermods = + usermods/usermod_v2_skystrip +``` +```
17-24: Document precedence when both Location and Lat/Long are set.Clarify which source wins and whether geocoding overrides manual coordinates.
I can add a short note once you confirm the precedence rules (e.g., “explicit Lat/Long overrides Location string; Location geocoded only if Lat/Long empty”).
10-16: Mention units and time basis.State default units (metric/imperial), how to change them, and what timezone the forecast uses for rendering.
This typically stays within free-tier limits, but check your current plan. +By default, SkyStrip uses <metric|imperial> units and <local|UTC> time for day/night +transitions. You can change units/time basis in the module settings.
26-31: Add a small “Gallery” section with photos/GIFs.Helps users understand the visuals; aligns with review feedback.
## Interpretation Please see the [Interpretation FAQ](./FAQ.md) for more information on how to interpret the forecast views. +## Gallery + +Photos and short clips of the Cloud, Wind, Temperature, Delta, and Test Pattern views. +Place images in `usermods/usermod_v2_skystrip/docs/` and reference them here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
usermods/usermod_v2_skystrip/readme.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
usermods/usermod_v2_skystrip/readme.md
[grammar] ~3-~3: Ensure spelling is correct
Context: # SkyStrip This usermod displays the weather forecast on severa...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~3-~3: There might be a mistake here.
Context: ...forecast on several parallel LED strips. It currently includes Cloud, Wind, Tempe...
(QB_NEW_EN)
[grammar] ~28-~28: There might be a mistake here.
Context: ..../FAQ.md) for more information on how to interpret the forecast views. ## Hardwa...
(QB_NEW_EN)
[grammar] ~34-~34: There might be a mistake here.
Context: ...music-addressable-led-strip-controller). - Display used for development: four [WS...
(QB_NEW_EN)
[grammar] ~35-~35: There might be a mistake here.
Context: ...r). - Display used for development: four [WS2815 LED strips](https://www.superlig...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ...rk; adjust power and wiring accordingly. - SkyStrip makes 25 API calls to the [Op...
(QB_NEW_EN)
[grammar] ~44-~44: There might be a mistake here.
Context: ...ng and one API call per hour after that. - Based on comparisons with a baseline bui...
(QB_NEW_EN)
[grammar] ~45-~45: There might be a mistake here.
Context: ...ons with a baseline build SkyStrip uses: * RAM: +2080 bytes * Flash: +153,812 byt...
(QB_NEW_EN)
[grammar] ~46-~46: There might be a mistake here.
Context: ...uild SkyStrip uses: * RAM: +2080 bytes * Flash: +153,812 bytes
(QB_NEW_EN)
[grammar] ~47-~47: There might be a mistake here.
Context: ...M: +2080 bytes * Flash: +153,812 bytes
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
usermods/usermod_v2_skystrip/readme.md
46-46: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
47-47: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
usermods/usermod_v2_skystrip/readme.md (2)
3-4: Add visuals and briefly explain how strips map to viewsA photo/GIF and a one‑sentence note on how many parallel strips are expected and how views map to them will make behavior immediately clear.
37-43: State platform support, outputs, power, and add wiring diagram/photoExplicitly mark ESP32‑only (if true), typical number of parallel outputs, and include a simple wiring diagram and power budget note.
Apply:
- SkyStrip was developed/tested using the [Athom esp32-based LED strip controller](https://www.athom.tech/blank-1/wled-esp32-rf433-music-addressable-led-strip-controller). + (ESP32 platform; not recommended on ESP8266 due to memory constraints.) - Display used for development: four WS2815 12 V 5050 RGB LED strips, @@ arranged side‑by‑side (physically parallel). Any equivalent WS281x‑compatible strip of similar density should work; adjust power and wiring accordingly. + Provide a wiring diagram showing data channels, ground, and power injection. +Power: budget for worst‑case brightness; avoid powering strips from USB; use proper fusing and common ground.
🧹 Nitpick comments (7)
usermods/usermod_v2_skystrip/readme.md (7)
4-4: Consistent naming and hyphenation for view listSuggest user-facing names: "24‑hour Delta" and "Test Pattern" (space, non‑breaking hyphen) for readability.
Apply:
-It currently includes Cloud, Wind, Temperature, 24-Hour Delta, and TestPattern views. +It currently includes Cloud, Wind, Temperature, 24‑hour Delta, and Test Pattern views.
8-8: Show a concrete PlatformIO snippet and minimum WLED versionAdd a minimal platformio.ini example and state the minimum WLED version/commit this depends on.
Apply:
Add `usermod_v2_skystrip` to `custom_usermods` in your PlatformIO environment. + +Example (fragment): + +; WLED ≥ 0.14.x (confirm exact minimum) +[env:wled-esp32] +platform = espressif32 +board = esp32dev +framework = arduino +custom_usermods = usermod_v2_skystrip +build_flags = + -D WLED_RELEASE_NAME="SkyStrip demo" + +Note: Requires ESP32 builds of WLED (confirm the minimum version/commit tested).
12-15: Clarify call burst behavior, caching, backoff, and API key handlingNote when the initial burst reoccurs (e.g., after reset/clear), whether responses are cached, any retry/backoff, and that the API key is stored in WLED config and visible to anyone with admin access.
Apply:
module makes one API call per hour, plus up to 24 calls on first startup. This typically stays within free-tier limits, but check your current plan. +On reboot or after clearing settings, the initial burst may repeat until data is cached. +If the API is unreachable, requests back off (document current strategy) to avoid rapid retries. +Security: the API key is stored in the device configuration; anyone with admin access to WLED can view it.
17-25: Call out geocoding ambiguity and prefer lat/long for accuracyGeolocation strings can resolve unexpectedly; advising lat/long for precise targeting reduces surprises.
Apply:
3. Enter a geo-location string (e.g., `oakland,ca,us`) in the `Location` field. + Note: City/region names can be ambiguous; for precise results prefer latitude/longitude.
26-28: Tighten precedence wording to avoid confusionBe explicit that precedence applies at save time and what happens on subsequent edits.
Apply:
-Note: If you edit both fields, the Location string takes precedence and will -update Latitude/Longitude. If you change Latitude/Longitude directly without -changing Location, the Location field is cleared. +Note: On save, if both are edited, Location takes precedence and will overwrite Latitude/Longitude. +If you later change Latitude/Longitude without changing Location, the Location field is cleared.
32-33: Add one‑liners for each view (quick scan before FAQ)Brief summaries reduce context‑switching; keep FAQ link for details.
Apply:
Please see the [Interpretation FAQ](./FAQ.md) for more information on how to interpret the forecast views. +At a glance: +- Cloud: cloudiness over time mapped to brightness/color. +- Wind: wind speed/direction cues along the strips. +- Temperature: absolute temperature gradient. +- 24‑hour Delta: change vs. 24h prior (warming/cooling). +- Test Pattern: sanity check for wiring/orientation.
43-45: Make RAM/Flash figures reproducibleInclude board, toolchain, WLED version/commit, and build flags used to derive these numbers.
Apply:
-- Based on comparisons with a baseline build SkyStrip uses: +- Based on comparisons with a baseline build (specify board, WLED version/commit, toolchain) SkyStrip uses: - RAM: +2080 bytes - Flash: +153,812 bytes
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
usermods/usermod_v2_skystrip/readme.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
usermods/usermod_v2_skystrip/readme.md
[grammar] ~3-~3: Ensure spelling is correct
Context: # SkyStrip This usermod displays the weather forecast on severa...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~3-~3: There might be a mistake here.
Context: ...forecast on several parallel LED strips. It currently includes Cloud, Wind, Tempe...
(QB_NEW_EN)
[grammar] ~18-~18: There might be a mistake here.
Context: ...n the Latitude and Longitude fields. 2. Enter a combined lat/long string in the ...
(QB_NEW_EN)
[grammar] ~26-~26: There might be a mistake here.
Context: ...ocation string takes precedence and will update Latitude/Longitude. If you change...
(QB_NEW_EN)
[grammar] ~27-~27: There might be a mistake here.
Context: ...ange Latitude/Longitude directly without changing Location, the Location field is...
(QB_NEW_EN)
[grammar] ~32-~32: There might be a mistake here.
Context: ..../FAQ.md) for more information on how to interpret the forecast views. ## Hardwa...
(QB_NEW_EN)
[grammar] ~38-~38: There might be a mistake here.
Context: ...music-addressable-led-strip-controller). - Display used for development: four WS281...
(QB_NEW_EN)
[grammar] ~42-~42: There might be a mistake here.
Context: ...rk; adjust power and wiring accordingly. - Based on comparisons with a baseline bui...
(QB_NEW_EN)
[grammar] ~43-~43: There might be a mistake here.
Context: ...ons with a baseline build SkyStrip uses: - RAM: +2080 bytes - Flash: +153,812 byt...
(QB_NEW_EN)
[grammar] ~44-~44: There might be a mistake here.
Context: ...uild SkyStrip uses: - RAM: +2080 bytes - Flash: +153,812 bytes
(QB_NEW_EN)
[grammar] ~45-~45: There might be a mistake here.
Context: ...M: +2080 bytes - Flash: +153,812 bytes
(QB_NEW_EN)
| strip of similar density should work; adjust power and wiring accordingly. | ||
| - Based on comparisons with a baseline build SkyStrip uses: | ||
| - RAM: +2080 bytes | ||
| - Flash: +153,812 bytes |
There was a problem hiding this comment.
you are probably aware of this, but 150k is A LOT. Just know this may stop compiling at one point and you will get support questions :)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
usermods/usermod_v2_skystrip/delta_view.cpp (1)
84-88: Nice guard on invalid/empty segmentsThe switch to
len <= 0correctly handles zero and inverted ranges.
🧹 Nitpick comments (12)
usermods/usermod_v2_skystrip/wind_view.h (1)
3-9: Trim header deps: drop redundant include of skymodel.hYou already forward‑declare SkyModel; include causes unnecessary rebuilds. Keep the forward decl and include skymodel.h in the .cpp.
#include "interfaces.h" -#include "skymodel.h" #include "util.h" -class SkyModel; +class SkyModel;usermods/usermod_v2_skystrip/temperature_view.cpp (1)
156-160: Marker blending darkens to black; confirm intentcolor_blend(col, 0, blend) creates dark notches. If you intended colored markers, blend toward a marker color instead of black.
- uint8_t blend = uint8_t(std::lround(m * 255.f)); - col = color_blend(col, 0, blend); + uint8_t blend = uint8_t(std::lround(m * 255.f)); + const uint32_t marker = skystrip::util::hsv2rgb(25.f, 0.6f, 0.5f); + col = color_blend(col, marker, blend);usermods/usermod_v2_skystrip/cloud_view.cpp (2)
132-135: Clamp precip type to known rangeGuard against out‑of‑range interpolation artifacts before branching.
- int p = int(std::round(precipTypeVal)); + int p = int(std::lround(precipTypeVal)); + if (p < 0) p = 0; else if (p > 3) p = 3; // 0:none,1:rain,2:snow,3:mixed
12-20: Document sunrise/sunset sentinelsisDay() relies on sunrise_/sunset_ sentinel values (0/MAX). Add a brief comment in skymodel.h where these are defined.
usermods/usermod_v2_skystrip/test_pattern_view.cpp (1)
124-131: Use shortest‑arc hue interpolationAvoids 350°→10° crossing the long way around the wheel.
- float h = startHue_ + (endHue_ - startHue_) * u; + float dh = fmodf((endHue_ - startHue_) + 540.f, 360.f) - 180.f; // (-180,180] + float h = startHue_ + dh * u; + if (h < 0.f) h += 360.f; else if (h >= 360.f) h -= 360.f;usermods/usermod_v2_skystrip/delta_view.h (2)
3-7: Trim header dependencies: forward‑declare SkyModel; include it only in the .cppThe header already forward‑declares SkyModel (Line 7). Drop the direct include to reduce compile times and coupling.
#include "interfaces.h" -#include "skymodel.h" #include "util.h" class SkyModel;
28-29: Debug buffer sizing consistencyDV uses 256 bytes here while TP uses 128. If the same UI consumes both, consider standardizing to 256 to avoid truncation differences in debug output.
usermods/usermod_v2_skystrip/test_pattern_view.h (2)
3-7: Same include hygiene as DV: keep only the forward declarationKeep the forward declaration and move the heavy include to the .cpp (it’s already included there).
#include "interfaces.h" -#include "skymodel.h" #include "util.h" class SkyModel;
28-28: Unify debug buffer size with DV to avoid accidental truncationTP’s debug format can approach 100+ chars; DV allocates 256. Align TP to 256 for parity and headroom.
- char debugPixelString[128]; + char debugPixelString[256];usermods/usermod_v2_skystrip/delta_view.cpp (3)
80-91: Segment freeze lifetime: confirm intended scope; consider releasing at end of view()You acquire a freeze handle but don’t release it on the normal path. If the intention is to keep the segment frozen across frames, this is fine. If not, release after drawing to avoid holding the freeze longer than necessary.
// Initialize segment drawing parameters so virtualLength()/mapping are valid seg.beginDraw(); constexpr double kHorizonSec = 48.0 * 3600.0; const double step = (len > 1) ? (kHorizonSec / double(len - 1)) : 0.0; const time_t day = 24 * 3600; for (int i = 0; i < len; ++i) { ... } + + // If persistent freezing is not required across frames, release here. + // Otherwise, keep as-is and rely on deactivate()/re-acquire behavior. + // freezeHandle_.release();
27-39: Minor: avoid repeated sizeof expressions; name the countSmall readability win and avoids recomputing the array size macro expression.
static float hueForDeltaF(double f) { - if (f <= kStopsF[0].f) + constexpr size_t kStopCount = sizeof(kStopsF) / sizeof(kStopsF[0]); + if (f <= kStopsF[0].f) return kStopsF[0].h; - for (size_t i = 1; i < sizeof(kStopsF) / sizeof(kStopsF[0]); ++i) { + for (size_t i = 1; i < kStopCount; ++i) { if (f <= kStopsF[i].f) { const auto &A = kStopsF[i - 1]; const auto &B = kStopsF[i]; const double u = (f - A.f) / (B.f - A.f); return float(skystrip::util::lerp(A.h, B.h, u)); } } - return kStopsF[sizeof(kStopsF) / sizeof(kStopsF[0]) - 1].h; + return kStopsF[kStopCount - 1].h; }
92-98: Optional: use float math in hot loop to reduce FP costOn ESP targets, double math can be slower and larger. Using float for horizon/step and
lroundfcan reduce cycles and code size with negligible accuracy impact for LED timing.- constexpr double kHorizonSec = 48.0 * 3600.0; - const double step = (len > 1) ? (kHorizonSec / double(len - 1)) : 0.0; + constexpr float kHorizonSec = 48.0f * 3600.0f; + const float step = (len > 1) ? (kHorizonSec / float(len - 1)) : 0.0f; const time_t day = 24 * 3600; for (int i = 0; i < len; ++i) { - const time_t t = now + time_t(std::llround(step * i)); + const time_t t = now + time_t(lroundf(step * i));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
usermods/usermod_v2_skystrip/cloud_view.cpp(1 hunks)usermods/usermod_v2_skystrip/cloud_view.h(1 hunks)usermods/usermod_v2_skystrip/delta_view.cpp(1 hunks)usermods/usermod_v2_skystrip/delta_view.h(1 hunks)usermods/usermod_v2_skystrip/interfaces.h(1 hunks)usermods/usermod_v2_skystrip/temperature_view.cpp(1 hunks)usermods/usermod_v2_skystrip/temperature_view.h(1 hunks)usermods/usermod_v2_skystrip/test_pattern_view.cpp(1 hunks)usermods/usermod_v2_skystrip/test_pattern_view.h(1 hunks)usermods/usermod_v2_skystrip/usermod_v2_skystrip.cpp(1 hunks)usermods/usermod_v2_skystrip/util.h(1 hunks)usermods/usermod_v2_skystrip/wind_view.cpp(1 hunks)usermods/usermod_v2_skystrip/wind_view.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- usermods/usermod_v2_skystrip/interfaces.h
- usermods/usermod_v2_skystrip/wind_view.cpp
- usermods/usermod_v2_skystrip/temperature_view.h
- usermods/usermod_v2_skystrip/cloud_view.h
- usermods/usermod_v2_skystrip/usermod_v2_skystrip.cpp
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-04-27T09:37:28.415Z
Learnt from: KrX3D
PR: wled/WLED#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/usermod_v2_skystrip/test_pattern_view.cpp
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
usermods/usermod_v2_skystrip/test_pattern_view.cpp
📚 Learning: 2025-06-07T15:58:42.579Z
Learnt from: willmmiles
PR: wled/WLED#4712
File: wled00/FX_fcn.cpp:992-993
Timestamp: 2025-06-07T15:58:42.579Z
Learning: In WLED segments, the backing pixel buffer is always allocated to the full physical segment size, but effect rendering only uses a subset when mirror or grouping are enabled. For 2D segments, this subset is vWidth() * vHeight(), and for 1D segments it's vLength(). Color processing operations like fade_out should iterate over this rendering subset, not the full allocated buffer.
Applied to files:
usermods/usermod_v2_skystrip/temperature_view.cpp
📚 Learning: 2025-04-30T05:41:03.633Z
Learnt from: blazoncek
PR: wled/WLED#4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like `if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))` is unnecessary and could be problematic, as it bypasses proper size verification.
Applied to files:
usermods/usermod_v2_skystrip/temperature_view.cpp
🧬 Code graph analysis (8)
usermods/usermod_v2_skystrip/test_pattern_view.cpp (3)
usermods/usermod_v2_skystrip/util.h (2)
clamp01(85-87)blinkDebug(216-223)usermods/usermod_v2_skystrip/test_pattern_view.h (2)
TestPatternView(9-32)view(14-24)usermods/usermod_v2_skystrip/util.cpp (2)
hsv2rgb(8-34)hsv2rgb(8-8)
usermods/usermod_v2_skystrip/delta_view.h (4)
usermods/usermod_v2_skystrip/skymodel.cpp (3)
SkyModel(46-55)invalidate_history(81-92)invalidate_history(81-81)usermods/usermod_v2_skystrip/skymodel.h (1)
SkyModel(16-47)usermods/usermod_v2_skystrip/delta_view.cpp (11)
DeltaView(56-61)view(63-164)view(63-63)deactivate(166-168)deactivate(166-166)addToConfig(170-172)addToConfig(170-170)appendConfigData(174-179)appendConfigData(174-174)readFromConfig(181-187)readFromConfig(181-182)usermods/usermod_v2_skystrip/interfaces.h (1)
std(50-57)
usermods/usermod_v2_skystrip/test_pattern_view.h (4)
usermods/usermod_v2_skystrip/skymodel.h (1)
SkyModel(16-47)usermods/usermod_v2_skystrip/test_pattern_view.cpp (11)
TestPatternView(87-94)view(96-143)view(96-97)deactivate(145-147)deactivate(145-145)addToConfig(149-157)addToConfig(149-149)appendConfigData(159-164)appendConfigData(159-159)readFromConfig(166-195)readFromConfig(166-167)usermods/usermod_v2_skystrip/interfaces.h (1)
std(50-57)usermods/usermod_v2_skystrip/usermod_v2_skystrip.cpp (6)
addToConfig(135-155)addToConfig(135-135)appendConfigData(157-196)appendConfigData(157-157)readFromConfig(199-238)readFromConfig(199-199)
usermods/usermod_v2_skystrip/cloud_view.cpp (3)
usermods/usermod_v2_skystrip/cloud_view.h (2)
CloudView(9-30)view(14-24)usermods/usermod_v2_skystrip/util.h (6)
current_offset(67-73)estimateCloudAt(172-175)estimatePrecipTypeAt(176-179)estimatePrecipProbAt(180-183)clamp01(85-87)blinkDebug(216-223)usermods/usermod_v2_skystrip/util.cpp (2)
hsv2rgb(8-34)hsv2rgb(8-8)
usermods/usermod_v2_skystrip/delta_view.cpp (3)
usermods/usermod_v2_skystrip/util.h (3)
lerp(90-90)clamp01(85-87)blinkDebug(216-223)usermods/usermod_v2_skystrip/delta_view.h (2)
DeltaView(9-30)view(14-24)usermods/usermod_v2_skystrip/util.cpp (2)
hsv2rgb(8-34)hsv2rgb(8-8)
usermods/usermod_v2_skystrip/temperature_view.cpp (3)
usermods/usermod_v2_skystrip/util.h (6)
clamp01(85-87)lerp(90-90)current_offset(67-73)estimateTempAt(126-129)estimateDewPtAt(130-133)blinkDebug(216-223)usermods/usermod_v2_skystrip/temperature_view.h (2)
TemperatureView(9-32)view(15-26)usermods/usermod_v2_skystrip/util.cpp (2)
hsv2rgb(8-34)hsv2rgb(8-8)
usermods/usermod_v2_skystrip/util.h (1)
usermods/usermod_v2_skystrip/util.cpp (2)
hsv2rgb(8-34)hsv2rgb(8-8)
usermods/usermod_v2_skystrip/wind_view.h (4)
usermods/usermod_v2_skystrip/skymodel.cpp (3)
SkyModel(46-55)invalidate_history(81-92)invalidate_history(81-81)usermods/usermod_v2_skystrip/skymodel.h (1)
SkyModel(16-47)usermods/usermod_v2_skystrip/interfaces.h (1)
std(50-57)usermods/usermod_v2_skystrip/usermod_v2_skystrip.cpp (4)
addToConfig(135-155)addToConfig(135-135)readFromConfig(199-238)readFromConfig(199-199)
🔇 Additional comments (3)
usermods/usermod_v2_skystrip/util.h (1)
75-82: DST caveat in fmt_local (uses current offset for any timestamp)If you intend historical DST correctness, prefer localtime_r(utc_ts) over adding current_offset and gmtime_r; otherwise leave as‑is.
usermods/usermod_v2_skystrip/temperature_view.cpp (1)
92-99: Confirm need for beginDraw()/endDraw() pairingYou call seg.beginDraw() but never seg.endDraw(). If this WLED version expects a pair, add endDraw() after the loop; if not, ignore.
usermods/usermod_v2_skystrip/test_pattern_view.cpp (1)
33-83: Parser robustness looks goodNice tolerant HSV parser with clamping and wrapping; fits constrained UI inputs.
| class SegmentFreezeHandle { | ||
| public: | ||
| SegmentFreezeHandle() = default; | ||
| ~SegmentFreezeHandle() { release(); } | ||
|
|
||
| Segment *acquire(int16_t segId) { | ||
| if (segId < 0) { | ||
| release(); | ||
| return nullptr; | ||
| } | ||
| uint8_t maxSeg = strip.getMaxSegments(); | ||
| if (segId >= maxSeg) { | ||
| release(); | ||
| return nullptr; | ||
| } | ||
| Segment &seg = strip.getSegment((uint8_t)segId); | ||
| if (active_ && heldId_ == segId) { | ||
| seg_ = &seg; | ||
| if (!seg.freeze) { | ||
| seg.freeze = true; | ||
| } | ||
| return seg_; | ||
| } | ||
|
|
||
| release(); | ||
|
|
||
| prevFreeze_ = seg.freeze; | ||
| seg.freeze = true; | ||
| active_ = true; | ||
| heldId_ = segId; | ||
| seg_ = &seg; | ||
| return seg_; | ||
| } | ||
|
|
||
| void release() { | ||
| if (!active_) return; | ||
| if (seg_) seg_->freeze = prevFreeze_; | ||
| seg_ = nullptr; | ||
| heldId_ = -1; | ||
| active_ = false; | ||
| } | ||
|
|
||
| private: | ||
| bool active_ = false; | ||
| int16_t heldId_ = -1; | ||
| bool prevFreeze_ = false; | ||
| Segment *seg_ = nullptr; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make SegmentFreezeHandle non‑copyable and movable (RAII safety)
Prevents accidental copies leading to mismatched freeze ownership. Provide move semantics for efficient member placement.
class SegmentFreezeHandle {
public:
SegmentFreezeHandle() = default;
~SegmentFreezeHandle() { release(); }
+
+ // non-copyable
+ SegmentFreezeHandle(const SegmentFreezeHandle&) = delete;
+ SegmentFreezeHandle& operator=(const SegmentFreezeHandle&) = delete;
+ // movable
+ SegmentFreezeHandle(SegmentFreezeHandle&& other) noexcept { *this = std::move(other); }
+ SegmentFreezeHandle& operator=(SegmentFreezeHandle&& other) noexcept {
+ if (this == &other) return *this;
+ release();
+ active_ = other.active_;
+ heldId_ = other.heldId_;
+ prevFreeze_ = other.prevFreeze_;
+ seg_ = other.seg_;
+ other.active_ = false;
+ other.heldId_ = -1;
+ other.seg_ = nullptr;
+ return *this;
+ }📝 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.
| class SegmentFreezeHandle { | |
| public: | |
| SegmentFreezeHandle() = default; | |
| ~SegmentFreezeHandle() { release(); } | |
| Segment *acquire(int16_t segId) { | |
| if (segId < 0) { | |
| release(); | |
| return nullptr; | |
| } | |
| uint8_t maxSeg = strip.getMaxSegments(); | |
| if (segId >= maxSeg) { | |
| release(); | |
| return nullptr; | |
| } | |
| Segment &seg = strip.getSegment((uint8_t)segId); | |
| if (active_ && heldId_ == segId) { | |
| seg_ = &seg; | |
| if (!seg.freeze) { | |
| seg.freeze = true; | |
| } | |
| return seg_; | |
| } | |
| release(); | |
| prevFreeze_ = seg.freeze; | |
| seg.freeze = true; | |
| active_ = true; | |
| heldId_ = segId; | |
| seg_ = &seg; | |
| return seg_; | |
| } | |
| void release() { | |
| if (!active_) return; | |
| if (seg_) seg_->freeze = prevFreeze_; | |
| seg_ = nullptr; | |
| heldId_ = -1; | |
| active_ = false; | |
| } | |
| private: | |
| bool active_ = false; | |
| int16_t heldId_ = -1; | |
| bool prevFreeze_ = false; | |
| Segment *seg_ = nullptr; | |
| }; | |
| class SegmentFreezeHandle { | |
| public: | |
| SegmentFreezeHandle() = default; | |
| ~SegmentFreezeHandle() { release(); } | |
| // non-copyable | |
| SegmentFreezeHandle(const SegmentFreezeHandle&) = delete; | |
| SegmentFreezeHandle& operator=(const SegmentFreezeHandle&) = delete; | |
| // movable | |
| SegmentFreezeHandle(SegmentFreezeHandle&& other) noexcept { *this = std::move(other); } | |
| SegmentFreezeHandle& operator=(SegmentFreezeHandle&& other) noexcept { | |
| if (this == &other) return *this; | |
| release(); | |
| active_ = other.active_; | |
| heldId_ = other.heldId_; | |
| prevFreeze_ = other.prevFreeze_; | |
| seg_ = other.seg_; | |
| other.active_ = false; | |
| other.heldId_ = -1; | |
| other.seg_ = nullptr; | |
| return *this; | |
| } | |
| Segment *acquire(int16_t segId) { | |
| if (segId < 0) { | |
| release(); | |
| return nullptr; | |
| } | |
| uint8_t maxSeg = strip.getMaxSegments(); | |
| if (segId >= maxSeg) { | |
| release(); | |
| return nullptr; | |
| } | |
| Segment &seg = strip.getSegment((uint8_t)segId); | |
| if (active_ && heldId_ == segId) { | |
| seg_ = &seg; | |
| if (!seg.freeze) { | |
| seg.freeze = true; | |
| } | |
| return seg_; | |
| } | |
| release(); | |
| prevFreeze_ = seg.freeze; | |
| seg.freeze = true; | |
| active_ = true; | |
| heldId_ = segId; | |
| seg_ = &seg; | |
| return seg_; | |
| } | |
| void release() { | |
| if (!active_) return; | |
| if (seg_) seg_->freeze = prevFreeze_; | |
| seg_ = nullptr; | |
| heldId_ = -1; | |
| active_ = false; | |
| } | |
| private: | |
| bool active_ = false; | |
| int16_t heldId_ = -1; | |
| bool prevFreeze_ = false; | |
| Segment *seg_ = nullptr; | |
| }; |
|
As this usermod requires a specific arrangement of your LEDs and feels quite niche, I'm not sure it would have a broad enough appeal to be included in the main WLED repo |
|
This user-module doesn't require a specific arrangement at all, it currently has 4 different weather views which can be individually selected and arranged in any way desired. For example, a single strip temperature view or single strip wind view would be reasonable configurations. |
This usermod displays the weather forecast on several parallel LED strips.
It currently includes Cloud, Wind, Temperature, 24 Hour Delta, and TestPattern views.
Summary by CodeRabbit
New Features
Documentation
Chores