Skip to content

BLE: BLECharacteristic::setValue() cleanup and optimization #11751

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

BLE: BLECharacteristic::setValue() cleanup and optimization #11751

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5653eb6
refactor(ble): Pass primitive types by value
Kolcha Aug 21, 2025
c6517e4
refactor(ble): Simplify setValue() for primitive types
Kolcha Aug 21, 2025
273eedf
refactor(ble): Pass pointer to const data to setValue()
Kolcha Aug 21, 2025
5edf464
refactor(ble): Avoid unnecessary String copies
Kolcha Aug 21, 2025
73ccce2
Merge branch 'master' into ble-cleanup
SuGlider Aug 22, 2025
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
2 changes: 1 addition & 1 deletion libraries/BLE/src/BLE2901.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ BLE2901::BLE2901() : BLEDescriptor(BLEUUID((uint16_t)BLE2901_UUID)) {}
/**
* @brief Set the Characteristic User Description
*/
void BLE2901::setDescription(String userDesc) {
void BLE2901::setDescription(const String &userDesc) {
if (userDesc.length() > ESP_GATT_MAX_ATTR_LEN) {
log_e("Size %d too large, must be no bigger than %d", userDesc.length(), ESP_GATT_MAX_ATTR_LEN);
return;
Expand Down
2 changes: 1 addition & 1 deletion libraries/BLE/src/BLE2901.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ class BLE2901 : public BLEDescriptor {
***************************************************************************/

BLE2901();
void setDescription(String desc);
void setDescription(const String &desc);
}; // BLE2901

#endif /* CONFIG_BLUEDROID_ENABLED || CONFIG_NIMBLE_ENABLED */
Expand Down
41 changes: 13 additions & 28 deletions libraries/BLE/src/BLECharacteristic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,7 @@ void BLECharacteristic::setReadProperty(bool value) {
* @param [in] data The data to set for the characteristic.
* @param [in] length The length of the data in bytes.
*/
void BLECharacteristic::setValue(uint8_t *data, size_t length) {
void BLECharacteristic::setValue(const uint8_t *data, size_t length) {
// The call to BLEUtils::buildHexData() doesn't output anything if the log level is not
// "VERBOSE". As it is quite CPU intensive, it is much better to not call it if not needed.
#if ARDUHAL_LOG_LEVEL >= ARDUHAL_LOG_LEVEL_VERBOSE
Expand All @@ -371,43 +371,28 @@ void BLECharacteristic::setValue(uint8_t *data, size_t length) {
* @param [in] Set the value of the characteristic.
* @return N/A.
*/
void BLECharacteristic::setValue(String value) {
setValue((uint8_t *)(value.c_str()), value.length());
void BLECharacteristic::setValue(const String &value) {
setValue(reinterpret_cast<const uint8_t *>(value.c_str()), value.length());
} // setValue

void BLECharacteristic::setValue(uint16_t &data16) {
uint8_t temp[2];
temp[0] = data16;
temp[1] = data16 >> 8;
setValue(temp, 2);
void BLECharacteristic::setValue(uint16_t data16) {
setValue(reinterpret_cast<uint8_t *>(&data16), sizeof(data16));
} // setValue

void BLECharacteristic::setValue(uint32_t &data32) {
uint8_t temp[4];
temp[0] = data32;
temp[1] = data32 >> 8;
temp[2] = data32 >> 16;
temp[3] = data32 >> 24;
setValue(temp, 4);
void BLECharacteristic::setValue(uint32_t data32) {
setValue(reinterpret_cast<uint8_t *>(&data32), sizeof(data32));
} // setValue

void BLECharacteristic::setValue(int &data32) {
uint8_t temp[4];
temp[0] = data32;
temp[1] = data32 >> 8;
temp[2] = data32 >> 16;
temp[3] = data32 >> 24;
setValue(temp, 4);
void BLECharacteristic::setValue(int data32) {
setValue(reinterpret_cast<uint8_t *>(&data32), sizeof(data32));
} // setValue

void BLECharacteristic::setValue(float &data32) {
float temp = data32;
setValue((uint8_t *)&temp, 4);
void BLECharacteristic::setValue(float data32) {
setValue(reinterpret_cast<uint8_t *>(&data32), sizeof(data32));
} // setValue

void BLECharacteristic::setValue(double &data64) {
double temp = data64;
setValue((uint8_t *)&temp, 8);
void BLECharacteristic::setValue(double data64) {
setValue(reinterpret_cast<uint8_t *>(&data64), sizeof(data64));
Copy link
Preview

Copilot AI Aug 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Casting away const from a value parameter creates undefined behavior. The data16 parameter is passed by value, so &data16 points to a temporary that will be destroyed. Use reinterpret_cast<const uint8_t *> and ensure the underlying setValue method can handle const data.

Copilot uses AI. Check for mistakes.

Copy link
Preview

Copilot AI Aug 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Casting away const from a value parameter creates undefined behavior. The data32 parameter is passed by value, so &data32 points to a temporary that will be destroyed. Use reinterpret_cast<const uint8_t *> and ensure the underlying setValue method can handle const data.

Copilot uses AI. Check for mistakes.

Copy link
Preview

Copilot AI Aug 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Casting away const from a value parameter creates undefined behavior. The data32 parameter is passed by value, so &data32 points to a temporary that will be destroyed. Use reinterpret_cast<const uint8_t *> and ensure the underlying setValue method can handle const data.

Copilot uses AI. Check for mistakes.

Copy link
Preview

Copilot AI Aug 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Casting away const from a value parameter creates undefined behavior. The data32 parameter is passed by value, so &data32 points to a temporary that will be destroyed. Use reinterpret_cast<const uint8_t *> and ensure the underlying setValue method can handle const data.

Copilot uses AI. Check for mistakes.

Copy link
Preview

Copilot AI Aug 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Casting away const from a value parameter creates undefined behavior. The data64 parameter is passed by value, so &data64 points to a temporary that will be destroyed. Use reinterpret_cast<const uint8_t *> and ensure the underlying setValue method can handle const data.

Suggested change
setValue(reinterpret_cast<uint8_t *>(&data64), sizeof(data64));
setValue(reinterpret_cast<const uint8_t *>(&data64), sizeof(data64));

Copilot uses AI. Check for mistakes.

} // setValue

/**
Expand Down
14 changes: 7 additions & 7 deletions libraries/BLE/src/BLECharacteristic.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,13 +184,13 @@ class BLECharacteristic {
void indicate();
void notify(bool is_notification = true);
void setCallbacks(BLECharacteristicCallbacks *pCallbacks);
void setValue(uint8_t *data, size_t size);
void setValue(String value);
void setValue(uint16_t &data16);
void setValue(uint32_t &data32);
void setValue(int &data32);
void setValue(float &data32);
void setValue(double &data64);
void setValue(const uint8_t *data, size_t size);
void setValue(const String &value);
void setValue(uint16_t data16);
void setValue(uint32_t data32);
void setValue(int data32);
void setValue(float data32);
void setValue(double data64);
String toString();
uint16_t getHandle();
void setAccessPermissions(uint8_t perm);
Expand Down
6 changes: 3 additions & 3 deletions libraries/BLE/src/BLEDescriptor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ void BLEDescriptor::setHandle(uint16_t handle) {
* @param [in] data The data to set for the descriptor.
* @param [in] length The length of the data in bytes.
*/
void BLEDescriptor::setValue(uint8_t *data, size_t length) {
void BLEDescriptor::setValue(const uint8_t *data, size_t length) {
if (length > m_value.attr_max_len) {
log_e("Size %d too large, must be no bigger than %d", length, m_value.attr_max_len);
return;
Expand All @@ -203,8 +203,8 @@ void BLEDescriptor::setValue(uint8_t *data, size_t length) {
* @brief Set the value of the descriptor.
* @param [in] value The value of the descriptor in string form.
*/
void BLEDescriptor::setValue(String value) {
setValue((uint8_t *)value.c_str(), value.length());
void BLEDescriptor::setValue(const String &value) {
setValue(reinterpret_cast<const uint8_t *>(value.c_str()), value.length());
} // setValue

void BLEDescriptor::setAccessPermissions(uint8_t perm) {
Expand Down
4 changes: 2 additions & 2 deletions libraries/BLE/src/BLEDescriptor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,8 @@ class BLEDescriptor {

void setAccessPermissions(uint8_t perm); // Set the permissions of the descriptor.
void setCallbacks(BLEDescriptorCallbacks *pCallbacks); // Set callbacks to be invoked for the descriptor.
void setValue(uint8_t *data, size_t size); // Set the value of the descriptor as a pointer to data.
void setValue(String value); // Set the value of the descriptor as a data buffer.
void setValue(const uint8_t *data, size_t size); // Set the value of the descriptor as a pointer to data.
void setValue(const String &value); // Set the value of the descriptor as a data buffer.

String toString(); // Convert the descriptor to a string representation.

Expand Down
8 changes: 4 additions & 4 deletions libraries/BLE/src/BLEValue.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ BLEValue::BLEValue() {
* The accumulation is a growing set of data that is added to until a commit or cancel.
* @param [in] part A message part being added.
*/
void BLEValue::addPart(String part) {
void BLEValue::addPart(const String &part) {
log_v(">> addPart: length=%d", part.length());
m_accumulation += part;
} // addPart
Expand All @@ -48,7 +48,7 @@ void BLEValue::addPart(String part) {
* @param [in] pData A message part being added.
* @param [in] length The number of bytes being added.
*/
void BLEValue::addPart(uint8_t *pData, size_t length) {
void BLEValue::addPart(const uint8_t *pData, size_t length) {
log_v(">> addPart: length=%d", length);
m_accumulation += String((char *)pData, length);
} // addPart
Expand Down Expand Up @@ -121,7 +121,7 @@ void BLEValue::setReadOffset(uint16_t readOffset) {
/**
* @brief Set the current value.
*/
void BLEValue::setValue(String value) {
void BLEValue::setValue(const String &value) {
m_value = value;
} // setValue

Expand All @@ -130,7 +130,7 @@ void BLEValue::setValue(String value) {
* @param [in] pData The data for the current value.
* @param [in] The length of the new current value.
*/
void BLEValue::setValue(uint8_t *pData, size_t length) {
void BLEValue::setValue(const uint8_t *pData, size_t length) {
m_value = String((char *)pData, length);
} // setValue

Expand Down
8 changes: 4 additions & 4 deletions libraries/BLE/src/BLEValue.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,17 +34,17 @@ class BLEValue {
***************************************************************************/

BLEValue();
void addPart(String part);
void addPart(uint8_t *pData, size_t length);
void addPart(const String &part);
void addPart(const uint8_t *pData, size_t length);
void cancel();
void commit();
uint8_t *getData();
size_t getLength();
uint16_t getReadOffset();
String getValue();
void setReadOffset(uint16_t readOffset);
void setValue(String value);
void setValue(uint8_t *pData, size_t length);
void setValue(const String &value);
void setValue(const uint8_t *pData, size_t length);

private:
/***************************************************************************
Expand Down
Loading