[libcamera-devel] [PATCH 2/8] libcamera: request: Add a toString()

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Fri, Mar 12, 2021 at 06:11:25AM +0000, Kieran Bingham wrote:
> Provide a toString helper to assist in printing Request state
> for debug and logging contexts.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  include/libcamera/request.h |  2 ++
>  src/libcamera/request.cpp   | 20 +++++++++++++++++++-
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index 6f2f881e840a..59d7f4bac0d2 100644
> --- a/include/libcamera/request.h
> +++ b/include/libcamera/request.h
> @@ -56,6 +56,8 @@ public:
>  
>  	bool hasPendingBuffers() const { return !pending_.empty(); }
>  
> +	const std::string toString() const;

You can drop the initial const.

And you should include <string>.

> +
>  private:
>  	LIBCAMERA_DISABLE_COPY(Request)
>  
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index 24c3694de5fc..12c2e7d425f9 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -268,7 +268,7 @@ void Request::complete()
>  	status_ = cancelled_ ? RequestCancelled : RequestComplete;
>  
>  	LOG(Request, Debug)
> -		<< "Request has completed - cookie: " << cookie_
> +		<< toString() << " has completed - cookie: " << cookie_
>  		<< (cancelled_ ? " [Cancelled]" : "");

Inheriting from Loggable would be nicer, but that's not part of the
public API :-S

Should the cookie and cancelled state be handled in toString() ?

>  
>  	LIBCAMERA_TRACEPOINT(request_complete, this);
> @@ -302,4 +302,22 @@ bool Request::completeBuffer(FrameBuffer *buffer)
>  	return !hasPendingBuffers();
>  }
>  
> +const std::string Request::toString() const
> +{
> +	std::stringstream ss;
> +
> +	static const char *statuses[] = {
> +		"Pending",
> +		"Complete",
> +		"Cancelled",
> +	};
> +
> +	ss << "Request (" << sequence_ << ") " << statuses[status_];
> +
> +	if (hasPendingBuffers())
> +		ss << " [Pending:" << pending_.size() << "]";

Should we make this a bit more concise ? It can then become a bit
cryptic, but as it's a debugging tool, I'd focus on making it readable
for the trained eye (as well as possibly easily machine-parseable).
Having abbreviated status names, for instance, to give them all the same
length, may not look as nice at first, but when you read thousands of
lines of log, it quickly makes it easier I think.

I'm thinking about something like this:

	static const char *statuses = "PCX";

	"Request(%u:%c:%u)", sequence_, statuses[status_], pending_.size()

(it's the most extreme case, a middle ground is certainly possible with
longer statuses, and possibly even the full status name)

Maybe this is just me ?

> +
> +	return ss.str();
> +}
> +
>  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart