Add isr example with the pio #259
Conversation
pio/isr/main.c
Outdated
| void simply_isr_handler() { | ||
| // Read the IRQ register to get the IRQ flags from the state machine | ||
| // This tells me which state machine sent the IRQ | ||
| irq_flags = read_register(PIO0_BASE, PIO_IRQ_OFFSET); |
There was a problem hiding this comment.
Looks like you can also get this from https://github.com/raspberrypi/pico-sdk/blob/master/src/rp2040/hardware_structs/include/hardware/structs/pio.h#L127-L130 which might be "less lowlevel" than defining your own read_register and write_register functions? 🤷
Or there's also https://github.com/raspberrypi/pico-sdk/blob/master/src/rp2_common/hardware_pio/include/hardware/pio.h#L799-L822 which is probably even better?
There was a problem hiding this comment.
Oh I didn't notice the pio_interrupt_get. I think that will work better for this example.
There was a problem hiding this comment.
Updated to use pio_interrupt_get.
I think personally I would prefer the struct version to read the IRQ, just because it's 1 read vs. 4 separate reads for pio_interrupt_get. But as an sdk example I think this is good.
|
Nice, thanks for addressing my comments 👍 |
done |
README.md
Outdated
| @@ -156,6 +156,7 @@ App|Description | |||
| [uart_tx](pio/uart_tx)| Implement the transmit component of a UART serial port, and print hello world. | |||
| [ws2812](pio/ws2812)| Examples of driving WS2812 addressable RGB LEDs. | |||
| [addition](pio/addition)| Add two integers together using PIO. Only around 8 billion times slower than Cortex-M0+. | |||
| [pio_isr](pio/isr)| Respond to interrupts generated by a PIO state machine. | |||
There was a problem hiding this comment.
For consistency with the other examples listed here, should this be [isr](pio/isr) ? Sorry for being picky!
There was a problem hiding this comment.
No problem, I understand! I used pio_isr because it's the name of the program that gets compiled, but I can change it to isr if you'd like.
pio/isr/main.c
Outdated
| * | ||
| * Of course, you can really do whatever you want in the ISR, it's up to you. | ||
| */ | ||
| volatile uint32_t irq_flags = 0; |
There was a problem hiding this comment.
Probably makes no difference, but I wonder if a uint8_t would be more appropriate here, given that there are only 8 PIO SMs in total?
There was a problem hiding this comment.
u8 would be more efficient here. I had a u32 because originally I had that register read function which returned a u32 even though I only cared about the lower 8 bits.
|
@lurch I made your requested review changes, and also a couple more. I renamed it to IRQ since that's a more standard name. I'm used to calling them ISR (interrupt service routine) which gets confusing because in pico world, ISR means Input Shift Register. So I renamed everything to IRQ. Also I updated my IRQ handler to use the lower level read/clear method. In this case I think it really does make it simpler for this use case than |
| * This will make the simple_irq program send an ISR to us! | ||
| */ | ||
| void trigger_irq(int sm) { | ||
| printf("Triggering ISR from state machine %d\n", sm); |
There was a problem hiding this comment.
ISR -> IRQ here? There's also a few comments that still say "ISR", where perhaps "IRQ" would be more appropriate? (although there are some comments where it might be appropriate to stick to "ISR"?)
In my understanding (which may still be wrong, as I've not done much low-level coding), IRQ = Interrupt ReQuest, i.e. the actual hardware signal, and ISR = Interrupt Service Routine i.e. the callback function you've registered to handle the IRQ.
| printf("Expected IRQ flags: 0x%08X\n", (1 << sm)); | ||
| printf("Actual IRQ Flags: 0x%08X\n", irq_flags); | ||
|
|
||
| // Here you could dispatch work for the irq depending one |
There was a problem hiding this comment.
"depending one" ? Looks like you might have missed some words out?
| } | ||
|
|
||
| // clear irq flags now. | ||
| irq_flags = 0; |
There was a problem hiding this comment.
Rather than setting all irq_flags to 0 here at the end, I wonder if it might be more idiomatic to clear the flags individually e.g. by doing something like irq_flags &= !PIO_SM_1_IRQ inside the if (irq_flags & PIO_SM_1_IRQ) { block? Or perhaps that's unnecessary complication? 🤷
|
|
||
| % c-sdk { | ||
| /** | ||
| * Initializer for the fake irq program program |
| // ISR would execute only once when simultaneous interrupts happen instead | ||
| // of being executed for each interrupt individually. | ||
| // For other applications, make sure to check the correct pio (pio0/pio1) | ||
| irq_flags = pio0_hw->irq; |
There was a problem hiding this comment.
Perhaps some compilers will need an explicit cast here, to avoid warnings? 🤷 (alternatively, I guess you could make irq_flags a uint32_t again)
| */ | ||
| void enable_pio_irqs() { | ||
| // Set the function that will be called when the PIO IRQ comes in. | ||
| irq_set_exclusive_handler(PIO0_IRQ_0, simple_irq_handler); |
There was a problem hiding this comment.
I think this should be:
irq_add_shared_handler(PIO0_IRQ_0, simple_irq_handler, PICO_SHARED_IRQ_HANDLER_DEFAULT_ORDER_PRIORITY);After all, each state machine may have its own handler attached, as they could each run entirely distinct programs.
Someone on the pico forum asked how interrupts work with only 2 interrupt lines and 4 state machines.
I put together this example, which only runs with one isr line, but shows how you could identify which state machine the irq is coming from.
The pio program simply stalls waiting for input to the tx fifo from firmware, and then fires an interrupt when it gets anything.
From main.c, I load the program into each pio and fire the interrupts one by one. The code shows how you can identify the state machine the interrupt came from and then dispatch some action.