RDKEMW-15846 : Elite classic controller is not disconnect on standby#52
RDKEMW-15846 : Elite classic controller is not disconnect on standby#52natrajmuthusamy wants to merge 13 commits intodevelopfrom
Conversation
Reason for change : Sets the operation to modify the device's blocked property, enabling dynamic block/unblock control via this function. Priority: P1 Test Procedure: Follow the steps provided in description Risks: High Signed-off-by:Natraj Muthusamy <Natraj_Muthusamy@comcast.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces a new API intended to programmatically block/unblock a paired Bluetooth device (likely to address the Elite classic controller behavior around standby).
Changes:
- Added
btrCore_UpdateDeviceBlockState(...)implementation insrc/btrCore.cto setenBTDevPropBlocked. - Exposed the new API in
include/btrCore.hwith Doxygen documentation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
src/btrCore.c |
Adds a helper to set the “Blocked” device property via BtrCore_BTSetProp. |
include/btrCore.h |
Exposes/document the new API in the public header. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @retval enBTRCoreInvalidArg Invalid argument provided. | ||
| * @retval enBTRCoreDeviceNotFound Device with given ID not found. | ||
| */ | ||
| enBTRCoreRet btrCore_UpdateDeviceBlockState (tBTRCoreHandle* pstlhBTRCore, tBTRCoreDevId aBTRCoreDevId, enBTRCoreDeviceType aenBTRCoreDevType, int isBlocked); |
There was a problem hiding this comment.
This public header declares btrCore_UpdateDeviceBlockState with a parameter type stBTRCoreHdl*, but stBTRCoreHdl is not declared/defined in btrCore.h (it’s an internal struct in btrCore.c). This will break compilation for any consumer including the header; please expose this as a proper public API (e.g., BTRCore_UpdateDeviceBlockState(tBTRCoreHandle hBTRCore, ...)) or move it to a private header.
| /** | ||
| * @brief Updates the block state (blocked/unblocked) of a Bluetooth device. | ||
| * | ||
| * This API sets the device block state for the specified Bluetooth device using its device ID and type. | ||
| * It may be used to block or unblock a device from interacting with the Bluetooth core. | ||
| * | ||
| * @param[in] pstlhBTRCore Pointer to the Bluetooth Core handle. | ||
| * @param[in] aBTRCoreDevId Device ID of the target Bluetooth device. | ||
| * @param[in] aenBTRCoreDevType Device type of the target Bluetooth device. | ||
| * @param[in] isBlocked Desired block state: 1 to block the device, 0 to unblock it. |
There was a problem hiding this comment.
This PR adds a new externally visible API, but there are no corresponding unit tests (the repo has extensive coverage in unitTest/test_btrCore.c for other BTRCore_* APIs). Please add tests that cover success/failure paths (e.g., device not found, BTSetProp failure, and block/unblock values).
| * @retval enBTRCoreInvalidArg Invalid argument provided. | ||
| * @retval enBTRCoreDeviceNotFound Device with given ID not found. | ||
| */ | ||
| enBTRCoreRet btrCore_UpdateDeviceBlockState (tBTRCoreHandle* pstlhBTRCore, tBTRCoreDevId aBTRCoreDevId, enBTRCoreDeviceType aenBTRCoreDevType, int isBlocked); |
There was a problem hiding this comment.
The new API is named btrCore_UpdateDeviceBlockState (lowercase btrCore_) while the rest of the public surface in this header uses BTRCore_.... Consider renaming to match the existing public API naming to avoid confusion about whether it’s public or internal.
| enBTRCoreRet btrCore_UpdateDeviceBlockState (tBTRCoreHandle* pstlhBTRCore, tBTRCoreDevId aBTRCoreDevId, enBTRCoreDeviceType aenBTRCoreDevType, int isBlocked); | |
| #define btrCore_UpdateDeviceBlockState BTRCore_UpdateDeviceBlockState | |
| enBTRCoreRet BTRCore_UpdateDeviceBlockState (tBTRCoreHandle* pstlhBTRCore, tBTRCoreDevId aBTRCoreDevId, enBTRCoreDeviceType aenBTRCoreDevType, int isBlocked); |
| enBTRCoreRet btrCore_UpdateDeviceBlockState ( | ||
| tBTRCoreHandle* pstlhBTRCore, | ||
| tBTRCoreDevId aBTRCoreDevId, | ||
| enBTRCoreDeviceType aenBTRCoreDevType, | ||
| int isBlocked |
There was a problem hiding this comment.
This function uses aenBTRCoreDevType in the signature but never references it, which will fail the build under -Wall -Werror due to -Wunused-parameter. Either use it (e.g., via btrCore_GetDeviceInfoKnown(...)) or explicitly mark it unused if it’s intentionally ignored.
| enBTDeviceType enBTDevice = enBTDevUnknown; | ||
|
|
||
| for (i = 0; i < pstlhBTRCore->numOfPairedDevices; ++i) { | ||
| if (pstlhBTRCore->stKnownDevicesArr[i].tDeviceId == aBTRCoreDevId) { | ||
| pDeviceAddress = pstlhBTRCore->stKnownDevicesArr[i].pcDeviceAddress; | ||
| enBTDevice = pstlhBTRCore->stKnownDevicesArr[i].enDeviceType; |
There was a problem hiding this comment.
enBTDevice is declared as enBTDeviceType, but it’s being assigned from stKnownDevicesArr[i].enDeviceType (type enBTRCoreDeviceClass). This mixes unrelated enums and will produce an invalid value for later calls; avoid deriving the BT interface type from the device class here.
| if (BtrCore_BTSetProp(pstlhBTRCore->connHdl, pDeviceAddress, enBTDevice, lunBtOpDevProp, &BlockDevice)) { | ||
| BTRCORELOG_ERROR("Set Device Property enBTDevPropBlocked - FAILED\n"); |
There was a problem hiding this comment.
BtrCore_BTSetProp takes enBTOpIfceType as its 3rd parameter (e.g., enBTDevice), but this call passes enBTDevice variable of a different enum type. This can break the property set logic; pass the correct interface type constant expected by BtrCore_BTSetProp.
|
|
||
| unBTOpIfceProp lunBtOpDevProp; | ||
| lunBtOpDevProp.enBtDeviceProp = enBTDevPropBlocked; | ||
| int BlockDevice = block ? 1 : 0; |
There was a problem hiding this comment.
BlockDevice is initialized using block, but the parameter name is isBlocked. As written this won’t compile (undefined identifier) and the API can’t work. Use the isBlocked parameter (and consider normalizing it to 0/1).
| int BlockDevice = block ? 1 : 0; | |
| int BlockDevice = isBlocked ? 1 : 0; |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| enBTRCoreRet btrCore_UpdateDeviceBlockState ( | ||
| tBTRCoreHandle* hBTRCore, | ||
| tBTRCoreDevId aBTRCoreDevId, | ||
| enBTRCoreDeviceType aenBTRCoreDevType, | ||
| int isBlocked | ||
| ) { | ||
|
|
||
| if (!hBTRCore) { | ||
| BTRCORELOG_ERROR("Invalid BT Core Handle\n"); | ||
| return enBTRCoreInvalidArg; | ||
| } | ||
|
|
||
| pstlhBTRCore = (stBTRCoreHdl*)hBTRCore; | ||
|
|
||
| const char *pDeviceAddress = NULL; | ||
| int i; | ||
| enBTDeviceType enBTDevice = enBTDevUnknown; | ||
|
|
||
| for (i = 0; i < pstlhBTRCore->numOfPairedDevices; ++i) { | ||
| if (pstlhBTRCore->stKnownDevicesArr[i].tDeviceId == aBTRCoreDevId) { | ||
| pDeviceAddress = pstlhBTRCore->stKnownDevicesArr[i].pcDeviceAddress; | ||
| enBTDevice = pstlhBTRCore->stKnownDevicesArr[i].enDeviceType; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (!pDeviceAddress) { | ||
| BTRCORELOG_ERROR("Device ID %lld not found\n", aBTRCoreDevId); | ||
| return enBTRCoreDeviceNotFound; | ||
| } | ||
|
|
||
| unBTOpIfceProp lunBtOpDevProp; | ||
| lunBtOpDevProp.enBtDeviceProp = enBTDevPropBlocked; | ||
| int BlockDevice = block ? 1 : 0; | ||
|
|
||
| if (BtrCore_BTSetProp(pstlhBTRCore->connHdl, pDeviceAddress, enBTDevice, lunBtOpDevProp, &BlockDevice)) { | ||
| BTRCORELOG_ERROR("Set Device Property enBTDevPropBlocked - FAILED\n"); | ||
| return enBTRCoreFailure; | ||
| } | ||
|
|
||
| return enBTRCoreSuccess; | ||
| } |
There was a problem hiding this comment.
There are existing unit tests for btrCore.c logic in unitTest/test_btrCore.c, but no tests cover this newly introduced device-block/unblock API. Please add unit tests for at least: invalid handle, device-not-found, and successful blocked/unblocked property setting (with BtrCore_BTSetProp mocked).
| tBTRCoreHandle* hBTRCore, | ||
| tBTRCoreDevId aBTRCoreDevId, | ||
| enBTRCoreDeviceType aenBTRCoreDevType, | ||
| int isBlocked | ||
| ) { | ||
|
|
||
| if (!hBTRCore) { | ||
| BTRCORELOG_ERROR("Invalid BT Core Handle\n"); | ||
| return enBTRCoreInvalidArg; | ||
| } | ||
|
|
||
| pstlhBTRCore = (stBTRCoreHdl*)hBTRCore; | ||
|
|
There was a problem hiding this comment.
pstlhBTRCore is used without being declared/initialized in this function, and hBTRCore is typed as tBTRCoreHandle* even though tBTRCoreHandle is already a void* (see include/btrCoreTypes.h). As written, the cast pstlhBTRCore = (stBTRCoreHdl*)hBTRCore; will be wrong (and likely won’t compile due to missing declaration). Declare a local stBTRCoreHdl* pstlhBTRCore = NULL; and make the API take tBTRCoreHandle hBTRCore (by value) like the other BTRCore_* public APIs.
| enBTDeviceType enBTDevice = enBTDevUnknown; | ||
|
|
||
| for (i = 0; i < pstlhBTRCore->numOfPairedDevices; ++i) { | ||
| if (pstlhBTRCore->stKnownDevicesArr[i].tDeviceId == aBTRCoreDevId) { | ||
| pDeviceAddress = pstlhBTRCore->stKnownDevicesArr[i].pcDeviceAddress; | ||
| enBTDevice = pstlhBTRCore->stKnownDevicesArr[i].enDeviceType; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (!pDeviceAddress) { | ||
| BTRCORELOG_ERROR("Device ID %lld not found\n", aBTRCoreDevId); | ||
| return enBTRCoreDeviceNotFound; | ||
| } | ||
|
|
||
| unBTOpIfceProp lunBtOpDevProp; | ||
| lunBtOpDevProp.enBtDeviceProp = enBTDevPropBlocked; | ||
| int BlockDevice = block ? 1 : 0; | ||
|
|
||
| if (BtrCore_BTSetProp(pstlhBTRCore->connHdl, pDeviceAddress, enBTDevice, lunBtOpDevProp, &BlockDevice)) { | ||
| BTRCORELOG_ERROR("Set Device Property enBTDevPropBlocked - FAILED\n"); |
There was a problem hiding this comment.
The third argument to BtrCore_BTSetProp is enBTOpIfceType (e.g., enBTDevice), but this function defines enBTDevice as an enBTDeviceType variable and passes it through. That’s a type/semantic mismatch and will either not compile cleanly or call the API with the wrong enum value. Pass the enBTDevice interface type constant here, and keep the device-type enum separate if needed.
| int i; | ||
| enBTDeviceType enBTDevice = enBTDevUnknown; | ||
|
|
||
| for (i = 0; i < pstlhBTRCore->numOfPairedDevices; ++i) { | ||
| if (pstlhBTRCore->stKnownDevicesArr[i].tDeviceId == aBTRCoreDevId) { | ||
| pDeviceAddress = pstlhBTRCore->stKnownDevicesArr[i].pcDeviceAddress; | ||
| enBTDevice = pstlhBTRCore->stKnownDevicesArr[i].enDeviceType; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (!pDeviceAddress) { | ||
| BTRCORELOG_ERROR("Device ID %lld not found\n", aBTRCoreDevId); | ||
| return enBTRCoreDeviceNotFound; | ||
| } | ||
|
|
There was a problem hiding this comment.
aenBTRCoreDevType is currently unused, and this function manually scans stKnownDevicesArr to find the MAC/type. Elsewhere (e.g., connect/disconnect/unpair) the code uses helpers like btrCore_GetDeviceInfoKnown / btrCore_GetDeviceInfo / btrCore_GetKnownDeviceMac, which also handle the aBTRCoreDevId < BTRCORE_MAX_NUM_BT_DEVICES case and keep the lookup logic consistent. Reuse the existing helper(s) here to avoid divergent device-ID resolution and to make use of the aenBTRCoreDevType parameter (or remove it if it’s not needed).
| int i; | |
| enBTDeviceType enBTDevice = enBTDevUnknown; | |
| for (i = 0; i < pstlhBTRCore->numOfPairedDevices; ++i) { | |
| if (pstlhBTRCore->stKnownDevicesArr[i].tDeviceId == aBTRCoreDevId) { | |
| pDeviceAddress = pstlhBTRCore->stKnownDevicesArr[i].pcDeviceAddress; | |
| enBTDevice = pstlhBTRCore->stKnownDevicesArr[i].enDeviceType; | |
| break; | |
| } | |
| } | |
| if (!pDeviceAddress) { | |
| BTRCORELOG_ERROR("Device ID %lld not found\n", aBTRCoreDevId); | |
| return enBTRCoreDeviceNotFound; | |
| } | |
| enBTDeviceType enBTDevice = enBTDevUnknown; | |
| stBTRCoreBTDevice lstBTDeviceInfo; | |
| memset(&lstBTDeviceInfo, 0, sizeof(lstBTDeviceInfo)); | |
| /* Use common helper to resolve device ID to address/type, so that | |
| * aBTRCoreDevId and aenBTRCoreDevType are handled consistently with | |
| * other operations (connect/disconnect/unpair, etc.). | |
| */ | |
| if (btrCore_GetDeviceInfo(pstlhBTRCore, | |
| aBTRCoreDevId, | |
| aenBTRCoreDevType, | |
| &lstBTDeviceInfo) != enBTRCoreSuccess) { | |
| BTRCORELOG_ERROR("Device ID %lld not found\n", aBTRCoreDevId); | |
| return enBTRCoreDeviceNotFound; | |
| } | |
| pDeviceAddress = lstBTDeviceInfo.pcDeviceAddress; | |
| enBTDevice = lstBTDeviceInfo.enDeviceType; |
| } | ||
|
|
||
| if (!pDeviceAddress) { | ||
| BTRCORELOG_ERROR("Device ID %lld not found\n", aBTRCoreDevId); |
There was a problem hiding this comment.
Log format specifier uses %lld for aBTRCoreDevId, but tBTRCoreDevId is an unsigned long long int and this file consistently logs device IDs with %llu (e.g., in BTRCore_ConnectDevice). Please update the format specifier (and/or cast) to avoid undefined behavior and incorrect logging on some platforms.
| BTRCORELOG_ERROR("Device ID %lld not found\n", aBTRCoreDevId); | |
| BTRCORELOG_ERROR("Device ID %llu not found\n", aBTRCoreDevId); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| enBTRCoreRet btrCore_UpdateDeviceBlockState ( | ||
| tBTRCoreHandle* hBTRCore, |
There was a problem hiding this comment.
No call sites for btrCore_UpdateDeviceBlockState exist in the repository after this change. If the intent is to fix the standby disconnect issue (per PR title), this PR may be incomplete unless another component will start calling this new API as part of the standby path.
| enBTRCoreRet btrCore_UpdateDeviceBlockState ( | ||
| tBTRCoreHandle* hBTRCore, | ||
| tBTRCoreDevId aBTRCoreDevId, | ||
| enBTRCoreDeviceType aenBTRCoreDevType, | ||
| int isBlocked | ||
| ) { | ||
|
|
||
| stBTRCoreHdl* pstlhBTRCore = NULL; | ||
| if (!hBTRCore) { | ||
| BTRCORELOG_ERROR("Invalid BT Core Handle\n"); | ||
| return enBTRCoreInvalidArg; | ||
| } | ||
|
|
||
| pstlhBTRCore = (stBTRCoreHdl*)hBTRCore; | ||
|
|
There was a problem hiding this comment.
The new API takes tBTRCoreHandle* (i.e., void**) but the rest of BTRCore public APIs take tBTRCoreHandle (void*). The implementation then casts hBTRCore directly to stBTRCoreHdl*, which will be an invalid pointer if callers pass the handle by value like other APIs. Please change the signature to take tBTRCoreHandle hBTRCore and align the null-handle return code with the existing pattern (enBTRCoreNotInitialized).
| const char *pDeviceAddress = NULL; | ||
| int i; | ||
| enBTDeviceType enBTDevice = enBTDevUnknown; | ||
|
|
||
| for (i = 0; i < pstlhBTRCore->numOfPairedDevices; ++i) { | ||
| if (pstlhBTRCore->stKnownDevicesArr[i].tDeviceId == aBTRCoreDevId) { | ||
| pDeviceAddress = pstlhBTRCore->stKnownDevicesArr[i].pcDeviceAddress; | ||
| enBTDevice = pstlhBTRCore->stKnownDevicesArr[i].enDeviceType; | ||
| break; |
There was a problem hiding this comment.
BtrCore_BTSetProp expects a BlueZ object path (see other call sites that pass the device path returned from btrCore_GetDeviceInfoKnown), but this code looks up and passes pcDeviceAddress (MAC) instead. Also, stKnownDevicesArr[i].enDeviceType is enBTRCoreDeviceClass, not enBTDeviceType, so passing it as enBTDeviceType is type/semantic mismatch. Consider reusing btrCore_GetDeviceInfoKnown(..., aenBTRCoreDevType, &lenBTDeviceType, ..., &pcDevicePath) and pass pcDevicePath + lenBTDeviceType to BtrCore_BTSetProp.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
sync with develop
No description provided.