Skip to content

Remove FEATURE_LWIP #7287

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jun 25, 2018
Merged

Remove FEATURE_LWIP #7287

merged 4 commits into from
Jun 25, 2018

Conversation

SeppoTakalo
Copy link
Contributor

Description

Follow up on removal of FEATURE_NANOSTACK
Now do the same for LWIP so that all networking functions are enabled on all builds so that adding networking capabilities to targets that did not originally have those, do not require enabling of any feature.

Does not affect RAM/ROM usage.

Pull request type

[ ] Fix
[X] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

Seppo Takalo added 3 commits June 21, 2018 10:28
Remove deprecatd LwIP eth drivers
adce2d9
Remove FEATURE_LWIP. The LwIP stack is enabled on all builds now
e4d1a9a 
Leave the FEATURE_LWIP enabled in build scripts so that it does
not break any builds.

Removed 'feature_add: ["LWIP"]' on all targets.
Add deprecation warning for those who enable LWIP or NANOSTACK
c544ce0 
This is dropped by linker so does not cause any RAM/ROM cost.
@SeppoTakalo
Copy link
Contributor Author

@kjbracey-arm please review.

@0xc0170 0xc0170 requested a review from kjbracey June 21, 2018 08:03
@0xc0170 0xc0170 requested a review from bulislaw June 21, 2018 08:03
@jeromecoutant
Copy link
Collaborator

Hi
Maybe a cleanup in tools/test_configs json files is also needed ?

kjbracey
kjbracey reviewed Jun 21, 2018
View reviewed changes
doxyfile_options
@@ -845,7 +845,7 @@ EXCLUDE_PATTERNS = */tools/* \
*/features/mbedtls/* \
*/features/storage/* \
*/features/unsupported/* \
*/features/FEATURE_LWIP/* \
*/features/lwipstack/* \
Copy link
Contributor

@kjbracey kjbracey Jun 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It's probably more straightforward just to be /features/lwip, I think. Does the "stack" really add anything in this context?

LWIPStack versus LWIPInterface objects kind of happened because they were in a single namespace, and reflected the underlying NetworkStack and NetworkInterface. But in the filing system like this, I think just lwip is fine. Maybe at some point lwip might end up in a networkstacks folder, if there were that many.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's possible there should be a general tidy-up to add a top-level features/networking or something too. But I think that's out of scope for this. Anyway, for now don't try to express something in your leafname that would be better expressed in a directory structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Within the lwipstack there is already folder lwip which is the ... stack.. :D

Would lwip/lwip look terrible? Currently the stack is in lwipstack/lwip

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 21, 2018

Travis failure in events is similar to the one we had for nanostack move? Please update

@SeppoTakalo
Copy link
Contributor Author

@jeromecoutant

Maybe a cleanup in tools/test_configs json files is also needed ?

Why?
I don't see anything relevant to LWIP there.

Maybe the title is a bit wrong, but I'm not removing the LwIP stack.
I'm enabling it for all builds.

So the FEATURE_LWIP that is used to enable the stack, is now gone. Same we did for Nanostack earlier.

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 25, 2018

I'll run first CI round

/morph build

@mbed-ci
Copy link

mbed-ci commented Jun 25, 2018

Build : SUCCESS

Build number : 2439
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7287/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci
Copy link

mbed-ci commented Jun 25, 2018

Exporter Build : SUCCESS

Build number : 2071
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/7287/

@mbed-ci
Copy link

mbed-ci commented Jun 25, 2018

Test : SUCCESS

Build number : 2219
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7287/2219

@cmonr
Copy link
Contributor

cmonr commented Jun 25, 2018

Waiting on @bulislaw for a final review.

@cmonr cmonr merged commit f3424da into ARMmbed:master Jun 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kjbracey kjbracey kjbracey left review comments

@bulislaw bulislaw bulislaw approved these changes

@0xc0170 0xc0170 0xc0170 approved these changes

Assignees
No one assigned
Labels
release-version: 5.10.0-rc1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@SeppoTakalo @jeromecoutant @0xc0170 @mbed-ci @cmonr @bulislaw @kjbracey