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

Remove use of internal RTX types #4776

Merged
merged 1 commit into from Sep 6, 2017

Conversation

Projects
None yet
8 participants
@c1728p9
Contributor

c1728p9 commented Jul 18, 2017

Make calls to cmsis-os to get thread state, stack size, and max stack usage rather than accessing internal RTX data directly. Wrap the code which has no public RTX function in TARGET_CORTEX_M since this needs to be defined for RTX to be included.

@c1728p9 c1728p9 requested a review from bulislaw Jul 18, 2017

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Jul 18, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Jul 19, 2017

Result: FAILURE

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

/morph test

Output

mbed Build Number: 848

Test failed!

rtos/Thread.cpp Outdated
@@ -238,10 +237,12 @@ uint32_t Thread::free_stack() {
uint32_t size = 0;
_mutex.lock();
#if defined(TARGET_CORTEX_M)

This comment has been minimized.

@bulislaw

bulislaw Jul 19, 2017

Member

Why are we adding that? I would hope the new Cortex A support in RTX is not that different

This comment has been minimized.

@c1728p9

c1728p9 Jul 19, 2017

Contributor

I'll add Cortex A to this as well.

rtos/Thread.cpp Outdated
@@ -168,7 +168,7 @@ Thread::State Thread::get_state() {
_mutex.lock();
if (_tid != NULL) {
state = _obj_mem.state;
state = osThreadGetState(_tid);

This comment has been minimized.

@bulislaw

bulislaw Jul 19, 2017

Member

That's possibly incorrect, if we want to preserve compatibility with pre CMSIS5 code, the osThreadGetState compresses statuses to short list, RTOS C++ API have quite extensive list. I personally agree we don't need all this detailed states like ('waiting for semaphore' as opposed to 'Waiting') but that's what we used to return.

This comment has been minimized.

@c1728p9

c1728p9 Jul 19, 2017

Contributor

I can revert this back to what it was. Just let me know what you prefer.

This comment has been minimized.

@0xc0170

0xc0170 Aug 31, 2017

Member

To be backward compatible if there is no strong reason to break this. In this case it is not?

rtos/Thread.cpp Outdated
while (((uint32_t *)(thread->stack_mem))[high_mark] == 0xE25A2EA5)
high_mark++;
size = thread->stack_size - (high_mark * sizeof(uint32_t));
size = osThreadGetStackSize(_tid) - osThreadGetStackSpace(_tid);

This comment has been minimized.

@bulislaw

bulislaw Jul 19, 2017

Member

to use osGetStackSpace we need to enable OS_STACK_WATERMARK, right now we only do it conditionally.

This comment has been minimized.

@c1728p9

c1728p9 Jul 19, 2017

Contributor

I didn't realize that, thanks for pointing it out. I think this code will be ok since osThreadGetStackSpace isn't compiled out when OS_STACK_WATERMARK isn't set, it just returns 0. Regardless, if stack watermarking isn't turned out then this code won't work as expected, since the stack won't be filled with 0xE25A2EA5.

This comment has been minimized.

@bulislaw

bulislaw Jul 20, 2017

Member

I would say we should either leave it the way it was or define OS_STACK_WATERMARK and change the code to fit it (eg not fill the stack with our pattern and let the RTX to use their 0xCCCCCCCCU).

This comment has been minimized.

@c1728p9

c1728p9 Jul 20, 2017

Contributor

Ah, I forgot there were two watermarking schemes. So the existing code would actually work when OS_STACK_WATERMARK isn't set. Either way works for me. Defining OS_STACK_WATERMARK seems like it might unify the code more, so that would be my preference.

This comment has been minimized.

@bulislaw

bulislaw Jul 20, 2017

Member

Agreed

@c1728p9 c1728p9 force-pushed the c1728p9:rtx_usage branch Jul 19, 2017

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Jul 19, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Jul 19, 2017

Result: FAILURE

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

/morph test

Output

mbed Build Number: 851

Test failed!

@c1728p9 c1728p9 force-pushed the c1728p9:rtx_usage branch Jul 19, 2017

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Jul 19, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Jul 19, 2017

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 853

All builds and test passed!

rtos/TARGET_CORTEX/rtx5/mbed_rtx_conf.h Outdated
@@ -24,6 +24,11 @@
#include "mbed_rtx.h"
/** Any access to RTX5 specific data structures used in common code should be wrapped in ifdef OS_BACKEND_RTX5 */

This comment has been minimized.

@bulislaw

bulislaw Jul 20, 2017

Member

I don't get why we have it this way. I would say it's fair enough to try to be 'generic' and let people replace RTX with other RTOSs as long as they implement CMSIS-RTOS2 API. But why do you enable it for CORTEX A and M? In any case that's the only HW we support. We could just define this flag in mbed_rtx_conf.h without checking the cores as it's RTX config and I would expect it should be provided per implementation (together with the storage and handlers files). Also I'm a big fan of mbed flags starting with mbed, so maybe something like MBED_OS_BACKEND_RTX5 for clarity.

@c1728p9 c1728p9 added needs: work and removed needs: review labels Jul 20, 2017

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Jul 20, 2017

This will be updated once #4786 has been merged.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jul 27, 2017

@c1728p9 Does this really need #4786 to go in first?

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Jul 27, 2017

The only dependency is the TARGET_CORTEX change.

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Jul 27, 2017

Updated this PR to include the TARGET_CORTEX directory addition. Also, expanded the scope of this PR to remove the use of internal RTX structures everywhere in the codebase.

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Jul 27, 2017

/morph test

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jul 27, 2017

@c1728p9 that change does make more sense here.

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Jul 27, 2017

retest uvisor

@c1728p9 c1728p9 force-pushed the c1728p9:rtx_usage branch Jul 27, 2017

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Jul 27, 2017

/morph test

@c1728p9 c1728p9 force-pushed the c1728p9:rtx_usage branch Sep 1, 2017

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Sep 1, 2017

I created a PR #5003 for the directory restructuring and rebased this to master.

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Sep 1, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 1, 2017

Result: FAILURE

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

/morph test

Output

mbed Build Number: 1149

Test failed!

features/FEATURE_UVISOR/source/rtx/box_init.c Outdated
@@ -29,7 +29,7 @@
extern void SVC_Handler(void);
extern void PendSV_Handler(void);
extern void SysTick_Handler(void);
extern uint32_t svcRtxKernelLock(void);

This comment has been minimized.

@bulislaw

bulislaw Sep 1, 2017

Member

That's going to be overwritten by next uvisor drop.

This comment has been minimized.

@0xc0170

0xc0170 Sep 1, 2017

Member

It was merged by uvisor, the new update contains this, its ready for merge: https://github.com/ARMmbed/mbed-os/pull/4907/files . This will be rebased once it gets in

This comment has been minimized.

@Patater

Patater Sep 1, 2017

Contributor

This PR shouldn't do the update, but should use the uVisor v0.30.0 changes.

Does this PR need to raise the priority of PR #4907? We thought the v0.30.0 PR was optional for 5.6, but maybe because this PR depends on it, the uVisor PR must go into 5.6.

This comment has been minimized.

@0xc0170

0xc0170 Sep 1, 2017

Member

Thanks @Patater. If both gets in, it would be nice. This is nice to have.
We will wait for uvisor to get in with the change, and rebase this one afterwards.

@bulislaw

LGTM

@0xc0170

0xc0170 approved these changes Sep 1, 2017

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Sep 1, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 1, 2017

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 1156

All builds and test passed!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 4, 2017

As marked above (#4776 (comment)), waiting for uvisor update.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 4, 2017

@c1728p9 Please rebase, uvisor PR update was merged to master

Remove use of internal RTX types
Make calls to cmsis-os to get thread state, stack size, and max stack
usage rather than accessing internal RTX data directly. Wrap RTX5
specific code in OS_BACKEND_RTX5.

Also refactor the code to use mbed types rather than RTX types:
os_timer_t -> mbed_rtos_storage_timer_t
os_event_flags_t -> mbed_rtos_storage_event_flags_t
osRtxMutex_t -> mbed_rtos_storage_thread_t

@c1728p9 c1728p9 force-pushed the c1728p9:rtx_usage branch to b2384b1 Sep 4, 2017

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Sep 4, 2017

Rebased and removed uvisor changes to rtx_box_index.h.

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Sep 4, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 5, 2017

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 1215

All builds and test passed!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 6, 2017

Ready for intergration.

@theotherjimmy theotherjimmy merged commit 1244802 into ARMmbed:master Sep 6, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
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