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

Jacopo Mondi jacopo at jmondi.org
Thu Oct 29 19:53:30 CET 2020 

Hi Sebastian,
  a few nits, just for sake of discussion, I hope not to be pedantic.

I see in the subject 'V2'. I assume it has been added by hand. You can
have git generate the 'v' tag for you in all patches of a series
adding -v option to $(git format-patch) :)

On Thu, Oct 29, 2020 at 09:37:35AM +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
> for how to use them for debugging.
>
> 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

Chagelog are usually recorded in a series' cover letter, or in the case
of a single patch like this one, placed under the Signed-off line
prefixed with '---' to tell git to ignore this section when the patch
is applied (as we don't want change logs to be part of the commit
history). See below

>
> Signed-off-by: Sebastian Fricke <sebastian.fricke.linux at gmail.com>

---

Your changelog here

> ---
>  .../guides/environment_variables.rst          | 107 ++++++++++++++++++
>  Documentation/index.rst                       |   1 +
>  Documentation/meson.build                     |   1 +
>  3 files changed, 109 insertions(+)
>  create mode 100644 Documentation/guides/environment_variables.rst
>

Applying the patch gives me:

$ git am /tmp/_\[libcamera-devel\]_\[PATCH_V2\]_Documentation_Add_descriptions_for_env._.patch
Applying: Documentation: Add descriptions for env. variables
.git/rebase-apply/patch:27: trailing whitespace.
        * Details: `Levels <#log-levels>`__ and `Categories <#log-categories>`__
.git/rebase-apply/patch:68: trailing whitespace.
Log levels
.git/rebase-apply/patch:80: trailing whitespace.
Log categories
.git/rebase-apply/patch:83: trailing whitespace.
* Major classes:
.git/rebase-apply/patch:101: trailing whitespace.
IPA configuration
warning: squelched 1 whitespace error
warning: 6 lines add whitespace errors.

It seems to me there are a few whitespaces at the end of the following
lines:

-        * Details: `Levels <#log-levels>`__ and `Categories <#log-categories>`__
+        * Details: `Levels <#log-levels>`__ and `Categories <#log-categories>`__

-Log levels
+Log levels
 ~~~~~~~~~~~

-Log categories
+Log categories
 ~~~~~~~~~~~~~~~

-* Major classes:
+* Major classes:

-IPA configuration
+IPA configuration
 ~~~~~~~~~~~~~~~~~~

-IPA module
+IPA module
 ~~~~~~~~~~~


> diff --git a/Documentation/guides/environment_variables.rst b/Documentation/guides/environment_variables.rst

You know, I'm not sure this is a guide. It would better fit in Documentation/
imho.

> new file mode 100644
> index 0000000..6e627fd
> --- /dev/null
> +++ b/Documentation/guides/environment_variables.rst
> @@ -0,0 +1,107 @@
> +Environment variables
> +=====================
> +
> +List of variables
> +-----------------
> +
> +* LIBCAMERA_LOG_FILE
> +        * Brief description: Custom destination for log output
> +        * Example value: ``/home/{user}/camera_log.log``
> +
> +* LIBCAMERA_LOG_LEVELS
> +        * Brief description: Configure the verbosity of log messages for different categories
> +        * Example value: ``*:DEBUG``

What about a few more examples with a little documentation ?

                "0" : Display all debug messages
                "4" : Filter all debug messages except from fatal ones
                "Camera:0,Pipeline:3" : Display all messages of
                category 'Camera' and filter all error messages but
                'Error' and 'Fatal' of category "Pipeline". It is
                equivalent to "Camera:DEBUG,Pipeline:ERROR"

> +        * Details: `Levels <#log-levels>`__ and `Categories <#log-categories>`__

I think it deserves a bit longer description. What about:

Configure the logging system verbosity by applying filters to log
categories. Filters and categories are specified in a comma separated
list of "category:level" pairs. If no `Category <#log-categories>`__
is specified, the supplied level is applied to all messages.

Levels can be specified by numerical values or using the `Levels <#log-levels>`__
identifiers. Levels are ordered by severity from 0 ('Debug') to 4
('Fatal'). Setting a log level implies only messages with severity
equal or higher than the specified one are displayed.

Log categories are defined by each libcamera component which use the
logging infrastructure using the LOG_DEFINE_CATEGORY() and
LOG_DECLARE_CATEGORY() macros, usually at the beginning of the file.


> +* LIBCAMERA_IPA_CONFIG_PATH
> +        * Brief description: Define custom search locations for IPA configurations
> +        * Example value: ``/usr/path/one:/tmp/path/two``
> +        * Details: `IPA configuration <#ipa-configuration>`__
> +* LIBCAMERA_IPA_MODULE_PATH
> +        * Brief description: Define custom search locations for IPA modules
> +        * Example value: ``/usr/path/one:/tmp/path/two``
> +        * Details: `IPA module <#ipa-module>`__
> +
> +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 the libcamera API.


