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

108 diff mbox series

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

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@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')