add rf traffic logging on local rf modules and remote hmlgw-interfaces#5
add rf traffic logging on local rf modules and remote hmlgw-interfaces#5jp112sdl wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds configurable RX/TX traffic logging for multimacd and rfd, daily log rotation, LAN-interface capture, and a CGI/browser interface for querying, decoding, filtering, analyzing, and exporting traffic logs. ChangesTraffic logging and RF traffic viewer
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant Multimacd
participant Rfd
participant TrafficLogger
participant TrafficLogFiles
Multimacd->>TrafficLogger: configure and record upstream/downstream frames
Rfd->>TrafficLogger: configure and record LAN RX/TX telegrams
TrafficLogger->>TrafficLogFiles: write daily traffic entries
sequenceDiagram
participant Browser
participant TrafficApi
participant TrafficLogs
Browser->>TrafficApi: request dates and log chunks
TrafficApi->>TrafficLogs: scan or read selected logs
TrafficApi-->>Browser: return dates or text lines
Browser->>TrafficApi: request device metadata
TrafficApi-->>Browser: return cached or rebuilt device list
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
jp112sdl
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/rfd/TrafficLogger.h (1)
46-46: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
_enabledshould bestd::atomic<bool>for consistency with the multimacd logger.The multimacd
TrafficLoggerusesstd::atomic<bool> _enabled(line 45 ofsrc/multimacd/TrafficLogger.h) explicitly for lock-free reads on the per-frame fast path. The rfd version uses a plainbool, which is a data race ifIsEnabled()is ever called from a different thread thanConfigure(). WhileConfigure()currently runs inmain()before the event loop (rfd.cpp:331-333), making this safe for single-threaded dispatch, the rfd concentrator handles multiple LAN gateway connections and could evolve to use per-connection threads. Aligning with the multimacd pattern prevents a future race.♻️ Proposed fix
- bool _enabled; + std::atomic<bool> _enabled;And add
#include <atomic>(already present in the multimacd header).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rfd/TrafficLogger.h` at line 46, Update the rfd TrafficLogger class member _enabled to std::atomic<bool> and add the required <atomic> header include, matching the multimacd TrafficLogger pattern while preserving existing Configure() and IsEnabled() behavior.src/multimacd/TrafficLogger.h (1)
44-53: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
_directoryis read on the fast path without synchronization.
_enabledis correctly atomic, but_directory(line 46) is a plainstd::stringread inGetLogFile/WriteLinewhileConfigure()writes it. This is safe only ifConfigure()is called once before any traffic. IfConfigure()is ever called at runtime (e.g., reload), concurrent reads of_directorywould be a data race. Consider documenting this constraint or copying_directoryunder the mutex inGetLogFile.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/multimacd/TrafficLogger.h` around lines 44 - 53, Protect runtime access to _directory in GetLogFile and WriteLine by reading or copying it under _mutex, while Configure() continues updating it under the same lock. Preserve the fast-path behavior of _enabled and ensure the copied directory remains valid after the lock is released.www/tools/rftrafficlogger.html (1)
905-908: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDebounce the high-frequency handlers.
fSearchfiresrenderon every keystroke ("input") andresizefiresrenderStatson every event. Both runfiltered()over up toMAX_KEEP(50000) telegrams and rebuild the table / redraw charts synchronously, which can visibly jank on large logs. A small debounce (~120–200 ms) keeps typing and window resizing smooth without changing behavior.♻️ Example debounce wrapper
+const debounce = (fn, ms = 150) => { let h; return (...a) => { clearTimeout(h); h = setTimeout(() => fn(...a), ms); }; }; +const renderDebounced = debounce(render); +const statsDebounced = debounce(renderStats); for (const id of ["fSearch", "fDir", "fProto", "fVia", "fType", "fDev", "fHideBcast"]) { - $(id).addEventListener(id === "fSearch" ? "input" : "change", render); + $(id).addEventListener(id === "fSearch" ? "input" : "change", id === "fSearch" ? renderDebounced : render); } -window.addEventListener("resize", () => { if (state.tab === "stats") renderStats(); }); +window.addEventListener("resize", () => { if (state.tab === "stats") statsDebounced(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@www/tools/rftrafficlogger.html` around lines 905 - 908, Debounce the high-frequency handlers in the filter-listener loop and the window resize listener: preserve immediate change handling for the non-search filters, but route fSearch input and resize events through approximately 120–200 ms delayed callbacks before invoking render or renderStats. Reuse a stable timer per handler so repeated events cancel the prior pending invocation without changing the final rendered behavior.www/tools/rftrafficlogger-api.cgi (1)
230-238: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAvoid caching an empty/failed device list.
If
rega_scriptthrows (line 178catch) or returns noSTDOUT,buildDevListstill returns a well-formed JSON with an emptydevicesarray.cmdDevListthen persists that empty result toCACHEFILE, so device names stay unresolved for up toCACHETTL(600 s) even after ReGa recovers — the user must manually hit "Geräte ↻" to break out. Only cache when the list is actually populated.♻️ Only persist a non-empty result
if {$json eq ""} { set json [buildDevList] - catch { - set fh [open $CACHEFILE w] - fconfigure $fh -encoding utf-8 - puts -nonewline $fh $json - close $fh - } + # nur cachen, wenn die Geraeteliste wirklich befuellt ist + if {[string match {*"devices":\[{*} $json]} { + catch { + set fh [open $CACHEFILE w] + fconfigure $fh -encoding utf-8 + puts -nonewline $fh $json + close $fh + } + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@www/tools/rftrafficlogger-api.cgi` around lines 230 - 238, Update the caching block in cmdDevList so CACHEFILE is written only when buildDevList returns a populated device list, not merely any non-empty JSON string. Inspect the parsed devices collection or equivalent result and skip the open/write operations when it is empty, while preserving the existing response behavior and cache handling for populated lists.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/multimacd/TrafficLogger.h`:
- Around line 44-53: Protect runtime access to _directory in GetLogFile and
WriteLine by reading or copying it under _mutex, while Configure() continues
updating it under the same lock. Preserve the fast-path behavior of _enabled and
ensure the copied directory remains valid after the lock is released.
In `@src/rfd/TrafficLogger.h`:
- Line 46: Update the rfd TrafficLogger class member _enabled to
std::atomic<bool> and add the required <atomic> header include, matching the
multimacd TrafficLogger pattern while preserving existing Configure() and
IsEnabled() behavior.
In `@www/tools/rftrafficlogger-api.cgi`:
- Around line 230-238: Update the caching block in cmdDevList so CACHEFILE is
written only when buildDevList returns a populated device list, not merely any
non-empty JSON string. Inspect the parsed devices collection or equivalent
result and skip the open/write operations when it is empty, while preserving the
existing response behavior and cache handling for populated lists.
In `@www/tools/rftrafficlogger.html`:
- Around line 905-908: Debounce the high-frequency handlers in the
filter-listener loop and the window resize listener: preserve immediate change
handling for the non-search filters, but route fSearch input and resize events
through approximately 120–200 ms delayed callbacks before invoking render or
renderStats. Reuse a stable timer per handler so repeated events cancel the
prior pending invocation without changing the final rendered behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 80674d2b-f1b3-4df7-9d73-d273c5b4f111
📒 Files selected for processing (17)
src/multimacd/MultimacManager.cppsrc/multimacd/Subsystem.cppsrc/multimacd/SubsystemManager.cppsrc/multimacd/TrafficLogger.cppsrc/multimacd/TrafficLogger.hsrc/multimacd/UpstreamCharConnection.cppsrc/multimacd/multimacd.confsrc/multimacd/multimacd.cppsrc/rfd/BidcosInterface.cppsrc/rfd/BidcosInterface.hsrc/rfd/BidcosInterfaceConcentrator.cppsrc/rfd/CMakeLists.txtsrc/rfd/TrafficLogger.cppsrc/rfd/TrafficLogger.hsrc/rfd/rfd.cppwww/tools/rftrafficlogger-api.cgiwww/tools/rftrafficlogger.html
2 participants
🌎 WebUI call
http://<openccu>/tools/rftrafficlogger.html🚧 TODO
10 0 * * * find /var/log -name "multimacd-traffic-*.log" -mtime +7 -deletemultimacd.confandrfd.confare neededTraffic Log = 0/1Traffic Log Directory = /var/logSummary by CodeRabbit