This patch also contains the Mac16 and Mac64 address improvements, as they are mandatory for ...
11 years, 10 months ago
(2013-07-08 21:45:35 UTC)
#1
This patch also contains the Mac16 and Mac64 address improvements, as they are
mandatory for this module.
They are also in another code review: http://codereview.appspot.com/7691046/
Please refrain to comment them here.
This new version bounds the MAC address -> IP address conversion to the specific use-case. ...
11 years, 9 months ago
(2013-07-19 06:31:38 UTC)
#7
This new version bounds the MAC address -> IP address conversion to the specific
use-case.
As is: if the "ForceEtherType" attribute is set, then the MAC addresses are
assumed to be Mac48Address (i.e., Ethernet-like). Else they could be either
Mac16 or Mac64 (802.15.4).
This fixes the problem that a Mac16 and Mac48 Addresses learnt from NS/NA are
exactly the same.
On 2013/07/25 16:24:54, Tom Henderson wrote: > In general this seems mostly there, but I'm ...
11 years, 9 months ago
(2013-07-26 11:39:00 UTC)
#9
On 2013/07/25 16:24:54, Tom Henderson wrote:
> In general this seems mostly there, but I'm concerned about the lack of test
> code, since there is a lot of code manipulating headers at a low level, and
> performing fragmentation/reassembly operations.
>
> Aside from addressing my other comments and cleanup, I think that test cases
> exercising the fragmentation and compression features need to be provided
before
> merging to ns-3-dev.
>
> Note, I did not do a detailed review of the header implementation.
Hi Tom,
thanks for the review. I'll clean up the code and I'll make the tests.
To be honest I was planning to make some tests.
As a side note, /bow. You found a potential bug. Valgrind does pass, but we
didn't have any test case or example with the communications stopping in the
middle of a fragmented packet reception.
Do you have any plans to test fragmentation and header code? https://codereview.appspot.com/10945044/diff/55001/src/sixlowpan/test/sixlowpan-model-test-suite.cc File src/sixlowpan/test/sixlowpan-model-test-suite.cc (right): ...
11 years, 9 months ago
(2013-07-28 19:53:23 UTC)
#14
On 2013/07/28 19:53:23, Tom Henderson wrote: > Do you have any plans to test fragmentation ...
11 years, 9 months ago
(2013-07-28 21:49:38 UTC)
#15
On 2013/07/28 19:53:23, Tom Henderson wrote:
> Do you have any plans to test fragmentation and header code?
Fragmentation: it is used in the example. I could modify the Ipv6's
fragmentation test if needed.
Headers: hard to test them "alone", since they're the result of other headers'
compression.
One could forge some headers and see the decompression results, but it's nearly
the same as transmitting a "normal" packet. Plus it would test only half of the
code.
I can say they're fine since they are correctly decoded by Wireshark in 802.15.4
case. Without 802.15.4, the only indirect proof is that the original headers are
back in place as they should be.
That's why I used UDP in the test. It's the one using all compression kinds.
Anyway, tomorrow I'll make the fragmentation test and another one (same as the
one already here, but with HC1 compression).
>
https://codereview.appspot.com/10945044/diff/55001/src/sixlowpan/test/sixlowp...
> File src/sixlowpan/test/sixlowpan-model-test-suite.cc (right):
>
>
https://codereview.appspot.com/10945044/diff/55001/src/sixlowpan/test/sixlowp...
> src/sixlowpan/test/sixlowpan-model-test-suite.cc:22: * be in an independent
file
> for clarity purposes.
> stale copyright, description, and author statement
>
>
https://codereview.appspot.com/10945044/diff/55001/src/sixlowpan/test/sixlowp...
> src/sixlowpan/test/sixlowpan-model-test-suite.cc:118: //
> 123, "200");
> delete commented code
Fixed, along with copyright dates, on all the files. I'll push the right code
along with the two more tests.
On 2013/07/29 22:19:56, Tommaso Pecorella wrote: I checked the fragmentation test but it's quite hard ...
11 years, 9 months ago
(2013-07-29 22:23:01 UTC)
#17
On 2013/07/29 22:19:56, Tommaso Pecorella wrote:
I checked the fragmentation test but it's quite hard to make one. If I'll be
able to make one, I'll post it.
In the meantime, the two tests cover both IPHC and HC1 compression kinds and
they do work (and Valgrind is ok with them).
On 2013/11/23 12:18:37, Tommaso Pecorella wrote: Note that some of the differences in doc/models/Makefile are ...
11 years, 5 months ago
(2013-11-23 12:25:34 UTC)
#20
On 2013/11/23 12:18:37, Tommaso Pecorella wrote:
Note that some of the differences in doc/models/Makefile are listed but are not
relevant. They are the changes imported from ns-3-dev.
The same applies to udp-header
All the tests passing and docs is complete (no warning except for tests and
examples).
T.
looks great overall. https://codereview.appspot.com/10945044/diff/71001/src/sixlowpan/model/sixlowpan-header.h File src/sixlowpan/model/sixlowpan-header.h (right): https://codereview.appspot.com/10945044/diff/71001/src/sixlowpan/model/sixlowpan-header.h#newcode47 src/sixlowpan/model/sixlowpan-header.h:47: class SixLowPanDispatch : public Header The ...
11 years, 5 months ago
(2013-11-25 07:59:13 UTC)
#21
Hi Mathieu, Thanks for the review, it's very appreciated. here are the answers to your ...
11 years, 5 months ago
(2013-11-25 11:08:25 UTC)
#22
Hi Mathieu,
Thanks for the review, it's very appreciated. here are the answers to your
questions.
About the namespace, I was thinking to remove it altogether... it doesn't seems
to be useful at all for real.
Cheers,
T.
https://codereview.appspot.com/10945044/diff/71001/src/sixlowpan/model/sixlow...
File src/sixlowpan/model/sixlowpan-header.h (right):
https://codereview.appspot.com/10945044/diff/71001/src/sixlowpan/model/sixlow...
src/sixlowpan/model/sixlowpan-header.h:47: class SixLowPanDispatch : public
Header
On 2013/11/25 07:59:14, Mathieu Lacage wrote:
> The point of that class ? You have functions you wish to reuse in multiple
> pieces of code ? Why not make the functions and type declarations public then
?
Correct analysis. It's a virtual class for all the 6LoWPAN headers. And all its
functions and enums are indeed public :)
https://codereview.appspot.com/10945044/diff/71001/src/sixlowpan/model/sixlow...
File src/sixlowpan/model/sixlowpan-net-device.cc (right):
https://codereview.appspot.com/10945044/diff/71001/src/sixlowpan/model/sixlow...
src/sixlowpan/model/sixlowpan-net-device.cc:389: if (m_useIphc)
On 2013/11/25 07:59:14, Mathieu Lacage wrote:
> Could you not use the protocolNumber variable to make this decision rather
than
> the m_useIphc variable ?
>
> (I am really curious)
Nope, IPHC and HC1 are two alternative compression mechanisms (defined in two
different RFCs).
The protocolNumber is there to "fake" the protocol number for Ethernet-like
protocols that have a protocol number on the MAC header.
802.15.4 doesn't have any protocol number in the header, so 6LoWPAN doesn't have
an official protocol number, hence the fake one.
https://codereview.appspot.com/10945044/diff/71001/src/sixlowpan/model/sixlow...
File src/sixlowpan/model/sixlowpan-net-device.h (right):
https://codereview.appspot.com/10945044/diff/71001/src/sixlowpan/model/sixlow...
src/sixlowpan/model/sixlowpan-net-device.h:65: class SixLowPanNetDevice : public
NetDevice
On 2013/11/25 07:59:14, Mathieu Lacage wrote:
> If you use a sixlowpan namespace, there is zero point in using the SixLowPan
> prefix on the NetDevice class defined here.
>
> i.e., I suspect you could write this:
>
> class NetDevice : public ns3::NetDevice
>
> I am not familiar with the details of the C++ namespace model and I have not
> tried this so, take it with a grain of salt
The sixlowpan namespace doesn't affect NetDevice, since it's a sub-namespace.
I'll try to use ns3::NetDevice, but I was thinking to remove altogether the
sixlowpan namespace. Namespaces are used in a very inconsistent way in ns-3
modules, and I guess that we could as well do some thinking about it (in the
future).
On 2013/11/25 21:21:16, Tommaso Pecorella wrote: Mathieu mad me think about the namespace and the ...
11 years, 5 months ago
(2013-11-25 21:30:21 UTC)
#24
On 2013/11/25 21:21:16, Tommaso Pecorella wrote:
Mathieu mad me think about the namespace and the purpose of SixLowPanDispatch
class.
Now the namespace is gone, and the headers are "normal" headers. The
SixLowPanDispatch class is still there only to provide conversion between "raw"
dispatch values and the correct dispatch types.
I could move the conversion functions into the SixLowPanNetDevice, but that
would imply a dependency between the header classes and the NetDevice, which I
don't like. or to have some functions returning an uint8_t instead of the proper
type, which I like even less.
If you're wondering why the conversion is necessary, the answer is simple: the
compressed header type represented by the dispatch is variable length. Hence,
you have to read a full byte and then decide what does it means.
E.g., LOWPAN_HC1 is 0x42, but LOWPAN_FRAG1 is [0xC0, 0xC7]. Damn, they use every
single bit.
Cheers,
T.
PS: as usual all the tests are positive.
Thank you all for your great effort. By the way, May I know when ns-3.19 ...
11 years, 5 months ago
(2013-12-01 12:58:20 UTC)
#29
Thank you all for your great effort.
By the way, May I know when ns-3.19 is scheduled to be released?
On Saturday, November 30, 2013 3:11:32 PM UTC+3, Tommaso Pecorella wrote:
>
> http://codereview.appspot.com/10945044/
>
On 2013/12/01 12:58:20, dr.eng.ghaleb_gmail.com wrote: > Thank you all for your great effort. > > ...
11 years, 5 months ago
(2013-12-01 15:04:12 UTC)
#30
On 2013/12/01 12:58:20, dr.eng.ghaleb_gmail.com wrote:
> Thank you all for your great effort.
>
> By the way, May I know when ns-3.19 is scheduled to be released?
>
> On Saturday, November 30, 2013 3:11:32 PM UTC+3, Tommaso Pecorella wrote:
> >
> > http://codereview.appspot.com/10945044/
> >
The release plan is here: http://www.nsnam.org/wiki/Ns-3.19
... but the website isn't working properly right now.
Cheers,
T.
Issue 10945044: 6LoWPAN module
(Closed)
Created 11 years, 10 months ago by Tommaso Pecorella
Modified 11 years, 4 months ago
Reviewers: Tom Henderson, Mathieu Lacage, dr.eng.ghaleb_gmail.com
Base URL:
Comments: 79
