-
Notifications
You must be signed in to change notification settings - Fork 4
RDKEMW-8587: consume the config variables using dlsym() in MW #185
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
ds/frontPanelConfig.cpp
Outdated
| int indicatorColorSize = -1; | ||
| int textDisplaySize = (configuration->pKTextDisplays_size) ? *(configuration->pKTextDisplays_size) : -1; | ||
| // Dump the configuration details | ||
| INT_INFO("\n\n===========================================================================\n\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this two line be comboned like
INT_INFO("\n===========Starting to Dump FrontPanel Configs=======\n");
ds/frontPanelConfig.cpp
Outdated
| #include "dslogger.h" | ||
| #include "manager.hpp" | ||
|
|
||
| #define DEBUG 1 // Using for dumpconfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you already have access("/opt/dsMgrDumpDeviceConfigs", chec kwhy we need DEBUG 1?
ds/videoOutputPortConfig.cpp
Outdated
| INT_INFO("resolution->interlaced= %d", resolution->interlaced); | ||
| } | ||
| INT_INFO("\n\n####################################################################### \n\n"); | ||
| INT_INFO("\n\n####################################################################### \n\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reduce unnecessary logs every where
apatel859
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i see excess logs can u reduce as commented in all placeses
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements dynamic configuration loading using dlsym() for the device settings middleware layer, enabling runtime loading of device capabilities from shared libraries instead of static compile-time configuration.
Key Changes:
- Introduced
loadDeviceCapabilities()function in manager.cpp that uses dlsym() to dynamically load configuration symbols from RDK_DSHAL_NAME library - Modified
load()methods across all config classes (Audio/Video/FrontPanel) to accept configuration structure pointers, supporting both dynamic and static configurations - Enhanced logging infrastructure by adding function name to log macros and replacing printf statements with INT_INFO/INT_ERROR for consistency
- Added dumpconfig() functions for debugging configuration data when
/opt/dsMgrDumpDeviceConfigsfile exists
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 25 comments.
Show a summary per file
| File | Description |
|---|---|
| ds/videoOutputPortConfig.hpp | Added videoPortConfigs_t struct and modified load() signature to accept config pointer |
| ds/videoOutputPortConfig.cpp | Implemented dynamic config loading with fallback to static, added dumpconfig() function, converted printf to INT_INFO |
| ds/videoDeviceConfig.hpp | Added videoDeviceConfig_t struct and modified load() signature |
| ds/videoDeviceConfig.cpp | Implemented dynamic config loading with fallback to static, added dumpconfig() function |
| ds/audioOutputPortConfig.hpp | Added audioConfigs_t struct and modified load() signature |
| ds/audioOutputPortConfig.cpp | Implemented dynamic config loading with fallback to static, added dumpconfig() function |
| ds/frontPanelConfig.hpp | Added fpdConfigs_t struct, moved load() to public, added m_isFPConfigLoaded flag |
| ds/frontPanelConfig.cpp | Implemented dynamic config loading, removed automatic load() call from getInstance(), added dumpconfig() |
| ds/manager.cpp | Added LoadDLSymbols() and loadDeviceCapabilities() functions, integrated dynamic loading into Initialize() and load() |
| ds/include/manager.hpp | Added DeviceCapabilityTypes enum, dlSymbolLookup struct, function declarations |
| ds/include/dslogger.h | Changed LogLevel to enum, added function parameter to ds_log() signature |
| ds/dslogger.cpp | Enhanced logging with thread ID, function name, and improved formatting |
| ds/videoOutputPort.cpp | Code formatting changes (indentation) and printf to INT_INFO conversion |
| ds/host.cpp | Converted printf to INT_INFO |
| ds/hdmiIn.cpp | Converted printf to INT_INFO throughout |
| ds/frontPanelIndicator.cpp | Removed empty lines (formatting only) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #include "dslogger.h" | ||
| #include "manager.hpp" | ||
|
|
||
| #define DEBUG 1 // Using for dumpconfig |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'DEBUG' macro is defined but never used in this file. If it's intended to control the dumpconfig functionality, it should be checked before calling dumpconfig(), or removed if unnecessary. Consider removing it to avoid confusion.
| #define DEBUG 1 // Using for dumpconfig |
| static void DeInitialize(); | ||
| static void load(); //!< This function is being used for loading configure in-process DSMgr. | ||
| static int IsInitialized; //!< Indicates the application has initialized with devicettings modules. | ||
| static int IsInitialized; //!< Indicates the application has initialized with devicettings modules. |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment references 'devicettings' which appears to be a typo for 'device settings'. This typo exists in the original code but should be corrected for clarity.
| static int IsInitialized; //!< Indicates the application has initialized with devicettings modules. | |
| static int IsInitialized; //!< Indicates the application has initialized with Device Settings modules. |
| {std::lock_guard<std::mutex> lock(gManagerInitMutex); | ||
| printf("Entering %s count %d with thread id %lu\n",__FUNCTION__,IsInitialized,pthread_self()); | ||
|
|
||
| INT_INFO("Entering ... count %d with thread id %lu\n",IsInitialized,pthread_self()); |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The format string at line 230 has an unnecessary '\n' character. The INT_INFO macro already handles line termination as shown in dslogger.cpp line 66-72 where '\n' is added. Remove the '\n' from the format string for consistency.
| INT_INFO("Entering ... count %d with thread id %lu\n",IsInitialized,pthread_self()); | |
| INT_INFO("Entering ... count %d with thread id %lu",IsInitialized,pthread_self()); |
| } | ||
| else | ||
| { | ||
| INT_ERROR("No valid text display configuration found\n"); |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The INT_ERROR message at line 507 ends with '\n', but the INT_ERROR macro already adds a newline. This results in extra blank lines in the log output. Remove the '\n' from the format string.
| INT_ERROR("No valid text display configuration found\n"); | |
| INT_ERROR("No valid text display configuration found"); |
| if (( false == m_isFPInitialized) || (true == m_isFPConfigLoaded)) { | ||
| INT_ERROR("'%s'", (!m_isFPInitialized) ? "Front Panel not initialized" : "Front Panel Config already loaded"); |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function returns early at line 427 if config is already loaded or if FP is not initialized, but the check should ensure that if FP is not initialized, it should return an error or throw an exception. The current logic with return statement may silently fail without notifying the caller of the configuration load failure. Consider throwing an exception instead of returning silently.
| if (( false == m_isFPInitialized) || (true == m_isFPConfigLoaded)) { | |
| INT_ERROR("'%s'", (!m_isFPInitialized) ? "Front Panel not initialized" : "Front Panel Config already loaded"); | |
| if (!m_isFPInitialized) { | |
| INT_ERROR("'Front Panel not initialized'"); | |
| throw IllegalArgumentException("Front Panel not initialized"); | |
| } | |
| if (m_isFPConfigLoaded) { | |
| INT_ERROR("'Front Panel Config already loaded'"); |
| _singleton.m_isFPInitialized = true; | ||
| INT_INFO("dsFPInit success\n"); |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The load() function is being called with nullptr when symbols are not loaded. However, the frontend panel initialization in FrontPanelConfig::getInstance() (line 97-100) was previously calling load() automatically, which has been removed. Now load() needs to be called explicitly with proper error handling. If the load fails or isn't called, the config won't be initialized, but m_isFPInitialized will still be set to true, leading to an inconsistent state.
| _singleton.m_isFPInitialized = true; | |
| INT_INFO("dsFPInit success\n"); | |
| // Attempt to load the front panel configuration | |
| if (_singleton.load()) | |
| { | |
| _singleton.m_isFPInitialized = true; | |
| INT_INFO("dsFPInit and config load success\n"); | |
| } | |
| else | |
| { | |
| INT_ERROR("dsFPInit succeeded but config load failed\n"); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the load() to Manager::Initialize().
| configuration.pKConfigs = kConfigs; | ||
| configSize = dsUTL_DIM(kConfigs); | ||
| configuration.pKConfigSize = &configSize; | ||
| configuration.pKPorts = kPorts; | ||
| portSize = dsUTL_DIM(kPorts); | ||
| configuration.pKPortSize = &portSize; |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the issue in videoOutputPortConfig.cpp, the size pointers in the configuration structure point to local stack variables ('configSize', 'portSize'). These local variables will go out of scope when the function returns, making the pointers invalid. This could lead to undefined behavior if the configuration structure is accessed after function completion.
| configuration.pKFPDIndicatorColors = kIndicatorColors; | ||
| indicatorColorSize = dsUTL_DIM(kIndicatorColors); | ||
| configuration.pKFPDIndicatorColors_size = &indicatorColorSize; | ||
| configuration.pKIndicators = kIndicators; | ||
| indicatorSize = dsUTL_DIM(kIndicators); | ||
| configuration.pKIndicators_size = &indicatorSize; | ||
| configuration.pKTextDisplays = kTextDisplays; | ||
| textDisplaySize = dsUTL_DIM(kTextDisplays); | ||
| configuration.pKTextDisplays_size = &textDisplaySize; |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the issue in other config files, the size pointers in the configuration structure point to local stack variables ('indicatorColorSize', 'indicatorSize', 'textDisplaySize'). These local variables will go out of scope when the function returns, making the pointers invalid. This could lead to undefined behavior if the configuration structure is accessed after function completion.
| int indicatorSize = (configuration->pKIndicators_size) ? *(configuration->pKIndicators_size) : -1; | ||
| int indicatorColorSize = (configuration->pKFPDIndicatorColors_size) ? *(configuration->pKFPDIndicatorColors_size) : -1; |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable 'indicatorColorSize' is declared twice - once at line 355 with initialization to -1, and again at line 363 inside the if block where it's reassigned. The first declaration is shadowed by the second, making the initial assignment ineffective. Remove the second declaration at line 362 to avoid shadowing.
| int indicatorSize = (configuration->pKIndicators_size) ? *(configuration->pKIndicators_size) : -1; | |
| int indicatorColorSize = (configuration->pKFPDIndicatorColors_size) ? *(configuration->pKFPDIndicatorColors_size) : -1; | |
| indicatorSize = (configuration->pKIndicators_size) ? *(configuration->pKIndicators_size) : -1; | |
| indicatorColorSize = (configuration->pKFPDIndicatorColors_size) ? *(configuration->pKFPDIndicatorColors_size) : -1; |
ds/videoOutputPortConfig.cpp
Outdated
| #define DEBUG 1 // Using for dumpconfig | ||
|
|
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'DEBUG' macro is defined but never used in this file. If it's intended to control the dumpconfig functionality, it should be checked before calling dumpconfig(), or removed if unnecessary. Consider removing it to avoid confusion.
| #define DEBUG 1 // Using for dumpconfig |
ds/frontPanelConfig.cpp
Outdated
| fpdTextDisplayCfg->levels, | ||
| fpdTextDisplayCfg->maxHorizontalIterations, | ||
| fpdTextDisplayCfg->maxVerticalIterations, | ||
| (fpdTextDisplayCfg->supportedCharacters) ? fpdTextDisplayCfg->supportedCharacters : "NULL", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of NULL use macro that u have defined for "ABCDxxxx"
apatel859
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix it
| device::VideoDeviceConfig::getInstance().load(); | ||
| printf("%d:%s load completed\n", __LINE__, __FUNCTION__); | ||
| INT_INFO("Enter function"); | ||
| loadDeviceCapabilities( device::DEVICE_CAPABILITY_VIDEO_PORT | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not DEVICE_CAPABILITY_FRONT_PANEL here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apatel859 , This manager::load() called only from dsMgr context. And FrontPanel libds controlled from FrontPanel plugin. So we dont need FrontPanel config load here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the DEVICE_CAPABILITY_FRONT_PANEL to use in dsmgr context.
ds/audioOutputPortConfig.cpp
Outdated
| } | ||
| void AudioOutputPortConfig::load(audioConfigs_t* dynamicAudioConfigs) | ||
| { | ||
| static int configSize = -1, portSize = -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We dont need static variables here
ds/frontPanelConfig.cpp
Outdated
| * 1. Create Supported Colors. | ||
| * 2. Create Indicators. | ||
| */ | ||
| static int indicatorSize, indicatorColorSize, textDisplaySize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We dont need static variables here
| #include "dslogger.h" | ||
| #include "manager.hpp" | ||
|
|
||
| #define DEBUG 1 // Using for dumpconfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove this
ds/videoDeviceConfig.cpp
Outdated
|
|
||
| void VideoDeviceConfig::load(videoDeviceConfig_t* dynamicVideoDeviceConfigs) | ||
| { | ||
| static int configSize = -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We dont need static variables here
ds/videoOutputPortConfig.cpp
Outdated
| #include <string.h> | ||
| using namespace std; | ||
|
|
||
| #define DEBUG 1 // Using for dumpconfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove this
ds/manager.cpp
Outdated
| loadDeviceCapabilities( device::DEVICE_CAPABILITY_VIDEO_PORT | | ||
| device::DEVICE_CAPABILITY_AUDIO_PORT | | ||
| device::DEVICE_CAPABILITY_VIDEO_DEVICE | | ||
| device::DEVICE_CAPABILITY_FRONT_PANEL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@santoshcomcast , Can we remove FrontPanel here once @apatel859 agreed based on comments as above
ds/include/frontPanelConfig.hpp
Outdated
| /* Terminate Front Panel */ | ||
| void fPTerm(); | ||
|
|
||
| void load(fpdConfigs_t* pDLHandle); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename pDLHandle into dynamicConfigPtr or something
ds/audioOutputPortConfig.hpp
Outdated
| List<AudioOutputPortType> getSupportedTypes(); | ||
|
|
||
| void load(); | ||
| void load(audioConfigs_t* pDLHandle); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename pDLHandle into dynamicConfigPtr or something
ds/videoDeviceConfig.hpp
Outdated
| VideoDFC & getDefaultDFC(); | ||
|
|
||
| void load(); | ||
| void load(videoDeviceConfig_t* pDLHandle); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename pDLHandle into dynamicConfigPtr or something
ds/videoOutputPortConfig.hpp
Outdated
| List<VideoResolution> getSupportedResolutions(bool isIgnoreEdid=false); | ||
|
|
||
| void load(); | ||
| void load(videoPortConfigs_t* pDLHandle); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename pDLHandle into dynamicConfigPtr or something
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
Signed-off-by: grandhi_santoshkumar <grandhi_santoshkumar@comcast.com>
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by: grandhi_santoshkumar <grandhi_santoshkumar@comcast.com>
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
Reason for change: consume the config variables using dlsym() in MW. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
Signed-off-by: grandhi_santoshkumar <grandhi_santoshkumar@comcast.com>
Signed-off-by: grandhi_santoshkumar <grandhi_santoshkumar@comcast.com>
Signed-off-by: grandhi_santoshkumar <grandhi_santoshkumar@comcast.com>
Signed-off-by: yuvaramachandran_gurusamy <yuvaramachandran_gurusamy@comcast.com>
Signed-off-by: yuvaramachandran_gurusamy <yuvaramachandran_gurusamy@comcast.com>
Signed-off-by: yuvaramachandran_gurusamy <yuvaramachandran_gurusamy@comcast.com>
Signed-off-by: yuvaramachandran_gurusamy <yuvaramachandran_gurusamy@comcast.com>
…logging format Signed-off-by: yuvaramachandran_gurusamy <yuvaramachandran_gurusamy@comcast.com>
Signed-off-by: yuvaramachandran_gurusamy <yuvaramachandran_gurusamy@comcast.com>
Reason for change: Address review comments. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
Reason for change: Addressed review comments. Test Procedure: refer RDKEMW-8587 Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
ef2294f to
b8cdf99
Compare
RDKEMW-8587: consume the config variables using dlsym() in MW.
Reason for change: consume the config variables using dlsym() in MW.
Test Procedure: refer RDKEMW-8587
Risks: High
Signed-off-by:gsanto722 grandhi_santoshkumar@comcast.com