RDKEMW-17086: Enable or Disable MS12 cont audio output mode.

[shashank4388]

https://ccp.sys.comcast.net/browse/RDKEMW-17086

[Copilot]

Pull request overview

This PR adds a new “application audio configuration” control path (intended for MS12 continuous audio output mode and similar per-application behaviors) by introducing new IARM RPC calls (client/server), RPC parameter structs, and new Host APIs to set/get/list supported configurations.

Changes:

• Added IARM bus API identifiers and RPC parameter structs for application audio configuration set/get/list.
• Implemented RPC server handlers in rpc/srv/dsAudio.c that dynamically resolve HAL entry points and service the new RPC calls.
• Added RPC client wrappers in rpc/cli/dsAudio.c and exposed higher-level Host methods in ds/host.cpp / ds/include/host.hpp.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 13 comments.

Show a summary per file

File	Description
rpc/srv/dsAudio.c	Registers new IARM calls and adds server-side handlers for set/get/list; current implementation has critical NULL-deref/stack-overwrite issues and a misleading error message.
rpc/include/dsRpc.h	Adds new IARM API string constants and RPC parameter structs for application audio configuration APIs.
rpc/cli/dsAudio.c	Adds client-side wrappers for set/get/list; current implementation has format-string bugs, missing validation, and doesn’t propagate server error codes.
ds/include/host.hpp	Exposes new Host APIs; currently declares an unimplemented const char* overload.
ds/host.cpp	Implements Host wrappers and adds documentation; contains incorrect @see reference and misleading/unprofessional log messages.
ds/audioOutputPort.cpp	Removes an extraneous blank line.

---

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

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (2)

rpc/cli/dsAudio.c:468

• enable is dereferenced without a NULL check ("*enable = param.enable"). This can crash callers passing a null enable pointer.

    if (audioConfig == NULL) {
	INT_ERROR("%s:Invalid parameter", __func__);
	return dsERR_GENERAL;
    }

ds/include/host.hpp:426

• Host declares setApplicationAudioConfig(const char*, bool) but there is no definition in this PR. Because it is a better match than the std::string overload for string literals, this can easily lead to link errors. Either remove it or provide an inline forwarding wrapper.

    void setAudioAtmosOutputMode(bool enable);
    void setApplicationAudioConfig(const char* audioConfig, bool enable);
    void setAssociatedAudioMixing(const bool mixing);

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (5)

rpc/cli/dsAudio.c:412

• memset(param.audioConfig, ...) passes a struct value where a pointer is required, which will not compile (and is redundant since param is already zeroed). Use &param.audioConfig (or remove the line entirely).

    memset(param.audioConfig, '\0', sizeof(param.audioConfig));

rpc/cli/dsAudio.c:476

• memset(param.audioConfig, ...) passes a struct value where a pointer is required, which will not compile (and is redundant since param is already zeroed). Use &param.audioConfig (or remove the line entirely).

    memset(param.audioConfig, 0, sizeof(param.audioConfig));

rpc/cli/dsAudio.c:434

• On invalid input, this should return dsERR_INVALID_PARAM (consistent with other APIs in this file) rather than dsERR_GENERAL. Also add a newline so the log doesn’t run into subsequent output.

        if (appAudioConfigList == NULL) {
	    INT_ERROR("%s:Invalid parameter", __func__);
	    return dsERR_GENERAL;
        }

rpc/cli/dsAudio.c:468

• enable is dereferenced on success but is never validated. This can segfault if the caller passes NULL. Also return dsERR_INVALID_PARAM for invalid input to match other APIs in this file.

    if (audioConfig == NULL) {
	INT_ERROR("%s:Invalid parameter", __func__);
	return dsERR_GENERAL;
    }

ds/include/host.hpp:425

• setApplicationAudioConfig(const char*, ...) is declared but has no definition in this PR, which can lead to undefined-symbol link errors if used. Consider making it an inline forwarder to the std::string overload (or remove the duplicate overload).

    void setApplicationAudioConfig(const char* audioConfig, bool enable);

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (7)

ds/include/host.hpp:425

• Host declares setApplicationAudioConfig(const char*) but there is no implementation in this PR (only the std::string overload is implemented). This leaves a public API with an unresolved symbol if called; either remove it or provide a small forwarding wrapper.

    void setApplicationAudioConfig(const char* audioConfig, bool enable);

ds/host.cpp:606

• This error log says "Failed to set" but the method is a getter, which makes troubleshooting confusing.

        INT_ERROR("Failed to set Host::getApplicationAudioConfigList\n");

rpc/cli/dsAudio.c:410

