[libcamera-devel] Expose the Request Sequence Number in Python Bindings

17957 diff mbox series

This is a one line change to expose the Request object's sequence number on
the pybind11 surfaces for use in upstream applications.

I'm new to this email based approach (let me know if I am doing this
right), but the associated PR is here:
https://github.com/kbingham/libcamera/pull/61

Signed-off-by: Matthew Goodman <matt@exclosure.io>
---
py::object value) {
                        self.controls().set(id.id(),
pyToControlValue(value, id.type()));

Hi Matthew,

Thank you for the patch.

On Tue, Dec 06, 2022 at 03:29:54PM -0800, Matthew Goodman via libcamera-devel wrote:
> This is a one line change to expose the Request object's sequence number on the
> pybind11 surfaces for use in upstream applications.
> 
> I'm new to this email based approach (let me know if I am doing this right),
> but the associated PR is here:
> https://github.com/kbingham/libcamera/pull/61

You're doing a fairly good job overall :-) A few comments:

- This last paragraph in the commit message shouldn't be part of the
  commit message itself (it shouldn't be recorded in the git history),
  so it should go after the --- line (see [*]).

- Please start the subject line with the correct prefix. You can use git
  log on the file or directory you're modifying to have an idea of what
  the right prefix is.

- Kieran likes to point to https://cbea.ms/git-commit/ as a good source
  of information regarding how to write commit messages (you can ignore
  the "Limit the subject line to 50 characters" rule, that's not
  achievable when adding prefixes). This should tell you that the commit
  message should explain *why* the change is useful, not just what it
  does.

- Please don't post HTML e-mails, especially for patches, that messes up
  the formatting and makes it impossible to apply them correctly. I
  recomment using git-send-email to send patches, it will avoid lots of
  issues with regular e-mail clients (web interfaces are particularly
  ill-suited, they are pretty much guaranteed to mess things up). We
  have a guide to assist configuration of git-send-email if you've never
  used it, you can find it at
  https://libcamera.org/contributing.html#submitting-patches.

The code change itself looks good :-)

> Signed-off-by: Matthew Goodman <matt@exclosure.io>
> ---

[*] here

> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
> index 75947889..d14e18e2 100644
> --- a/src/py/libcamera/py_main.cpp
> +++ b/src/py/libcamera/py_main.cpp
> @@ -335,6 +335,7 @@ PYBIND11_MODULE(_libcamera, m)
>                 .def_property_readonly("status", &Request::status)
>                 .def_property_readonly("buffers", &Request::buffers)
>                 .def_property_readonly("cookie", &Request::cookie)
> +               .def_property_readonly("sequence", &Request::sequence)
>                 .def_property_readonly("has_pending_buffers", &
> Request::hasPendingBuffers)
>                 .def("set_control", [](Request &self, const ControlId &id,
> py::object value) {
>                         self.controls().set(id.id(), pyToControlValue(value,
> id.type()));

Hi Matthew,

Quoting Laurent Pinchart via libcamera-devel (2022-12-07 00:54:05)
> Hi Matthew,
> 
> Thank you for the patch.
> 
> On Tue, Dec 06, 2022 at 03:29:54PM -0800, Matthew Goodman via libcamera-devel wrote:
> > This is a one line change to expose the Request object's sequence number on the
> > pybind11 surfaces for use in upstream applications.
> > 
> > I'm new to this email based approach (let me know if I am doing this right),
> > but the associated PR is here:
> > https://github.com/kbingham/libcamera/pull/61
> 
> You're doing a fairly good job overall :-) A few comments:
> 
> - This last paragraph in the commit message shouldn't be part of the
>   commit message itself (it shouldn't be recorded in the git history),
>   so it should go after the --- line (see [*]).
> 
> - Please start the subject line with the correct prefix. You can use git
>   log on the file or directory you're modifying to have an idea of what
>   the right prefix is.
> 
> - Kieran likes to point to https://cbea.ms/git-commit/ as a good source
>   of information regarding how to write commit messages (you can ignore
>   the "Limit the subject line to 50 characters" rule, that's not
>   achievable when adding prefixes). This should tell you that the commit
>   message should explain *why* the change is useful, not just what it
>   does.

Yeah ... ;-)

At least something like this:

"""
The python bindings are missing the ability to read the sequence number
of the Request object from the public API.

Expose the objects sequence number on the pybind11 surfaces to support
applications reading this value.
"""

And for the change from me,


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

You can add Review by tags that are given to you directly to any follow
up patches, so please add this text above your Signed-off: line.

> 
> - Please don't post HTML e-mails, especially for patches, that messes up
>   the formatting and makes it impossible to apply them correctly. I
>   recomment using git-send-email to send patches, it will avoid lots of

As this is python code, perhaps Tomi could give the second review (on
v2) - and then we can merge this.


>   issues with regular e-mail clients (web interfaces are particularly
>   ill-suited, they are pretty much guaranteed to mess things up). We
>   have a guide to assist configuration of git-send-email if you've never
>   used it, you can find it at
>   https://libcamera.org/contributing.html#submitting-patches.
> 
> The code change itself looks good :-)
> 

Agreed. I wish the sort order of these defs was the same as the public
interface though, but that's not something to fix in this patch.

--
Kieran


> > Signed-off-by: Matthew Goodman <matt@exclosure.io>
> > ---
> 
> [*] here
> 
> > diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
> > index 75947889..d14e18e2 100644
> > --- a/src/py/libcamera/py_main.cpp
> > +++ b/src/py/libcamera/py_main.cpp
> > @@ -335,6 +335,7 @@ PYBIND11_MODULE(_libcamera, m)
> >                 .def_property_readonly("status", &Request::status)
> >                 .def_property_readonly("buffers", &Request::buffers)
> >                 .def_property_readonly("cookie", &Request::cookie)
> > +               .def_property_readonly("sequence", &Request::sequence)
> >                 .def_property_readonly("has_pending_buffers", &
> > Request::hasPendingBuffers)
> >                 .def("set_control", [](Request &self, const ControlId &id,
> > py::object value) {
> >                         self.controls().set(id.id(), pyToControlValue(value,
> > id.type()));
> 
> -- 
> Regards,
> 
> Laurent Pinchart