Skip to content

netif: introduce generic network interface descriptor#11879

Merged
leandrolanzieri merged 3 commits intoRIOT-OS:masterfrom
jia200x:pr/netif_new_interface
Oct 11, 2019
Merged

netif: introduce generic network interface descriptor#11879
leandrolanzieri merged 3 commits intoRIOT-OS:masterfrom
jia200x:pr/netif_new_interface

Conversation

@jia200x
Copy link
Copy Markdown
Member

@jia200x jia200x commented Jul 22, 2019 •
edited
Loading

Contribution description

This PR changes the way a netif_t object is represented. Currently the representation is stack specific (e.g https://github.com/RIOT-OS/RIOT/blob/master/sys/net/gnrc/netif/include/netif_types.h#L29). With this PR, an interface is always represented by a netif_t object (where stack specific network interfaces can inherit).

The consequences of these changes are:

  • It's possible to move the stack dependent iteration logic to generic netif_t functions (see here)
  • It makes easier to add stack independent functions send and receive data from an interface (e.g netif_send(netif_t *netif), netif_recv(netif_t *netif)). TBD! (see netif: introduce generic network interface descriptor #11879)

The netif_t is now a linked list that point to the next interface, so it's required to register netifs via the newly introduced netif_register function.
Also, the NETIF_INVALID macro is not needed anymore (an invalid interface is mapped to NULL).

This PR is needed in order to have MAC/PHY layers independent of the network stack.

Testing procedure

It should be enough to run tests/gnrc_netif test.
Check if the documentation builds without issues: make doc

Issues/PRs references

It somehow relates to #11483.

@jia200x jia200x requested review from bergzand, kaspar030 and miri64 July 22, 2019 12:09
@jia200x jia200x force-pushed the pr/netif_new_interface branch from 6f9f778 to df0b1b0 Compare July 22, 2019 12:11
@jia200x jia200x added Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Jul 22, 2019
@jia200x jia200x requested a review from aabadie July 22, 2019 12:13
jia200x
jia200x commented Jul 22, 2019
View reviewed changes
tests/gnrc_netif/main.c
sprintf(exp_name, "if%d", (int) ((gnrc_netif_t*) netif)->pid);
TEST_ASSERT_EQUAL_INT(strlen(exp_name), res);
TEST_ASSERT_EQUAL_STRING(&exp_name[0], &name[0]);
TEST_ASSERT_EQUAL_INT(0, netif_get_name(INT16_MAX, name));
Copy link
Copy Markdown
Member Author

@jia200x jia200x Jul 22, 2019

Choose a reason for hiding this comment

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

This one is not required anymore, because netif_t pointers will always be valid or NULL

@jia200x jia200x requested a review from PeterKietzmann July 22, 2019 12:18
@jia200x jia200x mentioned this pull request Jul 23, 2019
Closed
leandrolanzieri
leandrolanzieri reviewed Sep 27, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@leandrolanzieri leandrolanzieri left a comment

Choose a reason for hiding this comment

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

It looks good to me. I have tested this and works well. This is a really good improvement in the API towards making it more compatible.

@miri64 could you take a look at this?

@leandrolanzieri leandrolanzieri added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines labels Sep 27, 2019
miri64
miri64 reviewed Sep 27, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Some comments from my side

@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Sep 30, 2019

@miri64 @leandrolanzieri I think I addressed al comments. May I rebase/squash?

@miri64
Copy link
Copy Markdown
Member

miri64 commented Sep 30, 2019

@miri64 @leandrolanzieri I think I addressed al comments. May I rebase/squash?

For my part, yes.

@leandrolanzieri leandrolanzieri added the Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines label Sep 30, 2019
leandrolanzieri
leandrolanzieri approved these changes Sep 30, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@leandrolanzieri leandrolanzieri left a comment

Choose a reason for hiding this comment

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

All comments were addressed. I retested and works as expected. Please rebase and squash.

ACK. We will need a second one.

miri64
miri64 approved these changes Sep 30, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

ACK for my part as well. I trust @leandrolanzieri's testing and the tests run by Murdock

@jia200x jia200x force-pushed the pr/netif_new_interface branch 2 times, most recently from 52cb2ca to 1144baf Compare September 30, 2019 13:11
@leandrolanzieri leandrolanzieri added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 30, 2019
@leandrolanzieri
Copy link
Copy Markdown
Contributor

leandrolanzieri commented Sep 30, 2019

hmm looks like tests/gnrc_ipv6_ext does not fit in the saml1x.

@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Sep 30, 2019

hmm looks like tests/gnrc_ipv6_ext does not fit in the saml1x.

How to proceed? Blacklisting?

@leandrolanzieri
Copy link
Copy Markdown
Contributor

leandrolanzieri commented Sep 30, 2019

The build size for the current master is:

text	   data	    bss	    dec	    hex	filename
  52888	    508	  15876	  69272	  10e98	
/home/leandro/Work/RIOT/tests/gnrc_ipv6_ext/bin/saml10-xpro/tests_gnrc_ipv6_ext.elf

So RAM usage is surprisingly 16.384 (16 Kb). It makes sense that with this PR it no longer fits the memory. I would say blacklisting is the way to go here.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Sep 30, 2019

I guess that's the kind of Spähne that happen if you hobel, as we say in German ;-P. I hope, further development will amortize the RAM usage in the end.

@jia200x jia200x force-pushed the pr/netif_new_interface branch from 1144baf to d19d26d Compare October 1, 2019 09:16
@leandrolanzieri
Copy link
Copy Markdown
Contributor

leandrolanzieri commented Oct 1, 2019

Soft feature freeze is now active, as this is an API change we won't be able to merge it until the hard feature freeze (10th of October).

miri64
miri64 reviewed Oct 1, 2019
View reviewed changes
@jia200x jia200x force-pushed the pr/netif_new_interface branch from d19d26d to 2e36ea2 Compare October 1, 2019 11:26
miri64
miri64 reviewed Oct 1, 2019
View reviewed changes
@leandrolanzieri leandrolanzieri mentioned this pull request Oct 2, 2019
Merged
@leandrolanzieri
Copy link
Copy Markdown
Contributor

leandrolanzieri commented Oct 11, 2019

@jia200x please rebase

@jia200x jia200x force-pushed the pr/netif_new_interface branch from 2e36ea2 to 9727f32 Compare October 11, 2019 08:59
@leandrolanzieri leandrolanzieri merged commit 760da79 into RIOT-OS:master Oct 11, 2019
This was referenced Nov 11, 2019
Closed
Closed
@haukepetersen
Copy link
Copy Markdown
Contributor

haukepetersen commented Nov 13, 2019

Just looked at this once more: do I understand it correctly, that the 'netif device list (registry)' is with this approach mandatory to use whenever network devices are used in a system? This seems to break the good practice in separating in interface from its concrete usage context (e.g. see saul vs saul_reg).

@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Nov 13, 2019

Just looked at this once more: do I understand it correctly, that the 'netif device list (registry)' is with this approach mandatory to use whenever network devices are used in a system? This seems to break the good practice in separating in interface from its concrete usage context (e.g. see saul vs saul_reg).

No, it's not mandatory. Only if the network device requires a "network interface".

Network devices and network interfaces are in 2 different layers.

@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Nov 13, 2019

Even more, there might be network interface that don't depend on any network device (e.g loopback)

@fjmolinas fjmolinas mentioned this pull request Jan 23, 2020
Closed
80 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@miri64 miri64 miri64 approved these changes

@leandrolanzieri leandrolanzieri leandrolanzieri approved these changes

@kaspar030 kaspar030 Awaiting requested review from kaspar030

@bergzand bergzand Awaiting requested review from bergzand

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

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 Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jia200x @miri64 @leandrolanzieri @haukepetersen