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

Increased stack size and stack overflow detection for Electron #884

Merged
merged 4 commits into from Mar 10, 2016

Conversation

@sergeuz
Copy link
Member

commented Feb 22, 2016

This PR fixes #821 by increasing stack size for threads created via std::thread wrapper and adds FreeRTOS hook for stack overflow detection -- latter is enabled only for debug builds on Electron currently (thanks to @avtolstoy for pointing me out).

@m-mcgowan m-mcgowan added this to the 0.5.x milestone Feb 23, 2016

@@ -94,7 +94,7 @@ namespace std {
thread_startup startup;
startup.call = base.get();
startup.started = false;
if (os_thread_create(&_M_id._M_thread, "std::thread", OS_THREAD_PRIORITY_DEFAULT, invoke_thread, &startup, 1024*3)) {
if (os_thread_create(&_M_id._M_thread, "std::thread", OS_THREAD_PRIORITY_DEFAULT, invoke_thread, &startup, 1024*4)) {

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Feb 23, 2016

Contributor

This will increase the stack size on the Photon too. I think that until we know the size increase is needed there, that this should be conditional. You can check HAL_PLATFORM_CLOUD_UDP which will be 1 when UDP is enabled.

I'd like to know what is requiring so much stack size in the UDP handshake, and I'm curious why we get corruption since the heap grows from the bottom of memory while the stack grows from the top. Ah...but the system thread stack is allocated on the heap so that's why any overflow corrupts the heap.

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Feb 23, 2016

Nice work, particularly enabling the stack overflow detection and glad you narrowed it down to the overflow. I did test overflow, but that was before I fixed the multithreading allocation problem, and didn't get around to retesting with a larger stack with that fix in place.

At some point it would be nice to know what is taking up so much stack (are there profilers that can tell is this?). In the meantime, bumping it up 1K for UDP comms is fine. 👍

@sergeuz

This comment has been minimized.

Copy link
Member Author

commented Feb 23, 2016

I'm not aware of any suitable profilers that we can use on microcontrollers, but we can try to use function call instrumentation provided by gcc -- it allows to specify couple of functions that should be automatically called after entering and before leaving of any other function. Within of those "injected" functions we can dump stack trace and stack usage, for example.

@sergeuz

This comment has been minimized.

Copy link
Member Author

commented Feb 23, 2016

Btw, I'd like to enable similar stack overflow detection on Photon, but WICED already has vApplicationStackOverflowHook() defined and exposes wiced_rtos_check_stack() function instead, which is quite useless as it needs to be called periodically. What is the best way to override vApplicationStackOverflowHook() with our own implementation in this case? Change link order?

@sergeuz sergeuz force-pushed the sergeuz:threadcrash branch from 7a067b5 to 8289dd6 Mar 2, 2016

@sergeuz

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2016

Added similar FreeRTOS hook to Photon. Larger stack size is used only if HAL_PLATFORM_CLOUD_UDP is defined.

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2016

Just doing a final check here, looks good. The electron firmware is built with debug enabled by default - will there be any performance or other impact from enabling stack overflow detection? (I'm asking since I don't know how it's implemented and don't know what any affects of having this enabled would be.)

@sergeuz

This comment has been minimized.

Copy link
Member Author

commented Mar 10, 2016

Yes, there's an impact definitely. The check is performed before the context switches to another task:

Stack overflow checking introduces a context switch overhead so its use is only recommended during the development or testing phases (http://www.freertos.org/Stacks-and-stack-overflow-checking.html).

Note that Photon's WICED is also built with stack overflow detection enabled.

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2016

Ah yes, I forgot it was in WICED by default, good call! 👍 That makes me feel better about including it.

@sergeuz

This comment has been minimized.

Copy link
Member Author

commented Mar 10, 2016

Also, overflow detection requires threading enabled at runtime -- otherwise everything will be running in the main thread without FreeRTOS involved. Correct me if I'm wrong here, but this check doesn't seem to affect much of our users at the moment.

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2016

That's correct. Apart from software timers, the default single threaded application mode doesn't use threads. But threading will become the default for the 1.0 release so then it will be used a lot!

m-mcgowan added a commit that referenced this pull request Mar 10, 2016
Merge pull request #884 from sergeuz/threadcrash
Increased stack size and stack overflow detection for Electron

@m-mcgowan m-mcgowan merged commit c5da63d into particle-iot:develop Mar 10, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.