[{"id":183,"web_url":"https://patchwork.libcamera.org/comment/183/","msgid":"<4352826.R2XpCxzena@avalon>","date":"2019-01-02T20:59:02","subject":"Re: [libcamera-devel] [PATCH] cam-ctl: add utility to control\n\tcameras","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Saturday, 29 December 2018 05:31:34 EET Niklas Söderlund wrote:\n> Provide a utility to interact with cameras. This initial state is\n> limited and only supports listing cameras in the system and selecting a\n> camera to interact with.\n> \n> There is not much a interacting possible yet with a camera so the tool\n> simply exercise the API to get and put a camera.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> src/cam-ctl/main.cpp | 98 +++++++++++++++++++++++++++++++++++++++++\n> src/cam-ctl/meson.build | 7 +++\n> src/meson.build | 1 +\n> 3 files changed, 106 insertions(+)\n> create mode 100644 src/cam-ctl/main.cpp\n> create mode 100644 src/cam-ctl/meson.build\n> \n> ---\n> \n> Hi,\n> \n> I'm not sure this is ready to be merged but I thought it good to share\n> it on the ML to avoid redoing the work as more pieces come together.\n> \n> diff --git a/src/cam-ctl/main.cpp b/src/cam-ctl/main.cpp\n> new file mode 100644\n> index 0000000000000000..0bf76cef3efc8741\n> --- /dev/null\n> +++ b/src/cam-ctl/main.cpp\n> @@ -0,0 +1,98 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n> + *\n> + * main.cpp - cam-ctl a tool to interact with the library\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +using namespace std;\n> +using namespace libcamera;\n> +\n> +enum Option {\n> +\tOptCamera = 1,\n> +\tOptList,\n> +\tOptLast,\n> +};\n> +\n> +static struct option long_options[] = {\n> +\t{ \"camera\", required_argument, 0, 'c' },\n> +\t{ \"list\", no_argument, 0, 'l' },\n\nPlease add -h/--help.\n\n> +};\n> +\n> +char options[OptLast];\n> +\n> +void usage()\n> +{\n> +\tcout << \"Options:\" << endl;\n> +\tcout << \" --camera \" << endl;\n> +\tcout << \" --list\" << endl;\n\nIt would be nice if the help text could be generated from the long_options \narray. We could create a new option class that would serve for both getopt and \nhelp text, but I fear we'd enter development of a new argument parsing library \n:-) On the other hand, given that this tool will become the swiss army command \nline knife of libcamera, it may be worth it as it will grow a plethora of \noptions.\n\nIn any case, you should document the short options too, and add a description \nfor each option.\n\n> +}\n> +\n> +int main(int argc, char **argv)\n> +{\n> +\tCameraManager *cm;\n\nYou could create this on the stack instead of allocating it from the heap.\n\n> +\tstring camera;\n> +\n> +\tif (argc == 1) {\n> +\t\tusage();\n> +\t\treturn 0;\n> +\t}\n> +\n> +\twhile (1) {\n> +\t\tint c, option_index = 0;\n\nDo you need to initialize option_index to 0 ? And as you don't use it, you \ncould just pass NULL as the last argument of getopt_long().\n\n> +\t\tc = getopt_long (argc, argv, \"c:l\", long_options, &option_index);\n> +\n\ns/ (/(/\n\nAnd no need for a blank line here.\n\n> +\t\tif (c == -1)\n> +\t\t\tbreak;\n> +\n> +\t\tswitch (c) {\n> +\t\tcase 'c':\n> +\t\t\toptions[OptCamera] = 1;\n> +\t\t\tcamera = optarg;\n> +\t\t\tbreak;\n> +\t\tcase 'l':\n> +\t\t\toptions[OptList] = 1;\n> +\t\t\tbreak;\n\nI would create an options structure that stores all options, and move option \nparsing to a separate function. The rest of the code should then operate on \nthe options structure.\n\n> +\t\tcase '?':\n> +\t\t\tbreak;\n> +\t\tdefault:\n> +\t\t\tabort();\n\nWhy abort() ?\n\n> +\t\t}\n> +\t}\n> +\n> +\tcm = new CameraManager();\n> +\tif (!cm)\n> +\t\treturn -ENOMEM;\n> +\n> +\tcm->start();\n\nNo error handling ?\n\n> +\tif (options[OptList]) {\n\nMaybe an header stating \"Available cameras:\" ?\n\n> +\t\tfor (auto name : cm->list())\n\nYou can spell out the type instead of using auto.\n\n> +\t\t\tcout << \"- \" << name << endl;\n> +\t}\n> +\n> +\tif (options[OptCamera]) {\n> +\t\tCamera *cam = cm->get(camera);\n> +\n> +\t\tif (cam) {\n> +\t\t\tcout << \"Using camera: \" << cam->name() << endl;\n\ns/://\n\n> +\t\t\tcam->put();\n> +\t\t} else {\n> +\t\t\tcout << \"Can't find camera '\" << camera << \"'\" << endl;\n\ncout << \"Camera \" << camera << \" not found\" << endl; ?\n\n> +\t\t}\n> +\t}\n> +\n> +\tcm->stop();\n> +\n> +\tdelete cm;\n> +\n> +\treturn 0;\n> +}\n> diff --git a/src/cam-ctl/meson.build b/src/cam-ctl/meson.build\n> new file mode 100644\n> index 0000000000000000..7c27a19dae98fe96\n> --- /dev/null\n> +++ b/src/cam-ctl/meson.build\n> @@ -0,0 +1,7 @@\n> +cam_ctl_sources = files([\n> + 'main.cpp',\n> +])\n> +\n> +cam_ctl = executable('cam-ctl', cam_ctl_sources,\n> + link_with : libcamera,\n> + include_directories : libcamera_includes)\n\nLet's call this cam and claim the name :-)\n\n> diff --git a/src/meson.build b/src/meson.build\n> index 4ce9668caa7b8997..86e5290e5e66eb68 100644\n> --- a/src/meson.build\n> +++ b/src/meson.build\n> @@ -1 +1,2 @@\n> subdir('libcamera')\n> +subdir('cam-ctl')","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 76C0C60B2F\n\tfor ;\n\tWed, 2 Jan 2019 21:58:01 +0100 (CET)","from avalon.localnet (dfj612ybrt5fhg77mgycy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:2e86:4862:ef6a:2804])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DC714505;\n\tWed, 2 Jan 2019 21:58:00 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1546462681;\n\tbh=AQZmu9l+/U71T/IEF0MbP05T5j3XNoeMvytHPbKLH2Q=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=Zo/AT9rN8B3iSqaVe8OCJrHnvOEkaThCEO8ZFf8VIdKrTBwUJj/S3kYi0Y890grPW\n\t6Mq72e05xJYGDq/F1UL5TsuLWW4ik1pWcSqgUtp/cs5GLwY64ShHHSh50TMv7eHtO6\n\tO52RYXgroZSuPaYeYMWK4M6ZmECih+Gj9jDDKh4k=","From":"Laurent Pinchart ","To":"libcamera-devel@lists.libcamera.org","Date":"Wed, 02 Jan 2019 22:59:02 +0200","Message-ID":"<4352826.R2XpCxzena@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"<20181229033134.26927-1-niklas.soderlund@ragnatech.se>","References":"<20181229033134.26927-1-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","Content-Type":"text/plain; charset=\"iso-8859-1\"","Subject":"Re: [libcamera-devel] [PATCH] cam-ctl: add utility to control\n\tcameras","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 02 Jan 2019 20:58:01 -0000"}}]