Skip to content

[BREAKING] - Refactor NimBLEUtils #221

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

Merged
h2zero merged 1 commit into from
Nov 12, 2024
Merged

[BREAKING] - Refactor NimBLEUtils #221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 10 additions & 12 deletions src/NimBLEAdvertisedDevice.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ NimBLEScan* NimBLEAdvertisedDevice::getScan() const {
* @return The number of addresses.
*/
uint8_t NimBLEAdvertisedDevice::getTargetAddressCount() const {
uint8_t count = findAdvField(BLE_HS_ADV_TYPE_PUBLIC_TGT_ADDR);
uint8_t count = findAdvField(BLE_HS_ADV_TYPE_PUBLIC_TGT_ADDR);
count += findAdvField(BLE_HS_ADV_TYPE_RANDOM_TGT_ADDR);

return count;
Expand All @@ -269,7 +269,7 @@ NimBLEAddress NimBLEAdvertisedDevice::getTargetAddress(uint8_t index) const {
uint8_t count = findAdvField(BLE_HS_ADV_TYPE_PUBLIC_TGT_ADDR, index, &data_loc);
if (count < index + 1) {
index -= count;
count = findAdvField(BLE_HS_ADV_TYPE_RANDOM_TGT_ADDR, index, &data_loc);
count = findAdvField(BLE_HS_ADV_TYPE_RANDOM_TGT_ADDR, index, &data_loc);
}

if (count > 0 && data_loc != ULONG_MAX) {
Expand Down Expand Up @@ -368,14 +368,14 @@ size_t NimBLEAdvertisedDevice::findServiceData(uint8_t index, uint8_t* bytes) co
}

index -= found;
found = findAdvField(BLE_HS_ADV_TYPE_SVC_DATA_UUID32, index, &data_loc);
found = findAdvField(BLE_HS_ADV_TYPE_SVC_DATA_UUID32, index, &data_loc);
if (found > index) {
*bytes = 4;
return data_loc;
}

index -= found;
found = findAdvField(BLE_HS_ADV_TYPE_SVC_DATA_UUID128, index, &data_loc);
found = findAdvField(BLE_HS_ADV_TYPE_SVC_DATA_UUID128, index, &data_loc);
if (found > index) {
*bytes = 16;
return data_loc;
Expand All @@ -389,7 +389,7 @@ size_t NimBLEAdvertisedDevice::findServiceData(uint8_t index, uint8_t* bytes) co
* @return The number of service data UUIDS in the vector.
*/
uint8_t NimBLEAdvertisedDevice::getServiceDataCount() const {
uint8_t count = findAdvField(BLE_HS_ADV_TYPE_SVC_DATA_UUID16);
uint8_t count = findAdvField(BLE_HS_ADV_TYPE_SVC_DATA_UUID16);
count += findAdvField(BLE_HS_ADV_TYPE_SVC_DATA_UUID32);
count += findAdvField(BLE_HS_ADV_TYPE_SVC_DATA_UUID128);

Expand Down Expand Up @@ -448,7 +448,7 @@ NimBLEUUID NimBLEAdvertisedDevice::getServiceUUID(uint8_t index) const {
* @return The count of services in the advertising packet.
*/
uint8_t NimBLEAdvertisedDevice::getServiceUUIDCount() const {
uint8_t count = findAdvField(BLE_HS_ADV_TYPE_INCOMP_UUIDS16);
uint8_t count = findAdvField(BLE_HS_ADV_TYPE_INCOMP_UUIDS16);
count += findAdvField(BLE_HS_ADV_TYPE_COMP_UUIDS16);
count += findAdvField(BLE_HS_ADV_TYPE_INCOMP_UUIDS32);
count += findAdvField(BLE_HS_ADV_TYPE_COMP_UUIDS32);
Expand Down Expand Up @@ -695,11 +695,9 @@ std::string NimBLEAdvertisedDevice::toString() const {
}

if (haveManufacturerData()) {
char* pHex =
NimBLEUtils::buildHexData(nullptr, (uint8_t*)getManufacturerData().data(), getManufacturerData().length());
res += ", manufacturer data: ";
res += pHex;
free(pHex);
auto mfgData = getManufacturerData();
res += ", manufacturer data: ";
res += NimBLEUtils::dataToHexString(reinterpret_cast<const uint8_t*>(mfgData.data()), mfgData.length());
}

if (haveServiceUUID()) {
Expand All @@ -714,7 +712,7 @@ std::string NimBLEAdvertisedDevice::toString() const {
}

if (haveServiceData()) {
uint8_t count = getServiceDataCount();
uint8_t count = getServiceDataCount();
res += "\nService Data:";
for (uint8_t i = 0; i < count; i++) {
res += "\nUUID: " + std::string(getServiceDataUUID(i));
Expand Down
87 changes: 28 additions & 59 deletions src/NimBLEClient.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,11 +201,10 @@ bool NimBLEClient::connect(const NimBLEAddress& address, bool deleteAttributes,
deleteServices();
}

int rc = 0;
m_asyncConnect = asyncConnect;
m_exchangeMTU = exchangeMTU;
TaskHandle_t curTask = xTaskGetCurrentTaskHandle();
BleTaskData taskData = {this, curTask, 0, nullptr};
int rc = 0;
m_asyncConnect = asyncConnect;
m_exchangeMTU = exchangeMTU;
NimBLETaskData taskData(this);
if (!asyncConnect) {
m_pTaskData = &taskData;
}
Expand Down Expand Up @@ -276,12 +275,8 @@ bool NimBLEClient::connect(const NimBLEAddress& address, bool deleteAttributes,
return true;
}

# ifdef ulTaskNotifyValueClear
// Clear the task notification value to ensure we block
ulTaskNotifyValueClear(curTask, ULONG_MAX);
# endif
// Wait for the connect timeout time +1 second for the connection to complete
if (ulTaskNotifyTake(pdTRUE, pdMS_TO_TICKS(m_connectTimeout + 1000)) == pdFALSE) {
if (!NimBLEUtils::taskWait(*m_pTaskData, m_connectTimeout + 1000)) {
m_pTaskData = nullptr;
// If a connection was made but no response from MTU exchange; disconnect
if (isConnected()) {
Expand All @@ -296,9 +291,9 @@ bool NimBLEClient::connect(const NimBLEAddress& address, bool deleteAttributes,

return false;

} else if (taskData.rc != 0) {
m_lastErr = taskData.rc;
NIMBLE_LOGE(LOG_TAG, "Connection failed; status=%d %s", taskData.rc, NimBLEUtils::returnCodeToString(taskData.rc));
} else if (taskData.m_flags != 0) {
m_lastErr = taskData.m_flags;
NIMBLE_LOGE(LOG_TAG, "Connection failed; status=%d %s", m_lastErr, NimBLEUtils::returnCodeToString(m_lastErr));
// If the failure was not a result of a disconnection, make sure we disconnect now to avoid dangling connections
if (isConnected()) {
disconnect();
Expand All @@ -324,9 +319,8 @@ bool NimBLEClient::connect(const NimBLEAddress& address, bool deleteAttributes,
*/
bool NimBLEClient::secureConnection() const {
NIMBLE_LOGD(LOG_TAG, ">> secureConnection()");
TaskHandle_t cur_task = xTaskGetCurrentTaskHandle();
BleTaskData taskData = {const_cast<NimBLEClient*>(this), cur_task, 0, nullptr};
int retryCount = 1;
NimBLETaskData taskData(const_cast<NimBLEClient*>(this));
int retryCount = 1;

do {
m_pTaskData = &taskData;
Expand All @@ -337,16 +331,12 @@ bool NimBLEClient::secureConnection() const {
return false;
}

# ifdef ulTaskNotifyValueClear
// Clear the task notification value to ensure we block
ulTaskNotifyValueClear(cur_task, ULONG_MAX);
# endif
ulTaskNotifyTake(pdTRUE, portMAX_DELAY);
Copy link
Collaborator

@thekurtovic thekurtovic Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've had issues with tasks getting stuck in spots like this*, seemingly due to some race condition. I've been modifying this locally to use a more realistic value of like 20-30s, and that would clear up the problems for me.
Could this be made into a compile-time configurable value?

*ulTaskNotifyTake(pdTRUE, portMAX_DELAY);
Which I assume is equivalent to
NimBLEUtils::taskWait(*m_pTaskData, BLE_NPL_TIME_FOREVER);

Copy link
Owner Author

@h2zero h2zero Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this should be something of a config value.

Please open an issue for discussion to address this so we can get more user feedback.

} while (taskData.rc == (BLE_HS_ERR_HCI_BASE + BLE_ERR_PINKEY_MISSING) && retryCount--);
NimBLEUtils::taskWait(*m_pTaskData, BLE_NPL_TIME_FOREVER);
} while (taskData.m_flags == (BLE_HS_ERR_HCI_BASE + BLE_ERR_PINKEY_MISSING) && retryCount--);

if (taskData.rc != 0) {
m_lastErr = taskData.rc;
NIMBLE_LOGE(LOG_TAG, "secureConnection: failed rc=%d", taskData.rc);
if (taskData.m_flags != 0) {
m_lastErr = taskData.m_flags;
NIMBLE_LOGE(LOG_TAG, "secureConnection: failed rc=%d", taskData.m_flags);
return false;
}

Expand Down Expand Up @@ -721,9 +711,8 @@ bool NimBLEClient::retrieveServices(const NimBLEUUID* uuidFilter) {
return false;
}

int rc = 0;
TaskHandle_t cur_task = xTaskGetCurrentTaskHandle();
BleTaskData taskData = {this, cur_task, 0, nullptr};
int rc = 0;
NimBLETaskData taskData(this);

if (uuidFilter == nullptr) {
rc = ble_gattc_disc_all_svcs(m_connHandle, NimBLEClient::serviceDiscoveredCB, &taskData);
Expand All @@ -737,24 +726,15 @@ bool NimBLEClient::retrieveServices(const NimBLEUUID* uuidFilter) {
return false;
}

# ifdef ulTaskNotifyValueClear
// Clear the task notification value to ensure we block
ulTaskNotifyValueClear(cur_task, ULONG_MAX);
# endif

// wait until we have all the services
ulTaskNotifyTake(pdTRUE, portMAX_DELAY);
m_lastErr = taskData.rc;

if (taskData.rc == 0) {
NimBLEUtils::taskWait(taskData, BLE_NPL_TIME_FOREVER);
rc = taskData.m_flags;
if (rc == 0 || rc == BLE_HS_EDONE) {
return true;
} else {
NIMBLE_LOGE(LOG_TAG,
"Could not retrieve services, rc=%d %s",
taskData.rc,
NimBLEUtils::returnCodeToString(taskData.rc));
return false;
}

m_lastErr = rc;
NIMBLE_LOGE(LOG_TAG, "Could not retrieve services, rc=%d %s", rc, NimBLEUtils::returnCodeToString(rc));
return false;
} // getServices

/**
Expand All @@ -771,8 +751,8 @@ int NimBLEClient::serviceDiscoveredCB(uint16_t connHandle,
error->status,
(error->status == 0) ? service->start_handle : -1);

BleTaskData* pTaskData = (BleTaskData*)arg;
NimBLEClient* pClient = (NimBLEClient*)pTaskData->pATT;
NimBLETaskData* pTaskData = (NimBLETaskData*)arg;
NimBLEClient* pClient = (NimBLEClient*)pTaskData->m_pInstance;

// Make sure the service discovery is for this device
if (pClient->getConnHandle() != connHandle) {
Expand All @@ -785,15 +765,7 @@ int NimBLEClient::serviceDiscoveredCB(uint16_t connHandle,
return 0;
}

if (error->status == BLE_HS_EDONE) {
pTaskData->rc = 0;
} else {
NIMBLE_LOGE(LOG_TAG, "serviceDiscoveredCB() rc=%d %s", error->status, NimBLEUtils::returnCodeToString(error->status));
pTaskData->rc = error->status;
}

xTaskNotifyGive(pTaskData->task);

NimBLEUtils::taskRelease(*pTaskData, error->status);
NIMBLE_LOGD(LOG_TAG, "<< Service Discovered");
return error->status;
}
Expand Down Expand Up @@ -1189,10 +1161,7 @@ int NimBLEClient::handleGapEvent(struct ble_gap_event* event, void* arg) {
} // Switch

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

Expand Down
4 changes: 2 additions & 2 deletions src/NimBLEClient.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class NimBLEAdvertisedDevice;
class NimBLEAttValue;
class NimBLEClientCallbacks;
class NimBLEConnInfo;
struct BleTaskData;
struct NimBLETaskData;

/**
* @brief A model of a BLE client.
Expand Down Expand Up @@ -111,7 +111,7 @@ class NimBLEClient {
NimBLEAddress m_peerAddress;
mutable int m_lastErr;
int32_t m_connectTimeout;
mutable BleTaskData* m_pTaskData;
mutable NimBLETaskData* m_pTaskData;
std::vector<NimBLERemoteService*> m_svcVec;
NimBLEClientCallbacks* m_pClientCallbacks;
uint16_t m_connHandle;
Expand Down
36 changes: 12 additions & 24 deletions src/NimBLERemoteCharacteristic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,8 @@ int NimBLERemoteCharacteristic::descriptorDiscCB(
NIMBLE_LOGD(LOG_TAG, "Descriptor Discovery >> status: %d handle: %d", rc, (rc == 0) ? dsc->handle : -1);

auto filter = (desc_filter_t*)arg;
auto pTaskData = (BleTaskData*)filter->task_data;
const auto pChr = (NimBLERemoteCharacteristic*)pTaskData->pATT;
auto pTaskData = (NimBLETaskData*)filter->task_data;
const auto pChr = (NimBLERemoteCharacteristic*)pTaskData->m_pInstance;
const NimBLEUUID* uuidFilter = filter->uuid;

if (pChr->getHandle() != chr_val_handle) {
Expand All @@ -85,12 +85,8 @@ int NimBLERemoteCharacteristic::descriptorDiscCB(
pChr->m_vDescriptors.push_back(new NimBLERemoteDescriptor(pChr, dsc));
}

if (rc != 0) {
pTaskData->rc = rc == BLE_HS_EDONE ? 0 : rc;
NIMBLE_LOGD(LOG_TAG, "<< Descriptor Discovery");
xTaskNotifyGive(pTaskData->task);
}

NimBLEUtils::taskRelease(*pTaskData, rc);
NIMBLE_LOGD(LOG_TAG, "<< Descriptor Discovery");
return rc;
}

Expand All @@ -101,9 +97,8 @@ int NimBLERemoteCharacteristic::descriptorDiscCB(
bool NimBLERemoteCharacteristic::retrieveDescriptors(const NimBLEUUID* uuidFilter) const {
NIMBLE_LOGD(LOG_TAG, ">> retrieveDescriptors() for characteristic: %s", getUUID().toString().c_str());

TaskHandle_t cur_task = xTaskGetCurrentTaskHandle();
BleTaskData taskData = {const_cast<NimBLERemoteCharacteristic*>(this), cur_task, 0, nullptr};
desc_filter_t filter = {uuidFilter, &taskData};
NimBLETaskData taskData(const_cast<NimBLERemoteCharacteristic*>(this));
desc_filter_t filter = {uuidFilter, &taskData};

int rc = ble_gattc_disc_all_dscs(getClient()->getConnHandle(),
getHandle(),
Expand All @@ -115,22 +110,15 @@ bool NimBLERemoteCharacteristic::retrieveDescriptors(const NimBLEUUID* uuidFilte
return false;
}

# ifdef ulTaskNotifyValueClear
// Clear the task notification value to ensure we block
ulTaskNotifyValueClear(cur_task, ULONG_MAX);
# endif
ulTaskNotifyTake(pdTRUE, portMAX_DELAY);

if (taskData.rc != 0) {
NIMBLE_LOGE(LOG_TAG,
"<< retrieveDescriptors(): failed: rc=%d %s",
taskData.rc,
NimBLEUtils::returnCodeToString(taskData.rc));
} else {
NimBLEUtils::taskWait(taskData, BLE_NPL_TIME_FOREVER);
rc = taskData.m_flags;
if (rc == 0 || rc == BLE_HS_EDONE) {
NIMBLE_LOGD(LOG_TAG, "<< retrieveDescriptors(): found %d descriptors.", m_vDescriptors.size());
return true;
}

return taskData.rc == 0;
NIMBLE_LOGE(LOG_TAG, "<< retrieveDescriptors(): failed: rc=%d %s", rc, NimBLEUtils::returnCodeToString(rc));
return false;
} // retrieveDescriptors

/**
Expand Down
35 changes: 16 additions & 19 deletions src/NimBLERemoteService.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,8 +145,8 @@ int NimBLERemoteService::characteristicDiscCB(uint16_t conn_handle,
const ble_gatt_chr* chr,
void* arg) {
NIMBLE_LOGD(LOG_TAG, "Characteristic Discovery >>");
auto pTaskData = (BleTaskData*)arg;
const auto pSvc = (NimBLERemoteService*)pTaskData->pATT;
auto pTaskData = (NimBLETaskData*)arg;
const auto pSvc = (NimBLERemoteService*)pTaskData->m_pInstance;

// Make sure the discovery is for this device
if (pSvc->getClient()->getConnHandle() != conn_handle) {
Expand All @@ -155,12 +155,11 @@ int NimBLERemoteService::characteristicDiscCB(uint16_t conn_handle,

if (error->status == 0) {
pSvc->m_vChars.push_back(new NimBLERemoteCharacteristic(pSvc, chr));
} else {
pTaskData->rc = error->status == BLE_HS_EDONE ? 0 : error->status;
NIMBLE_LOGD(LOG_TAG, "<< Characteristic Discovery");
xTaskNotifyGive(pTaskData->task);
return 0;
}

NimBLEUtils::taskRelease(*pTaskData, error->status);
NIMBLE_LOGD(LOG_TAG, "<< Characteristic Discovery");
return error->status;
}

Expand All @@ -171,9 +170,8 @@ int NimBLERemoteService::characteristicDiscCB(uint16_t conn_handle,
*/
bool NimBLERemoteService::retrieveCharacteristics(const NimBLEUUID* uuidFilter) const {
NIMBLE_LOGD(LOG_TAG, ">> retrieveCharacteristics()");
int rc = 0;
TaskHandle_t cur_task = xTaskGetCurrentTaskHandle();
BleTaskData taskData = {const_cast<NimBLERemoteService*>(this), cur_task, 0, nullptr};
int rc = 0;
NimBLETaskData taskData(const_cast<NimBLERemoteService*>(this));

if (uuidFilter == nullptr) {
rc = ble_gattc_disc_all_chrs(m_pClient->getConnHandle(),
Expand All @@ -190,21 +188,20 @@ bool NimBLERemoteService::retrieveCharacteristics(const NimBLEUUID* uuidFilter)
&taskData);
}

if (rc == 0) {
# ifdef ulTaskNotifyValueClear
// Clear the task notification value to ensure we block
ulTaskNotifyValueClear(cur_task, ULONG_MAX);
# endif
ulTaskNotifyTake(pdTRUE, portMAX_DELAY);
if (rc != 0) {
NIMBLE_LOGE(LOG_TAG, "ble_gattc_disc_chrs rc=%d %s", rc, NimBLEUtils::returnCodeToString(rc));
return false;
}

if (taskData.rc != 0) {
NIMBLE_LOGE(LOG_TAG, "<< retrieveCharacteristics() rc=%d %s", rc, NimBLEUtils::returnCodeToString(rc));
} else {
NimBLEUtils::taskWait(taskData, BLE_NPL_TIME_FOREVER);
rc = taskData.m_flags;
if (rc == 0 || rc == BLE_HS_EDONE) {
NIMBLE_LOGD(LOG_TAG, "<< retrieveCharacteristics()");
return true;
}

return taskData.rc == 0;
NIMBLE_LOGE(LOG_TAG, "<< retrieveCharacteristics() rc=%d %s", rc, NimBLEUtils::returnCodeToString(rc));
return false;
} // retrieveCharacteristics

/**
Expand Down
Loading