Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(35)

Issue 109450044: NS3 - CSMA Backoff Counter Correction

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 1 month ago by Mehdi Moussouni
Modified:
3 years, 10 months ago
Reviewers:
Tom Henderson, n.p
CC:
ns-3-reviews_googlegroups.com
Base URL:
file:///home/greti/Mehdi/svn/
Visibility:
Public.

Description

Network Simulator 3 This is an implementation of the contention window counter depending on the line activity. Mehdi Moussouni (GRETI) Groupe de Recherche En Technologie Avancées de l'Internet Advanced Internet Technology Research Group

Patch Set 1 : First modification #

Total comments: 2

Patch Set 2 : Clarification of Minimal max slot #

Total comments: 7
Unified diffs Side-by-side diffs Delta from patch set Stats (+62 lines, -20 lines) Patch
M backoff.h View 3 chunks +8 lines, -2 lines 2 comments Download
M backoff.cc View 1 4 chunks +21 lines, -12 lines 3 comments Download
M csma-net-device.cc View 3 chunks +33 lines, -6 lines 2 comments Download

Messages

Total messages: 4
Tom Henderson
Looks good, but how can we test it? I'm thinking about creating a TestCsmaChannel class ...
4 years, 1 month ago (2014-07-03 13:58:14 UTC) #1
Mehdi Moussouni
On 2014/07/03 13:58:14, Tom Henderson wrote: > Looks good, but how can we test it? ...
4 years, 1 month ago (2014-07-03 17:18:55 UTC) #2
Mehdi Moussouni
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 ...
4 years, 1 month ago (2014-07-03 17:19:51 UTC) #3
n.p
3 years, 10 months ago (2014-10-01 09:03:11 UTC) #4
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#newcod...
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b