RDKEMW-13122: Wi‑Fi networks in the picker are not ordered by signalstrength

[gururaajar]

Reason for change: Updated the onAvailableSSID to return strength and frequency as numbers so that the UI will do the sorting.
Test Procedure: Check the onAvailableSSID returns strength and frequency as numbers
Priority:P1
Risks: Medium

[Copilot]

Pull request overview

This pull request addresses issue RDKEMW-13122 by updating the XCast plugin to return WiFi signal strength and frequency values as numeric types instead of strings. This enables proper sorting of Wi-Fi networks in the picker UI based on signal strength.

Changes:

• Updated onWiFiSignalQualityChange method signature to accept integer parameters for strength, noise, and SNR instead of strings
• Modified logging format specifiers from %s to %d for the numeric WiFi signal parameters
• Updated test data in test_XCast.cpp to use numeric values for WiFi signal parameters

Reviewed changes

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

File	Description
XCast/XCastImplementation.h	Changed method signature from string to int parameters for WiFi signal metrics (strength, noise, SNR) and updated corresponding log format specifiers
Tests/L1Tests/tests/test_XCast.cpp	Updated test calls to pass numeric values instead of string literals for WiFi signal parameters and modified JSON test data to use numeric values for SignalStrength and Frequency

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

XCast/XCastImplementation.cpp:401

• When either registration fails (retStatus1 or retStatus2 is not ERROR_NONE), the code logs an error and sets _registeredNMEventHandlers to false. However, if one registration succeeds and the other fails, the successful registration is not cleaned up. This could lead to a partial registration state. Consider adding explicit unregistration logic in the error path to ensure both notifications are either fully registered or fully unregistered.

                if (Core::ERROR_NONE == retStatus1 && Core::ERROR_NONE == retStatus2)
                {
                    LOGINFO("INetworkManager::Register event registered");
                    _registeredNMEventHandlers = true;
                }
                else
                {
                    LOGERR("Failed to register INetworkManager::Register event");
                    _registeredNMEventHandlers = false;
                }

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

XCast/XCastImplementation.cpp:401

• The error handling only sets the flag to true if both registrations succeed. However, if one registration succeeds and the other fails, the first successful registration is not cleaned up. Consider adding cleanup logic to unregister the first interface if the second registration fails, to avoid leaving the plugin in an inconsistent state where only one of the two required notification interfaces is registered.

            Core::hresult retStatus1 = Core::ERROR_GENERAL;
            Core::hresult retStatus2 = Core::ERROR_GENERAL;
            if (_networkManagerPlugin)
            {
                retStatus1 = _networkManagerPlugin->RegisterActiveIfaceNotify(_networkManagerNotification.baseInterface<Exchange::INetworkManager::IActiveIfaceNotify>());
                retStatus2 = _networkManagerPlugin->RegisterIPAddNotify(_networkManagerNotification.baseInterface<Exchange::INetworkManager::IIPAddNotify>());
                if (Core::ERROR_NONE == retStatus1 && Core::ERROR_NONE == retStatus2)
                {
                    LOGINFO("INetworkManager::Register event registered");
                    _registeredNMEventHandlers = true;
                }
                else
                {
                    LOGERR("Failed to register INetworkManager::Register event");
                    _registeredNMEventHandlers = false;
                }

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

XCast/XCastImplementation.cpp:401

• If one of the two registrations fails, the other may already be registered, leaving a partially-registered state while _registeredNMEventHandlers is set to false. This can leak callbacks and prevents cleanup in unregisterNetworkEventHandlers(). Consider rolling back any successful registration when the other fails, and include the individual error codes in the log so it’s clear which registration failed.

            Core::hresult retStatus1 = Core::ERROR_GENERAL;
            Core::hresult retStatus2 = Core::ERROR_GENERAL;
            if (_networkManagerPlugin)
            {
                retStatus1 = _networkManagerPlugin->RegisterActiveIfaceNotify(_networkManagerNotification.baseInterface<Exchange::INetworkManager::IActiveIfaceNotify>());
                retStatus2 = _networkManagerPlugin->RegisterIPAddNotify(_networkManagerNotification.baseInterface<Exchange::INetworkManager::IIPAddNotify>());
                if (Core::ERROR_NONE == retStatus1 && Core::ERROR_NONE == retStatus2)
                {
                    LOGINFO("INetworkManager::Register event registered");
                    _registeredNMEventHandlers = true;
                }
                else
                {
                    LOGERR("Failed to register INetworkManager::Register event");
                    _registeredNMEventHandlers = false;
                }

[Copilot]

The PR is for RDKEMW-13122, but this script pins networkmanager to branch topic/RDKEMW-13124. If this is intentional, please document why (or align the dependency branch with the ticket/behavior being fixed) to avoid confusion and accidental regressions when the branch changes.

[Copilot]

These callbacks take parameters by value but then pass them to _parent using std::move. Because the parameters are effectively const here, the move will degrade to a copy; additionally, passing by value causes an extra copy for each callback. Prefer taking const string& and passing through by reference (or take non-const by value if you truly want to move).

[Copilot]

The NetworkManager event test is still invoking legacy INetworkManager::INotification callbacks (e.g., onWiFiStateChange, onInternetStatusChange, onInterfaceStateChange). The implementation now registers IActiveIfaceNotify and IIPAddNotify, so this test/mocks likely won’t compile or will fail to capture the notifications. Update the mock expectations and the stored notification pointers to the new notify interfaces and only call the callbacks that are still part of those interfaces.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

XCast/XCastImplementation.cpp:401

• registerNetworkEventHandlers() sets _registeredNMEventHandlers to false if either registration fails, but it doesn’t undo a successful partial registration. This can leave one notification registered while the plugin believes none are registered, preventing proper cleanup and causing unexpected callbacks. Unregister any successfully-registered notify on failure and keep track of registration state per-notification (or only set _registeredNMEventHandlers true when both succeed, but still roll back partial success).

            Core::hresult retStatus1 = Core::ERROR_GENERAL;
            Core::hresult retStatus2 = Core::ERROR_GENERAL;
            if (_networkManagerPlugin)
            {
                retStatus1 = _networkManagerPlugin->RegisterActiveIfaceNotify(_networkManagerNotification.baseInterface<Exchange::INetworkManager::IActiveIfaceNotify>());
                retStatus2 = _networkManagerPlugin->RegisterIPAddNotify(_networkManagerNotification.baseInterface<Exchange::INetworkManager::IIPAddNotify>());
                if (Core::ERROR_NONE == retStatus1 && Core::ERROR_NONE == retStatus2)
                {
                    LOGINFO("INetworkManager::Register event registered");
                    _registeredNMEventHandlers = true;
                }
                else
                {
                    LOGERR("Failed to register INetworkManager::Register event");
                    _registeredNMEventHandlers = false;
                }

XCast/XCastImplementation.cpp:400

• The log messages still reference INetworkManager::Register, but the code now uses RegisterActiveIfaceNotify / RegisterIPAddNotify. Updating the log text will make debugging clearer and avoid misleading operators.

                if (Core::ERROR_NONE == retStatus1 && Core::ERROR_NONE == retStatus2)
                {
                    LOGINFO("INetworkManager::Register event registered");
                    _registeredNMEventHandlers = true;
                }
                else
                {
                    LOGERR("Failed to register INetworkManager::Register event");
                    _registeredNMEventHandlers = false;

[Copilot]

The script runs with shell tracing enabled (set -x and the workflow also invokes sh -x), so embedding $GITHUB_TOKEN in the clone URL will leak the token into CI logs. Disable tracing around this command or use a credential mechanism that doesn’t echo secrets (e.g., set +x for the clone, GIT_ASKPASS, or an auth header).

Suggested change

	git clone --branch topic/RDKEMW-13124 https://$GITHUB_TOKEN@github.com/rdkcentral/entservices-testframework.git
	set +x
	git clone --branch topic/RDKEMW-13124 "https://${GITHUB_TOKEN}@github.com/rdkcentral/entservices-testframework.git"
	set -x

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

XCast/XCastImplementation.cpp:401

• The registration logic only checks if both retStatus1 and retStatus2 are ERROR_NONE to set _registeredNMEventHandlers to true. However, if only one of the registrations succeeds and the other fails, the successful registration won't be tracked, and the corresponding unregister call won't be made during cleanup. This could lead to memory leaks or dangling pointers. Consider tracking each registration separately or unregistering the successful one if any registration fails.

                retStatus1 = _networkManagerPlugin->RegisterActiveIfaceNotify(_networkManagerNotification.baseInterface<Exchange::INetworkManager::IActiveIfaceNotify>());
                retStatus2 = _networkManagerPlugin->RegisterIPAddNotify(_networkManagerNotification.baseInterface<Exchange::INetworkManager::IIPAddNotify>());
                if (Core::ERROR_NONE == retStatus1 && Core::ERROR_NONE == retStatus2)
                {
                    LOGINFO("INetworkManager::Register event registered");
                    _registeredNMEventHandlers = true;
                }
                else
                {
                    LOGERR("Failed to register INetworkManager::Register event");
                    _registeredNMEventHandlers = false;
                }

[Copilot]

The JSON string contains a syntax error in the second SSID entry. The "SignalStrength" field has a malformed value: "SignalStrength\":\"-45, is missing the closing quote. It should be \"SignalStrength\":-45, (as a number without quotes) to match the first entry and align with the PR's goal of returning signal strength as numbers instead of strings.

Suggested change

	_availSSIDsNotify->onAvailableSSIDs("{\"AvailableSSIDs\":[{\"SSID\":\"myHomeSSID\",\"BSSID\":\"00:11:22:33:44:55\",\"SignalStrength\":-32,\"Frequency\":2412,\"Security\":\"WPA2-Personal\"},{\"SSID\":\"myOfficeSSID\",\"BSSID\":\"66:77:88:99:AA:BB\",\"SignalStrength\":\"-45,\"Frequency\":2412,\"Security\":\"WPA2-Enterprise\"}]}");
	_availSSIDsNotify->onAvailableSSIDs("{\"AvailableSSIDs\":[{\"SSID\":\"myHomeSSID\",\"BSSID\":\"00:11:22:33:44:55\",\"SignalStrength\":-32,\"Frequency\":2412,\"Security\":\"WPA2-Personal\"},{\"SSID\":\"myOfficeSSID\",\"BSSID\":\"66:77:88:99:AA:BB\",\"SignalStrength\":-45,\"Frequency\":2412,\"Security\":\"WPA2-Enterprise\"}]}");

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

XCast/XCastImplementation.h:208

• Several Exchange::INetworkManager::INotification callbacks (e.g., onAvailableSSIDs, onWiFiStateChange, onInternetStatusChange, onInterfaceStateChange, onWiFiSignalQualityChange) were removed from NetworkManagerNotification. The L1 XCast test still calls these methods on the notification pointer, so this change will break the build/tests unless the interface has changed and the test/call sites are updated accordingly. Re-add no-op overrides or update the interface and all callers/tests consistently.

                    XCastImplementation& _parent;
            };
 
        public:
            Core::hresult Register(Exchange::IXCast::INotification *notification) override;

XCast/XCastImplementation.h:175

• NetworkManagerNotification class declaration has a trailing comma after the single base class (... : public Exchange::INetworkManager::INotification,). This is invalid C++ syntax and will fail compilation. Remove the comma (or add the intended additional base class).

            class NetworkManagerNotification : public Exchange::INetworkManager::INotification
            {
                private:

[Copilot]

PR description says onAvailableSSID was updated to return strength/frequency as numbers for UI sorting, but this PR only adjusts dependency fetching (cloning networkmanager and copying INetworkManager.h) and changes XCast notification handling. If the functional change is expected in the networkmanager service (JSON-RPC payload), it likely needs to be made/landed there as well; otherwise this repo change alone won’t affect runtime sorting.