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

[Pedro Botella <pbotella@xxxxxxxxx>]

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@xxxxxxxxx> 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@xxxxxxxxx> 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.