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

[Niklas S??derlund]

Hi Jacopo,

Thanks for your feedback,

On 2018-12-24 11:55:30 +0100, Jacopo Mondi wrote:
> 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.

See comment in 00/12.

> 
> 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?

Why not?

> 
> > +
> > +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?

 My bad it was part of a previous version of the code and should be 
 removed. Great catch I wonder why g++ do not complain about it.

> 
> > +
> > +	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

Thanks, this is fixed in the documentation patch but no reason to not be 
correct in the first place.

> > +	/* 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

Thanks.


> 
> > +			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 ? :)

Yes, thanks for spotting this.


> > --
> > 2.20.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel



-- 
Regards,
Niklas Söderlund