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: replace malloc with cordio buffer allocation #8269

Merged
merged 8 commits into from Oct 6, 2018

Conversation

Projects
None yet
5 participants
@paul-szczepanek-arm
Member

paul-szczepanek-arm commented Sep 27, 2018

Description

Event queue used in BLE was using allocating message on the queue from the heap. This causes an error when called from an interrupt context. This replaces the new (which calls malloc) with placement new which uses the provided memory. Memory is provided from a pool belonging to cordio which is where the queue is only used (hence not pulling in any new dependency).

Error discovered using the BLE_GAP example which was used to validate the fix.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Breaking change

@paul-szczepanek-arm paul-szczepanek-arm requested a review from pan- Sep 27, 2018

@@ -73,8 +73,8 @@ struct SimpleEventQueue : EventQueue {
if (_ble_base == NULL) {
return false;
}
EventNode* next = new (std::nothrow) EventNode(event);
uint8_t* event_buf = (uint8_t*)WsfBufAlloc(sizeof(EventNode));

This comment has been minimized.

@pan-

pan- Sep 27, 2018

Member
  • The NULL check should be made after WsfBufAlloc.
  • placement new accepts a void*. The cast to uint8_t is not required.
@pan-

Could you move SimpleEventQueue into TARGET_CORDIO as this is a cordio implementation ?

paul-szczepanek-arm added some commits Sep 27, 2018

@pan-

In the process of moving the basic event queue implementation, could you change its namespace as well: ble::vendor::cordio.

@@ -22,7 +22,7 @@
#include "ble/BLEInstanceBase.h"
#include "ble/generic/GenericGattClient.h"
#include "ble/generic/GenericSecurityManager.h"
#include "ble/pal/SimpleEventQueue.h"
#include "SimpleEventQueue.h"

This comment has been minimized.

@pan-

pan- Sep 27, 2018

Member

That's not used by nordic. We can remove that include.

@@ -22,7 +22,7 @@
#include "ble/BLEInstanceBase.h"
#include "ble/generic/GenericGattClient.h"
#include "ble/generic/GenericSecurityManager.h"
#include "ble/pal/SimpleEventQueue.h"
#include "SimpleEventQueue.h"

This comment has been minimized.

@pan-

pan- Sep 27, 2018

Member

That's not used by nordic. We can remove that include.

@cmonr cmonr added the needs: review label Sep 27, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 27, 2018

@pan- Looks like it's ready for another speed-review.

paul-szczepanek-arm added some commits Sep 27, 2018

@paul-szczepanek-arm

This comment has been minimized.

Member

paul-szczepanek-arm commented Sep 27, 2018

is now

EventNode* next = new (std::nothrow) EventNode(event);
void* event_buf = WsfBufAlloc(sizeof(EventNode));
if (event_buf == NULL) {
error("\r\n%s:%d Cordio WsfBufAlloc out of memory\r\n", __FILE__, __LINE__);

This comment has been minimized.

@pan-

pan- Sep 27, 2018

Member

I think that's a leftover for tracing.

This comment has been minimized.

@paul-szczepanek-arm

paul-szczepanek-arm Sep 27, 2018

Member

it's a unrecoverable error, what do you want to do?

@pan-

pan- approved these changes Sep 27, 2018

@pan-

This comment has been minimized.

Member

pan- commented Sep 27, 2018

@cmonr looks good now.

@cmonr cmonr added needs: CI and removed needs: review labels Sep 27, 2018

@0xc0170

0xc0170 approved these changes Oct 5, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 5, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 5, 2018

Build : SUCCESS

Build number : 3250
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/8269/

Triggering tests

/morph test
/morph mbed2-build

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 5, 2018

/morph export-build

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 5, 2018

License issue

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 5, 2018

/morph export-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 merged commit 5faac0e into ARMmbed:master Oct 6, 2018

15 checks passed

ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed , RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/cloud_client_smoke_test Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Passed, 625 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9999 cycles (-216 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@0xc0170 0xc0170 removed the ready for merge label Oct 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment