Skip to content

netdev: add netdev_register() to keep track of netdev IDs #13746

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

Merged
benpicco merged 5 commits into from
Aug 17, 2020
Merged

netdev: add netdev_register() to keep track of netdev IDs #13746

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7ceb3f0
netdev: add netdev_register() to keep track if netdev IDs
benpicco Feb 13, 2020
53eedc2
drivers/at86rf2xx: register driver with netdev
benpicco Jul 7, 2020
4f19b54
drivers/at86rf215: register driver with netdev
benpicco Jul 7, 2020
decadc4
drivers/dose: register driver with netdev
benpicco Jul 7, 2020
1582b10
drivers/at86rf2xx: fix typo
benpicco Jul 10, 2020
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
5 changes: 5 additions & 0 deletions Makefile.dep
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,11 @@ ifneq (,$(filter gnrc_mac,$(USEMODULE)))
endif
endif

# Don't register netdevs if there is only a single one of them
ifeq (,$(filter gnrc_netif_single,$(USEMODULE)))
USEMODULE += netdev_register
endif
Comment on lines +72 to +75

@leandrolanzieri leandrolanzieri Sep 9, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is pulling netdev_register almost always now. Although it is a pseudomodule I think the dependency should be changed. AFIK there is no equivalent to gnrc_netif_single for other stacks, right? (@miri64 ?) Should this be added when some netdev_% module is used instead?

@leandrolanzieri leandrolanzieri Sep 30, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is affecting #13565 now, as the unneeded dependency netdev_register is pulled.

@benpicco benpicco Sep 30, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

netdev_register is a pseudomodule, so it won't do anything on it's own.
If not netdev_ code is used, this will not pull in any additional code.

@jia200x jia200x Sep 30, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But it changes the dependency resolution. This can indeed change the binary,

@miri64 miri64 Sep 30, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Isn't netdev_register a sub-module to netdev? If not, IMHO there is nothing speaking against it being one. should have read the original question...

@miri64 miri64 Sep 30, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess we just missed a step in this discussion: #13746 (comment)

@jia200x jia200x Sep 30, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

how to proceed with that?

@benpicco benpicco Sep 30, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

but you just provided the solution yourself #13565 (comment)

@jia200x jia200x Sep 30, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I thought it was not intended to have a reverse dependency to gnrc_netif_single

@miri64 miri64 Sep 30, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am fine with a reverse dependency how you implemented it. My original concern was about GNRC code in the network device code.


ifneq (,$(filter gnrc_gomach,$(USEMODULE)))
USEMODULE += gnrc_netif
USEMODULE += gnrc_nettype_gomach
Expand Down
10 changes: 6 additions & 4 deletions drivers/at86rf215/at86rf215.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,30 +30,32 @@
#define ENABLE_DEBUG (0)
#include "debug.h"

static void _setup_interface(at86rf215_t *dev, const at86rf215_params_t *params)
static void _setup_interface(at86rf215_t *dev, const at86rf215_params_t *params, uint8_t index)
{
netdev_t *netdev = (netdev_t *)dev;

netdev->driver = &at86rf215_driver;
dev->params = *params;
dev->state = AT86RF215_STATE_OFF;

netdev_register(netdev, NETDEV_AT86RF215, index);
}

void at86rf215_setup(at86rf215_t *dev_09, at86rf215_t *dev_24, const at86rf215_params_t *params)
void at86rf215_setup(at86rf215_t *dev_09, at86rf215_t *dev_24, const at86rf215_params_t *params, uint8_t index)
{
/* configure the sub-GHz interface */
if (dev_09) {
dev_09->RF = &RF09_regs;
dev_09->BBC = &BBC0_regs;
_setup_interface(dev_09, params);
_setup_interface(dev_09, params, 2 * index);
dev_09->sibling = dev_24;
}

/* configure the 2.4 GHz interface */
if (dev_24) {
dev_24->RF = &RF24_regs;
dev_24->BBC = &BBC1_regs;
_setup_interface(dev_24, params);
_setup_interface(dev_24, params, 2 * index + 1);
dev_24->sibling = dev_09;
}
}
Expand Down
4 changes: 3 additions & 1 deletion drivers/at86rf2xx/at86rf2xx.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
#include "debug.h"


