Skip to content

nRF5x: pass ram linker start/length from config system #7957

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

Merged
merged 2 commits into from
Oct 1, 2018

Conversation

andrewleech
Copy link
Contributor

Description

The ram start and length settings can already be defined in targets.json with mbed_ram_start and mbed_ram_size etc.

These are supposed to be used in nrf5x targets to specify in the linker how much ram is set aside for the softdevice. Unfortunately the linker files have these values defined in directly so they can't be overridden by the target settings.
This PR fixes the problem by simply checking if the defines are already set before setting them in the linker scripts.

There was also a minor bug in the python config handler where these boot-loader target settings in were being overridden by a later part of the config system with None. This is also fixed.

Pull request type

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

@theotherjimmy
Copy link
Contributor

Thanks for fixing that config system bug. Nice diff!

Copy link
Contributor

@theotherjimmy theotherjimmy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for tools changes.

Copy link
Contributor

@TacoGrandeTX TacoGrandeTX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@TacoGrandeTX
Copy link
Contributor

@andrewleech In light of #7959 could you recommend how this should be merged?

@andrewleech
Copy link
Contributor Author

I've using both of these MR's together in my working branch, they're quite complimentary.

This MR allows the project to set where the start address should be, the other one ensures that the correct start address is passed to the softdevice.

I did consider making the other one work from this config system define rather than the linker element, but decided to leave it as the linker symbol as it's a smaller change to the existing nordic code and it should be safer, the symbol used is just in nordic linker scripts so is less likely to be accidentally changed in a breaking way than the config system symbol which is used for all targets.

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 18, 2018

This needs a rebase, will be scheduled for CI afterwards

Values like `target.mbed_rom_start` were being replaced by None and then ignored.
@andrewleech andrewleech force-pushed the nrf_override_ram_linker branch from 0288af6 to f553f26 Compare September 19, 2018 05:43
@andrewleech andrewleech force-pushed the nrf_override_ram_linker branch from f553f26 to a0d7d4e Compare September 19, 2018 06:14
@andrewleech
Copy link
Contributor Author

@0xc0170 I've rebased, should be good to go.

The travis failure above looks pretty unrelated:

The command "W: Failed to fetch http://www.apache.org/dist/cassandra/debian/dists/39x/InRelease Could not connect to www.apache.org:80 (95.216.24.32), connection timed out [IP: 95.216.24.32 80]" failed. Retrying, 2 of 3.

@cmonr
Copy link
Contributor

cmonr commented Sep 27, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Sep 27, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci
Copy link

mbed-ci commented Sep 27, 2018

@mbed-ci
Copy link

mbed-ci commented Sep 27, 2018

@cmonr
Copy link
Contributor

cmonr commented Sep 27, 2018

/morph mbed2-build

@0xc0170 0xc0170 merged commit 0ded1fa into ARMmbed:master Oct 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants