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

Execute button and OOM event handlers synchronously #1650

Merged
merged 4 commits into from Dec 20, 2018

Conversation

@sergeuz
Copy link
Member

commented Dec 19, 2018

Problem

Prior to the changes introduced in #1600, there was a bug in system_notify_event() that was causing a system panic in multithreaded applications when a button event was generated. That PR fixed the problem by making all button handlers asynchronous, which, however, is a breaking change, and we'd like to avoid introducing breaking changes at this point.

Solution

  • Execute button event handlers synchronously in the ISR. This matches the behavior of a single-threaded firmware in 0.6.x and 0.7.0.
  • Also makes the out_of_memory event synchronous.
  • Make sure system_notify_event() doesn't attempt to allocate memory in an ISR.

Note: This is a temporary solution. Ideally, it should be up to an application developer to decide which event handlers need to be executed synchronously, even in the context of an ISR if necessary.

Steps to Test

  • wiring/no_fixture (with and without USE_THREADING=y)
  • tests/app/button_event

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)

@technobly technobly changed the title Execute button event handlers synchronously Execute button and OOM event handlers synchronously Dec 20, 2018

@@ -244,7 +244,7 @@ void reset_button_click()
button_current_clicks = 0;
CLR_BUTTON_TIMEOUT();
if (clicks > 0) {
system_notify_event(button_final_click, clicks);
system_notify_event(button_final_click, clicks, nullptr, nullptr, nullptr, NOTIFY_SYNCHRONOUSLY);

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Dec 20, 2018

Contributor

I think I would add the flags as the 2nd parameter, so that it is always a conscious choice.

@technobly technobly merged commit c928057 into develop Dec 20, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@technobly technobly deleted the fix/button_handler branch Dec 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.