Code review - Issue 249560043: LR-WPAN Energy Modelhttps://codereview.appspot.com/2015-07-25T10:27:04+00:00rietveld
Message from unknown
2015-07-06T17:22:34+00:00vregeurn:md5:61dc62fbbfe83254feca5eaed387516d
Message from tomh.org@gmail.com
2015-07-07T13:56:14+00:00Tom Hendersonurn:md5:25d32b1e606b34ee37c037dff34d17c7
Vishwesh, my main comment is the lack of tests in src/lr-wpan/test. Also, please find a good place to extend lr-wpan.rst documentation with a paragraph or two about energy modeling in Lr-Wpan. Otherwise, it looks like it is close to being done.
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/examples/lr-wpan-data-energy-model.cc
File src/lr-wpan/examples/lr-wpan-data-energy-model.cc (right):
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/examples/lr-wpan-data-energy-model.cc#newcode16
src/lr-wpan/examples/lr-wpan-data-energy-model.cc:16: * Author: Vishwesh Rege <vrege2012@gmail.com>
this is an adaptation of lr-wpan-data.cc (it is substantially the same), so you must preserve the copyrights of that file. You can add yourself as an author.
However, I would rather see that you just extend 'lr-wpan-data.cc' than create this very similar example that just has energy enhancements.
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/examples/lr-wpan-energy-test.cc
File src/lr-wpan/examples/lr-wpan-energy-test.cc (right):
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/examples/lr-wpan-energy-test.cc#newcode18
src/lr-wpan/examples/lr-wpan-energy-test.cc:18:
Please include some comments describing to the user what this program does and what is an example of a successful execution of it.
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/examples/lr-wpan-energy-test.cc#newcode44
src/lr-wpan/examples/lr-wpan-energy-test.cc:44: << " from " << oldValue << "\n";
I'd like to see this program as a test suite (src/lr-wpan/test). It can be copied over if you prefer (it can be kept as an example also) but refactored into the test program format in the test directory.
You can enforce in this function that the value of remaining energy is expected for each time; logically, you write a switch statement and use the ns-3 test macros:
switch (Simulator::Now())
{
case Seconds (1):
NS_TEST_ASSERT_MSG_EQ (newValue, expectedNewValue, "Energy check failure for time 1 second");
break;
case Seconds (2):
...
}
Since you are comparing floating point values, you probably want to use the "TOL" variants of these macros; have a look at NS_TEST_ASSERT_MSG_EQ_TOL()
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/model/lr-wpan-phy.cc
File src/lr-wpan/model/lr-wpan-phy.cc (right):
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/model/lr-wpan-phy.cc#newcode50
src/lr-wpan/model/lr-wpan-phy.cc:50: const double LrWpanPhy::rxSetupTime = 0.001792; // The switching delay times from IDLE to SLEEP and SLEEP to IDLE in uS
state where this constant came from
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/model/lr-wpan-phy.cc#newcode259
src/lr-wpan/model/lr-wpan-phy.cc:259: }
Please check your indentation in this file, it is not aligned with ns-3 style.
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/model/lr-wpan-phy.cc#newcode705
src/lr-wpan/model/lr-wpan-phy.cc:705: Time ccaTime = Seconds (8.0 / GetDataOrSymbolRate (false)); //ccaTime given by the data sheet of the CC2420 radio transceiver is 0.192 milliseconds/12 symbol periods (the time required for a stable RSSI, needed for a stable CCA indication)
what is the point of this clarifying comment? it doesn't seem to match the code.
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/model/lr-wpan-phy.cc#newcode859
src/lr-wpan/model/lr-wpan-phy.cc:859: /*
remove dead code, don't comment it out.
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/model/lr-wpan-phy.cc#newcode919
src/lr-wpan/model/lr-wpan-phy.cc:919: Time setupTime = Seconds ( (double) rxSetupTime);
should rxSetupTime just be stored as a Time, not a double?
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/model/lr-wpan-phy.cc#newcode1040
src/lr-wpan/model/lr-wpan-phy.cc:1040: //m_trxState = IEEE_802_15_4_PHY_TRX_OFF;
remove dead code
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/model/lr-wpan-phy.h
File src/lr-wpan/model/lr-wpan-phy.h (right):
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/model/lr-wpan-phy.h#newcode294
src/lr-wpan/model/lr-wpan-phy.h:294: * Handles energy depletion.
this Doxygen is too brief, and some methods are missing it below
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/model/lr-wpan-radio-energy-model.cc
File src/lr-wpan/model/lr-wpan-radio-energy-model.cc (right):
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/model/lr-wpan-radio-energy-model.cc#newcode44
src/lr-wpan/model/lr-wpan-radio-energy-model.cc:44: MakeDoubleChecker<double> ())
you can enforce that this never goes below zero (also, never is set to zero, which might cause divide by zero exceptions?) with the MakeDoubleChecker<double> (). e.g. try MakeDoubleChecker<double> (DBL_MIN, DBL_MAX))
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/model/lr-wpan-radio-energy-model.cc#newcode59
src/lr-wpan/model/lr-wpan-radio-energy-model.cc:59: MakeTraceSourceAccessor (&LrWpanRadioEnergyModel::m_totalEnergyConsumption),
is m_totalEnergyConsumption initialized to zero anywhere? Suggest to try adding it to the constructor member initializers.
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/model/lr-wpan-radio-energy-model.cc#newcode71
src/lr-wpan/model/lr-wpan-radio-energy-model.cc:71: m_source = NULL; // EnergySource
in general, C++ best practice is to initialize members in the member initialization list, not in the body of the constructor. The reason is that the compiler will generate default member initializers, so you end up assigning it twice (which is normally not a big deal unless constructing the object is expensive).
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/model/lr-wpan-radio-energy-model.cc#newcode216
src/lr-wpan/model/lr-wpan-radio-energy-model.cc:216: // EnergySource::NotifyEnergyDrained -> LrWpanRadioEnergyModel::HandleEnergyDepletion -> LrWpanPhy::EnergyDepletionHandler
should you write some kind of a test enforcing the above?
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/model/lr-wpan-radio-energy-model.cc#newcode220
src/lr-wpan/model/lr-wpan-radio-energy-model.cc:220: // some debug message
this comment is unnecessary
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/model/lr-wpan-radio-energy-model.cc#newcode235
src/lr-wpan/model/lr-wpan-radio-energy-model.cc:235: if (m_phy != NULL)
what situation exists where m_phy is OK to be NULL? By the way, we typically use '0' instead of NULL for pointer checks (see the ns-3 coding style guide for why)
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/model/lr-wpan-radio-energy-model.cc#newcode291
src/lr-wpan/model/lr-wpan-radio-energy-model.cc:291: NS_FATAL_ERROR ("LrWpanRadioEnergyModel:Undefined radio state:" << m_nextState);
place a 'return 0' statement after your FATAL_ERROR statements (also below); some compilers will complain that there is no return statement.
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/model/lr-wpan-radio-energy-model.cc#newcode368
src/lr-wpan/model/lr-wpan-radio-energy-model.cc:368: m_nextState = (LrWpanPhy::State) nextState;
why is this cast needed? Why isn't nextState already a LrWpanPhy::State?
Message from vrege2012@gmail.com
2015-07-25T10:27:04+00:00vregeurn:md5:ac4fd472e929346611d6231a5ae028fb
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/examples/lr-wpan-data-energy-model.cc
File src/lr-wpan/examples/lr-wpan-data-energy-model.cc (right):
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/examples/lr-wpan-data-energy-model.cc#newcode16
src/lr-wpan/examples/lr-wpan-data-energy-model.cc:16: * Author: Vishwesh Rege <vrege2012@gmail.com>
On 2015/07/07 13:56:12, Tom Henderson wrote:
> this is an adaptation of lr-wpan-data.cc (it is substantially the same), so you
> must preserve the copyrights of that file. You can add yourself as an author.
>
> However, I would rather see that you just extend 'lr-wpan-data.cc' than create
> this very similar example that just has energy enhancements.
Done.
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/model/lr-wpan-phy.cc
File src/lr-wpan/model/lr-wpan-phy.cc (right):
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/model/lr-wpan-phy.cc#newcode50
src/lr-wpan/model/lr-wpan-phy.cc:50: const double LrWpanPhy::rxSetupTime = 0.001792; // The switching delay times from IDLE to SLEEP and SLEEP to IDLE in uS
On 2015/07/07 13:56:13, Tom Henderson wrote:
> state where this constant came from
rxSetupTime is the On/Off switching time of radio transceivers. Since the value is hardware dependent, I have changed it to a Time attribute. I found the corresponding value for CC2420 in the following paper (it isn't available in the datasheet):
http://www.anacom.pt/streaming/Impact_Switching_7congressURSI.pdf?contentId=1182494&field=ATTACHED_FILE
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/model/lr-wpan-phy.cc#newcode259
src/lr-wpan/model/lr-wpan-phy.cc:259: }
On 2015/07/07 13:56:13, Tom Henderson wrote:
> Please check your indentation in this file, it is not aligned with ns-3 style.
Done.
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/model/lr-wpan-phy.cc#newcode859
src/lr-wpan/model/lr-wpan-phy.cc:859: /*
On 2015/07/07 13:56:13, Tom Henderson wrote:
> remove dead code, don't comment it out.
Done.
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/model/lr-wpan-phy.cc#newcode919
src/lr-wpan/model/lr-wpan-phy.cc:919: Time setupTime = Seconds ( (double) rxSetupTime);
On 2015/07/07 13:56:12, Tom Henderson wrote:
> should rxSetupTime just be stored as a Time, not a double?
Done.
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/model/lr-wpan-phy.cc#newcode1040
src/lr-wpan/model/lr-wpan-phy.cc:1040: //m_trxState = IEEE_802_15_4_PHY_TRX_OFF;
On 2015/07/07 13:56:13, Tom Henderson wrote:
> remove dead code
Done.
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/model/lr-wpan-phy.h
File src/lr-wpan/model/lr-wpan-phy.h (right):
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/model/lr-wpan-phy.h#newcode294
src/lr-wpan/model/lr-wpan-phy.h:294: * Handles energy depletion.
On 2015/07/07 13:56:13, Tom Henderson wrote:
> this Doxygen is too brief, and some methods are missing it below
Done.
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/model/lr-wpan-radio-energy-model.cc
File src/lr-wpan/model/lr-wpan-radio-energy-model.cc (right):
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/model/lr-wpan-radio-energy-model.cc#newcode59
src/lr-wpan/model/lr-wpan-radio-energy-model.cc:59: MakeTraceSourceAccessor (&LrWpanRadioEnergyModel::m_totalEnergyConsumption),
On 2015/07/07 13:56:13, Tom Henderson wrote:
> is m_totalEnergyConsumption initialized to zero anywhere? Suggest to try adding
> it to the constructor member initializers.
Done.
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/model/lr-wpan-radio-energy-model.cc#newcode71
src/lr-wpan/model/lr-wpan-radio-energy-model.cc:71: m_source = NULL; // EnergySource
On 2015/07/07 13:56:13, Tom Henderson wrote:
> in general, C++ best practice is to initialize members in the member
> initialization list, not in the body of the constructor. The reason is that the
> compiler will generate default member initializers, so you end up assigning it
> twice (which is normally not a big deal unless constructing the object is
> expensive).
Thanks for the tip.
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/model/lr-wpan-radio-energy-model.cc#newcode220
src/lr-wpan/model/lr-wpan-radio-energy-model.cc:220: // some debug message
On 2015/07/07 13:56:14, Tom Henderson wrote:
> this comment is unnecessary
Done.
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/model/lr-wpan-radio-energy-model.cc#newcode235
src/lr-wpan/model/lr-wpan-radio-energy-model.cc:235: if (m_phy != NULL)
On 2015/07/07 13:56:13, Tom Henderson wrote:
> what situation exists where m_phy is OK to be NULL? By the way, we typically
> use '0' instead of NULL for pointer checks (see the ns-3 coding style guide for
> why)
Done.
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/model/lr-wpan-radio-energy-model.cc#newcode291
src/lr-wpan/model/lr-wpan-radio-energy-model.cc:291: NS_FATAL_ERROR ("LrWpanRadioEnergyModel:Undefined radio state:" << m_nextState);
On 2015/07/07 13:56:13, Tom Henderson wrote:
> place a 'return 0' statement after your FATAL_ERROR statements (also below);
> some compilers will complain that there is no return statement.
Fixed this... but there are a lot of places in the existing code where this doesn't seem to be followed?
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/model/lr-wpan-radio-energy-model.cc#newcode368
src/lr-wpan/model/lr-wpan-radio-energy-model.cc:368: m_nextState = (LrWpanPhy::State) nextState;
On 2015/07/07 13:56:13, Tom Henderson wrote:
> why is this cast needed? Why isn't nextState already a LrWpanPhy::State?
NotifyTransition implements LrWpanPhyListener::NotifyTransition (int). The argument isn't a LrWpanPhy::State as LrWpanPhy::State isn't defined at the time the function is declared.