Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improving NbTrans support for class A #114

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

JacoTuks
Copy link

This request improves NbTrans support by causing both unconfirmed and confirmed traffic to be transmitting NbTrans times.

This code was written for class A devices and whilst NbTrans is supported fully the LinkADRReq command does not update NbTrans. I assumed that most people aren't using ADR and those that do would have noticed that this command never supported NbTrans updates and would have implemented it themselves if required.

Furthermore, the code doesn't test if you provided a NbTrans value between 1 and 15. It will simply attempt to transmit the packet's the provided number of times.

This pull request fixes issues #110 and #112

Proposed Changes

  • LoraRetxParameters struct now has an additional boolean value sendingMultipleUnconfirmed.
  • PhyPacketData is now a multimap to allow for the storage of duplicate packets. Each packet's PHY result is now saved and thus PrintPhyPacketsPerGw() will now indicate what happened to all of the transmitted packets.
  • The default number of transmissions have been changed to the protocol's stated 1 instead of the current 8.

Unconfirmed traffic are now also retransmitted. Note that LinkADRReq does not update NbTrans.
Copy link
Member

@DvdMgr DvdMgr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the effort! I looked at the code and tried it out, it seems to work fine! I left a couple comments throughout the code in this review. Other general comments:

  • Maybe it's best to split the pull request in two: work on the packet tracker and work on re-transmissions.
  • We might be able to simplify some of the code in ClassAEndDeviceLorawanMac and EndDeviceLorawanMac, but let's leave this for another time.

Unfortunately, as of now it's kind of difficult to test the correctness of the code - I will take this contribution as a sign that it's finally time for me to write some in-depth tests. I will come back to this pull request once those are done! In the meanwhile, again, thanks :)

helper/lora-packet-tracker.cc Outdated Show resolved Hide resolved
@@ -87,7 +87,7 @@ EndDeviceLorawanMac::GetTypeId (void)
"ns3::TracedValueCallback::Double")
.AddAttribute ("MaxTransmissions",
"Maximum number of transmissions for a packet",
IntegerValue (8),
IntegerValue (1),
MakeIntegerAccessor (&EndDeviceLorawanMac::m_maxNumbTx),
MakeIntegerChecker<uint8_t> ())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also use MakeIntegerChecker's arguments to specify minimum and maximum allowed values:

Suggested change
MakeIntegerChecker<uint8_t> ())
MakeIntegerChecker<uint8_t> (1, 15))

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made this change, but no error is given when I run complete-network-example with numberOfTransmissions=17? I tried googling this checker but didn't get examples of how this should work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is expected, as the integer value is checked only if the value is set through the attribute system, which is bypassed if you use the method directly!

helper/lora-packet-tracker.h Show resolved Hide resolved
JacoTuks and others added 3 commits July 27, 2021 13:31
Created SetPacketOutcome()
Avoids issue with int vs uint8_t passing when using sem
@JacoTuks
Copy link
Author

I have made some of the smaller changed. I haven't split it into separate commits yet. I will wait for that once you have written more tests.

The last commit was to change numberOfTransmissions to an int complete-network-example. I noticed that when you pass a value using sem, the passed value will be different if this variable was a uint8_t.

@DvdMgr
Copy link
Member

DvdMgr commented Jul 28, 2021

The last commit was to change numberOfTransmissions to an int complete-network-example. I noticed that when you pass a value using sem, the passed value will be different if this variable was a uint8_t.

Can you expand on this? Is sem/the ns-3 script distorting the value?

@JacoTuks
Copy link
Author

JacoTuks commented Aug 4, 2021

The last commit was to change numberOfTransmissions to an int complete-network-example. I noticed that when you pass a value using sem, the passed value will be different if this variable was a uint8_t.

Can you expand on this? Is sem/the ns-3 script distorting the value?

This is how I understand it.

In my sem script, numberOfTransmissions is a Python int. If the ns-3 script expects an uint8_t and not an int it will receive a different value as Python doesn't have the concept of a uint8_t.

For example, if the sem script passes 3 it gives you 51 when you print with std::cout<< (unsigned) numberOfTransmissions :: std::endl;

An easy way to see the impact of this is when running a confirmed sim.

Adding the following to RequiredTransmissionsCallback() will allow the sim output to show
if(reqTx > 3)
std::cout<< "More than 3: " << (unsigned) reqTx << std::endl;

More than 3: 6
More than 3: 5
More than 3: 4
More than 3: 6
More than 3: 7
More than 3: 4
...
200.000000 200.000000

Changing to an int from uint8_t allows the value to be safely passed and then the conversion from int to uin8_t happens inside the ns-3 script rather than Python -> C++.

@non-det-alle non-det-alle force-pushed the develop branch 2 times, most recently from ca4e911 to 9a401e9 Compare November 10, 2023 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants