RDKEMW-17454 : Bluetooth Migration and Rollback Support#74
Open
pahearn73 wants to merge 20 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a manual test plan for Bluetooth paired-device persistence migration/rollback scenarios and introduces additional runtime logging around migration-related build/runtime configuration, while also adjusting the build configuration for the migration feature gate.
Changes:
- Added a comprehensive manual test plan document for migration and firmware rollback validation.
- Added info-level logs to print the configured persistence path and whether migration support is compiled in.
- Removed the top-level CMake option declaration for
BLUETOOTH_ENABLE_PERSISTENCE_MIGRATION.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
docs/paired_device_migration_manual_test_plan.md |
New manual test plan covering migration, rollback sync, and edge cases. |
CMakeLists.txt |
Removes the top-level CMake option for the migration feature gate. |
Bluetooth/BluetoothPersistenceAdapter.cpp |
Adds a log of the configured filesystem persistence path. |
Bluetooth/BluetoothDeviceManager.cpp |
Adds a log indicating whether migration support is compiled in. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
CMakeLists.txt:45
BLUETOOTH_ENABLE_PERSISTENCE_MIGRATIONis no longer declared as a CMake option at the top level. SinceBluetooth/CMakeLists.txtguards sources/compile-definitions withif (BLUETOOTH_ENABLE_PERSISTENCE_MIGRATION), an undefined variable evaluates to false and effectively disables migration support by default (contrary to the intended default-ON behavior). Reintroduce the option (default ON) at an appropriate scope (root or Bluetooth) so builds behave consistently without requiring an explicit -D override.
# All packages that did not deliver a CMake Find script (and some deprecated scripts that need to be removed)
# are located in the cmake directory. Include it in the search.
list(APPEND CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/cmake/")
option(COMCAST_CONFIG "Comcast services configuration" ON)
if(COMCAST_CONFIG)
include(services.cmake)
endif()
# Library installation section
string(TOLOWER ${NAMESPACE} STORAGE_DIRECTORY)
# for writing pc and config files
include(CmakeHelperFunctions)
if(RDK_SERVICES_L1_TEST)
add_subdirectory(Tests/L1Tests)
endif()
if(PLUGIN_BLUETOOTH)
add_subdirectory(Bluetooth)
endif()
pahearn73
force-pushed
the
feature/RDKEMW-17454
branch
from
4dfa453 to
465ea03
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
Bluetooth/BluetoothDeviceManager.cpp:400
BluetoothDeviceManager::init()storesserviceand callsAddRef(), but then returns early on failures (PersistentStore read failure, BTRMGR failure, or PersistentStore write failure) without releasing_service. SinceBluetooth::Initialize()returns immediately oninit()failure without callingdeinit(), this leaks the IShell reference (and leaves_servicenon-null). Ensure_service->Release()(and_service=nullptr) is performed on all error paths afterAddRef()(e.g., via a scope guard).
_service = service;
_service->AddRef();
const Core::hresult storageResult = updateCacheFromStorage();
if ((Core::ERROR_NONE != storageResult) && (Core::ERROR_NOT_EXIST != storageResult)) {
LOGERR("PersistentStore read failed (hresult=%d); aborting init to avoid data loss", storageResult);
return storageResult;
}
#ifdef BLUETOOTH_ENABLE_PERSISTENCE_MIGRATION
if (Core::ERROR_NOT_EXIST == storageResult) {
LOGINFO("Migration attempted: PersistentStore device info is missing; trying filesystem persistence import.");
const Core::hresult migrationResult = writeCacheFromFilesystemPersistence();
if (Core::ERROR_NONE == migrationResult) {
const Core::hresult migrationPersistResult = writeStorageFromCache();
if (Core::ERROR_NONE == migrationPersistResult) {
LOGINFO("Migration succeeded: Filesystem persistence data imported and persisted to PersistentStore.");
} else {
LOGWARN("Migration failed: Unable to persist imported filesystem persistence data, hresult=%d", migrationPersistResult);
}
} else if (Core::ERROR_NOT_EXIST == migrationResult) {
LOGINFO("Migration skipped: Filesystem persistence source not found.");
} else {
LOGWARN("Migration failed: Filesystem persistence source is invalid, hresult=%d", migrationResult);
}
}
#endif
const Core::hresult deviceResult = updateCacheFromDevice();
if (Core::ERROR_NONE != deviceResult) {
// BTRMGR is fundamental to all BT operations — if it's unavailable here it
// will be unavailable for everything else. Fail init so the plugin is not
// activated in a broken state.
LOGERR("Failed to update cache from device (hresult=%d); aborting init", deviceResult);
return deviceResult;
}
const Core::hresult writeResult = writeStorageFromCache();
if (Core::ERROR_NONE != writeResult) {
LOGWARN("Failed to write cache to PersistentStore, hresult=%d", writeResult);
return writeResult;
}
cb7fc2f to
ca54d37
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
Tests/L2Tests/tests/Bluetooth_L2Test.cpp:786
- setDeviceVolumeMuteInfoWrapper expects params: deviceID, deviceType, volume, mute (numeric). This test currently reuses deviceProfile and passes "muted" as a boolean, so the wrapper’s required-parameter checks will fail (or mute will be parsed incorrectly). Update the request to use the correct parameter names and types expected by the plugin.
params["volume"] = 50;
params["muted"] = false;
status = InvokeServiceMethod("org.rdk.Bluetooth.1", "setDeviceVolumeMuteInfo", params, result);
EXPECT_EQ(Core::ERROR_NONE, status);
EXPECT_TRUE(result["success"].Boolean());
Comment on lines
+402
to
+406
| params["enabled"] = "true"; | ||
| uint32_t status = InvokeServiceMethod("org.rdk.Bluetooth.1", "enable", params, result); | ||
| /* Plugin returns success=false in the JSON body; the RPC transport itself succeeds */ | ||
| EXPECT_FALSE(result["success"].Boolean()); | ||
| } |
Comment on lines
+769
to
+775
| params["deviceID"] = std::to_string(TEST_DEVICE_HANDLE); | ||
| params["deviceProfile"] = TEST_DEVICE_TYPE_STR; | ||
| status = InvokeServiceMethod("org.rdk.Bluetooth.1", "getDeviceVolumeMuteInfo", params, result); | ||
| EXPECT_EQ(Core::ERROR_NONE, status); | ||
| EXPECT_TRUE(result["success"].Boolean()); | ||
| EXPECT_EQ(80u, result["volumeInfo"].Object()["volume"].Number()); | ||
| EXPECT_FALSE(result["volumeInfo"].Object()["mute"].Boolean()); |
Comment on lines
+825
to
+826
| params["deviceID"] = std::to_string(TEST_DEVICE_HANDLE); | ||
| params["audioCtrlCmd"] = "PLAY"; |
| THUNDER_REF: "R4.4.1" | ||
| INTERFACES_REF: "develop" | ||
| AUTOMATICS_UNAME: ${{ secrets.AUTOMATICS_UNAME}} | ||
| AUTOMATICS_PASSCODE: ${{ secrets. AUTOMATICS_PASSCODE}} |
Comment on lines
+181
to
+182
| run: rm -rf $GITHUB_WORKSPACE/entservices-apis/jsonrpc/DTV.json | ||
|
|
Comment on lines
371
to
378
| _service = service; | ||
| _service->AddRef(); | ||
|
|
||
| const Core::hresult storageResult = updateCacheFromStorage(); | ||
| if ((Core::ERROR_NONE != storageResult) && (Core::ERROR_NOT_EXIST != storageResult)) { | ||
| if ((Core::ERROR_NONE != storageResult) && !missingFromPersistentStore(storageResult)) { | ||
| LOGERR("PersistentStore read failed (hresult=%d); aborting init to avoid data loss", storageResult); | ||
| return storageResult; | ||
| } |
Comment on lines
+171
to
+177
| /* BluetoothDeviceManager::updateCacheFromDevice() — return empty paired list */ | ||
| ON_CALL(*p_btmgrImplMock, BTRMGR_GetPairedDevices(::testing::_, ::testing::_)) | ||
| .WillByDefault(::testing::Invoke( | ||
| [](unsigned char, BTRMGR_PairedDevicesList_t* pList) -> BTRMGR_Result_t { | ||
| pList->m_numOfDevices = 0; | ||
| return BTRMGR_RESULT_SUCCESS; | ||
| })); |
|
|
||
| params["enabled"] = "true"; | ||
| uint32_t status = InvokeServiceMethod("org.rdk.Bluetooth.1", "enable", params, result); | ||
| /* Plugin returns success=false in the JSON body; the RPC transport itself succeeds */ |
| EXPECT_CALL(*p_btmgrImplMock, BTRMGR_GetAdapterName(0, ::testing::_)) | ||
| .WillOnce(::testing::Invoke( | ||
| [](unsigned char, char* pName) -> BTRMGR_Result_t { | ||
| strncpy(pName, "TestAdapter", BTRMGR_NAME_LEN_MAX - 1); |
Comment on lines
+832
to
+836
| /* ========================================================================= | ||
| * TC-18: onStatusChanged event — DISCOVERY_STARTED | ||
| * Inject BTRMGR_EVENT_DEVICE_DISCOVERY_STARTED and verify the plugin fires | ||
| * onStatusChanged with newStatus == "DISCOVERY_STARTED". | ||
| * ====================================================================== */ |
Comment on lines
+25
to
+30
| if(PLUGIN_BLUETOOTH) | ||
| set(SRC_FILES ${SRC_FILES} tests/Bluetooth_L2Test.cpp) | ||
| endif() | ||
|
|
||
| add_library(${MODULE_NAME} SHARED ${SRC_FILES}) | ||
|
|
| THUNDER_REF: "R4.4.1" | ||
| INTERFACES_REF: "develop" | ||
| AUTOMATICS_UNAME: ${{ secrets.AUTOMATICS_UNAME}} | ||
| AUTOMATICS_PASSCODE: ${{ secrets. AUTOMATICS_PASSCODE}} |
Comment on lines
+175
to
+182
| - name: Checkout entservices-apis | ||
| uses: actions/checkout@v3 | ||
| with: | ||
| repository: rdkcentral/entservices-apis | ||
| path: entservices-apis | ||
| ref: ${{env.INTERFACES_REF}} | ||
| run: rm -rf $GITHUB_WORKSPACE/entservices-apis/jsonrpc/DTV.json | ||
|
|
Comment on lines
+118
to
+125
| - name: Checkout googletest | ||
| if: steps.cache.outputs.cache-hit != 'true' | ||
| uses: actions/checkout@v3 | ||
| with: | ||
| repository: google/googletest | ||
| path: googletest | ||
| ref: v1.15.0 | ||
|
|
Comment on lines
+34
to
+37
| * If PersistentStore is not running, BluetoothDeviceManager treats the interface | ||
| * being unavailable as empty storage (Core::ERROR_NOT_EXIST) and proceeds with | ||
| * an empty in-memory cache. Bluetooth activation therefore succeeds regardless | ||
| * of PersistentStore availability. |
Comment on lines
+780
to
+786
| params["deviceID"] = std::to_string(TEST_DEVICE_HANDLE); | ||
| params["deviceProfile"] = TEST_DEVICE_TYPE_STR; | ||
| status = InvokeServiceMethod("org.rdk.Bluetooth.1", "getDeviceVolumeMuteInfo", params, result); | ||
| EXPECT_EQ(Core::ERROR_NONE, status); | ||
| EXPECT_TRUE(result["success"].Boolean()); | ||
| EXPECT_EQ(80u, result["volumeInfo"].Object()["volume"].Number()); | ||
| EXPECT_FALSE(result["volumeInfo"].Object()["mute"].Boolean()); |
Comment on lines
+793
to
+794
| params["volume"] = 50; | ||
| params["muted"] = false; |
Comment on lines
+29
to
+72
| add_library(${MODULE_NAME} SHARED ${SRC_FILES}) | ||
|
|
||
| set_target_properties(${MODULE_NAME} PROPERTIES | ||
| CXX_STANDARD 14 | ||
| CXX_STANDARD_REQUIRED YES) | ||
|
|
||
| target_compile_definitions(${MODULE_NAME} | ||
| PRIVATE | ||
| MODULE_NAME=Plugin_${PLUGIN_NAME} | ||
| THUNDER_PORT="${THUNDER_PORT}") | ||
|
|
||
| target_compile_options(${MODULE_NAME} PRIVATE -Wno-error) | ||
| target_link_libraries(${MODULE_NAME} PRIVATE ${NAMESPACE}Plugins::${NAMESPACE}Plugins) | ||
|
|
||
| if (NOT L2_TEST_OOP_RPC) | ||
| find_library(TESTMOCKLIB_LIBRARIES NAMES TestMocklib) | ||
| if (TESTMOCKLIB_LIBRARIES) | ||
| message ("Found mock library - ${TESTMOCKLIB_LIBRARIES}") | ||
| target_link_libraries(${MODULE_NAME} PRIVATE ${TESTMOCKLIB_LIBRARIES}) | ||
| else (TESTMOCKLIB_LIBRARIES) | ||
| message ("Require ${TESTMOCKLIB_LIBRARIES} library") | ||
| endif (TESTMOCKLIB_LIBRARIES) | ||
| endif (NOT L2_TEST_OOP_RPC) | ||
|
|
||
| find_library(MOCKACCESSOR_LIBRARIES NAMES MockAccessor) | ||
| if (MOCKACCESSOR_LIBRARIES) | ||
| message ("Found MockAccessor library - ${MOCKACCESSOR_LIBRARIES}") | ||
| target_link_libraries(${MODULE_NAME} PRIVATE ${MOCKACCESSOR_LIBRARIES}) | ||
| else (MOCKACCESSOR_LIBRARIES) | ||
| message ("Require ${MOCKACCESSOR_LIBRARIES} library") | ||
| endif (MOCKACCESSOR_LIBRARIES) | ||
|
|
||
| target_include_directories( | ||
| ${MODULE_NAME} PRIVATE ./ | ||
| ../../helpers | ||
| ../../../entservices-testframework/Tests/mocks | ||
| ../../../entservices-testframework/Tests/mocks/thunder | ||
| ../../../entservices-testframework/Tests/mocks/devicesettings | ||
| ../../../entservices-testframework/Tests/mocks/MockPlugin | ||
| ../../../entservices-testframework/Tests/L2Tests/L2TestsPlugin | ||
| ${CMAKE_INSTALL_PREFIX}/include | ||
| ) | ||
|
|
||
| install(TARGETS ${MODULE_NAME} DESTINATION lib) |
| ${CMAKE_INSTALL_PREFIX}/include | ||
| ) | ||
|
|
||
| install(TARGETS ${MODULE_NAME} DESTINATION lib) |
Comment on lines
+13
to
+19
|
|
||
| env: | ||
| BUILD_TYPE: Debug | ||
| THUNDER_REF: "R4.4.1" | ||
| INTERFACES_REF: "develop" | ||
| AUTOMATICS_UNAME: ${{ secrets.AUTOMATICS_UNAME}} | ||
| AUTOMATICS_PASSCODE: ${{ secrets. AUTOMATICS_PASSCODE}} |
Comment on lines
+181
to
+182
| run: rm -rf $GITHUB_WORKSPACE/entservices-apis/jsonrpc/DTV.json | ||
|
|
| token: ${{ secrets.RDKCM_RDKE }} | ||
|
|
||
| - name: Checkout googletest | ||
| if: steps.cache.outputs.cache-hit != 'true' |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
version: patch