LTP final code review

The current ltp protocol |ns3| implementation supports the following features:

* Basic data transmission between two nodes.
* Retransmission based reliability.
* Notices to client service instance.
* Udp Convergence layer adapter with link state cue support.
* Static Ltp engine id to IP address conversion.
* All types of Ltp Headers and Content Headers.
* Self-Delimiting Numeric Values (SDNV) support in headers.

The implementation has the following limitations:

* Only supports one concurrent transmission session per destination.
* Only a single timer of each type can be run concurrently in a single session (only a checkpoint or report can be handled at the same time).
* Segment reception offsets are assumed to be ordered.
* Upon loss of a red data segment, the whole red part is retransmitted, not just the specific segment.
* Checkpoint and Red Data Segment retransmission are handled together

The LTP model DOES NOT support the following features:

* Asynchronous checkpoints.
* Cancellation Requests.
* Handling of System Error conditions.
* Management Information Base, containing times of operation of remote ltp engines.
* LTP security and authentication extensions.

[Tom Henderson, 2014-08-21 14:58:21]

First set of comments (on .rst documentation, examples, tests, and helper).

Most are minor, but suggested extension to the helper API.

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/doc/ltp-prot...
File src/ltp-protocol/doc/ltp-protocol.rst (right):

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/doc/ltp-prot...
src/ltp-protocol/doc/ltp-protocol.rst:105: * Checkpoint and Red Data Segment
retransmission are handled together.
* When run over IP, and when the relevant IP interface has several addresses,
only the primary IP address of an underlying IP interface is looked up in the
address resolution process of the LTP helper

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/doc/ltp-prot...
src/ltp-protocol/doc/ltp-protocol.rst:135: This method creates a new
transmission session, unically identified by a session ID, and session state
record to keep track
uniquely

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/doc/ltp-prot...
src/ltp-protocol/doc/ltp-protocol.rst:153: 3. GetRoute(): 
``ns3::ltp::LtpIpResolutionTable::GetRoute (uint64_t ltpEngineId)`` acquire the
corresponding Ip ddress binded to a LTP
s/acquire the corresponding Ip ddress binded/method acquires the corresponding
IP address bound/

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/doc/ltp-prot...
src/ltp-protocol/doc/ltp-protocol.rst:191: * "LocalProcessingDelays": Define the
interval of time required for processing operations (queueing/dequeing).
Defines

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/doc/ltp-prot...
src/ltp-protocol/doc/ltp-protocol.rst:192: * "OneWayLightTime": Defines the time
required for transmitted data to reach the destination.
Provides an estimate of the time required ...  (upper bound?  average?  --
please clarify)

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/doc/ltp-prot...
src/ltp-protocol/doc/ltp-protocol.rst:194: LtpConvergenceLayerAdapter most
relevant attribute:
s/relevant/significant

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/doc/ltp-prot...
src/ltp-protocol/doc/ltp-protocol.rst:215: nodes, but it  additionally creates
links between the nodes and the correspondent
s/links/a single link/ ??  Do you want to say more about the nature of this
link?  It is completely setting up a session association with UDP convergence
layer adapters between two LTP engines, which may be multiple IP hops away.

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/doc/ltp-prot...
src/ltp-protocol/doc/ltp-protocol.rst:223: to set up a network topology.
Now there are two example scripts.

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/doc/ltp-prot...
src/ltp-protocol/doc/ltp-protocol.rst:231: Trace Sources are provided in the
form of Client Service Notice. The callback
s/in the form of Client Service Notice./returning Client Service notifications./

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/doc/ltp-prot...
src/ltp-protocol/doc/ltp-protocol.rst:267: data, while forces data corruption
using an ErrorModel. The different test-cases check the
s/while forces/while forcing/

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/examples/ltp...
File src/ltp-protocol/examples/ltp-protocol-example.cc (right):

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/examples/ltp...
src/ltp-protocol/examples/ltp-protocol-example.cc:111: cmd.AddValue ("verbose",
"Tell application to log if true", verbose);
is 'verbose' used?

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/examples/ltp...
src/ltp-protocol/examples/ltp-protocol-example.cc:135: // Define the
ClientService ID Code of the Client Service Instance that will be using the Ltp
protocol.
clarify that the same ID can be used on both sides of the link

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/examples/ltp...
src/ltp-protocol/examples/ltp-protocol-example.cc:145: "RetransCyclelimit", 
UintegerValue(20));
make indentation uniform

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/examples/ltp...
src/ltp-protocol/examples/ltp-protocol-example.cc:162: nodes.Get
(0)->GetObject<LtpProtocol> ()->StartTransmission
(ClientServiceId,ClientServiceId,receiverLtpId,data3,1500);
it would be nice to add an option (default on?) for pcap tracing, which can be
viewed in Wireshark.

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/helper/ltp-p...
File src/ltp-protocol/helper/ltp-protocol-helper.cc (right):

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/helper/ltp-p...
src/ltp-protocol/helper/ltp-protocol-helper.cc:150:
m_routingProtocol->AddBinding (m_ltpid++,ipv4->GetAddress (1,0).GetLocal
(),port.Get ());
this is where this helper will be fragile, in the case where nodes have multiple
interfaces and multiple addresses.

I think that there is no way around being more explicit in the helper API, to
pass in a reference to exactly which IP address or interface that you want to
bind to.  

So, perhaps InstallAndLink(NodeContainer c) will need to be restricted to the
use case in which all of the nodes have single IP interfaces, and perhaps you
will need to add something like:

InstallAndLink (Ptr<Node> n1, Ipv4Address a1, Ptr<Node> n2, Ipv4Address a2);
InstallAndLink (Ptr<Node> n1, Ipv6Address a1, Ptr<Node> n2, Ipv6Address a2);

to handle the case in which nodes have multiple interfaces (and restricted to
just creating a single link between two nodes).

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/helper/ltp-p...
src/ltp-protocol/helper/ltp-protocol-helper.cc:155:
m_routingProtocol->AddBinding (m_ltpid++,ipv6->GetAddress (1,0).GetAddress
(),port.Get ());
consider to move the m_ltpid++ increment outside of here to a separate
statement, to improve readability  (I was scanning the file to try to find
"where is m_ltpid incremented?")

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/helper/ltp-p...
File src/ltp-protocol/helper/ltp-protocol-helper.h (right):

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/helper/ltp-p...
src/ltp-protocol/helper/ltp-protocol-helper.h:44: * also create links between
the nodes.
again, define better what you mean by a 'link' in this context.

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/helper/ltp-p...
src/ltp-protocol/helper/ltp-protocol-helper.h:51: * \brief Create a LrWpan
helper in an empty state.
update this doxygen string

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/helper/ltp-p...
src/ltp-protocol/helper/ltp-protocol-helper.h:62: * it creates a point to point
links with the next node the container, the last one
it is not actually creating the point-to-point link, though.  I think it is
clearer to say "UDP transport association" or something like that.

Is there any use case for A<->B<->C<->D<->...<->A cycle of LTP links?

Please also state any prerequisites on this helper; e.g. if you haven't set a
routing protocol yet, it will terminate.

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/helper/ltp-p...
src/ltp-protocol/helper/ltp-protocol-helper.h:132: void Install (Ptr<Node>
node);
Usually we provide Install (Ptr<Node> node) as a public method, in addition to
the NodeContainer version.  This is nicer for users when working with single
nodes than requiring them to add single-node pointers to containers.  So I
suggest to make this method public, as well as the std::string nodename version
of Install().

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/test/ltp-pro...
File src/ltp-protocol/test/ltp-protocol-test-suite.cc (right):

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/test/ltp-pro...
src/ltp-protocol/test/ltp-protocol-test-suite.cc:186: m_extensions.Get
(0).m_expectedEncodedSz +                                                       
        // 12
are there tabs in these files?  suggest to run 'utils/check-style.py -l 3 -i -f
<this-file-name>' to clean up whitespace issues, or else align-justify these
comments such as '// 4' further leftward in the line.

[Tom Henderson, 2014-08-21 23:08:21]

some additional comments (ltp-protocol.h)

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/model/ltp-pr...
File src/ltp-protocol/model/ltp-protocol.h (right):

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/model/ltp-pr...
src/ltp-protocol/model/ltp-protocol.h:65: /* Default constructor */
not a Default

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/model/ltp-pr...
src/ltp-protocol/model/ltp-protocol.h:85: * \param srLtpEngine Source Ltp Engine
id.  (used by RED_PART_RCV and GP_SEGMENT_RCV)
srcLtpEngine

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/model/ltp-pr...
src/ltp-protocol/model/ltp-protocol.h:109: * \brief Add and active session id to
this client service instance.
Add an

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/model/ltp-pr...
src/ltp-protocol/model/ltp-protocol.h:130: std::vector<SessionId>
m_activeSessions; //!< Client Service Instance Active Sesssions
Sessions

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/model/ltp-pr...
src/ltp-protocol/model/ltp-protocol.h:155: *  - Protocol logic defining the
state machines for both sender a receiver.
sender and receiver

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/model/ltp-pr...
src/ltp-protocol/model/ltp-protocol.h:173: virtual TypeId GetInstanceTypeId
(void) const;
this method is typically used for run-time type checking, is it needed by this
class?

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/model/ltp-pr...
src/ltp-protocol/model/ltp-protocol.h:177: /* \brief Must be called by clients
to register their uint64_t, this
register their client service ID

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/model/ltp-pr...
src/ltp-protocol/model/ltp-protocol.h:198: * \param rdSize Size of red part
(data that needs to be sent in a reliable manner).
just curious why rdSize needs to be 64 bits, does it support such large values?

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/model/ltp-pr...
src/ltp-protocol/model/ltp-protocol.h:213: * \brief Send buffered data
Can you add more detail here?  Is user expected to call both StartTransmission()
and Send()?

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/model/ltp-pr...
src/ltp-protocol/model/ltp-protocol.h:225: /* Getters Methods */
I would remove all of these getters and setters as they are redundant with the
generic Get() and Set() methods for attributes.  The only reason to provide them
is if there are pre/post modifications to values, or if it is needed for
syntactic sugar for frequent usage.

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/model/ltp-pr...
src/ltp-protocol/model/ltp-protocol.h:261: Ptr<LtpConvergenceLayerAdapter>
GetDatalink (uint64_t engineId);
why is this "GetDatalink" instead of "GetConvergenceLayerAdapter"?

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/model/ltp-pr...
src/ltp-protocol/model/ltp-protocol.h:267: bool IsActive (Time t);
doxygen is unclear.  Is Time t an interval (length) of time starting at Now, or
a time point in simulation time?

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/model/ltp-pr...
src/ltp-protocol/model/ltp-protocol.h:280: * \param rprot Ltp Ip resolution
table.
why is this called 'rprot' instead of something like 'table'?

Why is this not part of the UdpConvergenceLayerAdapter?  The IP resolution table
only pertains to when running over an IP convergence layer.

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/model/ltp-pr...
src/ltp-protocol/model/ltp-protocol.h:288: void SetLinkStateCues
(Ptr<LtpConvergenceLayerAdapter> link);
should it be 'EnableLinkStateCues()'?

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/model/ltp-pr...
src/ltp-protocol/model/ltp-protocol.h:347: void ReportSegmentAckTransmission
(SessionId id, uint64_t rpSerialNum, Ptr<LtpConvergenceLayerAdapter>);
Ptr<LtpConvergenceLayerAdapter> cla);

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/model/ltp-pr...
src/ltp-protocol/model/ltp-protocol.h:405: // LtpEngineConfigParameters
m_confParams;               //!<  Protocol configuration parameters
can the above be deleted?

[bpswenson, 2014-08-24 17:22:59]

patch applied cleanly to ns-3-dev

Compile error
Missing file. Don't see it created in patch.  Need to be added to hg?  Not
seeing it in a wscript.

[1488/1970] cxx: src/lte/model/lte-anr.cc -> build/src/lte/model/lte-anr.cc.1.o
../src/ltp-protocol/model/ltp-header.cc:22:22: fatal error: ns3/sdnv.h: No such
file or directory
 #include "ns3/sdnv.h"
                      ^
compilation terminated.

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/examples/ltp...
File src/ltp-protocol/examples/ltp-protocol-example.cc (right):

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/examples/ltp...
src/ltp-protocol/examples/ltp-protocol-example.cc:54: std::cout <<
"ClientServiceNotification - Session ID: " << id.GetSessionNumber () << " Code :
" << code << std::endl;
Probably want to switch these couts to NS_LOG_INFO and enable with the verbose
parameter that currently doesn't appear to be used.

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/helper/ltp-p...
File src/ltp-protocol/helper/ltp-protocol-helper.cc (right):

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/helper/ltp-p...
src/ltp-protocol/helper/ltp-protocol-helper.cc:219: void
LtpProtocolHelper::SetBaseLtpEngineId (uint64_t id)
For consistency purposes please either stick with

void
LtpProtocolHelper::

or 

void LtpProtocolHelper

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/model/ltp-se...
File src/ltp-protocol/model/ltp-session-state-record.cc (right):

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/model/ltp-se...
src/ltp-protocol/model/ltp-session-state-record.cc:542:
SessionStateRecord::GetClaimsUpperBound (uint32_t serialNum)
const method?

https://codereview.appspot.com/123190043/diff/1/src/ltp-protocol/model/ltp-se...
src/ltp-protocol/model/ltp-session-state-record.cc:559:
SessionStateRecord::GetClaimsLowerBound (uint32_t serialNum)
const method?