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

Update Nordic NRF52 based targets to SDK 14.2 #6547

Merged
merged 39 commits into from Apr 20, 2018

Conversation

@marcuschangarm
Contributor

marcuschangarm commented Apr 5, 2018

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

@0xc0170 0xc0170 requested review from pan- and MarceloSalazar Apr 5, 2018

@0xc0170 0xc0170 requested review from mazimkhan and hanno-arm Apr 5, 2018

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

This comment has been minimized.

@hanno-arm

hanno-arm Apr 5, 2018

Contributor

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.

This comment has been minimized.

@marcuschangarm

marcuschangarm Apr 5, 2018

Contributor

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

This comment has been minimized.

@hanno-arm

hanno-arm Apr 6, 2018

Contributor

@marcuschangarm Yes I think that should be fine.

This comment has been minimized.

@mazimkhan

mazimkhan Apr 6, 2018

Contributor

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

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

This comment has been minimized.

@hanno-arm

hanno-arm Apr 5, 2018

Contributor

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

This comment has been minimized.

@marcuschangarm

marcuschangarm Apr 5, 2018

Contributor

Good point! Thanks!

@ciarmcom

This comment has been minimized.

Member

ciarmcom commented Apr 5, 2018

ARM Internal Ref: IOTSSL-2214

@ciarmcom ciarmcom added the mirrored label Apr 5, 2018

/* 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 comment has been minimized.

@hanno-arm

hanno-arm Apr 6, 2018

Contributor

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-arm

hanno-arm suggested changes Apr 6, 2018 edited

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.

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

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

This comment has been minimized.

@mazimkhan

mazimkhan Apr 6, 2018

Contributor

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

This comment has been minimized.

@marcuschangarm

marcuschangarm Apr 6, 2018

Contributor

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.

This comment has been minimized.

@hanno-arm

hanno-arm Apr 6, 2018

Contributor

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

This comment has been minimized.

@mazimkhan

mazimkhan Apr 6, 2018

Contributor

@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.

This comment has been minimized.

@marcuschangarm

marcuschangarm Apr 6, 2018

Contributor

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

This comment has been minimized.

@mazimkhan

mazimkhan Apr 6, 2018

Contributor

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.

This comment has been minimized.

@0xc0170

0xc0170 Apr 6, 2018

Member

Thanks @mazimkhan for the info

This comment has been minimized.

@marcuschangarm

marcuschangarm Apr 6, 2018

Contributor

Now I'm a bit confused! 😄

Do you want me to:

  1. take out all thread safety that is currently there.
  2. insert thread safety everywhere.

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

This comment has been minimized.

@mazimkhan

mazimkhan Apr 6, 2018

Contributor

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.

This comment has been minimized.

@marcuschangarm

marcuschangarm Apr 6, 2018

Contributor

Sounds good! I'll remove it completely then!

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

This comment has been minimized.

@mazimkhan

mazimkhan Apr 6, 2018

Contributor

typo was -> were

/* 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 comment has been minimized.

@mazimkhan

mazimkhan Apr 6, 2018

Contributor

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

This comment has been minimized.

@marcuschangarm

marcuschangarm Apr 6, 2018

Contributor

It does.

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

This comment has been minimized.

@mazimkhan

mazimkhan Apr 6, 2018

Contributor

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

This comment has been minimized.

@marcuschangarm

marcuschangarm Apr 6, 2018

Contributor

It does.

/* 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 comment has been minimized.

@mazimkhan

mazimkhan Apr 6, 2018

Contributor

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

This comment has been minimized.

@marcuschangarm

marcuschangarm Apr 6, 2018

Contributor

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

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

This comment has been minimized.

@0xc0170

0xc0170 Apr 6, 2018

Member

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

This comment has been minimized.

@marcuschangarm

marcuschangarm Apr 6, 2018

Contributor
  • 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.

This comment has been minimized.

@0xc0170

0xc0170 Apr 6, 2018

Member

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?

This comment has been minimized.

@theotherjimmy

theotherjimmy Apr 6, 2018

Contributor

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

This comment has been minimized.

Contributor

marcuschangarm commented Apr 7, 2018

@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.

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

This comment has been minimized.

@loverdeg-ep

loverdeg-ep Apr 8, 2018

@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

This comment has been minimized.

@0xc0170

0xc0170 Apr 9, 2018

Member

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).

This comment has been minimized.

@marcuschangarm

marcuschangarm Apr 9, 2018

Contributor

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.

This comment has been minimized.

@loverdeg-ep

loverdeg-ep Apr 9, 2018

@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.

This comment has been minimized.

@loverdeg-ep

loverdeg-ep Apr 9, 2018

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.

This comment has been minimized.

@theotherjimmy

theotherjimmy Apr 9, 2018

Contributor

@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.

This comment has been minimized.

@marcuschangarm

marcuschangarm Apr 9, 2018

Contributor

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

This comment has been minimized.

Member

0xc0170 commented Apr 9, 2018

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

This comment has been minimized.

Contributor

marcuschangarm commented Apr 9, 2018

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

This comment has been minimized.

Member

0xc0170 commented Apr 10, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Apr 10, 2018

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

This comment has been minimized.

@hanno-arm

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

marcuschangarm added some commits Mar 27, 2018

Fix serial_putc bug in NRF52 family
serial_putc didn't work when called with interrupts disabled.
Add resource management for serial for the NRF52 family
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.
Reorganize targets.json for NRF52 based targets
* 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 marcuschangarm dismissed stale reviews from MarceloSalazar and 0xc0170 via b964420 Apr 19, 2018

@marcuschangarm marcuschangarm force-pushed the marcuschangarm:feature-nrf52 branch from 90ca8d4 to b964420 Apr 19, 2018

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Apr 19, 2018

/morph build

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Apr 19, 2018

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

@mbed-ci

This comment has been minimized.

mbed-ci commented Apr 19, 2018

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

This comment has been minimized.

@mbed-ci

This comment has been minimized.

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

12 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/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9262 cycles (+320 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/littlefs Passed, code size is 10112B (+0.00%)
Details
travis-ci/tools Local tools testing has passed
Details

marcuschangarm added a commit to marcuschangarm/mbed-os that referenced this pull request Apr 23, 2018

marcuschangarm added a commit to marcuschangarm/mbed-os that referenced this pull request Apr 29, 2018

marcuschangarm added a commit to marcuschangarm/mbed-os that referenced this pull request May 3, 2018

marcuschangarm added a commit to marcuschangarm/mbed-os that referenced this pull request May 7, 2018

kegilbert added a commit to kegilbert/mbed-os that referenced this pull request May 17, 2018

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

@MateuszMaz MateuszMaz referenced this pull request Aug 8, 2018

Closed

NRF52 PwmOut inverted #7707

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment