[libcamera-devel] [PATCH] libcamera: Add debug printouts

[Laurent Pinchart]

Hello,

On Monday, 14 January 2019 15:44:21 EET Kieran Bingham wrote:
> On 11/01/2019 16:07, Jacopo Mondi wrote:
> > Add a few debug printouts that help follow the library intialization
> > process: what pipeline handlers are registered, what media devices are
> > created, and which pipeline manager gets matches with the current
> > system.
> 
> Great - I think this will be useful to have some visibility while
> developing, and we can easily disable the DBG output when we add
> LogLevel selection. (and we'll probably default it above the debug level
> of course).
> 
> > The resulting output is the following, on IPU3 devices:
> > DBG pipeline_handler.cpp:119 Pipeline handler: "PipeHandlerVimc"
> > registered
> > DBG pipeline_handler.cpp:119 Pipeline handler: "PipelineHandlerIPU3"
> > registered DBG device_enumerator.cpp:214 New media device: ipu3-imgu
> > created from: /dev/media0 DBG device_enumerator.cpp:214 New media device:
> > ipu3-cio2 created from: /dev/media1 DBG device_enumerator.cpp:255
> > Succesfull match for media device: ipu3-cio2 DBG
> > device_enumerator.cpp:255 Succesfull match for media device: ipu3-imgu
> > DBG pipeline_handler.cpp:150 Pipeline handler: "PipelineHandlerIPU3"
> > matched
> > 
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > 
> >  src/libcamera/device_enumerator.cpp | 8 +++++++-
> >  src/libcamera/pipeline_handler.cpp  | 5 ++++-
> >  2 files changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/libcamera/device_enumerator.cpp
> > b/src/libcamera/device_enumerator.cpp index 0d18e75..c1ddd7c 100644
> > --- a/src/libcamera/device_enumerator.cpp
> > +++ b/src/libcamera/device_enumerator.cpp
> > @@ -211,6 +211,9 @@ int DeviceEnumerator::addDevice(const std::string
> > &devnode)
> >  		return ret;
> >  	}
> > 
> > +	LOG(Debug) << "New media device: " << media->driver()
> > +		   << " created from: " << devnode;
> > +

Do we need the ":" ? This would print

New media device: uvcvideo created from: /dev/media0

Wouldn't the following be more readable ?

New media device "uvcvideo created from /dev/media0

That would be obtained by

	LOG(Debug) << "New media device \"" << media->driver()
		<< "\" created from " << devnode;

> >  	/* Associate entities to device node paths. */
> >  	for (MediaEntity *entity : media->entities()) {
> >  		if (entity->deviceMajor() == 0 && entity->deviceMinor() == 0)
> > @@ -248,8 +251,11 @@ MediaDevice *DeviceEnumerator::search(const
> > DeviceMatch &dm) const
> >  		if (dev->busy())
> >  			continue;
> > 
> > -		if (dm.match(dev))
> > +		if (dm.match(dev)) {
> > +			LOG(Debug) << "Succesfull match for media device: "
> 
> s/Succesfull/Successful/
> 
> > +				   << dev->driver();

Same here, I'd remove the ":".

> >  			return dev;
> > +		}
> >  	}
> >  	
> >  	return nullptr;
> > diff --git a/src/libcamera/pipeline_handler.cpp
> > b/src/libcamera/pipeline_handler.cpp index ee76948..9299c28 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -116,6 +116,7 @@ void PipelineHandlerFactory::registerType(const
> > std::string &name,
> >  		return;
> >  	}
> > 
> > +	LOG(Debug) << "Pipeline handler: \"" << name << "\" registered";

And here

	LOG(Debug) << "Registered pipeline handler \"" << name << "\"";

> >  	factories[name] = factory;
> >  }
> > 
> > @@ -145,8 +146,10 @@ PipelineHandler *PipelineHandlerFactory::create(const
> > std::string &name,
> >  	PipelineHandler *pipe = it->second->create();
> > 
> > -	if (pipe->match(enumerator))
> > +	if (pipe->match(enumerator)) {
> > +		LOG(Debug) << "Pipeline handler: \"" << name << "\" matched";

And removing the ":" here too.

> (not for this patch - just ideas out loud)
> 
> Can/Should we make a function/macro called
> 	 quoted((os), (_s))
> 
>  which will do:
> 
>   os << ("\"" << (_s) << "\"")

I suppose this would be

#define quote(s)		"\"" << s << "\""

> LOG(Debug) << "Pipeline handler: \"" << name << "\" matched";
> LOG(Debug) << "Pipeline handler: " << quoted(name) << " matched";

Wouldn't it reduce readability by hiding a simple construct behind a macro ? I 
know it's tempting to avoid writing the escape sequence...

> >  		return pipe;
> > +	}
> > 
> >  	delete pipe;
> >  	return nullptr;

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

-- 
Regards,

Laurent Pinchart