Update mutex and condition-variable interface (#3158)

* Adhering to new mutex api

* casting assert arg as expected

* Using correct mutex call

* Formatting

* Fixing interface for wait/pend

* Fixing conditionvariable to adhere to new interface

* Updated Os::Stub's implementation of ConditionVariable per new interface

* Formatting and updating Test's ConditionVariable implementation to the new API

* bugfix to ut

* Fixes per PR review.

Author: kevin-f-ortega

Os/Condition.cpp

@@ -8,14 +8,17 @@ ConditionVariable::~ConditionVariable() {
     m_delegate.~ConditionVariableInterface();
 }
 
-void ConditionVariable::wait(Os::Mutex& mutex) {
+ConditionVariable::Status ConditionVariable::pend(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
+    if (this->m_lock != nullptr && this->m_lock != &mutex) {
+        return Status::ERROR_DIFFERENT_MUTEX;
+    };
     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);
+    return this->m_delegate.pend(mutex);
+}
+void ConditionVariable::wait(Os::Mutex& mutex) {
+    Status status = this->pend(mutex);
+    FW_ASSERT(status == Status::OP_OK, static_cast<FwAssertArgType>(status));
 }
 void ConditionVariable::notify() {
     FW_ASSERT(&this->m_delegate == reinterpret_cast<ConditionVariableInterface*>(&this->m_handle_storage[0]));
@@ -26,9 +29,9 @@ void ConditionVariable::notifyAll() {
     this->m_delegate.notifyAll();
 }
 
-ConditionVariableHandle* ConditionVariable::getHandle(){
+ConditionVariableHandle* ConditionVariable::getHandle() {
     FW_ASSERT(&this->m_delegate == reinterpret_cast<const ConditionVariableInterface*>(&this->m_handle_storage[0]));
     return this->m_delegate.getHandle();
 }
 
-}
+}  // namespace Os

Os/Condition.hpp

@@ -19,6 +19,14 @@ class ConditionVariableHandle {};
 //! reacquiring the mutex once the condition has been notified.
 class ConditionVariableInterface {
   public:
+    enum Status {
+        OP_OK,                  //!<  Operation was successful
+        ERROR_MUTEX_NOT_HELD,   //!< When trying to wait but we don't hold the mutex
+        ERROR_DIFFERENT_MUTEX,  //!< When trying to use a different mutex than expected mutex
+        ERROR_NOT_IMPLEMENTED,  //!< When trying to use a feature that isn't implemented
+        ERROR_OTHER             //!< All other errors
+    };
+
     //! Default constructor
     ConditionVariableInterface() = default;
     //! Default destructor
@@ -37,7 +45,8 @@ class ConditionVariableInterface {
     //! thread of execution.
     //!
     //! \param mutex: mutex to unlock as part of this operation
-    virtual void wait(Os::Mutex& mutex) = 0;
+    //! \return status of the conditional wait
+    virtual Status pend(Os::Mutex& mutex) = 0;
 
     //! \brief notify a single waiter on this condition variable
     //!
@@ -88,10 +97,24 @@ class ConditionVariable final : public ConditionVariableInterface {
     //!
     //! \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
+    //! \warning the mutex must be locked by the calling task
+    //!
+    //! \param mutex: mutex to unlock as part of this operation
+    //! \return status of the conditional wait
+    Status pend(Os::Mutex& mutex) override;
+
+    //! \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
+    //! \warning the mutex must be locked by the calling task
     //!
     //! \param mutex: mutex to unlock as part of this operation
-    void wait(Os::Mutex& mutex) override;
+    void wait(Os::Mutex& mutex);
 
     //! \brief notify a single waiter on this condition variable
     //!
@@ -118,8 +141,9 @@ class ConditionVariable final : public ConditionVariableInterface {
     // 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
+    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_
+}  // namespace Os
+#endif  // OS_CONDITION_HPP_

Os/Mutex.cpp

@@ -48,7 +48,7 @@ ScopeLock::ScopeLock(Mutex& mutex) : m_mutex(mutex) {
 }
 
 ScopeLock::~ScopeLock() {
-    this->m_mutex.release();
+    this->m_mutex.unLock();
 }
 
 }  // namespace Os

Os/Posix/ConditionVariable.cpp

@@ -3,8 +3,9 @@
 // \brief Posix implementations for Os::ConditionVariable
 // ======================================================================
 #include "Os/Posix/ConditionVariable.hpp"
-#include "Os/Posix/Mutex.hpp"
 #include "Fw/Types/Assert.hpp"
+#include "Os/Posix/Mutex.hpp"
+#include "Os/Posix/error.hpp"
 
 namespace Os {
 namespace Posix {
@@ -18,9 +19,10 @@ PosixConditionVariable::~PosixConditionVariable() {
     (void)pthread_cond_destroy(&this->m_handle.m_condition);
 }
 
-void PosixConditionVariable::wait(Os::Mutex& mutex) {
+PosixConditionVariable::Status PosixConditionVariable::pend(Os::Mutex& mutex) {
     PosixMutexHandle* mutex_handle = reinterpret_cast<PosixMutexHandle*>(mutex.getHandle());
-    FW_ASSERT(pthread_cond_wait(&this->m_handle.m_condition, &mutex_handle->m_mutex_descriptor) == 0);
+    PlatformIntType status = pthread_cond_wait(&this->m_handle.m_condition, &mutex_handle->m_mutex_descriptor);
+    return posix_status_to_conditional_status(status);
 }
 void PosixConditionVariable::notify() {
     FW_ASSERT(pthread_cond_signal(&this->m_handle.m_condition) == 0);
@@ -33,6 +35,6 @@ ConditionVariableHandle* PosixConditionVariable::getHandle() {
     return &m_handle;
 }
 
-}
-}
-}
+}  // namespace Mutex
+}  // namespace Posix
+}  // namespace Os

Os/Posix/ConditionVariable.hpp

@@ -32,7 +32,7 @@ class PosixConditionVariable : public ConditionVariableInterface {
     ConditionVariableInterface& operator=(const ConditionVariableInterface& other) override = delete;
 
     //! \brief wait releasing mutex
-    void wait(Os::Mutex& mutex) override;
+    PosixConditionVariable::Status pend(Os::Mutex& mutex) override;
 
     //! \brief notify a single waiter
     void notify() override;

Os/Posix/DefaultMutex.cpp

@@ -2,26 +2,24 @@
 // \title Os/Posix/DefaultMutex.cpp
 // \brief sets default Os::Mutex Posix implementation via linker
 // ======================================================================
-#include "Os/Posix/Mutex.hpp"
-#include "Os/Posix/ConditionVariable.hpp"
 #include "Os/Delegate.hpp"
+#include "Os/Posix/ConditionVariable.hpp"
+#include "Os/Posix/Mutex.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(MutexHandleStorage& aligned_new_memory) {
-    return Os::Delegate::makeDelegate<MutexInterface, Os::Posix::Mutex::PosixMutex>(
-            aligned_new_memory
-    );
+MutexInterface* MutexInterface::getDelegate(MutexHandleStorage& aligned_new_memory) {
+    return Os::Delegate::makeDelegate<MutexInterface, Os::Posix::Mutex::PosixMutex>(aligned_new_memory);
 }
 
 //! \brief get a delegate for MutexInterface that intercepts calls for Posix
 //! \param aligned_new_memory: aligned memory to fill
 //! \return: pointer to delegate
-ConditionVariableInterface *ConditionVariableInterface::getDelegate(ConditionVariableHandleStorage& aligned_new_memory) {
-    return Os::Delegate::makeDelegate<ConditionVariableInterface, Os::Posix::Mutex::PosixConditionVariable, ConditionVariableHandleStorage>(
-        aligned_new_memory
-    );
-}
+ConditionVariableInterface* ConditionVariableInterface::getDelegate(
+    ConditionVariableHandleStorage& aligned_new_memory) {
+    return Os::Delegate::makeDelegate<ConditionVariableInterface, Os::Posix::Mutex::PosixConditionVariable,
+                                      ConditionVariableHandleStorage>(aligned_new_memory);
 }
+}  // namespace Os

Os/Posix/Mutex.cpp

@@ -2,9 +2,9 @@
 // \title Os/Posix/Mutex.cpp
 // \brief Posix implementation for Os::Mutex
 // ======================================================================
+#include <Fw/Types/Assert.hpp>
 #include <Os/Posix/Mutex.hpp>
 #include <Os/Posix/error.hpp>
-#include <Fw/Types/Assert.hpp>
 
 namespace Os {
 namespace Posix {
@@ -14,23 +14,23 @@ PosixMutex::PosixMutex() : Os::MutexInterface(), m_handle() {
     // set attributes
     pthread_mutexattr_t attribute;
     PlatformIntType status = pthread_mutexattr_init(&attribute);
-    FW_ASSERT(status == 0, status);
+    FW_ASSERT(status == 0, static_cast<FwAssertArgType>(status));
 
     // set to normal mutex type
     status = pthread_mutexattr_settype(&attribute, PTHREAD_MUTEX_ERRORCHECK);
-    FW_ASSERT(status == 0, status);
+    FW_ASSERT(status == 0, static_cast<FwAssertArgType>(status));
 
     // set to check for priority inheritance
     status = pthread_mutexattr_setprotocol(&attribute, PTHREAD_PRIO_INHERIT);
-    FW_ASSERT(status == 0, status);
+    FW_ASSERT(status == 0, static_cast<FwAssertArgType>(status));
 
     status = pthread_mutex_init(&this->m_handle.m_mutex_descriptor, &attribute);
-    FW_ASSERT(status == 0, status);
+    FW_ASSERT(status == 0, static_cast<FwAssertArgType>(status));
 }
 
 PosixMutex::~PosixMutex() {
     PlatformIntType status = pthread_mutex_destroy(&this->m_handle.m_mutex_descriptor);
-    FW_ASSERT(status == 0, status);
+    FW_ASSERT(status == 0, static_cast<FwAssertArgType>(status));
 }
 
 PosixMutex::Status PosixMutex::take() {

Os/Posix/error.cpp

@@ -184,5 +184,21 @@ Mutex::Status posix_status_to_mutex_status(PlatformIntType posix_status){
     }
     return status;
 }
+
+ConditionVariable::Status posix_status_to_conditional_status(PlatformIntType posix_status) {
+    ConditionVariable::Status status = ConditionVariable::Status::ERROR_OTHER;
+    switch (posix_status) {
+        case 0:
+            status = ConditionVariable::Status::OP_OK;
+            break;
+        case EPERM:
+            status =  ConditionVariable::Status::ERROR_MUTEX_NOT_HELD;
+            break;
+        default:
+            status =  ConditionVariable::Status::ERROR_OTHER;
+            break;
+    }
+    return status;
+}
 }
 }

Os/Posix/error.hpp

@@ -9,6 +9,7 @@
 #include "Os/FileSystem.hpp"
 #include "Os/Directory.hpp"
 #include "Os/RawTime.hpp"
+#include "Os/Condition.hpp"
 
 namespace Os {
 namespace Posix {
@@ -49,6 +50,13 @@ Os::Task::Status posix_status_to_task_status(PlatformIntType posix_status);
 //!
 Os::Mutex::Status posix_status_to_mutex_status(PlatformIntType posix_status);
 
+//! Convert a Posix return status (int) for Conditional Variable operations to the Os::ConditionVariable::Status
+//! representation.
+//! \param posix_status: return status
+//! \return: Os::ConditionVariable::Status representation of the error
+//!
+Os::ConditionVariable::Status posix_status_to_conditional_status(PlatformIntType posix_status);
+
 }
 }
 #endif

Os/Stub/ConditionVariable.cpp

@@ -9,10 +9,10 @@ namespace Os {
 namespace Stub {
 namespace Mutex {
 
-void StubConditionVariable::wait(Os::Mutex& mutex) {
+StubConditionVariable::Status StubConditionVariable::pend(Os::Mutex& mutex) {
     // This stub implementation can only be used in deployments that never need to wait on any ConditionVariable.
-    // Trigger an assertion if anyone ever tries to wait.
-    FW_ASSERT(0);
+    // Return error if anyone ever tries to wait.
+    return StubConditionVariable::Status::ERROR_NOT_IMPLEMENTED;
 }
 void StubConditionVariable::notify() {
     // Nobody is waiting, because we assert if anyone tries to wait.
@@ -27,6 +27,6 @@ ConditionVariableHandle* StubConditionVariable::getHandle() {
     return &m_handle;
 }
 
-}
-}
-}
+}  // namespace Mutex
+}  // namespace Stub
+}  // namespace Os