[libcamera-devel] [PATCH v4] Documentation: Add descriptions for env. variables

[Laurent Pinchart]

Hi Sebastian,

Thank you for the patch, and sorry for the very late review. We've
recently installed a patchwork instance for libcamera, so now I can
track all the patches I need to take care of. Hopefully it will avoid
such delays in the future.

The change looks good. I have a few minor comments. If you're OK with
them, I'll address them when applying the patch and you won't have to
send a v5.

You can spell "environment" in full in the subject line.

On Fri, Nov 20, 2020 at 04:52:34PM +0100, Sebastian Fricke wrote:
> Describe the environment variables used in libcamera, excluded
> variables are `LIBCAMERA_IPA_FORCE_C_API` and `LIBCAMERA_IPA_PROXY_PATH`,
> the former because it is likely to be removed and the later because
> it has no current use-case.
> 
> Add a brief explanation for the IPA configuration and IPA modules.
> List all the available Log levels and categories and add a short guide
> on how to use them for debugging.
> 
> Signed-off-by: Sebastian Fricke <sebastian.fricke.linux at gmail.com>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> ---
> 
> Changes since V3:
> * Rework the LIBCAMERA_LOG_LEVELS explanation
> * improve wording at various places
> * Move the changelog out of the commit description into the email
> 
> Changes since V2:
> * replace bullet point list with definition list
> * Remove the category list as it is too difficult to maintain, instead
>   explain how the categories are set with the LOG_DECLARE_CATEGORY &
>   LOG_DEFINE_CATEGORY macros
> * Remove the short summary for the IPA module, as it can be misleading
>   and probably not accurate enough
> * Improve the notes for debugging part by linking to a full explanation
>   while extending the description in the documentation as well
> * Add an example for the Log level setting
> * Remove the documentation from guides and add it to the documentation
>   base path
> 
> Changes since V1:
> * abandon the usage of tables as they are too clunky and difficult to
> maintain
> * Fix a wrong example that does not work on most distributions and setups
> * Improve structure of log categories
> 
> ---
> 
>  Documentation/environment_variables.rst | 132 ++++++++++++++++++++++++
>  Documentation/index.rst                 |   1 +
>  Documentation/meson.build               |   1 +
>  3 files changed, 134 insertions(+)
>  create mode 100644 Documentation/environment_variables.rst
> 
> diff --git a/Documentation/environment_variables.rst b/Documentation/environment_variables.rst
> new file mode 100644
> index 0000000..85346af
> --- /dev/null
> +++ b/Documentation/environment_variables.rst
> @@ -0,0 +1,132 @@

This is missing license information. To match the rest of the
documentation, it should be

.. SPDX-License-Identifier: CC-BY-SA-4.0

> +Environment variables
> +=====================

How about a short introduction ? I can propose

The libcamera behaviour can be tuned through environment variables. This
document lists all the available variables and describes their usage.

> +
> +List of variables
> +-----------------
> +
> +LIBCAMERA_LOG_FILE
> +   The custom destination for log output.
> +
> +   Example value: ``/home/{user}/camera_log.log``
> +
> +LIBCAMERA_LOG_LEVELS
> +   Configure the verbosity of log messages for different categories (`more <#log-levels>`__)

Nitpicking, I'd add a period at the end of the sentence, as the previous
variable has one. Same below.

> +
> +   Example value: ``*:DEBUG``
> +
> +LIBCAMERA_IPA_CONFIG_PATH
> +   Define custom search locations for IPA configurations (`more <#ipa-configuration>`__)
> +
> +   Example value: ``/usr/path/one:/tmp/path/two``

How about a bit more realistic example ?

    Example value: ``${HOME}/.libcamera/share/ipa:/opt/libcamera/vendor/share/ipa``

> +
> +LIBCAMERA_IPA_MODULE_PATH
> +   Define custom search locations for IPA modules (`more <#ipa-module>`__)
> +
> +   Example value: ``/usr/path/one:/tmp/path/two``

And here,

    Example value: ``${HOME}/.libcamera/lib:/opt/libcamera/vendor/lib``

> +
> +Further details
> +---------------
> +
> +Notes about debugging
> +~~~~~~~~~~~~~~~~~~~~~
> +
> +The environment variables `LIBCAMERA_LOG_FILE` and `LIBCAMERA_LOG_LEVELS`
> +are used to modify the destination and verbosity of messages provided by
> +libcamera.
> +
> +The `LIBCAMERA_LOG_LEVELS` variable accepts a comma-separated list of
> +'category:level' pairs.
> +
> +The `level <#log-levels>`__ part is mandatory and can either be
> +specified by name or by numerical index associated with each level.
> +
> +The optional `category <#log-categories>`__ part is a regular
> +expression, which is matched against the categories defined by each
> +file in the source base using the logging infrastructure.

It's not a full regular expression, only a string with an optional *
wildcard match at the end. How about

The optional `category <#log-categories>`__ is a string matching the categories
defined by each file in the source base using the logging infrastructure. It
can include a wildcard ('*') character at the end to match multiple categories.

> +
> +For more information refer to the `API-documentation <http://libcamera.org/api-html/log_8h.html#details>`__

Maybe s/API-documentation/API documentation/ ?

