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 update #76

Merged
merged 26 commits into from Apr 25, 2015
Merged

Lwip update #76

merged 26 commits into from Apr 25, 2015

Conversation

d-a-v
Copy link
Collaborator

@d-a-v d-a-v commented Apr 21, 2015

two main changes:

  • lwip is updated to latest git version (20apr2015)
  • tcpservice now debugged and stable

LWIP:
Old src/lwip is reworked and moved to src/lwip-esp/, new one is in src/lwip-git/.
Both are working and improved, lwip-git version is bigger in rom.
make menuconfig selects the version you wish.
For both of them, hard work has been done to reduce the espressif diff against original versions to its minimum.
Scripts are provided in src/contrib/lwipupdate/ to watch espressif patches against original version, and to bump lwip-git if needed (lwip guys should tag their repo, the git version is named 1.4.1 although the official stable 1.4.1 does not come from the git repo). espressif version is based on something between 1.4.0RC1 and RC2.
Timers management (sys_now()) has been reworked. espressif odd coarse timer removed. Original lwip timers are working (in both versions).

tcpservice:
it is aimed to be a very small and efficient infrastructure for raw tcp. the optional echo service is able to run for hours (with the tester src/contrib/tcpechotester.c) and transfer megabits with no freeze and no leaks. The very much goal of this is for a direct and above all stable serial<->wlan service which hopefully come soon.

Note that I have not tested AP and APSTA modes at all.

enjoy :)

d-a-v added 26 commits April 21, 2015 18:28
Signed-off-by: David Gauchard <gauchard@laas.fr>
… better diffs)

Signed-off-by: David Gauchard <gauchard@laas.fr>
Signed-off-by: David Gauchard <gauchard@laas.fr>
Signed-off-by: David Gauchard <gauchard@laas.fr>
…r to a known version of lwip

(removing optimisations from espressif and other little useful patch)
(tested running: STA mode, dhcp client, telnet and echo)
Signed-off-by: David Gauchard <gauchard@laas.fr>
same tests working (STA, telnet, echo)
Signed-off-by: David Gauchard <gauchard@laas.fr>
add missing.h with definitions that should go to antares
(tests telnet+echo running)
Signed-off-by: David Gauchard <gauchard@laas.fr>
…s in makefiles.

(they actually are thanks to antares build system with the good default configuration)
Signed-off-by: David Gauchard <gauchard@laas.fr>
Adding print in some special cases which never happen:
	PBUF_ESF_RX case never encountered while echo tester is flying at 2.5Mbits/s in STA mode
Signed-off-by: David Gauchard <gauchard@laas.fr>
…y me (telnet, echo, STA mode).

(goal: minimal patch, Will deal with it later when/if pbuf-type error is encountered with updated lwip)
Signed-off-by: David Gauchard <gauchard@laas.fr>
Signed-off-by: David Gauchard <gauchard@laas.fr>
	this is not the latest but diff/repatch scripts are provided into src/contrib/
add an option into kcnf to choose lwip version
reduce TCP_MSS and TCM_RWND to the smallest for ram memory footprint

Signed-off-by: David Gauchard <gauchard@laas.fr>
Signed-off-by: David Gauchard <gauchard@laas.fr>
no more log2(size) for cbuf
reintroduced ack cb in tcpservice
add tcpservice api comments
…n (compile-time and run-time)

Signed-off-by: David Gauchard <gauchard@laas.fr>
Signed-off-by: David Gauchard <gauchard@laas.fr>
Signed-off-by: David Gauchard <gauchard@laas.fr>
Signed-off-by: David Gauchard <gauchard@laas.fr>
Signed-off-by: David Gauchard <gauchard@laas.fr>
reduce again both lwip patches
add tcpservice options to setup poll (and nagle)
Signed-off-by: David Gauchard <gauchard@laas.fr>
Signed-off-by: David Gauchard <gauchard@laas.fr>
this is a big commit including change of structure name ip_addr -> ip4/6_addr (this is still working)
Signed-off-by: David Gauchard <gauchard@laas.fr>
@d-a-v
Copy link
Collaborator Author

d-a-v commented Apr 21, 2015

Forgot to tell:
TCP_MSS and TCP_WND have been reduced to their minimum commonly allowed (536 bytes and 2*MSS respectively). This reduces the ram footprint (works on both versions).
Special care has to be done when using raw lwip and espressif blobs, I tried to be clear in include/tcpservice.h.

@nekromant
Copy link
Owner

That looks like one hell of awesomeness. Just a few questions though:

  1. Why don't we just ditch all the ICACHE_FLASH_ATTR for good? Antares already does all the relocating, so why bother? When we do realocation ICACHE_FLASH_ATTR is not only unneeded - it can be even harmful (There's a corner case, if ICACHE_FLASH_ATTR function is present and there's also a function explicitly put into iram0 - the linker magic will break and the binary won't work).
  2. May be it's better to move the lwip to antares itself and escape all the espressif's hacks with #ifdef CONFIG_ARCH_ESP8266 ?

@d-a-v
Copy link
Collaborator Author

d-a-v commented Apr 21, 2015

On mar., avril 21, 2015 at 10:06:33 -0700, Andrew wrote:

That looks like one hell of awesomeness.

head banging for this to happen. really.
https://www.youtube.com/watch?v=q8SWMAQYQf0
s/unreal tournament/lwip/g

Just a few questions though:

  1. Why don't we just ditch all the ICACHE_FLASH_ATTR for good? Antares already does all the relocating, so why bother?

They are. There's one left in cmd_listen.c which I did not touch.

