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

ThisThread get_name() #8961

Merged
merged 1 commit into from Dec 14, 2018

Conversation

Projects
None yet
7 participants
@marcemmers
Copy link
Contributor

commented Dec 4, 2018

Description

Added function to get the name of the current thread to ThisThread.
Comes in handy when debugging and doesn't have any impact if not used.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[X] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

@0xc0170 0xc0170 requested review from kjbracey-arm and ARMmbed/mbed-os-core Dec 4, 2018

@kjbracey-arm
Copy link
Contributor

left a comment

Not totally sure about this, but I guess it's useful, and there's no other way to get the info other than going to the CMSIS-RTOS APIs.

Show resolved Hide resolved rtos/ThisThread.h
Show resolved Hide resolved rtos/ThisThread.cpp Outdated
rtos/ThisThread.h Outdated
@note You may call this function from ISR context.
*/
osThreadId_t get_id();

/** Get the thread name of the current running thread.
@return thread name pointer or NULL in case of error or in ISR context.

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Dec 4, 2018

Contributor

Could also be NULL if the thread has no name. (Possible in CMSIS-RTOS, even though the Thread object likes to put in "application_unnamed_thread")

rtos/ThisThread.h Outdated
@@ -174,7 +174,7 @@ void sleep_until(uint64_t millisec);
void yield();

/** Get the thread id of the current running thread.
@return thread ID for reference by other functions or NULL in case of error or in ISR context.
@return thread ID for reference by other functions or NULL in case of error.

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Dec 4, 2018

Contributor

You may be right here - I think it actually returns current foreground thread if called from ISR.

Wasn't inconsistent though - documenting it as returning NULL from ISR is perfectly logical.

Saying it returns NULL from ISR but then saying you can't call it from ISR would be inconsistent.

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Dec 4, 2018

Contributor

Although the new documented behaviour is maybe what you get from RTX osThreadGetId, I don't think it's actually what we would want from ThisThread::get_id, given the name.

Fixing implementation to match documented behaviour would be better I think. Or specifying it can't be called from ISR.

It was supposed to be a drop-in replacement for Thread::gettid(), which never specified ISR behaviour, but said it worked. Hmm.

@marcemmers

This comment has been minimized.

Copy link
Contributor Author

commented Dec 4, 2018

I misunderstood the "Cannot call" then, thinking it is possible to call but wouldn't function. Instead the functions that say they cannot be called from ISR crash?

It seems to me that both osThreadGetId and osThreadGetName can be called from ISR context. Only osThreadGetName returns NULL even if the ID is valid in ISR context.

It does make sense to block both get_id and get_name from being called in ISR context because there is no ThisThread if you are in ISR context. All other functions in the namespace are already blocked from ISR context so why should get_id be allowed?

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2018

I misunderstood the "Cannot call" then, thinking it is possible to call but wouldn't function. Instead the functions that say they cannot be called from ISR crash?

Often, yes, sometimes only enforced in a debug build. Or behaviour may be undefined.

It seems to me that both osThreadGetId and osThreadGetName can be called from ISR context. Only osThreadGetName returns NULL even if the ID is valid in ISR context.

CMSIS-RTOS 2 docs explicitly say osThreadGetName can't be. They say osThreadGetId can be, but don't say what it does. I thought it returned NULL, but current implementation seems to return running foreground thread.

It does make sense to block both get_id and get_name from being called in ISR context because there is no ThisThread if you are in ISR context. All other functions in the namespace are already blocked from ISR context so why should get_id be allowed?

I wrote that, and I think I allowed it because

  • Thread::gettid() allowed it, and this was supposed to replace it
  • I thought I was just documenting existing behaviour
  • I thought returning NULL to say "you're not in a thread" was potentially useful, and maybe a way of deciding what to do on that basis.

If it doesn't actually return NULL in that case, it no longer makes sense to be ThisThread::get_id - it would be ForegroundThread::get_id. So I agree with saying it's not allowed in ISR. We could conceivably put in a local "are we in IRQ context" test, but I don't really want to go there.

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2018

Note that error cases in RTX call EvrRtx... functions. Those are trapped and trigger mbed_error in many cases, so osThreadGetName from ISR may well die and not return NULL.

(Maybe debug build only?)

@marcemmers

This comment has been minimized.

Copy link
Contributor Author

commented Dec 4, 2018

I was looking at the cmsis_os2.h file instead of at the real docs. You're right about the osThreadGetName and I will change my comments.

Noticed the EvrRtx call but didn't know that could lead to an mbed_error call, thought it was only meant to record some events.

So the question remains, what to do with the get_id.
Should I leave it be so it can be looked at later on? Should i note that it cannot be called from ISR but not enforce it?

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2018

I'd leave the get_id call to another PR. Needs separate discussion. I'd initially suggest saying it can't be called from ISR, and not enforce, so just docs change.

@0xc0170 0xc0170 added needs: work and removed needs: review labels Dec 4, 2018

Show resolved Hide resolved rtos/ThisThread.cpp Outdated
@cmonr

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2018

@marcemmers A small astyle nit and we can get CI started.

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2018

I still want to leave the get_id change out - it needs more work/discussion. I'm fine with the get_name addition once the formatting change is done. Also, squash the PR.

@marcemmers marcemmers force-pushed the marcemmers:this-thread branch to 2f792a9 Dec 5, 2018

@0xc0170

0xc0170 approved these changes Dec 5, 2018

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Dec 5, 2018

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2018

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Dec 5, 2018

Test run: FAILED

Summary: 6 of 7 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_build-IAR
  • jenkins-ci/mbed-os-ci_mbed2-build-GCC_ARM
  • jenkins-ci/mbed-os-ci_mbed2-build-IAR
  • jenkins-ci/mbed-os-ci_mbed2-build-ARM
@alekla01

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2018

CI restarted

@mbed-ci

This comment has been minimized.

Copy link

commented Dec 5, 2018

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 2
Build artifacts

@cmonr cmonr added do not merge and removed needs: CI labels Dec 5, 2018

@cmonr cmonr removed the do not merge label Dec 14, 2018

@cmonr cmonr merged commit 463a453 into ARMmbed:master Dec 14, 2018

21 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Passed, 0 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9140 cycles (+40 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Dec 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.