Skip to content

Add multicast implementation for UDPSocket #4820

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

Closed
wants to merge 2 commits into from
Closed

Add multicast implementation for UDPSocket #4820

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c411cd7
Add multicast implementation for UDPSocket
kegilbert Jul 21, 2017
283024b
Address review comments in regards to IPv6 support and add leave grou…
kegilbert Aug 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions features/FEATURE_LWIP/lwip-interface/lwip_stack.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
#include "lwip/mld6.h"
#include "lwip/dns.h"
#include "lwip/udp.h"
#include "lwip_errno.h"
#include "netif/lwip_ethernet.h"
#include "emac_api.h"
#include "ppp_lwip.h"
Expand Down Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

struct in_addr inet_addr_convert;
// nsapi_addr_t stores IP address in byte array
// Convert byte aray -> uint32_t for lwip inet IP representation
ip_addr_t multiaddr_ip;
if (!convert_mbed_addr_to_lwip(&multiaddr_ip, nsapi_addr_ip)) {
return NSAPI_ERROR_PARAMETER;
}

inet_addr_convert.s_addr = multiaddr_ip.addr;

#if LWIP_IPV4
// LWIP inet IP representation (in_addr) to ip_addr_t, both structs containing
// uint32_t, this step just remaps the ip address for the igmp calls
lwip_ip->addr = inet_addr_convert.s_addr;
#endif
return NSAPI_ERROR_OK;
}

static nsapi_error_t mbed_lwip_setsockopt(nsapi_stack_t *stack, nsapi_socket_t handle, int level, int optname, const void *optval, unsigned optlen)
{
struct lwip_socket *s = (struct lwip_socket *)handle;
Expand Down Expand Up @@ -1023,6 +1043,35 @@ 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_IPV4
if (optlen != sizeof(nsapi_ip_mreq_t)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

return NSAPI_ERROR_UNSUPPORTED;
}

err_t igmp_err;
nsapi_ip_mreq_t *imr = optval;

ip_addr_t if_addr;
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);
Copy link
Contributor

@kjbracey kjbracey Sep 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.


if (optname == NSAPI_ADD_MEMBERSHIP) {
igmp_err = igmp_joingroup(&if_addr, &multi_addr);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

return mbed_lwip_err_remap(EADDRNOTAVAIL);
}
return 0;
#endif
return NSAPI_ERROR_UNSUPPORTED;
}

default:
return NSAPI_ERROR_UNSUPPORTED;
}
Expand Down
21 changes: 21 additions & 0 deletions features/netsocket/UDPSocket.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,27 @@ nsapi_protocol_t UDPSocket::get_proto()
return NSAPI_UDP;
}

int UDPSocket::modify_multicast_group(const SocketAddress &address, nsapi_socket_option_t socketopt)
{
nsapi_ip_mreq_t mreq;

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

Choose a reason for hiding this comment

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

nsapi_addr_t() would be simpler?

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


return this->setsockopt(0, socketopt, &mreq, sizeof(mreq));
}

int UDPSocket::join_multicast_group(const SocketAddress &address)
{
return modify_multicast_group(address, NSAPI_ADD_MEMBERSHIP);
}

int UDPSocket::leave_multicast_group(const SocketAddress &address)
{
return modify_multicast_group(address, NSAPI_DROP_MEMBERSHIP);
}

nsapi_size_or_error_t UDPSocket::sendto(const char *host, uint16_t port, const void *data, nsapi_size_t size)
{
SocketAddress address;
Expand Down
16 changes: 16 additions & 0 deletions features/netsocket/UDPSocket.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,21 @@ class UDPSocket : public Socket {
*/
virtual ~UDPSocket();

/** Subscribes to an IP multicast group
*
* @param address Multicast group IP address
* @return Negative error code on failure
*/
int join_multicast_group(const SocketAddress &address);

/** Leave an IP multicast group
*
* @param address Multicast group IP address
* @return Negative error code on failure
*/
int leave_multicast_group(const SocketAddress &address);


/** Send a packet over a UDP socket
*
* Sends data to the specified address specified by either a domain name
Expand Down Expand Up @@ -115,6 +130,7 @@ class UDPSocket : public Socket {
protected:
virtual nsapi_protocol_t get_proto();
virtual void event();
int modify_multicast_group(const SocketAddress &address, nsapi_socket_option_t socketopt);

volatile unsigned _pending;
rtos::Semaphore _read_sem;
Expand Down
35 changes: 22 additions & 13 deletions features/netsocket/nsapi_types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ typedef void *nsapi_socket_t;
/** Enum of socket protocols
*
* The socket protocol specifies a particular protocol to
* be used with a newly created socket.
* be used with a newly created socket.
*
* @enum nsapi_protocol
*/
Expand Down Expand Up @@ -207,13 +207,15 @@ typedef enum nsapi_socket_level {
* @enum nsapi_socket_option
*/
typedef enum nsapi_socket_option {
NSAPI_REUSEADDR, /*!< Allow bind to reuse local addresses */
NSAPI_KEEPALIVE, /*!< Enables sending of keepalive messages */
NSAPI_KEEPIDLE, /*!< Sets timeout value to initiate keepalive */
NSAPI_KEEPINTVL, /*!< Sets timeout value for keepalive */
NSAPI_LINGER, /*!< Keeps close from returning until queues empty */
NSAPI_SNDBUF, /*!< Sets send buffer size */
NSAPI_RCVBUF, /*!< Sets recv buffer size */
NSAPI_REUSEADDR, /*!< Allow bind to reuse local addresses */
NSAPI_KEEPALIVE, /*!< Enables sending of keepalive messages */
NSAPI_KEEPIDLE, /*!< Sets timeout value to initiate keepalive */
NSAPI_KEEPINTVL, /*!< Sets timeout value for keepalive */
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 */
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

NSAPI_DROP_MEMBERSHIP, /*!< Drop membership to multicast address */
} nsapi_socket_option_t;

/* Backwards compatibility - previously didn't distinguish stack and socket options */
Expand Down Expand Up @@ -254,6 +256,13 @@ typedef struct nsapi_stack {
unsigned _stack_buffer[16];
} nsapi_stack_t;

/** nsapi_ip_mreq structure
*/
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 */
Copy link
Contributor

@kjbracey kjbracey Aug 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

} nsapi_ip_mreq_t;

/** nsapi_stack_api structure
*
* Common api structure for network stack operations. A network stack
Expand All @@ -275,9 +284,9 @@ typedef struct nsapi_stack_api
*
* The hostname may be either a domain name or an IP address. If the
* hostname is an IP address, no network transactions will be performed.
*
*
* If no stack-specific DNS resolution is provided, the hostname
* will be resolve using a UDP socket on the stack.
* will be resolve using a UDP socket on the stack.
*
* @param stack Stack handle
* @param addr Destination for the host IP address
Expand Down Expand Up @@ -322,7 +331,7 @@ typedef struct nsapi_stack_api
* @param optval Destination for option value
* @param optlen Length of the option value
* @return 0 on success, negative error code on failure
*/
*/
nsapi_error_t (*getstackopt)(nsapi_stack_t *stack, int level,
int optname, void *optval, unsigned *optlen);

Expand Down Expand Up @@ -523,7 +532,7 @@ typedef struct nsapi_stack_api
* @param optval Option value
* @param optlen Length of the option value
* @return 0 on success, negative error code on failure
*/
*/
nsapi_error_t (*setsockopt)(nsapi_stack_t *stack, nsapi_socket_t socket, int level,
int optname, const void *optval, unsigned optlen);

Expand All @@ -540,7 +549,7 @@ typedef struct nsapi_stack_api
* @param optval Destination for option value
* @param optlen Length of the option value
* @return 0 on success, negative error code on failure
*/
*/
nsapi_error_t (*getsockopt)(nsapi_stack_t *stack, nsapi_socket_t socket, int level,
int optname, void *optval, unsigned *optlen);
} nsapi_stack_api_t;
Expand Down