tests: port tests/lwip to sock by miri64 · Pull Request #6372 · RIOT-OS/RIOT

miri64 · 2017-01-15T17:48:40Z

This ports tests/lwip to sock.

Depends on ~~#5936~~, ~~#5937~~, and ~~#5938~~. Will only rebase once all of these are merged.

miri64 · 2017-01-15T17:49:43Z

Will only rebase once all of these are merged.

(because merging those three is pretty tricky due to their similarities)

miri64 · 2017-01-15T19:39:11Z

Also needs #6373 to build successfully for msba2.

PeterKietzmann · 2017-01-16T09:25:28Z

Makefile.pseudomodules

+PSEUDOMODULES += sock
+PSEUDOMODULES += sock_ip
+PSEUDOMODULES += sock_tcp
+PSEUDOMODULES += sock_udp


Already provided in lines 59-62?

miri64 · 2017-01-20T11:30:26Z

Rebased to current master and current versions of #5937 and #5938.

miri64 · 2017-01-20T13:42:52Z

Made the output a little bit more specific.

PeterKietzmann · 2017-01-24T12:24:09Z

RIOT/tests/lwip/tcp.c:79:13: error: 'client_port' may be used uninitialized in this function [-Werror=maybe-uninitialized]
             printf("Received TCP data from client [%s]:%u:\n",

miri64 · 2017-01-24T12:48:33Z

Fixed

PeterKietzmann · 2017-02-08T08:44:56Z

@miri64 please rebase. @kYc0o as you approved #5937, please test this one. I added you as assignee.

kYc0o · 2017-02-08T09:10:01Z

OK no problem ;-)

miri64 · 2017-02-08T09:11:31Z

Remember that this also needs #5938 ;-)

miri64 · 2017-02-08T09:15:37Z

Rebased to current master and #5938.

miri64 · 2017-02-08T10:24:26Z

From #5937 (comment):

On a phytec board I don't get an IPv6 address. ps() crashes the node but this is probably an issue of #6372.

The phytec board issue is due it not being included in lwIP's auto-init routine yet (which was not possible due it not being netdev2 before #6453 was merged). This is more about fixing #4672 (or, as a workaround, writing a custom initialization routine for that device in lwip_bootstrap()) than about this specific application. Might be that the crashing of ps() is related to that, but I don't have a PhyTec board at hand to test.

PeterKietzmann · 2017-02-08T12:49:06Z

Runnung on the phytecs was one issue. Crashing was an other which can also be seen on atmel boards

miri64 · 2017-02-08T12:55:56Z

Crashing was an other which can also be seen on atmel boards

Okay, will look into that.

PeterKietzmann · 2017-02-16T08:22:49Z

I didn't follow your discussion completely but yes I prefer an application+Makefile that demonstrates sock->lwip->netdev->air. And most likely it is more an "example" than a "test". Without such an application it is always a hassle to research all dependencies and stuff instead of just taking this app as a basis, when someone actually wants to make use of this port. In my opinion a general sock example would be best which switches between stacks (lwip, emb6, gnrc) via make flag or so.
I assume @smlng was referring to code duplication in the single tests tests/lwip_sock_* and not in the "demonstrator" application?

smlng · 2017-02-16T08:33:46Z

I assume @smlng was referring to code duplication in the single tests tests/lwip_sock_* and not in the "demonstrator" application?

Mostly, but tests/lwip basically includes all 3 other tests, but without auto-test ability. For the latter, I suggested its more an example -- a good one btw. and sure helpful for someone willing to use RIOT+lwip.

To summarize: I don't see the benefit of having a) 3 separated tests for lwip ip, udp, and tcp (which should be merged to avoid code duplication, and to simplify the code enhancing its readability); and b) a single test (which is more an example) that integrates all 3 but cannot be run automatically -> for instance in a future CI with nightly build + test-everything.

miri64 · 2017-02-16T09:39:42Z

To summarize: I don't see the benefit of having a) 3 separated tests for lwip ip, udp, and tcp (which should be merged to avoid code duplication, and to simplify the code enhancing its readability); and b) a single test (which is more an example) that integrates all 3 but cannot be run automatically -> for instance in a future CI with nightly build + test-everything.

I'd argue this application doesn't even do that. It provides shell commands that can automatically be run. I re-add the test script and extend it.

miri64 · 2017-02-28T20:07:15Z

Rebased.

miri64 · 2017-02-28T20:07:50Z

(and adapted the test script @smlng).

miri64 · 2017-04-25T08:16:21Z

Rebased.

kaspar030 · 2017-04-25T08:23:14Z

tests/lwip/Makefile

 # including lwip_ipv6_mld would currently break this test on at86rf2xx radios
-USEMODULE += lwip lwip_ipv6_autoconfig lwip_conn_ip lwip_netdev
-USEMODULE += lwip_udp lwip_conn_udp
+USEMODULE += lwip lwip_ipv6_autoconfig lwip_sock_ip lwip_netdev


the dependencies seem a little much. At least lwip_* should depend on lwip, right?

and lwip should depend on lwip_netdev, which should depend on netdev_default?

Please read the title of this PR. This PR is about porting the tests, not fixing dependencies for lwIP ;-)

Well, don't we have a convention that once a mess has been identified, we fix it instead of adding on top?

Primarily we want to have topical PRs so they don't grow out of hand (and the review doesn't take an eternity). Let's fix this in a separate PR after this one is merged. Please!

kaspar030 · 2017-04-25T08:26:04Z

tests/lwip/Makefile

+USEMODULE += lwip lwip_ipv6_autoconfig lwip_sock_ip lwip_netdev
+USEMODULE += lwip_udp lwip_sock_udp
+USEMODULE += lwip_tcp lwip_sock_tcp
 USEMODULE += ipv6_addr


also this dependency should be implicit (through sock?).

See #6372 (comment)

kaspar030 · 2017-04-25T08:29:09Z

tests/lwip/tcp.c

+
+#ifdef MODULE_SOCK_TCP
+static char sock_inbuf[SOCK_INBUF_SIZE];
+static bool server_running, client_running = false;


only client running will be set to false, which is probably not what you intended. Anyhow, if declared like this, the default is zero (false) anyways.

kaspar030 · 2017-04-25T08:31:10Z

tests/lwip/tcp.c

+
+    msg_init_queue(server_msg_queue, SERVER_MSG_QUEUE_SIZE);
+    /* parse port */
+    server_addr.port = (uint16_t)atoi((char *)args);


both casts seem unnecessary

kaspar030 · 2017-04-25T08:33:43Z

tests/lwip/tcp.c

+
+static int tcp_send(char *data, unsigned int num, unsigned int delay)
+{
+    uint8_t byte_data[strlen(data) / 2];


dynamic array?

How else should I do it?

(and where did we forbid dynamic arrays, especially in shell context where we know the maximum length this doesn't seem necessary)

well, for one, the shell commands might be used elsewhere (shudder). Then the problem with dynamic arrays is the dynamic stack usage. Breaks the stack depending on usage. If the maximum shell line length is allowed, allocate that much memory.

kaspar030 · 2017-04-25T08:35:56Z

tests/lwip/tcp.c

+            return 1;
+        }
+        if (argc > 3) {
+            num = (uint32_t)atoi(argv[3]);


cast needed?

kaspar030 · 2017-04-25T08:36:02Z

tests/lwip/tcp.c

+            num = (uint32_t)atoi(argv[3]);
+        }
+        if (argc > 4) {
+            delay = (uint32_t)atoi(argv[4]);


kaspar030 · 2017-04-25T08:36:25Z

tests/lwip/tests/01-run.py

@@ -1,4 +1,4 @@
-#! /usr/bin/env python
+#! /usr/bin/env python2


does this run with python3?

Now that I replaced all occurrences of pexpect.spawn() with pexpect.spawnu() it does

kaspar030 · 2017-04-25T08:38:01Z

tests/lwip/udp.c

-    ipv6_addr_t src = IPV6_ADDR_UNSPECIFIED, dst;
-    uint16_t port;
+    sock_udp_ep_t dst = SOCK_IPV6_EP_ANY;
    uint8_t byte_data[strlen(data) / 2];


dynamic array

kaspar030 · 2017-04-25T08:38:09Z

tests/lwip/udp.c

    }
    /* parse port */
-    port = (uint16_t)atoi(port_str);
+    dst.port = (uint16_t)atoi(port_str);


emmanuelsearch · 2017-04-25T09:03:38Z

+1 for trying to merge this at H&A. Would achieve fully harmonized API (sock).

miri64 · 2017-04-25T09:06:57Z

+1 for trying to merge this at H&A. Would achieve fully harmonized API (sock).

If #6012 gets merged, too.

miri64 · 2017-04-25T09:33:50Z

Addressed comments.

kaspar030

almost there

kaspar030 · 2017-04-25T10:23:16Z

tests/lwip/tcp.c

@@ -0,0 +1,224 @@
+/*
+ * Copyright (C) 2015 Freie Universität Berlin


two years old ;)

Well the PR feels like being open for so long :o) (will fix though ;))

kaspar030 · 2017-04-25T10:25:02Z

tests/lwip/tcp.c

+    }
+    else if (strcmp(argv[1], "send") == 0) {
+        uint32_t num = 1;
+        uint32_t delay = 1000000;


please ad "LU" for our beloved 16bitters

miri64 · 2017-04-25T11:14:58Z

Addressed

miri64 · 2017-04-25T12:03:44Z

Added nucleo-l053 and nucleo32-l031 to the boards with insufficient memory.

miri64 · 2017-04-25T14:05:32Z

May I squash, @kaspar030?

miri64 · 2017-04-25T16:20:03Z

Squashed (asked @kaspar030 if this is alright)

kaspar030

ACK.

miri64 · 2017-04-26T08:46:07Z

The errors in arduino_hello-world on nucleo-f070 don't seem to be related.

miri64 added Area: network Area: Networking Area: tests Area: tests and testing framework State: waiting for other PR State: The PR requires another PR to be merged first labels Jan 15, 2017

miri64 added this to the Release 2017.01 milestone Jan 15, 2017

miri64 assigned PeterKietzmann Jan 15, 2017

miri64 mentioned this pull request Jan 15, 2017

pkg: provide sock_ip support for lwip #5936

Merged

PeterKietzmann reviewed Jan 16, 2017

View reviewed changes

miri64 mentioned this pull request Jan 16, 2017

tests/lwip: changed default BOARD to native in the Makefile #5698

Closed

miri64 force-pushed the tests/enh/port-lwip-test-to-sock branch from 98a7453 to 5494725 Compare January 20, 2017 11:28

miri64 mentioned this pull request Jan 20, 2017

tests: add missing include for Makefile.tests_common #6444

Merged

PeterKietzmann mentioned this pull request Jan 25, 2017

pkg: provide sock_udp support for lwip #5937

Merged

PeterKietzmann modified the milestones: Release 2017.04, Release 2017.01 Jan 26, 2017

miri64 mentioned this pull request Jan 29, 2017

GNRC TCP: code review #6476

Closed

17 tasks

PeterKietzmann assigned kYc0o Feb 8, 2017

miri64 force-pushed the tests/enh/port-lwip-test-to-sock branch from 6e70e85 to 329b176 Compare February 8, 2017 09:15

miri64 mentioned this pull request Feb 8, 2017

pkg: provide sock_tcp support for lwip #5938

Merged

miri64 force-pushed the tests/enh/port-lwip-test-to-sock branch from 916669f to 40fd399 Compare February 28, 2017 20:07

kaspar030 modified the milestone: Release 2017.04 Apr 21, 2017

miri64 added the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label Apr 25, 2017

lwip_sock: depend on sock pseudo-modules

14ed6d2

miri64 force-pushed the tests/enh/port-lwip-test-to-sock branch from f96ef1a to 76cdab9 Compare April 25, 2017 08:16

kaspar030 requested changes Apr 25, 2017

View reviewed changes

miri64 added 2 commits April 25, 2017 18:15

tests: port tests/lwip to sock

d1c75b4

tests: adapt tests/lwip for od changes

b84f3cc

miri64 force-pushed the tests/enh/port-lwip-test-to-sock branch from 9f3b5cd to b84f3cc Compare April 25, 2017 16:19

kaspar030 approved these changes Apr 25, 2017

View reviewed changes

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 26, 2017

miri64 merged commit 4df288f into RIOT-OS:master Apr 26, 2017

miri64 deleted the tests/enh/port-lwip-test-to-sock branch April 26, 2017 12:20

		@@ -1,4 +1,4 @@
		#! /usr/bin/env python
		#! /usr/bin/env python2

		@@ -0,0 +1,224 @@
		/*
		* Copyright (C) 2015 Freie Universität Berlin

Conversation

miri64 commented Jan 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Jan 15, 2017

Uh oh!

miri64 commented Jan 15, 2017

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!

miri64 commented Jan 20, 2017

Uh oh!

miri64 commented Jan 20, 2017

Uh oh!

PeterKietzmann commented Jan 24, 2017

Uh oh!

miri64 commented Jan 24, 2017

Uh oh!

PeterKietzmann commented Feb 8, 2017

Uh oh!

kYc0o commented Feb 8, 2017

Uh oh!

miri64 commented Feb 8, 2017

Uh oh!

miri64 commented Feb 8, 2017

Uh oh!

miri64 commented Feb 8, 2017

Uh oh!

PeterKietzmann commented Feb 8, 2017

Uh oh!

miri64 commented Feb 8, 2017

Uh oh!

PeterKietzmann commented Feb 16, 2017

Uh oh!

smlng commented Feb 16, 2017

Uh oh!

miri64 commented Feb 16, 2017

Uh oh!

miri64 commented Feb 28, 2017

Uh oh!

miri64 commented Feb 28, 2017

Uh oh!

miri64 commented Apr 25, 2017

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!

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!

miri64 commented Jan 15, 2017 •

edited

Loading

miri64 commented Apr 25, 2017 •

edited

Loading