[riot-notifications] [RIOT-OS/RIOT] cpu/nrf52/radio: initial support for nrf52's ieee802.15.4 radio (#10268)

Hauke Petersen notifications at github.com
Wed Feb 20 11:10:30 CET 2019


haukepetersen requested changes on this pull request.

Awesome progress, nice work!

Just did a rough code review with some things that caught my eye, nothing too serious.

> @@ -1,3 +1,5 @@
+FEATURES_REQUIRED += periph_timer

this should be only required if the radio (`nrf802154`) is actually compiled in, right?!

> @@ -36,10 +36,17 @@ static const timer_conf_t timer_config[] = {
         .channels = 3,
         .bitmode  = TIMER_BITMODE_BITMODE_32Bit,
         .irqn     = TIMER1_IRQn
+    },
+    {
+        .dev      = NRF_TIMER2,
+        .channels = 3,
+        .bitmode  = TIMER_BITMODE_BITMODE_08Bit,
+        .irqn     = TIMER2_IRQn

Not sure how we best do it, but we should
- document somewhere, that the radio depends on a dedicated hardware timer with the above configuration
- make the actual timer (TIMER_DEV(x)) configurable (overridable) by the board


> +        NRF_RADIO->TXPOWER = RADIO_TXPOWER_TXPOWER_Neg16dBm;
+    }
+    else if (txpower > -21) {
+        NRF_RADIO->TXPOWER = RADIO_TXPOWER_TXPOWER_Neg20dBm;
+    }
+    else {
+        NRF_RADIO->TXPOWER = RADIO_TXPOWER_TXPOWER_Neg40dBm;
+    }
+}
+
+static void _timer_cb(void *arg, int chan)
+{
+    (void)arg;
+    (void)chan;
+    mutex_unlock(&txlock);
+    timer_stop(TIMER_DEV(1));

as stated above, the timer device should probably be configurable, possibly using `TIMER_DEV(1)` as default value

> +static void _timer_cb(void *arg, int chan)
+{
+    (void)arg;
+    (void)chan;
+    mutex_unlock(&txlock);
+    timer_stop(TIMER_DEV(1));
+    timer_clear(TIMER_DEV(1), 0);
+}
+
+static int _init(netdev_t *dev)
+{
+    assert(dev);
+    (void)dev;
+
+    if (timer_init(TIMER_DEV(1), 250000UL, _timer_cb, NULL) < 0) {
+        DEBUG("Timer Init failed\n");

This would be a good place to use an `assert` :-)

> +            mutex_unlock(&txlock);
+            return -EOVERFLOW;
+        }
+        memcpy(&txbuf[len + 1], iolist->iol_base, iolist->iol_len);
+        len += iolist->iol_len;
+    }
+
+    /* specify the length of the package. */
+    txbuf[0] = len + IEEE802154_FCS_LEN;
+
+    /* trigger the actual transmission */
+    _enable_tx();
+    DEBUG("[nrf802154] send: putting %i byte into the ether\n", len);
+
+    /* set interframe spacing based on packet size */
+    if ((unsigned int)len > NRF_SIFS_MAXPKTSIZE)

to prevent you adding the missing parenthesis, how about 
```c
unsigned ifs = (len > NRF_SIFS_MAXPKTSIZE) ? NRF_LIFS : NRF_SIFS;
timer_set_absolute(TIMER_DEV(1), 0, ifs);
```
:-)

> +
+    return 0;
+}
+
+static int _send(netdev_t *dev,  const iolist_t *iolist)
+{
+    (void)dev;
+
+    DEBUG("[nrf802154] Send a packet\n");
+
+    assert(iolist);
+
+    mutex_lock(&txlock);
+
+    /* copy packet data into the transmit buffer */
+    int len = 0;

and any reason we don't simply make `len` `unsigned`? Would save some casting below and the value is always >0 anyway, right?

> @@ -342,6 +342,13 @@ ifneq (,$(filter my9221,$(USEMODULE)))
   USEMODULE += xtimer
 endif
 
+ifneq (,$(filter nrf802154,$(USEMODULE)))

>From a structural point of view, I would move this dependency into the CPUs Makefile.dep. On paper this is of course a driver, but it is completely CPU related and none of its code resides in the `/driver` code path, so I'd say its dependency defs should also not reside here.

> @@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2016 Freie Universität Berlin
+ * Copyright (C) 2018 Freie Universität Berlin

rather `2016,2018`?

> @@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2015-16 Freie Universität Berlin
+ * Copyright (C) 2018 Freie Universität Berlin

see comment above

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/10268#pullrequestreview-205663924
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190220/646ce09b/attachment.html>


More information about the notifications mailing list