Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pigpio Testing #819

Merged
merged 42 commits into from
Mar 12, 2022
Merged

Pigpio Testing #819

merged 42 commits into from
Mar 12, 2022

Conversation

TMRh20
Copy link
Member

@TMRh20 TMRh20 commented Jan 3, 2022

Initial working example of using pigpio for interrupts instead of wiringpi. While this adds a new driver called "pigpio", these changes affect the SPIDEV & RPi drivers since they have historically relied on wiringPi API that's usually present on all fresh Raspberry Pi images (which recently changed with the latest Bullseye release - see #818). To be clear, these changes don't affect Arduino/PlatformIO builds.

If pigpio is not installed (it should be on fresh Raspberry Pi OS images), then interrupt support is effectively disabled using the newly introduced RF24_NO_INTERRUPT macro definition.

The python wrappers are hard coded to use this new macro because interrupt support can be achieved via external python libraries like RPi.GPIO (& including the pigpio python binding). The examples_linux/interrupt_configure.py example still demonstrates using RPi.GPIO.

Due to using pigpio as an external dependency, we have tried our best to intuitively disable interrupt support when installing the library, but users compiling their own applications may need to now use pass -DRF24_NO_INTERRUPT option to the compiler in order to avoid an error from the specified driver's includes.h which looks like "file not found or does not exist" when looking to #include "interrupts.h ".

@TMRh20 TMRh20 linked an issue Jan 3, 2022 that may be closed by this pull request
configure Outdated Show resolved Hide resolved
@2bndy5
Copy link
Member

2bndy5 commented Jan 9, 2022

Ok I need to test it out, but I have CMake and the configure script linking to pigpio if the lib is found for SPIDEV and RPi drivers. The configure script will output a msg when pigpio is being linked to.

CI run reveals problems with using CMake to build the examples. I'll have to look into that later (going on a grocery run).

@TMRh20
Copy link
Member Author

TMRh20 commented Jan 19, 2022

"Brittle"
Lol, yeah it's not the best, but I haven't figured out at how else to manage interrupts. The thing that gets me is it works with SPIDEV and BCM drivers, using pigpio for interrupts, just not pigpio as the main driver.
All the other examples I tested work fine, so I'll assume its something with my code I guess.

@2bndy5
Copy link
Member

2bndy5 commented Jan 19, 2022

Interesting observation. I suspect it might have something to do with pigpio using a daemon running in the background as a system service, but I don't know if that would explain the better experience with SPIDEV or RPi drivers.

I don't know how to write multi-threaded applications (let alone a library) in C++ as I've only ever attempted that in python (and its not concurrent multi-threading due to CPython's GIL mechanism). I see an intriguing opportunity to learn in this regard...

@TMRh20
Copy link
Member Author

TMRh20 commented Jan 22, 2022

@2bndy5 I'm trying to update the installer script (https://github.com/TMRh20/tmrh20.github.io/blob/master/RF24Installer/RPi/install.sh) to use CMAKE, but I'm having troubles compiling the examples with the pigpio branch. If I use the master branch it works fine, but with the pigpio branch, it is failing with linker errors with RF24Network, RF24Mesh and RF24Gateway:

/usr/bin/ld: /usr/local/lib/librf24.so: undefined reference to `spiXfer'
/usr/bin/ld: /usr/local/lib/librf24.so: undefined reference to `gpioSetISRFunc'
/usr/bin/ld: /usr/local/lib/librf24.so: undefined reference to `gpioRead'
/usr/bin/ld: /usr/local/lib/librf24.so: undefined reference to `gpioSetMode'

It fails on all drivers , RPi, SPIDEV and PiGPIO, but with slightly different errors. I'm not that familiar with the CMAKE build system so thought I'd ask you...

@2bndy5
Copy link
Member

2bndy5 commented Jan 22, 2022

When compiling the examples are you also using CMake? When compiling the examples with CMake, it is better to specify the driver again (like when building/installing the lib). This is because I cannot rely on the presence of a Makefile.inc like the traditional Makefile does for the Linux examples. I suspect that CMake is not linking the examples to the pigpio driver properly because if the driver isn't specified then it defaults to the auto-detect approach which may resolve to a different driver from what was specified for the lib.

EDIT:

failing with linker errors with RF24Network, RF24Mesh and RF24Gateway:

@TMRh20 Oops, I didn't notice this on my first read through. I haven't modified the CMake solutions for the other RF24* libs in regards to pigpio. I'll start looking into that... The first part of this comment only relates to the RF24 lib's examples.

@TMRh20
Copy link
Member Author

TMRh20 commented Jan 22, 2022

It looks like its doing it with the master branch also. On a RPi3, anything except the bcm2835 driver is having linker issues with the RF24Network, Mesh and Gateway examples. I am calling cmake -D RF24_DRIVER="driverName" when compiling.

Here is the updated script so far:
https://gist.github.com/TMRh20/1c463f2c20bfd324b6bc9cef19ad66c7

@2bndy5
Copy link
Member

2bndy5 commented Jan 22, 2022

I suspect that using #include <pigpio.h> in the interrupt.h files is what causes the undefined references. The examples for subsequent network layers may need to be linked to pigpio if RF24 lib is linked to pigpio. This is because we're using pigpio as a statically linked lib.

  1. the other RF24* libs don't use the -DRF24_driver=*** option when configuring CMake. You probably got a WARNING message from cmake .. about an unused variable.
  2. To link to the pigpio lib without altering the CMake solutions for the other RF24 libs, you might be able to
    CLFAGS='$(CFLAGS) -lpigpio'
    cmake ..
    make
    sudo make install
    
    I have not tested this yet; its just an idea/theory.
  3. The CI workflows for the other RF24* libs use the SPIDEV driver when building (then linking to) the RF24 lib. I think there might be something wrong with your build env(s). Instead of using make clean, it would be advised to delete everything in the build folder that you're executing CMake from.

@2bndy5
Copy link
Member

2bndy5 commented Jan 22, 2022

suggestion in point 2 does not work. I'm still looking into it...

@2bndy5
Copy link
Member

2bndy5 commented Jan 23, 2022

I've been checking with the MakeFile in RF24Network/examples_RPi and it looks like the -lpigpio needs to be added there too. I have been able to locally cross-compile RF24Network examples for armhf with a simple modification to the CMakeLists.txt file in RF24Network/examples_RPi... Committing soon to new pigpio branch on RF24Network.

FYI, I'm testing locally with pigpio cross-compiled and installed and setting the -DRF24_DRIVER=SPIDEV. I love using WSL Ubuntu on Windows!

(env) brendan@B-DESKTOP:/mnt/c/Users/ytreh/Documents/GitHub/RF24Network/build$ cmake ../examples_RPi/ -DCMAKE_INSTALL_PREFIX=/usr/arm-linux-gnueabihf -DCMAKE_TOOLCHAIN_FILE=../cmake/toolchains/armhf.cmake

UPDATE: Turns out the Makefile for RF24Network uses actual makefile syntax (not shell script syntax). I have never been able to adequately use Makefile syntax because it is much stricter than shell script syntax (and it doesn't resemble anything I've ever used in the past - at least CMake resembles Lua which I can work with). I'll commit my attempt to link pigpio in RF24Network/examples_RPi/Makefile but with no guarantees that it won't break.

@2bndy5
Copy link
Member

2bndy5 commented Jan 23, 2022

Its official. I hate Makefile syntax (docs are very old) and nothing visually makes sense about nested if conditions.

@TMRh20 It looks like the Makefile in RF24Network/examples_RPi is not properly detecting the BCM chip for my RPi4, so, I'll undo my changes to that file and leave the required modifications to someone that knows Makefile syntax. Currently this results in

(env) pi@rpi4:~/github/RF24Network/examples_RPi $ make
g++  -Wall -I../ rx-test.cpp -lrf24-bcm -lrf24network -o rx-test
/usr/bin/ld: /usr/local/lib/librf24-bcm.so: undefined reference to `gpioSetISRFunc'
/usr/bin/ld: /usr/local/lib/librf24-bcm.so: undefined reference to `gpioInitialise'
collect2: error: ld returned 1 exit status
make: *** [Makefile:45: rx-test] Error 1

despite my attempts to link to pigpio. HINT: -lpigpio needs to be thew last lib that is linked when passing args to gcc.

However, the CMake modifications are working to build the RF24Network examples (only tested with RF24 built using SPIDEV driver and pigpio for IRQ only - not tested with pigpio as primary driver)

@TMRh20 TMRh20 marked this pull request as ready for review January 29, 2022 15:54
@2bndy5
Copy link
Member

2bndy5 commented Jan 30, 2022

I'm reviewing the cmake install instructions, and I'd like to mention here that the Linux CI builds installable packages for SPIDEV & RPi drivers (built for armhf and aarch64) that now also link to piigpio. Since these installable packages are uploaded to every release's assets when a new version is published, I think it would be prudent to make sure these pkgs do not link to pigpio (as a dependency).

I'm intentionally not uploading installable packages for MRAA, wiringPi, & pigpio drivers to releases' assets because I'm not sure if dpkg or rpm is aware of the required statically linked deps.

@2bndy5
Copy link
Member

2bndy5 commented Feb 4, 2022

I updated the PR description so we can gleam from it for the next release crusade.

I'd be a little more confortable if #include "interrupts.h" wasn't part of the includes.h file that gets installed (weather or not pigpio is available). It might be better to have interrupt.h not #included unless the compiler option specify otherwise (like the opposite of -DRF24_NO_INTERRUPT).

As it is currently designed, I expect users to have problems if pigpio is not installed when they go to compile their existing projects using these changes because they may not be aware of the new RF24_NO_INTERRUPT macro. @TMRh20 Can you think of (or even be ok with) a way to abstract the interrupt support in a way that there doesn't need to be 3 copies of interrupt.h in various utility driver folders?

@TMRh20
Copy link
Member Author

TMRh20 commented Feb 5, 2022

Nope.

I'm ok with interrupts being disabled by default. Its kind of an advanced feature, so advanced users can enable it.

@TMRh20
Copy link
Member Author

TMRh20 commented Mar 12, 2022

Is this ready to go? I'm a bit unhappy with the stability, but not sure what to do about it. Almost thinking we should just use pigpio for interrupt support. Either way I guess, I'm ok with it, just wish it were more robust.

@2bndy5
Copy link
Member

2bndy5 commented Mar 12, 2022

I'm ok with this. The Net and Mesh repos have pigpio branches that need merging for this branch to work with those layers.

@TMRh20 TMRh20 merged commit d89c8ae into master Mar 12, 2022
@TMRh20
Copy link
Member Author

TMRh20 commented Mar 12, 2022

OK, merging the repos, I think all that needs to be done is update the install script?

@2bndy5
Copy link
Member

2bndy5 commented Mar 12, 2022

update the install script?

Yes, after merging the CMake changes from the pigpio-support branches for Net and Mesh repos. It would probably be easiest to change the branch names in the install script. This way users see that they are installing the lib from the master branch when they see "Already on 'master' branch" when the script executes.

I'd also like to merge the pyRF24 PR, so the script optionally installs that python wrapper instead of the traditional python wrappers (which are harder to maintain due to dependency on the users to follow instructions).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Interrupts not available on RPi OS/Bullseye (No WiringPi)
2 participants