underwater acoustic device

[cra...@ee.washington.edu]

Mostly nits and doxygen requests.

I think this needs to go in its own examples directory and take all of
the dat files with it.

Overall very nice.

http://codereview.appspot.com/87043/diff/1/2
File examples/dat/105-10000.dat (right):

http://codereview.appspot.com/87043/diff/1/2#newcode1
Line 1: rd 5
This sits in a dat directory under examples. This is intimately tied to
uan, so should be in a uan/dat directory at least. Probably examples
should go in uan directory as well.

http://codereview.appspot.com/87043/diff/1/1016
File examples/uan-create-dat.cc (right):

http://codereview.appspot.com/87043/diff/1/1016#newcode23
Line 23:
This file is in a directory under examples. I think the uan examples if
not completely self-contained should be in a subdirectory uan.

For example, examples/uan/script and examples/uan/data

http://codereview.appspot.com/87043/diff/1/18
File examples/uan-cw-example.cc (right):

http://codereview.appspot.com/87043/diff/1/18#newcode123
Line 123: SeedManager::SetRun(SeedManager::GetRun() + 1);
Coding std says SetRun (Seedmanager::GetRun ()

I know it's annoying. Sometimes I have had to query-replace '(' with '
(' in many files ...

There are a few more of these above.

http://codereview.appspot.com/87043/diff/1/18#newcode134
Line 134: for (; it != nodes.End(); it++)
Nit: Not many people appreciate it, nor code it, but ++it is faster
since it doesn't have to return an intermediate value :-)

Nothing super-important, but I've gotten in the habit of typing prefix
increments and doing initializations instead of assigments in
constructors even though the universe won't end if its not done.

http://codereview.appspot.com/87043/diff/1/18#newcode137
Line 137: Vector a(uv.GetValue(), uv.GetValue(), 70.0);
Coding std says Vector a (uv.GetValue (), etc. ...

http://codereview.appspot.com/87043/diff/1/18#newcode164
Line 164: PacketSocketHelper pktskth;
I tried to pronounce pktskth and I don't think my tongue will be the
same :-)

http://codereview.appspot.com/87043/diff/1/18#newcode221
Line 221: Time now = Seconds (0.5);
I find it a bit counterintuitive that now is actually going to be some
time in the future below -- it really isn't now. Simulator::Now () will
return t=0 through this whole process.

http://codereview.appspot.com/87043/diff/1/18#newcode306
Line 306: uan.SetPhy (std::string ("ns3::UanPhyGen"),
StringValue ("ns3::UanPhyGen") ???

http://codereview.appspot.com/87043/diff/1/19
File examples/uan-rc-example.cc (right):

http://codereview.appspot.com/87043/diff/1/19#newcode184
Line 184: if(m_doNode)
Coding std says if (m_doNode)

Other instances through this file ...

http://codereview.appspot.com/87043/diff/1/1022
File src/devices/uan/uan-address.cc (right):

http://codereview.appspot.com/87043/diff/1/1022#newcode34
Line 34: UanAddress::UanAddress (uint8_t addr) :
Nit: Typically the ':' goes on the next line.

http://codereview.appspot.com/87043/diff/1/1022#newcode120
Line 120: return !(a == b);
Why is this not !(a.m_address == b.m_address)?
Conversely, why is operator == not (a == b)?

http://codereview.appspot.com/87043/diff/1/1022#newcode126
Line 126: os << (int) address.m_address;
i would use a c++-style cast; and we like to use more portable types
like int32_t.

http://codereview.appspot.com/87043/diff/1/1022#newcode132
Line 132: int x;
I would use int32_t here since you want to check for truncation.

http://codereview.appspot.com/87043/diff/1/1022#newcode134
Line 134: NS_ASSERT (0 <= x);
This is somewhat cryptic. I would use

NS_ASSERT_MSG (x >= 0 && x <= 255, "UanAddresses must be 0..255");

http://codereview.appspot.com/87043/diff/1/1024
File src/devices/uan/uan-channel.cc (right):

http://codereview.appspot.com/87043/diff/1/1024#newcode70
Line 70: UanChannel::UanChannel () :
Not sure if actually in coding std, but ':' typically goes on next line

http://codereview.appspot.com/87043/diff/1/1024#newcode71
Line 71: m_prop (0)
m_prop is zeroed. Why not m_noise?

http://codereview.appspot.com/87043/diff/1/1024#newcode76
Line 76: {
m_prop and m_noise are Ptr<> ... where are they released?

http://codereview.appspot.com/87043/diff/1/1026
File src/devices/uan/uan-header-common.cc (right):

http://codereview.appspot.com/87043/diff/1/1026#newcode96
Line 96: //Inherrited methods
Bouncy keyboard? Inherited.

http://codereview.appspot.com/87043/diff/1/1029
File src/devices/uan/uan-header-rc.cc (right):

http://codereview.appspot.com/87043/diff/1/1029#newcode36
Line 36: // TODO Auto-generated constructor stub
This kind of comment makes me very uneasy. What is not done?

http://codereview.appspot.com/87043/diff/1/1029#newcode50
Line 50: // TODO Auto-generated destructor stub
What is left to do? What implications?

http://codereview.appspot.com/87043/diff/1/1029#newcode57
Line 57: static TypeId tid = TypeId ("ns3::UanHeaderRcData")
Missing NS_OBJECT_ENSURE_REGISTERED?

http://codereview.appspot.com/87043/diff/1/1029#newcode98
Line 98: start.WriteU16( (uint16_t) (1000.0*m_propDelay.GetSeconds () +
0.5));
Coding std triva: s/b WriteU16 (( not WriteU16( (

Should use C++-style cast.

http://codereview.appspot.com/87043/diff/1/1029#newcode154
Line 154: static TypeId tid = TypeId ("ns3::UanHeaderRcRts")
Missing NS_OBJECT_ENSURE_REGISTERED?

http://codereview.appspot.com/87043/diff/1/1029#newcode223
Line 223: return 1 + 1 + 1 + 4 + 2;
Nit: Looking at Serialize, should be 1 + 1 + 1 + 2 + 4 for clarity.

http://codereview.appspot.com/87043/diff/1/1029#newcode291
Line 291: static TypeId tid = TypeId ("ns3::UanHeaderRcCtsGlobal")
Missing NS_OBJECT_ENSURE_REGISTERED?

http://codereview.appspot.com/87043/diff/1/1029#newcode358
Line 358: start.WriteU32 ( (uint32_t) (m_timeStampTx.GetSeconds
()*1000.0 + 0.5));
C++-style cast?

http://codereview.appspot.com/87043/diff/1/1029#newcode416
Line 416: static TypeId tid = TypeId ("ns3::UanHeaderRcCts")
Missing NS_OBJECT_ENSURE_REGISTERED?

http://codereview.appspot.com/87043/diff/1/1029#newcode539
Line 539: static TypeId tid = TypeId ("ns3::UanHeaderRcAck")
Missing NS_OBJECT_ENSURE_REGISTERED?

http://codereview.appspot.com/87043/diff/1/1033
File src/devices/uan/uan-mac-cw.cc (right):

http://codereview.appspot.com/87043/diff/1/1033#newcode141
Line 141: uint32_t cw = (uint32_t) rv.GetValue ();
c++-style cast?

http://codereview.appspot.com/87043/diff/1/1034
File src/devices/uan/uan-mac-rc-gw.cc (right):

http://codereview.appspot.com/87043/diff/1/1034#newcode52
Line 52: operator < (UanAddress &a, UanAddress &b)
I realize this is probably to get some stl container to work, but if you
have < IMO you should implement other similar operators.

http://codereview.appspot.com/87043/diff/1/1034#newcode77
Line 77: {
Where is Ptr<> m_phy released?

http://codereview.appspot.com/87043/diff/1/1034#newcode319
Line 319: if (numRts)
coding std says include braces even with one-liners

http://codereview.appspot.com/87043/diff/1/1034#newcode331
Line 331: for (;rit != m_requests.end (); rit++)
++rit is faster :-)

http://codereview.appspot.com/87043/diff/1/38
File src/devices/uan/uan-mac-rc.h (right):

http://codereview.appspot.com/87043/diff/1/38#newcode24
Line 24: #ifndef UANMACRC_H_
ns-3 typically doesn't use the trailing underscore

http://codereview.appspot.com/87043/diff/1/1038
File src/devices/uan/uan-net-device.cc (right):

http://codereview.appspot.com/87043/diff/1/1038#newcode66
Line 66: UanNetDevice::GetTypeId ()
The trace hooks have undergone lots of rework since you cloned this.
You should update to the current set of trace hooks.

http://codereview.appspot.com/87043/diff/1/41
File src/devices/uan/uan-noise-model-default.h (right):

http://codereview.appspot.com/87043/diff/1/41#newcode56
Line 56: virtual double GetNoiseDbHz (double fKhz) const;
Doxygen?

http://codereview.appspot.com/87043/diff/1/1042
File src/devices/uan/uan-phy-dual.cc (right):

http://codereview.appspot.com/87043/diff/1/1042#newcode80
Line 80: if (mode.GetModType () != UanTxMode::OTHER)
Coding std says braces even if one liner

http://codereview.appspot.com/87043/diff/1/46
File src/devices/uan/uan-phy-dual.h (right):

http://codereview.appspot.com/87043/diff/1/46#newcode52
Line 52: virtual double CalcSinrDb (Ptr<Packet> pkt,
Doxygen?

http://codereview.appspot.com/87043/diff/1/46#newcode83
Line 83: virtual void SendPacket (Ptr<Packet> pkt, uint32_t modeNum);
Doxygen?

Others below.

http://codereview.appspot.com/87043/diff/1/47
File src/devices/uan/uan-phy-gen.cc (right):

http://codereview.appspot.com/87043/diff/1/47#newcode134
Line 134: if (mode.GetModType () != UanTxMode::FSK)
coding std says braces needed

http://codereview.appspot.com/87043/diff/1/47#newcode267
Line 267: UanPhyPerUmodem::nchoosek (uint32_t n, uint32_t k)
Another class (don't remember which) called this NchooseK. Should
really be consistent.

http://codereview.appspot.com/87043/diff/1/47#newcode663
Line 663: UanPhyGen::GetCcaThresholdDb (void)
Inconsistent use of const throughout.

We all need to pay better attention to this kind of stuff.

http://codereview.appspot.com/87043/diff/1/47#newcode714
Line 714: if (m_pktRx)
coding std says braces needed

http://codereview.appspot.com/87043/diff/1/47#newcode721
Line 721: if (m_state == CCABUSY && GetInterferenceDb ( (Ptr<Packet>) 0)
< m_ccaThreshDb)
c-style cast with smart pointer?

http://codereview.appspot.com/87043/diff/1/48
File src/devices/uan/uan-phy-gen.h (right):

http://codereview.appspot.com/87043/diff/1/48#newcode49
Line 49: virtual double CalcPer (Ptr<Packet> pkt, double sinrDb,
UanTxMode mode);
Doxygen?

http://codereview.appspot.com/87043/diff/1/48#newcode68
Line 68: virtual double CalcPer (Ptr<Packet> pkt, double sinrDb,
UanTxMode mode);
Doxygen?

http://codereview.appspot.com/87043/diff/1/48#newcode70
Line 70: double nchoosek (uint32_t n, uint32_t k);
NchooseK in other places. Should be consistent.

http://codereview.appspot.com/87043/diff/1/48#newcode84
Line 84: virtual double CalcSinrDb (Ptr<Packet> pkt,
Doxygen?

http://codereview.appspot.com/87043/diff/1/48#newcode131
Line 131: class UanPhyGen : public ns3::UanPhy
Already in ns3 namespace?

http://codereview.appspot.com/87043/diff/1/1046
File src/devices/uan/uan-phy.h (right):

http://codereview.appspot.com/87043/diff/1/1046#newcode44
Line 44: class UanPhyCalcSinr : public ns3::Object
Already in ns3 namespace

http://codereview.appspot.com/87043/diff/1/1046#newcode65
Line 65: inline double DbToKp (double db) const {return pow(10,
db/10.0);}
Doxygen?

http://codereview.appspot.com/87043/diff/1/1046#newcode116
Line 116: * arg1: packet received successfully
Are you describing the signature of the callback? Not clear

http://codereview.appspot.com/87043/diff/1/1047
File src/devices/uan/uan-prop-model-bh.cc (right):

http://codereview.appspot.com/87043/diff/1/1047#newcode90
Line 90: if (line.empty () || line.at (0) == '#') //comment
coding standard sez braces needed

http://codereview.appspot.com/87043/diff/1/1047#newcode184
Line 184: else
need braces

http://codereview.appspot.com/87043/diff/1/51
File src/devices/uan/uan-prop-model-bh.h (right):

http://codereview.appspot.com/87043/diff/1/51#newcode46
Line 46: class BhConfig
Needs Doxygen

http://codereview.appspot.com/87043/diff/1/53
File src/devices/uan/uan-prop-model-thorp.cc (right):

http://codereview.appspot.com/87043/diff/1/53#newcode37
Line 37: // TODO Auto-generated constructor stub
These comments make me crazy. What is not done? What are the
implications? Is there some non-obvious reason i should be concerned?

http://codereview.appspot.com/87043/diff/1/1054
File src/devices/uan/uan-prop-model.h (right):

http://codereview.appspot.com/87043/diff/1/1054#newcode86
Line 86: class UanPdp {
Doxygen, doxygen, doxygen.

http://codereview.appspot.com/87043/diff/1/1056
File src/devices/uan/uan-test.cc (right):

http://codereview.appspot.com/87043/diff/1/1056#newcode160
Line 160: if (DoOnePhyTest (Seconds (1.0), Seconds (3.001), 50, 50,
prop) != 34)
Mixed indentations. Should be indented by two spaces per gnu.

http://codereview.appspot.com/87043/diff/1/59
File src/devices/uan/uan-transducer-hd.h (right):

http://codereview.appspot.com/87043/diff/1/59#newcode38
Line 38: class UanTransducerHd : public ns3::UanTransducer
Sorry to harp on this, but needs more Doxygen.

http://codereview.appspot.com/87043/diff/1/60
File src/devices/uan/uan-transducer.h (right):

http://codereview.appspot.com/87043/diff/1/60#newcode56
Line 56:
no destructor or dispose in the presence of smart pointers always makes
me nervous. Are you sure everything is closed down properly?

http://codereview.appspot.com/87043/diff/1/62
File src/devices/uan/uan-tx-mode.h (right):

http://codereview.appspot.com/87043/diff/1/62#newcode46
Line 46: typedef enum {PSK, QAM, FSK, OTHER} ModulationType;
Doxygen

http://codereview.appspot.com/87043/diff/1/63
File src/devices/uan/uan.h (right):

http://codereview.appspot.com/87043/diff/1/63#newcode5
Line 5: * \section UanOverview UAN Module Overview
Module Doxygen! I feel joy!

http://codereview.appspot.com/87043/diff/1/1064
File src/helper/uan-helper.cc (right):

http://codereview.appspot.com/87043/diff/1/1064#newcode204
Line 204: UanHelper::Install (NodeContainer c, Ptr<UanChannel> channel)
const
Ideally should have a version that takes a name and tranlates using name
service.

http://codereview.appspot.com/87043/diff/1/1065
File src/helper/uan-helper.h (right):

http://codereview.appspot.com/87043/diff/1/1065#newcode151
Line 151: // static void EnablePcap (std::string filename, uint32_t
nodeid, uint32_t deviceid);
there seems to be a couple of the common variations missing.

Specify net device by Ptr<>, specify net device by name of device
(translated by name service).

Do you need a promiscuous bit?

http://codereview.appspot.com/87043