Skip to content
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

boards/nrf52xxxdk: make reset pin work #10072

Merged
merged 2 commits into from Oct 12, 2018

Conversation

haukepetersen
Copy link
Contributor

Contribution description

Ever wondered why on most/all of our nrf52dk and nrf52840dk boards the reset pin is not working? Just stumbled upon the doc, where it states that the pin needs to be manually programmed (at least once) to work. This PR does exactly that, and voila, not the reset pins on those boards work like a charm.

Testing procedure

  • take a nrf52dk or nrf52840dk board where the reset pin is not working
  • flash any RIOT example on the board
  • reset the board (do a power-off-on or use make reset, as the changes take one reset cycle to take effect)
  • enjoy a working reset pin

Issues/PRs references

none

@haukepetersen haukepetersen added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: boards Area: Board ports State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Area: tools Area: Supplementary tools and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 28, 2018
@haukepetersen
Copy link
Contributor Author

Hmm, have spend some more thought on this. Since the config is held in persistent memory, there is not need to check on every system boot.

I will instead provide stand-alone application which provides the means to program the reset pin.

@bergzand
Copy link
Member

Is it possible to use some GDB magic to write these values (persistently) to the device?

@haukepetersen
Copy link
Contributor Author

have not found anything.

Copy link
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

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

Tested it on the nrf52dk and works like a charm!

@aabadie
Copy link
Contributor

aabadie commented Sep 28, 2018

with #9407 and when using openocd, I think the reset pin works

@@ -21,8 +21,46 @@
#include "cpu.h"
#include "board.h"

#if defined(BOARD_NRF52DK)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not but the macros in the board.h file?

Copy link
Member

Choose a reason for hiding this comment

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

Or periph_conf.h ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be in place that LED0 macros are defined since it is basically a friendly name for a pin.

Copy link
Contributor

Choose a reason for hiding this comment

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

But it is just for consideration, not trying to push.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, have spend some more thought on this. Since the config is held in persistent memory, there is not need to check on every system boot.

I will instead provide stand-alone application which provides the means to program the reset pin.

just wait some more minutes :-)

@@ -21,8 +21,46 @@
#include "cpu.h"
#include "board.h"

#if defined(BOARD_NRF52DK)
#define RESET_PIN (21U)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why pin 21 and 18?

Copy link
Member

Choose a reason for hiding this comment

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

This must be hardware related. nrf52832dk board reset is physically connected to P0.21.
Guess its P0.18 for nrf52840dk board.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ask Nordic why they have chosen them...

This new tool allows configuring the reset pin for nRF52-based
boards. As the reset pin configuration is persistent, it does not
make sense to include it into the board code...
@haukepetersen
Copy link
Contributor Author

Did rework this PR, now the actual configuration is put in its own tool. So no more runtime or code overhead for the existing board implementations. The tool is also usable for any other board we might stumble upon...

@haukepetersen haukepetersen removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Sep 28, 2018
@haukepetersen
Copy link
Contributor Author

@MrKevinWeiss would you mind to take another look? This PR is ready for final review.

@MrKevinWeiss
Copy link
Contributor

No problem!

@MrKevinWeiss MrKevinWeiss added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer 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 Oct 11, 2018
@MrKevinWeiss
Copy link
Contributor

Tested on the nrf52dk, on one that had already been programmed and it was able to reset. I then tested it on one that had not been programmed and it worked. I don't have nrf52840dk so I cannot test on it. @haukepetersen can you (or anyone) confirm it works on the nrf52840dk?

Maybe just add tools to the @defgroup in RIOT/doc.txt similar to the utils or something to make travis happy.

@MrKevinWeiss MrKevinWeiss removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 11, 2018
@haukepetersen
Copy link
Contributor Author

@MrKevinWeiss I can confirm that the tool works also for nrf52840dk boards.

@haukepetersen haukepetersen added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 11, 2018
@haukepetersen
Copy link
Contributor Author

Something is rather broken in the doxygen config, as *.c files should not even be looked at by doxygen, and the tool added here should also not show up in the API docs.

@haukepetersen
Copy link
Contributor Author

strange. Is complaining about a doxygen group in dist/tools/nrf52_resetpin_cfg/main.c, but doxygen should normally a) ignore *.c files, and b) not even parse anything in the dist/* folder of RIOT.

So anyone any idea?!

@miri64
Copy link
Member

miri64 commented Oct 11, 2018

So anyone any idea?!

Restart build?

@aabadie
Copy link
Contributor

aabadie commented Oct 11, 2018

It is not doxygen who checks existing groups but hand made shell stuff (based on regexp), that's why.

@aabadie
Copy link
Contributor

aabadie commented Oct 11, 2018

The best would be to either remove the @ingroup tools or create one somewhere else in a doc.txt.

@miri64
Copy link
Member

miri64 commented Oct 11, 2018

Or fixing the script ;-)

diff --git a/dist/tools/doccheck/check.sh b/dist/tools/doccheck/check.sh
index 6cd2e6e..d55d3c6 100755
--- a/dist/tools/doccheck/check.sh
+++ b/dist/tools/doccheck/check.sh
@@ -32,7 +32,7 @@ then
 fi
 
 exclude_filter() {
-    grep -v -e vendor -e examples -e tests
+    grep -v -e vendor -e examples -e tests -e dist
 }
 
 # Check all groups are defined

@aabadie
Copy link
Contributor

aabadie commented Oct 11, 2018

Or fixing the script ;-)

Indeed !

@miri64
Copy link
Member

miri64 commented Oct 11, 2018

See #10152!

@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 11, 2018
@MrKevinWeiss
Copy link
Contributor

Looks good! ACK

@MrKevinWeiss MrKevinWeiss merged commit 0719442 into RIOT-OS:master Oct 12, 2018
@haukepetersen
Copy link
Contributor Author

@miri64 thanks for addressing my issue!

All green -> go.

@haukepetersen haukepetersen deleted the fix_nrf52xxxdk_resetpin branch October 12, 2018 07:37
@haukepetersen
Copy link
Contributor Author

@MrKevinWeiss you beat me by 38 seconds :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants