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

Update data structures doxygen code to be consistent and more easily readable #506

Open
tammyleino opened this issue Oct 2, 2023 · 11 comments
Labels

Comments

@tammyleino
Copy link
Collaborator

tammyleino commented Oct 2, 2023

The data structures are not documented consistently, and the way they are currently documented, the member description does not show up in the documentation. This proposal will follow the convention set forth by Zephyr.

Example:

before:

/**
 * struct rpmsg_endpoint - binds a local rpmsg address to its user
 * @name: name of the service supported
 * @rdev: pointer to the rpmsg device
 * @addr: local address of the endpoint
 * @dest_addr: address of the default remote endpoint binded.
 * @cb: user rx callback, return value of this callback is reserved
 *      for future use, for now, only allow RPMSG_SUCCESS as return value.
 * @ns_unbind_cb: end point service unbind callback, called when remote
 *                ept is destroyed.
 * @node: end point node.
 * @priv: private data for the driver's use
 *
 * In essence, an rpmsg endpoint represents a listener on the rpmsg bus, as
 * it binds an rpmsg address with an rx callback handler.
 */
struct rpmsg_endpoint {
	char name[RPMSG_NAME_SIZE];
	struct rpmsg_device *rdev;
	uint32_t addr;
	uint32_t dest_addr;
	rpmsg_ept_cb cb;
	rpmsg_ns_unbind_cb ns_unbind_cb;
	struct metal_list node;
	void *priv;
};

after:

 /**
  * @brief structure that binds a local rpmsg address to its user
  *
  * In essence, an rpmsg endpoint represents a listener on the rpmsg bus, as
  * it binds an rpmsg address with an rx callback handler.
  */
 struct rpmsg_endpoint {
];      /** Name of the service  supported */
 	char name[RPMSG_NAME_SIZE

	/** Pointer to the rpmsg device */
	struct rpmsg_device *rdev;        

 	/** Local address of the endpoint */
	uint32_t addr;                    

 	/** Address of the default remote endpoint binded */
	uint32_t dest_addr;               

        /** User rx callback, return value of this callback is reserved
	 *  for future use, for now, only allow RPMSG_SUCCESS
	 * as return value
	 */
	rpmsg_ept_cb cb;

	/** Endpoint service unbind
	 *   callback, called when remote ept is destroyed
	 */
	rpmsg_ns_unbind_cb ns_unbind_cb;  

	/** Endpoint node */
	struct metal_list node;

	/** Private data for the driver's use */
 	void *priv;                       
 };

Secondly, update Doxyfile to remove line number / file from description of data structures members

@tammyleino
Copy link
Collaborator Author

tammyleino commented Oct 2, 2023

Which data structures do we want to consider public? Here is the current list that gets published in the documentation. Not all members are documented, and I am going to be hard-pressed to flesh it all out before release.

No documentation of members:
Elf32_Ehdr
elf32_info
Elf32_Phdr
Elf32_Rel  
Elf32_Rela  
Elf32_Shdr  
Elf32_Sym  
Elf64_Ehdr  
elf64_info  
Elf64_Phdr  
Elf64_Rel  
Elf64_Rela  
Elf64_Shdr  
Elf64_Sym  
rpmsg_rpc_data  
rpmsg_rpc_syscall  
rpmsg_rpc_syscall_header
vring_used_elem

Fully documented:
fw_rsc_carveout  
fw_rsc_devmem  
fw_rsc_hdr  
fw_rsc_trace  
fw_rsc_vdev  
fw_rsc_vdev_vring  
fw_rsc_vendor  
image_store_ops  
loader_ops  
remoteproc  
remoteproc_mem  
remoteproc_ops  
remoteproc_virtio  
resource_table  
rpmsg_device  
rpmsg_device_ops  
rpmsg_endpoint  
rpmsg_hdr  
rpmsg_ns_msg  
rpmsg_rpc_answer  
rpmsg_rpc_client_services  
rpmsg_rpc_clt  
rpmsg_rpc_services  
rpmsg_rpc_svr   
rpmsg_virtio_config  
rpmsg_virtio_device  
rpmsg_virtio_shm_pool  
virtio_device  
virtio_vring_info  
vring_desc  
vring_avail  
vring_used   

@wmamills @arnopo @edmooring

@tammyleino
Copy link
Collaborator Author

First PR opened for already documented data structures here #507

@arnopo
Copy link
Collaborator

arnopo commented Oct 4, 2023

Which data structures do we want to consider public? Here is the current list that gets published in the documentation. Not all members are documented, and I am going to be hard-pressed to flesh it all out before release.

