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 going open source #7869

Merged
merged 3 commits into from Aug 28, 2018

Conversation

@paul-szczepanek-arm
Member

paul-szczepanek-arm commented Aug 23, 2018

Description

Cordio is now open source so we are replacing compiled libraries with source code. Docs updated to match.

Needs: #7417 (merged)

Pull request type

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

@paul-szczepanek-arm paul-szczepanek-arm changed the title from Cordio sources to BLE: Cordio going open source Aug 23, 2018

@paul-szczepanek-arm

This comment has been minimized.

Member

paul-szczepanek-arm commented Aug 23, 2018

Not sure how to class this PR. There is no functionality change - it just compiles from source rather than linked against libs.

@paul-szczepanek-arm paul-szczepanek-arm requested review from pan- and donatieng Aug 23, 2018

@donatieng donatieng requested review from ARMmbed/mbed-os-pan, pavanmr94, tomtrotta, spavezaArm and ChiefBureaucraticOfficer Aug 23, 2018

@donatieng

This comment has been minimized.

Member

donatieng commented Aug 23, 2018

Adding @pavanmr94 @tomtrotta @spavezaArm from the Cordio team as reviewers and @ChiefBureaucraticOfficer for license checks.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 23, 2018

Not sure how to class this PR. There is no functionality change - it just compiles from source rather than linked against libs.

Refactor looks OK.

@0xc0170

Just one question, looks fine to me as it is

@@ -1,21 +1,21 @@
# Porting guide
# Porting guide

This comment has been minimized.

@0xc0170

0xc0170 Aug 23, 2018

Member

While we are doing this update. Is this porting guide somewhere else (is this a duplicate) ? We have docs portal where I would expect this or?

cc @AnotherButler

This comment has been minimized.

@0xc0170

0xc0170 Aug 23, 2018

Member

This is updating this file : #7417 . I assume it should go in first before this Cordio? Or which way to integrate these 2

This comment has been minimized.

@pan-

pan- Aug 23, 2018

Member

We had this question internally, it should gets in after #7417 . The documentation must be updated too.

This comment has been minimized.

@paul-szczepanek-arm

paul-szczepanek-arm Aug 23, 2018

Member

Yes, this was done earlier this month before the copy edits in the other PRs. Will resolve conflicts when that one lands

This comment has been minimized.

@0xc0170

0xc0170 Aug 24, 2018

Member

Documentation update was integrated, this shall be rebased

@pavanmr94

This comment has been minimized.

pavanmr94 commented Aug 23, 2018

Hi,
Is this from this repository? https://github.com/ARMmbed/CordioBLE-Host-Stable
Or some packaged bundle delivered earlier?

@spavezaArm

This comment has been minimized.

spavezaArm commented Aug 23, 2018

@jyi858 may want to be a part of this review if source code is going public.

@paul-szczepanek-arm

This comment has been minimized.

Member

paul-szczepanek-arm commented Aug 23, 2018

This was done earlier this month before we knew when the Cordio team would do their submission but they gave us the go ahead to push this into mbed-os.

@donatieng donatieng force-pushed the paul-szczepanek-arm:cordio-sources branch from 233ba46 to 657fc45 Aug 24, 2018

@donatieng

This comment has been minimized.

Member

donatieng commented Aug 24, 2018

Rebased against master and resolved conflicts

@donatieng

This one should be ready to roll!

@0xc0170 0xc0170 added needs: review and removed needs: work labels Aug 24, 2018

@ChiefBureaucraticOfficer

This comment has been minimized.

ChiefBureaucraticOfficer commented Aug 24, 2018

Confirmed license OK.

@AnotherButler

This comment has been minimized.

Contributor

AnotherButler commented Aug 24, 2018

Do we have any related handbook documentation?

@donatieng

This comment has been minimized.

Member

donatieng commented Aug 24, 2018

@AnotherButler The porting guide was updated, however we're not expect new handbook content for this PR as it's mostly targeting partners as opposed to end-users

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 27, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Aug 27, 2018

Build : SUCCESS

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

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@pan-

pan- approved these changes Aug 27, 2018

@pan- pan- referenced this pull request Aug 27, 2018

Merged

Bluetooth 5 Phy support #7899

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 28, 2018

The only remaining thing here is travis. It tries to clone the branch from mbed-os repository not the one that this PR came from (not certain what has happened, don't see anything here in the logs that someone has changed the source branch). I pushed this to the travis requested one (temporary branch for tests to be run). Seems to be progressing now

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Aug 28, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 28, 2018

@jyi858 / @pavanmr94 / @spavezaArm This is now ready for merge. We will integrate this today, if there are any comments please let us know asap

@0xc0170 0xc0170 added risk: G and removed risk: A labels Aug 28, 2018

@cmonr cmonr merged commit 4089d4e into ARMmbed:master Aug 28, 2018

17 checks passed

AWS-CI uVisor Build & Test Success
Details
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.0%) RAM(+0.1%)
Details
continuous-integration/jenkins/branch This commit looks good
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
jenkins-ci/cloud_client_smoke_test Test job was successful
Details
travis-ci/astyle Passed, 587 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10319 cycles (+1180 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

@cmonr cmonr removed the risk: G label Sep 2, 2018

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