memory leak in functionalInterrupt

[bertmelis]

memory leak on functional interrupt attachInterrupt() call.

https://github.com/esp8266/Arduino/blob/master/cores/esp8266/FunctionalInterrupt.cpp#L20

void attachInterrupt(uint8_t pin, std::function<void(void)> intRoutine, int mode)
{
	// use the local interrupt routine which takes the ArgStructure as argument
	__attachInterruptArg (pin, (voidFuncPtr)interruptFunctional, new ArgStructure{intRoutine}, mode);
}

Every time you call attachInterrupt() a new ArgStructure gets created and doesn't get destroyed.

When I create a (global) ArgStructure and just change the std::function inside the struct (hence reusing the ArgStructure), the memory leak disappears. Obviously, this isn't a valid solution.

[bertmelis]

Actually, I don't understand why there isn't a attachInterrupt(pin, handler, argument, mode). I know it doesn't exist in "standard" Arduino but that alone isn't a valid reason imho.

[devyte]

@bertmelis this is a known issue, with a fix already available, but untested. Please test pr #4609 , and report back here.

[bertmelis]

Thanks for the feedback. I'll post the results.

What do you think about making the attachInterrupt-with-argument available? Everything is already in the code. Or is it not compatible with FunctionalInterrupt?

[devyte]

I suspect it's not needed. AttachInterrupt() takes as arg a std::function, which means you can pass in a lambda that references context variables, or a functor that saves a context internally. Off the top of my head, I think one of those would accomplish the same.
I could be wrong though, it's 2am and I'm writing this on a phone with a severely broken screen :)
@hreintke what do you think?

[bertmelis]

😄 I know, but I just want to pass a class method to the interrupt. A std::function is somewhat overkill. Don't get me wrong: I want both funtional and argument be possible.

[hreintke]

@bertmelis You can use std::bind to use a class function.

This is one line from my code
attachInterrupt(reqEchoPin, std::bind(&SR04::echoInterrupt,this), CHANGE );

[bertmelis]

I know. That's also how I do it now. But after examining the interrupt code I wondered why the argument is not available. Then I could do this:

attachInterrupt(reqEchoPin, (cast)&Class::handler, this, CHANGE );
// and
void Class::handler(Class* instance) {
}

This avoids the overhead of functional/bind and saves some memory. The only reasons I can come up with is that "standard/original" Arduino doesn't have this or that it is incompatible with the code to make functionalInterrupt working.

[hreintke]

I am not 100% sure but I think that "standard/original" arduino does not have the option for argument.
It's up to @devyte to decide whether to stick to it or diverge.

I do not understand your example. How would you get the function from (cast)&Class::method

[igrr]

This avoids the overhead of functional/bind and saves some memory.

Since the implementation would still need to support attaching an arbitrary function object, there would likely be no actual memory savings.

An implementation tuned specifically for your case would only save one word (4 bytes) compared to the one based on std::function (using 12 vs 16 bytes).

With regards to code overhead, i don't understand where it would come from. There's one call to via vtable to the template implementing the call to your class method, and then one call from there to the class method. Hand-written implementation would do exactly the same...

[bertmelis]

Anyways, using the code from the PR, the memory leak is gone. So I'm good to go the way it was (heading at).
I do get a warning: warning: 'scheduledInterrupts' defined but not used

PS: I updated my example from my earlier post.

[bertmelis]

@igrr You're right. I was thinking of having both the functionpointer+argument method and std::function method available. But I lost the fact that you cannot overload a function pointer with a std::function.

[bertmelis]

Memleak solved with PR #4609