Usermod: Add BARTdepart transit departure usermod #4884
Conversation
WalkthroughAdds a new BartDepart usermod: data interfaces, a LegacyBartSource HTTPS fetcher, a BartStationModel with per‑platform ETD history and merging, PlatformView renderers, utilities (time/color/hsv), TrainColor helpers, orchestration code, documentation, and a library manifest. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (29)
usermods/usermod_v2_bartdepart/library.json (1)
1-4: Optional: add metadata for clarity.
Consider adding "version", "description", and "keywords" to help downstream tooling.usermods/usermod_v2_bartdepart/train_color.h (1)
3-11: Use a compact underlying type for the enum.
Saves RAM/flash in switch tables and structures.-enum class TrainColor { +enum class TrainColor : uint8_t {usermods/usermod_v2_bartdepart/util.cpp (2)
1-31: Include math header and favor portable rounding.
fmodf/fabsf/roundfneed<math.h>on some cores;lrintfisn’t available everywhere.-#include "util.h" +#include "util.h" +#include <math.h> @@ - uint8_t r = uint8_t(lrintf(rf * 255.f)); - uint8_t g = uint8_t(lrintf(gf * 255.f)); - uint8_t b = uint8_t(lrintf(bf * 255.f)); + uint8_t r = uint8_t(roundf(rf * 255.f)); + uint8_t g = uint8_t(roundf(gf * 255.f)); + uint8_t b = uint8_t(roundf(bf * 255.f));
6-31: Consider reusing existing WLED/FastLED HSV utilities.
If acceptable,CRGB color; color.setHSV(h8, s8, v8);avoids custom float math and reduces code size.usermods/usermod_v2_bartdepart/legacy_bart_source.cpp (4)
23-30: Add socket timeouts before begin().
Prevents hangs on bad networks.String url = composeUrl(apiBase_, apiKey_, apiStation_); + client_.setTimeout(5000); // ms if (!https_.begin(client_, url)) {
41-51: Reduce heap churn: parse JSON from stream instead of copying to String.
Avoids duplicating payload and lowers peak RAM.- String payload = https_.getString(); - https_.end(); - - size_t jsonSz = payload.length() * 2; - DynamicJsonDocument doc(jsonSz); - auto err = deserializeJson(doc, payload); + // Parse directly from the HTTP stream + DynamicJsonDocument doc(8192); // tune; or compute via ArduinoJson Assistant + auto err = deserializeJson(doc, https_.getStream()); + https_.end();
80-87: Only invalidate history when config actually changes.
Right now, any read wipes history. Compare old/new values first.-bool LegacyBartSource::readFromConfig(JsonObject& root, bool startup_complete, bool& invalidate_history) { +bool LegacyBartSource::readFromConfig(JsonObject& root, bool startup_complete, bool& invalidate_history) { bool ok = true; - ok &= getJsonValue(root["UpdateSecs"], updateSecs_, 60); - ok &= getJsonValue(root["ApiBase"], apiBase_, apiBase_); - ok &= getJsonValue(root["ApiKey"], apiKey_, apiKey_); - ok &= getJsonValue(root["ApiStation"], apiStation_, apiStation_); - invalidate_history = true; + uint16_t oldUpd = updateSecs_; String oldBase = apiBase_; String oldKey = apiKey_; String oldSt = apiStation_; + ok &= getJsonValue(root["UpdateSecs"], updateSecs_, updateSecs_); + ok &= getJsonValue(root["ApiBase"], apiBase_, apiBase_); + ok &= getJsonValue(root["ApiKey"], apiKey_, apiKey_); + ok &= getJsonValue(root["ApiStation"], apiStation_, apiStation_); + invalidate_history = (oldUpd != updateSecs_) || (oldBase != apiBase_) || (oldKey != apiKey_) || (oldSt != apiStation_); return ok; }
13-18: URL-encode and normalize station code.
IfapiStation_contains spaces or lowercase, encode/uppercase to avoid API surprises.usermods/usermod_v2_bartdepart/readme.md (2)
34-40: Clarify API key usage and rate limits.
Explicitly warn that the demo key is rate‑limited and not for production; suggest per‑user keys.Apply:
-- ApiKey: API key. The default demo key works for testing; consider providing - your own key for reliability. +- ApiKey: API key. The default demo key is rate‑limited and for testing only. + For reliable operation, obtain and use your own key.
45-52: Tighten phrasing around virtual segments and overlap.
Minor clarity edit; avoids ambiguity about “mapping settings”.Apply:
-- Virtual Segments: Rendering uses `virtualLength()` and `beginDraw()`, so - segment reverse/mirror/group settings are respected automatically. If the - displayed direction seems wrong, toggle the segment’s Reverse or adjust - mapping settings in WLED. +- Virtual Segments: Rendering uses `virtualLength()` and `beginDraw()`, so + Reverse/Mirror/Grouping are respected. If direction appears wrong, toggle + the segment’s Reverse setting in WLED.usermods/usermod_v2_bartdepart/legacy_bart_source.h (2)
37-39: Avoid per-call allocations for platformIds().
Return a static array or String literals to reduce heap churn on MCUs.Apply:
- std::vector<String> platformIds() const { return { "1", "2", "3", "4" }; } + const std::array<const char*,4>& platformIds() const { + static const std::array<const char*,4> kP = {"1","2","3","4"}; + return kP; + }Adjust call sites accordingly.
17-18: Consider widening UpdateSecs datatype or validating upper bounds.
uint16_t is fine for defaults, but add input validation (e.g., 5–3600) to prevent excessive backoff intervals.usermods/usermod_v2_bartdepart/platform_view.cpp (4)
47-50: Frame gating should be time‑based, not call‑count‑based.
A static frame counter desyncs across PlatformViews and depends on render rate. Use millis() to toggle the preference window.Apply:
- static uint8_t frameCnt = 0; // 0..99 - bool preferFirst = frameCnt < 50; // true for first 0.5 s - frameCnt = (frameCnt + 1) % 100; + bool preferFirst = ((millis() / 500) & 1) == 0; // alternate every 0.5s
73-89: Const‑correctness and micro‑allocations in inner loop.
Iterate by const reference to avoid copies.Apply:
- for (auto &e : batch.etds) { + for (const auto& e : batch.etds) {
90-94: Prefer WLED helpers for color packing (if available).
Manual bit‑packing is fine; if WLED provides a packColor(r,g,b), use it for consistency.
95-100: Selection rule can be simplified and documented.
The 2× heuristic is clever; add a comment explaining the intent (bias early/late frames). Also guard bestB to avoid redundant multiplications.Apply:
- if ((preferFirst && (bestColor == 0 || newB > 2 * bestB)) || - (!preferFirst && newB * 2 >= bestB)) { + // Prefer brighter color; bias toggles every 0.5s via preferFirst + if ((preferFirst && (bestColor == 0 || newB > (bestB << 1))) || + (!preferFirst && (newB << 1) >= bestB)) {usermods/usermod_v2_bartdepart/platform_view.h (3)
9-9: Decouple render offset from data source interval or make it configurable.
updateSecs_ here is a render offset but isn’t exposed in config; it can diverge from LegacyBartSource::UpdateSecs. Either rename to renderOffsetSecs_ and expose it, or plumb the data source’s interval into the view.Apply:
- uint16_t updateSecs_ = 60; + uint16_t renderOffsetSecs_ = 60; // (-) rename for intent; optionally expose in configAnd update usage in .cpp.
20-25: Config I/O: validate SegmentId bounds.
Clamp to [-1, strip.getMaxSegments()-1] during read to avoid invalid indices.Apply:
- return getJsonValue(root["SegmentId"], segmentId_, segmentId_); + bool ok = getJsonValue(root["SegmentId"], segmentId_, segmentId_); + if (segmentId_ < -1) segmentId_ = -1; + if (segmentId_ >= (int16_t)strip.getMaxSegments()) segmentId_ = -1; + return ok;
26-28: Minor: avoid repeated c_str() lifetime pitfalls.
configKey_ is stable, but exposing a std::string and const char* can confuse callers. Consider returning std::string_view (if available) or only const char*.usermods/usermod_v2_bartdepart/bart_station_model.h (3)
23-31: Document history retention and batch semantics.
Clarify max history length and ordering guarantees so renderers can rely on history.back() being the newest and sorted ETDs.Add comment:
explicit Platform(const String& platformId); void update(const JsonObject& root); void merge(const Platform& other); time_t oldest() const; const String& platformId() const; const std::deque<ETDBatch>& history() const; const std::vector<String>& destinations() const; String toString() const; + // Note: history_ retains up to N recent batches (newest at back), ETDs sorted by estDep.
38-40: Timezone note for parseHeaderTimestamp.
Confirm input is UTC and explicitly state that conversions don’t apply DST/local time to avoid ambiguity.
44-47: destinationsForPlatform: consider stable ordering and de‑dupe policy.
If ordering is user‑visible, define it (alpha vs. first‑seen). Also ensure de‑dupe is case‑insensitive if API case varies.usermods/usermod_v2_bartdepart/interfaces.h (1)
29-36: Minor API polish: method naming and return type.
- Consider renaming
checkhistory→checkHistoryfor consistency.- Returning
const char*(orString) fromname()avoids dynamic allocations fromstd::stringon MCUs.usermods/usermod_v2_bartdepart/util.h (2)
11-19: Clarify FreezeGuard behavior.Comment says “freeze during rendering,” but it’s also used to temporarily unfreeze (
freezeNow=false). Reword to reflect “set to provided value, then restore”.Apply this diff:
-// RAII guard to temporarily freeze a segment during rendering and always -// restore its original freeze state on exit (including early returns). +// RAII guard to temporarily set seg.freeze to the provided value and always +// restore its original state on exit (including early returns).
80-84: Avoid function-local static with runtime init.
hsv2rgb()call in a function-localstaticadds guards and code size. Use a literal debug color.Apply this diff:
- static const uint32_t dbgCol = hsv2rgb(0.f, 0.f, 0.4f); + constexpr uint32_t dbgCol = RGBW32(102, 102, 102, 0); // ~0.4 whiteusermods/usermod_v2_bartdepart/usermod_v2_bartdepart.cpp (1)
78-83: Skip overlay draw when disabled/off.Avoid unnecessary work and pixel writes when the mod is disabled or WLED is off.
Apply this diff:
void BartDepart::handleOverlayDraw() { - time_t now = bartdepart::util::time_now_utc(); + if (!enabled_ || offMode) return; + time_t now = bartdepart::util::time_now_utc(); for (auto& view : views_) { view->view(now, *model_, dbgPixelIndex_); } }usermods/usermod_v2_bartdepart/usermod_v2_bartdepart.h (1)
46-48: Const-correct accessor.
isEnabled()can beconst.Apply this diff:
- inline bool isEnabled() { return enabled_; } + inline bool isEnabled() const { return enabled_; }usermods/usermod_v2_bartdepart/bart_station_model.cpp (2)
29-33: Handle non-numeric “minutes” robustly.BART can return “Leaving”. Guard before
atoito avoid accidental negatives/UB if format changes.Apply this diff:
- int mins = atoi(est["minutes"] | "0"); + const char* mstr = est["minutes"] | "0"; + int mins = (mstr && mstr[0] >= '0' && mstr[0] <= '9') ? atoi(mstr) : 0;
48-55: Avoid storing empty ETD batches.Skip appending when no ETDs matched to reduce noise and save memory.
Apply this diff:
- history_.push_back(std::move(batch)); - while (history_.size() > 5) { - history_.pop_front(); - } + if (!batch.etds.empty()) { + history_.push_back(std::move(batch)); + while (history_.size() > 5) { + history_.pop_front(); + } + } - DEBUG_PRINTF("BartDepart::update platform %s: %s\n", - platformId_.c_str(), toString().c_str()); + DEBUG_PRINTF("BartDepart::update platform %s: %s\n", + platformId_.c_str(), toString().c_str());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
usermods/usermod_v2_bartdepart/bart_station_model.cpp(1 hunks)usermods/usermod_v2_bartdepart/bart_station_model.h(1 hunks)usermods/usermod_v2_bartdepart/interfaces.h(1 hunks)usermods/usermod_v2_bartdepart/legacy_bart_source.cpp(1 hunks)usermods/usermod_v2_bartdepart/legacy_bart_source.h(1 hunks)usermods/usermod_v2_bartdepart/library.json(1 hunks)usermods/usermod_v2_bartdepart/platform_view.cpp(1 hunks)usermods/usermod_v2_bartdepart/platform_view.h(1 hunks)usermods/usermod_v2_bartdepart/readme.md(1 hunks)usermods/usermod_v2_bartdepart/train_color.cpp(1 hunks)usermods/usermod_v2_bartdepart/train_color.h(1 hunks)usermods/usermod_v2_bartdepart/usermod_v2_bartdepart.cpp(1 hunks)usermods/usermod_v2_bartdepart/usermod_v2_bartdepart.h(1 hunks)usermods/usermod_v2_bartdepart/util.cpp(1 hunks)usermods/usermod_v2_bartdepart/util.h(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-07T15:58:42.579Z
Learnt from: willmmiles
PR: wled/WLED#4712
File: wled00/FX_fcn.cpp:992-993
Timestamp: 2025-06-07T15:58:42.579Z
Learning: In WLED segments, the backing pixel buffer is always allocated to the full physical segment size, but effect rendering only uses a subset when mirror or grouping are enabled. For 2D segments, this subset is vWidth() * vHeight(), and for 1D segments it's vLength(). Color processing operations like fade_out should iterate over this rendering subset, not the full allocated buffer.
Applied to files:
usermods/usermod_v2_bartdepart/util.h
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
usermods/usermod_v2_bartdepart/util.h
🧬 Code graph analysis (11)
usermods/usermod_v2_bartdepart/train_color.h (2)
usermods/usermod_v2_bartdepart/bart_station_model.cpp (2)
toString(87-121)toString(87-87)usermods/usermod_v2_bartdepart/train_color.cpp (2)
parseTrainColor(5-13)parseTrainColor(5-5)
usermods/usermod_v2_bartdepart/legacy_bart_source.h (3)
usermods/usermod_v2_bartdepart/legacy_bart_source.cpp (9)
LegacyBartSource(4-6)fetch(20-71)fetch(20-20)reload(8-11)reload(8-8)addToConfig(73-78)addToConfig(73-73)readFromConfig(80-88)readFromConfig(80-80)usermods/usermod_v2_bartdepart/interfaces.h (1)
std(50-58)usermods/usermod_v2_bartdepart/usermod_v2_bartdepart.cpp (4)
addToConfig(85-97)addToConfig(85-85)readFromConfig(109-135)readFromConfig(109-109)
usermods/usermod_v2_bartdepart/usermod_v2_bartdepart.h (7)
usermods/usermod_v2_bartdepart/legacy_bart_source.cpp (5)
LegacyBartSource(4-6)addToConfig(73-78)addToConfig(73-73)readFromConfig(80-88)readFromConfig(80-80)usermods/usermod_v2_bartdepart/legacy_bart_source.h (1)
LegacyBartSource(15-39)usermods/usermod_v2_bartdepart/platform_view.h (4)
PlatformView(7-28)addToConfig(20-20)appendConfigData(21-21)readFromConfig(23-25)usermods/usermod_v2_bartdepart/usermod_v2_bartdepart.cpp (19)
BartDepart(23-30)setup(32-39)setup(32-32)loop(41-76)loop(41-41)handleOverlayDraw(78-83)handleOverlayDraw(78-78)addToConfig(85-97)addToConfig(85-85)appendConfigData(99-107)appendConfigData(99-99)readFromConfig(109-135)readFromConfig(109-109)showBooting(137-145)showBooting(137-137)doneBooting(147-151)doneBooting(147-147)reloadSources(153-155)reloadSources(153-153)usermods/usermod_v2_bartdepart/interfaces.h (1)
std(50-58)usermods/usermod_v2_bartdepart/platform_view.cpp (2)
appendConfigData(107-130)appendConfigData(107-107)usermods/usermod_v2_bartdepart/util.h (1)
s(13-19)
usermods/usermod_v2_bartdepart/interfaces.h (5)
usermods/usermod_v2_bartdepart/util.h (1)
s(13-19)usermods/usermod_v2_bartdepart/legacy_bart_source.cpp (4)
fetch(20-71)fetch(20-20)reload(8-11)reload(8-8)usermods/usermod_v2_bartdepart/platform_view.cpp (4)
view(25-105)view(25-26)appendConfigData(107-130)appendConfigData(107-107)usermods/usermod_v2_bartdepart/platform_view.h (2)
appendDebugPixel(19-19)appendConfigData(21-21)usermods/usermod_v2_bartdepart/usermod_v2_bartdepart.cpp (2)
appendConfigData(99-107)appendConfigData(99-99)
usermods/usermod_v2_bartdepart/legacy_bart_source.cpp (2)
usermods/usermod_v2_bartdepart/legacy_bart_source.h (1)
LegacyBartSource(15-39)usermods/usermod_v2_bartdepart/usermod_v2_bartdepart.cpp (4)
addToConfig(85-97)addToConfig(85-85)readFromConfig(109-135)readFromConfig(109-109)
usermods/usermod_v2_bartdepart/platform_view.h (4)
usermods/usermod_v2_bartdepart/bart_station_model.cpp (2)
platformId(74-76)platformId(74-74)usermods/usermod_v2_bartdepart/platform_view.cpp (4)
view(25-105)view(25-26)appendConfigData(107-130)appendConfigData(107-107)usermods/usermod_v2_bartdepart/util.h (1)
s(13-19)usermods/usermod_v2_bartdepart/usermod_v2_bartdepart.cpp (6)
addToConfig(85-97)addToConfig(85-85)appendConfigData(99-107)appendConfigData(99-99)readFromConfig(109-135)readFromConfig(109-109)
usermods/usermod_v2_bartdepart/util.h (1)
usermods/usermod_v2_bartdepart/util.cpp (2)
hsv2rgb(6-32)hsv2rgb(6-6)
usermods/usermod_v2_bartdepart/bart_station_model.h (1)
usermods/usermod_v2_bartdepart/bart_station_model.cpp (2)
platformId(74-76)platformId(74-74)
usermods/usermod_v2_bartdepart/bart_station_model.cpp (3)
usermods/usermod_v2_bartdepart/util.h (2)
time_now(26-26)current_offset(28-33)usermods/usermod_v2_bartdepart/train_color.cpp (2)
parseTrainColor(5-13)parseTrainColor(5-5)usermods/usermod_v2_bartdepart/train_color.h (1)
toString(13-23)
usermods/usermod_v2_bartdepart/usermod_v2_bartdepart.cpp (5)
usermods/usermod_v2_bartdepart/usermod_v2_bartdepart.h (2)
BartDepart(21-53)setup(38-44)usermods/usermod_v2_bartdepart/util.h (2)
time_now_utc(24-24)s(13-19)usermods/usermod_v2_bartdepart/platform_view.cpp (4)
view(25-105)view(25-26)appendConfigData(107-130)appendConfigData(107-107)usermods/usermod_v2_bartdepart/legacy_bart_source.cpp (4)
addToConfig(73-78)addToConfig(73-73)readFromConfig(80-88)readFromConfig(80-80)usermods/usermod_v2_bartdepart/platform_view.h (3)
addToConfig(20-20)appendConfigData(21-21)readFromConfig(23-25)
usermods/usermod_v2_bartdepart/platform_view.cpp (4)
usermods/usermod_v2_bartdepart/bart_station_model.cpp (2)
history(78-81)history(79-79)usermods/usermod_v2_bartdepart/util.h (3)
c(63-70)blinkDebug(79-86)s(13-19)usermods/usermod_v2_bartdepart/platform_view.h (2)
appendConfigData(21-21)configKey(26-26)usermods/usermod_v2_bartdepart/usermod_v2_bartdepart.cpp (2)
appendConfigData(99-107)appendConfigData(99-99)
🪛 LanguageTool
usermods/usermod_v2_bartdepart/readme.md
[grammar] ~4-~4: There might be a mistake here.
Context: ...t; colored markers sweep toward “now” as departure times approach. ## Features ...
(QB_NEW_EN)
[grammar] ~9-~9: There might be a mistake here.
Context: ... (reverse, mirroring, grouping honored). - Configurable per‑platform segment mappin...
(QB_NEW_EN)
[grammar] ~10-~10: There might be a mistake here.
Context: ...mapping; disable any platform with -1. - Graceful overlay behavior (no freezing t...
(QB_NEW_EN)
[grammar] ~16-~16: There might be a mistake here.
Context: ...epartto your PlatformIO environment inplatformio_override.ini`: ``` custom_u...
(QB_NEW_EN)
[grammar] ~23-~23: There might be a mistake here.
Context: ...artdepart ``` Build and flash as usual: - Build: pio run -e <your_env> - Upload:...
(QB_NEW_EN)
[grammar] ~24-~24: There might be a mistake here.
Context: ...``` Build and flash as usual: - Build: pio run -e <your_env> - Upload: `pio run -e <your_env> -t upload...
(QB_NEW_EN)
[grammar] ~29-~29: There might be a mistake here.
Context: ...s → “BartDepart”. The following keys are available. - BartDepart.Enabled: enable...
(QB_NEW_EN)
[grammar] ~38-~38: There might be a mistake here.
Context: ...roviding your own key for reliability. - ApiStation: station abbreviation (e.g., ...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ...). Views: PlatformView1 … PlatformView4 - SegmentId: WLED segment index to render ...
(QB_NEW_EN)
[grammar] ~44-~44: There might be a mistake here.
Context: ...er that platform (-1 disables). Notes - Virtual Segments: Rendering uses `virtua...
(QB_NEW_EN)
[grammar] ~48-~48: There might be a mistake here.
Context: ...se or adjust mapping settings in WLED. - Multiple Segments: Map different platfor...
(QB_NEW_EN)
[grammar] ~57-~57: There might be a mistake here.
Context: ...on; keep UpdateSecs at 60 to start. 3) Save and reboot. After a short boot safe...
(QB_NEW_EN)
[grammar] ~66-~66: There might be a mistake here.
Context: ...ontroller has Wi‑Fi and time (NTP) sync. - Direction looks reversed: Toggle the seg...
(QB_NEW_EN)
[grammar] ~70-~70: There might be a mistake here.
Context: ...s separate segments for more resolution. - API errors/backoff: On fetch errors the ...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
usermods/usermod_v2_bartdepart/readme.md
19-19: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (6)
usermods/usermod_v2_bartdepart/library.json (1)
1-4: LGTM: minimal manifest is fine.
Name + libArchive=false matches WLED usermod patterns.usermods/usermod_v2_bartdepart/platform_view.cpp (1)
60-64: Double‑check Segment::beginDraw()/end pairing.
If WLED requires endDraw() or a RAII guard, add it to avoid partial state. Otherwise, add a brief comment stating it’s intentionally unpaired.usermods/usermod_v2_bartdepart/interfaces.h (1)
8-17: Interfaces look solid and minimal.Clear contracts and JSON hooks are consistent with WLED usermods.
usermods/usermod_v2_bartdepart/util.h (1)
35-41: Confirm gmtime_r availability across all targets.
gmtime_ris fine on ESP32/ESP8266, but double-check other build variants. If needed, add a fallback togmtime.usermods/usermod_v2_bartdepart/usermod_v2_bartdepart.cpp (1)
20-22: Safety delay seems reasonable.10s boot guard is a good balance; no issues.
usermods/usermod_v2_bartdepart/bart_station_model.cpp (1)
123-140: Timestamp parsing looks correct.AM/PM handling and offset normalization align with API format; UTC return is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
usermods/usermod_v2_bartdepart/usermod_v2_bartdepart.cpp (1)
109-113: Resolved: treat missing config as non-errorReturning
truewhen the section is absent matches WLED conventions. Thanks for adopting the earlier suggestion.usermods/usermod_v2_bartdepart/readme.md (1)
19-21: Resolved: fenced code block now has a language hintThe
inilanguage tag fixes MD040. LGTM.
🧹 Nitpick comments (4)
usermods/usermod_v2_bartdepart/usermod_v2_bartdepart.cpp (3)
120-127: Don’t invalidate history on every config read
LegacyBartSource::readFromConfig(...)(see legacy_bart_source.cpp 79–87) unconditionally setsinvalidate_history = true, causing needless clears/reloads. Set it only when identity-affecting fields (e.g., base/station) change.Proposed change in legacy_bart_source.cpp:
bool LegacyBartSource::readFromConfig(JsonObject& root, bool startup_complete, bool& invalidate_history) { bool ok = true; - ok &= getJsonValue(root["UpdateSecs"], updateSecs_, 60); - ok &= getJsonValue(root["ApiBase"], apiBase_, apiBase_); - ok &= getJsonValue(root["ApiKey"], apiKey_, apiKey_); - ok &= getJsonValue(root["ApiStation"], apiStation_, apiStation_); - invalidate_history = true; + uint16_t prevUpdate = updateSecs_; + String prevBase = apiBase_; + String prevKey = apiKey_; + String prevStation= apiStation_; + + ok &= getJsonValue(root["UpdateSecs"], updateSecs_, 60); + ok &= getJsonValue(root["ApiBase"], apiBase_, apiBase_); + ok &= getJsonValue(root["ApiKey"], apiKey_, apiKey_); + ok &= getJsonValue(root["ApiStation"],apiStation_,apiStation_); + + // Only invalidate when source identity changes (base/station) + invalidate_history = (apiBase_ != prevBase) || (apiStation_ != prevStation); return ok; }
147-151: Avoid forcing permanent freeze after boot
seg.freeze = true;keeps the main segment frozen indefinitely. That can surprise users and other overlays/effects.Apply:
void BartDepart::doneBooting() { Segment& seg = strip.getMainSegment(); - seg.freeze = true; seg.setMode(0); }If you need a static background, consider making this behavior configurable.
23-30: Verify C++ standard before refactoring
Confirm that your build requires C++14 or newer—otherwisestd::make_uniquewon’t be available and switching from::make_unique(the provided shim) will break C++11 builds. Check your project’s build configuration (e.g. platformio.ini or CMake settings) before applying the diff.- sources_.push_back(::make_unique<LegacyBartSource>()); + sources_.push_back(std::make_unique<LegacyBartSource>()); ...usermods/usermod_v2_bartdepart/readme.md (1)
32-41: Document DebugPixelIndex config keyThe code supports
BartDepart.DebugPixelIndex, but it’s not listed here.Apply:
- BartDepart.Enabled: enable/disable the module. +- BartDepart.DebugPixelIndex: pixel index to blink for debugging (`-1` disables).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
usermods/usermod_v2_bartdepart/legacy_bart_source.cpp(1 hunks)usermods/usermod_v2_bartdepart/readme.md(1 hunks)usermods/usermod_v2_bartdepart/train_color.cpp(1 hunks)usermods/usermod_v2_bartdepart/usermod_v2_bartdepart.cpp(1 hunks)usermods/usermod_v2_bartdepart/usermod_v2_bartdepart.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- usermods/usermod_v2_bartdepart/train_color.cpp
- usermods/usermod_v2_bartdepart/usermod_v2_bartdepart.h
- usermods/usermod_v2_bartdepart/legacy_bart_source.cpp
🧰 Additional context used
🧬 Code graph analysis (1)
usermods/usermod_v2_bartdepart/usermod_v2_bartdepart.cpp (5)
usermods/usermod_v2_bartdepart/usermod_v2_bartdepart.h (2)
BartDepart(22-54)setup(39-45)usermods/usermod_v2_bartdepart/util.h (2)
time_now_utc(24-24)s(13-19)usermods/usermod_v2_bartdepart/platform_view.cpp (4)
view(25-105)view(25-26)appendConfigData(107-130)appendConfigData(107-107)usermods/usermod_v2_bartdepart/legacy_bart_source.cpp (4)
addToConfig(73-78)addToConfig(73-73)readFromConfig(80-88)readFromConfig(80-80)usermods/usermod_v2_bartdepart/platform_view.h (3)
addToConfig(20-20)appendConfigData(21-21)readFromConfig(23-25)
🪛 LanguageTool
usermods/usermod_v2_bartdepart/readme.md
[grammar] ~4-~4: There might be a mistake here.
Context: ...t; colored markers sweep toward “now” as departure times approach. ## Features ...
(QB_NEW_EN)
[grammar] ~9-~9: There might be a mistake here.
Context: ... (reverse, mirroring, grouping honored). - Configurable per‑platform segment mappin...
(QB_NEW_EN)
[grammar] ~10-~10: There might be a mistake here.
Context: ...mapping; disable any platform with -1. - Graceful overlay behavior (no freezing t...
(QB_NEW_EN)
[grammar] ~16-~16: There might be a mistake here.
Context: ...epartto your PlatformIO environment inplatformio_override.ini`: ```ini custo...
(QB_NEW_EN)
[grammar] ~23-~23: There might be a mistake here.
Context: ...artdepart ``` Build and flash as usual: - Build: pio run -e <your_env> - Upload:...
(QB_NEW_EN)
[grammar] ~24-~24: There might be a mistake here.
Context: ...``` Build and flash as usual: - Build: pio run -e <your_env> - Upload: `pio run -e <your_env> -t upload...
(QB_NEW_EN)
[grammar] ~29-~29: There might be a mistake here.
Context: ...s → “BartDepart”. The following keys are available. - BartDepart.Enabled: enable...
(QB_NEW_EN)
[grammar] ~38-~38: There might be a mistake here.
Context: ...roviding your own key for reliability. - ApiStation: station abbreviation (e.g., ...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ...). Views: PlatformView1 … PlatformView4 - SegmentId: WLED segment index to render ...
(QB_NEW_EN)
[grammar] ~44-~44: There might be a mistake here.
Context: ...er that platform (-1 disables). Notes - Virtual Segments: Rendering uses `virtua...
(QB_NEW_EN)
[grammar] ~48-~48: There might be a mistake here.
Context: ...se or adjust mapping settings in WLED. - Multiple Segments: Map different platfor...
(QB_NEW_EN)
[grammar] ~57-~57: There might be a mistake here.
Context: ...on; keep UpdateSecs at 60 to start. 3) Save and reboot. After a short boot safe...
(QB_NEW_EN)
[grammar] ~66-~66: There might be a mistake here.
Context: ...ontroller has Wi‑Fi and time (NTP) sync. - Direction looks reversed: Toggle the seg...
(QB_NEW_EN)
[grammar] ~70-~70: There might be a mistake here.
Context: ...s separate segments for more resolution. - API errors/backoff: On fetch errors the ...
(QB_NEW_EN)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
usermods/usermod_v2_bartdepart/usermod_v2_bartdepart.cpp (1)
78-84: Overlay correctly skips when disabled/off.The added guard prevents unintended drawing when the mod is disabled or LEDs are off. LGTM.
🧹 Nitpick comments (3)
usermods/usermod_v2_bartdepart/usermod_v2_bartdepart.cpp (3)
32-39: Gate boot animation behind enabled_ to avoid surprising users when disabled.Only show/tear down the boot UI when the mod is enabled.
void BartDepart::setup() { DEBUG_PRINTLN(F("BartDepart::setup starting")); uint32_t now_ms = millis(); safeToStart_ = now_ms + SAFETY_DELAY_MS; - showBooting(); + if (enabled_) showBooting(); state_ = BartDepartState::Setup; DEBUG_PRINTLN(F("BartDepart::setup finished")); } ... if (state_ == BartDepartState::Setup) { if (now_ms < safeToStart_) return; state_ = BartDepartState::Running; - doneBooting(); + if (enabled_) doneBooting(); reloadSources(now); }Also applies to: 51-56
55-56: Avoid reloading sources until system time is valid.If time isn’t synced yet (
now == 0), reloads can yield bogus ETDs. Guard the reloads.- doneBooting(); - reloadSources(now); + doneBooting(); + if (now > 0) reloadSources(now); ... - if (becameOn || becameEnabled) { - reloadSources(now); - } + if (becameOn || becameEnabled) { + if (now > 0) reloadSources(now); + }Also applies to: 60-62
23-30: Pre-reserve vector capacity to reduce heap churn on MCUs.Small, safe optimization for fragmented heaps.
BartDepart::BartDepart() { - sources_.push_back(::make_unique<LegacyBartSource>()); + sources_.reserve(1); + sources_.push_back(::make_unique<LegacyBartSource>()); model_ = ::make_unique<BartStationModel>(); - views_.push_back(::make_unique<PlatformView>("1")); + views_.reserve(4); + views_.push_back(::make_unique<PlatformView>("1")); views_.push_back(::make_unique<PlatformView>("2")); views_.push_back(::make_unique<PlatformView>("3")); views_.push_back(::make_unique<PlatformView>("4")); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
usermods/usermod_v2_bartdepart/usermod_v2_bartdepart.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
usermods/usermod_v2_bartdepart/usermod_v2_bartdepart.cpp (5)
usermods/usermod_v2_bartdepart/usermod_v2_bartdepart.h (2)
BartDepart(22-54)setup(39-45)usermods/usermod_v2_bartdepart/util.h (2)
time_now_utc(24-24)s(13-19)usermods/usermod_v2_bartdepart/platform_view.cpp (4)
view(25-105)view(25-26)appendConfigData(107-130)appendConfigData(107-107)usermods/usermod_v2_bartdepart/legacy_bart_source.cpp (4)
addToConfig(73-78)addToConfig(73-73)readFromConfig(80-88)readFromConfig(80-80)usermods/usermod_v2_bartdepart/platform_view.h (3)
addToConfig(20-20)appendConfigData(21-21)readFromConfig(23-25)
|
Thanks for the PR, but I wonder if it's a little too niche to be included in the main codebase (even as a usermod) |
…sers when disabled
Yeah, that makes sense. I'm going to take a look to see if I can generalize it to support GTFS-RT (https://gtfs.org/) ... that would make it applicable in many more places ... "GTFSdepart" instead (or something like that) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
usermods/usermod_v2_bartdepart/legacy_bart_source.cpp (5)
13-18: Tiny RAM win in composeUrl: reserve and place literals in flash.Pre-reserve and wrap constant pieces with F() to keep them out of RAM.
static String composeUrl(const String& base, const String& key, const String& station) { - String url = base; - url += "&key="; url += key; - url += "&orig="; url += station; + String url = base; + url.reserve(base.length() + key.length() + station.length() + 16); + url += F("&key="); url += key; + url += F("&orig="); url += station; return url; }
60-66: Avoid temp + move; reserve and emplace in-place.Saves a small copy/move and pre-allocates correct capacity.
- std::unique_ptr<BartStationModel> model(new BartStationModel()); + auto model = std::make_unique<BartStationModel>(); + model->platforms.reserve(platformIds().size()); for (const String& pid : platformIds()) { if (pid.isEmpty()) continue; - BartStationModel::Platform tp(pid); - tp.update(root); - model->platforms.push_back(std::move(tp)); + model->platforms.emplace_back(pid); + model->platforms.back().update(root); }
68-71: Optional: add slight jitter to nextFetch_ to avoid sync storms across many devices.Keeps upstream/API happier when many nodes reboot together.
- nextFetch_ = now + updateSecs_; + nextFetch_ = now + updateSecs_; + // Optional jitter (0-2s) to desynchronize devices: + // nextFetch_ += (millis() % 3);
23-33: Set timeouts and enable connection reuse (if supported) for stability.Prevents hangs on slow networks and saves TLS handshakes.
String url = composeUrl(apiBase_, apiKey_, apiStation_); if (!https_.begin(client_, url)) { @@ } + // Keep operations bounded and reuse connection if the core supports it + https_.setTimeout(5000); + #ifdef HTTPCLIENT_KEEPOPEN_SUPPORTED + https_.setReuse(true); + #endifPlease verify the exact API availability for the HTTPClient in the WLED ESP8266/ESP32 core you target (setReuse name/guard may differ).
80-95: Tighten config handling: clamp UpdateSecs, remove unused locals, and apply new interval immediately.Prevents extreme intervals, avoids unused vars, and picks up changed cadence without waiting.
bool ok = true; - uint16_t prevUpdate = updateSecs_; + uint16_t prevUpdate = updateSecs_; String prevBase = apiBase_; - String prevKey = apiKey_; String prevStation= apiStation_; @@ ok &= getJsonValue(root["UpdateSecs"], updateSecs_, 60); ok &= getJsonValue(root["ApiBase"], apiBase_, apiBase_); ok &= getJsonValue(root["ApiKey"], apiKey_, apiKey_); ok &= getJsonValue(root["ApiStation"], apiStation_, apiStation_); + // Clamp UpdateSecs to sane bounds + if (updateSecs_ < 10) updateSecs_ = 10; + if (updateSecs_ > 3600) updateSecs_ = 3600; + // If interval changed at runtime, fetch ASAP + if (startup_complete && updateSecs_ != prevUpdate) nextFetch_ = 0;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
usermods/usermod_v2_bartdepart/legacy_bart_source.cpp(1 hunks)usermods/usermod_v2_bartdepart/usermod_v2_bartdepart.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- usermods/usermod_v2_bartdepart/usermod_v2_bartdepart.cpp
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-29T19:55:40.229Z
Learnt from: ksedgwic
PR: wled/WLED#4884
File: usermods/usermod_v2_bartdepart/legacy_bart_source.cpp:4-6
Timestamp: 2025-08-29T19:55:40.229Z
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_bartdepart/legacy_bart_source.cpp
📚 Learning: 2025-08-29T19:51:15.486Z
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.486Z
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_bartdepart/legacy_bart_source.cpp
📚 Learning: 2025-08-29T20:00:13.351Z
Learnt from: ksedgwic
PR: wled/WLED#4884
File: usermods/usermod_v2_bartdepart/legacy_bart_source.h:19-24
Timestamp: 2025-08-29T20:00:13.351Z
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_bartdepart/legacy_bart_source.cpp
🧬 Code graph analysis (1)
usermods/usermod_v2_bartdepart/legacy_bart_source.cpp (2)
usermods/usermod_v2_bartdepart/legacy_bart_source.h (1)
LegacyBartSource(15-39)usermods/usermod_v2_bartdepart/usermod_v2_bartdepart.cpp (4)
addToConfig(89-100)addToConfig(89-89)readFromConfig(112-137)readFromConfig(112-112)
🔇 Additional comments (2)
usermods/usermod_v2_bartdepart/legacy_bart_source.cpp (2)
4-6: setInsecure() trade-off acknowledged.Using prior learning from this PR track: setInsecure() is acceptable here given ESP heap limits and non-sensitive data. No action needed.
31-39: Good: robust non-2xx handling.Early-exiting on non-2xx and backing off avoids parsing junk and wasting RAM/CPU.
| String payload = https_.getString(); | ||
| https_.end(); | ||
|
|
||
| size_t jsonSz = payload.length() * 2; | ||
| DynamicJsonDocument doc(jsonSz); | ||
| auto err = deserializeJson(doc, payload); | ||
| if (err) { | ||
| nextFetch_ = now + updateSecs_ * backoffMult_; | ||
| if (backoffMult_ < 16) backoffMult_ *= 2; | ||
| return nullptr; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Stream JSON instead of building a String to cut peak RAM and avoid OOM.
Current flow holds payload String and DynamicJsonDocument simultaneously. Stream-deserialize from the HTTPClient stream, size the doc from Content-Length, and log parse errors.
Apply:
- String payload = https_.getString();
- https_.end();
-
- size_t jsonSz = payload.length() * 2;
- DynamicJsonDocument doc(jsonSz);
- auto err = deserializeJson(doc, payload);
+ size_t contentLen = https_.getSize();
+ size_t jsonSz = contentLen > 0 ? (size_t)(contentLen + 512) : 4096; // heuristic
+ DynamicJsonDocument doc(jsonSz);
+ auto err = deserializeJson(doc, https_.getStream());
+ https_.end();
if (err) {
+ DEBUG_PRINTF("BartDepart: LegacyBartSource::fetch: JSON parse error: %s\n", err.c_str());
nextFetch_ = now + updateSecs_ * backoffMult_;
if (backoffMult_ < 16) backoffMult_ *= 2;
return nullptr;
}📝 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.
| String payload = https_.getString(); | |
| https_.end(); | |
| size_t jsonSz = payload.length() * 2; | |
| DynamicJsonDocument doc(jsonSz); | |
| auto err = deserializeJson(doc, payload); | |
| if (err) { | |
| nextFetch_ = now + updateSecs_ * backoffMult_; | |
| if (backoffMult_ < 16) backoffMult_ *= 2; | |
| return nullptr; | |
| } | |
| // Stream‐deserialize JSON instead of buffering in a String to reduce peak RAM usage | |
| size_t contentLen = https_.getSize(); | |
| size_t jsonSz = contentLen > 0 | |
| ? static_cast<size_t>(contentLen + 512) // add slack | |
| : 4096; // fallback heuristic | |
| DynamicJsonDocument doc(jsonSz); | |
| auto err = deserializeJson(doc, https_.getStream()); | |
| https_.end(); | |
| if (err) { | |
| DEBUG_PRINTF("BartDepart: LegacyBartSource::fetch: JSON parse error: %s\n", err.c_str()); | |
| nextFetch_ = now + updateSecs_ * backoffMult_; | |
| if (backoffMult_ < 16) backoffMult_ *= 2; | |
| return nullptr; | |
| } |
Render upcoming Bay Area Rapid Transit (BART) departures on LED segments.
Each platform gets its own segment; colored markers sweep toward “now” as
departure times approach.
Summary by CodeRabbit
New Features
Documentation
Chores