void at86rf2xx_setup(at86rf2xx_t *dev, const at86rf2xx_params_t *params)
void at86rf2xx_setup(at86rf2xx_t *dev, const at86rf2xx_params_t *params, uint8_t index)
{
netdev_t *netdev = (netdev_t *)dev;

Expand All @@ -56,6 +56,8 @@ void at86rf2xx_setup(at86rf2xx_t *dev, const at86rf2xx_params_t *params)
/* initialize device descriptor */
dev->params = *params;
#endif

netdev_register(netdev, NETDEV_AT86RF2XX, index);
}

static void at86rf2xx_disable_clock_output(at86rf2xx_t *dev)
Expand Down
5 changes: 3 additions & 2 deletions drivers/dose/dose.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ static int _send(netdev_t *dev, const iolist_t *iolist);
static int _get(netdev_t *dev, netopt_t opt, void *value, size_t max_len);
static int _set(netdev_t *dev, netopt_t opt, const void *value, size_t len);
static int _init(netdev_t *dev);
void dose_setup(dose_t *ctx, const dose_params_t *params);

static uint16_t crc16_update(uint16_t crc, uint8_t octet)
{
Expand Down Expand Up @@ -540,7 +539,7 @@ static const netdev_driver_t netdev_driver_dose = {
.set = _set
};

void dose_setup(dose_t *ctx, const dose_params_t *params)
void dose_setup(dose_t *ctx, const dose_params_t *params, uint8_t index)
{
static const xtimer_ticks32_t min_timeout = {.ticks32 = XTIMER_BACKOFF};

Expand All @@ -557,6 +556,8 @@ void dose_setup(dose_t *ctx, const dose_params_t *params)
gpio_irq_disable(ctx->sense_pin);
}

netdev_register(&ctx->netdev, NETDEV_DOSE, index);

assert(sizeof(ctx->mac_addr.uint8) == ETHERNET_ADDR_LEN);
luid_get_eui48(&ctx->mac_addr);
DEBUG("dose dose_setup(): mac addr %02x:%02x:%02x:%02x:%02x:%02x\n",
Expand Down
4 changes: 3 additions & 1 deletion drivers/include/at86rf215.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,8 +292,10 @@ typedef struct at86rf215 {
* @param[out] dev_09 sub-GHz device descriptor
* @param[out] dev_24 2.4 GHz device descriptor
* @param[in] params parameters for device initialization
* @param[in] index index of @p params in a global parameter struct array.
* If initialized manually, pass a unique identifier instead.
*/
void at86rf215_setup(at86rf215_t *dev_09, at86rf215_t *dev_24, const at86rf215_params_t *params);
void at86rf215_setup(at86rf215_t *dev_09, at86rf215_t *dev_24, const at86rf215_params_t *params, uint8_t index);

/**
* @brief Trigger a hardware reset and configure radio with default values.
Expand Down
6 changes: 4 additions & 2 deletions drivers/include/at86rf2xx.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,8 +294,10 @@ typedef struct {
*
* @param[out] dev device descriptor
* @param[in] params parameters for device initialization
* @param[in] index index of @p params in a global parameter struct array.
* If initialized manually, pass a unique identifier instead.
*/
void at86rf2xx_setup(at86rf2xx_t *dev, const at86rf2xx_params_t *params);
void at86rf2xx_setup(at86rf2xx_t *dev, const at86rf2xx_params_t *params, uint8_t index);

/**
* @brief Trigger a hardware reset and configure radio with default values
Expand Down Expand Up @@ -387,7 +389,7 @@ uint8_t at86rf2xx_get_phy_mode(at86rf2xx_t *dev);
*
* @param[in] dev device to read from
*
* @return the currenty set rate mode
* @return the currently set rate mode
*/
uint8_t at86rf2xx_get_rate(at86rf2xx_t *dev);

Expand Down
4 changes: 3 additions & 1 deletion drivers/include/dose.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,10 @@ typedef struct {
* @brief Setup a DOSE based device state
* @param[out] dev Handle of the device to initialize
* @param[in] params Parameters for device initialization
* @param[in] index Index of @p params in a global parameter struct array.
* If initialized manually, pass a unique identifier instead.
*/
void dose_setup(dose_t *dev, const dose_params_t *params);
void dose_setup(dose_t *dev, const dose_params_t *params, uint8_t index);

#ifdef __cplusplus
}
Expand Down
38 changes: 38 additions & 0 deletions drivers/include/net/netdev.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ extern "C" {

#include "iolist.h"
#include "net/netopt.h"
#include "kernel_defines.h"

#ifdef MODULE_L2FILTER
#include "net/l2filter.h"
Expand Down Expand Up @@ -271,6 +272,19 @@ typedef struct netdev netdev_t;
*/
typedef void (*netdev_event_cb_t)(netdev_t *dev, netdev_event_t event);

/**
* @brief Driver types for netdev.
* @{
*/
typedef enum {
NETDEV_ANY = 0, /**< Will match any device type */
NETDEV_AT86RF215,
NETDEV_AT86RF2XX,
NETDEV_DOSE,
/* add more if needed */
} netdev_type_t;
Comment thread
benpicco marked this conversation as resolved.
/** @} */

/**
* @brief Structure to hold driver state
*
Expand All @@ -290,8 +304,32 @@ struct netdev {
#ifdef MODULE_L2FILTER
l2filter_t filter[CONFIG_L2FILTER_LISTSIZE]; /**< link layer address filters */
#endif
#ifdef MODULE_NETDEV_REGISTER
netdev_type_t type; /**< driver type used for netdev */
uint8_t index; /**< instance number of the device */
#endif
};

/**
* @brief Register a device with netdev.
* Must by called by the driver's setup function.
*
* @param[out] dev the new netdev
* @param[in] type the driver used for the netdev
* @param[in] index the index in the config struct
*/
static inline void netdev_register(struct netdev *dev, netdev_type_t type, uint8_t index)
{
#ifdef MODULE_NETDEV_REGISTER
dev->type = type;
dev->index = index;
#else
(void) dev;
(void) type;
(void) index;
#endif
}

/**
* @brief Structure to hold driver interface -> function mapping
*
Expand Down
1 change: 1 addition & 0 deletions makefiles/pseudomodules.inc.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ PSEUDOMODULES += mpu_noexec_ram
PSEUDOMODULES += nanocoap_%
PSEUDOMODULES += netdev_default
PSEUDOMODULES += netdev_ieee802154_%
PSEUDOMODULES += netdev_register
PSEUDOMODULES += netstats
PSEUDOMODULES += netstats_l2
PSEUDOMODULES += netstats_ipv6
Expand Down
2 changes: 1 addition & 1 deletion pkg/lwip/contrib/lwip.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ void lwip_bootstrap(void)
}
#elif defined(MODULE_AT86RF2XX)
for (unsigned i = 0; i < LWIP_NETIF_NUMOF; i++) {
at86rf2xx_setup(&at86rf2xx_devs[i], &at86rf2xx_params[i]);
at86rf2xx_setup(&at86rf2xx_devs[i], &at86rf2xx_params[i], i);
if (netif_add(&netif[i], &at86rf2xx_devs[i], lwip_netdev_init,
tcpip_6lowpan_input) == NULL) {
DEBUG("Could not add at86rf2xx device\n");
Expand Down
2 changes: 1 addition & 1 deletion pkg/openthread/contrib/openthread.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ void openthread_bootstrap(void)

/* setup netdev modules */
#ifdef MODULE_AT86RF2XX
at86rf2xx_setup(&at86rf2xx_dev, &at86rf2xx_params[0]);
at86rf2xx_setup(&at86rf2xx_dev, &at86rf2xx_params[0], 0);
netdev_t *netdev = (netdev_t *) &at86rf2xx_dev;
#endif
#ifdef MODULE_KW41ZRF
Expand Down
2 changes: 1 addition & 1 deletion pkg/openwsn/contrib/openwsn.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ int openwsn_bootstrap(void)

#ifdef MODULE_AT86RF2XX
netdev_t *netdev = (netdev_t *)&at86rf2xx_dev.netdev.netdev;
at86rf2xx_setup(&at86rf2xx_dev, &at86rf2xx_params[0]);
at86rf2xx_setup(&at86rf2xx_dev, &at86rf2xx_params[0], 0);
(void) netdev;
#endif

Expand Down
5 changes: 2 additions & 3 deletions sys/net/gnrc/netif/init_devs/auto_init_at86rf215.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,7 @@ static inline void _setup_netif(gnrc_netif_t *netif, void* netdev, void* stack,
void auto_init_at86rf215(void)
{
unsigned i = 0;
unsigned j = 0;
while (j < AT86RF215_NUM) {
for (unsigned j = 0; j < AT86RF215_NUM; ++j) {

at86rf215_t *dev_09 = NULL;
at86rf215_t *dev_24 = NULL;
Expand All @@ -103,7 +102,7 @@ void auto_init_at86rf215(void)
++i;
}

at86rf215_setup(dev_09, dev_24, &at86rf215_params[j++]);
at86rf215_setup(dev_09, dev_24, &at86rf215_params[j], j);

/* setup sub-GHz interface */
_setup_netif(netif_09, dev_09, stack_09, AT86RF215_MAC_PRIO_SUBGHZ);
Expand Down
2 changes: 1 addition & 1 deletion sys/net/gnrc/netif/init_devs/auto_init_at86rf2xx.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ void auto_init_at86rf2xx(void)
for (unsigned i = 0; i < AT86RF2XX_NUM; i++) {
LOG_DEBUG("[auto_init_netif] initializing at86rf2xx #%u\n", i);

at86rf2xx_setup(&at86rf2xx_devs[i], &at86rf2xx_params[i]);
at86rf2xx_setup(&at86rf2xx_devs[i], &at86rf2xx_params[i], i);
#if defined(MODULE_GNRC_GOMACH)
gnrc_netif_gomach_create(&_netif[i], _at86rf2xx_stacks[i],
AT86RF2XX_MAC_STACKSIZE,
Expand Down
2 changes: 1 addition & 1 deletion sys/net/gnrc/netif/init_devs/auto_init_dose.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ void auto_init_dose(void)
for (unsigned i = 0; i < DOSE_NUM; i++) {
LOG_DEBUG("[auto_init_netif] initializing dose #%d.\n", i);

dose_setup(&dose[i], &dose_params[i]);
dose_setup(&dose[i], &dose_params[i], i);
gnrc_netif_ethernet_create(&_netif[i], _netdev_eth_stack[i], DOSE_MAC_STACKSIZE,
DOSE_MAC_PRIO, "dose", (netdev_t *)&dose[i]);
}
Expand Down
2 changes: 1 addition & 1 deletion tests/driver_at86rf2xx/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ int main(void)
netdev_t *dev = (netdev_t *)(&devs[i]);

printf("Initializing AT86RF2xx radio at SPI_%d\n", p->spi);
at86rf2xx_setup(&devs[i], p);
at86rf2xx_setup(&devs[i], p, i);
dev->event_callback = _event_cb;
if (dev->driver->init(dev) < 0) {
continue;
Expand Down
2 changes: 1 addition & 1 deletion tests/driver_at86rf2xx_aes/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ int main(void)
{
at86rf2xx_t dev;
bool success = true;
at86rf2xx_setup(&dev, &at86rf2xx_params[0]);
at86rf2xx_setup(&dev, &at86rf2xx_params[0], 0);
dev.netdev.netdev.event_callback = _event_cb;
if (dev.netdev.netdev.driver->init(&dev.netdev.netdev) != 0) {
return EXIT_FAILURE;
Expand Down