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.

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

Ble notification/indication status and timeout #2998

Merged
merged 3 commits into from
Aug 20, 2019

Conversation

romansavrulin
Copy link
Contributor

@romansavrulin romansavrulin commented Jul 17, 2019

Sometimes it is useful to see if our BLE indication succeed or not. Current BLE implementation doesn't provide such functionality and has potential deadlock if indication confirmation response is not delivered to ESP by any reason.

This PR adds:

  • TimedWait to FreeRTOS abstraction layer
  • Indication timeout (and fix possible deadlock on semaphore)
  • Indication/Notification status callback, to give precise control over messages sent to BLE peer.
  • Backward codebase compatibility with previous sketches.

@@ -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

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

@me-no-dev me-no-dev merged commit 5137fc5 into espressif:master Aug 20, 2019
@romansavrulin romansavrulin deleted the ble-notification-status branch September 26, 2019 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants