gnrc: generalize gnrc_sixlowpan to enable resuability by cgundogan · Pull Request #8218 · RIOT-OS/RIOT

cgundogan · 2017-12-06T19:19:51Z

The current implementation of 802.15.4 adaptations and the handling of dispatch types is closely tied to the 6lowpan module (it's basically the same). Thus, it is not possible to reuse lowpan functionalities, like e.g. the lowpan fragmentation, for non-6lowpan frames.

This PR makes the following generalizations and changes:

rename GNRC_NETTYPE_SIXLOWPAN to GNRC_NETTYPE_LOWPAN
the 6LoWPAN thread is renamed to LoWPAN and directory structure is adjutsed
fragmentation is moved out of 6lowpan and added to the more generic lowpan module

Motivation: Generalizing the 6lowpan module to lowpan allows for further extensions and reusability by non-IP protocols. My goal is to implement a LoWPAN layer for ICN without duplicating code for IPv6/6lowpan agnostic dispatch types (like fragmentation).

@miri64 I would like to hear your ideas and feedback about this generalization. Oh, and please pardon me if I changed/renamed too much (:

miri64 · 2017-12-06T19:21:28Z

rename GNRC_NETTYPE_SIXLOWPAN to GNRC_NETTYPE_LOWPAN

Shouldn't it rather be 6LO???!?

miri64 · 2017-12-06T19:37:12Z

Ah... should have read the whole description before posting ^^". I have some head-aches with this PR

If we rename the module I would rather rename it to gnrc_sixlow to reflect the non-WPAN (aka IEEE 802.15.4) work of the 6Lo-WG. Much of the 6LoWPAN can be reused for that as well. Renaming it to gnrc_lowpan seems like a weird choice with that in mind.
If people search for 6LoWPAN code I expect them to either search for a variation of "6LoWPAN" or "6Lo", so I'm not sure this makes it harder to find the 6LoWPAN implementation or API.
Is the rename really necessary (even to 6Lo)? The name has has historic implication everyone(TM) knows what is meant with "6LoWPAN", while LoWPAN describes IEEE 802.15.4 at best.

Apart from that: though this PR claims it generalizes 6LoWPAN I have the feeling it really doesn't (why does gnrc_lowpan require ipv6_hdr e.g.?) and just moves stuff around. But as said: this is more a feeling than a substantial claim.

cgundogan · 2017-12-06T19:53:59Z

Thanks for your quick feedback (:

Is the rename really necessary (even to 6Lo)? The name has has historic implication everyone(TM) knows what is meant with "6LoWPAN", while LoWPAN describes IEEE 802.15.4 at best.

The problem with "6LoWPAN" is that this would lead to confusions for other network layer technologies (=>CCN/NDN). Though, I did not remove 6LoWPAN per se, it's still existent. But the more general module "LoWPAN" is doing the heavy lifting now (fragmentation handling, dispatching, ...).

Apart from that: though this PR claims it generalizes 6LoWPAN I have the feeling it really doesn't (why does gnrc_lowpan require ipv6_hdr e.g.?) and just moves stuff around. But as said: this is more a feeling than a substantial claim.

There are still some smaller parts that need to be refactored. E.g. there's a hard dependency in the fragmentation code to IPHC, which is IMO wrong. Though, changing this would require a little more moving around that's why I left it as is for now.

cgundogan · 2017-12-06T19:57:18Z

I also figured that RFC4944 has a pretty IP agnostic wording for these kind of dispatches. So I think it's in line with the RFC to remove the SIXLOWPAN prefix from many of the current macros.

miri64 · 2017-12-07T10:44:50Z

I also figured that RFC4944 has a pretty IP agnostic wording for these kind of dispatches. So I think it's in line with the RFC to remove the SIXLOWPAN prefix from many of the current macros.

Maybe we should name the module gnrc_rfc4944 then ^^.

miri64 · 2017-12-07T10:48:16Z

Can you maybe separate the generalization from the rename? I think that would simplify the review greatly.

cgundogan · 2017-12-07T17:19:27Z

@miri64 This PR basically only consists of renaming and reordering. I did not include any new functionalities. I'll rearrange the commits so that the macro renames happen in a separate commit. I also included a picture of what my motivation is (not for this PR, but follow-ups) in the main comment.

miri64 · 2017-12-13T10:55:59Z

sys/include/net/gnrc/netif/lowpan.h

 *
 * @file
- * @brief   6LoWPAN definitions for @ref net_gnrc_netif
+ * @brief   Lowpan definitions for @ref net_gnrc_netif


miri64 · 2017-12-13T10:56:13Z

sys/include/net/gnrc/netif/lowpan.h


 /**
- * @brief   6Lo component of @ref gnrc_netif_t
+ * @brief   Lowpan component of @ref gnrc_netif_t


miri64 · 2017-12-13T10:56:20Z

sys/include/net/gnrc/netif/lowpan.h

 typedef struct {
    /**
-     * @brief   Maximum fragment size for 6Lo fragmentation.
+     * @brief   Maximum fragment size for lowpan fragmentation.


miri64 · 2017-12-13T11:01:06Z

sys/include/net/sixlowpan.h

 *          </a>
 */
-#define SIXLOWPAN_IPHC1_DISP_MASK   (0xe0)
+#define LOWPAN_IPHC1_DISP_MASK      (0xe0)


Here and throughout this file: since the name of the file is sixlowpan.h, I would keep the name SIXLOWPAN_

miri64 · 2017-12-13T11:03:11Z

sys/net/gnrc/netif/gnrc_netif.c

 #endif  /* MODULE_GNRC_IPV6 */
-#ifdef MODULE_GNRC_SIXLOWPAN_IPHC
-        case NETOPT_6LO_IPHC:
+#ifdef MODULE_GNRC_LOWPAN_IPHC


This module doesn't exist (and I would prefer to keep the name gnrc_sixlowpan_iphc or rename it to gnrc_sixlo_iphc).

miri64 · 2017-12-13T11:04:07Z

sys/net/gnrc/netif/gnrc_netif.c

-#ifdef MODULE_GNRC_SIXLOWPAN_IPHC
-        case NETOPT_6LO_IPHC:
+#ifdef MODULE_GNRC_LOWPAN_IPHC
+        case NETOPT_LOWPAN_IPHC:


This netopt doesn't exist (and I would prefer to keep the name NETOPT_6LO_IPHC)

miri64 · 2017-12-13T12:00:56Z

Apart from the change request and a little bit of testing I'd like to do tomorrow I think we can merge it.

cgundogan · 2017-12-13T13:02:23Z

@miri64 addressed your comments

miri64

(Preliminary) ACK. Please squash. I still like to test (tomorrow).

smlng

minor nits

smlng · 2017-12-13T21:15:34Z

sys/include/net/gnrc/lowpan.h

+ * @brief   Default priority for the LoWPAN thread.
+ */
+#ifndef GNRC_LOWPAN_PRIO
+#define GNRC_LOWPAN_PRIO				(THREAD_PRIORITY_MAIN - 4)


This is another example of disturbing the work flow by trivialities

smlng · 2017-12-13T21:17:14Z

sys/include/net/gnrc/lowpan/frag.h

 /*
- * Copyright (C) 2015 Martine Lenders <mlenders@inf.fu-berlin.de>
- * Copyright (C) 2015 Hamburg University of Applied Sciences
+ * Copyright (C) 2015, 2017     Hamburg University of Applied Sciences


cut it short to HAW Hamburg would be (more) consistent with recent copyrights.

tcschmidt · 2018-05-25T20:50:28Z

@cgundogan can you fix the issues and get this merged?

miri64 · 2018-05-26T07:27:09Z

There still is some refactoring work done to reduce the coupling of IPHC and fragmentation. This should not be done in this PR, but separate. I already started the work a few month back, but it's a real brain twister. Mainly, because fragmentation is conceptionally a layer below IPHC, but information about the packet before compression (namely length of the first fragment) is required to be present during and this is not easily factored out with the current code structure.

Second, I'm really not happy about the all the renaming going on here. a) The ICNLoWPAN feature is still experimental, so if it will go a different route than this or doesn't come to fruition do we roll all this back? b) even if it becomes a standard, I would rather keep the name. Otherwise, we never reach API stability if we always also change names (though only minor functionality tweaks are required) as soon as the status quo changes. I did also not go through the code and renamed all of this from sixlowpan to sixlo when the first non-IEEE 802.15.4 RFC came out exactly because of that.

tcschmidt · 2018-05-26T14:36:58Z

@cgundogan @miri64 maybe we can discuss naming and how to proceed hand in hand on Monday F2F?

non-blocking comments

cgundogan · 2018-11-20T08:28:01Z

The architecture of handling sixlowpan dispatches changed drastically on master. The core ideas of this PR already transitioned to master so that this PR is useless now.

cgundogan added 5 commits December 6, 2017 19:30

drivers: use GNRC_NETTYPE_LOWPAN

a48a46c

pkg: use GNRC_NETTYPE_LOWPAN

914e498

netif: 6lowpan => lowpan

ce1863b

gnrc: generalize 6lowpan to enable reusability

c8c9317

tests: adjust to GNRC_NETTYPE_LOWPAN

3c509e6

cgundogan added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation GNRC Area: network Area: Networking labels Dec 6, 2017

cgundogan assigned miri64 Dec 6, 2017

cgundogan requested a review from miri64 December 6, 2017 19:19

miri64 requested changes Dec 13, 2017

View reviewed changes

fixup! gnrc: generalize 6lowpan to enable reusability

d55914a

cgundogan force-pushed the pr/generalize_6lowpan branch from 3b32f8a to d55914a Compare December 13, 2017 13:01

cgundogan added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Dec 13, 2017

miri64 reviewed Dec 13, 2017

View reviewed changes

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable labels Dec 13, 2017

smlng previously requested changes Dec 13, 2017

View reviewed changes

miri64 mentioned this pull request Feb 2, 2018

gnrc_sixlowpan: Refactoring #8511

Closed

6 tasks

miri64 removed the GNRC label Oct 5, 2018

cgundogan closed this Nov 20, 2018

Conversation

cgundogan commented Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Dec 6, 2017

Uh oh!

miri64 commented Dec 6, 2017

Uh oh!

cgundogan commented Dec 6, 2017

Uh oh!

cgundogan commented Dec 6, 2017

Uh oh!

miri64 commented Dec 7, 2017

Uh oh!

miri64 commented Dec 7, 2017

Uh oh!

cgundogan commented Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

miri64 commented Dec 13, 2017

Uh oh!

cgundogan commented Dec 13, 2017

Uh oh!

miri64 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smlng left a comment

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!

tcschmidt commented May 25, 2018

Uh oh!

miri64 commented May 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcschmidt commented May 26, 2018

Uh oh!

cgundogan commented Nov 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cgundogan commented Dec 6, 2017 •

edited

Loading

cgundogan commented Dec 7, 2017 •

edited

Loading

miri64 left a comment •

edited

Loading

miri64 commented May 26, 2018 •

edited

Loading