In general, we keep an 80-cols limit also for documentation whenever
possible. It seems like you've done this in the rest of the file. What
happened here ?

> +
> +You can define the values for these variables locally or globally. If you set it locally, note that for Bash-like shells, you need to set the value of `LIBCAMERA_LOG_LEVELS` on the same line as the command, as shown in the first example.

I think this is pretty general information about setting variables in
a shell environment. I would not put them here, it's not libcamera
specific stuff

> +
> +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
> +
> +Enable full debug output for the categories Camera & V4L2 within a
> +global environment:
> +
> +.. code:: bash
> +
> +   :~$ export LIBCAMERA_LOG_LEVELS='Camera:DEBUG,V4L2:DEBUG'
> +   :~$ cam --list
> +
> +An alternative value to for `LIBCAMERA_LOG_LEVELS` to enable
> +debugging output for all categories is 0.

I think this line is captured above by the description suggestion I
have proposed.

> +
> +Log levels
> +~~~~~~~~~~~
> +
> +This is the list of available log levels, notice that all levels below
> +the chosen one are printed, while those above are discarded.
> +
> +-  DEBUG
> +-  INFO
> +-  WARN
> +-  ERROR
> +-  FATAL
> +
> +Log categories
> +~~~~~~~~~~~~~~~
> +
> +* Major classes:
> +          + Camera, CameraMetadata, CameraSensor, Controls, DeviceEnumerator, Formats, MediaDevice, Request
> +
> +* Pipeline:
> +          + Pipeline, IPU3, RPI, RPISTREAM, RPI_S_W, RkISP1, SimplePipeline, Timeline, UVC, VIMC
> +
> +* V4L2-API:
> +          + V4L2, V4L2Compat
> +
> +* IPA (Image Processing Algorithms):
> +          + IPAManager, IPAModule, IPAProxy, IPAProxyLinuxWorker, IPARkISP1, IPARPI, IPAVimc, RPiFocus
> +
> +* Android specific:
> +          + CameraMetadata, EXIF, HAL, JPEG
> +
> +* Others:
> +          + Allocator, Buffer, Event, File, FileDescriptor, IPCUnixSocket, LogAPITest, LogProcessTest, Message, Object, Process, Serialization, Serializer, Stream, SysFs, Thread, Timeline, Timer
> +

This will have to be updated everytime we add a new category ?

> +IPA configuration
> +~~~~~~~~~~~~~~~~~~
> +
> +The format and contents of the configuration file are specific to the
> +IPA (Image processing algorithm). It usually contains data that optimize
> +the behaviour of the algorithms stored in JSON format. The
> +``LIBCAMERA_IPA_CONFIG_PATH`` variable can be used to specify custom
> +storeage locations to search for those configuration files.
> +
> +`Examples <https://git.libcamera.org/libcamera/libcamera.git/tree/src/ipa/raspberrypi/data>`__
> +
> +IPA module
> +~~~~~~~~~~~
> +
> +Existing IPA modules can be located in the
> +`src/ipa/ <https://git.libcamera.org/libcamera/libcamera.git/tree/src/ipa>`__
> +directory, they provide the interface to the ISP(image signal processer)
> +and the implementation of algorithms. With the

Do not explain in 2 lines what an IPA is :)
Just mention the default search location, when libcamera is run from
the source tree and how to use the env variable to expand the search
path.

Thank you for the extensive work!
   j

> +``LIBCAMERA_IPA_MODULE_PATH``, you can specify a non-default location to
> +search for these modules.
> diff --git a/Documentation/index.rst b/Documentation/index.rst
> index a30688a..924bd12 100644
> --- a/Documentation/index.rst
> +++ b/Documentation/index.rst
> @@ -16,3 +16,4 @@
>     Developer Guide <guides/introduction>
>     Application Writer's Guide <guides/application-developer>
>     Pipeline Handler Writer's Guide <guides/pipeline-handler>
> +   Environment variables <guides/environment_variables.rst>
> diff --git a/Documentation/meson.build b/Documentation/meson.build
> index d3d64f7..0e7ba7d 100644
> --- a/Documentation/meson.build
> +++ b/Documentation/meson.build
> @@ -56,6 +56,7 @@ if sphinx.found()
>          'guides/introduction.rst',
>          'guides/application-developer.rst',
>          'guides/pipeline-handler.rst',
> +        'guides/environment_variables.rst',
>      ]
>
>      release = 'release=v' + libcamera_git_version
> --
> 2.20.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

More information about the libcamera-devel mailing list