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

Move openamp demos #44

Merged
merged 4 commits into from
Aug 28, 2024
Merged

Move openamp demos #44

merged 4 commits into from
Aug 28, 2024

Conversation

tnmysh
Copy link
Collaborator

@tnmysh tnmysh commented Jun 19, 2024

Move apps directory from open-amp library repo to openamp-system-reference repo and modify cmake build infra structure as needed to build these apps.

Copy link
Collaborator

@arnopo arnopo left a comment

Choose a reason for hiding this comment

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

I think this is a step in the right direction to move forward.
Just a request: please split the commit to make it simpler to compare with the original code.

i will try to rebuild and run application to test it

examples/legacy_apps/CMakeLists.txt Show resolved Hide resolved
examples/legacy_apps/cmake/collect.cmake Show resolved Hide resolved
Copy link
Collaborator

@arnopo arnopo left a comment

Choose a reason for hiding this comment

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

I tested on my Linux PC using west and cmake. Few comments and suggestions

examples/legacy_apps/README.md Outdated Show resolved Hide resolved
examples/legacy_apps/README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@tnmysh tnmysh force-pushed the move_openamp_demos branch from e042794 to dcc9284 Compare July 23, 2024 22:08
Copy link
Collaborator

@arnopo arnopo left a comment

Choose a reason for hiding this comment

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

Few new comments.
After addressing them LGTM

examples/legacy_apps/README.md Outdated Show resolved Hide resolved
examples/legacy_apps/README.md Outdated Show resolved Hide resolved
examples/legacy_apps/README.md Show resolved Hide resolved
examples/legacy_apps/README.md Outdated Show resolved Hide resolved
@tnmysh tnmysh force-pushed the move_openamp_demos branch from dcc9284 to ca00345 Compare July 31, 2024 21:48
@tnmysh tnmysh requested a review from arnopo July 31, 2024 21:58
examples/legacy_apps/README.md Show resolved Hide resolved
@tnmysh tnmysh marked this pull request as ready for review August 1, 2024 16:13
Copy link
Contributor

@iuliana-prodan iuliana-prodan left a comment

Choose a reason for hiding this comment

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

This PR looks good but why do we call the folder legacy_apps?
When I read legacy app, I'm thinking of something old that might not even work - but maybe is just me :)
Can we call the folder generic_samples, or just generic (since it's already in examples).

Also, not very clear for me how these samples work?
Do we have a Linux-Zephyr communication?

Can we add, for each sample a README - like we have now here, for each app.
I see the comment from the start of each sample (like here) but IMO is not enough.

examples/legacy_apps/examples/CMakeLists.txt Outdated Show resolved Hide resolved
#define SHUTDOWN_MSG 0xEF56A55A

#define LPRINTF(format, ...) printf(format, ##__VA_ARGS__)
//#define LPRINTF(format, ...)
Copy link
Contributor

Choose a reason for hiding this comment

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

remove commented code

examples/legacy_apps/cmake/modules/Findopen_amp.cmake Outdated Show resolved Hide resolved
@tnmysh
Copy link
Collaborator Author

tnmysh commented Aug 15, 2024

This PR looks good but why do we call the folder legacy_apps? When I read legacy app, I'm thinking of something old that might not even work - but maybe is just me :) Can we call the folder generic_samples, or just generic (since it's already in examples).

Some background on this:
So, These apps used to be in open-amp repository here: https://github.com/OpenAMP/open-amp/tree/main/apps
At that time openamp-system-reference repo did not exist. Now, we have separate repo available only for demos, we are moving demos from library repository.

Ideally, we want a framework that is easily portable to any platform and any OS. This framework isn't available yet. Many demos under "legacy_apps" are hardcoded to run only on AMD-Xilinx platforms. Eventually they will be deprecated. Hence the name "legacy_apps" makes sense.

We don't want to call it "generic" for the same reason, as not all demos are easily portable. Instead, we can introduce new demos under ${REPO}/examples/{OS}/ directory that is portable on all the platforms.

Also, not very clear for me how these samples work? Do we have a Linux-Zephyr communication?

Some demos are using AMD-Xilinx BSP, and so they provide Linux-Baremetal, and Linux-FreeRTOS communication. Some demos provides Linux-Linux communication.
Linux-Linux RPMSG communicaiton is tested between two linux processes.

Can we add, for each sample a README - like we have now here, for each app. I see the comment from the start of each sample (like here) but IMO is not enough.

Yes, we should add it, but it can be part of new PR and as enhancement. This PR only moves demos as it is, and makes sure they can be compiled. So, modifying cmake build infra as needed.

Apps directory was tightly coupled with open_amp
library source code. Instead host demo applications
in openamp-system-reference repository. This patch
copies over source code of apps as it is so build
with only this commit is expected to fail.

Next commit will introduce changes required to build
demos and link open_amp library.
@tnmysh tnmysh force-pushed the move_openamp_demos branch from ca00345 to 3685755 Compare August 15, 2024 20:19
tnmysh added 3 commits August 15, 2024 13:26
- Remove version control information
- Add cmake infra to find open_amp library
- fix open_amp and libmetal library linking

Signed-off-by: Tanmay Shah <[email protected]>
Use west to fetch open-amp and libmetal library source code.

Signed-off-by: Tanmay Shah <[email protected]>
Add instructions to fetch libraries and build legacy apps and tests.

Signed-off-by: Tanmay Shah <[email protected]>
@tnmysh tnmysh force-pushed the move_openamp_demos branch from 3685755 to fe571b8 Compare August 15, 2024 20:26
@tnmysh
Copy link
Collaborator Author

tnmysh commented Aug 15, 2024

@iuliana-prodan Thanks for reviews. Fixed comments.

@arnopo arnopo merged commit d57bae7 into OpenAMP:main Aug 28, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants