RDKEMW-13117: Use IARM for PowerManager clients - #135
RDKEMW-13117: Use IARM for PowerManager clients#135yuvaramachandran-gurusamy wants to merge 2 commits into
Conversation
Signed-off-by: apatel859 <amit_patel5@comcast.com>
Signed-off-by: apatel859 <amit_patel5@comcast.com>
There was a problem hiding this comment.
Pull request overview
This PR replaces PowerManager COM-RPC interface calls with direct IARM (Inter-Application Resource Manager) API calls for power state management in the FrontPanel plugin. The change eliminates the dependency on the PowerManager Thunder plugin interface, simplifying the architecture by using IARM's native power management APIs directly.
Changes:
- Removed PowerManager COM-RPC interface dependencies (IPowerManager, PowerManagerInterface) from FrontPanel plugin
- Replaced PowerManager notification callbacks with IARM event handlers for power mode changes
- Migrated power state queries from PowerManager API to IARM_Bus_Call with IARM_BUS_PWRMGR_API_GetPowerState
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
| helpers/frontpanel.h | Removed PluginHost::IShell parameter from instance() method signature |
| helpers/frontpanel.cpp | Replaced PowerManager GetPowerState call with IARM_Bus_Call for power state initialization |
| FrontPanel/FrontPanel.h | Removed PowerManagerNotification class and related member variables; added IARM initialization methods |
| FrontPanel/FrontPanel.cpp | Replaced PowerManager initialization/notification registration with IARM event handler registration for power mode changes |
| if (Utils::IARM::isConnected()) | ||
| { | ||
| IARM_Result_t res; | ||
| IARM_CHECK( IARM_Bus_RemoveEventHandler(IARM_BUS_PWRMGR_NAME,IARM_BUS_PWRMGR_EVENT_MODECHANGED, powerModeChange) ); |
There was a problem hiding this comment.
Missing space after comma in function call. For consistency with coding style, there should be a space after the commas in the IARM_Bus_RemoveEventHandler call. The parameters should be formatted as "IARM_BUS_PWRMGR_NAME, IARM_BUS_PWRMGR_EVENT_MODECHANGED, powerModeChange" instead of "IARM_BUS_PWRMGR_NAME,IARM_BUS_PWRMGR_EVENT_MODECHANGED, powerModeChange".
| IARM_CHECK( IARM_Bus_RemoveEventHandler(IARM_BUS_PWRMGR_NAME,IARM_BUS_PWRMGR_EVENT_MODECHANGED, powerModeChange) ); | |
| IARM_CHECK( IARM_Bus_RemoveEventHandler(IARM_BUS_PWRMGR_NAME, IARM_BUS_PWRMGR_EVENT_MODECHANGED, powerModeChange) ); |
| } | ||
| void FrontPanel::powerModeChange(const char *owner, IARM_EventId_t eventId, void *data, size_t len) | ||
| { | ||
| if (strcmp(owner, IARM_BUS_PWRMGR_NAME) == 0) { |
There was a problem hiding this comment.
Extra space before comparison operator. There are two spaces between "IARM_BUS_PWRMGR_NAME" and "==", but the code style uses a single space. This should be "if (strcmp(owner, IARM_BUS_PWRMGR_NAME) == 0)" instead of "if (strcmp(owner, IARM_BUS_PWRMGR_NAME) == 0)".
| if (strcmp(owner, IARM_BUS_PWRMGR_NAME) == 0) { | |
| if (strcmp(owner, IARM_BUS_PWRMGR_NAME) == 0) { |
| } | ||
|
|
||
| const string FrontPanel::Initialize(PluginHost::IShell *service) | ||
| const string FrontPanel::Initialize(PluginHost::IShell * /* service */) |
There was a problem hiding this comment.
Inconsistent indentation: This line uses tabs instead of spaces. The rest of the file uses spaces for indentation (4 spaces per level). Please change the tab character to spaces to maintain consistent indentation throughout the file.
| CFrontPanel::instance(service); | ||
| CFrontPanel::instance()->start(); | ||
| InitializeIARM(); | ||
| CFrontPanel::instance()->start(); |
There was a problem hiding this comment.
Inconsistent indentation: This line uses a tab character instead of spaces. The rest of the file uses spaces for indentation (4 spaces per level). Please change the tab to spaces to maintain consistent indentation throughout the file.
| LOGINFO("setPowerStatus false"); | ||
| CFrontPanel::instance()->setPowerStatus(false); | ||
| IARM_Result_t res; | ||
| IARM_CHECK( IARM_Bus_RegisterEventHandler(IARM_BUS_PWRMGR_NAME,IARM_BUS_PWRMGR_EVENT_MODECHANGED, powerModeChange) ); |
There was a problem hiding this comment.
Missing space after comma in function call. For consistency with the codebase style (as seen in line 262), there should be a space after the commas in the IARM_Bus_RegisterEventHandler call. The parameters should be formatted as "IARM_BUS_PWRMGR_NAME, IARM_BUS_PWRMGR_EVENT_MODECHANGED, powerModeChange" instead of "IARM_BUS_PWRMGR_NAME,IARM_BUS_PWRMGR_EVENT_MODECHANGED, powerModeChange".
| IARM_Bus_PWRMgr_EventData_t *param = (IARM_Bus_PWRMgr_EventData_t *)data; | ||
| LOGINFO("Event IARM_BUS_PWRMGR_EVENT_MODECHANGED: State Changed %d -- > %d\r", | ||
| param->data.state.curState, param->data.state.newState); | ||
| if(param->data.state.newState == IARM_BUS_PWRMGR_POWERSTATE_ON) |
There was a problem hiding this comment.
Missing space after 'if' keyword. For consistency with C++ coding standards and the rest of the codebase, there should be a space between 'if' and the opening parenthesis. This should be "if (param->data.state.newState == IARM_BUS_PWRMGR_POWERSTATE_ON)" instead of "if(param->data.state.newState == IARM_BUS_PWRMGR_POWERSTATE_ON)".
| if(param->data.state.newState == IARM_BUS_PWRMGR_POWERSTATE_ON) | |
| if (param->data.state.newState == IARM_BUS_PWRMGR_POWERSTATE_ON) |
|
|
||
| using namespace WPEFramework; | ||
| using PowerState = WPEFramework::Exchange::IPowerManager::PowerState; | ||
| #include "pwrMgr.h" |
There was a problem hiding this comment.
The include path for pwrMgr.h is inconsistent with other files in the codebase. In FrontPanel.cpp, the include is "rdk/iarmmgrs-hal/pwrMgr.h", but here it's just "pwrMgr.h". For consistency and to avoid potential build issues, use the full path "rdk/iarmmgrs-hal/pwrMgr.h" as done in FrontPanel/FrontPanel.cpp and LEDControl/LEDControlImplementation.cpp.
| #include "pwrMgr.h" | |
| #include "rdk/iarmmgrs-hal/pwrMgr.h" |
| if (res == IARM_RESULT_SUCCESS) { | ||
| if (param.curState == IARM_BUS_PWRMGR_POWERSTATE_ON) | ||
| powerStatus = true; | ||
| } |
There was a problem hiding this comment.
Missing error logging when IARM_Bus_Call fails. Unlike the previous implementation that logged the power state values with LOGINFO, the new IARM call lacks logging when res != IARM_RESULT_SUCCESS. This makes debugging power state initialization issues more difficult. Consider adding error logging similar to other IARM calls in the codebase, or at minimum logging the result when the call fails.
| if (res == IARM_RESULT_SUCCESS) { | |
| if (param.curState == IARM_BUS_PWRMGR_POWERSTATE_ON) | |
| powerStatus = true; | |
| } | |
| if (res == IARM_RESULT_SUCCESS) { | |
| LOGINFO("IARM_Bus_Call(%s, %s) succeeded: curState=%d", | |
| IARM_BUS_PWRMGR_NAME, IARM_BUS_PWRMGR_API_GetPowerState, param.curState); | |
| if (param.curState == IARM_BUS_PWRMGR_POWERSTATE_ON) | |
| powerStatus = true; | |
| } | |
| else { | |
| LOGERR("IARM_Bus_Call(%s, %s) failed with result=%d", | |
| IARM_BUS_PWRMGR_NAME, IARM_BUS_PWRMGR_API_GetPowerState, res); | |
| } |
| } | ||
| patternUpdateTimer.Revoke(m_updateTimer); | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing cleanup for CFrontPanel in Deinitialize. While CFrontPanel::instance()->start() and addEventObserver(this) are called in Initialize, the corresponding cleanup is not performed in Deinitialize. Consider calling CFrontPanel::instance()->stop() and CFrontPanel::instance()->removeEventObserver(this) in Deinitialize to ensure proper resource cleanup and symmetry with initialization.
| CFrontPanel::instance()->removeEventObserver(this); | |
| CFrontPanel::instance()->stop(); |
| void FrontPanel::InitializeIARM() | ||
| { | ||
| if(newState == WPEFramework::Exchange::IPowerManager::POWER_STATE_ON) | ||
| { | ||
| LOGINFO("setPowerStatus true"); | ||
| CFrontPanel::instance()->setPowerStatus(true); | ||
| } | ||
| else | ||
| if (Utils::IARM::init()) | ||
| { | ||
| LOGINFO("setPowerStatus false"); | ||
| CFrontPanel::instance()->setPowerStatus(false); | ||
| IARM_Result_t res; | ||
| IARM_CHECK( IARM_Bus_RegisterEventHandler(IARM_BUS_PWRMGR_NAME,IARM_BUS_PWRMGR_EVENT_MODECHANGED, powerModeChange) ); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| void FrontPanel::registerEventHandlers() | ||
| void FrontPanel::DeinitializeIARM() | ||
| { | ||
| ASSERT (_powerManagerPlugin); | ||
|
|
||
| if(!_registeredEventHandlers && _powerManagerPlugin) { | ||
| _registeredEventHandlers = true; | ||
| _powerManagerPlugin->Register(_pwrMgrNotification.baseInterface<Exchange::IPowerManager::IModeChangedNotification>()); | ||
| } | ||
| if (Utils::IARM::isConnected()) | ||
| { | ||
| IARM_Result_t res; | ||
| IARM_CHECK( IARM_Bus_RemoveEventHandler(IARM_BUS_PWRMGR_NAME,IARM_BUS_PWRMGR_EVENT_MODECHANGED, powerModeChange) ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing ownership tracking for IARM connection. The InitializeIARM and DeinitializeIARM methods don't track whether this plugin owns the IARM connection. When Utils::IARM::init() returns true, it could mean either (1) IARM was already connected by another plugin, or (2) this plugin successfully initialized IARM. Following the pattern established in RemoteControl (RemoteControl.cpp:104-140) and VoiceControl (VoiceControl.cpp:98-126), a boolean flag (e.g., m_hasOwnProcess) should be added to track ownership, and DeinitializeIARM should only call IARM_Bus_Disconnect() and IARM_Bus_Term() if this plugin owns the connection.
No description provided.