arduino / ArduinoCore-megaavr

Arduino Core for the ATMEGA4809 CPU
103 stars 62 forks source link

attachInterrupt is pretty memory hungry #56

Closed MCUdude closed 4 years ago

MCUdude commented 4 years ago

attachInterrupt is pretty memory hungry for what it is. voidFuncPtr alone occupies 96 bytes of RAM, and it doesn't matter if you use 1 or 41 interrupt pins. I guess this is acceptable for a 48kB device but bear in mind that this core is the base for other mega/tinyAVR targets too. Targets with much less flash and RAM. I'm not quite sure how, but a way of handling function pointers in an array isn't as elegant on a device where all pins can be used as interrupts, compared to e.g the Arduino UNO where only two pins can be used (four bytes occupied by voidFuncPtr).

IMO the ISR itself is also quite bloated. I will try to experiment a little, but as a start; can't we just pass a PORT_t pointer to port_interrupt_handler instead of having to deal with the portToPortStruct array? Should be more efficient. Something like this code; tested with a pushbutton to pin PF6 on my Curiosity Nano. Any thoughts on this @facchinm?

I commented out the check if the function pointer is 0. Not sure why this was added. If you look at how the ISR is handled by the "classic" Arduino AVRs it's completely barebone without any "protection" inside the ISR. This should be the ideal goal. As little code as possible in the ISR.

static void port_interrupt_handler(PORT_t* PORT, uint8_t port_num) {

  /* Copy flags */
  uint8_t int_flags = PORT->INTFLAGS;

  uint8_t bit_pos = PIN0_bp;
  uint8_t bit_mask = PIN0_bm;

  /* Iterate through flags */
  while(bit_pos <= PIN7_bp){

    /* Check if flag raised */
    if(int_flags & bit_mask){

    /* Get interrupt */
    uint8_t interrupt_num = port_num * 8 + bit_pos;

      /* Check if function defined */
      //if(intFunc[interrupt_num] != 0){ // This _is_ an ISR. Keep it as short as possible!

        /* Call function */
        intFunc[interrupt_num]();
      //}
    }
    bit_pos++;
    bit_mask = (bit_mask << 1); 
  }

  /* Clear flags that have been handled */
  PORT->INTFLAGS = int_flags;
}

#define IMPLEMENT_ISR(vect, port, port_num) \
ISR(vect) { \
  port_interrupt_handler(port, port_num);\
} \

IMPLEMENT_ISR(PORTA_PORT_vect, &PORTA, PA)
IMPLEMENT_ISR(PORTB_PORT_vect, &PORTB, PB)
IMPLEMENT_ISR(PORTC_PORT_vect, &PORTC, PC)
IMPLEMENT_ISR(PORTD_PORT_vect, &PORTD, PD)
IMPLEMENT_ISR(PORTE_PORT_vect, &PORTE, PE)
IMPLEMENT_ISR(PORTF_PORT_vect, &PORTF, PF)
MCUdude commented 4 years ago

Well, I've discussed this with some other AVR guys, and they concluded that there isn't much to improve if we want to use this particular way of calling interrupts. And we do want that, so it's a fair tradeoff I guess.