[{"id":4754,"web_url":"https://patchwork.libcamera.org/comment/4754/","msgid":"<20200506203137.GE15206@pendragon.ideasonboard.com>","date":"2020-05-06T20:31:37","subject":"Re: [libcamera-devel] [PATCH 1/4] libcamera: device_enumerator:\n\tEmit a signal when a new device is hotplugged","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Wed, May 06, 2020 at 10:33:52AM +0000, Umang Jain wrote:\n> Emit a signal whenever is a new MediaDevice is added to the\n> DeviceEnumerator. This will allow CameraManager to get notified\n> about the new devices that have been hot-plugged.\n> \n> Signed-off-by: Umang Jain \n> ---\n> src/libcamera/camera_manager.cpp | 27 +++++++++++++++++++++--\n> src/libcamera/device_enumerator.cpp | 10 +++++++++\n> src/libcamera/include/device_enumerator.h | 3 +++\n> 3 files changed, 38 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> index fddf734..c75979a 100644\n> --- a/src/libcamera/camera_manager.cpp\n> +++ b/src/libcamera/camera_manager.cpp\n> @@ -54,6 +54,7 @@ protected:\n> private:\n> \tint init();\n> \tvoid cleanup();\n> +\tvoid enumerateNewDevices(DeviceEnumerator *enumerator);\n\nHow about calling this enumerateDevices(), and calling it from\nCameraManager::Private::init() ? Please move the TODO comment to this\nfunction (and while at it, s/TODO:/\\\\todo/\n\n> \n> \tCameraManager *cm_;\n> \n> @@ -144,14 +145,36 @@ int CameraManager::Private::init()\n> \t\t}\n> \t}\n> \n> -\t/* TODO: register hot-plug callback here */\n> +\tenumerator_->newDevicesFound.connect(this, &CameraManager::Private::enumerateNewDevices);\n\nYou can shorten this line to\n\n\tenumerator_->newDevicesFound.connect(this, &Private::enumerateNewDevices);\n\n> \n> \treturn 0;\n> }\n> \n> +void CameraManager::Private::enumerateNewDevices(DeviceEnumerator *enumerator)\n> +{\n> +\tstd::vector &factories = PipelineHandlerFactory::factories();\n> +\n> +\tfor (PipelineHandlerFactory *factory : factories) {\n> +\t\t/*\n> +\t\t * Try each pipeline handler until it exhaust\n> +\t\t * all pipelines it can provide.\n> +\t\t */\n> +\t\twhile (1) {\n> +\t\t\tstd::shared_ptr pipe = factory->create(cm_);\n> +\t\t\tif (!pipe->match(enumerator_.get()))\n> +\t\t\t\tbreak;\n> +\n> +\t\t\tLOG(Camera, Debug)\n> +\t\t\t\t<< \"Pipeline handler \\\"\" << factory->name()\n> +\t\t\t\t<< \"\\\" matched\";\n> +\t\t\tpipes_.push_back(std::move(pipe));\n> +\t\t}\n> +\t}\n> +}\n> +\n> void CameraManager::Private::cleanup()\n> {\n> -\t/* TODO: unregister hot-plug callback here */\n> +\tenumerator_->newDevicesFound.disconnect(this, &CameraManager::Private::enumerateNewDevices);\n\nAnd this one to\n\n\tenumerator_->newDevicesFound.disconnect(this, &Private::enumerateNewDevices);\n\n> \n> \t/*\n> \t * Release all references to cameras and pipeline handlers to ensure\n> diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp\n> index dd17e3e..2721120 100644\n> --- a/src/libcamera/device_enumerator.cpp\n> +++ b/src/libcamera/device_enumerator.cpp\n> @@ -227,6 +227,15 @@ std::unique_ptr DeviceEnumerator::createDevice(const std::string &d\n> \treturn media;\n> }\n> \n> +/**\n> + * \\var DeviceEnumerator::newDevicesFound\n> + * \\brief Signal emitted when a new MediaDevice is added to the DeviceEnumerator\n> + *\n> + * CameraManager connects to this signal to know about new MediaDevices being plugged-in,\n> + * while it is running. CameraManager will then iterate over the DeviceEnumerator, to match\n> + * their respective pipeline-handlers and prepare the newly plugged-in device for use.\n\nCould you please wrap lines at a 80 columns limit ? Up to 120 columns\nare accepted when that improves readability, but otherwise we try to\nkeep lines at most 80 characters long.\n\nI think this text should also be reworked a bit to focus on the\nDeviceEnumerator side of it. Signals are used to connected components\nthat are not necessarily tightly integrated, and thus should not make\ntoo many assumptions (and ideally no assumption at all) regarding what\nthey will be connected to. We could decide tomorrow to rework how the\nDeviceEnumerator is used, without changing how it's implemented, and\nconnect the signal to something else. How about the following ?\n\n * \\brief Notify of new media devices being found\n *\n * This signal is emitted when the device enumerator finds new media devices in\n * the system. It may be emitted for every newly detected device, or once for\n * multiple of devices, at the discretion of the device enumerator. Not all\n * device enumerator types may support dynamic detection of new devices.\n\nAnd this brings another comment I wanted to make: As matching pipeline\nhandlers with media devices is an expensive operation, it could be nice\nto emit the signal with a small delay, to batch multiple devices\ntogether. That won't bring much improvement for UVC, but for other types\nof cameras that could be exposed as multiple media devices, all handled\nby a single pipeline handler instance, it could be beneficial. I don't\nthink it needs to be implemented as part of this series though, it's\nlikely overkill for now.\n\n> + */\n> +\n> /**\n> * \\brief Add a media device to the enumerator\n> * \\param[in] media media device instance to add\n> @@ -242,6 +251,7 @@ void DeviceEnumerator::addDevice(std::unique_ptr &&media)\n> \t\t<< \"Added device \" << media->deviceNode() << \": \" << media->driver();\n> \n> \tdevices_.push_back(std::move(media));\n> +\tnewDevicesFound.emit(this);\n> }\n> \n> /**\n> diff --git a/src/libcamera/include/device_enumerator.h b/src/libcamera/include/device_enumerator.h\n> index 433e357..6cc6ec2 100644\n> --- a/src/libcamera/include/device_enumerator.h\n> +++ b/src/libcamera/include/device_enumerator.h\n> @@ -11,6 +11,7 @@\n> #include \n> #include \n> \n> +#include \n> #include \n\nThis should be\n\n#include \n\n#include \n\n(see http://libcamera.org/coding-style.html#order-of-includes)\n\n> \n> namespace libcamera {\n> @@ -43,6 +44,8 @@ public:\n> \n> \tstd::shared_ptr search(const DeviceMatch &dm);\n> \n> +\tSignal newDevicesFound;\n\nI think you can drop the DeviceEnumerator argument to the signal.\n\n\tSignal<> newDevicesFound;\n\n> +\n> protected:\n> \tstd::unique_ptr createDevice(const std::string &deviceNode);\n> \tvoid addDevice(std::unique_ptr &&media);","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 45D51603F2\n\tfor ;\n\tWed, 6 May 2020 22:31:43 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B9F2E542;\n\tWed, 6 May 2020 22:31:42 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"T6jU1tdr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1588797102;\n\tbh=cpomTxgIbza5Ke9mzb9AIzKNK5IcF6bj10KGNpkpQBA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=T6jU1tdr/TmapeibsGc7BfboiVdgaOBl/q+YdSXkoJGacMSzvCldg2kWWyiUrG+6W\n\tolARzXRcDGy34nWVcSEUytmxZmEQSDBTjE6Vk445FCimMBnlZeJbvbCCobyUet6WeI\n\tsPAdW5NinEbp0eQ7e7Fe+S6cWlW8F37jLEf1MVMA=","Date":"Wed, 6 May 2020 23:31:37 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Cc":"libcamera-devel ","Message-ID":"<20200506203137.GE15206@pendragon.ideasonboard.com>","References":"<20200506103346.3433-1-email@uajain.com>\n\t<20200506103346.3433-2-email@uajain.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200506103346.3433-2-email@uajain.com>","Subject":"Re: [libcamera-devel] [PATCH 1/4] libcamera: device_enumerator:\n\tEmit a signal when a new device is hotplugged","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 06 May 2020 20:31:43 -0000"}}]