Device setting split by MadhusuthananRavi · Pull Request #213 · rdkcentral/devicesettings

MadhusuthananRavi · 2026-02-06T08:00:26Z

DeviceSettingsManager Cli library Implementation

… and Interface changes

…Thunder is enabled.

This reverts commit 5b0063f.

1. Created the dsConnectionManager.cpp, dsConnectionManager.h and dsHdmiIn-com.cpp files 2. Modified the dsFPD-com.cpp file with SmartInterfaceType to get the common interface from FPD and using QueryInterface to get the interface handle for other component like HdmiIn, CompositeIn, etc.,

This reverts commit 70a09eb.

…/rpc/cli" This reverts commit 402f9bf.

This reverts commit 94475ec.

This reverts commit d220f98.

This reverts commit 5b0063f.

…evicesettings into DeviceSetting_split

Copilot

Pull request overview

This PR implements a Thunder COM-RPC client library for the Device Settings Front Panel Display (FPD) subsystem. The implementation provides an alternative transport mechanism to the existing IARM-based communication, allowing the FPD HAL client library to communicate with a Thunder plugin instead.

Changes:

Added new Thunder COM-RPC implementation for FPD client library (dsFPD-com.cpp)
Implemented conditional compilation support to choose between IARM and Thunder backends at build time
Updated build system (Makefile, Makefile.am, configure.ac) to support Thunder plugin configuration

Reviewed changes

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

Show a summary per file

File	Description
rpc/cli/dsFPD-com.cpp	New Thunder COM-RPC client implementation wrapping FPD APIs with singleton connection manager
rpc/cli/Makefile.am	Added conditional compilation logic to select between dsFPD.c (IARM) and dsFPD-com.cpp (Thunder)
rpc/cli/Makefile	Added conditional compilation and linking flags for Thunder vs IARM modes
cov_build.sh	Enabled Thunder plugin mode for coverage builds
configure.ac	Added --enable-thunder-plugin configure option with associated autoconf macros

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

Copilot · 2026-02-06T08:07:13Z

rpc/cli/dsFPD-com.cpp

+// Forward declarations
+dsError_t dsSetFPDBrightness(dsFPDIndicator_t eIndicator, dsFPDBrightness_t eBrightness, bool toPersist);
+dsError_t dsSetFPDColor(dsFPDIndicator_t eIndicator, dsFPDColor_t eColor, bool toPersist);
+


The forward declarations for dsSetFPDBrightness and dsSetFPDColor include a persist/toPersist parameter that is not part of the original API signature. The original functions (dsSetFPBrightness and dsSetFPColor) don't take a persist parameter - they call the "D" versions with persist=true. These forward declarations don't match the actual implementations and should be removed since they're not needed (the functions are defined before they're used).

Suggested change

// Forward declarations

dsError_t dsSetFPDBrightness(dsFPDIndicator_t eIndicator, dsFPDBrightness_t eBrightness, bool toPersist);

dsError_t dsSetFPDColor(dsFPDIndicator_t eIndicator, dsFPDColor_t eColor, bool toPersist);

Copilot · 2026-02-06T08:07:13Z

rpc/cli/Makefile

+ifdef USE_WPE_THUNDER_PLUGIN
+    # Thunder mode - use dsFPD-com.cpp, exclude dsFPD.c
+    OBJS := $(patsubst %.cpp,%.o,$(wildcard *.cpp))
+    OBJS += $(patsubst %.c,%.o,$(filter-out dsFPD.c,$(wildcard *.c)))
+else
+    # IARM mode - use dsFPD.c, exclude dsFPD-com.cpp
+    OBJS := $(patsubst %.cpp,%.o,$(filter-out dsFPD-com.cpp,$(wildcard *.cpp)))
+    OBJS += $(patsubst %.c,%.o,$(wildcard *.c))
+endif


The Makefile conditionally includes/excludes files based on USE_WPE_THUNDER_PLUGIN but doesn't add -DUSE_WPE_THUNDER_PLUGIN to CFLAGS. The dsFPD-com.cpp file guards all its code with #ifdef USE_WPE_THUNDER_PLUGIN, which won't be defined during compilation. Add '-DUSE_WPE_THUNDER_PLUGIN' to CFLAGS when this mode is enabled, similar to how LDLIBS is conditionally set.

Copilot · 2026-02-06T08:07:14Z

rpc/cli/Makefile

+#OBJS        := $(patsubst %.cpp,%.o,$(wildcard *.cpp))
+#OBJS        += $(patsubst %.c,%.o,$(wildcard *.c))


These commented-out lines should be removed. They represent the old implementation that has been replaced by the conditional logic above. Keeping dead code in comments reduces code clarity and maintainability.

Suggested change

#OBJS := $(patsubst %.cpp,%.o,$(wildcard *.cpp))

#OBJS += $(patsubst %.c,%.o,$(wildcard *.c))

Copilot · 2026-02-06T08:07:14Z

rpc/cli/dsFPD-com.cpp

+class DeviceSettingsFPD : public RPC::SmartInterfaceType<Exchange::IDeviceSettingsFPD> {
+private:
+    using BaseClass = RPC::SmartInterfaceType<Exchange::IDeviceSettingsFPD>;
+
+    Exchange::IDeviceSettingsFPD* _fpdInterface;


The class stores a separate _fpdInterface pointer alongside inheriting from SmartInterfaceType which also manages the interface. This creates potential for confusion and inconsistency. The BaseClass::Interface() method at line 90 returns an interface pointer, which is then stored in _fpdInterface. Verify that this dual management is intentional and that there's no double-release risk. Consider documenting why both are needed or refactoring to use only the base class's interface management.

Copilot · 2026-02-06T08:07:14Z

rpc/cli/dsFPD-com.cpp

+
+    // Wait for plugin to become operational
+    if (!instance->WaitForOperational(5000)) {
+        fprintf(stderr, "[dsFPD-com] DeviceSettings plugin not operational after 5 seconds\n");


The dsFPInit() function creates a singleton instance and waits for it to become operational, but if Init() succeeds but WaitForOperational() times out, the instance remains initialized and won't be cleaned up. This could lead to resource leaks or unexpected state in subsequent calls. Consider calling Term() to clean up the instance if WaitForOperational() fails.

Suggested change

fprintf(stderr, "[dsFPD-com] DeviceSettings plugin not operational after 5 seconds\n");

fprintf(stderr, "[dsFPD-com] DeviceSettings plugin not operational after 5 seconds\n");

DeviceSettingsFPD::Term();

Copilot · 2026-02-06T08:07:16Z

rpc/cli/Makefile.am

+if USE_THUNDER_PLUGIN
+    FPD_SOURCE = dsFPD-com.cpp
+    THUNDER_LIBS = -lWPEFrameworkCore -lWPEFrameworkCOM
+else
+    FPD_SOURCE = dsFPD.c
+    THUNDER_LIBS =
+endif


The Makefile.am conditionally sets FPD_SOURCE and THUNDER_LIBS based on USE_THUNDER_PLUGIN, but there's no corresponding conditional compilation flag (like -DUSE_WPE_THUNDER_PLUGIN) being passed to CPPFLAGS. The dsFPD-com.cpp file uses #ifdef USE_WPE_THUNDER_PLUGIN to guard its content, but this preprocessor define won't be set by the build system. You need to add this define to the CPPFLAGS when USE_THUNDER_PLUGIN is enabled.

Copilot · 2026-02-06T08:07:16Z

rpc/cli/dsFPD-com.cpp

+    bool _shutdown;
+
+    DeviceSettingsFPD()
+        : BaseClass()
+        , _fpdInterface(nullptr)
+        , _connected(false)
+        , _shutdown(false)
+    {
+        (void)Connect();
+    }
+
+    ~DeviceSettingsFPD()
+    {
+        _shutdown = true;


The _shutdown member variable is set to true in the destructor but is never read anywhere in the code. This appears to be dead code. Either remove it or use it to prevent operations after shutdown if that's the intended behavior.

Suggested change

bool _shutdown;

DeviceSettingsFPD()

: BaseClass()

, _fpdInterface(nullptr)

, _connected(false)

, _shutdown(false)

{

(void)Connect();

}

~DeviceSettingsFPD()

{

_shutdown = true;

DeviceSettingsFPD()

: BaseClass()

, _fpdInterface(nullptr)

, _connected(false)

{

(void)Connect();

}

~DeviceSettingsFPD()

{

Copilot · 2026-02-06T08:07:16Z

rpc/cli/dsFPD-com.cpp

+        _apiLock.Lock();
+        if (nullptr == _instance) {
+            _instance = new DeviceSettingsFPD();
+        }
+        _apiLock.Unlock();
+    }
+
+    static void Term()
+    {
+        _apiLock.Lock();
+        if (nullptr != _instance) {
+            delete _instance;
+            _instance = nullptr;
+        }
+        _apiLock.Unlock();
+    }
+
+    static DeviceSettingsFPD* Instance()
+    {
+        return _instance;


The Instance() method is not thread-safe. It returns a raw pointer to _instance without holding the lock, which can lead to a race condition if another thread calls Term() concurrently. Consider either returning the instance while holding the lock or using a reference-counted smart pointer pattern. If the caller must check for null and then use the instance, there's a time-of-check-to-time-of-use (TOCTOU) vulnerability.

Suggested change

_apiLock.Lock();

if (nullptr == _instance) {

_instance = new DeviceSettingsFPD();

}

_apiLock.Unlock();

}

static void Term()

{

_apiLock.Lock();

if (nullptr != _instance) {

delete _instance;

_instance = nullptr;

}

_apiLock.Unlock();

}

static DeviceSettingsFPD* Instance()

{

return _instance;

// Ensure the singleton is constructed in a thread-safe manner.

(void)Instance();

}

static void Term()

{

// No-op: the singleton is a function-local static and will live

// until program termination, avoiding concurrency issues with deletion.

}

static DeviceSettingsFPD* Instance()

{

// Use a function-local static to guarantee thread-safe initialization

// and lifetime for the singleton instance.

static DeviceSettingsFPD instance;

return &instance;

Copilot · 2026-02-06T08:07:17Z

rpc/cli/dsFPD-com.cpp

+
+    Exchange::IDeviceSettingsFPD::FPDIndicator indicator = 
+        static_cast<Exchange::IDeviceSettingsFPD::FPDIndicator>(eIndicator);
+


The toPersist parameter is ignored with a comment stating "Thunder interface doesn't support persist flag". This creates an API compatibility issue where calls to dsSetFPDColor() with toPersist=false will not behave as expected, potentially leading to unintended persistent storage. Consider logging a warning when toPersist is false to alert callers that their request cannot be honored, or document this limitation clearly.

Suggested change

if (!toPersist) {

fprintf(stderr, "[dsFPD-com] Warning: dsSetFPDColor() called with toPersist=false, "

"but Thunder interface does not support non-persistent FPD color settings; "

"request will be applied persistently.\n");

}

Copilot · 2026-02-06T08:07:17Z

rpc/cli/dsFPD-com.cpp

+    virtual void Operational(const bool upAndRunning) override
+    {
+        _apiLock.Lock();
+
+        if (upAndRunning) {
+            // Communicator opened && DeviceSettings is Activated
+            if (nullptr == _fpdInterface) {
+                _fpdInterface = BaseClass::Interface();
+                if (_fpdInterface != nullptr) {
+                    printf("[dsFPD-com] Successfully established COM-RPC connection with DeviceSettings plugin\n");
+                } else {
+                    fprintf(stderr, "[dsFPD-com] Failed to get interface - plugin implementation may have failed to load\n");
+                }
+            }
+        } else {
+            // DeviceSettings is Deactivated || Communicator closed
+            if (nullptr != _fpdInterface) {
+                _fpdInterface->Release();
+                _fpdInterface = nullptr;
+            }
+        }
+        _apiLock.Unlock();
+    }


In the Operational() method, when upAndRunning is false, the code calls _fpdInterface->Release() but does not check if the Release() call succeeds or if it fully cleans up the resource. Additionally, if an exception is thrown while holding _apiLock, the lock will not be released. Consider using RAII lock guards (e.g., std::lock_guard or Core::AutoLock if available) instead of manual Lock()/Unlock() calls to ensure proper cleanup in case of exceptions.

Manimaran Renganathan and others added 17 commits January 27, 2026 18:23

RDKEMW-9659 Implement DeviceSettings client library for FPD in rpc/cli

3324427

HdmiIn srv Hardcoded value POC

5b0063f

Modified the dsFPD-com.cpp code based on latest DeviceSettings plugin…

b6e760c

… and Interface changes

Modified the rpc/cliMakefile with conditionally exclude dsFPD.c when …

fdc4105

…Thunder is enabled.

Revert "HdmiIn srv Hardcoded value POC"

7ee959e

This reverts commit 5b0063f.

Added export USE_WPE_THUNDER_PLUGIN=y in cov_build.sh

034e5db

Resolved compilation errors

94475ec

Added the Makefile and Makefile.am changes in devicesettings /rpc/cli

402f9bf

Resolved undefined reference error in dsHdmiIn-com.cpp file

70a09eb

Revert "Resolved undefined reference error in dsHdmiIn-com.cpp file"

3944baf

This reverts commit 70a09eb.

Revert "Added the Makefile and Makefile.am changes in devicesettings …

7e25fcc

…/rpc/cli" This reverts commit 402f9bf.

Revert "Resolved compilation errors"

8c15bc0

This reverts commit 94475ec.

Revert "Modified the code"

59ccd08

This reverts commit d220f98.

HdmiIn srv Hardcoded value POC

3a26bf2

Revert "HdmiIn srv Hardcoded value POC"

83c9468

This reverts commit 5b0063f.

Merge branch 'DeviceSetting_split' of https://github.com/rdkcentral/d…

46bb7b3

…evicesettings into DeviceSetting_split

MadhusuthananRavi requested a review from a team as a code owner February 6, 2026 08:00

Copilot AI review requested due to automatic review settings February 6, 2026 08:00

Copilot started reviewing on behalf of MadhusuthananRavi February 6, 2026 08:00 View session

Copilot AI reviewed Feb 6, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Device setting split#213

Device setting split#213
MadhusuthananRavi wants to merge 17 commits intodevelopfrom
DeviceSetting_split

MadhusuthananRavi commented Feb 6, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 6, 2026

Uh oh!

Copilot AI Feb 6, 2026

Uh oh!

Copilot AI Feb 6, 2026

Uh oh!

Copilot AI Feb 6, 2026

Uh oh!

Copilot AI Feb 6, 2026

Uh oh!

Copilot AI Feb 6, 2026

Uh oh!

Copilot AI Feb 6, 2026

Uh oh!

Copilot AI Feb 6, 2026

Uh oh!

Copilot AI Feb 6, 2026

Uh oh!

Copilot AI Feb 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

	// Forward declarations
	dsError_t dsSetFPDBrightness(dsFPDIndicator_t eIndicator, dsFPDBrightness_t eBrightness, bool toPersist);
	dsError_t dsSetFPDColor(dsFPDIndicator_t eIndicator, dsFPDColor_t eColor, bool toPersist);

		#OBJS := $(patsubst %.cpp,%.o,$(wildcard *.cpp))
		#OBJS += $(patsubst %.c,%.o,$(wildcard *.c))

	fprintf(stderr, "[dsFPD-com] DeviceSettings plugin not operational after 5 seconds\n");
	fprintf(stderr, "[dsFPD-com] DeviceSettings plugin not operational after 5 seconds\n");
	DeviceSettingsFPD::Term();


		Exchange::IDeviceSettingsFPD::FPDIndicator indicator =
		static_cast<Exchange::IDeviceSettingsFPD::FPDIndicator>(eIndicator);

+    if (!toPersist) {
+        fprintf(stderr, "[dsFPD-com] Warning: dsSetFPDColor() called with toPersist=false, "
+                        "but Thunder interface does not support non-persistent FPD color settings; "
+                        "request will be applied persistently.\n");
+    }

Conversation

MadhusuthananRavi commented Feb 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant