sys: net: add gnrc netdev2 support by kaspar030 · Pull Request #3683 · RIOT-OS/RIOT

kaspar030 · 2015-08-21T14:44:16Z

This adds netdev2 support to gnrc and adapts dev_eth_tap to the new interface.

The new device driver structure has several advantages:

drivers written against netdev2 are not dependend on gnrc
a lot of link-layer code can be shared

~~While this is marked WIP as I'd like to test this more, it is working fine on native.~~

~~Depends on #3210.~~ (netdev2 merged)

kaspar030 · 2015-08-21T14:45:47Z

sys/net/gnrc/link_layer/netdev2/gnrc_netdev2_eth.c

@authmillenon Could you help me rewriting this so the packet doesn't get copied twice?

If I manage to spot the problem, yes :)

Well, to get it work, I create a big-enough pktsnip, and then build another one, copying the data I need.

I tried to just mark out the ethernet header, snip_remove that, and than use the initial big pktsnip, add the netif hdr, go... but It didn't manage to get it to work without memory leaks.

Why would there be memory leaks?

Right now there are none. But right now data is copied twice (once into "whole_pkt", and then partly into pkt).
Initially I wanted to just mark the headers in "whole_pkt", but I don't know why edit how.

But in general this should work (without error checking to make the code look a little simpler):

if (bytes_expected) { ethernet_hdr_t *hdr; gnrc_pktsnip_t *netif_hdr, *eth_hdr; gnrc_pktsnip_t *pkt = gnrc_pktbuf_add(NULL, NULL, bytes_expected, GNRC_NETTYPE_UNDEF); int nread = dev->driver->recv(dev, pkt->data, bytes_expected); if (nread < bytes_expected) { gnrc_pktbuf_realloc_data(pkt, nread); } eth_hdr = gnrc_pktbuf_mark(pkt, sizeof(ethernet_hdr_t), GNRC_NETTYPE_UNDEF); hdr = eth_hdr->data; gnrc_netif_hdr_init(netif_hdr->data, ETHERNET_ADDR_LEN, ETHERNET_ADDR_LEN); gnrc_netif_hdr_set_src_addr(netif_hdr->data, hdr->src, ETHERNET_ADDR_LEN); gnrc_netif_hdr_set_dst_addr(netif_hdr->data, hdr->dst, ETHERNET_ADDR_LEN); ((gnrc_netif_hdr_t *)netif_hdr->data)->if_pid = thread_getpid(); gnrc_pktbuf_remove_snip(pkt, hdr); LL_APPEND(pkt, hdr); return pkt; } return NULL;

But in general this should work

Thanks a lot! I'll be out of town over the weekend, but will try first thing on monday.

@authmillenon Hm, I don't quite get it.

I think you're mixing up hdr and eth_hdr in the last lines, right?
Where do you mark the payload according to ethertype?

I think you're mixing up hdr and eth_hdr in the last lines, right?

Right, that tends to happen when you head-compile ^^

Where do you mark the payload according to ethertype?

Forgot it. Just throw a

pkt->type = gnrc_nettype_from_ethertype(byteorder_ntohs(hdr->type));

in there after you set hdr (the real one this time), but before you remove eth_hdr in the gnrc_pktbuf_remove_snip(pkt, eth_hdr); line.

yup, that did it. thanks.

miri64 · 2015-08-21T15:12:55Z

Okay, can you just quickly explain to me: is this a gnrc_netdev2_eth wrapper (just for ethernet devices) or a gnrc_netdev2 wrapper (for all netdev2 devices)?

kaspar030 · 2015-08-21T15:34:30Z

Well, the PR contains both. gnrc_netdev2 is a "generic wrapper", gnrc_netdev2_eth contains ethernet specific code.

miri64 · 2015-08-21T15:35:09Z

sys/net/gnrc/link_layer/netdev2/gnrc_netdev2_eth.c

This potentially releases NULL pointers which does not fulfill the function's precondition.

kaspar030 · 2015-08-26T09:41:38Z

