-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
pkg: provide sock_ip support for lwip #5936
Conversation
Needs adaptation to #5945. |
Reassigning. |
Adaption to #6137 (comment) also needed... |
c11f7aa
to
9738d29
Compare
Provided the same treatment to this PR as I did for #5937. |
Ping? |
This one isn't WIP anymore either. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 :-)
#error "lwip_sock needs IPv4 or IPv6 support" | ||
#endif | ||
|
||
static inline bool _af_not_supported(int af) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this function on a first sight.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add some doc.
if ((*out = netconn_new_with_proto_and_callback(type, proto, NULL)) == NULL) { | ||
return -ENOMEM; | ||
} | ||
#if SO_REUSE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strange flag name. Is it lwip specific?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comes from lwIP. So I won't rename it.
To test both set the `LWIP_IPV4` and `LWIP_IPV6` to a non-zero value: | ||
|
||
```sh | ||
LWIP_IPV4=1 LWIP_IPV6 make all test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LWIP_IPV6=1
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. |
Nope, not yet. The assumption basically is so far
=> networking over sock works. |
Addressed comments |
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. |
@PeterKietzmann see #6372 |
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. |
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. |
Oh that's just lwIP not liking 8-bit and 16-bit platforms. Found that out when working on #6372. |
I was able to |
which application? |
(I also get errors, due to some libc functions not defined on |
|
Are there other applications than tests/lwip_sock_ip? My msp430 gcc is |
10aab8a
to
73fb6fc
Compare
Not in this PR, but might have been that you were referring to #6372. Curious on |
73fb6fc
to
f98418e
Compare
Oops, something went wrong.. Fixed now. |
f98418e
to
7965fc7
Compare
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); |
7965fc7
to
d58284a
Compare
d58284a
to
bf60c2f
Compare
Added more boards to |
Murdock and Jenkins are both happy :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok let's get this in but you have to take care of this during release testing (and later as well :-))!
Sure thing :-). |
if (conn == NULL) { | ||
return -EADDRNOTAVAIL; | ||
} | ||
#if LWIP_SO_RCVTIMEO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC mostly because lwIP makes it optional. But maybe, with sock
, this option should be enforced to be set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There you go #17779
This was taken out of #5802 and only provides the
sock_ip
part and the tests for it.