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

Avoid duplicate uplink sequence numbers when receiving bad MIC or add… #22

Closed
wants to merge 3 commits into from

Conversation

catsup
Copy link

@catsup catsup commented Nov 2, 2015

…ress in confirmed transmits.

If during a confirmed message transmission we receive a downlink frame with either
an address not matching ours, or an incorrect MIC, control is returned to the user
application with an error status, and the upLinkCounter is not incremented. Thus,
the next uplink frame is sent with the same sequence number, and likely discarded
by the receiving server.

The fix involves discarding the invalid downlink and continuing the retransmission
of the uplink frame until either a downlink response/ack is received, or the retry
count is reached.

…ress in confirmed transmits.

If during a confirmed message transmission we receive a downlink frame with either
an address not matching ours, or an incorrect MIC, control is returned to the user
application with an error status, and the upLinkCounter is not incremented. Thus,
the next uplink frame is sent with the same sequence number, and likely discarded
by the receiving server.

The fix involves discarding the invalid downlink and continuing the retransmission
of the uplink frame until either a downlink response/ack is received, or the retry
count is reached.
@catsup
Copy link
Author

catsup commented Nov 2, 2015

Note this patch also applies to Release 3.3.

@catsup catsup closed this Nov 2, 2015
@catsup catsup reopened this Nov 2, 2015
@reissjason
Copy link
Contributor

Calling the rx2 timer callback will not have the correct timing. The window will be opened when the timee r expires.

@mluis1
Copy link
Contributor

mluis1 commented Nov 3, 2015

We will investigate the issue.

@catsup
Copy link
Author

catsup commented Nov 3, 2015

Hi Jason,

yes, good point - the rx isn't really needed, but it makes sure all the proper callbacks get invoked.

Regards,
Marc

Marc Bongartz added 2 commits November 4, 2015 16:53
…when receiving bad MIC or address in confirmed transmits."

This reverts commit 50e057a.
The uplink counter would not be incremented if, during a confirmed uplink
transmission, the mac receives a frame with an invalid address or mic error.

In addition, no further retransmissions would occur after such a downlink.

This change should address the above two issues.
@catsup catsup closed this Nov 4, 2015
@catsup catsup reopened this Nov 4, 2015
@mluis1
Copy link
Contributor

mluis1 commented Nov 4, 2015

May we ask you to follow the coding conventions in place.

Please use spaces instead of TAB characters.

Your modification should look like to the following:

        } 
        else
        {
            /*
             * For confirmed uplinks, ignore MIC and address errors and keep retrying.
             */
            if( ( LoRaMacEventInfo.Status == LORAMAC_EVENT_INFO_STATUS_MIC_FAIL ) ||
                ( LoRaMacEventInfo.Status == LORAMAC_EVENT_INFO_STATUS_ADDRESS_FAIL ) )
            {
                AckTimeoutRetry = true;
            }

Instead of:

        } else {
                /*
                 * For confirmed uplinks, ignore MIC and address errors and keep retrying.
                 */
                if ((LoRaMacEventInfo.Status == LORAMAC_EVENT_INFO_STATUS_MIC_FAIL) ||
                    (LoRaMacEventInfo.Status == LORAMAC_EVENT_INFO_STATUS_ADDRESS_FAIL)) {
                AckTimeoutRetry = true;
            }

Once we have tested this fix we will merge it.

Thanks.

@catsup
Copy link
Author

catsup commented Nov 4, 2015

Sure, no problem.

Thanks,
Marc

@mluis1
Copy link
Contributor

mluis1 commented Nov 6, 2015

Applied on V3.4.1 version.

@mluis1 mluis1 closed this Nov 6, 2015
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.

3 participants