Adjustable Serial Event Task Stack Size And Priority #6685
Conversation
|
I will suggest that this PR is coupled with change to the component options here so that idf-component users are also able to configure this. |
Done! |
| @@ -82,6 +82,39 @@ choice ARDUINO_UDP_RUNNING_CORE | |||
|
|
|||
| endchoice | |||
|
|
|||
| choice ARDUINO_SERIAL_EVENT_TASK_RUNNING_CORE | |||
There was a problem hiding this comment.
this option should depend on wether the chip has more than one core. Example implementation: https://github.com/espressif/esp-idf/blob/master/components/lwip/Kconfig#L675
There was a problem hiding this comment.
Hello @me-no-dev , I'm a bit confused about this suggestion. In the other config entries (about running cores) there's no dependency on this define, should this be added to every entry aswell?
Also, I did a search about that specific define, but I've found CONFIG_FREERTOS_UNICORE and not FREERTOS_UNICORE, is that a bug in the line you linked to me?
I guess I could answer myself some of these questions, but I don't really know how to test this. I don't know what's the procedure to use this kconfig file. Could you point me to a guide?
There was a problem hiding this comment.
I went ahead anyway and pushed some changes. Let me know if I did it right! Thanks!
There was a problem hiding this comment.
CONFIG_ prefix is added to KConfig Options in its created header file automaticaly.
So Kconfig Menu option FREERTOS_UNICORE will be seen as a #define CONFIG_FREERTOS_UNICORE in the source code.
There was a problem hiding this comment.
I think we may need to review it in the whole Kconfig.projectbuild.
I also got the same doubt as @gonzabrusco ...
Shall we include this dependency for all not valid options when there is just one Core (S2/C3)?
Should the default option go to Core0 when there is just one Core?
There was a problem hiding this comment.
I think that if we use xTaskCreateUniversal it does not matter the default option.
arduino-esp32/cores/esp32/esp32-hal-misc.c
Line 148 in 0b3f1a9
By the way, thanks for the explanation on KConfig options. I removed the CONFIG_ prefix.
There was a problem hiding this comment.
True that xTaskCreateUniversal will take care regardless of how many cores, but this is a safe-net inside the function itself. It still makes sense to hide options that are invalid for the current target in ESP-IDF. This is how it is done for all IDF components. Arduino is no different :)
I think we may need to review it in the whole Kconfig.projectbuild.
Probably. It has not really been touched since ESP32, which has 2 cores always :D At least what we support for it.
There was a problem hiding this comment.
I'm on vacation now, so feel free to change whatever you want to move this forward.
There was a problem hiding this comment.
We are not in a hurry, so no worries :) Have fun at your vacation
|
Very nice indeed. Happy to see you use |
|
Conflicts with #6718, which seems to implement some of the same fixes. Recommend merging 6718 and then updating this PR as necessary. Great work on this @gonzabrusco ! |
Makes it alligned to changes in espressif#6718 Also eliminates conflict with espressif#6718 for merging
* Update Kconfig to autoselect the proper running core (espressif#6718) * Update Kconfig to autoselect the proper cunning core * Always run UDP on Core0 by default * Change SPI::transfer signature to match official Arduino API (espressif#6734) * Adjustable Serial Event Task Stack Size And Priority (espressif#6685) * Adjustable Serial Event Task Stack Size And Priority * Added options to Kconfig * Added Core Affinity * Added CONFIG_FREERTOS_UNICORE * Removed _CONFIG from FREERTOS_UNICORE * Fixing Core choice for OnReceive() Makes it alligned to changes in espressif#6718 Also eliminates conflict with espressif#6718 for merging Co-authored-by: Rodrigo Garcia <rodrigo.garcia@espressif.com> Co-authored-by: Me No Dev <me-no-dev@users.noreply.github.com> Co-authored-by: Me No Dev <me-no-dev@users.noreply.github.com> Co-authored-by: Gonzalo Brusco <gonzabrusco@gmail.com> Co-authored-by: Rodrigo Garcia <rodrigo.garcia@espressif.com>
|
Thanks for following this up @SuGlider @me-no-dev ! Great Work! |
This is just a proposal so we can change on compile time the stack size and priority of the Serial Event Task.
On my current software, the serial part does not fit the current 2048 bytes of stack that the Serial Event Task offers me. So I'm forced to implement a binary semaphore for synchronization (the serial event task just gives a semaphore). This is a waste of resources because now I have two tasks (one for the serial event task and one for my actual routine) and a semaphore. Even worse, it's slower.
Thanks!