[RDK-E]: NetworkconnectionRecovery.sh migration to C/C++ Module#274
[RDK-E]: NetworkconnectionRecovery.sh migration to C/C++ Module#274udaykrishnag wants to merge 17 commits intodevelopfrom
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.
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__ |
There was a problem hiding this comment.
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__ |
There was a problem hiding this comment.
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 ""; | ||
| } |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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" | ||
| }, |
There was a problem hiding this comment.
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.
| #include <atomic> | ||
| #include <queue> | ||
| #include <mutex> | ||
| #include <condition_variable> |
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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. |
udaykrishnag
force-pushed
the
topic/RDK-59986
branch
from
3fd5c23 to
b705641
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 28 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
CMakeLists.txt
Outdated
| option(ENABLE_LEGACY_PLUGINS "Enable Legacy Plugins" ON) | ||
| option(USE_RDK_LOGGER "Enable RDK Logger for logging" OFF ) | ||
| option(ENABLE_UNIT_TESTING "Enable unit tests" OFF) | ||
| option(USE_TELEMETRY "Enable Telemetry support for NetworkConnectionStats" ON) |
There was a problem hiding this comment.
This top-level option introduces USE_TELEMETRY defaulting to ON. USE_TELEMETRY is also used by other targets in this repo (e.g., tools/upnp), so this change will implicitly enable telemetry (and add a hard dependency on T2) beyond NetworkConnectionStats. Consider defaulting this option to OFF, or using a more scoped option name (e.g., NETWORKSTATS_USE_TELEMETRY) to avoid changing behavior of unrelated components.
|
|
||
| // Notification callbacks | ||
| std::list<Exchange::INetworkConnectionStats::INotification*> _notificationCallbacks; | ||
| Core::CriticalSection _notificationLock; |
There was a problem hiding this comment.
This header uses std::list for _notificationCallbacks but does not include <list>. Please include the required standard header directly here (avoid relying on transitive includes from Thunder headers).
| │ │ - Inherits IPlugin + JSONRPC │ │ | ||
| │ │ - Manages RPC connection │ │ | ||
| │ │ - Forwards JSON-RPC calls │ │ | ||
| │ │ - Handles notifications │ │ | ||
| │ └────────────────┬───────────────────────────────────────────┘ │ | ||
| │ │ COM-RPC │ |
There was a problem hiding this comment.
This quick reference states the plugin inherits PluginHost::JSONRPC and exposes many JSON-RPC methods/events, but the implementation in this PR is IPlugin-only and the COM-RPC interface only defines Configure/Register/Unregister. Please update this document to match the actual code (or implement the documented APIs) to avoid misleading integrators.
| │ │ - Inherits IPlugin + JSONRPC │ │ | |
| │ │ - Manages RPC connection │ │ | |
| │ │ - Forwards JSON-RPC calls │ │ | |
| │ │ - Handles notifications │ │ | |
| │ └────────────────┬───────────────────────────────────────────┘ │ | |
| │ │ COM-RPC │ | |
| │ │ - Implements IPlugin │ │ | |
| │ │ - Manages COM-RPC connection │ │ | |
| │ │ - Uses COM-RPC to talk to implementation │ │ | |
| │ └────────────────┬───────────────────────────────────────────┘ │ | |
| │ │ COM-RPC (INetworkConnectionStats) │ |
| configuration = JSON() | ||
| configuration.add("root", process) | ||
| configuration.add("reportingInterval", "@PLUGIN_NETWORKCONNECTIONSTATS_REPORTING_INTERVAL@") | ||
| configuration.add("autoStart", True) |
There was a problem hiding this comment.
configuration.add("autoStart", True) doesn’t match the format used by other *.conf.in files in this repo (they use quoted strings like "true" or CMake substitutions). True is likely invalid for this config templating and may break config generation/parsing.
| configuration.add("autoStart", True) | |
| configuration.add("autoStart", "@PLUGIN_NETWORKCONNECTIONSTATS_AUTOSTART@") |
| uint32_t Register(INetworkConnectionStats::INotification* notification) override; | ||
| uint32_t Unregister(INetworkConnectionStats::INotification* notification) override; |
There was a problem hiding this comment.
Register/Unregister use INetworkConnectionStats::INotification* but INetworkConnectionStats is declared under WPEFramework::Exchange. As written, this won’t resolve in WPEFramework::Plugin and should be fully qualified (e.g., Exchange::INetworkConnectionStats::INotification*) to match the inherited interface.
| uint32_t Register(INetworkConnectionStats::INotification* notification) override; | |
| uint32_t Unregister(INetworkConnectionStats::INotification* notification) override; | |
| uint32_t Register(Exchange::INetworkConnectionStats::INotification* notification) override; | |
| uint32_t Unregister(Exchange::INetworkConnectionStats::INotification* notification) override; |
| // Create network provider using factory | ||
| m_provider = NetworkDataProviderFactory::CreateProvider(providerType); | ||
| if (m_provider == nullptr) { |
There was a problem hiding this comment.
Configure() assigns a new object to m_provider without first cleaning up any existing provider instance (and any running threads/subscriptions tied to it). If Configure() is called more than once, this will leak and can leave multiple background threads running. Clean up the previous state before replacing m_provider.
|
|
||
| #include "Module.h" | ||
|
|
||
| MODULE_NAME_DECLARATION(BUILD_REFERENCE) |
There was a problem hiding this comment.
MODULE_NAME_DECLARATION(BUILD_REFERENCE) looks inconsistent with the rest of this repo, which uses PLUGIN_BUILD_REFERENCE (and this module’s CMake defines -DPLUGIN_BUILD_REFERENCE=...). If BUILD_REFERENCE isn’t defined, this will fail to compile or embed the wrong build reference.
| MODULE_NAME_DECLARATION(BUILD_REFERENCE) | |
| MODULE_NAME_DECLARATION(PLUGIN_BUILD_REFERENCE) |
| /* Test main function to validate NetworkJsonRPCProvider member functions */ | ||
| int main() | ||
| { | ||
| NetworkJsonRPCProvider provider; |
There was a problem hiding this comment.
The JSON-RPC provider test main() constructs NetworkJsonRPCProvider but never calls Initialize(). As a result, the menu actions will hit "client not initialized" and return empty/false. Call provider.Initialize() once before entering the menu (as done in the COM-RPC provider test).
| NetworkJsonRPCProvider provider; | |
| NetworkJsonRPCProvider provider; | |
| uint32_t initResult = provider.Initialize(); | |
| if (initResult != WPEFramework::Core::ERROR_NONE) { | |
| std::cerr << "Failed to initialize NetworkJsonRPCProvider, error code: " << initResult << std::endl; | |
| return 1; | |
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 28 changed files in this pull request and generated 15 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @param interface_name Interface name (e.g., eth0, wlan0) | ||
| * @return IPv4 address string | ||
| */ | ||
| std::string getIpv4Address(std::string interface_name) override; |
There was a problem hiding this comment.
String parameters should be passed by const reference to avoid unnecessary copies. Change std::string interface_name to const std::string& interface_name.
| int choice; | ||
| std::string interface_name; | ||
| bool running = true; | ||
|
|
There was a problem hiding this comment.
The Initialize method should be called before using the provider in the test main function. Add a check and call to provider.Initialize() before running tests to ensure proper initialization.
| const uint32_t initResult = provider.Initialize(); | |
| if (initResult != WPEFramework::Core::ERROR_NONE) { | |
| std::cerr << "Failed to initialize NetworkJsonRPCProvider, error code: " << initResult << std::endl; | |
| return -1; | |
| } |
|
|
||
| uint32_t NetworkConnectionStatsImplementation::Configure(const string configLine) | ||
| { | ||
| NSLOG_INFO("NetworkConnectionStatsImplementation::Configure"); |
There was a problem hiding this comment.
Missing error handling: If m_provider is deleted due to initialization failure but Configure() is called again (which shouldn't happen but could in error scenarios), m_provider will be nullptr and any subsequent access will cause crashes. Consider adding a check at the beginning of Configure() to prevent re-configuration or handle the re-initialization case properly.
| NSLOG_INFO("NetworkConnectionStatsImplementation::Configure"); | |
| NSLOG_INFO("NetworkConnectionStatsImplementation::Configure"); | |
| // Ensure provider is available before proceeding. If this occurs, | |
| // it likely indicates an earlier initialization failure or misuse. | |
| if (m_provider == nullptr) { | |
| NSLOG_ERROR("Configure called while provider is not initialized (m_provider is nullptr)"); | |
| return Core::ERROR_GENERAL; | |
| } |
| if (result.HasLabel("success")) { | ||
| bool success = result["success"].Boolean(); | ||
|
|
||
| // Extract ping statistics from JSON |
There was a problem hiding this comment.
Comment mismatch: The comment says "Extract ping statistics from JSON" but the field being accessed is "success" which is a Boolean, not statistics. Consider updating the comment to accurately reflect the code flow.
| // Extract ping statistics from JSON | |
| // Retrieve ping result status and statistics from JSON response |
| std::cout << "Primary DNS: " << (primaryDns.empty() ? "(empty)" : primaryDns) << "\n"; | ||
| } | ||
|
|
||
| std::cout << "\n[Test 6/7] getIpv6Address()\n"; |
There was a problem hiding this comment.
Test count mismatch: The text says "Test 6/7" but there are only 6 tests shown (getInterface, getIpv4Address, getIpv6Address, getConnectionType, getDnsEntries, populateNetworkData, pingToGatewayCheck). Either the label should be "Test 6/7" or the comment should reflect the actual number of tests.
| std::cout << "\n[Test 6/7] getIpv6Address()\n"; | |
| std::cout << "\n[Test 6/6] getIpv6Address()\n"; |
| { | ||
| try { | ||
| // Set Thunder access point | ||
| WPEFramework::Core::SystemInfo::SetEnvironment(_T("THUNDER_ACCESS"), _T("127.0.0.1:9998")); |
There was a problem hiding this comment.
Security concern: The THUNDER_ACCESS environment variable is hardcoded to "127.0.0.1:9998". If this plugin runs in an environment where Thunder is bound to a different address or port, or if the environment variable is already set, this could cause connection failures or unexpected behavior. Consider making this configurable through the plugin configuration or checking if the environment variable is already set before overwriting it.
| std::string getIpv4Address(std::string interface_name) override; | ||
|
|
||
| /* @brief Retrieve IPv6 address for specified interface | ||
| * @param interface_name Interface name (e.g., eth0, wlan0) | ||
| * @return IPv6 address string | ||
| */ | ||
| std::string getIpv6Address(std::string interface_name) override; | ||
|
|
||
| /* @brief Get IPv4 gateway/route address from last getIpv4Address call */ | ||
| std::string getIpv4Gateway() override; | ||
|
|
||
| /* @brief Get IPv6 gateway/route address from last getIpv6Address call */ | ||
| std::string getIpv6Gateway() override; | ||
|
|
||
| /* @brief Get IPv4 primary DNS from last getIpv4Address call */ | ||
| std::string getIpv4PrimaryDns() override; | ||
|
|
||
| /* @brief Get IPv6 primary DNS from last getIpv6Address call */ | ||
| std::string getIpv6PrimaryDns() override; | ||
|
|
||
| /* @brief Get current network connection type */ | ||
| std::string getConnectionType() override; | ||
|
|
||
| /* @brief Get DNS server entries */ | ||
| std::string getDnsEntries() override; | ||
|
|
||
| /* @brief Populate network interface data */ | ||
| void populateNetworkData() override; | ||
|
|
||
| /* @brief Get current active interface name */ | ||
| std::string getInterface() override; | ||
|
|
||
| /* @brief Ping to gateway to check packet loss | ||
| * @param endpoint Gateway IP address to ping | ||
| * @param ipversion Either "IPv4" or "IPv6" | ||
| * @param count Number of ping packets to send | ||
| * @param timeout Timeout in seconds | ||
| * @return true if ping successful, false otherwise | ||
| */ | ||
| bool pingToGatewayCheck(std::string endpoint, std::string ipversion, int count, int timeout) override; | ||
|
|
||
| /* @brief Get packet loss from last ping call */ | ||
| std::string getPacketLoss() override; | ||
|
|
||
| /* @brief Get average RTT from last ping call */ | ||
| std::string getAvgRtt() override; | ||
|
|
||
| /* @brief Subscribe to NetworkManager events | ||
| * @param eventName Name of the event (e.g., "onInterfaceStateChange") | ||
| * @param callback Callback function to be called when event fires | ||
| * @return Error code (Core::ERROR_NONE on success) | ||
| */ | ||
| uint32_t SubscribeToEvent(const std::string& eventName, | ||
| std::function<void(const WPEFramework::Core::JSON::VariantContainer&)> callback) override; |
There was a problem hiding this comment.
String parameters should be passed by const reference to avoid unnecessary copies. Change all std::string parameters to const std::string& throughout the interface methods.
| // Clean up the partially initialized provider to prevent leaks and later accidental use | ||
| delete m_provider; | ||
| m_provider = nullptr; | ||
| result = Core::ERROR_GENERAL; |
There was a problem hiding this comment.
Potential memory leak: When initialization fails (lines 176-177 and 209-211), m_provider is deleted but not set to nullptr. If Configure is called again or if other methods check m_provider before deletion in the destructor, this could cause use-after-free or double-free issues. After deleting m_provider, set it to nullptr.
udaykrishnag
force-pushed
the
topic/RDK-59986
branch
from
d9a28a8 to
c555c21
Compare
udaykrishnag
force-pushed
the
topic/RDK-59986
branch
from
c555c21 to
4c0fe1b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 28 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* @brief Get DNS server entries */ | ||
| std::string NetworkJsonRPCProvider::getDnsEntries() | ||
| { | ||
| // TODO: Implement DNS entries retrieval | ||
| return ""; | ||
| } | ||
|
|
||
| /* @brief Populate network interface data */ | ||
| void NetworkJsonRPCProvider::populateNetworkData() | ||
| { | ||
| // TODO: Implement network data population | ||
| } |
There was a problem hiding this comment.
getDnsEntries() is currently a TODO that always returns an empty string, while the COM-RPC provider returns the cached IPv4/IPv6 DNS values. This makes provider behavior inconsistent depending on configuration. Please implement this method (and populateNetworkData()) similarly to NetworkComRPCProvider or remove them from the interface if they’re not needed.
| // Get provider type from configuration (default: comrpc) | ||
| std::string providerType = "comrpc"; | ||
| if (config.HasLabel("providerType")) { | ||
| providerType = config["providerType"].Value(); | ||
| } | ||
|
|
||
| // Create network provider using factory | ||
| m_provider = NetworkDataProviderFactory::CreateProvider(providerType); | ||
| if (m_provider == nullptr) { | ||
| NSLOG_ERROR("Failed to create network provider for type: %s", providerType.c_str()); | ||
| result = Core::ERROR_GENERAL; | ||
| } else { | ||
| auto factoryType = NetworkDataProviderFactory::ParseProviderType(providerType); | ||
| std::string typeName = NetworkDataProviderFactory::GetProviderTypeName(factoryType); | ||
| NSLOG_INFO("%s provider created", typeName.c_str()); | ||
|
|
||
| // Initialize the provider | ||
| if (m_provider->Initialize()) { | ||
| NSLOG_INFO("%s provider initialized successfully", typeName.c_str()); | ||
|
|
||
| // Subscribe to NetworkManager events (with automatic retry) | ||
| _stopSubscriptionRetry = false; | ||
| subscribeToEvents(); | ||
| _subscriptionRetryThread = std::thread(&NetworkConnectionStatsImplementation::subscriptionRetryThread, this); | ||
|
|
||
| // Generate initial report | ||
| generateReport(); | ||
|
|
||
| // Start timer and consumer threads if enabled | ||
| if (_periodicReportingEnabled) { | ||
| _stopReporting = false; | ||
| _timerThread = std::thread(&NetworkConnectionStatsImplementation::timerThread, this); | ||
| _reportingThread = std::thread(&NetworkConnectionStatsImplementation::periodicReportingThread, this); | ||
| NSLOG_INFO("Timer and reporting threads started with %u second interval (%u minutes)", | ||
| _reportingIntervalSeconds.load(), _reportingIntervalSeconds.load() / 60); | ||
| } | ||
| } else { | ||
| auto factoryType = NetworkDataProviderFactory::ParseProviderType(providerType); | ||
| std::string typeName = NetworkDataProviderFactory::GetProviderTypeName(factoryType); | ||
| 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; | ||
| result = Core::ERROR_GENERAL; | ||
| } | ||
| } |
There was a problem hiding this comment.
Configure() overwrites m_provider without cleaning up any existing instance. If Thunder calls Configure() more than once (e.g., reconfiguration), the previous provider will leak and any existing subscriptions/threads may reference stale state. Please delete/reset the existing provider (and stop any running threads) before creating a new one, or guard against multiple Configure calls.
| - 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 | ||
|
|
||
| #### Out-of-Process Implementation | ||
| - **NetworkConnectionStatsImplementation.h/.cpp** - Business logic implementation: | ||
| - All original diagnostic functionality preserved | ||
| - Implements INetworkConnectionStats and IConfiguration interfaces | ||
| - Notification management for event subscribers | ||
| - Periodic reporting thread with configurable interval | ||
| - Telemetry integration (T2) | ||
| - Network provider integration (NetworkJsonRPCProvider) | ||
|
|
||
| #### API Specification | ||
| - **NetworkConnectionStats.json** - JSON-RPC API definition: | ||
| - Method definitions with parameters and return types | ||
| - Event definitions (onReportGenerated) | ||
| - Schema-compliant for Thunder code generation | ||
|
|
||
| #### Configuration Files | ||
| - **NetworkConnectionStats.config** - Plugin configuration (JSON format) | ||
| - **NetworkConnectionStats.conf.in** - CMake template for configuration generation | ||
|
|
||
| #### Build System | ||
| - **CMakeLists.txt** - Thunder-compliant build system: | ||
| - Interface library for COM-RPC definitions | ||
| - Plugin library (in-process) | ||
| - Implementation library (out-of-process) | ||
| - Proper dependency management | ||
| - Installation rules for plugins, headers, and API specs | ||
|
|
||
| ## API Methods Available | ||
|
|
||
| ### Diagnostic Methods | ||
| 1. **generateReport()** - Generate full network diagnostics report | ||
| 2. **getConnectionType()** - Get Ethernet/WiFi connection type | ||
| 3. **getIpv4Address(interface)** - Get IPv4 address | ||
| 4. **getIpv6Address(interface)** - Get IPv6 address | ||
| 5. **getIpv4Gateway()** - Get IPv4 gateway | ||
| 6. **getIpv6Gateway()** - Get IPv6 gateway | ||
| 7. **getIpv4Dns()** - Get IPv4 DNS server | ||
| 8. **getIpv6Dns()** - Get IPv6 DNS server | ||
| 9. **getInterface()** - Get active network interface | ||
| 10. **pingGateway(endpoint, ipversion, count, timeout)** - Ping gateway with stats | ||
|
|
||
| ### Configuration Methods | ||
| 11. **setPeriodicReporting(enable)** - Enable/disable periodic reports | ||
| 12. **setReportingInterval(intervalMinutes)** - Set reporting interval | ||
|
|
||
| ### Events | ||
| - **onReportGenerated** - Fired when report is generated | ||
|
|
||
| ## Key Features Preserved | ||
|
|
||
| ### Network Diagnostics | ||
| - Connection type detection (Ethernet/WiFi) | ||
| - IPv4/IPv6 address retrieval | ||
| - Gateway/route validation | ||
| - DNS configuration checks | ||
| - Packet loss monitoring | ||
| - RTT measurements | ||
|
|
||
| ### Telemetry Integration | ||
| - T2 telemetry events for network metrics | ||
| - Conditional compilation with USE_TELEMETRY flag | ||
| - Event logging for connection type, IP info, gateway stats | ||
|
|
||
| ### Periodic Reporting | ||
| - Configurable reporting interval (default 10 minutes) | ||
| - Background thread for periodic diagnostics | ||
| - Thread-safe start/stop control | ||
|
|
||
| ## Migration from Standalone to Plugin | ||
|
|
||
| ### What Was Removed | ||
| - `main()` function - No longer needed in plugin architecture | ||
| - Singleton pattern - Thunder manages plugin lifecycle | ||
| - Standalone initialization - Replaced with IPlugin::Initialize() | ||
|
|
||
| ### What Was Added | ||
| - Thunder plugin lifecycle (IPlugin interface) | ||
| - RPC connection management | ||
| - Notification system for events | ||
| - COM-RPC and JSON-RPC interfaces | ||
| - Configuration interface (IConfiguration) | ||
| - Proper Thunder logging (TRACE macros) | ||
|
|
||
| ### What Was Transformed |
There was a problem hiding this comment.
This document states the plugin supports/exposes JSON-RPC APIs and that NetworkConnectionStats.json is part of the shipped API surface, but the current code registers only PluginHost::IPlugin and aggregates a minimal COM-RPC interface (no PluginHost::JSONRPC). Please update this doc to match the internal-only implementation or add the missing JSON-RPC layer if that’s still the goal.
| #ifdef TEST_MAIN | ||
| /* Test main function to validate NetworkJsonRPCProvider member functions */ | ||
| int main() | ||
| { | ||
| NetworkJsonRPCProvider provider; | ||
| int choice; | ||
| std::string interface_name; | ||
| bool running = true; | ||
|
|
||
| std::cout << "\n========================================\n"; | ||
| std::cout << "NetworkJsonRPCProvider Test Suite\n"; | ||
| std::cout << "========================================\n"; | ||
|
|
||
| while (running) | ||
| { | ||
| std::cout << "\nMenu:\n"; | ||
| std::cout << "1. Test getIpv4Address()\n"; | ||
| std::cout << "2. Test getIpv6Address()\n"; | ||
| std::cout << "3. Test getConnectionType()\n"; | ||
| std::cout << "4. Test getDnsEntries()\n"; | ||
| std::cout << "5. Test populateNetworkData()\n"; | ||
| std::cout << "6. Test getInterface()\n"; | ||
| std::cout << "7. Test pingToGatewayCheck()\n"; | ||
| std::cout << "8. Run all tests\n"; | ||
| std::cout << "0. Exit\n"; | ||
| std::cout << "\nEnter your choice: "; | ||
| std::cin >> choice; | ||
|
|
||
| switch (choice) | ||
| { | ||
| case 1: | ||
| std::cout << "\nTesting getIpv4Address()...\n"; | ||
| { | ||
| //interface_name = provider.getInterface(); | ||
| interface_name = "eno1"; // Hardcoded for testing | ||
| std::cout << "Using interface: " << (interface_name.empty() ? "(empty)" : interface_name) << "\n"; | ||
| std::string ipv4 = provider.getIpv4Address(interface_name); | ||
| std::cout << "Result: " << (ipv4.empty() ? "(empty)" : ipv4) << "\n"; | ||
| std::string gateway = provider.getIpv4Gateway(); |
There was a problem hiding this comment.
The TEST_MAIN test harness constructs NetworkJsonRPCProvider but never calls Initialize(), so all JSON-RPC calls will fail with "client not initialized" and the tests won’t validate behavior. Please call provider.Initialize() before running the menu/tests (and fail fast if initialization fails).
| uint32_t NetworkConnectionStatsImplementation::Register(INetworkConnectionStats::INotification* notification) | ||
| { | ||
| if (nullptr == notification) { | ||
| NSLOG_ERROR("Register: notification parameter is nullptr"); | ||
| return Core::ERROR_BAD_REQUEST; | ||
| } | ||
|
|
||
| NSLOG_INFO("Register::Enter"); | ||
| _notificationLock.Lock(); | ||
|
|
||
| // Make sure we can't register the same notification callback multiple times | ||
| if (std::find(_notificationCallbacks.begin(), _notificationCallbacks.end(), notification) == _notificationCallbacks.end()) { | ||
| _notificationCallbacks.push_back(notification); | ||
| notification->AddRef(); | ||
| } | ||
|
|
||
| _notificationLock.Unlock(); | ||
|
|
||
| return Core::ERROR_NONE; | ||
| } | ||
|
|
||
| /** | ||
| * Unregister a notification callback | ||
| */ | ||
| uint32_t NetworkConnectionStatsImplementation::Unregister(INetworkConnectionStats::INotification* notification) | ||
| { | ||
| if (nullptr == notification) { | ||
| NSLOG_ERROR("Unregister: notification parameter is nullptr"); | ||
| return Core::ERROR_BAD_REQUEST; | ||
| } |
There was a problem hiding this comment.
The method definitions for Register/Unregister use INetworkConnectionStats::INotification* without qualifying Exchange::. This should match the interface type (Exchange::INetworkConnectionStats::INotification*) to avoid name lookup/compile errors and to correctly override the base interface methods.
| ### 1. NetworkConnectionStats (Plugin) | ||
| **File**: NetworkConnectionStats.h/cpp | ||
| **Inherits**: PluginHost::IPlugin, PluginHost::JSONRPC | ||
| **Purpose**: Main plugin class running in Thunder process | ||
|
|
||
| **Key Methods**: | ||
| - `Initialize(PluginHost::IShell*)` - Plugin activation | ||
| - `Deinitialize(PluginHost::IShell*)` - Plugin cleanup | ||
| - `Information()` - Plugin description | ||
| - `Deactivated(RPC::IRemoteConnection*)` - Handle out-of-process crash | ||
|
|
||
| **Inner Class**: `Notification` | ||
| - Implements `RPC::IRemoteConnection::INotification` | ||
| - Implements `INetworkConnectionStats::INotification` | ||
| - Forwards events to JSON-RPC clients | ||
|
|
||
| ### 2. INetworkConnectionStats (Interface) | ||
| **File**: INetworkConnectionStats.h | ||
| **Purpose**: COM-RPC interface definition | ||
|
|
||
| **Sub-Interface**: `INotification` | ||
| - `OnReportGenerated(const string&)` - Event notification | ||
|
|
||
| **Main Interface Methods**: | ||
| - `Register/Unregister` - Notification subscription | ||
| - `GenerateReport()` - Trigger diagnostics | ||
| - `GetConnectionType()` - Network type | ||
| - `GetIpv4Address/GetIpv6Address` - IP addresses | ||
| - `GetIpv4Gateway/GetIpv6Gateway` - Gateways | ||
| - `GetIpv4Dns/GetIpv6Dns` - DNS servers | ||
| - `GetInterface()` - Active interface | ||
| - `PingGateway()` - Gateway connectivity test | ||
| - `SetPeriodicReporting()` - Enable/disable periodic reports | ||
| - `SetReportingInterval()` - Configure interval | ||
|
|
There was a problem hiding this comment.
This quick reference claims the plugin inherits PluginHost::JSONRPC and exposes many JSON-RPC methods/events, but the current implementation (NetworkConnectionStats.h/.cpp and INetworkConnectionStats.h) is internal-only and doesn’t provide that API surface. Please update this document to match the actual architecture, or update the code to match the documented APIs.
|
|
||
| while (!_stopReporting && _periodicReportingEnabled) { | ||
| // Sleep for configured interval | ||
| std::this_thread::sleep_for(std::chrono::seconds(_reportingIntervalSeconds.load())); | ||
|
|
||
| if (_stopReporting || !_periodicReportingEnabled) { | ||
| break; | ||
| } | ||
|
|
||
| // Queue report generation | ||
| queueReportGeneration("Timer"); | ||
| } | ||
|
|
There was a problem hiding this comment.
timerThread() uses a single sleep_for(reportingInterval) which means shutdown/deinitialize can block for up to the full interval (default 600s) before the thread exits and join() returns. Consider using a condition_variable timed wait (or smaller sleep slices) that can be interrupted when _stopReporting becomes true.
| while (!_stopReporting && _periodicReportingEnabled) { | |
| // Sleep for configured interval | |
| std::this_thread::sleep_for(std::chrono::seconds(_reportingIntervalSeconds.load())); | |
| if (_stopReporting || !_periodicReportingEnabled) { | |
| break; | |
| } | |
| // Queue report generation | |
| queueReportGeneration("Timer"); | |
| } | |
| while (!_stopReporting && _periodicReportingEnabled) { | |
| // Sleep for the configured interval in small slices so we can react quickly to stop requests | |
| const auto intervalSeconds = _reportingIntervalSeconds.load(); | |
| auto startTime = std::chrono::steady_clock::now(); | |
| while (!_stopReporting && _periodicReportingEnabled) { | |
| auto elapsed = std::chrono::duration_cast<std::chrono::seconds>( | |
| std::chrono::steady_clock::now() - startTime).count(); | |
| if (elapsed >= intervalSeconds) { | |
| break; | |
| } | |
| auto remaining = intervalSeconds - elapsed; | |
| auto sleepSeconds = std::min<std::int64_t>(remaining, 1); | |
| std::this_thread::sleep_for(std::chrono::seconds(sleepSeconds)); | |
| } | |
| if (_stopReporting || !_periodicReportingEnabled) { | |
| break; | |
| } | |
| // Queue report generation | |
| queueReportGeneration("Timer"); | |
| } |
| ### Configuration Parameters | ||
|
|
||
| | Parameter | Type | Default | Description | | ||
| |-----------|------|---------|-------------| | ||
| | `reportingInterval` | number | 10 | Diagnostic interval in minutes | | ||
| | `autoStart` | boolean | true | Auto-start periodic reporting on plugin activation | | ||
| | `outofprocess` | boolean | true | Run implementation out-of-process (recommended) | | ||
|
|
||
| ## Files Structure |
There was a problem hiding this comment.
The docs describe reportingInterval as minutes, but the implementation treats it as seconds (_reportingIntervalSeconds and sleep_for(seconds(...))) and the default config uses 600. Please clarify the unit in this README (and/or rename the config key) so operators don’t misconfigure the interval by 60x.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
udaykrishnag
force-pushed
the
topic/RDK-59986
branch
from
4c0fe1b to
38ff429
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 28 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| m_provider = NetworkDataProviderFactory::CreateProvider(providerType); | ||
| if (m_provider == nullptr) { |
There was a problem hiding this comment.
Configure() assigns a new instance to m_provider without cleaning up any previously created provider or stopping any previously started threads/subscriptions. If Configure() can be called more than once (e.g., reconfiguration), this can leak the old provider and start duplicate worker threads. Consider guarding against re-entry or performing a full shutdown before replacing m_provider.
|
|
||
| // Create JSON-RPC client connection to NetworkManager | ||
| m_networkManagerClient = std::make_shared<WPEFramework::JSONRPC::SmartLinkType<WPEFramework::Core::JSON::IElement>>( | ||
| _T(NETWORK_MANAGER_CALLSIGN), | ||
| _T(""), // No specific version | ||
| _T("") // No token needed for inter-plugin communication |
There was a problem hiding this comment.
SmartLinkType is constructed with empty namespace/query/token fields here. Elsewhere in this repo, JSON-RPC calls to org.rdk.NetworkManager mint a SecurityAgent token and pass it via the query string; if security is enabled, the current construction may fail. Consider following the established token/query pattern or making these parameters configurable.
| // Create JSON-RPC client connection to NetworkManager | |
| m_networkManagerClient = std::make_shared<WPEFramework::JSONRPC::SmartLinkType<WPEFramework::Core::JSON::IElement>>( | |
| _T(NETWORK_MANAGER_CALLSIGN), | |
| _T(""), // No specific version | |
| _T("") // No token needed for inter-plugin communication | |
| // Optional: read query and token for NetworkManager from environment so that | |
| // secured deployments can provide a SecurityAgent token or custom query string. | |
| WPEFramework::Core::string networkManagerQuery; | |
| WPEFramework::Core::string networkManagerToken; | |
| WPEFramework::Core::SystemInfo::GetEnvironment(_T("NETWORK_MANAGER_QUERY"), networkManagerQuery); | |
| WPEFramework::Core::SystemInfo::GetEnvironment(_T("NETWORK_MANAGER_TOKEN"), networkManagerToken); | |
| // Create JSON-RPC client connection to NetworkManager | |
| m_networkManagerClient = std::make_shared<WPEFramework::JSONRPC::SmartLinkType<WPEFramework::Core::JSON::IElement>>( | |
| _T(NETWORK_MANAGER_CALLSIGN), | |
| networkManagerQuery, | |
| networkManagerToken |
| #### 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 |
There was a problem hiding this comment.
This document states the plugin provides COM-RPC + JSON-RPC APIs and lists many API methods/events, but the actual code/interface added in this PR implements an internal-only plugin with a minimal COM-RPC interface (and no JSON-RPC dispatcher). Please update this doc to reflect the current architecture so it doesn’t mislead future maintainers.
| // Retry subscription | ||
| NSLOG_INFO("Retrying event subscriptions..."); | ||
| subscribeToEvents(); | ||
| } |
There was a problem hiding this comment.
The retry loop logs "Retrying event subscriptions..." very frequently (every 500ms) until all subscriptions succeed. If NetworkManager is down for an extended period this will spam logs and increase CPU wakeups. Consider increasing the interval, using exponential backoff, and/or rate-limiting the log message (e.g., log every N retries).
Co-authored-by: gururaajar <r.gururaaja@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 28 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #include "INetworkData.h" | ||
| #include "Module.h" | ||
| #include <string> | ||
| #include <core/JSON.h> | ||
|
|
||
| class NetworkJsonRPCProvider : public INetworkData | ||
| { | ||
| public: | ||
| NetworkJsonRPCProvider(); | ||
| ~NetworkJsonRPCProvider(); | ||
|
|
||
| /* @brief Initialize the provider with Thunder connection */ | ||
| bool Initialize(); | ||
| /* @brief Retrieve IPv4 address for specified interface | ||
| * @param interface_name Interface name (e.g., eth0, wlan0) | ||
| * @return IPv4 address string | ||
| */ | ||
| std::string getIpv4Address(std::string interface_name) override; | ||
|
|
||
| /* @brief Retrieve IPv6 address for specified interface | ||
| * @param interface_name Interface name (e.g., eth0, wlan0) | ||
| * @return IPv6 address string | ||
| */ | ||
| std::string getIpv6Address(std::string interface_name) override; | ||
|
|
||
| /* @brief Get IPv4 gateway/route address from last getIpv4Address call */ | ||
| std::string getIpv4Gateway() override; | ||
|
|
||
| /* @brief Get IPv6 gateway/route address from last getIpv6Address call */ | ||
| std::string getIpv6Gateway() override; | ||
|
|
||
| /* @brief Get IPv4 primary DNS from last getIpv4Address call */ | ||
| std::string getIpv4PrimaryDns() override; | ||
|
|
||
| /* @brief Get IPv6 primary DNS from last getIpv6Address call */ | ||
| std::string getIpv6PrimaryDns() override; | ||
|
|
||
| /* @brief Get current network connection type */ | ||
| std::string getConnectionType() override; | ||
|
|
||
| /* @brief Get DNS server entries */ | ||
| std::string getDnsEntries() override; | ||
|
|
||
| /* @brief Populate network interface data */ | ||
| void populateNetworkData() override; | ||
|
|
||
| /* @brief Get current active interface name */ | ||
| std::string getInterface() override; | ||
|
|
||
| /* @brief Ping to gateway to check packet loss | ||
| * @param endpoint Gateway IP address to ping | ||
| * @param ipversion Either "IPv4" or "IPv6" | ||
| * @param count Number of ping packets to send | ||
| * @param timeout Timeout in seconds | ||
| * @return true if ping successful, false otherwise | ||
| */ | ||
| bool pingToGatewayCheck(std::string endpoint, std::string ipversion, int count, int timeout) override; | ||
|
|
||
| /* @brief Get packet loss from last ping call */ | ||
| std::string getPacketLoss() override; | ||
|
|
||
| /* @brief Get average RTT from last ping call */ | ||
| std::string getAvgRtt() override; | ||
|
|
||
| /* @brief Subscribe to NetworkManager events | ||
| * @param eventName Name of the event (e.g., "onInterfaceStateChange") | ||
| * @param callback Callback function to be called when event fires | ||
| * @return Error code (Core::ERROR_NONE on success) | ||
| */ | ||
| uint32_t SubscribeToEvent(const std::string& eventName, | ||
| std::function<void(const WPEFramework::Core::JSON::VariantContainer&)> callback) override; | ||
|
|
||
| /* @brief Invoke WiFiConnect API on NetworkManager with empty params to reconnect to last SSID */ | ||
| uint32_t invokeWiFiConnect() override; | ||
|
|
||
| private: | ||
| // Cached data from last API calls | ||
| std::string m_ipv4Gateway; | ||
| std::string m_ipv6Gateway; | ||
| std::string m_ipv4PrimaryDns; | ||
| std::string m_ipv6PrimaryDns; | ||
| std::string m_packetLoss; | ||
| std::string m_avgRtt; | ||
|
|
||
| // Thunder JSON-RPC client for NetworkManager communication | ||
| std::shared_ptr<WPEFramework::JSONRPC::SmartLinkType<WPEFramework::Core::JSON::IElement>> m_networkManagerClient; | ||
| }; |
| #ifdef TEST_MAIN | ||
| /* Test main function to validate NetworkJsonRPCProvider member functions */ | ||
| int main() | ||
| { | ||
| NetworkJsonRPCProvider provider; | ||
| int choice; | ||
| std::string interface_name; | ||
| bool running = true; | ||
|
|
||
| std::cout << "\n========================================\n"; | ||
| std::cout << "NetworkJsonRPCProvider Test Suite\n"; | ||
| std::cout << "========================================\n"; | ||
|
|
| void NetworkConnectionStatsImplementation::subscriptionRetryThread() | ||
| { | ||
| NSLOG_INFO("Subscription retry thread started"); | ||
|
|
||
| while (!_stopSubscriptionRetry) | ||
| { | ||
| // Check if all subscriptions are successful | ||
| if (m_subsIfaceStateChange && m_subsActIfaceChange && m_subsIPAddrChange) | ||
| { | ||
| NSLOG_INFO("All required events subscribed; Stopping retry thread"); | ||
| break; | ||
| } | ||
|
|
||
| // Wait before retry | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(SUBSCRIPTION_RETRY_INTERVAL_MS)); | ||
|
|
||
| if (_stopSubscriptionRetry) | ||
| break; | ||
|
|
||
| // Retry subscription | ||
| NSLOG_INFO("Retrying event subscriptions..."); | ||
| subscribeToEvents(); | ||
| } |
| #include "INetworkData.h" | ||
| #include "Module.h" | ||
| #include <string> | ||
| #include <core/JSON.h> | ||
|
|
||
| #define NETWORK_MANAGER_CALLSIGN "org.rdk.NetworkManager" | ||
|
|
||
| class NetworkComRPCProvider : public INetworkData | ||
| { | ||
| public: | ||
| NetworkComRPCProvider(); | ||
| virtual ~NetworkComRPCProvider(); | ||
|
|
||
| /* @brief Initialize COM-RPC connection to NetworkManager | ||
| * @return true if connection successful, false otherwise | ||
| */ | ||
| bool Initialize(); | ||
|
|
||
| /* @brief Retrieve IPv4 address for specified interface | ||
| * @param interface_name Interface name (e.g., eth0, wlan0) | ||
| * @return IPv4 address string | ||
| */ | ||
| std::string getIpv4Address(std::string interface_name) override; | ||
|
|
||
| /* @brief Retrieve IPv6 address for specified interface | ||
| * @param interface_name Interface name (e.g., eth0, wlan0) | ||
| * @return IPv6 address string | ||
| */ | ||
| std::string getIpv6Address(std::string interface_name) override; | ||
|
|
||
| /* @brief Get IPv4 gateway/route address from last getIpv4Address call */ | ||
| std::string getIpv4Gateway() override; | ||
|
|
||
| /* @brief Get IPv6 gateway/route address from last getIpv6Address call */ | ||
| std::string getIpv6Gateway() override; | ||
|
|
||
| /* @brief Get IPv4 primary DNS from last getIpv4Address call */ | ||
| std::string getIpv4PrimaryDns() override; | ||
|
|
||
| /* @brief Get IPv6 primary DNS from last getIpv6Address call */ | ||
| std::string getIpv6PrimaryDns() override; | ||
|
|
||
| /* @brief Get current network connection type */ | ||
| std::string getConnectionType() override; | ||
|
|
||
| /* @brief Get DNS server entries */ | ||
| std::string getDnsEntries() override; | ||
|
|
||
| /* @brief Populate network interface data */ | ||
| void populateNetworkData() override; | ||
|
|
||
| /* @brief Get current active interface name */ | ||
| std::string getInterface() override; | ||
|
|
||
| /* @brief Ping to gateway to check packet loss | ||
| * @param endpoint Gateway IP address to ping | ||
| * @param ipversion Either "IPv4" or "IPv6" | ||
| * @param count Number of ping packets to send | ||
| * @param timeout Timeout in seconds | ||
| * @return true if ping successful, false otherwise | ||
| */ | ||
| bool pingToGatewayCheck(std::string endpoint, std::string ipversion, int count, int timeout) override; | ||
|
|
||
| /* @brief Get packet loss from last ping call */ | ||
| std::string getPacketLoss() override; | ||
|
|
||
| /* @brief Get average RTT from last ping call */ | ||
| std::string getAvgRtt() override; | ||
|
|
||
| /* @brief Subscribe to NetworkManager events | ||
| * @param eventName Name of the event (e.g., "onInterfaceStateChange") | ||
| * @param callback Callback function to be called when event fires | ||
| * @return Error code (Core::ERROR_NONE on success) | ||
| */ | ||
| uint32_t SubscribeToEvent(const std::string& eventName, | ||
| std::function<void(const WPEFramework::Core::JSON::VariantContainer&)> callback) override; | ||
|
|
||
| /* @brief Invoke WiFiConnect API on NetworkManager with empty params to reconnect to last SSID */ | ||
| uint32_t invokeWiFiConnect() override; | ||
|
|
||
| private: | ||
| // Cached data from last API calls | ||
| std::string m_ipv4Gateway; | ||
| std::string m_ipv6Gateway; | ||
| std::string m_ipv4PrimaryDns; | ||
| std::string m_ipv6PrimaryDns; | ||
| std::string m_packetLoss; | ||
| std::string m_avgRtt; | ||
|
|
||
| // Thunder JSON-RPC client for inter-plugin communication | ||
| std::shared_ptr<WPEFramework::JSONRPC::LinkType<WPEFramework::Core::JSON::IElement>> m_networkManagerClient; | ||
| }; |
| NSLOG_INFO("Register::Enter"); | ||
| _notificationLock.Lock(); | ||
|
|
||
| // Make sure we can't register the same notification callback multiple times | ||
| if (std::find(_notificationCallbacks.begin(), _notificationCallbacks.end(), notification) == _notificationCallbacks.end()) { | ||
| _notificationCallbacks.push_back(notification); | ||
| notification->AddRef(); | ||
| } | ||
|
|
||
| _notificationLock.Unlock(); |
| ## API Methods Available | ||
|
|
||
| ### Diagnostic Methods | ||
| 1. **generateReport()** - Generate full network diagnostics report | ||
| 2. **getConnectionType()** - Get Ethernet/WiFi connection type | ||
| 3. **getIpv4Address(interface)** - Get IPv4 address | ||
| 4. **getIpv6Address(interface)** - Get IPv6 address | ||
| 5. **getIpv4Gateway()** - Get IPv4 gateway | ||
| 6. **getIpv6Gateway()** - Get IPv6 gateway | ||
| 7. **getIpv4Dns()** - Get IPv4 DNS server | ||
| 8. **getIpv6Dns()** - Get IPv6 DNS server | ||
| 9. **getInterface()** - Get active network interface | ||
| 10. **pingGateway(endpoint, ipversion, count, timeout)** - Ping gateway with stats | ||
|
|
||
| ### Configuration Methods | ||
| 11. **setPeriodicReporting(enable)** - Enable/disable periodic reports | ||
| 12. **setReportingInterval(intervalMinutes)** - Set reporting interval | ||
|
|
||
| ### Events | ||
| - **onReportGenerated** - Fired when report is generated | ||
|
|
||
| ## Key Features Preserved | ||
|
|
||
| ### Network Diagnostics | ||
| - Connection type detection (Ethernet/WiFi) | ||
| - IPv4/IPv6 address retrieval | ||
| - Gateway/route validation | ||
| - DNS configuration checks |
|
|
||
| NSLOG_INFO("NetworkConnectionStatsImplementation Constructor"); | ||
| /* Set NetworkManager Out-Process name to be "NetworkConnectionStats" */ | ||
| Core::ProcessInfo().Name("NetworkConnectionStats"); | ||
| NSLOG_INFO((_T("NetworkConnectionStats Out-Of-Process Instantiation; SHA: " _T(EXPAND_AND_QUOTE(PLUGIN_BUILD_REFERENCE))))); | ||
| #if USE_TELEMETRY | ||
| t2_init("networkstats"); | ||
| #endif | ||
| } |
udaykrishnag
force-pushed
the
topic/RDK-59986
branch
from
2c40500 to
9f68ba5
Compare
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 28 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Optional Telemetry support | ||
| if (USE_TELEMETRY) | ||
| find_package(T2 REQUIRED) | ||
| add_definitions(-DUSE_TELEMETRY) |
|
|
||
| // Queue report generation | ||
| std::lock_guard<std::mutex> lock(_queueMutex); |
| NetworkConnectionStatsImplementation::~NetworkConnectionStatsImplementation() | ||
| { | ||
| NSLOG_INFO("NetworkConnectionStatsImplementation Destructor"); | ||
|
|
||
| // Stop subscription retry thread | ||
| _stopSubscriptionRetry = true; | ||
| if (_subscriptionRetryThread.joinable()) { | ||
| _subscriptionRetryThread.join(); | ||
| } | ||
|
|
||
| // Stop reporting threads | ||
| _stopReporting = true; | ||
|
|
||
| // Push STOP message to unblock consumer thread | ||
| { | ||
| std::lock_guard<std::mutex> lock(_queueMutex); | ||
| _messageQueue.push({MessageType::STOP}); | ||
| } | ||
| _queueCondition.notify_one(); | ||
|
|
||
| // Wait for threads to finish | ||
| if (_timerThread.joinable()) { | ||
| _timerThread.join(); | ||
| } | ||
| if (_reportingThread.joinable()) { | ||
| _reportingThread.join(); | ||
| } | ||
|
|
||
| if (m_provider) { | ||
| delete m_provider; | ||
| m_provider = nullptr; | ||
| } |
| try { | ||
| // Set Thunder access point | ||
| WPEFramework::Core::SystemInfo::SetEnvironment(_T("THUNDER_ACCESS"), _T("127.0.0.1:9998")); | ||
|
|
||
| // Create JSON-RPC client connection to NetworkManager | ||
| m_networkManagerClient = std::make_shared<WPEFramework::JSONRPC::SmartLinkType<WPEFramework::Core::JSON::IElement>>( | ||
| _T(NETWORK_MANAGER_CALLSIGN), | ||
| _T(""), // No specific version | ||
| _T("") // No token needed for inter-plugin communication | ||
| ); |
| #ifdef TEST_MAIN | ||
| /* Test main function to validate NetworkJsonRPCProvider member functions */ | ||
| int main() | ||
| { | ||
| NetworkJsonRPCProvider provider; | ||
| int choice; | ||
| std::string interface_name; | ||
| bool running = true; | ||
|
|
| set(GENERATOR_SEARCH_PATH ${CMAKE_SYSROOT}${CMAKE_INSTALL_PREFIX}/include/${NAMESPACE}) | ||
| endif() | ||
|
|
||
| file(GLOB INTERFACE_HEADER ${CMAKE_CURRENT_SOURCE_DIR}) |
| bool m_subsIfaceStateChange; | ||
| bool m_subsActIfaceChange; | ||
| bool m_subsIPAddrChange; |
| /** | ||
| * Logging level with an increasing order of refinement | ||
| * (DEBUG_LEVEL = Finest logging) | ||
| * It is essental to start with 0 and increase w/o gaps as the value |
| "methods": { | ||
| "generateReport": { | ||
| "summary": "Generate network diagnostics report", | ||
| "description": "Triggers a full network diagnostic scan including connection type, IP configuration, routes, and DNS", | ||
| "result": { | ||
| "$ref": "#/definitions/success" | ||
| } | ||
| }, |
| #ifdef TEST_MAIN | ||
| /* Test main function to validate NetworkComRPCProvider member functions */ | ||
| int main() | ||
| { | ||
| NetworkComRPCProvider provider; | ||
| int choice; | ||
| std::string interface_name; | ||
| bool running = true; |
There was a problem hiding this comment.
NetworkStat is something cannot in sub folder.
In EntOS, each plugin is a separate repo is the model being used now. So it should be separate repo. Ex:
https://github.com/rdkcentral/entservices-hdmicecsource
https://github.com/rdkcentral/entservices-hdmicecsink
Or, we can have functional grouping..
The new interface has to go to existing interface folder
The new definition has to go to existing definition folder
ex:
https://github.com/rdkcentral/entservices-appgateway
Above is based on the expectation. I did not review the technicality yet.
Thanks,
No description provided.