nrf5x: Enable asserts -> mbed_error

[andrewleech]

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]

User not whitelisted, CI not run.

[0xc0170]

@ARMmbed/team-nordic Please review

[0xc0170]

@marcuschangarm Please review

[marcuschangarm]

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

[andrewleech]

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

[marcuschangarm]

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.

[andrewleech]

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]

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

[marcuschangarm]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

@marcuschangarm Is this good to start going through CI?

[marcuschangarm]

Hit it!

[cmonr]

@andrewleech Fyi, this needs a rebase.

[andrewleech]

Ok, rebased

[cmonr]

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

[cmonr]

/morph build

[mbed-ci]

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

[mbed-ci]

Test : SUCCESS

Build number : 1997
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6022/1997

[mbed-ci]

Exporter Build : ABORTED

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

[adbridge]

/morph export-build

[adbridge]

/morph mbed2-build

[cmonr]

/morph uvisor-test

[mbed-ci]

Exporter Build : SUCCESS

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

[cmonr]

/morph uvisor-test

[cmonr]

/morph uvisor-test

[adbridge]

/morph uvisor-test

[adbridge]

Approving on behalf of maintainers