-
Notifications
You must be signed in to change notification settings - Fork 10
[RDK-E]: NetworkconnectionRecovery.sh migration to C/C++ Module #274
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
base: develop
Are you sure you want to change the base?
Conversation
Signed-off-by: Balaji Punnuru <Balaji_Punnuru@comcast.com>
Signed-off-by: Balaji Punnuru <Balaji_Punnuru@comcast.com>
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
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.
Pull request overview
This PR introduces a new NetworkConnectionStats Thunder/WPEFramework module (in-process plugin + out-of-process implementation) intended to replace shell-based recovery/diagnostics with a C/C++ implementation that queries NetworkManager via COM-RPC/JSON-RPC and emits telemetry.
Changes:
- Adds an out-of-process diagnostics implementation that periodically generates network reports and subscribes to NetworkManager events.
- Adds two NetworkManager provider implementations (COM-RPC and JSON-RPC) plus a runtime factory selector.
- Integrates the new
networkstats/component into the build and adds configuration/docs assets.
Reviewed changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 24 comments.
Show a summary per file
| File | Description |
|---|---|
networkstats/plugin/ThunderJsonRPCProvider.h |
Declares JSON-RPC-based NetworkManager provider. |
networkstats/plugin/ThunderJsonRPCProvider.cpp |
Implements NetworkManager JSON-RPC calls (GetIPSettings, Ping, event subscribe). |
networkstats/plugin/ThunderComRPCProvider.h |
Declares COM-RPC-based NetworkManager provider. |
networkstats/plugin/ThunderComRPCProvider.cpp |
Implements NetworkManager COM-RPC calls (GetIPSettings, Ping, event subscribe). |
networkstats/plugin/NetworkDataProviderFactory.h |
Factory to select COM-RPC vs JSON-RPC provider based on config. |
networkstats/plugin/NetworkConnectionStatsLogger.h |
Adds a lightweight logging wrapper/macros for the module. |
networkstats/plugin/NetworkConnectionStatsLogger.cpp |
Implements stdout/RDK logger backend for logging wrapper. |
networkstats/plugin/NetworkConnectionStatsImplementation.h |
Declares out-of-process implementation class, threading, and event handlers. |
networkstats/plugin/NetworkConnectionStatsImplementation.cpp |
Implements periodic report generation, telemetry emission, and event-driven triggers. |
networkstats/plugin/NetworkConnectionStats.h |
Declares in-process plugin shell that spawns/aggregates the implementation. |
networkstats/plugin/NetworkConnectionStats.cpp |
Implements plugin lifecycle wiring and COM-RPC aggregation. |
networkstats/plugin/NetworkConnectionStats.config |
Adds example Thunder plugin JSON configuration. |
networkstats/plugin/NetworkConnectionStats.conf.in |
Adds config template for CMake write_config() generation. |
networkstats/plugin/Module.h |
Adds module header wiring for Thunder build macros/includes. |
networkstats/plugin/Module.cpp |
Adds module declaration for build reference. |
networkstats/plugin/INetworkData.h |
Defines common interface for network data providers. |
networkstats/plugin/CMakeLists.txt |
Builds the plugin + implementation libraries and installs config. |
networkstats/interface/INetworkConnectionStats.h |
Defines COM-RPC interface for the out-of-process implementation. |
networkstats/interface/CMakeLists.txt |
Generates ProxyStub library for the interface. |
networkstats/definition/NetworkConnectionStats.json |
Adds JSON-RPC API definition/documentation (currently not wired into build). |
networkstats/definition/CMakeLists.txt |
Adds JsonGenerator integration for docs/stubs generation. |
networkstats/THUNDER_PLUGIN_QUICK_REFERENCE.md |
Adds quick-reference documentation for the intended architecture. |
networkstats/THUNDER_PLUGIN_CONVERSION.md |
Adds conversion notes from standalone app to Thunder module. |
networkstats/README_INTERNAL_PLUGIN.md |
Describes internal-only plugin behavior/configuration and telemetry outputs. |
networkstats/NetworkStatsDesign_ver1.md |
Provides high-level design diagrams and operational flow. |
networkstats/CMakeLists.txt |
Adds networkstats sub-project build entrypoint. |
CMakeLists.txt |
Adds USE_TELEMETRY option and includes networkstats in the top-level build. |
.DS_Store |
Adds an unintended macOS metadata file. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #ifndef __THUNDERPROVIDER_H__ | ||
| #define __THUNDERPROVIDER_H__ |
Copilot
AI
Feb 10, 2026
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.
The include guard macro name starts with double underscores (THUNDERPROVIDER_H), which is reserved for the implementation in C/C++. Please rename it to a non-reserved identifier (e.g., THUNDER_JSONRPC_PROVIDER_H).
| #ifndef __THUNDERCOMRPCPROVIDER_H__ | ||
| #define __THUNDERCOMRPCPROVIDER_H__ |
Copilot
AI
Feb 10, 2026
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.
The include guard macro (THUNDERCOMRPCPROVIDER_H) begins with double underscores, which are reserved identifiers in C/C++. Please rename it to a non-reserved macro to avoid undefined behavior/toolchain issues.
| /* @brief Get DNS server entries */ | ||
| std::string NetworkJsonRPCProvider::getDnsEntries() | ||
| { | ||
| // TODO: Implement DNS entries retrieval | ||
| return ""; | ||
| } |
Copilot
AI
Feb 10, 2026
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.
getDnsEntries() is currently a stub that always returns an empty string. Since this provider can be selected at runtime via providerType=jsonrpc, this will lead to incorrect diagnostics when that provider is used; please implement DNS retrieval (or remove the method from the interface if it’s not needed).
| struct tm* lt; | ||
| const char* fileName = trimPath(file); | ||
|
|
||
| if (gDefaultLogLevel < level) | ||
| return; | ||
|
|
||
| gettimeofday(&tv, NULL); | ||
| lt = localtime(&tv.tv_sec); | ||
|
|
||
| printf("%.2d:%.2d:%.2d.%.6lld [%-5s] [PID=%d] [TID=%d] [%s +%d] %s : %s\n", lt->tm_hour, lt->tm_min, lt->tm_sec, (long long int)tv.tv_usec, levelMap[level], getpid(), gettid(), fileName, line, func, formattedLog); |
Copilot
AI
Feb 10, 2026
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.
logPrint() uses localtime(), which is not thread-safe and can cause data races/corrupted timestamps when logging from multiple threads (this plugin starts several threads). Please use localtime_r() (or gmtime_r()) with a stack-allocated tm instead.
| struct tm* lt; | |
| const char* fileName = trimPath(file); | |
| if (gDefaultLogLevel < level) | |
| return; | |
| gettimeofday(&tv, NULL); | |
| lt = localtime(&tv.tv_sec); | |
| printf("%.2d:%.2d:%.2d.%.6lld [%-5s] [PID=%d] [TID=%d] [%s +%d] %s : %s\n", lt->tm_hour, lt->tm_min, lt->tm_sec, (long long int)tv.tv_usec, levelMap[level], getpid(), gettid(), fileName, line, func, formattedLog); | |
| struct tm lt; | |
| const char* fileName = trimPath(file); | |
| if (gDefaultLogLevel < level) | |
| return; | |
| gettimeofday(&tv, NULL); | |
| localtime_r(&tv.tv_sec, <); | |
| printf("%.2d:%.2d:%.2d.%.6lld [%-5s] [PID=%d] [TID=%d] [%s +%d] %s : %s\n", lt.tm_hour, lt.tm_min, lt.tm_sec, (long long int)tv.tv_usec, levelMap[level], getpid(), gettid(), fileName, line, func, formattedLog); |
| printf("%.2d:%.2d:%.2d.%.6lld [%-5s] [PID=%d] [TID=%d] [%s +%d] %s : %s\n", lt->tm_hour, lt->tm_min, lt->tm_sec, (long long int)tv.tv_usec, levelMap[level], getpid(), gettid(), fileName, line, func, formattedLog); | ||
| fflush(stdout); |
Copilot
AI
Feb 10, 2026
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.
This code calls gettid(), but gettid() is not a portable/POSIX API and may be unavailable depending on libc/kernel headers, causing build failures. Consider using syscall(SYS_gettid) (since you already include <sys/syscall.h>) or conditionally compiling the TID output.
| #### COM-RPC Interface | ||
| - **INetworkConnectionStats.h** - Defines the COM-RPC interface with: | ||
| - INetworkConnectionStats interface with all network diagnostic methods | ||
| - INotification sub-interface for event notifications | ||
| - Proper interface IDs and stubgen annotations | ||
|
|
||
| #### Plugin Main Class | ||
| - **NetworkConnectionStats.h** - Plugin header with: | ||
| - Inherits from `PluginHost::IPlugin` and `PluginHost::JSONRPC` | ||
| - Notification handler for RPC connection lifecycle and events | ||
| - Interface aggregation for COM-RPC | ||
|
|
||
| - **NetworkConnectionStats.cpp** - Plugin implementation with: | ||
| - IPlugin lifecycle methods (Initialize, Deinitialize, Information) | ||
| - RPC connection management | ||
| - JSON-RPC registration | ||
| - Notification forwarding |
Copilot
AI
Feb 10, 2026
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.
This conversion document states the plugin inherits PluginHost::JSONRPC, registers JSON-RPC methods, and provides a JSON-RPC API spec. The current codebase implements an internal-only IPlugin + COM-RPC aggregate and the networkstats/definition subdir is not built. Please update the document to reflect the implemented architecture (or implement the described JSON-RPC layer).
| { | ||
| "$schema": "interface.schema.json", | ||
| "jsonrpc": "2.0", | ||
| "info": { | ||
| "title": "NetworkConnectionStats API", | ||
| "class": "NetworkConnectionStats", | ||
| "description": "NetworkConnectionStats plugin for network diagnostics and monitoring" | ||
| }, |
Copilot
AI
Feb 10, 2026
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.
This JSON-RPC definition describes a public API surface (methods/events) that is not implemented by the current plugin (which is internal-only and does not derive from PluginHost::JSONRPC). Please either update this spec to match the implemented interfaces, or remove/disable it to avoid generating misleading documentation/stubs later.
| } else { | ||
| auto factoryType = NetworkDataProviderFactory::ParseProviderType(providerType); | ||
| std::string typeName = NetworkDataProviderFactory::GetProviderTypeName(factoryType); | ||
| NSLOG_ERROR("Failed to initialize %s provider", typeName.c_str()); |
Copilot
AI
Feb 10, 2026
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.
If m_provider->Initialize() fails, Configure() returns an error but leaves m_provider allocated and in a partially initialized state. Please delete/reset m_provider (and avoid starting any threads) on this failure path to prevent leaks and accidental later use.
| NSLOG_ERROR("Failed to initialize %s provider", typeName.c_str()); | |
| NSLOG_ERROR("Failed to initialize %s provider", typeName.c_str()); | |
| // Clean up the partially initialized provider to prevent leaks and later accidental use | |
| delete m_provider; | |
| m_provider = nullptr; |
| #include <atomic> | ||
| #include <queue> | ||
| #include <mutex> | ||
| #include <condition_variable> |
Copilot
AI
Feb 10, 2026
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.
NetworkConnectionStatsImplementation.h uses std::list (_notificationCallbacks) but does not include . Please add the missing standard header to avoid compile failures depending on indirect includes.
| #include <condition_variable> | |
| #include <condition_variable> | |
| #include <list> |
| #include "INetworkData.h" | ||
| #include "Module.h" | ||
| #include <string> | ||
| #include <core/JSON.h> |
Copilot
AI
Feb 10, 2026
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.
This header declares std::shared_ptr (m_networkManagerClient) but never includes . Please include here to avoid build failures due to indirect include order.
| #include <core/JSON.h> | |
| #include <core/JSON.h> | |
| #include <memory> | |
| #include <functional> |
|
b'## WARNING: A Blackduck scan failure has been waived A prior failure has been upvoted
|
|
I have checked the fossid failures: There is one match to rdkcentral which is not of interest. The other two matches are from jmanc3/winbar and Karunakaran has pointed out that jmanc3 has used original code from rdkcentral (committed 17-Nov-24) which jmanc3 then committed several months later on 16-Jun-25. So the matches are a non-issue. |
3fd5c23 to
b705641
Compare
No description provided.