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

Enable stats reporting with a flag in targets.json #9072

Merged
merged 1 commit into from
Dec 13, 2018

Conversation

theotherjimmy
Copy link
Contributor

@theotherjimmy theotherjimmy commented Dec 12, 2018

Description

PR #8607 will cause problems for the NRF52832 and the NRF52840
in the online compiler starting with 5.10.2. This PR prevents this problem
by using a toggle in targets.json to enable these new defines for every
target except for the NRF52832 and NRF52840.

Update: BLE + Softdevice + NRF528XX is currently broken in master, and this will work around it for the time being by not enabling the memory-reporting for the NRF528XX targets.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

### Description

PR ARMmbed#8607 will cause problems for the NRF52832 and the NRF52840
in the online compiler starting with 5.10.2. This PR prevents this problem
by using a toggle in `targets.json` to enable these new defines for every
target except for the NRF52832 and NRF52840.

### Pull request type

    [x] Fix
    [ ] Refactor
    [ ] Target update
    [ ] Functionality change
    [ ] Docs update
    [ ] Test update
    [ ] Breaking change
@0xc0170 0xc0170 requested a review from a team December 12, 2018 14:49
@0xc0170 0xc0170 requested a review from a team December 12, 2018 14:49
Copy link
Contributor

@bridadan bridadan 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!

@adbridge
Copy link
Contributor

@ARMmbed/mbed-os-pan you should be aware of this

@donatieng
Copy link
Contributor

Could we get details on build failures - could not reproduce?

@theotherjimmy
Copy link
Contributor Author

@donatieng No build failures occur.

@deepikabhavnani
Copy link

PR #8607 will cause problems for the NRF52832 and the NRF52840 in the online compiler starting with 5.10.2.

PR #8607 was part of 5.11.0-rc1, how come it resulted in errors since 5.10.2?

@theotherjimmy
Copy link
Contributor Author

@deepikabhavnani It has not caused problems yet, which is why that sentence is future tense. Also, you're assuming that the online compiler somehow can use the build tools in the same way as Mbed CLI. It does not, and only uses a single copy of the tools that is near latest.

@deepikabhavnani
Copy link

deepikabhavnani commented Dec 12, 2018

It does not, and only uses a single copy of the tools that is near latest.

Wow, didn't knew that tools are latest some configured version (not necessary latest), irrespective of mbed-os version

@cmonr
Copy link
Contributor

cmonr commented Dec 12, 2018

So just to make sure I understand, this problem:

  • Only occurs when using Soft Devices
  • Compiles fine
  • But eventually causes runtime failures by attempting to allocate/use memory past the end of what the devices has?

@cmonr
Copy link
Contributor

cmonr commented Dec 12, 2018

CI started

@theotherjimmy
Copy link
Contributor Author

But eventually causes runtime failures by attempting to allocate/use memory past the end of what the devices has?

Nope. At no point does anything overflow RAM or allocate outside of RAM. The problem is that the Softdevice and Application allocate the same RAM when the Softdevice is used.

@mbed-ci
Copy link

mbed-ci commented Dec 13, 2018

Test run: SUCCESS

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

Copy link
Member

@pan- pan- left a comment

Choose a reason for hiding this comment

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

Thanks for the PR Jimmy. Do you plan to add a bit of documentation about this feature for partners that may encounter the same situation in the future ?

@adbridge adbridge merged commit a91dccd into ARMmbed:master Dec 13, 2018
@theotherjimmy
Copy link
Contributor Author

@pan- That's a good idea. I'll add it to the adding and configuring targets section.

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

9 participants