Skip to content

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

Merged
merged 8 commits into from
Jul 7, 2018
Merged

Allow using argument with attachInterrupt #1535

merged 8 commits into from
Jul 7, 2018

Conversation

bertmelis
Copy link
Contributor

split __attachArgument and __attachArgumentArg like on ESP8266.
it could pave the way for functional Interrupt.

@@ -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);
Copy link

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.

Copy link
Contributor Author

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.

@@ -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){
Copy link

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?

@ghost
Copy link

ghost commented Jun 27, 2018

Good job. looks good to me. Left a minor comments.

replace tabs with spaces
@me-no-dev
Copy link
Member

no alias for __attachInterruptArg? how do you call it?

@bertmelis
Copy link
Contributor Author

My idea was to have it not directly available because it is also not the case for ESP8266 nor AVR-Arduino.
If you want to use it, you'll have to extern "C" the function definition.

It should pave the way for FunctionalInterrupt.

@me-no-dev
Copy link
Member

@bertmelis why not add an example then :)

@bertmelis
Copy link
Contributor Author

bertmelis commented Jun 29, 2018

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.

@bertmelis
Copy link
Contributor Author

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?

@me-no-dev
Copy link
Member

nooo you can not call it in ISR ;) it calls esp_intr_disable/esp_intr_enable. Maybe add detachInterruptFromISR that does not call them. And please add void attachInterruptArg(uint8_t pin, voidFuncPtrArg handler, void* arg, int mode); to the API like the rest

@bertmelis
Copy link
Contributor Author

And please add void attachInterruptArg(uint8_t pin, voidFuncPtrArg handler, void* arg, int mode); to the API like the rest

Done!

@me-no-dev me-no-dev merged commit c77aed4 into espressif:master Jul 7, 2018
@me-no-dev
Copy link
Member

Merged :)

@hreintke
Copy link
Contributor

hreintke commented Jul 8, 2018

Now he attachInterruptArg is a "public" function instead of a "core only" function,
I cannot use the knowledge that the arg is a pointer to InterruptArgStructure which holds the functionalInterrupt routine.
Need to find another solution for FunctionalInterrupt.

Update : When FunctionalInterrupt is available the interruptArg is available using std::bind(..) for public use

@bertmelis
Copy link
Contributor Author

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.

@bertmelis
Copy link
Contributor Author

@hreintke When releasing a functional interrupt, you can just remove the "extern" alltogether.

@hreintke
Copy link
Contributor

hreintke commented Aug 5, 2018

@bertmelis see #1728

Curclamas pushed a commit to Curclamas/arduino-esp32 that referenced this pull request Aug 21, 2018
* 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
@bertmelis bertmelis deleted the attachInterrupt-with-arg branch November 20, 2018 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants