[3/3] py: Add bindings for ControlId vendor information
diff mbox series

Message ID 20241010084719.712485-4-paul.elder@ideasonboard.com
State Accepted
Headers show
Series
  • libcamera: controls: Add namespace to ControlId
Related show

Commit Message

Paul Elder Oct. 10, 2024, 8:47 a.m. UTC 
Add python bindings for quering vendor information from a ControlId.

Example usage:
>>> cid
libcamera.ControlId(20, Saturation, ControlType.Float)
>>> cid.vendor
'libcamera'

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 src/py/libcamera/py_main.cpp | 1 +
 1 file changed, 1 insertion(+)

Comments

Kieran Bingham Oct. 10, 2024, 8:26 p.m. UTC | #1 
Quoting Paul Elder (2024-10-10 09:47:19)
> Add python bindings for quering vendor information from a ControlId.
> 
> Example usage:
> >>> cid
> libcamera.ControlId(20, Saturation, ControlType.Float)

Is it worth updating __repr__ to report the vendor as well ? - or even
update the print of the name parameter so it shows the same way it could
be used?

> >>> cid.vendor
> 'libcamera'
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  src/py/libcamera/py_main.cpp | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
> index 983b76f6e998..11e9dd6bf6eb 100644
> --- a/src/py/libcamera/py_main.cpp
> +++ b/src/py/libcamera/py_main.cpp
> @@ -400,6 +400,7 @@ PYBIND11_MODULE(_libcamera, m)
>                 .def_property_readonly("id", &ControlId::id)
>                 .def_property_readonly("name", &ControlId::name)
>                 .def_property_readonly("type", &ControlId::type)
> +               .def_property_readonly("vendor", &ControlId::vendor)
>                 .def("__str__", [](const ControlId &self) { return self.name(); })
>                 .def("__repr__", [](const ControlId &self) {
>                         return py::str("libcamera.ControlId({}, {}, {})")
> -- 
> 2.39.2
>
Laurent Pinchart Oct. 10, 2024, 8:48 p.m. UTC | #2 
On Thu, Oct 10, 2024 at 09:26:47PM +0100, Kieran Bingham wrote:
> Quoting Paul Elder (2024-10-10 09:47:19)
> > Add python bindings for quering vendor information from a ControlId.
> > 
> > Example usage:
> > >>> cid
> > libcamera.ControlId(20, Saturation, ControlType.Float)
> 
> Is it worth updating __repr__ to report the vendor as well ?

Sounds like a worthwhile improvement.

> - or even
> update the print of the name parameter so it shows the same way it could
> be used?

If we include the vendor name in the name property then we should do the
same in C++. I don't think that would be a good idea though, as there
are use cases for handling the vendor and name separately (Paul
mentioned in the cover letter grouping controls per vendor in a UI).

> > >>> cid.vendor
> > 'libcamera'
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > ---
> >  src/py/libcamera/py_main.cpp | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
> > index 983b76f6e998..11e9dd6bf6eb 100644
> > --- a/src/py/libcamera/py_main.cpp
> > +++ b/src/py/libcamera/py_main.cpp
> > @@ -400,6 +400,7 @@ PYBIND11_MODULE(_libcamera, m)
> >                 .def_property_readonly("id", &ControlId::id)
> >                 .def_property_readonly("name", &ControlId::name)
> >                 .def_property_readonly("type", &ControlId::type)
> > +               .def_property_readonly("vendor", &ControlId::vendor)
> >                 .def("__str__", [](const ControlId &self) { return self.name(); })
> >                 .def("__repr__", [](const ControlId &self) {
> >                         return py::str("libcamera.ControlId({}, {}, {})")
Kieran Bingham Oct. 10, 2024, 10:10 p.m. UTC | #3 
Quoting Laurent Pinchart (2024-10-10 21:48:23)
> On Thu, Oct 10, 2024 at 09:26:47PM +0100, Kieran Bingham wrote:
> > Quoting Paul Elder (2024-10-10 09:47:19)
> > > Add python bindings for quering vendor information from a ControlId.
> > > 
> > > Example usage:
> > > >>> cid
> > > libcamera.ControlId(20, Saturation, ControlType.Float)
> > 
> > Is it worth updating __repr__ to report the vendor as well ?
> 
> Sounds like a worthwhile improvement.

My first intention here was for the above to say

	libcamera.ControlId(20, Saturation, ControlType.Float, libcamera)

> > - or even
> > update the print of the name parameter so it shows the same way it could
> > be used?
> 
> If we include the vendor name in the name property then we should do the
> same in C++. I don't think that would be a good idea though, as there
> are use cases for handling the vendor and name separately (Paul
> mentioned in the cover letter grouping controls per vendor in a UI).

while here I mean how it can be used in python. I do'nt know what that
is right now but I assume we need to reference controls in some
namespaced fashion in python so I'd expect:

	libcamera.ControlId(20, libcamera.Saturation, ControlType.Float)

I don't think the vendor should be *in* the name.


--
Kieran

 
> > > >>> cid.vendor
> > > 'libcamera'
> > > 
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > ---
> > >  src/py/libcamera/py_main.cpp | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
> > > index 983b76f6e998..11e9dd6bf6eb 100644
> > > --- a/src/py/libcamera/py_main.cpp
> > > +++ b/src/py/libcamera/py_main.cpp
> > > @@ -400,6 +400,7 @@ PYBIND11_MODULE(_libcamera, m)
> > >                 .def_property_readonly("id", &ControlId::id)
> > >                 .def_property_readonly("name", &ControlId::name)
> > >                 .def_property_readonly("type", &ControlId::type)
> > > +               .def_property_readonly("vendor", &ControlId::vendor)
> > >                 .def("__str__", [](const ControlId &self) { return self.name(); })
> > >                 .def("__repr__", [](const ControlId &self) {
> > >                         return py::str("libcamera.ControlId({}, {}, {})")
> 
> -- 
> Regards,
> 
> Laurent Pinchart
Paul Elder Oct. 16, 2024, 11:22 a.m. UTC | #4 
On Thu, Oct 10, 2024 at 11:10:05PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2024-10-10 21:48:23)
> > On Thu, Oct 10, 2024 at 09:26:47PM +0100, Kieran Bingham wrote:
> > > Quoting Paul Elder (2024-10-10 09:47:19)
> > > > Add python bindings for quering vendor information from a ControlId.
> > > > 
> > > > Example usage:
> > > > >>> cid
> > > > libcamera.ControlId(20, Saturation, ControlType.Float)
> > > 
> > > Is it worth updating __repr__ to report the vendor as well ?
> > 
> > Sounds like a worthwhile improvement.
> 
> My first intention here was for the above to say
> 
> 	libcamera.ControlId(20, Saturation, ControlType.Float, libcamera)
> 
> > > - or even
> > > update the print of the name parameter so it shows the same way it could
> > > be used?
> > 
> > If we include the vendor name in the name property then we should do the
> > same in C++. I don't think that would be a good idea though, as there
> > are use cases for handling the vendor and name separately (Paul
> > mentioned in the cover letter grouping controls per vendor in a UI).
> 
> while here I mean how it can be used in python. I do'nt know what that
> is right now but I assume we need to reference controls in some
> namespaced fashion in python so I'd expect:
> 
> 	libcamera.ControlId(20, libcamera.Saturation, ControlType.Float)

Good idea.


Paul

> 
> I don't think the vendor should be *in* the name.
> 
> 
> --
> Kieran
> 
>  
> > > > >>> cid.vendor
> > > > 'libcamera'
> > > > 
> > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > ---
> > > >  src/py/libcamera/py_main.cpp | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
> > > > index 983b76f6e998..11e9dd6bf6eb 100644
> > > > --- a/src/py/libcamera/py_main.cpp
> > > > +++ b/src/py/libcamera/py_main.cpp
> > > > @@ -400,6 +400,7 @@ PYBIND11_MODULE(_libcamera, m)
> > > >                 .def_property_readonly("id", &ControlId::id)
> > > >                 .def_property_readonly("name", &ControlId::name)
> > > >                 .def_property_readonly("type", &ControlId::type)
> > > > +               .def_property_readonly("vendor", &ControlId::vendor)
> > > >                 .def("__str__", [](const ControlId &self) { return self.name(); })
> > > >                 .def("__repr__", [](const ControlId &self) {
> > > >                         return py::str("libcamera.ControlId({}, {}, {})")
> > 
> > -- 
> > Regards,
> > 
> > Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
index 983b76f6e998..11e9dd6bf6eb 100644
--- a/src/py/libcamera/py_main.cpp
+++ b/src/py/libcamera/py_main.cpp
@@ -400,6 +400,7 @@  PYBIND11_MODULE(_libcamera, m)
 		.def_property_readonly("id", &ControlId::id)
 		.def_property_readonly("name", &ControlId::name)
 		.def_property_readonly("type", &ControlId::type)
+		.def_property_readonly("vendor", &ControlId::vendor)
 		.def("__str__", [](const ControlId &self) { return self.name(); })
 		.def("__repr__", [](const ControlId &self) {
 			return py::str("libcamera.ControlId({}, {}, {})")