Topic/rdkemw 16330 - #192
Conversation
Reason for Chanage: RDK-E uses a boolean to report whether Networked Standby Mode is enabled, rather than a JsonObject Test Procedure: Confirm that Control Manager enters NSM when it should Risks: This change might not work in RDK-V. We'll test to check, for now this change is only on an RDK-E release branch Priority: P0 Signed-off-by: Jason Thomson <jason_thomson@comcast.com>
There was a problem hiding this comment.
Pull request overview
This PR extends the Thunder plugin wrapper with a boolean-returning JSONRPC helper and updates the PowerManager Thunder integration to use it, while also making small doc/comment adjustments.
Changes:
- Added
call_plugin_boolean()to invoke Thunder methods that return a JSONRPC boolean result. - Updated PowerManager
getNetworkStandbyModeflow to use the new boolean helper instead of parsing a JsonObject response. - Corrected/expanded several header comments (though a few docstrings are now inconsistent with signatures).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/thunder/ctrlm_thunder_plugin.h |
Updates/introduces API documentation for plugin/property calls and adds the call_plugin_boolean declaration. |
src/thunder/ctrlm_thunder_plugin.cpp |
Implements call_plugin_boolean() using Invoke<..., Core::JSON::Boolean>. |
src/thunder/ctrlm_thunder_plugin_powermanager.cpp |
Switches getNetworkStandbyMode to the boolean helper and updates wakeup-reason comparison. |
Comments suppressed due to low confidence (1)
src/thunder/ctrlm_thunder_plugin.h:142
- In the
call_controllerdoc comment, the third@paramis labeledparamsagain, but it refers to the response object. This makes the API docs ambiguous; please rename it to@param response(and keep the descriptions consistent withcall_plugin).
/**
* This function is used to call a Thunder Controller method.
* @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 params The WPEFramework JsonObject containing the response from the call. (We can't include WPEFramework headers in controlMgr .h files as their logging macros clash)
* @return True if the call succeeded, otherwise False.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -115,7 +115,7 @@ class ctrlm_thunder_plugin_t { | |||
| bool property_get(std::string property, void *response, unsigned int retries = 0); | |||
There was a problem hiding this comment.
The docstring for property_get looks incorrect: it refers to @param method and @param params, but the signature is property_get(std::string property, void *response, ...) and there is no params argument. Please update the parameter names/descriptions to match the actual API to avoid misleading callers.
| * @param response The WPEFramework JsonObject containing the response from the call. (We can't include WPEFramework headers in controlMgr .h files as their logging macros clash) | ||
| * @param retries The number of retries if the call times out. |
There was a problem hiding this comment.
The call_plugin_boolean doc comment doesn’t match the function signature: it documents a JsonObject response and a retries parameter, but the signature takes bool *response and has no retries argument. Please adjust the comment (or add a retries argument) so the header accurately describes the API.
| * @param response The WPEFramework JsonObject containing the response from the call. (We can't include WPEFramework headers in controlMgr .h files as their logging macros clash) | |
| * @param retries The number of retries if the call times out. | |
| * @param response A pointer to the boolean response value from the call. |
There was a problem hiding this comment.
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)
src/thunder/ctrlm_thunder_plugin.h:112
- The docstring still refers to a
methodparameter, but the function signature isproperty_get(std::string property, ...). This is misleading for callers/maintainers—please update the@paramname/description to refer to the property being retrieved (and consider aligning wording withproperty_get).
* This function is used to get a Thunder Plugin property.
* @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 The WPEFramework JsonObject containing the response from the call. (We can't include WPEFramework headers in controlMgr .h files as their logging macros clash)
* @param retries The number of retries if the call times out.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/thunder/ctrlm_thunder_plugin.h:112
- The docblock for
property_gethas an incorrect@param methodentry, but the function parameter is namedproperty. This is misleading for callers; update the tag/description to match the actual parameter name and meaning.
* This function is used to get a Thunder Plugin property.
* @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 The WPEFramework JsonObject containing the response from the call. (We can't include WPEFramework headers in controlMgr .h files as their logging macros clash)
* @param retries The number of retries if the call times out.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
No description provided.