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

miri64 · 2016-10-12T23:04:30Z

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

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

Needs adaptation to #5945.

kaspar030 · 2016-11-18T11:46:01Z

Reassigning.

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

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

miri64 · 2016-11-29T19:48:37Z

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

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

Ping?

miri64 · 2017-01-13T17:02:50Z

This one isn't WIP anymore either.

PeterKietzmann

@miri64 could you please adapt tests/lwip to use the introduced sock port? It is needed anyway and makes testing much easier. Besides, the code looks fine (it is too much to go into all details) and your provided tests succeed which is great :-)

PeterKietzmann · 2017-01-14T11:41:50Z

pkg/lwip/contrib/sock/lwip_sock.c

+#error "lwip_sock needs IPv4 or IPv6 support"
+#endif
+
+static inline bool _af_not_supported(int af)


I don't understand this function on a first sight.

Will add some doc.

PeterKietzmann · 2017-01-14T11:46:53Z

pkg/lwip/contrib/sock/lwip_sock.c

+    if ((*out = netconn_new_with_proto_and_callback(type, proto, NULL)) == NULL) {
+        return -ENOMEM;
+    }
+#if SO_REUSE


Strange flag name. Is it lwip specific?

This comes from lwIP. So I won't rename it.

PeterKietzmann · 2017-01-14T12:13:58Z

tests/lwip_sock_ip/README.md

+To test both set the `LWIP_IPV4` and `LWIP_IPV6` to a non-zero value:
+
+```sh
+LWIP_IPV4=1 LWIP_IPV6 make all test


LWIP_IPV6=1

miri64 · 2017-01-14T14:49:40Z

@miri64 could you please adapt tests/lwip to use the introduced sock port? It is needed anyway and makes testing much easier.

I plan to do this after these are merged. Since the different flavors of sock are divided up into multiple PRs this makes porting in all the different PRs much harder.

PeterKietzmann · 2017-01-14T16:26:12Z

@miri64 that makes totally sense! However, I assume you've already tested the implementation with a networking application, right? Most likely the application code will look like described here. But can you share the Makefile here (and in the other sock PRs)? That would make testing a lot easier.

miri64 · 2017-01-15T13:25:33Z

@miri64 that makes totally sense! However, I assume you've already tested the implementation with a networking application, right? Most likely the application code will look like described here. But can you share the Makefile here (and in the other sock PRs)? That would make testing a lot easier.

Nope, not yet. The assumption basically is so far

networking with lwIP works
interfacing lwIP with sock works as shown by the test applications

=> networking over sock works.

miri64 · 2017-01-15T13:29:12Z

Addressed comments

PeterKietzmann · 2017-01-15T15:10:49Z

Hmm. I'm not happy with that answer. Now I have to write it by myself, find out module names and stuff, which costs time.

miri64 · 2017-01-15T15:24:51Z

Hmm. I'm not happy with that answer. Now I have to write it by myself, find out module names and stuff, which costs time.

Just to give you a heads-up if you really want to do this: it's a little bit more then that. You need to initialize the socks in code, too. Just adding some modules won't cut it.

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

@PeterKietzmann see #6372

PeterKietzmann · 2017-01-16T09:34:10Z

Thanks @miri64 that was really helpful. With#6372 I was able to send IP (as well as UDP) between native nodes and also Atmel boards. Guess I have to scan the code again before a merge.

PeterKietzmann · 2017-01-16T12:17:11Z

Ok I think I'd like to merge this soon. But Jenikns states some errors. We can also have a look at Murdock but for that you should squash your fixup-commits.

miri64 · 2017-01-16T12:23:15Z

Oh that's just lwIP not liking 8-bit and 16-bit platforms. Found that out when working on #6372.

PeterKietzmann · 2017-01-16T12:24:44Z

I was able to IPV6=1 BOARD=z1 make clean all flash term and got ALL TESTS SUCCESSFUL

miri64 · 2017-01-16T12:26:39Z

which application?

miri64 · 2017-01-16T12:28:16Z

(I also get errors, due to some libc functions not defined on z1. Are you using some other toolchain/libc than the default Ubuntu one?)

miri64 · 2017-01-16T12:29:07Z

$ msp430-gcc --version
msp430-gcc (GCC) 4.6.3 20120301 (mspgcc LTS 20120406 unpatched)
Copyright (C) 2011 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

PeterKietzmann · 2017-01-16T12:38:03Z

Are there other applications than tests/lwip_sock_ip? My msp430 gcc is
msp430-gcc (GCC) 4.6.3 20120301

miri64 · 2017-01-16T12:45:00Z

Are there other applications than tests/lwip_sock_ip? My msp430 gcc is
msp430-gcc (GCC) 4.6.3 20120301

Not in this PR, but might have been that you were referring to #6372. Curious on z1 I get error messages about missing fflush(). This is why I blacklisted it for now (and immediately squashed)

miri64 · 2017-01-16T12:49:16Z

Oops, something went wrong.. Fixed now.

miri64 · 2017-01-16T16:32:28Z

Removed useless assignment as reported by Jenkins (the variable wasn't used after that point, so it was valid) and squashed immediately:

diff --git a/pkg/lwip/contrib/sock/lwip_sock.c b/pkg/lwip/contrib/sock/lwip_sock.c
index 4dc9bd5..8ae799b 100644
--- a/pkg/lwip/contrib/sock/lwip_sock.c
+++ b/pkg/lwip/contrib/sock/lwip_sock.c
@@ -488,7 +488,6 @@ ssize_t lwip_sock_send(struct netconn **conn, const void *data, size_t len,
             return -EINVAL;
         }
         tmp = *conn;
-        type = (*conn)->type;
     }
     else {
         netbuf_delete(buf);

miri64 · 2017-01-17T08:01:59Z

Added more boards to BOARD_INSUFFICIENT_MEMORY that were reported by Murdock and squashed immediately.

miri64 · 2017-01-17T09:54:55Z

Murdock and Jenkins are both happy :-)

PeterKietzmann

Ok let's get this in but you have to take care of this during release testing (and later as well :-))!

miri64 · 2017-01-17T12:34:32Z

Sure thing :-).

benpicco · 2022-03-09T15:44:13Z

pkg/lwip/contrib/sock/lwip_sock.c

+    if (conn == NULL) {
+        return -EADDRNOTAVAIL;
+    }
+#if LWIP_SO_RCVTIMEO


What's the reason to make this optional?
It's kind of surprising that LWIP will by default ignore the timeout in e.g. sock_udp_recv()

IIRC mostly because lwIP makes it optional. But maybe, with sock, this option should be enforced to be set.

There you go #17779

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 CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 12, 2016

miri64 assigned kaspar030 Oct 12, 2016

This was referenced Oct 12, 2016

pkg: provide sock_udp support for lwip #5937

Merged

pkg: provide sock_tcp support for lwip #5938

Merged

pkg: provide sock support for lwIP #5802

Closed

kaspar030 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 Oct 13, 2016

miri64 mentioned this pull request Oct 14, 2016

sock: change IPv4 address type to array #5945

Merged

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 CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 5, 2016

kaspar030 assigned PeterKietzmann and unassigned kaspar030 Nov 18, 2016

miri64 force-pushed the pkg/feat/lwip-sock-ip branch from c11f7aa to 9738d29 Compare November 29, 2016 19:48

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

PeterKietzmann reviewed Jan 14, 2017

View reviewed changes

miri64 mentioned this pull request Jan 15, 2017

tests: port tests/lwip to sock #6372

Merged

PeterKietzmann 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 Jan 16, 2017

miri64 force-pushed the pkg/feat/lwip-sock-ip branch from 10aab8a to 73fb6fc Compare January 16, 2017 12:43

miri64 force-pushed the pkg/feat/lwip-sock-ip branch from 73fb6fc to f98418e Compare January 16, 2017 12:49

miri64 force-pushed the pkg/feat/lwip-sock-ip branch from f98418e to 7965fc7 Compare January 16, 2017 13:28

PeterKietzmann added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 16, 2017

miri64 force-pushed the pkg/feat/lwip-sock-ip branch from 7965fc7 to d58284a Compare January 16, 2017 16:32

pkg: provide sock_ip support for lwip

bf60c2f

miri64 force-pushed the pkg/feat/lwip-sock-ip branch from d58284a to bf60c2f Compare January 17, 2017 08:01

PeterKietzmann approved these changes Jan 17, 2017

View reviewed changes

PeterKietzmann merged commit 1730b88 into RIOT-OS:master Jan 17, 2017

miri64 deleted the pkg/feat/lwip-sock-ip branch January 17, 2017 12:34

benpicco reviewed Mar 9, 2022

View reviewed changes

Conversation

miri64 commented Oct 12, 2016

Uh oh!

miri64 commented Oct 31, 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!

miri64 commented Jan 13, 2017

Uh oh!

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

miri64 commented Jan 14, 2017

Uh oh!

PeterKietzmann commented Jan 14, 2017

Uh oh!

miri64 commented Jan 15, 2017

Uh oh!

miri64 commented Jan 15, 2017

Uh oh!

PeterKietzmann commented Jan 15, 2017

Uh oh!

miri64 commented Jan 15, 2017

Uh oh!

miri64 commented Jan 15, 2017

Uh oh!

PeterKietzmann commented Jan 16, 2017

Uh oh!

PeterKietzmann commented Jan 16, 2017

Uh oh!

miri64 commented Jan 16, 2017

Uh oh!

PeterKietzmann commented Jan 16, 2017

Uh oh!

miri64 commented Jan 16, 2017

Uh oh!

miri64 commented Jan 16, 2017

Uh oh!

miri64 commented Jan 16, 2017

Uh oh!

PeterKietzmann commented Jan 16, 2017

Uh oh!

miri64 commented Jan 16, 2017

Uh oh!

miri64 commented Jan 16, 2017

Uh oh!

miri64 commented Jan 16, 2017

Uh oh!

miri64 commented Jan 17, 2017

Uh oh!

miri64 commented Jan 17, 2017

Uh oh!

PeterKietzmann left a comment

Choose a reason for hiding this comment

Uh oh!

miri64 commented Jan 17, 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!

Reviewers