commit 5022f6a864e0859415fa9e599d93bc4bcbf450d9
author Arthur Ishiguro <arthuri@google.com> Tue Aug 11 15:38:22 2020 -0700
committer Arthur Ishiguro <arthuri@google.com> Wed Aug 12 07:42:47 2020 -0700
tree 3a5567b002545bf7d41593928395cace52d2aadc
parent 120ce1aa7bd9f237b357cc129ba71980d8df2d70

Adds handshaking timeout to UART link manager

Replaces the exiting semaphore with the CHPP condition variable,
to share the timeout logic used.

Bug: 160899751
Test: Send packet and verify timeout occurs
Change-Id: Ia101192b90b3fb1657efaba3442907ceb1803dc7

chpp/platform/aoc/chpp_uart_link_manager.cc

diff --git a/chpp/platform/aoc/chpp_uart_link_manager.cc b/chpp/platform/aoc/chpp_uart_link_manager.cc
index aab15ea..e300a71 100644
--- a/chpp/platform/aoc/chpp_uart_link_manager.cc
+++ b/chpp/platform/aoc/chpp_uart_link_manager.cc
@@ -53,10 +53,7 @@
   manager->getWakeInGpi()->SetTriggerFunction(GPIAoC::GPI_DISABLE);
   manager->getWakeInGpi()->ClearInterrupt();
 
-  BaseType_t xHigherPriorityTaskWoken = 0;
-  xSemaphoreGiveFromISR(manager->getSemaphoreHandle(),
-                        &xHigherPriorityTaskWoken);
-  portYIELD_FROM_ISR(xHigherPriorityTaskWoken);
+  chppConditionVariableSignal(manager->getCondVar());
 }
 
 }  // anonymous namespace
@@ -68,19 +65,16 @@
       mUart(uart),
       mWakeOutGpio(wakeOutPinNumber, IP_LOCK_GPIO),
       mWakeInGpi(wakeInGpiNumber) {
-  mSemaphoreHandle = xSemaphoreCreateBinaryStatic(&mStaticSemaphore);
-  if (mSemaphoreHandle == nullptr) {
-    FATAL_ERROR("Failed to create cv semaphore");
-  }
+  chppMutexInit(&mMutex);
+  chppConditionVariableInit(&mCondVar);
 
   mWakeOutGpio.SetDirection(GPIO::DIRECTION::OUTPUT);
   mWakeOutGpio.Clear();
 }
 
 UartLinkManager::~UartLinkManager() {
-  if (mSemaphoreHandle != nullptr) {
-    vSemaphoreDelete(mSemaphoreHandle);
-  }
+  chppConditionVariableDeinit(&mCondVar);
+  chppMutexDeinit(&mMutex);
 }
 
 void UartLinkManager::init() {
@@ -133,8 +127,10 @@
 
     mWakeInGpi.SetInterruptHandler(onTransactionHandshakeInterrupt, this);
     mWakeInGpi.SetTriggerFunction(GPIAoC::GPI_LEVEL_ACTIVE_HIGH);
-    // TODO: Add timeout
-    if (xSemaphoreTake(mSemaphoreHandle, portMAX_DELAY) == pdTRUE) {
+
+    chppMutexLock(&mMutex);
+    if (mWakeInGpi.PadValue() ||
+        chppConditionVariableTimedWait(&mCondVar, &mMutex, kStartTimeoutNs)) {
       if (hasTxPacket()) {
         int bytesTransmitted = mUart->Tx(mCurrentBuffer, mCurrentBufferLen);
 
@@ -152,17 +148,20 @@
       }
 
       mWakeInGpi.SetTriggerFunction(GPIAoC::GPI_LEVEL_ACTIVE_LOW);
-      // TODO: Add timeout
-      success &= (xSemaphoreTake(mSemaphoreHandle, portMAX_DELAY) == pdTRUE);
+      success &=
+          (!mWakeInGpi.PadValue() ||
+           chppConditionVariableTimedWait(&mCondVar, &mMutex, kStartTimeoutNs));
     } else {
       success = false;
       mWakeOutGpio.Clear();
     }
+    chppMutexUnlock(&mMutex);
 
     mTransactionPending = false;
   }
 
   // Re-enable the interrupt to handle transaction requests.
+  mWakeInGpi.SetTriggerFunction(GPIAoC::GPI_DISABLE);
   mWakeInGpi.SetInterruptHandler(onTransactionRequestInterrupt, this);
   mWakeInGpi.SetTriggerFunction(GPIAoC::GPI_LEVEL_ACTIVE_HIGH);
 

chpp/platform/aoc/include/chpp/platform/chpp_uart_link_manager.h

diff --git a/chpp/platform/aoc/include/chpp/platform/chpp_uart_link_manager.h b/chpp/platform/aoc/include/chpp/platform/chpp_uart_link_manager.h
index 5a30011..4a8187a 100644
--- a/chpp/platform/aoc/include/chpp/platform/chpp_uart_link_manager.h
+++ b/chpp/platform/aoc/include/chpp/platform/chpp_uart_link_manager.h
@@ -18,13 +18,15 @@
 #define CHPP_PLATFORM_UART_LINK_MANAGER_H_
 
 #include "FreeRTOS.h"
+#include "chpp/condition_variable.h"
 #include "chpp/link.h"
+#include "chpp/mutex.h"
 #include "chpp/transport.h"
 #include "chre/platform/atomic.h"
 #include "chre/util/non_copyable.h"
+#include "chre/util/time.h"
 #include "gpi_aoc.h"
 #include "gpio_aoc.h"
-#include "semphr.h"
 #include "uart.h"
 
 namespace chpp {
@@ -113,8 +115,8 @@
     return &mWakeInGpi;
   }
 
-  SemaphoreHandle_t getSemaphoreHandle() const {
-    return mSemaphoreHandle;
+  struct ChppConditionVariable *getCondVar() {
+    return &mCondVar;
   }
 
   void setTransactionPending() {
@@ -155,14 +157,18 @@
   uint8_t mRxBuf[kRxBufSize];
   size_t mRxBufIndex = 0;
 
-  /**
-   * Semaphore to use while waiting for GPI interrupts.
-   * NOTE: We use the semaphore directly instead of a condition
-   * variable, because there is no need to protect a predicate variable
-   * through a mutex.
-   */
-  SemaphoreHandle_t mSemaphoreHandle;
-  StaticSemaphore_t mStaticSemaphore;
+  //! The mutex/condition variable to use while waiting for GPI interrupts.
+  struct ChppMutex mMutex;
+  struct ChppConditionVariable mCondVar;
+
+  //! The timeout for waiting on the remote to indicate transaction readiness
+  //! (t_start per specifications).
+  static constexpr uint64_t kStartTimeoutNs =
+      100 * chre::kOneMillisecondInNanoseconds;
+
+  //! The timeout for waiting on the remote to indicate transaction ended
+  //! (t_end per specifications).
+  static constexpr uint64_t kEndTimeoutNs = chre::kOneSecondInNanoseconds;
 
   chre::AtomicBool mTransactionPending{false};