BLE: Cordio port #5060

pan- · 2017-09-08T16:06:38Z

Description

Add a BLE port based on the ARM Cordio stack into mbed OS.
It allows mbed OS users to exploit BLE API on any targets which is connected to an HCI compatible module.

Status

READY

Migrations

NO

theotherjimmy

Why TARGET_CORDIO?
Why TARGET_CORTEX?
Why not TARGTE_CORTEX_{M0,M0PLUS,M3,M4}
Trying to build this will result in a link error with duplicate symbols x4 for each and every cordio function in the static lib.

theotherjimmy · 2017-09-08T19:47:26Z

Also, what does an example look like?

pan- · 2017-09-08T20:36:01Z

@theotherjimmy

Why TARGET_CORDIO?

This is BLE implementation based on the Cordio stack, do you have another suggestion for the name ? AFAIR there is no concept of implementation usable in the build system so we have to rely on the target concept.

Why TARGET_CORTEX?
Why not TARGTE_CORTEX_{M0,M0PLUS,M3,M4}
Trying to build this will result in a link error with duplicate symbols x4 for each and every cordio function in the static lib.

That's a mistake, that I need to fix. Binary came out that way from the build machine since a long time and I never had a single error with any of the compiler we support (ARM, GCC_ARM, IAR). I suppose the linker doesn't consider objects files compiled for a different cpu that the one specified in command line.

Also, what does an example look like?

https://github.com/pan-/mbed/blob/cordio_port/features/FEATURE_BLE/targets/TARGET_CORDIO/doc/PortingGuide.md

I will add one based on BLE examples.

theotherjimmy · 2017-09-08T20:37:05Z

This is BLE implementation based on the Cordio stack, do you have another suggestion for the name ?

FEATURE_CORDIO?

You could then have an mbed_lib.json file that contains:

{
    "target_overrides" :{
         "*" : {
            "target.features_add": ["BLE"]
        }
    }
}

This has a few benefits:

A user can't turn off BLE when they turn on CORDIO (it's a config error)
A user will get BLE implicitly when they turn on CORDIO
Shorter paths.

theotherjimmy · 2017-09-08T20:37:17Z

That's a mistake, that I need to fix.

👍

theotherjimmy · 2017-09-08T20:40:50Z

I suppose the linker is doesn't consider objects files compiled for a different cpu that the one specified in command line.

Huh. Let's not depend on it.

pan- · 2017-09-08T20:54:09Z

FEATURE_CORDIO?

Why would it be more appropriate to use FEATURE_CORDIO than TARGET_CORDIO ?

From my perspective targets are exclusive from one another not inclusive like features can be and that's what we want in that precise case. But I might be completely wrong.

We also use TARGET to include the ST shield implementation of BLE API in all our examples however IIRC TARGET was used for "technical limitations" at the time.

theotherjimmy · 2017-09-08T21:28:00Z

From my perspective targets are exclusive from one another not inclusive like features

Well, that's simply not true.

theotherjimmy · 2017-09-08T21:28:38Z

"technical limitations" at the time.

Probably that nested features do not work/are not allowed.

theotherjimmy · 2017-09-08T21:35:04Z

The reason that I'm suggesting a feature, is that this (mbed_app.json) might be better:

{
    "target_overrides" :{
         "*" : {
            "target.features_add": ["CORDIO"]
        }
    }
}

than this (mbed_app.json):

{
    "target_overrides" :{
         "*" : {
            "target.extra_labes_add": ["CORDIO"]
        }
    }
}

I think that they will both work, and I think that extra_labels are more like black magic than features.

pan- · 2017-09-11T08:45:39Z

Probably that nested features do not work/are not allowed.

Thanks for pointing that out. If nested features are not allowed then I'm not sure to understand why TARGET_CORDIO should be replace by FEATURE_CORDIO because it live inside FEATURE_BLE.

That also means security and hci features fo the cordio stack should go inside targets.

theotherjimmy · 2017-09-11T14:22:50Z

@pan- I think you missed/I incorrectly worded that I'm advocating for NOT putting FEATURE_CORDIO inside FEATURE_BLE. Instead, I recommend FEATURE_CORDIO in the same directory as FEATURE_BLE and have the mbed_lib.json do "target.features_add" : ["BLE"]. This is generally how we have features depend on other features in mbed OS.

theotherjimmy · 2017-09-11T14:25:20Z

