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

[Link layer] GNRC PPP #5470

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
3 changes: 2 additions & 1 deletion drivers/include/net/netdev2.h
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,8 @@ enum {
NETDEV2_TYPE_ETHERNET,
NETDEV2_TYPE_IEEE802154,
NETDEV2_TYPE_CC110X,
NETDEV2_TYPE_NRFMIN
NETDEV2_TYPE_NRFMIN,
NETDEV2_TYPE_PPP
};

/**
Expand Down
111 changes: 111 additions & 0 deletions drivers/include/net/netdev2/ppp.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
/*
* Copyright (C) 2016 Fundación Inria Chile
*
* This file is subject to the terms and conditions of the GNU Lesser
* General Public License v2.1. See the file LICENSE in the top level
* directory for more details.
*/

/**
* @ingroup drivers_netdev_netdev2
* @brief
* @{
*
* @file
* @brief Definitions for netdev2 common Point to Point Protocol code
*
* @author José Ignacio Alamos
*/
#ifndef NETDEV2_PPP_H
#define NETDEV2_PPP_H

#include "net/gnrc/ppp/prot.h"
#include "net/gnrc/ppp/prot.h"
#include "net/gnrc/ppp/lcp.h"
#include "net/gnrc/ppp/pap.h"
#include "net/gnrc/ppp/ipcp.h"
Copy link
Member

Choose a reason for hiding this comment

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

With all these separate headers: may I propose a net/gnrc/ppp.h header that includes all these headers (there you can also @defgroup net_gnrc_ppp instead of duplicate define it multiple times [see below])

#include "net/gnrc/nettype.h"
#include "net/netopt.h"
#include "net/netdev2.h"

