all: fix all undefined doxygen groups by aabadie · Pull Request #9264 · RIOT-OS/RIOT

aabadie · 2018-06-01T13:15:16Z

Contribution description

This PR fixes #8972 by adding a doxygen groups checker script. This script is based on the gist provided by @kaspar030 and which is working very well.
I slightly adapted it to not take into account the vendor header files and to make it return the full list of non matching groups.

In the meantime, I fixed all issues raised by the script. But I think I'll split this PR in several parts to simplify the reviews:

provide the check script alone, e.g not integrated in the CI
provide a PR per top folder (boards, cpu, etc)
enable the script in the CI

Issues/PRs references

fixes #8972 ~~and based on #9266~~

miri64 · 2018-06-01T18:42:08Z

Can we divide this up into two PRs please? One introducing the script (but not wired hot yet, just silently warning in the output) and one fixing the warnings?

Also, since this is doc, why isn't this included into doccheck?

aabadie · 2018-06-01T19:12:25Z

Can we divide this up into two PRs please?

Sure, that's what I wrote in the initial comment of this PR (well not exactly, but the idea was there)

since this is doc, why isn't this included into doccheck?

I thought it would be simpler to have it separate first. But I'm fine with merging the two once all grouping issues are fixed.

miri64 · 2018-06-05T08:24:22Z

doc.txt

+
+/**
+ * @defgroup    tests Tests
+ * @brief       Tests applications


AFAIK those groups will be empty in the rendered doc. Do we really need them?

Yes, all examples are using @ingroup examples and tests @ingroup tests. I know this is not nice but at least it silents the check script.
There are 3 solutions:

Remove all @ingroup tests and @ingroup examples

Ignore undefined examples and tests groups in the check script

Define tests and examples groups and document them. If this solution is pushed a bit, we could have all examples and tests documented in doxygen (instead of READMEs).

I think 1. should be avoided because it will touch too many files and maybe one day it can be useful to have those groups defined.
2. is the simplest.
3. is IMHO technically and from a centralized documentation the best solution. But requires more work.

I think 3 does make little sense. While I prefer API documentation and usage documentation to be centralized, the READMEs of applications should be kept with the application for two reasons:

One can easily open a README in command-line or their favorite editor/IDE while working with the example and does not need to open an extra browser window

The context might get lost if the README is moved somewhere else

As such, I prefer 2, but instead of ignoring the groups I rather would ignore the directories (as e.g. the vendor headers).

Furthermore, I don't see the point of being so strict with group names in .c-files, as they are not parsed by doxygen, so they can't break the rendered doc (which was the whole point of my initial issue), so it might also be sensible to ignore .c-files altogether. But maybe I'm in the minority with this opinion.

I dropped the defined examples and tests groups. Will update #9266 with solution 2.

miri64 · 2018-06-05T08:25:06Z

pkg/lwip/include/arch/cc.h

@@ -8,7 +8,7 @@

 /**
 * @defgroup    lwip_arch_cc    Compiler and processor description


pkg_lwip_arch_cc?

Ok, but a bit out of the scope of this PR. If you insist, I can also change that.

It's less out of scope than the introduction of auto_init doc below since this PR is about group naming ;-).

Ok, you insist ;)

miri64 · 2018-06-05T08:25:24Z

pkg/lwip/include/arch/sys_arch.h

@@ -8,7 +8,7 @@

 /**
 * @defgroup    lwip_arch_sys_arch Architecture depentent definitions


pkg_lwip_arch_sys_arch?

miri64 · 2018-06-05T08:25:40Z

pkg/lwip/include/lwipopts.h

@@ -8,7 +8,7 @@

 /**
 * @defgroup    lwip_opts   lwIP options


pkg_lwip_opts?

all lwip pkg groups are fixed

miri64 · 2018-06-05T08:28:02Z

sys/auto_init/auto_init.c

 * directory for more details.
 *
- * @ingroup auto_init
+ * @ingroup sys_autoinit


Here and below: shouldn't it rather be sys_auto_init?

I kept the initially defined sys_autoinit, I'll use your suggestion.

name changed

miri64 · 2018-06-05T08:28:58Z

sys/include/auto_init.h

+ * This module contains all netif drivers that support auto-initialization.
+ */
+
+/**


Unrelated change?

the changes related to auto_init was a bit problematic. I went for the improvement of the initial doc and add the missing groups there. Do you have a better suggestion ?

Ah sorry, forgot about this comment. Iff you insist to keep the doc improvement in this PR, please at least factor it out of 6f91bf8 into a separate commit.

please at least factor it out of 6f91bf8 into a separate commit.

done

miri64 · 2018-06-05T08:29:27Z

sys/include/crypto/modes/ccm.h

+#define CCM_ERR_INVALID_DATA_LENGTH         (-3)
+#define CCM_ERR_INVALID_LENGTH_ENCODING     (-4)
+#define CCM_ERR_INVALID_MAC_LENGTH          (-5)
+/** @} */


