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

UDPSOCKET_ECHOTEST change to tolerate duplicate packets #11693

Merged
merged 1 commit into from Nov 4, 2019

Conversation

@mtomczykmobica
Copy link
Contributor

mtomczykmobica commented Oct 16, 2019

Description

UDP should ignore duplicate packets, as some networks may produce those.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@SeppoTakalo
@AnttiKauppila
@michalpasztamobica

Release Notes

@ciarmcom ciarmcom requested review from AnttiKauppila, michalpasztamobica, SeppoTakalo and ARMmbed/mbed-os-maintainers Oct 16, 2019
@ciarmcom

This comment has been minimized.

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 16, 2019

Please fix styling (see travis astyle failure)

Copy link
Member

0xc0170 left a comment

Once Travis passing

@mtomczykmobica mtomczykmobica force-pushed the mtomczykmobica:ONME-4398 branch 3 times, most recently from f176ba7 to c03f60c Oct 16, 2019
@mtomczykmobica

This comment has been minimized.

Copy link
Contributor Author

mtomczykmobica commented Oct 16, 2019

Please fix styling (see travis astyle failure)

Done

received_duplicate_packet = true;
}
} while (received_duplicate_packet);

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Oct 16, 2019

Contributor

I don't like this logic.

You assume that all duplicates will be received in order.

This might be true for ECHOTEST, but might not be true for ECHOTEST_BURST.

So while re-working this, can you change logic so that it tolerates re-ordering as well.

I would assume this kind of logic might work:

// List of packet sizes to send
static const int pkt_sizes[PKTS] = {...);
// List of boolean flags to mark each size as "received"
static bool pkt_received[PKTS] = {false};

// ... in the test case
for (int retry_cnt = 0; retry_cnt <= 2; retry_cnt++) {
    ...
    sent = sock.sendto(udp_addr, tx_buffer, pkt_s);
    if (...) {
        // errors..
    }
    do {
        recvd = sock.recvfrom(...);
        // Check that packet is what we sent
        if (recvd == pkt_sizes[s_idx] && memcmp(rx_last, rx_buffer, recvd) == 0)
            pkt_received[s_idx] = true;
        }
    } while(pkt_received[s_idx] == false);
}

So just accept what we were waiting for, ignore the rest.

This comment has been minimized.

Copy link
@mtomczykmobica

mtomczykmobica Oct 16, 2019

Author Contributor

I've changed logic. Now responses for all sends and duplicates of this responses are accepted. Other packets with incorrect size are interpreted as error.

@mtomczykmobica mtomczykmobica force-pushed the mtomczykmobica:ONME-4398 branch from c03f60c to 11864c7 Oct 16, 2019
@mtomczykmobica mtomczykmobica requested a review from SeppoTakalo Oct 16, 2019
Copy link
Contributor

SeppoTakalo left a comment

This looks OK.

One thing I'm wondering, do we set the socket timeout, or does it hang if there is packet losses?

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Oct 31, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Nov 4, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Nov 4, 2019

Test run: SUCCESS

Summary: 5 of 5 test jobs passed
Build number : 1
Build artifacts

@0xc0170 0xc0170 merged commit 5303b10 into ARMmbed:master Nov 4, 2019
19 checks passed
19 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8649 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8464B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
@0xc0170 0xc0170 removed the ready for merge label Nov 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.