[Ns-developers] Underwater acoustic module for NS-3

[craigdo@ee.washington.edu]

> I've been working on an NS3 model for simulation of wireless 
> underwater
> acoustic network simulation.  

[ ... ]

> Although this work is still ongoing, I'd like to submit my 
> progress thus far
> for design review.

[ ... ]

>From a follow-up email:

> The repository has been moved to:
http://code.nsnam.org/ltracy/ns-3-dev-uan
> 
> A couple of known open issues:  1. I plan on reworking UanPropModelBh
> (possibly to store information in a SQL DB).  2.  I'm guessing that
> UanTxMode can likely be combined with WifiMode.
> 

Hi Leonard,

The first "minor detail" :-) is that ns-3-dev-uan doesn't build on
ns-regression:

---------- Begin Included File ----------
cc1plus: warnings being treated as errors
../src/devices/uan/uan-prop-model-bh.cc: In member function â?~virtual bool
ns3::BhConfig::Config(std::string)â?T:
../src/devices/uan/uan-prop-model-bh.cc:80: warning: comparison is always
false due to limited range of data type
../src/devices/uan/uan-prop-model-bh.cc:92: warning: comparison is always
false due to limited range of data type
Build failed
---------- End Included File ----------

This is because npos is defined as -1 and you are comparing with a uint32_t.
I changed the two instances to std::sting::size_type to get it to compile.
The same error is present in create-dat.cc in uan-apps.  You should run this
code through the nightly build environment (at least to see that it compiles
everywhere) using:

sudo bash
su nsnam
cd /tmp
nohup /usr/local/bin/ns-3-run-tests.sh -n ltracy/ns-3-dev-uan -m
lentracy at u.washington.edu &

before you make an "official" submission.  I would use this as an initial
"acceptance test."  

Have you tried to generate and build Python bindings?  Have you tried to
build and run on Cygwin?  We have a list of supported platforms and a common
pitfall is to not try your code "everywhere."

I expected to see an example in the directory examples with something
descriptive in it.  There is a top-level directory called uan-apps that
contains what seems to be example programs.  These should probably go into
the examples directory.  If you think it is time to make a uan subdirectory
under examples, that would be fine.  You don't build uanapps/firstuan.cc --
is this intentional?

It's evident that there is a configuration file of some sort that needs to
be created.  I assumed that this was optional and that I could do something
semi-useful with defaults without running this.  When I run the remaining
example, bhtest, it asserts looking for StaticMobilityModel.  I changed this
to StaticPositionMobilityModel and ran bhtest again.  When I ran bhtest,
then it complained about not finding the config file.

So I gave up on trying to get the code to execute and produce some results.
I really think you should have an example I can just run and get results, or
some more obvious documentation (I didn't see it, but I didn't look very
hard either -- it should be easy to find, perhaps in a devices/uan/uan.h
file as described below).  An approach I took on some of the tap bridge code
that has external dependencies was to just write out what to do in the
example as a comment.  See examples/tap-wifi-dumbbell.cc for what I did.
Some people don't like this much commentary in an example, but it is easy to
find and if you type what I tell you, it just works.  YMMV.

That said, we really do need some kind of test that can be used in our
current regression environment to let us know that everything continues to
work as expected.  You could write a regression test that does the
configuration, but IMO it would be helpful to be able to have a default
configuration that doesn't require a configuration file.  Tom mentioned
something about storing configuration information in a database.  I didn't
see anything like that either.  

Ideally there should be example programs and regression tests, but we aren't
very good at this and so usually what has been done is to overload an
example program that produces trace files into doing double duty.  Take a
look at the regression/tests directory to see what needs to go in.  For
example, the file test-udp-echo.py causes udp-echo (source in
examples/udp-echo.cc) to be run and the trace output compared with
known-good bits checked into ns-3-dev-ref-traces.

IMO your examples should have a few comments at least.  Most examples have
at least an illustration of what is going on.  For example, this is from
examples/csma-one-subnet.cc:

// Network topology
//
//       n0    n1   n2   n3
//       |     |    |    |
//       =================
//              LAN
//
// - CBR/UDP flows from n0 to n1 and from n3 to n0
// - DropTail queues
// - Tracing of queues and packet receptions to file "csma-one-subnet.tr"

