nrf5x: ram start address fix

[andrewleech]

Description

In all nordic targets the start of application ram is needed by the softdevice to know where the end of its ram space is.

This is currently determined by checking the location of __data_start__ (for gcc, different name on other compilers) which is traditionally at the start of the volatile ram block.

In mbed however there is the extra small nvic block before this.
As it stands, if the softdevice uses all it's declared available ram it will overwrite this nvic block, breaking the application.

With this PR the start of ram is now pointing to the mbeds nvic section.

This does require editing the one Nordic SDK file which controls this.
This modification is noted in the associated readme.

Pull request type

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

[0xc0170]

As it stands, if the softdevice uses all it's declared available ram it will overwrite this nvic block, breaking the application.

@ARMmbed/mbed-os-pan Have you experienced this?

[andrewleech]

The failure only occurs if you're using up all the softdevice ram, eg by allocating quite a large number of characteristics (~15 - 20). It probably hasn't occurred in any of the BLE examples as they only have a few chars.

[TacoGrandeTX]

@andrewleech This looks correct, but could you supply a test which demonstrates the issue? Do you know of a good way of determining the SoftDevice RAM requirements for a given configuration or set of characteristics?

[andrewleech]

The issue of knowing how much ram is needed for the softdevice was discussed round and round in #5980 (review)

Basically, no, there's not good way to know or calculate the ram without help from Nordic, who've repeatedly said no they will not disclose this, saying just use trial and error.

Basically when you initialise the softdevice wiith a known set of configs it says how much ram it needs for them (num central, gatt size, etc) and whether you've given it enough. Then you just start initialising advert params and characteristics and check if each one succeeds, if you run out of attr_tab_size one of these will fail and you add a chunk more to it and try again.

I not too sure how to add a test for this either. I guess with the existing files, you would need an example project with fixed set of characteristics, then reduce the attr_tab_size until the initialisation fails, then increase just enough to pass. Then you probably need to exercise the characteristic a bit to trigger the memory overlap issue, maybe writing to it would be enough. By memory I eventually got a hard fault in the failing case.
I also don't have any more time/budget left on the project using this, perhaps a week ago I could have tried this, not so much now.

I found and fixed this more by inspection; the softdevice init code takes the start-of-ram value to know how much it has to use, and that start-of-ram value was pointing to after the mbed vector block.

[TacoGrandeTX]

@andrewleech Understood. I would like to run at least some local tests with this. I'll report back soon.

[TacoGrandeTX]

This doesn't seem to work as our BLE_Button() example doesn't properly execute. I put a debugger on the Arm compiler build and see nrf_sdh_ble_enable() returning an error code of 4. The LED does blink though from blinkCallback. On a GCC build the LED doesn't blink and there is no Button device to connect to in the nRF Connect app. Please investigate.

[andrewleech]

I've only got access to gcc compiler and that works fine for me on my nrf52810 (with same changes as the nRF52832 here). I don't have any other nrf52 boards to test on at the moment (pulled onto other projects).

The changes for the other two compilers looks trivially simple though, to the best of my understanding it should be a pretty safe change.

Did you try a compilation before this commit was applied to confirm it's still working there before adding my change?

ps. the commit above is just a rebase.

[TacoGrandeTX]

I've taken your rebase and see the same failure. If I revert the change then the Button example works okay.

As I debug this again I see the same issue - nrf_sdh_ble_enable() is returning 4 so btle_init() returns an error. I should have noted earlier that we see the underlying sd_ble_enable() is implemented with a SVC. This means the SoftDevice is handling the processing of sd_ble_enable(). As nrf_sdh_ble.c is a Nordic-supplied file they might be accounting for the presence of the vector table and expect m_ram_start (APP_RAM_START) to be located after it.

So this definitely breaks the nRF52_DK and nRF52840_DK builds as a byproduct of the v14.2 SDK.

What SoftDevice are you using for the nRF52810?

In hindsight we probably shouldn't be modifying the SDK files unless we are fixing an obvious bug or deficiency.

[andrewleech]

Yeah ok, that's quite strange, not sure what I've got wrong.
The nrf_sdh_ble_enable function returns 4 when it's detecting not enough ram for the settings provided, so the ram_start being passed to it is clearly not correct.

I'm testing with S112 as configured in my nRF52810 PR which is still waiting for a review... but it's modeled on the S132 / nRF52832 setup and is the same other than lengths of ram/flash.

I agree that modifying sdk files should be avoided if at all possible, however this file is referencing labels defined in the linker script which mbed has changed compared to the sdk defaults it expects.

The sdk linker scripts do not have a separate section for the vectors, mbed has added this themselves before the standard data_start block. hence nordic doesn't normally have to account for the vectors as they usually come after data_start, everything before this is normally given to the softdevice.

[TacoGrandeTX]

I was not aware of #7100 but have subscribed to it.

[cmonr]

@andrewleech @TacoGrandeTX What are the next steps here?

[andrewleech]

Not sure, it worked perfectly for me previously but I don't have hardware to test on any more.

[cmonr]

@TacoGrandeTX Would you or someone else from @ARMmbed/device-os-custom-engineering-and-support (or @ARMmbed/mbed-os-pan) be able to take this over? Otherwise, I think we'll need to close this since it sounds like @andrewleech is unable to work on this further.

[0xc0170]

We are closing , can be reopen anytime