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

LwIP v2 support #389

Closed
wants to merge 2 commits into from
Closed

Conversation

ourairquality
Copy link
Contributor

Getting more confident with this change, and requesting wider testing and feedback. It is working fine here in limited testing, in station and softap modes, with a http server and dns server and the dhcp server and client.

Tracking the upstream lwip master as it is adding some useful changes that help, particularly for pbuf management that helps tracking the esp rx pbufs.

Couldn't get all the dependant code decompiled to source, so had to do some minor patching of the binaries. But double checked all the code that has been converted to source.

Could this ever make it back into this repo or will it always be too much of a step?

@UncleRus
Copy link
Member

UncleRus commented Jun 7, 2017

Closing/reopening this PR to trigger Travis CI build.

@ourairquality
Copy link
Contributor Author

@UncleRus Thank you the build is looking much better now. Remains to update the extras/httpd to get this all building, but everything else builds here.

@ourairquality
Copy link
Contributor Author

@lujji I took a quick look at updating your http server, and have had to disable it for now so the build checks pass. Btw lwip now includes httpd in the main source code. The addition of the websocket support makes it more of a challenge for me to update this example. One benefit of getting this updated might be that your code would be based off the lwip master branch and might be a candidate to integrate there. One reason I am so keen to get lwip updated is to try to avoid wasted resources working with the old 1.4 version. Would you be interested in updating your http server example? Would people accept landing the lwip v2 change with this example disabled?

@lujji
Copy link
Contributor

lujji commented Jun 7, 2017

@ourairquality Great job on bringing lwip v2 to the project! I'll definitely try porting the example code, in the meantime - feel free to disable it.

@lujji
Copy link
Contributor

lujji commented Jun 8, 2017

@ourairquality could you point out where build checks fail? I get 3 warnings when I build the project - is that the problem?

@ourairquality
Copy link
Contributor Author

@lujji Thank you for taking a look. Problems looked worse at first, and glad it was an easy fix. Re-enabled the http_server example and the checks all pass now.

@UncleRus
Copy link
Member

Awesome job.
Will merge it if no objections.

@malachib
Copy link

I am very excited about this. mdns is super convenient, and http(s)d included is the missing link. Hopefully our devices have enough juice to to the https stuff..

@ourairquality
Copy link
Contributor Author

If you were having issues with a station with a static IP address then please give it another try - it never worked in the past for me either but it does now with some limited testing. We seem to have all the source code for that path now in wpa_main.c so it was possible to rewrite it, and the same code needed changing for lwip v2 anyway and has been cleaned up and a hack removed from the lwip code.

@malachib
Copy link

malachib commented Jul 9, 2017

In all my own testing, this lwip2 branch has performed admirably. Additionally, it looks like this build failure may have been a fluke. If a rebuild passes, I motion we proceed forward and merge in this pull request.

@ourairquality
Copy link
Contributor Author

Might be best to just wait a little longer. I am trying to get the same lwip v2 code running on the current SDK too and another pass over it might clean it up a little more.

@malachib
Copy link

I respect that. I've updated the docker images for a ready-to-go build environment. They work great for me, but try them at your own risk :) The one tagged "latest-ourairquality" points to your repo and branch of the same name

@ourairquality
Copy link
Contributor Author

Had some luck getting ipv6 running, and the patch at https://github.com/ourairquality/esp-open-rtos has ipv6 enabled. Currently only the http-get example uses ipv6, so there is some work to go cleaning up the code. Not sure how well ipv6 is tested in lwip, dns was broken, and neighbor solicitation was broken (or I did not know how to get it running as is), but these have been patched. Could use help on this, and perhaps having something barely working might give some encouragement.

@SaimenSays
Copy link

I have some serious problems with actual lwip or low level hardware adaption. After some time tcp_accept() callback gets never called anymore.
Hopefully this is fixed with this update. Does anybody know when this pull request will be "official" released?

@ourairquality
Copy link
Contributor Author

@SaimenSays Sorry it's probably not ready yet, and the patch here is rather stale now. I think good progress has been made and issues are being found and fixed at a good rate, but there are still open issues such as the queuing of pbufs in the TX paths. The upstream lwip code is in general receiving fixes at a good rate, and a good number of issues have been in the upstream code, so perhaps it has a little way to go to the next stable release. If you want to help then please see the version at https://github.com/ourairquality/esp-open-rtos for now.

@SaimenSays
Copy link

@ourairquality I checked out your branch. Some modifications because of changed function arguments need tobe done. Now it compiles and seems to work on first sight.

One issue I figured out already before, is the delayed freeing of heap. On each new connection there are 168Bytes more heap used, also if TCP connection is closed. It takes sometimes up to ones second until RAM gets released and I can see previous value with 'sdk_system_get_free_heap_size()'.
It looks little amount, but in one minute many connections can be opened and we know ESP lacks in RAM size.
Any idea how this happens? Does anybody know if it is an issue of RTOS memory handling or is there a longer timeout in tcp task?

@ourairquality
Copy link
Contributor Author

@SaimenSays It might be the TCP time-wait state. I had to rework the wificfg http server to try to have the client close the connection first to avoid the server being left in a time-wait, and that change helped a lot. Is that an option in your application, so avoid the esp8266 closing the TCP connection until after the client has? Alternatively perhaps you can lower this wait time. Perhaps try enabling LWIP_STATS_DISPLAY and calling stats_display() in lwip, see if it gives some clues.

@ourairquality
Copy link
Contributor Author

This patch has been updated to the latest code developed. Patch #430 had to be bundled in.

For practical reasons there will be no further development of this particular patch. I can't just keep rebasing as people using my fork need the history and having to deal with a rebased stack of patches is frustrating for them. So further work will build on this. It might still be possible to pick and bundle some follow up patches, but I expect it will become difficult due to conflicts.

This is as good as I have been able to get it in the time available.

@malachib
Copy link

I appreciate all your efforts @ourairquality . Your lwip2 code was running great for me

@flannelhead
Copy link
Contributor

@malachib These patches still exist and now reside in a different PR :) #437

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.

None yet

6 participants