• This client API currently drops the server-side dsError_t and always returns dsERR_GENERAL on failure, and it includes personal-name debug logs. Returning the actual param.result aligns with existing dsAudio RPC client patterns and preserves error details.

    INT_ERROR("Shashank...... dscli dsSetApplicationAudioConfig %s\n", audioConfig->configName);
    dsApplicationAudioConfigParam_t param;
    memset(&param, 0, sizeof(param));
    param.handle = handle;
    param.enable = enable;

rpc/cli/dsAudio.c:473

• This client API currently drops the server-side dsError_t and always returns dsERR_GENERAL on failure, and it includes personal-name debug logs. Return param.result (after a successful IARM call) to preserve the actual error and match other client functions in this file.

    INT_ERROR("Shashank...... dscli dsGetApplicationAudioConfig %s\n", audioConfig->configName);
    dsApplicationAudioConfigParam_t param;
    memset(&param, 0, sizeof(param));
    param.handle = handle;
    // By default enable will be false.

rpc/cli/dsAudio.c:450

• Error handling and copying in dsGetApplicationAudioConfigList should avoid printf, preserve the server-side dsError_t (param.result), and ensure copied configName strings are NUL-terminated.

        if (IARM_RESULT_SUCCESS != rpcRet || param.result != dsERR_NONE)
        {
                printf("%s: (GET) GENERAL ERROR\n", __FUNCTION__);
                return dsERR_GENERAL;
        }

rpc/srv/dsAudio.c:3713

• These are normal control-flow logs (function entry / symbol resolved) but are emitted as INT_ERROR, which is misleading and noisy. Nearby RPC wrappers use INT_DEBUG/INT_INFO for the same cases (e.g., rpc/srv/dsAudio.c:4368-4372).

    INT_ERROR("Inside _dsGetApplicationAudioConfigList_t srv\n");
    if (func == 0) {
        void *dllib = dlopen(RDK_DSHAL_NAME, RTLD_LAZY);
        if (dllib) {
            func = (dsGetApplicationAudioConfigList_t) dlsym(dllib, "dsGetApplicationAudioConfigList");

rpc/srv/dsAudio.c:3753

• This is a normal control-flow log but is emitted as INT_ERROR, which is misleading and noisy. Prefer INT_DEBUG (or remove) for function-exit tracing.

    INT_ERROR("Inside _dsGetApplicationAudioConfigList srv\n");

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (5)

rpc/cli/dsAudio.c:410

• The new client RPC wrapper logs personal/debug strings at error level and drops the underlying dsError_t on failure (it always returns dsERR_GENERAL unless fully successful). This makes troubleshooting harder and can spam error logs on normal paths; prefer neutral logs and return param.result when the bus call succeeded.

    INT_ERROR("Shashank...... dscli dsSetApplicationAudioConfig %s\n", audioConfig->configName);
    dsApplicationAudioConfigParam_t param;
    memset(&param, 0, sizeof(param));
    param.handle = handle;
    param.enable = enable;

rpc/cli/dsAudio.c:450

• On error, this wrapper always returns dsERR_GENERAL and logs a generic message. If the RPC call itself succeeded but the server/HAL returned a specific dsError_t, that information should be propagated back to the caller.

        if (IARM_RESULT_SUCCESS != rpcRet || param.result != dsERR_NONE)
        {
                printf("%s: (GET) GENERAL ERROR\n", __FUNCTION__);
                return dsERR_GENERAL;
        }

rpc/cli/dsAudio.c:458

• strncpy here does not guarantee NUL-termination, and the destination buffer is not cleared first. If the source name is exactly DS_MAX_APPLICATION_AUDIO_CONFIG_NAME_LEN-1 bytes, configName may become unterminated and later string operations can read past the buffer.

	for (int count=0; count <  param.appAudioConfigList.returnedCount; count++) {
	    strncpy(appAudioConfigList->config[count].configName, param.appAudioConfigList.config[count].configName, 
			    (DS_MAX_APPLICATION_AUDIO_CONFIG_NAME_LEN -1) );
	}

rpc/cli/dsAudio.c:473

• This new client RPC wrapper logs personal/debug strings at error level and drops the underlying dsError_t on failure (it always returns dsERR_GENERAL unless fully successful). Prefer neutral logs and return param.result when the RPC call succeeded so callers can react appropriately.

    INT_ERROR("Shashank...... dscli dsGetApplicationAudioConfig %s\n", audioConfig->configName);
    dsApplicationAudioConfigParam_t param;
    memset(&param, 0, sizeof(param));
    param.handle = handle;
    // By default enable will be false.

ds/host.cpp:611

• This method appends to the caller-provided vector without clearing it first. Repeated calls will accumulate duplicates, which is inconsistent with existing list-style APIs in this file (e.g., Host::getHostEDID clears its output vector before filling).

    for (int count =0; count < appAudioConfigList.returnedCount; count++) {
        configList.push_back(appAudioConfigList.config[count].configName);
    }

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

rpc/srv/dsAudio.c:3718

• These messages log success and "not defined" cases using INT_ERROR. Elsewhere in this file, successful dlsym loads use INT_DEBUG and missing symbols use INT_INFO (e.g., _dsGetAudioGain at rpc/srv/dsAudio.c:3061-3065). Align the log levels here accordingly.

                INT_ERROR("dsGetApplicationAudioConfigList_t is defined and loaded\r\n");
            }
            else {
                INT_ERROR("dsGetApplicationAudioConfigList is not defined\r\n");

rpc/srv/dsAudio.c:3776

• These messages log success and "not defined" cases using INT_ERROR. Elsewhere in this file, successful dlsym loads use INT_DEBUG and missing symbols use INT_INFO (e.g., _dsGetAudioGain at rpc/srv/dsAudio.c:3061-3065). Align the log levels here accordingly.

                INT_ERROR("dsSetApplicationAudioConfig_t(int, bool) is defined and loaded\r\n");
            }
            else {
                INT_ERROR("dsSetApplicationAudioConfig_t(int, bool) is not defined\r\n");

rpc/srv/dsAudio.c:3827

• These messages log success and "not defined" cases using INT_ERROR. Elsewhere in this file, successful dlsym loads use INT_DEBUG and missing symbols use INT_INFO (e.g., _dsGetAudioGain at rpc/srv/dsAudio.c:3061-3065). Align the log levels here accordingly.

                INT_ERROR("dsGetApplicationAudioConfig_t(int, bool) is defined and loaded\r\n");
            }
            else {
                INT_ERROR("dsGetApplicationAudioConfig_t(int, bool) is not defined\r\n");

rpc/cli/dsAudio.c:456

• The configName copy does not guarantee NUL-termination when the source string is long (strncpy with n = max-1). This can lead to unterminated strings being consumed later.

    for (int count=0; count <  param.appAudioConfigList.returnedCount; count++) {
        strncpy(appAudioConfigList->config[count].configName, param.appAudioConfigList.config[count].configName, 
			    (DS_MAX_APPLICATION_AUDIO_CONFIG_NAME_LEN -1) );
    }

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Suppressed comments (6)

rpc/srv/dsAudio.c:3778

• The dlopen() handle used to resolve dsSetApplicationAudioConfig is not closed on the success path, which leaks a library handle.

            func = (dsSetApplicationAudioConfig_t) dlsym(dllib, "dsSetApplicationAudioConfig");
            if (func) {
                INT_DEBUG("dsSetApplicationAudioConfig_t(int, bool) is defined and loaded\r\n");
            }
            else {

rpc/srv/dsAudio.c:3808

• dlclose(dllib) is called here, but dllib is not in scope (it is declared inside the earlier if-block). This will not compile.

    dlclose(dllib);

rpc/srv/dsAudio.c:3832

• The dlopen() handle used to resolve dsGetApplicationAudioConfig is not closed on the success path, which leaks a library handle.

            func = (dsGetApplicationAudioConfig_t) dlsym(dllib, "dsGetApplicationAudioConfig");
            if (func) {
                INT_DEBUG("dsGetApplicationAudioConfig_t(int, bool) is defined and loaded\r\n");
            }
            else {

rpc/srv/dsAudio.c:3862

• dlclose(dllib) is called here, but dllib is not in scope (it is declared inside the earlier if-block). This will not compile.

    dlclose(dllib);

rpc/srv/dsAudio.c:3723

• The dlopen() handle is never closed on the success path when resolving dsGetApplicationAudioConfigList, and the success message is logged as an error. This leaks the library handle and pollutes error logs.

This issue also appears in the following locations of the same file:

• line 3774
• line 3828

            func = (dsGetApplicationAudioConfigList_t) dlsym(dllib, "dsGetApplicationAudioConfigList");
            if (func) {
                INT_ERROR("dsGetApplicationAudioConfigList_t is defined and loaded\r\n");
            }
            else {

ds/host.cpp:601

• This log message says "Failed to set" but the operation is a get (getApplicationAudioConfigList), which makes debugging misleading.

        INT_ERROR("Failed to set Host::getApplicationAudioConfigList\n");

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Suppressed comments (5)

rpc/srv/dsAudio.c:3772

• func is cached as a static function pointer, but the dlopen() handle is later closed with dlclose(dllib). If the shared library gets unloaded, subsequent calls may invoke a dangling function pointer. Either keep the dlopen() handle alive for the process lifetime or avoid caching the pointer across calls.

    static dsSetApplicationAudioConfig_t func = 0;

rpc/srv/dsAudio.c:3827

• func is cached as a static function pointer, but the dlopen() handle is later closed with dlclose(dllib). If the shared library gets unloaded, subsequent calls may invoke a dangling function pointer. Either keep the dlopen() handle alive for the process lifetime or avoid caching the pointer across calls.

    static dsGetApplicationAudioConfig_t func = 0;

rpc/srv/dsAudio.c:3716

• This log is emitted on the success path (symbol found), but uses INT_ERROR, which makes troubleshooting harder by flagging normal startup as an error.

                INT_ERROR("dsGetApplicationAudioConfigList_t is defined and loaded\r\n");

rpc/cli/dsAudio.c:455

• The strncpy(..., DS_MAX_APPLICATION_AUDIO_CONFIG_NAME_LEN - 1) copy does not guarantee NUL-termination when the source string length is exactly DS_MAX_APPLICATION_AUDIO_CONFIG_NAME_LEN - 1. This can lead to non-terminated strings being propagated to callers (and later used as C-strings).

    for (int count=0; count <  param.appAudioConfigList.returnedCount; count++) {
        strncpy(appAudioConfigList->config[count].configName, param.appAudioConfigList.config[count].configName, 
			    (DS_MAX_APPLICATION_AUDIO_CONFIG_NAME_LEN -1) );
    }

ds/host.cpp:601

• The error message says "Failed to set" but this function is fetching the list. This makes logs misleading when diagnosing failures.

        INT_ERROR("Failed to set Host::getApplicationAudioConfigList\n");

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (8)

rpc/cli/dsAudio.c:432

• The invalid-parameter log is missing a newline and spacing, which makes logs harder to read and inconsistent with other messages in this file.

    if (appAudioConfigList == NULL) {
        INT_ERROR("%s:Invalid parameter", __func__);
	return dsERR_INVALID_PARAM;
    }

rpc/cli/dsAudio.c:465

• The invalid-parameter log is missing a newline and spacing, which makes logs harder to read and inconsistent with other messages in this file.

    if (audioConfig == NULL || enable == NULL) {
        INT_ERROR("%s:Invalid parameter", __func__);
        return dsERR_INVALID_PARAM;
    }

rpc/srv/dsAudio.c:3716

• Successful symbol load is logged with INT_ERROR, which can create noisy error logs and makes real failures harder to spot. Use INT_DEBUG/INT_INFO for the success path and reserve INT_ERROR for failures.

            func = (dsGetApplicationAudioConfigList_t) dlsym(dllib, "dsGetApplicationAudioConfigList");
            if (func) {
                INT_ERROR("dsGetApplicationAudioConfigList_t is defined and loaded\r\n");
            }

rpc/cli/dsAudio.c:405

• The invalid-parameter log is missing a newline and spacing, which makes logs harder to read and inconsistent with other messages in this file.

This issue also appears in the following locations of the same file:

• line 429
• line 462

    if (audioConfig == NULL) {
	INT_ERROR("%s:Invalid parameter", __func__);
	return dsERR_INVALID_PARAM;

ds/host.cpp:603

• This error message says "Failed to set" but the operation is a get. This can mislead operators when troubleshooting.

    if (ret != dsERR_NONE) {
        INT_ERROR("Failed to set Host::getApplicationAudioConfigList\n");
        throw Exception(ret);

ds/include/host.hpp:442

• The new Host method declarations use spacing that differs from the surrounding declarations (e.g., spaces before '(' and around '&' / '*'). Aligning with the local style improves readability and avoids churn in future edits.

    void setApplicationAudioConfig (const std::string &audioConfig, bool enable);
    void getApplicationAudioConfig (const std::string &audioConfig, bool *enable);
    void getApplicationAudioConfigList (std::vector<std::string>& configList);

ds/host.cpp:592

• Doxygen parameter name doesn't match the function signature (it refers to audioConfigList and calls it a pointer, but the function takes a reference named configList). This can confuse API consumers and generated docs.

 * @brief Gets the list of supported audio configurations.
 *
 * @param[out] audioConfigList  - pointer to List of supported audio configurations
 *

ds/host.cpp:549

• New public Host APIs (set/get/getList for application audio config) were added, but there are existing Boost unit tests for Host (test/testHost.cpp) and these new behaviors are not covered. Adding at least basic tests (e.g., invalid-parameter paths and/or stubbed RPC success paths) would help prevent regressions.

void Host::setApplicationAudioConfig (const std::string &audioConfig, bool enable)
{
    dsApplicationAudioConfig_t config;
    memset(config.configName, 0, DS_MAX_APPLICATION_AUDIO_CONFIG_NAME_LEN);
    strncpy(config.configName, audioConfig.c_str(), (DS_MAX_APPLICATION_AUDIO_CONFIG_NAME_LEN - 1));
    dsError_t ret = dsSetApplicationAudioConfig (NULL, &config, enable);