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

ESP32 task management #2371

Merged
merged 11 commits into from
Sep 23, 2021
Merged

ESP32 task management #2371

merged 11 commits into from
Sep 23, 2021

Conversation

mikee47
Copy link
Contributor

@mikee47 mikee47 commented Sep 22, 2021

This PR aims to fix some issues relating to tasks and Sming. Using an esp32-s2, here's the task list as it looks presently:

#   | Core | Prio | Run Time | % Time | Name
  9 |   0  |   23 |    40591 |    2%  | wifi
  2 |   0  |   22 |    28009 |    1%  | esp_timer
  1 |   0  |   20 |        0 |    0%  | sys_evt
  8 |   0  |   18 |     3003 |    0%  | tiT
  5 |   0  |    1 |        0 |    0%  | Tmr Svc
  6 |   0  |    1 |  1928397 |   96%  | Sming
  4 |   0  |    0 |        0 |    0%  | IDLE

*tiT == TCP/IP (LWIP)
*Tmr Svc == FreeRTOS software timers (low resolution, runs at tick rate 1kHz)

And for this PR:

#   | Core | Prio | Run Time | % Time | Name
  7 |   0  |   23 |        0 |    0%  | wifi
  1 |   0  |   22 |       46 |    0%  | esp_timer
  5 |   0  |   20 |    21243 |    1%  | Sming
  6 |   0  |   18 |      375 |    0%  | tiT
  4 |   0  |    1 |        0 |    0%  | Tmr Svc
  3 |   0  |    0 |  1978336 |   98%  | IDLE

This is for a dual-core ESP32:

#   | Core | Prio | Run Time | % Time | Name
  1 |   0  |   24 |        0 |    0%  | ipc0
 14 |   0  |   23 |    25717 |    0%  | wifi
  3 |   0  |   22 |     2537 |    0%  | esp_timer
  8 |   0  |    1 |        0 |    0%  | Tmr Svc
  6 |   0  |    0 |  1971743 |   49%  | IDLE
  2 |   1  |   24 |        0 |    0%  | ipc1
 12 |   1  |   20 |    31857 |    0%  | Sming
 13 |   1  |   18 |     2088 |    0%  | tiT
  7 |   1  |    0 |  1966055 |   49%  | IDLE 

*ipcN == InterProcess Communication allows IDF to run code in the context of a specific CPU.

The FreeRTOS scheduler will run the highest-priority thread which is ready to do work.
It reverts to 'round-robin' if two ready threads have the same priority.

Sming is now a high priority task, so is now capable of blocking other tasks.
Sming takes priority over the tcpip thread.

The Sming task implements the standard event loop.
This ensures events are handled in the correct context.
When there are no events to process it will idle correctly and allow lower priority tasks to run.
The task watchdog is configured to fire if an event takes more than about 7 seconds.

Sming code makes good use of stack for performance reasons and needs a reasonably-sized stack (currently 16KBytes).
This applies to any tasks which run Sming code, currently the tcpip and Sming tasks.

We now have one less task to deal with.

Default event queue

Once the default event queue is running we could just use the existing code as-is.
However, to get the task watchdog to work correctly we need a custom event handling loop.

That involves replacing all the default event loop functions.

Another benefit of this approach is that where there are no events to process the CPU is allowed to idle.
This is more consistent with how the IDF is supposed to work and may be important for power saving.

Software timers

Sming uses its Timer2 definition for this which now no longer uses the 'legacy' FRC timer implementation.

The IDF provides a queue for software timers managed by the esp_timer task.
As this is a higher priority than Sming it behaves like an interrupt, pre-empting Sming when it has work to do.
Instead of handling the timer event directly, we post it to the event queue so it is handled in the main Sming task.

Hardware timers

Sming uses its Timer1 definition for this, which has now been implemented so the HardwareTimer class works as expected.
It uses Timer Group 0, Index 0 for this.

Other changes

Wait for UART FIFOs to empty at startup to avoid truncated debug messages.

Found cache2phys SDK call which implements flashmem_get_address.

SDK config only needs to specify CONFIG_BOOTLOADER_LOG_LEVEL setting, others are handled automatically.

Add Wrap build function to replace multiple uses of -Wl,-wrap,XXXX in makefiles.

WifiEventsImpl attempts to register handlers in static constructor, so provide wifi_set_event_handler_cb to defer this (as for ESP8266).

Add TaskStat class to allow simple monitoring of task usage. Add example use to Basic_IFS sample.

Smooths transition from SDK printing to Sming driver.
This avoids truncated debug messages.
Ensures events are always run in Sming task.

All default event loop functions replaced in order to service watchdog correctly.
Allows idle task to operate as intended.

WifiEventsImpl attempts to register handlers in static constructor, so provide `wifi_set_event_handler_cb`
to defer this (as for ESP8266).
@slaff slaff added this to the 4.4.0 milestone Sep 22, 2021
Timer task is high priority so treat like an interrupt.
As timer events are posted to event task the timer task no longer needs a large stack.

Use custom os_timer_t structure.
NB. As timer events are posted to event task the timer task no longer needs a large stack.
@mikee47 mikee47 force-pushed the feature/esp32-tasks branch from ea9144f to 4fa246b Compare September 22, 2021 14:23
@mikee47
Copy link
Contributor Author

mikee47 commented Sep 22, 2021

I think there's still a potential race hazard with Sming and tcpip that is a bit more tricky to resolve.

If the tcpip task is running Sming's network code, it could get pre-empted by a Sming event. To sort that one would need to find a way to force such a situation. Note that the existing setup is still just as prone - perhaps more so -because the tcpip task could pre-empt Sming whilst starting a request.

Now I haven't observed anything weird as yet, but it should be possible to work up some test code which forces an issue with this.
It may be necessary to protect all the tcpip entry points with a semaphore.

esp_event_handler_t wifiEventHandler;

/*
* Initalise NVS which IDF WiFi uses to storage configuration parameters.
Copy link
Contributor

Choose a reason for hiding this comment

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

uses to storage configuration ...

I guess you've meant: "uses to store configuration..."

@slaff
Copy link
Contributor

slaff commented Sep 23, 2021

Now I haven't observed anything weird as yet, but it should be possible to work up some test code which forces an issue with this.
It may be necessary to protect all the tcpip entry points with a semaphore.

So far it looks good to me too. I was not able to segfault with the samples and my ultra-complex-convoluted test app so it is a good start. Shall I merge this PR as it is or you would like to add the test case also to this PR? (I would prefer to be a separate PR with semaphores and stuff ... )

@mikee47
Copy link
Contributor Author

mikee47 commented Sep 23, 2021

Now I haven't observed anything weird as yet, but it should be possible to work up some test code which forces an issue with this.
It may be necessary to protect all the tcpip entry points with a semaphore.

So far it looks good to me too. I was not able to segfault with the samples and my ultra-complex-convoluted test app so it is a good start.

Great!

Shall I merge this PR as it is or you would like to add the test case also to this PR? (I would prefer to be a separate PR with semaphores and stuff ... )

I'm happy with this PR as-is, so yes leave any tcpip work for later. I'm still trying to understand the flow for tcpip in the IDF as clearly they will have considered this. The task view is helpful as it shows how much traffic is getting routed through each task so think we're on the right track.

@slaff slaff merged commit fea192a into SmingHub:develop Sep 23, 2021
@mikee47 mikee47 deleted the feature/esp32-tasks branch September 23, 2021 09:25
mikee47 added a commit to mikee47/Sming that referenced this pull request Sep 27, 2021
Cannot set `CONFIG_BOOTLOADER_LOG_LEVEL` directly, only through choice variables
mikee47 added a commit to mikee47/Sming that referenced this pull request Sep 27, 2021
Cannot set `CONFIG_BOOTLOADER_LOG_LEVEL` directly, only through choice variables
mikee47 added a commit to mikee47/Sming that referenced this pull request Sep 27, 2021
Cannot set `CONFIG_BOOTLOADER_LOG_LEVEL` directly, only through choice variables
slaff pushed a commit that referenced this pull request Sep 27, 2021
This PR aims to fix some issues relating to tasks and Sming. Using an esp32-s2, here's the task list as it looks presently:

```
#   | Core | Prio | Run Time | % Time | Name
  9 |   0  |   23 |    40591 |    2%  | wifi
  2 |   0  |   22 |    28009 |    1%  | esp_timer
  1 |   0  |   20 |        0 |    0%  | sys_evt
  8 |   0  |   18 |     3003 |    0%  | tiT
  5 |   0  |    1 |        0 |    0%  | Tmr Svc
  6 |   0  |    1 |  1928397 |   96%  | Sming
  4 |   0  |    0 |        0 |    0%  | IDLE
```

*tiT == TCP/IP (LWIP)
*Tmr Svc == FreeRTOS software timers (low resolution, runs at tick rate 1kHz)


And for this PR:


```
#   | Core | Prio | Run Time | % Time | Name
  7 |   0  |   23 |        0 |    0%  | wifi
  1 |   0  |   22 |       46 |    0%  | esp_timer
  5 |   0  |   20 |    21243 |    1%  | Sming
  6 |   0  |   18 |      375 |    0%  | tiT
  4 |   0  |    1 |        0 |    0%  | Tmr Svc
  3 |   0  |    0 |  1978336 |   98%  | IDLE
```

This is for a dual-core ESP32:

```
#   | Core | Prio | Run Time | % Time | Name
  1 |   0  |   24 |        0 |    0%  | ipc0
 14 |   0  |   23 |    25717 |    0%  | wifi
  3 |   0  |   22 |     2537 |    0%  | esp_timer
  8 |   0  |    1 |        0 |    0%  | Tmr Svc
  6 |   0  |    0 |  1971743 |   49%  | IDLE
  2 |   1  |   24 |        0 |    0%  | ipc1
 12 |   1  |   20 |    31857 |    0%  | Sming
 13 |   1  |   18 |     2088 |    0%  | tiT
  7 |   1  |    0 |  1966055 |   49%  | IDLE 
```

*ipcN == InterProcess Communication allows IDF to run code in the context of a specific CPU.

The FreeRTOS scheduler will run the highest-priority thread which is ready to do work.
It reverts to 'round-robin' if two ready threads have the same priority.

Sming is now a high priority task, so is now capable of blocking other tasks.
Sming takes priority over the tcpip thread.

The Sming task implements the standard event loop.
This ensures events are handled in the correct context.
When there are no events to process it will idle correctly and allow lower priority tasks to run.
The task watchdog is configured to fire if an event takes more than about 7 seconds.

Sming code makes good use of stack for performance reasons and needs a reasonably-sized stack (currently 16KBytes).
This applies to any tasks which run Sming code, currently the tcpip and Sming tasks.

We now have one less task to deal with.


**Default event queue**

Once the default event queue is running we could just use the existing code as-is.
However, to get the task watchdog to work correctly we need a custom event handling loop.

That involves replacing all the default event loop functions.

Another benefit of this approach is that where there are no events to process the CPU is allowed to idle.
This is more consistent with how the IDF is supposed to work and may be important for power saving.


**Software timers**

Sming uses its `Timer2` definition for this which now no longer uses the 'legacy' FRC timer implementation.

The IDF provides a queue for software timers managed by the `esp_timer` task.
As this is a higher priority than Sming it behaves like an interrupt, pre-empting Sming when it has work to do.
Instead of handling the timer event directly, we post it to the event queue so it is handled in the main Sming task.


**Hardware timers**

Sming uses its `Timer1` definition for this, which has now been implemented so the HardwareTimer class works as expected.
It uses Timer Group 0, Index 0 for this.


**Other changes**


Wait for UART FIFOs to empty at startup to avoid truncated debug messages.

Found `cache2phys` SDK call which implements `flashmem_get_address`.

SDK config only needs to specify `CONFIG_BOOTLOADER_LOG_LEVEL` setting, others are handled automatically.

Add `Wrap` build function to replace multiple uses of `-Wl,-wrap,XXXX` in makefiles.

WifiEventsImpl attempts to register handlers in static constructor, so provide `wifi_set_event_handler_cb` to defer this (as for ESP8266).

Add `TaskStat` class to allow simple monitoring of task usage. Add example use to Basic_IFS sample.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants