Skip to content
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

nRF: use Mbed-default boot-stack-size & fix stack_size_unification test #12874

Merged
merged 1 commit into from
Apr 28, 2020

Conversation

LDong-Arm
Copy link
Contributor

@LDong-Arm LDong-Arm commented Apr 27, 2020

Summary of changes

The global default stack sizes for Mbed OS are

Previously, we overrode nRF targets to have a larger stack (0x800) for RTOS due to memory required by the (SDK-side) SoftDevice BLE stack. Having removed SoftDevice in favour of Cordio (PR #12674), such requirement does not apply anymore.

As for stack_size_unification test, it failed in a recent nightly CI after we made it available for bare-metal (PR #12827). Removing the NRF5x macro (for the reason above) fixes the test.

Impact of changes

Migration actions required

Documentation

None.


Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[x] Tests / results supplied as part of this PR

Manual tests

  • stack_size_unification, with & without bare-metal on NRF52840_DK - This will also be run in CI
  • BLE CLI app - This heavyweight app with Cordio BLE + RTOS runs without any issues, and its map file shows correct stack size.

Reviewers

@jamesbeyond @evedon @kjbracey-arm @mprse @MarceloSalazar


@@ -79,12 +79,6 @@
"MCU_NRF51": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I also remove NRF51 since they're deprecated now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Check with @MarceloSalazar

Copy link
Contributor Author

@LDong-Arm LDong-Arm Apr 27, 2020

Choose a reason for hiding this comment

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

Oh actually it's fine to remove it, because the option is more related to SoftDevice (which we removed) rather than NRF51 or 52. We use target names because the config cannot check SoftDevice directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@ciarmcom ciarmcom requested review from evedon, jamesbeyond, kjbracey, mprse and a team April 27, 2020 15:00
@ciarmcom
Copy link
Member

@LDong-Arm, thank you for your changes.
@evedon @kjbracey-arm @jamesbeyond @mprse @ARMmbed/mbed-os-test @ARMmbed/mbed-os-hal @ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

evedon
evedon previously approved these changes Apr 27, 2020
@@ -79,12 +79,6 @@
"MCU_NRF51": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Check with @MarceloSalazar

@mergify mergify bot added needs: CI and removed needs: review labels Apr 27, 2020
…used

Previously we overrode nRF targets to have a larger stack
due to memory required by SoftDevice. Having deprecated SoftDevice
in favour of Cordio for BLE (ARMmbed#12674), such requirement does not
apply anymore.
@LDong-Arm
Copy link
Contributor Author

@evedon Thanks for the review

@mergify mergify bot dismissed evedon’s stale review April 27, 2020 16:24

Pull request has been modified.

@LDong-Arm LDong-Arm changed the title nRF52*: use Mbed-default boot-stack-size & fix stack_size_unification test nRF: use Mbed-default boot-stack-size & fix stack_size_unification test Apr 27, 2020
@LDong-Arm LDong-Arm requested a review from evedon April 27, 2020 16:37
@0xc0170 0xc0170 removed the request for review from a team April 28, 2020 12:21
Copy link

@MarceloSalazar MarceloSalazar 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 and I like that we're removing TARGET dependencies on code.
We'll review support for NRF51 in the upcoming days anyway.

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 28, 2020

CI started

@mbed-ci
Copy link

mbed-ci commented Apr 28, 2020

Test run: SUCCESS

Summary: 6 of 6 test jobs passed
Build number : 1
Build artifacts

@0xc0170 0xc0170 merged commit 97a380f into ARMmbed:master Apr 28, 2020
@mergify mergify bot removed the ready for merge label Apr 28, 2020
@mergify mergify bot added the release version missing When PR does not contain release version, bot should label it and we fix it afterwards label Apr 28, 2020
@mergify
Copy link

mergify bot commented Apr 28, 2020

This PR does not contain release version label after merging.

@0xc0170 0xc0170 added release-version: 6.0.0-beta-1 and removed release version missing When PR does not contain release version, bot should label it and we fix it afterwards labels Apr 29, 2020
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.

None yet

6 participants