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

Increase events.share-eventsize to 768B because of ESP8266 AT driver and asynchronous DNS #9799

Merged
merged 3 commits into from Feb 27, 2019

Conversation

Projects
None yet
8 participants
@VeijoPesonen
Copy link
Contributor

commented Feb 21, 2019

Description

events.shared-eventsize: increased from 256B to 768B

Original value was too small once both ESP8266 driver and
asynchronous DNS started to use shared event queue. An assumption is
made that once shared event queue is taken into use there are going to
be multiple users instead of one, for which the original value would
have been sufficient.

Pull request type

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

Reviewers

@kjbracey-arm
@SeppoTakalo
@geky
@michalpasztamobica

Release Notes

"events.shared-eventsize" increased from 256B to 768B

Original value was too small once both ESP8266 driver and
asynchronous DNS started to use shared event queue. An assumption is
made that once shared event queue is taken into use there are going to
be multiple users instead of one, for which the original value would
have been sufficient.

@VeijoPesonen

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

mbedgt: test case report:

| target              | platform_name | test suite          | test case                             | passed | failed | result | elapsed_time (sec) |
|---------------------|---------------|---------------------|---------------------------------------|--------|--------|--------|--------------------|
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-netsocket-dns | ASYNCHRONOUS_DNS                      | 1      | 0      | OK     | 0.28               |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-netsocket-dns | ASYNCHRONOUS_DNS_CACHE                | 1      | 0      | OK     | 0.62               |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-netsocket-dns | ASYNCHRONOUS_DNS_CANCEL               | 1      | 0      | OK     | 5.35               |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-netsocket-dns | ASYNCHRONOUS_DNS_EXTERNAL_EVENT_QUEUE | 1      | 0      | OK     | 3.13               |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-netsocket-dns | ASYNCHRONOUS_DNS_INVALID_HOST         | 1      | 0      | OK     | 1.15               |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-netsocket-dns | ASYNCHRONOUS_DNS_NON_ASYNC_AND_ASYNC  | 1      | 0      | OK     | 0.79               |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-netsocket-dns | ASYNCHRONOUS_DNS_SIMULTANEOUS         | 1      | 0      | OK     | 0.75               |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-netsocket-dns | ASYNCHRONOUS_DNS_SIMULTANEOUS_CACHE   | 1      | 0      | OK     | 0.75               |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-netsocket-dns | ASYNCHRONOUS_DNS_SIMULTANEOUS_REPEAT  | 1      | 0      | OK     | 4.95               |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-netsocket-dns | ASYNCHRONOUS_DNS_TIMEOUTS             | 1      | 0      | OK     | 7.8                |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-netsocket-dns | SYNCHRONOUS_DNS                       | 1      | 0      | OK     | 0.1                |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-netsocket-dns | SYNCHRONOUS_DNS_INVALID               | 1      | 0      | OK     | 1.31               |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-netsocket-dns | SYNCHRONOUS_DNS_MULTIPLE              | 1      | 0      | OK     | 0.61               |
mbedgt: test case results: 13 OK
mbedgt: completed in 88.79 sec

@VeijoPesonen VeijoPesonen changed the title Increase events.share-eventsize to 768B because of ESP8266 AT driver and asynchronous DNS Increase events.share-eventsize to 768kB because of ESP8266 AT driver and asynchronous DNS Feb 21, 2019

@VeijoPesonen VeijoPesonen changed the title Increase events.share-eventsize to 768kB because of ESP8266 AT driver and asynchronous DNS Increase events.share-eventsize to 768B because of ESP8266 AT driver and asynchronous DNS Feb 21, 2019

@ciarmcom ciarmcom requested review from geky, kjbracey-arm, SeppoTakalo and ARMmbed/mbed-os-maintainers Feb 21, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

features/netsocket/NetworkStack.cpp Outdated
}
}

return NSAPI_ERROR_OK;
EXIT:

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Feb 21, 2019

Contributor

Make this block name as NO_MEM, and get rid of the "ret = " temporary variable, as it serves no purpose.

This comment has been minimized.

Copy link
@VeijoPesonen

VeijoPesonen Feb 22, 2019

Author Contributor

Fixed

@cmonr cmonr added needs: work and removed needs: review labels Feb 21, 2019

@VeijoPesonen VeijoPesonen force-pushed the VeijoPesonen:bugfix-esp8266-dns branch Feb 22, 2019

@VeijoPesonen

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

@teetak01, @simosillankorva for your information

@cmonr cmonr added needs: review and removed needs: work labels Feb 22, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2019

@VeijoPesonen A style nit, but should be good to go afterwards: https://travis-ci.org/ARMmbed/mbed-os/jobs/496917472

@cmonr

cmonr approved these changes Feb 22, 2019

@VeijoPesonen VeijoPesonen force-pushed the VeijoPesonen:bugfix-esp8266-dns branch Feb 25, 2019

@VeijoPesonen

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

@cmonr fixed. I should stop wasting other people's time and run the check myself...

git diff --name-only --diff-filter=d `git merge-base HEAD master`..HEAD | ( grep '.\(c\|cpp\|h\|hpp\)$' || true ) | ( fgrep -v -f .astyleignore || true ) | while read file; do astyle -n --options=.astylerc "${file}"; done

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

Waiting for final reviews, started CI meanwhile

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 25, 2019

Test run: FAILED

Summary: 1 of 1 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_unittests

@0xc0170 0xc0170 removed the needs: CI label Feb 25, 2019

@VeijoPesonen

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

@0xc0170 , from CMakeError.log. I would like to say this isn't due to my changes. My mistake. Would you please rerun the tests if possible.

Determining if the pthread_create exist failed with the following output:
Change Dir: /builds/ws/mbed-os-ci_unittests/unitTest-2237/mbed-os/build/CMakeFiles/CMakeTmp

Run Build Command:"make" "cmTC_a0f5e/fast"
make -f CMakeFiles/cmTC_a0f5e.dir/build.make CMakeFiles/cmTC_a0f5e.dir/build
make[1]: Entering directory '/builds/ws/mbed-os-ci_unittests/unitTest-2237/mbed-os/build/CMakeFiles/CMakeTmp'
Building C object CMakeFiles/cmTC_a0f5e.dir/CheckSymbolExists.c.o
/usr/bin/gcc-5     -o CMakeFiles/cmTC_a0f5e.dir/CheckSymbolExists.c.o   -c /builds/ws/mbed-os-ci_unittests/unitTest-2237/mbed-os/build/CMakeFiles/CMakeTmp/CheckSymbolExists.c
Linking C executable cmTC_a0f5e
/usr/bin/cmake -E cmake_link_script CMakeFiles/cmTC_a0f5e.dir/link.txt --verbose=1
/usr/bin/gcc-5       CMakeFiles/cmTC_a0f5e.dir/CheckSymbolExists.c.o  -o cmTC_a0f5e -rdynamic 
CMakeFiles/cmTC_a0f5e.dir/CheckSymbolExists.c.o: In function `main':
CheckSymbolExists.c:(.text+0x16): undefined reference to `pthread_create'
collect2: error: ld returned 1 exit status
CMakeFiles/cmTC_a0f5e.dir/build.make:97: recipe for target 'cmTC_a0f5e' failed
make[1]: *** [cmTC_a0f5e] Error 1
make[1]: Leaving directory '/builds/ws/mbed-os-ci_unittests/unitTest-2237/mbed-os/build/CMakeFiles/CMakeTmp'
Makefile:126: recipe for target 'cmTC_a0f5e/fast' failed
make: *** [cmTC_a0f5e/fast] Error 2

File /builds/ws/mbed-os-ci_unittests/unitTest-2237/mbed-os/build/CMakeFiles/CMakeTmp/CheckSymbolExists.c:
/* */
#include <pthread.h>

int main(int argc, char** argv)
{
  (void)argv;
#ifndef pthread_create
  return ((int*)(&pthread_create))[argc];
#else
  (void)argc;
  return 0;
#endif
}


@michalpasztamobica

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

@VeijoPesonen , I think the failure is due to using mbed_error in NetworkStack.
Fortunately there is already a stub in UNITTESTS/stubs/mbed_error.c and you probably just need to add it to UNITTESTS/features/netsocket/TCPSocket/unittest.cmake (and other unit tests cmake files).

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

CI started

@cmonr cmonr added needs: CI and removed needs: work labels Feb 25, 2019

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 25, 2019

Test run: FAILED

Summary: 1 of 1 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_unittests
@VeijoPesonen

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

Thanks @michalpasztamobica , wouldn't have crossed my mind the issue would be something like that.

VeijoPesonen added some commits Feb 21, 2019

events.shared-eventsize: increased from 256B to 768B
Original value was too small once both ESP8266 driver and
asynchronous DNS started to use shared event queue. An assumption is
made that once shared event queue is taken into use there are going to
be multiple users instead of one, for which the original value would
have been sufficient.

@VeijoPesonen VeijoPesonen force-pushed the VeijoPesonen:bugfix-esp8266-dns branch to 5caed17 Feb 26, 2019

@VeijoPesonen

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

100% tests passed, 0 tests failed out of 43

Fixed, thanks @michalpasztamobica. @cmonr , would you please restart the CI.

@bulislaw

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

This PR is at risk of missing 5.12 release as it's marked as "needs: work". Code freeze is coming! On Friday 1st. Please made necessary updates ASAP and make sure the reviewers are aligned for prompt code inspection.

@cmonr cmonr added needs: CI and removed needs: work labels Feb 27, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 27, 2019

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 3
Build artifacts

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Feb 27, 2019

@0xc0170 0xc0170 merged commit 6bdbe75 into ARMmbed:master Feb 27, 2019

27 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR8 Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR8 Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9083 cycles
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@VeijoPesonen VeijoPesonen deleted the VeijoPesonen:bugfix-esp8266-dns branch Feb 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.