Bug 2386

The patch doesn't seem correct to me
IHMO:
1) m_state should be changed to SLEEP inside UanPhyGen::EnergyDepletionHandler () function 

void
UanPhyGen::EnergyDepletionHandler ()
{
  NS_LOG_FUNCTION (this);
  NS_LOG_DEBUG ("Energy depleted at node " << m_device->GetNode ()->GetId () <<
                ", stopping rx/tx activities");

  m_disabled = true;
  m_state = SLEEP; /// <======= proposed change
}

2) 

562	  if (m_state == SLEEP || m_disabled == true)
563	    {	563	    {
564	      NS_LOG_DEBUG ("Transmission ended but node sleeping or dead");	564	      NS_LOG_DEBUG ("Transmission ended but node sleeping or dead");
565	     // m_state = SLEEP; <=== removed see 1)
566	      UpdatePowerConsumption (SLEEP); <=== * not correct see below
565	      return;	
566	    }	

* Power consumption should not be updated if the node is dead (m_disabled == true) since its energy it's already depleted. urthermore the uan-phy-gen.cc doesn't implement the concept of SLEEP power consumption.
In order to fully implement the feature also other event should be covered.

bottom line:
I would only implement change 1) and leave 2) as a separate ticket with tag new feature.

(In reply to Federico Guerra from comment #1)
> The patch doesn't seem correct to me
> IHMO:
> 1) m_state should be changed to SLEEP inside
> UanPhyGen::EnergyDepletionHandler () function 
> 
> void
> UanPhyGen::EnergyDepletionHandler ()
> {
>   NS_LOG_FUNCTION (this);
>   NS_LOG_DEBUG ("Energy depleted at node " << m_device->GetNode ()->GetId ()
> <<
>                 ", stopping rx/tx activities");
> 
>   m_disabled = true;
>   m_state = SLEEP; /// <======= proposed change
> }

it seems to me that m_disabled is another state that is stored in a separate variable, and that the value of m_state does not matter when m_disabled is true

what about removing m_disabled variable and making it m_state = DISABLED?

> 
> 2) 
> 
> 562	  if (m_state == SLEEP || m_disabled == true)
> 563	    {	563	    {
> 564	      NS_LOG_DEBUG ("Transmission ended but node sleeping or dead");	564
> NS_LOG_DEBUG ("Transmission ended but node sleeping or dead");
> 565	     // m_state = SLEEP; <=== removed see 1)
> 566	      UpdatePowerConsumption (SLEEP); <=== * not correct see below
> 565	      return;	
> 566	    }	
> 
> * Power consumption should not be updated if the node is dead (m_disabled ==
> true) since its energy it's already depleted. urthermore the uan-phy-gen.cc
> doesn't implement the concept of SLEEP power consumption.
> In order to fully implement the feature also other event should be covered.

I agree; I wonder if TxEndEvent() should even be called in either of these cases, or whether the event should be cancelled if the node transitions to sleep or disabled while a transmission is ongoing.

> 
> bottom line:
> I would only implement change 1) and leave 2) as a separate ticket with tag
> new feature.

I am not even sure that change 1) does anything, however.

m_state = SLEEP alone didn't fix the problem.
You still need to call UpdatePowerConsumption (SLEEP).

(In reply to Tom Henderson from comment #2)
> (In reply to Federico Guerra from comment #1)
> > The patch doesn't seem correct to me
> > IHMO:
> > 1) m_state should be changed to SLEEP inside
> > UanPhyGen::EnergyDepletionHandler () function 
> > 
> > void
> > UanPhyGen::EnergyDepletionHandler ()
> > {
> >   NS_LOG_FUNCTION (this);
> >   NS_LOG_DEBUG ("Energy depleted at node " << m_device->GetNode ()->GetId ()
> > <<
> >                 ", stopping rx/tx activities");
> > 
> >   m_disabled = true;
> >   m_state = SLEEP; /// <======= proposed change
> > }
> 
> it seems to me that m_disabled is another state that is stored in a separate
> variable, and that the value of m_state does not matter when m_disabled is
> true
> 
> what about removing m_disabled variable and making it m_state = DISABLED?
> 
> > 
> > 2) 
> > 
> > 562	  if (m_state == SLEEP || m_disabled == true)
> > 563	    {	563	    {
> > 564	      NS_LOG_DEBUG ("Transmission ended but node sleeping or dead");	564
> > NS_LOG_DEBUG ("Transmission ended but node sleeping or dead");
> > 565	     // m_state = SLEEP; <=== removed see 1)
> > 566	      UpdatePowerConsumption (SLEEP); <=== * not correct see below
> > 565	      return;	
> > 566	    }	
> > 
> > * Power consumption should not be updated if the node is dead (m_disabled ==
> > true) since its energy it's already depleted. urthermore the uan-phy-gen.cc
> > doesn't implement the concept of SLEEP power consumption.
> > In order to fully implement the feature also other event should be covered.
> 
> I agree; I wonder if TxEndEvent() should even be called in either of these
> cases, or whether the event should be cancelled if the node transitions to
> sleep or disabled while a transmission is ongoing.
> 
> > 
> > bottom line:
> > I would only implement change 1) and leave 2) as a separate ticket with tag
> > new feature.
> 
> I am not even sure that change 1) does anything, however.

Hi Tom,
I agree that m_disabled should be merged into m_state with DISABLED.
that would simplify the design. and the state would be changed from TX to DISABLED at the energy depleted event.

regarding the TxEndEvent() and also RxEndEvent()
1) simplest solution would be to keep the events, drop the packet but also notify. Currently the NotifyTxDrop is missing. 
2) removing both events in case of energy depletion would be the cleanest solution, provided that you notify the TXdrop or RX drop event.
3) furthermore, why MAC or higher layers in general are not informed that the energy is depleted? why let them continue to generate and send packets?

(In reply to Hossam Khader from comment #3)
> m_state = SLEEP alone didn't fix the problem.
> You still need to call UpdatePowerConsumption (SLEEP).

Hi Hossam,
please explain why you would need to call UpdatePowerConsumption (SLEEP).
thanks

If UpdatePowerConsumption (SLEEP) is not called, the TX state power will continue to be consumed from the energy source, and the energy source can't be recharged.

Something also to highlight is that energy will be consumed from the source below the source LowBatteryThreshold, which can be fixed by having a DISABLED state with zero power consumption.

(In reply to Hossam Khader from comment #6)
> If UpdatePowerConsumption (SLEEP) is not called, the TX state power will
> continue to be consumed from the energy source, and the energy source can't
> be recharged.
> 
> Something also to highlight is that energy will be consumed from the source
> below the source LowBatteryThreshold, which can be fixed by having a
> DISABLED state with zero power consumption.

Hi Hossam, 
I think I understood.

------------------------
A - SIMPLEST SOLUTION

UpdatePowerConsumption it also then changes the AcoustModemEnergyModel to SLEEP via SetMicroModemState.

However I do not agree with this implementation.
I would handle the modem state change in HandleEnergyDepletion

AcousticModemEnergyModel::HandleEnergyDepletion (void)
{
  NS_LOG_FUNCTION (this);
  NS_LOG_DEBUG ("AcousticModemEnergyModel:Energy is depleted at node #" <<
                m_node->GetId ());
  // invoke energy depletion callback, if set.
  if (!m_energyDepletionCallback.IsNull ())
    {
      m_energyDepletionCallback ();
    }
  // invoke the phy energy depletion handler
  Ptr<UanNetDevice> dev = m_node->GetDevice (0)->GetObject<UanNetDevice> ();
  dev->GetPhy ()->EnergyDepletionHandler ();
  SetMicroModemState(UanPhy::SLEEP); /// proposed change
}

and in uan-phy-gen.cc

void
UanPhyGen::EnergyDepletionHandler ()
{
  NS_LOG_FUNCTION (this);
  NS_LOG_DEBUG ("Energy depleted at node " << m_device->GetNode ()->GetId () <<
                ", stopping rx/tx activities");

  m_disabled = true;
  m_state = SLEEP; /// <======= proposed change
}
---------------------------------------

B - OPTIMAL SOLUTION

1) introduce a new state UanPhy::DISABLED
2) merge m_state and m_disabled

void
UanPhyGen::EnergyDepletionHandler ()
{
  NS_LOG_FUNCTION (this);
  NS_LOG_DEBUG ("Energy depleted at node " << m_device->GetNode ()->GetId () <<
                ", stopping rx/tx activities");

  // m_disabled = true; /// removed
  m_state = DISABLED; /// <======= proposed change
}

3) remove m_disabled from UanPhyGen code

4) if TxEndEvent is scheduled, then remove it from the scheduler and notify NotifyTxDrop()
5) if RxEndEvent is scheduled, then remove it from the scheduler, and NotifyRxDrop()
6) handle the new state UanPhy::DISABLED in the AcousticModemEnergyModel class
---------------------------

I would go with solution A, and then open a ticket in order to implement B as a future improvement.

please be aware that in both cases it seems that currently there is no event that changes m_disabled from false to true (i.e. once the energy is depleted the phy will never get back online even if the energy is restored).

small correction:

please be aware that in both cases it seems that currently there is no event that changes m_disabled from TRUE to FALSE (i.e. once the energy is depleted the phy will never get back online even if the energy is restored).

Thanks for bringing up this!
I will definitely need to implement EnergyRechargeHandler.
I will merge m_disabled with m_state as a part of the implementation.

Do we need to open a ticket for EnergyRechargeHandler?

> Do we need to open a ticket for EnergyRechargeHandler?

I'd say yes, but let's wait for Tom answer.

Please also explain what your proposed solution would be.

Would you go with my proposed solution B?
which I report here, after a small revision :)

or do you have a better idea?

thanks

-----
B - OPTIMAL SOLUTION

1) introduce a new state UanPhy::DISABLED
2) handle the new state UanPhy::DISABLED in the AcousticModemEnergyModel class

   furthermore, do the state change inside the energy depletion handle function. state change for other state will be still performed inside update
AcousticModemEnergyModel::ChangeState

AcousticModemEnergyModel::HandleEnergyDepletion (void)
{
  NS_LOG_FUNCTION (this);
  NS_LOG_DEBUG ("AcousticModemEnergyModel:Energy is depleted at node #" <<
                m_node->GetId ());
  // invoke energy depletion callback, if set.
  if (!m_energyDepletionCallback.IsNull ())
    {
      m_energyDepletionCallback ();
    }
  // invoke the phy energy depletion handler
  Ptr<UanNetDevice> dev = m_node->GetDevice (0)->GetObject<UanNetDevice> ();
  dev->GetPhy ()->EnergyDepletionHandler ();
  SetMicroModemState(UanPhy::DISABLED); /// <==== handle state here
}

3) merge m_state and m_disabled

void
UanPhyGen::EnergyDepletionHandler ()
{
  NS_LOG_FUNCTION (this);
  NS_LOG_DEBUG ("Energy depleted at node " << m_device->GetNode ()->GetId () <<
                ", stopping rx/tx activities");

  // m_disabled = true; /// <===== removed
  m_state = DISABLED; /// <======= proposed change
}

4) remove m_disabled from UanPhyGen code

5) if TxEndEvent is scheduled, then remove it from the scheduler and notify NotifyTxDrop() 
There is no need to call UpdatePowerConsumption (DISABLED) to perform state change, in DISABLED mode there is no power to consume!!

6) if RxEndEvent is scheduled, then remove it from the scheduler, and NotifyRxDrop()
There is no need to call UpdatePowerConsumption (DISABLED) to perform state change, in DISABLED mode there is no power to consume!!



---------------------------

Created attachment 2408 [details]
proposed patch

Please find the attached patch. If any changes are required, please let me know to redo the testing.

(In reply to Hossam Khader from comment #11)
> Created attachment 2408 [details]
> proposed patch
> 
> Please find the attached patch. If any changes are required, please let me
> know to redo the testing.

Hi Hossam,
well done.
Thanks for the patch, it seems now that we have everything in place now.

There is just one small error to fix

please check uan-phy-gen.cc

518	  if(m_txEndEvent.IsRunning ())
519	    {
520	      Simulator::Cancel (m_txEndEvent);
521	      NotifyRxDrop (m_pktTx); <====== NotifyTxDrop (m_pktTx); 
522	      m_pktTx = 0;
523	    }

(In reply to Hossam Khader from comment #11)
> Created attachment 2408 [details]
> proposed patch
> 
> Please find the attached patch. If any changes are required, please let me
> know to redo the testing.

Hi Hossam did you receive my reply?

Federico

Created attachment 2426 [details]
uan-final-patch

Hi Federico,

The testing went fine. Attached is the final patch.

Hi Hossam,
the latest patch is OK.
Thank you again!

Federico

fix pushed in changeset 12119:d9c715fffea3

This patch adds a new PHY state 'DISABLED' and moves the Phy to this
state when energy is depleted, cancelling any pending transmissions or
receptions, and preventing further energy consumption below the low battery
threshold from the energy source.  It also introduces new methods to handle
the event when energy is recharged (to move the state out of DISABLED) by
adding an EnergyRechargeHandler callback.  Federico Guerra also contributed
to the development of this patch.