[libcamera-devel,1/5] ipa: raspberrypi: controller: Replace Raspberry Pi debug with libcamera debug

10952 diff mbox series

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/ipa/raspberrypi/controller/controller.cpp | 29 +++++++++++--------
 1 file changed, 17 insertions(+), 12 deletions(-)

Hi David,

Thank you for the patch.

On Fri, Jan 22, 2021 at 10:22:07AM +0000, David Plowman wrote:
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  src/ipa/raspberrypi/controller/controller.cpp | 29 +++++++++++--------
>  1 file changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/controller/controller.cpp b/src/ipa/raspberrypi/controller/controller.cpp
> index 22461cc4..f81707c2 100644
> --- a/src/ipa/raspberrypi/controller/controller.cpp
> +++ b/src/ipa/raspberrypi/controller/controller.cpp
> @@ -5,6 +5,8 @@
>   * controller.cpp - ISP controller
>   */
>  
> +#include "libcamera/internal/log.h"
> +
>  #include "algorithm.hpp"
>  #include "controller.hpp"
>  
> @@ -12,6 +14,9 @@
>  #include <boost/property_tree/ptree.hpp>
>  
>  using namespace RPiController;
> +using namespace libcamera;

I wonder if the controllers should be moved to the libcamera namespace
(to be precise, I mean making RPiController a sub-namespace of
libcamera). Not something to address in this series in any case.

> +
> +LOG_DEFINE_CATEGORY(RPiController)

I've been toying with the idea of creating hierarchical logging
categories. This could become

LOG_DEFINE_CATEGORY(RPi.Controller)

(or something similar). We can already enable categories using wildcards
(e.g. LIBCAMERA_LOG_LEVELS=RPi*:0), but creating categories with
explicit prefixes would force us to think about the best way to group
categories. Would this be useful ? It's not a high priority on my todo
list in any case.

Another idea I've been toying with is adding a main() function to
libcamera.so, in order to print information when invoking the shared
library as an executable. This would include the list of available
categories. Same question, useful or not ?

>  Controller::Controller()
>  	: switch_mode_called_(false) {}
> @@ -27,7 +32,7 @@ Controller::~Controller() {}
>  
>  void Controller::Read(char const *filename)
>  {
> -	RPI_LOG("Controller starting");
> +	LOG(RPiController, Debug) << "Controller starting";

We usually refrain from using debug messages that just trace the code,
but we don't have a drop-in replacement for this (there's a tracing
infrastructure, but that's for a different purpose). Have you found
these particular debug messages useful ?

>  	boost::property_tree::ptree root;
>  	boost::property_tree::read_json(filename, root);
>  	for (auto const &key_and_value : root) {
> @@ -36,10 +41,10 @@ void Controller::Read(char const *filename)
>  			algo->Read(key_and_value.second);
>  			algorithms_.push_back(AlgorithmPtr(algo));
>  		} else
> -			RPI_LOG("WARNING: No algorithm found for \""
> -				<< key_and_value.first << "\"");
> +			LOG(RPiController, Debug) << "WARNING: No algorithm found for \""
> +						  << key_and_value.first << "\"";

If it's a warning, about about using the Warning log level, and dropping
the "WARNING: " prefix ?

>  	}
> -	RPI_LOG("Controller finished");
> +	LOG(RPiController, Debug) << "Controller finished";
>  }
>  
>  Algorithm *Controller::CreateAlgorithm(char const *name)
> @@ -50,39 +55,39 @@ Algorithm *Controller::CreateAlgorithm(char const *name)
>  
>  void Controller::Initialise()
>  {
> -	RPI_LOG("Controller starting");
> +	LOG(RPiController, Debug) << "Controller starting";
>  	for (auto &algo : algorithms_)
>  		algo->Initialise();
> -	RPI_LOG("Controller finished");
> +	LOG(RPiController, Debug) << "Controller finished";
>  }
>  
>  void Controller::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)
>  {
> -	RPI_LOG("Controller starting");
> +	LOG(RPiController, Debug) << "Controller starting";
>  	for (auto &algo : algorithms_)
>  		algo->SwitchMode(camera_mode, metadata);
>  	switch_mode_called_ = true;
> -	RPI_LOG("Controller finished");
> +	LOG(RPiController, Debug) << "Controller finished";
>  }
>  
>  void Controller::Prepare(Metadata *image_metadata)
>  {
> -	RPI_LOG("Controller::Prepare starting");
> +	LOG(RPiController, Debug) << "Controller::Prepare starting";

Should the previous three functions also mention the function name ?

>  	assert(switch_mode_called_);
>  	for (auto &algo : algorithms_)
>  		if (!algo->IsPaused())
>  			algo->Prepare(image_metadata);
> -	RPI_LOG("Controller::Prepare finished");
> +	LOG(RPiController, Debug) << "Controller::Prepare finished";
>  }
>  
>  void Controller::Process(StatisticsPtr stats, Metadata *image_metadata)
>  {
> -	RPI_LOG("Controller::Process starting");
> +	LOG(RPiController, Debug) << "Controller::Process starting";
>  	assert(switch_mode_called_);
>  	for (auto &algo : algorithms_)
>  		if (!algo->IsPaused())
>  			algo->Process(stats, image_metadata);
> -	RPI_LOG("Controller::Process finished");
> +	LOG(RPiController, Debug) << "Controller::Process finished";
>  }
>  
>  Metadata &Controller::GetGlobalMetadata()