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

[Leonard Tracy]

Hi Craig,

Thank you very much for the comments.  I will address all of these issues.
The specific piece of the model that it sounds like is most objectionable
(uan-prop-bh/ and the programs that use/support it bhtest/create-dat) I knew
were poor and I've been considering how best to rework that code.  I should
have this fixed up this weekend.  I've also begun writing more
documentation.  In short, I think I will have these issues addressed in
short order.  Thanks for the tip on the nightly build also.

Leonard


On Mon, Apr 13, 2009 at 7:04 PM, <craigdo at ee.washington.edu> wrote:

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