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

Add API to get ipv6 link local address #11279

Merged

Conversation

@cy-jayasankar
Copy link

cy-jayasankar commented Aug 21, 2019

Description

This PR provides API for applications to get the IPv6 link local address.

Pull request type

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

Reviewers

Please suggest

Release Notes

Added a new API called get_ipv6_link_local_address in NetworkInterface class.
This API returns IPv6 link local address and below is the syntax of the API.

nsapi_error_t NetworkInterface::get_ipv6_link_local_address(SocketAddress *address)
@ciarmcom ciarmcom requested review from ARMmbed/mbed-os-maintainers Aug 21, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

ciarmcom commented Aug 21, 2019

@cy-jayasankar, thank you for your changes.
@ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

@0xc0170 0xc0170 changed the title Added API to get ipv6 link local address Add API to get ipv6 link local address Aug 26, 2019
Copy link
Member

0xc0170 left a comment

"Added API to get ipv6 link local address" - adding functionality should contain better changes description in the commit message.

My questions would be "why this is needed" and "how to be used" that could be answered in the commit message.

@cy-jayasankar cy-jayasankar force-pushed the cy-jayasankar:pr/added-ipv6-link-local-address-api branch from 7f145b1 to 7d82eda Aug 27, 2019
@0xc0170 0xc0170 self-requested a review Aug 27, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Aug 27, 2019

Once you update, please send an update here with the current status, helps to keep track and notify us about the changes here as well.

@cy-jayasankar

This comment has been minimized.

Copy link
Author

cy-jayasankar commented Aug 28, 2019

Hi..I have updated the commit with following

Protocols like mdns requires IPv6 link local address to be advertised in its
records (AAAA record). LWIP::Interface::bringup() API is creating IPv6 link
local address;But as of now there is no API exposed by mbed-os to get the
IPv6 link local address.

This new API is required to deliver mDNS library support on mbed-os for Cypress
platforms. Unit tested it by invoking get_ipv6_link_local_address with a simple
application.

Copy link
Member

bulislaw left a comment

LGTM @AnttiKauppila @SeppoTakalo please review

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Aug 29, 2019

Note, we are close to the 5.14 code freeze. This needs update asap to be in 5.14 otherwise will move to the next minor version

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

SeppoTakalo commented Aug 29, 2019

I think this needs more thought.

Having able to fetch link local addresses, or multicast addresses, would be useful thing. So I do see the point.

I just don't like the way we are going with these string based APIs. I have been verbally against those, and proposed dropping all string based APIs in future. And I would also like to drop all get_ip_address() get_router_() things in favour of getifaddrs() type of API. However, we don't have plans for that.

Overall, I'm not in favour of this. It is a LwIP only, and it is a string based. Therefore I propose that we postpone it and rethink the API to be more general "give me list of all addresses" type that all other OS'es have.

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

SeppoTakalo commented Aug 29, 2019

By opposing string based API, I meant that we should use SocketAddress instead of char * for IP addresses.

For couple of reasons:

  • SocketAddress is generic, it can hold any IP version. Equivalent of POSIX struct sockaddr_t.
  • SocketAddress is binary
  • DNS query returns SocketAddress.
  • Strings should only be used for human readable things, like host name.
@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Aug 29, 2019

Removed 5.14 label, added needs: work

@sathishm6

This comment has been minimized.

Copy link

sathishm6 commented Aug 29, 2019

I think this needs more thought.

Having able to fetch link local addresses, or multicast addresses, would be useful thing. So I do see the point.

I just don't like the way we are going with these string based APIs. I have been verbally against those, and proposed dropping all string based APIs in future. And I would also like to drop all get_ip_address() get_router_() things in favour of getifaddrs() type of API. However, we don't have plans for that.

Overall, I'm not in favour of this. It is a LwIP only, and it is a string based. Therefore I propose that we postpone it and rethink the API to be more general "give me list of all addresses" type that all other OS'es have.

@SeppoTakalo - I do agree with your proposal, it makes a better API design and in the long run to have APIs like linux getifaddrs(). But I also have a slightly different approach to propose. Today there are many string based APIs exist in mbed-os (as you have listed it out). Hence having this API wouldn't be a harm, and it in-fact would help in unblocking certain development like mDNS protocol, IPv6 multicast usecases, etc. Once we have the cleaner API, we can depreciate this API along with all other existing string based APIs. Meanwhile everyone can use the API proposed in this PR to continue the development to make progress in bringing more protocols like mDNS to the mbed-os eco-system. This would make a win-win situation. Let me know your thoughts. Thanks.

@ifyall

This comment has been minimized.

Copy link

ifyall commented Aug 29, 2019

@SeppoTakalo and @maclobdell , I agree with @sathishm6 's analysis. If we want to clean up the Mbed API set to not have string-based APIs, we should do that. Adding this item now and then cleaning up Mbed APIs in general is a reasonable compromise between long term plans and short term feature enhancements of the platform.

Thanks,

Ian

@40Grit

This comment has been minimized.

Copy link

40Grit commented Aug 30, 2019

What effort is involved to provide an API that uses well defined types as described by @SeppoTakalo?

Can this wait till 5.14.1?

@cy-jayasankar cy-jayasankar force-pushed the cy-jayasankar:pr/added-ipv6-link-local-address-api branch from 7d82eda to b728142 Sep 25, 2019
@cy-jayasankar

This comment has been minimized.

Copy link
Author

cy-jayasankar commented Sep 25, 2019

@SeppoTakalo @maclobdell - I have pushed code for API changes as per the review comments. Can you please review the new changes.

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

SeppoTakalo commented Sep 25, 2019

This is now for @AnttiKauppila and @kjbracey-arm to review whether they are happy to support this new API on all other stacks than LwIP.

@@ -273,6 +273,52 @@ char *LWIP::Interface::get_interface_name(char *buf)
return buf;
}

static bool convert_lwip_addr_to_mbed(nsapi_addr_t *out, const ip_addr_t *in)

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Sep 25, 2019

Contributor

Why these have been moved from lwip_tools.cpp?

This comment has been minimized.

Copy link
@cy-jayasankar

cy-jayasankar Sep 27, 2019

Author

in older version of mbed-os this function was not available in lwit_tools.cpp. I have re-based the patch and removed this function from LWIPInterface.cpp file.

@@ -74,6 +74,23 @@ const ip_addr_t *LWIP::get_ipv4_addr(const struct netif *netif)
return NULL;
}

const ip_addr_t *LWIP::get_ipv6_link_local_addr(const struct netif *netif)

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Sep 25, 2019

Contributor

Why this is here, instead of LWIPStack.cpp where rest of the LWIP class members are.

This comment has been minimized.

Copy link
@cy-jayasankar

cy-jayasankar Sep 27, 2019

Author

get_ipv6_link_local_addr is a private function of the class. Public API's name is get_ipv6_link_local_address

