[hal] Revamp notifiers (#8424)

This changes the HAL notifier interface to:
- Use wpiutil signal objects. This means waiting is done through the
`WPI_WaitObject` API instead of a dedicated function and allows for
higher level code to simultaneously wait on notifiers and other events.
- Interval timers are supported at the HAL layer
- Handlers are now required to acknowledge notifications. This is
invisible to users unless they're directly using the HAL API.
- For interval timers, an overrun count is maintained to detect if the
handler didn't acknowledge

The underlying implementation still uses condition variables for the
actual waiting. In basic testing using this approach seemed to be lower
jitter than timerfd.

Currently, the simulation and systemcore implementations are nearly
identical except for a few additional sim hook bits. This could be
refactored, but keeping them separate may make sense to keep the
systemcore implementation easy to read and reason about, or if we ever
choose to use a different underlying timer implementation on systemcore.

The simulation side API is unchanged in form but does change in
function--waiting for notifiers now only waits for currently running (or
newly signaled) notifiers to acknowledge. To avoid a race condition in
sim stepTiming, users of the low level API must make any alarm updates
(especially for one-shot alarms) prior to acknowledging the previous
alarm.

The only current use of the interval timer feature is the `Notifier`
class. The `TimedRobot` implementation still uses a single notifier and
its own interval timing logic to ensure consistent callback order. Using
separate notifiers for each user-level interval would substantially
increase complexity. `Watchdog` also doesn't use the interval timer, as
it's looking for an amount of time since the last `set` call rather than
a recurring interval time.

To reduce flicker, the sim GUI uses a fade out when a timeout goes from
set to unset.

This fixes tsan for wpilib and commands, and also fixes some spurious
test failures.

wpilibc/src/main/native/cpp/framework/TimedRobot.cpp

@@ -25,6 +25,8 @@ void TimedRobot::StartCompetition() {
   std::puts("\n********** Robot program startup complete **********");
   HAL_ObserveUserProgramStarting();
 
+  bool first = true;
+
   // Loop forever, calling the appropriate mode-dependent function
   while (true) {
     // We don't have to check there's an element in the queue first because
@@ -33,17 +35,25 @@ void TimedRobot::StartCompetition() {
     auto callback = m_callbacks.pop();
 
     int32_t status = 0;
-    HAL_UpdateNotifierAlarm(m_notifier, callback.expirationTime.count(),
-                            &status);
+    HAL_SetNotifierAlarm(m_notifier, callback.expirationTime.count(), 0, true,
+                         &status);
     WPILIB_CheckErrorStatus(status, "UpdateNotifierAlarm");
 
-    std::chrono::microseconds currentTime{
-        HAL_WaitForNotifierAlarm(m_notifier, &status)};
-    if (currentTime.count() == 0 || status != 0) {
+    // Acknowledge previous alarm after setting the next one to avoid a race
+    // against getting the next notifier timeout in HALSIM StepTiming.
+    if (first) {
+      first = false;
+    } else {
+      HAL_AcknowledgeNotifierAlarm(m_notifier, &status);
+      WPILIB_CheckErrorStatus(status, "AcknowledgeNotifierAlarm");
+    }
+
+    if (WPI_WaitForObject(m_notifier) == 0) {
       break;
     }
 
     m_loopStartTimeUs = RobotController::GetFPGATime();
+    std::chrono::microseconds currentTime{m_loopStartTimeUs};
 
     callback.func();
 
@@ -71,8 +81,8 @@ void TimedRobot::StartCompetition() {
 }
 
 void TimedRobot::EndCompetition() {
-  int32_t status = 0;
-  HAL_StopNotifier(m_notifier, &status);
+  HAL_DestroyNotifier(m_notifier);
+  m_notifier = HAL_kInvalidHandle;
 }
 
 TimedRobot::TimedRobot(wpi::units::second_t period)
@@ -81,7 +91,7 @@ TimedRobot::TimedRobot(wpi::units::second_t period)
   AddPeriodic([=, this] { LoopFunc(); }, period);
 
   int32_t status = 0;
-  m_notifier = HAL_InitializeNotifier(&status);
+  m_notifier = HAL_CreateNotifier(&status);
   WPILIB_CheckErrorStatus(status, "InitializeNotifier");
   HAL_SetNotifierName(m_notifier, "TimedRobot", &status);
 
@@ -93,9 +103,7 @@ TimedRobot::TimedRobot(wpi::units::hertz_t frequency)
 
 TimedRobot::~TimedRobot() {
   if (m_notifier != HAL_kInvalidHandle) {
-    int32_t status = 0;
-    HAL_StopNotifier(m_notifier, &status);
-    WPILIB_ReportError(status, "StopNotifier");
+    HAL_DestroyNotifier(m_notifier);
   }
 }
 

wpilibc/src/main/native/cpp/system/Notifier.cpp

@@ -6,13 +6,11 @@
 
 #include <utility>
 
-#include <fmt/format.h>
-
 #include "wpi/hal/DriverStation.h"
 #include "wpi/hal/Notifier.h"
 #include "wpi/hal/Threads.h"
 #include "wpi/system/Errors.hpp"
-#include "wpi/system/Timer.hpp"
+#include "wpi/util/Synchronization.h"
 
 using namespace wpi;
 
@@ -22,8 +20,8 @@ Notifier::Notifier(std::function<void()> callback) {
   }
   m_callback = callback;
   int32_t status = 0;
-  m_notifier = HAL_InitializeNotifier(&status);
-  WPILIB_CheckErrorStatus(status, "InitializeNotifier");
+  m_notifier = HAL_CreateNotifier(&status);
+  WPILIB_CheckErrorStatus(status, "CreateNotifier");
 
   m_thread = std::thread([=, this] {
     for (;;) {
@@ -32,8 +30,7 @@ Notifier::Notifier(std::function<void()> callback) {
       if (notifier == 0) {
         break;
       }
-      uint64_t curTime = HAL_WaitForNotifierAlarm(notifier, &status);
-      if (curTime == 0 || status != 0) {
+      if (WPI_WaitForObject(notifier) == 0) {
         break;
       }
 
@@ -41,19 +38,16 @@ Notifier::Notifier(std::function<void()> callback) {
       {
         std::scoped_lock lock(m_processMutex);
         callback = m_callback;
-        if (m_periodic) {
-          m_expirationTime += m_period;
-          UpdateAlarm();
-        } else {
-          // Need to update the alarm to cause it to wait again
-          UpdateAlarm(UINT64_MAX);
-        }
       }
 
       // Call callback
       if (callback) {
         callback();
       }
+
+      // Ack notifier
+      HAL_AcknowledgeNotifierAlarm(notifier, &status);
+      WPILIB_CheckErrorStatus(status, "AcknowledgeNotifier");
     }
   });
 }
@@ -64,7 +58,7 @@ Notifier::Notifier(int priority, std::function<void()> callback) {
   }
   m_callback = callback;
   int32_t status = 0;
-  m_notifier = HAL_InitializeNotifier(&status);
+  m_notifier = HAL_CreateNotifier(&status);
   WPILIB_CheckErrorStatus(status, "InitializeNotifier");
 
   m_thread = std::thread([=, this] {
@@ -75,8 +69,7 @@ Notifier::Notifier(int priority, std::function<void()> callback) {
       if (notifier == 0) {
         break;
       }
-      uint64_t curTime = HAL_WaitForNotifierAlarm(notifier, &status);
-      if (curTime == 0 || status != 0) {
+      if (WPI_WaitForObject(notifier) == 0) {
         break;
       }
 
@@ -84,13 +77,6 @@ Notifier::Notifier(int priority, std::function<void()> callback) {
       {
         std::scoped_lock lock(m_processMutex);
         callback = m_callback;
-        if (m_periodic) {
-          m_expirationTime += m_period;
-          UpdateAlarm();
-        } else {
-          // need to update the alarm to cause it to wait again
-          UpdateAlarm(UINT64_MAX);
-        }
       }
 
       // call callback
@@ -111,32 +97,29 @@ Notifier::Notifier(int priority, std::function<void()> callback) {
           throw;
         }
       }
+
+      // Ack notifier
+      HAL_AcknowledgeNotifierAlarm(notifier, &status);
+      WPILIB_CheckErrorStatus(status, "AcknowledgeNotifier");
     }
   });
 }
 
 Notifier::~Notifier() {
-  int32_t status = 0;
   // atomically set handle to 0, then clean
   HAL_NotifierHandle handle = m_notifier.exchange(0);
-  HAL_StopNotifier(handle, &status);
-  WPILIB_ReportError(status, "StopNotifier");
+  HAL_DestroyNotifier(handle);
 
   // Join the thread to ensure the callback has exited.
   if (m_thread.joinable()) {
     m_thread.join();
   }
-
-  HAL_CleanNotifier(handle);
 }
 
 Notifier::Notifier(Notifier&& rhs)
     : m_thread(std::move(rhs.m_thread)),
       m_notifier(rhs.m_notifier.load()),
-      m_callback(std::move(rhs.m_callback)),
-      m_expirationTime(std::move(rhs.m_expirationTime)),
-      m_period(std::move(rhs.m_period)),
-      m_periodic(std::move(rhs.m_periodic)) {
+      m_callback(std::move(rhs.m_callback)) {
   rhs.m_notifier = HAL_kInvalidHandle;
 }
 
@@ -145,19 +128,12 @@ Notifier& Notifier::operator=(Notifier&& rhs) {
   m_notifier = rhs.m_notifier.load();
   rhs.m_notifier = HAL_kInvalidHandle;
   m_callback = std::move(rhs.m_callback);
-  m_expirationTime = std::move(rhs.m_expirationTime);
-  m_period = std::move(rhs.m_period);
-  m_periodic = std::move(rhs.m_periodic);
-
   return *this;
 }
 
 void Notifier::SetName(std::string_view name) {
-  fmt::memory_buffer buf;
-  fmt::format_to(fmt::appender{buf}, "{}", name);
-  buf.push_back('\0');  // null terminate
   int32_t status = 0;
-  HAL_SetNotifierName(m_notifier, buf.data(), &status);
+  HAL_SetNotifierName(m_notifier, name, &status);
 }
 
 void Notifier::SetCallback(std::function<void()> callback) {
@@ -166,19 +142,15 @@ void Notifier::SetCallback(std::function<void()> callback) {
 }
 
 void Notifier::StartSingle(wpi::units::second_t delay) {
-  std::scoped_lock lock(m_processMutex);
-  m_periodic = false;
-  m_period = delay;
-  m_expirationTime = Timer::GetFPGATimestamp() + m_period;
-  UpdateAlarm();
+  int32_t status = 0;
+  HAL_SetNotifierAlarm(m_notifier, static_cast<uint64_t>(delay * 1e6), 0, false,
+                       &status);
 }
 
 void Notifier::StartPeriodic(wpi::units::second_t period) {
-  std::scoped_lock lock(m_processMutex);
-  m_periodic = true;
-  m_period = period;
-  m_expirationTime = Timer::GetFPGATimestamp() + m_period;
-  UpdateAlarm();
+  int32_t status = 0;
+  HAL_SetNotifierAlarm(m_notifier, static_cast<uint64_t>(period * 1e6),
+                       static_cast<uint64_t>(period * 1e6), false, &status);
 }
 
 void Notifier::StartPeriodic(wpi::units::hertz_t frequency) {
@@ -186,26 +158,16 @@ void Notifier::StartPeriodic(wpi::units::hertz_t frequency) {
 }
 
 void Notifier::Stop() {
-  std::scoped_lock lock(m_processMutex);
-  m_periodic = false;
   int32_t status = 0;
   HAL_CancelNotifierAlarm(m_notifier, &status);
   WPILIB_CheckErrorStatus(status, "CancelNotifierAlarm");
 }
 
-void Notifier::UpdateAlarm(uint64_t triggerTime) {
+int32_t Notifier::GetOverrun() const {
   int32_t status = 0;
-  // Return if we are being destructed, or were not created successfully
-  auto notifier = m_notifier.load();
-  if (notifier == 0) {
-    return;
-  }
-  HAL_UpdateNotifierAlarm(notifier, triggerTime, &status);
-  WPILIB_CheckErrorStatus(status, "UpdateNotifierAlarm");
-}
-
-void Notifier::UpdateAlarm() {
-  UpdateAlarm(static_cast<uint64_t>(m_expirationTime * 1e6));
+  int32_t overrun = HAL_GetNotifierOverrun(m_notifier, &status);
+  WPILIB_CheckErrorStatus(status, "GetNotifierOverrun");
+  return overrun;
 }
 
 bool Notifier::SetHALThreadPriority(bool realTime, int32_t priority) {

wpilibc/src/main/native/cpp/system/Watchdog.cpp

@@ -11,9 +11,11 @@
 
 #include <fmt/format.h>
 
+#include "wpi/hal/HALBase.h"
 #include "wpi/hal/Notifier.h"
 #include "wpi/system/Errors.hpp"
 #include "wpi/system/Timer.hpp"
+#include "wpi/util/Synchronization.h"
 #include "wpi/util/mutex.hpp"
 #include "wpi/util/priority_queue.hpp"
 
@@ -47,7 +49,7 @@ class Watchdog::Impl {
 
 Watchdog::Impl::Impl() {
   int32_t status = 0;
-  m_notifier = HAL_InitializeNotifier(&status);
+  m_notifier = HAL_CreateNotifier(&status);
   WPILIB_CheckErrorStatus(status, "starting watchdog notifier");
   HAL_SetNotifierName(m_notifier, "Watchdog", &status);
 
@@ -55,18 +57,14 @@ Watchdog::Impl::Impl() {
 }
 
 Watchdog::Impl::~Impl() {
-  int32_t status = 0;
   // atomically set handle to 0, then clean
   HAL_NotifierHandle handle = m_notifier.exchange(0);
-  HAL_StopNotifier(handle, &status);
-  WPILIB_ReportError(status, "stopping watchdog notifier");
+  HAL_DestroyNotifier(handle);
 
   // Join the thread to ensure the handler has exited.
   if (m_thread.joinable()) {
     m_thread.join();
   }
-
-  HAL_CleanNotifier(handle);
 }
 
 void Watchdog::Impl::UpdateAlarm() {
@@ -79,11 +77,10 @@ void Watchdog::Impl::UpdateAlarm() {
   if (m_watchdogs.empty()) {
     HAL_CancelNotifierAlarm(notifier, &status);
   } else {
-    HAL_UpdateNotifierAlarm(
-        notifier,
-        static_cast<uint64_t>(m_watchdogs.top()->m_expirationTime.value() *
-                              1e6),
-        &status);
+    HAL_SetNotifierAlarm(notifier,
+                         static_cast<uint64_t>(
+                             m_watchdogs.top()->m_expirationTime.value() * 1e6),
+                         0, true, &status);
   }
   WPILIB_CheckErrorStatus(status, "updating watchdog notifier alarm");
 }
@@ -95,10 +92,10 @@ void Watchdog::Impl::Main() {
     if (notifier == 0) {
       break;
     }
-    uint64_t curTime = HAL_WaitForNotifierAlarm(notifier, &status);
-    if (curTime == 0 || status != 0) {
+    if (WPI_WaitForObject(notifier) == 0) {
       break;
     }
+    uint64_t curTime = HAL_GetFPGATime(&status);
 
     std::unique_lock lock(m_mutex);
 
@@ -129,6 +126,8 @@ void Watchdog::Impl::Main() {
     lock.lock();
 
     UpdateAlarm();
+
+    HAL_AcknowledgeNotifierAlarm(notifier, &status);
   }
 }