RDKEMW-14517: Use IARM for PowerManager clients#50
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request refactors the PowerManager utilities to use IARM (Inter-Architecture Remote Messaging) instead of WPEFramework PowerController. The change simplifies the implementation by removing complex client registration, callback handling, and delay mechanisms that were previously required for the PowerController API.
Changes:
- Replaced WPEFramework PowerController API calls with IARM Bus API calls for power state management
- Simplified command-line interface by removing advanced options (--client, --ack, --delay, --await, NOP operation)
- Updated build configuration to link against libIARMBus instead of libWPEFrameworkPowerController
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 18 comments.
| File | Description |
|---|---|
| iarm_set_powerstate/IARM_BUS_SetPowerStatus.c | Refactored from PowerController to IARM Bus API for setting power states; removed callback handling and client registration logic |
| iarm_query_powerstate/IARM_Bus_CheckPowerStatus.c | Updated to use IARM Bus API for querying power states; maintains backward compatibility with file-based state reading |
| Makefile.am | Updated build dependencies to remove WPEFramework PowerController library and add libIARMBus |
| .gitignore | Completely removed the .gitignore file (concerning as it removes important build artifact exclusions) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| else if (strncasecmp(pPowerON, argv[1], strlen (pPowerON)) == 0) | ||
| { | ||
| printf ("ON Request...\n"); | ||
| param.newState = IARM_BUS_PWRMGR_POWERSTATE_ON; | ||
| } | ||
| else if (strncasecmp(pPowerStandby, argv[1], strlen (pPowerStandby)) == 0) | ||
| { | ||
| param.newState = IARM_BUS_PWRMGR_POWERSTATE_STANDBY; | ||
| printf ("STANDBY Request...\n"); | ||
| } | ||
| else if (strncasecmp(pPowerLigtSleep, argv[1], strlen (pPowerLigtSleep)) == 0) | ||
| { | ||
| param.newState = IARM_BUS_PWRMGR_POWERSTATE_STANDBY_LIGHT_SLEEP; | ||
| printf ("Light Sleep Request...\n"); | ||
| } | ||
| else if (strncasecmp(pPowerDeepSleep, argv[1], strlen (pPowerDeepSleep)) == 0) | ||
| { | ||
| param.newState = IARM_BUS_PWRMGR_POWERSTATE_STANDBY_DEEP_SLEEP; | ||
| printf ("Deep Sleep Request...\n"); | ||
| } | ||
| else if (strncasecmp(pPowerOFF, argv[1], strlen (pPowerOFF)) == 0) |
There was a problem hiding this comment.
Inconsistent spacing around parentheses in 'strlen (pPowerON)'. The codebase convention based on other files shows 'strlen(pPowerON)' without a space before the opening parenthesis. This pattern repeats on lines 61, 66, 71, and 76.
| else if (strncasecmp(pPowerON, argv[1], strlen (pPowerON)) == 0) | |
| { | |
| printf ("ON Request...\n"); | |
| param.newState = IARM_BUS_PWRMGR_POWERSTATE_ON; | |
| } | |
| else if (strncasecmp(pPowerStandby, argv[1], strlen (pPowerStandby)) == 0) | |
| { | |
| param.newState = IARM_BUS_PWRMGR_POWERSTATE_STANDBY; | |
| printf ("STANDBY Request...\n"); | |
| } | |
| else if (strncasecmp(pPowerLigtSleep, argv[1], strlen (pPowerLigtSleep)) == 0) | |
| { | |
| param.newState = IARM_BUS_PWRMGR_POWERSTATE_STANDBY_LIGHT_SLEEP; | |
| printf ("Light Sleep Request...\n"); | |
| } | |
| else if (strncasecmp(pPowerDeepSleep, argv[1], strlen (pPowerDeepSleep)) == 0) | |
| { | |
| param.newState = IARM_BUS_PWRMGR_POWERSTATE_STANDBY_DEEP_SLEEP; | |
| printf ("Deep Sleep Request...\n"); | |
| } | |
| else if (strncasecmp(pPowerOFF, argv[1], strlen (pPowerOFF)) == 0) | |
| else if (strncasecmp(pPowerON, argv[1], strlen(pPowerON)) == 0) | |
| { | |
| printf ("ON Request...\n"); | |
| param.newState = IARM_BUS_PWRMGR_POWERSTATE_ON; | |
| } | |
| else if (strncasecmp(pPowerStandby, argv[1], strlen(pPowerStandby)) == 0) | |
| { | |
| param.newState = IARM_BUS_PWRMGR_POWERSTATE_STANDBY; | |
| printf ("STANDBY Request...\n"); | |
| } | |
| else if (strncasecmp(pPowerLigtSleep, argv[1], strlen(pPowerLigtSleep)) == 0) | |
| { | |
| param.newState = IARM_BUS_PWRMGR_POWERSTATE_STANDBY_LIGHT_SLEEP; | |
| printf ("Light Sleep Request...\n"); | |
| } | |
| else if (strncasecmp(pPowerDeepSleep, argv[1], strlen(pPowerDeepSleep)) == 0) | |
| { | |
| param.newState = IARM_BUS_PWRMGR_POWERSTATE_STANDBY_DEEP_SLEEP; | |
| printf ("Deep Sleep Request...\n"); | |
| } | |
| else if (strncasecmp(pPowerOFF, argv[1], strlen(pPowerOFF)) == 0) |
| } PWRMgr_Settings_t; | ||
|
|
||
| void usage() | ||
| void usage () |
There was a problem hiding this comment.
Inconsistent spacing in 'usage ()' function declaration. The codebase convention based on other files shows 'usage(void)' or 'usage()' without a space before the opening parenthesis.
| void usage () | |
| void usage() |
| const char *pPowerON = "ON"; | ||
| const char *pPowerOFF = "OFF"; | ||
| const char *pPowerStandby = "STANDBY"; | ||
| const char *pPowerLigtSleep = "LIGHTSLEEP"; |
There was a problem hiding this comment.
Spelling error in variable name: 'pPowerLigtSleep' should be 'pPowerLightSleep'. The word 'Light' is misspelled as 'Ligt'.
| const char *pPowerLigtSleep = "LIGHTSLEEP"; | |
| const char *pPowerLightSleep = "LIGHTSLEEP"; |
| if (pwrSettings.powerState == IARM_BUS_PWRMGR_POWERSTATE_OFF) | ||
| printf ("OFF"); | ||
| else if (pwrSettings.powerState == IARM_BUS_PWRMGR_POWERSTATE_STANDBY) | ||
| printf ("STANDBY"); | ||
| else if (pwrSettings.powerState == IARM_BUS_PWRMGR_POWERSTATE_ON) | ||
| printf ("ON"); | ||
| else if (pwrSettings.powerState == IARM_BUS_PWRMGR_POWERSTATE_STANDBY_LIGHT_SLEEP) | ||
| printf ("LIGHTSLEEP"); | ||
| else if (pwrSettings.powerState == IARM_BUS_PWRMGR_POWERSTATE_STANDBY_DEEP_SLEEP) | ||
| printf ("DEEPSLEEP"); | ||
| else | ||
| printf ("Unknown Power state"); | ||
| } | ||
|
|
||
| printf("\n"); | ||
| else | ||
| printf ("Error in reading PWRMgr settings File"); |
There was a problem hiding this comment.
Missing braces around single-statement if blocks. While this is syntactically correct, it's a best practice to use braces even for single statements to prevent future bugs when code is modified. This applies to the if-else chain starting at line 130.
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| */ |
There was a problem hiding this comment.
Missing space after the closing comment delimiter. The comment ends with '*/' but should have a space before it for proper formatting consistency with the rest of the codebase.
| if (argc < 2) | ||
| { | ||
| usage(); | ||
| return EXIT_FAILURE; | ||
| } | ||
|
|
||
| if (strncasecmp(pPowerON, argstate, strlen(pPowerON)) == 0) { | ||
| powerstate = POWER_STATE_ON; | ||
| printf("ON Request...\n"); | ||
| } else if (strncasecmp(pPowerStandby, argstate, strlen(pPowerStandby)) == 0) { | ||
| powerstate = POWER_STATE_STANDBY; | ||
| printf("STANDBY Request...\n"); | ||
| } else if (strncasecmp(pPowerLigtSleep, argstate, strlen(pPowerLigtSleep)) == 0) { | ||
| powerstate = POWER_STATE_STANDBY_LIGHT_SLEEP; | ||
| printf("Light Sleep Request...\n"); | ||
| } else if (strncasecmp(pPowerDeepSleep, argstate, strlen(pPowerDeepSleep)) == 0) { | ||
| powerstate = POWER_STATE_STANDBY_DEEP_SLEEP; | ||
| printf("Deep Sleep Request...\n"); | ||
| } else if (strncasecmp(pPowerOFF, argstate, strlen(pPowerOFF)) == 0) { | ||
| powerstate = POWER_STATE_OFF; | ||
| printf("OFF Request...\n"); | ||
| } else if (strcmp(argstate, pNOP) == 0) { | ||
| powerstate = POWER_STATE_UNKNOWN; | ||
| printf("NOP Request...\n"); | ||
| } else { | ||
| else if (strncasecmp(pPowerON, argv[1], strlen (pPowerON)) == 0) | ||
| { | ||
| printf ("ON Request...\n"); | ||
| param.newState = IARM_BUS_PWRMGR_POWERSTATE_ON; | ||
| } | ||
| else if (strncasecmp(pPowerStandby, argv[1], strlen (pPowerStandby)) == 0) | ||
| { | ||
| param.newState = IARM_BUS_PWRMGR_POWERSTATE_STANDBY; | ||
| printf ("STANDBY Request...\n"); | ||
| } | ||
| else if (strncasecmp(pPowerLigtSleep, argv[1], strlen (pPowerLigtSleep)) == 0) | ||
| { | ||
| param.newState = IARM_BUS_PWRMGR_POWERSTATE_STANDBY_LIGHT_SLEEP; | ||
| printf ("Light Sleep Request...\n"); | ||
| } | ||
| else if (strncasecmp(pPowerDeepSleep, argv[1], strlen (pPowerDeepSleep)) == 0) | ||
| { | ||
| param.newState = IARM_BUS_PWRMGR_POWERSTATE_STANDBY_DEEP_SLEEP; | ||
| printf ("Deep Sleep Request...\n"); | ||
| } | ||
| else if (strncasecmp(pPowerOFF, argv[1], strlen (pPowerOFF)) == 0) | ||
| { | ||
| param.newState = IARM_BUS_PWRMGR_POWERSTATE_OFF; | ||
| printf ("OFF Request...\n"); | ||
| } | ||
| else | ||
| { | ||
| usage(); | ||
| return EXIT_FAILURE; | ||
| } |
There was a problem hiding this comment.
When usage() is called with invalid or missing arguments, the program should return an error code (e.g., return 1 or EXIT_FAILURE) to indicate failure to the calling shell. Currently, after calling usage() for invalid arguments (lines 54 and 83), the program continues execution and may attempt to set the power state with an uninitialized or default value.
| if (param.curState == IARM_BUS_PWRMGR_POWERSTATE_OFF) | ||
| printf ("OFF"); | ||
| else if (param.curState == IARM_BUS_PWRMGR_POWERSTATE_STANDBY) | ||
| printf ("STANDBY"); | ||
| else if (param.curState == IARM_BUS_PWRMGR_POWERSTATE_ON) | ||
| printf ("ON"); | ||
| else if (param.curState == IARM_BUS_PWRMGR_POWERSTATE_STANDBY_LIGHT_SLEEP) | ||
| printf ("LIGHTSLEEP"); | ||
| else if (param.curState == IARM_BUS_PWRMGR_POWERSTATE_STANDBY_DEEP_SLEEP) | ||
| printf ("DEEPSLEEP"); | ||
| else | ||
| printf ("Unknown"); | ||
| } | ||
|
|
||
| /* Dispose closes RPC conn, do not make any power manager calls after this */ | ||
| PowerController_Term(); | ||
|
|
||
| } else if (argv[1][1] == 'h') { | ||
| else | ||
| printf ("Unknown"); |
There was a problem hiding this comment.
Missing braces around single-statement if blocks. While this is syntactically correct, it's a best practice to use braces even for single statements to prevent future bugs when code is modified. This applies to the if-else chain starting at line 92.
| PWRMgr_LED_Settings_t ledSettings; | ||
| uint32_t deep_sleep_timeout; |
There was a problem hiding this comment.
Trailing whitespace after the struct member. For code cleanliness, trailing spaces should be removed.
| PWRMgr_LED_Settings_t ledSettings; | |
| uint32_t deep_sleep_timeout; | |
| PWRMgr_LED_Settings_t ledSettings; | |
| uint32_t deep_sleep_timeout; |
| printf ("\t\t OFF -> Set to OFF\n"); | ||
| } | ||
|
|
||
| void setPowerMode(); |
There was a problem hiding this comment.
The forward declaration 'void setPowerMode();' is defined but never used in the code. This declaration should be removed as it serves no purpose and may cause confusion.
| void setPowerMode(); |
|
|
||
|
|
There was a problem hiding this comment.
Inconsistent whitespace: there's a trailing space on this empty line. For consistency with the rest of the codebase, empty lines should not contain any whitespace characters.
RDKEMW-14517: Use IARM based PowerManager
Signed-off-by: Yuvaramachandran Gurusamy [yuvaramachandran_gurusamy@comcast.com]