Waiting for levels on pins results in "GPIO isr service already installed" error

[jothan]

Bug description

Link to the relevant code

• Would you like to work on a fix? [y/n]
  Potentially, if I understand the issue better.

To Reproduce

From the same thread, I am trying to watch multiple levels on pins concurrently using the async gpio API wait_for_low() and wait_for_high().

I then get an error on one of the two calls and this is printed in the log:

I (11527) gpio: GPIO[4]| InputEn: 0| OutputEn: 0| OpenDrain: 0| Pullup: 0| Pulldown: 0| Intr:0 
I (11531) gpio: GPIO[2]| InputEn: 0| OutputEn: 0| OpenDrain: 0| Pullup: 0| Pulldown: 0| Intr:0 
E (11541) gpio: gpio_install_isr_service(502): GPIO isr service already installed
W (11549) automato_camera_ted::bme280: No BME280 found (Bus(Gpio(GpioError { kind: Other, cause: ESP_ERR_INVALID_STATE (error code 259) }))), continuing without it.

Pins 2 and 4 are the pins I am bit-banging I2C on.

Expected behavior

I should be able to watch for two levels at once from the same thread.

Environment

• Crate (esp-idf-hal) version: 0.45.2
• ESP-IDF branch or tag: 5.3.2
• Target device (MCU): esp32
• Debian Trixie

[Vollbrecht]

Hmm not sure what is going on with a quick glance. We normally should prevent calling gpio_install_isr_service mutliple times, by checking it here. The error you see indicate that it does not do it.

Do you always get the error deterministic or does it have a hunch of raciness to it?

[ivmarkov]

Would it be possible to also post the code that actually installs the PinDriver on top of your scl and sda pins?

I have the feeling that the error you are observing happens much earlier, and outside of the library code you have posted here.

Specifically, the gpio_install_isr_service call that seems to be called twice is called from here.

Then - in turn - this code should not be called twice, because enavle_isr_service is supposed to guard against that, with an atomic and a critical section. So either the atomic and the critical section do not work, which would be... super-weird but possible, or something else happens. But definitely not in your bit-banging lib but somehow outside...

[ivmarkov]

It could also be that something else inside ESP-IDF itself calls gpio_install_isr_service, and hence we end up calling this fn twice, as this "something else" obviously is not aware of our critical section and atomic.

[jothan]

I am using the esp32-camera library that might also be calling into the isr setup functions. The issue is not racy at all. It only occurs once I launch the I2C bus (BME280 sensor). I have added a 10 second delay and the error happens after 10 seconds. The "regular" version of the bit-banged I2C (bbai2c main branch) has no issue and reads temperatures concurrently with the camera taking pictures with no issue all day.

I am using external PSRAM that suffers from the "not-atomic-atomic" issues, but I doubt that the critical section is stored there.

The initialization code is nothing special:

async fn run_app(shutdown: ShutdownManager<()>, executor: &ExecutorHandle) -> anyhow::Result<()> {
    let peripherals = Peripherals::take()?;
    let sysloop = EspSystemEventLoop::take()?;
    let timer_service = EspTaskTimerService::new()?;
    let nvs = EspDefaultNvsPartition::take()?;
    let bme280_task = bme280::task(peripherals.pins.gpio4, peripherals.pins.gpio2);

    // ...
}

pub async fn task<D: IOPin, C: IOPin>(sda: D, scl: C) -> anyhow::Result<()> {
    embassy_time::Timer::after_secs(10).await;
    log::info!("BME280 task started");
    // Wrap is a really thin wrapper, no custom logic.
    let sda = Wrap(PinDriver::input_output_od(sda)?);
    let scl = Wrap(PinDriver::input_output_od(scl)?);

    let mut bus = bbai2c::Initiator::new(
        Timer,
        sda,
        scl,
        Duration::from_micros(5),
        Duration::from_millis(1),
    )?;

    // ...
}

[ivmarkov]

I am using the esp32-camera library that might also be calling into the isr setup functions. The issue is not racy at all. It only occurs once I launch the I2C bus (BME280 sensor). I have added a 10 second delay and the error happens after 10 seconds.

You nailed it. I'm 99.9% sure it is indeed esp32-camera which is giving us troubles therefore.

Well, we need some kind of an API or an impl change where we can just tell esp-idf-hal's GPIO subsystem that that GPIO ISR service is initialized externally, so it should not try to do it.

The only little trouble is whether esp32-camera is also deinitializing the ISR service as well, as if it does, you should make sure this code path inside esp32-camera is not called while you have active esp-idf-hal PinDrivers around. But given that most firmware (yours probably as well) is designed to - once all drivers are initialized - run forever and does not have any de-initialization logic, I doubt you would be hitting the esp32-camera deinitialization logic.

Perhaps all we need is to review the error code from the call to gpio_install_isr_service that we do, and if it complains "already installed" we just output a warning and proceed happily thereafter?

I am using external PSRAM that suffers from the "not-atomic-atomic" issues, but I doubt that the critical section is stored there.

No it is not. It is allocated statically in .bss.

[jothan]

The part that really puzzles me is why one call to wait_for_LEVEL is ok, but two calls triggers this issue. esp32-camera has a gpio_install_isr_service(ESP_INTR_FLAG_LOWMED | ESP_INTR_FLAG_IRAM) call, but no call to the _uninstall function.

[ivmarkov]

The part that really puzzles me is why one call to wait_for_LEVEL is ok, but two calls triggers this issue.

That's fine. Calling wait_for_N does not necessarily result in await if the level is already what you want to wait for. And moreover, the isr services installation call happens only lazily on the first await.

[jothan]

I have created PR #539 with my suggested solution.

Maybe I should go for a two-pronged approach and try to add something like ensure_gpio_service_init() that tolerates being initialized multiple times and checks for flag compatibility.

Ignoring the specific error code automatically seems risky for now without any upstream documentation or API guarantees.

[jothan]

I confirm that calling set_isr_service_flag_unchecked allows the gpio code to coexist with the camera code. I needed to think about the initialization sequence a bit so that both initializations happen in the proper order. I think this issue can be closed now.