Update Nordic NRF52 based targets to SDK 14.2 #6547

marcuschangarm · 2018-04-05T15:06:13Z

Description

SDK updated to version 14.2 for all NRF52 based targets.

See https://os.mbed.com/forum/upcoming-features/topic/29477/ for a full list of changes.

See README.md for documentation of new features.

Note: this PR is step 1 in updating the NRF52 based boards. Next step is to remove old NRF52 code and SDK. Third step is to reorganize the NRF51 directory structure to fall in line with the new one.

Pull request type

[X] Fix
[ ] Refactor
[ ] New target
[X] Feature
[X] Breaking change

hanno-becker · 2018-04-05T15:34:27Z

targets/TARGET_NORDIC/TARGET_NRF5x/trng_api.c

+    hal_critical_section_exit();
+
+    /* Deinitialize TRNG when all objects have been freed. */
+    if (counter == 0) {


This has a race: Say thread A calls trng_free and decreases nordig_trng_counter to 0, but gets interrupted before the branch condition counter == 0 is checked and nrf_drv_rng_uninit() is called, and say at that point another thread B calls trng_init, increasing nordig_trng_counter to 1 and calling nrf_drv_rng_init. Then, when thread A gets rescheduled, it calls nrf_drv_rng_uninit, leaving B's TRNG context unusable.

Good point! Extending hal_critical_section_exit to after both nrf_drv_rng_init and nrf_drv_rng_uninit should be sufficient, right?

@marcuschangarm Yes I think that should be fine.

Actually we only need to synchronise nrf_drv_rng_uninit with nordig_trng_counter. But it would be more robust to do both.

hanno-becker · 2018-04-05T15:36:15Z

targets/TARGET_NORDIC/TARGET_NRF5x/trng_api.c

+    }
+
+    /* Set return value based on how many bytes was read. */
+    return (*output_length == 0) ? -1 : 0;


In the corner case where length == 0, and where trng_get_bytes should be a no-op, this would return failure.

Good point! Thanks!

ciarmcom · 2018-04-05T15:50:31Z

ARM Internal Ref: IOTSSL-2214

hanno-becker · 2018-04-06T08:17:59Z

targets/TARGET_NORDIC/TARGET_NRF5x/trng_api.c

+
+    /* If no bytes are cached, block until at least 1 byte is available. */
+    if (bytes_available == 0) {
+        nrf_drv_rng_block_rand(output, 1);


This branch must not be taken if length == 0. I suggest to check for length == 0 in the beginning and return immediately in this case.

hanno-becker

I have reviewed trng_api.c and made three requests to resolve a race condition and to correct the behaviour of trng_get_bytes in the (useless but allowed) corner case where 0 bytes of entropy are requested.

mazimkhan · 2018-04-06T10:07:38Z

targets/TARGET_NORDIC/TARGET_NRF5x/trng_api.c

+ */
+int trng_get_bytes(trng_t *obj, uint8_t *output, size_t length, size_t *output_length)
+{
+    (void) obj;


trng_init and trng_free are synchronised but not this function. Please document if there are reasons to believe this is thread safe.

Mbed HAL functions are not supposed to be thread safe - the caller is responsible for handling thread safety.

I'm not sure why trng_init and trng_free are safe - they should actually be non-safe to be consistent with the rest of the HAL.

@marcuschangarm Can the original author perhaps shed some light on this?

@marcuschangarm These functions are called from feature/mbedtls and not directly by the user. Mbed TLS calls these functions as abstract API and it does not know if a particular platform requires synchronisation. Hence, it is fine to provide some synchronisation here if needed.

Shouldn't the thread safety be handled in Mbed TLS since that is the user facing API?

We shouldn't be using trng_get_bytes before trng_init is called and after trng_free is called. Hence, there should just be one mutex for these three.
But since other platforms don't do that and Mbed TLS will not be calling it from multiple thread contexts I would say don't bother about threading now. We have to anyways revisit this once we know how it will be handled for all platforms via Mbed TLS.

Thanks @mazimkhan for the info

Now I'm a bit confused! 😄

Do you want me to:

take out all thread safety that is currently there.

insert thread safety everywhere.

I'm fine with either, but the current 2/3 thread safety looks weird.

I also raised the point of 2/3 safety earlier. My opinion is to be consistent with other platforms that don't do any thread safety here. Hence, pt.1.
We will revisit thread safety later for all platforms and may introduce something similar.

Sounds good! I'll remove it completely then!

mazimkhan · 2018-04-06T10:13:42Z

targets/TARGET_NORDIC/TARGET_NRF5x/trng_api.c

+        }
+    }
+
+    /* Set return value based on how many bytes was read. */


typo was -> were

mazimkhan · 2018-04-06T10:16:20Z

targets/TARGET_NORDIC/TARGET_NRF5x/trng_api.c

+
+    /* If no bytes are cached, block until at least 1 byte is available. */
+    if (bytes_available == 0) {
+        nrf_drv_rng_block_rand(output, 1);


Question: Does this function performs a pop operation and clears the byte from the cache. So that same random number is not returned again?

mazimkhan · 2018-04-06T10:16:32Z

targets/TARGET_NORDIC/TARGET_NRF5x/trng_api.c

+            bytes_available = length;
+        }
+
+        ret_code_t result = nrf_drv_rng_rand(output, bytes_available);


Question: Does this function performs a pop operation and clears the bytes from the cache. So that same random number is not returned again?

mazimkhan · 2018-04-06T10:20:32Z

targets/TARGET_NORDIC/TARGET_NRF5x/trng_api.c

+
+    /* If no bytes are cached, block until at least 1 byte is available. */
+    if (bytes_available == 0) {
+        nrf_drv_rng_block_rand(output, 1);


How is this optimum compared to blocking for requested length bytes. Is it possible to state some typical time from the spec.

The typical time per byte is 120 microsecond, but there is no upper bound on per byte generation.

The 1 byte shortcut is to cater to a misinterpretation in the Cloud client on how the TRNG API should be used while still adhering to the TRNG API.

0xc0170

Just one comment in the targets json file. HAL changes looks fine to me

0xc0170 · 2018-04-06T10:07:31Z

targets/targets.json

+            "MBED_TICKLESS"
+        ],
+        "device_has": [
+            "ANALOGIN", 


why are all these on new lines? this is not consistent with the rest of the file.

JSON is typically pretty printed with each element on a new line like this. This is a human readable file after all.

It reduces merge conflicts when several people are adding and removing elements from the list.

From consistency point of view, I would argue leave it as it is for the rest. Those 2 points are good though.

@theotherjimmy What do you think?

I think we should change the style to this. It's always been a bit difficult to read these "device_has" lines like this.

marcuschangarm · 2018-04-07T21:17:14Z

@hanno-arm @mazimkhan

Please take a look and see if this addresses your comments: cc59ae5

@0xc0170

I'll squash the commit once it gets a thumbs up.

0Grit · 2018-04-08T23:23:40Z

targets/TARGET_NORDIC/TARGET_NRF5x/mbed_lib.json

            "help": "UART DMA buffer. 2 buffers per instance. DMA buffer is filled by UARTE",
            "value": 8
        }
+    },
+    "target_overrides": {


@marcuschangarm What is the benefit of placing board specific target_overrides in the nordic library's mbed_lib.json.

From the viewpoint of designing a proprietary board using nrf52840 and custom_target.json:
Assuming mbed_lib.json files automatically overrule custom_target.json, would it not be preferable to make an mbed_lib.json for each target?

example:

mbed-os/targets/TARGET_NORDIC/TARGET_NRF5x/TARGET_NRF52/TARGET_MCU_NRF52840/TARGET_NRF52840_DK/mbed_lib.json

Good question @loverdeg-ep

I am not certain if this change should be part of this PR. I would rather see it separately (needs also documentation where target configuration and why is it being the way it is).

You can't override configuration from one mbed_lib.json from another mbed_lib.json. All the settings in that file are related to configuring the HAL implementations, which is why I moved it out from targets.json.

There is a section in the README.md file about adding targets.

@marcuschangarm Based on @theotherjimmy's new config documentation.

The order in which overrides are considered is:
Libraries override target configuration with mbed_lib.json.
The application overrides target and library configuration with mbed_app.json

So you should be able to declare/define a config parameter in a targets.json and then override it with mbed_lib.json, or mbed_app.json as a last resort.

@vergil-SI and I have found that using custom_targets.json to override mbed_lib.json was a fruitless effort so we took the path of defining an mbed_lib.json for our custom target to override flow control set within "targets/TARGET_NORDIC/TARGET_NRF5x/mbed_lib.json"

Considering this, the approach we will take to implement custom board support is to have a repository containing the following:

PinNames.h device.h custom_target.json mbed_lib.json

Though mbed-cli may not support custom_target.json not being in the root of a project, so we'll see.

Apologies,

We used mbed_app.json to overcome uart-hwfc being defined in:

"mbed-os/targets/TARGET_NORDIC/TARGET_NRF5x/TARGET_NRF52/TARGET_MCU_NRF52840/TARGET_NRF52840_DK/mbed_lib.json"

If this proves to be the only solution then I'm not sure how we'll be able to make a portable and modular custom target BSP.

Will keep working at it but hoping @theotherjimmy or someone else can chime in.

@marcuschangarm @loverdeg-ep The only way I can think of to support custom BSPs is to move the hardware related configuration parameters to the target.

The thing is, that flag is a hack and not really related to the hardware.

In its original implementation, it would force all UART instances to use the RTC/CTS pins for flow control regardless whether the UART was used for STDIO or not. If you changed the interface firmware from DAPLINK to JLINK you would have to disable the flag since JLINK doesn't use flow control.

What we need is a flag in mbed_retarget.cpp to enable flow control explicitly for STDIO since that is where the STDIO instance is initialized from: https://github.com/ARMmbed/mbed-os/blob/master/platform/mbed_retarget.cpp#L148-L149

serial_init(&stdio_uart, tx, rx); serial_baud(&stdio_uart, baud); #if STDIO_FLOWCONTROL serial_set_flow_control(&stdio_uart, FlowControlRTSCTS, rts, cts); #endif

0xc0170 · 2018-04-09T15:24:33Z

Details requests:

[X] Breaking change

What is breaking here? Can this breaking change be part of another PR?

[X] Feature

What new features does this PR bring? Is it related to the SDK update ?

marcuschangarm · 2018-04-09T18:04:28Z

What is breaking here?

I've removed several DEVICE_HAS flags because the hardware can't support it and the previous implementation was wrong.
The file structure has changed. If people included .h files with paths it might not work.
The SDK has changed. If people were relying on functionality directly from the SDK it might be gone.

Can this breaking change be part of another PR?

No, the changes are too fundamental to all the other work.

What new features does this PR bring? Is it related to the SDK update ?

You can add/remove SoftDevice on the fly.
Remove DEVICE_HAS items to save space.
The UART uses DMA and has a more flexible buffer management.
The NRF52840 gets a second UART.
The new SoftDevice provides BLE 5 features to the NRF52840.

Plus loads of bug fixes and other stuff.

0xc0170 · 2018-04-10T09:11:42Z

/morph build

mbed-ci · 2018-04-10T09:55:37Z

Build : SUCCESS

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

Triggering tests

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

mbed-ci · 2018-04-10T11:38:49Z

Exporter Build : SUCCESS

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

hanno-becker

I reviewed the change in trng_api.c and it looks good to me.

Driver uses new API in SDK 14.2

Implement spi_api.c for NRF52X boards using SDK14. This driver does not implement SPI slave functions and does not use SPIM.

When possible, the I2C HAL will now use the TWI driver in SDK 14. The manual I2C API still maps onto the old TWI peripheral directly because the TWI driver doesn't provide the needed low-level functionality.

Added SPI and I2C section.

Serial implementation uses UARTE instead of UART peripheral: * EasyDMA is used for reading and writing data. * Triple buffering for receiving data. See README for full description.

serial_putc didn't work when called with interrupts disabled.

Instance counter keeps track of how many objects have been initialized and freed. On the first object the instance is enabled and on the last object the instance is disabled.

* Consolidated device_has and macros to the main MCU targets. * Moved errata configuration to mbed_lib.json for HAL implementation. * Moved clock configuration to mbed_lib.json for HAL implementation. * Moved UART configuration to mbed_lib.json for HAL implementation.

marcuschangarm · 2018-04-19T16:51:01Z

/morph build

marcuschangarm · 2018-04-19T16:52:18Z

Rebased with flow control changes. Flow control is now off by default and can be configured from custom_target.json.

mbed-ci · 2018-04-19T17:15:50Z

Build : SUCCESS

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

Triggering tests

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

mbed-ci · 2018-04-19T23:02:54Z

Exporter Build : SUCCESS

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

mbed-ci · 2018-04-20T05:46:18Z

Test : SUCCESS

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

0xc0170 added the needs: review label Apr 5, 2018

0xc0170 requested review from pan- and MarceloSalazar April 5, 2018 15:21

0xc0170 added tracking component: security labels Apr 5, 2018

0xc0170 requested review from mazimkhan and hanno-becker April 5, 2018 15:23

hanno-becker reviewed Apr 5, 2018

View reviewed changes

ciarmcom added the mirrored label Apr 5, 2018

hanno-becker reviewed Apr 6, 2018

View reviewed changes

hanno-becker suggested changes Apr 6, 2018

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Apr 6, 2018

mazimkhan suggested changes Apr 6, 2018

View reviewed changes

0xc0170 requested changes Apr 6, 2018

View reviewed changes

marcuschangarm mentioned this pull request Apr 7, 2018

[feature-nrf528xx] Serial re-implementation for the NRF52 series #6435

Merged

0Grit reviewed Apr 8, 2018

View reviewed changes

0xc0170 added the release-version: 5.9.0-rc1 label Apr 9, 2018

0xc0170 previously approved these changes Apr 10, 2018

View reviewed changes

hanno-becker approved these changes Apr 10, 2018

View reviewed changes

mazimkhan approved these changes Apr 10, 2018

View reviewed changes

Marcus Chang and others added 10 commits April 19, 2018 09:40

NRF52 series PWM reimplementation for SDK 14.2

d11f74c

Driver uses new API in SDK 14.2

Implement SPI API for NRF52x

c9d16e9

Implement spi_api.c for NRF52X boards using SDK14. This driver does not implement SPI slave functions and does not use SPIM.

I2C HAL update for NRF52

fc8f0e1

When possible, the I2C HAL will now use the TWI driver in SDK 14. The manual I2C API still maps onto the old TWI peripheral directly because the TWI driver doesn't provide the needed low-level functionality.

README file for NRF52 HAL implementation

9502011

Added SPI and I2C section.

Serial re-implementation for the NRF52 series

3663494

Serial implementation uses UARTE instead of UART peripheral: * EasyDMA is used for reading and writing data. * Triple buffering for receiving data. See README for full description.

Fix serial_putc bug in NRF52 family

cafa8ae

serial_putc didn't work when called with interrupts disabled.

Add resource management for serial for the NRF52 family

18fa941

Instance counter keeps track of how many objects have been initialized and freed. On the first object the instance is enabled and on the last object the instance is disabled.

Fix off-by-one error in NRF52 serial implementation

b9f91b3

Updated target MTB_LAIRD_BL652 to use SDK 14.2

2c651af

marcuschangarm dismissed stale reviews from MarceloSalazar and 0xc0170 via b964420 April 19, 2018 16:44

0xc0170 added ready for merge and removed needs: CI needs: preceding PR labels Apr 20, 2018

0xc0170 approved these changes Apr 20, 2018

View reviewed changes

adbridge merged commit 6634e46 into ARMmbed:master Apr 20, 2018

adbridge removed the ready for merge label Apr 20, 2018

ciarmcom added the closed_in_jira label Apr 20, 2018

MarceloSalazar mentioned this pull request Apr 23, 2018

I2C stops working after some time (/SPI collision?) #5671

Closed

maciejbocianski mentioned this pull request Apr 27, 2018

missed out on "fire_interrupt" #6767

Closed

adbridge mentioned this pull request May 4, 2018

Fix NRF52 SPI pin initialization #6717

Merged

marcuschangarm deleted the feature-nrf52 branch May 25, 2018 18:00

MateuszMaz mentioned this pull request Aug 8, 2018

NRF52 PwmOut inverted #7707

Closed

Update Nordic NRF52 based targets to SDK 14.2 #6547

Update Nordic NRF52 based targets to SDK 14.2 #6547

Uh oh!

Conversation

marcuschangarm commented Apr 5, 2018

Description

Pull request type

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ciarmcom commented Apr 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hanno-becker left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mazimkhan Apr 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcuschangarm commented Apr 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

hanno-becker left a comment •

edited

Loading

mazimkhan Apr 6, 2018 •

edited

Loading

0xc0170 commented Apr 9, 2018 •

edited

Loading