Bugzilla – Bug 2386
UAN-PHY-GEN stays in TX mode after EnergyDepletionHandler() is called
Last modified: 2016-05-14 21:22:49 UTC
Created attachment 2398 [details] suggested patch Attached suggested patch fixed the problem
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.