Skip to content

Commit

Permalink
Refactor Os::Queue into CMake selection (#2895)
Browse files Browse the repository at this point in the history
* Initial interface tests

* Initial working rules tests

* Moving MaxHeap to Types; Fixing minor bugs

* Basic ConditionVariables, Queue blocking, and UTs

* Queue updates for Ref

* Queue build all WIP

* UTs compile

* Passing UTs

* Removing unupdated linux queue implementations

* Fixing linux specific build issues

* Fixing task regression

* Fixing queue test memory leak

* Spelling, comments, formating, and todos

* Condition variable tests

* Minor edits from review

* Condition variable interface tests, fixes

* sp

* ci fixes

* Minor condition variable fix

* Fixing post merge issues

* Final review fixes

* Fixing FS stubs

* Fixing max heap failures

* Linux fixes

* Fixing issues w.r.t. CI
  • Loading branch information
LeStarch authored Sep 30, 2024
1 parent d6ebdff commit d034786
Show file tree
Hide file tree
Showing 113 changed files with 4,548 additions and 3,555 deletions.
4 changes: 3 additions & 1 deletion .github/actions/spelling/expect.txt
Original file line number Diff line number Diff line change
Expand Up @@ -794,6 +794,7 @@ prepeneding
pri
printables
prioritization
PRIORITYQUEUE
prm
PRMDB
PRMDBIMPL
Expand Down Expand Up @@ -997,6 +998,7 @@ STDC
stddef
stdint
stest
Stl
STREQ
STREQUAL
strerror
Expand Down Expand Up @@ -1216,4 +1218,4 @@ xsltproc
xxxx
yacgen
zimri
zmq
zmq
2 changes: 1 addition & 1 deletion FppTest/state_machine/test/ut/SmTestTester.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ void SmTestTester::schedIn_OK() {
void SmTestTester ::
dispatchAll()
{
while (this->component.m_queue.getNumMsgs() > 0)
while (this->component.m_queue.getMessagesAvailable() > 0)
this->component.doDispatch();
}

Expand Down
4 changes: 2 additions & 2 deletions Fw/Comp/ActiveComponentBase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ namespace Fw {
ActiveComponentExitSerializableBuffer exitBuff;
SerializeStatus stat = exitBuff.serialize(static_cast<I32>(ACTIVE_COMPONENT_EXIT));
FW_ASSERT(FW_SERIALIZE_OK == stat,static_cast<NATIVE_INT_TYPE>(stat));
(void)this->m_queue.send(exitBuff,0,Os::Queue::QUEUE_NONBLOCKING);
(void)this->m_queue.send(exitBuff,0,Os::Queue::BlockingType::NONBLOCKING);
DEBUG_PRINT("exit %s\n", this->getObjName());
}

Expand Down Expand Up @@ -137,7 +137,7 @@ namespace Fw {

ActiveComponentBase::MsgDispatchStatus ActiveComponentBase::dispatch() {
// Cooperative tasks should return rather than block when no messages are available
if (this->m_task.isCooperative() and m_queue.getNumMsgs() == 0) {
if (this->m_task.isCooperative() and m_queue.getMessagesAvailable() == 0) {
return MsgDispatchStatus::MSG_DISPATCH_EMPTY;
}
return this->doDispatch();
Expand Down
2 changes: 1 addition & 1 deletion Fw/Comp/QueuedComponentBase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ namespace Fw {
}
#endif

Os::Queue::QueueStatus QueuedComponentBase::createQueue(NATIVE_INT_TYPE depth, NATIVE_INT_TYPE msgSize) {
Os::Queue::Queue::Status QueuedComponentBase::createQueue(FwSizeType depth, FwSizeType msgSize) {

Os::QueueString queueName;
#if FW_OBJECT_NAMES == 1
Expand Down
2 changes: 1 addition & 1 deletion Fw/Comp/QueuedComponentBase.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ namespace Fw {
virtual ~QueuedComponentBase(); //!< Destructor
void init(NATIVE_INT_TYPE instance); //!< initialization function
Os::Queue m_queue; //!< queue object for active component
Os::Queue::QueueStatus createQueue(NATIVE_INT_TYPE depth, NATIVE_INT_TYPE msgSize);
Os::Queue::Status createQueue(FwSizeType depth, FwSizeType msgSize);
virtual MsgDispatchStatus doDispatch()=0; //!< method to dispatch a single message in the queue.
#if FW_OBJECT_TO_STRING == 1
virtual void toString(char* str, NATIVE_INT_TYPE size); //!< dump string representation of component
Expand Down
2 changes: 1 addition & 1 deletion Os/Baremetal/Queue.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ Queue::QueueStatus Queue::receive(U8* buffer, NATIVE_INT_TYPE capacity, NATIVE_I
return bareReceiveBlock(handle, buffer, capacity, actualSize, priority);
}

NATIVE_INT_TYPE Queue::getNumMsgs() const {
NATIVE_INT_TYPE Queue::getMessagesAvailable() const {
//Check if the handle is null or check the underlying queue is null
if ((nullptr == reinterpret_cast<BareQueueHandle*>(this->m_handle)) ||
(!reinterpret_cast<BareQueueHandle*>(this->m_handle)->m_init)) {
Expand Down
28 changes: 5 additions & 23 deletions Os/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
add_fprime_subdirectory("${CMAKE_CURRENT_LIST_DIR}/Models")
add_fprime_subdirectory("${CMAKE_CURRENT_LIST_DIR}/Stub")
add_fprime_subdirectory("${CMAKE_CURRENT_LIST_DIR}/Posix")
add_fprime_subdirectory("${CMAKE_CURRENT_LIST_DIR}/Generic")

set(SOURCE_FILES
"${CMAKE_CURRENT_LIST_DIR}/test/ut/file/SyntheticFileSystem.cpp"
Expand All @@ -26,10 +27,6 @@ set(MOD_DEPS
# Basic source files used in every OSAL layer. Contains common code and helpers.
set(SOURCE_FILES
"${CMAKE_CURRENT_LIST_DIR}/IntervalTimerCommon.cpp"
# "${CMAKE_CURRENT_LIST_DIR}/TaskCommon.cpp"
"${CMAKE_CURRENT_LIST_DIR}/QueueCommon.cpp"
"${CMAKE_CURRENT_LIST_DIR}/IPCQueueCommon.cpp"
"${CMAKE_CURRENT_LIST_DIR}/SimpleQueueRegistry.cpp"
"${CMAKE_CURRENT_LIST_DIR}/MemCommon.cpp"
"${CMAKE_CURRENT_LIST_DIR}/ValidateFileCommon.cpp"
"${CMAKE_CURRENT_LIST_DIR}/ValidatedFile.cpp"
Expand All @@ -41,6 +38,8 @@ set(SOURCE_FILES
"${CMAKE_CURRENT_LIST_DIR}/Mutex.cpp"
"${CMAKE_CURRENT_LIST_DIR}/FileSystem.cpp"
"${CMAKE_CURRENT_LIST_DIR}/Directory.cpp"
"${CMAKE_CURRENT_LIST_DIR}/Condition.cpp"
"${CMAKE_CURRENT_LIST_DIR}/Queue.cpp"
)
# Check for default logger
if (NOT FPRIME_DISABLE_DEFAULT_LOGGER)
Expand All @@ -52,10 +51,6 @@ endif()
if (FPRIME_USE_POSIX)
list(APPEND SOURCE_FILES
"${CMAKE_CURRENT_LIST_DIR}/Console.cpp"
"${CMAKE_CURRENT_LIST_DIR}/Pthreads/Queue.cpp"
"${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}/Linux/InterruptLock.cpp"
"${CMAKE_CURRENT_LIST_DIR}/Linux/WatchdogTimer.cpp"
"${CMAKE_CURRENT_LIST_DIR}/Posix/IntervalTimer.cpp"
Expand All @@ -64,14 +59,12 @@ endif()
# Darwin IPC queue implementation
if(${CMAKE_SYSTEM_NAME} STREQUAL "Darwin")
list(APPEND SOURCE_FILES
"${CMAKE_CURRENT_LIST_DIR}/MacOs/IPCQueueStub.cpp"
# "${CMAKE_CURRENT_LIST_DIR}/MacOs/IPCQueueStub.cpp"
"${CMAKE_CURRENT_LIST_DIR}/MacOs/SystemResources.cpp"
)
# Linux IPC queues implementation
elseif (${CMAKE_SYSTEM_NAME} STREQUAL "Linux")
list(APPEND SOURCE_FILES
"${CMAKE_CURRENT_LIST_DIR}/Posix/IPCQueue.cpp"
"${CMAKE_CURRENT_LIST_DIR}/Posix/LocklessQueue.cpp"
"${CMAKE_CURRENT_LIST_DIR}/Linux/SystemResources.cpp"
)
# Shared libraries need an -rt dependency for mq libs
Expand Down Expand Up @@ -103,11 +96,11 @@ require_fprime_implementation(Os/Directory)
require_fprime_implementation(Os/Mutex)
# require_fprime_implementation(Os/Task) # should be added in
# require_fprime_implementation(Os/Console) # should be added in
require_fprime_implementation(Os/Queue)


### UTS ### Note: 3 separate UTs registered here.
set(UT_SOURCE_FILES
"${CMAKE_CURRENT_LIST_DIR}/test/ut/OsQueueTest.cpp"
"${CMAKE_CURRENT_LIST_DIR}/test/ut/OsTestMain.cpp"
"${CMAKE_CURRENT_LIST_DIR}/test/ut/IntervalTimerTest.cpp"
"${CMAKE_CURRENT_LIST_DIR}/test/ut/OsValidateFileTest.cpp"
Expand All @@ -127,14 +120,3 @@ if (BUILD_TESTING)
endif()
endforeach()
endif()
# Second UT Pthreads
set(UT_SOURCE_FILES
"${CMAKE_CURRENT_LIST_DIR}/Pthreads/test/ut/BufferQueueTest.cpp"
)
register_fprime_ut("Os_pthreads")

# Third UT Pthreads MAX Heap
set(UT_SOURCE_FILES
"${CMAKE_CURRENT_LIST_DIR}/Pthreads/MaxHeap/test/ut/MaxHeapTest.cpp"
)
register_fprime_ut("Os_pthreads_max_heap")
34 changes: 34 additions & 0 deletions Os/Condition.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
#include "Os/Condition.hpp"
#include "Fw/Types/Assert.hpp"

namespace Os {
ConditionVariable::ConditionVariable() : m_delegate(*ConditionVariableInterface::getDelegate(m_handle_storage)) {}

ConditionVariable::~ConditionVariable() {
m_delegate.~ConditionVariableInterface();
}

void ConditionVariable::wait(Os::Mutex& mutex) {
FW_ASSERT(&this->m_delegate == reinterpret_cast<ConditionVariableInterface*>(&this->m_handle_storage[0]));
FW_ASSERT(this->m_lock == nullptr || this->m_lock == &mutex); // Confirm the same mutex used across waits
this->m_lock = &mutex;
// Attempt to lock the mutex and ensure that this was successful or the lock was already held
Mutex::Status lock_status = this->m_lock->take();
FW_ASSERT(lock_status == Mutex::Status::OP_OK || lock_status == Mutex::Status::ERROR_DEADLOCK);
this->m_delegate.wait(mutex);
}
void ConditionVariable::notify() {
FW_ASSERT(&this->m_delegate == reinterpret_cast<ConditionVariableInterface*>(&this->m_handle_storage[0]));
this->m_delegate.notify();
}
void ConditionVariable::notifyAll() {
FW_ASSERT(&this->m_delegate == reinterpret_cast<ConditionVariableInterface*>(&this->m_handle_storage[0]));
this->m_delegate.notifyAll();
}

ConditionVariableHandle* ConditionVariable::getHandle(){
FW_ASSERT(&this->m_delegate == reinterpret_cast<const ConditionVariableInterface*>(&this->m_handle_storage[0]));
return this->m_delegate.getHandle();
}

}
125 changes: 125 additions & 0 deletions Os/Condition.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
// ======================================================================
// \title Os/Condition.hpp
// \brief common function definitions for Os::ConditionVariables
// ======================================================================
#include "Os/Mutex.hpp"
#include "Os/Os.hpp"

#ifndef OS_CONDITION_HPP_
#define OS_CONDITION_HPP_

namespace Os {

//! \brief Condition variable handle parent
class ConditionVariableHandle {};

//! \brief interface for condition variables
//!
//! Condition variables allow a program to block on a condition while atomically releasing an Os::Mutex and atomically
//! reacquiring the mutex once the condition has been notified.
class ConditionVariableInterface {
public:
//! Default constructor
ConditionVariableInterface() = default;
//! Default destructor
virtual ~ConditionVariableInterface() = default;

//! \brief copy constructor is forbidden
ConditionVariableInterface(const ConditionVariableInterface& other) = delete;

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

//! \brief wait on a condition variable
//!
//! Wait on a condition variable. This function will atomically unlock the provided mutex and block on the condition
//! in one step. Blocking will occur until a future `notify` or `notifyAll` call is made to this variable on another
//! thread of execution.
//!
//! \param mutex: mutex to unlock as part of this operation
virtual void wait(Os::Mutex& mutex) = 0;

//! \brief notify a single waiter on this condition variable
//!
//! Notify a single waiter on this condition variable. It is not necessary to hold the mutex supplied by the waiters
//! and it is advantageous not to hold the lock to prevent immediate re-blocking.
virtual void notify() = 0;

//! \brief notify all waiters on this condition variable
//!
//! Notify all waiters on this condition variable. It is not necessary to hold the mutex supplied by the waiters
//! and it is advantageous not to hold the lock to prevent immediate re-blocking.
virtual void notifyAll() = 0;

//! \brief return the underlying condition variable handle (implementation specific).
//! \return internal task handle representation
virtual ConditionVariableHandle* getHandle() = 0;

//! \brief provide a pointer to a Mutex delegate object
static ConditionVariableInterface* getDelegate(ConditionVariableHandleStorage& aligned_new_memory);
};

//! \brief condition variable implementation
//!
//! Condition variables allow a program to block on a condition while atomically releasing an Os::Mutex and atomically
//! reacquiring the mutex once the condition has been notified.
class ConditionVariable final : public ConditionVariableInterface {
public:
//! \brief default constructor
ConditionVariable();

//! \brief default virtual destructor
~ConditionVariable() final;

//! \brief copy constructor is forbidden
ConditionVariable(const ConditionVariableInterface& other) = delete;

//! \brief copy constructor is forbidden
ConditionVariable(const ConditionVariableInterface* other) = delete;

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

//! \brief wait on a condition variable
//!
//! Wait on a condition variable. This function will atomically unlock the provided mutex and block on the condition
//! in one step. Blocking will occur until a future `notify` or `notifyAll` call is made to this variable on another
//! thread of execution. This function delegates to the underlying implementation.
//!
//! \warning it is invalid to supply a mutex different from those supplied by others
//! \warning conditions *must* be rechecked after the condition variable unlocks
//! \note unlocked mutexes will be locked before waiting and will be relocked before this function returns
//!
//! \param mutex: mutex to unlock as part of this operation
void wait(Os::Mutex& mutex) override;

//! \brief notify a single waiter on this condition variable
//!
//! Notify a single waiter on this condition variable. It is not necessary to hold the mutex supplied by the waiters
//! and it is advantageous not to hold the lock to prevent immediate re-blocking. This function delegates to the
//! underlying implementation.
void notify() override;

//! \brief notify all waiters on this condition variable
//!
//! Notify all waiters on this condition variable. It is not necessary to hold the mutex supplied by the waiters
//! and it is advantageous not to hold the lock to prevent immediate re-blocking. This function delegates to the
//! underlying implementation.
void notifyAll() override;

//! \brief return the underlying condition variable handle (implementation specific). Delegates to implementation.
//! \return internal task handle representation
ConditionVariableHandle* getHandle() override;

private:
//! Pointer to mutex object previously used
Os::Mutex* m_lock = nullptr;

// This section is used to store the implementation-defined file handle. To Os::File 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) ConditionVariableHandleStorage m_handle_storage; //!< Storage for aligned FileHandle data
ConditionVariableInterface& m_delegate; //!< Delegate for the real implementation
};
}
#endif //OS_CONDITION_HPP_
2 changes: 0 additions & 2 deletions Os/Console.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@
#include <Fw/Types/Assert.hpp>

namespace Os {
Console* Console::s_singleton;

Console::Console() : ConsoleInterface(), Fw::Logger(), m_handle_storage(), m_delegate(*ConsoleInterface::getDelegate(m_handle_storage)) {}

Console::~Console() {
Expand Down
1 change: 0 additions & 1 deletion Os/Console.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,6 @@ namespace Os {
static Console& getSingleton();

private:
static Console* s_singleton;
// This section is used to store the implementation-defined console handle. To Os::Console 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.
Expand Down
6 changes: 3 additions & 3 deletions Os/Delegate.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,11 @@ namespace Delegate {
//! \tparam Implementation: implementation class of the delegate (e.g. PosixTask)
//! \param aligned_new_memory: memory to be filled via placement new call
//! \return pointer to implementation result of placement new
template <class Interface, class Implementation>
inline Interface* makeDelegate(HandleStorage& aligned_new_memory) {
template <class Interface, class Implementation, class StorageType=HandleStorage>
inline Interface* makeDelegate(StorageType& aligned_new_memory) {
// Ensure prerequisites before performing placement new
static_assert(std::is_base_of<Interface, Implementation>::value, "Implementation must derive from Interface");
static_assert(sizeof(Implementation) <= FW_HANDLE_MAX_SIZE, "Handle size not large enough");
static_assert(sizeof(Implementation) <= sizeof(StorageType), "Handle size not large enough");
static_assert((FW_HANDLE_ALIGNMENT % alignof(Implementation)) == 0, "Handle alignment invalid");
// Placement new the object and ensure non-null result
Implementation* interface = new (aligned_new_memory) Implementation;
Expand Down
2 changes: 1 addition & 1 deletion Os/Directory.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ class DirectoryInterface {
virtual DirectoryHandle* getHandle() = 0;

//! \brief provide a pointer to a Directory delegate object
static DirectoryInterface* getDelegate(HandleStorage& aligned_new_memory);
static DirectoryInterface* getDelegate(DirectoryHandleStorage& aligned_new_memory);


// -----------------------------------------------------------------
Expand Down
2 changes: 0 additions & 2 deletions Os/FileSystem.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@
#include <Os/FileSystem.hpp>

namespace Os {
FileSystem* FileSystem::s_singleton;


FileSystem::FileSystem() : m_handle_storage(), m_delegate(*FileSystemInterface::getDelegate(m_handle_storage)) {
FW_ASSERT(&this->m_delegate == reinterpret_cast<FileSystemInterface*>(&this->m_handle_storage[0]));
Expand Down
4 changes: 1 addition & 3 deletions Os/FileSystem.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ class FileSystemInterface {
virtual FileSystemHandle* getHandle() = 0;

//! \brief provide a pointer to a FileSystem delegate object
static FileSystemInterface* getDelegate(HandleStorage& aligned_new_memory);
static FileSystemInterface* getDelegate(FileSystemHandleStorage& aligned_new_memory);


// ------------------------------------------------------------------
Expand Down Expand Up @@ -380,8 +380,6 @@ class FileSystem final : public FileSystemInterface {

alignas(FW_HANDLE_ALIGNMENT) FileSystemHandleStorage m_handle_storage; //!< FileSystem handle storage
FileSystemInterface& m_delegate;

static FileSystem* s_singleton;
};


Expand Down
Loading

0 comments on commit d034786

Please sign in to comment.