From f2cee6a481b88f0f691eff6f6b1dfeb79aa37251 Mon Sep 17 00:00:00 2001 From: h2zero Date: Fri, 24 Apr 2026 11:58:22 -0600 Subject: [PATCH 01/15] Replace task noitifcations with semaphores to avoid conflicts. This will prevent a case where applications may notify a task that is blocking on a BLE operation before it completes. This can occur when writing to a characteristic in the application and blocking with task notifications until a notification occurs that releases it and the notification arrives before the write response. --- Kconfig | 7 -- src/NimBLEClient.cpp | 22 +++--- src/NimBLEClient.h | 4 +- src/NimBLEDevice.cpp | 2 + src/NimBLEL2CAPChannel.cpp | 2 +- src/NimBLEL2CAPChannel.h | 5 +- src/NimBLERemoteCharacteristic.cpp | 8 +- src/NimBLERemoteService.cpp | 8 +- src/NimBLERemoteValueAttribute.cpp | 24 +++--- src/NimBLEScan.cpp | 2 +- src/NimBLEScan.h | 17 ++-- src/NimBLEUtils.cpp | 123 ++++++++++++++--------------- src/NimBLEUtils.h | 71 ++++++++++------- 13 files changed, 151 insertions(+), 144 deletions(-) diff --git a/Kconfig b/Kconfig index 590b37fe1..bfd47962a 100644 --- a/Kconfig +++ b/Kconfig @@ -197,11 +197,4 @@ config NIMBLE_CPP_DEBUG_ASSERT_ENABLED Enabling this option will add debug asserts to the NimBLE CPP library. This will use approximately 1kB of flash memory. -config NIMBLE_CPP_FREERTOS_TASK_BLOCK_BIT - int "FreeRTOS task block bit." - default 31 - help - Configure the bit to set in the task notification value when a task is blocked waiting for an event. - This should be set to a bit that is not used by other notifications in the system. - endmenu \ No newline at end of file diff --git a/src/NimBLEClient.cpp b/src/NimBLEClient.cpp index f17d0489c..2d219a73d 100644 --- a/src/NimBLEClient.cpp +++ b/src/NimBLEClient.cpp @@ -248,9 +248,9 @@ int NimBLEClient::startConnectionAttempt(const ble_addr_t* peerAddr) { */ bool NimBLEClient::connect(const NimBLEAddress& address, bool deleteAttributes, bool asyncConnect, bool exchangeMTU) { NIMBLE_LOGD(LOG_TAG, ">> connect(%s)", address.toString().c_str()); - NimBLETaskData taskData(this); - const ble_addr_t* peerAddr = address.getBase(); - int rc = 0; + NimBLEUtils::TaskData taskData(this); + const ble_addr_t* peerAddr = address.getBase(); + int rc = 0; if (!NimBLEDevice::m_synced) { NIMBLE_LOGE(LOG_TAG, "Host not synced with controller."); @@ -347,7 +347,7 @@ bool NimBLEClient::secureConnection(bool async) const { return true; } - NimBLETaskData taskData(const_cast(this), BLE_HS_ENOTCONN); + NimBLEUtils::TaskData taskData(const_cast(this), BLE_HS_ENOTCONN); m_pTaskData = &taskData; int retryCount = 1; do { @@ -769,8 +769,8 @@ bool NimBLEClient::retrieveServices(const NimBLEUUID* uuidFilter) { return false; } - int rc = 0; - NimBLETaskData taskData(this); + int rc = 0; + NimBLEUtils::TaskData taskData(this); if (uuidFilter == nullptr) { rc = ble_gattc_disc_all_svcs(m_connHandle, NimBLEClient::serviceDiscoveredCB, &taskData); @@ -809,8 +809,8 @@ int NimBLEClient::serviceDiscoveredCB(uint16_t connHandle, error->status, (error->status == 0) ? service->start_handle : -1); - NimBLETaskData* pTaskData = (NimBLETaskData*)arg; - NimBLEClient* pClient = (NimBLEClient*)pTaskData->m_pInstance; + NimBLEUtils::TaskData* pTaskData = (NimBLEUtils::TaskData*)arg; + NimBLEClient* pClient = (NimBLEClient*)pTaskData->m_pInstance; if (error->status == BLE_HS_ENOTCONN) { NIMBLE_LOGE(LOG_TAG, "<< Service Discovered; Disconnected"); @@ -1010,9 +1010,9 @@ void NimBLEClient::setConnectRetries(uint8_t numRetries) { * @param [in] arg A pointer to the client instance that registered for this callback. */ int NimBLEClient::handleGapEvent(struct ble_gap_event* event, void* arg) { - NimBLEClient* pClient = (NimBLEClient*)arg; - int rc = 0; - NimBLETaskData* pTaskData = pClient->m_pTaskData; // save a copy in case client is deleted + NimBLEClient* pClient = (NimBLEClient*)arg; + int rc = 0; + NimBLEUtils::TaskData* pTaskData = pClient->m_pTaskData; // save a copy in case client is deleted NIMBLE_LOGD(LOG_TAG, ">> handleGapEvent %s", NimBLEUtils::gapEventToString(event->type)); diff --git a/src/NimBLEClient.h b/src/NimBLEClient.h index 9b9b8538e..761f9e72d 100644 --- a/src/NimBLEClient.h +++ b/src/NimBLEClient.h @@ -28,6 +28,7 @@ # endif # include "NimBLEAddress.h" +# include "NimBLEUtils.h" # include # include @@ -41,7 +42,6 @@ class NimBLEAdvertisedDevice; class NimBLEAttValue; class NimBLEClientCallbacks; class NimBLEConnInfo; -struct NimBLETaskData; /** * @brief A model of a BLE client. @@ -155,7 +155,7 @@ class NimBLEClient { NimBLEAddress m_peerAddress; mutable int m_lastErr; int32_t m_connectTimeout; - mutable NimBLETaskData* m_pTaskData; + mutable NimBLEUtils::TaskData* m_pTaskData; std::vector m_svcVec; NimBLEClientCallbacks* m_pClientCallbacks; uint16_t m_connHandle; diff --git a/src/NimBLEDevice.cpp b/src/NimBLEDevice.cpp index 899242fc2..06cb78651 100644 --- a/src/NimBLEDevice.cpp +++ b/src/NimBLEDevice.cpp @@ -1071,6 +1071,8 @@ bool NimBLEDevice::deinit(bool clearAll) { deleteClient(clt); } # endif + + NimBLEUtils::deleteTaskSems(); } return rc == 0; diff --git a/src/NimBLEL2CAPChannel.cpp b/src/NimBLEL2CAPChannel.cpp index 2eff6a827..36b740ad3 100644 --- a/src/NimBLEL2CAPChannel.cpp +++ b/src/NimBLEL2CAPChannel.cpp @@ -88,7 +88,7 @@ int NimBLEL2CAPChannel::writeFragment(std::vector::const_iterator begin if (stalled) { NIMBLE_LOGD(LOG_TAG, "L2CAP Channel waiting for unstall..."); - NimBLETaskData taskData; + NimBLEUtils::TaskData taskData; m_pTaskData = &taskData; NimBLEUtils::taskWait(taskData, BLE_NPL_TIME_FOREVER); m_pTaskData = nullptr; diff --git a/src/NimBLEL2CAPChannel.h b/src/NimBLEL2CAPChannel.h index 3c1c95072..0e196bdbc 100644 --- a/src/NimBLEL2CAPChannel.h +++ b/src/NimBLEL2CAPChannel.h @@ -22,12 +22,13 @@ # undef max /**************************/ +# include "NimBLEUtils.h" + # include # include class NimBLEClient; class NimBLEL2CAPChannelCallbacks; -struct NimBLETaskData; /** * @brief Encapsulates a L2CAP channel. @@ -94,7 +95,7 @@ class NimBLEL2CAPChannel { // Runtime handling std::atomic stalled{false}; - NimBLETaskData* m_pTaskData{nullptr}; + NimBLEUtils::TaskData* m_pTaskData{nullptr}; // Allocate / deallocate NimBLE memory pool bool setupMemPool(); diff --git a/src/NimBLERemoteCharacteristic.cpp b/src/NimBLERemoteCharacteristic.cpp index 64a1f3083..a8d2801bc 100644 --- a/src/NimBLERemoteCharacteristic.cpp +++ b/src/NimBLERemoteCharacteristic.cpp @@ -66,7 +66,7 @@ int NimBLERemoteCharacteristic::descriptorDiscCB( uint16_t connHandle, const ble_gatt_error* error, uint16_t chrHandle, const ble_gatt_dsc* dsc, void* arg) { int rc = error->status; auto filter = (NimBLEDescriptorFilter*)arg; - auto pTaskData = (NimBLETaskData*)filter->taskData; + auto pTaskData = (NimBLEUtils::TaskData*)filter->taskData; const auto pChr = (NimBLERemoteCharacteristic*)pTaskData->m_pInstance; const auto uuid = filter->uuid; // UUID to filter for NIMBLE_LOGD(LOG_TAG, "Descriptor Discovery >> status: %d handle: %d", rc, (rc == 0) ? dsc->handle : -1); @@ -116,7 +116,7 @@ bool NimBLERemoteCharacteristic::retrieveDescriptors(NimBLEDescriptorFilter* pFi return true; } - NimBLETaskData taskData(const_cast(this)); + NimBLEUtils::TaskData taskData(const_cast(this)); NimBLEDescriptorFilter defaultFilter{nullptr, nullptr, &taskData}; if (pFilter == nullptr) { pFilter = &defaultFilter; @@ -134,7 +134,7 @@ bool NimBLERemoteCharacteristic::retrieveDescriptors(NimBLEDescriptorFilter* pFi auto prevDscCount = m_vDescriptors.size(); NimBLEUtils::taskWait(taskData, BLE_NPL_TIME_FOREVER); - rc = ((NimBLETaskData*)pFilter->taskData)->m_flags; + rc = ((NimBLEUtils::TaskData*)pFilter->taskData)->m_flags; if (rc != BLE_HS_EDONE) { NIMBLE_LOGE(LOG_TAG, "<< retrieveDescriptors(): failed: rc=%d %s", rc, NimBLEUtils::returnCodeToString(rc)); return false; @@ -156,7 +156,7 @@ bool NimBLERemoteCharacteristic::retrieveDescriptors(NimBLEDescriptorFilter* pFi NimBLERemoteDescriptor* NimBLERemoteCharacteristic::getDescriptor(const NimBLEUUID& uuid) const { NIMBLE_LOGD(LOG_TAG, ">> getDescriptor: uuid: %s", uuid.toString().c_str()); NimBLEUUID uuidTmp{uuid}; - NimBLETaskData taskData(const_cast(this)); + NimBLEUtils::TaskData taskData(const_cast(this)); NimBLEDescriptorFilter filter{nullptr, &uuidTmp, &taskData}; for (const auto& dsc : m_vDescriptors) { diff --git a/src/NimBLERemoteService.cpp b/src/NimBLERemoteService.cpp index f15a9a31e..f5831a410 100644 --- a/src/NimBLERemoteService.cpp +++ b/src/NimBLERemoteService.cpp @@ -75,7 +75,7 @@ NimBLERemoteCharacteristic* NimBLERemoteService::getCharacteristic(const char* u */ NimBLERemoteCharacteristic* NimBLERemoteService::getCharacteristic(const NimBLEUUID& uuid) const { NIMBLE_LOGD(LOG_TAG, ">> getCharacteristic: uuid: %s", uuid.toString().c_str()); - NimBLERemoteCharacteristic* pChar = nullptr; + NimBLERemoteCharacteristic* pChar = nullptr; for (const auto& it : m_vChars) { if (it->getUUID() == uuid) { @@ -136,7 +136,7 @@ int NimBLERemoteService::characteristicDiscCB(uint16_t conn_handle, "Characteristic Discovery >> status: %d handle: %d", error->status, (error->status == 0) ? chr->def_handle : -1); - auto pTaskData = (NimBLETaskData*)arg; + auto pTaskData = (NimBLEUtils::TaskData*)arg; const auto pSvc = (NimBLERemoteService*)pTaskData->m_pInstance; if (error->status == BLE_HS_ENOTCONN) { @@ -178,8 +178,8 @@ int NimBLERemoteService::characteristicDiscCB(uint16_t conn_handle, */ bool NimBLERemoteService::retrieveCharacteristics(const NimBLEUUID* uuidFilter, NimBLERemoteCharacteristic** ppChar) const { NIMBLE_LOGD(LOG_TAG, ">> retrieveCharacteristics()"); - int rc = 0; - NimBLETaskData taskData(const_cast(this)); + int rc = 0; + NimBLEUtils::TaskData taskData(const_cast(this)); if (uuidFilter == nullptr) { rc = ble_gattc_disc_all_chrs(m_pClient->getConnHandle(), diff --git a/src/NimBLERemoteValueAttribute.cpp b/src/NimBLERemoteValueAttribute.cpp index f7021145c..cb270d077 100644 --- a/src/NimBLERemoteValueAttribute.cpp +++ b/src/NimBLERemoteValueAttribute.cpp @@ -29,11 +29,11 @@ static const char* LOG_TAG = "NimBLERemoteValueAttribute"; bool NimBLERemoteValueAttribute::writeValue(const uint8_t* data, size_t length, bool response) const { NIMBLE_LOGD(LOG_TAG, ">> writeValue()"); - const NimBLEClient* pClient = getClient(); - int retryCount = 1; - int rc = 0; - uint16_t mtu = pClient->getMTU() - 3; - NimBLETaskData taskData(const_cast(this)); + const NimBLEClient* pClient = getClient(); + int retryCount = 1; + int rc = 0; + uint16_t mtu = pClient->getMTU() - 3; + NimBLEUtils::TaskData taskData(const_cast(this)); // Check if the data length is longer than we can write in one connection event. // If so we must do a long write which requires a response. @@ -98,7 +98,7 @@ bool NimBLERemoteValueAttribute::writeValue(const uint8_t* data, size_t length, * @return success == 0 or error code. */ int NimBLERemoteValueAttribute::onWriteCB(uint16_t conn_handle, const ble_gatt_error* error, ble_gatt_attr* attr, void* arg) { - auto pTaskData = static_cast(arg); + auto pTaskData = static_cast(arg); const auto pAtt = static_cast(pTaskData->m_pInstance); if (error->status == BLE_HS_ENOTCONN) { @@ -124,11 +124,11 @@ int NimBLERemoteValueAttribute::onWriteCB(uint16_t conn_handle, const ble_gatt_e NimBLEAttValue NimBLERemoteValueAttribute::readValue(time_t* timestamp) { NIMBLE_LOGD(LOG_TAG, ">> readValue()"); - NimBLEAttValue value{}; - const NimBLEClient* pClient = getClient(); - int rc = 0; - int retryCount = 1; - NimBLETaskData taskData(const_cast(this), 0, &value); + NimBLEAttValue value{}; + const NimBLEClient* pClient = getClient(); + int rc = 0; + int retryCount = 1; + NimBLEUtils::TaskData taskData(const_cast(this), 0, &value); do { rc = ble_gattc_read_long(pClient->getConnHandle(), getHandle(), 0, NimBLERemoteValueAttribute::onReadCB, &taskData); @@ -183,7 +183,7 @@ NimBLEAttValue NimBLERemoteValueAttribute::readValue(time_t* timestamp) { * @return success == 0 or error code. */ int NimBLERemoteValueAttribute::onReadCB(uint16_t conn_handle, const ble_gatt_error* error, ble_gatt_attr* attr, void* arg) { - auto pTaskData = static_cast(arg); + auto pTaskData = static_cast(arg); const auto pAtt = static_cast(pTaskData->m_pInstance); if (error->status == BLE_HS_ENOTCONN) { diff --git a/src/NimBLEScan.cpp b/src/NimBLEScan.cpp index 540280758..90a381aa2 100644 --- a/src/NimBLEScan.cpp +++ b/src/NimBLEScan.cpp @@ -689,7 +689,7 @@ NimBLEScanResults NimBLEScan::getResults(uint32_t duration, bool is_continue) { return m_scanResults; } - NimBLETaskData taskData; + NimBLEUtils::TaskData taskData; m_pTaskData = &taskData; if (start(duration, is_continue)) { diff --git a/src/NimBLEScan.h b/src/NimBLEScan.h index 2b8a24996..0c612851d 100644 --- a/src/NimBLEScan.h +++ b/src/NimBLEScan.h @@ -129,11 +129,16 @@ class NimBLEScan { snprintf(&out[0], out.size(), "Scan stats:\n" - " Devices seen : %" PRIu32 "\n" - " Duplicate advs : %" PRIu32 "\n" - " Scan responses : %" PRIu32 "\n" - " SR timing (ms) : min=%" PRIu32 ", max=%" PRIu32 ", avg=%" PRIu64 "\n" - " Orphaned SR : %" PRIu32 "\n" + " Devices seen : %" PRIu32 + "\n" + " Duplicate advs : %" PRIu32 + "\n" + " Scan responses : %" PRIu32 + "\n" + " SR timing (ms) : min=%" PRIu32 ", max=%" PRIu32 ", avg=%" PRIu64 + "\n" + " Orphaned SR : %" PRIu32 + "\n" " Missed SR : %" PRIu32 "\n", devCount, dupCount, @@ -187,7 +192,7 @@ class NimBLEScan { NimBLEScanCallbacks* m_pScanCallbacks; ble_gap_disc_params m_scanParams; NimBLEScanResults m_scanResults; - NimBLETaskData* m_pTaskData; + NimBLEUtils::TaskData* m_pTaskData; ble_npl_callout m_srTimer{}; ble_npl_time_t m_srTimeoutTicks{}; uint8_t m_maxResults; diff --git a/src/NimBLEUtils.cpp b/src/NimBLEUtils.cpp index 75969a59e..ec9767af0 100644 --- a/src/NimBLEUtils.cpp +++ b/src/NimBLEUtils.cpp @@ -21,9 +21,9 @@ # include "NimBLEAddress.h" # include "NimBLELog.h" -#ifdef USING_NIMBLE_ARDUINO_HEADERS +# ifdef USING_NIMBLE_ARDUINO_HEADERS # include "nimble/nimble/host/include/host/ble_hs.h" -#else +# else # include "host/ble_hs.h" # endif @@ -59,57 +59,18 @@ # endif # endif -# if defined INC_FREERTOS_H -# ifndef MYNEWT_VAL_NIMBLE_CPP_FREERTOS_TASK_BLOCK_BIT -# ifndef CONFIG_NIMBLE_CPP_FREERTOS_TASK_BLOCK_BIT -# define MYNEWT_VAL_NIMBLE_CPP_FREERTOS_TASK_BLOCK_BIT 31 -# else -# define MYNEWT_VAL_NIMBLE_CPP_FREERTOS_TASK_BLOCK_BIT CONFIG_NIMBLE_CPP_FREERTOS_TASK_BLOCK_BIT -# endif -# endif -# endif +static const char* LOG_TAG = "NimBLEUtils"; -constexpr uint32_t TASK_BLOCK_BIT = (1 << MYNEWT_VAL(NIMBLE_CPP_FREERTOS_TASK_BLOCK_BIT)); -static const char* LOG_TAG = "NimBLEUtils"; +std::vector NimBLEUtils::m_taskSemEntries{}; /** - * @brief Construct a NimBLETaskData instance. + * @brief Construct a NimBLEUtils::TaskData instance. * @param [in] pInstance An instance of the class that will be waiting. * @param [in] flags General purpose flags for the caller. * @param [in] buf A buffer for data. */ -NimBLETaskData::NimBLETaskData(void* pInstance, int flags, void* buf) - : m_pInstance{pInstance}, - m_flags{flags}, - m_pBuf{buf} -# ifdef INC_FREERTOS_H - , - m_pHandle{xTaskGetCurrentTaskHandle()} { -} -# else -{ - ble_npl_sem* sem = new ble_npl_sem; - if (ble_npl_sem_init(sem, 0) != BLE_NPL_OK) { - NIMBLE_LOGE(LOG_TAG, "Failed to init semaphore"); - delete sem; - m_pHandle = nullptr; - } else { - m_pHandle = sem; - } -} -# endif - -/** - * @brief Destructor. - */ -NimBLETaskData::~NimBLETaskData() { -# ifndef INC_FREERTOS_H - if (m_pHandle != nullptr) { - ble_npl_sem_deinit(static_cast(m_pHandle)); - delete static_cast(m_pHandle); - } -# endif -} +NimBLEUtils::TaskData::TaskData(void* pInstance, int flags, void* buf) + : m_pInstance{pInstance}, m_flags{flags}, m_pBuf{buf} {} /** * @brief Blocks the calling task until released or timeout. @@ -117,7 +78,7 @@ NimBLETaskData::~NimBLETaskData() { * @param [in] timeout The time to wait in milliseconds. * @return True if the task completed, false if the timeout was reached. */ -bool NimBLEUtils::taskWait(const NimBLETaskData& taskData, uint32_t timeout) { +bool NimBLEUtils::taskWait(const TaskData& taskData, uint32_t timeout) { ble_npl_time_t ticks; if (timeout == BLE_NPL_TIME_FOREVER) { ticks = BLE_NPL_TIME_FOREVER; @@ -125,18 +86,33 @@ bool NimBLEUtils::taskWait(const NimBLETaskData& taskData, uint32_t timeout) { ble_npl_time_ms_to_ticks(timeout, &ticks); } -# ifdef INC_FREERTOS_H - uint32_t notificationValue; - xTaskNotifyWait(0, TASK_BLOCK_BIT, ¬ificationValue, 0); - if (notificationValue & TASK_BLOCK_BIT) { - return true; + // find a semaphore that is not currently in use, or create a new one + for (auto& entry : m_taskSemEntries) { + if (!entry->inUse) { + taskData.m_pSem = entry; + break; + } } - return xTaskNotifyWait(0, TASK_BLOCK_BIT, nullptr, ticks) == pdTRUE; + if (taskData.m_pSem == nullptr) { + auto* sem = new ble_npl_sem; + if (ble_npl_sem_init(sem, 0) != BLE_NPL_OK) { + NIMBLE_LOGE(LOG_TAG, "Failed to initialize semaphore for taskWait"); + delete sem; + return false; + } -# else - return ble_npl_sem_pend(static_cast(taskData.m_pHandle), ticks) == BLE_NPL_OK; -# endif + TaskSemEntry* entry = new TaskSemEntry; + entry->sem = sem; + entry->inUse = true; + m_taskSemEntries.push_back(entry); + taskData.m_pSem = entry; + NIMBLE_LOGD(LOG_TAG, "Created new semaphore for taskWait, total semaphores: %d\n", (int)m_taskSemEntries.size()); + } + + NIMBLE_LOGD(LOG_TAG, "Task waiting with timeout %" PRIu32 "ms", timeout); + taskData.m_pSem->inUse = true; + return ble_npl_sem_pend(taskData.m_pSem->sem, ticks) == BLE_NPL_OK; } // taskWait /** @@ -144,17 +120,36 @@ bool NimBLEUtils::taskWait(const NimBLETaskData& taskData, uint32_t timeout) { * @param [in] taskData A pointer to the task data structure. * @param [in] flags A return value to set in the task data structure. */ -void NimBLEUtils::taskRelease(const NimBLETaskData& taskData, int flags) { +void NimBLEUtils::taskRelease(const TaskData& taskData, int flags) { taskData.m_flags = flags; - if (taskData.m_pHandle != nullptr) { -# ifdef INC_FREERTOS_H - xTaskNotify(static_cast(taskData.m_pHandle), TASK_BLOCK_BIT, eSetBits); -# else - ble_npl_sem_release(static_cast(taskData.m_pHandle)); -# endif + if (taskData.m_pSem == nullptr) { + NIMBLE_LOGE(LOG_TAG, "taskRelease called with null semaphore"); + return; + } + + taskData.m_pSem->inUse = false; + auto rc = ble_npl_sem_release(taskData.m_pSem->sem); + if (rc != BLE_NPL_OK) { + NIMBLE_LOGE(LOG_TAG, "Failed to release semaphore: rc=%d %s", rc, returnCodeToString(rc)); + return; } } // taskRelease +/** + * @brief Deletes all semaphores used for task waiting. + * @details This should be called when the NimBLE stack is deinitialized to clean up any resources used by waiting tasks. + */ +void NimBLEUtils::deleteTaskSems() { + for (auto& entry : m_taskSemEntries) { + ble_npl_sem_release(entry->sem); + ble_npl_time_delay(10); // give time for any pending tasks to be released + ble_npl_sem_deinit(entry->sem); + delete entry->sem; + delete entry; + } + m_taskSemEntries.clear(); +} + /** * @brief Converts a return code from the NimBLE stack to a text string. * @param [in] rc The return code to convert. @@ -563,7 +558,7 @@ const char* NimBLEUtils::gapEventToString(uint8_t eventType) { NIMBLE_LOGD(LOG_TAG, "Unknown event type %d 0x%.2x", eventType, eventType); return "Unknown event type"; } -# else // MYNEWT_VAL(NIMBLE_CPP_ENABLE_GAP_EVENT_CODE_TEXT) +# else // MYNEWT_VAL(NIMBLE_CPP_ENABLE_GAP_EVENT_CODE_TEXT) (void)eventType; return ""; # endif // MYNEWT_VAL(NIMBLE_CPP_ENABLE_GAP_EVENT_CODE_TEXT) diff --git a/src/NimBLEUtils.h b/src/NimBLEUtils.h index 63b7f1996..fafb02c19 100644 --- a/src/NimBLEUtils.h +++ b/src/NimBLEUtils.h @@ -29,51 +29,62 @@ # endif # endif -#if MYNEWT_VAL(NIMBLE_CPP_DEBUG_ASSERT_ENABLED) && !defined NDEBUG -void nimble_cpp_assert(const char *file, unsigned line) __attribute((weak, noreturn)); -# define NIMBLE_ATT_VAL_FILE (__builtin_strrchr(__FILE__, '/') ? \ - __builtin_strrchr (__FILE__, '/') + 1 : __FILE__) -# define NIMBLE_CPP_DEBUG_ASSERT(cond) \ - if (!(cond)) { \ - nimble_cpp_assert(NIMBLE_ATT_VAL_FILE, __LINE__); \ - } -#else -# define NIMBLE_CPP_DEBUG_ASSERT(cond) (void(0)) -#endif +# if MYNEWT_VAL(NIMBLE_CPP_DEBUG_ASSERT_ENABLED) && !defined NDEBUG +void nimble_cpp_assert(const char* file, unsigned line) __attribute((weak, noreturn)); +# define NIMBLE_ATT_VAL_FILE (__builtin_strrchr(__FILE__, '/') ? __builtin_strrchr(__FILE__, '/') + 1 : __FILE__) +# define NIMBLE_CPP_DEBUG_ASSERT(cond) \ + if (!(cond)) { \ + nimble_cpp_assert(NIMBLE_ATT_VAL_FILE, __LINE__); \ + } +# else +# define NIMBLE_CPP_DEBUG_ASSERT(cond) (void(0)) +# endif # include +# include class NimBLEAddress; - -/** - * @brief A structure to hold data for a task that is waiting for a response. - * @details This structure is used in conjunction with NimBLEUtils::taskWait() and NimBLEUtils::taskRelease(). - * All items are optional, the m_pHandle will be set in taskWait(). - */ -struct NimBLETaskData { - NimBLETaskData(void* pInstance = nullptr, int flags = 0, void* buf = nullptr); - ~NimBLETaskData(); - void* m_pInstance{nullptr}; - mutable int m_flags{0}; - void* m_pBuf{nullptr}; - - private: - mutable void* m_pHandle{nullptr}; // semaphore or task handle - friend class NimBLEUtils; -}; +struct ble_npl_sem; /** * @brief A BLE Utility class with methods for debugging and general purpose use. */ class NimBLEUtils { + struct TaskSemEntry { + ble_npl_sem* sem; + bool inUse{false}; + }; + public: + /** + * @brief A structure to hold data for a task that is waiting for a response. + * @details This structure is used in conjunction with NimBLEUtils::taskWait() and NimBLEUtils::taskRelease(). + * All items are optional; the internal semaphore entry is assigned in taskWait(). + */ + struct TaskData { + TaskData(void* pInstance = nullptr, int flags = 0, void* buf = nullptr); + ~TaskData() = default; + void* m_pInstance{nullptr}; + mutable int m_flags{0}; + void* m_pBuf{nullptr}; + + private: + friend class NimBLEUtils; + mutable TaskSemEntry* m_pSem{nullptr}; + }; + static const char* gapEventToString(uint8_t eventType); static std::string dataToHexString(const uint8_t* source, uint8_t length); static const char* advTypeToString(uint8_t advType); static const char* returnCodeToString(int rc); static NimBLEAddress generateAddr(bool nrpa); - static bool taskWait(const NimBLETaskData& taskData, uint32_t timeout); - static void taskRelease(const NimBLETaskData& taskData, int rc = 0); + static bool taskWait(const TaskData& taskData, uint32_t timeout); + static void taskRelease(const TaskData& taskData, int rc = 0); + + private: + friend class NimBLEDevice; + static void deleteTaskSems(); + static std::vector m_taskSemEntries; }; #endif // CONFIG_BT_NIMBLE_ENABLED From d2fa1ff5f0fe3ec179b5d0a9b2a97f434214f47a Mon Sep 17 00:00:00 2001 From: h2zero Date: Fri, 24 Apr 2026 13:28:45 -0600 Subject: [PATCH 02/15] [Bugfix] Use the correct taskData when retrieving descriptors --- src/NimBLERemoteCharacteristic.cpp | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/NimBLERemoteCharacteristic.cpp b/src/NimBLERemoteCharacteristic.cpp index a8d2801bc..300237999 100644 --- a/src/NimBLERemoteCharacteristic.cpp +++ b/src/NimBLERemoteCharacteristic.cpp @@ -26,10 +26,13 @@ # include +// Private structure used to filter descriptors during discovery and return results. struct NimBLEDescriptorFilter { - NimBLERemoteDescriptor* dsc; + NimBLEDescriptorFilter() = delete; + NimBLEDescriptorFilter(const NimBLEUUID* uuid, NimBLEUtils::TaskData* td = nullptr) : uuid(uuid), taskData(td) {} const NimBLEUUID* uuid; - void* taskData; + NimBLEUtils::TaskData* taskData; + NimBLERemoteDescriptor* dsc{nullptr}; }; static const char* LOG_TAG = "NimBLERemoteCharacteristic"; @@ -117,9 +120,11 @@ bool NimBLERemoteCharacteristic::retrieveDescriptors(NimBLEDescriptorFilter* pFi } NimBLEUtils::TaskData taskData(const_cast(this)); - NimBLEDescriptorFilter defaultFilter{nullptr, nullptr, &taskData}; + NimBLEDescriptorFilter defaultFilter(nullptr, &taskData); if (pFilter == nullptr) { pFilter = &defaultFilter; + } else { + pFilter->taskData = &taskData; } int rc = ble_gattc_disc_all_dscs(getClient()->getConnHandle(), @@ -134,7 +139,7 @@ bool NimBLERemoteCharacteristic::retrieveDescriptors(NimBLEDescriptorFilter* pFi auto prevDscCount = m_vDescriptors.size(); NimBLEUtils::taskWait(taskData, BLE_NPL_TIME_FOREVER); - rc = ((NimBLEUtils::TaskData*)pFilter->taskData)->m_flags; + rc = taskData.m_flags; if (rc != BLE_HS_EDONE) { NIMBLE_LOGE(LOG_TAG, "<< retrieveDescriptors(): failed: rc=%d %s", rc, NimBLEUtils::returnCodeToString(rc)); return false; @@ -156,8 +161,7 @@ bool NimBLERemoteCharacteristic::retrieveDescriptors(NimBLEDescriptorFilter* pFi NimBLERemoteDescriptor* NimBLERemoteCharacteristic::getDescriptor(const NimBLEUUID& uuid) const { NIMBLE_LOGD(LOG_TAG, ">> getDescriptor: uuid: %s", uuid.toString().c_str()); NimBLEUUID uuidTmp{uuid}; - NimBLEUtils::TaskData taskData(const_cast(this)); - NimBLEDescriptorFilter filter{nullptr, &uuidTmp, &taskData}; + NimBLEDescriptorFilter filter(&uuidTmp); for (const auto& dsc : m_vDescriptors) { if (dsc->getUUID() == uuid) { From 803fda50fdcecb0ef849390654e9562150434900 Mon Sep 17 00:00:00 2001 From: Chris Morgan Date: Thu, 18 Jun 2026 21:52:46 -0400 Subject: [PATCH 03/15] Restore GAP device name after GATT reset ESP-IDF NimBLE can reset the GAP Device Name when the standard GAP service is reinitialized during GATT server startup. NimBLEDevice::init(deviceName) calls NimBLEDevice::setDeviceName(deviceName), but that happens before the server is started. Later, starting advertising starts the server and resets/re-registers the GAP service, causing the C GAP name backing storage to be rebuilt from CONFIG_BT_NIMBLE_SVC_GAP_DEVICE_NAME. In projects where that Kconfig default remains "nimble", the advertised Local Name can be correct while the Generic Access Device Name characteristic reports "nimble". The failing sequence is: -> pAdvertising->start() -> NimBLEAdvertising::start() -> NimBLEDevice::getServer()->start() -> NimBLEServer::resetGATT() -> ble_gatts_reset() -> ble_svc_gap_init() -> ESP-IDF NimBLE resets GAP name to CONFIG_BT_NIMBLE_SVC_GAP_DEVICE_NAME -> "nimble" -> ble_svc_gatt_init() Fix this by saving the device name supplied through NimBLEDevice::setDeviceName() and reapplying it immediately after ble_svc_gap_init() in NimBLEServer::resetGATT(). This preserves the C++ wrapper contract that the configured device name remains effective even when the wrapper internally resets the GATT/GAP service tables. --- src/NimBLEServer.cpp | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/NimBLEServer.cpp b/src/NimBLEServer.cpp index 1cab58ed9..425797911 100644 --- a/src/NimBLEServer.cpp +++ b/src/NimBLEServer.cpp @@ -889,8 +889,21 @@ bool NimBLEServer::resetGATT() { NimBLEDevice::stopAdvertising(); # endif + // esp-idf nimble clears the device name and + // appearance during ble_gatts_reset() +#ifndef CONFIG_USING_NIMBLE_COMPONENT + std::string name(ble_svc_gap_device_name()); + uint16_t appearance = ble_svc_gap_device_appearance(); +#endif + ble_gatts_reset(); ble_svc_gap_init(); + +#ifndef CONFIG_USING_NIMBLE_COMPONENT + ble_svc_gap_device_name_set(name.c_str()); + ble_svc_gap_device_appearance_set(appearance); +#endif + ble_svc_gatt_init(); for (auto svcIt = m_svcVec.begin(); svcIt != m_svcVec.end();) { From 4d8413c40bd0700da05fa8f34dc4527bb337f950 Mon Sep 17 00:00:00 2001 From: Chris Morgan Date: Sun, 5 Jul 2026 21:30:17 -0400 Subject: [PATCH 04/15] NimBLEAttValue.cpp - preserve capacity after realloc failure NimBLEAttValue::append advanced m_capacity immediately after calling realloc. If realloc failed, the original buffer remained allocated but the object advertised a larger capacity than it actually owned. A later append could then skip reallocation and copy past the end of the allocation. Track whether the append actually needs growth and only update m_capacity after the realloc result passes the null check. Appends that fit the existing allocation leave capacity unchanged; failed growth preserves the old capacity invariant. --- src/NimBLEAttValue.cpp | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/NimBLEAttValue.cpp b/src/NimBLEAttValue.cpp index 8dbe4866a..be7695957 100644 --- a/src/NimBLEAttValue.cpp +++ b/src/NimBLEAttValue.cpp @@ -124,9 +124,9 @@ NimBLEAttValue& NimBLEAttValue::append(const uint8_t* value, uint16_t len) { uint8_t* res = m_attr_value; uint16_t new_len = m_attr_len + len; - if (new_len > m_capacity) { - res = static_cast(realloc(m_attr_value, (new_len + 1))); - m_capacity = new_len; + bool grow = new_len > m_capacity; + if (grow) { + res = static_cast(realloc(m_attr_value, (new_len + 1))); } NIMBLE_CPP_DEBUG_ASSERT(res); if (res == nullptr) { @@ -134,6 +134,10 @@ NimBLEAttValue& NimBLEAttValue::append(const uint8_t* value, uint16_t len) { return *this; } + if (grow) { + m_capacity = new_len; + } + # if MYNEWT_VAL(NIMBLE_CPP_ATT_VALUE_TIMESTAMP_ENABLED) time_t t = time(nullptr); # else From 497cc89132b155d311115cedc11c0bdc8cdce111 Mon Sep 17 00:00:00 2001 From: yi chen <87560781+94xhn@users.noreply.github.com> Date: Sat, 18 Jul 2026 22:35:31 +0800 Subject: [PATCH 05/15] Fix NimBLEEddystoneTLM byte-order mismatch between getters and setters (#431) * Fix NimBLEEddystoneTLM byte-order mismatch between getters and setters m_eddystoneData must always hold Eddystone-TLM wire-format (big-endian) bytes, since getData() returns it directly for the caller to memcpy straight into the BLE advertisement payload (see the BLE_EddystoneTLM_Beacon example), and setData()/getData() already treat it as raw wire bytes. The getters (getVolt/getTemp) correctly un-swap on read, but the setters (setVolt/setTemp/setCount/setTime) stored their host-order input directly with no swap, and the BeaconData default member initializers were host-order values instead of pre-swapped wire-format values. This breaks the setter/getter round trip for any value set locally (as opposed to arriving over the air via setData()), and means a freshly constructed object reports a garbage battery voltage. toString() also failed to un-swap the temperature field at all, and used unsigned division/modulo on a signed 8.8 fixed-point value, producing garbage output for negative temperatures. This mirrors the existing, correct pattern already used by the sibling NimBLEBeacon class, which swaps in its setters (setMajor/setMinor/ setManufacturerId). Fix: - setVolt/setTemp/setCount/setTime now apply ENDIAN_CHANGE_U16/U32 to their input before storing, matching what the getters already assume. - BeaconData's default volt/temp member initializers are now the wire-format (byte-swapped) encoding of the intended defaults (3300 mV, 23.0C), so a freshly constructed object reports correct values without calling a setter first. - toString() now swaps the temperature field the same way it already swaps voltage, and computes the displayed magnitude from a signed value with explicit sign handling instead of unsigned arithmetic on the raw field. --- src/NimBLEEddystoneTLM.cpp | 18 ++++++++++-------- src/NimBLEEddystoneTLM.h | 4 ++-- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/NimBLEEddystoneTLM.cpp b/src/NimBLEEddystoneTLM.cpp index 8ee405b5f..4cfd74987 100644 --- a/src/NimBLEEddystoneTLM.cpp +++ b/src/NimBLEEddystoneTLM.cpp @@ -101,10 +101,12 @@ std::string NimBLEEddystoneTLM::toString() { out += val; out += " mV\n"; - out += "Temperature "; - uint8_t intTemp = m_eddystoneData.temp / 256; - uint8_t frac = m_eddystoneData.temp % 256 * 100 / 256; - snprintf(val, sizeof(val), "%d.%d", intTemp, frac); + out += "Temperature "; + int16_t temp = ENDIAN_CHANGE_U16(m_eddystoneData.temp); + uint32_t absTemp = temp < 0 ? -static_cast(temp) : temp; + uint8_t intTemp = absTemp / 256; + uint8_t frac = absTemp % 256 * 100 / 256; + snprintf(val, sizeof(val), "%s%d.%02d", (temp < 0 ? "-" : ""), intTemp, frac); out += val; out += " C\n"; @@ -188,7 +190,7 @@ void NimBLEEddystoneTLM::setVersion(uint8_t version) { * @param [in] volt The voltage in millivolts. */ void NimBLEEddystoneTLM::setVolt(uint16_t volt) { - m_eddystoneData.volt = volt; + m_eddystoneData.volt = ENDIAN_CHANGE_U16(volt); } // setVolt /** @@ -196,7 +198,7 @@ void NimBLEEddystoneTLM::setVolt(uint16_t volt) { * @param [in] temp The temperature value in 8.8 fixed point format. */ void NimBLEEddystoneTLM::setTemp(int16_t temp) { - m_eddystoneData.temp = temp; + m_eddystoneData.temp = ENDIAN_CHANGE_U16(temp); } // setTemp /** @@ -204,7 +206,7 @@ void NimBLEEddystoneTLM::setTemp(int16_t temp) { * @param [in] advCount The advertisement number. */ void NimBLEEddystoneTLM::setCount(uint32_t advCount) { - m_eddystoneData.advCount = advCount; + m_eddystoneData.advCount = ENDIAN_CHANGE_U32(advCount); } // setCount /** @@ -212,7 +214,7 @@ void NimBLEEddystoneTLM::setCount(uint32_t advCount) { * @param [in] tmil The advertisement time in milliseconds. */ void NimBLEEddystoneTLM::setTime(uint32_t tmil) { - m_eddystoneData.tmil = tmil; + m_eddystoneData.tmil = ENDIAN_CHANGE_U32(tmil); } // setTime #endif // CONFIG_BT_NIMBLE_ENABLED && MYNEWT_VAL(BLE_ROLE_BROADCASTER) diff --git a/src/NimBLEEddystoneTLM.h b/src/NimBLEEddystoneTLM.h index f48cf94c0..fdfcb8415 100644 --- a/src/NimBLEEddystoneTLM.h +++ b/src/NimBLEEddystoneTLM.h @@ -37,8 +37,8 @@ class NimBLEEddystoneTLM { struct BeaconData { uint8_t frameType{EDDYSTONE_TLM_FRAME_TYPE}; uint8_t version{0}; - uint16_t volt{3300}; - uint16_t temp{23 * 256}; + uint16_t volt{0xE40C}; // Big-endian (wire format) encoding of 3300 mV; getVolt() swaps back to host order. + uint16_t temp{0x0017}; // Big-endian (wire format) encoding of 23.0C (23 * 256); getTemp() swaps back to host order. uint32_t advCount{0}; uint32_t tmil{0}; } __attribute__((packed)); From 205f293da9bc2d3d00016e5a7dc4a6b463b1f9bc Mon Sep 17 00:00:00 2001 From: fl4p Date: Tue, 26 May 2026 11:28:25 +0100 Subject: [PATCH 06/15] Clarify NimBLEAddress(uint8_t[6], type) byte order in docstring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The constructor calls std::reverse_copy on its input, expecting bytes in MSB-first order (Bluedroid's esp_bd_addr_t layout). The previous docstring said "compatibility with bluedroid esp library using native ESP representation," which is correct but ambiguous if the reader doesn't already know the conventions of both stacks. Callers reaching for this constructor with LSB-first bytes (e.g. as they'd come out of NimBLE's own ble_addr_t.val) silently end up with a reversed address — the radio uses val[] directly for HCI commands, so GAP connects target a fictional peer and time out as BLE_HS_ETIMEOUT with no diagnostic. Documentation-only change; behaviour is unchanged. Points readers to NimBLEAddress(ble_addr_t) for LSB-first wire bytes and to NimBLEAddress(const uint64_t&, uint8_t) for hex literals. Refs #423 --- src/NimBLEAddress.cpp | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/NimBLEAddress.cpp b/src/NimBLEAddress.cpp index 2a719375b..a631b25db 100644 --- a/src/NimBLEAddress.cpp +++ b/src/NimBLEAddress.cpp @@ -97,8 +97,19 @@ NimBLEAddress::NimBLEAddress(const std::string& addr, uint8_t type) { } // NimBLEAddress /** - * @brief Constructor for compatibility with bluedroid esp library using native ESP representation. - * @param [in] address A uint8_t[6] or esp_bd_addr_t containing the address. + * @brief Constructor accepting a 6-byte MSB-first BLE address (Bluedroid `esp_bd_addr_t` layout). + * @details The input bytes are stored internally in LSB-first wire + * order (NimBLE's `ble_addr_t.val` layout), so this constructor + * **reverses** the input. If you already have LSB-first wire bytes — + * e.g. straight out of a NimBLE callback's `ble_addr_t` — use the + * `NimBLEAddress(ble_addr_t)` constructor instead (it stores them + * as-is). If you have the address as a hex integer (e.g. + * `0xa4c1385def16` for `"a4:c1:38:5d:ef:16"`), use + * `NimBLEAddress(const uint64_t&, uint8_t)`. + * @param [in] address Six bytes in MSB-first order + * (`{0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff}` for + * `"aa:bb:cc:dd:ee:ff"`). Compatible with + * Bluedroid's `esp_bd_addr_t`. * @param [in] type The type of the address should be one of: * * BLE_ADDR_PUBLIC (0) * * BLE_ADDR_RANDOM (1) From 555c572cd2ea27011bb3adb6c4cc9f23f4ea3e8e Mon Sep 17 00:00:00 2001 From: h2zero Date: Thu, 30 Jul 2026 17:03:45 -0600 Subject: [PATCH 07/15] [Bugfix] NimBLEStream::availableForWrite signature compatability. This makes the availableForWrite signature override compatible with Arduino Print. --- src/NimBLEStream.cpp | 4 ++-- src/NimBLEStream.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/NimBLEStream.cpp b/src/NimBLEStream.cpp index 7ed0c9085..853cef22f 100644 --- a/src/NimBLEStream.cpp +++ b/src/NimBLEStream.cpp @@ -356,8 +356,8 @@ size_t NimBLEStream::write(const uint8_t* data, size_t len) { * @brief Get the available free space in the stream's TX buffer. * @return the number of bytes that can be written to the stream without blocking. */ -size_t NimBLEStream::availableForWrite() const { - return m_txBuf ? m_txBuf->freeSize() : 0; +int NimBLEStream::availableForWrite() { + return m_txBuf ? static_cast(m_txBuf->freeSize()) : 0; } /** diff --git a/src/NimBLEStream.h b/src/NimBLEStream.h index 47ccb83d8..a0b7d8ae3 100644 --- a/src/NimBLEStream.h +++ b/src/NimBLEStream.h @@ -86,7 +86,7 @@ class NimBLEStream : public Stream { return write(static_cast(data)); } - size_t availableForWrite() const; + int availableForWrite(); // Read up to len bytes into buffer (non-blocking) size_t read(uint8_t* buffer, size_t len); From 7d86ee6217c718546d21d9e7bc47e78f64099aff Mon Sep 17 00:00:00 2001 From: h2zero Date: Fri, 31 Jul 2026 19:25:56 -0600 Subject: [PATCH 08/15] Add include for newer Arduino core to prevent releasing memory before init --- src/NimBLEDevice.cpp | 3 +++ src/NimBLEStream.h | 3 ++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/NimBLEDevice.cpp b/src/NimBLEDevice.cpp index 06cb78651..313abd2f2 100644 --- a/src/NimBLEDevice.cpp +++ b/src/NimBLEDevice.cpp @@ -54,6 +54,9 @@ # if defined(ESP_PLATFORM) && defined(CONFIG_ENABLE_ARDUINO_DEPENDS) # include "esp32-hal-bt.h" +# if __has_include("esp32-hal-bt-mem.h") +# include "esp32-hal-bt-mem.h" +# endif # endif # include "NimBLELog.h" diff --git a/src/NimBLEStream.h b/src/NimBLEStream.h index a0b7d8ae3..5285294bb 100644 --- a/src/NimBLEStream.h +++ b/src/NimBLEStream.h @@ -55,6 +55,7 @@ class Stream : public Print { virtual int available() = 0; virtual int read() = 0; virtual int peek() = 0; + virtual int availableForWrite() { return 0; } virtual void flush() {} void setTimeout(unsigned long timeout) { m_timeout = timeout; } unsigned long getTimeout() const { return m_timeout; } @@ -86,7 +87,7 @@ class NimBLEStream : public Stream { return write(static_cast(data)); } - int availableForWrite(); + int availableForWrite() override; // Read up to len bytes into buffer (non-blocking) size_t read(uint8_t* buffer, size_t len); From adf33e63c9b42902f8cea025ec4e26dc97760f47 Mon Sep 17 00:00:00 2001 From: Stefanos Anastasiou Date: Sun, 19 Jul 2026 16:01:18 +0200 Subject: [PATCH 09/15] =?UTF-8?q?fix:=20whiteListRemove=20use-after-free?= =?UTF-8?q?=20=E2=80=94=20loop=20continues=20on=20invalidated=20iterator?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: whiteListRemove use-after-free - stop iterating after erase + shrink whiteListRemove erases the matched element (invalidating the iterator) and shrink-to-fits m_whiteList (freeing the buffer the iterator points into), then continues the loop on the dangling iterator. A layout-dependent spurious re-match then calls erase() with a foreign iterator, and the vector's internal element move writes across unrelated heap memory. Observed on ESP32-C3 as intermittent heap corruption under whitelist churn (multi_heap_free bad-head asserts with address bytes in the corrupted header; a clobbered NimBLE host mutex tripping xQueueSemaphoreTake). Verified fixed under CONFIG_HEAP_POISONING_COMPREHENSIVE plus periodic heap_caps_check_integrity_all() sweeps. whiteListAdd guards duplicates via onWhiteList(), so at most one element can match - breaking out after the removal preserves semantics. * fix: use m_whiteList.data() - operator[] on an empty vector is UB After erasing the last whitelist entry, &m_whiteList[0] indexes an empty vector before ble_gap_wl_set(..., 0). data() is well-defined for empty vectors; same change applied to the whiteListAdd call site for consistency. --- src/NimBLEDevice.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/NimBLEDevice.cpp b/src/NimBLEDevice.cpp index 313abd2f2..e140d6205 100644 --- a/src/NimBLEDevice.cpp +++ b/src/NimBLEDevice.cpp @@ -728,7 +728,7 @@ bool NimBLEDevice::onWhiteList(const NimBLEAddress& address) { bool NimBLEDevice::whiteListAdd(const NimBLEAddress& address) { if (!NimBLEDevice::onWhiteList(address)) { m_whiteList.push_back(address); - int rc = ble_gap_wl_set(reinterpret_cast(&m_whiteList[0]), m_whiteList.size()); + int rc = ble_gap_wl_set(reinterpret_cast(m_whiteList.data()), m_whiteList.size()); if (rc != 0) { NIMBLE_LOGE(LOG_TAG, "Failed adding to whitelist rc=%d", rc); m_whiteList.pop_back(); @@ -748,7 +748,7 @@ bool NimBLEDevice::whiteListRemove(const NimBLEAddress& address) { for (auto it = m_whiteList.begin(); it < m_whiteList.end(); ++it) { if (*it == address) { m_whiteList.erase(it); - int rc = ble_gap_wl_set(reinterpret_cast(&m_whiteList[0]), m_whiteList.size()); + int rc = ble_gap_wl_set(reinterpret_cast(m_whiteList.data()), m_whiteList.size()); if (rc != 0) { m_whiteList.push_back(address); NIMBLE_LOGE(LOG_TAG, "Failed removing from whitelist rc=%d", rc); @@ -756,6 +756,10 @@ bool NimBLEDevice::whiteListRemove(const NimBLEAddress& address) { } std::vector(m_whiteList).swap(m_whiteList); + break; // `it` was invalidated by erase() and its buffer freed by the + // swap above; continuing would iterate a dangling iterator. + // Duplicates are impossible (whiteListAdd checks onWhiteList), + // so a single match is total. } } From 01781fad32cb8caf5611bf3e504fd6f3506ba803 Mon Sep 17 00:00:00 2001 From: h2zero Date: Wed, 29 Jul 2026 16:43:44 -0600 Subject: [PATCH 10/15] [Bugfix] scan response timer causing crash on reinit cycle. Fixes a crash when the stack is reinitialized without clearing all data on deinit then starting scanning. --- src/NimBLEDevice.cpp | 8 ++++++++ src/NimBLEScan.cpp | 41 +++++++++++++++++++++++++++++++++++------ src/NimBLEScan.h | 2 ++ 3 files changed, 45 insertions(+), 6 deletions(-) diff --git a/src/NimBLEDevice.cpp b/src/NimBLEDevice.cpp index e140d6205..e125aeb9d 100644 --- a/src/NimBLEDevice.cpp +++ b/src/NimBLEDevice.cpp @@ -1031,6 +1031,11 @@ bool NimBLEDevice::deinit(bool clearAll) { if (m_initialized) { rc = nimble_port_stop(); if (rc == 0) { +# if MYNEWT_VAL(BLE_ROLE_OBSERVER) + if (NimBLEDevice::m_pScan != nullptr) { + NimBLEDevice::m_pScan->onHostDeinit(); + } +# endif nimble_port_deinit(); # ifndef USING_NIMBLE_ARDUINO_HEADERS # if ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0) @@ -1042,6 +1047,9 @@ bool NimBLEDevice::deinit(bool clearAll) { # endif m_initialized = false; m_synced = false; + } else { + NIMBLE_LOGE(LOG_TAG, "nimble_port_stop() failed with error: %d", rc); + return false; } } diff --git a/src/NimBLEScan.cpp b/src/NimBLEScan.cpp index 90a381aa2..870e233f7 100644 --- a/src/NimBLEScan.cpp +++ b/src/NimBLEScan.cpp @@ -84,7 +84,6 @@ NimBLEScan::NimBLEScan() }, m_pTaskData{nullptr}, m_maxResults{0xFF} { - ble_npl_callout_init(&m_srTimer, nimble_port_get_dflt_eventq(), NimBLEScan::srTimerCb, nullptr); ble_npl_time_ms_to_ticks(DEFAULT_SCAN_RESP_TIMEOUT_MS, &m_srTimeoutTicks); } // NimBLEScan::NimBLEScan @@ -92,7 +91,10 @@ NimBLEScan::NimBLEScan() * @brief Scan destructor, release any allocated resources. */ NimBLEScan::~NimBLEScan() { - ble_npl_callout_deinit(&m_srTimer); + if (m_srTimerInitialized) { + ble_npl_callout_deinit(&m_srTimer); + m_srTimerInitialized = false; + } for (const auto& dev : m_scanResults.m_deviceVec) { delete dev; @@ -178,7 +180,9 @@ void NimBLEScan::removeWaitingDevice(NimBLEAdvertisedDevice* pDev) { void NimBLEScan::clearWaitingList() { // Stop the timer and remove any pending timeout events since we're clearing // the list and won't be processing any more timeouts for these devices - ble_npl_callout_stop(&m_srTimer); + if (m_srTimerInitialized) { + ble_npl_callout_stop(&m_srTimer); + } ble_npl_hw_enter_critical(); NimBLEAdvertisedDevice* current = m_pWaitingListHead; while (current != nullptr) { @@ -196,10 +200,19 @@ void NimBLEScan::clearWaitingList() { */ void NimBLEScan::resetWaitingTimer() { if (m_srTimeoutTicks == 0 || m_pWaitingListHead == nullptr) { - ble_npl_callout_stop(&m_srTimer); + if (m_srTimerInitialized) { + ble_npl_callout_stop(&m_srTimer); + } return; } + if (!m_srTimerInitialized) { + m_srTimerInitialized = ble_npl_callout_init(&m_srTimer, nimble_port_get_dflt_eventq(), NimBLEScan::srTimerCb, nullptr) == 0; + if (!m_srTimerInitialized) { + return; + } + } + ble_npl_time_t now = ble_npl_time_get(); ble_npl_time_t elapsed = now - m_pWaitingListHead->m_time; ble_npl_time_t nextTime = elapsed >= m_srTimeoutTicks ? 1 : m_srTimeoutTicks - elapsed; @@ -346,7 +359,9 @@ int NimBLEScan::handleGapEvent(ble_gap_event* event, void* arg) { } case BLE_GAP_EVENT_DISC_COMPLETE: { - ble_npl_callout_stop(&pScan->m_srTimer); + if (pScan->m_srTimerInitialized) { + ble_npl_callout_stop(&pScan->m_srTimer); + } // If we have any scannable devices that haven't received a scan response, // we should trigger the callback with whatever data we have since the scan is complete @@ -392,7 +407,9 @@ int NimBLEScan::handleGapEvent(ble_gap_event* event, void* arg) { */ void NimBLEScan::setScanResponseTimeout(uint32_t timeoutMs) { if (timeoutMs == 0) { - ble_npl_callout_stop(&m_srTimer); + if (m_srTimerInitialized) { + ble_npl_callout_stop(&m_srTimer); + } m_srTimeoutTicks = 0; return; } @@ -673,6 +690,18 @@ void NimBLEScan::onHostSync() { m_pScanCallbacks->onScanEnd(m_scanResults, BLE_HS_ENOTSYNCED); } +/** + * @brief Called before host deinit so callouts don't outlive the default event queue. + */ +void NimBLEScan::onHostDeinit() { + clearWaitingList(); + + if (m_srTimerInitialized) { + ble_npl_callout_deinit(&m_srTimer); + m_srTimerInitialized = false; + } +} + /** * @brief Start scanning and block until scanning has been completed. * @param [in] duration The duration in milliseconds for which to scan. diff --git a/src/NimBLEScan.h b/src/NimBLEScan.h index 0c612851d..89c858c8d 100644 --- a/src/NimBLEScan.h +++ b/src/NimBLEScan.h @@ -181,6 +181,7 @@ class NimBLEScan { ~NimBLEScan(); static int handleGapEvent(ble_gap_event* event, void* arg); void onHostSync(); + void onHostDeinit(); static void srTimerCb(ble_npl_event* event); // Linked list helpers for devices awaiting scan responses @@ -194,6 +195,7 @@ class NimBLEScan { NimBLEScanResults m_scanResults; NimBLEUtils::TaskData* m_pTaskData; ble_npl_callout m_srTimer{}; + bool m_srTimerInitialized{false}; ble_npl_time_t m_srTimeoutTicks{}; uint8_t m_maxResults; NimBLEAdvertisedDevice* m_pWaitingListHead{}; // head of linked list for devices awaiting scan responses From e50c6195207278f8ce395b817031b37358698298 Mon Sep 17 00:00:00 2001 From: Anthony Doud Date: Fri, 7 Aug 2026 22:24:07 -0500 Subject: [PATCH 11/15] Initialize semaphore with braces in NimBLEUtils Fixes a connect crash in the client --- src/NimBLEUtils.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/NimBLEUtils.cpp b/src/NimBLEUtils.cpp index ec9767af0..02f5b0a99 100644 --- a/src/NimBLEUtils.cpp +++ b/src/NimBLEUtils.cpp @@ -95,7 +95,7 @@ bool NimBLEUtils::taskWait(const TaskData& taskData, uint32_t timeout) { } if (taskData.m_pSem == nullptr) { - auto* sem = new ble_npl_sem; + auto* sem = new ble_npl_sem{}; if (ble_npl_sem_init(sem, 0) != BLE_NPL_OK) { NIMBLE_LOGE(LOG_TAG, "Failed to initialize semaphore for taskWait"); delete sem; From 89f185b454e72d057cf5df91cfc286bb483104d8 Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Fri, 18 Sep 2026 17:28:15 -0500 Subject: [PATCH 12/15] feat(NimBLEServer): add registerServicesFirst() to place app services at low handles (#443) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(NimBLEServer): add registerServicesFirst() to place app services at low handles By default the standard GAP (0x1800) and GATT (0x1801) services register first and take the low attribute handles (0x0001+), with the application's services placed after them. Some peripherals must instead expose their own services at the low handles — e.g. when emulating a device whose central addresses attributes by hardcoded handle rather than by discovery. Add an opt-in NimBLEServer::registerServicesFirst(bool) setter (default false, so existing behavior is unchanged) that defers GAP/GATT registration until after the application services in resetGATT(), giving the application services the low handles. The device-name/appearance restore that follows ble_gatts_reset() is factored into a small lambda so it stays correct in either order. --- src/NimBLEServer.cpp | 43 +++++++++++++++++++++++++++++++++++++++---- src/NimBLEServer.h | 2 ++ 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/src/NimBLEServer.cpp b/src/NimBLEServer.cpp index 425797911..660a03130 100644 --- a/src/NimBLEServer.cpp +++ b/src/NimBLEServer.cpp @@ -54,6 +54,7 @@ NimBLEServer::NimBLEServer() : m_gattsStarted{false}, m_svcChanged{false}, m_deleteCallbacks{false}, + m_registerAppServicesFirst{false}, # if !MYNEWT_VAL(BLE_EXT_ADV) m_advertiseOnDisconnect{false}, # endif @@ -366,6 +367,20 @@ void NimBLEServer::advertiseOnDisconnect(bool enable) { } // advertiseOnDisconnect # endif +/** + * @brief Register the application's services before the standard GAP/GATT services. + * @param [in] enable true == register the application services first (they take the + * low attribute handles, starting at 0x0001, and GAP/GATT are placed after them); + * false (default) == the standard behavior, GAP/GATT register first and take the low + * handles. + * @details Must be called before the services are started (i.e. before + * NimBLEServer::start()). Useful when a peripheral must expose a fixed attribute-table + * layout that another device relies on by hardcoded handle rather than discovery. + */ +void NimBLEServer::registerAppServicesFirst(bool enable) { + m_registerAppServicesFirst = enable; +} // registerAppServicesFirst + /** * @brief Return the number of connected clients. * @return The number of connected clients. @@ -897,14 +912,24 @@ bool NimBLEServer::resetGATT() { #endif ble_gatts_reset(); - ble_svc_gap_init(); + // Register the standard GAP (0x1800) and GATT (0x1801) services, restoring the + // device name/appearance that ble_gatts_reset() clears. By default this happens + // first, so GAP/GATT take the low attribute handles (0x0001+). When + // registerAppServicesFirst() is enabled, it is deferred until after the application + // services below, so those take the low handles instead. + auto initGapGattServices = [&]() { + ble_svc_gap_init(); #ifndef CONFIG_USING_NIMBLE_COMPONENT - ble_svc_gap_device_name_set(name.c_str()); - ble_svc_gap_device_appearance_set(appearance); + ble_svc_gap_device_name_set(name.c_str()); + ble_svc_gap_device_appearance_set(appearance); #endif + ble_svc_gatt_init(); + }; - ble_svc_gatt_init(); + if (!m_registerAppServicesFirst) { + initGapGattServices(); + } for (auto svcIt = m_svcVec.begin(); svcIt != m_svcVec.end();) { auto* pSvc = *svcIt; @@ -941,6 +966,12 @@ bool NimBLEServer::resetGATT() { if (pSvc->getRemoved() == 0) { if (!pSvc->start_internal()) { NIMBLE_LOGE(LOG_TAG, "Failed to start service: %s", pSvc->getUUID().toString().c_str()); + // When deferring GAP/GATT (registerAppServicesFirst), still register + // them on the failure path so the mandatory GAP/GATT services (and + // the restored name/appearance) are never left out of the database. + if (m_registerAppServicesFirst) { + initGapGattServices(); + } return false; } } @@ -949,6 +980,10 @@ bool NimBLEServer::resetGATT() { ++svcIt; } + if (m_registerAppServicesFirst) { + initGapGattServices(); + } + return true; } // resetGATT diff --git a/src/NimBLEServer.h b/src/NimBLEServer.h index b2b79e1eb..cd07b1718 100644 --- a/src/NimBLEServer.h +++ b/src/NimBLEServer.h @@ -81,6 +81,7 @@ class NimBLEServer { NimBLEConnInfo getPeerInfo(const NimBLEAddress& address) const; NimBLEConnInfo getPeerInfoByHandle(uint16_t connHandle) const; void advertiseOnDisconnect(bool enable); + void registerAppServicesFirst(bool enable); void setDataLen(uint16_t connHandle, uint16_t tx_octets) const; bool updatePhy(uint16_t connHandle, uint8_t txPhysMask, uint8_t rxPhysMask, uint16_t phyOptions); bool getPhy(uint16_t connHandle, uint8_t* txPhy, uint8_t* rxPhy); @@ -129,6 +130,7 @@ class NimBLEServer { bool m_gattsStarted : 1; bool m_svcChanged : 1; bool m_deleteCallbacks : 1; + bool m_registerAppServicesFirst : 1; # if !MYNEWT_VAL(BLE_EXT_ADV) && MYNEWT_VAL(BLE_ROLE_BROADCASTER) bool m_advertiseOnDisconnect : 1; # endif From 4e75fe37cd035e7b7aa36e1471fb695a71ff493b Mon Sep 17 00:00:00 2001 From: h2zero Date: Sat, 17 Oct 2026 13:57:23 -0600 Subject: [PATCH 13/15] Add a mutex to NimBLEAttValue --- src/NimBLEAttValue.cpp | 17 +++---- src/NimBLEAttValue.h | 90 +++++++++++++++++++++++++++------ src/NimBLEClient.cpp | 17 ++++--- src/NimBLEDevice.cpp | 11 +++- src/NimBLELocalValueAttribute.h | 4 +- src/NimBLEServer.cpp | 3 +- 6 files changed, 105 insertions(+), 37 deletions(-) diff --git a/src/NimBLEAttValue.cpp b/src/NimBLEAttValue.cpp index be7695957..99faeff01 100644 --- a/src/NimBLEAttValue.cpp +++ b/src/NimBLEAttValue.cpp @@ -18,16 +18,12 @@ #include "NimBLEAttValue.h" #if CONFIG_BT_NIMBLE_ENABLED -# ifdef USING_NIMBLE_ARDUINO_HEADERS -# include "nimble/nimble/include/nimble/nimble_npl.h" -# else -# include "nimble/nimble_npl.h" -# endif - # include "NimBLEUtils.h" # include "NimBLELog.h" static const char* LOG_TAG = "NimBLEAttValue"; +ble_npl_mutex NimBLEAttValue::Lock::m_mutex; +bool NimBLEAttValue::Lock::m_initialized = false; // Default constructor implementation. NimBLEAttValue::NimBLEAttValue(uint16_t init_len, uint16_t max_len) @@ -64,6 +60,7 @@ NimBLEAttValue::~NimBLEAttValue() { // Move assignment operator implementation. NimBLEAttValue& NimBLEAttValue::operator=(NimBLEAttValue&& source) { if (this != &source) { + Lock lock; free(m_attr_value); m_attr_value = source.m_attr_value; m_attr_max_len = source.m_attr_max_len; @@ -86,6 +83,7 @@ NimBLEAttValue& NimBLEAttValue::operator=(const NimBLEAttValue& source) { // Copy all the data from the source object to this object, including allocated space. void NimBLEAttValue::deepCopy(const NimBLEAttValue& source) { + Lock lock; uint8_t* res = static_cast(realloc(m_attr_value, source.m_capacity + 1)); NIMBLE_CPP_DEBUG_ASSERT(res); if (res == nullptr) { @@ -93,18 +91,17 @@ void NimBLEAttValue::deepCopy(const NimBLEAttValue& source) { return; } - ble_npl_hw_enter_critical(); m_attr_value = res; m_attr_max_len = source.m_attr_max_len; m_attr_len = source.m_attr_len; m_capacity = source.m_capacity; setTimeStamp(source.getTimeStamp()); memcpy(m_attr_value, source.m_attr_value, m_attr_len + 1); - ble_npl_hw_exit_critical(0); } // Set the value of the attribute. bool NimBLEAttValue::setValue(const uint8_t* value, uint16_t len) { + Lock lock; m_attr_len = 0; // Just set the value length to 0 and append instead of repeating code. m_attr_value[0] = '\0'; // Set the first byte to 0 incase the len of the new value is 0. append(value, len); @@ -113,6 +110,7 @@ bool NimBLEAttValue::setValue(const uint8_t* value, uint16_t len) { // Append the new data, allocate as necessary. NimBLEAttValue& NimBLEAttValue::append(const uint8_t* value, uint16_t len) { + Lock lock; if (len == 0) { return *this; } @@ -144,18 +142,17 @@ NimBLEAttValue& NimBLEAttValue::append(const uint8_t* value, uint16_t len) { time_t t = 0; # endif - ble_npl_hw_enter_critical(); memcpy(res + m_attr_len, value, len); m_attr_value = res; m_attr_len = new_len; m_attr_value[m_attr_len] = '\0'; setTimeStamp(t); - ble_npl_hw_exit_critical(0); return *this; } uint8_t NimBLEAttValue::operator[](int pos) const { + Lock lock; NIMBLE_CPP_DEBUG_ASSERT(pos < m_attr_len); if (pos >= m_attr_len) { NIMBLE_LOGE(LOG_TAG, "pos >= len, pos=%u, len=%u", pos, m_attr_len); diff --git a/src/NimBLEAttValue.h b/src/NimBLEAttValue.h index 9ef92d52e..9dcf53e5b 100644 --- a/src/NimBLEAttValue.h +++ b/src/NimBLEAttValue.h @@ -21,6 +21,12 @@ #include "syscfg/syscfg.h" #if CONFIG_BT_NIMBLE_ENABLED +# ifdef USING_NIMBLE_ARDUINO_HEADERS +# include "nimble/nimble/include/nimble/nimble_npl.h" +# else +# include "nimble/nimble_npl.h" +# endif + /* Enables the use of Arduino String class for attribute values */ # ifndef NIMBLE_CPP_ARDUINO_STRING_AVAILABLE # define NIMBLE_CPP_ARDUINO_STRING_AVAILABLE (__has_include()) @@ -103,6 +109,39 @@ class NimBLEAttValue { void deepCopy(const NimBLEAttValue& source); public: + class Lock { + static ble_npl_mutex m_mutex; + static bool m_initialized; + bool m_locked{false}; + + public: + Lock() { + if (m_initialized) { + m_locked = ble_npl_mutex_pend(&m_mutex, BLE_NPL_TIME_FOREVER) == BLE_NPL_OK; + } + } + + ~Lock() { + if (m_initialized && m_locked) { + ble_npl_mutex_release(&m_mutex); + } + } + + static bool init() { + if (!m_initialized) { + m_initialized = ble_npl_mutex_init(&m_mutex) == BLE_NPL_OK; + } + return m_initialized; + } + + static void deinit() { + if (m_initialized) { + m_initialized = false; + ble_npl_mutex_deinit(&m_mutex); + } + } + }; + /** * @brief Default constructor. * @param[in] init_len The initial size in bytes. @@ -181,16 +220,22 @@ class NimBLEAttValue { /** @brief Returns the current size of the value in bytes */ uint16_t size() const { return m_attr_len; } - /** @brief Returns a pointer to the internal buffer of the value */ + /** + * @brief Returns a pointer to the internal buffer of the value + * @note The pointer is not protected by the lock and can be invalidated by a concurrent update. + * Only use the pointer getters (`data()`, `c_str()`, `begin()`, `end()`) on a copy of the value. + */ const uint8_t* data() const { return m_attr_value; } - /** @brief Returns a pointer to the internal buffer of the value as a const char* */ + /** @brief Returns a pointer to the internal buffer of the value as a const char* + * @note See data() for thread safety restrictions. + */ const char* c_str() const { return reinterpret_cast(m_attr_value); } - /** @brief Iterator begin */ + /** @brief Iterator begin; see data() for thread safety restrictions. */ const uint8_t* begin() const { return m_attr_value; } - /** @brief Iterator end */ + /** @brief Iterator end; see data() for thread safety restrictions. */ const uint8_t* end() const { return m_attr_value + m_attr_len; } # if MYNEWT_VAL(NIMBLE_CPP_ATT_VALUE_TIMESTAMP_ENABLED) @@ -262,9 +307,7 @@ class NimBLEAttValue { # ifdef _DOXYGEN_ bool # else - typename std::enable_if::value && - std::is_same::type, char>::value, - bool>::type + typename std::enable_if::value && std::is_same::type, char>::value, bool>::type # endif setValue(const T& s) { return setValue(reinterpret_cast(s), strnlen(s, sizeof(T))); @@ -281,8 +324,7 @@ class NimBLEAttValue { bool # else typename std::enable_if::value && !Has_c_str_length::value && !Has_data_size::value && - !(std::is_array::value && - std::is_same::type, char>::value), + !(std::is_array::value && std::is_same::type, char>::value), bool>::type # endif setValue(const T& v) { @@ -354,8 +396,7 @@ class NimBLEAttValue { } } else if constexpr (Has_c_str_length::value) { return setValue(reinterpret_cast(s.c_str()), s.length()); - } else if constexpr (std::is_array::value && - std::is_same::type, char>::value) { + } else if constexpr (std::is_array::value && std::is_same::type, char>::value) { return setValue(reinterpret_cast(s), strnlen(s, sizeof(s))); } else { return setValue(reinterpret_cast(&s), sizeof(s)); @@ -375,6 +416,7 @@ class NimBLEAttValue { */ template T getValue(time_t* timestamp = nullptr, bool skipSizeCheck = false) const { + Lock lock; if (timestamp != nullptr) { # if MYNEWT_VAL(NIMBLE_CPP_ATT_VALUE_TIMESTAMP_ENABLED) *timestamp = m_timestamp; @@ -395,16 +437,30 @@ class NimBLEAttValue { uint8_t operator[](int pos) const; /** @brief Operator; Get the value as a std::vector. */ - operator std::vector() const { return std::vector(m_attr_value, m_attr_value + m_attr_len); } + operator std::vector() const { + Lock lock; + return std::vector(m_attr_value, m_attr_value + m_attr_len); + } /** @brief Operator; Get the value as a std::string. */ - operator std::string() const { return std::string(reinterpret_cast(m_attr_value), m_attr_len); } + operator std::string() const { + Lock lock; + return std::string(reinterpret_cast(m_attr_value), m_attr_len); + } /** @brief Operator; Get the value as a const uint8_t*. */ operator const uint8_t*() const { return m_attr_value; } /** @brief Operator; Append another NimBLEAttValue. */ - NimBLEAttValue& operator+=(const NimBLEAttValue& source) { return append(source.data(), source.size()); } + NimBLEAttValue& operator+=(const NimBLEAttValue& source) { + Lock lock; // Recursive; keeps the source data and size consistent while appending + if (&source == this) { + // append() may reallocate the buffer, so append from a copy of our own data + std::vector copy(m_attr_value, m_attr_value + m_attr_len); + return append(copy.data(), copy.size()); + } + return append(source.data(), source.size()); + } /** @brief Operator; Set the value from a std::string source. */ NimBLEAttValue& operator=(const std::string& source) { @@ -420,6 +476,7 @@ class NimBLEAttValue { /** @brief Equality operator */ bool operator==(const NimBLEAttValue& source) const { + Lock lock; return (m_attr_len == source.size()) ? memcmp(m_attr_value, source.data(), m_attr_len) == 0 : false; } @@ -428,7 +485,10 @@ class NimBLEAttValue { # if NIMBLE_CPP_ARDUINO_STRING_AVAILABLE /** @brief Operator; Get the value as an Arduino String value. */ - operator String() const { return String(reinterpret_cast(m_attr_value)); } + operator String() const { + Lock lock; + return String(reinterpret_cast(m_attr_value)); + } # endif }; diff --git a/src/NimBLEClient.cpp b/src/NimBLEClient.cpp index 2d219a73d..14afad0e2 100644 --- a/src/NimBLEClient.cpp +++ b/src/NimBLEClient.cpp @@ -1179,13 +1179,16 @@ int NimBLEClient::handleGapEvent(struct ble_gap_event* event, void* arg) { return BLE_ATT_ERR_INVALID_HANDLE; } - auto len = event->notify_rx.om->om_len; - if (pChr->m_value.setValue(event->notify_rx.om->om_data, len)) { + // Build the value locally so the update of the characteristic value is atomic + // and the callback gets a stable copy without holding the value lock. + NimBLEAttValue value(MYNEWT_VAL(NIMBLE_CPP_ATT_VALUE_INIT_LENGTH), pChr->m_value.max_size()); + auto len = event->notify_rx.om->om_len; + if (value.setValue(event->notify_rx.om->om_data, len)) { os_mbuf* next; next = SLIST_NEXT(event->notify_rx.om, om_next); while (next != NULL) { - pChr->m_value.append(next->om_data, next->om_len); - if (pChr->m_value.length() != len + next->om_len) { + value.append(next->om_data, next->om_len); + if (value.length() != len + next->om_len) { rc = BLE_ATT_ERR_INVALID_ATTR_VALUE_LEN; break; } @@ -1201,11 +1204,13 @@ int NimBLEClient::handleGapEvent(struct ble_gap_event* event, void* arg) { return rc; } + pChr->m_value = value; + if (pChr->m_notifyCallback != nullptr) { // TODO: change this callback to use the NimBLEAttValue class instead of raw data and length pChr->m_notifyCallback(pChr, - const_cast(pChr->m_value.getValue().data()), - pChr->m_value.length(), + const_cast(value.data()), + value.length(), !event->notify_rx.indication); } diff --git a/src/NimBLEDevice.cpp b/src/NimBLEDevice.cpp index e125aeb9d..585aaebdc 100644 --- a/src/NimBLEDevice.cpp +++ b/src/NimBLEDevice.cpp @@ -833,7 +833,7 @@ void NimBLEDevice::onReset(int reason) { * @brief Host synced with controller, all clear to make calls to the stack. */ void NimBLEDevice::onSync(void) { - NIMBLE_LOGI(LOG_TAG, "NimBle host synced."); + NIMBLE_LOGI(LOG_TAG, "NimBLE host synced."); // This check is needed due to potentially being called multiple times in succession // If this happens, the call to scan start may get stuck or cause an advertising fault. if (m_synced) { @@ -894,6 +894,12 @@ void NimBLEDevice::host_task(void* param) { bool NimBLEDevice::init(const std::string& deviceName) { if (!m_initialized) { NIMBLE_LOGD(LOG_TAG, "Starting %s", getVersion()); + + if (!NimBLEAttValue::Lock::init()) { + NIMBLE_LOGE(LOG_TAG, "Failed to initialize NimBLEAttValue lock"); + return false; + } + # ifdef ESP_PLATFORM # if defined(CONFIG_ENABLE_ARDUINO_DEPENDS) && SOC_BT_SUPPORTED @@ -1015,7 +1021,7 @@ bool NimBLEDevice::init(const std::string& deviceName) { } m_initialized = true; // Set the initialization flag to ensure we are only initialized once. - NIMBLE_LOGD(LOG_TAG, "Initialized"); + NIMBLE_LOGD(LOG_TAG, "Initialized BLE stack"); return true; } // init @@ -1088,6 +1094,7 @@ bool NimBLEDevice::deinit(bool clearAll) { # endif NimBLEUtils::deleteTaskSems(); + NimBLEAttValue::Lock::deinit(); } return rc == 0; diff --git a/src/NimBLELocalValueAttribute.h b/src/NimBLELocalValueAttribute.h index 5fd37e550..0c4f39095 100644 --- a/src/NimBLELocalValueAttribute.h +++ b/src/NimBLELocalValueAttribute.h @@ -125,8 +125,8 @@ class NimBLELocalValueAttribute : public NimBLELocalAttribute, public NimBLEValu virtual void writeEvent(const uint8_t* val, uint16_t len, NimBLEConnInfo& connInfo) = 0; /** - * @brief Get a pointer to value of the attribute. - * @return A pointer to the value of the attribute. + * @brief Get a reference to value of the attribute. + * @return A reference to the value of the attribute. * @details This function is used by NimBLEServer when handling read/write requests. */ const NimBLEAttValue& getAttVal() const { return m_value; } diff --git a/src/NimBLEServer.cpp b/src/NimBLEServer.cpp index 660a03130..3129d113e 100644 --- a/src/NimBLEServer.cpp +++ b/src/NimBLEServer.cpp @@ -760,9 +760,8 @@ int NimBLEServer::handleGattEvent(uint16_t connHandle, uint16_t attrHandle, ble_ pAtt->readEvent(peerInfo); } - ble_npl_hw_enter_critical(); + NimBLEAttValue::Lock lock; int rc = os_mbuf_append(ctxt->om, val.data(), val.size()); - ble_npl_hw_exit_critical(0); return rc == 0 ? 0 : BLE_ATT_ERR_INSUFFICIENT_RES; } From e4a36711fb050ff044a5108c69dbac94ca72e3a6 Mon Sep 17 00:00:00 2001 From: h2zero Date: Tue, 20 Oct 2026 10:17:00 -0600 Subject: [PATCH 14/15] Fix deinit sync issues --- src/NimBLEAttValue.cpp | 43 +++++++++++++++++++-- src/NimBLEAttValue.h | 52 +++++++++++++++++++------ src/NimBLEDevice.cpp | 51 ++++++++++++------------ src/NimBLEDevice.h | 5 ++- src/NimBLEUtils.cpp | 88 ++++++++++++++++++++++++++++++++++-------- src/NimBLEUtils.h | 22 +++++++++-- 6 files changed, 199 insertions(+), 62 deletions(-) diff --git a/src/NimBLEAttValue.cpp b/src/NimBLEAttValue.cpp index 99faeff01..9fc09cab5 100644 --- a/src/NimBLEAttValue.cpp +++ b/src/NimBLEAttValue.cpp @@ -21,9 +21,46 @@ # include "NimBLEUtils.h" # include "NimBLELog.h" +# if defined(__ARM_ARCH_6M__) +// Cortex-M0/M0+ has no LDREX/STREX, so GCC emits libcalls for 4 byte atomic read-modify-write +// operations that are not provided by libgcc. Implement them by briefly masking interrupts. +extern "C" { +unsigned int __atomic_fetch_add_4(volatile void* ptr, unsigned int val, int) { + volatile unsigned int* p = static_cast(ptr); + unsigned int primask; + __asm volatile("mrs %0, primask\n\tcpsid i" : "=r"(primask)::"memory"); + unsigned int old = *p; + *p = old + val; + __asm volatile("msr primask, %0" ::"r"(primask) : "memory"); + return old; +} + +unsigned int __atomic_fetch_sub_4(volatile void* ptr, unsigned int val, int) { + volatile unsigned int* p = static_cast(ptr); + unsigned int primask; + __asm volatile("mrs %0, primask\n\tcpsid i" : "=r"(primask)::"memory"); + unsigned int old = *p; + *p = old - val; + __asm volatile("msr primask, %0" ::"r"(primask) : "memory"); + return old; +} + +unsigned char __atomic_exchange_1(volatile void* ptr, unsigned char val, int) { + volatile unsigned char* p = static_cast(ptr); + unsigned int primask; + __asm volatile("mrs %0, primask\n\tcpsid i" : "=r"(primask)::"memory"); + unsigned char old = *p; + *p = val; + __asm volatile("msr primask, %0" ::"r"(primask) : "memory"); + return old; +} +} +# endif + static const char* LOG_TAG = "NimBLEAttValue"; -ble_npl_mutex NimBLEAttValue::Lock::m_mutex; -bool NimBLEAttValue::Lock::m_initialized = false; +ble_npl_mutex NimBLEAttValue::Lock::m_mutex{}; +std::atomic NimBLEAttValue::Lock::m_initialized{false}; +std::atomic NimBLEAttValue::Lock::m_activeCount{0}; // Default constructor implementation. NimBLEAttValue::NimBLEAttValue(uint16_t init_len, uint16_t max_len) @@ -83,7 +120,7 @@ NimBLEAttValue& NimBLEAttValue::operator=(const NimBLEAttValue& source) { // Copy all the data from the source object to this object, including allocated space. void NimBLEAttValue::deepCopy(const NimBLEAttValue& source) { - Lock lock; + Lock lock; uint8_t* res = static_cast(realloc(m_attr_value, source.m_capacity + 1)); NIMBLE_CPP_DEBUG_ASSERT(res); if (res == nullptr) { diff --git a/src/NimBLEAttValue.h b/src/NimBLEAttValue.h index 9dcf53e5b..d73281264 100644 --- a/src/NimBLEAttValue.h +++ b/src/NimBLEAttValue.h @@ -41,6 +41,7 @@ # include # include # include +# include # ifndef MYNEWT_VAL_NIMBLE_CPP_ATT_VALUE_TIMESTAMP_ENABLED # ifndef CONFIG_NIMBLE_CPP_ATT_VALUE_TIMESTAMP_ENABLED @@ -110,34 +111,61 @@ class NimBLEAttValue { public: class Lock { - static ble_npl_mutex m_mutex; - static bool m_initialized; - bool m_locked{false}; + static ble_npl_mutex m_mutex; + static std::atomic m_initialized; + static std::atomic m_activeCount; + bool m_locked{false}; public: Lock() { - if (m_initialized) { - m_locked = ble_npl_mutex_pend(&m_mutex, BLE_NPL_TIME_FOREVER) == BLE_NPL_OK; + // Increment active count to signal that a constructor is running + m_activeCount.fetch_add(1); + + if (m_initialized.load()) { + m_locked = (ble_npl_mutex_pend(&m_mutex, BLE_NPL_TIME_FOREVER) == BLE_NPL_OK); + + // Post-lock check: If deinit flipped the flag while we were waiting in line, + // we must abort, give it back, and act un-locked. + if (!m_initialized.load() && m_locked) { + ble_npl_mutex_release(&m_mutex); + m_locked = false; + } } + + // Decrement active count to signal that the constructor has finished processing + m_activeCount.fetch_sub(1); } ~Lock() { - if (m_initialized && m_locked) { + if (m_locked) { ble_npl_mutex_release(&m_mutex); } } static bool init() { - if (!m_initialized) { - m_initialized = ble_npl_mutex_init(&m_mutex) == BLE_NPL_OK; + if (!m_initialized.load()) { + if (ble_npl_mutex_init(&m_mutex) == BLE_NPL_OK) { + m_initialized.store(true); + } } - return m_initialized; + return m_initialized.load(); } static void deinit() { - if (m_initialized) { - m_initialized = false; - ble_npl_mutex_deinit(&m_mutex); + if (m_initialized.load()) { + // Prevent any new instances from acquiring the lock while we deinitialize + m_initialized.store(false); + + // Wait for any active users to finish + while (m_activeCount.load() > 0) { + ble_npl_time_delay(1); + } + + // Acquire the mutex to ensure no other threads are holding it before deinitializing. + if (ble_npl_mutex_pend(&m_mutex, BLE_NPL_TIME_FOREVER) == BLE_NPL_OK) { + // Deinitialize the mutex now that we have exclusive ownership and no active users. + ble_npl_mutex_deinit(&m_mutex); + } } } }; diff --git a/src/NimBLEDevice.cpp b/src/NimBLEDevice.cpp index 585aaebdc..15b9e5eca 100644 --- a/src/NimBLEDevice.cpp +++ b/src/NimBLEDevice.cpp @@ -94,9 +94,9 @@ NimBLEAdvertising* NimBLEDevice::m_bleAdvertising = nullptr; std::array NimBLEDevice::m_pClients{}; # endif -bool NimBLEDevice::m_initialized{false}; +std::atomic NimBLEDevice::m_initialized{false}; uint32_t NimBLEDevice::m_passkey{123456}; -bool NimBLEDevice::m_synced{false}; +std::atomic NimBLEDevice::m_synced{false}; ble_gap_event_listener NimBLEDevice::m_listener{}; std::vector NimBLEDevice::m_whiteList{}; uint8_t NimBLEDevice::m_ownAddrType{BLE_OWN_ADDR_PUBLIC}; @@ -257,7 +257,7 @@ NimBLEScan* NimBLEDevice::getScan() { * @details Must only be called before calling NimBLEDevice::init. */ void NimBLEDevice::setScanDuplicateCacheSize(uint16_t size) { - if (m_initialized) { + if (isInitialized()) { NIMBLE_LOGE(LOG_TAG, "Cannot change scan cache size while initialized"); return; } else { @@ -286,7 +286,7 @@ void NimBLEDevice::setScanDuplicateCacheSize(uint16_t size) { * @details Must only be called before calling NimBLEDevice::init. */ void NimBLEDevice::setScanFilterMode(uint8_t mode) { - if (m_initialized) { + if (isInitialized()) { NIMBLE_LOGE(LOG_TAG, "Cannot change scan duplicate type while initialized"); return; } else if (mode > 2) { @@ -304,7 +304,7 @@ void NimBLEDevice::setScanFilterMode(uint8_t mode) { * even if already seen in the current scan. If set to 0 the cache will never be reset. */ void NimBLEDevice::setScanDuplicateCacheResetTime(uint16_t time) { - if (m_initialized) { + if (isInitialized()) { NIMBLE_LOGE(LOG_TAG, "Cannot change scan cache reset time while initialized"); return; } else if (time > 1000) { @@ -820,11 +820,11 @@ bool NimBLEDevice::setDefaultPhy(uint8_t txPhyMask, uint8_t rxPhyMask) { * @param [in] reason The reason code for the reset. */ void NimBLEDevice::onReset(int reason) { - if (!m_synced) { + if (!m_synced.load(std::memory_order_acquire)) { return; } - m_synced = false; + m_synced.store(false, std::memory_order_release); NIMBLE_LOGE(LOG_TAG, "Host reset; reason=%d, %s", reason, NimBLEUtils::returnCodeToString(reason)); } // onReset @@ -836,7 +836,7 @@ void NimBLEDevice::onSync(void) { NIMBLE_LOGI(LOG_TAG, "NimBLE host synced."); // This check is needed due to potentially being called multiple times in succession // If this happens, the call to scan start may get stuck or cause an advertising fault. - if (m_synced) { + if (m_synced.load(std::memory_order_acquire)) { return; } @@ -861,9 +861,9 @@ void NimBLEDevice::onSync(void) { // Occasionally triggers exception without. ble_npl_time_delay(1); - m_synced = true; + m_synced.store(true, std::memory_order_release); - if (m_initialized) { + if (isInitialized()) { # if MYNEWT_VAL(BLE_ROLE_OBSERVER) if (m_pScan != nullptr) { m_pScan->onHostSync(); @@ -892,14 +892,8 @@ void NimBLEDevice::host_task(void* param) { * @param [in] deviceName The device name of the device. */ bool NimBLEDevice::init(const std::string& deviceName) { - if (!m_initialized) { + if (!isInitialized()) { NIMBLE_LOGD(LOG_TAG, "Starting %s", getVersion()); - - if (!NimBLEAttValue::Lock::init()) { - NIMBLE_LOGE(LOG_TAG, "Failed to initialize NimBLEAttValue lock"); - return false; - } - # ifdef ESP_PLATFORM # if defined(CONFIG_ENABLE_ARDUINO_DEPENDS) && SOC_BT_SUPPORTED @@ -1010,17 +1004,23 @@ bool NimBLEDevice::init(const std::string& deviceName) { ble_hs_cfg.sm_their_key_dist |= BLE_SM_PAIR_KEY_DIST_ID; # endif + if (!NimBLEAttValue::Lock::init()) { + NIMBLE_LOGE(LOG_TAG, "Failed to initialize value lock"); + nimble_port_deinit(); + return false; + } + setDeviceName(deviceName); ble_store_config_init(); nimble_port_freertos_init(NimBLEDevice::host_task); } // Wait for host and controller to sync before returning and accepting new tasks - while (!m_synced) { + while (!m_synced.load(std::memory_order_acquire)) { ble_npl_time_delay(1); } - m_initialized = true; // Set the initialization flag to ensure we are only initialized once. + m_initialized.store(true); // Set the initialization flag to ensure we are only initialized once. NIMBLE_LOGD(LOG_TAG, "Initialized BLE stack"); return true; } // init @@ -1034,14 +1034,18 @@ bool NimBLEDevice::init(const std::string& deviceName) { */ bool NimBLEDevice::deinit(bool clearAll) { int rc = 0; - if (m_initialized) { + if (isInitialized()) { rc = nimble_port_stop(); if (rc == 0) { + m_initialized.store(false); + m_synced.store(false, std::memory_order_release); # if MYNEWT_VAL(BLE_ROLE_OBSERVER) if (NimBLEDevice::m_pScan != nullptr) { NimBLEDevice::m_pScan->onHostDeinit(); } # endif + NimBLEAttValue::Lock::deinit(); + NimBLEUtils::deinit(); nimble_port_deinit(); # ifndef USING_NIMBLE_ARDUINO_HEADERS # if ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0) @@ -1051,8 +1055,6 @@ bool NimBLEDevice::deinit(bool clearAll) { } # endif # endif - m_initialized = false; - m_synced = false; } else { NIMBLE_LOGE(LOG_TAG, "nimble_port_stop() failed with error: %d", rc); return false; @@ -1092,9 +1094,6 @@ bool NimBLEDevice::deinit(bool clearAll) { deleteClient(clt); } # endif - - NimBLEUtils::deleteTaskSems(); - NimBLEAttValue::Lock::deinit(); } return rc == 0; @@ -1105,7 +1104,7 @@ bool NimBLEDevice::deinit(bool clearAll) { * @return true if initialized. */ bool NimBLEDevice::isInitialized() { - return m_initialized; + return m_initialized.load(); } // getInitialized /* -------------------------------------------------------------------------- */ diff --git a/src/NimBLEDevice.h b/src/NimBLEDevice.h index fb64c83db..fc247bf78 100644 --- a/src/NimBLEDevice.h +++ b/src/NimBLEDevice.h @@ -42,6 +42,7 @@ # include # include +# include # if MYNEWT_VAL(BLE_ROLE_CENTRAL) # include @@ -214,8 +215,8 @@ class NimBLEDevice { # endif private: - static bool m_synced; - static bool m_initialized; + static std::atomic m_synced; + static std::atomic m_initialized; static uint32_t m_passkey; static ble_gap_event_listener m_listener; static uint8_t m_ownAddrType; diff --git a/src/NimBLEUtils.cpp b/src/NimBLEUtils.cpp index 02f5b0a99..683a05a6a 100644 --- a/src/NimBLEUtils.cpp +++ b/src/NimBLEUtils.cpp @@ -18,7 +18,7 @@ #include "NimBLEUtils.h" #if CONFIG_BT_NIMBLE_ENABLED -# include "NimBLEAddress.h" +# include "NimBLEDevice.h" # include "NimBLELog.h" # ifdef USING_NIMBLE_ARDUINO_HEADERS @@ -62,6 +62,8 @@ static const char* LOG_TAG = "NimBLEUtils"; std::vector NimBLEUtils::m_taskSemEntries{}; +std::atomic NimBLEUtils::m_semUserCount{0}; +std::atomic_flag NimBLEUtils::m_taskSemLock = ATOMIC_FLAG_INIT; /** * @brief Construct a NimBLEUtils::TaskData instance. @@ -79,6 +81,15 @@ NimBLEUtils::TaskData::TaskData(void* pInstance, int flags, void* buf) * @return True if the task completed, false if the timeout was reached. */ bool NimBLEUtils::taskWait(const TaskData& taskData, uint32_t timeout) { + m_semUserCount.fetch_add(1); + + if (!NimBLEDevice::isInitialized()) { + NIMBLE_LOGE(LOG_TAG, "NimBLE stack is not initialized"); + taskData.m_flags = BLE_HS_EOS; + m_semUserCount.fetch_sub(1); + return false; + } + ble_npl_time_t ticks; if (timeout == BLE_NPL_TIME_FOREVER) { ticks = BLE_NPL_TIME_FOREVER; @@ -86,11 +97,16 @@ bool NimBLEUtils::taskWait(const TaskData& taskData, uint32_t timeout) { ble_npl_time_ms_to_ticks(timeout, &ticks); } + taskData.m_pSem = nullptr; + // find a semaphore that is not currently in use, or create a new one - for (auto& entry : m_taskSemEntries) { - if (!entry->inUse) { - taskData.m_pSem = entry; - break; + { + SpinGuard guard(m_taskSemLock); + for (auto& entry : m_taskSemEntries) { + if (!entry->inUse.exchange(true, std::memory_order_acq_rel)) { + taskData.m_pSem = entry; + break; + } } } @@ -99,20 +115,31 @@ bool NimBLEUtils::taskWait(const TaskData& taskData, uint32_t timeout) { if (ble_npl_sem_init(sem, 0) != BLE_NPL_OK) { NIMBLE_LOGE(LOG_TAG, "Failed to initialize semaphore for taskWait"); delete sem; + m_semUserCount.fetch_sub(1); return false; } TaskSemEntry* entry = new TaskSemEntry; entry->sem = sem; entry->inUse = true; - m_taskSemEntries.push_back(entry); + { + SpinGuard guard(m_taskSemLock); + m_taskSemEntries.push_back(entry); + } taskData.m_pSem = entry; - NIMBLE_LOGD(LOG_TAG, "Created new semaphore for taskWait, total semaphores: %d\n", (int)m_taskSemEntries.size()); } + taskData.m_pSem->inUse.store(true); + m_semUserCount.fetch_sub(1); NIMBLE_LOGD(LOG_TAG, "Task waiting with timeout %" PRIu32 "ms", timeout); - taskData.m_pSem->inUse = true; - return ble_npl_sem_pend(taskData.m_pSem->sem, ticks) == BLE_NPL_OK; + if (ble_npl_sem_pend(taskData.m_pSem->sem, ticks) == BLE_NPL_OK) { + taskData.m_pSem->inUse.store(false); + return true; + } + + NIMBLE_LOGE(LOG_TAG, "Task wait timed out after %" PRIu32 "ms", timeout); + taskData.m_pSem->inUse.store(false); + return false; } // taskWait /** @@ -123,11 +150,9 @@ bool NimBLEUtils::taskWait(const TaskData& taskData, uint32_t timeout) { void NimBLEUtils::taskRelease(const TaskData& taskData, int flags) { taskData.m_flags = flags; if (taskData.m_pSem == nullptr) { - NIMBLE_LOGE(LOG_TAG, "taskRelease called with null semaphore"); return; } - taskData.m_pSem->inUse = false; auto rc = ble_npl_sem_release(taskData.m_pSem->sem); if (rc != BLE_NPL_OK) { NIMBLE_LOGE(LOG_TAG, "Failed to release semaphore: rc=%d %s", rc, returnCodeToString(rc)); @@ -136,13 +161,24 @@ void NimBLEUtils::taskRelease(const TaskData& taskData, int flags) { } // taskRelease /** - * @brief Deletes all semaphores used for task waiting. + * @brief Deinitializes the NimBLEUtils class and deletes all semaphores used for task waiting. * @details This should be called when the NimBLE stack is deinitialized to clean up any resources used by waiting tasks. */ -void NimBLEUtils::deleteTaskSems() { - for (auto& entry : m_taskSemEntries) { - ble_npl_sem_release(entry->sem); - ble_npl_time_delay(10); // give time for any pending tasks to be released +void NimBLEUtils::deinit() { + // wait for any active users of the semaphore vector to finish + while (m_semUserCount.load() > 0) { + ble_npl_time_delay(1); + } + + for (auto entry : m_taskSemEntries) { + if (entry->inUse.load()) { + ble_npl_sem_release(entry->sem); + // wait for the pend to complete before deinitializing the semaphore + while (entry->inUse.load()) { + ble_npl_time_delay(1); + } + } + ble_npl_sem_deinit(entry->sem); delete entry->sem; delete entry; @@ -599,4 +635,24 @@ NimBLEAddress NimBLEUtils::generateAddr(bool nrpa) { return NimBLEAddress{addr}; } // generateAddr +/** + * @brief Construct a SpinGuard and acquire the lock. + * @param [in] flag The atomic_flag to guard. + * @details Spins until the lock is acquired. + */ +NimBLEUtils::SpinGuard::SpinGuard(std::atomic_flag& flag) : m_flag(flag) { + while (m_flag.test_and_set(std::memory_order_acquire)) { + // spin until the lock is acquired + ble_npl_time_delay(1); + } +} + +/** + * @brief Destruct a SpinGuard and release the lock. + * @details Releases the lock acquired by the constructor. + */ +NimBLEUtils::SpinGuard::~SpinGuard() { + m_flag.clear(std::memory_order_release); +} + #endif // CONFIG_BT_NIMBLE_ENABLED diff --git a/src/NimBLEUtils.h b/src/NimBLEUtils.h index fafb02c19..0bf780c27 100644 --- a/src/NimBLEUtils.h +++ b/src/NimBLEUtils.h @@ -42,6 +42,7 @@ void nimble_cpp_assert(const char* file, unsigned line) __attribute((weak, noret # include # include +# include class NimBLEAddress; struct ble_npl_sem; @@ -51,11 +52,24 @@ struct ble_npl_sem; */ class NimBLEUtils { struct TaskSemEntry { - ble_npl_sem* sem; - bool inUse{false}; + ble_npl_sem* sem; + std::atomic inUse{false}; }; public: + /** + * @brief A simple spinlock guard for atomic_flag. + * @details Acquires the lock on construction and releases it on destruction. + */ + class SpinGuard { + public: + explicit SpinGuard(std::atomic_flag& flag); + ~SpinGuard(); + + private: + std::atomic_flag& m_flag; + }; + /** * @brief A structure to hold data for a task that is waiting for a response. * @details This structure is used in conjunction with NimBLEUtils::taskWait() and NimBLEUtils::taskRelease(). @@ -83,8 +97,10 @@ class NimBLEUtils { private: friend class NimBLEDevice; - static void deleteTaskSems(); + static void deinit(); static std::vector m_taskSemEntries; + static std::atomic m_semUserCount; + static std::atomic_flag m_taskSemLock; }; #endif // CONFIG_BT_NIMBLE_ENABLED From 84c334db34f8d7ae47cffd1a126825d8efeb3dba Mon Sep 17 00:00:00 2001 From: h2zero Date: Wed, 21 Oct 2026 17:49:18 -0600 Subject: [PATCH 15/15] Add NimBLEAttValue::copyFrom method for checking allocation failure. * Make deepCopy return whether memory allocation and copying succeeded. * Add copyFrom to expose that result to callers. * Allocate notification values using the full mbuf packet length, with BLE_ATT_ATTR_MAX_LEN as the maximum. * Reject oversized notifications and report temporary-buffer allocation or characteristic-value copy failures as insufficient resources. * Reassemble notification data before updating the characteristic value or invoking its callback. --- src/NimBLEAttValue.cpp | 5 +++-- src/NimBLEAttValue.h | 9 ++++++++- src/NimBLEClient.cpp | 41 +++++++++++++++++------------------------ 3 files changed, 28 insertions(+), 27 deletions(-) diff --git a/src/NimBLEAttValue.cpp b/src/NimBLEAttValue.cpp index 9fc09cab5..86055f30a 100644 --- a/src/NimBLEAttValue.cpp +++ b/src/NimBLEAttValue.cpp @@ -119,13 +119,13 @@ NimBLEAttValue& NimBLEAttValue::operator=(const NimBLEAttValue& source) { } // Copy all the data from the source object to this object, including allocated space. -void NimBLEAttValue::deepCopy(const NimBLEAttValue& source) { +bool NimBLEAttValue::deepCopy(const NimBLEAttValue& source) { Lock lock; uint8_t* res = static_cast(realloc(m_attr_value, source.m_capacity + 1)); NIMBLE_CPP_DEBUG_ASSERT(res); if (res == nullptr) { NIMBLE_LOGE(LOG_TAG, "Failed to realloc deepCopy"); - return; + return false; } m_attr_value = res; @@ -134,6 +134,7 @@ void NimBLEAttValue::deepCopy(const NimBLEAttValue& source) { m_capacity = source.m_capacity; setTimeStamp(source.getTimeStamp()); memcpy(m_attr_value, source.m_attr_value, m_attr_len + 1); + return true; } // Set the value of the attribute. diff --git a/src/NimBLEAttValue.h b/src/NimBLEAttValue.h index d73281264..a3571b530 100644 --- a/src/NimBLEAttValue.h +++ b/src/NimBLEAttValue.h @@ -107,7 +107,7 @@ class NimBLEAttValue { # if MYNEWT_VAL(NIMBLE_CPP_ATT_VALUE_TIMESTAMP_ENABLED) time_t m_timestamp{}; # endif - void deepCopy(const NimBLEAttValue& source); + bool deepCopy(const NimBLEAttValue& source); public: class Lock { @@ -323,6 +323,13 @@ class NimBLEAttValue { */ NimBLEAttValue& append(const uint8_t* value, uint16_t len); + /** + * @brief Copy the value from another NimBLEAttValue instance. + * @param[in] source The NimBLEAttValue instance to copy from. + * @returns True if the copy was successful. + */ + bool copyFrom(const NimBLEAttValue& source) { return deepCopy(source); } + /*********************** Template Functions ************************/ # if __cplusplus < 201703L diff --git a/src/NimBLEClient.cpp b/src/NimBLEClient.cpp index 14afad0e2..470dbf2cf 100644 --- a/src/NimBLEClient.cpp +++ b/src/NimBLEClient.cpp @@ -1181,37 +1181,30 @@ int NimBLEClient::handleGapEvent(struct ble_gap_event* event, void* arg) { // Build the value locally so the update of the characteristic value is atomic // and the callback gets a stable copy without holding the value lock. - NimBLEAttValue value(MYNEWT_VAL(NIMBLE_CPP_ATT_VALUE_INIT_LENGTH), pChr->m_value.max_size()); - auto len = event->notify_rx.om->om_len; - if (value.setValue(event->notify_rx.om->om_data, len)) { - os_mbuf* next; - next = SLIST_NEXT(event->notify_rx.om, om_next); - while (next != NULL) { - value.append(next->om_data, next->om_len); - if (value.length() != len + next->om_len) { - rc = BLE_ATT_ERR_INVALID_ATTR_VALUE_LEN; - break; - } - len += next->om_len; - next = SLIST_NEXT(next, om_next); - } - } else { - rc = BLE_ATT_ERR_INVALID_ATTR_VALUE_LEN; + const auto totalLen = OS_MBUF_PKTLEN(event->notify_rx.om); + if (totalLen > BLE_ATT_ATTR_MAX_LEN) { + NIMBLE_LOGE(LOG_TAG, "notification value error; exceeds limit"); + return BLE_ATT_ERR_INVALID_ATTR_VALUE_LEN; } - if (rc != 0) { // This should never happen - NIMBLE_LOGE(LOG_TAG, "notification value error; exceeds limit"); - return rc; + NimBLEAttValue value(totalLen, BLE_ATT_ATTR_MAX_LEN); + if (value.data() == nullptr) { + NIMBLE_LOGE(LOG_TAG, "Failed to allocate notification value"); + return BLE_ATT_ERR_INSUFFICIENT_RES; + } + + for (os_mbuf* segment = event->notify_rx.om; segment != nullptr; segment = SLIST_NEXT(segment, om_next)) { + value.append(segment->om_data, segment->om_len); } - pChr->m_value = value; + if (!pChr->m_value.copyFrom(value)) { + NIMBLE_LOGE(LOG_TAG, "Failed to copy value to characteristic"); + return BLE_ATT_ERR_INSUFFICIENT_RES; + } if (pChr->m_notifyCallback != nullptr) { // TODO: change this callback to use the NimBLEAttValue class instead of raw data and length - pChr->m_notifyCallback(pChr, - const_cast(value.data()), - value.length(), - !event->notify_rx.indication); + pChr->m_notifyCallback(pChr, const_cast(value.data()), value.length(), !event->notify_rx.indication); } return 0;