Add API to get ipv6 link local address #11279

cy-jayasankar · 2019-08-21T12:25:04Z

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 · 2019-08-21T13:00:41Z

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

0xc0170

"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.

0xc0170 · 2019-08-27T18:51:26Z

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 · 2019-08-28T04:30:12Z

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.

bulislaw

LGTM @AnttiKauppila @SeppoTakalo please review

0xc0170 · 2019-08-29T07:16:04Z

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 · 2019-08-29T09:12:07Z

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 · 2019-08-29T09:16:10Z

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 · 2019-08-29T09:19:46Z

Removed 5.14 label, added needs: work

sathishm6 · 2019-08-29T11:35:07Z

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 · 2019-08-29T16:04:19Z

@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 · 2019-08-30T07:22:23Z

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 · 2019-09-25T11:47:11Z

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

SeppoTakalo · 2019-09-25T13:47:30Z

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.

SeppoTakalo · 2019-09-25T13:49:33Z

features/lwipstack/LWIPInterface.cpp

@@ -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)


Why these have been moved from lwip_tools.cpp?

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.

SeppoTakalo · 2019-09-25T13:52:21Z

features/lwipstack/lwip_tools.cpp

@@ -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)


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

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

SeppoTakalo · 2019-09-25T13:53:54Z

features/netsocket/EMACInterface.cpp

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


You are translating all possible errors to NSAPI_ERROR_PARAMETER

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

SeppoTakalo · 2019-09-25T13:54:31Z

features/netsocket/OnboardNetworkStack.h

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


Should it be NSAPI_ERROR_UNSUPPORTED

Pushed code with this change

0xc0170 · 2019-09-25T14:41:35Z

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

mbed-ci · 2019-10-30T10:57:00Z

Test run: FAILED

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

Failed test jobs:

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

cy-jayasankar · 2019-10-30T12:09:17Z

[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 · 2019-10-30T14:02:34Z

CI restarted

mbed-ci · 2019-10-30T14:38:43Z

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 · 2019-10-31T08:28:26Z

@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

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

michalpasztamobica · 2019-10-31T09:29:28Z

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 · 2019-10-31T19:44:14Z

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).

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 · 2019-10-31T22:19:20Z

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.

sathishm6 · 2019-11-04T03:22:09Z

@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 · 2019-11-04T08:43:19Z

@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 · 2019-11-04T08:43:35Z

Ci started

sathishm6 · 2019-11-04T08:47:34Z

We will sort out this label later today.

Thanks @0xc0170 . Really appreciate your help.

mbed-ci · 2019-11-04T11:15:17Z

Test run: SUCCESS

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

0xc0170 · 2019-11-04T15:03:01Z

Release version updated to 5.15

sathishm6 · 2019-11-04T15:07:06Z

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 · 2019-11-04T17:36:36Z

@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 · 2019-11-05T03:27:43Z

@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 · 2019-11-07T14:52:11Z

As agreed, we are moving this to 5.14.2

sathishm6 · 2019-11-07T15:29:23Z

Thanks @0xc0170 Martin !

ciarmcom requested review from a team August 21, 2019 13:00

ciarmcom added the needs: review label Aug 21, 2019

0xc0170 added the release-version: 5.14.0-rc1 label Aug 22, 2019

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

0xc0170 requested changes Aug 26, 2019

View reviewed changes

cy-jayasankar force-pushed the pr/added-ipv6-link-local-address-api branch from 7f145b1 to 7d82eda Compare August 27, 2019 06:57

0xc0170 self-requested a review August 27, 2019 18:50

bulislaw approved these changes Aug 28, 2019

View reviewed changes

0xc0170 added needs: work and removed release-version: 5.14.0-rc1 needs: review labels Aug 29, 2019

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

SeppoTakalo reviewed Sep 25, 2019

View reviewed changes

cy-jayasankar force-pushed the pr/added-ipv6-link-local-address-api branch from fe8cd5d to d20f623 Compare October 30, 2019 11:59

cy-jayasankar force-pushed the pr/added-ipv6-link-local-address-api branch from d20f623 to cb51fa5 Compare October 31, 2019 22:15

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

0xc0170 merged commit f27aec3 into ARMmbed:master Nov 4, 2019

0xc0170 added release-version: 5.15.0-rc1 and removed needs: CI release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 labels Nov 4, 2019

0xc0170 added release-version: 5.14.2 and removed release-version: 5.15.0-rc1 labels Nov 7, 2019

Add API to get ipv6 link local address #11279

Add API to get ipv6 link local address #11279

Uh oh!

Conversation

cy-jayasankar commented Aug 21, 2019 • edited by adbridge Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Reviewers

Release Notes

Uh oh!

ciarmcom commented Aug 21, 2019

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Aug 27, 2019

Uh oh!

cy-jayasankar commented Aug 28, 2019

Uh oh!

bulislaw left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Aug 29, 2019

Uh oh!

SeppoTakalo commented Aug 29, 2019

Uh oh!

SeppoTakalo commented Aug 29, 2019

Uh oh!

0xc0170 commented Aug 29, 2019

Uh oh!

sathishm6 commented Aug 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ifyall commented Aug 29, 2019

Uh oh!

40Grit commented Aug 30, 2019

Uh oh!

cy-jayasankar commented Sep 25, 2019

Uh oh!

SeppoTakalo commented Sep 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Sep 25, 2019

Uh oh!

mbed-ci commented Oct 30, 2019

Uh oh!

cy-jayasankar commented Oct 30, 2019

Uh oh!

0xc0170 commented Oct 30, 2019

Uh oh!

mbed-ci commented Oct 30, 2019

Uh oh!

cy-jayasankar commented Oct 31, 2019

Uh oh!

michalpasztamobica commented Oct 31, 2019

Uh oh!

michalpasztamobica commented Oct 31, 2019

Uh oh!

cy-jayasankar commented Oct 31, 2019

Uh oh!

sathishm6 commented Nov 4, 2019

Uh oh!

0xc0170 commented Nov 4, 2019

Uh oh!

cy-jayasankar commented Aug 21, 2019 •

edited by adbridge

Loading

sathishm6 commented Aug 29, 2019 •

edited

Loading

sathishm6 commented Nov 5, 2019 •

edited

Loading