-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Adds hostname (serverDescription) to log in net debug #4650
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…create the right log file. Fix for wled#4647
WalkthroughThe changes refactor the UDP debug output logic in the Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wled00/net_debug.cpp (1)
43-63
: Improved multi-byte write implementation.The buffer-based approach is well implemented, with appropriate handling of newlines and buffer size limits. The code effectively processes the input buffer and flushes at logical points.
One minor improvement opportunity: consider moving the
hasNewline
tracking to a local scope if it's only needed for the final check.// Check if we can find a newline in the buffer - bool hasNewline = false; size_t lastNewlinePos = 0; for (size_t i = 0; i < size; i++) { buffer += (char)buf[i]; if (buf[i] == '\n') { - hasNewline = true; lastNewlinePos = i; flushBuffer(); } } // If buffer is large but no newline was found, flush anyway - if (!hasNewline && buffer.length() >= MAX_BUFFER_SIZE) { + if (buffer.length() >= MAX_BUFFER_SIZE) { flushBuffer(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wled00/net_debug.cpp
(2 hunks)wled00/net_debug.h
(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
wled00/net_debug.cpp (1)
wled00/net_debug.h (2)
c
(16-16)buf
(17-17)
🔇 Additional comments (4)
wled00/net_debug.h (1)
11-14
: Good additions for implementing buffered network debug output.The new buffer, size limit, and private methods provide a clean structure for improving the network debug functionality. Using a buffer with conditional flushing will reduce network overhead compared to sending individual packets for each character or small write.
wled00/net_debug.cpp (3)
5-17
: Well-organized hostname resolution.This helper method centralizes the hostname resolution logic that was likely duplicated in the previous implementation. Good use of conditional compilation for different platforms.
19-28
: Well-implemented buffer flushing with hostname prefix.The
flushBuffer
method effectively accomplishes the PR objective by adding the hostname (serverDescription
) as a prefix to each debug message. Good job checking for empty buffer to avoid unnecessary network operations.
30-41
: Effective buffering implementation for single character writes.The single-character write method now buffers characters and flushes at appropriate points (newlines or size limits), which is more efficient than sending individual UDP packets.
The title and description don't really match you code changes. Adding a buffer is a significant change to behavior and resolving your hostname for every message will give poor performance |
Hi, so i will revert the net debug back to the original. I created a seperate syslog.h/.cpp which can used and is able to use the format RFC3164/5424 or even send it raw like in the net debug (using a bufer) also being able to change the ip,port etc is now possible from the gui i will update this PR in the next days with the new files |
Hi, im closing this since like i said i created something seperate to the net debug here: for syslog there is also an option RAW which should send like the net debug the output |
Fix for #4647
Adds the hostname (serverDescription) to the send output.
Output before:
Apr 16 20:46:20 Segment geometry: 0,30 -> 0,1
With this fix:
Apr 19 16:06:20 EleksTube Wifi state: 3
So now the syslog server creates only one file syslog-EleksTube.log instead of multiple (see picture in #4647)
Summary by CodeRabbit