[Developers] Problem with interrupt handlers in Arduino and a proposed solution

[Aaron Williams]

Hi all,

While I may be new to this group and fairly new to Arduino I have over 20 years of experience working in embedded environments with a variety of operating systems and embedded platforms. When I went to work on my own rotary encoder library for an Arduino project I'm working on I came across a glaring limitation in the Arduino interrupt service routine implementation.

In just about every platform I've worked on except MS DOS, it is possible to register a user-defined pointer along with an ISR function pointer. This is extremely useful, especially in cases where there may be one ISR that is shared between multiple devices and it is especially useful in the C++ case.

With the current Arduino implementation with attachInterrupt it requires a different function pointer for every single GPIO that's used in order to tell which GPIO caused the interrupt. While this is nice for the simple case and for beginners, it causes many problems for more complex cases.

A perfect example of the problem is immediately visible with the Encoder library. That library jumps through great hoops in order to support more than one instance of a rotary encoder, requiring a large number of ISR functions, one per GPIO pin used. On top of this, it requires static variables and all sorts of hacks in order to do this. This can all be greatly simplified if some parameters are passed to the ISR.

In my case I happened to be working with the ESP32, but this is not specific to any particular Arduino platform. What I propose to do is to add another function, attachInterrupt2, which would allow a user-defined pointer to be passed to the ISR. This would allow the class this pointer, for example, to be passed to the ISR. This would make having multiple instantiations of a class that uses interrupts simple, without requiring multiple static ISR functions.

In my case, I implemented the function as:

void attachInterrupt2(uint8_t pin, void (*)(int pin, int value, void *data),

                      int mode, const void *data);

Now that I think about it it might make sense to rename pin to interrupt, though pin is the name used in esp32-hal-gpio.h. The value field is the value of the particular GPIO pin, but I think this can be removed from what I am proposing. The most important change is the data field. This makes it possible to make an ISR routine truly generic within a class. A user-defined data structure would be automatically passed to the ISR function or it could even be the class this pointer.

When I rewrote my own Encoder class, I was able to eliminate all of the ugly hacks and kludges that were implemented due to the current limited ISR implementation. The code is a lot cleaner and smaller. The only additional memory usage is that instead of having just an array of function pointers for the ISR handlers there is now an array of function pointers and void pointers.

My RotaryEncoder class looks like this compared to the existing Encoder class:

class RotaryEncoder

{

    struct RotaryEncoderIsrData {

        RotaryEncoder *encoder;

        uint8_t bit;

    };

    int32_t position;

    uint8_t pins[2];

    uint8_t state;

    bool pullup;

    RotaryEncoderIsrData isrData[2];

public:

    RotaryEncoder(uint8_t p1, uint8_t p2, bool pullup = true) 

    { 

        position = 0;

        state = 0;

        pins[0] = p1;

        pins[1] = p2;

        isrData[0].encoder = this;

        isrData[1].encoder = this;

        isrData[0].bit = 0;

        isrData[1].bit = 1;

        this->pullup = pullup;        

    }

    void attach()

    {            

        pinMode(pins[0], pullup ? INPUT_PULLUP : INPUT);

        pinMode(pins[1], pullup ? INPUT_PULLUP : INPUT);

        if (!pullup) {

            digitalWrite(pins[0], HIGH);

            digitalWrite(pins[1], HIGH);

        }

        noInterrupts();

        delayMicroseconds(2000);

        position = 0;

        state = 0;

        if (digitalRead(pins[0]))

            state |= 1;

        if (digitalRead(pins[1]))

            state |= 2;

        attachInterrupt2(digitalPinToInterrupt(pins[0]), isr, CHANGE, &isrData[0]);

        attachInterrupt2(digitalPinToInterrupt(pins[1]), isr, CHANGE, &isrData[1]);

        interrupts();

    }

    int32_t read()

    {

        int32_t pos;

        

        noInterrupts();

        pos = position;

        interrupts();

        return pos;

    }

    void write(int32_t pos)

    {

        noInterrupts();

        position = pos;

        interrupts();

    }

private:

    void update(int bit, int value)

    {

        uint8_t newState;

        static const int8_t change[16] = { 0, 1, -1, 2, -1, 0, -2, 1, 1, -2, 0, -1, 2, -1, 1, 0 };

        if (value)

            newState = state | (1 << bit);

        else

            newState = state & ~(1 << bit);

        position = position + change[state | (newState << 2)];

        state = newState;

    }

    static void isr(int pin, int value, void *data)

    {

        struct RotaryEncoderIsrData *iDat = (struct RotaryEncoderIsrData *)data;

        iDat->encoder->update(iDat->bit, value); 

    }

};

RotaryEncoder knob(27, 12);

While I don't have all the fancy optimization of the other class, it is much cleaner since only a single ISR function is needed to handle any number of class instantiations and GPIO pins. Compare this to Encoder.h. In the Encoder class there is a public array of data passed to the update function and there are up to 59 instantiations of the ISR function!!! This gets really ugly because of the lack of a user-defined pointer passed to the ISR function. Granted, I don't have all of the optimizations of the Encoder library for the AVR but then again with the ESP32 I have a lot more horsepower (and the Encoder library doesn't work on the ESP32 anyway). I'm not complaining about the Encoder library either because it it stuck working within the limited constraints that the current Arduino attachInterrupt function provides.

-Aaron

[Paul Stoffregen]

Have you measured the extra latency this adds?  By latency, I mean the time from the signal transition that triggers the interrupt to the first line of your function actually running?  Even though you are using only ESP32, this needs to be considered on all architectures Arduino supports, if proposed to become part of the official Arduino API.

FWIW, I happen to be the author of that code in Encoder.h which you're calling "ugly"...

--
You received this message because you are subscribed to the Google Groups "Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to developers+...@arduino.cc.

[Aaron Williams]

On Monday, August 28, 2017 at 1:42:57 PM UTC-7, PaulStoffregen wrote:

Have you measured the extra latency this adds?  By latency, I mean the time from the signal transition that triggers the interrupt to the first line of your function actually running?  Even though you are using only ESP32, this needs to be considered on all architectures Arduino supports, if proposed to become part of the official Arduino API.

 

I have not measured the added latency. The latency would include one additional load for the user data pointer in order to pass it as a parameter, so it should be minimal though it depends on the architecture to some extent. On the ESP32 it is already ugly since a single interrupt is used for all GPIOs and a for loop, i.e:

    uint8_t pin=0;

    if(gpio_intr_status_l) {

        do {

            if(gpio_intr_status_l & ((uint32_t)1 << pin)) {

                if(__pinInterruptHandlers2[pin].handler) {

                    __pinInterruptHandlers2[pin].handler(pin, 

                                                         __pinInterruptHandlers2[pin].data);

                } else if(__pinInterruptHandlers[pin]) {

                    __pinInterruptHandlers[pin]();

                }

            }

        } while(++pin<32);

    }

In this case,  my change adds one more load and compare instruction to the GPIO loop and a move instruction to pass the interrupt number (i.e. pin) as a parameter so the added latency should be minimal. Note that I have not looked in detail into the AVR code but I expect the added latency to be minimal as well. This could be simplified further to always use the new method since an interrupt handler that takes no parameters would just ignore the data field and attachInterrupt would just set it to NULL. If this were done then the only added latency would be passing the pin number to the ISR and loading the data from the handler array and passing it as a parameter.

 

FWIW, I happen to be the author of that code in Encoder.h which you're calling "ugly"...

 

By ugly, I mean that all the work involved in order to work around this huge limitation with attachInterrupt and the multiple ISR functions is quite ugly though I do not see any other clean solution within the Arduino framework. With this one change, however, all of these hacks in order to map a GPIO to a bit go away. For example, the code is full of things like "#ifdef CORE_INT33_PIN  ... #endif for 60 pins, plus you had to define 60 ISR functions!!! You might also consider changing your update command to just use a lookup table instead of a jump table. This is how I did it in my code (see below). This is more efficient than the large switch statement. I might add that I am working with the ESP32 and not the AVR and that Encoder does not compile or work with the ESP32.

One other nice thing is that with this change, everything is nicely stored inside the RotaryEncoder class. The public interruptArgs goes away. Similarly, I can keep update() private. It no longer needs to be public because now the ISR has access to the class pointer. The code is much cleaner with this change.

Here is a new version where the ISR is no longer passed the value since this is not always needed. I still pass the pin number since we basically already get that for free:

I also got rid of the data structure being passed and just pass the class pointer itself to the ISR. In order to do this, I now use two ISR functions, one per bit. I also cleaned up the state change table to let the compiler hide the |= (xx << 2) behind the scenes.

class RotaryEncoder

{

    int32_t position;

    uint8_t pins[2];

    uint8_t state;

    bool pullup;

public:

    RotaryEncoder(uint8_t p1, uint8_t p2, bool pullup = true) 

    { 

        position = 0;

        state = 0;

        pins[0] = p1;

        pins[1] = p2;

        this->pullup = pullup;        

    }

    void attach()

    {            

        pinMode(pins[0], pullup ? INPUT_PULLUP : INPUT);

        pinMode(pins[1], pullup ? INPUT_PULLUP : INPUT);

        if (!pullup) {

            digitalWrite(pins[0], HIGH);

            digitalWrite(pins[1], HIGH);

        }

        noInterrupts();

        delayMicroseconds(2000);

        position = 0;

        state = 0;

        if (digitalRead(pins[0]))

            state |= 1;

        if (digitalRead(pins[1]))

            state |= 2;

        attachInterrupt2(digitalPinToInterrupt(pins[0]), isr0, CHANGE, this);

        attachInterrupt2(digitalPinToInterrupt(pins[1]), isr1, CHANGE, this);

        interrupts();

    }

    void detach()

    {

        detachInterrupt(digitalPinToInterrupt(pins[0]));

        detachInterrupt(digitalPinToInterrupt(pins[1]));

    }

    int32_t read()

    {

        int32_t pos;

        

        noInterrupts();

        pos = position;

        interrupts();

        return pos;

    }

    void write(int32_t pos)

    {

        noInterrupts();

        position = pos;

        interrupts();

    }

    ~RotaryEncoder()

    {

        detach();

    }

private:

    void update(int bit, int value)

    {

        uint8_t newState;

        // [old state][new state] = position change amount

        static const int8_t change[4][4] = { 

            { 0, 1, -1, 2}, { -1, 0, -2, 1},

            { 1, -2, 0, -1}, { 2, -1, 1, 0 } 

        };

        if (value)

            newState = state | (1 << bit);

        else

            newState = state & ~(1 << bit);

        position = position + change[state][newState];

        state = newState;

    }

    static void isr0(int pin, void *data)

    {

        class RotaryEncoder *encoder = (class RotaryEncoder *)data;

        encoder->update(0, digitalRead(pin)); 

    }

    static void isr1(int pin, void *data)

    {

        class RotaryEncoder *encoder = (class RotaryEncoder *)data;

        encoder->update(1, digitalRead(pin)); 

    }

};

[Matthijs Kooijman]

Hey Aaron,

there has already been some discussion and a proposed implementation at
github: https://github.com/arduino/Arduino/pull/4519

There's also some notes about different approaches and the overhead they
add.

The github implementation also does not pass the pin number to the
interrupt function, but I would think that if an application needs it,
it could just store the pin number in the `void* data`, without adding
extra overhead and latency for the (I think majority of) applications
that do not need it.

Gr.

Matthijs

[Aaron Williams]

Hi Matthijs,

I was not aware of this. I look forward to using the new implementation. It will significantly enhance the use of interrupts, especially in a C++ environment. I found that by passing the this pointer as a parameter it makes it easy to get rid of a lot of kludges.

-Aaron