-
Notifications
You must be signed in to change notification settings - Fork 2.1k
pkg: provide sock_udp support for lwip #5937
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
Conversation
Needs adaptation to #5945. |
No time, reassigning. |
Adaption to #6137 (comment) also needed... |
What's the latest on this? |
5a1e6b2
to
05f6a12
Compare
Rebased and made adaptation. |
Ping? |
5125e2b
to
0be4ebb
Compare
Rebased and squashed to latest master. |
ef282dc
to
0be4ebb
Compare
a2c8f03
to
1f83319
Compare
Squashed |
1f83319
to
e801839
Compare
Added nucleo32-f042 to boards that do not have sufficient memory and squashed immediately. |
Jenkin's happy. I'm adding Murdock :-) |
I guess it's known that tests/lwip and examples/gnrc_networking is not interoperable? |
On a phytec board I don't get an IPv6 address. |
Well I think for interoperability lwip is using IPv4 and gnrc_networking IPv6 isn't it? @miri64 ? |
Yes, there is a bug in lwIP preventing this
Huh? lwIP also is able to speak IPv6, but its not parsing IEEE 802.15.4 headers correctly. |
@PeterKietzmann are the extensive tests in |
As said, |
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.
Just some minor comments, I'll test and then ACK.
|
||
ssize_t sock_udp_recv(sock_udp_t *sock, void *data, size_t max_len, | ||
uint32_t timeout, sock_udp_ep_t *remote) | ||
{ |
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.
Why ssize_t
? As far as I can see you're returning int
and at the end you cast another int to this type...
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 is unrelated to this PR. If you want to change the sock API, please do so in a separate PR. Otherwise I can't change it, because the API demands it.
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 I didn't know that the sock API forced that type. Then for this PR it's OK.
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.
Please don't change the API again :-(
tests/lwip_sock_udp/stack.c
Outdated
#define _MSG_QUEUE_SIZE (1) | ||
#define _SEND_DONE (0x92d7) | ||
#define _NETDEV_BUFFER_SIZE (128) | ||
|
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.
Maybe these could be aligned?
Tested with success on native. |
Adressed comments |
Then everything is OK, you squash and merge when Murdock agrees. |
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.
ACK
@kYc0o did you experience the same problems with |
I'll check it asap, since I just ran |
83407ea
to
a45256a
Compare
Squashed
@PeterKietzmann I highly doubt this is related to this PR. |
@PeterKietzmann I don't find a way to test |
@miri64, @kYc0o I've tested with #6372 and I would also assume the problem in there. However, I intended to compare test results. We can continue the discussion in there. |
Yapp, but are you okay with merging this one for now? |
I'd say merge everything necessary for #6732 and test there, if there is something pointing here then we can find it easily I think. So, merge at will. |
This was taken out of #5802 and only provides the
sock_udp
part and the tests for it.This contains duplicate changes to #5936, so merging any of these might require rebase.