Conversation
… and Interface changes
…Thunder is enabled.
There was a problem hiding this comment.
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.cppimplementation 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.
| #OBJS := $(patsubst %.cpp,%.o,$(wildcard *.cpp)) | ||
| #OBJS += $(patsubst %.c,%.o,$(wildcard *.c)) |
There was a problem hiding this comment.
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.
| #OBJS := $(patsubst %.cpp,%.o,$(wildcard *.cpp)) | |
| #OBJS += $(patsubst %.c,%.o,$(wildcard *.c)) |
rpc/cli/dsFPD-com.cpp
Outdated
| bool _shutdown; | ||
|
|
||
| DeviceSettingsFPD() | ||
| : BaseClass() | ||
| , _fpdInterface(nullptr) | ||
| , _connected(false) | ||
| , _shutdown(false) | ||
| { | ||
| (void)Connect(); | ||
| } | ||
|
|
||
| ~DeviceSettingsFPD() | ||
| { | ||
| _shutdown = true; |
There was a problem hiding this comment.
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.
| bool _shutdown; | |
| DeviceSettingsFPD() | |
| : BaseClass() | |
| , _fpdInterface(nullptr) | |
| , _connected(false) | |
| , _shutdown(false) | |
| { | |
| (void)Connect(); | |
| } | |
| ~DeviceSettingsFPD() | |
| { | |
| _shutdown = true; | |
| DeviceSettingsFPD() | |
| : BaseClass() | |
| , _fpdInterface(nullptr) | |
| , _connected(false) | |
| { | |
| (void)Connect(); | |
| } | |
| ~DeviceSettingsFPD() | |
| { |
rpc/cli/dsFPD-com.cpp
Outdated
| return (nullptr != _fpdInterface); | ||
| } | ||
|
|
||
| inline bool isConnected() const |
There was a problem hiding this comment.
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".
rpc/cli/dsFPD-com.cpp
Outdated
| inline bool IsActivatedLocked() const | ||
| { | ||
| return (nullptr != _fpdInterface); | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
| inline bool IsActivatedLocked() const | |
| { | |
| return (nullptr != _fpdInterface); | |
| } |
rpc/cli/dsFPD-com.cpp
Outdated
|
|
||
| static DeviceSettingsFPD* Instance() | ||
| { | ||
| return _instance; |
There was a problem hiding this comment.
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.
| return _instance; | |
| _apiLock.Lock(); | |
| DeviceSettingsFPD* instance = _instance; | |
| _apiLock.Unlock(); | |
| return instance; |
rpc/cli/dsFPD-com.cpp
Outdated
| // 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); |
There was a problem hiding this comment.
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.
| // 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); |
|
|
||
| dsError_t dsSetFPDBrightness(dsFPDIndicator_t eIndicator, dsFPDBrightness_t eBrightness, bool toPersist) | ||
| { | ||
| if (eIndicator >= dsFPD_INDICATOR_MAX || eBrightness > 100) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| clean: | ||
| @echo "Cleaning the directory..." | ||
| @$(RM) $(BINARIES) | ||
| @$(RM) DSApp |
There was a problem hiding this comment.
clean removes DSApp in the sample directory, but the dsapp target builds the executable to ../DSApp. This leaves the built artifact behind after make clean. Update clean to remove ../DSApp (or invoke dsapp-clean) so cleanup is consistent with the build output path.
| @$(RM) DSApp | |
| @$(RM) ../DSApp |
sample/Makefile
Outdated
| $(CXX) $(CFLAGS) -std=c++11 DSCliLibStandaloneApp.cpp -o ../DSApp -L$(INSTALL)/lib -lds -ldshalcli -lcurl -lpthread $(LDFLAGS); \ | ||
| echo "DSApp built successfully at devicesettings/DSApp"; \ | ||
| else \ |
There was a problem hiding this comment.
The success message says DSApp is built at devicesettings/DSApp, but the command outputs to ../DSApp. Please update the message to reflect the real output location (ideally derived from variables) so users aren’t misled when running the target from different directories.
| CURLcode curl_result = curl_global_init(CURL_GLOBAL_ALL); | ||
| if (curl_result != CURLE_OK) { | ||
| LOGE("Failed to initialize curl: %s", curl_easy_strerror(curl_result)); | ||
| cleanup_resources(); |
There was a problem hiding this comment.
On curl_global_init failure, the code calls cleanup_resources(), which unconditionally calls curl_global_cleanup(). libcurl documents curl_global_cleanup() should only be called after a successful curl_global_init(), otherwise behavior can be undefined. Track whether curl was initialized successfully and only call global cleanup when appropriate (or avoid calling cleanup_resources() on this failure path).
| cleanup_resources(); | |
| // Avoid calling cleanup_resources() here because curl was not initialized | |
| // and cleanup_resources() may call curl_global_cleanup(), which would be undefined. | |
| if (gDebugPrintBuffer) { | |
| delete[] gDebugPrintBuffer; | |
| gDebugPrintBuffer = nullptr; | |
| } | |
| closelog(); |
| libds_Initialize(); | ||
| printf("libds.so initialized successfully!\n"); |
There was a problem hiding this comment.
libds_Initialize() can throw, but it’s called before the try/catch that wraps showMainMenu(). If initialization fails, the exception will escape main() and skip cleanup/logging. Wrap libds_Initialize() in a try/catch in main (and ensure any partially initialized resources are cleaned up) to make startup failures graceful.
| libds_Initialize(); | |
| printf("libds.so initialized successfully!\n"); | |
| try { | |
| libds_Initialize(); | |
| printf("libds.so initialized successfully!\n"); | |
| } catch (const std::exception& e) { | |
| LOGE("Failed to initialize libds.so: %s", e.what()); | |
| cleanup_resources(); | |
| return -1; | |
| } catch (...) { | |
| LOGE("Failed to initialize libds.so: unknown exception"); | |
| cleanup_resources(); | |
| return -1; | |
| } |
| # Conditional compilation for Thunder COM-RPC | ||
| if USE_THUNDER_PLUGIN | ||
| FPD_SOURCE = dsFPD-com.cpp | ||
| THUNDER_LIBS = -lWPEFrameworkCore -lWPEFrameworkCOM | ||
| else | ||
| FPD_SOURCE = dsFPD.c | ||
| THUNDER_LIBS = | ||
| endif | ||
|
|
||
| libdshalcli_la_SOURCES = dsAudio.c dsclientlogger.c dsDisplay.c $(FPD_SOURCE) dsHost.cpp dsVideoDevice.c dsVideoPort.c | ||
| libdshalcli_la_LIBADD = $(THUNDER_LIBS) |
There was a problem hiding this comment.
When USE_THUNDER_PLUGIN is enabled, dsFPD-com.cpp is compiled, but there’s no corresponding -DUSE_WPE_THUNDER_PLUGIN added to CPPFLAGS/CXXFLAGS. Because the file is wrapped in #ifdef USE_WPE_THUNDER_PLUGIN, this can result in an empty translation unit and missing dsFPD* symbols at link time. Add the define to the build flags under the conditional (or remove the #ifdef wrapper).
| #ifdef USE_WPE_THUNDER_PLUGIN | ||
|
|
||
| #include <stdio.h> | ||
| #include <string.h> | ||
| #include <chrono> | ||
| #include <thread> | ||
|
|
There was a problem hiding this comment.
The entire file is wrapped in #ifdef USE_WPE_THUNDER_PLUGIN, but the build toggles for Thunder mode in this PR are primarily make-conditionals (USE_THUNDER_PLUGIN / USE_WPE_THUNDER_PLUGIN make var). If the preprocessor macro isn’t defined, this will compile to an empty object and the library will miss required C API symbols. Either remove this #ifdef (and rely on conditional source selection), or ensure the macro is always defined when compiling this file.
| // Signal handler for graceful shutdown | ||
| static void signal_handler(int signum) { | ||
| const char* signal_name = "UNKNOWN"; | ||
| switch (signum) { | ||
| case SIGTERM: signal_name = "SIGTERM"; break; | ||
| case SIGINT: signal_name = "SIGINT"; break; | ||
| case SIGHUP: signal_name = "SIGHUP"; break; | ||
| case SIGUSR1: signal_name = "SIGUSR1"; break; | ||
| case SIGUSR2: signal_name = "SIGUSR2"; break; | ||
| } | ||
|
|
||
| syslog(LOG_INFO, "[DSApp] Received signal %s (%d), initiating graceful shutdown", signal_name, signum); | ||
|
|
||
| // Mark application as not initialized to trigger cleanup | ||
| pthread_mutex_lock(&gDSAppMutex); | ||
| gAppInitialized = false; | ||
| pthread_mutex_unlock(&gDSAppMutex); | ||
|
|
||
| // Cleanup and exit | ||
| cleanup_resources(); | ||
| closelog(); | ||
| exit(0); | ||
| } |
There was a problem hiding this comment.
The signal handler calls non-async-signal-safe functions (syslog, pthread_mutex_lock, cleanup_resources, closelog, exit). This can deadlock or crash if the signal interrupts code holding locks or inside libc. Prefer setting an atomic/volatile shutdown flag (or writing to a self-pipe) and letting the main loop perform cleanup outside the signal context.
| # Conditional compilation: Thunder vs IARM | ||
| 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 |
There was a problem hiding this comment.
This Makefile switches object selection based on the make variable USE_WPE_THUNDER_PLUGIN, but it never defines the C/C++ preprocessor macro USE_WPE_THUNDER_PLUGIN. Since dsFPD-com.cpp is guarded by #ifdef USE_WPE_THUNDER_PLUGIN, enabling the make variable will compile an object with no symbols and likely break linking (because dsFPD.c is filtered out). Ensure the macro is passed in compiler flags when Thunder mode is enabled (or remove the #ifdef guard and rely solely on source selection).
| AM_CONDITIONAL([USE_THUNDER_PLUGIN], [test "x$enable_thunder_plugin" = "xyes"]) | ||
|
|
||
| AS_IF([test "x$enable_thunder_plugin" = "xyes"], | ||
| [AC_DEFINE([USE_WPE_THUNDER_PLUGIN], [1], [Define to 1 to enable Thunder COM-RPC plugin support]) |
There was a problem hiding this comment.
AC_DEFINE([USE_WPE_THUNDER_PLUGIN], ...) places the macro in cfg/config.h, but none of the rpc/cli sources include that header, and the Makefiles don’t -include it either (verified by searching for config.h). As a result, --enable-thunder-plugin may not actually enable the #ifdef USE_WPE_THUNDER_PLUGIN code. Ensure cfg/config.h is included in compilation (e.g., add -include $(top_builddir)/cfg/config.h or explicitly add -DUSE_WPE_THUNDER_PLUGIN when the option is enabled).
| [AC_DEFINE([USE_WPE_THUNDER_PLUGIN], [1], [Define to 1 to enable Thunder COM-RPC plugin support]) | |
| [AC_DEFINE([USE_WPE_THUNDER_PLUGIN], [1], [Define to 1 to enable Thunder COM-RPC plugin support]) | |
| CPPFLAGS="$CPPFLAGS -DUSE_WPE_THUNDER_PLUGIN" |
| #include <signal.h> | ||
| #include <curl/curl.h> | ||
| #include <iostream> | ||
| #include <cstdint> |
There was a problem hiding this comment.
This file uses printf/scanf/getchar and std::nothrow, but it doesn’t include the headers that declare them (<cstdio>/<stdio.h> and <new> respectively). Depending on the toolchain and included headers, this can fail to compile or rely on non-portable transitive includes. Add the appropriate standard headers explicitly.
| #include <cstdint> | |
| #include <cstdint> | |
| #include <cstdio> // For printf and related C I/O functions | |
| #include <new> // For std::nothrow |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
cov_build.sh:76
cov_build.shsetsUSE_WPE_THUNDER_PLUGIN=y, which triggers the non-autotools Makefiles to select Thunder sources and link-lWPEFrameworkCore/-lWPEFrameworkCOM, but the script does not add-DUSE_WPE_THUNDER_PLUGINto CFLAGS (needed by#ifdef USE_WPE_THUNDER_PLUGIN), nor does it ensure the Thunder libs/headers are available in this Coverity build environment. This will likely either compile out the implementation or fail at link time. Pass the macro viaCFLAGS(or remove the guard) and add the required include/lib paths (or stubs) for the Thunder libraries.
export USE_WPE_THUNDER_PLUGIN=y
find $WORKDIR -iname "*.o" -exec rm -v {} \;
find $WORKDIR -iname "*.so*" -exec rm -v {} \;
echo "##### Triggering make"
make CFLAGS+='-fPIC -DDSMGR_LOGGER_ENABLED=ON -DRDK_DSHAL_NAME=\"libdshal.so\" -I${DS_IF_PATH}/include -I${DS_HAL_PATH} -I${DS_MGRS}/stubs -I${IARMBUS_PATH}/core -I${IARMBUS_PATH}/core/include -I${IARM_MGRS}/sysmgr/include -I${DS_MGRS}/ds/include -I${DS_MGRS}/rpc/include -I${POWER_IF_PATH}/include/ -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I${IARM_MGRS}/mfr/include/ -I${IARM_MGRS}/mfr/common -I${DEEPSLEEP_IF_PATH}/include -I${IARM_MGRS}/hal/include -I${IARM_MGRS}/power -I${IARM_MGRS}/power/include' LDFLAGS="-L/usr/lib/x86_64-linux-gnu/ -L/usr/local/include -lglib-2.0 -lIARMBus -lWPEFrameworkPowerController -ldshal"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @echo "Copying the binaries to bin install folder..." | ||
| @cp $(BINARIES) $(INSTALL)/bin | ||
| @if [ -n "$(BINARIES)" ]; then \ | ||
| @cp $(BINARIES) $(INSTALL)/bin |
There was a problem hiding this comment.
The install recipe’s shell if block is malformed: the cp line is missing a trailing ; \ and it is prefixed with @, which will be passed to the shell as part of the command (likely causing @cp: not found). This will break make install when BINARIES is non-empty. Remove the @ inside the shell block and ensure each continued line is properly terminated (e.g., cp ...; \).
| @cp $(BINARIES) $(INSTALL)/bin | |
| cp $(BINARIES) $(INSTALL)/bin; \ |
| * @{ | ||
| **/ | ||
|
|
||
| #ifdef USE_WPE_THUNDER_PLUGIN |
There was a problem hiding this comment.
This file is fully wrapped in #ifdef USE_WPE_THUNDER_PLUGIN, but the non-autotools build (rpc/cli/Makefile) enables Thunder mode via a make variable and does not add -DUSE_WPE_THUNDER_PLUGIN to compilation flags. That combination will compile this translation unit as effectively empty and omit all dsFPD symbols at runtime. Either remove the #ifdef wrapper and rely on the build system selecting the correct source file, or ensure the build defines USE_WPE_THUNDER_PLUGIN when Thunder mode is enabled.
| #ifdef USE_WPE_THUNDER_PLUGIN | |
| #if 1 // Previously `#ifdef USE_WPE_THUNDER_PLUGIN`; now always compiled when this file is built. |
| [AC_DEFINE([USE_WPE_THUNDER_PLUGIN], [1], [Define to 1 to enable Thunder COM-RPC plugin support]) | ||
| AC_MSG_NOTICE([Thunder COM-RPC plugin support enabled])], |
There was a problem hiding this comment.
--enable-thunder-plugin toggles compilation/linking against WPEFramework (Core/COM), but configure.ac does not validate that the required headers and libraries are present (no PKG_CHECK_MODULES / AC_CHECK_HEADERS / AC_CHECK_LIB for Thunder). Enabling this option will likely fail later during compilation/linking with a less actionable error. Add explicit checks and surface a clear configure-time failure or disable the option when dependencies are missing.
| [AC_DEFINE([USE_WPE_THUNDER_PLUGIN], [1], [Define to 1 to enable Thunder COM-RPC plugin support]) | |
| AC_MSG_NOTICE([Thunder COM-RPC plugin support enabled])], | |
| [PKG_CHECK_MODULES([THUNDER], [WPEFrameworkCore WPEFrameworkCOM], | |
| [AC_DEFINE([USE_WPE_THUNDER_PLUGIN], [1], [Define to 1 to enable Thunder COM-RPC plugin support]) | |
| AC_MSG_NOTICE([Thunder COM-RPC plugin support enabled])], | |
| [AC_MSG_ERROR([Thunder COM-RPC plugin support requested, but WPEFramework Core/COM (pkg-config modules WPEFrameworkCore and WPEFrameworkCOM) were not found. Install the appropriate development packages or re-run configure without --enable-thunder-plugin.])])], |
| all: $(BINARIES) | ||
| @echo "Build Finished...." | ||
| all: $(BINARIES) dsapp | ||
| @echo "Build Finished and Installed (including DSApp) ..." |
There was a problem hiding this comment.
The all target prints "Build Finished and Installed ..." but it does not invoke the install target (it only builds binaries + dsapp). This message is misleading and makes it hard to tell whether anything was actually installed.
| @echo "Build Finished and Installed (including DSApp) ..." | |
| @echo "Build finished (binaries and DSApp built). Run 'make install' to install ..." |
rpc/cli/dsFPD-com.cpp
Outdated
| _apiLock.Lock(); | ||
| if (nullptr == _instance) { | ||
| _instance = new DeviceSettingsFPD(); | ||
| } | ||
| _apiLock.Unlock(); |
There was a problem hiding this comment.
DeviceSettingsFPD::Init() takes _apiLock and then constructs DeviceSettingsFPD, whose constructor immediately calls Connect(), which tries to take _apiLock again. Since Core::CriticalSection is not recursive, this can deadlock on initialization. Construct the singleton without holding _apiLock, or remove Connect() from the constructor and call it after releasing the lock.
| _apiLock.Lock(); | |
| if (nullptr == _instance) { | |
| _instance = new DeviceSettingsFPD(); | |
| } | |
| _apiLock.Unlock(); | |
| // Fast path: if already initialized, avoid any locking. | |
| if (_instance != nullptr) { | |
| return; | |
| } | |
| // Construct outside the lock to avoid potential deadlock if the | |
| // constructor calls methods that take _apiLock (e.g., Connect()). | |
| DeviceSettingsFPD* newInstance = new DeviceSettingsFPD(); | |
| _apiLock.Lock(); | |
| // Another thread may have initialized _instance while we were | |
| // constructing newInstance; only assign if it's still null. | |
| if (_instance == nullptr) { | |
| _instance = newInstance; | |
| newInstance = nullptr; | |
| } | |
| _apiLock.Unlock(); | |
| // If we lost the race, clean up the extra instance. | |
| if (newInstance != nullptr) { | |
| delete newInstance; | |
| } |
device::Manager::Initialize(); and init FPD only
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| dsapp: | ||
| @if [ -f DSCliLibStandaloneApp.cpp ]; then \ | ||
| echo "Building DSApp..."; \ | ||
| $(CXX) $(CFLAGS) -std=c++11 DSCliLibStandaloneApp.cpp -o ../DSApp -L$(INSTALL)/lib -lds -ldshalcli -lcurl -lpthread $(LDFLAGS); \ | ||
| echo "DSApp built successfully at devicesettings/DSApp"; \ | ||
| else \ |
There was a problem hiding this comment.
dsapp builds the output as ../DSApp, which writes outside the sample/ directory. This is fragile for out-of-tree builds and conflicts with the top-level Makefile.am messaging that suggests $(abs_top_builddir)/DSApp. Prefer writing the artifact into the build dir (e.g., $(top_builddir)/$(abs_top_builddir)) and using that same path in install/copy logic.
| // Signal handler for graceful shutdown | ||
| static void signal_handler(int signum) { | ||
| const char* signal_name = "UNKNOWN"; | ||
| switch (signum) { | ||
| case SIGTERM: signal_name = "SIGTERM"; break; | ||
| case SIGINT: signal_name = "SIGINT"; break; | ||
| case SIGHUP: signal_name = "SIGHUP"; break; | ||
| case SIGUSR1: signal_name = "SIGUSR1"; break; | ||
| case SIGUSR2: signal_name = "SIGUSR2"; break; | ||
| } | ||
|
|
||
| syslog(LOG_INFO, "[DSApp] Received signal %s (%d), initiating graceful shutdown", signal_name, signum); | ||
|
|
||
| // Mark application as not initialized to trigger cleanup | ||
| pthread_mutex_lock(&gDSAppMutex); | ||
| gAppInitialized = false; | ||
| pthread_mutex_unlock(&gDSAppMutex); | ||
|
|
||
| // Cleanup and exit | ||
| cleanup_resources(); | ||
| closelog(); | ||
| exit(0); | ||
| } |
There was a problem hiding this comment.
signal_handler calls non-async-signal-safe functions (syslog, pthread_mutex_lock/unlock, cleanup_resources, closelog, exit). This is undefined behavior and can deadlock/crash if the signal interrupts those same functions. Prefer setting a volatile sig_atomic_t shutdown flag (or writing to a self-pipe) and handling cleanup/exit in the main loop/context instead of inside the handler.
| # Conditional compilation: Thunder vs IARM | ||
| 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 |
There was a problem hiding this comment.
The Makefile ifdef USE_WPE_THUNDER_PLUGIN is used to pick Thunder objects, but it does not add -DUSE_WPE_THUNDER_PLUGIN to the compiler flags. Because dsFPD-com.cpp is wrapped in #ifdef USE_WPE_THUNDER_PLUGIN, compiling it without the macro will omit all implementations and cause link errors. Ensure the macro is passed via CFLAGS/CPPFLAGS in this branch (or remove the source-level guard).
| #include <unistd.h> | ||
| #include <pthread.h> | ||
| #include <sched.h> | ||
| #include <cstdlib> | ||
| #include <cstring> | ||
| #include <signal.h> | ||
| #include <curl/curl.h> | ||
| #include <iostream> | ||
| #include <cstdint> | ||
| #include <sys/time.h> | ||
| #include <errno.h> | ||
| #include <syslog.h> | ||
| #include <sys/resource.h> | ||
|
|
There was a problem hiding this comment.
This file uses printf/scanf extensively and std::nothrow, but it doesn’t include headers that guarantee those declarations in C++ (<cstdio>/<stdio.h> for printf/scanf, <new> for std::nothrow). This can fail to compile on stricter toolchains. Add the required standard headers explicitly.
| dsapp: | ||
| @echo "Building DSApp..." | ||
| $(MAKE) -C sample dsapp | ||
| @echo "DSApp executable: $(abs_top_builddir)/DSApp" |
There was a problem hiding this comment.
This target prints DSApp executable: $(abs_top_builddir)/DSApp, but the underlying sample/Makefile currently builds DSApp to ../DSApp relative to sample/, which won’t match $(abs_top_builddir) in out-of-tree builds. Align the build output path and the reported path (ideally output into $(abs_top_builddir)/$(top_builddir)).
| @echo "DSApp executable: $(abs_top_builddir)/DSApp" | |
| @echo "DSApp executable: $(top_builddir)/DSApp" |
interface changes in Ids.h and IDeviceSettings.h file
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
sample/Makefile:82
installcopies../DSAppinto$(INSTALL)/bin, butuninstallonly removes$(UNINSTALL)(the sample binaries) and leavesDSAppbehind. Add removal of$(INSTALL)/bin/DSAppfor symmetry souninstallfully revertsinstall.
uninstall:
@echo "Removing bin from install folder..."
@$(RM) $(UNINSTALL)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| void cleanup_resources() { | ||
| LOGI("Cleaning up application resources..."); | ||
|
|
||
| // Mark application as shutting down | ||
| pthread_mutex_lock(&gDSAppMutex); | ||
| gAppInitialized = false; | ||
| pthread_mutex_unlock(&gDSAppMutex); | ||
|
|
||
| // Clean up curl resources | ||
| if (mCurlShared) { | ||
| curl_share_cleanup(mCurlShared); | ||
| mCurlShared = nullptr; | ||
| LOGD("CURL shared handle cleaned up"); | ||
| } | ||
|
|
||
| curl_global_cleanup(); | ||
| LOGD("CURL global cleanup completed"); | ||
|
|
||
| // Free debug buffer | ||
| if (gDebugPrintBuffer) { | ||
| delete[] gDebugPrintBuffer; | ||
| gDebugPrintBuffer = nullptr; | ||
| LOGD("Debug buffer freed"); | ||
| } | ||
|
|
||
| // Clean up RDK-specific resources | ||
| LOGD("Terminating libds.so Device Settings..."); | ||
| libds_Terminate(); // Call libds.so cleanup | ||
|
|
||
| // TODO: Add IARM Bus cleanup if used | ||
| // IARM_Bus_Disconnect(); | ||
| // IARM_Bus_Term(); | ||
|
|
||
| // Destroy mutexes | ||
| pthread_mutex_destroy(&gDSAppMutex); | ||
| pthread_mutex_destroy(&gCurlInitMutex); | ||
| LOGD("Mutexes destroyed"); |
There was a problem hiding this comment.
cleanup_resources() destroys global mutexes (pthread_mutex_destroy) even though it can be invoked multiple times (normal exit path and also from the signal handler). Double-destroy is undefined behavior; also destroying a mutex while another thread might still use it is unsafe. Make cleanup idempotent (guard with a once-flag) and avoid destroying process-lifetime mutexes (or only destroy them at a single well-defined point).
| export STANDALONE_BUILD_ENABLED=y | ||
| export DS_MGRS=$WORKDIR | ||
|
|
||
| export USE_WPE_THUNDER_PLUGIN=y |
There was a problem hiding this comment.
This script enables USE_WPE_THUNDER_PLUGIN, which changes rpc/cli/Makefile to link against -lWPEFrameworkCore -lWPEFrameworkCOM, but the script doesn’t build/install stubs for those libraries (it only stubs libWPEFrameworkPowerController.so). Unless those libs are guaranteed to exist in the build environment, this will break the Coverity build. Either keep Thunder disabled here by default or add the required dependency setup for the Thunder libraries.
| export USE_WPE_THUNDER_PLUGIN=y | |
| # Thunder/WPEFramework plugin is disabled by default for Coverity builds to | |
| # avoid introducing unresolved link-time dependencies on WPEFrameworkCore/COM. | |
| # It can still be enabled explicitly by exporting USE_WPE_THUNDER_PLUGIN=y | |
| # before invoking this script, in environments where the libraries are present. | |
| export USE_WPE_THUNDER_PLUGIN=${USE_WPE_THUNDER_PLUGIN-n} |
| @if [ -f DSCliLibStandaloneApp.cpp ]; then \ | ||
| echo "Building DSApp..."; \ | ||
| $(CXX) $(CFLAGS) -std=c++11 DSCliLibStandaloneApp.cpp -o ../DSApp -L$(INSTALL)/lib -lds -ldshalcli -lcurl -lpthread $(LDFLAGS); \ | ||
| echo "DSApp built successfully at devicesettings/DSApp"; \ |
There was a problem hiding this comment.
The success message says DSApp is built at devicesettings/DSApp, but the command outputs to ../DSApp. This can confuse users trying to locate the binary; update the message (or change the output path) so they match.
| echo "DSApp built successfully at devicesettings/DSApp"; \ | |
| echo "DSApp built successfully at ../DSApp"; \ |
rpc/cli/dsFPD-com.cpp
Outdated
| /* // Wait for plugin to become operational | ||
| if (!instance->WaitForOperational(5000)) { | ||
| fprintf(stderr, "[dsFPD-com] DeviceSettings plugin not operational after 5 seconds\n"); | ||
| return dsERR_GENERAL; | ||
| } | ||
| */ |
There was a problem hiding this comment.
This new Thunder COM-RPC implementation introduces a large alternate code path for the FPD client, but the existing test suite doesn’t appear to exercise Thunder mode (no tests reference USE_WPE_THUNDER_PLUGIN). Consider adding at least a minimal test (even if guarded/optional) that validates the Thunder client wiring and basic error mapping, so regressions in the COM-RPC path are caught.
| /* // Wait for plugin to become operational | |
| if (!instance->WaitForOperational(5000)) { | |
| fprintf(stderr, "[dsFPD-com] DeviceSettings plugin not operational after 5 seconds\n"); | |
| return dsERR_GENERAL; | |
| } | |
| */ | |
| // Wait for plugin to become operational | |
| if (!instance->WaitForOperational(5000)) { | |
| fprintf(stderr, "[dsFPD-com] DeviceSettings plugin not operational after 5 seconds\n"); | |
| return dsERR_GENERAL; | |
| } | |
rpc/cli/dsFPD-com.cpp
Outdated
| // 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); |
There was a problem hiding this comment.
dsSetFPTime() receives uHour and uMinutes, but forwards them to SetFPDTime(timeFormat, minutes, seconds) as (uHour, uMinutes). This effectively treats hours as minutes and minutes as seconds, producing an incorrect time value. Convert the inputs properly (e.g., minutes = uHour * 60 + uMinutes; seconds = 0) or adjust the mapping to match the Thunder interface semantics.
| // 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); | |
| // Interface expects minutes and seconds, but API provides hours and minutes. | |
| // Convert to total minutes and set seconds to 0 for Thunder interface. | |
| uint32_t totalMinutes = (uHour * 60U) + uMinutes; | |
| uint32_t seconds = 0U; | |
| uint32_t result = instance->SetFPDTime(timeFormat, totalMinutes, seconds); |
| LDLIBS := -lWPEFrameworkCore -lWPEFrameworkCOM | ||
| else |
There was a problem hiding this comment.
In Thunder mode, LDLIBS drops -lIARMBus, but most other rpc/cli sources (e.g., dsAudio.c, dsDisplay.c, dsHost.cpp, dsVideo*.c) still call IARM_Bus_Call and require IARMBus symbols. This will either fail to link or produce a lib with unresolved dependencies. Keep -lIARMBus in Thunder mode as well (and add Thunder libs on top), or conditionally exclude/replace the other IARM-based sources too.
| LDLIBS := -lWPEFrameworkCore -lWPEFrameworkCOM | |
| else | |
| # Thunder mode: still need IARMBus symbols plus Thunder libraries | |
| LDLIBS := -lIARMBus -lWPEFrameworkCore -lWPEFrameworkCOM | |
| else | |
| # IARM-only mode |
| // Initialize libds.so Device Settings library | ||
| void libds_Initialize() | ||
| { | ||
| try { | ||
| LOGI("Initializing Device Settings Manager from libds.so..."); | ||
|
|
||
| // Initialize the Device Settings Manager | ||
| // This will internally call dsFPInit() and other init functions from libdshalcli.so | ||
| // device::Manager::Initialize(); As of now FPD only communicate through COM-RPC | ||
|
|
||
| LOGI("Initializing Front Panel from libds.so..."); | ||
| LOGI("Front panel init"); | ||
| fpIndicators = device::FrontPanelConfig::getInstance().getIndicators(); | ||
|
|
||
| for (size_t i = 0; i < fpIndicators.size(); i++) { | ||
| std::string IndicatorName = fpIndicators.at(i).getName(); | ||
| LOGI("Initializing Front Panel Indicator: %s", IndicatorName.c_str()); | ||
| } | ||
|
|
||
| LOGI("Device Settings FPD initialized successfully"); | ||
|
|
||
| LOGI("Device Settings Manager initialized successfully"); | ||
| } catch (const std::exception& e) { | ||
| LOGE("Failed to initialize Device Settings Manager: %s", e.what()); | ||
| throw; | ||
| } | ||
| } | ||
|
|
||
| // Terminate libds.so Device Settings library | ||
| void libds_Terminate() | ||
| { | ||
| try { | ||
| LOGI("Terminating Device Settings Manager from libds.so..."); | ||
| device::Manager::DeInitialize(); | ||
| LOGI("Device Settings Manager terminated successfully"); | ||
| } catch (const std::exception& e) { | ||
| LOGE("Error during Device Settings Manager termination: %s", e.what()); | ||
| } | ||
| } |
There was a problem hiding this comment.
libds_Initialize() does not call device::Manager::Initialize() (it’s commented out), but libds_Terminate() unconditionally calls device::Manager::DeInitialize(). This init/term mismatch can lead to undefined behavior depending on libds implementation. Either initialize the manager here or make terminate conditional on successful initialize.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rpc/cli/dsFPD-com.cpp
Outdated
| virtual void Operational(const bool upAndRunning) override | ||
| { | ||
| _apiLock.Lock(); | ||
|
|
||
| if (upAndRunning) { | ||
| // Communicator opened && DeviceSettings is Activated | ||
| if (nullptr == _deviceSettings) { | ||
| _deviceSettings = BaseClass::Interface(); | ||
| if (_deviceSettings != nullptr) { | ||
| printf("[dsFPD-com] Successfully obtained IDeviceSettings interface [%p] \n", _deviceSettings); | ||
|
|
||
| // Now QueryInterface to get IDeviceSettingsFPD from the aggregated implementation | ||
| _fpdInterface = _deviceSettings->QueryInterface<Exchange::IDeviceSettingsFPD>(); | ||
| if (_fpdInterface != nullptr) { | ||
| printf("[dsFPD-com] Successfully established COM-RPC connection with DeviceSettings FPD interface [%p]\n", _fpdInterface); | ||
| } else { | ||
| fprintf(stderr, "[dsFPD-com] Failed to get IDeviceSettingsFPD interface - plugin implementation may have failed to load\n"); | ||
| } | ||
| } else { | ||
| fprintf(stderr, "[dsFPD-com] Failed to get IDeviceSettings interface - plugin may not be activated\n"); | ||
| } | ||
| } | ||
| } else { | ||
| // DeviceSettings is Deactivated || Communicator closed | ||
| if (nullptr != _fpdInterface) { | ||
| _fpdInterface->Release(); | ||
| _fpdInterface = nullptr; | ||
| } | ||
| if (nullptr != _deviceSettings) { | ||
| _deviceSettings->Release(); | ||
| _deviceSettings = nullptr; | ||
| } | ||
| } | ||
| _apiLock.Unlock(); | ||
| } |
There was a problem hiding this comment.
The Operational callback does not check the _shutdown flag before performing operations. If the destructor is called while Operational is executing or about to execute, there could be a race condition where resources are being destroyed while still in use. Consider checking _shutdown at the beginning of Operational and returning early if true.
rpc/cli/dsFPD-com.cpp
Outdated
| printf("[dsFPD-com] Returning DeviceSettingsFPD instance [%p]\n", _instance); | ||
| return _instance; |
There was a problem hiding this comment.
The Instance() method does not acquire the lock before accessing _instance, creating a race condition. If one thread is in Init() or Term() modifying _instance while another thread calls Instance(), the returned pointer could be invalid or null when it's about to be deleted. Instance() should acquire _apiLock before reading _instance to ensure thread safety.
| printf("[dsFPD-com] Returning DeviceSettingsFPD instance [%p]\n", _instance); | |
| return _instance; | |
| _apiLock.Lock(); | |
| DeviceSettingsFPD* instance = _instance; | |
| printf("[dsFPD-com] Returning DeviceSettingsFPD instance [%p]\n", instance); | |
| _apiLock.Unlock(); | |
| return instance; |
| # Conditional linking flags | ||
| ifdef USE_WPE_THUNDER_PLUGIN | ||
| LDLIBS := -lWPEFrameworkCore -lWPEFrameworkCOM | ||
| else | ||
| LDLIBS := -lIARMBus | ||
| endif |
There was a problem hiding this comment.
The Makefile conditionally compiles based on USE_WPE_THUNDER_PLUGIN, but this macro is not added to CFLAGS. The source file dsFPD-com.cpp uses #ifdef USE_WPE_THUNDER_PLUGIN (line 27 in that file) to conditionally compile the Thunder implementation. Without -DUSE_WPE_THUNDER_PLUGIN in CFLAGS, the code in dsFPD-com.cpp will not be compiled even when it's included in the build. Add -DUSE_WPE_THUNDER_PLUGIN to CFLAGS when Thunder mode is enabled.
| const char* signal_name = "UNKNOWN"; | ||
| switch (signum) { | ||
| case SIGTERM: signal_name = "SIGTERM"; break; | ||
| case SIGINT: signal_name = "SIGINT"; break; | ||
| case SIGHUP: signal_name = "SIGHUP"; break; | ||
| case SIGUSR1: signal_name = "SIGUSR1"; break; | ||
| case SIGUSR2: signal_name = "SIGUSR2"; break; | ||
| } | ||
|
|
||
| syslog(LOG_INFO, "[DSApp] Received signal %s (%d), initiating graceful shutdown", signal_name, signum); | ||
|
|
||
| // Mark application as not initialized to trigger cleanup | ||
| pthread_mutex_lock(&gDSAppMutex); | ||
| gAppInitialized = false; | ||
| pthread_mutex_unlock(&gDSAppMutex); | ||
|
|
||
| // Cleanup and exit | ||
| cleanup_resources(); | ||
| closelog(); | ||
| exit(0); |
There was a problem hiding this comment.
The signal handler performs several operations that are not async-signal-safe. According to POSIX, signal handlers should only call async-signal-safe functions. This handler calls:
- syslog() (line 200) - not async-signal-safe
- pthread_mutex_lock() (line 203) - not async-signal-safe
- cleanup_resources() (line 208) - which calls multiple non-async-signal-safe functions
- exit() (line 210) - not fully safe in signal handlers
These operations can lead to deadlocks, undefined behavior, or data corruption. Consider using a self-pipe trick or setting a volatile sig_atomic_t flag that the main loop checks, rather than performing complex operations directly in the signal handler.
| const char* signal_name = "UNKNOWN"; | |
| switch (signum) { | |
| case SIGTERM: signal_name = "SIGTERM"; break; | |
| case SIGINT: signal_name = "SIGINT"; break; | |
| case SIGHUP: signal_name = "SIGHUP"; break; | |
| case SIGUSR1: signal_name = "SIGUSR1"; break; | |
| case SIGUSR2: signal_name = "SIGUSR2"; break; | |
| } | |
| syslog(LOG_INFO, "[DSApp] Received signal %s (%d), initiating graceful shutdown", signal_name, signum); | |
| // Mark application as not initialized to trigger cleanup | |
| pthread_mutex_lock(&gDSAppMutex); | |
| gAppInitialized = false; | |
| pthread_mutex_unlock(&gDSAppMutex); | |
| // Cleanup and exit | |
| cleanup_resources(); | |
| closelog(); | |
| exit(0); | |
| switch (signum) { | |
| case SIGTERM: | |
| /* Async-signal-safe diagnostic message */ | |
| write(STDERR_FILENO, | |
| "[DSApp] Received SIGTERM, exiting immediately\n", | |
| sizeof("[DSApp] Received SIGTERM, exiting immediately\n") - 1); | |
| break; | |
| case SIGINT: | |
| write(STDERR_FILENO, | |
| "[DSApp] Received SIGINT, exiting immediately\n", | |
| sizeof("[DSApp] Received SIGINT, exiting immediately\n") - 1); | |
| break; | |
| case SIGHUP: | |
| write(STDERR_FILENO, | |
| "[DSApp] Received SIGHUP, exiting immediately\n", | |
| sizeof("[DSApp] Received SIGHUP, exiting immediately\n") - 1); | |
| break; | |
| case SIGUSR1: | |
| write(STDERR_FILENO, | |
| "[DSApp] Received SIGUSR1, exiting immediately\n", | |
| sizeof("[DSApp] Received SIGUSR1, exiting immediately\n") - 1); | |
| break; | |
| case SIGUSR2: | |
| write(STDERR_FILENO, | |
| "[DSApp] Received SIGUSR2, exiting immediately\n", | |
| sizeof("[DSApp] Received SIGUSR2, exiting immediately\n") - 1); | |
| break; | |
| default: | |
| write(STDERR_FILENO, | |
| "[DSApp] Received unknown signal, exiting immediately\n", | |
| sizeof("[DSApp] Received unknown signal, exiting immediately\n") - 1); | |
| break; | |
| } | |
| /* Use _exit, which is async-signal-safe, to terminate immediately. */ | |
| _exit(128 + signum); |
rpc/cli/dsFPD-com.cpp
Outdated
| // 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); |
There was a problem hiding this comment.
There's a semantic mismatch in parameter interpretation. The dsSetFPTime API expects (timeFormat, hour, minutes) but the SetFPDTime interface expects (timeFormat, minutes, seconds). The comment on line 512 acknowledges this but the "fix" of treating uMinutes as seconds is incorrect - it's passing uHour as minutes and uMinutes as seconds. This means if someone calls dsSetFPTime with hour=14 and minutes=30, it will set the display to 14 minutes and 30 seconds instead of 14:30. Either the Thunder interface signature is wrong or this conversion logic needs to properly convert hours+minutes to total minutes+seconds, or this mismatch should be documented as a known limitation.
| // 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 hour:minutes into totalMinutes:seconds for the Thunder interface. | |
| const uint32_t totalMinutes = (uHour * 60u) + (uMinutes % 60u); | |
| const uint32_t seconds = 0u; | |
| uint32_t result = instance->SetFPDTime(timeFormat, totalMinutes, seconds); |
|
|
||
| LOGI("Initializing Front Panel from libds.so..."); |
There was a problem hiding this comment.
Inconsistent indentation detected. Lines 158-159 use tabs for indentation while the surrounding code uses spaces. This should be consistent with the rest of the file which uses spaces for indentation.
| LOGI("Initializing Front Panel from libds.so..."); | |
| LOGI("Initializing Front Panel from libds.so..."); |
|
|
||
| // Destroy mutexes | ||
| pthread_mutex_destroy(&gDSAppMutex); | ||
| pthread_mutex_destroy(&gCurlInitMutex); | ||
| LOGD("Mutexes destroyed"); | ||
|
|
There was a problem hiding this comment.
Destroying mutexes that may be in use is unsafe. The cleanup_resources function is called from the signal handler (line 208), but it destroys mutexes on lines 326-327 after potentially having locked them. If another thread is holding or waiting on these mutexes when the signal handler fires, destroying them leads to undefined behavior. Signal handlers should be async-signal-safe and avoid such operations. Consider setting a flag instead and handling cleanup in the main thread, or ensure the application is single-threaded when this cleanup occurs.
| // Destroy mutexes | |
| pthread_mutex_destroy(&gDSAppMutex); | |
| pthread_mutex_destroy(&gCurlInitMutex); | |
| LOGD("Mutexes destroyed"); | |
| // Note: Do not destroy mutexes here. This function may be invoked from | |
| // a signal handler, and destroying mutexes that might be in use by | |
| // other threads is undefined behavior. These global mutexes are | |
| // process-lifetime objects and will be cleaned up by the OS at exit. |
| try { | ||
| printf("Calling libds.so FrontPanelIndicator::setBrightness(%d)...\n", brightness); | ||
|
|
||
| // Get the Power LED indicator from libds.so | ||
| FrontPanelIndicator& powerLED = FrontPanelIndicator::getInstance(FrontPanelIndicator::kPower); | ||
|
|
||
| // Set brightness - this will call dsSetFPDBrightness from libdshalcli.so | ||
| powerLED.setBrightness(brightness, true); | ||
|
|
||
| printf("SUCCESS: Power LED brightness set to %d\n", brightness); | ||
| } catch (const std::exception& e) { | ||
| printf("ERROR: %s\n", e.what()); |
There was a problem hiding this comment.
Input validation is missing for the brightness value. While the range is mentioned in the prompt (0-100), there is no validation to ensure the input is within this range before calling setBrightness. Invalid values could cause unexpected behavior in the device settings layer. Add validation: if (brightness < 0 || brightness > 100) before the try block.
| try { | |
| printf("Calling libds.so FrontPanelIndicator::setBrightness(%d)...\n", brightness); | |
| // Get the Power LED indicator from libds.so | |
| FrontPanelIndicator& powerLED = FrontPanelIndicator::getInstance(FrontPanelIndicator::kPower); | |
| // Set brightness - this will call dsSetFPDBrightness from libdshalcli.so | |
| powerLED.setBrightness(brightness, true); | |
| printf("SUCCESS: Power LED brightness set to %d\n", brightness); | |
| } catch (const std::exception& e) { | |
| printf("ERROR: %s\n", e.what()); | |
| if (brightness < 0 || brightness > 100) { | |
| printf("Brightness must be between 0 and 100.\n"); | |
| } else { | |
| try { | |
| printf("Calling libds.so FrontPanelIndicator::setBrightness(%d)...\n", brightness); | |
| // Get the Power LED indicator from libds.so | |
| FrontPanelIndicator& powerLED = FrontPanelIndicator::getInstance(FrontPanelIndicator::kPower); | |
| // Set brightness - this will call dsSetFPDBrightness from libdshalcli.so | |
| powerLED.setBrightness(brightness, true); | |
| printf("SUCCESS: Power LED brightness set to %d\n", brightness); | |
| } catch (const std::exception& e) { | |
| printf("ERROR: %s\n", e.what()); | |
| } |
rpc/cli/dsFPD-com.cpp
Outdated
| /* // Wait for plugin to become operational | ||
| if (!instance->WaitForOperational(5000)) { | ||
| fprintf(stderr, "[dsFPD-com] DeviceSettings plugin not operational after 5 seconds\n"); | ||
| return dsERR_GENERAL; | ||
| } | ||
| */ |
There was a problem hiding this comment.
The commented-out code from lines 465-469 uses incorrect C-style block comment delimiters. The opening /* is on line 465, but the closing */ is on line 470 after actual code. This creates a malformed comment structure. If this code were uncommented or if the comment boundaries were adjusted, it would cause compilation issues. Either remove this commented code entirely or fix the comment delimiters to be on their own lines.
| /* // Wait for plugin to become operational | |
| if (!instance->WaitForOperational(5000)) { | |
| fprintf(stderr, "[dsFPD-com] DeviceSettings plugin not operational after 5 seconds\n"); | |
| return dsERR_GENERAL; | |
| } | |
| */ | |
| // Wait for plugin to become operational | |
| // if (!instance->WaitForOperational(5000)) { | |
| // fprintf(stderr, "[dsFPD-com] DeviceSettings plugin not operational after 5 seconds\n"); | |
| // return dsERR_GENERAL; | |
| // } | |
rpc/cli/Makefile.am
Outdated
| FPD_SOURCE = dsFPD-com.cpp | ||
| THUNDER_LIBS = -lWPEFrameworkCore -lWPEFrameworkCOM | ||
| else | ||
| FPD_SOURCE = dsFPD.c | ||
| THUNDER_LIBS = |
There was a problem hiding this comment.
The indentation of the conditional blocks in Makefile.am is incorrect. In Automake, the if/else/endif statements should not be indented, and the content within them should also not be indented beyond normal Makefile variable assignments. The spaces before FPD_SOURCE and THUNDER_LIBS will cause Automake parsing errors. Remove the leading spaces from lines 34-35 and 37-38.
| FPD_SOURCE = dsFPD-com.cpp | |
| THUNDER_LIBS = -lWPEFrameworkCore -lWPEFrameworkCOM | |
| else | |
| FPD_SOURCE = dsFPD.c | |
| THUNDER_LIBS = | |
| FPD_SOURCE = dsFPD-com.cpp | |
| THUNDER_LIBS = -lWPEFrameworkCore -lWPEFrameworkCOM | |
| else | |
| FPD_SOURCE = dsFPD.c | |
| THUNDER_LIBS = |
Modified the dsFPD-com.cpp file based on dsController-com.cpp and Makefile.am
ds/include/dsController-com.h file
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 28 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Exchange::IDeviceSettingsDisplay* result = _displayInterface; | ||
| _lock.Unlock(); | ||
| return result; | ||
| */} |
There was a problem hiding this comment.
Missing return statement. This function is declared to return Exchange::IDeviceSettingsDisplay* but has no return statement, which will cause undefined behavior and compilation warnings/errors.
| */} | |
| */ | |
| return nullptr; | |
| } |
| Exchange::IDeviceSettingsVideoDevice* DeviceSettingsController::GetVideoDeviceInterface() | ||
| { | ||
| /* | ||
| _lock.Lock(); | ||
| QueryComponentInterface<Exchange::IDeviceSettingsVideoDevice>(_videoDeviceInterface, "IDeviceSettingsVideoDevice"); | ||
| Exchange::IDeviceSettingsVideoDevice* result = _videoDeviceInterface; | ||
| _lock.Unlock(); | ||
| return result; | ||
| */ | ||
| } |
There was a problem hiding this comment.
Missing return statement. This function is declared to return Exchange::IDeviceSettingsVideoDevice* but has no return statement, which will cause undefined behavior and compilation warnings/errors.
| printf("Press Enter to continue..."); | ||
| getchar(); getchar(); | ||
| break; |
There was a problem hiding this comment.
Potential double getchar() issue for input handling. The code uses getchar(); getchar(); pattern multiple times (e.g., lines 431-432) to wait for user input, presumably to consume the newline and then wait for Enter. However, this pattern is fragile - if the input buffer already has a newline from a previous scanf, the first getchar() consumes it and the second one blocks, which is the intended behavior. But if there's no newline (e.g., after a failed scanf), this can cause the program to skip waiting. A more robust approach would be to clear the input buffer first with a while loop, then wait for a single Enter press.
| Exchange::IDeviceSettingsAudio* DeviceSettingsController::GetAudioInterface() | ||
| { | ||
| /* _lock.Lock(); | ||
| QueryComponentInterface<Exchange::IDeviceSettingsAudio>(_audioInterface, "IDeviceSettingsAudio"); | ||
| Exchange::IDeviceSettingsAudio* result = _audioInterface; | ||
| _lock.Unlock(); | ||
| return result; | ||
| */ | ||
| } |
There was a problem hiding this comment.
Missing return statement. This function is declared to return Exchange::IDeviceSettingsAudio* but has no return statement, which will cause undefined behavior and compilation warnings/errors.
| Exchange::IDeviceSettingsHost* DeviceSettingsController::GetHostInterface() | ||
| { | ||
| /* | ||
| _lock.Lock(); | ||
| QueryComponentInterface<Exchange::IDeviceSettingsHost>(_hostInterface, "IDeviceSettingsHost"); | ||
| Exchange::IDeviceSettingsHost* result = _hostInterface; | ||
| _lock.Unlock(); | ||
| return result; | ||
| */ | ||
| } |
There was a problem hiding this comment.
Missing return statement. This function is declared to return Exchange::IDeviceSettingsHost* but has no return statement, which will cause undefined behavior and compilation warnings/errors.
| printf("\nCalling DeviceSettings_GetCompositeInput()...\n"); | ||
| // TODO: Add actual function call | ||
| printf("Function called successfully!\n"); |
There was a problem hiding this comment.
Multiple TODO comments for unimplemented functionality. Lines 641, 648, 688, 724, 759 contain TODO comments with placeholder function calls. These menu options are presented to the user but don't actually perform the expected operations. Either implement these functions, remove the menu options, or clearly indicate in the menu that these features are not yet implemented (e.g., "2. Audio Ports (Not yet implemented)").
| printf("\nCalling DeviceSettings_GetCompositeInput()...\n"); | |
| // TODO: Add actual function call | |
| printf("Function called successfully!\n"); | |
| printf("\nDeviceSettings_GetCompositeInput is not yet implemented.\n"); |
ds/manager.cpp
Outdated
| #ifdef USE_THUNDER_PLUGIN | ||
| // For Thunder COM-RPC mode, initialize the DeviceSettingsController | ||
| if (WPEFramework::DeviceSettingsController::Initialize() != WPEFramework::Core::ERROR_NONE) { | ||
| fprintf(stderr, "[Manager] Failed to initialize DeviceSettingsController\n"); | ||
| throw std::runtime_error("DeviceSettingsController initialization failed"); | ||
| } | ||
| printf("[Manager] DeviceSettingsController initialized successfully\n"); | ||
| #endif |
There was a problem hiding this comment.
Inconsistent commenting style. Line 163 begins with tabs while line 164 has proper indentation. This creates visual inconsistency. Use consistent indentation for all lines including comments.
| #ifdef USE_THUNDER_PLUGIN | |
| // For Thunder COM-RPC mode, initialize the DeviceSettingsController | |
| if (WPEFramework::DeviceSettingsController::Initialize() != WPEFramework::Core::ERROR_NONE) { | |
| fprintf(stderr, "[Manager] Failed to initialize DeviceSettingsController\n"); | |
| throw std::runtime_error("DeviceSettingsController initialization failed"); | |
| } | |
| printf("[Manager] DeviceSettingsController initialized successfully\n"); | |
| #endif | |
| #ifdef USE_THUNDER_PLUGIN | |
| // For Thunder COM-RPC mode, initialize the DeviceSettingsController | |
| if (WPEFramework::DeviceSettingsController::Initialize() != WPEFramework::Core::ERROR_NONE) { | |
| fprintf(stderr, "[Manager] Failed to initialize DeviceSettingsController\n"); | |
| throw std::runtime_error("DeviceSettingsController initialization failed"); | |
| } | |
| printf("[Manager] DeviceSettingsController initialized successfully\n"); | |
| #endif |
rpc/cli/dsController-com.h
Outdated
| #ifdef USE_THUNDER_PLUGIN | ||
|
|
||
| #ifndef MODULE_NAME | ||
| #define MODULE_NAME DeviceSettings_Client | ||
| #endif | ||
|
|
||
| #include <core/core.h> | ||
| #include <com/com.h> | ||
| #include <plugins/Types.h> | ||
| #include <interfaces/IDeviceSettings.h> | ||
| #include <interfaces/IDeviceSettingsFPD.h> | ||
| #include <interfaces/IDeviceSettingsHDMIIn.h> | ||
| #include <interfaces/IDeviceSettingsAudio.h> | ||
| #include <interfaces/IDeviceSettingsDisplay.h> | ||
| #include <interfaces/IDeviceSettingsVideoPort.h> | ||
| #include <interfaces/IDeviceSettingsVideoDevice.h> | ||
| #include <interfaces/IDeviceSettingsHost.h> | ||
| #include <interfaces/IDeviceSettingsCompositeIn.h> |
There was a problem hiding this comment.
Missing include guard or #error for required Thunder headers. The file uses Thunder-specific types and classes (Exchange::IDeviceSettingsFPD, WPEFramework::Core::CriticalSection, etc.) but doesn't verify that the necessary Thunder header files are available. If someone tries to compile this with USE_THUNDER_PLUGIN defined but without the Thunder SDK installed, they'll get confusing compiler errors. Consider adding a comment or compile-time check to ensure Thunder headers are available when USE_THUNDER_PLUGIN is defined.
| // Just verify we have FPD interface access | ||
| DeviceSettingsController* controller = DeviceSettingsController::GetInstance(); | ||
| if (!controller) { | ||
| fprintf(stderr, "[dsFPD-com] DeviceSettingsController not initialized\n"); |
There was a problem hiding this comment.
No error handling for fprintf to stderr. Throughout the file, fprintf to stderr is used but the return value is not checked. While uncommon, stderr writes can fail (e.g., if stderr is redirected to a full disk). For critical error messages, consider checking the return value or using syslog as an alternative reliable logging mechanism, especially since this code already uses syslog elsewhere.
| fpIndicators = device::FrontPanelConfig::getInstance().getIndicators(); | ||
|
|
||
| for (size_t i = 0; i < fpIndicators.size(); i++) { | ||
| std::string IndicatorName = fpIndicators.at(i).getName(); | ||
| LOGI("Initializing Front Panel Indicator: %s", IndicatorName.c_str()); | ||
| } |
There was a problem hiding this comment.
Missing error check for FrontPanelConfig initialization. Line 161 calls FrontPanelConfig::getInstance().getIndicators() but doesn't wrap it in a try-catch block or check for errors, even though it's called before libds initialization is confirmed complete. If this call fails, it could throw an exception that isn't caught at line 171-174, or it could return an invalid/empty list. Ensure proper error handling for this initialization step.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 13 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| printf("\n=== DeviceSettings Interactive Test Application ===\n"); | ||
| printf("Build: %s %s\n", __DATE__, __TIME__); |
There was a problem hiding this comment.
Hardcoded build timestamp in production: Lines 73-74 embed DATE and TIME macros which create non-reproducible builds. This makes it impossible to verify that a binary was built from specific source code, which is important for security and debugging. Consider using a version string from the build system instead.
| return ConvertThunderError(result); | ||
| } | ||
|
|
||
| dsError_t dsFPEnableCLockDisplay(int enable) |
There was a problem hiding this comment.
Typo in function name: The function is named dsFPEnableCLockDisplay (line 481) with uppercase 'L' in "CLock", but it should likely be dsFPEnableClockDisplay with lowercase 'l' in "Clock". This inconsistent naming could cause confusion.
| dsError_t dsFPEnableCLockDisplay(int enable) | |
| dsError_t dsFPEnableClockDisplay(int enable) |
| while (1) | ||
| { | ||
| _connectionRetryCount++; | ||
|
|
||
| uint32_t result = controller->Connect(); | ||
|
|
||
| if (result == Core::ERROR_NONE) { | ||
| printf("[DeviceSettingsController] Thunder connection SUCCESS after %u attempts\n", | ||
| _connectionRetryCount); | ||
|
|
||
| // Post-connection initialization | ||
| FetchAndInitializeInterfaces(); | ||
|
|
||
| _isConnecting = false; | ||
| printf("[DeviceSettingsController] RetryThunderConnectionThread completed successfully\n"); | ||
| break; | ||
| } | ||
| else { | ||
| if (_connectionRetryCount % 10 == 0) { // Log every 3 seconds | ||
| printf("[DeviceSettingsController] Connection attempt %u failed, retrying...\n", | ||
| _connectionRetryCount); | ||
| } | ||
|
|
||
| // 300ms wait before retry (matches DSMGR_PWR_CNTRL_CONNECT_WAIT_TIME_MS) | ||
| usleep(DS_CONTROLLER_CONNECT_WAIT_TIME_MS); | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential infinite loop without exit condition: The while loop on line 290 has while (1) with no explicit exit condition other than a successful connection. If the controller instance becomes null or the system is shutting down, the thread may not exit gracefully. Consider adding a check for shutdown conditions or a maximum retry count.
| if (_connectionRetryCount % 10 == 0) { // Log every 3 seconds | ||
| printf("[DeviceSettingsController] Connection attempt %u failed, retrying...\n", | ||
| _connectionRetryCount); | ||
| } | ||
|
|
||
| // 300ms wait before retry (matches DSMGR_PWR_CNTRL_CONNECT_WAIT_TIME_MS) |
There was a problem hiding this comment.
Incorrect comment in logging: Line 308 states "Log every 3 seconds" but the actual wait time is 300ms (DS_CONTROLLER_CONNECT_WAIT_TIME_MS = 300000 microseconds), and logging happens every 10 attempts. This would be 10 * 300ms = 3 seconds, which matches the comment. However, the comment is slightly misleading as it doesn't clarify this is based on the retry interval.
| if (_connectionRetryCount % 10 == 0) { // Log every 3 seconds | |
| printf("[DeviceSettingsController] Connection attempt %u failed, retrying...\n", | |
| _connectionRetryCount); | |
| } | |
| // 300ms wait before retry (matches DSMGR_PWR_CNTRL_CONNECT_WAIT_TIME_MS) | |
| if (_connectionRetryCount % 10 == 0) { // Log every 10 attempts (~3s with current 300ms retry interval) | |
| printf("[DeviceSettingsController] Connection attempt %u failed, retrying...\n", | |
| _connectionRetryCount); | |
| } | |
| // 300ms wait before each retry attempt (matches DSMGR_PWR_CNTRL_CONNECT_WAIT_TIME_MS) |
| */ | ||
|
|
||
| /** | ||
| * @file DeviceSettingsController.h |
There was a problem hiding this comment.
Same documentation error in duplicate file: Line 21 states "@file DeviceSettingsController.h" but the actual filename is "dsController-com.h". This is the same issue as in rpc/cli/dsController-com.h, confirming these are duplicate files with the same documentation error.
| * @file DeviceSettingsController.h | |
| * @file dsController-com.h |
ds/include/dsController-com.h
Outdated
|
|
||
| #pragma once | ||
|
|
||
| #ifdef USE_THUNDER_PLUGIN |
There was a problem hiding this comment.
Same macro naming inconsistency in duplicate file: This header also uses USE_THUNDER_PLUGIN (line 31) instead of USE_WPE_THUNDER_PLUGIN which is defined by configure.ac. This confirms the files are duplicates and share the same error.
| gDebugPrintBuffer = new(std::nothrow) char[MAX_DEBUG_LOG_BUFF_SIZE]; | ||
| if (!gDebugPrintBuffer) { | ||
| LOGE("Failed to allocate debug buffer of size %d", MAX_DEBUG_LOG_BUFF_SIZE); | ||
| return -1; | ||
| } | ||
| memset(gDebugPrintBuffer, 0, MAX_DEBUG_LOG_BUFF_SIZE); |
There was a problem hiding this comment.
Potential buffer overflow: Lines 97-102 allocate a debug buffer with MAX_DEBUG_LOG_BUFF_SIZE (8192 bytes) but the buffer is never used in the visible code. If this buffer is intended for formatted output, there should be size checks when writing to it to prevent buffer overflow vulnerabilities.
| // Destroy mutexes | ||
| pthread_mutex_destroy(&gDSAppMutex); | ||
| pthread_mutex_destroy(&gCurlInitMutex); | ||
| LOGD("Mutexes destroyed"); | ||
|
|
There was a problem hiding this comment.
Mutex destroyed while potentially in use: The cleanup_resources() function destroys mutexes (lines 326-327) but doesn't verify that no other threads are holding these locks. If the connection retry thread is still running when cleanup occurs, this could lead to undefined behavior. Consider joining or canceling threads before destroying their synchronization primitives.
| // Destroy mutexes | |
| pthread_mutex_destroy(&gDSAppMutex); | |
| pthread_mutex_destroy(&gCurlInitMutex); | |
| LOGD("Mutexes destroyed"); | |
| */ | ||
|
|
||
| /** | ||
| * @file DeviceSettingsController.h |
There was a problem hiding this comment.
Incorrect file path reference in comment: Line 21 of dsController-com.h states "@file DeviceSettingsController.h" but the actual filename is "dsController-com.h". This mismatch between the comment and actual filename can cause confusion in documentation generation tools and code navigation.
| * @file DeviceSettingsController.h | |
| * @file dsController-com.h |
rpc/cli/dsController-com.cpp
Outdated
| * @brief Implementation of central controller for DeviceSettings COM-RPC connections | ||
| */ | ||
|
|
||
| #ifdef USE_THUNDER_PLUGIN |
There was a problem hiding this comment.
Macro naming inconsistency: dsController-com.cpp is conditionally compiled based on USE_THUNDER_PLUGIN (line 25), but configure.ac defines USE_WPE_THUNDER_PLUGIN. This will cause the entire file to be excluded from compilation when Thunder support is enabled.
| #ifdef USE_THUNDER_PLUGIN | |
| #ifdef USE_WPE_THUNDER_PLUGIN |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // device::Manager::Initialize(); As of now FPD only communicate through COM-RPC | ||
|
|
||
| LOGI("Initializing Front Panel from libds.so..."); | ||
| LOGI("Front panel init"); | ||
| fpIndicators = device::FrontPanelConfig::getInstance().getIndicators(); | ||
|
|
||
| for (size_t i = 0; i < fpIndicators.size(); i++) { | ||
| std::string IndicatorName = fpIndicators.at(i).getName(); | ||
| LOGI("Initializing Front Panel Indicator: %s", IndicatorName.c_str()); | ||
| } | ||
|
|
||
| LOGI("Device Settings FPD initialized successfully"); | ||
|
|
||
| LOGI("Device Settings Manager initialized successfully"); | ||
| } catch (const std::exception& e) { | ||
| LOGE("Failed to initialize Device Settings Manager: %s", e.what()); | ||
| throw; | ||
| } | ||
| } | ||
|
|
||
| // Terminate libds.so Device Settings library | ||
| void libds_Terminate() | ||
| { | ||
| try { | ||
| LOGI("Terminating Device Settings Manager from libds.so..."); | ||
| device::Manager::DeInitialize(); | ||
| LOGI("Device Settings Manager terminated successfully"); | ||
| } catch (const std::exception& e) { | ||
| LOGE("Error during Device Settings Manager termination: %s", e.what()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Commented code contradicts actual behavior. Line 157 has a comment stating "device::Manager::Initialize(); As of now FPD only communicate through COM-RPC" with the Manager::Initialize() call commented out. However, the cleanup in libds_Terminate() (line 182) calls device::Manager::DeInitialize(). This asymmetry suggests either Manager::Initialize() should be called during initialization, or DeInitialize() should not be called during cleanup. The current state may lead to improper cleanup or undefined behavior.
| #include "dsController-com.h" | ||
| #include <stdio.h> | ||
| #include <chrono> | ||
| #include <thread> |
There was a problem hiding this comment.
Missing atomic header include causes compilation error. The class uses std::atomic at line 42 (_isConnecting) but does not include the header. This will cause a compilation error in C++11 and later. Add '#include ' at the top of the file.
| #include <thread> | |
| #include <thread> | |
| #include <atomic> |
| dsapp: | ||
| @if [ -f DSCliLibStandaloneApp.cpp ]; then \ | ||
| echo "Building DSApp..."; \ | ||
| $(CXX) $(CFLAGS) -std=c++11 DSCliLibStandaloneApp.cpp -o ../DSApp -L$(INSTALL)/lib -lds -Wl,--no-as-needed -ldshalcli -Wl,--as-needed -lcurl -lpthread $(LDFLAGS); \ |
There was a problem hiding this comment.
Incorrect linker flag order may cause undefined references. At line 94, the library order places -lds before -ldshalcli, but libds.so depends on libdshalcli.so (as shown in ds/Makefile.am line 67 where libds_la_LIBADD includes libdshalcli.la). In GNU ld, libraries should be ordered so that dependent libraries appear before the libraries they depend on. The correct order should be: -lds -Wl,--no-as-needed -ldshalcli -Wl,--as-needed. However, given the current order with --no-as-needed, this may work but is still semantically incorrect.
| $(CXX) $(CFLAGS) -std=c++11 DSCliLibStandaloneApp.cpp -o ../DSApp -L$(INSTALL)/lib -lds -Wl,--no-as-needed -ldshalcli -Wl,--as-needed -lcurl -lpthread $(LDFLAGS); \ | |
| $(CXX) $(CFLAGS) -std=c++11 DSCliLibStandaloneApp.cpp -o ../DSApp -L$(INSTALL)/lib -Wl,--no-as-needed -ldshalcli -Wl,--as-needed -lds -lcurl -lpthread $(LDFLAGS); \ |
| export STANDALONE_BUILD_ENABLED=y | ||
| export DS_MGRS=$WORKDIR | ||
|
|
||
| export USE_WPE_THUNDER_PLUGIN=y |
There was a problem hiding this comment.
Spelling error in comment. "Devicesttings" should be "DeviceSettings" (missing 'e' and has extra 't').
| static std::atomic<bool> _isConnecting; | ||
| static uint32_t _connectionRetryCount; | ||
|
|
||
| #define DS_CONTROLLER_CONNECT_WAIT_TIME_MS 300000 // 300ms in microseconds |
There was a problem hiding this comment.
Incorrect comment in macro definition. The comment states "300ms in microseconds" but the value 300000 represents 300ms in microseconds (300000 µs = 300 ms), not milliseconds. However, the usage at line 314 calls usleep(DS_CONTROLLER_CONNECT_WAIT_TIME_MS), which expects microseconds. Either the macro name should be DS_CONTROLLER_CONNECT_WAIT_TIME_US to reflect that it's in microseconds, or the comment should clarify the unit conversion. The current naming/comment creates confusion about the actual time unit.
| #define DS_CONTROLLER_CONNECT_WAIT_TIME_MS 300000 // 300ms in microseconds | |
| #define DS_CONTROLLER_CONNECT_WAIT_TIME_MS 300000 // 300 ms delay, value expressed in microseconds (for usleep) |
| #include <core/core.h> | ||
| #include <com/com.h> | ||
| #include <plugins/Types.h> | ||
| #include <interfaces/IDeviceSettings.h> | ||
| #include <interfaces/IDeviceSettingsFPD.h> | ||
| #include <interfaces/IDeviceSettingsHDMIIn.h> | ||
| #include <interfaces/IDeviceSettingsAudio.h> | ||
| #include <interfaces/IDeviceSettingsDisplay.h> | ||
| #include <interfaces/IDeviceSettingsVideoPort.h> | ||
| #include <interfaces/IDeviceSettingsVideoDevice.h> | ||
| #include <interfaces/IDeviceSettingsHost.h> | ||
| #include <interfaces/IDeviceSettingsCompositeIn.h> |
There was a problem hiding this comment.
Missing atomic header include. The header declares std::atomic at line 107 but does not include the header, which is required for std::atomic. Add '#include ' to the includes section.
| void* DeviceSettingsController::RetryThunderConnectionThread(void* arg) | ||
| { | ||
| printf("[DeviceSettingsController] RetryThunderConnectionThread entry\n"); | ||
|
|
||
| DeviceSettingsController* controller = DeviceSettingsController::GetInstance(); | ||
| if (controller == nullptr) { | ||
| fprintf(stderr, "[DeviceSettingsController] Controller instance is null\n"); | ||
| _isConnecting = false; | ||
| return arg; | ||
| } | ||
|
|
||
| // Infinite retry loop (matches dsMgrPwrRetryEstablishConnThread pattern) | ||
| while (1) | ||
| { | ||
| _connectionRetryCount++; | ||
|
|
||
| uint32_t result = controller->Connect(); | ||
|
|
||
| if (result == Core::ERROR_NONE) { | ||
| printf("[DeviceSettingsController] Thunder connection SUCCESS after %u attempts\n", | ||
| _connectionRetryCount); | ||
|
|
||
| // Post-connection initialization | ||
| FetchAndInitializeInterfaces(); | ||
|
|
||
| _isConnecting = false; | ||
| printf("[DeviceSettingsController] RetryThunderConnectionThread completed successfully\n"); | ||
| break; | ||
| } | ||
| else { | ||
| if (_connectionRetryCount % 10 == 0) { // Log every 3 seconds | ||
| printf("[DeviceSettingsController] Connection attempt %u failed, retrying...\n", | ||
| _connectionRetryCount); | ||
| } | ||
|
|
||
| // 300ms wait before retry (matches DSMGR_PWR_CNTRL_CONNECT_WAIT_TIME_MS) | ||
| usleep(DS_CONTROLLER_CONNECT_WAIT_TIME_MS); | ||
| } | ||
| } | ||
|
|
||
| printf("[DeviceSettingsController] RetryThunderConnectionThread exit\n"); | ||
| return arg; | ||
| } |
There was a problem hiding this comment.
Detached thread accesses instance after potential deletion. The RetryThunderConnectionThread is detached and runs in an infinite loop, but the Terminate function (line 232-248) can delete the controller instance while this thread is still running. There's no mechanism to signal the thread to stop before destroying the instance. This creates a use-after-free vulnerability. Add a shutdown flag that the thread checks in its loop, and ensure the thread completes before destroying the instance.
1 participant
No description provided.