deleting BLEClient is not safe

[jackjansen]

My BLE client code was structured more-or-less as

BLEClient *client = new BLEClient();
client->connect(address, type);
// interact with client
client->disconnect();
delete client;

but this is unsafe. Sometimes I get a corrupted heap, sometimes I get other crashes, possibly after running this code again (I am creating many connections to many BLE devices over a long time).

I think the problem may be that disconnect() is asynchronous, because often my crashes are in BLEClient::gattClientEventHandler() in the ESP_GATTC_DISCONNECT_EVT case handler, but they can also happen in the BLE thread, or in completely unrelated code.

The workaround I have is to never delete my BLEClient objects but to cache and reuse them, but from the code (and the minimal documentation) it seems like I should have been able to create and delete them on-the-fly.

[jamesi8086]

From what I tested, you have to wait until client->isConnected returns false before you can delete it. I added a spinloop which checks every 50ms before deleting.
Other methods such as keeping old instances in a list for later garbage-collection does not yeild anything stable. Seems risky to have more than one BLEClient at any time.

Caching and reusing may work, but it did not work for me when I tried to use the same instance to reconnect to different devices.

[chegewara]

BLEClient is deleted inside library after disconnecting.

[jamesi8086]

oh? is it a recent change?

[chegewara]

No, its been added months ago, before BLE library has been merged.
BLEClient:
https://github.com/espressif/arduino-esp32/blob/master/libraries/BLE/src/BLEClient.cpp#L191
and BLEServer:
https://github.com/espressif/arduino-esp32/blob/master/libraries/BLE/src/BLEDevice.cpp#L589-L628

[buxtronix]

I have just had this issue also. It turns out that GATTC_DISCONNECT (and other events) are propagated to ALL clients, not just the one that disconnected.

Fix is in #4055 - my multi-client code has been pretty stable since i applied it.

[jackjansen]

Hmm. Looking at the BLEClient code a bit more.

I think that maybe adding a BLEDevice::removePeerDevice(m_appId, true); to the destructor is enough to fix the original issue (but unfortunately given the structure of my code I cannot test this hypothesis easily, sorry....).

But connect() looks a bit suspicious to me as well, especially in the case of a second connect. The global m_appid is incremented and the new value registered with BLEDevice::addDevice and with the esp-idf esp_ble_gattc_app_register(), but a previously registered connection id is not unregistered. And because the previous value of the local m_appId is lost that value will never be unregistered...

Together with what @yongkimleng and @buxtronix found this means that you can never delete the BLEClient, and moreover there may always be "ghost callbacks from past connections" when you reuse the BLEClient.

[buxtronix]

The 'ghost callbacks' I've been seeing (addressed in #4064) happen even when no connection was ever closed (ie two new clients connecting after a reboot). Something to note (maybe two issues here?)

[TheNitek]

You probably ran into this problem: #3973

[buxtronix]

No, #3973 is about cleanup with deleted clients. The problem I have seen is with new connections after boot, before any disconnect event has occurred (but also happens on disconnect, but this is for similar reasons relating to bad callbacks ).

[stale]

[STALE_SET] This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

[stale]

[STALE_DEL] This stale issue has been automatically closed. Thank you for your contributions.

[llemtt]

The BLEClient multi connect implementation is still buggy.

I started with reusing and reconnecting multiple BLEClients but it requires at least the following fix in "gattClientEventHandler" or something equivalent:

	case ESP_GATTC_DISCONNECT_EVT: {
			// If we receive a disconnect event, set the class flag that indicates that we are
			// no longer connected.
			m_isConnected = false;
			if (m_pClientCallbacks != nullptr) {
				m_pClientCallbacks->onDisconnect(this);
			}
			esp_ble_gattc_app_unregister(m_gattc_if);
			m_semaphoreOpenEvt.give(ESP_GATT_IF_NONE);
			m_semaphoreRssiCmplEvt.give();
			m_semaphoreSearchCmplEvt.give(1);
			BLEDevice::removePeerDevice(m_appId, true);
		-->     m_gattc_if = ESP_GATT_IF_NONE; // let Client multi reconnection working
			break;
	} // ESP_GATTC_DISCONNECT_EVT

This because upon disconnection BLEClients aren't properly cleaned up.

I tried also to use a new BLEClient each time I need a connection but BLEClient deletion is even more buggy.

[kind3r]

Any reason why the disconnect client callback is before all the cleanup ?
Like for example:

case ESP_GATTC_DISCONNECT_EVT: {
				if (evtParam->disconnect.conn_id != getConnId()) break;
				// If we receive a disconnect event, set the class flag that indicates that we are
				// no longer connected.
				bool m_wasConnected = m_isConnected;
				m_isConnected = false;
				esp_ble_gattc_app_unregister(m_gattc_if);
				m_semaphoreOpenEvt.give(ESP_GATT_IF_NONE);
				m_semaphoreRssiCmplEvt.give();
				m_semaphoreSearchCmplEvt.give(1);
				BLEDevice::removePeerDevice(m_appId, true);
				if (m_wasConnected && m_pClientCallbacks != nullptr) {
					m_pClientCallbacks->onDisconnect(this);
				}
				break;
		} // ESP_GATTC_DISCONNECT_EVT