-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Allow using argument with attachInterrupt #1535
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow using argument with attachInterrupt #1535
Conversation
cores/esp32/esp32-hal-gpio.c
Outdated
@@ -244,6 +253,10 @@ extern void __attachInterrupt(uint8_t pin, voidFuncPtr userFunc, int intr_type) | |||
esp_intr_enable(gpio_intr_handle); | |||
} | |||
|
|||
extern void __attachInterrupt(uint8_t pin, voidFuncPtr userFunc, int intr_type) { | |||
__attachInterruptArg(pin, (voidFuncPtrArg)userFunc, NULL, intr_type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The casting (voidFuncPtrArg) looks redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but then I get a compiler warning.
cores/esp32/esp32-hal-gpio.c
Outdated
@@ -208,7 +209,11 @@ static void IRAM_ATTR __onPinInterrupt(void *arg) | |||
do { | |||
if(gpio_intr_status_l & ((uint32_t)1 << pin)) { | |||
if(__pinInterruptHandlers[pin]) { | |||
__pinInterruptHandlers[pin](); | |||
if(arg){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks too much indentation in here. Are there tabs or gitub change like this?
Good job. looks good to me. Left a minor comments. |
replace tabs with spaces
no alias for __attachInterruptArg? how do you call it? |
My idea was to have it not directly available because it is also not the case for ESP8266 nor AVR-Arduino. It should pave the way for FunctionalInterrupt. |
@bertmelis why not add an example then :) |
Hehe, it doens't work. Obviously. I used it with only 1 interrupt. And the code as it stands now shares the ISR but it also shares the argument. So it only takes the argument that is used first. Solution would be to expand the __pinInterruptHandlers-array with the argument (make it a struct) and use it in the ISR. |
I made a very simple example but somethings not working: when I detach from within an interrupt it doesn't detach. Now I just "simulate" a button by touching the pins with a wire to 1 press are like 56 presses. But I suspect the detachInterrupt is not working in the master codebase neither, when called from within an ISR. Shouldn't detachInterrupt be marked IRAM_ATTR, and does this solve the issue? |
nooo you can not call it in ISR ;) it calls |
Done! |
Merged :) |
Now he attachInterruptArg is a "public" function instead of a "core only" function, Update : When FunctionalInterrupt is available the interruptArg is available using std::bind(..) for public use |
Can't you just use the same logic as for esp8266? There's not that much different. And the alias is "weak", so you can override. |
@hreintke When releasing a functional interrupt, you can just remove the "extern" alltogether. |
@bertmelis see #1728 |
* Allow using argument with attachInterrupt * formatting replace tabs with spaces * fix bug more then 1 interrupt * leftover * add example * make attachInterruptArg public * update example * leftover
split __attachArgument and __attachArgumentArg like on ESP8266.
it could pave the way for functional Interrupt.