[libcamera-devel] [PATCH] cam-ctl: add utility to control cameras
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jan 2 21:59:02 CET 2019
Hi Niklas,
Thank you for the patch.
On Saturday, 29 December 2018 05:31:34 EET Niklas Söderlund wrote:
> Provide a utility to interact with cameras. This initial state is
> limited and only supports listing cameras in the system and selecting a
> camera to interact with.
>
> There is not much a interacting possible yet with a camera so the tool
> simply exercise the API to get and put a camera.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> src/cam-ctl/main.cpp | 98 +++++++++++++++++++++++++++++++++++++++++
> src/cam-ctl/meson.build | 7 +++
> src/meson.build | 1 +
> 3 files changed, 106 insertions(+)
> create mode 100644 src/cam-ctl/main.cpp
> create mode 100644 src/cam-ctl/meson.build
>
> ---
>
> Hi,
>
> I'm not sure this is ready to be merged but I thought it good to share
> it on the ML to avoid redoing the work as more pieces come together.
>
> diff --git a/src/cam-ctl/main.cpp b/src/cam-ctl/main.cpp
> new file mode 100644
> index 0000000000000000..0bf76cef3efc8741
> --- /dev/null
> +++ b/src/cam-ctl/main.cpp
> @@ -0,0 +1,98 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2018, Google Inc.
> + *
> + * main.cpp - cam-ctl a tool to interact with the library
> + */
> +
> +#include <getopt.h>
> +#include <iostream>
> +#include <stdio.h>
> +#include <stdlib.h>
> +
> +#include <libcamera/libcamera.h>
> +
> +using namespace std;
> +using namespace libcamera;
> +
> +enum Option {
> + OptCamera = 1,
> + OptList,
> + OptLast,
> +};
> +
> +static struct option long_options[] = {
> + { "camera", required_argument, 0, 'c' },
> + { "list", no_argument, 0, 'l' },
Please add -h/--help.
> +};
> +
> +char options[OptLast];
> +
> +void usage()
> +{
> + cout << "Options:" << endl;
> + cout << " --camera <camera>" << endl;
> + cout << " --list" << endl;
It would be nice if the help text could be generated from the long_options
array. We could create a new option class that would serve for both getopt and
help text, but I fear we'd enter development of a new argument parsing library
:-) On the other hand, given that this tool will become the swiss army command
line knife of libcamera, it may be worth it as it will grow a plethora of
options.
In any case, you should document the short options too, and add a description
for each option.
> +}
> +
> +int main(int argc, char **argv)
> +{
> + CameraManager *cm;
You could create this on the stack instead of allocating it from the heap.
> + string camera;
> +
> + if (argc == 1) {
> + usage();
> + return 0;
> + }
> +
> + while (1) {
> + int c, option_index = 0;
Do you need to initialize option_index to 0 ? And as you don't use it, you
could just pass NULL as the last argument of getopt_long().
> + c = getopt_long (argc, argv, "c:l", long_options, &option_index);
> +
s/ (/(/
And no need for a blank line here.
> + if (c == -1)
> + break;
> +
> + switch (c) {
> + case 'c':
> + options[OptCamera] = 1;
> + camera = optarg;
> + break;
> + case 'l':
> + options[OptList] = 1;
> + break;
I would create an options structure that stores all options, and move option
parsing to a separate function. The rest of the code should then operate on
the options structure.
> + case '?':
> + break;
> + default:
> + abort();
Why abort() ?
> + }
> + }
> +
> + cm = new CameraManager();
> + if (!cm)
> + return -ENOMEM;
> +
> + cm->start();
No error handling ?
> + if (options[OptList]) {
Maybe an header stating "Available cameras:" ?
> + for (auto name : cm->list())
You can spell out the type instead of using auto.
> + cout << "- " << name << endl;
> + }
> +
> + if (options[OptCamera]) {
> + Camera *cam = cm->get(camera);
> +
> + if (cam) {
> + cout << "Using camera: " << cam->name() << endl;
s/://
> + cam->put();
> + } else {
> + cout << "Can't find camera '" << camera << "'" << endl;
cout << "Camera " << camera << " not found" << endl; ?
> + }
> + }
> +
> + cm->stop();
> +
> + delete cm;
> +
> + return 0;
> +}
> diff --git a/src/cam-ctl/meson.build b/src/cam-ctl/meson.build
> new file mode 100644
> index 0000000000000000..7c27a19dae98fe96
> --- /dev/null
> +++ b/src/cam-ctl/meson.build
> @@ -0,0 +1,7 @@
> +cam_ctl_sources = files([
> + 'main.cpp',
> +])
> +
> +cam_ctl = executable('cam-ctl', cam_ctl_sources,
> + link_with : libcamera,
> + include_directories : libcamera_includes)
Let's call this cam and claim the name :-)
> diff --git a/src/meson.build b/src/meson.build
> index 4ce9668caa7b8997..86e5290e5e66eb68 100644
> --- a/src/meson.build
> +++ b/src/meson.build
> @@ -1 +1,2 @@
> subdir('libcamera')
> +subdir('cam-ctl')
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list