Enhancement in Ns2 trace file reader

[Martin Giachino, 2010-03-27 06:32:37]

Hi, I've uploaded some files for code review. This files are new version of the
ns2 trace file reader and an example file.

To know more of the history of this, please go to
http://www.nsnam.org/bugzilla/show_bug.cgi?id=473

Thanks
Martín

[Martin Giachino, 2010-03-28 21:52:30]

Hi, I've uploaded some files for code review. This files are new version of the
ns2 trace file reader and an example file.

To know more of the history of this, please go to
http://www.nsnam.org/bugzilla/show_bug.cgi?id=473

Thanks
Martín

[Pavel Boyko, 2010-03-30 13:10:27]

First review iteration

http://codereview.appspot.com/802041/diff/4001/3003
File examples/mobility/main-ns2-mob.cc (right):

http://codereview.appspot.com/802041/diff/4001/3003#newcode21
examples/mobility/main-ns2-mob.cc:21: 
file name is not clear. Please rename to, e.g., ns2-mobility-trace.cc

http://codereview.appspot.com/802041/diff/4001/3003#newcode35
examples/mobility/main-ns2-mob.cc:35: 
Add a comment /* This example demonstrates the use of .... */ with detailed
example description including:
 - intended usage
 - behavior
 - expected output

http://codereview.appspot.com/802041/diff/4001/3003#newcode55
examples/mobility/main-ns2-mob.cc:55: // Check command line arguments
Use ns3::CommandLine class to parse command line arguments.

http://codereview.appspot.com/802041/diff/4001/3003#newcode74
examples/mobility/main-ns2-mob.cc:74: std::istringstream s(argv[2]);
Does it work now? I propose to pass number of nodes as a separate command line
argument.

http://codereview.appspot.com/802041/diff/4001/3007
File src/helper/ns2-transmobility-helper.cc (right):

http://codereview.appspot.com/802041/diff/4001/3007#newcode25
src/helper/ns2-transmobility-helper.cc:25: ///
Move this to class comment in the header. Currently doxygen is not configured to
parse .cc files.

http://codereview.appspot.com/802041/diff/4001/3007#newcode57
src/helper/ns2-transmobility-helper.cc:57: typedef struct parse_result
Coding style: change to struct ParseResult {...};

http://codereview.appspot.com/802041/diff/4001/3007#newcode69
src/helper/ns2-transmobility-helper.cc:69: parse_result *ParseNs2Line(const
string& str);
I see no reason for forward declarations, move implementation (aka definition)
here. Also make utility functions static (C style) or define them in the
anonymous namespace (C++ style).

http://codereview.appspot.com/802041/diff/4001/3007#newcode114
src/helper/ns2-transmobility-helper.cc:114: Ns2TransmobilityHelper::ReadDouble
(std::string valueString) const
I see no reason why this is a class method and not an utility function
(non-member non-friend) like above.

http://codereview.appspot.com/802041/diff/4001/3007#newcode124
src/helper/ns2-transmobility-helper.cc:124:
Ns2TransmobilityHelper::LayoutObjectStore (const ObjectStore &store) const
This method is too large. I propose to leave only parsing inside and move all
actions (e.g. set position, set destination, all future actions) to separate
methods. Try to make adding new action (e.g. node on/off) as much simple as
possible.

Also the method name is strange.

http://codereview.appspot.com/802041/diff/4001/3007#newcode133
src/helper/ns2-transmobility-helper.cc:133: int         iNodeId;
= 0. Never declare undefined int, pointer, etc.

http://codereview.appspot.com/802041/diff/4001/3007#newcode139
src/helper/ns2-transmobility-helper.cc:139: parse_result *pr =
ParseNs2Line(line); // Parse line and obtain tokens
Do you really need a pointer here?

http://codereview.appspot.com/802041/diff/4001/3007#newcode144
src/helper/ns2-transmobility-helper.cc:144: NS_LOG_DEBUG ("Parsed line has not a
correct number of parameters: " << line << "\n");
NS_LOG_WARN is better here

http://codereview.appspot.com/802041/diff/4001/3007#newcode172
src/helper/ns2-transmobility-helper.cc:172: continue;
This means unknown node ID. Warning is needed for user!

http://codereview.appspot.com/802041/diff/4001/3007#newcode185
src/helper/ns2-transmobility-helper.cc:185: NS_LOG_DEBUG("Rejected line " <<
line << ": !pr->has_dval[3]");
NS_LOG_WARN is better here

http://codereview.appspot.com/802041/diff/4001/3007#newcode242
src/helper/ns2-transmobility-helper.cc:242: NS_LOG_DEBUG("Time, x coor, y coord
or speed are not a number");
warn

http://codereview.appspot.com/802041/diff/4001/3007#newcode256
src/helper/ns2-transmobility-helper.cc:256: double Speed = pr->dvals[7];
coding style: Speed -> speed

http://codereview.appspot.com/802041/diff/4001/3007#newcode262
src/helper/ns2-transmobility-helper.cc:262: if (Speed > 0)
I'd write == 0 branch first to highlight it (actually comparing floating point
with an _exact_ 0 is a rare case).

http://codereview.appspot.com/802041/diff/4001/3007#newcode313
src/helper/ns2-transmobility-helper.cc:313: NS_LOG_DEBUG("Rejected line " <<
line << ": !pr->has_dval[3]");
warn

http://codereview.appspot.com/802041/diff/4001/3007#newcode321
src/helper/ns2-transmobility-helper.cc:321: //set the position for the coord.
please try to factor out the copy-pasted code from above. I'am pretty sure that
"set initial position" and "scheduled set position" can be processed by single
method.

http://codereview.appspot.com/802041/diff/4001/3007#newcode357
src/helper/ns2-transmobility-helper.cc:357: NS_LOG_DEBUG ("Format Line is not
correct: " << line << "\n");
warning

http://codereview.appspot.com/802041/diff/4001/3007#newcode360
src/helper/ns2-transmobility-helper.cc:360: delete pr;
Consider once again do not have pointer for pr.

http://codereview.appspot.com/802041/diff/4001/3007#newcode390
src/helper/ns2-transmobility-helper.cc:390: NS_LOG_DEBUG("Line has no node Id: "
<< line);
warning

http://codereview.appspot.com/802041/diff/4001/3007#newcode410
src/helper/ns2-transmobility-helper.cc:410: int tokensLength   =
ret->tokens.size();                 // number of tokens in line
int -> size_t

http://codereview.appspot.com/802041/diff/4001/3007#newcode457
src/helper/ns2-transmobility-helper.cc:457: TrimNs2Line(const string& s)
Check coding style in this function (indentation, spaces, ...)

http://codereview.appspot.com/802041/diff/4001/3007#newcode477
src/helper/ns2-transmobility-helper.cc:477: IsNumber(const string& str)
What about -1.0, 1e+10, etc.? Consider using std::strtod for parsing strings to
doubles. Good example can be found in ./contrib/stats/omnet-data-output.cc : 

inline bool isNumeric(const std::string& s) {
  char *endp;
  double unused = strtod(s.c_str(), &endp); // declared with warn_unused_result
  unused = unused; // quiet compiler
  return endp == s.c_str() + s.size();
}

http://codereview.appspot.com/802041/diff/4001/3007#newcode517
src/helper/ns2-transmobility-helper.cc:517: if (isdigit(str[i]) || str[i] ==
'.') ret += str[i];
Please support negative numbers (e.g. negative coordinates -- why not?). Don't
forget to add a test case for this.

[Martin Giachino, 2010-03-31 06:50:00]

http://codereview.appspot.com/802041/diff/4001/3003
File examples/mobility/main-ns2-mob.cc (right):

http://codereview.appspot.com/802041/diff/4001/3003#newcode21
examples/mobility/main-ns2-mob.cc:21: 
On 2010/03/30 13:10:28, Pavel Boyko wrote:
> file name is not clear. Please rename to, e.g., ns2-mobility-trace.cc

Done.

http://codereview.appspot.com/802041/diff/4001/3003#newcode35
examples/mobility/main-ns2-mob.cc:35: 
On 2010/03/30 13:10:28, Pavel Boyko wrote:
> Add a comment /* This example demonstrates the use of .... */ with detailed
> example description including:
>  - intended usage
>  - behavior
>  - expected output

Done.

http://codereview.appspot.com/802041/diff/4001/3007
File src/helper/ns2-transmobility-helper.cc (right):

http://codereview.appspot.com/802041/diff/4001/3007#newcode25
src/helper/ns2-transmobility-helper.cc:25: ///
On 2010/03/30 13:10:28, Pavel Boyko wrote:
> Move this to class comment in the header. Currently doxygen is not configured
to
> parse .cc files. 

Done.

http://codereview.appspot.com/802041/diff/4001/3007#newcode57
src/helper/ns2-transmobility-helper.cc:57: typedef struct parse_result
On 2010/03/30 13:10:28, Pavel Boyko wrote:
> Coding style: change to struct ParseResult {...};

Done.

http://codereview.appspot.com/802041/diff/4001/3007#newcode69
src/helper/ns2-transmobility-helper.cc:69: parse_result *ParseNs2Line(const
string& str);
On 2010/03/30 13:10:28, Pavel Boyko wrote:
> I see no reason for forward declarations, move implementation (aka definition)
> here. Also make utility functions static (C style) or define them in the
> anonymous namespace (C++ style).

Done.

http://codereview.appspot.com/802041/diff/4001/3007#newcode114
src/helper/ns2-transmobility-helper.cc:114: Ns2TransmobilityHelper::ReadDouble
(std::string valueString) const
On 2010/03/30 13:10:28, Pavel Boyko wrote:
> I see no reason why this is a class method and not an utility function
> (non-member non-friend) like above.

Done.

http://codereview.appspot.com/802041/diff/4001/3007#newcode133
src/helper/ns2-transmobility-helper.cc:133: int         iNodeId;
On 2010/03/30 13:10:28, Pavel Boyko wrote:
> = 0. Never declare undefined int, pointer, etc. 

Done.

http://codereview.appspot.com/802041/diff/4001/3007#newcode139
src/helper/ns2-transmobility-helper.cc:139: parse_result *pr =
ParseNs2Line(line); // Parse line and obtain tokens
On 2010/03/30 13:10:28, Pavel Boyko wrote:
> Do you really need a pointer here?

Done.

http://codereview.appspot.com/802041/diff/4001/3007#newcode144
src/helper/ns2-transmobility-helper.cc:144: NS_LOG_DEBUG ("Parsed line has not a
correct number of parameters: " << line << "\n");
On 2010/03/30 13:10:28, Pavel Boyko wrote:
> NS_LOG_WARN is better here

Done.

http://codereview.appspot.com/802041/diff/4001/3007#newcode172
src/helper/ns2-transmobility-helper.cc:172: continue;
On 2010/03/30 13:10:28, Pavel Boyko wrote:
> This means unknown node ID. Warning is needed for user!

Done.

http://codereview.appspot.com/802041/diff/4001/3007#newcode185
src/helper/ns2-transmobility-helper.cc:185: NS_LOG_DEBUG("Rejected line " <<
line << ": !pr->has_dval[3]");
On 2010/03/30 13:10:28, Pavel Boyko wrote:
> NS_LOG_WARN is better here

Done.

http://codereview.appspot.com/802041/diff/4001/3007#newcode242
src/helper/ns2-transmobility-helper.cc:242: NS_LOG_DEBUG("Time, x coor, y coord
or speed are not a number");
On 2010/03/30 13:10:28, Pavel Boyko wrote:
> warn

Done.

http://codereview.appspot.com/802041/diff/4001/3007#newcode256
src/helper/ns2-transmobility-helper.cc:256: double Speed = pr->dvals[7];
On 2010/03/30 13:10:28, Pavel Boyko wrote:
> coding style: Speed -> speed

Done.

http://codereview.appspot.com/802041/diff/4001/3007#newcode262
src/helper/ns2-transmobility-helper.cc:262: if (Speed > 0)
On 2010/03/30 13:10:28, Pavel Boyko wrote:
> I'd write == 0 branch first to highlight it (actually comparing floating point
> with an _exact_ 0 is a rare case). 

Pavel, I didn't understand this.

http://codereview.appspot.com/802041/diff/4001/3007#newcode313
src/helper/ns2-transmobility-helper.cc:313: NS_LOG_DEBUG("Rejected line " <<
line << ": !pr->has_dval[3]");
On 2010/03/30 13:10:28, Pavel Boyko wrote:
> warn

Done.

http://codereview.appspot.com/802041/diff/4001/3007#newcode357
src/helper/ns2-transmobility-helper.cc:357: NS_LOG_DEBUG ("Format Line is not
correct: " << line << "\n");
On 2010/03/30 13:10:28, Pavel Boyko wrote:
> warning

Done.

http://codereview.appspot.com/802041/diff/4001/3007#newcode360
src/helper/ns2-transmobility-helper.cc:360: delete pr;
On 2010/03/30 13:10:28, Pavel Boyko wrote:
> Consider once again do not have pointer for pr.

Done.

http://codereview.appspot.com/802041/diff/4001/3007#newcode390
src/helper/ns2-transmobility-helper.cc:390: NS_LOG_DEBUG("Line has no node Id: "
<< line);
On 2010/03/30 13:10:28, Pavel Boyko wrote:
> warning

Done.

http://codereview.appspot.com/802041/diff/4001/3007#newcode410
src/helper/ns2-transmobility-helper.cc:410: int tokensLength   =
ret->tokens.size();                 // number of tokens in line
On 2010/03/30 13:10:28, Pavel Boyko wrote:
> int -> size_t

Done.

[Martin Giachino, 2010-04-11 06:54:12]

Pavel, here is a new version.

Martín

[Mathieu Lacage, 2010-04-12 12:34:10]

it would be nice to run new code through checks-style.py

I did not read the code carefully so, I don't know how meaningful that would be,
but it would be nice to move the standalone static functions defined at the top
of the file into some class.

http://codereview.appspot.com/802041/diff/11001/12006
File src/helper/ns2-transmobility-helper.cc (right):

http://codereview.appspot.com/802041/diff/11001/12006#newcode245
src/helper/ns2-transmobility-helper.cc:245: }
coding style

http://codereview.appspot.com/802041/diff/11001/12006#newcode258
src/helper/ns2-transmobility-helper.cc:258: }
coding style

http://codereview.appspot.com/802041/diff/11001/12006#newcode383
src/helper/ns2-transmobility-helper.cc:383: return ret;
if you run this through check-style.py --level=3, I believe that it will insert
extra {} here.

http://codereview.appspot.com/802041/diff/11001/12006#newcode424
src/helper/ns2-transmobility-helper.cc:424: {
coding style: missing indent

[Martin Giachino, 2010-04-13 03:55:54]

Hi Mathieu, thanks for the review.

Before to do changes, I have two things:

I think that some static functions would be private methods of the helper,
because they only have sense for the helper (hasNodeIdNumber, GetNodeIdInt,
GetNodeIdString,SetOneInitialCoord, IsSetInitialPos, IsSchedSetPos,
IsSchedMobilityPos, SetMovement, SetInitialPosition, SetSchedPosition), do you
agree?

The rest of them, are more general, do you think is necessary to put into a new
class?

Martín

[Pavel Boyko, 2010-04-13 07:23:34]

Hi, Martin,

  Thank you for update, I'll try to review it this week. Please do run
check-style.py before updating this issue to prevent all those noisy code style
related comments. 

> I think that some static functions would be private methods of the helper,

  Generally you should _avoid_ private methods in favor of (static) free
functions where possible. The main reason is that methods can access an internal
object state and free function can't. Google for "non-member non-friend" to find
a number of good articles on the topic. I propose to keep all utility functions
you mention as is.

  Pavel

[Mathieu Lacage, 2010-04-13 08:40:24]

On Tue, Apr 13, 2010 at 5:55 AM,  <martin.giachino@gmail.com> wrote:
> Hi Mathieu, thanks for the review.
>
> Before to do changes, I have two things:
>
> I think that some static functions would be private methods of the
> helper, because they only have sense for the helper (hasNodeIdNumber,
> GetNodeIdInt, GetNodeIdString,SetOneInitialCoord, IsSetInitialPos,
> IsSchedSetPos, IsSchedMobilityPos, SetMovement, SetInitialPosition,
> SetSchedPosition), do you agree?

I would be fine with this.

> The rest of them, are more general, do you think is necessary to put
> into a new class?

I don't have any strong feeling about this: I usually try to limit the
visibility scope of the functions I write as much as I can but this is
pavel's call.

Mathieu
-- 
Mathieu Lacage <mathieu.lacage@gmail.com>

[Martin Giachino, 2010-05-18 00:28:11]

Here is a new update.

This update corrects code style, add support for negative numbers and for float
numbers like 1.0e+2, and add some test cases.

Martín

[Martin Giachino, 2010-05-18 00:35:18]

The last test case named "Values from a generator" is commented because I'm
having some problems. Output is shown at next, and it fails whit a strange
error. Maybe is only a precision trouble and not an error, do you think is
important to solve it? As you will see, values are equals, almost for reported
in the log, so I think is not a real problem.

-------
  FAIL: Test Case "Values from a generator" (real 0.000 user 0.000 system 0.000)
    Details:
      Message:   Time mismatch
      Condition: time (actual) == ref.time (limit)
      Actual:    417714814ns
      Limit:     417714814ns
      File:      ../src/helper/ns2-transmobility-helper.cc
      Line:      780
    Details:
      Message:   Position mismatch at time 0.417715 s for node 0
      Condition: pos (actual) == ref.pos (limit)
      Actual:    150:110:0
      Limit:     150:110:0
      File:      ../src/helper/ns2-transmobility-helper.cc
      Line:      782
------------------------------

Martín

[Pavel Boyko, 2010-05-18 06:55:58]

Hi, Martin,

  thank you for update, I will review it by the end of the week. 

On 2010/05/18 00:35:18, Martin Giachino wrote:
> The last test case named "Values from a generator" is commented because I'm
> having some problems. Output is shown at next, and it fails whit a strange
> error. Maybe is only a precision trouble and not an error, do you think is
> important to solve it? 

  I hope this is some simple mistake somewhere in the tests code, I will try to
figure it out.

  Pavel

[Pavel Boyko, 2010-05-21 07:47:37]

Hi, Martin,

  Your core is almost ready for the merge, I have the last two comments:

1. Your last test fails because of too precise coordinates and speed. NS-3 in
its default configuration has a nanosecond time precision, while your test
numbers like 50.40378694202284 m/s need 10^5 times more. Please truncate your
numbers to, e.g. mm and mm/s precision.  

2. Running the example I have a warning with bad diagnose:

$ ./waf --run "ns2-mobility-trace --nodeNum=2
--traceFile=$P/examples/mobility/main-ns2-mob.ns_movements --logFile=log"

[SKIP]

Calculated Speed: X=0 Y=50 Z=0
Positions after parse for node 1 1 x=170 y=143.34 z=0
Line has no node Id: 
Parsed line has not a correct number of parameters: 

And the last two records in my log file are: 

5991349486ns POS: x=210, y=155.667, z=0; VEL:0, y=0, z=0
5991349486ns POS: x=210, y=155.667, z=0; VEL:0, y=50, z=0
 
  This must be fixed.

Also come cosmetics in the examples: don't use (argc == 4) check, check that all
your command line arguments have non-default values instead. NS-3 command line
parser allows to setup many default things from command line (e.g. RNG seed) --
don't prevent your users of using this. And (the final one :).

  Good luck,
  Pavel

[Martin Giachino, 2010-05-22 22:54:04]

Hi Pavel, here is the update.

Fixes are ready, except for the last test case. It is still not working after
number truncations. Now the test case fails with position:

  FAIL: Test Case "Values from a generator" (real 0.000 user 0.000 system 0.000)
    Details:
      Message:   Position mismatch at time 0.417739 s for node 0
      Condition: pos (actual) == ref.pos (limit)
      Actual:    150:110:0
      Limit:     150:110:0
      File:      ../src/helper/ns2-transmobility-helper.cc
      Line:      797


Martín

[Tom Henderson, 2010-05-26 14:27:26]

Mostly minor things, but I have a concern about precision.  Please make sure
that any truncation that is done to convert the string time to an ns-time allows
for the possibility that users may set simulator TimeStepPrecision
(src/simulator/nstime.h) down to PS precision, and doesn't just assume NS.

http://codereview.appspot.com/802041/diff/35001/36003
File examples/mobility/ns2-mobility-trace.cc (right):

http://codereview.appspot.com/802041/diff/35001/36003#newcode46
examples/mobility/ns2-mobility-trace.cc:46: *                included in the
same directory that the present file.
Can this be fixed (allowing relative paths)?  suggest a different file name such
as "default.ns_movements" (where the ns_movements suffix seems to be a legacy
file suffix choice)

http://codereview.appspot.com/802041/diff/35001/36006
File src/helper/ns2-transmobility-helper.cc (right):

http://codereview.appspot.com/802041/diff/35001/36006#newcode21
src/helper/ns2-transmobility-helper.cc:21: *               Martín Giachino
<martin.giachino@gmail.com>
Did Francesco Malandrino contribute to this implementation-- he started this bug
473 thread and patching.

http://codereview.appspot.com/802041/diff/35001/36006#newcode64
src/helper/ns2-transmobility-helper.cc:64: // Type to mantain line parsed and
its values
s/mantain/maintain

http://codereview.appspot.com/802041/diff/35001/36006#newcode178
src/helper/ns2-transmobility-helper.cc:178: NS_LOG_WARN ("Parsed line has not a
correct number of parameters: " << line << "\n");
shouldn't you error out if the trace file is not correct?

http://codereview.appspot.com/802041/diff/35001/36006#newcode184
src/helper/ns2-transmobility-helper.cc:184: iNodeId = GetNodeIdInt (pr);
return value is not checked here-- what if returns -1?  I would suggest to fail
the program since it may be corrupted trace file.

http://codereview.appspot.com/802041/diff/35001/36006#newcode192
src/helper/ns2-transmobility-helper.cc:192: NS_LOG_WARN ("Unknown node ID: " <<
nodeId << "");
another instance of proceeding despite possibly mangled or unsupported trace
file.

http://codereview.appspot.com/802041/diff/35001/36006#newcode394
src/helper/ns2-transmobility-helper.cc:394: unused = unused; // quiet compiler
don't understand why this statement is needed.  The return value of strtod is
used (assigned to "unused").

http://codereview.appspot.com/802041/diff/35001/36007
File src/helper/ns2-transmobility-helper.h (right):

http://codereview.appspot.com/802041/diff/35001/36007#newcode34
src/helper/ns2-transmobility-helper.h:34: *        generated by SUMO and TRANS.
There are other mobility generators than TRANS, such as BonnMotion and scengen. 
If this helper is more generic than TRANS, then please describe here which of
the generators are believed to be supported (or not).  If this is just a generic
ns-2 mobility trace helper, we could consider to remove the "Trans" from the
class name.

[Martin Giachino, 2010-05-31 00:23:15]

Tom,

Mostly minor things, but I have a concern about precision.  Please make sure
that any truncation that is done to convert the string time to an ns-time allows
for the possibility that users may set simulator TimeStepPrecision
(src/simulator/nstime.h) down to PS precision, and doesn't just assume NS.
--
I didn't understand this. Could you explain me again?



http://codereview.appspot.com/802041/diff/35001/36003#newcode46
examples/mobility/ns2-mobility-trace.cc:46: *                included in
the same directory that the present file.
Can this be fixed (allowing relative paths)?  suggest a different file
name such as "default.ns_movements" (where the ns_movements suffix seems
to be a legacy file suffix choice)
----
Done

http://codereview.appspot.com/802041/diff/35001/36006
File src/helper/ns2-transmobility-helper.cc (right):

http://codereview.appspot.com/802041/diff/35001/36006#newcode21
src/helper/ns2-transmobility-helper.cc:21: *               Martín Giachino
<martin.giachino@gmail.com>
No directly, but I use the struct ParseResult from his code.

http://codereview.appspot.com/802041/diff/35001/36006#newcode64
src/helper/ns2-transmobility-helper.cc:64: // Type to mantain line parsed and
its values
On 2010/05/26 14:27:26, Tom Henderson wrote:
> s/mantain/maintain

Done.

http://codereview.appspot.com/802041/diff/35001/36006#newcode178
src/helper/ns2-transmobility-helper.cc:178: NS_LOG_WARN ("Parsed line has not a
correct number of parameters: " << line << "\n");
Yes, it could be better. Are you saying to change WARN to ERROR?

http://codereview.appspot.com/802041/diff/35001/36006#newcode184
src/helper/ns2-transmobility-helper.cc:184: iNodeId = GetNodeIdInt (pr);
On 2010/05/26 14:27:26, Tom Henderson wrote:
> return value is not checked here-- what if returns -1?  I would suggest to
fail
> the program since it may be corrupted trace file.

If parsed line hasn't node number the program aborts before., but anyway I could
add this fix. When you said "to fail the program" should I put an assert?

http://codereview.appspot.com/802041/diff/35001/36006#newcode192
src/helper/ns2-transmobility-helper.cc:192: NS_LOG_WARN ("Unknown node ID: " <<
nodeId << "");
ok

http://codereview.appspot.com/802041/diff/35001/36006#newcode394
src/helper/ns2-transmobility-helper.cc:394: unused = unused; // quiet compiler
compile process fails with:
../src/helper/ns2-transmobility-helper.cc:393: error: unused variable ‘unused’

I've changed code in this way:
  double unused;
  unused = strtod (s.c_str (), &endp); // declared with warn_unused_result

http://codereview.appspot.com/802041/diff/35001/36007
File src/helper/ns2-transmobility-helper.h (right):

http://codereview.appspot.com/802041/diff/35001/36007#newcode34
src/helper/ns2-transmobility-helper.h:34: *        generated by SUMO and TRANS.
This helper works with any trace file that uses the ns2 statements:

$node set X_ \<x1\>
$node set Y_ \<y1\>
$node set Z_ \<z1\>

$ns at $time $node setdest \<x2\> \<y2\> \<speed\>
$ns at $time $node set X_ \<x1\>
$ns at $time $node set Y_ \<Y1\>
$ns at $time $node set Z_ \<Z1\>

In fact all my tests are upon traces generated by BonnMotion, and not with TRANS
nor SUMO. File name is just carried from the start, but I think it's ok to
remove "trans" from it. If we do this, we have to ask to Mathiew Lacage because
a file named ns2-mobility-helper already exists.

[Tom Henderson, 2010-05-31 04:20:34]

On 2010/05/31 00:23:15, Martin Giachino wrote:
> Tom,
> 
> Mostly minor things, but I have a concern about precision.  Please make sure
> that any truncation that is done to convert the string time to an ns-time
allows
> for the possibility that users may set simulator TimeStepPrecision
> (src/simulator/nstime.h) down to PS precision, and doesn't just assume NS.
> --
> I didn't understand this. Could you explain me again?

I don't know whether you are planning to round all times to NS precision, but I
just wanted to note that ns-3 can optionally be set to finer time precision.  I
don't even know whether it is reasonable to have finer grained mobility
timestamps than NS but what I had envisioned (and drew my comment) was that a
user may see in the mobility trace that "oh, the timestamps require higher time
precision" and set ns-3 correspondingly, but not see any difference due to
rounding.

Feel free to ignore if you think this is a non issue; I just wanted to raise for
your consideration.

> 
> 
> 
> http://codereview.appspot.com/802041/diff/35001/36003#newcode46
> examples/mobility/ns2-mobility-trace.cc:46: *                included in
> the same directory that the present file.
> Can this be fixed (allowing relative paths)?  suggest a different file
> name such as "default.ns_movements" (where the ns_movements suffix seems
> to be a legacy file suffix choice)
> ----
> Done
> 
> http://codereview.appspot.com/802041/diff/35001/36006
> File src/helper/ns2-transmobility-helper.cc (right):
> 
> http://codereview.appspot.com/802041/diff/35001/36006#newcode21
> src/helper/ns2-transmobility-helper.cc:21: *               Martín Giachino
> <mailto:martin.giachino@gmail.com>
> No directly, but I use the struct ParseResult from his code.
> 
> http://codereview.appspot.com/802041/diff/35001/36006#newcode64
> src/helper/ns2-transmobility-helper.cc:64: // Type to mantain line parsed and
> its values
> On 2010/05/26 14:27:26, Tom Henderson wrote:
> > s/mantain/maintain
> 
> Done.
> 
> http://codereview.appspot.com/802041/diff/35001/36006#newcode178
> src/helper/ns2-transmobility-helper.cc:178: NS_LOG_WARN ("Parsed line has not
a
> correct number of parameters: " << line << "\n");
> Yes, it could be better. Are you saying to change WARN to ERROR?

yes, to force user to deal with the incompability.

> 
> http://codereview.appspot.com/802041/diff/35001/36006#newcode184
> src/helper/ns2-transmobility-helper.cc:184: iNodeId = GetNodeIdInt (pr);
> On 2010/05/26 14:27:26, Tom Henderson wrote:
> > return value is not checked here-- what if returns -1?  I would suggest to
> fail
> > the program since it may be corrupted trace file.
> 
> If parsed line hasn't node number the program aborts before., but anyway I
could
> add this fix. When you said "to fail the program" should I put an assert?

NS_FATAL_ERROR

> 
> http://codereview.appspot.com/802041/diff/35001/36006#newcode192
> src/helper/ns2-transmobility-helper.cc:192: NS_LOG_WARN ("Unknown node ID: "
<<
> nodeId << "");
> ok
> 
> http://codereview.appspot.com/802041/diff/35001/36006#newcode394
> src/helper/ns2-transmobility-helper.cc:394: unused = unused; // quiet compiler
> compile process fails with:
> ../src/helper/ns2-transmobility-helper.cc:393: error: unused variable ‘unused’
> 
> I've changed code in this way:
>   double unused;
>   unused = strtod (s.c_str (), &endp); // declared with warn_unused_result
> 
> http://codereview.appspot.com/802041/diff/35001/36007
> File src/helper/ns2-transmobility-helper.h (right):
> 
> http://codereview.appspot.com/802041/diff/35001/36007#newcode34
> src/helper/ns2-transmobility-helper.h:34: *        generated by SUMO and
TRANS.
> This helper works with any trace file that uses the ns2 statements:
> 
> $node set X_ \<x1\>
> $node set Y_ \<y1\>
> $node set Z_ \<z1\>
> 
> $ns at $time $node setdest \<x2\> \<y2\> \<speed\>
> $ns at $time $node set X_ \<x1\>
> $ns at $time $node set Y_ \<Y1\>
> $ns at $time $node set Z_ \<Z1\>
> 
> In fact all my tests are upon traces generated by BonnMotion, and not with
TRANS
> nor SUMO. File name is just carried from the start, but I think it's ok to
> remove "trans" from it. If we do this, we have to ask to Mathiew Lacage
because
> a file named ns2-mobility-helper already exists.

OK, maybe these two helper files should be merged?  Or do they deal with
different trace formats-- can you clarify?

[Martin Giachino, 2010-06-10 07:36:57]

Tom,

- NS_LOG_WARN changed by NS_LOG_ERROR

- Removed comments about TRANS/SUMO and added documentation for the ns2 traces
format.

- Removed last test case, I think it is helpful now.

Martín

[Pavel Boyko, 2010-06-20 13:28:12]

Martin,

  I have merged your reader replacing old Ns2MobilityHelper implementation. I am
sure that your contribution will help many people integrate ns-3 with different
mobility generators/analyzers. Thank you!

  Please close this review.

  Pavel

On 2010/06/10 07:36:57, Martin Giachino wrote:
> Tom,
> 
> - NS_LOG_WARN changed by NS_LOG_ERROR
> 
> - Removed comments about TRANS/SUMO and added documentation for the ns2 traces
> format.
> 
> - Removed last test case, I think it is helpful now.
> 
> Martín

[Martin Giachino, 2010-06-20 23:09:22]

It was a pleasure, and thanks to all of you for your help.

Martín


On 2010/06/20 13:28:12, Pavel Boyko wrote:
> Martin,
> 
>   I have merged your reader replacing old Ns2MobilityHelper implementation. I
am
> sure that your contribution will help many people integrate ns-3 with
different
> mobility generators/analyzers. Thank you!
> 
>   Please close this review.
> 
>   Pavel
> 
> On 2010/06/10 07:36:57, Martin Giachino wrote:
> > Tom,
> > 
> > - NS_LOG_WARN changed by NS_LOG_ERROR
> > 
> > - Removed comments about TRANS/SUMO and added documentation for the ns2
traces
> > format.
> > 
> > - Removed last test case, I think it is helpful now.
> > 
> > Martín