[riot-notifications] [RIOT-OS/RIOT] gnrc_tcp: test improvement (#11930)

Martine Lenders notifications at github.com
Mon Sep 23 13:48:06 CEST 2019


miri64 requested changes on this pull request.

Mostly, there is nothing to add. The test does not work on a non-native board due to some misconfiguration though (see my comment in the application's Makefile). `uncrustify` has the following to say about the application's Makefile:

```diff
diff --git a/tests/gnrc_tcp/main.c b/tests/gnrc_tcp/main.c
index 8a40a19dca..a2eb980f74 100644
--- a/tests/gnrc_tcp/main.c
+++ b/tests/gnrc_tcp/main.c
@@ -35,7 +35,8 @@ int get_af_family(char *family)
 {
     if (memcmp(family, "AF_INET6", sizeof("AF_INET6")) == 0) {
         return AF_INET6;
-    } else if (memcmp(family, "AF_INET", sizeof("AF_INET")) == 0) {
+    }
+    else if (memcmp(family, "AF_INET", sizeof("AF_INET")) == 0) {
         return AF_INET;
     }
     return AF_UNSPEC;
@@ -105,7 +106,8 @@ int gnrc_tcp_open_active_cmd(int argc, char **argv)
     uint16_t target_port = atol(argv[3]);
     uint16_t local_port = atol(argv[4]);
 
-    int err = gnrc_tcp_open_active(&tcb, af_family, target_addr, target_port, local_port);
+    int err = gnrc_tcp_open_active(&tcb, af_family, target_addr, target_port,
+                                   local_port);
     switch (err) {
         case -EAFNOSUPPORT:
             printf("%s: returns -EAFNOSUPPORT\n", argv[0]);
@@ -150,7 +152,8 @@ int gnrc_tcp_open_passive_cmd(int argc, char **argv)
 
     if (argc == 3) {
         local_port = atol(argv[2]);
-    } else if (argc == 4) {
+    }
+    else if (argc == 4) {
         local_addr = argv[2];
         local_port = atol(argv[3]);
     }
@@ -209,7 +212,7 @@ int gnrc_tcp_send_cmd(int argc, char **argv)
         sent += ret;
     }
 
-    printf("%s: sent %u\n", argv[0], (unsigned) sent);
+    printf("%s: sent %u\n", argv[0], (unsigned)sent);
     return sent;
 }
 
@@ -222,7 +225,8 @@ int gnrc_tcp_recv_cmd(int argc, char **argv)
     size_t rcvd = 0;
 
     while (rcvd < to_receive) {
-        int ret = gnrc_tcp_recv(&tcb, buffer + rcvd, to_receive - rcvd, timeout);
+        int ret =
+            gnrc_tcp_recv(&tcb, buffer + rcvd, to_receive - rcvd, timeout);
         switch (ret) {
             case -EAGAIN:
                 printf("%s: returns -EAGAIN\n", argv[0]);
@@ -247,7 +251,7 @@ int gnrc_tcp_recv_cmd(int argc, char **argv)
         rcvd += ret;
     }
 
-    printf("%s: received %u\n", argv[0], (unsigned) rcvd);
+    printf("%s: received %u\n", argv[0], (unsigned)rcvd);
     return 0;
 }
 
@@ -267,18 +271,25 @@ int gnrc_tcp_abort_cmd(int argc, char **argv)
 
 /* Exporting GNRC TCP Api to for shell usage */
 static const shell_command_t shell_commands[] = {
-    {"gnrc_tcp_tcb_init", "gnrc_tcp: init tcb", gnrc_tcp_tcb_init_cmd},
-    {"gnrc_tcp_open_active", "gnrc_tcp: open active connection", gnrc_tcp_open_active_cmd},
-    {"gnrc_tcp_open_passive", "gnrc_tcp: open passive connection", gnrc_tcp_open_passive_cmd},
-    {"gnrc_tcp_send", "gnrc_tcp: send data to connected peer", gnrc_tcp_send_cmd},
-    {"gnrc_tcp_recv", "gnrc_tcp: recv data from connected peer", gnrc_tcp_recv_cmd},
-    {"gnrc_tcp_close", "gnrc_tcp: close connection gracefully", gnrc_tcp_close_cmd},
-    {"gnrc_tcp_abort", "gnrc_tcp: close connection forcefully", gnrc_tcp_abort_cmd},
-    {"buffer_init", "init internal buffer", buffer_init_cmd},
-    {"buffer_get_max_size", "get max size of internal buffer", buffer_get_max_size_cmd},
-    {"buffer_write", "write data into internal buffer", buffer_write_cmd},
-    {"buffer_read", "read data from internal buffer", buffer_read_cmd},
-    {NULL, NULL, NULL}
+    { "gnrc_tcp_tcb_init", "gnrc_tcp: init tcb", gnrc_tcp_tcb_init_cmd },
+    { "gnrc_tcp_open_active", "gnrc_tcp: open active connection",
+      gnrc_tcp_open_active_cmd },
+    { "gnrc_tcp_open_passive", "gnrc_tcp: open passive connection",
+      gnrc_tcp_open_passive_cmd },
+    { "gnrc_tcp_send", "gnrc_tcp: send data to connected peer",
+      gnrc_tcp_send_cmd },
+    { "gnrc_tcp_recv", "gnrc_tcp: recv data from connected peer",
+      gnrc_tcp_recv_cmd },
+    { "gnrc_tcp_close", "gnrc_tcp: close connection gracefully",
+      gnrc_tcp_close_cmd },
+    { "gnrc_tcp_abort", "gnrc_tcp: close connection forcefully",
+      gnrc_tcp_abort_cmd },
+    { "buffer_init", "init internal buffer", buffer_init_cmd },
+    { "buffer_get_max_size", "get max size of internal buffer",
+      buffer_get_max_size_cmd },
+    { "buffer_write", "write data into internal buffer", buffer_write_cmd },
+    { "buffer_read", "read data from internal buffer", buffer_read_cmd },
+    { NULL, NULL, NULL }
 };
 
 int main(void)
```

Also, there is a last nit on documentation. Other than those three things, I think we are ready to merge this

> +Setup
+==========
+The test requires a tap-device setup. This can be achieved by running 'dist/tools/tapsetup/tapsetup'
+or by executing the following commands:
+
+    sudo ip tuntap add tap0 mode tap user ${USER}
+    sudo ip link set tap0 up
+
+Usage (native)
+==========
+
+    make all test
+
+Usage (non-native)
+==========
+    make BOARD=<BOARD_NAME> all flash test

```markdown
Usage
=====
    make BOARD="<board name>" flash test
```

should suffice for both cases, non-native and native.

> +                             nucleo-f072rb nucleo-f303k8 nucleo-f334r8 nucleo-l031k6 \
+                             nucleo-l053r8 mega-xplained waspmote-pro wsn430-v1_3b wsn430-v1_4 \
+                             yunjia-nrf51822 z1 thingy52
+
+# unsupported by ethos: chronos, hamilton, ruuvitag
+BOARD_BLACKLIST := chronos hamilton ruuvitag
+
+# This test depends on tap device setup (only allowed by root)
+# Suppress test execution to avoid CI errors
+TEST_ON_CI_BLACKLIST += all
+
+CFLAGS += -DSHELL_NO_ECHO
+CFLAGS += -DGNRC_TCP_MSL=$(MSL_US)
+CFLAGS += -DGNRC_TCP_CONNECTION_TIMEOUT_DURATION=$(TIMEOUT_US)
+
+USEMODULE += gnrc_netdev_default

```suggestion
ifeq (native,$(BOARD))
  USEMODULE += netdev_tap
  TERMFLAGS ?= $(TAP)
else
  USEMODULE += stdio_ethos

  ETHOS_BAUDRATE ?= 115200
  CFLAGS += -DETHOS_BAUDRATE=$(ETHOS_BAUDRATE)
  TERMDEPS += ethos
  TERMPROG ?= sudo $(RIOTTOOLS)/ethos/ethos
  TERMFLAGS ?= $(TAP) $(PORT) $(ETHOS_BAUDRATE)
endif
```

(And of course remove the `ifeq (,$(filter native,$(BOARD)))` block below). Otherwise you pull in the default network device for the device, which

a) wastes memory since it is not used
b) might block `ethos` from initialization (and having it run into [an assertion](https://github.com/RIOT-OS/RIOT/blob/abdebaa86f375a567a4735acd001e758f131140d/sys/net/gnrc/netif/gnrc_netif.c#L64), since `GNRC_NETIF_NUMOF == 1`

-- 
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/11930#pullrequestreview-291697317
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190923/c048364a/attachment.htm>


More information about the notifications mailing list