[libgpiod] Bug in python binding when requesting output?

[libgpiod] Bug in python binding when requesting output?

[Pedro Botella]

Hi,

I'm experiencing what I think is a bug in the python bindings for libgpiod.
I believe a line.request with type gpiod.LINE_REQ_DIR_OUT always
results in that line being set to '0'.
To reproduce:
1. request a line with type gpiod.LINE_REQ_DIR_OUT
2. set the line to '1'
3. release the line
4. request the same line with type gpiod.LINE_REQ_DIR_OUT
5. get the value, it should now be '0'

I think the issue is in "gpiod_LineBulk_request" in
https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/tree/bindings/python/gpiodmodule.c
There a call to "gpiod_line_request_bulk" with default_vals being
passed as a pointer. Later on in the code, this parameter is checked
for NULL, if it is not NULL then the values in the array are used as
default_vals.
I believe that a NULL pointer should be passed instead if no
default_vals have been requested when doing a Line.request from
Python.

BR
Pedro

Re: [libgpiod] Bug in python binding when requesting output?

[Kent Gibson]

On Thu, Feb 11, 2021 at 09:54:22PM +0100, Pedro Botella wrote:
> Hi,
> 
> I'm experiencing what I think is a bug in the python bindings for libgpiod.
> I believe a line.request with type gpiod.LINE_REQ_DIR_OUT always
> results in that line being set to '0'.

That is correct - when requesting a line as output at the kernel uAPI
the initial value must always be provided.  If you do not provide
default_vals via the Python API then the output should be defaulted to
'0' by the Python binding.

> To reproduce:
> 1. request a line with type gpiod.LINE_REQ_DIR_OUT
> 2. set the line to '1'
> 3. release the line
> 4. request the same line with type gpiod.LINE_REQ_DIR_OUT
> 5. get the value, it should now be '0'
> 

To clarify, the expected behaviour is that the output is defaulted
to '0' if default values are not provided.
So the problem you are seeing is that the output is not consistently '0'?

If you are expecting to see a '1' then you are expecting the lack of
default_vals in the kwds to leave the output value as is, but that is
not the case - it should default to '0'.

> I think the issue is in "gpiod_LineBulk_request" in
> https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/tree/bindings/python/gpiodmodule.c
> There a call to "gpiod_line_request_bulk" with default_vals being
> passed as a pointer. Later on in the code, this parameter is checked
> for NULL, if it is not NULL then the values in the array are used as
> default_vals.
> I believe that a NULL pointer should be passed instead if no
> default_vals have been requested when doing a Line.request from
> Python.
> 

Agreed - passing default_vals uninitialized to gpiod_line_request_bulk()
is a bug.
It should be zeroed, or a NULL pointer should be passed if the
default_vals were not provided in the kwds. Otherwise the output
value will be set based on the uninitializezd contents of default_vals.

Would you like to provide a patch?

In the meantime the obvious workaround is to always provide default_vals
in the kwds.

Cheers,
Kent.

Re: [libgpiod] Bug in python binding when requesting output?

[Pedro Botella]

Okay, got it. I have understood the function incorrectly then.

What I wanted to achieve was for the output to keep its current state
if it was already configured as an output, which I thought would be a
reasonable behavior. So I will instead wrap my requests with this:

def gpiod_safe_request_out(line, consumer):
    if line.direction() == gpiod.Line.DIRECTION_OUTPUT:
        # already an output, request as is and the output value won't
be modified
        line.request(consumer=consumer, type=gpiod.LINE_REQ_DIR_AS_IS)
    else:
        # Read current value
        line.request(consumer=consumer, type=gpiod.LINE_REQ_DIR_IN)
        value = line.get_value()
        line.release()
        # Request as output current value as default value
        line.request(consumer=consumer, type=gpiod.LINE_REQ_DIR_OUT,
default_val=value)

Which won't modify outputs, and if it is currently an input, will keep
the value at the pin.

I don't think I'm the most suitable for providing a patch for the
uninitialized default_vals, I'm not very well versed in providing
patches to the linux source tree, but I can give it a try if you want
me to.

Thanks for your help Kent!

Pedro

On Sat, Feb 13, 2021 at 1:23 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Thu, Feb 11, 2021 at 09:54:22PM +0100, Pedro Botella wrote:
> > Hi,
> >
> > I'm experiencing what I think is a bug in the python bindings for libgpiod.
> > I believe a line.request with type gpiod.LINE_REQ_DIR_OUT always
> > results in that line being set to '0'.
>
> That is correct - when requesting a line as output at the kernel uAPI
> the initial value must always be provided.  If you do not provide
> default_vals via the Python API then the output should be defaulted to
> '0' by the Python binding.
>
> > To reproduce:
> > 1. request a line with type gpiod.LINE_REQ_DIR_OUT
> > 2. set the line to '1'
> > 3. release the line
> > 4. request the same line with type gpiod.LINE_REQ_DIR_OUT
> > 5. get the value, it should now be '0'
> >
>
> To clarify, the expected behaviour is that the output is defaulted
> to '0' if default values are not provided.
> So the problem you are seeing is that the output is not consistently '0'?
>
> If you are expecting to see a '1' then you are expecting the lack of
> default_vals in the kwds to leave the output value as is, but that is
> not the case - it should default to '0'.
>
> > I think the issue is in "gpiod_LineBulk_request" in
> > https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/tree/bindings/python/gpiodmodule.c
> > There a call to "gpiod_line_request_bulk" with default_vals being
> > passed as a pointer. Later on in the code, this parameter is checked
> > for NULL, if it is not NULL then the values in the array are used as
> > default_vals.
> > I believe that a NULL pointer should be passed instead if no
> > default_vals have been requested when doing a Line.request from
> > Python.
> >
>
> Agreed - passing default_vals uninitialized to gpiod_line_request_bulk()
> is a bug.
> It should be zeroed, or a NULL pointer should be passed if the
> default_vals were not provided in the kwds. Otherwise the output
> value will be set based on the uninitializezd contents of default_vals.
>
> Would you like to provide a patch?
>
> In the meantime the obvious workaround is to always provide default_vals
> in the kwds.
>
> Cheers,
> Kent.

Re: [libgpiod] Bug in python binding when requesting output?

[Kent Gibson]

On Mon, Feb 15, 2021 at 09:15:00AM +0100, Pedro Botella wrote:
> Okay, got it. I have understood the function incorrectly then.
> 
> What I wanted to achieve was for the output to keep its current state
> if it was already configured as an output, which I thought would be a
> reasonable behavior.

The general policy is that userspace takes responsibility for the state
of the GPIO lines it requests.
If selecting the line as an output then you should know the state you
want the line to be in - any residual state is generally irrelevant.
Having said that, the as-is option is there for any case where you
really need to know the existing state of the line before changing it,
but that should be very rare.

> So I will instead wrap my requests with this:
> 
> def gpiod_safe_request_out(line, consumer):
>     if line.direction() == gpiod.Line.DIRECTION_OUTPUT:
>         # already an output, request as is and the output value won't
> be modified
>         line.request(consumer=consumer, type=gpiod.LINE_REQ_DIR_AS_IS)
>     else:
>         # Read current value
>         line.request(consumer=consumer, type=gpiod.LINE_REQ_DIR_IN)
>         value = line.get_value()
>         line.release()
>         # Request as output current value as default value
>         line.request(consumer=consumer, type=gpiod.LINE_REQ_DIR_OUT,
> default_val=value)
> 
> Which won't modify outputs, and if it is currently an input, will keep
> the value at the pin.
> 

A line being an input is electrically very different from being an output.
If an output line is set to input then its value will depend on the
particular circuit - it may be pulled up or down or it may float.
Either way the existing input value doesn't generally mean much.

