boards/nrf52-based: factor out shared code by haukepetersen · Pull Request #8102 · RIOT-OS/RIOT

haukepetersen · 2017-11-21T15:26:07Z

following the structure proposed in #8058

Both Nordic dev-kits share duplicate a bit of configuration, so this PR makes them share it.

dylad

Just a quick overview. Otherwise the PR looks good;
I'll run some tests as soon as I can.

dylad · 2017-11-21T20:37:33Z

boards/common/nrf52/Makefile.include

+export JLINK_DEVICE := nrf52
+
+# special options when using SoftDevice
+ifneq (,$(filter nordic_softdevice_ble,$(USEPKG)))


Why ifneq ? It should be ifeq.

forget this one, I read it too fast...

dylad · 2017-11-21T20:38:09Z

boards/common/nrf52/Makefile.include

+# special options when using SoftDevice
+ifneq (,$(filter nordic_softdevice_ble,$(USEPKG)))
+  export JLINK_PRE_FLASH := erase\nloadfile $(BINDIR)/softdevice.hex
+  export FLASH_ADDR := 0x1f000


Is there a way to get this value from the ld script instead ?

I have no idea, but then again, I'd say this is not part of this PR...

dylad · 2017-11-21T20:48:02Z

boards/common/nrf52/Makefile.dep

+
+ifeq (,$(filter nrfmin,$(USEMODULE)))
+  ifneq (,$(filter gnrc_netdev_default,$(USEMODULE)))
+    USEPKG += nordic_softdevice_ble


Apparently there is an order issue with the USEPKG here and the ifneq (,$(filter nordic_softdevice_ble,$(USEPKG))) in Makefile.include
see discussion in #8068

yep, so you think we should wait for #8068 before going on with this PR?

This is not mandatory, we can merge this one "as it", I'll rebase #8068 and try to fix this issue since it is related to the softdevice. But if you already have a potential fix you can add it there.

haukepetersen · 2017-11-22T12:51:35Z

@dylad thanks for your review! My plan was simply to copy together all the shared files for the two nrf52dk boards, I did not try to improve anything else (scope for other PRs...).

But I had one more thought: it seems to me by now, that naming the shared folder something like common/nrf52dk could be smarter, as many things in the shared code are rather specific to the nordic nrf52 dk boards and not so much to other nrf52-based boards. Would you agree?

dylad · 2017-11-22T13:09:15Z

@haukepetersen I am not against but I found the name common/nrf52dk a bit misleading as nrf52dk (on google) will point you to the nRF52832 only, without any word on the nRF52840.
Maybe common/nrf52xxxdk ?

haukepetersen · 2017-11-22T13:58:16Z

Maybe common/nrf52xxxdk ?

sounds good, will do.

haukepetersen · 2017-11-24T09:47:55Z

renamed the common folder to nrf52xxxdk.

haukepetersen · 2017-11-24T09:50:25Z

Anyone any idea what Murdock's problem is here? Locally the builds run fine on my system...

miri64 · 2017-11-24T09:54:33Z

If I try to compile one of the failing applications I get

/home/mlenders/Repositories/RIOT-OS/RIOT/makefiles/info-global.inc.mk:44: /home/mlenders/Repositories/RIOT-OS/RIOT/boards/common/Makefile.features: No such file or directory

You should really update your system some time ;-)

haukepetersen · 2017-11-24T10:11:03Z

Ok, running the buildtest I get the same error now.

This PR needs #8058 to work, so added the 'Waiting for Other PR' label

haukepetersen · 2017-11-30T12:19:47Z

rebased as #8058 was merged. PR is ready for review now.

haukepetersen · 2017-11-30T13:19:26Z

The nrf5280dk does not seem to work with the softdevice package as I thought, so I have moved the inclusion of the soft device into the nrf52dk only. This should fix all the build issues that Murdock complained about.

PeterKietzmann · 2017-11-30T13:57:24Z

I'll review this one in a minute. How did you figure out the nrf5280dk is not working with softdevice, you mean just compilation-wise? Because I assume it's not even executing with the nrf52dk at the moment (see #6379 and #8068)

PeterKietzmann

@haukepetersen code-wise your changes look fine. I'm sorry that I didn't join the discussion about naming the common folder earlier, but IMO "nrf52xxxdk" might get misleading when introducing #8035. The aconno board is also based on a nrf52832xxaa and might benefit from the common structure but "dk" is not part of the name. What do you think? Maybe nrf52-based?
Will do some tests next

PeterKietzmann · 2017-11-30T14:10:54Z

boards/nrf52dk/include/board.h

 #define LED2_PIN            GPIO_PIN(0, 19)
 #define LED3_PIN            GPIO_PIN(0, 20)

+#define LED_PORT            (NRF_P0)


Now that you've defined this, pleas apply below!

PeterKietzmann · 2017-11-30T14:38:20Z

Tests succeeded. Now that you touch these files how about addin to the LED/BTN SAUL adaption:
.flags = (SAUL_GPIO_INVERTED | SAUL_GPIO_INIT_CLEAR)

haukepetersen · 2017-11-30T15:16:42Z

Now that you touch these files how about addin to the LED/BTN SAUL adaption:

Makes indeed a lot of sense: done.

Regarding the naming: IMHO the name nrf52xxxdk fix perfectly, as this is what the common module abstracts right now. But of course we might w.r.t. upcoming/other nrf52-based boards either (i) split the common path even more to cater for shared code elements, or (ii) rename the folder once it actually houses shared code for more than just the dk boards. But for now I think the name makes sense...

PeterKietzmann · 2017-11-30T15:26:11Z

Alright then I'll consider renaming that folder once I am at #8035 again.

PeterKietzmann

Comments have been addressed, re-tested, ACK and go

haukepetersen added Area: boards Area: Board ports Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 21, 2017

haukepetersen assigned aabadie, kYc0o and PeterKietzmann Nov 21, 2017

dylad reviewed Nov 21, 2017

View reviewed changes

kaspar030 mentioned this pull request Nov 23, 2017

boards/ACD52832: add new NRF52 based development board #8035

Merged

haukepetersen force-pushed the opt_boards_nrf52based branch from 8c40d75 to 0a982eb Compare November 24, 2017 09:47

haukepetersen added State: waiting for other PR State: The PR requires another PR to be merged first and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 24, 2017

haukepetersen force-pushed the opt_boards_nrf52based branch from 0a982eb to 08fc950 Compare November 30, 2017 12:19

haukepetersen removed the State: waiting for other PR State: The PR requires another PR to be merged first label Nov 30, 2017

haukepetersen added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 30, 2017

haukepetersen force-pushed the opt_boards_nrf52based branch 2 times, most recently from c8ffdd7 to baba39e Compare November 30, 2017 13:18

PeterKietzmann requested changes Nov 30, 2017

View reviewed changes

haukepetersen added 2 commits November 30, 2017 15:50

boards: factored out shared code for nRF52 boards

076de4e

boards/nrf52dk: use common code

2d93d14

boards/nrf52840dk: use common code

cb2ca02

haukepetersen force-pushed the opt_boards_nrf52based branch from baba39e to cb2ca02 Compare November 30, 2017 14:51

boards/common/nrf52: fixed SAUL gpio params

41ec9b2

PeterKietzmann approved these changes Nov 30, 2017

View reviewed changes

PeterKietzmann merged commit 157d8e2 into RIOT-OS:master Nov 30, 2017

haukepetersen deleted the opt_boards_nrf52based branch November 30, 2017 15:42

aabadie added this to the Release 2018.01 milestone Jan 18, 2018

Conversation

haukepetersen commented Nov 21, 2017

Uh oh!

dylad 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!

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!

haukepetersen commented Nov 22, 2017

Uh oh!

dylad commented Nov 22, 2017

Uh oh!

haukepetersen commented Nov 22, 2017

Uh oh!

haukepetersen commented Nov 24, 2017

Uh oh!

haukepetersen commented Nov 24, 2017

Uh oh!

miri64 commented Nov 24, 2017

Uh oh!

haukepetersen commented Nov 24, 2017

Uh oh!

haukepetersen commented Nov 30, 2017

Uh oh!

haukepetersen commented Nov 30, 2017

Uh oh!

PeterKietzmann commented Nov 30, 2017

Uh oh!

PeterKietzmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PeterKietzmann commented Nov 30, 2017

Uh oh!

haukepetersen commented Nov 30, 2017

Uh oh!

PeterKietzmann commented Nov 30, 2017

Uh oh!

PeterKietzmann left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants