[libgpiod] gpiod_line_get_value_bulk may be broken?

[libgpiod] gpiod_line_get_value_bulk may be broken?

[andy pugh]

I am using v1.2 on a Raspberry Pi under Buster (as that is the
installed version)

However, the code appears to be the same in v1.6.

As far as I can see gpiod_line_get_value_bulk() always fails if the
bulk contains more than one line. The problem(I think)  is that
gpiod_line_same_chip() always returns -EINVAL

Test code output:

Chips line 0 0x3f1280 gpiochip0
Chips line 1 0x3f2af0 gpiochip0
Chips line 2 0x3f4350 gpiochip0

ie, all the same chip _name_ but the result of gpiod_line_get_chip (if
displayed as a pointer with %p) differs.

Test code

````
#include <gpiod.h>
#include <stdio.h>
#include <unistd.h>
#include <assert.h>
#include <errno.h>
#include <string.h>

int main(int argc, char **argv)
{
  struct gpiod_chip *chip;
  struct gpiod_line *line0, *line1, *line2;
  struct gpiod_line_bulk bulk;
  int retval;
  int val[4] = {0};

  // Open GPIO chip
  chip = gpiod_chip_open_by_name("gpiochip0");

  // Open GPIO lines
  line0 = gpiod_line_find("GPIO17");
  line1 = gpiod_line_find("GPIO18");
  line2 = gpiod_line_find("GPIO19");

  gpiod_line_request_input(line0, "test");
  gpiod_line_request_input(line1, "test");
  gpiod_line_request_input(line2, "test");

  gpiod_line_bulk_init(&bulk);
  gpiod_line_bulk_add(&bulk, line0);
  gpiod_line_bulk_add(&bulk, line1);
  gpiod_line_bulk_add(&bulk, line2);

  // Compare the return value of gpiod_line_get_chip() for each line
  printf("Chips line 0 %p %s\n", gpiod_line_get_chip(line0),
gpiod_chip_name(gpiod_line_get_chip(line0)));
  printf("Chips line 1 %p %s\n", gpiod_line_get_chip(line1),
gpiod_chip_name(gpiod_line_get_chip(line1)));
  printf("Chips line 2 %p %s\n", gpiod_line_get_chip(line2),
gpiod_chip_name(gpiod_line_get_chip(line2)));

  // try to read the IO lines
  retval = gpiod_line_get_value_bulk(&bulk, val);
  assert (retval == 0);

  // Release lines and chip
  gpiod_line_release_bulk(&bulk);
  gpiod_chip_close(chip);
  return 0;
}
````

--
atp
"A motorcycle is a bicycle with a pandemonium attachment and is
designed for the especial use of mechanical geniuses, daredevils and
lunatics."
— George Fitch, Atlanta Constitution Newspaper, 1912

Re: [libgpiod] gpiod_line_get_value_bulk may be broken?

[Kent Gibson]

On Thu, Jul 27, 2023 at 04:14:38PM +0100, andy pugh wrote:
> I am using v1.2 on a Raspberry Pi under Buster (as that is the
> installed version)
> 
> However, the code appears to be the same in v1.6.
> 
> As far as I can see gpiod_line_get_value_bulk() always fails if the
> bulk contains more than one line. The problem(I think)  is that
> gpiod_line_same_chip() always returns -EINVAL
> 
> Test code output:
> 
> Chips line 0 0x3f1280 gpiochip0
> Chips line 1 0x3f2af0 gpiochip0
> Chips line 2 0x3f4350 gpiochip0
> 
> ie, all the same chip _name_ but the result of gpiod_line_get_chip (if
> displayed as a pointer with %p) differs.
> 
> Test code
> 
> ````
> #include <gpiod.h>
> #include <stdio.h>
> #include <unistd.h>
> #include <assert.h>
> #include <errno.h>
> #include <string.h>
> 
> int main(int argc, char **argv)
> {
>   struct gpiod_chip *chip;
>   struct gpiod_line *line0, *line1, *line2;
>   struct gpiod_line_bulk bulk;
>   int retval;
>   int val[4] = {0};
> 
>   // Open GPIO chip
>   chip = gpiod_chip_open_by_name("gpiochip0");
> 
>   // Open GPIO lines
>   line0 = gpiod_line_find("GPIO17");
>   line1 = gpiod_line_find("GPIO18");
>   line2 = gpiod_line_find("GPIO19");
> 
>   gpiod_line_request_input(line0, "test");
>   gpiod_line_request_input(line1, "test");
>   gpiod_line_request_input(line2, "test");
> 
>   gpiod_line_bulk_init(&bulk);
>   gpiod_line_bulk_add(&bulk, line0);
>   gpiod_line_bulk_add(&bulk, line1);
>   gpiod_line_bulk_add(&bulk, line2);
> 
>   // Compare the return value of gpiod_line_get_chip() for each line
>   printf("Chips line 0 %p %s\n", gpiod_line_get_chip(line0),
> gpiod_chip_name(gpiod_line_get_chip(line0)));
>   printf("Chips line 1 %p %s\n", gpiod_line_get_chip(line1),
> gpiod_chip_name(gpiod_line_get_chip(line1)));
>   printf("Chips line 2 %p %s\n", gpiod_line_get_chip(line2),
> gpiod_chip_name(gpiod_line_get_chip(line2)));
> 
>   // try to read the IO lines
>   retval = gpiod_line_get_value_bulk(&bulk, val);
>   assert (retval == 0);
> 

That is not how the line_bulk API is used.
You don't request the lines separately and then add them to the bulk,
you add them to the bulk then request them with
gpiod_line_request_bulk_input(), or one of the other
gpiod_line_request_bulk_XXX() functions.

e.g.
 
    gpiod_line_bulk_init(&bulk);
    gpiod_line_bulk_add(&bulk, line0);
    gpiod_line_bulk_add(&bulk, line1);
    gpiod_line_bulk_add(&bulk, line2);
    gpiod_line_request_bulk_input(&bulk, "test");

    // try to read the IO lines
    retval = gpiod_line_get_value_bulk(&bulk, val);
    assert (retval == 0);


Refer to the test cases in libgpiod/tests/tests-line.c.
e.g. the request_bulk_output test case.

Btw, the primary use case for the bulk is for when you need to perform
operations on a set of lines as simultaneously as possible.  The
downside is that they have to be requested at the same time.
If you don't require the simultaneity, it may be simpler to request them
separately, and operate on them separately.

You might also want to consider looking at libgpiod v2, which is the
latest release and has a new API that is hopefully less confusing.
The get_multiple_line_values[1] example is similar to what you are doing
in your example.  You can use gpiod_chip_get_line_offset_from_name() to
perform the name to offset mapping, if necessary.

OTOH, libgpiod v2 hasn't made its way to many distros yet, and will never
get to Raspbian Buster, so you would need to download and build it
yourself, and your kernel may not even support it (libgpiod v2 requires
kernel 5.10 or later).

Cheers,
Kent.

[1] https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/tree/examples/get_multiple_line_values.c

Re: [libgpiod] gpiod_line_get_value_bulk may be broken?

[andy pugh]

On Thu, 27 Jul 2023 at 21:54, Kent Gibson <warthog618@gmail.com> wrote:

> That is not how the line_bulk API is used.
> You don't request the lines separately and then add them to the bulk,
> you add them to the bulk then request them with
> gpiod_line_request_bulk_input(), or one of the other
> gpiod_line_request_bulk_XXX() functions.

I did try that way first, but it didn't seem to be working for me.
I am currently upgrading the system to Bookworm (gpiod v1.6) to try again.

> Btw, the primary use case for the bulk is for when you need to perform
> operations on a set of lines as simultaneously as possible.

I am trying to do things as quickly as possible on a predetermined set
of lines.
I am experimenting with gpiod as a replacement for an existing (and
no-longer-working) driver that is part of LinuxCNC.

I suspect that gpiod won't be fast enough, ideally I would like to be
able to write to 15 IO lines in 15µs. (because the code will run in a
realtime thread which can't overrun)
(There are other reasons that it might not work too, you can probably
think of more than I can)

-- 
atp
"A motorcycle is a bicycle with a pandemonium attachment and is
designed for the especial use of mechanical geniuses, daredevils and
lunatics."
— George Fitch, Atlanta Constitution Newspaper, 1912

Re: [libgpiod] gpiod_line_get_value_bulk may be broken?

[Kent Gibson]

On Thu, Jul 27, 2023 at 10:17:05PM +0100, andy pugh wrote:
> On Thu, 27 Jul 2023 at 21:54, Kent Gibson <warthog618@gmail.com> wrote:
> 
> > That is not how the line_bulk API is used.
> > You don't request the lines separately and then add them to the bulk,
> > you add them to the bulk then request them with
> > gpiod_line_request_bulk_input(), or one of the other
> > gpiod_line_request_bulk_XXX() functions.
> 
> I did try that way first, but it didn't seem to be working for me.
> I am currently upgrading the system to Bookworm (gpiod v1.6) to try again.
> 

If you can repeat it, and ideally provide a failing test case, then we can
take a look at it.

> > Btw, the primary use case for the bulk is for when you need to perform
> > operations on a set of lines as simultaneously as possible.
> 
> I am trying to do things as quickly as possible on a predetermined set
> of lines.
> I am experimenting with gpiod as a replacement for an existing (and
> no-longer-working) driver that is part of LinuxCNC.
> 
> I suspect that gpiod won't be fast enough, ideally I would like to be
> able to write to 15 IO lines in 15µs. (because the code will run in a
> realtime thread which can't overrun)
> (There are other reasons that it might not work too, you can probably
> think of more than I can)
> 

Depends on what Pi you are on.  A Pi Zero would struggle, but on a Pi4
that is doable, of course depending on what else you are doing.
That is based on benchmarking libgpiod v2, but I would expect v1 to be
similar.

On a Pi is it significantly faster to go direct to hardware using
/dev/gpiomem, rather than going via the kernel as libgpiod does.
I do my best to avoid using gpiomem these days, but if you really need to
minimize CPU cycles or latency then that is another option.

Cheers,
Kent.

Re: [libgpiod] gpiod_line_get_value_bulk may be broken?

[andy pugh]

On Thu, 27 Jul 2023 at 22:55, Kent Gibson <warthog618@gmail.com> wrote:

> On a Pi is it significantly faster to go direct to hardware using
> /dev/gpiomem, rather than going via the kernel as libgpiod does.
> I do my best to avoid using gpiomem these days, but if you really need to
> minimize CPU cycles or latency then that is another option.

The existing driver uses gpiomem, but the interface used seems to not
be set up in Debian Bookworm.
It might be available in _Raspbian_ but that current Raspbian version
is based on Bullseye (or Buster)

https://github.com/LinuxCNC/linuxcnc/issues/2371

Is where the suggestion was made to try gpiod, initially because of a
problem with the way we were detecting Pi versions, but then because
gpiomem wasn't there.

(It can probably be configured, but we have a user base largely made
of machinists, not Linux enthusiasts)

-- 
atp
"A motorcycle is a bicycle with a pandemonium attachment and is
designed for the especial use of mechanical geniuses, daredevils and
lunatics."
— George Fitch, Atlanta Constitution Newspaper, 1912

Re: [libgpiod] gpiod_line_get_value_bulk may be broken?

[Kent Gibson]

On Thu, Jul 27, 2023 at 11:10:36PM +0100, andy pugh wrote:
> On Thu, 27 Jul 2023 at 22:55, Kent Gibson <warthog618@gmail.com> wrote:
> 
> > On a Pi is it significantly faster to go direct to hardware using
> > /dev/gpiomem, rather than going via the kernel as libgpiod does.
> > I do my best to avoid using gpiomem these days, but if you really need to
> > minimize CPU cycles or latency then that is another option.
> 
> The existing driver uses gpiomem, but the interface used seems to not
> be set up in Debian Bookworm.
> It might be available in _Raspbian_ but that current Raspbian version
> is based on Bullseye (or Buster)
> 

gpiomem is a Pi specific extension, and is not part of the mainline
kernel. Not sure what kernel your Debian Bookworm is providing, but if it
lacks the Pi extensions then you wont get gpiomem.

> https://github.com/LinuxCNC/linuxcnc/issues/2371
> 
> Is where the suggestion was made to try gpiod, initially because of a
> problem with the way we were detecting Pi versions, but then because
> gpiomem wasn't there.
> 

Yeah, the Pi4 requires a change to the driver as they changed the GPIO
register map, IIRC.

> (It can probably be configured, but we have a user base largely made
> of machinists, not Linux enthusiasts)
> 

The only way to disable gpiomem that I am aware of is to build the kernel
without it, so either a mainline kernel, or a Pi kernel without
BCM2835_DEVGPIOMEM set.
Even if there is some way to disable it in config, it seems unlikely
to me that they would compile it in, but then configure it out.

Cheers,
Kent.

Re: [libgpiod] gpiod_line_get_value_bulk may be broken?

[andy pugh]

On Thu, 27 Jul 2023 at 22:55, Kent Gibson <warthog618@gmail.com> wrote:

> > I did try that way first, but it didn't seem to be working for me.
> > I am currently upgrading the system to Bookworm (gpiod v1.6) to try again.
> >
>
> If you can repeat it, and ideally provide a failing test case, then we can
> take a look at it.

Now using gpiod v1.6 in Bookworm. gpiod_line_request_bulk() does not
seem to set the consumer.
Also, with the suggested use of bulk requests I still get an error
return from gpiod_line_get_value_bulk and an errno (22) that suggests
that it is line_bulk_same_chip() which has caused the problem.

test output:

line0 (null) line1 (null) line2 (null)
Error = Invalid argument (22)
a.out: test.c:47: main: Assertion `retval == 0' failed.
Aborted


test code:

````
#include <gpiod.h>
#include <stdio.h>
#include <unistd.h>
#include <assert.h>
#include <errno.h>
#include <string.h>
int main(int argc, char **argv)
{
  struct gpiod_chip *chip;
  struct gpiod_line *line0, *line1, *line2;
  struct gpiod_line_bulk bulk;
  int retval;
  int val[4] = {0};

  // Open GPIO chip
  chip = gpiod_chip_open_by_name("gpiochip0");

  // Open GPIO lines
  line0 = gpiod_line_find("GPIO17");
  line1 = gpiod_line_find("GPIO18");
  line2 = gpiod_line_find("GPIO19");

  gpiod_line_bulk_init(&bulk);
  gpiod_line_bulk_add(&bulk, line0);
  gpiod_line_bulk_add(&bulk, line1);
  gpiod_line_bulk_add(&bulk, line2);
  gpiod_line_request_bulk_input(&bulk, "test");

  printf("line0 %s line1 %s line2 %s\n", gpiod_line_consumer(line0),
                gpiod_line_consumer(line1), gpiod_line_consumer(line2));

 retval = gpiod_line_get_value_bulk(&bulk, val);
 printf("Error = %s (%i)\n", strerror(errno), errno);
 assert (retval == 0);
 usleep(1000000);

  // Release lines and chip
  gpiod_line_release_bulk(&bulk);
  gpiod_chip_close(chip);
  return 0;
}

````

If Instead I use

````
...
  gpiod_line_bulk_init(&bulk);
  gpiod_line_bulk_add(&bulk, line0);
  gpiod_line_bulk_add(&bulk, line1);
  gpiod_line_bulk_add(&bulk, line2);

  gpiod_line_request_input(line0, "test");
  gpiod_line_request_input(line1, "test");
  gpiod_line_request_input(line2, "test");
...
````

then the result is

line0 test line1 test line2 test

Which I think is why I was using separate requests. Initially I
thought that this might be the problem, as the
gpiod_line_get_value_bulk
function checks both all-the-same-chip and that all lines are requested the same


--
atp
"A motorcycle is a bicycle with a pandemonium attachment and is
designed for the especial use of mechanical geniuses, daredevils and
lunatics."
— George Fitch, Atlanta Constitution Newspaper, 1912

Re: [libgpiod] gpiod_line_get_value_bulk may be broken?

[andy pugh]

> line0 (null) line1 (null) line2 (null)
> Error = Invalid argument (22)
> a.out: test.c:47: main: Assertion `retval == 0' failed.
> Aborted

In case it matters. Raspberry Pi400
Debian 12 for Pi4 downloaded from: https://raspi.debian.net/daily-images/ today.

files compiled with

gcc test.c -lgpiod


-- 
atp
"A motorcycle is a bicycle with a pandemonium attachment and is
designed for the especial use of mechanical geniuses, daredevils and
lunatics."
— George Fitch, Atlanta Constitution Newspaper, 1912

Re: [libgpiod] gpiod_line_get_value_bulk may be broken?

[Kent Gibson]

On Fri, Jul 28, 2023 at 01:39:37AM +0100, andy pugh wrote:
> On Thu, 27 Jul 2023 at 22:55, Kent Gibson <warthog618@gmail.com> wrote:
> 
> > > I did try that way first, but it didn't seem to be working for me.
> > > I am currently upgrading the system to Bookworm (gpiod v1.6) to try again.
> > >
> >
> > If you can repeat it, and ideally provide a failing test case, then we can
> > take a look at it.
> 
> Now using gpiod v1.6 in Bookworm. gpiod_line_request_bulk() does not
> seem to set the consumer.
> Also, with the suggested use of bulk requests I still get an error
> return from gpiod_line_get_value_bulk and an errno (22) that suggests
> that it is line_bulk_same_chip() which has caused the problem.
> 
> test output:
> 
> line0 (null) line1 (null) line2 (null)
> Error = Invalid argument (22)
> a.out: test.c:47: main: Assertion `retval == 0' failed.
> Aborted
> 
> 
> test code:
> 
> ````
> #include <gpiod.h>
> #include <stdio.h>
> #include <unistd.h>
> #include <assert.h>
> #include <errno.h>
> #include <string.h>
> int main(int argc, char **argv)
> {
>   struct gpiod_chip *chip;
>   struct gpiod_line *line0, *line1, *line2;
>   struct gpiod_line_bulk bulk;
>   int retval;
>   int val[4] = {0};
> 
>   // Open GPIO chip
>   chip = gpiod_chip_open_by_name("gpiochip0");
> 
>   // Open GPIO lines
>   line0 = gpiod_line_find("GPIO17");
>   line1 = gpiod_line_find("GPIO18");
>   line2 = gpiod_line_find("GPIO19");
> 

Your problem is that finding lines this way produces gpiod_lines with
different chip pointers, and gpiod_line_request_bulk_input() is taking
that to mean different chips, so the request itself is failing - but you
didn't check.

>   gpiod_line_bulk_init(&bulk);
>   gpiod_line_bulk_add(&bulk, line0);
>   gpiod_line_bulk_add(&bulk, line1);
>   gpiod_line_bulk_add(&bulk, line2);
>   gpiod_line_request_bulk_input(&bulk, "test");
> 

If you change that to:
  retval = gpiod_line_request_bulk_input(&bulk, "test");
  printf("Error = %s (%i)\n", strerror(errno), errno);
  assert (retval == 0);

it will die on the assert.

Try this to find the lines instead:

  // Open GPIO lines
  // (actually this is just a find - the request performs the open)
  line0 = gpiod_chip_find_line(chip, "GPIO17");
  line1 = gpiod_chip_find_line(chip, "GPIO18");
  line2 = gpiod_chip_find_line(chip, "GPIO19");

That then works for me (including the extra Error print above):

$ ./a.out
Error = Success (0)
line0 test line1 test line2 test
Error = Success (0)


Not saying the gpiod_line_request_bulk_input() behaviour is correct, but
given v1 is obsoleted by v2, and there is a reasonable workaround for
v1 (assuming you know the chip the line is on), I'm not sure Bart will
want to fix that quirk.

For the same reason, I would suggest that you try libgpiod v2 and use
that instead if you possibly can - assuming libgpiod is fast enough for
your application.  

Cheers,
Kent.

Re: [libgpiod] gpiod_line_get_value_bulk may be broken?

[andy pugh]

On Fri, 28 Jul 2023 at 06:57, Kent Gibson <warthog618@gmail.com> wrote:

> Your problem is that finding lines this way produces gpiod_lines with
> different chip pointers, and gpiod_line_request_bulk_input() is taking
> that to mean different chips, so the request itself is failing - but you
> didn't check.

That was on my list of things to check next, but it was getting late
and I wanted to send the failing case as requested.
I had guessed that the same line_bulk_same_chip() routine would be the
cause of the bulk request failing too.

I think that test might be flawed....

>   line0 = gpiod_chip_find_line(chip, "GPIO17");
>   line1 = gpiod_chip_find_line(chip, "GPIO18");
>   line2 = gpiod_chip_find_line(chip, "GPIO19");

This is working for me now, thanks.

> Not saying the gpiod_line_request_bulk_input() behaviour is correct, but
> given v1 is obsoleted by v2, and there is a reasonable workaround for
> v1 (assuming you know the chip the line is on), I'm not sure Bart will
> want to fix that quirk.

Unfortunately I won't know what chip the line is on, but I have
already considered that and plan to have an array of structs
containing the "bulk" and the "chip" for each chip that is needed
according to the IO line list submitted by the (pesky!) users.

> For the same reason, I would suggest that you try libgpiod v2 and use
> that instead if you possibly can - assuming libgpiod is fast enough for
> your application.

libgpiod2 (and any further bugfix to 1.6 for that matter) have the
problem of not being currently available as packages in the
distributions we use.

It is possible that we could serve the package from our _own_ package
server which we have been running for a couple of decades, but as we
have _just_ been accepted as a mainline Debian package that would seem
a little peculiar (and involves trying to explain to machinists how to
add extra repositories and set up the associated keys)

I think I see the way forwards now. Thanks for your help.

-- 
atp
"A motorcycle is a bicycle with a pandemonium attachment and is
designed for the especial use of mechanical geniuses, daredevils and
lunatics."
— George Fitch, Atlanta Constitution Newspaper, 1912

Re: [libgpiod] gpiod_line_get_value_bulk may be broken?

[Kent Gibson]

On Fri, Jul 28, 2023 at 08:01:13PM +0100, andy pugh wrote:
> On Fri, 28 Jul 2023 at 06:57, Kent Gibson <warthog618@gmail.com> wrote:
> 
> > Your problem is that finding lines this way produces gpiod_lines with
> > different chip pointers, and gpiod_line_request_bulk_input() is taking
> > that to mean different chips, so the request itself is failing - but you
> > didn't check.
> 
> That was on my list of things to check next, but it was getting late
> and I wanted to send the failing case as requested.
> I had guessed that the same line_bulk_same_chip() routine would be the
> cause of the bulk request failing too.
> 
> I think that test might be flawed....
> 

Fixing the same chip check might be possible, but I would have to look
into all the other cases where is it used and for any other side effects.
I'm much more familiar with v2, so I'm not sure how deep that rabbit
hole goes.

> >   line0 = gpiod_chip_find_line(chip, "GPIO17");
> >   line1 = gpiod_chip_find_line(chip, "GPIO18");
> >   line2 = gpiod_chip_find_line(chip, "GPIO19");
> 
> This is working for me now, thanks.
> 
> > Not saying the gpiod_line_request_bulk_input() behaviour is correct, but
> > given v1 is obsoleted by v2, and there is a reasonable workaround for
> > v1 (assuming you know the chip the line is on), I'm not sure Bart will
> > want to fix that quirk.
> 
> Unfortunately I won't know what chip the line is on, but I have
> already considered that and plan to have an array of structs
> containing the "bulk" and the "chip" for each chip that is needed
> according to the IO line list submitted by the (pesky!) users.
> 

So the IO lines are specified by name?

The GPIO uAPI is chip based, so if you have lines on multiple chips you
will need a line_bulk for each chip.  So you are going to have to
partition the lines before adding them to a bulk either way.
The v1 API is a bit unfortunate as you have to add gpiod_lines to
the bulk.
The libgpiod v2 API makes that a bit easier by separating the chip and
offsets when creating line_requests (which replaced the line_bulk).

For v1 you would currently need to get a gpiod_line from that specific
chip object, as I did above with gpiod_chip_find_line().
To make that more general you would use gpiod_find_line() to determine
which chip the line is on, then get a new gpiod_line for each line on
that chip using gpiod_chip_get_line() or gpiod_chip_get_lines(), rather
than repeating the find on the chip - you know the offset now.

> > For the same reason, I would suggest that you try libgpiod v2 and use
> > that instead if you possibly can - assuming libgpiod is fast enough for
> > your application.
> 
> libgpiod2 (and any further bugfix to 1.6 for that matter) have the
> problem of not being currently available as packages in the
> distributions we use.
> 

Be aware that the libgpiod2 library package in Debian is actually libgpiod v1
- currently 1.6.3 in stable[1].
Not sure about the story behind that - before my time.
And no idea what they will package libgpiod v2 as, libgpiod-2??, but
there will be confusion whatever they do.
There is no libgpiod v2 package in Debian yet, AFAIAA, but they are
at least aware.

As libgpiod v1 and libgpiod v2 target different versions of the GPIO
uAPI, there is a case for having both installed at the same time - older
apps require v1 and newer ones v2.  So that is going to be fun.

> It is possible that we could serve the package from our _own_ package
> server which we have been running for a couple of decades, but as we
> have _just_ been accepted as a mainline Debian package that would seem
> a little peculiar (and involves trying to explain to machinists how to
> add extra repositories and set up the associated keys)
> 

Could you statically link until libgpiod v2 becomes more widely available?

I would strongly lean towards using v2 over v1 if that is at all
possible, to avoid having to port from v1 to v2 at a later date.

> I think I see the way forwards now. Thanks for your help.
> 

Sorry that your path ahead isn't as smooth as it could be.

Cheers,
Kent.

[1] https://tracker.debian.org/pkg/libgpiod

Re: [libgpiod] gpiod_line_get_value_bulk may be broken?

[andy pugh]

On Sat, 29 Jul 2023 at 03:03, Kent Gibson <warthog618@gmail.com> wrote:

> > Unfortunately I won't know what chip the line is on, but I have
> > already considered that and plan to have an array of structs
> > containing the "bulk" and the "chip" for each chip that is needed
> > according to the IO line list submitted by the (pesky!) users.
> >
>
> So the IO lines are specified by name?

Yes, they are.

Before I go too far down the wrong path, it has occurred to me that I
probably can't allow the lines to go out of scope?

ie, this (pseudocode) won't work

for each lineName in lineList
    temp_line = gpiod_chip_find_line(chip, lineName)
    gpiod_line_bulk_add(&bulk, temp_line);
next

As each line in the bulk will actually point to the same (last found) line?
Or am I missing a subtlety?

-- 
atp
"A motorcycle is a bicycle with a pandemonium attachment and is
designed for the especial use of mechanical geniuses, daredevils and
lunatics."
— George Fitch, Atlanta Constitution Newspaper, 1912

Re: [libgpiod] gpiod_line_get_value_bulk may be broken?

[Kent Gibson]

On Sat, Aug 05, 2023 at 11:55:44PM +0100, andy pugh wrote:
> On Sat, 29 Jul 2023 at 03:03, Kent Gibson <warthog618@gmail.com> wrote:
> 
> > > Unfortunately I won't know what chip the line is on, but I have
> > > already considered that and plan to have an array of structs
> > > containing the "bulk" and the "chip" for each chip that is needed
> > > according to the IO line list submitted by the (pesky!) users.
> > >
> >
> > So the IO lines are specified by name?
> 
> Yes, they are.
> 
> Before I go too far down the wrong path, it has occurred to me that I
> probably can't allow the lines to go out of scope?
> 
> ie, this (pseudocode) won't work
> 
> for each lineName in lineList
>     temp_line = gpiod_chip_find_line(chip, lineName)
>     gpiod_line_bulk_add(&bulk, temp_line);
> next
> 
> As each line in the bulk will actually point to the same (last found) line?
> Or am I missing a subtlety?
> 

This pseudocode is fine, as the gpiod_chip_find_line() returns a new
gpiod_line object for each line, and that is passed to the bulk.
So they are definitely not using the same gpiod_line object.

That pseudocode is essentially what gpiod_chip_find_lines() does too,
though that requires all the lines being on the same chip, and I assume
you have a separate bulk for each chip, so there is more going on in the
loop than you show.

And I maintain that the wrong path here is to use v1, rather than v2.
So v2 is absolutely not an option?

Cheers,
Kent.

Re: [libgpiod] gpiod_line_get_value_bulk may be broken?

[andy pugh]

On Sun, 6 Aug 2023 at 02:02, Kent Gibson <warthog618@gmail.com> wrote:

> And I maintain that the wrong path here is to use v1, rather than v2.
> So v2 is absolutely not an option?

Not unless it is available as an automatically installable dependency
of our package, no.
Which effectively means that it needs to be a package included in
current Debian.

-- 
atp
"A motorcycle is a bicycle with a pandemonium attachment and is
designed for the especial use of mechanical geniuses, daredevils and
lunatics."
— George Fitch, Atlanta Constitution Newspaper, 1912

Re: [libgpiod] gpiod_line_get_value_bulk may be broken?

[Kent Gibson]

On Sun, Aug 06, 2023 at 10:13:38AM +0100, andy pugh wrote:
> On Sun, 6 Aug 2023 at 02:02, Kent Gibson <warthog618@gmail.com> wrote:
> 
> > And I maintain that the wrong path here is to use v1, rather than v2.
> > So v2 is absolutely not an option?
> 
> Not unless it is available as an automatically installable dependency
> of our package, no.
> Which effectively means that it needs to be a package included in
> current Debian.
> 

You can't statically link libgpiod until it is available as a package?

Cheers,
Kent.

Re: [libgpiod] gpiod_line_get_value_bulk may be broken?

[andy pugh]

On Sun, 6 Aug 2023 at 10:29, Kent Gibson <warthog618@gmail.com> wrote:

> You can't statically link libgpiod until it is available as a package?

Would that mean merging the gpiod code into our codebase? Or can this
be done some other way?


Anyway, I now have it working, to an extent, but feel that there might
be a problem with gpiod_line_request_bulk_input() and
gpiod_line_request_bulk_output() if the bulk contains more than 5
lines.

My current code is here (Work in progress!)

https://github.com/LinuxCNC/linuxcnc/blob/andypugh/hal_gpio/src/hal/drivers/hal_gpio.c

The test input file (
https://github.com/LinuxCNC/linuxcnc/blob/andypugh/hal_gpio/src/test.hal
) attempts to configure 8 input and 8 output lines.
However, if I change the 5 to 6 in either of lines 157 or 180 then the
output looks something like:

HAL: initializing hal_lib
/home/andypugh/linuxcnc-dev/bin/rtapi_app load hal_gpio
inputs=GPIO5,GPIO6,GPIO12,GPIO13,GPIO16,GPIO17,GPIO18,GPIO19
outputs=GPIO20,GPIO21,GPIO22,GPIO23,GPIO24,GPIO25,GPIO26,GPIO27
HAL: initializing component 'halcmd9608'
smalloc_dn: shmem available 1048284
HAL: component 'halcmd9608' initialized, ID = 02
Note: Using POSIX realtime
before request
rtapi_app: caught signal 11 - dumping core

The "before request" line was added for debugging, the "after request"
line is never printed, nor is the error message if the request fails,
so my belief is that lines 163 and/or 185 are segfaulting in the
bulk_request functions.

The driver appears to work fine if I limit the number of parsed input
strings to 5.

-- 
atp
"A motorcycle is a bicycle with a pandemonium attachment and is
designed for the especial use of mechanical geniuses, daredevils and
lunatics."
— George Fitch, Atlanta Constitution Newspaper, 1912

Re: [libgpiod] gpiod_line_get_value_bulk may be broken?

[Kent Gibson]

On Thu, Aug 10, 2023 at 01:17:13AM +0100, andy pugh wrote:
> On Sun, 6 Aug 2023 at 10:29, Kent Gibson <warthog618@gmail.com> wrote:
> 
> > You can't statically link libgpiod until it is available as a package?
> 
> Would that mean merging the gpiod code into our codebase? Or can this
> be done some other way?
> 

There is no code movement at all, this is just telling the linker to
link libgpiod.a directly into your executable, rather than dynamically
linking against libgpiod.so.

The specifics of how to do that depend on your build.

Later, when libgpiod v2 becomes more widely available, you can switch
the build back to dynamic linkage.

> 
> Anyway, I now have it working, to an extent, but feel that there might
> be a problem with gpiod_line_request_bulk_input() and
> gpiod_line_request_bulk_output() if the bulk contains more than 5
> lines.
> 
> My current code is here (Work in progress!)
> 
> https://github.com/LinuxCNC/linuxcnc/blob/andypugh/hal_gpio/src/hal/drivers/hal_gpio.c
> 
> The test input file (
> https://github.com/LinuxCNC/linuxcnc/blob/andypugh/hal_gpio/src/test.hal
> ) attempts to configure 8 input and 8 output lines.
> However, if I change the 5 to 6 in either of lines 157 or 180 then the
> output looks something like:
> 
> HAL: initializing hal_lib
> /home/andypugh/linuxcnc-dev/bin/rtapi_app load hal_gpio
> inputs=GPIO5,GPIO6,GPIO12,GPIO13,GPIO16,GPIO17,GPIO18,GPIO19
> outputs=GPIO20,GPIO21,GPIO22,GPIO23,GPIO24,GPIO25,GPIO26,GPIO27
> HAL: initializing component 'halcmd9608'
> smalloc_dn: shmem available 1048284
> HAL: component 'halcmd9608' initialized, ID = 02
> Note: Using POSIX realtime
> before request
> rtapi_app: caught signal 11 - dumping core
> 
> The "before request" line was added for debugging, the "after request"
> line is never printed, nor is the error message if the request fails,
> so my belief is that lines 163 and/or 185 are segfaulting in the
> bulk_request functions.
> 
> The driver appears to work fine if I limit the number of parsed input
> strings to 5.

Check the sizes of the memory you are allocating.
e.g. this:

	    arr[c].chip = (struct gpiod_chip *)rtapi_kmalloc(sizeof(arr[c].chip), RTAPI_GFP_KERNEL);

allocates the space for a pointer to a struct, not the struct itself.
So you are corrupting your heap.

Try
	    arr[c].chip = (struct gpiod_chip *)rtapi_kmalloc(sizeof(*arr[c].chip), RTAPI_GFP_KERNEL);

Similarly your other mallocs.

Btw, v2 uses opaque structs so this wouldn't be an issue - it would do
the allocating for you, e.g. gpiod_chip_open() allocates and returns the
pointer to the struct gpiod_chip.

Cheers,
Kent.

Re: [libgpiod] gpiod_line_get_value_bulk may be broken?

[andy pugh]

On Thu, 10 Aug 2023 at 01:46, Kent Gibson <warthog618@gmail.com> wrote:

> Check the sizes of the memory you are allocating.
> e.g. this:
>
>             arr[c].chip = (struct gpiod_chip *)rtapi_kmalloc(sizeof(arr[c].chip), RTAPI_GFP_KERNEL);
>
> allocates the space for a pointer to a struct, not the struct itself.

Oh, that's embarrassing.

>             arr[c].chip = (struct gpiod_chip *)rtapi_kmalloc(sizeof(*arr[c].chip), RTAPI_GFP_KERNEL);

But that doesn't actually work:

"hal/drivers/hal_gpio.c: In function ‘build_chips_collection’:
hal/drivers/hal_gpio.c:119:41: error: dereferencing pointer to
incomplete type ‘struct gpiod_chip’
      arr[c].chip = rtapi_kmalloc(sizeof(*arr[c].chip), RTAPI_GFP_KERNEL);
"

The exact same structure compiles without error for the
gpiod_line_bulk a few lines lower. I think that the difference is that
gpiod.h includes the definition of struct gpiod_line_bulk, but only
the prototype of struct gpiod_chip.
(This is mainly based on googling Stack Overflow, so might be wrong)

> There is no code movement at all, this is just telling the linker to
> link libgpiod.a directly into your executable, rather than dynamically
> linking against libgpiod.so.
>
> The specifics of how to do that depend on your build.

But surely the compiler and linker need either the V2 code, or the
library object? How does it access these if we don't have them in our
repository?
Even if we add it as a build-time dependency, it isn't currently
available in the usual places?
I must be missing something? Or are you assuming that I am the only
one compiling LinuxCNC?

-- 
atp
"A motorcycle is a bicycle with a pandemonium attachment and is
designed for the especial use of mechanical geniuses, daredevils and
lunatics."
— George Fitch, Atlanta Constitution Newspaper, 1912

Re: [libgpiod] gpiod_line_get_value_bulk may be broken?

[Kent Gibson]

On Thu, Aug 10, 2023 at 11:07:44PM +0100, andy pugh wrote:
> On Thu, 10 Aug 2023 at 01:46, Kent Gibson <warthog618@gmail.com> wrote:
> 
> > Check the sizes of the memory you are allocating.
> > e.g. this:
> >
> >             arr[c].chip = (struct gpiod_chip *)rtapi_kmalloc(sizeof(arr[c].chip), RTAPI_GFP_KERNEL);
> >
> > allocates the space for a pointer to a struct, not the struct itself.
> 
> Oh, that's embarrassing.
> 

Been there, done that.

> >             arr[c].chip = (struct gpiod_chip *)rtapi_kmalloc(sizeof(*arr[c].chip), RTAPI_GFP_KERNEL);
> 
> But that doesn't actually work:
> 
> "hal/drivers/hal_gpio.c: In function ‘build_chips_collection’:
> hal/drivers/hal_gpio.c:119:41: error: dereferencing pointer to
> incomplete type ‘struct gpiod_chip’
>       arr[c].chip = rtapi_kmalloc(sizeof(*arr[c].chip), RTAPI_GFP_KERNEL);
> "
> 
> The exact same structure compiles without error for the
> gpiod_line_bulk a few lines lower. I think that the difference is that
> gpiod.h includes the definition of struct gpiod_line_bulk, but only
> the prototype of struct gpiod_chip.
> (This is mainly based on googling Stack Overflow, so might be wrong)
> 

You need the full definition of gpiod_chip to determine its size, and it
seems gpiod_chip is opaque, even in v1.  Yay?
So you don't need to alloc for it at all - gpiod_line_find() already did
that. As per that function's comment, you still need to close the chip
eventually to prevent memory leaks.

Btw, you don't need to keep the chip open once you have the line requests.

> > There is no code movement at all, this is just telling the linker to
> > link libgpiod.a directly into your executable, rather than dynamically
> > linking against libgpiod.so.
> >
> > The specifics of how to do that depend on your build.
> 
> But surely the compiler and linker need either the V2 code, or the
> library object? How does it access these if we don't have them in our
> repository?

Your build needs libgpiod v2, and the linker will include the relevant
bits from that in your execuatable that you then package.
The dependency on libgpiod then is only build-time, not run-time.

> Even if we add it as a build-time dependency, it isn't currently
> available in the usual places?

If necessary, you install it from source for your build.

> I must be missing something? Or are you assuming that I am the only
> one compiling LinuxCNC?
> 

I am assuming you are building an execuable to package as a binary.
If you are building a dev package then you can't hide the libgpiod
dependency from the end user, so you have the choice of either using
what is readily available or forcing the end user to install libgpiod
from source themselves.

Cheers,
Kent.

Re: [libgpiod] gpiod_line_get_value_bulk may be broken?

[andy pugh]

On Fri, 11 Aug 2023 at 01:59, Kent Gibson <warthog618@gmail.com> wrote:

> So you don't need to alloc for it at all - gpiod_line_find() already did
> that. As per that function's comment, you still need to close the chip
> eventually to prevent memory leaks.

Close then free?

> > I must be missing something? Or are you assuming that I am the only
> > one compiling LinuxCNC?

> I am assuming you are building an execuable to package as a binary.
> If you are building a dev package then you can't hide the libgpiod
> dependency from the end user, so you have the choice of either using
> what is readily available or forcing the end user to install libgpiod
> from source themselves

We do build binary packages, but not generally on my PC, instead the
builds happen on a number of buildbots, including the one at Debian.
Also, there are about 20 active developers who expect to pull the
repository from Git and have it build, and probably low-hundreds of
users who regularly build from source.

Basically, a git pull of the repository  + the listed build-time
dependencies has to build without anyone manually fetching an external
code repository.

-- 
atp
"A motorcycle is a bicycle with a pandemonium attachment and is
designed for the especial use of mechanical geniuses, daredevils and
lunatics."
— George Fitch, Atlanta Constitution Newspaper, 1912

Re: [libgpiod] gpiod_line_get_value_bulk may be broken?

[Kent Gibson]

On Fri, Aug 11, 2023 at 02:26:55AM +0100, andy pugh wrote:
> On Fri, 11 Aug 2023 at 01:59, Kent Gibson <warthog618@gmail.com> wrote:
> 
> > So you don't need to alloc for it at all - gpiod_line_find() already did
> > that. As per that function's comment, you still need to close the chip
> > eventually to prevent memory leaks.
> 
> Close then free?
> 

gpiod_chip_close() does both the close of the file descriptor and the free
of the allocated struct.

Cheers,
Kent.

How to use gpiod_line_set_flags

[andy pugh]

I am somewhat puzzled by an error report when I try to use the
function ‘gpiod_line_set_flags’

 warning: implicit declaration of function ‘gpiod_line_set_flags’

Many other function calls in the same code are working just fine.
gpiod_line_set_flags() appears as a function declaration in the header
file looking just like the ones that do work (as far as I can tell)

At the moment I have the code commented out, but I was hoping to be
able to use the flags to control pull-up and pull-down.

-- 
atp
"A motorcycle is a bicycle with a pandemonium attachment and is
designed for the especial use of mechanical geniuses, daredevils and
lunatics."
— George Fitch, Atlanta Constitution Newspaper, 1912

Re: How to use gpiod_line_set_flags

[Kent Gibson]

On Mon, Aug 14, 2023 at 11:25:18PM +0100, andy pugh wrote:
> I am somewhat puzzled by an error report when I try to use the
> function ‘gpiod_line_set_flags’
> 
>  warning: implicit declaration of function ‘gpiod_line_set_flags’
> 
> Many other function calls in the same code are working just fine.
> gpiod_line_set_flags() appears as a function declaration in the header
> file looking just like the ones that do work (as far as I can tell)
> 
> At the moment I have the code commented out, but I was hoping to be
> able to use the flags to control pull-up and pull-down.
> 

gpiod_line_set_flags() was a relatively recent addition to libgpiod, so
my guess is you have an old gpiod.h being included by your build.
Though that was added in 1.5, so it would have to be older than that.

Cheers,
Kent.

Re: How to use gpiod_line_set_flags

[andy pugh]

On Tue, 15 Aug 2023 at 01:49, Kent Gibson <warthog618@gmail.com> wrote:

> gpiod_line_set_flags() was a relatively recent addition to libgpiod, so
> my guess is you have an old gpiod.h being included by your build.
> Though that was added in 1.5, so it would have to be older than that.

Ah, yes, that will be it. I forgot that I went back to Raspbian/Buster
and that has v1.2


--
atp
"A motorcycle is a bicycle with a pandemonium attachment and is
designed for the especial use of mechanical geniuses, daredevils and
lunatics."
— George Fitch, Atlanta Constitution Newspaper, 1912

Re: [libgpiod] gpiod_line_get_value_bulk may be broken?

[Bartosz Golaszewski]

On Sat, Jul 29, 2023 at 4:03 AM Kent Gibson <warthog618@gmail.com> wrote:
>

[snip]

> >
> > libgpiod2 (and any further bugfix to 1.6 for that matter) have the
> > problem of not being currently available as packages in the
> > distributions we use.
> >
>
> Be aware that the libgpiod2 library package in Debian is actually libgpiod v1
> - currently 1.6.3 in stable[1].
> Not sure about the story behind that - before my time.
> And no idea what they will package libgpiod v2 as, libgpiod-2??, but
> there will be confusion whatever they do.

It's because of the difference in ABI and API versions. Early on in
libgpiod v1.x development there was an ABI change that didn't affect
the API. Debian packages the shared library files with the .so version
suffix for some reason.

Bart

[snip]