[libcamera-devel,v2,4/5] libcamera: Documentation: Split public/private documentation

19375 diff mbox series

The API reference pages generated by Doxygen are comprehensive, but
therefore quite overwhelming for application developers who will
likely never need to use the majority of the library's objects. To
reduce the complexity of the documentation, split it into two runs of
doxygen.

In the first run of doxygen we pass a specific list of source files
to parse, which is built from the File arrays in Meson's build files.
This ensures that we only generate the documentation for code from
those files.

In the second run allow doxygen to generate documentation for all of
the library's objects as it currently does. This set will now be
output into build/Documentation/internal-api-html.

Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
Changes in v2:

	- Formatting fixes (Jacopo)
	- Phraseology (Laurent)
	- Switched to passing specific files to parse to doxygen rather than
	  relying on \internal to remove other docu-comments.

 Documentation/Doxyfile-internal.in     | 21 +++++++
 Documentation/Doxyfile-public.in       |  5 ++
 Documentation/Doxyfile.in              | 26 ++-------
 Documentation/meson.build              | 76 +++++++++++++++++++++++---
 include/libcamera/base/meson.build     |  7 +++
 include/libcamera/internal/meson.build |  7 +++
 include/libcamera/meson.build          | 10 ++++
 meson.build                            |  8 +++
 src/libcamera/base/class.cpp           |  1 +
 src/libcamera/base/meson.build         |  7 +++
 src/libcamera/camera.cpp               |  7 +++
 src/libcamera/camera_manager.cpp       |  1 +
 src/libcamera/framebuffer.cpp          |  6 +-
 src/libcamera/meson.build              |  7 +++
 src/libcamera/request.cpp              |  1 +
 15 files changed, 160 insertions(+), 30 deletions(-)
 create mode 100644 Documentation/Doxyfile-internal.in
 create mode 100644 Documentation/Doxyfile-public.in

Hi Dan
  I really like the split!

On Fri, Jan 05, 2024 at 04:41:03PM +0000, Daniel Scally via libcamera-devel wrote:
> The API reference pages generated by Doxygen are comprehensive, but
> therefore quite overwhelming for application developers who will
> likely never need to use the majority of the library's objects. To
> reduce the complexity of the documentation, split it into two runs of
> doxygen.
>
> In the first run of doxygen we pass a specific list of source files
> to parse, which is built from the File arrays in Meson's build files.
> This ensures that we only generate the documentation for code from
> those files.
>
> In the second run allow doxygen to generate documentation for all of
> the library's objects as it currently does. This set will now be
> output into build/Documentation/internal-api-html.
>
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
> Changes in v2:
>
> 	- Formatting fixes (Jacopo)
> 	- Phraseology (Laurent)
> 	- Switched to passing specific files to parse to doxygen rather than
> 	  relying on \internal to remove other docu-comments.
>
>  Documentation/Doxyfile-internal.in     | 21 +++++++
>  Documentation/Doxyfile-public.in       |  5 ++
>  Documentation/Doxyfile.in              | 26 ++-------
>  Documentation/meson.build              | 76 +++++++++++++++++++++++---
>  include/libcamera/base/meson.build     |  7 +++
>  include/libcamera/internal/meson.build |  7 +++
>  include/libcamera/meson.build          | 10 ++++
>  meson.build                            |  8 +++
>  src/libcamera/base/class.cpp           |  1 +
>  src/libcamera/base/meson.build         |  7 +++
>  src/libcamera/camera.cpp               |  7 +++
>  src/libcamera/camera_manager.cpp       |  1 +
>  src/libcamera/framebuffer.cpp          |  6 +-
>  src/libcamera/meson.build              |  7 +++
>  src/libcamera/request.cpp              |  1 +
>  15 files changed, 160 insertions(+), 30 deletions(-)
>  create mode 100644 Documentation/Doxyfile-internal.in
>  create mode 100644 Documentation/Doxyfile-public.in
>
> diff --git a/Documentation/Doxyfile-internal.in b/Documentation/Doxyfile-internal.in
> new file mode 100644
> index 00000000..7b3cce49
> --- /dev/null
> +++ b/Documentation/Doxyfile-internal.in
> @@ -0,0 +1,21 @@
> +# SPDX-License-Identifier: CC-BY-SA-4.0
> +
> +INPUT                  = "@TOP_SRCDIR@/Documentation" \
> +                         "@TOP_SRCDIR@/include/libcamera" \
> +                         "@TOP_SRCDIR@/src/ipa/ipu3" \
> +                         "@TOP_SRCDIR@/src/ipa/libipa" \
> +                         "@TOP_SRCDIR@/src/libcamera" \
> +                         "@TOP_BUILDDIR@/include/libcamera" \
> +                         "@TOP_BUILDDIR@/src/libcamera"
> +
> +EXCLUDE                = @TOP_SRCDIR@/include/libcamera/base/span.h \

I'm still a bit puzzled on why we don't document Span<>, but this was
already here

> +                         @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_sysfs.h \
> +                         @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_udev.h \
> +                         @TOP_SRCDIR@/include/libcamera/internal/ipc_pipe_unixsocket.h \
> +                         @TOP_SRCDIR@/src/libcamera/device_enumerator_sysfs.cpp \
> +                         @TOP_SRCDIR@/src/libcamera/device_enumerator_udev.cpp \
> +                         @TOP_SRCDIR@/src/libcamera/ipc_pipe_unixsocket.cpp \
> +                         @TOP_SRCDIR@/src/libcamera/pipeline/ \
> +                         @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \
> +                         @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \
> +                         @TOP_BUILDDIR@/src/libcamera/proxy/
> diff --git a/Documentation/Doxyfile-public.in b/Documentation/Doxyfile-public.in
> new file mode 100644
> index 00000000..cdbc03a0
> --- /dev/null
> +++ b/Documentation/Doxyfile-public.in
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: CC-BY-SA-4.0
> +
> +INPUT                  = "@TOP_SRCDIR@/Documentation" \
> +                         "@INPUT@" \
> +                         "@TOP_BUILDDIR@/src/libcamera/version.cpp"
> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> index 48fea8bc..a271c7bc 100644
> --- a/Documentation/Doxyfile.in
> +++ b/Documentation/Doxyfile.in
> @@ -21,14 +21,6 @@ CASE_SENSE_NAMES       = YES
>
>  QUIET                  = YES
>
> -INPUT                  = "@TOP_SRCDIR@/Documentation" \
> -                         "@TOP_SRCDIR@/include/libcamera" \
> -                         "@TOP_SRCDIR@/src/ipa/ipu3" \
> -                         "@TOP_SRCDIR@/src/ipa/libipa" \
> -                         "@TOP_SRCDIR@/src/libcamera" \
> -                         "@TOP_BUILDDIR@/include/libcamera" \
> -                         "@TOP_BUILDDIR@/src/libcamera"
> -
>  FILE_PATTERNS          = *.c \
>                           *.cpp \
>                           *.h \
> @@ -36,17 +28,8 @@ FILE_PATTERNS          = *.c \
>
>  RECURSIVE              = YES
>
> -EXCLUDE                = @TOP_SRCDIR@/include/libcamera/base/span.h \
> -                         @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_sysfs.h \
> -                         @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_udev.h \
> -                         @TOP_SRCDIR@/include/libcamera/internal/ipc_pipe_unixsocket.h \
> -                         @TOP_SRCDIR@/src/libcamera/device_enumerator_sysfs.cpp \
> -                         @TOP_SRCDIR@/src/libcamera/device_enumerator_udev.cpp \
> -                         @TOP_SRCDIR@/src/libcamera/ipc_pipe_unixsocket.cpp \
> -                         @TOP_SRCDIR@/src/libcamera/pipeline/ \
> -                         @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \
> -                         @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \
> -                         @TOP_BUILDDIR@/src/libcamera/proxy/
> +@INCLUDE_PATH          = @TOP_BUILDDIR@/Documentation
> +@INCLUDE               = @INCLUDE_FILE@
>
>  EXCLUDE_PATTERNS       = @TOP_BUILDDIR@/include/libcamera/ipa/*_serializer.h \
>                           @TOP_BUILDDIR@/include/libcamera/ipa/*_proxy.h \
> @@ -70,7 +53,10 @@ EXCLUDE_SYMBOLS        = libcamera::BoundMethodArgs \
>
>  EXCLUDE_SYMLINKS       = YES
>
> -HTML_OUTPUT            = api-html
> +HIDE_UNDOC_CLASSES     = @HIDE_UNDOC_CLASSES@
> +HIDE_UNDOC_MEMBERS     = @HIDE_UNDOC_MEMBERS@
> +HTML_OUTPUT            = @HTML_OUTPUT@
> +INTERNAL_DOCS          = @INTERNAL_DOCS@
>
>  GENERATE_LATEX         = NO
>
> diff --git a/Documentation/meson.build b/Documentation/meson.build
> index 7a58fec8..afaad751 100644
> --- a/Documentation/meson.build
> +++ b/Documentation/meson.build
> @@ -23,12 +23,7 @@ if doxygen.found() and dot.found()
>
>      cdata.set('PREDEFINED', ' \\\n\t\t\t '.join(doxygen_predefined))
>
> -    doxyfile = configure_file(input : 'Doxyfile.in',
> -                              output : 'Doxyfile',
> -                              configuration : cdata)
> -
> -    doxygen_input = [
> -        doxyfile,
> +    global_doxygen_input = [
>          libcamera_base_headers,
>          libcamera_base_sources,
>          libcamera_internal_headers,
> @@ -41,16 +36,79 @@ if doxygen.found() and dot.found()
>      ]
>
>      if is_variable('ipu3_ipa_sources')
> -        doxygen_input += [ipu3_ipa_sources]
> +        global_doxygen_input += [ipu3_ipa_sources]
>      endif
>
> +    # We need to generate two "include" files for the final Doxyfile which
> +    # define a set of source files to use in the documentation parsing. We
> +    # collected a list of the public sources in doxygen_public_sources, so we
> +    # pass that to the doxyfiles so that Doxyfile-public refers only to those
> +    # files. Although INPUT is sent to both, Doxyfile-internal.in doesn't refer
> +    # to it and just hardcodes the directories to parse.

Thanks, this is quite clear now!

> +    cdata.set('INPUT', '" \\\n\t\t\t "'.join(doxygen_public_sources))
> +    doxyfile_include_public = configure_file(input : 'Doxyfile-public.in',
> +                                             output : 'Doxyfile-include-public',
> +                                             configuration : cdata)
> +    doxyfile_include_internal = configure_file(input : 'Doxyfile-internal.in',
> +                                               output : 'Doxyfile-include-internal',
> +                                               configuration : cdata)
> +
> +    # We run doxygen twice - the first run excludes internal API objects as it
> +    # is intended to document the public API only. A second run covers all of
> +    # the library's objects for libcamera developers. To achieve this we need to
> +    # flag as \internal some of the comments for objects which we wish to hide,
> +    # and remove the auto generated documents via HIDE_UNDOC_CLASSES and
> +    # HIDE_UNDOC_MEMBERS.
> +
> +    cdata_public = configuration_data()
> +    cdata_public.merge_from(cdata)
> +    cdata_public.set('HIDE_UNDOC_CLASSES', 'YES')
> +    cdata_public.set('HIDE_UNDOC_MEMBERS', 'YES')
> +    cdata_public.set('HTML_OUTPUT', 'api-html')
> +    cdata_public.set('INCLUDE_FILE', 'Doxyfile-include-public')
> +    cdata_public.set('INTERNAL_DOCS', 'NO')
> +
> +    doxyfile_public = configure_file(input : 'Doxyfile.in',
> +                                     output : 'Doxyfile-public',
> +                                     configuration : cdata_public)
> +
> +    public_doxygen_input = global_doxygen_input
> +    public_doxygen_input += doxyfile_public
> +
>      custom_target('doxygen',
> -                  input : doxygen_input,
> +                  input : public_doxygen_input,
>                    output : 'api-html',
> -                  command : [doxygen, doxyfile],
> +                  command : [doxygen, doxyfile_public],
>                    install : true,
>                    install_dir : doc_install_dir,
>                    install_tag : 'doc')
> +
> +    # This is the internal documentation, so _don't_ hide undocumented classes
> +    # as we want everything to show up and warnings to be generated if any
> +    # documentation is missing.
> +
> +    cdata_internal = configuration_data()
> +    cdata_internal.merge_from(cdata)
> +    cdata_internal.set('HIDE_UNDOC_CLASSES', 'NO')
> +    cdata_internal.set('HIDE_UNDOC_MEMBERS', 'NO')
> +    cdata_internal.set('HTML_OUTPUT', 'internal-api-html')
> +    cdata_internal.set('INCLUDE_FILE', 'Doxyfile-include-internal')
> +    cdata_internal.set('INTERNAL_DOCS', 'YES')
> +
> +    doxyfile_internal = configure_file(input : 'Doxyfile.in',
> +                                       output : 'Doxyfile-internal',
> +                                       configuration : cdata_internal)
> +
> +    internal_doxygen_input = global_doxygen_input
> +    internal_doxygen_input += doxyfile_internal
> +
> +    custom_target('doxygen-internal',
> +                  input : internal_doxygen_input,
> +                  output : 'internal-api-html',
> +                  command : [doxygen, doxyfile_internal],
> +                  install : true,
> +                  install_dir : doc_install_dir,
> +                  install_tag : 'doc-internal')
>  endif
>
>  #
> diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build
> index f24f47de..82277f46 100644
> --- a/include/libcamera/base/meson.build
> +++ b/include/libcamera/base/meson.build
> @@ -38,3 +38,10 @@ libcamera_base_headers = [
>
>  install_headers(libcamera_base_public_headers,
>                  subdir : libcamera_base_include_dir)
> +
> +foreach lbph : libcamera_base_public_headers
> +	doxygen_public_sources += '/'.join(
> +		meson.project_source_root(),
> +		'@0@'.format(lbph)
> +	)
> +endforeach
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> index 4d59cb2a..4af36884 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build
> @@ -58,4 +58,11 @@ libcamera_internal_headers = [
>      libcamera_internal_headers_publically_undocumented
>  ]
>
> +foreach lph : libcamera_internal_headers_publically_documented
> +	doxygen_public_sources += '/'.join(
> +		meson.project_source_root(),
> +		'@0@'.format(lph)
> +	)
> +endforeach
> +
>  subdir('converter')
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index bab858a3..25e2f8a4 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -26,6 +26,13 @@ subdir('ipa')
>  install_headers(libcamera_public_headers,
>                  subdir : libcamera_include_dir)
>
> +foreach lph : libcamera_public_headers
> +	doxygen_public_sources += '/'.join(
> +		meson.project_source_root(),
> +		'@0@'.format(lph)
> +	)
> +endforeach
> +
>  #
>  # Generate headers from templates.
>  #
> @@ -85,6 +92,7 @@ foreach mode, entry : controls_map
>                                                  '-r', ranges_file, '@INPUT@'],
>                                       install : true,
>                                       install_dir : libcamera_headers_install_dir)
> +    doxygen_public_sources += control_headers.get(-1).full_path()
>  endforeach
>
>  libcamera_public_headers += control_headers
> @@ -101,6 +109,7 @@ formats_h = custom_target('formats_h',
>                            install : true,
>                            install_dir : libcamera_headers_install_dir)
>  libcamera_public_headers += formats_h
> +doxygen_public_sources += formats_h.full_path()
>
>  # libcamera.h
>  libcamera_h = custom_target('gen-header',
> @@ -111,6 +120,7 @@ libcamera_h = custom_target('gen-header',
>                              install_dir : libcamera_headers_install_dir)
>
>  libcamera_public_headers += libcamera_h
> +doxygen_public_sources += libcamera_h.full_path()
>
>  # version.h
>  version = libcamera_version.split('.')
> diff --git a/meson.build b/meson.build
> index e49de4c2..cca1883e 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -231,6 +231,14 @@ endif
>  # Utilities are parsed first to provide support for other components.
>  subdir('utils')
>
> +# To support auto-generation of documentation We need an array of the paths to

s/We/we

> +# public headers and source files so that we can tell doxygen which files to
> +# look at later. Unfortunately the inclusion of custom targets in some of the
> +# existing arrays precludes using them directly and you cannot generate File
> +# objects from generated files, so we need to collect paths to relevant files
> +# within an array.
> +doxygen_public_sources = []
> +
>  subdir('include')
>  subdir('src')
>
> diff --git a/src/libcamera/base/class.cpp b/src/libcamera/base/class.cpp
> index 9c2d9f21..70fd5cd5 100644
> --- a/src/libcamera/base/class.cpp
> +++ b/src/libcamera/base/class.cpp
> @@ -184,6 +184,7 @@ Extensible::Extensible(std::unique_ptr<Extensible::Private> d)
>   */
>
>  /**
> + * \internal
>   * \class Extensible::Private
>   * \brief Base class for private data managed through a d-pointer
>   */
> diff --git a/src/libcamera/base/meson.build b/src/libcamera/base/meson.build
> index 523c5885..ae949a51 100644
> --- a/src/libcamera/base/meson.build
> +++ b/src/libcamera/base/meson.build
> @@ -30,6 +30,13 @@ libcamera_base_sources = [
>  	libcamera_base_internal_sources
>  ]
>
> +foreach lbps : libcamera_base_public_sources
> +	doxygen_public_sources += '/'.join(
> +		meson.project_source_root(),
> +		'@0@'.format(lbps)
> +	)
> +endforeach
> +
>  libdw = dependency('libdw', required : false)
>  libunwind = dependency('libunwind', required : false)
>
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 0ad1a4b5..ed46f853 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -560,6 +560,13 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF
>   */
>
>  /**
> + * \internal
> + * \file libcamera\internal\camera.h
> + * \brief Internal Camera device handling
> + */
> +
> +/**
> + * \internal
>   * \class Camera::Private
>   * \brief Base class for camera private data
>   *
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index 355f3ada..61d45256 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -23,6 +23,7 @@
>   */
>
>  /**
> + * \internal
>   * \file libcamera/internal/camera_manager.h
>   * \brief Internal camera manager support
>   */

That's weird, I don't see CameraManager::Private::addCamera() and
CameraManager::Private::removeCamera() being documented. It was not
documented even before this commit though :/

> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> index 5a7f3c0b..db450e11 100644
> --- a/src/libcamera/framebuffer.cpp
> +++ b/src/libcamera/framebuffer.cpp
> @@ -16,7 +16,10 @@
>  /**
>   * \file libcamera/framebuffer.h
>   * \brief Frame buffer handling
> - *
> + */
> +
> +/**
> + * \internal
>   * \file libcamera/internal/framebuffer.h
>   * \brief Internal frame buffer handling support
>   */
> @@ -105,6 +108,7 @@ LOG_DEFINE_CATEGORY(Buffer)
>   */
>
>  /**
> + * \internal
>   * \class FrameBuffer::Private
>   * \brief Base class for FrameBuffer private data
>   *
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 676470c1..413e14db 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -59,6 +59,13 @@ libcamera_sources = [
>  	libcamera_internal_sources
>  ]
>
> +foreach lps : libcamera_public_sources
> +	doxygen_public_sources += '/'.join(
> +		meson.project_source_root(),
> +		'@0@'.format(lps)
> +	)
> +endforeach
> +
>  libcamera_sources += libcamera_public_headers
>  libcamera_sources += libcamera_generated_ipa_headers
>  libcamera_sources += libcamera_tracepoint_header
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index 949c556f..930d8c92 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -33,6 +33,7 @@ namespace libcamera {
>  LOG_DEFINE_CATEGORY(Request)
>
>  /**
> + * \internal
>   * \class Request::Private
>   * \brief Request private data
>   *

So we should remember to mark every classes that derives a private
implementation from Extensible to mark it as \internal. I think it's
fine, and I like the patch  a lot!

With the few above points clarified
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
   j

> --
> 2.34.1
>

Quoting Jacopo Mondi via libcamera-devel (2024-01-09 14:28:07)
> Hi Dan
>   I really like the split!
> 
> On Fri, Jan 05, 2024 at 04:41:03PM +0000, Daniel Scally via libcamera-devel wrote:
> > The API reference pages generated by Doxygen are comprehensive, but
> > therefore quite overwhelming for application developers who will
> > likely never need to use the majority of the library's objects. To
> > reduce the complexity of the documentation, split it into two runs of
> > doxygen.
> >
> > In the first run of doxygen we pass a specific list of source files
> > to parse, which is built from the File arrays in Meson's build files.
> > This ensures that we only generate the documentation for code from
> > those files.
> >
> > In the second run allow doxygen to generate documentation for all of
> > the library's objects as it currently does. This set will now be
> > output into build/Documentation/internal-api-html.
> >
> > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> > ---
> > Changes in v2:
> >
> >       - Formatting fixes (Jacopo)
> >       - Phraseology (Laurent)
> >       - Switched to passing specific files to parse to doxygen rather than
> >         relying on \internal to remove other docu-comments.
> >
> >  Documentation/Doxyfile-internal.in     | 21 +++++++
> >  Documentation/Doxyfile-public.in       |  5 ++
> >  Documentation/Doxyfile.in              | 26 ++-------
> >  Documentation/meson.build              | 76 +++++++++++++++++++++++---
> >  include/libcamera/base/meson.build     |  7 +++
> >  include/libcamera/internal/meson.build |  7 +++
> >  include/libcamera/meson.build          | 10 ++++
> >  meson.build                            |  8 +++
> >  src/libcamera/base/class.cpp           |  1 +
> >  src/libcamera/base/meson.build         |  7 +++
> >  src/libcamera/camera.cpp               |  7 +++
> >  src/libcamera/camera_manager.cpp       |  1 +
> >  src/libcamera/framebuffer.cpp          |  6 +-
> >  src/libcamera/meson.build              |  7 +++
> >  src/libcamera/request.cpp              |  1 +
> >  15 files changed, 160 insertions(+), 30 deletions(-)
> >  create mode 100644 Documentation/Doxyfile-internal.in
> >  create mode 100644 Documentation/Doxyfile-public.in
> >
> > diff --git a/Documentation/Doxyfile-internal.in b/Documentation/Doxyfile-internal.in
> > new file mode 100644
> > index 00000000..7b3cce49
> > --- /dev/null
> > +++ b/Documentation/Doxyfile-internal.in
> > @@ -0,0 +1,21 @@
> > +# SPDX-License-Identifier: CC-BY-SA-4.0
> > +
> > +INPUT                  = "@TOP_SRCDIR@/Documentation" \
> > +                         "@TOP_SRCDIR@/include/libcamera" \
> > +                         "@TOP_SRCDIR@/src/ipa/ipu3" \
> > +                         "@TOP_SRCDIR@/src/ipa/libipa" \
> > +                         "@TOP_SRCDIR@/src/libcamera" \
> > +                         "@TOP_BUILDDIR@/include/libcamera" \
> > +                         "@TOP_BUILDDIR@/src/libcamera"
> > +
> > +EXCLUDE                = @TOP_SRCDIR@/include/libcamera/base/span.h \
> 
> I'm still a bit puzzled on why we don't document Span<>, but this was
> already here

Presumably this is expected to be covered by the C++ definition of a
Span. It's supposed to be a 'like for like' implementation. (or as close
as possible I think).

Perhaps somewhere we should document that fact. In the 'long future'
(when we can support C++20 - I would expect we'd convert to use
std::span in place of libcamera::Span.

> > +                         @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_sysfs.h \
> > +                         @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_udev.h \
> > +                         @TOP_SRCDIR@/include/libcamera/internal/ipc_pipe_unixsocket.h \
> > +                         @TOP_SRCDIR@/src/libcamera/device_enumerator_sysfs.cpp \
> > +                         @TOP_SRCDIR@/src/libcamera/device_enumerator_udev.cpp \
> > +                         @TOP_SRCDIR@/src/libcamera/ipc_pipe_unixsocket.cpp \
> > +                         @TOP_SRCDIR@/src/libcamera/pipeline/ \
> > +                         @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \
> > +                         @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \
> > +                         @TOP_BUILDDIR@/src/libcamera/proxy/
> > diff --git a/Documentation/Doxyfile-public.in b/Documentation/Doxyfile-public.in
> > new file mode 100644
> > index 00000000..cdbc03a0
> > --- /dev/null
> > +++ b/Documentation/Doxyfile-public.in
> > @@ -0,0 +1,5 @@
> > +# SPDX-License-Identifier: CC-BY-SA-4.0
> > +
> > +INPUT                  = "@TOP_SRCDIR@/Documentation" \
> > +                         "@INPUT@" \
> > +                         "@TOP_BUILDDIR@/src/libcamera/version.cpp"
> > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> > index 48fea8bc..a271c7bc 100644
> > --- a/Documentation/Doxyfile.in
> > +++ b/Documentation/Doxyfile.in
> > @@ -21,14 +21,6 @@ CASE_SENSE_NAMES       = YES
> >
> >  QUIET                  = YES
> >
> > -INPUT                  = "@TOP_SRCDIR@/Documentation" \
> > -                         "@TOP_SRCDIR@/include/libcamera" \
> > -                         "@TOP_SRCDIR@/src/ipa/ipu3" \
> > -                         "@TOP_SRCDIR@/src/ipa/libipa" \
> > -                         "@TOP_SRCDIR@/src/libcamera" \
> > -                         "@TOP_BUILDDIR@/include/libcamera" \
> > -                         "@TOP_BUILDDIR@/src/libcamera"
> > -
> >  FILE_PATTERNS          = *.c \
> >                           *.cpp \
> >                           *.h \
> > @@ -36,17 +28,8 @@ FILE_PATTERNS          = *.c \
> >
> >  RECURSIVE              = YES
> >
> > -EXCLUDE                = @TOP_SRCDIR@/include/libcamera/base/span.h \
> > -                         @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_sysfs.h \
> > -                         @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_udev.h \
> > -                         @TOP_SRCDIR@/include/libcamera/internal/ipc_pipe_unixsocket.h \
> > -                         @TOP_SRCDIR@/src/libcamera/device_enumerator_sysfs.cpp \
> > -                         @TOP_SRCDIR@/src/libcamera/device_enumerator_udev.cpp \
> > -                         @TOP_SRCDIR@/src/libcamera/ipc_pipe_unixsocket.cpp \
> > -                         @TOP_SRCDIR@/src/libcamera/pipeline/ \
> > -                         @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \
> > -                         @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \
> > -                         @TOP_BUILDDIR@/src/libcamera/proxy/
> > +@INCLUDE_PATH          = @TOP_BUILDDIR@/Documentation
> > +@INCLUDE               = @INCLUDE_FILE@
> >
> >  EXCLUDE_PATTERNS       = @TOP_BUILDDIR@/include/libcamera/ipa/*_serializer.h \
> >                           @TOP_BUILDDIR@/include/libcamera/ipa/*_proxy.h \
> > @@ -70,7 +53,10 @@ EXCLUDE_SYMBOLS        = libcamera::BoundMethodArgs \
> >
> >  EXCLUDE_SYMLINKS       = YES
> >
> > -HTML_OUTPUT            = api-html
> > +HIDE_UNDOC_CLASSES     = @HIDE_UNDOC_CLASSES@
> > +HIDE_UNDOC_MEMBERS     = @HIDE_UNDOC_MEMBERS@
> > +HTML_OUTPUT            = @HTML_OUTPUT@
> > +INTERNAL_DOCS          = @INTERNAL_DOCS@
> >
> >  GENERATE_LATEX         = NO
> >
> > diff --git a/Documentation/meson.build b/Documentation/meson.build
> > index 7a58fec8..afaad751 100644
> > --- a/Documentation/meson.build
> > +++ b/Documentation/meson.build
> > @@ -23,12 +23,7 @@ if doxygen.found() and dot.found()
> >
> >      cdata.set('PREDEFINED', ' \\\n\t\t\t '.join(doxygen_predefined))
> >
> > -    doxyfile = configure_file(input : 'Doxyfile.in',
> > -                              output : 'Doxyfile',
> > -                              configuration : cdata)
> > -
> > -    doxygen_input = [
> > -        doxyfile,
> > +    global_doxygen_input = [
> >          libcamera_base_headers,
> >          libcamera_base_sources,
> >          libcamera_internal_headers,
> > @@ -41,16 +36,79 @@ if doxygen.found() and dot.found()
> >      ]
> >
> >      if is_variable('ipu3_ipa_sources')
> > -        doxygen_input += [ipu3_ipa_sources]
> > +        global_doxygen_input += [ipu3_ipa_sources]
> >      endif
> >
> > +    # We need to generate two "include" files for the final Doxyfile which
> > +    # define a set of source files to use in the documentation parsing. We
> > +    # collected a list of the public sources in doxygen_public_sources, so we
> > +    # pass that to the doxyfiles so that Doxyfile-public refers only to those
> > +    # files. Although INPUT is sent to both, Doxyfile-internal.in doesn't refer
> > +    # to it and just hardcodes the directories to parse.
> 
> Thanks, this is quite clear now!
> 
> > +    cdata.set('INPUT', '" \\\n\t\t\t "'.join(doxygen_public_sources))
> > +    doxyfile_include_public = configure_file(input : 'Doxyfile-public.in',
> > +                                             output : 'Doxyfile-include-public',
> > +                                             configuration : cdata)
> > +    doxyfile_include_internal = configure_file(input : 'Doxyfile-internal.in',
> > +                                               output : 'Doxyfile-include-internal',
> > +                                               configuration : cdata)
> > +
> > +    # We run doxygen twice - the first run excludes internal API objects as it
> > +    # is intended to document the public API only. A second run covers all of
> > +    # the library's objects for libcamera developers. To achieve this we need to
> > +    # flag as \internal some of the comments for objects which we wish to hide,
> > +    # and remove the auto generated documents via HIDE_UNDOC_CLASSES and
> > +    # HIDE_UNDOC_MEMBERS.
> > +
> > +    cdata_public = configuration_data()
> > +    cdata_public.merge_from(cdata)
> > +    cdata_public.set('HIDE_UNDOC_CLASSES', 'YES')
> > +    cdata_public.set('HIDE_UNDOC_MEMBERS', 'YES')
> > +    cdata_public.set('HTML_OUTPUT', 'api-html')
> > +    cdata_public.set('INCLUDE_FILE', 'Doxyfile-include-public')
> > +    cdata_public.set('INTERNAL_DOCS', 'NO')
> > +
> > +    doxyfile_public = configure_file(input : 'Doxyfile.in',
> > +                                     output : 'Doxyfile-public',
> > +                                     configuration : cdata_public)
> > +
> > +    public_doxygen_input = global_doxygen_input
> > +    public_doxygen_input += doxyfile_public
> > +
> >      custom_target('doxygen',
> > -                  input : doxygen_input,
> > +                  input : public_doxygen_input,
> >                    output : 'api-html',
> > -                  command : [doxygen, doxyfile],
> > +                  command : [doxygen, doxyfile_public],
> >                    install : true,
> >                    install_dir : doc_install_dir,
> >                    install_tag : 'doc')
> > +
> > +    # This is the internal documentation, so _don't_ hide undocumented classes
> > +    # as we want everything to show up and warnings to be generated if any
> > +    # documentation is missing.
> > +
> > +    cdata_internal = configuration_data()
> > +    cdata_internal.merge_from(cdata)
> > +    cdata_internal.set('HIDE_UNDOC_CLASSES', 'NO')
> > +    cdata_internal.set('HIDE_UNDOC_MEMBERS', 'NO')
> > +    cdata_internal.set('HTML_OUTPUT', 'internal-api-html')
> > +    cdata_internal.set('INCLUDE_FILE', 'Doxyfile-include-internal')
> > +    cdata_internal.set('INTERNAL_DOCS', 'YES')
> > +
> > +    doxyfile_internal = configure_file(input : 'Doxyfile.in',
> > +                                       output : 'Doxyfile-internal',
> > +                                       configuration : cdata_internal)
> > +
> > +    internal_doxygen_input = global_doxygen_input
> > +    internal_doxygen_input += doxyfile_internal
> > +
> > +    custom_target('doxygen-internal',
> > +                  input : internal_doxygen_input,
> > +                  output : 'internal-api-html',
> > +                  command : [doxygen, doxyfile_internal],
> > +                  install : true,
> > +                  install_dir : doc_install_dir,
> > +                  install_tag : 'doc-internal')
> >  endif
> >
> >  #
> > diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build
> > index f24f47de..82277f46 100644
> > --- a/include/libcamera/base/meson.build
> > +++ b/include/libcamera/base/meson.build
> > @@ -38,3 +38,10 @@ libcamera_base_headers = [
> >
> >  install_headers(libcamera_base_public_headers,
> >                  subdir : libcamera_base_include_dir)
> > +
> > +foreach lbph : libcamera_base_public_headers
> > +     doxygen_public_sources += '/'.join(
> > +             meson.project_source_root(),
> > +             '@0@'.format(lbph)
> > +     )
> > +endforeach
> > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> > index 4d59cb2a..4af36884 100644
> > --- a/include/libcamera/internal/meson.build
> > +++ b/include/libcamera/internal/meson.build
> > @@ -58,4 +58,11 @@ libcamera_internal_headers = [
> >      libcamera_internal_headers_publically_undocumented
> >  ]
> >
> > +foreach lph : libcamera_internal_headers_publically_documented

I think I saw a suggestion somewhere to drop _publically anyway, but
otherwise I think the correct spelling is 'publicly'.


> > +     doxygen_public_sources += '/'.join(
> > +             meson.project_source_root(),
> > +             '@0@'.format(lph)
> > +     )
> > +endforeach
> > +
> >  subdir('converter')
> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > index bab858a3..25e2f8a4 100644
> > --- a/include/libcamera/meson.build
> > +++ b/include/libcamera/meson.build
> > @@ -26,6 +26,13 @@ subdir('ipa')
> >  install_headers(libcamera_public_headers,
> >                  subdir : libcamera_include_dir)
> >
> > +foreach lph : libcamera_public_headers
> > +     doxygen_public_sources += '/'.join(
> > +             meson.project_source_root(),
> > +             '@0@'.format(lph)
> > +     )

I'm surprised meson doesn't have better helpers for this.
It might be worth a check through with meson to see if there's a cleaner
way to handle this. But otherwise, if this is the only way to get the
fully qualified paths if that's what's needed...

> > +endforeach
> > +
> >  #
> >  # Generate headers from templates.
> >  #
> > @@ -85,6 +92,7 @@ foreach mode, entry : controls_map
> >                                                  '-r', ranges_file, '@INPUT@'],
> >                                       install : true,
> >                                       install_dir : libcamera_headers_install_dir)
> > +    doxygen_public_sources += control_headers.get(-1).full_path()

What's going on here? (Why a .get(-1) ?)

> >  endforeach
> >
> >  libcamera_public_headers += control_headers
> > @@ -101,6 +109,7 @@ formats_h = custom_target('formats_h',
> >                            install : true,
> >                            install_dir : libcamera_headers_install_dir)
> >  libcamera_public_headers += formats_h
> > +doxygen_public_sources += formats_h.full_path()
> >
> >  # libcamera.h
> >  libcamera_h = custom_target('gen-header',
> > @@ -111,6 +120,7 @@ libcamera_h = custom_target('gen-header',
> >                              install_dir : libcamera_headers_install_dir)
> >
> >  libcamera_public_headers += libcamera_h
> > +doxygen_public_sources += libcamera_h.full_path()
> >
> >  # version.h
> >  version = libcamera_version.split('.')
> > diff --git a/meson.build b/meson.build
> > index e49de4c2..cca1883e 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -231,6 +231,14 @@ endif
> >  # Utilities are parsed first to provide support for other components.
> >  subdir('utils')
> >
> > +# To support auto-generation of documentation We need an array of the paths to
> 
> s/We/we
> 
> > +# public headers and source files so that we can tell doxygen which files to
> > +# look at later. Unfortunately the inclusion of custom targets in some of the
> > +# existing arrays precludes using them directly and you cannot generate File
> > +# objects from generated files, so we need to collect paths to relevant files
> > +# within an array.
> > +doxygen_public_sources = []

Aha, that might have answered my question above.

> > +
> >  subdir('include')
> >  subdir('src')
> >
> > diff --git a/src/libcamera/base/class.cpp b/src/libcamera/base/class.cpp
> > index 9c2d9f21..70fd5cd5 100644
> > --- a/src/libcamera/base/class.cpp
> > +++ b/src/libcamera/base/class.cpp
> > @@ -184,6 +184,7 @@ Extensible::Extensible(std::unique_ptr<Extensible::Private> d)
> >   */
> >
> >  /**
> > + * \internal
> >   * \class Extensible::Private
> >   * \brief Base class for private data managed through a d-pointer
> >   */
> > diff --git a/src/libcamera/base/meson.build b/src/libcamera/base/meson.build
> > index 523c5885..ae949a51 100644
> > --- a/src/libcamera/base/meson.build
> > +++ b/src/libcamera/base/meson.build
> > @@ -30,6 +30,13 @@ libcamera_base_sources = [
> >       libcamera_base_internal_sources
> >  ]
> >
> > +foreach lbps : libcamera_base_public_sources
> > +     doxygen_public_sources += '/'.join(
> > +             meson.project_source_root(),
> > +             '@0@'.format(lbps)
> > +     )
> > +endforeach
> > +
> >  libdw = dependency('libdw', required : false)
> >  libunwind = dependency('libunwind', required : false)
> >
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 0ad1a4b5..ed46f853 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -560,6 +560,13 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF
> >   */
> >
> >  /**
> > + * \internal
> > + * \file libcamera\internal\camera.h
> > + * \brief Internal Camera device handling
> > + */
> > +
> > +/**
> > + * \internal
> >   * \class Camera::Private
> >   * \brief Base class for camera private data
> >   *
> > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> > index 355f3ada..61d45256 100644
> > --- a/src/libcamera/camera_manager.cpp
> > +++ b/src/libcamera/camera_manager.cpp
> > @@ -23,6 +23,7 @@
> >   */
> >
> >  /**
> > + * \internal
> >   * \file libcamera/internal/camera_manager.h
> >   * \brief Internal camera manager support
> >   */
> 
> That's weird, I don't see CameraManager::Private::addCamera() and
> CameraManager::Private::removeCamera() being documented. It was not
> documented even before this commit though :/
> 
> > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> > index 5a7f3c0b..db450e11 100644
> > --- a/src/libcamera/framebuffer.cpp
> > +++ b/src/libcamera/framebuffer.cpp
> > @@ -16,7 +16,10 @@
> >  /**
> >   * \file libcamera/framebuffer.h
> >   * \brief Frame buffer handling
> > - *
> > + */
> > +
> > +/**
> > + * \internal
> >   * \file libcamera/internal/framebuffer.h
> >   * \brief Internal frame buffer handling support
> >   */
> > @@ -105,6 +108,7 @@ LOG_DEFINE_CATEGORY(Buffer)
> >   */
> >
> >  /**
> > + * \internal
> >   * \class FrameBuffer::Private
> >   * \brief Base class for FrameBuffer private data
> >   *
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index 676470c1..413e14db 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -59,6 +59,13 @@ libcamera_sources = [
> >       libcamera_internal_sources
> >  ]
> >
> > +foreach lps : libcamera_public_sources
> > +     doxygen_public_sources += '/'.join(
> > +             meson.project_source_root(),
> > +             '@0@'.format(lps)
> > +     )
> > +endforeach
> > +
> >  libcamera_sources += libcamera_public_headers
> >  libcamera_sources += libcamera_generated_ipa_headers
> >  libcamera_sources += libcamera_tracepoint_header
> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> > index 949c556f..930d8c92 100644
> > --- a/src/libcamera/request.cpp
> > +++ b/src/libcamera/request.cpp
> > @@ -33,6 +33,7 @@ namespace libcamera {
> >  LOG_DEFINE_CATEGORY(Request)
> >
> >  /**
> > + * \internal
> >   * \class Request::Private
> >   * \brief Request private data
> >   *
> 
> So we should remember to mark every classes that derives a private
> implementation from Extensible to mark it as \internal. I think it's
> fine, and I like the patch  a lot!
> 

Likewise, I suspect we can try to come up with a checkstyle reminder
sometime.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> With the few above points clarified
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> Thanks
>    j
> 
> > --
> > 2.34.1
> >

Morning Kieran

On 09/01/2024 22:08, Kieran Bingham wrote:
> Quoting Jacopo Mondi via libcamera-devel (2024-01-09 14:28:07)
>> Hi Dan
>>    I really like the split!
>>
>> On Fri, Jan 05, 2024 at 04:41:03PM +0000, Daniel Scally via libcamera-devel wrote:
>>> The API reference pages generated by Doxygen are comprehensive, but
>>> therefore quite overwhelming for application developers who will
>>> likely never need to use the majority of the library's objects. To
>>> reduce the complexity of the documentation, split it into two runs of
>>> doxygen.
>>>
>>> In the first run of doxygen we pass a specific list of source files
>>> to parse, which is built from the File arrays in Meson's build files.
>>> This ensures that we only generate the documentation for code from
>>> those files.
>>>
>>> In the second run allow doxygen to generate documentation for all of
>>> the library's objects as it currently does. This set will now be
>>> output into build/Documentation/internal-api-html.
>>>
>>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>>> ---
>>> Changes in v2:
>>>
>>>        - Formatting fixes (Jacopo)
>>>        - Phraseology (Laurent)
>>>        - Switched to passing specific files to parse to doxygen rather than
>>>          relying on \internal to remove other docu-comments.
>>>
>>>   Documentation/Doxyfile-internal.in     | 21 +++++++
>>>   Documentation/Doxyfile-public.in       |  5 ++
>>>   Documentation/Doxyfile.in              | 26 ++-------
>>>   Documentation/meson.build              | 76 +++++++++++++++++++++++---
>>>   include/libcamera/base/meson.build     |  7 +++
>>>   include/libcamera/internal/meson.build |  7 +++
>>>   include/libcamera/meson.build          | 10 ++++
>>>   meson.build                            |  8 +++
>>>   src/libcamera/base/class.cpp           |  1 +
>>>   src/libcamera/base/meson.build         |  7 +++
>>>   src/libcamera/camera.cpp               |  7 +++
>>>   src/libcamera/camera_manager.cpp       |  1 +
>>>   src/libcamera/framebuffer.cpp          |  6 +-
>>>   src/libcamera/meson.build              |  7 +++
>>>   src/libcamera/request.cpp              |  1 +
>>>   15 files changed, 160 insertions(+), 30 deletions(-)
>>>   create mode 100644 Documentation/Doxyfile-internal.in
>>>   create mode 100644 Documentation/Doxyfile-public.in
>>>
>>> diff --git a/Documentation/Doxyfile-internal.in b/Documentation/Doxyfile-internal.in
>>> new file mode 100644
>>> index 00000000..7b3cce49
>>> --- /dev/null
>>> +++ b/Documentation/Doxyfile-internal.in
>>> @@ -0,0 +1,21 @@
>>> +# SPDX-License-Identifier: CC-BY-SA-4.0
>>> +
>>> +INPUT                  = "@TOP_SRCDIR@/Documentation" \
>>> +                         "@TOP_SRCDIR@/include/libcamera" \
>>> +                         "@TOP_SRCDIR@/src/ipa/ipu3" \
>>> +                         "@TOP_SRCDIR@/src/ipa/libipa" \
>>> +                         "@TOP_SRCDIR@/src/libcamera" \
>>> +                         "@TOP_BUILDDIR@/include/libcamera" \
>>> +                         "@TOP_BUILDDIR@/src/libcamera"
>>> +
>>> +EXCLUDE                = @TOP_SRCDIR@/include/libcamera/base/span.h \
>> I'm still a bit puzzled on why we don't document Span<>, but this was
>> already here
> Presumably this is expected to be covered by the C++ definition of a
> Span. It's supposed to be a 'like for like' implementation. (or as close
> as possible I think).
>
> Perhaps somewhere we should document that fact. In the 'long future'
> (when we can support C++20 - I would expect we'd convert to use
> std::span in place of libcamera::Span.
>
>>> +                         @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_sysfs.h \
>>> +                         @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_udev.h \
>>> +                         @TOP_SRCDIR@/include/libcamera/internal/ipc_pipe_unixsocket.h \
>>> +                         @TOP_SRCDIR@/src/libcamera/device_enumerator_sysfs.cpp \
>>> +                         @TOP_SRCDIR@/src/libcamera/device_enumerator_udev.cpp \
>>> +                         @TOP_SRCDIR@/src/libcamera/ipc_pipe_unixsocket.cpp \
>>> +                         @TOP_SRCDIR@/src/libcamera/pipeline/ \
>>> +                         @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \
>>> +                         @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \
>>> +                         @TOP_BUILDDIR@/src/libcamera/proxy/
>>> diff --git a/Documentation/Doxyfile-public.in b/Documentation/Doxyfile-public.in
>>> new file mode 100644
>>> index 00000000..cdbc03a0
>>> --- /dev/null
>>> +++ b/Documentation/Doxyfile-public.in
>>> @@ -0,0 +1,5 @@
>>> +# SPDX-License-Identifier: CC-BY-SA-4.0
>>> +
>>> +INPUT                  = "@TOP_SRCDIR@/Documentation" \
>>> +                         "@INPUT@" \
>>> +                         "@TOP_BUILDDIR@/src/libcamera/version.cpp"
>>> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
>>> index 48fea8bc..a271c7bc 100644
>>> --- a/Documentation/Doxyfile.in
>>> +++ b/Documentation/Doxyfile.in
>>> @@ -21,14 +21,6 @@ CASE_SENSE_NAMES       = YES
>>>
>>>   QUIET                  = YES
>>>
>>> -INPUT                  = "@TOP_SRCDIR@/Documentation" \
>>> -                         "@TOP_SRCDIR@/include/libcamera" \
>>> -                         "@TOP_SRCDIR@/src/ipa/ipu3" \
>>> -                         "@TOP_SRCDIR@/src/ipa/libipa" \
>>> -                         "@TOP_SRCDIR@/src/libcamera" \
>>> -                         "@TOP_BUILDDIR@/include/libcamera" \
>>> -                         "@TOP_BUILDDIR@/src/libcamera"
>>> -
>>>   FILE_PATTERNS          = *.c \
>>>                            *.cpp \
>>>                            *.h \
>>> @@ -36,17 +28,8 @@ FILE_PATTERNS          = *.c \
>>>
>>>   RECURSIVE              = YES
>>>
>>> -EXCLUDE                = @TOP_SRCDIR@/include/libcamera/base/span.h \
>>> -                         @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_sysfs.h \
>>> -                         @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_udev.h \
>>> -                         @TOP_SRCDIR@/include/libcamera/internal/ipc_pipe_unixsocket.h \
>>> -                         @TOP_SRCDIR@/src/libcamera/device_enumerator_sysfs.cpp \
>>> -                         @TOP_SRCDIR@/src/libcamera/device_enumerator_udev.cpp \
>>> -                         @TOP_SRCDIR@/src/libcamera/ipc_pipe_unixsocket.cpp \
>>> -                         @TOP_SRCDIR@/src/libcamera/pipeline/ \
>>> -                         @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \
>>> -                         @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \
>>> -                         @TOP_BUILDDIR@/src/libcamera/proxy/
>>> +@INCLUDE_PATH          = @TOP_BUILDDIR@/Documentation
>>> +@INCLUDE               = @INCLUDE_FILE@
>>>
>>>   EXCLUDE_PATTERNS       = @TOP_BUILDDIR@/include/libcamera/ipa/*_serializer.h \
>>>                            @TOP_BUILDDIR@/include/libcamera/ipa/*_proxy.h \
>>> @@ -70,7 +53,10 @@ EXCLUDE_SYMBOLS        = libcamera::BoundMethodArgs \
>>>
>>>   EXCLUDE_SYMLINKS       = YES
>>>
>>> -HTML_OUTPUT            = api-html
>>> +HIDE_UNDOC_CLASSES     = @HIDE_UNDOC_CLASSES@
>>> +HIDE_UNDOC_MEMBERS     = @HIDE_UNDOC_MEMBERS@
>>> +HTML_OUTPUT            = @HTML_OUTPUT@
>>> +INTERNAL_DOCS          = @INTERNAL_DOCS@
>>>
>>>   GENERATE_LATEX         = NO
>>>
>>> diff --git a/Documentation/meson.build b/Documentation/meson.build
>>> index 7a58fec8..afaad751 100644
>>> --- a/Documentation/meson.build
>>> +++ b/Documentation/meson.build
>>> @@ -23,12 +23,7 @@ if doxygen.found() and dot.found()
>>>
>>>       cdata.set('PREDEFINED', ' \\\n\t\t\t '.join(doxygen_predefined))
>>>
>>> -    doxyfile = configure_file(input : 'Doxyfile.in',
>>> -                              output : 'Doxyfile',
>>> -                              configuration : cdata)
>>> -
>>> -    doxygen_input = [
>>> -        doxyfile,
>>> +    global_doxygen_input = [
>>>           libcamera_base_headers,
>>>           libcamera_base_sources,
>>>           libcamera_internal_headers,
>>> @@ -41,16 +36,79 @@ if doxygen.found() and dot.found()
>>>       ]
>>>
>>>       if is_variable('ipu3_ipa_sources')
>>> -        doxygen_input += [ipu3_ipa_sources]
>>> +        global_doxygen_input += [ipu3_ipa_sources]
>>>       endif
>>>
>>> +    # We need to generate two "include" files for the final Doxyfile which
>>> +    # define a set of source files to use in the documentation parsing. We
>>> +    # collected a list of the public sources in doxygen_public_sources, so we
>>> +    # pass that to the doxyfiles so that Doxyfile-public refers only to those
>>> +    # files. Although INPUT is sent to both, Doxyfile-internal.in doesn't refer
>>> +    # to it and just hardcodes the directories to parse.
>> Thanks, this is quite clear now!
>>
>>> +    cdata.set('INPUT', '" \\\n\t\t\t "'.join(doxygen_public_sources))
>>> +    doxyfile_include_public = configure_file(input : 'Doxyfile-public.in',
>>> +                                             output : 'Doxyfile-include-public',
>>> +                                             configuration : cdata)
>>> +    doxyfile_include_internal = configure_file(input : 'Doxyfile-internal.in',
>>> +                                               output : 'Doxyfile-include-internal',
>>> +                                               configuration : cdata)
>>> +
>>> +    # We run doxygen twice - the first run excludes internal API objects as it
>>> +    # is intended to document the public API only. A second run covers all of
>>> +    # the library's objects for libcamera developers. To achieve this we need to
>>> +    # flag as \internal some of the comments for objects which we wish to hide,
>>> +    # and remove the auto generated documents via HIDE_UNDOC_CLASSES and
>>> +    # HIDE_UNDOC_MEMBERS.
>>> +
>>> +    cdata_public = configuration_data()
>>> +    cdata_public.merge_from(cdata)
>>> +    cdata_public.set('HIDE_UNDOC_CLASSES', 'YES')
>>> +    cdata_public.set('HIDE_UNDOC_MEMBERS', 'YES')
>>> +    cdata_public.set('HTML_OUTPUT', 'api-html')
>>> +    cdata_public.set('INCLUDE_FILE', 'Doxyfile-include-public')
>>> +    cdata_public.set('INTERNAL_DOCS', 'NO')
>>> +
>>> +    doxyfile_public = configure_file(input : 'Doxyfile.in',
>>> +                                     output : 'Doxyfile-public',
>>> +                                     configuration : cdata_public)
>>> +
>>> +    public_doxygen_input = global_doxygen_input
>>> +    public_doxygen_input += doxyfile_public
>>> +
>>>       custom_target('doxygen',
>>> -                  input : doxygen_input,
>>> +                  input : public_doxygen_input,
>>>                     output : 'api-html',
>>> -                  command : [doxygen, doxyfile],
>>> +                  command : [doxygen, doxyfile_public],
>>>                     install : true,
>>>                     install_dir : doc_install_dir,
>>>                     install_tag : 'doc')
>>> +
>>> +    # This is the internal documentation, so _don't_ hide undocumented classes
>>> +    # as we want everything to show up and warnings to be generated if any
>>> +    # documentation is missing.
>>> +
>>> +    cdata_internal = configuration_data()
>>> +    cdata_internal.merge_from(cdata)
>>> +    cdata_internal.set('HIDE_UNDOC_CLASSES', 'NO')
>>> +    cdata_internal.set('HIDE_UNDOC_MEMBERS', 'NO')
>>> +    cdata_internal.set('HTML_OUTPUT', 'internal-api-html')
>>> +    cdata_internal.set('INCLUDE_FILE', 'Doxyfile-include-internal')
>>> +    cdata_internal.set('INTERNAL_DOCS', 'YES')
>>> +
>>> +    doxyfile_internal = configure_file(input : 'Doxyfile.in',
>>> +                                       output : 'Doxyfile-internal',
>>> +                                       configuration : cdata_internal)
>>> +
>>> +    internal_doxygen_input = global_doxygen_input
>>> +    internal_doxygen_input += doxyfile_internal
>>> +
>>> +    custom_target('doxygen-internal',
>>> +                  input : internal_doxygen_input,
>>> +                  output : 'internal-api-html',
>>> +                  command : [doxygen, doxyfile_internal],
>>> +                  install : true,
>>> +                  install_dir : doc_install_dir,
>>> +                  install_tag : 'doc-internal')
>>>   endif
>>>
>>>   #
>>> diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build
>>> index f24f47de..82277f46 100644
>>> --- a/include/libcamera/base/meson.build
>>> +++ b/include/libcamera/base/meson.build
>>> @@ -38,3 +38,10 @@ libcamera_base_headers = [
>>>
>>>   install_headers(libcamera_base_public_headers,
>>>                   subdir : libcamera_base_include_dir)
>>> +
>>> +foreach lbph : libcamera_base_public_headers
>>> +     doxygen_public_sources += '/'.join(
>>> +             meson.project_source_root(),
>>> +             '@0@'.format(lbph)
>>> +     )
>>> +endforeach
>>> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
>>> index 4d59cb2a..4af36884 100644
>>> --- a/include/libcamera/internal/meson.build
>>> +++ b/include/libcamera/internal/meson.build
>>> @@ -58,4 +58,11 @@ libcamera_internal_headers = [
>>>       libcamera_internal_headers_publically_undocumented
>>>   ]
>>>
>>> +foreach lph : libcamera_internal_headers_publically_documented
> I think I saw a suggestion somewhere to drop _publically anyway, but
> otherwise I think the correct spelling is 'publicly'.
Derp - thanks!
>
>>> +     doxygen_public_sources += '/'.join(
>>> +             meson.project_source_root(),
>>> +             '@0@'.format(lph)
>>> +     )
>>> +endforeach
>>> +
>>>   subdir('converter')
>>> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
>>> index bab858a3..25e2f8a4 100644
>>> --- a/include/libcamera/meson.build
>>> +++ b/include/libcamera/meson.build
>>> @@ -26,6 +26,13 @@ subdir('ipa')
>>>   install_headers(libcamera_public_headers,
>>>                   subdir : libcamera_include_dir)
>>>
>>> +foreach lph : libcamera_public_headers
>>> +     doxygen_public_sources += '/'.join(
>>> +             meson.project_source_root(),
>>> +             '@0@'.format(lph)
>>> +     )
> I'm surprised meson doesn't have better helpers for this.
> It might be worth a check through with meson to see if there's a cleaner
> way to handle this. But otherwise, if this is the only way to get the
> fully qualified paths if that's what's needed...


It does; File.full_path() [1], but the problem is that it wasn't added until Meson 1.4.0 and Laurent 
was concerned that's too recent - quite reasonably, apparently, since my Ubuntu meson package only 
gives me 0.61.2. This method is apparently available back to 0.54.0


[1] https://mesonbuild.com/Reference-manual_returned_file.html

>
>>> +endforeach
>>> +
>>>   #
>>>   # Generate headers from templates.
>>>   #
>>> @@ -85,6 +92,7 @@ foreach mode, entry : controls_map
>>>                                                   '-r', ranges_file, '@INPUT@'],
>>>                                        install : true,
>>>                                        install_dir : libcamera_headers_install_dir)
>>> +    doxygen_public_sources += control_headers.get(-1).full_path()
> What's going on here? (Why a .get(-1) ?)


We're within a foreach loop that adds a CustomTarget to the control_headers array, but doesn't 
create a new variable for it, so to use it I'm just fetching the last entry from the array. A fuller 
context for that change would be:


     control_headers += custom_target(header + '_h',
                                      input : input_files,
                                      output : outfile,
                                      command : [gen_controls, '-o', '@OUTPUT@',
                                                 '--mode', mode, '-t', template_file,
                                                 '-r', ranges_file, '@INPUT@'],
                                      install : true,
                                      install_dir : libcamera_headers_install_dir)
     doxygen_public_sources += control_headers.get(-1).full_path()


Alternatively I could create a local variable for the custom target and use that, like so:


     ct = custom_target(header + '_h',
                        input : input_files,
                        output : outfile,
                        command : [gen_controls, '-o', '@OUTPUT@',
                                   '--mode', mode, '-t', template_file,
                                   '-r', ranges_file, '@INPUT@'],
                        install : true,
                        install_dir : libcamera_headers_install_dir)
     control_headers += ct
     doxygen_public_sources += ct.full_path()


But it didn't seem worth it.



>
>>>   endforeach
>>>
>>>   libcamera_public_headers += control_headers
>>> @@ -101,6 +109,7 @@ formats_h = custom_target('formats_h',
>>>                             install : true,
>>>                             install_dir : libcamera_headers_install_dir)
>>>   libcamera_public_headers += formats_h
>>> +doxygen_public_sources += formats_h.full_path()
>>>
>>>   # libcamera.h
>>>   libcamera_h = custom_target('gen-header',
>>> @@ -111,6 +120,7 @@ libcamera_h = custom_target('gen-header',
>>>                               install_dir : libcamera_headers_install_dir)
>>>
>>>   libcamera_public_headers += libcamera_h
>>> +doxygen_public_sources += libcamera_h.full_path()
>>>
>>>   # version.h
>>>   version = libcamera_version.split('.')
>>> diff --git a/meson.build b/meson.build
>>> index e49de4c2..cca1883e 100644
>>> --- a/meson.build
>>> +++ b/meson.build
>>> @@ -231,6 +231,14 @@ endif
>>>   # Utilities are parsed first to provide support for other components.
>>>   subdir('utils')
>>>
>>> +# To support auto-generation of documentation We need an array of the paths to
>> s/We/we
>>
>>> +# public headers and source files so that we can tell doxygen which files to
>>> +# look at later. Unfortunately the inclusion of custom targets in some of the
>>> +# existing arrays precludes using them directly and you cannot generate File
>>> +# objects from generated files, so we need to collect paths to relevant files
>>> +# within an array.
>>> +doxygen_public_sources = []
> Aha, that might have answered my question above.
>
>>> +
>>>   subdir('include')
>>>   subdir('src')
>>>
>>> diff --git a/src/libcamera/base/class.cpp b/src/libcamera/base/class.cpp
>>> index 9c2d9f21..70fd5cd5 100644
>>> --- a/src/libcamera/base/class.cpp
>>> +++ b/src/libcamera/base/class.cpp
>>> @@ -184,6 +184,7 @@ Extensible::Extensible(std::unique_ptr<Extensible::Private> d)
>>>    */
>>>
>>>   /**
>>> + * \internal
>>>    * \class Extensible::Private
>>>    * \brief Base class for private data managed through a d-pointer
>>>    */
>>> diff --git a/src/libcamera/base/meson.build b/src/libcamera/base/meson.build
>>> index 523c5885..ae949a51 100644
>>> --- a/src/libcamera/base/meson.build
>>> +++ b/src/libcamera/base/meson.build
>>> @@ -30,6 +30,13 @@ libcamera_base_sources = [
>>>        libcamera_base_internal_sources
>>>   ]
>>>
>>> +foreach lbps : libcamera_base_public_sources
>>> +     doxygen_public_sources += '/'.join(
>>> +             meson.project_source_root(),
>>> +             '@0@'.format(lbps)
>>> +     )
>>> +endforeach
>>> +
>>>   libdw = dependency('libdw', required : false)
>>>   libunwind = dependency('libunwind', required : false)
>>>
>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
>>> index 0ad1a4b5..ed46f853 100644
>>> --- a/src/libcamera/camera.cpp
>>> +++ b/src/libcamera/camera.cpp
>>> @@ -560,6 +560,13 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF
>>>    */
>>>
>>>   /**
>>> + * \internal
>>> + * \file libcamera\internal\camera.h
>>> + * \brief Internal Camera device handling
>>> + */
>>> +
>>> +/**
>>> + * \internal
>>>    * \class Camera::Private
>>>    * \brief Base class for camera private data
>>>    *
>>> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
>>> index 355f3ada..61d45256 100644
>>> --- a/src/libcamera/camera_manager.cpp
>>> +++ b/src/libcamera/camera_manager.cpp
>>> @@ -23,6 +23,7 @@
>>>    */
>>>
>>>   /**
>>> + * \internal
>>>    * \file libcamera/internal/camera_manager.h
>>>    * \brief Internal camera manager support
>>>    */
>> That's weird, I don't see CameraManager::Private::addCamera() and
>> CameraManager::Private::removeCamera() being documented. It was not
>> documented even before this commit though :/
>>
>>> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
>>> index 5a7f3c0b..db450e11 100644
>>> --- a/src/libcamera/framebuffer.cpp
>>> +++ b/src/libcamera/framebuffer.cpp
>>> @@ -16,7 +16,10 @@
>>>   /**
>>>    * \file libcamera/framebuffer.h
>>>    * \brief Frame buffer handling
>>> - *
>>> + */
>>> +
>>> +/**
>>> + * \internal
>>>    * \file libcamera/internal/framebuffer.h
>>>    * \brief Internal frame buffer handling support
>>>    */
>>> @@ -105,6 +108,7 @@ LOG_DEFINE_CATEGORY(Buffer)
>>>    */
>>>
>>>   /**
>>> + * \internal
>>>    * \class FrameBuffer::Private
>>>    * \brief Base class for FrameBuffer private data
>>>    *
>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
>>> index 676470c1..413e14db 100644
>>> --- a/src/libcamera/meson.build
>>> +++ b/src/libcamera/meson.build
>>> @@ -59,6 +59,13 @@ libcamera_sources = [
>>>        libcamera_internal_sources
>>>   ]
>>>
>>> +foreach lps : libcamera_public_sources
>>> +     doxygen_public_sources += '/'.join(
>>> +             meson.project_source_root(),
>>> +             '@0@'.format(lps)
>>> +     )
>>> +endforeach
>>> +
>>>   libcamera_sources += libcamera_public_headers
>>>   libcamera_sources += libcamera_generated_ipa_headers
>>>   libcamera_sources += libcamera_tracepoint_header
>>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
>>> index 949c556f..930d8c92 100644
>>> --- a/src/libcamera/request.cpp
>>> +++ b/src/libcamera/request.cpp
>>> @@ -33,6 +33,7 @@ namespace libcamera {
>>>   LOG_DEFINE_CATEGORY(Request)
>>>
>>>   /**
>>> + * \internal
>>>    * \class Request::Private
>>>    * \brief Request private data
>>>    *
>> So we should remember to mark every classes that derives a private
>> implementation from Extensible to mark it as \internal. I think it's
>> fine, and I like the patch  a lot!
>>
> Likewise, I suspect we can try to come up with a checkstyle reminder
> sometime.
Yeah I was going to take a look at that when time allowed
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


Thanks!

>
>> With the few above points clarified
>> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>>
>> Thanks
>>     j
>>
>>> --
>>> 2.34.1
>>>

Hi Jacopo

On 09/01/2024 14:28, Jacopo Mondi wrote:
> Hi Dan
>    I really like the split!
>
> On Fri, Jan 05, 2024 at 04:41:03PM +0000, Daniel Scally via libcamera-devel wrote:
>> The API reference pages generated by Doxygen are comprehensive, but
>> therefore quite overwhelming for application developers who will
>> likely never need to use the majority of the library's objects. To
>> reduce the complexity of the documentation, split it into two runs of
>> doxygen.
>>
>> In the first run of doxygen we pass a specific list of source files
>> to parse, which is built from the File arrays in Meson's build files.
>> This ensures that we only generate the documentation for code from
>> those files.
>>
>> In the second run allow doxygen to generate documentation for all of
>> the library's objects as it currently does. This set will now be
>> output into build/Documentation/internal-api-html.
>>
>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>> ---
>> Changes in v2:
>>
>> 	- Formatting fixes (Jacopo)
>> 	- Phraseology (Laurent)
>> 	- Switched to passing specific files to parse to doxygen rather than
>> 	  relying on \internal to remove other docu-comments.
>>
>>   Documentation/Doxyfile-internal.in     | 21 +++++++
>>   Documentation/Doxyfile-public.in       |  5 ++
>>   Documentation/Doxyfile.in              | 26 ++-------
>>   Documentation/meson.build              | 76 +++++++++++++++++++++++---
>>   include/libcamera/base/meson.build     |  7 +++
>>   include/libcamera/internal/meson.build |  7 +++
>>   include/libcamera/meson.build          | 10 ++++
>>   meson.build                            |  8 +++
>>   src/libcamera/base/class.cpp           |  1 +
>>   src/libcamera/base/meson.build         |  7 +++
>>   src/libcamera/camera.cpp               |  7 +++
>>   src/libcamera/camera_manager.cpp       |  1 +
>>   src/libcamera/framebuffer.cpp          |  6 +-
>>   src/libcamera/meson.build              |  7 +++
>>   src/libcamera/request.cpp              |  1 +
>>   15 files changed, 160 insertions(+), 30 deletions(-)
>>   create mode 100644 Documentation/Doxyfile-internal.in
>>   create mode 100644 Documentation/Doxyfile-public.in
>>
>> diff --git a/Documentation/Doxyfile-internal.in b/Documentation/Doxyfile-internal.in
>> new file mode 100644
>> index 00000000..7b3cce49
>> --- /dev/null
>> +++ b/Documentation/Doxyfile-internal.in
>> @@ -0,0 +1,21 @@
>> +# SPDX-License-Identifier: CC-BY-SA-4.0
>> +
>> +INPUT                  = "@TOP_SRCDIR@/Documentation" \
>> +                         "@TOP_SRCDIR@/include/libcamera" \
>> +                         "@TOP_SRCDIR@/src/ipa/ipu3" \
>> +                         "@TOP_SRCDIR@/src/ipa/libipa" \
>> +                         "@TOP_SRCDIR@/src/libcamera" \
>> +                         "@TOP_BUILDDIR@/include/libcamera" \
>> +                         "@TOP_BUILDDIR@/src/libcamera"
>> +
>> +EXCLUDE                = @TOP_SRCDIR@/include/libcamera/base/span.h \
> I'm still a bit puzzled on why we don't document Span<>, but this was
> already here
>
>> +                         @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_sysfs.h \
>> +                         @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_udev.h \
>> +                         @TOP_SRCDIR@/include/libcamera/internal/ipc_pipe_unixsocket.h \
>> +                         @TOP_SRCDIR@/src/libcamera/device_enumerator_sysfs.cpp \
>> +                         @TOP_SRCDIR@/src/libcamera/device_enumerator_udev.cpp \
>> +                         @TOP_SRCDIR@/src/libcamera/ipc_pipe_unixsocket.cpp \
>> +                         @TOP_SRCDIR@/src/libcamera/pipeline/ \
>> +                         @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \
>> +                         @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \
>> +                         @TOP_BUILDDIR@/src/libcamera/proxy/
>> diff --git a/Documentation/Doxyfile-public.in b/Documentation/Doxyfile-public.in
>> new file mode 100644
>> index 00000000..cdbc03a0
>> --- /dev/null
>> +++ b/Documentation/Doxyfile-public.in
>> @@ -0,0 +1,5 @@
>> +# SPDX-License-Identifier: CC-BY-SA-4.0
>> +
>> +INPUT                  = "@TOP_SRCDIR@/Documentation" \
>> +                         "@INPUT@" \
>> +                         "@TOP_BUILDDIR@/src/libcamera/version.cpp"
>> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
>> index 48fea8bc..a271c7bc 100644
>> --- a/Documentation/Doxyfile.in
>> +++ b/Documentation/Doxyfile.in
>> @@ -21,14 +21,6 @@ CASE_SENSE_NAMES       = YES
>>
>>   QUIET                  = YES
>>
>> -INPUT                  = "@TOP_SRCDIR@/Documentation" \
>> -                         "@TOP_SRCDIR@/include/libcamera" \
>> -                         "@TOP_SRCDIR@/src/ipa/ipu3" \
>> -                         "@TOP_SRCDIR@/src/ipa/libipa" \
>> -                         "@TOP_SRCDIR@/src/libcamera" \
>> -                         "@TOP_BUILDDIR@/include/libcamera" \
>> -                         "@TOP_BUILDDIR@/src/libcamera"
>> -
>>   FILE_PATTERNS          = *.c \
>>                            *.cpp \
>>                            *.h \
>> @@ -36,17 +28,8 @@ FILE_PATTERNS          = *.c \
>>
>>   RECURSIVE              = YES
>>
>> -EXCLUDE                = @TOP_SRCDIR@/include/libcamera/base/span.h \
>> -                         @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_sysfs.h \
>> -                         @TOP_SRCDIR@/include/libcamera/internal/device_enumerator_udev.h \
>> -                         @TOP_SRCDIR@/include/libcamera/internal/ipc_pipe_unixsocket.h \
>> -                         @TOP_SRCDIR@/src/libcamera/device_enumerator_sysfs.cpp \
>> -                         @TOP_SRCDIR@/src/libcamera/device_enumerator_udev.cpp \
>> -                         @TOP_SRCDIR@/src/libcamera/ipc_pipe_unixsocket.cpp \
>> -                         @TOP_SRCDIR@/src/libcamera/pipeline/ \
>> -                         @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \
>> -                         @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \
>> -                         @TOP_BUILDDIR@/src/libcamera/proxy/
>> +@INCLUDE_PATH          = @TOP_BUILDDIR@/Documentation
>> +@INCLUDE               = @INCLUDE_FILE@
>>
>>   EXCLUDE_PATTERNS       = @TOP_BUILDDIR@/include/libcamera/ipa/*_serializer.h \
>>                            @TOP_BUILDDIR@/include/libcamera/ipa/*_proxy.h \
>> @@ -70,7 +53,10 @@ EXCLUDE_SYMBOLS        = libcamera::BoundMethodArgs \
>>
>>   EXCLUDE_SYMLINKS       = YES
>>
>> -HTML_OUTPUT            = api-html
>> +HIDE_UNDOC_CLASSES     = @HIDE_UNDOC_CLASSES@
>> +HIDE_UNDOC_MEMBERS     = @HIDE_UNDOC_MEMBERS@
>> +HTML_OUTPUT            = @HTML_OUTPUT@
>> +INTERNAL_DOCS          = @INTERNAL_DOCS@
>>
>>   GENERATE_LATEX         = NO
>>
>> diff --git a/Documentation/meson.build b/Documentation/meson.build
>> index 7a58fec8..afaad751 100644
>> --- a/Documentation/meson.build
>> +++ b/Documentation/meson.build
>> @@ -23,12 +23,7 @@ if doxygen.found() and dot.found()
>>
>>       cdata.set('PREDEFINED', ' \\\n\t\t\t '.join(doxygen_predefined))
>>
>> -    doxyfile = configure_file(input : 'Doxyfile.in',
>> -                              output : 'Doxyfile',
>> -                              configuration : cdata)
>> -
>> -    doxygen_input = [
>> -        doxyfile,
>> +    global_doxygen_input = [
>>           libcamera_base_headers,
>>           libcamera_base_sources,
>>           libcamera_internal_headers,
>> @@ -41,16 +36,79 @@ if doxygen.found() and dot.found()
>>       ]
>>
>>       if is_variable('ipu3_ipa_sources')
>> -        doxygen_input += [ipu3_ipa_sources]
>> +        global_doxygen_input += [ipu3_ipa_sources]
>>       endif
>>
>> +    # We need to generate two "include" files for the final Doxyfile which
>> +    # define a set of source files to use in the documentation parsing. We
>> +    # collected a list of the public sources in doxygen_public_sources, so we
>> +    # pass that to the doxyfiles so that Doxyfile-public refers only to those
>> +    # files. Although INPUT is sent to both, Doxyfile-internal.in doesn't refer
>> +    # to it and just hardcodes the directories to parse.
> Thanks, this is quite clear now!
>
>> +    cdata.set('INPUT', '" \\\n\t\t\t "'.join(doxygen_public_sources))
>> +    doxyfile_include_public = configure_file(input : 'Doxyfile-public.in',
>> +                                             output : 'Doxyfile-include-public',
>> +                                             configuration : cdata)
>> +    doxyfile_include_internal = configure_file(input : 'Doxyfile-internal.in',
>> +                                               output : 'Doxyfile-include-internal',
>> +                                               configuration : cdata)
>> +
>> +    # We run doxygen twice - the first run excludes internal API objects as it
>> +    # is intended to document the public API only. A second run covers all of
>> +    # the library's objects for libcamera developers. To achieve this we need to
>> +    # flag as \internal some of the comments for objects which we wish to hide,
>> +    # and remove the auto generated documents via HIDE_UNDOC_CLASSES and
>> +    # HIDE_UNDOC_MEMBERS.
>> +
>> +    cdata_public = configuration_data()
>> +    cdata_public.merge_from(cdata)
>> +    cdata_public.set('HIDE_UNDOC_CLASSES', 'YES')
>> +    cdata_public.set('HIDE_UNDOC_MEMBERS', 'YES')
>> +    cdata_public.set('HTML_OUTPUT', 'api-html')
>> +    cdata_public.set('INCLUDE_FILE', 'Doxyfile-include-public')
>> +    cdata_public.set('INTERNAL_DOCS', 'NO')
>> +
>> +    doxyfile_public = configure_file(input : 'Doxyfile.in',
>> +                                     output : 'Doxyfile-public',
>> +                                     configuration : cdata_public)
>> +
>> +    public_doxygen_input = global_doxygen_input
>> +    public_doxygen_input += doxyfile_public
>> +
>>       custom_target('doxygen',
>> -                  input : doxygen_input,
>> +                  input : public_doxygen_input,
>>                     output : 'api-html',
>> -                  command : [doxygen, doxyfile],
>> +                  command : [doxygen, doxyfile_public],
>>                     install : true,
>>                     install_dir : doc_install_dir,
>>                     install_tag : 'doc')
>> +
>> +    # This is the internal documentation, so _don't_ hide undocumented classes
>> +    # as we want everything to show up and warnings to be generated if any
>> +    # documentation is missing.
>> +
>> +    cdata_internal = configuration_data()
>> +    cdata_internal.merge_from(cdata)
>> +    cdata_internal.set('HIDE_UNDOC_CLASSES', 'NO')
>> +    cdata_internal.set('HIDE_UNDOC_MEMBERS', 'NO')
>> +    cdata_internal.set('HTML_OUTPUT', 'internal-api-html')
>> +    cdata_internal.set('INCLUDE_FILE', 'Doxyfile-include-internal')
>> +    cdata_internal.set('INTERNAL_DOCS', 'YES')
>> +
>> +    doxyfile_internal = configure_file(input : 'Doxyfile.in',
>> +                                       output : 'Doxyfile-internal',
>> +                                       configuration : cdata_internal)
>> +
>> +    internal_doxygen_input = global_doxygen_input
>> +    internal_doxygen_input += doxyfile_internal
>> +
>> +    custom_target('doxygen-internal',
>> +                  input : internal_doxygen_input,
>> +                  output : 'internal-api-html',
>> +                  command : [doxygen, doxyfile_internal],
>> +                  install : true,
>> +                  install_dir : doc_install_dir,
>> +                  install_tag : 'doc-internal')
>>   endif
>>
>>   #
>> diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build
>> index f24f47de..82277f46 100644
>> --- a/include/libcamera/base/meson.build
>> +++ b/include/libcamera/base/meson.build
>> @@ -38,3 +38,10 @@ libcamera_base_headers = [
>>
>>   install_headers(libcamera_base_public_headers,
>>                   subdir : libcamera_base_include_dir)
>> +
>> +foreach lbph : libcamera_base_public_headers
>> +	doxygen_public_sources += '/'.join(
>> +		meson.project_source_root(),
>> +		'@0@'.format(lbph)
>> +	)
>> +endforeach
>> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
>> index 4d59cb2a..4af36884 100644
>> --- a/include/libcamera/internal/meson.build
>> +++ b/include/libcamera/internal/meson.build
>> @@ -58,4 +58,11 @@ libcamera_internal_headers = [
>>       libcamera_internal_headers_publically_undocumented
>>   ]
>>
>> +foreach lph : libcamera_internal_headers_publically_documented
>> +	doxygen_public_sources += '/'.join(
>> +		meson.project_source_root(),
>> +		'@0@'.format(lph)
>> +	)
>> +endforeach
>> +
>>   subdir('converter')
>> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
>> index bab858a3..25e2f8a4 100644
>> --- a/include/libcamera/meson.build
>> +++ b/include/libcamera/meson.build
>> @@ -26,6 +26,13 @@ subdir('ipa')
>>   install_headers(libcamera_public_headers,
>>                   subdir : libcamera_include_dir)
>>
>> +foreach lph : libcamera_public_headers
>> +	doxygen_public_sources += '/'.join(
>> +		meson.project_source_root(),
>> +		'@0@'.format(lph)
>> +	)
>> +endforeach
>> +
>>   #
>>   # Generate headers from templates.
>>   #
>> @@ -85,6 +92,7 @@ foreach mode, entry : controls_map
>>                                                   '-r', ranges_file, '@INPUT@'],
>>                                        install : true,
>>                                        install_dir : libcamera_headers_install_dir)
>> +    doxygen_public_sources += control_headers.get(-1).full_path()
>>   endforeach
>>
>>   libcamera_public_headers += control_headers
>> @@ -101,6 +109,7 @@ formats_h = custom_target('formats_h',
>>                             install : true,
>>                             install_dir : libcamera_headers_install_dir)
>>   libcamera_public_headers += formats_h
>> +doxygen_public_sources += formats_h.full_path()
>>
>>   # libcamera.h
>>   libcamera_h = custom_target('gen-header',
>> @@ -111,6 +120,7 @@ libcamera_h = custom_target('gen-header',
>>                               install_dir : libcamera_headers_install_dir)
>>
>>   libcamera_public_headers += libcamera_h
>> +doxygen_public_sources += libcamera_h.full_path()
>>
>>   # version.h
>>   version = libcamera_version.split('.')
>> diff --git a/meson.build b/meson.build
>> index e49de4c2..cca1883e 100644
>> --- a/meson.build
>> +++ b/meson.build
>> @@ -231,6 +231,14 @@ endif
>>   # Utilities are parsed first to provide support for other components.
>>   subdir('utils')
>>
>> +# To support auto-generation of documentation We need an array of the paths to
> s/We/we
>
>> +# public headers and source files so that we can tell doxygen which files to
>> +# look at later. Unfortunately the inclusion of custom targets in some of the
>> +# existing arrays precludes using them directly and you cannot generate File
>> +# objects from generated files, so we need to collect paths to relevant files
>> +# within an array.
>> +doxygen_public_sources = []
>> +
>>   subdir('include')
>>   subdir('src')
>>
>> diff --git a/src/libcamera/base/class.cpp b/src/libcamera/base/class.cpp
>> index 9c2d9f21..70fd5cd5 100644
>> --- a/src/libcamera/base/class.cpp
>> +++ b/src/libcamera/base/class.cpp
>> @@ -184,6 +184,7 @@ Extensible::Extensible(std::unique_ptr<Extensible::Private> d)
>>    */
>>
>>   /**
>> + * \internal
>>    * \class Extensible::Private
>>    * \brief Base class for private data managed through a d-pointer
>>    */
>> diff --git a/src/libcamera/base/meson.build b/src/libcamera/base/meson.build
>> index 523c5885..ae949a51 100644
>> --- a/src/libcamera/base/meson.build
>> +++ b/src/libcamera/base/meson.build
>> @@ -30,6 +30,13 @@ libcamera_base_sources = [
>>   	libcamera_base_internal_sources
>>   ]
>>
>> +foreach lbps : libcamera_base_public_sources
>> +	doxygen_public_sources += '/'.join(
>> +		meson.project_source_root(),
>> +		'@0@'.format(lbps)
>> +	)
>> +endforeach
>> +
>>   libdw = dependency('libdw', required : false)
>>   libunwind = dependency('libunwind', required : false)
>>
>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
>> index 0ad1a4b5..ed46f853 100644
>> --- a/src/libcamera/camera.cpp
>> +++ b/src/libcamera/camera.cpp
>> @@ -560,6 +560,13 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF
>>    */
>>
>>   /**
>> + * \internal
>> + * \file libcamera\internal\camera.h
>> + * \brief Internal Camera device handling
>> + */
>> +
>> +/**
>> + * \internal
>>    * \class Camera::Private
>>    * \brief Base class for camera private data
>>    *
>> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
>> index 355f3ada..61d45256 100644
>> --- a/src/libcamera/camera_manager.cpp
>> +++ b/src/libcamera/camera_manager.cpp
>> @@ -23,6 +23,7 @@
>>    */
>>
>>   /**
>> + * \internal
>>    * \file libcamera/internal/camera_manager.h
>>    * \brief Internal camera manager support
>>    */
> That's weird, I don't see CameraManager::Private::addCamera() and
> CameraManager::Private::removeCamera() being documented. It was not
> documented even before this commit though :/


Turns out CameraManager::Private is actually explicitly excluded at the symbol level in 
Documentation/Doxyfile.in, so that's why. So; probably unecessary to flag these...but perhaps for 
consistency's sake it's best to follow the rule that any public class deriving a private from 
::Extensible needs flagging.

>
>> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
>> index 5a7f3c0b..db450e11 100644
>> --- a/src/libcamera/framebuffer.cpp
>> +++ b/src/libcamera/framebuffer.cpp
>> @@ -16,7 +16,10 @@
>>   /**
>>    * \file libcamera/framebuffer.h
>>    * \brief Frame buffer handling
>> - *
>> + */
>> +
>> +/**
>> + * \internal
>>    * \file libcamera/internal/framebuffer.h
>>    * \brief Internal frame buffer handling support
>>    */
>> @@ -105,6 +108,7 @@ LOG_DEFINE_CATEGORY(Buffer)
>>    */
>>
>>   /**
>> + * \internal
>>    * \class FrameBuffer::Private
>>    * \brief Base class for FrameBuffer private data
>>    *
>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
>> index 676470c1..413e14db 100644
>> --- a/src/libcamera/meson.build
>> +++ b/src/libcamera/meson.build
>> @@ -59,6 +59,13 @@ libcamera_sources = [
>>   	libcamera_internal_sources
>>   ]
>>
>> +foreach lps : libcamera_public_sources
>> +	doxygen_public_sources += '/'.join(
>> +		meson.project_source_root(),
>> +		'@0@'.format(lps)
>> +	)
>> +endforeach
>> +
>>   libcamera_sources += libcamera_public_headers
>>   libcamera_sources += libcamera_generated_ipa_headers
>>   libcamera_sources += libcamera_tracepoint_header
>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
>> index 949c556f..930d8c92 100644
>> --- a/src/libcamera/request.cpp
>> +++ b/src/libcamera/request.cpp
>> @@ -33,6 +33,7 @@ namespace libcamera {
>>   LOG_DEFINE_CATEGORY(Request)
>>
>>   /**
>> + * \internal
>>    * \class Request::Private
>>    * \brief Request private data
>>    *
> So we should remember to mark every classes that derives a private
> implementation from Extensible to mark it as \internal. I think it's
> fine, and I like the patch  a lot!
>
> With the few above points clarified
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>
> Thanks
>     j
>
>> --
>> 2.34.1
>>