RDKEMW-23908: Reduce default ping count from 3 to 1 and timeout from 3s to 1s

[tukken-comcast]

Reason for change: Current Ping method is synchronous and blocks other method calls while ping is executing. This fix is a mitigation. A proper fix requires methods like Ping that trigger long-running operations to be asynchronous.
Test Procedure: Refer ticket
Risks: Low
Signed-off-by: Tony_Ukken2@comcast.com

[Copilot]

Pull request overview

Mitigates JSON-RPC Ping being a synchronous/blocking call by reducing its default work (fewer packets, shorter timeout), and updates API documentation/schema to reflect the new default request count.

Changes:

• Reduced Ping default request count from 3 to 1 in the JSON-RPC implementation.
• Reduced Ping default timeout from 3s to 1s in the JSON-RPC implementation.
• Updated API docs/schema to reflect the new default count value.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
plugin/NetworkManagerJsonRpc.cpp	Updates JSON-RPC Ping defaults (count/timeout) to reduce blocking duration.
docs/NetworkManagerPlugin.md	Updates Ping parameter docs (default count), but currently omits the new default timeout.
definition/NetworkManager.json	Updates count definition summary default, but timeout definition remains generic despite Ping default change.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[karuna2git]

Change the legacyNetwork.cpp also for the defaults

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (1)

docs/NetworkManagerPlugin.md:887

• The Ping parameter table updates the default count to 1, but the PR also changes the default timeout to 1s; the timeout row currently doesn’t document the new default or the unit (seconds), which can confuse API consumers.

| params?.count | integer | <sup>*(optional)*</sup> The number of requests to send. Default is 1 |
| params?.timeout | integer | <sup>*(optional)*</sup> Timeout |

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

plugin/NetworkManagerImplementation.cpp:458

• ping/ping6 interval was changed from 0.2s to 0.002s. On common iputils builds, intervals <0.2s are treated as “flood” and require elevated privileges (or special sysctls), so this can cause Ping to fail for non-privileged service users when callers pass count>1. Keeping the previous 0.2s avoids a runtime failure while still allowing the reduced default count/timeout to mitigate blocking.

                snprintf(cmd, sizeof(cmd), "ping  -c %d -W %d -i 0.002 '%s' 2>&1", noOfRequest, timeOutInSeconds, endpoint.c_str());

[bpunnuru]

Looks Good.
This should have been an asynchronous function, so we could have had better hand off poilicy and not hog a lot of resources in wpeframework.

[karuna2git]

If the Router declines PING requests that are sent by a STA within such a short interval, we will endup in ping failure and OPS team metric will be bothered.

EPG is not expected to do PING to decide on-line or not. So such a scenario is not a customer impacting.

[Copilot]

🔵 Needs a closer look

Document the timeout default and address the unconditional 2 ms interval change.

Review details

Suppressed comments (2)

docs/NetworkManagerPlugin.md:889

• The implementation now also changes Ping's default timeout to 1 second, but the Ping-specific API table still leaves params?.timeout undocumented. Consumers can discover the new count default from this table but not the new timeout default; please document the 1-second default alongside it.

| params?.count | integer | <sup>*(optional)*</sup> The number of requests to send. Default is 1 |
| params?.timeout | integer | <sup>*(optional)*</sup> Timeout |

plugin/NetworkManagerImplementation.cpp:555

• The interval change is applied to every ping, including requests that explicitly ask for multiple packets; -i 0.002 sends packets 2 ms apart instead of the previous 200 ms. Common ping implementations reject intervals below 200 ms for non-root callers, while implementations that accept it can generate an unintended burst of traffic. This mitigation only needs the default -c/-W changes, so please retain the existing interval here (or make any new interval policy explicit and conditional).

                snprintf(cmd, sizeof(cmd), "ping6 -c %d -W %d -i 0.002 '%s' 2>&1", noOfRequest, timeOutInSeconds, endpoint.c_str());

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved moderate test-coverage issues and documentation/schema nits remain.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (4)

docs/NetworkManagerPlugin.md:451

• The response example now declares autoconfig: false while retaining a DHCP server address. This contradicts the API's own description that false means manually configured IP; make this example consistently DHCP (or remove the DHCP-derived field and show a manual configuration).

    "autoconfig": false,

docs/NetworkManagerPlugin.md:1474

• This edited link has the same casing mismatch: the actual anchor is onWiFiStateChange, not onWIFIStateChange (see docs/NetworkManagerPlugin.md:1994). Please update this reference so the StartWPS documentation link resolves.

Also see: [onWIFIStateChange](#event.onWIFIStateChange), [onIPAddressChange](#event.onIPAddressChange), [onInternetStatusChange](#event.onInternetStatusChange)

docs/NetworkManagerPlugin.md:2006

• The newly added sentence is missing verbs, which makes the documented condition unclear; state that WPS "is initiated" and that no AP "is found".

| params.ssid | string | The SSID associated with the Wi-Fi profile causing the state transition. Disconnected state, contains the SSID associated with the connection that was disconnected. The SSID will be empty when WPS initiated and no AP found with WPS enabled |

legacy/LegacyNetworkAPIs.cpp:625

• The legacy default change is likewise not verified: l2_test_LegacyPlugin_NetworkAPIs.cpp supplies packets and matches every Ping argument with _, so it would pass even if these values remained 3. Assert the forwarded count and timeout for the legacy request (including the omitted-timeout case).

                uint32_t noOfRequest = 1;
                uint16_t timeOutInSeconds = 1;

• Files reviewed: 5/5 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved test coverage gaps and broken documentation cross-references remain.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (4)

docs/NetworkManagerPlugin.md:1376

• This updated cross-reference still points to #event.onWIFIStateChange, but the documented event anchor and emitted event are onWiFiStateChange (see docs/NetworkManagerPlugin.md:1994-1995 and plugin/NetworkManagerJsonRpc.cpp:1099). As a result, this link remains broken; use the actual event casing here.

Also see: [onWiFiStateChange](#event.onWiFiStateChange), [onIPAddressChange](#event.onIPAddressChange), [onInternetStatusChange](#event.onInternetStatusChange)

docs/NetworkManagerPlugin.md:1474

• This updated cross-reference still points to #event.onWIFIStateChange, but the documented event anchor and emitted event are onWiFiStateChange (see docs/NetworkManagerPlugin.md:1994-1995 and plugin/NetworkManagerJsonRpc.cpp:1099). As a result, this link remains broken; use the actual event casing here.

Also see: [onWiFiStateChange](#event.onWiFiStateChange), [onIPAddressChange](#event.onIPAddressChange), [onInternetStatusChange](#event.onInternetStatusChange)

plugin/NetworkManagerImplementation.cpp:559

• The existing Ping tests only check that the command contains ping/ping6 and the endpoint; they do not verify the generated flags. The changed -i 0.002 option is therefore untested in either branch. Assert the relevant -c, -W, and -i arguments for both IPv4 and IPv6 to catch command-line regressions.

                snprintf(cmd, sizeof(cmd), "ping6 -c %d -W %d -i 0.002 '%s' 2>&1", noOfRequest, timeOutInSeconds, endpoint.c_str());
            }
            else
            {
                snprintf(cmd, sizeof(cmd), "ping  -c %d -W %d -i 0.002 '%s' 2>&1", noOfRequest, timeOutInSeconds, endpoint.c_str());

plugin/NetworkManagerJsonRpc.cpp:585

• The new 1-request/1-second defaults are not asserted by the existing Ping tests. The RDK tests pass timeout but use the legacy packets field (the JSON-RPC handler reads count), and the mock only checks the command name/endpoint, so it never verifies either the effective count or timeout. Add a case omitting both optional fields and assert the generated command or forwarded arguments uses 1 and 1.

                uint32_t noOfRequest = 1;
                uint16_t timeOutInSeconds = 1;

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Add regression assertions covering the updated default count and timeout for both API paths.

Review details

Suppressed comments (2)

legacy/LegacyNetworkAPIs.cpp:625

• The legacy doPing defaults are also changed without an assertion. Its current test omits the timeout but uses an unconstrained mock expectation for INetworkManager::Ping, so it would pass for 3/3 as well as 1/1. Add a legacy-path test or tighten the existing expectation to verify the default request count and timeout are both 1.

                uint32_t noOfRequest = 1;
                uint16_t timeOutInSeconds = 1;

plugin/NetworkManagerJsonRpc.cpp:585

• The new JSON-RPC defaults are not covered by a regression assertion. The existing Ping tests pass explicit timeout values and only check that a ping command was launched; they do not verify that an omitted count/timeout reaches the implementation as 1/1. Please add a test that omits both fields and asserts the generated arguments, so a future change cannot silently restore the old blocking defaults.

                uint32_t noOfRequest = 1;
                uint16_t timeOutInSeconds = 1;

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Lite