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

Which demos for system reference should be included in the docs? #5

Closed
tammyleino opened this issue Dec 8, 2022 · 17 comments
Closed

Comments

@tammyleino
Copy link
Collaborator

For the first draft of the docs, I created a system_reference.rst which includes all the demos documented on this wiki page: https://github.com/OpenAMP/openamp-system-reference/wiki/Samples-and-demos.

Upon closer inspection, I see that there are demos in the openamp-system-reference repo with README.md files and they don't overlap completely with the demos on the wiki.

Can someone please look through the demos documented in sphinx and in openamp-system-reference and decide which should be included in the documentation and which are obsolete?

@tnmysh
Copy link
Collaborator

tnmysh commented Dec 9, 2022

@tammyleino we should atleast add above mentioned 3 README.md references in openamp-docs under AMD-xilinx demo page. I can do it however, I don't know how to add openamp-system-reference project in openamp-docs repo.

To remove any current demo from the documentation might take some time as it needs discussions.

@tammyleino
Copy link
Collaborator Author

@TanmayShah-xilinx I had started adding those demos, but I think the titles in the README files need updated because they don't make sense in the context of the docs; ie, app: echo_test. What do you think?

You can add a link to a repo:

git submodule add
git submodule init
git submodule update

Then commit changes and the link will be there in the repo.

@tnmysh
Copy link
Collaborator

tnmysh commented Dec 9, 2022

Yes I think "app:" is redundant from each README.md. I will remove it. Conventionally each demo in the AMD-Xilinx documentation is known by functionality that demo performs and host-binary name for that demo. So, "echo_test" is binary available in sdk on the Host processor. I am open to suggestions for any new title. I kept it as "echo_test" just to match with conventional documentation of AMD-Xilinx (UG1186). Also previous documentation has same name for this demo -> https://openamp.readthedocs.io/en/latest/open-amp/docs/apps/echo_test/README.html

@tnmysh
Copy link
Collaborator

tnmysh commented Dec 9, 2022

@tammyleino

Following is something I am planning for AMD-Xilinx demo file:

Demo: RpMsg in kernelspace
Demo1A: echo_test
Demo1B: matrix multiplication
Demo1C: proxy_app

If it looks good, I will change README.md in openamp-system-reference accordingly.

@tammyleino
Copy link
Collaborator Author

@TanmayShah-xilinx I think it would be ok to leave the demos named as-is if you add them all under a common heading so it is understood what platform they are for.

So as a suggestion you could add a new .rst file in the demo directory (maybe titled Xilinx ZynqMP Cortex-R5 Demos ??), then that .rst would just reference each readme in the system reference repo.

@tammyleino
Copy link
Collaborator Author

@TanmayShah-xilinx There's a precedent for "System Reference Samples and Demos on XYZ Platform" - maybe follow that?

@tnmysh
Copy link
Collaborator

tnmysh commented Dec 9, 2022

Yes correct. So, we are creating this file: https://github.com/OpenAMP/openamp-docs/blob/main/demos/system_reference-AMD-Xilinx.rst
All the Demo on Xilinx Platforms will be referenced in that file.

@tammyleino
Copy link
Collaborator Author

@TanmayShah-xilinx Sounds good to me.

@wmamills
Copy link
Collaborator

I don't know what you mean by saying "All the Demo on Xilinx Platforms will be referenced in that file."
We should pull in the content of those files into pages not just have a link to them.
I was expecting that they would replace the existing echo_test, etc pages.

Yes, I think we should hide the Microblaze page if it has not been tested recently.

What about the Linux to Linux proxy test? Keep or hide for now?

@tnmysh
Copy link
Collaborator

tnmysh commented Dec 12, 2022

I don't know what you mean by saying "All the Demo on Xilinx Platforms will be referenced in that file." We should pull in the content of those files into pages not just have a link to them. I was expecting that they would replace the existing echo_test, etc pages.

Yes we will pull in the content of those files into pages.

Yes, I think we should hide the Microblaze page if it has not been tested recently.

Ok, Ack.

What about the Linux to Linux proxy test? Keep or hide for now?

Same here. I don't think linux-to-linux proxy test is needed as well. we can hide it for now.

@tnmysh
Copy link
Collaborator

tnmysh commented Dec 12, 2022

@wmamills , @tammyleino

Following two PR are to address this issue:

  1. examples: Modify README formatting openamp-system-reference#9

  2. Amd xilinx demo documentation #13

First one modifies current README.md format in openamp-system-reference.
second PR adds openamp-system-reference repo link to openamp-docs and pulls-in the content of README.md files to openamp-docs repo.

I tested second PR with "make html pdf" command and formatting looks good to me.

Thanks,
Tanmay

@arnopo
Copy link
Collaborator

arnopo commented Dec 12, 2022

What about the Linux to Linux proxy test? Keep or hide for now?

Same here. I don't think linux-to-linux proxy test is needed as well. we can hide it for now.

This test is part of my non reg on release. It works. We can keep it as it demonstrates a generic RPC interface using openAMP RPC service. The proxy service is dedicated to redirect syscall API only.

@tnmysh
Copy link
Collaborator

tnmysh commented Dec 12, 2022

What about the Linux to Linux proxy test? Keep or hide for now?

Same here. I don't think linux-to-linux proxy test is needed as well. we can hide it for now.

This test is part of my non reg on release. It works. We can keep it as it demonstrates a generic RPC interface using openAMP RPC service. The proxy service is dedicated to redirect syscall API only.

Thanks Arnaud. I will keep it.

@tammyleino
Copy link
Collaborator Author

Is this issue resolved?

@tnmysh
Copy link
Collaborator

tnmysh commented Oct 2, 2023

Yes I think so. Following two PR are merged:

OpenAMP/openamp-system-reference#9
#13

@wmamills
Copy link
Collaborator

As agreed Oct 18 2023 this was resolved.
Anything new should be covered in a new issue

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

No branches or pull requests

4 participants