[libcamera-devel] [PATCH] cam-ctl: add utility to control cameras

[Laurent Pinchart]

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