net/gnrc_lorawan: implement unconfirmed uplink redundancy #15946
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jia200x
requested review from
cgundogan,
haukepetersen,
miri64 and
PeterKietzmann
as code owners
|
@akshaim might be interested in this feature |
jia200x
changed the title
jia200x
changed the title
jia200x
added
Area: LoRa
jia200x
force-pushed
the
pr/gnrc_lorawan_redundancy
branch
from
0d8f3a8
to
430b87c
Compare
akshaim
reviewed
Feb 17, 2021
| * not affect confirmed uplinks. By default, uplinks | ||
| * are sent without retransmissions (this means, the | ||
| * device sends only one uplink packet) | ||
| * |
There was a problem hiding this comment.
++ This value is usually configured by the network to control the redundancy of the node uplinks to achieve a given Quality of Service.
Maybe one more line to give a bit more context.
There was a problem hiding this comment.
This is used by ADR, but it's not directly related. Therefore I don't think these lines add to much for the documentation
There was a problem hiding this comment.
Otherwise this would be also for DR, TX Power, channels, etc
There was a problem hiding this comment.
Redundancy is only used when ADR is enabled right ?
There was a problem hiding this comment.
no, it is not :). You can also configure the node to do redundancy, as described in the "help" section.
In fact, if the ADRReq command sets the redundancy to 0, the standard specifies that the redundancy should be the device's default.
There was a problem hiding this comment.
alternatively we can avoid using redundancy when ADR is off. In fact, this is a gray zone in the standard IMO (it doesn't specifies what to do with the default value or when the redundancy is on)
There was a problem hiding this comment.
As discussed offline, lets try to avoid the grey area :)
There was a problem hiding this comment.
I adapted it to indicate it's for ADR
| not affect confirmed uplinks. By default, uplinks | ||
| are sent without retransmissions (this means, the | ||
| device sends only one uplink packet) | ||
|
|
There was a problem hiding this comment.
Can you correct the formatting here ?
akshaim
reviewed
Feb 17, 2021
sys/include/net/loramac.h
Outdated
| /** | ||
| * @brief Maximum unconfirmed uplink redundancy | ||
| */ | ||
| #define LORAMAC_REDUNDANCY_MAX (14U) |
There was a problem hiding this comment.
The valid range in LoRaMAC 1.0.3 spec is 1:15 ?
There was a problem hiding this comment.
for the number of transmissions, However, here these numbers are defined as "retransmissions" (therefore 15 maps to 14)
There was a problem hiding this comment.
Yes. Maybe it makes sense to rename macro to LORAMAC_REDUNDANCY_MAX_RETRANSMISSIONS or something similar ?
There was a problem hiding this comment.
as discuess offline, I can add the explanation of where the 14 comes from
There was a problem hiding this comment.
I removed it. I realized it's not needed because ADR already limits this
|
@jia200x Looks good to me. Please squash. |
jia200x
force-pushed
the
pr/gnrc_lorawan_redundancy
branch
from
1502ad8
to
3463e5a
Compare
|
Can you rebase to current master ? |
jia200x
force-pushed
the
pr/gnrc_lorawan_redundancy
branch
from
3463e5a
to
a63ce18
Compare
|
rebased! |
|
Great, I shall test it tomorrow :) |
akshaim
reviewed
Jul 13, 2021
jia200x
force-pushed
the
pr/gnrc_lorawan_redundancy
branch
from
a63ce18
to
ac94f36
Compare
jia200x
requested review from
aabadie,
fjmolinas and
MichelRottleuthner
as code owners
|
rebased on top of #16640 |
|
Test results: I configured the redundancy to 2. With that, every uplink is sent 3 times. Here's the output of the network server. As expected, the Unconfirmed uplinks are sent 3 times. |
|
As described in the test results, a downlink stops the retransmission procedure: The node only send one message (the downlink is the result of the Link Check command): The retransmission procedure of confirmable message also works: And the retransmission value is not affect by the redundancy (retrans is set to 5 -> 6 uplinks): |
|
Last but not least: the proposed patch for testing doesn't make sense any more because the proposed function is now void. I added an assertion to make sure NS (redundancy configured to 14): I also dropped 9948072 since it's not used anymore after the request changes by @akshaim |
|
@jia200x I think we can proceed ahead :) |
|
@jia200x Can you squash ? |
akshaim
reviewed
Jul 14, 2021
leandrolanzieri
added
the
CI: ready for build
|
This needs rebasing and squashing |
|
Tested with Chirspstack and it seems to fail. Here are the results.
Please note the tests above were using the same Chirpstack instance and hardware.
This standard "frequency hopping" is described here: The implementation is doing that. Just picking a pseudo-random number. The only exceptions are bands that require to implement FHSS, such as US915. In this case the channel must not repeat. In fact, this is the Semtech LoRaMAC implementation for EU868. Therefore, this is not against the standard. Since there are only 3 channels in ABP, there are high chances you will see the same channel on retransmissions.
Of course, since redundancy is not applied to confirmed uplinks. |
This commit implements uplink redundancy in GNRC LoRaWAN. Uplink redundancy is used to retransmit unconfirmed uplink frames. The retransmission stops when either a downlink message is received or the number of uplink retransmissions is reached. This functionality doesn't affect confirmed uplinks.
jia200x
force-pushed
the
pr/gnrc_lorawan_redundancy
branch
from
2762998
to
df97f34
Compare
|
squashed! |
github-actions
bot
removed
Area: tests
|
Is there anything left here to do? |
I don't think so. This PR should be ready to merge |
benpicco
approved these changes
Sep 17, 2021
miri64
added
CI: ready for build
|
Last build was a bit stale, so let's do one more before merging. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contribution description
This PR implements uplink redundancy for GNRC LoRaWAN.
Uplink redundancy is used to retransmit unconfirmed uplink frames. The retransmission stops when either a downlink message is received or the number of uplink retransmissions is reached. This functionality doesn't affect confirmed uplinks.
Although this functionality is intended to be used by the upcoming implementation of ADR, this can be also set at compile time by the user.
Testing procedure
Check if this features still works with the following patch:(see net/gnrc_lorawan: implement unconfirmed uplink redundancy #15946 (comment))Issues/PRs references
None so far, but required for implementing ADR