PinDriver's subscribe method and wait_for_* methods are incompatible about esp-idf-hal HOT 11 CLOSED

I'll do a quick fix now, without waiting for a possible anti-globalization rework.

from esp-idf-hal.

The subscribe method on the PinDriver should keep a hold of &mut self for as long as the subscription is required.

This means you can't call ANY method on the PinDriver until the subscription is active. As in... not even reading the state of the pin.

The pattern you are suggesting only works when it takes &self (not &mut self) and then all other methods (or most of them) also take &self.

from esp-idf-hal.

Of course a subscribe(&self) method has the disadvantage that you can call subscribe multiple times, which is suitable for something like an event bus, but probably not for the pin, which can only have one or zero active subscriptions at a time.

from esp-idf-hal.

Yeah &mut is probably too strict, though if you're listening for pin state changes then do you need to check the state of the pin?
It might okay to pass the pin in the lambda maybe, something like let _life = gpio12.subscribe(|pin| pin.is_on());

Taking &self is probably too loose but it's at least not worse than what's currently there I suppose.

from esp-idf-hal.

Yeah &mut is probably too strict, though if you're listening for pin state changes then do you need to check the state of the pin?

Typical use case:debouncing.

It might be okay to pass the pin in the lambda maybe, something like let _life = gpio12.subscribe(|pin| pin.is_on());

No it is not ok. When the subscribe lambda is called, you are in an ISR. First of all, I'm not sure if all pin methods are safe to call from an ISR context, but that's not even the point as the callback is unsafe and "use at your own risk", kind of. The point is, you might have legit needs to operate on the pin when you are subscribed to its state, yet your suggestion completely removes that option.

Taking &self is probably too loose but it's at least not worse than what's currently there I suppose.

It is exactly the same as what is currently there except it requires to redesign the driver to have everything &self compatible (if that's even possible), yet it still does not fulfill its main premise which would be that you can't subscribe more than once.

Given that subscribe is (a) unsafe (b) called from an ISR context, I think you are losing too much sleep over an API which is anyway very dangerous to use.

Use the async APIs, as they are safe and hide the whole ISR ugliness. Or create a similar blocking api.

from esp-idf-hal.

I think you are losing too much sleep over an API which is anyway very dangerous to use.

You're probably right.

yet it still does not fulfill its main premise which would be that you can't subscribe more than once.

My goal isn't necessarily to prevent multiple calls to subscribe. My goal was to prevent wait_for_* being called whilst a subscription is still active.

This allows me to remove the global PIN_NOTIFS and put it on the stack 😅 like it was done for SPI.

from esp-idf-hal.

There is a key difference between spi and the pin driver, which might be a roadblock for your anti-globalization plans: the current global notifs allow you to receive notification that the pin has changed its state even when you dont await on that. For awaiting for the pin to become low or high this is irrelevant. But if you are awaiting on edge change, you might miss it if you are only subscribed to the pin during the execution of the async method.

Whether that matters I'm not sure yet, but wanted to point it out.

from esp-idf-hal.

the current global notifs allow you to receive notification that the pin has changed its state even when you dont await on that.

Is that actually correct? That behavior doesn't actually match the e-hal docs.

from esp-idf-hal.

the current global notifs allow you to receive notification that the pin has changed its state even when you dont await on that.

Is that actually correct? That behavior doesn't actually match the e-hal docs.

You might have a point now that I'm reading this.

Not sure if this comment was there in the earlier versions. Might have been, and I just missed it.

from esp-idf-hal.

I'm closing this, as I was able to implement it (that is, subscribe and wait_for co-existence) as a side effect of an annoying issue where users were supposed to call PinDriver::disable_interrupt() from subscribe to avoid IWDT triggering on level-based (and maybe not only) interrupt types. Something definitely non-obvious (was very non-obvious to me at least until recently).

from esp-idf-hal.

Just looked at the new commit and it looks like gpio_intr_enable isn't being called anymore?

Also, how do I enable interrupts without having the gpio service installed? I've already installed it in my code and now my application is crashing when I call enable_interrupt because the service is being installed twice.

from esp-idf-hal.