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

[Laurent Pinchart]

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 at uajain.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > Reviewed-by: Kieran Bingham <kieran.bingham at 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,

-- 
Regards,

Laurent Pinchart