[libcamera-devel] [PATCH v2 3/7] libcamera: camera_sensor: Introduce CameraSensorFactory

[Jacopo Mondi]

Hi Laurent,

On Tue, Feb 18, 2020 at 11:35:11AM +0200, Laurent Pinchart wrote:

[snip]

> > > > +	 */
> > > > +	const char *entityName = entity->name().c_str();
> > > > +	const char *delim = strchrnul(entityName, ' ');
> > > > +	std::string sensorName(entityName, delim);
> > >
> > > Should you split after the last space, not the first space ? I think the
> > > following would avoid including string.h.
> > >
> > > 	std::string::size_type delim = entity->name().find_last_of(' ');
> > > 	std::string name = entity->name().substr(0, delim);
> >
> > Thanks, but I suspect this is not enough. It only happened with vimc,
> > which registers a "Sensor B" or "Sensor A", but I suspect other
> > drivers might as well register subdevices with spaces in their name.
> >
> > Instead of cutting to the first or last space, I suspect we might need
> > to look for the complete substring, even if it's a broader matching
> > criteria than what we have here.
> >
> > I'll give substring matching a try...
>
> Good point.
>
> https://en.cppreference.com/w/cpp/string/basic_string/starts_with would
> be useful, but is only available in C++20. Time for
> utils::string_starts_with ?
>

I've considered that, but at the same time it means we're now looking
for the 'key' (provided by the camera sensor at registration time) in
the 'name' (the name of the entity for which we're creating a camera
sensor).

So to me it seems enough to look for the substring with
std::string::find(). Also, and this should not happen, I know, the fact that
the entiy names are composed as "$sensorname $i2cbus::$i2caddr" is a
convention enforced by the driver usage of "v4l2_i2c_subdev_set_name()"
which is called by the canonical "v4l2_i2c_subdev_init()". I know upstream
and well behaved drivers should gurantee their names to be assembled with
those functions as

	snprintf(sd->name, sizeof(sd->name), "%s%s %d-%04x", devname, postfix,
		 i2c_adapter_id(client->adapter), client->addr);

but that's not guaranteed for downstream users. I know we can't
support any crazy BSP hack out there, and we don't want to, but in
this case enforcing that sensor name has that specific format in the
matching criteria, would tie hands to the ones who want to support
custom drivers using custom names with a custom camera sensor
subclass.

I would then just look for the key in the sensor name, whatever format
it has. It will work for us, and support sensor drivers with more
exotic naming schemes..


> > > > +
> > > > +	auto it = CameraSensorFactory::factories().find(sensorName);
> > > > +	if (it == CameraSensorFactory::factories().end()) {
> > > > +		LOG(CameraSensorFactory, Info)
> > > > +			<< "Unsupported sensor '" << entity->name()
> > > > +			<< "': using generic sensor handler";
> > > > +		return new CameraSensor(entity);
> > > > +	}
> > > > +
> > > > +	LOG(CameraSensorFactory, Info) << "Create handler for '"
> > > > +				       << entity->name() << "' sensor";
> > > > +
> > > > +	CameraSensorFactory *factory = it->second;
> > > > +	return factory->create(entity);
> > > > +}
> > > > +

[snip]

> > >
> > > Let's not add ##CameraSensor, it makes the code more confusing:
> > >
> > > class FooCameraSensor : CameraSensor
> > > {
> > > 	...
> > > };
> > >
> > > REGISTER_CAMERA_SENSOR(Foo);
> > >
> > > I'd rather write
> > >
> > > class FooCameraSensor : CameraSensor
> > > {
> > > 	...
> > > };
> > >
> > > REGISTER_CAMERA_SENSOR(FooCameraSensor);
> > >
> > > and make it possible for the author to name the class without a
> > > CameraSensor suffix.
> >
> > That requires implementations to be a bit more careful, as they should
> > come up with a name for the factory. To me, it seems more natural to
> > write
> >
> > class OV5670 : CameraSensor
> > {
> > }
> >
> > REGISTER_CAMERA_SENSOR(OV5670);
> >
> > Than
> >
> > class OV5670 : CameraSensor
> > {
> > }
> >
> > REGISTER_CAMERA_SENSOR(OV5670Factory);
>
> I haven't expressed myself very clearly, sorry about that. I didn't mean
> removing the ##Factory part, but the ##CameraSensor in
>
> 	: CameraSensorFactory(handler##CameraSensor::entityName()) {}
>
> We should end up writing
>
> class OV5670 : CameraSensor
> {
> 	...
> };
>
> REGISTER_CAMERA_SENSOR(OV5670);
>
> while I think your patch required
>
> class OV5670CameraSensor : CameraSensor
> {
> 	...
> };
>
> REGISTER_CAMERA_SENSOR(OV5670);
>

Correct. Sorry, I missed what you meant. I agree though, and now we
use OV5670 both as class name and as registration key.

Thanks
  j
-------------- 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/20200218/d4467232/attachment.sig>