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

Refactor OS::Mutex in CMake selection #2790

Merged
merged 24 commits into from
Aug 2, 2024
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
b388062
Initial implementation
thomas-bc Jun 6, 2024
468ad0d
Allow copy constructor, use POINTER_CAST Posix handle
thomas-bc Jun 11, 2024
57fd4b3
Add take/release and correct PosixMutexHandle type
thomas-bc Jun 13, 2024
32c59e4
Test initial pass
thomas-bc Jun 24, 2024
71fe8fb
StubTest implementation
thomas-bc Jun 24, 2024
185297d
Initial Posix rule-based tests
thomas-bc Jun 24, 2024
c20f231
More rule-based tests
thomas-bc Jun 24, 2024
5764797
Move DeleteLockedMutex to Posix tests
thomas-bc Jun 24, 2024
6f74f9c
Merge branch 'nasa:devel' into cmake-os-mutex
thomas-bc Jun 25, 2024
22fa2ca
Fix some Cmake stuff
thomas-bc Jun 26, 2024
762424d
Models, errors and more
thomas-bc Jun 26, 2024
716068d
Add models to UT_MOD_DEPS
thomas-bc Jun 26, 2024
ebb9747
Reset Os/Task setup to prior state for side-effect free tests
thomas-bc Jun 27, 2024
718d025
remove legacy stuff
thomas-bc Jun 27, 2024
1a9988a
add condition to unlocking mutex at destruction
thomas-bc Jun 27, 2024
5b51689
rename GET_HANDLE for spelling
thomas-bc Jul 1, 2024
206f067
Release unique_ptr before delete
thomas-bc Jul 1, 2024
cc1e73a
Add Posix check - data protection
thomas-bc Jul 2, 2024
005f152
Merge branch 'devel' into cmake-os-mutex
thomas-bc Jul 2, 2024
59d8c71
Fix comments and includes
thomas-bc Jul 2, 2024
55ec7d4
Add review changes
thomas-bc Aug 2, 2024
7a27ec9
Require Os/Task implementation and add Fw/Time link dependencies
thomas-bc Aug 2, 2024
22580bc
Revert "Require Os/Task implementation and add Fw/Time link dependenc…
thomas-bc Aug 2, 2024
9f1878a
Revert CMake DefaultFile and DefaultTask changes
thomas-bc Aug 2, 2024
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 .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ Tester.*
!**/test/ut/GTestBase.*
!**/test/ut/TesterBase.*
!**/test/ut/Tester.*
seed-history

*.sconsign.dblite
*.class
Expand Down
9 changes: 6 additions & 3 deletions Os/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ set(SOURCE_FILES
# Refactored common files
"${CMAKE_CURRENT_LIST_DIR}/File.cpp"
"${CMAKE_CURRENT_LIST_DIR}/Task.cpp"
"${CMAKE_CURRENT_LIST_DIR}/Mutex.cpp"
)
# Check for default logger
if (NOT FPRIME_DISABLE_DEFAULT_LOGGER)
Expand All @@ -52,11 +53,9 @@ if (FPRIME_USE_POSIX)
"${CMAKE_CURRENT_LIST_DIR}/Pthreads/BufferQueueCommon.cpp"
"${CMAKE_CURRENT_LIST_DIR}/Pthreads/PriorityBufferQueue.cpp"
"${CMAKE_CURRENT_LIST_DIR}/Pthreads/MaxHeap/MaxHeap.cpp"
# "${CMAKE_CURRENT_LIST_DIR}/Posix/Task.cpp"
"${CMAKE_CURRENT_LIST_DIR}/Linux/InterruptLock.cpp"
"${CMAKE_CURRENT_LIST_DIR}/Linux/WatchdogTimer.cpp"
"${CMAKE_CURRENT_LIST_DIR}/Posix/IntervalTimer.cpp"
"${CMAKE_CURRENT_LIST_DIR}/Posix/Mutex.cpp"
"${CMAKE_CURRENT_LIST_DIR}/Linux/FileSystem.cpp"
"${CMAKE_CURRENT_LIST_DIR}/Linux/Directory.cpp"
)
Expand Down Expand Up @@ -96,7 +95,11 @@ if (FPRIME_USE_BAREMETAL_SCHEDULER)
list(APPEND SOURCE_FILES "${CMAKE_CURRENT_LIST_DIR}/Baremetal/SystemResources.cpp")
endif()
register_fprime_module()

require_fprime_implementation(Os/File)
# require_fprime_implementation(Os/Task) # should be added in
LeStarch marked this conversation as resolved.
Show resolved Hide resolved
require_fprime_implementation(Os/Mutex)


### UTS ### Note: 3 separate UTs registered here.
set(UT_SOURCE_FILES
Expand All @@ -110,7 +113,7 @@ set(UT_SOURCE_FILES
)
register_fprime_ut()
if (BUILD_TESTING)
foreach (TEST IN ITEMS StubFileTest PosixFileTest)
foreach (TEST IN ITEMS StubFileTest PosixFileTest StubMutexTest PosixMutexTest) # add? : StubTaskTest PosixTaskTest
if (TARGET "${TEST}")
add_dependencies("Os_ut_exe" "${TEST}")
endif()
Expand Down
1 change: 1 addition & 0 deletions Os/Models/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,6 @@
set(SOURCE_FILES
"${CMAKE_CURRENT_LIST_DIR}/File.fpp"
"${CMAKE_CURRENT_LIST_DIR}/Task.fpp"
"${CMAKE_CURRENT_LIST_DIR}/Mutex.fpp"
)
register_fprime_module()
11 changes: 11 additions & 0 deletions Os/Models/Models.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@
#include "Os/Models/FileStatusEnumAc.hpp"
#include "Os/Models/FileModeEnumAc.hpp"
#include "Os/Models/TaskStatusEnumAc.hpp"
#include "Os/Models/MutexStatusEnumAc.hpp"
#include "Os/File.hpp"
#include "Os/Task.hpp"
#include "Os/Mutex.hpp"

#ifndef OS_MODELS_MODELS_HPP
#define OS_MODELS_MODELS_HPP
Expand Down Expand Up @@ -79,4 +81,13 @@ static_assert(static_cast<Os::TaskStatus::T>(Os::Task::Status::ERROR_PERMISSION)
static_assert(static_cast<Os::TaskStatus::T>(Os::Task::Status::INVALID_STATE) == Os::TaskStatus::T::INVALID_STATE,
"Task status and FPP shadow enum do not match");

static_assert(static_cast<Os::MutexStatus::T>(Os::Mutex::Status::OP_OK) == Os::MutexStatus::T::OP_OK,
"Mutex status and FPP shadow enum do not match");
static_assert(static_cast<Os::MutexStatus::T>(Os::Mutex::Status::ERROR_BUSY) == Os::MutexStatus::T::ERROR_BUSY,
"Mutex status and FPP shadow enum do not match");
static_assert(static_cast<Os::MutexStatus::T>(Os::Mutex::Status::ERROR_DEADLOCK) == Os::MutexStatus::T::ERROR_DEADLOCK,
"Mutex status and FPP shadow enum do not match");
static_assert(static_cast<Os::MutexStatus::T>(Os::Mutex::Status::ERROR_OTHER) == Os::MutexStatus::T::ERROR_OTHER,
"Mutex status and FPP shadow enum do not match");

#endif // OS_MODELS_MODELS_HPP
14 changes: 14 additions & 0 deletions Os/Models/Mutex.fpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# ======================================================================
# \title Os/Models/Mutex.fpp
# \brief FPP type definitions for Os/Mutex.hpp concepts
# ======================================================================

module Os {
@ FPP shadow-enum representing Os::Mutex::Status
enum MutexStatus {
OP_OK, @< Operation was successful
ERROR_BUSY, @< Mutex is busy
ERROR_DEADLOCK, @< Deadlock condition detected
ERROR_OTHER @< All other errors
}
}
43 changes: 43 additions & 0 deletions Os/Mutex.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// ======================================================================
// \title Os/Mutex.cpp
// \brief common function implementation for Os::Mutex
// ======================================================================
#include <Fw/Types/Assert.hpp>
#include <Os/Mutex.hpp>

namespace Os {

Mutex::Mutex() : m_handle_storage(), m_delegate(*MutexInterface::getDelegate(m_handle_storage)) {
FW_ASSERT(&this->m_delegate == reinterpret_cast<MutexInterface*>(&this->m_handle_storage[0]));
}

Mutex::~Mutex() {
FW_ASSERT(&this->m_delegate == reinterpret_cast<MutexInterface*>(&this->m_handle_storage[0]));
m_delegate.~MutexInterface();
}

MutexHandle* Mutex::getHandle() {
FW_ASSERT(&this->m_delegate == reinterpret_cast<MutexInterface*>(&this->m_handle_storage[0]));
return this->m_delegate.getHandle();
}

void Mutex::lock() {
FW_ASSERT(&this->m_delegate == reinterpret_cast<MutexInterface*>(&this->m_handle_storage[0]));
this->m_delegate.lock();
}

void Mutex::unLock() {
FW_ASSERT(&this->m_delegate == reinterpret_cast<MutexInterface*>(&this->m_handle_storage[0]));
this->m_delegate.unLock();
}

Mutex::Status Mutex::take() {
FW_ASSERT(&this->m_delegate == reinterpret_cast<MutexInterface*>(&this->m_handle_storage[0]));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

implement take/release calling the others here

return this->m_delegate.take();
}

Mutex::Status Mutex::release() {
FW_ASSERT(&this->m_delegate == reinterpret_cast<MutexInterface*>(&this->m_handle_storage[0]));
return this->m_delegate.release();
}
} // namespace Os
74 changes: 61 additions & 13 deletions Os/Mutex.hpp
Original file line number Diff line number Diff line change
@@ -1,24 +1,72 @@
#ifndef _Mutex_hpp_
#define _Mutex_hpp_
// Add header
#ifndef Os_Mutex_hpp
#define Os_Mutex_hpp

#include <FpConfig.hpp>
#include <Os/Os.hpp>

namespace Os {

class Mutex {
public:
struct MutexHandle {};

Mutex(); //!< Constructor. Mutex is unlocked when created
virtual ~Mutex(); //!< Destructor
class MutexInterface {
// add enum with
public:
enum Status {
OP_OK, //!< Operation was successful
ERROR_BUSY, //!< Mutex is busy
ERROR_DEADLOCK, //!< Deadlock condition detected
ERROR_OTHER //!< All other errors
};

void lock(); //!< lock the mutex
void unLock(); //!< unlock the mutex
void unlock() { this->unLock(); } //!< alias for unLock to meet BasicLockable requirements
//! \brief default constructor
MutexInterface() = default;
Dismissed Show dismissed Hide dismissed

private:
//! \brief default virtual destructor
virtual ~MutexInterface() = default;

POINTER_CAST m_handle; //!< Stored handle to mutex
};
}
//! \brief copy constructor is forbidden
MutexInterface(const MutexInterface& other) = delete;

//! \brief assignment operator is forbidden
MutexInterface& operator=(const MutexInterface& other) = delete;

//! \brief return the underlying mutex handle (implementation specific)
//! \return internal mutex handle representation
virtual MutexHandle* getHandle() = 0;

//! \brief provide a pointer to a Mutex delegate object
static MutexInterface* getDelegate(HandleStorage& aligned_new_memory); // TODO

virtual Status take() = 0; //!< lock the mutex and get return status
virtual Status release() = 0; //!< unlock the mutex and get return status
virtual void lock() = 0; //!< lock the mutex
virtual void unLock() = 0; //!< unlock the mutex
};

class Mutex final : public MutexInterface {
public:
Mutex(); //!< Constructor. Mutex is unlocked when created
~Mutex() final; //!< Destructor

//! \brief return the underlying mutex handle (implementation specific)
//! \return internal mutex handle representation
MutexHandle* getHandle() override;

Status take() override; //!< lock the mutex and get return status
Status release() override; //!< unlock the mutex and get return status
void lock() override; //!< lock the mutex
void unLock() override; //!< unlock the mutex
void unlock() { this->unLock(); } //!< alias for unLock to meet BasicLockable requirements

Check notice

Code scanning / CodeQL

More than one statement per line Note

This line contains 3 statements; only one is allowed.

private:
// This section is used to store the implementation-defined mutex handle. To Os::Mutex and fprime, this type is
// opaque and thus normal allocation cannot be done. Instead, we allow the implementor to store then handle in
// the byte-array here and set `handle` to that address for storage.
//
alignas(FW_HANDLE_ALIGNMENT) HandleStorage m_handle_storage; //!< Mutex handle storage
MutexInterface& m_delegate; //!< Delegate for the real implementation
};
} // namespace Os

#endif
29 changes: 27 additions & 2 deletions Os/Posix/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ set(HEADER_FILES
"${CMAKE_CURRENT_LIST_DIR}/Task.hpp"
)

set(MOD_DEPS Os_Posix_Shared Fw/Time Os)
set(MOD_DEPS Os_Posix_Shared Fw/Time Os) # Os needs to be removed here?
register_fprime_module(Os_Task_Posix)
register_fprime_implementation(Os/Task Os_Task_Posix "${CMAKE_CURRENT_LIST_DIR}/DefaultTask.cpp")

Expand All @@ -65,4 +65,29 @@ set(UT_MOD_DEPS
STest
)
choose_fprime_implementation(Os/Task Os_Task_Posix)
register_fprime_ut(PosixTaskTest)
register_fprime_ut(PosixTaskTest)

#### Os/Mutex/Posix Section ####
set(SOURCE_FILES
"${CMAKE_CURRENT_LIST_DIR}/Mutex.cpp"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

correct - delete and make sure it's not in other modules as well

# "${CMAKE_CURRENT_LIST_DIR}/DefaultMutex.cpp"
)
set(HEADER_FILES
"${CMAKE_CURRENT_LIST_DIR}/Mutex.hpp"
)
set(MOD_DEPS Os_Posix_Shared)
register_fprime_module(Os_Mutex_Posix)
register_fprime_implementation(Os/Mutex Os_Mutex_Posix "${CMAKE_CURRENT_LIST_DIR}/DefaultMutex.cpp")

#### Os/Mutex/Posix Unit Tests ####
set(UT_SOURCE_FILES
"${CMAKE_CURRENT_LIST_DIR}/../test/ut/mutex/CommonTests.cpp"
"${CMAKE_CURRENT_LIST_DIR}/../test/ut/mutex/MutexRules.cpp"
"${CMAKE_CURRENT_LIST_DIR}/test/ut/PosixMutexTests.cpp"
)
set(UT_MOD_DEPS
Os
STest
)
choose_fprime_implementation(Os/Mutex Os_Mutex_Posix)
register_fprime_ut(PosixMutexTest)
17 changes: 17 additions & 0 deletions Os/Posix/DefaultMutex.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// ======================================================================
// \title Os/Posix/DefaultMutex.cpp
// \brief sets default Os::Mutex Posix implementation via linker
// ======================================================================
#include "Os/Posix/Mutex.hpp"
#include "Os/Delegate.hpp"
namespace Os {

//! \brief get a delegate for MutexInterface that intercepts calls for Posix
//! \param aligned_new_memory: aligned memory to fill
//! \return: pointer to delegate
MutexInterface *MutexInterface::getDelegate(HandleStorage& aligned_new_memory) {
return Os::Delegate::makeDelegate<MutexInterface, Os::Posix::Mutex::PosixMutex>(
aligned_new_memory
);
}
}
83 changes: 46 additions & 37 deletions Os/Posix/Mutex.cpp
Original file line number Diff line number Diff line change
@@ -1,51 +1,60 @@
#include <Os/Mutex.hpp>
#include <pthread.h>
// ======================================================================
// \title Os/Posix/Mutex.cpp
// \brief Posix implementation for Os::Mutex
// ======================================================================
#include <Os/Posix/Mutex.hpp>
#include <Os/Posix/error.hpp>
#include <Fw/Types/Assert.hpp>
#include <new>

namespace Os {
namespace Posix {
namespace Mutex {

Mutex::Mutex() {
pthread_mutex_t* handle = new(std::nothrow) pthread_mutex_t;
FW_ASSERT(handle != nullptr);
PosixMutex::PosixMutex() : Os::MutexInterface(), m_handle() {

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
// set attributes
pthread_mutexattr_t attribute;
pthread_mutexattr_init(&attribute);
github-advanced-security[bot] marked this conversation as resolved.
Fixed
Show resolved Hide resolved

// set attributes
pthread_mutexattr_t attr;
pthread_mutexattr_init(&attr);
// set to normal mutex type
PlatformIntType status = pthread_mutexattr_settype(&attribute, PTHREAD_MUTEX_NORMAL);
Fixed Show fixed Hide fixed
FW_ASSERT(status == 0, status);

NATIVE_INT_TYPE stat;
// set to error checking
// stat = pthread_mutexattr_settype(&attr,PTHREAD_MUTEX_ERRORCHECK);
// FW_ASSERT(stat == 0,stat);
// set to check for priority inheritance
status = pthread_mutexattr_setprotocol(&attribute, PTHREAD_PRIO_INHERIT);
FW_ASSERT(status == 0, status);

// set to normal mutex type
stat = pthread_mutexattr_settype(&attr,PTHREAD_MUTEX_NORMAL);
FW_ASSERT(stat == 0,stat);

// set to check for priority inheritance
stat = pthread_mutexattr_setprotocol(&attr,PTHREAD_PRIO_INHERIT);
FW_ASSERT(stat == 0,stat);
status = pthread_mutex_init(&this->m_handle.m_mutex_descriptor, &attribute);
FW_ASSERT(status == 0, status);
}

stat = pthread_mutex_init(handle,&attr);
FW_ASSERT(stat == 0,stat);
PosixMutex::~PosixMutex() {
PlatformIntType status = pthread_mutex_destroy(&this->m_handle.m_mutex_descriptor);

Check notice

Code scanning / CodeQL

Use of basic integral type Note

status uses the basic integral type int rather than a typedef with size and signedness.
FW_ASSERT(status == 0, status);
}

this->m_handle = reinterpret_cast<POINTER_CAST>(handle);
}
PosixMutex::Status PosixMutex::take() {
PlatformIntType status = pthread_mutex_lock(&this->m_handle.m_mutex_descriptor);

Check notice

Code scanning / CodeQL

Use of basic integral type Note

status uses the basic integral type int rather than a typedef with size and signedness.
return Os::Posix::posix_status_to_mutex_status(status);
}

Mutex::~Mutex() {
NATIVE_INT_TYPE stat = pthread_mutex_destroy(reinterpret_cast<pthread_mutex_t*>(this->m_handle));
FW_ASSERT(stat == 0,stat);
delete reinterpret_cast<pthread_mutex_t*>(this->m_handle);
}
PosixMutex::Status PosixMutex::release() {
PlatformIntType status = pthread_mutex_unlock(&this->m_handle.m_mutex_descriptor);

Check notice

Code scanning / CodeQL

Use of basic integral type Note

status uses the basic integral type int rather than a typedef with size and signedness.
return Os::Posix::posix_status_to_mutex_status(status);
}

void Mutex::lock() {
NATIVE_INT_TYPE stat = pthread_mutex_lock(reinterpret_cast<pthread_mutex_t*>(this->m_handle));
FW_ASSERT(stat == 0,stat);
}
void PosixMutex::lock() {
PosixMutex::Status status = this->take();
FW_ASSERT(status == PosixMutex::Status::OP_OK, status);
}

void Mutex::unLock() {
NATIVE_INT_TYPE stat = pthread_mutex_unlock(reinterpret_cast<pthread_mutex_t*>(this->m_handle));
FW_ASSERT(stat == 0,stat);
}
void PosixMutex::unLock() {
PosixMutex::Status status = this->release();
FW_ASSERT(status == PosixMutex::Status::OP_OK, status);
}

MutexHandle* PosixMutex::getHandle() {
return &this->m_handle;
}
} // namespace Mutex
} // namespace Posix
} // namespace Os
Loading
Loading