No documentation of members: Elf32_Ehdr elf32_info Elf32_Phdr Elf32_Rel   Elf32_Rela   Elf32_Shdr   Elf32_Sym   Elf64_Ehdr   elf64_info   Elf64_Phdr   Elf64_Rel   Elf64_Rela   Elf64_Shdr   Elf64_Sym   rpmsg_rpc_data   rpmsg_rpc_request   rpmsg_rpc_syscall   rpmsg_rpc_syscall_header virtio_device_id   virtio_dispatch   virtio_feature_desc   virtqueue   virtqueue_buf   vq_desc_extra   vring   vring_alloc_info   vring_avail   vring_used   vring_used_elem

From my point of view; would be nice to comment

  • vring,   vring_alloc_info,   vring_avail,   vring_used,   vring_used_elem
    =>Descriptions are available in https://docs.oasis-open.org/virtio/virtio/v1.2/csd01/virtio-v1.2-csd01.html

  • virtio_device_id,   virtio_dispatch,   virtio_feature_desc,   virtqueue,   virtqueue_buf,   vq_desc_extra
    => Probably more complex to comment, If you don't know how to comment it, I will try to do it for the release.

For the ELF* structure, no strong need

@tammyleino
Copy link
Collaborator Author

Which data structures do we want to consider public? Here is the current list that gets published in the documentation. Not all members are documented, and I am going to be hard-pressed to flesh it all out before release.
No documentation of members: Elf32_Ehdr elf32_info Elf32_Phdr Elf32_Rel Elf32_Rela Elf32_Shdr Elf32_Sym Elf64_Ehdr elf64_info Elf64_Phdr Elf64_Rel Elf64_Rela Elf64_Shdr Elf64_Sym rpmsg_rpc_data rpmsg_rpc_request rpmsg_rpc_syscall rpmsg_rpc_syscall_header virtio_device_id virtio_dispatch virtio_feature_desc virtqueue virtqueue_buf vq_desc_extra vring vring_alloc_info vring_avail vring_used vring_used_elem

From my point of view; would be nice to comment

* vring,   vring_alloc_info,   vring_avail,   vring_used,   vring_used_elem
  =>Descriptions are available in https://docs.oasis-open.org/virtio/virtio/v1.2/csd01/virtio-v1.2-csd01.html

* virtio_device_id,   virtio_dispatch,   virtio_feature_desc,   virtqueue,   virtqueue_buf,   vq_desc_extra
  => Probably more complex to comment, If you don't know how to comment it, I will try to do it for the release.

For the ELF* structure, no strong need

I was able to find info for vring_avail, vring_used and vring from the reference above, but cannot find the others. If we could please merge my PR and then you could open a new one with additional comments, that would be ideal for me.

Should I mark the others private so they don't show up in the doc?

@arnopo
Copy link
Collaborator

arnopo commented Oct 5, 2023

I was able to find info for vring_avail, vring_used and vring from the reference above, but cannot find the others. If we could please merge my PR and then you could open a new one with additional comments, that would be ideal for me.

That sound good to me

Should I mark the others private so they don't show up in the doc?

I prefer to keep them in the doc even if uncommented.
Could you list in this issue the structures that are not yet commented?

@tammyleino
Copy link
Collaborator Author

@arnopo I updated the list above to show the currently undocumented and documented data structures. We can leave this ticket open until all data structures are documented. It would be nice to at least have a description for each one, even if the members are not documented.

@arnopo
Copy link
Collaborator

arnopo commented Oct 10, 2023

Following structures are commented in #510:

  • virtio_device_id
  • virtio_feature_desc
  • virtio_dispatch
  • virtqueue_buf
  • virtqueue
  • vring_alloc_info

Copy link

This issue has been marked as a stale issue because it has been open (more than) 45 days with no activity.

@github-actions github-actions bot added the Stale label Nov 25, 2023
@github-actions github-actions bot removed the Stale label Apr 3, 2024
@arnopo
Copy link
Collaborator

arnopo commented Jun 14, 2024

Following page give a up to date status on documented/undocumented structures:
https://openamp.readthedocs.io/en/latest/doxygen/openamp/annotated.html

Seems not useful to document ELFxx structures. Perhaps we could remove the elf_loader.h from the API list as it is more dedicated to an internal usage.
@tnmysh @edmooring : any opinion?

Remaining structure to document:

 virtio_ident
 rpmsg_rpc_data  
 rpmsg_rpc_syscall  
 rpmsg_rpc_syscall_header
 vring_used_elem

@tnmysh
Copy link
Collaborator

tnmysh commented Jul 18, 2024

Seems not useful to document ELFxx structures. Perhaps we could remove the elf_loader.h from the API list as it is more dedicated to an internal usage. @tnmysh @edmooring : any opinion?

Correct, elf loading is more internal usage of remoteproc framework. @arnopo , To me it's okay to remove elfxx structures and elf_loader.h.

Copy link

github-actions bot commented Sep 1, 2024

This issue has been marked as a stale issue because it has been open (more than) 45 days with no activity.

@github-actions github-actions bot added the Stale label Sep 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants