Issue 148810043: ns-3 code review: tv spectrum transmitter
|
Created:
9 years, 7 months ago by ben.cizdziel Modified:
9 years ago CC:
ns-3-reviews_googlegroups.com Visibility:
Public. |
Descriptionns-3 code review: tv spectrum transmitter
Patch Set 1 #
Total comments: 50
Patch Set 2 : ns-3 code review: tv spectrum transmitter rev2 #
Total comments: 20
Patch Set 3 : ns-3 code review: tv spectrum transmitter rev3 #
Total comments: 11
Patch Set 4 : ns-3 code review: tv spectrum transmitter rev4 #Patch Set 5 : ns-3 code review: tv spectrum transmitter & geographic positions rev5 #Patch Set 6 : tv spectrum transmitter & geographic positions rev6 #Patch Set 7 : ns-3 code review: tv spectrum transmitter & geographic positions rev7 #MessagesTotal messages: 6
Some (most formatting) comments. I don't run the code, but I'll take a closer look in next days. https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter-helpe... File tv-spectrum-transmitter-helper.cc (right): https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter-helpe... tv-spectrum-transmitter-helper.cc:47: 842e6, 848e6, 854e6, 860e6, 866e6, 872e6, 878e6, 884e6, 890e6}; 80 char column https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter-helper.h File tv-spectrum-transmitter-helper.h (right): https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter-helpe... tv-spectrum-transmitter-helper.h:47: * Provides method to create a random set of transmitters within a given region and location. I personally like example in doxygen documentation.. use the verbatim environment and write some example usage https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter-helpe... tv-spectrum-transmitter-helper.h:60: europe same formatting thing here.. constant values should be recognisable https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter.cc File tv-spectrum-transmitter.cc (right): https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter.cc#ne... tv-spectrum-transmitter.cc:72: SpectrumPhy::DoDispose (); Are these reset required for some circular dependency ? If not ( I really don't know the spectrum module) they are useless. https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter.cc#ne... tv-spectrum-transmitter.cc:80: .AddConstructor<TVSpectrumTransmitter> () No attributes ? they can be useful instead of using Set/Get methods https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter.cc#ne... tv-spectrum-transmitter.cc:80: .AddConstructor<TVSpectrumTransmitter> () No attributes? They can be useful if someone do not want to use Set/Get methods.. https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter.cc#ne... tv-spectrum-transmitter.cc:193: * Stephen Shellhammer, Ahmed Sadek, and Wenyi Zhang. "Technical Challenges for Cognitive Radio in the TV White Space Spectrum." Qualcomm Incorporated. with \n try to be in the 80-column mark.. https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter.cc#ne... tv-spectrum-transmitter.cc:239: else (*psd) [i] = basePSDWattsHz; For formatting reasons, I would prefer a switch/case instead of if/else if. https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter.cc#ne... tv-spectrum-transmitter.cc:302: else (*psd) [i] = 1.77828e-06 * basePSDWattsHz; A switch here is required. else if (i == 31 || i == 32 || i == 33) could be case 31: case 32: case 33: // do what you need https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter.h File tv-spectrum-transmitter.h (right): https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter.h#new... tv-spectrum-transmitter.h:3: * Copyright (c) 2014 Copyright owner missing (also in other file) https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter.h#new... tv-spectrum-transmitter.h:58: cofdm usually constant values should be recognisable. Example: enum SocketErrno { ERROR_NOTERROR, ERROR_ISCONN, ERROR_NOTCONN, ERROR_MSGSIZE, ERROR_AGAIN, ERROR_SHUTDOWN, ERROR_OPNOTSUPP, ERROR_AFNOSUPPORT, ERROR_INVAL, ERROR_BADF, ERROR_NOROUTETOHOST, ERROR_NODEV, ERROR_ADDRNOTAVAIL, ERROR_ADDRINUSE, SOCKET_ERRNO_LAST }; (from socket.h) https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter.h#new... tv-spectrum-transmitter.h:195: Ptr<SpectrumChannel> m_channel; All of these are obvious, but it's better to have documentation than nothing :-) https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter.h#new... tv-spectrum-transmitter.h:207: bool m_active; Document member variables for who, in future, wants to edit your code https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter.h#new... tv-spectrum-transmitter.h:211: } Signal with a comment which is referred to the namespace ns3 https://codereview.appspot.com/148810043/diff/1/tv-trans-example.cc File tv-trans-example.cc (right): https://codereview.appspot.com/148810043/diff/1/tv-trans-example.cc#newcode40 tv-trans-example.cc:40: As it is an example, some comments are needed for who see for the first time your code
Sign in to reply to this message.
In general, a nice first submission. This needs a review/comment from Nicola whether he will accept a modified patch into src/spectrum, or whether this should be spun into a stand-alone module. Echoing some other comments, here are the main issues to deal with: - lack of .rst documentation (can append spectrum.rst if this will be merged there) - lack of tests (discussed with Ben off-list) - lack of comments on examples - decide whether geographic/cartesian coordinate conversion belongs in the helper (probably not) and whether it should be provided by a library - all uses of rand() and srand() need to be changed to use ns-3 random variable stream objects. https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter-helpe... File tv-spectrum-transmitter-helper.cc (right): https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter-helpe... tv-spectrum-transmitter-helper.cc:363: double longitudeRadians = 0.01745329 * longitude; avoid burying constants such as these in the code; promote them to 'static const double' or const double variables, with internal linkage similar to your frequency arrays above. and we typically capitalize such variables. e.g. static const double EARTH_RADIUS =6371e3; but again, are there well-tested library routines for doing coordinate conversion? https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter-helpe... tv-spectrum-transmitter-helper.cc:404: srand(time(NULL)); //set random seed for random number generation we do not use srand() in ns-3. This needs a patch for use of ns3::RandomVariableStream. https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter-helpe... tv-spectrum-transmitter-helper.cc:409: if (density == low) randNumTransmitters = rand() % (int)(ceil((0.33) * freqVectorSize)) + 1; coding style (should be multi-line block with braces). Where does the 0.33 come from? https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter-helpe... tv-spectrum-transmitter-helper.cc:416: int randIndex = rand() % startFreqVector.size(); //get random index from start frequency vector same comment (avoid rand()) https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter-helpe... tv-spectrum-transmitter-helper.cc:441: const double pi = 3.1415926535897; use M_PI in math.h https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter-helpe... tv-spectrum-transmitter-helper.cc:446: originLatitude = 89.999; typically this would generate at least LOG_WARN or LOG_ERROR that bounds were exceeded, or cause assertion (if user error is suspected) https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter-helper.h File tv-spectrum-transmitter-helper.h (right): https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter-helpe... tv-spectrum-transmitter-helper.h:60: europe On 2014/09/24 14:28:00, n.p wrote: > same formatting thing here.. constant values should be recognisable NORTH_AMERICA JAPAN EUROPE https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter-helpe... tv-spectrum-transmitter-helper.h:71: }; LOW, MEDIUM, HIGH https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter-helpe... tv-spectrum-transmitter-helper.h:79: * @param a reference to the spectrum channel 'param c a pointer to the spectrum channel https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter-helpe... tv-spectrum-transmitter-helper.h:81: void SetChannel (Ptr<SpectrumChannel> c); As discussed, when the underlying model is converted to attributes, there is no need to plumb these individual setters/getters through the helper class; instead, the pattern of using SetAttribute can be used. e.g. from csma-helper.cc /** * \param n1 the name of the attribute to set * \param v1 the value of the attribute to set * * Set these attributes on each ns3::CsmaChannel created * by CsmaHelper::Install */ void SetChannelAttribute (std::string n1, const AttributeValue &v1); https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter-helpe... tv-spectrum-transmitter-helper.h:88: void SetTvType (TVSpectrumTransmitter::TvType); 'type' is missing from the function signature. Perhaps this is because it is a reserved keyword. Suggest to add 'tvType' argument (also to the doxygen @param) https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter-helpe... tv-spectrum-transmitter-helper.h:154: * @param a container containing the node(s) which the TV transmitter(s) will be attached to @param nodes a container... https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter-helpe... tv-spectrum-transmitter-helper.h:168: * @param the region of which the transmitter(s) will be characterized fix above params ('nodes', 'region') https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter-helpe... tv-spectrum-transmitter-helper.h:218: static Vector GeographicToCartesianCoordinates (double latitude, double longitude); This is a method that needs to be tested thoroughly, or perhaps provided by an external library. Should it be moved to mobility module? Doxygen is missing @return. https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter-helpe... tv-spectrum-transmitter-helper.h:229: * how many transmitters are created can you say a bit more about what density 'high', 'medium', and 'low' corresponds to? https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter.cc File tv-spectrum-transmitter.cc (right): https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter.cc#ne... tv-spectrum-transmitter.cc:26: NS_LOG_COMPONENT_DEFINE ("TVSpectrumTransmitter"); In general, can you replace "TVSpectrum" with "TvSpectrum" throughout this code? It is more conformant to our CamelCase style. https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter.cc#ne... tv-spectrum-transmitter.cc:72: SpectrumPhy::DoDispose (); On 2014/09/24 14:28:00, n.p wrote: > Are these reset required for some circular dependency ? If not ( I really don't > know the spectrum module) they are useless. Agree, I recommend to only zero out the Ptr variables, for clarity. https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter.cc#ne... tv-spectrum-transmitter.cc:88: NS_LOG_FUNCTION_NOARGS (); NS_LOG_FUNCTION (this << c); Do not use the NS_LOG_FUNCTION_NOARGS() variant; it is only intended for static methods. Use "NS_LOG_FUNCTION (this);" if there are no arguments https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter.cc#ne... tv-spectrum-transmitter.cc:159: NS_LOG_FUNCTION (this << bandwidth); inconsistent usage of logging; either log all the setter/getters or don't. I strongly agree to use attributes for these variables, and to delete the custom setter/getters unless they are expected to be called a lot by clients. https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter.cc#ne... tv-spectrum-transmitter.cc:323: TVSpectrumTransmitter::SetStartingTime (double timepoint) Rather than pass in a 'double', recommend to pass in an ns3::Time object https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter.cc#ne... tv-spectrum-transmitter.cc:325: NS_LOG_FUNCTION (this << timepoint); then this becomes "(this << timepoint.GetSeconds ())" https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter.cc#ne... tv-spectrum-transmitter.cc:330: TVSpectrumTransmitter::SetTransmitDuration (double duration) similar comment; store and manipulate time in your model with ns3::Time rather than double, where possible https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter.cc#ne... tv-spectrum-transmitter.cc:339: Ptr<SpectrumSignalParameters> signal = Create<SpectrumSignalParameters> (); missing NS_LOG_FUNCTION() https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter.h File tv-spectrum-transmitter.h (right): https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter.h#new... tv-spectrum-transmitter.h:58: cofdm On 2014/09/24 14:28:00, n.p wrote: > usually constant values should be recognisable. Example: I think what is meant is to capitalize these values. At least, that would be my suggestion here. ANALOG, VSB8, COFDM (minor nit, it should probably be "8VSB" instead of "VSB8") > enum SocketErrno { > ERROR_NOTERROR, > ERROR_ISCONN, > ERROR_NOTCONN, > ERROR_MSGSIZE, > ERROR_AGAIN, > ERROR_SHUTDOWN, > ERROR_OPNOTSUPP, > ERROR_AFNOSUPPORT, > ERROR_INVAL, > ERROR_BADF, > ERROR_NOROUTETOHOST, > ERROR_NODEV, > ERROR_ADDRNOTAVAIL, > ERROR_ADDRINUSE, > SOCKET_ERRNO_LAST > }; > > (from socket.h) https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter.h#new... tv-spectrum-transmitter.h:81: Ptr<SpectrumChannel> GetChannel (); GetChannel() const; https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter.h#new... tv-spectrum-transmitter.h:91: * Set the lower end frequency of the TV transmitter is there any floor on the allowable frequency? Please specify units (I assume Hz). https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter.h#new... tv-spectrum-transmitter.h:100: * @param bandwidth the bandwidth that will be transmitted Likewise, is any maximum bandwidth enforced, and also specify units. https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter.h#new... tv-spectrum-transmitter.h:105: * Set channel number to name the TV transmitter (channel 1, 2, 3, etc.) what does this do? If I set bandwidth, do I need to set this channel number? Or does setting the channel number obviate the need to set the frequency? Please clarify. https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter.h#new... tv-spectrum-transmitter.h:109: void SetChannelNumber (int num); uint type? (can't be negative, right?) https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter.h#new... tv-spectrum-transmitter.h:116: int GetChannelNumber (); any such getters should be const; https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter.h#new... tv-spectrum-transmitter.h:142: * @return a reference to the Power Spectral Density of the TV transmitter return a pointer https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter.h#new... tv-spectrum-transmitter.h:148: * Recommend using PSD created from CreateTVPSD(). this is unclear; do I need to CreateTVPSD then SetTxPSD? Why not one operation? Why are the names slightly different? (TVPSD vs TxPSD) https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter.h#new... tv-spectrum-transmitter.h:150: * @param txpsd the Power Spectral Density to be transmitted capitalization mismatch with parameter https://codereview.appspot.com/148810043/diff/1/tv-spectrum-transmitter.h#new... tv-spectrum-transmitter.h:155: * Set the AntennaModel to be used clarify, where does this antenna model come from? What are allowable antennas? What happens if user doesn't call this? https://codereview.appspot.com/148810043/diff/1/tv-trans-example.cc File tv-trans-example.cc (right): https://codereview.appspot.com/148810043/diff/1/tv-trans-example.cc#newcode54 tv-trans-example.cc:54: nodePositionList->Add (Vector (1.0, 0.0, 0.0)); // TV Transmitter 1 what are the units on these positions? Are they realistic?
Sign in to reply to this message.
some additional comments on the second patch https://codereview.appspot.com/148810043/diff/20001/spectrum.rst File spectrum.rst (right): https://codereview.appspot.com/148810043/diff/20001/spectrum.rst#newcode480 spectrum.rst:480: be created and transmitted on the channel. you have academic citations in the doxygen regarding how you derived the psd values; they should be copied here as well. https://codereview.appspot.com/148810043/diff/20001/spectrum.rst#newcode500 spectrum.rst:500: generated. this documentation can be enhanced in three ways. 1) the PDF has some nice images that are missing here; either insert also the images here also, or provide a pointer to an online location where that document can be obtained 2) more information about the assumptions or policies that you have coded. Especially, list the constraints or simplications. For example - the model uses a simple geoid spherical earth model, assuming the height of the antenna is zero above earth's radius - no propagation loss model or directional antenna model is inserted by default - bodies of water are not considered in the layout of transmitters - no terrain effects or spherical earth (horizon) effects are used in propagation loss considerations 3) brief comment about the examples you have provided, and how the code was validated (e.g. by inspection of test values, by comparison against a geographic library, etc.) ... https://codereview.appspot.com/148810043/diff/20001/tv-spectrum-transmitter-h... File tv-spectrum-transmitter-helper.cc (right): https://codereview.appspot.com/148810043/diff/20001/tv-spectrum-transmitter-h... tv-spectrum-transmitter-helper.cc:302: double longitude) this could be moved to mobility module, perhaps in a conversion class that has static methods (e.g. ConvertToCartesian::Geographic (double latitude, double longitude)), and unit tested against values produced by another library (e.g. GeographicLib) (define some kind of tolerance and compare the values produced with this code vs. the values of the library). It should also take an altitude rather than assume earth's radius; perhaps the argument could default to earth's radius). https://codereview.appspot.com/148810043/diff/20001/tv-spectrum-transmitter-h... tv-spectrum-transmitter-helper.cc:368: Ptr<UniformRandomVariable> uniRand = CreateObject<UniformRandomVariable> (); make this a class member (m_uniRand). Create it in the constructor. Provide an AssignStreams() method to allow user to fix the stream number for the random variable (see random variable documentation). https://codereview.appspot.com/148810043/diff/20001/tv-spectrum-transmitter-h... tv-spectrum-transmitter-helper.cc:377: randNumTransmitters = (int)(uniRand->GetValue ()) % (int)(ceil((0.33) * uniform random variables have a GetInteger() method that avoids needing the cast on the client side https://codereview.appspot.com/148810043/diff/20001/tv-spectrum-transmitter-h... tv-spectrum-transmitter-helper.cc:388: freqVectorSize)) + (int)(ceil((0.66) * freqVectorSize)) + 1; this is an example of the type of arithmetic that should be unit tested. Isolate the arithmetic inside a method such as GetRandomNumTransmitters(Density d, uint32_t freqVectorSize, uint32_t randomInt), then write a unit test that is somewhat documenting of the function in that it shows the reader the expected output given the input. Having this in a method allows you to write doxygen on the method so the user understands better how the distribution is being made. https://codereview.appspot.com/148810043/diff/20001/tv-spectrum-transmitter-h... tv-spectrum-transmitter-helper.cc:422: TvSpectrumTransmitterHelper::RandGeographicCoordsAroundPoint(double originLatitude, the name of this method might be more accurate as "RandCartesianCoordsAroundGeographicPoint()" https://codereview.appspot.com/148810043/diff/20001/tv-spectrum-transmitter-h... File tv-spectrum-transmitter-helper.h (right): https://codereview.appspot.com/148810043/diff/20001/tv-spectrum-transmitter-h... tv-spectrum-transmitter-helper.h:168: * selected from attributes. clarify what is meant by "Adjacent" in the method name. If two nodes are consecutive in the NodeContainer, in what way are they adjacent? https://codereview.appspot.com/148810043/diff/20001/tv-spectrum-transmitter-h... tv-spectrum-transmitter-helper.h:170: * Must set ChannelNumber attribute for this method to work properly. clarify this statement; what happens if user doesn't set ChannelNumber attribute? What is the default used? How can you protect against this condition? https://codereview.appspot.com/148810043/diff/20001/tv-spectrum-transmitter-h... tv-spectrum-transmitter-helper.h:171: * If multiple transmitters created, they will have same settings except for by same settings, do you mean same location? https://codereview.appspot.com/148810043/diff/20001/tv-spectrum-transmitter-h... tv-spectrum-transmitter-helper.h:204: * random TV transmitters on earth's surface around a given origin point clarify this a bit. instead of "random TV transmitters" say something like "TV transmitters located at randomly selected points centered around a given origin point, corresponding to a uniform distribution and radius". https://codereview.appspot.com/148810043/diff/20001/tv-spectrum-transmitter-h... tv-spectrum-transmitter-helper.h:206: * Channel numbers are random, with channel frequencies corresponding to again, more precise about what is 'random' about the channel numbers. https://codereview.appspot.com/148810043/diff/20001/tv-spectrum-transmitter-h... tv-spectrum-transmitter-helper.h:215: * simulated, which determines how many transmitters are created. Low density transmitters are created and how many channels are occupied. Low density https://codereview.appspot.com/148810043/diff/20001/tv-spectrum-transmitter-h... tv-spectrum-transmitter-helper.h:233: * Generates random points on earth's surface around a given origin point Generates points uniformly distributed on earth's surface. By the way, I think you should probably generate antenna heights above earth's radius in case someone later tries to use this with terrain-aware or horizon-aware propagation loss models. Height above average terrain is a key propagation factor for television. https://codereview.appspot.com/148810043/diff/20001/tv-spectrum-transmitter-t... File tv-spectrum-transmitter-test.cc (right): https://codereview.appspot.com/148810043/diff/20001/tv-spectrum-transmitter-t... tv-spectrum-transmitter-test.cc:22: #include <ns3/core-module.h> avoid using "*-module" headers in tests; just include what you need https://codereview.appspot.com/148810043/diff/20001/tv-spectrum-transmitter-t... tv-spectrum-transmitter-test.cc:197: TestCase::QUICK); I am not sure about the need to sweep all of these values. It seems that there will be a lot of redundancy, checking that the values in TvSpectrumTransmitter::CreateTvPsd() were assigned correctly (again and again). Other ideas for tests: - effect of varying the antenna model - do starting time and transmit duration work as expected? - place a transmitter at some location and receiver at some distance, and confirm the received PSD (such as validating the received values in the tv-trans-example program) - test the distribution of transmitters that results from the helper https://codereview.appspot.com/148810043/diff/20001/tv-trans-example.cc File tv-trans-example.cc (left): https://codereview.appspot.com/148810043/diff/20001/tv-trans-example.cc#oldco... tv-trans-example.cc:89: spectrumAnalyzerHelper.SetChannel (channel); can this example be enhanced to use a propagation loss model? One doesn't seem to be included by default.
Sign in to reply to this message.
this looks nearly ready to me; well done and well documented. the one thing that still needs some work is how the random variables are handled. https://codereview.appspot.com/148810043/diff/40001/geo-to-cartesian-test.cc File geo-to-cartesian-test.cc (right): https://codereview.appspot.com/148810043/diff/40001/geo-to-cartesian-test.cc#... geo-to-cartesian-test.cc:41: const double TOLERANCE = 10; please document this; what are the units? https://codereview.appspot.com/148810043/diff/40001/geographic-positions.cc File geographic-positions.cc (right): https://codereview.appspot.com/148810043/diff/40001/geographic-positions.cc#n... geographic-positions.cc:22: #include <math.h> we started to prefer cmath over math.h (see https://www.nsnam.org/bugzilla/show_bug.cgi?id=1276), can it be changed? https://codereview.appspot.com/148810043/diff/40001/geographic-positions.cc#n... geographic-positions.cc:57: { static members typically use a NS_LOG_FUNCTION_NOARGS() for function logging. https://codereview.appspot.com/148810043/diff/40001/geographic-positions.cc#n... geographic-positions.cc:94: // fixes divide by zero case and limits latitude bounds NS_LOG_FUNCTION_NOARGS (); https://codereview.appspot.com/148810043/diff/40001/geographic-positions.cc#n... geographic-positions.cc:104: } should nonsensical altitudes also be checked? What is assumed to be a valid range? https://codereview.appspot.com/148810043/diff/40001/rand-cart-around-geo-test.cc File rand-cart-around-geo-test.cc (right): https://codereview.appspot.com/148810043/diff/40001/rand-cart-around-geo-test... rand-cart-around-geo-test.cc:50: const double TOLERANCE = 10; please add a comment here about the units on this constant, and why '10' was chosen https://codereview.appspot.com/148810043/diff/40001/rand-cart-around-geo-test... rand-cart-around-geo-test.cc:110: GeographicPositions::AssignStreams (-1); this is not necessary; the default is -1. But probably what should instead be done is to fix it to a positive stream. https://codereview.appspot.com/148810043/diff/40001/rand-cart-around-geo-test... rand-cart-around-geo-test.cc:121: for (int i = 0; i < (int)points.size (); i++) small nit: simpler loop (to avoid the integer cast) would be 'while (!points.empty ()) {..}' https://codereview.appspot.com/148810043/diff/40001/tv-spectrum-transmitter-h... File tv-spectrum-transmitter-helper.cc (left): https://codereview.appspot.com/148810043/diff/40001/tv-spectrum-transmitter-h... tv-spectrum-transmitter-helper.cc:28: #include <ns3/mobility-module.h> the -module.h module headers are for user programs only; include only geographic-positions.h here (or whatever else is needed) https://codereview.appspot.com/148810043/diff/40001/tv-spectrum-transmitter-h... File tv-spectrum-transmitter-helper.cc (right): https://codereview.appspot.com/148810043/diff/40001/tv-spectrum-transmitter-h... tv-spectrum-transmitter-helper.cc:337: GeographicPositions::AssignStreams (m_uniRand->GetStream ()); this is going to result in deleting and recreating the underlying random variable each time it is called (because RandomVariableStream::SetStream() eventually is called). it may be better design to do one of two things: - make GeographicPositions a regular object, and have this helper instantiate such object and call methods on it, including calling AssignStreams () once on it. - make GeographicPositions require the client to pass in a pointer to the random variable (I tend to like this approach better; it makes GeographicPositions stateless) https://codereview.appspot.com/148810043/diff/40001/tv-spectrum-transmitter-h... File tv-spectrum-transmitter-helper.h (right): https://codereview.appspot.com/148810043/diff/40001/tv-spectrum-transmitter-h... tv-spectrum-transmitter-helper.h:209: * @param streamNum the stream number to be set missing '@return the number of streams used'
Sign in to reply to this message.
The biggest issue for me is the use of non-static instances of SpectrumModel (see detailed comments below). If it can be fixed then for me it's definitely ok to merge. Eventually we could think of merging without fixing it - the code is working properly for the purpose it's built for, it might just give trouble if someone wants to extend it - but at least I'd file a bug against this issue. https://codereview.appspot.com/148810043/diff/20001/tv-spectrum-transmitter.cc File tv-spectrum-transmitter.cc (right): https://codereview.appspot.com/148810043/diff/20001/tv-spectrum-transmitter.c... tv-spectrum-transmitter.cc:229: Ptr<SpectrumModel> model = Create<SpectrumModel> (bands); This approach will end up with a different SpectrumModel for each TvSpectrumTransmitter, even in the case where two transmitter are exactly on the same frequency and bandwidth. If TvTransmitter is used only as a source of interference, this will work, in the sense that MultiModelSpectrumChannel will properly calculate interference for any receiver using any spectrum model. However, this can cause inefficient spectrum model conversion processing, and can result in weird behavior in case somebody ends up testing if two transmitters are using the same SpectrumModel. Ideally, this should be changed to have statically allocated SpectrumModel instances, as described in the docs: http://www.nsnam.org/docs/models/html/spectrum.html#usage Also see how other SpectrumModel are allocated in the code base, it's practically always statical instances. See for example microwave-oven-spectrum-value-helper, wifi-spectrum-value-helper, and especially lte-spectrum-helper (the latter is the most similar to yours in terms of complexity of the model). https://codereview.appspot.com/148810043/diff/20001/tv-spectrum-transmitter.c... tv-spectrum-transmitter.cc:232: if (m_tvType == TVTYPE_8VSB) please use a switch statement here https://codereview.appspot.com/148810043/diff/20001/tv-spectrum-transmitter.c... tv-spectrum-transmitter.cc:314: else //TVTYPE_ANALOG if you really want that any unknown TV type is treated as TVTYPE_ANALOG, please add this as a "default:" case. Otherwise, make "default:" trigger an error message.
Sign in to reply to this message.
|