#ifdef __cplusplus
extern "C" {
#endif

#if defined(MODULE_GNRC_PPP) || doxygen
Copy link
Member

Choose a reason for hiding this comment

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

defined(DOXYGEN)

/**
* @brief class of custom driver control protocol
* @extends ppp_protocol_t
*
* @details the DCP is in charge of monitoring the link and exchanging messages with the ppp device
*/
typedef struct gnrc_ppp_dcp {
gnrc_ppp_protocol_t prot; /**< base ppp_protocol_t object */
msg_t timer_msg; /**< msg struct for handling timeouts messages */
Copy link
Member

Choose a reason for hiding this comment

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

Please align doc

xtimer_t xtimer; /**< xtimer struct for sending timeout messages */
uint8_t dead_counter; /**< when reaches zero, the link is assumed to be dead */
} gnrc_ppp_dcp_t;
Copy link
Member

Choose a reason for hiding this comment

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

Why define that here and not in sys/include/net/gnrc/ppp/dcp.h?

#endif


/**
* @brief Extended structure to hold PPP driver state
*
* @extends netdev2_t
*
* Supposed to be extended by driver implementations.
* The extended structure should contain all variable driver state.
*/
typedef struct {
netdev2_t netdev; /**< @ref netdev2_t base class */
#if defined(MODULE_GNRC_PPP) || doxygen
Copy link
Member

Choose a reason for hiding this comment

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

defined(DOXYGEN)

/**
* @brief PPP specific fields
* @{
*/
gnrc_ppp_dcp_t dcp; /**< Control protocol for driver */
Copy link
Member

Choose a reason for hiding this comment

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

Please align doc

gnrc_ppp_lcp_t lcp; /**< Link Control Protocol */
gnrc_ppp_pap_t pap; /**< Password Authentication Protocol */
gnrc_ppp_ipcp_t ipcp; /**< IPv4 Network Control Protocol */
gnrc_ppp_ipv4_t ipv4; /**< Handler for IPv4 packets */
Copy link
Member

Choose a reason for hiding this comment

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

You cast these to gnrc_ppp_protocol_t quite often in your code (https://github.com/RIOT-OS/RIOT/pull/5470/files#diff-6d2b26dab0a8a164f332c85c0910c139R35), but e.g. sizeof(gnrc_ppp_dcp_t) < sizeof(gnrc_ppp_protocol), so you will access data after netdev2_ppp_t.dcp (something in netdev2_ppp_t.lcp in this case).

#endif
} netdev2_ppp_t;


/**
* @brief Fallback function for netdev2 PPP devices' _get function
*
* Supposed to be used by netdev2 drivers as default case.
*
* @param[in] dev network device descriptor
* @param[in] opt option type
* @param[out] value pointer to store the option's value in
* @param[in] max_len maximal amount of byte that fit into @p value
*
* @return number of bytes written to @p value
* @return <0 on error
*/
int netdev2_ppp_get(netdev2_ppp_t *dev, netopt_t opt, void *value,
size_t max_len);

/**
* @brief Fallback function for netdev2 PPP devices' _set function
*
*
* @param[in] dev network device descriptor
* @param[in] opt option type
* @param[in] value value to set
* @param[in] value_len the length of @p value
*
* @return number of bytes used from @p value
* @return <0 on error
*/
int netdev2_ppp_set(netdev2_ppp_t *dev, netopt_t opt, void *value,
size_t value_len);

#ifdef __cplusplus
}
#endif

#endif /* NETDEV2_PPP_H */
/** @} */
1 change: 1 addition & 0 deletions drivers/netdev2_ppp/Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
include $(RIOTBASE)/Makefile.base
88 changes: 88 additions & 0 deletions drivers/netdev2_ppp/netdev2_ppp.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
/*
* Copyright (C) Fundación Inria Chile 2017
*
* This file is subject to the terms and conditions of the GNU Lesser
* General Public License v2.1. See the file LICENSE in the top level
* directory for more details.
*/

/**
* @ingroup driver_netdev2_ppp
Copy link
Member

Choose a reason for hiding this comment

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

This group does not exist. Also it doesn't make sense to do this in a c-file, so please remove this line

* @{
*
* @file
* @brief Common code for netdev2 ppp drivers
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this line

*
* @author José Ignacio Alamos <jose.alamos@inria.cl>
*
* @}
*/

#include <assert.h>
#include <errno.h>

#include "net/netdev2.h"
#include "net/netdev2/ppp.h"

#define ENABLE_DEBUG (0)
#include "debug.h"

int netdev2_ppp_get(netdev2_ppp_t *dev, netopt_t opt, void *value, size_t max_len)
{
int res;
switch (opt) {
case NETOPT_PPP_LCP_STATE:
*((uint8_t *) value) = ((gnrc_ppp_protocol_t*) &dev->lcp)->state;
res = 0;
break;
case NETOPT_PPP_AUTH_STATE:
*((uint8_t *) value) = ((gnrc_ppp_protocol_t*) &dev->pap)->state;
res = 0;
break;
case NETOPT_PPP_IPCP_STATE:
*((uint8_t *) value) = ((gnrc_ppp_protocol_t*) &dev->ipcp)->state;
res = 0;
break;
case NETOPT_PPP_IS_IPV6_READY:
res = ((gnrc_ppp_protocol_t*) &dev->ipv4)->state == PROTOCOL_UP;
*((uint8_t *) value) = res;
break;
case NETOPT_DEVICE_TYPE:
*((uint16_t*) value) = NETDEV2_TYPE_PPP;
res = 2;
break;
default:
return -ENOTSUP;
break;
}
return res;
}

int netdev2_ppp_set(netdev2_ppp_t *dev, netopt_t opt, void *value, size_t value_len)
{
int res;
switch (opt) {
case NETOPT_TUNNEL_IPV4_ADDRESS:
dev->ipv4.tunnel_addr = *((ipv4_addr_t *) value);
res = 0;
break;
case NETOPT_TUNNEL_UDP_PORT:
dev->ipv4.tunnel_port = *((uint16_t *) value);
res = 0;
break;
case NETOPT_APN_USER:
memcpy(dev->pap.username, value, value_len);
dev->pap.user_size = value_len;
res = 0;
break;
case NETOPT_APN_PASS:
memcpy(dev->pap.password, value, value_len);
dev->pap.pass_size = value_len;
res = 0;
break;
default:
return -ENOTSUP;
break;
}
return res;
}
3 changes: 3 additions & 0 deletions sys/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ endif
ifneq (,$(filter gnrc_netdev2,$(USEMODULE)))
DIRS += net/gnrc/link_layer/netdev2
endif
ifneq (,$(filter gnrc_ppp,$(USEMODULE)))
DIRS += net/gnrc/link_layer/ppp
endif
ifneq (,$(filter fib,$(USEMODULE)))
DIRS += net/network_layer/fib
endif
Expand Down
22 changes: 22 additions & 0 deletions sys/include/net/gnrc/netdev2.h
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,28 @@ typedef struct gnrc_netdev2 {
*/
int (*send)(struct gnrc_netdev2 *dev, gnrc_pktsnip_t *snip);


/**
* @brief Handle custom messages
*
* This function handles messages that are not present in gnrc_netdev2 loop.
*/
int (*msg_handler)(struct gnrc_netdev2 *dev, msg_t *msg);
Copy link
Member

Choose a reason for hiding this comment

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

netdev is supposed to work without IPC, so please remove this from netdev2.

#if defined(MODULE_GNRC_PPP) || doxygen
Copy link
Member

Choose a reason for hiding this comment

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

defined(DOXYGEN)

/**
* @brief Handle link up event from this device
*
* This function handles link up events from device.
*/
int (*link_up)(struct gnrc_netdev2 *dev);

/**
* @brief Handle link down event from this device
*
* This function handles link down events from device.
*/
int (*link_down)(struct gnrc_netdev2 *dev);
#endif
/**
* @brief Receive a pktsnip from this device
*
Expand Down
43 changes: 43 additions & 0 deletions sys/include/net/gnrc/netdev2/ppp.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/*
* Copyright (C) Fundación Inria Chile 2017
*
* This file is subject to the terms and conditions of the GNU Lesser
* General Public License v2.1. See the file LICENSE in the top level
* directory for more details.
*/

/**
* @addtogroup net_gnrc
Copy link
Member

Choose a reason for hiding this comment

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

please use @ingroup net_gnrc_netdev2 here.

* @{
*
* @file
* @brief GNRC to PPP netdev2 glue code interface
*
* @author José Ignacio Alamos <jose.alamos@inria.cl>
*/

#ifndef GNRC_NETDEV2_PPP_H_
#define GNRC_NETDEV2_PPP_H_

#include "net/netdev2/ppp.h"
#include "net/gnrc/netdev2.h"


#ifdef __cplusplus
extern "C" {
#endif

/**
* @brief Initialize GNRC handler for netdev2 PPP devices
*
* @param[out] gnrc_netdev2 gnrc_netdev2 struct to initialize
* @param[in] dev PPP device to handle
*/
void gnrc_netdev2_ppp_init(gnrc_netdev2_t *gnrc_netdev2, netdev2_ppp_t *dev);

#ifdef __cplusplus
}
#endif

#endif /* GNRC_NETDEV2_PPP_H_ */
/** @} */
60 changes: 60 additions & 0 deletions sys/include/net/gnrc/nettype.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@

#include "net/ethertype.h"
#include "net/protnum.h"
#include "net/ppptype.h"

#ifdef __cplusplus
extern "C" {
Expand Down Expand Up @@ -104,6 +105,21 @@ typedef enum {
* @}
*/

/**
* @{
* @name Link layer
*/
#ifdef MODULE_GNRC_PPP
GNRC_NETTYPE_LCP, /**< Protocol is PPP LCP */
GNRC_NETTYPE_IPCP, /**< Protocol is PPP IPCP */
GNRC_NETTYPE_HDLC, /**< Protocol is HDLC */
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need a type for each?

Copy link
Member

Choose a reason for hiding this comment

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

Discussed offline that it stays this way.

GNRC_NETTYPE_IPV4, /**< Protocol is IPV4 encapsulated in HDLC frame */
Copy link
Member

Choose a reason for hiding this comment

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

Considering that later on there might be IPv4 frames, too putting this into its own #ifdef and making gnrc_ppp dependent on a gnrc_ipv4 pseudo-module might be the cleaner approach.

Copy link
Member

Choose a reason for hiding this comment

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

Ping?

GNRC_NETTYPE_PAP, /**< Protocol is PAP auth */
#endif
/**
* @}
*/

GNRC_NETTYPE_NUMOF, /**< maximum number of available protocols */
} gnrc_nettype_t;

Expand Down Expand Up @@ -134,6 +150,7 @@ static inline gnrc_nettype_t gnrc_nettype_from_ethertype(uint16_t type)
}
}


/**
* @brief Translates @ref net_gnrc_nettype to an Ether Type number
* @see <a href="http://www.iana.org/assignments/ieee-802-numbers/ieee-802-numbers.xhtml#ieee-802-numbers-1">
Expand Down Expand Up @@ -241,6 +258,49 @@ static inline uint8_t gnrc_nettype_to_protnum(gnrc_nettype_t type)
}
}

static inline gnrc_nettype_t gnrc_nettype_from_ppp_protnum(uint16_t protnum)
{
switch (protnum) {
#ifdef MODULE_GNRC_PPP
case PPPTYPE_LCP:
return GNRC_NETTYPE_LCP;
case PPPTYPE_NCP_IPV4:
return GNRC_NETTYPE_IPCP;
case PPPTYPE_IPV4:
return GNRC_NETTYPE_IPV4;
case PPPTYPE_PAP:
return GNRC_NETTYPE_PAP;
# ifdef MODULE_GNRC_IPV6
case PPPTYPE_IPV6:
return GNRC_NETTYPE_IPV6;
# endif
#endif
default:
return GNRC_NETTYPE_UNDEF;
}
}
static inline uint16_t gnrc_nettype_to_ppp_protnum(gnrc_nettype_t type)
{
switch (type) {
#ifdef MODULE_GNRC_PPP
case GNRC_NETTYPE_LCP:
return PPPTYPE_LCP;
case GNRC_NETTYPE_IPCP:
return PPPTYPE_NCP_IPV4;
# ifdef MODULE_GNRC_IPV6
case GNRC_NETTYPE_IPV6:
return PPPTYPE_IPV6;
# endif
case GNRC_NETTYPE_IPV4:
return PPPTYPE_IPV4;
case GNRC_NETTYPE_PAP:
return PPPTYPE_PAP;
#endif
default:
return PPPTYPE_UNKNOWN;
}
}

#ifdef __cplusplus
}
#endif
Expand Down
Loading