Using interrupts on RPi #173

Merged
merged 5 commits into from Dec 19, 2015

Projects

None yet

2 participants

@Oitzu
Collaborator
Oitzu commented Dec 13, 2015
@TMRh20 TMRh20 commented on an outdated diff Dec 17, 2015
utility/RPi/interrupt.h
+
+#include "RF24_arch_config.h"
+
+#define INT_EDGE_SETUP 0
+#define INT_EDGE_FALLING 1
+#define INT_EDGE_RISING 2
+#define INT_EDGE_BOTH 3
+
+/*
+ * interruptHandler:
+ * This is a thread and gets started to wait for the interrupt we're
+ * hoping to catch. It will call the user-function when the interrupt
+ * fires.
+ *********************************************************************************
+ */
+static void *interruptHandler (void *arg);
@TMRh20
TMRh20 Dec 17, 2015 Owner

One thing I noticed was a warning when compiling: warning: ‘void* interruptHandler(void*)’ declared ‘static’ but never defined [-Wunused-function] which I was able to get rid of by changing the function in the interrupt.h and .c files. from static void to just void which still seems to work ok.

@TMRh20 TMRh20 commented on an outdated diff Dec 17, 2015
@@ -53,7 +53,8 @@ OBJECTS+=gpio.o compatibility.o
else ifeq "$(RPI)" "1"
DRIVER_DIR=$(ARCH_DIR)/RPi
-OBJECTS+=bcm2835.o
+OBJECTS+=bcm2835.o
+OBJECTS+=interrupt.o
# The recommended compiler flags for the Raspberry Pi
@TMRh20
TMRh20 Dec 17, 2015 Owner

Adding SHARED_LINKER_FLAGS+=-pthread removes the need to include -pthread in the makefiles for examples etc. in this and additional libraries.

@TMRh20 TMRh20 added the enhancement label Dec 17, 2015
@Oitzu
Collaborator
Oitzu commented Dec 17, 2015

Changed and tested your comments. Thanks for the feedback! 👍

@TMRh20 TMRh20 commented on the diff Dec 17, 2015
utility/RPi/interrupt.h
+
+/*
+ * piHiPri:
+ * Attempt to set a high priority schedulling for the running program
+ *********************************************************************************
+ */
+extern int piHiPri (const int pri);
+
+/*
+ * attachInterrupt (Original: wiringPiISR):
+ * Pi Specific.
+ * Take the details and create an interrupt handler that will do a call-
+ * back to the user supplied function.
+ *********************************************************************************
+ */
+extern int attachInterrupt (int pin, int mode, void (*function)(void));
@TMRh20
TMRh20 Dec 17, 2015 Owner
extern void rfNoInterrupts();
extern void rfInterrupts();

plz see next comment

@TMRh20 TMRh20 commented on the diff Dec 17, 2015
utility/RPi/interrupt.c
+ ioctl (sysFds [bcmGpioPin], FIONREAD, &count) ;
+ for (i = 0 ; i < count ; ++i)
+ read (sysFds [bcmGpioPin], &c, 1) ;
+
+ isrFunctions [pin] = function ;
+
+ pthread_mutex_lock (&pinMutex) ;
+ pinPass = pin ;
+ pthread_create (&threadId, NULL, interruptHandler, NULL) ;
+ while (pinPass != -1)
+ delay (1) ;
+ pthread_mutex_unlock (&pinMutex) ;
+
+ return 0 ;
+}
+
@TMRh20
TMRh20 Dec 17, 2015 Owner
void rfNoInterrupts(){
  pthread_mutex_lock (&pinMutex) ;
}

void rfInterrupts(){
  pthread_mutex_unlock (&pinMutex) ;
}

@TMRh20 TMRh20 commented on the diff Dec 17, 2015
utility/RPi/interrupt.c
+ return sched_setscheduler (0, SCHED_RR, &sched) ;
+}
+
+
+void *interruptHandler (void *arg)
+{
+ int myPin ;
+
+ (void)piHiPri (55) ; // Only effective if we run as root
+
+ myPin = pinPass ;
+ pinPass = -1 ;
+
+ for (;;)
+ if (waitForInterrupt (myPin, -1) > 0)
+ isrFunctions [myPin] () ;
@TMRh20
TMRh20 Dec 17, 2015 Owner
      pthread_mutex_lock (&pinMutex) ;
      isrFunctions [myPin] () ;
      pthread_mutex_unlock (&pinMutex) ;
@TMRh20
Owner
TMRh20 commented Dec 17, 2015

@Oitzu Np, and thanks for all of this so far!

I was just playing with interrupts little more, and was testing functions to en/disable interrupts also (similar to Arduino functions ) In testing it seems it was easiest to just lock/unlock in the interrupt function itself, which allows the following.

1.Call rfNoInterrupts();
2. Write
3. Check available() manually to ensure no missed payloads (if required)
4. CallrfInterrupts() to re-enable the interrupt
5. No conflicts??

This should prevent any conflicts on the SPI BUS and seems to be working good for me with with various RF24 libs.

I'm not sure what your thoughts are here, but I can always accept the current pull and merge this stuff in if OK like this, otherwise feel free to modify and/or make suggestions.

@Oitzu
Collaborator
Oitzu commented Dec 18, 2015

First time hearing about conflicts, of which nature are this conflicts? Any example?

About "detaching" interrupts:
It would be nice to also have an method to detach / turning of an interrupt again.
What do you think to simply go the same way as attaching interrupts?
That would be calling the gpio tool "gpio edge none" to deactivate detection on gpio tool side and calling pthread_cancel(<thread_id>) to cancel the thread.

@TMRh20 TMRh20 referenced this pull request in TMRh20/RF24Gateway Dec 18, 2015
@TMRh20 Add interrupt code
Testing support for interrupts #173
50a037d
@TMRh20
Owner
TMRh20 commented Dec 18, 2015

Well I don't really know what I'm talking about, I'm just referencing documentation and testing so...

It seems like both options (detachInterrupt() and lock/pause interrupts) may be useful. The advantage of the mutex lock is that interrupts are not actually disabled, as per here

A mutex has two possible states: unlocked (not owned by any thread),
and locked (owned by one thread). A mutex can never be owned by two
different threads simultaneously. A thread attempting to lock a mutex
that is already locked by another thread is suspended until the owning
thread unlocks the mutex first.

This basically seems to provide a nice system of queuing for interrupts, while automatically making queues, etc thread-safe, from what I understand of the documentation.

I referenced my latest commit to RF24Gateway above, since I was experiencing SPI conflicts, until adding the mutex lock/unlock functions. edit Just for clarification, I was debugging with GDB and it was hanging on BCM SPI functions.

It seems that, since the reading is done in a separate thread, writing to SPI can cause problems if it happens at the same time a read is incoming. This issue was only apparent via testing with RF24Gateway and RF24Mesh, and only with some active traffic.

@TMRh20 TMRh20 commented on the diff Dec 18, 2015
examples_RPi/transfer_interrupt.cpp
+
+ bool role_ping_out = 1, role_pong_back = 0;
+ bool role = 0;
+
+ // Print preamble:
+
+ cout << "RF24/examples/Transfer/\n";
+
+ radio.begin(); // Setup and configure rf radio
+ radio.setChannel(1);
+ radio.setPALevel(RF24_PA_MAX);
+ radio.setDataRate(RF24_1MBPS);
+ radio.setAutoAck(1); // Ensure autoACK is enabled
+ radio.setRetries(2,15); // Optionally, increase the delay between retries & # of retries
+ radio.setCRCLength(RF24_CRC_8); // Use 8-bit CRC for performance
+ radio.printDetails();
@TMRh20
TMRh20 Dec 18, 2015 Owner

I just realized also that adding radio.maskIRQ(1,1,0); in examples that only use the interrupt for reading will prevent the radio from triggering and interrupt on TX_FAIL and TX_OK. Going to test this more with RF24Gateway and the mutex locking etc.

@Oitzu
Collaborator
Oitzu commented Dec 18, 2015

Hm... i don't fully understand yet, how you want to use the mutex to queue interrupts.

The SPI problem makes sense to me, altough i thought the higher priority of the thread would prevent the main thread to write while the child is reading.

It makes to me more sense to lay a mutex on the spi to prevent simultan access to the bus from multiple threads.

@TMRh20
Owner
TMRh20 commented Dec 18, 2015

It makes to me more sense to lay a mutex on the spi to prevent simultan access to the bus from multiple threads.

That might work fine?

In more complex examples like RF24Network or RF24Gateway, in the update() function data is read into a buffer and written to the radio for routing, acks etc.

If a user decides to read from that buffer in a separate thread, it would be 'unprotected' (I think?... mostly testing with queues in RF24GW). This also makes the locking interface public, so users can use the same mutex for queues/data management.

I'm thinking if its done at the SPI layer, users or additional libs can use their own lock for queues etc. so it probably would work out about the same, just a bit more code to integrate separate locking?

Traditionally, on Arduino, interrupts are handled with separate functions, but it seems some form of locking can allow more fluid handling of radio functions using standard code with small modifications.

@Oitzu
Collaborator
Oitzu commented Dec 18, 2015

I seriously lost you at this point. 😥
In my understanding mutex are mainly there to protect data or ressources from simultan (in this case by threads) access and therefore conflicts or inconsistencies.

I'm at this point no longer sure what you want to achieve. ^^
Maybe its the best to let you do your thing. :D

@Avamander Avamander changed the title from #172 Using interrupts on RPi to Using interrupts on RPi Dec 18, 2015
@TMRh20
Owner
TMRh20 commented Dec 18, 2015

In short, I am trying to achieve prevention of apparent conflicts.

Example 1:
a: Radio receives data in interrupt function
b: Data is buffered to a queue or array
c: In a separate thread, data is read from the incoming queue

Problem/Conflicts:
a: Overlapping SPI writes/reads
b: Overlapping changes to data

Solutions:

  1. Add locking/unlocking to SPI - Addresses problem A
  2. Add locking/unlocking to SPI & interrupt handler - Addresses problem A & B

In attempting to implement locking/unlocking at the SPI layer it seems to work, but I have additional problems managing data with separate locks.

Thread safe queues typically need to be implemented to pass data between threads. I was hoping to remove the need to implement anything to make queues thread-safe.

If you are still unsure, I can accept this pull request, as-is and mess around with a side fork, until I get this figured out better.

@Oitzu
Collaborator
Oitzu commented Dec 18, 2015

Ah, yes now i understand better.
Multiple locks in this overlapping field also raises a risk to deadlock.

Need to think a little bit about it. But if you find a good solution go ahead, accept it and merge. ^^

@TMRh20 TMRh20 merged commit a381194 into TMRh20:master Dec 19, 2015
@TMRh20 TMRh20 added a commit that referenced this pull request Dec 19, 2015
@TMRh20 Add mutex locking to SPI functions for RPi
per #172 #173

- Add mutex locking to SPI for RPi interrupt usage
- Leave rfInterrupts() rfNoInterrupts() functions for testing purposes
715edc5
@TMRh20
Owner
TMRh20 commented Dec 19, 2015

So after taking a break, I decided to put locking into SPI functions, remove all locking from higher libs and recompiled everything.

Still refuses to work properly in GDB, but without debugging it works fine??

I think I may have proven myself wrong in regards to the need for thread-safe queues, because I can't seem to make it crash or anything. I've left the additional rfInterrupts() rfNoInterrupts() functions for testing, but for now I thought it easiest to have the main interrupt functionality in the master branch.

@TMRh20 TMRh20 added a commit that referenced this pull request Dec 19, 2015
@TMRh20 Mutex handling for interrupts on RPi
- Create separate mutex for SPI vs User data handling
Problems:
A: Memory/Data conflicts became apparent after time without thread-safe
queues
B: Without implementing locking at the SPI level, even simple code
examples (rx_transfer.cpp) would hang if a write was done out of sync
with reads

Solutions:
A: Keep initial locking mechanism for users
B: Implement separate automatic mutex and locking mechanism for SPI

#172 #173
243e25f
@TMRh20
Owner
TMRh20 commented Dec 19, 2015

banging head on desk

Uhh, I think I finally got the best of both worlds happening here, with automatic thread locking at the SPI level, and leaving the rfInterrupt() rfNoInterrupt() functions available for users that want a simple way to protect data/queues.

Per the last commit, this automatically prevents spi conflicts, demonstrated via the rx_transfer.cpp example, by adding RF24NetworkHeader header(01); network.write(header,0,0); after delay(2)

My initial solution would have required users to manually lock before the network.write().

This all seems to be working, so 🍻 for now.

@Oitzu
Collaborator
Oitzu commented Dec 20, 2015

Great that you got it working! 🍻
Just one comment:
What do you think about implementing it a bit more arduino like?
eg.: Adding beginTransaction and endTransaction to the SPI class in which the mutex would be locked/unlocked.

@TMRh20
Owner
TMRh20 commented Dec 20, 2015

I was kind of thinking along those lines, but not quite that far. Sounds like a good idea! It may make interrupt handling a bit more portable & standardized too?

I just created a new interrupts branch for further dev, which is just a copy of the master, but you should have direct access to it, in case you feel like doing some more coding.

@Oitzu
Collaborator
Oitzu commented Dec 20, 2015

I don't see the spi transaction implementation that strong connected to the interrupt handling but more to multi-threading in general. (It would also affect other application that use multi-threading that are build on top of the rf24 lib)

Thats also the reason i moved the spi mutex handling completly to the spi files.

I pushed a little bit of code to show you how i imagined that.
The code compiles but isn't tested yet!

@Oitzu Oitzu added a commit that referenced this pull request Dec 20, 2015
@Oitzu Oitzu Implement detachInterrupt method. See #173 b615951
@TMRh20
Owner
TMRh20 commented Dec 20, 2015

Roger that & looking good. I'll see about giving it a spin tonight.

@TMRh20
Owner
TMRh20 commented Dec 21, 2015

All testing good so far.

You are probably aware, but one deceiving thing when testing interrupts to receive & disabling them is the radio buffer will fill up, and interrupts will stop happening. A simple solution is to call the interrupt handler manually before re-enabling or just do a read.

I've been messing with a few minor changes to bring the SPI transaction code more in-line with the Arduino API, and I'll push them in a min. for you to look at. I've tested lightly with Arduino and RPi, and will post and update if I find any bugs.

@TMRh20 TMRh20 added a commit that referenced this pull request Dec 21, 2015
@TMRh20 Minor chgs to mirror Arduino SPI transaction API
- Bring beginTransaction inline with Arduino API
- #define SPI_HAS_TRANSACTION in SPI.h
- #define the spi speed on both Arduino and RPi/Linux using
RF24_SPI_SPEED instead of different vars
- Chg beginTransaction to use bitOrder & mode instead of CSN
- Add delay directly to chipSelect function (helps ensure pin is active
before beginning SPI transactions)

#173
f285fde
@TMRh20 TMRh20 referenced this pull request Dec 21, 2015
@TMRh20 Fixes for last commit
- SPI_HAS_TRANSACTION define was not being picked up properly: add
define to RF24_arch_config.h
- Fix beginTransaction call using RF_SPI_SPEED instead of RF24_SPI_SPEED
- Change beginTransaction to use SPISettings object
-
a57fafe
@TMRh20
Owner
TMRh20 commented Dec 21, 2015

A couple "ooops" and I think its looking & working a bit better, and using an SPISettings object like the Arduino API.

@Oitzu
Collaborator
Oitzu commented Dec 21, 2015

Looks good. I will test it a little bit more while working on my project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment