Add deinit() to _bleio.Characteristic

Espressif's Characteristics allocate memory that may be on the
supervisor heap. We need to free it when stopping BLE workflow.
Otherwise, we leak the memory and eventually safe mode.

Fixes #9599

Author: tannewt

devices/ble_hci/common-hal/_bleio/Characteristic.c

@@ -50,6 +50,17 @@ void common_hal_bleio_characteristic_construct(bleio_characteristic_obj_t *self,
     }
 }
 
+bool common_hal_bleio_characteristic_deinited(bleio_characteristic_obj_t *self) {
+    return self->handle == BLE_GATT_HANDLE_INVALID;
+}
+
+void common_hal_bleio_characteristic_deinit(bleio_characteristic_obj_t *self) {
+    if (common_hal_bleio_characteristic_deinited(self)) {
+        return;
+    }
+    self->handle = BLE_GATT_HANDLE_INVALID;
+}
+
 mp_obj_tuple_t *common_hal_bleio_characteristic_get_descriptors(bleio_characteristic_obj_t *self) {
     return mp_obj_new_tuple(self->descriptor_list->len, self->descriptor_list->items);
 }

ports/espressif/common-hal/_bleio/Characteristic.c

@@ -16,6 +16,7 @@
 #include "shared-bindings/_bleio/PacketBuffer.h"
 #include "shared-bindings/_bleio/Service.h"
 #include "shared-bindings/time/__init__.h"
+#include "supervisor/shared/safe_mode.h"
 
 #include "common-hal/_bleio/Adapter.h"
 #include "common-hal/_bleio/Service.h"
@@ -100,27 +101,20 @@ void common_hal_bleio_characteristic_construct(bleio_characteristic_obj_t *self,
         self->flags |= BLE_GATT_CHR_F_WRITE_AUTHEN;
     }
 
-    if (initial_value_bufinfo != NULL) {
-        // Copy the initial value if it's on the heap. Otherwise it's internal and we may not be able
-        // to allocate.
-        self->current_value_len = initial_value_bufinfo->len;
-        if (gc_alloc_possible()) {
-            self->current_value = m_malloc(max_length);
-            self->current_value_alloc = max_length;
-            if (gc_nbytes(initial_value_bufinfo->buf) > 0) {
-                memcpy(self->current_value, initial_value_bufinfo->buf, self->current_value_len);
-            }
-        } else {
-            self->current_value = initial_value_bufinfo->buf;
-            assert(self->current_value_len == max_length);
-        }
+    if (gc_alloc_possible()) {
+        self->current_value = m_malloc(max_length);
     } else {
         self->current_value = port_malloc(max_length, false);
-        if (self->current_value != NULL) {
-            self->current_value_alloc = max_length;
-            self->current_value_len = 0;
+        if (self->current_value == NULL) {
+            reset_into_safe_mode(SAFE_MODE_NO_HEAP);
         }
     }
+    self->current_value_alloc = max_length;
+    self->current_value_len = 0;
+
+    if (initial_value_bufinfo != NULL) {
+        common_hal_bleio_characteristic_set_value(self, initial_value_bufinfo);
+    }
 
     if (gc_alloc_possible()) {
         self->descriptor_list = mp_obj_new_list(0, NULL);
@@ -140,6 +134,26 @@ void common_hal_bleio_characteristic_construct(bleio_characteristic_obj_t *self,
     }
 }
 
+bool common_hal_bleio_characteristic_deinited(bleio_characteristic_obj_t *self) {
+    return self->current_value == NULL;
+}
+
+void common_hal_bleio_characteristic_deinit(bleio_characteristic_obj_t *self) {
+    if (common_hal_bleio_characteristic_deinited(self)) {
+        return;
+    }
+    if (self->current_value == NULL) {
+        return;
+    }
+
+    if (gc_nbytes(self->current_value) > 0) {
+        m_free(self->current_value);
+    } else {
+        port_free(self->current_value);
+    }
+    self->current_value = NULL;
+}
+
 mp_obj_tuple_t *common_hal_bleio_characteristic_get_descriptors(bleio_characteristic_obj_t *self) {
     if (self->descriptor_list == NULL) {
         return mp_const_empty_tuple;
@@ -245,21 +259,7 @@ void common_hal_bleio_characteristic_set_value(bleio_characteristic_obj_t *self,
         }
 
         self->current_value_len = bufinfo->len;
-        // If we've already allocated an internal buffer or the provided buffer
-        // is on the heap, then copy into the internal buffer.
-        if (self->current_value_alloc > 0 || gc_nbytes(bufinfo->buf) > 0) {
-            if (self->current_value_alloc < bufinfo->len) {
-                self->current_value = m_realloc(self->current_value, bufinfo->len);
-                // Get the number of bytes from the heap because it may be more
-                // than the len due to gc block size.
-                self->current_value_alloc = gc_nbytes(self->current_value);
-            }
-            memcpy(self->current_value, bufinfo->buf, bufinfo->len);
-        } else {
-            // Otherwise, use the provided buffer to delay any heap allocation.
-            self->current_value = bufinfo->buf;
-            self->current_value_alloc = 0;
-        }
+        memcpy(self->current_value, bufinfo->buf, self->current_value_len);
 
         ble_gatts_chr_updated(self->handle);
     }

ports/espressif/common-hal/_bleio/PacketBuffer.c

@@ -436,6 +436,7 @@ void common_hal_bleio_packet_buffer_deinit(bleio_packet_buffer_obj_t *self) {
         return;
     }
     bleio_characteristic_clear_observer(self->characteristic);
+    self->characteristic = NULL;
     ble_event_remove_handler(packet_buffer_on_ble_client_evt, self);
     ringbuf_deinit(&self->ringbuf);
 }

ports/nordic/common-hal/_bleio/Characteristic.c

@@ -109,6 +109,19 @@ void common_hal_bleio_characteristic_construct(bleio_characteristic_obj_t *self,
     }
 }
 
+bool common_hal_bleio_characteristic_deinited(bleio_characteristic_obj_t *self) {
+    return self->handle == BLE_GATT_HANDLE_INVALID;
+}
+
+void common_hal_bleio_characteristic_deinit(bleio_characteristic_obj_t *self) {
+    if (common_hal_bleio_characteristic_deinited(self)) {
+        return;
+    }
+    self->handle = BLE_GATT_HANDLE_INVALID;
+    // TODO: Can we remove this from the soft device? Right now we assume the
+    // reset clears things.
+}
+
 mp_obj_tuple_t *common_hal_bleio_characteristic_get_descriptors(bleio_characteristic_obj_t *self) {
     if (self->descriptor_list == NULL) {
         return mp_const_empty_tuple;

ports/silabs/common-hal/_bleio/Characteristic.c

@@ -155,6 +155,13 @@ void common_hal_bleio_characteristic_construct(
     }
 }
 
+bool common_hal_bleio_characteristic_deinited(bleio_characteristic_obj_t *self) {
+    return false;
+}
+
+void common_hal_bleio_characteristic_deinit(bleio_characteristic_obj_t *self) {
+}
+
 // A tuple of Descriptor that describe this characteristic
 mp_obj_tuple_t *common_hal_bleio_characteristic_get_descriptors(
     bleio_characteristic_obj_t *self) {

shared-bindings/_bleio/Characteristic.c

@@ -12,6 +12,8 @@
 #include "shared-bindings/_bleio/Characteristic.h"
 #include "shared-bindings/_bleio/Service.h"
 #include "shared-bindings/_bleio/UUID.h"
+#include "shared-bindings/util.h"
+
 
 //| class Characteristic:
 //|     """Stores information about a BLE service characteristic and allows reading
@@ -137,14 +139,29 @@ static mp_obj_t bleio_characteristic_add_to_service(size_t n_args, const mp_obj_
 static MP_DEFINE_CONST_FUN_OBJ_KW(bleio_characteristic_add_to_service_fun_obj, 1, bleio_characteristic_add_to_service);
 static MP_DEFINE_CONST_CLASSMETHOD_OBJ(bleio_characteristic_add_to_service_obj, MP_ROM_PTR(&bleio_characteristic_add_to_service_fun_obj));
 
+//|     def deinit(self) -> None:
+//|         """Deinitialises the Characteristic and releases any hardware resources for reuse."""
+//|         ...
+static mp_obj_t bleio_characteristic_deinit(mp_obj_t self_in) {
+    bleio_characteristic_obj_t *self = MP_OBJ_TO_PTR(self_in);
+    common_hal_bleio_characteristic_deinit(self);
+    return mp_const_none;
+}
+static MP_DEFINE_CONST_FUN_OBJ_1(bleio_characteristic_deinit_obj, bleio_characteristic_deinit);
 
+static void check_for_deinit(bleio_characteristic_obj_t *self) {
+    if (common_hal_bleio_characteristic_deinited(self)) {
+        raise_deinited_error();
+    }
+}
 
 //|     properties: int
 //|     """An int bitmask representing which properties are set, specified as bitwise or'ing of
 //|     of these possible values.
 //|     `BROADCAST`, `INDICATE`, `NOTIFY`, `READ`, `WRITE`, `WRITE_NO_RESPONSE`."""
 static mp_obj_t bleio_characteristic_get_properties(mp_obj_t self_in) {
     bleio_characteristic_obj_t *self = MP_OBJ_TO_PTR(self_in);
+    check_for_deinit(self);
 
     return MP_OBJ_NEW_SMALL_INT(common_hal_bleio_characteristic_get_properties(self));
 }
@@ -159,6 +176,7 @@ MP_PROPERTY_GETTER(bleio_characteristic_properties_obj,
 //|     Will be ``None`` if the 128-bit UUID for this characteristic is not known."""
 static mp_obj_t bleio_characteristic_get_uuid(mp_obj_t self_in) {
     bleio_characteristic_obj_t *self = MP_OBJ_TO_PTR(self_in);
+    check_for_deinit(self);
 
     bleio_uuid_obj_t *uuid = common_hal_bleio_characteristic_get_uuid(self);
     return uuid ? MP_OBJ_FROM_PTR(uuid) : mp_const_none;
@@ -172,6 +190,7 @@ MP_PROPERTY_GETTER(bleio_characteristic_uuid_obj,
 //|     """The value of this characteristic."""
 static mp_obj_t bleio_characteristic_get_value(mp_obj_t self_in) {
     bleio_characteristic_obj_t *self = MP_OBJ_TO_PTR(self_in);
+    check_for_deinit(self);
 
     uint8_t temp[512];
     size_t actual_len = common_hal_bleio_characteristic_get_value(self, temp, sizeof(temp));
@@ -181,6 +200,7 @@ static MP_DEFINE_CONST_FUN_OBJ_1(bleio_characteristic_get_value_obj, bleio_chara
 
 static mp_obj_t bleio_characteristic_set_value(mp_obj_t self_in, mp_obj_t value_in) {
     bleio_characteristic_obj_t *self = MP_OBJ_TO_PTR(self_in);
+    check_for_deinit(self);
 
     mp_buffer_info_t bufinfo;
     mp_get_buffer_raise(value_in, &bufinfo, MP_BUFFER_READ);
@@ -199,6 +219,7 @@ MP_PROPERTY_GETSET(bleio_characteristic_value_obj,
 //|     """The max length of this characteristic."""
 static mp_obj_t bleio_characteristic_get_max_length(mp_obj_t self_in) {
     bleio_characteristic_obj_t *self = MP_OBJ_TO_PTR(self_in);
+    check_for_deinit(self);
 
     return MP_OBJ_NEW_SMALL_INT(common_hal_bleio_characteristic_get_max_length(self));
 }
@@ -211,6 +232,8 @@ MP_PROPERTY_GETTER(bleio_characteristic_max_length_obj,
 //|     """A tuple of :py:class:`Descriptor` objects related to this characteristic. (read-only)"""
 static mp_obj_t bleio_characteristic_get_descriptors(mp_obj_t self_in) {
     bleio_characteristic_obj_t *self = MP_OBJ_TO_PTR(self_in);
+    check_for_deinit(self);
+
     // Return list as a tuple so user won't be able to change it.
     return MP_OBJ_FROM_PTR(common_hal_bleio_characteristic_get_descriptors(self));
 }
@@ -224,6 +247,7 @@ MP_PROPERTY_GETTER(bleio_characteristic_descriptors_obj,
 //|     """The Service this Characteristic is a part of."""
 static mp_obj_t bleio_characteristic_get_service(mp_obj_t self_in) {
     bleio_characteristic_obj_t *self = MP_OBJ_TO_PTR(self_in);
+    check_for_deinit(self);
 
     return common_hal_bleio_characteristic_get_service(self);
 }
@@ -241,6 +265,7 @@ MP_PROPERTY_GETTER(bleio_characteristic_service_obj,
 //|         ...
 static mp_obj_t bleio_characteristic_set_cccd(mp_uint_t n_args, const mp_obj_t *pos_args, mp_map_t *kw_args) {
     bleio_characteristic_obj_t *self = MP_OBJ_TO_PTR(pos_args[0]);
+    check_for_deinit(self);
 
     enum { ARG_notify, ARG_indicate };
     static const mp_arg_t allowed_args[] = {
@@ -258,6 +283,9 @@ static mp_obj_t bleio_characteristic_set_cccd(mp_uint_t n_args, const mp_obj_t *
 static MP_DEFINE_CONST_FUN_OBJ_KW(bleio_characteristic_set_cccd_obj, 1, bleio_characteristic_set_cccd);
 
 static const mp_rom_map_elem_t bleio_characteristic_locals_dict_table[] = {
+    { MP_ROM_QSTR(MP_QSTR_deinit), MP_ROM_PTR(&bleio_characteristic_deinit_obj) },
+    { MP_ROM_QSTR(MP_QSTR___del__), MP_ROM_PTR(&bleio_characteristic_deinit_obj) },
+
     { MP_ROM_QSTR(MP_QSTR_add_to_service), MP_ROM_PTR(&bleio_characteristic_add_to_service_obj) },
     { MP_ROM_QSTR(MP_QSTR_descriptors),    MP_ROM_PTR(&bleio_characteristic_descriptors_obj) },
     { MP_ROM_QSTR(MP_QSTR_properties),     MP_ROM_PTR(&bleio_characteristic_properties_obj) },

shared-bindings/_bleio/Characteristic.h

@@ -24,5 +24,7 @@ extern size_t common_hal_bleio_characteristic_get_max_length(bleio_characteristi
 extern size_t common_hal_bleio_characteristic_get_value(bleio_characteristic_obj_t *self, uint8_t *buf, size_t len);
 extern void common_hal_bleio_characteristic_add_descriptor(bleio_characteristic_obj_t *self, bleio_descriptor_obj_t *descriptor);
 extern void common_hal_bleio_characteristic_construct(bleio_characteristic_obj_t *self, bleio_service_obj_t *service, uint16_t handle, bleio_uuid_obj_t *uuid, bleio_characteristic_properties_t props, bleio_attribute_security_mode_t read_perm, bleio_attribute_security_mode_t write_perm, mp_int_t max_length, bool fixed_length, mp_buffer_info_t *initial_value_bufinfo, const char *user_description);
+extern bool common_hal_bleio_characteristic_deinited(bleio_characteristic_obj_t *self);
+extern void common_hal_bleio_characteristic_deinit(bleio_characteristic_obj_t *self);
 extern void common_hal_bleio_characteristic_set_cccd(bleio_characteristic_obj_t *self, bool notify, bool indicate);
 extern void common_hal_bleio_characteristic_set_value(bleio_characteristic_obj_t *self, mp_buffer_info_t *bufinfo);

supervisor/shared/bluetooth/bluetooth.c

@@ -339,6 +339,10 @@ void supervisor_stop_bluetooth(void) {
 
     ble_started = false;
 
+    #if CIRCUITPY_BLE_FILE_SERVICE
+    supervisor_stop_bluetooth_file_transfer();
+    #endif
+
     #if CIRCUITPY_SERIAL_BLE
     supervisor_stop_bluetooth_serial();
     #endif

supervisor/shared/bluetooth/file_transfer.c

@@ -101,6 +101,13 @@ void supervisor_start_bluetooth_file_transfer(void) {
         &static_handler_entry);
 }
 
+void supervisor_stop_bluetooth_file_transfer(void) {
+    common_hal_bleio_packet_buffer_deinit(&_transfer_packet_buffer);
+    common_hal_bleio_characteristic_deinit(&supervisor_ble_transfer_characteristic);
+    common_hal_bleio_characteristic_deinit(&supervisor_ble_version_characteristic);
+    common_hal_bleio_service_deinit(&supervisor_ble_service);
+}
+
 #define COMMAND_SIZE 1024
 
 #define ANY_COMMAND 0x00

supervisor/shared/bluetooth/file_transfer.h

@@ -8,6 +8,8 @@
 
 #include <stdbool.h>
 
-void supervisor_bluetooth_file_transfer_background(void);
 void supervisor_start_bluetooth_file_transfer(void);
+void supervisor_stop_bluetooth_file_transfer(void);
+
+void supervisor_bluetooth_file_transfer_background(void);
 void supervisor_bluetooth_file_transfer_disconnected(void);

supervisor/shared/bluetooth/serial.c

@@ -148,6 +148,11 @@ void supervisor_stop_bluetooth_serial(void) {
         return;
     }
     common_hal_bleio_packet_buffer_flush(&_tx_packet_buffer);
+    common_hal_bleio_packet_buffer_deinit(&_tx_packet_buffer);
+    common_hal_bleio_characteristic_deinit(&supervisor_ble_circuitpython_rx_characteristic);
+    common_hal_bleio_characteristic_deinit(&supervisor_ble_circuitpython_tx_characteristic);
+    common_hal_bleio_characteristic_deinit(&supervisor_ble_circuitpython_version_characteristic);
+    common_hal_bleio_service_deinit(&supervisor_ble_circuitpython_service);
 }
 
 bool ble_serial_connected(void) {