No, fixing the doxygen for this file is allowing the processing of doxygen in this file and, in the end, triggers a warning from doxygen because of undocumented defines. This change is fixing this. Do you want it in a separate commit ?

aabadie · 2018-06-07T17:50:24Z

Rebased, now that #9266 is merged.

aabadie · 2018-06-07T17:54:54Z

@miri64, are your concerns addressed here so I can squash ?

miri64

Given the number of defects, I really prefer to move all doc enhancements to a separate PR, and only have the group renames in here.

miri64 · 2018-06-07T18:08:40Z

sys/include/auto_init.h

+ * This feature can be enabled in any application by adding the `auto_init`
+ * module to the application Makefile:
+ *
+ * ```mk


This syntax doesn't work in Doxygen. Try

~~~~~~~~~~~~~~~~~~~~~~~ {.mk} USEMODULE += auto_init ~~~~~~~~~~~~~~~~~~~~~~~

instead if you like to enable syntax highlighting, otherwise remove the mk.

removed the commit related to that change. Will open a dedicated PR for that later.

miri64 · 2018-06-07T18:09:35Z

sys/include/auto_init.h

+ * `auto_init` initializes any included module that provides auto
+ * initialization capabilities.
+ *
+ * Drivers or cpu peripherals that provides a SAUL adaption and net interfaces


"network interfaces"

miri64 · 2018-06-07T18:09:53Z

sys/include/auto_init.h

+ * initialization capabilities.
+ *
+ * Drivers or cpu peripherals that provides a SAUL adaption and net interfaces
+ * can be initialized automatically with auto_init.


`` around auto_init

miri64 · 2018-06-07T18:10:14Z

sys/include/auto_init.h

+ * can be initialized automatically with auto_init.
+ *
+ * For high-level device drivers (@ref drivers), the default initialization
+ * parameters are taken from the `DRIVERS_params.h` files provided by the


miri64 · 2018-06-07T18:11:21Z

sys/include/auto_init.h

+ * The modules will be initialized in the context of the main thread right
+ * before the main function gets called. Be aware that most modules expect to
+ * be initialized only once, so do not call a module's init function when using
+ * auto_init unless you know what you're doing.


`auto_init`, unless you know what you're doing.

miri64 · 2018-06-07T18:13:19Z

sys/include/auto_init.h

+ * @ingroup     sys_auto_init
+ * @brief       Provides auto-initialization of SAUL drivers.
+ *
+ * This module contains all high-level drivers (sensor or actuators) that


Please rephrase. This sentence makes little sense:

What are high-level drivers? Do you mean SAUL as a high-level interface to the drivers?

This module only allows for auto-initialization of those drivers using SAUL, and does not contain the drivers.

miri64 · 2018-06-07T18:13:31Z

sys/include/auto_init.h

+ * @ingroup     sys_auto_init
+ * @brief       Provides auto-initialization of Netif drivers via GNRC.
+ *
+ * This module contains all netif drivers that support auto-initialization.


This module also doesn't contain any drivers, only enables their auto-initialization.

miri64 · 2018-06-07T18:15:01Z

sys/include/auto_init.h

+/**
+ * @defgroup    sys_auto_init_gnrc_netif GNRC netif drivers auto-initialization
+ * @ingroup     sys_auto_init
+ * @brief       Provides auto-initialization of Netif drivers via GNRC.


s/Netif/network device drivers/. Ideally, link to drivers_netdev, e.g.

Provides auto-initialization for [network device drivers](@ref drivers_netdev) via GNRC.

miri64

Another definitively unrelated thing.

miri64 · 2018-06-07T18:18:48Z

sys/crypto/modes/cbc.c


 /**
- * @ingroup     sys_crypto_modes
+ * @ingroup     sys_crypto_mode_cbc


If you just use sys_crypto here, the introduction of the new groups below would not be required.

You are right, fixed now.

miri64 · 2018-06-07T18:21:30Z

sys/include/crypto/modes/cbc.h

 */

 /**
+ * @defgroup    sys_crypto_mode_cbc Cipher block chaining mode


This is really completely unrelated. These files are perfectly fine to all be grouped in sys_crypto. If you disagree, please move this group introductions to a separate PR.

aabadie · 2018-06-11T15:22:33Z

@miri64, before this PR becomes unmaintainable, do you have other concerns ? I think I addressed all your comments and moved out the unrelated things.

miri64

Locally tested ACK. Please squash!

aabadie · 2018-06-11T17:13:27Z

squashed, let's wait for the CI.

miri64 · 2018-06-11T18:13:33Z

Travis fail is unrelated (and due to an old cppcheck version, I believe) => go

aabadie added Area: doc Area: Documentation Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: CI Area: Continuous Integration of RIOT components labels Jun 1, 2018

aabadie requested a review from miri64 June 1, 2018 13:15

aabadie changed the title ~~dist/tools: add doxygen groups check script~~ dist/tools: add doxygen groups checker script Jun 1, 2018

aabadie force-pushed the pr/tools/doxy_check branch from 7c0fe4a to ba66b18 Compare June 1, 2018 13:49

aabadie requested a review from kaspar030 June 1, 2018 14:01

aabadie added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: tools Area: Supplementary tools labels Jun 1, 2018

aabadie mentioned this pull request Jun 1, 2018

dist/tools/doccheck: add check for undefined groups #9266

Merged

aabadie force-pushed the pr/tools/doxy_check branch from ba66b18 to 094c4eb Compare June 1, 2018 19:35

aabadie added the State: waiting for other PR State: The PR requires another PR to be merged first label Jun 1, 2018

aabadie changed the title ~~dist/tools: add doxygen groups checker script~~ all: fix all undefined doxygen groups Jun 1, 2018

aabadie added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 2, 2018

miri64 requested changes Jun 5, 2018

View reviewed changes

aabadie force-pushed the pr/tools/doxy_check branch 7 times, most recently from d6f2153 to 279bc6d Compare June 7, 2018 17:46

miri64 requested changes Jun 7, 2018

View reviewed changes

aabadie force-pushed the pr/tools/doxy_check branch from 279bc6d to b193ad1 Compare June 7, 2018 19:27

aabadie force-pushed the pr/tools/doxy_check branch from 39bb193 to 1e8866f Compare June 11, 2018 15:23

miri64 approved these changes Jun 11, 2018

View reviewed changes

aabadie added 11 commits June 11, 2018 19:10

drivers: fix doxygen grouping

a08e991

boards: fix doxygen grouping

6bc494b

cpu/native: fix doxygen grouping

4133908

cpu*: fix doxygen grouping

938677c

pkg*: fix doxygen grouping

70407a1

core: fix doxygen grouping

bd2845a

sys: fix doxygen grouping

7d013eb

sys/auto_init: fix doxygen grouping

47fc4b0

examples: fix doxygen grouping

3e61b40

tests: fix doxygen grouping

32057a8

dist/tools/doccheck: enable group check in CI

00828bb

aabadie force-pushed the pr/tools/doxy_check branch from 1e8866f to 00828bb Compare June 11, 2018 17:12

miri64 merged commit 8fd85e1 into RIOT-OS:master Jun 11, 2018

aabadie deleted the pr/tools/doxy_check branch June 11, 2018 20:01

aabadie mentioned this pull request Jun 12, 2018

sys/auto_init: improve documentation #9328

Merged

cladmi added this to the Release 2018.07 milestone Jul 10, 2018

		@@ -8,7 +8,7 @@

		/**
		* @defgroup lwip_arch_cc Compiler and processor description

		@@ -8,7 +8,7 @@

		/**
		* @defgroup lwip_arch_sys_arch Architecture depentent definitions

Conversation

aabadie commented Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Issues/PRs references

Uh oh!

miri64 commented Jun 1, 2018

Uh oh!

aabadie commented Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie Jun 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie commented Jun 7, 2018

Uh oh!

aabadie commented Jun 7, 2018

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

aabadie commented Jun 1, 2018 •

edited

Loading

aabadie commented Jun 1, 2018 •

edited

Loading

aabadie Jun 5, 2018 •

edited

Loading