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

Support for FlexPRET platform #412

Merged
merged 42 commits into from
May 22, 2024
Merged

Conversation

magnmaeh
Copy link
Member

@magnmaeh magnmaeh commented Apr 21, 2024

This PR adds support for the FlexPRET platform.

@magnmaeh
Copy link
Member Author

magnmaeh commented May 3, 2024

I believe this PR now is ready for review :)

@magnmaeh magnmaeh marked this pull request as ready for review May 3, 2024 13:30
@magnmaeh
Copy link
Member Author

magnmaeh commented May 3, 2024

One change I made that probably affects other platforms too:

I added #include "low_level_platform.h" in platform.h and target_link_libraries(lf-platform-api INTERFACE lf-low-level-platform-api) to platform's CMakeLists.txt. This ensures low_level_platform.h is always included when platform.h is included, which I think makes a lot of sense.

EDIT: This caused Zephyr CI tests to fail, so removed this.

@lhstrh lhstrh added the feature New feature label May 4, 2024
@lhstrh lhstrh changed the title Add support for FlexPRET Support for FlexPRET platform May 4, 2024
@lhstrh lhstrh requested a review from lsk567 May 4, 2024 06:45
Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

This generally looks very good. I left some comments/questions.

core/threaded/reactor_threaded.c Show resolved Hide resolved
core/utils/util.c Outdated Show resolved Hide resolved
low_level_platform/api/platform/lf_flexpret_support.h Outdated Show resolved Hide resolved
low_level_platform/impl/CMakeLists.txt Outdated Show resolved Hide resolved
low_level_platform/impl/CMakeLists.txt Show resolved Hide resolved
low_level_platform/impl/src/lf_flexpret_support.c Outdated Show resolved Hide resolved
low_level_platform/impl/src/lf_flexpret_support.c Outdated Show resolved Hide resolved
@lhstrh lhstrh requested a review from erlingrj May 6, 2024 16:40
@magnmaeh
Copy link
Member Author

magnmaeh commented May 6, 2024

Thank you for the review! I'm on vacation until Friday, so there will be much less work on my end until then, just so you know. Might do a little work here and there :)

@magnmaeh magnmaeh requested a review from erlingrj May 14, 2024 15:44
@lhstrh lhstrh requested a review from edwardalee May 17, 2024 20:50
@magnmaeh
Copy link
Member Author

Hmm, no jobs were run... Do you know what the issue might be @lhstrh?

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 dont like that platform-specific details are creeping out of low-level-platform

core/utils/util.c Outdated Show resolved Hide resolved
include/core/tracepoint.h Outdated Show resolved Hide resolved
core/threaded/reactor_threaded.c Show resolved Hide resolved
@magnmaeh
Copy link
Member Author

magnmaeh commented May 18, 2024

@erlingrj I fixed all concerns now. I was able to fix the underlying issue that made me add the platform-specific code in util.c and tracepoint.h in the first place (#412 (comment)). Only thing is I don't exactly understand why just the first test runs...

EDIT: Right, I was referencing a file that does not exist in the main LF repo yet

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 to me. If you want to run CI with a specific branch of lingua-franca, then write the branch name into lingua-franca-ref.txt. Before we merge we modify back to master

@magnmaeh
Copy link
Member Author

magnmaeh commented May 18, 2024

This looks good to me. If you want to run CI with a specific branch of lingua-franca, then write the branch name into lingua-franca-ref.txt. Before we merge we modify back to master

Hmm, the ref thing does not seem to work since my branch is on a fork as well... I can change to URL (lf-lang/lingua-franca/.github/workflows/c-flexpret-tests.yml@master) to point to my fork as well though, i.e., magnmaeh/lingua-franca/.github/workflows/c-flexpret-tests.yml@add-flexpret-support, which is valid. But that file depends on lots of other files again and fails.

Maybe its better to just try and resolve any CI issues in the main repo PR instead?

@erlingrj
Copy link
Collaborator

Yes, when CI passes in the PR of lingua-franca then we merge both!

@lhstrh
Copy link
Member

lhstrh commented May 21, 2024

Let me push out one more patch release before merging this.

@lhstrh lhstrh added this pull request to the merge queue May 21, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 21, 2024
@lhstrh lhstrh added this pull request to the merge queue May 22, 2024
@erlingrj erlingrj removed this pull request from the merge queue due to a manual request May 22, 2024
@erlingrj erlingrj enabled auto-merge May 22, 2024 07:39
@erlingrj erlingrj disabled auto-merge May 22, 2024 07:39
@erlingrj erlingrj enabled auto-merge May 22, 2024 07:40
@lhstrh lhstrh disabled auto-merge May 22, 2024 15:15
@lhstrh lhstrh merged commit c7583dc into lf-lang:main May 22, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants