[libcamera-devel] [PATCH 11/11] Adds useful debug print statements.

[Laurent Pinchart]

Hi Nicholas,

Thank you for the patch.

On Mon, Oct 24, 2022 at 12:55:43AM -0500, Nicholas Roth via libcamera-devel wrote:
> From: Nicholas Roth <nicholas at rothemail.net>
> 
> ---
>  src/android/camera_capabilities.cpp           | 12 +++++++++---
>  src/android/camera_hal_manager.cpp            |  3 ++-
>  src/libcamera/base/log.cpp                    |  6 +++++-
>  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp |  2 ++
>  src/libcamera/v4l2_subdevice.cpp              |  3 ++-
>  5 files changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> index 64bd8dde..ef0d10d0 100644
> --- a/src/android/camera_capabilities.cpp
> +++ b/src/android/camera_capabilities.cpp
> @@ -374,14 +374,20 @@ void CameraCapabilities::computeHwLevel(
>  	camera_metadata_enum_android_info_supported_hardware_level
>  		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL;
>  
> -	if (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR))
> +	if (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR)) {
> +		LOG(HAL, Info) << noFull << "missing manual sensor";
>  		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> +	}
>  
> -	if (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING))
> +	if (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING)) {
> +		LOG(HAL, Info) << noFull << "missing manual post processing";
>  		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> +	}
>  
> -	if (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE))
> +	if (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE)) {
> +		LOG(HAL, Info) << noFull << "missing burst capture";
>  		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> +	}

Paul, Jacopo, could you maybe review this part ?

>  
>  	found = staticMetadata_->getEntry(ANDROID_SYNC_MAX_LATENCY, &entry);
>  	if (!found || *entry.data.i32 != 0) {
> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
> index 7512cc4e..7fac4e3f 100644
> --- a/src/android/camera_hal_manager.cpp
> +++ b/src/android/camera_hal_manager.cpp
> @@ -140,7 +140,8 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)
>  	 */
>  	if (!isCameraExternal && !halConfig_.exists()) {
>  		LOG(HAL, Error)
> -			<< "HAL configuration file is mandatory for internal cameras";
> +			<< "HAL configuration file is mandatory for internal cameras."
> +			<< " Camera NOT loaded: \"" << cam->id() << "\"";
>  		return;
>  	}
>  
> diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp
> index 55fbd7b0..b8c2c99f 100644
> --- a/src/libcamera/base/log.cpp
> +++ b/src/libcamera/base/log.cpp
> @@ -625,8 +625,12 @@ void Logger::parseLogFile()
>  void Logger::parseLogLevels()
>  {
>  	const char *debug = utils::secure_getenv("LIBCAMERA_LOG_LEVELS");
> -	if (!debug)
> +	if (!debug) {
> +		syslog(LOG_INFO, "Could not find LIBCAMERA_LOG_LEVELS in env");
>  		return;
> +	} else {
> +		syslog(LOG_INFO, "LIBCAMERA_LOG_LEVELS is %s", debug);
> +	}

I don't think we should log a message to syslog every time libcamera is
started to report the LIBCAMERA_LOG_LEVELS value. Is this a debugging
leftover ?

>  
>  	for (const char *pair = debug; *debug != '\0'; pair = debug) {
>  		const char *comma = strchrnul(debug, ',');
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> index 2d38f0fb..a2038704 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> @@ -33,10 +33,12 @@ bool RkISP1Path::init(MediaDevice *media)
>  	std::string resizer = std::string("rkisp1_resizer_") + name_ + "path";
>  	std::string video = std::string("rkisp1_") + name_ + "path";
>  
> +	LOG(RkISP1, Debug) << "Creating " << resizer;
>  	resizer_ = V4L2Subdevice::fromEntityName(media, resizer);
>  	if (resizer_->open() < 0)
>  		return false;
>  
> +	LOG(RkISP1, Debug) << "Creating " << video;

Here too, is this something that you added to debug specific issues you
were facing, or do you expect this to be useful for users ? If the
latter, could you explain why ?

>  	video_ = V4L2VideoDevice::fromEntityName(media, video);
>  	if (video_->open() < 0)
>  		return false;
> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> index 15e8206a..8f86387b 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -392,7 +392,8 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,
>  	if (ret < 0) {
>  		LOG(V4L2, Error)
>  			<< "Unable to get rectangle " << target << " on pad "
> -			<< pad << ": " << strerror(-ret);
> +			<< pad << ": " << strerror(-ret) << "."
> +			<< "device path: " << devicePath() << " device node: " << deviceNode();

If the device path and device node are useful in error messages, they
should be printed in all of them, not just this one. This should then be
done through the logPrefix() function. The code currently uses the
entity name as a log prefix, is that not enough ? The device path, in
particular, seems too verbose to me.

>  		return ret;
>  	}
>  

-- 
Regards,

Laurent Pinchart