Issue 103470050: new WAVE example VANET routing compare

Konstantinos Katsaros

Hi Scott, This patch contains only the changes to the wscript file for WAVE examples ...

9 years, 10 months ago (2014-06-23 09:20:06 UTC) #1

scarpen

On 2014/06/23 09:20:06, Konstantinos Katsaros wrote: > Hi Scott, > > This patch contains only ...

9 years, 10 months ago (2014-06-23 20:35:46 UTC) #2

Tom Henderson

thanks for contributing this, it looks useful. it looks like it shouldn't take much more ...

9 years, 10 months ago (2014-06-25 17:25:08 UTC) #3

scarpen

Thank you for these excellent review comments. I have addressed most of them, but before ...

9 years, 9 months ago (2014-07-05 19:53:04 UTC) #4

Thank you for these excellent review comments.  I have addressed most of
them, but before an update I still have problems understanding how (in the
world of ns-3) to address the following:

https://codereview.appspot.com/103470050/diff/40001/src/
wave/examples/vanet-routing-compare.cc#newcode399
src/wave/examples/vanet-routing-compare.cc:399: static WaveBsmStats &
GetWaveBsmStats ();
in general, we do not expose raw pointers like this in ns-3.  Why does
this need to be exposed by reference instead of by value?  Why is it
static?

I need help or a solid example to follow to get past this one.  Let me
describe what I am trying to do, which is send packets, and count stats.  I
have chosen to separate the functionality into two different classes.
Let's call one the PACKET_SENDER and one the STATS_COUNTER.  They are
disjoint.  The example I originally started with set up packet sending
using static methods, which it then invoked through the scheduler.  All is
fine.  But, on the callback, I understand that I can pass back a context.
So, do I pass back the PACKET_SENDER (so I can determine if I still need to
send more packets) or do I pass back the STATS_COUNTER (so that I can
"count" the fact that I just sent one more packet)?  I don't necessarily
want to couple PACKET_SENDER and STATS_COUNTER together, just so that I can
have a single context of both, because they really are not so directly
related, but I'll do that if necessary.  So, yes, the current code is messy
with static instances, so that I could find and access all the instance
objects necessary from within my methods for sending and receiving packets
and counting the packet statistics.

I have a similar issue with decoupling classes, as follow - if I have an
example which uses a helper which depends on a model file, but their is
shared data between them, what is the convention for deciding where/how to
put the data elements and accessors?  If I put them in the model, then the
example needs to access the model directly, when I think the example should
only interface with the helper (and avoid directly accessing the model, if
possible).

Thx in advance.
-Scott



On Wed, Jun 25, 2014 at 1:25 PM, <tomh.org@gmail.com> wrote:

