ng_sixlowpan: Initial import of datagram fragmentation by miri64 · Pull Request #2781 · RIOT-OS/RIOT

miri64 · 2015-04-10T05:24:11Z

This adds (optional) 6LoWPAN fragmentation to ng_sixlowpan.

~~Depends on #2614~~ (merged)

miri64 · 2015-04-13T23:08:35Z

Rebased to current #2781.

miri64 · 2015-04-16T16:38:41Z

Rebased to current #2614

miri64 · 2015-04-21T16:10:43Z

Rebased to current #2614

miri64 · 2015-04-22T23:18:10Z

Rebased to current #2614 and squashed

miri64 · 2015-04-23T14:42:16Z

Rebased to current master

OlegHahm · 2015-04-23T18:54:33Z

sys/include/net/ng_sixlowpan/frag.h

Matter of taste, but I find upper case letters for hex values better readable.

OlegHahm · 2015-04-24T08:02:04Z

sys/net/network_layer/ng_sixlowpan/frag/rbuf.c

memmove()?

Takes a lot of stack space, if I understand its doc correctly. From memmove(3)

DESCRIPTION The memmove() function copies n bytes from memory area src to memory area dest. The memory areas may overlap: copy‐ ing takes place as though the bytes in src are first copied into a temporary array that does not overlap src or dest, and the bytes are then copied from the temporary array to dest.

Could be interesting to do some benchmarking, but again out of scope of this PR.

I ran a benchmark on native, iot-lab_M3, and SAMR21-xpro. Results were quite surprising:
Memory consumption is the same for both solutions on all platforms. But libc memmove more than 20 times faster on native, but significantly slower on both ARM platforms (+25% on SAMR21 and +500% on iot-LAB_M3).

So what is your conclusion?

That you were completely right and we should avoid memmove().

That was not my statement. For more complex memory movement or even just for more than one byte we need to reevaluate this.

That was not my statement. For more complex memory movement or even just for more than one byte we need to reevaluate this.

Yes, and we might even want to change the libc implementation at some point, but since we don't have a generic `memmove()` replacement and here and in @jremmert-phytec-iot's PR the proposed alternative seems more efficient, the solution is fine.

I was referring to #2756 (diff)

miri64 · 2015-04-24T09:42:23Z

Addressed comments

OlegHahm · 2015-04-24T15:01:11Z

sys/net/network_layer/ng_sixlowpan/frag/rbuf.c

I still think that &(rbuf[i]) is more readable, but I won't NACK.

OlegHahm · 2015-04-25T18:40:03Z

All "mandatory" comments are addressed (except for the typo). Ready to squash and merge after Travis is fine. I will open a bunch of follow-up issues. (Please remind me to do so.)

OlegHahm · 2015-04-25T18:40:17Z

That's an ACK.

miri64 · 2015-04-25T20:03:25Z

Fixed the typo and squashed

miri64 · 2015-04-25T21:12:44Z

There were documentation errors. Since I don't trust my spelling skills, I decided against immediate squashing.

OlegHahm · 2015-04-26T00:12:50Z

looks good, go squashing again.

miri64 · 2015-04-26T00:21:38Z

Squashed again

…tial ng_sixlowpan: Initial import of datagram fragmentation

miri64 added Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT NSTF labels Apr 10, 2015

miri64 assigned OlegHahm Apr 10, 2015

miri64 added this to the Network Stack Task Force milestone Apr 10, 2015

miri64 mentioned this pull request Apr 10, 2015

Network stack refactoring: TODO list #2278

Closed

36 tasks

miri64 force-pushed the ng_sixlowpan_frag/feat/initial branch from 0bd16b7 to d17b108 Compare April 10, 2015 06:00

miri64 mentioned this pull request Apr 10, 2015

ng_sixlowpan: initial import #2614

Merged

OlegHahm added the State: waiting for other PR State: The PR requires another PR to be merged first label Apr 10, 2015

miri64 force-pushed the ng_sixlowpan_frag/feat/initial branch from f1c9481 to 07bbf7c Compare April 13, 2015 23:08

miri64 force-pushed the ng_sixlowpan_frag/feat/initial branch from 07bbf7c to 19c90dc Compare April 16, 2015 16:38

miri64 force-pushed the ng_sixlowpan_frag/feat/initial branch from 19c90dc to c404f4e Compare April 21, 2015 16:10

miri64 force-pushed the ng_sixlowpan_frag/feat/initial branch from c404f4e to d0236df Compare April 22, 2015 23:17

miri64 force-pushed the ng_sixlowpan_frag/feat/initial branch from d0236df to 5be6bfd Compare April 23, 2015 14:33

miri64 removed the State: waiting for other PR State: The PR requires another PR to be merged first label Apr 23, 2015

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

OlegHahm reviewed Apr 23, 2015
View reviewed changes

sys/include/net/ng_sixlowpan/frag.h

Copy link
Copy Markdown

Member

OlegHahm Apr 23, 2015

Choose a reason for hiding this comment

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

Matter of taste, but I find upper case letters for hex values better readable.

OlegHahm reviewed Apr 24, 2015
View reviewed changes

miri64 force-pushed the ng_sixlowpan_frag/feat/initial branch from 5ed282f to bbed05e Compare April 25, 2015 20:02

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

ng_sixlowpan: Initial import of datagram fragmentation

73d9739

miri64 force-pushed the ng_sixlowpan_frag/feat/initial branch from 2a2bdb6 to 73d9739 Compare April 26, 2015 00:21

miri64 added a commit that referenced this pull request Apr 26, 2015

Merge pull request #2781 from authmillenon/ng_sixlowpan_frag/feat/ini…

8a5e0d7

…tial ng_sixlowpan: Initial import of datagram fragmentation

miri64 merged commit 8a5e0d7 into RIOT-OS:master Apr 26, 2015

miri64 deleted the ng_sixlowpan_frag/feat/initial branch April 26, 2015 01:20

This was referenced May 16, 2015

ng_sixlowpan: make lookup in reassembly buffer more efficient #3006

Closed

ng_sixlowpan: release sent fragment snippets #3007

Closed

Conversation

miri64 commented Apr 10, 2015

Uh oh!

miri64 commented Apr 13, 2015

Uh oh!

miri64 commented Apr 16, 2015

Uh oh!

miri64 commented Apr 21, 2015

Uh oh!

miri64 commented Apr 22, 2015

Uh oh!

miri64 commented Apr 23, 2015

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 May 7, 2015 via email

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 May 8, 2015 via email

Choose a reason for hiding this comment

Uh oh!

OlegHahm May 8, 2015 via email

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OlegHahm May 8, 2015 via email

Choose a reason for hiding this comment

Uh oh!

miri64 commented Apr 24, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OlegHahm commented Apr 25, 2015

Uh oh!

OlegHahm commented Apr 25, 2015

Uh oh!

miri64 commented Apr 25, 2015

Uh oh!

miri64 commented Apr 25, 2015

Uh oh!

OlegHahm commented Apr 26, 2015

Uh oh!

miri64 commented Apr 26, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants