[riot-notifications] [RIOT-OS/RIOT] pkg/openwsn: re-integrate the network stack as a package (#13824)

Kaspar Schleiser notifications at github.com
Mon May 18 17:39:16 CEST 2020


@kaspar030 requested changes on this pull request.

First round from my side. Mostly cosmetics.

- integration looks OK. I'd prefer a fork instead of patches, but that's controversial and thus up to you.
- the test applications are not really in shape, I guess they've been copied from OpenWSN

> @@ -0,0 +1,61 @@
+PKG_NAME=openwsn-fw
+PKG_URL=https://github.com/openwsn-berkeley/openwsn-fw.git
+PKG_VERSION=a10e292aec8702e612074b4cd3c81ddd7960a279
+PKG_BUILDDIR ?= $(BINDIRBASE)/pkg/$(BOARD)/$(PKG_NAME)

not needed, it is same as the default?

> +PSEUDOMODULES += openwsn_ledpins
+PSEUDOMODULES += openwsn_sctimer
+PSEUDOMODULES += openwsn_cryptoengine
+PSEUDOMODULES += openwsn_radio
+
+# In OpenWSN the ISR stack is shared with the network stack. OpenWSN stack is
+# 2Kb, this means that the ISR stack in OpenWSN might have up to 2Kb available.
+# To keep the same marging increase the ISR stack to 2Kb as well. In practice
+# 1Kb should be enough.
+CFLAGS += -DISR_STACKSIZE=2048
+
+# at86rf2xx state machine is in enhanced mode by default, OpenWSN requires
+# basic mode.
+ifneq (,$(filter at86rf2xx,$(USEMODULE)))
+    CFLAGS += -DAT86RF2XX_BASIC_MODE
+endif

newline missing

> @@ -0,0 +1,11 @@
+CFLAGS += -Wno-implicit-fallthrough

list could be sorted

> +
+#include <stdio.h>
+
+#include "opendefs.h"
+
+#include "crypto/ciphers.h"
+#include "crypto/modes/ecb.h"
+#include "crypto/modes/ccm.h"
+
+owerror_t cryptoengine_aes_ccms_enc(uint8_t *a, uint8_t len_a, uint8_t *m,
+                                    uint8_t *len_m, uint8_t *nonce, uint8_t l,
+                                    uint8_t *key, uint8_t len_mac)
+{
+    cipher_t cipher;
+    int ret, len;
+    uint8_t tmp_buff[*len_m + len_mac];

Here and below: is there a way to avoid VLA's? IMO this is error-prone. Stuff works in testing, then in the real world(tm), someone starts decrypting network packets (with attacker supplied length), and it breaks.
IMO, a MAX_MESSAGE_SIZE define, size check and static array is preferrable.

> +        memset(&configuration, GPIO_UNDEF, sizeof(debugpins_config_t));
+
+        if (user_config != NULL) {
+            memcpy(&configuration, user_config, sizeof(debugpins_config_t));
+            debugpins_init();
+        }
+    }
+    else {
+        (void) user_config;
+    }
+}
+
+void debugpins_init(void)
+{
+    if (IS_USED(MODULE_OPENWSN_DEBUGPINS)) {
+        gpio_init(configuration.frame, GPIO_OUT);

do these need to be checked against '==GPIO_UNDEF'?

> +        gpio_init(configuration.task, GPIO_OUT);
+        gpio_init(configuration.isr, GPIO_OUT);
+        gpio_init(configuration.radio, GPIO_OUT);
+
+        debugpins_frame_clr();
+        debugpins_slot_clr();
+        debugpins_fsm_clr();
+        debugpins_task_clr();
+        debugpins_isr_clr();
+        debugpins_radio_clr();
+    }
+}
+
+void debugpins_frame_toggle(void)
+{
+    if (IS_USED(MODULE_OPENWSN_DEBUGPINS)) {

Maybe save some lines by defining and using sth like this (also for clear and toggle):

```
static _set_checked(gpio_t pin) {
  if (IS_USED(MODULE_OPENWSN_DEBUGPINS)) {
   if (pin != GPIO_UNDEF){
    gpio_set(pin);
    }
  }
}
```


> +#include "net/ieee802154.h"
+
+extern openwsn_radio_t openwsn_radio;
+
+void eui64_get(uint8_t *addressToWrite)
+{
+    eui64_t eui64;
+
+    if (openwsn_radio.dev->driver->get(openwsn_radio.dev, NETOPT_ADDRESS_LONG,
+                                       &eui64,
+                                       sizeof(eui64_t)) == sizeof(eui64_t)) {
+        memcpy(addressToWrite, eui64.uint8, sizeof(eui64.uint8));
+    }
+    else {
+        /* Use your code as a fallback or print an error message here */
+        luid_get((void *)addressToWrite, IEEE802154_LONG_ADDRESS_LEN);

```suggestion
        luid_get(addressToWrite, IEEE802154_LONG_ADDRESS_LEN);
```

> + * @brief       RIOT adaption of the "debugpins" bsp module
+ *
+ * @author      Michael Frey <michael.frey at msasafety.com>
+ * @author      Peter Kietzmann <peter.kietzmann at haw-hamburg.de>
+ *
+ * @}
+ */
+
+#include "debugpins.h"
+#include "openwsn_debugpins.h"
+
+#include <stdint.h>
+#include <string.h>
+
+/* holds the internal configuration for debugpins */
+static debugpins_config_t configuration = {

here and same for leds: better namespace (`_configuration`).

> +
+#include "openwsn.h"
+#include "openwsn_board.h"
+#include "openwsn_radio.h"
+
+#ifdef MODULE_AT86RF2XX
+#include "at86rf2xx.h"
+#include "at86rf2xx_params.h"
+#endif
+
+#define LOG_LEVEL LOG_NONE
+#include "log.h"
+
+#define OPENWSN_SCHED_NAME            "openwsn"
+#define OPENWSN_SCHED_PRIO            (THREAD_PRIORITY_MAIN - 4)
+#define OPENWSN_SCHED_STACKSIZE       (2048)

maybe use `THREAD_STACKSIZE_LARGE`?

> + *
+ * This file is subject to the terms and conditions of the GNU Lesser
+ * General Public License v2.1. See the file LICENSE in the top level
+ * directory for more details.
+ */
+
+/**
+ * @ingroup     pkg_openwsn
+ * @{
+ *
+ * @file
+ * @brief       RIOT adaption of the "sctimer" bsp module
+ *
+ * The `sctimer` ("single compare timer") in OpenWSN is the lowest timer
+ * abstraction which is used by the higher layer timer module `opentimers`. In
+ * the end t is responsible for scheduling on the MAC layer. To enable low power

```suggestion
 * the end it is responsible for scheduling on the MAC layer. To enable low power
```

> + * @file
+ * @brief       RIOT adaption of the "sctimer" bsp module
+ *
+ * The `sctimer` ("single compare timer") in OpenWSN is the lowest timer
+ * abstraction which is used by the higher layer timer module `opentimers`. In
+ * the end t is responsible for scheduling on the MAC layer. To enable low power
+ * energy modes, this timer usually uses the RTC (real time clock) or RTT (real
+ * time timer) module.
+ *
+ * In order to get the most portable code, this implementation uses ztimer and
+ * defines a new `ztimer_clock` (ZTIMER_32768) that operates at 32768Khz to have
+ * a resolution of ~30usec/tick (same as OpenWSN).
+ *
+ * When available ZTIMER_32768 will be built on top of `periph_rtt` to get low
+ * power capabilities. If not it will be built on top of a regular timer. In
+ * either case it will be shifted up if the base frequency y lower than 32768Hz

```suggestion
 * either case it will be shifted up if the base frequency is lower than 32768Hz
```

> + * OpenWSN uses source routing. This means that unless the recipient of a packet
+ * is one of the parents in the RPL tree the packet will have to go up the tree
+ * upto the rootnode. But in OpenWSN RPL implementation the node does not know
+ * how to route, instead it is `OpenVisualizer` which generates an SRH, attaches
+ * to the incoming packet and sends it down the tree.
+ *
+ * ## Hardware abstraction implementation
+ *
+ * Following, we share some insights about the implementation of selected
+ * hardware modules.
+ *
+ * ### sctimer
+ *
+ * The `sctimer` ("single compare timer") in OpenWSN is the lowest timer
+ * abstraction which is used by the higher layer timer module `opentimers`. In
+ * the end t is responsible for scheduling on the MAC layer. To enable low power

```suggestion
 * the end it is responsible for scheduling on the MAC layer. To enable low power
```

> + * a resolution of ~30usec/tick (same as OpenWSN).
+ *
+ * When available ZTIMER_32768 will be built on top of `periph_rtt` to get low
+ * power capabilities. If not it will be built on top of a regular timer. In
+ * either case it will be shifted up if the base frequency y lower than 32768Hz
+ * or frac if higher.
+ *
+ * The `sctimer` is responsible to set the next interrupt. Under circumstances,
+ * it may happen, that the next interrupt to schedule is already late, compared
+ * to the current time. In this case, timer implementations in OpenWSN directly
+ * trigger a hardware interrupt. Until able to trigger sw isr directly we set
+ * the callback 0 ticks in the future, which internally will be set to `now + 2`.
+ *
+ * ### radio
+ *
+ * The radio adaptation runs in an own thread with the highest priority

```suggestion
 * The radio adaptation runs in its own thread with the highest priority
```

> + * trigger a hardware interrupt. Until able to trigger sw isr directly we set
+ * the callback 0 ticks in the future, which internally will be set to `now + 2`.
+ *
+ * ### radio
+ *
+ * The radio adaptation runs in an own thread with the highest priority
+ * (`THREAD_PRIORITY_MAIN - 4`) and maps to RIOT's `netdev` API.
+ *
+ * Hardware MAC layer features such as CSMA/CA, ACK handling and retransmissions
+ * are handled by OpenWSN, so the radio driver must support disabling AUTOACK
+ * and CSMA handling by the hardware. Frame filtering must as well be disabled.
+ *
+ * The radio adaptation preloads the buffer so `NETOPT_PRELOADING` must be
+ * supported.
+ *
+ * OpenWSN needs to be notified when a frame reception/transmition stats and

```suggestion
 * OpenWSN needs to be notified when a frame reception/transmition starts and
```

> + * doesn't get notified about these frames), but it might print the following
+ * error if using `openwsn_serial`:
+ *
+ * - `[IEEE802154E] wdDataDuration overflows while at state 19 in slotOffset 0`
+ *
+ * ### uart
+ *
+ * In RIOT, the first configured UART device is mapped to STDIO in most cases.
+ * In OpenWSN however, the `openserial` tool uses UART to feed external software
+ * running on a host computer such as
+ * [Openvisualizer](https://github.com/openwsn-berkeley/openvisualizer).
+ * To enable use of these tools, we provide a UART adaptation.
+ *
+ * By default when `openwsn_serial` (`openserial`) is used STDIO will be disabled
+ * (it will use `stdio_null`). When multiple uart are available STDIO and `openserial`
+ * can be used in parallel..

```suggestion
 * can be used in parallel.
```

> + * - `[IEEE802154E] wdDataDuration overflows while at state 19 in slotOffset 0`
+ *
+ * ### uart
+ *
+ * In RIOT, the first configured UART device is mapped to STDIO in most cases.
+ * In OpenWSN however, the `openserial` tool uses UART to feed external software
+ * running on a host computer such as
+ * [Openvisualizer](https://github.com/openwsn-berkeley/openvisualizer).
+ * To enable use of these tools, we provide a UART adaptation.
+ *
+ * By default when `openwsn_serial` (`openserial`) is used STDIO will be disabled
+ * (it will use `stdio_null`). When multiple uart are available STDIO and `openserial`
+ * can be used in parallel..
+ *
+ * OpenWSN uart abstraction makes use of tx hardware interrupts to execute a
+ * previously registered callback after every byte is sent out. This interrupts

```suggestion
 * previously registered callback after every byte is sent out. These interrupts
```

> + * - To explore the channel hopping mechanism there are rather expensive
+ * multi-channel sniffers such as the BeamLogic 802.15.4 Site Analyzer that can
+ * sniff all channels simultaneously. Alternatively you can set up multiple
+ * separate sniffer devices locally or make use of the `sniffer_aggregator` on
+ * the IoT-LAB testbed.
+ * - There is a collection of external tools to interact with the IoT nodes from
+ * a host computer via `openserial`. Please refer to the
+ * [OpenWSN software](https://github.com/openwsn-berkeley/openwsn-sw) repository
+ * for further information.
+ *
+ * ## Todos
+ *
+ * - `sctimer` to trigger an ISR immediately using sw isr.
+ * - `RTT_FREQUENCY` is not configurable for most platforms, implementations
+ *   should be adapted to make this configurable to be able to set RTT_FREQUENCY
+ *   to the highest possible value.

32768 would be preferable to "highest possible value", no?

> + *
+ * - `sctimer` to trigger an ISR immediately using sw isr.
+ * - `RTT_FREQUENCY` is not configurable for most platforms, implementations
+ *   should be adapted to make this configurable to be able to set RTT_FREQUENCY
+ *   to the highest possible value.
+ * - The UART wrapper uses a peripheral timer to fake an interrupt after one byte
+ *   has been sent. this should also be done with sw isr.
+ *
+ * ## Future Steps
+ *
+ * OpenWSN are working on refactoring their code base. As one of the outputs of
+ * this modules like `cjoin`, `udp`, `coap` will become optional. Once this is
+ * upstream the support for this pkg should be adapted.
+ *
+ * #8570 successors managed to isolate the mac layer. The required changes have
+ * not been included in this PR. Mainly because This PR intends on supporting all

```suggestion
 * not been included in this PR. Mainly because this PR intends to support all
```

> @@ -0,0 +1,72 @@
+/**

I assume some of these tests are taken from OpenWSN? Most are missing RIOTized license headers.

> +   puts("isr pins");
+   debugpins_isr_set();      xtimer_usleep(DELAY);
+   debugpins_isr_toggle();   xtimer_usleep(DELAY);
+   debugpins_isr_toggle();   xtimer_usleep(DELAY);
+   debugpins_isr_clr();      xtimer_usleep(DELAY);
+
+   puts("radio pins");
+   debugpins_radio_set();    xtimer_usleep(DELAY);
+   debugpins_radio_toggle(); xtimer_usleep(DELAY);
+   debugpins_radio_toggle(); xtimer_usleep(DELAY);
+   debugpins_radio_clr();    xtimer_usleep(DELAY);
+
+   board_reset();
+
+   return 0;
+}

newline

> +int main(void) {
+
+   uint8_t eui[8];
+
+   printf("Get euid now\n");
+
+   for(int j = 0; j<10;j++) {
+    eui64_get(eui);
+     for (int i=0; i<8; i++){
+      printf("%x:", eui[i]);
+     }
+     printf("\n");
+     memset(eui, 0, 8);
+   }
+   printf("\nEnd main\n");
+ }

newline and maybe uncrustify

> @@ -0,0 +1,46 @@
+#include "stdint.h"
+#include "string.h"
+
+#include "ztimer.h"
+
+#include "board.h"
+#include "debugpins.h"
+#include "sctimer.h"
+
+
+#define REPETITIONS 100
+
+uint32_t cnt = 0;

these should be static

> @@ -0,0 +1,16 @@
+APPLICATION = openwsn_sctimer

this forces a name clash with the actual openwsn module. I think the default should be ok (removing that line would default to `test_openwsn_sctimer`).

> @@ -0,0 +1,20 @@
+APPLICATION = openwsn_serial

name clash

> @@ -0,0 +1,16 @@
+APPLICATION = openwsn_uart

name clash

> +}
+
+uint8_t cb_uartRxCb(void) {
+   uint8_t byte;
+   
+   // toggle LED
+   leds_error_toggle();
+   
+   // read received byte
+   byte = uart_readByte();
+   
+   // echo that byte over serial
+   uart_writeByte(byte);
+   
+   return 0;
+}

newline

-- 
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/13824#pullrequestreview-413648963
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20200518/d3c9f8fe/attachment-0001.htm>


More information about the notifications mailing list