Merge branch 'h2zero:master' into feat/aggregate-format-2905

Author: srgg

docs/Usage_tips.md

@@ -1,11 +1,8 @@
 # Usage Tips
 
-## Put BLE functions in a task running on the NimBLE stack core
+## Threadsafety
 
-When commands are sent to the stack from a different core they can experience delays in execution.  
-This library detects this and invokes the esp32 IPC to reroute these commands through the correct core but this also increases overhead.  
-Therefore it is highly recommended to create tasks for BLE to run on the same core, the macro `CONFIG_BT_NIMBLE_PINNED_TO_CORE` can be used to set the core.
-<br/>  
+This library is threadsafe. Attribues can be manipulated freely.
 
 ## Do not delete client instances unless necessary or unused
 
@@ -32,6 +29,41 @@ or nullptr such as when  calling `NimBLEClient::getService`. The latter being a
 Most of the functions in this library return something that should be checked before proceeding.  
 <br/>  
 
+## Persisted bonds can be lost due to low MAX_CCCDS
+
+The symptom: CONFIG_BT_NIMBLE_MAX_BONDS is set to N, but a smaller number of bonds is preserved, perhaps even a single bond. The limitation of persisted bonds can be observed via NimBLEDevice::getNumBonds(). The number may not reach CONFIG_BT_NIMBLE_MAX_BONDS.
+
+Cause: For each bond, NimBLE persists each of the CCCD (client characteristic configuration descriptor) values that have been subscribed
+to by the client. If CONFIG_BT_NIMBLE_MAX_CCCDS is too low, the older CCCD values are overwritten by the newer ones. The loss of the older
+CCCDs values results in those bonds being lost.
+
+Fix: Increase CONFIG_BT_NIMBLE_MAX_CCCDS. These take approximately 40 bytes in NVS, 2 bytes for the CCCD value and the NVS metadata overhead. The value of
+CONFIG_BT_NIMBLE_MAX_CCCDS should conservatively be no less than (CONFIG_BT_NIMBLE_MAX_BONDS * {maximum number of characteristics that can be subscribed to}).
+
+## Device 'Local Name'
+
+'Local name' refers to how the device is seen and displayed.
+
+A devices 'Local name' can be thought of as coming from two places, the <i>Advertising "Local name"</i> and the <i>the GATT Device Name</i>.
+
+### Advertising "Local name"
+
+Field found in the advertising data payload. Value is set via NimBLEAdvertising::setName().
+
+### GATT Device Name
+
+Characteristic UUID 0x2A00 in the Generic Access service. Set via NimBLEDevice::init() or NimBLEDevice::setDeviceName().
+
+This characteristic is read <b>after</b> connecting to the device.
+
+### Important considerations
+
+* OSes cache the <i>'GATT Device Name'</i>.
+* OSes update the device name based on the <i>'GATT Device Name'</i> after connecting to a device. This means that if you set the <i>Advertising 'Local name'</i> to "ABCD" but the <i>'GATT Device Name'</i> to "12345", the device will be seen as "ABCD" until connecting to the device, at which time the devices name will change to "12345".
+* If no <i>'Advertising "Local name"'</i> is set, OSes, such as iOS, may display the devices name as 'Unnamed' until the device is connected to, at which time the <i>'GATT Device Name'</i> is read and used instead.
+
+It is recommended that both <i>'Advertising "Local name"'</i> <b>and</b> <i>'GATT Device Name'</i> be set appropriately, after considering the above described behavior.
+
 ## There will be bugs - please report them
 
 No code is bug free and unit testing will not find them all on it's own. If you encounter a bug, please report it along with any logs and decoded backtrace if applicable.  

src/NimBLEAttValue.h

@@ -73,6 +73,14 @@ template <typename T>
 struct Has_c_str_length<T, decltype(void(std::declval<T&>().c_str())), decltype(void(std::declval<T&>().length()))>
     : std::true_type {};
 
+/* Used to determine if the type passed to a template has a value_type member (std::vector, std::array, std::string, etc.). */
+template <typename T, typename = void>
+struct Has_value_type : std::false_type {};
+
+template <typename T>
+struct Has_value_type<T, decltype(void(sizeof(typename T::value_type)))>
+    : std::true_type {};
+
 /**
  * @brief A specialized container class to hold BLE attribute values.
  * @details This class is designed to be more memory efficient than using\n
@@ -274,13 +282,32 @@ class NimBLEAttValue {
     /**
      * @brief Template to set value to the value of <type\>val.
      * @param [in] v The <type\>value to set.
-     * @details Only used if the <type\> has a `data()` and `size()` method.
+     * @details Only used if the <type\> has a `data()` and `size()` method with `value_type`.
+     * Correctly calculates byte size for containers with multi-byte element types.
+     */
+    template <typename T>
+#  ifdef _DOXYGEN_
+    bool
+#  else
+    typename std::enable_if<Has_data_size<T>::value && Has_value_type<T>::value, bool>::type
+#  endif
+    setValue(const T& v) {
+        return setValue(
+            reinterpret_cast<const uint8_t*>(v.data()),
+            v.size() * sizeof(typename T::value_type)
+        );
+    }
+
+    /**
+     * @brief Template to set value to the value of <type\>val.
+     * @param [in] v The <type\>value to set.
+     * @details Only used if the <type\> has a `data()` and `size()` method without `value_type`.
      */
     template <typename T>
 #  ifdef _DOXYGEN_
     bool
 #  else
