Skip to content

Conversation

@rruuaanng
Copy link
Contributor

Replaced incorrect ':c:func:' tag for devicetree macro with ':c:macro' tag.

Replaced incorrect ':c:func:' tag for devicetree macro
with ':c:macro' tag.

Signed-off-by: James Roy <[email protected]>
@rruuaanng rruuaanng added area: Documentation DNM This PR should not be merged (Do Not Merge) area: Devicetree labels Nov 6, 2024
@rruuaanng rruuaanng marked this pull request as ready for review November 6, 2024 12:59
@rruuaanng
Copy link
Contributor Author

rruuaanng commented Nov 6, 2024

The PR should wait until #80511 is merged before merging.

Edit
I modified the PR on the other side and now this PR is merged at will :)

@rruuaanng rruuaanng removed the DNM This PR should not be merged (Do Not Merge) label Nov 6, 2024
Copy link
Member

@decsny decsny left a comment

Choose a reason for hiding this comment

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

not a documentation expert but this might be intentional https://devopstutodoc.readthedocs.io/en/0.3.0/documentation/doc_generators/sphinx/rest_sphinx/domain/c_domain.html#c-function

the sphinx documentation seems to suggest to use the :c:func for function-like preprocessor macros? and looking at the docs right now, it seems fine, and hovering over the macros in this text with the mouse shows a nice popup of how to use it like a function, do you know what does changing it to :c:macro do (maybe give a screenshot if you built the docs)?

right now it looks like this:
image

and the doxygen in devicetree.h is written like you would for functions

@decsny decsny requested review from gmarull and kartben November 7, 2024 00:44
@rruuaanng
Copy link
Contributor Author

rruuaanng commented Nov 7, 2024

Hmm, it looks like this.

Edit
However, I need to give the document picture of cpython.

image

This macro is used to initialize the object header, but it is actually a macro and similar to the macro of the devicetree.

And, the macro argument of the devicetree are not displayed in the document now. They are just blank brackets. This may need to be improved.

@kartben
Copy link
Contributor

kartben commented Nov 7, 2024

not a documentation expert but this might be intentional devopstutodoc.readthedocs.io/en/0.3.0/documentation/doc_generators/sphinx/rest_sphinx/domain/c_domain.html#c-function

the sphinx documentation seems to suggest to use the :c:func for function-like preprocessor macros?

Ya, either way would be fine. I don't think it's necessarily worth changing, unless you can clarify the problem you're trying to solve, @rruuaanng?
It looks like maybe you would like the documentation to look like: "Use DT_PATH(label) along..." so that it's clearer what arguments each macro accepts?

@rruuaanng
Copy link
Contributor Author

rruuaanng commented Nov 7, 2024

Yes, that's what I meant. Changing them to macro tags indicates that they are macros in nature.

Edit
In fact, the documentation does not specify the parameters, even when using the func tag.

Reply again
图片

I'm not sure what browser Declan is using that pops up the link, maybe it's better not to change it. I use the original browser to browse the document that uses the macro tag, as shown above.

(My build server is under maintenance and I cannot compile the Zephyr documentation. I'm sorry :( )

@rruuaanng
Copy link
Contributor Author

rruuaanng commented Nov 7, 2024

It looks nice, and conveys that it is a macro, which helps readers understand the API definition. And it shows the macro parameters nicely.

图片
图片

@rruuaanng rruuaanng requested a review from decsny November 7, 2024 14:06
@decsny
Copy link
Member

decsny commented Nov 7, 2024

I'm not sure what browser Declan is using that pops up the link, maybe it's better not to change it. I use the original browser to browse the document that uses the macro tag, as shown above.

I use firefox

Copy link
Contributor

@kartben kartben left a comment

Choose a reason for hiding this comment

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

@decsny this documentation page you're linking is not the official Sphinx docs, even if I understand the idea of suggesting the use of :c:func: for function-like macros.

@rruuaanng Your change is OK with me, but to be clear it's working just the same without it in terms of 1/ linking to doxygen when one clicks on the link 2/ showing a popup. It does however make it clearer in the .rst source that these are macros, so +1 to that

@decsny
Copy link
Member

decsny commented Nov 7, 2024

was only blocking to get opinion of @kartben or @gmarull first since I didn't know

i found out also that you can view the doc build of a PR with a URL like this https://builds.zephyrproject.io/zephyr/pr/80992/docs/build/dts/api-usage.html

it looks basically the same to me except that the text becomes blue instead of purple for macros

@kartben
Copy link
Contributor

kartben commented Nov 7, 2024

it looks basically the same to me except that the text becomes blue instead of purple for macros

Purple is just the color for links you've visited at least once :) so the CI "website" is initially all blue as you've never visited anything there. But yes, otherwise, and like I mentioned, this PR introduces no functional change

@rruuaanng
Copy link
Contributor Author

Thank you for your understanding. I believe this approach will allow the documentation to show the original semantics, which is that they are macros (and it also demonstrates the macro arguments).

@kartben
Copy link
Contributor

kartben commented Nov 7, 2024

[...] (and it also demonstrates the macro arguments).

Sorry to insist, but that was already there, just see for yourself when clicking on any of the macros in https://docs.zephyrproject.org/latest/build/dts/api-usage.html :)

@mmahadevan108 mmahadevan108 merged commit e7c3436 into zephyrproject-rtos:main Nov 8, 2024
23 of 27 checks passed
@rruuaanng rruuaanng deleted the dts-doc branch November 21, 2024 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants