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

fixes SOS in 0.8.0-rc.11 #1600

Merged
merged 3 commits into from Dec 13, 2018

Conversation

@m-mcgowan
Copy link
Contributor

commented Nov 14, 2018

Backport of PR #1598 for non-mesh hardware.


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)

@m-mcgowan m-mcgowan requested a review from jlovinger-particle Nov 14, 2018

@m-mcgowan m-mcgowan changed the title /system event on isr develop fixes SOS in 0.8.0-rc.11 Nov 14, 2018

@m-mcgowan m-mcgowan added this to the 0.8.0-rc.12 milestone Nov 14, 2018

@jlovinger-particle
Copy link

left a comment

Seems slightly safer to have system_notify_event() check for and adjust behavior when called from ISR instead of requiring ISR to call the ISR specific system_notify_event_isr(). However this fixes the original issue and that modification is arguable.

@sergeuz

This comment has been minimized.

Copy link
Member

commented Dec 5, 2018

I agree with @jlovinger-particle. It's a little bit confusing that system_notify_event_isr(), which is a more specialized function, can be called in any context, while system_notify_event() can be used only in a thread. I'll refactor this PR to make system_notify_event() work both in an ISR or thread if nobody minds

@sergeuz

This comment has been minimized.

Copy link
Member

commented Dec 5, 2018

@jlovinger-particle Could you please re-review this PR?

@m-mcgowan

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2018

The intent is that system_notify_event() can never be on an ISR, while send_system_event_isr() may be on an ISR so that the known context is explicitly called out rather than being left to guesswork. I suppose it's ok to change the logic - the isISR() check is presumably fairly performant, so could be a case of premature optimization if that were the only consideration. Also I wanted to go for minimum change. If this isn't clear, happy to have system_notify_event() work for all cases.

When coding this I wanted a way to have functions describe their calling context (ISR/thread), I didn't come up with anything, but would be great to have the compiler check that only ISR-allowed functions are called from an ISR context. Could perhaps be a custom extension with some special attributes defined on ISR-allowed functions?

@jlovinger-particle

This comment has been minimized.

Copy link

commented Dec 5, 2018

Whatever you guys decide on I'm sure will be OK and all will be equally functional. Improvement on the status quo.

We're kind of getting into the weeds because system_notify_event with this Pull request hides three kinds of behaviors.

Defer initial execution into System thread if running from an interrupt.
Defer final execution into Application thread if running from the System Thread.
Execute immediately if running from the Application Thread (but not an ISR!).

Without really ripping it apart or refactoring it will be hard to make explicitly clear the execution paths without also parsing through the code. For example...

  1. ISR interrupts Application Thread and triggers an event
  2. Defer event into the System Thread
  3. Drop back into the Application Thread
  4. Preempt/etc into the System Thread
  5. Begin processing event in the System Thread
  6. Defer event into the Application Thread
  7. Preempt/etc into the Application Thread
  8. Process event

None of which is made clear by the old/new names of system_notify_event/system_notify_event_isr OR the new/new with system_notify_event/system_notify_impl.

And on top of that, previously it would have just been...

  1. ISR interrupts Application Thread and triggers an event
  2. Process event immediately since detects Application Thread context.

Which was likely bad in its own way as the ISR would have been executing like it was the Application Thread introducing all sorts of concurrency issues I wonder if people were handling. But if they were it sure would have been responsive...

Unexpected complexity, for sure.

@sergeuz

This comment has been minimized.

Copy link
Member

commented Dec 5, 2018

@m-mcgowan My reasoning is that this is the system code, which is (or should be) pretty well abstracted from the low-level platform code. If some handler runs in an ISR on one platform it doesn't mean that it will be running in an ISR on another platform as well, and by hiding these details we're improving portability of the system code. Also, most of our current APIs perform the ISR check internally rather than provide separate functions that can be used in ISRs

@sergeuz

This comment has been minimized.

Copy link
Member

commented Dec 5, 2018

@jlovinger-particle That's correct. Ideally, there should be a separate ISRTaskQueue for the application thread. I think that the current solution is fine for now though

@sergeuz

This comment has been minimized.

Copy link
Member

commented Dec 5, 2018

@m-mcgowan I doubt it's possible to determine the context of a function at compile time, but what we can do is to introduce a custom attribute (e.g. "isr_safe") and require that all functions called from such an ISR-safe function should also be ISR-safe – similarly to how const methods work in C++

@jlovinger-particle

This comment has been minimized.

Copy link

commented Dec 5, 2018

introduce a custom attribute (e.g. "isr_safe") and require that all functions called from such an ISR-safe function should also be ISR-safe

Famous last words :)

@sergeuz

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

@jlovinger-particle Could you please approve this PR if the changes look good to you?

@sergeuz sergeuz requested a review from avtolstoy Dec 13, 2018

if (space) {
auto task = new (space) SystemEventTask(event, data, pointer, fn, fndata);
SystemISRTaskQueue.enqueue(task);
};

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Dec 13, 2018

Member

nit: extra ;

@sergeuz sergeuz merged commit b113b6c into develop Dec 13, 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

@sergeuz sergeuz deleted the fix/ch24165/system_event_on_isr_develop branch Dec 13, 2018

@sergeuz sergeuz referenced this pull request Dec 19, 2018
4 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.