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

lwip2: fix static address management #4677

Merged
merged 5 commits into from
May 1, 2018
Merged

lwip2: fix static address management #4677

merged 5 commits into from
May 1, 2018

Conversation

d-a-v
Copy link
Collaborator

@d-a-v d-a-v commented Apr 25, 2018

No description provided.

Copy link
Member

@igrr igrr left a comment

Choose a reason for hiding this comment

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

I wanted to comment on leftover if 1 and if 0 preprocessor blocks in lwip-builder, but GitHub doesn't let me do this for some reason. Other changes LGTM.
Is it possible to come up with a test case for static IP assignment, or too complex?

@devyte
Copy link
Collaborator

devyte commented Apr 25, 2018

@igrr that's because the code is really in the other repo.
I've been thinking that the glue code should be moved here, and the other repo should be "pure" lwip. I have the following reasons:

  1. The glue code actually needs some maintenance, and it is easier to look at and review here
  2. The glue code is really ESP-specific, sonI think it really belongs here
  3. It would be more in line with e.g. spiffs
  4. The other repo wouldd be just standalone lwip, configured for the ESP, but without ESP specifics

@d-a-v
Copy link
Collaborator Author

d-a-v commented Apr 25, 2018

@igrr there is no lwIP api to check whether dhcp is running (or I could not find it), so yes I currently don't know how I could make a device test with this.
However I think it is worth adding the interactive sketch in libraries/esp8266/examples unless there would be a better place for it. I actually used it with some specific options to switch back and forth with static address. This test could become more consistent and evolve if it was in the core repo.
In the meantime I will update lwip2 to remove old comments and #if 0 dead code. There will also be the small packet capture changes which are harmless in normal conditions.

@devyte thanks to pinpoint. The "lwip2" repo is for esp8266-nonos-sdk only. lwIP -the real upstream library- is a sub-module (hence a sub-sub-module of the arduino core) and is not held in the "lwip2" repo. "lwip2" is only the glue between esp8266-nonos-sdk and upstream lwIP hosted on savannah.
I currently host the glue, but it could be changed.

remove dead code
add (*phy_capture)() for input/output packet capture
add lwip_unhandled_packet() weak empty function
Copy link
Collaborator

@devyte devyte left a comment

Choose a reason for hiding this comment

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

Nothing jumps out at me, so approving.

@d-a-v d-a-v merged commit 76a14b1 into esp8266:master May 1, 2018
@d-a-v d-a-v deleted the lwip2 branch May 1, 2018 23:21
@d-a-v d-a-v added this to the 2.4.2 milestone Aug 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants