add syslog function to wled #4664
Conversation
WalkthroughThis change set introduces syslog support as a new debug output option in the WLED project. The syslog functionality is conditionally compiled using the Changes
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 5
🧹 Nitpick comments (3)
wled00/syslog.h (2)
58-58: Remove the unused test variable.This boolean member variable doesn't appear to be used anywhere in the implementation and seems to be leftover debug code.
- bool test = true;
60-60: Consider a configurable buffer size.The fixed 128-byte buffer might be too small for complex log messages, especially when including timestamps and other syslog metadata. Consider making this configurable or handling message truncation more explicitly.
You could add a compile-time configuration option for the buffer size:
- char _buffer[128]; // Buffer for collecting characters + #ifndef SYSLOG_BUFFER_SIZE + #define SYSLOG_BUFFER_SIZE 128 + #endif + char _buffer[SYSLOG_BUFFER_SIZE]; // Buffer for collecting characterswled00/syslog.cpp (1)
135-149: Add year to BSD syslog timestamp.The BSD format timestamp doesn't include the year, which could cause issues with log aggregation over year boundaries. Consider adding the year or a configuration option to include it.
if (ntpEnabled && ntpConnected) { // Month abbreviation const char* months[] = {"Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec"}; - syslogUdp.printf("%s %2d %02d:%02d:%02d ", + syslogUdp.printf("%s %2d %04d %02d:%02d:%02d ", months[month(localTime) - 1], day(localTime), + year(localTime), hour(localTime), minute(localTime), second(localTime)); } else { // No valid time available - syslogUdp.print("Jan 01 00:00:00 "); + syslogUdp.print("Jan 01 1970 00:00:00 "); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
wled00/bus_manager.h(1 hunks)wled00/cfg.cpp(2 hunks)wled00/data/settings_sync.htm(1 hunks)wled00/json.cpp(2 hunks)wled00/set.cpp(1 hunks)wled00/syslog.cpp(1 hunks)wled00/syslog.h(1 hunks)wled00/wled.cpp(3 hunks)wled00/wled.h(1 hunks)wled00/xml.cpp(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
wled00/xml.cpp (2)
wled00/fcn_declare.h (4)
printSetFormCheckbox(498-498)settingsScript(421-421)printSetFormValue(499-499)printSetFormValue(500-500)wled00/util.cpp (6)
printSetFormCheckbox(98-100)printSetFormCheckbox(98-98)printSetFormValue(101-103)printSetFormValue(101-101)printSetFormValue(108-110)printSetFormValue(108-108)
🔇 Additional comments (16)
wled00/bus_manager.h (1)
26-35: Well-implemented syslog integration in debug output mechanismThe conditional inclusion of syslog support is correctly ordered to prioritize syslog over network debug and serial output. This maintains backward compatibility while extending functionality.
wled00/data/settings_sync.htm (1)
240-292: Well-structured Syslog UI sectionThe Syslog settings section is well-organized and follows the existing UI patterns. All relevant syslog parameters (enable/disable, host, port, protocol, facility, severity) are included with appropriate options.
wled00/set.cpp (1)
476-496: Thorough parameter validation in syslog configuration handlerThe implementation correctly validates all syslog parameters before applying them, including checking for valid ranges for protocol, facility, and severity. The immediate initialization of the syslog client ensures changes take effect right away.
wled00/wled.cpp (3)
332-334: Proper conditional for USB CDC debug outputGood modification to prevent disabling kernel messages when syslog is enabled, maintaining debug output capabilities when needed.
390-392: Avoid TX pin allocation when using syslogCorrectly prevents allocating the hardware TX pin for debug output when syslog is enabled, avoiding potential pin conflicts.
425-430: Clean syslog initialization during setupThe syslog client is properly initialized with the configured parameters and application name during setup. This ensures syslog is ready to use as soon as the system starts.
wled00/cfg.cpp (2)
539-547: Good implementation of syslog configuration deserialization.The code adds proper support for reading syslog configuration from a JSON file, following the existing patterns used for other network services. All necessary syslog parameters are included and properly contained within the conditional compilation directive.
1064-1072: Well-structured syslog configuration serialization.This implementation correctly writes syslog configuration to JSON, maintaining consistency with other configuration sections. Good use of conditional compilation to ensure the code is only included when syslog is enabled.
wled00/json.cpp (2)
302-305: Good addition of syslog toggle via JSON API with feedback.The implementation correctly enables enabling/disabling syslog via the JSON API, with appropriate debug feedback. It follows the same pattern as the existing debug options, maintaining consistency.
787-789: Consistent implementation of syslog flags in serialized info.This change properly updates the
osflags to reflect syslog debug status in the serialized JSON output, following the same bit manipulation pattern used for other debug options. Excellent consistency with existing code.wled00/xml.cpp (2)
108-108: Correct update to debug pin reservation logic.This change ensures the TX pin isn't needlessly reserved when syslog debugging is enabled, allowing it to be used for other purposes. Good attention to detail in working with the existing conditional compilation logic.
520-529: Well-implemented syslog settings UI integration.The code adds all necessary UI elements for syslog configuration using existing helper functions. The toggle for hiding syslog settings when not compiled in follows the same pattern used for other optional features. Clean implementation.
wled00/wled.h (2)
967-992: Well-structured syslog globals and configuration defaults.This implementation properly adds all necessary global variables and default settings for syslog functionality. The code is well-documented with helpful comments and follows the established patterns for configuration macros.
Each setting has a sensible default that can be overridden at compile time, and the naming conventions are consistent with the rest of the codebase.
993-993: Correct modification to debug output selection chain.Changed
#ifdefto#elifto properly form a conditional chain with the new syslog section, ensuring only one debug output mechanism is selected.wled00/syslog.cpp (2)
104-111:⚠️ Potential issuePrevent buffer overflow in write method.
The buffer check should use
>=instead of<to ensure there's always room for the null terminator.size_t SyslogPrinter::write(uint8_t c) { // Store in buffer regardless of connection status - if (_bufferIndex < sizeof(_buffer) - 1) { + if (_bufferIndex < sizeof(_buffer) - 1) { // Leave space for null terminator _buffer[_bufferIndex++] = c; } // If newline or buffer full, flush - if (c == '\n' || _bufferIndex >= sizeof(_buffer) - 1) { + if (c == '\n' || _bufferIndex >= sizeof(_buffer) - 1) { flushBuffer(); }Likely an incorrect or invalid review comment.
64-65:❌ Incorrect review comment
Reduce dependency on global variables.
The code depends on global variables like
WLED_CONNECTEDandsyslogEnabledwhich creates tight coupling and makes testing difficult. Consider passing these as parameters or having more self-contained logic.bool SyslogPrinter::resolveHostname() { - if (!WLED_CONNECTED || !syslogEnabled) return false; + // Check connection status locally - can be a class method or passed as parameter + bool isConnected = WLED_CONNECTED; + extern bool syslogEnabled; // Declare as extern if it must be used + if (!isConnected || !syslogEnabled) return false;
🏁 Script executed:
#!/bin/bash # Find where WLED_CONNECTED and syslogEnabled are defined echo "Searching for WLED_CONNECTED definition:" rg -A 2 -B 2 "define\s+WLED_CONNECTED" --no-ignore echo -e "\nSearching for syslogEnabled definition:" rg -A 2 -B 2 "bool\s+syslogEnabled" --no-ignoreLength of output: 816
Global connectivity and syslog flags are intended—no refactor needed
The use of WLED_CONNECTED and syslogEnabled here follows project conventions for runtime connectivity checks and feature toggles. Extracting them into method parameters would add complexity without improving clarity or testability.
- WLED_CONNECTED is a macro in wled00/wled.h that evaluates Wi-Fi/Ethernet status at runtime.
- syslogEnabled is declared as a WLED_GLOBAL bool in wled00/wled.h and controls syslog output globally.
If you need to isolate or mock these for unit tests, wrap them in a thin interface or use test‐level overrides rather than altering production code.
Likely an incorrect or invalid review comment.
|
i could just test it with the RFC 3164 format since my syslog server only works with this. The output of RAW should be the same as the Net Debug. Maybe someone could look through it since i wasnt really sure about: wled00/json.cpp: I just took it from the WLED_DEBUG_HOST but im not sure what is does exactly. also i wansnt really sure where to put this in the setup part wled00/wled.cpp: i just thought after Wifi would be good |
wled00/data/settings_sync.htm
Outdated
| </select><br> | ||
| <i>Keep at 115200 to use Improv. Some boards may not support high rates.</i> | ||
| </div> | ||
| <h3>Syslog</h3> |
There was a problem hiding this comment.
this will add size to all builds, not acceptable for a debug function IMHO
There was a problem hiding this comment.
Yeah I would agree that allowing the user to select the facility and level is overkill
There was a problem hiding this comment.
this should solve that size problem when not using syslog:
There was a problem hiding this comment.
While that is an interesting approach to the problem, I still see no point in allowing the user to define nonsensical values. For example , WLED is not a printer, so what is the point of the user being able to select LPR as the facility.
There was a problem hiding this comment.
Hi i just added the full list i found online. So which should i remove/keep? i read somewhere that SYSLOG_LOCAL4 (LOCAL4) is normaly used for user output, thats why i also set is as default.
// Syslog facility codes
#define SYSLOG_KERN 0 // kernel messages
#define SYSLOG_USER 1 // user-level messages
#define SYSLOG_MAIL 2 // mail system
#define SYSLOG_DAEMON 3 // system daemons
#define SYSLOG_AUTH 4 // security/authorization messages
#define SYSLOG_SYSLOG 5 // messages generated internally by syslogd
#define SYSLOG_LPR 6 // line printer subsystem
#define SYSLOG_NEWS 7 // network news subsystem
#define SYSLOG_UUCP 8 // UUCP subsystem
#define SYSLOG_CRON 9 // clock daemon
#define SYSLOG_AUTHPRIV 10 // security/authorization messages (private)
#define SYSLOG_FTP 11 // FTP daemon
#define SYSLOG_NTP 12 // NTP subsystem (used in some systems)
#define SYSLOG_LOG_AUDIT 13 // log audit (used in some systems like Linux auditd)
#define SYSLOG_LOG_ALERT 14 // log alert
#define SYSLOG_CLOCK_DAEMON 15 // clock daemon (alternate)
#define SYSLOG_LOCAL0 16 // local use 0
#define SYSLOG_LOCAL1 17 // local use 1
#define SYSLOG_LOCAL2 18 // local use 2
#define SYSLOG_LOCAL3 19 // local use 3
#define SYSLOG_LOCAL4 20 // local use 4
#define SYSLOG_LOCAL5 21 // local use 5
#define SYSLOG_LOCAL6 22 // local use 6
#define SYSLOG_LOCAL7 23 // local use 7
i would think of least those ?
#define SYSLOG_USER 1 // user-level messages
#define SYSLOG_DAEMON 3 // system daemons
#define SYSLOG_LOCAL0 16 // local use 0
#define SYSLOG_LOCAL1 17 // local use 1
#define SYSLOG_LOCAL2 18 // local use 2
#define SYSLOG_LOCAL3 19 // local use 3
#define SYSLOG_LOCAL4 20 // local use 4
#define SYSLOG_LOCAL5 21 // local use 5
#define SYSLOG_LOCAL6 22 // local use 6
#define SYSLOG_LOCAL7 23 // local use 7
and maybe change th edefault to:
LOCAL0 is reserved for custom applications (i.e. it will never collide with kernel, mail, auth, etc.)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
- Add error state tracking to the SyslogPrinter class. - configurable buffer size. That way, anyone can -D SYSLOG_BUFFER_SIZE=256 (or larger) in their build flags to avoid silent truncation.
it sets or clears bits i.e. flags in os variaable, used in index.js |
|
Looks ok to me (untested), though it is a lot of code for somewhat useful feature. At least it is optional. |
to not wast size to all builds the html part will now be injected only when
is used: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pio-scripts/inject_syslog_ui.py (3)
2-2: Remove unused importThe
sysmodule is imported but never used in the script.-import os, shutil, sys +import os, shutil🧰 Tools
🪛 Ruff (0.8.2)
2-2:
sysimported but unusedRemove unused import:
sys(F401)
120-121: Use context manager for file operationsOpening files without a context manager can lead to resource leaks if exceptions occur.
- original = open(html_path, 'r', encoding='utf8').read() - modified = original + with open(html_path, 'r', encoding='utf8') as f: + original = f.read() + modified = original🧰 Tools
🪛 Ruff (0.8.2)
120-120: Use a context manager for opening files
(SIM115)
116-151: Consider a more robust approach to detect completed injectionsThe script assumes that if a backup file exists, the injection has already been completed successfully. This might not be true if a previous run was interrupted or crashed.
Consider adding a more explicit indicator of successful injection, such as checking for the presence of the injected HTML content rather than just relying on the existence of the backup file.
# backup + inject only once if not os.path.exists(bak): print("Backing up and injecting Syslog UI...") shutil.copyfile(html_path, bak) try: with open(html_path, 'r', encoding='utf8') as f: original = f.read() modified = original # replace existing section if present if '<!-- SYSLOG-START -->' in modified and '<!-- SYSLOG-END -->' in modified: start = modified.index('<!-- SYSLOG-START -->') end = modified.index('<!-- SYSLOG-END -->') + len('<!-- SYSLOG-END -->') modified = ( modified[:start] + '<!-- SYSLOG-START -->\n' + SYSLOG_HTML + '\n<!-- SYSLOG-END -->' + modified[end:] ) else: # insert before last <hr> idx = modified.rfind('<hr>') if idx == -1: print("\033[41mCould not find <hr> to insert Syslog UI!\033[0m") return modified = ( modified[:idx] + '<!-- SYSLOG-START -->\n' + SYSLOG_HTML + '\n<!-- SYSLOG-END -->\n' + modified[idx:] ) with open(html_path, 'w', encoding='utf8') as f: f.write(modified) print("\033[42mSyslog UI injected successfully!\033[0m") except Exception as e: print(f"\033[41mError during injection: {e}\033[0m") + # Clean up the backup if injection failed + if os.path.exists(bak): + os.remove(bak) else: - print("Backup exists; assume already injected.") + # Verify that the injection was actually completed + with open(html_path, 'r', encoding='utf8') as f: + content = f.read() + if '<!-- SYSLOG-START -->' not in content or '<!-- SYSLOG-END -->' not in content: + print("Backup exists but injection markers not found. Re-injecting...") + os.remove(bak) # Remove the backup to force re-injection + inject_syslog_ui(source, target, env) # Recursive call to retry injection + else: + print("Backup exists and injection markers found; already injected.")🧰 Tools
🪛 Ruff (0.8.2)
120-120: Use a context manager for opening files
(SIM115)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pio-scripts/inject_syslog_ui.py(1 hunks)platformio.ini(1 hunks)wled00/data/settings_sync.htm(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- platformio.ini
🚧 Files skipped from review as they are similar to previous changes (1)
- wled00/data/settings_sync.htm
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: KrX3D
PR: wled/WLED#4664
File: wled00/syslog.cpp:50-51
Timestamp: 2025-04-26T12:51:04.972Z
Learning: In the WLED codebase, syslog configuration variables (syslogHost, syslogPort, syslogProtocol, syslogFacility, syslogSeverity) are defined as global variables in wled.h and are accessed directly in the SyslogPrinter implementation, rather than being class members.
🪛 Ruff (0.8.2)
pio-scripts/inject_syslog_ui.py
2-2: sys imported but unused
Remove unused import: sys
(F401)
8-8: Undefined name env
(F821)
120-120: Use a context manager for opening files
(SIM115)
167-167: Undefined name env
(F821)
171-171: Undefined name env
(F821)
🔇 Additional comments (5)
pio-scripts/inject_syslog_ui.py (5)
32-32: Consider handling placeholder valueThe
%SL_port%placeholder is included in the HTML template. Ensure this is properly replaced during runtime by the WLED template system, otherwise users will see the literal placeholder.Is there a template replacement mechanism in WLED that handles these placeholders? If not, you may need to replace it with a default value or handle it in the script.
5-8: LGTM for SCons environment handlingThe import and use of the SCons environment is correct. The static analysis tool incorrectly flagged
envas undefined, but this is a standard SCons pattern.🧰 Tools
🪛 Ruff (0.8.2)
8-8: Undefined name
env(F821)
80-114: Well-structured build flag detection and state trackingThe build flag detection and state tracking logic is robust, with good handling of different flag formats and proper state persistence between builds.
152-165: Effective restoration functionThe restoration function correctly handles cleaning up after the build, with proper file permission management and backup removal.
1-171: Overall well-designed injection scriptThis script provides a clean solution for conditionally including the Syslog UI in the settings page. The approach of injecting at build time keeps the source HTML clean while allowing the feature to be conditionally enabled. The colored console output and detailed progress messages are helpful for debugging.
🧰 Tools
🪛 Ruff (0.8.2)
2-2:
sysimported but unusedRemove unused import:
sys(F401)
8-8: Undefined name
env(F821)
120-120: Use a context manager for opening files
(SIM115)
167-167: Undefined name
env(F821)
171-171: Undefined name
env(F821)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pio-scripts/inject_syslog_ui.py (3)
18-22: Consider adding directory creation check before writing state fileThe
set_syslog_statefunction assumes the directory structure exists. If the script runs in a clean environment or the directory is missing, this could fail.def set_syslog_state(project_dir, enabled): state_file = os.path.join(project_dir, "wled00/data/.syslog_state") + os.makedirs(os.path.dirname(state_file), exist_ok=True) # Ensure directory exists with open(state_file, 'w') as f: f.write("1" if enabled else "0")
158-162: Add retry limit to prevent potential infinite recursionThe current implementation uses recursion without a limit when re-injecting content with missing markers, which could lead to infinite recursion if there's a persistent issue.
- if '<!-- SYSLOG-START -->' not in content or '<!-- SYSLOG-END -->' not in content: - print("Backup exists but SYSLOG markers missing—forcing re-injection.") - os.remove(bak) - inject_syslog_ui(source, target, env) + if '<!-- SYSLOG-START -->' not in content or '<!-- SYSLOG-END -->' not in content: + print("Backup exists but SYSLOG markers missing—forcing re-injection.") + os.remove(bak) + # Pass a retry parameter to prevent infinite recursion + if 'retry_count' not in globals(): + global retry_count + retry_count = 0 + if retry_count < 3: + retry_count += 1 + inject_syslog_ui(source, target, env) + retry_count = 0 # Reset on success + else: + print("\033[41mToo many retry attempts. Manual intervention required.\033[0m") + retry_count = 0 # Reset for next runAlternatively, modify the function signature to accept a retry parameter:
def inject_syslog_ui(source, target, env, retry_count=0): # ... existing code ... # When recursing: if retry_count < 3: inject_syslog_ui(source, target, env, retry_count + 1) else: print("\033[41mToo many retry attempts. Manual intervention required.\033[0m")
119-122: Consider handling large files more efficientlyLoading the entire file into memory could be problematic for very large HTML files. Consider reading and processing the file in chunks for better memory efficiency.
For most WLED installations this won't be an issue as the HTML files are relatively small, but it's a good practice to handle files more efficiently, especially in build scripts that might run on resource-constrained systems.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pio-scripts/inject_syslog_ui.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: KrX3D
PR: wled/WLED#4664
File: wled00/syslog.cpp:50-51
Timestamp: 2025-04-26T12:51:04.972Z
Learning: In the WLED codebase, syslog configuration variables (syslogHost, syslogPort, syslogProtocol, syslogFacility, syslogSeverity) are defined as global variables in wled.h and are accessed directly in the SyslogPrinter implementation, rather than being class members.
🪛 Ruff (0.8.2)
pio-scripts/inject_syslog_ui.py
8-8: Undefined name env
(F821)
180-180: Undefined name env
(F821)
184-184: Undefined name env
(F821)
🔇 Additional comments (4)
pio-scripts/inject_syslog_ui.py (4)
32-32: Note about placeholder variableThe
%SL_port%placeholder is used here, but this script doesn't replace it with an actual value. This appears to be an intentional design where the placeholder will be processed by WLED's template system when the HTML is served.
88-90: LGTM: Good handling of build flagsGood approach for handling build flags that might be provided either as a list or a string.
174-177: Good file permission handlingSetting the chmod before copying is a nice touch that ensures the file can be written even if permissions were changed.
1-184: Overall feedback on inject_syslog_ui.pyThis is a well-structured build script that intelligently handles Syslog UI injection. The approach of backing up the original file, tracking state changes, and verifying markers is robust. The color-coded console output also makes it easy to track what's happening during the build process.
The script correctly integrates with the PlatformIO build system using SCons hooks and handles different build scenarios appropriately.
🧰 Tools
🪛 Ruff (0.8.2)
8-8: Undefined name
env(F821)
180-180: Undefined name
env(F821)
184-184: Undefined name
env(F821)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pio-scripts/inject_syslog_ui.py (3)
155-156: Add explicit handling for mismatched markers.Consider adding specific handling for the case where one marker exists but the other doesn't (incomplete previous run). Currently, you check if either marker is missing, but you might want to log which specific marker is missing for easier debugging.
- if '<!-- SYSLOG-START -->' not in content or '<!-- SYSLOG-END -->' not in content: - print("Backup exists but SYSLOG markers missing—forcing re-injection.") + if '<!-- SYSLOG-START -->' not in content and '<!-- SYSLOG-END -->' not in content: + print("Backup exists but both SYSLOG markers missing—forcing re-injection.") + elif '<!-- SYSLOG-START -->' not in content: + print("Backup exists but SYSLOG-START marker missing—forcing re-injection.") + elif '<!-- SYSLOG-END -->' not in content: + print("Backup exists but SYSLOG-END marker missing—forcing re-injection.")
159-162: Consider adding delay between retry attempts.When retrying the injection after failure, consider adding a small delay between attempts to avoid potential file system contention. This could improve reliability when retrying rapidly.
# only retry up to 3 times if retry_count < 3: + # Add a small delay before retrying + import time + time.sleep(0.5 * (retry_count + 1)) # Increasing delay with each retry inject_syslog_ui(source, target, env, retry_count + 1) else: print("\033[41mToo many retry attempts. Manual intervention required.\033[0m")
1-2: Consider adding a brief docstring for maintainability.While the code is well-structured and self-explanatory, adding a brief docstring at the top of the file would improve maintainability and help future developers understand the script's purpose.
# pio-scripts/inject_syslog_ui.py +""" +PlatformIO build script to conditionally inject Syslog UI elements into the settings HTML file. + +This script: +1. Injects Syslog UI elements when WLED_ENABLE_SYSLOG is defined in build flags +2. Restores the original HTML file after build completion +3. Tracks state between builds to force UI rebuilds when necessary +""" import os, shutil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pio-scripts/inject_syslog_ui.py(1 hunks)wled00/xml.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wled00/xml.cpp
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: KrX3D
PR: wled/WLED#4664
File: wled00/syslog.cpp:50-51
Timestamp: 2025-04-26T12:51:04.972Z
Learning: In the WLED codebase, syslog configuration variables (syslogHost, syslogPort, syslogProtocol, syslogFacility, syslogSeverity) are defined as global variables in wled.h and are accessed directly in the SyslogPrinter implementation, rather than being class members.
🪛 Ruff (0.8.2)
pio-scripts/inject_syslog_ui.py
8-8: Undefined name env
(F821)
181-181: Undefined name env
(F821)
185-185: Undefined name env
(F821)
🔇 Additional comments (7)
pio-scripts/inject_syslog_ui.py (7)
1-6: Well-structured script with clear imports and environment handling.The script properly imports necessary modules and retrieves the PlatformIO environment. The organization is clean and follows best practices for PlatformIO build scripts.
10-22: Good state tracking implementation for build-to-build persistence.The state tracking functions are well-implemented with proper file handling and error cases. The approach of using a hidden state file to track changes between builds is elegant and efficient.
24-75: Well-structured HTML for the Syslog UI configuration.The HTML definition provides a comprehensive UI for configuring all syslog parameters (protocol, facility, severity, etc.) with appropriate input types and validation.
87-87: Robust build flag detection handles both spacing variants.Good job checking for both
-D WLED_ENABLE_SYSLOGand-DWLED_ENABLE_SYSLOGvariants in the build flags. This ensures the script works correctly regardless of how the flag is specified.
94-101: Effective change detection to force UI rebuilds when needed.The script intelligently detects state changes between builds and touches the HTML file to force a rebuild when necessary. This ensures the UI stays in sync with the build configuration.
166-179: Good restoration logic with permission handling.The restoration function correctly handles file permissions (chmod) before copying the backup, which prevents potential permission-related errors. The function is also safely guarded by checking for the backup file's existence.
180-185: Excellent build system integration.The script properly integrates with the PlatformIO build system by registering a post-action for cleanup and conditionally running the injection only on full builds. This ensures the HTML file is restored to its original state after the build completes.
🧰 Tools
🪛 Ruff (0.8.2)
181-181: Undefined name
env(F821)
185-185: Undefined name
env(F821)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pio-scripts/inject_syslog_ui.py (2)
97-97: Consider using regex for more robust build flag detection.The current approach checks for two specific patterns of the define flag. A regex approach might be more robust against variations in spacing or other flag formats.
- has_syslog = ("-D WLED_ENABLE_SYSLOG" in build_flags or "-DWLED_ENABLE_SYSLOG" in build_flags) + import re + has_syslog = bool(re.search(r'-D\s*WLED_ENABLE_SYSLOG', build_flags))
155-160: Consider more specific exception handling for file operations.The current exception handling is generic. It might be beneficial to catch specific exceptions related to file operations separately from other potential issues.
- except Exception as e: - print(f"\033[41mError during injection: {e}\033[0m") - # injection failed → remove backup so we'll retry next time - if os.path.exists(bak): - os.remove(bak) + except (IOError, OSError) as e: + print(f"\033[41mFile operation error during injection: {e}\033[0m") + # injection failed → remove backup so we'll retry next time + if os.path.exists(bak): + os.remove(bak) + except Exception as e: + print(f"\033[41mUnexpected error during injection: {e}\033[0m") + # injection failed → remove backup so we'll retry next time + if os.path.exists(bak): + os.remove(bak)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pio-scripts/inject_syslog_ui.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: KrX3D
PR: wled/WLED#4664
File: wled00/syslog.cpp:50-51
Timestamp: 2025-04-26T12:51:04.972Z
Learning: In the WLED codebase, syslog configuration variables (syslogHost, syslogPort, syslogProtocol, syslogFacility, syslogSeverity) are defined as global variables in wled.h and are accessed directly in the SyslogPrinter implementation, rather than being class members.
🪛 Ruff (0.8.2)
pio-scripts/inject_syslog_ui.py
18-18: Undefined name env
(F821)
194-194: Undefined name env
(F821)
198-198: Undefined name env
(F821)
🔇 Additional comments (7)
pio-scripts/inject_syslog_ui.py (7)
1-19: Well-structured script with clear purpose and good documentation.The file has a clear header, descriptive docstring, and appropriate imports. The detection of full vs. partial builds is a good optimization to avoid unnecessary processing.
🧰 Tools
🪛 Ruff (0.8.2)
18-18: Undefined name
env(F821)
21-32: Good implementation of state tracking between builds.The state tracking functions are well-implemented with proper file handling and error management. This approach ensures UI rebuilds only happen when the syslog state changes.
33-85: Complete and well-structured HTML template for Syslog configuration.The HTML template includes all necessary configuration options for syslog: enable/disable toggle, host, port, protocol selection, facility, and severity levels. The dropdown options cover all standard syslog facilities and severity levels.
126-154: Good implementation of HTML injection with marker-based replacement.The code properly handles both cases: replacing content between existing markers or inserting at a strategic location. The approach is robust and maintainable.
163-177: Robust verification and retry mechanism for injection failures.The code verifies that markers are actually present in the file and implements a retry mechanism with incremental delays. This helps recover from potential race conditions or transient file system issues.
179-192: Clean implementation of restore function with proper permission handling.The restore function properly handles file permissions before copying, which prevents potential permission-related errors. The backup file is correctly removed after restoration.
193-198: Good integration with PlatformIO build process.The script properly integrates with the PlatformIO build process by registering the restore function as a post-action and conditionally running the injection only on full builds.
🧰 Tools
🪛 Ruff (0.8.2)
194-194: Undefined name
env(F821)
198-198: Undefined name
env(F821)
|
i removed the facility one i mentioned above. Is it ok or do you want me to remomve more of them? changing facility doesnt change anything for now (at least not for my syslog server) but maybe this could be usefull in the future when QuickDebug is added: |
|
It also doesn't make sense to have the log level in the config, this is something you decide on a per log basis. As WLED does not currently have log levels, all messages will be debug |
ok so should i keep it for the future and make it so it can only be changed via define and remove it from the gui? so i would set SYSLOG_DEBUG as default EDIT: or since the log level (Severity) and Facility do currently nothing i could remove them from the gui but leave them in for the future |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pio-scripts/inject_syslog_ui.py (3)
22-30: Consider relocating the state file outside of the data directory.The state file
.syslog_stateis currently stored inwled00/data/which is part of the project's data directory that gets included in the firmware image. Consider moving it to a build-specific directory like.pio/buildor the project root to avoid including it in the firmware.- state_file = os.path.join(project_dir, "wled00/data/.syslog_state") + state_file = os.path.join(project_dir, ".syslog_state")
165-172: Consider making the retry count configurable.The script retries injection up to 3 times with an increasing delay, which is a good approach. For better flexibility, consider making the maximum retry count configurable, either as a constant at the top of the file or through an environment variable.
+ MAX_RETRY_ATTEMPTS = 3 # Can be adjusted as needed ... - if retry_count < 3: + if retry_count < MAX_RETRY_ATTEMPTS:
186-186: Ensure cross-platform compatibility for file permissions.The
os.chmod(html_path, 0o644)call sets Unix-style file permissions which may not work as expected on Windows. Consider adding platform detection to skip this step on Windows or use a cross-platform approach.+ import platform + if platform.system() != 'Windows': os.chmod(html_path, 0o644)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pio-scripts/inject_syslog_ui.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
pio-scripts/inject_syslog_ui.py
18-18: Undefined name env
(F821)
191-191: Undefined name env
(F821)
195-195: Undefined name env
(F821)
🔇 Additional comments (3)
pio-scripts/inject_syslog_ui.py (3)
39-39: Verify template placeholder%SL_port%.The port input has a default value of
%SL_port%which appears to be a template placeholder. Ensure this is properly replaced during runtime by the WLED template engine, or consider providing a hard-coded default value.
47-74: Clarify the purpose of commented-out Facility and Severity settings.The code includes commented-out HTML elements for Facility and Severity settings. This matches your PR comments where you discussed whether to keep or remove these options. Are these intentionally disabled for now but planned for future implementation?
If you want to keep these options for future use but hidden from the UI, consider adding a comment explaining this intention for other developers.
1-195: Overall well-designed build script with good error handling.The script is well-structured with appropriate error handling, state tracking, and backup/restore functionality. The approach of injecting UI elements conditionally is clean and effective, and the retry mechanism for injection failures shows good defensive programming practices.
🧰 Tools
🪛 Ruff (0.8.2)
18-18: Undefined name
env(F821)
191-191: Undefined name
env(F821)
195-195: Undefined name
env(F821)
i removed now the option to configure Facility and Severity from the gui, but i left them in the code, since maybe it will be used in the future. So now only this is available in the gui:
hi, since write "some strings" which one do you mean? |
- removing wled#15 and only whitespace line from beeing sent - replace space with _ from serverDescription
|
It's never a good idea to have used code hanging about, so please strip this back to just what ads value and a clear usecase. For example, just because your syslog server can accept multiple formats, why does WLED need to support multiple formats? |
There was a problem hiding this comment.
hi, since write "some strings" which one do you mean?
I mean most of larger strings, arduino core for esp32 supports format strings in flash too size_t printf(const __FlashStringHelper *ifsh, ...);
I don't know if there is performance difference.
i changed that, the next comit will comment most of it, by the request of netmindz |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pio-scripts/inject_syslog_ui.py (1)
34-76: Consider documenting why some UI elements are commented out.The HTML snippet contains commented-out elements for protocol selection, facility, and severity settings. Based on the PR objectives, I understand these were intentionally removed from the UI but kept in the code for future use. Adding a brief comment explaining this would clarify the intention for future developers.
<h3>Syslog</h3> <div id="Syslog"> Enable Syslog: <input type="checkbox" name="SL_en"><br> Host: <input type="text" name="SL_host" maxlength="32"><br> Port: <input type="number" name="SL_port" min="1" max="65535" value="%SL_port%"><br> + <!-- These UI elements are commented out but preserved for potential future use --> <!-- Protocol:wled00/syslog.h (1)
57-60: Consider adding clarifying comments for configuration members.The code comment mentions "using globals from wled.h" but the relation between member variables (_facility, _severity, _protocol) and global variables could be clearer.
// Syslog configuration - using globals from wled.h -uint8_t _facility; // Fixed to SYSLOG_LOCAL0 -uint8_t _severity; // Fixed to SYSLOG_DEBUG -uint8_t _protocol; // Fixed to SYSLOG_PROTO_BSD +uint8_t _facility; // Internal copy of syslogFacility (from wled.h), fixed to SYSLOG_LOCAL0 +uint8_t _severity; // Internal copy of syslogSeverity (from wled.h), fixed to SYSLOG_DEBUG +uint8_t _protocol; // Internal copy of syslogProtocol (from wled.h), fixed to SYSLOG_PROTO_BSDwled00/syslog.cpp (1)
205-286: Consider adding a comment explaining why protocol implementations are commented out.The code contains commented-out implementations for RFC5424 and RAW protocols but uses only the BSD protocol. A brief comment explaining that this was done to reduce code size or for future extensibility would be helpful.
// Handle different syslog protocol formats + // Note: Only BSD protocol is currently implemented; RFC5424 and RAW are preserved for future use // switch (_protocol) { // case SYSLOG_PROTO_BSD: // RFC 3164 format: <PRI>TIMESTAMP HOSTNAME APP-NAME: MESSAGE
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
pio-scripts/inject_syslog_ui.py(1 hunks)wled00/cfg.cpp(2 hunks)wled00/set.cpp(1 hunks)wled00/syslog.cpp(1 hunks)wled00/syslog.h(1 hunks)wled00/wled.h(1 hunks)wled00/xml.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- wled00/cfg.cpp
- wled00/xml.cpp
- wled00/wled.h
- wled00/set.cpp
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: KrX3D
PR: wled/WLED#4664
File: wled00/syslog.cpp:50-51
Timestamp: 2025-04-26T12:51:04.993Z
Learning: In the WLED codebase, syslog configuration variables (syslogHost, syslogPort, syslogProtocol, syslogFacility, syslogSeverity) are defined as global variables in wled.h and are accessed directly in the SyslogPrinter implementation, rather than being class members.
wled00/syslog.h (1)
Learnt from: KrX3D
PR: wled/WLED#4664
File: wled00/syslog.cpp:50-51
Timestamp: 2025-04-26T12:51:04.993Z
Learning: In the WLED codebase, syslog configuration variables (syslogHost, syslogPort, syslogProtocol, syslogFacility, syslogSeverity) are defined as global variables in wled.h and are accessed directly in the SyslogPrinter implementation, rather than being class members.
wled00/syslog.cpp (1)
Learnt from: KrX3D
PR: wled/WLED#4664
File: wled00/syslog.cpp:50-51
Timestamp: 2025-04-26T12:51:04.993Z
Learning: In the WLED codebase, syslog configuration variables (syslogHost, syslogPort, syslogProtocol, syslogFacility, syslogSeverity) are defined as global variables in wled.h and are accessed directly in the SyslogPrinter implementation, rather than being class members.
🪛 Ruff (0.11.9)
pio-scripts/inject_syslog_ui.py
18-18: Undefined name env
(F821)
191-191: Undefined name env
(F821)
195-195: Undefined name env
(F821)
🔇 Additional comments (13)
pio-scripts/inject_syslog_ui.py (4)
1-11: Well-structured script with clear documentation.The script header and documentation explain the purpose and functionality clearly, which is essential for build scripts that modify files automatically.
18-18: Ignore static analysis warning about undefined 'env'.The static analyzer flagged
envas undefined, but this is a false positive sinceenvis correctly imported from SCons.Script on line 15.🧰 Tools
🪛 Ruff (0.11.9)
18-18: Undefined name
env(F821)
78-175: Good error handling with retry logic.The script has robust error handling for file operations and includes a retry mechanism with incremental delays. It also ensures that if injection fails, the backup is removed to allow retry on the next build.
191-195: Good approach to register actions and conditionally run injection.The script correctly registers the post-action to restore files after every build but only runs injection on full builds. This ensures the HTML files remain unchanged in the source while being modified only for builds.
🧰 Tools
🪛 Ruff (0.11.9)
191-191: Undefined name
env(F821)
195-195: Undefined name
env(F821)
wled00/syslog.h (3)
1-10: Well-structured header with proper include guards.The header file follows good C++ practices with appropriate include guards and includes only the necessary dependencies.
11-46: Good approach for preserving future extensibility.Keeping commented-out facility codes and severity levels makes future extension easier while only defining the essential constants needed for the current implementation.
48-85: Well-designed class with proper error handling.The
SyslogPrinterclass is well-structured with appropriate private and public members. The error handling fields added based on previous review feedback (_lastOperationSucceeded and _lastErrorMessage) improve the robustness of the implementation.wled00/syslog.cpp (6)
6-40: Good use of PROGMEM for string constants.Storing string constants in flash memory (PROGMEM) is a best practice for memory-constrained devices like ESP8266/ESP32. Although most arrays are commented out, they're preserved for future use and properly use PROGMEM.
42-49: Constructor correctly initializes error tracking fields.The constructor now properly initializes all fields including the error tracking fields (_lastOperationSucceeded, _lastErrorMessage) as recommended in previous reviews.
92-112: Robust hostname resolution with platform-specific handling.The hostname resolution logic is well-implemented with proper checks for existing IP, direct IP parsing, and platform-specific DNS resolution methods. It correctly handles both WiFi and Ethernet connections.
114-144: Good buffer management with filtering of irrelevant messages.The buffer management logic efficiently handles special cases like "#15" sequences and whitespace-only messages, preventing unnecessary network traffic.
164-185: Excellent error handling implementation.The error handling in the write method addresses the previous review suggestions effectively. Each failure case is clearly identified with a specific error message, and the operation status is tracked.
220-236: Good handling of timestamp formatting based on NTP availability.The code correctly handles timestamps by using the system time when NTP is enabled and connected, or falling back to a default timestamp when NTP is unavailable.
i commented out all the code that currently does not "add value". i commented it out and didnt remove it since i think it maybe could be used in the future. i hardcoded now those: and now only those options are configurable via gui and define:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pio-scripts/inject_syslog_ui.py (1)
34-77: Good approach for future-proofing with commented UI elements.The HTML injection includes commented-out UI elements for protocol, facility, and severity settings. This aligns with the PR comments where you decided to simplify the UI but preserve code for potential future use.
However, consider adding a more explicit comment about why only Host and Port are exposed in the UI while other settings are hardcoded in the firmware. This would help future maintainers understand the design decision.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pio-scripts/inject_syslog_ui.py(1 hunks)wled00/syslog.cpp(1 hunks)wled00/syslog.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- wled00/syslog.h
- wled00/syslog.cpp
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: KrX3D
PR: wled/WLED#4664
File: wled00/syslog.cpp:50-51
Timestamp: 2025-04-26T12:51:04.993Z
Learning: In the WLED codebase, syslog configuration variables (syslogHost, syslogPort, syslogProtocol, syslogFacility, syslogSeverity) are defined as global variables in wled.h and are accessed directly in the SyslogPrinter implementation, rather than being class members.
🪛 Ruff (0.11.9)
pio-scripts/inject_syslog_ui.py
18-18: Undefined name env
(F821)
192-192: Undefined name env
(F821)
196-196: Undefined name env
(F821)
🔇 Additional comments (6)
pio-scripts/inject_syslog_ui.py (6)
1-19: Well-structured build script with clear purpose and functionality.The script is well-organized with a clear docstring explaining its purpose and functionality. The detection of full vs. partial builds is handled appropriately to avoid unnecessary operations during incremental builds.
Note that the static analysis warnings about
envbeing undefined are false positives - this is correctly imported from SCons on line 15, which is standard practice in PlatformIO build scripts.🧰 Tools
🪛 Ruff (0.11.9)
18-18: Undefined name
env(F821)
95-103: Excellent state tracking implementation.The approach of tracking Syslog enablement state between builds and forcing a UI rebuild when the state changes is elegant and efficient. Touching the file's timestamp to trigger a rebuild is a clean solution that works well with build systems.
124-142: Robust HTML injection with fallback mechanism.The code intelligently handles both the case where markers already exist (replacement) and where they don't (insertion). The fallback to insert before the last
<hr>tag is a good approach, though it does assume the HTML structure will remain consistent.
148-174: Excellent error handling and retry mechanism.The implementation includes proper error handling with specific exception types and a thoughtful retry mechanism with incremental delays. This makes the script robust against transient file system issues or race conditions.
The approach of removing the backup file when injection fails ensures the script will retry on the next build, which is a good self-healing mechanism.
177-190: Clean restore implementation with proper file handling.The restoration function correctly checks for the existence of the backup file before attempting to restore, and it properly cleans up by removing the backup after restoration. The file permission change on line 187 ensures the file is writable across different platforms.
191-196: Appropriate build integration approach.Registering the restore function as a post-action on "checkprogsize" ensures it runs on every build, while only running the injection on full builds is an optimization that prevents unnecessary file operations during incremental builds.
🧰 Tools
🪛 Ruff (0.11.9)
192-192: Undefined name
env(F821)
196-196: Undefined name
env(F821)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pio-scripts/inject_syslog_ui.py (4)
43-53: Improve regex pattern for more robust flag detectionThe regex pattern for detecting the
WLED_ENABLE_SYSLOGflag could be more robust to handle edge cases.Consider this more precise pattern:
- has_syslog = bool(re.search(r'-D\s*WLED_ENABLE_SYSLOG', build_flags)) + has_syslog = bool(re.search(r'-D\s*WLED_ENABLE_SYSLOG\b', build_flags))The
\bword boundary ensures we don't match partial flag names likeWLED_ENABLE_SYSLOG_EXTENDED.
87-100: Improve robustness of HTML injection fallbackThe fallback logic for inserting before the last
<hr>tag might be fragile if the HTML structure changes.Consider adding more specific error handling and validation:
# insert before last <hr> idx = modified.rfind('<hr>') if idx == -1: - print("\033[41mCould not find <hr> to insert Syslog UI!\033[0m") - return + print("\033[41mCould not find <hr> to insert Syslog UI!\033[0m") + # Clean up backup since injection failed + if os.path.exists(bak): + os.remove(bak) + raise Exception("HTML injection failed: no suitable insertion point found")This ensures consistent error handling and backup cleanup.
121-133: Simplify retry logic to avoid recursive callsThe retry logic using recursive function calls could be confusing and might hit Python's recursion limit in edge cases.
Consider using a loop-based approach:
- if '<!-- SYSLOG-INJECT -->' not in content: - print("Backup exists but SYSLOG markers missing—forcing re-injection.") - os.remove(bak) - # only retry up to 3 times - if retry_count < 3: - # Add a small delay before retrying - import time - time.sleep(0.5 * (retry_count + 1)) # Increasing delay with each retry - inject_syslog_ui(source, target, env, retry_count + 1) - else: - print("\033[41mToo many retry attempts. Manual intervention required.\033[0m") + if '<!-- SYSLOG-INJECT -->' not in content: + print("Backup exists but SYSLOG markers missing—forcing re-injection.") + os.remove(bak) + # Use a simple retry without recursion + max_retries = 3 + for attempt in range(max_retries): + try: + import time + time.sleep(0.5 * (attempt + 1)) + # Re-run the injection logic here + inject_syslog_ui(source, target, env, attempt + 1) + break + except Exception as e: + if attempt == max_retries - 1: + print(f"\033[41mToo many retry attempts: {e}. Manual intervention required.\033[0m") + continueHowever, this would require refactoring the injection logic into a separate function to avoid duplication.
145-145: Consider removing unnecessary file permission changeThe explicit
chmodcall may be unnecessary sinceshutil.copy2preserves file permissions.- if os.path.exists(html_path): - os.chmod(html_path, 0o644) - shutil.copy2(bak, html_path) + shutil.copy2(bak, html_path)Unless there's a specific reason to ensure 644 permissions, this change is likely redundant.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pio-scripts/inject_syslog_ui.py(1 hunks)wled00/cfg.cpp(2 hunks)wled00/data/settings_sync.htm(1 hunks)wled00/set.cpp(1 hunks)wled00/syslog.cpp(1 hunks)wled00/syslog.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- wled00/set.cpp
- wled00/data/settings_sync.htm
- wled00/cfg.cpp
- wled00/syslog.h
- wled00/syslog.cpp
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: KrX3D
PR: wled/WLED#4664
File: wled00/syslog.cpp:50-51
Timestamp: 2025-04-26T12:51:04.993Z
Learning: In the WLED codebase, syslog configuration variables (syslogHost, syslogPort, syslogProtocol, syslogFacility, syslogSeverity) are defined as global variables in wled.h and are accessed directly in the SyslogPrinter implementation, rather than being class members.
pio-scripts/inject_syslog_ui.py (2)
Learnt from: KrX3D
PR: wled/WLED#4664
File: wled00/syslog.cpp:50-51
Timestamp: 2025-04-26T12:51:04.993Z
Learning: In the WLED codebase, syslog configuration variables (syslogHost, syslogPort, syslogProtocol, syslogFacility, syslogSeverity) are defined as global variables in wled.h and are accessed directly in the SyslogPrinter implementation, rather than being class members.
Learnt from: Jason2866
PR: wled/WLED#4167
File: platformio.ini:427-430
Timestamp: 2025-07-06T16:11:05.874Z
Learning: In PlatformIO, using `-fno-lto` in `build_unflags` is a valid pattern to remove potentially existing `-fno-lto` flags from the build system before adding `-flto=auto` in `build_flags`. This ensures that LTO can be enabled without conflicts from default build system flags that might disable LTO.
🪛 Ruff (0.11.9)
pio-scripts/inject_syslog_ui.py
18-18: Undefined name env
(F821)
150-150: Undefined name env
(F821)
154-154: Undefined name env
(F821)
🔇 Additional comments (3)
pio-scripts/inject_syslog_ui.py (3)
12-15: LGTM: Proper PlatformIO script setupThe imports and environment setup are correct for a PlatformIO build script. The static analysis warnings about undefined
envare false positives sinceenvis imported via SCons.
21-31: LGTM: Clean state tracking implementationThe state tracking functions are well-implemented with proper file handling and clear logic for detecting build flag changes between builds.
149-154: LGTM: Proper PlatformIO integrationThe registration of post-action and conditional execution logic is correctly implemented for PlatformIO build scripts.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pio-scripts/inject_syslog_ui.py (1)
34-39: Consider moving the HTML template to a separate file for better maintainability.The hardcoded HTML snippet works but could become a maintenance burden if the UI structure changes frequently. Consider extracting it to a separate template file.
-# This is the HTML we want to inject -SYSLOG_HTML = """<h3>Syslog</h3> -<div id="Syslog"> - Enable Syslog: <input type="checkbox" name="SL_en"><br> - Host: <input type="text" name="SL_host" maxlength="32"><br> - Port: <input type="number" name="SL_port" min="1" max="65535" value="%SL_port%"><br> -</div>""" +def get_syslog_html(): + """Load syslog HTML template from file or return default.""" + template_path = os.path.join(os.path.dirname(__file__), "syslog_template.html") + if os.path.exists(template_path): + with open(template_path, 'r', encoding='utf-8') as f: + return f.read() + # Fallback to hardcoded template + return """<h3>Syslog</h3> +<div id="Syslog"> + Enable Syslog: <input type="checkbox" name="SL_en"><br> + Host: <input type="text" name="SL_host" maxlength="32"><br> + Port: <input type="number" name="SL_port" min="1" max="65535" value="%SL_port%"><br> +</div>"""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pio-scripts/inject_syslog_ui.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: KrX3D
PR: wled/WLED#4664
File: wled00/syslog.cpp:50-51
Timestamp: 2025-04-26T12:51:04.993Z
Learning: In the WLED codebase, syslog configuration variables (syslogHost, syslogPort, syslogProtocol, syslogFacility, syslogSeverity) are defined as global variables in wled.h and are accessed directly in the SyslogPrinter implementation, rather than being class members.
pio-scripts/inject_syslog_ui.py (2)
Learnt from: KrX3D
PR: wled/WLED#4664
File: wled00/syslog.cpp:50-51
Timestamp: 2025-04-26T12:51:04.993Z
Learning: In the WLED codebase, syslog configuration variables (syslogHost, syslogPort, syslogProtocol, syslogFacility, syslogSeverity) are defined as global variables in wled.h and are accessed directly in the SyslogPrinter implementation, rather than being class members.
Learnt from: Jason2866
PR: wled/WLED#4167
File: platformio.ini:427-430
Timestamp: 2025-07-06T16:11:05.874Z
Learning: In PlatformIO, using `-fno-lto` in `build_unflags` is a valid pattern to remove potentially existing `-fno-lto` flags from the build system before adding `-flto=auto` in `build_flags`. This ensures that LTO can be enabled without conflicts from default build system flags that might disable LTO.
🧬 Code Graph Analysis (1)
pio-scripts/inject_syslog_ui.py (1)
wled00/syslog.cpp (6)
write(92-104)write(92-92)write(106-108)write(106-106)write(110-192)write(110-110)
🪛 Ruff (0.11.9)
pio-scripts/inject_syslog_ui.py
18-18: Undefined name env
(F821)
149-149: Undefined name env
(F821)
153-153: Undefined name env
(F821)
🔇 Additional comments (6)
pio-scripts/inject_syslog_ui.py (6)
1-18: Script structure and imports look good.The SCons import pattern and build detection logic are correctly implemented. The static analysis warnings about undefined
envare false positives sinceenvis properly imported from SCons on line 15.
21-31: State tracking implementation is solid.The state file management correctly tracks syslog enablement between builds, which is essential for detecting when UI rebuilds are needed. The file operations are properly handled with explicit encoding.
76-116: Robust error handling and cleanup logic.The backup creation, injection logic, and error recovery are well-implemented. The script properly cleans up failed operations by removing backups, which prevents inconsistent states on subsequent builds.
118-134: Excellent retry mechanism with exponential backoff.The verification logic that checks for missing markers and implements retry with increasing delays is a thoughtful solution to handle transient file system issues. The retry limit prevents infinite loops.
86-101: Injection logic handles both explicit markers and fallback gracefully.The dual approach of first looking for the
<!-- SYSLOG-INJECT -->comment and falling back to inserting before the last<hr>tag provides good flexibility. The error handling when neither option works is appropriate.
149-153: Build integration is correctly implemented.The post-action registration ensures cleanup happens after every build, while the injection only runs on full builds. This prevents unnecessary file operations during incremental builds.
| SYSLOG_HTML = """<h3>Syslog</h3> | ||
| <div id="Syslog"> | ||
| Enable Syslog: <input type="checkbox" name="SL_en"><br> | ||
| Host: <input type="text" name="SL_host" maxlength="32"><br> | ||
| Port: <input type="number" name="SL_port" min="1" max="65535" value="%SL_port%"><br> | ||
| </div>""" |
There was a problem hiding this comment.
Now the html is so much smaller, i wonder if we really need the complication of this selective injection or if we should follow the pattern used for optional settings that is used for DMX Input support
There was a problem hiding this comment.
you mean move the html part to the settings_sync.htm with "hide function" and remove the injection py script?
like i had it in the begining and had to change it because it adds size?
WLED Syslog Component
Overview
Adds a SyslogPrinter to the WLED core that captures all debug-print output and forwards it—over UDP—to a remote syslog server, in any of three standard formats (BSD/RFC3164, RFC5424 or raw text). This makes it easy to integrate WLED logs into your existing centralized logging/monitoring infrastructure.
How it works
1. Print interception & buffering
flushBuffer()trims any trailing\r, null-terminates, and sends the complete message over UDP.2. Hostname resolution
resolveHostname()does a DNS lookup viaWiFi.hostByName()(orETH.hostByName()on Ethernet builds) and caches the resulting IP.3. Message formatting & UDP send
PRI) =(facility << 3) | severity.WiFiUDPto emit one packet per message.In RAW mode the message payload is same as WLED’s existing NetDebug output—without any PRI, timestamp or header added.
Example log output
Usage
Build-time configuration (platform.ini)
Enable debug output and syslog in your
platform.ini:Runtime configuration via GUI
In the WLED web interface, go to Settings → Sync (at the bottom) to enable/disable syslog and adjust:
You can also toggle syslog on/off under Info & Debug on the main page.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation