Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1511)

Issue 249560043: LR-WPAN Energy Model

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 2 months ago by vrege
Modified:
4 years, 1 month ago
Reviewers:
Tom Henderson
CC:
ns-3-reviews_googlegroups.com
Visibility:
Public.

Description

LR-WPAN Energy Model * Code review issue for LR WPAN energy model * LrWpanPhyListener class, Energy model states, Phy state transitions - lr-wpan-phy.h - lr-wpan-phy.cc * Energy Model - lr-wpan-radio-energy-model.cc - lr-wpan-radio-energy-model.h * Tests/examples **Simple test with the radio in Tx, Rx and Off state for few seconds. - lr-wpan-energy-test.cc **Modified lr-wpan-data example to use energy model - lr-wpan-data-energy-model.cc

Patch Set 1 #

Total comments: 31
Unified diffs Side-by-side diffs Delta from patch set Stats (+1081 lines, -8 lines) Patch
M src/energy/model/basic-energy-source.cc View 1 chunk +9 lines, -1 line 0 comments Download
A src/lr-wpan/examples/lr-wpan-data-energy-model.cc View 1 chunk +179 lines, -0 lines 2 comments Download
A src/lr-wpan/examples/lr-wpan-energy-test.cc View 1 chunk +86 lines, -0 lines 2 comments Download
M src/lr-wpan/helper/lr-wpan-helper.cc View 1 chunk +1 line, -0 lines 0 comments Download
M src/lr-wpan/model/lr-wpan-phy.h View 6 chunks +61 lines, -0 lines 2 comments Download
M src/lr-wpan/model/lr-wpan-phy.cc View 13 chunks +157 lines, -6 lines 11 comments Download
A src/lr-wpan/model/lr-wpan-radio-energy-model.h View 1 chunk +211 lines, -0 lines 0 comments Download
A src/lr-wpan/model/lr-wpan-radio-energy-model.cc View 1 chunk +374 lines, -0 lines 14 comments Download
M src/lr-wpan/wscript View 3 chunks +3 lines, -1 line 0 comments Download

Messages

Total messages: 2
Tom Henderson
Vishwesh, my main comment is the lack of tests in src/lr-wpan/test. Also, please find a ...
4 years, 2 months ago (2015-07-07 13:56:14 UTC) #1
vrege
4 years, 1 month ago (2015-07-25 10:27:04 UTC) #2
https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/examples/lr-wpan-...
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-...
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...
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=1...

https://codereview.appspot.com/249560043/diff/1/src/lr-wpan/model/lr-wpan-phy...
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...
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...
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...
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...
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-rad...
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-rad...
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-rad...
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-rad...
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-rad...
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-rad...
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-rad...
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b