LEAR extension of DSR module

LEAR extension of DSR module

[Tom Henderson, 2014-08-31 23:55:56]

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.

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()).  

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?

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?

https://codereview.appspot.com/96130043/diff/1/src/dsr/examples/dsr.cc
File src/dsr/examples/dsr.cc (left):

https://codereview.appspot.com/96130043/diff/1/src/dsr/examples/dsr.cc#oldcode30
src/dsr/examples/dsr.cc:30: */
perhaps add a comment as to what this example does, and what the effect on
output is if the 'lear' option is set to true?

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);
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);
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;
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 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
the existing DsrOptionRreq::Process() method is about 500 lines long.  This adds
several hundred more lines of code to the method.
I have not looked at it carefully but it makes me wonder whether there is code
duplication and whether it ought to be factored somehow into smaller methods,
for improved readability and testability.

https://codereview.appspot.com/96130043/diff/1/src/dsr/model/dsr-options.cc#n...
src/dsr/model/dsr-options.cc:977: // every node should have only one energy
source in the energy source container
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: //   }
should the above be removed (dead code)?

https://codereview.appspot.com/96130043/diff/1/src/dsr/model/dsr-options.cc#n...
src/dsr/model/dsr-options.cc:2424: NS_LOG_FUNCTION_NOARGS ();
Prefer "NS_LOG_FUNCTION (this)" to "NS_LOG_FUNCTION_NOARGS ()", unless it is a
static method.  The latter should only be used for static methods.

https://codereview.appspot.com/96130043/diff/1/src/dsr/model/dsr-options.cc#n...
src/dsr/model/dsr-options.cc:2561: //   }
dead code to remove?

https://codereview.appspot.com/96130043/diff/1/src/dsr/model/dsr-options.h
File src/dsr/model/dsr-options.h (right):

https://codereview.appspot.com/96130043/diff/1/src/dsr/model/dsr-options.h#ne...
src/dsr/model/dsr-options.h:567: DsrOptionDropRReq ();
missing some doxygen on non-virtual methods?

https://codereview.appspot.com/96130043/diff/1/src/dsr/model/dsr-routing.cc
File src/dsr/model/dsr-routing.cc (right):

https://codereview.appspot.com/96130043/diff/1/src/dsr/model/dsr-routing.cc#n...
src/dsr/model/dsr-routing.cc:288: .AddAttribute ("ThresholdJ","Threshold
algorithm parameter should be lower than initial energy of every node.",
should it say that this is only applicable when lear is active?

https://codereview.appspot.com/96130043/diff/1/src/dsr/model/dsr-routing.cc#n...
src/dsr/model/dsr-routing.cc:292: .AddAttribute ("DParamJ","\"d\" parameter of
algorithm used to adjust threshold parameter during simulation.",
same question as previous attribute

https://codereview.appspot.com/96130043/diff/1/src/dsr/model/dsr-routing.cc#n...
src/dsr/model/dsr-routing.cc:3249: //m_downTarget (packet, source, nextHop,
GetProtocolNumber (), route);
delete dead code?

[tomasz.seweryn7, 2014-09-04 20:12:17]

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.

> 
> 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.

> 
> 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)

> 
> 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.

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?

On 2014/08/31 23:55:56, Tom Henderson wrote:
> the existing DsrOptionRreq::Process() method is about 500 lines long.  This
adds
> several hundred more lines of code to the method.
> I have not looked at it carefully but it makes me wonder whether there is code
> duplication and whether it ought to be factored somehow into smaller methods,
> for improved readability and testability.

https://codereview.appspot.com/96130043/diff/1/src/dsr/model/dsr-options.cc#n...
src/dsr/model/dsr-options.cc:977: // every node should have only one energy
source in the energy source container
It was written before fixing the bug related to energy sources (Bug 1873). I
improperly thought, that only one energy source can be installed on the node. I
will fix this here and in other parts of the code, to sum the energy from all of
possibly added energy sources to the energy source container.

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.

On 2014/08/31 23:55:56, Tom Henderson wrote:
> should the above be removed (dead code)?

https://codereview.appspot.com/96130043/diff/1/src/dsr/model/dsr-options.cc#n...
src/dsr/model/dsr-options.cc:2424: NS_LOG_FUNCTION_NOARGS ();
I will correct it in LEAR-specific options and DSR ones too.

On 2014/08/31 23:55:55, Tom Henderson wrote:
> Prefer "NS_LOG_FUNCTION (this)" to "NS_LOG_FUNCTION_NOARGS ()", unless it is a
> static method.  The latter should only be used for static methods.

https://codereview.appspot.com/96130043/diff/1/src/dsr/model/dsr-options.cc#n...
src/dsr/model/dsr-options.cc:2561: //   }
Same situation as in previous occurrence. I think Yufei should decide what to do
with it.

On 2014/08/31 23:55:55, Tom Henderson wrote:
> dead code to remove?

https://codereview.appspot.com/96130043/diff/1/src/dsr/model/dsr-options.h
File src/dsr/model/dsr-options.h (right):

https://codereview.appspot.com/96130043/diff/1/src/dsr/model/dsr-options.h#ne...
src/dsr/model/dsr-options.h:567: DsrOptionDropRReq ();
I will correct it for my definitions and for the rest of options.

On 2014/08/31 23:55:56, Tom Henderson wrote:
> missing some doxygen on non-virtual methods?

https://codereview.appspot.com/96130043/diff/1/src/dsr/model/dsr-routing.cc
File src/dsr/model/dsr-routing.cc (right):

https://codereview.appspot.com/96130043/diff/1/src/dsr/model/dsr-routing.cc#n...
src/dsr/model/dsr-routing.cc:288: .AddAttribute ("ThresholdJ","Threshold
algorithm parameter should be lower than initial energy of every node.",
It is only related to LEAR. True.

On 2014/08/31 23:55:56, Tom Henderson wrote:
> should it say that this is only applicable when lear is active?

https://codereview.appspot.com/96130043/diff/1/src/dsr/model/dsr-routing.cc#n...
src/dsr/model/dsr-routing.cc:3249: //m_downTarget (packet, source, nextHop,
GetProtocolNumber (), route);
Also updated it after Yufei's update. Probably, it will be deleted.

On 2014/08/31 23:55:56, Tom Henderson wrote:
> delete dead code?

[Tom Henderson, 2014-11-17 22:51:44]

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#newcod...
src/dsr/doc/dsr.rst:211: *CANCEL_RCACHE - Cancel Route Cache
please clarify, are these routing options related to the energy awareness of
LEAR, or orthogonal?

https://codereview.appspot.com/96130043/diff/40001/src/dsr/doc/dsr.rst#newcod...
src/dsr/doc/dsr.rst:222: Simulation cases similar to those presented in [4] have
been tested and have comparable results.
are these test cases available, can they be included in the examples directory?

https://codereview.appspot.com/96130043/diff/40001/src/dsr/model/dsr-option-h...
File src/dsr/model/dsr-option-header.cc (right):

https://codereview.appspot.com/96130043/diff/40001/src/dsr/model/dsr-option-h...
src/dsr/model/dsr-option-header.cc:1229: void
DsrOptionDropRReqHeader::SetNumberOfAddresses (uint8_t n)
minor style nit, put return type on separate line, such as:

void
DsrOptionDropRReqHeader::SetNumber...

https://codereview.appspot.com/96130043/diff/40001/src/dsr/model/dsr-option-h...
File src/dsr/model/dsr-option-header.h (right):

https://codereview.appspot.com/96130043/diff/40001/src/dsr/model/dsr-option-h...
src/dsr/model/dsr-option-header.h:1491: VectorIpv4Address_t m_ipv4Address;
in general, I prefer to avoid a typedef of a basic C++ type such as
std::vector<>; it impairs readability (IMO).

https://codereview.appspot.com/96130043/diff/40001/src/dsr/model/dsr-routing.cc
File src/dsr/model/dsr-routing.cc (right):

https://codereview.appspot.com/96130043/diff/40001/src/dsr/model/dsr-routing....
src/dsr/model/dsr-routing.cc:686: /// Adjust threshold parameter of LEAR option
logging?

https://codereview.appspot.com/96130043/diff/40001/src/dsr/model/dsr-routing.h
File src/dsr/model/dsr-routing.h (right):

https://codereview.appspot.com/96130043/diff/40001/src/dsr/model/dsr-routing....
src/dsr/model/dsr-routing.h:135: getDestinationAddress () const
we prefer to avoid implementation in header files, unless performance requires
inlining (also, the method names should be capitalized).  Also, private variable
names should be prefixed with "m_" which will allow you to avoid name clashes
that require you to use the this pointer in the assignment methods.

https://codereview.appspot.com/96130043/diff/40001/src/dsr/wscript
File src/dsr/wscript (right):

https://codereview.appspot.com/96130043/diff/40001/src/dsr/wscript#newcode4
src/dsr/wscript:4: module = bld.create_ns3_module('dsr', ['internet', 'wifi',
'mesh', 'applications', 'energy'])
why mesh and applications?  Try to delete these (you should only additionally
need 'energy')

[Tom Henderson, 2014-11-18 00:17:05]

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.