| Message ID | 20210312061131.854849-3-kieran.bingham@ideasonboard.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
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@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 */
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@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 */ >
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; + 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]" : ""); 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() << "]"; + + return ss.str(); +} + } /* namespace libcamera */
Provide a toString helper to assist in printing Request state for debug and logging contexts. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- include/libcamera/request.h | 2 ++ src/libcamera/request.cpp | 20 +++++++++++++++++++- 2 files changed, 21 insertions(+), 1 deletion(-)