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

cpu/esp8266: change of ETS task handling #10656

Merged
merged 7 commits into from
Jan 21, 2019

Conversation

gschorcht
Copy link
Contributor

@gschorcht gschorcht commented Dec 26, 2018

Contribution description

This PR changes the ETS task handling approach when using the Espressif SDK. This new ETS task handling approach is required for the upcoming ESP-NOW netdev driver. In addition, the default thread stack size can be reduced by half to save a lot of memory.

Update The PR is also prerequsit for refactoring in upcoming PR that reduces the code duplication by using some parts of the vendor code for ESP32 and ESP8266.

Background

To use the WiFi interface or software timers with the esp_sw_timers module, functions of the Espressif SDK have be used. For handling hardware interrupts, the SDK internally uses its own priority-based multitasking system, the so-called ETS:

  • Instead of handling hardware interrupts directly in the interrupt service routines, there is an ETS task for each hardware component.
  • The interrupt service routines use the ets_post ROM function to send an event to the corresponding ETS tasks, which then process the interrupts asynchronously.
  • The ETS uses the ROM function ets_run to periodically execute all ETS tasks with pending interrupt events in an endless loop. Context switches are not possible in interrupt service routines.

If the SDK is used, the ETS tasks with the pending interrupt events must also be processed by the RIOT port. For that purpose, the RIOT port overwrites the ROM functions ets_run and ets_post:

  • The ets_post function is overridden to receive a call back to the RIOT system in interrupt service routines.
  • The ets_run function is replaced by function ets_task_run. This function performs the execution of all ETS tasks with pending interrupt events exactly once.

Before this PR, function ets_task_run was called

  • before every context switch and
  • before and after going to sleep mode.

This approach had the disadvantage that context switches based on RIOT priorities were mixed with handling interrupt events in prioritized ETS tasks. This resulted in some stability issues with the WiFi interface and required a lot of memory for the thread stacks.

Changes

The approach introduced with this PR assumes that ETS tasks are only used to handle hardware interrupts and their handling requires therefore always the highest priority. It

  • introduces a flag which indicates that there are ETS tasks with pending interrupt events,
  • changes the ets_post function which is called at the end of an ETS interrupt service routine to set the the flag, and
  • introduces a separate thread ets_task_func with the highest possible priority which
    • waits for the flag and
    • calls the ets_task_run function to handle all ETS tasks with pending interrupt events once the flag is set.

Further changes with this PR are:

  • default thread stack size is downsized to the half
  • number of priority levels increased due to the additional thread
  • context switches based on software-interrupts have to be used by default
  • some cleanups of the makefile

Testing procedure

Compile tests/shell with enabled ets_sw_timer and flash any ESP8266 board, e.g.,

USEMODULE=ets_sw_timer make -C tests/shell BOARD=esp8266-eps-13x

Issues/PRs references

This PR is prerequisite for PRs #9917 and #10792 as well ass refactoring according to issue #10658.

@aabadie aabadie added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Platform: ESP Platform: This PR/issue effects ESP-based platforms labels Dec 28, 2018
@aabadie aabadie added this to the Release 2019.01 milestone Dec 28, 2018
@gschorcht gschorcht force-pushed the esp8266_ets_handling branch from 3fa6ac6 to 404297f Compare January 5, 2019 16:08
@gschorcht gschorcht force-pushed the esp8266_ets_handling branch from 32ff37b to e334e82 Compare January 11, 2019 14:40
@gschorcht
Copy link
Contributor Author

@aabadie @kaspar030 Is it possible to ask someone to review this PR because it is essential for further changes?

@gschorcht gschorcht added the Area: cpu Area: CPU/MCU ports label Jan 16, 2019
@smlng smlng self-requested a review January 21, 2019 10:43
Copy link
Member

@smlng smlng left a comment

Choose a reason for hiding this comment

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

one question, otherwise looks good. Tested together with #10792, which wasn't working without this one.

@@ -143,5 +147,5 @@ else
export FFLAGS += -p $(PORT) -b $(PROGRAMMER_SPEED) write_flash
export FFLAGS += -fm $(FLASH_MODE)
export FFLAGS += 0 $(ELFFILE)-0x00000.bin
export FFLAGS += 0x10000 $(ELFFILE)-0x10000.bin
export FFLAGS += 0x10000 $(ELFFILE)-0x10000.bin; esptool.py -p $(PORT) run
Copy link
Member

Choose a reason for hiding this comment

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

this looks a bit weird, can you explain. To me this looks like the latter is a necessary POSTFLASH command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, it is the comment to reset after flashing. I couldn't figure out any other way to define something like that. RESET variable is not doing that since it requires to call make reset explicitly.

Copy link
Member

Choose a reason for hiding this comment

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

okay, then leave as is for now. But we should keep this in mind and clean it up later on - meaning: this should be handled properly by the build system.

@smlng
Copy link
Member

smlng commented Jan 21, 2019

tested with board esp8266-esp-12x and examples/default (and others), all still working and also the esp_wifi is working now.

@smlng smlng added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Jan 21, 2019
@smlng
Copy link
Member

smlng commented Jan 21, 2019

please squash

ETS tasks are now handled by a high priority RIOT thread
Changes of ETS task handling require the context switch by software interrupt. The context switch based on interrupt is therefore enabled by default. Furthermore, the number of priority levels are increased due to the new additional thread.
With the new ETS task handling thread, the stack sizes could be down sized.
During flash write access, the IROM cache cannot be used and is disabled therefore. During that time, ets_post crashes if a functions is called which is not in IRAM. Therefore thread_flags_set must not be called if IROM cache is disabled.
@gschorcht gschorcht force-pushed the esp8266_ets_handling branch from ee4aad9 to e4b0ace Compare January 21, 2019 15:31
@gschorcht
Copy link
Contributor Author

@smlng Many thanks for reviewing and testing.

Rebased and squashed.

@smlng smlng added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 21, 2019
@smlng smlng merged commit e22e582 into RIOT-OS:master Jan 21, 2019
@gschorcht gschorcht deleted the esp8266_ets_handling branch January 24, 2019 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ESP Platform: This PR/issue effects ESP-based platforms Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants