Counting pin interrupts no longer possible with 0.42.5?

[sirhcel]

Upgrading esp-idf-svc from 0.40.1 to 0.42.5 breaks my attempt for counting edges on a GPIO pin. I'm running an application on an ESP32-C3 where the RMT is used for driving LEDs. So I'm using interrupt handler as follows:

static GPIO3_EDGES: AtomicU32 = AtomicU32::new(0);

let mut gpio3 = PinDriver::input(pins.gpio3)?;
gpio3.set_interrupt_type(InterruptType::AnyEdge)?;
unsafe {
    gpio3.subscribe(move || {
        GPIO3_EDGES.fetch_add(1, Ordering::Relaxed);
    })?
};

Enabling this interrupt from "main" and re-enabling it from the interrupt handler does not work out as I can't share the PinDriver mutably by reference between them. Moving it to the handler prevents enabling the interrupt from "main".

If and if yes, how is this supposed to work with esp-idf-hal 0.42.5?

[ivmarkov]

Is there any reason why you want to count the edge interrupts in the interrupt routine? If not, then the routine can just notify a regular (yet maybe high priority) task/thread which can do the counting and the re-enablement of the interrupt for you?

[sirhcel]

I'm using this for checking a fan tachometer signal generating one pulse per revolution. I don't know why I ended up using InterruptType::AnyEdge in the first place but even with just checking for a positive or negative edge, at a speed of 3,000 rpm the interrupt still fires at 3 kHz. This already feels like being on the heavy side.

Using a separate task for counting and re-enabling the interrupt would add two context switches for each interrupt. I have not done the exact numbers yet. But wouldn't this be an an order of magnitude more instructions which need to be executed in this case?

What's the reason behind making it no longer possible to re-enable an interrupt from its handler?

Nevertheless, this was already a workaround in the first place on the ESP32-C3. As I don't need to monitor fan speed continuously, I would nowadays go for using the RMT peripheral in time sharing mode between driving the smart LED and checking the pulses from the fan tachometer in my application. And with the new ESP32-C6 i would just use the dedicated pulse counter.

[Vollbrecht]

Is there any reason why you want to count the edge interrupts in the interrupt routine? If not, then the routine can just notify a regular (yet maybe high priority) task/thread which can do the counting and the re-enablement of the interrupt for you?

@ivmarkov I think this is not the intended use-case here by sirhcel. While it is possible to do that with a notify_and_yield(), the former way of just let it run and get notified eventually with notify is effected here. He wants to count it fast with as little overhead as possible, but don't need live updates for it, just wants to receive it sometime later ( using our async code would also relight on notify_and_yield so no direct option).

IIRC you introduced the changes to automatically disable interrupts, because of LEVEL_INTERRUPTS correct? Because a level interrupt will always trigger as long as the pin is held in that defined level ( That can lead to the system crashing if one stays in isr for to long).

I personally think we just crippled EDGE_INTERRUPTS, to make LEVEL_INTERRUPTS work like you would expect form a EDGE_INTERRUPT. But i think that is wrong because the initial assumption, that one just can use LEVEL_INTERRUPTS willy nilly is wrong. The purpose of them is they force the mcu in an interrupt as long as the line is hold and in my opinion serve a pretty specific niche. Obviously we can do with them whatever we want, as long as we make it workable. If i understand correctly your use-case is you want to trigger on something where you know that the line did potentially already switched level before you enabled the routine and just want to asynchronously now the level once?

I see three paths forward here:

1. We make it depended on the type of INTERRUPT the user proved. On edge-trigger it does not disable interrupts. on level trigger disabling it and bending the definition of it to our own opinionated view of it.
2. Don't disable INTERRUPT at all, and clearly communicate to the user what one should expect from a LEVEL_INTERRUPT.
3. Don't change anything and stay on this highly opinionated implementation and force the users to always yield and re-enable with the extra overhead.

[ivmarkov]

1. We make it depended on the type of INTERRUPT the user proved. On edge-trigger it does not disable interrupts. on level trigger disabling it and bending the definition of it to our own opinionated view of it.
2. Don't disable INTERRUPT at all, and clearly communicate to the user what one should expect from a LEVEL_INTERRUPT.

The problem with these two is that if - for whatever reason - the user would like to disable/enable interrupts from the subscribe callback, she would have no way of doing that as the driver is not available in the interrupt closure, and cannot be made available by e.g. a mutex protection, because, well, you are in an interrupt. While for (1) this might or might not be a problem, for (2) this is a show-stopper.

I think these two can only work if we extend the signature of the subscribe closure in a way where it takes *mut PinDriver. And yes, I do mean *mut and not &mut as the user would be potentially aliasing a mutable ref to the driver, as in "user space" the code might be mutably (or immutably) operating on the driver already, when the interrupt happens.

Other than that, I have to look whether the async code will continue to work correctly under the new constellation of not disabling the interrupts once the first one hits.

[acmorrow]

I'm struggling with this too. Admittedly, I'm rather new to ESP32 and Rust and esp-idf-hal, so it is entirely possible I'm misunderstanding how to use the APIs and how the board works, etc.. Apologies in advance if that is the case. However, what I want to do is fairly simple, and the decision to make subscribe effectively one-shot seems to be getting in the way of using what would otherwise be a really convenient API.

There is an independently changing variable (AtomicI32 shared with subscribe callback via Arc, say) that I want to read from in a subscribe callback a when a GPIO gets driven high and then read again when it goes back to low, then report the change in the variable between those two events. The separation between these events is maybe rather small. I can subscribe with AnyEdge and the callback will capture the first transition, but the interrupt becomes disabled by processing the first edge, and my controlling logic (which maybe hasn't even gotten scheduled again and is still sitting in Notification::wait) is now in a race to re-enable the interrupt and re-subscribe before the second transition arrives, which it loses. So I never see the falling edge.

If the interrupt was not auto-disabled after firing, I could just have the subscribe callback stash the the value seen on the first interrupt somewhere, then do the notification to wake the waiter with the computed delta only after the second interrupt arrives.

Perhaps subscribe and subscribe_nonstatic should be restored to the original behavior, and new subscribe_once and subscribe_once_nonstatic could be added with the current semantics, if those are still considered desirable? Or maybe the one-shot-ness could remain be default-on but configurable and therefore you could opt out? Something like PinDriver::disable_oneshot.

I'm also a little curious about the intended behavior of subscribe with AnyEdge, since you don't actually know in the interrupt callback whether you are there because of a PosEdge or a NegEdge type event, as far as I can see. It'd be nice to be able to, instead, subscribe to both PosEdge and NegEdge. That isn't possible though, as far as I can tell, because I believe you can only set_interrupt_type once, then call subscribe. So you can't subscribe separately to PosEdge and NegEdge if you need or want to tell them apart. And since the current implementation makes subscribe one-shot, you can't reliably build a little state machine that'd track it for you from a known starting state.