Add thread terminate hook #4684

deepikabhavnani · 2017-06-30T18:45:38Z

Description

Hook was added in RTX4 code to assist test framework to log thread info on thread terminate, which was not working with RTX5.

Status

READY

Steps to test or reproduce

Execute test mbed-os-tests-mbedmicro-rtos-mbed-threads with MBED_STACK_STATS_ENABLED set to 1.

@c1728p9 @bulislaw

c1728p9

Thanks for the fix

sg-

The event recorder changes seem to be an addition not needed for the fix. If we're going to use the event recorder that should come in an additional PR or commit (if an extension of how its already used).

0xc0170 · 2017-07-02T06:58:46Z

rtos/rtx5/mbed_rtx_handlers.c

+#endif
+}
+
+void EvrRtxThreadTerminate (osThreadId_t thread_id) {


{ on the new line for both functions as the rest of functions in this file

c1728p9 · 2017-07-02T17:21:35Z

@sg- the event recorder stuff is to keep the RTOS behavior the same. It's not an addition. The functions EvrRtxThreadExit and EvrRtxThreadTerminate in this PR override weak the weak versions in rtx_evr.c. If the event logic isn't the same in these overridden functions, then RTX 5 event reporting will no longer work correctly in Keil.

bulislaw

LGTM

sg- · 2017-07-04T11:34:05Z

@sg- the event recorder stuff is to keep the RTOS behavior the same. It's not an addition. The functions EvrRtxThreadExit and EvrRtxThreadTerminate in this PR override weak the weak versions in rtx_evr.c. If the event logic isn't the same in these overridden functions, then RTX 5 event reporting will no longer work correctly in Keil.

OK thanks for the clarification. I had searched for that but failed from the trailing _ so I guess now the question is why is there a trailing _, why RTX_Config.h needed and why not copy the define over as part of the block rather than literal with no meaning?

deepikabhavnani · 2017-07-05T14:18:58Z

@sg- : I added trailing _ to avoid re-definition and to know what was updated in RTX5 in case of future RTX changes. Do we have some standard mechanism to keep track of such changes (over-write behavior of RTX5), so that we do not miss them in future merges?

bulislaw · 2017-07-05T14:29:07Z

So why are we defining it if it causes redefinition? Can't we just use the RTX defined events? We have some CMSIS/RTX documentation here including changes to RTX, adding section that documents what's we overload in mbed_rtx_handlers.c is a good idea https://docs.mbed.com/docs/mbed-os-handbook/en/5.5/advanced/cmsis-rtos/

theotherjimmy · 2017-07-10T15:43:13Z

@deepikabhavnani Bump. You may want to have a chat with @bulislaw offline to get clarification of what needs to be done.

bulislaw · 2017-07-12T09:09:31Z

@RobertRostohar @JonatanAntoni is there any reason why all the event ids are defined in .c file? https://github.com/ARM-software/CMSIS_5/blob/develop/CMSIS/RTOS2/RTX/Source/rtx_evr.c all the functions are __WEAK as to be overloaded, but the event ids themselves need to be redefined in this case. Would you be open to a PR that moves the event IDs to .h file?

theotherjimmy · 2017-07-24T15:30:37Z

Bump. @bulislaw @deepikabhavnani Any news?

bulislaw · 2017-07-24T15:33:47Z

@JonatanAntoni can you look at the question above?

deepikabhavnani · 2017-07-24T16:25:00Z

As per @JonatanAntoni: over writing event handlers is not correct way to fix this issue. PR is required to enhance RTX thread with an official exit hook.

deepikabhavnani · 2017-07-25T14:32:04Z

Solution from CMSIS might be delayed till CMSIS 5.1 release.
@sg- @bulislaw Should we proceed with this temporary fix or hold it?

theotherjimmy · 2017-08-21T15:17:01Z

@sg- @bulislaw @deepikabhavnani any news?

deepikabhavnani · 2017-08-29T14:24:24Z

@bulislaw @sg- Solution from CMSIS will not be part of next CMSIS release as well (5.2.0) and might be delayed for long.

Shall we proceed with this temporary fix, else I would like to close this PR and keep issue open.

bulislaw · 2017-08-29T14:33:19Z

What issues does it cause? If it's an actual problem I'll say lets bring it in now, if it's an improvement/good to have it'll probably be better to wait till it's fixed in RTX.

deepikabhavnani · 2017-08-29T14:59:55Z

@bulislaw - Thread terminate hook is to record stack/heap related information during CI, we will not get details of threads which were terminated due to some issues. It won;t stall anything only will make debugging difficult in case of issues.

theotherjimmy · 2017-08-29T18:20:36Z

@deepikabhavnani Could you rebase to resolve the conflicts?

0xc0170 · 2017-09-01T10:46:15Z

Hook was added in RTX4 code to assist test framework to log thread info on thread terminate, which was not working with RTX5.

@deepikabhavnani This info could be part of the commit msg to provide useful info why its being added.

I would like to close this PR and keep issue open.

Is there an issue for this? I don't see it referenced here anywhere.

Shall we proceed with this temporary fix, else I would like to close this PR and keep issue open.

I am fine with this as the fix to bring the old behavior back as it was useful and being used (in the tests mentioned above). Once RTX gets an update, we will replace this with a proper fix. If someone has a strong no for this, speak now.

deepikabhavnani · 2017-09-07T14:23:25Z

Rebased to resolve conflicts and updated commit message

deepikabhavnani · 2017-09-12T02:15:50Z

If everyone is ok with this solution shall we merge it? After CI :-)

c1728p9 · 2017-09-14T15:39:40Z

I created ARM-software/CMSIS_5#245 to track this with the RTX team.

c1728p9 · 2017-09-14T15:49:14Z

/morph test

mbed-bot · 2017-09-14T21:05:11Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1309

Test failed!

Hook was added in RTX4 code to assist test framework to log thread info on thread terminate, which was not working with RTX5.

deepikabhavnani · 2017-09-14T22:18:16Z

3 test were skipped for all targets, not sure why :-(

Testing thread priority ops
Testing thread states: wait message put
Testing thread with external stack memory -

Rebased and verified K64F on local setup

deepikabhavnani · 2017-09-14T22:18:25Z

/morph test

mbed-bot · 2017-09-15T01:30:44Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1312

Test failed!

deepikabhavnani · 2017-09-15T16:40:35Z

/morph test

mbed-bot · 2017-09-15T19:03:16Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1318

All builds and test passed!

deepikabhavnani requested review from bulislaw and c1728p9 June 30, 2017 18:46

c1728p9 approved these changes Jun 30, 2017

View reviewed changes

sg- suggested changes Jul 1, 2017

View reviewed changes

0xc0170 reviewed Jul 2, 2017

View reviewed changes

0xc0170 added the needs: work label Jul 3, 2017

bulislaw approved these changes Jul 3, 2017

View reviewed changes

deepikabhavnani force-pushed the thread_stack_issue branch 2 times, most recently from 2c34d16 to cd65c34 Compare July 11, 2017 19:08

theotherjimmy added needs: review and removed needs: work labels Jul 17, 2017

deepikabhavnani added the needs: preceding PR label Jul 24, 2017

deepikabhavnani added needs: clarification and removed needs: preceding PR needs: review labels Aug 29, 2017

theotherjimmy added the needs: review label Aug 29, 2017

deepikabhavnani force-pushed the thread_stack_issue branch from cd65c34 to a6cce99 Compare August 29, 2017 20:26

0xc0170 changed the title ~~Added thread terminate hook~~ Add thread terminate hook Sep 1, 2017

deepikabhavnani force-pushed the thread_stack_issue branch from a6cce99 to 5ec41e1 Compare September 6, 2017 14:44

theotherjimmy added needs: review and removed needs: clarification labels Sep 14, 2017

c1728p9 mentioned this pull request Sep 14, 2017

Hooks for thread terminate and exit ARM-software/CMSIS_5#245

Open

sg- approved these changes Sep 14, 2017

View reviewed changes

deepikabhavnani added needs: CI and removed needs: review labels Sep 14, 2017

Added thread terminate hook

a2b5301

Hook was added in RTX4 code to assist test framework to log thread info on thread terminate, which was not working with RTX5.

deepikabhavnani force-pushed the thread_stack_issue branch from 5ec41e1 to a2b5301 Compare September 14, 2017 22:17

Threads test require more time to finish

6f5df1f

deepikabhavnani added ready for merge and removed needs: CI labels Sep 19, 2017

0xc0170 added the release-version: 5.6.1 label Sep 22, 2017

0xc0170 merged commit 1fed1d0 into ARMmbed:master Sep 22, 2017

0xc0170 removed the ready for merge label Sep 22, 2017

deepikabhavnani deleted the thread_stack_issue branch September 26, 2017 18:53

Add thread terminate hook #4684

Add thread terminate hook #4684

Uh oh!

Conversation

deepikabhavnani commented Jun 30, 2017

Description

Status

Steps to test or reproduce

Uh oh!

c1728p9 left a comment

Choose a reason for hiding this comment

Uh oh!

sg- left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 Jul 2, 2017

Choose a reason for hiding this comment

Uh oh!

c1728p9 commented Jul 2, 2017

Uh oh!

bulislaw left a comment

Choose a reason for hiding this comment

Uh oh!

sg- commented Jul 4, 2017

Uh oh!

deepikabhavnani commented Jul 5, 2017

Uh oh!

bulislaw commented Jul 5, 2017

Uh oh!

theotherjimmy commented Jul 10, 2017

Uh oh!

bulislaw commented Jul 12, 2017

Uh oh!

theotherjimmy commented Jul 24, 2017

Uh oh!

bulislaw commented Jul 24, 2017

Uh oh!

deepikabhavnani commented Jul 24, 2017

Uh oh!

deepikabhavnani commented Jul 25, 2017

Uh oh!

theotherjimmy commented Aug 21, 2017

Uh oh!

deepikabhavnani commented Aug 29, 2017

Uh oh!

bulislaw commented Aug 29, 2017

Uh oh!

deepikabhavnani commented Aug 29, 2017

Uh oh!

theotherjimmy commented Aug 29, 2017

Uh oh!

0xc0170 commented Sep 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deepikabhavnani commented Sep 7, 2017

Uh oh!

deepikabhavnani commented Sep 12, 2017

Uh oh!

c1728p9 commented Sep 14, 2017

Uh oh!

c1728p9 commented Sep 14, 2017

Uh oh!

mbed-bot commented Sep 14, 2017

Result: FAILURE

Output

Uh oh!

deepikabhavnani commented Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deepikabhavnani commented Sep 14, 2017

Uh oh!

mbed-bot commented Sep 15, 2017

Result: FAILURE

Output

Uh oh!

deepikabhavnani commented Sep 15, 2017

Uh oh!

mbed-bot commented Sep 15, 2017

Result: SUCCESS

Output

0xc0170 commented Sep 1, 2017 •

edited

Loading

deepikabhavnani commented Sep 14, 2017 •

edited

Loading