From 5653eb6906367bfd8132afb3f39052d15e76dc35 Mon Sep 17 00:00:00 2001 From: Nick Korotysh Date: Thu, 21 Aug 2025 11:06:56 +0200 Subject: [PATCH 1/4] refactor(ble): Pass primitive types by value There is no need to pass primitive types by reference, especially by non-const reference as it was in BLECharacteristic. Pass primitive types by value instead, as it should be, and used everywhere else except this one odd class. This allows allows to pass function result as `setValue()` argument without creating temporary variable. --- libraries/BLE/src/BLECharacteristic.cpp | 10 +++++----- libraries/BLE/src/BLECharacteristic.h | 10 +++++----- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/libraries/BLE/src/BLECharacteristic.cpp b/libraries/BLE/src/BLECharacteristic.cpp index 0234cd11cca..88a574fa177 100644 --- a/libraries/BLE/src/BLECharacteristic.cpp +++ b/libraries/BLE/src/BLECharacteristic.cpp @@ -375,14 +375,14 @@ void BLECharacteristic::setValue(String value) { setValue((uint8_t *)(value.c_str()), value.length()); } // setValue -void BLECharacteristic::setValue(uint16_t &data16) { +void BLECharacteristic::setValue(uint16_t data16) { uint8_t temp[2]; temp[0] = data16; temp[1] = data16 >> 8; setValue(temp, 2); } // setValue -void BLECharacteristic::setValue(uint32_t &data32) { +void BLECharacteristic::setValue(uint32_t data32) { uint8_t temp[4]; temp[0] = data32; temp[1] = data32 >> 8; @@ -391,7 +391,7 @@ void BLECharacteristic::setValue(uint32_t &data32) { setValue(temp, 4); } // setValue -void BLECharacteristic::setValue(int &data32) { +void BLECharacteristic::setValue(int data32) { uint8_t temp[4]; temp[0] = data32; temp[1] = data32 >> 8; @@ -400,12 +400,12 @@ void BLECharacteristic::setValue(int &data32) { setValue(temp, 4); } // setValue -void BLECharacteristic::setValue(float &data32) { +void BLECharacteristic::setValue(float data32) { float temp = data32; setValue((uint8_t *)&temp, 4); } // setValue -void BLECharacteristic::setValue(double &data64) { +void BLECharacteristic::setValue(double data64) { double temp = data64; setValue((uint8_t *)&temp, 8); } // setValue diff --git a/libraries/BLE/src/BLECharacteristic.h b/libraries/BLE/src/BLECharacteristic.h index 27df5a30c3e..580f71dc5e7 100644 --- a/libraries/BLE/src/BLECharacteristic.h +++ b/libraries/BLE/src/BLECharacteristic.h @@ -186,11 +186,11 @@ class BLECharacteristic { 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(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); From c6517e463a53445fa389157465191419e579887b Mon Sep 17 00:00:00 2001 From: Nick Korotysh Date: Thu, 21 Aug 2025 11:17:26 +0200 Subject: [PATCH 2/4] refactor(ble): Simplify setValue() for primitive types ESP32 is little-endian architecture, so the code that filled temporary arrays created from integers basically did nothing. Pass primitives as 'buffers' to generic `setValue()` instead. Also use sizeof() instead of hardcoded sizes as the argument. --- libraries/BLE/src/BLECharacteristic.cpp | 25 +++++-------------------- 1 file changed, 5 insertions(+), 20 deletions(-) diff --git a/libraries/BLE/src/BLECharacteristic.cpp b/libraries/BLE/src/BLECharacteristic.cpp index 88a574fa177..ebf1e48a80d 100644 --- a/libraries/BLE/src/BLECharacteristic.cpp +++ b/libraries/BLE/src/BLECharacteristic.cpp @@ -376,38 +376,23 @@ void BLECharacteristic::setValue(String value) { } // setValue void BLECharacteristic::setValue(uint16_t data16) { - uint8_t temp[2]; - temp[0] = data16; - temp[1] = data16 >> 8; - setValue(temp, 2); + setValue(reinterpret_cast(&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); + setValue(reinterpret_cast(&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); + setValue(reinterpret_cast(&data32), sizeof(data32)); } // setValue void BLECharacteristic::setValue(float data32) { - float temp = data32; - setValue((uint8_t *)&temp, 4); + setValue(reinterpret_cast(&data32), sizeof(data32)); } // setValue void BLECharacteristic::setValue(double data64) { - double temp = data64; - setValue((uint8_t *)&temp, 8); + setValue(reinterpret_cast(&data64), sizeof(data64)); } // setValue /** From 273eedffc22230983e441ae414e5df61a65ed548 Mon Sep 17 00:00:00 2001 From: Nick Korotysh Date: Thu, 21 Aug 2025 12:29:12 +0200 Subject: [PATCH 3/4] refactor(ble): Pass pointer to const data to setValue() There is no technical need to pass non-const data there. Also it is a common practice to pass const data to the functions with write semantics, as they not supposed to modify the data. This allows to pass the data from read-only sources without removing the constness what is not always safe. --- libraries/BLE/src/BLECharacteristic.cpp | 2 +- libraries/BLE/src/BLECharacteristic.h | 2 +- libraries/BLE/src/BLEDescriptor.cpp | 2 +- libraries/BLE/src/BLEDescriptor.h | 2 +- libraries/BLE/src/BLEValue.cpp | 4 ++-- libraries/BLE/src/BLEValue.h | 4 ++-- 6 files changed, 8 insertions(+), 8 deletions(-) diff --git a/libraries/BLE/src/BLECharacteristic.cpp b/libraries/BLE/src/BLECharacteristic.cpp index ebf1e48a80d..29d6490f3b0 100644 --- a/libraries/BLE/src/BLECharacteristic.cpp +++ b/libraries/BLE/src/BLECharacteristic.cpp @@ -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 diff --git a/libraries/BLE/src/BLECharacteristic.h b/libraries/BLE/src/BLECharacteristic.h index 580f71dc5e7..fc546439006 100644 --- a/libraries/BLE/src/BLECharacteristic.h +++ b/libraries/BLE/src/BLECharacteristic.h @@ -184,7 +184,7 @@ class BLECharacteristic { void indicate(); void notify(bool is_notification = true); void setCallbacks(BLECharacteristicCallbacks *pCallbacks); - void setValue(uint8_t *data, size_t size); + void setValue(const uint8_t *data, size_t size); void setValue(String value); void setValue(uint16_t data16); void setValue(uint32_t data32); diff --git a/libraries/BLE/src/BLEDescriptor.cpp b/libraries/BLE/src/BLEDescriptor.cpp index 51c77bb9b49..9efd8f05083 100644 --- a/libraries/BLE/src/BLEDescriptor.cpp +++ b/libraries/BLE/src/BLEDescriptor.cpp @@ -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; diff --git a/libraries/BLE/src/BLEDescriptor.h b/libraries/BLE/src/BLEDescriptor.h index d0d34b24388..49ee15939af 100644 --- a/libraries/BLE/src/BLEDescriptor.h +++ b/libraries/BLE/src/BLEDescriptor.h @@ -94,7 +94,7 @@ 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(const 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. String toString(); // Convert the descriptor to a string representation. diff --git a/libraries/BLE/src/BLEValue.cpp b/libraries/BLE/src/BLEValue.cpp index efc97697baa..b09a5775c8d 100644 --- a/libraries/BLE/src/BLEValue.cpp +++ b/libraries/BLE/src/BLEValue.cpp @@ -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 @@ -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 diff --git a/libraries/BLE/src/BLEValue.h b/libraries/BLE/src/BLEValue.h index 56a7a5bc4ec..cedc93e29cd 100644 --- a/libraries/BLE/src/BLEValue.h +++ b/libraries/BLE/src/BLEValue.h @@ -35,7 +35,7 @@ class BLEValue { BLEValue(); void addPart(String part); - void addPart(uint8_t *pData, size_t length); + void addPart(const uint8_t *pData, size_t length); void cancel(); void commit(); uint8_t *getData(); @@ -44,7 +44,7 @@ class BLEValue { String getValue(); void setReadOffset(uint16_t readOffset); void setValue(String value); - void setValue(uint8_t *pData, size_t length); + void setValue(const uint8_t *pData, size_t length); private: /*************************************************************************** From 5edf46419266a9abdb97370eda5c89003ee720f1 Mon Sep 17 00:00:00 2001 From: Nick Korotysh Date: Thu, 21 Aug 2025 12:40:17 +0200 Subject: [PATCH 4/4] refactor(ble): Avoid unnecessary String copies Pass String by const reference to many `setValue()` functions to avoid copying it. Even there are not so much data inside these strings, it just looks too odd to copy them every time. Also this change decreases binary size (~200 bytes in my case). --- libraries/BLE/src/BLE2901.cpp | 2 +- libraries/BLE/src/BLE2901.h | 2 +- libraries/BLE/src/BLECharacteristic.cpp | 4 ++-- libraries/BLE/src/BLECharacteristic.h | 2 +- libraries/BLE/src/BLEDescriptor.cpp | 4 ++-- libraries/BLE/src/BLEDescriptor.h | 2 +- libraries/BLE/src/BLEValue.cpp | 4 ++-- libraries/BLE/src/BLEValue.h | 4 ++-- 8 files changed, 12 insertions(+), 12 deletions(-) diff --git a/libraries/BLE/src/BLE2901.cpp b/libraries/BLE/src/BLE2901.cpp index 1fa4857ac33..3b5edb0b59f 100644 --- a/libraries/BLE/src/BLE2901.cpp +++ b/libraries/BLE/src/BLE2901.cpp @@ -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; diff --git a/libraries/BLE/src/BLE2901.h b/libraries/BLE/src/BLE2901.h index 21e7cc9398c..87ce76090fe 100644 --- a/libraries/BLE/src/BLE2901.h +++ b/libraries/BLE/src/BLE2901.h @@ -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 */ diff --git a/libraries/BLE/src/BLECharacteristic.cpp b/libraries/BLE/src/BLECharacteristic.cpp index 29d6490f3b0..83475820b58 100644 --- a/libraries/BLE/src/BLECharacteristic.cpp +++ b/libraries/BLE/src/BLECharacteristic.cpp @@ -371,8 +371,8 @@ void BLECharacteristic::setValue(const 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(value.c_str()), value.length()); } // setValue void BLECharacteristic::setValue(uint16_t data16) { diff --git a/libraries/BLE/src/BLECharacteristic.h b/libraries/BLE/src/BLECharacteristic.h index fc546439006..dc87177644f 100644 --- a/libraries/BLE/src/BLECharacteristic.h +++ b/libraries/BLE/src/BLECharacteristic.h @@ -185,7 +185,7 @@ class BLECharacteristic { void notify(bool is_notification = true); void setCallbacks(BLECharacteristicCallbacks *pCallbacks); void setValue(const uint8_t *data, size_t size); - void setValue(String value); + void setValue(const String &value); void setValue(uint16_t data16); void setValue(uint32_t data32); void setValue(int data32); diff --git a/libraries/BLE/src/BLEDescriptor.cpp b/libraries/BLE/src/BLEDescriptor.cpp index 9efd8f05083..2452f3cd45b 100644 --- a/libraries/BLE/src/BLEDescriptor.cpp +++ b/libraries/BLE/src/BLEDescriptor.cpp @@ -203,8 +203,8 @@ void BLEDescriptor::setValue(const 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(value.c_str()), value.length()); } // setValue void BLEDescriptor::setAccessPermissions(uint8_t perm) { diff --git a/libraries/BLE/src/BLEDescriptor.h b/libraries/BLE/src/BLEDescriptor.h index 49ee15939af..83008743ae5 100644 --- a/libraries/BLE/src/BLEDescriptor.h +++ b/libraries/BLE/src/BLEDescriptor.h @@ -95,7 +95,7 @@ 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(const 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 String &value); // Set the value of the descriptor as a data buffer. String toString(); // Convert the descriptor to a string representation. diff --git a/libraries/BLE/src/BLEValue.cpp b/libraries/BLE/src/BLEValue.cpp index b09a5775c8d..6559766cfea 100644 --- a/libraries/BLE/src/BLEValue.cpp +++ b/libraries/BLE/src/BLEValue.cpp @@ -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 @@ -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 diff --git a/libraries/BLE/src/BLEValue.h b/libraries/BLE/src/BLEValue.h index cedc93e29cd..6a1b7cfd7e4 100644 --- a/libraries/BLE/src/BLEValue.h +++ b/libraries/BLE/src/BLEValue.h @@ -34,7 +34,7 @@ class BLEValue { ***************************************************************************/ BLEValue(); - void addPart(String part); + void addPart(const String &part); void addPart(const uint8_t *pData, size_t length); void cancel(); void commit(); @@ -43,7 +43,7 @@ class BLEValue { uint16_t getReadOffset(); String getValue(); void setReadOffset(uint16_t readOffset); - void setValue(String value); + void setValue(const String &value); void setValue(const uint8_t *pData, size_t length); private: