-
Notifications
You must be signed in to change notification settings - Fork 24
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
Conversation
ca43d59
to
0696521
Compare
There was a problem hiding this 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.
There was a problem hiding this 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.
e272ef6
to
2d5f710
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good!
dcbcc16
to
37c8e66
Compare
@erlingrj just rebased onto the most recent changes in main, would be great if we could get this merged in |
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? |
We should get this merged, but there are some conflicts. Maybe @petervdonovan or @erlingrj could help? |
There was a problem hiding this 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.
@erlingrj looks like this is blocked because of requested changes; could you take a final look at this and unblock it? |
There was a problem hiding this 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.
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):
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: