-
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
lwIP: Initial import #3551
lwIP: Initial import #3551
Conversation
Could you explain how this relates to netdev/netif and gnrc? Would this stack offer the same functionality as some parts of gnrc? Are there any differences in supported features etc? |
I port this stack mainly for comparability in my master thesis (there will follow a few more implementations over the next weeks ;-)). I will port lwIP's driver interface so that it will speak to netdev. As far as I know (but I did not delve deep enough yet to say for sure) the limitations compared to gnrc are:
The main advantage might be it's size, but the final verdict will have to wait until I finished my master thesis ;-) |
Ported to and rebased upon new semaphore module |
f2d9e37
to
b0f2873
Compare
2ac4bcb
to
f81323a
Compare
Rebased to current master |
6fbebaf
to
c0baa12
Compare
Tested with |
+#define LWIP_ASSERT(message, assertion) \ | ||
+ if(!(assertion)) { \ | ||
+ LWIP_PLATFORM_ASSERT(message); \ | ||
+ } |
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 is this patch required?
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.
As the patch message states: it fixes warnings, that would make it impossible to compile without W_ERROR=0
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.
(and no, I don't remember the precise warning ;-P)
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.
For the record: the warning was something like pkg/lwip/lwip/src/include/lwip/debug.h:71:32: error: suggest braces around empty body in an ‘if’ statement [-Werror=empty-body]
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.
Ah right, and I thought then: why have a do { } while (0)
around it anyway then?
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 a reminder for me to try it without this patch
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.
Does not compile without the patch, so yes it is required.
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.
Do you think there could be a way to get this patch upstream?
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 can try...
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.
The problem is that it is technically perfectly fine code, since the #ifndef
throws an error anyway if LWIP_PLATFORM_ASSERT
isn't defined and it's just our -Werror
that makes problems here. In my experience with the community there I don't think this will come through.
fixed a few assertion related issues |
Now I tested receiving of IPv6 frames, too ;-) |
* @brief Compiler hints for packing structures | ||
* @{ | ||
*/ | ||
#define PACK_STRUCT_FIELD(x) x |
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.
Shouldn't we put parens around the x
?
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.
No, this is a macro to build structs platform indepentent.
Check this: remove the |
First let me look what Murdock is saying about this. |
|
DRIVER keeps set to |
Should be reset in https://github.com/RIOT-OS/RIOT/pull/3551/files#diff-b2af745a4a882b6e5cb2abb8b6ac785bR23 (l. 23). But anyway, can you try and see, what happens if the colon is removed before the equal sign for the DRIVER assignments? |
same result |
I think the problem is somehow related to the fact that If you do |
Then setting |
As a workaround for the broken FEATURES_REQUIRED/make buildtest thing, that would work, yes. |
For some reason the Makefile on Murdock is only evaluated after the build happens:
compare that to my output:
|
Okay adapted for workaround now. |
It's still saying |
Derp... |
\o/ Murdock is finally green like Ireland! |
Then merge at will :-) |
Fixed assert issues and squashed immediately. |
ACK |
Murdock is happy. |
I'd say: the honor to push the button is yours! |
Thanks for the review :-) |
It was a pleasure! |
Imports lwIP for RIOT as a
pkg
.Depends on #3549.Todos:
conn
connectivityconn_ip
conn_udp
conn_tcp
(optional)