Thunder team review #212
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -50,6 +50,19 @@ PKG_CHECK_MODULES([DBUS], [dbus-1]) | |||||||||||||
|
|
||||||||||||||
| AC_CHECK_LIB(gthread-2.0, g_thread_init) | ||||||||||||||
|
|
||||||||||||||
| # Thunder COM-RPC plugin support | ||||||||||||||
| AC_ARG_ENABLE([thunder-plugin], | ||||||||||||||
| AS_HELP_STRING([--enable-thunder-plugin], [Enable Thunder COM-RPC plugin support (default: no)]), | ||||||||||||||
| [enable_thunder_plugin=$enableval], | ||||||||||||||
| [enable_thunder_plugin=no]) | ||||||||||||||
|
|
||||||||||||||
| AM_CONDITIONAL([USE_WPE_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.
Suggested change
|
||||||||||||||
| AC_MSG_NOTICE([Thunder COM-RPC plugin support enabled])], | ||||||||||||||
|
Comment on lines
+62
to
+63
There was a problem hiding this comment.
Suggested change
|
||||||||||||||
| [AC_MSG_NOTICE([Thunder COM-RPC plugin support disabled])]) | ||||||||||||||
|
|
||||||||||||||
| # Checks for typedefs, structures, and compiler characteristics. | ||||||||||||||
| AC_TYPE_PID_T | ||||||||||||||
| AC_TYPE_SIZE_T | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -66,8 +66,10 @@ cd ${RDK_SOURCE_PATH} | |||||||||||||
| 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
Suggested change
There was a problem hiding this comment. Spelling error in comment. "Devicesttings" should be "DeviceSettings" (missing 'e' and has extra 't'). |
||||||||||||||
|
|
||||||||||||||
| 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" | ||||||||||||||
|
Comment on lines
66
to
75
There was a problem hiding this comment. This script exports |
||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,261 @@ | ||||||
| /* | ||||||
| * If not stated otherwise in this file or this component's LICENSE file the | ||||||
|
Check failure on line 2 in ds/include/dsController-com.h
|
||||||
| * following copyright and licenses apply: | ||||||
| * | ||||||
| * Copyright 2016 RDK Management | ||||||
| * | ||||||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||||||
| * you may not use this file except in compliance with the License. | ||||||
| * You may obtain a copy of the License at | ||||||
| * | ||||||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||||||
| * | ||||||
| * Unless required by applicable law or agreed to in writing, software | ||||||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||||||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||||||
| * See the License for the specific language governing permissions and | ||||||
| * limitations under the License. | ||||||
| */ | ||||||
|
|
||||||
| /** | ||||||
| * @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.
Suggested change
|
||||||
| * @brief Central controller for all DeviceSettings COM-RPC connections | ||||||
| * | ||||||
| * This controller manages a single RPC connection to the Thunder DeviceSettings plugin | ||||||
| * and provides access to all component interfaces (FPD, HDMIIn, Audio, etc.) through | ||||||
| * QueryInterface on the main IDeviceSettings interface. | ||||||
| */ | ||||||
|
|
||||||
| #pragma once | ||||||
|
|
||||||
| #ifdef USE_WPE_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> | ||||||
|
|
||||||
| namespace WPEFramework { | ||||||
|
|
||||||
| /** | ||||||
| * @brief Component types supported by DeviceSettings | ||||||
| */ | ||||||
| enum class DeviceSettingsComponent : uint8_t { | ||||||
| FPD, | ||||||
| HDMIIn, | ||||||
| Audio, | ||||||
| Display, | ||||||
| VideoPort, | ||||||
| VideoDevice, | ||||||
| Host, | ||||||
| CompositeIn | ||||||
| }; | ||||||
|
|
||||||
| /** | ||||||
| * @class DeviceSettingsController | ||||||
| * @brief Singleton controller managing all DeviceSettings component interfaces | ||||||
| * | ||||||
| * This class provides: | ||||||
| * - Single RPC connection to Thunder DeviceSettings plugin | ||||||
| * - Thread-safe access to all component interfaces | ||||||
| * - Automatic lifecycle management | ||||||
| * - Connection status monitoring | ||||||
| * - Reconnection support | ||||||
| */ | ||||||
| class DeviceSettingsController : public RPC::SmartInterfaceType<Exchange::IDeviceSettings> { | ||||||
| private: | ||||||
| using BaseClass = RPC::SmartInterfaceType<Exchange::IDeviceSettings>; | ||||||
|
|
||||||
| // Main interface | ||||||
| Exchange::IDeviceSettings* _deviceSettings; | ||||||
|
|
||||||
| // Component interfaces obtained via QueryInterface | ||||||
| Exchange::IDeviceSettingsFPD* _fpdInterface; | ||||||
| Exchange::IDeviceSettingsHDMIIn* _hdmiInInterface; | ||||||
| Exchange::IDeviceSettingsAudio* _audioInterface; | ||||||
| Exchange::IDeviceSettingsDisplay* _displayInterface; | ||||||
| Exchange::IDeviceSettingsVideoPort* _videoPortInterface; | ||||||
| Exchange::IDeviceSettingsVideoDevice* _videoDeviceInterface; | ||||||
| Exchange::IDeviceSettingsHost* _hostInterface; | ||||||
| Exchange::IDeviceSettingsCompositeIn* _compositeInInterface; | ||||||
|
|
||||||
| // Singleton instance | ||||||
| static DeviceSettingsController* _instance; | ||||||
| static Core::CriticalSection _lock; | ||||||
|
|
||||||
| // Connection state | ||||||
| bool _connected; | ||||||
| bool _shutdown; | ||||||
|
|
||||||
| // Thunder callsign | ||||||
| static constexpr const TCHAR _callSign[] = _T("org.rdk.DeviceSettings"); | ||||||
|
|
||||||
| // Connection retry thread management | ||||||
| static pthread_t _connectionThreadID; | ||||||
| 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 units in comment. The comment states "300ms in microseconds" but the value 300000 is actually 300 milliseconds when used with usleep() which expects microseconds. The comment should say "300ms converted to microseconds (300000 µs)" or simply "300000 microseconds (300ms)" to avoid confusion.
Suggested change
|
||||||
|
|
||||||
| // Helper methods | ||||||
| static void EstablishThunderConnection(); | ||||||
| static void* RetryThunderConnectionThread(void* arg); | ||||||
| static void FetchAndInitializeInterfaces(); | ||||||
|
|
||||||
| /** | ||||||
| * @brief Private constructor for singleton pattern | ||||||
| */ | ||||||
| DeviceSettingsController(); | ||||||
|
|
||||||
| /** | ||||||
| * @brief Private destructor | ||||||
| */ | ||||||
| ~DeviceSettingsController(); | ||||||
|
|
||||||
| /** | ||||||
| * @brief Operational callback from SmartInterfaceType | ||||||
| * @param upAndRunning true if plugin is operational, false otherwise | ||||||
| */ | ||||||
| virtual void Operational(const bool upAndRunning) override; | ||||||
|
|
||||||
| /** | ||||||
| * @brief Check if connected (without lock) | ||||||
| */ | ||||||
| inline bool isConnected() const { return _connected; } | ||||||
|
|
||||||
| /** | ||||||
| * @brief Query and cache a component interface | ||||||
| * @tparam T Component interface type | ||||||
| * @param interfacePtr Reference to cached interface pointer | ||||||
| * @param componentName Name for logging | ||||||
| * @return true if interface obtained successfully | ||||||
| */ | ||||||
| template<typename T> | ||||||
| bool QueryComponentInterface(T*& interfacePtr, const char* componentName); | ||||||
|
|
||||||
| public: | ||||||
| // Delete copy constructor and assignment operator | ||||||
| DeviceSettingsController(const DeviceSettingsController&) = delete; | ||||||
| DeviceSettingsController& operator=(const DeviceSettingsController&) = delete; | ||||||
|
|
||||||
| /** | ||||||
| * @brief Get singleton instance | ||||||
| * @return Pointer to singleton instance | ||||||
| */ | ||||||
| static DeviceSettingsController* GetInstance(); | ||||||
|
|
||||||
| /** | ||||||
| * @brief Initialize the controller | ||||||
| * @return Core::ERROR_NONE on success, error code otherwise | ||||||
| */ | ||||||
| static uint32_t Initialize(); | ||||||
|
|
||||||
| /** | ||||||
| * @brief Terminate the controller and release all resources | ||||||
| */ | ||||||
| static void Terminate(); | ||||||
|
|
||||||
| /** | ||||||
| * @brief Connect to Thunder DeviceSettings plugin | ||||||
| * @return Core::ERROR_NONE on success, error code otherwise | ||||||
| */ | ||||||
| uint32_t Connect(); | ||||||
|
|
||||||
| /** | ||||||
| * @brief Disconnect from Thunder DeviceSettings plugin | ||||||
| * @return Core::ERROR_NONE on success, error code otherwise | ||||||
| */ | ||||||
| uint32_t Disconnect(); | ||||||
|
|
||||||
| /** | ||||||
| * @brief Check if the main interface is operational | ||||||
| * @return true if operational, false otherwise | ||||||
| */ | ||||||
| bool IsOperational() const; | ||||||
|
|
||||||
| /** | ||||||
| * @brief Check if a specific component interface is available | ||||||
| * @param component Component type to check | ||||||
| * @return true if available, false otherwise | ||||||
| */ | ||||||
| bool IsComponentAvailable(DeviceSettingsComponent component) const; | ||||||
|
|
||||||
| /** | ||||||
| * @brief Wait for the main interface to become operational | ||||||
| * @param timeoutMs Timeout in milliseconds | ||||||
| * @return true if operational within timeout, false otherwise | ||||||
| */ | ||||||
| bool WaitForOperational(uint32_t timeoutMs = 5000) const; | ||||||
|
|
||||||
| /** | ||||||
| * @brief Wait for a specific component to become available | ||||||
| * @param component Component type to wait for | ||||||
| * @param timeoutMs Timeout in milliseconds | ||||||
| * @return true if available within timeout, false otherwise | ||||||
| */ | ||||||
| bool WaitForComponent(DeviceSettingsComponent component, uint32_t timeoutMs = 5000) const; | ||||||
|
|
||||||
| /** | ||||||
| * @brief Get FPD component interface | ||||||
| * @return Pointer to IDeviceSettingsFPD interface or nullptr | ||||||
| */ | ||||||
| Exchange::IDeviceSettingsFPD* GetFPDInterface(); | ||||||
|
|
||||||
| /** | ||||||
| * @brief Get HDMIIn component interface | ||||||
| * @return Pointer to IDeviceSettingsHDMIIn interface or nullptr | ||||||
| */ | ||||||
| Exchange::IDeviceSettingsHDMIIn* GetHDMIInInterface(); | ||||||
|
|
||||||
| /** | ||||||
| * @brief Get Audio component interface | ||||||
| * @return Pointer to IDeviceSettingsAudio interface or nullptr | ||||||
| */ | ||||||
| Exchange::IDeviceSettingsAudio* GetAudioInterface(); | ||||||
|
|
||||||
| /** | ||||||
| * @brief Get Display component interface | ||||||
| * @return Pointer to IDeviceSettingsDisplay interface or nullptr | ||||||
| */ | ||||||
| Exchange::IDeviceSettingsDisplay* GetDisplayInterface(); | ||||||
|
|
||||||
| /** | ||||||
| * @brief Get VideoPort component interface | ||||||
| * @return Pointer to IDeviceSettingsVideoPort interface or nullptr | ||||||
| */ | ||||||
| Exchange::IDeviceSettingsVideoPort* GetVideoPortInterface(); | ||||||
|
|
||||||
| /** | ||||||
| * @brief Get VideoDevice component interface | ||||||
| * @return Pointer to IDeviceSettingsVideoDevice interface or nullptr | ||||||
| */ | ||||||
| Exchange::IDeviceSettingsVideoDevice* GetVideoDeviceInterface(); | ||||||
|
|
||||||
| /** | ||||||
| * @brief Get Host component interface | ||||||
| * @return Pointer to IDeviceSettingsHost interface or nullptr | ||||||
| */ | ||||||
| Exchange::IDeviceSettingsHost* GetHostInterface(); | ||||||
|
|
||||||
| /** | ||||||
| * @brief Get CompositeIn component interface | ||||||
| * @return Pointer to IDeviceSettingsCompositeIn interface or nullptr | ||||||
| */ | ||||||
| Exchange::IDeviceSettingsCompositeIn* GetCompositeInInterface(); | ||||||
| }; | ||||||
|
|
||||||
| } // namespace WPEFramework | ||||||
|
|
||||||
| #endif // USE_WPE_THUNDER_PLUGIN | ||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -21,8 +21,20 @@ CFLAGS += -g -fPIC -D_REENTRANT -Wall | |||||||||||||
| LIBNAME := dshalcli | ||||||||||||||
| LIBNAMEFULL := lib$(LIBNAME).so | ||||||||||||||
| INSTALL := $(PWD)/install | ||||||||||||||
|
|
||||||||||||||
| # Conditional compilation: Thunder vs IARM | ||||||||||||||
| ifdef USE_WPE_THUNDER_PLUGIN | ||||||||||||||
|
There was a problem hiding this comment. Build inconsistency between autotools and plain Makefile. The rpc/cli/Makefile.am uses the variable USE_THUNDER_PLUGIN (line 33), while rpc/cli/Makefile uses USE_WPE_THUNDER_PLUGIN (line 26). This inconsistency means that the two build systems will have different conditional compilation behavior. Use the same variable name in both build systems, or ensure proper translation between them. |
||||||||||||||
| # Thunder mode - use dsFPD-com.cpp and dsController-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 both dsFPD-com.cpp and dsController-com.cpp | ||||||||||||||
| OBJS := $(patsubst %.cpp,%.o,$(filter-out dsFPD-com.cpp dsController-com.cpp,$(wildcard *.cpp))) | ||||||||||||||
| OBJS += $(patsubst %.c,%.o,$(wildcard *.c)) | ||||||||||||||
| endif | ||||||||||||||
|
Comment on lines
25
to
34
There was a problem hiding this comment. This Makefile switches object selection based on the make variable
Comment on lines
25
to
34
There was a problem hiding this comment. The Makefile |
||||||||||||||
|
|
||||||||||||||
| #OBJS := $(patsubst %.cpp,%.o,$(wildcard *.cpp)) | ||||||||||||||
| #OBJS += $(patsubst %.c,%.o,$(wildcard *.c)) | ||||||||||||||
|
Comment on lines
+36
to
+37
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.
Suggested change
|
||||||||||||||
| INCLUDE := -I$(PWD) \ | ||||||||||||||
| -I$(PWD)/hal/include \ | ||||||||||||||
| -I$(PWD)/rpc/include | ||||||||||||||
|
|
@@ -31,13 +43,19 @@ INCLUDE += $(HAL_INCLUDE) | |||||||||||||
|
|
||||||||||||||
| CFLAGS += $(INCLUDE) | ||||||||||||||
|
|
||||||||||||||
| # Conditional linking flags | ||||||||||||||
| ifdef USE_WPE_THUNDER_PLUGIN | ||||||||||||||
| LDLIBS := -lWPEFrameworkCore -lWPEFrameworkCOM | ||||||||||||||
| else | ||||||||||||||
|
Comment on lines
+48
to
+49
There was a problem hiding this comment. In Thunder mode,
Suggested change
|
||||||||||||||
| LDLIBS := -lIARMBus | ||||||||||||||
| endif | ||||||||||||||
|
Comment on lines
+46
to
+51
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 |
||||||||||||||
|
|
||||||||||||||
| all: install | ||||||||||||||
| @echo "Build Finished...." | ||||||||||||||
|
|
||||||||||||||
| library: $(OBJS) | ||||||||||||||
| @echo "Building $(LIBNAMEFULL) ...." | ||||||||||||||
| $(CXX) $(OBJS) $(CFLAGS) $(LDLIBS) -shared -o $(LIBNAMEFULL) | ||||||||||||||
|
|
||||||||||||||
| %.o: %.cpp | ||||||||||||||
| @echo "Building $@ ...." | ||||||||||||||
|
|
||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This target prints
DSApp executable: $(abs_top_builddir)/DSApp, but the underlyingsample/Makefilecurrently builds DSApp to../DSApprelative tosample/, 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)).