Hi,

could you provide a test example showing the null pointer exception ?

Moreover, I think that the simplest (and most effective) patch is to simply add this:
if (m_source == NULL)
  {
     return 0;
  }

to DoGetCurrentA. The proposed patch can lead to slightly wrong results, as it reports a consumed energy even if the power source is disconnected.

T.

Hi,

If you take out m_source = NULL, the problem is solved.


void
AcousticModemEnergyModel::DoDispose (void)
{
  NS_LOG_FUNCTION (this);
  m_node = 0;
  m_source = 0;  //###This is causing the problem###
  m_energyDepletionCallback.Nullify ();
}


I' facing the issue when running the uan/auv/examples/uan-energy-auv

Hi,

I beg to differ. We'll have to review the auv module with great attention, as the problem is there.

DoDispose is called when the simulation is destroyed. There is no real reason to call DoGetCurrentA when the simulation is ended.

On the opposite, m_source *must* be nullified in DoDispose, or there will be a memory leak.

Cheers,

T.

(In reply to Hossam Khader from comment #2)
> Hi,
> 
> If you take out m_source = NULL, the problem is solved.
> 
> 
> void
> AcousticModemEnergyModel::DoDispose (void)
> {
>   NS_LOG_FUNCTION (this);
>   m_node = 0;
>   m_source = 0;  //###This is causing the problem###
>   m_energyDepletionCallback.Nullify ();
> }
> 
> 
> I' facing the issue when running the uan/auv/examples/uan-energy-auv

The ns3::LiIonEnergySource::DoDispose is causing a call to AcousticModemEnergyModel::DoGetCurrentA at the end of the simulation. Other models (e.g. wifi) are not facing the same issue because there is no reference to m_source in DoGetCurrentA.

What do you suggest changing DoGetCurrentA to mimic wifi or return 0 if m_source== NULL.


#0  0x00007ffff43dc5f7 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1  0x00007ffff43ddce8 in __GI_abort () at abort.c:90
#2  0x00007ffff4efc9d5 in __gnu_cxx::__verbose_terminate_handler () at ../../../../libstdc++-v3/libsupc++/vterminate.cc:95
#3  0x00007ffff4efa946 in __cxxabiv1::__terminate (handler=<optimized out>) at ../../../../libstdc++-v3/libsupc++/eh_terminate.cc:38
#4  0x00007ffff4efa973 in std::terminate () at ../../../../libstdc++-v3/libsupc++/eh_terminate.cc:48
#5  0x00007ffff6c2ccf1 in ns3::AcousticModemEnergyModel::DoGetCurrentA (this=0x641f50) at ../src/uan/model/acoustic-modem-energy-model.cc:275
#6  0x00007ffff6899938 in ns3::DeviceEnergyModel::GetCurrentA (this=0x641f50) at ../src/energy/model/device-energy-model.cc:54
#7  0x00007ffff687af64 in ns3::EnergySource::CalculateTotalCurrent (this=0x641d60) at ../src/energy/model/energy-source.cc:171
#8  0x00007ffff688b02c in ns3::LiIonEnergySource::CalculateRemainingEnergy (this=0x641d60) at ../src/energy/model/li-ion-energy-source.cc:280
#9  0x00007ffff688aa9b in ns3::LiIonEnergySource::DoDispose (this=0x641d60) at ../src/energy/model/li-ion-energy-source.cc:257
#10 0x00007ffff5b18655 in ns3::Object::Dispose (this=0x641d60) at ../src/core/model/object.cc:226

The problem is in LiIonEnergySource

void
LiIonEnergySource::DoDispose (void)
{
  NS_LOG_FUNCTION (this);
  // calculate remaining energy at the end of simulation
  CalculateRemainingEnergy ();
  BreakDeviceEnergyModelRefCycle ();  // break reference cycle
}

There is no need whatsoever to call CalculateRemainingEnergy when the object is being disposed. That's the bug.

Cheers,

T.


(In reply to Hossam Khader from comment #4)
> The ns3::LiIonEnergySource::DoDispose is causing a call to
> AcousticModemEnergyModel::DoGetCurrentA at the end of the simulation. Other
> models (e.g. wifi) are not facing the same issue because there is no
> reference to m_source in DoGetCurrentA.
> 
> What do you suggest changing DoGetCurrentA to mimic wifi or return 0 if
> m_source== NULL.
> 
> 
> #0  0x00007ffff43dc5f7 in __GI_raise (sig=sig@entry=6) at
> ../nptl/sysdeps/unix/sysv/linux/raise.c:56
> #1  0x00007ffff43ddce8 in __GI_abort () at abort.c:90
> #2  0x00007ffff4efc9d5 in __gnu_cxx::__verbose_terminate_handler () at
> ../../../../libstdc++-v3/libsupc++/vterminate.cc:95
> #3  0x00007ffff4efa946 in __cxxabiv1::__terminate (handler=<optimized out>)
> at ../../../../libstdc++-v3/libsupc++/eh_terminate.cc:38
> #4  0x00007ffff4efa973 in std::terminate () at
> ../../../../libstdc++-v3/libsupc++/eh_terminate.cc:48
> #5  0x00007ffff6c2ccf1 in ns3::AcousticModemEnergyModel::DoGetCurrentA
> (this=0x641f50) at ../src/uan/model/acoustic-modem-energy-model.cc:275
> #6  0x00007ffff6899938 in ns3::DeviceEnergyModel::GetCurrentA
> (this=0x641f50) at ../src/energy/model/device-energy-model.cc:54
> #7  0x00007ffff687af64 in ns3::EnergySource::CalculateTotalCurrent
> (this=0x641d60) at ../src/energy/model/energy-source.cc:171
> #8  0x00007ffff688b02c in ns3::LiIonEnergySource::CalculateRemainingEnergy
> (this=0x641d60) at ../src/energy/model/li-ion-energy-source.cc:280
> #9  0x00007ffff688aa9b in ns3::LiIonEnergySource::DoDispose (this=0x641d60)
> at ../src/energy/model/li-ion-energy-source.cc:257
> #10 0x00007ffff5b18655 in ns3::Object::Dispose (this=0x641d60) at
> ../src/core/model/object.cc:226

Created attachment 2324 [details]
patch

I don't completely agree with this. 
Calling the function CalculateRemainingEnergy() inside the DoDispose() determines the status of the battery at the end of the simulation. In some cases this can be useful, since we don't know a priori when the periodic updates of the energy source and the changes of status of the device energy models happen.

So I would say that the problem is inside the AcousticModemEnergyModel::DoGetCurrentA that calls m_source->GetSupplyVoltage() on a null m_source pointer, and I think that we
should make the change there. 
I don't know exactly how to best address this... The solution that probably creates more consistent results would be to make supplyVoltage a class variable and update it as usual.         

I attached a possible patch.

What do you think?

(In reply to Tommaso Pecorella from comment #5)
> The problem is in LiIonEnergySource
> 
> void
> LiIonEnergySource::DoDispose (void)
> {
>   NS_LOG_FUNCTION (this);
>   // calculate remaining energy at the end of simulation
>   CalculateRemainingEnergy ();
>   BreakDeviceEnergyModelRefCycle ();  // break reference cycle
> }
> 
> There is no need whatsoever to call CalculateRemainingEnergy when the object
> is being disposed. That's the bug.
> 
> Cheers,
> 
> T.
> 
> 
> (In reply to Hossam Khader from comment #4)
> > The ns3::LiIonEnergySource::DoDispose is causing a call to
> > AcousticModemEnergyModel::DoGetCurrentA at the end of the simulation. Other
> > models (e.g. wifi) are not facing the same issue because there is no
> > reference to m_source in DoGetCurrentA.
> > 
> > What do you suggest changing DoGetCurrentA to mimic wifi or return 0 if
> > m_source== NULL.
> > 
> > 
> > #0  0x00007ffff43dc5f7 in __GI_raise (sig=sig@entry=6) at
> > ../nptl/sysdeps/unix/sysv/linux/raise.c:56
> > #1  0x00007ffff43ddce8 in __GI_abort () at abort.c:90
> > #2  0x00007ffff4efc9d5 in __gnu_cxx::__verbose_terminate_handler () at
> > ../../../../libstdc++-v3/libsupc++/vterminate.cc:95
> > #3  0x00007ffff4efa946 in __cxxabiv1::__terminate (handler=<optimized out>)
> > at ../../../../libstdc++-v3/libsupc++/eh_terminate.cc:38
> > #4  0x00007ffff4efa973 in std::terminate () at
> > ../../../../libstdc++-v3/libsupc++/eh_terminate.cc:48
> > #5  0x00007ffff6c2ccf1 in ns3::AcousticModemEnergyModel::DoGetCurrentA
> > (this=0x641f50) at ../src/uan/model/acoustic-modem-energy-model.cc:275
> > #6  0x00007ffff6899938 in ns3::DeviceEnergyModel::GetCurrentA
> > (this=0x641f50) at ../src/energy/model/device-energy-model.cc:54
> > #7  0x00007ffff687af64 in ns3::EnergySource::CalculateTotalCurrent
> > (this=0x641d60) at ../src/energy/model/energy-source.cc:171
> > #8  0x00007ffff688b02c in ns3::LiIonEnergySource::CalculateRemainingEnergy
> > (this=0x641d60) at ../src/energy/model/li-ion-energy-source.cc:280
> > #9  0x00007ffff688aa9b in ns3::LiIonEnergySource::DoDispose (this=0x641d60)
> > at ../src/energy/model/li-ion-energy-source.cc:257
> > #10 0x00007ffff5b18655 in ns3::Object::Dispose (this=0x641d60) at
> > ../src/core/model/object.cc:226

Created attachment 2338 [details]
Alternative patch to bug 2310

Created class variable m_supplyVoltage to track the energy source supply voltage.

I disagree.

If the goal is to calculate the remaining energy, it should be done in a failsafe way, assuming that the sources are effectively disposed.

In the documentation, and in all the other battery models, CalculateRemainingEnergy is *not* called during the battery disposal.
Moreover calling other objects methods is to be considered unsafe during a DoDispose (and should be avoided).

If the goal is to update one last time the energy status before a DoDispose, we should find a completely different way to do it, and document the assumptions.

Just as an example of why CalculateRemainingEnergy is not to be called during a DoDispose.
Would you call CalculateRemainingEnergy after the simulation stop ? No, of course.
Then why should you call it during a disposal (which happens during a stop) ?

In my opinion if one wants a precise battery status should call *explicitly* GetRemainingEnergy just before the simulation stop. That's the safe method.

Cheers,

T.

(In reply to Cristiano from comment #7)
> I don't completely agree with this. 
> Calling the function CalculateRemainingEnergy() inside the DoDispose()
> determines the status of the battery at the end of the simulation. In some
> cases this can be useful, since we don't know a priori when the periodic
> updates of the energy source and the changes of status of the device energy
> models happen.
> 
> So I would say that the problem is inside the
> AcousticModemEnergyModel::DoGetCurrentA that calls
> m_source->GetSupplyVoltage() on a null m_source pointer, and I think that we
> should make the change there. 
> I don't know exactly how to best address this... The solution that probably
> creates more consistent results would be to make supplyVoltage a class
> variable and update it as usual.         
> 
> I attached a possible patch.
> 
> What do you think?
> 
> (In reply to Tommaso Pecorella from comment #5)
> > The problem is in LiIonEnergySource
> > 
> > void
> > LiIonEnergySource::DoDispose (void)
> > {
> >   NS_LOG_FUNCTION (this);
> >   // calculate remaining energy at the end of simulation
> >   CalculateRemainingEnergy ();
> >   BreakDeviceEnergyModelRefCycle ();  // break reference cycle
> > }
> > 
> > There is no need whatsoever to call CalculateRemainingEnergy when the object
> > is being disposed. That's the bug.
> > 
> > Cheers,
> > 
> > T.
> > 
> > 
> > (In reply to Hossam Khader from comment #4)
> > > The ns3::LiIonEnergySource::DoDispose is causing a call to
> > > AcousticModemEnergyModel::DoGetCurrentA at the end of the simulation. Other
> > > models (e.g. wifi) are not facing the same issue because there is no
> > > reference to m_source in DoGetCurrentA.
> > > 
> > > What do you suggest changing DoGetCurrentA to mimic wifi or return 0 if
> > > m_source== NULL.
> > > 
> > > 
> > > #0  0x00007ffff43dc5f7 in __GI_raise (sig=sig@entry=6) at
> > > ../nptl/sysdeps/unix/sysv/linux/raise.c:56
> > > #1  0x00007ffff43ddce8 in __GI_abort () at abort.c:90
> > > #2  0x00007ffff4efc9d5 in __gnu_cxx::__verbose_terminate_handler () at
> > > ../../../../libstdc++-v3/libsupc++/vterminate.cc:95
> > > #3  0x00007ffff4efa946 in __cxxabiv1::__terminate (handler=<optimized out>)
> > > at ../../../../libstdc++-v3/libsupc++/eh_terminate.cc:38
> > > #4  0x00007ffff4efa973 in std::terminate () at
> > > ../../../../libstdc++-v3/libsupc++/eh_terminate.cc:48
> > > #5  0x00007ffff6c2ccf1 in ns3::AcousticModemEnergyModel::DoGetCurrentA
> > > (this=0x641f50) at ../src/uan/model/acoustic-modem-energy-model.cc:275
> > > #6  0x00007ffff6899938 in ns3::DeviceEnergyModel::GetCurrentA
> > > (this=0x641f50) at ../src/energy/model/device-energy-model.cc:54
> > > #7  0x00007ffff687af64 in ns3::EnergySource::CalculateTotalCurrent
> > > (this=0x641d60) at ../src/energy/model/energy-source.cc:171
> > > #8  0x00007ffff688b02c in ns3::LiIonEnergySource::CalculateRemainingEnergy
> > > (this=0x641d60) at ../src/energy/model/li-ion-energy-source.cc:280
> > > #9  0x00007ffff688aa9b in ns3::LiIonEnergySource::DoDispose (this=0x641d60)
> > > at ../src/energy/model/li-ion-energy-source.cc:257
> > > #10 0x00007ffff5b18655 in ns3::Object::Dispose (this=0x641d60) at
> > > ../src/core/model/object.cc:226

I was looking at an easy fix and didn't think about this through.. I agree with your patch then!

In addition... I just noticed that the energyToDecrease calculation within AcousticModemEnergyModel::ChangeState is wrong and there is a bug for this here https://www.nsnam.org/bugzilla/﷒0﷓.

Basically, it calculates the energy as 
time * Power * Voltage = time * energy (??)
instead of the correct  
time * Power

The easiest solution to this is to remove supplyVoltage from the AcousticModemEnergyModel::ChangeState since it doesn't matter what the actual supply voltage of the battery is. This is because DoGetCurrentA() return the current by dividing the power by the voltage, then the energy source use the current to get the energy by multiplying it back by the voltage and the time.

(In reply to Tommaso Pecorella from comment #9)
> I disagree.
> 
> If the goal is to calculate the remaining energy, it should be done in a
> failsafe way, assuming that the sources are effectively disposed.
> 
> In the documentation, and in all the other battery models,
> CalculateRemainingEnergy is *not* called during the battery disposal.
> Moreover calling other objects methods is to be considered unsafe during a
> DoDispose (and should be avoided).
> 
> If the goal is to update one last time the energy status before a DoDispose,
> we should find a completely different way to do it, and document the
> assumptions.
> 
> Just as an example of why CalculateRemainingEnergy is not to be called
> during a DoDispose.
> Would you call CalculateRemainingEnergy after the simulation stop ? No, of
> course.
> Then why should you call it during a disposal (which happens during a stop) ?
> 
> In my opinion if one wants a precise battery status should call *explicitly*
> GetRemainingEnergy just before the simulation stop. That's the safe method.
> 
> Cheers,
> 
> T.
> 
> (In reply to Cristiano from comment #7)
> > I don't completely agree with this. 
> > Calling the function CalculateRemainingEnergy() inside the DoDispose()
> > determines the status of the battery at the end of the simulation. In some
> > cases this can be useful, since we don't know a priori when the periodic
> > updates of the energy source and the changes of status of the device energy
> > models happen.
> > 
> > So I would say that the problem is inside the
> > AcousticModemEnergyModel::DoGetCurrentA that calls
> > m_source->GetSupplyVoltage() on a null m_source pointer, and I think that we
> > should make the change there. 
> > I don't know exactly how to best address this... The solution that probably
> > creates more consistent results would be to make supplyVoltage a class
> > variable and update it as usual.         
> > 
> > I attached a possible patch.
> > 
> > What do you think?
> > 
> > (In reply to Tommaso Pecorella from comment #5)
> > > The problem is in LiIonEnergySource
> > > 
> > > void
> > > LiIonEnergySource::DoDispose (void)
> > > {
> > >   NS_LOG_FUNCTION (this);
> > >   // calculate remaining energy at the end of simulation
> > >   CalculateRemainingEnergy ();
> > >   BreakDeviceEnergyModelRefCycle ();  // break reference cycle
> > > }
> > > 
> > > There is no need whatsoever to call CalculateRemainingEnergy when the object
> > > is being disposed. That's the bug.
> > > 
> > > Cheers,
> > > 
> > > T.
> > > 
> > > 
> > > (In reply to Hossam Khader from comment #4)
> > > > The ns3::LiIonEnergySource::DoDispose is causing a call to
> > > > AcousticModemEnergyModel::DoGetCurrentA at the end of the simulation. Other
> > > > models (e.g. wifi) are not facing the same issue because there is no
> > > > reference to m_source in DoGetCurrentA.
> > > > 
> > > > What do you suggest changing DoGetCurrentA to mimic wifi or return 0 if
> > > > m_source== NULL.
> > > > 
> > > > 
> > > > #0  0x00007ffff43dc5f7 in __GI_raise (sig=sig@entry=6) at
> > > > ../nptl/sysdeps/unix/sysv/linux/raise.c:56
> > > > #1  0x00007ffff43ddce8 in __GI_abort () at abort.c:90
> > > > #2  0x00007ffff4efc9d5 in __gnu_cxx::__verbose_terminate_handler () at
> > > > ../../../../libstdc++-v3/libsupc++/vterminate.cc:95
> > > > #3  0x00007ffff4efa946 in __cxxabiv1::__terminate (handler=<optimized out>)
> > > > at ../../../../libstdc++-v3/libsupc++/eh_terminate.cc:38
> > > > #4  0x00007ffff4efa973 in std::terminate () at
> > > > ../../../../libstdc++-v3/libsupc++/eh_terminate.cc:48
> > > > #5  0x00007ffff6c2ccf1 in ns3::AcousticModemEnergyModel::DoGetCurrentA
> > > > (this=0x641f50) at ../src/uan/model/acoustic-modem-energy-model.cc:275
> > > > #6  0x00007ffff6899938 in ns3::DeviceEnergyModel::GetCurrentA
> > > > (this=0x641f50) at ../src/energy/model/device-energy-model.cc:54
> > > > #7  0x00007ffff687af64 in ns3::EnergySource::CalculateTotalCurrent
> > > > (this=0x641d60) at ../src/energy/model/energy-source.cc:171
> > > > #8  0x00007ffff688b02c in ns3::LiIonEnergySource::CalculateRemainingEnergy
> > > > (this=0x641d60) at ../src/energy/model/li-ion-energy-source.cc:280
> > > > #9  0x00007ffff688aa9b in ns3::LiIonEnergySource::DoDispose (this=0x641d60)
> > > > at ../src/energy/model/li-ion-energy-source.cc:257
> > > > #10 0x00007ffff5b18655 in ns3::Object::Dispose (this=0x641d60) at
> > > > ../src/core/model/object.cc:226

(In reply to Cristiano from comment #10)
 
> Basically, it calculates the energy as 
> time * Power * Voltage = time * energy (??)

I Meant

time * Power * Voltage = energy * VOLTAGE (??)