#27 Yield for micros could get stuck because of running task. #25 Locking made to work within bus in sync and async modes

Author: dave

examples/reentrantLocking/reentrantLocking.ino

@@ -1,10 +1,11 @@
 
 #include <TaskManagerIO.h>
-#include <ReentrantYieldingLock.h>
+#include <SimpleSpinLock.h>
 
 // this is the global lock that that we'll use to protect the variable below myVar.
-ReentrantYieldingLock myLock;
+SimpleSpinLock myLock;
 volatile int myVar = 0;
+int localVarCopy = 0;
 
 //
 // A simple log function for writing to serial
@@ -22,45 +23,71 @@ void log(const char* str, int val) {
 // functionality is working
 //
 void nestedFunction() {
+    // here we take a nested lock, which is allowed by our lock, which is essentially reference counted for up to
+    // 255 levels of depth.
     TaskMgrLock locker(myLock);
     log("in nested function", myVar);
 }
 
+void stateMachineLockingAsync();
+
 void setup() {
     Serial.begin(115200);
 
     // start a task that locks the bus, calls a nested function and yields time back to task manager.
-    taskManager.scheduleFixedRate(1000, [] {
-        TaskMgrLock locker(myLock);
-        int myVarAtStart = myVar;
-        log("start task function", myVar);
-
-        // the nested function will lock again, which is fine because it's in the same task
-        nestedFunction();
+    taskManager.scheduleFixedRate(1000, stateMachineLockingAsync);
 
-        // now we release task manager to run other tasks, the will not be able to take our lock
-        taskManager.yieldForMicros(millisToMicros(500));
 
-        // now we have the context back call the nested lock again
-        nestedFunction();
+    taskManager.scheduleFixedRate(250, [] {
+        log("Running inner task, before lock ", myLock.getLockCount());
 
-        if(myVar != myVarAtStart) {
-            log("ERROR in locking ", myVarAtStart);
-        }
+        // take the lock
+        TaskMgrLock locker(myLock);
 
-        log("exit task function", myVar);
-    });
+        log("locked synchronously, count", myLock.getLockCount());
 
-    taskManager.scheduleFixedRate(5, [] {
-        // here we have another task that locks, it will need to acquire the lock exclusively before entering
-        TaskMgrLock locker(myLock);
         // do something that's unlikely to be optimised out, to waste some time
         for(int i=0; i < 100; i++) {
             myVar++;
         }
+
+        log("Synchronous task done", myVar);
+
+        // lock released at end of function
     });
 }
 
 void loop() {
     taskManager.runLoop();
-}
+}
+
+//
+// This function shows how to use the lock asynchronously, in code where the lock being taken and unlocked are not
+// sequential actions. Great care is needed with this approach to avoid deadlocks. It is far easier to get the regular
+// TaskMgrLock based locking right.
+//
+void stateMachineLockingAsync() {
+    log("Before try lock, count", myLock.getLockCount());
+    if(myLock.tryLock()) {
+        // do a check to ensure nothing else has run.
+        if(localVarCopy != myVar) {
+            log("ERROR - another task got into the lock", myVar - localVarCopy);
+        }
+
+        log("We already had the async lock, now unlock, count", myLock.getLockCount());
+        myLock.unlock();
+    }
+    else {
+        // we didn't have the lock, try and acquire with a spin wait, that times out if it takes too long.
+        log("We need to do async lock", myLock.getLockCount());
+
+        bool gotLock = myLock.spinLock(millisToMicros(10));
+        if(gotLock) {
+            localVarCopy = myVar;
+            log("  - got the lock", myLock.getLockCount());
+        }
+        else {
+            log("  - spin wait failed", myLock.getLockCount());
+        }
+    }
+}

src/ReentrantYieldingLock.cpp renamed to src/SimpleSpinLock.cpp

@@ -3,36 +3,45 @@
  * This product is licensed under an Apache license, see the LICENSE file in the top-level directory.
  */
 
-#include "ReentrantYieldingLock.h"
+#include "SimpleSpinLock.h"
 
-bool ReentrantYieldingLock::spinLock(unsigned long micros) {
+bool SimpleSpinLock::tryLock() {
     // if our task already owns the lock then we are good.
-    if(locked && initiatingTask && taskManager.getRunningTask() == initiatingTask) return true;
+    return (locked && initiatingTask && taskManager.getRunningTask() == initiatingTask);
+}
+
+bool SimpleSpinLock::spinLock(unsigned long iterations) {
+    if(tryLock()) {
+        ++count;
+        return true;
+    }
 
     // otherwise we contend to get the lock in a spin wait until we exhaust the micros provided.
-    bool areWeLocked = false;
-    while(!locked && micros > 50) {
-        areWeLocked = tm_internal::atomicSwapBool(&locked, false, true);
-        if (areWeLocked) {
-            ++count;
+    while(iterations) {
+        if (tm_internal::atomicSwapBool(&locked, false, true)) {
             tm_internal::atomicWritePtr(&initiatingTask, taskManager.getRunningTask());
+            ++count;
             return true;
         }
         else {
-            taskManager.yieldForMicros(50);
+            taskManager.yieldForMicros(100);
         }
-        micros -= 50;
+        --iterations;
     }
+
     return false;
 }
 
-void ReentrantYieldingLock::unlock() {
-    if(count != 0) {
-        --count;
+void SimpleSpinLock::unlock() {
+    if(count == 0) {
         return;
     }
-    else {
+
+    --count;
+
+    if(count == 0) {
         tm_internal::atomicWritePtr(&initiatingTask, nullptr);
         tm_internal::atomicWriteBool(&locked, false);
     }
 }
+

src/ReentrantYieldingLock.h renamed to src/SimpleSpinLock.h

@@ -9,21 +9,21 @@
 #include "TaskManagerIO.h"
 
 /**
- * A lock that is intended only for use within tasks, if the lock is taken it will allow task manager to run using
- * it's yield for micros call until the lock becomes free. It is a spin lock, so it is safe to use with task manager.
- * Unless you want to use the spin behaviour in a specific way, prefer using with TaskSafeLock
+ * A very simple lock that can be used to provide a very simple mutex like behaviour based on task manager
+ * atomic constructs. It has the ability to try and spin lock, and also to fully lock in conjunction with
+ * TaskMgrLock class.
  */
-class ReentrantYieldingLock {
+class SimpleSpinLock {
 private:
     tm_internal::TimerTaskAtomicPtr initiatingTask;
-    tm_internal::TmAtomicBool locked;
     volatile uint8_t count;
+    tm_internal::TmAtomicBool locked;
 
 public:
     /**
-     * Construct a reentrant yielding lock that is designed for use within task manager tasks
+     * Construct a lock that represents this object
      */
-    ReentrantYieldingLock() {
+    SimpleSpinLock() {
         initiatingTask = nullptr;
         locked = false;
         count = 0;
@@ -36,12 +36,18 @@ class ReentrantYieldingLock {
         spinLock(0xFFFFFFFFUL);
     }
 
+    /**
+     * tries the lock and returns immediately if it was already locked by us.
+     * @return true if the lock was already owned by us, otherwise false
+     */
+    bool tryLock();
+
     /**
      * Attempt to take the lock using a spin wait, it only returns true if the lock was taken.
-     * @param micros micros to wait
+     * @param number of iterations to wait, each iteration is at least 100 mics
      * @return true if the lock was taken, otherwise false
      */
-    bool spinLock(unsigned long micros);
+    bool spinLock(unsigned long iterations);
 
     /**
      * Release the lock taken by spinlock or lock. DOES NOT check that the callee is correct so use carefully.
@@ -55,22 +61,24 @@ class ReentrantYieldingLock {
 
 /**
  * A wrapper around the task manager locking facilities that allow you to lock within a block of code by putting
- * an instance on the stack, for example:
+ * an instance on the stack. Be very careful not to use yieldForMicros along with this method of locking.
+ * For example:
  *
  * ```
- * ReentrantYieldingLock myLock;
+ * SimpleSpinLock myLock;
  * void myFunctionToLock() {
  *     TaskSafeLock(myLock);
  *     // do some work that needs the lock here. lock will always be released.
+ *     // never use yieldForMicros(..) here or your code may deadlock.
  * }
  * ```
  */
 class TaskMgrLock {
 private:
-    ReentrantYieldingLock& lock;
+    SimpleSpinLock& lock;
 
 public:
-    TaskMgrLock(ReentrantYieldingLock& theLock) : lock(theLock) {
+    TaskMgrLock(SimpleSpinLock& theLock) : lock(theLock) {
         lock.lock();
     }
 

src/TaskManagerIO.cpp

@@ -209,22 +209,22 @@ void TaskManager::runLoop() {
 	// these until the first one that isn't ready.
 	TimerTask* tm = tm_internal::atomicReadPtr(&first);
 
-    while (tm && tm->isReady()) {
-
-        // by here we know that the task is in use. If it's in use nothing will touch it until it's marked as
-        // available. We can do this part without a lock, knowing that we are the only thing that will touch
-        // the task. We further know that all non-immutable fields on TimerTask are volatile.
-        tm_internal::atomicWritePtr(&runningTask, tm);
-        tm->execute();
-        tm_internal::atomicWritePtr(&runningTask, nullptr);
-        removeFromQueue(tm);
-        if (tm->isRepeating()) {
-            putItemIntoQueue(tm);
-        } else {
-            tm->clear();
-            tm_internal::tmNotification(tm_internal::TM_INFO_TASK_FREE, TASKMGR_INVALIDID);
+    while (tm && tm->microsFromNow() == 0) {
+        if(!tm->isRunning()) {
+            // by here we know that the task is in use. If it's in use nothing will touch it until it's marked as
+            // available. We can do this part without a lock, knowing that we are the only thing that will touch
+            // the task. We further know that all non-immutable fields on TimerTask are volatile.
+            tm_internal::atomicWritePtr(&runningTask, tm);
+            tm->execute();
+            tm_internal::atomicWritePtr(&runningTask, nullptr);
+            removeFromQueue(tm);
+            if (tm->isRepeating()) {
+                putItemIntoQueue(tm);
+            } else {
+                tm->clear();
+                tm_internal::tmNotification(tm_internal::TM_INFO_TASK_FREE, TASKMGR_INVALIDID);
+            }
         }
-        if(microsToNextTask() > 0) break;
         tm = tm->getNext();
 
 #if defined(ESP8266) || defined(ESP32)

tests/taskMgrTests/reentrantLockingTests.cpp

@@ -1,19 +1,27 @@
 
 #include <AUnit.h>
-#include <ReentrantYieldingLock.h>
+#include <SimpleSpinLock.h>
 #include "TaskManagerIO.h"
 #include "test_utils.h"
 
 using namespace aunit;
 
-ReentrantYieldingLock testLock;
+SimpleSpinLock testLock;
 taskid_t runTaskId1;
 taskid_t runTaskId2;
 
 bool task1RunningPtrCheck = false;
 bool task2RunningPtrCheck = false;
+bool task3RunningPtrCheck = false;
+bool allGood = false;
+
+int runCount1, runCount2, runCount3;
 
 test(testGettingRunningTaskAlwaysCorrect) {
+    task1RunningPtrCheck = false;
+    task2RunningPtrCheck = false;
+    runCount1 = runCount2 = runCount3 = 0;
+
     taskManager.reset();
     assertEqual(nullptr, taskManager.getRunningTask());
 
@@ -26,10 +34,12 @@ test(testGettingRunningTaskAlwaysCorrect) {
 
             task1RunningPtrCheck = taskManager.getRunningTask() == taskManager.getTask(runTaskId1);
         }
+        runCount1++;
     });
 
     runTaskId2 = taskManager.scheduleFixedRate(50, [] {
         task2RunningPtrCheck = taskManager.getRunningTask() == taskManager.getTask(runTaskId2);
+        runCount2++;
     }, TIME_MICROS);
 
     serdebugF("Scheduled running task check");
@@ -40,8 +50,49 @@ test(testGettingRunningTaskAlwaysCorrect) {
 
     assertTrue(task1RunningPtrCheck);
     assertTrue(task2RunningPtrCheck);
+    assertMore(runCount1, 30);
+    assertMore(runCount2, 250);
 }
 
 test(testReentrantLock) {
+    taskManager.reset();
+    task1RunningPtrCheck = false;
+    task2RunningPtrCheck = false;
+    task3RunningPtrCheck = false;
+    allGood = true;
+    runCount1 = runCount2 = runCount3 = 0;
+
+    runTaskId1 = taskManager.scheduleFixedRate(1, [] {
+        TaskMgrLock locker(testLock);
+
+        task1RunningPtrCheck = true;
+        task2RunningPtrCheck = false;
+        task3RunningPtrCheck = false;
+        taskManager.yieldForMicros(1000);
+        if(task2RunningPtrCheck || task3RunningPtrCheck) {
+            allGood = false;
+        }
+        runCount1++;
+    });
+
+    runTaskId2 = taskManager.scheduleFixedRate(100, [] {
+        TaskMgrLock locker(testLock);
+        task2RunningPtrCheck = true;
+        runCount2++;
+    }, TIME_MICROS);
+
+    taskManager.scheduleFixedRate(50, [] {
+        TaskMgrLock locker(testLock);
+        task3RunningPtrCheck = true;
+        runCount3++;
+    }, TIME_MICROS);
+
+    taskManager.yieldForMicros(millisToMicros(1000));
 
+    assertTrue(allGood);
+    assertTrue(runTaskId1 != TASKMGR_INVALIDID);
+    assertTrue(runTaskId2 != TASKMGR_INVALIDID);
+    assertMore(runCount1, 10000);
+    assertMore(runCount2, 10000);
+    assertMore(runCount3, 10000);
 }