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/esp*: Access to scheduler internals #14771

Open
maribu opened this issue Aug 17, 2020 · 6 comments
Open

cpu/esp*: Access to scheduler internals #14771

maribu opened this issue Aug 17, 2020 · 6 comments
Assignees
Labels
Platform: ESP Platform: This PR/issue effects ESP-based platforms Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Comments

@maribu
Copy link
Member

maribu commented Aug 17, 2020

Description

The ESP32 and ESP8266 platform specific code is accessing scheduler internal stuff and writing to it. E.g. NORETURN void task_exit(void) is with __XTENSA_CALL0_ABI__ indeed just mapping to NORETURN void sched_task_exit(void), but otherwise is handling the task exit internally.

IMO, we should make sure that platform specific code is not modifying the scheduler internal state. Instead, a well defined API should be used for interactions between platform specific code and the scheduler. This would allow changing the internals of the scheduler without breaking other stuff

Suggestion

Use existing scheduler API, such as sched_task_exit()

Replace code writing to sched_* by calls to the existing scheduler API. E.g. rewrite the task_exit() function to also use sched_task_exit() even when __XTENSA_CALL0_ABI__ is not used. (Btw: Is there indeed a need to support two different ABIs?)

Or would this be infeasible to get the ESP platform working on top of the existing core APIs?

Extend the scheduler API

If indeed the current core APIs are not enough to get the ESP platform spinning, we should IMO just extend the APIs as needed and use that instead.

Versions

Current master

@maribu maribu added Platform: ESP Platform: This PR/issue effects ESP-based platforms Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Aug 17, 2020
@maribu
Copy link
Member Author

maribu commented Aug 17, 2020

vTaskDelete() is also anther function reinventing the sched_task_exit() wheel.

@gschorcht
Copy link
Contributor

gschorcht commented Aug 17, 2020

The code is a bit tricky, but it was the only way to get it working with the existing API for ESPs.

Btw: Is there indeed a need to support two different ABIs?)

Yes, although ESPs are Xtensa cores, the ESP8266 uses the Call0 ABI while ESP32 uses the Window ABI. That's why ESP32 requires special handling on task_exit.

@maribu
Copy link
Member Author

maribu commented Aug 17, 2020

The first n lines of the ESP32 task_exit() and sched_task_exit() do exactly the same: Updating the scheduler internals. Afterwards, sched_task_exit() calls into the cpu code, namely cpu_switch_context_exit(), while task_exit() does other stuff.

Why wouldn't it be possible to just use sched_task_exit() and move the other stuff from task_exit() to cpu_switch_context_exit()?

@gschorcht
Copy link
Contributor

gschorcht commented Aug 17, 2020

It might work. To be honest, I don't remember exactly why I did it in that way. I only remember that applications that return from the main thread did crash.

@stale
Copy link

stale bot commented Mar 19, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added State: stale State: The issue / PR has no activity for >185 days and removed State: stale State: The issue / PR has no activity for >185 days labels Mar 19, 2021
@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 22, 2021
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
@stale
Copy link

stale bot commented Mar 2, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Mar 2, 2022
@gschorcht gschorcht removed the State: stale State: The issue / PR has no activity for >185 days label Mar 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Platform: ESP Platform: This PR/issue effects ESP-based platforms Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Development

No branches or pull requests

4 participants