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

Rp2040 multithreaded target support #344

Merged
merged 13 commits into from
May 27, 2024
Merged

Rp2040 multithreaded target support #344

merged 13 commits into from
May 27, 2024

Conversation

sberkun
Copy link
Collaborator

@sberkun sberkun commented Feb 1, 2024

This PR adds multithreaded support from the RP2040 platform. Builds off of the work in #291.

Corresponding PR in lingua-franca: lf-lang/lingua-franca#2178.

Some important notes on this implementation (main differences from Abhi's implementation):

  • semaphores are used as an analog for condition variables. This works because the condition variable only needs to notify at most one other thread. Will trigger a spurious wakeup later if the other thread is not waiting on the condition variable.
  • only one other thread (in addition to the main thread) is allowed. This will be a tricky constraint for the runtime to work with (in particular, scheduling enclaves and watchdog timers will need some work), but will allow for the runtime to work with no context switches.

Edit: this PR is meant as part 1 of a series. This PR is meant to be minimal changes to get the threaded runtime working. The next few PRs will be:

  • modify the runtime so that the main thread also runs a worker
  • merge atomics implementation with Zephyr
  • lf_initialize_clock return value

@sberkun sberkun changed the title initial draft for multithreaded rp2040 Rp2040 multithreaded target support Feb 1, 2024
core/CMakeLists.txt Outdated Show resolved Hide resolved
@sberkun sberkun marked this pull request as ready for review February 1, 2024 09:57
@lhstrh lhstrh requested review from gundralaa and erlingrj February 1, 2024 18:22
Copy link
Collaborator

@gundralaa gundralaa left a comment

Choose a reason for hiding this comment

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

Pretty cool someone’s picking this up :) The only major comment I have is enabling more than 1 worker thread. With only one, it defeats the purpose of having a threaded implementation since the main reactor-c thread does not execute reactions.

core/platform/lf_rp2040_support.c Outdated Show resolved Hide resolved
core/platform/lf_rp2040_support.c Outdated Show resolved Hide resolved
core/platform/lf_rp2040_support.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@erlingrj erlingrj left a comment

Choose a reason for hiding this comment

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

This looks good. Just some nitpicks about checking the return values from the Pico SDK. Also look into whether we need a recursive mutex. When getting threaded support for FlexPRET we ran into that issue and had to create a FlexPRET lock implementation that supported recursive calls.

core/platform/lf_rp2040_support.c Outdated Show resolved Hide resolved
core/platform/lf_rp2040_support.c Outdated Show resolved Hide resolved
core/platform/lf_rp2040_support.c Outdated Show resolved Hide resolved
core/platform/lf_rp2040_support.c Outdated Show resolved Hide resolved
core/platform/lf_rp2040_support.c Outdated Show resolved Hide resolved
core/platform/lf_rp2040_support.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@gundralaa gundralaa left a comment

Choose a reason for hiding this comment

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

looks good!

@sberkun
Copy link
Collaborator Author

sberkun commented Feb 22, 2024

@erlingrj just rebased onto the most recent changes in main, would be great if we could get this merged in

@lhstrh
Copy link
Member

lhstrh commented Apr 3, 2024

Whoops, @sberkun, it looks like we forgot to merge this, and it has fallen a bit behind. Sorry! Could you once again fix the merge conflicts?

@lhstrh
Copy link
Member

lhstrh commented May 17, 2024

We should get this merged, but there are some conflicts. Maybe @petervdonovan or @erlingrj could help?

@sberkun sberkun force-pushed the rp2040-multicore branch from 9bc9c9a to 82f42e1 Compare May 25, 2024 11:24
@sberkun
Copy link
Collaborator Author

sberkun commented May 25, 2024

@lhstrh @erlingrj Sorry for the delay! This is once again ready for review.

Copy link
Contributor

@edwardalee edwardalee left a comment

Choose a reason for hiding this comment

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

LGTM! One comment included that we should not take as blocking the merge.

low_level_platform/impl/src/lf_rp2040_support.c Outdated Show resolved Hide resolved
@edwardalee edwardalee added enhancement Enhancement of existing feature feature New feature labels May 26, 2024
@sberkun sberkun enabled auto-merge May 26, 2024 08:36
@sberkun
Copy link
Collaborator Author

sberkun commented May 26, 2024

@erlingrj looks like this is blocked because of requested changes; could you take a final look at this and unblock it?

Copy link
Collaborator

@erlingrj erlingrj left a comment

Choose a reason for hiding this comment

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

Looks mostly good. But I have some questions about the cond bar implementation. I think the spurious wake-up problem in pthreads is not something we need to copy.

@sberkun sberkun added this pull request to the merge queue May 27, 2024
Merged via the queue into main with commit d9f86a0 May 27, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement of existing feature feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants