-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gnrc: generalize gnrc_sixlowpan to enable resuability #8218
Conversation
Shouldn't it rather be |
Ah... should have read the whole description before posting ^^". I have some head-aches with this PR
Apart from that: though this PR claims it generalizes 6LoWPAN I have the feeling it really doesn't (why does |
Thanks for your quick feedback (:
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, ...).
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. |
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 |
Maybe we should name the module |
Can you maybe separate the generalization from the rename? I think that would simplify the review greatly. |
@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. |
sys/include/net/gnrc/netif/lowpan.h
Outdated
@@ -11,12 +12,13 @@ | |||
* @{ | |||
* | |||
* @file | |||
* @brief 6LoWPAN definitions for @ref net_gnrc_netif | |||
* @brief Lowpan definitions for @ref net_gnrc_netif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LoWPAN
sys/include/net/gnrc/netif/lowpan.h
Outdated
@@ -25,21 +27,21 @@ extern "C" { | |||
#endif | |||
|
|||
/** | |||
* @brief 6Lo component of @ref gnrc_netif_t | |||
* @brief Lowpan component of @ref gnrc_netif_t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LoWPAN
sys/include/net/gnrc/netif/lowpan.h
Outdated
*/ | ||
typedef struct { | ||
/** | ||
* @brief Maximum fragment size for 6Lo fragmentation. | ||
* @brief Maximum fragment size for lowpan fragmentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LoWPAN
sys/include/net/sixlowpan.h
Outdated
|
||
/** | ||
* @brief Dispatch mask for LOWPAN_IPHC. | ||
* @see <a href="https://tools.ietf.org/html/rfc6282#section-3.1"> | ||
* RFC 6282, section 3.1 | ||
* </a> | ||
*/ | ||
#define SIXLOWPAN_IPHC1_DISP_MASK (0xe0) | ||
#define LOWPAN_IPHC1_DISP_MASK (0xe0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and throughout this file: since the name of the file is sixlowpan.h
, I would keep the name SIXLOWPAN_
sys/net/gnrc/netif/gnrc_netif.c
Outdated
@@ -203,14 +203,14 @@ int gnrc_netif_get_from_netdev(gnrc_netif_t *netif, gnrc_netapi_opt_t *opt) | |||
break; | |||
#endif /* GNRC_IPV6_NIB_CONF_ROUTER */ | |||
#endif /* MODULE_GNRC_IPV6 */ | |||
#ifdef MODULE_GNRC_SIXLOWPAN_IPHC | |||
case NETOPT_6LO_IPHC: | |||
#ifdef MODULE_GNRC_LOWPAN_IPHC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This module doesn't exist (and I would prefer to keep the name gnrc_sixlowpan_iphc
or rename it to gnrc_sixlo_iphc
).
sys/net/gnrc/netif/gnrc_netif.c
Outdated
#ifdef MODULE_GNRC_SIXLOWPAN_IPHC | ||
case NETOPT_6LO_IPHC: | ||
#ifdef MODULE_GNRC_LOWPAN_IPHC | ||
case NETOPT_LOWPAN_IPHC: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This netopt doesn't exist (and I would prefer to keep the name NETOPT_6LO_IPHC
)
Apart from the change request and a little bit of testing I'd like to do tomorrow I think we can merge it. |
3b32f8a
to
d55914a
Compare
@miri64 addressed your comments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Preliminary) ACK. Please squash. I still like to test (tomorrow).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor nits
* @brief Default priority for the LoWPAN thread. | ||
*/ | ||
#ifndef GNRC_LOWPAN_PRIO | ||
#define GNRC_LOWPAN_PRIO (THREAD_PRIORITY_MAIN - 4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indention?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is another example of disturbing the work flow by trivialities
@@ -1,76 +1,77 @@ | |||
/* | |||
* 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cut it short to HAW Hamburg would be (more) consistent with recent copyrights.
@cgundogan can you fix the issues and get this merged? |
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 |
@cgundogan @miri64 maybe we can discuss naming and how to proceed hand in hand on Monday F2F? |
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. |
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:
GNRC_NETTYPE_SIXLOWPAN
toGNRC_NETTYPE_LOWPAN
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 (: