sys: net: introduce low-level ethernet driver API #2766

kaspar030 · 2015-04-08T19:10:42Z

While implementing the ethernet driver for encx24j600, I realized that ng_netdev is too fat for some applications, so I tried to create an interface just for ethernet drivers.

The actual tap device handling is taken from #2558.

edit
Let me rephrase the "too fat" statement, as it is totally inaccurate.

IMHO the currently developed network stack might not be the only one we might have in RIOT.
So when I started developing an ethernet driver for RIOT, I didn't want to tie it to netdev and friends.
netdev does make some assumptions on how a driver has to be implemented, and also implies a lot of dependencies.

This PR introduces a fairly thin interface with no dependencies, which can easily be used with netdev (see #2776), but doesn't depend on it.

I've implemented two drivers using it (the tap driver in this PR and a driver for encx24j600 chips, which I'll PR soon). As always with interfaces designed by yourself, I'm quite happy with the result.

The only disadvantage of using this interface instead of directly writing a driver for netdev is that pktsnips have to be converted to a flat buffer before sending to the device. This could be added to the interface if deemed necessary.

cgundogan · 2015-04-08T19:25:49Z

cpu/native/ng_net/dev_eth_tap.c

+            err(EXIT_FAILURE, "dev_eth_tap: read");
+        }
+    }
+    else {


Please have a look at #2686 and #2664 (for the case nread == 0)

kaspar030 · 2015-04-09T06:26:55Z

@cgundogan integrated.

haukepetersen · 2015-04-09T14:11:31Z

tests/dev_eth/main.c

+    thread_wakeup(handler_pid);
+}
+
+void dev_eth_rx_handler(dev_eth_t *dev) {


this is never being called?

It is supposed to be called from a drivers _isr implementation. Does the test app work (e.g., print something if you sent a packet to the node / broadcast one)?

kaspar030 · 2015-04-13T13:00:35Z

updated
rebased

haukepetersen · 2015-04-15T05:31:46Z

I might have an idea to prevent having to copy the data to send into one continuous chunk before you send it: how about splitting the send function in three parts: prepare, load, and execute. This way you could call the load function with an data offset parameter multiple times and then call exec once all the data is loaded into the device buffer... The original send function could stay for convenience reasons, it would just call the three 'lower-level' functions.

haukepetersen · 2015-04-15T05:33:49Z

Also I am not quite sure about the global functions you defined outside the eth_driver_t struct. I have the impression that they do not scale very well if you have multiple ethernet devices?!

kaspar030 · 2015-04-15T06:58:44Z

... splitting the send function in three parts ...
I came to the same conclusion. Will change the interface accordingly.

Also I am not quite sure about the global functions you defined outside the eth_driver_t struct. I have the impression that they do not scale very well if you have multiple ethernet devices?!

Why not? Those functions are called with the device handle as parameter. Should be easy to dispatch to whatever the network stack needs.

kaspar030 · 2015-04-15T07:00:45Z

load function with an data offset parameter

Do you think we can omit that parameter? The two drivers I have don't need it.

OlegHahm · 2015-04-15T07:38:28Z

I didn't really follow the latest discussions on netdev and this stuff, but looking at the proposed interface that looks exactly how I remember netdev to look like. Now, that I look at driver/include/netdev again, I see that it became a little bit bigger over time, but still I don't really get the purpose/advantage of your interface. What dependencies are pulled in by netdev?

kaspar030 · 2015-04-15T07:53:35Z

@OlegHahm ng_netdev pulls at least ng_pkt, ng_pktbuf, ng_netconf. All of them are not needed for a low-level driver. pktbuf alone disqualifies the interface for class-0 devices. And as you realized, the interface is nearly the same, just without those dependencies. That is exactly the purpose / advantage of it.

OlegHahm · 2015-04-15T07:57:54Z

ng_netdev pulls at least ng_pkt, ng_pktbuf, ng_netconf.

Maybe I'm starring at the wrong files. Can you point me to these dependencies?

And as you realized, the interface is nearly the same, just without those dependencies. That is exactly the purpose / advantage of it.

Sounds completely reasonable to me. But this sounds to me as we should use this interface as a base and put additional features as a kind of add-on on top of it if necessary.

(Sorry if I discuss about this from a rather abstract level, but I don't have time to dig into the actual code too much.)

kaspar030 · 2015-04-15T09:18:59Z

ng_netdev pulls at least ng_pkt, ng_pktbuf, ng_netconf.

Maybe I'm starring at the wrong files. Can you point me to these dependencies?

From ng_netdev.h:

#include "net/ng_pkt.h"
#include "net/ng_netconf.h"

Actually pktbuf is not a direct dependency, but ng_pkt.h states:
@note This type has no initializer on purpose. Please use @ref net_ng_pktbuf as factory.

Also the pktsnip_t is more complex than neccessary, forcing this additional complexity into every triver that uses ng_netdev.

Sounds completely reasonable to me. But this sounds to me as we should use this interface as a base and put additional features as a kind of add-on on top of it if necessary.

Which interface is "this" in that sentence? :)

OlegHahm · 2015-04-15T09:40:05Z

Ah, I was indeed looking at the wrong file. Why is device driver interface specified in sys? Hm, ng_netconf.h is just header including some enums, so this dependency is bearable.

For the the other depencies, I agree that this is a bit unfortunate. Can we find a way to abstract from this?

I agree, that it may make sense to have different network stacks with different focuses (class 0 devices or class 1+ devices, for instance), but on the device driver level, I would have assumed that it is possible to define a basic set of functions that can (almost) always be satisfied.

Which interface is "this" in that sentence? :)

Sorry, I meant the interface you propose in this PR.

kaspar030 · 2015-04-15T09:47:59Z

Why is device driver interface specified in sys?

You're saying it should move to drivers/include?

Hm, ng_netconf.h is just header including some enums, so this dependency is bearable.

Agreed. Still personally I always prefer not to dictate implementation through interface, just for the flexibility.

I agree, that it may make sense to have different network stacks with different focuses (class 0 devices or class 1+ devices, for instance), but on the device driver level, I would have assumed that it is possible to define a basic set of functions that can (almost) always be satisfied.

Totally. That's what I tried with this interface. Should be simple to extend it so it also works for radio devices. And it can already be used with the ng network stack.

OlegHahm · 2015-04-15T10:25:32Z

You're saying it should move to drivers/include?

That seems the natural place for me to look for a device driver interface - and that's the place where I searched for it.

Totally. That's what I tried with this interface. Should be simple to extend it so it also works for radio devices. And it can already be used with the ng network stack.

@haukepetersen, @authmillenon, do you have any opinion on this?

miri64 · 2015-04-15T13:37:26Z

@OlegHahm I'm also of the opinion to put it into drivers (as the legacy version of netdev you found there indicates). @haukepetersen however put it into sys/include/net since he feels it more belonging to the network architecture than the device architecture. The arguments for and against drivers/include or sys/include/net weigh each other pretty much out, I guess.

His argument was more or less, that yes, it's device related and it presents an API for a driver (that can in theory be directly connected to the hardware), but things like this PR and previous discussions not to enforce ng_netdev on devices, mainly because its strong dependency to ng_pktbuf, show that it is more an API for the network stack to speak with a device.

Does this reflect your argument @haukepetersen?

OlegHahm · 2015-04-15T14:08:50Z

Why is it that dependent on ng_pktbuf? In the end the device just needs a pointer to some data (for sending) or a pointer to some buffer where to copy data (for receiving).

IMO we should have network device interface (i.e. netdev) in drivers/include that can be used as a basis for all network stacks. Otherwise (being dependent on RIOT specific structures) we would have to implement a device driver for each network stack like OpenWSN.

miri64 · 2015-04-15T14:13:54Z

@OlegHahm because a network device is required to at least release a sending packet from packet buffer, because it is the last one touching it. Apart from it sure, you can just use arbitrary ng_pktsnip_t pointers, but in practice it makes most sense to put them into the packet buffer directly to reduce overhead.

haukepetersen · 2015-04-15T16:34:07Z

I spend some more thoughts about these driver interfaces the last days, but I have not come up with a clear opinion, yet. I understand @kaspar030s thoughts about netdev being too complicated/having too many implications on the actual implementation, but I don't see them necessarily as too bad...

I guess creating true low-level driver interfaces for ethernet, ieee802.15.4 etc and mapping the netdev interface on top of these would be an option. This is in my opinion however only valid, if we do not introduce addional copying of data and/or addiontial depth in function call hierarchy... Otherwise we would take efficiency away from the netdev interface.

miri64 · 2015-04-15T20:42:24Z

My question about your opinion was more about the directory of ng_netdev.h.

OlegHahm · 2015-05-18T12:58:37Z

cpu/native/ng_net/dev_eth_tap.c

