Skip to content

Ble notification/indication status and timeout #2998

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
merged 3 commits into from
Aug 20, 2019
Merged

Ble notification/indication status and timeout #2998

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
090ae85
add timed wait
romansavrulin Jul 17, 2019
bd7b3dc
Added Notification/Indication data and status callbacks
romansavrulin Jun 12, 2019
2255bf8
imply null-object pattern for BLE callback
romansavrulin Jul 18, 2019
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
54 changes: 51 additions & 3 deletions libraries/BLE/src/BLECharacteristic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,10 +480,20 @@ void BLECharacteristic::notify(bool is_notification) {
assert(getService() != nullptr);
assert(getService()->getServer() != nullptr);

auto cb = BLECharacteristicCallbacks();
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no need to create additional callback, which will be created easch time you send notification/indications (memory leak).

Just use the same mechanism like with onRead/onWrite:

if(m_pCallback != nullptr){
     callback->onNotify(this);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chegewara I personally prefer to use Null object pattern to maintain KISS and DRY

It's not a big deal for performance or memory considerations with BLE data rates. But much simpler to read, isn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

If it wont make memory leak, then ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chegewara It's a scoped variable, placed on stack. It is automatically deleted when we leave its outbound {} block ( see RAII )

Copy link
Collaborator

Choose a reason for hiding this comment

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

for consistency it should be using the same approach as onRead/onWrite...

also this shouldn't leak any memory as it is allocated and freed automatically as part of the method execution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@atanisoft Did I got you right: you mean use Null object pattern in other callback checks?

Copy link
Collaborator

@atanisoft atanisoft Jul 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The general approach used so far (in this and other libs) is:

if(m_pCallback){
     m_pCallback->onNotify(this);
}

I'm not certain your approach is any cleaner/readable. The only thing I'd suggest is use one pattern and stick to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@atanisoft so, you mean readability of the code with extra

if(m_pCallback){
     m_pCallback->onNotify(this);
}

in dozen places with other 5-level conditionals is much cleaner than

callback->onNotify(this);

and we should use that approach because we used to? This doesn't sound convictive to me, sorry. I'd prefer to replace other usages of that callback to decrease overall codebase and improve maintainability.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm fine either way, just be consistent in usage :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@atanisoft Thanks, Mike


BLECharacteristicCallbacks * callback = &cb;
if(m_pCallbacks != nullptr){
callback = m_pCallbacks;
}

callback->onNotify(this); // Invoke the notify callback.

GeneralUtils::hexDump((uint8_t*)m_value.getValue().data(), m_value.getValue().length());

if (getService()->getServer()->getConnectedCount() == 0) {
log_v("<< notify: No connected clients.");
callback->onStatus(this, BLECharacteristicCallbacks::Status::ERROR_NO_CLIENT, 0);
return;
}

Expand All @@ -494,12 +504,14 @@ void BLECharacteristic::notify(bool is_notification) {
if(is_notification) {
if (p2902 != nullptr && !p2902->getNotifications()) {
log_v("<< notifications disabled; ignoring");
callback->onStatus(this, BLECharacteristicCallbacks::Status::ERROR_NOTIFY_DISABLED, 0); // Invoke the notify callback.
return;
}
}
else{
if (p2902 != nullptr && !p2902->getIndications()) {
log_v("<< indications disabled; ignoring");
callback->onStatus(this, BLECharacteristicCallbacks::Status::ERROR_INDICATE_DISABLED, 0); // Invoke the notify callback.
return;
}
}
Expand All @@ -510,7 +522,7 @@ void BLECharacteristic::notify(bool is_notification) {
}

size_t length = m_value.getValue().length();
if(!is_notification)
if(!is_notification) // is indication
m_semaphoreConfEvt.take("indicate");
esp_err_t errRc = ::esp_ble_gatts_send_indicate(
getService()->getServer()->getGattsIf(),
Expand All @@ -519,10 +531,23 @@ void BLECharacteristic::notify(bool is_notification) {
if (errRc != ESP_OK) {
log_e("<< esp_ble_gatts_send_ %s: rc=%d %s",is_notification?"notify":"indicate", errRc, GeneralUtils::errorToString(errRc));
m_semaphoreConfEvt.give();
callback->onStatus(this, BLECharacteristicCallbacks::Status::ERROR_GATT, errRc); // Invoke the notify callback.
return;
}
if(!is_notification)
m_semaphoreConfEvt.wait("indicate");
if(!is_notification){ // is indication
if(!m_semaphoreConfEvt.timedWait("indicate", indicationTimeout)){
callback->onStatus(this, BLECharacteristicCallbacks::Status::ERROR_INDICATE_TIMEOUT, 0); // Invoke the notify callback.
} else {
auto code = (esp_gatt_status_t) m_semaphoreConfEvt.value();
if(code == ESP_GATT_OK) {
callback->onStatus(this, BLECharacteristicCallbacks::Status::SUCCESS_INDICATE, code); // Invoke the notify callback.
} else {
callback->onStatus(this, BLECharacteristicCallbacks::Status::ERROR_INDICATE_FAILURE, code);
}
}
} else {
callback->onStatus(this, BLECharacteristicCallbacks::Status::SUCCESS_NOTIFY, 0); // Invoke the notify callback.
}
}
log_v("<< notify");
} // Notify
Expand Down Expand Up @@ -754,4 +779,27 @@ void BLECharacteristicCallbacks::onWrite(BLECharacteristic* pCharacteristic) {
log_d("BLECharacteristicCallbacks", "<< onWrite");
} // onWrite


/**
* @brief Callback function to support a Notify request.
* @param [in] pCharacteristic The characteristic that is the source of the event.
*/
void BLECharacteristicCallbacks::onNotify(BLECharacteristic* pCharacteristic) {
log_d("BLECharacteristicCallbacks", ">> onNotify: default");
log_d("BLECharacteristicCallbacks", "<< onNotify");
} // onNotify


/**
* @brief Callback function to support a Notify/Indicate Status report.
* @param [in] pCharacteristic The characteristic that is the source of the event.
* @param [in] s Status of the notification/indication
* @param [in] code Additional code of underlying errors
*/
void BLECharacteristicCallbacks::onStatus(BLECharacteristic* pCharacteristic, Status s, uint32_t code) {
log_d("BLECharacteristicCallbacks", ">> onStatus: default");
log_d("BLECharacteristicCallbacks", "<< onStatus");
} // onStatus


#endif /* CONFIG_BT_ENABLED */
15 changes: 15 additions & 0 deletions libraries/BLE/src/BLECharacteristic.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,8 @@ class BLECharacteristic {
static const uint32_t PROPERTY_INDICATE = 1<<4;
static const uint32_t PROPERTY_WRITE_NR = 1<<5;

static const uint32_t indicationTimeout = 1000;

private:

friend class BLEServer;
Expand Down Expand Up @@ -130,9 +132,22 @@ class BLECharacteristic {
*/
class BLECharacteristicCallbacks {
public:
typedef enum {
SUCCESS_INDICATE,
SUCCESS_NOTIFY,
ERROR_INDICATE_DISABLED,
ERROR_NOTIFY_DISABLED,
ERROR_GATT,
ERROR_NO_CLIENT,
ERROR_INDICATE_TIMEOUT,
ERROR_INDICATE_FAILURE
}Status;

virtual ~BLECharacteristicCallbacks();
virtual void onRead(BLECharacteristic* pCharacteristic);
virtual void onWrite(BLECharacteristic* pCharacteristic);
virtual void onNotify(BLECharacteristic* pCharacteristic);
virtual void onStatus(BLECharacteristic* pCharacteristic, Status s, uint32_t code);
};
#endif /* CONFIG_BT_ENABLED */
#endif /* COMPONENTS_CPP_UTILS_BLECHARACTERISTIC_H_ */
32 changes: 32 additions & 0 deletions libraries/BLE/src/FreeRTOS.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,38 @@ uint32_t FreeRTOS::Semaphore::wait(std::string owner) {
return m_value;
} // wait

/**
* @brief Wait for a semaphore to be released in a given period of time by trying to take it and
* then releasing it again. The value associated with the semaphore can be taken by value() call after return
* @param [in] owner A debug tag.
* @param [in] timeoutMs timeout to wait in ms.
* @return True if we took the semaphore within timeframe.
*/
bool FreeRTOS::Semaphore::timedWait(std::string owner, uint32_t timeoutMs) {
log_v(">> wait: Semaphore waiting: %s for %s", toString().c_str(), owner.c_str());

if (m_usePthreads && timeoutMs != portMAX_DELAY) {
assert(false); // We apparently don't have a timed wait for pthreads.
}

auto ret = pdTRUE;

if (m_usePthreads) {
pthread_mutex_lock(&m_pthread_mutex);
} else {
ret = xSemaphoreTake(m_semaphore, timeoutMs);
}

if (m_usePthreads) {
pthread_mutex_unlock(&m_pthread_mutex);
} else {
xSemaphoreGive(m_semaphore);
}

log_v("<< wait: Semaphore %s released: %d", toString().c_str(), ret);
return ret;
} // wait


FreeRTOS::Semaphore::Semaphore(std::string name) {
m_usePthreads = false; // Are we using pThreads or FreeRTOS?
Expand Down
2 changes: 2 additions & 0 deletions libraries/BLE/src/FreeRTOS.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ class FreeRTOS {
bool take(uint32_t timeoutMs, std::string owner = "<Unknown>");
std::string toString();
uint32_t wait(std::string owner = "<Unknown>");
bool timedWait(std::string owner = "<Unknown>", uint32_t timeoutMs = portMAX_DELAY);
uint32_t value(){ return m_value; };

private:
SemaphoreHandle_t m_semaphore;
Expand Down