[libcamera-devel] [PATCH 2/2] libcamera: Documentation: Split public/private documentation
Dan Scally
dan.scally at ideasonboard.com
Fri Jan 5 14:54:24 CET 2024
On 05/01/2024 11:37, Kieran Bingham wrote:
> Quoting Dan Scally via libcamera-devel (2024-01-05 10:52:01)
>> On 26/12/2023 22:59, Laurent Pinchart via libcamera-devel wrote:
>>> On Tue, Dec 26, 2023 at 05:24:37PM +0200, Laurent Pinchart via libcamera-devel wrote:
>>>> On Tue, Dec 26, 2023 at 11:41:35AM +0100, Jacopo Mondi via libcamera-devel wrote:
>>>>> Hi Dan
>>>>> thanks for this big chunk of work
>>>>>
>>>>> On Wed, Dec 20, 2023 at 04:31:44PM +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
>>>> More than likely, it's outright forbidden :-) You can write
>>>>
>>>> "who will only use the public API objects."
>>>>
>>>>>> reduce the complexity of the documentation, split it into two runs of
>>>>>> doxygen. In the first run leverage doxygen's \internal and \if flags
>>>>>> to hide library objects which are not intended to be exposed through
>>>>>> the public headers.
>>>> "to hide internal library objects" is enough.
>>>>
>>>>>> 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>
>>>>>> ---
>>>>>> This patch touches a lot of files, but almost exclusively adds doxygen's
>>>>>> \internal flag to docu-comments along with the necessary changes to Doxyfile.in
>>>>>> and meson.build to implement the hiding of those objects in the auto-generated
>>>>>> documentation.
>>>>>>
>>>>>> The deviation from that rule is in object.cpp, where reference is made to
>>>>>> the "internal" Thread class documentation. Because the Object class is retained
>>>>>> in the public documents we can't remove the whole thing with \internal which
>>>>>> means resorting to \if .. \endif. It's specifically a reference to the Stopping
>>>>>> Threads section, and an alternative means of resolving this might be to move
>>>>>> that whole section to the Thread Support _page_ instead of being part of the
>>>>>> Thread class.
>>>> I think you can use
>>>>
>>>> * \internal
>>>> * ...
>>>> * \endinternal
>>>>
>>>> instead of '\if internal' in object.cpp.
>>>>
>>>>>> Documentation/Doxyfile.in | 9 +++-
>>>>>> Documentation/meson.build | 47 +++++++++++++++++--
>>>>>> include/libcamera/ipa/core.mojom | 5 ++
>>>>>> src/ipa/ipu3/algorithms/af.cpp | 2 +
>>>>>> src/ipa/ipu3/algorithms/agc.cpp | 2 +
>>>>>> src/ipa/ipu3/algorithms/awb.cpp | 4 ++
>>>>>> src/ipa/ipu3/algorithms/blc.cpp | 2 +
>>>>>> src/ipa/ipu3/algorithms/tone_mapping.cpp | 2 +
>>>>>> src/ipa/ipu3/ipa_context.cpp | 5 ++
>>>>>> src/ipa/ipu3/ipu3.cpp | 1 +
>>>>>> src/ipa/libipa/algorithm.cpp | 3 ++
>>>>>> src/ipa/libipa/camera_sensor_helper.cpp | 8 ++++
>>>>>> src/ipa/libipa/fc_queue.cpp | 3 ++
>>>>>> src/ipa/libipa/histogram.cpp | 2 +
>>>>>> src/ipa/libipa/module.cpp | 2 +
>>>>>> src/ipa/rkisp1/algorithms/agc.cpp | 2 +
>>>>>> src/ipa/rkisp1/algorithms/awb.cpp | 2 +
>>>>>> src/ipa/rkisp1/algorithms/blc.cpp | 2 +
>>>>>> src/ipa/rkisp1/algorithms/cproc.cpp | 2 +
>>>>>> src/ipa/rkisp1/algorithms/dpcc.cpp | 2 +
>>>>>> src/ipa/rkisp1/algorithms/dpf.cpp | 2 +
>>>>>> src/ipa/rkisp1/algorithms/filter.cpp | 2 +
>>>>>> src/ipa/rkisp1/algorithms/gsl.cpp | 2 +
>>>>>> src/ipa/rkisp1/algorithms/lsc.cpp | 2 +
>>>>>> src/ipa/rkisp1/ipa_context.cpp | 13 +++++
>>>>>> src/libcamera/base/backtrace.cpp | 2 +
>>>>>> src/libcamera/base/event_dispatcher.cpp | 2 +
>>>>>> src/libcamera/base/event_dispatcher_poll.cpp | 2 +
>>>>>> src/libcamera/base/event_notifier.cpp | 3 ++
>>>>>> src/libcamera/base/file.cpp | 4 ++
>>>>>> src/libcamera/base/log.cpp | 7 +++
>>>>>> src/libcamera/base/message.cpp | 4 ++
>>>>>> src/libcamera/base/mutex.cpp | 4 ++
>>>>>> src/libcamera/base/object.cpp | 13 +++--
>>>>>> src/libcamera/base/semaphore.cpp | 2 +
>>>>>> src/libcamera/base/thread.cpp | 5 ++
>>>>>> src/libcamera/base/timer.cpp | 2 +
>>>>>> src/libcamera/base/utils.cpp | 2 +
>>>>>> src/libcamera/bayer_format.cpp | 4 ++
>>>>>> src/libcamera/byte_stream_buffer.cpp | 2 +
>>>>>> src/libcamera/camera.cpp | 1 +
>>>>>> src/libcamera/camera_controls.cpp | 2 +
>>>>>> src/libcamera/camera_lens.cpp | 2 +
>>>>>> src/libcamera/camera_manager.cpp | 1 +
>>>>>> src/libcamera/camera_sensor.cpp | 2 +
>>>>>> src/libcamera/camera_sensor_properties.cpp | 2 +
>>>>>> src/libcamera/control_serializer.cpp | 3 ++
>>>>>> src/libcamera/control_validator.cpp | 2 +
>>>>>> src/libcamera/converter.cpp | 4 ++
>>>>>> .../converter/converter_v4l2_m2m.cpp | 2 +
>>>>>> src/libcamera/delayed_controls.cpp | 3 ++
>>>>>> src/libcamera/device_enumerator.cpp | 3 ++
>>>>>> src/libcamera/framebuffer.cpp | 2 +
>>>>>> src/libcamera/ipa_controls.cpp | 4 ++
>>>>>> src/libcamera/ipa_data_serializer.cpp | 2 +
>>>>>> src/libcamera/ipa_interface.cpp | 2 +
>>>>>> src/libcamera/ipa_manager.cpp | 2 +
>>>>>> src/libcamera/ipa_module.cpp | 4 ++
>>>>>> src/libcamera/ipa_proxy.cpp | 3 ++
>>>>>> src/libcamera/ipc_pipe.cpp | 4 ++
>>>>>> src/libcamera/ipc_unixsocket.cpp | 3 ++
>>>>>> src/libcamera/mapped_framebuffer.cpp | 4 ++
>>>>>> src/libcamera/media_device.cpp | 2 +
>>>>>> src/libcamera/media_object.cpp | 6 +++
>>>>>> src/libcamera/pipeline/ipu3/imgu.cpp | 2 +
>>>>>> .../pipeline/rpi/common/delayed_controls.cpp | 3 ++
>>>>>> src/libcamera/pipeline_handler.cpp | 4 ++
>>>>>> src/libcamera/process.cpp | 4 ++
>>>>>> src/libcamera/pub_key.cpp | 2 +
>>>>>> src/libcamera/request.cpp | 1 +
>>>>>> src/libcamera/source_paths.cpp | 1 +
>>>>>> src/libcamera/sysfs.cpp | 1 +
>>>>>> src/libcamera/v4l2_device.cpp | 2 +
>>>>>> src/libcamera/v4l2_pixelformat.cpp | 3 ++
>>>>>> src/libcamera/v4l2_subdevice.cpp | 7 +++
>>>>>> src/libcamera/v4l2_videodevice.cpp | 7 +++
>>>>>> src/libcamera/yaml_parser.cpp | 5 ++
>>>>>> 77 files changed, 285 insertions(+), 8 deletions(-)
>>> [snip]
>>>
>>>>>> diff --git a/src/libcamera/base/backtrace.cpp b/src/libcamera/base/backtrace.cpp
>>>>>> index be30589d..8c0acdfc 100644
>>>>>> --- a/src/libcamera/base/backtrace.cpp
>>>>>> +++ b/src/libcamera/base/backtrace.cpp
>>>>> All components in src/libcamera/base except BoundMethod are internal.
>>>> include/libcamera/base/meson.build contains
>>>>
>>>> libcamera_base_public_headers = files([
>>>> 'bound_method.h',
>>>> 'class.h',
>>>> 'compiler.h',
>>>> 'flags.h',
>>>> 'object.h',
>>>> 'shared_fd.h',
>>>> 'signal.h',
>>>> 'span.h',
>>>> 'unique_fd.h',
>>>> ])
>>>>
>>>> so there's more than just BoundMethod.
>>>>
>>>>> Should we exclude the directory ?
>>>>>
>>>>> Same for src/libcamera/pipeline. Have you considered the option of not
>>>>> parsing the dir completely ?
>>>> I think excluding some directories would make sense. I'll experiment.
>>> For the record, I initially tried the set the list of input files
>>> explicitly. Doxyfile contains an INPUT configuration parameter that we
>>> currently set to a list of directories, and I thought I could pass it a
>>> list of files instead. We have a partial list of files corresponding to
>>> the public API, and it wasn't difficult to split libcamera_sources into
>>> public and internal sources.
>>>
>>> I hit a blocker because the list of files is stored in an array of File
>>> objects. I couldn't find a way to convert that to the real path of the
>>> files to pass to cdata.set() prior to meson 1.4.0 that introduced
>>> file.full_path(). File objects get converted automatically to file paths
>>> when passed to lots of functions, so I suppose it would be possible to
>>> use a custom_target() to convert the array of Files to strings, but
>>> that's becoming quite a bit of a hack.
>>>
>>> The other option is to include/exclude directories, which we already do
>>> actually. I'll see if that can be expanded.
>>
>> So having picked up this experiment, I think sticking with the list of specific files would be the
>> way to go. I can generate that without using file.full_path() using a method available to Meson 0.54
>> and later, and that seems to work ok. There are two main problems I hit from doing things this way:
>>
>>
>> 1. Definitions of functions for the Private counterpart class in the public class' source file -
>> this throws warnings complaining that the functions are undeclared.
>>
>> 2. Myriad references to the thread-safe anchor in src/libcamera/base/thread.cpp are unresolveable
>> without including that source file.
>>
>>
>> To solve the first problem the simplest solution is likely to be to include the internal headers -
>> as the classes are Private no documentation is generated for them anyway, though the headers do then
>> appear in the Files list in the generated documentation. For the second we can include thread.h/cpp
>> and simply flag as \internal everything except the \page block...or alternatively break that comment
>> out into a separate file in Documentation/ instead.
> Is the Threading model internal or public API?
Internal
> I suspect we don't want
> to docuemnt the thread functions in the public API - but we do want to
> include the threading model documentation, so breaking out to a separate
> .dox file might be appropriate here.
I think I lean towards that too.
>
> I think it depends how much of thread.cpp is part of the public API
> anyway.
>
>
>
>> Either way, this route reduces the files the commit touches significantly to basically just the
>> meson.builds, and additionally removes the burden of remembering to flag \internal any new functions
>> or objects we add, so I do think it's better. Any thoughts on the solutions proposed above?
> I think maintaining updates to meson.build should be easier already and
> I think we have a checkstyle rule that already tests to see if updates
> are made to meson.build in the event of adding new files. So this does
> sound like an easier route.
OK - I'll post a v2 with this so we can see what it looks like properly.
>
>>>>> Also, I see in the public documentation the Extensible::Private class
>>>>> being documented. Is this intentional ?
>>>> That class seems internal.
>>>>
>>>>> I'll skip the rest as I have enough questions already.
>>>>>
>>>>>> @@ -33,6 +33,7 @@
>>>>>> #include <libcamera/base/utils.h>
>>>>>>
>>>>>> /**
>>>>>> + * \internal
>>>>>> * \file backtrace.h
>>>>>> * \brief Generate call stack backtraces
>>>>>> */
>>>>>> @@ -135,6 +136,7 @@ std::string DwflParser::stackEntry(const void *ip)
>>>>>> } /* namespace */
>>>>>>
>>>>>> /**
>>>>>> + * \internal
>>>>>> * \class Backtrace
>>>>>> * \brief Representation of a call stack backtrace
>>>>>> *
>>>> [snip]
More information about the libcamera-devel
mailing list