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

PeterKietzmann · 2017-11-14T13:50:15Z

This PR takes over #7501. It adds a new development board by 'aconno' which is based on a NRF52X MCU and has a bunch of nice features on top (unfortunately not all supported yet). Further information can be found here.

Until know I used an external USB/UART converter for testing but we should consider the Segger RTT Client tool as default behind the make command in future.

smlng

looks good to me, untested ACK!

PeterKietzmann · 2017-11-14T20:03:21Z

You can test (if you want). I can give you the such a board tomorrow. Otherwise I've tested UART, LED, GPIO, SAUL and gave it a quick shot to communicate over NRFMIN with a nrf52dk board.

kaspar030

Some duplication must go...

kaspar030 · 2017-11-14T22:03:41Z

boards/acd52832/Makefile.features

@@ -0,0 +1,18 @@
+# Put defined MCU peripherals here (in alphabetical order)
+FEATURES_PROVIDED += periph_cpuid


drop cpuid, flashpage, hwrng as they're provided by the cpu

kaspar030 · 2017-11-14T22:03:57Z

boards/acd52832/Makefile.features

+FEATURES_PROVIDED += periph_uart
+
+# Various other features (if any)
+FEATURES_PROVIDED += cpp


drop cpp too

kaspar030 · 2017-11-14T22:04:54Z

boards/acd52832/Makefile.dep

@@ -0,0 +1,13 @@
+ifneq (,$(filter saul_default,$(USEMODULE)))


This file seems to be a copy of the one in boards/nrf52dk. Please deduplicate.

I assume it is. What kind of deduplication do you have in mind, something like a board_common folder?

What kind of deduplication do you have in mind, something like a board_common folder?

Yes.
In this case, I'd suggest a folder called "nrf5x-common". The non-common nrf5x Makefile.dep would contain a line that just include the common one.

Technically I understand your proposal to avoid duplication. On the other hand this is board specific and the board here is not an NRF Nordic board but the aconno. I fear this might get un-intuitive. For example: I find it quite un-intuitive that the waspmote-pro depends on arduino-atmega-common. Did I change your mind :-)? If not, I still think we should introduce a common folder in a separate PR, right?

So you think we should wait for something like #8044 right?

Yes... Maybe #8102!

Sure but before I start reviewing #8102 I'd like to see #8058 merged

kaspar030 · 2017-11-14T22:05:33Z

boards/acd52832/Makefile.include

@@ -0,0 +1,21 @@
+# define the cpu used by the acd52832


Also a copy of nrf52dk?

Only the CPU_MODEL differs from common/nrf52xxxdk/Makefile.include?

PeterKietzmann · 2017-12-01T08:36:53Z

@haukepetersen I've tried to re-use as as muchcode as possible from common/nrf52BLABLA. Unfortunately, I can not overwrite the board_init() function that easy, but IMO it makes sense to have a separate one here, as the aconno has several of on-board sensors/actuators. What's your opinion on that?

kaspar030 · 2017-12-01T08:45:59Z

What's your opinion on that?

Don't initialize the sensors at board level. This has to be solved higher up.
If the sensors have saul drivers, add them as dependency of saul_default in the board's Makefile.dep.

PeterKietzmann · 2017-12-01T09:05:17Z

Fine. The next question is how/if to generalize the common nrf52BLBLA board_init function. It currently initializes all four LEDs which is valid for addressed boards. We could -of course- adapt/dispatch somehow so the aconno is also covered, but I'm not sure whether this is a clean approach (because the aconno is actually a different board). What do you think @kaspar030?

kYc0o

Just some small changes that can improve the support for this board, I'd like to hear more about the motivations to include it as a nrf52 board.

kYc0o · 2017-12-01T14:33:47Z

boards/acd52832/Makefile

@@ -0,0 +1,4 @@
+MODULE = board
+DIRS = $(RIOTBOARD)/common/nrf52xxxdk


It's not better +=? (in case of this actually belongs to nrf52 common boards)

In the current state it doesn't matter but you might be right in general. I just copied from the other nrf52-based boards. Can we take it as for the others now and open another PR in case you want to see this addressed?

Ok, let's see how the support for this, IMHO, very complete CPU, evolves.

kYc0o · 2017-12-01T14:36:34Z

boards/acd52832/include/periph_conf.h

+/**
+ * @name    Clock configuration
+ *
+ * @note    The radio will not work with the internal RC oscillator!


Hmmm... this sounds familiar from the EFM family CPUs, can you take a look on how it was solved?

Will check. Can you reference a discussion?

Yes, you can find @basilfx PR here.

I don't see how this is related. I just checked the reference manual which states The HFXO must be running to use the RADIO -> so this is a hardware 'limitation' and nothing to address here.

I meant that the clock configuration for the whole family can be factorised and generalised, but well... it can come in a following up PR...

Ahhh ok. Well that is an other feature which I don't want to address here. But yes, good idea in general

meant that the clock configuration for the whole family can be factorised and generalised,

This is already done: the two values here are actually needed to be defined by each board, as only the boards knows about its used oscillators...

kYc0o · 2017-12-01T14:38:44Z

examples/default/Makefile

-        microbit native nrf51dongle nrf52dk nrf6310 openmote-cc2538 pba-d-01-kw2x \
-        remote-pa remote-reva samr21-xpro \
-        spark-core telosb yunjia-nrf51822 z1
+BOARD_PROVIDES_NETIF := acd52832 airfy-beacon cc2538dk fox iotlab-m3 iotlab-a8-m3 \


Isn't possible for this board to also use the softdevice? I guess #8068 has good chances to be merged soon which brings back the softdevice compatibility.

Generally it should be possible. But to be honest, currently the automatic inclusion of the softdevice is a pain in the neck. Let's include it once fixed.

Ok, I might take care of that.

kYc0o · 2017-12-01T14:48:42Z

IMHO this board doesn't belong to nrf52BLABLA common boards. It shares only the CPU, which is already using nrf5x_common IIRC. Thus I think you should provide its own board.c along with its own board_init function.

If code reutilisation is an issue, then the common board folder is not well decoupled, which is another issue.

PeterKietzmann · 2017-12-05T09:19:11Z

@kYc0o thanks for your review. I fully understand your argumentation about separate board_init functions. Now imagine both implementation do exactly the same. Would you still argue for separate ones?

kYc0o · 2017-12-05T12:54:51Z

Now imagine both implementation do exactly the same

Well it appears that for this board the implementations are NOT the same, hence your trouble with the LEDs. I don't mind to have the "same" implementations, because they actually differ on this specific LED initialisation. Thus, IMHO, it's worth to have separated files.

If we cannot take advantage of the code de-duplication in this situation, maybe we are not decoupling it in the right way, or simply it's not possible to reuse ALL the code which seems similar (OK, without #ifdefs everywhere).

PeterKietzmann · 2017-12-05T13:42:00Z

@kaspar030 now that I thought about it again (@kYc0o as well) I still think we should go with a bit of separation (especially talking about boaprd_init). Ideally the separation between BOARD, MCU and CPU might bring the best result in terms of code de-duplication, but we decided against this concept AFAIR. As a compromise I re-used most 'Makefiles here which define features of the MCU. But the board initialization I left separately.

Did I convince you?

kYc0o

OK for me in general. ACK.

PeterKietzmann · 2017-12-05T14:19:57Z

Thanks @kYc0o. Lets wait for @kaspar030. He requested changes and started the discussion about code de-duplication

PeterKietzmann · 2017-12-19T08:04:08Z

@kaspar030 ping

PeterKietzmann · 2017-12-19T17:18:52Z

Rebased

kaspar030 · 2017-12-19T20:55:28Z

boards/acd52832/Makefile.include

@@ -0,0 +1,21 @@
+# define the cpu used by the acd52832


Only the CPU_MODEL differs from common/nrf52xxxdk/Makefile.include?

PeterKietzmann · 2017-12-20T07:27:31Z

@kaspar030 the Makefile.include file differs in the CPU_MODEL and the default port. However, including common/nrf52xxxdk/Makefile.include here requires to use the boards_common_nrf52 module which is not wanted (as discussed above).

kaspar030 · 2017-12-20T11:46:18Z

CPU_MODEL and the ports can be set before inuding the common file.

if the common file doesn't fit 100%, it can be changed.
for the board_common_nrf52 module, I suggest either excluding this board or including the others using an if clause around the two lines selecting the module.
either way, much better than having all the other cruft duplicated.

PeterKietzmann · 2017-12-20T14:00:02Z

@kaspar030 please have a look at my latest commit. You prefer this way? IMO this line is not the nicest solution but IMO unavoidable if we want to use the shared Makefile.

kaspar030 · 2017-12-20T15:35:26Z

You prefer this way?

Yes! Thanks.

PeterKietzmann · 2017-12-20T15:37:54Z

Squashed

PeterKietzmann · 2017-12-20T15:42:12Z

All lights green

kaspar030 · 2017-12-20T15:47:08Z

&go!

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

PeterKietzmann requested review from haukepetersen and smlng November 14, 2017 13:50

PeterKietzmann assigned smlng Nov 14, 2017

PeterKietzmann added Area: boards Area: Board ports Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Nov 14, 2017

PeterKietzmann mentioned this pull request Nov 14, 2017

boards: add acd52832/nrf52832 #7501

Closed

smlng approved these changes Nov 14, 2017

View reviewed changes

kaspar030 requested changes Nov 14, 2017

View reviewed changes

kaspar030 changed the title ~~boards/ACD52832: add new NRF52 based developement board~~ boards/ACD52832: add new NRF52 based development board Nov 18, 2017

kaspar030 mentioned this pull request Nov 21, 2017

boards: move shared code to boards/common/xx #8058

Merged

PeterKietzmann mentioned this pull request Nov 23, 2017

drivers: add i2c driver for nrf52 #7557

Closed

PeterKietzmann mentioned this pull request Nov 30, 2017

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

Merged

PeterKietzmann force-pushed the pr_dnahm_aconno branch 3 times, most recently from 92205b1 to 600af68 Compare December 1, 2017 08:30

PeterKietzmann added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Dec 1, 2017

kYc0o requested changes Dec 1, 2017

View reviewed changes

PeterKietzmann force-pushed the pr_dnahm_aconno branch 2 times, most recently from 3837f8f to 0c88b81 Compare December 5, 2017 13:16

kYc0o approved these changes Dec 5, 2017

View reviewed changes

PeterKietzmann removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Dec 19, 2017

PeterKietzmann force-pushed the pr_dnahm_aconno branch from 0c88b81 to 32066d1 Compare December 19, 2017 17:10

kaspar030 requested changes Dec 19, 2017

View reviewed changes

kaspar030 approved these changes Dec 20, 2017

View reviewed changes

boards/ACD52832: add new NRF52 based developement board

201d532

PeterKietzmann force-pushed the pr_dnahm_aconno branch from 1087655 to 201d532 Compare December 20, 2017 15:37

kaspar030 merged commit 9acf7c9 into RIOT-OS:master Dec 20, 2017

PeterKietzmann deleted the pr_dnahm_aconno branch December 20, 2017 15:47

PeterKietzmann mentioned this pull request Jan 5, 2018

boards: adds support for Nordic Thingy:52 #8205

Merged

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

		@@ -0,0 +1,18 @@
		# Put defined MCU peripherals here (in alphabetical order)
		FEATURES_PROVIDED += periph_cpuid

		@@ -0,0 +1,13 @@
		ifneq (,$(filter saul_default,$(USEMODULE)))

		@@ -0,0 +1,4 @@
		MODULE = board
		DIRS = $(RIOTBOARD)/common/nrf52xxxdk

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

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

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

Uh oh!

Uh oh!

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

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!