Again, if you know the line is suitable to use as an output then just set
the initial state to whichever level makes sense for your application.
And if you aren't sure the line is suitable to be an output then
definitely don't - that can make the smoke come out.

> I don't think I'm the most suitable for providing a patch for the
> uninitialized default_vals, I'm not very well versed in providing
> patches to the linux source tree, but I can give it a try if you want
> me to.
>

No problem - I can write a patch for it - just thought you might like to
take a swing at it since you found it.

Cheers,
Kent.

> Thanks for your help Kent!
> 
> Pedro
> 
> On Sat, Feb 13, 2021 at 1:23 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Thu, Feb 11, 2021 at 09:54:22PM +0100, Pedro Botella wrote:
> > > Hi,
> > >
> > > I'm experiencing what I think is a bug in the python bindings for libgpiod.
> > > I believe a line.request with type gpiod.LINE_REQ_DIR_OUT always
> > > results in that line being set to '0'.
> >
> > That is correct - when requesting a line as output at the kernel uAPI
> > the initial value must always be provided.  If you do not provide
> > default_vals via the Python API then the output should be defaulted to
> > '0' by the Python binding.
> >
> > > To reproduce:
> > > 1. request a line with type gpiod.LINE_REQ_DIR_OUT
> > > 2. set the line to '1'
> > > 3. release the line
> > > 4. request the same line with type gpiod.LINE_REQ_DIR_OUT
> > > 5. get the value, it should now be '0'
> > >
> >
> > To clarify, the expected behaviour is that the output is defaulted
> > to '0' if default values are not provided.
> > So the problem you are seeing is that the output is not consistently '0'?
> >
> > If you are expecting to see a '1' then you are expecting the lack of
> > default_vals in the kwds to leave the output value as is, but that is
> > not the case - it should default to '0'.
> >
> > > I think the issue is in "gpiod_LineBulk_request" in
> > > https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/tree/bindings/python/gpiodmodule.c
> > > There a call to "gpiod_line_request_bulk" with default_vals being
> > > passed as a pointer. Later on in the code, this parameter is checked
> > > for NULL, if it is not NULL then the values in the array are used as
> > > default_vals.
> > > I believe that a NULL pointer should be passed instead if no
> > > default_vals have been requested when doing a Line.request from
> > > Python.
> > >
> >
> > Agreed - passing default_vals uninitialized to gpiod_line_request_bulk()
> > is a bug.
> > It should be zeroed, or a NULL pointer should be passed if the
> > default_vals were not provided in the kwds. Otherwise the output
> > value will be set based on the uninitializezd contents of default_vals.
> >
> > Would you like to provide a patch?
> >
> > In the meantime the obvious workaround is to always provide default_vals
> > in the kwds.
> >
> > Cheers,
> > Kent.

Re: [libgpiod] Bug in python binding when requesting output?

[Pedro Botella]

Yep, the use case I'm developing for right now is probably not the
norm for GPIO, so I understand if I have to do some quirks to get it
to do what I want.

Thanks again for the help Kent, much appreciated.

Pedro

On Mon, Feb 15, 2021 at 10:11 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Mon, Feb 15, 2021 at 09:15:00AM +0100, Pedro Botella wrote:
> > Okay, got it. I have understood the function incorrectly then.
> >
> > What I wanted to achieve was for the output to keep its current state
> > if it was already configured as an output, which I thought would be a
> > reasonable behavior.
>
> The general policy is that userspace takes responsibility for the state
> of the GPIO lines it requests.
> If selecting the line as an output then you should know the state you
> want the line to be in - any residual state is generally irrelevant.
> Having said that, the as-is option is there for any case where you
> really need to know the existing state of the line before changing it,
> but that should be very rare.
>
> > So I will instead wrap my requests with this:
> >
> > def gpiod_safe_request_out(line, consumer):
> >     if line.direction() == gpiod.Line.DIRECTION_OUTPUT:
> >         # already an output, request as is and the output value won't
> > be modified
> >         line.request(consumer=consumer, type=gpiod.LINE_REQ_DIR_AS_IS)
> >     else:
> >         # Read current value
> >         line.request(consumer=consumer, type=gpiod.LINE_REQ_DIR_IN)
> >         value = line.get_value()
> >         line.release()
> >         # Request as output current value as default value
> >         line.request(consumer=consumer, type=gpiod.LINE_REQ_DIR_OUT,
> > default_val=value)
> >
> > Which won't modify outputs, and if it is currently an input, will keep
> > the value at the pin.
> >
>
> A line being an input is electrically very different from being an output.
> If an output line is set to input then its value will depend on the
> particular circuit - it may be pulled up or down or it may float.
> Either way the existing input value doesn't generally mean much.
>
> Again, if you know the line is suitable to use as an output then just set
> the initial state to whichever level makes sense for your application.
> And if you aren't sure the line is suitable to be an output then
> definitely don't - that can make the smoke come out.
>
> > I don't think I'm the most suitable for providing a patch for the
> > uninitialized default_vals, I'm not very well versed in providing
> > patches to the linux source tree, but I can give it a try if you want
> > me to.
> >
>
> No problem - I can write a patch for it - just thought you might like to
> take a swing at it since you found it.
>
> Cheers,
> Kent.
>
> > Thanks for your help Kent!
> >
> > Pedro
> >
> > On Sat, Feb 13, 2021 at 1:23 AM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > On Thu, Feb 11, 2021 at 09:54:22PM +0100, Pedro Botella wrote:
> > > > Hi,
> > > >
> > > > I'm experiencing what I think is a bug in the python bindings for libgpiod.
> > > > I believe a line.request with type gpiod.LINE_REQ_DIR_OUT always
> > > > results in that line being set to '0'.
> > >
> > > That is correct - when requesting a line as output at the kernel uAPI
> > > the initial value must always be provided.  If you do not provide
> > > default_vals via the Python API then the output should be defaulted to
> > > '0' by the Python binding.
> > >
> > > > To reproduce:
> > > > 1. request a line with type gpiod.LINE_REQ_DIR_OUT
> > > > 2. set the line to '1'
> > > > 3. release the line
> > > > 4. request the same line with type gpiod.LINE_REQ_DIR_OUT
> > > > 5. get the value, it should now be '0'
> > > >
> > >
> > > To clarify, the expected behaviour is that the output is defaulted
> > > to '0' if default values are not provided.
> > > So the problem you are seeing is that the output is not consistently '0'?
> > >
> > > If you are expecting to see a '1' then you are expecting the lack of
> > > default_vals in the kwds to leave the output value as is, but that is
> > > not the case - it should default to '0'.
> > >
> > > > I think the issue is in "gpiod_LineBulk_request" in
> > > > https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/tree/bindings/python/gpiodmodule.c
> > > > There a call to "gpiod_line_request_bulk" with default_vals being
> > > > passed as a pointer. Later on in the code, this parameter is checked
> > > > for NULL, if it is not NULL then the values in the array are used as
> > > > default_vals.
> > > > I believe that a NULL pointer should be passed instead if no
> > > > default_vals have been requested when doing a Line.request from
> > > > Python.
> > > >
> > >
> > > Agreed - passing default_vals uninitialized to gpiod_line_request_bulk()
> > > is a bug.
> > > It should be zeroed, or a NULL pointer should be passed if the
> > > default_vals were not provided in the kwds. Otherwise the output
> > > value will be set based on the uninitializezd contents of default_vals.
> > >
> > > Would you like to provide a patch?
> > >
> > > In the meantime the obvious workaround is to always provide default_vals
> > > in the kwds.
> > >
> > > Cheers,
> > > Kent.