Conversation
There was a problem hiding this comment.
Pull request overview
Updates ControlMgr’s Thunder PowerManager integration to treat getNetworkStandbyMode as a boolean RPC result (instead of parsing a JSON object), adding a dedicated boolean-call helper to the Thunder plugin base.
Changes:
- Add
call_plugin_boolean()helper to invoke Thunder JSONRPC methods returning a boolean. - Update PowerManager NSM fetch to use the boolean helper.
- Add an extra NSM log line in the main power-state-change handling.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/thunder/ctrlm_thunder_plugin_powermanager.cpp |
Switch NSM retrieval to boolean helper and adjust logging. |
src/thunder/ctrlm_thunder_plugin.h |
Declare new call_plugin_boolean() helper. |
src/thunder/ctrlm_thunder_plugin.cpp |
Implement call_plugin_boolean() using Core::JSON::Boolean. |
src/ctrlm_main.cpp |
Add additional NSM logging during power state changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -124,6 +124,8 @@ class ctrlm_thunder_plugin_t { | |||
| */ | |||
| bool call_plugin(std::string method, void *params, void *response, unsigned int retries = 0); | |||
|
|
|||
There was a problem hiding this comment.
call_plugin_boolean is added without any header documentation, while the surrounding helper methods have Doxygen-style comments describing params/return values. Please add a matching comment block (and if you add a retries parameter for timeout handling, document that as well) to keep the header self-describing.
| /** | |
| * This function is used to call a Thunder Plugin method that returns a boolean result. | |
| * @param method The method in which the user wants to call. | |
| * @param params The WPEFramework JsonObject containing the parameters for the call. (We can't include WPEFramework headers in controlMgr .h files as their logging macros clash) | |
| * @param response Pointer to a boolean that will contain the result from the call. | |
| * @return True if the call succeeded, otherwise False. | |
| */ |
| XLOGD_INFO("NSM is <%s>", (ctrlm_main_get_networked_standby_mode())?"ENABLED":"DISABLED"); | ||
| } | ||
| XLOGD_ERROR("DELIA-70047 NSM is <%s>", (ctrlm_main_get_networked_standby_mode())?"ENABLED":"DISABLED"); |
There was a problem hiding this comment.
This looks like a temporary ticket-specific debug log and it uses XLOGD_ERROR unconditionally. It also duplicates the earlier NSM log (and calls ctrlm_main_get_networked_standby_mode again), which can add noise. Please remove this line or downgrade it to DEBUG and gate it the same way as the existing NSM log.
| XLOGD_INFO("NSM is <%s>", (ctrlm_main_get_networked_standby_mode())?"ENABLED":"DISABLED"); | |
| } | |
| XLOGD_ERROR("DELIA-70047 NSM is <%s>", (ctrlm_main_get_networked_standby_mode())?"ENABLED":"DISABLED"); | |
| bool nsm_enabled = ctrlm_main_get_networked_standby_mode(); | |
| XLOGD_INFO("NSM is <%s>", nsm_enabled ? "ENABLED" : "DISABLED"); | |
| XLOGD_DEBUG("DELIA-70047 NSM is <%s>", nsm_enabled ? "ENABLED" : "DISABLED"); | |
| } |
| XLOGD_DEBUG("networked_standby_mode is %s", networked_standby_mode?"TRUE":"FALSE"); | ||
| sem_wait(&this->semaphore); | ||
| if(this->call_plugin_boolean("getNetworkStandbyMode", (void *)¶ms, &networked_standby_mode)) { | ||
| XLOGD_ERROR("networked_standby_mode is %s", networked_standby_mode ? "TRUE" : "FALSE"); |
There was a problem hiding this comment.
The success path logs the NSM value with XLOGD_ERROR, which will mark normal behavior as an error and can create noisy/alerting logs. This file uses DEBUG for successful state/value logs (e.g., power state and wakeup reason) and ERROR for failures; consider changing this to XLOGD_DEBUG/INFO and reserving XLOGD_ERROR for the failure branch.
| XLOGD_ERROR("networked_standby_mode is %s", networked_standby_mode ? "TRUE" : "FALSE"); | |
| XLOGD_DEBUG("networked_standby_mode is %s", networked_standby_mode ? "TRUE" : "FALSE"); |
| if(thunderRet == Core::ERROR_NONE) { | ||
| *response = jsonResponse.Value(); | ||
| ret = true; | ||
| XLOGD_ERROR("%s: Thunder call success <%s> <%u> response:%d \n", __FUNCTION__, method.c_str(), thunderRet, *response); |
There was a problem hiding this comment.
call_plugin_boolean logs a successful Thunder invoke using XLOGD_ERROR. This will incorrectly surface normal calls as errors; consider removing the success log entirely (to match call_plugin) or logging it at DEBUG/INFO instead.
| XLOGD_ERROR("%s: Thunder call success <%s> <%u> response:%d \n", __FUNCTION__, method.c_str(), thunderRet, *response); |
| uint32_t thunderRet = clientObject->Invoke<JsonObject, Core::JSON::Boolean>(CALL_TIMEOUT, _T(method), *jsonParams, jsonResponse); | ||
| if(thunderRet == Core::ERROR_NONE) { | ||
| *response = jsonResponse.Value(); | ||
| ret = true; | ||
| XLOGD_ERROR("%s: Thunder call success <%s> <%u> response:%d \n", __FUNCTION__, method.c_str(), thunderRet, *response); | ||
| } else { | ||
| XLOGD_ERROR("%s: Thunder call failed <%s> <%u>\n", __FUNCTION__, method.c_str(), thunderRet); |
There was a problem hiding this comment.
This introduces a new Thunder call helper but it doesn’t support retries on Core::ERROR_TIMEDOUT like call_plugin/property_get do. To keep behavior consistent and avoid transient timeouts becoming hard failures, consider adding a retries parameter (default 0) and wrapping Invoke in the same timed-out retry loop used by call_plugin.
| uint32_t thunderRet = clientObject->Invoke<JsonObject, Core::JSON::Boolean>(CALL_TIMEOUT, _T(method), *jsonParams, jsonResponse); | |
| if(thunderRet == Core::ERROR_NONE) { | |
| *response = jsonResponse.Value(); | |
| ret = true; | |
| XLOGD_ERROR("%s: Thunder call success <%s> <%u> response:%d \n", __FUNCTION__, method.c_str(), thunderRet, *response); | |
| } else { | |
| XLOGD_ERROR("%s: Thunder call failed <%s> <%u>\n", __FUNCTION__, method.c_str(), thunderRet); | |
| unsigned int attempts = 0; | |
| unsigned int retries = 0; // keep current behavior: no retries beyond the initial attempt | |
| uint32_t thunderRet = Core::ERROR_TIMEDOUT; | |
| while(thunderRet == Core::ERROR_TIMEDOUT && attempts <= retries) { // Only retry on TIMEDOUT | |
| thunderRet = clientObject->Invoke<JsonObject, Core::JSON::Boolean>(CALL_TIMEOUT, _T(method), *jsonParams, jsonResponse); | |
| if(thunderRet == Core::ERROR_NONE) { | |
| *response = jsonResponse.Value(); | |
| ret = true; | |
| XLOGD_ERROR("%s: Thunder call success <%s> <%u> response:%d \n", __FUNCTION__, method.c_str(), thunderRet, *response); | |
| } else { | |
| attempts += 1; | |
| XLOGD_ERROR("%s: Thunder call failed <%s> <%u>, attempt %u of %u\n", | |
| __FUNCTION__, | |
| method.c_str(), | |
| thunderRet, | |
| attempts, | |
| (thunderRet == Core::ERROR_TIMEDOUT ? retries : 0) + 1); // retries + initial attempt | |
| } |
DELIA-70047
NSM to be fetched as a boolean instead of JSON object