Issue 13831049: Epidemic routing protocol addition for review

Peter Barnes

Link to the paper: http://issg.cs.duke.edu/epidemic/epidemic.pdf

11 years, 7 months ago (2013-10-02 19:46:30 UTC) #1

Peter Barnes

In addition to the inline comments, this module needs a Models chapter and tests (what's ...

11 years, 7 months ago (2013-10-03 01:05:17 UTC) #2

mjf.alenazi

Thank you very much for your comments. I will try to fix them all as ...

11 years, 7 months ago (2013-10-03 21:28:11 UTC) #3

Thank you very much for your comments.
I will try to fix them all as soon as I can (possibly within two weeks).





On Wed, Oct 2, 2013 at 8:05 PM, <pdbj@mac.com> wrote:

> In addition to the inline comments, this module needs a Models chapter
> and tests (what's here is just the templates)
>
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/examples/epidemic-**example.cc<https://codereview.appspot.com/13831049/diff/1/src/epidemic/examples/epidemic-example.cc>
> File src/epidemic/examples/**epidemic-example.cc (right):
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/examples/epidemic-**example.cc#newcode22<https://codereview.appspot.com/13831049/diff/1/src/epidemic/examples/epidemic-example.cc#newcode22>
> src/epidemic/examples/**epidemic-example.cc:22:
> Some comments outlining the example model would be really helpful here.
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**packet-queue.cc<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-packet-queue.cc>
> File src/epidemic/model/epidemic-**packet-queue.cc (right):
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**packet-queue.cc#newcode51<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-packet-queue.cc#newcode51>
> src/epidemic/model/epidemic-**packet-queue.cc:51: if (m_queue.size () >=
> m_maxLen)
> Why not Purge() before checking m_queue.size?
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**packet-queue.cc#newcode70<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-packet-queue.cc#newcode70>
> src/epidemic/model/epidemic-**packet-queue.cc:70: std::vector<uint64_t>
> sm;
> This will be much faster if you create with the known size, GetSize()
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**packet-queue.cc#newcode83<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-packet-queue.cc#newcode83>
> src/epidemic/model/epidemic-**packet-queue.cc:83: std::vector<uint64_t>
> sm;
> Reserve the known maximum size, GetSize()
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**packet-queue.cc#newcode104<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-packet-queue.cc#newcode104>
> src/epidemic/model/epidemic-**packet-queue.cc:104: for
> (std::vector<QueueEntry>::**iterator i = m_queue.begin (); i !=
> m_queue.end (); ++i)
> Why is this a loop?  It executes 0 or 1 times, depending on
> m_queue.begin() != m_queue.end
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**packet-queue.cc#newcode134<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-packet-queue.cc#newcode134>
> src/epidemic/model/epidemic-**packet-queue.cc:134: bool
> PacketQueue::IsPresent (uint64_t packetID)
> Implement this in terms of Find()
> Is this function even used?
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**packet-queue.h<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-packet-queue.h>
> File src/epidemic/model/epidemic-**packet-queue.h (right):
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**packet-queue.h#newcode182<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-packet-queue.h#newcode182>
> src/epidemic/model/epidemic-**packet-queue.h:182: std::vector<QueueEntry>
> m_queue;
> Is a vector the best choice here?  You treat it like a LRU queue, adding
> new entries at the back, and expiring entries from the front.  Expiring
> from the front is expensive in a vector.  std::deque might be a better
> choice.
>
> The other access pattern you have is in Find, where you search for a
> packetId.  That suggests a map<packetID, QueueEntry> might be the best.
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**packet.cc<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-packet.cc>
> File src/epidemic/model/epidemic-**packet.cc (right):
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**packet.cc#newcode183<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-packet.cc#newcode183>
> src/epidemic/model/epidemic-**packet.cc:183: uint64_t sm_length =
> i.ReadNtohU64 ();
> Call m_summary_vector.reserve (sm_length) before the loop
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**packet.h<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-packet.h>
> File src/epidemic/model/epidemic-**packet.h (right):
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**packet.h#newcode66<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-packet.h#newcode66>
> src/epidemic/model/epidemic-**packet.h:66: void SetInstanceTypeId
> (MessageType type);
> This sets the message type. The name associates it with
> GetInstanceTypeId, which is totally different.  Suggest renaming this
> method to SetMessageType
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**packet.h#newcode85<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-packet.h#newcode85>
> src/epidemic/model/epidemic-**packet.h:85: bool m_valid;
> Rather than increasing the size of this class, you can add a
> EPIDEMIC_TYPE_INVALID, then use that to determine if you deserialize
> correctly.
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**packet.h#newcode204<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-packet.h#newcode204>
> src/epidemic/model/epidemic-**packet.h:204: uint32_t m_floodCount;   //
> Count to keep track of number of traveled hops
> Is this the same as the hop count in the paper?  If you have to change
> the name, please document the correspondence.
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.cc<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.cc>
> File src/epidemic/model/epidemic-**routing-protocol.cc (right):
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.cc#newcode60<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.cc#newcode60>
> src/epidemic/model/epidemic-**routing-protocol.cc:60: const uint32_t
> RoutingProtocol::EPIDEMIC_PORT = 269;
> This if from RFC 5498, right?  Please document that.
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.cc#newcode148<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.cc#newcode148>
> src/epidemic/model/epidemic-**routing-protocol.cc:148: int32_t m_oif;
> More descriptive variable name, please.
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.cc#newcode183<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.cc#newcode183>
> src/epidemic/model/epidemic-**routing-protocol.cc:183: }
> Is this implementation difficult?  If not, please provide it.
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.cc#newcode246<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.cc#newcode246>
> src/epidemic/model/epidemic-**routing-protocol.cc:246: header.SetSource
> (header.GetSource ());
> Isn't this a noop?
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.cc#newcode247<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.cc#newcode247>
> src/epidemic/model/epidemic-**routing-protocol.cc:247: header.SetTtl
> (header.GetTtl () + 1);
> Why are you incrementing?  To avoid it getting decremented to zero and
> the packet dropped?  A comment would be helpful.
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.cc#newcode384<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.cc#newcode384>
> src/epidemic/model/epidemic-**routing-protocol.cc:384: bool
> RoutingProtocol::RouteInput (Ptr<const Packet> p,const Ipv4Header
> &header,Ptr<const NetDevice> idev,UnicastForwardCallback
> ucb,MulticastForwardCallback mcb,LocalDeliverCallback lcb,ErrorCallback
> ecb)
> Need some line breaks.
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.cc#newcode387<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.cc#newcode387>
> src/epidemic/model/epidemic-**routing-protocol.cc:387: if (p->GetSize () <
> 8 )
> What is this magic number?
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.cc#newcode405<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.cc#newcode405>
> src/epidemic/model/epidemic-**routing-protocol.cc:405: NS_LOG_DEBUG ("No
> route to forward broadcast. Drop packet " << p->GetUid ());
> Shouldn't the message be "TTL expired"?
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.cc#newcode470<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.cc#newcode470>
> src/epidemic/model/epidemic-**routing-protocol.cc:470: //Reset the counter
> if it reaches the max value 2^32 -1
> m_dataPacketCounter is uint32_t, so it will roll over automatically.
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.cc#newcode476<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.cc#newcode476>
> src/epidemic/model/epidemic-**routing-protocol.cc:476: uint64_t
> global_packet_ID = header.GetSource ().Get ();
> Unnecessary.
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.cc#newcode483<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.cc#newcode483>
> src/epidemic/model/epidemic-**routing-protocol.cc:483: a =
> (uint32_t)((global_packet_ID & 0xFFFFFFFF00000000) >> 32);
> Isn't this just (uint32_t)pkt_id?
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.cc#newcode484<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.cc#newcode484>
> src/epidemic/model/epidemic-**routing-protocol.cc:484: b =
> (uint32_t)(global_packet_ID & 0xFFFFFFFF);
> Isn't this just m_dataPacketCounter?
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.cc#newcode521<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.cc#newcode521>
> src/epidemic/model/epidemic-**routing-protocol.cc:521: // Exist  the
> function to not add the packet to the queue since the flood count limit
> is reached
> "Exit"
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.cc#newcode579<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.cc#newcode579>
> src/epidemic/model/epidemic-**routing-protocol.cc:579: Ptr<WifiNetDevice>
> wifi = dev->GetObject<WifiNetDevice> ();
> Unused?
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.cc#newcode668<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.cc#newcode668>
> src/epidemic/model/epidemic-**routing-protocol.cc:668: QueueEntry newEntry
> = m_queue.Find (*i);
> You're traversing the queue twice, once in IsPresent(), then again in
> Find().  Instead, just call Find, then check for a non-empty QueueEntry.
>  (Or better, instead of returning QueueEntry explicitly, which requires
> a copy, return a Ptr<QueueEntry>, and check for non-null.)
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.h<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.h>
> File src/epidemic/model/epidemic-**routing-protocol.h (right):
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.h#newcode26<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.h#newcode26>
> src/epidemic/model/epidemic-**routing-protocol.h:26: * Work supported by
> King Saud University and the ITTC at The University of Kansas.
> Do we usually put funding attribution in the code?  Tom?
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.h#newcode64<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.h#newcode64>
> src/epidemic/model/epidemic-**routing-protocol.h:64: static const uint32_t
> EPIDEMIC_PORT;
> This const is public in the header, but only given a value in the .cc.
> Doesn't this mean it's unusable by anyone including this header? even
> though it's public? I think this should either be private, or given a
> value here.
>
>
https://codereview.appspot.**com/13831049/<https://codereview.appspot.com/138...
>



-- 
Mohammed Alenazi
Graduate Research Assistant
Information and Telecommunications Technology Center
University of Kansas
malenazi@ittc.ku.edu
http://www.ittc.ku.edu/~malenazi
+1 785 979 5358

Sign in to reply to this message.

jpgs_ittc.ku.edu

On 3 Oct 2013, at 16:28, Mohammed Alenazi wrote: > Thank you very much for ...

11 years, 7 months ago (2013-10-04 16:41:50 UTC) #4

tomh_tomh.org

On 10/04/2013 09:41 AM, James P.G. Sterbenz wrote: > > On 3 Oct 2013, at ...

11 years, 6 months ago (2013-10-09 19:52:09 UTC) #5

mjf.alenazi

On Wed, Oct 2, 2013 at 8:05 PM, <pdbj@mac.com> wrote: > In addition to the ...

11 years, 6 months ago (2013-10-14 19:48:46 UTC) #6

On Wed, Oct 2, 2013 at 8:05 PM, <pdbj@mac.com> wrote:

> In addition to the inline comments, this module needs a Models chapter
> and tests (what's here is just the templates)
>
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/examples/epidemic-**example.cc<https://codereview.appspot.com/13831049/diff/1/src/epidemic/examples/epidemic-example.cc>
> File src/epidemic/examples/**epidemic-example.cc (right):
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/examples/epidemic-**example.cc#newcode22<https://codereview.appspot.com/13831049/diff/1/src/epidemic/examples/epidemic-example.cc#newcode22>
> src/epidemic/examples/**epidemic-example.cc:22:
> Some comments outlining the example model would be really helpful here.
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**packet-queue.cc<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-packet-queue.cc>
> File src/epidemic/model/epidemic-**packet-queue.cc (right):
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**packet-queue.cc#newcode51<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-packet-queue.cc#newcode51>
> src/epidemic/model/epidemic-**packet-queue.cc:51: if (m_queue.size () >=
> m_maxLen)
> Why not Purge() before checking m_queue.size?
>
>
Fixed

> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**packet-queue.cc#newcode70<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-packet-queue.cc#newcode70>
> src/epidemic/model/epidemic-**packet-queue.cc:70: std::vector<uint64_t>
> sm;
> This will be much faster if you create with the known size, GetSize()
>
>
Fixed

> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**packet-queue.cc#newcode83<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-packet-queue.cc#newcode83>
> src/epidemic/model/epidemic-**packet-queue.cc:83: std::vector<uint64_t>
> sm;
> Reserve the known maximum size, GetSize()
>

Fixed

>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**packet-queue.cc#newcode104<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-packet-queue.cc#newcode104>
> src/epidemic/model/epidemic-**packet-queue.cc:104: for
> (std::vector<QueueEntry>::**iterator i = m_queue.begin (); i !=
> m_queue.end (); ++i)
> Why is this a loop?  It executes 0 or 1 times, depending on
> m_queue.begin() != m_queue.end
>
>
Fixed

> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**packet-queue.cc#newcode134<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-packet-queue.cc#newcode134>
> src/epidemic/model/epidemic-**packet-queue.cc:134: bool
> PacketQueue::IsPresent (uint64_t packetID)
> Implement this in terms of Find()
> Is this function even used?
>
This is removed

>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**packet-queue.h<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-packet-queue.h>
> File src/epidemic/model/epidemic-**packet-queue.h (right):
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**packet-queue.h#newcode182<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-packet-queue.h#newcode182>
> src/epidemic/model/epidemic-**packet-queue.h:182: std::vector<QueueEntry>
> m_queue;
> Is a vector the best choice here?  You treat it like a LRU queue, adding
> new entries at the back, and expiring entries from the front.  Expiring
> from the front is expensive in a vector.  std::deque might be a better
> choice.
>
I am not an expert in C++ but I need to search the queue sometime and I
need a vector because I know it works for searching. I have not tried or
worked with LRU before


>
> The other access pattern you have is in Find, where you search for a
> packetId.  That suggests a map<packetID, QueueEntry> might be the best.
>

I do know know how to approach this .

>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**packet.cc<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-packet.cc>
> File src/epidemic/model/epidemic-**packet.cc (right):
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**packet.cc#newcode183<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-packet.cc#newcode183>
> src/epidemic/model/epidemic-**packet.cc:183: uint64_t sm_length =
> i.ReadNtohU64 ();
> Call m_summary_vector.reserve (sm_length) before the loop
>

Fixed

>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**packet.h<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-packet.h>
> File src/epidemic/model/epidemic-**packet.h (right):
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**packet.h#newcode66<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-packet.h#newcode66>
> src/epidemic/model/epidemic-**packet.h:66: void SetInstanceTypeId
> (MessageType type);
> This sets the message type. The name associates it with
> GetInstanceTypeId, which is totally different.  Suggest renaming this
> method to SetMessageType
>
I copied it from AODV implementation


>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**packet.h#newcode85<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-packet.h#newcode85>
> src/epidemic/model/epidemic-**packet.h:85: bool m_valid;
> Rather than increasing the size of this class, you can add a
> EPIDEMIC_TYPE_INVALID, then use that to determine if you deserialize
> correctly.
>

I followed AODV implementation.

>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**packet.h#newcode204<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-packet.h#newcode204>
> src/epidemic/model/epidemic-**packet.h:204: uint32_t m_floodCount;   //
> Count to keep track of number of traveled hops
> Is this the same as the hop count in the paper?  If you have to change
> the name, please document the correspondence.
>

Fixed

>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.cc<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.cc>
> File src/epidemic/model/epidemic-**routing-protocol.cc (right):
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.cc#newcode60<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.cc#newcode60>
> src/epidemic/model/epidemic-**routing-protocol.cc:60: const uint32_t
> RoutingProtocol::EPIDEMIC_PORT = 269;
> This if from RFC 5498, right?  Please document that.
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.cc#newcode148<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.cc#newcode148>
> src/epidemic/model/epidemic-**routing-protocol.cc:148: int32_t m_oif;
> More descriptive variable name, please.
>

Fixed

>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.cc#newcode183<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.cc#newcode183>
> src/epidemic/model/epidemic-**routing-protocol.cc:183: }
> Is this implementation difficult?  If not, please provide it.
>

No routing table for Epidemic

>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.cc#newcode246<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.cc#newcode246>
> src/epidemic/model/epidemic-**routing-protocol.cc:246: header.SetSource
> (header.GetSource ());
> Isn't this a noop?
>

Fixed

>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.cc#newcode247<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.cc#newcode247>
> src/epidemic/model/epidemic-**routing-protocol.cc:247: header.SetTtl
> (header.GetTtl () + 1);
> Why are you incrementing?  To avoid it getting decremented to zero and
> the packet dropped?  A comment would be helpful.
>

Fixed

>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.cc#newcode384<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.cc#newcode384>
> src/epidemic/model/epidemic-**routing-protocol.cc:384: bool
> RoutingProtocol::RouteInput (Ptr<const Packet> p,const Ipv4Header
> &header,Ptr<const NetDevice> idev,UnicastForwardCallback
> ucb,MulticastForwardCallback mcb,LocalDeliverCallback lcb,ErrorCallback
> ecb)
> Need some line breaks.
>
Fixed

>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.cc#newcode387<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.cc#newcode387>
> src/epidemic/model/epidemic-**routing-protocol.cc:387: if (p->GetSize () <
> 8 )
> What is this magic number?
>
:) I had it to avoid ICMP packets. Now it is removed.
Fixed

>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.cc#newcode405<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.cc#newcode405>
> src/epidemic/model/epidemic-**routing-protocol.cc:405: NS_LOG_DEBUG ("No
> route to forward broadcast. Drop packet " << p->GetUid ());
> Shouldn't the message be "TTL expired"?
>

Fixed

>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.cc#newcode470<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.cc#newcode470>
> src/epidemic/model/epidemic-**routing-protocol.cc:470: //Reset the counter
> if it reaches the max value 2^32 -1
> m_dataPacketCounter is uint32_t, so it will roll over automatically.
>

Fixed

>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.cc#newcode476<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.cc#newcode476>
> src/epidemic/model/epidemic-**routing-protocol.cc:476: uint64_t
> global_packet_ID = header.GetSource ().Get ();
> Unnecessary.
>

Fixed

>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.cc#newcode483<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.cc#newcode483>
> src/epidemic/model/epidemic-**routing-protocol.cc:483: a =
> (uint32_t)((global_packet_ID & 0xFFFFFFFF00000000) >> 32);
> Isn't this just (uint32_t)pkt_id?
>

Yes. This is just to check that the concatenating and splitting work. It is
commented anyway.

>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.cc#newcode484<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.cc#newcode484>
> src/epidemic/model/epidemic-**routing-protocol.cc:484: b =
> (uint32_t)(global_packet_ID & 0xFFFFFFFF);
> Isn't this just m_dataPacketCounter?
>

Yes for the same reason above

>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.cc#newcode521<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.cc#newcode521>
> src/epidemic/model/epidemic-**routing-protocol.cc:521: // Exist  the
> function to not add the packet to the queue since the flood count limit
> is reached
> "Exit"
>

fixed

>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.cc#newcode579<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.cc#newcode579>
> src/epidemic/model/epidemic-**routing-protocol.cc:579: Ptr<WifiNetDevice>
> wifi = dev->GetObject<WifiNetDevice> ();
> Unused?
>
fixed

>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.cc#newcode668<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.cc#newcode668>
> src/epidemic/model/epidemic-**routing-protocol.cc:668: QueueEntry newEntry
> = m_queue.Find (*i);
> You're traversing the queue twice, once in IsPresent(), then again in
> Find().  Instead, just call Find, then check for a non-empty QueueEntry.
>  (Or better, instead of returning QueueEntry explicitly, which requires
> a copy, return a Ptr<QueueEntry>, and check for non-null.)
>

Fixed

>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.h<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.h>
> File src/epidemic/model/epidemic-**routing-protocol.h (right):
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.h#newcode26<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.h#newcode26>
> src/epidemic/model/epidemic-**routing-protocol.h:26: * Work supported by
> King Saud University and the ITTC at The University of Kansas.
> Do we usually put funding attribution in the code?  Tom?
>
> https://codereview.appspot.**com/13831049/diff/1/src/**
>
epidemic/model/epidemic-**routing-protocol.h#newcode64<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-routing-protocol.h#newcode64>
> src/epidemic/model/epidemic-**routing-protocol.h:64: static const uint32_t
> EPIDEMIC_PORT;
> This const is public in the header, but only given a value in the .cc.
> Doesn't this mean it's unusable by anyone including this header? even
> though it's public? I think this should either be private, or given a
> value here.
>

Fixed

>
>
https://codereview.appspot.**com/13831049/<https://codereview.appspot.com/138...
>



-- 
Mohammed Alenazi
Graduate Research Assistant
Information and Telecommunications Technology Center
University of Kansas
malenazi@ittc.ku.edu
http://www.ittc.ku.edu/~malenazi
+1 785 979 5358

Sign in to reply to this message.

Peter Barnes

Still need docs and tests. You should really rethink the queue and summary vector implementations. ...

11 years, 6 months ago (2013-10-17 22:09:32 UTC) #7

Peter Barnes

On 2013/10/14 19:48:46, mjf.alenazi wrote: > > https://codereview.appspot.**com/13831049/diff/1/src/** > > > epidemic/model/epidemic-**packet-queue.h#newcode182<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-packet-queue.h#newcode182> > > src/epidemic/model/epidemic-**packet-queue.h:182: ...

11 years, 6 months ago (2013-10-17 22:58:48 UTC) #8

On 2013/10/14 19:48:46, mjf.alenazi wrote:
> > https://codereview.appspot.**com/13831049/diff/1/src/**
> >
>
epidemic/model/epidemic-**packet-queue.h#newcode182<https://codereview.appspot.com/13831049/diff/1/src/epidemic/model/epidemic-packet-queue.h#newcode182>
> > src/epidemic/model/epidemic-**packet-queue.h:182: std::vector<QueueEntry>
> > m_queue;
> > Is a vector the best choice here?  You treat it like a LRU queue, adding
> > new entries at the back, and expiring entries from the front.  Expiring
> > from the front is expensive in a vector.  std::deque might be a better
> > choice.
>
> I am not an expert in C++ but I need to search the queue sometime and I
> need a vector because I know it works for searching. I have not tried or
> worked with LRU before

You *are* working with LRU (least recently used) ;)  You can search a dequeu
just like a vector.  That's the beauty of STL containers.

> > The other access pattern you have is in Find, where you search for a
> > packetId.  That suggests a map<packetID, QueueEntry> might be the best.
> 
> I do know know how to approach this .

Take a look at std::map http://www.cplusplus.com/reference/map/map/

PacketQueue would have 

  std::map<packetID, QueueEntry> m_map;

In outline the key functions become (code not compiled or tested):

bool PacketQueue::Enqueue (QueueEntry & entry)
{
  // Add or update the entry
  m_map[GetPacketID ()] = entry;

  Purge (true);  // Drop oldest entry if we're over the limit
}

QueueEntry PacketQueue::Find (uint64_t packetID)
{
  std::map<packetID, QueueEntry>::iterator entry = m_map.find (packetID);
  if (entry != m_map.end ())
    {
      return *entry;
  } else {
      return QueueEntry ();
  }
}

struct IsEarlier
{
  bool
  operator() (QueueEntry const & e1, QueueEntry const & e2) const
  {
    return (e1.GetExpireTime () < e2.GetExpireTime ());
  }
};

void
PacketQueue::Purge (bool oldestOnly /* = false */)
{
  if (oldestOnly && m_map.size () > m_maxLen)
    {
        Drop (std::min_element (m_map.begin (), m_map.end (), IsEarlier),
                 "Drop the most aged packet");
    }
  else
    {
      for (std::vector<QueueEntry>::iterator i = m_queue.begin ();
            i != m_queue.end (); ++i)
        {
          if (i->GetExpireTime () < Seconds (0))
           {
             Drop (*i, "Drop outdated packet ");
           }
        }
    }
}

void PacketQueue::Drop (QueueEntry en, std::string reason)
{
  m_map.erase (en.GetPacketID ());
}


The map approach will give logarithmic search time for all accesses (summary
vector, expiring queue entries), compared to the current approach, which is
linear in all accesses (even expiring the oldest entry, since it forces a copy
of the entire vector).

Sign in to reply to this message.

Peter Barnes

Patch 2 doesn't build for me: Waf: Entering directory `epidemic/build' Waf: Leaving directory `epidemic/build' source ...

11 years, 6 months ago (2013-10-17 23:08:25 UTC) #9

mjf.alenazi

Thank you for the comments. I will work on fixing the code once I get ...

11 years, 6 months ago (2013-10-18 03:43:39 UTC) #10

barnes26_llnl.gov

On Thursday, October 17, 2013 8:43:37 PM UTC-7, Mohammed Alenazi wrote: > > The code ...

11 years, 6 months ago (2013-10-21 19:42:23 UTC) #11

mjf.alenazi

I will fix these in my end On Mon, Oct 21, 2013 at 2:42 PM, ...

11 years, 6 months ago (2013-10-22 02:58:57 UTC) #12

mjf.alenazi

I am in a process of changing the vector with the a map as suggested. ...

11 years, 6 months ago (2013-10-22 06:12:59 UTC) #13

I am in a process of changing the vector with the a map as suggested. I am
mostly done with it but i have an error that I could not fix in the in this
function
void PacketQueue::Purge (bool oldestOnly /* = false */)
{
  if (oldestOnly && m_map.size () > m_maxLen)
    {
        Drop(std::min_element(m_map.begin(), m_map.end(),IsEarlier),"Drop
the most aged packet");
    }
  else
    {

      for (std::map<uint64_t, QueueEntry>::iterator i = m_map.begin ();
            i != m_map.end (); ++i)
        {
          if (i->second.GetExpireTime () < Seconds (0))
           {
             Drop (i->second, "Drop outdated packet ");
           }
        }
    }
}


The error message is
malenazi@malenazi-DX4822:~/workspace/epidemic_routing/ns-3.17$ ./waf --run
scratch/epidemic-example
Waf: Entering directory
`/home/malenazi/workspace/epidemic_routing/ns-3.17/build'
[1135/1490] cxx: src/epidemic/model/epidemic-packet-queue.cc ->
build/src/epidemic/model/epidemic-packet-queue.cc.1.o
../src/epidemic/model/epidemic-packet-queue.cc: In member function ‘void
ns3::epidemic::PacketQueue::Purge(bool)’:
../src/epidemic/model/epidemic-packet-queue.cc:70:67: error: expected
primary-expression before ‘)’ token
Waf: Leaving directory
`/home/malenazi/workspace/epidemic_routing/ns-3.17/build'
Build failed
 -> task in 'ns3-epidemic' failed (exit status 1):
{task 53617168: cxx epidemic-packet-queue.cc ->
epidemic-packet-queue.cc.1.o}
['/usr/bin/g++', '-O0', '-ggdb', '-g3', '-Wall', '-Werror',
'-Wno-error=deprecated-declarations', '-fstrict-aliasing',
'-Wstrict-aliasing', '-fPIC', '-pthread', '-Isrc/epidemic',
'-I../src/epidemic', '-I.', '-I..', '-DNS3_ASSERT_ENABLE',
'-DNS3_LOG_ENABLE', '-DHAVE_SYS_IOCTL_H=1', '-DHAVE_IF_NETS_H=1',
'-DHAVE_PACKET_H=1', '-DHAVE_IF_TUN_H=1',
'../src/epidemic/model/epidemic-packet-queue.cc', '-c', '-o',
'src/epidemic/model/epidemic-packet-queue.cc.1.o']



If I comment the first drop statement, it works but without dropping.



On Mon, Oct 21, 2013 at 9:58 PM, Mohammed Alenazi <mjf.alenazi@gmail.com>wrote:

> I will fix these in my end
>
>
> On Mon, Oct 21, 2013 at 2:42 PM, Peter Barnes <barnes26@llnl.gov> wrote:
>
>>
>>
>> On Thursday, October 17, 2013 8:43:37 PM UTC-7, Mohammed Alenazi wrote:
>>>
>>> The code builds for me with no errors.
>>>
>>
>> To get this to build I have to:
>>
>>
>>    1. Edit src/epidemic/wscript to rename 'test/dsr-epidemic-suite.cc'
>>    to 'test/epidemic-test-suite.cc'
>>    2. Remove '#include "ns3/epidemic-.h"' from
>>    test/epidemic-test-suite.cc
>>
>>
>
>
> --
> Mohammed Alenazi
> Graduate Research Assistant
> Information and Telecommunications Technology Center
> University of Kansas
> malenazi@ittc.ku.edu
> http://www.ittc.ku.edu/~malenazi
> +1 785 979 5358
>



-- 
Mohammed Alenazi
Graduate Research Assistant
Information and Telecommunications Technology Center
University of Kansas
malenazi@ittc.ku.edu
http://www.ittc.ku.edu/~malenazi
+1 785 979 5358

Sign in to reply to this message.

Peter Barnes

On 2013/10/22 06:12:59, mjf.alenazi wrote: > I am in a process of changing the vector ...

11 years, 6 months ago (2013-10-22 19:43:33 UTC) #14

mjf.alenazi

I fixed almost all the issues (details are are below). Before I upload, I run ...

11 years, 5 months ago (2013-11-08 20:57:09 UTC) #15

I fixed almost all the issues (details are are below). Before I upload, I
run the style checker.

However, there is one issue. In line src/epidemic/model/epidemic-
packet-queue.cc:158

This statement is commented. Could you please uncomment it and tell me what
is the issue so i can fix it?
//Drop (std::min_element (m_map.begin (), m_map.end (), IsEarlier()),"Drop
the oldest packet");


On Thu, Oct 17, 2013 at 5:09 PM, <pdbj@mac.com> wrote:

> Still need docs and tests.
>
> You should really rethink the queue and summary vector implementations.
> I've made some comments in the code on the issues and possible
> solutions.
>
>
> https://codereview.appspot.com/13831049/diff/17001/
> scratch/epidemic-example.cc
> File scratch/epidemic-example.cc (right):
>
> https://codereview.appspot.com/13831049/diff/17001/
> scratch/epidemic-example.cc#newcode1
> scratch/epidemic-example.cc:1: #include "ns3/core-module.h"
> This file is duplicative of src/epidemic/examples/epidemic-example.cc.
> Patches shouldn't include files in scratch/
>
fixed

>
> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/doc/epidemic.rst
> File src/epidemic/doc/epidemic.rst (right):
>
> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/doc/epidemic.rst#newcode1
> src/epidemic/doc/epidemic.rst:1: Example Module Documentation
> Need module documentation.
>
Added basic file

>
> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/examples/epidemic-example.cc
> File src/epidemic/examples/epidemic-example.cc (right):
>
> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/examples/epidemic-example.cc#newcode17
> src/epidemic/examples/epidemic-example.cc:17:
> Please explain the model used in this example, and the expected output:
>
> /**
>   This example creates a 10-node wireless network ...
>
> */
>
> Fixed

> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/examples/epidemic-example.cc#newcode43
> src/epidemic/examples/epidemic-example.cc:43:
> //      LogComponentEnable("EpidemicRoutingProtocol", LOG_LEVEL_ALL);
> Please remove extraneous code, unless it illustrates an alternative
> process, in which case it needs an explanatory comment.
>
>
Fixed

> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/examples/epidemic-example.cc#newcode122
> src/epidemic/examples/epidemic-example.cc:122: //
> More extraneous code.
>
> Fixed

> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/examples/wscript
> File src/epidemic/examples/wscript (right):
>
> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/examples/wscript#newcode4
> src/epidemic/examples/wscript:4: obj =
> bld.create_ns3_program('epidemic-manet', ['epidemic'])
> Should this be 'epidemic-example'?
>
> Yes. Fixed.

> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/helper/epidemic-helper.h
> File src/epidemic/helper/epidemic-helper.h (right):
>
> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/helper/epidemic-helper.h#newcode15
> src/epidemic/helper/epidemic-helper.h:15: EpidemicHelper ();
> Need minimal doxygen:  /** Constructor */
>
> Fixed

> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/helper/epidemic-helper.h#newcode16
> src/epidemic/helper/epidemic-helper.h:16: ~EpidemicHelper ();
> Need minimal doxygen:  /** Destructor */
>
> Fixed

> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/helper/epidemic-helper.h#newcode19
> src/epidemic/helper/epidemic-helper.h:19: * \returns pointer to clone of
> this EpidemicHelper
> Reverse these two lines, so the function has a brief description.
>
> Fixed

> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/model/epidemic-packet-queue.cc
> File src/epidemic/model/epidemic-packet-queue.cc (right):
>
> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/model/epidemic-packet-queue.cc#newcode55
> src/epidemic/model/epidemic-packet-queue.cc:55: m_queue.erase
> (m_queue.begin ());
> Since erase is part of dropping, (and always called after Drop, just
> include erase in the body of Drop.
>
> The packet queue has been rewritten based on your outline and suggestions

> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/model/epidemic-packet-queue.cc#newcode71
> src/epidemic/model/epidemic-packet-queue.cc:71: std::vector<uint64_t> sm
> (m_queue.size ());
> Purge first?
>
> The packet queue has been rewritten based on your outline and suggestions

> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/model/epidemic-packet-queue.cc#newcode84
> src/epidemic/model/epidemic-packet-queue.cc:84: std::vector<uint64_t>
> current_node_sum_vec = GetSummaryVector ();
> Reverse these two lines, so Purge is called, but only once, before
> allocating sm.
>
> Fixed

> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/model/epidemic-packet-queue.cc#newcode88
> src/epidemic/model/epidemic-packet-queue.cc:88: if (!(std::find
> (list.begin (), list.end (), *i) != list.end ()))
> Isn't this just std::find() == list.end ()?
>
> The packet queue has been rewritten based on your outline and suggestions

> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/model/epidemic-packet-queue.cc#newcode154
> src/epidemic/model/epidemic-packet-queue.cc:154: m_queue.end ());
> Put the erase in the body of Drop.  It's particularly bad here since
> you're traversing the queue twice, once explicitly to call Drop(), then
> again to erase().
>
> The packet queue has been rewritten based on your outline and suggestions

> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/model/epidemic-packet-queue.cc#newcode161
> src/epidemic/model/epidemic-packet-queue.cc:161: return;
> Perhaps NS_LOG_INFO ("dropping " << en... << ", reason: " << reason);
>
> The packet queue has been rewritten based on your outline and suggestions

> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/model/epidemic-packet-queue.h
> File src/epidemic/model/epidemic-packet-queue.h (right):
>
> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/model/epidemic-packet-queue.h#newcode43
> src/epidemic/model/epidemic-packet-queue.h:43: typedef
> Ipv4RoutingProtocol::UnicastForwardCallback UnicastForwardCallback;
> Please give at least minimal doxygen for all functions, typdefs, enums,
> ...
>
> Fixed

> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/model/epidemic-packet-queue.h#newcode62
> src/epidemic/model/epidemic-packet-queue.h:62: {
> ns-3 coding standard discourages inline functions, unless you can show a
> significant performance issue.  Suggest you move these bodies to a .cc
> file, such as epidemic-packet-queue.cc.  In fact, can you just forward
> declare the QueueEntry class here, and put the entire class declaration
> in epidemic-packet-queue.cc?  (Looks like with a few more &'s that would
> work, and reduce the exposed implementation.
>
> The packet queue has been rewritten based on your outline and suggestions

> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/model/epidemic-packet.h
> File src/epidemic/model/epidemic-packet.h (right):
>
> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/model/epidemic-packet.h#newcode50
> src/epidemic/model/epidemic-packet.h:50: *      1. Beacon Packet: it is
> used to advertise the presence of a node in the network.
> Please use smaller indent, and wrap long lines, so this comment is
> legible.
>
> fixed

> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/model/epidemic-packet.h#newcode63
> src/epidemic/model/epidemic-packet.h:63: TypeHeader (MessageType t =
> EPIDEMIC_TYPE_BEACON);
> Doxygen needed everywhere.
>
> Fixed

> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/model/epidemic-packet.h#newcode74
> src/epidemic/model/epidemic-packet.h:74: {
> Please move all inline bodies to epidemic-packet.cc
>
> Fixed

> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/model/epidemic-packet.h#newcode102
> src/epidemic/model/epidemic-packet.h:102: EpidemicSummaryVectorHeader
> (std::vector<uint64_t> sv = std::vector<uint64_t> ());
> This design can be greatly improved.  As is, you're forced into copying
> vectors multiple times.  Instead, just inherit from std::vector, then
> have PacketQueue::GetSummaryVector/FindDisjointPackets assemble the
> EpidemicSummaryVectorHeader directly.
>
> Any suggestions

> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/model/epidemic-routing-protocol.cc
> File src/epidemic/model/epidemic-routing-protocol.cc (right):
>
> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/model/epidemic-routing-protocol.cc#newcode208
> src/epidemic/model/epidemic-routing-protocol.cc:208: Ptr<Socket> socket
> = j->first;
> Move this into the conditional
>
> Fixed

> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/model/epidemic-routing-protocol.cc#newcode243
> src/epidemic/model/epidemic-routing-protocol.cc:243: void
> RoutingProtocol::SendPacketFromQueue (Ipv4Address dst,QueueEntry
> queueEntry)
> Missing ' ' after ','.  Probably should run all the code through the
> style checker.
>
> Fixed

> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/model/epidemic-routing-protocol.cc#newcode434
> src/epidemic/model/epidemic-routing-protocol.cc:434: int32_t iif =
> m_ipv4->GetInterfaceForDevice (idev);
> Isnt' this a loop invariant?  So it can be done before the loop?
>
> no it depends on i

> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/model/epidemic-routing-protocol.cc#newcode470
> src/epidemic/model/epidemic-routing-protocol.cc:470:
> I can't understand this function below here.  Please fix the indentation
> and line lengths, remove dead code, as a start  Please describe the
> overall logic as you go, so I have a clue when each block fires and what
> it's supposed to do.
>
> Small reorganization

> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/model/epidemic-routing-protocol.cc#newcode668
>
> src/epidemic/model/epidemic-routing-protocol.cc:668: QueueEntry newEntry
> = m_queue.Find (*i);
> Wait, we've already walked my m_queue and the other side's summary
> vector to find the list of disjoint packets, now we have to walk the
> queue again?!?  Much better to have FindDisjointPackets return a list of
> iterators into the queue, then we don't have to do another Find here.
>
> The packet queue has been rewritten based on your outline and suggestions

> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/model/epidemic-routing-protocol.cc#newcode713
> src/epidemic/model/epidemic-routing-protocol.cc:713:
> header_summary.SetSummaryVector (my_vec);
> Aren't these two lines redundant with the header_summary initialization
> above?
>
> Yes. Fixed

> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/model/epidemic-routing-protocol.h
> File src/epidemic/model/epidemic-routing-protocol.h (right):
>
> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/model/epidemic-routing-protocol.h#newcode87
> src/epidemic/model/epidemic-routing-protocol.h:87: uint32_t
> m_floodCount;          //Number of times a packet is resent
> m_hopCount?
>
> Fixed

> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/model/epidemic-routing-protocol.h#newcode111
> src/epidemic/model/epidemic-routing-protocol.h:111: *  dest: destination
> address
> \param dest destination address
>
> fixed

> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/model/epidemic-routing-protocol.h#newcode112
> src/epidemic/model/epidemic-routing-protocol.h:112: *  firstNode: check
> the anti-entropy session in the Epidemic paper
> \param firstNode ...
>
> Fixed

> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/test/epidemic-test-suite.cc
> File src/epidemic/test/epidemic-test-suite.cc (right):
>
> https://codereview.appspot.com/13831049/diff/17001/src/
> epidemic/test/epidemic-test-suite.cc#newcode14
> src/epidemic/test/epidemic-test-suite.cc:14: class EpidemicTestCase1 :
> public TestCase
> Need to implement test cases.
>
> Fixed

> https://codereview.appspot.com/13831049/diff/17001/upload.py
> File upload.py (right):
>
> https://codereview.appspot.com/13831049/diff/17001/upload.py#newcode1
> upload.py:1: #!/usr/bin/env python
> This file doesn't belong in the patch.
>
> Fixed

> https://codereview.appspot.com/13831049/
>



-- 
Mohammed Alenazi
Graduate Research Assistant
Information and Telecommunications Technology Center
University of Kansas
malenazi@ittc.ku.edu
http://www.ittc.ku.edu/~malenazi
+1 785 979 5358

Sign in to reply to this message.

Peter Barnes

I'm traveling so I won't be able to try this until Tues at the earliest. ...

11 years, 5 months ago (2013-11-08 21:52:24 UTC) #16

Tom Henderson

The main issues that need to be resolved before merging to ns-3-dev are: - use ...

11 years, 4 months ago (2013-12-08 22:24:26 UTC) #17

Peter Barnes

A few comments from patch-2 are still unaddressed; I've brought them forward. Much doxygen is ...

11 years, 4 months ago (2013-12-10 00:02:32 UTC) #18

mjf.alenazi

I have uploaded a new patch fixing the issues presented. However, there is a runtime ...

11 years, 4 months ago (2013-12-30 23:11:10 UTC) #19

Peter Barnes

A few general comments: * Many of Tom's comments on patch3 have not been addressed, ...

11 years, 3 months ago (2014-01-07 23:57:08 UTC) #20

mjf.alenazi

I have run the style-check.py before submitting the code using this command find . -type ...

11 years, 3 months ago (2014-01-18 02:23:03 UTC) #21

I have run the style-check.py before submitting the code using this command 
 find . -type f -name "*.cc" -o -name "*.h" | xargs -I '{}'
../../utils/check-style.py -f '{}' -i $1 -l 3



For the error I mentioned earlier in src/epidemic/examples/epidemic-benchmark.cc
I forget to say that the error would appear if the queue length is large for
example 2000. I think the error appear because of the summary vector packet
length header becomes larger than allowed length i.e 2^16. I am just
speculating. Could someone please confirm that. If it's confirmed, what would be
a good solution for that?

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/doc/epidemic...
File src/epidemic/doc/epidemic.rst (right):

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/doc/epidemic...
src/epidemic/doc/epidemic.rst:14: `the ResiliNets research group
<http://www.ittc.ku.edu/resilinets>`_
On 2014/01/07 23:57:09, Peter Barnes wrote:
> the `ResiliNets...

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/doc/epidemic...
src/epidemic/doc/epidemic.rst:40: epidemic.Set ("BeaconInterval", UintegerValue
(1));
On 2014/01/07 23:57:09, Peter Barnes wrote:
> I think Tom is suggesting that you delete this code sample; all the parameters
> are better documented in the table which follows.  Besides, this same block
> appears later in the doc, so it's redundant here.
> On 2013/12/30 23:11:11, mjf.alenazi wrote:
> > On 2013/12/08 22:24:26, Tom Henderson wrote:
> > > 
> > > What are these statements included for?
> > 
> > Done.

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/doc/epidemic...
src/epidemic/doc/epidemic.rst:47: | HopCount              | Maximum number of
hops a packet   | 64            |
where exactly?
On 2014/01/07 23:57:09, Peter Barnes wrote:
> Please put this explanation in the docs.
> On 2013/12/30 23:11:11, mjf.alenazi wrote:
> > This does not depend on the TTL. I try to increment the TTL in each node so
> the
> > packet does not drop because of the TTL.
> > On 2013/12/08 22:24:26, Tom Henderson wrote:
> > > Can you explain somewhere how HopCount interacts with the IP TTL?  Is IP
TTL
> > > regenerated each hop?

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/doc/epidemic...
src/epidemic/doc/epidemic.rst:55: |                       | network-wide is
synchronization   |               |
On 2014/01/07 23:57:09, Peter Barnes wrote:
> … at the source.  Network …
> (Punctuate sentences.)

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/doc/epidemic...
src/epidemic/doc/epidemic.rst:60: |                       | added to avoid
collision          |               |
On 2014/01/07 23:57:09, Peter Barnes wrote:
> What is the random distribution?  Is this configurable?
> How much randomness is introduced?  Is this configurable?

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/doc/epidemic...
src/epidemic/doc/epidemic.rst:68: in your simulation script. For instance:::
On 2014/01/07 23:57:09, Peter Barnes wrote:
> Only 2 ':' 
> http://sphinx-doc.org/rest.html#source-code

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/doc/epidemic...
src/epidemic/doc/epidemic.rst:71: epidemic.Set ("HopCount", UintegerValue (64));
On 2014/01/07 23:57:09, Peter Barnes wrote:
> Suggest using non-default values for clarity.

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/doc/epidemic...
src/epidemic/doc/epidemic.rst:75: mainNodes.Install (epidemic, adhocNodes);
I really do not know. 
On 2014/01/07 23:57:09, Peter Barnes wrote:
> (Bump:  Tom's first Q not addressed)
> On 2013/12/08 22:24:26, Tom Henderson wrote:
> > Clarify, does Install() have to be called before or after the Internet stack
> is
> > added?

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/doc/epidemic...
src/epidemic/doc/epidemic.rst:77: The epidemic.Set() statements are needed
On 2014/01/07 23:57:09, Peter Barnes wrote:
> ``epidemic.Set ()``

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/doc/epidemic...
src/epidemic/doc/epidemic.rst:84: The example can be found in
``src/epidemic/examples/epidemic-example.cc``
On 2014/01/07 23:57:09, Peter Barnes wrote:
> It's helpful to say a bit here, so the reader knows what they might find
there.
> On 2013/12/30 23:11:11, mjf.alenazi wrote:
> > I explained these in the example itself.
> > On 2013/12/08 22:24:26, Tom Henderson wrote:
> > > Please say a bit more about this example; what does it do and what options
> > > does it provide to the user?

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/doc/epidemic...
src/epidemic/doc/epidemic.rst:96: **********
On 2014/01/07 23:57:09, Peter Barnes wrote:
> ***********
> (Underline has to be same length as section title text)

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/examples/epi...
File src/epidemic/examples/epidemic-benchmark.cc (right):

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/examples/epi...
src/epidemic/examples/epidemic-benchmark.cc:17: the default is set to 50 m.
There are several scenarios. The results are in the paper. I have not started
comparing them in details. I will once I fix large epidemic queue length
problem. It throws an error because I suspect that the the header size of the
summary vector packet exceeds 2^16. 
On 2014/01/07 23:57:09, Peter Barnes wrote:
> What is the expected output?  How does it compare to the paper?

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/examples/epi...
src/epidemic/examples/epidemic-benchmark.cc:48: cmd.AddValue ("nodeSpeed", "Node
speed in RandomWayPoint model, Default:20", nodeSpeed);
On 2014/01/07 23:57:09, Peter Barnes wrote:
> CommandLine will print the default value automatically in the message
generated
> by '--help'; no need to put the default in the help string.  (In fact, putting
> the default in the help string is undesirable: the default has to be updated
in
> two place. Notice that you already have this problem with nodeSpeed:  the
> default is 10, but your string says 20.)

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/examples/epi...
src/epidemic/examples/epidemic-benchmark.cc:57: cmd.Parse (argc,argv);
On 2014/01/07 23:57:09, Peter Barnes wrote:
> Need space ' ' after comma ','  Please fix this in all files.

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/examples/epi...
src/epidemic/examples/epidemic-benchmark.cc:65: * */
On 2014/01/07 23:57:09, Peter Barnes wrote:
> */ is sufficient.

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/examples/epi...
File src/epidemic/examples/epidemic-example.cc (right):

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/examples/epi...
src/epidemic/examples/epidemic-example.cc:5: #include
"ns3/config-store-module.h"
On 2014/01/07 23:57:09, Peter Barnes wrote:
> Well, is it?  Why?
> On 2013/12/30 23:11:11, mjf.alenazi wrote:
> > On 2013/12/08 22:24:26, Tom Henderson wrote:
> > > Is config-store-module.h needed?

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/examples/wsc...
File src/epidemic/examples/wscript (right):

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/examples/wsc...
src/epidemic/examples/wscript:6: 
On 2014/01/07 23:57:09, Peter Barnes wrote:
>     obj = bld.create_ns3_program('epidemic-benchmark', ['epidemic'])
>     obj.source = 'epidemic-benchmark.cc'

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/helper/epide...
File src/epidemic/helper/epidemic-helper.h (right):

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/helper/epide...
src/epidemic/helper/epidemic-helper.h:40: * \ingroup aodv
On 2014/01/07 23:57:09, Peter Barnes wrote:
> \ingroup epidemic

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/helper/epide...
src/epidemic/helper/epidemic-helper.h:40: * \ingroup aodv
On 2014/01/07 23:57:09, Peter Barnes wrote:
> \ingroup epidemic

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/helper/epide...
src/epidemic/helper/epidemic-helper.h:59: /**
On 2014/01/07 23:57:09, Peter Barnes wrote:
>    *  Add an epidemic::RoutingProtocol object to to a node.

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
File src/epidemic/model/epidemic-packet-queue.cc (right):

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-packet-queue.cc:41: namespace epidemic {
On 2014/01/07 23:57:09, Peter Barnes wrote:
> I would prefer "namespace Epidemic" as more in line with ns-3 naming
guidelines.

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-packet-queue.cc:177: if (i->second.GetExpireTime ()
< Seconds (0))
On 2014/01/07 23:57:09, Peter Barnes wrote:
> Shouldn't this be GetExpireTime () < Now ()?

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
File src/epidemic/model/epidemic-packet-queue.h (right):

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-packet-queue.h:43: class QueueEntry
I tried to move the QueueEntry class definition and implementation to
epidemic-packet-queue.cc but it generated many errors that I could not solve.
Then I did undo. If you can help me with that, I would appreciate it.
On 2014/01/07 23:57:09, Peter Barnes wrote:
> Yes, for exposed API.  Purely internal classes used in a single .cc file
> can/should be declared and defined in the .cc file.
> On 2013/12/30 23:11:11, mjf.alenazi wrote:
> > I thought you want the implementation in the .cc  files and the definitions
in
> > the .h files
> >
>
https://codereview.appspot.com/13831049/diff/17001/src/epidemic/model/epidemi...
> > 
> > 
> > On 2013/12/10 00:02:33, Peter Barnes wrote:
> > > Can you just forward declare the QueueEntry class here, and put the entire
> > class
> > > declaration in epidemic-packet-queue.cc?  (Looks like with a few more &'s
> that
> > > would work, and reduce the exposed implementation.)
> > 
>

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-packet-queue.h:47: typedef
Ipv4RoutingProtocol::UnicastForwardCallback UnicastForwardCallback;
What do you mean by private?
On 2014/01/07 23:57:09, Peter Barnes wrote:
> Then shouldn't they be private?
> On 2013/12/30 23:11:11, mjf.alenazi wrote:
> > They are used in the QueueEntry constructer.
> > On 2013/12/08 22:24:26, Tom Henderson wrote:
> > > what do these callbacks do?  They don't seem to be used anywhere.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-packet-queue.h:108: PacketQueue (uint64_t maxLen,
Time queueTimeout)
On 2014/01/07 23:57:09, Peter Barnes wrote:
> Not done.
> On 2013/12/30 23:11:11, mjf.alenazi wrote:
> > On 2013/12/08 22:24:26, Tom Henderson wrote:
> > > document parameters; what is "routeToQueueTimeout"?

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
File src/epidemic/model/epidemic-packet.cc (right):

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-packet.cc:49: TypeId TypeHeader::GetTypeId ()
On 2014/01/07 23:57:09, Peter Barnes wrote:
> TypeId 
> TypeHeader::GetTypeId ()
> 
> on all function definitions.
> On 2013/12/30 23:11:11, mjf.alenazi wrote:
> > I did not understand what you mean here?
> > On 2013/12/10 00:02:33, Peter Barnes wrote:
> > > I prefer return type on a separate line than the function name, as in the
> > > previous patch.

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
File src/epidemic/model/epidemic-packet.h (right):

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-packet.h:41: EPIDEMIC_TYPE_BEACON  = 1,    //!<
EPIDEMIC_TYPE_BEACON
It is not assigned in the paper. 
On 2014/01/07 23:57:09, Peter Barnes wrote:
> 1.  Not helpful to just repeat the symbol name.  What does the symbol *mean*?
> This would be easier if the enum was part of the epidemic::TypeHeader class:
> 
> EPIDEMIC_TYPE_BEACON  = 1,    //!< Beacon packet to advertise node presence.
> (Sorry, should have said this earlier.)
> 
> 2.  Is the value specified in the paper?  If it is, then specify it here as
you
> did "=1" to ensure interoperability.  If it's not in the paper, then let it
get
> auto-assigned:  
> EPIDEMIC_TYPE_BEACON,
> EPIDEMIC_TYPE_REPLY,
> EPIDEMIC_TYPE_REPLY_BACK
> }

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-packet.h:41: EPIDEMIC_TYPE_BEACON  = 1,    //!<
EPIDEMIC_TYPE_BEACON
On 2014/01/07 23:57:09, Peter Barnes wrote:
> 1.  Not helpful to just repeat the symbol name.  What does the symbol *mean*?
> This would be easier if the enum was part of the epidemic::TypeHeader class:
> 
> EPIDEMIC_TYPE_BEACON  = 1,    //!< Beacon packet to advertise node presence.
> (Sorry, should have said this earlier.)
> 
> 2.  Is the value specified in the paper?  If it is, then specify it here as
you
> did "=1" to ensure interoperability.  If it's not in the paper, then let it
get
> auto-assigned:  
> EPIDEMIC_TYPE_BEACON,
> EPIDEMIC_TYPE_REPLY,
> EPIDEMIC_TYPE_REPLY_BACK
> }

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-packet.h:42: EPIDEMIC_TYPE_REPLY  = 2,            
//!< EPIDEMIC_TYPE_REPLY
On 2014/01/07 23:57:09, Peter Barnes wrote:
> //!< Reply to a beacon, with the packet Id summary vector.

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-packet.h:43: EPIDEMIC_TYPE_REPLY_BACK  = 3,       
//!< EPIDEMIC_TYPE_REPLY_BACK
On 2014/01/07 23:57:09, Peter Barnes wrote:
> //!< Response to a Reply packet, with the list of disjoint packets.

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-packet.h:43: EPIDEMIC_TYPE_REPLY_BACK  = 3,       
//!< EPIDEMIC_TYPE_REPLY_BACK
On 2014/01/07 23:57:09, Peter Barnes wrote:
> //!< Response to a Reply packet, with the list of disjoint packets.

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-packet.h:50: * \brief  Epidemic routing packets come
in three types::
On 2014/01/07 23:57:09, Peter Barnes wrote:
> See previous comment on this line.  Your edit now includes the long
description
> in the brief summary.
> 
> (Also, single ':')

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-packet.h:68: * \brief Epidemic types
On 2014/01/07 23:57:09, Peter Barnes wrote:
> A single \brief, please, at the beginning of the comment.

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-packet.h:70: class TypeHeader : public Header
No, Type header is for identifying which header is used in the summary vector
exchange. Epidemic header is used in the data packets
On 2014/01/07 23:57:09, Peter Barnes wrote:
> Seems like TypeHeader and EpidemicHeader could/should be combined into one
> header.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-packet.h:94: void SetInstanceTypeId (MessageType
type);
On 2014/01/07 23:57:09, Peter Barnes wrote:
> SetMessageType ()

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-packet.h:114: * \return info about this packet
On 2014/01/07 23:57:09, Peter Barnes wrote:
> No return

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-packet.h:119: MessageType Get () const;
On 2014/01/07 23:57:09, Peter Barnes wrote:
> GetMessageType ()

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-packet.h:119: MessageType Get () const;
On 2014/01/07 23:57:09, Peter Barnes wrote:
> GetMessageType ()

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-packet.h:127: /// flag
In the TypeHeader::Deserialize fucntions
On 2014/01/07 23:57:09, Peter Barnes wrote:
> What defines "valid"?

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-packet.h:168: class EpidemicSummaryVectorHeader :
public Header
On 2014/01/07 23:57:09, Peter Barnes wrote:
> Since this is already in namespace ns3::epidemic, please rename this
> SummaryVectorHeader (without the redundant "Epidemic").  Even better would be
> something like PacketListHeader, which is more descriptive of its function.

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-packet.h:201: static inline std::ostream &
operator<< (std::ostream& os, const EpidemicSummaryVectorHeader & packet)
On 2014/01/07 23:57:09, Peter Barnes wrote:
> static std::ostream & operator<< (…)
> 
> Move the implementation to epidemic-packet.cc
> On 2013/12/30 23:11:11, mjf.alenazi wrote:
> > Followed the AODV example.
> > On 2013/12/08 22:24:26, Tom Henderson wrote:
> > > Why is this static and inline?  See e.g. Mac48Address where operator<< is 
> > > 
> > > std::ostream& operator<< (std::ostream& os, const Mac48Address & address);

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-packet.h:243: class EpidemicHeader : public Header
No, Type header is for identifying which header is used in the summary vector
exchange. Epidemic header is used in the data packets
On 2014/01/07 23:57:09, Peter Barnes wrote:
> Seems like TypeHeader and EpidemicHeader could/should be combined into one
> header.
> 
> Since this is already in namespace ns3::epidemic, could you rename this just 
> class Header : public ns3::Header

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
File src/epidemic/model/epidemic-routing-protocol.cc (right):

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-routing-protocol.cc:71: .AddAttribute
("QueueEntryExpireTime","Time in seconds after which a packet in the queue will
expire.",
On 2014/01/07 23:57:09, Peter Barnes wrote:
> They shouldn't be:  they should differ by at least the channel propagation
> delay.  So which is it?  
> On 2013/12/30 23:11:11, mjf.alenazi wrote:
> > Time after enqueuing on the local node or the packet originated both are the
> > same. 
> > On 2013/12/08 22:24:26, Tom Henderson wrote:
> > > Time after enqueuing on the local node, or time after which it was
> originally
> > > sent?

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-routing-protocol.cc:75: .AddAttribute
("BeaconInterval","Time in seconds after which a beacon packet is broadcast.",
I have added a new parameter to configure this time.
On 2014/01/07 23:57:09, Peter Barnes wrote:
> Please clarify: according to the code, this is the minimum time between
beacons
> (up to 100 ms uniformly random gets added)
> On 2013/12/30 23:11:11, mjf.alenazi wrote:
> > Time specified in seconds + some milliseconds to avoid collisions. 
> > On 2013/12/08 22:24:26, Tom Henderson wrote:
> > > Mean time, or exact time?

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-routing-protocol.cc:127: Ptr<UniformRandomVariable>
uv = CreateObject<UniformRandomVariable> ();
I really do not understand the new stream uniform random variable. Could you
please fix it so it is unreproducible. 
On 2014/01/07 23:57:09, Peter Barnes wrote:
> I'm not sure this is what you want.  If I understand correctly, each Start,
and
> each SendBeacon, gets an *independent* UniformRandomVariable. The starting
point
> for each depends on the initialization order of *all* other random variables,
> which makes the behavior of a single node unreproducible if anything in the
> model changes (such as the number of nodes).  I think the r.v object should be
> part of the router (not a new one created each time you need a r.v), and
should
> be accessible via attribute, in order to deterministically set the stream
> number, at least.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-routing-protocol.cc:287: *  Control packets
generated at this node, are tagged with EpedemicTag.
On 2014/01/07 23:57:09, Peter Barnes wrote:
> EpidemicTag

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-routing-protocol.cc:300: if (  tag.GetEpidemicTag ()
!= -1)
Are IsDefault() and IsData() standard functions or should I implement them
myself. 
On 2014/01/07 23:57:09, Peter Barnes wrote:
> -1 is the magic default tag value.  This should be an enum constant in
> EpidemicTag, or a function test IsDefault(), or even better, IsData().

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-routing-protocol.cc:303: * if the packet is not
tagged, it means a data packet
I am just trying to see if the packet is generated at the source. In the
routeoutput function, the source is always not defined. If there is another way,
please let me know. 
I even filed this as a bug but Tom it's OK
https://www.nsnam.org/bugzilla/show_bug.cgi?id=1480
On 2014/01/07 23:57:09, Peter Barnes wrote:
> This is a misuse of Tags.  Tags are not present in real network packets; don't
> use Tags to implement protocols.  If you need this information, you have to
> encode it into the protocol itself, typically in the protocol header.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-routing-protocol.cc:376: EpidemicHeader
epidemicHeader;
On 2014/01/07 23:57:09, Peter Barnes wrote:
> This is unused (but gcc doesn't warn?!?)

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
File src/epidemic/model/epidemic-routing-protocol.h (right):

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-routing-protocol.h:68: /// Transport Port for MANET
routing protocols ports
On 2014/01/07 23:57:09, Peter Barnes wrote:
> "ports, based on \RFC{5498}"

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-routing-protocol.h:74: Ptr<Ipv4Route> RouteOutput
(Ptr<Packet> p, const Ipv4Header &header, Ptr<NetDevice> oif,
Socket::SocketErrno &sockerr);
On 2014/01/07 23:57:09, Peter Barnes wrote:
> // Inherited methods:

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-routing-protocol.h:109: uint32_t
findOutputDeviceForAddress ( Ipv4Address  dst);
On 2014/01/07 23:57:09, Peter Barnes wrote:
> CamelCase:  Find...

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-routing-protocol.h:135: /// Tag used by epidemic
implementation
On 2014/01/07 23:57:09, Peter Barnes wrote:
> \ingroup epidemic

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-routing-protocol.h:141: EpidemicTag (int32_t o = -1)
: Tag (),
On 2014/01/07 23:57:09, Peter Barnes wrote:
> Default value should be an enum, or you should have a IsDefault() checking
> function, so user code doesn't get littered with "== -1" magic numbers.

Done.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-routing-protocol.h:187: void  Deserialize (TagBuffer
i)
It is inherited from tag.h and this is how it's defined
On 2014/01/07 23:57:09, Peter Barnes wrote:
> IIRC, DeSerialize should return the number of bytes consumed.

https://codereview.appspot.com/13831049/diff/105001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-routing-protocol.h:187: void  Deserialize (TagBuffer
i)
On 2014/01/07 23:57:09, Peter Barnes wrote:
> IIRC, DeSerialize should return the number of bytes consumed.

Done.

Sign in to reply to this message.

Peter Barnes

Overall making great progress. Still missing lots of doxygen, and a number of earlier comments ...

11 years, 1 month ago (2014-03-10 22:36:52 UTC) #22

Konstantinos Katsaros

Hi I have one suggestion for this patch. Since one of the key metrics when ...

11 years ago (2014-04-23 15:49:21 UTC) #23

mjf.alenazi

I have just submitted patch 6. I've run the check-style.py before submitting. I have changed ...

10 years, 11 months ago (2014-05-28 21:57:43 UTC) #24

Peter Barnes

(Sorry I took so long to look at your latest.) Much good progress. Biggest remaining ...

10 years, 9 months ago (2014-07-22 19:25:52 UTC) #25

mjf.alenazi

I have fixed inline comments. https://codereview.appspot.com/13831049/diff/145001/src/epidemic/doc/epidemic.h File src/epidemic/doc/epidemic.h (right): https://codereview.appspot.com/13831049/diff/145001/src/epidemic/doc/epidemic.h#newcode39 src/epidemic/doc/epidemic.h:39: */ Again, I was ...

10 years, 9 months ago (2014-08-02 19:15:12 UTC) #26

Tommaso Pecorella

Hi, I'll not comment every line, but I'd strongly suggest to improve the documentation. 1) ...

10 years, 9 months ago (2014-08-02 21:35:45 UTC) #27

Peter Barnes

- Include guards needed in all headers. - Too many comments from prior patches are ...

10 years, 7 months ago (2014-09-12 20:33:50 UTC) #28

mjf.alenazi

https://codereview.appspot.com/13831049/diff/145001/src/epidemic/test/epidemic-test-suite.cc File src/epidemic/test/epidemic-test-suite.cc (right): https://codereview.appspot.com/13831049/diff/145001/src/epidemic/test/epidemic-test-suite.cc#newcode1 src/epidemic/test/epidemic-test-suite.cc:1: /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */ On 2014/07/22 ...

10 years, 7 months ago (2014-09-18 00:34:27 UTC) #29

Peter Barnes

1. Source formatting ------------------ Please remove multiple blank lines in the middle of functions. Please ...

10 years, 7 months ago (2014-09-23 22:54:24 UTC) #30

mjf.alenazi

I have uploaded patch9. For the epidemic routing protocol operation test case, I know the ...

10 years, 5 months ago (2014-11-25 19:51:56 UTC) #31

Peter Barnes

This is much farther along. Good work. Aside from trivial comments in line: 1. Jitter ...

10 years, 4 months ago (2014-12-17 00:15:45 UTC) #32

mjf.alenazi

I have fixed the pointed issues. I have added a function to avoid re-initiating packets ...

10 years, 4 months ago (2014-12-19 13:24:00 UTC) #33

I have fixed the pointed issues. I have added a function to avoid re-initiating
packets exchange with recently contacted hosts. Also, I have made sure that
lines do not exceed 80 chars in each line.

https://codereview.appspot.com/13831049/diff/205001/.hgignore
File .hgignore (left):

https://codereview.appspot.com/13831049/diff/205001/.hgignore#oldcode1
.hgignore:1: \.rej$
After moving to another computer, this file appeared as modified in "hg status".
I tried to ignore it so it does not affect others. 
On 2014/12/17 00:15:46, Peter Barnes wrote:
> Why is this file deleted?

https://codereview.appspot.com/13831049/diff/205001/src/epidemic/doc/epidemic...
File src/epidemic/doc/epidemic.rst (right):

https://codereview.appspot.com/13831049/diff/205001/src/epidemic/doc/epidemic...
src/epidemic/doc/epidemic.rst:11: the sender's buffer. When two nodes come into
contact, disjoint
On 2014/12/17 00:15:46, Peter Barnes wrote:
> *incoming* packets can't be stored on the sender: the sender doesn't get
> incoming packets.  Perhaps you need to define or clarify the terms.
> On 2014/11/25 19:51:51, mjf.alenazi wrote:
> > Packets are actually stored in the sender's buffer.
> > On 2014/09/23 22:54:25, Peter Barnes wrote:
> > > receiver's buffer.

Done.

https://codereview.appspot.com/13831049/diff/205001/src/epidemic/doc/epidemic...
src/epidemic/doc/epidemic.rst:31: is not implemented in |ns-3|, a beacon
mechanism is added to the implementation.
On 2014/12/17 00:15:46, Peter Barnes wrote:
> |ns3|
> (I know, this is an odd convention)

Acknowledged.

https://codereview.appspot.com/13831049/diff/205001/src/epidemic/doc/epidemic...
src/epidemic/doc/epidemic.rst:72: Packets, stored in buffers, are dropped if
they exceed HopCount, they are older than QueueEntryExpireTime,
On 2014/12/17 00:15:46, Peter Barnes wrote:
> Please reflow so all lines are less than 80 characters wide.
> 
> Do this on *all* files, even source code and headers.

Done.

https://codereview.appspot.com/13831049/diff/205001/src/epidemic/doc/epidemic...
src/epidemic/doc/epidemic.rst:73: or the holding buffer exceed QueueLength.
On 2014/12/17 00:15:46, Peter Barnes wrote:
> Delete all whitespace at the beginning of continuation lines.  (White space at
> the beginning of a line becomes indentation in the Sphinx output.)

Done.

https://codereview.appspot.com/13831049/diff/205001/src/epidemic/doc/epidemic...
src/epidemic/doc/epidemic.rst:96: ********
On 2014/12/17 00:15:46, Peter Barnes wrote:
> This section needs cleanup.  Right now it is one paragraph, with many
redundant
> phrases.

Done.

https://codereview.appspot.com/13831049/diff/205001/src/epidemic/doc/epidemic...
src/epidemic/doc/epidemic.rst:125: [#Vahdat] Amin Vahdat and David Becker,
"Epidemic Routing for Partially-Connected
On 2014/12/17 00:15:46, Peter Barnes wrote:
> This should be:
> 
> .. rubric Footnotes
> 
> .. [#Vahdat] Amin Vahdat and David Becker, "Epidemic Routing for
>    Partially-Connected Ad Hoc Networks," Duke University, Technical
>    Report CS-200006, http://issg.cs.duke.edu/epidemic/epidemic.pdf

Done.

https://codereview.appspot.com/13831049/diff/205001/src/epidemic/examples/epi...
File src/epidemic/examples/epidemic-benchmark.cc (right):

https://codereview.appspot.com/13831049/diff/205001/src/epidemic/examples/epi...
src/epidemic/examples/epidemic-benchmark.cc:77: cmd.Usage ("Benchmark example
shows epidemic routing scenario presented in the original paper."
On 2014/12/17 00:15:46, Peter Barnes wrote:
> paper.  "
> 
> (Need two spaces after periods.  Please fix this throughout this usage
message,
> and in epidemic-example.cc)

Done.

https://codereview.appspot.com/13831049/diff/205001/src/epidemic/examples/epi...
File src/epidemic/examples/epidemic-example.cc (right):

https://codereview.appspot.com/13831049/diff/205001/src/epidemic/examples/epi...
src/epidemic/examples/epidemic-example.cc:118: std::cout << "Source number can
not excced number of nodes" << std::endl;
On 2014/12/17 00:15:46, Peter Barnes wrote:
> Errors should be reported to 
> 
>     std::cerr << …
> 
> and followed with 
> 
>     std::cerr << cmd;
>     exit (-1);
> 
> to print the usage message (which should state the restriction) and exit.
> 
> Alternatively, you could fix the error automatically, by setting 
> 
>     source_num = nWifis, 
> 
> for example.  In this case, you should state the error (to std::cout) and how
> you are fixing it:
> 
>     std::cout << "Source number can not excced number of nodes" << std::endl;
>     std::cout << "Setting source number to number of nodes." << std::endl;
> 
> This should occur *before* the summary of command line parameters, so the
> summary shows what is actually being used.

Done.

https://codereview.appspot.com/13831049/diff/205001/src/epidemic/examples/epi...
src/epidemic/examples/epidemic-example.cc:123: std::cout << "Sink number can not
excced number of nodes" << std::endl;
On 2014/12/17 00:15:46, Peter Barnes wrote:
> See previous comment.

Done.

https://codereview.appspot.com/13831049/diff/205001/src/epidemic/examples/epi...
src/epidemic/examples/epidemic-example.cc:179: "MaxY", DoubleValue (1500.0)
On 2014/12/17 00:15:46, Peter Barnes wrote:
> (See comment on patch 8)
> 
> You'll have to read the tutorial and models docs on mobility.

Done.

https://codereview.appspot.com/13831049/diff/205001/src/epidemic/helper/epide...
File src/epidemic/helper/epidemic-helper.cc (right):

https://codereview.appspot.com/13831049/diff/205001/src/epidemic/helper/epide...
src/epidemic/helper/epidemic-helper.cc:31: NS_LOG_COMPONENT_DEFINE
("EpidemicHelper");
On 2014/12/17 00:15:46, Peter Barnes wrote:
> Since your last patch, we've moved all NS_LOG_COMPONENT_DEFINE *inside*
> namespace ns3…
> 
> See bug 1551:
> https://www.nsnam.org/bugzilla/show_bug.cgi?id=1551

Done.

https://codereview.appspot.com/13831049/diff/205001/src/epidemic/helper/epide...
File src/epidemic/helper/epidemic-helper.h (right):

https://codereview.appspot.com/13831049/diff/205001/src/epidemic/helper/epide...
src/epidemic/helper/epidemic-helper.h:61: * \param the node on which the routing
protocol will run.
On 2014/12/17 00:15:46, Peter Barnes wrote:
> \param node The node
> 
> as you had before.  (The first word after \param ("node") is the parameter
name
> as it appears in the code; the remainder of the line is the documentation.)

Done.

https://codereview.appspot.com/13831049/diff/205001/src/epidemic/model/epidem...
File src/epidemic/model/epidemic-packet.h (right):

https://codereview.appspot.com/13831049/diff/205001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-packet.h:136: /// Check if MessageType of value is
valid.
On 2014/12/17 00:15:46, Peter Barnes wrote:
> Check that this is a valid MessageType.

Done.

https://codereview.appspot.com/13831049/diff/205001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-packet.h:143: /// true if the message serialized
correctly;otherwise it is set to false.
On 2014/12/17 00:15:46, Peter Barnes wrote:
> /// Valid flag: \c true if the message deserialized correctly. otherwise \c
> false.

Done.

https://codereview.appspot.com/13831049/diff/205001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-packet.h:195: static TypeId GetTypeId (void);
On 2014/12/17 00:15:46, Peter Barnes wrote:
> /**
>  *  Get the registered TypeId for this class.
>  *  \return The object TypeId.
>  */

Done.

https://codereview.appspot.com/13831049/diff/205001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-packet.h:293: static TypeId GetTypeId (void);
On 2014/12/17 00:15:46, Peter Barnes wrote:
> /**
>  *  Get the registered TypeId for this class.
>  *  \return The object TypeId.
>  */

Done.

https://codereview.appspot.com/13831049/diff/205001/src/epidemic/model/epidem...
File src/epidemic/model/epidemic-routing-protocol.cc (right):

https://codereview.appspot.com/13831049/diff/205001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-routing-protocol.cc:143:
m_beaconJitter->SetAttribute ("Max", DoubleValue (m_beaconMaxJitterMs));
On 2014/12/17 00:15:47, Peter Barnes wrote:
> (patch 8 comment:)
> It looks like you add jitter both to scheduling the next SendBeacons,
> and in scheduling the resulting BroadcastPacket.  I think you should be doing
> one, not both.

Done.

https://codereview.appspot.com/13831049/diff/205001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-routing-protocol.cc:234: packet->AddHeader (header);
I tried to do it but I could not. In the route output function packet, I do not
see anything about the outgoing packet to distinguish between control packets
and data packets. I would be happy to change it if someone can help be in this
issue.
On 2014/12/17 00:15:47, Peter Barnes wrote:
> (See patch 8 comment)
> Why do need the EpidemicHeader?  It would seem that TypeHeader would be
> sufficient?

https://codereview.appspot.com/13831049/diff/205001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-routing-protocol.cc:242: Simulator::Schedule
(MilliSeconds (m_beaconJitter->GetValue
()),&RoutingProtocol::BroadcastPacket,this, packet);
On 2014/12/17 00:15:47, Peter Barnes wrote:
> (patch 8 comment:)
> See comment line 143.

Done.

https://codereview.appspot.com/13831049/diff/205001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-routing-protocol.cc:572: NS_LOG_FUNCTION (this << "
interface " << i << " address " << address);
On 2014/12/17 00:15:46, Peter Barnes wrote:
> NS_LOG_FUNCTION (this << i << address);

Done.

https://codereview.appspot.com/13831049/diff/205001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-routing-protocol.cc:659: Simulator::Schedule
(MilliSeconds (m_beaconJitter->GetValue ()),
On 2014/12/17 00:15:46, Peter Barnes wrote:
> (See patch 8 discussion)
> So you assume that many beacons are exchanged in the time nodes are in range? 
> This is a pretty important assumption, and should be stated in the .rst docs
in
> section called "Limitations" or "Assumptions", and again in the docs for
> m_beaconJitter, explaining how it's used and where.

Done.

https://codereview.appspot.com/13831049/diff/205001/src/epidemic/model/epidem...
src/epidemic/model/epidemic-routing-protocol.cc:710: Simulator::Schedule
(MilliSeconds (m_beaconJitter->GetValue ()),
On 2014/12/17 00:15:47, Peter Barnes wrote:
> (See patch 8 discussion)
> Same comment as line 659 and previous.

Done.

Sign in to reply to this message.

Peter Barnes

Missing Doxygen: src/epidemic/model/epidemic-packet-queue.h:131: warning: parameters of member ns3::Epidemic::PacketQueue::Enqueue are not (all) documented src/epidemic/model/epidemic-packet-queue.h:131: warning: return ...

10 years, 4 months ago (2014-12-19 20:38:40 UTC) #34

mjf.alenazi

I fixed the pointed issues. Also, I tried to fix all the doxygen issues in ...

10 years, 4 months ago (2014-12-20 12:35:38 UTC) #35

Peter Barnes

In addition to the line by line comments, I've asked TomH to help us with: ...

10 years, 2 months ago (2015-02-03 01:25:48 UTC) #36

Tommaso Pecorella

Just a few comments. I didn't check all the code, these are the ones from ...

10 years, 2 months ago (2015-02-08 09:29:28 UTC) #37

barnes26_llnl.gov

Hello Tomasso, On Feb 8, 2015, at 1:29 AM, tommypec@gmail.com wrote: > https://codereview.appspot.com/13831049/diff/245001/src/epidemic/doc/epidemic.rst#newcode113 > src/epidemic/doc/epidemic.rst:113: ...

10 years, 2 months ago (2015-02-10 22:18:15 UTC) #38

Hello Tomasso,

On Feb 8, 2015, at 1:29 AM, tommypec@gmail.com wrote:
>
https://codereview.appspot.com/13831049/diff/245001/src/epidemic/doc/epidemic...
> src/epidemic/doc/epidemic.rst:113: The total messages are 45 * 44 = 1980
> messages. The buffer size is 2000
> If the buffer is 2000, then it is not infinite. Rephrase please.

Agreed.  I suggested this wording to indicate that no packets are dropped in
this example.  Mohammed pointed out that this is the example from the original
paper, so we agreed to keep it as is.  IIRC this is configurable (or we should
make it so).

>
https://codereview.appspot.com/13831049/diff/245001/src/epidemic/doc/epidemic...
> src/epidemic/doc/epidemic.rst:135: the time nodes are in range.
> Explain better. I.e., is this a limitation on the node movement ? It is
> a hard or soft limitation ? What happens if the criterion is not met ?

Agreed this should be clearer.

>
https://codereview.appspot.com/13831049/diff/245001/src/epidemic/model/epidem...
> src/epidemic/model/epidemic-routing-protocol.h:79: class RoutingProtocol
> : public Ipv4RoutingProtocol
> I know it's normal for IPv4 routing protocols, but I still find it very
> annoying: the classes are all named "RoutingProtocol" and they are
> "just" in different namespaces.
> Is it possible to name it "EpidemicRoutingProtocol" ?

IIRC it was named this way originally, but I pushed to rename it since it's
already in the namespace.  It seemed kind of silly to force a user to write
"Epidemic::EpidemicRoutingProtocol"; isn't that what namespaces are for?  On the
other hand I know you're allergic to namespaces. :)  If this is just stylistic
we could a) add an item to the style guide, or b) leave this as an implementer's
choice.  If there are bigger issues than just style, I'd like to understand
them.

Peter
_______________________________________________________________________
Dr. Peter D. Barnes, Jr.                Physics Division
Lawrence Livermore National Laboratory  Physical and Life Sciences
7000 East Avenue, L-50                  email:  pdbarnes@llnl.gov
P. O. Box 808                           Voice:  (925) 422-3384
Livermore, California 94550             Fax:    (925) 423-3371

Sign in to reply to this message.

mjf.alenazi

Sorry for the delay. I have been busy with my dissertation. I have fixed the ...

10 years ago (2015-04-26 22:56:10 UTC) #39

Peter Barnes

Mostly minor; all easy to fix. https://codereview.appspot.com/13831049/diff/265001/src/epidemic/examples/epidemic-example.cc File src/epidemic/examples/epidemic-example.cc (right): https://codereview.appspot.com/13831049/diff/265001/src/epidemic/examples/epidemic-example.cc#newcode122 src/epidemic/examples/epidemic-example.cc:122: std::cerr << "Source ...

10 years ago (2015-04-28 23:42:34 UTC) #40

mjf.alenazi

Hello everyone, I am resuming my epidemic code review. I have fixed all indicted issues ...

7 years, 11 months ago (2017-05-13 23:57:16 UTC) #41

Tom Henderson

This code review has migrated to https://codereview.appspot.com/323290043/ in order to accommodate the port to ns-3.27.

7 years, 9 months ago (2017-07-14 13:40:11 UTC) #42

jpgs_ittc.ku.edu

> On 14 Jul 2017, at 21:40, tomh.org@gmail.com wrote: > > This code review has ...

7 years, 9 months ago (2017-07-14 14:37:06 UTC) #43

mjf.alenazi

On 2017/07/14 14:37:06, jpgs_ittc.ku.edu wrote: > > On 14 Jul 2017, at 21:40, mailto:tomh.org@gmail.com wrote: ...

7 years, 9 months ago (2017-07-15 11:33:10 UTC) #44

barimahappiah

7 years, 4 months ago (2017-12-26 17:57:59 UTC) #45

Hi,
I am working on a project but I would want to how the percentage of messages
delivered and message delivery latency were done. I am new to ns3 and I would
appreciate any help. Thank you very much.

Sign in to reply to this message.

Issue 13831049: Epidemic routing protocol addition for review

Description

Patch Set 1 #

Patch Set 2 : Fixed multiple issues based on inline comments from Peter Barnes #

Patch Set 3 : Adding comments, test cases and fixing more issues fore Epidemic routing #

Patch Set 4 : Fixing code styles and doxygen docs #

Patch Set 5 : Fixing summary vector. Seperating epidmic tag in seprate files and other issues #

Patch Set 6 : Fixing code based on comments. Changed globalPacketID from 64bit to 32bit to match original paper. #

Patch Set 7 : Fixing doxygen docs and code style #

Patch Set 8 : Fixing more doxygen comments and adding epidemic tag enum types #

Patch Set 9 : Fixing logging functions #

Patch Set 10 : Adding a function to not communicate with recently contacted hosts. Fixing more comments. #

Patch Set 11 : Fixing doxygen issues #

Patch Set 12 : Fixing doxygen #

Messages