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

Issue 96130043: LEAR extension of DSR module

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 6 months ago by tomasz.seweryn7
Modified:
3 years, 10 months ago
Reviewers:
Tom Henderson
CC:
ns-3-reviews_googlegroups.com, yfcheng_ittc.ku.edu
Visibility:
Public.

Description

LEAR extension of DSR module

Patch Set 1 #

Total comments: 26

Patch Set 2 : "LEAR extension partial update" #

Total comments: 7
Unified diffs Side-by-side diffs Delta from patch set Stats (+3834 lines, -92 lines) Patch
M src/dsr/doc/dsr.rst View 1 2 chunks +24 lines, -0 lines 2 comments Download
M src/dsr/examples/dsr.cc View 1 1 chunk +0 lines, -1 line 0 comments Download
A src/dsr/examples/dsr-lear.cc View 1 1 chunk +388 lines, -0 lines 0 comments Download
M src/dsr/examples/wscript View 1 chunk +1 line, -1 line 0 comments Download
M src/dsr/model/dsr-option-header.h View 1 5 chunks +597 lines, -3 lines 1 comment Download
M src/dsr/model/dsr-option-header.cc View 1 12 chunks +582 lines, -10 lines 1 comment Download
M src/dsr/model/dsr-options.h View 1 12 chunks +340 lines, -23 lines 0 comments Download
M src/dsr/model/dsr-options.cc View 1 24 chunks +1403 lines, -29 lines 0 comments Download
M src/dsr/model/dsr-routing.h View 8 chunks +110 lines, -10 lines 1 comment Download
M src/dsr/model/dsr-routing.cc View 1 15 chunks +208 lines, -14 lines 1 comment Download
M src/dsr/test/dsr-test-suite.cc View 3 chunks +180 lines, -0 lines 0 comments Download
M src/dsr/wscript View 1 1 chunk +1 line, -1 line 1 comment Download

Messages

Total messages: 5
tomasz.seweryn7
4 years, 6 months ago (2014-05-08 09:00:30 UTC) #1
Tom Henderson
I found this to be generally well written and conformant to ns-3 styles and conventions; ...
4 years, 2 months ago (2014-08-31 23:55:56 UTC) #2
tomasz.seweryn7
On 2014/08/31 23:55:56, Tom Henderson wrote: > I found this to be generally well written ...
4 years, 2 months ago (2014-09-04 20:12:17 UTC) #3
Tom Henderson
some additional comments https://codereview.appspot.com/96130043/diff/40001/src/dsr/doc/dsr.rst File src/dsr/doc/dsr.rst (right): https://codereview.appspot.com/96130043/diff/40001/src/dsr/doc/dsr.rst#newcode211 src/dsr/doc/dsr.rst:211: *CANCEL_RCACHE - Cancel Route Cache please ...
4 years ago (2014-11-17 22:51:44 UTC) #4
Tom Henderson
4 years ago (2014-11-18 00:17:05 UTC) #5
On 2014/09/04 20:12:17, tomasz.seweryn7 wrote:
> On 2014/08/31 23:55:56, Tom Henderson wrote:
> > I found this to be generally well written and conformant to ns-3 styles and
> > conventions; I had mostly minor comments in this review.  I did not compile
> and
> > run the code but just reviewed it within this tool.
> > 
> > The DSR model has an existing model library chapter, but this patch doesn't
> > propose to extend it.  I think that additional documentation in the
> doc/dsr.rst
> > file should be added before merging these extensions.  In particular, some
> > treatment on how users can use LEAR and get interesting data out of the
model
> > about LEAR's operation, any caveats or limitations of the model, and what
> > aspects of the code have been tested.
> 
> I will update the *.rst file definitely. It will explain everything.

I added a couple more comments to that extension, but thanks for getting it
started.

> 
> > 
> > The tests provided seem to just focus on header operations, and it wasn't
> clear
> > how much of the basic operation of the logic of the protocol is being
> exercised
> > (such as all of the code proposed for DsrOptionRreq::Process()).  
> 
> This is true. Tests in DSR itself are not complex. I didn't focus on testing
> single methods, but rather analysed output of simulations. I will think about
> adding some tests.

This is still an open issue.  I'd suggest:

1) what kind of validation tests (mentioned in the documentation) could be run
to confirm that the basic operation is fine?  This might be on a small scale
network (e.g. figure 2 of the LEAR paper) and inspecting the state of the route
cache or messages exchanged at specific points in time.

2) if there are large scale system tests such as dsr-lear.cc, can some checking
of overall system performance be done?  For instance, that the overall network
throughput was above a certain threshold?  

Presently, the dsr.cc example with default parameters takes a very long time to
run, and writes a large file.  If necessary, we can add a test under the
"TAKES_FOREVER" duration category, but we should still have at least one shorter
ones that could be run quickly and that didn't involve writing a file (although
I'm not sure whether Yufei coded this to require the file for his cross-layer
signaling).

> 
> > 
> > The patch to the example file changes the behavior with respect to the
> operation
> > when LEAR is not enabled (changing default values, adding an energy model
> > unconditionally to the nodes).  Is the existing behavior, that the example
was
> > intended to illustrate, preserved?   Yufei may want to review this aspect. 
> Does
> > the example allow users to toggle the usage of LEAR and observe differences
to
> > the network lifetime?
> 
> Changes in the code do add energy source installation unconditionally. There
is
> also files creation with energy levels of nodes after simulation finishes.
This
> for sure preserves the original behavior of DSR. Some of the default values
were
> modified to make situations in the simulation specific for LEAR (when this
> extension gives benefit over DSR) to occur (lower node's transmitting range
> making nodes loose routes more often). In the end, user may run the simulation
> with "lear=true" or "lear=false" and compare "dsr-remainingEnergy*" file with
> "dsr-lear-remainingEnergy*". Distribution of the nodes energy in LEAR case
> should be more uniform than in DSR (avoiding the existence of much more
> exhausted nodes with lower than the average level of energy)

OK, this should be documented in comments in the example, perhaps also in the
.rst file.  Please tell future readers what they should expect to see and
understand from the output produced by the example under different
configurations.

I suggest to just combine dsr.cc and dsr-lear.cc.
> 
> > 
> > In addition, is there any canonical example such as showing how the
> energy-aware
> > DSR nodes choose a different path than default based on a reduced
willingness
> to
> > forward by energy-depleted nodes, allowing longer network operation?  Or a
> case
> > in which all of the candidate paths have a node below the energy threshold,
> > causing the route request to be reissued with increased sequence number?
> 
> I hope the modified example already shows by the output files, that LEAR makes
> the energy of the nodes more uniformly distributed and avoids exhaustion. One
> can only see it after running LEAR with some configuration and then DSR with
the
> same and comparing output files of both simulations (average level of the
> energy, lowest, highest, standard deviation). The lowest standard deviation,
the
> more uniformly distributed the energy between all the nodes of simulation.
> 
> https://codereview.appspot.com/96130043/diff/1/src/dsr/examples/dsr.cc
> File src/dsr/examples/dsr.cc (right):
> 
>
https://codereview.appspot.com/96130043/diff/1/src/dsr/examples/dsr.cc#newcod...
> src/dsr/examples/dsr.cc:223: cmd.AddValue ("dParam", "\"d\" parameter of LEAR
> algorithm, Default: 0.1", dParam);
> You're right. I will correct my variables and DSR specific too.
> 
> On 2014/08/31 23:55:55, Tom Henderson wrote:
> > In general, one should not encode the default values in the help strings. 
> This
> > can be fragile and these strings are often not aligned with code changes. 
> ns-3
> > already has a built-in mechanism to display default values when the help
> strings
> > are printed out.  To see this, try running the third tutorial program such
as
> > "./waf --run "third --help"" (or this program with the --help option).
> 
>
https://codereview.appspot.com/96130043/diff/1/src/dsr/examples/dsr.cc#newcod...
> src/dsr/examples/dsr.cc:226: SeedManager::SetSeed (877);
> I was taught at studies, that prime numbers should be given as seeds for
pseudo
> random generators to get better distribution. The exact number was chosen by
me
> unintentionally.

Please add the comment though that a fixed seed with arbitrary seed number is
used to control the default randomness.  

> 
> On 2014/08/31 23:55:55, Tom Henderson wrote:
> > If this example depends on a special value for seed, perhaps add a comment
as
> to
> > why this specific value was chosen (and what may happen if the seed is
> altered).
> 
>
https://codereview.appspot.com/96130043/diff/1/src/dsr/examples/dsr.cc#newcod...
> src/dsr/examples/dsr.cc:349: /*std::ostringstream o2;
> Together with bound methods "AppRxCntr","AppTxCntr" this code is not necessary
> here. you're right. I used it for testing. 
> On 2014/08/31 23:55:55, Tom Henderson wrote:
> > this is all commented out, should it all be deleted?
> 
>
https://codereview.appspot.com/96130043/diff/1/src/dsr/model/dsr-option-header.h
> File src/dsr/model/dsr-option-header.h (right):
> 
>
https://codereview.appspot.com/96130043/diff/1/src/dsr/model/dsr-option-heade...
> src/dsr/model/dsr-option-header.h:1418: * \brief Set a Node IPv4 Address.
> This definition is confusing. You're right. I copied it from RREQ header
> definition. I will change it everywhere to "Set a Node IPv4 Address in the
> packet".
> 
> On 2014/08/31 23:55:55, Tom Henderson wrote:
> > This method does not actually set a node's IP address, it just encodes an
> > address in the header.  Please clarify this in the doxygen.
> 
> https://codereview.appspot.com/96130043/diff/1/src/dsr/model/dsr-options.cc
> File src/dsr/model/dsr-options.cc (right):
> 
>
https://codereview.appspot.com/96130043/diff/1/src/dsr/model/dsr-options.cc#n...
> src/dsr/model/dsr-options.cc:958: else    // LEAR version of DSR is active, so
> execute this part of code
> DSR processes RREQ in the same way as DSR-LEAR only in case when RREQ reaches
> target node (122 repeated lines of code). Apart from that, main "if-else"
blocks
> remain the same but decisions in DSR-LEAR are made upon remaining energy. I
> thought, that it would be confusing for the viewer to see many times division
> between DSR and DSR-LEAR in a form of "if (!m_isLearActive){ ... } else { ...
}"
> blocks, so I decided to make only one such block. Should I refactor the code
to
> avoid repetition anyway?

I'll relax this comment since the original code has the issue.  I think that,
yes, in general, functions shouldn't be so long, but to make major changes now
may destabilize things.  At least, I think refactoring could be done as a later
step by someone interested in this.


> On 2014/08/31 23:55:56, Tom Henderson wrote:
> > do you want to enforce this condition, or just state instead that the model
> only
> > uses the first energy source, if more than one are present?
> 
>
https://codereview.appspot.com/96130043/diff/1/src/dsr/model/dsr-options.cc#n...
> src/dsr/model/dsr-options.cc:1079: //   }
> I commented it after Yufei updated his part of code the same way (lines
> 713-718). I can delete it or leave, in case it turns out to be needed.

Let's plan to do a bit of cleanup on this after LEAR is merged, removing dead
code in both the lear and non-lear branches, and also clean up some of the other
nits found.  To do so now would complicate the possible merge of this feature.
Sign in to reply to this message.

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