Skip to content
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

Avoid freeing memory in detachInterrupt #2356

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

metcalf
Copy link

@metcalf metcalf commented Sep 14, 2021

Problem

When attachHandler is called with a function pointer (or a pointer to a member function and instance), it allocates a std::function. When detachHandler is then called on the same pin, it calls delete to free that memory. That causes a heap error if you try to call detachHandler on this pin from an ISR. The same is true for attaching and detaching system interrupts.

Solution

This PR removes the deletes and leaves the memory allocated to be re-used the next time attach* is called. This prevents the application from reclaiming the memory std::function is allocating on the heap, but I suspect that's OK since there can be no more allocations than the number of pins.

I think the alternative is to update the documentation to specify when detachInterrupt can be called. It's probably still worth updating the docs to specify that attachInterrupt and attachSystemInterrupt cannot be called from an ISR when used with a function pointer / class+member.

Steps to Test

Wrote and ran an integration test on my Photon.

Example App

Deferring to the integration test, but can write something if necessary.

References

The delete in detachSystemInterrupt was added in #951. I think the primary problem fixed in that PR was an unbounded memory leak repeatedly calling attachSystemInterrupt.


Completeness

  • User is totes amazing for contributing!
  • Contributor has signed CLA (Info here)
  • Problem and Solution clearly stated
  • Run unit/integration/application tests on device
  • Added documentation
  • Added to CHANGELOG.md after merging (add links to docs and issues)

@metcalf
Copy link
Author

metcalf commented Nov 19, 2021

Checking in on this since it's blocking me from using Cloud Flash. Is this likely to get merged or should I rewrite my application to avoid this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants