6LoWPAN - multiple reassembly fix and cleanup #1258
Conversation
|
A bit of squashing on the last commits would be welcome :) |
|
Thanks for the comments! I will do a bit of squashing on the last small commits and a bit of splitting on the big one. Possibly I can also remove some of the code that is not yet used and upstream that later when we get the the code that make use of that functionality. |
|
sounds great |
|
I did some squash on the small commits and also removed some of the unused features in the multiple-reassembly fix so that it is a bit smaller to view. Note that I now also removed the HC1 compression in a separate commit. |
|
OK, except from a few comments I left directly in the commit https://github.com/joakimeriksson/contiki/commit/c76a5fe640f4246d0333353b816013d60a97ca7c, this is really excellent! (and needed :)) One thing I was wondering it why have fragments point to a I'll give this feature a try next week. |
|
Thanks for all the comments, I will address them ASAP! |
| #endif /* USE_FRAMER_HDRLEN */ | ||
| max_payload = MAC_MAX_PAYLOAD - framer_hdrlen - NETSTACK_LLSEC.get_overhead(); |
There was a problem hiding this comment.
These changes look like they might be unwanted changes due to the contents of this file being copied over a newer version of the file. Because the new code still needs to compute the llsec overhead, right?
There was a problem hiding this comment.
@adamdunkels yes, you can look at the commit where I started commenting (including the same issue ;))
https://github.com/joakimeriksson/contiki/commit/c76a5fe640f4246d0333353b816013d60a97ca7c
There was a problem hiding this comment.
Yes - this is an unwanted change - I will fix that and other things - will notify you as soon as it is fixed!
|
Now I think I have addressed the issues. The memory allocation mentioned in the comments will likely move in later. We are using that in things like the Yanzi Application Layer implementation, but since it is not used at the moment there is no need to bring it in - it is a lot of code to review anyway so let's take that later. |
|
Just tested, works nicely for me 👍 |
|
I'll try to get this tested with our Thingsquare regression tests, which has a bit different traffic patterns than the contiki-os tests and also uses a different llsec layer. |
|
@adamdunkels any news on regression tests? Do you need me to do a rebase before you test? |
|
@joakimeriksson I merged your changes into our internal test suite this morning (and hopefully fixed the merge conflicts correctly) - takes a while to run though. Will get back with the results! |
|
I ran this through our tests last weekend, and everything looked green! (Except that it increases the memory usage, which happened to break one test, but that should be possible to fix by adjusting the number of fragmentation buffers.) |
|
Thanks @adamdunkels for all the efforts to tes this! Is there anything I can do to get this in? We have a few more PRs in the pipeline that somewhat depend on this one. |
|
+1 |
|
👍 needs fixing tho |
|
Yes, a rebase is needed - will do that and fix any conflicts. |
| @@ -327,7 +500,7 @@ static struct sicslowpan_addr_context *context; | |||
| /** pointer to the byte where to write next inline field. */ | |||
| static uint8_t *hc06_ptr; | |||
|
|
|||
| /* Uncompression of linklocal */ | |||
| /* ession of linklocal */ | |||
|
Finally got around to do a rebase of this. Now It should be back in a "pullable" state. I also fixed some of the recent comments (typo in comments). |
|
I think this is ready to be merged, @adamdunkels did you finally get all your tests green? |
|
@simonduq Yep, the tests did work! @joakimeriksson One problem though was that this code used more memory than before so some builds broke. Is it easy to tweak the memory footprint? I guess |
|
@adamdunkels yes setting down to 1 will still be better than the previous implementation since it will not drop the current reassembly if there is a non-fragmented packet coming in. Number of buffers can also be low - a few (3) - is an ok value if fragmentation is active. Do you have a specific platform or scenario/application context in mind when it needs to be very low? |
|
Ok, good to know! Our problem is just that we are running low on RAM on several platforms, such as the cc2650, that have tight RAM to begin with. The CC2538 is also a bit tight because of the 16k limite of the retention RAM, but the reassembly buffers can be placed in non-retention RAM to solve it. Speaking of footprint, the flash on the sensortag is also really tight, and our build with these changes goes just above the mark with some 100 bytes. Is there anything that be tweaked to reduce the flash footprint, or is it better to just bite the bullet and optimize something else? |
|
There might be some bytes of flash that we could get the code size down, but not sure how easy it is to get 100 bytes downsizing done. |
|
@adamdunkels I think I found one thing. The rime_sniffer code - if we make that configurable e.g. #SICSLOWPAN_CONF_RIME_SNIFFER_ENABLED or something like that I do think you can save more than 100 bytes and still have fragmentation working. But not in combination with power-tracing. Would that be a solution? |
|
👍 |
6LoWPAN - multiple reassembly fix and cleanup
This is a fix that make the 6lowpan layer in Contiki handle multiple simultaneous incoming packets. Before this fix I had around thousand lost packets per 24 hours in my 40 nodes 6lowpan network (with RPL) due to 6lowpan fragmentation drops. Either due to two fragmented packets being handled (and one dropped) or one fragmentet packet being handled while another single-fragment packet cause the other to be dropped. After this fix there is no need to drop the old (or the new) packet. There are configuration both for the number of simultaneous packets to handle and total number of fragment buffers to store. Defaults aim at handling two packets and around 1280 bytes in total.
There are also some cleanups made in both comments and code. Deprecated compression methods have been removed (old HC1 which is no longer expected to be used was removed).
Issues: there seems to be a problem when compiling for 8051. If anyone that happens to know the 8051 compiler well could help me figure that one out I would be very happy - It is now fixed - but the code will
likely not compile on 8051 with fragmentation support.
Documentation is available and will be added to the wiki as soon as the fix is pulled.
This work has been performed as a part of an open source IoT edge project and is contributed by Yanzi Networks AB and SICS Swedish ICT. Intel Corporation is a sponsor of the open source IoT edge.