Feature/rdkemw 8587 dlsym - Test

[yuvaramachandran-gurusamy]

No description provided.

[Copilot]

Pull request overview

This PR adds dynamic configuration loading (via dlopen/dlsym) for device settings (audio/video ports, video devices, front panel) and wires those dynamically loaded configurations into the RPC services, while also upgrading internal logging to include function/thread context and replacing some printf usage.

Changes:

• Add dynamic symbol loading (dlopen/dlsym) in ds/manager.cpp and update config loaders (AudioOutputPortConfig, VideoOutputPortConfig, VideoDeviceConfig, FrontPanelConfig) to accept optional dynamically loaded config structs.
• Expose C-ABI bridge helpers (e.g., dsGetVideoPortResolutions, dsGetVideoPortVPorts, dsGetAudioConfigs) and update RPC server code to query runtime port/resolution lists.
• Revise internal logging (dslogger.*) and convert multiple printf calls to INT_* logging.

Reviewed changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 17 comments.

Show a summary per file

File	Description
rpc/srv/dsVideoPort.c	Uses runtime-loaded port/resolution lists (and adds debug dumps) when resolving ports/resolutions.
rpc/srv/dsDisplay.c	Uses runtime-loaded resolution/port lists for EDID filtering and handle→port type mapping.
rpc/srv/dsAudio.c	Uses runtime-loaded audio port list for handle→port type mapping.
ds/videoOutputPortType.cpp	Changes HDCP enable path to resolve a port handle before calling dsEnableHDCP.
ds/videoOutputPortConfig.hpp	Adds dynamic-config struct + new load(videoPortConfigs_t*); exposes supported resolutions vector.
ds/videoOutputPortConfig.cpp	Implements dynamic-config loading, config dump helper, and C-ABI bridge functions for ports/resolutions.
ds/videoOutputPort.cpp	Minor logging change (printf → INT_INFO) and formatting cleanup.
ds/videoDeviceConfig.hpp	Adds dynamic-config struct + new load(videoDeviceConfig_t*).
ds/videoDeviceConfig.cpp	Implements dynamic-config loading and config dump helper.
ds/videoDevice.cpp	Changes supported-resolutions retrieval to use cached resolutions from VideoOutputPortConfig.
ds/manager.cpp	Adds capability-based dynamic loading via dlopen/dlsym and routes initialization through it.
ds/include/videoDevice.hpp	Adds include needed for updated supported-resolution logic.
ds/include/manager.hpp	Adds capability enums + dlsym lookup struct declarations (and related includes).
ds/include/frontPanelConfig.hpp	Adds dynamic-config struct and new load(fpdConfigs_t*) API.
ds/include/dslogger.h	Updates logger API to include function name and switches log level constants to an enum.
ds/host.cpp	Converts a printf to INT_INFO.
ds/hdmiIn.cpp	Converts multiple printf calls to INT_INFO/INT_ERROR.
ds/frontPanelIndicator.cpp	Whitespace cleanup.
ds/frontPanelConfig.cpp	Implements dynamic-config loading for front panel and adds config dump helper.
ds/dslogger.cpp	Implements new ds_log signature and enhanced stderr formatting.
ds/audioOutputPortConfig.hpp	Adds dynamic-config struct + new load(audioConfigs_t*) and audio config export method.
ds/audioOutputPortConfig.cpp	Implements dynamic-config loading and exports dsGetAudioConfigs bridge.

Comments suppressed due to low confidence (2)

rpc/srv/dsVideoPort.c:1962

• getPixelResolution now fetches resolutions into a fixed 16-entry buffer, but dsGetVideoPortResolutions can return/copy more than 16 entries, which can overflow kVidoeResolutionsSettings. Also if iCount comes back as 0, this function returns kVidoeResolutionsSettings[0].pixelResolution (currently zero-initialized) rather than a defined default; consider falling back to kResolutions[kDefaultResIndex] (or equivalent) when the list is empty or no match is found.

	dsVideoPortResolution_t kVidoeResolutionsSettings[16] = {};
	int iCount = 0;
	dsGetVideoPortResolutions(&iCount, kVidoeResolutionsSettings);
	
	// Print kVidoeResolutionsSettings array
	INT_INFO("[DsMgr] ========== Dumping kVidoeResolutionsSettings Array in getPixelResolution ==========\r\n");
	INT_INFO("[DsMgr] Total resolutions retrieved: %d\r\n", iCount);
	for (int k = 0; k < iCount; k++) {
		dsVideoPortResolution_t *res = &kVidoeResolutionsSettings[k];
		INT_INFO("[DsMgr] [%d] name=%s, pixelRes=%d, aspectRatio=%d, stereoMode=%d, frameRate=%d, interlaced=%d\r\n",
				 k, res->name, res->pixelResolution, res->aspectRatio, 
				 res->stereoScopicMode, res->frameRate, res->interlaced);
	}
	INT_INFO("[DsMgr] ========== End of kVidoeResolutionsSettings Dump ==========\r\n");
	
  	dsVideoPortResolution_t *Resn = &kVidoeResolutionsSettings[0]; 
	
	for (int i = 0; i < iCount; i++)
	{
		Resn = &kVidoeResolutionsSettings[i];
		if (resolution.compare(Resn->name) == 0 )
		{
			break;
		}
	}
	return Resn->pixelResolution;

rpc/srv/dsDisplay.c:590

• dsVideoPortResolution_t *presolution = NULL, kVidoeResolutionsSettings[16] = { }; declares kVidoeResolutionsSettings as an array of pointers (because of the *), but later it’s used as an array of structs and passed to dsGetVideoPortResolutions. This should be split into two declarations so the array is dsVideoPortResolution_t kVidoeResolutionsSettings[16]. Also the NULL-check immediately after malloc(sizeof(dsDisplayEDID_t)) is checking edid instead of edidData, so malloc failure wouldn’t be detected correctly.

    dsVideoPortResolution_t *presolution = NULL, kVidoeResolutionsSettings[16] = { } ;
    dsDisplayEDID_t *edidData = (dsDisplayEDID_t*)malloc(sizeof(dsDisplayEDID_t));
    dsVideoPortType_t _VPortType = _GetDisplayPortType(handle);
    if (edid == NULL) {
        free(edidData);
    	return; // Handle malloc failure

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

ds_log redefines enum LogLevel inside the function even though LogLevel is already defined in dslogger.h. This shadowing is redundant and can confuse tooling/warnings; it should be removed. Also levelMap[static_cast<int>(priority)] assumes priority is always in-range; adding a bounds check (or clamping/unknown fallback) would prevent out-of-bounds reads if an invalid value is ever passed.

[Copilot]

The new debug output in this function uses std::cout directly (and there’s also a printf earlier in the function), which bypasses the project logging controls and can spam stdout in production. Please switch these to INT_DEBUG/INT_INFO as appropriate (ideally gated behind the same runtime flag used elsewhere, like /opt/dsMgrDumpDeviceConfigs) and avoid per-resolution/per-call dumps at info level.

[Copilot]

Changing AudioOutputPortConfig::load() to load(audioConfigs_t*) is an API-breaking change. There are existing callers still using the no-arg form (e.g., test/testConfigAOP.cpp:59 and test/testConfigVOP.cpp:60 from repo search), which will fail to compile unless updated. Consider keeping a no-arg overload that forwards to load(nullptr) (or update all call sites in the same PR).

Suggested change

	// Backward-compatible no-arg overload: delegates to pointer-based load.
	void load() { load(nullptr); }

[Copilot]

VideoOutputPortConfig::load() was changed to load(videoPortConfigs_t*), which breaks existing callers that still invoke the no-arg overload (e.g., test/testConfigVOP.cpp:66). Either update those call sites in this PR or keep a backwards-compatible load() overload that delegates to load(nullptr).

Suggested change

	void load() { load(nullptr); }

[Copilot]

VideoDeviceConfig::load() was changed to load(videoDeviceConfig_t*), but there are existing callers still using the previous no-arg API (e.g., test/testVideoDevice.cpp:58). This will cause compile failures unless those call sites are updated or a load() overload is retained that forwards to load(nullptr).

Suggested change

	void load() { load(nullptr); }

[Copilot]

kVidoeResolutionsSettings is fixed at 16 entries, but dsGetVideoPortResolutions returns the full _supportedResolutions.size() and (as currently implemented) writes that many entries into the provided buffer. If the platform has >16 resolutions this will overflow the stack buffer and corrupt memory; either cap the copy to the caller-provided capacity or change the API to treat *pResolutionCount as an in/out (capacity in, actual out) value.

[Copilot]

These forward declarations don’t match the exported C symbols. This file is compiled as C++ (uses std::string), but the wrappers in ds/videoOutputPortConfig.cpp are extern "C" (dsGetVideoPortResolutions / dsGetVideoPortVPorts). Declare them with extern "C" here too, and rename getVideoPortVPorts to dsGetVideoPortVPorts to avoid link failures.

Suggested change

	extern void dsGetVideoPortResolutions(int *pResolutionCount, dsVideoPortResolution_t *pResolutions);
	extern void getVideoPortVPorts(int *pPortSize, dsVideoPortPortConfig_t *pkVideoPorts);
	extern "C" {
	void dsGetVideoPortResolutions(int *pResolutionCount, dsVideoPortResolution_t *pResolutions);
	void dsGetVideoPortVPorts(int *pPortSize, dsVideoPortPortConfig_t *pkVideoPorts);
	}

[Copilot]

This file is compiled as C++ (uses std::string/std::atomic), but dsGetAudioConfigs is exported as extern "C" in ds/audioOutputPortConfig.cpp. The forward declaration here should be extern "C" as well to avoid name-mangling/link errors. Also _GetAudioPortType uses a fixed kAudioPorts[16] buffer while dsGetAudioConfigs returns _aPorts.size() and currently copies that many entries without checking capacity, which can overflow this stack buffer.

[Copilot]

getAudioConfigs writes _aPorts.size() entries into pkAudioPorts without any way for the caller to indicate the buffer capacity. Callers in rpc/srv/dsAudio.c pass a fixed 16-entry stack array, so platforms with >16 ports will overflow. Consider changing the API contract (e.g., treat *pPortSize as in/out capacity, or add a separate maxPorts argument) and clamp the copy to the provided capacity.

Suggested change

	*pPortSize = _aPorts.size();
	if (nullptr != pkAudioPorts) {
	// Reverse logic: Create pKPorts from _aPorts
	for (size_t i = 0; i < _aPorts.size(); i++) {
	pkAudioPorts[i].id.type = (dsAudioPortType_t)_aPorts[i].getType().getId();
	pkAudioPorts[i].id.index = _aPorts[i].getIndex();
	}
	INT_INFO("Populated %zu audio ports to pkAudioPorts", _aPorts.size());
	// Total number of available ports
	const size_t totalPorts = _aPorts.size();
	// Interpret incoming *pPortSize (if > 0) as the capacity of pkAudioPorts (in elements).
	size_t capacity = 0;
	if (*pPortSize > 0) {
	capacity = static_cast<size_t>(*pPortSize);
	}
	// Report the total number of ports back to the caller.
	*pPortSize = static_cast<int>(totalPorts);
	if (nullptr != pkAudioPorts) {
	// Determine how many entries we are allowed to write into pkAudioPorts.
	size_t portsToCopy = totalPorts;
	if (capacity > 0 && capacity < portsToCopy) {
	portsToCopy = capacity;
	}
	// Reverse logic: Create pkAudioPorts from _aPorts, clamped to buffer capacity.
	for (size_t i = 0; i < portsToCopy; i++) {
	pkAudioPorts[i].id.type = (dsAudioPortType_t)_aPorts[i].getType().getId();
	pkAudioPorts[i].id.index = _aPorts[i].getIndex();
	}
	if (portsToCopy < totalPorts) {
	INT_WARN("Truncated audio port list: copied %zu of %zu ports into pkAudioPorts",
	portsToCopy, totalPorts);
	} else {
	INT_INFO("Populated %zu audio ports to pkAudioPorts", totalPorts);
	}

[Copilot]

using namespace std; in a public header leaks std symbols into every translation unit that includes manager.hpp, which can cause name collisions and surprising compile errors. Please remove this from the header and qualify std:: where needed (or keep using directives limited to .cpp files).

Suggested change

using namespace std;

[Copilot]

Pull request overview

Copilot reviewed 31 out of 31 changed files in this pull request and generated 24 comments.

Comments suppressed due to low confidence (3)

rpc/srv/dsDisplay.c:636

• Missing NULL pointer check. If dsGetVideoPortResolutions returns NULL for kVidoeResolutionsSettings, the code will dereference a NULL pointer at lines 604-607 and 623-630. Add a NULL check after getting the resolutions and before using them.

    dsVideoPortResolution_t *presolution = NULL, *kVidoeResolutionsSettings = NULL;
    dsDisplayEDID_t *edidData = (dsDisplayEDID_t*)malloc(sizeof(dsDisplayEDID_t));
    dsVideoPortType_t _VPortType = _GetDisplayPortType(handle);
    if (edid == NULL) {
        free(edidData);
    	return; // Handle malloc failure
    }
    int numOfSupportedResolution = 0;

    if(_VPortType == dsVIDEOPORT_TYPE_HDMI)
    { 
        INT_DEBUG("EDID for HDMI Port\r\n");    
        //size_t iCount = dsUTL_DIM(kResolutions);
        int iCount = 0;
        //Get details from libds
        dsGetVideoPortResolutions(&iCount, &kVidoeResolutionsSettings);
        
        // Print kVidoeResolutionsSettings array
        INT_INFO("[DsMgr] ========== Dumping kVidoeResolutionsSettings Array ==========\r\n");
        INT_INFO("[DsMgr] Total resolutions retrieved: %d\r\n", iCount);
        for (int k = 0; k < iCount; k++) {
            dsVideoPortResolution_t *res = &kVidoeResolutionsSettings[k];
            INT_INFO("[DsMgr] [%d] name=%s, pixelRes=%d, aspectRatio=%d, stereoMode=%d, frameRate=%d, interlaced=%d\r\n",
                     k, res->name, res->pixelResolution, res->aspectRatio, 
                     res->stereoScopicMode, res->frameRate, res->interlaced);
        }
        INT_INFO("[DsMgr] ========== End of kVidoeResolutionsSettings Dump ==========\r\n");
        
        /*Initialize the struct*/
        memset(edidData,0,sizeof(*edidData));
        /*Copy the content */
        rc = memcpy_s(edidData,sizeof(*edidData),edid,sizeof(*edidData));
        if(rc!=EOK)
        {
                ERR_CHK(rc);
        }
        INT_INFO("[DsMgr] Total Resolution Count from HAL: %d\r\n",edid->numOfSupportedResolution);
        edid->numOfSupportedResolution = 0;
        for (size_t i = 0; i < iCount; i++)
        {
            presolution = &kVidoeResolutionsSettings[i];
            for (size_t j = 0; j < edidData->numOfSupportedResolution; j++)
            {    
                edidResn = &(edidData->suppResolutionList[j]);
                
                if (0 == (strcmp(presolution->name,edidResn->name)))
                {
                    edid->suppResolutionList[edid->numOfSupportedResolution] = kVidoeResolutionsSettings[i];
                    edid->numOfSupportedResolution++;
                    numOfSupportedResolution++;
                    INT_DEBUG("[DsMgr] presolution->name : %s, resolution count : %d\r\n",presolution->name,numOfSupportedResolution);
                }
            }    
        }    

rpc/srv/dsVideoPort.c:2458

• Missing NULL pointer check. If dsGetVideoPortPortConfigs returns NULL for kVideoPortPorts, the code will dereference a NULL pointer at lines 2442-2446 and 2452-2454. Add a NULL check after getting the config and before using it.

    const dsVideoPortPortConfig_t *kVideoPortPorts = NULL;
    
    dsGetVideoPortPortConfigs(&numPorts, &kVideoPortPorts);
    
    // Print kVideoPortPorts array
    INT_INFO("[DsMgr] ========== Dumping kVideoPortPorts Array in dsGetDefaultPortHandle ==========\r\n");
    INT_INFO("[DsMgr] Total ports retrieved: %d\r\n", numPorts);
    for (int k = 0; k < numPorts; k++) {
        const dsVideoPortPortConfig_t *port = &kVideoPortPorts[k];
        INT_INFO("[DsMgr] [%d] type=%d, index=%d, connectedAOP_type=%d, connectedAOP_index=%d, defaultResolution=%s\r\n",
                 k, port->id.type, port->id.index, 
                 port->connectedAOP.type, port->connectedAOP.index, 
                 port->defaultResolution ? port->defaultResolution : "NULL");
    }
    INT_INFO("[DsMgr] ========== End of kVideoPortPorts Dump ==========\r\n");
    
    for(i=0; i< numPorts; i++)
    {
        dsGetVideoPort(kVideoPortPorts[i].id.type, kVideoPortPorts[i].id.index, &halhandle);
        if (dsVIDEOPORT_TYPE_HDMI == kVideoPortPorts[i].id.type ||
             dsVIDEOPORT_TYPE_INTERNAL == kVideoPortPorts[i].id.type)
        {
            return halhandle;
        }
    }

rpc/srv/dsVideoPort.c:1970

• Missing NULL pointer check. If dsGetVideoPortResolutions returns NULL for kVidoeResolutionsSettings, the code will dereference a NULL pointer at lines 1952-1955 and 1959-1963. Add a NULL check after getting the resolutions and before using them.

	dsVideoPortResolution_t *kVidoeResolutionsSettings = NULL;
	int iCount = 0;
	dsGetVideoPortResolutions(&iCount, &kVidoeResolutionsSettings);
	
	// Print kVidoeResolutionsSettings array
	INT_INFO("[DsMgr] ========== Dumping kVidoeResolutionsSettings Array in getPixelResolution ==========\r\n");
	INT_INFO("[DsMgr] Total resolutions retrieved: %d\r\n", iCount);
	for (int k = 0; k < iCount; k++) {
		dsVideoPortResolution_t *res = &kVidoeResolutionsSettings[k];
		INT_INFO("[DsMgr] [%d] name=%s, pixelRes=%d, aspectRatio=%d, stereoMode=%d, frameRate=%d, interlaced=%d\r\n",
				 k, res->name, res->pixelResolution, res->aspectRatio, 
				 res->stereoScopicMode, res->frameRate, res->interlaced);
	}
	INT_INFO("[DsMgr] ========== End of kVidoeResolutionsSettings Dump ==========\r\n");
	
  	dsVideoPortResolution_t *Resn = &kVidoeResolutionsSettings[0]; 
	
	for (int i = 0; i < iCount; i++)
	{
		Resn = &kVidoeResolutionsSettings[i];
		if (resolution.compare(Resn->name) == 0 )
		{
			break;
		}
	}
	return Resn->pixelResolution;
}

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Missing NULL pointer validation. The 'source' parameter is not validated before being used in memcpy. If source is NULL, this will cause undefined behavior. Add a NULL check for the source parameter before the memcpy operation.

[Copilot]

Inconsistent use of NULL and nullptr. In C code, nullptr is a C++ keyword and should not be used. Use NULL instead for consistency and C89/C99 compatibility.

[Copilot]

Inconsistent use of NULL and nullptr. In C code, nullptr is a C++ keyword and should not be used. Use NULL instead for consistency and C89/C99 compatibility.

[Copilot]

Memory leak: Allocated memory for video port configurations (allocatedVideoPortConfigs, allocatedVideoPortPorts, allocatedVideoPortResolutions) is never freed. There should be a cleanup function to free these resources, or the load function should free previously allocated memory before allocating new memory to prevent leaks on repeated calls.

[Copilot]

Missing NULL pointer validation. The 'source' parameter is not validated before being used in memcpy. If source is NULL, this will cause undefined behavior. Add a NULL check for the source parameter before the memcpy operation.

[Copilot]

Incorrect API usage. dsGetDefaultResolutionIndex() returns an int directly, not through an output parameter. The call should be int defaultIndex = dsGetDefaultResolutionIndex(); instead of passing &defaultIndex as a parameter.

Suggested change

	dsGetDefaultResolutionIndex(&defaultIndex);
	defaultIndex = dsGetDefaultResolutionIndex();

[Copilot]

Memory leak: Allocated memory for video device configurations (allocatedVideoDeviceConfigs) is never freed. There should be a cleanup function to free these resources, or the load function should free previously allocated memory before allocating new memory to prevent leaks on repeated calls.

[Copilot]

Misspelled variable name: 'kVidoeResolutionsSettings' should be 'kVideoResolutionsSettings'. The typo appears in multiple locations and should be corrected for consistency and clarity.

[Copilot]

Misspelled comment: 'funcation' should be 'function'.

Suggested change

	//This funcation does not have any caller.
	//This function does not have any caller.

[Copilot]

Missing NULL pointer validation. The 'source' parameter is not validated before being used in memcpy. If source is NULL, this will cause undefined behavior. Add a NULL check for the source parameter before the memcpy operation.

[Copilot]

Pull request overview

Copilot reviewed 31 out of 31 changed files in this pull request and generated 32 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Potential null pointer dereference in getter function. If pKDefaultResIndex is NULL, this will crash when dereferenced.

[Copilot]

Potential null pointer dereference in getter function. If pKConfigSize is NULL, this will crash when dereferenced.

[Copilot]

Typo in variable name used in logging. Should be kVideoResolutionsSettings instead of kVidoeResolutionsSettings.

[Copilot]

Memory leak: Memory allocated for pKVideoPortConfigs, pKVideoPortPorts, and pKVideoPortResolutionsSettings is never freed. There should be a cleanup/release function to free these allocations, or they should be freed before being reallocated if this function is called multiple times.

[Copilot]

Potential null pointer dereference in getter function. If pKResolutionsSettings_size is NULL, this will crash when dereferenced.

[Copilot]

Potential null pointer dereference in getter function. If pKVideoPortConfigs_size is NULL (which it is after static initialization), this will crash when dereferenced.

[Copilot]

Duplicate type definition. The videoPortConfigs_t type is defined both in videoOutputPortConfig.hpp (lines 39-47) and in manager.hpp. This can lead to ODR violations. Consider using a shared header.

Suggested change

	typedef struct videoPortConfigs
	{
	const dsVideoPortTypeConfig_t *pKConfigs;
	int *pKVideoPortConfigs_size;
	const dsVideoPortPortConfig_t *pKPorts;
	int *pKVideoPortPorts_size;
	dsVideoPortResolution_t *pKResolutionsSettings;
	int *pKResolutionsSettings_size;
	}videoPortConfigs_t;
	typedef struct videoPortConfigs videoPortConfigs_t;

[Copilot]

Duplicate type definition. The fpdConfigs_t type is defined both here and in manager.hpp. This can lead to ODR violations. Consider using a shared header.

[Copilot]

Potential null pointer dereference in getter function. If pKVideoDeviceConfigs_size is NULL, this will crash when dereferenced.

[Copilot]

Duplicate type definition. The videoDeviceConfig_t type is defined both in this header file and in dsVideoDeviceConfig.h (lines 37-41). This can lead to ODR violations if both headers are included. Consider consolidating the definitions.

[Copilot]

Pull request overview

Copilot reviewed 31 out of 31 changed files in this pull request and generated 17 comments.

Comments suppressed due to low confidence (1)

rpc/srv/dsVideoPort.c:1969

• getPixelResolution() sets Resn = &kVidoeResolutionsSettings[0] even when iCount == 0 or kVidoeResolutionsSettings == NULL, and then returns Resn->pixelResolution. This will crash when no resolutions are loaded. Add a guard for iCount <= 0 / null pointer and return a safe default (e.g., use the configured default resolution index).

  	dsVideoPortResolution_t *Resn = &kVidoeResolutionsSettings[0]; 
	
	for (int i = 0; i < iCount; i++)
	{
		Resn = &kVidoeResolutionsSettings[i];
		if (resolution.compare(Resn->name) == 0 )
		{
			break;
		}
	}
	return Resn->pixelResolution;

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The header declares dsLoadAudioOutputPortConfig as returning void, but the implementation in rpc/srv/dsAudioConfig.c defines it as returning int and appears to use return codes. Please make the declaration match the definition (prefer returning an explicit status code) so compilation and callers are consistent.

Suggested change

	*/
	void dsLoadAudioOutputPortConfig(const audioConfigs_t* dynamicAudioConfigs);
	* @return Status code indicating success or failure
	*/
	int dsLoadAudioOutputPortConfig(const audioConfigs_t* dynamicAudioConfigs);

[Copilot]

dsAudioConfigFree() is used from rpc/srv/dsMgr.c, but it is not declared in this public header. Expose the *Free() API (or another explicit lifecycle API) in the header so other translation units can include it and compile cleanly.

[Copilot]

dsLoadVideoOutputPortConfig() allocates new arrays every call but doesn't free any previously allocated videoPortConfiguration.* buffers first. If configs are reloaded (e.g., re-init or hot reload), this will leak memory and may leave stale pointers. Free/replace existing allocations at the start of the load function (or reuse dsVideoPortConfigFree() internally).

[Copilot]

dsLoadAudioOutputPortConfig is declared as void in rpc/include/dsAudioConfig.h, but is defined here as returning int, and it also has bare return; statements and no final return value. This will fail to compile due to conflicting types and invalid returns; align the signature with the header and consistently return a status (or make it void everywhere).

[Copilot]

videoDeviceDumpconfig() uses access() but this file doesn't include <unistd.h>, which will fail to compile when built as C++. Add <unistd.h> in this compilation unit (or another header included here) before using access().

[Copilot]

The header declares dsLoadVideoDeviceConfig as void, but the implementation returns int and appears intended to return status. Update this prototype to match the implementation (or change the implementation to void) so the build doesn't fail with conflicting types.

Suggested change

	void dsLoadVideoDeviceConfig(const videoDeviceConfig_t* dynamicVideoDeviceConfigs);
	int dsLoadVideoDeviceConfig(const videoDeviceConfig_t* dynamicVideoDeviceConfigs);

[Copilot]

videoPortDumpconfig() dereferences typeCfg->supportedResolutions unconditionally (e.g., typeCfg->supportedResolutions->name). If a platform config provides supportedResolutions == NULL (or numSupportedResolutions == 0), this will crash when dumping is enabled. Guard the pointer before dereferencing and log something sensible when it's absent.

[Copilot]

dsMgr_term() calls dsAudioConfigFree(), dsVideoDeviceConfigFree(), and dsVideoPortConfigFree(), but dsMgr.c doesn't include headers that declare these functions (and the headers in this PR currently don't declare the *Free() APIs). This will fail to compile in C++. Add the proper prototypes (preferably in the corresponding ds*Config.h headers) and include those headers here.

[Copilot]

This adds verbose cout-based debug logging in getSupportedResolutions() that runs whenever isDynamicList == 0. Writing to stdout/stderr directly from a library can be noisy and hard to control in production. Prefer the existing INT_* logging and gate it behind a debug flag (or remove once validated).

[Copilot]

The detailed resolution dump added here runs unconditionally for HDMI EDID filtering and can significantly increase log volume. Consider gating it behind INT_DEBUG and/or the existing /opt/dsMgrDumpDeviceConfigs toggle used elsewhere.

[Copilot]

Pull request overview

Copilot reviewed 31 out of 31 changed files in this pull request and generated 16 comments.

Comments suppressed due to low confidence (1)

ds/frontPanelConfig.cpp:108

• The frontPanelConfig.load() method was removed from the getInstance() method but is now expected to be called separately via loadDeviceCapabilities(). However, there's no guard to prevent operations before load() is called. If code tries to use front panel functionality before loadDeviceCapabilities() is invoked, it will fail because _colors, _indicators, and _textDisplays vectors will be empty. Consider adding a check in getter methods or documenting this initialization requirement.

FrontPanelConfig & FrontPanelConfig::getInstance()
{
    static FrontPanelConfig _singleton;
    if (!_singleton.m_isFPInitialized)
    {
        dsError_t errorCode = dsERR_NONE;
        unsigned int retryCount = 1;
        do
        {
            errorCode = dsFPInit();
            if (dsERR_NONE == errorCode)
            {
                _singleton.m_isFPInitialized = true;
                INT_INFO("dsFPInit success\n");
            }
            else{
                INT_ERROR("dsFPInit failed with error[%d]. Retrying... (%d/20)\n", errorCode, retryCount);
                usleep(50000); // Sleep for 50ms before retrying
            }
        }
        while ((!_singleton.m_isFPInitialized) && (retryCount++ < 20));
    }
	return _singleton;

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Typo in variable name 'kVidoeResolutionsSettings' - should be 'kVideoResolutionsSettings' (missing 'd' in 'Video'). This typo appears multiple times in the file and affects code readability.

[Copilot]

Shallow copy of structures with pointer members. The memcpy operation performs a shallow copy of dsVideoPortTypeConfig_t structures. Based on the stub definition at stubs/dsVideoPortSettings.h:93-103, these structures contain pointer members like 'supportedResolutions' (line 101). When copying from dynamic configs loaded via dlsym, the pointers will reference memory in the dynamically loaded library. If the library is unloaded (as it is at line 168 of dsConfigs.c), these pointers become dangling, leading to undefined behavior when accessed later. Consider either keeping the library loaded or performing deep copies of the data.

[Copilot]

Shallow copy of structures with pointer members. The memcpy operation performs a shallow copy of dsAudioTypeConfig_t structures which likely contain pointer members (encodings, compressions, stereoModes arrays based on usage at audioOutputPortConfig.cpp:229-241). When copying from dynamic configs loaded via dlsym, the pointers reference memory in the dynamically loaded library. If the library is unloaded (as it is at line 168 of dsConfigs.c), these pointers become dangling, leading to undefined behavior. Consider either keeping the library loaded or performing deep copies.

[Copilot]

Mixing C and C++ null pointer constants. In C code (line 52), 'nullptr' is used which is a C++ keyword. In C, use 'NULL' instead. This will cause a compilation error in C code.

Suggested change

	if (nullptr == config) {
	if (NULL == config) {

[Copilot]

Duplicate function implementation. loadDeviceCapabilities is implemented in both manager.cpp (lines 101-184) and dsConfigs.c (lines 84-172). These implementations are nearly identical and load the same device configurations. This code duplication violates DRY principle and creates maintenance burden. Consider consolidating into a single shared implementation or clearly documenting why both are needed.

[Copilot]

The logging function signature changed to include a 'func' parameter, but the INT_INFO macro at line 114 and 134 doesn't include the '\n' character which was present in the previous logging. This inconsistency could affect log formatting, though it appears the new logging function adds its own newline at line 73 of dslogger.cpp.

[Copilot]

Potential buffer overflow risk. The log formatting uses kFormatMessageSize (896 bytes) for the user message, but then formats an additional header that could be up to ~128 bytes. If the user message is exactly 896 bytes, the total formatted output at line 66-72 could exceed MAX_LOG_BUFF (1024 bytes), causing buffer overflow. Consider reducing kFormatMessageSize further or using dynamic allocation.

[Copilot]

The function return type is declared as 'int' but the function always returns 0 or -1. However, the caller in dsConfigs.c (line 111) checks the result but doesn't actually use the return value properly - it checks 'isSymbolsLoaded' which is the result of this function, then passes it as a boolean to determine whether to use dynamic or static config. The function should return a boolean type (or the caller should be updated to handle -1 as false).

[Copilot]

The function signature declares return type as 'int', but at line 53 in the header file it's declared as 'void'. This is a function signature mismatch between declaration and definition that will cause compilation errors.

[Copilot]

Duplicate log message at lines 252 and 256. The same information is being logged twice consecutively, which is redundant and clutters the logs.