Code review - Issue 1743057: UAN Framework project (GSoC) final reviewhttps://codereview.appspot.com/2010-11-16T11:23:21+00:00rietveld
Message from unknown
2010-08-05T10:15:43+00:00socketurn:md5:2aa73d750f6b1163596468d5db2ffc4b
Message from unknown
2010-08-07T15:25:16+00:00socketurn:md5:83f5a530f5913b4b54801ce0f7becd72
Message from lentracy@gmail.com
2010-08-09T05:49:47+00:00lentracyurn:md5:396cc9ab9fc7db33f72fa24a9b83214c
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.
Message from unknown
2010-08-14T12:54:55+00:00socketurn:md5:af4cdbf0752d73e13db68f13ba4ff338
Message from tomh.org@gmail.com
2010-08-14T22:27:11+00:00Tom Hendersonurn:md5:644ab69aa1b884191ed23709db472246
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?
Message from lentracy@u.washington.edu
2010-08-17T03:13:25+00:00lentracy_u.washington.eduurn:md5:65d9f5a1e18f395a961d0013071ae221
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
>
Message from andrea.sacco85@gmail.com
2010-08-17T17:02:11+00:00socketurn:md5:4621e2a5820c8d4c165dbd24f1d21112
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.
Message from andrea.sacco85@gmail.com
2010-09-20T14:51:58+00:00socketurn:md5:7f3328a4e31e0ebda5f444de96e86c89
http://codereview.appspot.com/1743057/diff/7001/src/contrib/energy/model/simple-device-energy-model.cc
File src/contrib/energy/model/simple-device-energy-model.cc (right):
http://codereview.appspot.com/1743057/diff/7001/src/contrib/energy/model/simple-device-energy-model.cc#newcode71
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.
Message from unknown
2010-09-21T07:49:51+00:00socketurn:md5:1b1368a81d091e9f2c76d8b328d6e323
Message from andrea.sacco85@gmail.com
2010-09-21T07:50:08+00:00socketurn:md5:a5f81fde480d74965bbc7c7f6aac0703
Message from andrea.sacco85@gmail.com
2010-09-21T10:01:40+00:00socketurn:md5:fa2b495ef08e89fcc3e2073982a1e7b3
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.
Message from unknown
2010-11-16T11:15:50+00:00socketurn:md5:e81765cf675164bdcdde5f4c382d9875
Message from andrea.sacco85@gmail.com
2010-11-16T11:16:16+00:00socketurn:md5:33c2c34c19112aa927110998c1ed9cfa
Message from andrea.sacco85@gmail.com
2010-11-16T11:23:21+00:00socketurn:md5:fa8e35bba26849fcdfcec08fab06480f
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