-    typename std::enable_if<Has_data_size<T>::value, bool>::type
+    typename std::enable_if<Has_data_size<T>::value && !Has_value_type<T>::value, bool>::type
 #  endif
     setValue(const T& v) {
         return setValue(reinterpret_cast<const uint8_t*>(v.data()), v.size());
@@ -295,7 +322,11 @@ class NimBLEAttValue {
     template <typename T>
     typename std::enable_if<!std::is_pointer<T>::value, bool>::type setValue(const T& s) {
         if constexpr (Has_data_size<T>::value) {
-            return setValue(reinterpret_cast<const uint8_t*>(s.data()), s.size());
+            if constexpr (Has_value_type<T>::value) {
+                return setValue(reinterpret_cast<const uint8_t*>(s.data()), s.size() * sizeof(typename T::value_type));
+            } else {
+                return setValue(reinterpret_cast<const uint8_t*>(s.data()), s.size());
+            }
         } else if constexpr (Has_c_str_length<T>::value) {
             return setValue(reinterpret_cast<const uint8_t*>(s.c_str()), s.length());
         } else {

src/NimBLECharacteristic.h

@@ -115,15 +115,35 @@ class NimBLECharacteristic : public NimBLELocalValueAttribute {
     }
 
     /**
-     * @brief Template to send a notification with a value from a class that has a data() and size() method.
+         * @brief Template to send a notification with a value from a class that has a data() and size() method with value_type.
      * @param [in] v The value to send.
      * @param [in] connHandle Optional, a connection handle to send the notification to.
+     * @details Correctly calculates byte size for containers with multi-byte element types.
      */
     template <typename T>
 #  ifdef _DOXYGEN_
     bool
 #  else
-    typename std::enable_if<Has_data_size<T>::value, bool>::type
+    typename std::enable_if<Has_data_size<T>::value && Has_value_type<T>::value, bool>::type
+#  endif
+    notify(const T& v, uint16_t connHandle = BLE_HS_CONN_HANDLE_NONE) const {
+        return notify(
+            reinterpret_cast<const uint8_t*>(v.data()),
+            v.size() * sizeof(typename T::value_type),
+            connHandle
+        );
+    }
+
+    /**
+     * @brief Template to send a notification with a value from a class that has a data() and size() method without value_type.
+     * @param [in] v The value to send.
+     * @param [in] connHandle Optional, a connection handle to send the notification to.
+     */
+    template <typename T>
+#  ifdef _DOXYGEN_
+    bool
+#  else
+    typename std::enable_if<Has_data_size<T>::value && !Has_value_type<T>::value, bool>::type
 #  endif
     notify(const T& v, uint16_t connHandle = BLE_HS_CONN_HANDLE_NONE) const {
         return notify(reinterpret_cast<const uint8_t*>(v.data()), v.size(), connHandle);
@@ -163,15 +183,35 @@ class NimBLECharacteristic : public NimBLELocalValueAttribute {
     }
 
     /**
-     * @brief Template to send a indication with a value from a class that has a data() and size() method.
+     * @brief Template to send a indication with a value from a class that has a data() and size() method with value_type.
+     * @param [in] v The value to send.
+     * @param [in] connHandle Optional, a connection handle to send the notification to.
+     * @details Correctly calculates byte size for containers with multi-byte element types.
+     */
+    template <typename T>
+#  ifdef _DOXYGEN_
+    bool
+#  else
+    typename std::enable_if<Has_data_size<T>::value && Has_value_type<T>::value, bool>::type
+#  endif
+    indicate(const T& v, uint16_t connHandle = BLE_HS_CONN_HANDLE_NONE) const {
+        return indicate(
+            reinterpret_cast<const uint8_t*>(v.data()),
+            v.size() * sizeof(typename T::value_type),
+            connHandle
+        );
+    }
+
+    /**
+     * @brief Template to send a indication with a value from a class that has a data() and size() method without value_type.
      * @param [in] v The value to send.
      * @param [in] connHandle Optional, a connection handle to send the notification to.
      */
     template <typename T>
 #  ifdef _DOXYGEN_
     bool
 #  else
-    typename std::enable_if<Has_data_size<T>::value, bool>::type
+    typename std::enable_if<Has_data_size<T>::value && !Has_value_type<T>::value, bool>::type
 #  endif
     indicate(const T& v, uint16_t connHandle = BLE_HS_CONN_HANDLE_NONE) const {
         return indicate(reinterpret_cast<const uint8_t*>(v.data()), v.size(), connHandle);
@@ -193,7 +233,11 @@ class NimBLECharacteristic : public NimBLELocalValueAttribute {
     typename std::enable_if<!std::is_pointer<T>::value && !std::is_array<T>::value, bool>::type notify(
         const T& value, uint16_t connHandle = BLE_HS_CONN_HANDLE_NONE) const {
         if constexpr (Has_data_size<T>::value) {
-            return notify(reinterpret_cast<const uint8_t*>(value.data()), value.size(), connHandle);
+            if constexpr (Has_value_type<T>::value) {
+                return notify(reinterpret_cast<const uint8_t*>(value.data()), value.size() * sizeof(typename T::value_type), connHandle);
+            } else {
+                return notify(reinterpret_cast<const uint8_t*>(value.data()), value.size(), connHandle);
+            }
         } else if constexpr (Has_c_str_length<T>::value) {
             return notify(reinterpret_cast<const uint8_t*>(value.c_str()), value.length(), connHandle);
         } else {
@@ -215,7 +259,11 @@ class NimBLECharacteristic : public NimBLELocalValueAttribute {
     typename std::enable_if<!std::is_pointer<T>::value && !std::is_array<T>::value, bool>::type indicate(
         const T& value, uint16_t connHandle = BLE_HS_CONN_HANDLE_NONE) const {
         if constexpr (Has_data_size<T>::value) {
-            return indicate(reinterpret_cast<const uint8_t*>(value.data()), value.size(), connHandle);
+            if constexpr (Has_value_type<T>::value) {
+                return indicate(reinterpret_cast<const uint8_t*>(value.data()), value.size() * sizeof(typename T::value_type), connHandle);
+            } else {
+                return indicate(reinterpret_cast<const uint8_t*>(value.data()), value.size(), connHandle);
+            }
         } else if constexpr (Has_c_str_length<T>::value) {
             return indicate(reinterpret_cast<const uint8_t*>(value.c_str()), value.length(), connHandle);
         } else {

src/NimBLEClient.cpp

@@ -940,6 +940,11 @@ int NimBLEClient::handleGapEvent(struct ble_gap_event* event, void* arg) {
                 pClient = NimBLEDevice::getClientByPeerAddress(event->disconnect.conn.peer_id_addr);
             }
 
+            // try by connection handle
+            if (pClient == nullptr) {
+                pClient = NimBLEDevice::getClientByHandle(event->disconnect.conn.conn_handle);
+            }
+
             if (pClient == nullptr) {
                 NIMBLE_LOGE(LOG_TAG, "Disconnected client not found, conn_handle=%d",
                             event->disconnect.conn.conn_handle);

src/NimBLERemoteValueAttribute.h

@@ -109,13 +109,34 @@ class NimBLERemoteValueAttribute : public NimBLEValueAttribute, public NimBLEAtt
      * @brief Template to set the remote characteristic value to <type\>val.
      * @param [in] v The value to write.
      * @param [in] response True == request write response.
-     * @details Only used if the <type\> has a `data()` and `size()` method.
+     * @details Only used if the <type\> has a `data()` and `size()` method with `value_type`.
+     * Correctly calculates byte size for containers with multi-byte element types.
      */
     template <typename T>
 #  ifdef _DOXYGEN_
     bool
 #  else
-    typename std::enable_if<Has_data_size<T>::value, bool>::type
+    typename std::enable_if<Has_data_size<T>::value && Has_value_type<T>::value, bool>::type
+#  endif
+    writeValue(const T& v, bool response = false) const {
+        return writeValue(
+            reinterpret_cast<const uint8_t*>(v.data()),
+            v.size() * sizeof(typename T::value_type),
+            response
+        );
+    }
+
+    /**
+     * @brief Template to set the remote characteristic value to <type\>val.
+     * @param [in] v The value to write.
+     * @param [in] response True == request write response.
+     * @details Only used if the <type\> has a `data()` and `size()` method without `value_type`.
+     */
+    template <typename T>
+#  ifdef _DOXYGEN_
+    bool
+#  else
+    typename std::enable_if<Has_data_size<T>::value && !Has_value_type<T>::value, bool>::type
 #  endif
     writeValue(const T& v, bool response = false) const {
         return writeValue(reinterpret_cast<const uint8_t*>(v.data()), v.size(), response);
@@ -131,7 +152,11 @@ class NimBLERemoteValueAttribute : public NimBLEValueAttribute, public NimBLEAtt
     template <typename T>
     typename std::enable_if<!std::is_pointer<T>::value, bool>::type writeValue(const T& v, bool response = false) const {
         if constexpr (Has_data_size<T>::value) {
-            return writeValue(reinterpret_cast<const uint8_t*>(v.data()), v.size(), response);
+            if constexpr (Has_value_type<T>::value) {
+                return writeValue(reinterpret_cast<const uint8_t*>(v.data()), v.size() * sizeof(typename T::value_type), response);
+            } else {
+                return writeValue(reinterpret_cast<const uint8_t*>(v.data()), v.size(), response);
+            }
         } else if constexpr (Has_c_str_length<T>::value) {
             return writeValue(reinterpret_cast<const uint8_t*>(v.c_str()), v.length(), response);
         } else {

src/NimBLEServer.cpp

@@ -502,6 +502,15 @@ int NimBLEServer::handleGapEvent(ble_gap_event* event, void* arg) {
             break;
         } // BLE_GAP_EVENT_NOTIFY_TX
 
+# if MYNEWT_VAL(BLE_ROLE_CENTRAL)
+        case BLE_GAP_EVENT_NOTIFY_RX: {
+            if (pServer->m_pClient && pServer->m_pClient->m_connHandle == event->notify_rx.conn_handle) {
+                NimBLEClient::handleGapEvent(event, pServer->m_pClient);
+            }
+            break;
+        } // BLE_GAP_EVENT_NOTIFY_RX
+# endif
+
         case BLE_GAP_EVENT_ADV_COMPLETE: {
 # if MYNEWT_VAL(BLE_EXT_ADV) && MYNEWT_VAL(BLE_ROLE_BROADCASTER)
             case BLE_GAP_EVENT_SCAN_REQ_RCVD:

src/nimble/esp_port/esp-hci/src/esp_nimble_hci.c

@@ -167,14 +167,14 @@ static void ble_hci_rx_acl(uint8_t *data, uint16_t len)
         m = ble_transport_alloc_acl_from_ll();
 
         if (!m) {
-            esp_rom_printf("Failed to allocate buffer, retrying ");
-	    /* Give some time to free buffer and try again */
-	    vTaskDelay(1);
-	}
-    }while(!m);
+            esp_rom_printf("Failed to allocate buffer, retrying\n");
+            /* Give some time to free buffer and try again */
+            vTaskDelay(pdMS_TO_TICKS(10));
+	    }
+    } while(!m);
 
     if ((rc = os_mbuf_append(m, data, len)) != 0) {
-        esp_rom_printf("%s failed to os_mbuf_append; rc = %d", __func__, rc);
+        esp_rom_printf("%s failed to os_mbuf_append; rc = %d\n", __func__, rc);
         os_mbuf_free_chain(m);
         return;
     }