Use Linux kernel's character device API to implement IRQ capability

[2bndy5]

This guide does mention how IRQ could be done using some poll() function (which is unknown to me), but it uses a blocking example.

The main obstacle (for me) is using a separate processor thread to poll the GPIO for IRQ edge detection.

Originally posted by @2bndy5 in #942 (comment)

---

Useful resources

• Linux kernel GPIO character device docs (for ABI v2)
• poll() docs
• pthread docs

[TMRh20]

FYI just tested the interrupt example and pigpio doesn't seem to work on the RPi5 yet.

+---------------------------------------------------------+
|Sorry, this system does not appear to be a raspberry pi. |
|aborting.                                                |
+---------------------------------------------------------+

Now you have me playing with pthreads again... this is going to take some time. I have a simple example triggering an IRQ using pins 22 or others that are toggled manually, but can't get it to trigger on the radio IRQ pin for some reason. Wondering if it needs to be set to an input or something first? The guide doesn't seem to do that...

Also wondering if I should continue, or if this is something you wanted to take on?

[2bndy5]

I could take a stab at it, but I'll need you to verify RPi5 compatibility.

[2bndy5]

Still researching pthread... Looks like the function passed to pthread_create() only executes once in a separate thread -- using default scheduling and thread attrs, I think -- then the thread terminates upon exit of the passed function.

Clearly, I've never done multi-threaded processing in C++. Some of my input here might be obvious to those with experience using pthread.

[2bndy5]

Ok, I'm booted into Ubuntu and inspecting /usr/include/linux/gpio.h. And I just found out that there is a v2 API in gpio.h. Some of the structs used in #942 are actually deprecated already 😡. For example:
gpiohandle_request is defined as

/**
 * struct gpiohandle_request - Information about a GPIO handle request
 * @lineoffsets: an array of desired lines, specified by offset index for the
 * associated GPIO device
 * @flags: desired flags for the desired GPIO lines, such as
 * %GPIOHANDLE_REQUEST_OUTPUT, %GPIOHANDLE_REQUEST_ACTIVE_LOW etc, added
 * together. Note that even if multiple lines are requested, the same flags
 * must be applicable to all of them, if you want lines with individual
 * flags set, request them one by one. It is possible to select
 * a batch of input or output lines, but they must all have the same
 * characteristics, i.e. all inputs or all outputs, all active low etc
 * @default_values: if the %GPIOHANDLE_REQUEST_OUTPUT is set for a requested
 * line, this specifies the default output value, should be 0 (low) or
 * 1 (high), anything else than 0 or 1 will be interpreted as 1 (high)
 * @consumer_label: a desired consumer label for the selected GPIO line(s)
 * such as "my-bitbanged-relay"
 * @lines: number of lines requested in this request, i.e. the number of
 * valid fields in the above arrays, set to 1 to request a single line
 * @fd: if successful this field will contain a valid anonymous file handle
 * after a %GPIO_GET_LINEHANDLE_IOCTL operation, zero or negative value
 * means error
 *
 * Note: This struct is part of ABI v1 and is deprecated.
 * Use &struct gpio_v2_line_request instead.
 */
struct gpiohandle_request {
	__u32 lineoffsets[GPIOHANDLES_MAX];
	__u32 flags;
	__u8 default_values[GPIOHANDLES_MAX];
	char consumer_label[GPIO_MAX_NAME_SIZE];
	__u32 lines;
	int fd;
};

and gpio_v2_line_request is defined as

/**
 * struct gpio_v2_line_request - Information about a request for GPIO lines
 * @offsets: an array of desired lines, specified by offset index for the
 * associated GPIO chip
 * @consumer: a desired consumer label for the selected GPIO lines such as
 * "my-bitbanged-relay"
 * @config: requested configuration for the lines.
 * @num_lines: number of lines requested in this request, i.e. the number
 * of valid fields in the %GPIO_V2_LINES_MAX sized arrays, set to 1 to
 * request a single line
 * @event_buffer_size: a suggested minimum number of line events that the
 * kernel should buffer.  This is only relevant if edge detection is
 * enabled in the configuration. Note that this is only a suggested value
 * and the kernel may allocate a larger buffer or cap the size of the
 * buffer. If this field is zero then the buffer size defaults to a minimum
 * of @num_lines * 16.
 * @padding: reserved for future use and must be zero filled
 * @fd: if successful this field will contain a valid anonymous file handle
 * after a %GPIO_GET_LINE_IOCTL operation, zero or negative value means
 * error
 */
struct gpio_v2_line_request {
	__u32 offsets[GPIO_V2_LINES_MAX];
	char consumer[GPIO_MAX_NAME_SIZE];
	struct gpio_v2_line_config config;
	__u32 num_lines;
	__u32 event_buffer_size;
	/* Pad to fill implicit padding and reserve space for future use. */
	__u32 padding[5];
	__s32 fd;
};

[TMRh20]

LOL.

Well I don't know about you, but I'm taking a break.

[2bndy5]

Yeah, I have plans for later today. But I'll keep at it throughout the coming weeks. I think I'll finish the clang-format updates first.

[2bndy5]

I've also been looking at the libgpiod code, and they cache everything, possibly because nVidia Jetson/TX2/etc (& probably some RPi clones) can have multiple character devices (dev/gpiochipX) for GPIO ports.

[2bndy5]

Apparently, MRAA can detect RPi5 hardware, but I don't know if that means MRAA will completely work on RPi5. They have been supporting character device API for some time...

Pigpio (joan2937/pigpio#586) and BCM2835 (google groups discussion) libs are still assessing how to implement compatibility. I doubt BCM2835 will go on supporting the new hardware paradigm (which ultimately requires interfacing with PCI express). Pigpio might move forward since it already uses the Linux kernel for some stuff.

Remember, WiringPi is dead and littleWire has been broken since before I joined nRF24 org. It is starting to look like our SPIDEV driver might be the only way to support Linux in the future. I'm not saddened by this from a maintenance point of view, but user projects will suffer this new RPi hardware paradigm.

[2bndy5]

libgpiod's asynch_watch_line_value.c example seems like a big clue about using pthread with char-dev API.

[TMRh20]

Yeah, I had a feeling the GPIO changes would be slower than previous implementations, but never did any performance testing. Now that we have some working code for v2, caching everything shouldn't be a big challenge.

[2bndy5]

I took a break. From a design perspective, I think the gpio caches and interrupt implementation should be separate to reduce the created thread's resources.

As far as caching, I think we can just have 1 gpio_line_request and use the gpio_line_request.attrs to configure multiple offsets as input or output. I'm also considering 1 cached gpio_line_request for input lines and another for output lines. Not sure about GPIO destructor though since the cached FDs would have to be closed upon before app exit.

I might be entirely overthinking again.

[TMRh20]

Hey if it works it works. We should be able to get the same or better performance out of it than prior versions I would hope.

I agree with keeping the GPIO and IRQ stuff separate. The caching I don't know about, I'd have to look at GPIO code more in depth to form a relevant opinion.

[2bndy5]

So I think I'm caching the FDs properly (in char-dev-irq branch).

• I have a gpio_cache struct closing the FDs in it's destructor, so there shouldn't be any need to modify RF24's destructor.
• I'm only using 1 cached gpio_v2_line_request obj for all pins used by RF24; uses request.config.attrs[offset].attr.flags instead of request.config.flags.
• I also cache 1 gpio_v2_line_values obj for all pins' input/output ops.

It compiles and executes without errors, but running the scanner example shows no signal gets detected. 😞

On a side note, I think our old sysfs approach does not properly free the pins upon app exit. I have to reboot my RPi to allow the char-dev API access to the GPIO22.

[TMRh20]

Nice work!

Mostly tests fine for me, but I am seeing differences in the scanner example on RPi5, RPi4 and RPi3.
The RPi2 seems to not care.

Teh RPI4 comes up with long lines like this sometimes tho:
666666655555555555554444444444433333333333333333333333333333333333333333333333333333333333333333333333333334444444444445556666

Also, on a side note, since the FDs are cached and remain open, the driver now gives a nice error report if you try to run two instances of RF24 at the same time, using the same pins. Sweet deal, this is something I did regularly when running through tests etc. so its a nice behavior to have.

[TMRh20]

Hmm, taking a look at RF24.cpp, I found something unusual that I haven't thought about in a while, but see the lines here

Essentially, on faster devices, I put in a delay of 5us when toggling the CS pin. If I modify the code to the following, it seems to work much better on RPi with the scanner example.

#if !defined(RF24_LINUX)
    digitalWrite(csn_pin, mode);
#else
    static_cast<void>(mode); // ignore -Wunused-parameter
#endif // !defined(RF24_LINUX)
  delayMicroseconds(csDelay);  // Delay for all devices
}

