Skip to content

Add code to verify if external Wifi module is still responsible #11819

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

Merged
merged 1 commit into from
Nov 25, 2019

Conversation

tymoteuszblochmobica
Copy link
Contributor

DNS timeout test fixed to mitigate ESP8266 Wifi module low performance.

Description (required)

DNS timeout test requires flooding device with DNS queries.
After this test finishes gethostbyname check is performed to ensure the device is still alive.

Summary of change (What the change is for and why)

DNS timeout test requires flooding device with DNS queries.
This causes problem to ESP8266 module causing it to stuck for 11 sec.
In result all following tests fails. To avoid this "smart delay" is added.
If device preforms gethostbyname correctly then tests can proceed, otherwise after 1 sec sleep gethostbyname is repeated until success or re-check limit (set to 15).
Thanks to this all ethernet and non ESP8266 wireless devices don't need to wait but ESP must wait.

Documentation (Details of any document updates required)

Pull request type (required)

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results (required)

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers (optional)

@SeppoTakalo
@AnttiKauppila

Release Notes (required for feature/major PRs)

Summary of changes

DNS timeout test module fix

Impact of changes
Migration actions required

@michalpasztamobica
Copy link
Contributor

I generally like this solution more than the blind sleep.
But let's not merge this before this fix gets verified: #11817

@ciarmcom ciarmcom requested review from AnttiKauppila, SeppoTakalo and a team November 6, 2019 12:00
@ciarmcom
Copy link
Member

ciarmcom commented Nov 6, 2019

@tymoteuszblochmobica, thank you for your changes.
@AnttiKauppila @SeppoTakalo @ARMmbed/mbed-os-test @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 6, 2019

DNS timeout test requires flooding device with DNS queries.
This causes problem to ESP8266 module causing it to stuck for 11 sec.
In result all following tests fails. To avoid this "smart delay" is added.
If device preforms gethostbyname correctly then tests can proceed, otherwise after 1 sec sleep gethostbyname is repeated until success or re-check limit (set to 15).
Thanks to this all ethernet and non ESP8266 wireless devices don't need to wait but ESP must wait.

@tymoteuszblochmobica Please provide these details in the commit msg. I've noticed in another PR, similar thin: very brief commit messages. It only helps if you can add these there.

Once updated, we will start CI

@0xc0170 0xc0170 changed the title Adeded code to verify if external Wifi module is still responsible Add code to verify if external Wifi module is still responsible Nov 6, 2019
@tymoteuszblochmobica
Copy link
Contributor Author

Updated commit msg.

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 7, 2019

But let's not merge this before this fix gets verified: #11817

Set to preceding PR, will wait

after DNS Query flooding during DNS timeout test.
DNS timeout test requires flooding device with DNS queries.
This causes problem to ESP8266 module causing it to stuck for 11 sec.
In result all following tests fails. To avoid this "smart delay" is added.
If device preforms gethostbyname correctly then tests can proceed.
Otherwise after 1 sec sleep gethostbyname is repeated until success or re-check limit (set to 15).
Thanks to this all ethernet and non ESP8266 wireless devices don't need to wait but ESP must wait.
@michalpasztamobica
Copy link
Contributor

@0xc0170 , can you help us figure out what the problem is? When we click "Details" we get 404 error.

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 20, 2019

pr-merge fixed (was internal error).

@michalpasztamobica As this still waits for another PR , is there anything we can do here?

@michalpasztamobica
Copy link
Contributor

@0xc0170 , I thought the other PR would come in faster, so perhaps I take back my original idea to wait... This PR is approved by Antti, lets merge it in.
It is an improvement over a fixed 12s delay, which we have right now. In case Antti's solution proves right, then we can remove it (instead of removing the fixed 12s delay). No point to keep it open, as it still is an improvement over the current situation.

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 20, 2019

@AnttiKauppila Please review the suggestion above and let us know if we proceed with this one for 5.15 ?

@michalpasztamobica
Copy link
Contributor

@AnttiKauppila , would you please, review again, this time - just to approve that this can go in now, without waiting for the ESP8266 fix being validated (that might take a while)?

@michalpasztamobica
Copy link
Contributor

@0xc0170 this seems ready for CI 🚀

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 25, 2019

CI restarted

@mbed-ci
Copy link

mbed-ci commented Nov 25, 2019

Test run: SUCCESS

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

@0xc0170 0xc0170 removed the needs: CI label Nov 25, 2019
@0xc0170 0xc0170 merged commit 860f18b into ARMmbed:master Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants