Skip to content

Add breathe and doxylink for doxygen integration#45

Merged
arnopo merged 11 commits intoOpenAMP:main-nextfrom
sipke:feature/add-breathe-and-doxylink
Jul 1, 2025
Merged

Add breathe and doxylink for doxygen integration#45
arnopo merged 11 commits intoOpenAMP:main-nextfrom
sipke:feature/add-breathe-and-doxylink

Conversation

@sipke
Copy link
Copy Markdown

@sipke sipke commented Mar 24, 2025

In order to be able to embed and link to doxygen content generated by submodules libmetal and openamp, add the sphinx extensions breathe and doxylink.

Adding these, though not trivial, will enable better consolidation of documentation between doxygen of submodule and openamp-docs docs.

Requires pull request changes in libmetal and openamp for the embedding/linking to actually work, otherwise it will give error in build output, though will not block rest of document build.
OpenAMP/libmetal#329
OpenAMP/open-amp#641

openamp-docs git submodule for open-amp and libmetal have been bumped to make the change effective for these repository document generation.

@arnopo
Copy link
Copy Markdown
Collaborator

arnopo commented Mar 25, 2025

@sipke
could you create a temporary commit that update the openamp submode to point to your open-amp and libmetal github? That would allow to see the generated doc.

@sipke
Copy link
Copy Markdown
Author

sipke commented Mar 26, 2025

Added the requested temporary commit as well as content changes to demonstrate how breathe and doxylink work.

Not sure why the readthedocs build did not succeed on checkout, the commit should be there.

Alternate locations of examples:
See https://sipke-openamp-docs.readthedocs.io/en/sv-develop/docs/data_structures.html
and
https://sipke-openamp-docs.readthedocs.io/en/sv-develop/docs/porting_guide.html
which has examples of each.

@sipke sipke force-pushed the feature/add-breathe-and-doxylink branch from 0533302 to 293a36e Compare April 28, 2025 05:47
Copy link
Copy Markdown
Collaborator

@edmooring edmooring left a comment

Choose a reason for hiding this comment

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

Looks OK to me.,

@sipke sipke force-pushed the feature/add-breathe-and-doxylink branch from 293a36e to 99be20f Compare May 19, 2025 04:46
@sipke
Copy link
Copy Markdown
Author

sipke commented May 19, 2025

Updated this pull request with new temporary submodule commits for open-amp and libmetal as well as updated remoteproc_design and rpmsg_design with embedded doxygen using breathe extension. I think this should cover the review request items

  • point to API page generated by doxygen, instead of duplicating the API

for issue 38.
If the libmetal and open-amp pull requests are merged and the submodules in this openamp-docs repository updated, I can rebase this to allow for merging.
OpenAMP/libmetal#329
OpenAMP/open-amp#641

@sipke sipke force-pushed the feature/add-breathe-and-doxylink branch from 99be20f to f1cd0f7 Compare May 26, 2025 03:36
@sipke
Copy link
Copy Markdown
Author

sipke commented May 26, 2025

Bumped open-amp and libmetal git modules to those with xml and tag file generated by doxygen so this could now be merged as they are not longer temporary commits.
Just noticed that main already auto-updates the gitmodules, so probably main-next needs to be force updated first, after which this branch probably needs rebasing...

@arnopo
Copy link
Copy Markdown
Collaborator

arnopo commented May 26, 2025

you should be able to rebase it now, I mergered main branch in main-next to align the documentation

@sipke sipke force-pushed the feature/add-breathe-and-doxylink branch from f1cd0f7 to 06263c7 Compare June 9, 2025 06:04
.. doxygenstruct:: vring
:members:

Refer also :openamp_doc_link:`vring <vring>` documentation and `vring code implementation <https://github.com/OpenAMP/open-amp/blob/main/lib/include/openamp/virtio_ring.h>`_.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Author

@sipke sipke Jun 10, 2025

Choose a reason for hiding this comment

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

That looks to be in doxygen generated content (included via breathe), so might be in code comments. Will have to check.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

to address in a separate PR, I created the issue #63

Comment thread docs/porting_guide.rst Outdated
Platform Specific Remoteproc Driver
***********************************

<<<<<<< HEAD
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

merge conflict need to be fixed in this commit

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I did some rebase mistake there I'll need to check. Had fixed it so must have pushed wrong branch or similar.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Secondary commit had fixed the forgotten merge conflict, but first hadn't. Squashed the commits into one, as both were adding breathe/doxylink, as b72a3b4

Comment thread docs/porting_guide.rst Outdated
Platform Specific Porting to Use Remoteproc to Manage Remote Processor
**********************************************************************

<<<<<<< HEAD
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

same

@@ -0,0 +1,136 @@
Remoteproc Design Document
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

your proposal is not crystal clear to me. Here, you duplicate the documentation. Do you propose to remove the file in open-amp repo?
What about changing the format from markdown to RST directly in open-amp repo?

Copy link
Copy Markdown
Author

@sipke sipke Jun 10, 2025

Choose a reason for hiding this comment

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

I had read mention of you possibly wanting to move documentation to openamp-docs and only doxygen in code repos, or something like that, so I did this to show how breathe and doxylink looks without needing to change code repos. It's up to you which you prefer to do, then I can change code repos from md to rst or delete md file in code repos and move images from code repos to openamp-docs too.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I just tested changing md to rst in open-amp repo, and adjusting references in openamp-docs repo, and that works the same, so if you can let me know which of the two locations you want by next week, I can adjust and then create PRs in open-amp repo to either

  • change md to rst or
  • delete docs and images

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@arnopo / @wmamills if you could let me know if you prefer these design docs to move to openamp-docs repository, or stay in open-amp.
If they stay in open-amp repo, then I suspect they would not generate in that repository cleanly, as the changes require breathe and doxylink, until a similar readthedocs build flow would be created in open-amp repo as is in openamp-docs.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

meeting done to discuss the design base on breathe:
decision:

  • integrate this PR and move forward with breathe
  • move md and image documentation from openamp repository to openamp-doc repository and convert file in RST.

@sipke sipke force-pushed the feature/add-breathe-and-doxylink branch from 06263c7 to 164621f Compare June 11, 2025 00:28
Copy link
Copy Markdown
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.

this PR need also to be rebase to solve conflicts

Comment thread openamp/contributing.rst Outdated

If you want to contribute and port OpenAMP to your platform read more about OpenAMP porting
:ref:`here<porting-guide-work-label>`.
You are encouraged to contribute to the OpenAMP project to help create a successful solution through a vibrant and engaged community.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please apply the rule done on .rst files on the main branch: 100 chars max per line as much as possible

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done

Comment thread docs/porting_guide.rst Outdated
{
/* Instantiate the remoteproc instance */
remoteproc_init(&rproc, &rproc_ops, &private_data);
/* Instantiate the remoteproc instance */
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

tabulation is part of the formating coding ruleof the libraries , I would prefer to keep them except if that generates issues

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

put back, but still needs two spaces for the formatting at start. using a tab (instead of two spaces at start) does not format as well.

Sipke Vriend added 11 commits June 23, 2025 09:41
Add breathe extension to requirements.txt to allow embedding of doxygen
content through rst rendered files.
Add openamp and libmetal breathe projects to conf.py to integrate breathe
to both library generated doxygen content.
refer
 https://breathe.readthedocs.io/en/latest/index.html
This commit relies on having doc/Doxyfile.in modified in openamp and
libmetal repositories to include
GENERATE_XML           = YES
Add sphinxcontrib.doxylink extension to requirements.txt to allow linking,
in rst files, to generated doxygen files of submodule repositories.
refer
 https://github.com/sphinx-contrib/doxylink
 https://sphinxcontrib-doxylink.readthedocs.io

This commit relies on having doc/Doxyfile.in modified in openamp and
libmetal repositories to include
GENERATE_TAGFILE       = <filename as in doxylink dictionary of conf.py>
adjust the Porting guide for Remoteproc to use breathe's doxygenfunction
and doxygenstruct to embed doxygen code documentation of the
remoteproc_ops structure and functions using it. Also add doxylink link to
the same struct in case anyone wants to look at all the doxygen
documentation.
add highlighted code-block for virtio example.
code-block requires two spaces for code post tag.
add links for virtio and remote proc driver
…rences

the data_structures file is a markdown (md) file in open-amp repository
which uses code within the file to demonstrate datastructures.
Review comments were to use doxygen linking, so use breathe and doxylink
in a new file within readthedocs to achieve the same reference but
through linking which will maintain itself as long as doxygen comments
are maintained in open-amp repo.
point and new data structures and move it to under design docs and move
the porting guide as its own section rather than under library users
guide.
…erences

the remoteproc-design file is a markdown (md) file in open-amp repository
which uses code within the file to demonstrate APIs.
Review comments were to use doxygen linking, so use breathe and doxylink
in a new file within readthedocs to achieve the same references but
through embedding which will maintain itself as long as doxygen comments
are maintained in open-amp repo.
the rpmsg-design file is a markdown (md) file in open-amp repository
which uses code within the file to demonstrate APIs.
Review comments were to use doxygen linking, so use breathe and doxylink
in a new file within readthedocs to achieve the same references but
through embedding which will maintain itself as long as doxygen comments
are maintained in open-amp repo.
remove sentence as design documents have moved to readthedocs to allow
embedding of doxygen content via breathe extension to readthedocs.
add line limit of 100 characters per review feedback
@sipke sipke force-pushed the feature/add-breathe-and-doxylink branch from 164621f to 7546e7c Compare June 23, 2025 00:13
@@ -0,0 +1,136 @@
Remoteproc Design Document
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

meeting done to discuss the design base on breathe:
decision:

  • integrate this PR and move forward with breathe
  • move md and image documentation from openamp repository to openamp-doc repository and convert file in RST.

.. doxygenstruct:: vring
:members:

Refer also :openamp_doc_link:`vring <vring>` documentation and `vring code implementation <https://github.com/OpenAMP/open-amp/blob/main/lib/include/openamp/virtio_ring.h>`_.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

to address in a separate PR, I created the issue #63

@arnopo arnopo merged commit a2409d0 into OpenAMP:main-next Jul 1, 2025
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