Unneeded m_requiredTxCallback() call?

[JacoTuks]

Expected Behavior

No call to m_requiredTxCallback().

Actual Behavior

Line 257 calls m_requiredTxCallback() for unconfirmed traffic. It gives the function m_retxParams.packet but that packet is 0 as m_retxParams is currently only used for confirmed traffic.

Lora packet tracker thus calls m_reTransmissionTracker.insert(0, entry) which succeeds once and after that is ignored as packet 0 already exists.

This causes CountMacPacketsGloballyCpsr() to find a packet in a 100% unconfirmed traffic simulation (there should be none).

Is there a reason for this call that I'm missing?

Steps to Reproduce the Problem

1. Change RequiredTransmissionsCallback() to be

std::pair<std::map<Ptr<const Packet>, RetransmissionStatus>::iterator,bool> ret;
ret = m_reTransmissionTracker.insert (std::pair<Ptr<Packet>, RetransmissionStatus>
                                    (packet, entry));

if(ret.second==false)
    NS_LOG_DEBUG ("Packet already exists in reTransmissionTracker");

2. Change complete-network-example to show confirmed performance as well

   LogComponentEnable("LoraPacketTracker", LOG_LEVEL_INFO);


   std::cout << tracker.CountMacPacketsGlobally (Seconds (0), appStopTime + Hours (1)) << std::endl;
   std::cout << tracker.CountMacPacketsGloballyCpsr (Seconds (0), appStopTime + Hours (1)) << std::endl;

3. Note how confirmed performance shows 1 1 + the printing of Packet already exists.

Specifications

• ns-3 version: 3.30
• lorawan module version: latest
• Platform:
• Compiler:

[DvdMgr]

It looks like the problematic line is model/class-a-end-device-lorawan-mac.cc:435, not model/class-a-end-device-lorawan-mac.cc:257. As a solution, I would completely remove this block of code:

  else
    {
      uint8_t txs = m_maxNumbTx - (m_retxParams.retxLeft );
      m_requiredTxCallback (txs, true, m_retxParams.firstAttempt, m_retxParams.packet);
      NS_LOG_INFO ("We have " << unsigned(m_retxParams.retxLeft) <<
                   " transmissions left. We were not transmitting confirmed messages.");

      // Reset retransmission parameters
      resetRetransmissionParameters ();
    }

Do you agree this would fix the issue without introducing any regressions? It certainly makes no sense to reset retransmission parameters for unconfirmed traffic, and at first sight for confirmed traffic this looks useless, too, but I'm not 100% sure. We need more tests for this kind of stuff.

[JacoTuks]

I agree with your thinking. I'm not going to send a pull request at this stage as I'm tackling issue #110 and that would overwrite this code anyway.

Still figuring out the best time to reset myself!

[DvdMgr]

That's great news - let me know if you need any help. Let's keep this open for now, and remember to close it if it gets fixed with the new code!

[DvdMgr]

Closing, let's move this discussion to the pull request.