nsapi_error_t EMACInterface::get_ipv6_link_local_address(SocketAddress *address)
{
if (_interface && _interface->get_ipv6_link_local_address(address)) {
return NSAPI_ERROR_PARAMETER;

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Sep 25, 2019

Contributor

You are translating all possible errors to NSAPI_ERROR_PARAMETER

This comment has been minimized.

Copy link
@cy-jayasankar

cy-jayasankar Sep 27, 2019

Author

pushed code to return the return value of _interface->get_ipv6_link_local_address


virtual nsapi_error_t get_ipv6_link_local_address(SocketAddress *address)
{
return NSAPI_ERROR_OK;

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Sep 25, 2019

Contributor

Should it be NSAPI_ERROR_UNSUPPORTED

This comment has been minimized.

Copy link
@cy-jayasankar

cy-jayasankar Sep 27, 2019

Author

Pushed code with this change

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Sep 25, 2019

One note:

Add API for IPv6 link local address.

Can we be more specific here ? Where we are adding this, how the API looks like ? This is in the release notes, it should be clear what is being added and what it does. can be one small paragraph describing this new API

@cy-jayasankar cy-jayasankar force-pushed the cy-jayasankar:pr/added-ipv6-link-local-address-api branch from fe8cd5d to d20f623 Oct 30, 2019
@cy-jayasankar

This comment has been minimized.

Copy link
Author

cy-jayasankar commented Oct 30, 2019

[Error] @0,0: L6218E: Undefined symbol Nanostack::Interface::get_ipv6_link_local_address(SocketAddress*) (referred from BUILD/tests/CY8CPROTO_064_SB/ARM/features/nanostack/mbed-mesh-api/source/LoWPANNDInterface.o).

Failures are related and for all targets, please fix

@0xc0170 Nanostack class is deriving OnboardNetworkStack class and get_ipv6_link_local_address was defined as pure virtual function in OnboardNetworkStack class. To fix the CI failures related to nanostack, I have changed get_ipv6_link_local_address to virtual function with default implementation in OnboardNetworkStack.h file and pushed the changes. Please trigger Ci and also let me know if you have any comments with this code change.

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 30, 2019

CI restarted

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Oct 30, 2019

Test run: FAILED

Summary: 3 of 4 test jobs failed
Build number : 5
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
@cy-jayasankar

This comment has been minimized.

Copy link
Author

cy-jayasankar commented Oct 31, 2019

@0xc0170 @michalpasztamobica from the failure log of the recent CI mbed-os-examples_MTB_ADV_WISE_1530_GCC_ARM.log looks like the errors are not related to this PR. Below are the errors copied from the log

  1. undefined reference to `virtual thunk to EMACInterface::get_netmask()'
  2. [Error] main.cpp@95,39: 'MBED_CONF_APP_WIFI_SSID' was not declared in this scope
  3. [Error] main.cpp@96,54: 'MBED_CONF_APP_WIFI_PASSWORD' was not declared in this scope
@michalpasztamobica

This comment has been minimized.

Copy link
Contributor

michalpasztamobica commented Oct 31, 2019

The linker errors are relevant. This PR changes the NetworkInterface and EMACInterface classes. We have some external libraries (for example for WISE_1530, like this one) which implement those interfaces and provide static libs with the implementation.
I found a PR from February where we were in the same place: #9387 and I can see @SeppoTakalo rebuilt the binaries back then.
I will use the same procedure, rebuild the libs and post them here for you to add to the PR.

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor

michalpasztamobica commented Oct 31, 2019

The new binaries are available here. @cy-jayasankar , please, put them on your branch and ping Martin when ready for another CI run.

I noticed that in February Seppo submitted more binaries, but those extra targets have been removed.

I checked that new binaries contain the new get_ipv6_link_local_address symbol. I also checked that the build failed for me locally with this PR until I got new binaries (I only checked ARM+ADV_WISE_1540, but I think this should be the same for other targets and compilers).

Jayasankar Nara
Protocols like mdns requires IPv6 link local address to be advertised in its
records (AAAA record). LWIP::Interface::bringup() API is creating IPv6 link
local address;But as of now there is no API exposed by mbed-os to get the
IPv6 link local address.

This new API is required to deliver mDNS library support on mbed-os for Cypress
platforms. Unit tested it by invoking get_ipv6_link_local_address with a simple
application.
@cy-jayasankar cy-jayasankar force-pushed the cy-jayasankar:pr/added-ipv6-link-local-address-api branch from d20f623 to cb51fa5 Oct 31, 2019
@cy-jayasankar

This comment has been minimized.

Copy link
Author

cy-jayasankar commented Oct 31, 2019

The new binaries are available here. @cy-jayasankar , please, put them on your branch and ping Martin when ready for another CI run.

I noticed that in February Seppo submitted more binaries, but those extra targets have been removed.

I checked that new binaries contain the new get_ipv6_link_local_address symbol. I also checked that the build failed for me locally with this PR until I got new binaries (I only checked ARM+ADV_WISE_1540, but I think this should be the same for other targets and compilers).

@michalpasztamobica thanks for sharing the new binaries. I have pushed these binaries to the PR. @0xc0170 Can you please trigger the CI.

@AnttiKauppila AnttiKauppila added needs: CI and removed needs: work labels Nov 1, 2019
@sathishm6

This comment has been minimized.

Copy link

sathishm6 commented Nov 4, 2019

@0xc0170 - I notice this is labeled for 6.0 RC release. Whereas we have been working-on to get this fix into mbed-os from 5.14.1 release. This is just an addition to existing set of IPv6 APIs, and not a new feature. Hence I feel this fix should be merged to mbed-os asap. Also this PR is currently blocking some of the software features we offer on mbed-os with our platforms, hence we would need this fix in mbed-os 5.14.2 release. (attn: @ifyall)

@AnttiKauppila - Thanks for reviewing this PR, I noticed you have also added a label 'needs: work'. Do you still see any rework pending on this PR? Let us know, so that we can address them before 5.14.2 code freeze date. Thanks.

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Nov 4, 2019

@0xc0170 - I notice this is labeled for 6.0 RC release. Whereas we have been working-on to get this fix into mbed-os from 5.14.1 release. This is just an addition to existing set of IPv6 APIs, and not a new feature. Hence I feel this fix should be merged to mbed-os asap. Also this PR is currently blocking some of the software features we offer on mbed-os with our platforms, hence we would need this fix in mbed-os 5.14.2 release. (attn: @ifyall)

We will sort out this label later today.

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Nov 4, 2019

Ci started

@sathishm6

This comment has been minimized.

Copy link

sathishm6 commented Nov 4, 2019

We will sort out this label later today.

Thanks @0xc0170 . Really appreciate your help.

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Nov 4, 2019

Test run: SUCCESS

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

@0xc0170 0xc0170 merged commit f27aec3 into ARMmbed:master Nov 4, 2019
26 checks passed
26 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8698 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8420B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Nov 4, 2019

Release version updated to 5.15

@sathishm6

This comment has been minimized.

Copy link

sathishm6 commented Nov 4, 2019

Release version updated to 5.15

Hello Martin (@0xc0170), why this fix can't make it in mbed-os 5.14.2 release. Could you please share the reason. What is the ETA for 5.15 release? Thanks.

@maclobdell

This comment has been minimized.

Copy link
Contributor

maclobdell commented Nov 4, 2019

@sathishm6 - changes to functionality, including extending APIs are intended for minor or major release versions. Patch releases are intended for bug fixes only. This is why this change is marked for 5.15, which is a minor release. In the future, the Mbed project plans to adopt a more flexible release schedule which will allow minor/major releases to happen more frequently if necessary.

@sathishm6

This comment has been minimized.

Copy link

sathishm6 commented Nov 5, 2019

@maclobdell - Thanks for the details. What is the ETA for 5.15 availability. Is there a possibility to make the next release as 5.15 release instead of 5.14.2. Is that a option? I want to know your suggestion and thoughts on this, so that, we can do the best to unblock our release, which would help us offer more software features to our customers on mbed-os platform. Let me know your suggestion. Thanks.

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Nov 7, 2019

As agreed, we are moving this to 5.14.2

@sathishm6

This comment has been minimized.

Copy link

sathishm6 commented Nov 7, 2019

Thanks @0xc0170 Martin !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.