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

Cleanup TARGET_NRF5 and TARGET_NRF5x #6711

Merged
merged 2 commits into from May 12, 2018

Conversation

Projects
None yet
7 participants
@marcuschangarm
Contributor

marcuschangarm commented Apr 23, 2018

Description

The unified NRF51 target and feature BLE directories have been
reorganized to follow the naming and directory structure of the
NRF52 implementation.

This reorganization does not include TARGET_MCU_NRF51822 and
derived targets.

Unused NRF52 files have been removed.

Pull request type

[ ] Fix
[x] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change
@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Apr 23, 2018

@0xc0170
I didn't touch targets/TARGET_NORDIC/TARGET_MCU_NRF51822/ since the structure is incompatible with TARGET_MCU_NRF51822_UNIFIED.

@donatieng
I had to split out the NRF51 and NRF52 BLE implementation into separate directories since they use different storage implementations.

@bulislaw @mprse
I had to split out the clocks into separate directories in targets/TARGET_NORDIC/TARGET_NRF5x/ in order to make the NRF52 work. However, I expect we can merge them again when #5629 has been rebased?

@0xc0170 0xc0170 requested review from bulislaw and donatieng Apr 24, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 26, 2018

I had to split out the clocks into separate directories in targets/TARGET_NORDIC/TARGET_NRF5x/ in order to make the NRF52 work. However, I expect we can merge them again when #5629 has been rebased?

Which commit is this one, or reference on your branch to review?
I reviewed changes, looks fine to me.

Please rebase to resolve conflicts.

@0xc0170 0xc0170 added needs: work and removed needs: review labels Apr 26, 2018

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Apr 26, 2018

Which commit is this one, or reference on your branch to review?

bd8dd20

Both TARGET_NRF51 and TARGET_NRF52 contain ticker implementations. With #5629 we can combine the two into the same implementation.

@marcuschangarm marcuschangarm force-pushed the marcuschangarm:cleanup-nrf5x branch from bd8dd20 to 1e20aeb Apr 29, 2018

@adbridge

This comment has been minimized.

Contributor

adbridge commented Apr 30, 2018

@bulislaw , @donatieng We are still awaiting your reviews on this PR, could you please do so asap.

@pan-

This comment has been minimized.

Member

pan- commented Apr 30, 2018

I had to split out the NRF51 and NRF52 BLE implementation into separate directories since they use different storage implementations.

Could you detail this point ? Is it related to the peer manager used ?

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Apr 30, 2018

@pan-

There is a big mismatch between whether the header files should have nrf_ prefixed or not, but the main issue is the new softdevice header:

https://github.com/marcuschangarm/mbed-os/blob/cleanup-nrf5x/features/FEATURE_BLE/targets/TARGET_NORDIC/TARGET_NRF51/source/btle/btle.cpp#L49-L50

https://github.com/marcuschangarm/mbed-os/blob/cleanup-nrf5x/features/FEATURE_BLE/targets/TARGET_NORDIC/TARGET_NRF52/source/btle/btle.cpp#L59-L60

I'm also not familiar enough with the NRF51 to make too many changes in one go.

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented May 2, 2018

@donatieng @bulislaw

Can you have a look please?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 3, 2018

It looks fine to me (SDK11 to SDK_11 change follows the rest of the naming ?)

There are conflicts to be resolved

@marcuschangarm marcuschangarm force-pushed the marcuschangarm:cleanup-nrf5x branch from 1e20aeb to a32394d May 3, 2018

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented May 3, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented May 3, 2018

Build : SUCCESS

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

Triggering tests

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

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented May 3, 2018

It looks fine to me (SDK11 to SDK_11 change follows the rest of the naming ?)

Yes, that was from external input.

There are conflicts to be resolved

Done.

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 7, 2018

One test fails for all toolchains, related to the changes?

We have a conflict

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 7, 2018

This also needs a rebase.

Remove unused NRF52 SDK and MCU files
Superseded by new SDK 14.2 in #6547

@marcuschangarm marcuschangarm force-pushed the marcuschangarm:cleanup-nrf5x branch from a32394d to 655721a May 7, 2018

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented May 7, 2018

/morph build

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented May 7, 2018

One test fails for all toolchains, related to the changes?

I only moved the file to a different folder, I didn't change it.

@mbed-ci

This comment has been minimized.

mbed-ci commented May 7, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented May 8, 2018

Good catch! Thank you!

@marcuschangarm marcuschangarm force-pushed the marcuschangarm:cleanup-nrf5x branch from 655721a to 0d0eaae May 8, 2018

Reorganize TARGET_MCU_NRF51822_UNIFIED directories
The unified NRF51 target and feature BLE directories have been
reorganized to follow the naming and directory structure of the
NRF52 implementation.

This reorganization does not include TARGET_MCU_NRF51822 and
derived targets.

@marcuschangarm marcuschangarm force-pushed the marcuschangarm:cleanup-nrf5x branch from 0d0eaae to 1aebdcb May 8, 2018

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented May 8, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented May 8, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented May 8, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented May 9, 2018

/morph export-build

@mbed-ci

This comment has been minimized.

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented May 9, 2018

/morph export-build

@mbed-ci

This comment has been minimized.

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented May 10, 2018

/morph export-build

@mbed-ci

This comment has been minimized.

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented May 10, 2018

@theotherjimmy Fourth time is indeed the charm! 😄

@cmonr cmonr added needs: review and removed needs: work labels May 11, 2018

@cmonr

cmonr approved these changes May 11, 2018

That was much less painful than I was expecting.

👍 on using git mv commands.

@cmonr cmonr added ready for merge and removed needs: review labels May 11, 2018

@cmonr cmonr merged commit 2104d8a into ARMmbed:master May 12, 2018

13 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
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/astyle Passed, 845 warnings (+0 warnings)
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9979 cycles (+437 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/littlefs Passed, code size is 9964B (+0.00%)
Details
travis-ci/tools Local tools testing has passed
Details

@cmonr cmonr removed the ready for merge label May 21, 2018

marcuschangarm added a commit to marcuschangarm/mbed-os that referenced this pull request May 25, 2018

@marcuschangarm marcuschangarm deleted the marcuschangarm:cleanup-nrf5x branch May 25, 2018

adbridge added a commit that referenced this pull request Jun 15, 2018

Fix array overflow in gpio configuration code for NRF5x
Reintroduce PR #6021

#6021

which was accidentally removed by PR #6711

#6711

adbridge added a commit that referenced this pull request Jun 15, 2018

Fix array overflow in gpio configuration code for NRF5x
Reintroduce PR #6021

#6021

which was accidentally removed by PR #6711

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