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.

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

Cleaned up Mbed LWIP configurations #11586

Merged
merged 2 commits into from
Oct 21, 2019
Merged

Cleaned up Mbed LWIP configurations #11586

merged 2 commits into from
Oct 21, 2019

Conversation

AnttiKauppila
Copy link

Description

Cleaned up LWIP configuration mechanism. Moved default settings into mbed_lib.json and updated "help" sections accordingly. This is not a breaking change, if some developers have overridden those values earlier, this change has no effect for them.
Also as mbed_retarget.h defines errno values, there were warnings about duplicates because LWIP defined those as well by default. This has also been fixed in this PR.

Pull request type

[ ] Fix
[X] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@VeijoPesonen @kjbracey-arm @ARMmbed/mbed-os-ipcore

Release Notes

LWIP configuration mechanism has been updated to make it easier to control default values directly from mbed_lib.json. This is not a breaking change and existing applications should still work unmodified with this change.

@ciarmcom ciarmcom requested review from kjbracey, VeijoPesonen and a team September 27, 2019 15:00
@ciarmcom
Copy link
Member

@AnttiKauppila, thank you for your changes.
@VeijoPesonen @kjbracey-arm @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

VeijoPesonen
VeijoPesonen reviewed Sep 30, 2019
View reviewed changes
features/netsocket/ppp/include/ppp_opts.h
@@ -71,7 +71,9 @@
#define PPP_DNS 1

// Used as maximum size for output buffer, to restrict the memory manager get_pool_alloc_unit()
#ifndef PBUF_POOL_BUFSIZE
#define PBUF_POOL_BUFSIZE 536 + 40
Copy link
Contributor

Choose a reason for hiding this comment

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

536+40 for IPv6 and 536+20 for IPv4?

VeijoPesonen
VeijoPesonen approved these changes Sep 30, 2019
View reviewed changes
Copy link
Contributor

@VeijoPesonen VeijoPesonen left a comment

Choose a reason for hiding this comment

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

The only comment I wrote was not about these changes, only something I happened to spot during the review.

Looks good to me.

0xc0170
0xc0170 reviewed Oct 1, 2019
View reviewed changes
Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

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

Otherwise looks good.

I might add more details to the commit msg about the clean up. Reading the code, you fix the config to provide default values instead of null.

features/lwipstack/lwip/src/include/lwip/lwip_errno.h
@@ -44,6 +44,7 @@ extern "C" {
#endif

#ifdef LWIP_PROVIDE_ERRNO
#if LWIP_PROVIDE_ERRNO == 1
Copy link
Contributor

Choose a reason for hiding this comment

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

line 46 and 47 are very similar, shouldn't ifdef be removed?

Copy link
Author

@AnttiKauppila AnttiKauppila Oct 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Did not want to change LwIP header logic here (other than remove duplicates)

@adbridge adbridge added release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 and removed release-version: 5.15.0-rc1 labels Oct 3, 2019
@AnttiKauppila
Copy link
Author

@adbridge Why 6.0? Why not 5.14.x for example?

@adbridge
Copy link
Contributor

adbridge commented Oct 4, 2019

@AnttiKauppila ahhh it's because Martin had previously marked this as needing to go to a feature branch. If there is no api change then we could conceivably take this to a patch ?

@adbridge
Copy link
Contributor

CI started

@mbed-ci
Copy link

mbed-ci commented Oct 10, 2019

Test run: FAILED

Summary: 3 of 4 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-IAR
  • jenkins-ci/mbed-os-ci_build-ARM

@adbridge
Copy link
Contributor

Looks like a possible CI error, restarting

@mbed-ci
Copy link

mbed-ci commented Oct 11, 2019

Test run: FAILED

Summary: 3 of 4 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_build-IAR
  • jenkins-ci/mbed-os-ci_build-GCC_ARM

@adbridge
Copy link
Contributor

@AnttiKauppila on further analysis this looks like a genuine failure, please investigate

@adbridge
Copy link
Contributor

[Warning] maclib_task.c@82,17: implicit declaration of function 'rda_mail_put' is invalid in C99 [-Wimplicit-function-declaration]
[Warning] maclib_task.c@161,27: implicit declaration of function 'rda_mail_create' is invalid in C99 [-Wimplicit-function-declaration]
[Warning] maclib_task.c@172,29: implicit declaration of function 'rda_mail_get' is invalid in C99 [-Wimplicit-function-declaration]
[Warning] maclib_task.c@201,21: implicit declaration of function 'rda_mail_put' is invalid in C99 [-Wimplicit-function-declaration]
[Error] maclib_task.c@221,29: expected ';' after expression

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 18, 2019

Travis restarted, CI restarted as well

@mbed-ci
Copy link

mbed-ci commented Oct 18, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 3
Build artifacts

0xc0170
0xc0170 approved these changes Oct 21, 2019
View reviewed changes
Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

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

I would like to see better commit msg than "Cleaned up Mbed LWIP configurations" for clean up like that in future.

As this was set as "Refactor" - thus I initially set it to the feature release.

@0xc0170 0xc0170 merged commit f6c28e5 into ARMmbed:master Oct 21, 2019
@0xc0170 0xc0170 mentioned this pull request Oct 21, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VeijoPesonen VeijoPesonen VeijoPesonen approved these changes

@0xc0170 0xc0170 0xc0170 approved these changes

@kjbracey kjbracey Awaiting requested review from kjbracey

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

Successfully merging this pull request may close these issues.
6 participants
@AnttiKauppila @ciarmcom @adbridge @mbed-ci @0xc0170 @VeijoPesonen