[libcamera-devel] [PATCH v3 01/11] libcamera: Print Timer identifier

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Dec 01, 2021 at 12:36:24AM +0100, Jacopo Mondi wrote:
> The Timer debug output does not report to which timer a condition refers
> to. Fix that by printing the Timer address.

Do I assume correctly that this comes from an issue you were trying to
debug ? If so, could you share some information (for my own information,
doesn't have to be recorded in the commit message) ?

> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/base/event_dispatcher_poll.cpp | 3 ++-
>  src/libcamera/base/timer.cpp                 | 4 ++--
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/base/event_dispatcher_poll.cpp b/src/libcamera/base/event_dispatcher_poll.cpp
> index 8ee22d5adcc4..20cc6e617189 100644
> --- a/src/libcamera/base/event_dispatcher_poll.cpp
> +++ b/src/libcamera/base/event_dispatcher_poll.cpp
> @@ -214,7 +214,8 @@ int EventDispatcherPoll::poll(std::vector<struct pollfd> *pollfds)
>  			timeout = { 0, 0 };
>  
>  		LOG(Event, Debug)
> -			<< "timeout " << timeout.tv_sec << "."
> +			<< "timeout " << nextTimer << " "
> +			<< timeout.tv_sec << "."

"timeout <pointer> <value>" could be a bit hard to read, how about

		LOG(Event, Debug)
			<< "next timer " << nextTimer << " expires in "
			<< timeout.tv_sec << "."
			<< std::setfill('0') << std::setw(9)
			<< timeout.tv_nsec;

If you think that's overkill you can ignore it.

>  			<< std::setfill('0') << std::setw(9)
>  			<< timeout.tv_nsec;
>  	}
> diff --git a/src/libcamera/base/timer.cpp b/src/libcamera/base/timer.cpp
> index 9c54352d46bd..187336e3a1a4 100644
> --- a/src/libcamera/base/timer.cpp
> +++ b/src/libcamera/base/timer.cpp
> @@ -96,7 +96,7 @@ void Timer::start(std::chrono::milliseconds duration)
>  void Timer::start(std::chrono::steady_clock::time_point deadline)
>  {
>  	if (Thread::current() != thread()) {
> -		LOG(Timer, Error) << "Timer can't be started from another thread";
> +		LOG(Timer, Error) << "Timer " << this << " << can't be started from another thread";

Maybe a timer ID would be useful to have in the future for debugging
purpose. Then we could inherit from Loggable. For now this will do.

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

>  		return;
>  	}
>  
> @@ -128,7 +128,7 @@ void Timer::stop()
>  		return;
>  
>  	if (Thread::current() != thread()) {
> -		LOG(Timer, Error) << "Timer can't be stopped from another thread";
> +		LOG(Timer, Error) << "Timer " << this << " can't be stopped from another thread";
>  		return;
>  	}
>  

-- 
Regards,

Laurent Pinchart