Skip to content

CriticalSectionLock class improvement #5420

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

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions mbed.h
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@
#include "platform/FileSystemHandle.h"
#include "platform/FileHandle.h"
#include "platform/DirHandle.h"
#include "platform/ScopedLock.h"
#include "platform/CriticalSectionLock.h"
#include "platform/DeepSleepLock.h"

Expand Down
99 changes: 82 additions & 17 deletions platform/CriticalSectionLock.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#define MBED_CRITICALSECTIONLOCK_H

#include "platform/mbed_critical.h"
#include "platform/ScopedLock.h"

namespace mbed {

Expand All @@ -30,48 +31,112 @@ namespace mbed {
*/

/** RAII object for disabling, then restoring, interrupt state
* Usage:
* @code
*
* void f() {
* // some code here
* {
* CriticalSectionLock lock;
* // Code in this block will run with interrupts disabled
* }
* // interrupts will be restored to their previous state
* }
* @endcode
*/
*
* @deprecated
* The CriticalSectionLock has been superseded by the ScopedCriticalSectionLock
*
* Usage:
* @code
*
* void f() {
* // some code here
* {
* CriticalSectionLock lock;
* // Code in this block will run with interrupts disabled
* }
* // interrupts will be restored to their previous state
* }
* @endcode
*/
class CriticalSectionLock {
public:
CriticalSectionLock()
MBED_DEPRECATED_SINCE("mbed-os-5.7",
"The CriticalSectionLock has been superseded by the ScopedCriticalSectionLock.")
CriticalSectionLock()
{
core_util_critical_section_enter();
}

~CriticalSectionLock()
~CriticalSectionLock()
{
core_util_critical_section_exit();
}

/** Mark the start of a critical section
*
*
*/
void lock()
{
core_util_critical_section_enter();
}

/** Mark the end of a critical section
*
*
*/
void unlock()
{
core_util_critical_section_exit();
}
};


/** Class for disabling and restoring interrupt state
* Usage:
* @code
*
* void foo() {
* // some code here
* CriticalSectionLock::lock();
* // Code in this block will run with interrupts disabled
* CriticalSectionLock::unlock();
* // interrupts will be restored to their previous state
* }
* @endcode
*/
class CriticalSection : private NonCopyable<CriticalSection> {
public:
/** Mark the start of a critical section
*/
static void lock()
{
core_util_critical_section_enter();
}
/** Mark the end of a critical section
*/
static void unlock()
{
core_util_critical_section_exit();
}
};

/**@}*/

/**@}*/

/** RAII object for disabling, then restoring, interrupt state
*
* Usage:
* @code
*
* void foo() {
* // some code here
* {
* ScopedCriticalSectionLock lock;
* // Code in this block will run with interrupts disabled
* }
* // interrupts will be restored to their previous state
* }
* @endcode
*/
class ScopedCriticalSectionLock : private CriticalSection, private ScopedLock<CriticalSection> {
// Note: inherit from CriticalSection and ScopedLock to use empty base optimisation
public:
ScopedCriticalSectionLock() :
CriticalSection(),
ScopedLock<CriticalSection>(static_cast<CriticalSection&>(*this)) {
}
};

/**@}*/

/**@}*/
Expand Down
87 changes: 87 additions & 0 deletions platform/ScopedLock.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
/* mbed Microcontroller Library
* Copyright (c) 2017 ARM Limited
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
#ifndef MBED_SCOPEDLOCK_H
#define MBED_SCOPEDLOCK_H

#include "platform/NonCopyable.h"

namespace mbed {

/** \addtogroup platform */
/** @{*/
/**
* \defgroup platform_ScopedLock ScopedLock functions
* @{
*/

/** RAII-style mechanism for owning a lock of Lockable object for the duration of a scoped block
*
* @tparam Lockable The type implementing BasicLockable concept
*
* @note For type Lockable to be BasicLockable the following conditions have to be satisfied:
* - has public member function @a lock which blocks until a lock can be obtained for the current execution context
* - has public member function @a unlock which releases the lock
*
* Usage:
*
* Example with rtos::Mutex
*
* @code
* void foo(Mutex &m) {
* ScopedLock<Mutex> lock(m);
* // Code in this block will be protected by Mutex lock
* }
* @endcode
*
*
* More generic example
*
* @code
* template<typename Lockable>
* void foo(Lockable& lockable) {
* ScopedLock<Lockable> lock(lockable);
* // Code in this block will run under lock
* }
* @endcode
*/
template <typename Lockable>
class ScopedLock : private NonCopyable<ScopedLock<Lockable> > {
public:
/** Locks given locable object
*
* @param lockable reference to the instance of Lockable object
* @note lockable object should outlive the ScopedLock object
*/
ScopedLock(Lockable& lockable): _lockable(lockable)
{
_lockable.lock();
}

~ScopedLock()
{
_lockable.unlock();
}
private:
Lockable& _lockable;
};

/**@}*/

/**@}*/

} // embed

#endif // MBED_SCOPEDLOCK_H
17 changes: 16 additions & 1 deletion rtos/Mutex.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,30 @@
#include "mbed_rtos_storage.h"

#include "platform/NonCopyable.h"
#include "platform/ScopedLock.h"

namespace rtos {
/** \addtogroup rtos */
/** @{*/

class Mutex;
/** Typedef for the mutex lock
*
* Usage:
* @code
* void foo(Mutex &m) {
* ScopedMutexLock lock(m);
* // Code in this block will be protected by Mutex lock
* }
* @endcode
*/
typedef mbed::ScopedLock<Mutex> ScopedMutexLock;
Copy link
Contributor

@kjbracey kjbracey Nov 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only concern for this one is that Mutex::lock() returns CMSIS-RTOS errors, which ScopedLock will ignore. I'd feel happier if lock was a void that asserted or mbed_errored on CMSIS-RTOS errors.

But I guess we're largely okay because most of those errors will be caught by our EvrRtxMutexError handler in a debug build - a lot of our RTOS functions are now effectively limited in what errors they can return, despite their documentation.

So I think this is probably fine - just interested in others' thoughts.

(One "missed" error path I think I can see is deleting a Mutex while someone is blocking on it - they'll wake with osErrorResource, but that doesn't go through EvrRtxMutexError.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add a partial specialization of ScopedLock that asserts on lock and unlock if they return an osStatus. Would that work for you ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would work. I'd prefer a variant of Mutex with revised API, or revising Mutex itself, but a specialisation of ScopedLock addresses it easily here for now, and means any transformations from manual locking to ScopedLock will get this improvement.


/**
* \defgroup rtos_Mutex Mutex class
* @{
*/

/** The Mutex class is used to synchronize the execution of threads.
This is for example used to protect access to a shared resource.

Expand Down