@pan- I don't like calling anything but a vendor, family, mcu, module or board a "target". I don't want to use TARGET_ as a catch all "anything goes" category. I also don't want to see very nested structure in mbed-os. It's already bad as it is!

pan- · 2017-09-11T22:33:17Z

@theotherjimmy Library path has been fixed and examples can be run with the Cordio port here.

@sg- Should I put the Cordio stack in a feature as suggested by Jimmy ? I have no particular opinion about this change however it disrupt current practices. As an example, the ST Blue NRG library was always imported as a target, not a feature. I also want to point out that if the Cordio stack is packaged as a feature, it's very likely to experience errors if the feature is enabled on targets which already provide an implementation of BLE API (Nordic, Maxim, Beetle).

theotherjimmy · 2017-09-12T00:01:22Z

I also want to point out that if the Cordio stack is packaged as a feature, it's very likely to experience errors if the feature is enabled on targets which already provide an implementation of BLE API (Nordic, Maxim, Beetle).

How is this any different than of it was packaged as a target? A user could just do "target.extra_labels_add" : ["CORDIO"] to see the same errors. I think that you should definitely add "target.features_add": ["BLE"] to the mbed_lib.json either way. A user that turns on cordio probably knows that it's for ble.

sg- · 2017-09-12T13:20:25Z

This is the default stack therefore would expect its enabled by default, not requiring any configuration (same goes for BLE). Maybe this isn't quite ready yet.

theotherjimmy · 2017-09-12T16:47:27Z

This is the default stack therefore would expect its enabled by default,

And then you would do "target.features_remove": ["CORDIO"] only if you wanted to remove it? (I think that's much cleaner than "target.extra_labels_remove": ["CORDIO"])

not requiring any configuration (same goes for BLE)

Which would be nice, agreed.

sg- · 2017-09-12T23:12:39Z

Given this is early access and just needs to land to master we can iterate how it is used or not used.

theotherjimmy · 2017-09-14T21:34:55Z

Approved with the understanding that we can bikeshed about directory structure later.

0xc0170 · 2017-09-20T13:35:19Z

As my understanding, to fix jenkins CI, we need https://github.com/ARMmbed/mbed-os-cliapp/pull/307, should be in soon

0xc0170 · 2017-09-20T13:42:29Z

The fix is in, restarted CI

0xc0170 · 2017-09-20T15:04:47Z

/morph test

0xc0170 · 2017-09-21T07:55:47Z

@pan- The failure from the latest test does not seem to be related to this patch

[1505966920.84][CONN][INF] found KV pair in stream: {{__testcase_start;Testing parallel threads}}, queued...
[1505966920.88][CONN][RXD] Operator new[] out of memory

If you can confirm that we should restart CI, we will do it right away

pan- · 2017-09-21T17:47:37Z

The issue seen in CI is caused by the use of the the non throwing version of the new operator which is used in GenericGattClient.cpp.
Use of this operator includes portion of the exception support on GCC (not a joke!) and even if the code is not used - GenericGattClient is not used by the Nordic target - it is not removed at link time.

I submitted another PR to overcome that issue: #5165 .

0xc0170 · 2017-09-26T14:08:32Z

/morph test

mbed-bot · 2017-09-26T14:19:35Z

Result: ABORTED

Your command has finished executing! Here's what you wrote!

/morph test

…inationCallbackParams_t

Also fix wrong usage of designed initializer in CPP files.

This changes introduce a platform adaptation over ATT/GATT that can be implemented by porter. Unlike the GattClient interface, the ATT/GATT adaptation is simple, follow closely the Bluetooth specification and won't change over time. Implementation of the GattClient interface is realized by the class GenericGattClient which accept in input a pal::GattClient. This change will also free design space once adopted by partners, addition to the GattClient interface won't require partner support.

It allows mbed users to enable BLE on targets with an external BLE module.

pan- · 2017-09-26T14:21:55Z

@0xc0170 Apologies, I didn't rebase the PR. It is done now.

0xc0170 · 2017-09-26T14:22:43Z

/morph test

mbed-bot · 2017-09-26T20:38:08Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1397

All builds and test passed!

0xc0170 · 2017-09-27T12:28:39Z

@tommikas Please restart CI job

pan- changed the title ~~Cordio port~~ BLE: Cordio port Sep 8, 2017

theotherjimmy suggested changes Sep 8, 2017

View reviewed changes

theotherjimmy added the needs: work label Sep 8, 2017

pan- force-pushed the cordio_port branch from 497ef3c to 6c17609 Compare September 8, 2017 20:14

pan- force-pushed the cordio_port branch from 6c17609 to b904fac Compare September 11, 2017 19:07

theotherjimmy approved these changes Sep 14, 2017

View reviewed changes

adbridge added the release-version: 5.7.0-rc1 label Sep 20, 2017

0xc0170 added needs: CI and removed needs: work labels Sep 20, 2017

pan- mentioned this pull request Sep 25, 2017

Platform: Replace non throwing versions of new operator. #5165

Merged

pan- added 12 commits September 26, 2017 15:20

BLE: Fix GattClient destructor by adding a virtual specifier.

c58d7de

BLE: Add ArrayView in utilities.

dd303b7

BLE: use common type ble::connection_handle_t as Handle_t.

c022c4a

BLE: Fix header inclusion of DiscoveredCharacteristicDescriptor.

97995b8

BLE: Add optionnal error_code CharacteristicDescriptorDiscovery::Term…

4416ef6

…inationCallbackParams_t

BLE: Use shared type ble_attribute_handle_t in GattAttribute.

1fd953e

BLE NRF: Fix wrong usage of designed initializer in cpp code.

85e88cc

BLE: Add error code management in Gatt read and write data structures.

38bb6b4

Also fix wrong usage of designed initializer in CPP files.

BLE: Add collection of basic BLE types shared accross all layers.

512dd8c

BLE: Fix space

aa0b5d5

BLE: Add Cordio port.

75c9dfc

It allows mbed users to enable BLE on targets with an external BLE module.

pan- force-pushed the cordio_port branch from b904fac to 75c9dfc Compare September 26, 2017 14:21

0xc0170 added ready for merge and removed needs: CI labels Sep 27, 2017

theotherjimmy merged commit fb5241c into ARMmbed:master Sep 27, 2017

theotherjimmy removed the ready for merge label Sep 27, 2017

pan- mentioned this pull request Sep 28, 2017

BLE: feature gatt client - Generic GattClient implementation #4781

Closed

This was referenced Nov 17, 2017

BLE: Fix initialization of GattHVXCallbackParams in Nordic port. #5426

Merged

Improve ble docs #5392

Merged

pan- deleted the cordio_port branch July 3, 2018 11:04

BLE: Cordio port #5060

BLE: Cordio port #5060

Uh oh!

Conversation

pan- commented Sep 8, 2017

Description

Status

Migrations

Uh oh!

theotherjimmy left a comment

Choose a reason for hiding this comment

Uh oh!

theotherjimmy commented Sep 8, 2017

Uh oh!

pan- commented Sep 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theotherjimmy commented Sep 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theotherjimmy commented Sep 8, 2017

Uh oh!

theotherjimmy commented Sep 8, 2017

Uh oh!

pan- commented Sep 8, 2017

Uh oh!

theotherjimmy commented Sep 8, 2017

Uh oh!

theotherjimmy commented Sep 8, 2017

Uh oh!

theotherjimmy commented Sep 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pan- commented Sep 11, 2017

Uh oh!

theotherjimmy commented Sep 11, 2017

Uh oh!

theotherjimmy commented Sep 11, 2017

Uh oh!

pan- commented Sep 11, 2017

Uh oh!

theotherjimmy commented Sep 12, 2017

Uh oh!

sg- commented Sep 12, 2017

Uh oh!

theotherjimmy commented Sep 12, 2017

Uh oh!

sg- commented Sep 12, 2017

Uh oh!

theotherjimmy commented Sep 14, 2017

Uh oh!

0xc0170 commented Sep 20, 2017

Uh oh!

0xc0170 commented Sep 20, 2017

Uh oh!

0xc0170 commented Sep 20, 2017

Uh oh!

0xc0170 commented Sep 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pan- commented Sep 21, 2017

Uh oh!

0xc0170 commented Sep 26, 2017

Uh oh!

mbed-bot commented Sep 26, 2017

Result: ABORTED

Uh oh!

pan- commented Sep 26, 2017

Uh oh!

0xc0170 commented Sep 26, 2017

Uh oh!

mbed-bot commented Sep 26, 2017

Result: SUCCESS

Output

Uh oh!

0xc0170 commented Sep 27, 2017

Uh oh!

Uh oh!

pan- commented Sep 8, 2017 •

edited

Loading

theotherjimmy commented Sep 8, 2017 •

edited

Loading

theotherjimmy commented Sep 8, 2017 •

edited

Loading

0xc0170 commented Sep 21, 2017 •

edited

Loading