Maybe we could do something like the following for the CS pin?:

#if (!defined(F_CPU) || F_CPU > 20000000) && defined FAILURE_HANDLING 
    delayMicroseconds(csDelay);
#endif

The whole point of the delay was double:
a: Ensure the pin gets toggled for at least 5us
b: Slow down the polling via SPI, leaving the radio to process radio data instead of being hammered by SPI requests

The higher layers seem to perform better too with this change.

[2bndy5]

I was going to play with char-dev debouncing too. I think with caching, we've hit the too fast problem.

[2bndy5]

its a nice behavior to have

I also set the consumer string in the request obj. So, now if you run gpioinfo gpiochip0 (a tool provided by libgpiod) while a RF24 app is running, you'll see which pins are consumed by the "RF24 lib".

[TMRh20]

I think with caching, we've hit the too fast problem.

Yup, there needs to be some sort of GPIO/CSN delay for Linux now along with the faster MCUs. Will you include this in your branch then?

[2bndy5]

Yeah, coding the debouncing now. There's a limit of 10 attrs that we can configure for each request.config, so I'm going with 3: 1 attr to declare which pins are inputs, 1 attr to declare which pins are outputs, and 1 attr to declare the debouncing period of 5 microseconds on each output pin.

[2bndy5]

I think only 1 type of attr (input flag, output flag, debounce period) can be associated with a pin. Meaning, setting an attr for output and another attr for debouncing on 1 pin doesn't take.

/**
 * struct gpio_v2_line_config - Configuration for GPIO lines
 * @flags: flags for the GPIO lines, with values from &enum
 * gpio_v2_line_flag, such as %GPIO_V2_LINE_FLAG_ACTIVE_LOW,
 * %GPIO_V2_LINE_FLAG_OUTPUT etc, added together.  This is the default for
 * all requested lines but may be overridden for particular lines using
 * @attrs.
 * @num_attrs: the number of attributes in @attrs
 * @padding: reserved for future use and must be zero filled
 * @attrs: the configuration attributes associated with the requested
 * lines.  Any attribute should only be associated with a particular line
 * once.  If an attribute is associated with a line multiple times then the
 * first occurrence (i.e. lowest index) has precedence.
 */
struct gpio_v2_line_config {
	__aligned_u64 flags;
	__u32 num_attrs;
	/* Pad to fill implicit padding and reserve space for future use. */
	__u32 padding[5];
	struct gpio_v2_line_config_attribute attrs[GPIO_V2_LINE_NUM_ATTRS_MAX];
};

My initial (local) test has not shown any difference. I pushed what I have in case you can test with it. I also tried using 1 attr to define both output flag and debouncing period (for all output pins), alas I still don't see a difference.

I might have to switch to a std::map of port -> request key/value pairs for each pin used. Alternatively, I guess we could try altering RF24.cpp, but I was hoping to avoid more ifdef soup.

[TMRh20]

Yeah, I just tested it and I don't think debounce introduces a delay of 5us after toggling, it would be to prevent togging more often than 5us, so I don't think it will work in this application.
We might need an actual delay. Unfortunately I've been thinking about adding another, separate define to specifically make more functions interrupt safe, as right now all you can do is comment out FAILURE_HANDLING. It might be best left as is though lol.

[2bndy5]

My std::map attempt yielded same result. Its a pain when Linux kernel doc strings are so terse and online tutorials are scarce or outdated.

I think the debounce period is more for input pins. https://www.kernel.org/doc/html/v4.17/driver-api/gpio/driver.html#gpios-with-debounce-support

I added the delayMicroseconds() call to RF24::ce(). 😞 18045c7
We should probably have a RF24_SPIDEV defined so we only delay 5us on CE toggle when using the SPIDEV driver. Other Linux drivers are slow enough to not have this problem.

[2bndy5]

Seems to be working now. I'm going to take a break before tackling the IRQ implementation...

[TMRh20]

I'm still getting the same erroneous results with the scanner examples with your current code.

The CE pin is only toggled on transmit, and delays are added in the lib already if needed. re: startWrite()
Adding a delay to CS will affect reception as well as how often available() can be called etc.

[2bndy5]

But the gpio stuff in SPIDEV isn't managing the CS. The asserted CE used for entering RX is how I perceived the problem with the scanner example. Works fine on my RPi3.

[2bndy5]

With polling it always ends up returning true and triggering the interrupt in an endless loop.

In theory, using read() to empty the kernel event buffer (for that requested line) should in turn make poll() clear the POLLIN flag in poll_fd.revents. But it isn't...

Also, according to the Linux kernel docs:

The read() will block if no event is available and the req_fd has not been set O_NONBLOCK.

[2bndy5]

well at least we know the debounce attr was having an effect, but I don't think that is the desired behavior we're looking for.

        data_sent: 1, data_fail: 0, data_ready: 1
   'Data Fail' event test failed

Maybe if we decrease the debounce time? I only ever tried increasing the debounce time and it didn't seem to have an effect while using poll().

[2bndy5]

I also tried using a __usleep(100) after invoking the callback. I thought: Maybe we made it too fast again. A good problem to have but something I don't have to worry about in python. 🤣

[TMRh20]

Hmm, the data_fail test is passing now, not sure what was going on before, but I have the interrupt example running with RF24Gateway now with the two changes mentioned above and it works great!

[2bndy5]

With only using read() (neglecting poll()), we could track the gpio_v2_line_event.line_seqno value and only invoke the callback when it increases.

[2bndy5]

Yeah, I expect it to work much faster than any of our previous IRQ support/attempts. The char-dev API is definitively better than the old sysfs API.

[2bndy5]

I also modified the example to timeout after waiting 500ms for an IRQ.

Can you push these changes? I like this idea.

[TMRh20]

You want me to push all the changes or just to the example?

[2bndy5]

just the example changes. I'm reworking your read() only suggestion to account for events' sequential number.

[2bndy5]

Yeah, the debounce config was mucking it up somehow. Sad that we can't get that to work (for now). I pushed your read() only suggestion with line_seqno tracking. I can confirm the example works as expected with those changes.

[2bndy5]

I would be more excited if I didn't have to work so hard toward this achievement. Now I just feel like I've learned all new ways in which I am a dummy. 😆

[TMRh20]

Haha, welcome to the club.

[2bndy5]

Now I want to subsidize the other Linux drivers to use this IRQ implementation where the driver doesn't already have IRQ support... for both CMake and the old ./configure script.

[TMRh20]

Pretty nice not having to run as root and still have interrupts!

[2bndy5]

I'll probably have to expose this in pyRF24 as well. Currently, that lib's IRQ example uses RPi.GPIO for IRQ support. But RPi.GPIO (& pigpio) won't work on RPi5...

BTW, installing the python binding for libgpiod (via pip) requires users build it from source. I'm not sure if pyRF24 will compile properly for binary distribution since it requires the system have linux/gpio.h header, and the CI uses CentOS (or the CentOS replacement Rocky Linux) to compile the pyRF24 wrappers.

I see piwheels can build gpiod lib with linux/gpio.h present (that gpiod pkg requires python v3.9+), so 32-bit RPi OS will still be able to use binary dists from piwheels (as is done currently).

[2bndy5]

Just pushed some major code clean-up (specifically for Linux drivers)

• revised all #include statements and removed any unnecessary ones
• added char-dev IRQ implementation to RPi driver
• add #define attachInterrupt wiringPiISR to wiringPi wrapper/driver; this allows us to include compiling interruptConfigure.cpp with the wiringPi driver
• fixed compiling examples using wiringPi with old makefile
• removed any build conditions in which IRQ support was dependent on pigpio presence; including
  • CMake
  • old configure/makefile scripts
  • pyRF24/setup.py
• interrupt source for SPIDEV and RPi drivers use their own IRQException class
• revised the SPIException messages thrown in SPIDEV/spi.cpp; they now show more helpful info

I test-compiled these changes in WSL Ubuntu, but I haven't actually tested these changes on hardware yet.

[TMRh20]

Have this running via SPIDEV on my RPi4 & 5 and others soon!

[2bndy5]

This thread just became the 2^nd-most commented issue in the repo. I'm going to sleep now. I'll start HW testing tomorrow.

[TMRh20]

When ur up and at er, plz take a look at

RF24/utility/SPIDEV/interrupt.cpp

Line 79 in a066d0c

request.event_buffer_size = 16U;

I was having some problems on my RPi3 with heavy traffic running the Gateway Interrupt Ncurses example, and added some code to check for out-of-sync interrupts. There were some interrupts being dropped so I increased this to 48U. The return value of read is also 48, so I think there may have been a problem with overflowing the buffer but I'm not sure based on the documentation.

[2bndy5]

According to the docs & header comments), its actually the default value when not specified (16 * num_lines). I also increased it to sizeof(gpio_v2_line_event) (48) when troubleshooting the poll() + read() approach. I didn't see a difference in read() behavior, so I didn't push the change. I have no objections to using sizeof(gpio_v2_line_event) because it makes sense. Maybe the docs and header comments are outdated because sizeof(gpioevent_data) (from ABI v1) is 12 bytes (with no explicit padding).

[2bndy5]

We could try printing a warning in poll_rq()

if (lastEventSeqNo + 1 != irqEventInfo.line_seqno) {
   // I haven't had much luck using printf() from a non-main thread
   std::cout << "missed event on IRQ pin" << std::endl;
}

just to see if/when this occurs during testing.

[TMRh20]

I think if we were to add something I would prefer a counter or something that could be accessed outside RF24, because I do a lot of my testing at the Gateway/Ethernet layers using the ncurses examples.

[2bndy5]

We could pass the event number into the ISR callback. For that matter, we could just pass the entire irqEventInfo data into the callback. I did this during troubleshooting but considered it a breaking change. WiringPi expects the ISR signature to use no args like Arduino does.

[2bndy5]

We also can't use an ISR signature with a default value to avoid breaking behavior

void interruptHandler(void* eventInfo = nullptr);
attachInterrupt(24 FALLING, &interruptHandler);

because void (*function)(void) is not the same as void (*function)(void*) (casting won't help either)

[TMRh20]

I think we're getting a bit complicated, maybe just stick to the cout lol

[2bndy5]

pushed increase to event_buffer_size

[2bndy5]

I just tested the IRQ implementation with the RPi driver on my RPi3 & RPi4. On my RPi4, I got garbled text printed from the RX FIFO:

*** PRESS 'T' to begin transmitting to the other node
*** PRESS 'R' to begin receiving from the other node
*** PRESS 'Q' to exit
r
Complete RX FIFO: Piog0Xw0{qv}
*** PRESS 'T' to begin transmitting to the other node
*** PRESS 'R' to begin receiving from the other node
*** PRESS 'Q' to exit
t

# ... all event flags passed the checks ...

Complete RX FIFO: Yak Cqs0ACo

I lowered the SPI speed on my RPi4 to 6MHz and it worked out fine. Weird that I didn't get that problem with the SPIDEV driver using default SPI speed.

[2bndy5]

Using wiringPi on RPi4 with interruptConfigure.cpp, I had to change the pin numbers according to

gpio readall output

 +-----+-----+---------+------+---+---Pi 4B--+---+------+---------+-----+-----+
 | BCM | wPi |   Name  | Mode | V | Physical | V | Mode | Name    | wPi | BCM |
 +-----+-----+---------+------+---+----++----+---+------+---------+-----+-----+
 |     |     |    3.3v |      |   |  1 || 2  |   |      | 5v      |     |     |
 |   2 |   8 |   SDA.1 | ALT0 | 1 |  3 || 4  |   |      | 5v      |     |     |
 |   3 |   9 |   SCL.1 | ALT0 | 1 |  5 || 6  |   |      | 0v      |     |     |
 |   4 |   7 | GPIO. 7 |   IN | 1 |  7 || 8  | 1 | ALT5 | TxD     | 15  | 14  |
 |     |     |      0v |      |   |  9 || 10 | 1 | ALT5 | RxD     | 16  | 15  |
 |  17 |   0 | GPIO. 0 |   IN | 0 | 11 || 12 | 0 | IN   | GPIO. 1 | 1   | 18  |
 |  27 |   2 | GPIO. 2 |   IN | 0 | 13 || 14 |   |      | 0v      |     |     |
 |  22 |   3 | GPIO. 3 |   IN | 0 | 15 || 16 | 0 | IN   | GPIO. 4 | 4   | 23  |
 |     |     |    3.3v |      |   | 17 || 18 | 1 | IN   | GPIO. 5 | 5   | 24  |
 |  10 |  12 |    MOSI | ALT0 | 0 | 19 || 20 |   |      | 0v      |     |     |
 |   9 |  13 |    MISO | ALT0 | 0 | 21 || 22 | 0 | IN   | GPIO. 6 | 6   | 25  |
 |  11 |  14 |    SCLK | ALT0 | 0 | 23 || 24 | 1 | OUT  | CE0     | 10  | 8   |
 |     |     |      0v |      |   | 25 || 26 | 1 | OUT  | CE1     | 11  | 7   |
 |   0 |  30 |   SDA.0 |   IN | 1 | 27 || 28 | 1 | IN   | SCL.0   | 31  | 1   |
 |   5 |  21 | GPIO.21 |   IN | 1 | 29 || 30 |   |      | 0v      |     |     |
 |   6 |  22 | GPIO.22 |   IN | 1 | 31 || 32 | 0 | IN   | GPIO.26 | 26  | 12  |
 |  13 |  23 | GPIO.23 |   IN | 0 | 33 || 34 |   |      | 0v      |     |     |
 |  19 |  24 | GPIO.24 |   IN | 0 | 35 || 36 | 0 | IN   | GPIO.27 | 27  | 16  |
 |  26 |  25 | GPIO.25 |   IN | 0 | 37 || 38 | 0 | IN   | GPIO.28 | 28  | 20  |
 |     |     |      0v |      |   | 39 || 40 | 0 | IN   | GPIO.29 | 29  | 21  |
 +-----+-----+---------+------+---+----++----+---+------+---------+-----+-----+
 | BCM | wPi |   Name  | Mode | V | Physical | V | Mode | Name    | wPi | BCM |
 +-----+-----+---------+------+---+---Pi 4B--+---+------+---------+-----+-----+

#define IRQ_PIN 5
#define CE_PIN 3

But I still get the following error on my RPi4:

gpio: Unable to open GPIO direction interface for pin 24: No such file or directory
wiringPiISR: unable to open /sys/class/gpio/gpio24/value: No such file or directory

where ls /sys/class/gpio returns

export  gpiochip512  gpiochip570  unexport

On my RPi3, I get "radio hardware is not responding!!"

---

MRAA on my RPi4 is throwing a std::invalid_argument exception about "Invalid GPIO pin specified" on my RPi4. MRAA on my RPi3 just says "radio hardware is not responding!!" I had to use the scanner example because the interrupt example is not compatible with MRAA.

I haven't really modified the MRAA source code on this branch, so I guess I'll leave that problem to another time (if at all).