> thanks for contributing this, it looks useful.  it looks like it
> shouldn't take much more effort to get this ready for merge.
>
>
> https://codereview.appspot.com/103470050/diff/40001/src/
> wave/examples/vanet-routing-compare.cc
> File src/wave/examples/vanet-routing-compare.cc (right):
>
> https://codereview.appspot.com/103470050/diff/40001/src/
> wave/examples/vanet-routing-compare.cc#newcode109
> src/wave/examples/vanet-routing-compare.cc:109: * - Selecting DSR for
> scenario=4 (370 vehicles) crashes.
> Please file these as bugs against the DSR module, providing your example
> as the test case.
>
> https://codereview.appspot.com/103470050/diff/40001/src/
> wave/examples/vanet-routing-compare.cc#newcode144
> src/wave/examples/vanet-routing-compare.cc:144: class WaveBsmStats
> overall, I wonder about the need to write a class with accessors for
> these private data integer counters, vs. just declaring a struct.  The
> only thing that is not just a counter is the GetBsmPdr which could be
> implemented outside of a class.  Just a thought...
>
> https://codereview.appspot.com/103470050/diff/40001/src/
> wave/examples/vanet-routing-compare.cc#newcode161
> src/wave/examples/vanet-routing-compare.cc:161: * \brief Increments the
> count of tranmitted packets
> transmitted
>
> https://codereview.appspot.com/103470050/diff/40001/src/
> wave/examples/vanet-routing-compare.cc#newcode167
> src/wave/examples/vanet-routing-compare.cc:167: * \brief Returns the
> count of tranmitted packets
> transmitted
>
> https://codereview.appspot.com/103470050/diff/40001/src/
> wave/examples/vanet-routing-compare.cc#newcode174
> src/wave/examples/vanet-routing-compare.cc:174: * that are expected to
> be received
> can you define "expected to be received?"  How is this determined?
>
> https://codereview.appspot.com/103470050/diff/40001/src/
> wave/examples/vanet-routing-compare.cc#newcode186
> src/wave/examples/vanet-routing-compare.cc:186: * \brief Increments the
> count of actual packets received within range
> what is the distinction between this method and the previous?  How does
> one receive packets "out of range"?
>
> https://codereview.appspot.com/103470050/diff/40001/src/
> wave/examples/vanet-routing-compare.cc#newcode247
> src/wave/examples/vanet-routing-compare.cc:247: int wavePktSendCount;
> in general, ns-3 naming conventions would preface these private member
> variables with "m_" such as m_wavePktSendCount
>
> https://codereview.appspot.com/103470050/diff/40001/src/
> wave/examples/vanet-routing-compare.cc#newcode359
> src/wave/examples/vanet-routing-compare.cc:359: class WaveBsmHelper
> this class should probably be moved to src/wave/helper
>
> https://codereview.appspot.com/103470050/diff/40001/src/
> wave/examples/vanet-routing-compare.cc#newcode374
> src/wave/examples/vanet-routing-compare.cc:374: * \param totalTime total
> amount of time that BSM packets should be tranmitted
> transmitted
>
> https://codereview.appspot.com/103470050/diff/40001/src/
> wave/examples/vanet-routing-compare.cc#newcode379
> src/wave/examples/vanet-routing-compare.cc:379: * GPS time-sync is
> ~40-100 ns.  Universally sychronized time among all vehicles
> synchronized
>
> https://codereview.appspot.com/103470050/diff/40001/src/
> wave/examples/vanet-routing-compare.cc#newcode382
> src/wave/examples/vanet-routing-compare.cc:382: * \param rangeSq the
> expected transmission range, in m.  (value is squared
> it may be better to store it squared but hide this detail from users (it
> is more natural to just input the non-squared value)
>
> https://codereview.appspot.com/103470050/diff/40001/src/
> wave/examples/vanet-routing-compare.cc#newcode396
> src/wave/examples/vanet-routing-compare.cc:396: * \brief Returns the
> WaveBsmStats instanceta transmitted
> instance?
>
> https://codereview.appspot.com/103470050/diff/40001/src/
> wave/examples/vanet-routing-compare.cc#newcode399
> src/wave/examples/vanet-routing-compare.cc:399: static WaveBsmStats &
> GetWaveBsmStats ();
> in general, we do not expose raw pointers like this in ns-3.  Why does
> this need to be exposed by reference instead of by value?  Why is it
> static?
>
> https://codereview.appspot.com/103470050/diff/40001/src/
> wave/examples/vanet-routing-compare.cc#newcode528
> src/wave/examples/vanet-routing-compare.cc:528: // more packets to send?
> in general (if this is moved to src/wave/helper) every method should
> include NS_LOG statements.
>
> https://codereview.appspot.com/103470050/diff/40001/src/
> wave/examples/vanet-routing-compare.cc#newcode551
> src/wave/examples/vanet-routing-compare.cc:551: NS_LOG_UNCOND ("Sending
> WAVE pkt # " << wavePktsSent );
> should this kind of output be explicitly enabled by the user?
>
> https://codereview.appspot.com/103470050/diff/40001/src/
> wave/examples/vanet-routing-compare.cc#newcode602
> src/wave/examples/vanet-routing-compare.cc:602: int wavePort = 9080;
> in general, may be better to put this at the top of your program such
> as:
>
> static const int wavePort = 9080;
>
> rather than put the magic number here.
>
> https://codereview.appspot.com/103470050/diff/40001/src/
> wave/examples/vanet-routing-compare.cc#newcode639
> src/wave/examples/vanet-routing-compare.cc:639: UniformVariable n (1,
> m_gpsAccuracyNs);
> one thing that you will want to control for is random variable stream
> assignments.  If uncontrolled, perturbations in the program
> configuration may result in subtle changes in the random variables used
> elsewhere.  For example, changing a routing protocol may result in the
> above variable being changed, so while you think you are running the
> identical scenario but only with a different routing protocol, you are
> not.
>
> https://codereview.appspot.com/103470050/diff/40001/src/
> wave/examples/vanet-routing-compare.cc#newcode680
> src/wave/examples/vanet-routing-compare.cc:680: std::pair<Ptr<Ipv4>,
> uint32_t> xx = m_adhocTxInterfaces->Get (i);
> better variable name than 'xx'?
>
> https://codereview.appspot.com/103470050/diff/40001/src/
> wave/examples/vanet-routing-compare.cc#newcode703
> src/wave/examples/vanet-routing-compare.cc:703: * \ingroup wifi
> in general, review what doxygen groups you are assigning in this file.
> Probably should delete all of these.
>
> https://codereview.appspot.com/103470050/diff/40001/src/
> wave/examples/vanet-routing-compare.cc#newcode742
> src/wave/examples/vanet-routing-compare.cc:742: * \brief Increments the
> number of (application-date)
> application-data
>
> https://codereview.appspot.com/103470050/diff/40001/src/
> wave/examples/vanet-routing-compare.cc#newcode1068
> src/wave/examples/vanet-routing-compare.cc:1068: : m_TotalTime (300.01),
> any chance you should tie this value (300.01) instead to the simulation
> total time
>
> https://codereview.appspot.com/103470050/diff/40001/src/
> wave/examples/vanet-routing-compare.cc#newcode1253
> src/wave/examples/vanet-routing-compare.cc:1253: NS_LOG_UNCOND
> (m_protocolName + " " + PrintReceivedRoutingPacket (socket, packet));
> again, perhaps control the generation of log messages based on user
> input preferences
>
> https://codereview.appspot.com/103470050/diff/40001/src/
> wave/examples/vanet-routing-compare.cc#newcode1568
> src/wave/examples/vanet-routing-compare.cc:1568: * \brief The
> ConfigStoreHelper class simplifies config-store raw text load and save
> not sure this merits a helper in src/config-store/
>
> https://codereview.appspot.com/103470050/diff/40001/src/
> wave/examples/vanet-routing-compare.cc#newcode2240
> src/wave/examples/vanet-routing-compare.cc:2240:
> this seems like a tedious way to manage program configuration (directing
> them through global values) but I guess we need to be able to manage
> through the config-store, correct?
>
> https://codereview.appspot.com/103470050/
>



-- 

Best,
Scott Carpenter
Graduate student (PhD track), NCSU Department of Computer Science
scarpen@ncsu.edu
919-413-5083

Sign in to reply to this message.

scarpen

I have refactored and addressed the code review comments. For the DSR issues I previously ...

9 years, 9 months ago (2014-07-18 18:15:22 UTC) #5

Konstantinos Katsaros

Hi Scott, I have two comments on the BSM application. These messages are generally sent ...

9 years, 9 months ago (2014-07-22 12:10:21 UTC) #6

Konstantinos Katsaros

I can't get any BSM messages using the trace file. The goodput is on the ...

9 years, 9 months ago (2014-07-22 14:26:05 UTC) #7

scarpen

Thank you for the comments. I have been out of town for several days. I ...

9 years, 9 months ago (2014-07-28 12:51:43 UTC) #8

scarpen

Thank you for the observations. Again, I have been out of town. I will review ...

9 years, 9 months ago (2014-07-28 12:54:39 UTC) #9

scarpen

On 2014/07/28 12:51:43, scarpen wrote: > Thank you for the comments. I have been out ...

9 years, 9 months ago (2014-07-29 20:25:51 UTC) #10

On 2014/07/28 12:51:43, scarpen wrote:
> Thank you for the comments.  I have been out of town for several days.  I
> agree with both of your suggestions, and will begin on addressing them
> later this week.
> -Scott
> 
> 
> On Tue, Jul 22, 2014 at 8:10 AM, <mailto:dinos.katsaros@gmail.com> wrote:
> 
> > Hi Scott,
> >
> > I have two comments on the BSM application.
> >
> > These messages are generally sent with higher priority, so I would
> > suggest to use the QoSTag
> > (http://www.nsnam.org/doxygen/classns3_1_1_qos_tag.html) and have these
> > BSM messages with higher AC, while the OnOff traffic can have lower.
> > Again, this could be an attribute so the user can change it.
> >
> > Also, the definition of wavePort should be done in the BSMHelper header
> > file not in the middle of the source file.
> >
> >
> > https://codereview.appspot.com/103470050/diff/60001/src/
> > wave/helper/wave-bsm-helper.cc
> > File src/wave/helper/wave-bsm-helper.cc (right):
> >
> > https://codereview.appspot.com/103470050/diff/60001/src/
> > wave/helper/wave-bsm-helper.cc#newcode140
> > src/wave/helper/wave-bsm-helper.cc:140: socket->Send (Create<Packet>
> > (pktSize));
> > When sending a BSM, you should consider using a higher AC by attaching a
> > QoSTag.
> >
> > https://codereview.appspot.com/103470050/diff/60001/src/
> > wave/helper/wave-bsm-helper.cc#newcode200
> > src/wave/helper/wave-bsm-helper.cc:200: static const int wavePort =
> > 9080;
> > The wavePort should be defined in the header file not in the middle of
> > source file.
> >
> > https://codereview.appspot.com/103470050/
> >
> 
> 
> 
> -- 
> 
> Best,
> Scott Carpenter
> Graduate student (PhD track), NCSU Department of Computer Science
> mailto:scarpen@ncsu.edu
> 919-413-5083

1. The definition of the wavePort is trial.  Fixed locally, and will be part of
my next patch set.
2. In reviewing the QoSTag idea, I would like to defer, for the following
reasons.  First, Junling Bu has submitted a pending WAVE patch set, which
includes "WAVE EDCA" functionality.  I know that all of this stuff needs to be
worked out and merged together, and it seems that when that happens, then it is
the best time to assign BSM messages as higher priority than non-BSM packets.  I
think this is part of the overall issue - the current released WAVE
implementation is really only 802.11p without the 1609 stuff.  WiFi already has
EDCA, and the pending WAVE 1609 patch set adds its own EDCA.  So, adding QoSTag
now seems to conflict with whatever needs to happen with WAVE EDCA and existing
WiFi QoS (i.e. tags, EDCA).  So, I would prefer to see that sorted out first, so
that I don't have to rework my code again.  If there is strong opinion that I go
ahead and add QoSTag now, then I will do so, with an understanding that I will
rework after the new WAVE code is merged in.

Sign in to reply to this message.

Tom Henderson

In general, most of the ns-3 helper Install() methods are responsible for plumbing things together ...

9 years, 7 months ago (2014-09-06 02:02:12 UTC) #11

scarpen

Tom (et. al.) Thank you for the comments. They are truly useful. I have addressed ...

9 years, 7 months ago (2014-09-06 16:13:12 UTC) #12

Tom (et. al.)

Thank you for the comments.  They are truly useful.

I have addressed most with an additional patch set, now pushed.  A few
responses to some comments, as follows:

https://codereview.appspot.com/103470050/diff/100001/src/
wave/helper/wave-bsm-helper.cc#newcode148
src/wave/helper/wave-bsm-helper.cc:148: NS_LOG_UNCOND ("Sending WAVE pkt
# " << wavePktsSent );
Is there a reason you can't log the normal way in ns-3, by controlling
log message generation via log components being enabled and disabled?

SC:  I think that is possible.  But, I will note:  Konstantinos previously
code-commented to make this a configurable setting, which I already did.
The idea here is that the VANET simulations can run for a really long time,
and so I wanted to (optionally) provide feedback as the script runs - e.g.,
once per second.  It can be easily turned on/off, but I personally like the
idea of seeing some feedback, so that I can assess "oh, the script is
starting to run now.  And it looks like I will need to go get several cups
of coffee."  I didn't know of a way to force the output, other than
*_UNCOND()

https://codereview.appspot.com/103470050/diff/100001/src/
wave/helper/wave-bsm-helper.h#newcode93
src/wave/helper/wave-bsm-helper.h:93: static int wavePort;
exposed, undocumented public member.  Is access needed?

SC:  Again, previous code-review comment from Konstantinos was to move this
from the cc file to the h file.  I can put it wherever, and comment it
however you all would like it.  It is an arbitrary port number, and only
needed so that a WAVE message can be sent (and then "found" once received)
through the internet stack.  Ideally, this should not be needed, as WAVE
messages are to be sent as WSMP frames, not IP frames, but that is not
supported until we merge Junling's stuff.  So, ultimately, I think the need
for this would actually be removed here.  Of course, Will do as you all
wish.

In general, most of the ns-3 helper Install() methods are responsible
for plumbing things together simulation setup time, and not much else,
but there is much more "action" going on with these helpers, with little
public API to control it.  I wonder whether some of the logic for
generating and receiving traffic should migrate outside of the
WaveBsmHelper, or whether more public APIs are needed.

SC:  I fully understand the point, here.  BsmHelper is indeed assisting
with the plumbing for message setup on the WAVE-enabled nodes/devices.
But, as you note, also helping with the setup for actually sending,
receiving, counting the BSMs.  However, I am a bit torn about exposing more
APIs, unless there is a justification for explicitly needing them now (or
others can add them in the future, as need arises).  A big issue in VANET
simulation (IMO) is inconsistency in setup.  I think researchers (and
students) need a tool to quickly run a VANET experiment to say "what's the
throughput or performance if change packet size, interval, number of nodes"
etc., especially if they are not varying the channel communications and do
not want to bother setting up that code.  I see a lot of posts where intro
students ask "what simulator should I use for VANET?" and most are told to
use OMNeT++ because of the ease of setup (although I personally think there
are accuracy issues in those results).  Thus, I am trying to expose to the
user via Install() the key things that you need to specify for VANET safety
messages - i.e., packet size, and broadcasting interval.  Knowing those
types of parameters, then the way in which the packets are setup,
transmitted, received and counted can be done on behalf of the user.
Exposing too much requires the user to have to figure out what channel to
use, time synchronization issues, etc.  Think about how many times there
are questions posted on the board like "I sent packets, but nothing is
received" to which the answer is "don't transmit everything at the same
time, because all the packets are colliding with one another".  Or "how do
I count received / dropped / errored packets?  How do I calculate PDR?"  So
my approach is to provide a safety message helper (BsmHelper) that the user
uses to "install" on every WAVE-enabled device, and then
sending/receiving/counting packets is done consistently (via a "helper").
Related, think about how packets are set up for transmission.  Currently,
each is send using the IP stack as a UDP packet.  Once Junling's code is
merged in, I think this can be replaced to properly send the packets as a
WSMP packet, and to avoid the reliance on UDP.  Where do users want to make
that change?  Does every user want to change every VANET script, or
remember to have to go back and change that?  If that is put in one spot -
in the helper - then the helper can be changed without any anyone else
having to think about it further.  Encapsulation.  Nonetheless, of course
am willing to refactor as you all desire.

On Fri, Sep 5, 2014 at 10:02 PM, <tomh.org@gmail.com> wrote:

> In general, most of the ns-3 helper Install() methods are responsible
> for plumbing things together simulation setup time, and not much else,
> but there is much more "action" going on with these helpers, with little
> public API to control it.  I wonder whether some of the logic for
> generating and receiving traffic should migrate outside of the
> WaveBsmHelper, or whether more public APIs are needed.
>
>
> https://codereview.appspot.com/103470050/diff/100001/src/
> wave/helper/wave-bsm-helper.cc
> File src/wave/helper/wave-bsm-helper.cc (right):
>
> https://codereview.appspot.com/103470050/diff/100001/src/
> wave/helper/wave-bsm-helper.cc#newcode25
> src/wave/helper/wave-bsm-helper.cc:25: using namespace ns3;
> this should be in namespace ns3, not using namespace ns3
>
> https://codereview.appspot.com/103470050/diff/100001/src/
> wave/helper/wave-bsm-helper.cc#newcode35
> src/wave/helper/wave-bsm-helper.cc:35: m_adhocTxNodes (NULL),
> use '0' instead of symbolic NULL
>
> https://codereview.appspot.com/103470050/diff/100001/src/
> wave/helper/wave-bsm-helper.cc#newcode36
> src/wave/helper/wave-bsm-helper.cc:36: m_TotalSimTime (300.01),
> why is total simulation time assumed to be 300 seconds in a helper?
> what happens if simulation is not this exact time?
>
> https://codereview.appspot.com/103470050/diff/100001/src/
> wave/helper/wave-bsm-helper.cc#newcode104
> src/wave/helper/wave-bsm-helper.cc:104: getDistSq (Ptr<Node> n1,
> Ptr<Node> n2)
> why not make this a private method of this helper?
>
> https://codereview.appspot.com/103470050/diff/100001/src/
> wave/helper/wave-bsm-helper.cc#newcode133
> src/wave/helper/wave-bsm-helper.cc:133: NodeContainer const & n =
> *m_adhocTxNodes;
> this 'n' is same variable name as the random variable? (and container
> below)
>
> https://codereview.appspot.com/103470050/diff/100001/src/
> wave/helper/wave-bsm-helper.cc#newcode148
> src/wave/helper/wave-bsm-helper.cc:148: NS_LOG_UNCOND ("Sending WAVE pkt
> # " << wavePktsSent );
> Is there a reason you can't log the normal way in ns-3, by controlling
> log message generation via log components being enabled and disabled?
>
> https://codereview.appspot.com/103470050/diff/100001/src/
> wave/helper/wave-bsm-helper.cc#newcode157
> src/wave/helper/wave-bsm-helper.cc:157: Ptr<Node> object = *j;
> why not an integer for loop index, with n.Get(j)?
>
> https://codereview.appspot.com/103470050/diff/100001/src/
> wave/helper/wave-bsm-helper.h
> File src/wave/helper/wave-bsm-helper.h (right):
>
> https://codereview.appspot.com/103470050/diff/100001/src/
> wave/helper/wave-bsm-helper.h#newcode29
> src/wave/helper/wave-bsm-helper.h:29: #include "ns3/mobility-module.h"
> any code in src/* should not include the "*-module.h" variants of
> headers; instead, include only the specific headers.
>
> https://codereview.appspot.com/103470050/diff/100001/src/
> wave/helper/wave-bsm-helper.h#newcode60
> src/wave/helper/wave-bsm-helper.h:60: * \param gspAccuracyNs the timing
> synchronization accuracy of GPS time, in ns.
> s/gsp/gps
>
> https://codereview.appspot.com/103470050/diff/100001/src/
> wave/helper/wave-bsm-helper.h#newcode80
> src/wave/helper/wave-bsm-helper.h:80: WaveBsmStats * GetWaveBsmStats ();
> Our convention is to not expose raw pointers in public APIs.  Can
> WaveBsmStats inherit from ns3::Object or ns3::SimpleRefCount and this
> API be changed to:
>
> Ptr<WaveBsmStats> GetWaveBsmStats()?
>
> https://codereview.appspot.com/103470050/diff/100001/src/
> wave/helper/wave-bsm-helper.h#newcode93
> src/wave/helper/wave-bsm-helper.h:93: static int wavePort;
> exposed, undocumented public member.  Is access needed?
>
> https://codereview.appspot.com/103470050/diff/100001/src/
> wave/helper/wave-bsm-helper.h#newcode135
> src/wave/helper/wave-bsm-helper.h:135: double m_gpsAccuracyNs;    // ns
> in general, can time be maintained in Time objects?
>
> https://codereview.appspot.com/103470050/diff/100001/src/
> wave/helper/wave-bsm-helper.h#newcode138
> src/wave/helper/wave-bsm-helper.h:138: Ptr<UniformRandomVariable> n;
> better variable name needed than 'n'
>
> https://codereview.appspot.com/103470050/
>

-- 

Best,
Scott Carpenter
Graduate student (PhD track), NCSU Department of Computer Science
scarpen@ncsu.edu
919-413-5083

Sign in to reply to this message.

Tom Henderson

On 2014/09/06 16:13:12, scarpen wrote: > Tom (et. al.) > > Thank you for the ...

9 years, 7 months ago (2014-09-09 19:25:54 UTC) #13

On 2014/09/06 16:13:12, scarpen wrote:
> Tom (et. al.)
> 
> Thank you for the comments.  They are truly useful.
> 
> I have addressed most with an additional patch set, now pushed.  

I made some additional changes on top of this patch while I was testing it; they
are temporarily stored here:
http://code.nsnam.org/tomh/ns-3-dev-vanet

More inline below...

> A few
> responses to some comments, as follows:
> 
> https://codereview.appspot.com/103470050/diff/100001/src/
> wave/helper/wave-bsm-helper.cc#newcode148
> src/wave/helper/wave-bsm-helper.cc:148: NS_LOG_UNCOND ("Sending WAVE pkt
> # " << wavePktsSent );
> Is there a reason you can't log the normal way in ns-3, by controlling
> log message generation via log components being enabled and disabled?
> 
> SC:  I think that is possible.  But, I will note:  Konstantinos previously
> code-commented to make this a configurable setting, which I already did.
> The idea here is that the VANET simulations can run for a really long time,
> and so I wanted to (optionally) provide feedback as the script runs - e.g.,
> once per second.  It can be easily turned on/off, but I personally like the
> idea of seeing some feedback, so that I can assess "oh, the script is
> starting to run now.  And it looks like I will need to go get several cups
> of coffee."  I didn't know of a way to force the output, other than
> *_UNCOND()

I would prefer to separate verbose status reporting from logging.  Perhaps
m_verbose (default true) to get the once-per-second reporting of stats, and
m_logging (default false) which would enable log components.  Presently, there
is no way to disable outputting even if m_verbose is false.

But I am fine with your overall concept of a "default on" status reporting; I
agree this is helpful to users.

> 
> 
> https://codereview.appspot.com/103470050/diff/100001/src/
> wave/helper/wave-bsm-helper.h#newcode93
> src/wave/helper/wave-bsm-helper.h:93: static int wavePort;
> exposed, undocumented public member.  Is access needed?
> 
> SC:  Again, previous code-review comment from Konstantinos was to move this
> from the cc file to the h file.  I can put it wherever, and comment it
> however you all would like it.  It is an arbitrary port number, and only
> needed so that a WAVE message can be sent (and then "found" once received)
> through the internet stack.  Ideally, this should not be needed, as WAVE
> messages are to be sent as WSMP frames, not IP frames, but that is not
> supported until we merge Junling's stuff.  So, ultimately, I think the need
> for this would actually be removed here.  Of course, Will do as you all
> wish.

I changed this to a constant integer in the repo I linked to above.

> 
> In general, most of the ns-3 helper Install() methods are responsible
> for plumbing things together simulation setup time, and not much else,
> but there is much more "action" going on with these helpers, with little
> public API to control it.  I wonder whether some of the logic for
> generating and receiving traffic should migrate outside of the
> WaveBsmHelper, or whether more public APIs are needed.
> 
> SC:  I fully understand the point, here.  BsmHelper is indeed assisting
> with the plumbing for message setup on the WAVE-enabled nodes/devices.
> But, as you note, also helping with the setup for actually sending,
> receiving, counting the BSMs.  However, I am a bit torn about exposing more
> APIs, unless there is a justification for explicitly needing them now (or
> others can add them in the future, as need arises).  A big issue in VANET
> simulation (IMO) is inconsistency in setup.  I think researchers (and
> students) need a tool to quickly run a VANET experiment to say "what's the
> throughput or performance if change packet size, interval, number of nodes"
> etc., especially if they are not varying the channel communications and do
> not want to bother setting up that code.  I see a lot of posts where intro
> students ask "what simulator should I use for VANET?" and most are told to
> use OMNeT++ because of the ease of setup (although I personally think there
> are accuracy issues in those results).  Thus, I am trying to expose to the
> user via Install() the key things that you need to specify for VANET safety
> messages - i.e., packet size, and broadcasting interval.  Knowing those
> types of parameters, then the way in which the packets are setup,
> transmitted, received and counted can be done on behalf of the user.
> Exposing too much requires the user to have to figure out what channel to
> use, time synchronization issues, etc.  

While I agree that exposing too much can overwhelm some users, isn't that what
is being done with your vanet-routing-compare.cc?

A general concern I have is that the many command line options and global values
in vanet-routing-compare.cc are not very well tested, especially how they
interact.  I began to address some issues in the above repo, such as the
m_mobility and m_scenario arguments which were interacting.  I still can crash
the simulator if I provide certain arguments.  So, I think we need to test a bit
more the arguments across the range of valid inputs.

But back to the Helper::Install() discussion.  I'm OK with what you are doing;
you are providing API through the Install() arguments.  So I'll revert my
original comment.

  /**
   * \brief Returns the list of moving nove indicators
   * \return the list of moving node indicators
   */
  static std::vector<int>& GetNodesMoving ();

I'm uncomfortable with how this is working (it is not clear from the doxygen but
the user of this helper object needs to be accessing this static variable and
setting it for the mobility to work as expected inside the helper) and I also am
not a fan of exposing a std::vector this way.  I'm not in favor of underlying
static variable for this; what if users decide to use two WaveBsmHelper objects?
 I'd prefer to see non-static member, and different API such as
"EnableNodeMotion (index i);" and "std::vector<int> GetNodesMoving() const;" or
something where the underlying private data is not exported by reference.

Sign in to reply to this message.

scarpen

Thanks again for the comments, and the code updates. Re: logging vs. verbose reporting. I ...

9 years, 7 months ago (2014-09-09 20:07:17 UTC) #14

Thanks again for the comments, and the code updates.

Re:  logging vs. verbose reporting.  I see where you are going.  Makes
sense.  I think I had logging vs. verbose reversed from what you
explained.  That is, I think logging=false was intended to NOT log
anything.  And verbose=true simply meant "if logging, make the output
verbose" instead of terse.

Re: command line options and interactions, overwhelming users and further
testing.  I also see the code changes in your changesets.  My original
intentions of "scenarios" was to collect together many parameters and make
sure they get set up consistently (I found myself forgetting to set up 1 or
more parameters in my experiments).  But, a user might also want to take a
default scenario, and only change one parameter.  Your're right - maybe
they could change some combination that winds up with an invalid
combination.  So, I can see where the interactions between options and
scenarios is confusing - e.g., m_mobility vs. m_scenario.  For mobility -
if you playback(abc) then the sim-time has to be set to the time that
corresponds to abc.  Otherwise, it could crash.  (I think this is an
ns2Mobility requirement).  So the idea of a scenario is to make sure these
stay in sync, and remain legal.  But, you are correct that some
permutations of options probably would be "illegal".  I would have to think
on this more, first.  If you have combos that are crashing, please let me
know, and I can start there.  I think ideally, we need logic to check and
report a fatal error e.g., "you can't use option X=valX with option Y=valY"

Re:  EnableNodeMotion.  I hadn't thought of multiple instances of the
helper (and to be honest, am embarrassed that I had not).  Of
course, what you suggest is therefore much better.    Thanks.

-Scott

On Tue, Sep 9, 2014 at 3:25 PM, <tomh.org@gmail.com> wrote:

> On 2014/09/06 16:13:12, scarpen wrote:
>
>> Tom (et. al.)
>>
>
>  Thank you for the comments.  They are truly useful.
>>
>
>  I have addressed most with an additional patch set, now pushed.
>>
>
>
> I made some additional changes on top of this patch while I was testing
> it; they are temporarily stored here:
> http://code.nsnam.org/tomh/ns-3-dev-vanet
>
> More inline below...
>
>
>  A few
>> responses to some comments, as follows:
>>
>
>  https://codereview.appspot.com/103470050/diff/100001/src/
>> wave/helper/wave-bsm-helper.cc#newcode148
>> src/wave/helper/wave-bsm-helper.cc:148: NS_LOG_UNCOND ("Sending WAVE
>>
> pkt
>
>> # " << wavePktsSent );
>> Is there a reason you can't log the normal way in ns-3, by controlling
>> log message generation via log components being enabled and disabled?
>>
>
>  SC:  I think that is possible.  But, I will note:  Konstantinos
>>
> previously
>
>> code-commented to make this a configurable setting, which I already
>>
> did.
>
>> The idea here is that the VANET simulations can run for a really long
>>
> time,
>
>> and so I wanted to (optionally) provide feedback as the script runs -
>>
> e.g.,
>
>> once per second.  It can be easily turned on/off, but I personally
>>
> like the
>
>> idea of seeing some feedback, so that I can assess "oh, the script is
>> starting to run now.  And it looks like I will need to go get several
>>
> cups
>
>> of coffee."  I didn't know of a way to force the output, other than
>> *_UNCOND()
>>
>
> I would prefer to separate verbose status reporting from logging.
> Perhaps m_verbose (default true) to get the once-per-second reporting of
> stats, and m_logging (default false) which would enable log components.
> Presently, there is no way to disable outputting even if m_verbose is
> false.
>
> But I am fine with your overall concept of a "default on" status
> reporting; I agree this is helpful to users.
>
>
>
>  https://codereview.appspot.com/103470050/diff/100001/src/
>> wave/helper/wave-bsm-helper.h#newcode93
>> src/wave/helper/wave-bsm-helper.h:93: static int wavePort;
>> exposed, undocumented public member.  Is access needed?
>>
>
>  SC:  Again, previous code-review comment from Konstantinos was to move
>>
> this
>
>> from the cc file to the h file.  I can put it wherever, and comment it
>> however you all would like it.  It is an arbitrary port number, and
>>
> only
>
>> needed so that a WAVE message can be sent (and then "found" once
>>
> received)
>
>> through the internet stack.  Ideally, this should not be needed, as
>>
> WAVE
>
>> messages are to be sent as WSMP frames, not IP frames, but that is not
>> supported until we merge Junling's stuff.  So, ultimately, I think the
>>
> need
>
>> for this would actually be removed here.  Of course, Will do as you
>>
> all
>
>> wish.
>>
>
> I changed this to a constant integer in the repo I linked to above.
>
>
>  In general, most of the ns-3 helper Install() methods are responsible
>> for plumbing things together simulation setup time, and not much else,
>> but there is much more "action" going on with these helpers, with
>>
> little
>
>> public API to control it.  I wonder whether some of the logic for
>> generating and receiving traffic should migrate outside of the
>> WaveBsmHelper, or whether more public APIs are needed.
>>
>
>  SC:  I fully understand the point, here.  BsmHelper is indeed
>>
> assisting
>
>> with the plumbing for message setup on the WAVE-enabled nodes/devices.
>> But, as you note, also helping with the setup for actually sending,
>> receiving, counting the BSMs.  However, I am a bit torn about exposing
>>
> more
>
>> APIs, unless there is a justification for explicitly needing them now
>>
> (or
>
>> others can add them in the future, as need arises).  A big issue in
>>
> VANET
>
>> simulation (IMO) is inconsistency in setup.  I think researchers (and
>> students) need a tool to quickly run a VANET experiment to say "what's
>>
> the
>
>> throughput or performance if change packet size, interval, number of
>>
> nodes"
>
>> etc., especially if they are not varying the channel communications
>>
> and do
>
>> not want to bother setting up that code.  I see a lot of posts where
>>
> intro
>
>> students ask "what simulator should I use for VANET?" and most are
>>
> told to
>
>> use OMNeT++ because of the ease of setup (although I personally think
>>
> there
>
>> are accuracy issues in those results).  Thus, I am trying to expose to
>>
> the
>
>> user via Install() the key things that you need to specify for VANET
>>
> safety
>
>> messages - i.e., packet size, and broadcasting interval.  Knowing
>>
> those
>
>> types of parameters, then the way in which the packets are setup,
>> transmitted, received and counted can be done on behalf of the user.
>> Exposing too much requires the user to have to figure out what channel
>>
> to
>
>> use, time synchronization issues, etc.
>>
>
> While I agree that exposing too much can overwhelm some users, isn't
> that what is being done with your vanet-routing-compare.cc?
>
> A general concern I have is that the many command line options and
> global values in vanet-routing-compare.cc are not very well tested,
> especially how they interact.  I began to address some issues in the
> above repo, such as the m_mobility and m_scenario arguments which were
> interacting.  I still can crash the simulator if I provide certain
> arguments.  So, I think we need to test a bit more the arguments across
> the range of valid inputs.
>
> But back to the Helper::Install() discussion.  I'm OK with what you are
> doing; you are providing API through the Install() arguments.  So I'll
> revert my original comment.
>
>   /**
>    * \brief Returns the list of moving nove indicators
>    * \return the list of moving node indicators
>    */
>   static std::vector<int>& GetNodesMoving ();
>
> I'm uncomfortable with how this is working (it is not clear from the
> doxygen but the user of this helper object needs to be accessing this
> static variable and setting it for the mobility to work as expected
> inside the helper) and I also am not a fan of exposing a std::vector
> this way.  I'm not in favor of underlying static variable for this; what
> if users decide to use two WaveBsmHelper objects?  I'd prefer to see
> non-static member, and different API such as "EnableNodeMotion (index
> i);" and "std::vector<int> GetNodesMoving() const;" or something where
> the underlying private data is not exported by reference.
>
>
>
> https://codereview.appspot.com/103470050/
>



-- 

Best,
Scott Carpenter
Graduate student (PhD track), NCSU Department of Computer Science
scarpen@ncsu.edu
919-413-5083

Sign in to reply to this message.

Konstantinos Katsaros

Hi Scott, I am working on a BSM/CAM application and I plan to use your ...

9 years, 5 months ago (2014-11-27 14:34:03 UTC) #15

scarpen

9 years, 5 months ago (2014-11-27 20:13:18 UTC) #16

Thank you for the questions.

1.  The use of r-squared instead of just r is merely a performance
optimization.  Consider Euclidean distance between two points, R, s.t. R =
sqrt ((x2-x1)^2 + (y2-y1)^2).  Then R^2 = (x2-x1)^2 + (y2-y1)^2.  Sqrt is
generally a complex operation.  Thus, since I need to do so many
comparisons to see if the distances between two points are within a certain
"range", I simply pre-compute the distance-square = R^2, and then compare
that to the sum of the squares for the two points.  Just saves a
square-root operation with every compare.  This is just a performance
optimization, and should be "hidden" from the user.

2. I believe the use of the index-1 is simply a 0-based vs. 1-based issue.
That is, we want to have a series of distances.  E.g., the first distance,
second distance, etc.  Give them an index = [1, .. n].  Now the actual
values are stored in a 0-based array.  So, to find the I-th indexed value
in the array, use (I-1) as the array index.  It just seemed more intuitive
to me to use index=1 to mean the "first" distance that I am interested in.
Storing it in a 0-based array is an internal implementation decision.

Please let me know if there are further questions, or if I have failed to
address your concerns.

-Scott

On Thu, Nov 27, 2014 at 9:34 AM, <dinos.katsaros@gmail.com> wrote:

> Hi Scott,
>
> I am working on a BSM/CAM application and I plan to use your stats
> helper.
> I have a question regarding the txSafetyRangeSq element. Why do you use
> the distance squared and not as it is?
>
> with the default
>   m_txSafetyRangesSq[0] = 50.0;
>   m_txSafetyRangesSq[1] = 100.0;
>   m_txSafetyRangesSq[2] = 200.0;...
>
> The used would assume that this is the different radius, but in fact it
> is the radius squared.
>
> Also, this part looks a bit strange. (when calculating the expected Rx)
>
> ...
> int rangeCount = m_txSafetyRangesSq.size ();
> for (int index = 1; index <= rangeCount; index++)
> {
>    if (distSq <= m_txSafetyRangesSq[index - 1])
>    {
>    // we should expect dest node to receive broadcast pkt
>    m_waveBsmStats.IncExpectedRxPktCount (index);
>    }
> }
>
> I understand you want to identify the expected receivers in each area.
> If the distance (distSq) is less than or equal to the (index-1) then you
> increase the index counter.
> e.g. if distSq = 40 (the actual distance of the two nodes is sqrt(40) =
> 6.3) and index = 1, the expected Rx to be increased should be the
> index=0 (for 50). Which is basically what you do in the BSM stats. You
> actually increase the index-1.
>
> void
> WaveBsmStats::IncExpectedRxPktCount (int index)
> {
>   m_wavePktExpectedReceiveCounts[index - 1]++;
> }
>
> My point here is that this (index-1) that you use while you require the
> index, looks confusing.
> Is there any particular reason?
>
> https://codereview.appspot.com/103470050/
>

-- 

Best,
Scott Carpenter
Graduate student (PhD track), NCSU Department of Computer Science
scarpen@ncsu.edu
919-413-5083

Sign in to reply to this message.

Issue 103470050: new WAVE example VANET routing compare

Description

Patch Set 1 #

Patch Set 2 : previous upload failed to include new files #

Patch Set 3 : added new files via mercurial queues commands #

Patch Set 4 : Changes per code review #

Patch Set 5 : tweaks for patchset4 #

Patch Set 6 : tweaks to patchset5 #

Patch Set 7 : additional code review changes #

Messages