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

Add ThisThread namespace and deprecate static Thread methods #7872

Merged
merged 3 commits into from Sep 2, 2018

Conversation

Projects
None yet
8 participants
@kjbracey-arm
Contributor

kjbracey-arm commented Aug 23, 2018

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.


/** Returns the Thread Flags currently set for the currently running thread.
@return current thread flags or 0 if not in a valid thread.
@note You may call this function from ISR context.

This comment has been minimized.

@pan-

pan- Aug 23, 2018

Member

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.

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Aug 27, 2018

Contributor

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.

*/
namespace ThisThread {
/** Clears the specified Thread Flags of the currently running thread.
@param flags specifies the thread flags of the thread that should be cleared.

This comment has been minimized.

@pan-

pan- Aug 23, 2018

Member

of the thread may be redundant.

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Aug 27, 2018

Contributor

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.

#include "mbed_assert.h"

namespace rtos {

This comment has been minimized.

@pan-

pan- Aug 23, 2018

Member

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

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Aug 27, 2018

Contributor

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?

@cmonr cmonr requested a review from 0xc0170 Aug 24, 2018

@cmonr cmonr added the needs: review label Aug 24, 2018

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-core Aug 24, 2018

@deepikabhavnani

Looks good 👍

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 27, 2018

Please review the astyle job (fix them).

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Aug 27, 2018

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.

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:thisthread branch from ff9bf91 to c10d167 Aug 27, 2018

@kjbracey-arm kjbracey-arm changed the title from Add ThisThread class and deprecate static Thread methods to Add ThisThread namespace and deprecate static Thread methods Aug 27, 2018

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:thisthread branch 3 times, most recently from 3d5baa7 to 37433b0 Aug 27, 2018

@pan-

pan- approved these changes Aug 27, 2018

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 cmonr added needs: CI and removed needs: review labels Aug 27, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 27, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Aug 27, 2018

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

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:thisthread branch from 37433b0 to 0ddd1d9 Aug 28, 2018

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Aug 28, 2018

Broken testcase corrected.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 29, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Aug 29, 2018

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

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Aug 31, 2018

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

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 31, 2018

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

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 2, 2018

/morph test

@mbed-ci

This comment has been minimized.

@cmonr cmonr merged commit 48232be into ARMmbed:master Sep 2, 2018

14 checks passed

ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed , RTOS ROM(+0.05%) RAM(+0.05%)
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/cloud_client_smoke_test Test job was successful
Details
travis-ci/astyle Passed, 551 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10368 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/tools-py2.7 Local tools-py2.7 testing has passed
Details

@0xc0170 0xc0170 removed the ready for merge label Sep 2, 2018

@mattbrown015

This comment has been minimized.

Contributor

mattbrown015 commented Sep 3, 2018

(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-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Sep 4, 2018

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

@kjbracey-arm kjbracey-arm deleted the kjbracey-arm:thisthread branch Sep 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment