Skip to content

refactor(RemoteServ): Reduce nesting #291

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

refactor(RemoteServ): Reduce nesting #291

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
149 changes: 59 additions & 90 deletions src/NimBLERemoteService.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,50 +76,34 @@ NimBLERemoteCharacteristic* NimBLERemoteService::getCharacteristic(const char* u
*/
NimBLERemoteCharacteristic* NimBLERemoteService::getCharacteristic(const NimBLEUUID& uuid) const {
NIMBLE_LOGD(LOG_TAG, ">> getCharacteristic: uuid: %s", uuid.toString().c_str());
NimBLERemoteCharacteristic* pChar = nullptr;
size_t prev_size = m_vChars.size();
NimBLERemoteCharacteristic* pChar = nullptr;
NimBLEUUID uuidTmp;

for (const auto& it : m_vChars) {
if (it->getUUID() == uuid) {
pChar = it;
for (const auto& chr : m_vChars) {
if (chr->getUUID() == uuid) {
pChar = chr;
goto Done;
}
}

if (retrieveCharacteristics(&uuid)) {
if (m_vChars.size() > prev_size) {
pChar = m_vChars.back();
goto Done;
}

// If the request was successful but 16/32 bit uuid not found
// try again with the 128 bit uuid.
if (uuid.bitSize() == BLE_UUID_TYPE_16 || uuid.bitSize() == BLE_UUID_TYPE_32) {
NimBLEUUID uuid128(uuid);
uuid128.to128();
if (retrieveCharacteristics(&uuid128)) {
if (m_vChars.size() > prev_size) {
pChar = m_vChars.back();
}
}
} else {
// If the request was successful but the 128 bit uuid not found
// try again with the 16 bit uuid.
NimBLEUUID uuid16(uuid);
uuid16.to16();
// if the uuid was 128 bit but not of the BLE base type this check will fail
if (uuid16.bitSize() == BLE_UUID_TYPE_16) {
if (retrieveCharacteristics(&uuid16)) {
if (m_vChars.size() > prev_size) {
pChar = m_vChars.back();
}
}
}
}
if (!retrieveCharacteristics(&uuid, pChar) || pChar) {
goto Done;
}
// Try again with 128 bit uuid if request succeeded with no uuid found.
if (uuid.bitSize() == BLE_UUID_TYPE_16 || uuid.bitSize() == BLE_UUID_TYPE_32) {
uuidTmp = NimBLEUUID(uuid).to128();
retrieveCharacteristics(&uuidTmp, pChar);
goto Done;
}
// Try again with 16 bit uuid if request succeeded with no uuid found.
// If the uuid was 128 bit but not of the BLE base type this check will fail.
uuidTmp = NimBLEUUID(uuid).to16();
if (uuidTmp.bitSize() == BLE_UUID_TYPE_16) {
retrieveCharacteristics(&uuidTmp, pChar);
}

Done:
NIMBLE_LOGD(LOG_TAG, "<< Characteristic %sfound", pChar ? "" : "not ");
NIMBLE_LOGD(LOG_TAG, "<< getCharacteristic: %sfound", !pChar ? "not " : "");
return pChar;
} // getCharacteristic

Expand All @@ -143,77 +127,68 @@ const std::vector<NimBLERemoteCharacteristic*>& NimBLERemoteService::getCharacte
* @brief Callback for Characteristic discovery.
* @return success == 0 or error code.
*/
int NimBLERemoteService::characteristicDiscCB(uint16_t conn_handle,
const ble_gatt_error* error,
const ble_gatt_chr* chr,
void* arg) {
NIMBLE_LOGD(LOG_TAG,
"Characteristic Discovery >> status: %d handle: %d",
error->status,
(error->status == 0) ? chr->def_handle : -1);
int NimBLERemoteService::chrDiscCB(uint16_t connHandle,
const ble_gatt_error* error,
const ble_gatt_chr* chr,
void* arg) {
const int rc = error->status;
auto pTaskData = (NimBLETaskData*)arg;
const auto pSvc = (NimBLERemoteService*)pTaskData->m_pInstance;

if (error->status == BLE_HS_ENOTCONN) {
NIMBLE_LOGE(LOG_TAG, "<< Characteristic Discovery; Not connected");
NimBLEUtils::taskRelease(*pTaskData, error->status);
return error->status;
}
NIMBLE_LOGD(LOG_TAG, "Characteristic Discovery >> status: %d handle: %d", rc, (rc == 0) ? chr->def_handle : -1);

// Make sure the discovery is for this device
if (pSvc->getClient()->getConnHandle() != conn_handle) {
if (pSvc->getClient()->getConnHandle() != connHandle) {
return 0;
}

if (error->status == 0) {
if (rc == 0) {
pSvc->m_vChars.push_back(new NimBLERemoteCharacteristic(pSvc, chr));
return 0;
}

NimBLEUtils::taskRelease(*pTaskData, error->status);
NIMBLE_LOGD(LOG_TAG, "<< Characteristic Discovery");
return error->status;
NimBLEUtils::taskRelease(*pTaskData, rc);
NIMBLE_LOGD(LOG_TAG, "<< Characteristic Discovery%s", (rc == BLE_HS_ENOTCONN) ? "; Not connected" : "");
return rc;
}

/**
* @brief Retrieve all the characteristics for this service.
* This function will not return until we have all the characteristics.
* @return True if successful.
* @return True if successfully retrieved, success = BLE_HS_EDONE.
*/
bool NimBLERemoteService::retrieveCharacteristics(const NimBLEUUID* uuidFilter) const {
bool NimBLERemoteService::retrieveCharacteristics(const NimBLEUUID* uuid, NimBLERemoteCharacteristic* out) const {
NIMBLE_LOGD(LOG_TAG, ">> retrieveCharacteristics()");
int rc = 0;
NimBLETaskData taskData(const_cast<NimBLERemoteService*>(this));

if (uuidFilter == nullptr) {
rc = ble_gattc_disc_all_chrs(m_pClient->getConnHandle(),
getHandle(),
getEndHandle(),
NimBLERemoteService::characteristicDiscCB,
&taskData);
} else {
rc = ble_gattc_disc_chrs_by_uuid(m_pClient->getConnHandle(),
getHandle(),
getEndHandle(),
uuidFilter->getBase(),
NimBLERemoteService::characteristicDiscCB,
&taskData);
const uint16_t hdlConn = m_pClient->getConnHandle();
const uint16_t hdlEnd = getEndHandle();
const uint16_t hdlStart = getHandle();
// If this is the last handle then there are no more characteristics
if (hdlStart == hdlEnd) {
NIMBLE_LOGD(LOG_TAG, "<< retrieveCharacteristics(): found %d characteristics.", m_vChars.size());
return true;
}

int rc = (uuid == nullptr)
? ble_gattc_disc_all_chrs(hdlConn, hdlStart, hdlEnd, chrDiscCB, &taskData)
: ble_gattc_disc_chrs_by_uuid(hdlConn, hdlStart, hdlEnd, uuid->getBase(), chrDiscCB, &taskData);

if (rc != 0) {
NIMBLE_LOGE(LOG_TAG, "ble_gattc_disc_chrs rc=%d %s", rc, NimBLEUtils::returnCodeToString(rc));
return false;
}

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;
if (rc != BLE_HS_EDONE) {
NIMBLE_LOGE(LOG_TAG, "<< retrieveCharacteristics(): failed: rc=%d %s", rc, NimBLEUtils::returnCodeToString(rc));
return false;
}

NIMBLE_LOGE(LOG_TAG, "<< retrieveCharacteristics() rc=%d %s", rc, NimBLEUtils::returnCodeToString(rc));
return false;
if (out) {
Copy link
Owner

Choose a reason for hiding this comment

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

💯

Copy link
Owner

Choose a reason for hiding this comment

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

You have to love those mysterious compiler warnings when it comes to pointers, been there done that lol!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I could have swore the consolidation PR omits the nullptr check and was able to pass.
Wasted so much time on this stupid error lol, especially since it was building fine on my end.
I'm assuming the build check uses more pedantic flags or something, would be nice to have a similar setup locally.

out = m_vChars.back();
}
NIMBLE_LOGD(LOG_TAG, "<< retrieveCharacteristics(): found %d characteristics.", m_vChars.size());
return true;
} // retrieveCharacteristics

/**
Expand All @@ -231,11 +206,8 @@ NimBLEClient* NimBLERemoteService::getClient() const {
*/
NimBLEAttValue NimBLERemoteService::getValue(const NimBLEUUID& uuid) const {
const auto pChar = getCharacteristic(uuid);
if (pChar) {
return pChar->readValue();
}

return NimBLEAttValue{};
return pChar ? pChar->readValue()
: NimBLEAttValue{};
} // readValue

/**
Expand All @@ -246,11 +218,8 @@ NimBLEAttValue NimBLERemoteService::getValue(const NimBLEUUID& uuid) const {
*/
bool NimBLERemoteService::setValue(const NimBLEUUID& uuid, const NimBLEAttValue& value) const {
const auto pChar = getCharacteristic(uuid);
if (pChar) {
return pChar->writeValue(value);
}

return false;
return pChar ? pChar->writeValue(value)
: false;
} // setValue

/**
Expand All @@ -260,8 +229,8 @@ bool NimBLERemoteService::setValue(const NimBLEUUID& uuid, const NimBLEAttValue&
* them. This method does just that.
*/
void NimBLERemoteService::deleteCharacteristics() const {
for (const auto& it : m_vChars) {
delete it;
for (const auto& chr : m_vChars) {
delete chr;
}
std::vector<NimBLERemoteCharacteristic*>{}.swap(m_vChars);
} // deleteCharacteristics
Expand Down Expand Up @@ -303,4 +272,4 @@ std::string NimBLERemoteService::toString() const {
return res;
} // toString

#endif /* CONFIG_BT_ENABLED && CONFIG_BT_NIMBLE_ROLE_CENTRAL */
#endif /* CONFIG_BT_ENABLED && CONFIG_BT_NIMBLE_ROLE_CENTRAL */
13 changes: 7 additions & 6 deletions src/NimBLERemoteService.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,16 +53,17 @@ class NimBLERemoteService : public NimBLEAttribute {

NimBLERemoteService(NimBLEClient* pClient, const struct ble_gatt_svc* service);
~NimBLERemoteService();
bool retrieveCharacteristics(const NimBLEUUID* uuidFilter = nullptr) const;
static int characteristicDiscCB(uint16_t conn_handle,
const struct ble_gatt_error* error,
const struct ble_gatt_chr* chr,
void* arg);
bool retrieveCharacteristics(const NimBLEUUID* uuid = nullptr,
NimBLERemoteCharacteristic* out = nullptr) const;
static int chrDiscCB(uint16_t connHandle,
const struct ble_gatt_error* error,
const struct ble_gatt_chr* chr,
void* arg);

mutable std::vector<NimBLERemoteCharacteristic*> m_vChars{};
NimBLEClient* m_pClient{nullptr};
uint16_t m_endHandle{0};
}; // NimBLERemoteService

#endif /* CONFIG_BT_ENABLED && CONFIG_BT_NIMBLE_ROLE_CENTRAL */
#endif /* NIMBLE_CPP_REMOTE_SERVICE_H_*/
#endif /* NIMBLE_CPP_REMOTE_SERVICE_H_*/
Loading