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

[Kieran Bingham]

Hi Laurent,

On 14/03/2021 01:40, Laurent Pinchart wrote:
> 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>.

Ack,

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

Yes, been here ;-)

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

Cancelled is already handled because the state gets printed.
Although - indeed - before this call if cancelled was set - that
wouldn't have been updated yet.

I guess I was only adding the information I cared about (the sequence,
and state) ... the cookie is application specific - so I don't think we
care about that when dealing with internal debug ...

And I don't yet know how much cross over there will be between tracking
internal debug against whatever the applciation stores in the cookie.


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

Hrm ... I don't know yet. I 'liked' having the extended print while
trawling through the logs - but it could be made more concise.

You're example isn't so bad, and I like that it would then be machine
parseable too.

I'll try it out.


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

-- 
Regards
--
Kieran