Giter Club home page Giter Club logo

Comments (113)

TMRh20 avatar TMRh20 commented on June 21, 2024 2

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.

from rf24.

TMRh20 avatar TMRh20 commented on June 21, 2024 2

Don't ask me I just work here... and the pay is not too hot either.
I can take a closer look soon hopefully, been a long time since I did anything with pthreads.

from rf24.

Avamander avatar Avamander commented on June 21, 2024 2

how long it takes to get some high powered versions of the 52x?

Depends on what you mean by high-powered version, there's the nRF21540 and nRF54H20 for two types of power by Nordic, there are a few designs with the former out there last I saw.

from rf24.

Avamander avatar Avamander commented on June 21, 2024 2

but only as a complement to the nRF52 or nRF53 series chips.

Sorry for replying to an old comment. The chip is optimized for nRF SoCs but the documentation says it doesn't really care - it can be used with any.

It's tempting to get the EK and see how it fares with just the nRF24.

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024 1

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

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024 1

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.

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024 1

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.

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024 1

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".

from rf24.

TMRh20 avatar TMRh20 commented on June 21, 2024 1

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.

from rf24.

TMRh20 avatar TMRh20 commented on June 21, 2024 1

I think the CE delay makes more logical sense, so lets go with that for now. We can always adjust later if needed.

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024 1

Further tests without CE delay seem to be fine in gettingstarted. The scanner example on my RPi4 and RPi3 still require at least a 1us delay. I think the scanner example alone just needs an added delay instead of every call to ce(), but raising the wait time to 130 does not have the same effect as having the delay in ce(). 🤷🏼

from rf24.

TMRh20 avatar TMRh20 commented on June 21, 2024 1

Changing the CE pins direction while the RF24 driver is running? lol
You might be overthinking again, but I'll leave this one with you to figure out a proper solution.

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024 1

You might be overthinking again

Oh, I definitely am. I wouldn't have even worried about it if the code was namespaced. I'm also hoping to code it in a way that can be abstracted as a submodule (separate lib). I would love to be able to have this much Linux kernel wrappers (+ my work on wrapping I2C) for other Arduino libs.

All my work on that branch is in my Ubuntu partition (easier to look at the gpio.h and libgpiod code that way). I'll take a look at it tomorrow. Thanks for the steps to reproduce!

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024 1

Ok I looked into it, and it looks like the request.fd can't be re-opened once it is already opened.

this patch worked

diff --git a/examples_linux/gettingstarted.cpp b/examples_linux/gettingstarted.cpp
index 78e244a..a1d1187 100644
--- a/examples_linux/gettingstarted.cpp
+++ b/examples_linux/gettingstarted.cpp
@@ -57,6 +57,7 @@ int main(int argc, char** argv)
         cout << "radio hardware is not responding!!" << endl;
         return 0; // quit now
     }
+    radio.begin();
 
     // to use different addresses on a pair of radios, we need a variable to
     // uniquely identify which address this radio will use to transmit
diff --git a/utility/SPIDEV/gpio.cpp b/utility/SPIDEV/gpio.cpp
index aab3dbc..340ce1a 100644
--- a/utility/SPIDEV/gpio.cpp
+++ b/utility/SPIDEV/gpio.cpp
@@ -107,12 +107,15 @@ void GPIO::open(rf24_gpio_pin_t port, int DDR)
         attr_input.mask |= (1LL << offset);
     }
 
-    int ret = ioctl(gpio_cache.fd, GPIO_V2_GET_LINE_IOCTL, &request);
-    if (ret == -1) {
-        std::string msg = "[GPIO::open] Can't get line handle from IOCTL; ";
-        msg += strerror(errno);
-        throw GPIOException(msg);
-        return;
+    int ret;
+    if (request.fd <= 0) {
+        ret = ioctl(gpio_cache.fd, GPIO_V2_GET_LINE_IOCTL, &request);
+        if (ret == -1) {
+            std::string msg = "[GPIO::open] Can't get line handle from IOCTL; ";
+            msg += strerror(errno);
+            throw GPIOException(msg);
+            return;
+        }
     }
     ret = ioctl(request.fd, GPIO_V2_LINE_SET_CONFIG_IOCTL, &request.config);
     if (ret == -1) {


PS - I just found out I can code in a repo on my RPi from my everyday PC using VSCode's Remote SSH feature! 😲 I actually pushed the fix from my RPi using VSCode remotely.

from rf24.

TMRh20 avatar TMRh20 commented on June 21, 2024 1

Hmm, I can't seem to get it working on any pin except 22 even with 1 radio, every other pin I try results in [GPIO::write] Can't set line value from IOCTL; Operation not permitted

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024 1

I think I need to switch to using a separate request object for each pin. This would involve std::map...

from rf24.

TMRh20 avatar TMRh20 commented on June 21, 2024 1

Tested the latest on my RPi5, 4 & 3. No problems! I haven't actually looked at the code yet, but it works.

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024 1

I just rebased char-dev-irq branch to pull in updates from #952. Transmission times (on my RPi3 and RPi4) are just as good as (or better than) before with using sysfs API. 🚀 And the scanner example works with no additional delay in ce(). 🎉

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024 1

I'm not sure if we can keep the cached FDs separate for IRQ and GPIO. The convention is

pinMode(24, INTPUT); // caches the request.fd in gpio.cpp

// attachInterrupt() cannot claim pin 24 because
// pinMode() cached an open FD in gpio.cpp
attachInterrupt(24, INT_EDGE_FALLING, &myISR);

The real problem is that GPIO::open() does not configure the pin for polling it as an input (requires additional request.config.flags be set, not to mention the debounce time attribute).

I could call GPIO::close() from attachInterrupt() to deallocate the request.fd corresponding to the specified pin. This would require an actual definition for GPIO::close() (which is currently blank).

from rf24.

TMRh20 avatar TMRh20 commented on June 21, 2024 1

Heh, I guess that just speaks to the general popularity of these radios. I wonder how long it will take the community in general to adapt to nrf52x and how long it takes to get some high powered versions of the 52x?
The nrf24 still holds its own pretty well, seeing as we have no radio IRQs for the 52x.

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024 1

I thought he meant "better PA/LNA". The nRF21540 looks like it could get the job done but only as a complement to the nRF52 or nRF53 series chips.

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024 1

Yep. I figured taking a break and coming back to it with fresh eyes might help. It didn't.

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024 1

I am seeing a problem with gpioinfo gpiochip0 while running the example in idle (waiting for user prompt [T/R/Q]):

        line  22:     "GPIO22"   "RF24 lib"  output  active-high [used]
        line  23:     "GPIO23"       unused   input  active-high 
        line  24:     "GPIO24"       unused   input  active-high 

Pin 24 should read "RF24 IRQ" instead of "unused" because that is the pin I'm using (locally) in the example.

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024 1

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.

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024 1

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

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024 1

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.

from rf24.

TMRh20 avatar TMRh20 commented on June 21, 2024 1

Haha, welcome to the club.

from rf24.

TMRh20 avatar TMRh20 commented on June 21, 2024 1

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

from rf24.

TMRh20 avatar TMRh20 commented on June 21, 2024

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?

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

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.

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

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;
};

from rf24.

TMRh20 avatar TMRh20 commented on June 21, 2024

LOL.

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

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

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.

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

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.

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

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

from rf24.

TMRh20 avatar TMRh20 commented on June 21, 2024

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.

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

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.

from rf24.

TMRh20 avatar TMRh20 commented on June 21, 2024

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.

from rf24.

TMRh20 avatar TMRh20 commented on June 21, 2024

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.

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

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

from rf24.

TMRh20 avatar TMRh20 commented on June 21, 2024

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?

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

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.

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

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.

from rf24.

TMRh20 avatar TMRh20 commented on June 21, 2024

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.

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

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.

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

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

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

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.

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

ok, I'm getting periodic failures in gettingstarted on RPi3 followed by very high transmission times. I'm not getting any errors on my RPi4 with gettingstarted. Is this similar to what you're seeing?

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

I applied your idea locally

diff --git a/RF24.cpp b/RF24.cpp
index d4a7b33..7d7836e 100644
--- a/RF24.cpp
+++ b/RF24.cpp
@@ -92,10 +92,11 @@ void RF24::csn(bool mode)
 
 #if !defined(RF24_LINUX)
     digitalWrite(csn_pin, mode);
-    delayMicroseconds(csDelay);
+    //delayMicroseconds(csDelay);
 #else
     static_cast<void>(mode); // ignore -Wunused-parameter
 #endif // !defined(RF24_LINUX)
+    delayMicroseconds(csDelay);
 }
 
 /****************************************************************************/
@@ -108,7 +109,7 @@ void RF24::ce(bool level)
 #endif
         digitalWrite(ce_pin, level);
 #ifdef RF24_LINUX
-        delayMicroseconds(5);
+        //delayMicroseconds(5);
 #endif
 #ifndef RF24_LINUX
     }

It yielded the same result but much slower scanner sweeps. The gettingstarted transmission times were considerably slower but more just as reliable on my RPi3

Transmission successful! Time to transmit = 1042 us. Sent: 0
Transmission successful! Time to transmit = 1047 us. Sent: 0.01
Transmission successful! Time to transmit = 1066 us. Sent: 0.02
Transmission successful! Time to transmit = 1047 us. Sent: 0.03
Transmission successful! Time to transmit = 1027 us. Sent: 0.04
Transmission successful! Time to transmit = 1031 us. Sent: 0.05
Transmission successful! Time to transmit = 1039 us. Sent: 0.06
Transmission successful! Time to transmit = 2752 us. Sent: 0.07
Transmission successful! Time to transmit = 2658 us. Sent: 0.08
Transmission successful! Time to transmit = 2721 us. Sent: 0.09
Transmission successful! Time to transmit = 1024 us. Sent: 0.1
Transmission failed or timed out
Transmission successful! Time to transmit = 4387 us. Sent: 0.11
Transmission failed or timed out
Transmission successful! Time to transmit = 2675 us. Sent: 0.12
Transmission successful! Time to transmit = 1039 us. Sent: 0.13
Transmission successful! Time to transmit = 9715 us. Sent: 0.14
Transmission successful! Time to transmit = 999 us. Sent: 0.15
Transmission successful! Time to transmit = 1038 us. Sent: 0.16
Transmission failed or timed out
Transmission failed or timed out
Transmission successful! Time to transmit = 1039 us. Sent: 0.17
Transmission successful! Time to transmit = 1023 us. Sent: 0.18
Transmission failed or timed out
Transmission failed or timed out
6 failures detected. Leaving TX role.

from rf24.

TMRh20 avatar TMRh20 commented on June 21, 2024

You are correct I figure, forgot to add the CE delay to RF24.cpp when testing. Now the scanner examples seem to work fine!

My gettingstarted examples seem to work fine with or without the CE delay.

It seems I was getting better high-speed transfers at the RF24Gateway layer with the CS delay. Testing with data over SSH and IPerf3. Now I don't know what the answer is. :p

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

I'm at a loss as well. This is why I can't have nice things.

from rf24.

TMRh20 avatar TMRh20 commented on June 21, 2024

I found only one issue so far after a bunch of testing on your char-dev-irq branch:
a: If the radio experiences an error or the radio is simply removed from the Raspberry Pi, it will crash if using RF24Mesh or RF24Gateway.
b: This is due to radio.begin() being called a second time and the subsequent call to pinMode(ce_pin,OUTPUT)

I think either we could add something in RF24.cpp if(!pinOpened){ pinMode(ce_pin,OUTPUT); } or else do something similar in GPIO.cpp, but you may have better ideas on how to handle this.

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

hmm. I thought I had some code in GPIO::open() that would check if the pin specified is already in cache. perhaps that isn't working?

int offset = gpio_cache.getPortOffset(port);
if (offset < 0) { // pin not in use; add it to cached request
offset = request.num_lines;
request.offsets[offset] = port;
request.num_lines += 1;
}

int getPortOffset(rf24_gpio_pin_t port)
{
int offset = -1;
for (__u32 i = 0; i < request.num_lines; ++i) {
if (request.offsets[i] == (__u32)port) {
offset = i;
break;
}
}
return offset;
}

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

__u32 i = 0

maybe use int 1 = 0;? Because offset is signed int (negative means not in cached).

I think the max lines per request obj is 64, so if we need to use uint32, then the sentinel -1 could instead be anything > 64.

from rf24.

TMRh20 avatar TMRh20 commented on June 21, 2024

Changing to int just gives a warning when compiling.
How about

    int offset = gpio_cache.getPortOffset(port);
    if (offset < 0) { // pin not in use; add it to cached request
        offset = request.num_lines;
        request.offsets[offset] = port;
        request.num_lines += 1;
    }else{
      return;    
    }

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

IDK, my gut says no because it just ignores the problem and lets begin() carry on without proper cache validation. I originally added that code because: What if the user ended up changing the pin's direction (for whatever reason)?

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

Now I'm having trouble init-ing another RF24 object. Kinda need to support multiple radio objects if there's more than 1 SPI bus (with multiple CS options for each).

from rf24.

TMRh20 avatar TMRh20 commented on June 21, 2024

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

I can pinMode(x, OUTPUT) multiple pins, but for some reason I can't run begin() on multiple RF24 objects. The problem seems to be in GPIO::write()

offset[0] = 22
initing second radio
offset[1] = 21
terminate called after throwing an instance of 'GPIOException'
  what():  [GPIO::write] Can't set line value from IOCTL; Operation not permitted
Aborted

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

Using std::map to cache request objects seems to be slower. But at least I can init a second, different pin and another different RF24 object (in the same program). Although scanner example still requires delayMicrosecond(1) in ce().

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

I figured out that we can use the same request object to configure different pins. All we have to cache is the request.fd after configuring with ioctl(). There's a bit more "code golfing" in f184a2f and b13d480, but I like the results (on my RPi3).

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

I see the wiringPi mirror on github just releases v3.0 with RPi5 support. Looking at the code, they're still using deprecated sysfs calls though, not the recommended char-dev API. I'm guessing to support RPi5, they went with a table to map BCM pin numbers to the newer sysfs pin numbers (which are inexplicably high like 4xx).

But how they do interrupts is interesting because it involves both poll() and pthread (with callbacks to cached function pointers provided by user code).

from rf24.

TMRh20 avatar TMRh20 commented on June 21, 2024

Good deal, wiringPi is pretty handy, so I'm glad they are still supporting it. Just tested with our RF24 libs on RPi5 and latest wiringPi and it works well too!

I'm guessing we will need to do something pretty similar for interrupts in SPIDEV.

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

Yep. Clearly, I'm still researching how to use poll() together with pthread... I plan on tackling that very soon.

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

Warning

I have re-based the char-dev-irq branch since merging #959. But I have not tested it at all. The changes in that branch are just what I had stashed locally (mostly theoretical code).

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

I recently found out that the original author of pigpio has started a new library called lg which uses the newer GPIO char-dev API (v2) and includes SPI, I2C, & USART. However, that lib still uses a daemon to mitigate access to hardware, so it's speed might be hindered like with pigpio.

It might already be shipped with recent RPi OS (& Ubuntu) images. I see this package's details on launchpad.net. And using sudo apt list --installed *lgpio* results in

liblgpio1/stable,now 0.2.2-1~rpt1 arm64 [installed,automatic]
python3-lgpio/stable,now 0.2.2-1~rpt1 arm64 [installed,automatic]

from rf24.

TMRh20 avatar TMRh20 commented on June 21, 2024

Its on my RPi5 running RPi OS Light as well. Another thing that needs changing although this I wouldn't say has much urgency to change because we have SPI dev pretty much working at full capacity.

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

lgpio repo has an example using the nRF24L01! I guess that could be a reference for anyone that wants a nRF24L driver written in pure C (in 1000+ lines). 😬

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

I wonder how long it will take

too long. probably years after nordic deprecates the nrf5 family. I think the price difference is really significant (nRF24L01+: <$4/unit & nRF52840: >$6/unit when not buying massive reels).

I've been waiting for someone to create a board based on the nRF5340, which released during the pandemic and was poorly received due to the chip shortage. The main advantage of the nRF5340 is BLE audio capability, which implies dual-core processor to de/encode audio codec in realtime. Imagine a DIY Star Trek TNG communicator...

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

I've been playing around on the char-dev-irq branch... The interruptConfigure example hangs when it gets to pthread_create(). IDK if I'm configuring the line request properly. I'm also not sure if I'm using poll() properly with the char-dev API.

from rf24.

TMRh20 avatar TMRh20 commented on June 21, 2024

Yeah I meant PA + LNA / Long range, but it looks like there are already variants with these features. Nice! Thanks for the info.

from rf24.

TMRh20 avatar TMRh20 commented on June 21, 2024

The interruptConfigure example hangs when it gets to pthread_create()...

Played around for a bit. I don't know whats going on so far. Even if I empty the poll_irq function and just leave a loop the main thread hangs. It returns fine if I remove the loop.

If I do the above and change the code to

    pthread_t testing = 0;
    pthread_create(&testing, NULL, poll_irq, NULL);

It seems to (almost) work and the main thread doesn't hang.

Seems to be something specific to pthreads though I think. Out of time for today, but will try again.

from rf24.

TMRh20 avatar TMRh20 commented on June 21, 2024

https://github.com/nRF24/RF24/blob/char-dev-irq/utility/SPIDEV/interrupt.cpp#L127 needs to be declared outside this function, since the function returns as soon as the thread is created. That seems to be the cause of the initial hang.

Then yes, I think there is something wrong with the polling as well. Its constantly returning > 0

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

needs to be declared outside this function,

Ah, yes. I was passing a reference that no longer existed after function exit. I fixed this locally by passing the reference to the cached instance instead.

I think there is something wrong with the polling as well. Its constantly returning > 0

Yeah, I noticed that too. I modified the poll_irq() (thread routine) to only callback to the user function when

int x = poll(poll_fd, 1, -1);
if (x > 0 && poll_fd.revents & POLLIN) {
     //call user function
     cache->function();
}
else if (x < 0) {
    throw GPIOException("failed to poll GPIO event; " + strerror(errno));
}

Unfortunately, this is still not triggering on GPIO falling events. I'll push what I have soon, but I'm currently trying to adequately close the pollin thread on app exit (pthread_join() isn't doing that in cache elements' destructor).

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

I pushed some changes... pthread_cancel() + pthread_testcancel() works better than just pthread_join(). Exiting the example without running the test now works without needing to press Ctrl + C

from rf24.

TMRh20 avatar TMRh20 commented on June 21, 2024

Per some poll docs adding some switch/case code to the IRQ, it seems the poll is returning: POLLNVAL The specified fd value is invalid.

It seem to be getting passed correctly to the poll_irq function, but obviously something is messed up.

I even tried removing the pthread call and polling the FD right after the SET_CONFIG call and it fails with the same msg.

from rf24.

TMRh20 avatar TMRh20 commented on June 21, 2024

Again I commented out the pthread call and replaced it with this:

    uint8_t buffer[sizeof(gpio_v2_line_event)];
    for (;;) {
        int result = read(request.fd,buffer,sizeof(gpio_v2_line_event));
        if(result > 0){
            
            printf("IRQ\n");
        }
        if(result < 0){
            printf("%s\n",strerror(errno));           
        }
    }

It again prints "Bad file descriptor."
This is odd, not sure where to go from here.

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

I got a bad descriptor error when I tried to read() the event from the kernel buffer (commented out code in poll_rq()). So, this POLLNVAL value tracks with my experience too.

I've been reading the Linux kernel docs about setting up a request line and reading events. But I don't see anything to indicate what I'm doing wrong.

from rf24.

TMRh20 avatar TMRh20 commented on June 21, 2024

Yeah, here it even says "The presence of an event can be tested for by checking that the req_fd is readable using poll() or an equivalent." which suggests we should be able to poll the req_fd...

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

Removing IrqPinCache::~IrqPinCache() prevents releasing the pin when attachInterrupt() exits, The IrqChipCache::~IrqChipCache() will adequately release the pin and cancel the thread. But, I'm still getting an infinite hang during the example's TX role. This also includes the code to check for pollObj.revents & POLLNVAL, and gpioinfo shows pin 24 is consumed by "RF24 IRQ". So, I think I might have solved the invalid FD problem and "graduated" to a new problem.

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

Hmm, changing the detected event mode from INT_EDGE_FALLING to INT_EDGE_BOTH will trigger the ISR callback; I guess its back the the linux kernel docs again. I've been wanting to put an LED on the radio's IRQ pin as well.

from rf24.

TMRh20 avatar TMRh20 commented on June 21, 2024

When I run the example and look at gpioinfo I get the following, having chosen pin18 as the IRQ pin:
line 18: "PIN12" "RF24 IRQ" input active-high [used]

When I believe it should be line 24: "PIN18"

Maybe I just have this backwards...

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

For FALLING edges, I added some extra flags:

        case INT_EDGE_FALLING:
            request.config.flags |= mode | GPIO_V2_LINE_FLAG_ACTIVE_LOW | GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN;
            break;

which shows as

line  24:     "GPIO24"   "RF24 IRQ"   input   active-low [used pull-down]

This doesn't help and has no difference if I set the bias to pull-up instead of pull-down.


Looking at the pinout from official RPi docs:
https://www.raspberrypi.com/documentation/computers/images/GPIO-Pinout-Diagram-2.png

I think offset 24 (GPIO24) should correspond to header-pin-number 18. I think you have to use the GPIO number in code, not the header pin number.

I was also considering a change in pin number in the example's code to suite those breakout boards you made (that's what I'm currently using 😉), which uses GPIO24 attached to the radio's IRQ.

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

It works 🎉

I had to remove the extra flags for the FALLING edges. It seems using the pin's default config (about bias and active-high) works better for edge detection... Its odd but it works.

FYI: I still have debouncing set to 10 us.

from rf24.

TMRh20 avatar TMRh20 commented on June 21, 2024

Wow, a few simple changes! Nice!

Kind of hate to say it, it runs through the motions on my RPi4, but on the RPi5 I'm getting a segfault after the second successful transmission. If I disconnect the RX device it keeps working. Not sure if a problem with the example or the IRQ code though.

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

I'm seeing an infinite hang after the first payload is sent. I'm wondering how to clear the edge event from the kernel buffer (or if I even have to)... I have the kernel using REALTIME clock for timestamps in the event buffer.

            if (pollObj.revents & POLLIN) {
                // clear the interrupt event in the kernel buffer
                memset(&irqEventInfo, 0, sizeof(irqEventInfo));
                int ret = read(pinCache->fd, &irqEventInfo, sizeof(irqEventInfo));
                if (ret < 0) {
                    std::string msg = "[attachInterrupt] Could not read event info; ";
                    msg += strerror(errno);
                    throw GPIOException(msg);
                    return NULL;
                }
                if (irqEventInfo.timestamp_ns != 0) {
                    pinCache->function();
                }
            }

According to the poll() docs, the revents attr is cleared before actually polling the fd. But its worded in a somewhat confusing way.

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

I changed the example code in 995bca5. Now I'm getting

*** PRESS 'T' to begin transmitting to the other node
*** PRESS 'R' to begin receiving from the other node
*** PRESS 'Q' to exit
t

Configuring IRQ pin to ignore the 'data sent' event
   Pinging RX node for 'data ready' event...    IRQ pin is actively LOW
        data_sent: 1, data_fail: 0, data_ready: 1
   'Data Ready' event test passed

Configuring IRQ pin to ignore the 'data ready' event
   Pinging RX node for 'data sent' event...

and then infinite hang. On the RX side it times out and prints

*** 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: Ping

😞 We're so close.

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

I'm overthinking again

I'm wondering how to clear the edge event from the kernel buffer (or if I even have to)...

We don't have to clear the kernel's buffer, at least not in the case of interruptConfigure example. According to the Linux kernel docs, the buffer is popped when a new edge event is pushed:

The buffer may overflow if bursts of events occur quicker than they are read by userspace. If an overflow occurs then the oldest buffered event is discarded. Overflow can be detected from userspace by monitoring the event sequence numbers.

When I remove the read() from kernel buffer, then the example completes the TX role. Although the flags returned by whatHappened() are not as expected:

Configuring IRQ pin to ignore the 'data sent' event
   Pinging RX node for 'data ready' event...    IRQ pin is actively LOW
        data_sent: 1, data_fail: 0, data_ready: 1
   'Data Ready' event test passed

Configuring IRQ pin to ignore the 'data ready' event
   Pinging RX node for 'data sent' event...     IRQ pin is actively LOW
        data_sent: 0, data_fail: 0, data_ready: 0
   'Data Sent' event test failed

Sending 1 payload to fill RX node's FIFO. IRQ pin is neglected.
RX node's FIFO is full; it is not listening any more

Configuring IRQ pin to reflect all events
   Pinging inactive RX node for 'data fail' event...    IRQ pin is actively LOW
        data_sent: 0, data_fail: 0, data_ready: 0
   'Data Fail' event test failed
Complete RX FIFO: Yak Back ACK

from rf24.

TMRh20 avatar TMRh20 commented on June 21, 2024

I changed the poll_irq function to the following and it almost works, just doesn't trigger for the data sent event, but triggers for data ready, data fail as well on reception...

    IrqPinCache* pinCache = (IrqPinCache*)(arg);
    
   
    while(1){
      uint8_t buf[sizeof(gpio_v2_line_event)];
      int test = read(pinCache->fd, buf, sizeof(gpio_v2_line_event));
      if(test > 0){
          printf("Success\n");
          pinCache->function();
      }
    }

I also modified the example to timeout after waiting 500ms for an IRQ.
With polling it always ends up returning true and triggering the interrupt in an endless loop.

Very close I think to having it working!

from rf24.

TMRh20 avatar TMRh20 commented on June 21, 2024

Hmm, commented out the 4 debounce config lines, and it seems to trigger on all modes!

Configuring IRQ pin to ignore the 'data sent' event
   Pinging RX node for 'data ready' event...Success
        IRQ pin is actively LOW
        data_sent: 1, data_fail: 0, data_ready: 1
   'Data Ready' event test passed

Configuring IRQ pin to ignore the 'data ready' event
   Pinging RX node for 'data sent' event...Success
        IRQ pin is actively LOW
        data_sent: 1, data_fail: 0, data_ready: 1
   'Data Sent' event test passed

Sending 1 payload to fill RX node's FIFO. IRQ pin is neglected.
Transmission failed or timed out. Continuing anyway.

Configuring IRQ pin to reflect all events
   Pinging inactive RX node for 'data fail' event...Success
        IRQ pin is actively LOW
        data_sent: 1, data_fail: 0, data_ready: 1
   'Data Fail' event test failed
Complete RX FIFO: Yak Back ACK

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

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.

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

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().

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

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. 🤣

from rf24.

TMRh20 avatar TMRh20 commented on June 21, 2024

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!

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

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.

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

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

Can you push these changes? I like this idea.

from rf24.

TMRh20 avatar TMRh20 commented on June 21, 2024

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

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

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. 😆

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

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.

from rf24.

2bndy5 avatar 2bndy5 commented on June 21, 2024

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).

from rf24.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.