-
Notifications
You must be signed in to change notification settings - Fork 7.7k
feat(udp): Allow to change the async_udp task stack size #12003
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
Conversation
👋 Hello mathieucarbou, we appreciate your contribution to this project! 📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more. 🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project. Click to see more instructions ...
Review and merge process you can expect ...
|
Memory usage test (comparing PR against master branch)The table below shows the summary of memory usage change (decrease - increase) in bytes and percentage for each target.
Click to expand the detailed deltas report [usage change in BYTES]
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Test Results 76 files 76 suites 16m 41s ⏱️ Results for commit 6ea62d4. ♻️ This comment has been updated with latest results. |
|
@mathieucarbou |
Hello, That's what I did first if you look here: https://github.com/espressif/arduino-esp32/compare/61aaa44254c1ccc4e45eaecbee1cd268e7324795..b3c3a31df445ce1e6990b412e0b1740a50187930 But then I saw that all the macros are only defined in So for this one exceptionally, I should do both ? Is it what you mean ? Thanks :-) |
|
Since this has not been merged and the lib-builder did not get a chance to run Kconfig and generate the defines, they do not currently exists in Arduino at all. That is the reason why CI is failing and anyone that would normally try that code now will also get a compile error. Adding check on top will ensure that the code will compile in all cases. The other values are missing because before the libs used to be built in a different way and included in the core. Later the define check could be removed if need be. There are also better ways to handle this and allow overwrite in Arduino or pioarduino #ifndef CONFIG_ARDUINO_UDP_TASK_STACK_SIZE
#define CONFIG_ARDUINO_UDP_TASK_STACK_SIZE 4096
#endif
#ifndef ARDUINO_UDP_TASK_STACK_SIZE
#define ARDUINO_UDP_TASK_STACK_SIZE CONFIG_ARDUINO_UDP_TASK_STACK_SIZE
#endif
xTaskCreateUniversal(
_udp_task, "async_udp", ARDUINO_UDP_TASK_STACK_SIZE, NULL, CONFIG_ARDUINO_UDP_TASK_PRIORITY, (TaskHandle_t *)&_udp_task_handle, CONFIG_ARDUINO_UDP_RUNNING_CORE
);This way you can |
Thanks for your help! Should I do the same for CONFIG_ARDUINO_UDP_RUNNING_CORE and CONFIG_ARDUINO_UDP_TASK_PRIORITY then ? In my app I set CONFIG_ARDUINO_UDP_RUNNING_CORE to core 0 for example and on purpose since it is running critical in the callback and code I do not want to be impacted by the loop. |
Please do. We have overlooked this and since nobody asked for it, we did not catch it |
Example: `-D CONFIG_ARDUINO_UDP_TASK_STACK_SIZE=2048` The default is to much conservative since a correctly developed app probably won't allocate too much on stack on the callbacks. I currently see a wast of memory: `I (1222501) MONITOR: async_udp (p=3) 3416 bytes`
done! #ifndef CONFIG_ARDUINO_UDP_TASK_STACK_SIZE
#define CONFIG_ARDUINO_UDP_TASK_STACK_SIZE 4096
#endif
#ifndef ARDUINO_UDP_TASK_STACK_SIZE
#define ARDUINO_UDP_TASK_STACK_SIZE CONFIG_ARDUINO_UDP_TASK_STACK_SIZE
#endif
#ifndef CONFIG_ARDUINO_UDP_TASK_PRIORITY
#define CONFIG_ARDUINO_UDP_TASK_PRIORITY 3
#endif
#ifndef ARDUINO_UDP_TASK_PRIORITY
#define ARDUINO_UDP_TASK_PRIORITY CONFIG_ARDUINO_UDP_TASK_PRIORITY
#endif
#ifndef CONFIG_ARDUINO_UDP_RUNNING_CORE
#define CONFIG_ARDUINO_UDP_RUNNING_CORE -1
#endif
#ifndef ARDUINO_UDP_RUNNING_CORE
#define ARDUINO_UDP_RUNNING_CORE CONFIG_ARDUINO_UDP_RUNNING_CORE
#endifI set -1 like it is done at other places, hopping this is ok for async udp too. |
It is. |
SuGlider
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Example:
-D CONFIG_ARDUINO_UDP_TASK_STACK_SIZE=2048The default is to much conservative since a correctly developed app probably won't allocate too much on stack on the callbacks. I currently see a waste of memory:
I (1222501) MONITOR: async_udp (p=3) 3416 bytes