Bug 1873

Summary: Energy source checked to be aggregated to the node instead of energy source container
Product: ns-3 Reporter: Tomasz Seweryn <tomasz.seweryn7>
Component: energyAssignee: He Wu <mdzz>
Status: RESOLVED FIXED    
Severity: minor CC: ns-bugs, tomh, tommaso.pecorella
Priority: P5    
Version: ns-3-dev   
Hardware: All   
OS: All   
Attachments: EnergySourceContainer should be checked to be aggregated to the node
Deletion of additional energy source unneeded check
patch

Description Tomasz Seweryn 2014-03-06 15:22:23 EST
Inside the "BasicEnergySourceHelper::DoInstall" method, which is called in "EnergySourceHelper::Install", an if statement checks whether an energy source has been already installed on the node. This energy source is obtained by "GetObject" method called on the node. Although, in "EnergySourceHelper::Install" every node has EnergySourceContainer object aggregated to it, not EnergySource.

EnergySource should be replaced by EnergySourceContainer in "BasicEnergySourceHelper::DoInstall" to make the if statement work.
Comment 1 Tomasz Seweryn 2014-03-06 15:24:44 EST
Created attachment 1795 [details]
EnergySourceContainer should be checked to be aggregated to the node
Comment 2 Tom Henderson 2014-04-13 14:59:05 EDT
I am not sure about the patch; it seems to me that the original code is not correct and the patch should be elsewhere.

My read of the documentation is that there is intended to be a single EnergySource per node, and that the EnergySourceContainer exists to allow the helper to aggregate an EnergySource to each node in the Node container and return to the caller some kind of data structure containing all of these EnergySource objects (which exist on different nodes).

I think that instead the EnergySource should be aggregated to the node and that this is the method that should be patched to instead aggregate EnergySource:

EnergySourceContainer EnergySourceHelper::Install (NodeContainer c) const

Can Tomasz and Tony please review and confirm?  If so, can you please provide a new patch?
Comment 3 Tommaso Pecorella 2014-04-13 15:29:10 EDT
From my understanding, a node can have multiple EnergySources.

From the documentation:
"A node can have one or more energy sources, and each energy source can be connected to multiple device energy models."
Comment 4 Tom Henderson 2014-04-13 16:27:45 EDT
(In reply to Tommaso Pecorella from comment #3)
> From my understanding, a node can have multiple EnergySources.
> 
> From the documentation:
> "A node can have one or more energy sources, and each energy source can be
> connected to multiple device energy models."

Hmm, elsewhere it says:

/**
 * \ingroup energy
 * \brief Creates EnergySource objects.
 *
 * This class creates and installs an energy source onto network nodes. Only a
 * single source can exist on a network node.
 *
 */
class EnergySourceHelper


-----

I am not familiar with the modeling details, but it seems to me that having multiple sources would make it more complicated to associate sources with different device models?  Why are multiple sources needed?
Comment 5 Tomasz Seweryn 2014-04-13 16:30:31 EDT
Created attachment 1816 [details]
Deletion of additional energy source unneeded check

I think Tommaso is right. I also noticed this feature, that energy source management was designed in a way to allow multiple energy sources installation on nodes. This is why "EnergySourceContainer EnergySourceHelper::Install (NodeContainer c) const" method is correct. Actually, I think BasicEnergySourceHelper::DoInstall just does not need checking if the node has any other energy sources in the energy source container, does it? My patch refuses the node to have any other energy source to be installed, which is incorrect, but I guess this was the intention of the author of the original implementation. My final offer for the fix is in this attachment.
Comment 6 Tommaso Pecorella 2014-04-13 16:44:58 EDT
I don't know if the model is an overkill (by assuming that different energy sources can exist in a node and that each device can drain from a different one).

We could simplify the model, but I'm not really keen on removing features.

One possible explanation is that the authors did not foresee that a node could have more than one EnergySource of the same type. In this case the helper is wrong by not checking that the energy source is of the specific type.

Btw, the very same error is in RvBatteryModelHelper::DoInstall
Comment 7 Tomasz Seweryn 2014-04-17 07:32:25 EDT
It seems it would be good if the author of the module shared with his opinion. When installing energy sources to nodes, it is nowhere supported by any helper to aggregate the energy source object to the node object. This is why I sent my first patch, which deals with this problem. If the author says it is allowed to install more than one energy source to the node, than my second solution will solve the problem.
Comment 8 Tom Henderson 2014-04-26 08:57:03 EDT
(In reply to Tomasz Seweryn from comment #7)
> It seems it would be good if the author of the module shared with his
> opinion. When installing energy sources to nodes, it is nowhere supported by
> any helper to aggregate the energy source object to the node object. This is
> why I sent my first patch, which deals with this problem. If the author says
> it is allowed to install more than one energy source to the node, than my
> second solution will solve the problem.

I contacted the energy model maintainers (original authors), who said that they do not have time to maintain this module further.

So, I think it is up to us at the moment to decide which way to proceed (allow one or multiple energy sources per node, and align the implementation and documentation accordingly).

I might lean towards the fix with the least impact on the current code, API, and usage model, which seems oriented to allow multiple sources, correct?  With your initial patch Tomasz, do you think that this way of modeling will now be implemented correctly, or are there other issues?
Comment 9 Tommaso Pecorella 2014-04-26 11:16:39 EDT
Hi,

I'll check again later tonight. I'm still not convinced that Tomasz patches are right.
There's something in the EnergySourceContainer which I have to check.

About He Wu's answer, it's good to know. However we need (also) to find a new maintainer (and/or put the module in the bigger ns-bugs umbrella).
At least we'll not wait for an answer from the maintainers.

BTW, I totally understand if they don't have time anymore.
Comment 10 Tommaso Pecorella 2014-04-26 14:35:17 EDT
Created attachment 1829 [details]
patch

Similar to Tomasz's latest patch (i.e., remove altogether the checks.
Differences: done it in both Basic and Rv. Plus clarified the documentation.

Rationale:
1) There may be multiple energy sources in a node
2) Energy sources aren't aggregated to the node (EnergyContainer is)
3) An energy source may be created *before* the EnergyContainer

Credit Tomasz for the patch.
Comment 11 Tom Henderson 2014-04-26 16:56:19 EDT
Latest patch seems fine to me.
Comment 12 Tommaso Pecorella 2014-04-26 17:02:29 EDT
changeset:   10701:37823f33885f