Adding Generic Interrupt function to IRQManager

[delta-G]

I'm proposing this as an alternative to #315 that keeps the dangerous code inside the IRQManager class. This is a fix for #310

@facchinm would something like this work as an alternative?

Here's a test sketch. I know we already have attachInterrupt for pin interrupts, but I already had this set up for testing.

#include "IRQManager.h"

volatile bool interruptFired = false;

GenericIrqCfg_t cfg;

void setup() {

  Serial.begin(115200);
  while (!Serial)
    ;
  Serial.println("\n\n *** " __FILE__ " ***\n\n");

  R_ICU->IRQCR[0] = 0xB0;  // Configure some peripheral for an interrupt

  // Create a generic interrupt configuration and attach
  cfg.irq = FSP_INVALID_VECTOR;
  cfg.ipl = 12;
  cfg.event = ELC_EVENT_ICU_IRQ0;  
  IRQManager::getInstance().addGenericInterrupt(cfg, Myirq0_callback);

  // Enable the interrupt at the peripheral. 
  R_PFS->PORT[1].PIN[5].PmnPFS = (1 << R_PFS_PORT_PIN_PmnPFS_PCR_Pos) | (1 << R_PFS_PORT_PIN_PmnPFS_ISEL_Pos);

}

void loop() {
  Serial.println("Loop Running");
  delay(500);

  if (interruptFired) {
    interruptFired = false;
    Serial.println("Interrupt Fired");
  }
}

void Myirq0_callback() {
  R_ICU->IELSR[cfg.irq] &= ~(R_ICU_IELSR_IR_Msk);
  interruptFired = true;
}

[facchinm]

I like it very much! Also, it looks like it can be integrated with fsp drivers structure by reusing cfg.irq after calling addGenericInterrupt(), so there's no artificial limitation compared to the "dangerous" implementation.
@alrvid @iabdalkader what do you think? Good to merge? 🙂

[iabdalkader]

I'm not sure why we need to set cfg.irq = FSP_INVALID_VECTOR and then check if if(cfg.irq == FSP_INVALID_VECTOR) {. It would make sense if calling addGenericInterrupt with cfg.irq set does something else, like remove the IRQ, but that's not the case.

Also note that every function in that file should probably check if last_interrupt_index is less than BSP_ICU_VECTOR_MAX_ENTRIES (or PROG_IRQ_NUM) as the number of IRQ entries is not infinite.

[delta-G]

The reason for setting to INVALID_VECTOR and then testing for it is to guard against attaching the same interrupt twice. Most of the other implementations in IRQManager do it so I just followed the pattern.

[iabdalkader]

That doesn't really protect against installing a handler twice using a different config. A better approach would be to check if there's a handler already installed, but I understand that you're following the existing code which is fair enough. However, you should check the last_interrupt_index against the max IRQ though, even if the other code missed doing that.

[delta-G]

I was guessing at the purpose. For every other peripheral the code expects you to start with the irq set to that. I was just following the patters that's there for the rest of the file.

I do agree that last_interrupt_index needs to be checked against the max value. I think the best idea would be to get rid of all that repeated code around finding the vector index and put it into a function like getNextIndex that checks against max and returns the next good index.

[delta-G]

I added the check against PROG_IRQ_NUM to make sure last_interrupt_index doesn't get used out of bounds.

I can do a separate PR to fix that for the other peripherals if that would be helpful.

[iabdalkader]

I added the check against PROG_IRQ_NUM to make sure last_interrupt_index doesn't get used out of bounds.

Thank you!

I can do a separate PR to fix that for the other peripherals if that would be helpful.

I think so. All the other functions should check the max index the same way too.