Add ThisThread namespace and deprecate static Thread methods

[kjbracey]

Description

The static methods of Thread have repeatedly caused confusion - while it's very clear in C that osThreadGetId() must be returning the current thread ID, as it takes no parameters, it's not at all obvious that thread->gettid() is static and will return the current thread's ID, not thread's

Remove the confusion by

• deprecating the existing static methods,
• adding updated replacements in namespace ThisThread and namespace Kernel,
• adding a new non-static Thread::get_id().

The distinction between ThisThread::get_id() and thread->get_id() is clear, and they cannot be confused.

Syntax and naming follows C++11, which has namespace std::this_thread and corresponding methods.

Most visible change to typical code is that Thread::wait() (which people often use because of problems with mbed::wait()) is deprecated in favour of ThisThread::sleep_for().

Effectively fixes #1831

Pull request type

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

[pan-]

Looks very good. I'm a bit worried about naming conventions for namespace: on one hand we got mbed, event and rtos and on the other we have Kernel and ThisThread.

[pan-]

That's a bit strange to call this function from ISR; even if it is legal.
More generally this pack of function are not meant to be called from ISR context.

[kjbracey]

Actually it isn't legal, according to docs.

The RTX implementation of osThreadFlagsGet does return 0 without error if in ISR context, but the CMSIS-RTOS 2.1.2 documentation says it can't be called from interrupt context. Will modify.

[pan-]

of the thread may be redundant.

[kjbracey]

Made it "flags of the thread", matching CMSIS-RTOS 2 doc wording. Still feels a bit wonky for something acting only on current thread, but it matches the wording of Thread::flags_set / osThreadFlagsSet, either intentionally or by copy-and-paste.

[pan-]

Would it be possible to enclose function implementations into a namespace. With the prefix it looks like a member function implementation.

[kjbracey]

That was intentional because that's how I was mainly expecting them to be used - as if they were static member functions. So people are likely to be searching for ThisThread::sleep_for, having seen it in code.

Does that make sense?

[deepikabhavnani]

Looks good 👍

[0xc0170]

Please review the astyle job (fix them).

[kjbracey]

Looks very good. I'm a bit worried about naming conventions for namespace: on one hand we got mbed, event and rtos and on the other we have Kernel and ThisThread.

I think there's a reasonable distinction to be made. mbed, rtos and event are namespaces in the traditional sense, like std or boost, covering a complete system, although event is a tad small, and possibly should just have been in mbed.

Kernel and ThisThread are pseudo-static-classes (within a traditional namespace) covering a specific small API. As such, they're named to match the classes they slot in alongside.

C++11 has class std::thread and namespace std::this_thread, which are analogues sharing same-named functions or members - I've directly mirrored that with class rtos::Thread and namespace rtos::ThisThread.

I would say for these pseudo-static-classes the namespace is very much to be read as part of the API name, and they are normally expected to be called explicitly as Kernel::get_ms_count or ThisThread::get_id(), not via a using-declaration.

[pan-]

Thanks Kevin, it all makes sense. I've opened an issue in the docs. It would be nice to document in our guidelines what the pseudo-static-classes pattern is and what conventions it uses.

[cmonr]

/morph build

[mbed-ci]

Build : SUCCESS

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

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

[mbed-ci]

Exporter Build : SUCCESS

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

[mbed-ci]

Test : FAILURE

Build number : 2679
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7872/2679

[kjbracey]

Broken testcase corrected.

[0xc0170]

/morph build

[mbed-ci]

Build : SUCCESS

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

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

[mbed-ci]

Exporter Build : SUCCESS

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

[mbed-ci]

Test : FAILURE

Build number : 2698
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7872/2698

[kjbracey]

Seems to be a spurious failure unconnected to PR - passed in previous run.

[cmonr]

Correct. We're currently running Testing CI on other PRs. Will restart this PR when able to.

[cmonr]

/morph test

[mbed-ci]

Test : SUCCESS

Build number : 2743
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7872/2743

[mattbrown015]

(I believe) This changed has created some warnings.

Compile [ 60.4%]: mbed_wait_api_rtos.cpp
[Warning] mbed_wait_api_rtos.cpp@45,17: 'static osStatus rtos::Thread::wait(uint32_t)' is deprecated: Static methods only affecting current thread cause confusion. Replaced by ThisThread::sleep_for. [since mbed-os-5.10] [-Wdeprecated-declarations]
[Warning] mbed_wait_api_rtos.cpp@45,41: 'static osStatus rtos::Thread::wait(uint32_t)' is deprecated: Static methods only affecting current thread cause confusion. Replaced by ThisThread::sleep_for. [since mbed-os-5.10] [-Wdeprecated-declarations]

Is someone on the case?

It has also created warnings in the LoRa radio drivers but I guess no one here is interested in those! I'll open an issue there.

[kjbracey]

Thanks for the poke. Created PR #7980 to clean up warnings.