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: Cordio port #5060

Merged
merged 12 commits into from Sep 27, 2017

Conversation

Projects
None yet
6 participants
@pan-
Member

pan- commented Sep 8, 2017

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

@pan- pan- changed the title from Cordio port to BLE: Cordio port Sep 8, 2017

@theotherjimmy
  1. Why TARGET_CORDIO?
  2. Why TARGET_CORTEX?
  3. Why not TARGTE_CORTEX_{M0,M0PLUS,M3,M4}
  4. 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

This comment has been minimized.

Contributor

theotherjimmy commented Sep 8, 2017

Also, what does an example look like?

@pan- pan- force-pushed the pan-:cordio_port branch Sep 8, 2017

@pan-

This comment has been minimized.

Member

pan- commented Sep 8, 2017

@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

This comment has been minimized.

Contributor

theotherjimmy commented Sep 8, 2017

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

This comment has been minimized.

Contributor

theotherjimmy commented Sep 8, 2017

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

👍

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 8, 2017

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-

This comment has been minimized.

Member

pan- commented Sep 8, 2017

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

This comment has been minimized.

Contributor

theotherjimmy commented Sep 8, 2017

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

Well, that's simply not true.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 8, 2017

"technical limitations" at the time.

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

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 8, 2017

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-

This comment has been minimized.

Member

pan- commented Sep 11, 2017

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

This comment has been minimized.

Contributor

theotherjimmy commented Sep 11, 2017

@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

This comment has been minimized.

Contributor

theotherjimmy commented Sep 11, 2017

@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- pan- force-pushed the pan-:cordio_port branch Sep 11, 2017

@pan-

This comment has been minimized.

Member

pan- commented Sep 11, 2017

@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

This comment has been minimized.

Contributor

theotherjimmy commented Sep 12, 2017

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-

This comment has been minimized.

Member

sg- commented Sep 12, 2017

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

This comment has been minimized.

Contributor

theotherjimmy commented Sep 12, 2017

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-

This comment has been minimized.

Member

sg- commented Sep 12, 2017

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

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 14, 2017

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 20, 2017

As my understanding, to fix jenkins CI, we need ARMmbed/mbed-os-cliapp#307, should be in soon

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 20, 2017

The fix is in, restarted CI

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 20, 2017

/morph test

@pan-

This comment has been minimized.

Member

pan- commented Sep 21, 2017

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

This comment has been minimized.

Member

0xc0170 commented Sep 26, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 26, 2017

Result: ABORTED

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

/morph test

pan- added some commits Sep 8, 2017

BLE: Add error code management in Gatt read and write data structures.
Also fix wrong usage of designed initializer in CPP files.
BLE: Introduce GenericGattClient and platform abstraction over ATT/GATT.
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.
BLE: Add Cordio port.
It allows mbed users to enable BLE on targets with an external BLE module.

@pan- pan- force-pushed the pan-:cordio_port branch to 75c9dfc Sep 26, 2017

@pan-

This comment has been minimized.

Member

pan- commented Sep 26, 2017

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 26, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 26, 2017

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

This comment has been minimized.

Member

0xc0170 commented Sep 27, 2017

@tommikas Please restart CI job

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

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

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pan- pan- deleted the pan-:cordio_port branch Jul 3, 2018

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