Skip to content

Add a mutex to NimBLEAttValue #1197

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
h2zero merged 3 commits into from
Oct 22, 2026
Merged

Add a mutex to NimBLEAttValue #1197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e39ed8b
Add a mutex to NimBLEAttValue
h2zero Oct 17, 2026
a3d4022
Fix deinit sync issues
h2zero Oct 20, 2026
5f930d5
Add NimBLEAttValue::copyFrom method for checking allocation failure.
h2zero Oct 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 47 additions & 12 deletions src/NimBLEAttValue.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,49 @@
#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"

# 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<volatile unsigned int*>(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<volatile unsigned int*>(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<volatile unsigned char*>(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{};
std::atomic<bool> NimBLEAttValue::Lock::m_initialized{false};
std::atomic<int> NimBLEAttValue::Lock::m_activeCount{0};

// Default constructor implementation.
NimBLEAttValue::NimBLEAttValue(uint16_t init_len, uint16_t max_len)
Expand Down Expand Up @@ -64,6 +97,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;
Expand All @@ -85,26 +119,27 @@ 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<uint8_t*>(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;
}

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);
return true;
}

// 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);
Expand All @@ -113,6 +148,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;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
if (len == 0) {
return *this;
}
Expand Down Expand Up @@ -144,18 +180,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);
Expand Down
127 changes: 111 additions & 16 deletions src/NimBLEAttValue.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(<Arduino.h>))
Expand All @@ -35,6 +41,7 @@
# include <ctime>
# include <cstring>
# include <cstdint>
# include <atomic>

# ifndef MYNEWT_VAL_NIMBLE_CPP_ATT_VALUE_TIMESTAMP_ENABLED
# ifndef CONFIG_NIMBLE_CPP_ATT_VALUE_TIMESTAMP_ENABLED
Expand Down Expand Up @@ -99,9 +106,69 @@ 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 {
static ble_npl_mutex m_mutex;
static std::atomic<bool> m_initialized;
static std::atomic<int> m_activeCount;
bool m_locked{false};

public:
Lock() {
// 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_locked) {
ble_npl_mutex_release(&m_mutex);
}
}

static bool init() {
if (!m_initialized.load()) {
if (ble_npl_mutex_init(&m_mutex) == BLE_NPL_OK) {
m_initialized.store(true);
}
}
return m_initialized.load();
}

static void deinit() {
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);
}
}
}
};
Comment thread
coderabbitai[bot] marked this conversation as resolved.

/**
* @brief Default constructor.
* @param[in] init_len The initial size in bytes.
Expand Down Expand Up @@ -181,16 +248,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<const char*>(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)
Expand Down Expand Up @@ -250,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
Expand All @@ -262,9 +342,7 @@ class NimBLEAttValue {
# ifdef _DOXYGEN_
bool
# else
typename std::enable_if<std::is_array<T>::value &&
std::is_same<typename std::remove_extent<T>::type, char>::value,
bool>::type
typename std::enable_if<std::is_array<T>::value && std::is_same<typename std::remove_extent<T>::type, char>::value, bool>::type
# endif
setValue(const T& s) {
return setValue(reinterpret_cast<const uint8_t*>(s), strnlen(s, sizeof(T)));
Expand All @@ -281,8 +359,7 @@ class NimBLEAttValue {
bool
# else
typename std::enable_if<!std::is_pointer<T>::value && !Has_c_str_length<T>::value && !Has_data_size<T>::value &&
!(std::is_array<T>::value &&
std::is_same<typename std::remove_extent<T>::type, char>::value),
!(std::is_array<T>::value && std::is_same<typename std::remove_extent<T>::type, char>::value),
bool>::type
# endif
setValue(const T& v) {
Expand Down Expand Up @@ -351,8 +428,7 @@ class NimBLEAttValue {
}
} else if constexpr (Has_c_str_length<T>::value) {
return setValue(reinterpret_cast<const uint8_t*>(s.c_str()), s.length());
} else if constexpr (std::is_array<T>::value &&
std::is_same<typename std::remove_extent<T>::type, char>::value) {
} else if constexpr (std::is_array<T>::value && std::is_same<typename std::remove_extent<T>::type, char>::value) {
return setValue(reinterpret_cast<const uint8_t*>(s), strnlen(s, sizeof(s)));
} else {
return setValue(reinterpret_cast<const uint8_t*>(&s), sizeof(s));
Expand All @@ -372,6 +448,7 @@ class NimBLEAttValue {
*/
template <typename T>
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;
Expand All @@ -392,16 +469,30 @@ class NimBLEAttValue {
uint8_t operator[](int pos) const;

/** @brief Operator; Get the value as a std::vector<uint8_t>. */
operator std::vector<uint8_t>() const { return std::vector<uint8_t>(m_attr_value, m_attr_value + m_attr_len); }
operator std::vector<uint8_t>() const {
Lock lock;
return std::vector<uint8_t>(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<char*>(m_attr_value), m_attr_len); }
operator std::string() const {
Lock lock;
return std::string(reinterpret_cast<char*>(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<uint8_t> 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) {
Expand All @@ -417,6 +508,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;
}

Expand All @@ -425,7 +517,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<char*>(m_attr_value)); }
operator String() const {
Lock lock;
return String(reinterpret_cast<char*>(m_attr_value));
}
# endif
};

Expand Down
Loading
Loading