Created attachment 2465 [details]
AUV patch as developed by Andrea Sacco

original AUV patch developed by Andrea Sacco

I think that we should have a deep (pun intended) look at the design of the AUV module.

First and foremost, I don't really like the nested structure.
The second point is the overall rationale of the code - and its possible applications.

The rationale of the code is simple (at first glance): AUV don't have wheels and they don't move in a straight line. Moreover, AUV motors are energy hungry, so it does make sense to model them as well.

The first point (not a straight line) is *the* problem. If I ask an AUV to go from point A to point B, it might not be able to use a straight line (glider), when they change direction they have constraints (remus) and it could even be that they can not stand still on a point.

Now, this isn't a movement model, this is a *cinematic* model.
In my opinion we need a whole new model: cinematic. Of course the very first cinematic object to be modeled will be an AUV, but it could be expanded to model aerial vehicles, or boats.

Moreover, since we're at it, I'd add a GetEstimatedNodePosition to the mobility model, in order to reflect that the node could have a non-perfect knowledge of its true position.

There are multiple issues with this patch.
Namely:
1) The energy consumption is calculated with the wrong values,
2) GetTotalEnergyConsumption returns an old value (and not the actual value)
3) AuvMobilityModel derives from ConstantVelocityMobilityModel instead of MobilityModel
4) The use of ConstantVelocityHelper is questionable (at best)
5) AuvWaypointMobilityModel is "too simple", as it doesn't take into account the actual mobility model limits. 

I have already a patch for 1, 2 and 3.
Point 4 is a "simple" refactoring.
Point 5 is a bit more complex.

I'll post an updated patch once I have fixed also point 4.

Created attachment 2531 [details]
Revised models

I figured that fixing 1, 2 and 3 is a priority, so here is the patch.

I also removed the GridPositionAllocator from the patch, as it's not specific to the AUV case.
Even if the AUV models placement is subject to further discussions, THAT position allocator definitely belongs to the mobility model.
I'm going to add a new enhancement for that.

Hi Tommaso,
sorry for the late reply but I was on summer vacation.
I'll check the patch ASAP.

Thanks for your precious work, as always :)

regarding 5) do you have something specific in mind?

Federico

(In reply to Federico Guerra from comment #5)
> Hi Tommaso,
> sorry for the late reply but I was on summer vacation.
> I'll check the patch ASAP.

Take your time :)

> Thanks for your precious work, as always :)

YW.
 
> regarding 5) do you have something specific in mind?

The problem is that if you ask the model to go from point A to point B and the model doesn't allow to reach them directly, it fails (e.g., A and B just with different depths).

Also, if you set point A and B at the same depth and you use Glider, even if the model will go to point B, it will reach a point with the wrong depth.

Without adding too much complexity, the waypoint should add intermediate points where needed. Further optimizations would require a control box, that would be nice, but probably out of scope.

Hi Tommaso,
I've finally found time to check the new patch.

my only concern is on 2)

+double
+RemusEnergyModel::GetTotalEnergyConsumption (void) const
+{
+  NS_LOG_FUNCTION (this);
+
+  Time duration = Simulator::Now () - m_lastUpdateTime;
+  double power = GetPower (m_currentSpeed);
+  double energyToDecrease = duration.GetSeconds () * power;
+
+  return m_totalEnergyConsumption + energyToDecrease;
+}

In the previous function you are actually implying that the model would not change between Now () and m_lastUpdateTime.
I still have to deepen my study of the model, so I don't know if its really possible that the model could be in a different state without calling Update () 
before a call to GetTotalEnergyConsumption ()


Anyway, why not letting the module compute the energy by itself with

+double
+RemusEnergyModel::GetTotalEnergyConsumption (void) const
+{
+  NS_LOG_FUNCTION (this);
+
+  UpdatePowerConsumption(m_currentSpeed);
+
+  return m_totalEnergyConsumption;
+}

Or even better, in light of future modification to the glider model where fluctuations might be possible based on the waypoints, I would also trigger an Update () before returning the total energy.


+double
+RemusEnergyModel::GetTotalEnergyConsumption (void) const
+{
+  NS_LOG_FUNCTION (this);
+
+  Update ();
+
+  UpdatePowerConsumption(m_currentSpeed);
+
+  return m_totalEnergyConsumption;
+}

Be advised that I did not have the chance to check if this would work with current code, anyway I just wanted to discuss this strategy with you

Federico

Hi Federico,

the problem is: GetTotalEnergyConsumption is const. As a consequence, you can not change any internal state variable (unless we explicitly declare as mutable).

Anyway, the problem isn't a problem in my eyes.
GetTotalEnergyConsumption is used (mainly) to get a periodic update of the energy consumption.
If there is a course change, then m_totalEnergyConsumption and m_lastUpdateTime will be updated accordingly. The only thing left to do is to calculate the energy between the last update and now, and return the correct value.
This is more than enough to return the expected value between two course changes.

Cheers,
T.


(In reply to Federico Guerra from comment #7)
> Hi Tommaso,
> I've finally found time to check the new patch.
> 
> my only concern is on 2)
> 
> +double
> +RemusEnergyModel::GetTotalEnergyConsumption (void) const
> +{
> +  NS_LOG_FUNCTION (this);
> +
> +  Time duration = Simulator::Now () - m_lastUpdateTime;
> +  double power = GetPower (m_currentSpeed);
> +  double energyToDecrease = duration.GetSeconds () * power;
> +
> +  return m_totalEnergyConsumption + energyToDecrease;
> +}
> 
> In the previous function you are actually implying that the model would not
> change between Now () and m_lastUpdateTime.
> I still have to deepen my study of the model, so I don't know if its really
> possible that the model could be in a different state without calling Update
> () 
> before a call to GetTotalEnergyConsumption ()
> 
> 
> Anyway, why not letting the module compute the energy by itself with
> 
> +double
> +RemusEnergyModel::GetTotalEnergyConsumption (void) const
> +{
> +  NS_LOG_FUNCTION (this);
> +
> +  UpdatePowerConsumption(m_currentSpeed);
> +
> +  return m_totalEnergyConsumption;
> +}
> 
> Or even better, in light of future modification to the glider model where
> fluctuations might be possible based on the waypoints, I would also trigger
> an Update () before returning the total energy.
> 
> 
> +double
> +RemusEnergyModel::GetTotalEnergyConsumption (void) const
> +{
> +  NS_LOG_FUNCTION (this);
> +
> +  Update ();
> +
> +  UpdatePowerConsumption(m_currentSpeed);
> +
> +  return m_totalEnergyConsumption;
> +}
> 
> Be advised that I did not have the chance to check if this would work with
> current code, anyway I just wanted to discuss this strategy with you
> 
> Federico

(In reply to Tommaso Pecorella from comment #8)
> Hi Federico,
> 
> the problem is: GetTotalEnergyConsumption is const. As a consequence, you
> can not change any internal state variable (unless we explicitly declare as
> mutable).
> 
> Anyway, the problem isn't a problem in my eyes.
> GetTotalEnergyConsumption is used (mainly) to get a periodic update of the
> energy consumption.
> If there is a course change, then m_totalEnergyConsumption and
> m_lastUpdateTime will be updated accordingly. The only thing left to do is
> to calculate the energy between the last update and now, and return the
> correct value.
> This is more than enough to return the expected value between two course
> changes.
> 
> Cheers,
> T.
> 
> 
> (In reply to Federico Guerra from comment #7)
> > Hi Tommaso,
> > I've finally found time to check the new patch.
> > 
> > my only concern is on 2)
> > 
> > +double
> > +RemusEnergyModel::GetTotalEnergyConsumption (void) const
> > +{
> > +  NS_LOG_FUNCTION (this);
> > +
> > +  Time duration = Simulator::Now () - m_lastUpdateTime;
> > +  double power = GetPower (m_currentSpeed);
> > +  double energyToDecrease = duration.GetSeconds () * power;
> > +
> > +  return m_totalEnergyConsumption + energyToDecrease;
> > +}
> > 
> > In the previous function you are actually implying that the model would not
> > change between Now () and m_lastUpdateTime.
> > I still have to deepen my study of the model, so I don't know if its really
> > possible that the model could be in a different state without calling Update
> > () 
> > before a call to GetTotalEnergyConsumption ()
> > 
> > 
> > Anyway, why not letting the module compute the energy by itself with
> > 
> > +double
> > +RemusEnergyModel::GetTotalEnergyConsumption (void) const
> > +{
> > +  NS_LOG_FUNCTION (this);
> > +
> > +  UpdatePowerConsumption(m_currentSpeed);
> > +
> > +  return m_totalEnergyConsumption;
> > +}
> > 
> > Or even better, in light of future modification to the glider model where
> > fluctuations might be possible based on the waypoints, I would also trigger
> > an Update () before returning the total energy.
> > 
> > 
> > +double
> > +RemusEnergyModel::GetTotalEnergyConsumption (void) const
> > +{
> > +  NS_LOG_FUNCTION (this);
> > +
> > +  Update ();
> > +
> > +  UpdatePowerConsumption(m_currentSpeed);
> > +
> > +  return m_totalEnergyConsumption;
> > +}
> > 
> > Be advised that I did not have the chance to check if this would work with
> > current code, anyway I just wanted to discuss this strategy with you
> > 
> > Federico

OK, 
I did miss the const. 
The only thing I don't really like is that you are computing the updated power without using the dedicated private function. 
This means that you'd need to change the code in two places instead of one. 
Nevertheless this is just a finesse thing, the patch is already good the way it is!

(In reply to Federico Guerra from comment #9)
> OK, 
> I did miss the const. 
> The only thing I don't really like is that you are computing the updated
> power without using the dedicated private function. 
> This means that you'd need to change the code in two places instead of one. 
> Nevertheless this is just a finesse thing, the patch is already good the way
> it is!

I'm not particularly excited either, but the other function is non-const, so we can't really use it.

An alternative is to have another (const) function named "GetEnergyToDecreaseSinceLastUpdate" and use it in the 2 places where this calculation is made.
However, since the function would be essentially 3 lines, I felt like an overkill to do it.

Cheers,
T.

> 
> An alternative is to have another (const) function named
> "GetEnergyToDecreaseSinceLastUpdate" and use it in the 2 places where this
> calculation is made.
> However, since the function would be essentially 3 lines, I felt like an
> overkill to do it.
> 
> Cheers,
> T.

Agreed, that is overkill indeed :)

If it were me, I'd drop the const, to underline that getting the total power is actually forcing a state update at current time, and then use the update power functions, and then returning the m_totalpower.

but I understand that it might sound overkill too :)

Federico

*** Bug 2435 has been marked as a duplicate of this bug. ***

Hi Tommaso, Tom
do we want to drop this ticket, or do you want me to move this issue to Gitlab?
is the latest patch code worth of a commit?

Please move to GitLab, I'll take a look at it asap.

Thanks.

moved to https://gitlab.com/nsnam/ns-3-dev/-/issues/337