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

Using interrupts on RPi #173

Merged
merged 5 commits into from Dec 19, 2015

Conversation

Projects
None yet
2 participants
@Oitzu
Collaborator

Oitzu commented Dec 13, 2015

Show outdated Hide outdated utility/RPi/interrupt.h
* fires.
*********************************************************************************
*/
static void *interruptHandler (void *arg);

This comment has been minimized.

@TMRh20

TMRh20 Dec 17, 2015

Member

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 Dec 17, 2015

Member

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.

Show outdated Hide outdated Makefile
@@ -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

This comment has been minimized.

@TMRh20

TMRh20 Dec 17, 2015

Member

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

@TMRh20

TMRh20 Dec 17, 2015

Member

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

This comment has been minimized.

Show comment
Hide comment
@Oitzu

Oitzu Dec 17, 2015

Collaborator

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

Collaborator

Oitzu commented Dec 17, 2015

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

* back to the user supplied function.
*********************************************************************************
*/
extern int attachInterrupt (int pin, int mode, void (*function)(void));

This comment has been minimized.

@TMRh20

TMRh20 Dec 17, 2015

Member
extern void rfNoInterrupts();
extern void rfInterrupts();

plz see next comment

@TMRh20

TMRh20 Dec 17, 2015

Member
extern void rfNoInterrupts();
extern void rfInterrupts();

plz see next comment

return 0 ;
}

This comment has been minimized.

@TMRh20

TMRh20 Dec 17, 2015

Member
void rfNoInterrupts(){
  pthread_mutex_lock (&pinMutex) ;
}

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

@TMRh20

TMRh20 Dec 17, 2015

Member
void rfNoInterrupts(){
  pthread_mutex_lock (&pinMutex) ;
}

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

for (;;)
if (waitForInterrupt (myPin, -1) > 0)
isrFunctions [myPin] () ;

This comment has been minimized.

@TMRh20

TMRh20 Dec 17, 2015

Member
      pthread_mutex_lock (&pinMutex) ;
      isrFunctions [myPin] () ;
      pthread_mutex_unlock (&pinMutex) ;
@TMRh20

TMRh20 Dec 17, 2015

Member
      pthread_mutex_lock (&pinMutex) ;
      isrFunctions [myPin] () ;
      pthread_mutex_unlock (&pinMutex) ;
@TMRh20

This comment has been minimized.

Show comment
Hide comment
@TMRh20

TMRh20 Dec 17, 2015

Member

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

Member

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

This comment has been minimized.

Show comment
Hide comment
@Oitzu

Oitzu Dec 18, 2015

Collaborator

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.

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 referenced this pull request in nRF24/RF24Gateway Dec 18, 2015

Add interrupt code
Testing support for interrupts #173
@TMRh20

This comment has been minimized.

Show comment
Hide comment
@TMRh20

TMRh20 Dec 18, 2015

Member

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.

Member

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.

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

This comment has been minimized.

@TMRh20

TMRh20 Dec 18, 2015

Member

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.

@TMRh20

TMRh20 Dec 18, 2015

Member

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

This comment has been minimized.

Show comment
Hide comment
@Oitzu

Oitzu Dec 18, 2015

Collaborator

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.

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

This comment has been minimized.

Show comment
Hide comment
@TMRh20

TMRh20 Dec 18, 2015

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@Oitzu

Oitzu Dec 18, 2015

Collaborator

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

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

This comment has been minimized.

Show comment
Hide comment
@TMRh20

TMRh20 Dec 18, 2015

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@Oitzu

Oitzu Dec 18, 2015

Collaborator

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

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 added a commit that referenced this pull request Dec 19, 2015

Merge pull request #173 from Oitzu/master
Using interrupts on RPi

@TMRh20 TMRh20 merged commit a381194 into nRF24:master Dec 19, 2015

TMRh20 added a commit that referenced this pull request Dec 19, 2015

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
@TMRh20

This comment has been minimized.

Show comment
Hide comment
@TMRh20

TMRh20 Dec 19, 2015

Member

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.

Member

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 added a commit that referenced this pull request Dec 19, 2015

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
@TMRh20

This comment has been minimized.

Show comment
Hide comment
@TMRh20

TMRh20 Dec 19, 2015

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@Oitzu

Oitzu Dec 20, 2015

Collaborator

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.

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

This comment has been minimized.

Show comment
Hide comment
@TMRh20

TMRh20 Dec 20, 2015

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@Oitzu

Oitzu Dec 20, 2015

Collaborator

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!

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 added a commit that referenced this pull request Dec 20, 2015

@TMRh20

This comment has been minimized.

Show comment
Hide comment
@TMRh20

TMRh20 Dec 20, 2015

Member

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

Member

TMRh20 commented Dec 20, 2015

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

@TMRh20

This comment has been minimized.

Show comment
Hide comment
@TMRh20

TMRh20 Dec 21, 2015

Member

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.

Member

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 added a commit that referenced this pull request Dec 21, 2015

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

TMRh20 referenced this pull request Dec 21, 2015

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
-
@TMRh20

This comment has been minimized.

Show comment
Hide comment
@TMRh20

TMRh20 Dec 21, 2015

Member

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

Member

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

This comment has been minimized.

Show comment
Hide comment
@Oitzu

Oitzu Dec 21, 2015

Collaborator

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

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