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

NRF52: add a separate .nvictable section and allow .noinit to be used… #7145

Merged
merged 2 commits into from Jun 22, 2018

Conversation

Projects
None yet
6 participants
@drahnr
Contributor

drahnr commented Jun 6, 2018

Description

Creates a separate section for the NVIC relocated vector table.
Yields the common .noinit for application use cases.

Pull request type

[X] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[X] Breaking change

This could be potentially breaking if an app does some dynamic vector table modifcation or similiar.

@pan-

@drahnr Thanks for your submission.

Could you comment on why that would be a breaking change ? To me it seems that position and the symbol name of the vector tables remain untouched; the only change is the name of the section holding the vector table.

IAR and ARMCC linker scripts needs to be updated as well.

As a side note it would be appreciated if you backport these changes to NRF51 too.

@0xc0170 0xc0170 added the needs: work label Jun 6, 2018

@0xc0170 0xc0170 requested a review from marcuschangarm Jun 6, 2018

@drahnr

This comment has been minimized.

Contributor

drahnr commented Jun 6, 2018

The breaking change might be applications which exploit the defined symbols based on __start_noinit which is not the NVIC table anymore, but an indepentend section.

I will look into the other linker files, but I can't promise much there since I don't have any setups for IAR nor ARMCC nor any licenses for either nor experience writing linke files for them.

@drahnr

This comment has been minimized.

Contributor

drahnr commented Jun 6, 2018

Also NRF51 does not have a dedicated section for the relocated table yet, so I am not sure this makes sense to tackle, especially since I can't even test any of those.

@cmonr cmonr added the needs: review label Jun 11, 2018

Latest commit appears to be addressing comments

@cmonr cmonr removed the needs: work label Jun 12, 2018

@cmonr cmonr requested a review from donatieng Jun 18, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 18, 2018

@pan- @marcuschangarm Mind taking another look?

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Jun 19, 2018

The NRF52 looks good, thank you! I don't have enough experience with the NRF51 to review it.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 20, 2018

/morph build

@drahnr

This comment has been minimized.

Contributor

drahnr commented Jun 20, 2018

@marcuschangarm there are no changes for NRF51 in this PR since I don't do any development with that platform nor are the changes directly applicable to the NRF51 without some additional preliminay work

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Jun 20, 2018

@drahnr

This comment has been minimized.

Contributor

drahnr commented Jun 20, 2018

@marcuschangarm I totally messed that up, I am removing those accidentally commited changes and things should be fine.

@mbed-ci

This comment has been minimized.

mbed-ci commented Jun 20, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 20, 2018

/morph mbed2-build

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 20, 2018

Re-enqueueing since it looks like we hit a device allocation issue.

/morph test

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Jun 20, 2018

@cmonr I think we are still waiting for changes.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 20, 2018

@marcuschangarm Ah, crud. Yeah, looks like it.
Just saw the failure come up, not the preceeding comments.

@cmonr cmonr added needs: work and removed needs: review labels Jun 20, 2018

@mbed-ci

This comment has been minimized.

@drahnr drahnr force-pushed the drahnr:master branch from 367d95e to 124e15f Jun 21, 2018

@drahnr

This comment has been minimized.

Contributor

drahnr commented Jun 21, 2018

Removed NRF51 related changes from the last commit.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 21, 2018

/morph build

@cmonr

cmonr approved these changes Jun 21, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Jun 21, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 21, 2018

/morph mbed2-build

@mbed-ci

This comment has been minimized.

@cmonr cmonr added ready for merge and removed needs: CI labels Jun 21, 2018

@cmonr cmonr merged commit 446de69 into ARMmbed:master Jun 22, 2018

14 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, 919 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9633 cycles (+698 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 9964B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment