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

[Umang Jain]

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 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.
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,