+/*
+ * @ingroup net_dev_eth
+ * @{
+ * @file    dev_eth_tap.c


remove the filename

kaspar030 · 2015-05-18T18:01:18Z

@OlegHahm I think this is squashable?

OlegHahm · 2015-05-18T18:09:57Z

sure, go ahead

kaspar030 · 2015-05-19T07:56:48Z

squashed
rebased

OlegHahm · 2015-05-21T21:07:01Z

cpu/native/ng_net/dev_eth_tap.c

+    }
+    else if (nread == -1) {
+        if ((errno == EAGAIN) || (errno == EWOULDBLOCK)) {
+        }


empty if case. :) you made me remove the warning.

OlegHahm · 2015-05-21T21:13:17Z

The last two comments are no stoppers. I leave it to you, @kaspar030, if you want to address them. Otherwise go ahead with merging.

OlegHahm · 2015-05-22T07:48:52Z

sys/include/dev_eth_autoinit.h

+ * @defgroup    sys_net_dev_eth dev_eth auto setup
+ * @ingroup     sys_net_eth
+ * @brief       Automatically setup available ethernet devices
+ * @{


@file is missing

kaspar030 · 2015-05-26T13:36:59Z

-rebased
-addressed @OlegHahm's doxygen comments

kaspar030 · 2015-05-26T13:37:29Z

go, travis, go!

…nterface

miri64 · 2015-05-26T17:10:09Z

driver_nrfmin is failing, but this is unrelated to this PR. Go as soon as it goes … red

kaspar030 · 2015-05-26T17:28:10Z

GO.

sys: net: introduce low-level ethernet driver API

OlegHahm · 2015-05-26T17:30:08Z

🎉 🍻

cgundogan reviewed Apr 8, 2015
View reviewed changes

haukepetersen reviewed Apr 9, 2015
View reviewed changes

miri64 mentioned this pull request Apr 9, 2015

Network stack refactoring: TODO list #2278

Closed

36 tasks

This was referenced Apr 9, 2015

sys: net: add netdev device driver using dev_eth low-level ethernet driver API #2770

Closed

sys: net: add netdev device driver using dev_eth low-level ethernet driver API #2776

Merged

OlegHahm assigned haukepetersen Apr 9, 2015

kaspar030 force-pushed the add_dev_eth branch from f194b75 to a7c53f5 Compare April 13, 2015 12:58

kaspar030 mentioned this pull request Apr 13, 2015

drivers: add driver for encx24j600 ethernet chips #2800

Merged

OlegHahm reviewed May 18, 2015
View reviewed changes

cpu/native/ng_net/dev_eth_tap.c

/*

* @ingroup net_dev_eth

* @{

* @file dev_eth_tap.c

Copy link

Member

OlegHahm May 18, 2015

Choose a reason for hiding this comment

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

remove the filename

kaspar030 force-pushed the add_dev_eth branch 2 times, most recently from f0a2b82 to 6c579c6 Compare May 19, 2015 07:56

OlegHahm removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label May 19, 2015

OlegHahm reviewed May 21, 2015
View reviewed changes

OlegHahm reviewed May 22, 2015
View reviewed changes

kaspar030 force-pushed the add_dev_eth branch from 6c579c6 to 6e30218 Compare May 26, 2015 13:36

kaspar030 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 26, 2015

kaspar030 added 4 commits May 26, 2015 15:37

sys: net: dev_eth: add low-level ethernet driver interface

f8177de

cpu: native: add tap implementation of dev_eth (ng_nativenet)

fbfb095

sys: auto_init: add auto initialization for dev_eth

7a80d31

tests: dev_eth: add test application for dev_eth low-level ethernet i…

6e30218

…nterface

kaspar030 added a commit that referenced this pull request May 26, 2015

Merge pull request #2766 from kaspar030/add_dev_eth

cfd9a59

sys: net: introduce low-level ethernet driver API

kaspar030 merged commit cfd9a59 into RIOT-OS:master May 26, 2015

kaspar030 deleted the add_dev_eth branch May 26, 2015 17:28

rfswarm mentioned this pull request Jun 15, 2015

sys: ble: introduce low-level ble driver API #3206

Closed

sys: net: introduce low-level ethernet driver API #2766

sys: net: introduce low-level ethernet driver API #2766

Conversation

kaspar030 commented Apr 8, 2015

Choose a reason for hiding this comment

kaspar030 commented Apr 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaspar030 commented Apr 13, 2015

haukepetersen commented Apr 15, 2015

haukepetersen commented Apr 15, 2015

kaspar030 commented Apr 15, 2015

kaspar030 commented Apr 15, 2015

OlegHahm commented Apr 15, 2015

kaspar030 commented Apr 15, 2015

OlegHahm commented Apr 15, 2015

kaspar030 commented Apr 15, 2015

OlegHahm commented Apr 15, 2015

kaspar030 commented Apr 15, 2015

OlegHahm commented Apr 15, 2015

miri64 commented Apr 15, 2015

OlegHahm commented Apr 15, 2015

miri64 commented Apr 15, 2015

haukepetersen commented Apr 15, 2015

miri64 commented Apr 15, 2015

Choose a reason for hiding this comment

kaspar030 commented May 18, 2015

OlegHahm commented May 18, 2015

kaspar030 commented May 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OlegHahm commented May 21, 2015

Choose a reason for hiding this comment

kaspar030 commented May 26, 2015

kaspar030 commented May 26, 2015

miri64 commented May 26, 2015

kaspar030 commented May 26, 2015

OlegHahm commented May 26, 2015