Fixes and improvements (#642)

TactilityFreeRtos/Include/Tactility/Timer.h

@@ -4,6 +4,13 @@
 #include "Thread.h"
 #include "freertoscompat/Timers.h"
 
+#ifdef ESP_PLATFORM
+#include <freertos/semphr.h>
+#else
+#include <semphr.h>
+#endif
+
+#include <atomic>
 #include <functional>
 #include <memory>
 
@@ -36,6 +43,15 @@ private:
     Callback callback;
     std::unique_ptr<std::remove_pointer_t<TimerHandle_t>, TimerHandleDeleter> handle;
 
+    // Set for the duration of a callback invocation. xTimerStop()/xTimerDelete() only prevent
+    // *future* dispatches: if a callback was already dispatched by the timer daemon task, they
+    // return immediately without waiting for it to finish. Callers that stop a timer and then
+    // immediately destroy state the callback reads/writes (e.g. a Service destructing itself
+    // right after stopping its own update timer) can otherwise race an in-flight callback against
+    // that destruction. stop() below spins on this flag so it only returns once no callback is
+    // executing and none can start afterward.
+    std::atomic<bool> callbackRunning {false};
+
     static TimerHandle_t createTimer(Type type, TickType_t ticks, void* timerId, TimerCallbackFunction_t callback) {
         assert(timerId != nullptr);
         assert(callback != nullptr);
@@ -47,10 +63,20 @@ private:
     static void onCallback(TimerHandle_t hTimer) {
         auto* timer = static_cast<Timer*>(pvTimerGetTimerID(hTimer));
         if (timer != nullptr) {
+            timer->callbackRunning.store(true, std::memory_order_release);
             timer->callback();
+            timer->callbackRunning.store(false, std::memory_order_release);
         }
     }
 
+    // Signals a SemaphoreHandle_t (passed as context) from the timer daemon task. Used by stop()
+    // as a barrier: FreeRTOS timer commands are processed FIFO, so queuing this via
+    // xTimerPendFunctionCall right after xTimerStop() guarantees it only runs once the daemon has
+    // drained everything queued ahead of it - including an expiry command that raced the stop.
+    static void onStopBarrier(void* context, uint32_t /*arg*/) {
+        xSemaphoreGive(static_cast<SemaphoreHandle_t>(context));
+    }
+
 public:
 
     /**
@@ -78,13 +104,35 @@ public:
         return xTimerStart(handle.get(), kernel::FREERTOS_MAX_TICKS) == pdPASS;
     }
 
-    /** Stop the timer
-     * @warning If the timer was just triggered, the callback might still be going on after stop() was called
+    /**
+     * Stop the timer. Unlike a bare xTimerStop(), this blocks until any
+     * callback invocation already queued or dispatched by the timer daemon
+     * task at the time of the call has finished running, so it is safe to
+     * destroy state the callback reads or writes as soon as this returns.
+     * @warning Do not call this from within the timer's own callback - it
+     *          would deadlock waiting on itself.
      * @return success result
      */
     bool stop() const {
         assert(xPortInIsrContext() == pdFALSE);
-        return xTimerStop(handle.get(), kernel::FREERTOS_MAX_TICKS) == pdPASS;
+        bool result = xTimerStop(handle.get(), kernel::FREERTOS_MAX_TICKS) == pdPASS;
+        if (result) {
+            // xTimerStop() only queues tmrCOMMAND_STOP - the daemon may not have processed it yet,
+            // and an expiry command already ahead of it in the queue can still dispatch a callback
+            // after this returns. Queuing a pend-function-call barrier right after the stop command
+            // guarantees (FIFO command processing) that it only runs once everything queued ahead
+            // of it - including such an expiry - has been handled.
+            SemaphoreHandle_t barrier = xSemaphoreCreateBinary();
+            assert(barrier != nullptr);
+            if (setPendingCallback(onStopBarrier, barrier, 0, kernel::FREERTOS_MAX_TICKS)) {
+                xSemaphoreTake(barrier, kernel::FREERTOS_MAX_TICKS);
+            }
+            vSemaphoreDelete(barrier);
+        }
+        while (callbackRunning.load(std::memory_order_acquire)) {
+            vTaskDelay(1);
+        }
+        return result;
     }
 
     /**