If I understand well (according to what you told me before -
and correct me if I'm wrong), ICACHE_FLASH_ATTR makes the code
copied and executed in ram, so OTA is possible.

Currently antares, thanks to a kcnf option, artificially tags
all functions to move them in ram (we don't need
ICACHE_FLASH_ATTR anymore).

What would be interesting is to tag only the minimum subset
(including lwip and some others) for OTA, and some few others
for speed, than leave the rest in rom. We would gain ram
space.
All we have to do is to put open-source objects into two
differents library, and tag only one.

Right ?

  1. May be it's better to move the lwip to antares itself and
    escape all the espressif's hacks with #ifdef CONFIG_ARCH_ESP8266 ?

We could do that. But I guess the #if and #ifdef names need to
be rock-clear. And it would be better for antares a taggued or
released version of lwip.

david

@nekromant
Copy link
Owner

david gauchard писал 21.04.2015 20:23:

On mar., avril 21, 2015 at 10:06:33 -0700, Andrew wrote:

That looks like one hell of awesomeness.

head banging for this to happen. really.
https://www.youtube.com/watch?v=q8SWMAQYQf0
s/unreal tournament/lwip/g

Just a few questions though:

  1. Why don't we just ditch all the ICACHE_FLASH_ATTR for good?
    Antares already does all the relocating, so why bother?

They are. There's one left in cmd_listen.c which I did not touch.

If I understand well (according to what you told me before -
and correct me if I'm wrong), ICACHE_FLASH_ATTR makes the code
copied and executed in ram, so OTA is possible.

Nope, the opposite.
Epressif's stuff works like this:

.text gets copied and executed from RAM
.irom0.text gets executed from flash as is.

ICACHE_FLASH_ATTR instructs compiler to put function into irom0.text

To avoid that ICACHE_FLASH_ATTR everywhere antares does the following:

  1. Adds a new section iram0.text that always goes to RAM, just like
    .text
  2. Renames .text to irom0.text in all our code AFTER compilation
    before linking.
  3. Leaves all the blobs that have .text in ram as is, not to break
    anything.

Therefore, ICACHE_FLASH_ATTR is no longer needed, so that we only
specify
section name for stuff we really want in RAM (like we do in tftp
firmware sync code).
Since there's not much RAM available, we normally don't want that.
Yes, it's an ugly hack and black magic, but the sole purpose of that was
so that
we can avoid hacking the code of every little bit to port it to esp8266.

Currently antares, thanks to a kcnf option, artificially tags
all functions to move them in ram (we don't need
ICACHE_FLASH_ATTR anymore).

What would be interesting is to tag only the minimum subset
(including lwip and some others) for OTA, and some few others
for speed, than leave the rest in rom. We would gain ram
space.
All we have to do is to put open-source objects into two
differents library, and tag only one.

Right ?

  1. May be it's better to move the lwip to antares itself and
    escape all the espressif's hacks with #ifdef CONFIG_ARCH_ESP8266 ?

We could do that. But I guess the #if and #ifdef names need to
be rock-clear. And it would be better for antares a taggued or
released version of lwip.

david

Reply to this email directly or view it on GitHub [1].

Links:

[1]
#76 (comment)

Regards,
Andrew

@nekromant
Copy link
Owner

In other words, the rule of thumb for frankenstein is - screw the ICACHE_FLASH_ATTR and never use it.

@d-a-v
Copy link
Collaborator Author

d-a-v commented Apr 22, 2015

got it.
The only one left in cmd_listen.c is not mine. I'll modify cmd_listen to use tcpservice instead if needed.

@d-a-v
Copy link
Collaborator Author

d-a-v commented Apr 22, 2015

For the record, I have now 5 echoservice running concurrently for 17.5 hours. They transfered 2.75GBytes each so far, at an average bitrate of 1800 kbits/s overall (on a busy wifi network). The bit rate could be better at the price of bigger buffer (and less free ram). But my thinking is that there is no need for a large bandwidth, if it is at least above the maximal serial bitrate.
I think this stability is thanks to the timer management which was not working well from the beginning. Before this patch, I had some transfers which stopped on heavy load, I guess this was because of the bad timer infrastructure which caused the tcp RTO (retransmit time out) to malfunction.

@nekromant
Copy link
Owner

@d-a-v I'm talking about ICACHE_FLASH_ATTR as seen in for example src/lwip/include/ipv4/lwip/icmp.h and other lwip headers. Do we really need them?
As for the rest of the codebase, I used a sed script to purge them all already, so whatever's in cmd_listen.c - it must be a leftover, I'll fix it right away.

@d-a-v
Copy link
Collaborator Author

d-a-v commented Apr 22, 2015

src/lwip does not exist anymore. It is now src/lwip-esp/ and there is no ICACHE_FLASH_ATTR in the pull request.

@nekromant nekromant merged commit 6fab9b7 into nekromant:master Apr 25, 2015
@nekromant
Copy link
Owner

@d-a-v I gave your lwip branch a quick try, looks like most stuff works, so I've merged it into master.

@d-a-v
Copy link
Collaborator Author

d-a-v commented Apr 25, 2015

On sam., avril 25, 2015 at 11:08:00 -0700, Andrew wrote:

@d-a-v I gave your lwip branch a quick try, looks like most stuff works, so I've merged it into master.

Thanks, I have a timer management fix. I'll push in a short while.

@nekromant
Copy link
Owner

@d-a-v Yes, and mind that we have something broken on the latest SDK. I've updated the README, but had no chance to troubleshoot it yet.

@d-a-v d-a-v deleted the lwip-update branch April 26, 2015 20:49
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

2 participants