Usermod Google Calendar Scheduler #4988
Conversation
Features: - Fetches calendar events from Google Calendar (public iCal URL) - Triggers WLED presets/macros based on calendar events - Pattern matching for event titles/descriptions - Configurable polling interval (default 5 minutes) - Web UI configuration support - AsyncClient-based HTTP (non-blocking) - Custom iCal parser (no external dependencies) Usage: - Enable in Usermod Settings - Provide public Google Calendar iCal URL - Configure event pattern mappings - Events trigger presets at start/end times 🤖 Generated with Claude Code https://claude.com/claude-code Co-Authored-By: Claude <[email protected]>
Replaces AsyncClient with WiFiClient/WiFiClientSecure to support HTTPS calendar URLs. Removes event mapping configuration and related logic, simplifying event actions to use either JSON API or preset name from event description. Improves iCal parsing to handle line folding and unescaping, and refines event trigger logic for repeated events.
Enhanced the config UI to display loaded and active events, as well as last poll time. Improved preset matching by using case-insensitive comparison and WLED's getPresetName function, with additional debug output for preset search. Added manual poll trigger via JSON state.
Simplified debug output for preset matching in google_calendar_scheduler.cpp to only log when a match is found. Major improvements to the readme: clarified installation steps, added detailed configuration instructions, explained event mapping and JSON API usage, provided troubleshooting tips, and updated feature and limitation lists for better user guidance.
Replaces hardcoded 8KB JSON buffer with WLED's standard JSON_BUFFER_SIZE in event action execution. Updates documentation to reflect buffer size change for improved compatibility and maintainability.
Added a 10 second timeout to both secure and non-secure WiFi clients in fetchCalendarEvents to prevent indefinite blocking during network operations.
WalkthroughAdds a Google Calendar Scheduler usermod that polls an iCal URL, parses VEVENTs, maintains trigger state, and executes WLED actions (JSON API or preset-name). Includes manifest, README, and a new usermod ID. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 6
🧹 Nitpick comments (1)
usermods/google_calendar_scheduler/google_calendar_scheduler.cpp (1)
26-27: Consider reducing String usage for ESP8266 stability.Extensive use of the
Stringclass throughout the code (for URLs, HTTP fields, event data) can cause heap fragmentation on ESP8266 devices with limited memory. While functional, this may lead to stability issues over time.For future enhancement, consider:
- Using
char[]buffers for fixed-size strings- Leveraging ArduinoJson's string storage where appropriate
- Implementing string pooling for frequently allocated/deallocated strings
This is not urgent for the initial implementation but could improve long-term stability.
Also applies to: 33-34, 40-41
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
usermods/google_calendar_scheduler/google_calendar_scheduler.cpp(1 hunks)usermods/google_calendar_scheduler/library.json(1 hunks)usermods/google_calendar_scheduler/readme.md(1 hunks)wled00/const.h(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/!(html_*)*.h
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for non-generated C++ header files (.h)
Files:
wled00/const.h
🧬 Code graph analysis (1)
wled00/const.h (1)
usermods/google_calendar_scheduler/google_calendar_scheduler.cpp (1)
USERMOD_ID_CALENDAR_SCHEDULER(201-203)
🪛 markdownlint-cli2 (0.18.1)
usermods/google_calendar_scheduler/readme.md
84-84: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
100-100: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (5)
wled00/const.h (1)
210-210: LGTM!The new usermod ID constant follows the sequential numbering scheme, naming conventions, and formatting style of existing entries. The comment accurately identifies the associated usermod.
usermods/google_calendar_scheduler/library.json (1)
1-6: LGTM!The library manifest follows the standard structure for WLED usermods with appropriate settings.
usermods/google_calendar_scheduler/google_calendar_scheduler.cpp (3)
67-79: LGTM!The lifecycle methods are implemented correctly. The
setup()method initializes state, andconnected()appropriately triggers the initial calendar fetch when WiFi connects.
130-154: LGTM!Configuration serialization and deserialization are implemented correctly with proper unit conversion for the poll interval.
448-468: LGTM!Event triggering logic is correct and handles recurring events properly by resetting trigger flags after events end.
| * - Configurable poll interval and event mappings | ||
| */ | ||
|
|
||
| #define USERMOD_ID_CALENDAR_SCHEDULER 59 |
There was a problem hiding this comment.
Remove duplicate constant definition.
This constant is already defined in wled00/const.h (Line 210). The duplicate definition violates DRY principles and could cause issues if the values ever diverge. The class's getId() method (Line 202) should use the constant from const.h.
Apply this diff:
* - Configurable poll interval and event mappings
*/
-#define USERMOD_ID_CALENDAR_SCHEDULER 59
-
// Forward declarations
class GoogleCalendarScheduler : public Usermod {🤖 Prompt for AI Agents
In usermods/google_calendar_scheduler/google_calendar_scheduler.cpp around line
16, remove the duplicate "#define USERMOD_ID_CALENDAR_SCHEDULER 59" and any
local references to that macro; instead ensure the project header that defines
the constant (wled00/const.h) is included if not already, and update the class
to use USERMOD_ID_CALENDAR_SCHEDULER from const.h in getId() (and anywhere else)
so the code relies on the single canonical definition.
| // Check if it's JSON (starts with { or [) | ||
| if (desc.startsWith("{") || desc.startsWith("[")) { | ||
| // JSON API mode | ||
| if (!requestJSONBufferLock(17)) { |
There was a problem hiding this comment.
Replace magic number with named constant.
The value 17 passed to requestJSONBufferLock() is a magic number without explanation. Consider defining a named constant to improve code readability and maintainability.
Example:
// At the top of the file or in class definition
static const uint8_t JSON_LOCK_ID_CALENDAR = 17;
// Then use:
if (!requestJSONBufferLock(JSON_LOCK_ID_CALENDAR)) {🤖 Prompt for AI Agents
In usermods/google_calendar_scheduler/google_calendar_scheduler.cpp at line 488,
the call if (!requestJSONBufferLock(17)) uses the magic literal 17; define a
named constant (e.g., a constexpr or static const uint8_t JSON_LOCK_ID_CALENDAR
= 17) near the top of the file or inside the class definition where other
constants live, then replace the literal with that constant (if
(!requestJSONBufferLock(JSON_LOCK_ID_CALENDAR))) to improve readability and
maintainability.
The iCal parser now retains the triggered and endTriggered flags for events that persist across updates, matching by start time and title. This prevents unnecessary reset of trigger states for recurring or unchanged events.
Deleted the USERMOD_ID_CALENDAR_SCHEDULER macro definition from google_calendar_scheduler.cpp as it was not used in the file.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
usermods/google_calendar_scheduler/google_calendar_scheduler.cpp (1)
442-467: Consider validating extracted datetime components.The parsing extracts date/time components from the iCal string but doesn't validate them before passing to
makeTime(). WhilemakeTime()typically handles invalid inputs by returning 0, explicit validation would improve robustness and make debugging easier when dealing with malformed calendar data.Consider adding validation:
int year = dtStr.substring(0, 4).toInt(); int month = dtStr.substring(4, 6).toInt(); int day = dtStr.substring(6, 8).toInt(); int hour = dtStr.substring(9, 11).toInt(); int minute = dtStr.substring(11, 13).toInt(); int second = dtStr.substring(13, 15).toInt(); + + // Validate components + if (year < 1970 || month < 1 || month > 12 || day < 1 || day > 31 || + hour > 23 || minute > 59 || second > 59) { + DEBUG_PRINTLN(F("Calendar: Invalid datetime components")); + return 0; + } // Convert to Unix timestamp (simplified, doesn't account for all edge cases)
Optimized response reading from the calendar API to use buffered reads and limit response size, reducing heap fragmentation. Enhanced iCal event parsing to correctly handle TZID parameters in DTSTART and DTEND fields. Updated JSON buffer lock usage and improved code comments in the readme for clarity.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
usermods/google_calendar_scheduler/google_calendar_scheduler.cpp (2)
379-394: Critical: Line folding incorrectly advances position by 2 bytes regardless of line ending type.When a line ends with only
\n(Line 382), the code still advancesposby 2 (Line 386), which skips the\nplus one additional character from the next line. This corrupts DESCRIPTION parsing for iCal files with Unix-style line endings.Apply this diff to track the actual line ending length:
// Handle iCal line folding (lines starting with space/tab are continuations) while (pos < eventBlock.length()) { + int lineEndLength = 2; // Default for \r\n int lineEnd = eventBlock.indexOf("\r\n", pos); - if (lineEnd < 0) lineEnd = eventBlock.indexOf("\n", pos); - if (lineEnd < 0) lineEnd = eventBlock.length(); + if (lineEnd < 0) { + lineEnd = eventBlock.indexOf("\n", pos); + lineEndLength = 1; // Only \n + } + if (lineEnd < 0) { + lineEnd = eventBlock.length(); + lineEndLength = 0; // No line ending + } event.description += eventBlock.substring(pos, lineEnd); - pos = lineEnd + 2; // Skip \r\n + pos = lineEnd + lineEndLength; // Check if next line is a continuation (starts with space or tab) if (pos < eventBlock.length() && (eventBlock.charAt(pos) == ' ' || eventBlock.charAt(pos) == '\t')) {
494-514: Critical: Events won't trigger if system time is not synced.Using
toki.second()(Line 495) without verifying NTP sync can return 0 or boot-relative time. This causes the comparisoncurrentTime >= event.startTime(Line 501) to fail for all real calendar events, preventing the scheduler from functioning.Add a time-sync check before processing events:
void GoogleCalendarScheduler::checkAndTriggerEvents() { unsigned long currentTime = toki.second(); // Use WLED's time + + // Don't trigger events if time is not synced (Unix epoch starts 1970-01-01) + // A reasonable threshold is 1000000000 (2001-09-09) + if (currentTime < 1000000000) { + return; + } for (uint8_t i = 0; i < eventCount; i++) {
🧹 Nitpick comments (1)
usermods/google_calendar_scheduler/google_calendar_scheduler.cpp (1)
516-598: Unused parameter suggests incomplete implementation.The
isStartparameter (Line 516) is passed toexecuteEventActionbut never used in the function body. This suggests that start/end event differentiation may be planned but not implemented.If separate start/end actions are not needed, remove the parameter. Otherwise, implement logic to handle different actions based on event phase (e.g., different presets or JSON payloads for event start vs. end).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
usermods/google_calendar_scheduler/google_calendar_scheduler.cpp(1 hunks)usermods/google_calendar_scheduler/readme.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- usermods/google_calendar_scheduler/readme.md
| WiFiClient *client; | ||
| if (useHTTPS) { | ||
| WiFiClientSecure *secureClient = new WiFiClientSecure(); | ||
| secureClient->setInsecure(); // Skip certificate validation |
There was a problem hiding this comment.
Security: Certificate validation is disabled.
Calling setInsecure() disables TLS certificate validation, making the connection vulnerable to man-in-the-middle (MITM) attacks. An attacker could intercept calendar data or inject malicious events.
Consider implementing proper certificate validation or at minimum provide a configuration option to enable/disable certificate checks with a clear security warning.
🤖 Prompt for AI Agents
In usermods/google_calendar_scheduler/google_calendar_scheduler.cpp around line
258, the code calls secureClient->setInsecure(), which disables TLS certificate
validation and opens the connection to MITM attacks; remove the setInsecure()
call and implement proper cert validation by loading and using the CA root
certificate or pinning the server certificate/fingerprint via the
WiFiClientSecure API (e.g., setCACert or setFingerprint), and add an explicit
configuration option (disabled by default) to allow disabling validation only
for testing with a clear logged security warning; ensure errors on certificate
validation failures are handled and logged so connections fail safely.
There was a problem hiding this comment.
As far as I can tell WLED and ESP32 do not support async https. If you know a better way to handle this please let me know.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for other uses of WiFiClientSecure in the WLED codebase
echo "=== Searching for WiFiClientSecure usage ==="
rg -n "WiFiClientSecure" --type=cpp -C 5
echo -e "\n=== Searching for certificate handling ==="
rg -n "setCACert|setInsecure|setFingerprint|setCertificate" --type=cpp -C 3
echo -e "\n=== Searching for HTTPS patterns in WLED ==="
rg -n "https://" --type=cpp -C 2 | head -50
Length of output: 6368
🌐 Web query:
ESP32 WiFiClientSecure certificate validation options setCACert setFingerprint
💡 Result:
Short answer:
- setCACert(...) — supply a CA root (or CA bundle) in PEM so the ESP32 verifies the server certificate chain (recommended). Use this when you can provide the CA that signed the server cert. [1][2]
- setFingerprint(...) — supply a server-certificate fingerprint (SHA-1) to accept only that exact server cert. Works as a pinning shortcut but is weaker/fragile (fingerprints change on cert renewal) and generally not recommended for public HTTPS. [1]
- setInsecure() — disables certificate verification (no auth) — useful for testing only. [5]
Additional notes:
- For mutual TLS also use setCertificate(...) + setPrivateKey(...). [1]
- If you need many CAs use a CA bundle (or esp-idf x509 bundle) rather than a single CA string. There are helper/load functions and build-time bundle options. [1][7]
- The library historically has quirks (time validation, reuse, API changes across releases) — test on your ESP32 core version. [4][7]
Sources:
[1] WiFiClientSecure README / docs (Arduino ESP32).
[2] WiFiClientSecure reference pages (setCACert, setCertificate, setPrivateKey).
[3] Discussion on providing CA bundles & gen_crt_bundle (Arduino Stack Exchange).
[4] GitHub issue re: time/validation behavior.
@kendrick90, I think there's a bit of confusion here—async HTTPS and certificate validation are separate concerns. Your code is already synchronous (blocking), which is perfectly fine for periodic calendar polling every 5 minutes. The setInsecure() call is about certificate validation, not whether the connection is async or sync.
Certificate Validation Options
ESP32's WiFiClientSecure supports proper certificate validation without requiring async operations:
Option 1: Use Google's Root CA Certificate (Recommended)
// Add as a constant in your class
const char* google_root_ca = \
"-----BEGIN CERTIFICATE-----\n" \
"MIIFVzCCAz+gAwIBAgINAgPlk28xsBNJiGuiFzANBgkqhkiG9w0BAQwFADBHMQsw\n" \
// ... (Google Trust Services root CA)
"-----END CERTIFICATE-----\n";
// In fetchCalendarEvents(), replace setInsecure() with:
secureClient->setCACert(google_root_ca);Option 2: Certificate Pinning (Moderate Security)
// Pin Google's certificate fingerprint (must update when cert expires)
secureClient->setFingerprint("AA BB CC DD ..."); // SHA-1 fingerprintOption 3: Configuration Option (What I'd recommend)
Add a config option to disable validation for testing, but default to secure:
bool skipCertValidation = false; // Add to config
// In fetchCalendarEvents():
if (skipCertValidation) {
DEBUG_PRINTLN(F("⚠️ WARNING: Certificate validation disabled!"));
secureClient->setInsecure();
} else {
secureClient->setCACert(google_root_ca);
}Why This Matters
Without validation, anyone on the network can:
- Intercept calendar data (privacy leak)
- Inject malicious events to trigger unwanted actions
- Modify event timing/descriptions
For a public calendar URL, the risk may be lower, but it's still a security hole.
Practical Recommendation
Given WLED's resource constraints, I'd suggest:
- Include Google's root CA certificate (adds ~1-2KB)
- Add a config option to disable validation (off by default)
- Log a clear warning when validation is disabled
- Document the security implications in the readme
This balances security with flexibility for users who need it while protecting the default case.
Would you like me to help generate the certificate constant or the configuration code?
Enhanced iCal line ending handling to support both \r\n and \n, preventing incorrect parsing when line endings vary. Added a check in event triggering to ensure events are not triggered if system time is not properly synced, avoiding false triggers with invalid timestamps.
The isStart parameter was removed from the executeEventAction method and its calls, as it was no longer used. This simplifies the method signature and related code.
Added checks to ensure year, month, day, hour, minute, and second values are within valid ranges in parseICalDateTime. Returns 0 and logs a debug message if components are invalid.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
usermods/google_calendar_scheduler/google_calendar_scheduler.cpp (1)
258-258: Security: Certificate validation is still disabled.This is a duplicate of a previous unaddressed concern. Calling
setInsecure()disables TLS certificate validation, leaving the connection vulnerable to man-in-the-middle attacks.
🧹 Nitpick comments (1)
usermods/google_calendar_scheduler/google_calendar_scheduler.cpp (1)
589-608: Consider early exit or constant for preset ID range.The loop iterates through all preset IDs from 1 to 250, which could be slow if many presets are sparse. While acceptable for infrequent event triggers, consider using a named constant for the maximum preset ID for maintainability.
Define a constant for the maximum preset ID:
// In the private section or at file scope static const uint8_t MAX_PRESET_ID = 250;Then use it in the loop:
- for (uint8_t i = 1; i < 251; i++) { + for (uint8_t i = 1; i <= MAX_PRESET_ID; i++) {
usermods/google_calendar_scheduler/google_calendar_scheduler.cpp
Outdated
Show resolved
Hide resolved
usermods/google_calendar_scheduler/google_calendar_scheduler.cpp
Outdated
Show resolved
Hide resolved
Added checks to ensure NTP time is synced and valid before displaying active calendar events, showing a placeholder if not. Refactored HTTP client constants for response size, buffer size, and timeout to improve code clarity and reduce heap fragmentation.
Introduced HTTP_PORT and HTTPS_PORT constants for clarity and replaced hardcoded port numbers. Improved variable naming in iCal description parsing for better readability.
Introduces error tracking with lastError and lastErrorTime fields, displays errors in the UI and JSON state, and improves HTTP response handling by reporting connection and status code errors. Also fixes polling interval comparison for overflow safety.
Replaces the static MAX_EVENTS with a configurable maxEvents parameter, allowing up to 50 events. The events array is now dynamically allocated and reallocated when maxEvents changes. Also improves HTTP request construction and preset name comparison logic for efficiency.
Replaces NTP sync check with a time source check when retrieving current time in GoogleCalendarScheduler. This ensures time is only used if a valid time source is set, improving reliability.
This commit adds parsing for iCal DURATION fields to support events without DTEND, implements a retry mechanism for calendar polling, and refactors debug output to use DEBUG_PRINTF under WLED_DEBUG. These changes improve reliability and debugging for Google Calendar event scheduling.
Wrapped debug print statements with WLED_DEBUG guards for conditional compilation. Refactored polling interval calculation for clarity. Removed unused 'endTriggered' flag from CalendarEvent and related logic. Improved preset name comparison by using a lowercase copy of the description.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
usermods/google_calendar_scheduler/google_calendar_scheduler.cpp (2)
112-122: Simplify the poll timing logic.The computation of
nextPollTimeat line 113 is only used in a subtraction at line 114, making it redundant. The expressionnow - lastPollTime >= nextPollTime - lastPollTimesimplifies tonow - lastPollTime >= interval, which is clearer.Apply this diff for better readability:
// Poll calendar at configured interval (overflow-safe comparison) if (!isFetching && calendarUrl.length() > 0) { - unsigned long nextPollTime = lastPollTime + (retryCount > 0 ? retryDelay : pollInterval); - if (now - lastPollTime >= nextPollTime - lastPollTime) { + unsigned long interval = (retryCount > 0 ? retryDelay : pollInterval); + if (now - lastPollTime >= interval) { lastPollTime = now; if (fetchCalendarEvents()) { retryCount = 0; // Reset retry counter on success } else if (retryCount < MAX_RETRIES) { retryCount++; } } }
756-772: Consider using a named constant for preset range.The hard-coded upper bound
251assumes WLED's maximum preset ID. While correct, using a named constant would improve maintainability if this limit changes.Define a constant near the top of the class:
static const uint8_t MAX_PRESET_ID = 250;Then use it in the loop:
// Search through presets for matching name using WLED's getPresetName function - for (uint8_t i = 1; i < 251; i++) { + for (uint8_t i = 1; i <= MAX_PRESET_ID; i++) { String presetName; if (getPresetName(i, presetName)) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/google_calendar_scheduler/google_calendar_scheduler.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-02T19:36:49.513Z
Learnt from: freakintoddles2
PR: wled/WLED#4904
File: wled00/FX.cpp:3901-3913
Timestamp: 2025-09-02T19:36:49.513Z
Learning: In FX.cpp mode_percent time-based options (% of Minute/Hour/Day), do not guard against invalid RTC; showing a full bar with unsynced time is intentional to avoid confusing UX.
Applied to files:
usermods/google_calendar_scheduler/google_calendar_scheduler.cpp
🔇 Additional comments (5)
usermods/google_calendar_scheduler/google_calendar_scheduler.cpp (5)
227-261: LGTM: Proper time synchronization checks.The code correctly validates time synchronization by checking
toki.getTimeSource()and verifying the timestamp is valid before displaying time-based UI elements. The placeholder message when time is syncing provides good user feedback.
521-548: LGTM: Correct handling of timezone parameters.The DTSTART/DTEND parsing correctly handles timezone parameters (e.g.,
TZID=America/New_York) by usinglastIndexOf()to find the value separator colon and properly handles missing line endings.
482-519: LGTM: Correct iCal line folding implementation.The DESCRIPTION parsing correctly handles iCal line folding by tracking the actual line ending length (
\r\n,\n, or EOF) and checking for continuation lines (starting with space/tab). This prevents character skipping bugs.
679-705: LGTM: Robust event triggering logic.The event triggering logic properly validates time synchronization before processing events and correctly manages trigger state to avoid duplicate triggers while supporting recurring events.
707-783: Use the named constant instead of magic number.Line 725 uses the literal
17instead ofUSERMOD_ID_CALENDAR_SCHEDULER(which is defined as 59 inwled00/const.h). Wait—there's a discrepancy here. ThegetId()method at line 278 returnsUSERMOD_ID_CALENDAR_SCHEDULER(which is 59), but line 725 passes17torequestJSONBufferLock().This appears to be a copy-paste error. The lock ID should match the usermod ID returned by
getId():// JSON API mode - if (!requestJSONBufferLock(17)) { + if (!requestJSONBufferLock(USERMOD_ID_CALENDAR_SCHEDULER)) { DEBUG_PRINTLN(F("Calendar: Buffer locked, skipping")); return; }Likely an incorrect or invalid review comment.
usermods/google_calendar_scheduler/google_calendar_scheduler.cpp
Outdated
Show resolved
Hide resolved
Adds exponential backoff for calendar fetch retries, improves memory allocation checks, validates iCal response format and event times, and optimizes preset matching logic. Also enforces poll interval bounds and enhances debug output for error scenarios.
Added rate limiting to calendar fetches to prevent rapid successive requests. Improved event array reallocation logic to avoid race conditions, enhanced HTTP response handling with redirect support, and implemented automatic expiration of old events. Minor optimizations for memory usage and debug output.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/google_calendar_scheduler/google_calendar_scheduler.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-02T19:36:49.513Z
Learnt from: freakintoddles2
PR: wled/WLED#4904
File: wled00/FX.cpp:3901-3913
Timestamp: 2025-09-02T19:36:49.513Z
Learning: In FX.cpp mode_percent time-based options (% of Minute/Hour/Day), do not guard against invalid RTC; showing a full bar with unsynced time is intentional to avoid confusing UX.
Applied to files:
usermods/google_calendar_scheduler/google_calendar_scheduler.cpp
| if (presetName.length() != desc.length()) { | ||
| // Length mismatch after trim - case-insensitive comparison would fail anyway | ||
| // This avoids expensive toLowerCase() call | ||
| presetName.toLowerCase(); | ||
| if (presetName != descLower) continue; | ||
| } else { | ||
| presetName.toLowerCase(); | ||
| } | ||
|
|
||
| // Case-insensitive comparison (both already lowercased) | ||
| if (presetName == descLower) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Optimization logic is backwards and confusing.
The code and comments at lines 1053-1060 attempt to optimize preset name matching by skipping toLowerCase() when lengths differ. However, the logic is inverted:
- Comment says: "if length mismatch, skip expensive toLowerCase()"
- Code does: calls
toLowerCase()when lengths DON'T match (line 1056), then compares at line 1057 - Issue: The comparison at line 1057 is redundant with line 1063—if they match at 1057, they'll match at 1063
The correct optimization should skip the entire comparison when lengths differ (since case-insensitive comparison can never match if lengths differ):
- // Another quick length check after trim
- if (presetName.length() != desc.length()) {
- // Length mismatch after trim - case-insensitive comparison would fail anyway
- // This avoids expensive toLowerCase() call
- presetName.toLowerCase();
- if (presetName != descLower) continue;
- } else {
- presetName.toLowerCase();
- }
-
- // Case-insensitive comparison (both already lowercased)
- if (presetName == descLower) {
+ // Skip if lengths differ after trim (case-insensitive match is impossible)
+ if (presetName.length() != desc.length()) {
+ continue;
+ }
+
+ // Lengths match - perform case-insensitive comparison
+ presetName.toLowerCase();
+ if (presetName == descLower) {
presetId = i;
#ifdef WLED_DEBUG
DEBUG_PRINTF("Calendar: Found preset at ID %d\n", i);
#endif
break;
}This eliminates the redundant comparison and actually achieves the stated optimization goal.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In usermods/google_calendar_scheduler/google_calendar_scheduler.cpp around lines
1053-1063, the length-check optimization is inverted and causes redundant
comparisons; change the logic so that if presetName.length() != desc.length()
you immediately continue (skip comparison), otherwise lowercase presetName once
and then compare it to descLower; remove the branch that lowercases when lengths
differ and eliminate the redundant comparison so only the single
case-insensitive equality check remains.
Enhanced code documentation with detailed Doxygen-style comments for all major methods. Added 'validateCerts' configuration option to allow optional HTTPS certificate validation, improving security and compatibility. Refactored preset name matching logic for clarity and reliability.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/google_calendar_scheduler/google_calendar_scheduler.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-02T19:36:49.513Z
Learnt from: freakintoddles2
PR: wled/WLED#4904
File: wled00/FX.cpp:3901-3913
Timestamp: 2025-09-02T19:36:49.513Z
Learning: In FX.cpp mode_percent time-based options (% of Minute/Hour/Day), do not guard against invalid RTC; showing a full bar with unsynced time is intentional to avoid confusing UX.
Applied to files:
usermods/google_calendar_scheduler/google_calendar_scheduler.cpp
🔇 Additional comments (16)
usermods/google_calendar_scheduler/google_calendar_scheduler.cpp (16)
1-27: LGTM!The includes are appropriate and the documentation block is comprehensive, covering features, usage, and metadata.
29-99: LGTM!The class structure is well-organized with appropriate constants, configuration variables, and helper method declarations. The use of static const members for buffer sizes and timeouts addresses previous concerns about magic numbers.
101-120: LGTM!Proper memory allocation with null checking and error handling. The destructor correctly cleans up the allocated array.
122-130: LGTM!The connected() method appropriately triggers initial calendar fetch with proper guards.
132-166: LGTM!The loop logic properly implements exponential backoff with overflow-safe time comparisons and appropriate retry limits.
168-207: LGTM!The JSON state methods are properly implemented. The manual poll trigger via
pollNowis a useful feature.
209-284: LGTM!Config methods properly validate bounds and handle memory reallocation safely, including preventing reallocation during fetch and restoring the old array on allocation failure.
286-355: LGTM!The config data UI properly checks for time synchronization before displaying time-based information, showing a "Time syncing..." message when time is not yet available.
357-368: LGTM!The usermod correctly uses
USERMOD_ID_CALENDAR_SCHEDULERfromconst.hwithout duplicate definition. Static members are properly defined.
370-402: LGTM!URL parsing is straightforward and handles common cases correctly, including protocol detection and path extraction.
664-871: LGTM!Event parsing properly handles memory allocation failures, iCal line folding, timezone parameters in DTSTART/DTEND, event validation, duplicate detection, and trigger state preservation.
873-937: LGTM!Datetime parsing includes proper validation of components, month-specific day limits, and leap year handling.
939-990: LGTM!Duration parsing correctly handles ISO 8601 format with proper unit conversions.
992-1053: LGTM!Event checking properly validates time synchronization, expires old events efficiently using in-place array compaction, and correctly manages trigger state including reset after events end.
1055-1150: LGTM!Event action execution correctly handles both JSON API mode and preset name matching. The use of
USERMOD_ID_CALENDAR_SCHEDULER(fromconst.h) for the JSON buffer lock is proper.
1152-1155: LGTM!Standard WLED usermod registration pattern.
| if (validateCerts) { | ||
| // Certificate validation enabled - use default CA bundle | ||
| // Note: This may fail with some calendar providers that use non-standard CAs | ||
| // For maximum security with Google Calendar specifically, use: | ||
| // const char* google_root_ca = "-----BEGIN CERTIFICATE-----\n..."; | ||
| // secureClient->setCACert(google_root_ca); | ||
| #ifdef WLED_DEBUG | ||
| DEBUG_PRINTLN(F("Calendar: Certificate validation enabled")); | ||
| #endif | ||
| // On ESP32, uses built-in CA bundle. On ESP8266, may require setCACert() | ||
| // For now, fall back to setInsecure() if validation causes issues | ||
| secureClient->setInsecure(); // TODO: Implement proper CA validation | ||
| } else { | ||
| // Certificate validation disabled for broad compatibility | ||
| #ifdef WLED_DEBUG | ||
| DEBUG_PRINTLN(F("Calendar: Certificate validation disabled (setInsecure)")); | ||
| #endif | ||
| secureClient->setInsecure(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Security: Certificate validation is still disabled.
The code calls setInsecure() at line 457 even when validateCerts is true. While there's a TODO comment acknowledging this, it means the validateCerts configuration option currently has no effect, leaving connections vulnerable to MITM attacks.
Based on previous discussion with the author, this appears to be a known temporary limitation. However, users who enable validateCerts expecting protection will be misled.
Consider either:
- Implementing actual certificate validation (e.g., using
setCACert()with Google's root CA) - Removing the
validateCertsoption until it can be properly implemented - Adding a clear warning log when
validateCertsis enabled but not actually enforced
If option 1 is chosen, here's Google's root CA certificate that can be embedded:
// Add near the top of the file with other constants
static const char google_root_ca[] PROGMEM = R"(
-----BEGIN CERTIFICATE-----
MIIFVzCCAz+gAwIBAgINAgPlk28xsBNJiGuiFzANBgkqhkiG9w0BAQwFADBHMQsw
CQYDVQQGEwJVUzEiMCAGA1UEChMZR29vZ2xlIFRydXN0IFNlcnZpY2VzIExMQzEU
MBIGA1UEAxMLR1RTIFJvb3QgUjEwHhcNMTYwNjIyMDAwMDAwWhcNMzYwNjIyMDAw
MDAwWjBHMQswCQYDVQQGEwJVUzEiMCAGA1UEChMZR29vZ2xlIFRydXN0IFNlcnZp
Y2VzIExMQzEUMBIGA1UEAxMLR1RTIFJvb3QgUjEwggIiMA0GCSqGSIb3DQEBAQUA
A4ICDwAwggIKAoICAQC2EQKLHuOhd5s73L+UPreVp0A8of2C+X0yBoJx9vaMf/vo
27xqLpeXo4xL+Sv2sfnOhB2x+cWX3u+58qPpvBKJXqeqUqv4IyfLpLGcY9vXmX7w
Cl7raKb0xlpHDU0QM+NOsROjyBhsS+z8CZDfnWQpJSMHobTSPS5g4M/SCYe7zUjw
TcLCeoiKu7rPWRnWr4+wB7CeMfGCwcDfLqZtbBkOtdh+JhpFAz2weaSUKK0Pfybl
qAj+lug8aJRT7oM6iCsVlgmy4HqMLnXWnOunVmSPlk9orj2XwoSPwLxAwAtcvfaH
szVsrBhQf4TgTM2S0yDpM7xSma8ytSmzJSq0SPly4cpk9+aCEI3oncKKiPo4Zor8
Y/kB+Xj9e1x3+naH+uzfsQ55lVe0vSbv1gHR6xYKu44LtcXFilWr06zqkUspzBmk
MiVOKvFlRNACzqrOSbTqn3yDsEB750Orp2yjj32JgfpMpf/VjsPOS+C12LOORc92
wO1AK/1TD7Cn1TsNsYqiA94xrcx36m97PtbfkSIS5r762DL8EGMUUXLeXdYWk70p
aDPvOmbsB4om3xPXV2V4J95eSRQAogB/mqghtqmxlbCluQ0WEdrHbEg8QOB+DVrN
VjzRlwW5y0vtOUucxD/SVRNuJLDWcfr0wbrM7Rv1/oFB2ACYPTrIrnqYNxgFlQID
AQABo0IwQDAOBgNVHQ8BAf8EBAMCAYYwDwYDVR0TAQH/BAUwAwEB/zAdBgNVHQ4E
FgQU5K8rJnEaK0gnhS9SZizv8IkTcT4wDQYJKoZIhvcNAQEMBQADggIBAJ+qQibb
C5u+/x6Wki4+omVKapi6Ist9wTrYggoGxval3sBOh2Z5ofmmWJyq+bXmYOfg6LEe
QkEzCzc9zolwFcq1JKjPa7XSQCGYzyI0zzvFIoTgxQ6KfF2I5DUkzps+GlQebtuy
h6f88/qBVRRiClmpIgUxPoLW7ttXNLwzldMXG+gnoot7TiYaelpkttGsN/H9oPM4
7HLwEXWdyzRSjeZ2axfG34arJ45JK3VmgRAhpuo+9K4l/3wV3s6MJT/KYnAK9y8J
ZgfIPxz88NtFMN9iiMG1D53Dn0reWVlHxYciNuaCp+0KueIHoI17eko8cdLiA6Ef
MeB2MZI8vr68RPbpF20xZCYqSQcmEAv6Y1pZP9qCMv6PQMCB8AwuEPvLa8c8jxSF
0b9dD9xaZtJ3XBGA6y6qjWa1I0b1IHC0fLBuZoVgT3HQJkHCnXzTL7X5qOGUdcNs
yqRqxQPxZjOMSJ/L0a5U+3jBbqSvjDf2nJp0U9Yt0o7dY6RRmJr6MCNPQp8KBExN
fJZOo1KPO7R1mHZQSxD7bQtRh3K7aE1TFcCaQVT+oTMlN8f3tLKC2bLq4L2rZqvJ
BLcgXBMz/xDl8q8gLJFGQoGrE6J2DGKXHX8X3bxbPvwvW1NPlJzPmkKB8C1Y8HL1
kfWkN5P+5qOaG3FiOqQVG+t0sZKwPILHSMfD
-----END CERTIFICATE-----
)";
// Then in fetchCalendarEvents():
if (validateCerts) {
secureClient->setCACert(google_root_ca);
} else {
secureClient->setInsecure();
}🏁 Script executed:
#!/bin/bash
rg -n "setCACert" -g "*.cpp" -g "*.h"
rg -n "google_root_ca" -g "*.cpp" -g "*.h"Length of output: 529
Enforce certificate validation or remove validateCerts option
The validateCerts flag is currently ignored (always calls secureClient->setInsecure()). Either embed and apply a CA cert (e.g. via secureClient->setCACert(google_root_ca)) when validateCerts is true, or remove the option to avoid misleading users and log a clear warning until proper validation is implemented.
🤖 Prompt for AI Agents
In usermods/google_calendar_scheduler/google_calendar_scheduler.cpp around lines
446 to 464, the validateCerts branch currently ignores the flag and always calls
secureClient->setInsecure(); fix this by applying a real CA when validateCerts
is true (embed a PEM root CA string like google_root_ca and call
secureClient->setCACert(google_root_ca)) and only call setInsecure() in the else
branch; if you cannot embed a CA now, remove the validateCerts flag or change
the true-branch to log a clear warning and return an error instead of silently
calling setInsecure().
|
how much of the code that claude.ai is providing did you review and do understand yourself? |
|
What does out of tree mean? I tested the code initially when I submitted the PR then got lots of suggestion from your code rabbit ai and have been attempting to implement these ideas and testing as I go. Of course there is a trade off between minimal and working and complex and "better". Input on the Https issue would be appreciated. I am surprised WLED does not support https out of the box. Maybe give me a couple days and check back in and I will have it further tested and settled. But the code is not very complex it checks the calendar and passes the api call or looks up the preset by name. I understand the hesitation to add AI generated code but I don't think you should just dismiss it at first glance without providing any substantive critique. Especially when you guys using AI tool too is half the reason for all these changes. I'd rather talk about what is good and what is bad with the code itself and improve it. This is a super useful tool that will make scheduling lighting events way easier than it is now where the common suggestion is to have a full home assistant install as well. |
it means the code is hosted and maintained by you instead of the WLED team
WLED runs on local network only, so https would be overkill and just eat up resorces. Btw, your code adds over 100k to the bin, it may stop compiling in the future except for 8M+ flash size boards.
you want to add over 1000 lines of code, I would judge that as fairly complex.
I am by no means agains using AI to generate code, BUT it takes a developer to judge if its any good. Just throwing in purely AI generated code withou you having reviewed it thoroughly and actually understand in detail what the code does (which was my initial question) I am not willing to spend time on reviewing it. |
|
I am down to reduce the length of the code back to the ~500 lines version if that helps but then it would lack input validation, user readable error messages, retry logic and other checks that the code rabbit AI and claude suggested to add. I'm assuming the bin increase is due to the need to use wificlientsecure library. I'm not sure there is a way around it and would love some constructive feedback as fetching the ical is required to be over https. It's possible we could fetch over http with user credentials but I was kind of hoping to avoid having people inputting their google credentials into wled. I can look around for more minimal implementations of https. IMO WLED should stop supporting the smaller boards because we are constantly faced with this skimping for resources issue when there are equivalently priced boards that even more capable. Why is a feature like hue, mqtt, or alexa given first class positioning when calendar integration seems like it would have a much broader use case? Anyways I have been reading and understanding the code and tested it with a simple set up. I will try to address your concern about final bin size. I will also do more in depth testing this week and make a write up on how this usermod works. |
its going to be your usermod, so your call really on what you want to include functionality wise. I see this as quite niche, I am guessing HA already can do something like this.
Probably.
I am guessing 100k people will disagree on this. ;)
Personally I do not see any use case for a calendar while I can see use cases for the others. |
3 participants
This pull request introduces a new usermod for WLED called
google_calendar_scheduler, which enables WLED devices to automatically trigger presets or API calls based on Google Calendar events. The changes include the addition of configuration files, documentation, and a new usermod ID for integration.New Google Calendar Scheduler Usermod:
google_calendar_scheduler/library.jsonto define the usermod and its build options.google_calendar_scheduler/readme.mddocumenting features, installation, configuration, usage, troubleshooting, technical details, security notes, limitations, and future enhancements.Integration with WLED Core:
#define USERMOD_ID_CALENDAR_SCHEDULER 59towled00/const.h, enabling WLED to recognize and use the calendar scheduler usermod.Summary by CodeRabbit