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

Statically allocate ARMCC required mutex objects #5687

Merged
merged 1 commit into from Dec 20, 2017

Conversation

Projects
None yet
7 participants
@SenRamakri
Contributor

SenRamakri commented Dec 11, 2017

This is fix for - #5664
Previously we used OS_MUTEX_NUM define to pre-allocate Mutex objects for ARMCC libs. But that doesn't guarantee that they will be avaliable for ARMCC usage always as any other client can grab them before ARMCC lib requires that to be used/initialized. This change is to statically allocate memory for Mutex objects for ARMCC to ensure ARMCC have them when it needs.

@0xc0170 0xc0170 requested review from bulislaw and geky Dec 12, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Dec 12, 2017

@TaniaMirzin Can you test if this resolves the issue you reported?

@TaniaMirzin

This comment has been minimized.

TaniaMirzin commented Dec 12, 2017

@0xc0170
@SenRamakri
I already checked it, Senthil is this not the same fix i already checked?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Dec 12, 2017

@TaniaMirzin Not sure which fix you are referencing, as this one is changing what is currently on master. Adding _static_mutexes_mem.

The fix looks good (we should provide cb_mem and its size).

If you look at CMSIS_5, they provided a new macro to disable multithreaded clib impl that is currently in there only for ARMCC. Reference: https://github.com/ARM-software/CMSIS_5/blob/bb78a8aa71384f2c7d909570d88d3cce3467ffe1/CMSIS/RTOS2/RTX/Source/rtx_lib.c#L566. If we define the macro to disable their strong definitions, we are still missing some functions ( for instance __user_perthread_libspace ).
We should provide it, and have this defined RTX_NO_MULTITHREAD_CLIB.

All this because ARMCC does not guarantee how many locks C lib needs? (as IAR does via __MAX_LOCK, we could then use the same what we do for IAR clib locks and not to have this static + heap hacks in here). Not clear with this one.

@SenRamakri

This comment has been minimized.

Contributor

SenRamakri commented Dec 12, 2017

@TaniaMirzin - Hi Tania, this is the same fix you already verified. @0xc0170 - Martin, Tania verified this fix already.

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Dec 12, 2017

@0xc0170

LGTM

@SenRamakri Please note my comment above, about missing userthread lib space, we should track that separately.

@geky

geky approved these changes Dec 12, 2017

Looks great 👍

@geky

This comment has been minimized.

Member

geky commented Dec 12, 2017

All this because ARMCC does not guarantee how many locks C lib needs?

The issue is that ARMCC is using this same API to create a mutex per FILE, and the user can create an unlimited number of FILEs.

I'm not exactly sure how the other toolchains handle this (@c1728p9?). It looks like they might have separate ties for malloc mutexes vs other stdlib mutexes.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Dec 12, 2017

I'm not exactly sure how the other toolchains handle this (@c1728p9?). It looks like they might have separate ties for malloc mutexes vs other stdlib mutexes.

IAR does that is correct.

IAR locks they provide two defines _MAX_LOCK, _FOPEN_MAX. First is defined by C lib, and cant be changed. The second we define, 10 file locks (this is valid only if full lib is used, and its our case, we might missing there a check for this). Not certain why we do not align IAR and ARMCC (in ARMCC we would define similar, 8 would be max_lock, and fopen same as for IAR, 10, thus 18 mutexes provided by default ?

I can create an issue for this, as this fix here is correct, just that during looking at the code why in the first place we have this misalignment with CMSIS, and how we can get our fixes there.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Dec 13, 2017

/morph build

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Dec 15, 2017

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Dec 15, 2017

Build : SUCCESS

Build number : 707
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5687/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

@mbed-ci

This comment has been minimized.

@kegilbert

This comment has been minimized.

Contributor

kegilbert commented Dec 15, 2017

/morph export-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Dec 18, 2017

@bulislaw

This comment has been minimized.

Member

bulislaw commented Dec 18, 2017

Well, the ARMCC doesn't provide any macros like that, but we could simplify the whole thing by setting it on Mbed OS level.

@0xc0170 0xc0170 merged commit a762e7a into ARMmbed:master Dec 20, 2017

10 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/events Local events testing has passed
Details
travis-ci/littlefs Local littlefs testing has passed
Details
travis-ci/mbed2 Local mbed2 testing has passed
Details
travis-ci/tools Local tools testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment