Hi all,
I'm not sure if this is the correct place to post this but I have a proposed addition or change to attachInterrupt that greatly simplifies numerous cases involving the use of interrupt service routines.
I have been writing device drivers for various operating systems for many years and I found that the interrupt handling in the Arduino environment is very annoying due to one major deficiency.
In just about every environment I have worked on when I register an interrupt service routine I can also assign a user-defined pointer to be passed to the ISR. This is extremely useful, especially in a C++ environment like Arduino.
For example, I wrote my own rotary encoder class to deal with the ESP32 and found that without this that I needed a separate ISR function for every single GPIO that is registered and for every single instantiation of my class. On top of this, it requires a lot of static variables and other things to get around this. This is how the normal RotaryEncoder class works. It's a huge ugly kludge that is totally unnecessary with this one change.
What I did is I went into the esp32-hal-gpio.[ch] files and added a new function, attachInterrupt2.
Prototype:
void attachInterrupt2(uint8_t pin, void (*)(int pin, int value, void *data),
int mode, const void *data);
Since the pin information is already available I decided to pass it on to the handler as well as the value for the pin. This allowed me to totally eliminate all the kludges. Now I can have a single ISR function that can handle all of the GPIO pins. I just created a short 2-entry array inside my class that has the bit number for the GPIO and a pointer to the class. When the ISR is called it is passed a pointer to this structure. Since it has the "this" pointer as well as the bit number, a single ISR function handles everything. On top of that, it allowed me to get rid of all the globals and static crap needed with the old method.
My code now looks like this:
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;
Serial.printf("RotaryEncoder: start state: %d\n", state);
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);
This is a lot cleaner than the old Encoder class that also doesn't work on the ESP32. I'm sorry about the lack of comments since this is a work in progress.
I have attached my patch to the esp32 platform in order to support this. The change to the feather_esp32/pins_arduino.h can be ignored. I needed this to help make the SD card work properly.
A different patch could be made where instead of two arrays of ISR functions only one array is maintained and the old code would just ignore the extra parameters since it expects no parameters.
My proposed addition is not specific to any particular Arduino platform. It does require more memory be consumed since now in addition to a function pointer a data pointer must also be allocated for every GPIO interrupt service routine, however I feel that the benefits greatly outweigh the additional memory overhead.