From 7726d3bb094d7ade0e9e1f2ad77f922ba65bb6ad Mon Sep 17 00:00:00 2001 From: Shubham Patil Date: Fri, 28 Jun 2024 19:20:52 +0530 Subject: [PATCH] [v1.3][ESP32] Cherry-pick few BLEManager fixes (#34123) * [ESP32] Replaced FreeRTOS timer with CHIP timers in nimble BLEManagerImpl. (#34050) * Replace free RTOS timer with ChipTimer * Replaced FreeRTOS timer with CHIP timer * ESP32: Fixes in BLEManager (#34082) - Fix the ble re-advertisement if invalid bytes are written on the characteristic. - cancel the ble advertisement timer on ble connection - remove the unnecessary if check * Fix crash for eps32 commissioner if ble disconnect during commissioning (#33332) --------- Co-authored-by: Mahesh <92411857+pimpalemahesh@users.noreply.github.com> Co-authored-by: cdj <45139296+DejinChen@users.noreply.github.com> --- src/platform/ESP32/BLEManagerImpl.h | 2 +- .../ESP32/bluedroid/BLEManagerImpl.cpp | 1 + src/platform/ESP32/nimble/BLEManagerImpl.cpp | 79 ++++++++----------- 3 files changed, 35 insertions(+), 47 deletions(-) diff --git a/src/platform/ESP32/BLEManagerImpl.h b/src/platform/ESP32/BLEManagerImpl.h index 8265b36f177028..2baa2a04836c8b 100644 --- a/src/platform/ESP32/BLEManagerImpl.h +++ b/src/platform/ESP32/BLEManagerImpl.h @@ -302,7 +302,7 @@ class BLEManagerImpl final : public BLEManager, CHIP_ERROR StartAdvertising(void); void StartBleAdvTimeoutTimer(uint32_t aTimeoutInMs); void CancelBleAdvTimeoutTimer(void); - static void BleAdvTimeoutHandler(TimerHandle_t xTimer); + static void BleAdvTimeoutHandler(System::Layer *, void *); #if CONFIG_BT_BLUEDROID_ENABLED void HandleGATTControlEvent(esp_gatts_cb_event_t event, esp_gatt_if_t gatts_if, esp_ble_gatts_cb_param_t * param); diff --git a/src/platform/ESP32/bluedroid/BLEManagerImpl.cpp b/src/platform/ESP32/bluedroid/BLEManagerImpl.cpp index 06c95b3b45f153..e86f7dfd3cd805 100644 --- a/src/platform/ESP32/bluedroid/BLEManagerImpl.cpp +++ b/src/platform/ESP32/bluedroid/BLEManagerImpl.cpp @@ -916,6 +916,7 @@ bool BLEManagerImpl::SendReadResponse(BLE_CONNECTION_OBJECT conId, BLE_READ_REQU void BLEManagerImpl::NotifyChipConnectionClosed(BLE_CONNECTION_OBJECT conId) { ChipLogProgress(Ble, "Got notification regarding chip connection closure"); + CloseConnection(conId); } CHIP_ERROR BLEManagerImpl::MapBLEError(int bleErr) diff --git a/src/platform/ESP32/nimble/BLEManagerImpl.cpp b/src/platform/ESP32/nimble/BLEManagerImpl.cpp index 9ee4476c39730c..997e36bc53dcac 100644 --- a/src/platform/ESP32/nimble/BLEManagerImpl.cpp +++ b/src/platform/ESP32/nimble/BLEManagerImpl.cpp @@ -76,8 +76,7 @@ namespace Internal { namespace { -TimerHandle_t sbleAdvTimeoutTimer; // FreeRTOS sw timer. -#if CONFIG_ENABLE_ESP32_BLE_CONTROLLER +#ifdef CONFIG_ENABLE_ESP32_BLE_CONTROLLER static constexpr uint16_t kNewConnectionScanTimeout = 60; static constexpr uint16_t kConnectTimeout = 20; #endif @@ -212,16 +211,6 @@ CHIP_ERROR BLEManagerImpl::_Init() { CHIP_ERROR err; - // Create FreeRTOS sw timer for BLE timeouts and interval change. - sbleAdvTimeoutTimer = xTimerCreate("BleAdvTimer", // Just a text name, not used by the RTOS kernel - 1, // == default timer period - false, // no timer reload (==one-shot) - (void *) this, // init timer id = ble obj context - BleAdvTimeoutHandler // timer callback handler - ); - - VerifyOrReturnError(sbleAdvTimeoutTimer != nullptr, CHIP_ERROR_NO_MEMORY); - // Initialize the Chip BleLayer. #if CONFIG_ENABLE_ESP32_BLE_CONTROLLER err = BleLayer::Init(this, this, this, &DeviceLayer::SystemLayer()); @@ -257,9 +246,7 @@ CHIP_ERROR BLEManagerImpl::_Init() void BLEManagerImpl::_Shutdown() { - VerifyOrReturn(sbleAdvTimeoutTimer != nullptr); - xTimerDelete(sbleAdvTimeoutTimer, portMAX_DELAY); - sbleAdvTimeoutTimer = nullptr; + CancelBleAdvTimeoutTimer(); BleLayer::Shutdown(); @@ -294,7 +281,7 @@ CHIP_ERROR BLEManagerImpl::_SetAdvertisingEnabled(bool val) return err; } -void BLEManagerImpl::BleAdvTimeoutHandler(TimerHandle_t xTimer) +void BLEManagerImpl::BleAdvTimeoutHandler(System::Layer *, void *) { if (BLEMgrImpl().mFlags.Has(Flags::kFastAdvertisingEnabled)) { @@ -306,7 +293,6 @@ void BLEManagerImpl::BleAdvTimeoutHandler(TimerHandle_t xTimer) BLEMgrImpl().mFlags.Clear(Flags::kExtAdvertisingEnabled); BLEMgrImpl().StartBleAdvTimeoutTimer(CHIP_DEVICE_CONFIG_BLE_EXT_ADVERTISING_INTERVAL_CHANGE_TIME_MS); #endif - PlatformMgr().ScheduleWork(DriveBLEState, 0); } #if CHIP_DEVICE_CONFIG_BLE_EXT_ADVERTISING else @@ -316,9 +302,9 @@ void BLEManagerImpl::BleAdvTimeoutHandler(TimerHandle_t xTimer) BLEMgrImpl().mFlags.Set(Flags::kExtAdvertisingEnabled); BLEMgr().SetAdvertisingMode(BLEAdvertisingMode::kSlowAdvertising); BLEMgrImpl().mFlags.Set(Flags::kAdvertisingRefreshNeeded, 1); - PlatformMgr().ScheduleWork(DriveBLEState, 0); } #endif + PlatformMgr().ScheduleWork(DriveBLEState, 0); } CHIP_ERROR BLEManagerImpl::_SetAdvertisingMode(BLEAdvertisingMode mode) @@ -513,6 +499,10 @@ bool BLEManagerImpl::SubscribeCharacteristic(BLE_CONNECTION_OBJECT conId, const uint8_t value[2]; int rc; struct peer * peer = peer_find(conId); + if (peer == nullptr) + { + return false; + } dsc = peer_dsc_find_uuid(peer, (ble_uuid_t *) (&ShortUUID_CHIPoBLEService), (ble_uuid_t *) (&UUID_CHIPoBLEChar_TX), (ble_uuid_t *) (&ShortUUID_CHIPoBLE_CharTx_Desc)); @@ -547,6 +537,10 @@ bool BLEManagerImpl::UnsubscribeCharacteristic(BLE_CONNECTION_OBJECT conId, cons uint8_t value[2]; int rc; struct peer * peer = peer_find(conId); + if (peer == nullptr) + { + return false; + } dsc = peer_dsc_find_uuid(peer, (ble_uuid_t *) (&ShortUUID_CHIPoBLEService), (ble_uuid_t *) (&UUID_CHIPoBLEChar_TX), (ble_uuid_t *) (&ShortUUID_CHIPoBLE_CharTx_Desc)); @@ -696,7 +690,11 @@ bool BLEManagerImpl::SendReadResponse(BLE_CONNECTION_OBJECT conId, BLE_READ_REQU return false; } -void BLEManagerImpl::NotifyChipConnectionClosed(BLE_CONNECTION_OBJECT conId) {} +void BLEManagerImpl::NotifyChipConnectionClosed(BLE_CONNECTION_OBJECT conId) +{ + ChipLogDetail(Ble, "Received notification of closed CHIPoBLE connection (con %u)", conId); + CloseConnection(conId); +} CHIP_ERROR BLEManagerImpl::MapBLEError(int bleErr) { @@ -729,26 +727,17 @@ CHIP_ERROR BLEManagerImpl::MapBLEError(int bleErr) } void BLEManagerImpl::CancelBleAdvTimeoutTimer(void) { - VerifyOrReturn(sbleAdvTimeoutTimer != nullptr); - - if (xTimerStop(sbleAdvTimeoutTimer, pdMS_TO_TICKS(0)) == pdFAIL) + if (SystemLayer().IsTimerActive(BleAdvTimeoutHandler, nullptr)) { - ChipLogError(DeviceLayer, "Failed to stop BledAdv timeout timer"); + SystemLayer().CancelTimer(BleAdvTimeoutHandler, nullptr); } } void BLEManagerImpl::StartBleAdvTimeoutTimer(uint32_t aTimeoutInMs) { - VerifyOrReturn(sbleAdvTimeoutTimer != nullptr); - - if (xTimerIsTimerActive(sbleAdvTimeoutTimer)) - { - CancelBleAdvTimeoutTimer(); - } + CancelBleAdvTimeoutTimer(); - // timer is not active, change its period to required value (== restart). - // FreeRTOS- Block for a maximum of 100 ticks if the change period command - // cannot immediately be sent to the timer command queue. - if (xTimerChangePeriod(sbleAdvTimeoutTimer, pdMS_TO_TICKS(aTimeoutInMs), pdMS_TO_TICKS(100)) != pdPASS) + CHIP_ERROR err = SystemLayer().StartTimer(System::Clock::Milliseconds32(aTimeoutInMs), BleAdvTimeoutHandler, nullptr); + if ((err != CHIP_NO_ERROR)) { ChipLogError(DeviceLayer, "Failed to start BledAdv timeout timer"); } @@ -843,23 +832,21 @@ void BLEManagerImpl::DriveBLEState(void) ExitNow(); } } - // mFlags.Clear(Flags::kAdvertisingRefreshNeeded); // Transition to the not Advertising state... - if (mFlags.Has(Flags::kAdvertising)) - { - mFlags.Clear(Flags::kAdvertising); - mFlags.Set(Flags::kFastAdvertisingEnabled, true); + mFlags.Clear(Flags::kAdvertising); + mFlags.Set(Flags::kFastAdvertisingEnabled, true); - ChipLogProgress(DeviceLayer, "CHIPoBLE advertising stopped"); + ChipLogProgress(DeviceLayer, "CHIPoBLE advertising stopped"); - // Post a CHIPoBLEAdvertisingChange(Stopped) event. - { - ChipDeviceEvent advChange; - advChange.Type = DeviceEventType::kCHIPoBLEAdvertisingChange; - advChange.CHIPoBLEAdvertisingChange.Result = kActivity_Stopped; - err = PlatformMgr().PostEvent(&advChange); - } + CancelBleAdvTimeoutTimer(); + + // Post a CHIPoBLEAdvertisingChange(Stopped) event. + { + ChipDeviceEvent advChange; + advChange.Type = DeviceEventType::kCHIPoBLEAdvertisingChange; + advChange.CHIPoBLEAdvertisingChange.Result = kActivity_Stopped; + err = PlatformMgr().PostEvent(&advChange); } ExitNow();