Refactoring 6top Implementation #1

yatch · 2016-11-09T16:56:34Z

Overall, this work untangles 6P message manipulation code and the sample schedule function code which reside in a single file, sixtop.c. Two C-files are introduced: sixtop-6p.c and sixtop-sf-simple.c.

sixtop-6p.c is for 6P message manipulation, building and parsing. sixtop-sf-simple.c is for the sample schedule function which is used by the example application. sixtop.c is now recreated to implement external APIs and to manage protocol states.

There are also changes on variable names and constants to follow wordings in draft-ietf-6tisch-6top-protocol. For example, PAYLOAD_IE_SIXTOP is renamed to PAYLOAD_IE_IANA_IETF. Another example is using "cell" instead of "link".

Speaking of the internet-draft, there are some fixes to comply with the specification. The Metadata field of Add Request and Delete Request is now taken into account. The value for "Seqnum" is maintained per neighbor.

From the point of view of an application, its external behaviors are (hopefully) not changed. You'll see almost the same mote output as the original implementation with rpl-tsch-sixtop-z1.csc as shown below. Although, if you disable the filter of mote output, you'll see many differences in log messages.

You may notice changes of spacing in core/net/mac/tsch/tsch.c. The intention here is to revert the changes considered to have been introduced accidentally at 6ac1dce.

simonduq · 2016-11-23T08:49:10Z

core/net/mac/frame802154e-ie.c

@@ -60,7 +60,8 @@ enum ieee802154e_header_ie_id {
 enum ieee802154e_payload_ie_id {
  PAYLOAD_IE_ESDU = 0,
  PAYLOAD_IE_MLME,
-  PAYLOAD_IE_SIXTOP,
+  PAYLOAD_IE_VENDOR,


what is that one for?

I put it so that PALYLOAD_IE_IANA_IETF (should be renamed to PAYLOAD_IE_IETF) doesn't have the Group ID value for another group, which is "Vendor Specific Nested IE". PAYLOAD_IE_VENDOR is not used effectively anywhere for now.

simonduq · 2016-11-23T08:52:01Z

core/net/mac/frame802154e-ie.c

+          case PAYLOAD_IE_IANA_IETF:
+            /* Now expect 'len' bytes of Sixtop sub-IEs */
+            switch(*buf) {
+            case IANA_IETF_IE_6TOP:


code style: please indent case

simonduq · 2016-11-23T08:52:18Z

core/net/mac/frame802154e-ie.c

+            switch(*buf) {
+            case IANA_IETF_IE_6TOP:
+              ies->sixtop_ie_content_ptr = buf + 1;
+              ies->sixtop_ie_content_len = len - 1;


the line above are not self-explanatory, please add a comment

Agree; put a commen like this: /* adjust buf and len by the size of the Sub-ID field, one octet. */

simonduq · 2016-11-23T08:57:08Z

core/net/mac/framer-802154.c

@@ -97,6 +97,9 @@ create_frame(int type, int do_create)
   * source nor destination address, we have dest PAN ID iff compression is *set*. */
  params.fcf.panid_compression = 0;

+  /* Set IE Present bit */
+  params.fcf.ie_list_present = packetbuf_attr(PACKETBUF_ATTR_MAC_METADATA);


Cool. Maybe we could even use the framer for EB and EACK generation now :)
(if so, definitely a different PR)

I agree with you; it should go with a different PR. I can do it later.

[off-topic] I've rewritten tsch_packet_create_eb() and tsch_packet_create_eack() with PACKETBUF_ATTR_MAC_METADATA in a different branch. It's ready for PR; but waiting for contiki-os#1914 to be merged since the test data depends on contiki-os#1914.

simonduq · 2016-11-23T09:42:31Z

core/net/mac/tsch/sixtop/sixtop-6p.h

+#include "sixtop.h"
+
+int sixtop_6p_build_6top_request(uint8_t return_code, uint8_t sfid,
+                                  uint8_t seqno, const sixtop_msg_body_t *body);


naming for all functions in this file: I would remove _6top_ (already implied by the module name sixtop_6p)

simonduq · 2016-11-23T09:48:54Z

core/net/mac/tsch/sixtop/sixtop.c

 /**
 * \file
- *         IEEE 802.15.4 TSCH MAC Sixtop.
+ *         6top Protocol (6P) - Core


I'm confused, is this 6top (as the file name suggests) or 6P?

Obviously more precise short explanation for the file is needed... Anyway, it's a part of 6P (6top Protocol).

simonduq · 2016-11-23T09:54:01Z

core/net/mac/tsch/sixtop/sixtop-6p.c

+  sixtop_ie->version = buf[0] & 0x0f;
+  sixtop_ie->code = (buf[0] & 0xf0) >> 4;
+  sixtop_ie->sfid = buf[1];
+  sixtop_ie->seqno = buf[2] & 0x07;


shouldn't this be 0x0f?

This should be 0x0f!!

simonduq · 2016-11-23T09:56:24Z

core/net/mac/tsch/sixtop/sixtop-6p.c

+  sixtop_ie->code = (buf[0] & 0xf0) >> 4;
+  sixtop_ie->sfid = buf[1];
+  sixtop_ie->seqno = buf[2] & 0x07;
+  sixtop_ie->gab = (buf[2] & 0x30) >> 0x04;


for clarity and consistency I would not use hex notation for the shift, i.e. >> 4

Yeah, I have no idea why I used hex notation here... Thanks for pointing it out.

simonduq · 2016-11-23T09:56:42Z

core/net/mac/tsch/sixtop/sixtop-6p.c

+  sixtop_ie->sfid = buf[1];
+  sixtop_ie->seqno = buf[2] & 0x07;
+  sixtop_ie->gab = (buf[2] & 0x30) >> 0x04;
+  sixtop_ie->gba = (buf[2] & 0xc0) >> 0x04;


isn't this >> 6?

Yes. Should be >>6.

simonduq · 2016-11-23T09:57:18Z

core/net/mac/tsch/sixtop/sixtop-6p.c

+  buf[0] = SIXTOP_6P_SUBIE_ID;
+  buf[1] = (sixtop_ie->code << 4) | SIXTOP_6P_VERSION;
+  buf[2] = sixtop_ie->sfid;
+  buf[3] = sixtop_ie->seqno & 0x0f;


looks like we do not handle gab and gba. Add a comment

Your eyes are correct. Better to put a comment. The gab/gba thing be added by a following patch..

yatch

I'll resolve all comments from @simonduq with a following patch.

yatch · 2016-11-23T10:23:29Z

core/net/mac/frame802154e-ie.c

@@ -60,7 +60,8 @@ enum ieee802154e_header_ie_id {
 enum ieee802154e_payload_ie_id {
  PAYLOAD_IE_ESDU = 0,
  PAYLOAD_IE_MLME,
-  PAYLOAD_IE_SIXTOP,
+  PAYLOAD_IE_VENDOR,


I put it so that PALYLOAD_IE_IANA_IETF (should be renamed to PAYLOAD_IE_IETF) doesn't have the Group ID value for another group, which is "Vendor Specific Nested IE". PAYLOAD_IE_VENDOR is not used effectively anywhere for now.

yatch · 2016-11-23T10:24:51Z

core/net/mac/frame802154e-ie.c

+          case PAYLOAD_IE_IANA_IETF:
+            /* Now expect 'len' bytes of Sixtop sub-IEs */
+            switch(*buf) {
+            case IANA_IETF_IE_6TOP:


yatch · 2016-11-23T10:28:59Z

core/net/mac/frame802154e-ie.c

+            switch(*buf) {
+            case IANA_IETF_IE_6TOP:
+              ies->sixtop_ie_content_ptr = buf + 1;
+              ies->sixtop_ie_content_len = len - 1;


Agree; put a commen like this: /* adjust buf and len by the size of the Sub-ID field, one octet. */

yatch · 2016-11-23T10:30:53Z

core/net/mac/framer-802154.c

@@ -97,6 +97,9 @@ create_frame(int type, int do_create)
   * source nor destination address, we have dest PAN ID iff compression is *set*. */
  params.fcf.panid_compression = 0;

+  /* Set IE Present bit */
+  params.fcf.ie_list_present = packetbuf_attr(PACKETBUF_ATTR_MAC_METADATA);


I agree with you; it should go with a different PR. I can do it later.

yatch · 2016-11-23T10:32:10Z

core/net/mac/tsch/sixtop/sixtop-6p.c

@@ -0,0 +1,284 @@
+/*
+ * Copyright (c) 2014, Swedish Institute of Computer Science.


It's inherited from the original sixtop.c. I've asked @shalurajendran to handle this properly.

yatch · 2016-11-23T10:37:54Z

core/net/mac/tsch/sixtop/sixtop-6p.h

+#include "sixtop.h"
+
+int sixtop_6p_build_6top_request(uint8_t return_code, uint8_t sfid,
+                                  uint8_t seqno, const sixtop_msg_body_t *body);


yatch · 2016-11-23T10:43:44Z

core/net/mac/tsch/sixtop/sixtop.c

 /**
 * \file
- *         IEEE 802.15.4 TSCH MAC Sixtop.
+ *         6top Protocol (6P) - Core


Obviously more precise short explanation for the file is needed... Anyway, it's a part of 6P (6top Protocol).

yatch · 2016-11-23T10:47:25Z

core/net/mac/tsch/sixtop/sixtop.c

+  /* send the message with TSCH MAC */
+  packetbuf_set_addr(PACKETBUF_ADDR_RECEIVER, dest_addr);
+  packetbuf_set_addr(PACKETBUF_ADDR_SENDER, &linkaddr_node_addr);
+  tschmac_driver.send(NULL, NULL);


That's fine. It'd be nice to have a macro to check the definition of NETSTACK_MAC at the beginning of the file. If it's not the TSCH MAC, stop compiling the file with #error.

yatch · 2016-11-23T10:48:08Z

core/net/mac/tsch/sixtop/sixtop-6p.c

+  sixtop_ie->version = buf[0] & 0x0f;
+  sixtop_ie->code = (buf[0] & 0xf0) >> 4;
+  sixtop_ie->sfid = buf[1];
+  sixtop_ie->seqno = buf[2] & 0x07;


This should be 0x0f!!

yatch · 2016-11-23T10:48:09Z

core/net/mac/tsch/sixtop/sixtop-6p.c

+  sixtop_ie->code = (buf[0] & 0xf0) >> 4;
+  sixtop_ie->sfid = buf[1];
+  sixtop_ie->seqno = buf[2] & 0x07;
+  sixtop_ie->gab = (buf[2] & 0x30) >> 0x04;


Yeah, I have no idea why I used hex notation here... Thanks for pointing it out.

- untangle 6P message manipulation and the sample schedule function - sixtop.c: 6P core part, newly introduced - sixtop-6p.c : 6P message manipulation code, based on the former sixtop.c - sixtop-sf-simple.c : Sample SF code,based on the former sixtop.c - maintain a sequence number per neighbor to be used for request - rename variables and constants - comply more with draft-ietf-6tisch-6top-protocol-02

yatch · 2016-12-01T21:04:36Z

I've pushed new changes to the branch. It becomes nicer and cleaner from my point of view, and supports draft-03.

simonduq · 2016-12-02T10:33:10Z

It's so much of a change to the original PR that I wonder if the best way to move forward isn't to simply have @yatch do a new independent PR to Contiki, rebased on @shalurajendran 's commits, and referring to @shalurajendran 's PR discussion. This will make code review a lot easier. @shalurajendran what do you think?

yatch · 2016-12-06T21:42:41Z

@shalurajendran What do you think of the suggestion from @simonduq ...? By the way, I made trivial changes and did force-push a while ago.

- support draft-ietf-6tisch-6top-protocol-03 - reconsider file structure and file names - make sf-simple part of example - update sixtop/README.md - apply C-DAC copyright notice - bugfix + cleanup

simonduq · 2016-12-07T08:15:28Z

IMO you should just go ahead: as long as the original commits are left in the history it will be clear you're not stealing the contribution. The PR is no more than a means to get the commits in after all :)

shalurajendran · 2016-12-08T05:36:00Z

Good work @yatch .

Proceed with the new PR as suggested by @simonduq . Happy that our effort will be referred :)

yatch · 2016-12-08T12:39:22Z

All right; close this PR and open another on on the Contiki repo. See you there :-)

yatch mentioned this pull request Nov 9, 2016

Contiki-6top : Added 6top (6TiSCH Operation Sublayer) Implementation contiki-os/contiki#1898

Open

yatch force-pushed the pr/sixtop-refactoring branch from 7da4b52 to 225ccf2 Compare November 9, 2016 23:00

simonduq reviewed Nov 23, 2016

View reviewed changes

yatch commented Nov 23, 2016

View reviewed changes

yatch force-pushed the pr/sixtop-refactoring branch from 225ccf2 to aac72b6 Compare December 1, 2016 19:01

yatch force-pushed the pr/sixtop-refactoring branch 2 times, most recently from 7e363f6 to 4fff8a0 Compare December 1, 2016 20:56

yatch force-pushed the pr/sixtop-refactoring branch from 4fff8a0 to ea6af88 Compare December 6, 2016 21:36

yatch force-pushed the pr/sixtop-refactoring branch from ea6af88 to d920f42 Compare December 6, 2016 21:57

6top: refactoring

2dc1bc4

- support draft-ietf-6tisch-6top-protocol-03 - reconsider file structure and file names - make sf-simple part of example - update sixtop/README.md - apply C-DAC copyright notice - bugfix + cleanup

yatch force-pushed the pr/sixtop-refactoring branch from d920f42 to 2dc1bc4 Compare December 6, 2016 22:08

yatch closed this Dec 8, 2016

		@@ -0,0 +1,284 @@
		/*
		* Copyright (c) 2014, Swedish Institute of Computer Science.

Refactoring 6top Implementation #1

Refactoring 6top Implementation #1

Conversation

yatch commented Nov 9, 2016

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yatch left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yatch commented Dec 1, 2016

simonduq commented Dec 2, 2016

yatch commented Dec 6, 2016

simonduq commented Dec 7, 2016

shalurajendran commented Dec 8, 2016

yatch commented Dec 8, 2016