Bugzilla – Full Text Bug Listing |
Summary: | Energy source checked to be aggregated to the node instead of energy source container | ||
---|---|---|---|
Product: | ns-3 | Reporter: | Tomasz Seweryn <tomasz.seweryn7> |
Component: | energy | Assignee: | 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
Created attachment 1795 [details]
EnergySourceContainer should be checked to be aggregated to the node
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? 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." (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? 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.
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 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. (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? 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. 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.
Latest patch seems fine to me. changeset: 10701:37823f33885f |