nettype: intial import #2397

miri64 · 2015-02-06T16:10:44Z

Type to define protocols

miri64 · 2015-02-06T16:21:04Z

~~Please do not merge yet. I realized there are still some minor issues to this header. :D~~

haukepetersen · 2015-02-06T16:52:10Z

Does it make sense to put the ng header already into sys/include/net/.. as there is nothing in this place at this point anyway?

miri64 · 2015-02-06T17:30:09Z

I thought it might be clearer for newcomers to the new network stack to give a clear "this is where the new stuff is" ;-).

miri64 · 2015-02-06T17:47:37Z

Ready for review :-)

haukepetersen · 2015-02-06T18:03:55Z

still think that starting now to put stuff into sys/include/net is already a good idea...

To the PR: I think it would make sense to take the changes to pkt.h out of this PR as I would say they are unrelated -> I would opt for making these changes when you (re)commit the pktsnip.h file

haukepetersen · 2015-02-06T18:04:38Z

sys/include/ng_net/ng_nettype.h

+ * @note    Expand at will.
+ */
+typedef struct {
+    NG_NETTYPE_UNDEF = 0,       /**< Protocol is undefined */


if we really use these protocol number as array offset, I would not include this value.

This can also be done by doing type - 1 internally. I'd rather have some fallback value for testing etc.

OlegHahm · 2015-02-06T18:18:07Z

What's the purpose of these types? Where should they be used?

OlegHahm · 2015-02-06T18:18:57Z

sys/include/net/ng_nettype.h

+    NG_NETTYPE_IEEE802154,      /**< Protocol is IEEE 802.15.4 */
+#endif
+#ifdef MODULE_NG_SIXLOWPAN
+    NG_NETTYPE_SIXLOWPAN,       /**< Protocol is 6LoWPAN */


Hm, I would not call 6LoWPAN a link layer protocol.

Me neither. But in our stack it would take the place of a link layer protocol, since it sits behind a network interface of the IPv6 layer.

I don't get this. In the envisioned stack I'm theoretically able to put as many threads/modules/layers as I want (in case I have enough memory) between two OSI layers. I would either drop the substructuring at all or make a new subsection for adaptation layers.

+--------------+ | IPv6 | | IF | IF | +--------------+ ^ ^ netapi netapi v v +-----+ +-----+ | MAC | | 6LP | | | +-----+ | | ^ | | netapi | | v | | +-----+ | | | MAC | +-----+ +-----+ |radio| |radio| +-----+ +-----+

Does it make anymore sense to you now?

Putting 6LoWPAN between anything other than IPv6 and a MAC is non-sensical

Does it make anymore sense to you now?

No. I still disagree calling 6LoWPAN a link layer protocol. That's simply wrong for me.

I can take it out of the group, but I thought the implementation would be easier to find then, since it would also reside in sys/net/link_layer (if not their, where else)?

Either create a new folder name something like "adaption_layer" or leave it in network layer.

I'm in favor of putting/leaving it in the network layer.

miri64 · 2015-02-06T18:32:10Z

Is replacement for pkt_proto_t and in older versions netdev_proto_t. They are also used to identify protocols in the netreg.

OlegHahm · 2015-02-06T18:35:14Z

sys/include/net/ng_nettype.h

+#endif
+#ifdef MODULE_NG_ICMPV6
+    NG_NETTYPE_ICMPV6,          /**< Protocol is ICMPv6 */
+    NG_NETTYPE_ICMPV6_ECHO,     /**< Protocol is ICMPv6, echo reply/request */


Does it really make sense for upper layers to register for an echo reply?

Sure, for example a ping application, or a application if the tests result are as expected in these things.

Sorry, I meant echo request.

Goes the same for the Plugtests

haukepetersen · 2015-02-06T19:12:10Z

I think it would make sense to PR the interface for netreg as soon as possible, so this PR can be put into context - it might help for clarification!

cgundogan · 2015-02-06T19:32:37Z

sys/include/net/ng_nettype.h

+ *
+ * @note    Expand at will.
+ */
+typedef struct {


Why is this not an enum? Can this actually compile this way as a struct def?

You're right! Will fix this

miri64 · 2015-02-07T12:03:44Z

@cgundogan sure, if someone implements a version of those protocols that utilizes netapi we can add them. I only added the stuff we decided to implement for now.

miri64 · 2015-02-07T13:49:01Z

@haukepetersen done: #2404

miri64 · 2015-02-07T13:49:22Z

Fixed @cgundogan comment

miri64 · 2015-02-07T18:14:28Z

Removed the ICMPv6 subtypes. Registration can be done doing e.g.

netreg_register(NETTYPE_ICMPV6, ipv6_pid, ICMPV6_TYPE_RPL, rpl_pid);

haukepetersen · 2015-02-09T17:36:58Z

sys/include/net/ng_nettype.h

+     * @name Link Layer
+     */
+#ifdef MODULE_NG_IEEE802154
+    NG_NETTYPE_IEEE802154,      /**< Protocol is IEEE 802.15.4 */


as discussed today, in my opinion we can drop this option, as link layers should register independent of their protocol

haukepetersen · 2015-02-09T17:38:24Z

looks good to me (except the 15.4 entry in the list). ack when fixed.

miri64 · 2015-02-09T23:34:19Z

Addressed @haukepetersen's comments.

cgundogan · 2015-02-10T08:13:09Z

@authmillenon

Removed the ICMPv6 subtypes. Registration can be done doing e.g.
netreg_register(NETTYPE_ICMPV6, ipv6_pid, ICMPV6_TYPE_RPL, rpl_pid);

Where would be / is ICMPV6_TYPE_RPL defined?
Why is NG_NETTYPE_AODV defined and not NG_NETTYPE_RPL?

EDIT: I understand that you removed the ICMPv6 subtypes a commit earlier, but why did you not remove the NG_NETTYPE_AODV?

haukepetersen · 2015-02-10T08:44:29Z

@cgundogan: RPL is missing indeed, but as the code states: everyone who adds a new protocol should just add it to this list. As I understood it, the current list includes just some randomly selected protocols to show how this file is supposed to work - but it is not complete, so adding e.g. RPL, TCP, etc, has to be done whenever those implementations are added...

cgundogan · 2015-02-10T08:49:20Z

@haukepetersen Well, for the sake of completeness, I would like to see the RPL NETTYPE also in this PR - both, because it is an existing module in RIOT and to prevent further time consuming questions in a later phase

haukepetersen · 2015-02-10T08:53:42Z

fine with me. @authmillenon, could you add at least RPL and TCP to this list?

miri64 · 2015-02-10T11:40:46Z

For RPL the idea was to do it via demux context (as described) so a NETTYPE_RPL is not needed.

Where would be / is ICMPV6_TYPE_RPL defined?

Where ever the values for the ICMPv6 type field will be defined.

Why is NG_NETTYPE_AODV defined and not NG_NETTYPE_RPL?

Because AODV uses its own package format, while RPL utilizes ICMPv6. AODV will be demultiplexed in the IP thread's dultiplexer (as ICMPv6, UDP, TCP, etc.), while RPL will be demultiplexed in the ICMPv6 module. @haukepetersen and I talked about this dual nature that you basically have two ways now to register to a protocol yesterday, but come to the conclusion, that this is the most memory-saving one, while also being flexible.

As for TCP, I can re-add it.

cgundogan · 2015-02-10T11:54:54Z

@authmillenon I am not sure if this is relevant here, but data-plane packets (udp, tcp ..) can and most probably will be annotated later with an IPv6-Hop-By-Hop extension header with RPL information (https://tools.ietf.org/html/rfc6553). These packets, regardless of their transport layer, need to interact with RPL in some way.

miri64 · 2015-02-10T11:58:22Z

@cgundogan then RPL can also register via netreg_register(NETTYPE_IPV6, IPV6_EXT_HOP_BY_HOP, rpl_pid) ;).

cgundogan · 2015-02-10T12:09:11Z

@authmillenon I am still not convinced.. AODV can also use netreg_register(NETTYPE_UDP, AODV_ROUTE_REQUEST, aodv_pid) and similar with AODV_ROUTE_REPLY and _ERROR.

All AODV messages are sent to port 654 using UDP. (https://www.ietf.org/rfc/rfc3561.txt)

haukepetersen · 2015-02-10T12:11:44Z

True, AODV utilizes UDP, so there is actually no reason to include it into this list.

For RPL @authmillenon is right, it registers for certain types of next-headers and ICMP messages, so there is also no reason to include RPL in this list.

miri64 · 2015-02-10T12:18:58Z

Done

haukepetersen · 2015-02-10T12:24:13Z

thx. I have nothing to add: ACK (merge at will once Travis is happy)

miri64 · 2015-02-10T12:33:23Z

Squashed

cgundogan · 2015-02-10T13:13:17Z

me gusta, let's wait for travis and then merge

nettype: intial import

miri64 added this to the Network Stack Task Force milestone Feb 6, 2015

miri64 added NSTF Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Area: network Area: Networking labels Feb 6, 2015

haukepetersen self-assigned this Feb 6, 2015

miri64 added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Feb 6, 2015

miri64 force-pushed the nettype/feat/initial branch from 9e4e57b to 065b6b3 Compare February 6, 2015 17:50

haukepetersen reviewed Feb 6, 2015
View reviewed changes

OlegHahm reviewed Feb 6, 2015
View reviewed changes

OlegHahm modified the milestone: Network Stack Task Force Feb 6, 2015

OlegHahm added NSTF and removed NSTF labels Feb 6, 2015

OlegHahm reviewed Feb 6, 2015
View reviewed changes

cgundogan reviewed Feb 6, 2015
View reviewed changes

miri64 mentioned this pull request Feb 7, 2015

netreg: Initial import #2404

Merged

miri64 mentioned this pull request Feb 7, 2015

pkt: simplify API #2342

Merged

miri64 mentioned this pull request Feb 8, 2015

Network stack refactoring: TODO list #2278

Closed

36 tasks

haukepetersen reviewed Feb 9, 2015
View reviewed changes

miri64 mentioned this pull request Feb 9, 2015

netif: Initial import of ng_* version #2417

Merged

nettype: intial import

9d36c05

miri64 force-pushed the nettype/feat/initial branch from 851be09 to 9d36c05 Compare February 10, 2015 12:33

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

miri64 added a commit that referenced this pull request Feb 10, 2015

Merge pull request #2397 from authmillenon/nettype/feat/initial

e5e1d0d

nettype: intial import

miri64 merged commit e5e1d0d into RIOT-OS:master Feb 10, 2015

miri64 deleted the nettype/feat/initial branch February 10, 2015 14:10

nettype: intial import #2397

nettype: intial import #2397

Conversation

miri64 commented Feb 6, 2015

miri64 commented Feb 6, 2015

haukepetersen commented Feb 6, 2015

miri64 commented Feb 6, 2015

miri64 commented Feb 6, 2015

haukepetersen commented Feb 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OlegHahm commented Feb 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miri64 commented Feb 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haukepetersen commented Feb 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miri64 commented Feb 7, 2015

miri64 commented Feb 7, 2015

miri64 commented Feb 7, 2015

miri64 commented Feb 7, 2015

Choose a reason for hiding this comment

haukepetersen commented Feb 9, 2015

miri64 commented Feb 9, 2015

cgundogan commented Feb 10, 2015

haukepetersen commented Feb 10, 2015

cgundogan commented Feb 10, 2015

haukepetersen commented Feb 10, 2015

miri64 commented Feb 10, 2015

cgundogan commented Feb 10, 2015

miri64 commented Feb 10, 2015

cgundogan commented Feb 10, 2015

haukepetersen commented Feb 10, 2015

miri64 commented Feb 10, 2015

haukepetersen commented Feb 10, 2015

miri64 commented Feb 10, 2015

cgundogan commented Feb 10, 2015