[libcamera-devel] [PATCH v2 4/5] libcamera: Documentation: Split public/private documentation
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Jan 9 23:08:53 CET 2024
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 at 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/
> > + at INCLUDE_PATH = @TOP_BUILDDIR@/Documentation
> > + at 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 at ideasonboard.com>
> With the few above points clarified
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
>
> Thanks
> j
>
> > --
> > 2.34.1
> >
More information about the libcamera-devel
mailing list