boards: add acd52832/nrf52832 · Pull Request #7501 · RIOT-OS/RIOT

ghost · 2017-08-23T11:31:53Z

This PR includes the configuration for the aconno board acd52832.

smlng · 2017-08-24T07:38:41Z

Quick note: I'd prefer to have the ADC driver and the new board configuration in separate PRs. @PeterKietzmann what to you think?

haukepetersen · 2017-08-25T07:11:20Z

+1 for splitting this PR.

haukepetersen

I have some further comments regarding the ADC driver, but would prefer to handle them in a separate PR...

haukepetersen · 2017-08-25T07:15:02Z

boards/acd52832/board.c

+  /* initialize the boards LEDs */
+  NRF_P0->DIRSET = (LED0_MASK);
+  NRF_P0->OUTSET = (LED0_MASK);
+  NRF_P0->OUTSET = (LED0_MASK);


indention is off by two spaces

also: the line above is duplicated...

@haukepetersen Taken over from boards/nrf52dk/board.c

then you can fix that file as well :-)

haukepetersen · 2017-08-25T07:15:35Z

boards/acd52832/board.c

+ * @file
+ * @brief       Board initialization for the nRF52 DK
+ *
+ * @author      Hauke Petersen <[email protected]>


author and copyright should be you? Also the brief description is wrong for this board.

haukepetersen · 2017-08-25T07:16:44Z

boards/acd52832/include/board.h

+ * @brief       Board specific configuaration for the acd52832
+ *
+ * @author      Hauke Petersen <[email protected]>
+ * @author      Sebastian Meiling <[email protected]>


haukepetersen · 2017-08-25T07:18:19Z

boards/acd52832/include/periph_conf.h

+static const timer_conf_t timer_config[] = {
+    {
+        .dev      =  NRF_TIMER1,
+        .channels =  3,


both lines above have one space char too much after the equal sign

haukepetersen · 2017-08-25T07:19:28Z

boards/acd52832/include/periph_conf.h

+        .dev  = NRF_SPI0,
+        .sclk = 4,
+        .mosi = 3,
+        .miso = 13           // NC


what is the // NC comment supposed to mean here? If it is supposed to stay, there should be a more comprehensive comment and /* */-style comment should be used.

It means: Not Connected

so not connected at all, also not to some pin header or similar? Maybe then it would make sense to set it to some undefined value and adapt the SPI driver to not initialize the pin in the first place.

haukepetersen · 2017-08-25T07:20:16Z

cpu/nrf52/periph/adc.c

+ */
+
+/**
+ * @ingroup     cpu_nrf52832


the group is cpu_nrf52.

ghost · 2017-08-25T07:29:52Z

OK, I will split it.

haukepetersen · 2017-08-25T07:53:53Z

cool, thx!

ghost · 2017-08-25T10:23:05Z

This commit includes now only the board configuration.

haukepetersen

Looking good, untested ACK

smlng · 2017-08-25T10:32:15Z

boards/acd52832/board.c

+void board_init(void)
+{
+    /* initialize the boards LEDs */
+    NRF_P0->DIRSET = (LED0_MASK);


IMO this should rather be gpio_init(LED0_PIN, GPIO_OUT);, or not?

could, but not necessarily. as the 'native' pin init is a 1-liner here, so why hassle...

smlng · 2017-08-25T10:32:48Z

boards/acd52832/board.c

+{
+    /* initialize the boards LEDs */
+    NRF_P0->DIRSET = (LED0_MASK);
+    NRF_P0->OUTSET = (LED0_MASK);


and hence this gpis_clear(LED0_PIN);, if needed at all?

actually it would be gpio_set() but could also be LED0_OFF

smlng · 2017-08-25T10:33:22Z

boards/acd52832/board.c

+    NRF_P0->OUTSET = (LED0_MASK);
+
+    /* initialize the CPU */
+    cpu_init();


most boards init cpu first, and LED(s) second but that may not matter that much, just for consistency

smlng · 2017-08-25T11:43:36Z

boards/acd52832/include/periph_conf.h

+ * @{
+ */
+#define UART_NUMOF          (1U)
+#define UART_PIN_RX         GPIO_PIN(0,30)


space after , here and below

smlng · 2017-08-25T11:45:16Z

examples/default/Makefile

-        microbit native nrf51dongle nrf52dk nrf6310 openmote-cc2538 pba-d-01-kw2x \
-        pca10000 pca10005 remote-pa remote-reva saml21-xpro samr21-xpro \
-        spark-core telosb yunjia-nrf51822 z1
+        microbit native nrf51dongle nrf52dk nrf6310 acd52832 openmote-cc2538 \


this list is lexicographically sorted, hence acd52832 should be in pole position 😄 though it might be based on NRFxy and hence related to nrf6310

smlng · 2017-10-05T08:41:10Z

please rebase, fix issue with examples/default/Makefile and squash into 1 commit. Otherwise ACK.

PeterKietzmann · 2017-11-14T13:51:09Z

Closing in favor of #8035

smlng added the Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation label Aug 24, 2017

haukepetersen added the Type: new feature The issue requests / The PR implemements a new feature for RIOT label Aug 25, 2017

haukepetersen added this to the Release 2017.10 milestone Aug 25, 2017

haukepetersen requested changes Aug 25, 2017

View reviewed changes

haukepetersen approved these changes Aug 25, 2017

View reviewed changes

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

ghost mentioned this pull request Aug 25, 2017

Add adc driver nrf52 #7515

Closed

smlng suggested changes Aug 25, 2017

View reviewed changes

ghost mentioned this pull request Sep 1, 2017

drivers: add i2c driver for nrf52 #7557

Closed

miri64 assigned haukepetersen and smlng Sep 19, 2017

This was referenced Sep 28, 2017

drivers: add driver mcp23017 #7652

Closed

tests: add test for the aconno board acd52832 #7653

Closed

boards: add ACD52832

7e916fe

PeterKietzmann mentioned this pull request Nov 14, 2017

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

Merged

PeterKietzmann closed this Nov 14, 2017

Conversation

ghost commented Aug 23, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smlng commented Aug 24, 2017

Uh oh!

haukepetersen commented Aug 25, 2017

Uh oh!

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

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!

ghost commented Aug 25, 2017

Uh oh!

haukepetersen commented Aug 25, 2017

Uh oh!

ghost commented Aug 25, 2017

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

smlng commented Oct 5, 2017

Uh oh!

PeterKietzmann commented Nov 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ghost commented Aug 23, 2017 •

edited by ghost

Loading