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

Adding core selection support #10

Closed
wants to merge 4 commits into from

Conversation

Voha888
Copy link
Contributor

@Voha888 Voha888 commented Jan 13, 2021

Due to my poor knowledge of the English language, I did not add comments to the code.
I made two commits.
The first one adds the ability to specify the kernel for the background process
The second, when creating multiple instances of the process (several stepper motors), removes the message from Serial: [E] [esp32-hal-misc.c: 94] disableCore0WDT (): Failed to remove Core 0 IDLE task from WDT
It was not done very nicely, but I did not find a method to check if WDT is disabled or not.

Copy link
Contributor Author

@Voha888 Voha888 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replacing xTaskCreatePinnedToCore with xTaskCreateUniversal

@Voha888
Copy link
Contributor Author

Voha888 commented Jan 14, 2021

I did a little wrong, instead of a global variable, I should have used a static variable

@pkerspe pkerspe changed the title Adding dual core support Adding core selection support Jan 14, 2021
src/ESP_FlexyStepper.cpp Show resolved Hide resolved
{
if(!core_0_wdt_was_disabled_from_flexyStepper){
disableCore0WDT(); // we have to disable the Watchdog timer to prevent it from rebooting the ESP all the time another option would be to add a vTaskDelay but it would slow down the stepper
core_0_wdt_was_disabled_from_flexyStepper=1;
}

xTaskCreatePinnedToCore(
xTaskCreateUniversal(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Voha888 can you point me to the api doc for this function "xTaskCreateUniversal"? I cannot find and doc on it and what the difference is to "xTaskCreatePinnedToCore"

https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/system/freertos.html

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately I am not a programmer and I have not read the documentation on freertos. But please take a look at the implementation of this function in the ESP32:

BaseType_t xTaskCreateUniversal( TaskFunction_t pxTaskCode,
                        const char * const pcName,
                        const uint32_t usStackDepth,
                        void * const pvParameters,
                        UBaseType_t uxPriority,
                        TaskHandle_t * const pxCreatedTask,
                        const BaseType_t xCoreID ){
#ifndef CONFIG_FREERTOS_UNICORE
    if(xCoreID >= 0 && xCoreID < 2) {
        return xTaskCreatePinnedToCore(pxTaskCode, pcName, usStackDepth, pvParameters, uxPriority, pxCreatedTask, xCoreID);
    } else {
#endif
    return xTaskCreate(pxTaskCode, pcName, usStackDepth, pvParameters, uxPriority, pxCreatedTask);
#ifndef CONFIG_FREERTOS_UNICORE
    }
#endif
}

You may have see that in the header file, I set the kernel number to "-1", which means this function will call xTaskCreate, also, thanks to #ifndef CONFIG_FREERTOS_UNICORE, this function will work correctly on single-core versions of the ESP32 chip anyway.
Coming back to your comment #10 (comment)
we see that when the condition (xCoreID> = 0 && xCoreID <2) is satisfied, xTaskCreatePinnedToCore will be called

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the benefits of this function, yet I am worried that it is not documented anywhere, this could result in lack of support of this function in the future or in general. So how did you find out about this function if you also did not see it in any documentation? How can we be sure this function is officially supported?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just for reference definition of this function in git:
https://github.com/espressif/arduino-esp32/blob/master/cores/esp32/esp32-hal-misc.c#L116

I learned so far, that this is an Arduino-ESP32 specific function from this comment:
https://gitter.im/me-no-dev/ESPAsyncWebServer?at=5d97843e940b4c2fc0755933

couldn't find much more info about it, well, we can try it and hope it keeps getting maintained and not become deprecated at some point :-)

Copy link
Owner

@pkerspe pkerspe Jan 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more thing:
I noticed that the disableCore1WDT / disableCore0WDT are also only available on the dual core models, so if we use the xTaskCreateUniversal function which will perform the unicore check, we also need to wrap the WDT disabled code portions into the #ifndef guard statements otherwise it will fail to run/compile on single core EPS32 imho.
I never intended to support single core ESP32 since I doubt they will be able to provide jitter free stepper signals (which is already a problem on the dual core model) if the Wifi Stack is constantly interfering with the timing due to Interrupts, but if we go the route to support single core, we need to do it in all places.

https://github.com/espressif/arduino-esp32/blob/master/cores/esp32/esp32-hal-misc.c#L116:

#ifndef CONFIG_FREERTOS_UNICORE
void enableCore1WDT(){
    TaskHandle_t idle_1 = xTaskGetIdleTaskHandleForCPU(1);
    if(idle_1 == NULL || esp_task_wdt_add(idle_1) != ESP_OK){
        log_e("Failed to add Core 1 IDLE task to WDT");
    }
}

void disableCore1WDT(){
    TaskHandle_t idle_1 = xTaskGetIdleTaskHandleForCPU(1);
    if(idle_1 == NULL || esp_task_wdt_delete(idle_1) != ESP_OK){
        log_e("Failed to remove Core 1 IDLE task from WDT");
    }
}
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/espressif/arduino-esp32/blob/8d0e68db4f73c6243be4e9c5955ef1eb842dd95b/cores/esp32/esp32-hal-misc.c#L86
disableCore0WDT - is available on single core version.
disableCore1WDT() - we don't need it, at least for me it raises a warning the first time I try to use it.
So I don't see any problems

@@ -91,21 +91,21 @@ ESP_FlexyStepper::~ESP_FlexyStepper()
}

//TODO: use https://github.com/nrwiersma/ESP8266Scheduler/blob/master/examples/simple/simple.ino for ESP8266
void ESP_FlexyStepper::startAsService(bool core)
void ESP_FlexyStepper::startAsService(int core)
{
if(!core_0_wdt_was_disabled_from_flexyStepper){
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to check here if it is for the same core (according to variable name core 0). So a separated check needs to be added for core 1 as well....at least I guess so.
Not sure if it makes sense to disbaled WDT for core 0 if the user defined core = 1 and wants the task to run in core 1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's about removing the WDT disabling on core 0, when using only core 1, this is an interesting idea, I'll check it out.
Regarding disabling WDT on core 1 - in my version of ESP it is not there by default

Copy link
Owner

@pkerspe pkerspe Jan 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can use a single variable (with proper name) with default value = -1 (meaning the wdt has not yet been manually disabled), once disabled you can set the value to the core number. So the check could be

if(wdt_disabled_for_core == -1){
    ....
    wdt_disabled_for_core = core;
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be that the Arduino framework disabled the WDT on core 1 by default, this is why I mentioned that I am just guessing we need to do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#10 (comment)
This is a very interesting trick, and very cool. But it seems to me that such use is not entirely safe and appropriate in our case.

#10 (comment)
For your remarks, regarding disabling WDT0 only when creating tasks on kernel 0, I have already made edits and will now send it, I also added comments if possible

pkerspe added a commit that referenced this pull request Sep 27, 2021
@pkerspe
Copy link
Owner

pkerspe commented Sep 27, 2021

manually implemented the basic idea of this PR in commit 3ea6694 so closing it here

@pkerspe pkerspe closed this Sep 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants