[libcamera-devel] [RFC PATCH 6/8] libcamera: Add configuration interface
Jacopo Mondi
jacopo at jmondi.org
Wed Nov 25 11:18:26 CET 2020
On Wed, Nov 25, 2020 at 09:11:10AM +0000, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 25/11/2020 08:52, Jacopo Mondi wrote:
> > Hi Kieran,
> >
> > On Mon, Nov 23, 2020 at 04:43:17PM +0000, Kieran Bingham wrote:
> >> Provide an interface to support reading configuration files.
> >>
> >> Initial support is included for JSON formatted files, but extending this
> >> with other configuration file formats is not excluded.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> ---
> >> README.rst | 2 +-
> >> include/libcamera/internal/configuration.h | 37 +++++++++
> >> src/libcamera/configuration.cpp | 91 ++++++++++++++++++++++
> >> src/libcamera/meson.build | 1 +
> >> 4 files changed, 130 insertions(+), 1 deletion(-)
> >> create mode 100644 include/libcamera/internal/configuration.h
> >> create mode 100644 src/libcamera/configuration.cpp
> >>
> >> diff --git a/README.rst b/README.rst
> >> index 251291b77c62..c09e262fcc43 100644
> >> --- a/README.rst
> >> +++ b/README.rst
> >> @@ -58,7 +58,7 @@ Meson Build system: [required]
> >> pip3 install --user --upgrade meson
> >>
> >> for the libcamera core: [required]
> >> - python3-yaml python3-ply python3-jinja2
> >> + python3-yaml python3-ply python3-jinja2 nlohmann-json3-dev
> >
> > First question I have is about the dependency on this package.
> >
> > I have no real reasons to doubt about the availability of a packaged
> > version in most linux distro, nor about their stability and update
> > rate. It might be that projects named after a single person always
> > sound fragile to me, might be the rather low bus factor :)
>
>
> Yes, I find the 'named after me' packaging a little ... awkward. But
> there's really two choices for json libraries if we don't want boost.
> nlohmann-json and json-cpp.
>
> For bus factor, nlohmann-json has 178 contributors ... so I don't think
> it's just 'one guy'.
>
> I had planned to try both in earnest, and subclass Configuration so that
> they could be swapped at will, but the nlohmann was seemingly easier to
> get started with in the end, and subclassing to swap would mean wrapping
> the nlohmann generic container types in a generic container type and
> creating my own tree structures.
>
> We can do that of course, but that will take longer, and I honestly
> couldn't see the point of wrapping it for the internal usages (yet).
>
> If this was public API then it would be different.
>
> And maybe we'll update/change later - but I wanted to get things
> actually going so we can start figuring out what and where we store data
> in the first place.
>
> Who knows, maybe Configuration() will sometime need to parse yaml files
> so it will build a ConfigurationJSON or a ConfigurationYAML depending on
> the data - but I don't want to over engineer just to be able to read
> some key-values at this stage.
>
> > Joking aside this seems to be packaged in most distros
> > https://repology.org/project/nlohmann-json/versions
> > with rather different versions (3.9.1 on Arch, 2.1.1 on Ubuntu 18.04
> > which is an LTS if I'm not mistaken).
>
> It did seem to be quite well packaged, but I would probably lean towards
> bringing the headers in under third_party/ or such as it's header only
> to prevent any potential issues.
>
I would go in that direction too probably
>
> > There are notable exception though, in example Linux Mint.
> >
> > In ChromiumOS R87 I don't see it packaged, my equery skills are very
> > low so I might have missed it.
>
> Also, while available in the Raspbian OS distribution - it's an older
> version which didn't have a feature I was using "j.contains()".
>
> I thought that was actually a nicer way to implement the code rather
> than using iterators and j.find(), even if the .find() is perhaps more
> efficient as it will only do one look up - I think the code just doesn't
> flow quite as nicely. (Having to suddenly dereference iterators to a new
> type etc...)
>
In general, knowing we can rely on known version would bring quite
some peace of mind in regard to versioning and available features
>
> > The real question is:
> > - Are we happy to have this as externally packaged dependency ?
> >
> > If I look at the Arch package content:
> > https://www.archlinux.org/packages/community/any/nlohmann-json/files/
> >
> > This seems to be an header-only library. I wonder if we should not
> > rope it in during the build. There seems to be built-in support in
> > meson for that:
> > https://github.com/nlohmann/json#package-managers
> > https://wrapdb.mesonbuild.com/nlohmann_json
>
> It is in wrapdb - but it's out of date.
>
Right, 3.7.0 as I read it
> We can either include the headers in libcamera, or we can update the
> wrapdb ;-)
>
> I'd probably lean towards updating the wrapdb as that's more beneficial
> everywhere.
>
Indeed, I'm not aware of what the process is to do so though
> The meson wraps seem to work quite well - and will seemlessly download
> the package if it's missing and build/use that.
>
> Part of me is then weary of requiring an internet connection for that -
> but I really don't' think that's an issue anymore, as if you've cloned
> the repo - we can expect that there is a connection.
Probably so. I'm thinking of a use case where all the dependencies and
libcamera sources are packaged on a rootfs and -then- libcamera is
built on a live target, but I hardly find one.
>
>
>
> > Or include ourselves a version of the library in our source tree,
> > which opens a different question: how do we feel about distributing MIT
> > licensed code ? It should be no issue, but IANAL so it's better to
> > give this a thought.
>
> That's also what makes me lean towards updating and using the wrapdb ;-)
>
> Alternatively of course we can rewrite all this with json-cpp
> (https://github.com/open-source-parsers/jsoncpp) - but then we'll have
> exactly the same discussion on that library/package too ;-)
You've done the background investigations, so I assume you chose
nlohmann_json for valid reasons. Also, jsoncpp seems not to be an
header only library, so integration might become more problematic.
They're both MIT-licensed project, in one case we will need to only
include un-touched MIT-licensed code headers in LGPL code. If I'm not
mistaken with json-cpp we would need to link against it, so either
build it as a .so or rely on pre-packaged versions (unless we go the
'amalgamated' way:
https://github.com/open-source-parsers/jsoncpp/wiki/Amalgamated-(Possibly-outdated)
but we would have to build it ourselves to obtain the amalgamated
version)
Is this your understanding as well ?
Thanks
j
>
> Kieran
>
>
> > Thanks
> > j
> >
> >>
> >> for IPA module signing: [required]
> >> libgnutls28-dev openssl
> >> diff --git a/include/libcamera/internal/configuration.h b/include/libcamera/internal/configuration.h
> >> new file mode 100644
> >> index 000000000000..a89732f0210f
> >> --- /dev/null
> >> +++ b/include/libcamera/internal/configuration.h
> >> @@ -0,0 +1,37 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2020, Google Inc.
> >> + *
> >> + * configuration.h - Parsing configuration files
> >> + */
> >> +#ifndef __LIBCAMERA_INTERNAL_CONFIGURATION_H__
> >> +#define __LIBCAMERA_INTERNAL_CONFIGURATION_H__
> >> +
> >> +#include <fstream>
> >> +#include <string>
> >> +
> >> +/* https://nlohmann.github.io/json/home/exceptions/#switch-off-exceptions */
> >> +#define JSON_NOEXCEPTION 1
> >> +#include <nlohmann/json.hpp>
> >> +
> >> +using json = nlohmann::json;
> >> +
> >> +namespace libcamera {
> >> +
> >> +class Configuration
> >> +{
> >> +public:
> >> + int open(std::string filename);
> >> +
> >> + json &data() { return json_; }
> >> +
> >> +private:
> >> + std::string findFile(std::string filename);
> >> +
> >> + json json_;
> >> +};
> >> +
> >> +} /* namespace libcamera */
> >> +
> >> +#endif /* __LIBCAMERA_INTERNAL_CONFIGURATION_H__ */
> >> +
> >> diff --git a/src/libcamera/configuration.cpp b/src/libcamera/configuration.cpp
> >> new file mode 100644
> >> index 000000000000..f849088bbc45
> >> --- /dev/null
> >> +++ b/src/libcamera/configuration.cpp
> >> @@ -0,0 +1,91 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2020, Google Inc.
> >> + *
> >> + * configuration.cpp - Parsing configuration files
> >> + */
> >> +#include "libcamera/internal/configuration.h"
> >> +
> >> +#include "libcamera/internal/file.h"
> >> +#include "libcamera/internal/log.h"
> >> +#include "libcamera/internal/utils.h"
> >> +
> >> +#include <iostream>
> >> +#include <stdlib.h>
> >> +
> >> +/**
> >> + * \file configuration.h
> >> + * \brief Read interface for configuration files
> >> + */
> >> +
> >> +namespace libcamera {
> >> +
> >> +LOG_DEFINE_CATEGORY(Configuration)
> >> +
> >> +/*
> >> + * Configuration files can be stored in system paths, which are identified
> >> + * through the build configuration.
> >> + *
> >> + * However, when running uninstalled - the source location takes precedence.
> >> + */
> >> +std::string Configuration::findFile(std::string filename)
> >> +{
> >> + static std::array<std::string, 2> searchPaths = {
> >> + LIBCAMERA_SYSCONF_DIR,
> >> + LIBCAMERA_DATA_DIR,
> >> + };
> >> +
> >> + std::string root = utils::libcameraSourcePath();
> >> + if (!root.empty()) {
> >> + std::string configurationPath = root + "data/" + filename;
> >> +
> >> + if (File::exists(configurationPath))
> >> + return configurationPath;
> >> + }
> >> +
> >> + for (std::string &path : searchPaths) {
> >> + std::string configurationPath = path + "/" + filename;
> >> + if (File::exists(configurationPath))
> >> + return configurationPath;
> >> + }
> >> +
> >> + return "";
> >> +}
> >> +
> >> +/**
> >> + * \brief Open and parse a configuration file.
> >> + *
> >> + * The filename will be searched for on the libcamera configuration and paths,
> >> + * and then parsed.
> >> + *
> >> + * Successfully parsed files will present the data contained therein through the
> >> + * json object exposed from data();
> >> + */
> >> +int Configuration::open(std::string filename)
> >> +{
> >> + std::string data = findFile(filename);
> >> + if (data.empty()) {
> >> + LOG(Configuration, Warning)
> >> + << "file: \"" << filename
> >> + << "\" was not found.";
> >> + return -ENOENT;
> >> + }
> >> +
> >> + LOG(Configuration, Debug) << "Reading configuration from " << data;
> >> +
> >> + /* Parse with no error callbacks and exceptions disabled. */
> >> + std::ifstream input(data);
> >> + json j = json::parse(input, nullptr, false);
> >> + if (j.is_discarded()) {
> >> + LOG(Configuration, Error)
> >> + << "file: \"" << data
> >> + << "\" was not parsable.";
> >> + return -EINVAL;
> >> + }
> >> +
> >> + json_ = std::move(j);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +} /* namespace libcamera */
> >> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> >> index 387d5d88ecae..5d655c87a7a0 100644
> >> --- a/src/libcamera/meson.build
> >> +++ b/src/libcamera/meson.build
> >> @@ -9,6 +9,7 @@ libcamera_sources = files([
> >> 'camera_controls.cpp',
> >> 'camera_manager.cpp',
> >> 'camera_sensor.cpp',
> >> + 'configuration.cpp',
> >> 'controls.cpp',
> >> 'control_serializer.cpp',
> >> 'control_validator.cpp',
> >> --
> >> 2.25.1
> >>
> >> _______________________________________________
> >> libcamera-devel mailing list
> >> libcamera-devel at lists.libcamera.org
> >> https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards
> --
> Kieran
More information about the libcamera-devel
mailing list