rebased
added a lot of license headers & doxygen

kaspar030 · 2015-08-26T15:37:20Z

fixed unnecessary copying in _recv for ethernet packets

miri64 · 2015-08-26T15:43:39Z

Rebase please.

kaspar030 · 2015-08-26T16:46:09Z

rebased

kaspar030 · 2015-08-26T20:11:46Z

(triggered CI)

kaspar030 · 2015-08-27T10:22:47Z

@emmanuelsearch Could you try this on OS X?

emmanuelsearch · 2015-08-27T10:27:41Z

Sure. What test should I run?

kaspar030 · 2015-08-27T10:28:36Z

Anything using the network. Do you know how to ping e.g., "gnrc_networking" from your OS X host?

edit This PR replaces the tap network driver native uses.

miri64 · 2015-08-27T11:50:14Z

sys/include/net/gnrc/gnrc_netdev2.h

kaspar030 · 2015-09-03T18:09:36Z

@thomaseichinger Hm, I've no clue about OS X networking. If you've got no time to test this, could you shortly explain how we can test native networking?

@emmanuelsearch I can confirm that tapsetup is broken on OS X.

@OlegHahm Would you mind testing on Linux?

OlegHahm · 2015-09-03T18:18:32Z

Of course I can test. The question is what and how?

kaspar030 · 2015-09-03T18:25:47Z

Well, this PR replaces dev_eth_tap, so testing of native networking is needed. It should behave as before.

OlegHahm · 2015-09-03T18:43:25Z

cpu/native/include/netdev2_tap.h

IFNAMSIZ is undeclared, according to gcc.

Even with the latest commit (f5f0392)?

I have to admit I have no clue why this broke.

Ah, yes, the remote wasn't up to date. Sorry for the noise.

OlegHahm · 2015-09-03T21:13:51Z

see kaspar030#12

OlegHahm · 2015-09-03T21:17:21Z

and with RPL initialized I get after some time:

gnrc_networking.elf: /home/oleg/git/RIOT/cpu/native/netdev2_tap/netdev2_tap.c:151: _get: Assertion `max_len == 2' failed.

on the non-root node.

To reproduce:

start two native instances with gnrc_networking
call rpl init 7 on both
call ifconfig 7 add affe::1 and rpl root 1 affe::1 on one of them
see the crash on the other one

kaspar030 · 2015-09-03T21:30:29Z

@OlegHahm Hm, netopt.h explicitly states that the result is "uint16_t", but gnrc_ndp wants to read into size_t. I feel weird removing the assert... Could you remove it just for testing?

@authmillenon, do you agree we should use uint16_t in gnrc_ndp?

kaspar030 · 2015-09-03T21:36:27Z

Fix for assertion PR'ed in #3774.

OlegHahm · 2015-09-03T22:08:55Z

I get a strange error if I just remove the assert:

"make" -C /home/oleg/git/RIOT/cpu/native/netdev2_tap
/home/oleg/git/RIOT/cpu/native/netdev2_tap/netdev2_tap.c: In function ‘_get’
/home/oleg/git/RIOT/cpu/native/netdev2_tap/netdev2_tap.c:151:13: error: a label can only be part of a statement and a declaration is not a statement
             uint16_t *tgt = (uint16_t*)value;

If I put the case block in curly bracket it compiles again...

OlegHahm · 2015-09-03T22:11:18Z

But without the assert things seem to work as they should.

kaspar030 · 2015-09-04T09:58:07Z

@OlegHahm Assuming #3774 gets merged, should I squash?

OlegHahm · 2015-09-04T10:03:42Z

Go ahead!

kaspar030 · 2015-09-04T13:39:50Z

squash-added missing license header

OlegHahm · 2015-09-04T15:42:11Z

Double green!

kaspar030 · 2015-09-04T15:42:53Z

Nice, but no ACK ...

OlegHahm · 2015-09-04T15:59:33Z

Sorry, couldn't find a way to break it. ACK

kaspar030 · 2015-09-04T16:00:12Z

&go! thanks for testing!

sys: net: add gnrc netdev2 support

kaspar030 added 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 NSTF labels Aug 21, 2015

kaspar030 assigned miri64 Aug 21, 2015

kaspar030 added the State: waiting for other PR State: The PR requires another PR to be merged first label Aug 21, 2015

kaspar030 reviewed Aug 21, 2015
View reviewed changes

miri64 mentioned this pull request Aug 21, 2015

sys: add improved network device API #3210

Merged

miri64 reviewed Aug 21, 2015
View reviewed changes

kaspar030 force-pushed the add_gnrc_netdev2_support branch from db6880b to 2bc998c Compare August 26, 2015 09:41

This was referenced Aug 26, 2015

drivers: add cc110x driver #3716

Merged

drivers: add driver for encx24j600 ethernet chips #2800

Merged

kaspar030 force-pushed the add_gnrc_netdev2_support branch from 2bc998c to b22f769 Compare August 26, 2015 15:35

kaspar030 added Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. and removed State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Aug 26, 2015

kaspar030 force-pushed the add_gnrc_netdev2_support branch from b22f769 to 9e72aa4 Compare August 26, 2015 16:45

kaspar030 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Aug 26, 2015

miri64 reviewed Aug 27, 2015
View reviewed changes

sys/include/net/gnrc/gnrc_netdev2.h

Copy link
Copy Markdown

Member

miri64 Aug 27, 2015

Choose a reason for hiding this comment

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

const?

kaspar030 assigned OlegHahm and unassigned miri64 Sep 3, 2015

OlegHahm reviewed Sep 3, 2015
View reviewed changes

kaspar030 force-pushed the add_gnrc_netdev2_support branch from e87907b to 8deb11b Compare September 4, 2015 10:19

kaspar030 added State: waiting for other PR State: The PR requires another PR to be merged first and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Sep 4, 2015

kaspar030 added 3 commits September 4, 2015 15:39

sys: net: gnrc: add basic netdev2 support

78b4bf6

sys: net: replace dev_eth with netdev2, adapt native tap driver

ef97273

tests: remove obsolete ethernet tests

4e5a675

kaspar030 force-pushed the add_gnrc_netdev2_support branch from 8deb11b to 4e5a675 Compare September 4, 2015 13:39

kaspar030 added a commit that referenced this pull request Sep 4, 2015

Merge pull request #3683 from kaspar030/add_gnrc_netdev2_support

48baf5d

sys: net: add gnrc netdev2 support

kaspar030 merged commit 48baf5d into RIOT-OS:master Sep 4, 2015

kaspar030 deleted the add_gnrc_netdev2_support branch September 4, 2015 16:00

Conversation

kaspar030 commented Aug 21, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaspar030 Aug 21, 2015 via email

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Aug 21, 2015

Uh oh!

kaspar030 commented Aug 21, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaspar030 commented Aug 26, 2015

Uh oh!

kaspar030 commented Aug 26, 2015

Uh oh!

miri64 commented Aug 26, 2015

Uh oh!

kaspar030 commented Aug 26, 2015

Uh oh!

kaspar030 commented Aug 26, 2015

Uh oh!

kaspar030 commented Aug 27, 2015

Uh oh!

emmanuelsearch commented Aug 27, 2015

Uh oh!

kaspar030 commented Aug 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaspar030 commented Sep 3, 2015

Uh oh!

OlegHahm commented Sep 3, 2015

Uh oh!

kaspar030 commented Sep 3, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OlegHahm commented Sep 3, 2015

Uh oh!

OlegHahm commented Sep 3, 2015

Uh oh!

kaspar030 commented Sep 3, 2015

Uh oh!

kaspar030 commented Sep 3, 2015

Uh oh!

OlegHahm commented Sep 3, 2015

Uh oh!

OlegHahm commented Sep 3, 2015

Uh oh!

kaspar030 commented Sep 4, 2015

Uh oh!

OlegHahm commented Sep 4, 2015

Uh oh!