adafruit / Adafruit_Seesaw

Arduino library driver for seesaw multi-use chip
93 stars 64 forks source link

Adafruit Joy Featherwing not responding to interrupts when using Seesaw library #64

Closed theficus closed 2 years ago

theficus commented 2 years ago

I have also posted about this on the Adafruit forums but haven't received any response. After doing extensive investigations on this I'm pretty confident this isn't a simple PEBKAC issue but I'm unsure if it's a hardware issue, library issue, or something else I'm missing.

Hardware:

Scenario: I am trying to use an interrupt to respond to button presses from the Joy Featherwing without needing to monitor the button state in a loop.

I'm seeing two suspicious behaviors:

  1. When I press a button, the ISR fires once and only once
  2. When I try to read the Seesaw GPIO state from the ISR, it never returns a result other than 0.

The only way I seem to be able to actually read the state of the Joy Featherwing is from within the main execution loop, which is exactly what I'm trying to avoid.

Here is some sample code:

#include "Adafruit_seesaw.h"

#define BUTTON_RIGHT 6
#define BUTTON_DOWN 7
#define BUTTON_LEFT 9
#define BUTTON_UP 10
#define BUTTON_SEL 14
#define JOY_IRQ_PIN 14

static uint32_t s_button_mask = (1 << BUTTON_RIGHT) | (1 << BUTTON_DOWN) | (1 << BUTTON_LEFT) | (1 << BUTTON_UP) | (1 << BUTTON_SEL);
static Adafruit_seesaw ss;

void onButtonPress() {
    Serial.print("Read: "); 
    Serial.println(ss.digitalReadBulk(s_button_mask), BIN);
}

void setup() {
    Serial.begin(115200);
    ss.begin(0x49);
    ss.pinModeBulk(s_button_mask, INPUT_PULLUP);
    ss.setGPIOInterrupts(s_button_mask, true);
    pinMode(JOY_IRQ_PIN, INPUT);
    Serial.println("Initial read:");
    onButtonPress(); // Log initial state
    Serial.println("setup() done");
    attachInterrupt(JOY_IRQ_PIN, onButtonPress, FALLING);
}

void loop() {
}

For extra debugging, I have built with -DSEESAW_I2C_DEBUG.

Here's the output when I run the program:

ets Jul 29 2019 12:21:46

rst:0x1 (POWERON_RESET),boot:0x13 (SPI_FAST_FLASH_BOOT)
configsip: 0, SPIWP:0xee
clk_drv:0x00,q_drv:0x00,d_drv:0x00,cs0_drv:0x00,hd_drv:0x00,wp_drv:0x00
mode:DIO, clock div:2
load:0x3fff0018,len:4
load:0x3fff001c,len:1044
load:0x40078000,len:10124
load:0x40080400,len:5828
entry 0x400806a8
Begun
Reset
Reading 1 bytes
pos: 1 num:1
Reading 1 bytes
pos: 1 num:1
Done!
Initial read:
Read: Reading 4 bytes
pos: 4 num:4
100011011000000
setup() done
Read: 0

Note that the initial read that happens in setup() shows the expected value. The subsequent read that happens from within the ISR after pressing a button (the final line) shows 0. The ISR fires once, and only once from button presses; any subsequent button presses are ignored. I've further confirmed that the ISR will fire if I short GPIO 14 to + so the ISR should in theory be fine.

I'm guessing at this point the issue has something to do with the ISR context trying to read Seesaw state and something internally not being as expected. The Seesaw API documentation notes that interrupts won't clear until code reads the state, and the fact that the interrupt fires once and only once and doesn't get any result back makes me assume that there's no read actually happening and the internal state doesn't clear. This theory is further bolstered by the debug logging that shows the initial read from setup() getting 4 bytes back while the subsequent read from within the ISR logs nothing.

theficus commented 2 years ago

I did some extensive debugging into this issue this evening and I believe I understand what's going on. It appears that what is happening is when the ISR is calling into the ESP TwoWire library to read data from the I2C bus, it will ultimately fail with I2C_ERROR_BUSY and this is unrecoverable.

Digging further into this, it appears that there are some known limitations on the ESP platform specific to doing I2C operations from within an ISR due to how its FreeRTOS handles task switching. The recommendation for this seems to be to use a ESP task queue to do the work, and have the ISR simply signal the queue to do something. I played around with this some and actually made some pretty good progress.

Here's some prototype sample code (incomplete -- based on the original sample):

QueueHandle_t joyQueue;

void onButtonPress()
{
    int i = 0;
    xQueueSend(joyQueue, &i, portMAX_DELAY);
}

void joyTaskConsumer(void* p)
{
    Serial.println("In queue!");
    int element;
    size_t i = 0;
    while(true)
    {
        Serial.printf("q=%zu\n", i);
        xQueueReceive(joyQueue, &element, portMAX_DELAY);
        uint32_t v = 0;
        v = ss.digitalReadBulk(s_button_mask);
        Serial.println(v, BIN);
        i++;
    }
    vTaskDelete(NULL);
}

void setup()
{
    Serial.begin(115200);
    joyQueue = xQueueCreate(10, sizeof(int));
    ss.begin(0x49);
    ss.pinModeBulk(s_button_mask, INPUT_PULLUP);
    ss.setGPIOInterrupts(s_button_mask, true);
    pinMode(JOY_IRQ_PIN, INPUT);

    xTaskCreate(
        joyTaskConsumer,
        "JoyTaskConsumer",
        10000,
        NULL,
        1,
        NULL);

    attachInterrupt(JOY_IRQ_PIN, onButtonPress, FALLING);
}

This actually seems to work reasonably well -- I am seeing events fire pretty much immediately when the button is depressed and released and on every subsequent button press, and the queue task simply cedes to any other work on the CPU until something gets queued which is basically what I want. In my testing I am running into a new issue where pressing too many buttons at once or too quickly can eventually cause the ESP32 to crash which I'm still troubleshooting (I think this is due to a race with the Serial.println statement), but in all this is working a million times better than before!

It's possible I could also use a semaphore or some other similar approach to do this if I wanted to and I'll do some more experimentation. It seems like the key thing here is to keep the actual work of communicating with the I2C bus out of the ISR.

So in conclusion this seems more like an issue specific to the interrupt use case with the ESP platform rather than a simple bug or PEBKAC problem. This would probably be something useful to call out in the docs or samples as this is something that can come as quite a shock to someone who's trying to use interrupts but is new to the ESP platform.

Further reading:

ladyada commented 2 years ago

ooooh thats interesting and not something we knew about but yeah irq's are not as simple as we'd like and arduino uses A LOT of them behind the scenes. can you submit a PR with this example sketch? call it like "seesaw_ESP32_IRQ" ?

theficus commented 2 years ago

Yes, absolutely. Hopefully it can save else from falling into this same trap in the future. I’ll try to get a PR done this weekend.

ladyada commented 2 years ago

awesome thankx :)

theficus commented 2 years ago

I figured out a fix to the crashes in my sample and also added support for reading the analog stick in a separate task. This allows the ESP32 to handle all controller inputs without blocking other work. I’ll update the sample with this when I get a chance and will close this Issue out when that’s done.

theficus commented 2 years ago

Going to close this after checking in some sample code demonstrating an approach for getting this to work on an ESP32.

Hopefully this saves somebody else some time and headaches. :)