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

Fix memory reservation for Softdevice in NRF52_DK #7971

Merged
merged 1 commit into from Sep 7, 2018

Conversation

Projects
None yet
9 participants
@JammuKekkonen
Contributor

JammuKekkonen commented Sep 3, 2018

Description

The current system checks mbed app start trying to check whether softdevice is in use or not during linking. This doesn't work when bootloader is in use, so softdevice existence needs to be explicitly injected to linker scripts.

Pull request type

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

This comment has been minimized.

Contributor

JammuKekkonen commented Sep 3, 2018

@yogpan01

This comment has been minimized.

Contributor

yogpan01 commented Sep 3, 2018

@bulislaw can you please review, if this is good enough. We needed to free the remaining RAM for our usage when not using SOFT DEVICE but have our own bootloader as well.
@0xc0170 if possible we want this to go in mbed-os-5.10-rc1

@adbridge

This comment has been minimized.

Contributor

adbridge commented Sep 3, 2018

@yogpan01 Code freeze for RC1 was last Friday so unless this is a blocker it won't make it in now. If this is critical for 5.10 please discuss with @ChiefBureaucraticOfficer as to whether we consider this for RC2.

@0xc0170 0xc0170 requested review from theotherjimmy and TacoGrandeTX Sep 3, 2018

@yogpan01

This comment has been minimized.

Contributor

yogpan01 commented Sep 3, 2018

@ChiefBureaucraticOfficer This is blocker for us to support NRF52 on Client Lite. Can you please take this in for 5.10 release.

@ChiefBureaucraticOfficer

This comment has been minimized.

ChiefBureaucraticOfficer commented Sep 3, 2018

@yogpan01 - Do you have a timeline for the need? This appears to be a point fix, which can go in to fortnightly patch releases. Should it be extremely time sensitive we can pull it in for RC2. Please advise on criticality and downstream impacts.

#define MBED_RAM_SIZE 0x10000
#else
/* If softdevice is present, set aside space for it */
#if defined(SOFTDEVICE_PRESENT)

This comment has been minimized.

@0xc0170

0xc0170 Sep 3, 2018

Member

With this change, MBED_APP_START is being removed, thus what specifies that there is app moved (bootlader in) ? I would expect to have a condition to check MBED_APP_START and then softdevice might change the values or am I missing something?

This comment has been minimized.

@yogpan01

yogpan01 Sep 4, 2018

Contributor

In my understanding the original code made assumption that if your MBED_APP_START is 0 it implied that you are using SoftDevice, which is blocking the case where you don't have SoftDevice but have your own bootloader. This change makes the flow more logical.
If you have SoftDevice, we reserve RAM for you otherwise you get the complete RAM. There is no ambiguity in the interpretation now.

@yogpan01

This comment has been minimized.

Contributor

yogpan01 commented Sep 4, 2018

@ChiefBureaucraticOfficer We are fine if this can be taken into RC2 as well. Without this we cannot add support for NRF52 for Client Lite as this is one of the reference board we are planning to release in our next increment scheduled for next release, which is supposed to be ASAP.

@pan-

This comment has been minimized.

Member

pan- commented Sep 4, 2018

This doesn't work when bootloader is in use, so softdevice existence needs to be explicitly injected to linker scripts.

Could you provide more explanation ? What are the bootloader requirements ?

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 5, 2018

@yogpan01 Please answer @pan-'s question before we start CI.

@yogpan01

This comment has been minimized.

Contributor

yogpan01 commented Sep 6, 2018

@pan- We need to run cloud client on NRF52 board and we are not requiring Soft device. Cloud Client has a requirement that it comes with its own bootloader support so the app start address is not 0 but offset from bootloader region. With the current linker design of NRF52 layout its not possible to achieve this, since the assumption is that application without SoftDevice will always start from address 0, which is not the case, hence this change.

@0xc0170

0xc0170 approved these changes Sep 7, 2018

As long as bootloader and also soft device are all working

@pan-

pan- approved these changes Sep 7, 2018

Approved the NRF52 changes. Need validation for the tooling part.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 7, 2018

/morph build

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Sep 7, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Sep 7, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr cmonr merged commit 76827c3 into ARMmbed:master Sep 7, 2018

14 checks passed

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.0%)
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/cloud_client_smoke_test Test job: successful
Details
travis-ci/astyle Passed, 597 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10281 cycles (+208 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

@0xc0170 0xc0170 removed the needs: CI label Sep 7, 2018

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Oct 25, 2018

I don't approve of part of these tools changes. Please don't merge tools things without my approval.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Oct 25, 2018

This is especially bad because It's super target specific, uses MACROS and CAN'T BE REMOVED without breaking compatibility with 5.10.x!

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