Skip to content

Commit 1cad43a

Browse files
committed
extmod/zephyr_ble: Fix soft reset resource leaks causing intermittent hang.
After 4-5 BLE test cycles, the device would hang during soft reset. Root cause was accumulated state corruption from unfree'd resources and stale flags persisting across soft resets. Fixes: - Reset static flags on deinit: mp_bt_zephyr_callbacks_registered, indication pool in_use flags, HCI RX task flags - Add mp_bluetooth_zephyr_work_reset() to clear stale work queue linkages that prevented proper reinitialization - Add mp_bt_zephyr_free_service() to free malloc'd GATT service memory (UUIDs, characteristic structs, attribute arrays) - Free temporary UUIDs from create_zephyr_uuid() immediately after gatt_db_add() copies them Verified with 20+ consecutive BLE multitest cycles without hang.
1 parent 683bf36 commit 1cad43a

File tree

4 files changed

+131
-6
lines changed

4 files changed

+131
-6
lines changed

extmod/zephyr_ble/hal/zephyr_ble_work.c

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -831,3 +831,35 @@ bool mp_bluetooth_zephyr_work_drain(void) {
831831
DEBUG_WORK_printf("work_drain: done, processed=%d\n", any_work);
832832
return any_work;
833833
}
834+
835+
// Reset work queue state for clean re-initialization
836+
// This clears the global work queue list and resets the system work queue
837+
// Called from mp_bluetooth_deinit() to prevent stale queue linkages
838+
void mp_bluetooth_zephyr_work_reset(void) {
839+
DEBUG_WORK_printf("work_reset: clearing work queue state\n");
840+
841+
// Clear the global work queue list
842+
global_work_q = NULL;
843+
844+
// Reset system work queue
845+
k_sys_work_q.head = NULL;
846+
k_sys_work_q.nextq = NULL;
847+
k_sys_work_q.name = NULL;
848+
849+
// Reset init work queue
850+
k_init_work_q.head = NULL;
851+
k_init_work_q.nextq = NULL;
852+
k_init_work_q.name = NULL;
853+
854+
// Reset recursion guards
855+
regular_work_processor_running = false;
856+
init_work_processor_running = false;
857+
858+
// Reset init phase flag
859+
in_bt_enable_init = false;
860+
in_sys_work_q_context = false;
861+
862+
// Reset debug counters
863+
work_process_call_count = 0;
864+
work_items_processed = 0;
865+
}

extmod/zephyr_ble/hal/zephyr_ble_work.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -215,6 +215,10 @@ void mp_bluetooth_zephyr_work_thread_stop(void);
215215
// Returns true if any work was processed
216216
bool mp_bluetooth_zephyr_work_drain(void);
217217

218+
// Reset work queue state for clean re-initialization
219+
// Called from mp_bluetooth_deinit() to clear stale queue linkages
220+
void mp_bluetooth_zephyr_work_reset(void);
221+
218222
// Get delayable work from work (for internal use)
219223
// work is embedded in the delayable work structure
220224
static inline struct k_work_delayable *k_work_delayable_from_work(struct k_work *work) {

extmod/zephyr_ble/modbluetooth_zephyr.c

Lines changed: 89 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,7 @@ static void add_descriptor(struct bt_gatt_attr *chrc, struct add_descriptor *d,
235235
static void mp_bt_zephyr_gatt_indicate_done(struct bt_conn *conn, struct bt_gatt_indicate_params *params, uint8_t err);
236236
static void mp_bt_zephyr_gatt_indicate_destroy(struct bt_gatt_indicate_params *params);
237237
static struct bt_gatt_attr *mp_bt_zephyr_find_attr_by_handle(uint16_t value_handle);
238+
static void mp_bt_zephyr_free_service(struct bt_gatt_service *service);
238239

239240
static struct bt_conn_cb mp_bt_zephyr_conn_callbacks = {
240241
.connected = mp_bt_zephyr_connected,
@@ -682,9 +683,14 @@ int mp_bluetooth_deinit(void) {
682683

683684
#if CONFIG_BT_GATT_DYNAMIC_DB
684685
for (size_t i = 0; i < MP_STATE_PORT(bluetooth_zephyr_root_pointers)->n_services; ++i) {
685-
bt_gatt_service_unregister(MP_STATE_PORT(bluetooth_zephyr_root_pointers)->services[i]);
686-
MP_STATE_PORT(bluetooth_zephyr_root_pointers)->services[i] = NULL;
686+
struct bt_gatt_service *service = MP_STATE_PORT(bluetooth_zephyr_root_pointers)->services[i];
687+
if (service != NULL) {
688+
bt_gatt_service_unregister(service);
689+
mp_bt_zephyr_free_service(service);
690+
MP_STATE_PORT(bluetooth_zephyr_root_pointers)->services[i] = NULL;
691+
}
687692
}
693+
MP_STATE_PORT(bluetooth_zephyr_root_pointers)->n_services = 0;
688694
#endif
689695

690696
// Call bt_disable() to properly shutdown the BLE stack
@@ -729,6 +735,10 @@ int mp_bluetooth_deinit(void) {
729735
mp_bluetooth_zephyr_hci_rx_task_stop();
730736
mp_bluetooth_zephyr_work_thread_stop();
731737

738+
// Reset work queue state to clear stale queue linkages
739+
extern void mp_bluetooth_zephyr_work_reset(void);
740+
mp_bluetooth_zephyr_work_reset();
741+
732742
// Deinit port-specific resources (CYW43 cleanup, soft timers, etc.)
733743
// This must be done after bt_disable() completes.
734744
#if MICROPY_PY_NETWORK_CYW43 || MICROPY_PY_BLUETOOTH_USE_ZEPHYR_HCI
@@ -739,6 +749,16 @@ int mp_bluetooth_deinit(void) {
739749
MP_STATE_PORT(bluetooth_zephyr_root_pointers) = NULL;
740750
mp_bt_zephyr_next_conn = NULL;
741751

752+
// Reset indication pool - all indications should be done by now
753+
for (int i = 0; i < CONFIG_BT_MAX_CONN; i++) {
754+
mp_bt_zephyr_indicate_pool[i].in_use = false;
755+
}
756+
757+
// Reset callback registration flag so callbacks will be re-registered on next init.
758+
// This is necessary because Zephyr's internal callback list may have been corrupted
759+
// by the soft reset, and we want a clean slate.
760+
mp_bt_zephyr_callbacks_registered = false;
761+
742762
// Set state to OFF so next init does full re-initialization
743763
// (including controller init and callback registration)
744764
mp_bluetooth_zephyr_ble_state = MP_BLUETOOTH_ZEPHYR_BLE_STATE_OFF;
@@ -877,10 +897,14 @@ int mp_bluetooth_gatts_register_service_begin(bool append) {
877897
return MP_EOPNOTSUPP;
878898
}
879899

880-
// Unregister and unref any previous service definitions.
900+
// Unregister and free any previous service definitions.
881901
for (size_t i = 0; i < MP_STATE_PORT(bluetooth_zephyr_root_pointers)->n_services; ++i) {
882-
bt_gatt_service_unregister(MP_STATE_PORT(bluetooth_zephyr_root_pointers)->services[i]);
883-
MP_STATE_PORT(bluetooth_zephyr_root_pointers)->services[i] = NULL;
902+
struct bt_gatt_service *service = MP_STATE_PORT(bluetooth_zephyr_root_pointers)->services[i];
903+
if (service != NULL) {
904+
bt_gatt_service_unregister(service);
905+
mp_bt_zephyr_free_service(service);
906+
MP_STATE_PORT(bluetooth_zephyr_root_pointers)->services[i] = NULL;
907+
}
884908
}
885909
MP_STATE_PORT(bluetooth_zephyr_root_pointers)->n_services = 0;
886910

@@ -931,7 +955,10 @@ int mp_bluetooth_gatts_register_service(mp_obj_bluetooth_uuid_t *service_uuid, m
931955
uint64_t attrs_are_chrs = 0;
932956
uint64_t chr_has_ccc = 0;
933957

934-
add_service(create_zephyr_uuid(service_uuid), &svc_attributes[attr_index]);
958+
// Create and add service, then free the temporary UUID (gatt_db_add copies it)
959+
struct bt_uuid *svc_uuid = create_zephyr_uuid(service_uuid);
960+
add_service(svc_uuid, &svc_attributes[attr_index]);
961+
free(svc_uuid);
935962
attr_index += 1;
936963

937964
for (size_t i = 0; i < num_characteristics; ++i) {
@@ -956,6 +983,8 @@ int mp_bluetooth_gatts_register_service(mp_obj_bluetooth_uuid_t *service_uuid, m
956983
}
957984

958985
add_characteristic(&add_char, &svc_attributes[attr_index], &svc_attributes[attr_index + 1]);
986+
// Free the temporary UUID (gatt_db_add copied it)
987+
free((void *)add_char.uuid);
959988

960989
struct bt_gatt_attr *curr_char = &svc_attributes[attr_index];
961990
attrs_are_chrs |= (1 << attr_index);
@@ -980,6 +1009,8 @@ int mp_bluetooth_gatts_register_service(mp_obj_bluetooth_uuid_t *service_uuid, m
9801009
}
9811010

9821011
add_descriptor(curr_char, &add_desc, &svc_attributes[attr_index]);
1012+
// Free the temporary UUID (gatt_db_add copied it)
1013+
free((void *)add_desc.uuid);
9831014
attr_index += 1;
9841015

9851016
descriptor_index++;
@@ -1000,6 +1031,8 @@ int mp_bluetooth_gatts_register_service(mp_obj_bluetooth_uuid_t *service_uuid, m
10001031
attrs_to_ignore |= (1 << attr_index);
10011032

10021033
add_descriptor(curr_char, &add_desc, &svc_attributes[attr_index]);
1034+
// Free the temporary UUID (gatt_db_add copied it)
1035+
free((void *)add_desc.uuid);
10031036
attr_index += 1;
10041037
}
10051038
}
@@ -1600,6 +1633,56 @@ static void add_descriptor(struct bt_gatt_attr *chrc, struct add_descriptor *d,
16001633
}
16011634
}
16021635

1636+
// Free all memory associated with a GATT service.
1637+
// Called when unregistering services to prevent memory leaks.
1638+
//
1639+
// Memory allocation pattern in GATT registration:
1640+
// - gatt_db_add() allocates attr->uuid via malloc for ALL attributes
1641+
// - gatt_db_add() allocates attr->user_data via malloc ONLY when user_data_len > 0:
1642+
// - Service declaration (index 0): user_data = malloc'd copy of service UUID
1643+
// - Characteristic declaration: user_data = malloc'd bt_gatt_chrc struct
1644+
// - Other attrs: user_data is either NULL or later assigned to gatts_db entry (GC heap)
1645+
// - Service struct and attrs array are malloc'd in mp_bluetooth_gatts_register_service
1646+
//
1647+
// We must NOT free user_data that points to gatts_db entries (GC managed).
1648+
// We identify characteristic declarations by their read callback (bt_gatt_attr_read_chrc).
1649+
static void mp_bt_zephyr_free_service(struct bt_gatt_service *service) {
1650+
if (service == NULL) {
1651+
return;
1652+
}
1653+
1654+
if (service->attrs != NULL) {
1655+
// First: free user_data for service declaration (index 0) and characteristic declarations
1656+
// Service declaration is always at index 0
1657+
if (service->attr_count > 0 && service->attrs[0].user_data != NULL) {
1658+
free(service->attrs[0].user_data);
1659+
}
1660+
1661+
// Characteristic declarations have read callback = bt_gatt_attr_read_chrc
1662+
extern ssize_t bt_gatt_attr_read_chrc(struct bt_conn *conn,
1663+
const struct bt_gatt_attr *attr, void *buf, uint16_t len, uint16_t offset);
1664+
for (size_t i = 1; i < service->attr_count; i++) {
1665+
struct bt_gatt_attr *attr = &service->attrs[i];
1666+
if (attr->read == bt_gatt_attr_read_chrc && attr->user_data != NULL) {
1667+
free(attr->user_data);
1668+
}
1669+
}
1670+
1671+
// Second: free all UUIDs (all were malloc'd by gatt_db_add)
1672+
for (size_t i = 0; i < service->attr_count; i++) {
1673+
if (service->attrs[i].uuid != NULL) {
1674+
free((void *)service->attrs[i].uuid);
1675+
}
1676+
}
1677+
1678+
// Free the attributes array itself
1679+
free(service->attrs);
1680+
}
1681+
1682+
// Free the service struct itself
1683+
free(service);
1684+
}
1685+
16031686
// GATT Client implementation
16041687
#if MICROPY_PY_BLUETOOTH_ENABLE_GATT_CLIENT
16051688

ports/rp2/mpzephyrport_rp2.c

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -875,6 +875,12 @@ void mp_bluetooth_zephyr_port_deinit(void) {
875875
poll_uart_skipped_no_cb = 0;
876876
poll_uart_skipped_task = 0;
877877
poll_uart_cyw43_calls = 0;
878+
879+
#if MICROPY_PY_THREAD
880+
// Reset HCI RX task flags so next init starts fresh
881+
hci_rx_task_started = false;
882+
hci_rx_task_exited = false;
883+
#endif
878884
}
879885

880886
void mp_bluetooth_zephyr_poll_uart(void) {

0 commit comments

Comments
 (0)