Usermod: Add DepartStrip transit departure usermod #4919
Conversation
WalkthroughImplements a DepartStrip usermod: a DepartModel with board merging and color map, SiriSource HTTP+JSON ingestion and RFC3339 parsing, DepartureView LED rendering and config persistence, utility helpers (time/HSV/natural sort), interfaces, README, and manifest. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: 11
🧹 Nitpick comments (25)
usermods/usermod_v2_departstrip/library.json (1)
1-4: Add basic metadata for discoverability.Consider adding
version,description, andlicenseto help tooling and readers.{ - "name": "usermod_v2_departstrip", - "build": { "libArchive": false } + "name": "usermod_v2_departstrip", + "version": "0.1.0", + "description": "Render upcoming transit departures (SIRI StopMonitoring) on WLED segments.", + "license": "MIT", + "build": { "libArchive": false } }usermods/usermod_v2_departstrip/readme.md (3)
51-60: Call out query-string API key exposure and redaction in logs.Recommend noting that keys in query params may appear in proxies/logs and advising users to treat configs as secrets; also mention that module logs redact
{apikey}.If placeholders are present they are replaced directly. If none are present, the provided URL is used as‑is (no legacy query parameters are appended). + +Security note: When using `{apikey}` in the query string, your key may be visible +to intermediaries or logs. Prefer HTTPS and avoid sharing config exports. The +module redacts keys from debug logs.
44-45: Clarify the “-1 disables view” contract.If code treats
-1specially, mention that non-negative values must be valid segment indices; otherwise it falls back to main/selected segment per WLED defaults.Do you want me to scan the repo to confirm
SegmentId == -1is handled safely throughout the usermod?
70-72: Specify where ColorMap is configured.“at the bottom” is ambiguous. Link to the exact config section/key name so users can find it quickly.
-- Colors: The colors for each line in an agencies feed may be set in - the ColorMap section at the bottom. +- Colors: Per-line colors can be set under `DepartStrip → ColorMap` in the + usermod config UI (keyed by `AGENCY:LineRef`).usermods/usermod_v2_departstrip/util.cpp (2)
6-24: Clamp and round components to avoid overflow/underflow and banding.Inputs aren’t bounded; casting truncates. Clamp h/s/v to expected ranges and round to nearest byte.
-uint32_t hsv2rgb(float h, float s, float v) { - if (s <= 0.f) return RGBW32(v * 255, v * 255, v * 255, 0); - h = fmodf(h, 1.f); if (h < 0.f) h += 1.f; +uint32_t hsv2rgb(float h, float s, float v) { + // normalize inputs + if (!isfinite(h) || !isfinite(s) || !isfinite(v)) return RGBW32(0,0,0,0); + h = fmodf(h, 1.f); if (h < 0.f) h += 1.f; + s = fminf(fmaxf(s, 0.f), 1.f); + v = fminf(fmaxf(v, 0.f), 1.f); + if (s <= 0.f) { + uint8_t g = (uint8_t)lrintf(v * 255.f); + return RGBW32(g, g, g, 0); + } float i = floorf(h * 6.f); float f = h * 6.f - i; float p = v * (1.f - s); float q = v * (1.f - s * f); float t = v * (1.f - s * (1.f - f)); float r=0,g=0,b=0; switch(((int)i) % 6) { case 0: r=v,g=t,b=p; break; case 1: r=q,g=v,b=p; break; case 2: r=p,g=v,b=t; break; case 3: r=p,g=q,b=v; break; case 4: r=t,g=p,b=v; break; case 5: r=v,g=p,b=q; break; } - return RGBW32((uint8_t)(r*255),(uint8_t)(g*255),(uint8_t)(b*255),0); + return RGBW32((uint8_t)lrintf(r*255.f),(uint8_t)lrintf(g*255.f),(uint8_t)lrintf(b*255.f),0); }
6-24: Replace local hsv2rgb with existing WLED utility
usermods/usermod_v2_departstrip/util.cpp (L6) defines a float‐based hsv2rgb that duplicates the one in wled00/colors.h (void hsv2rgb(const CHSV32&, uint32_t&)). Convert yourh,s,vfloats into aCHSV32(e.g.hue = h*0xFFFF,sat = s*255,val = v*255), include<wled/colors.h>, and call the existinghsv2rgbto eliminate duplication and reduce binary size.usermods/usermod_v2_departstrip/util.h (4)
11-20: Mark RAII destructor noexcept and make intent explicit.Small safety/readability win.
struct FreezeGuard { Segment &seg; bool prev; explicit FreezeGuard(Segment &s, bool freezeNow = true) : seg(s), prev(s.freeze) { seg.freeze = freezeNow; } - ~FreezeGuard() { seg.freeze = prev; } + ~FreezeGuard() noexcept { seg.freeze = prev; } FreezeGuard(const FreezeGuard &) = delete; FreezeGuard &operator=(const FreezeGuard &) = delete; };
25-29: Extract and document the max local-time offset constant.Improves clarity and reuse.
-inline long current_offset() { +static constexpr long kMaxOffsetSec = 15L * 3600L; // +/-15h safety clamp +inline long current_offset() { long off = (long)localTime - (long)toki.getTime().sec; - if (off < -54000 || off > 54000) off = 0; + if (off < -kMaxOffsetSec || off > kMaxOffsetSec) off = 0; return off; }
31-37: Handle unsynced time and document expectations.If
toki/localTimeare unsynced (0),fmt_localformats epoch. Consider a guard or note.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(); + const time_t off = current_offset(); + const time_t local_sec = utc_ts + off; struct tm tmLocal; gmtime_r(&local_sec, &tmLocal); strftime(out, n, fmt, &tmLocal); }Add a brief comment stating that if time isn’t synced, epoch-based formatting is expected.
22-24:toki.getTime()is always safe –tokiis declared asWLED_GLOBAL Toki tokiin wled.h and constructed before any usermod runs; itssecfield will be zero until NTP/RTC sync, so guard againsttoki.getTime().sec == 0if you require a valid timestamp.usermods/usermod_v2_departstrip/departure_view.cpp (4)
54-57: Reduce heap churn in render loop (reuse vectors, reserve once).Per-pixel construction of std::vector (and strong) allocates/frees frequently on a constrained MCU. Hoist vectors outside the i-loop and reserve capacity once.
Apply this diff:
- for (int i = 0; i < len; ++i) { - struct Cand { uint32_t color; uint16_t bsum; String key; uint8_t alpha; }; - std::vector<Cand> cands; + struct Cand { uint32_t color; uint16_t bsum; String key; uint8_t alpha; }; + size_t estCap = 0; + for (const auto& src : sources) estCap += src.batch->items.size(); + std::vector<Cand> cands; cands.reserve(estCap); + std::vector<Cand> strong; strong.reserve(8); + + for (int i = 0; i < len; ++i) { + cands.clear(); + strong.clear(); @@ - std::vector<Cand> strong; strong.reserve(cands.size()); - for (const auto& c : cands) if (c.alpha >= CYCLE_ALPHA) strong.push_back(c); + for (const auto& c : cands) if (c.alpha >= CYCLE_ALPHA) strong.push_back(c);Also applies to: 78-99
60-68: Minor: guard index math explicitly.Not strictly necessary, but makes intent clear and avoids accidental idx+1 overflow changes later.
Apply this diff:
- if (i == idx) alpha = uint8_t((1.0f - frac) * 255); - else if (i == idx + 1) alpha = uint8_t(frac * 255); + if (i == idx) alpha = uint8_t((1.0f - frac) * 255); + else if (idx + 1 < len && i == idx + 1) alpha = uint8_t(frac * 255);
1-6: Include for floor().Some cores require /<math.h> for floor(). Avoid relying on transitive includes.
Apply this diff:
#include <vector> #include <algorithm> +#include <cmath>
105-127: Config UI snippet works; consider escaping configKey defensively.configKey is derived from user-provided keys; although sanitized, escaping quotes would be safer for JS single-quoted context.
If you adopt the sanitize patch in header (see comment on departure_view.h), this is already mitigated. Otherwise, ensure configKey() contains no quotes/backslashes.
usermods/usermod_v2_departstrip/departure_view.h (2)
17-52: DRY the key-parsing logic and harden configKey sanitization.Parsing is duplicated here and in readFromConfig; move it to a helper. Also sanitize quotes/backslashes to keep JS addInfo('...') safe.
Apply this diff:
explicit DepartureView(const String& keys) : keysStr_(keys), segmentId_(-1), configKey_() { - // Parse now so view() has a ready list - auto parseKeys = [&](const String& in) { - keys_.clear(); - // Split by comma or whitespace - String agencyHint; - String token; - auto flush = [&]() { - token.trim(); - if (!token.length()) return; - int c = token.indexOf(':'); - if (c > 0) { - keys_.push_back(token); - if (agencyHint.length() == 0) agencyHint = token.substring(0, c); - } else { - if (agencyHint.length() > 0) { - String k = agencyHint; k += ':'; k += token; keys_.push_back(k); - } else { - // As a fallback, keep token as-is (may be full key already) - keys_.push_back(token); - } - } - token = String(); - }; - for (unsigned i = 0; i < in.length(); ++i) { - char ch = in.charAt(i); - if (ch == ',' || ch == ' ' || ch == '\t' || ch == '\n' || ch == ';') flush(); - else token += ch; - } - flush(); - }; - parseKeys(keysStr_); - String s = String("DepartureView_") + keysStr_; - s.replace(':', '_'); s.replace(',', '_'); s.replace(' ', '_'); s.replace(';', '_'); + parseKeysFrom(keysStr_); + String s = String("DepartureView_") + keysStr_; + s.replace(':', '_'); s.replace(',', '_'); s.replace(' ', '_'); s.replace(';', '_'); + s.replace('\'', '_'); s.replace('\"', '_'); s.replace('\\', '_'); configKey_ = std::string(s.c_str()); }And here’s the helper to add (inside the class’ private section or as a private method):
private: void parseKeysFrom(const String& in) { keys_.clear(); String agencyHint, token; auto flush = [&]() { token.trim(); if (!token.length()) return; int c = token.indexOf(':'); if (c > 0) { keys_.push_back(token); if (!agencyHint.length()) agencyHint = token.substring(0, c); } else { if (agencyHint.length() > 0) { String k = agencyHint; k += ':'; k += token; keys_.push_back(k); } else { keys_.push_back(token); } } token = String(); }; for (unsigned i = 0; i < in.length(); ++i) { char ch = in.charAt(i); if (ch == ',' || ch == ' ' || ch == '\t' || ch == '\n' || ch == ';') flush(); else token += ch; } flush(); }
62-106: Reuse the new parser in readFromConfig and sanitize configKey likewise.Simplifies maintenance and ensures consistent behavior.
Apply this diff:
if (keysStr_ != prev) { - // Re-parse and update config key - // Parse keys - keys_.clear(); - { - String in = keysStr_; - String agencyHint; - String token; - auto flush = [&]() { - token.trim(); - if (!token.length()) return; - int c = token.indexOf(':'); - if (c > 0) { - keys_.push_back(token); - if (agencyHint.length() == 0) agencyHint = token.substring(0, c); - } else { - if (agencyHint.length() > 0) { String k = agencyHint; k += ':'; k += token; keys_.push_back(k); } - else keys_.push_back(token); - } - token = String(); - }; - for (unsigned i = 0; i < in.length(); ++i) { - char ch = in.charAt(i); - if (ch == ',' || ch == ' ' || ch == '\t' || ch == '\n' || ch == ';') flush(); - else token += ch; - } - flush(); - } + // Re-parse and update config key + parseKeysFrom(keysStr_); String s = String("DepartureView_") + keysStr_; - s.replace(':', '_'); s.replace(',', '_'); s.replace(' ', '_'); s.replace(';', '_'); + s.replace(':', '_'); s.replace(',', '_'); s.replace(' ', '_'); s.replace(';', '_'); + s.replace('\'', '_'); s.replace('\"', '_'); s.replace('\\', '_'); configKey_ = std::string(s.c_str()); }usermods/usermod_v2_departstrip/siri_source.h (1)
41-44: Optional: populate appendConfigData with source status.appendConfigData() is empty; consider emitting last fetch time, backoff multiplier, and stop name to aid users.
usermods/usermod_v2_departstrip/depart_model.h (1)
36-46: De-duplicate route-sorting helpers across files.parseLeadingInt/cmp logic exists in both depart_model.cpp and usermod_v2_departstrip.cpp. Centralize in a shared helper to avoid divergence.
usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp (3)
94-96: Remove unused locals.rebuiltSourcesUsed and rebuiltViewsUsed are declared but never used.
- bool rebuiltSourcesUsed = false; - bool rebuiltViewsUsed = false;
206-210: Escape ampersands in stop names to avoid HTML issues.Stop names containing '&' will render incorrectly; escape it before other substitutions.
- // Escape for HTML and JS single-quoted string context - nm.replace("<","<"); nm.replace(">",">"); + // Escape for HTML and JS single-quoted string context + nm.replace("&","&"); + nm.replace("<","<"); nm.replace(">",">"); nm.replace("\\","\\\\"); nm.replace("'","\\'");
351-359: Defensive config read: handle missing sub-objects.When reading per-source/view sections, sub may be null if configKey changed; check isNull() before readFromConfig to avoid dereferencing null JSON and silently resetting defaults.
- JsonObject sub = top[src->configKey()]; - ok &= src->readFromConfig(sub, startup_complete, invalidate_history); + JsonObject sub = top[src->configKey()]; + if (!sub.isNull()) ok &= src->readFromConfig(sub, startup_complete, invalidate_history);Apply similarly for views.
usermods/usermod_v2_departstrip/depart_model.cpp (2)
31-57: Avoid duplicating line-sort logic; reuse existing helpers.CmpLine::parseLeadingInt()/lt() duplicates the same logic already present in usermod_v2_departstrip.cpp (cmpLine/parseLeadingInt). Consider centralizing this in a shared helper to keep sort semantics consistent and reduce maintenance.
189-212: Broaden color parsing to support #RGB shorthand.Small UX win: accept 3‑digit hex form (#RGB) in addition to #RRGGBB.
Apply:
bool DepartModel::parseColorString(const String& s, uint32_t& rgbOut) { - if (s.length() >= 7 && s[0] == '#') { + if (s.length() >= 7 && s[0] == '#') { // #RRGGBB char buf[3] = {0,0,0}; long r=0,g=0,b=0; buf[0]=s[1]; buf[1]=s[2]; r = strtol(buf,nullptr,16); buf[0]=s[3]; buf[1]=s[4]; g = strtol(buf,nullptr,16); buf[0]=s[5]; buf[1]=s[6]; b = strtol(buf,nullptr,16); rgbOut = ((uint32_t)r<<16)|((uint32_t)g<<8)|(uint32_t)b; return true; } + if (s.length() == 4 && s[0] == '#') { + // #RGB -> expand to #RRGGBB + int r = strtol(String("") + s[1] + s[1], nullptr, 16); + int g = strtol(String("") + s[2] + s[2], nullptr, 16); + int b = strtol(String("") + s[3] + s[3], nullptr, 16); + rgbOut = ((uint32_t)r<<16)|((uint32_t)g<<8)|(uint32_t)b; + return true; + }usermods/usermod_v2_departstrip/usermod_v2_departstrip.h (1)
39-41: Make accessor const-correct.isEnabled() should be const to enable calls on const instances.
Apply:
- inline bool isEnabled() { return enabled_; } + inline bool isEnabled() const { return enabled_; }usermods/usermod_v2_departstrip/siri_source.cpp (1)
120-160: Consider enabling gzip when servers ignore Accept-Encoding: identity.Some endpoints still gzip responses. If you hit parse failures on real feeds, add optional gzip support (HTTPClient handles it when not forced to identity). Keep identity by default to lower peak RAM.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
usermods/usermod_v2_departstrip/depart_model.cpp(1 hunks)usermods/usermod_v2_departstrip/depart_model.h(1 hunks)usermods/usermod_v2_departstrip/departure_view.cpp(1 hunks)usermods/usermod_v2_departstrip/departure_view.h(1 hunks)usermods/usermod_v2_departstrip/interfaces.h(1 hunks)usermods/usermod_v2_departstrip/library.json(1 hunks)usermods/usermod_v2_departstrip/readme.md(1 hunks)usermods/usermod_v2_departstrip/siri_source.cpp(1 hunks)usermods/usermod_v2_departstrip/siri_source.h(1 hunks)usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp(1 hunks)usermods/usermod_v2_departstrip/usermod_v2_departstrip.h(1 hunks)usermods/usermod_v2_departstrip/util.cpp(1 hunks)usermods/usermod_v2_departstrip/util.h(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-04-24T09:31:06.879Z
Learnt from: blazoncek
PR: wled/WLED#4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Applied to files:
usermods/usermod_v2_departstrip/util.h
📚 Learning: 2025-04-24T09:31:06.879Z
Learnt from: blazoncek
PR: wled/WLED#4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs by falling back to the main segment ID, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Applied to files:
usermods/usermod_v2_departstrip/util.h
📚 Learning: 2025-09-02T19:42:57.475Z
Learnt from: freakintoddles2
PR: wled/WLED#4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Applied to files:
usermods/usermod_v2_departstrip/util.h
🧬 Code graph analysis (11)
usermods/usermod_v2_departstrip/util.cpp (1)
usermods/usermod_v2_departstrip/util.h (1)
s(11-17)
usermods/usermod_v2_departstrip/interfaces.h (4)
usermods/usermod_v2_departstrip/siri_source.cpp (4)
fetch(328-401)fetch(328-328)reload(67-70)reload(67-67)usermods/usermod_v2_departstrip/departure_view.cpp (4)
view(16-103)view(16-16)appendConfigData(105-127)appendConfigData(105-105)usermods/usermod_v2_departstrip/departure_view.h (1)
appendConfigData(60-60)usermods/usermod_v2_departstrip/siri_source.h (1)
appendConfigData(41-41)
usermods/usermod_v2_departstrip/siri_source.h (2)
usermods/usermod_v2_departstrip/siri_source.cpp (24)
SiriSource(60-65)fetch(328-401)fetch(328-328)reload(67-70)reload(67-67)addToConfig(403-411)addToConfig(403-403)readFromConfig(413-446)readFromConfig(413-413)composeUrl(72-87)composeUrl(72-72)parseRFC3339ToUTC(100-118)parseRFC3339ToUTC(100-100)httpBegin(120-160)httpBegin(120-120)parseJsonFromHttp(162-231)parseJsonFromHttp(162-162)doc(365-365)computeJsonCapacity(233-238)computeJsonCapacity(233-233)getSiriRoot(240-251)getSiriRoot(240-240)buildModelFromSiri(253-326)buildModelFromSiri(253-253)usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp (10)
key(131-135)key(131-131)appendConfigData(177-230)appendConfigData(177-177)s(136-144)s(136-136)addToConfig(74-175)addToConfig(74-74)readFromConfig(232-408)readFromConfig(232-232)
usermods/usermod_v2_departstrip/siri_source.cpp (4)
usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp (12)
s(136-144)s(136-136)key(131-135)key(131-131)a(145-161)a(145-145)a(162-167)a(162-162)addToConfig(74-175)addToConfig(74-74)readFromConfig(232-408)readFromConfig(232-232)usermods/usermod_v2_departstrip/util.h (3)
s(11-17)time_now(23-23)time_now_utc(22-22)usermods/usermod_v2_departstrip/siri_source.h (1)
SiriSource(17-43)usermods/usermod_v2_departstrip/departure_view.h (2)
addToConfig(56-59)readFromConfig(62-109)
usermods/usermod_v2_departstrip/depart_model.h (2)
usermods/usermod_v2_departstrip/depart_model.cpp (2)
s(46-48)s(46-46)usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp (4)
s(136-144)s(136-136)key(131-135)key(131-131)
usermods/usermod_v2_departstrip/departure_view.h (4)
usermods/usermod_v2_departstrip/siri_source.cpp (5)
in(45-45)addToConfig(403-411)addToConfig(403-403)readFromConfig(413-446)readFromConfig(413-413)usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp (8)
s(136-144)s(136-136)addToConfig(74-175)addToConfig(74-74)appendConfigData(177-230)appendConfigData(177-177)readFromConfig(232-408)readFromConfig(232-232)usermods/usermod_v2_departstrip/util.h (1)
s(11-17)usermods/usermod_v2_departstrip/departure_view.cpp (4)
view(16-103)view(16-16)appendConfigData(105-127)appendConfigData(105-105)
usermods/usermod_v2_departstrip/departure_view.cpp (3)
usermods/usermod_v2_departstrip/depart_model.cpp (6)
getColorRGB(164-187)getColorRGB(164-164)a(49-54)a(49-49)s(46-48)s(46-46)usermods/usermod_v2_departstrip/departure_view.h (1)
appendConfigData(60-60)usermods/usermod_v2_departstrip/util.h (1)
s(11-17)
usermods/usermod_v2_departstrip/util.h (1)
usermods/usermod_v2_departstrip/util.cpp (2)
hsv2rgb(6-24)hsv2rgb(6-6)
usermods/usermod_v2_departstrip/depart_model.cpp (1)
usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp (8)
key(131-135)key(131-131)s(136-144)s(136-136)a(145-161)a(145-145)a(162-167)a(162-162)
usermods/usermod_v2_departstrip/usermod_v2_departstrip.h (7)
usermods/usermod_v2_departstrip/siri_source.cpp (5)
SiriSource(60-65)addToConfig(403-411)addToConfig(403-403)readFromConfig(413-446)readFromConfig(413-413)usermods/usermod_v2_departstrip/siri_source.h (2)
SiriSource(17-43)appendConfigData(41-41)usermods/usermod_v2_departstrip/departure_view.h (4)
DepartureView(9-112)addToConfig(56-59)appendConfigData(60-60)readFromConfig(62-109)usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp (21)
DepartStrip(23-27)setup(29-35)setup(29-29)loop(37-64)loop(37-37)handleOverlayDraw(66-72)handleOverlayDraw(66-66)addToConfig(74-175)addToConfig(74-74)appendConfigData(177-230)appendConfigData(177-177)s(136-144)s(136-136)readFromConfig(232-408)readFromConfig(232-232)showBooting(410-418)showBooting(410-410)doneBooting(420-423)doneBooting(420-420)reloadSources(425-427)reloadSources(425-425)usermods/usermod_v2_departstrip/departure_view.cpp (2)
appendConfigData(105-127)appendConfigData(105-105)usermods/usermod_v2_departstrip/depart_model.cpp (2)
s(46-48)s(46-46)usermods/usermod_v2_departstrip/util.h (1)
s(11-17)
usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp (7)
usermods/usermod_v2_departstrip/usermod_v2_departstrip.h (2)
DepartStrip(15-46)setup(31-37)usermods/usermod_v2_departstrip/util.h (2)
time_now_utc(22-22)s(11-17)usermods/usermod_v2_departstrip/departure_view.cpp (4)
view(16-103)view(16-16)appendConfigData(105-127)appendConfigData(105-105)usermods/usermod_v2_departstrip/departure_view.h (3)
addToConfig(56-59)appendConfigData(60-60)readFromConfig(62-109)usermods/usermod_v2_departstrip/siri_source.cpp (4)
addToConfig(403-411)addToConfig(403-403)readFromConfig(413-446)readFromConfig(413-413)usermods/usermod_v2_departstrip/depart_model.cpp (18)
a(49-54)a(49-49)colorMap(131-133)colorMap(131-131)s(46-48)s(46-46)colorToString(214-218)colorToString(214-214)getColorRGB(164-187)getColorRGB(164-164)clearColorMap(135-137)clearColorMap(135-135)removeColorKey(146-149)removeColorKey(146-146)parseColorString(189-212)parseColorString(189-189)setColorRGB(151-157)setColorRGB(151-151)usermods/usermod_v2_departstrip/siri_source.h (1)
appendConfigData(41-41)
🔇 Additional comments (6)
usermods/usermod_v2_departstrip/library.json (1)
1-4: Manifest is minimal and correct for a usermod.
libArchive: falsemakes sense here.usermods/usermod_v2_departstrip/departure_view.cpp (2)
9-14: Color resolution path looks good.Delegating to DepartModel::getColorRGB captures defaults and agency-specific fallbacks.
45-47: Ignore endDraw pairing suggestion.
TheSegmentAPI does not provide anendDraw()method; no explicit pairing is required.Likely an incorrect or invalid review comment.
usermods/usermod_v2_departstrip/interfaces.h (1)
10-18: Interface surface LGTM.Clear config and view/data contracts; default appendConfigData passthrough is sensible.
usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp (1)
49-56: Source reload on enable/on transitions is good.Reloading sources when turning on or enabling is the right trade-off with the safety delay.
usermods/usermod_v2_departstrip/usermod_v2_departstrip.h (1)
9-9: Confirm usermod ID uniqueness.USERMOD_ID_DEPARTSTRIP 561: please verify no collision with existing/assigned IDs in main to avoid config clashes.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
usermods/usermod_v2_departstrip/siri_source.cpp (1)
247-252: Cap ArduinoJson capacity to avoid oversized allocations on large Content-Length.contentLen*2 can request hundreds of KB and crash/fragment heap on ESP. Add a ceiling.
size_t SiriSource::computeJsonCapacity(int contentLen) { - if (contentLen > 0) return (size_t)contentLen * 2; + if (contentLen > 0) { + size_t cap = (size_t)contentLen * 2; + if (cap > 32768) cap = 32768; // 32 KB ceiling + return cap; + } // Unknown length: pick a slightly larger default to reduce NoMemory risk // while the filter keeps actual usage low on most feeds. return 20480; // 20 KB }
🧹 Nitpick comments (4)
usermods/usermod_v2_departstrip/siri_source.cpp (4)
9-55: BOM-skipping stream looks solid; minor override nit.Implementation is correct and safe for short/empty streams. Add an explicit override on write(const uint8_t*, size_t) for compile-time checking.
Apply:
- size_t write(const uint8_t*, size_t) { return 0; } + size_t write(const uint8_t*, size_t) override { return 0; }
72-87: URL templating: add STOPCODE variant (nit).Some templates may use {STOPCODE}. Consider handling it for completeness.
url.replace(F("{stopcode}"), stopCode); url.replace(F("{stopCode}"), stopCode); + url.replace(F("{STOPCODE}"), stopCode);
148-150: Collect common alt header too (nit).Many providers use X-RateLimit-Remaining. Add it to collectHeaders.
- static const char* hdrs[] = {"Content-Type", "Content-Encoding", "Content-Length", "RateLimit-Remaining"}; - http_.collectHeaders(hdrs, 4); + static const char* hdrs[] = {"Content-Type", "Content-Encoding", "Content-Length", "RateLimit-Remaining", "X-RateLimit-Remaining"}; + http_.collectHeaders(hdrs, 5);
311-319: Skip stale departures to reduce noise (optional).Filtering items already in the past (e.g., older than now-60s) avoids showing expired visits.
- if (tstr && parseRFC3339ToUTC(tstr, depUtc)) { + if (tstr && parseRFC3339ToUTC(tstr, depUtc)) { + if (depUtc + 60 < now) continue; // drop stale items (grace 60s) ++parsedTime; DepartModel::Entry::Item item; item.estDep = depUtc;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
usermods/usermod_v2_departstrip/departure_view.cpp(1 hunks)usermods/usermod_v2_departstrip/siri_source.cpp(1 hunks)usermods/usermod_v2_departstrip/siri_source.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- usermods/usermod_v2_departstrip/siri_source.h
- usermods/usermod_v2_departstrip/departure_view.cpp
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ksedgwic
PR: wled/WLED#4919
File: usermods/usermod_v2_departstrip/depart_model.cpp:164-187
Timestamp: 2025-09-09T23:14:06.285Z
Learning: In the DepartStrip usermod, it's intentionally valid to have different colors for different directions (e.g., "72M-N" vs "72M-S"), even though they may map to the same color values in the current implementation.
🧬 Code graph analysis (1)
usermods/usermod_v2_departstrip/siri_source.cpp (3)
usermods/usermod_v2_departstrip/util.h (3)
s(11-17)time_now(23-23)time_now_utc(22-22)usermods/usermod_v2_departstrip/siri_source.h (1)
SiriSource(17-43)usermods/usermod_v2_departstrip/departure_view.h (2)
addToConfig(56-59)readFromConfig(62-109)
🔇 Additional comments (6)
usermods/usermod_v2_departstrip/siri_source.cpp (6)
100-132: RFC3339 tz scan no longer matches the date hyphens. LGTM.Scanning from after the 'T' fixes the previous bug; ±HH:MM and ±HHMM are supported. Good.
176-245: JSON parse path and filters look good.Wide vs. narrow filters are well chosen; BOM-skipping wrapper is applied; http_.end() is correctly paired.
254-265: SIRI root extraction logic is robust.Fallback to top-level ServiceDelivery is appropriate.
417-425: Config emission looks consistent with reader.Enabled/UpdateSecs/TemplateUrl/ApiKey and Agency:StopCode key align with readFromConfig.
427-462: Config read path is coherent; min cadence guard is good.Combined key parsing and friendly section naming are clean. No action.
134-174: HTTPS not supported with explicit WiFiClient; API keys risk being sent in cleartext.http_.begin(client_, url) forces plain WiFiClient and will fail on https:// endpoints (or worse, entice use of http:// with keys). Switch to the scheme-aware overload so HTTPS is used automatically.
Apply:
- if (!http_.begin(client_, url)) { + if (!http_.begin(url)) { http_.end(); DEBUG_PRINTLN(F("DepartStrip: SiriSource::fetch: begin() failed")); return false; }Optionally, if you prefer explicit control, detect https and use WiFiClientSecure with setInsecure()/setCACert.
⛔ Skipped due to learnings
Learnt from: ksedgwic PR: wled/WLED#4883 File: usermods/usermod_v2_skystrip/rest_json_client.cpp:40-46 Timestamp: 2025-08-29T00:22:34.115Z 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.Learnt from: ksedgwic PR: wled/WLED#4919 File: usermods/usermod_v2_departstrip/readme.md:15-23 Timestamp: 2025-09-09T23:20:44.907Z Learning: For the DepartStrip usermod on ESP8266/ESP32 devices, HTTP is used instead of HTTPS for API endpoints due to heap memory constraints. HTTPS would require too much memory for the combined TLS handshake and JSON parsing operations on these embedded devices, making HTTP a necessary compromise despite security considerations.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
usermods/usermod_v2_departstrip/siri_source.cpp (1)
247-252: Cap JSON capacity to protect heap on small MCUs.Unbounded
contentLen*2can over-allocate and OOM devices.size_t SiriSource::computeJsonCapacity(int contentLen) { - if (contentLen > 0) return (size_t)contentLen * 2; + if (contentLen > 0) { + size_t cap = (size_t)contentLen * 2; + if (cap > 32768) cap = 32768; // 32 KB ceiling + return cap; + } // Unknown length: pick a slightly larger default to reduce NoMemory risk // while the filter keeps actual usage low on most feeds. return 20480; // 20 KB }
🧹 Nitpick comments (5)
usermods/usermod_v2_departstrip/siri_source.cpp (5)
17-18: Make write() overrides explicit and return consumed byte counts.Clarifies intent and aligns with Print semantics; harmless since this stream is read-only.
- size_t write(uint8_t) override { return 0; } - size_t write(const uint8_t*, size_t) { return 0; } + size_t write(uint8_t) override { return 1; } + size_t write(const uint8_t* /*buf*/, size_t len) override { return len; }
58-58: Remove unused using-declaration.
time_nowisn’t referenced in this TU.-using departstrip::util::time_now;
146-156: Tighten headers and capture alt rate-limit header.Prefer JSON in Accept and also collect/log “X-RateLimit-Remaining”.
- http_.addHeader("Accept", "*/*", true, true); + http_.addHeader("Accept", "application/json", true, true); @@ - static const char* hdrs[] = {"Content-Type", "Content-Encoding", "Content-Length", "RateLimit-Remaining"}; - http_.collectHeaders(hdrs, 4); + static const char* hdrs[] = {"Content-Type", "Content-Encoding", "Content-Length", + "RateLimit-Remaining", "X-RateLimit-Remaining"}; + http_.collectHeaders(hdrs, 5); @@ - String rl = http_.header("RateLimit-Remaining"); + String rl = http_.header("RateLimit-Remaining"); if (rl.length() > 0) { DEBUG_PRINTF("DepartStrip: SiriSource::fetch: RateLimit-Remaining=%s\n", rl.c_str()); } + String xrl = http_.header("X-RateLimit-Remaining"); + if (xrl.length() > 0) { + DEBUG_PRINTF("DepartStrip: SiriSource::fetch: X-RateLimit-Remaining=%s\n", xrl.c_str()); + }Also applies to: 148-150, 153-158
176-235: Avoid large stack spikes from filter documents.The 4 KB narrow filter lives on the stack; make both filters static and clear per-call.
- StaticJsonDocument<768> filter; + static StaticJsonDocument<768> filter; + filter.clear(); @@ - StaticJsonDocument<4096> filter; + static StaticJsonDocument<4096> filter; + filter.clear();
295-323: Guard against pathological visit counts.Cap items to avoid large vectors if a feed bursts data.
for (JsonObject v : visits) { ++totalVisits; + if (batch.items.size() >= 64) break; // soft cap
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/usermod_v2_departstrip/siri_source.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ksedgwic
PR: wled/WLED#4919
File: usermods/usermod_v2_departstrip/depart_model.cpp:164-187
Timestamp: 2025-09-09T23:14:06.285Z
Learning: In the DepartStrip usermod, it's intentionally valid to have different colors for different directions (e.g., "72M-N" vs "72M-S"), even though they may map to the same color values in the current implementation.
🧬 Code graph analysis (1)
usermods/usermod_v2_departstrip/siri_source.cpp (2)
usermods/usermod_v2_departstrip/util.h (3)
s(11-17)time_now(23-23)time_now_utc(22-22)usermods/usermod_v2_departstrip/siri_source.h (1)
SiriSource(17-43)
🔇 Additional comments (2)
usermods/usermod_v2_departstrip/siri_source.cpp (2)
100-132: RFC3339 tz parsing fix looks good.Scanning for the TZ designator after 'T' avoids the date hyphen trap; supports Z/±HH:MM and ±HHMM.
364-382: API key redaction in URL logging: LGTM.Covers common param names and preserves original URL for the request.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
usermods/usermod_v2_departstrip/departure_view.cpp (1)
96-109: Cycling fix LGTM; wrap/width hazards removed.Using size_t for n and clamping slice to >=1 eliminates the divide-by-zero/uint8_t wrap issue from earlier. Deterministic sort by key is also nice.
🧹 Nitpick comments (4)
usermods/usermod_v2_departstrip/departure_view.cpp (1)
60-82: Avoid O(len * items) inner scanning; pre-bin candidates by pixel.Current per-pixel scan over all items is quadratic in practice. Build bins once per frame (vector<vector> of size len), push each item into at most two bins (idx and idx+1), then select from bins[i]. This keeps work proportional to items + len.
Example sketch (frame-local prepass):
std::vector<std::vector<Cand>> bins(len); for (const auto& src : sources) { for (auto& e : src.batch->items) { float diff = float(boardingSecs_ + e.estDep - now) * len / 3600.0f; if (diff < 0.0f || diff >= len) continue; int idx = int(floor(diff)); float frac = diff - float(idx); auto emit = [&](int i, uint8_t a){ if (i < 0 || i >= len || a == 0) return; CRGB col = colorForAgencyLine(src.agency, e.lineRef); uint32_t colScaled = (((uint32_t)col.r * a / 255) << 16) | (((uint32_t)col.g * a / 255) << 8) | ((uint32_t)col.b * a / 255); bins[i].push_back(Cand{colScaled, brightness(colScaled), e.lineRef, a}); }; emit(idx, uint8_t((1.0f - frac) * 255)); emit(idx+1, uint8_t(frac * 255)); } } // then for (i) use bins[i] instead of scanning all itemsusermods/usermod_v2_departstrip/departure_view.h (2)
17-23: De-duplicate configKey generation.The configKey_ normalization logic is duplicated in ctor and readFromConfig(). Extract to a small helper to keep it consistent.
Apply:
explicit DepartureView(const String& keys) : keysStr_(keys), segmentId_(-1), configKey_() { - parseKeysFrom(keysStr_); - String s = String("DepartureView_") + keysStr_; - s.replace(':', '_'); s.replace(',', '_'); s.replace(' ', '_'); s.replace(';', '_'); - s.replace('\'', '_'); s.replace('\"', '_'); s.replace('\\', '_'); - configKey_ = std::string(s.c_str()); + parseKeysFrom(keysStr_); + updateConfigKey_(); } ... if (keysStr_ != prev) { // Re-parse and update config key parseKeysFrom(keysStr_); - String s = String("DepartureView_") + keysStr_; - s.replace(':', '_'); s.replace(',', '_'); s.replace(' ', '_'); s.replace(';', '_'); - s.replace('\'', '_'); s.replace('\"', '_'); s.replace('\\', '_'); - configKey_ = std::string(s.c_str()); + updateConfigKey_(); }And add privately:
void updateConfigKey_() { String s = String("DepartureView_") + keysStr_; s.replace(':', '_'); s.replace(',', '_'); s.replace(' ', '_'); s.replace(';', '_'); s.replace('\'', '_'); s.replace('\"', '_'); s.replace('\\', '_'); configKey_ = std::string(s.c_str()); }Also applies to: 46-53
61-86: First token without agency is ambiguous—document or normalize.If the first token lacks “AGENCY:”, keys_ stores a raw StopCode that won’t match model keys. Either (a) document that the first token must include AGENCY, or (b) reject such configs up front, or (c) optionally read a default agency from elsewhere.
Minimal guard:
void parseKeysFrom(const String& in) { keys_.clear(); String agencyHint, token; auto flush = [&]() { token.trim(); if (!token.length()) return; int c = token.indexOf(':'); if (c > 0) { keys_.push_back(token); if (!agencyHint.length()) agencyHint = token.substring(0, c); } else { - if (agencyHint.length() > 0) { + if (agencyHint.length() > 0) { String k = agencyHint; k += ':'; k += token; keys_.push_back(k); } else { - keys_.push_back(token); + // No agency hint yet; ignore ambiguous token to avoid silent mismatch + // (alternatively: keep but warn in appendConfigData) } } token = String(); };usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp (1)
307-335: Consolidate duplicate deletion patterns.The nearly identical erase-backwards patterns for sources and views can be factored into a small helper to reduce drift.
Sketch:
template <typename TVec, typename Pred> void erase_if_backward(TVec& v, Pred p) { std::vector<size_t> idx; for (size_t i = 0; i < v.size(); ++i) if (p(v[i], i)) idx.push_back(i); for (size_t k = 0; k < idx.size(); ++k) v.erase(v.begin() + (idx[idx.size()-1-k])); }Also applies to: 336-348
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
usermods/usermod_v2_departstrip/departure_view.cpp(1 hunks)usermods/usermod_v2_departstrip/departure_view.h(1 hunks)usermods/usermod_v2_departstrip/readme.md(1 hunks)usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp(1 hunks)usermods/usermod_v2_departstrip/usermod_v2_departstrip.h(1 hunks)usermods/usermod_v2_departstrip/util.h(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- usermods/usermod_v2_departstrip/readme.md
🚧 Files skipped from review as they are similar to previous changes (2)
- usermods/usermod_v2_departstrip/util.h
- usermods/usermod_v2_departstrip/usermod_v2_departstrip.h
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ksedgwic
PR: wled/WLED#4919
File: usermods/usermod_v2_departstrip/depart_model.cpp:164-187
Timestamp: 2025-09-09T23:14:06.285Z
Learning: In the DepartStrip usermod, it's intentionally valid to have different colors for different directions (e.g., "72M-N" vs "72M-S"), even though they may map to the same color values in the current implementation.
🧬 Code graph analysis (3)
usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp (6)
usermods/usermod_v2_departstrip/usermod_v2_departstrip.h (2)
DepartStrip(15-46)setup(31-37)usermods/usermod_v2_departstrip/util.h (2)
time_now_utc(22-22)s(11-17)usermods/usermod_v2_departstrip/departure_view.cpp (4)
view(16-115)view(16-16)appendConfigData(117-139)appendConfigData(117-117)usermods/usermod_v2_departstrip/departure_view.h (3)
addToConfig(27-30)appendConfigData(31-31)readFromConfig(33-56)usermods/usermod_v2_departstrip/siri_source.cpp (5)
addToConfig(434-442)addToConfig(434-434)k(368-368)readFromConfig(444-479)readFromConfig(444-444)usermods/usermod_v2_departstrip/depart_model.cpp (18)
a(49-54)a(49-49)colorMap(131-133)colorMap(131-131)s(46-48)s(46-46)colorToString(214-218)colorToString(214-214)getColorRGB(164-187)getColorRGB(164-164)clearColorMap(135-137)clearColorMap(135-135)removeColorKey(146-149)removeColorKey(146-146)parseColorString(189-212)parseColorString(189-189)setColorRGB(151-157)setColorRGB(151-151)
usermods/usermod_v2_departstrip/departure_view.h (4)
usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp (8)
s(134-142)s(134-134)addToConfig(74-173)addToConfig(74-74)appendConfigData(175-229)appendConfigData(175-175)readFromConfig(231-407)readFromConfig(231-231)usermods/usermod_v2_departstrip/util.h (1)
s(11-17)usermods/usermod_v2_departstrip/departure_view.cpp (4)
view(16-115)view(16-16)appendConfigData(117-139)appendConfigData(117-117)usermods/usermod_v2_departstrip/siri_source.cpp (6)
addToConfig(434-442)addToConfig(434-434)readFromConfig(444-479)readFromConfig(444-444)in(45-45)k(368-368)
usermods/usermod_v2_departstrip/departure_view.cpp (3)
usermods/usermod_v2_departstrip/depart_model.cpp (6)
getColorRGB(164-187)getColorRGB(164-164)a(49-54)a(49-49)s(46-48)s(46-46)usermods/usermod_v2_departstrip/departure_view.h (2)
appendConfigData(31-31)configKey(57-57)usermods/usermod_v2_departstrip/util.h (1)
s(11-17)
🔇 Additional comments (7)
usermods/usermod_v2_departstrip/departure_view.cpp (3)
22-37: Source aggregation looks solid; agency extraction is correct.Deriving the agency prefix from the board key and carrying it alongside the batch is clean and keeps color lookup cheap later.
117-139: UI summary block is safe and helpful.Pointer checks, item counting, and the sanitized configKey() output look good. No string injection risks observed here.
45-47: Ignore theendDraw()suggestion; no such API exists onSegment, so no explicit “close” call is needed.Likely an incorrect or invalid review comment.
usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp (4)
49-53: Nice enable/on-edge reload gating.Only reloading after safety delay and when time is sane prevents boot churn and API hammering.
123-171: ColorMap emission and natural sort are well done.Stable ordering by agency then numeric route (with suffix tie-break) reads great in UI. This also preserves direction-specific keys, matching your stated intent to allow per-direction colors.
350-358: Read-back per-section looks correct.Forwarding sub-objects to each component’s readFromConfig with invalidate_history threading is clean.
175-229: UI swatches: escaping and per-route colors LGTM.Proper HTML/JS escaping for stop names and per-route color chips match the model’s map. Good UX touch.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
usermods/usermod_v2_departstrip/depart_model.cpp (1)
155-177: Direction-specific colors: acknowledged and LGTMPersisting defaults per full LineRef (including direction) matches the intended design discussed earlier; no change requested.
usermods/usermod_v2_departstrip/siri_source.cpp (1)
250-255: Cap ArduinoJson capacity to avoid oversized allocations.The
contentLen*2allocation can exceed typical free heap on ESP8266/ESP32. Add a reasonable upper bound to prevent memory exhaustion.size_t SiriSource::computeJsonCapacity(int contentLen) { - if (contentLen > 0) return (size_t)contentLen * 2; + if (contentLen > 0) { + size_t cap = (size_t)contentLen * 2; + if (cap > 32768) cap = 32768; // 32 KB ceiling + return cap; + } // Unknown length: pick a slightly larger default to reduce NoMemory risk // while the filter keeps actual usage low on most feeds. return 20480; // 20 KB }
🧹 Nitpick comments (13)
usermods/usermod_v2_departstrip/util.cpp (3)
6-23: Clamp HSV inputs and use integer sector to avoid edge artifactsClamp s/v to [0..1] and compute the sector as an integer to prevent float rounding glitches (e.g., h≈1.0 landing in wrong case). Also clamp final channels before cast to uint8_t.
uint32_t hsv2rgb(float h, float s, float v) { - if (s <= 0.f) return RGBW32(v * 255, v * 255, v * 255, 0); - h = fmodf(h, 1.f); if (h < 0.f) h += 1.f; - float i = floorf(h * 6.f); + // clamp inputs + s = fminf(fmaxf(s, 0.f), 1.f); + v = fminf(fmaxf(v, 0.f), 1.f); + if (s <= 0.f) return RGBW32((uint8_t)lroundf(v*255.f), + (uint8_t)lroundf(v*255.f), + (uint8_t)lroundf(v*255.f), 0); + h = fmodf(h, 1.f); if (h < 0.f) h += 1.f; + int i = (int)floorf(h * 6.f); float f = h * 6.f - i; float p = v * (1.f - s); float q = v * (1.f - s * f); float t = v * (1.f - s * (1.f - f)); float r=0,g=0,b=0; - switch(((int)i) % 6) { + switch(i % 6) { case 0: r=v,g=t,b=p; break; case 1: r=q,g=v,b=p; break; case 2: r=p,g=v,b=t; break; case 3: r=p,g=q,b=v; break; case 4: r=t,g=p,b=v; break; case 5: r=v,g=p,b=q; break; } - return RGBW32((uint8_t)(r*255),(uint8_t)(g*255),(uint8_t)(b*255),0); + r = fminf(fmaxf(r, 0.f), 1.f); + g = fminf(fmaxf(g, 0.f), 1.f); + b = fminf(fmaxf(b, 0.f), 1.f); + return RGBW32((uint8_t)lroundf(r*255.f), + (uint8_t)lroundf(g*255.f), + (uint8_t)lroundf(b*255.f), 0); }
26-34: Support optional '+' sign in leading integer parseMinor: allow a leading '+' to be ignored for friendlier natural ordering on inputs like "+1A".
static bool parseLeadingInt(const String& s, int& val, int& consumed) { val = 0; consumed = 0; bool any = false; - for (unsigned i = 0; i < s.length(); ++i) { + unsigned i = 0; + if (s.length() && s.charAt(0) == '+') { i = 1; } + for (; i < s.length(); ++i) { char ch = s.charAt(i); if (ch >= '0' && ch <= '9') { any = true; val = val*10 + (ch - '0'); ++consumed; } else break; } return any; }
36-52: Consider case-insensitive suffix compare for stabilityNatural compares often treat suffixes case-insensitively (“10a” vs “10A”). Optional, but improves deterministic ordering across feeds with mixed casing.
if (ha && hb) { if (na != nb) return (na < nb) ? -1 : 1; - String sa = a.substring(ca); - String sb = b.substring(cb); + String sa = a.substring(ca); sa.toUpperCase(); + String sb = b.substring(cb); sb.toUpperCase(); int r = sa.compareTo(sb); if (r != 0) return (r < 0) ? -1 : 1; return 0; } - if (ha != hb) return ha ? -1 : 1; // numeric routes before non-numeric - int r = a.compareTo(b); + if (ha != hb) return ha ? -1 : 1; // numeric routes before non-numeric + String aa = a; aa.toUpperCase(); + String bb = b; bb.toUpperCase(); + int r = aa.compareTo(bb);usermods/usermod_v2_departstrip/util.h (3)
11-20: Make FreezeGuard explicitly non-movable to prevent accidental double-restoreGuard objects should never be moved. Explicitly delete move ops for future-proofing.
~FreezeGuard() noexcept { seg.freeze = prev; } FreezeGuard(const FreezeGuard &) = delete; FreezeGuard &operator=(const FreezeGuard &) = delete; + FreezeGuard(FreezeGuard&&) = delete; + FreezeGuard& operator=(FreezeGuard&&) = delete;
32-38: Check strftime result to avoid unterminated/empty output on overflowIf the buffer is too small, strftime returns 0; consider falling back to an ISO time or ensuring out[0]='\0'.
gmtime_r(&local_sec, &tmLocal); - strftime(out, n, fmt, &tmLocal); + if (strftime(out, n, fmt, &tmLocal) == 0 && n) out[0] = '\0';
42-46: API doc nit: clarify comparator’s total orderingMention that comparison is case-insensitive (if adopted) and stable for equal keys to guide callers relying on strict weak ordering.
usermods/usermod_v2_departstrip/depart_model.cpp (4)
9-13: Move-assign batch to avoid copies during mergeNo behavior change; saves an allocation/copy when updating an existing board.
- it->batch = e.batch; + it->batch = std::move(e.batch);
31-48: Dedup/sort micro-opts for small vectorsOptional: reserve capacity and use sort/unique to dedup, then stable-sort with natural comparator.
void DepartModel::currentLinesForBoard(const String& key, std::vector<String>& out) const { out.clear(); const Entry* e = find(key); if (!e) return; const auto& items = e->batch.items; - // Deduplicate lines - for (const auto& it : items) { + out.reserve(items.size()); + for (const auto& it : items) { const String& lr = it.lineRef; if (lr.length() == 0) continue; - bool seen = false; - for (const auto& s : out) { if (s == lr) { seen = true; break; } } - if (!seen) out.push_back(lr); + out.push_back(lr); } - // Natural sort: numeric prefix, then suffix (shared helper) - std::sort(out.begin(), out.end(), [](const String& a, const String& b){ - return departstrip::util::cmpLineRefNatural(a, b) < 0; - }); + std::sort(out.begin(), out.end()); + out.erase(std::unique(out.begin(), out.end()), out.end()); + std::sort(out.begin(), out.end(), [](const String& a, const String& b){ + return departstrip::util::cmpLineRefNatural(a, b) < 0; + }); }
180-203: Be tolerant of “RGB:” prefix casing and whitespaceAccept upper/mixed-case “RGB:” and optional spaces after commas to reduce config friction.
bool DepartModel::parseColorString(const String& s, uint32_t& rgbOut) { if (s.length() >= 7 && s[0] == '#') { @@ - if (s.startsWith("rgb:")) { - int r=0,g=0,b=0; - if (sscanf(s.c_str()+4, "%d,%d,%d", &r,&g,&b) == 3) { + String lower = s; lower.toLowerCase(); + if (lower.startsWith("rgb:")) { + int r=0,g=0,b=0; + // allow optional spaces after commas + if (sscanf(lower.c_str()+4, "%d , %d , %d", &r,&g,&b) == 3) { r = std::max(0, std::min(255, r)); g = std::max(0, std::min(255, g)); b = std::max(0, std::min(255, b)); rgbOut = ((uint32_t)r<<16)|((uint32_t)g<<8)|(uint32_t)b; return true; } }
122-128: Expose read-only colors safelyReturning a const ref is fine, but note callers can still copy iterators and be invalidated by later mutations. If this bites later, consider exposing a snapshot copy or accessor APIs instead.
usermods/usermod_v2_departstrip/siri_source.cpp (3)
111-124: Timezone parsing may miss fractional seconds before the timezone indicator.The current implementation searches for timezone designators directly in the string after 'T', but RFC3339 allows fractional seconds (e.g.,
2025-01-01T12:00:00.123Z). The fractional part should be skipped when looking for the timezone indicator.// Find timezone indicator only after the time component (after 'T', and optional fractional seconds) const char* tmark = strchr(s, 'T'); const char* tz = nullptr; if (tmark) { - // Search in the remainder for 'Z', 'z', '+' or '-' (timezone designators) - tz = strpbrk(tmark + 1, "Zz+-"); + // Skip to after seconds (position past HH:MM:SS) + const char* p = tmark + 1; + // Skip HH:MM:SS (already validated by sscanf) + p = strchr(p, ':'); if (p) p = strchr(p+1, ':'); + if (p) { + p += 3; // Move past SS + // Skip fractional seconds if present + if (*p == '.') { + ++p; + while (*p >= '0' && *p <= '9') ++p; + } + // Now look for timezone + tz = (*p == 'Z' || *p == 'z' || *p == '+' || *p == '-') ? p : nullptr; + } }
319-320: Consider validating LineRef extraction.When
LineRefis an object but doesn't have a "value" field, this will set an empty string. Consider logging when this happens for debugging feed format issues.// LineRef may be nested or plain; handle both - if (mvj["LineRef"].is<JsonObject>()) item.lineRef = (const char*)(mvj["LineRef"]["value"] | ""); + if (mvj["LineRef"].is<JsonObject>()) { + const char* val = mvj["LineRef"]["value"] | nullptr; + if (!val) { + DEBUG_PRINTLN(F("DepartStrip: LineRef object missing 'value' field")); + } + item.lineRef = val ? val : ""; + } else item.lineRef = (const char*)(mvj["LineRef"] | "");
349-363: Consider adding jitter to backoff delays.The periodic backoff status logging is helpful. Consider adding small random jitter to the backoff delays to prevent synchronized retry storms if multiple devices are polling the same endpoint.
// Schedule retry with exponential backoff (per source) long delay = (long)updateSecs_ * (long)backoffMult_; + // Add 0-10% jitter to prevent synchronized retries + delay += (delay * (random(0, 11))) / 100; DEBUG_PRINTF("DepartStrip: SiriSource::fetch: scheduling backoff x%u %s for %lds (begin/HTTP error)\n", (unsigned)backoffMult_, sourceKey().c_str(), delay);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
usermods/usermod_v2_departstrip/depart_model.cpp(1 hunks)usermods/usermod_v2_departstrip/departure_view.h(1 hunks)usermods/usermod_v2_departstrip/siri_source.cpp(1 hunks)usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp(1 hunks)usermods/usermod_v2_departstrip/util.cpp(1 hunks)usermods/usermod_v2_departstrip/util.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp
- usermods/usermod_v2_departstrip/departure_view.h
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ksedgwic
PR: wled/WLED#4919
File: usermods/usermod_v2_departstrip/depart_model.cpp:164-187
Timestamp: 2025-09-09T23:14:06.323Z
Learning: In the DepartStrip usermod, it's intentionally valid to have different colors for different directions (e.g., "72M-N" vs "72M-S"), even though they may map to the same color values in the current implementation.
📚 Learning: 2025-09-09T23:14:06.323Z
Learnt from: ksedgwic
PR: wled/WLED#4919
File: usermods/usermod_v2_departstrip/depart_model.cpp:164-187
Timestamp: 2025-09-09T23:14:06.323Z
Learning: In the DepartStrip usermod, the ColorMap entries are limited per stop to a manageable set, so unbounded growth is not a practical concern. The colormap can be reset if lots of changing of stops is done.
Applied to files:
usermods/usermod_v2_departstrip/depart_model.cpp
📚 Learning: 2025-09-09T23:14:06.323Z
Learnt from: ksedgwic
PR: wled/WLED#4919
File: usermods/usermod_v2_departstrip/depart_model.cpp:164-187
Timestamp: 2025-09-09T23:14:06.323Z
Learning: In the DepartStrip usermod, it's intentionally valid to have different colors for different directions (e.g., "72M-N" vs "72M-S"), even though they may map to the same color values in the current implementation.
Applied to files:
usermods/usermod_v2_departstrip/depart_model.cpp
🧬 Code graph analysis (4)
usermods/usermod_v2_departstrip/depart_model.cpp (2)
usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp (4)
key(129-133)key(129-129)a(134-139)a(134-134)usermods/usermod_v2_departstrip/util.cpp (2)
cmpLineRefNatural(36-52)cmpLineRefNatural(36-36)
usermods/usermod_v2_departstrip/siri_source.cpp (3)
usermods/usermod_v2_departstrip/util.h (2)
s(11-17)time_now_utc(22-22)usermods/usermod_v2_departstrip/siri_source.h (1)
SiriSource(17-43)usermods/usermod_v2_departstrip/departure_view.h (2)
addToConfig(24-27)readFromConfig(30-50)
usermods/usermod_v2_departstrip/util.h (2)
usermods/usermod_v2_departstrip/util.cpp (4)
hsv2rgb(6-24)hsv2rgb(6-6)cmpLineRefNatural(36-52)cmpLineRefNatural(36-36)usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp (2)
a(134-139)a(134-134)
usermods/usermod_v2_departstrip/util.cpp (1)
usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp (2)
a(134-139)a(134-134)
🔇 Additional comments (5)
usermods/usermod_v2_departstrip/siri_source.cpp (5)
370-385: Good API key redaction implementation!The implementation properly redacts multiple API key parameter formats in debug logs, preventing sensitive credentials from being exposed. This is a security best practice.
134-146: HTTP timeout and headers configuration looks good.The implementation properly sets a reasonable timeout (10s), mimics curl to avoid compressed responses, and explicitly requests identity encoding to prevent gzip issues on memory-constrained devices.
477-478: Good defensive programming with minimum update interval.Enforcing a minimum 10-second update interval prevents excessive API calls and potential rate limiting issues.
389-394: Exponential backoff implementation is robust.The backoff strategy with a maximum multiplier of 16 provides good resilience against temporary failures while avoiding excessive delays. The logging at each backoff point aids debugging.
Also applies to: 403-408, 416-421
9-56: Clever BOM handling with SkipBOMStream.The stream wrapper elegantly handles UTF-8 BOM removal, which is important for compatibility with various SIRI feed implementations that may include BOMs.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
usermods/usermod_v2_departstrip/departure_view.h (1)
63-93: Graceful fallback when tokens don’t resolve to model keysIf keys_ parses but none map to model boards (e.g., all bare StopCodes with no agency hint), view() won’t hit the existing “keys_.empty()” fallback and nothing renders. Consider a secondary fallback when sources_ ends up empty after scanning keys_.
Example (outside this header, in departure_view.cpp after building sources_):
if (sources_.empty() && keysStr_.length()) { if (const auto* e = model.find(keysStr_)) sources_.push_back(Src{ &e->batch, nullptr }); }usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp (1)
175-187: Escape route codes in appended HTML to avoid layout/JS issuesline (LineRef) is injected into HTML without escaping. While usually alphanumeric, defensive escaping avoids breakage with unexpected data.
- for (const auto& line : lines) { + for (const auto& line : lines) { uint32_t rgb = 0x606060; if (agency.length() > 0) DepartModel::getColorRGB(agency, line, rgb); String col = DepartModel::colorToString(rgb); s.print(F("<span style=\\'display:inline-flex;align-items:center;border:1px solid #888;padding:2px 6px;border-radius:4px;\\'>")); s.print(F("<span style=\\'display:inline-block;width:14px;height:14px;margin-right:6px;background:")); s.print(col); - s.print(F(";\\'></span><code>")); - s.print(line); + s.print(F(";\\'></span><code>")); + String ln = line; + ln.replace("&","&"); + ln.replace("<","<"); ln.replace(">",">"); + ln.replace("\\","\\\\"); ln.replace("'","\\'"); + s.print(ln); s.print(F("</code></span>")); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
usermods/usermod_v2_departstrip/depart_model.cpp(1 hunks)usermods/usermod_v2_departstrip/departure_view.cpp(1 hunks)usermods/usermod_v2_departstrip/departure_view.h(1 hunks)usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- usermods/usermod_v2_departstrip/depart_model.cpp
- usermods/usermod_v2_departstrip/departure_view.cpp
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ksedgwic
PR: wled/WLED#4919
File: usermods/usermod_v2_departstrip/depart_model.cpp:164-187
Timestamp: 2025-09-09T23:14:06.323Z
Learning: In the DepartStrip usermod, it's intentionally valid to have different colors for different directions (e.g., "72M-N" vs "72M-S"), even though they may map to the same color values in the current implementation.
🧬 Code graph analysis (2)
usermods/usermod_v2_departstrip/departure_view.h (4)
usermods/usermod_v2_departstrip/departure_view.cpp (4)
view(17-120)view(17-17)appendConfigData(122-144)appendConfigData(122-122)usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp (6)
addToConfig(74-147)addToConfig(74-74)appendConfigData(149-193)appendConfigData(149-149)readFromConfig(195-371)readFromConfig(195-195)usermods/usermod_v2_departstrip/siri_source.cpp (6)
addToConfig(437-445)addToConfig(437-437)readFromConfig(447-482)readFromConfig(447-447)in(45-45)k(371-371)usermods/usermod_v2_departstrip/util.h (1)
s(11-17)
usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp (7)
usermods/usermod_v2_departstrip/usermod_v2_departstrip.h (2)
DepartStrip(15-46)setup(31-37)usermods/usermod_v2_departstrip/util.h (2)
time_now_utc(22-22)s(11-17)usermods/usermod_v2_departstrip/departure_view.cpp (4)
view(17-120)view(17-17)appendConfigData(122-144)appendConfigData(122-122)usermods/usermod_v2_departstrip/departure_view.h (3)
addToConfig(32-35)appendConfigData(36-36)readFromConfig(38-58)usermods/usermod_v2_departstrip/siri_source.cpp (5)
addToConfig(437-445)addToConfig(437-437)k(371-371)readFromConfig(447-482)readFromConfig(447-447)usermods/usermod_v2_departstrip/depart_model.cpp (10)
colorMap(122-124)colorMap(122-122)getColorRGB(155-178)getColorRGB(155-155)clearColorMap(126-128)clearColorMap(126-126)parseColorString(180-203)parseColorString(180-180)setColorRGB(142-148)setColorRGB(142-142)usermods/usermod_v2_departstrip/util.cpp (2)
cmpLineRefNatural(36-52)cmpLineRefNatural(36-36)
🔇 Additional comments (1)
usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp (1)
66-72: Confirm RAII pairing of beginDraw()/endDraw()
DepartureView::view() wraps seg.beginDraw() in a FreezeGuard; verify that FreezeGuard’s destructor invokes seg.endDraw(). If it does not, add an explicit seg.endDraw() after the guarded scope.
| void updateConfigKey_() { | ||
| String s = String("DepartureView_") + keysStr_; | ||
| s.replace(':', '_'); s.replace(',', '_'); s.replace(' ', '_'); s.replace(';', '_'); | ||
| s.replace('\'', '_'); s.replace('\"', '_'); s.replace('\\', '_'); | ||
| configKey_ = std::string(s.c_str()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden configKey_ sanitization to avoid breaking the JSON/JS UI
configKey_ is embedded in addInfo('DepartStrip::...') without escaping. Newlines/tabs/slashes (and any non [A–Z,a–z,0–9,_]) can break JS or create invalid object keys. Replace/normalize a broader set.
void updateConfigKey_() {
String s = String("DepartureView_") + keysStr_;
- s.replace(':', '_'); s.replace(',', '_'); s.replace(' ', '_'); s.replace(';', '_');
- s.replace('\'', '_'); s.replace('\"', '_'); s.replace('\\', '_');
+ s.replace(':','_'); s.replace(',','_'); s.replace(' ','_'); s.replace(';','_');
+ s.replace('\'','_'); s.replace('\"','_'); s.replace('\\','_');
+ s.replace('\n','_'); s.replace('\r','_'); s.replace('\t','_'); s.replace('/','_');
+ // Normalize any other odd chars to '_'
+ for (uint16_t i = 0; i < s.length(); ++i) {
+ char c = s.charAt(i);
+ bool ok = (c=='_') || (c>='0'&&c<='9') || (c>='A'&&c<='Z') || (c>='a'&&c<='z');
+ if (!ok) s.setCharAt(i,'_');
+ }
configKey_ = std::string(s.c_str());
}📝 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.
| void updateConfigKey_() { | |
| String s = String("DepartureView_") + keysStr_; | |
| s.replace(':', '_'); s.replace(',', '_'); s.replace(' ', '_'); s.replace(';', '_'); | |
| s.replace('\'', '_'); s.replace('\"', '_'); s.replace('\\', '_'); | |
| configKey_ = std::string(s.c_str()); | |
| } | |
| void updateConfigKey_() { | |
| String s = String("DepartureView_") + keysStr_; | |
| s.replace(':','_'); s.replace(',','_'); s.replace(' ','_'); s.replace(';','_'); | |
| s.replace('\'','_'); s.replace('\"','_'); s.replace('\\','_'); | |
| s.replace('\n','_'); s.replace('\r','_'); s.replace('\t','_'); s.replace('/','_'); | |
| // Normalize any other odd chars to '_' | |
| for (uint16_t i = 0; i < s.length(); ++i) { | |
| char c = s.charAt(i); | |
| bool ok = (c=='_') || (c>='0'&&c<='9') || (c>='A'&&c<='Z') || (c>='a'&&c<='z'); | |
| if (!ok) s.setCharAt(i,'_'); | |
| } | |
| configKey_ = std::string(s.c_str()); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
usermods/usermod_v2_departstrip/siri_source.cpp (1)
226-231: Cap ArduinoJson capacity to avoid oversized allocations.contentLen*2 can exceed free heap on ESP8266/ESP32; add a ceiling (e.g., 32 KB).
size_t SiriSource::computeJsonCapacity(int contentLen) { - if (contentLen > 0) return (size_t)contentLen * 2; + if (contentLen > 0) { + size_t cap = (size_t)contentLen * 2; + if (cap > 32768) cap = 32768; // ceiling + return cap; + } // Unknown length: pick a slightly larger default to reduce NoMemory risk // while the filter keeps actual usage low on most feeds. return 20480; // 20 KB }
🧹 Nitpick comments (5)
usermods/usermod_v2_departstrip/siri_source.h (1)
41-44: Expose dynamic metadata via appendConfigData().Consider emitting StopName (when known) so UIs can display a friendly label without reopening JSON.
Apply:
- void appendConfigData(Print& s) override {} + void appendConfigData(Print& s) override { + if (lastStopName_.length() > 0) { + s.print(F("\"StopName\":\"")); + s.print(lastStopName_); + s.print(F("\",")); + } + }usermods/usermod_v2_departstrip/siri_source.cpp (4)
99-131: RFC3339 parser fixes look good; add tiny resilience.You now handle 'z' and ±HHMM. Optionally accept bare ±HH as a fallback.
- if (sscanf(tz+1, "%2d:%2d", &th, &tmn) == 2) { + if (sscanf(tz+1, "%2d:%2d", &th, &tmn) == 2) { tzH = th; tzM = tmn; - } else if (sscanf(tz+1, "%2d%2d", &th, &tmn) == 2) { + } else if (sscanf(tz+1, "%2d%2d", &th, &tmn) == 2) { tzH = th; tzM = tmn; + } else if (sscanf(tz+1, "%2d", &th) == 1) { // accept ±HH + tzH = th; tzM = 0; }
133-173: Follow in-protocol redirects and collect common RL header.Enable redirects (HTTP→HTTP) and also collect X-RateLimit-Remaining.
http_.setTimeout(10000); + http_.setFollowRedirects(HTTPC_STRICT_FOLLOW_REDIRECTS); @@ - static const char* hdrs[] = {"Content-Type", "Content-Encoding", "Content-Length", "RateLimit-Remaining"}; - http_.collectHeaders(hdrs, 4); + static const char* hdrs[] = { + "Content-Type","Content-Encoding","Content-Length", + "RateLimit-Remaining","X-RateLimit-Remaining" + }; + http_.collectHeaders(hdrs, sizeof(hdrs)/sizeof(hdrs[0])); @@ - { - String rl = http_.header("RateLimit-Remaining"); + { + String rl = http_.header("RateLimit-Remaining"); + if (!rl.length()) rl = http_.header("X-RateLimit-Remaining"); if (rl.length() > 0) { DEBUG_PRINTF("DepartStrip: SiriSource::fetch: RateLimit-Remaining=%s\n", rl.c_str()); } }Note: If a provider forces HTTP→HTTPS, this will still fail by design (no TLS), which is acceptable per your memory constraints.
246-319: Model build is solid; add two small robustness tweaks.
- Iterate all StopMonitoringDelivery entries (some providers send >1).
- Fallback to PublishedLineName when LineRef missing.
- JsonArray visits; - if (sd["StopMonitoringDelivery"].is<JsonArray>()) { - JsonObject del = sd["StopMonitoringDelivery"][0].as<JsonObject>(); - visits = del["MonitoredStopVisit"].as<JsonArray>(); - } else if (sd["StopMonitoringDelivery"].is<JsonObject>()) { - JsonObject del = sd["StopMonitoringDelivery"].as<JsonObject>(); - visits = del["MonitoredStopVisit"].as<JsonArray>(); - } + JsonArray visits; + auto addVisits = [&](JsonObject del){ + JsonArray a = del["MonitoredStopVisit"].as<JsonArray>(); + if (!a.isNull()) { if (visits.isNull()) visits = a; else for (JsonObject v : a) visits.add(v); } + }; + if (sd["StopMonitoringDelivery"].is<JsonArray>()) { + for (JsonObject del : sd["StopMonitoringDelivery"].as<JsonArray>()) addVisits(del); + } else if (sd["StopMonitoringDelivery"].is<JsonObject>()) { + addVisits(sd["StopMonitoringDelivery"].as<JsonObject>()); + } @@ - if (mvj["LineRef"].is<JsonObject>()) item.lineRef = (const char*)(mvj["LineRef"]["value"] | ""); - else item.lineRef = (const char*)(mvj["LineRef"] | ""); + if (mvj["LineRef"].is<JsonObject>()) item.lineRef = (const char*)(mvj["LineRef"]["value"] | ""); + else item.lineRef = (const char*)(mvj["LineRef"] | ""); + if (item.lineRef.length() == 0) { + item.lineRef = (const char*)(mvj["PublishedLineName"] | ""); + }
343-361: Broaden API-key redaction patterns.Also redact uppercase variants to avoid leaks.
redactParam(F("api_key=")); redactParam(F("apikey=")); redactParam(F("key=")); + redactParam(F("API_KEY=")); + redactParam(F("APIKEY=")); + redactParam(F("KEY="));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
usermods/usermod_v2_departstrip/siri_source.cpp(1 hunks)usermods/usermod_v2_departstrip/siri_source.h(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: ksedgwic
PR: wled/WLED#4919
File: usermods/usermod_v2_departstrip/depart_model.cpp:164-187
Timestamp: 2025-09-09T23:14:06.323Z
Learning: In the DepartStrip usermod, it's intentionally valid to have different colors for different directions (e.g., "72M-N" vs "72M-S"), even though they may map to the same color values in the current implementation.
📚 Learning: 2025-09-09T23:21:00.549Z
Learnt from: ksedgwic
PR: wled/WLED#4919
File: usermods/usermod_v2_departstrip/readme.md:39-49
Timestamp: 2025-09-09T23:21:00.549Z
Learning: For the DepartStrip usermod on ESP8266/ESP32 devices, HTTP is preferred over HTTPS in documentation examples and templates due to heap memory constraints. The combined memory requirements of TLS handshake and JSON parsing for transit API responses can exceed available memory on these embedded devices.
Applied to files:
usermods/usermod_v2_departstrip/siri_source.h
📚 Learning: 2025-09-09T23:20:44.936Z
Learnt from: ksedgwic
PR: wled/WLED#4919
File: usermods/usermod_v2_departstrip/readme.md:15-23
Timestamp: 2025-09-09T23:20:44.936Z
Learning: For the DepartStrip usermod on ESP8266/ESP32 devices, HTTP is used instead of HTTPS for API endpoints due to heap memory constraints. HTTPS would require too much memory for the combined TLS handshake and JSON parsing operations on these embedded devices, making HTTP a necessary compromise despite security considerations.
Applied to files:
usermods/usermod_v2_departstrip/siri_source.h
📚 Learning: 2025-08-29T19:51:15.511Z
Learnt from: ksedgwic
PR: wled/WLED#4883
File: usermods/usermod_v2_skystrip/open_weather_map_source.cpp:13-19
Timestamp: 2025-08-29T19:51:15.511Z
Learning: On ESP8266/ESP32 devices with limited heap, using HTTPS for API calls that return large JSON responses may not be feasible due to the combined memory requirements of TLS handshake and JSON parsing. HTTP may be necessary despite security concerns.
Applied to files:
usermods/usermod_v2_departstrip/siri_source.h
📚 Learning: 2025-08-29T20:00:13.371Z
Learnt from: ksedgwic
PR: wled/WLED#4884
File: usermods/usermod_v2_bartdepart/legacy_bart_source.h:19-24
Timestamp: 2025-08-29T20:00:13.371Z
Learning: For the BartDepart usermod, HTTPS with setInsecure() is necessary because BART API servers redirect HTTP requests to HTTPS, not for security reasons. The application displays public transit data and is not security critical.
Applied to files:
usermods/usermod_v2_departstrip/siri_source.h
📚 Learning: 2025-08-29T19:55:40.246Z
Learnt from: ksedgwic
PR: wled/WLED#4884
File: usermods/usermod_v2_bartdepart/legacy_bart_source.cpp:4-6
Timestamp: 2025-08-29T19:55:40.246Z
Learning: For the BartDepart usermod on ESP8266/ESP32, setInsecure() is used for TLS connections due to heap memory constraints, and this is acceptable because it's not a security critical application (transit departure display).
Applied to files:
usermods/usermod_v2_departstrip/siri_source.h
🧬 Code graph analysis (2)
usermods/usermod_v2_departstrip/siri_source.h (1)
usermods/usermod_v2_departstrip/siri_source.cpp (25)
SiriSource(58-63)fetch(321-410)fetch(321-321)reload(65-68)reload(65-65)k(347-347)addToConfig(412-420)addToConfig(412-412)readFromConfig(422-457)readFromConfig(422-422)composeUrl(70-86)composeUrl(70-70)parseRFC3339ToUTC(99-131)parseRFC3339ToUTC(99-99)httpBegin(133-173)httpBegin(133-133)parseJsonFromHttp(175-224)parseJsonFromHttp(175-175)doc(375-375)computeJsonCapacity(226-231)computeJsonCapacity(226-226)getSiriRoot(233-244)getSiriRoot(233-233)buildModelFromSiri(246-319)buildModelFromSiri(246-246)
usermods/usermod_v2_departstrip/siri_source.cpp (3)
usermods/usermod_v2_departstrip/util.h (2)
s(11-17)time_now_utc(22-22)usermods/usermod_v2_departstrip/siri_source.h (1)
SiriSource(17-43)usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp (2)
readFromConfig(195-371)readFromConfig(195-195)
🔇 Additional comments (9)
usermods/usermod_v2_departstrip/siri_source.h (3)
8-13: HTTP-only client choice is appropriate for ESP-class heap limits.Keeping only WiFiClient/HTTPClient here aligns with your stated constraint to avoid TLS due to memory pressure. No action needed.
19-27: Good: widened polling interval type and explicit backoff fields.uint32_t for updateSecs_ plus nextFetch_/backoffMult_ bookkeeping is a solid base for stable polling.
51-59: API surface looks tidy and self-contained.Helper separation (composeUrl/HTTP/JSON parsing/model build) keeps fetch() readable.
usermods/usermod_v2_departstrip/siri_source.cpp (6)
8-56: SkipBOMStream is correct and lightweight.Handles leading BOM without extra allocations; overrides are sufficient for ArduinoJson streaming.
70-86: URL templating is robust.Case-variant placeholders and optional {apiKey} handling are good. Treating baseUrl strictly as a template avoids accidental query duplication.
175-224: JSON filter is well-scoped and ends HTTP promptly.Good use of a static filter doc and BOM-skipping stream; end() is called even on error.
321-410: Fetch/backoff flow is clear and source-scoped.Good: periodic backoff logging, exponential backoff with cap, and reset on success.
412-420: Config export fields are sufficient.Enabled/UpdateSecs/TemplateUrl/ApiKey and combined AgencyStopCode cover typical needs.
422-457: Config ingest: nice combined key handling and min-cadence clamp.Renaming configKey_ to SiriSource_AGENCY_StopCode improves UX. Reload on enable at startup is correct.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
usermods/usermod_v2_departstrip/util.cpp (2)
6-24: Round HSV→RGB bytes to nearest to avoid dimming bias.Use simple +0.5f rounding for tighter color fidelity on low values.
- return RGBW32((uint8_t)(r*255),(uint8_t)(g*255),(uint8_t)(b*255),0); + return RGBW32((uint8_t)(r*255 + 0.5f), + (uint8_t)(g*255 + 0.5f), + (uint8_t)(b*255 + 0.5f), 0);
59-61: Centralize DisplayMinutes bounds to avoid drift across files.Define shared min/max in util.h and reuse here and in config parsing.
- if (minutes < 10) minutes = 10; // minimum 10 minutes - if (minutes > 240) minutes = 240; // maximum 4 hours + if (minutes < departstrip::util::kDisplayMinutesMin) minutes = departstrip::util::kDisplayMinutesMin; + if (minutes > departstrip::util::kDisplayMinutesMax) minutes = departstrip::util::kDisplayMinutesMax;usermods/usermod_v2_departstrip/util.h (1)
47-51: Expose shared DisplayMinutes bounds.Prevents magic numbers across modules.
namespace departstrip { namespace util { +static constexpr uint16_t kDisplayMinutesMin = 10; +static constexpr uint16_t kDisplayMinutesMax = 240; // Global display time scale for views (in minutes). // Default 60; configurable via usermod top-level setting. uint16_t getDisplayMinutes(); void setDisplayMinutes(uint16_t minutes);usermods/usermod_v2_departstrip/siri_source.cpp (3)
99-131: RFC3339: reset tz when offset parse fails.If "+/-" is present but parsing HH[:]?MM fails, treat as Z to avoid bogus signed 0 offset.
- } else if (*tz == '+' || *tz == '-') { + } else if (*tz == '+' || *tz == '-') { tzSign = (*tz == '+') ? +1 : -1; int th=0, tmn=0; + bool okOff = false; // Expect "+HH:MM" or "-HH:MM" first; fallback to "+HHMM" or "-HHMM" - if (sscanf(tz+1, "%2d:%2d", &th, &tmn) == 2) { - tzH = th; tzM = tmn; - } else if (sscanf(tz+1, "%2d%2d", &th, &tmn) == 2) { - tzH = th; tzM = tmn; - } + if (sscanf(tz+1, "%2d:%2d", &th, &tmn) == 2) { tzH = th; tzM = tmn; okOff = true; } + else if (sscanf(tz+1, "%2d%2d", &th, &tmn) == 2) { tzH = th; tzM = tmn; okOff = true; } + if (!okOff) { tzSign = 0; tzH = 0; tzM = 0; } }
168-172: Early-bail on gzip to save heap and time.If a server still gzips, fail fast instead of allocating JSON doc and deserializing garbage.
String enc = http_.header("Content-Encoding"); String ctype = http_.header("Content-Type"); + if (enc.equalsIgnoreCase(F("gzip"))) { + DEBUG_PRINTLN(F("DepartStrip: SiriSource::fetch: gzip not supported; ending")); + http_.end(); + return false; + } outLen = http_.getSize();
352-370: Redact more credential parameter names.Covers common variants beyond key/apikey.
redactParam(F("api_key=")); redactParam(F("apikey=")); redactParam(F("key=")); + redactParam(F("token=")); + redactParam(F("access_token=")); + redactParam(F("app_key=")); + redactParam(F("appId=")); + redactParam(F("app_id="));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
usermods/usermod_v2_departstrip/departure_view.cpp(1 hunks)usermods/usermod_v2_departstrip/readme.md(1 hunks)usermods/usermod_v2_departstrip/siri_source.cpp(1 hunks)usermods/usermod_v2_departstrip/siri_source.h(1 hunks)usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp(1 hunks)usermods/usermod_v2_departstrip/usermod_v2_departstrip.h(1 hunks)usermods/usermod_v2_departstrip/util.cpp(1 hunks)usermods/usermod_v2_departstrip/util.h(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- usermods/usermod_v2_departstrip/readme.md
🚧 Files skipped from review as they are similar to previous changes (3)
- usermods/usermod_v2_departstrip/departure_view.cpp
- usermods/usermod_v2_departstrip/usermod_v2_departstrip.h
- usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: ksedgwic
PR: wled/WLED#4919
File: usermods/usermod_v2_departstrip/depart_model.cpp:164-187
Timestamp: 2025-09-09T23:14:06.323Z
Learning: In the DepartStrip usermod, it's intentionally valid to have different colors for different directions (e.g., "72M-N" vs "72M-S"), even though they may map to the same color values in the current implementation.
📚 Learning: 2025-09-09T23:21:00.549Z
Learnt from: ksedgwic
PR: wled/WLED#4919
File: usermods/usermod_v2_departstrip/readme.md:39-49
Timestamp: 2025-09-09T23:21:00.549Z
Learning: For the DepartStrip usermod on ESP8266/ESP32 devices, HTTP is preferred over HTTPS in documentation examples and templates due to heap memory constraints. The combined memory requirements of TLS handshake and JSON parsing for transit API responses can exceed available memory on these embedded devices.
Applied to files:
usermods/usermod_v2_departstrip/siri_source.h
📚 Learning: 2025-09-09T23:20:44.936Z
Learnt from: ksedgwic
PR: wled/WLED#4919
File: usermods/usermod_v2_departstrip/readme.md:15-23
Timestamp: 2025-09-09T23:20:44.936Z
Learning: For the DepartStrip usermod on ESP8266/ESP32 devices, HTTP is used instead of HTTPS for API endpoints due to heap memory constraints. HTTPS would require too much memory for the combined TLS handshake and JSON parsing operations on these embedded devices, making HTTP a necessary compromise despite security considerations.
Applied to files:
usermods/usermod_v2_departstrip/siri_source.h
📚 Learning: 2025-08-29T19:51:15.511Z
Learnt from: ksedgwic
PR: wled/WLED#4883
File: usermods/usermod_v2_skystrip/open_weather_map_source.cpp:13-19
Timestamp: 2025-08-29T19:51:15.511Z
Learning: On ESP8266/ESP32 devices with limited heap, using HTTPS for API calls that return large JSON responses may not be feasible due to the combined memory requirements of TLS handshake and JSON parsing. HTTP may be necessary despite security concerns.
Applied to files:
usermods/usermod_v2_departstrip/siri_source.h
📚 Learning: 2025-08-29T20:00:13.371Z
Learnt from: ksedgwic
PR: wled/WLED#4884
File: usermods/usermod_v2_bartdepart/legacy_bart_source.h:19-24
Timestamp: 2025-08-29T20:00:13.371Z
Learning: For the BartDepart usermod, HTTPS with setInsecure() is necessary because BART API servers redirect HTTP requests to HTTPS, not for security reasons. The application displays public transit data and is not security critical.
Applied to files:
usermods/usermod_v2_departstrip/siri_source.h
📚 Learning: 2025-08-29T19:55:40.246Z
Learnt from: ksedgwic
PR: wled/WLED#4884
File: usermods/usermod_v2_bartdepart/legacy_bart_source.cpp:4-6
Timestamp: 2025-08-29T19:55:40.246Z
Learning: For the BartDepart usermod on ESP8266/ESP32, setInsecure() is used for TLS connections due to heap memory constraints, and this is acceptable because it's not a security critical application (transit departure display).
Applied to files:
usermods/usermod_v2_departstrip/siri_source.h
📚 Learning: 2025-09-09T23:14:06.323Z
Learnt from: ksedgwic
PR: wled/WLED#4919
File: usermods/usermod_v2_departstrip/depart_model.cpp:164-187
Timestamp: 2025-09-09T23:14:06.323Z
Learning: In the DepartStrip usermod, it's intentionally valid to have different colors for different directions (e.g., "72M-N" vs "72M-S"), even though they may map to the same color values in the current implementation.
Applied to files:
usermods/usermod_v2_departstrip/util.cpp
📚 Learning: 2025-09-02T19:42:57.475Z
Learnt from: freakintoddles2
PR: wled/WLED#4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Applied to files:
usermods/usermod_v2_departstrip/util.h
🧬 Code graph analysis (3)
usermods/usermod_v2_departstrip/siri_source.h (3)
usermods/usermod_v2_departstrip/siri_source.cpp (25)
SiriSource(58-63)fetch(330-419)fetch(330-330)reload(65-68)reload(65-65)k(356-356)addToConfig(421-429)addToConfig(421-421)readFromConfig(431-466)readFromConfig(431-431)composeUrl(70-86)composeUrl(70-70)parseRFC3339ToUTC(99-131)parseRFC3339ToUTC(99-99)httpBegin(133-173)httpBegin(133-133)parseJsonFromHttp(175-220)parseJsonFromHttp(175-175)doc(384-384)computeJsonCapacity(222-240)computeJsonCapacity(222-222)getSiriRoot(242-253)getSiriRoot(242-242)buildModelFromSiri(255-328)buildModelFromSiri(255-255)usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp (6)
appendConfigData(152-196)appendConfigData(152-152)addToConfig(76-150)addToConfig(76-76)readFromConfig(198-378)readFromConfig(198-198)usermods/usermod_v2_departstrip/departure_view.h (4)
appendConfigData(36-36)addToConfig(32-35)readFromConfig(38-58)configKey(59-59)
usermods/usermod_v2_departstrip/siri_source.cpp (3)
usermods/usermod_v2_departstrip/util.h (2)
s(11-17)time_now_utc(22-22)usermods/usermod_v2_departstrip/siri_source.h (1)
SiriSource(17-43)usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp (5)
addToConfig(76-150)addToConfig(76-76)readFromConfig(198-378)readFromConfig(198-198)keyStr(230-230)
usermods/usermod_v2_departstrip/util.h (1)
usermods/usermod_v2_departstrip/util.cpp (8)
hsv2rgb(6-24)hsv2rgb(6-6)cmpLineRefNatural(36-52)cmpLineRefNatural(36-36)getDisplayMinutes(56-56)getDisplayMinutes(56-56)setDisplayMinutes(57-62)setDisplayMinutes(57-57)
🔇 Additional comments (6)
usermods/usermod_v2_departstrip/util.cpp (1)
36-52: Natural comparator logic LGTM.Numeric-leading before non-numeric and suffix tie‑break is correct for route sorting.
usermods/usermod_v2_departstrip/siri_source.h (2)
19-27: Source state and cadence fields look solid.uint32_t updateSecs_ with backoff fields matches the fetch logic.
41-41: Confirm empty appendConfigData is intentional.If no extra UI fields are needed, fine; otherwise consider exposing StopName for UX.
usermods/usermod_v2_departstrip/util.h (1)
11-20: FreezeGuard RAII is correct.Restores prior freeze state reliably; copy/assign deleted appropriately.
usermods/usermod_v2_departstrip/siri_source.cpp (2)
8-56: BOM-skipping stream wrapper LGTM.Minimal, safe, and avoids parser choke on UTF‑8 BOM.
221-240: Capacity heuristic and quantization LGTM.Nice balance of safety and fragmentation control with a hard cap.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
usermods/usermod_v2_departstrip/departure_view.h (1)
97-102: Harden configKey_ sanitization to avoid breaking the JSON/JS UI.configKey_ is emitted into addInfo(...) and used as a JSON section key. Extend sanitization to normalize newlines, tabs, slashes, and any non [A–Z,a–z,0–9,] to '' to prevent malformed keys.
void updateConfigKey_() { String s = String("DepartureView_") + keysStr_; - s.replace(':', '_'); s.replace(',', '_'); s.replace(' ', '_'); s.replace(';', '_'); - s.replace('\'', '_'); s.replace('\"', '_'); s.replace('\\', '_'); + s.replace(':','_'); s.replace(',','_'); s.replace(' ','_'); s.replace(';','_'); + s.replace('\'','_'); s.replace('\"','_'); s.replace('\\','_'); + s.replace('\n','_'); s.replace('\r','_'); s.replace('\t','_'); s.replace('/','_'); + // Normalize any remaining odd chars + for (uint16_t i = 0; i < s.length(); ++i) { + char c = s.charAt(i); + bool ok = (c=='_') || (c>='0'&&c<='9') || (c>='A'&&c<='Z') || (c>='a'&&c<='z'); + if (!ok) s.setCharAt(i,'_'); + } configKey_ = std::string(s.c_str()); }
🧹 Nitpick comments (2)
usermods/usermod_v2_departstrip/departure_view.h (2)
58-59: Clamp BoardingSecs to a sane range.Protect against extreme values to avoid weird render windows or overflow when scaling to pixels.
- ok &= getJsonValue(root["BoardingSecs"], boardingSecs_, (uint16_t)0); + ok &= getJsonValue(root["BoardingSecs"], boardingSecs_, (uint16_t)0); + if (boardingSecs_ > 600) boardingSecs_ = 600; // cap at 10 minutes
65-68: Pre‑reserve token capacity to reduce heap churn.Minor allocation win on MCUs.
void parseKeysFrom(const String& in) { - keys_.clear(); - agencies_.clear(); + keys_.clear(); agencies_.clear(); + // Approximate token count to reserve capacity + size_t approx = 1; + for (unsigned i = 0; i < in.length(); ++i) { + char ch = in.charAt(i); + if (ch==','||ch==' '||ch=='\t'||ch=='\n'||ch=='\r'||ch==';') ++approx; + } + if (keys_.capacity() < approx) keys_.reserve(approx); + if (agencies_.capacity() < approx) agencies_.reserve(approx);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
usermods/usermod_v2_departstrip/departure_view.cpp(1 hunks)usermods/usermod_v2_departstrip/departure_view.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- usermods/usermod_v2_departstrip/departure_view.cpp
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ksedgwic
PR: wled/WLED#4919
File: usermods/usermod_v2_departstrip/depart_model.cpp:164-187
Timestamp: 2025-09-09T23:14:06.323Z
Learning: In the DepartStrip usermod, it's intentionally valid to have different colors for different directions (e.g., "72M-N" vs "72M-S"), even though they may map to the same color values in the current implementation.
🧬 Code graph analysis (1)
usermods/usermod_v2_departstrip/departure_view.h (4)
usermods/usermod_v2_departstrip/departure_view.cpp (4)
view(17-123)view(17-17)appendConfigData(125-149)appendConfigData(125-125)usermods/usermod_v2_departstrip/siri_source.cpp (6)
addToConfig(421-429)addToConfig(421-421)readFromConfig(431-466)readFromConfig(431-431)in(45-45)k(356-356)usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp (6)
addToConfig(76-150)addToConfig(76-76)appendConfigData(152-196)appendConfigData(152-152)readFromConfig(198-378)readFromConfig(198-198)usermods/usermod_v2_departstrip/util.h (1)
s(11-17)
🔇 Additional comments (1)
usermods/usermod_v2_departstrip/departure_view.h (1)
41-56: Make configKey_ unique per SegmentId to avoid config collisionsTwo DepartureViews with identical AgencyStopCodes currently produce the same configKey_, causing config-section collisions and ambiguous updates/deletes. Include SegmentId and a short stable hash in configKey_ and refresh it when SegmentId changes.
File: usermods/usermod_v2_departstrip/departure_view.h (also applies to: lines 97-102)
@@ - bool readFromConfig(JsonObject& root, bool startup_complete, bool& invalidate_history) override { + bool readFromConfig(JsonObject& root, bool startup_complete, bool& invalidate_history) override { bool ok = true; - ok &= getJsonValue(root["SegmentId"], segmentId_, segmentId_); + int16_t prevSeg = segmentId_; + ok &= getJsonValue(root["SegmentId"], segmentId_, segmentId_); @@ - if (keysStr_ != prev) { + if (keysStr_ != prev) { // Re-parse and update config key parseKeysFrom(keysStr_); updateConfigKey_(); } } + // If segment changed, refresh config key to keep it unique + if (segmentId_ != prevSeg) updateConfigKey_(); @@ void updateConfigKey_() { - String s = String("DepartureView_") + keysStr_; + String s = String("DepartureView_") + keysStr_; s.replace(':','_'); s.replace(',','_'); s.replace(' ','_'); s.replace(';','_'); s.replace('\'','_'); s.replace('\"','_'); s.replace('\\','_'); + s.replace('\n','_'); s.replace('\r','_'); s.replace('\t','_'); s.replace('/','_'); + for (uint16_t i = 0; i < s.length(); ++i) { + char c = s.charAt(i); + bool ok = (c=='_') || (c>='0'&&c<='9') || (c>='A'&&c<='Z') || (c>='a'&&c<='z'); + if (!ok) s.setCharAt(i,'_'); + } + // Append uniqueness suffix: segmentId and short FNV-1a hash of keysStr_ + uint32_t h = 2166136261u; + for (uint16_t i = 0; i < keysStr_.length(); ++i) { h ^= (uint8_t)keysStr_.charAt(i); h *= 16777619u; } + h ^= (uint32_t)segmentId_; h *= 16777619u; + char suf[20]; + snprintf(suf, sizeof(suf), "_%d_%04X", (int)segmentId_, (unsigned)((h>>16) ^ (h & 0xFFFF))); + s += suf; configKey_ = std::string(s.c_str()); }Confirm whether multiple views with the same AgencyStopCodes on different segments are expected; if yes, apply the change.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp (1)
51-62: Boot effect now cleared on first reload; resolves prior commentCalling doneBooting() when didReload && bootEffectActive_ addresses the earlier “boot effect never cleared” concern. Good guard to ensure it runs once.
🧹 Nitpick comments (4)
usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp (4)
299-313: O(n²) duplicate cull; trivial scale but easy to tidyFor many sources this becomes quadratic. Not urgent, but consider unordered_set or sort+unique for linearithmic behavior.
378-381: Reload immediately on config change (outside startup) to reflect editsstartup_complete is hardcoded false, so edits won’t trigger an immediate reload; users may wait up to UpdateSecs. Optional: reload when enabled and time is valid.
Apply this diff:
- if (invalidate_history) { - model_->boards.clear(); - if (startup_complete) reloadSources(departstrip::util::time_now_utc()); - } + if (invalidate_history) { + model_->boards.clear(); + time_t now = departstrip::util::time_now_utc(); + if (enabled_ && !offMode && now > 0) reloadSources(now); + }
383-385: Return the accumulated status instead of unconditional trueUnconditionally returning true can mask parse/load failures. Return ok to surface errors correctly.
Apply this diff:
- return true; + return ok;
387-403: Consider restoring prior segment state after boot effectdoneBooting() switches mode to 0 but leaves palette/colors altered by showBooting(). Optionally capture and restore previous mode/palette/colors to avoid surprising the main effect.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp(1 hunks)usermods/usermod_v2_departstrip/usermod_v2_departstrip.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- usermods/usermod_v2_departstrip/usermod_v2_departstrip.h
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ksedgwic
PR: wled/WLED#4919
File: usermods/usermod_v2_departstrip/depart_model.cpp:164-187
Timestamp: 2025-09-09T23:14:06.323Z
Learning: In the DepartStrip usermod, it's intentionally valid to have different colors for different directions (e.g., "72M-N" vs "72M-S"), even though they may map to the same color values in the current implementation.
🧬 Code graph analysis (1)
usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp (7)
usermods/usermod_v2_departstrip/usermod_v2_departstrip.h (2)
DepartStrip(15-48)setup(33-39)usermods/usermod_v2_departstrip/util.cpp (4)
setDisplayMinutes(57-62)setDisplayMinutes(57-57)cmpLineRefNatural(36-52)cmpLineRefNatural(36-36)usermods/usermod_v2_departstrip/util.h (2)
time_now_utc(22-22)s(11-17)usermods/usermod_v2_departstrip/departure_view.cpp (4)
view(17-123)view(17-17)appendConfigData(125-149)appendConfigData(125-125)usermods/usermod_v2_departstrip/departure_view.h (3)
addToConfig(32-36)appendConfigData(37-37)readFromConfig(39-60)usermods/usermod_v2_departstrip/siri_source.cpp (5)
addToConfig(421-429)addToConfig(421-421)k(356-356)readFromConfig(431-466)readFromConfig(431-431)usermods/usermod_v2_departstrip/depart_model.cpp (14)
colorMap(122-124)colorMap(122-122)colorToString(205-209)colorToString(205-205)getColorRGB(155-178)getColorRGB(155-155)clearColorMap(126-128)clearColorMap(126-126)removeColorKey(137-140)removeColorKey(137-137)parseColorString(180-203)parseColorString(180-180)setColorRGB(142-148)setColorRGB(142-142)
🔇 Additional comments (5)
usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp (5)
29-37: Setup sequencing and safety window look solidSAFETY_DELAY_MS gating, early displayMinutes init, and conditional showBooting() are appropriate for a safe boot path.
66-73: Non-blocking render guard is good; ensure HTTP timeouts in sourcesstrip.isUpdating() short‑circuit prevents render contention. Please verify SiriSource::fetch() uses connection/read timeouts and avoids long blocking on the main loop.
88-103: Stable ordering and placeholders for UX are well doneSorting sources/views by key and emitting NewSource/NewView blocks yields a predictable UI/JSON layout.
Also applies to: 112-127
132-156: ColorMap emission: deterministic and user-friendlyNatural sort by agency/route (numeric-aware) is a nice touch; ColorMapReset flag placement is sensible.
168-201: Config UI path mutates the ColorMap via getColorRGB()appendConfigData calls DepartModel::getColorRGB(), which persists defaults for unseen routes. That means merely viewing the page can grow ColorMap. If that’s unintended, switch to a non‑persisting lookup. If intentional, please confirm.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
usermods/usermod_v2_departstrip/departure_view.cpp (1)
196-203: Escape route/line text before injecting into HTML (prevent XSS/broken UI).line comes from external feeds; print escapes like Stop name to avoid script/html injection.
- s.print(F(";\\'></span><code>")); - s.print(line); - s.print(F("</code></span>")); + s.print(F(";\\'></span><code>")); + String le = line; + le.replace("&","&"); + le.replace("<","<"); le.replace(">",">"); + le.replace("\\","\\\\"); le.replace("'","\\'"); + s.print(le); + s.print(F("</code></span>"));
♻️ Duplicate comments (1)
usermods/usermod_v2_departstrip/departure_view.h (1)
106-111: Harden configKey_ sanitization (avoid breaking addInfo()/JS and odd keys).Extend replacements to cover CR/LF/TAB/slash and normalize to [A-Za-z0-9_].
void updateConfigKey_() { String s = String("DepartureView_") + keysStr_; - s.replace(':', '_'); s.replace(',', '_'); s.replace(' ', '_'); s.replace(';', '_'); - s.replace('\'', '_'); s.replace('\"', '_'); s.replace('\\', '_'); + s.replace(':','_'); s.replace(',','_'); s.replace(' ','_'); s.replace(';','_'); + s.replace('\'','_'); s.replace('\"','_'); s.replace('\\','_'); s.replace('/','_'); + s.replace('\n','_'); s.replace('\r','_'); s.replace('\t','_'); + for (uint16_t i = 0; i < s.length(); ++i) { + char c = s.charAt(i); + bool ok = (c=='_') || (c>='0'&&c<='9') || (c>='A'&&c<='Z') || (c>='a'&&c<='z'); + if (!ok) s.setCharAt(i,'_'); + } configKey_ = std::string(s.c_str()); }
🧹 Nitpick comments (5)
usermods/usermod_v2_departstrip/util.h (2)
21-27: Guard fmt_local against strftime failure (ensure null-termination).strftime returns 0 on failure/insufficient buffer; out may be left unspecified. Cheap safety: clear on failure.
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(); struct tm tmLocal; gmtime_r(&local_sec, &tmLocal); - strftime(out, n, fmt, &tmLocal); + if (!out || n == 0) return; + size_t wrote = strftime(out, n, fmt, &tmLocal); + if (wrote == 0) out[0] = '\0'; }
14-19: Use fixed-width type for offset to avoid signed-width surprises across platforms.Use int32_t for the bounded +/-15h offset; it aligns with the clamp and avoids long/time_t width ambiguities.
-static constexpr long kMaxOffsetSec = 15L * 3600L; // +/-15h safety clamp -inline long current_offset() { - long off = (long)localTime - (long)toki.getTime().sec; +static constexpr int32_t kMaxOffsetSec = 15 * 3600; // +/-15h safety clamp +inline int32_t current_offset() { + int32_t off = (int32_t)((int64_t)localTime - (int64_t)toki.getTime().sec);usermods/usermod_v2_departstrip/departure_view.cpp (2)
117-121: Prefer natural sort for cycling order to match route semantics (e.g., 9 < 10 < 10A).Use util::cmpLineRefNatural instead of strcmp for stable, intuitive ordering.
- std::sort(strong_.begin(), strong_.end(), [](const Cand& a, const Cand& b){ - const char* ak = (a.key && a.key->length()) ? a.key->c_str() : ""; - const char* bk = (b.key && b.key->length()) ? b.key->c_str() : ""; - return strcmp(ak, bk) < 0; - }); + std::sort(strong_.begin(), strong_.end(), [](const Cand& a, const Cand& b){ + const String& as = (a.key && a.key->length()) ? *a.key : String(); + const String& bs = (b.key && b.key->length()) ? *b.key : String(); + return departstrip::util::cmpLineRefNatural(as, bs) < 0; + });
81-103: Hot-path complexity: O(LEDs × items) each frame.If performance becomes tight on long strips or many items, precompute per-item pixel position once per frame and only visit nearby pixels.
usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp (1)
389-391: Propagate readFromConfig success status instead of always returning true.Returning true unconditionally hides parse errors. Consider returning ok (and still persist partial state).
- // Always report success to persist changes - return true; + // Report actual status; config will still persist partial changes above + return ok;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
usermods/usermod_v2_departstrip/departure_view.cpp(1 hunks)usermods/usermod_v2_departstrip/departure_view.h(1 hunks)usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp(1 hunks)usermods/usermod_v2_departstrip/util.h(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ksedgwic
PR: wled/WLED#4919
File: usermods/usermod_v2_departstrip/depart_model.cpp:164-187
Timestamp: 2025-09-09T23:14:06.323Z
Learning: In the DepartStrip usermod, it's intentionally valid to have different colors for different directions (e.g., "72M-N" vs "72M-S"), even though they may map to the same color values in the current implementation.
🧬 Code graph analysis (4)
usermods/usermod_v2_departstrip/util.h (1)
usermods/usermod_v2_departstrip/util.cpp (8)
hsv2rgb(6-24)hsv2rgb(6-6)cmpLineRefNatural(36-52)cmpLineRefNatural(36-36)getDisplayMinutes(56-56)getDisplayMinutes(56-56)setDisplayMinutes(57-62)setDisplayMinutes(57-57)
usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp (7)
usermods/usermod_v2_departstrip/usermod_v2_departstrip.h (2)
DepartStrip(15-48)setup(33-39)usermods/usermod_v2_departstrip/util.cpp (4)
setDisplayMinutes(57-62)setDisplayMinutes(57-57)cmpLineRefNatural(36-52)cmpLineRefNatural(36-36)usermods/usermod_v2_departstrip/util.h (1)
time_now_utc(11-11)usermods/usermod_v2_departstrip/departure_view.cpp (4)
view(15-139)view(15-15)appendConfigData(141-165)appendConfigData(141-141)usermods/usermod_v2_departstrip/departure_view.h (3)
addToConfig(34-38)appendConfigData(39-39)readFromConfig(42-63)usermods/usermod_v2_departstrip/siri_source.cpp (5)
addToConfig(421-429)addToConfig(421-421)k(356-356)readFromConfig(431-466)readFromConfig(431-431)usermods/usermod_v2_departstrip/depart_model.cpp (14)
colorMap(122-124)colorMap(122-122)colorToString(205-209)colorToString(205-205)getColorRGB(155-178)getColorRGB(155-155)clearColorMap(126-128)clearColorMap(126-126)removeColorKey(137-140)removeColorKey(137-137)parseColorString(180-203)parseColorString(180-180)setColorRGB(142-148)setColorRGB(142-142)
usermods/usermod_v2_departstrip/departure_view.cpp (3)
usermods/usermod_v2_departstrip/depart_model.cpp (2)
getColorRGB(155-178)getColorRGB(155-155)usermods/usermod_v2_departstrip/departure_view.h (1)
segmentForId_(113-118)usermods/usermod_v2_departstrip/util.cpp (2)
getDisplayMinutes(56-56)getDisplayMinutes(56-56)
usermods/usermod_v2_departstrip/departure_view.h (3)
usermods/usermod_v2_departstrip/departure_view.cpp (6)
view(15-139)view(15-15)appendConfigData(141-165)appendConfigData(141-141)ensureUnfrozen(167-173)ensureUnfrozen(167-167)usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp (6)
addToConfig(89-163)addToConfig(89-89)appendConfigData(165-209)appendConfigData(165-165)readFromConfig(211-391)readFromConfig(211-211)usermods/usermod_v2_departstrip/siri_source.cpp (6)
addToConfig(421-429)addToConfig(421-421)readFromConfig(431-466)readFromConfig(431-431)in(45-45)k(356-356)
🔇 Additional comments (3)
usermods/usermod_v2_departstrip/departure_view.cpp (1)
64-65: Confirm whether Segment::beginDraw() requires a matching endDraw().If WLED expects endDraw(), wrap the render loop; otherwise, this is fine. Please verify against current WLED segment API.
usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp (1)
139-161: Nice: natural sort for ColorMap output aligns with user expectations.Sorting by agency and cmpLineRefNatural is a good touch for UI readability.
usermods/usermod_v2_departstrip/departure_view.h (1)
68-104: Parsing looks robust; good call on updating agencyHint per fully-qualified token.Delimiters and CR handling are correct; avoids misbinding bare tokens.
| } | ||
| if (!lines.empty()) { | ||
| s.print(F("<div style=\\'font-weight:600;margin-bottom:4px;\\'>Routes</div>")); | ||
| s.print(F("<div style=\\'display:flex;flex-wrap:wrap;gap:8px;justify-content:center;\\'>")); | ||
| for (const auto& line : lines) { | ||
| uint32_t rgb = 0x606060; | ||
| if (agency.length() > 0) DepartModel::getColorRGB(agency, line, rgb); | ||
| String col = DepartModel::colorToString(rgb); | ||
| s.print(F("<span style=\\'display:inline-flex;align-items:center;border:1px solid #888;padding:2px 6px;border-radius:4px;\\'>")); | ||
| s.print(F("<span style=\\'display:inline-block;width:14px;height:14px;margin-right:6px;background:")); | ||
| s.print(col); | ||
| s.print(F(";\\'></span><code>")); | ||
| s.print(line); | ||
| s.print(F("</code></span>")); | ||
| } | ||
| s.print(F("</div>")); | ||
| } | ||
| s.print(F("</div>")); | ||
| s.println(F("','');")); | ||
| } |
There was a problem hiding this comment.
Escape route/line text in generated HTML (mirror fix in DepartureView).
Same injection surface here; escape before printing.
- for (const auto& line : lines) {
+ for (const auto& line : lines) {
uint32_t rgb = 0x606060;
if (agency.length() > 0) DepartModel::getColorRGB(agency, line, rgb);
String col = DepartModel::colorToString(rgb);
s.print(F("<span style=\\'display:inline-flex;align-items:center;border:1px solid #888;padding:2px 6px;border-radius:4px;\\'>"));
s.print(F("<span style=\\'display:inline-block;width:14px;height:14px;margin-right:6px;background:"));
s.print(col);
- s.print(F(";\\'></span><code>"));
- s.print(line);
- s.print(F("</code></span>"));
+ s.print(F(";\\'></span><code>"));
+ String le = line;
+ le.replace("&","&");
+ le.replace("<","<"); le.replace(">",">");
+ le.replace("\\","\\\\"); le.replace("'","\\'");
+ s.print(le);
+ s.print(F("</code></span>"));
}📝 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.
| } | |
| if (!lines.empty()) { | |
| s.print(F("<div style=\\'font-weight:600;margin-bottom:4px;\\'>Routes</div>")); | |
| s.print(F("<div style=\\'display:flex;flex-wrap:wrap;gap:8px;justify-content:center;\\'>")); | |
| for (const auto& line : lines) { | |
| uint32_t rgb = 0x606060; | |
| if (agency.length() > 0) DepartModel::getColorRGB(agency, line, rgb); | |
| String col = DepartModel::colorToString(rgb); | |
| s.print(F("<span style=\\'display:inline-flex;align-items:center;border:1px solid #888;padding:2px 6px;border-radius:4px;\\'>")); | |
| s.print(F("<span style=\\'display:inline-block;width:14px;height:14px;margin-right:6px;background:")); | |
| s.print(col); | |
| s.print(F(";\\'></span><code>")); | |
| s.print(line); | |
| s.print(F("</code></span>")); | |
| } | |
| s.print(F("</div>")); | |
| } | |
| s.print(F("</div>")); | |
| s.println(F("','');")); | |
| } | |
| } | |
| if (!lines.empty()) { | |
| s.print(F("<div style=\\'font-weight:600;margin-bottom:4px;\\'>Routes</div>")); | |
| s.print(F("<div style=\\'display:flex;flex-wrap:wrap;gap:8px;justify-content:center;\\'>")); | |
| for (const auto& line : lines) { | |
| uint32_t rgb = 0x606060; | |
| if (agency.length() > 0) DepartModel::getColorRGB(agency, line, rgb); | |
| String col = DepartModel::colorToString(rgb); | |
| s.print(F("<span style=\\'display:inline-flex;align-items:center;border:1px solid #888;padding:2px 6px;border-radius:4px;\\'>")); | |
| s.print(F("<span style=\\'display:inline-block;width:14px;height:14px;margin-right:6px;background:")); | |
| s.print(col); | |
| s.print(F(";\\'></span><code>")); | |
| String le = line; | |
| le.replace("&","&"); | |
| le.replace("<","<"); le.replace(">",">"); | |
| le.replace("\\","\\\\"); le.replace("'","\\'"); | |
| s.print(le); | |
| s.print(F("</code></span>")); | |
| } | |
| s.print(F("</div>")); | |
| } | |
| s.print(F("</div>")); | |
| s.println(F("','');")); | |
| } |
🤖 Prompt for AI Agents
In usermods/usermod_v2_departstrip/usermod_v2_departstrip.cpp around lines
189-208, the route/line text is inserted directly into generated HTML (printed
inside the <code> element) which allows injection; escape the line string before
printing it. Add or call an HTML-escape helper (e.g., escapeHtml or HtmlEncode)
to replace &, <, >, " and ' in the line value and use the escaped result for
s.print(line) (and any other place the raw line is output into HTML), ensuring
the printed color and other logic still use the original value where needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
usermods/usermod_v2_departstrip/departure_view.cpp (4)
41-41: Release freeze when no sources to avoid stale frozen segment.Early-return on empty sources leaves the previous freeze state intact. Unfreeze here so the segment can resume normal rendering when data disappears.
Apply this diff:
- if (sources_.empty()) return; + if (sources_.empty()) { ensureUnfrozen(); return; }
79-82: Defensive guard against zero/invalid display window.If minutes were ever 0 (misconfig or uninitialized), this divides by zero. Add a cheap guard.
Apply this diff:
- uint16_t mins = departstrip::util::getDisplayMinutes(); - float windowSec = (float)mins * 60.0f; + uint16_t mins = departstrip::util::getDisplayMinutes(); + float windowSec = (float)mins * 60.0f; + if (windowSec <= 0.0f) continue; // safety: skip degenerate config
112-116: Use natural sort for line refs for stable human ordering.Lexicographic strcmp will place "100" before "72" and ignore numeric intent. You already have cmpLineRefNatural(); use it here with null-safety.
Apply this diff:
- std::sort(strong_.begin(), strong_.end(), [](const Cand& a, const Cand& b){ - const char* ak = (a.key && a.key->length()) ? a.key->c_str() : ""; - const char* bk = (b.key && b.key->length()) ? b.key->c_str() : ""; - return strcmp(ak, bk) < 0; - }); + std::sort(strong_.begin(), strong_.end(), [](const Cand& a, const Cand& b){ + // Null/empty keys sort last; otherwise use natural order (e.g., "10" < "100" < "100A"). + if (!a.key || a.key->length() == 0) return false; + if (!b.key || b.key->length() == 0) return true; + return departstrip::util::cmpLineRefNatural(*a.key, *b.key) < 0; + });
124-128: Cycle speed can become unreadably fast with many overlaps.With n > 1000, slice becomes 1 ms; even with dozens of overlaps the cycle flickers. Use a fixed full-cycle duration with a minimum slice.
Apply this diff:
- uint32_t n32 = (uint32_t)n; - uint32_t slice = 1000u / n32; // total cycle ~1s; clamp to >=1ms - if (slice == 0) slice = 1; + uint32_t n32 = (uint32_t)n; + const uint32_t totalCycleMs = 2000u; // ~2s per full cycle + uint32_t slice = (n32 > 0) ? (totalCycleMs / n32) : 0; + if (slice < 75u) slice = 75u; // avoid sub‑frame strobingusermods/usermod_v2_departstrip/util.h (1)
69-75: Handle strftime failure to guarantee null-termination.strftime can return 0 on failure; ensure the buffer is explicitly terminated.
Apply this diff:
gmtime_r(&local_sec, &tmLocal); - strftime(out, n, fmt, &tmLocal); + if (!strftime(out, n, fmt, &tmLocal) && n > 0) out[0] = '\0';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
usermods/usermod_v2_departstrip/departure_view.cpp(1 hunks)usermods/usermod_v2_departstrip/departure_view.h(1 hunks)usermods/usermod_v2_departstrip/util.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- usermods/usermod_v2_departstrip/departure_view.h
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ksedgwic
PR: wled/WLED#4919
File: usermods/usermod_v2_departstrip/depart_model.cpp:164-187
Timestamp: 2025-09-09T23:14:06.323Z
Learning: In the DepartStrip usermod, it's intentionally valid to have different colors for different directions (e.g., "72M-N" vs "72M-S"), even though they may map to the same color values in the current implementation.
🧬 Code graph analysis (2)
usermods/usermod_v2_departstrip/departure_view.cpp (3)
usermods/usermod_v2_departstrip/depart_model.cpp (2)
getColorRGB(155-178)getColorRGB(155-155)usermods/usermod_v2_departstrip/util.cpp (2)
getDisplayMinutes(56-56)getDisplayMinutes(56-56)usermods/usermod_v2_departstrip/departure_view.h (2)
appendConfigData(38-38)configKey(63-63)
usermods/usermod_v2_departstrip/util.h (1)
usermods/usermod_v2_departstrip/util.cpp (8)
hsv2rgb(6-24)hsv2rgb(6-6)cmpLineRefNatural(36-52)cmpLineRefNatural(36-36)getDisplayMinutes(56-56)getDisplayMinutes(56-56)setDisplayMinutes(57-62)setDisplayMinutes(57-57)
🔇 Additional comments (4)
usermods/usermod_v2_departstrip/departure_view.cpp (2)
8-13: Helper looks good; relies on DepartModel defaults appropriately.The RGB extraction and CRGB construction are correct; defers unknowns to model defaults as intended.
59-59: No matching endDraw required — keep seg.beginDraw()Verified: Segment::beginDraw(...) is declared in wled00/FX.h and defined in wled00/FX_fcn.cpp; there is no endDraw() in the tree and callers use beginDraw without a paired end, so no endDraw call is needed here.
usermods/usermod_v2_departstrip/util.h (2)
17-27: Freeze handle behavior is sound and reentrant.Validates segId, preserves prior freeze state, and is idempotent across frames. Good utility.
Also applies to: 44-50
62-67: Offset clamp looks right; prevents wild localTime drifts.The +/-15h safety bound is sensible for DST/misconfig glitches.
DepartStrip
Render upcoming departures from SIRI StopMonitoring feeds on WLED LED segments.
SIRI stands for “Service Interface for Real Time Information” — a CEN/Transmodel family standard for exchanging real‑time public transport data. StopMonitoring is the SIRI service that returns predicted arrivals/departures at a stop.
Feed Examples:
511.org StopMonitoring (Bay Area)
http://api.511.org/transit/StopMonitoring?format=json&api_key=KEY&agency=AGENCY&stopCode=STOPBA), AC Transit (AC), and many others across the nine‑county region.MTA Bus Time StopMonitoring (NYC)
http://bustime.mta.info/api/siri/stop-monitoring.json?key=KEY&OperatorRef=MTA&MonitoringRef=STOPincluding local, limited, SBS, and express).
Summary by CodeRabbit
New Features
New APIs / Interfaces
Utilities
Documentation
Chores