Skip to content
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

Closed
wants to merge 17 commits into from
Closed

updates for lwip2 #1147

wants to merge 17 commits into from

Conversation

d-a-v
Copy link

@d-a-v d-a-v commented Jun 6, 2017

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):

ENABLE_CUSTOM_LWIP=2

Comments are welcome.

Copy link
Contributor

@slaff slaff left a 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
Copy link
Contributor

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

@@ -188,7 +193,11 @@ endif

LIBLWIP = lwip
ifeq ($(ENABLE_CUSTOM_LWIP), 1)
Copy link
Contributor

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

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
Copy link
Contributor

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?

Copy link
Author

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.

@d-a-v
Copy link
Author

d-a-v commented Jun 16, 2017

@slaff all change requests are addressed

@d-a-v
Copy link
Author

d-a-v commented Jun 17, 2017

I synchronized this PR with the latest commits. I hope this is the best thing to do.
Please advise if this is unwished so I can roll that backwards.

@slaff
Copy link
Contributor

slaff commented Jun 19, 2017

I synchronized this PR with the latest commits.

Please, rebase your branch on develop. This way we can see easily only your changes.

git checkout develop
git pull
git checkout <your-branch>
git merge develop
git rebase develop
git push -f <your-origin> <your-branch>

@d-a-v
Copy link
Author

d-a-v commented Jun 19, 2017

Done, thanks :)

@slaff
Copy link
Contributor

slaff commented Nov 13, 2017

@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.

@d-a-v
Copy link
Author

d-a-v commented Nov 13, 2017

This is true. The problem is that Sming/system/ does include lwip but also some Sming's system files. If they were separated, then the renames would not be needed.

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).

@d-a-v
Copy link
Author

d-a-v commented Nov 13, 2017

Also, now lwip2 uses espressif's lwip include files (because some definitions are subject to changes, like recently with the new definition of lwip_err_t). It seems Sming has a decorrelated copy of these include files. For instance, the dhcps_pcb field in netif is not in Sming, but is in esp-lwip-sdk and in esp-open-sdk too.
Since Sming depends on esp-open-sdk, I would suggest, if lwip2 is to be included, that Sming sources files may directly use esp-open-sdk's lwip include files. I rekon this would be a significative change (not that much in sources), but since the library comes from there, why not taking the matching include files too.

@d-a-v
Copy link
Author

d-a-v commented Nov 13, 2017

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).
Another solution would be the following: all lwip include files should be removed from Sming itself, and used from esp-open-sdk, as proposed above.
lwip2 can be proposed to esp-open-sdk (it's in progress) so once it's done, Sming could take benefit from this.
Still, the changes in SmingCore/Network/ are mandatory.
What do you think ?
Should I close here ?

@slaff
Copy link
Contributor

slaff commented Nov 15, 2017

What do you think ?

@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?

@d-a-v
Copy link
Author

d-a-v commented Nov 15, 2017

Your changes are elegant :)
So I'm confident you can find another elegant way to separate lwIP include files from Sming/system/.
About espconn_* there is no reason why they could not be forwardported using the same LWIP_IP_ADDR_T-like definitions. I did not try to do it because they are not called from the binary libraries nor the arduino ones. dhcps is an app that was forwardported and it's been done without much hassle.

@slaff slaff added this to the 3.5.0 milestone Nov 16, 2017
@slaff
Copy link
Contributor

slaff commented Nov 21, 2017

Will be merged with rebase #1289.

@slaff slaff closed this Nov 21, 2017
@d-a-v d-a-v deleted the lwip2 branch November 21, 2017 13:57
@slaff slaff removed the 3 - Review label Sep 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants