[libcamera-devel,v2] cam: Add --monitor option

9123 diff mbox series

Add --monitor to monitor new hotplug and unplug camera events from
the CameraManager.

Signed-off-by: Umang Jain <email@uajain.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/cam/main.cpp | 26 ++++++++++++++++++++++++++
 src/cam/main.h   |  1 +
 2 files changed, 27 insertions(+)

Hi Umang,

Thank you for the patch.

On Fri, Jul 31, 2020 at 07:46:54PM +0000, Umang Jain wrote:
> Add --monitor to monitor new hotplug and unplug camera events from
> the CameraManager.
> 
> Signed-off-by: Umang Jain <email@uajain.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/cam/main.cpp | 26 ++++++++++++++++++++++++++
>  src/cam/main.h   |  1 +
>  2 files changed, 27 insertions(+)
> 
> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> index f5aba04..b03437e 100644
> --- a/src/cam/main.cpp
> +++ b/src/cam/main.cpp
> @@ -36,6 +36,8 @@ public:
>  	void quit();
>  
>  private:
> +	void cameraAdded(std::shared_ptr<Camera> cam);
> +	void cameraRemoved(std::shared_ptr<Camera> cam);
>  	int parseOptions(int argc, char *argv[]);
>  	int prepareConfig();
>  	int listControls();
> @@ -121,6 +123,10 @@ int CamApp::init(int argc, char **argv)
>  		ret = prepareConfig();
>  		if (ret)
>  			return ret;
> +	} else if (options_.isSet(OptMonitor)) {
> +		cm_->cameraAdded.connect(this, &CamApp::cameraAdded);
> +		cm_->cameraRemoved.connect(this, &CamApp::cameraRemoved);
> +		std::cout << "Monitoring new hotplug or unplug camera events…" << std::endl;
>  	}
>  
>  	loop_ = new EventLoop(cm_->eventDispatcher());
> @@ -189,6 +195,8 @@ int CamApp::parseOptions(int argc, char *argv[])
>  	parser.addOption(OptStrictFormats, OptionNone,
>  			 "Do not allow requested stream format(s) to be adjusted",
>  			 "strict-formats");
> +	parser.addOption(OptMonitor, OptionNone, "Monitor for hotplug and unplug camera events",

I'd move the message to the next line to avoid too long lines.

> +			 "monitor");
>  
>  	options_ = parser.parse(argc, argv);
>  	if (!options_.valid())
> @@ -309,6 +317,16 @@ int CamApp::infoConfiguration()
>  	return 0;
>  }
>  
> +void CamApp::cameraAdded(std::shared_ptr<Camera> cam)
> +{
> +	std::cout << "Camera Added: " << cam->name() << std::endl;
> +}
> +
> +void CamApp::cameraRemoved(std::shared_ptr<Camera> cam)
> +{
> +	std::cout << "Camera Removed: " << cam->name() << std::endl;
> +}
> +
>  int CamApp::run()
>  {
>  	int ret;
> @@ -342,10 +360,18 @@ int CamApp::run()
>  	}
>  
>  	if (options_.isSet(OptCapture)) {
> +		cm_->cameraAdded.connect(this, &CamApp::cameraAdded);
> +		cm_->cameraRemoved.connect(this, &CamApp::cameraRemoved);

This is done in CamApp::init() already, do we need it here ? One can
specific both --monitor and --capture if they want to monitor for
hotplug during capture, I wouldn't make it automatic.

>  		Capture capture(camera_, config_.get(), loop_);
>  		return capture.run(options_);
>  	}
>  
> +	if (options_.isSet(OptMonitor)) {
> +		ret = loop_->exec();
> +		if (ret)
> +			std::cout << "Failed to run monitor loop" << std::endl;
> +	}
> +
>  	return 0;
>  }
>  
> diff --git a/src/cam/main.h b/src/cam/main.h
> index 6f95add..ea8dfd3 100644
> --- a/src/cam/main.h
> +++ b/src/cam/main.h
> @@ -15,6 +15,7 @@ enum {
>  	OptInfo = 'I',
>  	OptList = 'l',
>  	OptListProperties = 'p',
> +	OptMonitor = 'm',
>  	OptStream = 's',
>  	OptListControls = 256,
>  	OptStrictFormats = 257,

On Fri, Jul 31, 2020 at 11:38:28PM +0300, Laurent Pinchart wrote:
> Hi Umang,
> 
> Thank you for the patch.
> 
> On Fri, Jul 31, 2020 at 07:46:54PM +0000, Umang Jain wrote:
> > Add --monitor to monitor new hotplug and unplug camera events from
> > the CameraManager.
> > 
> > Signed-off-by: Umang Jain <email@uajain.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  src/cam/main.cpp | 26 ++++++++++++++++++++++++++
> >  src/cam/main.h   |  1 +
> >  2 files changed, 27 insertions(+)
> > 
> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > index f5aba04..b03437e 100644
> > --- a/src/cam/main.cpp
> > +++ b/src/cam/main.cpp
> > @@ -36,6 +36,8 @@ public:
> >  	void quit();
> >  
> >  private:
> > +	void cameraAdded(std::shared_ptr<Camera> cam);
> > +	void cameraRemoved(std::shared_ptr<Camera> cam);
> >  	int parseOptions(int argc, char *argv[]);
> >  	int prepareConfig();
> >  	int listControls();
> > @@ -121,6 +123,10 @@ int CamApp::init(int argc, char **argv)
> >  		ret = prepareConfig();
> >  		if (ret)
> >  			return ret;
> > +	} else if (options_.isSet(OptMonitor)) {
> > +		cm_->cameraAdded.connect(this, &CamApp::cameraAdded);
> > +		cm_->cameraRemoved.connect(this, &CamApp::cameraRemoved);
> > +		std::cout << "Monitoring new hotplug or unplug camera events…" << std::endl;
> >  	}
> >  
> >  	loop_ = new EventLoop(cm_->eventDispatcher());
> > @@ -189,6 +195,8 @@ int CamApp::parseOptions(int argc, char *argv[])
> >  	parser.addOption(OptStrictFormats, OptionNone,
> >  			 "Do not allow requested stream format(s) to be adjusted",
> >  			 "strict-formats");
> > +	parser.addOption(OptMonitor, OptionNone, "Monitor for hotplug and unplug camera events",
> 
> I'd move the message to the next line to avoid too long lines.
> 
> > +			 "monitor");
> >  
> >  	options_ = parser.parse(argc, argv);
> >  	if (!options_.valid())
> > @@ -309,6 +317,16 @@ int CamApp::infoConfiguration()
> >  	return 0;
> >  }
> >  
> > +void CamApp::cameraAdded(std::shared_ptr<Camera> cam)
> > +{
> > +	std::cout << "Camera Added: " << cam->name() << std::endl;
> > +}
> > +
> > +void CamApp::cameraRemoved(std::shared_ptr<Camera> cam)
> > +{
> > +	std::cout << "Camera Removed: " << cam->name() << std::endl;
> > +}
> > +
> >  int CamApp::run()
> >  {
> >  	int ret;
> > @@ -342,10 +360,18 @@ int CamApp::run()
> >  	}
> >  
> >  	if (options_.isSet(OptCapture)) {
> > +		cm_->cameraAdded.connect(this, &CamApp::cameraAdded);
> > +		cm_->cameraRemoved.connect(this, &CamApp::cameraRemoved);
> 
> This is done in CamApp::init() already, do we need it here ? One can
> specific both --monitor and --capture if they want to monitor for
> hotplug during capture, I wouldn't make it automatic.

And for this to work of course, the else if should be turned into an if
in CamApp::init().

> >  		Capture capture(camera_, config_.get(), loop_);
> >  		return capture.run(options_);
> >  	}
> >  
> > +	if (options_.isSet(OptMonitor)) {
> > +		ret = loop_->exec();
> > +		if (ret)
> > +			std::cout << "Failed to run monitor loop" << std::endl;
> > +	}
> > +
> >  	return 0;
> >  }
> >  
> > diff --git a/src/cam/main.h b/src/cam/main.h
> > index 6f95add..ea8dfd3 100644
> > --- a/src/cam/main.h
> > +++ b/src/cam/main.h
> > @@ -15,6 +15,7 @@ enum {
> >  	OptInfo = 'I',
> >  	OptList = 'l',
> >  	OptListProperties = 'p',
> > +	OptMonitor = 'm',
> >  	OptStream = 's',
> >  	OptListControls = 256,
> >  	OptStrictFormats = 257,

Hi Laurent,

On 8/1/20 2:08 AM, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> On Fri, Jul 31, 2020 at 07:46:54PM +0000, Umang Jain wrote:
>> Add --monitor to monitor new hotplug and unplug camera events from
>> the CameraManager.
>>
>> Signed-off-by: Umang Jain <email@uajain.com>
>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>   src/cam/main.cpp | 26 ++++++++++++++++++++++++++
>>   src/cam/main.h   |  1 +
>>   2 files changed, 27 insertions(+)
>>
>> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
>> index f5aba04..b03437e 100644
>> --- a/src/cam/main.cpp
>> +++ b/src/cam/main.cpp
>> @@ -36,6 +36,8 @@ public:
>>   	void quit();
>>   
>>   private:
>> +	void cameraAdded(std::shared_ptr<Camera> cam);
>> +	void cameraRemoved(std::shared_ptr<Camera> cam);
>>   	int parseOptions(int argc, char *argv[]);
>>   	int prepareConfig();
>>   	int listControls();
>> @@ -121,6 +123,10 @@ int CamApp::init(int argc, char **argv)
>>   		ret = prepareConfig();
>>   		if (ret)
>>   			return ret;
>> +	} else if (options_.isSet(OptMonitor)) {
>> +		cm_->cameraAdded.connect(this, &CamApp::cameraAdded);
>> +		cm_->cameraRemoved.connect(this, &CamApp::cameraRemoved);
>> +		std::cout << "Monitoring new hotplug or unplug camera events…" << std::endl;
>>   	}
>>   
>>   	loop_ = new EventLoop(cm_->eventDispatcher());
>> @@ -189,6 +195,8 @@ int CamApp::parseOptions(int argc, char *argv[])
>>   	parser.addOption(OptStrictFormats, OptionNone,
>>   			 "Do not allow requested stream format(s) to be adjusted",
>>   			 "strict-formats");
>> +	parser.addOption(OptMonitor, OptionNone, "Monitor for hotplug and unplug camera events",
> I'd move the message to the next line to avoid too long lines.
>
>> +			 "monitor");
>>   
>>   	options_ = parser.parse(argc, argv);
>>   	if (!options_.valid())
>> @@ -309,6 +317,16 @@ int CamApp::infoConfiguration()
>>   	return 0;
>>   }
>>   
>> +void CamApp::cameraAdded(std::shared_ptr<Camera> cam)
>> +{
>> +	std::cout << "Camera Added: " << cam->name() << std::endl;
>> +}
>> +
>> +void CamApp::cameraRemoved(std::shared_ptr<Camera> cam)
>> +{
>> +	std::cout << "Camera Removed: " << cam->name() << std::endl;
>> +}
>> +
>>   int CamApp::run()
>>   {
>>   	int ret;
>> @@ -342,10 +360,18 @@ int CamApp::run()
>>   	}
>>   
>>   	if (options_.isSet(OptCapture)) {
>> +		cm_->cameraAdded.connect(this, &CamApp::cameraAdded);
>> +		cm_->cameraRemoved.connect(this, &CamApp::cameraRemoved);
> This is done in CamApp::init() already, do we need it here ? One can
> specific both --monitor and --capture if they want to monitor for
> hotplug during capture, I wouldn't make it automatic.
hmm, one of reviews from Kieran in v1 was to make it automatic, if we 
run any
kind of loop (like Capture), hence the patch got driven in that direction.

I think specifying --monitor (shorthand -m) explicitly shouldn't be that 
big a deal.


>
>>   		Capture capture(camera_, config_.get(), loop_);
>>   		return capture.run(options_);
>>   	}
>>   
>> +	if (options_.isSet(OptMonitor)) {
>> +		ret = loop_->exec();
>> +		if (ret)
>> +			std::cout << "Failed to run monitor loop" << std::endl;
>> +	}
>> +
>>   	return 0;
>>   }
>>   
>> diff --git a/src/cam/main.h b/src/cam/main.h
>> index 6f95add..ea8dfd3 100644
>> --- a/src/cam/main.h
>> +++ b/src/cam/main.h
>> @@ -15,6 +15,7 @@ enum {
>>   	OptInfo = 'I',
>>   	OptList = 'l',
>>   	OptListProperties = 'p',
>> +	OptMonitor = 'm',
>>   	OptStream = 's',
>>   	OptListControls = 256,
>>   	OptStrictFormats = 257,

Hi Umang,

CC'ing Kieran,

On Sat, Aug 01, 2020 at 10:19:11AM +0000, Umang Jain wrote:
> On 8/1/20 2:08 AM, Laurent Pinchart wrote:
> > On Fri, Jul 31, 2020 at 07:46:54PM +0000, Umang Jain wrote:
> >> Add --monitor to monitor new hotplug and unplug camera events from
> >> the CameraManager.
> >>
> >> Signed-off-by: Umang Jain <email@uajain.com>
> >> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >> ---
> >>   src/cam/main.cpp | 26 ++++++++++++++++++++++++++
> >>   src/cam/main.h   |  1 +
> >>   2 files changed, 27 insertions(+)
> >>
> >> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> >> index f5aba04..b03437e 100644
> >> --- a/src/cam/main.cpp
> >> +++ b/src/cam/main.cpp
> >> @@ -36,6 +36,8 @@ public:
> >>   	void quit();
> >>   
> >>   private:
> >> +	void cameraAdded(std::shared_ptr<Camera> cam);
> >> +	void cameraRemoved(std::shared_ptr<Camera> cam);
> >>   	int parseOptions(int argc, char *argv[]);
> >>   	int prepareConfig();
> >>   	int listControls();
> >> @@ -121,6 +123,10 @@ int CamApp::init(int argc, char **argv)
> >>   		ret = prepareConfig();
> >>   		if (ret)
> >>   			return ret;
> >> +	} else if (options_.isSet(OptMonitor)) {
> >> +		cm_->cameraAdded.connect(this, &CamApp::cameraAdded);
> >> +		cm_->cameraRemoved.connect(this, &CamApp::cameraRemoved);
> >> +		std::cout << "Monitoring new hotplug or unplug camera events…" << std::endl;
> >>   	}
> >>   
> >>   	loop_ = new EventLoop(cm_->eventDispatcher());
> >> @@ -189,6 +195,8 @@ int CamApp::parseOptions(int argc, char *argv[])
> >>   	parser.addOption(OptStrictFormats, OptionNone,
> >>   			 "Do not allow requested stream format(s) to be adjusted",
> >>   			 "strict-formats");
> >> +	parser.addOption(OptMonitor, OptionNone, "Monitor for hotplug and unplug camera events",
> >
> > I'd move the message to the next line to avoid too long lines.
> >
> >> +			 "monitor");
> >>   
> >>   	options_ = parser.parse(argc, argv);
> >>   	if (!options_.valid())
> >> @@ -309,6 +317,16 @@ int CamApp::infoConfiguration()
> >>   	return 0;
> >>   }
> >>   
> >> +void CamApp::cameraAdded(std::shared_ptr<Camera> cam)
> >> +{
> >> +	std::cout << "Camera Added: " << cam->name() << std::endl;
> >> +}
> >> +
> >> +void CamApp::cameraRemoved(std::shared_ptr<Camera> cam)
> >> +{
> >> +	std::cout << "Camera Removed: " << cam->name() << std::endl;
> >> +}
> >> +
> >>   int CamApp::run()
> >>   {
> >>   	int ret;
> >> @@ -342,10 +360,18 @@ int CamApp::run()
> >>   	}
> >>   
> >>   	if (options_.isSet(OptCapture)) {
> >> +		cm_->cameraAdded.connect(this, &CamApp::cameraAdded);
> >> +		cm_->cameraRemoved.connect(this, &CamApp::cameraRemoved);
> >
> > This is done in CamApp::init() already, do we need it here ? One can
> > specific both --monitor and --capture if they want to monitor for
> > hotplug during capture, I wouldn't make it automatic.
>
> hmm, one of reviews from Kieran in v1 was to make it automatic, if we 
> run any
> kind of loop (like Capture), hence the patch got driven in that direction.
> 
> I think specifying --monitor (shorthand -m) explicitly shouldn't be that 
> big a deal.

I'll let Kieran comment on this before merging the patch.

> >>   		Capture capture(camera_, config_.get(), loop_);
> >>   		return capture.run(options_);
> >>   	}
> >>   
> >> +	if (options_.isSet(OptMonitor)) {
> >> +		ret = loop_->exec();
> >> +		if (ret)
> >> +			std::cout << "Failed to run monitor loop" << std::endl;
> >> +	}
> >> +
> >>   	return 0;
> >>   }
> >>   
> >> diff --git a/src/cam/main.h b/src/cam/main.h
> >> index 6f95add..ea8dfd3 100644
> >> --- a/src/cam/main.h
> >> +++ b/src/cam/main.h
> >> @@ -15,6 +15,7 @@ enum {
> >>   	OptInfo = 'I',
> >>   	OptList = 'l',
> >>   	OptListProperties = 'p',
> >> +	OptMonitor = 'm',
> >>   	OptStream = 's',
> >>   	OptListControls = 256,
> >>   	OptStrictFormats = 257,

Hi Umang, Laurent,

On 01/08/2020 13:53, Laurent Pinchart wrote:
> Hi Umang,
> 
> CC'ing Kieran,
> 
> On Sat, Aug 01, 2020 at 10:19:11AM +0000, Umang Jain wrote:
>> On 8/1/20 2:08 AM, Laurent Pinchart wrote:
>>> On Fri, Jul 31, 2020 at 07:46:54PM +0000, Umang Jain wrote:
>>>> Add --monitor to monitor new hotplug and unplug camera events from
>>>> the CameraManager.
>>>>
>>>> Signed-off-by: Umang Jain <email@uajain.com>
>>>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
>>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>>> ---
>>>>   src/cam/main.cpp | 26 ++++++++++++++++++++++++++
>>>>   src/cam/main.h   |  1 +
>>>>   2 files changed, 27 insertions(+)
>>>>
>>>> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
>>>> index f5aba04..b03437e 100644
>>>> --- a/src/cam/main.cpp
>>>> +++ b/src/cam/main.cpp
>>>> @@ -36,6 +36,8 @@ public:
>>>>   	void quit();
>>>>   
>>>>   private:
>>>> +	void cameraAdded(std::shared_ptr<Camera> cam);
>>>> +	void cameraRemoved(std::shared_ptr<Camera> cam);
>>>>   	int parseOptions(int argc, char *argv[]);
>>>>   	int prepareConfig();
>>>>   	int listControls();
>>>> @@ -121,6 +123,10 @@ int CamApp::init(int argc, char **argv)
>>>>   		ret = prepareConfig();
>>>>   		if (ret)
>>>>   			return ret;
>>>> +	} else if (options_.isSet(OptMonitor)) {
>>>> +		cm_->cameraAdded.connect(this, &CamApp::cameraAdded);
>>>> +		cm_->cameraRemoved.connect(this, &CamApp::cameraRemoved);
>>>> +		std::cout << "Monitoring new hotplug or unplug camera events…" << std::endl;
>>>>   	}

My comment previously was that I think we should always connect these
signals in this function (without an 'if isSet(OptMonitor)') so that any
time cam is running, if there is an event which makes it to the
application the user is notified.


>>>>   
>>>>   	loop_ = new EventLoop(cm_->eventDispatcher());
>>>> @@ -189,6 +195,8 @@ int CamApp::parseOptions(int argc, char *argv[])
>>>>   	parser.addOption(OptStrictFormats, OptionNone,
>>>>   			 "Do not allow requested stream format(s) to be adjusted",
>>>>   			 "strict-formats");
>>>> +	parser.addOption(OptMonitor, OptionNone, "Monitor for hotplug and unplug camera events",
>>>
>>> I'd move the message to the next line to avoid too long lines.
>>>
>>>> +			 "monitor");
>>>>   
>>>>   	options_ = parser.parse(argc, argv);
>>>>   	if (!options_.valid())
>>>> @@ -309,6 +317,16 @@ int CamApp::infoConfiguration()
>>>>   	return 0;
>>>>   }
>>>>   
>>>> +void CamApp::cameraAdded(std::shared_ptr<Camera> cam)
>>>> +{
>>>> +	std::cout << "Camera Added: " << cam->name() << std::endl;
>>>> +}
>>>> +
>>>> +void CamApp::cameraRemoved(std::shared_ptr<Camera> cam)
>>>> +{
>>>> +	std::cout << "Camera Removed: " << cam->name() << std::endl;
>>>> +}
>>>> +
>>>>   int CamApp::run()
>>>>   {
>>>>   	int ret;
>>>> @@ -342,10 +360,18 @@ int CamApp::run()
>>>>   	}
>>>>   
>>>>   	if (options_.isSet(OptCapture)) {
>>>> +		cm_->cameraAdded.connect(this, &CamApp::cameraAdded);
>>>> +		cm_->cameraRemoved.connect(this, &CamApp::cameraRemoved);
>>>
>>> This is done in CamApp::init() already, do we need it here ? One can
>>> specific both --monitor and --capture if they want to monitor for
>>> hotplug during capture, I wouldn't make it automatic.
>>
>> hmm, one of reviews from Kieran in v1 was to make it automatic, if we 
>> run any
>> kind of loop (like Capture), hence the patch got driven in that direction.
>>
>> I think specifying --monitor (shorthand -m) explicitly shouldn't be that 
>> big a deal.
> 
> I'll let Kieran comment on this before merging the patch.

Thus, if the connections are done in CamApp::init() (always, regardless
of the -m) then we don't need to reconnect here ...


> 
>>>>   		Capture capture(camera_, config_.get(), loop_);
>>>>   		return capture.run(options_);
>>>>   	}
>>>>   
>>>> +	if (options_.isSet(OptMonitor)) {
>>>> +		ret = loop_->exec();
>>>> +		if (ret)
>>>> +			std::cout << "Failed to run monitor loop" << std::endl;
>>>> +	}

But we do still need to provide a loop.

I'm not sure I'm keen on the idea of mixing --monitor and --capture as a
requirement to obtain notifications during a capture. I think it should
always notify. The '--monitor' is just a convenience to provide an event
loop which will do nothing else except wait for events...

Any thoughts anyone?

I don't mind if the consensus is that to see notifications while doing a
capture you should still specify the '-m', because the connection will
already be done (conditionally in init), and we will be running in an
event loop in 'return capture.run(options_);' which will return when it
completes.


So if consensus is "to get events, then the -m option should be
provided", then v1 of this patch already does that.

Otherwise, if we should always report hotplug events, then v1 just needs
to be adjusted to unconditionally connect the signal/slots during
CamApp::init.


Personally, I would choose to always display the events. They are cheap,
and the monitor loop becomes just a convenience.


--
Kieran



>>>> +
>>>>   	return 0;
>>>>   }
>>>>   
>>>> diff --git a/src/cam/main.h b/src/cam/main.h
>>>> index 6f95add..ea8dfd3 100644
>>>> --- a/src/cam/main.h
>>>> +++ b/src/cam/main.h
>>>> @@ -15,6 +15,7 @@ enum {
>>>>   	OptInfo = 'I',
>>>>   	OptList = 'l',
>>>>   	OptListProperties = 'p',
>>>> +	OptMonitor = 'm',
>>>>   	OptStream = 's',
>>>>   	OptListControls = 256,
>>>>   	OptStrictFormats = 257,
>

Hi Kieran,

On Mon, Aug 03, 2020 at 10:01:09AM +0100, Kieran Bingham wrote:
> On 01/08/2020 13:53, Laurent Pinchart wrote:
> > On Sat, Aug 01, 2020 at 10:19:11AM +0000, Umang Jain wrote:
> >> On 8/1/20 2:08 AM, Laurent Pinchart wrote:
> >>> On Fri, Jul 31, 2020 at 07:46:54PM +0000, Umang Jain wrote:
> >>>> Add --monitor to monitor new hotplug and unplug camera events from
> >>>> the CameraManager.
> >>>>
> >>>> Signed-off-by: Umang Jain <email@uajain.com>
> >>>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> >>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >>>> ---
> >>>>   src/cam/main.cpp | 26 ++++++++++++++++++++++++++
> >>>>   src/cam/main.h   |  1 +
> >>>>   2 files changed, 27 insertions(+)
> >>>>
> >>>> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> >>>> index f5aba04..b03437e 100644
> >>>> --- a/src/cam/main.cpp
> >>>> +++ b/src/cam/main.cpp
> >>>> @@ -36,6 +36,8 @@ public:
> >>>>   	void quit();
> >>>>   
> >>>>   private:
> >>>> +	void cameraAdded(std::shared_ptr<Camera> cam);
> >>>> +	void cameraRemoved(std::shared_ptr<Camera> cam);
> >>>>   	int parseOptions(int argc, char *argv[]);
> >>>>   	int prepareConfig();
> >>>>   	int listControls();
> >>>> @@ -121,6 +123,10 @@ int CamApp::init(int argc, char **argv)
> >>>>   		ret = prepareConfig();
> >>>>   		if (ret)
> >>>>   			return ret;
> >>>> +	} else if (options_.isSet(OptMonitor)) {
> >>>> +		cm_->cameraAdded.connect(this, &CamApp::cameraAdded);
> >>>> +		cm_->cameraRemoved.connect(this, &CamApp::cameraRemoved);
> >>>> +		std::cout << "Monitoring new hotplug or unplug camera events…" << std::endl;
> >>>>   	}
> 
> My comment previously was that I think we should always connect these
> signals in this function (without an 'if isSet(OptMonitor)') so that any
> time cam is running, if there is an event which makes it to the
> application the user is notified.
> 
> >>>>   
> >>>>   	loop_ = new EventLoop(cm_->eventDispatcher());
> >>>> @@ -189,6 +195,8 @@ int CamApp::parseOptions(int argc, char *argv[])
> >>>>   	parser.addOption(OptStrictFormats, OptionNone,
> >>>>   			 "Do not allow requested stream format(s) to be adjusted",
> >>>>   			 "strict-formats");
> >>>> +	parser.addOption(OptMonitor, OptionNone, "Monitor for hotplug and unplug camera events",
> >>>
> >>> I'd move the message to the next line to avoid too long lines.
> >>>
> >>>> +			 "monitor");
> >>>>   
> >>>>   	options_ = parser.parse(argc, argv);
> >>>>   	if (!options_.valid())
> >>>> @@ -309,6 +317,16 @@ int CamApp::infoConfiguration()
> >>>>   	return 0;
> >>>>   }
> >>>>   
> >>>> +void CamApp::cameraAdded(std::shared_ptr<Camera> cam)
> >>>> +{
> >>>> +	std::cout << "Camera Added: " << cam->name() << std::endl;
> >>>> +}
> >>>> +
> >>>> +void CamApp::cameraRemoved(std::shared_ptr<Camera> cam)
> >>>> +{
> >>>> +	std::cout << "Camera Removed: " << cam->name() << std::endl;
> >>>> +}
> >>>> +
> >>>>   int CamApp::run()
> >>>>   {
> >>>>   	int ret;
> >>>> @@ -342,10 +360,18 @@ int CamApp::run()
> >>>>   	}
> >>>>   
> >>>>   	if (options_.isSet(OptCapture)) {
> >>>> +		cm_->cameraAdded.connect(this, &CamApp::cameraAdded);
> >>>> +		cm_->cameraRemoved.connect(this, &CamApp::cameraRemoved);
> >>>
> >>> This is done in CamApp::init() already, do we need it here ? One can
> >>> specific both --monitor and --capture if they want to monitor for
> >>> hotplug during capture, I wouldn't make it automatic.
> >>
> >> hmm, one of reviews from Kieran in v1 was to make it automatic, if we 
> >> run any
> >> kind of loop (like Capture), hence the patch got driven in that direction.
> >>
> >> I think specifying --monitor (shorthand -m) explicitly shouldn't be that 
> >> big a deal.
> > 
> > I'll let Kieran comment on this before merging the patch.
> 
> Thus, if the connections are done in CamApp::init() (always, regardless
> of the -m) then we don't need to reconnect here ...
> 
> >>>>   		Capture capture(camera_, config_.get(), loop_);
> >>>>   		return capture.run(options_);
> >>>>   	}
> >>>>   
> >>>> +	if (options_.isSet(OptMonitor)) {
> >>>> +		ret = loop_->exec();
> >>>> +		if (ret)
> >>>> +			std::cout << "Failed to run monitor loop" << std::endl;
> >>>> +	}
> 
> But we do still need to provide a loop.
> 
> I'm not sure I'm keen on the idea of mixing --monitor and --capture as a
> requirement to obtain notifications during a capture. I think it should
> always notify. The '--monitor' is just a convenience to provide an event
> loop which will do nothing else except wait for events...
> 
> Any thoughts anyone?
> 
> I don't mind if the consensus is that to see notifications while doing a
> capture you should still specify the '-m', because the connection will
> already be done (conditionally in init), and we will be running in an
> event loop in 'return capture.run(options_);' which will return when it
> completes.
> 
> 
> So if consensus is "to get events, then the -m option should be
> provided", then v1 of this patch already does that.

The reason why I think -m should be mandatory to listen to hotplug
events is that the cam application is meant to be a low-level test
application that can exercise the whole libcamera API. Having the
ability to capture without listening to hotplug events could be useful.
It would be a different story in a more specialized application, but for
cam I think it's best to let the user decide what they need.

> Otherwise, if we should always report hotplug events, then v1 just needs
> to be adjusted to unconditionally connect the signal/slots during
> CamApp::init.
> 
> 
> Personally, I would choose to always display the events. They are cheap,
> and the monitor loop becomes just a convenience.
> 
> >>>> +
> >>>>   	return 0;
> >>>>   }
> >>>>   
> >>>> diff --git a/src/cam/main.h b/src/cam/main.h
> >>>> index 6f95add..ea8dfd3 100644
> >>>> --- a/src/cam/main.h
> >>>> +++ b/src/cam/main.h
> >>>> @@ -15,6 +15,7 @@ enum {
> >>>>   	OptInfo = 'I',
> >>>>   	OptList = 'l',
> >>>>   	OptListProperties = 'p',
> >>>> +	OptMonitor = 'm',
> >>>>   	OptStream = 's',
> >>>>   	OptListControls = 256,
> >>>>   	OptStrictFormats = 257,