The Doxygen throughout is very sparse.  For example, there is no doxygen for
class UanMacAloha.  Now I can guess what this means, but we really need to
document this stuff.

It is convenient to check in a copy of waf in your directory so one can
build out of that directory, so I would check in a src/devices/uan/waf (copy
from wifi).

There are many instance of code that looks like:

namespace ns3
{
class UanMac : public ns3::Object

The ns3:: is redundant.  It looks like you may have added the ns3 namespace
later.

I personally like to see more NS_LOG statements, but that is really a matter
of style that is up to you.  I think they are helpful, especially to people
not intimately familiar with the code, when they're trying to get their
bearings.

I personally like to see the use of _MSG variants of the assertion and error
macros.  It is quite helpful to get some initial information when something
crashes.  But this is really a matter of style that is up to you.

I personally like to see more inline comments in the code.  This can be a
sensitive subject; but for example, 

---------- Begin Included File ----------
double
UanNoiseModelDefault::GetNoiseDbHz(double fKhz) const
{
  double turb, win, ship, thermal, noise;
  turb = 17.0 - 30.0 * log10(fKhz);
  turb = pow(10.0, turb*0.1);

  ship = 40.0 + 20.0 *(this->m_shipping-0.5) + 26.0*log10(fKhz) -
60.0*log10(fKhz+0.03);
  ship = pow(10.0,(ship*0.01));

  win = 50.0+7.5*pow(m_wind, 0.5) + 20.0*log10(fKhz) - 40.0*log10(fKhz+0.4);
  win = pow(10.0, m_wind*0.1);

  thermal = -15 + 20*log10(fKhz);
  thermal = pow(10, thermal*0.1);

  noise = 10*log10(turb + ship + win + thermal);

  return noise;
}
---------- End Included File ----------

The Doxygen says that this a standard ambient acoustic noise model.  Is this
something that would be obvious to anyone using this device in the real
world?  Are all of these various numbers from some authoritative source?
Should there be a better description or a pointer to some further
description.  e.g., this is taken from the YaddaYaddaNoiseModel (see
http://YaddaYaddaNoiseModel.htm for details).

Other devices have a file (cf src/devices/csma/csma.h) that describes what
the purpose of the model is, what it is attempting to model and a basic
description of the device and channel.  This would be very helpful.  For
example, I can imagine how all of the pieces of your device fit together,
but it is always better to have some documentation.  Eventually, someone is
going to suggest that you write a chapter for the manual and have pictures
with circles and arrows ...

I get nervous when I see things like,

  // TODO Auto-generated constructor stub

in the code.  Have you run valgrind?

It's really par for the source, but I have the usual coding standard nits
that seem to be part of every review.  This is going to be painful to fix,
and probably sounds pedantic, but every project has its own version and we
really do want to have a similar coding style across the project.  Everyone
seems to get caught by this to some extent or another.  

1) You should make sure that every source file starts with an emacs mode
line, has an appropriate GPL comment at the top with copyright holder, etc.
You have some of this, but for example, uan-address.cc doesn't have an emacs
mode line and uan-prop-model-bh.cc doesn't have any.

2) You have lots of cases of CamelCase member variables (e.g., uint32_t
m_FreqStepHz) that should be camelBack (e.g., uint32_t m_freqStepHz).  See
the file codingstd.txt under the doc directory.

3) You have lots of cases where your types and parens don't follow the GNU
practice.  For example, you should use 

  string& 
  BhConfig::trim (string& s)

and not

  string& BhConfig::trim( string& s )

I found a couple of malformed while and do constructs, but I lost them ...
sorry.

I realize this has been a litany of negative comments.  The majority of this
should be easily addressed.

The one thing that stood out to me was that I couldn't just "open the box,"
run an example program and get something to work, start looking at traces,
turn on logs, etc.  I submit that this is an important feature for people
who are starting to use your model.  You don't want to turn them off before
they see how great your stuff really is (:-).  You'll have to remedy the
config issue somehow for a regression test, so hopefully it's no big deal to
check in some kind of default configuration and stick in some log messages.

I really can't comment on the model itself since I'm not a domain expert;
and I really have no reasonable way of knowing what your model is supposed
to do other than reading 25 KLOC.  

Regards,

-- Craig