Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Client connection improvements #754

Merged
merged 2 commits into from
Nov 18, 2024
Merged

Client connection improvements #754

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a39ee03
Refactor client connection establishment and client deletion.
h2zero Nov 14, 2024
b78c248
Revert #697277f and replace with stack checks.
h2zero Nov 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
162 changes: 88 additions & 74 deletions src/NimBLEClient.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,7 @@ NimBLEClient::NimBLEClient(const NimBLEAddress& peerAddress)
m_pClientCallbacks{&defaultCallbacks},
m_connHandle{BLE_HS_CONN_HANDLE_NONE},
m_terminateFailCount{0},
m_deleteCallbacks{false},
m_connEstablished{false},
m_asyncConnect{false},
m_exchangeMTU{true},
m_config{},
# if CONFIG_BT_NIMBLE_EXT_ADV
m_phyMask{BLE_GAP_LE_PHY_1M_MASK | BLE_GAP_LE_PHY_2M_MASK | BLE_GAP_LE_PHY_CODED_MASK},
# endif
Expand All @@ -89,7 +86,7 @@ NimBLEClient::~NimBLEClient() {
// Before we are finished with the client, we must release resources.
deleteServices();

if (m_deleteCallbacks) {
if (m_config.deleteCallbacks) {
delete m_pClientCallbacks;
}
} // ~NimBLEClient
Expand Down Expand Up @@ -122,7 +119,7 @@ size_t NimBLEClient::deleteService(const NimBLEUUID& uuid) {
}

return m_svcVec.size();
} // deleteServices
} // deleteService

/**
* @brief Connect to the BLE Server using the address of the last connected device, or the address\n
Expand Down Expand Up @@ -174,16 +171,11 @@ bool NimBLEClient::connect(const NimBLEAddress& address, bool deleteAttributes,
return false;
}

if (isConnected() || m_connEstablished) {
if (isConnected()) {
NIMBLE_LOGE(LOG_TAG, "Client already connected");
return false;
}

if (NimBLEDevice::isConnectionInProgress()) {
NIMBLE_LOGE(LOG_TAG, "Connection already in progress");
return false;
}

const ble_addr_t* peerAddr = address.getBase();
if (ble_gap_conn_find_by_addr(peerAddr, NULL) == 0) {
NIMBLE_LOGE(LOG_TAG, "A connection to %s already exists", address.toString().c_str());
Expand All @@ -201,12 +193,9 @@ bool NimBLEClient::connect(const NimBLEAddress& address, bool deleteAttributes,
deleteServices();
}

int rc = 0;
m_asyncConnect = asyncConnect;
m_exchangeMTU = exchangeMTU;

// Set the connection in progress flag to prevent a scan from starting while connecting.
NimBLEDevice::setConnectionInProgress(true);
int rc = 0;
m_config.asyncConnect = asyncConnect;
m_config.exchangeMTU = exchangeMTU;

do {
# if CONFIG_BT_NIMBLE_EXT_ADV
Expand Down Expand Up @@ -261,11 +250,10 @@ bool NimBLEClient::connect(const NimBLEAddress& address, bool deleteAttributes,

if (rc != 0) {
m_lastErr = rc;
NimBLEDevice::setConnectionInProgress(false);
return false;
}

if (m_asyncConnect) {
if (m_config.asyncConnect) {
return true;
}

Expand All @@ -289,19 +277,14 @@ bool NimBLEClient::connect(const NimBLEAddress& address, bool deleteAttributes,
rc = taskData.m_flags;
if (rc != 0) {
NIMBLE_LOGE(LOG_TAG, "Connection failed; status=%d %s", rc, NimBLEUtils::returnCodeToString(rc));
// If the failure was not a result of a disconnection, make sure we disconnect now to avoid dangling connections
if (isConnected()) {
disconnect();
}
m_lastErr = rc;
if (m_config.deleteOnConnectFail) {
NimBLEDevice::deleteClient(this);
}
return false;
} else {
NIMBLE_LOGI(LOG_TAG, "Connection established");
}

m_connEstablished = true;
m_pClientCallbacks->onConnect(this);

NIMBLE_LOGD(LOG_TAG, "<< connect()");
// Check if still connected before returning
return isConnected();
Expand Down Expand Up @@ -357,7 +340,7 @@ bool NimBLEClient::disconnect(uint8_t reason) {
* @brief Cancel an ongoing connection attempt.
* @return True if the command was successfully sent.
*/
bool NimBLEClient::cancelConnect() {
bool NimBLEClient::cancelConnect() const {
int rc = ble_gap_conn_cancel();
if (rc != 0 && rc != BLE_HS_EALREADY) {
NIMBLE_LOGE(LOG_TAG, "ble_gap_conn_cancel failed: rc=%d %s", rc, NimBLEUtils::returnCodeToString(rc));
Expand All @@ -368,6 +351,32 @@ bool NimBLEClient::cancelConnect() {
return true;
} // cancelConnect

/**
* @brief Set or unset a flag to delete this client when disconnected or connection failed.
* @param [in] deleteOnDisconnect Set the client to self delete when disconnected.
* @param [in] deleteOnConnectFail Set the client to self delete when a connection attempt fails.
*/
void NimBLEClient::setSelfDelete(bool deleteOnDisconnect, bool deleteOnConnectFail) {
m_config.deleteOnDisconnect = deleteOnDisconnect;
m_config.deleteOnConnectFail = deleteOnConnectFail;
} // setSelfDelete

/**
* @brief Get a copy of the clients configuration.
* @return A copy of the clients configuration.
*/
NimBLEClient::Config NimBLEClient::getConfig() const {
return m_config;
} // getConfig

/**
* @brief Set the client configuration options.
* @param [in] config The config options instance to set the client configuration to.
*/
void NimBLEClient::setConfig(NimBLEClient::Config config) {
m_config = config;
} // setConfig

# if CONFIG_BT_NIMBLE_EXT_ADV
/**
* @brief Set the PHY types to use when connecting to a server.
Expand Down Expand Up @@ -492,9 +501,8 @@ uint16_t NimBLEClient::getConnHandle() const {
* To disconnect from a peer, use disconnect().
*/
void NimBLEClient::clearConnection() {
m_connHandle = BLE_HS_CONN_HANDLE_NONE;
m_connEstablished = false;
m_peerAddress = NimBLEAddress{};
m_connHandle = BLE_HS_CONN_HANDLE_NONE;
m_peerAddress = NimBLEAddress{};
} // clearConnection

/**
Expand All @@ -509,15 +517,13 @@ void NimBLEClient::clearConnection() {
* This enables the GATT Server to read the attributes of the client connected to it.
*/
bool NimBLEClient::setConnection(const NimBLEConnInfo& connInfo) {
if (isConnected() || m_connEstablished) {
if (isConnected()) {
NIMBLE_LOGE(LOG_TAG, "Already connected");
return false;
}

m_peerAddress = connInfo.getAddress();
m_connHandle = connInfo.getConnHandle();
m_connEstablished = true;

m_peerAddress = connInfo.getAddress();
m_connHandle = connInfo.getConnHandle();
return true;
} // setConnection

Expand Down Expand Up @@ -763,6 +769,12 @@ int NimBLEClient::serviceDiscoveredCB(uint16_t connHandle,
NimBLETaskData* pTaskData = (NimBLETaskData*)arg;
NimBLEClient* pClient = (NimBLEClient*)pTaskData->m_pInstance;

if (error->status == BLE_HS_ENOTCONN) {
NIMBLE_LOGE(LOG_TAG, "<< Service Discovered; Not connected");
NimBLEUtils::taskRelease(*pTaskData, error->status);
return error->status;
}

// Make sure the service discovery is for this device
if (pClient->getConnHandle() != connHandle) {
return 0;
Expand Down Expand Up @@ -902,8 +914,9 @@ bool NimBLEClient::exchangeMTU() {
* @param [in] arg A pointer to the client instance that registered for this callback.
*/
int NimBLEClient::handleGapEvent(struct ble_gap_event* event, void* arg) {
NimBLEClient* pClient = (NimBLEClient*)arg;
int rc = 0;
NimBLEClient* pClient = (NimBLEClient*)arg;
int rc = 0;
NimBLETaskData* pTaskData = pClient->m_pTaskData; // save a copy in case client is deleted

NIMBLE_LOGD(LOG_TAG, "Got Client event %s", NimBLEUtils::gapEventToString(event->type));

Expand All @@ -917,7 +930,7 @@ int NimBLEClient::handleGapEvent(struct ble_gap_event* event, void* arg) {

rc = event->disconnect.reason;
// If Host reset tell the device now before returning to prevent
// any errors caused by calling host functions before resyncing.
// any errors caused by calling host functions before re-syncing.
switch (rc) {
case BLE_HS_ECONTROLLER:
case BLE_HS_ETIMEOUT_HCI:
Expand All @@ -930,41 +943,45 @@ int NimBLEClient::handleGapEvent(struct ble_gap_event* event, void* arg) {
break;
}

NIMBLE_LOGD(LOG_TAG, "disconnect; reason=%d, %s", rc, NimBLEUtils::returnCodeToString(rc));

pClient->m_terminateFailCount = 0;
NimBLEDevice::removeIgnored(pClient->m_peerAddress);
pClient->m_connHandle = BLE_HS_CONN_HANDLE_NONE;

// If we received a connected event but did not get established
// then a disconnect event will be sent but we should not send it to the
// app for processing. Instead we will ensure the task is released
// and report the error.
if (!pClient->m_connEstablished) {
break;
// Don't call the disconnect callback if we are waiting for a connection to complete and it fails
if (rc != (BLE_HS_ERR_HCI_BASE + BLE_ERR_CONN_ESTABLISHMENT) || pClient->m_config.asyncConnect) {
pClient->m_pClientCallbacks->onDisconnect(pClient, rc);
}

NIMBLE_LOGI(LOG_TAG, "disconnect; reason=%d, %s", rc, NimBLEUtils::returnCodeToString(rc));
pClient->m_connHandle = BLE_HS_CONN_HANDLE_NONE;

if (pClient->m_config.deleteOnDisconnect) {
// If we are set to self delete on disconnect but we have a task waiting on the connection
// completion we will set the flag to delete on connect fail instead of deleting here
// to prevent segmentation faults or double deleting
if (pTaskData != nullptr && rc == (BLE_HS_ERR_HCI_BASE + BLE_ERR_CONN_ESTABLISHMENT)) {
pClient->m_config.deleteOnConnectFail = true;
break;
}
NimBLEDevice::deleteClient(pClient);
}

pClient->m_connEstablished = false;
pClient->m_pClientCallbacks->onDisconnect(pClient, rc);
break;
} // BLE_GAP_EVENT_DISCONNECT

case BLE_GAP_EVENT_CONNECT: {
// If we aren't waiting for this connection response we should drop the connection immediately.
if (pClient->isConnected() || (!pClient->m_asyncConnect && pClient->m_pTaskData == nullptr)) {
if (pClient->isConnected() || (!pClient->m_config.asyncConnect && pClient->m_pTaskData == nullptr)) {
ble_gap_terminate(event->connect.conn_handle, BLE_ERR_REM_USER_CONN_TERM);
return 0;
}

NimBLEDevice::setConnectionInProgress(false);
rc = event->connect.status;
if (rc == 0) {
NIMBLE_LOGI(LOG_TAG, "Connected event");

pClient->m_connHandle = event->connect.conn_handle;
if (pClient->m_exchangeMTU) {
if (!pClient->exchangeMTU() && !pClient->m_asyncConnect) {
rc = pClient->m_lastErr;
if (pClient->m_config.exchangeMTU) {
if (!pClient->exchangeMTU() && !pClient->m_config.asyncConnect) {
rc = pClient->m_lastErr; // sets the error in the task data
break;
}
}
Expand All @@ -974,15 +991,19 @@ int NimBLEClient::handleGapEvent(struct ble_gap_event* event, void* arg) {
NimBLEDevice::addIgnored(pClient->m_peerAddress);
} else {
pClient->m_connHandle = BLE_HS_CONN_HANDLE_NONE;
if (!pClient->m_asyncConnect) {
if (!pClient->m_config.asyncConnect) {
break;
}

if (pClient->m_config.deleteOnConnectFail) { // async connect
delete pClient;
return 0;
}
}

if (pClient->m_asyncConnect) {
pClient->m_connEstablished = rc == 0;
if (pClient->m_config.asyncConnect) {
pClient->m_pClientCallbacks->onConnect(pClient);
} else if (!pClient->m_exchangeMTU) {
} else if (!pClient->m_config.exchangeMTU) {
break; // not waiting for MTU exchange so release the task now.
}

Expand All @@ -1005,13 +1026,6 @@ int NimBLEClient::handleGapEvent(struct ble_gap_event* event, void* arg) {

case BLE_GAP_EVENT_NOTIFY_RX: {
if (pClient->m_connHandle != event->notify_rx.conn_handle) return 0;

// If a notification comes before this flag is set we might
// access a vector while it is being cleared in connect()
if (!pClient->m_connEstablished) {
return 0;
}

NIMBLE_LOGD(LOG_TAG, "Notify Received for handle: %d", event->notify_rx.attr_handle);

for (const auto& svc : pClient->m_svcVec) {
Expand Down Expand Up @@ -1170,8 +1184,8 @@ int NimBLEClient::handleGapEvent(struct ble_gap_event* event, void* arg) {
}
} // Switch

if (pClient->m_pTaskData != nullptr) {
NimBLEUtils::taskRelease(*pClient->m_pTaskData, rc);
if (pTaskData != nullptr) {
NimBLEUtils::taskRelease(*pTaskData, rc);
}

return 0;
Expand All @@ -1192,11 +1206,11 @@ bool NimBLEClient::isConnected() const {
*/
void NimBLEClient::setClientCallbacks(NimBLEClientCallbacks* pClientCallbacks, bool deleteCallbacks) {
if (pClientCallbacks != nullptr) {
m_pClientCallbacks = pClientCallbacks;
m_deleteCallbacks = deleteCallbacks;
m_pClientCallbacks = pClientCallbacks;
m_config.deleteCallbacks = deleteCallbacks;
} else {
m_pClientCallbacks = &defaultCallbacks;
m_deleteCallbacks = false;
m_pClientCallbacks = &defaultCallbacks;
m_config.deleteCallbacks = false;
}
} // setClientCallbacks

Expand Down
20 changes: 15 additions & 5 deletions src/NimBLEClient.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ class NimBLEClient {
bool connect(const NimBLEAddress& address, bool deleteAttributes = true, bool asyncConnect = false, bool exchangeMTU = true);
bool connect(bool deleteAttributes = true, bool asyncConnect = false, bool exchangeMTU = true);
bool disconnect(uint8_t reason = BLE_ERR_REM_USER_CONN_TERM);
bool cancelConnect();
bool cancelConnect() const;
void setSelfDelete(bool deleteOnDisconnect, bool deleteOnConnectFail);
NimBLEAddress getPeerAddress() const;
bool setPeerAddress(const NimBLEAddress& address);
int getRssi() const;
Expand Down Expand Up @@ -95,6 +96,17 @@ class NimBLEClient {
void setConnectPhy(uint8_t mask);
# endif

struct Config {
uint8_t deleteCallbacks : 1; // Delete the callback object when the client is deleted.
uint8_t deleteOnDisconnect : 1; // Delete the client when disconnected.
uint8_t deleteOnConnectFail : 1; // Delete the client when a connection attempt fails.
uint8_t asyncConnect : 1; // Connect asynchronously.
uint8_t exchangeMTU : 1; // Exchange MTU after connection.
};

Config getConfig() const;
void setConfig(Config config);

private:
NimBLEClient(const NimBLEAddress& peerAddress);
~NimBLEClient();
Expand All @@ -117,10 +129,8 @@ class NimBLEClient {
NimBLEClientCallbacks* m_pClientCallbacks;
uint16_t m_connHandle;
uint8_t m_terminateFailCount;
bool m_deleteCallbacks;
bool m_connEstablished;
bool m_asyncConnect;
bool m_exchangeMTU;
Config m_config;

# if CONFIG_BT_NIMBLE_EXT_ADV
uint8_t m_phyMask;
# endif
Expand Down
Loading
Loading