Add multicast implementation for UDPSocket #4820

kegilbert · 2017-07-26T23:48:24Z

First pass at adding multicast support to mbed-os from previous implementations. This PR is in relation to the following issue: #3512

I have not yet added in the membership register/deregister steps, working on porting those now from lwip_socket.

Tested on a K64F with GCC_ARM over ethernet with the following setup:

#include "mbed.h"                                                            
#include "EthernetInterface.h"                                               
                                                                             
int MCAST_PORT = 5007;                                                                                                      
                                                                             
int main() {                                                                 
    EthernetInterface eth;                                                   
    eth.connect();                                                           
                                                                             
    UDPSocket server;                                                        
    server.open(&eth);                                                       
    server.bind(MCAST_PORT);                                                 
    int ret = server.join_multicast_group("239.0.0.1");                       
    if (ret != 0) {                                                          
        printf("Error joining the multicast group, error: %d\n", ret);       
        while (true) {}                                                      
    }                                                                        
                                                                             
    SocketAddress client;                                                    
    char buffer[256];                                                        
    while (true) {                                                           
        printf("\nWait for packet...\n");                                    
        int n = server.recvfrom(&client, buffer, sizeof(buffer));            
        printf("Packet from \"%s\": %s\n", client.get_ip_address(), buffer); 
    }                                                                        
}

theotherjimmy · 2017-07-27T14:24:41Z

@kegilbert Please put only one of those tags on at a time.

kegilbert · 2017-07-27T18:38:27Z

cc @geky

geky

Looks good so far, several nits and comments, but it looks like it's coming along.

Also ccing @kjbracey-arm

geky · 2017-07-27T19:22:45Z

features/netsocket/nsapi_types.h

+typedef struct nsapi_ip_mreq
+{
+    nsapi_addr_t imr_multiaddr; /* IP multicast address of group */
+    uint32_t     imr_interface; /* local IP address of interface */


@kegilbert, shouldn't imr_interface also be a nsapi_addr_t?

geky · 2017-07-27T19:25:21Z

features/netsocket/nsapi_types.h

+/** nsapi_ip_mreq structure
+ */
+typedef struct nsapi_ip_mreq
+{


nit: this should match the style of the surrounding source (braces)

geky · 2017-07-27T19:28:01Z

features/netsocket/UDPSocket.cpp

+
+    // Set up group address
+    mreq.imr_multiaddr = address->get_addr();
+    mreq.imr_interface = ((uint32_t)0x00000000UL);


Could this be

mreq.imr_interface = SocketAddress().get_addr();

geky · 2017-07-27T19:29:25Z

features/FEATURE_LWIP/lwip-interface/lwip_stack.c

+            if (optname == NSAPI_ADD_MEMBERSHIP) {
+                igmp_err = igmp_joingroup(&if_addr, &multi_addr);
+            } else {
+              igmp_err = igmp_leavegroup(&if_addr, &multi_addr);


small nit: the tabbing here looks a bit off

geky · 2017-07-27T19:32:05Z

features/netsocket/UDPSocket.h

@@ -56,6 +56,8 @@ class UDPSocket : public Socket {
     */
    virtual ~UDPSocket();

+    int join_multicast_group(SocketAddress* address);


Needs documentation?

Also SocketAddress is a first class object, so it should be passed around by value or reference. The should be

int join_multicast_group(const SocketAddress &address);

That way you can pass implicitly cast strings

TCPSocket s s.join_multicast_group("250.0.0.10");

geky

Looks good to me 👍

kegilbert · 2017-07-28T16:08:32Z

@geky mbed OS 2 used an array in the lwip_socket layer to keep track of multicast sockets. Besides the register and unregister membership function which added/removed sockets from the array, I can't find any other references to indicate who used that list of sockets. That implementation is currently missing from this PR. I mentioned it in the original comment, so just wanted to address that. If you or anyone else knows more about where that list was used I can add it to the ADD/DROP_MEMBERSHIP case otherwise I'll leave it as is.

geky · 2017-07-28T16:21:48Z

Hmm, I can't really find any consumers for that array. My guess is it's old cruft from a previous port of lwip. It's not currently in mbed OS so it's probably fine to move forward.

kegilbert · 2017-07-28T19:41:31Z

NOTE This implementation is not designed for IPv6. There was a change in how multicast works in IPv6 and has not yet been fully tested. Will move any IPv6 work that may be needed to another PR to keep this one minimal.

0xc0170 · 2017-08-02T17:32:48Z

Please rebase, to resolve travis failure. Is this ready then for review?

kegilbert · 2017-08-03T15:49:51Z

@0xc0170 Rebased. It is ready for review.

0xc0170 · 2017-08-07T13:45:07Z

@kjbracey-arm Can you please review?

0xc0170 · 2017-08-07T13:55:00Z

features/FEATURE_LWIP/lwip-interface/lwip_stack.c

+    inet_addr_convert.s_addr = multiaddr_ip.addr;
+
+#if LWIP_IPV4
+    // LWIP inet IP represnetation (in_addr) to ip_addr_t, both structs containing


representation typo in the comment

Woops, good catch thanks!

0xc0170 · 2017-08-07T13:55:26Z

features/netsocket/UDPSocket.cpp

@@ -33,6 +33,16 @@ nsapi_protocol_t UDPSocket::get_proto()
    return NSAPI_UDP;
 }

+int UDPSocket::join_multicast_group(const SocketAddress &address) {


follow the style that is in this file (codebase) { on the new line

int UDPSocket::join_multicast_group(const SocketAddress &address) {

kegilbert · 2017-08-18T19:41:12Z

@kjbracey-arm @0xc0170 Was there anything you wanted me to take care of for this review?

0xc0170 · 2017-08-21T11:30:36Z

@kjbracey-arm @0xc0170 Was there anything you wanted me to take care of for this review?

Someone from networking team to review besides Chris (thx for review 👍 ). @mikaleppanen @hasnainvirk I assume kevin is still not back, can you or tag someone who could review this?

kjbracey · 2017-08-21T11:47:28Z

I'm back today. I'll leave a few comments.

kjbracey · 2017-08-21T11:49:09Z

features/netsocket/nsapi_types.h

+ */
+typedef struct nsapi_ip_mreq {
+    nsapi_addr_t imr_multiaddr; /* IP multicast address of group */
+    nsapi_addr_t imr_interface; /* local IP address of interface */


The standard ipv6_mreq uses an interface ID rather than an address to indirectly name it.

If this structure is intended for use by both IPv4 and IPv6, could we use an actual NetworkInterface * pointer instead (given that we don't have integer interface IDs)? Easier to get from the interface to the address if needed than the other way around.

This PR is aiming to add IPv4 support only. I can look into changing this to make the transition to IPv6 support easier if you'd rather get it down now but left as is for the moment.

The current implementation supports IPv4 and IPv6 equally. I don't think a PR that leaves IPv6 support less functional than IPv4 support is acceptable. Regardless, if you're adding IPv4 something you need to think what the IPv6 will look like.

kjbracey · 2017-08-21T11:51:51Z

features/FEATURE_LWIP/lwip-interface/lwip_stack.c

@@ -1023,6 +1043,36 @@ static nsapi_error_t mbed_lwip_setsockopt(nsapi_stack_t *stack, nsapi_socket_t h
            }
            return 0;

+        case NSAPI_ADD_MEMBERSHIP:
+        case NSAPI_DROP_MEMBERSHIP: {
+#if LWIP_IPV6


This flag is not an either-or, so the #if/#else is inappropriate. Both LWIP_IPV4 and LWIP_IPV6 can be enabled.

If this option will be for IPv4 only, then just wrap the whole case in #if LWIP_IPv4.

If this option is for either IPv4 or 6, then you would need to have a bit of messing around to check the address type, with a bunch of separate `#if's. Search the code for examples.

kjbracey · 2017-08-21T11:58:10Z

features/netsocket/nsapi_types.h

+    NSAPI_LINGER,            /*!< Keeps close from returning until queues empty */
+    NSAPI_SNDBUF,            /*!< Sets send buffer size */
+    NSAPI_RCVBUF,            /*!< Sets recv buffer size */
+    NSAPI_ADD_MEMBERSHIP,    /*!< Add membership to multicast address */


I'm beginning to hurt a little about the lack of prefixes or indeed proper levels here. The earlier ones should all have been TCP_. Can these at least be IP_?

Do we think we use a single socket option for both IPv4 and IPv6, as the nsapi_addr_t inside the mreq contains a version number? Is that your intent here? (Systems normally have separate IP_ADD_MEMBERSHIP and IPV6_JOIN_GROUP/IPV6_ADD_MEMBERSHIP.)

This PR is currently only for IPv4 support. This would be a good point for the IPv6 support PR.

I can change it to IP_ if you feel strongly for it, but my preference is to keep new additions consistent with our current naming if we can't modify all of them to TCP_/IP_.

kjbracey · 2017-08-21T12:01:38Z

features/netsocket/UDPSocket.cpp

+    mreq.imr_multiaddr = address.get_addr();
+    mreq.imr_interface = SocketAddress().get_addr();   // 0.0.0.0 INADDR_ANY
+
+    return this->setsockopt(0, NSAPI_ADD_MEMBERSHIP, &mreq, sizeof(mreq));


_stack as first parameter, no?

This calls into Socket's setsockopt function, first param is level:
See here

My mistake. In that case, it should be NSAPI_SOCKET (indicating that it's a generic NSAPI option, rather than a stack-specific one that should be passed through).

lwIP has no "native" socket options, so its adaptation layer gets away with ignoring the level parameter, but other stacks will need that.

kjbracey · 2017-08-21T12:09:09Z

features/netsocket/UDPSocket.cpp

@@ -33,6 +33,17 @@ nsapi_protocol_t UDPSocket::get_proto()
    return NSAPI_UDP;
 }

+int UDPSocket::join_multicast_group(const SocketAddress &address)


Is this needed in addition to the socket option? Having both feels kind of the worst of both worlds - the socket option switch statement has the disadvantage (in an embedded system) that it pulls in all the stack code at link time, even for cases that are never invoked. This is a neat separate API that would allow linker exclusion, but then goes through the switch anyway.

Don't feel terribly strongly, except maybe you should have the leave function to match if you have this.

Not necessarily, although I prefer this style since it looks a lot cleaner to a user and mimics what we used to have.

kjbracey · 2017-08-21T12:11:36Z

features/FEATURE_LWIP/lwip-interface/lwip_stack.c

+            }
+
+            err_t igmp_err;
+            nsapi_ip_mreq_t *imr = (nsapi_ip_mreq_t *)optval;


Cast not required cos it's C. Its removal will encourage you to preserve const-correctness.

kjbracey · 2017-08-21T12:14:12Z

features/FEATURE_LWIP/lwip-interface/lwip_stack.c

+            } else {
+                igmp_err = igmp_leavegroup(&if_addr, &multi_addr);
+            }
+            if (igmp_err != ERR_OK) {


I suggest return mbed_lwip_err_remap(igmp_err);, if that returns normal error codes

It would default to NSAPI_ERROR_DEVICE_ERROR. If that's fine I can change it.

If I read correctly, the only current possible error return is ERR_VAL, which maps NSAPI_ERROR_PARAMETER.

…p function

0xc0170 · 2017-09-01T10:35:20Z

@kjbracey-arm There was an update, can you review pls?

@kegilbert It's helpful to bump this with an update description (what has changed since the last review.

kjbracey

Still not happy with this unless it supports IPv6 - it currently doesn't even correctly handle the various compilation possibilities for dual stack.

Added a suggested rework of the core bit that should cover IPv4 and IPv6 correctly (I hope).

kjbracey · 2017-09-01T10:42:09Z

features/FEATURE_LWIP/lwip-interface/lwip_stack.c

@@ -980,6 +981,25 @@ static nsapi_size_or_error_t mbed_lwip_socket_recvfrom(nsapi_stack_t *stack, nsa
    return recv;
 }

+static nsapi_error_t nsapi_addr_to_ip_addr(ip_addr_t *lwip_ip, nsapi_addr_t *nsapi_addr_ip) {


I can't quite figure out what this function is doing, and why you're not just calling convert_mbed_addr_to_lwip directly. It's trying to fill in only part of the destination address structure? Is it covering up lack of proper IPv4/IPv6 support?

kjbracey · 2017-09-01T10:50:46Z

features/FEATURE_LWIP/lwip-interface/lwip_stack.c

+        case NSAPI_ADD_MEMBERSHIP:
+        case NSAPI_DROP_MEMBERSHIP: {
+#if LWIP_IPV4
+            if (optlen != sizeof(nsapi_ip_mreq_t)) {


If your option structure contains nsapi_addr_ts, then you also need a check somewhere that the addresses are IPv4.

And in that case you may as well include the code to call mld6_joingroup instead for IPv6. Having the logic to correctly detect and error when passed IPv6 addresses, and dealing with all 3 compilation modes (4 only, 6 only, 4+6) is no simpler than handling the IPv6.

I'm currently thinking we should keep the mreq structure as is, so it contains nsapi_addr_t with the type field, and then you should do the right IPv4/IPv6 thing depending on the address type indicated in the group.

kjbracey · 2017-09-01T10:55:07Z

features/FEATURE_LWIP/lwip-interface/lwip_stack.c

+            nsapi_addr_to_ip_addr(&if_addr, &imr->imr_interface);
+
+            if (optname == NSAPI_ADD_MEMBERSHIP) {
+                igmp_err = igmp_joingroup(&if_addr, &multi_addr);


Don't see how this compiles if LWIP_IPV6 is on as well as LWIP_IPV4 - you need to use ip_2_ip4 to convert the ip_addr_t * to ip_addr4_t *.

kjbracey · 2017-09-01T11:25:21Z

features/netsocket/UDPSocket.cpp

+
+    // Set up group address
+    mreq.imr_multiaddr = address.get_addr();
+    mreq.imr_interface = SocketAddress().get_addr();   // 0.0.0.0 INADDR_ANY


nsapi_addr_t() would be simpler?

Comment is misleading - it's an "NSAPI_UNSPEC" address, rather than IPv4 0.0.0.0.

kjbracey · 2017-09-01T11:46:23Z

features/FEATURE_LWIP/lwip-interface/lwip_stack.c

+            ip_addr_t multi_addr;
+
+            nsapi_addr_to_ip_addr(&multi_addr, &imr->imr_multiaddr);
+            nsapi_addr_to_ip_addr(&if_addr, &imr->imr_interface);


For full IPv4/IPv6 correctness, I think you need to make sure that if multi_addr.version is NSAPI_UNSPEC, you produce an ip_addr_t of the correct type using ip_addr_set_zero_ip4 or ip_addr_set_zero_ip6.

I think I'd restructure like:

/* Check interface address type matches group, or is unspecified */ if (imr->imr_interface.version != NSAPI_UNSPEC && imr->imr_interface.version != imr->imr_multiaddr.version) { return NSAPI_ERROR_PARAMETER; } ip_addr_t if_addr; ip_addr_t multi_addr; /* Convert the group address */ if (!convert_mbed_addr_to_lwip(&multi_addr, &imr->imr_multiaddr)) { return NSAPI_ERROR_PARAMETER; } /* Convert the interface address, or make sure it's the correct sort of "any" */ if (imr->imr_interface.version != NSAPI_UNSPEC) { if (!convert_mbed_addr_to_lwip(&if_addr, &imr->imr_interface)) { return NSAPI_ERROR_PARAMETER; } } else { ip_addr_set_any(IP_IS_V6(if_addr), &imr->imr_interface) } igmp_err = NSAPI_ERROR_UNSUPPORTED; #if LWIP_IPV4 if (IP_IS_V4(if_addr)) { igmp_err = (NSAPI_ADD_MEMBERSHIP ? igmp_joingroup : igmp_leavegroup) (ip_2_ip4(&if_addr), ip_2_ip4(&multi_addr)); } #endif #if LWIP_IPV6 if (IP_IS_V6(if_addr)) { igmp_err = (NSAPI_ADD_MEMBERSHIP ? mld6_joingroup : mld6_leavegroup) (ip_2_ip6(&if_addr), ip_2_ip6(&multi_addr)); } #endif

Untested, but I think should cover all the compilation/runtime permutations of IPv4/IPv6.

kjbracey · 2017-09-01T11:52:51Z

cc @mikaleppanen to sanity-check my suggestions

theotherjimmy · 2017-09-25T15:17:41Z

@kegilbert @kjbracey-arm What's going on with this PR?

kegilbert · 2017-09-25T23:34:13Z

@theotherjimmy
Sorry for the long delay, reworked the multicast implementation with @kjbracey-arm's suggestions to support IPv6. Put up a new PR with the changes here: #5196

0xc0170 · 2017-10-05T07:51:00Z

Sorry for the long delay, reworked the multicast implementation with @kjbracey-arm's suggestions to support IPv6. Put up a new PR with the changes here: #5196

Closing this one then

kegilbert added needs: CI needs: review needs: work labels Jul 26, 2017

theotherjimmy removed needs: CI needs: work labels Jul 27, 2017

geky reviewed Jul 27, 2017

View reviewed changes

geky approved these changes Jul 27, 2017

View reviewed changes

kegilbert force-pushed the multicast branch from ee882eb to 1d35ca1 Compare July 27, 2017 21:10

geky mentioned this pull request Jul 27, 2017

Where is the old 'net' library? #4818

Closed

kegilbert force-pushed the multicast branch from 48a689e to e797cda Compare July 28, 2017 19:00

kegilbert added needs: squashing and removed needs: squashing labels Jul 28, 2017

kegilbert force-pushed the multicast branch from e797cda to 445a209 Compare July 29, 2017 00:01

kegilbert force-pushed the multicast branch from 445a209 to 0c00ff1 Compare August 3, 2017 15:49

0xc0170 reviewed Aug 7, 2017

View reviewed changes

Add multicast implementation for UDPSocket

c411cd7

kegilbert force-pushed the multicast branch from 0c00ff1 to c411cd7 Compare August 7, 2017 16:41

kjbracey reviewed Aug 21, 2017

View reviewed changes

theotherjimmy added the needs: work label Aug 21, 2017

theotherjimmy removed the needs: review label Aug 21, 2017

Address review comments in regards to IPv6 support and add leave grou…

283024b

…p function

0xc0170 added needs: review and removed needs: work labels Sep 1, 2017

kjbracey suggested changes Sep 1, 2017

View reviewed changes

kjbracey mentioned this pull request Oct 4, 2017

Make our own multicast group and join it. ARMmbed/mbed-os-example-mesh-minimal#137

Merged

0xc0170 closed this Oct 5, 2017

0xc0170 removed the needs: review label Oct 5, 2017

Add multicast implementation for UDPSocket #4820

Add multicast implementation for UDPSocket #4820

Uh oh!

Conversation

kegilbert commented Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theotherjimmy commented Jul 27, 2017

Uh oh!

kegilbert commented Jul 27, 2017

Uh oh!

geky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geky Jul 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

geky left a comment

Choose a reason for hiding this comment

Uh oh!

kegilbert commented Jul 28, 2017

Uh oh!

geky commented Jul 28, 2017

Uh oh!

kegilbert commented Jul 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Aug 2, 2017

Uh oh!

kegilbert commented Aug 3, 2017

Uh oh!

0xc0170 commented Aug 7, 2017

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!

kegilbert commented Aug 18, 2017

Uh oh!

0xc0170 commented Aug 21, 2017

Uh oh!

kjbracey commented Aug 21, 2017

Uh oh!

kjbracey Aug 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

kjbracey Aug 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjbracey Aug 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kegilbert commented Jul 26, 2017 •

edited

Loading

geky Jul 27, 2017 •

edited

Loading

kegilbert commented Jul 28, 2017 •

edited

Loading

kjbracey Aug 21, 2017 •

edited

Loading

kjbracey Aug 22, 2017 •

edited

Loading

kjbracey Aug 21, 2017 •

edited

Loading

kjbracey Sep 1, 2017 •

edited

Loading