Skip to content

Fix all Doxygen errors and make any future errors fatal #118

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

Merged
merged 33 commits into from
Dec 30, 2022

Conversation

multiplemonomials
Copy link
Collaborator

@multiplemonomials multiplemonomials commented Dec 30, 2022

Summary of changes

Up until now, Mbed has had a huge number of minor errors in the documentation -- things like incorrect command syntax, broken links, and wrong parameter names. This makes it tough to spot when a new change introduces any docs errors, because those errors will get mixed in with the existing ones and the build will be allowed to pass.

I've spent the past month or two working on this MR here and there, and am happy to announce that the hundreds and hundreds Doxygen errors are now resolved. This way, any future errors can be detected by the GitHub Actions CI.

While this MR has a massive code size, nearly all of the changes are to docs only. I made the following major changes:

  • Added back Mbed TLS to the docs, and fixed grouping and lots of other Doxygen syntax errors in its code
  • Made it so that only one of the three copies of Mbed PSA is being scanned, and fixed lots of doc errors there (why do we have three copies anyway?)
  • Added the __cplusplus define to Doxygen, so that some code, e.g. DNS server functions, is not being skipped
  • Clarified some parameter docs for NetworkInterface and DNS functions (had to do some deep forensics in the code to find out what the parameters were actually for)
  • Disabled WARN_IF_UNDOCUMENTED Doxygen option, because there are far too many undocumented things in Mbed to fix them all. Code reviews will be an effective way of spotting undocumented code that's being newly added.
  • Changed WARN_AS_ERROR option to FAIL_ON_WARNINGS, which causes Doxygen to die at the end if any warnings were encountered during code scanning
  • Upgraded Doxygen and doxyfile version to 1.9.1
  • Fixed typedef for NetworkStack::call_in_callback_cb_t that was duplicated in two other places.

Besides these, I just went through and fixed lots of minor mistakes in the documentation.

Impact of changes

Creating a syntax error in doxygen docs will now fail the CI

Migration actions required

Documentation

This is for docs!


Pull request type

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[X] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers


@jay-sridharan
Copy link

not really able to give it a detailed read given the size, but looking through the docs looks pretty good! Nice work!

One nice thing to have would be group descriptions. For example, looking at the top level Mbed TLS page (BUILD/html/group__mbedtls.html), all the groups within are listed, but there are no descriptions. Would be cool to add those so that one can get a general idea of what is encapsulated within that group without digging through specific function docs.

@jay-sridharan
Copy link

also, I wonder how different the mbed-tls library included with mbed-os is from this: https://github.com/Mbed-TLS/mbedtls

@multiplemonomials
Copy link
Collaborator Author

One nice thing to have would be group descriptions.

Yeah, that would be nice. Maybe we can try and add those into code as we go along.

also, I wonder how different the mbed-tls library included with mbed-os is from this: https://github.com/Mbed-TLS/mbedtls

Yeah, that's a good question. It seems like they did some rather janky stuff with copying in the source rather than adding in a submodule. At some point it should probably get switched to use a submodule, though I'm not sure I'll be able to take that on anytime soon. We will also have to upstream these docs changes at some point...

@multiplemonomials multiplemonomials merged commit 97c2672 into master Dec 30, 2022
@multiplemonomials multiplemonomials deleted the dev/fix-doxygen-warnings branch December 30, 2022 23:41
Copy link
Member

@JohnK1987 JohnK1987 left a comment

Choose a reason for hiding this comment

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

There is a lot of files and I do not have too much experiences with this. Some things are unclear for me, but I do not know if it is right or not, so I placed some points.

Edit: @multiplemonomials I had it opened from yesterday night so I did not see it was already Merged. I was not able to check it in two hour...

BR, Jan

@multiplemonomials
Copy link
Collaborator Author

Oh, right, Jay was helping me review and approved this MR. Thanks for the comments though, I'll take them under advisement

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