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

Doxygen updates for data structures #507

Merged
merged 1 commit into from
Oct 6, 2023
Merged

Conversation

tammyleino
Copy link
Collaborator

Improved doxygen formatting and consistency for data structures.
Signed-off-by: Tammy Leino [email protected]

@tammyleino
Copy link
Collaborator Author

tammyleino commented Oct 3, 2023

This is a first commit, which updates formatting of the already fully documented data structures. There are still 30 data structures that need to be updated that are not currently documented but show up in the documentation as public data structures as documented here #506. Hopefully we can split that work up among multiple people once a consistent format is established.

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 minor comments, else LGTM

lib/include/openamp/remoteproc.h Outdated Show resolved Hide resolved
lib/include/openamp/rpmsg.h Outdated Show resolved Hide resolved
* @size: size of the memory
* @io: pointer to the I/O region
* @node: list node
* @brief Memory used by the remote processor
Copy link
Collaborator

Choose a reason for hiding this comment

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

/** @brief Memory used by the remote processor */
Somes other occurrences in you PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you think we should elaborate more on this description? I don't really like things to have just a brief - it would be nice to have additional information.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes would be nice, just need to find the good threshold to not over comment.
I would say it depends on whether it is easy or not to understand the structure without extra description.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@arnopo I'm just going to leave things as-is for this PR. We can decide if things need more detail in a future revision.

lib/include/openamp/virtio.h Outdated Show resolved Hide resolved
lib/include/openamp/virtio_ring.h Outdated Show resolved Hide resolved
lib/rpmsg/rpmsg_internal.h Outdated Show resolved Hide resolved
@arnopo arnopo requested review from edmooring, arnopo and tnmysh October 4, 2023 08:28
@arnopo arnopo added this to the Release V2023.10 milestone Oct 4, 2023
@tammyleino tammyleino force-pushed the issue506 branch 2 times, most recently from 1bbf2a7 to fd79127 Compare October 4, 2023 16:52
@tammyleino
Copy link
Collaborator Author

@arnopo I have addressed your outstanding comments with the latest version of the PR.

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.

minors new comments


/**
* RPMsg buffer reclaimer that contains buffers released by the
* rpmsg_virtio_release_tx_buffer function
Copy link
Collaborator

Choose a reason for hiding this comment

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

is the link to rpmsg_virtio_release_tx_buffer is generated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, but I have updated it so it will. Thanks!

* @size: size of the memory
* @io: pointer to the I/O region
* @node: list node
* @brief Memory used by the remote processor
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes would be nice, just need to find the good threshold to not over comment.
I would say it depends on whether it is easy or not to understand the structure without extra description.

lib/include/openamp/virtio.h Outdated Show resolved Hide resolved
char name[RPMSG_NAME_SIZE];

/** Address of the remote service that is being published */
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can replace "address" with "endpoint number"

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer endpoint address as it represent the address of the endpoint for RP message delivery.

Copy link
Collaborator

@tnmysh tnmysh left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Improved doxygen formatting and consistency for data structures.
Signed-off-by: Tammy Leino <[email protected]>
@arnopo arnopo merged commit 937ba1d into OpenAMP:main Oct 6, 2023
2 checks 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