Skip to content

cpu/nrf5x_common: map hwrng to SoC library if SoftDevice is present #11559

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 4, 2019
Merged

cpu/nrf5x_common: map hwrng to SoC library if SoftDevice is present #11559

merged 1 commit into from
Jul 4, 2019

Conversation

PeterKietzmann
Copy link
Member

@PeterKietzmann PeterKietzmann commented May 21, 2019 •
edited
Loading

Contribution description

According to this comment which bases on Nordic documentation, the hardware RNG can't be accessed when the SoftDevice is used. The fix maps our hwrng_read() function the the respective library function call (see here for reference)

Testing procedure

(i)

Build/flash/term examples/gnrc_networking for an nrf5X based board.

(ii)

Build/flash/term tests/periph_hwrng for an nrf5x based board and include the SoftDevice package. E.g.:
USEPKG=nordic_softdevice_ble BOARD=nrf52dk make -C tests/periph_hwrng flash term

W/o this fix, there is no output as the initialization hangs before main() is called. W/ this fix, everything should work as expected.

Issues/PRs references

Fixes #11091

@PeterKietzmann PeterKietzmann added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: drivers Area: Device drivers Area: BLE Area: Bluetooth Low Energy support labels May 21, 2019
@PeterKietzmann
Copy link
Member Author

@Citrullin please test

@PeterKietzmann PeterKietzmann mentioned this pull request May 22, 2019
Closed
@SemjonWilke
Copy link
Member

Does not compile for examples/gnrc_networking and tests/periph_hwrng on nrf52840dk.

Also the example does not open a shell on nrf52dk (the same for master, so unrelated but not testable), when using the softdevice. Does open a shell with nrfmin though.

tests/periph_hwrng goes through on nrf52dk.

@PeterKietzmann
Copy link
Member Author

Does not compile for examples/gnrc_networking and tests/periph_hwrng on nrf52840dk.

I just found that it doesn't even compile on master when USEPKG=nordic_softdevice_ble. I'm not very familiar with the nordic hardware nor with the softdevice. Is this feature expected on the nrf52840?

Also the example does not open a shell on nrf52dk

I can't reproduce that. Everything works smooth on my machine. Did you check out my branch or merge/pick the commit somehow? Otherwise this seems like a toolchain issue. I'm on:

Operating System Environment
-----------------------------
       Operating System: "Ubuntu" "18.04.2 LTS (Bionic Beaver)"
                 Kernel: Linux 4.15.0-50-generic x86_64 x86_64

Installed compiler toolchains
-----------------------------
             native gcc: gcc (Ubuntu 7.4.0-1ubuntu1~18.04) 7.4.0
      arm-none-eabi-gcc: arm-none-eabi-gcc (GNU Tools for Arm Embedded Processors 7-2018-q3-update) 7.3.1 20180622 (release) [ARM/embedded-7-branch revision 261907]
                avr-gcc: missing
       mips-mti-elf-gcc: missing
             msp430-gcc: msp430-gcc (GCC) 4.6.3 20120301 (mspgcc LTS 20120406 unpatched)
   riscv-none-embed-gcc: missing
   xtensa-esp32-elf-gcc: missing
   xtensa-lx106-elf-gcc: missing
                  clang: missing

Installed compiler libs
-----------------------
   arm-none-eabi-newlib: "3.0.0"
    mips-mti-elf-newlib: missing
riscv-none-embed-newlib: missing
xtensa-esp32-elf-newlib: missing
xtensa-lx106-elf-newlib: missing
               avr-libc: missing (missing)

Installed development tools
---------------------------
                  cmake: cmake version 3.10.2
               cppcheck: missing
                doxygen: 1.8.13
                 flake8: 3.6.0 (mccabe: 0.6.1, pycodestyle: 2.4.0, pyflakes: 2.0.0) CPython 3.6.7 on Linux
                    git: git version 2.17.1
                   make: GNU Make 4.1
                openocd: Open On-Chip Debugger 0.10.0
                 python: Python 2.7.15rc1
                python2: Python 2.7.15rc1
                python3: Python 3.6.7
             coccinelle: missing

@haukepetersen haukepetersen mentioned this pull request Jul 1, 2019
Merged
@haukepetersen
Copy link
Contributor

Just checked, the fix seems to do the job for nrf52832-based boards. I do however have some improvements regarding the code in mind -> see PeterKietzmann#11

AFAIK the softdevice was never verified to work with the nrf52840, so no surprise this is not working. I opened a PR introducing an explicit feature to resolve this -> #11769

@SemjonWilke SemjonWilke removed their request for review July 1, 2019 18:19
@PeterKietzmann PeterKietzmann mentioned this pull request Jul 1, 2019
Merged
@PeterKietzmann PeterKietzmann force-pushed the pr_nrf5x_hwrng_softdev branch from 139780b to 84ca8b5 Compare July 1, 2019 18:25
@PeterKietzmann PeterKietzmann added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 1, 2019
@PeterKietzmann
Copy link
Member Author

Thanks @haukepetersen! I've reviewed, tested and merged your changes into my branch and rebased + squashed this PR afterwards to run CI

miri64
miri64 reviewed Jul 1, 2019
View reviewed changes
cpu/nrf5x_common/periph/hwrng.c
} while (avail < (uint8_t)num);

ret = sd_rand_application_vector_get((uint8_t *)buf, (uint8_t)num);
assert(ret == NRF_SUCCESS);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should also be (void)ret;'ed, in case assert resolves to NOP.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I have missed that.

@PeterKietzmann I think its the easiest if you fix that in your branch directly?!

@PeterKietzmann
Copy link
Member Author

Ok. Ready to squash?

@haukepetersen
Copy link
Contributor

yes, please go ahead

haukepetersen
haukepetersen approved these changes Jul 4, 2019
View reviewed changes
Copy link
Contributor

@haukepetersen haukepetersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK, works as expected and code is valid

@haukepetersen haukepetersen added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Jul 4, 2019
@PeterKietzmann PeterKietzmann force-pushed the pr_nrf5x_hwrng_softdev branch from d7c2871 to 7ee9905 Compare July 4, 2019 10:12
@PeterKietzmann
Copy link
Member Author

Squashed

@haukepetersen haukepetersen merged commit 1744b6b into RIOT-OS:master Jul 4, 2019
@haukepetersen
Copy link
Contributor

Merged :-)

@cladmi cladmi added this to the Release 2019.07 milestone Jul 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miri64 miri64 miri64 left review comments

@haukepetersen haukepetersen haukepetersen approved these changes

Assignees
No one assigned
Labels
Area: BLE Area: Bluetooth Low Energy support Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2019.07
Development

Successfully merging this pull request may close these issues.

gnrc_minimal on nRF52DK do not work anymore
5 participants
@PeterKietzmann @SemjonWilke @haukepetersen @miri64 @cladmi