This should also have a trailing period.

> +
> +Examples:
> +
> +Enable full debug output to a separate file, for every `category <#log-categories>`__
> +within a local environment:
> +
> +.. code:: bash
> +
> +        :~$ LIBCAMERA_LOG_FILE='/tmp/example_log.log' \
> +            LIBCAMERA_LOG_LEVELS=0 \
> +            cam --list

You can reduce the indentation to match the other examples below.

> +
> +Enable full debug output for the categories Camera & V4L2 within a

How about writing

	[...] categories `Camera` and `V4L2` within [...]

to emphasize both terms ?

> +global environment:
> +
> +.. code:: bash
> +
> +   :~$ export LIBCAMERA_LOG_LEVELS='Camera:DEBUG,V4L2:DEBUG'
> +   :~$ cam --list
> +
> +Log levels
> +~~~~~~~~~~~

There's an extra ~ here. Same in a few locations below.

> +
> +This is the list of available log levels, notice that all levels below
> +the chosen one are printed, while those above are discarded.
> +
> +-  DEBUG (0)
> +-  INFO (1)
> +-  WARN (2)
> +-  ERROR (3)
> +-  FATAL (4)
> +
> +Example:
> +If you choose WARN (2), you will be able to see WARN (2), ERROR (3) & FATAL (4)
> +but not DEBUG (0) & INFO (1).

'and' would be better than '&' in text I think.

> +
> +Log categories
> +~~~~~~~~~~~~~~~
> +
> +Every category represents a specific area of the libcamera codebase,
> +the names can be located within the source code, for example:
> +`src/libcamera/camera_manager.cpp <https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/camera_manager.cpp#n35>`__
> +
> +.. code:: bash

This is C++ code :-)

> +
> +   LOG_DEFINE_CATEGORY(Camera)
> +
> +There are 2 available macros used to assign a category name to a part of the

s/2/two/

> +libcamera codebase:
> +
> +LOG_DEFINE_CATEGORY
> +        This macro is required, in order to use the `LOGC` macro for a

Isn't this LOG, not LOGC ?

> +	particular category.  It can only be used once for each category.

Should we avoid mixing tabs and spaces for indentation ?

> +	If you want to create log messages within multiple compilation
> +	units for the same category utilize the `LOG_DECLARE_CATEGORY` macro,
> +	in every file except the definition file.
> +LOG_DECLARE_CATEGORY
> +        Used for sharing an already defined category in between multiple

s/in between/between/

> +	separate compilation units.
> +
> +Both macros have to be used within the libcamera namespace of the C++ source
> +code.
> +
> +IPA configuration
> +~~~~~~~~~~~~~~~~~~
> +
> +The format and contents of the configuration file are specific to the
> +IPA (Image Processing Algorithm). It usually contains tuning parameters
> +for the IPA algorithms stored in JSON format.

Nothing above mentions an IPA configuration file, it could be useful to
introduce the concept. How about the following ?

IPA modules use configuration files to store parameters. The format and
contents of the configuration files is specific to the IPA module. They
usually contain tuning parameters for the algorithms, in JSON format.

> +The `LIBCAMERA_IPA_CONFIG_PATH` variable can be used to specify custom
> +storage locations to search for those configuration files.
> +
> +`Examples <https://git.libcamera.org/libcamera/libcamera.git/tree/src/ipa/raspberrypi/data>`__
> +
> +IPA module
> +~~~~~~~~~~~
> +
> +In order to locate the correct IPA module for your hardware, libcamera gathers

s/your/the/

> +existing IPA modules from multiple locations. The default locations for this
> +operation are the installed system path (for example on Debian:
> +``/usr/local/x86_64-pc-linux-gnu/libcamera``) and the build directory.
> +With the `LIBCAMERA_IPA_MODULE_PATH`, you can specify a non-default
> +location to search for IPA modules.
> +
> +`Examples for existing IPA modules <https://git.libcamera.org/libcamera/libcamera.git/tree/src/ipa>`__

I'm confused by the example. It points to the source directory for IPA
modules, which doesn't really tell me what an example would be for this
environment variable. I'd propose dropping it, as there's already an
example above.

> diff --git a/Documentation/index.rst b/Documentation/index.rst
> index ff697d4..c49db18 100644
> --- a/Documentation/index.rst
> +++ b/Documentation/index.rst
> @@ -17,3 +17,4 @@
>     Application Writer's Guide <guides/application-developer>
>     Pipeline Handler Writer's Guide <guides/pipeline-handler>
>     Tracing guide <guides/tracing>
> +   Environment variables <environment_variables>
> diff --git a/Documentation/meson.build b/Documentation/meson.build
> index 26a12fc..8086abf 100644
> --- a/Documentation/meson.build
> +++ b/Documentation/meson.build
> @@ -53,6 +53,7 @@ if sphinx.found()
>          'contributing.rst',
>          'docs.rst',
>          'index.rst',
> +        'environment_variables.rst',

This should go before index.rst to keep the alphabetical order (which I
now realize is broken by the guides/ directory).

>          'guides/introduction.rst',
>          'guides/application-developer.rst',
>          'guides/pipeline-handler.rst',

-- 
Regards,

Laurent Pinchart