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
ng_sixlowpan: Initial import of datagram fragmentation #2781
Conversation
0bd16b7
to
d17b108
Compare
f1c9481
to
07bbf7c
Compare
Rebased to current #2781. |
07bbf7c
to
19c90dc
Compare
Rebased to current #2614 |
19c90dc
to
c404f4e
Compare
Rebased to current #2614 |
c404f4e
to
d0236df
Compare
Rebased to current #2614 and squashed |
d0236df
to
5be6bfd
Compare
Rebased to current master |
#define NG_SIXLOWPAN_FRAG_1_DISP (0xc0) /**< dispatch for 1st fragment */ | ||
#define NG_SIXLOWPAN_FRAG_N_DISP (0xe0) /**< dispatch for subsequent | ||
* fragments */ | ||
#define NG_SIXLOWPAN_FRAG_SIZE_MASK (0x07ff) /**< mask for datagram size */ |
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.
Matter of taste, but I find upper case letters for hex values better readable.
/* move already inserted fragments 1 to the right */ | ||
for (int i = pkt->size - 1; i > 0; i--) { | ||
uint8_t *d = ((uint8_t *)(pkt->data)) + i; | ||
*d = *(d - 1); |
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.
memmove()
?
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.
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.
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.
Could be interesting to do some benchmarking, but again out of scope of this PR.
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.
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).
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.
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.
That you were completely right and we should avoid memmove()
.
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.
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.
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.
Which PR?
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.
Addressed comments |
rbuf[i].pkt->size, rbuf[i].tag); | ||
|
||
ng_pktbuf_release(rbuf[i].pkt); | ||
_rbuf_rem(rbuf + i); |
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.
I still think that &(rbuf[i])
is more readable, but I won't NACK.
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.
Fixed
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.) |
That's an ACK. |
5ed282f
to
bbed05e
Compare
Fixed the typo and squashed |
There were documentation errors. Since I don't trust my spelling skills, I decided against immediate squashing. |
looks good, go squashing again. |
2a2bdb6
to
73d9739
Compare
Squashed again |
…tial ng_sixlowpan: Initial import of datagram fragmentation
This adds (optional) 6LoWPAN fragmentation to
ng_sixlowpan
.Depends on #2614(merged)