pkg: provide sock_tcp support for lwip by miri64 · Pull Request #5938 · RIOT-OS/RIOT

miri64 · 2016-10-12T23:09:05Z

This was taken out of #5802 and only provides the sock_tcp part and the tests for it.

This contains duplicate changes to #5936, so merging any of these might require rebase.

Depends on ~~#5923~~ (merged).

miri64 · 2016-10-31T12:08:41Z

Needs adaptation to #5945 edit: and #6114.

miri64 · 2016-11-05T09:28:01Z

But just rebased for now, since #5923 was merged.

kaspar030 · 2016-11-18T11:44:43Z

Not time, sorry.

miri64 · 2016-11-18T13:33:13Z

Adaption to #6137 (comment) also needed...

miri64 · 2016-11-29T19:56:43Z

Provided the same treatment to this PR as I did for #5937.

miri64 · 2017-01-13T16:07:23Z

Ping?

OlegHahm · 2017-01-13T16:49:34Z

Not WIP any more?

miri64 · 2017-01-13T17:00:57Z

Yapp.

OlegHahm · 2017-01-13T17:11:07Z

Not sure I'm the best person to review this since I haven't used sock so far.

miri64 · 2017-01-13T17:12:24Z

I'll add @smlng as our resident TCP expert ;-)

miri64 · 2017-01-13T17:14:06Z

(also I would suggest to first review and merge sock_udp or sock_ip, since it would reduce the number of lines in this PR)

miri64 · 2017-02-08T09:10:47Z

Rebased to current master

miri64 · 2017-02-08T09:28:14Z

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.

miri64 · 2017-02-08T10:23:53Z

Oops wrong PR 😄

kYc0o · 2017-02-08T13:34:51Z

I think this PR needs to be rebased since Jenkins spotted some errors about timex.h constants being changed.

miri64 · 2017-02-08T13:56:23Z

There was no change to master since my last rebase, so a rebase won't change that.

miri64 · 2017-02-08T13:57:16Z

I need however adapt this PR for #6399

kYc0o · 2017-02-08T13:58:23Z

That was what I meant.

miri64 · 2017-02-08T14:01:11Z

Done

miri64 · 2017-02-10T13:17:34Z

@smlng ping?

smlng · 2017-02-10T13:20:06Z

@miri64 sorry, busy ... However, Jenkins founds ram overflow for board nucleo32-f042 in tests/lwip_sock_tcp -> add to insuff mem list?

miri64 · 2017-02-10T13:21:44Z

it was renamed in #6417. Fixed.

smlng

See comments. In general: there is a lot of if .. else and switch .. case madness in pkg/lwip/contrib/sock/lwip_sock.c. I tried to untangle all cases, but I got lost at some places.

smlng · 2017-02-10T13:34:31Z

pkg/lwip/contrib/sock/lwip_sock.c

+#if LWIP_TCP
+    else if (tmp->type & NETCONN_TCP) {
+        err = netconn_write_partly(tmp, data, len, 0, &written);
+        if (err == ERR_OK) {


this is duplicate code, same is done below in switch

smlng · 2017-02-10T13:35:53Z

pkg/lwip/contrib/sock/lwip_sock.c

    }
+#if LWIP_TCP
+    else if (tmp->type & NETCONN_TCP) {
+        err = netconn_write_partly(tmp, data, len, 0, &written);


can't you reuse res instead of introducing an addition var with written? As far as I see, you set res = written anyway afterwards?!

Mh… this might lead to overflows, but that is also true for the current way of handling => will adapt.

smlng · 2017-02-10T13:43:50Z

pkg/lwip/contrib/sock/tcp/lwip_sock_tcp.c

+    int res;
+    assert(sock != NULL);
+    mutex_lock(&sock->mutex);
+    if (sock->conn == NULL) {


could be simplified to

int res = 0; if (!sock->conn || lwip_sock_get_addr(sock->conn, (struct _sock_tl_ep *)ep,0)) { res = -ENOTCONN; } mutex_unlock(&sock->mutex); return res;

Will do, but I prefer sock->conn == NULL over !sock->conn since it does not mix boolean operations with pointer checks

smlng · 2017-02-10T13:44:09Z

pkg/lwip/contrib/sock/tcp/lwip_sock_tcp.c

+
+    assert(queue != NULL);
+    mutex_lock(&queue->mutex);
+    if (queue->conn == NULL) {


could be simplified, see above

smlng · 2017-02-10T13:50:32Z

pkg/lwip/contrib/sock/tcp/lwip_sock_tcp.c

+                res = -1;
+                break;
+        }
+        if (res == 0) {


look like this could or should go into the ERR_OK case above?

smlng · 2017-02-10T13:51:57Z

pkg/lwip/contrib/sock/tcp/lwip_sock_tcp.c

+            return -EAGAIN;
+        }
+    }
+    else if (timeout != 0) {


so actually else should suffice here

smlng · 2017-02-10T14:04:27Z

btw. I still need to test this

miri64 · 2017-02-10T16:30:43Z

Adressed

smlng · 2017-02-13T11:22:56Z

I just tested with RIOT native and netcat from host -> success. My test app is here.

However, I think the test application needs work - lots of code duplication ...

smlng

In general the test application is quite long, which is good as it covers many cases. However, it also makes it hard to read and many functions could be merged to avoid code duplication. For instance everything exists twice for IPv4 and IPv6, and further: many function only differ only in an argument used e.g. with or w/o port defined and so on.

And I'm unsure if everything works as expected (see comments), because the subtasks do not reset all settings to start from scratch.

smlng · 2017-02-13T12:19:20Z

tests/lwip_sock_tcp/main.c

+    _server_addr.port = _TEST_PORT_REMOTE;
+    _server_addr.netif = SOCK_ADDR_ANY_NETIF;
+
+    msg_send(&msg, _server);    /* start server on _TEST_PORT_REMOTE */


I think this won't do anything, as the server is started above in L156 already and wasn't stopped in between.

The server is stopped in the tear_down() function which is called in between tests.

(see l. 69)

smlng · 2017-02-13T12:21:15Z

tests/lwip_sock_tcp/main.c

+    msg_t msg = { .type = _SERVER_MSG_START };
+
+    _server_addr.family = AF_INET;
+    _server_addr.port = _TEST_PORT_LOCAL;


Will this setting be applied, the server is already running, hence, no changes will be made to its settings and port will stay _TEST_PORT_REMOTE, or not?

miri64 · 2017-02-13T15:40:27Z

However, I think the test application needs work - lots of code duplication ...

This seems to be the general nature with these kind of unittest-like test. Furthermore, this is also true for the sock_udp and sock_ip tests. Ideally I would like to unify them anyways later on. So can we maybe move that issue to a separate PR?

smlng · 2017-02-13T16:13:02Z

@miri64 sure reworking and unifying the tests seems reasonable, but also a task (or multiple) for future PR(s). So I'm fine with postponing this, but what about my other comments, regarding (re)starting the TCP server process, which IMO does not work as it should, or am I mistaken?

miri64 · 2017-02-13T18:34:26Z

Those came after I wrote the comment, but before I hit "Comment" (which took me several hours 😊)

miri64 · 2017-02-13T18:34:44Z

Will look into them

smlng · 2017-02-13T19:07:37Z

@miri64 ah damn, oversaw the CALL makro definition. Well then, I tested and it works, so ACK -> please squash as needed and let Murdock have a final look.

BUT: we should remember (make a knot somewhere), that the tests should be made more readable and without that much code duplication.

miri64 · 2017-02-13T19:09:53Z

BUT: we should remember (make a knot somewhere), that the tests should be made more readable and without that much code duplication.

These knots are called issues in GitHub :P

miri64 · 2017-02-13T19:14:13Z

~~Rebased~~ squashed

miri64 · 2017-02-13T19:16:27Z

These knots are called issues in GitHub :P

See #6596

miri64 added Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: pkg Area: External package ports State: waiting for other PR State: The PR requires another PR to be merged first labels Oct 12, 2016

miri64 assigned kaspar030 Oct 12, 2016

This was referenced Oct 12, 2016

pkg: provide sock support for lwIP #5802

Closed

gnrc_tcp: initial implementation #4744

Merged

sock: change IPv4 address type to array #5945

Merged

miri64 force-pushed the pkg/feat/lwip-sock-tcp branch from 9e404f6 to 6aeaf16 Compare November 5, 2016 09:27

miri64 added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Nov 5, 2016

kaspar030 assigned OlegHahm and unassigned kaspar030 Nov 18, 2016

miri64 force-pushed the pkg/feat/lwip-sock-tcp branch from 6aeaf16 to 2cc62bd Compare November 29, 2016 19:56

miri64 mentioned this pull request Jan 13, 2017

posix_sockets: port to sock #6004

Merged

miri64 removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Jan 13, 2017

miri64 assigned smlng Jan 13, 2017

This was referenced Jan 15, 2017

pkg: provide sock_ip support for lwip #5936

Merged

tests: port tests/lwip to sock #6372

Merged

smlng suggested changes Feb 10, 2017

View reviewed changes

smlng suggested changes Feb 13, 2017

View reviewed changes

smlng approved these changes Feb 13, 2017

View reviewed changes

pkg: provide sock_tcp support for lwip

b915d15

miri64 force-pushed the pkg/feat/lwip-sock-tcp branch from 0165af7 to b915d15 Compare February 13, 2017 19:12

miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 13, 2017

miri64 merged commit c0b9b0f into RIOT-OS:master Feb 13, 2017

miri64 deleted the pkg/feat/lwip-sock-tcp branch February 13, 2017 20:45

Conversation

miri64 commented Oct 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Oct 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Nov 5, 2016

Uh oh!

kaspar030 commented Nov 18, 2016

Uh oh!

miri64 commented Nov 18, 2016

Uh oh!

miri64 commented Nov 29, 2016

Uh oh!

miri64 commented Jan 13, 2017

Uh oh!

OlegHahm commented Jan 13, 2017

Uh oh!

miri64 commented Jan 13, 2017

Uh oh!

OlegHahm commented Jan 13, 2017

Uh oh!

miri64 commented Jan 13, 2017

Uh oh!

miri64 commented Jan 13, 2017

Uh oh!

miri64 commented Feb 8, 2017

Uh oh!

miri64 commented Feb 8, 2017

Uh oh!

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

kYc0o commented Feb 8, 2017

Uh oh!

miri64 commented Feb 8, 2017

Uh oh!

miri64 commented Feb 10, 2017

Uh oh!

smlng commented Feb 10, 2017

Uh oh!

miri64 commented Feb 10, 2017

Uh oh!

smlng 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 Feb 10, 2017

Uh oh!

miri64 commented Feb 10, 2017

Uh oh!

smlng commented Feb 13, 2017

Uh oh!

smlng 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

miri64 commented Oct 12, 2016 •

edited

Loading

miri64 commented Oct 31, 2016 •

edited

Loading

miri64 commented Feb 13, 2017 •

edited

Loading