UAN Framework project (GSoC) final review

[lentracy, 2010-08-09 05:49:47]

I don't have much in the way of new comments on this code.

I've made one note where I believe we keep separate .h/.cc files even in
examples now.

The couple of things I would like to see:

An update to uan.h or something similar that will provide an overview of the new
models.  Separate overviews for the new energy models and the mobility models
might be more appropriate, i.e. something in /mobility like auv-mobility +
something similar for the energy model). There could be links from uan.h to this
other documentation.  This would also be a good place to link the code to the
wiki page, so that future work can reference the design goals.

I believe the above documentation will be helpful in others reviewing the code
as well.

http://codereview.appspot.com/1743057/diff/1/7
File examples/uan/uan-energy-auv.cc (right):

http://codereview.appspot.com/1743057/diff/1/7#newcode37
examples/uan/uan-energy-auv.cc:37: class UanEnergyAuv
I received comments in main UAN code to move classes to .h files, even in
examples.  You may want to preempt such comments before merge time.

[Tom Henderson, 2010-08-14 22:27:11]

This looks nicely done in general.  I had a few questions in the comments.  
- document what you are doing to WaypointMobilityModel (and maybe discuss with
Phillip)
- should the existing modem profile be hardcoded into the Uan Phy
- what happens when all energy is drained?

plus a few others.

Also, please remember to do something for the manual.

http://codereview.appspot.com/1743057/diff/7001/8001
File examples/mobility/auv-energy-model.cc (right):

http://codereview.appspot.com/1743057/diff/7001/8001#newcode65
examples/mobility/auv-energy-model.cc:65: DoubleValue (10000000));
I am not sure we need to use the full class name as a prefix to attribute names
(seems verbose)

http://codereview.appspot.com/1743057/diff/7001/8014
File src/contrib/energy/model/micro-modem-energy-model.h (right):

http://codereview.appspot.com/1743057/diff/7001/8014#newcode76
src/contrib/energy/model/micro-modem-energy-model.h:76: };
I see also a UanPhy::State enum that is similar-- should the existing one be
extended to include SLEEP state?

http://codereview.appspot.com/1743057/diff/7001/8015
File src/contrib/energy/model/remus-energy-model.cc (right):

http://codereview.appspot.com/1743057/diff/7001/8015#newcode178
src/contrib/energy/model/remus-energy-model.cc:178: double power = speed * 10;
this magic number 10 would make a good Attribute

http://codereview.appspot.com/1743057/diff/7001/8017
File src/contrib/energy/model/simple-device-energy-model.cc (right):

http://codereview.appspot.com/1743057/diff/7001/8017#newcode71
src/contrib/energy/model/simple-device-energy-model.cc:71:
m_totalEnergyConsumption += energyToDecrease;
I don't really see how this is to work.  If you set a current, aren't you
setting a continual drain on the device?  Yet, this callback only gets fired
when user calls SetCurrentA().  

I would have expected to see some kind of periodic updating such as scheduling
this event for some time in the future so that there were continual periodic
updates.

http://codereview.appspot.com/1743057/diff/7001/8022
File src/devices/uan/uan-phy-gen.cc (right):

http://codereview.appspot.com/1743057/diff/7001/8022#newcode488
src/devices/uan/uan-phy-gen.cc:488: MicroModemEnergyModel::MicroModemState
destState;
It seems like you are hard coding a particular WHOI modem into the UanPhy.  What
happens when a new modem comes along?  Are all future modem energy models
expected to subclass from MicroModemEnergyModel?

http://codereview.appspot.com/1743057/diff/7001/8022#newcode545
src/devices/uan/uan-phy-gen.cc:545: UpdatePowerConsumption (TX);
I don't see any handling of what happens when energy is exhausted on the node. 
Can you define what you expect to happen, then write an example or testcase that
shows a node draining all energy before simulation ends?

http://codereview.appspot.com/1743057/diff/7001/8043
File src/mobility/waypoint-mobility-model.h (right):

http://codereview.appspot.com/1743057/diff/7001/8043#newcode29
src/mobility/waypoint-mobility-model.h:29: #include
"ns3/constant-velocity-mobility-model.h"
I don't think these ns3 changes are strictly needed.

http://codereview.appspot.com/1743057/diff/7001/8043#newcode60
src/mobility/waypoint-mobility-model.h:60: * ConstantVelocityMobilityModel or a
child of it.
This is not well explained; why are you adding a second mobility model?

http://codereview.appspot.com/1743057/diff/7001/8043#newcode82
src/mobility/waypoint-mobility-model.h:82: Ptr<ConstantVelocityMobilityModel>
GetMobilityModel (void) const;
should it be more descriptive method name:  GetValidatorMobilityModel?

[lentracy_u.washington.edu, 2010-08-17 03:13:25]

Andrea,

Were you able to respond to Tom's comments?  I realize the coding portion is
over, but can we set some timeframe to address anything that must be
addressed before commits.

I think another useful section of the wiki would be suggested future
enhancements that you might have.  Perhaps a more generic interface between
UanPhy and the Energy model would fit into those lines (It's one I was also
considering).

Thanks,
Leonard

On Sat, Aug 14, 2010 at 3:27 PM, <tomh.org@gmail.com> wrote:

> This looks nicely done in general.  I had a few questions in the
> comments.
> - document what you are doing to WaypointMobilityModel (and maybe
> discuss with Phillip)
> - should the existing modem profile be hardcoded into the Uan Phy
> - what happens when all energy is drained?
>
> plus a few others.
>
> Also, please remember to do something for the manual.
>
>
>
> http://codereview.appspot.com/1743057/diff/7001/8001
> File examples/mobility/auv-energy-model.cc (right):
>
> http://codereview.appspot.com/1743057/diff/7001/8001#newcode65
> examples/mobility/auv-energy-model.cc:65: DoubleValue (10000000));
> I am not sure we need to use the full class name as a prefix to
> attribute names (seems verbose)
>
> http://codereview.appspot.com/1743057/diff/7001/8014
> File src/contrib/energy/model/micro-modem-energy-model.h (right):
>
> http://codereview.appspot.com/1743057/diff/7001/8014#newcode76
> src/contrib/energy/model/micro-modem-energy-model.h:76: };
> I see also a UanPhy::State enum that is similar-- should the existing
> one be extended to include SLEEP state?
>
> http://codereview.appspot.com/1743057/diff/7001/8015
> File src/contrib/energy/model/remus-energy-model.cc (right):
>
> http://codereview.appspot.com/1743057/diff/7001/8015#newcode178
> src/contrib/energy/model/remus-energy-model.cc:178: double power = speed
> * 10;
> this magic number 10 would make a good Attribute
>
> http://codereview.appspot.com/1743057/diff/7001/8017
> File src/contrib/energy/model/simple-device-energy-model.cc (right):
>
> http://codereview.appspot.com/1743057/diff/7001/8017#newcode71
> src/contrib/energy/model/simple-device-energy-model.cc:71:
> m_totalEnergyConsumption += energyToDecrease;
> I don't really see how this is to work.  If you set a current, aren't
> you setting a continual drain on the device?  Yet, this callback only
> gets fired when user calls SetCurrentA().
>
> I would have expected to see some kind of periodic updating such as
> scheduling this event for some time in the future so that there were
> continual periodic updates.
>
> http://codereview.appspot.com/1743057/diff/7001/8022
> File src/devices/uan/uan-phy-gen.cc (right):
>
> http://codereview.appspot.com/1743057/diff/7001/8022#newcode488
> src/devices/uan/uan-phy-gen.cc:488:
> MicroModemEnergyModel::MicroModemState destState;
> It seems like you are hard coding a particular WHOI modem into the
> UanPhy.  What happens when a new modem comes along?  Are all future
> modem energy models expected to subclass from MicroModemEnergyModel?
>
> http://codereview.appspot.com/1743057/diff/7001/8022#newcode545
> src/devices/uan/uan-phy-gen.cc:545: UpdatePowerConsumption (TX);
> I don't see any handling of what happens when energy is exhausted on the
> node.  Can you define what you expect to happen, then write an example
> or testcase that shows a node draining all energy before simulation
> ends?
>
> http://codereview.appspot.com/1743057/diff/7001/8043
> File src/mobility/waypoint-mobility-model.h (right):
>
> http://codereview.appspot.com/1743057/diff/7001/8043#newcode29
> src/mobility/waypoint-mobility-model.h:29: #include
> "ns3/constant-velocity-mobility-model.h"
> I don't think these ns3 changes are strictly needed.
>
> http://codereview.appspot.com/1743057/diff/7001/8043#newcode60
> src/mobility/waypoint-mobility-model.h:60: *
> ConstantVelocityMobilityModel or a child of it.
> This is not well explained; why are you adding a second mobility model?
>
> http://codereview.appspot.com/1743057/diff/7001/8043#newcode82
> src/mobility/waypoint-mobility-model.h:82:
> Ptr<ConstantVelocityMobilityModel> GetMobilityModel (void) const;
> should it be more descriptive method name:  GetValidatorMobilityModel?
>
>
> http://codereview.appspot.com/1743057/show
>

[socket, 2010-08-17 17:02:11]

http://codereview.appspot.com/1743057/diff/7001/8001
File examples/mobility/auv-energy-model.cc (right):

http://codereview.appspot.com/1743057/diff/7001/8001#newcode65
examples/mobility/auv-energy-model.cc:65: DoubleValue (10000000));
On 2010/08/14 22:27:11, Tom Henderson wrote:
> I am not sure we need to use the full class name as a prefix to attribute
names
> (seems verbose)

Only followed the guidelines from Tony. This can be changed to something like
InitialEnergyJ

http://codereview.appspot.com/1743057/diff/7001/8014
File src/contrib/energy/model/micro-modem-energy-model.h (right):

http://codereview.appspot.com/1743057/diff/7001/8014#newcode76
src/contrib/energy/model/micro-modem-energy-model.h:76: };
On 2010/08/14 22:27:11, Tom Henderson wrote:
> I see also a UanPhy::State enum that is similar-- should the existing one be
> extended to include SLEEP state?

Yes, we have talked about that with Leonard.

http://codereview.appspot.com/1743057/diff/7001/8015
File src/contrib/energy/model/remus-energy-model.cc (right):

http://codereview.appspot.com/1743057/diff/7001/8015#newcode178
src/contrib/energy/model/remus-energy-model.cc:178: double power = speed * 10;
On 2010/08/14 22:27:11, Tom Henderson wrote:
> this magic number 10 would make a good Attribute

Ok, I will insert a new Attribute.

http://codereview.appspot.com/1743057/diff/7001/8022
File src/devices/uan/uan-phy-gen.cc (right):

http://codereview.appspot.com/1743057/diff/7001/8022#newcode545
src/devices/uan/uan-phy-gen.cc:545: UpdatePowerConsumption (TX);
On 2010/08/14 22:27:11, Tom Henderson wrote:
> I don't see any handling of what happens when energy is exhausted on the node.

> Can you define what you expect to happen, then write an example or testcase
that
> shows a node draining all energy before simulation ends?

Ok, I will expect that a node will go to an "off" state, where it cannot sends
or receives packets.
I will write an example about this situation.

http://codereview.appspot.com/1743057/diff/7001/8043
File src/mobility/waypoint-mobility-model.h (right):

http://codereview.appspot.com/1743057/diff/7001/8043#newcode60
src/mobility/waypoint-mobility-model.h:60: * ConstantVelocityMobilityModel or a
child of it.
On 2010/08/14 22:27:11, Tom Henderson wrote:
> This is not well explained; why are you adding a second mobility model?

I'm adding a second mobility model to "validate" the waypoints. I mean, with an
ideal mobility model all the waypoints are valid. But with specifics mobility
model, with constraints on maximum speed, pitch andgles, etc. not all the
waypoints set by the user could be valid. The second mobility model is used to
check if the waypoints can be reached or not.

[socket, 2010-09-20 14:51:58]

http://codereview.appspot.com/1743057/diff/7001/src/contrib/energy/model/simp...
File src/contrib/energy/model/simple-device-energy-model.cc (right):

http://codereview.appspot.com/1743057/diff/7001/src/contrib/energy/model/simp...
src/contrib/energy/model/simple-device-energy-model.cc:71:
m_totalEnergyConsumption += energyToDecrease;
The energy model starts by default with a 0.0 current drain. If the user calls
SetCurrentA the constant current drain is updated with the new value.
The periodic update you were talking about, has been moved to the energy source.
The energy source model, periodically, updates the energy drained from the
source calling (Do)GetCurrent on every device associated with it.

On 2010/08/14 22:27:11, Tom Henderson wrote:
> I don't really see how this is to work.  If you set a current, aren't you
> setting a continual drain on the device?  Yet, this callback only gets fired
> when user calls SetCurrentA().  
> 
> I would have expected to see some kind of periodic updating such as scheduling
> this event for some time in the future so that there were continual periodic
> updates.

[socket, 2010-09-21 10:01:40]

In patch set 4, I've addressed all the Tom's comments. I've changed the
interaction between physical layer and energy layer to make it more general, not
depending on a specific acoustic transducer implementation. I've renamed the
MicroModemEnergyModel to a more general AcousticModemEnergyModel. I've
introduced the energy depletion handling, and a test case to show how it works.
I've added an Helper for the acoustic modem energy model.

[socket, 2010-11-16 11:23:21]

Patch Set 5 Highlights

- Moved all the project's files to src/contrib/uan-framework/
- Added manual draft documentation (UAN Framework chapter)
- Removed old includes
- Minor Doxygen changes