Thunder team review by ManimaranRenganathan · Pull Request #212 · rdkcentral/devicesettings

ManimaranRenganathan · 2026-02-05T13:00:56Z

No description provided.

… and Interface changes

…Thunder is enabled.

Copilot

Pull request overview

This pull request introduces Thunder COM-RPC support for the Front Panel Display (FPD) functionality as an alternative to the existing IARM-based implementation. The PR enables conditional compilation to support both communication frameworks.

Changes:

Adds new dsFPD-com.cpp implementation using Thunder COM-RPC framework
Implements conditional compilation in build system (Makefile, Makefile.am, configure.ac) to select between IARM and Thunder modes
Adds Thunder library dependencies when Thunder plugin support is enabled

Reviewed changes

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

Show a summary per file

File	Description
rpc/cli/dsFPD-com.cpp	New Thunder COM-RPC implementation for FPD client, providing C API wrappers around Thunder interface
rpc/cli/Makefile.am	Conditional source file selection and Thunder library linking based on configuration flag
rpc/cli/Makefile	Conditional object file compilation and library linking for Thunder vs IARM modes
configure.ac	Adds --enable-thunder-plugin configuration option with USE_WPE_THUNDER_PLUGIN define
cov_build.sh	Exports USE_WPE_THUNDER_PLUGIN environment variable for coverage builds

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

Copilot · 2026-02-05T13:06:19Z

rpc/cli/Makefile

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


These commented-out lines should be removed rather than kept in the codebase. The version control system preserves the history, so keeping commented code creates clutter and confusion about what the intended implementation should be.

Suggested change

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

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

Copilot · 2026-02-05T13:06:20Z

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 initialized and set to true in the destructor, but it's never actually used anywhere in the code. Consider removing this unused member variable to reduce code clutter.

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-05T13:06:20Z

rpc/cli/dsFPD-com.cpp

+        return (nullptr != _fpdInterface);
+    }
+
+    inline bool isConnected() const


The method name "isConnected" uses camelCase, which is inconsistent with the PascalCase naming convention used by other methods in this class like "IsOperational" and "IsActivatedLocked". For consistency with the class's public interface, this should be renamed to "IsConnected".

Copilot · 2026-02-05T13:06:20Z

rpc/cli/dsFPD-com.cpp

+    inline bool IsActivatedLocked() const
+    {
+        return (nullptr != _fpdInterface);
+    }
+


The method "IsActivatedLocked" is defined but never called anywhere in the code. Consider removing this unused method to reduce code clutter, or if it's intended for future use, add a comment explaining its purpose.

Suggested change

inline bool IsActivatedLocked() const

{

return (nullptr != _fpdInterface);

}

Copilot · 2026-02-05T13:06:21Z

rpc/cli/dsFPD-com.cpp

+
+    static DeviceSettingsFPD* Instance()
+    {
+        return _instance;


The static Instance method is not thread-safe. It returns the _instance pointer without any locking, which could lead to a race condition if called concurrently with Init or Term methods. The caller could receive a partially initialized pointer or a pointer that's being deleted. Consider acquiring _apiLock before returning the instance, or better yet, ensure callers use Init/Instance pattern within locked sections.

Suggested change

return _instance;

_apiLock.Lock();

DeviceSettingsFPD* instance = _instance;

_apiLock.Unlock();

return instance;

Copilot · 2026-02-05T13:06:21Z

rpc/cli/dsFPD-com.cpp

+    // Note: Interface expects minutes and seconds, but API provides hour and minutes
+    // Converting: treating uMinutes as seconds for interface compatibility
+    uint32_t result = instance->SetFPDTime(timeFormat, uHour, uMinutes);


The parameters are being incorrectly mapped. The Thunder interface expects "minutes" and "seconds" as parameters (line 217-218), but the C API provides "uHour" and "uMinutes". Currently, uHour is passed as the minutes parameter and uMinutes is passed as the seconds parameter, which will cause incorrect time values to be set. The correct mapping should convert hours and minutes to the appropriate format expected by the Thunder interface, or there's a mismatch in the interface definition that needs to be addressed.

Suggested change

// Note: Interface expects minutes and seconds, but API provides hour and minutes

// Converting: treating uMinutes as seconds for interface compatibility

uint32_t result = instance->SetFPDTime(timeFormat, uHour, uMinutes);

// Note: Interface expects minutes and seconds, but API provides hour and minutes.

// Convert hours and minutes to total minutes, and use 0 seconds (no second-level precision in API).

const uint32_t totalMinutes = (uHour * 60u) + (uMinutes % 60u);

const uint32_t seconds = 0u;

uint32_t result = instance->SetFPDTime(timeFormat, totalMinutes, seconds);

Copilot · 2026-02-05T13:06:21Z

rpc/cli/dsFPD-com.cpp

+
+dsError_t dsSetFPDBrightness(dsFPDIndicator_t eIndicator, dsFPDBrightness_t eBrightness, bool toPersist)
+{
+    if (eIndicator >= dsFPD_INDICATOR_MAX || eBrightness > 100) {


The validation check for brightness should use a comparison with dsFPDBrightness_t enum max value or a defined constant instead of hardcoding "100". This makes the code more maintainable and ensures consistency if the brightness range changes. Consider defining a constant like MAX_BRIGHTNESS or using the enum's maximum value.

functionalities from Standalone application

Manimaran Renganathan added 4 commits January 27, 2026 18:23

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

3324427

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.

Added export USE_WPE_THUNDER_PLUGIN=y in cov_build.sh

034e5db

ManimaranRenganathan requested a review from a team as a code owner February 5, 2026 13:00

Copilot AI review requested due to automatic review settings February 5, 2026 13:00

Copilot started reviewing on behalf of ManimaranRenganathan February 5, 2026 13:01 View session

Copilot AI reviewed Feb 5, 2026

View reviewed changes

Added the DSCliLibStandaloneApp.cpp file for validate the FPD

3ed996d

functionalities from Standalone application

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Thunder team review#212

Thunder team review#212
ManimaranRenganathan wants to merge 5 commits intodevelopfrom
Thunder-Team-Review

ManimaranRenganathan commented Feb 5, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

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

-        return _instance;
+        _apiLock.Lock();
+        DeviceSettingsFPD* instance = _instance;
+        _apiLock.Unlock();
+        return instance;

-    // Note: Interface expects minutes and seconds, but API provides hour and minutes
-    // Converting: treating uMinutes as seconds for interface compatibility
-    uint32_t result = instance->SetFPDTime(timeFormat, uHour, uMinutes);
+    // Note: Interface expects minutes and seconds, but API provides hour and minutes.
+    // Convert hours and minutes to total minutes, and use 0 seconds (no second-level precision in API).
+    const uint32_t totalMinutes = (uHour * 60u) + (uMinutes % 60u);
+    const uint32_t seconds = 0u;
+    uint32_t result = instance->SetFPDTime(timeFormat, totalMinutes, seconds);

Conversation

ManimaranRenganathan commented Feb 5, 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 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant