Code review - Issue 109450044: NS3 - CSMA Backoff Counter Correctionhttps://codereview.appspot.com/2014-10-01T09:03:11+00:00rietveld
Message from unknown
2014-07-03T13:10:50+00:00Mehdi Moussouniurn:md5:1b01f859244ad07b2ef8e5ae30b7a230
Message from tomh.org@gmail.com
2014-07-03T13:58:14+00:00Tom Hendersonurn:md5:4f18f47fb3acefb4cc335056d1b66781
Looks good, but how can we test it?
I'm thinking about creating a TestCsmaChannel class for which the user can explicitly force the state of the channel, then create a CsmaNetDevice, hook the test channel to it, hook some trace sinks to it to capture e.g. drop events, and then create sequences of events (forcing channel to BUSY or IDLE as needed) that validate that the backoff algorithm is behaving as expected.
https://codereview.appspot.com/109450044/diff/1/backoff.cc
File backoff.cc (right):
https://codereview.appspot.com/109450044/diff/1/backoff.cc#newcode66
backoff.cc:66: // This 4 stands for the minimal maxSlots.
Should you declare a static const uint16_t MINIMAL_MAX_SLOTS = 4; above instead of adding the explicit value and comment here? Should m_maxSlots be tested that it is above this value when the value is set?
Message from unknown
2014-07-03T17:17:11+00:00Mehdi Moussouniurn:md5:546e17913a11489561362d1793d8dfba
Message from mmoussouni.public@gmail.com
2014-07-03T17:18:55+00:00Mehdi Moussouniurn:md5:72e82674675dc33d79a757230a534189
On 2014/07/03 13:58:14, Tom Henderson wrote:
> Looks good, but how can we test it?
>
> I'm thinking about creating a TestCsmaChannel class for which the user can
> explicitly force the state of the channel, then create a CsmaNetDevice, hook the
> test channel to it, hook some trace sinks to it to capture e.g. drop events, and
> then create sequences of events (forcing channel to BUSY or IDLE as needed) that
> validate that the backoff algorithm is behaving as expected.
>
> https://codereview.appspot.com/109450044/diff/1/backoff.cc
> File backoff.cc (right):
>
> https://codereview.appspot.com/109450044/diff/1/backoff.cc#newcode66
> backoff.cc:66: // This 4 stands for the minimal maxSlots.
> Should you declare a static const uint16_t MINIMAL_MAX_SLOTS = 4; above instead
> of adding the explicit value and comment here? Should m_maxSlots be tested that
> it is above this value when the value is set?
I'm gonna work on the test part but i have to admit that i don't have a precise idea to how to do it...
Message from mmoussouni.public@gmail.com
2014-07-03T17:19:51+00:00Mehdi Moussouniurn:md5:5142b3f3ed6fdc9c7bfe8e5a7d83a785
Minimal max slot explicited
https://codereview.appspot.com/109450044/diff/1/backoff.cc
File backoff.cc (right):
https://codereview.appspot.com/109450044/diff/1/backoff.cc#newcode66
backoff.cc:66: // This 4 stands for the minimal maxSlots.
On 2014/07/03 13:58:14, Tom Henderson wrote:
> Should you declare a static const uint16_t MINIMAL_MAX_SLOTS = 4; above instead
> of adding the explicit value and comment here? Should m_maxSlots be tested that
> it is above this value when the value is set?
Done.
Message from natale.patriciello@gmail.com
2014-10-01T09:03:11+00:00n.purn:md5:167fd746fffdd166713d733b7311772c
Stilistic review, and some in-depth question about testing and API changes
https://codereview.appspot.com/109450044/diff/40001/backoff.cc
File backoff.cc (right):
https://codereview.appspot.com/109450044/diff/40001/backoff.cc#newcode34
backoff.cc:34: m_ceiling = 6;
Why are these changed?
Also, do you think that these values could be upgraded to Attributes of the class?
https://codereview.appspot.com/109450044/diff/40001/backoff.cc#newcode52
backoff.cc:52: Backoff::GetMinBackoffTime (void)
See the comment on API change in the header file
https://codereview.appspot.com/109450044/diff/40001/backoff.cc#newcode68
backoff.cc:68: ceiling = 1;
but if you wrote it, it can happen.. and if it happens, what is the result?
https://codereview.appspot.com/109450044/diff/40001/backoff.h
File backoff.h (right):
https://codereview.appspot.com/109450044/diff/40001/backoff.h#newcode20
backoff.h:20: * Derived from the p2p net device file
We have mercurial history for these thing, so no need to add yourself as co-author
https://codereview.appspot.com/109450044/diff/40001/backoff.h#newcode77
backoff.h:77: Time GetMinBackoffTime ();
Changing a public API is sometimes undesiderable. You should check all code which uses this API in the existing mainline, and tell the users that their scripts will not be compatible with this version. Can the old API be maintained, and GetMinBackoffTime added?
Possibly, meanings of the two APIs should be different.
https://codereview.appspot.com/109450044/diff/40001/csma-net-device.cc
File csma-net-device.cc (right):
https://codereview.appspot.com/109450044/diff/40001/csma-net-device.cc#newcode19
csma-net-device.cc:19: * Modif: Mehdi Moussouni (GRETI) <mehdi.moussouni@grenoble-inp.org>
See comment on mercurial
https://codereview.appspot.com/109450044/diff/40001/csma-net-device.cc#newcode497
csma-net-device.cc:497: }
I'd like to see a regression test for this.
In other words, a test which fail with previous version of the csma-channel, but which runs fine with yours.