Skip to content

[WIP] automatically set GNRC_NETIF_NUMOF#12308

Closed
benpicco wants to merge 2 commits intoRIOT-OS:masterfrom
benpicco:gnrc_netif_numof
Closed

[WIP] automatically set GNRC_NETIF_NUMOF#12308
benpicco wants to merge 2 commits intoRIOT-OS:masterfrom
benpicco:gnrc_netif_numof

Conversation

@benpicco
Copy link
Copy Markdown
Contributor

@benpicco benpicco commented Sep 26, 2019

Contribution description

Currently, GNRC_NETIF_NUMOF has to be set manually.
This PR changes that so that each network interface will increment GNRC_NETIF_NUMOF at compile-time.

Since GNRC_NETIF_NUMOF now consists of 0 + 0 + sizeof(…) + 0 + sizeof(…) + … the processor can not evaluate it anymore. This is not a problem since the compiler will see the same job if it sees an if (0).
But as a prerequisite, many #if (GNRC_NETIF_NUMOF > 0) had to be changed into if (GNRC_NETIF_NUMOF > 0).

As expected, code size did not increase.

Testing procedure

Check if everything still works, especially examples/tests that define GNRC_NETIF_NUMOF := 2

Issues/PRs references

#11979
#9903

@benpicco
sys/net/gnrc: let the compiler evaluate GNRC_NETIF_NUMOF
1f619e3 
The preprocessor is not able to do arithmetic using the sizeof() operator.
Instead, let the compiler evaluate the constant, the result should be the
same.
@benpicco benpicco added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 26, 2019
gschorcht
gschorcht reviewed Sep 27, 2019
View reviewed changes
sys/include/auto_init_netif.h
#define STM32_ETH_NUM (0)
#endif

#define GNRC_NETIF_NUMOF (AT86RF2XX_NUM + CC2420_NUM + KW2XRF_NUM + MRF24J40_NUM \
Copy link
Copy Markdown
Contributor

@gschorcht gschorcht Sep 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

What about modules esp_now, esp_wifi and esp_eth? All of them provide a separate netif.

Copy link
Copy Markdown
Contributor Author

@benpicco benpicco Sep 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Will add those!
Why are they not initialized in sys/auto_init/netif/?

Copy link
Copy Markdown
Contributor

@gschorcht gschorcht Sep 27, 2019

Choose a reason for hiding this comment

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

The initialization of these netdevs is called as for other drivers in sys/auto_init/auto_init.c

RIOT/sys/auto_init/auto_init.c

Lines 244 to 259 in 3779635

#ifdef MODULE_ESP_ETH
extern void auto_init_esp_eth(void);
auto_init_esp_eth();
#endif
/* don't change the order of auto_init_esp_now and auto_init_esp_wifi */
#ifdef MODULE_ESP_NOW
extern void auto_init_esp_now(void);
auto_init_esp_now();
#endif
/* don't change the order of auto_init_esp_now and auto_init_esp_wifi */
#ifdef MODULE_ESP_WIFI
extern void auto_init_esp_wifi(void);
auto_init_esp_wifi();
#endif

The difference is, that the implementations of the auto_init_* functions are not placed in separate files in sys/auto_init/netif but in cpu/esp*/esp_*/esp_*_netdev.c. The reason is that these netdevs are platform specific and shouldn't be placed in a common place.

Copy link
Copy Markdown
Contributor Author

@benpicco benpicco Sep 27, 2019

Choose a reason for hiding this comment

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

I see! But stm32_eth and nrf802154 are platform specific too…

Copy link
Copy Markdown
Contributor

@gschorcht gschorcht Sep 27, 2019

Choose a reason for hiding this comment

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

Yes. The question is whether we should move the auto_init_esp_* code to sys/auto_init/netif or whether we should leave it as it is.

leandrolanzieri
leandrolanzieri reviewed Oct 28, 2019
View reviewed changes
sys/include/auto_init_netif.h
#define STM32_ETH_NUM (0)
#endif

#define GNRC_NETIF_NUMOF (AT86RF2XX_NUM + CC2420_NUM + KW2XRF_NUM + MRF24J40_NUM \
Copy link
Copy Markdown
Contributor

@leandrolanzieri leandrolanzieri Oct 28, 2019

Choose a reason for hiding this comment

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

What about defining a generic NETIF_NUMOF here and make GNRC use that? so we could keep this stack agnostic

@gschorcht gschorcht mentioned this pull request Nov 22, 2019
Merged
@jia200x jia200x mentioned this pull request Dec 19, 2019
Merged
@benpicco benpicco closed this Mar 26, 2020
@benpicco benpicco deleted the gnrc_netif_numof branch March 26, 2020 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@leandrolanzieri leandrolanzieri leandrolanzieri left review comments

@gschorcht gschorcht gschorcht left review comments

@miri64 miri64 Awaiting requested review from miri64 miri64 will be requested when the pull request is marked ready for review miri64 is a code owner

Assignees

No one assigned

Labels

Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@benpicco @leandrolanzieri @gschorcht