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

10295 diff mbox series

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

Signed-off-by: Sebastian Fricke <sebastian.fricke.linux@gmail.com>
---
 .../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

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@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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hello Sebastian,

Thank you for the patch.

On Thu, Oct 29, 2020 at 07:53:30PM +0100, Jacopo Mondi wrote:
> Hi Sebastian,
>   a few nits, just for sake of discussion, I hope not to be pedantic.

I'll add a few comments :-)

> 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@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.

Jacopo, this looks very good to me, but I wonder, doesn't it duplicate
information we have in src/libcamera/log.cpp ? Would it make sense to
instead link to the Doxygen-generated documentation (and possibly
expanding it a little bit to mention LOG_DEFINE_CATEGORY and
LOG_DECLARE_CATEGORY), or do you think this is best documented here ?

If you think having more extensive documentation in this file is the
right option, then how about moving this text to 'Notes about debugging'
below, to keep the list of variables short and quick to read, with links
to more detailed explanations ?

> > +* 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>`__

How about using a definition list
(https://docutils.sourceforge.io/docs/ref/rst/restructuredtext.html#definition-lists)
? I think we could drop the "Brief description" prefix in the brief
descriptions, and bundles the details link on the same line. Something
like this:

LIBCAMERA_LOG_FILE
   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>`__)

   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``

LIBCAMERA_IPA_MODULE_PATH
   Define custom search locations for IPA modules (`more <#ipa-module>`__)

   Example value: ``/usr/path/one:/tmp/path/two``

> > +
> > +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

I tend to agree, if we have to write a shell howto guide, it will
quickly grow big and out of scope :-)

> > +
> > +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 ?

That's the part that bothers me a little bit too. This is something we
could possibly generate automatically. But from the point of view of a
user trying to diagnose an issue, I think the most likely course of
action is either to raise the level to DEBUG for all categories and then
possibly to restrict to specific categories identified from the pretty
verbose log, or to look at the code. I thus wonder if listing categories
explicitly here will really have an audiance. Maybe we could instead
explain how to identify categories (with the two methods I just
mentioned) ?

> > +IPA configuration
> > +~~~~~~~~~~~~~~~~~~
> > +
> > +The format and contents of the configuration file are specific to the
> > +IPA (Image processing algorithm). It usually contains data that optimize

s/(Image processing algorithm)/(Image Processing Algorithm) modules/

> > +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