-
-
Notifications
You must be signed in to change notification settings - Fork 346
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
updates for lwip2 #1147
updates for lwip2 #1147
Conversation
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.
See the comments that are part of the review. In short
- it would be better to simplify the makefile changes
- it would be better to have the lwip2 include files coming from the third-party module.
Sming/Makefile
Outdated
EXTRA_INCDIR += third-party/esp-open-lwip/include | ||
ifeq ($(LWIP_FLAVOUR), lwip2) | ||
THIRD_PARTY_DATA += third-party/lwip2/Makefile.sming | ||
EXTRA_INCDIR += compiler/include/lwip2 |
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.
@d-a-v The "compiler" directory is not the right place to put the include files for LWIP 2. They should be part of the submodule and ideally should end up in third-party/lwip2/include
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.
agreed
Sming/Makefile-project.mk
Outdated
@@ -188,7 +193,11 @@ endif | |||
|
|||
LIBLWIP = lwip | |||
ifeq ($(ENABLE_CUSTOM_LWIP), 1) |
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.
@d-a-v We can read the value of the ENABLE_CUSTOM_LWIP
and if it is 1 then we will use the esp-open-lwip version 1. If it is 2 we will use the lwip2 code. This way we don't have to introduce yet another directive.
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.
agreed
Sming/Makefile-rboot.mk
Outdated
LWIP_INCDIR = $(SMING_HOME)/third-party/esp-open-lwip/include | ||
ifeq ($(LWIP_FLAVOUR), lwip2) | ||
LWIP_INCDIR = $(SMING_HOME)/compiler/include/lwip2 | ||
MORE_CFLAGS += -DLWIP_NO_STDINT_H=1 |
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.
@d-a-v Is that really needed here? Won't it be possible to add it to the lwipopts.h file or just patch the stock lwipopts.h that is coming with your repository?
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 was lazy :) it has moved outside from sming.
@slaff all change requests are addressed |
I synchronized this PR with the latest commits. I hope this is the best thing to do. |
Please, rebase your branch on develop. This way we can see easily only your changes.
|
…P_HASH_STR' is unknown)
Done, thanks :) |
…P_HASH_STR' is unknown)
@d-a-v There are a lot of renamed files in this PR. For example "Sming/system/include/arch/cc.h → Sming/system/esp-lwip/arch/cc.h". Please, revert those renames because we still need them if someone decides to compile Sming with the stock LWIP. |
This is true. The problem is that The stock lwip could (and should) still be used, at the cost of one more "-Ipath/to" gcc option. I'm open to any solution, but for lwip2 to freely live inside Sming, lwip must be out of the way (but still around - at least the include files - because it is needed for building lwip2). |
Also, now lwip2 uses espressif's lwip include files (because some definitions are subject to changes, like recently with the new definition of |
Note that in the current PR, lwip is still usable (gcc -I options are accordingly updated to the directories where lwip includes files are moved). |
@d-a-v Your suggestions sound reasonable. I just need time to evaluate the best way to go. The fact that we support multiple SDKs or different OSes, some of which have wrong or incomplete LWIP headers is the reason to have them as part of the library. In addition I experimented with less intrusive code changes on our side that you can see from here: develop...slaff:feature/lwip2#diff-5d6d3354bd9d34ab47696d7f7088ea65 One question: If LWIP2 is used there is no possibility to use espconn_* methods, right? Is that correct or wrong? |
Your changes are elegant :) |
Will be merged with rebase #1289. |
this PR brings lwIP-v2 to Sming.
original lwIP source code version (stable 2.0.2) is pulled as a sub-sub-module.
To use it, use this environment variable (edit:
was LWIP_FLAVOUR):Comments are welcome.