Fix crash caused by race condition in xSemaphoreTake (#173)

* Fix race condition in xSemaphoreTake

This race condition would make infinisim eventually crash with the message "Mutex released without being held" on my machine.

The function `xSemaphoreTake` waits for the semaphore to be available to take by  looping + sleeping until there is an element in the `pxQueue->queue`. Then it locks the `pxQueue->mutex` and checks the queuesize again. Since the first check is not done under the mutex, some other thread could remove the element from the queue before the mutex is locked. Then the second check in `xSemaphoreTake` will return false.
The method `DateTimeController::CurrentDateTime()` uses a semaphore as a mutex to guard calls to `UpdateTime()`. But it (and the other methods from `DateTimeController`) do not check if `xSemaphoreTake` actually gave them the semaphore. They will try to give it back even if they failed to take it, which leads to the crash in `xSemaphoreGive`.
The other thread in this crash was calling the watchface refresh method, which also calls `CurrentDateTime()`.

To fix the race condition, lock the mutex guarding `pxQeue->queue`  before checking the queue size and unlock before sleeping. When implemented like this, `xSemaphoreTake` from InfiniSim follows the API from FreeRTOS more closely, which waits the full `xTicksToWait` while InfiniSim would abort prematurely in some cases.

With the assumption that `UpdateTime()` will finish within some finite amount of time, this make the methods in `DateTimerController` work even without error handling since they have such long wait times.

* Change to constexpr and use the appropriate type for DELAY_BETWEEN_ATTEMPTS

Since xTicksToWait is TickType_t (uint32) and SDL_Delay takes uint32 as well, we should avoid any chance of funny business due to differences in signedness.

Author: dariusarnold

sim/semphr.cpp

@@ -5,27 +5,33 @@
 QueueHandle_t xSemaphoreCreateMutex() {
   SemaphoreHandle_t xSemaphore = xQueueCreate(1, 1);
   Queue_t *pxQueue = (Queue_t *)xSemaphore;
+  // Queue full represents taken semaphore/locked mutex
   pxQueue->queue.push_back(0);
   return xSemaphore;
-};
+}
 
 BaseType_t xSemaphoreTake(SemaphoreHandle_t xSemaphore,
                           TickType_t xTicksToWait) {
   Queue_t *pxQueue = (Queue_t *)xSemaphore;
-  while (!pxQueue->queue.empty()) {
-    if (xTicksToWait <= 25) {
-      return false;
+  constexpr TickType_t DELAY_BETWEEN_ATTEMPTS = 25;
+  do {
+    if (pxQueue->mutex.try_lock()) {
+      std::lock_guard<std::mutex> lock(pxQueue->mutex, std::adopt_lock);
+      if (pxQueue->queue.empty()) {
+        pxQueue->queue.push_back(0);
+        return true;
+      }
     }
-    SDL_Delay(25);
-    xTicksToWait -= 25;
-  }
-  std::lock_guard<std::mutex> guard(pxQueue->mutex);
-  if (!pxQueue->queue.empty()) {
-    return false;
-  }
-  pxQueue->queue.push_back(0);
-  return true;
+    // Prevent underflow
+    if (xTicksToWait >= DELAY_BETWEEN_ATTEMPTS) {
+      // Someone else is modifying queue, wait for them to finish
+      SDL_Delay(DELAY_BETWEEN_ATTEMPTS);
+      xTicksToWait -= DELAY_BETWEEN_ATTEMPTS;
+    }
+  } while (xTicksToWait >= DELAY_BETWEEN_ATTEMPTS);
+  return false;
 }
+
 BaseType_t xSemaphoreGive(SemaphoreHandle_t xSemaphore) {
   Queue_t *pxQueue = (Queue_t *)xSemaphore;
   std::lock_guard<std::mutex> guard(pxQueue->mutex);