[libcamera-devel] [PATCH 04/12] libcamera: deviceenumerator: add DeviceMatch class

[Jacopo Mondi]

Hi Niklas,
  as I've commented on 00/12, I think the DeviceMatcher should be
replaced by a MediaDeviceDesc and have the MediaDevice do the matching
on it. Nonethless, I have some questions here, as they might apply
generally to all our code base.

On Sun, Dec 23, 2018 at 12:00:33AM +0100, Niklas S??derlund wrote:
> Provide a DeviceMatch class which represents all properties of a media
> device a pipeline hander can specify when searching for a device to use
> in its pipeline.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/libcamera/deviceenumerator.cpp       | 54 ++++++++++++++++++++++++
>  src/libcamera/include/deviceenumerator.h | 17 ++++++++
>  2 files changed, 71 insertions(+)
>
> diff --git a/src/libcamera/deviceenumerator.cpp b/src/libcamera/deviceenumerator.cpp
> index 7c44a65b45472ef3..17b6874d229c791c 100644
> --- a/src/libcamera/deviceenumerator.cpp
> +++ b/src/libcamera/deviceenumerator.cpp
> @@ -75,4 +75,58 @@ bool DeviceInfo::lookup(const std::string &name, std::string &devnode) const
>  	return true;
>  }
>
> +/* -----------------------------------------------------------------------------
> + * DeviceMatch
> + */
> +
> +DeviceMatch::DeviceMatch(const std::string &driver)
> +	: driver_(driver)
> +{
> +}

Is it worth placing constructors with list initializers only in the
.cpp file?

> +
> +void DeviceMatch::add(const std::string &entity)
> +{
> +	entities_.push_back(std::string(entity));
> +}
> +
> +bool DeviceMatch::match(const DeviceInfo *info) const
> +{
> +	std::vector<std::string> entities;

That's weird. This is clearly unused, but the compiler does not
complain. Actually I can keep it or remove it without any message from
g++. Was it here intentionally?

> +
> +	if (!matchInfo(info->info()))
> +		return false;
> +
> +	if (!matchEntities(info->entities()))
> +		return false;
> +
> +	return true;
> +}
> +
> +bool DeviceMatch::matchInfo(const struct media_device_info &info) const
> +{
> +	/* TODO: Add more optinal matching pairs from struct media_device_info */
optional
> +	/* TODO: Allow for empty driver in DeviceMatch */
> +	return driver_ == info.driver;
> +}
> +
> +bool DeviceMatch::matchEntities(const std::vector<std::string> &entities) const
> +{
> +	for (const std::string &name : entities_) {
> +		bool found = false;
> +
> +		for (const std::string &entity : entities) {
> +
nit: unnecessary empty line

> +			if (name == entity) {
> +				found = true;
> +				break;
> +			}
> +		}
> +
> +		if (!found)
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
>  } /* namespace libcamera */
> diff --git a/src/libcamera/include/deviceenumerator.h b/src/libcamera/include/deviceenumerator.h
> index 0136ed6ea23bf65e..fb412b8840cb2ffe 100644
> --- a/src/libcamera/include/deviceenumerator.h
> +++ b/src/libcamera/include/deviceenumerator.h
> @@ -39,6 +39,23 @@ private:
>  	std::map<std::string, std::string> entities_;
>  };
>
> +class DeviceMatch
> +{
> +public:
> +	DeviceMatch(const std::string &driver);
> +
> +	void add(const std::string &entity);
> +
> +	bool match(const DeviceInfo *info) const;
> +
> +private:
> +	std::string driver_;
> +	std::vector<std::string> entities_;
> +
> +	bool matchInfo(const struct media_device_info &info) const;
> +	bool matchEntities(const std::vector<std::string> &entities) const;
> +};
> +
>  } /* namespace libcamera */
>
>  #endif	/* __LIBCAMERA_DEVICEENUMERATE_H__ */

This is less than nit, but shouldn't this be DEVICEENUMERATOR ? :)
> --
> 2.20.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20181224/376ad828/attachment.sig>