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

nrf5x: Enable asserts -> mbed_error #6022

Merged
merged 5 commits into from Jun 4, 2018

Conversation

Projects
None yet
8 participants
@andrewleech
Contributor

andrewleech commented Feb 6, 2018

Description

The nordic sdk has a lot of asserts to protect against incorrect usage.

Currently these are disabled by default which allows serious issues to go undetected such as #6020

In this particular case an array internal to nordic sdk code was being written out of bounds m_cb.handlers[-1] = 0xFF, with the only tests to prevent this being as ASSERTS

This PR enables the nordic aserts by default for the nrf unified platform, with the error handing being passed through to the standard mbed error() handler (complete with file:line reference).

Unfortunately enabling nordic asserts does expose a (known?) typo in SDK 11 code which I've corrected in the second commit here.
I avoid touching the SDK code as a rule but there doesn't appear to be any way around this?
ref: https://devzone.nordicsemi.com/f/nordic-q-a/14000/nrf_drv_adc-c-doesn-t-compile-with--ddebug_nrf

Status

READY

Migrations

NO

@mbed-ci

This comment has been minimized.

mbed-ci commented Feb 6, 2018

User not whitelisted, CI not run.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 6, 2018

@ARMmbed/team-nordic Please review

@0xc0170 0xc0170 requested a review from pan- Feb 8, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 19, 2018

@marcuschangarm Please review

@0xc0170 0xc0170 requested a review from marcuschangarm Feb 19, 2018

@@ -3301,7 +3301,8 @@
"CMSIS_VECTAB_VIRTUAL",
"CMSIS_VECTAB_VIRTUAL_HEADER_FILE=\"cmsis_nvic.h\"",
"NO_SYSTICK",
"MBED_TICKLESS"
"MBED_TICKLESS",
"DEBUG_NRF_USER"

This comment has been minimized.

@marcuschangarm

marcuschangarm Feb 19, 2018

Contributor

ASSERT is usually controlled by the build profile. Won't this enable it permanently?

This comment has been minimized.

@andrewleech

andrewleech Feb 20, 2018

Contributor

This doesn't enable the system wide ASSERT, only the nordic sdk internal asserts.

Perhaps it would be better if we could define this only when NDEBUG is not defined, however I don't know a particularly good way to do that.

That being said, there are a number of these nrf asserts used to check the range of function input parameters such that without the assert it's frighteningly easy to create array overflow errors and corrupt your ram.
With this change these errors get redirected the the standard mbed_error handler which will then only printf the error when not in NDEBUG mode, before stopping.

This is dramatically safer then allowing the system to run after a buffer overflow as was happening to me before #6020

This comment has been minimized.

@marcuschangarm

marcuschangarm Feb 21, 2018

Contributor

I completely agree about the safety, but there is an expectation that when you build with the release profile that all kinds of ASSERTs are disabled and compiled out.

I would suggest either finding a place where DEBUG_NRF_USER can be controlled by NDEBUG or to keep is off by default, so that people have to opt-in by adding DEBUG_NRF_USER to their mbed_app.json or mbed_lib.json.

This comment has been minimized.

@andrewleech

andrewleech Feb 23, 2018

Contributor

You're right, I've had plenty of cases in the past where I'm counting bytes to fit into a chip, trimming out these checks in a release build is important.

However, having to know to manually opt-in in the config file by setting a define is basically where we are already... and it misses important bugs.

Unfortunately I can't find any header files included in the nordic code that originate outside of the sdk codebase to place this kind of check in, which leaves either further modifying sdk code or finding some other way to set a compiler global define programatically from NDEBUG not being set.

Alternatively, this define basically just enables a definition of #define ASSERT(expr) ...
I could just add a global define of #define ASSERT(expr) MBED_ASSERT(expr)
This is a pretty broad change then but likely to be desirable in most cases?

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 5, 2018

@marcuschangarm @pan- Any more thoughts? It sounds like @andrewleech need to update this PR, correct?

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Mar 5, 2018

As far as I understand, there are two issues here:

  1. Fix compile errors when DEBUG_NRF_USER is defined.
  2. Define DEBUG_NRF_USER permanently.

I think 1. going in is fine since people are asking for it.
2. should be left off by default and people should enable it themselves when they are debugging.

@andrewleech

This comment has been minimized.

Contributor

andrewleech commented Mar 5, 2018

Yes I agree with the two changes analysis.

Seeing as 1 is already modifying sdk code, would it be ok to fix 2 to be enabled with ! NDEBUG with another change to the relevant sdk header?

Or at the very least document the use of DEBUG_NRF_USER for all nrf platforms and find some way to enable it in unit tests here to stop new bugs getting into the repo.

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Mar 5, 2018

I thought you mentioned that there weren't any obvious places to have NDEBUG tied to DEBUG_NRF_USER or did I misunderstand that?

You may have noticed we are in the process of porting the NRF52 series to SDK 14.2: https://os.mbed.com/forum/news-announcements/topic/29477/

I don't know what's going to happen with the NRF51, but I would at least keep SDK 14.2 in mind when working on changes for the NRF51.

@andrewleech

This comment has been minimized.

Contributor

andrewleech commented Mar 5, 2018

I meant nowhere obvious that wasn't dependant on customising sdk code, I strongly avoid editing third party code where possible.

Yes I'd heard an sdk update was under way, though that can't help nrf51. It's pathetic that Nordic dropped support for the nrf51 in their sdk updates (even though they still call them nrf5 sdk). The nrf51 is still incredibly popular and will continue to be in commercial use as it's dramatically cheaper.

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Mar 5, 2018

Nordic dropped support for the nrf51 in their sdk updates (even though they still call them nrf5 sdk)

Wait - when did they drop NRF51 support? I was under the impression that SDK 14.2 would be compatible with NRF51 as well? If that is the case, is SDK 12.3 the last NRF51 SDK then?

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Mar 5, 2018

I meant nowhere obvious that wasn't dependant on customising sdk code, I strongly avoid editing third party code where possible.

I agree, but I don't think we can avoid it. I was planning on creating a parallel folder structure for all the SDK files I have to change from stock SDK 14.2.

@andrewleech

This comment has been minimized.

Contributor

andrewleech commented Mar 5, 2018

Yep: https://www.nordicsemi.com/eng/Products/Bluetooth-low-energy/nRF5-SDK
"nRF51 Series devices supported up to version v12"
But even then I think v12 might have dropped support for some revs of 51, in which case it's best to stick with 11.

@andrewleech

This comment has been minimized.

Contributor

andrewleech commented Mar 5, 2018

Ah nope could go to v12 for nrf51: http://infocenter.nordicsemi.com/index.jsp?topic=%2Fcom.nordic.infocenter.nrf52%2Fdita%2Fnrf52%2Fcompatibility_matrix%2Fic_rev_sdk_sd_comp_matrix.html

The older rev chips were dropped much earlier (sdk6/7), I guess you're just out of luck if you get one of them in a cheap dev board.

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Mar 5, 2018

That is so misleading:

New in nRF5 SDK v14.2

  • Support for the new S112 SoftDevice v5.1.0
  • S132 v5.1.0 is not included but will be drop in compatible. See Release Notes - S132 compatibility for details
  • See the release notes for more information

It includes a broad selection of drivers, libraries, examples for peripherals, SoftDevices, and proprietary radio protocols of the nRF52 Series and nRF51 Series.

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Mar 7, 2018

I like it. We are going to do some NRF51 clean up at some point, the question is how do we track changes to the SDK so we don't lose them? For the NRF52, I'll put all changed files in a new directory structure.

@donatieng what do you think?

@andrewleech

This comment has been minimized.

Contributor

andrewleech commented Mar 7, 2018

We'll all the changes are still in git, so you can just git logon the root of the sdk folder to see all the local changes (and why).

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 15, 2018

@marcuschangarm Is this good to start going through CI?

@cmonr cmonr added needs: review and removed needs: work labels Mar 15, 2018

@cmonr cmonr requested a review from donatieng Mar 15, 2018

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Mar 15, 2018

Hit it!

@cmonr cmonr added needs: CI and removed needs: review labels Mar 15, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 14, 2018

@andrewleech Fyi, this needs a rebase.

@andrewleech andrewleech force-pushed the andrewleech:nrf_asserts_error branch from 30657a6 to 7eb6d2e May 18, 2018

@andrewleech andrewleech force-pushed the andrewleech:nrf_asserts_error branch from 7eb6d2e to 67140a2 May 22, 2018

@andrewleech

This comment has been minimized.

Contributor

andrewleech commented May 22, 2018

Ok, rebased

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 22, 2018

@andrewleech Will start CI once some things on our backend have settled down. It's been a day.

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 31, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented May 31, 2018

Build : SUCCESS

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

Triggering tests

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

@cmonr cmonr added needs: CI and removed needs: review labels May 31, 2018

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@adbridge

This comment has been minimized.

Contributor

adbridge commented Jun 1, 2018

/morph export-build

@adbridge

This comment has been minimized.

Contributor

adbridge commented Jun 1, 2018

/morph mbed2-build

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 1, 2018

/morph uvisor-test

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 3, 2018

/morph uvisor-test

2 similar comments
@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 3, 2018

/morph uvisor-test

@adbridge

This comment has been minimized.

Contributor

adbridge commented Jun 4, 2018

/morph uvisor-test

@adbridge

Approving on behalf of maintainers

@adbridge adbridge merged commit 2d0e5f0 into ARMmbed:master Jun 4, 2018

13 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, 845 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10013 cycles (+399 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/littlefs Passed, code size is 9964B (+0.00%)
Details
travis-ci/tools Local tools testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment