BLE: replace malloc with cordio buffer allocation #8269

paul-szczepanek-arm · 2018-09-27T11:23:31Z

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

pan- · 2018-09-27T11:35:49Z

features/FEATURE_BLE/ble/pal/SimpleEventQueue.h

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


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 ?

pan-

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

pan- · 2018-09-27T13:00:05Z

features/FEATURE_BLE/targets/TARGET_NORDIC/TARGET_NRF52/source/nRF5xn.h

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


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

pan- · 2018-09-27T13:00:12Z

features/FEATURE_BLE/targets/TARGET_NORDIC/TARGET_NRF51/source/nRF5xn.h

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


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

cmonr · 2018-09-27T15:35:40Z

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

paul-szczepanek-arm · 2018-09-27T16:00:46Z

is now

pan- · 2018-09-27T16:04:38Z

features/FEATURE_BLE/targets/TARGET_CORDIO/SimpleEventQueue.h

-        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__);


I think that's a leftover for tracing.

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

pan- · 2018-09-27T17:17:59Z

@cmonr looks good now.

0xc0170 · 2018-10-05T11:54:20Z

/morph build

mbed-ci · 2018-10-05T13:00:43Z

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 · 2018-10-05T14:55:50Z

/morph export-build

mbed-ci · 2018-10-05T17:12:27Z

Exporter Build : FAILURE

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

0xc0170 · 2018-10-05T17:45:02Z

License issue

0xc0170 · 2018-10-05T17:45:09Z

/morph export-build

mbed-ci · 2018-10-05T20:08:38Z

Exporter Build : SUCCESS

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

mbed-ci · 2018-10-06T04:45:26Z

Test : SUCCESS

Build number : 3057
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/8269/3057

replace malloc with cirdio buffer allocation

90be8d7

paul-szczepanek-arm requested a review from pan- September 27, 2018 11:23

pan- requested changes Sep 27, 2018

View reviewed changes

paul-szczepanek-arm added 2 commits September 27, 2018 13:45

check WsfBufAlloc

46d19d3

moved SimpleEventQueue

abfd647

pan- requested changes Sep 27, 2018

View reviewed changes

removed unusused header

5fee50b

cmonr added the needs: review label Sep 27, 2018

paul-szczepanek-arm added 2 commits September 27, 2018 16:36

change namespace

a0df7fc

fix namespace errors

64f9358

remove pointless cast

8f5e272

pan- requested changes Sep 27, 2018

View reviewed changes

assert instead of error

88a8751

pan- approved these changes Sep 27, 2018

View reviewed changes

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

0xc0170 approved these changes Oct 5, 2018

View reviewed changes

0xc0170 added the release-version: 5.10.2 label Oct 5, 2018

0xc0170 added ready for merge release-version: 5.10.1 and removed needs: CI release-version: 5.10.2 labels Oct 6, 2018

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

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

BLE: replace malloc with cordio buffer allocation #8269

BLE: replace malloc with cordio buffer allocation #8269

Uh oh!

Conversation

paul-szczepanek-arm commented Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pan- left a comment

Choose a reason for hiding this comment

Uh oh!

pan- left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmonr commented Sep 27, 2018

Uh oh!

paul-szczepanek-arm commented Sep 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pan- commented Sep 27, 2018

Uh oh!

0xc0170 commented Oct 5, 2018

Uh oh!

mbed-ci commented Oct 5, 2018

Triggering tests

Uh oh!

0xc0170 commented Oct 5, 2018

Uh oh!

mbed-ci commented Oct 5, 2018

Uh oh!

0xc0170 commented Oct 5, 2018

Uh oh!

0xc0170 commented Oct 5, 2018

Uh oh!

mbed-ci commented Oct 5, 2018

Uh oh!

mbed-ci commented Oct 6, 2018

Uh oh!

Uh oh!

paul-szczepanek-arm commented Sep 27, 2018 •

edited

Loading