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.

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

boards/nrf52-based: factor out shared code #8102

Merged
merged 4 commits into from
Nov 30, 2017
Merged

boards/nrf52-based: factor out shared code #8102

merged 4 commits into from
Nov 30, 2017

Conversation

haukepetersen
Copy link
Contributor

following the structure proposed in #8058

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

@haukepetersen 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
dylad
dylad reviewed Nov 21, 2017
View reviewed changes
Copy link
Member

@dylad dylad left a comment

Choose a reason for hiding this comment

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

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

boards/common/nrf52/Makefile.include Outdated
export JLINK_DEVICE := nrf52

# special options when using SoftDevice
ifneq (,$(filter nordic_softdevice_ble,$(USEPKG)))
Copy link
Member

Choose a reason for hiding this comment

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

Why ifneq ? It should be ifeq.

Copy link
Member

Choose a reason for hiding this comment

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

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

boards/common/nrf52/Makefile.include Outdated
# special options when using SoftDevice
ifneq (,$(filter nordic_softdevice_ble,$(USEPKG)))
export JLINK_PRE_FLASH := erase\nloadfile $(BINDIR)/softdevice.hex
export FLASH_ADDR := 0x1f000
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

boards/common/nrf52/Makefile.dep Outdated

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

@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
Copy link
Member

dylad commented Nov 22, 2017

@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
Copy link
Contributor Author

Maybe common/nrf52xxxdk ?

sounds good, will do.

@kaspar030 kaspar030 mentioned this pull request Nov 23, 2017
Merged
@haukepetersen
Copy link
Contributor Author

renamed the common folder to nrf52xxxdk.

@haukepetersen
Copy link
Contributor Author

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

@miri64
Copy link
Member

miri64 commented Nov 24, 2017

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 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
Copy link
Contributor Author

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 haukepetersen removed the State: waiting for other PR State: The PR requires another PR to be merged first label Nov 30, 2017
@haukepetersen
Copy link
Contributor Author

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

@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 Nov 30, 2017
@haukepetersen haukepetersen force-pushed the opt_boards_nrf52based branch 2 times, most recently from c8ffdd7 to baba39e Compare November 30, 2017 13:18
@haukepetersen
Copy link
Contributor Author

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
Copy link
Member

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
PeterKietzmann requested changes Nov 30, 2017
View reviewed changes
Copy link
Member

@PeterKietzmann PeterKietzmann left a comment

Choose a reason for hiding this comment

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

@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

boards/nrf52dk/include/board.h
@@ -37,6 +37,7 @@ extern "C" {
#define LED2_PIN GPIO_PIN(0, 19)
#define LED3_PIN GPIO_PIN(0, 20)

#define LED_PORT (NRF_P0)
Copy link
Member

Choose a reason for hiding this comment

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

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

@PeterKietzmann
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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

PeterKietzmann
PeterKietzmann approved these changes Nov 30, 2017
View reviewed changes
Copy link
Member

@PeterKietzmann PeterKietzmann left a comment

Choose a reason for hiding this comment

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

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

@PeterKietzmann PeterKietzmann merged commit 157d8e2 into RIOT-OS:master Nov 30, 2017
@haukepetersen haukepetersen deleted the opt_boards_nrf52based branch November 30, 2017 15:42
@aabadie aabadie added this to the Release 2018.01 milestone Jan 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dylad dylad dylad left review comments

@PeterKietzmann PeterKietzmann PeterKietzmann approved these changes

Assignees

@aabadie aabadie

@kYc0o kYc0o

@PeterKietzmann PeterKietzmann

Labels
Area: boards Area: Board ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Milestone
Release 2018.01
Development

Successfully merging this pull request may close these issues.
6 participants
@haukepetersen @dylad @miri64 @PeterKietzmann @aabadie @kYc0o